diff --git a/extensions/taskplane/path-resolver.ts b/extensions/taskplane/path-resolver.ts index 3a651553..78f06064 100644 --- a/extensions/taskplane/path-resolver.ts +++ b/extensions/taskplane/path-resolver.ts @@ -112,32 +112,53 @@ const PI_PACKAGE_SCOPES = ["@earendil-works", "@mariozechner"] as const; * `dist/cli.js` so callers can spawn it with `node` directly, without a shell * intermediary. * - * Resolution order: the cross product of base directories × package scopes, - * with each base directory tried for the new scope before any base directory - * is tried for the legacy scope. (Equivalently: scope is the inner loop, base - * is the outer loop.) + * Resolution order: + * + * 0. **AUTHORITATIVE** — `process.argv[1]` when it points at a Pi `cli.js`. + * When Taskplane is running as a Pi extension, the parent process IS + * Pi, and Node sets `process.argv[1]` to the path of the file used to + * start it. This is the single most reliable resolution path: it works + * for npm-global, mise, asdf, NVM (Windows + Unix), Nix, Bun-installed + * Pi, and any future install method we can't enumerate. Issues #519 + * and #598 both stem from this signal being ignored in favor of a + * static-path search that misses non-canonical install layouts. + * + * If `process.argv[1]` isn't a Pi `cli.js` (e.g. running standalone in tests, + * or invoked through an indirect wrapper), the function falls through to a + * cross product of base directories × package scopes: * - * Base directories (outer loop): - * 1. `npm root -g` result (dynamic — covers all setups: nvm, Homebrew, volta, etc.) + * 1. `npm root -g` result (dynamic — covers npm-global, Homebrew, volta, etc.) * 2. `%APPDATA%\npm\node_modules\...` (Windows, APPDATA env var) * 3. `%USERPROFILE%\AppData\Roaming\npm\node_modules\...` (Windows, HOME-relative) * 4. `~/.npm-global/lib/node_modules/...` (macOS/Linux custom global prefix) - * 5. `/usr/local/lib/node_modules/...` (macOS system Node, Linux) - * 6. `/opt/homebrew/lib/node_modules/...` (macOS Homebrew) + * 5. `$NVM_SYMLINK\node_modules` (NVM-for-Windows, when the env var is set) + * 6. `dirname($NVM_BIN)/../lib/node_modules` (NVM-for-Unix, when the env var is set) + * 7. `/usr/local/lib/node_modules/...` (macOS system Node, Linux) + * 8. `/opt/homebrew/lib/node_modules/...` (macOS Homebrew) * * Scopes per base (inner loop): * a. `@earendil-works/pi-coding-agent/dist/cli.js` * b. `@mariozechner/pi-coding-agent/dist/cli.js` * - * @returns Absolute path to a Pi CLI `dist/cli.js` (under whichever scope was found). - * @throws {Error} If the CLI entrypoint cannot be found under any base × scope - * combination. The error message includes the `npm root -g` value - * AND lists both scopes searched, for operator diagnosis. + * @returns Absolute path to a Pi CLI `dist/cli.js`. + * @throws {Error} If the CLI entrypoint cannot be found by any strategy. + * The error message includes the `npm root -g` value AND lists + * both scopes searched, for operator diagnosis. */ export function resolvePiCliPath(): string { + // 0. AUTHORITATIVE: trust process.argv[1] when it points at a Pi cli.js. + // Pi's package.json declares `"bin": { "pi": "dist/cli.js" }`, so the + // `endsWith("cli.js")` guard is a tight sanity check that rejects e.g. + // test runners or wrapper scripts that happen to leave argv[1] pointing + // somewhere else. existsSync() guards against stale argv state in mocks. + const piEntry = process.argv[1] || ""; + if (piEntry.endsWith("cli.js") && existsSync(piEntry)) { + return piEntry; + } + const bases: string[] = []; - // 1. Dynamic: npm root -g (covers nvm, Homebrew, volta, custom npm prefix, etc.) + // 1. Dynamic: npm root -g (covers npm-global, Homebrew, volta, custom npm prefix, etc.) const npmRoot = getNpmGlobalRoot(); if (npmRoot) bases.push(npmRoot); @@ -151,9 +172,25 @@ export function resolvePiCliPath(): string { // 4. macOS/Linux custom global prefix bases.push(join(home, ".npm-global", "lib", "node_modules")); } - // 5. macOS system Node / Linux + + // 5. NVM-for-Windows defense in depth: NVM_SYMLINK points at the active + // Node install (typically C:\Program Files\nodejs as a junction), and the + // global packages live under \node_modules. Child processes + // inherit this env var even when PATH is stripped of npm. + if (process.env.NVM_SYMLINK) { + bases.push(join(process.env.NVM_SYMLINK, "node_modules")); + } + + // 6. NVM-for-Unix defense in depth: NVM_BIN points at the active version's + // bin directory, and the corresponding node_modules sit alongside it at + // `../lib/node_modules`. Same inheritance properties as NVM_SYMLINK. + if (process.env.NVM_BIN) { + bases.push(join(process.env.NVM_BIN, "..", "lib", "node_modules")); + } + + // 7. macOS system Node / Linux bases.push(join("/usr", "local", "lib", "node_modules")); - // 6. macOS Homebrew + // 8. macOS Homebrew bases.push(join("/opt", "homebrew", "lib", "node_modules")); // Cross product: scope is the inner loop so a single base directory is diff --git a/extensions/tests/path-resolver-pi-scope.test.ts b/extensions/tests/path-resolver-pi-scope.test.ts index 88286e38..1ef9ae67 100644 --- a/extensions/tests/path-resolver-pi-scope.test.ts +++ b/extensions/tests/path-resolver-pi-scope.test.ts @@ -82,6 +82,11 @@ function makeNpmRootWithScopes(scopes: ReadonlyArray<"@earendil-works" | "@mario * Run a child Node process that imports `path-resolver.ts` with the given * `npm_config_prefix` redirecting `npm root -g`. Returns the resolved path * or throws (capturing stderr) so test assertions can match either outcome. + * + * Optional `overrides` parameter lets a test mock the various signals the + * resolver consults (`process.argv[1]`, `NVM_SYMLINK`, `NVM_BIN`, etc.) + * without contaminating the parent process. The mocks are applied before + * the dynamic import so the resolver sees them on its first read. */ // TP-195: `stderr?: undefined` on success branch makes the discriminated // union narrowable under `strict: false` (the codebase-wide convention @@ -89,8 +94,22 @@ function makeNpmRootWithScopes(scopes: ReadonlyArray<"@earendil-works" | "@mario // and persistence.ts:ReconstructResult). function probeResolveInChild( npmConfigPrefix: string | null, + overrides?: { + mockArgv1?: string | null; + nvmSymlink?: string | null; + nvmBin?: string | null; + clearStaticFallbacks?: boolean; + }, ): { ok: true; resolved: string; stderr?: undefined } | { ok: false; stderr: string } { + const mockArgv1 = overrides?.mockArgv1 ?? null; + const nvmSymlink = overrides?.nvmSymlink ?? null; + const nvmBin = overrides?.nvmBin ?? null; + const clearStaticFallbacks = overrides?.clearStaticFallbacks ?? false; + + const argv1Mutation = mockArgv1 !== null ? `process.argv[1] = ${JSON.stringify(mockArgv1)};` : ""; + const probeScript = ` + ${argv1Mutation} import("${pathToFileUrl(join(repoRoot, "taskplane", "path-resolver.ts"))}").then((m) => { try { const resolved = m.resolvePiCliPath(); @@ -108,6 +127,21 @@ function probeResolveInChild( delete env.NPM_CONFIG_PREFIX; env.NPM_CONFIG_PREFIX = npmConfigPrefix; } + if (nvmSymlink !== null) env.NVM_SYMLINK = nvmSymlink; + else delete env.NVM_SYMLINK; + if (nvmBin !== null) env.NVM_BIN = nvmBin; + else delete env.NVM_BIN; + + // Strip env vars that would let static fallback paths resolve from the + // host machine — critical for tests that need to isolate a specific + // resolution mechanism. Without this, a dev machine with Pi installed at + // /usr/local/lib/node_modules would mask a bug in the new code paths. + if (clearStaticFallbacks) { + delete env.APPDATA; + delete env.USERPROFILE; + delete env.HOME; + } + try { const out = execFileSync( process.execPath, @@ -187,6 +221,198 @@ describe("resolvePiCliPath — Pi scope rename (#560)", () => { } }); + // ── #598 / #519 regression: process.argv[1] as the authoritative source ── + // Pre-fix, resolvePiCliPath ignored process.argv[1] entirely and relied on + // `npm root -g` plus static fallback paths. That stranded NVM-on-Windows + // users (npm not on child PATH) and any non-canonical Pi install location. + // The fix returns process.argv[1] directly when it points at a Pi cli.js, + // since the parent process loading Taskplane as an extension IS Pi. + + it("8.8 (#598): resolves directly via process.argv[1] when it points at a real cli.js", () => { + // Create a fake cli.js OUTSIDE any npm-root layout, so the only way the + // resolver can find it is via process.argv[1]. + const tmpDir = mkdtempSync(join(tmpdir(), "tp598-argv1-")); + try { + const fakeCli = join(tmpDir, "dist", "cli.js"); + mkdirSync(dirname(fakeCli), { recursive: true }); + writeFileSync(fakeCli, "// fake pi cli for #598 regression test\n", "utf-8"); + + const result = probeResolveInChild(null, { + mockArgv1: fakeCli, + clearStaticFallbacks: true, + }); + assert.ok(result.ok, `expected resolution to succeed, got: ${result.ok ? "OK" : result.stderr}`); + if (result.ok) { + assert.strictEqual( + result.resolved, + fakeCli, + "process.argv[1] should be returned verbatim when it ends in cli.js and exists", + ); + } + } finally { + rmSync(tmpDir, { recursive: true, force: true }); + } + }); + + it("8.9 (#598): process.argv[1] takes precedence over npm-root-installed Pi", () => { + // Set up BOTH: a Pi install in a temp npm root AND a Pi cli.js pointed + // at by process.argv[1]. The authoritative argv[1] entry must win. + const npmTmp = mkdtempSync(join(tmpdir(), "tp598-npm-")); + const argvTmp = mkdtempSync(join(tmpdir(), "tp598-argv-")); + try { + // Pi installed in the fake npm root + const posixRoot = join(npmTmp, "lib", "node_modules"); + const npmCli = join(posixRoot, "@earendil-works", "pi-coding-agent", "dist", "cli.js"); + mkdirSync(dirname(npmCli), { recursive: true }); + writeFileSync(npmCli, "// npm-root pi\n", "utf-8"); + // Pi cli.js pointed at by argv[1] — totally outside npm conventions + const argvCli = join(argvTmp, "alternate-install", "dist", "cli.js"); + mkdirSync(dirname(argvCli), { recursive: true }); + writeFileSync(argvCli, "// argv[1] pi\n", "utf-8"); + + const result = probeResolveInChild(npmTmp, { mockArgv1: argvCli }); + assert.ok(result.ok, `expected resolution to succeed, got: ${result.ok ? "OK" : result.stderr}`); + if (result.ok) { + assert.strictEqual( + result.resolved, + argvCli, + "argv[1] should win over npm-root resolution when both are present", + ); + } + } finally { + rmSync(npmTmp, { recursive: true, force: true }); + rmSync(argvTmp, { recursive: true, force: true }); + } + }); + + it("8.10 (#598): falls through when process.argv[1] does NOT end in cli.js", () => { + // Sanity guard: if argv[1] points at e.g. a test runner or wrapper, we + // must NOT mistake it for Pi. Fall through to the standard search. + const { tmpDir, cleanup } = makeNpmRootWithScopes(["@earendil-works"]); + const argvTmp = mkdtempSync(join(tmpdir(), "tp598-wrong-name-")); + try { + // Create a real file at argv[1] but with the wrong name + const wrongName = join(argvTmp, "runner.mjs"); + writeFileSync(wrongName, "// not pi\n", "utf-8"); + + const result = probeResolveInChild(tmpDir, { mockArgv1: wrongName }); + assert.ok( + result.ok, + `expected resolution to succeed via fallback, got: ${result.ok ? "OK" : result.stderr}`, + ); + if (result.ok) { + assert.match( + result.resolved, + /[\\/]@earendil-works[\\/]pi-coding-agent[\\/]dist[\\/]cli\.js$/, + "resolver must fall through to npm-root search when argv[1] fails the cli.js guard", + ); + } + } finally { + cleanup(); + rmSync(argvTmp, { recursive: true, force: true }); + } + }); + + it("8.11 (#598): falls through when process.argv[1] ends in cli.js but doesn't exist", () => { + // Edge case: argv[1] might be stale or mocked to a non-existent path + // (e.g. in test harnesses). Must not crash; must fall through. + const { tmpDir, cleanup } = makeNpmRootWithScopes(["@earendil-works"]); + try { + const phantomCli = join(tmpdir(), "phantom-tp598-does-not-exist", "dist", "cli.js"); + const result = probeResolveInChild(tmpDir, { mockArgv1: phantomCli }); + assert.ok( + result.ok, + `expected resolution to succeed via fallback, got: ${result.ok ? "OK" : result.stderr}`, + ); + if (result.ok) { + assert.match( + result.resolved, + /[\\/]@earendil-works[\\/]pi-coding-agent[\\/]dist[\\/]cli\.js$/, + "resolver must fall through to npm-root search when argv[1] file does not exist", + ); + } + } finally { + cleanup(); + } + }); + + // ── #598 defense in depth: NVM env-var fallbacks ───────────────────── + // galiling's report: Windows 11 + NVM means child processes get a stripped + // PATH where `npm` isn't found, so `npm root -g` returns empty. NVM-for- + // Windows sets NVM_SYMLINK pointing at the active Node install — children + // inherit it. NVM-for-Unix sets NVM_BIN with the same property. + + it("8.12 (#598): resolves under $NVM_SYMLINK/node_modules (NVM-for-Windows fallback)", () => { + const tmpDir = mkdtempSync(join(tmpdir(), "tp598-nvmsymlink-")); + // Separate empty prefix so `npm root -g` resolves somewhere WITHOUT + // Pi installed — isolates the NVM_SYMLINK path from any real Pi the + // dev machine may have at the canonical npm-global location. + const isolatedPrefix = mkdtempSync(join(tmpdir(), "tp598-empty-prefix-")); + try { + const symlinkRoot = join(tmpDir, "v25.9.0"); + const cli = join( + symlinkRoot, + "node_modules", + "@earendil-works", + "pi-coding-agent", + "dist", + "cli.js", + ); + mkdirSync(dirname(cli), { recursive: true }); + writeFileSync(cli, "// fake pi under NVM_SYMLINK\n", "utf-8"); + + const result = probeResolveInChild(isolatedPrefix, { + nvmSymlink: symlinkRoot, + clearStaticFallbacks: true, + }); + assert.ok(result.ok, `expected resolution to succeed, got: ${result.ok ? "OK" : result.stderr}`); + if (result.ok) { + assert.strictEqual(result.resolved, cli); + } + } finally { + rmSync(tmpDir, { recursive: true, force: true }); + rmSync(isolatedPrefix, { recursive: true, force: true }); + } + }); + + it("8.13 (#598): resolves under dirname($NVM_BIN)/../lib/node_modules (NVM-for-Unix fallback)", () => { + const tmpDir = mkdtempSync(join(tmpdir(), "tp598-nvmbin-")); + const isolatedPrefix = mkdtempSync(join(tmpdir(), "tp598-empty-prefix-")); + try { + const versionDir = join(tmpDir, "versions", "node", "v20.10.0"); + const binDir = join(versionDir, "bin"); + const cli = join( + versionDir, + "lib", + "node_modules", + "@earendil-works", + "pi-coding-agent", + "dist", + "cli.js", + ); + mkdirSync(binDir, { recursive: true }); + mkdirSync(dirname(cli), { recursive: true }); + writeFileSync(cli, "// fake pi under NVM_BIN\n", "utf-8"); + + const result = probeResolveInChild(isolatedPrefix, { + nvmBin: binDir, + clearStaticFallbacks: true, + }); + assert.ok(result.ok, `expected resolution to succeed, got: ${result.ok ? "OK" : result.stderr}`); + if (result.ok) { + // Normalize for cross-platform path comparison: NVM_BIN/../lib/... + // resolves to the lib path, which should match our created path. + assert.match( + result.resolved, + /[\\/]v20\.10\.0[\\/]lib[\\/]node_modules[\\/]@earendil-works[\\/]pi-coding-agent[\\/]dist[\\/]cli\.js$/, + ); + } + } finally { + rmSync(tmpDir, { recursive: true, force: true }); + rmSync(isolatedPrefix, { recursive: true, force: true }); + } + }); + it("8.7 (#560): error message names BOTH scopes when neither is found", () => { // Empty npm root \u2014 no Pi installed under any scope. const tmpDir = mkdtempSync(join(tmpdir(), "tp560-empty-"));