diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index ca0bf6938..567c3bbfe 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -9,6 +9,7 @@ ### Changed +- Changed `pi update` to batch npm package updates per scope and run git package updates with bounded parallelism, reducing multi-package update time while preserving skip behavior for pinned and already-current packages ([#2980](https://github.com/badlogic/pi-mono/issues/2980)) - Documented async extension factory functions in the extensions and custom-provider docs, including startup ordering and dynamic model/provider discovery via async initialization ([#3469](https://github.com/badlogic/pi-mono/issues/3469)) ### Fixed diff --git a/packages/coding-agent/src/core/package-manager.ts b/packages/coding-agent/src/core/package-manager.ts index 4de0c3355..fded4d50f 100644 --- a/packages/coding-agent/src/core/package-manager.ts +++ b/packages/coding-agent/src/core/package-manager.ts @@ -24,6 +24,7 @@ import type { PackageSource, SettingsManager } from "./settings-manager.js"; const NETWORK_TIMEOUT_MS = 10000; const UPDATE_CHECK_CONCURRENCY = 4; +const GIT_UPDATE_CONCURRENCY = 4; function isOfflineModeEnabled(): boolean { const value = process.env.PI_OFFLINE; @@ -116,6 +117,21 @@ type LocalSource = { type ParsedSource = NpmSource | GitSource | LocalSource; +type InstalledSourceScope = Exclude; + +interface ConfiguredUpdateSource { + source: string; + scope: InstalledSourceScope; +} + +interface NpmUpdateTarget extends ConfiguredUpdateSource { + parsed: NpmSource; +} + +interface GitUpdateTarget extends ConfiguredUpdateSource { + parsed: GitSource; +} + interface PiManifest { extensions?: string[]; skills?: string[]; @@ -970,18 +986,19 @@ export class DefaultPackageManager implements PackageManager { const projectSettings = this.settingsManager.getProjectSettings(); const identity = source ? this.getPackageIdentity(source) : undefined; let matched = false; + const updateSources: ConfiguredUpdateSource[] = []; for (const pkg of globalSettings.packages ?? []) { const sourceStr = typeof pkg === "string" ? pkg : pkg.source; if (identity && this.getPackageIdentity(sourceStr, "user") !== identity) continue; matched = true; - await this.updateSourceForScope(sourceStr, "user"); + updateSources.push({ source: sourceStr, scope: "user" }); } for (const pkg of projectSettings.packages ?? []) { const sourceStr = typeof pkg === "string" ? pkg : pkg.source; if (identity && this.getPackageIdentity(sourceStr, "project") !== identity) continue; matched = true; - await this.updateSourceForScope(sourceStr, "project"); + updateSources.push({ source: sourceStr, scope: "project" }); } if (source && !matched) { @@ -992,48 +1009,106 @@ export class DefaultPackageManager implements PackageManager { ]), ); } + + await this.updateConfiguredSources(updateSources); } - private async updateSourceForScope(source: string, scope: SourceScope): Promise { - if (isOfflineModeEnabled()) { + private async updateConfiguredSources(sources: ConfiguredUpdateSource[]): Promise { + if (isOfflineModeEnabled() || sources.length === 0) { return; } - const parsed = this.parseSource(source); - if (parsed.type === "npm") { - if (parsed.pinned) return; - const installedPath = this.getNpmInstallPath(parsed, scope); - const installedVersion = existsSync(installedPath) ? this.getInstalledNpmVersion(installedPath) : undefined; - if (installedVersion) { - try { - const latestVersion = await this.getLatestNpmVersion(parsed.name); - if (latestVersion === installedVersion) { - return; - } - } catch { - // Preserve existing update behavior when version lookup fails. - } + const npmCandidates: NpmUpdateTarget[] = []; + const gitCandidates: GitUpdateTarget[] = []; + + for (const entry of sources) { + const parsed = this.parseSource(entry.source); + if (parsed.type === "local" || parsed.pinned) { + continue; } + if (parsed.type === "npm") { + npmCandidates.push({ ...entry, parsed }); + continue; + } + gitCandidates.push({ ...entry, parsed }); + } - await this.withProgress("update", source, `Updating ${source}...`, async () => { - await this.installNpm( - { - ...parsed, - spec: `${parsed.name}@latest`, - }, - scope, - false, - ); - }); + const npmCheckTasks = npmCandidates.map((entry) => async () => ({ + entry, + shouldUpdate: await this.shouldUpdateNpmSource(entry.parsed, entry.scope), + })); + const npmCheckResults = await this.runWithConcurrency(npmCheckTasks, UPDATE_CHECK_CONCURRENCY); + const userNpmUpdates: NpmUpdateTarget[] = []; + const projectNpmUpdates: NpmUpdateTarget[] = []; + for (const result of npmCheckResults) { + if (!result.shouldUpdate) { + continue; + } + if (result.entry.scope === "user") { + userNpmUpdates.push(result.entry); + } else { + projectNpmUpdates.push(result.entry); + } + } + + const tasks: Promise[] = []; + if (userNpmUpdates.length > 0) { + tasks.push(this.updateNpmBatch(userNpmUpdates, "user")); + } + if (projectNpmUpdates.length > 0) { + tasks.push(this.updateNpmBatch(projectNpmUpdates, "project")); + } + if (gitCandidates.length > 0) { + const gitTasks = gitCandidates.map( + (entry) => async () => + this.withProgress("update", entry.source, `Updating ${entry.source}...`, async () => { + await this.updateGit(entry.parsed, entry.scope); + }), + ); + tasks.push(this.runWithConcurrency(gitTasks, GIT_UPDATE_CONCURRENCY).then(() => {})); + } + + await Promise.all(tasks); + } + + private async shouldUpdateNpmSource(source: NpmSource, scope: InstalledSourceScope): Promise { + const installedPath = this.getNpmInstallPath(source, scope); + const installedVersion = existsSync(installedPath) ? this.getInstalledNpmVersion(installedPath) : undefined; + if (!installedVersion) { + return true; + } + + try { + const latestVersion = await this.getLatestNpmVersion(source.name); + return latestVersion !== installedVersion; + } catch { + // Preserve existing update behavior when version lookup fails. + return true; + } + } + + private async updateNpmBatch(sources: NpmUpdateTarget[], scope: InstalledSourceScope): Promise { + if (sources.length === 0) { return; } - if (parsed.type === "git") { - if (parsed.pinned) return; - await this.withProgress("update", source, `Updating ${source}...`, async () => { - await this.updateGit(parsed, scope); - }); + + const sourceLabel = sources.length === 1 ? sources[0].source : `${scope} npm packages`; + const message = sources.length === 1 ? `Updating ${sources[0].source}...` : `Updating ${scope} npm packages...`; + const specs = sources.map((entry) => `${entry.parsed.name}@latest`); + + await this.withProgress("update", sourceLabel, message, async () => { + await this.installNpmBatch(specs, scope); + }); + } + + 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]); } async checkForAvailableUpdates(): Promise { diff --git a/packages/coding-agent/test/package-manager.test.ts b/packages/coding-agent/test/package-manager.test.ts index ffd639fd0..3d0e93584 100644 --- a/packages/coding-agent/test/package-manager.test.ts +++ b/packages/coding-agent/test/package-manager.test.ts @@ -1497,6 +1497,115 @@ 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")); + + const userOldPath = join(agentDir, "node_modules", "user-old"); + const userCurrentPath = join(agentDir, "node_modules", "user-current"); + const userUnknownPath = join(agentDir, "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]; + for (const installPath of installPaths) { + mkdirSync(installPath, { recursive: true }); + } + writeFileSync(join(userOldPath, "package.json"), JSON.stringify({ name: "user-old", version: "1.0.0" })); + writeFileSync( + join(userCurrentPath, "package.json"), + JSON.stringify({ name: "user-current", version: "1.0.0" }), + ); + writeFileSync( + join(userUnknownPath, "package.json"), + JSON.stringify({ name: "user-unknown", version: "1.0.0" }), + ); + writeFileSync(join(projectOldPath, "package.json"), JSON.stringify({ name: "project-old", version: "1.0.0" })); + writeFileSync( + join(projectCurrentPath, "package.json"), + JSON.stringify({ name: "project-current", version: "1.0.0" }), + ); + + settingsManager.setPackages([ + "npm:user-old", + "npm:user-current", + "npm:user-unknown", + "npm:user-pinned@1.0.0", + "git:github.com/example/user-repo-a", + "git:github.com/example/user-repo-b", + "git:github.com/example/user-repo-pinned@v1", + ]); + settingsManager.setProjectPackages([ + "npm:project-old", + "npm:project-current", + "npm:project-missing", + "git:github.com/example/project-repo-a", + ]); + + const runCommandCaptureSpy = vi + .spyOn(packageManager as any, "runCommandCapture") + .mockImplementation(async (...callArgs: unknown[]) => { + const [_command, args] = callArgs as [string, string[]]; + if (args[0] !== "view") { + throw new Error(`Unexpected runCommandCapture args: ${args.join(" ")}`); + } + switch (args[1]) { + case "user-old": + case "project-old": + return '"2.0.0"'; + case "user-current": + case "project-current": + return '"1.0.0"'; + case "user-unknown": + throw new Error("registry unavailable"); + default: + throw new Error(`Unexpected package lookup: ${args[1]}`); + } + }); + + let activeNpmUpdates = 0; + let maxConcurrentNpmUpdates = 0; + const runCommandSpy = vi + .spyOn(packageManager as any, "runCommand") + .mockImplementation(async (...callArgs: unknown[]) => { + const [command, args] = callArgs as [string, string[]]; + if (command !== "npm") { + throw new Error(`Unexpected runCommand call: ${command} ${args.join(" ")}`); + } + activeNpmUpdates += 1; + maxConcurrentNpmUpdates = Math.max(maxConcurrentNpmUpdates, activeNpmUpdates); + await new Promise((resolve) => setTimeout(resolve, 20)); + activeNpmUpdates -= 1; + }); + + let activeGitUpdates = 0; + let maxConcurrentGitUpdates = 0; + const updateGitSpy = vi.spyOn(packageManager as any, "updateGit").mockImplementation(async () => { + activeGitUpdates += 1; + maxConcurrentGitUpdates = Math.max(maxConcurrentGitUpdates, activeGitUpdates); + await new Promise((resolve) => setTimeout(resolve, 20)); + activeGitUpdates -= 1; + }); + + await packageManager.update(); + + expect(runCommandCaptureSpy).toHaveBeenCalledTimes(5); + expect(runCommandSpy).toHaveBeenCalledTimes(2); + expect(runCommandSpy).toHaveBeenNthCalledWith( + 1, + "npm", + ["install", "-g", "user-old@latest", "user-unknown@latest"], + undefined, + ); + expect(runCommandSpy).toHaveBeenNthCalledWith( + 2, + "npm", + ["install", "project-old@latest", "project-missing@latest", "--prefix", join(tempDir, ".pi", "npm")], + undefined, + ); + expect(updateGitSpy).toHaveBeenCalledTimes(3); + expect(maxConcurrentNpmUpdates).toBeGreaterThan(1); + expect(maxConcurrentGitUpdates).toBeGreaterThan(1); + }); + it("should suggest npm source prefixes for update lookups", async () => { settingsManager.setProjectPackages(["npm:example"]);