ci: guard the code-comments rule with a grep in the build job - #202
Conversation
|
CONTRIBUTING.md's "Code comments" section relies on review to hold. This fails the build job on the framing vocabulary instead. Only that vocabulary is matched. Bare `#NNN` is deliberately not matched: issue links are allowed where the thread carries detail the comment cannot, and the pattern would hit every hex colour. Prose docs are exempt by path. One letter of each alternative is bracketed so the workflow file, which holds the pattern, does not match itself. Verified: passes on the tree, catches a planted `Phase 7` and a planted `PLAN.md` reference, does not self-match, and ci.yml still parses. The step is purely additive. Split out of the comment PR so that one stays comment- and docs-only, which is the property its mechanical verification rests on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QAy8brFG9x8fXet3AV9dJE
4f38cf4 to
4db1fea
Compare
🎭 Visual regression results
Details
Skipped testsmobile-chrome › theme.spec.ts › QuantEcon theme — visual regression › without JavaScript › drawer-opens-without-javascript |
There was a problem hiding this comment.
🟡 Changes recommended
The guard scans tests/visual/README.md even though its own comment and CONTRIBUTING.md declare that file exempt, so permitted history in it would trigger a false CI failure (fixable with --exclude=README.md, plus -I to skip binary snapshots).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds a lightweight CI guard that enforces the "Code comments" rule introduced in its base PR (#200): comments should describe what the code does, not narrate project history. A new Build & Typecheck step greps the code tree for framing vocabulary (Phase N, PLAN.md, parity milestone, open question N) and fails the build if any is found. Because this PR is stacked on chore/code-comments, the diff against main also carries all of #200's comment-only rewrites; the sole substantive delta of this PR is the 16-line ci.yml step.
I verified the guard passes on the current tree, the bracket trick prevents the workflow file from matching itself, and the planted-example behavior works as described. The one concrete issue is that the scan does not actually exempt tests/visual/README.md, contradicting both its own comment and CONTRIBUTING.md.
Changes:
- New
ci.ymlstep "No project framing in code comments" that greps a fixed set of paths for framing vocabulary and fails on a match. - Inherited from #200: comment/doc rewrites across
app/,styles/,tests/, workflows, and root config to drop project-history framing. - CONTRIBUTING.md gains the "Code comments" rule (also inherited context) that this guard enforces.
File summaries
| File | Description |
|---|---|
.github/workflows/ci.yml |
The PR's substantive change: adds the grep guard step; also renames the FOUC step. Guard does not exempt tests/visual/README.md despite claiming to. |
CONTRIBUTING.md |
Documents the "Code comments" rule and the "exempt by path" scope the guard is meant to implement. |
styles/quantecon.css, styles/rtl.css, styles/mpl-widget.css, styles/app.css |
Comment rewrites removing Sphinx/phase framing and correcting px/contrast figures (from #200). |
app/* (root.tsx, seo.ts, types.ts, i18n.ts, links.ts, renderers.tsx, revalidate.ts, routes, components) |
Comment-only rewrites dropping phase/issue framing (from #200). |
tests/** (specs, fixtures, unit tests) |
Comment/title/fixture-string rewrites dropping framing; tests/visual/README.md remains prose history (relevant to the guard's exemption gap). |
template.yml, tailwind.config.js, playwright.config.ts, Makefile, .npmrc, plugins/git-metadata.mjs, scripts/relative-css-asset-urls.mjs, workflows |
Comment rewrites removing history framing (from #200). |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The guard contradicted the rule it enforces. CONTRIBUTING.md exempts prose docs by name, "the test suite's own README.md" included, and the step's own comment repeats that -- but the scan recurses through `tests`, so tests/visual/README.md was scanned like any source file. Confirmed by appending a permitted history line to it: the step failed. It carries eight issue references today, so it is exactly the file the exemption is for. --exclude=README.md is scoped to READMEs, not all Markdown: the fixture `.md` files under tests/visual/fixture are still scanned, verified with a planted `<!-- Phase 9 -->`. -I skips the 29 binary PNG snapshots under tests/visual/__snapshots__. Nothing matches this pattern in them today, but grep reports "Binary file ... matches" and exits 0 on a stray byte hit, which would fail the step for no reason -- a pattern earlier in this work did match those PNGs. Verified: passes clean, exempts the README, still catches a planted `Phase 8` in styles/rtl.css and `Phase 9` in a fixture, does not self-match, and ci.yml still parses. Reported by the Copilot reviewer on #202. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QAy8brFG9x8fXet3AV9dJE
Follow-up to #200, which is now merged. This targets
main; the diff is one commit, 16 added lines in.github/workflows/ci.yml, and nothing else changes.Why
#200 added a Code comments section to CONTRIBUTING.md and cleaned the tree to match it. Nothing stops the next contributor reintroducing what it removed, and the point of the rewrite is that this kind of comment goes stale silently. This fails the existing
Build & Typecheckjob on the framing vocabulary instead.What it matches, and what it deliberately does not
over
app styles plugins scripts tests template.yml tailwind.config.js playwright.config.ts .github/workflows.#NNNis not matched. chore(comments): code comments explain the code, not its history #200's rule permits issue links where the thread carries detail the comment cannot —QuantEcon/mystmd#50,#126,#130,#172,#117are all kept on purpose. The pattern would also hit every hex colour instyles/quantecon.css, so it would be noise rather than a guard.README.md,docs/,PLAN.mdandtests/visual/README.mdare written for a reader who wants the project's history and keep it — the scope CONTRIBUTING now states.Phas[e],PLA[N]) so this workflow file, which contains the pattern, is not itself a match. The scan covers.github/workflows, so without that the guard fails on itself — it did on the first attempt.Verification
mainas merged./* Phase 8 */instyles/rtl.css, and previously a planted// see PLAN.md item 3inapp/revalidate.ts.ci.ymlstill parses as YAML, and the step is purely additive — no existing line changed.Unlike the earlier stacked version of this PR, CI now runs against it:
ci.ymltriggers onpull_request: branches: [main], so while this targetedchore/code-commentsthe guard could not exercise itself.🤖 Generated with Claude Code
https://claude.ai/code/session_01QAy8brFG9x8fXet3AV9dJE