diff --git a/bin/gsd-sdk.js b/bin/gsd-sdk.js index cdbfad855..ffc0aa1f2 100755 --- a/bin/gsd-sdk.js +++ b/bin/gsd-sdk.js @@ -2,11 +2,16 @@ /** * bin/gsd-sdk.js — back-compat shim for external callers of `gsd-sdk`. * - * When the parent package is installed globally (`npm install -g get-shit-done-cc` - * or `npx get-shit-done-cc`), npm creates a `gsd-sdk` symlink in the global bin - * directory pointing at this file. npm correctly chmods bin entries from a tarball, - * so the execute-bit problem that afflicted the sub-install approach (issue #2453) - * cannot occur here. + * When the parent package is installed globally (`npm install -g get-shit-done-cc`) + * npm creates a `gsd-sdk` symlink in the global bin directory pointing at this + * file. npm correctly chmods bin entries from a tarball, so the execute-bit + * problem that afflicted the sub-install approach (issue #2453) cannot occur here. + * + * NOTE (#2775): `npx get-shit-done-cc` does NOT link this shim — npx only + * exposes the package's primary bin (`get-shit-done-cc`). For npx-based usage, + * the installer (`bin/install.js#installSdkIfNeeded`) self-symlinks `gsd-sdk` + * into `~/.local/bin` when needed and verifies PATH callability before + * reporting `✓ GSD SDK ready`. * * This shim resolves sdk/dist/cli.js relative to its own location and delegates * to it via `node`, so `gsd-sdk ` behaves identically to diff --git a/bin/install.js b/bin/install.js index 48ce47167..3757033fa 100755 --- a/bin/install.js +++ b/bin/install.js @@ -7427,14 +7427,49 @@ function installSdkIfNeeded(opts) { // `node sdkCliPath` invocation in bin/gsd-sdk.js. } - console.log(` ${green}✓${reset} GSD SDK ready (sdk/dist/cli.js)`); + // #2775: do not assert "GSD SDK ready" until `gsd-sdk` actually resolves on + // PATH. `npx get-shit-done-cc` only links the package's primary bin; the + // secondary `gsd-sdk` shim is left dangling under the npx cache and is NOT + // callable as a bare command. The previous file-presence-only check was a + // strictly weaker invariant than the one workflows depend on + // (`command -v gsd-sdk` resolving), and led to a false ✓ in npx-cache + // installs (issue #2775). + const shimSrc = path.resolve(__dirname, 'gsd-sdk.js'); + let onPath = isGsdSdkOnPath(); + + if (!onPath) { + // Try to materialize the shim into a user-writable PATH location so the + // installer can deliver on the success message without requiring the user + // to run `npm install -g` separately. Picks the first PATH entry that + // looks like a user-owned bin dir; falls back to ~/.local/bin even if + // it's not on PATH (then a follow-up suggestion is printed). + const linked = trySelfLinkGsdSdk(shimSrc); + if (linked) { + onPath = isGsdSdkOnPath(); + if (onPath) { + console.log(` ${dim}↪ linked gsd-sdk → ${linked}${reset}`); + } + } + } + + if (onPath) { + console.log(` ${green}✓${reset} GSD SDK ready (sdk/dist/cli.js)`); + } else { + console.log(''); + console.log(` ${yellow}⚠${reset} GSD SDK files are present but ${bold}gsd-sdk${reset} is not on your PATH.`); + console.log(` Workflows that call ${cyan}gsd-sdk query …${reset} will fail with "command not found".`); + console.log(` Install globally to materialize the bin symlink:`); + console.log(` ${cyan}npm install -g get-shit-done-cc${reset}`); + console.log(` Or add a directory containing the shim to your PATH manually.`); + console.log(''); + } // #2620: warn if npm's global bin is not on PATH, suppressing the // absolute-path suggestion when the user's rc already covers it via // a HOME-relative entry (e.g. `export PATH="$HOME/.npm-global/bin:$PATH"`). try { - const { execSync } = require('child_process'); - const npmPrefix = execSync('npm prefix -g', { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }).trim(); + const cp = require('child_process'); + const npmPrefix = cp.execSync('npm prefix -g', { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }).trim(); if (npmPrefix) { // On Windows npm prefix IS the bin dir; on POSIX it's `${prefix}/bin`. const globalBin = process.platform === 'win32' ? npmPrefix : path.join(npmPrefix, 'bin'); @@ -7445,6 +7480,105 @@ function installSdkIfNeeded(opts) { } } +/** + * #2775 helper: check whether a callable `gsd-sdk` exists on the current PATH. + * + * Pure PATH walk (no spawn) — we look for a regular file or symlink named + * `gsd-sdk` (or `gsd-sdk.cmd`/`.exe` on Windows) in any directory on PATH and + * verify it carries the execute bit on POSIX. Avoids paying spawn cost and + * avoids the chicken-and-egg of needing to run the not-yet-installed binary. + */ +function isGsdSdkOnPath() { + const path = require('path'); + const fs = require('fs'); + const pathEnv = process.env.PATH || ''; + const exts = process.platform === 'win32' ? ['.cmd', '.exe', '.bat', ''] : ['']; + for (const seg of pathEnv.split(path.delimiter)) { + if (!seg) continue; + for (const ext of exts) { + const candidate = path.join(seg, `gsd-sdk${ext}`); + try { + const st = fs.statSync(candidate); + if (st.isFile()) { + if (process.platform === 'win32') return true; + if ((st.mode & 0o111) !== 0) return true; + } + } catch { + // missing / EACCES on dir — keep scanning. + } + } + } + return false; +} + +/** + * #2775 helper: attempt to materialize the `gsd-sdk` shim at a user-writable + * PATH location. Returns the absolute path created on success, or null if no + * suitable location was usable. + * + * Strategy (POSIX): prefer ~/.local/bin (creating it if absent — many distros + * already have it on PATH via .profile). Fall back to the first PATH entry + * under HOME we can write to. Skip on Windows (npm install -g is the right + * primitive there; we don't try to fabricate a .cmd shim). + */ +function trySelfLinkGsdSdk(shimSrc) { + if (process.platform === 'win32') return null; + const path = require('path'); + const fs = require('fs'); + const home = os.homedir(); + if (!home) return null; + + const localBin = path.join(home, '.local', 'bin'); + const pathCandidates = []; + const pathEnv = process.env.PATH || ''; + for (const seg of pathEnv.split(path.delimiter)) { + if (!seg) continue; + const abs = path.resolve(seg); + if (abs.startsWith(home + path.sep) && !pathCandidates.includes(abs)) { + pathCandidates.push(abs); + } + } + // If ~/.local/bin is already on PATH, keep it first (preserves existing UX + // for the common case). Otherwise prefer PATH-backed HOME dirs first so we + // self-link somewhere actually on PATH, falling back to ~/.local/bin only + // when no on-PATH HOME dir is writable. (#2775 CodeRabbit follow-up) + const candidates = pathCandidates.includes(localBin) + ? [localBin, ...pathCandidates.filter((dir) => dir !== localBin)] + : [...pathCandidates, localBin]; + + for (const dir of candidates) { + try { + fs.mkdirSync(dir, { recursive: true }); + const target = path.join(dir, 'gsd-sdk'); + // Replace any existing entry — it may be stale (prior install of an + // older version pointing at a now-absent shim). + try { fs.unlinkSync(target); } catch {} + try { + fs.symlinkSync(shimSrc, target); + } catch { + // Filesystems that don't support symlinks (some FUSE mounts): write a + // tiny wrapper that `require()`s the real shim by absolute path. We + // cannot copyFileSync(shimSrc, target) — bin/gsd-sdk.js resolves the + // CLI via `path.resolve(__dirname, '..', 'sdk', 'dist', 'cli.js')`, + // and after a copy `__dirname` would be the link directory (e.g. + // ~/.local/bin), causing the resolved CLI path to be broken + // (~/.local/sdk/dist/cli.js). Wrapping via require() preserves + // __dirname resolution because the require runs against shimSrc's + // own location. (#2775 CodeRabbit follow-up) + fs.writeFileSync( + target, + `#!/usr/bin/env node\nrequire(${JSON.stringify(shimSrc)});\n`, + ); + try { fs.chmodSync(target, 0o755); } catch {} + } + return target; + } catch { + // permission / EROFS — try next candidate. + } + } + return null; +} + /** * Install GSD for all selected runtimes */ diff --git a/tests/bug-2775-sdk-shim-path-verify.test.cjs b/tests/bug-2775-sdk-shim-path-verify.test.cjs new file mode 100644 index 000000000..57eb33bce --- /dev/null +++ b/tests/bug-2775-sdk-shim-path-verify.test.cjs @@ -0,0 +1,233 @@ +/** + * Regression test for bug #2775 + * + * `npx get-shit-done-cc@latest --global` runs the installer, which prints + * `✓ GSD SDK ready` even though the secondary `gsd-sdk` bin is not on the + * user's PATH. Root cause: `npx` only links the package's primary bin into + * the ephemeral cache; secondary bins are not symlinked. The installer's + * `installSdkIfNeeded` only verified that `sdk/dist/cli.js` exists on disk + * — a strictly weaker invariant than `command -v gsd-sdk` resolving. + * + * The fix tightens the success gate: after confirming the dist is present, + * the installer must verify `gsd-sdk` resolves on PATH. If it does not, the + * installer attempts to materialize the shim into a user-writable PATH + * location (`~/.local/bin/gsd-sdk`) and re-checks. Only when the PATH probe + * succeeds does it print `✓ GSD SDK ready`. Otherwise it emits a clear + * warning + remediation and does NOT lie about readiness. + * + * This test exercises `installSdkIfNeeded` against a synthetic npx-cache + * shape: sdk/dist/cli.js present, but PATH does not contain any directory + * with a `gsd-sdk` shim. The legacy code printed the success line in this + * shape; the fixed code must not. + */ + +'use strict'; + +process.env.GSD_TEST_MODE = '1'; + +const { describe, test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const installModule = require('../bin/install.js'); +const { installSdkIfNeeded } = installModule; +const { createTempDir, cleanup } = require('./helpers.cjs'); + +function captureConsole(fn) { + const stdout = []; + const stderr = []; + const origLog = console.log; + const origWarn = console.warn; + const origError = console.error; + console.log = (...a) => stdout.push(a.join(' ')); + console.warn = (...a) => stderr.push(a.join(' ')); + console.error = (...a) => stderr.push(a.join(' ')); + let threw = null; + try { + fn(); + } catch (e) { + threw = e; + } finally { + console.log = origLog; + console.warn = origWarn; + console.error = origError; + } + // Re-throw any captured exception AFTER restoring console so callers don't + // have to destructure-and-assert on `threw` (and a future regression that + // crashes before printing won't falsely pass `!hasReady`). (#2775 + // CodeRabbit follow-up) + if (threw) throw threw; + // strip ANSI for matching + const strip = (s) => s.replace(/\x1b\[[0-9;]*m/g, ''); + return { + stdout: stdout.map(strip).join('\n'), + stderr: stderr.map(strip).join('\n'), + }; +} + +describe('bug #2775: installSdkIfNeeded must verify gsd-sdk on PATH before reporting ready', () => { + let tmpRoot; + let sdkDir; + let pathDir; + let homeDir; + let savedEnv; + + beforeEach(() => { + tmpRoot = createTempDir('gsd-2775-'); + sdkDir = path.join(tmpRoot, 'sdk'); + fs.mkdirSync(path.join(sdkDir, 'dist'), { recursive: true }); + fs.writeFileSync( + path.join(sdkDir, 'dist', 'cli.js'), + '#!/usr/bin/env node\nconsole.log("0.0.0-test");\n', + { mode: 0o755 }, + ); + pathDir = path.join(tmpRoot, 'somebin'); + fs.mkdirSync(pathDir, { recursive: true }); + homeDir = path.join(tmpRoot, 'home'); + fs.mkdirSync(homeDir, { recursive: true }); + savedEnv = { PATH: process.env.PATH, HOME: process.env.HOME }; + // PATH does NOT contain anything with a gsd-sdk shim — simulates npx-cache. + process.env.PATH = pathDir; + process.env.HOME = homeDir; + }); + + afterEach(() => { + if (savedEnv.PATH == null) delete process.env.PATH; + else process.env.PATH = savedEnv.PATH; + if (savedEnv.HOME == null) delete process.env.HOME; + else process.env.HOME = savedEnv.HOME; + cleanup(tmpRoot); + }); + + test('does NOT print "GSD SDK ready" when gsd-sdk is not callable on PATH and cannot be linked', () => { + // Make ~/.local/bin not on PATH and not creatable-friendly: PATH stays + // as a single dir with no gsd-sdk. The installer may attempt to create + // ~/.local/bin/gsd-sdk, but that location isn't on PATH either, so the + // post-link probe should still fail and the success line must be withheld. + const { stdout, stderr } = captureConsole(() => { + installSdkIfNeeded({ sdkDir }); + }); + const combined = `${stdout}\n${stderr}`; + const hasReady = /GSD SDK ready/.test(combined); + const mentionsPath = /not on (your )?PATH|gsd-sdk.*PATH|PATH.*gsd-sdk/i.test(combined); + assert.ok( + !hasReady, + `installer must not print "GSD SDK ready" when gsd-sdk is not on PATH. Output:\n${combined}`, + ); + assert.ok( + mentionsPath, + `installer must surface a PATH-related warning when gsd-sdk is not callable. Output:\n${combined}`, + ); + }); + + test('DOES print "GSD SDK ready" after self-linking into a directory that IS on PATH', () => { + // Put ~/.local/bin on PATH; the installer should create the shim there + // and the post-link callability probe should succeed. + const localBin = path.join(homeDir, '.local', 'bin'); + fs.mkdirSync(localBin, { recursive: true }); + process.env.PATH = `${localBin}${path.delimiter}${pathDir}`; + + const { stdout, stderr } = captureConsole(() => { + installSdkIfNeeded({ sdkDir }); + }); + const combined = `${stdout}\n${stderr}`; + assert.ok( + /GSD SDK ready/.test(combined), + `installer must print "GSD SDK ready" after self-linking to a dir on PATH. Output:\n${combined}`, + ); + // And the link must actually exist + resolve back to the shim. + const linkPath = path.join(localBin, 'gsd-sdk'); + assert.ok(fs.existsSync(linkPath), `installer must materialize ${linkPath}`); + }); + + test('symlink-fallback writes a wrapper that require()s the real shim by absolute path (preserves __dirname)', () => { + // Simulate a symlink-hostile filesystem by forcing fs.symlinkSync to throw. + // The fallback must NOT copy bin/gsd-sdk.js into ~/.local/bin (which would + // break the shim's `path.resolve(__dirname, '..', 'sdk', 'dist', 'cli.js')` + // resolution). Instead it must write a tiny wrapper script that + // require()s the real shim by absolute path so __dirname stays correct. + const localBin = path.join(homeDir, '.local', 'bin'); + fs.mkdirSync(localBin, { recursive: true }); + process.env.PATH = `${localBin}${path.delimiter}${pathDir}`; + + const realShimSrc = path.resolve(__dirname, '..', 'bin', 'gsd-sdk.js'); + const origSymlink = fs.symlinkSync; + fs.symlinkSync = () => { + const err = new Error('EPERM: simulated symlink-hostile filesystem'); + err.code = 'EPERM'; + throw err; + }; + try { + captureConsole(() => { + installSdkIfNeeded({ sdkDir }); + }); + } finally { + fs.symlinkSync = origSymlink; + } + + const target = path.join(localBin, 'gsd-sdk'); + assert.ok(fs.existsSync(target), `fallback must materialize ${target}`); + // Critical: it must NOT be a verbatim copy of bin/gsd-sdk.js. + const targetContent = fs.readFileSync(target, 'utf8'); + const realShimContent = fs.readFileSync(realShimSrc, 'utf8'); + assert.notStrictEqual( + targetContent, + realShimContent, + 'fallback must not copyFileSync bin/gsd-sdk.js — that breaks __dirname-based CLI resolution', + ); + // It must be a wrapper that require()s the real shim by absolute path. + assert.ok( + targetContent.includes('require(') && targetContent.includes(realShimSrc), + `fallback wrapper must require() the real shim by absolute path. Got:\n${targetContent}`, + ); + // And it must be executable. + const st = fs.statSync(target); + assert.ok( + (st.mode & 0o111) !== 0, + `fallback wrapper must have execute bit set (mode=${st.mode.toString(8)})`, + ); + // (Earlier assertions on targetContent already verify the wrapper points + // at the real shim by absolute path, which is what guarantees __dirname + // resolves correctly. A separate "does /sdk/dist exist?" check would + // be tautological — that path is true regardless of what the wrapper + // wrote.) (#2775 CodeRabbit follow-up) + }); + + test('self-link prefers a PATH-backed HOME dir over ~/.local/bin when ~/.local/bin is off-PATH', () => { + // Regression for #2775 CodeRabbit follow-up: the candidate ordering must + // try PATH-backed HOME dirs FIRST, falling back to ~/.local/bin only when + // it's not on PATH. Otherwise we self-link to ~/.local/bin (off-PATH) and + // warn — when we could have linked to ~/bin (on-PATH) and printed success. + const homeBin = path.join(homeDir, 'bin'); + fs.mkdirSync(homeBin, { recursive: true }); + // PATH contains ~/bin (a HOME dir) but NOT ~/.local/bin. + process.env.PATH = `${homeBin}${path.delimiter}${pathDir}`; + + const { stdout, stderr } = captureConsole(() => { + installSdkIfNeeded({ sdkDir }); + }); + const combined = `${stdout}\n${stderr}`; + assert.ok( + /GSD SDK ready/.test(combined), + `installer must self-link into the on-PATH HOME dir and print success. Output:\n${combined}`, + ); + assert.ok( + fs.existsSync(path.join(homeBin, 'gsd-sdk')), + `installer must materialize the link in the on-PATH HOME dir (~/bin), not ~/.local/bin`, + ); + }); + + test('DOES print "GSD SDK ready" when gsd-sdk is already resolvable on PATH', () => { + // Pre-populate PATH with a `gsd-sdk` shim so the probe finds one. + const preexisting = path.join(pathDir, 'gsd-sdk'); + fs.writeFileSync(preexisting, '#!/bin/sh\nexit 0\n', { mode: 0o755 }); + const { stdout } = captureConsole(() => { + installSdkIfNeeded({ sdkDir }); + }); + assert.ok( + /GSD SDK ready/.test(stdout), + `installer must print "GSD SDK ready" when gsd-sdk is already on PATH. Output:\n${stdout}`, + ); + }); +});