fix(install): require persistent SDK reachability before reporting ready (#3231) (#3249)

* test: reproduce false GSD SDK ready signals on Linux (#3231)

* fix(install): require persistent SDK reachability before reporting ready (#3231)

* changeset: pr=3249 for #3231

* fix(install): filter _npx from login-shell PATH probe (CR finding 1)

Apply filterNpxFromPath() to the getUserShellPath() result before passing
it to isGsdSdkOnPath(), mirroring the same filtering already applied to
process.env.PATH. Without this, a transient _npx entry in the login-shell
PATH can falsely satisfy the cross-shell reachability check and reintroduce
the false-ready condition this PR fixes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(test): unconditional legacy-shim replacement assertion (CR finding 2)

Replace readFileSync+includes source-grep check with isLegacyGsdSdkShim()
and add an else branch asserting that when sdkReady is false, a warning/error
was emitted. Previously the sdkReady===false path had no assertion at all,
allowing the test to pass without verifying any postcondition.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test: replace text-grep assertions with structured ones (CR finding 2 + nitpick)

Finding 2: restructure the legacy-shim replacement assertion to branch on
isLegacyGsdSdkShim() state (a behavioral fact) rather than console output,
and add an unconditional postcondition for both branches.

Nitpick 3 (4 locations):
- lines 149-153: replace /GSD SDK ready/.test(combined) with
  isGsdSdkOnPath(filterNpxFromPath(PATH)) === false
- lines 167-169, 185-189: split filterNpxFromPath result into segments array
  and use array.includes() instead of string.includes() on the raw PATH string
- lines 375-377: replace /GSD SDK ready/.test(combined) with
  fs.existsSync(shimPath) + isGsdSdkOnPath(filterNpxFromPath(localBin))

All 8 tests pass. lint-no-source-grep: 0 violations.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(build-hooks): per-PID staging dir eliminates concurrent-cleanup TOCTOU race

When multiple test before() hooks spawned build-hooks.js concurrently
(--test-concurrency=4), a race existed: Process A would finish all copies,
call rmdirSync('.dist-staging/') in cleanup, then Process B — still in its
copy loop — would call copyFileSync(src, '.dist-staging/hook.pid.ts') and
get ENOENT because the staging directory was gone.

On macOS/Linux, copyFileSync reports the SOURCE path in ENOENT errors when
the destination directory is missing, making the failure appear to be a
missing source file (hooks/gsd-statusline.js) rather than a missing
destination directory. This misled the diagnosis.

Fix: make STAGE_DIR per-PID ('.dist-staging-<pid>/') so each builder owns
its own staging directory. No other process touches it, eliminating all
contention on staging-dir creation and cleanup. Update .gitignore to match
the new 'hooks/.dist-staging-*/' glob.

Reproduces as: CI test matrix (macos-24, ubuntu-22, ubuntu-24) all failing
with ENOENT on hooks/gsd-statusline.js in bug-2136 before() hook. The new
test file added in this PR (bug-3231) shifts the concurrency schedule just
enough to expose the race on every CI run.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test: assert on captured console output, not tautological PATH state (CR finding)

The two discarded `captureConsole()` return values in the bug-3231 test
were flagged by CodeRabbit as tautological assertions. Fix:

- Test 1 (transient _npx PATH): capture stdout/stderr and assert the
  installer does NOT emit "GSD SDK ready" (the false-positive the PR
  fixes), and that it does emit some diagnostic output instead.

- Test 3 (clean install): capture stdout/stderr and assert the installer
  DOES emit "GSD SDK ready" after successfully self-linking into a
  persistent PATH dir — confirming the positive path works correctly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

scripts/build-hooks.js

@@ -12,12 +12,15 @@ 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');
+// Per-process staging directory for atomic writes. Using process.pid in the
+// name eliminates all contention between concurrent builders: each process
+// owns its own staging dir and never races with another builder's cleanup.
+// 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.
+// The parent pattern hooks/.dist-staging-*/ is gitignored.
+const STAGE_DIR = path.join(HOOKS_DIR, `.dist-staging-${process.pid}`);
 
 // Hooks to copy (pure Node.js, no bundling needed)
 const HOOKS_TO_COPY = [
@@ -143,7 +146,7 @@ function build() {
     }
 
     console.log(`\x1b[32m✓\x1b[0m Copying ${hook}...`);
-    // Atomic write: copy to a per-process staging file in the sibling
+    // Atomic write: copy to a per-process staging file in the per-PID sibling
     // 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
@@ -154,7 +157,9 @@ function build() {
     // 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()}`);
+    // Each process uses its own STAGE_DIR (keyed by PID) so concurrent
+    // builders never race on staging-dir creation or cleanup.
+    const stagedDest = path.join(STAGE_DIR, `${hook}.${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.
@@ -164,22 +169,12 @@ function build() {
     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.
+  // Best-effort cleanup of this process's own staging dir. Since STAGE_DIR
+  // is per-PID (`.dist-staging-<pid>/`), no other builder touches it — so
+  // rmSync with recursive:true is safe and leaves no race window.
   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 */ }
+    fs.rmSync(STAGE_DIR, { recursive: true, force: true });
+  } catch (e) { /* tolerate ENOENT if the dir was never created (e.g. all hooks skipped) */ }
 
   if (hasErrors) {
     console.error('\n\x1b[31mBuild failed: fix syntax errors above before publishing.\x1b[0m');