From 6664190888066c1aa6a80ae6bce32f58a69a078f Mon Sep 17 00:00:00 2001 From: Fabio Date: Mon, 4 May 2026 22:53:29 +0200 Subject: [PATCH 1/3] fix(hooks): execFileSync 'npm' needs shell:true on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without shell:true, execFileSync('npm', ...) on Windows fails with ENOENT because npm is distributed as npm.cmd, not as a literal 'npm' binary. The silent try/catch swallows the error, latest stays null, update_available becomes null, and the statusline never shows "⬆ /gsd-update" — Windows users miss every release. Adding shell:true makes execFileSync route through cmd.exe which resolves npm.cmd via PATHEXT, identical behavior on POSIX. Repro on Windows: $env:GSD_CACHE_FILE = "$env:USERPROFILE\.cache\gsd\gsd-update-check.json" node ~\.claude\hooks\gsd-check-update-worker.js Get-Content "$env:USERPROFILE\.cache\gsd\gsd-update-check.json" Before: {"update_available":null,"installed":"1.40.0","latest":"unknown",...} After: {"update_available":false,"installed":"1.40.0","latest":"1.40.0",...} --- hooks/gsd-check-update-worker.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hooks/gsd-check-update-worker.js b/hooks/gsd-check-update-worker.js index 8c2250870..e6b7f6d58 100644 --- a/hooks/gsd-check-update-worker.js +++ b/hooks/gsd-check-update-worker.js @@ -93,6 +93,11 @@ try { encoding: 'utf8', timeout: 10000, windowsHide: true, + // shell:true is required on Windows so 'npm' resolves to npm.cmd via PATHEXT. + // Without it, execFileSync looks for a literal 'npm' binary, fails with ENOENT, + // the catch swallows the error, latest stays null, and the statusline never shows + // the "⬆ /gsd-update" indicator on Windows. + shell: true, }).trim(); } catch (e) {} From ad0747ccac50c9537c2cc6418f33b2109db9e609 Mon Sep 17 00:00:00 2001 From: Fabio Date: Tue, 5 May 2026 21:16:36 +0200 Subject: [PATCH 2/3] fix(hooks): scope shell:true to Windows + add changeset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address adversarial review feedback on PR #3102: 1. shell:true is now conditional (process.platform === 'win32') - POSIX path unchanged: no shell spawn, no overhead, original signal/exit-code semantics and windowsHide effect preserved - Windows path: still routes through cmd.exe to resolve npm.cmd via PATHEXT (the actual fix for ENOENT) 2. Added .changeset/windows-npm-shell-fix.md (Fixed type) Reviewed feedback resolved: - Cross-platform regression risk → shell now Windows-only - Missing changeset → added --- .changeset/windows-npm-shell-fix.md | 5 +++++ hooks/gsd-check-update-worker.js | 12 +++++++----- 2 files changed, 12 insertions(+), 5 deletions(-) create mode 100644 .changeset/windows-npm-shell-fix.md diff --git a/.changeset/windows-npm-shell-fix.md b/.changeset/windows-npm-shell-fix.md new file mode 100644 index 000000000..f33c5d5e6 --- /dev/null +++ b/.changeset/windows-npm-shell-fix.md @@ -0,0 +1,5 @@ +--- +type: Fixed +--- + +**Windows update-check no longer silently fails** — `gsd-check-update-worker` now passes `shell: true` only on Windows, allowing `execFileSync('npm', ...)` to resolve `npm.cmd` via PATHEXT. POSIX path (Linux/macOS) is unchanged. Without this fix, the worker failed with ENOENT, `latest` stayed `null`, `update_available` became `null`, and the statusline `⬆ /gsd-update` indicator never rendered for Windows users. Fixes #3103. diff --git a/hooks/gsd-check-update-worker.js b/hooks/gsd-check-update-worker.js index e6b7f6d58..ff5a16f7e 100644 --- a/hooks/gsd-check-update-worker.js +++ b/hooks/gsd-check-update-worker.js @@ -93,11 +93,13 @@ try { encoding: 'utf8', timeout: 10000, windowsHide: true, - // shell:true is required on Windows so 'npm' resolves to npm.cmd via PATHEXT. - // Without it, execFileSync looks for a literal 'npm' binary, fails with ENOENT, - // the catch swallows the error, latest stays null, and the statusline never shows - // the "⬆ /gsd-update" indicator on Windows. - shell: true, + // On Windows, 'npm' is distributed as npm.cmd. Node's execFileSync does + // not apply PATHEXT resolution and looks for a literal 'npm' binary, + // failing with ENOENT. Setting shell:true on Windows routes through + // cmd.exe which resolves npm.cmd via PATHEXT. + // POSIX (Linux/macOS) is left untouched — no shell spawn, no extra + // signal/exit-code semantics, no overhead. + shell: process.platform === 'win32', }).trim(); } catch (e) {} From 3c1b91609fc2ba188fbbc903425da0b36ed33ec5 Mon Sep 17 00:00:00 2001 From: Fabio Date: Wed, 6 May 2026 18:34:18 +0200 Subject: [PATCH 3/3] test(hooks): structural test for Windows npm spawn platform gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Locks the contract from PR #3102 / issue #3103: the shell option on the execFileSync('npm', ...) call must be `process.platform === 'win32'`, never an unconditional `shell: true`. A regression to `shell: true` would silently change POSIX behavior (spawn /bin/sh -c, signal/exit-code semantics shift, windowsHide can lose effect on some Node versions) — exactly the cross-platform risk flagged in the adversarial review. Test approach: - Reads worker source via readFileSync (hooks/*.js, outside the lint-no-source-grep .cjs scope; allow-test-rule annotated with reason). - Strips comments before checking for `shell: true` so prose mentions in the JSDoc-style block comment do not trigger the regression check. - Asserts execFileSync is still the spawn primitive (a swap to execSync would silently shell-spawn on POSIX and defeat the gate). - Why structural, not runtime: the win32 branch only manifests on a Windows runner and the repo's CI is POSIX-only. All 4 subtests pass. Lint-no-source-grep: clean. --- ...check-update-worker-platform-gate.test.cjs | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 tests/gsd-check-update-worker-platform-gate.test.cjs diff --git a/tests/gsd-check-update-worker-platform-gate.test.cjs b/tests/gsd-check-update-worker-platform-gate.test.cjs new file mode 100644 index 000000000..e20be2490 --- /dev/null +++ b/tests/gsd-check-update-worker-platform-gate.test.cjs @@ -0,0 +1,99 @@ +/** + * Tests for gsd-check-update-worker.js — Windows npm resolution platform gate. + * + * Background (issue #3103, PR #3102): + * On Windows, `npm` ships as `npm.cmd`. Node's execFileSync does not apply + * PATHEXT resolution (unlike execSync/exec) and fails with ENOENT. The fix + * is to spawn through a shell on Windows (cmd.exe resolves npm.cmd via + * PATHEXT). On POSIX, `npm` is a node-script symlink and resolves without + * a shell, so spawning `/bin/sh -c` is pure overhead and changes signal / + * exit-code semantics — undesirable. + * + * This test locks the contract: shell must be platform-gated to win32 only, + * never an unconditional `shell: true`. A regression that re-introduces + * `shell: true` would change POSIX runtime behavior silently — exactly the + * cross-platform risk that adversarial review on PR #3102 flagged. + * + * Source-grep policy: this test reads the worker source via readFileSync. + * The repo's lint-no-source-grep rule (scripts/lint-no-source-grep.cjs) + * targets `.cjs` files in bin/lib/get-shit-done — `hooks/*.js` is out of + * scope. The behavior we need to lock is a single static-spawn-options + * shape, which only manifests at runtime under Windows; runtime testing + * would require a Windows CI lane. A structural assertion is the + * minimum-cost contract. + */ + +// allow-test-rule: structural assertion on hook spawn-options shape; the +// behavior being tested (Windows-only shell resolution) is platform-gated +// at runtime and cannot be reached on POSIX CI without a Windows lane. + +'use strict'; + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const WORKER_PATH = path.join(__dirname, '..', 'hooks', 'gsd-check-update-worker.js'); + +describe('gsd-check-update-worker: Windows npm spawn platform gate', () => { + test('worker file exists', () => { + assert.ok(fs.existsSync(WORKER_PATH), `worker not found at ${WORKER_PATH}`); + }); + + test('shell option is gated to process.platform === "win32"', () => { + const src = fs.readFileSync(WORKER_PATH, 'utf8'); + + // Locks the platform gate. Allows whitespace/quote variation around + // the comparison so trivial style fixes do not break the contract. + const platformGate = + /shell:\s*process\.platform\s*===\s*['"]win32['"]/; + + assert.match( + src, + platformGate, + [ + 'shell option must be `process.platform === "win32"`.', + 'A regression to `shell: true` would spawn /bin/sh -c on POSIX', + '(adds shell overhead, changes signal/exit semantics, can mask', + 'windowsHide on some Node versions). See PR #3102.', + ].join(' '), + ); + }); + + test('no unconditional shell: true on the npm spawn', () => { + const src = fs.readFileSync(WORKER_PATH, 'utf8'); + + // Strip line and block comments so prose mentions of "shell:true" in + // documentation comments do not trigger the regression check. + const codeOnly = src + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/(^|[^:])\/\/[^\n]*/g, '$1'); + + // Reject literal `shell: true` in CODE only. The correct fix uses + // `shell: process.platform === 'win32'` (an expression, not the + // literal `true`), so this never matches the platform-gated form. + // Trailing `[,\s}]` ensures we match an object-property assignment, + // not an unrelated identifier. + const naiveShell = /shell\s*:\s*true\s*[,\s}]/; + + assert.doesNotMatch( + codeOnly, + naiveShell, + 'shell: true is forbidden — use `process.platform === "win32"` gate.', + ); + }); + + test('execFileSync is still the spawn primitive (not exec/execSync)', () => { + const src = fs.readFileSync(WORKER_PATH, 'utf8'); + + // execFileSync is intentional: it does not invoke a shell on POSIX, + // unlike exec/execSync. A regression that swaps to execSync would + // silently always spawn a shell, defeating the platform gate. + assert.match( + src, + /execFileSync\s*\(\s*['"]npm['"]/, + 'npm spawn must use execFileSync (not exec/execSync) to keep POSIX shell-free.', + ); + }); +});