Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions bin/gstack-paths
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions codex/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
10 changes: 5 additions & 5 deletions codex/SKILL.md.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
85 changes: 85 additions & 0 deletions test/regression-issue2091-codex-bsd-mktemp.test.ts
Original file line number Diff line number Diff line change
@@ -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('/');
});
});