docs: make the self-review loop fast, parallel, and stall-proof by vivek7405 · Pull Request #1159 · webjsdev/webjs

vivek7405 · 2026-07-28T13:35:11Z

Closes #1158
Closes #1157
Closes #1160
Closes #1161
Closes #1162
Closes #1163
Closes #1164
Closes #1165
Closes #1166

The self-review loop required a fresh subagent reviewer but said nothing about what to do when that reviewer never comes back. Every failure mode (a declined permission prompt, an internal error, a killed background task) ended the same way: the loop stalled silently, or worse, quietly degraded to an inline self-review that then counted as a completed round. That degradation is the expensive part. On PR #1156 two inline rounds reported clean and the first real subagent round that followed found ten findings, three of them genuine bugs in code those rounds had already passed.

What changed

.claude/skills/webjs-start-work/SKILL.md, step 1 of ### How the loop works, gains a Reviewer liveness (non-negotiable) block alongside the existing working-tree-safety block:

Harness status is the ONLY liveness signal. For the mandated foreground spawn that status IS the Agent call's own result in the same turn. For a background reviewer in play anyway (the fix: refuse a function in form action=, it leaked server action source #1156 /code-review case) it is the harness completion notification, which arrives on its own and carries the real result. An agent's own prose ("I'm partway through the careful pass") is never evidence it is alive when its status reads killed. A dead background agent is not by itself a blocked loop, since it was never the mandated reviewer: write it off and spawn the foreground one.
A declined, errored, or empty-result spawn means the round did not happen, so it is not counted or reported as a round. The failure itself is still reported, loudly. The repo-health check keys off the SPAWN rather than a round completing, because a spawn that dies after its isolated worktree exists is the likeliest leaker of a stray worktree.
A reviewer that returns without reviewing is also a failed round. A subagent can come back successfully having reviewed nothing ("I could not fetch the diff"). The prompt now has it report BLOCKED in that case, but the rule does not rely on the sentinel alone: anything that is neither a finding list nor a literal CLEAN is the same state, and absence of findings is not a clean round.
Never downgrade to an inline self-review. It re-derives the same assumptions that produced the bug, and it is worse than no review because it looks like one.
Re-spawn a mechanically failed reviewer until one takes, varying the approach after a few identical failures, and never interrupt the user mid-loop over a failed spawn: recovering costs seconds, a status update costs the loop its momentum. Only a reviewer that cannot be produced at all (unspawnable, or never returning an actual review) blocks the loop, reported once at the end.
The blockage stays in the conversation. A spawn that could not run is session tooling rather than a fact about the change, so it goes in neither the PR body nor a PR comment; the PR-visible consequence is that the flip to ready for review is withheld, since the loop never converged.
The reviewer prompt gains a BLOCKED sentinel. Previously the template sanctioned exactly two answers, a finding list or the literal CLEAN, so a reviewer that never fetched the diff had found nothing genuinely wrong and was steered into CLEAN, which the loop accepts as converged. The returns-without-reviewing rule depended on an output shape the template forbade.

The loop's round shape is also specified now, measured on this very PR's review (five sequential broad rounds cost ~608k subagent tokens and ~25 minutes, dominated by a comment-history payload that reached 171 KB per reviewer): round 1 is the deep-review workflow for EVERY task entering the loop (a scout proposes up to six dynamic lenses that run alongside six fixed ones, split across two model families, with an adversarial jury per finding), every later round is one delta-scoped reviewer whose question is the fixes but whose evidence stays the full changed files, and reviewers get the diff, title and body, touched files, and rule files, never prior PR comments or a growing exclusion list. Deep-review re-runs in full when the PR's scope changes mid-review, and a manual "review the PR" ask always starts a fresh one. Reviewers spawn async with a WATCHDOG on the task's transcript file (fire on ~2 minutes of stalled mtime or ~10 minutes total, peek at harness evidence only, stop and re-spawn a stalled one): a synchronous spawn blocked the whole turn and let a hung reviewer eat 20 minutes unwatched, which is the blind wait this PR exists to kill. The bullets apply in the order written (classify, then retry, then declare blocked) and are cross-referenced by name rather than ordinal, so adding one cannot silently truncate the list for a literal reader.

### Reporting after the loop now says only a round in which a fresh subagent actually REVIEWED counts toward the round total. A completed call and a real subagent result are explicitly not the test, since a reviewer that says "I could not fetch the diff" satisfies both; the test is findings or the literal CLEAN. So a loop whose reviewer never reviewed reports as blocked rather than as clean. The two downstream absolutes that said the loop can only ever end on a clean round now cross-reference the blocked path, and the standalone-review entry point also points back at the working-tree-safety half of step 1 that it was skipping. ## Failure handling gains the blocked-reviewer case, since it is the file's designated failure taxonomy and this is the loop's only non-clean exit.

Fixes outside the new block, all of them rules that duplicated something the change moved. The working-tree block and the spawned prompt template both justified the read-only-git prohibition with "a SHARED working directory", which the mandated isolation: "worktree" makes false; the real reason is that every worktree shares one .git directory, so isolation covers the working tree while the prohibition covers the refs and config it cannot reach, which is also what explains the core.bare incident. The per-round posting rule told a findless round to post a clean review object even when nothing was reviewed. The reviewer prompt assumed the branch was checked out locally, which is false for a PR you were merely asked to review.

Both copies of the skill are byte-identical at head (in-repo is the newer one and was copied to ~/.claude, verified with diff -q).

Review-quality upgrades: refutation, model diversity, and a deep-review tier

Three later additions harden the loop itself. A shipped-source finding whose fix would be expensive or behavior-changing gets an adversarial refuter before anyone acts on it, spawned under the same isolation and liveness rules as any reviewer, failing open toward treating the finding as real. At least one fan-out lens runs on a different model, so the lenses do not all share one family's blind spots. And the riskiest surfaces get a committed, repo-generic deep-review workflow (.claude/workflows/deep-review.js, run via the Workflow tool with the PR number as args, or owner/repo#N for another repository): six finder lenses in parallel with two pinned to distinct models, then an adaptive adversarial jury per finding (three refuters for critical, two for major, one for minor, strict majority kills, empty juries and ties fail open), returning only what survives refutation with rejected findings carrying their refuters' reasons. .gitignore gains !.claude/workflows/ entries, without which the file was silently unstaged by the existing .claude/* ignore, a miss one review round caught after the pointer had already shipped ahead of the file.

The one settings change: a committed `Agent` permission

.claude/settings.json gains permissions.allow: ["Agent"]. A declined permission prompt is one of the four failures #1158 names and the only one with a configuration fix rather than a prose one, and this repo mandates the review loop for every agent that works it, so the permission that lets the loop run belongs next to the hooks that enforce the rest. Verified live: subagent spawns stopped prompting with no restart.

Two things to weigh, because the grant is broader than the problem. Permission rules are tool-level, so this auto-approves EVERY subagent spawn, not only reviewers. And the file is committed, so it applies to anyone who clones the repo. The same rule is set in ~/.claude/settings.json for my own machine. If the repo-wide grant is not wanted, dropping that one commit leaves the rest of the PR intact.

Deliberate deviations from two acceptance criteria in #1158

The "tell the user immediately, one retry" criterion is inverted, at the user's direction. #1158 asked for a same-turn report to the user when a round cannot run, with a single retry for transient failures. Mid-review the user rejected that behavior in practice ("I don't want any interruption until the last clean pass"), so the file now says the opposite: re-spawn a mechanical failure until one takes, never stop mid-loop to report a spawn failure, and only a reviewer that cannot be produced at all ends the loop, reported once at the end. The part of the criterion that survives is the part that mattered: a failed spawn is never counted as a round, never silently downgraded to an inline pass, and a blocked loop is reported rather than papered over.

The `TaskList` criterion

#1158 also requires the section to name TaskList as the confirmation that nothing is in flight. It does not, on purpose. TaskList is the shared to-do list (id / subject / status / owner / blockedBy) and returns empty in any session that never called TaskCreate, so it answers "nothing in flight" identically whether the reviewer is dead or running. A check whose answer never varies is worse than no check, and this one fails toward abandoning a live reviewer. The file names the mechanisms above instead, and states outright that an empty TaskList is evidence of nothing here so the next reader does not reach for it again. Every other criterion in #1158 is met as written. Full reasoning in the decision comment, which also covers why TaskOutput is not the answer either.

Also folded in: #1157 and #1160

Both were going to be follow-ups and are done here instead, since all three issues edit the same file and splitting them guarantees the two copies of the skill drift apart between PRs.

#1157, keep the loop fast. A round is a reading pass over the diff and costs seconds, but the skill never said what NOT to run around one, so sessions re-ran multi-minute suites between rounds and watched CI on top. Measured on #1156: one 32-minute gap between commits, spent running the Node and e2e suites twice each, none of it changing a finding and all of it re-run by CI anyway. Now only the touched test files run per fix, and the suites plus the CI read happen once after the last clean round. The text says outright that this changes WHEN they run and never WHETHER, because an earlier attempt at this wording read as permission to skip a layer.

#1160, stop restating the review-posting rules. Step 2 of the loop paraphrased the posting mechanics and had drifted from the authoritative section on both points: it sent findings through the standalone-comment API instead of one review object, used issue comments for round summaries, and had each finding carry its own disposition. All three are what the posting rules forbid, and the first is how a review on #1115 ended up ungrouped and had to be reposted. Step 2 now defers to those rules rather than restating them, keeping only what is genuinely its own. The division is the one a person actually uses on GitHub: review content goes up as a review object, and issue comments carry the context you add while committing and pushing, so an agent arriving cold at the PR has enough to work from.

Process note

The review ran five sequential subagent rounds, then a parallel fan-out of four narrow lenses, then a chain of delta-scoped verifies (each on the previous round's fix), which is the shape the change itself now prescribes. Findings from every round are on this PR as review objects, fixed and resolved; the last round was clean.

Test plan

node --test test/hooks/route-skills.test.mjs (16/16 pass, including "every skill the hook can route to is committed in-repo"). The file stays at its current path.
Grepped AGENTS.md "Code workflow" and "Git workflow" for contradictions: none. The one adjacent rule is Autonomous mode's "never block on questions", which the new wording addresses directly (the loop stops and reports rather than asking, and autonomous mode auto-decides branches/rebases/tests, not whether a review happened).
Every tool claim in the new prose was resolved against the actual tool schema rather than assumed, which is how the TaskList and TaskOutput mis-namings were caught.
The four failure paths from Never let a dead review subagent stall or downgrade the loop #1158 (declined prompt, internal error, killed background agent, empty result) were each traced through the file as it now reads, which is how the returns-without-reviewing gap and the skipped repo-health check were caught.

Doc surfaces

Tests: N/A beyond the router test above. This edits a Claude Code skill file and the repo's Claude Code settings, not shipped source.
Markdown / docs site / scaffold / MCP / editor plugins / marketing / dogfood apps / version bumps: N/A because nothing in packages/*/src or any user-facing surface changed. The settings change affects agent tooling in this repo, not anything the framework ships.

The loop required a fresh subagent reviewer but said nothing about what to do when that reviewer never comes back. Every failure mode (a declined permission prompt, an internal error, a killed background task) ended the same way: the loop stalled silently, or worse, quietly degraded to an inline self-review that then counted as a completed round. That degradation is the expensive part. On PR #1156 two inline rounds reported clean and the first real subagent round that followed found ten findings, three of them genuine bugs in code those rounds had already passed. An inline pass is worse than no review because it looks like one. So: harness status is the only liveness signal (an agent's own prose never is), a failed spawn means the round did not happen, an inline pass is never a round, and a blocked reviewer stops the loop and is reported in the same turn rather than becoming an unreported wait. The reporting shape now says only a round a subagent completed counts toward the total.

TaskList is the shared to-do list, not a registry of running subagents, so "empty means nothing in flight" reads empty in any session that never called TaskCreate. It reports "dead" identically whether the reviewer is dead or running, which is worse than no check. Issue #1158 specified it, but the premise does not hold. The real signal for the mandated foreground spawn is the Agent call's own result in the same turn, and for a stray background agent it is the harness completion notification or TaskOutput on its name. Also drops the claim that backgrounding is what lets a spawn be declined (a refusal lands on the Agent call either way), and cross-references the new blocked path from the two downstream absolutes that said the loop can only ever end on a clean round.

vivek7405

I went over this against the acceptance criteria in #1158 rather than just the diff, and the wording holds up on the parts that matter (a failed spawn is not a round, an inline pass is never a round, the reporting shape can no longer be satisfied by rounds that did not run). Where it falls down is the one concrete verification step: the rule says harness status is the only liveness signal, then names TaskList as the way to read it, and TaskList does not read it. That is the load-bearing bit, so it needs to be right before anything else here is useful.

The rest is coherence. Pinning a foreground spawn and then handing the reader a background-only apparatus (in flight, never poll) leaves a cold reader unsure which regime applies, and the two absolutes further down still say the loop can only ever end on a clean round, which the new blocked path contradicts with no cross-reference either way.

vivek7405 · 2026-07-28T13:41:54Z

Decision: the loop does not name TaskList, contrary to one criterion in #1158

#1158 requires the loop section to name TaskList as the confirmation that no reviewer is in flight. It does not, deliberately.

TaskList is the shared to-do list. Its schema returns id / subject / status / owner / blockedBy, and it is documented for finding work to claim and checking teammate progress. It is not a registry of running subagents, and in any session that never called TaskCreate, which is the normal case here, it returns empty. So "empty means nothing in flight" reports "your reviewer is dead" identically whether the reviewer is dead or actively working. A check whose answer never varies is worse than no check, and this one fails toward abandoning a live reviewer.

What the file names instead: for the mandated foreground spawn, the Agent call's own result in that turn IS the status, and there is nothing else to consult. For a background reviewer that is in play anyway, it is the harness completion notification, which arrives on its own and carries the real result. Both distinguish dead from running. The file also states outright that an empty TaskList is evidence of nothing here, so the next reader does not reach for it again on the strength of the issue text.

TaskOutput is not the answer either, for the record, since it is deprecated and its own description says a local agent's result comes from the Agent tool result rather than the output file.

The spirit of the criterion is met: there is a named, mechanical confirmation, and an agent's own prose is explicitly not it. The letter is not.

TaskOutput is deprecated and its own description says a local agent's result comes from the Agent tool result, not from the output file (which is the whole subagent transcript). Pointing at it by agent name was the same defect as the TaskList one it replaced: a named mechanism that does not do the job. The harness completion notification arrives on its own and carries the real result, so that is what the file names now. Also: the failed-spawn bullet said "do not report it" next to a bullet demanding you report the blockage, which reads as the exact silent stall this is meant to kill, so it now says do not report it AS A ROUND. And the standalone-review entry point now points at both halves of step 1, since it was sending readers to the liveness rules while skipping the working-tree-safety block that has actually corrupted a checkout.

vivek7405

This round went after the fixes rather than the original diff, and the pattern that showed up is the one worth naming: I replaced a named mechanism that does not work (TaskList) with another named mechanism that does not work (TaskOutput), without checking the second one either. TaskOutput is deprecated and its own description says a local agent's result comes from the Agent tool result, not the output file. Same defect, one commit later. So the standing lesson for a file like this is that every tool name in it has to be resolved against a real schema before it ships, because prose in a skill gets followed literally.

Two smaller things: the failed-spawn bullet said "do not report it" right next to a bullet demanding you report the blockage, which reads as the exact silent stall this whole change exists to kill, and the standalone-review entry point was pointing readers at the liveness half of step 1 while still skipping the working-tree-safety half, which is the one that has actually corrupted a checkout. Also caught that I claimed both copies of the skill were in sync when the fix commit had never been mirrored, and that the PR body still described the pre-fix behaviour.

Tracing the four failure paths through the new wording found daylight in all of them. A spawn that dies after its isolated worktree exists is the likeliest leaker of a stray worktree, and declaring a failed spawn "not a round" had quietly excused it from the repo-health check that fires after each round. The check now keys off the spawn. A killed BACKGROUND agent was routed straight to a blocked loop even though the mandated foreground reviewer had never been tried, so a perfectly reviewable PR could halt on evidence about an agent that was never the reviewer. It now writes that agent off and spawns the real one. A reviewer can return successfully having reviewed nothing ("I could not fetch the diff"), which was neither a finding list nor a literal CLEAN and hit no branch at all. That is now explicitly a failed round, because absence of findings is not a clean round. Retry is pinned: one per round, mechanical failures only, and a fresh round is not a way to buy a second retry. The durable record (draft PR plus the blockage in the body) now applies in every mode rather than only autonomously, since the interactive mode had the weaker record. Failure handling gains the blocked-reviewer case, and the spawned prompt template no longer justifies its git prohibition with a shared working directory the mandated isolation makes false; worktrees sharing one .git directory is the true reason and it survives isolation.

vivek7405

This round stopped reading the wording and traced the four failure paths from #1158 through the file end to end, which is what the first two rounds should have done. All four still had daylight.

The one I care about most is a reviewer that returns successfully having reviewed nothing. It is not declined, not errored, not killed, not empty, so every liveness rule I added passes it, and it produces no findings and no literal CLEAN, so neither loop branch catches it either. The only way forward for an agent reading the file was to improvise, and the plausible improvisation is "no findings, so clean", which is the exact outcome this change exists to prevent. Absence of findings is not a clean round, and the file now says so.

The rest: a spawn that dies after its worktree exists is the likeliest leaker of a stray worktree, and calling a failed spawn "not a round" had excused it from the repo-health check that fires per round. A killed background agent sent the loop straight to blocked without ever trying the foreground reviewer that was never blocked. Retry was under-pinned and readable as one-per-round-forever. The durable record was scoped to autonomous mode only, so the mode with a human watching got the weaker record. And the spawned prompt template still justifies its git prohibition with "a SHARED working directory", which the mandated isolation: "worktree" makes false; the true reason is that worktrees share one .git directory, which survives isolation and explains the core.bare incident.

vivek7405 · 2026-07-28T13:54:06Z

Review: the blocked-reviewer case was missing from ## Failure handling

Recording a round finding that landed on an unchanged line and had no diff anchor.

## Failure handling is the file's designated enumeration of what goes wrong, and it already carries a mid-loop case of this exact family (a review subagent that mutated shared git state, with a recovery recipe). The blocked-reviewer case, which is the loop's only exit that is not a clean round, was absent from it, so a reader consulting the failure taxonomy would not find the failure this change is about.

Added, including what happens to the branch, the commits, and the card: nothing. Only the flip to ready for review is withheld.

A declined permission prompt is one of the four reviewer failures #1158 names, and it is the only one with a configuration fix rather than a prose one. The repo mandates the self-review loop for every agent that works it, so the permission that lets the loop run belongs in the repo's own committed settings next to the hooks that enforce the rest. Tool-level is the only granularity available, so this allows every Agent spawn rather than only reviewers.

Read top-down, step 1 told an agent to declare the loop blocked and edit the PR body before it ever reached the bullet granting a retry, so an internal error ended in a blocked PR while still being retryable. Retry now comes first and the ordering is stated outright. The bullets are also no longer referenced by ordinal, since the previous commit added two and left the lead-in saying four, which would have stopped a literal reader one bullet short of the retry rule itself. Three duplicates had drifted from the rules they restate. The working-tree block still opened with the SHARED working directory premise that the prompt template stopped using, the standalone-review pointer still summarized the repo-health check as per-round after it moved to per-spawn, and the Failure handling entry told an autonomous agent to ask the user how to proceed, which AGENTS.md forbids outright. Each now matches its source, and the blocked path no longer demotes a ready PR that another session opened.

vivek7405

The pattern across this whole PR is now clear enough to name: every round's fix has drifted from a copy of the same rule somewhere else in the file. This round found three of those at once. The working-tree block still opened with the SHARED working directory premise the prompt template stopped using, the standalone-review pointer still summarized the repo-health check as per-round after it moved to per-spawn, and the Failure handling entry I added last round told an autonomous agent to ask the user how to proceed, which AGENTS.md forbids outright. A duplicated rule is a rule that will rot, so each of those now matches its source rather than restating it loosely.

The one that would actually have bitten someone: read top-down, step 1 told you to declare the loop blocked and edit the PR body BEFORE it ever reached the bullet granting a retry. So an internal error, the canonical retryable case, ended in a blocked PR that was still retryable. Retry now comes first and the ordering is stated outright. Related, last round's commit added two bullets and left the lead-in saying "four rules follow", which would have stopped a literal reader one bullet short of the retry rule itself; the bullets are cross-referenced by name now, not ordinal.

Also worth flagging on the settings commit: the body did not mention it at all and still claimed this PR was prose-only, which was wrong at head. It is disclosed now, including that the grant is tool-level (every subagent spawn, not just reviewers) and committed (everyone who clones the repo).

The returns-without-reviewing rule assumed an output shape the prompt template forbade. The template sanctioned exactly two answers, a finding list or the literal CLEAN, so a reviewer that never fetched the diff had found nothing genuinely wrong and was steered straight into CLEAN, which the loop accepts as converged. The template now defines a BLOCKED sentinel for that case and says outright that reporting CLEAN for a review you could not perform is the worst available outcome. Two rules that admitted the same non-round are fixed with it: the reporting shape tested for a completed call and a subagent result, both of which a non-review satisfies, and the per-round posting rule told a findless round to post a clean review object even when nothing was reviewed. Also: worktree isolation was described as containing every git op, which is the claim the shared .git premise replaces, so it now says isolation covers the working tree while the read-only prohibition covers the refs and config it cannot reach. The two retry paths were unified onto one budget, a recovered failure is reported in the turn rather than through the blocked-PR procedure, the reviewer prompt no longer assumes the branch is checked out locally, and a blocked review is reported to the user instead of being written onto the PR, since a spawn that could not run is session tooling and not a fact about the change.

vivek7405

The important one this round is that the rule and the prompt disagreed about what a reviewer is allowed to say. The template sanctioned exactly two answers, a finding list or the literal CLEAN, and told the reviewer not to pad. A reviewer that never fetched the diff has, quite truthfully, found nothing genuinely wrong, so the template steered it into CLEAN and the loop accepted that as converged. The returns-without-reviewing rule I added last round was therefore checking for an output shape the prompt forbade. There is a BLOCKED sentinel now, and the rule does not lean on it alone, since a reviewer that ignores the instruction lands in the same state.

Two other rules admitted that same non-round: the reporting shape tested for "a round a subagent completed" with "a subagent result to point at", both of which a non-review satisfies, and the per-round posting rule told a findless round to post a clean review object regardless. Both now test on findings-or-CLEAN instead.

The duplicate-drift pattern showed up once more on last round's own fix: replacing the SHARED-directory premise in two places left the bullet beneath it still claiming isolation contains every git op, which is the opposite conclusion. Isolation covers the working tree, the read-only prohibition covers the refs and config it cannot reach, and only that version explains the core.bare incident.

Separately, the blocked-reviewer path no longer writes anything onto the PR. A spawn that could not run is session tooling, not a fact about the change, so it is reported to the user and the only PR-visible consequence is that ready-for-review stays withheld.

The blocked path stopped the loop and asked the user how to proceed after a single retry. That is the wrong trade: a spawn that broke mechanically is harness noise, recovering costs seconds, and handing back a half-finished loop costs the review its whole momentum. The user asked for a converged review, not a status update. So a mechanical failure is now re-spawned until one takes, with the prompt adjusted when the reviewer named what it was missing, and varied rather than repeated after a few identical failures. Nothing about this relaxes the rule the block exists for: an inline pass is still never a substitute for keeping the loop moving. A reviewer that genuinely cannot be produced remains the one stopping point, it withholds ready-for-review, and it is reported once at the end rather than as a mid-loop interruption.

Two fixes that share a cause with everything else in this PR: a rule stated twice drifts, and a loop that stops to wait is a loop nobody runs. Closes #1157. A round is a reading pass over the diff and costs seconds, but the skill never said what NOT to run around one, so sessions re-ran multi-minute suites between rounds and then watched CI on top. Measured on #1156: a single 32-minute gap between two commits, spent running the Node and e2e suites twice each, none of which changed a finding and all of which CI re-ran anyway. Now only the touched test files run per fix, the suites and the CI read happen once after the last clean round, and the text says outright that this changes WHEN they run and never WHETHER, since an earlier attempt at this wording read as permission to skip a layer. Closes #1160. Step 2 of the loop restated the review-posting mechanics and had drifted from the authoritative section on both points: it sent findings through the standalone-comment API rather than one review object, used issue comments for round summaries, and had each finding carry its own disposition. All three are what the posting rules forbid, and the first is how a review on #1115 ended up ungrouped. Step 2 now defers to those rules instead of paraphrasing them, keeping only what is genuinely its own. A pointer cannot drift; a paraphrase will.

Switching the blocked path from one-retry-then-ask to re-spawn-until-it- takes updated the rule and left three places still describing the old one: the failed-spawn bullet told you to report each failure in the turn it happened, the returns-without-reviewing bullet still budgeted a single shared retry, and the Failure handling entry still said to retry once and then ask the user how to proceed. That last one is the same drift this PR keeps finding, in the same file, from my own edit.

Measured on this PR's own review: five sequential broad rounds cost 608k subagent tokens and 25 minutes, dominated not by local commands (5.6s of I/O per round) but by each reviewer ingesting a growing payload, and the prior-comments feed alone reached 171 KB by round 5. A parallel fan-out of four narrow lenses then covered more surface in one round's wall clock. So the loop now spawns round 1 as 3-4 narrow reviewers in parallel (correctness, internal consistency, issue satisfaction, mechanical), makes every later round delta-scoped on the fixes, and starves reviewers of context: the diff and touched files only, never prior PR comments, never an exclusion list, with duplicates deduped by the author for free. The same fan-out reviewed this change and its findings are fixed here too: the order lead-in referenced a retry distinction that no longer exists, two bullets pointed at "the bullet below" that was not below, the prompt-spec claimed the template ends with CLEAN when BLOCKED comes after it, the returns-without-reviewing bullet over-claimed the sentinel's coverage, the template never received the branch its spec promised, the reject path still wrote reasons into the PR body, the two blocked-loop summaries omitted the never-reviews case, ready-to-merge was announceable before the deferred suites and CI read, and the no-machinery-tells voice rule contradicted the body's required test plan and dogfood evidence.

vivek7405

This round ran as the shape the PR itself now prescribes: four narrow reviewers in parallel (loop-block coherence, whole-file consistency, issue satisfaction, mechanical), each fed only the diff and the file, no comment history. One round's wall clock, fourteen distinct findings after dedup, and the mechanical lens came back clean on invariant 11, the settings change, the mirror, and every cross-reference.

The pattern behind most of them is the one this PR has been fighting all along: an edit updates a rule and misses a reader of it. The order lead-in still promised a retry-budget distinction that no longer exists, two bullets pointed at "the bullet below" that was not below, the prompt spec said the template ends with CLEAN while BLOCKED comes after it, and the two blocked-loop one-liners still said "cannot be spawned" after the definition widened to "cannot be produced". Cross-references by position rot on every insertion, so the bullets now reference by name.

Three findings were not drift but genuine contradictions worth the round on their own: the reject path still wrote rejection reasons into the PR body, which the posting rules forbid; ready-to-merge was announceable at loop convergence even though the loop now defers every suite and the CI read past that point; and the no-machinery-tells voice rule, read literally, banned the test plan and dogfood evidence the Definition of done requires in the body. The body also described two rules as they stood several commits ago, and the never-interrupt design change was an undisclosed inversion of one #1158 criterion, now disclosed in its own section.

Adding fan-out and delta rounds to the preamble left the downstream machinery written for one broad sequential reviewer. The delta verify on the previous commit caught all of it: the template still told reviewers to rotate focus per round, a delta round had no prompt shape and would re-read the whole surface, the starve rule forbade the AGENTS.md read the spec required two bullets later, step 3 banned the fan-out re-run the preamble allows, step 4 ended the loop on a clean round 1 that the minimum-two-rounds rule sends to a delta pass, nothing said a fan-out is clean only when every lens is, and the strengthened Contains-verbatim requirement was violated by the file's own template. A fan-out now has explicit round semantics: each lens re-spawns individually without voiding the others, the round completes when every lens returns findings or CLEAN, its findings post as one aggregated review object, and the whole fan-out counts as one round.

vivek7405

The delta round on the fan-out's fix commit found what the fan-out itself could not have: I converted the loop's preamble to the new shape and left the per-round machinery written for one broad sequential reviewer. Every finding was that seam. The template still rotated focus per round, a delta round had no prompt shape of its own and would have re-read the whole surface the rules forbid re-reading, the starve rule banned the AGENTS.md read the spec demanded two bullets later, step 3 and the preamble disagreed on when the fan-out re-runs, step 4 ended the loop on a clean round 1 that the minimum-two-rounds rule sends to a delta pass anyway, nothing defined what a clean fan-out even means, and my own strengthened Contains-verbatim rule was violated by the file's own template.

The fan-out now has stated round semantics: lenses re-spawn individually, the round completes when every lens returns findings or CLEAN, findings post as one aggregated review object, and the whole fan-out is one round.

The starve rule enumerated a reviewer's permitted context exhaustively and left out the PR title and body that template step 2 has every reviewer fetch, so the rule and the template disagreed about what may be ingested. The body is the author's claims, which the review checks against, so it joins the enumeration rather than leaving the template. And the delta pass a clean fan-out earns had no instantiable scope: with no fix commits, every delta prompt shape pointed at a diff that does not exist. The spec and template now say that pass scopes to the riskiest section of the original diff, matching the minimum-two-rounds rule that created it.

vivek7405

Second delta pass, scoped to the previous fix commit. Two residuals, both the same seam class: the starve rule's exhaustive enumeration left out the PR title and body that template step 2 has every reviewer fetch, and the delta pass a clean fan-out earns had no instantiable scope, since with no fix commits every delta prompt shape pointed at a diff that does not exist. Both closed. The rest of the previous fix checked out: the sentinel wording now matches the template exactly, the one-round semantics are consistent across the preamble, step 4, and the reporting rule, and no trace of the rotation model survives.

The previous commit widened the delta scope in two of the three places that state it and missed the round-shape bullet that governs the loop, which still scoped every later round to fix commits that do not exist after a clean fan-out. Same for the starve enumeration: expanded in the rule, unexpanded in the prompt-construction bullet built from it, so an agent composing from the bullets starved the reviewer of the title and body the rule had just admitted. The template's charter placeholder also offered no instantiation for the clean-fan-out pass. All three now carry the same rule with the same carve-out.

vivek7405

Third delta pass, on the previous seam fix. It found the fix itself half-applied: the delta-scope rule is stated in three places and the widening reached two of them, leaving the round-shape bullet that actually governs the loop still pointing at fix commits that do not exist after a clean fan-out. The starve enumeration had the same shape, expanded in the rule but not in the prompt-construction bullet derived from it, and the template's charter placeholder offered no instantiation for the clean-fan-out pass. All three statements now carry the identical rule and carve-out. This is the file's recurring disease and the reason the loop keeps paying for delta rounds: every multi-statement rule is one edit away from disagreeing with itself.

The no-fix-commits fallback was conditioned on "after a clean fan-out", but a round whose findings were ALL rejected also produces no fix commits and is still followed by a mandatory round, so it had no defined scope anywhere: the headline clause was empty and the carve-out did not apply. The condition is now "a round that produced no fix commits", with both causes named, in all four places the rule appears. Also pinned --repo on the title-and-body fetch so a prompt composed from the bullets works when the branch is not checked out locally, matching the template.

vivek7405

Fourth delta pass. Two findings, and the first is a good catch of an edge my carve-out missed: a round whose findings are ALL rejected produces no fix commits either, is still followed by a mandatory round, and had no defined scope anywhere, since the fallback was conditioned solely on a clean fan-out. The condition is now the actual invariant, a round that produced no fix commits, with both causes named, in all four statements. The second was self-inflicted in the same hunk it reviewed: I wrote the title-and-body fetch without --repo while the template's copy carries it, exactly the flagless-command drift the template warns about.

Findings can be fixed, rejected, or filed, and a round of only rejected and filed findings produces no fix commits either. Step 3's fallback fired only on "every finding rejected", so the filed case executed the main clause against an empty diff. The trigger is now the mechanism itself, the round produced no fix commits, which cannot be under-inclusive whatever dispositions compose it.

vivek7405

Delta pass on the 3/3 model pinning: clean. The half-opus/half-fable scheme reads identically in the lens array, its comment, the meta, and the skill pointer; no trace of the two-pinned scheme survives; the always-truthy spread fails safe for a future unpinned lens; and the everyday fan-out's different-model sentence coexists with the workflow's scheme without drift.

Six fixed lenses cover the categories every PR shares, but the angles a specific PR earns depend on what it actually does: concurrency for async coordination, wire compatibility for a serializer change, migration paths for a schema move. A new Scope phase runs one scout that reads the diff, is told the fixed six so it never duplicates them, and proposes zero to six additional lens charters, zero being a fine answer for a PR the fixed set already covers. Dynamic lenses alternate between fable and opus by index, run in the same Find fan-out, and report through the same dedup and jury pipeline. A caller can also pass explicit lenses via args, which skips the scout. Stats now name the dynamic lenses used so a run's shape is auditable from its result.

The user directed a uniform rule: every task entering the review loop starts with the deep-review workflow, whatever the work touches. The scout removed the last advantage the ad-hoc fan-out held (per-PR lens tailoring), the workflow's cost self-scales because juries convene per finding, and a uniform rule cannot be misjudged the way a by-risk bifurcation can. The fan-out machinery, the lens-swapping guidance, and the match-depth-to-risk bullet are deleted rather than kept alongside, since duplicated review rules drifting apart is this file's documented disease. The refuter gate survives scoped to delta-round findings, whose reports are the only ones that now arrive without a jury behind them. Two rules added at the user's direction: deep-review re-runs in full when the PR's scope changes after round 1 (new work folded in, a pivot, an issue absorbed mid-review), because a delta round verifies fixes to an audited surface and cannot stand in for the audit of a surface round 1 never saw. And a standalone "review the PR" request always starts with a fresh deep-review, regardless of prior cycles, because the ask itself says the existing trail is not trusted or not current. The trivial-change skip is unchanged: a one-line typo still enters no loop at all.

The unification made round 1 a Workflow run and left the machinery written for Agent-tool spawns, the same seam the fan-out conversion hit. The delta round enumerated all of it. The liveness rules now name the Workflow run's completion notification as round 1's status signal, and the clean-round tests recognize each reviewer's expected result shape, since a deep-review run returns a structured result and never the literal CLEAN the old test demanded. The foreground mandate and prompt spec are scoped to the delta verifier. Working-tree safety states honestly that deep-review's internal agents run on the prompt defense alone, which is exactly why the repo-health check now fires after workflow runs too. Round semantics for a jury-adjudicated round are defined: confirmed findings are the round's findings, jury-rejected ones return for the audit trail only and force nothing, while a delta finding the author rejects still forces a round because the author's own rejection is unadjudicated in a way a jury's is not. A manual review ask overrides the trivial-change skip: when the user asks for a review, they get one.

The liveness tail still wrote off every dead background agent as never the mandated reviewer, which stopped being true the moment round 1 became a background Workflow run; it now distinguishes a stray agent from the round's own reviewer and names the right recovery for each. The standalone-review recap still restated the old both-defenses-always scheme its source no longer says, and the Failure handling bullet still spoke only of re-spawning a subagent. All three now match the workflow-shaped machinery they summarize.

The foreground mandate rebuilt the exact blind wait this PR exists to kill: a synchronous spawn blocks the whole turn, so a hung reviewer is indistinguishable from a working one for as long as the harness takes to give up, and one delta round that should cost minutes ate 20 that way, unwatchable from inside the blocked call. Reviewers now spawn in the background with a watchdog on the task's transcript file, firing on two minutes of stalled mtime or ten minutes total. The peek it triggers reads harness evidence only, task status and whether the transcript is still growing; the words in a transcript remain an agent's own prose and count for nothing, so the #1158 rule survives intact. A stalled reviewer is stopped and re-spawned, a progressing one is left alone, and a hang past the watchdog joins the failure taxonomy.

The async flip left one clause still calling the delta verifier foreground, inside the non-negotiable block, which was the one sentence that could rebuild the blind wait. The liveness split also gave the round-1 workflow run no mid-flight signal at all, a blind wait for the longest-running reviewer kind, while the failure taxonomy assumed it had one. Both reviewer kinds now share the same pair of signals: the harness notification on completion, the watchdog's output-file activity probe mid-flight, TaskStop on either task id. And the watchdog's path is now specified rather than implied: taken from the spawn's own tool result or task notification, never guessed, since a guess once aimed a watchdog at its own output file; with no path available it degrades to a hard timer on the same thresholds.

An audit of all nine skills (filed as #1161, #1162, #1163, all closed by this PR) found four whose dominant cost is independent work run serially, and five that are correctly sequential (a board query, a write-up, a post gain nothing from agents). start-work: the deferred suites and the CI read launch as one batch of background tasks after the last clean round, with every result collected before anything is reported, so the wall clock is the slowest suite rather than the sum, and the WHEN-not-WHETHER rule is explicitly untouched. Also pinned: reviewers are one-shot, never persistent teammates, because accumulated context is the blind-spot sharing the fresh-reviewer rule exists to kill. file-issue: multi-area grounding fans out as parallel read-only Explore agents, one per concern, with the cold-start test unchanged as the gate. doc-sync and scaffold-sync: surface sweeps fan out as read-only Explore agents while edits stay in the main session, since parallel writers on overlapping docs collide; scaffold verifications run as parallel background boots, preferring the portless in-process harness so runs cannot collide, with distinct --port values when a real listen is unavoidable.

A worktree-isolated agent's output file in the tasks directory is a static stub whose mtime never moves, so probing it reports a stall for every healthy isolated reviewer, and two were killed mid-review on exactly that misread before the CLEAN verdict arrived from the second. For isolated reviewers the watchdog now degrades to the hard timer alone, and only the hard cap or a killed/errored task status justifies a kill.

Closes #1164: research-record investigations fan out one read-only Explore agent per option or angle, with the writeup and its judgment staying with the caller, and the where-the-record-lands rules untouched. Closes #1165: blog-write delegates the de-dup sweep to one Explore agent returning per-post summaries, and runs the dogfood verification's independent parts as one batch of background tasks, Node and Bun boots in parallel with both still mandatory, every result collected before a post is called verified, and browser-dependent probes kept in the main session.

The stub caveat was one sentence patched into a paragraph whose every other sentence still described the transcript model: the mtime-fire clause fired on every healthy isolated reviewer, the stalled-gets- stopped and tail-peek sentences contradicted the nothing-softer kill rule, the 30-second cadence was decorative for the mandated shape, the liveness paragraph still promised a mid-flight probe for both kinds, and failure handling kept a stalled transcript as a kill trigger. The watchdog now has two explicit modes: transcript mode (the file grows, mtime probe, stall fires, TaskStop and re-spawn) and stub mode (the permanent healthy state of every isolated reviewer, hard timer alone, kill only on the cap or a killed/errored status), and the liveness promise is corrected from never-a-blind-wait to never-an-UNBOUNDED wait, which is the guarantee that was actually true all along.

…nder it

The run's worst case was ~41 agents (scout, twelve finders, full juries), which is a token bill nobody chose per review. A whole-run budget now caps it at 24 by default, overridable via maxAgents and clamped 8 to 60, degrading gracefully: the six fixed lenses always run, dynamic lenses trim to what remains after reserving four jury slots, juries allocate severity-first from the leftover, and a finding the budget cannot verify is returned in an unverified list with a log line, never silently dropped, per the no-silent-caps rule. Both return paths carry the unverified list and the effective cap in stats so a run's shape is auditable from its result.

Closes #1166. The worktree rule was conditional on concurrency, with a lone-agent plain-branch carve-out whose premise is unverifiable: an agent cannot know another session will not start mid-task, and this repo runs several a night. The start-work skill's steps 3 and 4 now cut a worktree from origin/main unconditionally (a dirty primary neither blocks nor gets fixed, since it is never edited at all), AGENTS.md states the rule without the carve-out, and the instagram skill roots its own worktree at the primary checkout so it cannot nest under a task worktree. Enforcement is real now rather than fictional: AGENTS.md pointed at .claude/hooks/guard-branch-context.sh, which does not exist in the repo. The new require-worktree-for-edits.sh hook blocks (exit 2) any tracked-file edit in a repo's PRIMARY checkout, judged against the FILE's directory rather than the session cwd, since the harness resets cwd to the primary while edits legitimately target worktree files by absolute path. Untracked and gitignored files stay editable, primary detection is the git-dir/git-common-dir equality invariant, and WEBJS_NO_WORKTREE_GATE=1 is the escape hatch, all covered by seven tests including the counterfactual block case.

The review ran the hook itself and caught it failing open for every tracked file in a SUBDIRECTORY of the primary checkout: invoked via -C on a subdir, git prints --git-dir absolute and --git-common-dir relative (../../.git), so the raw string compare misclassified the primary as a linked worktree, and the gate guarded only root-level files while the tests stayed green by testing exactly those. Both paths now resolve through --path-format=absolute, the test repo carries a nested tracked file whose block case fails against the old hook, and the escape-hatch env var is pinned off in non-bypass tests so an inherited value cannot flip the assertions. Also from the round: the instagram recipe roots every path (mkdir, cp, cd, teardown) at one captured PRIMARY variable instead of half at the primary and half at the session cwd; the start-work skill's three surviving checkout-in-place passages now describe the worktree model, where HEAD-on-main is the primary's healthy state and the task worktree is the only holder of its branch; and AGENTS.md's autonomous-mode line cuts a worktree from origin/main instead of branching in place with the retired feature/ prefix.

The instagram teardown's rooting was illusory: Step 4's user confirmation sits between setup and teardown, shell state does not survive across tool calls, and git -C "" silently degrades to cwd-relative, so the teardown was exactly the unrooted form the prior round flagged. It now re-derives PRIMARY and BR at the top of its own block, and the derivation swaps awk for sed since awk splits a path containing spaces. The start-work recovery command anchors its checkout to the task worktree: run bare from the primary it would succeed and park the PRIMARY on the feature branch, because a detached worktree no longer holds the branch and the git-refuses safety net does not apply.

Dynamic lenses alternated fable and opus by index, so a full run could pin six reviewers to fable. A MAX_FABLE budget now caps the pinned fable count across the whole run: the three fixed fable lenses (security, invariants-docs, fresh-eyes) spend it, and every dynamic lens takes opus. Model diversity is unchanged, since both families still always read the diff, and stats report the effective fable count so a run's split is auditable from its result.

Five substantive rounds bound the loop by default (OVER BUDGET is a third exit with its own report, next to converged and BLOCKED), a mid-loop fix that changes shipped behavior runs the same red/green counterfactual toggle as the original change (an unproven fix once made an older test non-discriminating and bought two extra rounds), counterfactual claims are dated to the commit they were proven at, and the loop defers out-of-scope findings (thread record + report line) instead of filing issues, since the owner pays for every filed issue and one 18-round loop filed five. Folded from #1176 at the owner's direction rather than left as a follow-up, the #1159 precedent.

…1173) * docs: exit the review loop on a substantive gate plus one prose pass The loop's exit was 'a round that finds nothing', which cannot terminate on prose surfaces: a 14-round loop stopped touching the shipped guard after round 6 and spent rounds 10 to 13 relitigating wording about a transpiler. The exit is now two-tier: loop while a round finds anything SUBSTANTIVE (shipped source, a test's ability to observe the defect it claims to cover, or a factual runtime claim in docs), then one bounded prose pass whose fixes are applied without re-review. The gate is surface-based, not importance-based, so the agent motivated to terminate cannot classify its way out; reviewers tag each finding and the orchestrator verifies the tag, with untagged findings treated as substantive. Also bans publishing rules about a third-party tool's internals (four successive falsified Bun constant-folding rules). * docs: close review-found gaps in the gate exit; add its guard test The two-round-minimum carve-out now covers a prose-only round 1, the re-tag license is asymmetric (downgrades recorded on the thread), the round-1 bullet and round-counting rule are tier-consistent, and an unproducible prose-pass reviewer ends the loop BLOCKED. The skill router's injected review directive states the new exit instead of the old clean-round absolute it still carried. Both reviewer-facing prompts qualify counts (a number in docs stating runtime behavior is surface three). test/hooks/review-loop-exit.test.mjs pins the load-bearing anchors in all three files so a revert or typo-drift reds CI. * docs: keep the router directive and dedup in lockstep with the gate The gate now defines the one reviewer kind that structurally cannot tag (a fixed result schema, the built-in code-review shape): the orchestrator classifies it alone, doubt resolving to substantive and prose classifications recorded like downgrades, and the router directive states that instead of a freestanding self-classify license. Deep-review dedup prefers a substantive finding at a file:line collision so a prose finding can never flip the exit signal. The round-1 bullet no longer reads as skipping the mandatory delta pass. The guard test anchors the definition bullet uniquely (its shared phrases also live in the delta template, so alone they were vacuous) and pins the previously unpinned hunks. * docs: extend tier precedence to the CAP slice and jury walk Tier now outranks severity everywhere findings compete, not only at the same-line dedup collision: the sort that feeds the CAP slice and the jury-budget walk is tier-first, the CAP tail is returned in unverified instead of only logged, and a missing tier fails open to substantive. The router directive names the same round-1 reviewer as the skill (the deep-review workflow, code-review findings as auxiliary input), and the docs-number qualifier now lives in the normative definition the verify and downgrade steps audit against, not only in the reviewer prompts. * docs: state the CAP remedy truthfully; fail a dead jury slot open The verification cap is a fixed 12, so its unverified tail is re-run-recoverable only; the log line, header comment, and the skill's round-1 bullet now say which remedy fits which cause instead of pointing every shortfall at maxAgents. A finding whose whole jury task resolves falsy now fails open to CONFIRMED like the empty jury, so every allocated finding lands in a returned list. The jury-walk comment matches the tier-first sort, and the router directive scopes self-classification to the code-review skill's own findings. * docs: apply the prose pass The router directive's appositive attaches the reviewer kind to the skill rather than its findings, step 4 uses the same name for the two-round minimum as its other mentions, and the guard test's CAP comment says which file its third assertion targets. * docs: fold in #1176, budget the loop and stop auto-filing follow-ups Five substantive rounds bound the loop by default (OVER BUDGET is a third exit with its own report, next to converged and BLOCKED), a mid-loop fix that changes shipped behavior runs the same red/green counterfactual toggle as the original change (an unproven fix once made an older test non-discriminating and bought two extra rounds), counterfactual claims are dated to the commit they were proven at, and the loop defers out-of-scope findings (thread record + report line) instead of filing issues, since the owner pays for every filed issue and one 18-round loop filed five. Folded from #1176 at the owner's direction rather than left as a follow-up, the #1159 precedent. * docs: align every exit and disposition seam with the fold The three-exit taxonomy (converged, BLOCKED, OVER BUDGET) and the defer disposition now read the same at every cross-reference: the failure-handling reviewer bullet, the standalone-review entry, the liveness block's stopping-point sentence, the prose-tier bullet, and the threaded-reply disposition list. An over-budget stop skips the deferred suites and CI read, stated where the report shape is defined. * docs: a finding on the PR's own code is never deferrable Deferral exists for findings the diff did not create (a pre-existing bug, an unrelated problem, a separate feature). A finding on code the PR adds or changes is fixed or rejected in the loop, whatever its size, so deferral cannot become the path around either. * docs: the final summary review carries a deferral ledger When a loop deferred anything, its final summary review on the PR lists each deferred finding with its one-sentence reason, so a cold reader sees every deferral in one place without walking the threads. The report accounting treats a deferral missing from the thread, the report, or the ledger as a dropped finding. * docs: the chat report expands the deferral ledger The end-of-loop chat report mirrors the PR's deferral ledger instead of a bare name list (each finding, its reason, its thread) and ends by asking which, if any, should be filed, so the user reads the summary without opening the PR. * docs: the deferral report must end by asking whether to file The chat report's filing question is mandatory: it ends the report on its own line, silence is never consent, and a deferral report that does not ask has not finished reporting.

vivek7405 self-assigned this Jul 28, 2026