From 9842ad8494937460892379eeddf21bfc44138471 Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 11 Jun 2026 16:32:27 +0000 Subject: [PATCH 1/4] fix(cli): rejected unloadable npm plugin installs Validated npm plugin extension entry points before recording installs and rolled back fresh installs that cannot resolve their extension imports. Fixes #2312 --- packages/coding-agent/CHANGELOG.md | 4 + .../src/extensibility/plugins/manager.ts | 80 ++++++++++++++-- .../test/plugin-install-validation.test.ts | 92 +++++++++++++++++++ 3 files changed, 167 insertions(+), 9 deletions(-) create mode 100644 packages/coding-agent/test/plugin-install-validation.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 8fdbce5bc..fb1a1182d 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed npm plugin installs to reject packages whose declared extension entry points cannot load because imports or nested dependencies are unresolved ([#2312](https://github.com/can1357/oh-my-pi/issues/2312)). + ## [15.11.2] - 2026-06-11 ### Added diff --git a/packages/coding-agent/src/extensibility/plugins/manager.ts b/packages/coding-agent/src/extensibility/plugins/manager.ts index 1f58a72c5..8d6721af3 100644 --- a/packages/coding-agent/src/extensibility/plugins/manager.ts +++ b/packages/coding-agent/src/extensibility/plugins/manager.ts @@ -11,6 +11,8 @@ import { logger, } from "@oh-my-pi/pi-utils"; import { type GitSource, parseGitUrl } from "./git-url"; +import { installLegacyPiSpecifierShim, loadLegacyPiModule } from "./legacy-pi-compat"; +import { resolvePluginExtensionPaths } from "./loader"; import { extractPackageName, parsePluginSpec } from "./parser"; import type { DoctorCheck, @@ -74,6 +76,14 @@ function gitInstallSpec(original: string, source: GitSource): string { return `${source.repo}#${source.ref}`; } +function hasDefaultExport(value: unknown): value is { default?: unknown } { + return typeof value === "object" && value !== null && "default" in value; +} + +function hasExtensionFactoryExport(module: unknown): boolean { + return typeof module === "function" || (hasDefaultExport(module) && typeof module.default === "function"); +} + // ============================================================================= // Plugin Manager // ============================================================================= @@ -173,6 +183,48 @@ export class PluginManager { } } + async #rollbackFailedInstall( + actualName: string, + packageJsonBefore: string, + wasInstalledBefore: boolean, + ): 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) }); + } + } + + async #validateInstalledExtensions(plugin: InstalledPlugin): Promise { + const extensionPaths = resolvePluginExtensionPaths(plugin); + if (plugin.manifest.extensions && plugin.manifest.extensions.length > 0 && extensionPaths.length === 0) { + throw new Error(`Plugin ${plugin.name} declares extension entries but none resolved to loadable files`); + } + if (extensionPaths.length === 0) { + return; + } + + installLegacyPiSpecifierShim(); + const errors: string[] = []; + for (const extensionPath of extensionPaths) { + try { + const module = await loadLegacyPiModule(extensionPath); + if (!hasExtensionFactoryExport(module)) { + errors.push(`${extensionPath}: extension does not export a valid factory function`); + } + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + errors.push(`${extensionPath}: ${message}`); + } + } + if (errors.length > 0) { + throw new Error(`Plugin ${plugin.name} extension validation failed:\n${errors.join("\n")}`); + } + } + // ========================================================================== // Install / Uninstall // ========================================================================== @@ -217,7 +269,8 @@ export class PluginManager { }; } const pkgJsonPath = getPluginsPackageJson(); - const depsBefore = gitSource ? await this.#readDeps(pkgJsonPath) : {}; + const packageJsonBefore = await Bun.file(pkgJsonPath).text(); + const depsBefore = await this.#readDeps(pkgJsonPath); const packageInstallSpec = gitSource ? gitInstallSpec(spec.packageName, gitSource) : spec.packageName; // Run npm install @@ -307,6 +360,22 @@ export class PluginManager { } // 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] = { @@ -316,14 +385,7 @@ export class PluginManager { }; await this.#saveRuntimeConfig(); - return { - name: pkg.name, - version: pkg.version, - path: path.join(getPluginsNodeModules(), actualName), - manifest, - enabledFeatures, - enabled: true, - }; + return installedPlugin; } /** diff --git a/packages/coding-agent/test/plugin-install-validation.test.ts b/packages/coding-agent/test/plugin-install-validation.test.ts new file mode 100644 index 000000000..668e333d6 --- /dev/null +++ b/packages/coding-agent/test/plugin-install-validation.test.ts @@ -0,0 +1,92 @@ +import { afterEach, beforeEach, describe, expect, test, vi } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { PluginManager } from "@oh-my-pi/pi-coding-agent/extensibility/plugins/manager"; +import * as piUtils from "@oh-my-pi/pi-utils"; +import type { Subprocess } from "bun"; + +function emptyStream(): ReadableStream { + const body = new Response("").body; + if (!body) { + throw new Error("Failed to create empty response stream"); + } + return body; +} + +describe("PluginManager.install load validation", () => { + let tmpRoot: string; + let pluginsDir: string; + let pluginsNodeModules: string; + let pluginsPkgJson: string; + + beforeEach(async () => { + tmpRoot = await fs.mkdtemp(path.join(os.tmpdir(), "omp-plugin-validation-")); + pluginsDir = path.join(tmpRoot, "plugins"); + pluginsNodeModules = path.join(pluginsDir, "node_modules"); + pluginsPkgJson = path.join(pluginsDir, "package.json"); + await fs.mkdir(pluginsNodeModules, { recursive: true }); + + vi.spyOn(piUtils, "getPluginsDir").mockReturnValue(pluginsDir); + vi.spyOn(piUtils, "getPluginsNodeModules").mockReturnValue(pluginsNodeModules); + vi.spyOn(piUtils, "getPluginsPackageJson").mockReturnValue(pluginsPkgJson); + vi.spyOn(piUtils, "getPluginsLockfile").mockReturnValue(path.join(tmpRoot, "omp-plugins.lock.json")); + vi.spyOn(piUtils, "getProjectDir").mockReturnValue(tmpRoot); + vi.spyOn(piUtils, "getProjectPluginOverridesPath").mockReturnValue(path.join(tmpRoot, "plugin-overrides.json")); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + await fs.rm(tmpRoot, { recursive: true, force: true }); + }); + + test("rejects an install whose extension entry cannot resolve its dependencies", async () => { + 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": "1.0.0" } }, + null, + 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', + ); + })(); + + 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({}); + 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); + }); +}); From dd5936701e829fdceb5fe4d639b88cd4e4b19961 Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 11 Jun 2026 16:39:39 +0000 Subject: [PATCH 2/4] 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 }); + }); }); From bfbbbb02afc17f08ceedba0e5d08a7f92124e1ee Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 11 Jun 2026 16:42:27 +0000 Subject: [PATCH 3/4] fix(cli): rejected plugins declaring missing extension entries Surfaced manifest entries that resolve to nothing on disk at install time instead of silently skipping them through resolvePluginExtensionPaths. --- .../src/extensibility/plugins/loader.ts | 49 ++++++++++++------- .../src/extensibility/plugins/manager.ts | 41 ++++++++++------ .../test/plugin-install-validation.test.ts | 49 +++++++++++++++++++ 3 files changed, 104 insertions(+), 35 deletions(-) diff --git a/packages/coding-agent/src/extensibility/plugins/loader.ts b/packages/coding-agent/src/extensibility/plugins/loader.ts index fac12a8c9..6cf3fc3ac 100644 --- a/packages/coding-agent/src/extensibility/plugins/loader.ts +++ b/packages/coding-agent/src/extensibility/plugins/loader.ts @@ -172,34 +172,49 @@ function resolveManifestEntryFile(joined: string): string | null { * Handles both single-string and string[] base entries, plus feature-specific entries. */ function resolvePluginPaths(plugin: InstalledPlugin, key: "tools" | "hooks" | "commands" | "extensions"): string[] { - const paths: string[] = []; + const resolved: string[] = []; + for (const entry of resolvePluginManifestEntries(plugin, key)) { + if (entry.resolvedPath) { + resolved.push(entry.resolvedPath); + } + } + return resolved; +} + +/** + * Declared manifest entries paired with their resolved file path. Returns one + * record per declared entry — base entries first, then enabled-feature entries + * — so callers (e.g. install-time validation) can detect manifest entries that + * point at missing files instead of silently skipping them like + * {@link resolvePluginPaths} does. + */ +export function resolvePluginManifestEntries( + plugin: InstalledPlugin, + key: "tools" | "hooks" | "commands" | "extensions", +): Array<{ entry: string; resolvedPath: string | null }> { + const declared: Array<{ entry: string; resolvedPath: string | null }> = []; const manifest = plugin.manifest; - // Base entry (always included if exists) + const resolveEntry = (entry: string): { entry: string; resolvedPath: string | null } => ({ + entry, + resolvedPath: resolveManifestEntryFile(path.join(plugin.path, entry)), + }); + const base = manifest[key]; if (base) { const entries = Array.isArray(base) ? base : [base]; for (const entry of entries) { - const resolved = resolveManifestEntryFile(path.join(plugin.path, entry)); - if (resolved) { - paths.push(resolved); - } + declared.push(resolveEntry(entry)); } } - // Feature-specific entries if (manifest.features && plugin.enabledFeatures) { const enabledSet = new Set(plugin.enabledFeatures); - for (const [featName, feat] of Object.entries(manifest.features)) { if (!enabledSet.has(featName)) continue; - if (feat[key]) { for (const entry of feat[key]) { - const resolved = resolveManifestEntryFile(path.join(plugin.path, entry)); - if (resolved) { - paths.push(resolved); - } + declared.push(resolveEntry(entry)); } } } @@ -207,19 +222,15 @@ function resolvePluginPaths(plugin: InstalledPlugin, key: "tools" | "hooks" | "c // null means use defaults - enable features with default: true for (const [_featName, feat] of Object.entries(manifest.features)) { if (!feat.default) continue; - if (feat[key]) { for (const entry of feat[key]) { - const resolved = resolveManifestEntryFile(path.join(plugin.path, entry)); - if (resolved) { - paths.push(resolved); - } + declared.push(resolveEntry(entry)); } } } } - return paths; + return declared; } export function resolvePluginToolPaths(plugin: InstalledPlugin): string[] { diff --git a/packages/coding-agent/src/extensibility/plugins/manager.ts b/packages/coding-agent/src/extensibility/plugins/manager.ts index 25871e820..af5af87dd 100644 --- a/packages/coding-agent/src/extensibility/plugins/manager.ts +++ b/packages/coding-agent/src/extensibility/plugins/manager.ts @@ -13,7 +13,7 @@ import { } from "@oh-my-pi/pi-utils"; import { type GitSource, parseGitUrl } from "./git-url"; import { installLegacyPiSpecifierShim, loadLegacyPiModule } from "./legacy-pi-compat"; -import { resolvePluginExtensionPaths } from "./loader"; +import { resolvePluginManifestEntries } from "./loader"; import { extractPackageName, parsePluginSpec } from "./parser"; import type { DoctorCheck, @@ -248,27 +248,36 @@ export class PluginManager { } async #validateInstalledExtensions(plugin: InstalledPlugin): Promise { - const extensionPaths = resolvePluginExtensionPaths(plugin); - if (plugin.manifest.extensions && plugin.manifest.extensions.length > 0 && extensionPaths.length === 0) { - throw new Error(`Plugin ${plugin.name} declares extension entries but none resolved to loadable files`); - } - if (extensionPaths.length === 0) { + const declaredEntries = resolvePluginManifestEntries(plugin, "extensions"); + if (declaredEntries.length === 0) { return; } - installLegacyPiSpecifierShim(); const errors: string[] = []; - for (const extensionPath of extensionPaths) { - try { - const module = await loadLegacyPiModule(extensionPath); - if (!hasExtensionFactoryExport(module)) { - errors.push(`${extensionPath}: extension does not export a valid factory function`); - } - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - errors.push(`${extensionPath}: ${message}`); + const loadable: string[] = []; + for (const { entry, resolvedPath } of declaredEntries) { + if (resolvedPath === null) { + errors.push(`${entry}: declared extension entry not found on disk`); + } else { + loadable.push(resolvedPath); } } + + if (loadable.length > 0) { + installLegacyPiSpecifierShim(); + for (const extensionPath of loadable) { + try { + const module = await loadLegacyPiModule(extensionPath); + if (!hasExtensionFactoryExport(module)) { + errors.push(`${extensionPath}: extension does not export a valid factory function`); + } + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + errors.push(`${extensionPath}: ${message}`); + } + } + } + if (errors.length > 0) { throw new Error(`Plugin ${plugin.name} extension validation failed:\n${errors.join("\n")}`); } diff --git a/packages/coding-agent/test/plugin-install-validation.test.ts b/packages/coding-agent/test/plugin-install-validation.test.ts index e2bbca85d..f8c3ea507 100644 --- a/packages/coding-agent/test/plugin-install-validation.test.ts +++ b/packages/coding-agent/test/plugin-install-validation.test.ts @@ -164,4 +164,53 @@ describe("PluginManager.install load validation", () => { 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 }); }); + + test("rejects an install whose manifest declares a missing extension entry", async () => { + vi.spyOn(Bun, "spawn").mockImplementation(((cmd: string[]) => { + expect(cmd).toEqual(["bun", "install", "partial-plugin"]); + + const prepare = (async () => { + await Bun.write( + pluginsPkgJson, + JSON.stringify( + { name: "omp-plugins", private: true, dependencies: { "partial-plugin": "1.0.0" } }, + null, + 2, + ), + ); + const installedDir = path.join(pluginsNodeModules, "partial-plugin"); + await fs.mkdir(path.join(installedDir, "dist"), { recursive: true }); + await Bun.write( + path.join(installedDir, "package.json"), + JSON.stringify( + { + name: "partial-plugin", + version: "1.0.0", + omp: { extensions: ["./dist/valid.ts", "./dist/missing.ts"] }, + }, + null, + 2, + ), + ); + await Bun.write( + path.join(installedDir, "dist", "valid.ts"), + 'export default function(pi) { pi.registerCommand("valid-ext", { 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("partial-plugin")).rejects.toThrow(/dist\/missing\.ts/); + + const pluginsPackage = await Bun.file(pluginsPkgJson).json(); + expect(pluginsPackage.dependencies ?? {}).toEqual({}); + expect(await Bun.file(path.join(pluginsNodeModules, "partial-plugin", "package.json")).exists()).toBe(false); + expect(await Bun.file(path.join(tmpRoot, "omp-plugins.lock.json")).exists()).toBe(false); + }); }); From 54922241d9115595f286a2f50e1331088f8eea70 Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 11 Jun 2026 16:46:41 +0000 Subject: [PATCH 4/4] fix(cli): restored git plugin upgrades by repo identity Matched installed git dependencies by parsed host and repo path instead of the new ref so failed ref upgrades restore the prior package tree. --- .../src/extensibility/plugins/manager.ts | 17 +++-- .../test/plugin-install-validation.test.ts | 65 +++++++++++++++++++ 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/packages/coding-agent/src/extensibility/plugins/manager.ts b/packages/coding-agent/src/extensibility/plugins/manager.ts index af5af87dd..18844a851 100644 --- a/packages/coding-agent/src/extensibility/plugins/manager.ts +++ b/packages/coding-agent/src/extensibility/plugins/manager.ts @@ -77,10 +77,13 @@ 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, ""); +function findGitPackageName(source: GitSource, deps: Record): string | undefined { for (const [key, value] of Object.entries(deps)) { - if (typeof value === "string" && value.includes(needle)) { + if (typeof value !== "string") { + continue; + } + const installedSource = parseGitUrl(value); + if (installedSource && installedSource.host === source.host && installedSource.path === source.path) { return key; } } @@ -331,7 +334,7 @@ export class PluginManager { const depsBefore = await this.#readDeps(pkgJsonPath); const packageInstallSpec = gitSource ? gitInstallSpec(spec.packageName, gitSource) : spec.packageName; const existingActualName = gitSource - ? findExistingGitPackageName(packageInstallSpec, depsBefore) + ? findGitPackageName(gitSource, depsBefore) : extractPackageName(spec.packageName); const packageSnapshot = await this.#snapshotInstalledPackage(existingActualName); @@ -364,10 +367,10 @@ export class PluginManager { } // 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). + // Match by repository identity, not by ref, so failed upgrades from + // one ref to another still resolve to the original package name. if (!resolved) { - resolved = findExistingGitPackageName(packageInstallSpec, depsAfter); + resolved = findGitPackageName(gitSource, depsAfter); } if (!resolved) { throw new Error( diff --git a/packages/coding-agent/test/plugin-install-validation.test.ts b/packages/coding-agent/test/plugin-install-validation.test.ts index f8c3ea507..b81f8bd2a 100644 --- a/packages/coding-agent/test/plugin-install-validation.test.ts +++ b/packages/coding-agent/test/plugin-install-validation.test.ts @@ -165,6 +165,71 @@ describe("PluginManager.install load validation", () => { expect(lock.plugins["broken-plugin"]).toEqual({ version: "1.0.0", enabledFeatures: null, enabled: true }); }); + test("restores the previous git plugin tree when reinstalling a different ref fails validation", async () => { + await Bun.write( + pluginsPkgJson, + JSON.stringify( + { name: "omp-plugins", private: true, dependencies: { "git-plugin": "github:org/plugin#v1" } }, + null, + 2, + ), + ); + await Bun.write( + path.join(tmpRoot, "omp-plugins.lock.json"), + JSON.stringify( + { plugins: { "git-plugin": { version: "1.0.0", enabledFeatures: null, enabled: true } }, settings: {} }, + null, + 2, + ), + ); + await writePluginPackage(pluginsNodeModules, "git-plugin", { + version: "1.0.0", + source: 'export default function(pi) { pi.registerCommand("git-old-ok", { handler: async () => {} }); }\n', + }); + + vi.spyOn(Bun, "spawn").mockImplementation(((cmd: string[]) => { + expect(cmd).toEqual(["bun", "install", "github:org/plugin#v2"]); + + const prepare = (async () => { + await Bun.write( + pluginsPkgJson, + JSON.stringify( + { name: "omp-plugins", private: true, dependencies: { "git-plugin": "github:org/plugin#v2" } }, + null, + 2, + ), + ); + await writePluginPackage(pluginsNodeModules, "git-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("github:org/plugin#v2")).rejects.toThrow(/missing-peer/); + + const pluginsPackage = await Bun.file(pluginsPkgJson).json(); + expect(pluginsPackage.dependencies).toEqual({ "git-plugin": "github:org/plugin#v1" }); + const restoredPackage = await Bun.file(path.join(pluginsNodeModules, "git-plugin", "package.json")).json(); + expect(restoredPackage.version).toBe("1.0.0"); + const restoredExtension = await Bun.file( + path.join(pluginsNodeModules, "git-plugin", "dist", "extension.ts"), + ).text(); + expect(restoredExtension).toContain("git-old-ok"); + expect(restoredExtension).not.toContain("missing-peer"); + const lock = await Bun.file(path.join(tmpRoot, "omp-plugins.lock.json")).json(); + expect(lock.plugins["git-plugin"]).toEqual({ version: "1.0.0", enabledFeatures: null, enabled: true }); + }); + test("rejects an install whose manifest declares a missing extension entry", async () => { vi.spyOn(Bun, "spawn").mockImplementation(((cmd: string[]) => { expect(cmd).toEqual(["bun", "install", "partial-plugin"]);