From 03c66ca3dcf47317151e283af04e811fd3b3d9bb Mon Sep 17 00:00:00 2001 From: Vivek Date: Fri, 7 Aug 2026 10:46:29 +0530 Subject: [PATCH 1/6] chore: read CI only at merge, not at the end of the review cycle 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 --- .claude/skills/webjs-start-work/SKILL.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 388f1607c..326860e4d 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -237,7 +237,7 @@ All four are written in the owner's voice (first person, plain, no AI/agent fram ## Pre-merge review cycle (MUST run before reporting "ready for merge") -Saying "ready for merge" before the review cycle completes is the single biggest source of low-quality PRs. The recurring pattern to AVOID: claim ready-for-merge, the user requests a review, find issues, fix them, claim ready-for-merge again, repeat 4-5 times before a review comes back clean. The cure is to run that cycle internally BEFORE the first "ready" signal. The user should only hear "ready to merge" after the cycle has finished AND the suites it deferred have run AND CI has been read green. +Saying "ready for merge" before the review cycle completes is the single biggest source of low-quality PRs. The recurring pattern to AVOID: claim ready-for-merge, the user requests a review, find issues, fix them, claim ready-for-merge again, repeat 4-5 times before a review comes back clean. The cure is to run that cycle internally BEFORE the first "ready" signal. The user should only hear "ready to merge" after the cycle has finished AND the suites it deferred have run. ### Every PR review is posted ON the PR (summary + per-line comments) @@ -344,7 +344,7 @@ So a clean or minor-only round 1 finishes the cycle with ONE review. That is the **Do not tell it what to look for.** The prompt sets the SCOPE (which diff) and nothing else: no list of defect classes, no "specifically check for X and Y", no ranking of what matters, however sure you are about where the risk sits. A checklist narrows a fresh reviewer to what you already suspect, which is the bias it exists to escape, and everything outside your list becomes what it does not look at. The one exception is a REFUTER, whose whole job is the single claim it is handed. Naming the touched files is scope; naming the bugs to hunt is steering. -**Keep the cycle fast.** After a fix, run only the test file(s) covering the line you changed, with the counterfactual toggle the Definition of done mandates (a fix can make an older test non-discriminating without failing it). The e2e, full Node, browser, and Bun suites and the two-app dogfood check run ONCE, after the cycle ends. Never wait on CI between rounds; read it once at the end. Both rules change WHEN work happens, never WHETHER. +**Keep the cycle fast.** After a fix, run only the test file(s) covering the line you changed, with the counterfactual toggle the Definition of done mandates (a fix can make an older test non-discriminating without failing it). The e2e, full Node, browser, and Bun suites and the two-app dogfood check run ONCE, after the cycle ends. CI is not read during the cycle at all, and not at the end of it either. It is read once, at merge, under the merge gate below. Both rules change WHEN work happens, never WHETHER. **Do not restore what this replaced.** This cycle used to run a 16-agent fleet with a scout, parallel lenses, and a jury, pick round 1's shape by a path check, sort findings into two tiers, cap itself at five rounds, and poll a file to watch each spawn. Almost all of it is gone on purpose: termination is mostly structural now (only a fix buys a round, delta rounds narrow the question, the minor call stops wording from buying rounds, the final review plus one fix-check is a hard end). Reviews are async, so the harness completion notification is the signal, with at most an optional background progress check that never kills anything. The one exception to the removals is the round cap, which came back in a narrower form, because structure alone cannot bound a chain where every fix produces the next round's finding: it now bounds ONLY that case, at five delta rounds, instead of counting every round of the cycle. @@ -375,7 +375,7 @@ Each round must: **Record every finding ON THE PR**, through the mechanics in `### Every PR review is posted ON the PR` and `### Follow the real review flow`, which are authoritative: one review object per round carrying the summary plus every inline `file:line` finding, each stating the problem only, with the disposition (`fixed in ` / `rejected because ` / `deferred as out of scope because `) in a threaded reply, then the thread resolved. Post rejections and false positives too, so the reasoning is auditable. A round that found nothing posts a short summary saying so. Build the review JSON with a real serializer, never by interpolating into a shell string: a review on #1115 lost every code reference to shell command substitution and had to be reposted. -**When the cycle FINISHES, run everything it deferred:** the full suites for every layer the change touches (e2e, Node, browser, Bun matrix, the two-app dogfood boot check, per the Definition of done), and only now read CI. Launch them as parallel background tasks in one batch, plus a background CI watch, and collect EVERY result before reporting: a task you forget to collect is a silently skipped layer. A cycle that STOPPED unfinished runs none of this and says so in the report, because these gate the flip to ready for review and that flip is not happening. +**When the cycle FINISHES, run everything it deferred:** the full suites for every layer the change touches (e2e, Node, browser, Bun matrix, the two-app dogfood boot check, per the Definition of done). Launch them as parallel background tasks in one batch and collect EVERY result before reporting: a task you forget to collect is a silently skipped layer. A cycle that STOPPED unfinished runs none of this and says so in the report, because these gate the flip to ready for review and that flip is not happening. **A fix is never the end.** A fix changes the branch, so the changed branch needs its own round; that is what the delta rounds are, and why a round with no fixes still buys the final review. Never report "fixed it" or "ready to merge" off a round that found something must-fix, however obviously correct the fix looks. On #1159 three consecutive rounds each found problems introduced by the previous round's fix, which is what a re-used reviewer, already invested in that fix, is worst at seeing. @@ -387,7 +387,7 @@ Skip only for PRs that change a single line of trivially-correct content (a doc ### Reporting after the cycle -After the final review (and its fix-check, if it had one), the deferred suites, and the CI read, report exactly this shape: +After the final review (and its fix-check, if it had one) and the deferred suites, report exactly this shape: > PR # is up at . Reviewed it over rounds plus a final pass over the whole diff; nothing must-fix is left open. Issues found and fixed: . Out-of-scope findings, recorded on the PR and awaiting your call on filing: . Ready to merge. @@ -397,7 +397,9 @@ When anything was deferred, expand each one right there (the finding, its one-se If you cannot honestly say the final review left nothing must-fix open, you cannot say "ready to merge". Mention any finding you rejected as a false positive so the user can second-guess it. Every finding must be accounted for here as fixed, rejected-with-reason, or deferred, and must also appear on the PR, so the report and the PR agree; a deferred finding missing from its thread, this report, or the ledger is a dropped finding. -**Merge is gated on green CI, enforced at the branch level, not by trust.** A PR must not merge until all CI checks pass. `main` branch protection requires the five `ci.yml` checks (Conventions, Unit+integration, Browser, E2E, Build) before any merge; if `gh api repos/webjsdev/webjs/branches/main/protection` shows `required_status_checks: null`, run `bash scripts/protect-main.sh` once (needs repo admin) to restore it. Do not work around a red or pending check; wait for green. +**Merge is gated on green CI, enforced at the branch level, not by trust.** A PR must not merge until all CI checks pass. `main` branch protection requires the five `ci.yml` checks (Conventions, Unit+integration, Browser, E2E, Build) before any merge; if `gh api repos/webjsdev/webjs/branches/main/protection` shows `required_status_checks: null`, run `bash scripts/protect-main.sh` once (needs repo admin) to restore it. Do not work around a red or pending check. Wait for green, and fix whatever is red before merging. + +**This is the ONLY place CI is read, on purpose. Do not add one back to the end of the review cycle.** An end-of-cycle read was removed because it was redundant against this gate: branch protection refuses the merge whatever the report claimed, so a red check gets caught and fixed right here, and reading it earlier only parks the finished cycle on a multi-minute CI run. What that costs is worth stating plainly, because it looks like a gap: three failures surface at merge instead of before the ready signal, namely the `conventions` job over `examples/blog` and `website`, a `package-lock.json` desync failing `npm ci`, and the webjs.nvim vendored-intellisense drift guard. That is the same work done later rather than work skipped, which is the trade that was chosen. The local suites the Definition of done demands still run at the end of the cycle and still gate the flip to ready for review; only the CI read moved. **NEVER use `gh pr merge --admin` to bypass a FAILING check.** `--admin` skips ALL branch-protection gates, not only the review requirement, so a red check merges silently and lands broken code on `main`. This has happened (a Unit-test failure was admin-merged, breaking `main`). It is acceptable ONLY to bypass a required-review gate on a PR whose CI is confirmed all-green, so re-run `gh pr checks ` first and confirm EVERY check reads `pass` (a `BLOCKED` state can mean review-required OR a failing check, so never assume which). From 5dc7d19bc5a07a3d80ab6023f346a1ebe14fa3b2 Mon Sep 17 00:00:00 2001 From: Vivek Date: Fri, 7 Aug 2026 11:12:00 +0530 Subject: [PATCH 2/6] chore: pin the CI-read removal with a test, and correct its cost list 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//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. --- .claude/skills/webjs-start-work/SKILL.md | 2 +- test/hooks/review-loop-exit.test.mjs | 26 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 326860e4d..8af333ae6 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -399,7 +399,7 @@ If you cannot honestly say the final review left nothing must-fix open, you cann **Merge is gated on green CI, enforced at the branch level, not by trust.** A PR must not merge until all CI checks pass. `main` branch protection requires the five `ci.yml` checks (Conventions, Unit+integration, Browser, E2E, Build) before any merge; if `gh api repos/webjsdev/webjs/branches/main/protection` shows `required_status_checks: null`, run `bash scripts/protect-main.sh` once (needs repo admin) to restore it. Do not work around a red or pending check. Wait for green, and fix whatever is red before merging. -**This is the ONLY place CI is read, on purpose. Do not add one back to the end of the review cycle.** An end-of-cycle read was removed because it was redundant against this gate: branch protection refuses the merge whatever the report claimed, so a red check gets caught and fixed right here, and reading it earlier only parks the finished cycle on a multi-minute CI run. What that costs is worth stating plainly, because it looks like a gap: three failures surface at merge instead of before the ready signal, namely the `conventions` job over `examples/blog` and `website`, a `package-lock.json` desync failing `npm ci`, and the webjs.nvim vendored-intellisense drift guard. That is the same work done later rather than work skipped, which is the trade that was chosen. The local suites the Definition of done demands still run at the end of the cycle and still gate the flip to ready for review; only the CI read moved. +**This is the ONLY place CI is read, on purpose. Do not add one back to the end of the review cycle.** An end-of-cycle read was removed because it was redundant against this gate: branch protection refuses the merge whatever the report claimed, so a red check gets caught and fixed right here, and reading it earlier only parks the finished cycle on a multi-minute CI run. What that costs is worth stating plainly, because it looks like a gap. Two failures surface at merge instead of before the ready signal: the `conventions` job over `examples/blog` and `website`, and a `package-lock.json` desync failing `npm ci`. Neither has a local counterpart in the deferred suites. That is the same work done later rather than work skipped, which is the trade that was chosen. The local suites the Definition of done demands still run at the end of the cycle and still gate the flip to ready for review, so only the CI read moved. Do NOT pad that list with the webjs.nvim vendored-intellisense drift guard, which reads as a third item and is not one: `scripts/run-node-tests.js` walks `packages/editors//test/`, so `vendor-sync.test.mjs` runs in the full Node suite the cycle already ends on, and the ordinary miss (edited `packages/editors/intellisense/src` without re-vendoring) reds locally exactly as L151 says. Only the narrow variant escapes, where the re-vendor ran on disk but was never `git add -f`'d, so the working tree the local run reads is in sync while the commit CI checks out is not. **NEVER use `gh pr merge --admin` to bypass a FAILING check.** `--admin` skips ALL branch-protection gates, not only the review requirement, so a red check merges silently and lands broken code on `main`. This has happened (a Unit-test failure was admin-merged, breaking `main`). It is acceptable ONLY to bypass a required-review gate on a PR whose CI is confirmed all-green, so re-run `gh pr checks ` first and confirm EVERY check reads `pass` (a `BLOCKED` state can mean review-required OR a failing check, so never assume which). diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index ce859ac7c..3540190a6 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -162,6 +162,32 @@ test('the removed machinery stays removed, with the reason recorded', () => { } }); +test('CI is read only at the merge gate, never at the end of the cycle', () => { + // The third removal in this section, pinned the same way as the fleet and + // the two-review floor above: the note that records WHY it went, plus the + // counterfactual that the read itself has not crept back. + assert.match(skill, /\*\*This is the ONLY place CI is read, on purpose\. Do not add one back to the end of the review cycle\.\*\*/); + // The cost it accepts must stay stated, or the next reader takes the + // removal for an oversight and restores the read to "fix" it. + assert.match(skill, /Two failures surface at merge instead of before the ready signal/); + + // The three instructions that USED to make the cycle wait on CI. Each is + // gone, and a revert of the hunk that removed it puts its phrasing back. + for (const [label, re] of [ + ['the ready-to-merge condition', /suites it deferred have run AND CI has been read green/], + ['the keep-the-cycle-fast rule', /Never wait on CI between rounds/], + ['the end-of-cycle batch', /and only now read CI|plus a background CI watch/], + ['the report preamble', /the deferred suites, and the CI read, report exactly/], + ]) { + assert.ok(!re.test(skill), `${label} tells the cycle to read CI again`); + } + + // The gate the removal leans on has to stay strict, since it is now the + // only CI checkpoint there is. + assert.match(skill, /\*\*Merge is gated on green CI, enforced at the branch level, not by trust\.\*\*/); + assert.match(skill, /\*\*NEVER use `gh pr merge --admin` to bypass a FAILING check\.\*\*/); +}); + test('the cycle keeps the guarantees the trim was not allowed to touch', () => { // A fix is never the end: the delta round after a fix is what the whole // cycle exists to force. From 185437346ed42351168da30ab75eb1ae229f9a79 Mon Sep 17 00:00:00 2001 From: Vivek Date: Fri, 7 Aug 2026 11:21:23 +0530 Subject: [PATCH 3/6] chore: state the accepted CI cost as a class, not a closed list 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. --- .claude/skills/webjs-start-work/SKILL.md | 4 +++- test/hooks/review-loop-exit.test.mjs | 11 ++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 8af333ae6..ddf544025 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -399,7 +399,9 @@ If you cannot honestly say the final review left nothing must-fix open, you cann **Merge is gated on green CI, enforced at the branch level, not by trust.** A PR must not merge until all CI checks pass. `main` branch protection requires the five `ci.yml` checks (Conventions, Unit+integration, Browser, E2E, Build) before any merge; if `gh api repos/webjsdev/webjs/branches/main/protection` shows `required_status_checks: null`, run `bash scripts/protect-main.sh` once (needs repo admin) to restore it. Do not work around a red or pending check. Wait for green, and fix whatever is red before merging. -**This is the ONLY place CI is read, on purpose. Do not add one back to the end of the review cycle.** An end-of-cycle read was removed because it was redundant against this gate: branch protection refuses the merge whatever the report claimed, so a red check gets caught and fixed right here, and reading it earlier only parks the finished cycle on a multi-minute CI run. What that costs is worth stating plainly, because it looks like a gap. Two failures surface at merge instead of before the ready signal: the `conventions` job over `examples/blog` and `website`, and a `package-lock.json` desync failing `npm ci`. Neither has a local counterpart in the deferred suites. That is the same work done later rather than work skipped, which is the trade that was chosen. The local suites the Definition of done demands still run at the end of the cycle and still gate the flip to ready for review, so only the CI read moved. Do NOT pad that list with the webjs.nvim vendored-intellisense drift guard, which reads as a third item and is not one: `scripts/run-node-tests.js` walks `packages/editors//test/`, so `vendor-sync.test.mjs` runs in the full Node suite the cycle already ends on, and the ordinary miss (edited `packages/editors/intellisense/src` without re-vendoring) reds locally exactly as L151 says. Only the narrow variant escapes, where the re-vendor ran on disk but was never `git add -f`'d, so the working tree the local run reads is in sync while the commit CI checks out is not. +**This is the ONLY place CI is read, on purpose. Do not add one back to the end of the review cycle.** An end-of-cycle read was removed because it was redundant against this gate: branch protection refuses the merge whatever the report claimed, so a red check gets caught and fixed right here, and reading it earlier only parks the finished cycle on a multi-minute CI run. What that costs is worth stating plainly, because it looks like a gap. 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 a CLASS, not a list, so read `.github/workflows/ci.yml` for the current membership rather than trusting an enumeration here, which is exactly the kind of prose that rots as jobs are added. It is a large class: the `conventions` job over `examples/blog` and `website`, the `Build (@webjsdev/core dist)` job (a REQUIRED check, and nothing local runs `npm run build:dist`), the in-repo app tests for `website` and `examples/blog` (whose own suites `scripts/run-node-tests.js` never discovers, the website typecheck among them), a `package-lock.json` desync failing `npm ci`, the Postgres round-trip, and the Docker image build. That is the same work done later rather than work skipped, which is the trade that was chosen, and the trade is a real one rather than a free one. The local suites the Definition of done demands still run at the end of the cycle and still gate the flip to ready for review, so only the CI read moved. + +One near-miss belongs to the local side rather than this class. The webjs.nvim vendored-intellisense drift guard reads like a member and is not one, because `scripts/run-node-tests.js` walks `packages/editors//test/`, so `vendor-sync.test.mjs` runs in the full Node suite the cycle already ends on and the ordinary miss (editing `packages/editors/intellisense/src` without re-vendoring) reds locally. Only the narrow variant escapes, where the re-vendor ran on disk but was never `git add -f`-ed, so the working tree the local run reads is in sync while the commit CI checks out is not. **NEVER use `gh pr merge --admin` to bypass a FAILING check.** `--admin` skips ALL branch-protection gates, not only the review requirement, so a red check merges silently and lands broken code on `main`. This has happened (a Unit-test failure was admin-merged, breaking `main`). It is acceptable ONLY to bypass a required-review gate on a PR whose CI is confirmed all-green, so re-run `gh pr checks ` first and confirm EVERY check reads `pass` (a `BLOCKED` state can mean review-required OR a failing check, so never assume which). diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index 3540190a6..24dc10593 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -168,10 +168,15 @@ test('CI is read only at the merge gate, never at the end of the cycle', () => { // counterfactual that the read itself has not crept back. assert.match(skill, /\*\*This is the ONLY place CI is read, on purpose\. Do not add one back to the end of the review cycle\.\*\*/); // The cost it accepts must stay stated, or the next reader takes the - // removal for an oversight and restores the read to "fix" it. - assert.match(skill, /Two failures surface at merge instead of before the ready signal/); + // removal for an oversight and restores the read to "fix" it. It is stated + // as an open CLASS pointing at ci.yml, never a closed list: the list form + // has already been wrong in both directions on this paragraph, once + // inflated by an item the local suites do cover, once closed at two while + // omitting a REQUIRED check the paragraph above it names. + assert.match(skill, /Every `ci\.yml` job with no counterpart in the deferred local suites/); + assert.match(skill, /That is a CLASS, not a list/); - // The three instructions that USED to make the cycle wait on CI. Each is + // The four instructions that USED to make the cycle wait on CI. Each is // gone, and a revert of the hunk that removed it puts its phrasing back. for (const [label, re] of [ ['the ready-to-merge condition', /suites it deferred have run AND CI has been read green/], From 7f50d465fd0002bd734308c526411b8c4ef8b55f Mon Sep 17 00:00:00 2001 From: Vivek Date: Fri, 7 Aug 2026 11:42:31 +0530 Subject: [PATCH 4/6] chore: stop writing down which CI jobs have no local counterpart 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//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. --- .claude/skills/webjs-start-work/SKILL.md | 6 ++++-- test/hooks/review-loop-exit.test.mjs | 9 +++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index ddf544025..f2131e2a4 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -399,9 +399,11 @@ If you cannot honestly say the final review left nothing must-fix open, you cann **Merge is gated on green CI, enforced at the branch level, not by trust.** A PR must not merge until all CI checks pass. `main` branch protection requires the five `ci.yml` checks (Conventions, Unit+integration, Browser, E2E, Build) before any merge; if `gh api repos/webjsdev/webjs/branches/main/protection` shows `required_status_checks: null`, run `bash scripts/protect-main.sh` once (needs repo admin) to restore it. Do not work around a red or pending check. Wait for green, and fix whatever is red before merging. -**This is the ONLY place CI is read, on purpose. Do not add one back to the end of the review cycle.** An end-of-cycle read was removed because it was redundant against this gate: branch protection refuses the merge whatever the report claimed, so a red check gets caught and fixed right here, and reading it earlier only parks the finished cycle on a multi-minute CI run. What that costs is worth stating plainly, because it looks like a gap. 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 a CLASS, not a list, so read `.github/workflows/ci.yml` for the current membership rather than trusting an enumeration here, which is exactly the kind of prose that rots as jobs are added. It is a large class: the `conventions` job over `examples/blog` and `website`, the `Build (@webjsdev/core dist)` job (a REQUIRED check, and nothing local runs `npm run build:dist`), the in-repo app tests for `website` and `examples/blog` (whose own suites `scripts/run-node-tests.js` never discovers, the website typecheck among them), a `package-lock.json` desync failing `npm ci`, the Postgres round-trip, and the Docker image build. That is the same work done later rather than work skipped, which is the trade that was chosen, and the trade is a real one rather than a free one. The local suites the Definition of done demands still run at the end of the cycle and still gate the flip to ready for review, so only the CI read moved. +**This is the ONLY place CI is read, on purpose. Do not add one back to the end of the review cycle.** An end-of-cycle read was removed because it was redundant against this gate: branch protection refuses the merge whatever the report claimed, so a red check gets caught and fixed right here, and reading it earlier only parks the finished cycle on a multi-minute CI run. What that costs is worth stating plainly, because it looks like a gap. 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 a CLASS, not a list. Its membership moves as jobs are added and as the local suites grow to cover them, so derive it when you need it, by reading `.github/workflows/ci.yml` against the deferred set named above. Do NOT write the membership down here. -One near-miss belongs to the local side rather than this class. The webjs.nvim vendored-intellisense drift guard reads like a member and is not one, because `scripts/run-node-tests.js` walks `packages/editors//test/`, so `vendor-sync.test.mjs` runs in the full Node suite the cycle already ends on and the ordinary miss (editing `packages/editors/intellisense/src` without re-vendoring) reds locally. Only the narrow variant escapes, where the re-vendor ran on disk but was never `git add -f`-ed, so the working tree the local run reads is in sync while the commit CI checks out is not. +That instruction is the finding of two failed attempts, both made while writing this very paragraph, and both caught only by review. The first named the webjs.nvim vendored-intellisense drift guard and the `Build (@webjsdev/core dist)` job as members. Neither is one: `scripts/run-node-tests.js` walks `packages/editors//test/`, so `vendor-sync.test.mjs` runs in the full Node suite, and `test/packaging/build-dist.test.js` shells the same `scripts/build-framework-dist.js` that `build:dist` runs, so a bundling break reds locally too. The second attempt closed the list at the survivors and read complete while omitting `E2E (blog served on Bun)`, which IS a member, because `scripts/run-bun-tests.js` excludes the `e2e/` segment and the deferred e2e arm is the Node-served run. A list here is wrong in one direction or the other, and each wrong version is load-bearing, since the trade below only holds if the membership is right. + +The class is real and not small, and what it costs is the same work done later rather than work skipped, which is the trade that was chosen. It is a real cost rather than a free one. The local suites the Definition of done demands still run at the end of the cycle and still gate the flip to ready for review, so only the CI read moved. **NEVER use `gh pr merge --admin` to bypass a FAILING check.** `--admin` skips ALL branch-protection gates, not only the review requirement, so a red check merges silently and lands broken code on `main`. This has happened (a Unit-test failure was admin-merged, breaking `main`). It is acceptable ONLY to bypass a required-review gate on a PR whose CI is confirmed all-green, so re-run `gh pr checks ` first and confirm EVERY check reads `pass` (a `BLOCKED` state can mean review-required OR a failing check, so never assume which). diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index 24dc10593..553a7b1d4 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -169,12 +169,13 @@ test('CI is read only at the merge gate, never at the end of the cycle', () => { assert.match(skill, /\*\*This is the ONLY place CI is read, on purpose\. Do not add one back to the end of the review cycle\.\*\*/); // The cost it accepts must stay stated, or the next reader takes the // removal for an oversight and restores the read to "fix" it. It is stated - // as an open CLASS pointing at ci.yml, never a closed list: the list form - // has already been wrong in both directions on this paragraph, once - // inflated by an item the local suites do cover, once closed at two while - // omitting a REQUIRED check the paragraph above it names. + // as an open CLASS derived from ci.yml, never a written-down membership. + // Three attempts at listing it were wrong in both directions (inflated by + // jobs the local suites do cover, then closed while omitting one they do + // not), so the instruction not to enumerate is itself load-bearing. assert.match(skill, /Every `ci\.yml` job with no counterpart in the deferred local suites/); assert.match(skill, /That is a CLASS, not a list/); + assert.match(skill, /Do NOT write the membership down here/); // The four instructions that USED to make the cycle wait on CI. Each is // gone, and a revert of the hunk that removed it puts its phrasing back. From ce437e081473166a902b3f830deb1836beb8fc4d Mon Sep 17 00:00:00 2001 From: Vivek Date: Fri, 7 Aug 2026 11:51:06 +0530 Subject: [PATCH 5/6] chore: cut the per-attempt history from the do-not-enumerate note 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. --- .claude/skills/webjs-start-work/SKILL.md | 2 +- test/hooks/review-loop-exit.test.mjs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index f2131e2a4..b10347dd3 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -401,7 +401,7 @@ If you cannot honestly say the final review left nothing must-fix open, you cann **This is the ONLY place CI is read, on purpose. Do not add one back to the end of the review cycle.** An end-of-cycle read was removed because it was redundant against this gate: branch protection refuses the merge whatever the report claimed, so a red check gets caught and fixed right here, and reading it earlier only parks the finished cycle on a multi-minute CI run. What that costs is worth stating plainly, because it looks like a gap. 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 a CLASS, not a list. Its membership moves as jobs are added and as the local suites grow to cover them, so derive it when you need it, by reading `.github/workflows/ci.yml` against the deferred set named above. Do NOT write the membership down here. -That instruction is the finding of two failed attempts, both made while writing this very paragraph, and both caught only by review. The first named the webjs.nvim vendored-intellisense drift guard and the `Build (@webjsdev/core dist)` job as members. Neither is one: `scripts/run-node-tests.js` walks `packages/editors//test/`, so `vendor-sync.test.mjs` runs in the full Node suite, and `test/packaging/build-dist.test.js` shells the same `scripts/build-framework-dist.js` that `build:dist` runs, so a bundling break reds locally too. The second attempt closed the list at the survivors and read complete while omitting `E2E (blog served on Bun)`, which IS a member, because `scripts/run-bun-tests.js` excludes the `e2e/` segment and the deferred e2e arm is the Node-served run. A list here is wrong in one direction or the other, and each wrong version is load-bearing, since the trade below only holds if the membership is right. +That instruction is load-bearing and was expensive to learn. Three attempts to write the membership into this paragraph were each wrong, some by naming a job the local suites already cover, some by reading complete while omitting one they do not, and every one of them was caught by review rather than by the author. The reason the error keeps recurring is structural: deciding membership means holding `ci.yml` and the deferred set side by side and checking a non-obvious local counterpart for each job, which is a derivation, and a derivation frozen into prose is wrong the moment either side moves. So derive it, and do not leave the answer here for the next reader to trust. The class is real and not small, and what it costs is the same work done later rather than work skipped, which is the trade that was chosen. It is a real cost rather than a free one. The local suites the Definition of done demands still run at the end of the cycle and still gate the flip to ready for review, so only the CI read moved. diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index 553a7b1d4..c9aea3706 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -170,9 +170,9 @@ test('CI is read only at the merge gate, never at the end of the cycle', () => { // The cost it accepts must stay stated, or the next reader takes the // removal for an oversight and restores the read to "fix" it. It is stated // as an open CLASS derived from ci.yml, never a written-down membership. - // Three attempts at listing it were wrong in both directions (inflated by - // jobs the local suites do cover, then closed while omitting one they do - // not), so the instruction not to enumerate is itself load-bearing. + // Three attempts at writing that membership down were each wrong, in one + // direction or the other, so the instruction not to enumerate is itself + // the load-bearing part and is pinned alongside the class phrasing. assert.match(skill, /Every `ci\.yml` job with no counterpart in the deferred local suites/); assert.match(skill, /That is a CLASS, not a list/); assert.match(skill, /Do NOT write the membership down here/); From 4f236a8a30765f2dfc4b4fc0a7ea3d7ec2b25d34 Mon Sep 17 00:00:00 2001 From: Vivek Date: Fri, 7 Aug 2026 12:02:13 +0530 Subject: [PATCH 6/6] chore: stop claiming branch protection gates every CI job 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. --- .claude/skills/webjs-start-work/SKILL.md | 2 +- test/hooks/review-loop-exit.test.mjs | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index b10347dd3..83bcafef4 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -399,7 +399,7 @@ If you cannot honestly say the final review left nothing must-fix open, you cann **Merge is gated on green CI, enforced at the branch level, not by trust.** A PR must not merge until all CI checks pass. `main` branch protection requires the five `ci.yml` checks (Conventions, Unit+integration, Browser, E2E, Build) before any merge; if `gh api repos/webjsdev/webjs/branches/main/protection` shows `required_status_checks: null`, run `bash scripts/protect-main.sh` once (needs repo admin) to restore it. Do not work around a red or pending check. Wait for green, and fix whatever is red before merging. -**This is the ONLY place CI is read, on purpose. Do not add one back to the end of the review cycle.** An end-of-cycle read was removed because it was redundant against this gate: branch protection refuses the merge whatever the report claimed, so a red check gets caught and fixed right here, and reading it earlier only parks the finished cycle on a multi-minute CI run. What that costs is worth stating plainly, because it looks like a gap. 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 a CLASS, not a list. Its membership moves as jobs are added and as the local suites grow to cover them, so derive it when you need it, by reading `.github/workflows/ci.yml` against the deferred set named above. Do NOT write the membership down here. +**This is the ONLY place CI is read, on purpose. Do not add one back to the end of the review cycle.** An end-of-cycle read was removed because it was redundant against this gate, which reads EVERY check and fixes what is red before merging, so a failure gets caught here whatever the ready-to-merge report claimed, and reading it earlier only parks the finished cycle on a multi-minute CI run. Be precise about what enforces that, because the two halves are not equally strong. Branch protection MECHANICALLY refuses the merge for the required contexts only, and `ci.yml` defines roughly twice as many jobs as `main` requires, so the rest are held by the INSTRUCTION above to read every check rather than by anything that can refuse a merge. Removing the end-of-cycle read did not weaken that, since the removed read was an instruction too and branch protection covered the same subset before and after. It does mean a green REQUIRED set is not the same as green CI, so read `gh pr checks` in full rather than trusting the merge button to have judged for you. What that costs is worth stating plainly, because it looks like a gap. 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 a CLASS, not a list. Its membership moves as jobs are added and as the local suites grow to cover them, so derive it when you need it, by reading `.github/workflows/ci.yml` against the deferred set named above. Do NOT write the membership down here. That instruction is load-bearing and was expensive to learn. Three attempts to write the membership into this paragraph were each wrong, some by naming a job the local suites already cover, some by reading complete while omitting one they do not, and every one of them was caught by review rather than by the author. The reason the error keeps recurring is structural: deciding membership means holding `ci.yml` and the deferred set side by side and checking a non-obvious local counterpart for each job, which is a derivation, and a derivation frozen into prose is wrong the moment either side moves. So derive it, and do not leave the answer here for the next reader to trust. diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index c9aea3706..b4529c5a2 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -188,6 +188,13 @@ test('CI is read only at the merge gate, never at the end of the cycle', () => { assert.ok(!re.test(skill), `${label} tells the cycle to read CI again`); } + // The gate leans on reading every check, NOT on branch protection alone, + // which mechanically covers only the required subset. Overstating that was + // the removal's original justification and it was false for exactly the + // jobs it was justifying the cost of. + assert.match(skill, /Branch protection MECHANICALLY refuses the merge for the required contexts only/); + assert.match(skill, /a green REQUIRED set is not the same as green CI/); + // The gate the removal leans on has to stay strict, since it is now the // only CI checkpoint there is. assert.match(skill, /\*\*Merge is gated on green CI, enforced at the branch level, not by trust\.\*\*/);