diff --git a/bin/gstack-codex-probe b/bin/gstack-codex-probe index 47f8a68cb..15d466e94 100755 --- a/bin/gstack-codex-probe +++ b/bin/gstack-codex-probe @@ -48,9 +48,14 @@ _gstack_codex_model_probe() { # Contract: # MODEL_OK (exit 0) — round trip succeeded; cached 1h. # MODEL_UNUSABLE (exit 1) — deterministic model 400; hints printed. + # Cached 15 min: the 400 is config-driven, so re-probing every preflight + # charged the affected user a 30s round trip + real tokens per review + # section, forever. Editing config.toml (the fix) changes the cache + # signature and re-probes immediately; the short TTL covers server-side + # entitlement recovery the signature can't see. # MODEL_PROBE_INCONCLUSIVE (exit 0) — timeout/transient; FAIL-OPEN so a # slow network never wedges codex mode (the per-invocation Error - # Handling entry still covers a later 400). + # Handling entry still covers a later 400). Never cached. # # Only call this AFTER _gstack_codex_auth_probe passes — probing without # auth just measures the auth failure again. @@ -76,6 +81,12 @@ _gstack_codex_model_probe() { echo "MODEL_OK (cached)" return 0 fi + if [ "$_c_status" = "MODEL_UNUSABLE" ] && [ "$_c_sig" = "$_sig" ] && [ $((_now - _c_ts)) -lt 900 ]; then + echo "MODEL_UNUSABLE (cached)" + echo "HINT: the rejected model comes from the 'model = ' line in $_codex_home/config.toml." + echo "HINT: check its [notice.model_migrations] table — Codex records the intended replacement there." + return 1 + fi fi local _out _code _out=$(_gstack_codex_timeout_wrapper 30 codex exec --skip-git-repo-check -s read-only "reply OK" &1) @@ -87,6 +98,8 @@ _gstack_codex_model_probe() { return 0 fi if printf '%s' "$_out" | grep -qiE 'model.{0,40}is not supported|"status":[[:space:]]*400'; then + mkdir -p "$_gstack_home" 2>/dev/null || true + printf 'MODEL_UNUSABLE %s %s\n' "$_now" "$_sig" > "$_cache" 2>/dev/null || true echo "MODEL_UNUSABLE" printf '%s\n' "$_out" | grep -i "model" | head -3 echo "HINT: the rejected model comes from the 'model = ' line in $_codex_home/config.toml." @@ -120,8 +133,8 @@ _gstack_codex_version_check() { _gstack_codex_timeout_wrapper() { # Resolve wrapper binary: prefer gtimeout (Homebrew coreutils on macOS), - # fall back to timeout (Linux), else run unwrapped. Arguments: $1 is the - # duration in seconds; rest is the command to run. + # fall back to timeout (Linux), else a bash-native watchdog. Arguments: + # $1 is the duration in seconds; rest is the command to run. local _duration="$1" shift local _to @@ -129,7 +142,29 @@ _gstack_codex_timeout_wrapper() { if [ -n "$_to" ]; then "$_to" "$_duration" "$@" else - "$@" + # Stock macOS ships neither coreutils gtimeout nor timeout(1); running + # unwrapped let a hung `codex exec` block the probe — and the calling + # workflow — indefinitely. Emulate: background the command, TERM it at + # the deadline, mirror timeout(1)'s exit-124 contract. The watchdog's + # stdout is detached so an early finish never blocks a caller's $(...) + # capture on the orphaned sleep. + "$@" & + local _cmd_pid=$! + ( sleep "$_duration" && kill -TERM "$_cmd_pid" 2>/dev/null ) >/dev/null 2>&1 & + local _watch_pid=$! + local _rc + wait "$_cmd_pid" + _rc=$? + if kill -0 "$_watch_pid" 2>/dev/null; then + # Command finished before the deadline. Retiring the watchdog subshell + # also defuses its pending kill (the `&& kill` lives in the subshell); + # its detached sleep expires harmlessly. + kill "$_watch_pid" 2>/dev/null + wait "$_watch_pid" 2>/dev/null + elif [ "$_rc" -ge 128 ]; then + _rc=124 # killed by the watchdog: report timeout(1)'s code + fi + return "$_rc" fi } diff --git a/test/codex-hardening.test.ts b/test/codex-hardening.test.ts index ade705a55..3c7600a58 100644 --- a/test/codex-hardening.test.ts +++ b/test/codex-hardening.test.ts @@ -287,6 +287,29 @@ describe('gstack-codex-probe: timeout wrapper + namespace hygiene', () => { } }); + test('bash-native watchdog kills a hung command at the deadline (exit 124, no timeout binary)', () => { + // Stock macOS ships neither gtimeout nor timeout(1) — the old fallback ran + // the command unwrapped, so a hung `codex exec` blocked the calling + // workflow forever. Force the fallback everywhere (Linux /bin has timeout + // via usrmerge) with a PATH holding ONLY bash and sleep, then prove a + // 30s sleep dies at the 1s deadline with timeout(1)'s exit code. The + // runProbe 5s spawnSync cap doubles as the "actually killed fast" bound. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-watchdog-')); + try { + const which = (tool: string) => + spawnSync('bash', ['-c', `command -v ${tool}`]).stdout.toString().trim() || `/bin/${tool}`; + fs.symlinkSync(which('bash'), path.join(dir, 'bash')); + fs.symlinkSync(which('sleep'), path.join(dir, 'sleep')); + const r = runProbe({ + snippet: `_gstack_codex_timeout_wrapper 1 sleep 30; echo "rc=$?"`, + env: { PATH: dir }, + }); + expect(r.stdout).toContain('rc=124'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + test('sourcing probe does NOT set errexit/trap/IFS in caller shell (namespace hygiene)', () => { // Capture `set -o` output before and after sourcing. Any drift means the // probe polluted the caller. diff --git a/test/codex-model-probe.test.ts b/test/codex-model-probe.test.ts index 2915fd01c..381379634 100644 --- a/test/codex-model-probe.test.ts +++ b/test/codex-model-probe.test.ts @@ -10,9 +10,13 @@ * - exit 0 -> MODEL_OK, result cached (1h TTL + config/auth * mtime signature), second call does NOT re-invoke * - model 400 output -> MODEL_UNUSABLE (exit 1) + config.toml HINT lines, - * nothing cached - * - transient failure -> MODEL_PROBE_INCONCLUSIVE, FAIL-OPEN (exit 0) - * - config.toml mtime change invalidates a cached MODEL_OK + * negative-cached 15 min (same exit-1 + hints from + * cache; re-probing every preflight charged the + * affected user 30s + real tokens per section) + * - transient failure -> MODEL_PROBE_INCONCLUSIVE, FAIL-OPEN (exit 0), + * never cached + * - config.toml mtime change invalidates a cached MODEL_OK and a cached + * MODEL_UNUSABLE (editing the pin IS the fix) */ import { describe, test, expect } from 'bun:test'; import { spawnSync } from 'child_process'; @@ -105,7 +109,7 @@ describe('codex model probe (#2477)', () => { } }); - test('model 400 -> MODEL_UNUSABLE with config.toml hints, exit 1, nothing cached', () => { + test('model 400 -> MODEL_UNUSABLE with config.toml hints, exit 1, negative-cached', () => { const f = makeFixture(); try { const r = runProbe(f, 'model400'); @@ -115,7 +119,35 @@ describe('codex model probe (#2477)', () => { // Surfaces the actual rejection so the user sees WHICH model. expect(r.stdout).toContain('gpt-5.4'); expect(r.status).toBe(1); - expect(fs.existsSync(path.join(f.gstackHome, '.codex-model-probe'))).toBe(false); + // The deterministic 400 is config-driven: re-probing every preflight + // charged the user a 30s round trip + real tokens per review section. + // A second run within the 15-min TTL must NOT re-invoke codex, and must + // keep the exit-1 + hints contract so callers can't tell the difference. + expect(invocations(f)).toBe(1); + const second = runProbe(f, 'model400'); + expect(second.stdout).toContain('MODEL_UNUSABLE (cached)'); + expect(second.stdout).toContain('config.toml'); + expect(second.status).toBe(1); + expect(invocations(f)).toBe(1); + } finally { + fs.rmSync(f.home, { recursive: true, force: true }); + } + }); + + test('config.toml change re-probes past a cached MODEL_UNUSABLE (the recovery path)', () => { + const f = makeFixture(); + try { + runProbe(f, 'model400'); + expect(invocations(f)).toBe(1); + // Fixing the model pin changes the mtime signature — the negative cache + // must not outlive the config it condemned. + fs.writeFileSync(path.join(f.codexHome, 'config.toml'), 'model = "gpt-5.5"\n'); + const future = Date.now() / 1000 + 10; + fs.utimesSync(path.join(f.codexHome, 'config.toml'), future, future); + const r = runProbe(f, 'ok'); + expect(r.stdout.trim()).toBe('MODEL_OK'); + expect(r.status).toBe(0); + expect(invocations(f)).toBe(2); } finally { fs.rmSync(f.home, { recursive: true, force: true }); }