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); + }); });