Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new test helper tree() constructs SVG override paths using invalid Path + str concatenation, which will raise a TypeError if the svgs override mechanism is exercised.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a CI gate to ensure the README’s generated SVG diagrams under assets/readme/ stay byte-identical to the generator output and that README.md’s alt text continues to name every identifier-like component actually drawn in the diagrams. This extends the existing “repository hygiene” audits by making the diagram generator the enforced source of truth and preventing silent drift between diagrams and prose.
Changes:
- Wire a dedicated venv (installing pinned Pillow) into
repository-auditsto run the diagram audit and its unit tests. - Add
.github/scripts/check_readme_diagrams.pyto regenerate the SVGs and validate: generator parity, light/dark<title>/<desc>parity, and READMEaltcoverage of drawn component labels. - Add
.github/scripts/test_check_readme_diagrams.pyto cover failure modes (missing variants, malformed SVG, alt drift, hand edits, etc.).
File summaries
| File | Description |
|---|---|
| .github/workflows/ci.yml | Adds venv setup + runs the README diagram audit and its tests in the repository audit job. |
| .github/scripts/check_readme_diagrams.py | Implements the diagram/prose gate by parsing README, extracting SVG metadata/text, and comparing against regenerated outputs. |
| .github/scripts/test_check_readme_diagrams.py | Adds unit tests using a stub generator and a temporary repo tree to validate audit behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The four SVGs under assets/readme/ are generated by build_readme_svg.py, but
nothing ran the generator in CI. A hand-edited SVG stayed committed, and a
composition change pushed without regenerating both themes went unnoticed --
so the generator was the source of truth only by convention. README.md then
describes those pictures a third time, in alt text written independently of
each SVG's own <desc>, and a box renamed in the diagram left both descriptions
asserting a component that is no longer drawn.
check_readme_diagrams.py checks three things:
- each committed SVG is byte-identical to what the generator produces, so a
diagram can only be changed by changing the composition
- a light and dark pair share one <title> and <desc>, since they come from
one composition and a divergence means one was edited by hand
- every component the diagram draws is named in the alt text README gives
it, so a renamed box cannot leave the page describing the old one
The component list is derived, not hard-coded: a drawn label counts when it
reads as an identifier rather than as prose (dotted PascalCase or snake_case),
which picks out the six architecture components and nothing on the hero. A box
added later is covered without anyone remembering to add it here. Only <text>
elements are read, so the alt text is compared against what is drawn rather
than against other prose.
Deliberately not checked: that alt text and <desc> are worded the same. They
are independent descriptions on purpose -- the alt text is longer, and the
<desc> names the three assemblies by role -- and forcing them together would
rewrite published accessibility text to satisfy a gate rather than a reader.
The audit regenerates the SVGs, so it needs the generator's Pillow dependency.
This is the first Python dependency in CI, pinned to an exact version and
installed into a venv under RUNNER_TEMP, because a later step in the same job
parses every JSON and YAML file in the tree.
Verified: the audit passes on this tree; hand-editing an SVG, renaming a drawn
component and regenerating, and editing one variant's <desc> each fail with
the matching message; its 17 tests pass, as do the five sibling audits; the
scripts run in a clean venv holding only Pillow; ci.yml parses.
Signed-off-by: Furroxide <221987073+Furroxide@users.noreply.github.com>
03048f9 to
12a88f9
Compare
|
Unity PR artifacts are ready.
Workflow run: Unity Source Validation #258 Artifacts are temporary and expire according to the workflow retention policy. |
`tree(**svgs)` was never passed by any test, and would have raised TypeError if it had been: `root / "assets" / "readme" / name + ".svg"` concatenates a str onto a Path, because `/` binds tighter than `+`. Removed rather than repaired. Every test states its drift through `edit` for a committed SVG or `generator` for the composition, so the parameter had no caller to serve -- and an untested branch in the fixture is the same defect the audits it covers exist to prevent. Verified: 17 tests still pass and the audit passes on this tree. Signed-off-by: Furroxide <221987073+Furroxide@users.noreply.github.com>
|
Reviewed the resolved Path/string finding and reran the exact-head audit at 78f4b1d. The unused **svgs override was removed and edited SVG paths use the Path-join helper. All 17 regression tests pass, including the actual repository audit; all 28 hosted checks on that head passed. No further source change is needed for the existing review comments. |
Why
The four SVGs under
assets/readme/are generated byassets/readme/source/build_readme_svg.py, but nothing ran the generator in CI. A hand-edited SVG stayed committed, and a composition change pushed without regenerating both themes went unnoticed — so the generator was the source of truth only by convention. #93 named this as the natural follow-up and deliberately left it out.README.mdthen describes those pictures a third time, inalttext written independently of each SVG's own<desc>. A box renamed in the diagram left both descriptions asserting a component that is no longer drawn, and nothing noticed.What changed
.github/scripts/check_readme_diagrams.pychecks three things:<title>and<desc>. They come from one composition, so a divergence means one variant was edited by hand.alttextREADME.mdgives it, so a renamed box cannot leave the page describing the old one.The component list is derived rather than hard-coded: a drawn label counts when it reads as an identifier rather than as prose (dotted PascalCase or snake_case). That picks out the six architecture components —
TopiaForge.ModManager,ModManager.Core,Mods.Abstractions,Mods.UnityUi,launcher_data,launcher_domain— and nothing on the hero, whose text is all prose and command lines. A box added later is covered without anyone remembering to add it here.Only
<text>elements are read, never<title>/<desc>, so the check compares the alt text against what is actually drawn rather than against other prose.Deliberately not checked
That
altand<desc>are worded the same. They are independent descriptions on purpose — thealttext is the longer one, and the<desc>describes the three assemblies by role rather than by name — so requiring them to match would rewrite published accessibility text to satisfy a gate rather than a reader.A note on the dependency
The audit regenerates the SVGs to compare them, so it needs the generator's own Pillow dependency. This is the first Python dependency in CI: every audit so far is stdlib-only. It is pinned to an exact version and installed into a venv under
$RUNNER_TEMP— not the workspace, because a later step in the same job parses every JSON and YAML file in the tree. Hash-pinning was considered and skipped: it would tie the job to a specific wheel for a specific Python minor, and break on a runner-image bump rather than on a real change.Verification
<desc>.ci.ymlparses, andcheck_topiaforge_residue.pypasses.Relationship to #98
Independent of it, and mergeable in either order. Both add steps to the same
repository-auditsjob, but at anchors far enough apart that they do not conflict — verified withgit merge-tree, which merges clean and produces aci.ymlthat parses with all five steps present.This PR was briefly stacked on #98's branch. That was a mistake:
ci.ymlonly triggers for pull requests targetingmain,dev, orrelease/**, so CI never ran on it. Rebased ontodevso it is actually gated.