fix: prevent one project's bad config from breaking shared plugin
When multiple projects share the same external plugin, a misconfiguration in one project (wrong expectedPluginName) would prevent the plugin from being registered at all, silently breaking it for all projects. Fix by: 1. Register the plugin FIRST, before validating individual project configs 2. Validate each project's config in its own try-catch 3. Log validation errors but continue processing other projects This ensures: - Plugin is always registered if it loads successfully - Projects with correct config get updated - Projects with bad config get a warning but don't break others - Misconfigured projects fail at point of use with clearer errors Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
parent
aae6377de4
commit
fcdbe24d6c
|
|
@ -530,7 +530,7 @@ describe("External plugin manifest validation", () => {
|
|||
expect(registry.get("tracker", "jira")).not.toBeNull();
|
||||
});
|
||||
|
||||
it("warns when manifest.name does not match expectedPluginName", async () => {
|
||||
it("warns when manifest.name does not match expectedPluginName but still registers plugin", async () => {
|
||||
const registry = createPluginRegistry();
|
||||
|
||||
const mockPlugin = {
|
||||
|
|
@ -566,12 +566,16 @@ describe("External plugin manifest validation", () => {
|
|||
],
|
||||
});
|
||||
|
||||
// Should warn but not throw (error is caught and logged)
|
||||
// Should warn about validation failure but still register the plugin
|
||||
const stderrSpy = vi.spyOn(process.stderr, "write").mockImplementation(() => true);
|
||||
await registry.loadFromConfig(config, importFn);
|
||||
|
||||
// Plugin should still be registered under its manifest.name
|
||||
expect(registry.get("tracker", "jira-enterprise")).not.toBeNull();
|
||||
|
||||
// Should have logged a validation warning
|
||||
expect(stderrSpy).toHaveBeenCalledWith(
|
||||
expect.stringContaining("Failed to load plugin"),
|
||||
expect.stringContaining("Config validation failed for projects.proj1.tracker"),
|
||||
);
|
||||
stderrSpy.mockRestore();
|
||||
});
|
||||
|
|
@ -771,4 +775,76 @@ describe("External plugin manifest validation", () => {
|
|||
// Plugin should be registered under manifest.name
|
||||
expect(registry.get("tracker", "jira-cloud")).not.toBeNull();
|
||||
});
|
||||
|
||||
it("registers plugin even when one project has misconfigured expectedPluginName", async () => {
|
||||
const registry = createPluginRegistry();
|
||||
|
||||
const mockPlugin = {
|
||||
manifest: { name: "jira-cloud", slot: "tracker" as const, version: "1.0.0", description: "Jira Cloud" },
|
||||
create: vi.fn(() => ({})),
|
||||
};
|
||||
|
||||
const importFn = vi.fn(async () => mockPlugin);
|
||||
|
||||
const config = makeOrchestratorConfig({
|
||||
configPath: "/test/config.yaml",
|
||||
plugins: [
|
||||
{ name: "jira", source: "npm", package: "@acme/ao-plugin-tracker-jira", enabled: true },
|
||||
],
|
||||
projects: {
|
||||
proj1: {
|
||||
path: "/repos/test1",
|
||||
repo: "org/test1",
|
||||
name: "proj1",
|
||||
defaultBranch: "main",
|
||||
sessionPrefix: "test1",
|
||||
tracker: { plugin: "jira", package: "@acme/ao-plugin-tracker-jira" },
|
||||
},
|
||||
proj2: {
|
||||
path: "/repos/test2",
|
||||
repo: "org/test2",
|
||||
name: "proj2",
|
||||
defaultBranch: "main",
|
||||
sessionPrefix: "test2",
|
||||
// Same external plugin but with WRONG explicit plugin name
|
||||
tracker: { plugin: "wrong-name", package: "@acme/ao-plugin-tracker-jira" },
|
||||
},
|
||||
},
|
||||
_externalPluginEntries: [
|
||||
{
|
||||
source: "projects.proj1.tracker",
|
||||
location: { kind: "project", projectId: "proj1", configType: "tracker" },
|
||||
slot: "tracker",
|
||||
package: "@acme/ao-plugin-tracker-jira",
|
||||
// No expectedPluginName - will accept any manifest.name
|
||||
},
|
||||
{
|
||||
source: "projects.proj2.tracker",
|
||||
location: { kind: "project", projectId: "proj2", configType: "tracker" },
|
||||
slot: "tracker",
|
||||
package: "@acme/ao-plugin-tracker-jira",
|
||||
expectedPluginName: "wrong-name", // Mismatches manifest.name "jira-cloud"
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
const stderrSpy = vi.spyOn(process.stderr, "write").mockImplementation(() => true);
|
||||
await registry.loadFromConfig(config, importFn);
|
||||
|
||||
// Plugin should STILL be registered despite proj2's misconfiguration
|
||||
expect(registry.get("tracker", "jira-cloud")).not.toBeNull();
|
||||
|
||||
// proj1 should be updated correctly (no expectedPluginName = accepts any)
|
||||
expect(config.projects.proj1.tracker?.plugin).toBe("jira-cloud");
|
||||
|
||||
// proj2's config should NOT be updated (validation failed)
|
||||
expect(config.projects.proj2.tracker?.plugin).toBe("wrong-name");
|
||||
|
||||
// Should have logged a warning about proj2's validation failure
|
||||
expect(stderrSpy).toHaveBeenCalledWith(
|
||||
expect.stringContaining("Config validation failed for projects.proj2.tracker"),
|
||||
);
|
||||
|
||||
stderrSpy.mockRestore();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -365,18 +365,29 @@ export function createPluginRegistry(): PluginRegistry {
|
|||
const mod = normalizeImportedPluginModule(await doImport(specifier));
|
||||
if (!mod) continue;
|
||||
|
||||
// Register the plugin FIRST, before validating individual project configs.
|
||||
// This ensures the plugin is available even if some projects have misconfigurations.
|
||||
const pluginConfig = extractPluginConfig(mod.manifest.slot, mod.manifest.name, config);
|
||||
this.register(mod, pluginConfig);
|
||||
|
||||
// Check if this plugin was auto-added from inline tracker/scm/notifier config
|
||||
// Multiple projects may share the same external plugin, so find ALL matching entries
|
||||
const matchingEntries = findAllExternalPluginEntries(plugin, externalEntries);
|
||||
for (const externalEntry of matchingEntries) {
|
||||
// Validate manifest.name matches expectedPluginName (if specified)
|
||||
validateManifestName(mod.manifest, externalEntry, specifier);
|
||||
// Update the config with the actual manifest.name
|
||||
updateConfigWithManifestName(mod.manifest, externalEntry, config);
|
||||
try {
|
||||
// Validate manifest.name matches expectedPluginName (if specified)
|
||||
validateManifestName(mod.manifest, externalEntry, specifier);
|
||||
// Update the config with the actual manifest.name
|
||||
updateConfigWithManifestName(mod.manifest, externalEntry, config);
|
||||
} catch (validationError) {
|
||||
// Log validation errors but don't abort - other projects can still use the plugin.
|
||||
// The misconfigured project will fail later when it tries to use the plugin
|
||||
// with the wrong name, giving a clearer error at point of use.
|
||||
process.stderr.write(
|
||||
`[plugin-registry] Config validation failed for ${externalEntry.source}: ${validationError}\n`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
const pluginConfig = extractPluginConfig(mod.manifest.slot, mod.manifest.name, config);
|
||||
this.register(mod, pluginConfig);
|
||||
} catch (error) {
|
||||
process.stderr.write(`[plugin-registry] Failed to load plugin "${specifier}": ${error}\n`);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue