diff --git a/bin/install.js b/bin/install.js index 3a787e2ca..7b0ddfbf5 100755 --- a/bin/install.js +++ b/bin/install.js @@ -6797,6 +6797,144 @@ function promptLocation(runtimes) { }); } +/** + * Check whether any common shell rc file already contains a `PATH=` line + * whose HOME-expanded value places `globalBin` on PATH (#2620). + * + * Parses `~/.zshrc`, `~/.bashrc`, `~/.bash_profile`, `~/.profile` (or the + * override list in `rcFileNames`), matches `export PATH=` / bare `PATH=` + * lines, and substitutes the common HOME forms (`$HOME`, `${HOME}`, `~`) + * with `homeDir` before comparing each PATH segment against `globalBin`. + * + * Best-effort: any unreadable / malformed / non-existent rc file is ignored + * and the fallback is the caller's existing absolute-path suggestion. Only + * the `$HOME/…`, `${HOME}/…`, and `~/…` forms are handled — we do not try + * to fully parse bash syntax. + * + * @param {string} globalBin Absolute path to npm's global bin directory. + * @param {string} homeDir Absolute path used to substitute HOME / ~. + * @param {string[]} [rcFileNames] Override the default rc file list. + * @returns {boolean} true iff any rc file adds globalBin to PATH. + */ +function homePathCoveredByRc(globalBin, homeDir, rcFileNames) { + if (!globalBin || !homeDir) return false; + const path = require('path'); + const fs = require('fs'); + + const normalise = (p) => { + if (!p) return ''; + let n = p.replace(/[\\/]+$/g, ''); + if (n === '') n = p.startsWith('/') ? '/' : p; + return n; + }; + + const targetAbs = normalise(path.resolve(globalBin)); + const homeAbs = path.resolve(homeDir); + const files = rcFileNames || ['.zshrc', '.bashrc', '.bash_profile', '.profile']; + + const expandHome = (segment) => { + let s = segment; + s = s.replace(/\$\{HOME\}/g, homeAbs); + s = s.replace(/\$HOME/g, homeAbs); + if (s.startsWith('~/') || s === '~') { + s = s === '~' ? homeAbs : path.join(homeAbs, s.slice(2)); + } + return s; + }; + + // Match `PATH=…` (optionally prefixed with `export `). The RHS captures + // through end-of-line; surrounding quotes are stripped before splitting. + const assignRe = /^\s*(?:export\s+)?PATH\s*=\s*(.+?)\s*$/; + + for (const name of files) { + const rcPath = path.join(homeAbs, name); + let content; + try { + content = fs.readFileSync(rcPath, 'utf8'); + } catch { + continue; + } + + for (const rawLine of content.split(/\r?\n/)) { + const line = rawLine.replace(/^\s+/, ''); + if (line.startsWith('#')) continue; + + const m = assignRe.exec(rawLine); + if (!m) continue; + + let rhs = m[1]; + if ((rhs.startsWith('"') && rhs.endsWith('"')) || + (rhs.startsWith("'") && rhs.endsWith("'"))) { + rhs = rhs.slice(1, -1); + } + + for (const segment of rhs.split(':')) { + if (!segment) continue; + const trimmed = segment.trim(); + const expanded = expandHome(trimmed); + if (expanded.includes('$')) continue; + // Skip segments that are still relative after HOME expansion. A bare + // `bin` entry (or `./bin`, `node_modules/.bin`, etc.) depends on the + // shell's cwd at lookup time — it is NOT equivalent to `$HOME/bin`, + // so resolving against homeAbs would produce false positives. + if (!path.isAbsolute(expanded)) continue; + try { + const abs = normalise(path.resolve(expanded)); + if (abs === targetAbs) return true; + } catch { + // ignore unresolvable segments + } + } + } + } + + return false; +} + +/** + * Emit a PATH-export suggestion if globalBin is not already on PATH AND + * the user's shell rc files do not already cover it via a HOME-relative + * entry (#2620). + * + * Prints one of: + * - nothing, if `globalBin` is already present on `process.env.PATH` + * - a diagnostic "already covered via rc file" note, if an rc file has + * `export PATH="$HOME/…/bin:$PATH"` (or equivalent) and the user just + * needs to reopen their shell + * - the absolute `echo 'export PATH="…:$PATH"' >> ~/.zshrc` suggestion, + * if neither PATH nor any rc file covers globalBin + * + * Exported for tests; the installer calls this from finishInstall. + * + * @param {string} globalBin Absolute path to npm's global bin directory. + * @param {string} homeDir Absolute HOME path. + */ +function maybeSuggestPathExport(globalBin, homeDir) { + if (!globalBin || !homeDir) return; + const path = require('path'); + + const pathEnv = process.env.PATH || ''; + const targetAbs = path.resolve(globalBin).replace(/[\\/]+$/g, '') || globalBin; + const onPath = pathEnv.split(path.delimiter).some((seg) => { + if (!seg) return false; + const abs = path.resolve(seg).replace(/[\\/]+$/g, '') || seg; + return abs === targetAbs; + }); + if (onPath) return; + + if (homePathCoveredByRc(globalBin, homeDir)) { + console.log(` ${yellow}⚠${reset} ${bold}gsd-sdk${reset}'s directory is already on your PATH via an rc file entry — try reopening your shell (or ${cyan}source ~/.zshrc${reset}).`); + return; + } + + console.log(''); + console.log(` ${yellow}⚠${reset} ${bold}${globalBin}${reset} is not on your PATH.`); + console.log(` Add it with one of:`); + console.log(` ${cyan}echo 'export PATH="${globalBin}:$PATH"' >> ~/.zshrc${reset}`); + console.log(` ${cyan}echo 'export PATH="${globalBin}:$PATH"' >> ~/.bashrc${reset}`); + console.log(''); +} + /** * Verify the prebuilt SDK dist is present and the gsd-sdk shim is wired up. * @@ -6855,6 +6993,21 @@ function installSdkIfNeeded() { } console.log(` ${green}✓${reset} GSD SDK ready (sdk/dist/cli.js)`); + + // #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(); + 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'); + maybeSuggestPathExport(globalBin, os.homedir()); + } + } catch { + // npm not available / exec failed — silently skip the PATH advice. + } } /** @@ -6972,6 +7125,8 @@ if (process.env.GSD_TEST_MODE) { preserveUserArtifacts, restoreUserArtifacts, finishInstall, + homePathCoveredByRc, + maybeSuggestPathExport, }; } else { diff --git a/tests/install-path-detection.test.cjs b/tests/install-path-detection.test.cjs new file mode 100644 index 000000000..00309745c --- /dev/null +++ b/tests/install-path-detection.test.cjs @@ -0,0 +1,292 @@ +/** + * Regression test for #2620 — installer should not suggest adding an absolute + * PATH export when the user's rc file already contains a HOME-relative entry + * that covers the same directory. + * + * Covers `homePathCoveredByRc(globalBin, homeDir, rcFileNames?)` which parses + * each rc file's `export PATH=` lines, substitutes `$HOME` / `${HOME}` / `~`, + * and returns true when any resolved PATH entry equals globalBin. + */ + +'use strict'; + +const { test, describe, before, after } = 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 createTempHome() { + return fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-home-')); +} + +function cleanup(dir) { + fs.rmSync(dir, { recursive: true, force: true }); +} + +describe('installer HOME-relative PATH detection (#2620)', () => { + let installer; + before(() => { + installer = loadInstaller(); + }); + + test('homePathCoveredByRc is exported', () => { + assert.strictEqual( + typeof installer.homePathCoveredByRc, + 'function', + 'bin/install.js must export homePathCoveredByRc for #2620', + ); + }); + + test('detects $HOME/.npm-global/bin pattern', () => { + const home = createTempHome(); + try { + fs.writeFileSync( + path.join(home, '.zshrc'), + 'export PATH="$HOME/.npm-global/bin:$PATH"\n', + ); + const globalBin = path.join(home, '.npm-global', 'bin'); + assert.strictEqual(installer.homePathCoveredByRc(globalBin, home), true); + } finally { + cleanup(home); + } + }); + + test('detects ${HOME}/.npm-global/bin pattern', () => { + const home = createTempHome(); + try { + fs.writeFileSync( + path.join(home, '.bashrc'), + 'export PATH="${HOME}/.npm-global/bin:$PATH"\n', + ); + const globalBin = path.join(home, '.npm-global', 'bin'); + assert.strictEqual(installer.homePathCoveredByRc(globalBin, home), true); + } finally { + cleanup(home); + } + }); + + test('detects ~/.npm-global/bin tilde form', () => { + const home = createTempHome(); + try { + fs.writeFileSync( + path.join(home, '.profile'), + 'export PATH=~/.npm-global/bin:$PATH\n', + ); + const globalBin = path.join(home, '.npm-global', 'bin'); + assert.strictEqual(installer.homePathCoveredByRc(globalBin, home), true); + } finally { + cleanup(home); + } + }); + + test('detects absolute path that exactly matches globalBin', () => { + const home = createTempHome(); + try { + const globalBin = path.join(home, '.npm-global', 'bin'); + fs.writeFileSync( + path.join(home, '.zshrc'), + `export PATH="${globalBin}:$PATH"\n`, + ); + assert.strictEqual(installer.homePathCoveredByRc(globalBin, home), true); + } finally { + cleanup(home); + } + }); + + test('returns false when rc files exist but do not cover globalBin', () => { + const home = createTempHome(); + try { + fs.writeFileSync( + path.join(home, '.zshrc'), + 'export PATH="$HOME/.cargo/bin:$PATH"\nexport FOO=bar\n', + ); + const globalBin = path.join(home, '.npm-global', 'bin'); + assert.strictEqual(installer.homePathCoveredByRc(globalBin, home), false); + } finally { + cleanup(home); + } + }); + + test('returns false when no rc files exist', () => { + const home = createTempHome(); + try { + const globalBin = path.join(home, '.npm-global', 'bin'); + assert.strictEqual(installer.homePathCoveredByRc(globalBin, home), false); + } finally { + cleanup(home); + } + }); + + test('swallows unreadable rc files without throwing', () => { + const home = createTempHome(); + try { + const rc = path.join(home, '.zshrc'); + fs.mkdirSync(rc); // directory where a file is expected — reading throws + const globalBin = path.join(home, '.npm-global', 'bin'); + assert.doesNotThrow(() => installer.homePathCoveredByRc(globalBin, home)); + assert.strictEqual(installer.homePathCoveredByRc(globalBin, home), false); + } finally { + cleanup(home); + } + }); + + test('ignores commented-out export PATH lines', () => { + const home = createTempHome(); + try { + fs.writeFileSync( + path.join(home, '.zshrc'), + '# export PATH="$HOME/.npm-global/bin:$PATH"\n', + ); + const globalBin = path.join(home, '.npm-global', 'bin'); + assert.strictEqual(installer.homePathCoveredByRc(globalBin, home), false); + } finally { + cleanup(home); + } + }); + + test('matches globalBin regardless of trailing slash', () => { + const home = createTempHome(); + try { + fs.writeFileSync( + path.join(home, '.zshrc'), + 'export PATH="$HOME/.npm-global/bin/:$PATH"\n', + ); + const globalBin = path.join(home, '.npm-global', 'bin'); + assert.strictEqual(installer.homePathCoveredByRc(globalBin, home), true); + } finally { + cleanup(home); + } + }); + + // CodeRabbit finding: bare relative PATH segments (e.g. `bin`) must not be + // resolved against $HOME. Relative segments depend on the shell's cwd at + // lookup time and are unrelated to $HOME/bin. + test('does not treat bare relative PATH segment as HOME-relative', () => { + const home = createTempHome(); + try { + fs.writeFileSync( + path.join(home, '.zshrc'), + 'export PATH="bin:$PATH"\n', + ); + const globalBin = path.join(home, 'bin'); + assert.strictEqual( + installer.homePathCoveredByRc(globalBin, home), + false, + 'relative PATH segments must not be resolved against $HOME', + ); + } finally { + cleanup(home); + } + }); + + test('does not treat nested relative PATH segment as HOME-relative', () => { + const home = createTempHome(); + try { + fs.writeFileSync( + path.join(home, '.zshrc'), + 'export PATH="node_modules/.bin:$PATH"\n', + ); + const globalBin = path.join(home, 'node_modules', '.bin'); + assert.strictEqual( + installer.homePathCoveredByRc(globalBin, home), + false, + ); + } finally { + cleanup(home); + } + }); + + // CodeRabbit actionable 1 + nitpick: the installer's PATH-export + // suggestion banner must be suppressed when an rc file already covers + // globalBin via a HOME-relative entry. + test('maybeSuggestPathExport suppresses suggestion when rc covers globalBin', () => { + const home = createTempHome(); + try { + const globalBin = path.join(home, '.npm-global', 'bin'); + fs.mkdirSync(globalBin, { recursive: true }); + fs.writeFileSync( + path.join(home, '.zshrc'), + 'export PATH="$HOME/.npm-global/bin:$PATH"\n', + ); + + const logs = []; + const origLog = console.log; + console.log = (...args) => { logs.push(args.join(' ')); }; + try { + installer.maybeSuggestPathExport(globalBin, home); + } finally { + console.log = origLog; + } + + const joined = logs.join('\n'); + assert.ok( + !/echo 'export PATH=/.test(joined), + `installer should not emit absolute export suggestion; got:\n${joined}`, + ); + } finally { + cleanup(home); + } + }); + + test('maybeSuggestPathExport emits suggestion when rc does not cover globalBin', () => { + const home = createTempHome(); + try { + const globalBin = path.join(home, '.npm-global', 'bin'); + fs.mkdirSync(globalBin, { recursive: true }); + fs.writeFileSync( + path.join(home, '.zshrc'), + 'export PATH="$HOME/.cargo/bin:$PATH"\n', + ); + + const logs = []; + const origLog = console.log; + console.log = (...args) => { logs.push(args.join(' ')); }; + try { + installer.maybeSuggestPathExport(globalBin, home); + } finally { + console.log = origLog; + } + + const joined = logs.join('\n'); + assert.ok( + /echo 'export PATH=/.test(joined), + `installer should emit absolute export suggestion when rc does not cover globalBin; got:\n${joined}`, + ); + } finally { + cleanup(home); + } + }); + + test('maybeSuggestPathExport is a no-op when globalBin already on process.env.PATH', () => { + const home = createTempHome(); + const origPath = process.env.PATH; + try { + const globalBin = path.join(home, '.npm-global', 'bin'); + fs.mkdirSync(globalBin, { recursive: true }); + process.env.PATH = `${globalBin}${path.delimiter}${origPath || ''}`; + + const logs = []; + const origLog = console.log; + console.log = (...args) => { logs.push(args.join(' ')); }; + try { + installer.maybeSuggestPathExport(globalBin, home); + } finally { + console.log = origLog; + } + + assert.strictEqual(logs.length, 0, `expected no output when on PATH; got:\n${logs.join('\n')}`); + } finally { + if (origPath === undefined) delete process.env.PATH; else process.env.PATH = origPath; + cleanup(home); + } + }); +});