From fad28d81ce698e78976a2e7296b5e4e39decbba6 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 14 Aug 2026 19:21:43 -0700 Subject: [PATCH] fix(review,ship): run the codex diff passes under the timeout wrapper (#1036) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `_gstack_codex_timeout_wrapper` added in #1056 was wired into codex/SKILL.md but never into the /review and /ship diff passes, which kept running under a bare 5-minute Bash gate. An unwrapped stall returns no exit code and no output, which downstream reads as "Codex reviewed and found nothing" — a truncated pass silently became a clean bill. Measured on codex-cli 0.145.0: a pass was killed at 287s of a 300s budget mid-tool-call, and the same prompt completed in 336s. Both passes in scripts/resolvers/review.ts (adversarial `codex exec` and the structured `codex review --base` pass) now re-source gstack-codex-probe and run under `_gstack_codex_timeout_wrapper 540`, with the Bash tool gate raised to 600000 ms so the wrapper fires FIRST and a stall surfaces as a diagnosable exit 124. The timeout guidance now says a timed-out pass is MISSING COVERAGE, not a clean result, and points at the run's rollout log under ~/.codex/sessions/ for partial output. The stale "timeout doesn't exist on macOS" claim is gone — the wrapper resolves gtimeout, then timeout, then runs unwrapped, so it is safe without coreutils. Static guards in test/codex-hardening.test.ts pin all three sites (resolver, review/SKILL.md, ship/sections/adversarial.md): both calls wrapped, wrapper budget strictly under the Bash gate, and no reappearance of the macOS claim that steered these call sites away from the wrapper in the first place. The Claude-output path guard in test/gen-skill-docs.test.ts now scrubs ~/.codex/sessions/ (a user-facing Codex CLI path, same class as the ~/.codex/logs/ exemption) before banning Codex host paths. Generated files regenerated via gen:skill-docs; factory golden refreshed. Contributed by @aegixx (PR #2379). Co-Authored-By: Claude Fable 5 --- review/SKILL.md | 18 +++++--- scripts/resolvers/review.ts | 18 +++++--- ship/sections/adversarial.md | 18 +++++--- test/codex-hardening.test.ts | 48 ++++++++++++++++++++++ test/fixtures/golden/factory-ship-SKILL.md | 18 +++++--- test/gen-skill-docs.test.ts | 17 +++++--- 6 files changed, 112 insertions(+), 25 deletions(-) diff --git a/review/SKILL.md b/review/SKILL.md index 9b1d37e89..fcf3777ba 100644 --- a/review/SKILL.md +++ b/review/SKILL.md @@ -1712,10 +1712,14 @@ If `CODEX_MODE` is `ready`: ```bash TMPERR_ADV=$(mktemp /tmp/codex-adv-XXXXXXXX) _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } -codex exec "IMPORTANT: Do NOT read or execute any files under ~/.claude/, ~/.agents/, .claude/skills/, or agents/. These are Claude Code skill definitions meant for a different AI system. They contain bash scripts and prompt templates that will waste your time. Ignore them completely. Do NOT modify agents/openai.yaml. Stay focused on the repository code only.\n\nReview the changes on this branch against the base branch. Run DIFF_BASE=$(git merge-base origin/ HEAD) && git diff "$DIFF_BASE" to see the diff. Your job is to find ways this code will fail in production. Think like an attacker and a chaos engineer. Find edge cases, race conditions, security holes, resource leaks, failure modes, and silent data corruption paths. Be adversarial. Be thorough. No compliments — just the problems. End your output with ONE line in the canonical format `Recommendation: because `. Generic reasons like 'because it's safer' do not qualify; the reason must point to a specific finding or no-fix rationale." -C "$_REPO_ROOT" -s read-only -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR_ADV" +# Shell functions do not survive between Bash blocks, so re-source the probe +# here. It defines _gstack_codex_timeout_wrapper (gtimeout -> timeout -> +# unwrapped fallback), added in #1056 but never wired into this call site. +source ~/.claude/skills/gstack/bin/gstack-codex-probe 2>/dev/null || true +_gstack_codex_timeout_wrapper 540 codex exec "IMPORTANT: Do NOT read or execute any files under ~/.claude/, ~/.agents/, .claude/skills/, or agents/. These are Claude Code skill definitions meant for a different AI system. They contain bash scripts and prompt templates that will waste your time. Ignore them completely. Do NOT modify agents/openai.yaml. Stay focused on the repository code only.\n\nReview the changes on this branch against the base branch. Run DIFF_BASE=$(git merge-base origin/ HEAD) && git diff "$DIFF_BASE" to see the diff. Your job is to find ways this code will fail in production. Think like an attacker and a chaos engineer. Find edge cases, race conditions, security holes, resource leaks, failure modes, and silent data corruption paths. Be adversarial. Be thorough. No compliments — just the problems. End your output with ONE line in the canonical format `Recommendation: because `. Generic reasons like 'because it's safer' do not qualify; the reason must point to a specific finding or no-fix rationale." -C "$_REPO_ROOT" -s read-only -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR_ADV" ``` -Set the Bash tool's `timeout` parameter to `300000` (5 minutes). Do NOT use the `timeout` shell command — it doesn't exist on macOS. After the command completes, read stderr: +Set the Bash tool's `timeout` parameter to `600000` (10 minutes). It sits ABOVE the 540s wrapper deliberately, so the wrapper fires first and a stall surfaces as a diagnosable exit 124 instead of a harness kill that returns nothing. The wrapper resolves `gtimeout`, then `timeout`, then runs unwrapped, so it is safe on a macOS without coreutils. After the command completes, read stderr: ```bash cat "$TMPERR_ADV" ``` @@ -1724,7 +1728,7 @@ Present the full output verbatim. This is informational — it never blocks ship **Error handling:** All errors are non-blocking — adversarial review is a quality enhancement, not a prerequisite. - **Auth failure:** If stderr contains "auth", "login", "unauthorized", or "API key": "Codex authentication failed. Run \`codex login\` to authenticate." -- **Timeout:** "Codex timed out after 5 minutes." +- **Timeout (exit 124):** "Codex exceeded 9 minutes and was terminated; this pass produced NO findings." A timed-out pass is MISSING COVERAGE, not a clean bill — say so explicitly rather than continuing as if Codex had reviewed. Whatever it produced before the cut is recoverable from that run's rollout log under `~/.codex/sessions///
/`. - **Empty response:** "Codex returned no response. Stderr: ." **Cleanup:** Run `rm -f "$TMPERR_ADV"` after processing. @@ -1741,12 +1745,16 @@ If `DIFF_TOTAL >= 200` AND `CODEX_MODE` is `ready`: TMPERR=$(mktemp /tmp/codex-review-XXXXXXXX) _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } cd "$_REPO_ROOT" -codex review --base -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR" +# Shell functions do not survive between Bash blocks, so re-source the probe +# here. It defines _gstack_codex_timeout_wrapper (gtimeout -> timeout -> +# unwrapped fallback), added in #1056 but never wired into this call site. +source ~/.claude/skills/gstack/bin/gstack-codex-probe 2>/dev/null || true +_gstack_codex_timeout_wrapper 540 codex review --base -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR" ``` **No prompt argument.** `--base` is what scopes the review, and the positional `[PROMPT]` is mutually exclusive with it — passing both fails at argv parsing. Do NOT "fix" that error by dropping `--base` and keeping the prompt: a prompt-only `codex review` silently falls back to the **uncommitted working-tree** scope (`git status --short; git diff`), so it reviews the wrong changes and reports "no changes" on a clean tree. Prompt text describing the diff range does not change what the CLI feeds the reviewer. Unlike the adversarial pass above, which uses `codex exec` and really does run the git command it's told to, this path gets a pre-computed diff from the CLI — which is also why it needs no filesystem boundary. -Set the Bash tool's `timeout` parameter to `300000` (5 minutes). Do NOT use the `timeout` shell command — it doesn't exist on macOS. Present output under `CODEX SAYS (code review):` header. +Set the Bash tool's `timeout` parameter to `600000` (10 minutes). It sits ABOVE the 540s wrapper deliberately, so the wrapper fires first and a stall surfaces as a diagnosable exit 124 instead of a harness kill that returns nothing. The wrapper resolves `gtimeout`, then `timeout`, then runs unwrapped, so it is safe on a macOS without coreutils. Present output under `CODEX SAYS (code review):` header. Check for `[P1]` markers: found → `GATE: FAIL`, not found → `GATE: PASS`. If GATE is FAIL, use AskUserQuestion: diff --git a/scripts/resolvers/review.ts b/scripts/resolvers/review.ts index ed22eefdd..f1a92a1a3 100644 --- a/scripts/resolvers/review.ts +++ b/scripts/resolvers/review.ts @@ -527,10 +527,14 @@ If \`CODEX_MODE\` is \`ready\`: \`\`\`bash TMPERR_ADV=$(mktemp /tmp/codex-adv-XXXXXXXX) _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } -codex exec "${CODEX_BOUNDARY}Review the changes on this branch against the base branch. Run DIFF_BASE=$(git merge-base origin/ HEAD) && git diff "$DIFF_BASE" to see the diff. Your job is to find ways this code will fail in production. Think like an attacker and a chaos engineer. Find edge cases, race conditions, security holes, resource leaks, failure modes, and silent data corruption paths. Be adversarial. Be thorough. No compliments — just the problems. End your output with ONE line in the canonical format \`Recommendation: because \`. Generic reasons like 'because it's safer' do not qualify; the reason must point to a specific finding or no-fix rationale." -C "$_REPO_ROOT" -s read-only -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR_ADV" +# Shell functions do not survive between Bash blocks, so re-source the probe +# here. It defines _gstack_codex_timeout_wrapper (gtimeout -> timeout -> +# unwrapped fallback), added in #1056 but never wired into this call site. +source ~/.claude/skills/gstack/bin/gstack-codex-probe 2>/dev/null || true +_gstack_codex_timeout_wrapper 540 codex exec "${CODEX_BOUNDARY}Review the changes on this branch against the base branch. Run DIFF_BASE=$(git merge-base origin/ HEAD) && git diff "$DIFF_BASE" to see the diff. Your job is to find ways this code will fail in production. Think like an attacker and a chaos engineer. Find edge cases, race conditions, security holes, resource leaks, failure modes, and silent data corruption paths. Be adversarial. Be thorough. No compliments — just the problems. End your output with ONE line in the canonical format \`Recommendation: because \`. Generic reasons like 'because it's safer' do not qualify; the reason must point to a specific finding or no-fix rationale." -C "$_REPO_ROOT" -s read-only -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR_ADV" \`\`\` -Set the Bash tool's \`timeout\` parameter to \`300000\` (5 minutes). Do NOT use the \`timeout\` shell command — it doesn't exist on macOS. After the command completes, read stderr: +Set the Bash tool's \`timeout\` parameter to \`600000\` (10 minutes). It sits ABOVE the 540s wrapper deliberately, so the wrapper fires first and a stall surfaces as a diagnosable exit 124 instead of a harness kill that returns nothing. The wrapper resolves \`gtimeout\`, then \`timeout\`, then runs unwrapped, so it is safe on a macOS without coreutils. After the command completes, read stderr: \`\`\`bash cat "$TMPERR_ADV" \`\`\` @@ -539,7 +543,7 @@ Present the full output verbatim. This is informational — it never blocks ship **Error handling:** All errors are non-blocking — adversarial review is a quality enhancement, not a prerequisite. - **Auth failure:** If stderr contains "auth", "login", "unauthorized", or "API key": "Codex authentication failed. Run \\\`codex login\\\` to authenticate." -- **Timeout:** "Codex timed out after 5 minutes." +- **Timeout (exit 124):** "Codex exceeded 9 minutes and was terminated; this pass produced NO findings." A timed-out pass is MISSING COVERAGE, not a clean bill — say so explicitly rather than continuing as if Codex had reviewed. Whatever it produced before the cut is recoverable from that run's rollout log under \`~/.codex/sessions///
/\`. - **Empty response:** "Codex returned no response. Stderr: ." **Cleanup:** Run \`rm -f "$TMPERR_ADV"\` after processing. @@ -556,12 +560,16 @@ If \`DIFF_TOTAL >= 200\` AND \`CODEX_MODE\` is \`ready\`: TMPERR=$(mktemp /tmp/codex-review-XXXXXXXX) _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } cd "$_REPO_ROOT" -codex review --base -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR" +# Shell functions do not survive between Bash blocks, so re-source the probe +# here. It defines _gstack_codex_timeout_wrapper (gtimeout -> timeout -> +# unwrapped fallback), added in #1056 but never wired into this call site. +source ~/.claude/skills/gstack/bin/gstack-codex-probe 2>/dev/null || true +_gstack_codex_timeout_wrapper 540 codex review --base -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR" \`\`\` **No prompt argument.** \`--base\` is what scopes the review, and the positional \`[PROMPT]\` is mutually exclusive with it — passing both fails at argv parsing. Do NOT "fix" that error by dropping \`--base\` and keeping the prompt: a prompt-only \`codex review\` silently falls back to the **uncommitted working-tree** scope (\`git status --short; git diff\`), so it reviews the wrong changes and reports "no changes" on a clean tree. Prompt text describing the diff range does not change what the CLI feeds the reviewer. Unlike the adversarial pass above, which uses \`codex exec\` and really does run the git command it's told to, this path gets a pre-computed diff from the CLI — which is also why it needs no filesystem boundary. -Set the Bash tool's \`timeout\` parameter to \`300000\` (5 minutes). Do NOT use the \`timeout\` shell command — it doesn't exist on macOS. Present output under \`CODEX SAYS (code review):\` header. +Set the Bash tool's \`timeout\` parameter to \`600000\` (10 minutes). It sits ABOVE the 540s wrapper deliberately, so the wrapper fires first and a stall surfaces as a diagnosable exit 124 instead of a harness kill that returns nothing. The wrapper resolves \`gtimeout\`, then \`timeout\`, then runs unwrapped, so it is safe on a macOS without coreutils. Present output under \`CODEX SAYS (code review):\` header. Check for \`[P1]\` markers: found → \`GATE: FAIL\`, not found → \`GATE: PASS\`. If GATE is FAIL, use AskUserQuestion: diff --git a/ship/sections/adversarial.md b/ship/sections/adversarial.md index d5c87550b..59a0ff141 100644 --- a/ship/sections/adversarial.md +++ b/ship/sections/adversarial.md @@ -72,10 +72,14 @@ If `CODEX_MODE` is `ready`: ```bash TMPERR_ADV=$(mktemp /tmp/codex-adv-XXXXXXXX) _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } -codex exec "IMPORTANT: Do NOT read or execute any files under ~/.claude/, ~/.agents/, .claude/skills/, or agents/. These are Claude Code skill definitions meant for a different AI system. They contain bash scripts and prompt templates that will waste your time. Ignore them completely. Do NOT modify agents/openai.yaml. Stay focused on the repository code only.\n\nReview the changes on this branch against the base branch. Run DIFF_BASE=$(git merge-base origin/ HEAD) && git diff "$DIFF_BASE" to see the diff. Your job is to find ways this code will fail in production. Think like an attacker and a chaos engineer. Find edge cases, race conditions, security holes, resource leaks, failure modes, and silent data corruption paths. Be adversarial. Be thorough. No compliments — just the problems. End your output with ONE line in the canonical format `Recommendation: because `. Generic reasons like 'because it's safer' do not qualify; the reason must point to a specific finding or no-fix rationale." -C "$_REPO_ROOT" -s read-only -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR_ADV" +# Shell functions do not survive between Bash blocks, so re-source the probe +# here. It defines _gstack_codex_timeout_wrapper (gtimeout -> timeout -> +# unwrapped fallback), added in #1056 but never wired into this call site. +source ~/.claude/skills/gstack/bin/gstack-codex-probe 2>/dev/null || true +_gstack_codex_timeout_wrapper 540 codex exec "IMPORTANT: Do NOT read or execute any files under ~/.claude/, ~/.agents/, .claude/skills/, or agents/. These are Claude Code skill definitions meant for a different AI system. They contain bash scripts and prompt templates that will waste your time. Ignore them completely. Do NOT modify agents/openai.yaml. Stay focused on the repository code only.\n\nReview the changes on this branch against the base branch. Run DIFF_BASE=$(git merge-base origin/ HEAD) && git diff "$DIFF_BASE" to see the diff. Your job is to find ways this code will fail in production. Think like an attacker and a chaos engineer. Find edge cases, race conditions, security holes, resource leaks, failure modes, and silent data corruption paths. Be adversarial. Be thorough. No compliments — just the problems. End your output with ONE line in the canonical format `Recommendation: because `. Generic reasons like 'because it's safer' do not qualify; the reason must point to a specific finding or no-fix rationale." -C "$_REPO_ROOT" -s read-only -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR_ADV" ``` -Set the Bash tool's `timeout` parameter to `300000` (5 minutes). Do NOT use the `timeout` shell command — it doesn't exist on macOS. After the command completes, read stderr: +Set the Bash tool's `timeout` parameter to `600000` (10 minutes). It sits ABOVE the 540s wrapper deliberately, so the wrapper fires first and a stall surfaces as a diagnosable exit 124 instead of a harness kill that returns nothing. The wrapper resolves `gtimeout`, then `timeout`, then runs unwrapped, so it is safe on a macOS without coreutils. After the command completes, read stderr: ```bash cat "$TMPERR_ADV" ``` @@ -84,7 +88,7 @@ Present the full output verbatim. This is informational — it never blocks ship **Error handling:** All errors are non-blocking — adversarial review is a quality enhancement, not a prerequisite. - **Auth failure:** If stderr contains "auth", "login", "unauthorized", or "API key": "Codex authentication failed. Run \`codex login\` to authenticate." -- **Timeout:** "Codex timed out after 5 minutes." +- **Timeout (exit 124):** "Codex exceeded 9 minutes and was terminated; this pass produced NO findings." A timed-out pass is MISSING COVERAGE, not a clean bill — say so explicitly rather than continuing as if Codex had reviewed. Whatever it produced before the cut is recoverable from that run's rollout log under `~/.codex/sessions///
/`. - **Empty response:** "Codex returned no response. Stderr: ." **Cleanup:** Run `rm -f "$TMPERR_ADV"` after processing. @@ -101,12 +105,16 @@ If `DIFF_TOTAL >= 200` AND `CODEX_MODE` is `ready`: TMPERR=$(mktemp /tmp/codex-review-XXXXXXXX) _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } cd "$_REPO_ROOT" -codex review --base -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR" +# Shell functions do not survive between Bash blocks, so re-source the probe +# here. It defines _gstack_codex_timeout_wrapper (gtimeout -> timeout -> +# unwrapped fallback), added in #1056 but never wired into this call site. +source ~/.claude/skills/gstack/bin/gstack-codex-probe 2>/dev/null || true +_gstack_codex_timeout_wrapper 540 codex review --base -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR" ``` **No prompt argument.** `--base` is what scopes the review, and the positional `[PROMPT]` is mutually exclusive with it — passing both fails at argv parsing. Do NOT "fix" that error by dropping `--base` and keeping the prompt: a prompt-only `codex review` silently falls back to the **uncommitted working-tree** scope (`git status --short; git diff`), so it reviews the wrong changes and reports "no changes" on a clean tree. Prompt text describing the diff range does not change what the CLI feeds the reviewer. Unlike the adversarial pass above, which uses `codex exec` and really does run the git command it's told to, this path gets a pre-computed diff from the CLI — which is also why it needs no filesystem boundary. -Set the Bash tool's `timeout` parameter to `300000` (5 minutes). Do NOT use the `timeout` shell command — it doesn't exist on macOS. Present output under `CODEX SAYS (code review):` header. +Set the Bash tool's `timeout` parameter to `600000` (10 minutes). It sits ABOVE the 540s wrapper deliberately, so the wrapper fires first and a stall surfaces as a diagnosable exit 124 instead of a harness kill that returns nothing. The wrapper resolves `gtimeout`, then `timeout`, then runs unwrapped, so it is safe on a macOS without coreutils. Present output under `CODEX SAYS (code review):` header. Check for `[P1]` markers: found → `GATE: FAIL`, not found → `GATE: PASS`. If GATE is FAIL, use AskUserQuestion: diff --git a/test/codex-hardening.test.ts b/test/codex-hardening.test.ts index f1c00031a..4d47c487a 100644 --- a/test/codex-hardening.test.ts +++ b/test/codex-hardening.test.ts @@ -427,3 +427,51 @@ describe('codex SKILL.md.tmpl Step 2A: PROMPT + --base mutual exclusion guard', }); } }); + +// Regression guard for #1036. The wrapper added in #1056 was wired into +// codex/SKILL.md but not into the /review and /ship diff passes, which kept +// running under a bare 5-minute Bash gate. Measured on codex-cli 0.145.0: a +// pass was killed at 287s of a 300s budget mid-tool-call, and the same prompt +// completed in 336s. An unwrapped stall returns no exit code and no output, +// which downstream reads as "Codex reviewed and found nothing". +describe('codex timeout wrapper: /review + /ship diff passes', () => { + const WRAPPED_SITES = [ + 'scripts/resolvers/review.ts', // generator (source of truth) + 'review/SKILL.md', // generated + 'ship/sections/adversarial.md', // ship section source + ]; + + // Outer Bash gate for the wrapped passes. The wrapper must be strictly + // shorter so IT fires first and the failure is a diagnosable exit 124. + const BASH_GATE_MS = 600000; + + for (const relPath of WRAPPED_SITES) { + const read = () => fs.readFileSync(path.join(ROOT, relPath), 'utf8'); + + test(`${relPath}: both diff-review Codex calls run under the wrapper`, () => { + const wrapped = + read().match(/_gstack_codex_timeout_wrapper\s+\d+\s+codex\s+(exec|review)\b/g) ?? []; + // Adversarial pass + structured review pass. + expect(wrapped.length).toBeGreaterThanOrEqual(2); + }); + + test(`${relPath}: does not claim \`timeout\` is unavailable on macOS`, () => { + // _gstack_codex_timeout_wrapper resolves gtimeout -> timeout -> unwrapped, + // so the coreutils-less case is already handled. The old claim is what + // steered these call sites away from the wrapper in the first place. + expect(read()).not.toMatch(/doesn't exist on macOS/); + }); + + test(`${relPath}: wrapper budget stays under the outer Bash gate`, () => { + const budgets = [...read().matchAll(/_gstack_codex_timeout_wrapper\s+(\d+)\s+codex\b/g)].map( + (m) => Number(m[1]) * 1000, + ); + expect(budgets.length).toBeGreaterThan(0); + for (const ms of budgets) { + // Inverting this makes the wrapper unreachable: the harness kills the + // call first and the exit-124 branch below it becomes dead code. + expect(ms).toBeLessThan(BASH_GATE_MS); + } + }); + } +}); diff --git a/test/fixtures/golden/factory-ship-SKILL.md b/test/fixtures/golden/factory-ship-SKILL.md index ebb051ddd..efd8ffb96 100644 --- a/test/fixtures/golden/factory-ship-SKILL.md +++ b/test/fixtures/golden/factory-ship-SKILL.md @@ -2458,10 +2458,14 @@ If `CODEX_MODE` is `ready`: ```bash TMPERR_ADV=$(mktemp /tmp/codex-adv-XXXXXXXX) _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } -codex exec "IMPORTANT: Do NOT read or execute any files under ~/.claude/, ~/.agents/, .factory/skills/, or agents/. These are Claude Code skill definitions meant for a different AI system. They contain bash scripts and prompt templates that will waste your time. Ignore them completely. Do NOT modify agents/openai.yaml. Stay focused on the repository code only.\n\nReview the changes on this branch against the base branch. Run DIFF_BASE=$(git merge-base origin/ HEAD) && git diff "$DIFF_BASE" to see the diff. Your job is to find ways this code will fail in production. Think like an attacker and a chaos engineer. Find edge cases, race conditions, security holes, resource leaks, failure modes, and silent data corruption paths. Be adversarial. Be thorough. No compliments — just the problems. End your output with ONE line in the canonical format `Recommendation: because `. Generic reasons like 'because it's safer' do not qualify; the reason must point to a specific finding or no-fix rationale." -C "$_REPO_ROOT" -s read-only -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR_ADV" +# Shell functions do not survive between Bash blocks, so re-source the probe +# here. It defines _gstack_codex_timeout_wrapper (gtimeout -> timeout -> +# unwrapped fallback), added in #1056 but never wired into this call site. +source $GSTACK_ROOT/bin/gstack-codex-probe 2>/dev/null || true +_gstack_codex_timeout_wrapper 540 codex exec "IMPORTANT: Do NOT read or execute any files under ~/.claude/, ~/.agents/, .factory/skills/, or agents/. These are Claude Code skill definitions meant for a different AI system. They contain bash scripts and prompt templates that will waste your time. Ignore them completely. Do NOT modify agents/openai.yaml. Stay focused on the repository code only.\n\nReview the changes on this branch against the base branch. Run DIFF_BASE=$(git merge-base origin/ HEAD) && git diff "$DIFF_BASE" to see the diff. Your job is to find ways this code will fail in production. Think like an attacker and a chaos engineer. Find edge cases, race conditions, security holes, resource leaks, failure modes, and silent data corruption paths. Be adversarial. Be thorough. No compliments — just the problems. End your output with ONE line in the canonical format `Recommendation: because `. Generic reasons like 'because it's safer' do not qualify; the reason must point to a specific finding or no-fix rationale." -C "$_REPO_ROOT" -s read-only -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR_ADV" ``` -Set the Bash tool's `timeout` parameter to `300000` (5 minutes). Do NOT use the `timeout` shell command — it doesn't exist on macOS. After the command completes, read stderr: +Set the Bash tool's `timeout` parameter to `600000` (10 minutes). It sits ABOVE the 540s wrapper deliberately, so the wrapper fires first and a stall surfaces as a diagnosable exit 124 instead of a harness kill that returns nothing. The wrapper resolves `gtimeout`, then `timeout`, then runs unwrapped, so it is safe on a macOS without coreutils. After the command completes, read stderr: ```bash cat "$TMPERR_ADV" ``` @@ -2470,7 +2474,7 @@ Present the full output verbatim. This is informational — it never blocks ship **Error handling:** All errors are non-blocking — adversarial review is a quality enhancement, not a prerequisite. - **Auth failure:** If stderr contains "auth", "login", "unauthorized", or "API key": "Codex authentication failed. Run \`codex login\` to authenticate." -- **Timeout:** "Codex timed out after 5 minutes." +- **Timeout (exit 124):** "Codex exceeded 9 minutes and was terminated; this pass produced NO findings." A timed-out pass is MISSING COVERAGE, not a clean bill — say so explicitly rather than continuing as if Codex had reviewed. Whatever it produced before the cut is recoverable from that run's rollout log under `~/.codex/sessions///
/`. - **Empty response:** "Codex returned no response. Stderr: ." **Cleanup:** Run `rm -f "$TMPERR_ADV"` after processing. @@ -2487,12 +2491,16 @@ If `DIFF_TOTAL >= 200` AND `CODEX_MODE` is `ready`: TMPERR=$(mktemp /tmp/codex-review-XXXXXXXX) _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } cd "$_REPO_ROOT" -codex review --base -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR" +# Shell functions do not survive between Bash blocks, so re-source the probe +# here. It defines _gstack_codex_timeout_wrapper (gtimeout -> timeout -> +# unwrapped fallback), added in #1056 but never wired into this call site. +source $GSTACK_ROOT/bin/gstack-codex-probe 2>/dev/null || true +_gstack_codex_timeout_wrapper 540 codex review --base -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR" ``` **No prompt argument.** `--base` is what scopes the review, and the positional `[PROMPT]` is mutually exclusive with it — passing both fails at argv parsing. Do NOT "fix" that error by dropping `--base` and keeping the prompt: a prompt-only `codex review` silently falls back to the **uncommitted working-tree** scope (`git status --short; git diff`), so it reviews the wrong changes and reports "no changes" on a clean tree. Prompt text describing the diff range does not change what the CLI feeds the reviewer. Unlike the adversarial pass above, which uses `codex exec` and really does run the git command it's told to, this path gets a pre-computed diff from the CLI — which is also why it needs no filesystem boundary. -Set the Bash tool's `timeout` parameter to `300000` (5 minutes). Do NOT use the `timeout` shell command — it doesn't exist on macOS. Present output under `CODEX SAYS (code review):` header. +Set the Bash tool's `timeout` parameter to `600000` (10 minutes). It sits ABOVE the 540s wrapper deliberately, so the wrapper fires first and a stall surfaces as a diagnosable exit 124 instead of a harness kill that returns nothing. The wrapper resolves `gtimeout`, then `timeout`, then runs unwrapped, so it is safe on a macOS without coreutils. Present output under `CODEX SAYS (code review):` header. Check for `[P1]` markers: found → `GATE: FAIL`, not found → `GATE: PASS`. If GATE is FAIL, use AskUserQuestion: diff --git a/test/gen-skill-docs.test.ts b/test/gen-skill-docs.test.ts index b1e51796a..3579a754b 100644 --- a/test/gen-skill-docs.test.ts +++ b/test/gen-skill-docs.test.ts @@ -1944,16 +1944,21 @@ describe('Codex generation (--host codex)', () => { const content = fs.readFileSync(path.join(ROOT, 'review', 'SKILL.md'), 'utf-8'); expect(content).toContain('.claude/skills/review/checklist.md'); expect(content).toContain('~/.claude/skills/gstack'); - // Must NOT contain Codex paths + // Must NOT contain Codex HOST paths. `~/.codex/sessions/` is exempt: the + // timeout-wrapper guidance documents the Codex CLI's own rollout-log + // location (a user-facing CLI path, same class as ~/.codex/logs/ in the + // codex skill), not the gstack Codex host install path. expect(content).not.toContain('.agents/skills'); - expect(content).not.toContain('~/.codex/'); + expect(content.replaceAll('~/.codex/sessions/', '')).not.toContain('~/.codex/'); }); test('Claude output unchanged: ship skill still uses .claude/skills/ paths', () => { const content = readShipUnion(); expect(content).toContain('~/.claude/skills/gstack'); expect(content).not.toContain('.agents/skills'); - expect(content).not.toContain('~/.codex/'); + // ~/.codex/sessions/ is the Codex CLI's rollout-log path (user-facing), + // documented by the adversarial-pass timeout guidance — see review test above. + expect(content.replaceAll('~/.codex/sessions/', '')).not.toContain('~/.codex/'); }); test('Claude output unchanged: all Claude skills have zero Codex paths', () => { @@ -1962,9 +1967,11 @@ describe('Codex generation (--host codex)', () => { // pair-agent legitimately documents how Codex agents store credentials. // codex + autoplan document the Codex CLI auth file (~/.codex/auth.json) // and log path (~/.codex/logs/) — those are user-facing Codex CLI paths, - // not the gstack Codex host install path. + // not the gstack Codex host install path. ~/.codex/sessions/ (rollout + // logs, referenced by the review/ship timeout guidance) is the same + // user-facing class, so it is scrubbed before the ban. if (skill.dir !== 'pair-agent' && skill.dir !== 'codex' && skill.dir !== 'autoplan') { - expect(content).not.toContain('~/.codex/'); + expect(content.replaceAll('~/.codex/sessions/', '')).not.toContain('~/.codex/'); } // gstack-upgrade legitimately references .agents/skills for cross-platform detection if (skill.dir !== 'gstack-upgrade') {