* fix(sdk): decouple SDK from build-from-source install path, close #2441 and #2453 Ship sdk/dist prebuilt in the tarball and replace the npm-install-g sub-install with a parent-package bin shim (bin/gsd-sdk.js). npm chmods bin entries from a packed tarball correctly, eliminating the mode-644 failure (#2453) and the full class of NPM_CONFIG_PREFIX/ignore-scripts/ corepack/air-gapped failure modes that caused #2439 and #2441. Changes: - sdk/package.json: prepublishOnly runs `rm -rf dist && tsc && chmod +x dist/cli.js` (stale-build guard + execute-bit fix at publish time) - package.json: add "gsd-sdk": "bin/gsd-sdk.js" bin entry; add sdk/dist to files so the prebuilt CLI ships in the tarball - bin/gsd-sdk.js: new back-compat shim — resolves sdk/dist/cli.js relative to the package root and delegates via `node`, so all existing PATH call sites (slash commands, agents, hooks) continue to work unchanged (S1 shim) - bin/install.js: replace installSdkIfNeeded() build-from-source + global- install dance with a dist-verify + chmod-in-place guard; delete resolveGsdSdk(), detectShellRc(), emitSdkFatal() helpers now unused - .github/workflows/install-smoke.yml: add smoke-unpacked job that strips execute bit from sdk/dist/cli.js before install to reproduce the exact #2453 failure mode - tests/bug-2441-sdk-decouple.test.cjs: new regression tests asserting all invariants (no npm install -g from sdk/, shim exists, sdk/dist in files, prepublishOnly has rm -rf + chmod) - tests/bugs-1656-1657.test.cjs: update stale assertions that required build-from-source behavior (now asserts new prebuilt-dist invariants) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(release): bump to 1.38.2, wire release.yml to build SDK dist - Bump version 1.38.1 -> 1.38.2 for the #2441/#2453 fix shipped in 0f6903d. - Add `build:sdk` script (`cd sdk && npm ci && npm run build`). - `prepublishOnly` now runs hooks + SDK builds as a safety net. - release.yml (rc + finalize): build SDK dist before `npm publish` so the published tarball always ships fresh `sdk/dist/` (kept gitignored). - CHANGELOG: document 1.38.2 entry and `--sdk` flag semantics change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: build SDK dist before tests and smoke jobs sdk/dist/ is gitignored (built fresh at publish time via release.yml), but both the test suite and install-smoke jobs run `bin/install.js` or `npm pack` against the checked-out tree where dist doesn't exist yet. - test.yml: `npm run build:sdk` before `npm run test:coverage`, so tests that spawn `bin/install.js` don't hit `installSdkIfNeeded()`'s fatal missing-dist check. - install-smoke.yml (both smoke and smoke-unpacked): build SDK before pack/chmod so the published tarball contains dist and the unpacked install has a file to strip exec-bit from. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sdk): lift SDK runtime deps to parent so tarball install can resolve them The SDK's runtime deps (ws, @anthropic-ai/claude-agent-sdk) live in sdk/package.json, but sdk/node_modules is NOT shipped in the parent tarball — only sdk/dist, sdk/src, sdk/prompts, and sdk/package.json are. When a user runs `npm install -g get-shit-done-cc`, npm installs the parent's node_modules but never runs `npm install` inside the nested sdk/ directory. Result: `node sdk/dist/cli.js` fails with ERR_MODULE_NOT_FOUND for 'ws'. The smoke tarball job caught this; the unpacked variant masked it because `npm install -g <dir>` copies the entire workspace including sdk/node_modules (left over from `npm run build:sdk`). Fix: declare the same deps in the parent package.json so they land in <pkg>/node_modules, which Node's resolution walks up to from <pkg>/sdk/dist/cli.js. Keep them declared in sdk/package.json too so the SDK remains a self-contained package for standalone dev. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(lockfile): regenerate package-lock.json cleanly The previous `npm install` run left the lockfile internally inconsistent (resolved esbuild@0.27.7 referenced but not fully written), causing `npm ci` to fail in CI with "Missing from lock file" errors. Clean regen via rm + npm install fixes all three failed jobs (test, smoke, smoke-unpacked), which were all hitting the same `npm ci` sync check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(deps): remove unused esbuild + vitest from root devDependencies Both were declared but never imported anywhere in the root package (confirmed via grep of bin/, scripts/, tests/). They lived in sdk/ already, which is the only place they're actually used. The transitive tree they pulled in (vitest → vite → esbuild 0.28 → @esbuild/openharmony-arm64) was the root of the CI npm ci failures: the openharmony platform package's `optional: true` flag was not being applied correctly by npm 10 on Linux runners, causing EBADPLATFORM. After removal: 800+ transitive packages → 155. Lockfile regenerated cleanly. All 4170 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sdk): pretest:coverage builds sdk; tighten shim test assertions Add "pretest:coverage": "npm run build:sdk" so npm run test:coverage works in clean checkouts where sdk/dist/ hasn't been built yet. Tighten the two loose shim assertions in bug-2441-sdk-decouple.test.cjs: - forwards-to test now asserts path.resolve() is called with the 'sdk','dist','cli.js' path segments, not just substring presence - node-invocation test now asserts spawnSync(process.execPath, [...]) pattern, ruling out matches in comments or the shebang line Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: address PR review — pretest:coverage + tighten shim tests Review feedback from trek-e on PR 2457: 1. pretest:coverage + pretest hooks now run `npm run build:sdk` so `npm run test[:coverage]` in a clean checkout produces the required sdk/dist/ artifacts before running the installer-dependent tests. CI already does this explicitly; local contributors benefit. 2. Shim tests in bug-2441-sdk-decouple.test.cjs tightened from loose substring matches (which would pass on comments/shebangs alone) to regex assertions on the actual path.resolve call, spawnSync with process.execPath, process.argv.slice(2), and process.exit pattern. These now provide real regression protection for #2453-class bugs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: correct CHANGELOG entry and add [1.38.2] reference link Two issues in the 1.38.2 CHANGELOG entry: - installSdkIfNeeded() was described as deleted but it still exists in bin/install.js (repurposed to verify sdk/dist/cli.js and fix execute bit). Corrected the description to say 'repurposes' rather than 'deletes'. - The reference-link block at the bottom of the file was missing a [1.38.2] compare URL and [Unreleased] still pointed to v1.37.1...HEAD. Added the [1.38.2] link and updated [Unreleased] to compare/v1.38.2...HEAD. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(sdk): double-cast WorkflowConfig to Record for strict tsc build TypeScript error on main (introduced in #2611) blocks `npm run build` in sdk/, which now runs as part of this PR's tarball build path. Apply the double-cast via `unknown` as the compiler suggests. Same fix as #2622; can be dropped if that lands first. * test: remove bug-2598 test obsoleted by SDK decoupling The bug-2598 test guards the Windows CVE-2024-27980 fix in the old build-from-source path (npm spawnSync with shell:true + formatSpawnFailure diagnostics). This PR removes that entire code path — installSdkIfNeeded no longer spawns npm, it just verifies the prebuilt sdk/dist/cli.js shipped in the tarball. The test asserts `installSdkIfNeeded.toString()` contains a formatSpawnFailure helper. After decoupling, no such helper exists (nothing to format — there's no spawn). Keeping the test would assert invariants of the rejected architecture. The original #2598 defect (silent failure of npm spawn on Windows) is structurally impossible in the shim path: bin/gsd-sdk.js invokes `node sdk/dist/cli.js` directly via child_process.spawn with an explicit argv array. No .cmd wrapper, no shell delegation. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
Jeremy McSpadden
165
tests/bug-2441-sdk-decouple.test.cjs
Normal file
165
tests/bug-2441-sdk-decouple.test.cjs
Normal file
@@ -0,0 +1,165 @@
|
||||
/**
|
||||
* Regression tests for fix/2441-sdk-decouple
|
||||
*
|
||||
* Verifies the architectural invariants introduced by the SDK decouple:
|
||||
*
|
||||
* (a) bin/install.js does NOT invoke `npm install -g` for the SDK at all.
|
||||
* The old `installSdkIfNeeded()` built from source and ran `npm install -g .`
|
||||
* in sdk/; the new version only verifies the prebuilt dist.
|
||||
*
|
||||
* (b) The parent package.json declares a `gsd-sdk` bin entry pointing at
|
||||
* bin/gsd-sdk.js (the back-compat shim), so npm chmods it correctly.
|
||||
*
|
||||
* (c) sdk/dist/ is in the parent package `files` so it ships in the tarball.
|
||||
*
|
||||
* (d) sdk/package.json `prepublishOnly` runs `rm -rf dist && tsc && chmod +x dist/cli.js`
|
||||
* (guards against the mode-644 bug and npm's stale-prepublishOnly issue).
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
const { test, describe } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
|
||||
const INSTALL_JS = path.join(__dirname, '..', 'bin', 'install.js');
|
||||
const ROOT_PKG = path.join(__dirname, '..', 'package.json');
|
||||
const SDK_PKG = path.join(__dirname, '..', 'sdk', 'package.json');
|
||||
const GSD_SDK_SHIM = path.join(__dirname, '..', 'bin', 'gsd-sdk.js');
|
||||
|
||||
const installContent = fs.readFileSync(INSTALL_JS, 'utf-8');
|
||||
const rootPkg = JSON.parse(fs.readFileSync(ROOT_PKG, 'utf-8'));
|
||||
const sdkPkg = JSON.parse(fs.readFileSync(SDK_PKG, 'utf-8'));
|
||||
|
||||
describe('fix #2441: SDK decouple — installer no longer builds from source', () => {
|
||||
test('bin/install.js does not call npm install -g in sdk/', () => {
|
||||
// The old approach ran `npm install -g .` from sdk/. This must be gone.
|
||||
// We check for the specific pattern that installed the SDK globally.
|
||||
const hasGlobalInstallFromSdk =
|
||||
/spawnSync\(npmCmd,\s*\[['"]install['"],\s*['"](-g|--global)['"]/m.test(installContent) &&
|
||||
/cwd:\s*sdkDir/.test(installContent);
|
||||
assert.ok(
|
||||
!hasGlobalInstallFromSdk,
|
||||
'bin/install.js must not run `npm install -g .` from sdk/. ' +
|
||||
'The SDK is shipped prebuilt in the tarball (fix #2441).'
|
||||
);
|
||||
});
|
||||
|
||||
test('bin/install.js does not run npm run build in sdk/', () => {
|
||||
// The old approach ran `npm run build` (tsc) at install time.
|
||||
const hasBuildStep =
|
||||
/spawnSync\(npmCmd,\s*\[['"]run['"],\s*['"]build['"]\]/m.test(installContent) &&
|
||||
/cwd:\s*sdkDir/.test(installContent);
|
||||
assert.ok(
|
||||
!hasBuildStep,
|
||||
'bin/install.js must not run `npm run build` in sdk/ at install time. ' +
|
||||
'TypeScript compilation happens at publish time via prepublishOnly.'
|
||||
);
|
||||
});
|
||||
|
||||
test('installSdkIfNeeded checks sdk/dist/cli.js exists instead of building', () => {
|
||||
assert.ok(
|
||||
installContent.includes('sdk/dist/cli.js') || installContent.includes("'dist', 'cli.js'"),
|
||||
'installSdkIfNeeded() must reference sdk/dist/cli.js to verify the prebuilt dist.'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('fix #2441: back-compat shim — parent package bin entry', () => {
|
||||
test('root package.json declares gsd-sdk bin entry', () => {
|
||||
assert.ok(
|
||||
rootPkg.bin && rootPkg.bin['gsd-sdk'],
|
||||
'root package.json must have a bin["gsd-sdk"] entry for the back-compat shim.'
|
||||
);
|
||||
});
|
||||
|
||||
test('gsd-sdk bin entry points at bin/gsd-sdk.js', () => {
|
||||
assert.equal(
|
||||
rootPkg.bin['gsd-sdk'],
|
||||
'bin/gsd-sdk.js',
|
||||
'bin["gsd-sdk"] must point at bin/gsd-sdk.js'
|
||||
);
|
||||
});
|
||||
|
||||
test('bin/gsd-sdk.js shim file exists', () => {
|
||||
assert.ok(
|
||||
fs.existsSync(GSD_SDK_SHIM),
|
||||
'bin/gsd-sdk.js must exist as the back-compat PATH shim.'
|
||||
);
|
||||
});
|
||||
|
||||
test('bin/gsd-sdk.js resolves sdk/dist/cli.js relative to itself', () => {
|
||||
const shimContent = fs.readFileSync(GSD_SDK_SHIM, 'utf-8');
|
||||
// Require the actual path.resolve call with the expected segments, not
|
||||
// loose substring matches that would pass from comments or shebangs.
|
||||
assert.match(
|
||||
shimContent,
|
||||
/path\.resolve\(\s*__dirname\s*,\s*['"]\.\.['"]\s*,\s*['"]sdk['"]\s*,\s*['"]dist['"]\s*,\s*['"]cli\.js['"]\s*\)/,
|
||||
'bin/gsd-sdk.js must call path.resolve(__dirname, "..", "sdk", "dist", "cli.js") to locate the prebuilt CLI.'
|
||||
);
|
||||
});
|
||||
|
||||
test('bin/gsd-sdk.js invokes cli.js via spawnSync(process.execPath, ...)', () => {
|
||||
const shimContent = fs.readFileSync(GSD_SDK_SHIM, 'utf-8');
|
||||
// The shim must invoke via node (not rely on execute bit), which means
|
||||
// spawnSync(process.execPath, [cliPath, ...args]).
|
||||
assert.match(
|
||||
shimContent,
|
||||
/spawnSync\(\s*process\.execPath\s*,/,
|
||||
'bin/gsd-sdk.js must spawn node via process.execPath so the execute bit on cli.js is irrelevant (#2453).'
|
||||
);
|
||||
assert.match(
|
||||
shimContent,
|
||||
/process\.argv\.slice\(\s*2\s*\)/,
|
||||
'bin/gsd-sdk.js must forward user args via process.argv.slice(2).'
|
||||
);
|
||||
assert.match(
|
||||
shimContent,
|
||||
/process\.exit\(/,
|
||||
'bin/gsd-sdk.js must propagate the child exit status via process.exit.'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('fix #2441: sdk/dist shipped in tarball', () => {
|
||||
test('root package.json files includes sdk/dist', () => {
|
||||
assert.ok(
|
||||
Array.isArray(rootPkg.files) && rootPkg.files.some(f => f === 'sdk/dist' || f.startsWith('sdk/dist')),
|
||||
'root package.json files must include "sdk/dist" so the prebuilt CLI ships in the tarball.'
|
||||
);
|
||||
});
|
||||
|
||||
test('root package.json files still includes sdk/src (for dev/clone builds)', () => {
|
||||
assert.ok(
|
||||
Array.isArray(rootPkg.files) && rootPkg.files.some(f => f === 'sdk/src' || f.startsWith('sdk/src')),
|
||||
'root package.json files should still include sdk/src for developer builds.'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('fix #2453: sdk/package.json prepublishOnly guards execute bit', () => {
|
||||
test('sdk prepublishOnly deletes old dist before build (npm stale-prepublishOnly guard)', () => {
|
||||
const prepub = sdkPkg.scripts && sdkPkg.scripts.prepublishOnly;
|
||||
assert.ok(
|
||||
prepub && prepub.includes('rm -rf dist'),
|
||||
'sdk/package.json prepublishOnly must start with `rm -rf dist` to avoid stale build output.'
|
||||
);
|
||||
});
|
||||
|
||||
test('sdk prepublishOnly chmods dist/cli.js after tsc', () => {
|
||||
const prepub = sdkPkg.scripts && sdkPkg.scripts.prepublishOnly;
|
||||
assert.ok(
|
||||
prepub && prepub.includes('chmod +x dist/cli.js'),
|
||||
'sdk/package.json prepublishOnly must run `chmod +x dist/cli.js` after tsc to fix mode-644 (#2453).'
|
||||
);
|
||||
});
|
||||
|
||||
test('sdk prepublishOnly runs tsc', () => {
|
||||
const prepub = sdkPkg.scripts && sdkPkg.scripts.prepublishOnly;
|
||||
assert.ok(
|
||||
prepub && prepub.includes('tsc'),
|
||||
'sdk/package.json prepublishOnly must include tsc to compile TypeScript.'
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -1,88 +0,0 @@
|
||||
/**
|
||||
* Regression test for bug #2453
|
||||
*
|
||||
* installSdkIfNeeded() builds sdk/dist/cli.js via `tsc` then runs
|
||||
* `npm install -g .`. TypeScript emits files at process umask (0o644) and
|
||||
* npm install from a local directory does NOT chmod bin-script targets the
|
||||
* way tarball extraction does. The result: the globally-installed
|
||||
* dist/cli.js lands with mode 644 (non-executable), the `gsd-sdk` symlink
|
||||
* points at a non-executable file, and `command -v gsd-sdk` fails on every
|
||||
* new install.
|
||||
*
|
||||
* Fix: after `npm install -g .`, the installer must explicitly
|
||||
* `chmodSync(cliPath, 0o755)` on the installed dist/cli.js. This mirrors
|
||||
* the pattern already used four times in install.js for hook files.
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
const { test, describe } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
|
||||
const INSTALL_SRC = path.join(__dirname, '..', 'bin', 'install.js');
|
||||
|
||||
describe('bug #2453: installSdkIfNeeded chmods sdk dist/cli.js to 0o755', () => {
|
||||
let installSrc;
|
||||
|
||||
test('install.js source exists', () => {
|
||||
assert.ok(fs.existsSync(INSTALL_SRC), 'bin/install.js must exist');
|
||||
installSrc = fs.readFileSync(INSTALL_SRC, 'utf-8');
|
||||
});
|
||||
|
||||
test('installSdkIfNeeded contains a chmodSync call for dist/cli.js', () => {
|
||||
installSrc = installSrc || fs.readFileSync(INSTALL_SRC, 'utf-8');
|
||||
|
||||
// Locate the installSdkIfNeeded function body
|
||||
const fnStart = installSrc.indexOf('function installSdkIfNeeded()');
|
||||
assert.ok(fnStart !== -1, 'installSdkIfNeeded function must exist in install.js');
|
||||
|
||||
// Find the end of the function (next top-level function declaration)
|
||||
const fnEnd = installSrc.indexOf('\nfunction ', fnStart + 1);
|
||||
const fnBody = fnEnd !== -1 ? installSrc.slice(fnStart, fnEnd) : installSrc.slice(fnStart);
|
||||
|
||||
// Must chmod dist/cli.js to make it executable after npm install -g .
|
||||
const hasChmod = fnBody.includes('chmodSync') && fnBody.includes('dist/cli.js');
|
||||
assert.ok(
|
||||
hasChmod,
|
||||
'installSdkIfNeeded must call chmodSync on dist/cli.js after npm install -g . ' +
|
||||
'(tsc emits 644; npm does not chmod bin targets from local dir installs — ' +
|
||||
'root cause of #2453: gsd-sdk symlink target is non-executable on first install)'
|
||||
);
|
||||
});
|
||||
|
||||
test('chmodSync for dist/cli.js uses mode 0o755', () => {
|
||||
installSrc = installSrc || fs.readFileSync(INSTALL_SRC, 'utf-8');
|
||||
|
||||
const fnStart = installSrc.indexOf('function installSdkIfNeeded()');
|
||||
const fnEnd = installSrc.indexOf('\nfunction ', fnStart + 1);
|
||||
const fnBody = fnEnd !== -1 ? installSrc.slice(fnStart, fnEnd) : installSrc.slice(fnStart);
|
||||
|
||||
// The chmod call must use 0o755 (executable), not 0o644
|
||||
const has755 = fnBody.includes('0o755') && fnBody.includes('dist/cli.js');
|
||||
assert.ok(
|
||||
has755,
|
||||
'chmodSync for dist/cli.js must use mode 0o755 to make the binary executable'
|
||||
);
|
||||
});
|
||||
|
||||
test('chmodSync appears after npm install -g . step', () => {
|
||||
installSrc = installSrc || fs.readFileSync(INSTALL_SRC, 'utf-8');
|
||||
|
||||
const fnStart = installSrc.indexOf('function installSdkIfNeeded()');
|
||||
const fnEnd = installSrc.indexOf('\nfunction ', fnStart + 1);
|
||||
const fnBody = fnEnd !== -1 ? installSrc.slice(fnStart, fnEnd) : installSrc.slice(fnStart);
|
||||
|
||||
const npmGlobalIdx = fnBody.indexOf("'install', '-g', '.'");
|
||||
const chmodIdx = fnBody.indexOf('chmodSync');
|
||||
|
||||
assert.ok(npmGlobalIdx !== -1, "npm install -g . step must be present in installSdkIfNeeded");
|
||||
assert.ok(chmodIdx !== -1, 'chmodSync must be present in installSdkIfNeeded');
|
||||
assert.ok(
|
||||
chmodIdx > npmGlobalIdx,
|
||||
'chmodSync must appear AFTER the npm install -g . step ' +
|
||||
'(the file to chmod does not exist until npm installs it globally)'
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -1,82 +0,0 @@
|
||||
/**
|
||||
* Regression test for bug #2525
|
||||
*
|
||||
* After running the GSD installer on macOS with a Homebrew npm prefix,
|
||||
* `gsd-sdk` is installed but `command -v gsd-sdk` returns nothing because
|
||||
* `dist/cli.js` is installed with mode 644 (no executable bit). tsc emits
|
||||
* .js files as 644, and `npm install -g .` creates the bin symlink without
|
||||
* chmod-ing the target. The kernel then refuses to exec the file.
|
||||
*
|
||||
* Fix: between the `npm run build` step and `npm install -g .`, chmod
|
||||
* dist/cli.js to 0o755. This mirrors the pattern already used for hook
|
||||
* files at lines 5838, 5846, 5959, and 5965.
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
const { test, describe } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
|
||||
const INSTALL_PATH = path.join(__dirname, '..', 'bin', 'install.js');
|
||||
|
||||
describe('bug #2525: dist/cli.js chmod 0o755 after tsc build', () => {
|
||||
test('install.js exists', () => {
|
||||
assert.ok(fs.existsSync(INSTALL_PATH), 'bin/install.js should exist');
|
||||
});
|
||||
|
||||
test('chmodSync is called for dist/cli.js after the build step', () => {
|
||||
const content = fs.readFileSync(INSTALL_PATH, 'utf-8');
|
||||
// Find the installSdkIfNeeded function body
|
||||
const fnStart = content.indexOf('function installSdkIfNeeded()');
|
||||
assert.ok(fnStart !== -1, 'installSdkIfNeeded function must exist in bin/install.js');
|
||||
|
||||
// Find the closing brace of the function (next top-level function definition)
|
||||
const nextFnIdx = content.indexOf('\nfunction ', fnStart + 1);
|
||||
const fnEnd = nextFnIdx === -1 ? content.length : nextFnIdx;
|
||||
const fnBody = content.slice(fnStart, fnEnd);
|
||||
|
||||
// Locate the build step
|
||||
const buildStep = fnBody.indexOf("'run', 'build'");
|
||||
assert.ok(buildStep !== -1, "installSdkIfNeeded must contain the 'run', 'build' spawn call");
|
||||
|
||||
// Locate the global install step
|
||||
const globalStep = fnBody.indexOf("'install', '-g', '.'");
|
||||
assert.ok(globalStep !== -1, "installSdkIfNeeded must contain the 'install', '-g', '.' spawn call");
|
||||
|
||||
// Locate chmodSync for dist/cli.js
|
||||
const chmodIdx = fnBody.indexOf("chmodSync");
|
||||
assert.ok(chmodIdx !== -1, "installSdkIfNeeded must call chmodSync to set the executable bit on dist/cli.js");
|
||||
|
||||
// The path may be assembled via path.join(sdkDir, 'dist', 'cli.js') so check
|
||||
// for the component strings rather than the joined slash form.
|
||||
const cliPathIdx = fnBody.indexOf("'cli.js'");
|
||||
assert.ok(cliPathIdx !== -1, "installSdkIfNeeded must reference 'cli.js' (via path.join or literal) for the chmod call");
|
||||
|
||||
// chmodSync must appear AFTER the build step
|
||||
assert.ok(
|
||||
chmodIdx > buildStep,
|
||||
'chmodSync for dist/cli.js must appear AFTER the npm run build step'
|
||||
);
|
||||
|
||||
// chmodSync must appear AFTER the global install step
|
||||
assert.ok(
|
||||
chmodIdx > globalStep,
|
||||
'chmodSync for dist/cli.js must appear AFTER the npm install -g . step'
|
||||
);
|
||||
});
|
||||
|
||||
test('chmod mode is 0o755', () => {
|
||||
const content = fs.readFileSync(INSTALL_PATH, 'utf-8');
|
||||
const fnStart = content.indexOf('function installSdkIfNeeded()');
|
||||
const nextFnIdx = content.indexOf('\nfunction ', fnStart + 1);
|
||||
const fnEnd = nextFnIdx === -1 ? content.length : nextFnIdx;
|
||||
const fnBody = content.slice(fnStart, fnEnd);
|
||||
|
||||
assert.ok(
|
||||
fnBody.includes('0o755'),
|
||||
"chmodSync call in installSdkIfNeeded must use mode 0o755 (not 0o644 or a bare number)"
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -1,138 +0,0 @@
|
||||
/**
|
||||
* Regression test for bug #2598
|
||||
*
|
||||
* On Windows, Node.js refuses to spawn `.cmd`/`.bat` files via spawnSync
|
||||
* unless `shell: true` is passed (post CVE-2024-27980, fixed in
|
||||
* Node >= 18.20.2 / >= 20.12.2 / >= 21.7.3 and every version since).
|
||||
*
|
||||
* installSdkIfNeeded() picks `npmCmd = 'npm.cmd'` on win32 and then shells
|
||||
* out five times: `npm install`, `npm run build`, `npm install -g .`, and
|
||||
* two `npm config get prefix` calls. Without `shell: true`, every single
|
||||
* one of those returns `{ status: null, error: EINVAL }` before npm ever
|
||||
* launches. The installer checks `status !== 0` (null !== 0 is true) and
|
||||
* trips its failure path — producing a silent SDK build failure with zero
|
||||
* diagnostic output because `stdio: 'inherit'` never got a child to stream.
|
||||
*
|
||||
* Fix: every spawnSync that targets `npmCmd` inside installSdkIfNeeded
|
||||
* must pass `shell: true` on Windows. The structural invariant verified
|
||||
* here is that no bare `spawnSync(npmCmd, ...)` remains inside the
|
||||
* function — all npm invocations go through a helper that injects the
|
||||
* shell option on win32 (or have it explicitly on the call site).
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
const { test, describe } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
|
||||
const INSTALL_SRC = path.join(__dirname, '..', 'bin', 'install.js');
|
||||
|
||||
function installSdkBody() {
|
||||
const src = fs.readFileSync(INSTALL_SRC, 'utf-8');
|
||||
const fnStart = src.indexOf('function installSdkIfNeeded()');
|
||||
assert.ok(fnStart !== -1, 'installSdkIfNeeded function must exist in install.js');
|
||||
const fnEnd = src.indexOf('\nfunction ', fnStart + 1);
|
||||
return fnEnd !== -1 ? src.slice(fnStart, fnEnd) : src.slice(fnStart);
|
||||
}
|
||||
|
||||
describe('bug #2598: Windows npm.cmd spawnSync must pass shell: true', () => {
|
||||
test('install.js source exists', () => {
|
||||
assert.ok(fs.existsSync(INSTALL_SRC), 'bin/install.js must exist');
|
||||
});
|
||||
|
||||
test('installSdkIfNeeded does not call spawnSync(npmCmd, ...) directly', () => {
|
||||
const body = installSdkBody();
|
||||
// A bare `spawnSync(npmCmd,` call will fail with EINVAL on Windows
|
||||
// because npm.cmd requires `shell: true`. Every npm invocation must
|
||||
// go through a wrapper that injects the shell option on win32.
|
||||
//
|
||||
// Exclude the implementation of the wrapper itself (its `spawnSync(npmCmd, args, ...)`
|
||||
// line is the ONE legitimate place that spawns npmCmd directly — and it forwards
|
||||
// a shell option from `opts`).
|
||||
const allCalls = body.match(/spawnSync\s*\(\s*npmCmd[^)]*\)/g) || [];
|
||||
const bareCalls = allCalls.filter(c => !/\bshell\s*:/.test(c));
|
||||
assert.equal(
|
||||
bareCalls.length,
|
||||
0,
|
||||
'installSdkIfNeeded must not call spawnSync(npmCmd, ...) directly. ' +
|
||||
'On Windows, npm.cmd spawns fail with EINVAL (CVE-2024-27980) unless ' +
|
||||
'shell: true is passed. Route all npm invocations through a helper ' +
|
||||
'that injects `shell: process.platform === "win32"`.'
|
||||
);
|
||||
});
|
||||
|
||||
test('installSdkIfNeeded defines a shell-aware npm wrapper', () => {
|
||||
const body = installSdkBody();
|
||||
// The canonical implementation introduces `spawnNpm` (or equivalent)
|
||||
// that injects `shell: true` on win32. Accept either a helper
|
||||
// function or `shell: true` / `shell: needsShell` / `shell: win32`
|
||||
// appearing alongside an npm spawn.
|
||||
const hasHelper = /\bspawnNpm\b/.test(body);
|
||||
const hasShellOption = /shell\s*:\s*(true|needsShell|process\.platform\s*===\s*['"]win32['"])/.test(body);
|
||||
assert.ok(
|
||||
hasHelper || hasShellOption,
|
||||
'installSdkIfNeeded must introduce a helper or pass shell:true/win32-aware ' +
|
||||
'shell option for npm spawnSync calls. Found neither.'
|
||||
);
|
||||
});
|
||||
|
||||
test('installSdkIfNeeded calls the npm wrapper at least five times', () => {
|
||||
const body = installSdkBody();
|
||||
// Five documented npm invocations: install, run build, install -g .,
|
||||
// and two config get prefix calls. If any are still bare spawnSync
|
||||
// calls targeting npmCmd, the first assertion above already fails.
|
||||
// `\bspawnNpm\s*\(` matches real call sites only — a `const spawnNpm = (…)`
|
||||
// declaration does not match because `=` sits between name and `(`. A
|
||||
// `function spawnNpm(…)` declaration WOULD match, so subtract one for that
|
||||
// form. `explicitShellNpm` previously double-counted the wrapper's own
|
||||
// `spawnSync(npmCmd, args, { …, shell })` — when a helper exists, its body
|
||||
// is the only legitimate raw spawn and must be excluded.
|
||||
const wrapperCallMatches = (body.match(/\bspawnNpm\s*\(/g) || []).length;
|
||||
const hasArrowHelper = /\b(?:const|let|var)\s+spawnNpm\s*=/.test(body);
|
||||
const hasFunctionHelper = /\bfunction\s+spawnNpm\s*\(/.test(body);
|
||||
const hasHelper = hasArrowHelper || hasFunctionHelper;
|
||||
const wrapperCalls = hasFunctionHelper
|
||||
? Math.max(0, wrapperCallMatches - 1)
|
||||
: wrapperCallMatches;
|
||||
const explicitShellNpm = hasHelper
|
||||
? 0
|
||||
: (body.match(/spawnSync\s*\(\s*npmCmd[^)]*\bshell\s*:/g) || []).length;
|
||||
const total = wrapperCalls + explicitShellNpm;
|
||||
assert.ok(
|
||||
total >= 5,
|
||||
`installSdkIfNeeded should route at least 5 npm invocations through ` +
|
||||
`a shell-aware wrapper (install, run build, install -g ., and two ` +
|
||||
`config get prefix calls). Found ${total}.`
|
||||
);
|
||||
});
|
||||
|
||||
test('installSdkIfNeeded surfaces underlying spawnSync failure in fatal branches', () => {
|
||||
// Root cause of #2598 was invisible because `{ status: null, error: EINVAL }`
|
||||
// was reduced to a generic "Failed to `npm install` in sdk/." with no
|
||||
// diagnostic — stdio: 'inherit' had no child to stream and result.error was
|
||||
// dropped. Each of the three `emitSdkFatal` calls inside the install/build/
|
||||
// global-install failure paths must now thread spawn diagnostics (error,
|
||||
// signal, or numeric status) into the reason string so future regressions
|
||||
// print their real cause instead of failing silently.
|
||||
const body = installSdkBody();
|
||||
const hasFormatter = /formatSpawnFailure\s*\(/.test(body);
|
||||
const fatalCalls = body.match(/emitSdkFatal\s*\([^)]*`[^`]*`[^)]*\)/gs) || [];
|
||||
const fatalWithDiagnostic = fatalCalls.filter(
|
||||
(c) => /formatSpawnFailure|\.error|\.signal|\.status/.test(c),
|
||||
);
|
||||
assert.ok(
|
||||
hasFormatter,
|
||||
'installSdkIfNeeded must define a spawn-failure formatter so fatal ' +
|
||||
'npm failures surface result.error / result.signal / result.status ' +
|
||||
'instead of swallowing them (root cause of #2598 being invisible).',
|
||||
);
|
||||
assert.ok(
|
||||
fatalWithDiagnostic.length >= 3,
|
||||
`At least 3 emitSdkFatal calls (install, build, install -g .) must ` +
|
||||
`include spawn diagnostics. Found ${fatalWithDiagnostic.length} that ` +
|
||||
`reference formatSpawnFailure or result.error/signal/status.`,
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -80,35 +80,42 @@ describe('#1657 / #2385: SDK install must be wired into installer source', () =>
|
||||
);
|
||||
});
|
||||
|
||||
test('install.js builds gsd-sdk from in-repo sdk/ source (#2385)', () => {
|
||||
test('install.js verifies prebuilt sdk/dist/cli.js instead of building from source (#2441)', () => {
|
||||
src = src || fs.readFileSync(INSTALL_SRC, 'utf-8');
|
||||
// The installer must locate the in-repo sdk/ directory, run the build,
|
||||
// and install it globally. We intentionally do NOT install
|
||||
// @gsd-build/sdk from npm because that published version lags the source
|
||||
// tree and shipping it breaks query handlers added since the last
|
||||
// publish.
|
||||
// As of fix/2441-sdk-decouple, the installer no longer runs `npm run build`
|
||||
// or `npm install -g .` from sdk/. Instead it verifies sdk/dist/cli.js exists
|
||||
// (shipped prebuilt in the tarball) and optionally chmods it.
|
||||
assert.ok(
|
||||
src.includes("path.resolve(__dirname, '..', 'sdk')") ||
|
||||
src.includes('path.resolve(__dirname, "..", "sdk")'),
|
||||
'installer must locate the in-repo sdk/ directory'
|
||||
src.includes('sdk/dist/cli.js') || src.includes("'dist', 'cli.js'"),
|
||||
'installer must reference sdk/dist/cli.js to verify the prebuilt dist (#2441)'
|
||||
);
|
||||
// Confirm the old build-from-source pattern is gone.
|
||||
const hasBuildFromSource =
|
||||
src.includes("['run', 'build']") &&
|
||||
src.includes("cwd: sdkDir");
|
||||
assert.ok(
|
||||
src.includes("'npm install -g .'") ||
|
||||
src.includes("['install', '-g', '.']"),
|
||||
'installer must run `npm install -g .` from sdk/ to install the built package globally'
|
||||
!hasBuildFromSource,
|
||||
'installer must NOT run `npm run build` from sdk/ at install time (#2441)'
|
||||
);
|
||||
const hasGlobalInstall =
|
||||
(src.includes("['install', '-g', '.']") || src.includes("'npm install -g .'")) &&
|
||||
src.includes("cwd: sdkDir");
|
||||
assert.ok(
|
||||
src.includes("['run', 'build']"),
|
||||
'installer must compile TypeScript via `npm run build` before installing globally'
|
||||
!hasGlobalInstall,
|
||||
'installer must NOT run `npm install -g .` from sdk/ (#2441)'
|
||||
);
|
||||
});
|
||||
|
||||
test('package.json ships sdk source in published tarball (#2385)', () => {
|
||||
test('package.json ships sdk dist and source in published tarball (#2441)', () => {
|
||||
const rootPkg = JSON.parse(fs.readFileSync(path.join(__dirname, '..', 'package.json'), 'utf-8'));
|
||||
const files = rootPkg.files || [];
|
||||
assert.ok(
|
||||
files.some((f) => f === 'sdk' || f.startsWith('sdk/')),
|
||||
'root package.json `files` must include sdk source so npm-registry installs can build gsd-sdk from source'
|
||||
files.some((f) => f === 'sdk/src' || f.startsWith('sdk/src')),
|
||||
'root package.json `files` must include sdk/src'
|
||||
);
|
||||
assert.ok(
|
||||
files.some((f) => f === 'sdk/dist' || f.startsWith('sdk/dist')),
|
||||
'root package.json `files` must include sdk/dist so the prebuilt CLI ships in the tarball (#2441)'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user