fix(#598,#519): use process.argv[1] + NVM env vars to resolve Pi CLI

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 <symlink>\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

extensions/tests/path-resolver-pi-scope.test.ts

@@ -82,15 +82,34 @@ 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
 // applied here for the same reason as engine.ts:processSegmentExpansion
 // 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-"));