From 79e42e6d03f73ea41be99427ff4a16a821167e26 Mon Sep 17 00:00:00 2001 From: Peter Ulsteen Date: Tue, 29 Sep 2026 08:46:43 -0500 Subject: [PATCH 1/4] fix(code): make codex-review run on current codex-cli (ISS-11082) - Replace --full-auto, which codex-cli 0.154+ rejects with exit 2, with -c sandbox_mode=read-only in run_codex_review.sh. Both `codex exec` and `codex exec resume` accept it; -s does not work on resume. - Capture codex stderr instead of discarding it. CODEX_FAILED now carries one stderr line after the exit code, the full stderr goes to the script's stderr, and a failed resume names its cause. - Add test_run_codex_review.py, which runs the script against a fake codex that rejects flags the way codex-cli 0.154+ does. - Bump code plugin to 1.15.1. Testing: pytest test_run_codex_review.py (4 passed); real round 1 + resumed round 2 against codex-cli 0.158.0 returned verdicts; flag matrix probed on codex-cli 0.136.0 through 0.158.0. Risks: the reviewer now runs read-only instead of workspace-write. It only reads the plan and code; the script writes the feedback file. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 + plugins/code/.claude-plugin/plugin.json | 2 +- .../codex-review/scripts/run_codex_review.sh | 21 ++- .../tools/python/test_run_codex_review.py | 135 ++++++++++++++++++ 4 files changed, 159 insertions(+), 5 deletions(-) create mode 100644 plugins/code/tools/python/test_run_codex_review.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f9307ea..d4e61038 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to the claude-plugins project will be documented in this fil The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Entries are listed newest-first; each plugin section is treated as released when merged to `main`. +### code v1.15.1 + +#### Fixed +- `codex-review`'s `run_codex_review.sh` no longer passes `--full-auto`, which current codex-cli rejects with exit 2, so every review round failed. It now passes `-c sandbox_mode=read-only`, which both `codex exec` and `codex exec resume` accept; `-s read-only` would have broken every resumed round. +- `run_codex_review.sh` keeps codex's stderr instead of discarding it. `CODEX_FAILED` now carries one line of it after the exit code (the first line naming an error, else the last line, skipping the `Reading ... from stdin...` banner), the full stderr goes to the script's stderr, and a failed session resume names its cause before falling back to a fresh session. + ### code v1.15.0 #### Added diff --git a/plugins/code/.claude-plugin/plugin.json b/plugins/code/.claude-plugin/plugin.json index 0727e36c..88adb090 100644 --- a/plugins/code/.claude-plugin/plugin.json +++ b/plugins/code/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "code", "description": "Code and planning framework plugin", - "version": "1.15.0", + "version": "1.15.1", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" diff --git a/plugins/code/skills/codex-review/scripts/run_codex_review.sh b/plugins/code/skills/codex-review/scripts/run_codex_review.sh index 4f6f6bb1..fb57bc96 100755 --- a/plugins/code/skills/codex-review/scripts/run_codex_review.sh +++ b/plugins/code/skills/codex-review/scripts/run_codex_review.sh @@ -84,6 +84,7 @@ tmp_dir=$(mktemp -d) trap 'rm -rf "$tmp_dir"' EXIT codex_json="$tmp_dir/codex_output.json" +codex_stderr="$tmp_dir/codex_stderr.txt" prompt_file="$tmp_dir/prompt.txt" # ── Build the review prompt ────────────────────────────────────────────────── @@ -263,13 +264,23 @@ run_codex_cmd() { # Log round header printf '\n--- Round %s | %s ---\n' "$ROUND" "$(date -u +%Y-%m-%dT%H:%M:%SZ)" >> "$LOG_FILE" # Tee raw JSON stream to both the capture file and the persistent log - codex "$@" 2>/dev/null | tee -a "$LOG_FILE" > "$json_out" + codex "$@" 2>"$codex_stderr" | tee -a "$LOG_FILE" > "$json_out" +} + +# One line from the last codex run's stderr: the first naming an error, else the +# last. Skips codex's "Reading ... from stdin..." banner, which precedes failures. +codex_stderr_reason() { + local lines + lines=$(tr -d '\r' < "$codex_stderr" 2>/dev/null | grep -v -e '^[[:space:]]*$' -e '^Reading .*stdin\.\.\.$') || true + grep -i -m1 'error' <<<"$lines" || tail -n1 <<<"$lines" } effective_session_id="$SESSION_ID" codex_exit=0 -base_args=(--full-auto --json -m "$CODEX_MODEL" -c model_reasoning_effort=high) +# `-c sandbox_mode=` rather than `--full-auto` (rejected since codex-cli 0.154) +# or `-s` (rejected by `codex exec resume`), so one arg set serves both calls. +base_args=(--json -m "$CODEX_MODEL" -c sandbox_mode=read-only -c model_reasoning_effort=high) prompt_content=$(cat "$prompt_file") # Attempt session resume if we have a prior session ID @@ -292,7 +303,7 @@ if [[ -n "$SESSION_ID" ]]; then # Resume succeeded -- skip to verdict extraction : else - echo "Codex session resume failed, starting fresh session..." >&2 + echo "Codex session resume failed ($(codex_stderr_reason)), starting fresh session..." >&2 effective_session_id="" rm -f "$codex_json" @@ -330,7 +341,9 @@ feedback_content=$(cat "$FEEDBACK_FILE" 2>/dev/null || echo "") # Handle failures if [[ $codex_exit -ne 0 ]] && [[ -z "$feedback_content" ]]; then - echo "CODEX_FAILED:codex exited with code $codex_exit" + cat "$codex_stderr" >&2 2>/dev/null || true + reason=$(codex_stderr_reason) + echo "CODEX_FAILED:codex exited with code $codex_exit${reason:+: $reason}" echo "CODEX_SESSION:${effective_session_id:-none}" echo "LOG_ID:$LOG_ID" exit 0 diff --git a/plugins/code/tools/python/test_run_codex_review.py b/plugins/code/tools/python/test_run_codex_review.py new file mode 100644 index 00000000..05210692 --- /dev/null +++ b/plugins/code/tools/python/test_run_codex_review.py @@ -0,0 +1,135 @@ +"""Tests for skills/codex-review/scripts/run_codex_review.sh against a fake codex. + +The fake rejects arguments the way codex-cli 0.154+ does: `--full-auto` on any +subcommand, and `-s`/`--sandbox` on `exec resume`, each with exit 2 and a clap +error on stderr. +""" + +import json +import os +import subprocess +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[4] +SCRIPT = REPO_ROOT / "plugins/code/skills/codex-review/scripts/run_codex_review.sh" + +FAKE_CODEX = r"""#!/usr/bin/env python3 +import json, os, sys + +args = sys.argv[1:] +with open(os.environ["FAKE_CODEX_ARGV_LOG"], "a") as log: + log.write(json.dumps(args) + "\n") + +sys.stderr.write("Reading additional input from stdin...\n") + +def reject(flag): + sys.stderr.write( + f"error: unexpected argument '{flag}' found\n\n" + f" tip: to pass '{flag}' as a value, use '-- {flag}'\n\n" + "Usage: codex exec [OPTIONS] [PROMPT]\n\n" + "For more information, try '--help'.\n" + ) + sys.exit(2) + +if "--full-auto" in args: + reject("--full-auto") +if args[:2] == ["exec", "resume"]: + for flag in ("-s", "--sandbox"): + if flag in args: + reject(flag) + +mode = os.environ.get("FAKE_CODEX_MODE") +if mode == "reject-json": + reject("--json") +if mode == "runtime-error": + sys.stderr.write("Not inside a trusted directory and --skip-git-repo-check was not specified.\n") + sys.exit(1) + +print(json.dumps({"type": "thread.started", "thread_id": "thread-new"})) +print(json.dumps({"type": "item.completed", "item": {"type": "agent_message", "text": "VERDICT: APPROVED"}})) +""" + + +def run_review( + tmp_path: Path, *extra: str, mode: str = "" +) -> tuple[subprocess.CompletedProcess[str], list[list[str]]]: + bin_dir = tmp_path / "bin" + bin_dir.mkdir(exist_ok=True) + fake = bin_dir / "codex" + fake.write_text(FAKE_CODEX) + fake.chmod(0o755) + plan = tmp_path / "plan.md" + plan.write_text("# Plan\n") + argv_log = tmp_path / "argv.jsonl" + env = { + **os.environ, + "HOME": str(tmp_path), + "PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", + "FAKE_CODEX_ARGV_LOG": str(argv_log), + "FAKE_CODEX_MODE": mode, + } + result = subprocess.run( + [ + "bash", + str(SCRIPT), + "--plan-file", + str(plan), + "--feedback-file", + str(tmp_path / "feedback.txt"), + *extra, + ], + text=True, + capture_output=True, + check=False, + env=env, + stdin=subprocess.DEVNULL, + ) + calls = ( + [json.loads(line) for line in argv_log.read_text().splitlines()] + if argv_log.exists() + else [] + ) + return result, calls + + +def has_pair(args: list[str], flag: str, value: str) -> bool: + return any(args[i] == flag and args[i + 1] == value for i in range(len(args) - 1)) + + +def test_first_round_runs_codex_exec_read_only(tmp_path: Path) -> None: + result, calls = run_review(tmp_path, "--round", "1") + + assert result.stdout.splitlines()[0] == "VERDICT:APPROVED", result.stdout + assert len(calls) == 1 + assert calls[0][0] == "exec" + assert has_pair(calls[0], "-c", "sandbox_mode=read-only"), calls[0] + + +def test_resumed_round_runs_codex_exec_resume_read_only(tmp_path: Path) -> None: + result, calls = run_review(tmp_path, "--round", "2", "--session-id", "thread-old") + + assert result.stdout.splitlines()[0] == "VERDICT:APPROVED", result.stdout + # One call means the resume itself succeeded, not the fresh-session fallback. + assert len(calls) == 1 + assert calls[0][:3] == ["exec", "resume", "thread-old"] + assert has_pair(calls[0], "-c", "sandbox_mode=read-only"), calls[0] + + +def test_codex_failed_carries_a_cli_rejection(tmp_path: Path) -> None: + result, _ = run_review(tmp_path, "--round", "1", mode="reject-json") + + assert ( + result.stdout.splitlines()[0] + == "CODEX_FAILED:codex exited with code 2: error: unexpected argument '--json' found" + ) + + +def test_codex_failed_carries_the_last_stderr_line_when_none_names_an_error( + tmp_path: Path, +) -> None: + result, _ = run_review(tmp_path, "--round", "1", mode="runtime-error") + + assert result.stdout.splitlines()[0] == ( + "CODEX_FAILED:codex exited with code 1: " + "Not inside a trusted directory and --skip-git-repo-check was not specified." + ) From d43f3ef5ed8bf548bf426bd2c5e6b8874daea921 Mon Sep 17 00:00:00 2001 From: Peter Ulsteen Date: Tue, 29 Sep 2026 08:47:15 -0500 Subject: [PATCH 2/4] fix(code): pin that codex's stdin banner is not reported as a cause (ISS-11082) - Add a codex-review test where codex exits 1 with only its 'Reading additional input from stdin...' banner on stderr, as it does when an API error arrives in the JSON stream. Testing: pytest test_run_codex_review.py (5 passed) Risks: None identified Co-Authored-By: Claude Opus 5.5 --- plugins/code/tools/python/test_run_codex_review.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/plugins/code/tools/python/test_run_codex_review.py b/plugins/code/tools/python/test_run_codex_review.py index 05210692..c15b3b6f 100644 --- a/plugins/code/tools/python/test_run_codex_review.py +++ b/plugins/code/tools/python/test_run_codex_review.py @@ -44,6 +44,8 @@ def reject(flag): if mode == "runtime-error": sys.stderr.write("Not inside a trusted directory and --skip-git-repo-check was not specified.\n") sys.exit(1) +if mode == "banner-only": + sys.exit(1) print(json.dumps({"type": "thread.started", "thread_id": "thread-new"})) print(json.dumps({"type": "item.completed", "item": {"type": "agent_message", "text": "VERDICT: APPROVED"}})) @@ -133,3 +135,11 @@ def test_codex_failed_carries_the_last_stderr_line_when_none_names_an_error( "CODEX_FAILED:codex exited with code 1: " "Not inside a trusted directory and --skip-git-repo-check was not specified." ) + + +def test_codex_failed_does_not_report_the_stdin_banner_as_the_cause( + tmp_path: Path, +) -> None: + result, _ = run_review(tmp_path, "--round", "1", mode="banner-only") + + assert result.stdout.splitlines()[0] == "CODEX_FAILED:codex exited with code 1" From 2e01f2db597c543ab9133072aef575c74eb4a25e Mon Sep 17 00:00:00 2001 From: Peter Ulsteen Date: Tue, 29 Sep 2026 09:03:44 -0500 Subject: [PATCH 3/4] fix(code): name the real cause in CODEX_FAILED (ISS-11082) - Read turn failures from codex's JSON stream first; codex writes them to stdout under --json, not to stderr. - Match stderr lines that start with 'error', so an ERROR tracing line is not reported as the cause; keep the last-line fallback. - Omit the empty parentheses when a failed resume has no reason. - Print the reason in debate-loop.sh with printf '%s', not echo -e. - Correct the removal version to codex-cli 0.147 (#36054). - Tests: resume fallback, one-line token, last-line fallback, anchored match, JSON-stream turn failure. Testing: pytest test_run_codex_review.py (8 passed) Risks: None identified Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 +- plugins/code/scripts/debate-loop.sh | 2 +- .../codex-review/scripts/run_codex_review.sh | 44 ++++++-- .../tools/python/test_run_codex_review.py | 105 +++++++++++++----- 4 files changed, 118 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d4e61038..dad51f3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### code v1.15.1 #### Fixed -- `codex-review`'s `run_codex_review.sh` no longer passes `--full-auto`, which current codex-cli rejects with exit 2, so every review round failed. It now passes `-c sandbox_mode=read-only`, which both `codex exec` and `codex exec resume` accept; `-s read-only` would have broken every resumed round. -- `run_codex_review.sh` keeps codex's stderr instead of discarding it. `CODEX_FAILED` now carries one line of it after the exit code (the first line naming an error, else the last line, skipping the `Reading ... from stdin...` banner), the full stderr goes to the script's stderr, and a failed session resume names its cause before falling back to a fresh session. +- `codex-review`'s `run_codex_review.sh` no longer passes `--full-auto`, which codex-cli 0.147 removed, so every review round exited 2. It now passes `-c sandbox_mode=read-only`, which both `codex exec` and `codex exec resume` accept; `-s read-only` would have broken every resumed round. The reviewer's sandbox narrows from `--full-auto`'s workspace-write to read-only. +- `run_codex_review.sh` keeps codex's stderr instead of discarding it. `CODEX_FAILED` now carries one line after the exit code saying why codex failed: the last `turn.failed` (else `error`) message from the JSON stream, otherwise the first stderr line starting with `error`, otherwise the last stderr line, skipping the `Reading ... from stdin...` banner. The full stderr goes to the script's stderr, and a failed session resume names its cause before falling back to a fresh session. +- `debate-loop.sh` prints the `CODEX_FAILED` reason with `printf '%s'` instead of `echo -e`, so backslashes in codex's message are printed as-is instead of being read as escapes. ### code v1.15.0 diff --git a/plugins/code/scripts/debate-loop.sh b/plugins/code/scripts/debate-loop.sh index 577fbcb5..2a5eb038 100755 --- a/plugins/code/scripts/debate-loop.sh +++ b/plugins/code/scripts/debate-loop.sh @@ -501,7 +501,7 @@ while [[ $round -le $MAX_ROUNDS ]]; do # Handle failures and empty responses if [[ "$CODEX_VERDICT" == FAILED:* ]]; then - echo -e "${RED}Error: Codex failed: ${CODEX_VERDICT#FAILED:}${NC}" >&2 + printf '%bError: Codex failed: %s%b\n' "$RED" "${CODEX_VERDICT#FAILED:}" "$NC" >&2 exit 1 fi diff --git a/plugins/code/skills/codex-review/scripts/run_codex_review.sh b/plugins/code/skills/codex-review/scripts/run_codex_review.sh index fb57bc96..69059964 100755 --- a/plugins/code/skills/codex-review/scripts/run_codex_review.sh +++ b/plugins/code/skills/codex-review/scripts/run_codex_review.sh @@ -257,6 +257,25 @@ sys.stdout.write('\n'.join(lines)) " "$json_file" > "$output_file" 2>/dev/null } +# Extract the failure message from the JSON stream: the last turn.failed error, +# else the last top-level error event. Prints it on one line, or nothing. +parse_failure_message() { + python3 -c " +import json, sys +turn = err = '' +for line in open(sys.argv[1]): + try: + e = json.loads(line.strip()) + if e.get('type') == 'turn.failed': + turn = e['error']['message'] or turn + elif e.get('type') == 'error': + err = e['message'] or err + except Exception: + pass +print(' '.join(str(turn or err).split())) +" "$1" 2>/dev/null || true +} + # ── Run codex ──────────────────────────────────────────────────────────────── run_codex_cmd() { @@ -267,19 +286,25 @@ run_codex_cmd() { codex "$@" 2>"$codex_stderr" | tee -a "$LOG_FILE" > "$json_out" } -# One line from the last codex run's stderr: the first naming an error, else the -# last. Skips codex's "Reading ... from stdin..." banner, which precedes failures. -codex_stderr_reason() { - local lines +# One line saying why the last codex run failed. Turn failures arrive in the JSON +# stream; CLI and config errors only on stderr, where the first line starting with +# "error" wins, else the last line, skipping the "Reading ... stdin..." banner. +codex_failure_reason() { + local reason lines + reason=$(parse_failure_message "$codex_json") + if [[ -n "$reason" ]]; then + echo "$reason" + return + fi lines=$(tr -d '\r' < "$codex_stderr" 2>/dev/null | grep -v -e '^[[:space:]]*$' -e '^Reading .*stdin\.\.\.$') || true - grep -i -m1 'error' <<<"$lines" || tail -n1 <<<"$lines" + grep -i -m1 '^error' <<<"$lines" || tail -n1 <<<"$lines" } effective_session_id="$SESSION_ID" codex_exit=0 -# `-c sandbox_mode=` rather than `--full-auto` (rejected since codex-cli 0.154) -# or `-s` (rejected by `codex exec resume`), so one arg set serves both calls. +# `-c sandbox_mode=` rather than `--full-auto` (removed in codex-cli 0.147) or +# `-s` (rejected by `codex exec resume`), so one arg set serves both calls. base_args=(--json -m "$CODEX_MODEL" -c sandbox_mode=read-only -c model_reasoning_effort=high) prompt_content=$(cat "$prompt_file") @@ -303,7 +328,8 @@ if [[ -n "$SESSION_ID" ]]; then # Resume succeeded -- skip to verdict extraction : else - echo "Codex session resume failed ($(codex_stderr_reason)), starting fresh session..." >&2 + resume_reason=$(codex_failure_reason) + echo "Codex session resume failed${resume_reason:+ ($resume_reason)}, starting fresh session..." >&2 effective_session_id="" rm -f "$codex_json" @@ -342,7 +368,7 @@ feedback_content=$(cat "$FEEDBACK_FILE" 2>/dev/null || echo "") # Handle failures if [[ $codex_exit -ne 0 ]] && [[ -z "$feedback_content" ]]; then cat "$codex_stderr" >&2 2>/dev/null || true - reason=$(codex_stderr_reason) + reason=$(codex_failure_reason) echo "CODEX_FAILED:codex exited with code $codex_exit${reason:+: $reason}" echo "CODEX_SESSION:${effective_session_id:-none}" echo "LOG_ID:$LOG_ID" diff --git a/plugins/code/tools/python/test_run_codex_review.py b/plugins/code/tools/python/test_run_codex_review.py index c15b3b6f..97604bb3 100644 --- a/plugins/code/tools/python/test_run_codex_review.py +++ b/plugins/code/tools/python/test_run_codex_review.py @@ -1,8 +1,8 @@ """Tests for skills/codex-review/scripts/run_codex_review.sh against a fake codex. -The fake rejects arguments the way codex-cli 0.154+ does: `--full-auto` on any +The fake rejects arguments the way codex-cli 0.147+ does: `--full-auto` on any subcommand, and `-s`/`--sandbox` on `exec resume`, each with exit 2 and a clap -error on stderr. +error on stderr. FAKE_CODEX_MODE selects a failure shape. """ import json @@ -19,8 +19,8 @@ args = sys.argv[1:] with open(os.environ["FAKE_CODEX_ARGV_LOG"], "a") as log: log.write(json.dumps(args) + "\n") - -sys.stderr.write("Reading additional input from stdin...\n") +mode = os.environ.get("FAKE_CODEX_MODE") +resuming = args[:2] == ["exec", "resume"] def reject(flag): sys.stderr.write( @@ -31,24 +31,38 @@ def reject(flag): ) sys.exit(2) +def fail(*stderr_lines): + sys.stderr.write("".join(line + "\n" for line in stderr_lines)) + sys.exit(1) + +def emit(event): + print(json.dumps(event)) + if "--full-auto" in args: reject("--full-auto") -if args[:2] == ["exec", "resume"]: - for flag in ("-s", "--sandbox"): - if flag in args: - reject(flag) - -mode = os.environ.get("FAKE_CODEX_MODE") +if resuming and ("-s" in args or "--sandbox" in args): + reject("-s" if "-s" in args else "--sandbox") if mode == "reject-json": reject("--json") -if mode == "runtime-error": - sys.stderr.write("Not inside a trusted directory and --skip-git-repo-check was not specified.\n") - sys.exit(1) -if mode == "banner-only": - sys.exit(1) -print(json.dumps({"type": "thread.started", "thread_id": "thread-new"})) -print(json.dumps({"type": "item.completed", "item": {"type": "agent_message", "text": "VERDICT: APPROVED"}})) +banner = "Reading additional input from stdin..." +tracing = "2026-09-29T00:00:00.000000Z ERROR codex_core::mcp: MCP client for `docs` failed to start" +if mode == "stderr-errors": + fail(banner, tracing, "Error: failed to load config.toml", "error: second error line") +if mode == "untrusted-dir": + fail(banner, "WARNING: a notice", "Not inside a trusted directory and --skip-git-repo-check was not specified.") +if mode == "banner-only": + fail(banner) +if mode == "turn-failed": + emit({"type": "thread.started", "thread_id": "thread-new"}) + emit({"type": "error", "message": "Reconnecting... 1/5"}) + emit({"type": "turn.failed", "error": {"message": "The 'bogus' model is not supported."}}) + fail(banner, tracing) +if mode == "resume-fails" and resuming: + fail(banner, "Error: no session thread-old") + +emit({"type": "thread.started", "thread_id": "thread-new"}) +emit({"type": "item.completed", "item": {"type": "agent_message", "text": "VERDICT: APPROVED"}}) """ @@ -98,6 +112,16 @@ def has_pair(args: list[str], flag: str, value: str) -> bool: return any(args[i] == flag and args[i + 1] == value for i in range(len(args) - 1)) +def failed_token(tmp_path: Path, mode: str) -> str: + """Run round 1 in ``mode`` and return the CODEX_FAILED line, which must be one line.""" + result, _ = run_review(tmp_path, "--round", "1", mode=mode) + lines = result.stdout.splitlines() + assert len(lines) == 3, result.stdout + assert lines[1].startswith("CODEX_SESSION:") + assert lines[2].startswith("LOG_ID:") + return lines[0] + + def test_first_round_runs_codex_exec_read_only(tmp_path: Path) -> None: result, calls = run_review(tmp_path, "--round", "1") @@ -117,21 +141,42 @@ def test_resumed_round_runs_codex_exec_resume_read_only(tmp_path: Path) -> None: assert has_pair(calls[0], "-c", "sandbox_mode=read-only"), calls[0] -def test_codex_failed_carries_a_cli_rejection(tmp_path: Path) -> None: - result, _ = run_review(tmp_path, "--round", "1", mode="reject-json") +def test_failed_resume_names_its_cause_and_falls_back_to_exec(tmp_path: Path) -> None: + result, calls = run_review( + tmp_path, "--round", "2", "--session-id", "thread-old", mode="resume-fails" + ) + assert result.stdout.splitlines()[:2] == [ + "VERDICT:APPROVED", + "CODEX_SESSION:thread-new", + ] assert ( - result.stdout.splitlines()[0] - == "CODEX_FAILED:codex exited with code 2: error: unexpected argument '--json' found" + "Codex session resume failed (Error: no session thread-old), starting fresh session..." + in result.stderr ) + assert len(calls) == 2 + assert calls[1][0] == "exec" + assert has_pair(calls[1], "-c", "sandbox_mode=read-only"), calls[1] -def test_codex_failed_carries_the_last_stderr_line_when_none_names_an_error( +def test_codex_failed_carries_a_cli_rejection(tmp_path: Path) -> None: + assert failed_token(tmp_path, "reject-json") == ( + "CODEX_FAILED:codex exited with code 2: error: unexpected argument '--json' found" + ) + + +def test_codex_failed_carries_the_first_line_starting_with_error( tmp_path: Path, ) -> None: - result, _ = run_review(tmp_path, "--round", "1", mode="runtime-error") + assert failed_token(tmp_path, "stderr-errors") == ( + "CODEX_FAILED:codex exited with code 1: Error: failed to load config.toml" + ) + - assert result.stdout.splitlines()[0] == ( +def test_codex_failed_carries_the_last_stderr_line_when_none_starts_with_error( + tmp_path: Path, +) -> None: + assert failed_token(tmp_path, "untrusted-dir") == ( "CODEX_FAILED:codex exited with code 1: " "Not inside a trusted directory and --skip-git-repo-check was not specified." ) @@ -140,6 +185,14 @@ def test_codex_failed_carries_the_last_stderr_line_when_none_names_an_error( def test_codex_failed_does_not_report_the_stdin_banner_as_the_cause( tmp_path: Path, ) -> None: - result, _ = run_review(tmp_path, "--round", "1", mode="banner-only") + assert ( + failed_token(tmp_path, "banner-only") == "CODEX_FAILED:codex exited with code 1" + ) + - assert result.stdout.splitlines()[0] == "CODEX_FAILED:codex exited with code 1" +def test_codex_failed_carries_the_turn_failure_from_the_json_stream( + tmp_path: Path, +) -> None: + assert failed_token(tmp_path, "turn-failed") == ( + "CODEX_FAILED:codex exited with code 1: The 'bogus' model is not supported." + ) From ec114d8753ac6bb220f51703cc059902ce97a112 Mon Sep 17 00:00:00 2001 From: Peter Ulsteen Date: Thu, 1 Oct 2026 17:21:51 -0500 Subject: [PATCH 4/4] fix(code): pin codex approval and surface stderr (ISS-11082) - run_codex_review.sh passes -c approval_policy=never. codex exec defaults approval to never but drops that default when the user's config sets approvals_reviewer = "auto_review" (exec/src/lib.rs build_exec_config at rust-v0.159.3). - run_codex_review.sh writes codex's stderr to its own stderr on both CODEX_EMPTY paths, as it already does for CODEX_FAILED. - hooks/plan-review.sh replaces --full-auto with -c sandbox_mode=read-only -c approval_policy=never and appends codex's stderr to its debug log instead of /dev/null. - Tests: approval pair on exec, resume and fallback calls; stderr on both CODEX_EMPTY paths; hook argv and hook stderr logging. Testing: pytest test_run_codex_review.py (12 passed); each new assertion fails when its code change is reverted. Real codex-cli 0.159.3 and 0.154.0 runs report approval: on-request with an auto_review config and approval: never with the new flag. Risks: None identified Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 + plugins/code/hooks/plan-review.sh | 2 +- .../codex-review/scripts/run_codex_review.sh | 6 +- .../tools/python/test_run_codex_review.py | 91 ++++++++++++++++--- 4 files changed, 86 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f573160f..05704036 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - `codex-review`'s `run_codex_review.sh` no longer passes `--full-auto`, which codex-cli 0.147 removed, so every review round exited 2. It now passes `-c sandbox_mode=read-only`, which both `codex exec` and `codex exec resume` accept; `-s read-only` would have broken every resumed round. The reviewer's sandbox narrows from `--full-auto`'s workspace-write to read-only. - `run_codex_review.sh` keeps codex's stderr instead of discarding it. `CODEX_FAILED` now carries one line after the exit code saying why codex failed: the last `turn.failed` (else `error`) message from the JSON stream, otherwise the first stderr line starting with `error`, otherwise the last stderr line, skipping the `Reading ... from stdin...` banner. The full stderr goes to the script's stderr, and a failed session resume names its cause before falling back to a fresh session. - `debate-loop.sh` prints the `CODEX_FAILED` reason with `printf '%s'` instead of `echo -e`, so backslashes in codex's message are printed as-is instead of being read as escapes. +- `run_codex_review.sh` also passes `-c approval_policy=never`. `codex exec` defaults approval to `never` but drops that default when the user's config sets `approvals_reviewer = "auto_review"`, which left the user's `approval_policy` in effect. +- `run_codex_review.sh` writes codex's stderr to the script's stderr when it reports `CODEX_EMPTY`, as it already does for `CODEX_FAILED`. +- `hooks/plan-review.sh` passes `-c sandbox_mode=read-only -c approval_policy=never` instead of `--full-auto`, and appends codex's stderr to its debug log instead of discarding it. ### code v1.16.3 diff --git a/plugins/code/hooks/plan-review.sh b/plugins/code/hooks/plan-review.sh index 97e2e7b3..3c61f683 100755 --- a/plugins/code/hooks/plan-review.sh +++ b/plugins/code/hooks/plan-review.sh @@ -57,7 +57,7 @@ EOF # Get Codex's review using stdin to avoid shell escaping issues log "Calling codex exec..." -REVIEW=$(codex exec --full-auto -m "gpt-5.3-codex-spark" < "$TMPFILE" 2>/dev/null) +REVIEW=$(codex exec -c sandbox_mode=read-only -c approval_policy=never -m "gpt-5.3-codex-spark" < "$TMPFILE" 2>>"$LOG_FILE") # If codex failed, exit silently if [ -z "$REVIEW" ]; then diff --git a/plugins/code/skills/codex-review/scripts/run_codex_review.sh b/plugins/code/skills/codex-review/scripts/run_codex_review.sh index 69059964..1bd1226e 100755 --- a/plugins/code/skills/codex-review/scripts/run_codex_review.sh +++ b/plugins/code/skills/codex-review/scripts/run_codex_review.sh @@ -305,7 +305,9 @@ codex_exit=0 # `-c sandbox_mode=` rather than `--full-auto` (removed in codex-cli 0.147) or # `-s` (rejected by `codex exec resume`), so one arg set serves both calls. -base_args=(--json -m "$CODEX_MODEL" -c sandbox_mode=read-only -c model_reasoning_effort=high) +# approval_policy is explicit because exec drops its own `never` default when the +# user's config sets `approvals_reviewer = "auto_review"`. +base_args=(--json -m "$CODEX_MODEL" -c sandbox_mode=read-only -c approval_policy=never -c model_reasoning_effort=high) prompt_content=$(cat "$prompt_file") # Attempt session resume if we have a prior session ID @@ -377,6 +379,7 @@ fi # Handle empty response if [[ -z "$feedback_content" ]]; then + cat "$codex_stderr" >&2 2>/dev/null || true echo "CODEX_EMPTY" echo "CODEX_SESSION:${effective_session_id:-none}" echo "LOG_ID:$LOG_ID" @@ -393,6 +396,7 @@ elif echo "$feedback_content" | grep -q "^### Finding"; then echo "VERDICT:NEEDS_CHANGES" else # No verdict AND no findings -- likely truncated response, not a real review + cat "$codex_stderr" >&2 2>/dev/null || true echo "CODEX_EMPTY" fi diff --git a/plugins/code/tools/python/test_run_codex_review.py b/plugins/code/tools/python/test_run_codex_review.py index 97604bb3..2b1271dc 100644 --- a/plugins/code/tools/python/test_run_codex_review.py +++ b/plugins/code/tools/python/test_run_codex_review.py @@ -1,4 +1,5 @@ -"""Tests for skills/codex-review/scripts/run_codex_review.sh against a fake codex. +"""Tests for skills/codex-review/scripts/run_codex_review.sh and hooks/plan-review.sh +against a fake codex. The fake rejects arguments the way codex-cli 0.147+ does: `--full-auto` on any subcommand, and `-s`/`--sandbox` on `exec resume`, each with exit 2 and a clap @@ -10,8 +11,11 @@ import subprocess from pathlib import Path +import pytest + REPO_ROOT = Path(__file__).resolve().parents[4] SCRIPT = REPO_ROOT / "plugins/code/skills/codex-review/scripts/run_codex_review.sh" +HOOK = REPO_ROOT / "plugins/code/hooks/plan-review.sh" FAKE_CODEX = r"""#!/usr/bin/env python3 import json, os, sys @@ -60,30 +64,46 @@ def emit(event): fail(banner, tracing) if mode == "resume-fails" and resuming: fail(banner, "Error: no session thread-old") +if mode in ("empty", "no-verdict"): + sys.stderr.write("WARNING: stream disconnected before completion\n") + emit({"type": "thread.started", "thread_id": "thread-new"}) + if mode == "no-verdict": + emit({"type": "item.completed", "item": {"type": "agent_message", "text": "Looking at the plan"}}) + sys.exit(0) emit({"type": "thread.started", "thread_id": "thread-new"}) emit({"type": "item.completed", "item": {"type": "agent_message", "text": "VERDICT: APPROVED"}}) """ -def run_review( - tmp_path: Path, *extra: str, mode: str = "" -) -> tuple[subprocess.CompletedProcess[str], list[list[str]]]: +def fake_codex_env(tmp_path: Path, mode: str) -> dict[str, str]: bin_dir = tmp_path / "bin" bin_dir.mkdir(exist_ok=True) fake = bin_dir / "codex" fake.write_text(FAKE_CODEX) fake.chmod(0o755) - plan = tmp_path / "plan.md" - plan.write_text("# Plan\n") - argv_log = tmp_path / "argv.jsonl" - env = { + return { **os.environ, "HOME": str(tmp_path), "PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", - "FAKE_CODEX_ARGV_LOG": str(argv_log), + "FAKE_CODEX_ARGV_LOG": str(tmp_path / "argv.jsonl"), "FAKE_CODEX_MODE": mode, } + + +def codex_calls(tmp_path: Path) -> list[list[str]]: + argv_log = tmp_path / "argv.jsonl" + if not argv_log.exists(): + return [] + return [json.loads(line) for line in argv_log.read_text().splitlines()] + + +def run_review( + tmp_path: Path, *extra: str, mode: str = "" +) -> tuple[subprocess.CompletedProcess[str], list[list[str]]]: + env = fake_codex_env(tmp_path, mode) + plan = tmp_path / "plan.md" + plan.write_text("# Plan\n") result = subprocess.run( [ "bash", @@ -100,12 +120,22 @@ def run_review( env=env, stdin=subprocess.DEVNULL, ) - calls = ( - [json.loads(line) for line in argv_log.read_text().splitlines()] - if argv_log.exists() - else [] + return result, codex_calls(tmp_path) + + +def run_plan_review_hook( + tmp_path: Path, mode: str = "" +) -> tuple[subprocess.CompletedProcess[str], list[list[str]]]: + payload = {"cwd": str(tmp_path), "tool_response": {"plan": "# Plan\n"}} + result = subprocess.run( + ["bash", str(HOOK)], + input=json.dumps(payload), + text=True, + capture_output=True, + check=False, + env=fake_codex_env(tmp_path, mode), ) - return result, calls + return result, codex_calls(tmp_path) def has_pair(args: list[str], flag: str, value: str) -> bool: @@ -129,6 +159,7 @@ def test_first_round_runs_codex_exec_read_only(tmp_path: Path) -> None: assert len(calls) == 1 assert calls[0][0] == "exec" assert has_pair(calls[0], "-c", "sandbox_mode=read-only"), calls[0] + assert has_pair(calls[0], "-c", "approval_policy=never"), calls[0] def test_resumed_round_runs_codex_exec_resume_read_only(tmp_path: Path) -> None: @@ -139,6 +170,7 @@ def test_resumed_round_runs_codex_exec_resume_read_only(tmp_path: Path) -> None: assert len(calls) == 1 assert calls[0][:3] == ["exec", "resume", "thread-old"] assert has_pair(calls[0], "-c", "sandbox_mode=read-only"), calls[0] + assert has_pair(calls[0], "-c", "approval_policy=never"), calls[0] def test_failed_resume_names_its_cause_and_falls_back_to_exec(tmp_path: Path) -> None: @@ -157,6 +189,7 @@ def test_failed_resume_names_its_cause_and_falls_back_to_exec(tmp_path: Path) -> assert len(calls) == 2 assert calls[1][0] == "exec" assert has_pair(calls[1], "-c", "sandbox_mode=read-only"), calls[1] + assert has_pair(calls[1], "-c", "approval_policy=never"), calls[1] def test_codex_failed_carries_a_cli_rejection(tmp_path: Path) -> None: @@ -196,3 +229,33 @@ def test_codex_failed_carries_the_turn_failure_from_the_json_stream( assert failed_token(tmp_path, "turn-failed") == ( "CODEX_FAILED:codex exited with code 1: The 'bogus' model is not supported." ) + + +@pytest.mark.parametrize("mode", ["empty", "no-verdict"]) +def test_codex_empty_passes_codex_stderr_through(tmp_path: Path, mode: str) -> None: + result, _ = run_review(tmp_path, "--round", "1", mode=mode) + + assert result.stdout.splitlines()[0] == "CODEX_EMPTY", result.stdout + assert "WARNING: stream disconnected before completion" in result.stderr + + +def test_plan_review_hook_runs_codex_exec_read_only(tmp_path: Path) -> None: + result, calls = run_plan_review_hook(tmp_path) + + assert "VERDICT: APPROVED" in json.loads(result.stdout)["hookSpecificOutput"][ + "additionalContext" + ], result.stdout + assert len(calls) == 1 + assert calls[0][0] == "exec" + assert has_pair(calls[0], "-c", "sandbox_mode=read-only"), calls[0] + assert has_pair(calls[0], "-c", "approval_policy=never"), calls[0] + + +def test_plan_review_hook_logs_codex_stderr(tmp_path: Path) -> None: + result, _ = run_plan_review_hook(tmp_path, mode="stderr-errors") + + assert result.stdout == "" + logs = (tmp_path / ".closedloop-ai/plan-review-logs").glob("*.log") + assert any( + "Error: failed to load config.toml" in log.read_text() for log in logs + )