Merge d1de0831fc into 2beb636f7c

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

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="<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"
@@ -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 "<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
@@ -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,

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="<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,

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, 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('/');
+  });
+});