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