From d1de0831fc239b539b59e4f0b92570bf258e5b8c Mon Sep 17 00:00:00 2001 From: Shaked Eyal Date: Wed, 24 Jun 2026 12:36:18 +0300 Subject: [PATCH] fix(codex): BSD-safe temp-file creation on macOS (mktemp suffix + trailing-slash TMP_ROOT) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On macOS, /codex (all three modes) failed before Codex ever ran: the mktemp calls that create the stderr/prompt/response temp files errored out, the bash block exited 1, and the user got no review. Two compounding, independent bugs. 1. Suffix after the placeholder. The codex skill used templates like `mktemp "$TMP_ROOT/codex-err-XXXXXX.txt"` (also codex-prompt / codex-resp). GNU mktemp tolerates a suffix after the X run, but BSD mktemp (macOS) does not substitute the X's at all when the placeholder isn't at the end — so call #1 creates a LITERAL `codex-err-XXXXXX.txt` (exit 0) and a later call (a second /codex run, a stale leftover, or a concurrent worktree) fails with `mkstemp failed: File exists` and aborts. The failure is state-dependent, which is why some macOS users saw /codex "work." Fix: move the placeholder to the end of every codex mktemp template (drop the `.txt` suffix) in codex/SKILL.md.tmpl, regenerate codex/SKILL.md. 2. Trailing slash in TMP_ROOT. bin/gstack-paths emitted TMP_ROOT straight from $TMPDIR, which on macOS ends in `/` (e.g. /var/folders/.../T/), so "$TMP_ROOT/codex-err-..." expanded with a double slash. Fix: strip the trailing slash at the source so every consumer benefits, not just /codex (a bare "/" is preserved, not collapsed to empty). Both fixes are needed; the suffix bug is independent of the slash bug. Adds test/regression-issue2091-codex-bsd-mktemp.test.ts: a static invariant that no codex mktemp template carries a suffix after XXXXXX (asserted across both the .tmpl and the generated SKILL.md so regen drift can't reopen it), plus runtime checks that gstack-paths normalizes the trailing slash. Reported-by: Scott Hardin (@scotthardin) Refs #2091 Co-Authored-By: Claude Opus 4.8 (1M context) --- bin/gstack-paths | 9 ++ codex/SKILL.md | 10 +-- codex/SKILL.md.tmpl | 10 +-- ...ression-issue2091-codex-bsd-mktemp.test.ts | 85 +++++++++++++++++++ 4 files changed, 104 insertions(+), 10 deletions(-) create mode 100644 test/regression-issue2091-codex-bsd-mktemp.test.ts diff --git a/bin/gstack-paths b/bin/gstack-paths index 1a7e07306..a2682aa44 100755 --- a/bin/gstack-paths +++ b/bin/gstack-paths @@ -56,6 +56,15 @@ else _tmp_root=".gstack/tmp" fi +# Strip any trailing slash so consumers can safely concatenate "$TMP_ROOT/name" +# without producing a double slash. On macOS $TMPDIR ends in `/` by default +# (e.g. /var/folders/.../T/), which would otherwise yield paths like +# `…/T//codex-err-…`. Normalizing at the source means every consumer benefits, +# not just /codex. +_tmp_root="${_tmp_root%/}" +# A value of "/" collapses to "" above; restore it so TMP_ROOT is never empty. +[ -z "$_tmp_root" ] && _tmp_root="/" + # Best-effort mkdir; if it fails (read-only fs, permission denied), the caller # will discover that on their own write attempt. Don't fail the eval here. mkdir -p "$_tmp_root" 2>/dev/null || true diff --git a/codex/SKILL.md b/codex/SKILL.md index cd40e075d..a5738575f 100644 --- a/codex/SKILL.md +++ b/codex/SKILL.md @@ -938,7 +938,7 @@ Run Codex code review against the current branch diff. 1. Create temp files for output capture: ```bash -TMPERR=$(mktemp "$TMP_ROOT/codex-err-XXXXXX.txt") +TMPERR=$(mktemp "$TMP_ROOT/codex-err-XXXXXX") ``` 2. Run the review (5-minute timeout). **Codex CLI ≥ 0.130.0 rejects passing a @@ -986,7 +986,7 @@ when the diff content is adversarial: _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } cd "$_REPO_ROOT" _USER_INSTRUCTIONS="" -_PROMPT_FILE=$(mktemp "$TMP_ROOT/codex-prompt-XXXXXX.txt") +_PROMPT_FILE=$(mktemp "$TMP_ROOT/codex-prompt-XXXXXX") { printf '%s\n' "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. Do NOT modify agents/openai.yaml. Stay focused on repository code only." printf '\nCustom focus: %s\n\n' "$_USER_INSTRUCTIONS" @@ -1237,7 +1237,7 @@ if [ -z "$PYTHON_CMD" ]; then fi # Fix 1+2: wrap with timeout (gtimeout/timeout fallback chain via probe helper), # capture stderr to $TMPERR for auth error detection (was: 2>/dev/null). -TMPERR=${TMPERR:-$(mktemp "$TMP_ROOT/codex-err-XXXXXX.txt")} +TMPERR=${TMPERR:-$(mktemp "$TMP_ROOT/codex-err-XXXXXX")} _gstack_codex_timeout_wrapper 600 codex exec "" -C "$_REPO_ROOT" -s read-only -c 'model_reasoning_effort="high"' --enable web_search_cached --json < /dev/null 2>"$TMPERR" | PYTHONUNBUFFERED=1 "$PYTHON_CMD" -u -c " import sys, json turn_completed_count = 0 @@ -1336,8 +1336,8 @@ B) Start a new conversation 2. Create temp files: ```bash -TMPRESP=$(mktemp "$TMP_ROOT/codex-resp-XXXXXX.txt") -TMPERR=$(mktemp "$TMP_ROOT/codex-err-XXXXXX.txt") +TMPRESP=$(mktemp "$TMP_ROOT/codex-resp-XXXXXX") +TMPERR=$(mktemp "$TMP_ROOT/codex-err-XXXXXX") ``` 3. **Plan review auto-detection:** If the user's prompt is about reviewing a plan, diff --git a/codex/SKILL.md.tmpl b/codex/SKILL.md.tmpl index 333de7d8d..afb8b38a7 100644 --- a/codex/SKILL.md.tmpl +++ b/codex/SKILL.md.tmpl @@ -158,7 +158,7 @@ Run Codex code review against the current branch diff. 1. Create temp files for output capture: ```bash -TMPERR=$(mktemp "$TMP_ROOT/codex-err-XXXXXX.txt") +TMPERR=$(mktemp "$TMP_ROOT/codex-err-XXXXXX") ``` 2. Run the review (5-minute timeout). **Codex CLI ≥ 0.130.0 rejects passing a @@ -206,7 +206,7 @@ when the diff content is adversarial: _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } cd "$_REPO_ROOT" _USER_INSTRUCTIONS="" -_PROMPT_FILE=$(mktemp "$TMP_ROOT/codex-prompt-XXXXXX.txt") +_PROMPT_FILE=$(mktemp "$TMP_ROOT/codex-prompt-XXXXXX") { printf '%s\n' "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. Do NOT modify agents/openai.yaml. Stay focused on repository code only." printf '\nCustom focus: %s\n\n' "$_USER_INSTRUCTIONS" @@ -335,7 +335,7 @@ if [ -z "$PYTHON_CMD" ]; then fi # Fix 1+2: wrap with timeout (gtimeout/timeout fallback chain via probe helper), # capture stderr to $TMPERR for auth error detection (was: 2>/dev/null). -TMPERR=${TMPERR:-$(mktemp "$TMP_ROOT/codex-err-XXXXXX.txt")} +TMPERR=${TMPERR:-$(mktemp "$TMP_ROOT/codex-err-XXXXXX")} _gstack_codex_timeout_wrapper 600 codex exec "" -C "$_REPO_ROOT" -s read-only -c 'model_reasoning_effort="high"' --enable web_search_cached --json < /dev/null 2>"$TMPERR" | PYTHONUNBUFFERED=1 "$PYTHON_CMD" -u -c " import sys, json turn_completed_count = 0 @@ -434,8 +434,8 @@ B) Start a new conversation 2. Create temp files: ```bash -TMPRESP=$(mktemp "$TMP_ROOT/codex-resp-XXXXXX.txt") -TMPERR=$(mktemp "$TMP_ROOT/codex-err-XXXXXX.txt") +TMPRESP=$(mktemp "$TMP_ROOT/codex-resp-XXXXXX") +TMPERR=$(mktemp "$TMP_ROOT/codex-err-XXXXXX") ``` 3. **Plan review auto-detection:** If the user's prompt is about reviewing a plan, diff --git a/test/regression-issue2091-codex-bsd-mktemp.test.ts b/test/regression-issue2091-codex-bsd-mktemp.test.ts new file mode 100644 index 000000000..16d4b4be8 --- /dev/null +++ b/test/regression-issue2091-codex-bsd-mktemp.test.ts @@ -0,0 +1,85 @@ +/** + * Regression tests for issue #2091 — `/codex` temp-file creation fails on macOS + * (BSD mktemp). Diagnosed by Scott Hardin. + * + * Two compounding bugs, both in gstack: + * + * 1. Suffix after the placeholder. The codex skill used templates like + * `mktemp "$TMP_ROOT/codex-err-XXXXXX.txt"`. GNU mktemp tolerates a suffix + * after the X run; BSD mktemp (macOS) does NOT — it does not substitute the + * X's at all, so call #1 creates a LITERAL `codex-err-XXXXXX.txt` (exit 0) + * and a later call (a second /codex run, a stale leftover, or a concurrent + * worktree) fails with `mkstemp failed: File exists` and aborts the review. + * Fixed by moving the placeholder to the END of every codex mktemp template. + * + * 2. Trailing slash in TMP_ROOT. `bin/gstack-paths` emitted TMP_ROOT straight + * from $TMPDIR, which on macOS ends in `/` (e.g. /var/folders/.../T/), + * producing a double-slash path (`…/T//codex-err-…`). Fixed by stripping + * the trailing slash at the source so every consumer benefits, not just + * /codex. + * + * Both bugs are independent; these static + runtime checks pin each one so + * template drift can't silently re-introduce them. + */ +import { describe, test, expect } from 'bun:test'; +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as path from 'path'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const PATHS_BIN = path.join(ROOT, 'bin', 'gstack-paths'); + +// ── Bug 1: BSD mktemp requires the X placeholder at the END of the template ── +// Asserted across both the .tmpl source and the generated SKILL.md so a regen +// drift (or a hand-edit of one but not the other) can't reopen the bug. +describe('#2091 bug 1: codex mktemp templates are BSD-safe (X placeholder at end)', () => { + for (const relPath of ['codex/SKILL.md.tmpl', 'codex/SKILL.md']) { + test(`${relPath}: no codex mktemp template has a suffix after XXXXXX`, () => { + const content = fs.readFileSync(path.join(ROOT, relPath), 'utf-8'); + // Every quoted mktemp template that targets a codex temp file. + const templates = [...content.matchAll(/mktemp\s+"([^"]*codex-[^"]*)"/g)].map(m => m[1]); + // Sanity: the templates still exist (guards against the regex silently + // matching nothing after a future refactor, which would pass vacuously). + expect(templates.length).toBeGreaterThan(0); + // BSD mktemp only substitutes a run of X's that is the LAST thing in the + // template. Anything after the final X (e.g. `.txt`) is the bug. + const offenders = templates.filter(t => !/X{6,}$/.test(t)); + expect(offenders).toEqual([]); + }); + } +}); + +// ── Bug 2: gstack-paths normalizes TMP_ROOT (no trailing slash) ────────────── +// Mirrors the invocation contract used by test/gstack-paths.test.ts: the helper +// is always sourced from a bash block, so we run it via `bash`. +function tmpRoot(env: Record): string { + const result = spawnSync('bash', [PATHS_BIN], { + env: { PATH: process.env.PATH, USERPROFILE: '', ...env } as Record, + encoding: 'utf-8', + }); + if (result.status !== 0) { + throw new Error(`gstack-paths failed (status ${result.status}): ${result.stderr}`); + } + for (const line of result.stdout.split('\n')) { + if (line.startsWith('TMP_ROOT=')) return line.slice('TMP_ROOT='.length); + } + throw new Error('gstack-paths did not emit TMP_ROOT'); +} + +describe('#2091 bug 2: gstack-paths strips the trailing slash from TMP_ROOT', () => { + test('macOS-style TMPDIR with trailing slash → trailing slash stripped', () => { + expect(tmpRoot({ TMPDIR: '/var/folders/ab/T/', HOME: '/tmp/h' })).toBe('/var/folders/ab/T'); + }); + + test('TMP (Windows/container fallback) with trailing slash is also normalized', () => { + expect(tmpRoot({ TMP: '/tmp/y/', HOME: '/tmp/h' })).toBe('/tmp/y'); + }); + + test('a path without a trailing slash is left unchanged', () => { + expect(tmpRoot({ TMPDIR: '/tmp/x', HOME: '/tmp/h' })).toBe('/tmp/x'); + }); + + test('a bare "/" does not collapse to empty', () => { + expect(tmpRoot({ TMPDIR: '/', HOME: '/tmp/h' })).toBe('/'); + }); +});