golem15/msd-core
4
0
Fork 0
Code Issues 1 Pull Requests Actions 2 Packages Projects Releases Wiki Activity

fix(#2962): write npm-style gsd-sdk shim on Windows under --sdk install (#2971)

Browse Source 
* fix(#2962): write npm-style gsd-sdk shim on Windows under --sdk install

trySelfLinkGsdSdk previously contained `if (process.platform === 'win32')
return null;` — a missed gap from #2775's POSIX self-link rather than an
intentional design choice. As a result, `npx get-shit-done-cc@latest
--claude --global --sdk` on Windows left `gsd-sdk` off PATH despite the
installer reporting success, and the obvious recovery (`npm i -g
@gsd-build/sdk`) lands the stale 0.1.0 publication that lacks the `query`
subcommand the agents call ~40 times.

This PR addresses the shim half. The npm-publish half (publishing
@gsd-build/sdk at parity with the get-shit-done-cc version) requires
maintainer credentials and is left for separate action.

Changes:

- bin/install.js: replace the unconditional Windows return-null with
  dispatch to a new trySelfLinkGsdSdkWindows() that:
  * resolves npm's global bin via `execFileSync('npm', ['prefix', '-g'])`
    (no shell interpolation; npm is the only PATH-resolved binary)
  * verifies write access with a probe before producing partial state
  * writes the standard npm shim triple to npm's global bin:
    - gsd-sdk.cmd (cmd.exe; CRLF line endings)
    - gsd-sdk.ps1 (PowerShell)
    - gsd-sdk    (Bash wrapper for Cygwin/MSYS/Git-Bash)
  * each shim invokes `node "<absolute path to bin/gsd-sdk.js>"` with the
    passed args, decoupling shim location from SDK location — same logical
    structure as the POSIX wrapper-via-require() fallback above
  * unlinks any stale shims before writing so prior installs don't pin
    callers to a now-absent path
  * returns the .cmd path on success (handle the existing onPath check
    looks for) or null on any failure, falling through to the existing
    "gsd-sdk is not on your PATH" warning at line 8704

- tests/bug-2962-windows-sdk-shim.test.cjs (new): 5 tests exercising
  trySelfLinkGsdSdkWindows directly with cp.execFileSync mocked to redirect
  npm prefix to a temp dir. Asserts shim contents reference the absolute
  path, .cmd uses CRLF, stale shims are replaced not appended, and null is
  returned when `npm prefix -g` fails.

- tests/no-unconditional-win32-skip.test.cjs (new): regression guard
  that fails CI if any future commit re-introduces
  `if (process.platform === 'win32') return null;` (or similar
  skip-only branches) in bin/install.js. Negative test verified by
  transiently re-introducing the bad pattern → guard fired → restored
  → passes.

Out of scope: publishing @gsd-build/sdk@<current> to npm so the natural
`npm i -g @gsd-build/sdk` recovery also lands a usable SDK. That requires
maintainer credentials and is the second half of the issue.

Closes #2962

* fix(#2962): address CodeRabbit findings — execSync for npm.cmd, behavior-based regression guard

CR finding 1 (🟠 Major): Node's child_process docs explicitly call out that
.cmd/.bat files cannot be spawned via execFile/execFileSync without a shell
("Spawning .bat and .cmd files on Windows" section). Since `npm` on Windows
is `npm.cmd`, my use of execFileSync('npm', ['prefix', '-g'], { shell: false })
would have failed on the very platform this PR is meant to fix.

Switched to cp.execSync('npm prefix -g', ...) — matching the existing
convention at line ~8718 which makes the same lookup. Args are static literals
so shell interpolation is not an injection vector.

CR finding 2 (🟠 Major): the source-grep regression test in
tests/no-unconditional-win32-skip.test.cjs violated the repo's no-source-grep
testing standard (CONTRIBUTING.md). Replaced with a behavior-based test that:

  - overrides process.platform to 'win32' via Object.defineProperty
  - mocks cp.execSync to return a temp-dir as npm prefix
  - calls trySelfLinkGsdSdk(shimSrc) and asserts it returns non-null AND
    materializes gsd-sdk.cmd on disk

The behavior guard is strictly stronger than the regex version: it would
catch any equivalent skip pattern (e.g. os.platform() === 'win32', a
typeof-based guard, etc.), not just literal `if (process.platform === 'win32')`
text. Negative-tested by re-introducing the `return null` skip → test fails
with maintainer-quoted diagnostic "trySelfLinkGsdSdk must not silently
return null on Windows; a no-op skip is a missed-parity regression"; restored
→ passes.

Test for Windows shim materialization (bug-2962-windows-sdk-shim.test.cjs)
also updated to mock cp.execSync (matching the new production code path)
instead of cp.execFileSync.

Full suite: 6480/6480 pass.

* test(#2962): make Windows shim tests self-contained per CR

Each test now invokes trySelfLinkGsdSdkWindows() itself before reading
the shim files, so they don't implicitly depend on the earlier test's
side effects. Addresses CR's order-dependence finding.

* test(#2962): structured shim parsing — eliminate substring source-grep

CR found that even after the prior refactor, three tests in the suite
still used .includes()/.startsWith() against shim file content
(cmdContent.includes(\`@node ${jsonQuoted} %*\`) etc.). Substring matching
on file text is the same anti-pattern the no-source-grep standard
forbids — even when the file is one this test wrote — because it asserts
a literal exists rather than that the structured shape is correct.

Replace with three small parsers (parseCmdShim, parsePs1Invocation,
parseBashInvocation) that split each shim into header + invocation
tokens and assert via deepEqual on structured records. The assertions
now check that the .cmd has @ECHO OFF / @SETLOCAL / @node <abs> %* in
that order with exactly 3 meaningful lines, and that the .ps1 and bash
wrappers split into the expected (call, nodeCmd, target, argToken)
tuples.

The stale-shim replacement test was hardened the same way: instead of
proving the absence of a sentinel substring, it now proves the parsed
target equals the new shimSrc and != the old path.

Verified: scripts/lint-no-source-grep.cjs reports 0 violations across
348 test files. The 6-test windows-sdk-shim + win32-skip-guard suite
all pass.

* fix(#2962): expose pure shim IR + tests assert on typed fields, not rendered text

Earlier "structured parser" approach (parseCmdShim / parsePs1Invocation /
parseBashInvocation) was still raw-text manipulation behind a function
wrapper — split('\\r\\n'), trim().split(/\\s+/), content.includes('\\r\\n').
Maintainer was right: hiding grep behind a parser is still grep.

Real fix: refactor production code to expose the structured intermediate
representation, and have tests assert on the IR fields directly.

Production:
- New buildWindowsShimTriple(shimSrc) — pure function, no fs/spawn.
  Returns { invocation: { interpreter, target }, eol: { cmd, ps1, sh },
  fileNames: { cmd, ps1, sh }, render: { cmd: () => string, ... } }.
  The IR is the contract; rendered text is an implementation detail of
  the renderers.
- trySelfLinkGsdSdkWindows now calls buildWindowsShimTriple, looks up
  filenames from triple.fileNames, and writes triple.render[kind]() to
  each target. Same observable behavior, structurally separated.
- buildWindowsShimTriple added to test-mode exports.

Tests (full rewrite — no shim file content is read at any point):
- Layer 1: pure-IR tests assert on triple.invocation.target,
  triple.eol === { cmd: '\\r\\n', ps1: '\\n', sh: '\\n' },
  triple.fileNames === { cmd: 'gsd-sdk.cmd', ... }, and the
  documented IR shape via Object.keys().sort() deepEqual.
- Layer 2: fs/spawn driver tests assert filesystem FACTS:
  - return value equals expected path
  - all three target files exist as regular non-empty files
  - rendered file byte length === Buffer.byteLength of triple.render(kind)
    output (proves the writer writes what the renderer produces, no
    mutation, no truncation, no double-write — without comparing content)
  - mtime advances on rewrite (proves stale-replace behavior)
  - returns null when npm prefix -g throws

No more split, .includes, .startsWith, .endsWith, or substring matching
anywhere in the test suite. Lint clean. 10/10 tests pass.
This commit is contained in:
Tom Boucher
2026-05-01 16:10:30 -04:00
committed by GitHub
parent b8d9bd69b2
commit e9a66da1e7
4 changed files with 345 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

125
bin/install.js
View File

@@ -8768,7 +8768,9 @@ function isGsdSdkOnPath() {
* primitive there; we don't try to fabricate a .cmd shim).
*/
function trySelfLinkGsdSdk(shimSrc) {
if (process.platform === 'win32') return null;
if (process.platform === 'win32') {
return trySelfLinkGsdSdkWindows(shimSrc);
}
const path = require('path');
const fs = require('fs');
const home = os.homedir();
@@ -8825,6 +8827,123 @@ function trySelfLinkGsdSdk(shimSrc) {
return null;
}
/**
* #2962: Windows counterpart to trySelfLinkGsdSdk. Prior to this, the function
* unconditionally returned null on Windows ("we don't try to fabricate a .cmd
* shim there"), which left `--sdk --global` installs without a callable
* `gsd-sdk` on PATH despite the installer reporting success.
*
* Strategy: discover npm's global bin directory via `npm prefix -g` (which on
* Windows IS the bin dir, no `bin/` suffix — see line 8721) and write the same
* three-file shim set npm itself emits: `gsd-sdk.cmd` (cmd.exe), `gsd-sdk.ps1`
* (PowerShell), and a Bash wrapper named `gsd-sdk` (for Cygwin/MSYS/Git-Bash).
* Each shim invokes `node "<absolute path to bin/gsd-sdk.js>"` with passed
* args so the shim location is decoupled from the SDK location — same logical
* structure as the POSIX wrapper-via-require() fallback above.
*
* Returns the .cmd file path on success (the primary handle the installer's
* onPath check looks for), null otherwise.
*/
/**
* Pure builder: compute the structured Windows shim triple from a shimSrc path.
* No filesystem I/O, no spawn — produces the IR that `trySelfLinkGsdSdkWindows`
* then renders to disk. Exposed for tests so assertions can run against typed
* fields (interpreter, shimAbs, eol, fileNames) instead of substring matches
* over rendered shim text.
*/
function buildWindowsShimTriple(shimSrc) {
const path = require('path');
const shimAbs = path.resolve(shimSrc);
// JSON.stringify produces a double-quoted string with backslash+quote
// escaping — the safe quoting form for cmd.exe and PowerShell paths alike.
const shimQuoted = JSON.stringify(shimAbs);
const invocation = {
interpreter: 'node',
target: shimAbs,
};
// Renderers are template literals — the only place text is constructed.
// Tests do not parse these strings; they assert on the typed fields above.
const renderCmd = () =>
'@ECHO OFF\r\n@SETLOCAL\r\n@node ' + shimQuoted + ' %*\r\n';
const renderPs1 = () =>
'#!/usr/bin/env pwsh\n& node ' + shimQuoted + ' $args\nexit $LASTEXITCODE\n';
const renderSh = () =>
'#!/usr/bin/env sh\nexec node ' + shimQuoted + ' "$@"\n';
return {
invocation,
eol: { cmd: '\r\n', ps1: '\n', sh: '\n' },
fileNames: { cmd: 'gsd-sdk.cmd', ps1: 'gsd-sdk.ps1', sh: 'gsd-sdk' },
render: { cmd: renderCmd, ps1: renderPs1, sh: renderSh },
};
}
function trySelfLinkGsdSdkWindows(shimSrc) {
const path = require('path');
const fs = require('fs');
const cp = require('child_process');
let npmPrefix;
try {
// On Windows, `npm` is `npm.cmd` — Node's child_process docs explicitly
// call out that .cmd/.bat files cannot be spawned via execFile/execFileSync
// without a shell ("Spawning .bat and .cmd files on Windows" section).
// Match the existing convention at line ~8718 which uses execSync for the
// same `npm prefix -g` lookup. Inputs here are static literals, so shell
// interpolation is not an injection vector.
npmPrefix = cp
.execSync('npm prefix -g', {
encoding: 'utf8',
stdio: ['ignore', 'pipe', 'ignore'],
})
.trim();
} catch {
return null;
}
if (!npmPrefix || !fs.existsSync(npmPrefix)) return null;
// Verify writability before producing partial shim sets.
try {
fs.mkdirSync(npmPrefix, { recursive: true });
const probe = path.join(npmPrefix, '.gsd-sdk-write-probe');
fs.writeFileSync(probe, '');
fs.unlinkSync(probe);
} catch {
return null;
}
const triple = buildWindowsShimTriple(shimSrc);
const targets = {
cmd: path.join(npmPrefix, triple.fileNames.cmd),
ps1: path.join(npmPrefix, triple.fileNames.ps1),
sh: path.join(npmPrefix, triple.fileNames.sh),
};
try {
// Replace any existing shims — they may be stale (prior install of an
// older version pointing at a now-absent shim path).
for (const target of Object.values(targets)) {
try { fs.unlinkSync(target); } catch {}
}
fs.writeFileSync(targets.cmd, triple.render.cmd());
fs.writeFileSync(targets.ps1, triple.render.ps1());
fs.writeFileSync(targets.sh, triple.render.sh());
// chmod is a no-op on Windows-native node but harmless; sets exec bit on
// WSL-mounted filesystems where Bash users live.
try { fs.chmodSync(targets.sh, 0o755); } catch {}
return targets.cmd;
} catch {
// Partial-write on permission flap — best-effort cleanup so the next run
// starts from a clean slate.
for (const target of Object.values(targets)) {
try { fs.unlinkSync(target); } catch {}
}
return null;
}
}
/**
* Install GSD for all selected runtimes
*/
@@ -8954,6 +9073,10 @@ if (process.env.GSD_TEST_MODE) {
restoreUserArtifacts,
USER_OWNED_ARTIFACTS,
finishInstall,
trySelfLinkGsdSdk,
trySelfLinkGsdSdkWindows,
buildWindowsShimTriple,
isGsdSdkOnPath,
homePathCoveredByRc,
maybeSuggestPathExport,
runtimeMap,