diff --git a/bin/gstack-developer-profile b/bin/gstack-developer-profile index 1b0594630..a5b1ab771 100755 --- a/bin/gstack-developer-profile +++ b/bin/gstack-developer-profile @@ -55,7 +55,7 @@ do_migrate() { # Run migration in a temp file, then atomic rename. local TMPOUT - TMPOUT=$(mktemp "$GSTACK_HOME/developer-profile.json.XXXXXX.tmp") + TMPOUT=$(mktemp "$GSTACK_HOME/developer-profile.json.tmp.XXXXXX") trap 'rm -f "$TMPOUT"' EXIT cat "$LEGACY_FILE" | bun -e " @@ -182,7 +182,7 @@ do_log_session() { ensure_profile local TMPOUT - TMPOUT=$(mktemp "$GSTACK_HOME/developer-profile.json.XXXXXX.tmp") + TMPOUT=$(mktemp "$GSTACK_HOME/developer-profile.json.tmp.XXXXXX") trap 'rm -f "$TMPOUT"' EXIT PROFILE_FILE_PATH="$PROFILE_FILE" RECORD_INPUT="$INPUT" TMPOUT_PATH="$TMPOUT" bun -e " 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/claude/SKILL.md.tmpl b/claude/SKILL.md.tmpl index 94552cbe4..e109f21f7 100644 --- a/claude/SKILL.md.tmpl +++ b/claude/SKILL.md.tmpl @@ -95,8 +95,8 @@ Create temp files: ```bash PROMPT_FILE=$(mktemp /tmp/gstack-claude-prompt-XXXXXX) -RESP_FILE=$(mktemp /tmp/gstack-claude-response-XXXXXX.json) -ERR_FILE=$(mktemp /tmp/gstack-claude-error-XXXXXX.txt) +RESP_FILE=$(mktemp /tmp/gstack-claude-response-XXXXXX) +ERR_FILE=$(mktemp /tmp/gstack-claude-error-XXXXXX) ``` Cleanup at the end of every mode: @@ -151,7 +151,7 @@ Review the current branch diff with nested Claude in tool-less mode. ```bash _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } cd "$_REPO_ROOT" -DIFF_FILE=$(mktemp /tmp/gstack-claude-diff-XXXXXX.patch) +DIFF_FILE=$(mktemp /tmp/gstack-claude-diff-XXXXXX) git fetch origin --quiet 2>/dev/null || true git diff "origin/" > "$DIFF_FILE" 2>/dev/null || git diff "" > "$DIFF_FILE" ``` diff --git a/codex/SKILL.md b/codex/SKILL.md index c06d3affa..26283809f 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/office-hours/SKILL.md b/office-hours/SKILL.md index d81d89f2d..31d7736b2 100644 --- a/office-hours/SKILL.md +++ b/office-hours/SKILL.md @@ -1329,7 +1329,7 @@ If B: skip Phase 3.5 entirely. Remember that the second opinion did NOT run (aff 2. **Write the assembled prompt to a temp file** (prevents shell injection from user-derived content): ```bash -CODEX_PROMPT_FILE=$(mktemp /tmp/gstack-codex-oh-XXXXXXXX.txt) +CODEX_PROMPT_FILE=$(mktemp /tmp/gstack-codex-oh-XXXXXXXX) ``` Write the full prompt to this file. **Always start with the filesystem boundary:** diff --git a/scripts/resolvers/review.ts b/scripts/resolvers/review.ts index 7dccd8e50..b708c90a3 100644 --- a/scripts/resolvers/review.ts +++ b/scripts/resolvers/review.ts @@ -351,7 +351,7 @@ If B: skip Phase 3.5 entirely. Remember that the second opinion did NOT run (aff 2. **Write the assembled prompt to a temp file** (prevents shell injection from user-derived content): \`\`\`bash -CODEX_PROMPT_FILE=$(mktemp /tmp/gstack-codex-oh-XXXXXXXX.txt) +CODEX_PROMPT_FILE=$(mktemp /tmp/gstack-codex-oh-XXXXXXXX) \`\`\` Write the full prompt to this file. **Always start with the filesystem boundary:** diff --git a/test/regression-issue2091-bsd-mktemp.test.ts b/test/regression-issue2091-bsd-mktemp.test.ts new file mode 100644 index 000000000..4b858dbdf --- /dev/null +++ b/test/regression-issue2091-bsd-mktemp.test.ts @@ -0,0 +1,165 @@ +/** + * Regression tests for issue #2091 / #2370 — mktemp templates fail on macOS + * (BSD mktemp) and Alpine (busybox mktemp) when the X placeholder run is not + * the LAST thing in the template. + * + * Two compounding bugs, both in gstack: + * + * 1. Suffix after the placeholder. Skills 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. + * busybox mktemp (Alpine) rejects the template outright on the FIRST run. + * Fixed by moving the placeholder to the END of every 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. + * + * Bug 1's tripwire is repo-wide: it sweeps EVERY .tmpl, every SKILL.md, and + * every scripts/resolvers/*.ts (the sources that feed generated skills), so a + * new skill can't re-introduce the suffix shape anywhere. + */ +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 ── +// Swept across all skills (templates AND generated output) plus the resolver +// modules that feed generated sections, so neither a hand-edit nor a regen +// drift can reopen the bug. + +/** Directories that are not gstack-authored skill/template sources. */ +const SKIP_DIRS = new Set([ + 'node_modules', + '.git', + 'dist', + '.claude', + '.agents', + '.factory', + 'fixtures', // test/fixtures — goldens snapshot generated output separately +]); + +function collectScannedFiles(dir: string, out: string[]): void { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + if (SKIP_DIRS.has(entry.name)) continue; + const full = path.join(dir, entry.name); + // Symlinks (e.g. connect-chrome → open-gstack-browser) would double-count + // or escape the tree; the link target is scanned via its real path. + if (entry.isSymbolicLink()) continue; + if (entry.isDirectory()) { + collectScannedFiles(full, out); + } else if ( + entry.name === 'SKILL.md' || + entry.name.endsWith('.tmpl') || + (full.includes(path.join('scripts', 'resolvers')) && entry.name.endsWith('.ts')) + ) { + out.push(full); + } + } +} + +/** + * Extract every mktemp template token on a line and return the ones whose + * X-run (4+ X's) is NOT the final character run. Tokens are whitespace-split + * words after `mktemp`, stripped of shell/TS quoting and closers, so + * `$(mktemp "$TMP_ROOT/codex-err-XXXXXX.txt")` yields the offending + * `$TMP_ROOT/codex-err-XXXXXX.txt`. + */ +function offendingTemplatesOnLine(line: string): string[] { + const idx = line.indexOf('mktemp'); + if (idx === -1) return []; + const offenders: string[] = []; + for (const raw of line.slice(idx).split(/\s+/)) { + const token = raw.replace(/^["'(`]+|[)"'`;,\\}]+$/g, ''); + if (!/X{4,}/.test(token)) continue; + if (!/X{4,}$/.test(token)) offenders.push(token); + } + return offenders; +} + +describe('#2091/#2370 bug 1: every mktemp template is BSD-safe (X placeholder at end)', () => { + const files: string[] = []; + collectScannedFiles(ROOT, files); + + test('scan sweep finds the known mktemp call sites (not vacuous)', () => { + // Guards against the walker silently matching nothing after a refactor. + const withMktemp = files.filter((f) => fs.readFileSync(f, 'utf-8').includes('mktemp')); + expect(withMktemp.length).toBeGreaterThanOrEqual(5); + expect(withMktemp).toContain(path.join(ROOT, 'codex', 'SKILL.md.tmpl')); + expect(withMktemp).toContain(path.join(ROOT, 'codex', 'SKILL.md')); + expect(withMktemp).toContain(path.join(ROOT, 'scripts', 'resolvers', 'review.ts')); + }); + + test('no .tmpl, SKILL.md, or resolver carries a suffix after the X-run', () => { + const violations: string[] = []; + for (const file of files) { + const lines = fs.readFileSync(file, 'utf-8').split('\n'); + lines.forEach((line, i) => { + for (const token of offendingTemplatesOnLine(line)) { + violations.push(`${path.relative(ROOT, file)}:${i + 1} — ${token}`); + } + }); + } + expect(violations).toEqual([]); + }); + + test('the offender-detector itself still detects the original bug shapes', () => { + // Self-test so a regex tweak can't quietly blind the tripwire. + expect(offendingTemplatesOnLine('TMPERR=$(mktemp "$TMP_ROOT/codex-err-XXXXXX.txt")')).toEqual([ + '$TMP_ROOT/codex-err-XXXXXX.txt', + ]); + expect( + offendingTemplatesOnLine('TMPOUT=$(mktemp "$GSTACK_HOME/developer-profile.json.XXXXXX.tmp")'), + ).toEqual(['$GSTACK_HOME/developer-profile.json.XXXXXX.tmp']); + expect(offendingTemplatesOnLine('RESP_FILE=$(mktemp /tmp/gstack-claude-response-XXXXXX.json)')).toEqual([ + '/tmp/gstack-claude-response-XXXXXX.json', + ]); + // Fixed shapes pass. + expect(offendingTemplatesOnLine('TMPERR=$(mktemp "$TMP_ROOT/codex-err-XXXXXX")')).toEqual([]); + expect(offendingTemplatesOnLine('ALL_JSONL=$(mktemp -t autoplan-tasks.XXXXXXXX)')).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('/'); + }); +});