diff --git a/CHANGELOG.md b/CHANGELOG.md index f2c2cd42..019eb90b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,16 @@ 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.16.4 + +#### Fixed +- `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-review v3.10.3 #### Fixed diff --git a/plugins/code/.claude-plugin/plugin.json b/plugins/code/.claude-plugin/plugin.json index 2c33de2f..dd13f718 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.16.3", + "version": "1.16.4", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" 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/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 4f6f6bb1..1bd1226e 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 ────────────────────────────────────────────────── @@ -256,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() { @@ -263,13 +283,31 @@ 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 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" } 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` (removed in codex-cli 0.147) or +# `-s` (rejected by `codex exec resume`), so one arg set serves both calls. +# 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 @@ -292,7 +330,8 @@ if [[ -n "$SESSION_ID" ]]; then # Resume succeeded -- skip to verdict extraction : else - echo "Codex session resume failed, 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" @@ -330,7 +369,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_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" exit 0 @@ -338,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" @@ -354,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 new file mode 100644 index 00000000..2b1271dc --- /dev/null +++ b/plugins/code/tools/python/test_run_codex_review.py @@ -0,0 +1,261 @@ +"""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 +error on stderr. FAKE_CODEX_MODE selects a failure shape. +""" + +import json +import os +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 + +args = sys.argv[1:] +with open(os.environ["FAKE_CODEX_ARGV_LOG"], "a") as log: + log.write(json.dumps(args) + "\n") +mode = os.environ.get("FAKE_CODEX_MODE") +resuming = args[:2] == ["exec", "resume"] + +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) + +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 resuming and ("-s" in args or "--sandbox" in args): + reject("-s" if "-s" in args else "--sandbox") +if mode == "reject-json": + reject("--json") + +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") +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 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) + return { + **os.environ, + "HOME": str(tmp_path), + "PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", + "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", + 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, + ) + 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, codex_calls(tmp_path) + + +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") + + 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] + 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: + 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] + 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: + 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 ( + "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] + assert has_pair(calls[1], "-c", "approval_policy=never"), calls[1] + + +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: + assert failed_token(tmp_path, "stderr-errors") == ( + "CODEX_FAILED:codex exited with code 1: Error: failed to load config.toml" + ) + + +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." + ) + + +def test_codex_failed_does_not_report_the_stdin_banner_as_the_cause( + tmp_path: Path, +) -> None: + assert ( + failed_token(tmp_path, "banner-only") == "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." + ) + + +@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 + )