From dd5936701e829fdceb5fe4d639b88cd4e4b19961 Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 11 Jun 2026 16:39:39 +0000 Subject: [PATCH] fix(cli): restored plugin tree after invalid reinstall Backed up existing npm plugin package trees before reinstall validation so failed extension loads restore the previous working package contents. --- .../src/extensibility/plugins/manager.ts | 269 +++++++++++------- .../test/plugin-install-validation.test.ts | 113 ++++++-- 2 files changed, 257 insertions(+), 125 deletions(-) diff --git a/packages/coding-agent/src/extensibility/plugins/manager.ts b/packages/coding-agent/src/extensibility/plugins/manager.ts index 8d6721af3..25871e820 100644 --- a/packages/coding-agent/src/extensibility/plugins/manager.ts +++ b/packages/coding-agent/src/extensibility/plugins/manager.ts @@ -1,4 +1,5 @@ import * as fs from "node:fs"; +import * as os from "node:os"; import * as path from "node:path"; import { getPluginsDir, @@ -76,6 +77,16 @@ function gitInstallSpec(original: string, source: GitSource): string { return `${source.repo}#${source.ref}`; } +function findExistingGitPackageName(packageInstallSpec: string, deps: Record): string | undefined { + const needle = packageInstallSpec.replace(/^git\+/i, ""); + for (const [key, value] of Object.entries(deps)) { + if (typeof value === "string" && value.includes(needle)) { + return key; + } + } + return undefined; +} + function hasDefaultExport(value: unknown): value is { default?: unknown } { return typeof value === "object" && value !== null && "default" in value; } @@ -84,6 +95,13 @@ function hasExtensionFactoryExport(module: unknown): boolean { return typeof module === "function" || (hasDefaultExport(module) && typeof module.default === "function"); } +interface PluginPackageSnapshot { + readonly actualName: string; + readonly packagePath: string; + readonly backupRoot: string; + readonly backupPath: string; +} + // ============================================================================= // Plugin Manager // ============================================================================= @@ -183,19 +201,50 @@ export class PluginManager { } } + async #snapshotInstalledPackage(actualName: string | undefined): Promise { + if (!actualName) { + return null; + } + const packagePath = path.join(getPluginsNodeModules(), actualName); + try { + await fs.promises.lstat(packagePath); + } catch (err) { + if (isEnoent(err)) { + return null; + } + throw err; + } + + const backupRoot = await fs.promises.mkdtemp(path.join(os.tmpdir(), "omp-plugin-backup-")); + const backupPath = path.join(backupRoot, "package"); + await fs.promises.cp(packagePath, backupPath, { recursive: true, verbatimSymlinks: true }); + return { actualName, packagePath, backupRoot, backupPath }; + } + + async #cleanupSnapshot(snapshot: PluginPackageSnapshot | null): Promise { + if (!snapshot) { + return; + } + try { + await fs.promises.rm(snapshot.backupRoot, { recursive: true, force: true }); + } catch (err) { + logger.warn("Failed to remove plugin install backup", { plugin: snapshot.actualName, error: String(err) }); + } + } + async #rollbackFailedInstall( actualName: string, packageJsonBefore: string, - wasInstalledBefore: boolean, + snapshot: PluginPackageSnapshot | null, ): Promise { - try { - await Bun.write(getPluginsPackageJson(), packageJsonBefore); - if (!wasInstalledBefore) { - await fs.promises.rm(path.join(getPluginsNodeModules(), actualName), { recursive: true, force: true }); - } - } catch (err) { - logger.warn("Failed to roll back invalid plugin install", { plugin: actualName, error: String(err) }); + await Bun.write(getPluginsPackageJson(), packageJsonBefore); + const packagePath = path.join(getPluginsNodeModules(), actualName); + await fs.promises.rm(packagePath, { recursive: true, force: true }); + if (!snapshot) { + return; } + await fs.promises.mkdir(path.dirname(snapshot.packagePath), { recursive: true }); + await fs.promises.cp(snapshot.backupPath, snapshot.packagePath, { recursive: true, verbatimSymlinks: true }); } async #validateInstalledExtensions(plugin: InstalledPlugin): Promise { @@ -272,120 +321,128 @@ export class PluginManager { const packageJsonBefore = await Bun.file(pkgJsonPath).text(); const depsBefore = await this.#readDeps(pkgJsonPath); const packageInstallSpec = gitSource ? gitInstallSpec(spec.packageName, gitSource) : spec.packageName; + const existingActualName = gitSource + ? findExistingGitPackageName(packageInstallSpec, depsBefore) + : extractPackageName(spec.packageName); + const packageSnapshot = await this.#snapshotInstalledPackage(existingActualName); - // Run npm install - const proc = Bun.spawn(["bun", "install", packageInstallSpec], { - cwd: getPluginsDir(), - stdin: "ignore", - stdout: "pipe", - stderr: "pipe", - windowsHide: true, - }); + try { + // Run npm install + const proc = Bun.spawn(["bun", "install", packageInstallSpec], { + cwd: getPluginsDir(), + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + windowsHide: true, + }); - const exitCode = await proc.exited; - if (exitCode !== 0) { - const stderr = await new Response(proc.stderr).text(); - throw new Error(`npm install failed: ${stderr}`); - } - // Resolve actual package name. npm specs encode the name (strip version); - // git specs do not, so diff plugins/package.json deps to find the new entry. - let actualName: string; - if (gitSource) { - const depsAfter = await this.#readDeps(pkgJsonPath); - let resolved: string | undefined; - for (const key of Object.keys(depsAfter)) { - if (!(key in depsBefore)) { - resolved = key; - break; - } + const exitCode = await proc.exited; + if (exitCode !== 0) { + const stderr = await new Response(proc.stderr).text(); + throw new Error(`npm install failed: ${stderr}`); } - // Fallback: a force-reinstall of an already-present git plugin will not - // add a new key, just rewrite the existing one to the new spec value. - // Match by the install value for force-reinstalls where no new key is - // added (non-GitHub shorthands are normalized before bun sees them). - if (!resolved) { - const needle = packageInstallSpec.replace(/^git\+/i, ""); - for (const [key, value] of Object.entries(depsAfter)) { - if (typeof value === "string" && value.includes(needle)) { + // Resolve actual package name. npm specs encode the name (strip version); + // git specs do not, so diff plugins/package.json deps to find the new entry. + let actualName: string; + if (gitSource) { + const depsAfter = await this.#readDeps(pkgJsonPath); + let resolved: string | undefined; + for (const key of Object.keys(depsAfter)) { + if (!(key in depsBefore)) { resolved = key; break; } } + // Fallback: a force-reinstall of an already-present git plugin will not + // add a new key, just rewrite the existing one to the new spec value. + // Match by the install value for force-reinstalls where no new key is + // added (non-GitHub shorthands are normalized before bun sees them). + if (!resolved) { + resolved = findExistingGitPackageName(packageInstallSpec, depsAfter); + } + if (!resolved) { + throw new Error( + `Installed ${spec.packageName} but could not determine package name from plugins/package.json`, + ); + } + actualName = resolved; + } else { + actualName = extractPackageName(spec.packageName); } - if (!resolved) { - throw new Error( - `Installed ${spec.packageName} but could not determine package name from plugins/package.json`, - ); - } - actualName = resolved; - } else { - actualName = extractPackageName(spec.packageName); - } - const pkgPath = path.join(getPluginsNodeModules(), actualName, "package.json"); + const pkgPath = path.join(getPluginsNodeModules(), actualName, "package.json"); - let pkg: { name: string; version: string; omp?: PluginManifest; pi?: PluginManifest }; - try { - pkg = await Bun.file(pkgPath).json(); - } catch (err) { - if (isEnoent(err)) { - throw new Error(`Package installed but package.json not found at ${pkgPath}`); + let pkg: { name: string; version: string; omp?: PluginManifest; pi?: PluginManifest }; + try { + pkg = await Bun.file(pkgPath).json(); + } catch (err) { + if (isEnoent(err)) { + throw new Error(`Package installed but package.json not found at ${pkgPath}`); + } + throw err; } - throw err; - } - const manifest: PluginManifest = pkg.omp || pkg.pi || { version: pkg.version }; - manifest.version = pkg.version; + const manifest: PluginManifest = pkg.omp || pkg.pi || { version: pkg.version }; + manifest.version = pkg.version; - // Resolve enabled features - let enabledFeatures: string[] | null = null; - if (spec.features === "*") { - // All features - enabledFeatures = manifest.features ? Object.keys(manifest.features) : null; - } else if (Array.isArray(spec.features)) { - if (spec.features.length > 0) { - // Validate requested features exist - if (manifest.features) { - for (const feat of spec.features) { - if (!(feat in manifest.features)) { - throw new Error( - `Unknown feature "${feat}" in ${actualName}. Available: ${Object.keys(manifest.features).join(", ")}`, - ); + // Resolve enabled features + let enabledFeatures: string[] | null = null; + if (spec.features === "*") { + // All features + enabledFeatures = manifest.features ? Object.keys(manifest.features) : null; + } else if (Array.isArray(spec.features)) { + if (spec.features.length > 0) { + // Validate requested features exist + if (manifest.features) { + for (const feat of spec.features) { + if (!(feat in manifest.features)) { + throw new Error( + `Unknown feature "${feat}" in ${actualName}. Available: ${Object.keys(manifest.features).join(", ")}`, + ); + } } } + enabledFeatures = spec.features; + } else { + // Empty array = no optional features + enabledFeatures = []; } - enabledFeatures = spec.features; - } else { - // Empty array = no optional features - enabledFeatures = []; } + // null = use defaults + + const installedPlugin: InstalledPlugin = { + name: pkg.name, + version: pkg.version, + path: path.join(getPluginsNodeModules(), actualName), + manifest, + enabledFeatures, + enabled: true, + }; + + try { + await this.#validateInstalledExtensions(installedPlugin); + } catch (err) { + try { + await this.#rollbackFailedInstall(actualName, packageJsonBefore, packageSnapshot); + } catch (rollbackErr) { + const message = err instanceof Error ? err.message : String(err); + const rollbackMessage = rollbackErr instanceof Error ? rollbackErr.message : String(rollbackErr); + throw new Error(`${message}\nRollback failed: ${rollbackMessage}`); + } + throw err; + } + + // Update runtime config + const config = await this.#ensureConfigLoaded(); + config.plugins[pkg.name] = { + version: pkg.version, + enabledFeatures, + enabled: true, + }; + await this.#saveRuntimeConfig(); + + return installedPlugin; + } finally { + await this.#cleanupSnapshot(packageSnapshot); } - // null = use defaults - - const installedPlugin: InstalledPlugin = { - name: pkg.name, - version: pkg.version, - path: path.join(getPluginsNodeModules(), actualName), - manifest, - enabledFeatures, - enabled: true, - }; - - try { - await this.#validateInstalledExtensions(installedPlugin); - } catch (err) { - await this.#rollbackFailedInstall(actualName, packageJsonBefore, actualName in depsBefore); - throw err; - } - - // Update runtime config - const config = await this.#ensureConfigLoaded(); - config.plugins[pkg.name] = { - version: pkg.version, - enabledFeatures, - enabled: true, - }; - await this.#saveRuntimeConfig(); - - return installedPlugin; } /** diff --git a/packages/coding-agent/test/plugin-install-validation.test.ts b/packages/coding-agent/test/plugin-install-validation.test.ts index 668e333d6..e2bbca85d 100644 --- a/packages/coding-agent/test/plugin-install-validation.test.ts +++ b/packages/coding-agent/test/plugin-install-validation.test.ts @@ -14,6 +14,33 @@ function emptyStream(): ReadableStream { return body; } +interface PluginFixture { + readonly version: string; + readonly source: string; + readonly dependencyVersion?: string; + readonly peerDependencies?: Record; +} + +async function writePluginPackage(pluginsNodeModules: string, name: string, fixture: PluginFixture): Promise { + const installedDir = path.join(pluginsNodeModules, name); + await fs.mkdir(path.join(installedDir, "dist"), { recursive: true }); + await Bun.write( + path.join(installedDir, "package.json"), + JSON.stringify( + { + name, + version: fixture.version, + ...(fixture.peerDependencies ? { peerDependencies: fixture.peerDependencies } : {}), + omp: { extensions: ["./dist/extension.ts"] }, + }, + null, + 2, + ), + ); + await Bun.write(path.join(installedDir, "dist", "extension.ts"), fixture.source); + return installedDir; +} + describe("PluginManager.install load validation", () => { let tmpRoot: string; let pluginsDir: string; @@ -53,25 +80,12 @@ describe("PluginManager.install load validation", () => { 2, ), ); - const installedDir = path.join(pluginsNodeModules, "broken-plugin"); - await fs.mkdir(path.join(installedDir, "dist"), { recursive: true }); - await Bun.write( - path.join(installedDir, "package.json"), - JSON.stringify( - { - name: "broken-plugin", - version: "1.0.0", - peerDependencies: { "missing-peer": "^1.0.0" }, - omp: { extensions: ["./dist/extension.ts"] }, - }, - null, - 2, - ), - ); - await Bun.write( - path.join(installedDir, "dist", "extension.ts"), - 'import { missing } from "missing-peer";\nexport default function(pi) { pi.registerCommand(String(missing), { handler: async () => {} }); }\n', - ); + await writePluginPackage(pluginsNodeModules, "broken-plugin", { + version: "1.0.0", + peerDependencies: { "missing-peer": "^1.0.0" }, + source: + 'import { missing } from "missing-peer";\nexport default function(pi) { pi.registerCommand(String(missing), { handler: async () => {} }); }\n', + }); })(); return { @@ -89,4 +103,65 @@ describe("PluginManager.install load validation", () => { expect(await Bun.file(path.join(pluginsNodeModules, "broken-plugin", "package.json")).exists()).toBe(false); expect(await Bun.file(path.join(tmpRoot, "omp-plugins.lock.json")).exists()).toBe(false); }); + + test("restores the previous package tree when reinstall validation fails", async () => { + await Bun.write( + pluginsPkgJson, + JSON.stringify({ name: "omp-plugins", private: true, dependencies: { "broken-plugin": "1.0.0" } }, null, 2), + ); + await Bun.write( + path.join(tmpRoot, "omp-plugins.lock.json"), + JSON.stringify( + { plugins: { "broken-plugin": { version: "1.0.0", enabledFeatures: null, enabled: true } }, settings: {} }, + null, + 2, + ), + ); + await writePluginPackage(pluginsNodeModules, "broken-plugin", { + version: "1.0.0", + source: 'export default function(pi) { pi.registerCommand("old-ok", { handler: async () => {} }); }\n', + }); + + vi.spyOn(Bun, "spawn").mockImplementation(((cmd: string[]) => { + expect(cmd).toEqual(["bun", "install", "broken-plugin"]); + + const prepare = (async () => { + await Bun.write( + pluginsPkgJson, + JSON.stringify( + { name: "omp-plugins", private: true, dependencies: { "broken-plugin": "2.0.0" } }, + null, + 2, + ), + ); + await writePluginPackage(pluginsNodeModules, "broken-plugin", { + version: "2.0.0", + peerDependencies: { "missing-peer": "^1.0.0" }, + source: + 'import { missing } from "missing-peer";\nexport default function(pi) { pi.registerCommand(String(missing), { handler: async () => {} }); }\n', + }); + })(); + + return { + pid: 1, + stdout: emptyStream(), + stderr: emptyStream(), + exited: prepare.then(() => 0), + } as Subprocess; + }) as typeof Bun.spawn); + + await expect(new PluginManager(tmpRoot).install("broken-plugin")).rejects.toThrow(/missing-peer/); + + const pluginsPackage = await Bun.file(pluginsPkgJson).json(); + expect(pluginsPackage.dependencies).toEqual({ "broken-plugin": "1.0.0" }); + const restoredPackage = await Bun.file(path.join(pluginsNodeModules, "broken-plugin", "package.json")).json(); + expect(restoredPackage.version).toBe("1.0.0"); + const restoredExtension = await Bun.file( + path.join(pluginsNodeModules, "broken-plugin", "dist", "extension.ts"), + ).text(); + expect(restoredExtension).toContain("old-ok"); + expect(restoredExtension).not.toContain("missing-peer"); + const lock = await Bun.file(path.join(tmpRoot, "omp-plugins.lock.json")).json(); + expect(lock.plugins["broken-plugin"]).toEqual({ version: "1.0.0", enabledFeatures: null, enabled: true }); + }); });