From 6f0ce9e7d490f3d284109da7c66eb44ad8946055 Mon Sep 17 00:00:00 2001 From: Henry Lach Date: Wed, 17 Jun 2026 15:41:01 -0400 Subject: [PATCH] fix(#598,#519): use process.argv[1] + NVM env vars to resolve Pi CLI When Taskplane runs as a Pi extension, process.argv[1] IS the absolute path to Pi's cli.js \u2014 set by Node from the parent process invocation. Trusting it directly is the single most reliable Pi-resolution strategy: it works for npm-global, mise, asdf, NVM (Windows + Unix), Nix, Bun-installed Pi, and any future install method we can't enumerate. resolvePiCliPath previously ignored argv[1] entirely and relied on 'npm root -g' plus a small static fallback list. That stranded: * #598 (itguy327 + galiling): NVM-on-Windows users whose child processes inherit a stripped PATH \u2014 npm root -g returns empty and the fallback list doesn't include NVM-versioned dirs. * #519 (chenxin-yan): Nix users whose Pi lives at /nix/store/..., not under any npm prefix the resolver knew about. The fix adds process.argv[1] as candidate 0, with two sanity guards: endsWith('cli.js') (matches Pi's bin entrypoint per its package.json's "bin": { "pi": "dist/cli.js" }) AND existsSync() (handles stale state in tests or wrappers). Both guards must pass or the resolver falls through to the existing base-directory search. Defense in depth: two new env-var-driven bases for NVM environments where argv[1] is unavailable (e.g., test harnesses, indirect spawn): * NVM_SYMLINK\node_modules (NVM-for-Windows, points at active version) * dirname(NVM_BIN)/../lib/node_modules (NVM-for-Unix, same property) Both env vars are set by NVM and inherited by child processes even when PATH is stripped, so they survive the failure mode that defeated npm root -g for galiling. Tests (extensions/tests/path-resolver-pi-scope.test.ts): 8.8 \u2014 argv[1] -> real cli.js resolves directly without any npm root 8.9 \u2014 argv[1] takes precedence over npm-root-installed Pi (priority) 8.10 \u2014 falls through when argv[1] doesn't end in cli.js (false-positive guard against wrappers/test runners) 8.11 \u2014 falls through when argv[1] ends in cli.js but file is missing 8.12 \u2014 resolves under $NVM_SYMLINK/node_modules (Windows NVM) 8.13 \u2014 resolves under dirname($NVM_BIN)/../lib/node_modules (Unix NVM) The existing #560 regression tests (8.4\u20138.7) still pass unchanged, verifying back-compat for both @earendil-works and @mariozechner scopes and the both-scopes-named error message. Validation npm run typecheck pass npm run lint 286 warnings / 671 infos (identical to main) npm run format:check pass (1 biome format applied to new tests) path-resolver tests 10/10 pass (6 new for #598) Full test suite 3714/3715 pass, 1 skipped, zero new failures taskplane help / doctor pass Closes #598 Closes #519 Apologies to @chenxin-yan: PR #520 was closed on 2026-05-25 with the claim that the fix was already in main, but I had misread path-resolver.ts \u2014 the process.argv[1] usage at line 235 was in the sibling resolveTaskplanePackageFile function, NOT in resolvePiCliPath. The fix the contributor actually proposed (which is also what this commit lands) was never applied to the function it was meant for. Reopening #519. --- extensions/taskplane/path-resolver.ts | 67 ++++-- .../tests/path-resolver-pi-scope.test.ts | 226 ++++++++++++++++++ 2 files changed, 278 insertions(+), 15 deletions(-) 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-"));