feat(dispatch): tell a crashed shell reviewer apart from a slow one (G3)

[kevin-agrology]

TL;DR: When a gemini reviewer crashes on launch, the review currently can't tell that apart from a reviewer that's just being slow — so it waits up to ~40 minutes and then blames the reviewer for "not taking a turn". This PR saves what the reviewer process actually printed, plus whether it died, and reads that before deciding. A crash now gets reported as a crash, immediately, with the real reason.

The bug (G3)

The shell dispatch branch consulted neither exit code nor stderr. A gemini that died on launch — bad flag, expired auth, exhausted quota — leaves a working copy byte-identical to its seed. So does a reviewer that is still thinking. Those are opposite facts and the protocol saw one value.

What followed was all wrong, in order:

1. Step 5's exit-9 grace re-run fired, spending another bound on a dead process.
2. Failing that, the exit-8 retry budget could spend three more — up to ~40 minutes total.
3. The provider was quarantined as no turn taken, which is the reason for a reviewer that read the document and declined.

The cause was on the process's own stderr the whole time. Nothing ever looked at it.

This is the same class as #66 and the DISPATCH-FAILED fix in a4bcf71: the round reports the symptom it can observe rather than the cause it never captured.

The change

Dispatch captures both streams and its own exit status:

rm -f "<doc>.<id>.multi-review.log"
if (( ${#argv[@]} )); then
  ( cd "<session-root>" && "${argv[@]}" ) >"<doc>.<id>.multi-review.log" 2>&1
  printf '\nmulti-review: dispatch exited %s\n' "$?" >>"<doc>.<id>.multi-review.log"
fi

Step 5 reads that log before the first wait and again at every bound hit. The decision is keyed off the copy, not the exit code — the status answers only "has the process exited?", and it counts only as the log's final non-empty line (an earlier match is echoed text, and while the reviewer is alive the real status does not exist yet):

copy state	meaning	action
marker already awaiting-author	the turn completed	verify normally; never quarantine, whatever the status says
status present, unflipped, copy changed since its seed	wrote something, then died	do not re-wait — recover via channel-check; quarantine only if the findings are unreadable
status present, unflipped, copy byte-identical to its seed	nothing written	quarantine, reason dispatch exited <rc>; see <log>
no status line	still running	existing bound-hit rules apply unchanged

Residual, stated rather than implied: a process that dies just after the pre-wait read is not noticed until the next bound hit — one bound, against the four this removes.

.multi-review.log is an already-gitignored shape (.gitignore:1), so the new artifact does not leak into consuming repos and does not worsen the retained-artifact problem.

Review history (this is what round 3 should look at)

Reviewed through this repo's own multi-review: codex (gpt-5) + fable. Round 1: 6 findings. Round 2: 7 findings, all agreed, none disputed. Round 3 was requested explicitly by the engineer.

Round 1 found that the status sentinel was unreadable in exactly the crash it targets — a bare echo appends onto whatever the process last wrote, so a death mid-write produced quota exceededmulti-review: dispatch exited 1 with no line-anchored match. Both vendors found this independently. Also: a flipped marker had to outrank a non-zero status (else a turn that completed then died on teardown was discarded with its findings); the quarantine reason must name the log rather than quote it (an auth error there carries a credential into a durably-recorded field); the previous round's log must be cleared; and the rc-zero branch mislabelled "flip forgotten" as "wrote nothing".

Round 2 found that three of those round-1 fixes were wrong:

• The stale-log removal was relocated, not fixed — rm -f sat inside the dispatch block, which runs as a background task, so it raced the pre-wait read it existed to prevent. It now runs in the seeding step, synchronously.
• The decision table keyed off the exit code, giving opposite fates to one on-disk state and re-waiting up to 3× on processes already proven dead. It now keys off the copy: marker flipped → verify; changed-then-died → recover via channel-check; byte-identical → quarantine. The exit code is evidence the process ended, nothing more.
• The widened test window had zero margin again — the printf line landed on the window's last row, the exact condition the widening was for. Both windows are now bounded by document structure.

Plus the sentinel search now takes the LAST match (the log is verbatim CLI output, and this repo's self-reviews contain the sentinel string), and the residual cost is stated rather than implied: a process dying just after the pre-wait read is not noticed until the next bound hit — one bound, against the four this removes.

Two coverage failures the reviewers did not catch — the mutation sweep did:

1. Fixing a round-1 finding added a second mention of the log filename inside the window one assertion greps, silently giving it zero coverage.
2. The round-2 test rewrite located its insertion point with a marker occurring three times, took the first, and deleted three assertion blocks from the cwd-pin block to end-of-file. The suite stayed green. --verify-table stayed green. Only the full sweep caught it, as three SURVIVED and one MISCREDITED.

That is four instances in one PR of a verification that reads correctly and cannot fail. One was caught by the author, one by two reviewers, two by the mutation sweep.

Not addressed

The codex arm still discards its task-<id> and quarantines blind (X1/X2 in the backlog). This PR fixes the shell arm only; the codex arm needs the companion's job registry, which is a dependency on its on-disk layout and a separate decision.

Security

No new external input is parsed and nothing is transmitted. The log captures the reviewer CLI's stdout and stderr verbatim, and a CLI that fails to authenticate could in principle print credential material into it. The file is gitignored and local, never uploaded, and never read by any script. Round 1 closed the one path that would have moved that content somewhere durable (fable-rd1-r3): the quarantine reason now names the log rather than quoting it.

Verification

• TDD throughout: assertions written first, confirmed RED for their expected reasons, then GREEN.
• All 15 suites green; shellcheck --severity=warning clean on scripts/multi-review-*.sh and .githooks/pre-push.
• multi-review-version-check.sh: 1.23.1 → 1.24.0 (minor — new behavior).
• --verify-table: 94/94 entries match.
• Full mutation sweep: all 94 caught, exit 0, no SURVIVED/MISCREDITED/STALE/ERROR, tree clean afterwards. An earlier sweep on this branch reported one SURVIVED; that is the coverage gap described above, now fixed and re-verified.

Not verified live: the gemini CLI is not installed on this machine, so the shell arm is code-read only. No end-to-end run of a crashing gemini was performed. The sentinel behaviour was verified directly against a simulated dying process.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Nm3hNXA4WHbgKjFbxX7VUE

[kevin-agrology]

Full mutation sweep: green.

mutation-check: all 91 mutation(s) caught

Exit 0, no SURVIVED / MISCREDITED / STALE / ERROR lines, and the working tree was clean afterwards (a killed sweep can leave a mutation in place, so that last check is part of the result, not a formality).

91 entries, up from 88 on main: three new (command/shell-dispatch-log-capture, command/shell-dispatch-exit-status, command/shell-crash-log-consulted), plus command/gemini-dispatch-cwd-pinned re-pointed at the moved dispatch line.

---

🤖 Comment posted by an AI agent — Claude Opus 5 (claude-opus-5), on behalf of @kevin-agrology, who remains accountable for its contents.

[kevin-agrology]

Commented inline (3)

Could not place inline (12)

• commands/multi-review.md:487-489 — 🟠 med — The fable-rd1-r4 fix relocates the stale-log race instead of closing it, yet the doc now claims it closed: rm -f runs INSIDE the background dispatch task, after the argv-building helper, so a pre-wait read can still see round N-1's status line — risk: a just-launched reviewer is quarantined on the previous round's dispatch exited <rc> — the exact bug r4 raised, now behind prose telling the primary to trust the log unconditionally
• commands/multi-review.md:498-502 — 🟠 med — The rc-non-zero bullet quarantines immediately without consulting the seed, discarding a copy that CHANGED — on-disk findings the rc-zero bullet explicitly calls "recoverable rather than discardable" for the identical on-disk state — risk: a reviewer that wrote its findings and died one step before the flip loses its whole turn, the failure mode fable-rd1-r1 was agreed to prevent, protected here only when the death lands after the flip
• commands/multi-review.md:511-515 — 🟠 med — rc-zero + changed copy defers to the exit-8 handler, whose premise — "demonstrably alive and still writing" — the log has just disproven: the primary spends up to 3 more 600s waits on a process it knows exited, then quarantines with the wrong reason — risk: the ~40-minute wait-on-a-corpse this PR exists to eliminate survives in the rc=0 lane, ending in still writing at the retry cap for a reviewer that stopped writing before the first retry; "recoverable" is never operationalized into an action
• commands/multi-review.md:489-490 — 🟡 low — The status sentinel is spoofable by log content: the log is the CLI's verbatim stdout/stderr, "Look for the line multi-review: dispatch exited <rc>" names no occurrence rule, and in this repo's self-reviews the reviewed material itself contains that exact string (commands/multi-review.md:465,467,489 and any PR diff of them) — a CLI echoing prompt or doc content could plant a false status while still running — risk: a live reviewer is quarantined on a status line its own output quoted rather than the dispatch wrote
• scripts/multi-review-packaging.test.sh:559-561 — 🟡 low — The widened n+30 window has zero margin again: the printf status line sits at exactly the window's last row (heading at commands/multi-review.md:437, printf at 467), reinstating the precise condition the widening comment warns about — the next line added above it pushes assertion (b) out of the window (loudly, but avoidably) — risk: the block breaks on the first unrelated insertion above the printf, and the comment's own rationale ("a window with no margin stops covering...") is contradicted by the chosen value
• scripts/multi-review-packaging.test.sh:649 — 🟡 low — The new stale-log check reuses the label "(c)" already held by the log-consulted check three comments below, so the block reads (a) (b) (c) (c) (d) (e) — risk: cross-references to these subchecks (mutation-table comments, future findings) become ambiguous
• commands/multi-review.md:456-457 — 🟠 med — Ensure the appended exit sentinel starts on its own line; echo does not separate stderr that lacks a final newline. — risk: a crashed reviewer can still be classified as slow
• commands/multi-review.md:482-487 — 🟠 med — The non-zero-rc row quarantines unconditionally, without first checking whether the marker flipped — a reviewer that completed its turn and then exited non-zero is discarded with its findings. — risk: a valid completed turn is quarantined as a crash and its findings never merge
• commands/multi-review.md:459 — 🟠 med — The status line is appended with a bare echo onto whatever the CLI last wrote; a crashing CLI whose final stderr write lacks a trailing newline glues the two into one line, and the "last line is multi-review: dispatch exited <rc>" test then reads as Absent. — risk: the crash detection disarms in exactly the crash case it was built for — the dead reviewer reads as "still running" and burns the full retry budget again
• commands/multi-review.md:483 — 🟠 med — The quarantine reason copies a verbatim log line (<the last non-empty line above that one>) into the coordination doc, which undercuts the Security section's own containment argument that the log is gitignored, local, and never leaves the machine. — risk: credential material printed by a failing-auth CLI propagates from the gitignored log into the durably-recorded quarantine reason and from there into gate output
• commands/multi-review.md:479-480 — 🟡 low — On a round-N re-dispatch the log is truncated only when the background subshell's redirect opens, so "read the log before the first wait" can race it and read the PREVIOUS round's dispatch exited <rc> line, quarantining a just-launched reviewer instantly. — risk: a healthy round-2 reviewer is quarantined at t=0 on round-1's stale crash line
• commands/multi-review.md:489-491 — 🟡 low — The rc-zero row labels every unflipped copy "ran to completion and wrote nothing", but rc 0 + unflipped + copy-changed-since-seed (findings written, flip forgotten) is a distinct state the table gives the wrong name — and the wait's --seed exists precisely to tell those apart. — risk: a copy carrying real findings is recorded as no turn taken, misinforming the gate

Multi-review

Agreed findings (17)
🟠 med — The PR summary still routes a status-present rc=0 copy through multi-review-wait, but the revised step-5 table quarantines that same unflipped byte-identical or changed copy immediately. — risk: the documented change has two incompatible outcomes for a cleanly exited unhanded-off reviewer
🟠 med — "The change" step-5 table is the stale round-1 design: it still keys off the exit code and contradicts the shipped bullets in commands/multi-review.md. — risk: the PR body teaches opposite semantics from the code it describes, in exactly the table round 3 was asked to scrutinize
🟠 med — The LAST-match rule only defuses echoed sentinels after the process dies: while it is still live, an echoed sentinel IS the last match and gets trusted. — risk: a live reviewer whose stdout echoes protocol text is judged exited — bullet 3 then admits a half-finished turn (or bullet 4 quarantines a live reviewer) while it is still writing
🟠 med — A shell reviewer that dies immediately after the pre-wait log read can still consume the first full wait bound before its non-zero sentinel is consulted. — risk: launch crashes can still be misclassified as slow for one bound
🟠 med — The fable-rd1-r4 fix relocates the stale-log race instead of closing it, yet the doc now claims it closed: rm -f runs INSIDE the background dispatch task, after the argv-building helper, so a pre-wait read can still see round N-1's status line — risk: a just-launched reviewer is quarantined on the previous round's dispatch exited <rc> — the exact bug r4 raised, now behind prose telling the primary to trust the log unconditionally
🟠 med — The rc-non-zero bullet quarantines immediately without consulting the seed, discarding a copy that CHANGED — on-disk findings the rc-zero bullet explicitly calls "recoverable rather than discardable" for the identical on-disk state — risk: a reviewer that wrote its findings and died one step before the flip loses its whole turn, the failure mode fable-rd1-r1 was agreed to prevent, protected here only when the death lands after the flip
🟠 med — rc-zero + changed copy defers to the exit-8 handler, whose premise — "demonstrably alive and still writing" — the log has just disproven: the primary spends up to 3 more 600s waits on a process it knows exited, then quarantines with the wrong reason — risk: the ~40-minute wait-on-a-corpse this PR exists to eliminate survives in the rc=0 lane, ending in still writing at the retry cap for a reviewer that stopped writing before the first retry; "recoverable" is never operationalized into an action
🟠 med — Ensure the appended exit sentinel starts on its own line; echo does not separate stderr that lacks a final newline. — risk: a crashed reviewer can still be classified as slow
🟠 med — The non-zero-rc row quarantines unconditionally, without first checking whether the marker flipped — a reviewer that completed its turn and then exited non-zero is discarded with its findings. — risk: a valid completed turn is quarantined as a crash and its findings never merge
🟠 med — The status line is appended with a bare echo onto whatever the CLI last wrote; a crashing CLI whose final stderr write lacks a trailing newline glues the two into one line, and the "last line is multi-review: dispatch exited <rc>" test then reads as Absent. — risk: the crash detection disarms in exactly the crash case it was built for — the dead reviewer reads as "still running" and burns the full retry budget again
🟠 med — The quarantine reason copies a verbatim log line (<the last non-empty line above that one>) into the coordination doc, which undercuts the Security section's own containment argument that the log is gitignored, local, and never leaves the machine. — risk: credential material printed by a failing-auth CLI propagates from the gitignored log into the durably-recorded quarantine reason and from there into gate output
🟡 low — Assertion (d) locates the seeding-step rm -f with a fresh fixed-offset window (sd..sd+20) in the same PR that twice documents fixed offsets shipping zero-margin (22, then 30, fable-rd2-r5). — risk: drift between 'snapshot each copy as' and the rm -f line past 20 rows breaks the check — loudly here, since it is a positive grep — but it re-seeds the drift class the rest of this file just converted to structural bounds
🟡 low — The status sentinel is spoofable by log content: the log is the CLI's verbatim stdout/stderr, "Look for the line multi-review: dispatch exited <rc>" names no occurrence rule, and in this repo's self-reviews the reviewed material itself contains that exact string (commands/multi-review.md:465,467,489 and any PR diff of them) — a CLI echoing prompt or doc content could plant a false status while still running — risk: a live reviewer is quarantined on a status line its own output quoted rather than the dispatch wrote
🟡 low — The widened n+30 window has zero margin again: the printf status line sits at exactly the window's last row (heading at commands/multi-review.md:437, printf at 467), reinstating the precise condition the widening comment warns about — the next line added above it pushes assertion (b) out of the window (loudly, but avoidably) — risk: the block breaks on the first unrelated insertion above the printf, and the comment's own rationale ("a window with no margin stops covering...") is contradicted by the chosen value
🟡 low — The new stale-log check reuses the label "(c)" already held by the log-consulted check three comments below, so the block reads (a) (b) (c) (c) (d) (e) — risk: cross-references to these subchecks (mutation-table comments, future findings) become ambiguous
🟡 low — On a round-N re-dispatch the log is truncated only when the background subshell's redirect opens, so "read the log before the first wait" can race it and read the PREVIOUS round's dispatch exited <rc> line, quarantining a just-launched reviewer instantly. — risk: a healthy round-2 reviewer is quarantined at t=0 on round-1's stale crash line
🟡 low — The rc-zero row labels every unflipped copy "ran to completion and wrote nothing", but rc 0 + unflipped + copy-changed-since-seed (findings written, flip forgotten) is a distinct state the table gives the wrong name — and the wait's --seed exists precisely to tell those apart. — risk: a copy carrying real findings is recorded as no turn taken, misinforming the gate

Disagreements (1)
🟡 low — Bullet 3 directs step-6 channel-check at a copy whose marker still says awaiting-reviewer; if channel-check or the merge step requires the flipped marker, the recovery path fails exactly when it is needed. — risk: the changed-then-died recovery is unrunnable in practice and degrades to the quarantine it was written to avoid — flagged by claude-fable-5; claude-opus-5 disputes: verified against both scripts: channel-check and merge each accept a copy whose marker was never flipped (exit 0, finding ingested), so the recovery path is runnable

———
🤖 Posted by AI agents (claude-opus-5 + gpt-5 + claude-fable-5) via multi-review star review.

[kevin-agrology]

🟠 med — The PR summary still routes a status-present rc=0 copy through multi-review-wait, but the revised step-5 table quarantines that same unflipped byte-identical or changed copy immediately. — risk: the documented change has two incompatible outcomes for a cleanly exited unhanded-off reviewer — 🤖 multi-review star review (gpt-5 + claude-opus-5)

[kevin-agrology]

🟠 med — The LAST-match rule only defuses echoed sentinels after the process dies: while it is still live, an echoed sentinel IS the last match and gets trusted. — risk: a live reviewer whose stdout echoes protocol text is judged exited — bullet 3 then admits a half-finished turn (or bullet 4 quarantines a live reviewer) while it is still writing — 🤖 multi-review star review (claude-fable-5 + claude-opus-5)

[kevin-agrology]

🟡 low — Assertion (d) locates the seeding-step rm -f with a fresh fixed-offset window (sd..sd+20) in the same PR that twice documents fixed offsets shipping zero-margin (22, then 30, fable-rd2-r5). — risk: drift between 'snapshot each copy as' and the rm -f line past 20 rows breaks the check — loudly here, since it is a positive grep — but it re-seeds the drift class the rest of this file just converted to structural bounds — 🤖 multi-review star review (claude-fable-5 + claude-opus-5)