ci: gate merges on the documentation validators - #255
Conversation
Merge Checks ran four jobs and none of them read `docs/`, so every defect the installed `living-project-documentation` validators catch — an unindexed document, a link broken by a rename three documents away, a spec whose vocabulary never reached the glossary, a malformed decision filename, a half-written supersede chain — reached `main` unless a reviewer happened to notice it. Each is invisible in review precisely because the file the author touched looks fine. `scripts/check-docs.mjs` discovers the validators at their installed path and runs each against `docs/`, streaming their own output. It exits 2 rather than 0 for the two ways this gate would otherwise pass while checking nothing: a skill renamed or removed upstream leaves the validator list empty, and an absent `docs/index.md` makes every validator exit 0 with "Nothing to check" under its own two-level opt-in. The Documentation job annotates that case apart from a documentation finding and re-raises the runner's exit code. The job runs no `npm ci`. The validators import only the Node standard library and `npm run` executes a script without an install, so the gate costs checkout plus Node setup and little else. All five validators pass as things stand, so the gate lands green and the first thing it catches is a regression. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DvYB79RNQgyuCd2jD9DboQ
|
@claude review Generated by Claude Code |
🚀 Preview deploymenthttps://btnopen-pr-255.vercel.app Deployed |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code review1 important, 0 nits
Mandatory checks (per REVIEW.md)
This reviewer is advisory and does not gate merges. |
Node exits 1 on an uncaught exception, and 1 is also how a validator reports findings under `docs/`. A bug inside a validator therefore reached the aggregator as a documentation defect, and the Documentation job annotated it as one — blaming `docs/` for a failure `docs/` did not cause. Reproduced by throwing from a validator's `run()`: the gate reported "4/5 passed, 1 reported findings" and exited 1. The validators ship with an installed skill and are not editable here, so the two cases are separated from outside. `validator-crash-guard.mjs` is preloaded into each validator with `node --import` and ends a throw with an exit code no validator uses, which the aggregator then counts as a check that could not run rather than as a verdict. The guard changes nothing else: a pass still exits 0, findings 1, and a bad invocation 2, each verified against the installed validators. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DvYB79RNQgyuCd2jD9DboQ
|
@claude review Generated by Claude Code |
🚀 Preview deploymenthttps://btnopen-pr-255.vercel.app Deployed |
Code review — round 2 (on 6ad9975)0 important, 1 nit Reviewed the two commits landed since the round-1 review, on top of the full diff. Round-1 fix verifiedReproduced the round-1 finding directly against this branch: injected a
Mandatory checks (per REVIEW.md)
This reviewer is advisory and does not gate merges. |
The aggregator carried its own copy of the guard's exit code, and two copies of a wire constant drift. Importing the guard to share it would be worse than the duplication: importing that module runs it, so the aggregator would install the validators' crash handlers on itself and answer its own crash with a validator's exit code. So the code and its explanation stay in the guard, which is the only side that needs either. The aggregator already treats any exit outside 0 and 1 as a check that reached no verdict, which is all it has to know, and the guard now names the validator it was preloaded into. Its own crash gets the same treatment one level up: Node would exit 1 from an uncaught exception here too, and the workflow reads 1 as findings under `docs/`. It exits 2 instead, so a broken gate is never reported as a documentation defect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DvYB79RNQgyuCd2jD9DboQ
|
@claude review Generated by Claude Code |
🚀 Preview deploymenthttps://btnopen-pr-255.vercel.app Deployed |
Code review — round 3 (on 02848fb)0 important, 0 nits Reviewed the full diff fresh, including the two commits landed since round 2, in a local worktree of Round-2 fix verifiedReproduced the scenario directly: Also independently re-verified, by actually running the commands rather than reading the code:
No new issues found in the round-3 commit or elsewhere in the diff. Mandatory checks (per REVIEW.md)
This reviewer is advisory and does not gate merges. |
`package.json` no longer wraps `scripts/check-docs.mjs`, and the Documentation job runs the file itself. The README command table documents it the way it already documents `npx payload generate:importmap` and `npx payload generate:types` — invoked directly, with the table still the authoritative list of this repository's commands. Nothing now stands between the workflow step and the gate, so the exit code the step branches on is the runner's own rather than one relayed by a package manager. Verified with `npm` absent from PATH: 0 exits clean, 1 takes the findings annotation, 2 takes the gate-broken one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DvYB79RNQgyuCd2jD9DboQ
🚀 Preview deploymenthttps://btnopen-pr-255.vercel.app Deployed |
|
@claude review Generated by Claude Code |
Code review — round 4 (on dbd6cb3)0 important, 0 nits Reviewed the full diff fresh in a local worktree of Round-3-to-round-4 deltaThe only commit since round 3 ( Independently re-verified, by running the commands rather than reading the code
No new issues found in the round-4 commit or elsewhere in the diff. Mandatory checks (per REVIEW.md)
This reviewer is advisory and does not gate merges. |
🚀 Preview deploymentTorn down — the isolated Turso database ( |
2 participants
Summary
Merge Checks ran four jobs —
lint,typecheck,test,payload-artifacts— and none of them readdocs/. That left this repository's 40 hand-written documents with no mechanical gate at all, while the installedliving-project-documentationskill already ships five validators written for exactly that corpus. Each of them catches a defect that is invisible in review precisely because the file the author touched looks fine: a document nobody added toindex.md, a link broken by a rename three documents away, a spec whose vocabulary never reached the glossary, a decision filename with an unreal date, a supersede chain left half-written. The repository has already run three renames of the kind these protect against (#228, #240, #244).This adds a fifth Documentation job that runs all five against
docs/, andnode scripts/check-docs.mjsso a contributor runs exactly what CI runs. The runner is invoked directly rather than wrapped in an npm script — @axross's call, and the README command table already documentsnpx payload generate:importmapandnpx payload generate:typesthe same way, so it stays the authoritative list of commands. Nothing stands between the workflow step and the gate, so the exit code the step branches on is the runner's own rather than one relayed by a package manager.Why a project-owned runner rather than the shell loop the skill suggests. A gate over this corpus has four ways to pass, or to blame the wrong thing, that a one-liner has no room to close:
Done!while never updating it. A glob matching nothing must not read as five-of-five green.docs/index.mdit exits 0 with "Nothing to check", so installing the skill never turns red adocs/holding something else. This repository has adopteddocs/.docs/for it.scripts/validator-crash-guard.mjs, preloaded withnode --import, ends a throw outside the 0/1/2 a validator can meanmain()is wrapped to exit 2The last two were found by the independent review; both threads are resolved.
scripts/check-docs.mjstherefore exits 0 on pass, 1 on findings, and 2 when the gate could not run as intended — and the workflow step annotates 1 and 2 apart before re-raising the code, so a broken gate never reads as a documentation problem or vice versa.Trade-offs. The job reaches an installed skill by path, which couples CI to a generated artifact. That coupling is the point — invoking the installed path is what keeps the validators current across refreshes, and vendoring copies would stop tracking upstream — but it means a future rename breaks CI. The exit-2 guard makes that break loud instead of silent, and
docs/operations/agent-skills.mdnow records the obligation to repoint the runner in the same change. The job runs nonpm ci: the runner and the validators it spawns import only the Node standard library.Related issues
Closes #246
Verification
Every command below was run from the repository root, re-verified at
dbd6cb3. Implementation ran in a delegated worker; every result here was re-run and confirmed independently before each push, rather than taken from the worker's report.npm run formatnpm run lintnpm run typechecknode scripts/check-docs.mjsA check that has only ever passed is not evidence of anything, so each condition was broken in turn, observed, and reverted. Nothing below reached a commit;
git statuswas clean after each.conventions/routing.mdentry removed fromdocs/index.mdcheck-indexcheck-referencescheck-glossarycheck-decision-namingstatusflipped whilesuperseded_bystayedcheck-decision-supersededocs/index.mdmoved aside(ENOENT)throwinjected intocheck-index.mjs'srun()throwinjected into the runner's ownmain()try/catchAlso confirmed:
bash -e(GitHub's default shell) withnpmabsent fromPATHentirely. Therun:body was extracted verbatim from the edited YAML and executed against a stubnodereturning each code: 0 → exit 0, no annotation; 1 → the findings annotation, exit 1; 2 → the gate-broken annotation, exit 2. Then run again against the realnodeon the real tree: exit 0.check-index.mjs docs extra) → 2, unadopted docs root → 0. Both crash shapes are covered — a synchronous throw during module evaluation and a rejected top-levelawaitinsidemain().node_modules/and the runner exited 0 there.package.jsoncarries no script for it, and nocheck:docsreference remains anywhere in the repository.docsjob carriesname: Documentation,actions/checkout@v7andactions/setup-node@v7(matching the sibling jobs, both GitHub's own actions left on major tags perdocs/conventions/security.md), nonpm cistep, and nocachekey. The workflow-levelon:gives it the same pull-request and push-to-maintriggers as the other four.Acceptance criteria
docs/, on PRs and pushes tomaindocs/index.mdexits 2mainas it standsnode scripts/check-docs.mjsdocumented in the README command table; the counting sentence names it;package.jsongains no scriptREVIEW.md's Do Not Report list names the new jobnpm ci, wall-clock below each existing jobdbd6cb3: Documentation 12s, Lint 40s, Unit Tests 45s, Typecheck 51s, Payload Artifacts 52snpm run lintandnpm run typecheckpassRisks and breaking changes
No breaking change: the gate lands green, so the first thing it can catch is a regression.
npm run lintchecks nothing under.github/(recorded indocs/conventions/security.md), so the workflow's correctness rests on the YAML parse and thebash -estep simulation above rather than on the repository's own gate. GitHub's own expression and schema validation is exercised by this PR's own Merge Checks runs, which are green.docs/operations/agent-skills.md, but it is a real new coupling.npm run. With no npm script,npm runno longer lists the command; the command table is where a contributor finds it, which is what makes keeping that table current load-bearing here.docs/conventions/directory-structure.mdalready requires consulting the README before changing verification commands.docsjob; nothing else depends on it.Notes for reviewers
Start at
scripts/check-docs.mjs— the exit-code contract is the load-bearing part, and everything else follows from it.scripts/validator-crash-guard.mjsis the other half of that contract and is not run directly.The plan revision moved. Issue #246 originally required exposing the run as an npm script; @axross directed direct invocation instead on 2026-08-19, and the issue's plan now records that at revision
sha256:d45b755c8f31b8df. Check the acceptance criteria against the current plan, not the archived original description.Two decisions were settled at the plan-approval gate and are not open questions here:
skills-lock.json-to-directory assertion is out of scope. Issue Gatedocs/in Merge Checks by running the five living-project-documentation validators #246 raised it as an open question; @axross settled it as out of scope on 2026-08-19. The runner's missing-validators guard covers the one path this job actually depends on.One pre-existing inaccuracy was noticed and deliberately left alone as out of scope:
REVIEW.mdcallsnpm run test:unit"the Jest unit-test run" when the runner has been Vitest since #230. It sits in a paragraph this diff touches, so it is visible in the diff without being part of the change — happy to fix it here or in a separate pull request, whichever you prefer.