Skip to content

Never let a dead review subagent stall or downgrade the loop #1158

Description

@vivek7405

Problem

The self-review loop requires a fresh subagent reviewer (SKILL.md L317) but says nothing about what to do when that reviewer does not come back. It fails in several ways, and every one of them currently ends in the same place: the loop stalls silently while the session appears busy.

Observed on PR #1156 (closed) in a single session:

  • The Agent spawn was declined at the permission prompt three separate times. Each time the loop just stopped, with no statement that the review had not run.
  • One spawn returned an internal error with no usable result. Same silence.
  • A /code-review background agent was killed. Its final message read "Yes, it's running. I have the PR fix: refuse a function in form action=, it leaked server action source #1156 diff and I'm partway through the careful pass" while its harness status was killed and TaskList was empty. An agent's own prose is not evidence that it is alive. Reading that text as liveness is what produces an unbounded wait.

The compounding failure: when the reviewer was unavailable, the loop was quietly downgraded to an inline self-review, which passed as a completed round. That is strictly worse than not reviewing, because the whole point of a separate reviewer is that it does not share the author's blind spots. In this session the inline rounds reported clean, and the very next real subagent round found ten findings, three of them genuine bugs in code that had already been self-reviewed twice: an unguarded .action=${fn} property path, a test-less branch that could have been deleted with the suite still green, and a factually wrong claim in a commit message.

Design / approach

Three rules in the loop's step 1, next to the existing reviewer instructions.

1. Treat the harness status as the only liveness signal. A killed / errored / missing result means nothing is running, regardless of what the agent's last message says. Confirm with TaskList (empty means nothing in flight) and proceed. Never poll, never wait, never report an agent as "still going" on the strength of its own text.

2. Never silently downgrade to an inline self-review. The subagent is required, not preferred, because a self-review re-derives the same assumptions that produced the bug. If a round cannot run with a fresh reviewer, the round did not happen and must not be counted or reported as one.

3. Surface the failure to the user immediately, in the same turn. State plainly that the review is blocked, why, and what the options are. Retry once if the failure looks transient. A blocked reviewer stops the loop and becomes a question for the user; it never becomes an unreported wait.

Also worth pinning: spawn with run_in_background: false so a failure returns as a visible result rather than a task that might be sat on.

Implementation notes (for the implementing agent)

  • Where to edit: .claude/skills/webjs-start-work/SKILL.md (git-tracked), step 1 of ### How the loop works (L317). The existing sub-blocks there are "Working-tree safety (non-negotiable)" (L319) and the "Pass a prompt that:" bullet list (L327-332); add the new block alongside them. The reporting shape at L355-361 (### Reporting after the loop) should also make an unrun round impossible to report as clean.
  • Mirror to ~/.claude/skills/webjs-start-work/SKILL.md. Two copies exist and they drift. The in-repo copy has historically been the NEWER one (it knows docs and packages/ui/packages/website are redirect-only hosts, seo: serve the docs at webjs.dev/docs with the marketing site chrome #1098/seo: serve the UI gallery at webjs.dev/ui with the marketing site chrome #1099, which the global copy did not), so diff before copying and never blind-copy global over in-repo. Verify with diff -q afterwards.
  • Landmine: an earlier attempt at this wording made the fallback be "review it yourself inline and keep the loop going". That is the wrong fix and was reverted: it optimizes for the loop continuing rather than for the review being real, and it is exactly how three bugs reached a supposedly-clean PR. The rule must be that a blocked reviewer STOPS the loop and asks the user, not that it degrades to a weaker reviewer.
  • Landmine: do not couple this to the timing rules in Stop the self-review loop re-running suites and waiting on CI #1157 (no suites in the loop, never wait on CI). Related area, separate concern, and Stop the self-review loop re-running suites and waiting on CI #1157 is deliberately minimal.
  • Invariants: AGENTS.md invariant 11 applies to the new prose (no em-dash, no space-hyphen or space-semicolon as pause punctuation, WebJs capitalized when naming the project); .claude/hooks/block-prose-punctuation.sh enforces it on write. Nothing in AGENTS.md currently contradicts these three rules, but grep "Code workflow" and "Git workflow" before finalizing wording, since a previous edit to this same file did contradict them.
  • Tests / docs surfaces: none. This edits a Claude Code skill file, not shipped source. test/hooks/route-skills.test.mjs asserts every skill the router references is committed in-repo, so keep the file at its current path.

Acceptance criteria

  • The loop section states that harness status, not an agent's own message, determines whether a reviewer is alive, and names TaskList as the confirmation
  • It states that a declined, errored, killed, or empty-result spawn means the round did not happen
  • It forbids counting or reporting an inline self-review as a review round
  • It requires telling the user immediately when a round cannot run, with a single retry allowed for a transient failure
  • It specifies run_in_background: false for the reviewer spawn
  • The reporting shape at L355-361 cannot be satisfied by a loop whose rounds did not actually run
  • No contradiction with AGENTS.md "Code workflow" or "Git workflow" (grep both and confirm)
  • Both copies of the skill are identical afterwards (diff -q in-repo vs ~/.claude), in-repo treated as newer

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Type

No type

Projects

Status
Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions