diff --git a/README.md b/README.md index 789eb25..f04caec 100644 --- a/README.md +++ b/README.md @@ -114,7 +114,7 @@ chosen config file: "Bash(cat /tmp/codex-prompt-* | timeout 600 codex exec *)", // Codex: resume (cd prefix because resume has no -C flag; prompt via cat | pipe) "Bash(cd * && cat /tmp/codex-resume-prompt-* | timeout 600 codex exec resume *)", -// Session-id filesystem fallback (newest rollout file in ~/.codex/sessions/) +// Session-id filesystem fallback (POSIX: find -newer + grep -l for content-match) "Bash(find ~/.codex/sessions*)", // Diagnostic aid when filesystem fallback finds nothing "Bash(ls -t ~/.codex/sessions*)", @@ -229,9 +229,15 @@ In some Claude Code sandbox configurations codex's `--json` event stream is suppressed when stdout is redirected to a file — the `/tmp/codex-stdout-*.jsonl` ends up 0 bytes even though the review itself (`-o /tmp/codex-review-*.md`) completes correctly. The skill handles this automatically via a filesystem -fallback: when the JSONL stream is empty it extracts the session UUID from -the newest `~/.codex/sessions/YYYY/MM/DD/rollout-*-.jsonl` filename -created since the pre-exec timestamp. Resume continues to work normally. +fallback: every prompt includes a unique session marker +(``) that gets written to +the rollout JSONL on disk. When the JSONL stream is empty, the skill runs +`find ~/.codex/sessions -name 'rollout-*.jsonl' -newer -exec +grep -l {} +` to positively identify this session's rollout by +content match (not by newest-mtime, which would be unsafe against parallel +codex invocations) and extracts the UUID from the filename. Resume continues +to work normally. The commands used are POSIX (`find -newer`, `-exec grep -l`) +and work identically on Linux and macOS. **"NOT VERIFIED" result.** The skill applied fixes but the reviewer did not re-verify them (resume @@ -267,12 +273,10 @@ review correctness. scoped to the submodule — `git rev-parse --show-toplevel` does not walk up to the parent. A warning is printed; invoke from the parent repo if you want parent scope. -- **GNU find on macOS.** The secondary session-id capture uses - `find -newermt "@"` and `-printf`, both GNU extensions. On - macOS (BSD find) the skill's default command does not work; the skill - states the *goal* of the step in SKILL.md and invites the model (or - user) to substitute an equivalent BSD-compatible command. The skill - has not been end-to-end tested on macOS. +- **macOS end-to-end not tested.** The secondary session-id capture + uses only POSIX flags (`find -newer FILE`, `-exec CMD {} +`, `grep -l`), + so it should work identically on macOS as on Linux, but the skill has + not been end-to-end tested on macOS. ## Roadmap diff --git a/SKILL.md b/SKILL.md index 8acd774..7c71cdc 100644 --- a/SKILL.md +++ b/SKILL.md @@ -23,7 +23,7 @@ Sends current work for adversarial review through an external AI model (OpenAI C ## Instructions -> **Placeholders:** `${REVIEW_ID}`, `${CODEX_SESSION_ID}`, `${CODEX_SESSIONS_BEFORE}`, `${REPO_ROOT}`, and `${BASE_BRANCH}` in the steps below are template placeholders, NOT shell variables. Substitute literal values directly into each tool call. In particular, `${REPO_ROOT}` is ALWAYS an absolute path captured at Step 2 (never `$(pwd)`); `${CODEX_SESSIONS_BEFORE}` is a unix-timestamp integer captured immediately before every `codex exec` / `codex exec resume` (see Steps 4 and 7), used by the filesystem session-id fallback — substitute the integer verbatim into `find -newermt "@"`, never leave `${CODEX_SESSIONS_BEFORE}` as a shell variable reference. +> **Placeholders:** `${REVIEW_ID}`, `${CODEX_SESSION_ID}`, `${REPO_ROOT}`, and `${BASE_BRANCH}` in the steps below are template placeholders, NOT shell variables. Substitute literal values directly into each tool call. In particular, `${REPO_ROOT}` is ALWAYS an absolute path captured at Step 2; never replace it with `$(pwd)`. `${REVIEW_ID}` is also embedded verbatim in every prompt (as an HTML-style comment marker) so the filesystem session-id fallback can positively identify this session's rollout by content-match; do NOT generate a different REVIEW_ID for the marker, use the same one as for file paths. ### Step 1: Determine review mode @@ -127,9 +127,18 @@ If all sources are empty — no changes to review, inform the user. Build the prompt depending on the mode. All prompts use the adversarial stance. +**All prompts begin with a session marker.** The FIRST line of every prompt (plan, code, code-vs-plan, resume, fresh-exec fallback) must be a literal HTML-style comment: + +``` + +``` + +Substitute the actual REVIEW_ID value (e.g., `1711872000-48217593`). The comment is ignored by Codex as content but becomes part of the rollout transcript on disk, which is how the filesystem session-id fallback in check 4 positively binds a rollout file to this review (grep for the marker in rollout JSONL). Without this marker the fallback cannot distinguish this session's rollout from a parallel codex invocation. + **Prompt for plan review:** ``` + You are a senior adversarial reviewer of implementation plans. Your job is to break confidence in the plan, not to validate it. @@ -198,6 +207,7 @@ VERDICT: REVISE **Prompt for code review (<= 50 files):** ``` + You are a senior adversarial code reviewer. Your job is to break confidence in the change, not to validate it. @@ -319,11 +329,7 @@ Flags: **Plan Mode note:** Writing to `/tmp` via Write tool may trigger a permission prompt or exit Plan Mode. This is a known Claude Code limitation — Plan Mode restricts edits to the plan file only. If this happens, it does not affect review correctness: the review mode is already determined, and the skill only edits the plan file and `/tmp` temp files. -**Capture pre-exec timestamp** (for filesystem fallback of session-id; see the secondary-path check below). - -Compute `CODEX_SESSIONS_BEFORE` **in your own reasoning, without a Bash call** — take the current Unix timestamp (you know the wall-clock time from your session context), subtract 1, and substitute the resulting integer literally into the `find -newermt "@"` call in check 4. Example: if your current time is 2026-04-17 17:30:00 UTC, then `CODEX_SESSIONS_BEFORE = 1776447000 - 1 = 1776446999`. - -The `- 1` shifts the window back one second to avoid a same-epoch race: `find -newermt "@N"` treats mtime **strictly greater** than N, so if codex finishes in the same epoch-second as the capture (fast path, cached response), the rollout file would be missed without this shift. Cost: the lookup window widens by 1 second, irrelevant against codex exec duration. If you are uncertain of the exact current epoch second, subtract an extra few seconds to be safe — the window is only used to filter out obviously-stale rollout files, precision is not important. +The prompt file (`/tmp/codex-prompt-${REVIEW_ID}.md`) just written serves as the anchor for the filesystem session-id fallback: its mtime is strictly earlier than any rollout file codex will create for this session, and it exists on disk without requiring any extra write. Check 4 below uses `find -newer` against this file instead of a timestamp arithmetic computation. ```bash cat /tmp/codex-prompt-${REVIEW_ID}.md | timeout 600 codex exec --json \ @@ -374,29 +380,23 @@ cat /tmp/codex-prompt-${REVIEW_ID}.md | timeout 600 codex exec --json \ - **First line parses as JSON AND has a valid `thread_id` UUID** → save as `CODEX_SESSION_ID`, proceed to Step 5. - **Any other case** (file empty / 0 bytes, first line not valid JSON, JSON has no `thread_id`, `thread_id` is not a UUID, partial/garbage output) → fall through to the secondary path below. Do NOT save an empty or malformed `CODEX_SESSION_ID`. - **Secondary: rollout filename.** The primary fails for two independent reasons: (a) in some Claude Code sandbox configurations `--json` stdout is empty (0 bytes) even on exit 0 with populated `-o`; (b) partial or format-drifted output from a future codex version. In both cases the session is still recoverable from disk: every `codex exec` writes a rollout file named `rollout--.jsonl` under `~/.codex/sessions/YYYY/MM/DD/` (see `DESIGN.md §2.3`). The trailing UUID in the filename is the session id. + **Secondary: rollout content-match.** The primary fails for two independent reasons: (a) in some Claude Code sandbox configurations `--json` stdout is empty (0 bytes) even on exit 0 with populated `-o`; (b) partial or format-drifted output from a future codex version. In both cases the session is recoverable from disk: every `codex exec` writes a rollout file named `rollout--.jsonl` under `~/.codex/sessions/YYYY/MM/DD/` (see `DESIGN.md §2.3`). The trailing UUID in the filename is the session id — but blindly picking the newest rollout risks binding to a parallel codex invocation (silent corruption). To bind positively, the skill matches **both** (i) rollout mtime newer than the prompt file (timestamp anchor) AND (ii) rollout contains the session marker (content anchor). - Run: + Run this single POSIX-portable invocation: ```bash - find ~/.codex/sessions -name 'rollout-*.jsonl' -newermt "@${CODEX_SESSIONS_BEFORE}" -printf '%T@ %f\n' 2>/dev/null + find ~/.codex/sessions -name 'rollout-*.jsonl' -newer /tmp/codex-prompt-${REVIEW_ID}.md -exec grep -l 'ADVERSARIAL-REVIEW-SESSION: ${REVIEW_ID}' {} + 2>/dev/null ``` - This prints zero or more lines of ` ` for rollout files created after the pre-exec timestamp. From the result: + Substitute the actual REVIEW_ID value in BOTH places (the prompt file path and the grep pattern — same value). `-newer FILE` and `-exec ... {} +` are POSIX; `grep -l` is POSIX. Works identically on Linux and macOS. - - **Zero lines** → no rollout file found. Before aborting, surface useful diagnostic context to the user: the contents of `/tmp/codex-stdout-${REVIEW_ID}.jsonl` (if non-empty), `/tmp/codex-stderr-${REVIEW_ID}.txt`, and the 3 most-recent rollout filenames (`ls -t ~/.codex/sessions/*/*/*/rollout-*.jsonl 2>/dev/null | head -3`). Then treat as launch failure, retry once, then abort. - - **One or more lines** → pick the line with the largest epoch-mtime (there is usually just one; multiple lines indicate a parallel codex invocation in the same second). Extract the trailing UUID from that filename (the 36-char hex-and-dashes pattern above) and save as `CODEX_SESSION_ID`. + The output is zero or more rollout paths that (a) postdate our prompt file AND (b) contain our session marker. From the result: - A single `find` invocation keeps the permission rule simple (`Bash(find ~/.codex/sessions*)`) and the parsing stays in your head — no shell pipeline needed. + - **Exactly one path** (the expected case) → this is our rollout. Extract the trailing UUID from the filename (the 36-char hex-and-dashes pattern above) and save as `CODEX_SESSION_ID`. + - **Zero paths** → **fail closed.** Either codex did not create a rollout, or something prevented the marker from reaching disk. We cannot safely guess — picking anything else risks binding to an unrelated session. Before aborting, surface diagnostic context to the user: the contents of `/tmp/codex-stdout-${REVIEW_ID}.jsonl` (if non-empty), `/tmp/codex-stderr-${REVIEW_ID}.txt`, and the 3 most-recent rollout filenames (`ls -t ~/.codex/sessions/*/*/*/rollout-*.jsonl 2>/dev/null | head -3`). Then treat as launch failure, retry once, then abort. + - **Multiple paths** (should not happen — `REVIEW_ID` collision has probability ~10⁻⁸) → pick any, proceed. If it is wrong, Step 7 resume will fail one of the three checks and route to the fallback chain. - **Platform note.** `-newermt "@"` and `-printf` are GNU extensions. On macOS (BSD find) they are unsupported — substitute an equivalent that achieves the same goal: "list rollout files modified since `${CODEX_SESSIONS_BEFORE}`, newest first". For example, `find ~/.codex/sessions -name 'rollout-*.jsonl' -type f` plus `stat -f '%m %N' ` per result, or `ls -t ~/.codex/sessions/*/*/*/rollout-*.jsonl` and filter by a reference file's mtime. The goal is what matters, not the exact flags. - - **Parallel-codex caveat — real silent-corruption risk, not benign.** If the user runs `codex` in parallel in the same cwd during the pre-exec timestamp window, the newest rollout may belong to that other invocation. Step 7's resume against the wrong session returns a normally-shaped response (VERDICT + severity markers), so the skill's post-resume checks in Step 7 will NOT detect the mismatch. The skill then applies "fixes" guided by a review of the wrong artifact. Two mitigations in effect: - - 1. The `CODEX_SESSIONS_BEFORE` timestamp is captured immediately before the exec, so the window is narrow (seconds). - 2. `--last` is never used (`§4.5`) — explicit UUID is always passed to resume. - - Neither eliminates the race. If you see suspicious behavior mid-review (reviewer mentions files or sections not in this work), halt and tell the user before applying any fix. + **Why positive-bind instead of newest-by-mtime:** the reviewer on Round-6 flagged that picking newest-by-mtime allows a parallel codex invocation (user running codex in another terminal, CI job, etc.) to create a newer rollout during the race window, which our secondary would then silently pick — Step 7 resume would succeed against that wrong session, and the skill would apply fixes informed by an unrelated review. Positive content-match eliminates this: only rollouts containing **our** `REVIEW_ID` marker are accepted; everything else is invisible to the fallback. **Where `thread_id` / session id is NOT:** @@ -474,9 +474,11 @@ Based on the reviewer's findings: **Resume is the primary path.** Saves tokens and preserves session context. A fresh `codex exec` without resume is an **emergency fallback** — costly in tokens, and requires rebuilding prior-round context. -**1. Write the resume prompt** to `/tmp/codex-resume-prompt-${REVIEW_ID}.md` via **Write tool**. Use a separate file from the initial prompt so round-1 material remains available for diagnostics. +**1. Write the resume prompt** to `/tmp/codex-resume-prompt-${REVIEW_ID}.md` via **Write tool**. Use a separate file from the initial prompt so round-1 material remains available for diagnostics. The resume prompt must begin with the same session marker as the initial prompt so the filesystem fallback can positively bind this resume's rollout: ``` + + I've revised based on your feedback. Here's what I changed: @@ -491,7 +493,7 @@ End with VERDICT: APPROVED or VERDICT: REVISE **2. Run resume.** Resume does NOT accept `-C`, so prefix the command with an explicit `cd` to `${REPO_ROOT}` (captured at Step 2). Use single quotes around `${REPO_ROOT}` — the path was validated at Step 2 to contain no single quotes. -Capture a pre-resume timestamp: compute `CODEX_SESSIONS_BEFORE` in your own reasoning as in Step 4 (current Unix timestamp minus 1; no Bash call). Then launch resume via the same `cat | ... -` pattern: +The resume prompt file (`/tmp/codex-resume-prompt-${REVIEW_ID}.md`) acts as the anchor for this resume's filesystem fallback, the same way the initial prompt file anchors Step 4. Launch resume via the `cat | ... -` pattern: ```bash cd '${REPO_ROOT}' && cat /tmp/codex-resume-prompt-${REVIEW_ID}.md | timeout 600 codex exec resume --json \ @@ -520,9 +522,9 @@ Use `timeout: 620000` in Bash tool parameters. 3. **Review file sanity.** Read `/tmp/codex-review-${REVIEW_ID}.md` and apply the same checks as Step 5.2: - Missing / empty / no `^VERDICT: (APPROVED|REVISE)$` line / REVISE without `[severity:` lines → route to fallback. Do NOT update `CODEX_SESSION_ID`. -**4. Only if all three checks pass AND the verdict is REVISE** → refresh `CODEX_SESSION_ID` using two tiers (primary = first JSONL line of `/tmp/codex-stdout-${REVIEW_ID}.jsonl`; secondary = newest rollout filename with mtime > `CODEX_SESSIONS_BEFORE`, UUID extracted from the basename). On APPROVED verdict, skip the refresh — there is no round N+1. +**4. Only if all three checks pass AND the verdict is REVISE** → refresh `CODEX_SESSION_ID` using two tiers (primary = first JSONL line of `/tmp/codex-stdout-${REVIEW_ID}.jsonl`; secondary = rollout file that is both newer than `/tmp/codex-resume-prompt-${REVIEW_ID}.md` AND contains the `ADVERSARIAL-REVIEW-SESSION: ${REVIEW_ID}` marker, with UUID extracted from the basename — exactly the positive-binding approach from Step 4 check 4, but anchored on the resume prompt instead of the initial prompt). On APPROVED verdict, skip the refresh — there is no round N+1. -> **Important — this is NOT identical to Step 4 check 4.** Step 4 check 4 treats zero-find as a launch failure because in Step 4 the session id is needed for resume to even happen. In Step 7 the resume has **already succeeded** (checks 1-3 passed), and per `DESIGN.md §2.4.4` the thread id does not rotate across resumes — so if both tiers yield nothing here, **do NOT abort and do NOT retry**: keep the previous `CODEX_SESSION_ID` unchanged, log a one-line warning to the user (`"Step 7 session-id refresh: both tiers empty, continuing with previous ID per §2.4.4"`), and continue to Step 5. +> **Important — NOT identical to Step 4 check 4 on the failure side.** Step 4 check 4 treats "no matching rollout" as a launch failure because in Step 4 the session id is needed for resume to even happen. In Step 7 the resume has **already succeeded** (checks 1-3 passed), and per `DESIGN.md §2.4.4` the thread id does not rotate across resumes — so if both tiers yield nothing here, **do NOT abort and do NOT retry**: keep the previous `CODEX_SESSION_ID` unchanged, log a one-line warning to the user (`"Step 7 session-id refresh: both tiers empty, continuing with previous ID per §2.4.4"`), and continue to Step 5. After the refresh (or the no-op refresh on zero-find), return to **Step 5** with the new review. @@ -557,9 +559,10 @@ Options: - Max severity `critical` or `high` → fresh exec automatically. The risk of silently skipping a serious finding outweighs the token cost. - Max severity `medium` only → Step 8 with the **not-verified** terminal state. -**Fresh-exec prompt template.** The lead rebuilds prior-round context from the conversation (all prior rounds were shown verbatim in Step 5.3 user messages, so they are available in context): +**Fresh-exec prompt template.** The lead rebuilds prior-round context from the conversation (all prior rounds were shown verbatim in Step 5.3 user messages, so they are available in context). The prompt must begin with the same session marker as the initial prompt so the fallback positively binds: ``` + [Original adversarial prompt for the current mode, from Step 4] ## Previous review rounds @@ -596,7 +599,7 @@ If the fresh exec later needs investigating, both the failed-resume trail (`*-fa Write the fresh-exec prompt to `/tmp/codex-prompt-${REVIEW_ID}.md` (overwriting the original is acceptable). -Launch using the **same command template as Step 4** (`cat file | timeout 600 codex exec --json ... -` with `-C`, `-o`, stdout jsonl, stderr; also re-capture `CODEX_SESSIONS_BEFORE` immediately before the call), apply the same post-launch strict check order including the two-tier session-id capture, then return to **Step 5**. +Launch using the **same command template as Step 4** (`cat file | timeout 600 codex exec --json ... -` with `-C`, `-o`, stdout jsonl, stderr). Because this fresh-exec path **overwrites** `/tmp/codex-prompt-${REVIEW_ID}.md` with new content just written above, that file's mtime is automatically the post-write moment — it serves as the `-newer` anchor for the two-tier secondary session-id capture on the fresh exec's rollout, the same way Step 4 uses it on the initial exec's rollout. Apply the same post-launch strict check order including the two-tier session-id capture, then return to **Step 5**. > This fresh exec consumes one round from the 5-round counter — same as a successful resume would have. @@ -682,7 +685,8 @@ Do NOT delete plan files that existed before the review (only temp files created - **`REPO_ROOT` is captured at Step 2** via `git rev-parse --show-toplevel` and substituted as an absolute literal path into every codex command. Never use `$(pwd)` inside codex commands — cwd drift between Bash calls makes it unreliable. - **Resume requires `cd '${REPO_ROOT}' && ...`** because `codex exec resume` has no `-C` flag; cwd is inherited from the shell. The initial exec uses `-C "${REPO_ROOT}"` instead. - **`CODEX_SESSION_ID` is updated only on full success** — ALL of (exit=0 AND stderr has no `Error:`/`thread/resume failed` line AND review file contains a valid `VERDICT:` line with findings on REVISE). On any failure, leave it unchanged and route to the fallback. -- **Session ID capture is two-tier.** Primary: `thread_id` from the first JSONL line of stdout. Secondary (primary empty / malformed / missing `thread_id`): UUID from the trailing component of the newest `~/.codex/sessions/**/rollout-*.jsonl` filename with mtime > `CODEX_SESSIONS_BEFORE`. Compute `CODEX_SESSIONS_BEFORE` (current Unix timestamp minus 1) **in your own reasoning**, no Bash call — substitute the integer literally into `find -newermt "@"`. The `-1` shift prevents a same-epoch race against `-newermt`'s strict-greater semantics. +- **Session ID capture is two-tier.** Primary: `thread_id` from the first JSONL line of stdout. Secondary (primary empty / malformed / missing `thread_id`): the rollout file that is both `-newer` than the prompt file AND contains the `ADVERSARIAL-REVIEW-SESSION: ${REVIEW_ID}` marker, with UUID from the filename. Positive content-binding eliminates the wrong-session hazard from parallel codex invocations: only our session's rollout matches the grep, everything else is invisible. +- **Every prompt sent to codex starts with `` as its first line.** This applies to initial, resume, and fresh-exec fallback prompts alike. The marker is how the filesystem session-id fallback distinguishes our rollout from a parallel codex invocation; dropping the marker breaks positive-binding and reopens the silent-corruption risk. - **Prompt delivery is `cat file | codex exec ... -`.** The `- < file` stdin-redirect form is accepted by codex but exits 1 with empty stderr in some Claude Code sandbox configurations. Pipe is portable across both envs observed. - **The `--json` stdout stream is never human-readable review text** — JSONL events when populated, empty when suppressed by sandbox. Never treat Bash result as review content; the review lives exclusively in `/tmp/codex-review-*.md`. - **Launch-failure retry** is capped at 1 per round and does NOT consume the 5-round counter. The retry counter is per-round; it resets at the start of every new round and is tracked only in that round's reasoning. diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 99047f3..8a58434 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -240,19 +240,30 @@ 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. +`thread.started` event to stdout. The rollout JSONL body also contains +the initial prompt text — so the skill can positively bind by putting +a unique marker in the prompt (`REVIEW_ID`) and grepping for it +across candidate rollouts, rather than relying on timing alone. 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 \ +MARKER="PROBE-$(date +%s)-$$" +cat > /tmp/x-prompt.md < +respond PONG +EOF +cat /tmp/x-prompt.md | 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 \ +ROLLOUT=$(find ~/.codex/sessions -name 'rollout-*.jsonl' \ + -newer /tmp/x-prompt.md \ + -exec grep -l "${MARKER}" {} + 2>/dev/null | head -1) +basename "${ROLLOUT}" .jsonl \ | 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` +rm -f /tmp/x-prompt.md /tmp/x.md ``` ### §2.4. Resume semantics @@ -422,82 +433,91 @@ Each decision below follows the same template: - **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) +### §4.1. Two-tier session ID capture (`--json` primary, positive content-bind 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**: + `/tmp/codex-stdout-${REVIEW_ID}.jsonl`. Every prompt (initial, resume, + fresh-exec fallback) starts with a session marker + `` as its first line. + Session ID capture then tries: - **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. + - **Secondary** (`§4.1b`): the rollout file that is both `-newer` than + the prompt file AND contains the session marker (grep), with UUID + extracted from the filename: + ``` + find ~/.codex/sessions -name 'rollout-*.jsonl' \ + -newer /tmp/codex-prompt-${REVIEW_ID}.md \ + -exec grep -l 'ADVERSARIAL-REVIEW-SESSION: ${REVIEW_ID}' {} + + ``` + All flags (`-newer FILE`, `-exec CMD {} +`, `grep -l`) are POSIX — + the command works unchanged on Linux and macOS. If the grep returns + zero paths, the fallback **fails closed**: the skill cannot safely + pick an unrelated rollout. +- **Where in SKILL.md.** Step 4 (launch), Step 7 (resume), Step 7 fresh- + exec fallback. All three sites use the same positive-binding pattern, + differing only in which prompt file anchors the `-newer` check. - **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. + least one Claude Code sandbox the JSONL stdout is suppressed (0 bytes) + even on exit 0 with populated `-o` (`§2.2`, `§6.6`), and without a + secondary path the skill cannot resume — every round becomes a fresh + `codex exec`, wasting tokens on project re-reads. An earlier iteration + of this secondary (mtime-only: newest rollout with mtime > + `CODEX_SESSIONS_BEFORE`) was rejected in Round 6 of adversarial review + because it binds on timing alone: a parallel codex invocation running + during the exec window creates a newer rollout, which the fallback + then silently picks — Step 7's post-resume checks (`§4.8`) see a + normally-shaped wrong-session response and the skill applies fixes + informed by an unrelated artifact. Positive content-binding via the + session marker eliminates this entirely: only rollouts containing our + specific `REVIEW_ID` pass the grep filter. - **Alternatives considered.** - *Keep parsing `session id:` from stderr.* Rejected: Bash tool - truncates output at ~30 KB from the head (§3.1); long reasoning + 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, but — honestly — NOT - auto-detectable by the skill: a wrong-session resume returns a - normally-shaped review (VERDICT + `[severity:` markers), so the - Step 7 post-resume checks (`§4.8`) pass and the skill applies - fixes based on an unrelated artifact. The `CODEX_SESSIONS_BEFORE` - timestamp narrows the race window to seconds, but does not - eliminate it. Risk is acknowledged in `SKILL.md` Step 4 check 4 - ("Parallel-codex caveat") as a silent-corruption hazard, not a - fallback-handled hazard. + - *Redirect stderr to a file, Read via Read tool.* Rejected: since + `§4.1a` covers the reference env cleanly and `§4.1b` covers the + sandboxed env, adding a third path is not worth the complexity. + - *Newest-rollout-by-mtime (timestamp-only bind).* Rejected in + Round 6: parallel codex invocation race produces silent + wrong-session corruption (details in `§6.6`). Superseded by + positive content-bind. + - *Write a dedicated marker file on disk (e.g., + `/tmp/codex-start-${REVIEW_ID}.marker`) and grep rollouts for that + file's path.* Rejected: adds another temp-file artifact to manage + and clean up. The prompt file is already written for the launch + and can serve as both the `-newer` anchor and (via embedded + marker) the grep target — no new file needed. + - *Embed `REVIEW_ID` as an XML element inside the prompt rather + than as an HTML comment.* Rejected: a prompt-level XML element + could interfere with the reviewer's parsing or be surfaced in + the reviewer's response as if it were content to address. An + HTML-style comment at the top is unambiguous metadata to any + reader and survives intact in the rollout JSONL where grep sees + it. - *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. +- **Chosen because.** Primary is cheap and documented on the Codex side + (the `thread.started` event is in the CLI contract). Secondary is + positively-bound: zero ambiguity between our rollout and any other. + Together they cover both observed environments without a silent- + corruption risk from parallel codex. - **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 (not eliminated) by - the pre-exec timestamp `CODEX_SESSIONS_BEFORE` (computed by the - lead in-reasoning as "current Unix timestamp minus 1" and - substituted as a literal integer — no Bash call), narrowing the - window to "files created within ~1-2 seconds of the exec start". - The `-1` shift against `-newermt`'s strict-greater semantics - prevents same-epoch miss; the race window is one second wider as - a result, still negligible compared to a real codex exec - duration. + - Every prompt now has a leading HTML-comment line. Reviewer sees + it but ignores (Codex treats it as non-instructional content). - Session-id capture happens only after review-file sanity passes - AND only when verdict is `REVISE` (Step 4 check order in - `SKILL.md`). This avoids aborting a valid round-1 APPROVED over - a secondary-capture failure: APPROVED means no resume, no - session-id needed. - - Extra permission surface: `Bash(find ...)` is now in the + AND only when verdict is `REVISE` (Step 4 check order). This + avoids aborting a valid round-1 APPROVED over a secondary + failure: APPROVED means no resume, no session-id needed. + - Extra permission surface: `Bash(find ~/.codex/sessions*)` in the recommended permissions list. ### §4.2. Capture `REPO_ROOT` at Step 2, substitute literally @@ -651,10 +671,9 @@ Each decision below follows the same template: and a valid APPROVED review completes without depending on session-id capture. An earlier draft ordered session-id *before* review-sanity, which meant a secondary-capture failure (e.g., - empty `~/.codex/sessions/` on a first-ever codex run, or a super- - fast codex exec hitting the `-newermt` same-epoch edge) would - abort an otherwise-successful APPROVED round. The current order - avoids that. + empty `~/.codex/sessions/` on a first-ever codex run, or a rollout + that somehow lacked the session marker) would abort an otherwise- + successful APPROVED round. The current order avoids that. - **Alternatives considered.** - *Ad-hoc checks in whatever order.* Rejected: invites null-pointer- style crashes on missing files. @@ -973,6 +992,55 @@ 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. +### §6.7. 2026-04-17 (Round 6): Silent wrong-session corruption from timestamp-only fallback + +**Claim trajectory.** Rounds 1-5 of development converged on a two-tier +session-id design where the secondary path identified our rollout as +"newest `rollout-*.jsonl` with mtime greater than a captured pre-exec +timestamp". Rounds 4 and 5 of self-review noted the parallel-codex +hazard but accepted it as a documented limitation: a narrow race +window + `--last` being unused were argued as sufficient mitigation. + +**Reality (Round 6 team review).** A parallel codex invocation in any +shell on the same machine (user running `codex` in another terminal, +a CI job, a hook, etc.) creates a newer rollout during the review's +exec window. The skill's `find -newermt + pick newest` then captures +that unrelated UUID. `codex exec resume ` succeeds against that +thread, returns a normally-shaped review (`VERDICT:`, `[severity:`) +for an unrelated artifact, and Step 7's post-resume checks pass. The +skill applies "fixes" informed by a review of some other work. + +Neither the narrow window nor the absence of `--last` actually +closes this: a parallel codex starting even seconds after the skill's +exec still qualifies for the window, and not using `--last` does not +help because the fallback explicitly picks newest-by-mtime anyway. + +**Root cause of the misdiagnosis.** Both self-reviews underestimated +the likelihood of parallel codex (operators running `codex` in a side +terminal is common during development), and both treated the narrow +timing window as equivalent to "safe enough". The reviewer recommended +fail-closed unless a rollout can be positively bound to this launch. + +**Mitigation.** Replaced the timestamp-only secondary with positive +content-binding (`§4.1b`): every prompt embeds +`` as its first line, +and the fallback uses `find -newer -exec grep -l +'...${REVIEW_ID}' {} +`. Only rollouts whose transcript contains our +specific `REVIEW_ID` pass the grep; everything else (including any +parallel codex's rollout) is invisible. Zero matches → fail closed. + +As a side benefit, all flags used are POSIX (`-newer FILE`, `-exec +CMD {} +`, `grep -l`) — the GNU-find dependency documented as a +known limitation in the prior iteration of `§9.5` went away. + +**Lesson (augmenting §6.5).** When documenting a "narrow window" +mitigation, ask: what is the failure mode *when* the race fires, and +how would the skill know? If the answer is "silent incorrect output +that passes the skill's own sanity checks", the mitigation is +insufficient regardless of how narrow the window is. Positive binding +by content (not by timing) is the correct answer; fail-closed on +no-match is the correct default. + --- ## §7. Smoke test protocol @@ -989,8 +1057,8 @@ the repo root. Expected outputs are in comments. ```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' +cat > /tmp/codex-prompt-${REVIEW_ID}.md < You are a senior adversarial reviewer of implementation plans. @@ -1015,11 +1083,13 @@ head -1 /tmp/codex-stdout-${REVIEW_ID}.jsonl # reference env: thread. 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. +# Verify the filesystem secondary path also works (§4.1b) — positive content-bind. +# Returns the rollout path that both postdates our prompt file AND contains the marker. +find ~/.codex/sessions -name 'rollout-*.jsonl' \ + -newer /tmp/codex-prompt-${REVIEW_ID}.md \ + -exec grep -l "ADVERSARIAL-REVIEW-SESSION: ${REVIEW_ID}" {} + 2>/dev/null +# expect: exactly one path. Extract the UUID from basename — it must equal the +# thread_id from the primary path above (if the primary was populated). ``` ### §7.2. Resume with cd prefix @@ -1030,20 +1100,22 @@ Continuing from §7.1 — extract the thread id and resume. # Primary session-id capture (may be empty in affected sandboxes) THREAD_ID=$(head -1 /tmp/codex-stdout-${REVIEW_ID}.jsonl \ | grep -oE '"thread_id":"[^"]+"' | cut -d'"' -f4) -# Secondary: rollout-filename UUID (always works) +# Secondary: positive content-bind (§4.1b). POSIX-portable. if [ -z "${THREAD_ID}" ]; then - THREAD_ID=$(find ~/.codex/sessions -name 'rollout-*.jsonl' \ - -newermt "@${CODEX_SESSIONS_BEFORE}" 2>/dev/null \ - | sort | tail -1 | xargs -r -n1 basename \ + ROLLOUT=$(find ~/.codex/sessions -name 'rollout-*.jsonl' \ + -newer /tmp/codex-prompt-${REVIEW_ID}.md \ + -exec grep -l "ADVERSARIAL-REVIEW-SESSION: ${REVIEW_ID}" {} + 2>/dev/null \ + | head -1) + THREAD_ID=$(basename "${ROLLOUT}" .jsonl \ | grep -oE '[0-9a-f]{8}(-[0-9a-f]{4}){3}-[0-9a-f]{12}') fi echo "THREAD_ID=${THREAD_ID}" # expect a UUID -cat > /tmp/codex-resume-prompt-${REVIEW_ID}.md <<'EOF' +cat > /tmp/codex-resume-prompt-${REVIEW_ID}.md < Still there? Reply with VERDICT: APPROVED. EOF -CODEX_SESSIONS_BEFORE=$(date +%s) cd "${REPO_ROOT}" && cat /tmp/codex-resume-prompt-${REVIEW_ID}.md \ | timeout 300 codex exec resume --json \ "${THREAD_ID}" \ @@ -1142,6 +1214,7 @@ If §7.1–§7.5 do not produce the expected outputs: |------|-----------|-------------|----------|-------| | 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`. | +| 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. | When you re-verify (either during routine maintenance or when triggered by §7.7), add a row. Keep the log chronological. @@ -1199,25 +1272,23 @@ 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. GNU find dependency in the filesystem session-id fallback +### §9.5. macOS not end-to-end tested -The secondary session-id capture (`§4.1b`) uses `find -newermt "@"` -and `-printf`, both GNU extensions. On macOS (BSD `find`) the commands -do not accept these flags. The skill does not detect the platform and -does not translate commands automatically. +The secondary session-id capture (`§4.1b`) uses only POSIX find flags +(`-newer FILE`, `-exec CMD {} +`) and POSIX `grep -l`, so it should +work identically on macOS as on Linux. However, the skill has not +been end-to-end tested on macOS. Edge cases that may differ: -Mitigation today: `SKILL.md` Step 4 check 4 includes a one-paragraph -platform note that states the *goal* of the command ("list rollout -files modified since `CODEX_SESSIONS_BEFORE`, pick newest, extract -UUID from filename") and invites the operator or the lead to substitute -an equivalent BSD-compatible command (`find ... -type f` + `stat -f -'%m %N'`, or `ls -t ... | head -1` against a reference marker file). -This is a "template + understanding" approach: rely on the lead's -adaptability rather than branching the skill for every platform. +- Default shell (zsh on modern macOS vs bash on Linux) — the skill's + Bash-tool commands do not rely on bash-specific features (the + `cat | pipe` form is POSIX), so this is unlikely to matter. +- `~/.codex/sessions` layout — expected identical on both platforms + (codex-cli is cross-platform). +- Permission prompts for `find ~/.codex/sessions*` — should match + the pattern on any Claude Code harness. -If BSD support ever becomes load-bearing (a macOS-running user base, -a CI on macOS runners), this can be upgraded to a bundled portable -command variant or a platform-detection branch. +If a macOS user reports breakage, add findings to `§6` and file a +version-log row in `§8`. ### §9.6. No automated tests