mirror of https://github.com/garrytan/gstack.git
fix(mktemp): move the X-run to the end of every temp-file template (BSD/busybox safe)
BSD mktemp (macOS) does not substitute an X-run that has a suffix after it: `mktemp "$TMP_ROOT/codex-err-XXXXXX.txt"` creates a LITERAL codex-err-XXXXXX.txt on the first call (exit 0) and every later call fails with `mkstemp failed: File exists` — so /codex breaks from the SECOND run on every Mac, masquerading as a model stall. busybox mktemp (Alpine) rejects the template on the first run. Fixes #2091, #2370. Union of both community fixes, compared at the diff level: - PR #2372: all 11 source sites with a suffix after the X-run — codex SKILL.md.tmpl (5), claude SKILL.md.tmpl (3), bin/gstack-developer-profile (2, suffix folded into the prefix: .json.tmp.XXXXXX), and the office-hours codex pass in scripts/resolvers/review.ts (1). - PR #2103: the second half of #2091 — bin/gstack-paths now strips the trailing slash from TMP_ROOT at the source (macOS $TMPDIR ends in `/`), plus runtime tests pinning that normalization. New repo-wide tripwire in test/regression-issue2091-bsd-mktemp.test.ts: every .tmpl, every SKILL.md, and every scripts/resolvers/*.ts is swept — no mktemp template may carry a suffix after the X-run, with a self-test so the detector can't be quietly blinded. Generated SKILL.md files regenerated via gen:skill-docs in this commit. Contributed by @ShuratCode (PR #2103) and @noron12234 (PR #2372); PR #2285 by @cathrynlavery covered a subset. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
parent
a4daa352d6
commit
7a8e39d2cb
|
|
@ -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 "
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 <base> --quiet 2>/dev/null || true
|
||||
git diff "origin/<base>" > "$DIFF_FILE" 2>/dev/null || git diff "<base>" > "$DIFF_FILE"
|
||||
```
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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:**
|
||||
|
|
|
|||
|
|
@ -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:**
|
||||
|
|
|
|||
|
|
@ -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, 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('/');
|
||||
});
|
||||
});
|
||||
Loading…
Reference in New Issue