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 33228ff9b..d5617335d 100644 --- a/codex/SKILL.md +++ b/codex/SKILL.md @@ -967,7 +967,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 @@ -1015,7 +1015,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" @@ -1266,7 +1266,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 @@ -1365,8 +1365,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('/'); + }); +});