fix(review): give review legs an owner instead of an observer (#590)

.github/workflows/ci.yml

@@ -123,6 +123,9 @@ jobs:
       - name: Run Codex progress wrapper tests (#259)
         run: ./tests/test-codex-progress-wrapper.sh
 
+      - name: Run review-leg launcher tests (#590)
+        run: ./tests/test-run-review-leg.sh
+
       - name: Run session-load audit tests (token bloat phase 2)
         run: ./tests/test-audit-session-load.sh
 

CONTRIBUTING.md

@@ -14,6 +14,7 @@ Thank you for your interest in improving the SDLC Harness!
    ./tests/test-stats.sh && ./tests/test-hooks.sh && \
    ./tests/test-token-spike.sh && \
    ./tests/test-codex-progress-wrapper.sh && \
+   ./tests/test-run-review-leg.sh && \
    ./tests/test-audit-session-load.sh && \
    ./tests/test-compliance.sh && ./tests/test-sdp-calculation.sh && \
    ./tests/test-evaluate-bugs.sh && ./tests/test-evaluate-cli-mode.sh && \
@@ -196,6 +197,7 @@ python3 -c "import yaml; yaml.safe_load(open('.github/workflows/ci.yml'))"
 ./tests/test-hook-stdin-bounded.sh
 ./tests/test-token-spike.sh
 ./tests/test-codex-progress-wrapper.sh
+./tests/test-run-review-leg.sh
 ./tests/test-audit-session-load.sh
 ./tests/test-compliance.sh
 ./tests/test-sdp-calculation.sh

cowork/skills/sdlc/SKILL.md

@@ -155,8 +155,9 @@ PROTOCOL is universal across domains; only `review_instructions` and `verificati
 
 **Handoff/preflight mechanics: wizard doc.** These two stay; improvising them cost real time (#364, #437).
 
-1. **Run reviewer:** `codex exec -c 'model_reasoning_effort="high"' -s danger-full-access -o .reviews/latest-review.md "<prompt>" < /dev/null`. Always `high`, `run_in_background: true` + `dangerouslyDisableSandbox: true`, always `< /dev/null`. **Why:** `< /dev/null` prevents a stdin hang at 0% CPU; background avoids the Bash 10-min cap that kills foreground codex (bundles run 5–30 min). Foreground burned 70 min on a 7-min review (#364).
-2. **Dialogue loop:** per-finding response (`{"finding":"1","action":"FIXED|DISPUTED|ACCEPTED","summary":"..."}` in `.reviews/response.json`). Bump round, set `PENDING_RECHECK`, add `fixes_applied` (numbered, file:line), and write `"branch": "<git symbolic-ref --short HEAD>"` — the gate (#533) only lets an in-flight round commit on the branch it declares, so an undeclared round cannot save its work and lands the reviewer back on a mutable tree. Recheck prompt: "TARGETED RECHECK. FIXED → verify certify condition. DISPUTED → ACCEPT if sound, REJECT with reasoning. ACCEPTED → verify applied. Report every defect at any severity; don't hunt new surfaces. A finding blocks only if it is P0/P1 AND against a REQUESTED behavior — and a finding showing a requested behavior is wrong IS P1, whatever label it arrived with." **NEVER unilaterally dismiss** — run the recheck; the reviewer may accept your dispute or counter with evidence you missed. **On CERTIFIED write `"commit_sha": "<git rev-parse HEAD>"` into `handoff.json`** — the gate hook (#437) treats a missing/mismatched SHA as stale, not just the status string.
+1. **Run reviewer through a launcher that owns the leg**, as a background task (`run_in_background: true` + `dangerouslyDisableSandbox: true`). The launcher runs `codex exec -c 'model_reasoning_effort="high"' -s danger-full-access "<prompt>" >> <output> 2>&1 < /dev/null` and exits with codex's own status. Redirect the transcript; do **not** also pass `-o <output>` — pointing both at one path makes codex truncate most of the redirected transcript and duplicate the final response. Always `high`, always background, always `< /dev/null` — and give the child that redirect inside the launcher, so it holds no matter what the caller inherited. **Why:** `< /dev/null` prevents a stdin hang at 0% CPU; background avoids the Bash 10-min cap that kills foreground codex (bundles run 5–30 min). Foreground burned 70 min on a 7-min review (#364).
+2. **The launcher's exit status is the verdict. Do not build a second observer to reconstruct it.** 0 completed, non-zero failed — read the output for the failure and relaunch. Nothing by your deadline: inspect and relaunch, never keep waiting. **Before reusing an output path, cancel the owned background task and await its termination; if termination cannot be confirmed, relaunch to a unique per-attempt output file.** A relaunch truncates that path, and an old leg that is still alive keeps appending into it — two transcripts interleave and both legs report success (measured against codex 0.147.0). A leg started outside a launcher has no owner and no status, so its fate is unknown *immediately* — relaunch it through one rather than waiting on it. **Why:** by wall clock a hang is identical to a slow review, so it gets waited on — 51 and 28 minutes on 2026-08-13, the loop's "still working or done?" decision made twice against a process that never started (#590, #341). Two attempts to detect that from outside were falsified by running code: the output file cannot tell you (`-o` carries no completion marker; `tokens used` appears in the echoed prompt of a leg that *crashed*; an fd reported as a pipe may be at EOF and healthy; the hang's byte signature differs per machine — 39 on one, 143 on another), and pid/status sidecars cannot either (the child exits before a status is published, so a *successful* leg reads as dead; killing only the launcher leaves a status that never arrives; a stale status or a reused pid is indistinguishable from a live one). Never treat a quiet leg as progress — a gate that can hang invisibly is a gate that gets assumed passed.
+3. **Dialogue loop:** per-finding response (`{"finding":"1","action":"FIXED|DISPUTED|ACCEPTED","summary":"..."}` in `.reviews/response.json`). Bump round, set `PENDING_RECHECK`, add `fixes_applied` (numbered, file:line), and write `"branch": "<git symbolic-ref --short HEAD>"` — the gate (#533) only lets an in-flight round commit on the branch it declares, so an undeclared round cannot save its work and lands the reviewer back on a mutable tree. Recheck prompt: "TARGETED RECHECK. FIXED → verify certify condition. DISPUTED → ACCEPT if sound, REJECT with reasoning. ACCEPTED → verify applied. Report every defect at any severity; don't hunt new surfaces. A finding blocks only if it is P0/P1 AND against a REQUESTED behavior — and a finding showing a requested behavior is wrong IS P1, whatever label it arrived with." **NEVER unilaterally dismiss** — run the recheck; the reviewer may accept your dispute or counter with evidence you missed. **On CERTIFIED write `"commit_sha": "<git rev-parse HEAD>"` into `handoff.json`** — the gate hook (#437) treats a missing/mismatched SHA as stale, not just the status string.
 
 **Convergence — TWO PASSES PER FROZEN SCOPE, COUNTED CUMULATIVELY PER ROOT TASK.** One review, one verify (verify reads only the diff since last verdict). A fix adding code or promises beyond the reviewed diff is NEW SCOPE — log continue/stop + cost in `handoff.json` `scope_decisions` BEFORE the pass; single driver records its own, next pass ratifies; human only on a cross-model split. **The count is cumulative and never resets when scope is re-frozen** — record `root_task` (the verbatim request) and `base_sha` once, then let `round` accumulate across every re-freeze; new scopes **append** to `scope_decisions`, never replace it. Re-freezing after each fix is how two-per-scope becomes unlimited (#520: 20 rounds). This is accounting, not a cap: continuing stays the recorded decision's call. **Blame the line:** a blocker in code nobody asked for — cut it, don't repair it. Review committed SHAs, not a mutable tree. #520: 20 rounds, 46 lines.
 **Loop autonomy — no per-round check-ins.** While a pass is owed, don't hand the turn back: fix, push, launch it in the SAME turn. Ending a turn with no pending work IS a stop decision; cite **CONVERGED** (verdicts in on head SHA, zero unresolved in-allowlist), **DEADLOCK** (finding unmoved after 2 rechecks, or non-waivable gate after 2 attempts), **BOUND** (context ceiling, or gate needing a human), or **SCOPE** (two passes used — recorded decision required). Every pass carries a delta; resubmitting unchanged = reviewer-shopping.

scripts/run-review-leg.sh

@@ -0,0 +1,76 @@
+#!/bin/bash
+# Launch a cross-model review leg so it cannot hang, and so its fate is its
+# exit status (#590).
+#
+# Usage:
+#   scripts/run-review-leg.sh OUTPUT_FILE PROMPT [EXTRA_CODEX_ARGS...]
+#
+# Run it as a background task. The task's exit status IS the leg's verdict:
+# 0 completed, non-zero failed, nothing by your deadline means inspect and
+# relaunch. There is no separate waiter to consult and nothing to poll.
+#
+# ---------------------------------------------------------------------------
+# THE FAILURE
+#
+# `codex exec` reads stdin to EOF and appends it to the argv prompt — argv and
+# stdin are concatenated, not alternatives. Handed a pipe nobody closes, it
+# blocks on that read BEFORE contacting the model, forever. On 2026-08-13 two
+# review legs were waited on for 51 and 28 minutes; the loop's "still working,
+# or done?" decision was made twice against a process that had never started.
+#
+# ---------------------------------------------------------------------------
+# WHY THERE IS NO WAITER HERE, AFTER TWO ATTEMPTS AT ONE
+#
+# The first attempt watched the output file and the process table from
+# outside. Sol (GPT-5.6 high) falsified every signal with running code: `-o`
+# output carries no completion marker; `tokens used` appears in codex's echoed
+# prompt, so a crashed leg read as complete; an fd reported as PIPE may be at
+# EOF and healthy; the hang's byte signature differs per machine (39 here, 143
+# on another); and enumerating holders of the file counted readers, so a
+# `tail -f` flipped the verdict.
+#
+# The second attempt recorded the child's pid and exit status in sidecar
+# files for a waiter to read. Sol falsified that too: the child exits before
+# the status is published, so a successful leg reads as dead; killing only the
+# launcher leaves a status that can never arrive while the child pid still
+# looks alive; a stale status from a previous run is read as this run's; and a
+# reused pid is indistinguishable from the original process.
+#
+# Both are the same mistake in different clothes — a SECOND OBSERVER trying to
+# reconstruct what the first process already knows. The launcher's own exit
+# status is that knowledge, delivered by the process that has it. So the
+# waiter is gone, and the sidecars with it: with no reader they were only a
+# way to be stale.
+#
+# What remains is the part that actually prevents the failure: the child gets
+# /dev/null on stdin, whatever this script inherited. Verified against real
+# codex 0.147.0 with this script's own stdin on an unclosed fifo — the shape
+# that reproduces a live hang — the leg completed normally.
+#
+# A leg typed ad hoc, outside this script, has no owner and no status. Its
+# fate is unknown immediately; relaunch it through here rather than waiting on
+# it. That is the one case detection was ever needed for, and it is prose in
+# the skill, not a signal any code can soundly reconstruct.
+
+set -e
+
+if [ $# -lt 2 ]; then
+    echo "usage: $0 OUTPUT_FILE PROMPT [EXTRA_CODEX_ARGS...]" >&2
+    exit 64
+fi
+
+OUTPUT="$1"
+shift
+
+: > "$OUTPUT"
+
+# `< /dev/null` is the whole prevention: the child gets EOF immediately and
+# proceeds to the model. Keep it on this line — it is not incidental.
+#
+# exec replaces this shell, so the caller sees codex's own exit status with
+# nothing in between that could die separately and leave the status unknown.
+exec codex exec \
+    -c 'model_reasoning_effort="high"' \
+    -s danger-full-access \
+    "$@" \
+    >> "$OUTPUT" 2>&1 < /dev/null

skills/sdlc/SKILL.md

@@ -155,8 +155,9 @@ PROTOCOL is universal across domains; only `review_instructions` and `verificati
 
 **Handoff/preflight mechanics: wizard doc.** These two stay; improvising them cost real time (#364, #437).
 
-1. **Run reviewer:** `codex exec -c 'model_reasoning_effort="high"' -s danger-full-access -o .reviews/latest-review.md "<prompt>" < /dev/null`. Always `high`, `run_in_background: true` + `dangerouslyDisableSandbox: true`, always `< /dev/null`. **Why:** `< /dev/null` prevents a stdin hang at 0% CPU; background avoids the Bash 10-min cap that kills foreground codex (bundles run 5–30 min). Foreground burned 70 min on a 7-min review (#364).
-2. **Dialogue loop:** per-finding response (`{"finding":"1","action":"FIXED|DISPUTED|ACCEPTED","summary":"..."}` in `.reviews/response.json`). Bump round, set `PENDING_RECHECK`, add `fixes_applied` (numbered, file:line), and write `"branch": "<git symbolic-ref --short HEAD>"` — the gate (#533) only lets an in-flight round commit on the branch it declares, so an undeclared round cannot save its work and lands the reviewer back on a mutable tree. Recheck prompt: "TARGETED RECHECK. FIXED → verify certify condition. DISPUTED → ACCEPT if sound, REJECT with reasoning. ACCEPTED → verify applied. Report every defect at any severity; don't hunt new surfaces. A finding blocks only if it is P0/P1 AND against a REQUESTED behavior — and a finding showing a requested behavior is wrong IS P1, whatever label it arrived with." **NEVER unilaterally dismiss** — run the recheck; the reviewer may accept your dispute or counter with evidence you missed. **On CERTIFIED write `"commit_sha": "<git rev-parse HEAD>"` into `handoff.json`** — the gate hook (#437) treats a missing/mismatched SHA as stale, not just the status string.
+1. **Run reviewer through a launcher that owns the leg**, as a background task (`run_in_background: true` + `dangerouslyDisableSandbox: true`). The launcher runs `codex exec -c 'model_reasoning_effort="high"' -s danger-full-access "<prompt>" >> <output> 2>&1 < /dev/null` and exits with codex's own status. Redirect the transcript; do **not** also pass `-o <output>` — pointing both at one path makes codex truncate most of the redirected transcript and duplicate the final response. Always `high`, always background, always `< /dev/null` — and give the child that redirect inside the launcher, so it holds no matter what the caller inherited. **Why:** `< /dev/null` prevents a stdin hang at 0% CPU; background avoids the Bash 10-min cap that kills foreground codex (bundles run 5–30 min). Foreground burned 70 min on a 7-min review (#364).
+2. **The launcher's exit status is the verdict. Do not build a second observer to reconstruct it.** 0 completed, non-zero failed — read the output for the failure and relaunch. Nothing by your deadline: inspect and relaunch, never keep waiting. **Before reusing an output path, cancel the owned background task and await its termination; if termination cannot be confirmed, relaunch to a unique per-attempt output file.** A relaunch truncates that path, and an old leg that is still alive keeps appending into it — two transcripts interleave and both legs report success (measured against codex 0.147.0). A leg started outside a launcher has no owner and no status, so its fate is unknown *immediately* — relaunch it through one rather than waiting on it. **Why:** by wall clock a hang is identical to a slow review, so it gets waited on — 51 and 28 minutes on 2026-08-13, the loop's "still working or done?" decision made twice against a process that never started (#590, #341). Two attempts to detect that from outside were falsified by running code: the output file cannot tell you (`-o` carries no completion marker; `tokens used` appears in the echoed prompt of a leg that *crashed*; an fd reported as a pipe may be at EOF and healthy; the hang's byte signature differs per machine — 39 on one, 143 on another), and pid/status sidecars cannot either (the child exits before a status is published, so a *successful* leg reads as dead; killing only the launcher leaves a status that never arrives; a stale status or a reused pid is indistinguishable from a live one). Never treat a quiet leg as progress — a gate that can hang invisibly is a gate that gets assumed passed.
+3. **Dialogue loop:** per-finding response (`{"finding":"1","action":"FIXED|DISPUTED|ACCEPTED","summary":"..."}` in `.reviews/response.json`). Bump round, set `PENDING_RECHECK`, add `fixes_applied` (numbered, file:line), and write `"branch": "<git symbolic-ref --short HEAD>"` — the gate (#533) only lets an in-flight round commit on the branch it declares, so an undeclared round cannot save its work and lands the reviewer back on a mutable tree. Recheck prompt: "TARGETED RECHECK. FIXED → verify certify condition. DISPUTED → ACCEPT if sound, REJECT with reasoning. ACCEPTED → verify applied. Report every defect at any severity; don't hunt new surfaces. A finding blocks only if it is P0/P1 AND against a REQUESTED behavior — and a finding showing a requested behavior is wrong IS P1, whatever label it arrived with." **NEVER unilaterally dismiss** — run the recheck; the reviewer may accept your dispute or counter with evidence you missed. **On CERTIFIED write `"commit_sha": "<git rev-parse HEAD>"` into `handoff.json`** — the gate hook (#437) treats a missing/mismatched SHA as stale, not just the status string.
 
 **Convergence — TWO PASSES PER FROZEN SCOPE, COUNTED CUMULATIVELY PER ROOT TASK.** One review, one verify (verify reads only the diff since last verdict). A fix adding code or promises beyond the reviewed diff is NEW SCOPE — log continue/stop + cost in `handoff.json` `scope_decisions` BEFORE the pass; single driver records its own, next pass ratifies; human only on a cross-model split. **The count is cumulative and never resets when scope is re-frozen** — record `root_task` (the verbatim request) and `base_sha` once, then let `round` accumulate across every re-freeze; new scopes **append** to `scope_decisions`, never replace it. Re-freezing after each fix is how two-per-scope becomes unlimited (#520: 20 rounds). This is accounting, not a cap: continuing stays the recorded decision's call. **Blame the line:** a blocker in code nobody asked for — cut it, don't repair it. Review committed SHAs, not a mutable tree. #520: 20 rounds, 46 lines.
 **Loop autonomy — no per-round check-ins.** While a pass is owed, don't hand the turn back: fix, push, launch it in the SAME turn. Ending a turn with no pending work IS a stop decision; cite **CONVERGED** (verdicts in on head SHA, zero unresolved in-allowlist), **DEADLOCK** (finding unmoved after 2 rechecks, or non-waivable gate after 2 attempts), **BOUND** (context ceiling, or gate needing a human), or **SCOPE** (two passes used — recorded decision required). Every pass carries a delta; resubmitting unchanged = reviewer-shopping.