mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-09 14:38:59 +02:00
fix(codex): a CLI that cannot execute no longer reports CODEX_MODE: ready
Follow-up to #2477. The model probe it added does a real round trip, but its
final branch is the `else` of a "model 400" grep, so it swallowed spawn ENOENT,
non-executable binaries and missing vendor payloads alongside genuine network
timeouts. All three are deterministic — retrying never helps — yet they landed
in the fail-open bucket and resolved to `ready`, so every Codex pass was
skipped in silence and the review reported itself complete.
Observed live: @openai/codex was on PATH with an empty
vendor/aarch64-apple-darwin/codex/ directory. gstack said `ready` for two
months while no Codex pass ran.
Three changes:
- `_gstack_codex_model_probe` classifies deterministic install failures (exit
126/127, or stderr matching ENOENT/ENOEXEC/EACCES/"cannot execute binary
file") as MODEL_UNUSABLE_INSTALL, exit 2, never cached — a reinstall is
picked up on the next probe. Exit 124 and genuine transients still fail open,
which is what #2477 intended.
- The preflight chain captures the probe's code instead of testing it for
truthiness, so exit 2 routes to a new `broken_install` mode whose remedy is
`npm install -g @openai/codex` rather than "check your model pin". A missing
binary and an unusable model are different problems with different fixes.
- `_gstack_codex_version_check` no longer reads a broken CLI as healthy. It ran
`codex --version 2>/dev/null | head -1`, which captures head's status, not
codex's — and 2>/dev/null discarded the one diagnostic available. It now
captures the real exit code and warns on non-zero. Empty-but-successful
output stays silent, per the existing "empty output → OK" case.
Tests: 6 added to test/codex-hardening.test.ts covering both broken-install
shapes, the exit-2 contract, no caching, the transient still failing open, the
model 400 still classifying as MODEL_UNUSABLE, and the version-check warning.
845 pass / 0 fail across all 8 suites touching the changed files.
Closes #2742
Wave-amended: autoplan hand-maintained preflight chain completed (tmpl+render); install-signature grep gated on failed spawn only; goldens regenerated against the wave tree (author's golden commit 5797d326 superseded); +2 tests
This commit is contained in:
@@ -599,3 +599,168 @@ describe('codex skeleton+sections union: review sandbox + fail-closed gate + tim
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
// #2742: a Codex CLI that is on PATH but cannot execute (spawn ENOENT, missing
|
||||
// vendor payload, non-executable binary) used to land in the model probe's
|
||||
// fail-open bucket and resolve to CODEX_MODE: ready — so every Codex pass was
|
||||
// skipped in silence. These pin the classification, the exit-code contract, and
|
||||
// the fact that the fail-open path still exists for genuine transients.
|
||||
describe('codex broken-install detection (#2742)', () => {
|
||||
// A fake `codex` on PATH that reproduces the real failure: node's spawn dump
|
||||
// on stderr, non-zero exit. `mode` picks which failure shape to emit.
|
||||
function shimHome(mode: 'enoent' | 'notexec' | 'timeout' | 'model400' | 'oksuspicious') {
|
||||
const home = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-codex-shim-'));
|
||||
const bin = path.join(home, 'bin');
|
||||
fs.mkdirSync(bin, { recursive: true });
|
||||
// auth.json so the auth probe passes and we reach the model probe.
|
||||
fs.mkdirSync(path.join(home, '.codex'), { recursive: true });
|
||||
fs.writeFileSync(path.join(home, '.codex/auth.json'), '{}');
|
||||
const bodies: Record<string, string> = {
|
||||
enoent:
|
||||
`echo "Error: spawn /x/vendor/aarch64-apple-darwin/codex/codex ENOENT" >&2\n` +
|
||||
`echo " errno: -2, code: 'ENOENT'" >&2\nexit 1\n`,
|
||||
notexec: `echo "bash: codex: cannot execute binary file" >&2\nexit 126\n`,
|
||||
timeout: `echo "network hiccup" >&2\nexit 124\n`,
|
||||
model400: `echo "The 'gpt-x' model is not supported when using Codex with a ChatGPT account" >&2\nexit 1\n`,
|
||||
oksuspicious: `echo "OK — note: the log you pasted mentions permission denied on /var/log"\nexit 0\n`,
|
||||
};
|
||||
fs.writeFileSync(path.join(bin, 'codex'), `#!/usr/bin/env bash\n${bodies[mode]}`, { mode: 0o755 });
|
||||
return { home, bin };
|
||||
}
|
||||
|
||||
const cases: Array<[string, 'enoent' | 'notexec', string]> = [
|
||||
['spawn ENOENT', 'enoent', 'ENOENT'],
|
||||
['non-executable binary (exit 126)', 'notexec', 'cannot execute binary file'],
|
||||
];
|
||||
|
||||
for (const [label, mode, needle] of cases) {
|
||||
test(`${label} is classified as a broken install, not a transient`, () => {
|
||||
const { home, bin } = shimHome(mode);
|
||||
try {
|
||||
const r = runProbe({
|
||||
snippet: '_gstack_codex_model_probe; echo "EXIT:$?"',
|
||||
home,
|
||||
env: { PATH: `${bin}:${process.env.PATH ?? ''}`, GSTACK_HOME: home },
|
||||
});
|
||||
expect(r.stdout).toContain('MODEL_UNUSABLE_INSTALL');
|
||||
// Exit 2 is what lets the preflight tell this apart from a model 400.
|
||||
expect(r.stdout).toContain('EXIT:2');
|
||||
// It must NOT fail open — that was the whole defect.
|
||||
expect(r.stdout).not.toContain('MODEL_PROBE_INCONCLUSIVE');
|
||||
// The remedy names the install, not the model pin.
|
||||
expect(r.stdout).toContain('npm install -g @openai/codex');
|
||||
expect(r.stdout.toLowerCase()).toContain(needle.toLowerCase());
|
||||
} finally {
|
||||
fs.rmSync(home, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
test('a broken install is never cached — a reinstall is picked up next probe', () => {
|
||||
const { home, bin } = shimHome('enoent');
|
||||
try {
|
||||
runProbe({
|
||||
snippet: '_gstack_codex_model_probe >/dev/null 2>&1',
|
||||
home,
|
||||
env: { PATH: `${bin}:${process.env.PATH ?? ''}`, GSTACK_HOME: home },
|
||||
});
|
||||
const cache = path.join(home, '.codex-model-probe');
|
||||
if (fs.existsSync(cache)) {
|
||||
expect(fs.readFileSync(cache, 'utf8')).not.toContain('MODEL_OK');
|
||||
}
|
||||
} finally {
|
||||
fs.rmSync(home, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('a genuine transient (exit 124) still fails open', () => {
|
||||
const { home, bin } = shimHome('timeout');
|
||||
try {
|
||||
const r = runProbe({
|
||||
snippet: '_gstack_codex_model_probe; echo "EXIT:$?"',
|
||||
home,
|
||||
env: { PATH: `${bin}:${process.env.PATH ?? ''}`, GSTACK_HOME: home },
|
||||
});
|
||||
expect(r.stdout).toContain('MODEL_PROBE_INCONCLUSIVE');
|
||||
expect(r.stdout).toContain('EXIT:0');
|
||||
expect(r.stdout).not.toContain('MODEL_UNUSABLE_INSTALL');
|
||||
} finally {
|
||||
fs.rmSync(home, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('the model 400 still classifies as MODEL_UNUSABLE, not a broken install', () => {
|
||||
const { home, bin } = shimHome('model400');
|
||||
try {
|
||||
const r = runProbe({
|
||||
snippet: '_gstack_codex_model_probe; echo "EXIT:$?"',
|
||||
home,
|
||||
env: { PATH: `${bin}:${process.env.PATH ?? ''}`, GSTACK_HOME: home },
|
||||
});
|
||||
expect(r.stdout).toContain('MODEL_UNUSABLE');
|
||||
expect(r.stdout).not.toContain('MODEL_UNUSABLE_INSTALL');
|
||||
expect(r.stdout).toContain('EXIT:1');
|
||||
} finally {
|
||||
fs.rmSync(home, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('version check warns instead of returning silently when codex cannot report a version', () => {
|
||||
const { home, bin } = shimHome('enoent');
|
||||
try {
|
||||
const r = runProbe({
|
||||
snippet: '_gstack_codex_version_check; echo "EXIT:$?"',
|
||||
home,
|
||||
env: { PATH: `${bin}:${process.env.PATH ?? ''}`, GSTACK_HOME: home },
|
||||
});
|
||||
// Previously this printed nothing: `codex --version 2>/dev/null | head -1`
|
||||
// captured head's status, so a CLI that only ever errored read as healthy.
|
||||
expect(r.stdout).toContain('WARN');
|
||||
expect(r.stdout).toContain('npm install -g @openai/codex');
|
||||
// Still non-fatal — the version check has never gated anything.
|
||||
expect(r.stdout).toContain('EXIT:0');
|
||||
} finally {
|
||||
fs.rmSync(home, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
// Wave-amended (#2745 absorption): string signatures only count on a FAILED
|
||||
// spawn — a SUCCESSFUL response whose text mentions "permission denied"
|
||||
// (e.g. the model quoting a log the user pasted) must stay healthy.
|
||||
test('exit-0 response mentioning "permission denied" is NOT a broken install', () => {
|
||||
const { home, bin } = shimHome('oksuspicious');
|
||||
try {
|
||||
const r = runProbe({
|
||||
snippet: '_gstack_codex_model_probe; echo "EXIT:$?"',
|
||||
home,
|
||||
env: { PATH: `${bin}:${process.env.PATH ?? ''}`, GSTACK_HOME: home },
|
||||
});
|
||||
expect(r.stdout).not.toContain('MODEL_UNUSABLE_INSTALL');
|
||||
expect(r.stdout).toContain('EXIT:0');
|
||||
} finally {
|
||||
fs.rmSync(home, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
// Wave-amended (#2745 absorption): autoplan's preflight chain is the one
|
||||
// hand-maintained copy that isn't resolver-generated — it must capture the
|
||||
// probe's exit code and route 2 to its own broken-install arm, or /autoplan
|
||||
// prints the wrong remedy for a broken binary.
|
||||
test('autoplan preflight (tmpl + rendered) captures the probe exit and routes 2 to broken-install', () => {
|
||||
for (const rel of ['autoplan/SKILL.md.tmpl', 'autoplan/SKILL.md']) {
|
||||
const src = fs.readFileSync(path.join(ROOT, rel), 'utf-8');
|
||||
expect(src).toContain('_gstack_codex_model_probe; _CODEX_MP=$?');
|
||||
expect(src).toMatch(/_CODEX_MP" -eq 2/);
|
||||
expect(src).toContain('binary cannot run');
|
||||
expect(src).not.toContain('elif ! _gstack_codex_model_probe');
|
||||
}
|
||||
});
|
||||
|
||||
test('the preflight resolver routes exit 2 to broken_install', () => {
|
||||
const src = fs.readFileSync(path.join(ROOT, 'scripts/resolvers/constants.ts'), 'utf8');
|
||||
expect(src).toContain('broken_install');
|
||||
// The chain must capture the probe's code; `elif ! _gstack_codex_model_probe`
|
||||
// collapses 1 and 2 into one branch and loses the distinction.
|
||||
expect(src).toContain('_CODEX_MP=$?');
|
||||
});
|
||||
});
|
||||
|
||||
+11
-3
@@ -2148,10 +2148,17 @@ elif ! command -v codex >/dev/null 2>&1; then
|
||||
_CODEX_MODE="not_installed"; _gstack_codex_log_event "codex_cli_missing" 2>/dev/null || true
|
||||
elif ! _gstack_codex_auth_probe >/dev/null 2>&1; then
|
||||
_CODEX_MODE="not_authed"; _gstack_codex_log_event "codex_auth_failed" 2>/dev/null || true
|
||||
elif ! _gstack_codex_model_probe; then
|
||||
_CODEX_MODE="model_unusable"
|
||||
else
|
||||
_CODEX_MODE="ready"; _gstack_codex_version_check 2>/dev/null || true
|
||||
# Capture the probe's code: 2 means the CLI cannot execute at all, which is a
|
||||
# different problem (and a different fix) from a model the account can't use.
|
||||
_gstack_codex_model_probe; _CODEX_MP=$?
|
||||
if [ "$_CODEX_MP" -eq 2 ]; then
|
||||
_CODEX_MODE="broken_install"
|
||||
elif [ "$_CODEX_MP" -ne 0 ]; then
|
||||
_CODEX_MODE="model_unusable"
|
||||
else
|
||||
_CODEX_MODE="ready"; _gstack_codex_version_check 2>/dev/null || true
|
||||
fi
|
||||
fi
|
||||
echo "CODEX_MODE: $_CODEX_MODE"
|
||||
```
|
||||
@@ -2161,6 +2168,7 @@ Branch on the echoed `CODEX_MODE`:
|
||||
- **`not_installed`** — Codex CLI absent. Print: "Codex not installed — using Claude subagent. Install for cross-model coverage: `npm install -g @openai/codex`." Fall back to the Claude subagent path.
|
||||
- **`under_codex`** — this session is already running INSIDE a Codex host, so spawning codex again is the same model reviewing itself at multiplied token cost (#2519). Print exactly one line: "[running under Codex — nested codex passes skipped; set GSTACK_FORCE_CODEX_REVIEW=1 to force]" and skip the codex invocations below; run the section's free in-host pass instead if it defines one.
|
||||
- **`not_authed`** — installed but no credentials. Print: "Codex installed but not authenticated — using Claude subagent. Run `codex login` or set `$CODEX_API_KEY`." Fall back to the Claude subagent path.
|
||||
- **`broken_install`** — the CLI is on PATH but cannot execute (spawn ENOENT, non-executable binary, missing vendor payload). Print: "Codex is installed but its binary cannot run — Codex passes skipped. Reinstall: `npm install -g @openai/codex`." Relay the probe's HINT lines and fall back to the Claude subagent path. This state exists because a missing binary used to land in the model probe's fail-open bucket and report `ready`, so every Codex pass was skipped silently (#2742).
|
||||
- **`model_unusable`** — authed but the account cannot use its configured model (#2477: HTTP 400 on every call, usually a stale `model =` pin in `~/.codex/config.toml`). Relay the probe's HINT lines, tell the user the one-line fix (update the pin; `[notice.model_migrations]` names the replacement), and fall back to the Claude subagent path. The ~10s round trip is cached for 1h; timeouts fail open to `ready`.
|
||||
- **`ready`** — run the Codex pass below.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user