refactor: switch to --json for session capture, harden fallbacks
Address 10+ findings from two rounds of adversarial review of the
previous Step 4/5/7 design. Major changes:
- Use `codex exec --json` so `thread_id` can be parsed deterministically
from the first JSONL line on stdout (bypasses the ~30KB Bash-tool
truncation that could drop stderr metadata in the old flow).
- Capture REPO_ROOT via `git rev-parse --show-toplevel` at Step 2 and
substitute the absolute path literally. Pin the initial exec with
`-C "${REPO_ROOT}"` and prefix every resume with `cd '${REPO_ROOT}' &&`
because `codex exec resume` has no `-C` flag and inherits cwd from
the invoking shell.
- Drop `resume --last` from the fallback chain (cwd filtering is not
enough to distinguish our session from unrelated parallel codex runs).
- Update CODEX_SESSION_ID only on full success (exit 0, no stderr error
line, review file contains VERDICT and findings on REVISE); rotate
to the resumed session's new thread_id each round.
- Harden the "show review" gate (Step 5 "YOUR NEXT MESSAGE" instruction
and Step 6 precondition check) now that --json stdout no longer leaks
review text into the Bash tool result.
- Add strict check order for launch and resume (exit → stderr → review
file) so we never commit a broken session-id on a half-failed run.
- Replace silent fresh-exec fallback with interactive ask / headless
severity-based decision. Fresh-exec prompt rebuilds prior rounds from
conversation history.
- Bare repo / submodule / shell-hostile paths abort at Step 2 with a
clear message rather than failing silently later.
- Conditional cleanup: keep temp files on abort paths for diagnostics.
- Expand REVIEW_ID random to 8 digits.
README: update permissions (add stdout JSONL read, resume-prompt write,
narrower `cd * && ... codex exec resume *` pattern) and troubleshooting
(NOT VERIFIED outcome, bare repo, submodule).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -105,17 +105,21 @@ chosen config file:
|
||||
|
||||
```jsonc
|
||||
// --- adversarial-review permissions ---
|
||||
// Git: diff, status, branch detection
|
||||
// Git: diff, status, branch detection, repo root, submodule check
|
||||
"Bash(git diff*)",
|
||||
"Bash(git status*)",
|
||||
"Bash(git symbolic-ref*)",
|
||||
"Bash(git rev-parse*)",
|
||||
// Codex: review execution (always wrapped in timeout)
|
||||
// Codex: initial launch (uses -C; no cd prefix needed)
|
||||
"Bash(timeout 600 codex exec *)",
|
||||
// Temp files: prompts, plans, output capture
|
||||
// Codex: resume (needs cd prefix to REPO_ROOT because resume has no -C flag)
|
||||
"Bash(cd * && timeout 600 codex exec resume *)",
|
||||
// Temp files: prompts (initial + resume), plans, review output, JSONL stdout, stderr
|
||||
"Write(/tmp/codex-plan-*)",
|
||||
"Write(/tmp/codex-prompt-*)",
|
||||
"Write(/tmp/codex-resume-prompt-*)",
|
||||
"Read(/tmp/codex-review-*)",
|
||||
"Read(/tmp/codex-stdout-*)",
|
||||
"Read(/tmp/codex-stderr-*)",
|
||||
// Cleanup and output piping
|
||||
"Bash(rm -f /tmp/codex-*)",
|
||||
@@ -135,9 +139,12 @@ chosen config file:
|
||||
"Bash(git symbolic-ref*)",
|
||||
"Bash(git rev-parse*)",
|
||||
"Bash(timeout 600 codex exec *)",
|
||||
"Bash(cd * && timeout 600 codex exec resume *)",
|
||||
"Write(/tmp/codex-plan-*)",
|
||||
"Write(/tmp/codex-prompt-*)",
|
||||
"Write(/tmp/codex-resume-prompt-*)",
|
||||
"Read(/tmp/codex-review-*)",
|
||||
"Read(/tmp/codex-stdout-*)",
|
||||
"Read(/tmp/codex-stderr-*)",
|
||||
"Bash(rm -f /tmp/codex-*)",
|
||||
"Bash(tee *)"
|
||||
@@ -200,9 +207,27 @@ exit code 124, the reviewer did not respond in time. Retry — this is usually
|
||||
transient.
|
||||
|
||||
**Resume fails with session error.**
|
||||
The skill uses `codex exec resume <session-id>` for rounds 2+. If the session
|
||||
expired or the ID was not captured, the skill falls back to a fresh `codex exec`
|
||||
automatically. No action needed.
|
||||
The skill uses `codex exec resume <session-id>` for rounds 2+. On failure
|
||||
(non-zero exit, `thread/resume failed` in stderr, or a malformed review),
|
||||
the skill does NOT silently fall back. In an interactive session it asks
|
||||
whether to run a fresh `codex exec` (higher token cost) or conclude the
|
||||
review as NOT VERIFIED. In headless runs it decides based on the maximum
|
||||
severity of the last successful round's findings: critical/high → fresh
|
||||
exec; medium-only → conclude as NOT VERIFIED.
|
||||
|
||||
**"NOT VERIFIED" result.**
|
||||
The skill applied fixes but the reviewer did not re-verify them (resume
|
||||
failed or the operator chose to conclude). This is not an approval —
|
||||
manually review the applied fixes before merging.
|
||||
|
||||
**Running inside a git submodule.**
|
||||
`git rev-parse --show-toplevel` returns the submodule path, not the parent
|
||||
repo. The skill warns you and scopes the review to the submodule. If you
|
||||
meant to review the parent, invoke the skill from the parent working tree.
|
||||
|
||||
**Bare repository or not inside a work tree.**
|
||||
The skill aborts at Step 2 with a clear message. Run it from inside a
|
||||
git working tree.
|
||||
|
||||
**Plan Mode exits when writing temp files.**
|
||||
In Claude Code Plan Mode, writing to `/tmp` may trigger a permission prompt
|
||||
@@ -215,6 +240,15 @@ review correctness.
|
||||
a permission prompt or cause Plan Mode to exit. Does not affect review correctness.
|
||||
- **`resume` inherits sandbox.** `codex exec resume` does not accept `-s` —
|
||||
sandbox is inherited from the original session (always `read-only`).
|
||||
- **`resume` has no `-C` flag.** The skill captures `REPO_ROOT` via
|
||||
`git rev-parse --show-toplevel` at Step 2 and prefixes every resume with
|
||||
`cd '<REPO_ROOT>' && ...`. This requires paths without single quotes;
|
||||
pathological paths (containing `'`, `"`, `$`, backtick, newline) cause
|
||||
the skill to abort at Step 2.
|
||||
- **Submodule scoping.** When invoked inside a submodule, the review is
|
||||
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.
|
||||
|
||||
## Roadmap
|
||||
|
||||
|
||||
Reference in New Issue
Block a user