RomanNum3ral
/
gstack
mirror of https://github.com/garrytan/gstack.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(codex): BSD-safe temp-file creation on macOS (mktemp suffix + trailing-slash TMP_ROOT) 

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) <noreply@anthropic.com>
This commit is contained in:
Shaked Eyal 2026-06-24 12:36:18 +03:00
parent 9fd03fae9e
commit d1de0831fc
4 changed files with 104 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
bin/gstack-paths
View File

@ -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

10
codex/SKILL.md
View File

@ -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="<everything after '/codex review ' in user input>"
_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 "<prompt>" -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,

10
codex/SKILL.md.tmpl
View File

@ -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="<everything after '/codex review ' in user input>"
_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 "<prompt>" -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,

85
test/regression-issue2091-codex-bsd-mktemp.test.ts Normal file
View File

@ -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, string | undefined>): string {
const result = spawnSync('bash', [PATHS_BIN], {
env: { PATH: process.env.PATH, USERPROFILE: '', ...env } as Record<string, string>,
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('/');
});
});