From 6f931797cae24ad9ea392d220c1f7c7f4585b50e Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Sun, 17 May 2026 20:47:39 +0200 Subject: [PATCH] fix(coding-agent): install npm packages in managed root closes #4587 --- packages/coding-agent/CHANGELOG.md | 6 + packages/coding-agent/README.md | 2 +- packages/coding-agent/docs/packages.md | 4 +- packages/coding-agent/docs/settings.md | 4 +- .../coding-agent/src/core/package-manager.ts | 76 +++++---- .../coding-agent/test/package-manager.test.ts | 147 ++++++++++++++---- 6 files changed, 172 insertions(+), 67 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 97de3a6e..b1acdb37 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## [Unreleased] + +### Fixed + +- Fixed user-scoped npm pi packages to install under `~/.pi/agent/npm/` instead of npm's global package root, avoiding permission errors with system-managed Node installs ([#4587](https://github.com/earendil-works/pi/issues/4587)). + ## [0.74.1] - 2026-05-16 ### New Features diff --git a/packages/coding-agent/README.md b/packages/coding-agent/README.md index 44e1bce1..fedc53d2 100644 --- a/packages/coding-agent/README.md +++ b/packages/coding-agent/README.md @@ -406,7 +406,7 @@ pi update npm:@foo/pi-tools # update one package pi config # enable/disable extensions, skills, prompts, themes ``` -Packages install to `~/.pi/agent/git/` (git) or global npm. Use `-l` for project-local installs (`.pi/git/`, `.pi/npm/`). Git packages install dependencies with `npm install --omit=dev` by default, so runtime deps must be listed under `dependencies`; when `npmCommand` is configured, git packages use plain `install` for compatibility with wrappers. If you use a Node version manager and want package installs to reuse a stable npm context, set `npmCommand` in `settings.json`, for example `["mise", "exec", "node@20", "--", "npm"]`. +Packages install to `~/.pi/agent/git/` (git) or `~/.pi/agent/npm/` (npm). Use `-l` for project-local installs (`.pi/git/`, `.pi/npm/`). Git packages install dependencies with `npm install --omit=dev` by default, so runtime deps must be listed under `dependencies`; when `npmCommand` is configured, git packages use plain `install` for compatibility with wrappers. If you use a Node version manager and want package installs to reuse a stable npm context, set `npmCommand` in `settings.json`, for example `["mise", "exec", "node@20", "--", "npm"]`. Create a package by adding a `pi` key to `package.json`: diff --git a/packages/coding-agent/docs/packages.md b/packages/coding-agent/docs/packages.md index e0a2f2c2..575f0acc 100644 --- a/packages/coding-agent/docs/packages.md +++ b/packages/coding-agent/docs/packages.md @@ -36,7 +36,7 @@ pi update npm:@foo/bar # update one package pi update --extension npm:@foo/bar ``` -By default, `install` and `remove` write to global settings (`~/.pi/agent/settings.json`). Use `-l` to write to project settings (`.pi/settings.json`) instead. Project settings can be shared with your team, and pi installs any missing packages automatically on startup. +By default, `install` and `remove` write to user settings (`~/.pi/agent/settings.json`). Use `-l` to write to project settings (`.pi/settings.json`) instead. Project settings can be shared with your team, and pi installs any missing packages automatically on startup. To try a package without installing it, use `--extension` or `-e`. This installs to a temporary directory for the current run only: @@ -57,7 +57,7 @@ npm:pkg ``` - Versioned specs are pinned and skipped by package updates (`pi update`, `pi update --extensions`). -- Global installs use `npm install -g`. +- User installs go under `~/.pi/agent/npm/`. - Project installs go under `.pi/npm/`. - Set `npmCommand` in `settings.json` to pin npm package lookup and install operations to a specific wrapper command such as `mise` or `asdf`. diff --git a/packages/coding-agent/docs/settings.md b/packages/coding-agent/docs/settings.md index 05b38fa1..befd4da3 100644 --- a/packages/coding-agent/docs/settings.md +++ b/packages/coding-agent/docs/settings.md @@ -153,9 +153,7 @@ When a provider requests a retry delay longer than `retry.provider.maxRetryDelay } ``` -`npmCommand` is used for all npm package-manager operations, including installs, uninstalls, and dependency installs inside git packages. Use argv-style entries exactly as the process should be launched. When `npmCommand` is configured, git package dependency installs use plain `install` to avoid npm-specific flags in wrappers or alternate package managers. - -Normally the package manager's global modules location is queried using `root -g`. As a special case, if the first element of `npmCommand` is `"bun"`, the modules location will instead be queried with `pm bin -g`. +`npmCommand` is used for all npm package-manager operations, including installs, uninstalls, and dependency installs inside git packages. User-scoped npm packages install under `~/.pi/agent/npm/`; project-scoped npm packages install under `.pi/npm/`. Use argv-style entries exactly as the process should be launched. When `npmCommand` is configured, git package dependency installs use plain `install` to avoid npm-specific flags in wrappers or alternate package managers. ### Sessions diff --git a/packages/coding-agent/src/core/package-manager.ts b/packages/coding-agent/src/core/package-manager.ts index 27c18fd4..92f9bede 100644 --- a/packages/coding-agent/src/core/package-manager.ts +++ b/packages/coding-agent/src/core/package-manager.ts @@ -1084,7 +1084,7 @@ export class DefaultPackageManager implements PackageManager { } private async shouldUpdateNpmSource(source: NpmSource, scope: InstalledSourceScope): Promise { - const installedPath = this.getNpmInstallPath(source, scope); + const installedPath = this.getManagedNpmInstallPath(source, scope); const installedVersion = existsSync(installedPath) ? this.getInstalledNpmVersion(installedPath) : undefined; if (!installedVersion) { return true; @@ -1114,13 +1114,9 @@ export class DefaultPackageManager implements PackageManager { } private async installNpmBatch(specs: string[], scope: InstalledSourceScope): Promise { - if (scope === "user") { - await this.runNpmCommand(["install", "-g", ...specs]); - return; - } const installRoot = this.getNpmInstallRoot(scope, false); this.ensureNpmProject(installRoot); - await this.runNpmCommand(["install", ...specs, "--prefix", installRoot]); + await this.runNpmCommand(this.getNpmInstallArgs(specs, installRoot)); } async checkForAvailableUpdates(): Promise { @@ -1669,6 +1665,14 @@ export class DefaultPackageManager implements PackageManager { return { command, args }; } + private getPackageManagerName(): string { + const npmCommand = this.getNpmCommand(); + const commandParts = [npmCommand.command, ...npmCommand.args]; + const separatorIndex = commandParts.lastIndexOf("--"); + const packageManagerCommand = separatorIndex >= 0 ? commandParts[separatorIndex + 1] : npmCommand.command; + return packageManagerCommand ? basename(packageManagerCommand).replace(/\.(cmd|exe)$/i, "") : ""; + } + private async runNpmCommand(args: string[], options?: { cwd?: string }): Promise { const npmCommand = this.getNpmCommand(); await this.runCommand(npmCommand.command, [...npmCommand.args, ...args], options); @@ -1687,25 +1691,32 @@ export class DefaultPackageManager implements PackageManager { return this.runCommandSync(npmCommand.command, [...npmCommand.args, ...args]); } - private async installNpm(source: NpmSource, scope: SourceScope, temporary: boolean): Promise { - if (scope === "user" && !temporary) { - await this.runNpmCommand(["install", "-g", source.spec]); - return; + private getNpmInstallArgs(specs: string[], installRoot: string): string[] { + const packageManagerName = this.getPackageManagerName(); + if (packageManagerName === "bun") { + return ["install", ...specs, "--cwd", installRoot]; } + if (packageManagerName === "pnpm") { + return ["install", ...specs, "--prefix", installRoot, "--config.strict-dep-builds=false"]; + } + return ["install", ...specs, "--prefix", installRoot]; + } + + private async installNpm(source: NpmSource, scope: SourceScope, temporary: boolean): Promise { const installRoot = this.getNpmInstallRoot(scope, temporary); this.ensureNpmProject(installRoot); - await this.runNpmCommand(["install", source.spec, "--prefix", installRoot]); + await this.runNpmCommand(this.getNpmInstallArgs([source.spec], installRoot)); } private async uninstallNpm(source: NpmSource, scope: SourceScope): Promise { - if (scope === "user") { - await this.runNpmCommand(["uninstall", "-g", source.name]); - return; - } const installRoot = this.getNpmInstallRoot(scope, false); if (!existsSync(installRoot)) { return; } + if (this.getPackageManagerName() === "bun") { + await this.runNpmCommand(["uninstall", source.name, "--cwd", installRoot]); + return; + } await this.runNpmCommand(["uninstall", source.name, "--prefix", installRoot]); } @@ -1836,7 +1847,7 @@ export class DefaultPackageManager implements PackageManager { if (scope === "project") { return join(this.cwd, CONFIG_DIR_NAME, "npm"); } - return join(this.getGlobalNpmRoot(), ".."); + return join(this.agentDir, "npm"); } private getGlobalNpmRoot(): string { @@ -1845,8 +1856,7 @@ export class DefaultPackageManager implements PackageManager { if (this.globalNpmRoot && this.globalNpmRootCommandKey === commandKey) { return this.globalNpmRoot; } - const isBunPackageManager = npmCommand.command === "bun"; - if (isBunPackageManager) { + if (this.getPackageManagerName() === "bun") { const binDir = this.runNpmCommandSync(["pm", "bin", "-g"]).trim(); this.globalNpmRoot = join(dirname(binDir), "install", "global", "node_modules"); } else { @@ -1857,14 +1867,7 @@ export class DefaultPackageManager implements PackageManager { } private getPnpmGlobalPackagePath(packageName: string): string | undefined { - const npmCommand = this.getNpmCommand(); - const commandParts = [npmCommand.command, ...npmCommand.args]; - const separatorIndex = commandParts.lastIndexOf("--"); - const packageManagerCommand = separatorIndex >= 0 ? commandParts[separatorIndex + 1] : npmCommand.command; - const packageManagerName = packageManagerCommand - ? basename(packageManagerCommand).replace(/\.(cmd|exe)$/i, "") - : ""; - if (packageManagerName !== "pnpm") { + if (this.getPackageManagerName() !== "pnpm") { return undefined; } @@ -1877,14 +1880,31 @@ export class DefaultPackageManager implements PackageManager { return undefined; } - private getNpmInstallPath(source: NpmSource, scope: SourceScope): string { + private getManagedNpmInstallPath(source: NpmSource, scope: SourceScope): string { if (scope === "temporary") { return join(this.getTemporaryDir("npm"), "node_modules", source.name); } if (scope === "project") { return join(this.cwd, CONFIG_DIR_NAME, "npm", "node_modules", source.name); } - return this.getPnpmGlobalPackagePath(source.name) ?? join(this.getGlobalNpmRoot(), source.name); + return join(this.agentDir, "npm", "node_modules", source.name); + } + + private getLegacyGlobalNpmInstallPath(source: NpmSource): string | undefined { + try { + return this.getPnpmGlobalPackagePath(source.name) ?? join(this.getGlobalNpmRoot(), source.name); + } catch { + return undefined; + } + } + + private getNpmInstallPath(source: NpmSource, scope: SourceScope): string { + const managedPath = this.getManagedNpmInstallPath(source, scope); + if (scope !== "user" || existsSync(managedPath)) { + return managedPath; + } + const legacyPath = this.getLegacyGlobalNpmInstallPath(source); + return legacyPath && existsSync(legacyPath) ? legacyPath : managedPath; } private getGitInstallPath(source: GitSource, scope: SourceScope): string { diff --git a/packages/coding-agent/test/package-manager.test.ts b/packages/coding-agent/test/package-manager.test.ts index 7c0fef67..edd2ce95 100644 --- a/packages/coding-agent/test/package-manager.test.ts +++ b/packages/coding-agent/test/package-manager.test.ts @@ -645,7 +645,28 @@ Content`, expect(runCommandSpy).toHaveBeenCalledWith( "mise", - ["exec", "node@20", "--", "npm", "install", "-g", "@scope/pkg"], + ["exec", "node@20", "--", "npm", "install", "@scope/pkg", "--prefix", join(agentDir, "npm")], + undefined, + ); + }); + + it("should use bun --cwd for npm package installs", async () => { + settingsManager = SettingsManager.inMemory({ + npmCommand: ["mise", "exec", "bun@1", "--", "bun"], + }); + packageManager = new DefaultPackageManager({ + cwd: tempDir, + agentDir, + settingsManager, + }); + + const runCommandSpy = vi.spyOn(packageManager as any, "runCommand").mockResolvedValue(undefined); + + await packageManager.install("npm:@scope/pkg"); + + expect(runCommandSpy).toHaveBeenCalledWith( + "mise", + ["exec", "bun@1", "--", "bun", "install", "@scope/pkg", "--cwd", join(agentDir, "npm")], undefined, ); }); @@ -799,7 +820,7 @@ Content`, expect(runCommandSyncSpy).toHaveBeenNthCalledWith(2, "mise", ["exec", "node@22", "--", "npm", "root", "-g"]); }); - it("should resolve pnpm global package paths from pnpm list output", async () => { + it("should install user npm packages into the pi-managed npm root", async () => { settingsManager = SettingsManager.inMemory({ npmCommand: ["pnpm"], packages: ["npm:pnpm-pkg"], @@ -810,38 +831,25 @@ Content`, settingsManager, }); - const pnpmRoot = join(tempDir, "pnpm", "global", "v11"); - const packagePath = join(pnpmRoot, "20-hash", "node_modules", "pnpm-pkg"); - let installed = false; - - vi.spyOn(packageManager as any, "runCommandSync").mockImplementation((...callArgs: unknown[]) => { - const [command, args] = callArgs as [string, string[]]; - if (command !== "pnpm") { - throw new Error(`unexpected command ${command}`); - } - if (args.join(" ") === "list -g --depth 0 --json") { - return JSON.stringify([ - { - path: pnpmRoot, - dependencies: installed ? { "pnpm-pkg": { version: "1.0.0", path: packagePath } } : {}, - }, - ]); - } - if (args.join(" ") === "root -g") { - return pnpmRoot; - } - throw new Error(`unexpected args ${args.join(" ")}`); + const packagePath = join(agentDir, "npm", "node_modules", "pnpm-pkg"); + vi.spyOn(packageManager as any, "runCommandSync").mockImplementation(() => { + throw new Error("legacy lookup unavailable"); }); const runCommandSpy = vi .spyOn(packageManager as any, "runCommand") .mockImplementation(async (...callArgs: unknown[]) => { const [command, args] = callArgs as [string, string[]]; expect(command).toBe("pnpm"); - expect(args).toEqual(["install", "-g", "pnpm-pkg"]); + expect(args).toEqual([ + "install", + "pnpm-pkg", + "--prefix", + join(agentDir, "npm"), + "--config.strict-dep-builds=false", + ]); mkdirSync(join(packagePath, "extensions"), { recursive: true }); writeFileSync(join(packagePath, "package.json"), JSON.stringify({ name: "pnpm-pkg", version: "1.0.0" })); writeFileSync(join(packagePath, "extensions", "index.ts"), "export default function() {};"); - installed = true; }); const first = await packageManager.resolve(); @@ -857,6 +865,49 @@ Content`, expect(packageManager.getInstalledPath("npm:pnpm-pkg", "user")).toBe(packagePath); }); + it("should load legacy pnpm global package paths from pnpm list output", async () => { + settingsManager = SettingsManager.inMemory({ + npmCommand: ["pnpm"], + packages: ["npm:pnpm-pkg"], + }); + packageManager = new DefaultPackageManager({ + cwd: tempDir, + agentDir, + settingsManager, + }); + + const pnpmRoot = join(tempDir, "pnpm", "global", "v11"); + const packagePath = join(pnpmRoot, "20-hash", "node_modules", "pnpm-pkg"); + mkdirSync(join(packagePath, "extensions"), { recursive: true }); + writeFileSync(join(packagePath, "package.json"), JSON.stringify({ name: "pnpm-pkg", version: "1.0.0" })); + writeFileSync(join(packagePath, "extensions", "index.ts"), "export default function() {};"); + + vi.spyOn(packageManager as any, "runCommandSync").mockImplementation((...callArgs: unknown[]) => { + const [command, args] = callArgs as [string, string[]]; + if (command !== "pnpm") { + throw new Error(`unexpected command ${command}`); + } + if (args.join(" ") === "list -g --depth 0 --json") { + return JSON.stringify([ + { + path: pnpmRoot, + dependencies: { "pnpm-pkg": { version: "1.0.0", path: packagePath } }, + }, + ]); + } + throw new Error(`unexpected args ${args.join(" ")}`); + }); + const runCommandSpy = vi.spyOn(packageManager as any, "runCommand").mockResolvedValue(undefined); + + const result = await packageManager.resolve(); + + expect( + result.extensions.some((r) => r.path === join(packagePath, "extensions", "index.ts") && r.enabled), + ).toBe(true); + expect(runCommandSpy).not.toHaveBeenCalled(); + expect(packageManager.getInstalledPath("npm:pnpm-pkg", "user")).toBe(packagePath); + }); + it("should resolve wrapped pnpm global package paths from pnpm list output", () => { settingsManager = SettingsManager.inMemory({ npmCommand: ["mise", "exec", "node@20", "--", "pnpm"], @@ -883,7 +934,7 @@ Content`, expect(packageManager.getInstalledPath("npm:pnpm-pkg", "user")).toBe(packagePath); }); - it("should fail when pnpm global package list is malformed", () => { + it("should ignore malformed legacy pnpm global package lists", () => { settingsManager = SettingsManager.inMemory({ npmCommand: ["pnpm"], }); @@ -895,7 +946,7 @@ Content`, vi.spyOn(packageManager as any, "runCommandSync").mockReturnValue("not json"); - expect(() => packageManager.getInstalledPath("npm:pnpm-pkg", "user")).toThrow(); + expect(packageManager.getInstalledPath("npm:pnpm-pkg", "user")).toBeUndefined(); }); }); @@ -1829,12 +1880,42 @@ export default function(api) { api.registerTool({ name: "test", description: "te expect(runCommandSpy).not.toHaveBeenCalled(); }); - it("should batch npm updates per scope and run git updates in parallel while skipping pinned and current packages", async () => { - vi.spyOn(packageManager as any, "getGlobalNpmRoot").mockReturnValue(join(agentDir, "node_modules")); + it("should migrate legacy user npm installs into the managed npm root during update", async () => { + const legacyRoot = join(tempDir, "legacy-global", "node_modules"); + const legacyPath = join(legacyRoot, "legacy-pkg"); + const managedPath = join(agentDir, "npm", "node_modules", "legacy-pkg"); + mkdirSync(legacyPath, { recursive: true }); + writeFileSync(join(legacyPath, "package.json"), JSON.stringify({ name: "legacy-pkg", version: "1.0.0" })); + settingsManager.setPackages(["npm:legacy-pkg"]); - const userOldPath = join(agentDir, "node_modules", "user-old"); - const userCurrentPath = join(agentDir, "node_modules", "user-current"); - const userUnknownPath = join(agentDir, "node_modules", "user-unknown"); + vi.spyOn(packageManager as any, "getGlobalNpmRoot").mockReturnValue(legacyRoot); + const runCommandCaptureSpy = vi.spyOn(packageManager as any, "runCommandCapture").mockResolvedValue('"1.0.0"'); + const runCommandSpy = vi + .spyOn(packageManager as any, "runCommand") + .mockImplementation(async (...callArgs: unknown[]) => { + const [command, args] = callArgs as [string, string[]]; + expect(command).toBe("npm"); + expect(args).toEqual(["install", "legacy-pkg@latest", "--prefix", join(agentDir, "npm")]); + mkdirSync(managedPath, { recursive: true }); + writeFileSync( + join(managedPath, "package.json"), + JSON.stringify({ name: "legacy-pkg", version: "1.0.0" }), + ); + }); + + expect(packageManager.getInstalledPath("npm:legacy-pkg", "user")).toBe(legacyPath); + + await packageManager.update("npm:legacy-pkg"); + + expect(runCommandCaptureSpy).not.toHaveBeenCalled(); + expect(runCommandSpy).toHaveBeenCalledTimes(1); + expect(packageManager.getInstalledPath("npm:legacy-pkg", "user")).toBe(managedPath); + }); + + it("should batch npm updates per scope and run git updates in parallel while skipping pinned and current packages", async () => { + const userOldPath = join(agentDir, "npm", "node_modules", "user-old"); + const userCurrentPath = join(agentDir, "npm", "node_modules", "user-current"); + const userUnknownPath = join(agentDir, "npm", "node_modules", "user-unknown"); const projectOldPath = join(tempDir, ".pi", "npm", "node_modules", "project-old"); const projectCurrentPath = join(tempDir, ".pi", "npm", "node_modules", "project-current"); const installPaths = [userOldPath, userCurrentPath, userUnknownPath, projectOldPath, projectCurrentPath]; @@ -1924,7 +2005,7 @@ export default function(api) { api.registerTool({ name: "test", description: "te expect(runCommandSpy).toHaveBeenNthCalledWith( 1, "npm", - ["install", "-g", "user-old@latest", "user-unknown@latest"], + ["install", "user-old@latest", "user-unknown@latest", "--prefix", join(agentDir, "npm")], undefined, ); expect(runCommandSpy).toHaveBeenNthCalledWith(