feat(adversarial-review): make self-collect path multi-turn so it can actually work

docs/superpowers/specs/2026-06-08-fix-status-on-soft-error-design.md

@@ -0,0 +1,190 @@
+# Design: fix recovered-turn status being mis-marked as failed
+
+Date: 2026-06-08
+Repo: /Users/kentpeng/projects/codex-plugin-cc
+Branch: feat/codex-self-collect-multiturn (PR #328 → openai:main)
+Source brainstorm: docs/fix-status-on-soft-error.md
+
+## Problem
+
+Commit `d222542` on this PR changed the shared `buildResultStatus`
+(`plugins/codex/scripts/lib/codex.mjs:740`) to return non-zero whenever a turn
+recorded *any* `error` — including a transient one the turn recovered from:
+
+```js
+function buildResultStatus(turnState) {
+  if (turnState.error) return 1;        // added by d222542
+  return turnState.finalTurn?.status === "completed" ? 0 : 1;
+}
+```
+
+The app-server multiplexes transient retry notices (e.g. `Reconnecting... 1/5`)
+onto the same `error` notification channel as fatal turn failures, and the
+capture state records the last one seen **without clearing it**. So a turn can
+simultaneously have `turnState.error` set (stale transient notice) AND
+`finalTurn.status === "completed"` with a valid `lastAgentMessage`. After
+`d222542`, `buildResultStatus` returns 1 for that recovered turn.
+
+### Who compensates, who doesn't (verified against current code)
+
+| Path | State | Location |
+|------|-------|----------|
+| Adversarial review | COMPENSATED | `executeReviewRun` codex-companion.mjs:575 — exits 0 when a valid parsed verdict exists |
+| Investigation runner | COMPENSATED internally | codex.mjs:1218 — aborts only when `error && !turnRecovered` |
+| **/codex:task** | **NOT compensated** | `executeTaskRun` codex-companion.mjs:655 returns `exitStatus: result.status` raw |
+| **Native /codex:review** | **NOT compensated** | `executeReviewRun` Review branch codex-companion.mjs:414 returns `exitStatus: result.status` raw |
+
+### Blast radius
+
+1. `runTrackedJob` (lib/tracked-jobs.mjs:156): `exitStatus !== 0` ⇒ job recorded
+   as **failed**, foreground command exits non-zero.
+2. Stop-review-gate hook (scripts/stop-review-gate-hook.mjs:120): keys off
+   `result.status !== 0` and returns a "task failed" block — without ever
+   parsing the `ALLOW:`/`BLOCK:` answer the model produced. A recovered gate
+   review = **false-positive session block**.
+
+### Key nuance found during brainstorm
+
+The three callers do not share one definition of "usable output":
+
+- task → `result.finalMessage`
+- native review → `result.reviewText`
+- adversarial review → a *parsed structured verdict* (`parsed.parsed`), stricter
+  than "any message present", and it reuses `result.status` as its failure
+  fallback.
+
+Therefore pushing the fix down into `buildResultStatus`/the runners is **not**
+free: a recovered-but-unparseable adversarial run would flip exit 1 → 0 (a new
+regression) unless the adversarial fallback were also tightened. We avoid that
+by fixing at the caller layer with a shared helper, leaving `result.status`
+semantics and the already-correct adversarial path untouched.
+
+## Required behavior
+
+A turn that **completed with a usable result** (`finalTurn.status ===
+"completed"` and usable output present) but recorded a transient `error` must be
+treated as SUCCESS: exit 0, job recorded "completed", gate proceeds to parse the
+answer. A turn that genuinely failed (no usable output, or `finalTurn.status !==
+"completed"`) keeps non-zero status.
+
+## Chosen approach: shared caller-level helper (A2)
+
+Add the recovery rule in ONE named place and apply it at the two un-compensated
+caller sites. Do not touch `buildResultStatus`, the runners, or the adversarial
+path.
+
+### `plugins/codex/scripts/lib/codex.mjs` — new function
+
+```js
+function resolveRunExitStatus(result, usableText) {
+  const recovered = result.turn?.status === "completed"
+    && Boolean(String(usableText ?? "").trim());
+  return recovered ? 0 : result.status;
+}
+```
+
+Both runners already return `turn: turnState.finalTurn` (codex.mjs:1040 and
+:1109), so `result.turn?.status` is available on both paths.
+
+### `plugins/codex/scripts/codex-companion.mjs` — three sites
+
+- `executeTaskRun` (:655):
+  `exitStatus: resolveRunExitStatus(result, result.finalMessage)`, and set
+  `payload.status` to the same resolved value (removes the JSON inconsistency
+  where a recovered/success task still reports `status: 1`).
+- Native-review branch (:414):
+  `exitStatus: resolveRunExitStatus(result, result.reviewText)`, and set
+  `payload.codex.status` to the same resolved value.
+- Adversarial branch (:575): **unchanged** — already correct.
+
+### Rendering — no change needed (verified)
+
+- `renderTaskResult` (render.mjs:350) prefers `rawOutput` (= `finalMessage`) and
+  only falls back to `failureMessage` when it is empty. A recovered task renders
+  its real answer, not the stale `Reconnecting...` notice.
+- `renderNativeReviewResult` (render.mjs:323) prefers `stdout` (= `reviewText`)
+  regardless of `status`.
+
+### Stop-gate hook — no change needed (verify by test)
+
+The hook keys off the child process exit status (`result.status` in
+stop-review-gate-hook.mjs:120). Once the task exits 0 on recovery, the hook
+proceeds to parse the `ALLOW:`/`BLOCK:` answer. This propagation must be
+confirmed by test C, not assumed.
+
+## Test plan (`node --test`, TDD: write failing tests first)
+
+### A. Task path — queue-driven fixture
+Mirror `"finalize turn that recovered from a transient reconnect keeps its valid
+verdict (e2e)"` in tests/investigation.test.mjs, but drive `task --json`.
+
+1. Recovered = success: queue
+   `{ finalAnswer: { text: "ALLOW: looks fine" }, turnError: { message: "Reconnecting... 1/5" } }`,
+   run companion `task --json`. Assert `result.status === 0` (process exit),
+   `payload.rawOutput` contains the answer, `payload.status === 0` (the
+   normalized JSON field).
+2. Genuine-failure guard: a turn with `turnError` AND no `finalMessage` (or
+   `finalTurn` not completed) must still exit non-zero.
+
+### B. Native review path — same fixture, `review` branch
+Recovered turn with `reviewText` present + `turnError` ⇒ `exitStatus 0`; a
+failure turn with no `reviewText` ⇒ non-zero. Confirms `resolveRunExitStatus`
+uses `reviewText`, not `finalMessage`.
+
+### C. Stop-gate hook — `installFakeCodex` named-scenario harness (runtime.test.mjs)
+The existing gate tests use `installFakeCodex(binDir, behavior)` named scenarios,
+not the queue-driven fixture. The queue-driven path already emits `entry.turnError`
+as an `error` notification (tests/fake-codex-fixture.mjs ~413); extend the
+named-scenario path with an equivalent switch (or add a `stop-gate-recovered`
+scenario) that emits `error` notice + valid agent message + `turn/completed`.
+
+- Assert: a recovered gate task yields `ok:true` and the ALLOW/BLOCK answer is
+  parsed — NOT a "task failed" block.
+- Keep the existing `"... blocks on findings"` and `"... allows ... when clean"`
+  gate tests green.
+
+### D. Regression guards (must stay green)
+- Existing adversarial `"recovered finalize keeps its valid verdict"` (confirms
+  A2 did not touch the adversarial path).
+- Idle-watchdog tests (a genuine idle timeout must still be a failure).
+
+## Verification before done
+
+- `node --test tests/*.test.mjs` — full suite. Known PRE-EXISTING failures
+  unrelated to this work (NOT regressions): `status shows phases, hints, and the
+  latest finished job`, `status preserves adversarial review kind labels`,
+  `result returns the stored output for the latest finished job by default`,
+  `resolveStateDir uses a temp-backed per-workspace directory`. Net new failures
+  must be zero.
+- Confirm the suite EXITS CLEANLY (no hang). Do not abandon turns in tests —
+  always let them settle with the fixture's normal queued responses.
+
+## Deploy to the live local install (after merge-ready)
+
+The running plugin is the CACHE build, not this repo. Copy the changed files to
+`~/.claude/plugins/cache/openai-codex/codex/1.0.4/scripts/...` (back up first;
+mapping in memory `codex-plugin-runtime-source`).
+
+## Out of scope (track separately; do NOT bundle)
+
+`?? ""`-empty-string family; native review missing idle watchdog / empty-diff
+short-circuit; `--turn-idle-timeout` upper bound; `runAppServerInvestigation`
+`truncated` mislabel at 0 commands; uncommitted dead `runAppServerTurn` import in
+tests; stale `DEFAULT_INLINE_DIFF_MAX_FILES` comment.
+
+**Promoted to a separate work item (own brainstorm → spec → plan):** the
+investigation-loop turn-lifecycle race. The recon loop advances to the finalize
+turn on an *inferred* completion (`scheduleInferredCompletion`, codex.mjs:393)
+that fires on the first `final_answer`-phase message — which can be a "ready to
+finalize" readiness cue, not the real end of the turn. The finalize `turn/start`
+is then dispatched while the app-server's recon turn is still active; the server
+queues it and never opens it, the real verdict streams under the recon turn and
+is discarded, and the turn hangs until the 180s idle watchdog aborts. This
+bundles **Defect A** (premature finalize dispatch; fix = wait for a real
+`turn/completed`, demote inference to a guarded fallback) with **Defect B**
+(`captureTurn` `armIdle()` runs before the `belongsToTurn` filter, codex.mjs:631,
+so orphaned cross-turn traffic re-arms the captured turn's watchdog and masks the
+stuck turn). Both live in `captureTurn`/the idle-watchdog subsystem; fix them
+together, NOT in this status-fix change. Evidence: live run thread
+`019ea5d4-0c53-75f1-9032-5573d18cd878` in `~/.codex/logs_2.sqlite` (2026-06-08
+~06:21).

plugins/codex/commands/adversarial-review.md

@@ -1,6 +1,6 @@
 ---
 description: Run a Codex review that challenges the implementation approach and design choices
-argument-hint: '[--wait|--background] [--base <ref>] [--scope auto|working-tree|branch] [focus ...]'
+argument-hint: '[--wait|--background] [--base <ref>] [--scope auto|working-tree|branch] [--max-investigation-turns N] [--turn-idle-timeout SECONDS] [focus ...]'
 disable-model-invocation: true
 allowed-tools: Read, Glob, Grep, Bash(node:*), Bash(git:*), AskUserQuestion
 ---
@@ -43,6 +43,8 @@ Argument handling:
 - It supports working-tree review, branch review, and `--base <ref>`.
 - It does not support `--scope staged` or `--scope unstaged`.
 - Unlike `/codex:review`, it can still take extra focus text after the flags.
+- For very large diffs that exceed the inline threshold, Codex investigates the diff with read-only commands across multiple turns. Use `--max-investigation-turns N` (default 10) to raise or lower the cap.
+- If a turn stalls with no output for `--turn-idle-timeout SECONDS` (default 180), the run aborts gracefully with a clear failure instead of hanging. Lower it to fail faster on a flaky connection; raise it for very slow turns.
 
 Foreground flow:
 - Run:

plugins/codex/prompts/adversarial-review-finalize.md

@@ -0,0 +1,54 @@
+<role>
+You have just completed an investigation of a code change. Now produce the structured adversarial review.
+</role>
+
+<task>
+Based on your investigation in the prior turns of this thread, write up your findings as a structured review.
+Target: {{TARGET_LABEL}}
+User focus: {{USER_FOCUS}}
+</task>
+
+<finding_bar>
+Report only material findings.
+Do not include style feedback, naming feedback, low-value cleanup, or speculative concerns without evidence.
+A finding should answer:
+1. What can go wrong?
+2. Why is this code path vulnerable?
+3. What is the likely impact?
+4. What concrete change would reduce the risk?
+</finding_bar>
+
+<structured_output_contract>
+This is the finalization turn. Do NOT run any shell commands or tool calls in this turn — your investigation is already complete and you have all the context you need from the prior turns of this thread.
+Return only valid JSON matching the provided schema. Your entire output must be that JSON — no prose before or after, no shell commands, no tool-call payloads.
+Keep the output compact and specific.
+Use `needs-attention` if there is any material risk worth blocking on.
+Use `approve` only if you cannot support any substantive adversarial finding from your investigation.
+Every finding must include:
+- the affected file
+- `line_start` and `line_end`
+- a confidence score from 0 to 1
+- a concrete recommendation
+Write the summary like a terse ship/no-ship assessment, not a neutral recap.
+</structured_output_contract>
+
+<grounding_rules>
+Be aggressive, but stay grounded.
+Every finding must be defensible from what you read during the investigation.
+Do not invent files, lines, code paths, incidents, attack chains, or runtime behavior you cannot support.
+If a conclusion depends on an inference, state that explicitly in the finding body and keep the confidence honest.
+</grounding_rules>
+
+<calibration_rules>
+Prefer one strong finding over several weak ones.
+Do not dilute serious issues with filler.
+If the change looks safe, say so directly and return no findings.
+</calibration_rules>
+
+<final_check>
+Before finalizing, check that each finding is:
+- adversarial rather than stylistic
+- tied to a concrete code location
+- plausible under a real failure scenario
+- actionable for an engineer fixing the issue
+</final_check>

plugins/codex/prompts/adversarial-review-investigate.md

@@ -0,0 +1,48 @@
+<role>
+You are Codex performing an adversarial software review.
+Your job is to break confidence in the change, not to validate it.
+This is the investigation phase: gather evidence with read-only commands before producing any structured output.
+</role>
+
+<task>
+Investigate the change so you can later produce a confident adversarial assessment.
+Target: {{TARGET_LABEL}}
+User focus: {{USER_FOCUS}}
+</task>
+
+<operating_stance>
+Default to skepticism.
+Assume the change can fail in subtle, high-cost, or user-visible ways until the evidence says otherwise.
+Do not give credit for good intent, partial fixes, or likely follow-up work.
+If something only works on the happy path, treat that as a real weakness.
+</operating_stance>
+
+<attack_surface>
+Prioritize the kinds of failures that are expensive, dangerous, or hard to detect:
+- auth, permissions, tenant isolation, and trust boundaries
+- data loss, corruption, duplication, and irreversible state changes
+- rollback safety, retries, partial failure, and idempotency gaps
+- race conditions, ordering assumptions, stale state, and re-entrancy
+- empty-state, null, timeout, and degraded dependency behavior
+- version skew, schema drift, migration hazards, and compatibility regressions
+- observability gaps that would hide failure or make recovery harder
+</attack_surface>
+
+<investigation_method>
+Use read-only shell commands to inspect the diff and the surrounding code.
+Useful starting points: `git diff`, `git log`, `git show`, `git blame`, `cat`, `rg`/`grep`.
+Read the changed files, follow references, and confirm or refute hypotheses with evidence from the code.
+Do not modify any files. Your sandbox is read-only.
+{{REVIEW_COLLECTION_GUIDANCE}}
+</investigation_method>
+
+<convergence>
+Continue investigating until you can defend a confident adversarial assessment.
+When you have seen enough, emit a brief summary message describing what you found and stop running commands.
+A summary message with no further command calls signals that you are ready for the finalization phase.
+Do not produce a structured review yet — that comes in the next phase.
+</convergence>
+
+<repository_context>
+{{REVIEW_INPUT}}
+</repository_context>

plugins/codex/prompts/adversarial-review.md

@@ -46,7 +46,8 @@ A finding should answer:
 </finding_bar>
 
 <structured_output_contract>
-Return only valid JSON matching the provided schema.
+This is a single-turn review. Do NOT run any shell commands or tool calls — the file contents you need are already embedded in the repository_context block below. Do not emit a tool-use stub like `{"cmd": "..."}` instead of the review; that is not the schema.
+Return only valid JSON matching the provided schema. Your entire output must be that JSON — no prose before or after, no shell commands, no tool-call payloads.
 Keep the output compact and specific.
 Use `needs-attention` if there is any material risk worth blocking on.
 Use `approve` only if you cannot support any substantive adversarial finding from the provided context.