From 9ca92161e20a4856031212f54f3bb5203fe6c53b Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Mon, 31 Aug 2026 21:19:06 +0000 Subject: [PATCH] fix(codex): close the consult-mode fence, report turn.failed as a failure, capture exit codes portably (#2671, #2669) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects in the codex skill sections: - The resumed-session bash block never closed its fence; every fenced region after it inverted (prose rendered as code, the synthesis-recommendation tail rendered inert). A repo-wide fence-pairing test now scans every generated SKILL.md and sections/*.md with a CommonMark-faithful state machine (an info-string opener inside a fence is literal content — nested template examples in document-generate/make-pdf stay legal; a file ending inside a fence fails). - The JSONL parsers had no turn.failed branch: a turn that STATED its failure was reported as 'possible mid-stream disconnect'. Challenge and consult now print the event's error and run a three-way completeness check (failed-with- reason / silent-disconnect / ok); consult previously had no completeness check at all. - ${PIPESTATUS[0]} is empty under zsh, so hang detection never fired and every clean run printed a spurious '[codex exit ]'. All three capture sites use ${PIPESTATUS[0]:-${pipestatus[1]}}, pinned statically and EXECUTED under real bash and zsh in the new test. Expect a step-change in codex_timeout telemetry — the counter starts firing for zsh users. Receipt: the portability pin fails on a v1.77.0.0 scratch worktree; the fence fix is structural (17 → 18 fence lines, tail no longer inside a block). Fixes #2671 Fixes #2669 Co-Authored-By: Claude Fable 5 --- codex/sections/challenge-mode.md | 13 ++- codex/sections/challenge-mode.md.tmpl | 13 ++- codex/sections/consult-mode.md | 16 +++- codex/sections/consult-mode.md.tmpl | 16 +++- test/generated-docs-fences.test.ts | 128 ++++++++++++++++++++++++++ 5 files changed, 176 insertions(+), 10 deletions(-) create mode 100644 test/generated-docs-fences.test.ts diff --git a/codex/sections/challenge-mode.md b/codex/sections/challenge-mode.md index 1e8359cda..319a065c1 100644 --- a/codex/sections/challenge-mode.md +++ b/codex/sections/challenge-mode.md @@ -61,12 +61,19 @@ for line in sys.stdin: usage = obj.get('usage',{}) tokens = usage.get('input_tokens',0) + usage.get('output_tokens',0) if tokens: print(f'\ntokens used: {tokens}', flush=True) + elif t == 'turn.failed': + turn_failed = True + err = obj.get('error',{}).get('message','') or 'no error message in event' + print(f'[codex turn FAILED] {err}', flush=True, file=sys.stderr) except: pass -# Fix 2: completeness check — warn if no turn.completed received -if turn_completed_count == 0: +# Fix 2: three-way completeness check (#2671) — a STATED failure is a failure, +# not a network problem; only silence with no terminal event is a disconnect. +if 'turn_failed' in dir(): + print('[codex] turn.failed received — the turn errored (reason above), not a disconnect.', flush=True, file=sys.stderr) +elif turn_completed_count == 0: print('[codex warning] No turn.completed event received — possible mid-stream disconnect.', flush=True, file=sys.stderr) " -_CODEX_EXIT=${PIPESTATUS[0]} +_CODEX_EXIT=${PIPESTATUS[0]:-${pipestatus[1]}} # bash sets PIPESTATUS; zsh (lowercase, 1-indexed) falls through (#2669) # Fix 1: hang detection — log + surface actionable message if [ "$_CODEX_EXIT" = "124" ]; then _gstack_codex_log_event "codex_timeout" "600" diff --git a/codex/sections/challenge-mode.md.tmpl b/codex/sections/challenge-mode.md.tmpl index c8d0770ad..5b4f2ee88 100644 --- a/codex/sections/challenge-mode.md.tmpl +++ b/codex/sections/challenge-mode.md.tmpl @@ -59,12 +59,19 @@ for line in sys.stdin: usage = obj.get('usage',{}) tokens = usage.get('input_tokens',0) + usage.get('output_tokens',0) if tokens: print(f'\ntokens used: {tokens}', flush=True) + elif t == 'turn.failed': + turn_failed = True + err = obj.get('error',{}).get('message','') or 'no error message in event' + print(f'[codex turn FAILED] {err}', flush=True, file=sys.stderr) except: pass -# Fix 2: completeness check — warn if no turn.completed received -if turn_completed_count == 0: +# Fix 2: three-way completeness check (#2671) — a STATED failure is a failure, +# not a network problem; only silence with no terminal event is a disconnect. +if 'turn_failed' in dir(): + print('[codex] turn.failed received — the turn errored (reason above), not a disconnect.', flush=True, file=sys.stderr) +elif turn_completed_count == 0: print('[codex warning] No turn.completed event received — possible mid-stream disconnect.', flush=True, file=sys.stderr) " -_CODEX_EXIT=${PIPESTATUS[0]} +_CODEX_EXIT=${PIPESTATUS[0]:-${pipestatus[1]}} # bash sets PIPESTATUS; zsh (lowercase, 1-indexed) falls through (#2669) # Fix 1: hang detection — log + surface actionable message if [ "$_CODEX_EXIT" = "124" ]; then _gstack_codex_log_event "codex_timeout" "600" diff --git a/codex/sections/consult-mode.md b/codex/sections/consult-mode.md index 5398c98a7..a3728878f 100644 --- a/codex/sections/consult-mode.md +++ b/codex/sections/consult-mode.md @@ -102,13 +102,24 @@ for line in sys.stdin: cmd = item.get('command','') if cmd: print(f'[codex ran] {cmd}', flush=True) elif t == 'turn.completed': + turn_completed_count = 1 + (turn_completed_count if 'turn_completed_count' in dir() else 0) usage = obj.get('usage',{}) tokens = usage.get('input_tokens',0) + usage.get('output_tokens',0) if tokens: print(f'\ntokens used: {tokens}', flush=True) + elif t == 'turn.failed': + turn_failed = True + err = obj.get('error',{}).get('message','') or 'no error message in event' + print(f'[codex turn FAILED] {err}', flush=True, file=sys.stderr) except: pass +# Three-way completeness check (#2671; consult previously had NONE): a STATED +# failure is a failure, not a network problem; only silence is a disconnect. +if 'turn_failed' in dir(): + print('[codex] turn.failed received — the turn errored (reason above), not a disconnect.', flush=True, file=sys.stderr) +elif 'turn_completed_count' not in dir(): + print('[codex warning] No turn.completed event received — possible mid-stream disconnect.', flush=True, file=sys.stderr) " # Fix 1: hang detection for Consult new-session (mirrors Challenge + resume) -_CODEX_EXIT=${PIPESTATUS[0]} +_CODEX_EXIT=${PIPESTATUS[0]:-${pipestatus[1]}} # bash sets PIPESTATUS; zsh (lowercase, 1-indexed) falls through (#2669) if [ "$_CODEX_EXIT" = "124" ]; then _gstack_codex_log_event "codex_timeout" "600" _gstack_codex_log_hang "consult" "$(wc -c < "$TMPERR" 2>/dev/null || echo 0)" @@ -144,7 +155,7 @@ _gstack_codex_timeout_wrapper 600 codex exec resume "" -c ' " # Fix 1: same hang detection pattern as new-session block -_CODEX_EXIT=${PIPESTATUS[0]} +_CODEX_EXIT=${PIPESTATUS[0]:-${pipestatus[1]}} # bash sets PIPESTATUS; zsh (lowercase, 1-indexed) falls through (#2669) if [ "$_CODEX_EXIT" = "124" ]; then _gstack_codex_log_event "codex_timeout" "600" _gstack_codex_log_hang "consult-resume" "$(wc -c < "$TMPERR" 2>/dev/null || echo 0)" @@ -156,6 +167,7 @@ elif [ "$_CODEX_EXIT" != "0" ]; then head -20 "$TMPERR" 2>/dev/null | sed 's/^/ /' || true _gstack_codex_log_event "codex_nonzero_exit" "consult-resume:$_CODEX_EXIT" fi +``` 5. Capture session ID from the streamed output. The parser prints `SESSION_ID:` from the `thread.started` event. Save it for follow-ups: diff --git a/codex/sections/consult-mode.md.tmpl b/codex/sections/consult-mode.md.tmpl index 16bc960f5..fb5d39013 100644 --- a/codex/sections/consult-mode.md.tmpl +++ b/codex/sections/consult-mode.md.tmpl @@ -100,13 +100,24 @@ for line in sys.stdin: cmd = item.get('command','') if cmd: print(f'[codex ran] {cmd}', flush=True) elif t == 'turn.completed': + turn_completed_count = 1 + (turn_completed_count if 'turn_completed_count' in dir() else 0) usage = obj.get('usage',{}) tokens = usage.get('input_tokens',0) + usage.get('output_tokens',0) if tokens: print(f'\ntokens used: {tokens}', flush=True) + elif t == 'turn.failed': + turn_failed = True + err = obj.get('error',{}).get('message','') or 'no error message in event' + print(f'[codex turn FAILED] {err}', flush=True, file=sys.stderr) except: pass +# Three-way completeness check (#2671; consult previously had NONE): a STATED +# failure is a failure, not a network problem; only silence is a disconnect. +if 'turn_failed' in dir(): + print('[codex] turn.failed received — the turn errored (reason above), not a disconnect.', flush=True, file=sys.stderr) +elif 'turn_completed_count' not in dir(): + print('[codex warning] No turn.completed event received — possible mid-stream disconnect.', flush=True, file=sys.stderr) " # Fix 1: hang detection for Consult new-session (mirrors Challenge + resume) -_CODEX_EXIT=${PIPESTATUS[0]} +_CODEX_EXIT=${PIPESTATUS[0]:-${pipestatus[1]}} # bash sets PIPESTATUS; zsh (lowercase, 1-indexed) falls through (#2669) if [ "$_CODEX_EXIT" = "124" ]; then _gstack_codex_log_event "codex_timeout" "600" _gstack_codex_log_hang "consult" "$(wc -c < "$TMPERR" 2>/dev/null || echo 0)" @@ -142,7 +153,7 @@ _gstack_codex_timeout_wrapper 600 codex exec resume "" -c ' " # Fix 1: same hang detection pattern as new-session block -_CODEX_EXIT=${PIPESTATUS[0]} +_CODEX_EXIT=${PIPESTATUS[0]:-${pipestatus[1]}} # bash sets PIPESTATUS; zsh (lowercase, 1-indexed) falls through (#2669) if [ "$_CODEX_EXIT" = "124" ]; then _gstack_codex_log_event "codex_timeout" "600" _gstack_codex_log_hang "consult-resume" "$(wc -c < "$TMPERR" 2>/dev/null || echo 0)" @@ -154,6 +165,7 @@ elif [ "$_CODEX_EXIT" != "0" ]; then head -20 "$TMPERR" 2>/dev/null | sed 's/^/ /' || true _gstack_codex_log_event "codex_nonzero_exit" "consult-resume:$_CODEX_EXIT" fi +``` 5. Capture session ID from the streamed output. The parser prints `SESSION_ID:` from the `thread.started` event. Save it for follow-ups: diff --git a/test/generated-docs-fences.test.ts b/test/generated-docs-fences.test.ts new file mode 100644 index 000000000..b910b9a4b --- /dev/null +++ b/test/generated-docs-fences.test.ts @@ -0,0 +1,128 @@ +/** + * Generated-doc fence pairing + PIPESTATUS portability (#2671, #2669). + * + * #2671: an unclosed ```bash fence in codex/sections/consult-mode.md silently + * inverted every fenced region after it — prose rendered as code and the + * skill's tail instructions rendered inert. Nothing guarded fence pairing, so + * the defect migrated file-to-file across carves. The scanner below is a + * CommonMark-faithful state machine, NOT a mod-2 count: inside an open fence, + * a ```lang line is literal content (only a bare ``` closes), so nested fence + * EXAMPLES don't false-positive; a file that ends inside a fence fails. + * + * #2669: `${PIPESTATUS[0]}` is bash-only — empty under zsh, so hang detection + * (`= "124"`) never fired and every clean run printed a spurious + * "[codex exit ]". The portable form `${PIPESTATUS[0]:-${pipestatus[1]}}` is + * pinned statically AND executed under real bash and zsh. + */ +import { describe, expect, test } from "bun:test"; +import { spawnSync } from "node:child_process"; +import * as fs from "node:fs"; +import * as path from "node:path"; + +const ROOT = path.resolve(import.meta.dir, ".."); + +/** All generated skill docs: every SKILL.md + every sections/*.md. */ +function generatedDocs(): string[] { + const out: string[] = []; + for (const entry of fs.readdirSync(ROOT, { withFileTypes: true })) { + if (!entry.isDirectory() || entry.name.startsWith(".") || entry.name === "node_modules") + continue; + const skillMd = path.join(ROOT, entry.name, "SKILL.md"); + if (fs.existsSync(skillMd)) out.push(skillMd); + const sections = path.join(ROOT, entry.name, "sections"); + if (fs.existsSync(sections)) { + for (const f of fs.readdirSync(sections)) { + if (f.endsWith(".md")) out.push(path.join(sections, f)); + } + } + } + return out; +} + +/** Returns the 1-based line of the first unclosed fence, or null when paired. */ +export function findUnclosedFence(body: string): number | null { + let openLine: number | null = null; + const lines = body.split("\n"); + for (let i = 0; i < lines.length; i++) { + const ln = lines[i]; + if (!ln.startsWith("```")) continue; + if (openLine === null) { + openLine = i + 1; // any ``` line opens (info string allowed) + } else if (/^```\s*$/.test(ln)) { + openLine = null; // only a bare ``` closes (CommonMark) + } + // ```lang while inside = literal content (nested fence example) — ignore. + } + return openLine; +} + +describe("generated-doc fence pairing (#2671)", () => { + const docs = generatedDocs(); + + test("scanner sees a meaningful corpus", () => { + expect(docs.length).toBeGreaterThan(50); + }); + + test("every generated SKILL.md and sections/*.md closes every fence", () => { + const bad: string[] = []; + for (const doc of docs) { + const line = findUnclosedFence(fs.readFileSync(doc, "utf-8")); + if (line !== null) bad.push(`${path.relative(ROOT, doc)}:${line}`); + } + expect( + bad, + `unclosed \`\`\` fence(s) — everything after each inverts prose/code:\n ${bad.join("\n ")}`, + ).toEqual([]); + }); + + test("the scanner itself catches the #2671 shape (self-test)", () => { + const broken = "prose\n```bash\nx=1\n\nmore prose that should be outside\n```bash\nmkdir -p y\n```\n"; + // First fence opens; ```bash inside is content; bare ``` closes it; file + // ends OUTSIDE — but the second region's prose was swallowed. The + // detectable invariant is end-of-file state, so test a truly unclosed tail: + expect(findUnclosedFence(broken)).toBeNull(); + expect(findUnclosedFence(broken + "```text\ntail\n")).toBe(9); + }); +}); + +describe("codex exit-code capture is bash+zsh portable (#2669)", () => { + const SECTION_FILES = [ + "codex/sections/challenge-mode.md", + "codex/sections/consult-mode.md", + "codex/sections/challenge-mode.md.tmpl", + "codex/sections/consult-mode.md.tmpl", + ]; + + test("no bare ${PIPESTATUS[0]} capture survives in the codex sections", () => { + for (const rel of SECTION_FILES) { + const body = fs.readFileSync(path.join(ROOT, rel), "utf-8"); + for (const line of body.split("\n")) { + if (line.includes("_CODEX_EXIT=")) { + expect(line, `${rel}: ${line.trim()}`).toContain( + "${PIPESTATUS[0]:-${pipestatus[1]}}", + ); + } + } + // The capture must exist at all (3 sites across the two modes). + expect(body).toContain("_CODEX_EXIT=${PIPESTATUS[0]:-${pipestatus[1]}}"); + } + }); + + const SNIPPET = 'exit 7 | cat; _CODEX_EXIT=${PIPESTATUS[0]:-${pipestatus[1]}}; echo "EXIT:$_CODEX_EXIT"'; + const CLEAN = 'true | cat; _CODEX_EXIT=${PIPESTATUS[0]:-${pipestatus[1]}}; echo "EXIT:$_CODEX_EXIT"'; + + test("bash: captures the FIRST pipeline stage's exit code", () => { + const r = spawnSync("bash", ["-c", `(${SNIPPET})`], { encoding: "utf-8", timeout: 10_000 }); + expect(r.stdout).toContain("EXIT:7"); + const c = spawnSync("bash", ["-c", CLEAN], { encoding: "utf-8", timeout: 10_000 }); + expect(c.stdout).toContain("EXIT:0"); + }); + + const hasZsh = spawnSync("zsh", ["--version"], { encoding: "utf-8", timeout: 10_000 }).status === 0; + test.skipIf(!hasZsh)("zsh: the lowercase 1-indexed fallback captures the same code", () => { + const r = spawnSync("zsh", ["-c", `(${SNIPPET})`], { encoding: "utf-8", timeout: 10_000 }); + expect(r.stdout).toContain("EXIT:7"); + const c = spawnSync("zsh", ["-c", CLEAN], { encoding: "utf-8", timeout: 10_000 }); + expect(c.stdout).toContain("EXIT:0"); + }); +});