From 33ebd1de195a128d51256b6d7939bfb910f517b8 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 26 May 2026 22:46:09 -0400 Subject: [PATCH] fix(#371): invoke installed bin via shell for Windows .cmd shims in release-tarball-smoke (#375) * fix(#371): invoke installed bin via shell for Windows .cmd shims in release-tarball-smoke `node ` cannot execute a Windows batch shim as a JS script, so runSmoke returned bin_not_callable for every check on Windows. Route .cmd/.bat shims through shell:true (required by Node >=18.20/20.12) and keep the POSIX node-invocation path unchanged. Add an exported binInvocation seam plus a platform-agnostic regression test. Co-Authored-By: Claude Opus 4.7 (1M context) * chore(#371): surface bin-invocation failure details in release-tarball-smoke The smoke harness captured stderr/stdout in `details` but never printed them, so Windows bin_not_callable failures gave no actionable cause in CI. Log the resolved bin, invocation descriptor, exit status/signal/error, and captured stderr/stdout on spawn-derived failures so the real error is visible. Co-Authored-By: Claude Opus 4.7 (1M context) * chore(#371): include smoke result details in install-test assertion messages The node --test TAP runner swallows in-test console.error, so Windows bin_not_callable failures gave no cause. Embed code + details (incl. captured stderr/stdout) into the assertion messages, which DO reach the CI log, so the real Windows failure is diagnosable. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(#371): resolve installed bin from prefix root on Windows npm install -g --prefix X writes bin shims to X\ (the prefix root) on Windows, not X\node_modules\.bin\. The smoke harness only searched node_modules\.bin on win32, so the installed gsd-tools/installer bin was never found and runSmoke returned bin_not_callable before invoking anything. Search the prefix root first (then node_modules/.bin as fallback), report the searched candidates on miss, and drop the TAP-swallowed console.error probes. The .cmd-via-shell binInvocation fix remains for actually running the shim. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- scripts/release-tarball-smoke.cjs | 104 +++++++++++++----- ...ease-tarball-smoke-bin-invocation.test.cjs | 57 ++++++++++ tests/release-tarball-smoke.install.test.cjs | 21 ++-- 3 files changed, 145 insertions(+), 37 deletions(-) create mode 100644 tests/release-tarball-smoke-bin-invocation.test.cjs diff --git a/scripts/release-tarball-smoke.cjs b/scripts/release-tarball-smoke.cjs index fe0f0ccbb..6f8641bd3 100644 --- a/scripts/release-tarball-smoke.cjs +++ b/scripts/release-tarball-smoke.cjs @@ -65,6 +65,44 @@ const SMOKE = Object.freeze({ WORKFLOW_BODY_COLON_LEAK: 'workflow_body_colon_leak', }); +// --------------------------------------------------------------------------- +// Exported helper: binInvocation +// --------------------------------------------------------------------------- + +/** + * Build the { command, args, shell } descriptor needed to spawn an installed + * npm bin correctly on both Windows and POSIX. + * + * On Windows, npm installs a `.cmd` (or `.bat`) shim in .bin/. Node ≥18.20.2 + * / ≥20.12.2 throws EINVAL when you try to spawnSync a .cmd/.bat without + * shell:true (CVE-2024-27980 mitigation). With shell:true, Node does NOT + * auto-quote argv, so a bin path that contains spaces must be wrapped in + * double-quotes to arrive at the shell as one token. + * + * On POSIX the bin is a regular shebang JS file; we invoke it directly via + * process.execPath (the same Node binary) without a shell. + * + * @param {string} binPath - Absolute path to the resolved bin file. + * @param {string[]} [args] - Additional arguments (e.g. ['--help']). + * @returns {{ command: string, args: string[], shell: boolean }} + */ +function binInvocation(binPath, args = []) { + const lower = binPath.toLowerCase(); + // Note: .ps1 shims are intentionally NOT handled here. The bin-resolution + // helpers (findGsdToolsBin / findInstallerBin) only ever surface a .cmd path + // on Windows — npm does not write .ps1 shims into .bin/ by default — so a + // .ps1 path never reaches this function in practice. + if (lower.endsWith('.cmd') || lower.endsWith('.bat')) { + // Quote the path if it contains a space so the Windows shell treats it as + // a single token. Simple double-quote wrap is sufficient because npm- + // generated shim paths don't contain embedded double-quotes. + const command = binPath.includes(' ') ? `"${binPath}"` : binPath; + return { command, args: [...args], shell: true }; + } + // POSIX: invoke via node, no shell needed. + return { command: process.execPath, args: [binPath, ...args], shell: false }; +} + // --------------------------------------------------------------------------- // Internal helpers // --------------------------------------------------------------------------- @@ -81,19 +119,36 @@ function pkgRoot(installPrefix) { return fs.existsSync(posix) ? posix : win; } +/** + * Return the ordered list of candidate paths to check when locating an npm + * global bin named `name` under `installPrefix`. + * + * On Windows, `npm install -g --prefix X` writes shims (*.cmd, *.ps1, bare) + * to the PREFIX ROOT (X\), NOT to X\node_modules\.bin\. We therefore probe + * the prefix root first, then fall back to node_modules\.bin in case a + * non-standard layout puts them there. + * + * On POSIX the shim lands in /bin/ as a symlink; only one candidate. + */ +function binCandidates(installPrefix, name) { + if (process.platform === 'win32') { + return [ + // npm global --prefix on Windows writes shims to the prefix ROOT + path.join(installPrefix, `${name}.cmd`), + path.join(installPrefix, name), + // fallback: some layouts use node_modules/.bin + path.join(installPrefix, 'node_modules', '.bin', `${name}.cmd`), + path.join(installPrefix, 'node_modules', '.bin', name), + ]; + } + return [path.join(installPrefix, 'bin', name)]; +} + /** * Locate the installed gsd-tools binary (symlink in /bin/). */ function findGsdToolsBin(installPrefix) { - const binDir = process.platform === 'win32' - ? path.join(installPrefix, 'node_modules', '.bin') - : path.join(installPrefix, 'bin'); - - const candidates = process.platform === 'win32' - ? [path.join(binDir, 'gsd-tools.cmd'), path.join(binDir, 'gsd-tools')] - : [path.join(binDir, 'gsd-tools')]; - - for (const c of candidates) { + for (const c of binCandidates(installPrefix, 'gsd-tools')) { if (fs.existsSync(c)) return c; } return null; @@ -103,15 +158,7 @@ function findGsdToolsBin(installPrefix) { * Locate the get-shit-done-redux installer binary (the symlink in /bin/). */ function findInstallerBin(installPrefix) { - const binDir = process.platform === 'win32' - ? path.join(installPrefix, 'node_modules', '.bin') - : path.join(installPrefix, 'bin'); - - const candidates = process.platform === 'win32' - ? [path.join(binDir, 'get-shit-done-redux.cmd'), path.join(binDir, 'get-shit-done-redux')] - : [path.join(binDir, 'get-shit-done-redux')]; - - for (const c of candidates) { + for (const c of binCandidates(installPrefix, 'get-shit-done-redux')) { if (fs.existsSync(c)) return c; } return null; @@ -266,22 +313,21 @@ function runSmoke({ const actualBin = findGsdToolsBin(installPrefix); if (!actualBin) { - const binDir = process.platform === 'win32' - ? path.join(installPrefix, 'node_modules', '.bin') - : path.join(installPrefix, 'bin'); + const searched = binCandidates(installPrefix, 'gsd-tools'); return { code: SMOKE.BIN_NOT_CALLABLE, - details: { ...details, binDir, searched: [] }, + details: { ...details, searched }, }; } // --- Invoke `gsd-tools --help` to assert the shipped binary is callable --- // Use effectiveNpmEnv so the installed binary sees an isolated HOME on Docker // hosts where HOME may be unwritable (same isolation as the npm install). (#131) + const versionInvocation = binInvocation(actualBin, ['--help']); const versionResult = spawnSync( - process.execPath, - [actualBin, '--help'], - { encoding: 'utf-8', timeout: CHILD_TIMEOUT_MS, env: effectiveNpmEnv }, + versionInvocation.command, + versionInvocation.args, + { encoding: 'utf-8', timeout: CHILD_TIMEOUT_MS, env: effectiveNpmEnv, shell: versionInvocation.shell }, ); if (versionResult.status !== 0) { @@ -341,9 +387,10 @@ function runSmoke({ const initEnv = { ...process.env }; delete initEnv.GSD_TEST_MODE; + const initInvocation = binInvocation(installerBin, ['--local', '--claude']); const initResult = spawnSync( - process.execPath, - [installerBin, '--local', '--claude'], + initInvocation.command, + initInvocation.args, { encoding: 'utf-8', cwd: fixtureDir, @@ -351,6 +398,7 @@ function runSmoke({ stdio: ['pipe', 'pipe', 'pipe'], env: initEnv, timeout: CHILD_TIMEOUT_MS, + shell: initInvocation.shell, }, ); @@ -556,7 +604,7 @@ function cleanup(...dirs) { // Exports // --------------------------------------------------------------------------- -module.exports = { SMOKE, runSmoke }; +module.exports = { SMOKE, runSmoke, binInvocation }; if (require.main === module) { cliMain(); diff --git a/tests/release-tarball-smoke-bin-invocation.test.cjs b/tests/release-tarball-smoke-bin-invocation.test.cjs new file mode 100644 index 000000000..b09d40c70 --- /dev/null +++ b/tests/release-tarball-smoke-bin-invocation.test.cjs @@ -0,0 +1,57 @@ +'use strict'; + +/** + * Unit tests for the binInvocation helper in scripts/release-tarball-smoke.cjs. + * + * Asserts ONLY on the returned descriptor — no process spawning. + */ + +const { describe, it } = require('node:test'); +const assert = require('node:assert/strict'); + +const { binInvocation } = require('../scripts/release-tarball-smoke.cjs'); + +describe('binInvocation', () => { + it('returns shell:true and command=binPath (not execPath) for a .cmd path', () => { + const bin = 'C:\\prefix\\node_modules\\.bin\\gsd-tools.cmd'; + const result = binInvocation(bin, ['--help']); + + assert.strictEqual(result.command, bin, + 'command must be the .cmd path itself, not process.execPath'); + assert.strictEqual(result.shell, true, + 'shell must be true for .cmd shims (Node CVE-2024-27980 mitigation)'); + // The bin path must NOT appear in args as if it were a node script argument + assert.ok( + !result.args.includes(bin), + 'the .cmd path must not be pushed into args as a node-script positional', + ); + }); + + it('returns shell:true and command=binPath for a .bat path', () => { + const bin = 'C:\\prefix\\node_modules\\.bin\\gsd-tools.bat'; + const result = binInvocation(bin, ['--help']); + + assert.strictEqual(result.shell, true, 'shell must be true for .bat shims'); + assert.strictEqual(result.command, bin, 'command must be the .bat path'); + }); + + it('returns command===process.execPath and args[0]===binPath and shell falsy for a POSIX path', () => { + const bin = '/tmp/prefix/bin/gsd-tools'; + const result = binInvocation(bin, ['--help']); + + assert.strictEqual(result.command, process.execPath, + 'POSIX bin must be invoked via node (process.execPath)'); + assert.strictEqual(result.args[0], bin, + 'POSIX bin path must be args[0] (the node script argument)'); + assert.ok(!result.shell, + 'shell must be falsy for POSIX shebang bins'); + }); + + it('quotes a .cmd path containing a space so the shell receives one token', () => { + const bin = 'C:\\Users\\a b\\node_modules\\.bin\\gsd-tools.cmd'; + const result = binInvocation(bin, ['--help']); + + assert.strictEqual(result.shell, true, 'shell must be true'); + assert.strictEqual(result.command, `"${bin}"`, 'spaced .cmd path must be wrapped in double-quotes as a single shell token'); + }); +}); diff --git a/tests/release-tarball-smoke.install.test.cjs b/tests/release-tarball-smoke.install.test.cjs index 8ef0f6cc2..619de67b1 100644 --- a/tests/release-tarball-smoke.install.test.cjs +++ b/tests/release-tarball-smoke.install.test.cjs @@ -15,6 +15,9 @@ const path = require('node:path'); const { cleanup, createTempDir, runNpm, isolatedNpmEnv } = require('./helpers.cjs'); const { SMOKE, runSmoke } = require('../scripts/release-tarball-smoke.cjs'); +const smokeMsg = (label, result) => + `${label}: code=${result.code} details=${JSON.stringify(result.details)}`; + const PKG_PATH = path.join(__dirname, '..', 'package.json'); const pkg = JSON.parse(fs.readFileSync(PKG_PATH, 'utf-8')); @@ -74,8 +77,8 @@ describe('release-tarball-smoke', () => { npmEnv: isolatedNpmEnv(), }); - assert.equal(result.code, SMOKE.OK); - assert.equal(result.details.version, pkg.version); + assert.equal(result.code, SMOKE.OK, smokeMsg('A', result)); + assert.equal(result.details.version, pkg.version, smokeMsg('A', result)); }); // ── Test B — version mismatch detected ──────────────────────────────────── @@ -88,7 +91,7 @@ describe('release-tarball-smoke', () => { npmEnv: isolatedNpmEnv(), }); - assert.equal(result.code, SMOKE.VERSION_MISMATCH); + assert.equal(result.code, SMOKE.VERSION_MISMATCH, smokeMsg('B', result)); }); // ── Test C — happy lifecycle ─────────────────────────────────────────────── @@ -106,7 +109,7 @@ describe('release-tarball-smoke', () => { npmEnv: isolatedNpmEnv(), }); - assert.equal(result.code, SMOKE.OK); + assert.equal(result.code, SMOKE.OK, smokeMsg('C', result)); // Each non-init command must be in lifecycleResolved with both paths populated const resolved = result.details.lifecycleResolved; @@ -145,9 +148,9 @@ describe('release-tarball-smoke', () => { npmEnv: isolatedNpmEnv(), }); - assert.equal(result.code, SMOKE.COMMAND_FILE_MISSING); - assert.equal(result.details.command, 'nonexistent-phase-xyz'); - assert.ok(typeof result.details.path === 'string' && result.details.path.length > 0); + assert.equal(result.code, SMOKE.COMMAND_FILE_MISSING, smokeMsg('D', result)); + assert.equal(result.details.command, 'nonexistent-phase-xyz', smokeMsg('D', result)); + assert.ok(typeof result.details.path === 'string' && result.details.path.length > 0, smokeMsg('D', result)); }); // ── Test E — workflow-body checks run (informational) ───────────────────── @@ -167,11 +170,11 @@ describe('release-tarball-smoke', () => { // Structural: the scan ran and populated the counters assert.ok( Number.isInteger(result.details.workflowsScanned) && result.details.workflowsScanned >= 1, - `expected workflowsScanned >= 1, got ${result.details.workflowsScanned}`, + smokeMsg('E', result), ); assert.ok( Number.isInteger(result.details.colonLeakCount), - `expected colonLeakCount to be an integer, got ${result.details.colonLeakCount}`, + smokeMsg('E', result), ); }); });