From 1237e168c3b3993680c4d5c8beab35bec9afeef8 Mon Sep 17 00:00:00 2001 From: Dmitry Dementev Date: Thu, 21 May 2026 09:56:08 +0300 Subject: [PATCH] =?UTF-8?q?feat(skill):=20refresh=20adversarial-review=20?= =?UTF-8?q?=D0=BF=D0=BE=D0=B4=20Codex=200.132?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Зачем: - актуализация под новый Codex CLI (0.132 убрал флаг -a, ввёл -c approval_policy / -c approvals_reviewer); - переход на безопасные операторские дефолты (sandbox=workspace-write для эмпирической верификации, явные overrides sandbox:* и approvals:*); - наведение порядка в принятии решений по раунду ревью: review_quality + evaluation matrix + structural operator gate вместо «оператор сам разбирается». - Что: - SKILL.md: override-граммар (sandbox:*, approvals:*, model:*, low/medium/high/xhigh), детект OPERATOR_LANGUAGE, runtime hint, conditional dirty-file warning в Step 2 (после захвата REPO_ROOT), dual-layer mutation snapshots (git status + sha256sum input-файлов), operation-aware dispatch table, evaluation matrix + structural operator gate (batch-pause), structured resume body (Applied / Re-scoped / Rejected / Specific asks), final operator summary на OPERATOR_LANGUAGE. - references/runner.md: переведён на Sonnet runner, добавлен Step R2.5 bwrap preflight, Step R4.5 review_quality + bounded triage, расширена 11-полевая JSON-схема результата. - docs/DESIGN.md: §4.14–§4.21 с обоснованиями новых решений, §7.8 refresh-era smoke checks, §8 version log с двумя раундами dogfood-а этого refresh-а (включая R2-корректировку обоснования residual gap для уже-грязных tracked-файлов). - README.md: таблица дефолтов, Safety considerations (честно задокументирован residual gap), Linux sandbox prerequisites (bwrap + AppArmor user namespaces), Operator language, Final operator summary, troubleshooting. - examples/review-output.md: модель в сэмпле обновлена на gpt-5.5. - docs/superpowers/specs/2026-05-20-...: сохранена спека дизайна с уточнениями после dogfood. - Проверка: - dogfood: 2 раунда /adversarial-review code на этом refresh-е, R2 верифицировал R2#1 (ordering) и R2#2 (Option B, honest residual gap); - smoke: codex --version ≥ 0.132.0, bwrap preflight зелёный на reference WSL2. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 338 ++++++++- SKILL.md | 483 ++++++++++-- docs/DESIGN.md | 449 ++++++++++- ...05-20-adversarial-review-refresh-design.md | 718 ++++++++++++++++++ examples/review-output.md | 4 +- references/runner.md | 199 ++++- 6 files changed, 2085 insertions(+), 106 deletions(-) create mode 100644 docs/superpowers/specs/2026-05-20-adversarial-review-refresh-design.md diff --git a/README.md b/README.md index d4c6f92..0cc5254 100644 --- a/README.md +++ b/README.md @@ -75,6 +75,10 @@ If Codex is missing: `npm install -g @openai/codex` - Sign in interactively: `codex` (opens browser) - Or set `CODEX_API_KEY` env var for non-interactive use +**Codex CLI version.** The skill targets Codex CLI ≥ 0.132.0. From 0.132 onward the top-level `-a` / `--ask-for-approval` flag is gone from `codex exec`; approval policy is expressed only via `-c approval_policy=...`. The skill emits the `-c` form unconditionally. If you are stuck on an older Codex CLI, upgrade before using the refreshed defaults. + +**Linux sandbox prerequisites.** The default sandbox (`workspace-write`) relies on `bubblewrap` (`bwrap`) and unprivileged user namespaces. See the [Linux sandbox prerequisites](#linux-sandbox-prerequisites) section below — Ubuntu 24.04+ requires installing an AppArmor profile. + ### 2. Install the skill ```bash @@ -113,9 +117,10 @@ chosen config file: ```jsonc // --- adversarial-review permissions --- -// Git: diff, status, branch detection, repo root, submodule check +// Git: diff, status, branch detection, repo root, submodule check, scoped status snapshots "Bash(git diff*)", "Bash(git status*)", +"Bash(git -C * status --porcelain*)", "Bash(git symbolic-ref*)", "Bash(git rev-parse*)", // Codex: initial launch (uses -C; prompt fed via cat | pipe for env portability) @@ -126,6 +131,19 @@ chosen config file: "Bash(find ~/.codex/sessions*)", // Diagnostic aid when filesystem fallback finds nothing "Bash(ls -t ~/.codex/sessions*)", +// Sandbox preflight probe (runner subagent runs this on every initial / fresh-exec when CODEX_SANDBOX uses bwrap) +"Bash(bwrap --dev-bind / / --unshare-net /bin/echo ok*)", +// Workspace mutation snapshots (main thread, per dispatch) +"Bash(sha256sum /tmp/codex-*)", +"Bash(diff -q /tmp/codex-*)", +"Write(/tmp/codex-git-pre-*)", +"Write(/tmp/codex-git-post-*)", +"Write(/tmp/codex-inputs-pre-*)", +"Write(/tmp/codex-inputs-post-*)", +"Read(/tmp/codex-git-pre-*)", +"Read(/tmp/codex-git-post-*)", +"Read(/tmp/codex-inputs-pre-*)", +"Read(/tmp/codex-inputs-post-*)", // Temp files: prompts (initial + resume), plans, review output, JSONL stdout, stderr "Write(/tmp/codex-plan-*)", "Write(/tmp/codex-prompt-*)", @@ -138,13 +156,16 @@ chosen config file: "Bash(mv /tmp/codex-stderr-* /tmp/codex-stderr-*-failed-resume.txt)", // Cleanup "Bash(rm -f /tmp/codex-*)", -// Main thread: write prompt body for the runner subagent (NEW in refactor) +// Main thread: write prompt body for the runner subagent "Write(/tmp/codex-body-*)", -// Main thread: read the structured JSON result returned by the runner (NEW) +// Main thread: write resume body (structured Applied / Re-scoped / Rejected) +"Write(/tmp/codex-resume-body-*)", +// Main thread: read the structured JSON result returned by the runner "Read(/tmp/codex-runner-result-*)", -// Runner subagent (inherited): read the prompt body main wrote (NEW) +// Runner subagent (inherited): read the prompt body main wrote "Read(/tmp/codex-body-*)", -// Runner subagent (inherited): write the result JSON main reads (NEW) +"Read(/tmp/codex-resume-body-*)", +// Runner subagent (inherited): write the result JSON main reads "Write(/tmp/codex-runner-result-*)", // Runner-spec discovery: tier 1 (user-scoped install) "Bash(ls ~/.claude/skills/adversarial-review/references/runner.md*)", @@ -162,12 +183,24 @@ chosen config file: // adversarial-review "Bash(git diff*)", "Bash(git status*)", + "Bash(git -C * status --porcelain*)", "Bash(git symbolic-ref*)", "Bash(git rev-parse*)", "Bash(cat /tmp/codex-prompt-* | timeout 600 codex exec *)", "Bash(cd * && cat /tmp/codex-resume-prompt-* | timeout 600 codex exec resume *)", "Bash(find ~/.codex/sessions*)", "Bash(ls -t ~/.codex/sessions*)", + "Bash(bwrap --dev-bind / / --unshare-net /bin/echo ok*)", + "Bash(sha256sum /tmp/codex-*)", + "Bash(diff -q /tmp/codex-*)", + "Write(/tmp/codex-git-pre-*)", + "Write(/tmp/codex-git-post-*)", + "Write(/tmp/codex-inputs-pre-*)", + "Write(/tmp/codex-inputs-post-*)", + "Read(/tmp/codex-git-pre-*)", + "Read(/tmp/codex-git-post-*)", + "Read(/tmp/codex-inputs-pre-*)", + "Read(/tmp/codex-inputs-post-*)", "Write(/tmp/codex-plan-*)", "Write(/tmp/codex-prompt-*)", "Write(/tmp/codex-resume-prompt-*)", @@ -178,8 +211,10 @@ chosen config file: "Bash(mv /tmp/codex-stderr-* /tmp/codex-stderr-*-failed-resume.txt)", "Bash(rm -f /tmp/codex-*)", "Write(/tmp/codex-body-*)", + "Write(/tmp/codex-resume-body-*)", "Read(/tmp/codex-runner-result-*)", "Read(/tmp/codex-body-*)", + "Read(/tmp/codex-resume-body-*)", "Write(/tmp/codex-runner-result-*)", "Bash(ls ~/.claude/skills/adversarial-review/references/runner.md*)", "Bash(ls ~/.claude/plugins/cache/*/*/*/skills/adversarial-review/references/runner.md*)" @@ -191,10 +226,13 @@ chosen config file: **Security note:** The `codex exec` rule allows any `codex exec` invocation -wrapped in `timeout 600`. The skill only uses read-only mode (`-s read-only`), -but Claude Code's permission patterns are prefix-based and cannot enforce flag -constraints. If you prefer tighter control, omit the `codex exec` rule and -approve each invocation manually. +wrapped in `timeout 600`. The default sandbox is `workspace-write` (the reviewer +needs to run tests, builds, and CLI introspection to verify findings — read-only +blocks all of that). Reviewer-side mutation is governed by the prompt-level +"auditor, not contributor" contract plus pre/post `git status --porcelain` and +`sha256sum` snapshots on every dispatch. If you prefer tighter control, see the +[Safety considerations](#safety-considerations) section below for the +`sandbox:read-only` opt-out and the worktree-isolation pattern. ### 4. Use @@ -204,9 +242,24 @@ approve each invocation manually. /adversarial-review code # force code review /adversarial-review path/to/f # review a specific file /adversarial-review xhigh # higher reasoning effort -/adversarial-review model:gpt-5.3-codex # use a different model +/adversarial-review model:gpt-5.4 # use a different model +/adversarial-review sandbox:read-only # block reviewer writes (loses empirical verification) +/adversarial-review approvals:never # boundary crossings fail instead of asking ``` +Overrides can be combined: `/adversarial-review plan xhigh sandbox:read-only`. + +### Defaults + +| Setting | Default | Override | +|---------------------|------------------------------------------------|-----------------------------------------------------------------------------------------| +| Reviewer model | `gpt-5.5` | `model:` | +| Reasoning effort | `high` | `low` / `medium` / `high` / `xhigh` | +| Codex sandbox | `workspace-write` | `sandbox:read-only` / `sandbox:workspace-write` / `sandbox:danger-full-access` / `sandbox:inherit` | +| Approval policy | `on-request` with `auto_review` reviewer | `approvals:user` / `approvals:auto_review` / `approvals:never` | +| Max rounds | `5` | not configurable | +| Operator language | auto-detected from recent messages, fallback English | not configurable | + ## Prompt architecture The skill uses XML-structured prompts with adversarial stance: @@ -224,13 +277,269 @@ The skill uses XML-structured prompts with adversarial stance: See [examples/review-output.md](examples/review-output.md) for a sample review. +## Safety considerations + +The default sandbox is `workspace-write`, not `read-only`. This is a deliberate +trade-off: the reviewer needs to run tests, builds, project CLIs, MCP doc +lookups, and web searches to produce findings worth more than a same-model +self-check. Read-only blocks all of that. + +Reviewer-side mutation is governed by three layers: + +1. **Prompt-level contract** — every review prompt includes a + `` block that constrains the reviewer to read + + verify, not edit. The reviewer must report mutation as a limitation + instead of performing it. +2. **Tracked-tree status detection** — main captures `git status --porcelain` + before and after every Codex dispatch. New modifications, new untracked + files, or status transitions (e.g. clean → modified) on tracked files + trigger a hard stop before fixes are applied. This catches the common + case where the reviewer touches a previously-clean tracked file. +3. **Review-input detection** — main captures `sha256sum` of + `/tmp/codex-{body,plan,resume-body}-*` before and after every dispatch. + Any change is treated identically to tracked-file mutation. + +### Residual risk: already-dirty tracked files + +`git status --porcelain` records status codes, not content hashes. If a +tracked file was already modified (`M`) pre-dispatch and the reviewer +mutates the same file again, the porcelain line stays ` M file` — +the status snapshot diff is empty, and main does not hard-stop. + +**This is an accepted residual safety gap.** Workspace writes are +permitted by the `workspace-write` sandbox and do NOT generate an +approval request; the `approval_policy=on-request` + `auto_review` +machinery only reviews approval requests, so it never sees these +writes. The single protection layer at the skill level is the +`` prompt contract telling the reviewer not to +modify project files. The realistic vector is a reviewer-invoked +verification command with an unintended side effect — a formatter, +snapshot updater (`pytest --update-snapshots`, jest `--ci=false`), +lockfile regeneration (`npm install`, `cargo update`), or code +generator — that legitimately runs but writes to a file the operator +was already editing. Damage is bounded (the mutation remains visible +in `git diff` after the review concludes), but the skill does not +hard-stop at "apply fixes" time. + +Mitigations available to the operator: + +1. **Runtime warning** — at the start of every review, if `git status + --porcelain` shows any already-modified tracked files, the skill + emits a warning naming the count and pointing operators to the + options below. +2. **Commit (or stash) before review** — the simplest defense. A + review of branch commits against `master` with a clean working tree + is fully covered by the porcelain snapshot; any reviewer-side + mutation produces a fresh ` M` line and trips the hard-stop. +3. **`sandbox:read-only` opt-out** — when the working file matters more + than empirical verification by the reviewer, switch to read-only. + Same trade-off as for gitignored state below. + +If this risk class hits in practice, the design notes a clear path +to a content-aware snapshot — see `docs/DESIGN.md §4.20`. + +### Residual risk: gitignored state inside REPO_ROOT + +There is one mutation vector that the snapshots do NOT detect: +`.gitignored` files already present inside `REPO_ROOT`. Examples include +local SQLite databases (`dev.sqlite`), `.env.local`, service-state +directories, and build caches. `git status` ignores them by definition, +and full-tree snapshotting would be too expensive to run every round. + +The realistic exposure is the reviewer running a project test or build +command that side-effects an ignored file — for example `pytest` +triggering an unintended migration on `dev.sqlite` because the test +settings point at it. The damage is bounded (the state is re-seedable; +tests generally read `.env.local`, they do not write to it), but it is +real on default `workspace-write` reviews. + +Three operator-facing mitigations: + +1. **`sandbox:read-only` opt-out** — operators who know they have + sensitive ignored state can opt out per-run: + + ```bash + /adversarial-review sandbox:read-only + ``` + + **Trade-off:** the reviewer loses empirical verification — it cannot + run tests, linters, builds, project CLIs, or most MCP-backed + verification. You trade reviewer capability for write-protection. + Use when the sensitivity of the local state outweighs the value of + empirical findings. + +2. **Worktree isolation** — for repositories where the trade-off above is + unacceptable, run the review in an isolated git worktree: + + ```bash + git worktree add /tmp/review-worktree HEAD + cd /tmp/review-worktree + /adversarial-review + # ... when done ... + cd - + git worktree remove /tmp/review-worktree + ``` + + The worktree shares git history but has its own working tree — the + reviewer can run tests freely, and any gitignored state lives in + `/tmp/review-worktree`, not in your primary checkout. This is the + recommended pattern for sensitive repos. + + **Important — committed work only.** `git worktree add … HEAD` creates a + clean worktree at the HEAD commit and does NOT transfer unstaged or + staged changes from your primary checkout. If your WIP is uncommitted, + the review will only see branch commits (`master...HEAD`) and silently + skip your local edits. Commit (or `git stash`) before running the + recipe, and double-check with `git status --short` inside the new + worktree to confirm what the review will cover. + +3. **Runtime hint** — at the start of every review (unless an explicit + `sandbox:*` override was passed), the skill emits a one-line reminder: + + ``` + ℹ workspace-write in effect; pass sandbox:read-only if sensitive + ignored state lives under REPO_ROOT. + ``` + +This is a deliberate trade-off. Defaulting reviews to `read-only` or to +an isolated worktree would gut the reviewer's empirical verification +capability — which is precisely what makes adversarial cross-model +review more valuable than a same-model self-check. + +## Linux sandbox prerequisites + +With the default `workspace-write` sandbox, Codex relies on `bubblewrap` +(`bwrap`) and unprivileged user namespaces. On Ubuntu 24.04+ this is +restricted by AppArmor and needs explicit setup. + +**Probe whether bwrap works on your host:** + +```bash +bwrap --dev-bind / / --unshare-net /bin/echo ok +``` + +If this prints `ok`, you're done. If it fails (typical errors: +`bwrap: setting up uid map: Permission denied`, or +`bwrap: clone: Operation not permitted`), apply the official bwrap +AppArmor profile: + +```bash +sudo apt install -y apparmor-profiles apparmor-utils +sudo install -m 0644 /usr/share/apparmor/extra-profiles/bwrap-userns-restrict \ + /etc/apparmor.d/bwrap-userns-restrict +sudo apparmor_parser -r /etc/apparmor.d/bwrap-userns-restrict +``` + +Then re-run the probe: + +```bash +bwrap --dev-bind / / --unshare-net /bin/echo ok +``` + +If the probe still fails, see the official Codex sandboxing documentation: +. + +**For installer agents:** do NOT change AppArmor policy silently. Probe +first, show the exact `apt install` / `install` / `apparmor_parser` +commands, request explicit operator permission, apply the profile only +after approval, and re-run the probe. + +**Per-dispatch preflight.** The runner subagent runs the same `bwrap` +probe before every initial / fresh-exec dispatch that selects a +bwrap-backed sandbox mode (`read-only` or `workspace-write`). If the +probe fails, the runner returns a `success + degraded_environmental` +result with a one-line `user_warning` pointing back to this section, +which the lead surfaces and treats as terminal. The preflight is +skipped under `sandbox:inherit` (effective sandbox is unknown until +Codex launches) and under `sandbox:danger-full-access` (no bwrap). + +## Operator language + +The skill detects the operator's language from recent conversation +messages and translates runtime prose accordingly: the runtime hint, +the structural-gate prompt, the workspace-mutation diagnostics, the +final operator summary, and ad-hoc warnings. + +Machine-readable literals stay in English regardless: + +- Severity tags: `[severity: critical|high|medium]` +- Verdict line: `VERDICT: APPROVED` / `VERDICT: REVISE` +- Review section headers: `Summary`, `Findings`, `Verdict` + +Repository documentation (this README, `SKILL.md`, `references/runner.md`, +`docs/DESIGN.md`, specs under `docs/superpowers/specs/`) is always in +English. If you contribute changes, keep documentation paragraphs in a +single language. + +## Final operator summary + +Every terminal state — approved, max rounds reached, not verified, or +aborted — produces a final operator-facing summary in the operator's +language, emitted AFTER the canonical per-state header block and AFTER +the final verbatim reviewer response (where one exists). + +The summary covers: + +- Final status and what it means for what the operator should do next. +- What changed across all rounds (compact, no full diffs). +- Findings applied / re-scoped / rejected, with focus on rejections + and structural changes. +- Whether structural fixes received operator sign-off (or were applied + in autonomous / headless mode). +- Verification performed by the reviewer vs. by the lead vs. still + unverified. +- Remaining findings or risks (for non-approved terminal states). + +The summary is built from per-round decision accounts already shown +earlier in the conversation. Main never reads Codex stdout, stderr, or +rollout files to assemble it. If context compaction has obscured part +of the history, the summary states that explicitly rather than +fabricating details. + ## Troubleshooting **`codex exec` exits with model error.** Some models are unavailable with ChatGPT accounts (e.g. `o3-mini`). -The default `gpt-5.4` works with both ChatGPT and API key auth. +The default `gpt-5.5` works with both ChatGPT and API key auth. Override with `/adversarial-review model:`. +**`codex exec` rejects `-c approval_policy=...` or `-c approvals_reviewer=...`.** +You are on a Codex CLI older than 0.132. Upgrade +(`npm install -g @openai/codex@latest`). The skill emits the `-c` form +unconditionally because the top-level `-a` / `--ask-for-approval` flag was +removed in 0.132. + +**Review aborts with "Reviewer or runner mutated tracked files during dispatch".** +The pre/post `git status --porcelain` snapshot detected changes to tracked +files between dispatch start and runner return. This is a HARD STOP — the +artifact under review may have been silently mutated. Inspect the diff +shown in the diagnostic, decide whether to keep or revert manually, and +re-invoke `/adversarial-review` when the working tree is in the state you +expect. If you suspect a specific verification command in the reviewer's +toolchain is responsible (test runner doing a migration, build script +regenerating a file), pass `sandbox:read-only` next time. + +**Review aborts with "ABORTED — environmental failure before any valid review".** +The initial Codex dispatch returned a `degraded_environmental` review (the +reviewer self-reported it could not run because of sandbox or environment +failure). Common causes on Linux: `bwrap` not installed, AppArmor +restricting unprivileged user namespaces, or a rate-limit / trust-prompt +stub from Codex. Run the [Linux sandbox prerequisites](#linux-sandbox-prerequisites) +probe and apply the AppArmor profile if needed. + +If the failure is the bwrap preflight specifically, `sandbox:read-only` does +NOT bypass it — `read-only` is bwrap-backed and runs the same probe. The +actual bypass options are `sandbox:danger-full-access` (no bwrap, only use +in trusted local debugging), `sandbox:inherit` (trust your local Codex +config), or fixing bwrap/AppArmor per the prerequisites section. + +**`degraded_environmental` on resume.** +The resume produced a non-actionable review (typically a sandbox or +rate-limit issue mid-loop). The skill does NOT count it as a round and +routes through the Step 7.4 fallback chain using the prior round's +severity. If the fallback's fresh-exec also returns +`degraded_environmental`, the review terminates as `NOT VERIFIED`. + **Permission prompts on every action.** Add the permissions from the [setup section](#3-add-permissions). Check that the file is valid JSON and in the right location (project `.claude/settings.local.json` @@ -293,8 +602,11 @@ review correctness. - **Plan Mode and `/tmp` writes.** Writing review prompts to `/tmp` may trigger a permission prompt or cause Plan Mode to exit. Does not affect review correctness. -- **`resume` inherits sandbox.** `codex exec resume` does not accept `-s` — - sandbox is inherited from the original session (always `read-only`). +- **`resume` inherits sandbox.** `codex exec resume` does not accept `-s`, + `-m`, or approval-related `-c` overrides — sandbox and approval mode are + properties of the original session. Changing `sandbox:` or `approvals:` + mid-review requires aborting and re-invoking `/adversarial-review` with + the new override, which starts a fresh review. - **`resume` has no `-C` flag.** The skill captures `REPO_ROOT` via `git rev-parse --show-toplevel` at Step 2 and prefixes every resume with `cd '' && ...`. This requires paths without single quotes; diff --git a/SKILL.md b/SKILL.md index 697558d..cbb4716 100644 --- a/SKILL.md +++ b/SKILL.md @@ -18,8 +18,12 @@ Sends current work for adversarial review through an external AI model (OpenAI C - `/adversarial-review plan` — force plan review - `/adversarial-review code` — force code review - `/adversarial-review ` — review a specific file (argument contains `/` or `.`) -- Override reasoning: `/adversarial-review xhigh` or `/adversarial-review medium` (one of: `medium`, `high`, `xhigh`) -- Override model: `/adversarial-review model:gpt-5.3-codex` (argument with `model:` prefix) +- Override reasoning: `/adversarial-review xhigh` or `/adversarial-review medium` (one of: `low`, `medium`, `high`, `xhigh`) +- Override model: `/adversarial-review model:gpt-5.4` (argument with `model:` prefix) +- Override Codex sandbox: `/adversarial-review sandbox:read-only` (one of: `read-only`, `workspace-write`, `danger-full-access`, `inherit`) +- Override Codex approval policy: `/adversarial-review approvals:user` (one of: `user`, `auto_review`, `never`) + +Overrides can be combined: `/adversarial-review plan xhigh sandbox:read-only`. ## Instructions @@ -52,6 +56,40 @@ Determine what to review. Check in priority order: | Yes | No | **code** — review code changes | | No | No | Ask the user what to review | +#### Step 1 (continued): Capture overrides, detect operator language, emit runtime hint + +**Parse the invocation arguments for overrides** (case-sensitive on the prefix). Apply them to the per-review configuration captured below. Any unrecognized token after stripping the mode and overrides is treated as an unknown argument — surface a one-line "ignoring unknown argument: " warning and continue. + +| Argument shape | Sets | Default if absent | +|-------------------------------------------|---------------------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------| +| `model:` | `CODEX_MODEL = ` | `CODEX_MODEL = gpt-5.5` | +| `low` / `medium` / `high` / `xhigh` | `CODEX_REASONING = ` | `CODEX_REASONING = high` | +| `sandbox:read-only` | `CODEX_SANDBOX = read-only` | `CODEX_SANDBOX = workspace-write` | +| `sandbox:workspace-write` | `CODEX_SANDBOX = workspace-write` | (same as default) | +| `sandbox:danger-full-access` | `CODEX_SANDBOX = danger-full-access`. **Emit a one-line warning** to the operator: `⚠ sandbox:danger-full-access — reviewer can write anywhere on disk; reserve for trusted local debugging.` | (n/a — explicit-only, never default) | +| `sandbox:inherit` | `CODEX_SANDBOX = inherit`. Runner skips bwrap preflight; effective sandbox is whatever the user's Codex config selects. | (n/a — explicit-only) | +| `approvals:auto_review` (default) | `CODEX_APPROVAL_POLICY = on-request`, `CODEX_APPROVALS_REVIEWER = auto_review` | (same as default) | +| `approvals:user` | `CODEX_APPROVAL_POLICY = on-request`, `CODEX_APPROVALS_REVIEWER = null` (omit `-c approvals_reviewer` → Codex falls back to its built-in `user` reviewer, which can ask the operator for approval). Nested approvals may hang the run from the parent Claude session — use only when explicitly desired. | (n/a — explicit-only) | +| `approvals:never` | `CODEX_APPROVAL_POLICY = never`, `CODEX_APPROVALS_REVIEWER = null`. Boundary crossings fail instead of asking. | (n/a — explicit-only) | + +Codex's `untrusted` approval policy is intentionally NOT exposed as an override — the skill needs predictable boundary semantics, not per-command trust prompts. + +**Resume invariant.** Sandbox and approval mode are properties of the original Codex session. `codex exec resume` does NOT accept `-s` or approval-related `-c` flags. If the operator changes `sandbox:` or `approvals:` mid-review, the new value takes effect only on a fresh-exec dispatch (which consumes a round). Surface this in the warning if the operator passes a sandbox/approvals override on a re-invocation of the skill while a prior session is still live. + +**No silent fallback** may change sandbox or approval semantics. If any of the captured values cannot be passed to Codex on the target host (e.g. an installed Codex CLI version that doesn't support `-c approvals_reviewer`), surface an explicit diagnostic before dispatch — do NOT downgrade silently. + +**Detect operator language.** Inspect the last few human-authored messages in the current conversation. If they are predominantly in a non-English language, capture `OPERATOR_LANGUAGE = ` (e.g. `Russian`, `Spanish`, `Japanese`). If detection is ambiguous, default to `OPERATOR_LANGUAGE = English`. Runtime prose shown to the operator (warnings, summaries, intermediate updates) MUST use `OPERATOR_LANGUAGE` when practical. Repository files (this `SKILL.md`, `references/runner.md`, `README.md`, `docs/DESIGN.md`, specs under `docs/superpowers/specs/`) stay in English regardless. + +**Emit the one-line runtime hint** about sandbox defaults — **once per review, before Step 2 starts**. The hint is suppressed when the operator passed an explicit `sandbox:*` override (any of `read-only`, `workspace-write`, `danger-full-access`, `inherit`): + +``` +ℹ workspace-write in effect; pass sandbox:read-only if sensitive ignored state lives under REPO_ROOT. +``` + +Translate the hint into `OPERATOR_LANGUAGE` if non-English. Do NOT repeat per round. + +The conditional warning about already-dirty tracked files lives in Step 2 (it depends on a captured `REPO_ROOT`). + ### Step 2: Generate Session ID, capture REPO_ROOT, determine base branch **REVIEW_ID:** generate yourself, format `{unix_timestamp}-{random_8digit_number}`. @@ -75,6 +113,14 @@ git rev-parse --show-superproject-working-tree If this returns non-empty, the user is inside a git submodule. Tell the user: `You are inside a submodule. The review will be scoped to this submodule (${REPO_ROOT}), not the parent repo. If you meant to review the parent, invoke from there.` Proceed — this is a warning, not an abort. +**Conditional warning about already-dirty tracked files** (skip when `OPERATION=plan`). Now that `REPO_ROOT` is captured and validated, run `git -C "${REPO_ROOT}" status --porcelain` and inspect the output. If any line matches `^[ M][M ] ` (a tracked file already modified in working tree or index pre-review), emit this extra one-line warning (once per review): + +``` +ℹ unstaged or staged tracked-file edit(s) detected. The porcelain mutation snapshot only detects status transitions; reviewer-side content drift of already-dirty files is NOT auto-caught. Commit/stash WIP first for stronger protection, or pass sandbox:read-only. +``` + +Translate to `OPERATOR_LANGUAGE`. Substitute `` with the count of matching lines. Suppressed when no already-dirty tracked files exist. See `docs/DESIGN.md §4.20` and `README.md` "Safety considerations" for the residual-risk rationale. + **Determining base branch (only for `code` and `code-vs-plan` modes):** For `plan` mode — skip base branch detection, proceed to step 3. @@ -174,6 +220,12 @@ DO NOT comment on: formatting, wording style, speculative issues without concret Prefer one strong finding over several weak ones. If the plan is solid, say so clearly — false positives erode trust. + +Judge the plan at its declared level of abstraction. +Do not demand implementation details unless their absence blocks feasibility, +safety, rollback, verification, or a public contract. +If a detail can reasonably be decided during implementation, do not count it +as a finding. @@ -312,14 +364,41 @@ The inlined prompt bodies above contain template placeholders that main must res Substitute `${BASE_BRANCH}` first (it appears nested inside ``), then compute the outer human-readable description based on which diffs have content. Main writes the substituted string to the Write tool — no template placeholders should remain in the body file sent to the runner. -**Capture user overrides for `CODEX_MODEL` / `CODEX_REASONING` at Step 1:** +**Append the operator-language block to the prompt body** when `OPERATOR_LANGUAGE != "English"` (captured in Step 1). Append the following block verbatim AFTER the `` section and BEFORE any trailing content: -The skill supports overrides like `/adversarial-review xhigh`, `/adversarial-review medium`, `/adversarial-review model:gpt-5.3-codex`. At Step 1, capture: +``` + +Respond in the operator's language: . +Keep these machine-readable literals unchanged in English (they are parsed by the lead and must not be translated): +- [severity: critical|high|medium] +- VERDICT: APPROVED +- VERDICT: REVISE +The Summary / Findings / Verdict section headers should also stay in English so the runner's content classifier and triage rg patterns continue to match. + +``` -- `CODEX_MODEL` — default `gpt-5.4`. Overridden by any argument matching `^model:(.+)$`; use the capture group. -- `CODEX_REASONING` — default `high`. Overridden by any argument exactly matching `low`, `medium`, `high`, or `xhigh`. +Substitute the literal name of the detected language for ``. Do NOT translate the block itself — the reviewer reads English instructions and produces prose in the target language. When `OPERATOR_LANGUAGE = "English"`, OMIT the block entirely (default behavior). -These are passed into the runner YAML input block below. +**Reviewer permissions and approval semantics.** The Codex reviewer is an auditor, not a contributor. Append the following block to every prompt body (regardless of mode), AFTER the `` section and AFTER the optional `` block: + +``` + +You may run commands to verify findings when useful: tests, linters, build +commands, git inspection, MCP-backed doc lookups, web search, project CLI +introspection. + +Do not create, edit, delete, commit, or apply fixes to project files. +Prefer commands that do not mutate the working tree. +Do not run commands likely to rewrite generated files, snapshots, migrations, +lockfiles, or configs. +If verification would require mutation, report that limitation instead. +If a command unexpectedly changes files, stop and report it. + +You are an auditor, not a contributor. The lead applies fixes; you find issues. + +``` + +This is the primary safeguard against reviewer-side mutation. Workspace-level mutation detection (see "Workspace mutation snapshot" below) is the secondary safeguard. **Write the prompt body to disk via Write tool:** @@ -357,6 +436,22 @@ REPO=$(git rev-parse --show-toplevel 2>/dev/null) && ls "$REPO/references/runner Save the resolved absolute path as `RUNNER_SPEC_PATH`. Do NOT attempt to extract the path from any "Base directory for this skill:" line in the conversation — that line is a system injection Claude cannot reliably read from inside its own context. +**Workspace mutation snapshot (pre-dispatch):** + +Mutation detection runs at two layers: the repo tree (git-tracked + new untracked files inside `REPO_ROOT`) and the skill's `/tmp` review inputs. A third class — gitignored files already inside `REPO_ROOT` — is documented as a known, operator-mitigated risk and is NOT detected automatically (see `README.md` "Safety considerations" and the runtime hint from Step 1). + +Capture both layers BEFORE every runner dispatch (initial, resume, fresh-exec): + +```bash +git -C "${REPO_ROOT}" status --porcelain > /tmp/codex-git-pre-${REVIEW_ID}.txt +sha256sum /tmp/codex-body-${REVIEW_ID}.md \ + /tmp/codex-plan-${REVIEW_ID}.md \ + /tmp/codex-resume-body-${REVIEW_ID}.md 2>/dev/null \ + > /tmp/codex-inputs-pre-${REVIEW_ID}.sha +``` + +The `2>/dev/null` is deliberate: not every review has all three files (plan-mode reviews skip the body, round-1 dispatches skip resume-body). Missing files are silently elided from the snapshot and are still caught later if they appear unexpectedly. + **Dispatch the runner subagent via Agent tool:** **Do NOT Read `${RUNNER_SPEC_PATH}` in main.** Pass the path to the subagent; it reads the spec itself. This keeps runner.md (~12K) out of main's context — both the Read result AND the Agent prompt duplication. Saves ~12K per round × up to 5 rounds per review. @@ -374,8 +469,11 @@ Read your full instruction spec at ${RUNNER_SPEC_PATH} and follow the steps ther REVIEW_ID: 1711872000-48217593 REPO_ROOT: /home/dementev/sources/myproject OPERATION: initial -CODEX_MODEL: gpt-5.4 +CODEX_MODEL: gpt-5.5 CODEX_REASONING: high +CODEX_SANDBOX: workspace-write +CODEX_APPROVAL_POLICY: on-request +CODEX_APPROVALS_REVIEWER: auto_review PROMPT_BODY_PATH: /tmp/codex-body-1711872000-48217593.md RESULT_PATH: /tmp/codex-runner-result-1711872000-48217593.json --- @@ -383,6 +481,8 @@ RESULT_PATH: /tmp/codex-runner-result-1711872000-48217593.json Substitute the actual resolved `${RUNNER_SPEC_PATH}` (absolute path) and real values for every other placeholder. `RESULT_PATH` always follows the pattern `/tmp/codex-runner-result-${REVIEW_ID}.json`. +Use the values captured in Step 1 for `CODEX_MODEL`, `CODEX_REASONING`, `CODEX_SANDBOX`, `CODEX_APPROVAL_POLICY`, and `CODEX_APPROVALS_REVIEWER`. When `CODEX_APPROVALS_REVIEWER` is `null` (set by `approvals:user` or `approvals:never`), pass the literal string `null` as the YAML value — the runner interprets it and omits the `-c approvals_reviewer` flag. + **Do NOT run the Agent tool call in background.** Wait for the subagent to return. (Runner's own codex exec is also synchronous per runner Step R3.) **Parse the subagent's response — two-channel protocol:** @@ -391,7 +491,52 @@ Apply the regex `RUNNER_RESULT_AT:\s+(\S+)` (UNANCHORED — matches anywhere in If the regex finds NO match in the subagent's response, fall back to a Glob for the deterministic path `/tmp/codex-runner-result-${REVIEW_ID}.json` — REVIEW_ID is already known to main. If Glob also returns nothing, treat as `infra_error` with `errors: "runner did not write result file at deterministic path and did not emit RUNNER_RESULT_AT line"` and abort. -Read the file at the resolved path. Parse as JSON. Extract `result`, `verdict`, `review_file`, `codex_session_id`, `errors`, `user_warning`, `archived_stdout`, `archived_stderr`. +Read the file at the resolved path. Parse as JSON. Extract `result`, `verdict`, `review_file`, `codex_session_id`, `errors`, `user_warning`, `archived_stdout`, `archived_stderr`, `review_quality`, and the `triage` object. + +**Backward compatibility for `review_quality` / `triage`.** A legacy runner result that omits `review_quality` and `triage` is treated as `review_quality = "unknown"` and `triage = { status: "skipped", finding_count: 0, max_severity: "none", covered_critical: 0, covered_high: 0, covered_medium: 0, truncated: false, needs_lead_judgment: false }`. No abort, no operator prompt; the legacy result is consumed as if the runner had emitted those values explicitly. + +**Workspace mutation snapshot (post-dispatch).** BEFORE consulting the dispatch table below, and BEFORE applying any fixes, capture the post-state and diff against the pre-state: + +```bash +git -C "${REPO_ROOT}" status --porcelain > /tmp/codex-git-post-${REVIEW_ID}.txt +sha256sum /tmp/codex-body-${REVIEW_ID}.md \ + /tmp/codex-plan-${REVIEW_ID}.md \ + /tmp/codex-resume-body-${REVIEW_ID}.md 2>/dev/null \ + > /tmp/codex-inputs-post-${REVIEW_ID}.sha + +diff -q /tmp/codex-git-pre-${REVIEW_ID}.txt \ + /tmp/codex-git-post-${REVIEW_ID}.txt +diff -q /tmp/codex-inputs-pre-${REVIEW_ID}.sha \ + /tmp/codex-inputs-post-${REVIEW_ID}.sha +``` + +Two diffs, two cases: + +1. **Tracked-file mutation** — the `git status --porcelain` diff shows new modified (`^[ M]M`) or deleted (`^[ D]D`) entries that didn't exist pre-dispatch, OR untracked files (`^\?\?`) that look like edits to real source files (not editor scratchpads). **HARD STOP** before applying any fixes. Surface an operator diagnostic: + + ``` + ❌ Reviewer or runner mutated tracked files during dispatch. + Pre-state vs post-state diff: + + Aborting before fixes. Inspect the diff and decide whether to revert + or keep the changes manually. Re-run /adversarial-review when ready. + ``` + + Then skip Steps 5–9 and exit. Do NOT proceed to apply fixes — the artifact under review may have been silently mutated, invalidating the round. + +2. **Untracked generated artifacts only** — `^\?\?` entries that look benign (e.g. build caches, log files). Warn the operator but allow continuation: + + ``` + ⚠ Reviewer left untracked files behind: . + Proceeding with the round, but please review whether these should be + .gitignored or removed. + ``` + +3. **`/tmp` review-input mutation** — the `sha256sum` diff is non-empty. Treat this exactly like tracked-file mutation: hard stop and surface the diagnostic. The reviewer should NEVER modify its own prompt body or plan file. + +4. **No mutation** — both diffs are empty (or only show whitespace differences from the eager pre-snapshot). Proceed to the dispatch table. + +The pre/post snapshot pair is repeated for every Codex dispatch (Step 4 initial, Step 7 resume, Step 7.4 fresh-exec). It is NOT optional — skipping it forfeits the only detection of reviewer-side mutation that does not depend on the reviewer self-reporting. **If `user_warning` is non-null, surface it as a SEPARATE short user-visible message BEFORE the Step 5 verbatim-review message.** Format: @@ -401,17 +546,26 @@ Read the file at the resolved path. Parse as JSON. Extract `result`, `verdict`, Emit this on its own turn — do NOT concatenate into the Step 5 `## Adversarial Review — Round N` header message (that message's body must remain the review's verbatim content, nothing else). Emit the warning FIRST, then the Step 5 message. This preserves both the pre-refactor §2.4.4 "no-op refresh" diagnostic AND the Step 5 verbatim-display contract. -Dispatch based on `result`: +**Dispatch based on `result` × `OPERATION` × `review_quality`.** The table below is operation-aware: `degraded_environmental` on the initial dispatch is terminal because there is no prior valid round to fall back to, whereas the same classification on a resume can be re-routed through the existing fresh-exec fallback chain. -| `result` value | Main thread action | -|---|---| -| `success` | Save `codex_session_id` (keep prior if `null` per §2.4.4). Surface `user_warning` if set. Proceed to Step 5. | -| `timeout` | **TERMINAL — do NOT re-dispatch.** Runner already attempted twice internally (R4.1 + R5 retry = 2 × 10min). Tell user: "Reviewer timed out after two attempts (20 minutes total)." Abort the skill. User can re-invoke `/adversarial-review` to start a fresh review. | -| `launch_failure` | **TERMINAL — do NOT re-dispatch.** The runner already retried once internally (Step R5). Show `errors` to user, abort the skill. This keeps the total-attempts-per-round invariant at 2 (matches pre-refactor: 1 initial + 1 retry). | -| `infra_error` | Show `errors` to user (infrastructure: /tmp not writable, stderr file missing, RUNNER_RESULT_AT line absent). Abort. | -| `input_error` | Bug in orchestration. Show `errors` to user. Abort. | +| `result` | `OPERATION` | `review_quality` | Main thread action | +|------------------|---------------|--------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `success` | any | `valid` | Save `codex_session_id` (keep prior if `null` per §2.4.4). Surface `user_warning` if set. Proceed to Step 5: show review verbatim, run evaluation matrix in Step 6, advance round. | +| `success` | any | `degraded_content` | Surface `user_warning`. Show review verbatim per Step 5. THEN ask the operator: "Reviewer output flagged as low-confidence — advance the round anyway, or abort?" Default to advance after a short wait if running headlessly. | +| `success` | any | `unknown` | Surface `user_warning`. Treat as `valid` for round advancement (Step 5 + Step 6). Note the unknown classification in the final operator summary (Step 8). | +| `success` | `initial` | `degraded_environmental` | Surface `user_warning`. Do NOT show review verbatim (the file does not contain a real review). Treat as **terminal infrastructure failure** — abort the skill with the operator diagnostic. No prior valid review exists to fall back to. | +| `success` | `resume` | `degraded_environmental` | Surface `user_warning`. Do NOT show review verbatim. Do NOT count this dispatch as a round. Route to the Step 7.4 fallback chain, using the **prior round's** maximum severity for the fallback decision. | +| `success` | `fresh-exec` | `degraded_environmental` | Surface `user_warning`. Do NOT show review verbatim. Treat as **terminal not-verified** (the Step 8 NOT VERIFIED branch). Fresh-exec already was the fallback chain; a second environmental failure means the environment is reliably broken. | +| `timeout` | any | n/a | **TERMINAL.** Runner already attempted twice internally (R4.1 + R5 retry = 2 × 10min). Tell user: "Reviewer timed out after two attempts (20 minutes total)." Abort the skill. User can re-invoke `/adversarial-review` to start fresh. | +| `launch_failure` | `initial` | n/a | **TERMINAL.** The runner already retried once internally (Step R5). Show `errors` to user, abort the skill. | +| `launch_failure` | `resume` | n/a | **TERMINAL for this round.** Route to the Step 7.4 fallback chain (runner already archived stdout/stderr to `-failed-resume.*` per `archived_stdout`/`archived_stderr`). | +| `launch_failure` | `fresh-exec` | n/a | **TERMINAL.** Fresh-exec was the fallback; show `errors`, abort with the not-verified terminal state. | +| `infra_error` | any | n/a | Show `errors` to user (infrastructure: `/tmp` not writable, stderr file missing, RUNNER_RESULT_AT line absent, bwrap preflight failed). Abort. | +| `input_error` | any | n/a | Bug in orchestration. Show `errors` to user. Abort. | -**Round-level attempt invariant:** exactly ONE runner dispatch per round. Every failure result is terminal at main. The runner owns the full retry budget (≤2 attempts per dispatch, internal) regardless of failure type. Total codex invocations per round ≤ 2. +Whenever the dispatch outcome says "Surface `user_warning`", emit it on its own turn BEFORE the Step 5 verbatim message (or BEFORE the abort message, when there is no Step 5). Never concatenate the warning into the Step 5 header. + +**Round-level attempt invariant:** exactly ONE runner dispatch per round (except when `degraded_environmental` on resume routes through fallback without counting as a round). The runner owns the full retry budget (≤2 attempts per dispatch, internal) regardless of failure type. Total codex invocations per round ≤ 2. > **CRITICAL — main thread does NOT read stdout/stderr/JSONL/rollout files BY CONTENT.** Those live and die inside the subagent. Main reads: the runner result JSON at `RESULT_PATH`, the review file at `review_file`, and nothing else from `/tmp/codex-*`. Archival `mv` (on resume failure) is done by the runner, not main — main never references `/tmp/codex-stdout-*` or `/tmp/codex-stderr-*` in any Bash argv. @@ -443,7 +597,7 @@ If any check fails → this is a **launch failure** (model produced no actionabl Message format: ``` -## Adversarial Review — Round N (mode: , model: gpt-5.4) +## Adversarial Review — Round N (mode: , model: ) ``` @@ -454,54 +608,198 @@ Message format: - `VERDICT: REVISE` → Step 6 (Fixes). - Maximum rounds reached (5 rounds) → Step 8 with the max-rounds note. -### Step 6: Apply fixes +### Step 6: Evaluate findings, gate structural fixes, then apply > **Precondition gate (check first).** Before calling any Edit, Write, or other fix-applying tool: confirm that you have already sent a user-visible message in THIS round whose body contains the verbatim review text (short `⚠ ` diagnostic messages do NOT count). If you have not — STOP. Go back to Step 5 and send the review message now. This is the same rule that protects the "user sees the review" contract; a literal reader may otherwise slip past it. -Based on the reviewer's findings: +**External feedback = suggestions to evaluate, not orders to follow.** Reviewer findings are inputs to your decision; applying them blindly causes real damage when the reviewer is technically wrong (e.g. a critical "bug" that is actually a feature request, or a security flag based on a misread of the threat model). Use `superpowers:receiving-code-review` when available; the key principles are inlined below as the always-available fallback. -**For plan review:** fix the plan — address each finding. Update the plan file (or temp file). Show the user: +#### Step 6.1: Build the evaluation matrix + +For each finding in the verbatim review, fill out one row: + +| # | Severity | Verified? | Type | Action | +|---|----------|------------------------------------|-----------------|----------------------| +| 1 | high | ✓ Context7 confirms behavior | architectural | accept | +| 2 | critical | ✗ cited issue is feature request | tool-mechanic | reject with reasoning| +| 3 | medium | ✓ small repro confirms | tool-mechanic | accept | +| 4 | medium | re-scoped to docs-only fix | architectural | re-scope | + +`Action` options are EQUAL first-class outcomes: + +- **`accept`** — finding is valid as stated; apply the proposed fix (or a minimal variant that resolves it). +- **`reject with reasoning`** — finding is technically wrong, out of scope, or contradicts an explicit user requirement; do NOT apply, and prepare a technical counter-argument for the re-review prompt (Step 7). +- **`re-scope`** — finding is partially valid; apply a narrower fix than the reviewer proposed (e.g. clarify wording instead of restructuring the section), and explain the narrowing in the re-review prompt. + +Use runner triage (`triage.finding_count`, `triage.max_severity`, `triage.needs_lead_judgment`) ONLY as a hint to prioritize. The matrix is built from the verbatim review, not from triage; triage is too cheap to be authoritative. + +**Verification methods by finding type:** + +| Finding type | What constitutes verification | +|------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------| +| Architectural / design | Reasoning + codebase grep, plus a pattern check against existing code | +| Tool-mechanic (DSL syntax, config parser, API contract, library behavior) | **Empirical test on the real system** — reasoning alone is not enough. Open cited upstream issues by URL. | +| Style / convention | Match against actual codebase conventions | +| Security | Reasoning + concrete threat model | + +Tool-mechanic findings are the most dangerous to accept on reasoning alone — mental models of obscure tools are often wrong. If the reviewer cites an upstream issue or doc, **open it**; citations age, issues get reclassified, and the cited number may not describe what the reviewer thinks it does. + +**Receiving-feedback principles** (inlined from `superpowers:receiving-code-review` for portability): + +- Read every finding end-to-end before reacting. +- Restate each finding's technical claim in your own words (mentally — don't pad the response with it). +- Verify against codebase / docs / a quick run before accepting. +- Push back when wrong, with technical reasoning, not deference. +- No performative agreement ("you're absolutely right" is a violation of the discipline). +- Skip thanks. State the fix or the reasoning. + +#### Step 6.2: Classify accepted/re-scoped fixes as structural vs non-structural + +After every finding has an `Action`, walk the matrix once and classify each `accept` / `re-scope` row as **structural** or **non-structural**. `reject with reasoning` rows skip this step (nothing is applied). + +**Structural fixes** — pause the operator before applying: + +- Invocation grammar or argument semantics (new/removed arg, renamed mode, changed value semantics). +- Output format or parsed literals (e.g. the `VERDICT: APPROVED|REVISE` line, severity tags, section headers, named workflow states). +- Workflow steps, fallback semantics, or terminal states. +- Sandbox, approval, or security guarantees. +- Public configuration semantics (what an override does). +- Schema, migration, or data format changes. +- Broad architectural rewrites. +- Any fix whose scope you are uncertain about — when in doubt, classify as structural. + +**Non-structural fixes** — apply without pausing: + +- Wording / phrasing changes that do not change semantics. +- Correcting factual inaccuracies (wrong API name, wrong tool mechanic, wrong attribution). +- Removing outdated comments or examples. +- Adding clarifying sentences or examples that do not change observable behavior. +- Internal heuristic refinements with no externally visible effect. + +#### Step 6.3: Structural operator gate (one pause per round) + +Apply the **batch-pause rule**: do NOT pause once per fix. Walk the entire matrix first; then: + +- If structural count is **zero** → apply everything (non-structural accepts + re-scopes) without pausing. No operator gate needed. +- If structural count is **≥ 1** AND the operator is present (a direct human message exists earlier in this session AND the host exposes a user-facing channel) → make exactly **one** pause showing: + + ``` + ### Round N — structural fixes pending operator sign-off + + **Structural (need go/no-go):** + - [#N — one-line description] + - ... + + **Non-structural (will auto-apply):** + - [#M — one-line description] + - ... + + **Rejected with reasoning (informational):** + - [#K — one-line description] + - ... + + Approve the structural batch? (yes / no / select specific items) + ``` + + Wait for operator response before applying any structural fix. Non-structural fixes still auto-apply. + +- If structural count is **≥ 1** AND the operator explicitly requested autonomous mode (e.g. `/adversarial-review` invoked from a scheduled task, or an explicit "go ahead without asking" earlier in the conversation) → apply all structural fixes without pausing, but record this fact for the final operator summary (Step 8): + + ``` + Structural fixes applied without operator sign-off due to autonomous mode: + - [#N — description] + - ... + ``` + +- If structural count is **≥ 1** AND no operator is reachable (headless / scheduled run with no explicit autonomous flag) → apply structural fixes anyway and record the same "applied without sign-off" note for Step 8. Refusing to apply would leave the artifact half-fixed; the operator reviews after the fact. + +#### Step 6.4: Apply fixes + +For each `accept` and `re-scope` row: + +- **Plan review** — update the plan file (or temp file). Make the change minimal: address the specific finding, don't refactor surrounding sections. +- **Code review** — edit files, run tests if applicable, run a build if the change is non-trivial. +- **Code-vs-plan** — update whichever side is wrong (plan or code), per the matrix. + +**Verify your own technical claims before publishing them.** When a fix or the re-review reply makes a claim about tool mechanics (DSL syntax, config parser, API contract, library behavior): + +- If a quick test is possible, run it (a small repro, `docker run …`, a real database container) — not "I think this works". +- If a quick test is not possible, frame the claim as a hypothesis ("seems to", "needs verification") rather than as fact. + +**Skip** a fix if it contradicts an explicit user requirement — note this in the re-review reply with reasoning, not silent omission. + +Show the user a brief account: ``` -### Fixes (Round N) -- [What was changed and why, one item per finding] +### Round N fixes +- Applied: [#1 — what changed, 1 line] +- Re-scoped: [#3 — what changed, why narrower] +- Rejected: [#2 — short reason; full reasoning goes to the reviewer in Step 7] ``` -**For code review:** fix the code directly — edit files, run tests if applicable. Show the user: +#### Step 6.5: Severity-decline soft signal + +After applying, glance at the round-by-round severity trajectory. Expect severity to decline across rounds: ``` -### Fixes (Round N) -- [What was fixed and why, one item per finding] +R1: 3 critical, 6 high, 5 medium (typical opening) +R2: 1 high, 1 medium, 3 low (good) +R3: 1 high (closing in) +R4: APPROVED (terminal) ``` -**Skip** a fix if it contradicts the user's explicit requirements — note this for the user. +If severity stays flat (e.g. `high → high → high` across three consecutive rounds), something is structurally off — the lead may not understand the technology, the reviewer may be looping on the same misunderstanding, or the artifact has a deep problem that surface fixes can't reach. Pause and surface to the operator: + +``` +⚠ Severity has stayed at for rounds. This usually means +either (a) the artifact has a structural problem the current fixes +are not addressing, or (b) the reviewer is misreading something the +lead and reviewer disagree about. Continue, switch approach, or +abort? +``` + +This is a soft signal, not a hard gate. Default to continuing if the operator does not respond. ### Step 7: Resubmit to Codex (Rounds 2-5) **Resume is the primary path.** Saves tokens and preserves session context. A fresh `codex exec` without resume is an **emergency fallback** when resume itself fails. -**Step 7.1: Write the resume prompt body to disk.** +**Step 7.1: Write the structured resume prompt body to disk.** -Write `/tmp/codex-resume-body-${REVIEW_ID}.md` containing: +The re-review body is NOT a "I applied your feedback, please re-check" note. It is a structured response that lets the reviewer (a) verify the applied fixes resolve the original findings, (b) contest the rejections with reasoning, and (c) catch new issues introduced by the fixes. Write `/tmp/codex-resume-body-${REVIEW_ID}.md` containing: ``` -I've revised based on your feedback. +I've evaluated the findings. -Here's what I changed: -[List of fixes from Step 6] +## Applied +- [#N]: [what was changed and why, 1–2 lines] +- ... -Re-review with the same adversarial stance. Focus on: -1. Whether my fixes actually resolve the reported issues -2. Any NEW issues introduced by the fixes +## Re-scoped +- [#N]: [narrower scope, with reasoning for the narrowing] +- ... -End with VERDICT: APPROVED or VERDICT: REVISE +## Rejected with reasoning +- [#N]: [technical reason for not applying — not "I disagree", but a concrete counter-argument the reviewer can engage with] +- ... + +## Specific asks for re-review +1. Are my rejections technically valid? Where I rejected with reasoning, do you accept the counter-argument or push back? +2. Did the applied / re-scoped fixes resolve the original findings? +3. Did the fixes introduce any new issues? ``` -Substitute the fixes list from Step 6 (one bullet per finding addressed). Do NOT include the session marker — the subagent adds it. +Substitute the lists from Step 6's evaluation matrix (one bullet per finding per section). Sections with zero items can be omitted, but `## Specific asks for re-review` is always present. Do NOT include the session marker — the subagent adds it. + +The three-section shape (Applied / Re-scoped / Rejected with reasoning) gives the reviewer a chance to contest the rejections. A re-review that says "your rejection of #2 is valid; here's why" is just as useful as one that fixes new issues — both keep the loop honest. + +**Sonnet triage metadata from Step 4 is NOT passed to the reviewer.** Codex sees the verbatim findings (already in its conversation context from the prior round) and the lead's structured response. The runner's `triage.*` fields are an internal hint for the lead, never forwarded to Codex. **Step 7.2: Dispatch the runner subagent for resume.** -Same Agent tool invocation as Step 4 (bootstrap instruction with `${RUNNER_SPEC_PATH}` + YAML input block; subagent Reads the spec itself). Reuse the `RUNNER_SPEC_PATH` resolved in Step 4 (do not re-resolve). Input block: +Same Agent tool invocation as Step 4 (bootstrap instruction with `${RUNNER_SPEC_PATH}` + YAML input block; subagent Reads the spec itself). Reuse the `RUNNER_SPEC_PATH` resolved in Step 4 (do not re-resolve). Run the **pre-dispatch workspace mutation snapshot** described in Step 4 (`git status --porcelain` + `sha256sum` of the three `/tmp/codex-*-body-*` paths) BEFORE invoking the Agent tool. + +Input block: ```yaml --- @@ -516,19 +814,23 @@ CODEX_SESSION_ID: --- ``` -**Step 7.3: Parse the two-channel result.** +Sandbox and approval fields (`CODEX_SANDBOX`, `CODEX_APPROVAL_POLICY`, `CODEX_APPROVALS_REVIEWER`) are deliberately omitted from the resume YAML — `codex exec resume` does NOT accept these flags; the runner ignores them on `OPERATION=resume`. If the operator wants a different sandbox or approval policy for the rest of the review, the only path is to abort and re-invoke `/adversarial-review` with new overrides, which starts a fresh review with a fresh REVIEW_ID. -Extract `RUNNER_RESULT_AT:` line (same tolerant regex + Glob fallback as Step 4), read the JSON file, extract fields. If `user_warning` is non-null, emit it as its own `⚠ ` message BEFORE any other action (including before the Step 5 verbatim review) — see Step 4's user_warning rule. +**Step 7.3: Parse the two-channel result and consult the dispatch table.** -| `result` value | Main thread action | -|---|---| -| `success`, verdict `APPROVED` | Read `review_file`, go to Step 5 (it will dispatch to Step 8 on APPROVED). | -| `success`, verdict `REVISE` | Save new `codex_session_id`. If the subagent returned null (zero-find resume), keep the prior id per §2.4.4 — `user_warning` will already have been surfaced. Go to Step 5. | -| `timeout` | **TERMINAL for this round** — runner already attempted twice. Route to fallback below. (Fresh-exec is a NEW round from the 5-round counter — its own ≤2-attempts budget applies.) No user-offered retry; that would compound. | -| `launch_failure` | **TERMINAL for this round** — runner already retried once internally. Route to fallback below (runner already archived stdout/stderr to `-failed-resume.*` — paths in `archived_stdout` / `archived_stderr`). | -| `infra_error` | Show `errors` to user, abort. | +Extract the `RUNNER_RESULT_AT:` line (same tolerant regex + Glob fallback as Step 4), read the JSON file, extract all 11 top-level fields. Run the **post-dispatch workspace mutation snapshot** (see Step 4) and treat tracked-file or `/tmp`-input mutation as a hard stop per the same rules. -**Round-level attempt invariant:** exactly ONE runner dispatch per resume round. Every failure result routes to fallback (not re-dispatch within the same round). Fallback's fresh-exec dispatch consumes a NEW round from the 5-round counter, which has its own independent 2-attempts-per-round budget. Total codex invocations per round ≤ 2 regardless of failure type — matches pre-refactor; closes Round-2 finding #1. +Surface `user_warning` if non-null. Then consult the **operation-aware dispatch table in Step 4** with `OPERATION=resume`. In particular: + +- `success` + `review_quality=valid` and `verdict=APPROVED` → Step 8 (approved). +- `success` + `review_quality=valid` and `verdict=REVISE` → save new `codex_session_id` (keep prior if `null` per §2.4.4) and go to Step 5. +- `success` + `review_quality=degraded_environmental` → do NOT show review verbatim, do NOT count as a round, route to the Step 7.4 fallback chain using the prior round's severity. +- `success` + `review_quality=degraded_content` → show verbatim per Step 5, ask the operator whether to advance. +- `success` + `review_quality=unknown` → treat as `valid` for advancement; note in the final summary. +- `timeout` or `launch_failure` → route to the Step 7.4 fallback chain (runner already archived stdout/stderr to `-failed-resume.*` per `archived_stdout`/`archived_stderr` on `launch_failure`). +- `infra_error` or `input_error` → show `errors`, abort. + +**Round-level attempt invariant:** exactly ONE runner dispatch per resume round. Every failure result routes to fallback (not re-dispatch within the same round). Fallback's fresh-exec dispatch consumes a NEW round from the 5-round counter, which has its own independent 2-attempts-per-round budget. Total codex invocations per round ≤ 2 regardless of failure type. The `degraded_environmental` outcome on resume is the one exception that does NOT count as a round — it routes through fallback without consuming the round counter, because the resume produced no usable review. **Step 7.4: Fallback chain** — triggered by `launch_failure` or repeated `timeout` from the runner. @@ -555,11 +857,18 @@ Options: Dispatch the runner subagent with `OPERATION=fresh-exec` (same input schema, new PROMPT_BODY_PATH pointing at the rebuilt prompt). The fresh-exec consumes one round from the 5-round counter. Return to Step 5 with the new review. -### Step 8: Final result +### Step 8: Final result + operator summary + +Every terminal state emits TWO messages, in this order: + +1. **Per-state header block** (templates below) — the canonical "what happened" framing in English. +2. **Operator summary** — a separate operator-facing summary in `OPERATOR_LANGUAGE` (captured in Step 1; English by default). The summary comes AFTER the final verbatim reviewer response (if any) and does NOT replace it. + +#### Terminal state templates **Approved:** ``` -## Adversarial Review — Summary (mode: , model: gpt-5.4) +## Adversarial Review — Summary (mode: , model: ) **Status:** Approved after N round(s) @@ -571,20 +880,20 @@ Dispatch the runner subagent with `OPERATION=fresh-exec` (same input schema, new **Maximum rounds reached:** ``` -## Adversarial Review — Summary (mode: , model: gpt-5.4) +## Adversarial Review — Summary (mode: , model: ) **Status:** Maximum reached (5 rounds) — not fully approved **Remaining findings:** -[Unresolved issues] +[Unresolved issues from the last round] --- **The reviewer still has findings. Please review them and decide how to proceed.** ``` -**Not verified** (resume failed and the operator chose to conclude, or headless with only medium severity): +**Not verified** (resume failed and the operator chose to conclude, headless with only medium severity, or a second `degraded_environmental` on fresh-exec): ``` -## Adversarial Review — Summary (mode: , model: gpt-5.4) +## Adversarial Review — Summary (mode: , model: ) **Status:** NOT VERIFIED — fixes applied, reviewer did not re-verify @@ -598,6 +907,42 @@ Dispatch the runner subagent with `OPERATION=fresh-exec` (same input schema, new **WARNING: This is NOT an approval. Fixes were applied but never verified by the reviewer. Manual review is required before merging.** ``` +**Aborted due to environmental failure** (initial dispatch returned `success + degraded_environmental`, or a workspace-mutation hard stop, or a `bwrap` preflight failure surfaced as `infra_error`): +``` +## Adversarial Review — Summary (mode: , model: ) + +**Status:** ABORTED — environmental failure before any valid review + +**Diagnostic:** +[user_warning or errors from the runner result] + +--- +**No review was produced. Inspect the diagnostic and the README's "Safety considerations" / "Linux sandbox prerequisites" sections, then re-invoke /adversarial-review when the environment is ready.** +``` + +#### Operator summary (always emitted) + +After the per-state header block, emit a separate operator-facing summary in `OPERATOR_LANGUAGE`. This summary is built from per-round decision summaries already shown earlier in the conversation (the matrix in Step 6.1, the fix-account in Step 6.4, the verbatim reviews in Step 5). Do NOT re-read Codex stdout/stderr/rollout files; main never has those in context. + +Include: + +- **Final status** — approved, maximum rounds reached, not verified, or aborted. +- **What changed across all review rounds** — a compact list of artifact changes (one bullet per file/section, not full diffs). +- **Findings applied / re-scoped / rejected** — counts per round, with one-line descriptions only for findings the operator should pay attention to (rejections, re-scopes, structural fixes). +- **Structural changes** — whether structural fixes were applied, and whether operator sign-off was obtained or was skipped due to autonomous / headless mode. +- **Verification performed and NOT performed** — what the reviewer ran vs. what the lead verified vs. what is still unverified. +- **Remaining findings or risks** — only for non-approved terminal states; otherwise omit this section. +- **Explanation of the status** — one sentence on what the status means for what the operator should do next (especially for `NOT VERIFIED` and `ABORTED`). + +Constraints on the summary: + +- Do NOT include full diffs. +- Do NOT repeat full reviewer findings verbatim unless an unresolved finding still matters. +- Keep it concise and operator-useful. +- If context compaction has made the per-round history incomplete (a known limitation — see `docs/DESIGN.md §9.2`), state that limitation explicitly in the summary instead of inventing details. A summary that says "round 2 details unavailable due to compaction" beats a fabricated round-2 account. + +Render the summary in `OPERATOR_LANGUAGE`. Section headers and severity tags stay in English so the operator can grep them back if needed; everything else is in the operator's language. + ### Step 9: Cleanup **Conditional on terminal state:** @@ -623,7 +968,12 @@ rm -f /tmp/codex-plan-${REVIEW_ID}.md \ /tmp/codex-stdout-${REVIEW_ID}-failed-resume.jsonl \ /tmp/codex-stderr-${REVIEW_ID}-failed-resume.txt \ /tmp/codex-body-${REVIEW_ID}.md \ - /tmp/codex-runner-result-${REVIEW_ID}.json + /tmp/codex-resume-body-${REVIEW_ID}.md \ + /tmp/codex-runner-result-${REVIEW_ID}.json \ + /tmp/codex-git-pre-${REVIEW_ID}.txt \ + /tmp/codex-git-post-${REVIEW_ID}.txt \ + /tmp/codex-inputs-pre-${REVIEW_ID}.sha \ + /tmp/codex-inputs-post-${REVIEW_ID}.sha ``` If the user declined `rm` — continue without error. @@ -644,12 +994,19 @@ Do NOT delete plan files that existed before the review (only temp files created - **Runner is dispatched via Agent tool** with `subagent_type: general-purpose, model: sonnet`. Agent tool call is synchronous (not `run_in_background`). - **ALL runner failure results are TERMINAL at main** (`launch_failure`, `timeout`, `infra_error`, `input_error`). Runner retries once internally on ANY failure. Main does NOT re-dispatch and does NOT offer the user a retry — those lanes would compound retries across layers. Total codex invocations per round ≤ 2 (matches pre-refactor invariant: 1 initial + 1 retry). Fresh-exec fallback is a NEW round with its own independent 2-attempts budget. - **`user_warning` from the runner must be surfaced to the user** on a single line BEFORE any other action. This preserves the pre-refactor §2.4.4 "both tiers empty, continuing with previous ID" diagnostic. -- **`CODEX_MODEL` / `CODEX_REASONING`** in the runner input schema refer to the model codex CLI launches (e.g. `gpt-5.4`). The runner's OWN model is Sonnet, set via Agent tool's `model: "sonnet"`. Do NOT conflate. +- **`CODEX_MODEL` / `CODEX_REASONING` / `CODEX_SANDBOX` / `CODEX_APPROVAL_POLICY` / `CODEX_APPROVALS_REVIEWER`** in the runner input schema refer to the codex-exec invocation (default model `gpt-5.5`, default sandbox `workspace-write`, default approval policy `on-request` with `auto_review` reviewer). The runner's OWN model is Sonnet, set via Agent tool's `model: "sonnet"`. Do NOT conflate. Sandbox and approval fields apply to `OPERATION=initial` and `OPERATION=fresh-exec` only; `codex exec resume` ignores them because they are properties of the original session. - **Resume is the primary path for rounds 2-5.** Fresh-exec fallback consumes one round from the 5-round counter. -- **Step 9 cleanup `rm` glob is UNCHANGED from pre-refactor.** It still covers `/tmp/codex-plan-${REVIEW_ID}.md`, `/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-stdout-${REVIEW_ID}-failed-resume.jsonl`, `/tmp/codex-stderr-${REVIEW_ID}-failed-resume.txt`. ADD the two new paths introduced by the refactor: `/tmp/codex-body-${REVIEW_ID}.md` and `/tmp/codex-runner-result-${REVIEW_ID}.json`. -- Cleanup is **conditional on terminal state**: remove temp files on approved/max-reached/not-verified; LEAVE them on abort (diagnostic value). Skip all cleanup in Plan Mode. -- Always read-only sandbox — reviewer never writes files. -- Maximum 5 rounds to protect against infinite loops. +- **Step 9 cleanup `rm` glob covers ALL `/tmp/codex-*-${REVIEW_ID}*` files this skill writes** — initial-round body, resume-round body, prompts, review, stdout/stderr (current + archived failed-resume), runner result JSON, pre/post git-status snapshots, pre/post `sha256sum` snapshots. See the explicit list in Step 9. +- Cleanup is **conditional on terminal state**: remove temp files on approved/max-reached/not-verified/aborted-env; LEAVE them on abort due to launch failure / infra error (diagnostic value). Skip all cleanup in Plan Mode. +- **Default sandbox is `workspace-write`**, not `read-only`. The reviewer needs to run tests, build, query upstream docs, and exercise CLIs to verify findings — all write-class operations. Reviewer-side mutation is governed by (a) the `` prompt block (auditor, not contributor), and (b) the dual-layer mutation snapshot in Step 4 (`git status --porcelain` + `sha256sum` of `/tmp/codex-{body,plan,resume-body}-*`). Operators with sensitive ignored state in `REPO_ROOT` can opt out via `sandbox:read-only` (with the explicit trade-off that the reviewer loses empirical verification). +- **`review_quality` and `triage`** are part of the runner result schema and consumed by the operation-aware dispatch table in Step 4. Legacy runner results without these fields are treated as `review_quality=unknown` / `triage.status=skipped` and continue to work. +- **Workspace mutation snapshots are mandatory** before AND after every Codex dispatch (initial, resume, fresh-exec). Tracked-file mutation or `/tmp` review-input mutation is a hard stop before applying any fixes. +- **One-line runtime hint** about `workspace-write` and the `sandbox:read-only` opt-out is emitted exactly once per review (at Step 1, before Step 2), suppressed when the operator passed an explicit `sandbox:*` override. +- **Operator language** is detected at Step 1. Runtime prose (warnings, summaries, intermediate updates) uses `OPERATOR_LANGUAGE`; repository files stay in English; machine-readable literals (`[severity:]`, `VERDICT:`, section headers) stay in English regardless of language. +- **Reviewer findings are suggestions to evaluate, not orders to follow.** Step 6 builds an evaluation matrix with three first-class actions: `accept`, `reject with reasoning`, `re-scope`. The lead applies only accepted / re-scoped fixes; rejections go to Codex as structured counter-arguments in Step 7. +- **Structural fixes need operator sign-off** unless the operator explicitly requested autonomous mode or no operator is reachable. The batch-pause rule: exactly one operator prompt per round listing structural / non-structural / rejected. Headless runs apply structural fixes anyway but record "applied without operator sign-off" in the Step 8 operator summary. +- **Maximum 5 rounds** to protect against infinite loops. +- **Final operator summary** is emitted at every terminal state (approved, max rounds, not verified, aborted) in `OPERATOR_LANGUAGE`, AFTER the final verbatim reviewer response. Built from in-conversation per-round summaries — main never reads Codex stdout/stderr/rollout files. - Show the user reviews and fixes for each round. -- If Codex CLI is not installed or crashed — tell the user: `npm install -g @openai/codex`. -- If a fix contradicts the user's explicit requirements — skip and explain why. +- If Codex CLI is not installed or crashed — tell the user: `npm install -g @openai/codex` (requires Codex CLI ≥ 0.132.0 for the `-c approval_policy` form; earlier versions may need different override syntax). +- If a fix contradicts an explicit user requirement — skip it, record it in the Step 7 `## Rejected with reasoning` section, and surface in the Step 8 operator summary. diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 3ce39a4..974d0a9 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -829,6 +829,311 @@ Each decision below follows the same template: command (codex), which matches what the skill already checks; no `pipefail` needed. +### §4.14. Default model `gpt-5.5` (refresh 2026-05-20) + +- **Decision.** The default reviewer model is `gpt-5.5`. Earlier defaults + were `gpt-5.4` and `gpt-5.3-codex`. +- **Why.** Each Codex generation reduces false-positive rate on + adversarial review and improves citation accuracy. The default tracks + the latest stable Codex model that ChatGPT+API auth can both reach. +- **Rejected alternatives.** + - *Pin to a fixed model forever.* Causes the skill to silently drift + out of date. + - *Default to the latest available regardless of stability.* Mid-loop + behavior changes invalidate the round-by-round severity trajectory + that the loop relies on. +- **Trade-offs accepted.** The version-log table in §8 needs a new row + per default bump so users can verify the smoke protocol on the chosen + model. + +### §4.15. Default sandbox `workspace-write`, not `read-only` (refresh 2026-05-20) + +- **Decision.** Initial / fresh-exec dispatches default to + `-s workspace-write`. `read-only` is a deliberate operator opt-in via + `sandbox:read-only`, never an automatic per-mode default. +- **Why.** The reviewer's value proposition is empirical verification — + "the reviewer ran the tests and X failed", "the reviewer built the + project and Y broke", "the reviewer queried the live API and the + assumed signature does not exist." Each of those requires write-class + operations: tests write cache and stdout files, builds emit + artifacts, CLIs touch local state. Read-only blocks all of them. The + remaining read-only-compatible verification (file inspection, `rg`, + `--help` introspection, MCP doc lookups that don't cache) is a small + fraction of the high-value path. +- **Rejected alternatives.** + - *Default `read-only` for plan reviews.* The original 2026-05 + design tried this; reverted because plan reviewers also benefit + from running a test the plan relies on, building to confirm a + structural claim, or hitting an external API to validate an + assumption. + - *Sandbox-level enforcement of "no project file writes".* Codex + does not expose a "write to `/tmp` only, not `${REPO_ROOT}`" + sandbox tier — the closest is `read-only`, which blocks exec. + - *Auto-create a `git worktree` per review.* Adds setup latency to + every dispatch and complicates `RUNNER_SPEC_PATH` resolution and + `REPO_ROOT` capture (the worktree path is not the operator's + canonical repo root). Documented as an operator-side mitigation + in `README.md` instead. +- **Trade-offs accepted.** Reviewer-side writes to gitignored state + inside `REPO_ROOT` are real and not architecturally prevented; they + are mitigated through documentation (`README.md` "Safety + considerations"), the per-review runtime hint, and the + `sandbox:read-only` opt-out. Tracked-file mutation IS detected via + `git status --porcelain` snapshots — see §4.20. + +### §4.16. Approval policy `on-request` + `auto_review` reviewer (refresh 2026-05-20) + +- **Decision.** The default approval flow is + `-c approval_policy='"on-request"'` plus + `-c approvals_reviewer='"auto_review"'`. The top-level + `-a` / `--ask-for-approval` flag (removed in Codex CLI 0.132) is NOT + emitted by the runner. +- **Why.** + - `on-request` keeps boundary crossings explicit so the reviewer can + still ask before running a destructive operation, rather than + silently failing. + - `auto_review` for the approval reviewer avoids nested human + approval prompts inside the Codex subprocess. From the parent + Claude session, the operator cannot reliably see, answer, or even + detect those prompts — they appear as the run "hanging". The + `auto_review` policy auto-approves benign operations and rejects + obviously harmful ones, matching the boundary semantics the skill + needs. + - The config-based `-c approval_policy=...` form is the only form + supported on Codex CLI 0.132+; the runner emits it + unconditionally for forward compatibility with the unknown shape + of future flag churn. +- **Rejected alternatives.** + - *Pass `-a on-request` directly.* Removed in 0.132; would silently + fail on modern CLI versions. + - *Default to `approvals:user` (nested human prompt).* The nested + prompt is invisible from the parent Claude session and will hang + the run. + - *Default to `approvals:never` (boundary crossings fail + automatically).* Loses graceful degradation for routine operations + that just need a one-time approval. + - *Expose Codex's `untrusted` approval policy as an override.* The + skill needs predictable boundary semantics, not per-command trust + prompts. +- **Trade-offs accepted.** `auto_review` is not a security boundary — + it reviews approval requests, not actions already permitted by the + selected sandbox. The skill relies on the `` + prompt contract plus the workspace mutation snapshot for the real + guardrails. + +### §4.17. Sonnet runner triage as a hint, not a decision (refresh 2026-05-20) + +- **Decision.** The runner subagent (Sonnet) produces a compact + `triage` object — finding counts, max severity, per-severity + coverage counts, truncation flag, lead-judgment hint. This object is + returned in the result JSON and consumed by the lead as a hint. The + lead's evaluation matrix in Step 6 is built from the verbatim + review, NOT from triage. Triage is never passed back to Codex. +- **Why.** + - Codex returns a long-form review; main historically had to read + the entire review file to extract simple counts ("how many + critical findings did the reviewer raise?"). With triage, main + gets cheap structured signal without reading the file twice. + - The runner is the right layer to do triage because (a) it already + has the review file in context after Step R4 checks, and (b) the + runner's context is disposable — its 1M context can hold the + review and triage logic without polluting main. + - Keeping triage as a hint (not a decision) preserves the rule that + final `accept` / `reject` / `re-scope` outcomes are the lead's. + Triage that ranked findings or pre-emptively rejected them would + move the decision boundary into the cheaper model, which is + exactly the kind of authority drift past adversarial-review + rounds warned against. +- **Rejected alternatives.** + - *Let the runner pre-apply a heuristic accept/reject.* Drifts + final-decision authority into Sonnet. Rejected on principle. + - *Let the runner re-write the review file with a triage summary.* + Mutates the artifact main shows verbatim. Rejected to preserve + the "show review verbatim" contract. + - *Forward triage metadata to Codex on resume.* Codex would see + Sonnet's judgments and either echo them (false agreement) or + push back on them (wasted round on a triage artifact). Rejected. +- **Trade-offs accepted.** Triage may be wrong — wrong counts, + miscategorized severity, false `needs_lead_judgment` flag. The lead + re-derives counts during the evaluation matrix anyway; the hint + saves a small amount of cognitive load on round 1 but does not + load-bear on correctness. + +### §4.18. Operator-language detection from recent messages (refresh 2026-05-20) + +- **Decision.** At Step 1, main inspects the last few human-authored + conversation turns and captures `OPERATOR_LANGUAGE` (English by + default; non-English when detection is unambiguous). Runtime prose + uses `OPERATOR_LANGUAGE`. Repository files (this `DESIGN.md`, + `SKILL.md`, `README.md`, `references/runner.md`, specs under + `docs/superpowers/specs/`) stay in English regardless. The reviewer + is asked to produce findings in `OPERATOR_LANGUAGE` while keeping + machine-readable literals (severity tags, `VERDICT:`, section + headers) in English. +- **Why.** Operators routinely work in non-English; verbatim review + show-back, structural-gate prompts, and final summaries are more + useful in the operator's language. Repository files stay in English + because the contributor pool and AI-assistant training mass are + predominantly English, and mixed-language docs are harder to + maintain and grep. +- **Rejected alternatives.** + - *Ask the operator to declare language at invocation time.* Extra + friction; the conversation already encodes the signal. + - *Translate everything including section headers and severity + tags.* Breaks the parser regexes (`^VERDICT:`, `\[severity:\s*`, + `Summary` / `Findings` / `Verdict` section anchors). + - *Default to the operator's locale.* Locale != active language — + a Russian operator may be working through an English-language + codebase and want English review prose. +- **Trade-offs accepted.** Detection is heuristic and can be wrong on + short conversations or code-switching. Falling back to English on + ambiguity is safe; the operator can still read it, and the + machine-readable literals are stable. + +### §4.19. Lead evaluation matrix + structural operator gate (refresh 2026-05-20) + +- **Decision.** Step 6 builds an explicit evaluation matrix + (`finding | severity | verified? | type | action`) with three + first-class actions: `accept`, `reject with reasoning`, `re-scope`. + Step 6.3 applies a batch-pause rule: one operator prompt per round + listing structural / non-structural / rejected, only when the + structural count is ≥ 1 and an operator is reachable. Headless and + autonomous runs apply structural fixes anyway but record the fact + in the final operator summary. +- **Why.** + - Pre-refresh Step 6 used a flat "fix everything the reviewer + raised" loop. This is the failure mode the + `superpowers:receiving-code-review` skill exists to prevent — the + reviewer can be technically wrong, and applying its findings + blindly produces large structural edits based on misreadings. + The matrix forces an evaluation pass before any fix. + - `reject with reasoning` and `re-scope` are equal first-class + outcomes, not exceptions. The structured resume prompt's + "Rejected with reasoning" section gives the reviewer a chance to + contest, which keeps the loop honest. + - Structural-vs-non-structural gating exists because the highest- + leverage refactor a misread finding can trigger is "rewrite the + invocation grammar to match the reviewer's mental model." That's + the exact case where one operator pause prevents large + irrecoverable damage. +- **Rejected alternatives.** + - *Pause once per fix.* Confirmation fatigue; operator stops + reading after pause 3. + - *Apply all fixes automatically and let the operator review + post-hoc.* Reverting a misread structural fix is far more + expensive than one upfront prompt. + - *Let the runner classify structural vs non-structural and + pre-gate.* Returns to the §4.17 problem — moves final-decision + authority into the cheaper model. +- **Trade-offs accepted.** The matrix adds verbosity to the + conversation. The structural-gate prompt adds latency. Both are + small per round and saturate at low rounds because severity + declines (see §4.19 in spirit — Step 6.5 severity-decline soft + signal). + +### §4.20. Workspace mutation snapshots (dual-layer, mandatory) (refresh 2026-05-20) + +- **Decision.** Main captures `git status --porcelain` of `REPO_ROOT` + AND `sha256sum` of `/tmp/codex-{body,plan,resume-body}-*` BEFORE + and AFTER every Codex dispatch. Any tracked-file mutation, or any + `/tmp` review-input mutation, is a hard stop before applying fixes. + Gitignored mutation inside `REPO_ROOT` is NOT detected (deliberate + trade-off; see §4.15 and `README.md` "Safety considerations"). +- **Why.** The default `workspace-write` sandbox allows the reviewer + to run commands that can mutate the workspace. The + `` prompt contract is the primary control, + but a prompt-only safeguard has a non-zero failure rate + (reviewer-side bug, prompt drift, ambiguous toolchain command). The + snapshot provides a secondary, after-the-fact detection layer that + catches mutation regardless of the cause. +- **Rejected alternatives.** + - *Snapshot only `git status`.* Misses reviewer mutation of the + skill's own `/tmp` prompt body — a real attack surface where a + reviewer could rewrite the prompt to suppress future findings. + - *Snapshot the entire `REPO_ROOT` tree including gitignored + files.* Too expensive per round (`node_modules`, `target/`, + `.next/`) and produces confirmation fatigue on every benign + side-effect. + - *Run the snapshot once at start-of-review only.* Misses + mid-loop mutation; mid-loop is when the round counter has + already advanced and rollback is most expensive. + - *Content-aware tracked-file snapshot (`git diff HEAD` patch + + sha256sum) on every dispatch.* Considered during round-1 dogfood + review of this refresh, and reconsidered in round-2 after a + reviewer pushed back on the original rejection rationale (see + Round-2 correction below). Would close the already-dirty + tracked-file residual risk listed in "Trade-offs accepted" below, + at the cost of an extra patch-file write + hash per dispatch, four + additional permissions entries (`Bash(git -C * diff HEAD*)`, + `Write/Read(/tmp/codex-tracked-pre-*)`, + `Write/Read(/tmp/codex-tracked-post-*)`, + `Bash(diff -q /tmp/codex-tracked-*)`), two additional cleanup + glob paths, and a third parallel snapshot layer in Step 4. + Rejected on a complexity-vs-frequency trade-off: the vector is a + legitimately-invoked verification command with unintended write + side effects on an already-dirty tracked file, with the + `` prompt contract as the only skill-level + protection layer and `sandbox:read-only` as the operator-side + opt-out. The skill ships the residual gap honestly documented + rather than hidden behind a third snapshot layer; if the risk + materializes in practice the design notes the implementation path + above. + - *Round-1 rejection rationale that cited the + `approval_policy=on-request` + `auto_review` gate as a protection + layer for this vector.* Withdrawn in round-2. Workspace writes are + permitted by the `workspace-write` sandbox and do not generate an + approval request, so `auto_review` never evaluates them (consistent + with §4.16's own statement that auto-review "reviews approval + requests, not actions already permitted by the selected sandbox"). + The realistic vector reduces to a reviewer-invoked verification + command (formatter, snapshot updater, lockfile regen, codegen) + with unintended side effects on an already-dirty file. See + `README.md` "Residual risk: already-dirty tracked files" for the + operator-facing wording. +- **Trade-offs accepted.** Per-dispatch overhead is two `git` + commands and one `sha256sum` over three small files — negligible. + Gitignored mutation inside `REPO_ROOT` remains the documented + residual risk. **Already-dirty tracked-file content drift** is the + second documented residual risk: the porcelain snapshot tracks status + codes, not content hashes, so a reviewer that mutates a file already + marked ` M` will not trip a hard stop. The single skill-level + protection is the `` prompt contract; no + approval-policy gate exists for workspace-permitted writes. + Mitigated by (a) a runtime warning in `SKILL.md` Step 2 that fires + when pre-review `git status --porcelain` shows already-modified + tracked entries, (b) `README.md` "Safety considerations" explicitly + documenting the vector and the lack of an approval-policy gate, and + (c) the operator-side opt-out of `sandbox:read-only` (loses + reviewer-side verification, gains write-protection). + +### §4.21. Final operator summary at every terminal state (refresh 2026-05-20) + +- **Decision.** Step 8 emits an operator-facing summary in + `OPERATOR_LANGUAGE` at every terminal state — approved, max rounds + reached, not verified, aborted. The summary follows the canonical + per-state header block AND the final verbatim reviewer response; + it does not replace either. Built from per-round decision accounts + already shown in conversation; never from `/tmp` codex logs. +- **Why.** Pre-refresh Step 8 ended with the verbatim review and a + one-line status. For multi-round reviews this is insufficient — + the operator has scrolled past round 1 and 2 by the time they see + the final state, and "approved after 4 rounds" hides which + rejections and re-scopes survived to the end. The summary + consolidates the decision audit trail at the place where the + operator decides whether to merge. +- **Rejected alternatives.** + - *Show full diffs in the summary.* Too long; operator can `git + diff` for that. + - *Re-read Codex stdout/stderr to assemble the summary.* Violates + the architectural rule that main does not consume codex + transport logs (§12.x of subagent architecture). + - *Skip the summary on `Approved`.* The `Approved` summary is + actually the most useful for the merge-decision audience. +- **Trade-offs accepted.** Summary quality depends on context + compaction. The skill states this limitation explicitly when + history is incomplete rather than fabricating round-by-round + details. + --- ## §5. Rejected ideas @@ -1127,8 +1432,11 @@ End the LAST line with exactly: VERDICT: APPROVED EOF cat /tmp/codex-prompt-${REVIEW_ID}.md | timeout 300 codex exec --json \ - -m gpt-5.4 -c model_reasoning_effort=low \ - -s read-only -C "${REPO_ROOT}" \ + -m gpt-5.5 -c model_reasoning_effort=low \ + -s workspace-write \ + -c approval_policy='"on-request"' \ + -c approvals_reviewer='"auto_review"' \ + -C "${REPO_ROOT}" \ -o /tmp/codex-review-${REVIEW_ID}.md \ - \ > /tmp/codex-stdout-${REVIEW_ID}.jsonl \ @@ -1223,11 +1531,17 @@ mkdir -p /tmp/smoke-cwd-a /tmp/smoke-cwd-b ( 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}') + codex exec -s workspace-write -m gpt-5.5 \ + -c model_reasoning_effort=low \ + -c approval_policy='"on-request"' \ + -c approvals_reviewer='"auto_review"' \ + - 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}') + codex exec -s workspace-write -m gpt-5.5 \ + -c model_reasoning_effort=low \ + -c approval_policy='"on-request"' \ + -c approvals_reviewer='"auto_review"' \ + - 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 @@ -1264,6 +1578,125 @@ If §7.1–§7.5 do not produce the expected outputs: before modifying `SKILL.md`. Future contributors should know which facts they can still trust. +### §7.8. Refresh-era checks (added 2026-05-20) + +These supplement §7.1–§7.6 and verify the behavior introduced by the +2026-05-20 refresh. Run them on the installed Codex CLI version and +record results in `§8`. + +**Flag emission.** + +- [ ] `codex exec --help | rg 'approval_policy|approvals_reviewer'` — confirm the + installed Codex CLI accepts the `-c` form. +- [ ] On a defaulted invocation, the runner subagent's launch command + contains `-s workspace-write`, `-c approval_policy='"on-request"'`, + and `-c approvals_reviewer='"auto_review"'`. Verify by reading the + runner's R3 step (the runner does NOT emit the launch command to + main, so this is checked by reading the runner spec, not by + inspecting main's logs). +- [ ] `codex exec --help` does NOT list `-a` / `--ask-for-approval` on + 0.132+. Expected; the runner does not emit `-a` regardless. +- [ ] `codex exec resume --help` does NOT list `-s`, `-m`, or + approval-related `-c` overrides. The runner must not pass them + on resume. + +**Sandbox preflight.** + +- [ ] On a host where `bwrap --dev-bind / / --unshare-net /bin/echo ok` + fails, dispatch initial review with default sandbox. Expected: + runner returns `success + degraded_environmental` with a + `user_warning` pointing at README "Linux sandbox prerequisites"; + main treats as terminal infrastructure failure; no fake review + round is shown. +- [ ] Apply the AppArmor profile per `README.md`. Re-run. Expected: + preflight passes; review proceeds normally. +- [ ] Pass `sandbox:read-only`. Expected: preflight still runs + (read-only is also bwrap-backed); review proceeds; the runtime + hint is suppressed. +- [ ] Pass `sandbox:inherit`. Expected: preflight is skipped; review + runs with whatever sandbox the user's Codex config selects. + +**Review-quality classification.** + +- [ ] A clean review with concrete findings → `review_quality=valid`, + `triage.status=ok`, counts populated. +- [ ] Synthesize a review file whose only content is "bwrap: setting up + uid map: Permission denied" (no findings, no real verdict) and + have the runner classify it. Expected: + `review_quality=degraded_environmental`, + `triage.status=skipped`. +- [ ] Synthesize a review with `VERDICT: REVISE` plus severity tags but + no concrete finding bodies (<80 chars each). Expected: + `review_quality=degraded_content`, `triage.status=ok`. +- [ ] Stop the triage-step Bash tool mid-run (or remove `rg` from + PATH). Expected: `triage.status=failed`, + `review_quality=valid` (the review itself is fine). + +**Operation-aware dispatch.** + +- [ ] Inject `success + degraded_environmental` on `OPERATION=initial`. + Expected: main emits `user_warning`, does NOT show review + verbatim, aborts with the "ABORTED — environmental failure" + Step 8 template. +- [ ] Inject `success + degraded_environmental` on `OPERATION=resume`. + Expected: main emits `user_warning`, does NOT show review + verbatim, does NOT consume a round, routes to Step 7.4 fallback + with prior round's severity. +- [ ] Inject `success + degraded_environmental` on + `OPERATION=fresh-exec`. Expected: main treats as terminal + not-verified. +- [ ] Inject `success + degraded_content` on any operation. Expected: + main emits `user_warning`, shows Step 5 verbatim, prompts the + operator (interactive) or auto-advances after a short wait + (headless). + +**Workspace mutation snapshots.** + +- [ ] Simulate the reviewer modifying a tracked file (touch a file in + `REPO_ROOT` during a synthetic dispatch). Expected: post-snapshot + `diff` is non-empty; main hard-stops before fixes with the + tracked-file diagnostic. +- [ ] Simulate the reviewer modifying `/tmp/codex-body-.md` + mid-dispatch. Expected: `sha256sum` diff is non-empty; main hard- + stops with the `/tmp` input diagnostic. +- [ ] Clean dispatch with no mutation. Expected: both diffs empty; + Step 5 proceeds. + +**Backward compat for legacy runner results.** + +- [ ] Manually write a runner result JSON missing `review_quality` and + `triage`. Expected: main treats as `review_quality=unknown`, + `triage.status=skipped`; no abort, no operator prompt. + +**Runtime hint.** + +- [ ] Default invocation: the hint appears exactly once, before Step 2. +- [ ] Default invocation, round 2/3/4/5: hint does NOT repeat. +- [ ] Invocation with `sandbox:read-only`: hint is suppressed. +- [ ] Invocation with `sandbox:workspace-write` (explicit, same as + default): hint is suppressed. + +**Operator language.** + +- [ ] Recent operator messages in Russian → `OPERATOR_LANGUAGE = Russian`. + Reviewer output is in Russian; severity tags, `VERDICT:`, and + section headers stay in English. Final operator summary is in + Russian. +- [ ] Recent operator messages mixed or short → `OPERATOR_LANGUAGE` + falls back to English; no language block is appended to the + prompt. + +**Final operator summary.** + +- [ ] Approved terminal: summary appears in `OPERATOR_LANGUAGE` after + the verbatim approved review. +- [ ] Max rounds: summary appears with remaining findings. +- [ ] Not verified: summary appears with the + `WARNING: This is NOT an approval` framing. +- [ ] Aborted environmental: summary appears with the diagnostic. +- [ ] Compacted history: summary states "round N details unavailable + due to compaction" instead of fabricating. + --- ## §8. Version and verification log @@ -1274,6 +1707,8 @@ If §7.1–§7.5 do not produce the expected outputs: | 2026-04-17 | 0.121.0 | containerized sandbox (yantar-k8s) | external agent + lead | §7.1 `- < file` form fails EXIT=1 with empty stderr. `cat \| pipe` form works for `-o` review, but `--json` stdout is empty. Filesystem secondary session-id capture (§4.1b) verified functional: UUID extracted from rollout filename successfully resumes. Not a version issue (reproduced on 0.120.0 and 0.121.0). Root cause undiagnosed — see §6.6. Skill adapted: `§4.1` now two-tier, `§4.13` switches canonical form to `cat \| pipe`. | | 2026-04-17 | 0.121.0 | reference env (WSL2) | live dogfood + team review | Round 6: timestamp-only secondary (§4.1b as of round 5) flagged for silent wrong-session hazard against parallel codex. Verified empirically that rollout JSONL contains prompt text (3 matches of prompt content via grep). Replaced with positive content-binding: prompt marker `` + `find -newer -exec grep -l {} +`. All flags POSIX — GNU-find dependency of earlier §9.5 goes away. See §6.7. | | 2026-04-17 | 0.121.0 | reference env (WSL2) | live dogfood round 2 | Round 7: review-stable marker flagged as insufficient — SKILL.md's own launch-retry flow can leave multiple rollouts matching the same `${REVIEW_ID}`, and "pick any" reintroduces silent intra-review session drift. Fixed by adding per-launch `${ATTEMPT_ID}` (6-digit random regenerated for every exec/retry/resume/fresh-exec). Marker is now `${REVIEW_ID}-${ATTEMPT_ID}`. Multi-match changed from "pick any" to fail-closed. See §6.8. | +| 2026-05-20 | TBD (refresh-era ≥ 0.132.0 required for `-c approval_policy`) | reference env (WSL2) | refresh design + implementation | Default model bump `gpt-5.4 → gpt-5.5`; default sandbox `read-only → workspace-write`; approval policy expressed via `-c approval_policy='"on-request"'` + `-c approvals_reviewer='"auto_review"'` (`-a` flag dropped per Codex 0.132 removal); new overrides `sandbox:*` and `approvals:*`. Runner result schema extended with `review_quality` + `triage` object (back-compat: legacy results treated as `unknown` / skipped). New operation-aware dispatch table in §SKILL.md Step 4. Mandatory pre/post `git status --porcelain` + `sha256sum` snapshots on every dispatch; tracked-file or `/tmp` review-input mutation is a hard stop. Step 6 rewritten with evaluation matrix + structural operator gate (batch-pause rule). Step 7 resume body restructured into Applied / Re-scoped / Rejected with reasoning / Specific asks. Step 8 adds final operator summary in `OPERATOR_LANGUAGE` at every terminal state. New rationale entries §4.14–§4.21. README adds "Safety considerations" and "Linux sandbox prerequisites" sections. Smoke protocol §7 updated for the new flags. Verification: dogfood plan + code review against this refresh (see §7.8 once added). | +| 2026-05-21 | 0.132.0 | reference env (WSL2) | dogfood rounds 1 & 2 (`/adversarial-review code` against this branch) | Round 1: applied #2 (worktree recipe loses unstaged), #3 (stale Full example permissions), #4 (bwrap diagnostic recommends `sandbox:read-only`); re-scoped #1 (porcelain misses content drift on already-dirty tracked files) to docs + runtime warning. Round 2 caught two structural issues: (a) the round-1 warning was placed in `SKILL.md` Step 1 before `REPO_ROOT` capture, making it non-functional — moved to Step 2 after capture; (b) the round-1 re-scope rationale cited `approval_policy=on-request` + `auto_review` as a protection layer for the residual gap, which is incorrect because workspace writes are sandbox-permitted and never generate approval requests — rationale withdrawn in §4.20, README "Safety considerations" rewritten to document the gap honestly. Already-dirty tracked-file content drift remains an accepted residual risk; mitigations are the `` prompt contract, the runtime warning, and the `sandbox:read-only` operator opt-out. | When you re-verify (either during routine maintenance or when triggered by §7.7), add a row. Keep the log chronological. @@ -1484,7 +1919,7 @@ The attempt-scoped `ADVERSARIAL-REVIEW-SESSION` marker (round-7 finding) and pos **Hypothesis to test:** Plan Mode restrictions propagate from main to any subagent main dispatches; the subagent inherits limitations on Write/Edit/Bash. If this holds: - Main's Write to `/tmp/codex-body-*.md` may trigger a permission prompt or exit Plan Mode. - The runner's Writes to `/tmp/codex-prompt-*.md` (Step R2, including the mtime-bump repeat Write) may also trigger prompts. -- The runner's `codex exec` (read-only sandbox) should be unaffected since it writes nothing to the user's repo. +- The runner's `codex exec` (default `workspace-write` sandbox, governed by the `` prompt contract per §4.15) writes to `/tmp` for review artifacts but is bound by main's mutation snapshot (§4.20) against unintended `REPO_ROOT` writes. **What Task 7 Step 4 must determine:** 1. Does dispatching the Agent tool from Plan Mode work (is it blocked, does it prompt, does it just work)? diff --git a/docs/superpowers/specs/2026-05-20-adversarial-review-refresh-design.md b/docs/superpowers/specs/2026-05-20-adversarial-review-refresh-design.md new file mode 100644 index 0000000..10c2b10 --- /dev/null +++ b/docs/superpowers/specs/2026-05-20-adversarial-review-refresh-design.md @@ -0,0 +1,718 @@ +# Adversarial Review Refresh Design + +## Overview + +This design updates the `adversarial-review` skill while preserving its core +shape: Claude remains the lead, Codex provides an external cross-model +adversarial review, and the Sonnet runner isolates mechanical Codex CLI work +from the expensive lead context. + +The refresh makes the skill more useful to the operator, more robust on modern +Codex CLI setups, and less prone to blindly applying reviewer feedback. It does +not turn the skill into a larger framework, add mandatory custom agent +definitions, or move final decisions from the lead into the runner. + +## Goals + +- Use `gpt-5.5` as the default Codex reviewer model while preserving explicit + model and reasoning overrides. +- Let Codex reviews use the operator's language while keeping parsed literals + stable. +- Improve reviewer capability by avoiding overly restrictive default sandboxing. +- Avoid nested human approval prompts that the operator cannot see from the + Claude session. +- Keep runner-side work bounded and disposable so the lead context stays clean. +- Add compact Sonnet triage to reduce repeated review rounds without letting + Sonnet make final decisions. +- Require the lead to evaluate findings before applying fixes. +- Support operator sign-off for structural fixes without blocking explicitly + autonomous runs. +- Provide a final operator-facing summary of what changed across the review. +- Document Linux sandbox prerequisites clearly enough for a human or installer + agent to act on them. + +## Non-Goals + +- Do not require a custom Claude agent type or extra installation artifact. +- Do not use `danger-full-access` as an automatic fallback. +- Do not make the runner apply fixes, start extra review rounds, or decide + final `accept` / `reject` / `re-scope` outcomes. +- Do not move Codex stdout, stderr, or rollout contents into the main Claude + context. +- Do not add a mandatory detailed implementation plan for small instructional + changes. +- Do not mix documentation languages in repository files. + +## Default Reviewer Configuration + +The default Codex invocation should be optimized for non-interactive review +inside a Claude-runner child process: + +```text +CODEX_MODEL = gpt-5.5 +CODEX_REASONING = high +CODEX_SANDBOX = workspace-write +CODEX_APPROVAL_POLICY = on-request +CODEX_APPROVALS_REVIEWER = auto_review +``` + +`workspace-write` is the default for all review modes (plan, code, +code-vs-plan). The reasoning is load-bearing for this skill, so it is +captured here rather than in a comment. + +The reviewer needs to actually run things to verify findings: run +tests, build the project, query the web or upstream APIs to confirm +current behavior, and exercise project CLIs end-to-end. These are +write-class operations — test runners produce output and cache files, +build commands write artifacts, and most non-trivial CLI execution +touches local state. A read-only sandbox blocks all of that. + +Read-only is not a *total* verification blocker. It still allows file +inspection, `rg` / `grep` searches, MCP-backed doc lookups, and +`--help`-style CLI introspection that does not write to the workspace. +But the highest-value findings come from the path read-only blocks: +"the reviewer ran the tests and X failed", "the reviewer built the +project and Y broke", "the reviewer queried the live API and the +assumed signature does not exist." Forfeiting those to gain protection +against `.gitignored`-file side effects is a poor trade. + +This applies to plan reviews as much as to code. A plan reviewer +should be able to run a test the plan relies on, build the project to +confirm a structural claim, or hit an external API to validate an +assumption — not just `rg` for cited filenames. A previous design +pass made `read-only` the default for plan mode and was reverted for +this reason. + +The safety argument for forcing read-only is also weaker than it +appears. The broader `obra:superpowers` skill family demonstrates that +careful prompt-level discipline is sufficient to govern complex agent +behavior in security-relevant contexts without sandbox-level +restrictions. A concrete instance is `superpowers:receiving-code-review`: +it constrains how a lead processes adversarial feedback — read every +finding before reacting, restate the technical claim in own words, +verify empirically (especially tool-mechanic claims) before accepting, +push back with technical reasoning when wrong, no performative +agreement — and that discipline is enforced **entirely through prompt +instructions**, not through any tool-level sandboxing of the lead. +The same pattern applies to the reviewer side of this skill: a strict +instruction contract (see §Reviewer Behavior), backed by pre/post +`git status --porcelain` and the `/tmp` sha256 snapshot for +after-the-fact detection. + +Read-only's specific failure modes are covered without a default +sandbox change: + +- Tracked-file mutation is caught by `git status --porcelain` in + §Workspace Mutation Detection. +- `/tmp` review-input mutation is caught by the `sha256sum` snapshot + in the same section. +- Mutation of `.gitignored` state inside `REPO_ROOT` is bounded + (re-seedable dev state; tests read `.env.local`, they do not write + it) and is mitigated through documentation plus the + `sandbox:read-only` override for operators who knowingly accept the + trade-off. + +Operators with a concrete reason to opt into read-only (sensitive +local state, untrusted reviewer prompt source, or any other case where +sandbox-level write protection outweighs verification capability) can +do so per-run via `sandbox:read-only`. The override stays available +exactly because the trade-off is real — for those operators. +Defaulting the rest of the user base to that mode would gut the +skill's main value. + +The intended initial `codex exec` shape: + +```bash +codex exec --json \ + -m gpt-5.5 \ + -c model_reasoning_effort=high \ + -s workspace-write \ + -c approval_policy='"on-request"' \ + -c approvals_reviewer='"auto_review"' \ + -C "" \ + -o /tmp/codex-review-.md \ + - +``` + +Codex CLI 0.132+ removed the top-level `--ask-for-approval` / `-a` flag +from `codex exec`; approval policy is now expressed only through +`-c approval_policy=`. The spec uses `-c` form for both +`approval_policy` and `approvals_reviewer` so the command shape stays +self-consistent and future-proof against further short-flag churn. The +implementation should verify the supported approval-control surface +against the actual installed Codex CLI version during smoke testing. + +`-o` writes only the assistant's final message. The reviewer prompt must +deliver the full structured review (findings, severity tags, VERDICT) +inside that final message; intermediate tool calls and reasoning will +not appear in the review file. + +`workspace-write` gives the reviewer enough capability to run local +checks without forcing every useful command through a read-only wall. +`approval_policy="on-request"` keeps boundary crossings explicit. +`approvals_reviewer="auto_review"` avoids human approval prompts inside +the nested Codex process, which the operator cannot reliably see or +answer from the parent Claude session. + +Auto-review is not a security boundary. It reviews approval requests; it does +not inspect actions already permitted by the selected sandbox. The skill still +needs strong reviewer instructions and workspace mutation checks. + +Existing overrides remain: + +- `model:` +- `low`, `medium`, `high`, `xhigh` + +New minimal overrides: + +- `sandbox:read-only | workspace-write | danger-full-access | inherit` +- `approvals:user | auto_review | never` + +Rules: + +- Default `-s` is `workspace-write` for every review mode. Read-only is a + deliberate operator opt-in via `sandbox:read-only`, never an automatic + per-mode default. +- `sandbox:inherit` omits `-s` and relies on the user's Codex config. + Because the effective sandbox is unknown until Codex actually + launches, the runner skips bwrap preflight under `inherit`. If the + inherited config selects a bwrap-backed mode on a host where bwrap + is misconfigured, the failure surfaces as a §Runner Result Schema + `degraded_environmental` result and is treated as terminal + infrastructure failure on the initial dispatch per the dispatch table. +- `approvals:auto_review` (the default) passes + `-c approval_policy='"on-request"'` plus + `-c approvals_reviewer='"auto_review"'`. +- `approvals:user` passes `-c approval_policy='"on-request"'` only, + omitting the `approvals_reviewer` override so Codex falls back to its + default `user` reviewer. Allowed only by explicit override because + nested human approvals can hang the run. +- `approvals:never` passes `-c approval_policy='"never"'`; boundary + crossings fail instead of asking. +- `sandbox:danger-full-access` is explicit-only and should surface a warning. +- Codex's `untrusted` approval policy is intentionally not exposed as an + override; the skill needs predictable boundary semantics, not per-command + trust prompts. +- No silent fallback may change sandbox or approval semantics. + +Resume commands should respect Codex CLI support for `resume`: sandbox and +approval mode are properties of the initial session unless the current CLI +explicitly supports changing them on resume. Do not pass unsupported `-s` +flags or approval-related `-c` overrides to `codex exec resume`. + +## Runner Responsibilities + +The runner remains a Sonnet subagent responsible for Codex CLI mechanics: + +- parse the input block; +- write the prompt with the attempt-scoped session marker; +- launch exactly one Codex operation (`initial`, `resume`, or `fresh-exec`); +- own the one internal retry budget; +- validate exit code, stderr, review file, and session id; +- archive failed-resume diagnostics; +- write the authoritative result JSON; +- return the `RUNNER_RESULT_AT: ` line. + +The runner gains bounded analysis: + +- run sandbox preflight when the selected mode requires a bwrap-backed sandbox; +- detect obvious degraded reviews, such as sandbox or environment failures + disguised as reviewer output; +- extract finding count, maximum severity, and review quality; +- produce compact triage metadata. + +Triage rules: + +- Cover all critical and high findings. +- Cover up to 10 medium findings. +- If more medium findings remain, summarize the remainder and set an explicit + truncation flag. +- Use only cheap checks: file existence, section existence, simple `rg`, and + small read-only snippets. +- Do not run long test suites, web searches, or broad documentation lookups + during triage. +- Do not produce final `accept`, `reject`, or `re-scope` decisions. +- If uncertain, mark that lead judgment is needed. + +If triage fails but the Codex review is valid, the review should continue with +a warning. If the review itself is degraded by infrastructure failure, the +runner should not count it as a normal round. + +Runner instructions must include negative examples: + +- Do not edit project files. +- Do not apply fixes. +- Do not run multiple Codex review rounds in one dispatch. +- Do not delete `/tmp/codex-*` files. +- Do not decide which findings the lead must accept. +- If a command unexpectedly changes project files, stop and report it. + +## Runner Result Schema + +The runner writes a single JSON file at `RESULT_PATH`. The schema below is +the authoritative contract between runner and lead. New fields added by +this refresh are marked `(new)`; everything else preserves the pre-refresh +shape. + +```json +{ + "result": "success | timeout | launch_failure | infra_error | input_error", + "verdict": "APPROVED | REVISE | null", + "review_file": "", + "codex_session_id": "", + "attempt_id": "", + "errors": "", + "archived_stdout": "", + "archived_stderr": "", + "user_warning": "", + "review_quality": "valid | degraded_environmental | degraded_content | unknown", // (new) + "triage": { // (new) + "status": "ok | skipped | failed", + "finding_count": "", + "max_severity": "critical | high | medium | none", + "covered_critical": "", + "covered_high": "", + "covered_medium": "", + "truncated": "", + "needs_lead_judgment": "" + } +} +``` + +Field semantics: + +- `review_quality=valid` — review file passes R4 checks and content matches + a normal review shape. +- `review_quality=degraded_environmental` — Codex returned an exit-0 + pseudo-review caused by sandbox or environment failure (bwrap-EPERM, + trust prompt, rate-limit stub, etc.). The text may contain `VERDICT:` + and severity tags but describes an inability to perform the review. +- `review_quality=degraded_content` — review parses cleanly but the + runner's cheap heuristics suggest the body is not actionable (e.g. + only a sandbox self-report, no concrete findings backing severity + tags). Conservative catch — when in doubt, mark `valid` and let the + lead decide. +- `review_quality=unknown` — triage could not classify (e.g. triage step + itself crashed). The lead should treat this as `valid` plus a warning. +- `triage.status=ok` — triage ran, fields populated. +- `triage.status=skipped` — triage skipped because Codex itself failed + (no review to triage). +- `triage.status=failed` — triage crashed; counts and severity may be + missing. Review remains usable if `review_quality=valid`. + +Lead-side dispatch is **operation-aware** because `degraded_environmental` +on the first dispatch has no prior valid round to fall back on. The full +table: + +| `result` | `OPERATION` | `review_quality` | Lead action | +|------------------|---------------|--------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `success` | any | `valid` | Proceed to Step 5: show review verbatim, run evaluation matrix, advance round. | +| `success` | any | `degraded_content` | Emit `user_warning`, show review verbatim, ask operator whether to advance the round. | +| `success` | any | `unknown` | Emit `user_warning`, treat as `valid` for advancement, note in final operator summary. | +| `success` | `initial` | `degraded_environmental` | Emit `user_warning`, do NOT show review verbatim, treat as terminal infrastructure failure. No prior valid review exists to recover from. | +| `success` | `resume` | `degraded_environmental` | Emit `user_warning`, do NOT show review verbatim, do NOT count as a round, route to fallback chain using prior round's severity. | +| `success` | `fresh-exec` | `degraded_environmental` | Emit `user_warning`, do NOT show review verbatim, terminal not-verified. Fresh-exec already was the fallback; a second environmental failure means the env is reliably broken. | +| `timeout` | any | n/a | Terminal at main per existing rules. | +| `launch_failure` | any | n/a | Terminal at main per existing rules (`resume` `launch_failure` routes to fresh-exec fallback per existing §7.4). | +| `infra_error` | any | n/a | Show `errors`, abort. | +| `input_error` | any | n/a | Show `errors`, abort (orchestration bug). | + +Backward compatibility: a runner that omits `review_quality` or `triage` +fields entirely is treated as `review_quality=unknown` with +`triage.status=skipped`. Existing field semantics (`result`, `verdict`, +`review_file`, `codex_session_id`, `user_warning`, etc.) are unchanged. + +## Reviewer Behavior + +The Codex reviewer is an auditor, not a contributor. The prompt should allow +useful verification while prohibiting project mutation: + +```text +You may run commands to verify findings when useful. +Do not create, edit, delete, commit, or apply fixes to project files. +Prefer commands that do not mutate the working tree. +Do not run commands likely to rewrite generated files, snapshots, migrations, +lockfiles, or configs. +If verification would require mutation, report that limitation instead. +If a command unexpectedly changes files, stop and report it. +``` + +This is a deliberate compromise. A reviewer locked into a strict read-only +sandbox often cannot run the checks needed to validate tool behavior, tests, or +documentation-dependent claims. The primary safeguard is the instruction +contract plus post-dispatch mutation detection, not a hard read-only shell. + +## Lead Responsibilities + +The lead owns all product decisions. After each valid Codex review: + +1. Show the Codex review to the operator verbatim before applying fixes. +2. Use runner triage only as a hint. +3. Build a compact evaluation matrix: + + ```text + finding | severity | evidence | action | reason + ``` + +4. Choose one of three first-class actions for each finding: + `accept`, `re-scope`, or `reject with reasoning`. +5. Apply only accepted or re-scoped fixes. +6. Send the reviewer a structured re-review prompt with `Applied`, + `Re-scoped`, and `Rejected with reasoning` sections. + +Reviewer findings are suggestions to evaluate, not orders to follow. The lead +should verify findings proportionally to risk. Critical and high findings need +more scrutiny; medium findings can often be accepted, narrowed, or rejected +based on local checks and reasoning. + +For plan reviews, the lead and reviewer must judge the plan at its declared +level of abstraction. Missing implementation details are findings only when +their absence blocks feasibility, safety, rollback, verification, or a public +contract. + +## Operator Language + +The skill should detect the operator language from recent user messages and +ask Codex to respond in that language. If detection is unclear, use English. + +Prompt block: + +```text + +Respond in the operator's language: . +Keep these machine-readable literals unchanged in English: +- [severity: critical|high|medium] +- VERDICT: APPROVED +- VERDICT: REVISE + +``` + +Runtime prose shown to the operator should use the operator's language when +practical. Repository documentation and skill files remain English. + +## Prompt Changes + +Plan review prompts should add abstraction-level calibration: + +```text +Judge the plan at its declared level of abstraction. +Do not demand implementation details unless their absence blocks feasibility, +safety, rollback, verification, or a public contract. +If a detail can reasonably be decided during implementation, do not count it +as a finding. +``` + +Re-review prompts should replace the current "I've revised based on your +feedback" shape with: + +```text +I've evaluated the findings. + +## Applied +- ... + +## Re-scoped +- ... + +## Rejected with reasoning +- ... + +## Specific asks for re-review +1. Are my rejections technically valid? +2. Did the applied/re-scoped fixes resolve the original findings? +3. Did the fixes introduce new issues? +``` + +Sonnet triage metadata should not be passed to Codex automatically. Codex sees +verbatim findings and the lead's decisions, not the runner's notes. + +## Structural Operator Gate + +Before applying structural fixes, the lead should pause once and ask the +operator to approve the batch unless the operator explicitly requested +autonomous work. + +Structural changes include: + +- invocation grammar or argument semantics; +- output format or parsed literals; +- workflow steps, fallback semantics, or terminal states; +- sandbox, approval, or security guarantees; +- public configuration semantics; +- schema, migration, or data format changes; +- broad architectural rewrites; +- any fix whose scope the lead is uncertain about. + +Non-structural fixes include wording, factual clarifications, examples, and +local changes that do not alter external behavior. + +If the operator explicitly requested autonomous mode, the lead may apply +structural fixes without pausing, but the final summary must state that +structural changes were applied without operator sign-off due to autonomous +mode. + +## Workspace Mutation Detection + +Mutation detection runs at two layers: the repo tree (git-tracked + new +untracked files) and the skill's own `/tmp` review inputs. A third class +of mutation — gitignored files already inside `REPO_ROOT` — is documented +as a known, operator-mitigated risk rather than detected automatically. + +**Repo tree.** The main thread should snapshot workspace state with +`git status --porcelain` before and after each runner dispatch, before +Step 5 applies any fixes. + +If tracked files changed during the runner dispatch, the skill must hard +stop before applying fixes and show an operator diagnostic. This catches +reviewer or runner mutation without requiring a custom agent type. + +If only untracked generated artifacts appeared, the skill should warn and +gate continuation. Some tools leave local artifacts, but the skill must +not silently fold them into fixes. + +**Review inputs in `/tmp`.** The materialized plan file +(`/tmp/codex-plan-.md` when present), the prompt body +(`/tmp/codex-body-.md`), and the resume body +(`/tmp/codex-resume-body-.md`) sit outside `REPO_ROOT` and are +not covered by `git status`. The main thread must hash these files before +dispatch and re-hash after the runner returns: + +```bash +sha256sum /tmp/codex-{body,plan,resume-body}-.md 2>/dev/null \ + > /tmp/codex-inputs-pre-.sha +# ... runner dispatch ... +sha256sum /tmp/codex-{body,plan,resume-body}-.md 2>/dev/null \ + > /tmp/codex-inputs-post-.sha +diff -q /tmp/codex-inputs-pre-.sha \ + /tmp/codex-inputs-post-.sha +``` + +A non-empty diff is treated identically to a tracked-file mutation: hard +stop before applying fixes and surface the operator diagnostic. + +**Out-of-scope: gitignored files inside `REPO_ROOT`.** `.gitignored` files +that already exist inside `REPO_ROOT` (local SQLite DBs, `.env.local`, +service-state directories, build caches) are NOT detected by either +layer. Full-tree snapshotting would be too expensive to run every round, +and a pre-dispatch confirmation about "ignored files present" would +trigger on essentially every repo (`node_modules`, `target/`, `.next/`) +and produce confirmation fatigue — security theater rather than +protection. + +The realistic mutation vector for these files is the reviewer running a +project test or build command that side-effects on the ignored file — +for example `pytest` triggering an unintended migration on `dev.sqlite` +because the test settings point at it. The damage is bounded (the state +is re-seedable; tests generally read `.env.local`, they do not write to +it), but it is real on default workspace-write reviews. + +The skill mitigates this with three operator-facing affordances rather +than architectural force: + +1. The `sandbox:read-only` override is always available — operators who + know they have sensitive ignored state can opt out of test execution + at the cost of empirical verification (the reviewer loses the + ability to run tests, linters, and most MCP-backed verification). +2. The README must include a "Safety considerations" section with the + concrete pytest → `dev.sqlite` vector, the opt-out guidance (with + its explicit verification trade-off), and the worktree-isolation + pattern (`git worktree add /tmp/review-worktree `) for sensitive + repos. +3. At the start of every review, the skill emits a one-line runtime + hint: `workspace-write in effect; pass sandbox:read-only if + sensitive ignored state lives under REPO_ROOT`. The hint appears + exactly once per review (not per round) and is suppressed when the + operator passed an explicit `sandbox:*` override. + +This is a deliberate trade-off. Defaulting reviews to `read-only` or to +an isolated worktree would gut the reviewer's empirical verification +capability (tests, linters, builds, MCP doc lookups, web searches all +require execute) — and empirical verification is precisely what makes +adversarial review more valuable than a same-model self-check. The +mitigation level is documentation and operator awareness rather than +architectural force. + +## Linux Sandbox Prerequisites + +The README should document sandbox prerequisites for Linux users and installer +agents. With the default `workspace-write` mode, Codex may rely on bubblewrap +and unprivileged user namespaces. + +Diagnostic probe: + +```bash +bwrap --dev-bind / / --unshare-net /bin/echo ok +``` + +If the probe fails on Ubuntu 24.04+ due to AppArmor user namespace +restrictions, recommend the official bwrap AppArmor profile: + +```bash +sudo apt install -y apparmor-profiles apparmor-utils +sudo install -m 0644 /usr/share/apparmor/extra-profiles/bwrap-userns-restrict \ + /etc/apparmor.d/bwrap-userns-restrict +sudo apparmor_parser -r /etc/apparmor.d/bwrap-userns-restrict +``` + +Then verify: + +```bash +bwrap --dev-bind / / --unshare-net /bin/echo ok +``` + +The README should link to the official OpenAI Codex sandboxing documentation: +`https://developers.openai.com/codex/concepts/sandboxing`. + +Installer agents should not change AppArmor policy silently. They should probe, +show the exact commands, request explicit permission, apply the profile only +after approval, and re-run the probe. + +## Final Operator Summary + +At every terminal state, the skill must provide an operator-facing summary in +the operator's language. This summary comes after the final verbatim reviewer +response and does not replace it. + +Include: + +- final status: approved, maximum rounds reached, not verified, or aborted; +- what changed across all review rounds; +- findings applied, re-scoped, and rejected; +- structural changes and whether operator sign-off was obtained; +- verification performed and verification not performed; +- remaining findings or risks; +- a short explanation of what the status means for non-approved terminal + states. + +Constraints: + +- Do not include full diffs. +- Do not repeat full reviewer findings unless unresolved findings matter. +- Keep it concise and useful to the operator. +- Build it from per-round decision summaries already shown in conversation. +- Do not read Codex stdout, stderr, or rollout files from the main thread. +- If context compaction makes history incomplete, state that limitation + explicitly instead of inventing details. + +## Documentation Language Rule + +Repository documentation and skill files stay in English: + +- `SKILL.md` +- `references/runner.md` +- `README.md` +- `docs/DESIGN.md` +- design specs under `docs/superpowers/specs/` + +Runtime output may use the operator's language. Do not mix Russian and English +inside repository documentation paragraphs except for machine-readable literals, +CLI flags, config keys, JSON keys, or quoted runtime examples. + +## Files To Update + +Expected implementation touches: + +- `SKILL.md` + - default model, sandbox, approval parsing (note: approval policy is + expressed via `-c approval_policy=...`, not the removed `-a` flag); + - language detection and language block; + - runner input schema extensions; + - runner result schema consumption (`review_quality`, `triage.*`) and the + operation-aware lead-side dispatch table from §Runner Result Schema; + - evaluation matrix; + - structural operator gate; + - structured resume prompt; + - workspace mutation snapshots at both layers (`git status --porcelain` + AND `sha256sum` of `/tmp/codex-{body,plan,resume-body}-*`); + - one-line runtime hint at the start of every review about the + `workspace-write` default and the `sandbox:read-only` opt-out + (suppressed when an explicit `sandbox:*` override is passed); + - final operator summary. + +- `references/runner.md` + - default Codex command flags (config-based approval policy via `-c`); + - bwrap preflight before every dispatch that selects a bwrap-backed + sandbox mode; + - degraded-review detection; + - bounded triage metadata; + - emit the full runner result schema (§Runner Result Schema), including + `review_quality` and the `triage` object; + - stronger mandate and negative examples. + +- `docs/DESIGN.md` + - rationale for reviewer permissions; + - default model update; + - nested approval reasoning; + - Sonnet triage compromise; + - operator-language behavior; + - final summary rationale; + - version and verification log update after smoke testing. + +- `README.md` + - updated defaults (including config-based approval policy via `-c`, + not `-a`); + - sandbox/approval behavior; + - Safety considerations section: the gitignored-file mutation vector + (pytest → `dev.sqlite` migration), residual risk acknowledgment, the + `sandbox:read-only` opt-out (with the explicit caveat that read-only + blocks empirical verification — operators trade verification for + write-protection), and the `git worktree`-based isolation pattern for + sensitive repos; + - Linux bwrap/AppArmor setup; + - operator language behavior; + - final summary behavior. + +Do not renumber existing `docs/DESIGN.md` sections. + +## Verification + +Use the existing smoke protocol in `docs/DESIGN.md §7`, updated for the new +model and flags where needed. + +Additional manual smoke checks: + +- `codex exec` launches with `-s workspace-write`, + `-c approval_policy='"on-request"'`, and + `-c approvals_reviewer='"auto_review"'` on the target Codex CLI version + (verify against the actual installed version — `codex exec --help` + must list `-s` and `-c`; the absence of `-a/--ask-for-approval` is + expected on 0.132+). +- `codex exec resume` does not receive unsupported sandbox or approval + flags. +- bwrap-backed sandbox failure for `workspace-write` produces an early + diagnostic before Codex launches, instead of a fake review round. +- `sandbox:read-only` override still works and the runner skips approval + prompts inside the nested process (the operator can verify this by + running a review against a known-bwrap-failing host with the override). +- language block preserves parseable `[severity:]` and `VERDICT` literals. +- runner result JSON contains `review_quality` and `triage` fields when + triage runs; legacy result JSON (no `review_quality`, no `triage`) is + accepted by the lead as `review_quality=unknown` / + `triage.status=skipped`. +- triage failure does not fail an otherwise valid review. +- a synthetic `success + degraded_environmental` on `OPERATION=initial` is + treated as terminal infrastructure failure (no prior round to recover + from); the verbatim review is NOT shown. +- a synthetic `success + degraded_environmental` on `OPERATION=resume` + emits the warning, does not consume a round, and routes to the existing + fallback chain using the prior round's severity. +- a synthetic `success + degraded_environmental` on `OPERATION=fresh-exec` + is treated as terminal not-verified. +- a synthetic `success + degraded_content` (on any operation) emits the + warning, shows Step 5 verbatim, and prompts the operator for explicit + advancement. +- pre/post `git status --porcelain` catches workspace-tree mutation. +- pre/post `sha256sum` of `/tmp/codex-{body,plan,resume-body}-.md` + catches reviewer mutation of `/tmp` review inputs. +- the runtime hint about `workspace-write` and the `sandbox:read-only` + opt-out appears exactly once per review (regardless of mode) and is + suppressed when the operator passed an explicit `sandbox:*` override. +- final operator summary appears at approved, max-rounds, not-verified, + and aborted terminal states. + +Dogfood the result with: + +- plan review of this design; +- code review after implementation. + +No automated CI is required for this refresh. diff --git a/examples/review-output.md b/examples/review-output.md index 95624e2..48cb39b 100644 --- a/examples/review-output.md +++ b/examples/review-output.md @@ -4,7 +4,7 @@ This is a synthetic example showing what a typical adversarial review looks like --- -## Adversarial Review — Round 1 (mode: code, model: gpt-5.4) +## Adversarial Review — Round 1 (mode: code, model: gpt-5.5) ## Summary @@ -61,7 +61,7 @@ VERDICT: REVISE --- -## Adversarial Review — Round 2 (mode: code, model: gpt-5.4) +## Adversarial Review — Round 2 (mode: code, model: gpt-5.5) ## Summary diff --git a/references/runner.md b/references/runner.md index ba2b12e..907ee2f 100644 --- a/references/runner.md +++ b/references/runner.md @@ -1,8 +1,8 @@ # Adversarial-Review Runner Subagent -> This file is read by a Haiku subagent dispatched from `SKILL.md` Step 4 or Step 7. It is NOT loaded in the main thread. +> This file is read by a Sonnet subagent dispatched from `SKILL.md` Step 4 or Step 7. It is NOT loaded in the main thread. -You are a thin runner subagent. Your single job: launch ONE codex-exec invocation (initial, resume, or fresh-exec), validate the result, capture the session id, and return a small JSON summary. You do NOT interpret the review, propose fixes, or loop — the main orchestrator does all of that. +You are a thin runner subagent. Your job: launch ONE codex-exec invocation (initial, resume, or fresh-exec), validate the result, capture the session id, classify the review quality, run bounded triage on the findings, and return a structured JSON summary. You do NOT interpret the review, apply fixes, choose `accept` / `reject` / `re-scope`, or loop — the main orchestrator does all of that. Your triage output is a hint for the lead, not a decision. ## Input contract @@ -12,8 +12,11 @@ The main thread dispatches you via Claude Code's Agent tool. The prompt contains REVIEW_ID: REPO_ROOT: OPERATION: initial | resume | fresh-exec -CODEX_MODEL: # the model codex CLI launches; DO NOT confuse with your own (Haiku) model +CODEX_MODEL: # the model codex CLI launches; DO NOT confuse with your own (Sonnet) model CODEX_REASONING: +CODEX_SANDBOX: # default workspace-write; "inherit" omits -s and relies on the user's Codex config +CODEX_APPROVAL_POLICY: # default on-request +CODEX_APPROVALS_REVIEWER: # default auto_review; null means do not pass -c approvals_reviewer at all (used by approvals:user override and by approvals:never) PROMPT_BODY_PATH: CODEX_SESSION_ID: RESULT_PATH: /tmp/codex-runner-result-.json # you write the structured result here @@ -21,9 +24,11 @@ RESULT_PATH: /tmp/codex-runner-result-.json # you write the structur For `OPERATION=initial` and `OPERATION=fresh-exec`, `CODEX_SESSION_ID` is absent (ignore if present). +`CODEX_SANDBOX`, `CODEX_APPROVAL_POLICY`, and `CODEX_APPROVALS_REVIEWER` apply to `OPERATION=initial` and `OPERATION=fresh-exec` ONLY. `codex exec resume` does NOT accept `-s`, `-m`, or approval-related `-c` overrides — sandbox and approval mode are properties of the original session. Ignore these fields when `OPERATION=resume`. + ## Output contract — two-channel -To avoid fragility of JSON-in-final-message (Haiku frequently wraps structured output in markdown fences or adds preamble), you return results via TWO channels: +To avoid fragility of JSON-in-final-message (subagents sometimes wrap structured output in markdown fences or add preamble), you return results via TWO channels: **Channel 1 — result file (authoritative).** Write the JSON object below to `${RESULT_PATH}` via Write tool. Main reads this file directly; its bytes are the contract. Do NOT omit any field — use `null` for absent values. @@ -37,10 +42,34 @@ To avoid fragility of JSON-in-final-message (Haiku frequently wraps structured o "errors": "" | null, "archived_stdout": "/tmp/codex-stdout--failed-resume.jsonl" | null, "archived_stderr": "/tmp/codex-stderr--failed-resume.txt" | null, - "user_warning": "" | null + "user_warning": "" | null, + "review_quality": "valid" | "degraded_environmental" | "degraded_content" | "unknown", + "triage": { + "status": "ok" | "skipped" | "failed", + "finding_count": , + "max_severity": "critical" | "high" | "medium" | "none", + "covered_critical": , + "covered_high": , + "covered_medium": , + "truncated": , + "needs_lead_judgment": + } } ``` +Field semantics for the refresh-era fields: + +- `review_quality=valid` — review file passes R4 checks and the body looks like a normal adversarial review (concrete findings, real verdict). +- `review_quality=degraded_environmental` — Codex returned an exit-0 pseudo-review caused by sandbox or environment failure (bwrap-EPERM, trust prompt, rate-limit stub, missing-binary self-report, etc.). The text may still contain `VERDICT:` and severity tags but describes an inability to perform the review. +- `review_quality=degraded_content` — review parses cleanly but cheap heuristics suggest the body is not actionable (e.g. only a sandbox self-report, no concrete findings backing severity tags). Conservative — when in doubt, prefer `valid` and let the lead decide. +- `review_quality=unknown` — triage could not classify (triage step crashed, or `OPERATION=success` but the file is structured in a way the runner doesn't recognize). Main treats this as `valid` plus a warning. +- `triage.status=ok` — triage ran, count + severity + coverage fields populated. +- `triage.status=skipped` — triage did not run because Codex itself failed (no review to triage); other triage fields are zero / `none` / `false`. +- `triage.status=failed` — triage crashed mid-run; counts and severity may be missing. The review is still usable if `review_quality=valid`. +- `triage.covered_*` are the per-severity counts the runner inspected with cheap checks (file existence, simple `rg`, small read-only snippets). The lead does its own evaluation matrix; these counts are a hint, not a prescription. +- `triage.truncated=true` means there were more than 10 medium-severity findings and the runner summarized the remainder. +- `triage.needs_lead_judgment=true` means the runner was uncertain about a finding it inspected and explicitly defers to the lead. + **Channel 2 — final message (short).** Your FINAL message to main should be a single line: ``` @@ -57,9 +86,11 @@ Rules: - `result=success` ⇒ `verdict` and `review_file` must be set. `codex_session_id` must be set iff `verdict=REVISE` (or null per §2.4.4 on resume zero-find — see Step R4.4). - `result=timeout` ⇒ codex timed out (exit 124). `review_file` may be null. - `result=launch_failure` ⇒ infrastructure retry (one internal retry) already failed. Main treats this as TERMINAL — it will NOT re-dispatch you. `errors` MUST include the tail of stderr. -- `result=infra_error` ⇒ something outside codex (e.g. `/tmp` not writable). -- `user_warning` is non-null when main should surface a one-line warning to the user (e.g. §2.4.4 zero-find on resume). +- `result=infra_error` ⇒ something outside codex (e.g. `/tmp` not writable, `bwrap` preflight failed). +- `user_warning` is non-null when main should surface a one-line warning to the user (e.g. §2.4.4 zero-find on resume, or `degraded_environmental` classification). - Do NOT return the review text in the JSON. Main reads `review_file` directly. +- `review_quality` MUST be set on every result, including `success`, `timeout`, `launch_failure`, `infra_error`, and `input_error`. For non-`success` results, set `review_quality=unknown` and `triage.status=skipped`. +- `triage` MUST always be present as an object. When triage is skipped, fill counts with `0`, `max_severity="none"`, `truncated=false`, `needs_lead_judgment=false`. ## Step-by-step @@ -91,17 +122,70 @@ On systems with coarse mtime granularity (1s), two successive Writes within the Alternatively and equivalently safe: skip the mtime bump entirely and rely on ATTEMPT_ID rotation alone — the positive content-match in Step R4.4 binds on the marker, not solely on `-newer`. If the retry's new ATTEMPT_ID is embedded in the prompt's first line (which it is), no prior rollout can false-match. The `-newer` condition is a second guard, not a primary one. If the repeat-Write approach fails in practice, drop it and rely on content-match + multi-match-aborts. +### Step R2.5: Sandbox preflight (initial / fresh-exec only) + +This step runs **only when** `OPERATION=initial` or `OPERATION=fresh-exec` AND `CODEX_SANDBOX` is a bwrap-backed mode (`read-only` or `workspace-write`). Skip for `OPERATION=resume` (sandbox is inherited from the original session — the resume itself will fail if the host can't run bwrap, but main re-routes that as `degraded_environmental`). Skip for `CODEX_SANDBOX` values `danger-full-access` (no bwrap) and `inherit` (effective sandbox is unknown until Codex launches; trust the operator's chosen Codex config). + +Run the probe: + +```bash +bwrap --dev-bind / / --unshare-net /bin/echo ok 2>&1 +``` + +Bash tool `timeout` parameter: `10000` (10 s — the probe is cheap, anything slower than that is broken). + +- Exit code `0` AND stdout contains `ok` → preflight passed. Proceed to Step R3. +- Any other outcome → write this terminal result to `${RESULT_PATH}` and return the `RUNNER_RESULT_AT:` line. This is treated by main as `degraded_environmental` on the initial dispatch (terminal per the dispatch table in `SKILL.md`): + +```json +{ + "result": "success", + "verdict": "REVISE", + "review_file": null, + "codex_session_id": null, + "attempt_id": "", + "errors": null, + "archived_stdout": null, + "archived_stderr": null, + "user_warning": "Sandbox preflight failed: `bwrap --dev-bind / / --unshare-net /bin/echo ok` did not return 0. On Ubuntu 24.04+ this is usually AppArmor blocking unprivileged user namespaces — see README.md \"Linux sandbox prerequisites\". The bwrap preflight runs for both `read-only` and `workspace-write`, so `sandbox:read-only` does NOT bypass it. Real bypass options: `sandbox:danger-full-access` (no bwrap), `sandbox:inherit` (trust the user's Codex config), or apply the bwrap-userns-restrict AppArmor profile.", + "review_quality": "degraded_environmental", + "triage": { + "status": "skipped", + "finding_count": 0, + "max_severity": "none", + "covered_critical": 0, + "covered_high": 0, + "covered_medium": 0, + "truncated": false, + "needs_lead_judgment": false + } +} +``` + +Use `result=success` (not `infra_error`) deliberately: `success + degraded_environmental` is the schema-level signal main consumes via the operation-aware dispatch table. `infra_error` is reserved for orchestration-side problems like `/tmp` unwritability that aren't caused by Codex / the host sandbox. + ### Step R3: Launch codex **Synchronous launch only.** Always invoke the Bash tool with `run_in_background: false` (the default). Never set `run_in_background: true` for this call — if codex runs in background, you will proceed to Step R4 before stdout/stderr/review files are populated, and the stderr-missing check will incorrectly route to `infra_error`. -For `OPERATION=initial` and `OPERATION=fresh-exec`: +**Sandbox / approval flag construction (initial / fresh-exec only):** + +- If `CODEX_SANDBOX = "inherit"` → OMIT the `-s` flag entirely. +- Otherwise → include `-s ${CODEX_SANDBOX}`. +- Always include `-c approval_policy='""'` (the value is wrapped in escaped double quotes because Codex's `-c` expects a TOML-quoted string). +- If `CODEX_APPROVALS_REVIEWER` is `null` (or omitted by main) → DO NOT pass `-c approvals_reviewer=...`. Codex falls back to its default `user` reviewer. +- Otherwise → include `-c approvals_reviewer='""'`. +- The pre-refresh `--ask-for-approval`/`-a` flag is REMOVED. Codex CLI 0.132+ no longer accepts it at the top level; approval policy is expressed only via `-c approval_policy=...`. Do not emit `-a` regardless of the installed Codex CLI version — the `-c` form is supported across the range we care about. + +For `OPERATION=initial` and `OPERATION=fresh-exec` (showing the default case `CODEX_SANDBOX=workspace-write`, `CODEX_APPROVAL_POLICY=on-request`, `CODEX_APPROVALS_REVIEWER=auto_review`): ```bash cat /tmp/codex-prompt-${REVIEW_ID}.md | timeout 600 codex exec --json \ -m ${CODEX_MODEL} \ -c model_reasoning_effort=${CODEX_REASONING} \ - -s read-only \ + -s ${CODEX_SANDBOX} \ + -c approval_policy='"${CODEX_APPROVAL_POLICY}"' \ + -c approvals_reviewer='"${CODEX_APPROVALS_REVIEWER}"' \ -C "${REPO_ROOT}" \ -o /tmp/codex-review-${REVIEW_ID}.md \ - \ @@ -109,6 +193,8 @@ cat /tmp/codex-prompt-${REVIEW_ID}.md | timeout 600 codex exec --json \ 2>/tmp/codex-stderr-${REVIEW_ID}.txt ``` +Drop the `-s` line if sandbox is `inherit`; drop the `-c approvals_reviewer=...` line if it is `null`. + Bash tool `timeout` parameter: `620000` (10 min + headroom). For `OPERATION=resume`: @@ -122,7 +208,7 @@ cd "${REPO_ROOT}" && cat /tmp/codex-resume-prompt-${REVIEW_ID}.md | timeout 600 2>/tmp/codex-stderr-${REVIEW_ID}.txt ``` -Note: resume does NOT accept `-C`, `-s`, or `-m`; these are inherited from the original session. Use `cd` to pin cwd. +Note: `codex exec resume` does NOT accept `-C`, `-s`, `-m`, or approval-related `-c` overrides — these are properties of the original session. Use `cd` to pin cwd. Do NOT attempt to "change sandbox mid-review" by passing flags to resume; if main needs a different sandbox, it must initiate a fresh exec (which consumes a new round from the 5-round counter). Substitute literal values for every `${...}` placeholder before invoking Bash — they are template placeholders, not shell variables. @@ -143,7 +229,7 @@ Do these in order. Stop and return as soon as one fails. - File missing or empty → route to retry (Step R5). - Does NOT contain a line matching `^VERDICT: (APPROVED|REVISE)$` → route to retry. - Verdict is `REVISE` AND file contains NO line matching `\[severity:\s*(critical|high|medium)` → route to retry (reviewer format drift). -- Verdict is `APPROVED` → write this EXACT JSON object to `${RESULT_PATH}` and return the `RUNNER_RESULT_AT:` line: +- Verdict is `APPROVED` → record the following 9 base fields. **Do NOT write the JSON yet — proceed to Step R4.5 for `review_quality` + `triage` enrichment and the final write:** ```json { @@ -165,7 +251,7 @@ Do these in order. Stop and return as soon as one fails. *Primary — first line of JSONL stdout:* -Read `/tmp/codex-stdout-${REVIEW_ID}.jsonl`. If the first line parses as JSON with a `thread_id` field matching `^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`, save it as `CODEX_SESSION_ID`, then write the SAME 9-field JSON shape as the secondary tier's "Exactly one path" branch (below) to `${RESULT_PATH}` and return the `RUNNER_RESULT_AT:` line. Otherwise fall through to the secondary tier. +Read `/tmp/codex-stdout-${REVIEW_ID}.jsonl`. If the first line parses as JSON with a `thread_id` field matching `^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`, save it as `CODEX_SESSION_ID`, then record the SAME 9 base fields as the secondary tier's "Exactly one path" branch (below). **Do NOT write the JSON yet — proceed to Step R4.5 for `review_quality` + `triage` enrichment and the final write.** Otherwise fall through to the secondary tier. *Secondary — rollout content-match:* @@ -177,7 +263,7 @@ find ~/.codex/sessions -name 'rollout-*.jsonl' -newer -exec grep -l 'AD **Interpret the result by STDOUT, not exit code.** Split the command's stdout on newlines; count non-empty lines. Empty stdout means ZERO paths regardless of the pipeline's exit status (`find` returning no matches and `grep -l` matching nothing in found files both yield empty stdout with different exit codes; treat both as zero). -- **Exactly one path** → extract the trailing UUID from the filename (pattern `rollout--.jsonl`; UUID is the 36-char hex-and-dashes segment before `.jsonl`), then write this EXACT JSON object to `${RESULT_PATH}` (all 9 fields explicitly; do NOT leave any omitted or as literal placeholder like `""`): +- **Exactly one path** → extract the trailing UUID from the filename (pattern `rollout--.jsonl`; UUID is the 36-char hex-and-dashes segment before `.jsonl`), then record the following 9 base fields. **Do NOT write the JSON yet — proceed to Step R4.5 for `review_quality` + `triage` enrichment and the final write:** ```json { @@ -193,7 +279,7 @@ find ~/.codex/sessions -name 'rollout-*.jsonl' -newer -exec grep -l 'AD } ``` -- **Zero paths for resume** → write this EXACT JSON object (9 fields, `codex_session_id` is null, `user_warning` carries the §2.4.4 diagnostic): +- **Zero paths for resume** → record the following 9 base fields with `codex_session_id=null` and `user_warning` carrying the §2.4.4 diagnostic. **Do NOT write the JSON yet — proceed to Step R4.5:** ```json { @@ -210,9 +296,61 @@ find ~/.codex/sessions -name 'rollout-*.jsonl' -newer -exec grep -l 'AD ``` - **Zero paths for initial/fresh-exec** → route to retry (Step R5). Main needs the id to launch next round. Set `errors: "session-id capture failed: both tiers empty on initial/fresh-exec"`. -- **Multiple paths** → write `launch_failure` result with `errors: "multiple rollouts matched marker — aborting to avoid wrong-session bind"`. Do NOT pick one. +- **Multiple paths** → write a terminal `launch_failure` result with `errors: "multiple rollouts matched marker — aborting to avoid wrong-session bind"` and `review_quality="unknown"` + the skipped-triage object (`triage.status="skipped"`, counts `0`, `max_severity="none"`, `truncated=false`, `needs_lead_judgment=false`). Do NOT pick one rollout. Do NOT route through Step R5 — this is an "aborting to avoid silent drift" terminal result, not a retryable failure. The result file MUST be 11-field complete. -(The two `success` JSON shapes are inlined above per branch. Every success path through R4.4 MUST emit a complete 9-field JSON object — never rely on implicit defaults, never leave a field omitted, never write a literal placeholder like `""` in the output.) +(Every success path through R4.4 records 9 base fields. The final 11-field write — base + `review_quality` + `triage` — happens in Step R4.5.) + +### Step R4.5: Classify review quality and triage findings + +This step runs once per success result, after R4.3/R4.4 have recorded the 9 base fields. It produces the `review_quality` classification and the bounded `triage` object, then writes the final 11-field JSON to `${RESULT_PATH}` and returns the `RUNNER_RESULT_AT:` line. + +**Step R4.5.1: Classify `review_quality`.** + +Default to `valid`. Re-classify based on cheap content checks against `/tmp/codex-review-${REVIEW_ID}.md`: + +- **`degraded_environmental`** when the review body matches any of these patterns (case-insensitive, anchored at line start where applicable): + - `bwrap: ` (bubblewrap diagnostic leaking into review) + - `^Sandbox(ing)?( error| failure)?:` or `sandbox setup failed` + - `permission denied .* (sandbox|exec|bind)` + - `cannot run (commands|tests|tools) in (this )?sandbox` + - `rate limit (exceeded|hit)` AS THE ONLY substantive content (review body < 500 chars total) + - `trust prompt` / `requires (interactive )?approval` AS THE ONLY substantive content + - `unable to perform (the )?review` / `failed to start (the )?review` AS THE ONLY substantive content + Use `rg` (`-i -F` for literal substrings, `-i` for short regex). If two or more independent patterns match, classify as `degraded_environmental` even if the body is long — the reviewer likely produced a meta-explanation rather than a review. + +- **`degraded_content`** (conservative catch) when: + - `VERDICT: REVISE` is present BUT the file contains zero `[severity: critical|high|medium]` tags (already caught by R4.3, but if a previous attempt slipped through, this is a backstop), OR + - Severity tags exist but each finding body is < 80 chars (no concrete scenario, no file references) — heuristic for placeholder findings. + When in doubt, prefer `valid`. False positives here force the lead through an unnecessary user-prompt; false negatives at worst surface a noisy review the lead will downgrade in the evaluation matrix. + +- **`unknown`** if classification itself errors (file disappeared between R4.3 and here, `rg` not on PATH, etc.). Do NOT retry classification — emit `unknown` and let the lead handle it. + +- **`valid`** otherwise. + +**Step R4.5.2: Run bounded triage.** + +If `review_quality = degraded_environmental` → set `triage.status = skipped`; fill numeric fields with `0`, `max_severity = "none"`, `truncated = false`, `needs_lead_judgment = false`. Skip to R4.5.3. + +Otherwise: + +1. Count severity tags: `rg -c -i '^\[severity:\s*(critical|high|medium)' /tmp/codex-review-${REVIEW_ID}.md` (or equivalent — match the actual finding-header form used by the prompt template). Populate `finding_count`. +2. Compute `max_severity` from the highest tier present (`critical > high > medium > none`). +3. For each `[severity: critical]` finding (up to all of them) and each `[severity: high]` finding (up to all of them), do a cheap inspection: if the finding cites a file path or line range, verify with `ls`/`rg` that the path exists and the citation is plausible. Increment `covered_critical` / `covered_high` accordingly. **Do NOT run tests, builds, doc lookups, or web searches in this step** — those belong to the main thread / the lead's evaluation matrix. +4. For medium findings: inspect up to 10. If more remain, set `truncated = true`. Increment `covered_medium` for each inspected. +5. If any inspected finding is ambiguous (cited path doesn't exist, severity feels miscategorized, or the inspection produced no signal), set `needs_lead_judgment = true`. This is a hint, not a gate. +6. If any rg / ls / read call errors out, set `triage.status = failed`, leave fields at whatever was populated so far (zero-initialize anything not yet set), and proceed. A failed triage does NOT downgrade `review_quality` — main treats triage as a hint. + +On success, set `triage.status = ok`. + +**Hard ceiling.** Total wall time for R4.5.2 SHOULD NOT exceed 30 seconds. If you're approaching that, stop, mark `triage.status = failed`, and proceed. The lead does the real evaluation; triage is a hint. + +**Step R4.5.3: Write the final 11-field JSON.** + +Combine the 9 base fields recorded in R4.3/R4.4 with `review_quality` (R4.5.1) and the `triage` object (R4.5.2). Use the Write tool to overwrite `${RESULT_PATH}`. All 11 top-level fields MUST be present; never leave a field omitted, never write a literal `""` placeholder. + +If `review_quality = degraded_environmental` and `user_warning` is currently `null`, replace `user_warning` with a one-line operator diagnostic describing the most likely cause (e.g. "Reviewer returned an environmentally-degraded response — content matched sandbox/permission-failure patterns. Inspect /tmp/codex-review-.md and consider `sandbox:read-only` or fresh-exec."). + +Return the `RUNNER_RESULT_AT: ${RESULT_PATH}` line. ### Step R5: Retry once on any failure (TERMINAL — main will not re-dispatch) @@ -237,9 +375,16 @@ Then write the result with `archived_stdout` and `archived_stderr` set to the `- - For `OPERATION=initial` or `OPERATION=fresh-exec`: no archival needed (there is no next attempt within this REVIEW_ID to collide). Leave files at their normal paths for main's diagnostic read (main is allowed to `mv`/`rm` by path; it just doesn't read content). Write the appropriate terminal result and return the `RUNNER_RESULT_AT: ...` line: -- Second attempt exit was 124 → write `{"result":"timeout","errors":"codex exceeded 600s on both attempts", ...}` (9 fields, all others null as applicable). +- Second attempt exit was 124 → write `{"result":"timeout","errors":"codex exceeded 600s on both attempts", ...}`. - Any other failure mode → write `launch_failure` with stderr tail (≤500 chars) in `errors`. -In both cases, fill all 9 fields (set `archived_stdout`/`archived_stderr` only when the archival mv in the OPERATION=resume branch ran, else null; set `user_warning` null; set `verdict` null; set `review_file` to `/tmp/codex-review-${REVIEW_ID}.md` only if that file contains a valid VERDICT line, else null). + +In both cases, fill all 11 top-level fields: + +- Base 9 fields: set `verdict = null`, `review_file = /tmp/codex-review-${REVIEW_ID}.md` only if that file contains a valid VERDICT line (else `null`), `codex_session_id = null`, `attempt_id = `, `errors` per above, `archived_stdout`/`archived_stderr` set only when the archival mv in the OPERATION=resume branch ran (else `null`), `user_warning = null`, plus the chosen `result`. +- `review_quality = "unknown"` (no successful review to classify). +- `triage = { "status": "skipped", "finding_count": 0, "max_severity": "none", "covered_critical": 0, "covered_high": 0, "covered_medium": 0, "truncated": false, "needs_lead_judgment": false }`. + +The same 11-field rule applies to the `infra_error` and `input_error` paths elsewhere in this spec: every result file MUST be 11-field complete. ### Step R6: Cleanup @@ -252,7 +397,19 @@ The one exception is the `mv` in Step R5 above — this is NOT cleanup (files ar ## Notes -- You run as a Haiku subagent. Your 250K context is disposed when you return. Anything you read (stderr files, rollout paths, JSONL streams) does NOT reach the main thread — that is the whole point. -- Do NOT ask the main thread clarifying questions. If input is missing or malformed, write an `input_error` result to `${RESULT_PATH}` and return the `RUNNER_RESULT_AT:` line. -- Do NOT attempt to apply fixes, interpret severity, or re-run more than one retry. The 5-round orchestration loop lives in main. +- You run as a Sonnet subagent. Your 1M context is disposed when you return. Anything you read (stderr files, rollout paths, JSONL streams, the review file during triage) does NOT reach the main thread — that is the whole point of the runner layer. +- Do NOT ask the main thread clarifying questions. If input is missing or malformed, write an `input_error` result to `${RESULT_PATH}` (11-field complete: base + `review_quality="unknown"` + skipped triage) and return the `RUNNER_RESULT_AT:` line. +- Do NOT attempt to apply fixes, interpret severity beyond the bounded triage in R4.5, or run more than one retry. The 5-round orchestration loop and ALL final `accept` / `reject with reasoning` / `re-scope` decisions live in main. - The final line of your message is ONLY `RUNNER_RESULT_AT: ` — nothing before, nothing after, no markdown fence. Main's regex tolerates minor wrapping, but adhering to the spec eliminates edge cases entirely. + +## Negative examples — what the runner MUST NOT do + +These are common ways to drift outside the runner's mandate. If you find yourself doing any of these, stop and re-read this spec. + +- **Do not edit, create, or delete any file inside `${REPO_ROOT}`.** The runner is read-only with respect to the workspace tree. The reviewer (Codex) may or may not be sandboxed depending on `CODEX_SANDBOX`, but the runner itself never modifies project files. +- **Do not apply fixes to the artifact under review.** Even if you can see exactly what the reviewer is asking for, "fix and re-launch" is not the runner's job. Return the result; main applies fixes. +- **Do not start a second Codex review round.** Each dispatch is exactly one Codex operation (initial, resume, or fresh-exec) plus at most one internal retry. Multi-round orchestration is in `SKILL.md` Steps 5–7. +- **Do not delete or rename `/tmp/codex-*` files** except for the explicit archival `mv` in Step R5's `OPERATION=resume` branch. Main owns the cleanup glob at `SKILL.md` Step 9. +- **Do not pick which findings the lead should accept, reject, or re-scope.** Triage emits counts and a `needs_lead_judgment` hint; nothing else. If you wrote any per-finding "accept" or "reject" annotation into the JSON, you've overstepped — remove it. +- **Do not decide that a review is "good enough" to terminate the loop.** `verdict=APPROVED` comes from Codex; the runner only passes it through. Never emit `verdict=APPROVED` based on your own judgment. +- **If a command unexpectedly modifies project files (e.g. a probe command had a side effect on `${REPO_ROOT}`), STOP immediately and report.** Write a result with `review_quality="degraded_environmental"`, `user_warning` describing what was modified, and the `errors` field naming the offending command. Do NOT attempt to undo the modification — main does the post-dispatch `git status --porcelain` snapshot and will detect the drift on its own.