From 47906b052d1d22b9660e4e66b6ea45b7a393571c Mon Sep 17 00:00:00 2001 From: Behruz Nassre Esfahani Date: Wed, 24 Jun 2026 14:27:45 -0700 Subject: [PATCH] fix(#1520): randomize mktemp temp paths on BSD/macOS (XXXXXX must be path-final) (#1550) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#1520): randomize mktemp temp paths on BSD/macOS (XXXXXX must be path-final) BSD/macOS mktemp only substitutes the XXXXXX template when it is the final path component. Templates like `...-XXXXXX.json` / `gsd-pr-body.XXXXXX.md` return a LITERAL `XXXXXX` path (no randomization) on macOS, so concurrent workflow runs collide on the same temp manifest/body file — one run can overwrite or consume another's. Reproduced on macOS: the second call to the suffixed template fails `mkstemp: File exists`. Fix: use a suffixless `XXXXXX` template (so it IS the final component), then rename to add the intended extension — portable across BSD + GNU userlands, no GNU-only `--suffix` flag. Empty-file-then-write semantics are preserved at every site. Affected workflow temp files: - execute-phase.md: gsd-worktree-wave-*.json (wave worktree manifest) - quick.md: gsd-quick-worktree-*.json - spec-phase.md: edge-probe-reqs-*.json - ship.md: gsd-pr-body-*.md - profile-user.md: gsd-profile-answers-*.json, gsd-profile-analysis-*.json The execute-phase.md edit uses a compact intermediate var + trailing comment to stay under the ADR-857 phase-6 size ceiling (93166); regenerated the workflow size baseline accordingly. Validated on macOS: 20 concurrent calls yield 20 unique randomized paths. Co-Authored-By: Claude Opus 4.8 (1M context) * chore(#1520): add changeset fragment (Fixed) Co-Authored-By: Claude Opus 4.8 (1M context) * test(#1520): add fail-first workflow-prose guard for mktemp XXXXXX suffix Repo-wide scan of gsd-core/workflows/**/*.md that fails on any mktemp template whose XXXXXX run is followed by a filename suffix (the BSD/macOS non-randomizing form). Fails on the six pre-fix instances and passes on the fix, and locks the copy-paste-prone idiom out of future workflows. Mirrors the bug-637 hardcoded-$HOME workflow guard. Co-Authored-By: Claude Opus 4.8 (1M context) * test(#1520): rename regression test to fix- prefix (regression-test-names lint) New tests/bug-NNNN-*.test.cjs files are banned by the lint-regression-test-names ratchet; use the fix- prefix (matches the fix-1445 precedent). Co-Authored-By: Claude Opus 4.8 (1M context) * test(#1520): add issue ref to allow-test-rule exemption (ADR-456 lint) lint-allow-test-rule-refs requires every new `allow-test-rule:` comment to carry a #NNN reference (don't allowlist). Add (#1520) to the source-text exemption. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(#1520): abort touched mktemp chains on failure (|| exit 1) Per review: the VAR=$(mktemp …) && mv … && VAR=… chains dropped the issue's suggested failure guard. If mktemp fails, $VAR is empty and the subsequent mv/write lands on an unintended relative path. Add `|| exit 1` to all six touched chains so a mktemp failure aborts the snippet. Regenerated the workflow size baseline for the slightly longer lines. Co-Authored-By: Claude Opus 4.8 (1M context) * chore(#1520): rebase onto next — regen size baseline + describe rename Resolve the workflow-size-baseline.json conflict from next advancing by regenerating from the current workflow sizes. Also rename the test describe from `bug #1520` to `#1520` (the file uses the fix- prefix) per review nit. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(#1520): harmonize profile-user mktemp to ${TMPDIR:-/tmp} (review nit) The two profile-user.md temp sites this PR already rewrites kept a hardcoded /tmp while the four sibling workflows use ${TMPDIR:-/tmp}. Harmonize for consistency and macOS-correctness (some sandboxes have no writable /tmp). Regenerated the workflow size baseline. Co-Authored-By: Claude Opus 4.8 (1M context) * chore(#1520): regen size baseline after rebase onto next --------- Co-authored-by: Claude Opus 4.8 (1M context) Co-authored-by: Tom Boucher --- .../1520-mktemp-suffix-randomization.md | 5 ++ gsd-core/workflows/execute-phase.md | 2 +- gsd-core/workflows/profile-user.md | 8 +- gsd-core/workflows/quick.md | 4 +- gsd-core/workflows/ship.md | 4 +- gsd-core/workflows/spec-phase.md | 4 +- ...1520-workflow-mktemp-suffix-final.test.cjs | 78 +++++++++++++++++++ tests/workflow-size-baseline.json | 10 +-- 8 files changed, 104 insertions(+), 11 deletions(-) create mode 100644 .changeset/1520-mktemp-suffix-randomization.md create mode 100644 tests/fix-1520-workflow-mktemp-suffix-final.test.cjs diff --git a/.changeset/1520-mktemp-suffix-randomization.md b/.changeset/1520-mktemp-suffix-randomization.md new file mode 100644 index 000000000..d3137c715 --- /dev/null +++ b/.changeset/1520-mktemp-suffix-randomization.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1550 +--- +**Workflow temp files now randomize correctly on BSD/macOS** — several workflows called `mktemp` with templates where `XXXXXX` was followed by a `.json`/`.md` suffix (e.g. `gsd-worktree-wave-XXXXXX.json`, `gsd-pr-body.XXXXXX.md`). BSD/macOS `mktemp` only substitutes `XXXXXX` when it is the final path component, so those templates returned a literal, non-randomized path, letting concurrent workflow runs collide on the same temp manifest/body file (one run overwriting or consuming another's). The fix creates a suffixless temp then renames to add the extension — portable across BSD + GNU. Affected: `execute-phase`, `quick`, `spec-phase`, `ship`, `profile-user`. (#1520) diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index 99416e5de..155fccfc8 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -580,7 +580,7 @@ increases monotonically across waves. `{status}` is `complete` (success), DISPATCH_TS=$(date -u +"%Y-%m-%dT%H:%M:%SZ") EXPECTED_BRANCH=$(git rev-parse --abbrev-ref HEAD) if [ "${USE_WORKTREES_FOR_PLAN:-true}" != "false" ] && [ -z "${WAVE_WORKTREE_MANIFEST:-}" ]; then - WAVE_WORKTREE_MANIFEST=$(mktemp "${TMPDIR:-/tmp}/gsd-worktree-wave-XXXXXX.json") + M=$(mktemp "${TMPDIR:-/tmp}/gsd-worktree-wave-XXXXXX") && mv "$M" "$M.json" && WAVE_WORKTREE_MANIFEST="$M.json" || exit 1 # XXXXXX must be path-final on BSD/macOS (#1520) # Persist the dispatch-time orchestrator worktree root so wave-cleanup can pin back to the # orchestrator's OWN worktree — NOT `git worktree list`'s first entry (always the main # checkout), which pins a non-primary (per-phase lane) orchestrator off its branch (#630). diff --git a/gsd-core/workflows/profile-user.md b/gsd-core/workflows/profile-user.md index 3ef88d7eb..7d723cf1b 100644 --- a/gsd-core/workflows/profile-user.md +++ b/gsd-core/workflows/profile-user.md @@ -218,7 +218,9 @@ Collect all answers into an answers JSON object mapping dimension keys to select **Save answers to temp file:** ```bash -ANSWERS_PATH=$(mktemp /tmp/gsd-profile-answers-XXXXXX.json) +# BSD/macOS mktemp only randomizes XXXXXX when it is the final path component, so make a +# suffixless temp then append the extension — portable across BSD + GNU (#1520). +ANSWERS_PATH=$(mktemp "${TMPDIR:-/tmp}/gsd-profile-answers-XXXXXX") && mv "$ANSWERS_PATH" "${ANSWERS_PATH}.json" && ANSWERS_PATH="${ANSWERS_PATH}.json" || exit 1 ``` Write the answers JSON to `$ANSWERS_PATH`. @@ -232,7 +234,9 @@ Parse the analysis JSON from the result. Save analysis JSON to a temp file: ```bash -ANALYSIS_PATH=$(mktemp /tmp/gsd-profile-analysis-XXXXXX.json) +# BSD/macOS mktemp only randomizes XXXXXX when it is the final path component, so make a +# suffixless temp then append the extension — portable across BSD + GNU (#1520). +ANALYSIS_PATH=$(mktemp "${TMPDIR:-/tmp}/gsd-profile-analysis-XXXXXX") && mv "$ANALYSIS_PATH" "${ANALYSIS_PATH}.json" && ANALYSIS_PATH="${ANALYSIS_PATH}.json" || exit 1 ``` Write the analysis JSON to `$ANALYSIS_PATH`. diff --git a/gsd-core/workflows/quick.md b/gsd-core/workflows/quick.md index a4e7ec3e0..cc6b03614 100644 --- a/gsd-core/workflows/quick.md +++ b/gsd-core/workflows/quick.md @@ -675,7 +675,9 @@ Capture current HEAD before spawning (used for worktree branch check): ```bash EXPECTED_BASE=$(git rev-parse HEAD) if [ "${USE_WORKTREES:-true}" != "false" ]; then - QUICK_WORKTREE_MANIFEST=$(mktemp "${TMPDIR:-/tmp}/gsd-quick-worktree-XXXXXX.json") + # BSD/macOS mktemp only randomizes XXXXXX when it is the final path component, so make a + # suffixless temp then append the extension — portable across BSD + GNU (#1520). + QUICK_WORKTREE_MANIFEST=$(mktemp "${TMPDIR:-/tmp}/gsd-quick-worktree-XXXXXX") && mv "$QUICK_WORKTREE_MANIFEST" "${QUICK_WORKTREE_MANIFEST}.json" && QUICK_WORKTREE_MANIFEST="${QUICK_WORKTREE_MANIFEST}.json" || exit 1 printf '{"worktrees":[]}\n' > "$QUICK_WORKTREE_MANIFEST" export QUICK_WORKTREE_MANIFEST fi diff --git a/gsd-core/workflows/ship.md b/gsd-core/workflows/ship.md index ff966789f..845bf4189 100644 --- a/gsd-core/workflows/ship.md +++ b/gsd-core/workflows/ship.md @@ -280,7 +280,9 @@ Use the exact key order `skill=`, `fallback=`, `exempt=`, `missing=` so downstre Create the PR using the generated body. Write the body to a temp file first so large generated PRD sections do not hit shell argument limits: ```bash -PR_BODY_FILE=$(mktemp "${TMPDIR:-/tmp}/gsd-pr-body.XXXXXX.md") +# BSD/macOS mktemp only randomizes XXXXXX when it is the final path component, so make a +# suffixless temp then append the extension — portable across BSD + GNU (#1520). +PR_BODY_FILE=$(mktemp "${TMPDIR:-/tmp}/gsd-pr-body-XXXXXX") && mv "$PR_BODY_FILE" "${PR_BODY_FILE}.md" && PR_BODY_FILE="${PR_BODY_FILE}.md" || exit 1 trap 'rm -f "${PR_BODY_FILE:-}"' EXIT printf '%s\n' "${PR_BODY}" > "${PR_BODY_FILE}" diff --git a/gsd-core/workflows/spec-phase.md b/gsd-core/workflows/spec-phase.md index 06c876d53..119fbf53c 100644 --- a/gsd-core/workflows/spec-phase.md +++ b/gsd-core/workflows/spec-phase.md @@ -235,7 +235,9 @@ fi # canonical coverage compute. Populate the heredoc from the SPEC's Requirements — one object # per requirement: {"id","text","shapes"?}. This is the load-bearing step: an empty file makes # the probe a no-op, so the guard below fails loud rather than silently skipping (RR-04). -REQS_JSON=$(mktemp "${TMPDIR:-/tmp}/edge-probe-reqs-XXXXXX.json") +# BSD/macOS mktemp only randomizes XXXXXX when it is the final path component, so make a +# suffixless temp then append the extension — portable across BSD + GNU (#1520). +REQS_JSON=$(mktemp "${TMPDIR:-/tmp}/edge-probe-reqs-XXXXXX") && mv "$REQS_JSON" "${REQS_JSON}.json" && REQS_JSON="${REQS_JSON}.json" || exit 1 cat > "$REQS_JSON" <<'JSON' [ { "id": "R1", "text": "" } diff --git a/tests/fix-1520-workflow-mktemp-suffix-final.test.cjs b/tests/fix-1520-workflow-mktemp-suffix-final.test.cjs new file mode 100644 index 000000000..1e904a5f4 --- /dev/null +++ b/tests/fix-1520-workflow-mktemp-suffix-final.test.cjs @@ -0,0 +1,78 @@ +// allow-test-rule: source-text-is-the-product (#1520) +// Workflow .md text IS what the runtime loads and the agent executes, so +// asserting on its shell invocations tests the deployed contract directly. +// +// Repo-wide regression guard for #1520: NO workflow .md may invoke `mktemp` +// with a template whose `XXXXXX` run is followed by a filename suffix +// (e.g. `…-XXXXXX.json`, `…-XXXXXX.md`). BSD/macOS `mktemp` only substitutes +// the `X` run when it is the FINAL path component; a trailing suffix yields a +// literal, non-randomized path, so concurrent workflow runs collide on the same +// temp file (one run overwriting or consuming another's). The portable fix is +// `mktemp …-XXXXXX` (suffix-less) then `mv` to add the extension. +// +// This is a copy-paste-prone shell idiom — the same defect first shipped across +// five workflows before #1520 — so a prose guard is the right lock-out, mirroring +// the bug-637 hardcoded-$HOME workflow scan. + +'use strict'; + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const WORKFLOWS_DIR = path.join(__dirname, '..', 'gsd-core', 'workflows'); + +// Match `mktemp ` where, within the single whitespace-delimited template +// token, a maximal run of 3+ `X` is immediately followed by a filename +// character (`.`, alnum, `-`, `_`) — i.e. a suffix the BSD/macOS substitution +// can't reach. +// - `\s+` requires an argument (bare `mktemp` is fine — path-final +// is N/A — and prose like "mktemp only randomizes XXXXXX" +// is excluded because the X-run is in a later token). +// - `["']?\S*?` walks within the one quoted/unquoted template token. +// - `X{3,}(?!X)` anchors on the WHOLE X-run (so `XXXXXX)` does not match +// via a sub-run leaving a trailing `X`). +// - `[.A-Za-z0-9_-]` the offending suffix char. A legitimate path-final form +// ends the token with `"`, `'`, whitespace, or `)`, none of +// which are in this class. +const SUFFIXED_MKTEMP_TEMPLATE = /mktemp\s+["']?\S*?X{3,}(?!X)[.A-Za-z0-9_-]/; + +function collectWorkflowMarkdown(dir) { + const out = []; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + out.push(...collectWorkflowMarkdown(full)); + } else if (entry.isFile() && entry.name.endsWith('.md')) { + out.push(full); + } + } + return out; +} + +describe('#1520: workflow mktemp templates keep XXXXXX path-final', () => { + test('no gsd-core/workflows/**/*.md calls mktemp with a suffix after the XXXXXX run', () => { + const files = collectWorkflowMarkdown(WORKFLOWS_DIR); + assert.ok(files.length > 0, 'expected workflow markdown files to exist'); + + const offenders = []; + for (const file of files) { + const lines = fs.readFileSync(file, 'utf8').split(/\r?\n/); + lines.forEach((line, i) => { + if (SUFFIXED_MKTEMP_TEMPLATE.test(line)) { + offenders.push(`${path.relative(WORKFLOWS_DIR, file)}:${i + 1}: ${line.trim()}`); + } + }); + } + + assert.deepStrictEqual( + offenders, + [], + 'Workflow mktemp templates must keep XXXXXX as the final path component ' + + '(create suffix-less, then `mv` to add the extension) so BSD/macOS ' + + 'randomizes the path. Offenders:\n' + + offenders.join('\n'), + ); + }); +}); diff --git a/tests/workflow-size-baseline.json b/tests/workflow-size-baseline.json index 1ed65362b..6e785faa5 100644 --- a/tests/workflow-size-baseline.json +++ b/tests/workflow-size-baseline.json @@ -24,7 +24,7 @@ "docs-update.md": 55662, "edit-phase.md": 12883, "eval-review.md": 9923, - "execute-phase.md": 93426, + "execute-phase.md": 93517, "execute-plan.md": 32611, "explore.md": 10497, "extract-learnings.md": 12849, @@ -56,9 +56,9 @@ "plan-review-convergence.md": 23468, "plant-seed.md": 11741, "pr-branch.md": 15919, - "profile-user.md": 20650, + "profile-user.md": 21202, "progress.md": 30555, - "quick.md": 48830, + "quick.md": 49139, "reapply-patches.md": 20393, "remove-phase.md": 8469, "remove-workspace.md": 7507, @@ -70,10 +70,10 @@ "settings-advanced.md": 39666, "settings-integrations.md": 15848, "settings.md": 33413, - "ship.md": 24388, + "ship.md": 24647, "sketch-wrap-up.md": 14223, "sketch.md": 19960, - "spec-phase.md": 31503, + "spec-phase.md": 31752, "spike-wrap-up.md": 15092, "spike.md": 24517, "stats.md": 6718,