Skip to content

Code review · D6 · Docs pipeline in CI (minimal version)

Range: 99826d2..0930f5c on autopilot/D6. The code fixes are in 0930f5c; the commit after it only changes docs. Test command: python3 -m unittest tools/docs/test_check.py (7 tests, OK), then python3 tools/docs/check.py (docs gate: 0 failure(s)).

CODE  step=D6  review=approve b/M/m=0/0/4  security=approve b/M/m=0/0/2  rounds=1  simplify=nochange -0/+0  tests=pass
Pass Round 1 (HEAD d22b4d1)
code-reviewer approve, b/M/m = 0/0/4
security-reviewer approve, 0/0/2
code-simplifier nochange at HEAD a79a72a, tests pass

CR-1 · minor · The unit tests printed ::error:: lines

  • Where: tools/docs/test_check.py, run_check
  • Evidence: the verbose run showed annotation lines between test names.
  • Problem: noisy test output, and in CI the lines would show up as real annotations.
  • Proposal: run the check under contextlib.redirect_stdout(io.StringIO()).
  • Found by: code-reviewer
  • Status: applied (0930f5c).

CR-2 · minor · No workflow ran the new unit tests

  • Where: .github/workflows/docs.yml, job docs-gate
  • Evidence: the tests existed only for local runs.
  • Problem: a regression in check_progress_hook() could pass CI unnoticed.
  • Proposal: a step python3 -m unittest tools/docs/test_check.py before the docs gate.
  • Found by: code-reviewer
  • Status: applied (0930f5c). The step uses no new action.

CR-3 · minor · A full placeholder written anywhere is rendered

  • Where: hooks/progress.py and check_progress_hook()
  • Evidence: the hook replaces the placeholder in raw Markdown, code spans included. A page that quotes the full comment would get a strip or table, and would count as a carrier page.
  • Problem: this rule was not written down.
  • Proposal: say it in the docstring and in the spec.
  • Found by: code-reviewer
  • Status: applied (0930f5c): docstring of check_progress_hook(), and documentation-system.md §3.3 item 1.

CR-4 · minor · Two loops over the files

  • Where: check_progress_hook()
  • Evidence: one loop scans every built HTML page, the other every source page.
  • Problem: the two passes could be merged.
  • Proposal: a single pass.
  • Found by: code-reviewer
  • Status: kept, acceptable noise. The two loops check different things: any page left with a placeholder, and the pages that must show the output.

SEC-1 · minor · A local font could be published with the site

  • Where: mkdocs.yml
  • Evidence: docs/fonts/*.ttf is ignored by version control, but MkDocs copies every file under docs/, so a local Virgil.ttf would go into a locally built and deployed site.
  • Problem: the font's licence is unconfirmed (FONT-LICENSE.md).
  • Proposal: exclude_docs: fonts/*.ttf, and say in FONT-LICENSE.md that OFL.txt covers only the excalidraw/virgil release.
  • Found by: security-reviewer
  • Status: applied (0930f5c). With a dummy docs/fonts/Virgil.ttf present, mkdocs build --strict passed and the site's fonts/ held only OFL.txt. The dummy was then removed.

SEC-2 · minor · Annotation injection through the message

  • Where: tools/docs/check.py, fail()
  • Evidence: messages hold tool output, such as the tail of a failed build. A newline in it ends the ::error command, and the next line can be read as a new workflow command.
  • Problem: a multi-line message was split, and its later lines could be read as workflow commands.
  • Proposal: escape %, CR and LF as %25, %0D and %0A in the message and in the file property.
  • Found by: security-reviewer
  • Status: applied (0930f5c, _escape()), tested by test_newlines_and_percent_are_escaped_in_the_error_command, red before the change.

Outside the diff

  • documentation-system.md §3.3 item 3 says tools/diagrams/sketch.py fails when the font is missing, unless --allow-fallback-font is passed. sketch.py has no such flag (its main_for() takes --no-png and --out), and it falls back to a cursive system font without warning. Not fixed here; left for a follow-up.