fix(#2620): detect HOME-relative PATH entries before suggesting absolute export (#2625)

* fix(#2620): detect HOME-relative PATH entries before suggesting absolute export When the installer reported `gsd-sdk` not on PATH and suggested appending an absolute `export PATH="/home/user/.npm-global/bin:$PATH"` line to the user's rc file, a user who had the equivalent `export PATH="$HOME/.npm-global/bin:$PATH"` already in their shell profile would get a duplicate entry — the installer only compared the absolute form. Add `homePathCoveredByRc(globalBin, homeDir, rcFileNames?)` to `bin/install.js` and export it for test-mode callers. The helper scans `~/.zshrc`, `~/.bashrc`, `~/.bash_profile`, `~/.profile`, grepping each file for `export PATH=` / bare `PATH=` lines and substituting the common HOME forms (\$HOME, \${HOME}, leading ~/) with the real home directory before comparing each resolved PATH segment against globalBin. Trailing slashes are normalised so `.npm-global/bin/` matches `.npm-global/bin`. Missing / unreadable / malformed rc files are swallowed — the caller falls back to the existing absolute suggestion. Tests cover $HOME, \${HOME}, and ~/ forms, absolute match, trailing-slash match, commented-out lines, missing rc files, and unreadable rc files (directory where a file is expected). Closes #2620 * fix(#2620): skip relative PATH segments in homePathCoveredByRc CodeRabbit flagged that the helper unconditionally resolved every non-$-containing segment against homeAbs via path.resolve(homeAbs, …), which silently turns a bare relative segment like `bin` or `node_modules/.bin` into `$HOME/bin` / `$HOME/node_modules/.bin`. That is wrong: bare PATH segments depend on the shell's cwd at lookup time, not on $HOME — so the helper was returning true for rc files that do not actually cover globalBin. Guard the compare with path.isAbsolute(expanded) after HOME expansion. Only segments that are absolute on their own (or that became absolute via $HOME / \${HOME} / ~ substitution) are compared against targetAbs. Relative segments are skipped. Add two regression tests covering a bare `bin` segment and a nested `node_modules/.bin` segment; both previously returned true when home happened to contain a matching subdirectory and now correctly return false. Closes #2620 (CodeRabbit follow-up) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(#2620): wire homePathCoveredByRc into installer suggestion path CodeRabbit flagged that homePathCoveredByRc was added in the previous commit but never called from the installer, so the user-facing PATH warning stayed unchanged — users with `export PATH="$HOME/.npm-global/bin:$PATH"` in their rc would still get a duplicate absolute-path suggestion. Add `maybeSuggestPathExport(globalBin, homeDir)` that: - skips silently when globalBin is already on process.env.PATH; - prints a "try reopening your shell" diagnostic when homePathCoveredByRc returns true (the directory IS on PATH via an rc entry — just not in the current shell); - otherwise falls through to the absolute-path `echo 'export PATH="…:$PATH"' >> ~/.zshrc` suggestion. Call it from installSdkIfNeeded after the sdk/dist check succeeds, resolving globalBin via `npm prefix -g` (plus `/bin` on POSIX). Swallow any exec failure so the installer keeps working when npm is weird. Export maybeSuggestPathExport for tests. Add three new regression tests (installer-flow coverage per CodeRabbit nitpick): - rc covers globalBin via $HOME form → no absolute suggestion emitted - rc covers only an unrelated directory → absolute suggestion emitted - globalBin already on process.env.PATH → no output at all Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-23 11:53:51 -04:00
parent 74da61fb4a
commit 807db75d55
2 changed files with 447 additions and 0 deletions
--- 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 {

--- a/tests/install-path-detection.test.cjs
+++ 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);
+    }
+  });
+});