diff --git a/docs/DESIGN.md b/docs/DESIGN.md new file mode 100644 index 0000000..16d1f3c --- /dev/null +++ b/docs/DESIGN.md @@ -0,0 +1,1082 @@ +# Design Notes — adversarial-review + +This document records the *why* behind the current implementation of the +`adversarial-review` skill: the empirical facts about external tools it +depends on, the rationale for each non-obvious design decision, and the +alternatives that were considered and rejected. Read `SKILL.md` for +*what* the skill does step by step; read this file when you need to +understand *why it does it that way* before changing something. + +--- + +## §0. Purpose and audience + +### Why this file exists + +The skill orchestrates two AI systems (Claude as the lead, OpenAI Codex +as the external reviewer) through a CLI subprocess interface. The +observable behavior of the skill depends on details of the Codex CLI +(stream splitting, exit codes, flag support), on details of the +Claude Code harness (Bash-tool output size, Opus interpretation of +instructions), and on a set of trade-offs between token cost, +robustness, and operator ergonomics. Those details drift with each new +Codex release and each new Claude model version. A maintainer reading +only `SKILL.md` sees the instructions but not which facts are +load-bearing and which are historical accidents — and so may undo a +subtle fix during a refactor. + +This file fixes that. It names the facts, the decisions, the rejected +alternatives, and — importantly — the prior diagnostic reports that +turned out to be wrong. The goal is that a contributor six months from +now can change the skill confidently instead of re-discovering the +terrain. + +### Audiences + +1. **A future Claude** resuming work on the skill in a fresh session. + It knows Claude Code in general but has no memory of the discussion + that produced the current design. +2. **A human developer** who knows git and bash but does not know the + Claude Code internals or the Codex CLI quirks. +3. **A contributor** who wants to add a feature (new reviewer backend, + CI integration) and needs to know what invariants to preserve. + +### What this file is NOT + +- Not a user guide — that is `README.md`. +- Not authoritative on current behavior — `SKILL.md` is. This file + explains why `SKILL.md` is written the way it is. If the two + disagree, `SKILL.md` wins; fix this file. + +### How to use this file + +If you are about to modify the skill: + +1. Skim `§1. System context` to recall the flow. +2. Find the step you want to change in `§4. Design decisions` — each + decision lists which `SKILL.md` step it ties into. +3. Check `§2. Codex CLI empirical facts` for the environment + assumptions you are about to lean on. If the date in `§8. Version + and verification log` is older than a few Codex releases, re-verify + before trusting. +4. Run `§7. Smoke test protocol` before and after your change. If the + before-run already fails, stop and investigate — don't layer a + change on top of undetected drift. + +--- + +## §1. System context + +### One paragraph + +`adversarial-review` is a Claude Code skill. The user types +`/adversarial-review` in a Claude Code session; Claude (the "lead") +writes an adversarial review prompt to a temp file, launches +`codex exec` as a subprocess with the prompt on stdin, and captures the +reviewer's response to a `-o` file. Claude then shows the review to the +user verbatim, applies fixes to the plan or code, and re-submits the +revised state to the reviewer through `codex exec resume`. The loop +runs up to five rounds, or until the reviewer emits +`VERDICT: APPROVED`. + +### Roles + +- **Lead (Claude).** Orchestrator. Reads `SKILL.md`, runs the Bash / + Write / Read / Edit tools, authors the fixes, decides when to stop. +- **Reviewer (Codex).** External AI process invoked per round. Receives + the adversarial prompt, reads repo/plan content in a read-only + sandbox, emits a structured review with `VERDICT:`. +- **User.** Reads the verbatim review Claude shows each round, + accepts/rejects the skill's final result. + +### Flow + +```mermaid +flowchart TD + A[launch: codex exec --json] --> B{checks: exit, stderr, review file} + B -- fail --> RETRY{retry budget} + RETRY -- yes --> A + RETRY -- no --> ABORT([abort, leave temp files]) + B -- ok --> SHOW[show review verbatim to user] + SHOW --> V{VERDICT} + V -- APPROVED --> APPROVED([Step 8: approved]) + V -- max rounds --> MAX([Step 8: max reached]) + V -- REVISE --> FIX[apply fixes] + FIX --> RESUME[resume: codex exec resume --json] + RESUME --> C{checks: exit, stderr, review file} + C -- ok --> SHOW + C -- fail --> FB{fallback} + FB -- interactive --> ASK[ask user: fresh exec or conclude] + FB -- headless --> SEV{max severity} + ASK -- fresh --> FRESH[fresh codex exec + conv history] + ASK -- conclude --> NOTVER([Step 8: NOT VERIFIED]) + SEV -- critical/high --> FRESH + SEV -- medium only --> NOTVER + FRESH --> B +``` + +The diagram collapses retries and round counting; for the exact +ordering see the strict check lists in `SKILL.md` Steps 4 and 7. + +### Why an external reviewer at all + +An adversarial review from the *same* model as the writer tends toward +validation bias. Running the review through a different model family +(GPT via Codex CLI) reduces shared blind spots. The cost is an external +dependency and a CLI-level integration — which is exactly what most of +this document exists to manage. + +--- + +## §2. Codex CLI empirical facts (verified on v0.121.0) + +All facts in this section were verified on `codex-cli 0.121.0` on +2026-04-17 (see `§8. Version and verification log`). If you are reading +this more than a few Codex releases later, re-verify before relying on +a specific behavior. `§7. Smoke test protocol` is a minimal suite you +can run in a few minutes. + +### §2.1. Invocation shapes + +Two relevant subcommands: + +``` +codex exec [OPTIONS] [PROMPT] +codex exec resume [OPTIONS] [SESSION_ID] [PROMPT] +``` + +Both accept PROMPT either as a trailing argument or as stdin (pass `-` +as the PROMPT argument, then redirect from a file). The skill uses the +stdin form to avoid shell quoting issues on long XML prompts: + +```bash +codex exec ... - < /tmp/codex-prompt-*.md +``` + +### §2.2. Output streams + +Two output modes, with different stream semantics: + +**Default mode (no `--json`):** + +- stdout: only the final agent message, plain text. Identical to what + `-o FILE` writes to disk. +- stderr: a metadata block printed before the agent response, followed + by a tail including token usage. The metadata block includes the + line `session id: `. + +Verify: + +```bash +echo "respond PONG" | codex exec -m gpt-5.4 -s read-only \ + -C "$(git rev-parse --show-toplevel)" - > /tmp/a.out 2> /tmp/a.err +cat /tmp/a.err | grep 'session id:' +cat /tmp/a.out +``` + +**JSON mode (`--json`):** + +- stdout: newline-delimited JSONL events. The first event is always + `{"type":"thread.started","thread_id":"", ...}`. Subsequent + events are `turn.started`, `item.completed`, `turn.completed`, etc. +- stderr: empty on success. Populated only on errors. + +Verify: + +```bash +echo "respond PONG" | codex exec --json -m gpt-5.4 -s read-only \ + -C "$(git rev-parse --show-toplevel)" - > /tmp/a.out 2> /tmp/a.err +head -1 /tmp/a.out # expect {"type":"thread.started",...} +wc -c /tmp/a.err # expect 0 +``` + +**`-o FILE` flag:** + +Writes the final agent message to FILE as plain text, *regardless* of +whether `--json` is set. In JSON mode, this is the only way to get a +human-readable review — stdout is machine-readable events only. + +### §2.3. Session persistence + +Every non-ephemeral `codex exec` run creates a rollout file: + +``` +~/.codex/sessions/YYYY/MM/DD/rollout-TIMESTAMP-UUID.jsonl +``` + +Filename format: `rollout--.jsonl`. +The UUID is the session / thread id and is accepted verbatim by +`codex exec resume`. + +`--ephemeral` disables persistence. Not used by the skill — resume +requires persistence. + +### §2.4. Resume semantics + +``` +codex exec resume [OPTIONS] [SESSION_ID] [PROMPT] +codex exec resume --last # pick newest in cwd +``` + +**Supported flags (per `codex exec resume --help`):** `--json`, `-o`, +`-m`, `-i`, `--last`, `--all`. + +**NOT supported:** `-s` (sandbox inherited from original session; always +`read-only` in this skill), `-C` / `--cd` (cwd is whatever the shell +had when `codex` was invoked — see §2.4.2). + +**§2.4.1. `--last` filters by cwd.** `resume --last` only considers +sessions whose rollout records the current cwd. Invoking from one repo +cannot pick up a session in another repo. But within the same cwd, it +picks the *newest* session regardless of origin — including one-shots, +unrelated tool invocations, or user-initiated codex work happening in +parallel. + +Verify: + +```bash +mkdir -p /tmp/cwd-a /tmp/cwd-b +cd /tmp/cwd-a && git init -q +cd /tmp/cwd-b && git init -q +cd /tmp/cwd-a && echo "respond ALPHA" | codex exec -s read-only \ + --skip-git-repo-check - 2>&1 | grep 'session id:' +cd /tmp/cwd-b && echo "respond BRAVO" | codex exec -s read-only \ + --skip-git-repo-check - 2>&1 | grep 'session id:' +cd /tmp/cwd-a && echo "which?" | codex exec resume --last \ + --skip-git-repo-check - 2>&1 | tail -5 +# expect: resumes ALPHA session, not BRAVO +``` + +**§2.4.2. cwd is inherited from the shell, not the rollout.** The +original `codex exec -C /repo ...` pins the session to `/repo` for its +initial turn. A subsequent `codex exec resume ` from a *different* +cwd does NOT inherit `/repo`. It either fails with +`Not inside a trusted directory` (exit 1, no `-o` written), or silently +runs with the new cwd. The skill works around this with `cd '' &&` +as a command prefix before every resume (see `§4.3`). + +**§2.4.3. Bad UUID exit code.** `codex exec resume ` +exits with code **1** (not 0). stderr contains: + +``` +Error: thread/resume: thread/resume failed: no rollout found for thread id +``` + +stdout is empty. `-o` file is NOT created. + +Verify: + +```bash +echo "hi" | codex exec resume 00000000-0000-0000-0000-000000000000 \ + --skip-git-repo-check - 2>/tmp/e.err +echo "EXIT=$?" # expect EXIT=1 +cat /tmp/e.err # expect "thread/resume failed..." line +``` + +**§2.4.4. Thread ID does not rotate across successful resumes.** Each +successful `codex exec resume --json` emits a `thread.started` event +whose `thread_id` equals the original session's UUID. It is *not* a new +id. The skill's rule "update `CODEX_SESSION_ID` on every successful +resume" therefore is functionally a no-op on the current version, but +remains defensive for future Codex versions that might rotate ids. + +Verify: compare the UUID in the first `thread.started` event of an +initial `codex exec --json` with the UUID in the first `thread.started` +event of `codex exec resume --json `. Should be identical. + +### §2.5. Known failure modes + +| Trigger | Exit code | stdout | stderr | `-o` file | +|---|---|---|---|---| +| Success | 0 | final text / JSONL | empty (json) or metadata+token block (non-json) | written | +| Timeout (wrapped `timeout 600`) | 124 | partial or empty | partial | may be missing or partial | +| Model not available (`-m bogus`) | 1 | empty | error line | not written | +| `-o` path unwritable | 0 | final text / JSONL | `Failed to write last message file ...` line | not written | +| Not in git work tree, no `--skip-git-repo-check` | 1 | empty | `Not inside a trusted directory ...` | not written | +| Resume with bad UUID | 1 | empty | `thread/resume failed ...` | not written | + +The `-o` unwritable case is dangerous: exit code is misleading. The +skill defends by always reading stderr even on exit 0 (see `§4.8`). + +### §2.6. CLI gaps relevant to the skill + +- `codex exec resume` does not accept `-C`. +- `codex exec resume` did not accept `-o` before issue openai/codex#12538 + was resolved. On current versions it does. +- `codex --version` prints `codex-cli 0.121.0` — not empty, despite one + earlier agent's diagnostic claim (see `§6`). + +--- + +## §3. Claude Code / harness facts + +These facts apply to the Claude Code runtime (the "harness") that +executes the skill, verified during work on this refactor. + +### §3.1. Bash tool + +- Returns combined stdout + stderr as the tool result text. +- Truncates output at approximately 30 KB. Under truncation, the *tail* + is retained; the *head* is dropped. This is why Codex's `session id:` + metadata line (printed before a potentially long reasoning trace) can + disappear from the Bash result under load — the real root of the old + session-id bug, not anything wrong with Codex. +- Current working directory is treated as transient between calls. Do + not depend on `cd` persisting, and do not rely on `$(pwd)` in + composed commands; capture the absolute path once via a deterministic + source (see `§4.2`). + +### §3.2. Write and Read tools + +- Bypass the Bash truncation limit entirely. Prompts longer than a few + kilobytes should be written to a file and passed through stdin + redirection (`- < file`) rather than embedded as a Bash argument. +- Read can open any file — there is no skill-level restriction. + +### §3.3. Safety rules on destructive git operations + +The harness includes a built-in "Git Push to Default Branch" safety +rule that blocks `git push origin master` (or `main`) unless the user +has explicitly granted a permission. The skill does not push, so this +does not affect runtime, but it is relevant when releasing skill +changes: maintainer must push master themselves or add a permission. + +### §3.4. Opus interpretation of instructions + +Later Opus releases tend to interpret SKILL.md instructions more +literally than earlier ones. An instruction like "Show the user +verbatim" without a hard procedural anchor can be internally +rationalized as "the context already contains the review, the user +sees the context" and skipped. Current design compensates with: + +- A strict "YOUR NEXT MESSAGE to the user must begin with ..." clause + (`SKILL.md` Step 5) that names the message, not just the act. +- Architectural enforcement: the `--json` invocation puts machine- + readable JSONL in stdout, so the review text exists only in the + `-o` file. The lead *cannot* satisfy the "show the review" contract + by quoting from the Bash result, because the Bash result has no + review text in the first place. + +See `§4.9` for the rejected-marker-file alternative. + +--- + +## §4. Design decisions + +Each decision below follows the same template: + +- **Decision** — one sentence. +- **Where in SKILL.md** — step reference. +- **Context** — the problem it addresses. +- **Alternatives considered** — with reasons for rejection. +- **Chosen because** — the load-bearing argument. +- **Trade-offs accepted** — what we gave up. + +### §4.1. Use `--json` for session ID capture + +- **Decision.** Every `codex exec` and `codex exec resume` invocation + uses `--json` and redirects stdout to `/tmp/codex-stdout-${REVIEW_ID}.jsonl`. + Session ID is parsed from the first JSONL event's `thread_id` field. +- **Where in SKILL.md.** Step 4 (launch), Step 7 (resume). +- **Context.** Earlier skill versions parsed `session id:` from + stderr. That line was reliable on the Codex side but unreliable on + the Claude side because the Bash tool truncates output at ~30 KB, + retaining the tail. Long reasoning pushed the early metadata block + (including the session id) out of the retained window on high-effort + reviews. +- **Alternatives considered.** + - *Keep parsing stderr.* Rejected: same truncation problem. + - *Redirect stderr to a file, Read via Read tool.* Rejected: an + earlier skill version did exactly this and a diagnostic dump still + reported empty stderr files, suggesting a fragility we did not + want to investigate further. Worse, stderr's session-id line is + not in a stable absolute position. + - *Extract UUID from the latest `~/.codex/sessions/**/rollout-*.jsonl` + filename.* Rejected: fragile against parallel codex invocations; + filesystem race. +- **Chosen because.** In JSON mode the `thread_id` is in the *first + line* of stdout, at an offset of <100 bytes from the file start. Read + tool retrieves it independent of Bash truncation. The format is + documented and enforced by Codex. +- **Trade-offs accepted.** Human-readable review is no longer in stdout + (it went to `-o` only). That is fine for the skill — in fact it is + *load-bearing* for the show-review gate (§4.9). + +### §4.2. Capture `REPO_ROOT` at Step 2, substitute literally + +- **Decision.** At Step 2, run `git rev-parse --show-toplevel` once, + save the absolute path as `REPO_ROOT`, and substitute it verbatim + (quoted) into every Codex command. Do not use `$(pwd)` in composed + commands. +- **Where in SKILL.md.** Step 2 (capture), Steps 4 and 7 (use). +- **Context.** Codex commands need a stable cwd: `-C` for initial exec, + `cd '...' &&` prefix for resume. If the cwd is evaluated at Bash-call + time via `$(pwd)`, it is susceptible to Claude Code's weak + cwd-persistence between calls (§3.1). +- **Alternatives considered.** + - *`$(pwd)` everywhere.* Rejected: cwd drift. + - *`pwd -P` at each call.* Same problem, plus added complexity. +- **Chosen because.** One capture, many uses, all deterministic. +- **Trade-offs accepted.** Requires error handling for bare repos + (exit 128) and awareness of submodule scoping — the skill aborts on + bare repos with a clear message and warns on submodules (see + `SKILL.md` Step 2). + +### §4.3. Hybrid cwd pinning: `-C` for initial, `cd` prefix for resume + +- **Decision.** Initial `codex exec` uses `-C "${REPO_ROOT}"`. Resume + uses a shell prefix: `cd '${REPO_ROOT}' && codex exec resume ...`. +- **Where in SKILL.md.** Steps 4 and 7. +- **Context.** Codex's `exec` accepts `-C`; `resume` does not. Resume + inherits cwd from the invoking shell. +- **Alternatives considered.** + - *Use `cd` prefix for both.* Rejected: `-C` is more precise for + initial (it is the documented way to pin), and uses less + permissions surface. + - *Rely on the harness's ambient cwd.* Rejected: see §3.1 — + insufficiently reliable. +- **Chosen because.** This is the minimal-surgery solution that matches + what each subcommand actually supports. +- **Trade-offs accepted.** Two slightly different command shapes in + `SKILL.md`; one extra permission pattern in README + (`Bash(cd * && timeout 600 codex exec resume *)`). + +### §4.4. Conditional `CODEX_SESSION_ID` update (only on full success) + +- **Decision.** Update `CODEX_SESSION_ID` from the JSONL stdout's first + line only when ALL of the following hold: exit code 0, stderr has no + `Error:` or `thread/resume failed` line, `-o` file contains a valid + `VERDICT:` line (and, for REVISE, at least one `[severity:` marker). +- **Where in SKILL.md.** Step 7. +- **Context.** A failed resume (bad model, model error, infrastructure + failure) still emits a `thread.started` event with a fresh but dead + `thread_id`. An unconditional update would poison the session id and + cause subsequent resumes to fail against a non-existent session. +- **Alternatives considered.** + - *Unconditional update.* Rejected: demonstrated poisoning on bad + model invocations during review. + - *Update on exit 0 only.* Rejected: `-o` unwritable returns exit 0 + with a broken session; stderr error line needs checking too. +- **Chosen because.** All three checks together give a reliable "the + session actually produced a review" signal. +- **Trade-offs accepted.** More conditions to specify and execute, but + they are already required for the review-file sanity check — marginal + cost. + +### §4.5. No `--last` in any fallback + +- **Decision.** The fallback chain does not use `codex exec resume --last`. + On resume failure the skill either asks the user (interactive) or + chooses by severity (headless); the "retry" option is always a fresh + `codex exec`, not `--last`. +- **Where in SKILL.md.** Step 7 fallback. +- **Context.** `--last` silently picks the newest session in the + current cwd (§2.4.1), which can be an unrelated one-shot or a + parallel user invocation. +- **Alternatives considered.** + - *`--last` as first fallback.* Rejected: wrong-session hazard; if + the user happens to be running Codex interactively in the same + repo, the skill's "I've revised based on your feedback ..." + message would be injected into the user's unrelated work. +- **Chosen because.** The safety failure mode is catastrophic (silent + incorrect reviews, context injection into user's sessions); the cost + of skipping this shortcut is modest (one more fresh exec per failure, + which is rare). +- **Trade-offs accepted.** Slightly higher token cost on the rare + resume-failure path. + +### §4.6. Fresh-exec fallback rebuilds context from conversation history + +- **Decision.** When the fallback triggers a fresh `codex exec`, Claude + reconstructs the "previous rounds" section of the prompt from the + conversation — the round-1 review, round-1 fixes, round-2 review, + round-2 fixes, etc. — which were already shown verbatim to the user + in earlier Step 5 outputs. +- **Where in SKILL.md.** Step 7 fallback prompt template. +- **Context.** The `-o` file at `/tmp/codex-review-${REVIEW_ID}.md` is + overwritten on every round. Round-1 review content is gone from disk + by the time a round-3 fallback triggers. +- **Alternatives considered.** + - *Per-round file naming* (`-r1.md`, `-r2.md`, ...). Rejected: user + preference for minimizing file proliferation. Also required + matching changes in Step 9 cleanup globs. + - *Archive previous -o file before overwrite.* Rejected: adds + complexity (pre-write copy step) for a case that triggers rarely. +- **Chosen because.** The Step 5 "show review verbatim" contract + already ensures the content is in conversation history. Leveraging + that makes a new step unnecessary. +- **Trade-offs accepted.** Depends on the conversation context window + preserving prior outputs. If Claude Code compacts the context + mid-review, the fallback template may be degraded. No mitigation + currently; see `§9. Known limitations`. + +### §4.7. Semantic VERDICT + findings check (no byte threshold) + +- **Decision.** Step 5 sanity checks the review file by regex only. The + file must contain a line matching `^VERDICT: (APPROVED|REVISE)$`; if + the verdict is REVISE, it must also contain at least one line + matching `\[severity:\s*(critical|high|medium)`. +- **Where in SKILL.md.** Step 5. +- **Context.** An earlier draft used a byte-size threshold (`< 50 + bytes = launch failure`). A legitimate terse approval (`VERDICT: + APPROVED\n`, 17 bytes) would be misclassified. Worse, a REVISE + verdict with no findings would pass a byte check but create an + infinite-loop hazard: nothing to fix, resubmit empty fixes, same + verdict, loop until max rounds. +- **Alternatives considered.** + - *Byte threshold.* Rejected as above. + - *Require only `VERDICT:`.* Rejected: REVISE-without-findings loop. +- **Chosen because.** Semantics over heuristic; catches both the + terse-approval false positive and the empty-REVISE infinite loop. +- **Trade-offs accepted.** Depends on the reviewer emitting the + `[severity:` marker format prescribed by the prompt. Enforced via + prompt wording; prompt drift would be a separate failure mode (see + §6). + +### §4.8. Strict check order: exit → stderr → review file + +- **Decision.** After every `codex exec` / `codex exec resume` call, + checks are run in a fixed order: exit code first, then stderr file + (even on exit 0), then the `-o` review file. +- **Where in SKILL.md.** Steps 4 and 7 post-launch. +- **Context.** Non-zero exit implies `-o` may not exist; reading it + would crash. Exit 0 does not imply everything is fine (e.g., `-o` + unwritable case, §2.5). A defined order routes every failure mode to + the right diagnostic without the lead having to improvise. +- **Alternatives considered.** + - *Ad-hoc checks in whatever order.* Rejected: invites null-pointer- + style crashes on missing files. + - *All-at-once check, aggregate errors.* Rejected: harder for Claude + to follow step-by-step; harder to recover partial information. +- **Chosen because.** Deterministic, fail-fast, each check's output + points to the next action. +- **Trade-offs accepted.** Slightly verbose to describe. + +### §4.9. Hardened "show review" gate, no marker file + +- **Decision.** The gate enforcing verbatim review output is + text-based, repeated in three places (Step 4 anti-confusion note, + Step 5 YOUR-NEXT-MESSAGE instruction, Step 6 precondition check), + and backstopped by the architectural fact that `--json` stdout is + non-human-readable so the review can only be accessed via Read on + the `-o` file. +- **Where in SKILL.md.** Steps 4, 5, 6. +- **Context.** Earlier design used only "Show the user verbatim" in + Step 5. Observed behavior: Opus received the review (began applying + fixes that referenced it) but never displayed it to the user. Soft + instructions can be internally rationalized away (§3.4). +- **Alternatives considered.** + - *Marker-file precondition* — Claude writes + `/tmp/codex-review-${REVIEW_ID}.shown` as the act of showing; Step + 6 verifies the file exists. Rejected: cargo-cult risk. A literal + reader that skips the show step may still write the marker, + producing a false audit trail that is worse than no trail. And the + marker cannot be programmatically enforced from within SKILL.md + — it is still instructional. + - *Leave as soft "Show verbatim".* Rejected: the observed bug. +- **Chosen because.** The `--json` switch does more to prevent the bug + than any textual instruction: with stdout as JSONL events, there is + no review text in the Bash tool result for Opus to accidentally + consume instead of re-reading the `-o` file. The triple text gate + is belt-and-braces. +- **Trade-offs accepted.** Relies on model compliance with the text + gate. If a future Opus slips past all three, we will need another + architectural hook (possibly a verifiable marker after all). + +### §4.10. Conditional cleanup (keep temp files on abort) + +- **Decision.** Step 9 cleans up `/tmp/codex-*-${REVIEW_ID}.*` only on + success paths (APPROVED, MAX rounds, NOT VERIFIED). On abort paths + (launch failure, infrastructure error), the files are left in place. +- **Where in SKILL.md.** Step 9. +- **Context.** When the skill aborts, diagnostics live in the stderr + and stdout temp files. Cleaning them up immediately makes post-hoc + debugging impossible. +- **Alternatives considered.** + - *Always clean up.* Rejected: loses forensics on the cases you + most want forensics for. + - *Per-round naming to preserve history across rounds.* Rejected: + see §4.6. +- **Chosen because.** The abort path is rare and the files are small. + The trade-off between a few KB of residual `/tmp` content and the + ability to understand a failure is lopsided. +- **Trade-offs accepted.** Residual files accumulate in `/tmp`. The OS + will clear them on reboot; a later successful invocation with the + same `REVIEW_ID` collides with probability ~10⁻⁸ per same-second + invocation (8-digit random). + +### §4.11. Ask-user on resume failure in interactive mode; severity-based in headless + +- **Decision.** When resume fails (one of the three checks in §4.8): + - Interactive: ask the user to choose between (a) fresh exec with + prior context, or (b) conclude the review as NOT VERIFIED. + - Headless: auto-fresh-exec if max severity of last round is + critical/high; auto-NOT-VERIFIED if only medium. +- **Where in SKILL.md.** Step 7 fallback. +- **Context.** Fresh exec is expensive (token-wise) and sometimes + unnecessary (if the last round's findings were medium, skipping + re-verification is often acceptable). An unconditional fresh exec + wastes tokens; an unconditional conclude-as-not-verified risks + shipping with a critical finding unaddressed. +- **Alternatives considered.** + - *Unconditional fresh exec.* Rejected: cost and user-interruption + model. + - *Unconditional NOT-VERIFIED.* Rejected: critical/high findings + could be silently skipped. + - *Always ask the user.* Rejected: no way to ask in headless + (scheduled, CI) contexts. +- **Chosen because.** Respects the user's "minimize interruptions" + preference while preserving correctness for serious findings. The + severity parsing uses a tight regex (§6's lesson on format drift); + zero matches defaults to critical for safety. +- **Trade-offs accepted.** Severity parsing is a soft dependency on + reviewer output format; see `§5` for alternatives considered. + +### §4.12. Per-round retry budget separate from the round counter + +- **Decision.** Step 5 launch-failure retry is capped at 1 per round + and does NOT consume the 5-round counter. The counter advances only + when a *valid* review (one that passes §4.7 checks) has been + produced. +- **Where in SKILL.md.** Step 5. +- **Context.** A launch failure is an infrastructure issue, not a + failed review. Counting it against the round budget would be + inappropriate — the user would get four rounds of review instead of + five because of a flaky codex launch. +- **Alternatives considered.** + - *Retry consumes a round.* Rejected: see above. + - *Unlimited retries.* Rejected: creates an infinite loop on + persistent infrastructure failure. +- **Chosen because.** The one-retry cap bounds the cost; the + separate-counter rule preserves the user's 5-round expectation. +- **Trade-offs accepted.** The retry counter lives in the lead's + round-local reasoning — if an implementer treats it as global across + rounds, retry budget is inconsistently available. Rules section of + `SKILL.md` states this explicitly. + +--- + +## §5. Rejected ideas + +Ideas that came up in adversarial review rounds or in exploration and +were rejected. Documenting them so a future contributor doesn't +re-propose them without reading why. + +### §5.1. Marker file (`.shown` precondition) + +Round 2 adversarial review proposed requiring the lead to write +`/tmp/codex-review-${REVIEW_ID}.shown` as an atomic "I have shown the +review" signal, with Step 6 hard-gating on its existence. + +Rejected: the marker is itself an instructional artifact. A literal +reader who skips the show step may also write the marker, producing a +false audit trail — strictly worse than a visible skip, because it +masquerades as compliance. The real fix was architectural (`--json` +making review text inaccessible from Bash output, forcing a Read). + +### §5.2. Per-round file naming + +Proposed during Round 2 review: name temp files with both `${REVIEW_ID}` +and round number (`/tmp/codex-review-${REVIEW_ID}-r1.md`, +`...-r2.md`). Benefits: prior-round diagnostics survive later rounds; +fresh-exec fallback can read prior review text from disk. + +Rejected: user preference for minimizing file proliferation; the Step +5 verbatim-output contract already places prior reviews in the +conversation history, which is where `§4.6` draws from. + +### §5.3. Stderr parsing for session ID + +The historical approach. Works in non-`--json` mode (the `session id:` +line is reliably printed to stderr), but fragile under Bash tool +output truncation (§3.1). Replaced by `--json` + first-line JSONL +parsing (§4.1). + +### §5.4. `$(pwd)` in composed commands + +An intuitive but fragile shortcut. Claude Code's Bash tool does not +persist cwd reliably between calls, so `$(pwd)` can resolve to an +unexpected directory. Replaced by the single-capture `REPO_ROOT` +pattern (§4.2). + +### §5.5. Auto-fresh-exec without user consent on resume failure + +Earlier skill drafts silently ran a fresh `codex exec` whenever resume +failed. This burned significant tokens (each fresh exec is a full +project re-read) for cases that sometimes were not worth re-verifying +(e.g., only medium-severity findings). Replaced by ask-user / +severity-based fallback (§4.11). + +### §5.6. Always cleaning up temp files on exit + +Earlier Step 9 unconditionally ran `rm -f` on all temp files at +end-of-run. That erased diagnostic trail for abort paths. Replaced by +conditional cleanup (§4.10). + +--- + +## §6. Prior diagnostic errors and lessons + +During the refactor that produced the current design, a diagnostic +dump from an earlier agent-auditor contained several assertions that +turned out to be wrong when verified empirically. We document them +here as a lesson, not as blame. + +### §6.1. "Codex stderr is 0 bytes — no `session id:` line is printed" + +**Claim.** The auditor's dump claimed that redirecting stderr to a file +during `codex exec` produced an empty file, across eight observed +invocations. + +**Reality.** In non-`--json` mode, stderr contains a multi-line +metadata block that includes `session id: `. In `--json` mode, +stderr is empty on success. The auditor probably tested in `--json` +mode and generalized. + +**Impact on the design.** We initially believed stderr was unreliable +in general. Empirical verification showed stderr is reliable — it is +*Bash tool truncation* that makes stderr parsing unreliable (§3.1). +Different root cause; different fix (`--json` for first-line parsing, +§4.1). + +### §6.2. "`codex --version` prints nothing" + +**Claim.** The binary was said to emit no version string. + +**Reality.** `codex --version` prints `codex-cli 0.121.0`. + +**Impact on the design.** Minor — but indicative of measurement sloppiness. + +### §6.3. "`codex exec resume --last` is unsafe" + +**Claim.** The dump implied `--last` picks an arbitrary session. + +**Reality.** `--last` filters by cwd but picks the newest session in +that cwd (§2.4.1). So it is "unsafe" in a narrower, more specific sense +than stated — it is safe across repos, unsafe within the same repo +against parallel or unrelated one-shots. The distinction matters: the +fix (drop `--last` entirely, §4.5) was driven by the specific risk of +context injection into user's parallel codex sessions, not by any +general unsafety. + +### §6.4. "`codex exec resume ` exits with code 0" + +**Claim.** The dump (and the first round of adversarial review of our +own plan) both asserted that `codex exec resume` with an invalid UUID +exits 0 — making exit code useless as a success signal. + +**Reality.** It exits **1**. stderr has `thread/resume failed`. stdout +is empty. Exit code is a reliable signal. + +**Impact on the design.** We originally planned an elaborate stderr- +error-line check as the primary fallback trigger. That check is still +present in §4.8 as a defense against future Codex versions, but on the +current version exit code alone is sufficient. + +### §6.5. Lesson + +Single-source diagnostic reports are hypothesis, not fact. Every +load-bearing claim should be verified empirically before a design +decision is built on top of it. This document's `§2` and `§7` are +structured so future contributors can replicate the verification in +minutes, not hours. + +--- + +## §7. Smoke test protocol + +Minimal set of checks to run after editing `SKILL.md`, changing the +Codex invocation pattern, or upgrading Codex CLI. Purpose: detect +regressions in the external-CLI contract the skill depends on. + +Each check is copy-pasteable bash. All use a real git repo; run from +the repo root. Expected outputs are in comments. + +### §7.1. Initial launch (mirrors Step 4 flow) + +```bash +REVIEW_ID=$(date +%s)-$(printf '%08d' $RANDOM) +REPO_ROOT=$(git rev-parse --show-toplevel) +cat > /tmp/codex-prompt-${REVIEW_ID}.md <<'EOF' + +You are a senior adversarial reviewer of implementation plans. + + +Confirm you received this prompt. + + +End the LAST line with exactly: VERDICT: APPROVED + +EOF + +timeout 300 codex exec --json \ + -m gpt-5.4 -c model_reasoning_effort=low \ + -s read-only -C "${REPO_ROOT}" \ + -o /tmp/codex-review-${REVIEW_ID}.md \ + - < /tmp/codex-prompt-${REVIEW_ID}.md \ + > /tmp/codex-stdout-${REVIEW_ID}.jsonl \ + 2>/tmp/codex-stderr-${REVIEW_ID}.txt + +echo "EXIT=$?" # expect 0 +head -1 /tmp/codex-stdout-${REVIEW_ID}.jsonl # expect {"type":"thread.started","thread_id":"..."} +wc -c /tmp/codex-stderr-${REVIEW_ID}.txt # expect 0 +grep -E '^VERDICT:' /tmp/codex-review-${REVIEW_ID}.md # expect VERDICT: APPROVED +``` + +### §7.2. Resume with cd prefix + +Continuing from §7.1 — extract the thread id and resume. + +```bash +THREAD_ID=$(head -1 /tmp/codex-stdout-${REVIEW_ID}.jsonl \ + | grep -oE '"thread_id":"[^"]+"' | cut -d'"' -f4) +echo "THREAD_ID=${THREAD_ID}" # expect a UUID + +cat > /tmp/codex-resume-prompt-${REVIEW_ID}.md <<'EOF' +Still there? Reply with VERDICT: APPROVED. +EOF + +cd "${REPO_ROOT}" && timeout 300 codex exec resume --json \ + "${THREAD_ID}" \ + -o /tmp/codex-review-${REVIEW_ID}.md \ + - < /tmp/codex-resume-prompt-${REVIEW_ID}.md \ + > /tmp/codex-stdout-${REVIEW_ID}.jsonl \ + 2>/tmp/codex-stderr-${REVIEW_ID}.txt + +echo "EXIT=$?" # expect 0 +head -1 /tmp/codex-stdout-${REVIEW_ID}.jsonl # first line still has thread_id (same UUID — §2.4.4) +wc -c /tmp/codex-stderr-${REVIEW_ID}.txt # expect 0 +grep -E '^VERDICT:' /tmp/codex-review-${REVIEW_ID}.md # expect VERDICT: APPROVED +``` + +### §7.3. Resume with bad UUID + +```bash +echo "test" > /tmp/codex-bad-resume-prompt.md +timeout 60 codex exec resume --json \ + 00000000-0000-0000-0000-000000000000 \ + - < /tmp/codex-bad-resume-prompt.md \ + > /tmp/codex-bad-resume.stdout \ + 2>/tmp/codex-bad-resume.stderr + +echo "EXIT=$?" # expect 1 +cat /tmp/codex-bad-resume.stderr | head -3 # expect "thread/resume failed" line +wc -c /tmp/codex-bad-resume.stdout # expect 0 +``` + +### §7.4. Bare repo detection + +```bash +TMPBARE=$(mktemp -d) +git init --bare "${TMPBARE}" -q +( cd "${TMPBARE}" && git rev-parse --show-toplevel 2>&1 ) +# expect: "fatal: this operation must be run in a work tree" or similar +# expect exit 128 +rm -rf "${TMPBARE}" +``` + +### §7.5. `--last` cwd filter + +```bash +# setup two dummy git repos +mkdir -p /tmp/smoke-cwd-a /tmp/smoke-cwd-b +( cd /tmp/smoke-cwd-a && git init -q ) +( cd /tmp/smoke-cwd-b && git init -q ) +# run codex in each; capture session ids +ID_A=$(cd /tmp/smoke-cwd-a && echo "ALPHA" | \ + codex exec -s read-only -m gpt-5.4 \ + -c model_reasoning_effort=low - 2>&1 | grep 'session id:' | awk '{print $3}') +ID_B=$(cd /tmp/smoke-cwd-b && echo "BRAVO" | \ + codex exec -s read-only -m gpt-5.4 \ + -c model_reasoning_effort=low - 2>&1 | grep 'session id:' | awk '{print $3}') +echo "A=${ID_A}" +echo "B=${ID_B}" +# resume --last from cwd-a; expect to resume ID_A, not ID_B +( cd /tmp/smoke-cwd-a && echo "which?" | \ + codex exec resume --last - 2>&1 | grep 'session id:' ) +# expect: session id matches ID_A +rm -rf /tmp/smoke-cwd-a /tmp/smoke-cwd-b +``` + +### §7.6. Cleanup smoke artifacts + +```bash +rm -f /tmp/codex-prompt-${REVIEW_ID}.md \ + /tmp/codex-resume-prompt-${REVIEW_ID}.md \ + /tmp/codex-review-${REVIEW_ID}.md \ + /tmp/codex-stdout-${REVIEW_ID}.jsonl \ + /tmp/codex-stderr-${REVIEW_ID}.txt \ + /tmp/codex-bad-resume-prompt.md \ + /tmp/codex-bad-resume.stdout \ + /tmp/codex-bad-resume.stderr +``` + +### §7.7. If anything fails + +If §7.1–§7.5 do not produce the expected outputs: + +1. Note the exact codex CLI version (`codex --version`). +2. Check against the facts in `§2`. If your observation contradicts a + `§2` fact, the design may need adjustment — propose via PR and + include a new entry in `§8. Version and verification log` for the + new Codex version. +3. If the contradiction is severe (e.g., `--json` format change, + `-o` no longer works in `resume`), mark `§2` entries outdated + before modifying `SKILL.md`. Future contributors should know which + facts they can still trust. + +--- + +## §8. Version and verification log + +| Date | Codex CLI | Claude Code | Verifier | Notes | +|------|-----------|-------------|----------|-------| +| 2026-04-17 | 0.121.0 | current at time of refactor | initial author | All §2 facts verified; §7 smoke test passes end to end. Initial commit of this document. | + +When you re-verify (either during routine maintenance or when +triggered by §7.7), add a row. Keep the log chronological. + +--- + +## §9. Known limitations and future work + +### §9.1. Parallel codex in the same cwd + +If the user runs `codex exec` manually in the same working tree while +the skill is mid-review, `~/.codex/sessions/` will contain sessions +from both processes. The skill does not use `--last`, so this does not +cause wrong-session hazards, but the user's parallel work may produce +rollout files that are filesystem-level noise during debugging. + +No mitigation. Rules in `SKILL.md` mention the constraint indirectly +by forbidding `cd` between rounds, but they don't prevent the user's +own parallel invocations. + +### §9.2. Context compaction risk in fresh-exec fallback + +The fallback template (§4.6) rebuilds prior rounds from conversation +history. If Claude Code's context compaction kicks in mid-review and +summarizes earlier rounds, the rebuilt template is degraded — the new +reviewer may see a summary instead of the original verbatim findings. + +Not mitigated today. If encountered in practice, options include +archiving each round's `-o` file to a per-round name (§5.2 idea, +currently rejected), or persisting the review chain to a single +append-only file in `/tmp` that the fallback reads directly. + +### §9.3. Reviewer prompt format drift + +The §4.7 sanity check and §4.11 severity parsing rely on the reviewer +using the format prescribed by the skill's adversarial prompt +(`VERDICT: APPROVED|REVISE` on a line; `[severity: ]` in +finding sub-headers). If the reviewer drifts (e.g., outputs +`**Severity:** High` instead), the skill falls through launch-failure +paths inappropriately. + +Mitigation today: case-insensitive regex tolerating bracketed and +non-bracketed severity forms. If drift becomes frequent, prompt +engineering is the first response; a last-resort option is more +permissive regex at the cost of weaker signal. + +### §9.4. Path containing shell-special characters + +If `REPO_ROOT` contains `'`, `"`, `$`, backtick, or newline, the skill +aborts at Step 2 with a clear message rather than attempting +sanitation. This is a user-visible limitation; in practice repo paths +rarely contain these characters. + +If this becomes an issue, the fix is to sanitize / escape before +substitution, which requires careful handling of double-quoted `-C` +argument and single-quoted `cd` prefix. + +### §9.5. No automated tests + +`§7. Smoke test protocol` is manual. Automating it would require: + +- A `scripts/smoke.sh` with the checks from §7. +- A way to run under a non-interactive codex auth. Today codex auth is + per-user; CI integration requires exchanging an `OPENAI_API_KEY` + via environment variables. + +Deferred until there is a CI story for the repo. + +--- + +## §10. Update protocol — when and how to revise this file + +### §10.1. When to update + +Revise this document when any of the following happens: + +- **Codex CLI releases.** If a release changes any of §2's facts — + e.g., a new `resume` flag, a change in `--json` event format, a + fix to thread_id rotation, a new exit code — re-run §7, then + update §2 and add a row to §8. If a design decision in §4 relied + on the old behavior, evaluate whether the decision still holds + and revise or rationalize. +- **A new Claude model generation.** Later Opus releases tend to + interpret `SKILL.md` instructions more literally. What worked as + a "soft rule" in the previous generation may fail for the next. + When a new Opus ships, re-run the skill end-to-end on a small + artifact (smoke test from the user side, not just from the CLI + side) and check that review output is actually shown to the user, + that the fixes are applied, that the verdict is parsed. If not, + the §3.4 / §4.9 instructions may need tightening — rewrite with + more explicit "DO NOT" lists, more explicit procedural anchors + ("YOUR NEXT MESSAGE", "BEFORE calling any fix tool", etc.), and + update §3.4 to record the observed literal interpretation. +- **Claude Code harness updates.** If Bash tool truncation size + changes, if cwd behavior between calls changes, if new + safety rules appear, update §3. +- **A reviewer finds a new failure mode.** Add a finding to §5 (if + the idea was considered and rejected) or §9 (if it is an accepted + limitation). +- **You encounter a diagnostic report you did not write.** Before + acting on it, verify its claims empirically. If any claim is + wrong, add an entry to §6. This file should be the repository of + lessons, not just of the current state. + +### §10.2. How to update + +- Keep the structure. Section numbering is part of the cross-reference + network; renumbering silently breaks links in `SKILL.md` and in + this document itself. +- When a fact in §2 is superseded, mark the superseded version clearly + (strike through, "was:" prefix, or similar) before adding the new + fact. Do not delete superseded facts silently — a future reader + needs to know what *used to* be true, because some old SKILL.md + behavior still assumes it. +- When a design decision in §4 is revised, move the old decision text + into §5 (rejected ideas) with the new reason for rejection. Keep + the institutional memory explicit. +- Always add a §8 row when you re-verify against a new tool version. +- Every major update should run §7 and commit the observations. + +### §10.3. Writing for literal readers + +When adding instructions to `SKILL.md` alongside a design change here, +remember that future models may interpret them more literally than +the current generation. Rules of thumb: + +- State the *action*, not the *intent*. "Show the review verbatim" + is intent; "Your next message to the user must begin with + `## Adversarial Review — Round N ...` and must contain the file + contents verbatim" is action. +- List what must NOT be done in addition to what must be done. + "Do not wrap the review in a code fence", "Do not call Edit/Write + in the same message". +- Anchor procedural steps to verifiable conditions: "before any fix + tool call", "only after sending the user message", etc. These + survive literal interpretation better than "first ... then ...". + +--- + +## §11. References + +- `SKILL.md` — authoritative source of runtime behavior. +- `README.md` — user-facing install, permissions, troubleshooting. +- Codex CLI documentation: https://developers.openai.com/codex/cli +- Codex CLI non-interactive mode: https://developers.openai.com/codex/noninteractive +- Codex CLI command-line reference: https://developers.openai.com/codex/cli/reference +- Codex GitHub issue #12538 (resume `-o` support): https://github.com/openai/codex/issues/12538 +- Codex GitHub issue #14544 (resume exec sessions): https://github.com/openai/codex/issues/14544