From 8dac2174650a87ad6514de91d42c32501b69a3d8 Mon Sep 17 00:00:00 2001 From: garrytan Date: Thu, 8 Oct 2026 02:22:03 +0000 Subject: [PATCH] fix: separate readiness diagnostics and bound version checks --- autoplan/SKILL.md | 2 +- bin/gstack-codex-probe | 6 +++- docs/model-policy.md | 15 ++++++---- scripts/resolvers/constants.ts | 2 +- test/claude-code-skill.test.ts | 23 +++++++++++++-- test/model-role-invocation.test.ts | 47 ++++++++++++++++++++++++++---- 6 files changed, 80 insertions(+), 15 deletions(-) diff --git a/autoplan/SKILL.md b/autoplan/SKILL.md index 9e54f745d..d310539b8 100644 --- a/autoplan/SKILL.md +++ b/autoplan/SKILL.md @@ -793,7 +793,7 @@ Branch on the echoed `CODEX_MODE`: - **`quota_exhausted`** — Codex usage limit: relay the probe's lines verbatim (reset time, retry); no more Codex calls this run. Keep the required Claude adversarial pass; do not dispatch a duplicate. - **`sandbox_unavailable`** — Codex's sandbox cannot start here (containers without user namespaces); the probe printed the reason and fix. No paid call ran; outside coverage is unavailable. Keep the required Claude adversarial pass; do not dispatch a duplicate. - **`ready`** or **`unverified`** — run the Codex pass below. `unverified` means the model check timed out or, with `(rate_limited)`, hit a 429; say so, and let the pass's own verdict decide. -Plan-review role: the invocation probes the [policy](https://github.com/garrytan/gstack/blob/main/docs/model-policy.md) model; its exit 1, 2 or 4 is `model_unusable`, `broken_install` or `quota_exhausted`. +Plan-review readiness probes the [policy](https://github.com/garrytan/gstack/blob/main/docs/model-policy.md) model. Relay its diagnostics; never infer quota from the review block's exit status. Disabled/unavailable retains applicable native passes. Recheck each outside dispatch. Record provider and completed/unavailable/disabled/skipped per phase; CEO covers diff --git a/bin/gstack-codex-probe b/bin/gstack-codex-probe index 1f71a6cad..613f8ef9f 100755 --- a/bin/gstack-codex-probe +++ b/bin/gstack-codex-probe @@ -654,9 +654,13 @@ _gstack_codex_version_check() { # Capture the code from codex, not from `head` — a pipeline reports the LAST # command's status, which is why a CLI that only ever printed a spawn error # still read as healthy here (#2742). Keep stderr: it carries the diagnosis. - _ver=$(codex --version 2>&1) + _ver=$(_gstack_codex_timeout_wrapper 5 codex --version &1) _vcode=$? _ver=$(printf '%s' "$_ver" | head -1) + if [ "$_vcode" -eq 124 ]; then + echo 'WARN: `codex --version` timed out; version unknown. The invocation keeps its existing deadline.' + return 0 + fi # Only a NON-ZERO exit is evidence of a broken CLI. Empty-but-successful # output stays silent by design (a CLI may legitimately print nothing), which # the "empty output → OK" case in this file's suite pins. diff --git a/docs/model-policy.md b/docs/model-policy.md index 18f8341b6..274d46adf 100644 --- a/docs/model-policy.md +++ b/docs/model-policy.md @@ -146,9 +146,11 @@ reviews stay disabled; an unavailable model is reported, never silently replaced Policy setup and a completed review are different milestones. Install and authenticate the opposing CLI using the [host setup instructions](../README.md#install--about-30-seconds) -before invoking a paid review. From Claude Code, use `/codex challenge --role -plan-review` with your plan. From Codex, use `/claude-code challenge --role -plan-review` (or its installed namespaced name). +before invoking a paid review. From Claude Code, use `/codex consult --role +plan-review`. From Codex, use `/claude-code consult --role plan-review` (or its +installed namespaced name). Ask it to challenge the plan and supply the full plan +text. Consult is prompt-driven and works before there is a repository diff; +Challenge mode is for reviewing code changes. Confirm the announced selection, the provider's actual result and the coverage status. A resolved model, a successful availability probe or a native fallback @@ -186,8 +188,11 @@ source check without changing an issue; unlike `gstack-models`, it uses the network. The workflow's guarded `--publish` path owns issue updates. Its JSON report and job summary retain the source evidence. -The report distinguishes `current`, `update-candidate`, `stale` and -`unknown/source-unavailable`. Newer or newly recommended models are candidates, +The report's primary status distinguishes `current`, `update-candidate` and +`unknown/source-unavailable`. Its separate `evidenceStatus` marks retained +successful evidence `stale` after thirty days; a failed fetch remains unknown +in the headline even when that retained evidence is stale. +Newer or newly recommended models are candidates, not automatic replacements. A support promise such as “not sooner than” is not a scheduled retirement, and an incidental model mention is not a new default. This evidence covers public API recommendations, not partner-platform entitlement. diff --git a/scripts/resolvers/constants.ts b/scripts/resolvers/constants.ts index 12b826d56..4dc3b786e 100644 --- a/scripts/resolvers/constants.ts +++ b/scripts/resolvers/constants.ts @@ -219,7 +219,7 @@ Branch on the echoed \`CODEX_MODE\`: - **\`quota_exhausted\`** — Codex usage limit: relay the probe's lines verbatim (reset time, retry); no more Codex calls this run. ${nativeRoute} - **\`sandbox_unavailable\`** — Codex's sandbox cannot start here (containers without user namespaces); the probe printed the reason and fix. No paid call ran; outside coverage is unavailable. ${nativeRoute} - **\`ready\`** or **\`unverified\`** — run the Codex pass below. \`unverified\` means the model check timed out or, with \`(rate_limited)\`, hit a 429; say so, and let the pass's own verdict decide.${opts.role ? ` -Plan-review role: the invocation probes the [policy](https://github.com/garrytan/gstack/blob/main/docs/model-policy.md) model; its exit 1, 2 or 4 is \`model_unusable\`, \`broken_install\` or \`quota_exhausted\`.` : ''}`; +Plan-review readiness probes the [policy](https://github.com/garrytan/gstack/blob/main/docs/model-policy.md) model. Relay its diagnostics; never infer quota from the review block's exit status.` : ''}`; } /** diff --git a/test/claude-code-skill.test.ts b/test/claude-code-skill.test.ts index 92eda8582..61139c927 100644 --- a/test/claude-code-skill.test.ts +++ b/test/claude-code-skill.test.ts @@ -71,7 +71,7 @@ console.log(JSON.stringify({result:process.env.FAKE_TEXT || '[P1] Review found a }); afterAll(() => fs.rmSync(DIR, {recursive:true, force:true})); -function shell(mode: Mode, extra: NodeJS.ProcessEnv = {}, options: {resume?: boolean; runtime?: string; cwd?: string} = {}) { +function shell(mode: Mode, extra: NodeJS.ProcessEnv = {}, options: {resume?: boolean; runtime?: string; cwd?: string; planReview?: boolean} = {}) { fs.rmSync(CAPTURE, {force:true}); fs.writeFileSync(PROMPT, PROMPT_TEXT, {mode:0o600}); // These are precisely the literal substitutions the skill requests. Do not @@ -80,7 +80,8 @@ function shell(mode: Mode, extra: NodeJS.ProcessEnv = {}, options: {resume?: boo .replace("''", q(PROMPT)) .replace("''", q(options.runtime ?? RUNTIME)) .replace("''", q('main')) - .replace("''", q(options.resume ? 'resume' : 'fresh')); + .replace("''", q(options.resume ? 'resume' : 'fresh')) + .replaceAll('"$CLAUDE_RUNNER" --cwd', `"$CLAUDE_RUNNER"${options.planReview ? ' --role plan-review' : ''} --cwd`); return spawnSync('bash', ['-c',script], {cwd:options.cwd ?? REPO, env:{...ENV,...extra}, encoding:'utf8',timeout:10000}); } function captured() { return JSON.parse(fs.readFileSync(CAPTURE,'utf8')); } @@ -203,4 +204,22 @@ describe('complete generated Claude Code wrapper modes', () => { expectTempsCleaned(); } }); + + test('the documented plan-review consultation consumes a plan even without a repository diff', () => { + const clean = fs.mkdtempSync(path.join(DIR, 'plan-only-')); + git(['init', '-b', 'main'], clean); + fs.writeFileSync(path.join(clean, 'file.txt'), 'unchanged\n'); + git(['add', 'file.txt'], clean); + git(['commit', '-m', 'baseline'], clean); + const result = shell('consult', { HOME: clean, CLAUDE_CONFIG_DIR: path.join(clean, '.claude'), + GSTACK_STATE_ROOT: path.join(clean, 'policy'), GSTACK_CLAUDE_MODEL: '', FAKE_TEXT: 'The plan needs an overflow check.' }, + { cwd: clean, planReview: true }); + expect(result.status).toBe(0); + expect(result.stdout).toContain('The plan needs an overflow check.'); + const observed = JSON.parse(fs.readFileSync(CAPTURE, 'utf8')); + expect(observed.prompt).toBe(PROMPT_TEXT); + expect(observed.args).toContain('--model=claude-fable-5-1'); + expect(result.stdout).not.toContain('Nothing to review'); + expectTempsCleaned(); + }); }); diff --git a/test/model-role-invocation.test.ts b/test/model-role-invocation.test.ts index b54b9bfdd..8220e61fd 100644 --- a/test/model-role-invocation.test.ts +++ b/test/model-role-invocation.test.ts @@ -30,7 +30,7 @@ fs.writeFileSync(FAKE, ` import { appendFileSync, writeFileSync } from 'node:fs'; const args = process.argv.slice(2); if (args[0] === 'sandbox') process.exit(0); -if (args[0] === '--version') { if (process.env.FAKE_VERSION_SLEEP) Bun.sleepSync(Number(process.env.FAKE_VERSION_SLEEP) * 1000); console.log('codex-cli ' + (process.env.FAKE_CODEX_VERSION ?? '0.160.0')); process.exit(0); } +if (args[0] === '--version') { if (process.env.FAKE_VERSION_IGNORE_TERM) process.on('SIGTERM', () => {}); if (process.env.FAKE_VERSION_SLEEP) Bun.sleepSync(Number(process.env.FAKE_VERSION_SLEEP) * 1000); console.log('codex-cli ' + (process.env.FAKE_CODEX_VERSION ?? '0.160.0')); process.exit(0); } const claude = process.env.FAKE_AS === 'claude'; const probe = !claude && args.includes('reply OK'); appendFileSync(process.env.CALLS!, JSON.stringify({ args, probe }) + '\\n'); @@ -43,14 +43,30 @@ if (probe) { process.exit(0); } if (!claude || args.includes('-')) await Bun.stdin.text(); -const response = 'Medium: the plan couples two rollouts.\\nRecommendation: split the migration because the plan couples two rollouts.'; +const response = process.env.FAKE_RESPONSE || 'Medium: the plan couples two rollouts.\\nRecommendation: split the migration because the plan couples two rollouts.'; if (claude) console.log(JSON.stringify({ result: response, session_id: 's', modelUsage: { 'model-actual': { inputTokens: 1 } } })); else writeFileSync(args[args.indexOf('-o') + 1], response); `); fs.writeFileSync(path.join(BIN, 'codex'), `#!/usr/bin/env bash\nexec bun "${FAKE}" "$@"\n`, { mode: 0o755 }); // Records when each supervised command starts and the deadline it was given. const TIMEOUT_LOG = path.join(TMP, 'timeout.log'); -fs.writeFileSync(path.join(BIN, 'timeout'), `#!/usr/bin/env bash\n[ -z "\${TIMEOUT_LOG:-}" ] || printf '%s %s\\n' "$(date +%s.%N)" "$*" >> "$TIMEOUT_LOG"\nexec "${Bun.which('timeout')}" "$@"\n`, { mode: 0o755 }); +const TIMEOUT_FAKE = path.join(TMP, 'timeout.ts'); +fs.writeFileSync(TIMEOUT_FAKE, ` +import { appendFileSync } from 'node:fs'; +const original = process.argv.slice(2); +if (process.env.TIMEOUT_LOG) appendFileSync(process.env.TIMEOUT_LOG, (Date.now()/1000) + ' ' + original.join(' ') + '\\n'); +const args = [...original]; +const grace = args[0] === '-k' ? (args.shift(), Number(args.shift())) : 10; +const duration = Number(args.shift()); +const child = Bun.spawn(args, { stdin: 'inherit', stdout: 'inherit', stderr: 'inherit' }); +let expired = false; +let killTimer; +const timer = setTimeout(() => { expired = true; child.kill('SIGTERM'); killTimer = setTimeout(() => child.kill('SIGKILL'), grace*1000); }, duration*1000); +const code = await child.exited; +clearTimeout(timer); clearTimeout(killTimer); +process.exit(expired ? 124 : code); +`); +for (const name of ['gtimeout', 'timeout']) fs.writeFileSync(path.join(BIN, name), `#!/usr/bin/env bash\nexec bun "${TIMEOUT_FAKE}" "$@"\n`, { mode: 0o755 }); const git = (args: string[]) => { const r = spawnSync('git', args, { cwd: REPO, encoding: 'utf8', timeout: 5000, env: { ...process.env, GIT_AUTHOR_NAME: 'T', GIT_AUTHOR_EMAIL: 't@example.invalid', GIT_COMMITTER_NAME: 'T', GIT_COMMITTER_EMAIL: 't@example.invalid' } }); @@ -112,6 +128,17 @@ describe('plan-review role membership in shared generators', () => { expect(legacy).toContain('or config.toml `model`'); }); + test('readiness guidance never assigns quota meaning to the whole review exit status', () => { + const role = outsideVoicePreflight(ctxFor('autoplan', 'claude', HOST_PATHS.claude), { disabledBehavior: 'codex-only', role: 'plan-review' }); + expect(role).not.toContain('its exit 1, 2 or 4 is'); + expect(role).toContain("never infer quota from the review block's exit status"); + const result = run('claude', home(), { FAKE_RESPONSE: 'Recommendation: proceed because this is a synthetic incomplete-format response.' }); + expect(result.status).toBe(4); + expect(result.stdout).toContain('OUTSIDE_STATUS: unverified'); + expect(result.stderr).not.toContain('MODEL_QUOTA_EXHAUSTED'); + expect(result.calls.map((call: { probe: boolean }) => call.probe)).toEqual([true, false]); + }); + const roleBearing: Array<[string, (ctx: TemplateContext) => string]> = [ ['autoplan', ctx => RESOLVERS.OUTSIDE_INVOCATION(ctx, ['autoplan'])], ['spec', ctx => RESOLVERS.OUTSIDE_INVOCATION(ctx, ['spec'])], @@ -325,7 +352,7 @@ describe('one deadline bounds Codex role readiness and dispatch (native F3)', () return { at: Number(at) - t0, duration: Number(duration), command: command.join(' ') }; }); const calls = fs.existsSync(CALLS) ? fs.readFileSync(CALLS, 'utf8').split('\n').filter(Boolean).map(line => JSON.parse(line)) : []; - return { ...r, out: `${r.stdout}${r.stderr}`, calls, dispatch: supervised.find(s => / exec - /.test(s.command)) }; + return { ...r, out: `${r.stdout}${r.stderr}`, calls, supervised, dispatch: supervised.find(s => / exec - /.test(s.command)) }; } // Readiness time already spent plus the dispatch's own limit stays within the // provider budget (1.5s covers whole-second clock rounding and process start). @@ -377,12 +404,22 @@ describe('one deadline bounds Codex role readiness and dispatch (native F3)', () }, 60000); test('exhausted: readiness that outlives the whole budget dispatches nothing and is missing coverage', () => { - const r = timed(home(), { FAKE_VERSION_SLEEP: String(T + 1) }); + const r = timed(home(), { FAKE_VERSION_SLEEP: String(T + 1), FAKE_VERSION_IGNORE_TERM: '1' }); expect(r.status).toBe(124); expect(r.calls).toEqual([]); expect(r.dispatch).toBeUndefined(); expect(r.out).not.toContain('OUTSIDE_STATUS: completed'); }, 60000); + + test('a hanging version command is supervised before model readiness and dispatch', () => { + const r = timed(home(), { FAKE_VERSION_SLEEP: '8' }); + expect(r.status).toBe(0); + const version = r.supervised.find(command => command.command === 'codex --version'); + expect(version).toBeDefined(); + expect(version!.duration).toBeLessThanOrEqual(5); + expect(r.out).toContain('codex --version` timed out'); + withinBudget(r.dispatch!); + }, 60000); }); describe('Claude plan-review invocation emits one selected model or none', () => {