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.
This commit is contained in:
@@ -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[] {
|
||||
|
||||
@@ -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<void> {
|
||||
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")}`);
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user