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') {