Merge pull request #3216 from gsd-build/fix/ci-bug-2136-sh-hook-version

fix(build-hooks): atomic rename to prevent race with concurrent install reads

.changeset/build-hooks-atomic-write.md

@@ -0,0 +1,5 @@
+---
+type: Fixed
+pr: 3216
+---
+**Atomic writes in `scripts/build-hooks.js` to fix flaky release CI** — nine test files invoke `build-hooks.js` from their `before()` hooks, and `scripts/run-tests.cjs` runs test files with `--test-concurrency=4`, so multiple builders raced to rewrite the same files in `hooks/dist/`. `fs.copyFileSync(src, dest)` truncates `dest` then writes it; a parallel `bin/install.js` subprocess (spawned by another install test) could `fs.readFileSync` between the truncate and the write and observe an empty file. install.js then wrote that empty content into the install target, so installed `.sh` hooks lacked their `# gsd-hook-version:` header. This surfaced as the release-blocking failure in `tests/bug-2136-sh-hook-version.test.cjs` part 4 even though the same SHA passed on every other Node-22/Node-24 install-smoke matrix run. `build-hooks.js` now stages each output to a sibling `hooks/.dist-staging/` directory (same filesystem as `hooks/dist/`) and uses `fs.renameSync` to swap into place — POSIX `rename(2)` is atomic, so concurrent readers always observe a complete file. The existing `tests/bug-2136-sh-hook-version.test.cjs` part 4 already locks the post-fix invariant. (Failing run: https://github.com/gsd-build/get-shit-done/actions/runs/25472202941/job/74738276687)

.gitignore

@@ -14,6 +14,9 @@ commands.html
 # Build artifacts (committed to npm, not git)
 hooks/dist/
 
+# Atomic-write staging dir used by scripts/build-hooks.js (see comment there)
+hooks/.dist-staging/
+
 # Coverage artifacts
 coverage/
 

CHANGELOG.md

@@ -8,6 +8,7 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ### Fixed
 
+- **Atomic writes in `scripts/build-hooks.js` to fix flaky release CI** — nine test files invoke `build-hooks.js` from their `before()` hooks, and `scripts/run-tests.cjs` runs test files with `--test-concurrency=4`, so multiple builders raced to rewrite the same files in `hooks/dist/`. `fs.copyFileSync(src, dest)` truncates `dest` then writes it; a parallel `bin/install.js` subprocess (spawned by another install test) could `fs.readFileSync` between the truncate and the write and observe an empty file. install.js then wrote that empty content into the install target, so installed `.sh` hooks lacked their `# gsd-hook-version:` header. This surfaced as the release-blocking failure in `tests/bug-2136-sh-hook-version.test.cjs` part 4 even though the same SHA passed on every other Node-22/Node-24 install-smoke matrix run. `build-hooks.js` now stages each output to a sibling `hooks/.dist-staging/` directory (same filesystem as `hooks/dist/`) and uses `fs.renameSync` to swap into place — POSIX `rename(2)` is atomic, so concurrent readers always observe a complete file. (Failing run: https://github.com/gsd-build/get-shit-done/actions/runs/25472202941/job/74738276687)
 - **Stable node path on Homebrew** — `resolveNodeRunner()` now maps versioned Homebrew Cellar paths (e.g. `/usr/local/Cellar/node/25.8.1/bin/node`) to the stable Homebrew symlinks (`/usr/local/bin/node` on Intel, `/opt/homebrew/bin/node` on Apple Silicon). `rewriteLegacyManagedNodeHookCommands()` applies the same normalization to baked Cellar paths in existing hook commands. This prevents `dyld: Library not loaded` errors after `brew upgrade node`. (#3181)
 - **Milestone-archive layout support** — `validate consistency`, `validate health`, and `find-phase` now scan `.planning/milestones/v*-phases/` directories in addition to the flat `.planning/phases/` layout. Projects that have graduated to milestone-archive layout no longer receive spurious W006 "Phase N in ROADMAP.md but no directory on disk" warnings for every active phase. (#3164)
 

scripts/build-hooks.js

@@ -12,6 +12,12 @@ const vm = require('vm');
 
 const HOOKS_DIR = path.join(__dirname, '..', 'hooks');
 const DIST_DIR = path.join(HOOKS_DIR, 'dist');
+// Sibling directory used to stage atomic writes. Lives under hooks/ so it
+// shares a filesystem with DIST_DIR (POSIX rename(2) is only atomic within
+// the same filesystem) but is NOT inside DIST_DIR — so readers that
+// readdirSync(DIST_DIR) (e.g. bin/install.js, install-hooks-copy tests)
+// never observe a transient ".tmp" sibling file there.
+const STAGE_DIR = path.join(HOOKS_DIR, '.dist-staging');
 
 // Hooks to copy (pure Node.js, no bundling needed)
 const HOOKS_TO_COPY = [
@@ -30,6 +36,61 @@ const HOOKS_TO_COPY = [
   'gsd-phase-boundary.sh'
 ];
 
+// Sync millisecond sleep using Atomics.wait on a throwaway SharedArrayBuffer.
+// Used between Windows rename retries; this script is sync end-to-end so
+// setTimeout would not work. Total worst-case backoff across MAX_ATTEMPTS
+// is bounded (~400ms) — acceptable for a one-shot build script.
+function sleepSync(ms) {
+  Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms);
+}
+
+/**
+ * Atomic-replace via fs.renameSync, with Windows-only retry and fallback.
+ *
+ * POSIX rename(2) atomically replaces dest even when readers hold open
+ * handles on it. Windows MoveFileEx (which fs.renameSync uses with
+ * MOVEFILE_REPLACE_EXISTING) cannot — it throws EPERM/EBUSY when another
+ * process has the destination open. Concurrent install.js readers and
+ * antivirus scanners are the realistic triggers; both release handles
+ * within milliseconds, so a short backoff resolves the race. After
+ * retries are exhausted, fall back to copy-then-unlink (re-introduces
+ * the truncate-then-write race for this single file but keeps the build
+ * moving rather than crashing). If even copy fails because dest is hard-
+ * locked, log a non-fatal warning and leave the prior dest in place — a
+ * subsequent build invocation will retry from a fresh state.
+ */
+function renameAtomicWithRetry(stagedDest, dest, hook) {
+  if (process.platform !== 'win32') {
+    fs.renameSync(stagedDest, dest);
+    return;
+  }
+  const BACKOFFS_MS = [10, 30, 90, 270];
+  for (let attempt = 0; attempt <= BACKOFFS_MS.length; attempt++) {
+    try {
+      fs.renameSync(stagedDest, dest);
+      return;
+    } catch (e) {
+      const transient = e && (e.code === 'EPERM' || e.code === 'EBUSY');
+      if (!transient) throw e;
+      if (attempt < BACKOFFS_MS.length) {
+        sleepSync(BACKOFFS_MS[attempt]);
+        continue;
+      }
+      // Retries exhausted; fall back to copy-then-unlink.
+      try {
+        fs.copyFileSync(stagedDest, dest);
+        try { fs.unlinkSync(stagedDest); } catch (_) { /* tolerate */ }
+        console.warn(`\x1b[33m! ${hook}: rename failed (${e.code}) after ${BACKOFFS_MS.length} retries; used copy-fallback\x1b[0m`);
+        return;
+      } catch (fallbackErr) {
+        try { fs.unlinkSync(stagedDest); } catch (_) { /* tolerate */ }
+        console.warn(`\x1b[33m! ${hook}: rename + copy fallback both failed (${e.code} → ${fallbackErr.code || fallbackErr.message}); leaving prior dest in place\x1b[0m`);
+        return;
+      }
+    }
+  }
+}
+
 /**
  * Validate JavaScript syntax without executing the file.
  * Catches SyntaxError (duplicate const, missing brackets, etc.)
@@ -50,10 +111,14 @@ function validateSyntax(filePath) {
 }
 
 function build() {
-  // Ensure dist directory exists
+  // Ensure dist and staging directories exist (staging is a sibling of dist
+  // used to make writes atomic — see STAGE_DIR comment above).
   if (!fs.existsSync(DIST_DIR)) {
     fs.mkdirSync(DIST_DIR, { recursive: true });
   }
+  if (!fs.existsSync(STAGE_DIR)) {
+    fs.mkdirSync(STAGE_DIR, { recursive: true });
+  }
 
   let hasErrors = false;
 
@@ -78,13 +143,44 @@ function build() {
     }
 
     console.log(`\x1b[32m✓\x1b[0m Copying ${hook}...`);
-    fs.copyFileSync(src, dest);
+    // Atomic write: copy to a per-process staging file in the sibling
-    // Preserve executable bit for shell scripts
+    // STAGE_DIR (same filesystem as DIST_DIR so rename(2) is atomic), then
+    // rename into place. Multiple test files invoke this script concurrently
+    // from their before() hooks; fs.copyFileSync truncates then writes the
+    // destination — readers (install.js subprocesses spawned by parallel
+    // install tests) can observe the dest empty or partial mid-write,
+    // producing flaky failures such as bug-2136 part 4 where installed .sh
+    // hooks lacked their "# gsd-hook-version:" header. POSIX rename(2)
+    // makes the swap atomic so readers see either the old file or the new
+    // file. The staging file lives outside DIST_DIR so readdirSync(DIST_DIR)
+    // (in install.js and tests) never observes a transient ".tmp" sibling.
+    const stagedDest = path.join(STAGE_DIR, `${hook}.${process.pid}.${Date.now()}`);
+    fs.copyFileSync(src, stagedDest);
+    // Preserve executable bit for shell scripts before rename so the
+    // installed file is executable from the very first observation.
     if (hook.endsWith('.sh')) {
-      try { fs.chmodSync(dest, 0o755); } catch (e) { /* Windows */ }
+      try { fs.chmodSync(stagedDest, 0o755); } catch (e) { /* Windows */ }
     }
+    renameAtomicWithRetry(stagedDest, dest, hook);
   }
 
+  // Best-effort cleanup of the staging dir. If concurrent builders are still
+  // running, their staged files will be left in STAGE_DIR and cleaned up by
+  // whichever builder calls fs.rmdirSync last. fs.rmdirSync throws ENOTEMPTY
+  // on a non-empty directory (it is NOT a silent no-op), so we first read
+  // the directory via fs.readdirSync(STAGE_DIR) -> leftovers and only call
+  // fs.rmdirSync(STAGE_DIR) when leftovers.length === 0. A TOCTOU window
+  // remains: another builder can drop a staged file between the readdirSync
+  // and the rmdirSync, in which case rmdirSync still throws ENOTEMPTY — the
+  // outer try/catch swallows that, plus ENOENT if the dir was already
+  // removed by a peer. Either way, build proceeds; cleanup is best-effort.
+  try {
+    const leftovers = fs.readdirSync(STAGE_DIR);
+    if (leftovers.length === 0) {
+      fs.rmdirSync(STAGE_DIR);
+    }
+  } catch (e) { /* tolerate TOCTOU ENOTEMPTY or ENOENT from peer cleanup */ }
+
   if (hasErrors) {
     console.error('\n\x1b[31mBuild failed: fix syntax errors above before publishing.\x1b[0m');
     process.exit(1);