From 8caa7d4c3a2aff6ed49726ec707bf9aa7bb1f002 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 24 Apr 2026 18:05:04 -0400 Subject: [PATCH] fix(#2649): installer fail-fast when sdk/dist missing in npx cache (#2667) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause shared with #2647: a broken 1.38.3 tarball shipped without sdk/dist/. The pre-#2441-decouple installer reacted by running spawnSync('npm.cmd', ['install'], { cwd: sdkDir }) inside the npx cache on Windows, where the cache is read-only, producing the misleading "Failed to npm install in sdk/" error. Defensive changes here (user-facing behavior only; packaging fix lives in the sibling PR for #2647): - Classify the install context (classifySdkInstall): detect npx cache paths, node_modules-based installs, and dev clones via path heuristics plus a side-effect-free write probe. Exported for test. - Rewrite the dist-missing error to branch on context: tarball + npxCache -> "don't touch npx cache; npm i -g ...@latest" tarball (other) -> upgrade path + clone-build escape hatch dev-clone -> keep existing cd sdk && npm install && npm run build - Preserve the invariant that the installer never shells out to npm install itself — users always drive that. - Add tests/bug-2649-sdk-fail-fast.test.cjs covering the classifier and both failure messages, with spawnSync/execSync interceptors that assert no nested npm install is attempted. Cross-ref: #2647 (packaging). Fixes #2649 Co-authored-by: Claude Opus 4.7 (1M context) --- bin/install.js | 104 ++++++++++++++- tests/bug-2649-sdk-fail-fast.test.cjs | 179 ++++++++++++++++++++++++++ 2 files changed, 277 insertions(+), 6 deletions(-) create mode 100644 tests/bug-2649-sdk-fail-fast.test.cjs diff --git a/bin/install.js b/bin/install.js index 90d6da086..e5a6a4d53 100755 --- a/bin/install.js +++ b/bin/install.js @@ -7023,8 +7023,72 @@ function maybeSuggestPathExport(globalBin, homeDir) { * --no-sdk skips the check entirely (back-compat). * --sdk forces the check even if it would otherwise be skipped. */ -function installSdkIfNeeded() { - if (hasNoSdk) { +/** + * Classify the install context for the SDK directory. + * + * Distinguishes three shapes the installer must handle differently when + * `sdk/dist/` is missing: + * + * - `tarball` + `npxCache: true` + * User ran `npx get-shit-done-cc@latest`. sdk/ lives under + * `/_npx//node_modules/get-shit-done-cc/sdk` which + * is treated as read-only by npm/npx on Windows (#2649). We MUST + * NOT attempt a nested `npm install` there — it will fail with + * EACCES/EPERM and produce the misleading "Failed to npm install + * in sdk/" error the user reported. Point at the global upgrade. + * + * - `tarball` + `npxCache: false` + * User ran a global install (`npm i -g get-shit-done-cc`). sdk/dist + * ships in the published tarball; if it's missing, the published + * artifact itself is broken (see #2647). Same user-facing fix: + * upgrade to latest. + * + * - `dev-clone` + * Developer running from a git clone. Keep the existing "cd sdk && + * npm install && npm run build" hint — the user is expected to run + * that themselves. The installer itself never shells out to npm. + * + * Detection heuristics are path-based and side-effect-free: we look for + * `_npx` and `node_modules` segments that indicate a packaged install, + * and for a `.git` directory nearby that indicates a clone. A best-effort + * write probe detects read-only filesystems (tmpfile create + unlink); + * probe failures are treated as read-only. + */ +function classifySdkInstall(sdkDir) { + const path = require('path'); + const fs = require('fs'); + const segments = sdkDir.split(/[\\/]+/); + const npxCache = segments.includes('_npx'); + const inNodeModules = segments.includes('node_modules'); + const parent = path.dirname(sdkDir); + const hasGitNearby = fs.existsSync(path.join(parent, '.git')); + + let mode; + if (hasGitNearby && !npxCache && !inNodeModules) { + mode = 'dev-clone'; + } else if (npxCache || inNodeModules) { + mode = 'tarball'; + } else { + mode = 'dev-clone'; + } + + let readOnly = npxCache; // assume true for npx cache + if (!readOnly) { + try { + const probe = path.join(sdkDir, `.gsd-write-probe-${process.pid}`); + fs.writeFileSync(probe, ''); + fs.unlinkSync(probe); + } catch { + readOnly = true; + } + } + + return { mode, npxCache, readOnly }; +} + +function installSdkIfNeeded(opts) { + opts = opts || {}; + if (hasNoSdk && !opts.sdkDir) { console.log(`\n ${dim}Skipping GSD SDK check (--no-sdk)${reset}`); return; } @@ -7032,9 +7096,11 @@ function installSdkIfNeeded() { const path = require('path'); const fs = require('fs'); - const sdkCliPath = path.resolve(__dirname, '..', 'sdk', 'dist', 'cli.js'); + const sdkDir = opts.sdkDir || path.resolve(__dirname, '..', 'sdk'); + const sdkCliPath = path.join(sdkDir, 'dist', 'cli.js'); if (!fs.existsSync(sdkCliPath)) { + const ctx = classifySdkInstall(sdkDir); const bar = '━'.repeat(72); const redBold = `${red}${bold}`; console.error(''); @@ -7043,9 +7109,33 @@ function installSdkIfNeeded() { console.error(`${redBold}${bar}${reset}`); console.error(` ${red}Reason:${reset} sdk/dist/cli.js not found at ${sdkCliPath}`); console.error(''); - console.error(` This should not happen with a published tarball install.`); - console.error(` If you are running from a git clone, build the SDK first:`); - console.error(` ${cyan}cd sdk && npm install && npm run build${reset}`); + + if (ctx.mode === 'tarball') { + // User install (including `npx get-shit-done-cc@latest`, which stages + // a read-only tarball under the npx cache). The sdk/dist/ artifact + // should ship in the published tarball. If it's missing, the only + // sane fix from the user's side is a fresh global install of a + // version that includes dist/. Do NOT attempt a nested `npm install` + // inside the (read-only) npx cache — that's the #2649 failure mode. + if (ctx.npxCache) { + console.error(` Detected read-only npx cache install (${dim}${sdkDir}${reset}).`); + console.error(` The installer will ${bold}not${reset} attempt \`npm install\` inside the npx cache.`); + console.error(''); + } else { + console.error(` The published tarball appears to be missing sdk/dist/ (see #2647).`); + console.error(''); + } + console.error(` Fix: install a version that ships sdk/dist/ globally:`); + console.error(` ${cyan}npm install -g get-shit-done-cc@latest${reset}`); + console.error(` Or, if you prefer a one-shot run, clear the npx cache first:`); + console.error(` ${cyan}npx --yes get-shit-done-cc@latest${reset}`); + console.error(` Or build from source (git clone):`); + console.error(` ${cyan}git clone https://github.com/gsd-build/get-shit-done && cd get-shit-done/sdk && npm install && npm run build${reset}`); + } else { + // Dev clone: keep the existing build-from-source hint. + console.error(` Running from a git clone — build the SDK first:`); + console.error(` ${cyan}cd sdk && npm install && npm run build${reset}`); + } console.error(`${redBold}${bar}${reset}`); console.error(''); process.exit(1); @@ -7146,6 +7236,8 @@ if (process.env.GSD_TEST_MODE) { readGsdEffectiveModelOverrides, install, uninstall, + installSdkIfNeeded, + classifySdkInstall, convertClaudeCommandToCodexSkill, convertClaudeToOpencodeFrontmatter, convertClaudeToKiloFrontmatter, diff --git a/tests/bug-2649-sdk-fail-fast.test.cjs b/tests/bug-2649-sdk-fail-fast.test.cjs new file mode 100644 index 000000000..f6d3f0404 --- /dev/null +++ b/tests/bug-2649-sdk-fail-fast.test.cjs @@ -0,0 +1,179 @@ +/** + * Regression test for #2649 — installer must fail fast with a clear, + * actionable error when `sdk/dist/cli.js` is missing, and must NOT attempt + * a nested `npm install` inside the sdk directory (which, on Windows, lives + * in the read-only npx cache `%LOCALAPPDATA%\\npm-cache\\_npx\\\\...`). + * + * Shares a root cause with #2647 (packaging drops sdk/dist/). This test + * covers the installer's defensive behavior when that packaging bug — or + * any future regression that loses the prebuilt dist — reaches users. + */ + +'use strict'; + +const { test, describe, before } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); + +const INSTALL_PATH = path.join(__dirname, '..', 'bin', 'install.js'); + +function loadInstaller() { + process.env.GSD_TEST_MODE = '1'; + delete require.cache[require.resolve(INSTALL_PATH)]; + return require(INSTALL_PATH); +} + +function makeTempSdk({ npxCache = false } = {}) { + let root; + if (npxCache) { + root = path.join(os.tmpdir(), `gsd-npx-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`, 'npm-cache', '_npx', 'deadbeefcafe0001', 'node_modules', 'get-shit-done-cc'); + fs.mkdirSync(root, { recursive: true }); + } else { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-clone-')); + } + const sdkDir = path.join(root, 'sdk'); + fs.mkdirSync(sdkDir, { recursive: true }); + // Note: intentionally no sdk/dist/ directory. + return { root, sdkDir }; +} + +function cleanup(dir) { + try { fs.rmSync(dir, { recursive: true, force: true }); } catch {} +} + +function runWithIntercepts(fn) { + const stderr = []; + const stdout = []; + const origErr = console.error; + const origLog = console.log; + console.error = (...a) => stderr.push(a.join(' ')); + console.log = (...a) => stdout.push(a.join(' ')); + + const origExit = process.exit; + let exitCode = null; + process.exit = (code) => { exitCode = code; throw new Error('__EXIT__'); }; + + const cp = require('child_process'); + const origSpawnSync = cp.spawnSync; + const origExecSync = cp.execSync; + const spawnCalls = []; + cp.spawnSync = (cmd, argv, opts) => { + spawnCalls.push({ cmd, argv, opts }); + return { status: 0, stdout: Buffer.from(''), stderr: Buffer.from('') }; + }; + cp.execSync = (cmd, opts) => { + spawnCalls.push({ cmd, opts, via: 'execSync' }); + return Buffer.from(''); + }; + + try { + try { fn(); } catch (e) { if (e.message !== '__EXIT__') throw e; } + } finally { + console.error = origErr; + console.log = origLog; + process.exit = origExit; + cp.spawnSync = origSpawnSync; + cp.execSync = origExecSync; + } + + return { + stderr: stderr.join('\n'), + stdout: stdout.join('\n'), + exitCode, + spawnCalls, + }; +} + +describe('installer SDK dist-missing fail-fast (#2649)', () => { + let installer; + before(() => { installer = loadInstaller(); }); + + test('exposes test hooks for SDK check', () => { + assert.ok(typeof installer.installSdkIfNeeded === 'function', + 'installSdkIfNeeded must be exported in test mode'); + assert.ok(typeof installer.classifySdkInstall === 'function', + 'classifySdkInstall must be exported in test mode'); + }); + + test('classifySdkInstall tags npx cache paths as tarball + npxCache', () => { + const { root, sdkDir } = makeTempSdk({ npxCache: true }); + try { + const c = installer.classifySdkInstall(sdkDir); + assert.strictEqual(c.mode, 'tarball'); + assert.strictEqual(c.npxCache, true); + assert.ok('readOnly' in c); + } finally { + cleanup(root); + } + }); + + test('classifySdkInstall tags plain git-clone dirs as dev-clone', () => { + const { root, sdkDir } = makeTempSdk({ npxCache: false }); + try { + fs.mkdirSync(path.join(root, '.git'), { recursive: true }); + const c = installer.classifySdkInstall(sdkDir); + assert.strictEqual(c.mode, 'dev-clone'); + assert.strictEqual(c.npxCache, false); + } finally { + cleanup(root); + } + }); + + test('missing dist in npx cache: fail fast, no nested npm install', () => { + const { root, sdkDir } = makeTempSdk({ npxCache: true }); + try { + const result = runWithIntercepts(() => { + installer.installSdkIfNeeded({ sdkDir }); + }); + + assert.strictEqual(result.exitCode, 1, 'must exit non-zero'); + + // (a) actionable upgrade path in error output + assert.match(result.stderr, /npm i(nstall)? -g get-shit-done-cc@latest/, + 'error must mention the global-install upgrade path'); + assert.match(result.stderr, /sdk\/dist/, + 'error must name the missing artifact'); + + // (b) no nested `npm install` / `npm.cmd install` inside sdkDir + const nestedInstall = result.spawnCalls.find((c) => { + const argv = Array.isArray(c.argv) ? c.argv : []; + const cwd = c.opts && c.opts.cwd; + const isNpm = /\bnpm(\.cmd)?$/i.test(String(c.cmd || '')); + const isInstall = argv.includes('install') || argv.includes('i'); + const isInSdk = typeof cwd === 'string' && cwd.includes(sdkDir); + return isNpm && isInstall && isInSdk; + }); + assert.strictEqual(nestedInstall, undefined, + 'must NOT spawn `npm install` inside the npx-cache sdk dir'); + } finally { + cleanup(root); + } + }); + + test('missing dist in a dev clone: fail fast with clone build hint', () => { + const { root, sdkDir } = makeTempSdk({ npxCache: false }); + try { + fs.mkdirSync(path.join(root, '.git'), { recursive: true }); + const result = runWithIntercepts(() => { + installer.installSdkIfNeeded({ sdkDir }); + }); + assert.strictEqual(result.exitCode, 1); + // Dev clone path: suggest the local build, not the global upgrade. + assert.match(result.stderr, /cd sdk && npm install && npm run build/, + 'dev-clone error must keep the build-from-clone instructions'); + + const nestedInstall = result.spawnCalls.find((c) => { + const argv = Array.isArray(c.argv) ? c.argv : []; + const isNpm = /\bnpm(\.cmd)?$/i.test(String(c.cmd || '')); + const isInstall = argv.includes('install') || argv.includes('i'); + return isNpm && isInstall; + }); + assert.strictEqual(nestedInstall, undefined, + 'installer itself must never shell out to `npm install`; the user does that'); + } finally { + cleanup(root); + } + }); +});