fix: address remaining PR review comments

- Fix multiple projects sharing same external plugin: change
  findExternalPluginEntry to findAllExternalPluginEntries which returns
  all matching entries, allowing all projects to be updated with the
  actual manifest.name

- Fix path without slashes misidentified as npm package: use separate
  pkg and path parameters in generateTempPluginName instead of combining
  them and checking for slashes. Local paths like "my-tracker" (no
  slashes) are now correctly returned as-is

- Replace console.warn with process.stderr.write for structured logging
  in plugin-registry.ts

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
harshitsinghbhandari 2026-04-02 20:06:28 +05:30
parent 812bfa5bef
commit 3e0ce4e3a9
4 changed files with 114 additions and 27 deletions

View File

@ -1032,4 +1032,22 @@ describe("External Plugin Name Generation", () => {
expect(config.projects.proj1.tracker?.plugin).toBe("my-custom-name");
});
it("handles local path without slashes correctly", () => {
const config = validateConfig({
projects: {
proj1: {
path: "/repos/test",
repo: "org/test",
defaultBranch: "main",
tracker: {
path: "my-tracker",
},
},
},
});
// Should use the path as-is (not split by hyphens like npm packages)
expect(config.projects.proj1.tracker?.plugin).toBe("my-tracker");
});
});

View File

@ -535,14 +535,13 @@ describe("External plugin manifest validation", () => {
});
// Should warn but not throw (error is caught and logged)
const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
const stderrSpy = vi.spyOn(process.stderr, "write").mockImplementation(() => true);
await registry.loadFromConfig(config, importFn);
expect(consoleSpy).toHaveBeenCalledWith(
expect(stderrSpy).toHaveBeenCalledWith(
expect.stringContaining("Failed to load plugin"),
expect.any(Error),
);
consoleSpy.mockRestore();
stderrSpy.mockRestore();
});
it("infers plugin name when expectedPluginName is not specified", async () => {
@ -671,12 +670,73 @@ describe("External plugin manifest validation", () => {
],
});
const consoleSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
const stderrSpy = vi.spyOn(process.stderr, "write").mockImplementation(() => true);
await registry.loadFromConfig(config, importFn);
expect(consoleSpy).toHaveBeenCalledWith(
expect(stderrSpy).toHaveBeenCalledWith(
expect.stringContaining("has slot \"notifier\" but was configured as \"tracker\""),
);
consoleSpy.mockRestore();
stderrSpy.mockRestore();
});
it("updates all projects sharing same external plugin with manifest.name", 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 as proj1
tracker: { plugin: "jira", 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",
// No expectedPluginName - will accept any manifest.name
},
],
});
await registry.loadFromConfig(config, importFn);
// Both projects should be updated with the actual manifest.name
expect(config.projects.proj1.tracker?.plugin).toBe("jira-cloud");
expect(config.projects.proj2.tracker?.plugin).toBe("jira-cloud");
// Plugin should be registered under manifest.name
expect(registry.get("tracker", "jira-cloud")).not.toBeNull();
});
});

View File

@ -313,22 +313,26 @@ function expandPaths(config: OrchestratorConfig): OrchestratorConfig {
* Format: extract the last segment from the package/path, removing common prefixes.
* e.g., "@acme/ao-plugin-tracker-jira" -> "jira"
* e.g., "./plugins/my-tracker" -> "my-tracker"
* e.g., "my-tracker" (local path without slashes) -> "my-tracker"
*/
function generateTempPluginName(pkg?: string, path?: string): string {
const specifier = pkg ?? path ?? "unknown";
// For npm packages, extract the last part after the last hyphen or slash
// Handle npm packages: extract the last part after the last hyphen
// @acme/ao-plugin-tracker-jira -> jira
// @composio/ao-plugin-scm-gitlab -> gitlab
if (specifier.startsWith("@") || !specifier.includes("/")) {
const parts = specifier.split(/[-/]/);
return parts[parts.length - 1] ?? specifier;
if (pkg) {
const parts = pkg.split(/[-/]/);
return parts[parts.length - 1] ?? pkg;
}
// For local paths, use the basename
// Handle local paths: use the basename
// ./plugins/my-tracker -> my-tracker
const segments = specifier.split("/").filter((s) => s && s !== "." && s !== "..");
return segments[segments.length - 1] ?? specifier;
// my-tracker -> my-tracker (no slashes is still a valid path)
if (path) {
const segments = path.split("/").filter((s) => s && s !== "." && s !== "..");
return segments[segments.length - 1] ?? path;
}
return "unknown";
}
/**

View File

@ -86,16 +86,20 @@ function extractPluginConfig(
}
/**
* Find the external plugin entry that matches a given plugin config.
* Find ALL external plugin entries that match a given plugin config.
* Used for manifest.name validation when loading inline tracker/scm/notifier plugins.
*
* Returns all matching entries because multiple projects may share the same
* external plugin (same package/path), and all their configs need to be updated
* with the actual manifest.name.
*/
function findExternalPluginEntry(
function findAllExternalPluginEntries(
plugin: InstalledPluginConfig,
externalEntries: ExternalPluginEntryRef[] | undefined,
): ExternalPluginEntryRef | undefined {
if (!externalEntries) return undefined;
): ExternalPluginEntryRef[] {
if (!externalEntries) return [];
return externalEntries.find((entry) => {
return externalEntries.filter((entry) => {
if (plugin.package && entry.package === plugin.package) return true;
if (plugin.path && entry.path === plugin.path) return true;
return false;
@ -153,9 +157,9 @@ function updateConfigWithManifestName(
// Also validate slot matches
if (manifest.slot !== slot) {
console.warn(
process.stderr.write(
`[plugin-registry] Plugin at ${source} has slot "${manifest.slot}" but was configured as "${slot}". ` +
`The plugin will be registered under its declared slot "${manifest.slot}".`,
`The plugin will be registered under its declared slot "${manifest.slot}".\n`,
);
}
}
@ -344,7 +348,7 @@ export function createPluginRegistry(): PluginRegistry {
const specifier = resolvePluginSpecifier(plugin, config);
if (!specifier) {
console.warn(`[plugin-registry] Could not resolve specifier for plugin "${plugin.name}" (source: ${plugin.source})`);
process.stderr.write(`[plugin-registry] Could not resolve specifier for plugin "${plugin.name}" (source: ${plugin.source})\n`);
continue;
}
@ -353,8 +357,9 @@ export function createPluginRegistry(): PluginRegistry {
if (!mod) continue;
// Check if this plugin was auto-added from inline tracker/scm/notifier config
const externalEntry = findExternalPluginEntry(plugin, externalEntries);
if (externalEntry) {
// 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
@ -364,7 +369,7 @@ export function createPluginRegistry(): PluginRegistry {
const pluginConfig = extractPluginConfig(mod.manifest.slot, mod.manifest.name, config);
this.register(mod, pluginConfig);
} catch (error) {
console.warn(`[plugin-registry] Failed to load plugin "${specifier}":`, error);
process.stderr.write(`[plugin-registry] Failed to load plugin "${specifier}": ${error}\n`);
}
}
},