chore: read CI only at merge, not at the end of the review cycle by vivek7405 · Pull Request #1316 · webjsdev/webjs

vivek7405 · 2026-08-07T05:17:01Z

Summary

The pre-merge review cycle used to end by reading CI, and that read bought nothing. The merge gate already reads every check and fixes what is red before merging, so a failure is caught there whatever the ready-to-merge report said. Reading it earlier only parked the finished cycle on a multi-minute CI run.

CI is now read in exactly one place, the merge gate. Four lines in .claude/skills/webjs-start-work/SKILL.md drop the earlier read:

the ready-to-merge condition no longer includes a green CI read
the keep-the-cycle-fast rule now says CI is not read during the cycle or at the end of it
the end-of-cycle batch no longer launches a background CI watch
the report preamble no longer lists the CI read as a precondition

The local suites are untouched. Every layer the Definition of done demands still runs at the end of the cycle and still gates the flip to ready for review.

What actually enforces the gate

Worth stating because the first draft of this PR got it wrong: branch protection mechanically refuses a merge only for the required contexts, and main requires five of the ten jobs ci.yml defines. The merge gate therefore rests on the instruction to read every check, not on protection alone. This PR does not weaken that, since the read it removes was an instruction too and protection covered the same subset before and after, but the skill now says so plainly, including that a green required set is not the same as green CI.

The cost, recorded on purpose

Every ci.yml job with no counterpart in the deferred local suites now fails for the first time at merge rather than before the ready signal. That is the same work done later rather than work skipped, and it is the trade being made deliberately.

The skill states that as a class and refuses to write down its membership. Three attempts to enumerate it in that paragraph were each wrong, in one direction or the other, because deciding membership means holding ci.yml and the deferred set side by side and checking a non-obvious local counterpart per job. That is a derivation, and a derivation frozen into prose is wrong as soon as either side moves. The skill says to derive it at the point of use, and the guard test pins that instruction.

Test plan

test/hooks/review-loop-exit.test.mjs gains CI is read only at the merge gate, never at the end of the cycle: positive assertions on the do-not-restore note, the class phrasing, the do-not-enumerate instruction, and the two branch-protection sentences, plus a negative assertion for each of the four instructions that used to make the cycle wait on CI, plus the two merge-gate paragraphs the removal leans on
Counterfactual re-proven at 4f236a8a, after the three later commits that changed the assertion set: restoring SKILL.md to its origin/main state reds that test and only that test (7 pass, 1 fail); restoring the branch state returns 8 pass
CI appears inside the review cycle only as the statement that it is NOT read there (SKILL.md:347) and in the no-machinery-tells rule about review prose (SKILL.md:256). The only place the cycle READS it is the merge gate
The merge gate and the --admin prohibition are unchanged in force
The new prose passes .claude/hooks/block-prose-punctuation.sh

Other layers: N/A because this changes an agent-workflow document and its static guard, with no runtime surface. Browser, e2e, and Bun have nothing to observe.

Docs surfaces: N/A because the wording exists in this one file. .agents/, AGENTS.md, framework-dev.md, packages/cli/templates/, and the docs site carry no copy of it. This is an agent-workflow doc, not public API surface.

Dogfood: N/A because no framework source changed.

The review cycle blocked its own ending on a CI read that branch protection already enforces at merge. A red check cannot merge no matter what the ready-to-merge report claimed, so reading CI earlier never changed whether a failure got fixed, only when the cycle got to finish. It parked every PR on a multi-minute CI run at the exact moment the cycle was trying to end. The local suites still run at the end of the cycle and still gate the flip to ready for review. Only the CI read moved. The accepted cost is recorded on the merge gate so it is not read as a gap and restored: the conventions job, an npm ci lockfile desync, and the nvim vendored-intellisense drift guard now surface at merge instead of before the ready signal. Closes #1315

Two fixes from review. The do-not-restore note claimed the nvim vendored-intellisense drift guard now surfaces at merge. It does not: scripts/run-node-tests.js walks packages/editors/<sub>/test/, so vendor-sync.test.mjs runs in the full Node suite the cycle already ends on, which is what L151 has said all along. The note contradicted it and inflated the accepted cost from two items to three. Only the narrow not-staged variant escapes a local run, so that is what it says now. The section pins its other two removals in review-loop-exit.test.mjs with a positive assertion on the do-not-restore note and a negative one on the removed mechanism. This removal had neither, which left the one thing the note exists to prevent as the only unenforced removal in the section.

vivek7405

Read the whole diff. The removal itself is right, and the reasoning holds up: branch protection is the real gate, so an earlier read never changed whether a red check got fixed, only when the cycle got to stop.

Two problems, both in the part meant to keep the removal from being undone.

The cost list overstates itself by one item, and it is the item a future reader is most likely to act on, since the whole paragraph exists to say the removal is not a gap. And the removal ships unpinned: the two peer removals in that section each carry a test, this one carries none, so the one thing the note is trying to prevent is the only removal here with nothing enforcing it.

The cost list was wrong in both directions across two attempts. First it was inflated by the nvim drift guard, which the local Node suite does cover. Correcting that closed it at two items and made it wrong the other way: it omitted the Build (@webjsdev/core dist) job, which the paragraph two above names as one of the five required checks and which nothing local runs, plus the in-repo app tests that scripts/run-node-tests.js never discovers, the Postgres round-trip, and the Docker image build. An enumeration in prose cannot stay true as jobs are added, so it now names the class (every ci.yml job with no local counterpart), points at the workflow file for membership, and gives examples marked as examples. Also drops an overclaim that attributed the nvim guard's local coverage to L151, which says it fails the CI job and nothing about the local suite. The support is the run-node-tests.js walk, which is what it cites now.

vivek7405

Reviewed the fix commit and traced it out across the file. The correction to the nvim item is right, but fixing an inflated list by closing it at two made it wrong the other way, and that is the finding that matters here. Also an attribution error and a comment miscount that came in with the same commit.

One finding I am not treating as a change to this PR: the removal conflicts with a standing note of mine that says to check CI before marking a PR ready. That note predates this decision and is superseded by it, so I updated the note rather than the skill.

Third attempt at this paragraph, and the first two were both wrong. The first named the nvim drift guard and the Build (@webjsdev/core dist) job as jobs with no local counterpart. Neither is: run-node-tests.js walks packages/editors/<sub>/test/, and test/packaging/build-dist.test.js shells the same scripts/build-framework-dist.js that build:dist runs, so a bundling break reds in the full Node suite. The second closed the list at the survivors and read complete while omitting E2E (blog served on Bun), which genuinely is a member, since run-bun-tests.js excludes the e2e segment and the deferred e2e arm is Node-served. A membership list in prose is wrong in one direction or the other, and it is load-bearing here because the trade only holds if the membership is right. So the paragraph names the class, says to derive it from ci.yml against the deferred set, and says outright not to write the membership down. The two failed attempts stay recorded as the reason.

vivek7405

Third read of this paragraph and the second time a fix to it introduced a new error. The pattern is the point: every attempt to write down WHICH ci.yml jobs lack a local counterpart has been wrong, in one direction or the other, and each wrong version was load-bearing because the trade only holds if the membership is right.

So the list is gone rather than corrected again. The paragraph names the class, says to derive membership from ci.yml against the deferred set, and says outright not to write it down here. Both failed attempts stay recorded as the reason, and the guard test pins the do-not-enumerate instruction.

The note recounted which earlier attempt got which job wrong, and the recounting was itself wrong: it credited the first attempt with naming Build (@webjsdev/core dist), which the third named, and pinned the e2e-on-Bun omission on the second, which omitted four other jobs too. It also said two attempts where the guard test said three, and three is right. The history was never the durable content. What a future reader needs is the instruction and why it holds, so the paragraph now says the membership is a derivation over ci.yml and the deferred set, which is wrong the moment either side moves, and stops narrating the attempts. The count now matches the guard test.

vivek7405

All three findings are about the same thing: the paragraph narrated which earlier attempt got which job wrong, and the narration was wrong. Checked it against the branch history and the reviewer is right on every point. 03c66ca3 named the nvim guard and never mentioned Build; 185437346 was what named Build; 5dc7d19b omitted far more than the Bun e2e job. The count disagreed with its own guard test as well, and three is the right number.

The history was never the durable content, so it is gone rather than corrected a fourth time. What stays is the instruction and the structural reason it holds: membership is a derivation over ci.yml and the deferred set, so freezing it into prose is wrong as soon as either side moves.

The removal's justification said branch protection refuses the merge whatever the report claimed. That is true only for the required contexts, and main requires five of the roughly eleven jobs ci.yml defines. Every job established as a member of the no-local-counterpart class sits in the non-required set, so the sentence was false for exactly the jobs whose cost it was justifying. Nothing was actually weakened by this PR: the removed end-of-cycle read was an instruction, not a mechanism, and branch protection covered the same subset before and after. But the merge gate rests on the instruction to read every check, so the paragraph says that, and says outright that a green required set is not green CI.

vivek7405

The first finding is the serious one and it is right. The removal was justified on branch protection refusing the merge, and protection covers five required contexts against roughly eleven jobs, with every class member sitting in the non-required set. So the sentence was false for exactly the jobs whose cost it was justifying. Nothing was actually weakened by this PR, since the read it removes was an instruction and protection covered the same subset before and after, but the justification had to stop overstating the mechanism.

The other three are the PR body drifting from the file it describes. The body kept the per-attempt history after the skill dropped it, dated its counterfactual to a commit three revisions stale, and asserted a grep result the grep does not produce.

vivek7405

No must-fix on the branch this round. Two things, and neither changes the source.

The job count in the PR body was wrong (ci.yml defines ten, not eleven), which is a PR-body count and so minor by the rule in this same file. Corrected in the body; the shipped prose says "roughly twice as many", which is exactly right at ten against five.

Browser (web-test-runner / Playwright) is red on the head, from a Firefox Tests were interrupted because the page was reloaded, with all three browsers finishing 69/69 files at zero failures. A markdown paragraph and two regexes in a static node:test file cannot reload a browser page, and the same suite passed on the immediately preceding commit, so this is the known interruption flake rather than the diff. It is a required check, so it blocks the merge and gets a re-run there. Which is exactly where this PR moves that work.

vivek7405

Read the whole diff again on the finished change. Clean.

Worth recording what this cost, because it is the most interesting thing about the PR. The change itself is four deleted clauses and a guard test, and that part has been correct and untouched since the second commit. Every round after it found a defect in the PROSE I wrote around it: an inflated cost list, then a deflated one, then a wrong history of those two mistakes, then a justification that overstated what branch protection enforces. Six rounds, five of them spent on two paragraphs that explain a change nobody was confused about.

The lesson is in the file now: the paragraph states the class, refuses to write down its membership, and says to derive it from ci.yml at the point of use. Prose that encodes a derivation is wrong as soon as either side of the derivation moves, and I proved that four times in a row while writing it.

vivek7405 self-assigned this Aug 7, 2026