chore: read CI only at merge, not at the end of the review cycle

.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 <sha>` / `rejected because <reason>` / `deferred as out of scope because <reason>`) 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 #<N> is up at <URL>. Reviewed it over <K> rounds plus a final pass over the whole diff; nothing must-fix is left open. Issues found and fixed: <one-line list, or "none">. Out-of-scope findings, recorded on the PR and awaiting your call on filing: <one-line list, or "none">. Ready to merge.
 
@@ -397,7 +397,13 @@ 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, 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.
+
+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 <N>` first and confirm EVERY check reads `pass` (a `BLOCKED` state can mean review-required OR a failing check, so never assume which).
 

test/hooks/review-loop-exit.test.mjs

@@ -162,6 +162,45 @@ 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. It is stated
+  // as an open CLASS derived from ci.yml, never a written-down membership.
+  // 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/);
+
+  // 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/],
+    ['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 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\.\*\*/);
+  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.