diff --git a/packages/cli/src/commands/doctor.ts b/packages/cli/src/commands/doctor.ts index 5bdaa0dd9..9edd697b9 100644 --- a/packages/cli/src/commands/doctor.ts +++ b/packages/cli/src/commands/doctor.ts @@ -345,6 +345,9 @@ async function sendTestNotifications( for (const [name, notifierConfig] of configuredNotifiers) { if (notifierConfig.plugin) { targets.set(notifierConfig.plugin, { label: name, pluginName: notifierConfig.plugin }); + } else { + // External plugin without explicit plugin name - manifest.name not yet resolved + warn(`${name}: notifier plugin name not resolved (external plugin may not be loaded yet)`); } } diff --git a/packages/core/src/__tests__/plugin-registry.test.ts b/packages/core/src/__tests__/plugin-registry.test.ts index 6cf20e6d7..a297d0c78 100644 --- a/packages/core/src/__tests__/plugin-registry.test.ts +++ b/packages/core/src/__tests__/plugin-registry.test.ts @@ -312,7 +312,7 @@ describe("loadBuiltins", () => { return null; }); - expect(stderrSpy).toHaveBeenCalledWith(expect.stringContaining('path" is reserved')); + expect(stderrSpy).toHaveBeenCalledWith(expect.stringContaining('"path" field conflicts with reserved')); stderrSpy.mockRestore(); // Plugin should not be registered due to config error diff --git a/packages/core/src/config.ts b/packages/core/src/config.ts index d2ff74414..9fed53af8 100644 --- a/packages/core/src/config.ts +++ b/packages/core/src/config.ts @@ -317,23 +317,19 @@ function expandPaths(config: OrchestratorConfig): OrchestratorConfig { * e.g., "my-tracker" (local path without slashes) -> "my-tracker" */ function generateTempPluginName(pkg?: string, path?: string): string { - // Handle npm packages: extract the plugin name after the slot prefix - // @acme/ao-plugin-tracker-jira -> jira - // @acme/ao-plugin-tracker-jira-cloud -> jira-cloud (preserves multi-word names) - // @composio/ao-plugin-scm-gitlab -> gitlab if (pkg) { - // First get the package name without scope: "@acme/ao-plugin-tracker-jira" -> "ao-plugin-tracker-jira" + // Extract package name without scope: "@acme/ao-plugin-tracker-jira" -> "ao-plugin-tracker-jira" const slashParts = pkg.split("/"); const packageName = slashParts[slashParts.length - 1] ?? pkg; - // Try to extract name after common prefix pattern: ao-plugin-{slot}-{name} - // This preserves multi-word names like "jira-cloud" + // Extract plugin name after ao-plugin-{slot}- prefix, preserving multi-word names like "jira-cloud" const prefixMatch = packageName.match(/^ao-plugin-(?:runtime|agent|workspace|tracker|scm|notifier|terminal)-(.+)$/); if (prefixMatch?.[1]) { return prefixMatch[1]; } - // Fallback: split by hyphens and take last segment + // Non-standard package name (doesn't follow ao-plugin convention): use last hyphen-segment + // e.g., "custom-tracker-plugin" -> "plugin" const parts = packageName.split("-"); return parts[parts.length - 1] ?? packageName; } @@ -349,6 +345,41 @@ function generateTempPluginName(pkg?: string, path?: string): string { return "unknown"; } +/** + * Helper to process a single external plugin config entry. + * Expands home paths, generates temp plugin name if needed, and returns the entry ref. + */ +function processExternalPluginConfig( + pluginConfig: { plugin?: string; package?: string; path?: string }, + source: string, + location: ExternalPluginEntryRef["location"], + slot: ExternalPluginEntryRef["slot"], +): ExternalPluginEntryRef | null { + if (!pluginConfig.package && !pluginConfig.path) return null; + + // Expand home paths (~/...) for consistency with config.plugins + if (pluginConfig.path) { + pluginConfig.path = expandHome(pluginConfig.path); + } + + // Track if user explicitly specified plugin name (for validation) + const userSpecifiedPlugin = pluginConfig.plugin; + + // If plugin name not specified, generate a temporary one from package/path + if (!pluginConfig.plugin) { + pluginConfig.plugin = generateTempPluginName(pluginConfig.package, pluginConfig.path); + } + + return { + source, + location, + slot, + package: pluginConfig.package, + path: pluginConfig.path, + expectedPluginName: userSpecifiedPlugin, + }; +} + /** * Collect external plugin configs from tracker, scm, and notifier inline configs. * These will be auto-added to config.plugins for loading. @@ -363,87 +394,39 @@ function generateTempPluginName(pkg?: string, path?: string): string { export function collectExternalPluginConfigs(config: OrchestratorConfig): ExternalPluginEntryRef[] { const entries: ExternalPluginEntryRef[] = []; - // Collect from project tracker configs + // Collect from project tracker and scm configs for (const [projectId, project] of Object.entries(config.projects)) { - if (project.tracker && (project.tracker.package || project.tracker.path)) { - // Expand home paths (~/...) for consistency with config.plugins - if (project.tracker.path) { - project.tracker.path = expandHome(project.tracker.path); - } - - // Track if user explicitly specified plugin name (for validation) - const userSpecifiedPlugin = project.tracker.plugin; - - // If plugin name not specified, generate a temporary one from package/path - if (!project.tracker.plugin) { - project.tracker.plugin = generateTempPluginName( - project.tracker.package, - project.tracker.path, - ); - } - entries.push({ - source: `projects.${projectId}.tracker`, - location: { kind: "project", projectId, configType: "tracker" }, - slot: "tracker", - package: project.tracker.package, - path: project.tracker.path, - // Only validate manifest.name when user explicitly specified plugin - expectedPluginName: userSpecifiedPlugin, - }); + if (project.tracker) { + const entry = processExternalPluginConfig( + project.tracker, + `projects.${projectId}.tracker`, + { kind: "project", projectId, configType: "tracker" }, + "tracker", + ); + if (entry) entries.push(entry); } - if (project.scm && (project.scm.package || project.scm.path)) { - // Expand home paths (~/...) for consistency with config.plugins - if (project.scm.path) { - project.scm.path = expandHome(project.scm.path); - } - - // Track if user explicitly specified plugin name (for validation) - const userSpecifiedPlugin = project.scm.plugin; - - // If plugin name not specified, generate a temporary one from package/path - if (!project.scm.plugin) { - project.scm.plugin = generateTempPluginName(project.scm.package, project.scm.path); - } - entries.push({ - source: `projects.${projectId}.scm`, - location: { kind: "project", projectId, configType: "scm" }, - slot: "scm", - package: project.scm.package, - path: project.scm.path, - // Only validate manifest.name when user explicitly specified plugin - expectedPluginName: userSpecifiedPlugin, - }); + if (project.scm) { + const entry = processExternalPluginConfig( + project.scm, + `projects.${projectId}.scm`, + { kind: "project", projectId, configType: "scm" }, + "scm", + ); + if (entry) entries.push(entry); } } // Collect from global notifier configs for (const [notifierId, notifierConfig] of Object.entries(config.notifiers ?? {})) { - if (notifierConfig && (notifierConfig.package || notifierConfig.path)) { - // Expand home paths (~/...) for consistency with config.plugins - if (notifierConfig.path) { - notifierConfig.path = expandHome(notifierConfig.path); - } - - // Track if user explicitly specified plugin name (for validation) - const userSpecifiedPlugin = notifierConfig.plugin; - - // If plugin name not specified, generate a temporary one from package/path - if (!notifierConfig.plugin) { - notifierConfig.plugin = generateTempPluginName( - notifierConfig.package, - notifierConfig.path, - ); - } - entries.push({ - source: `notifiers.${notifierId}`, - location: { kind: "notifier", notifierId }, - slot: "notifier", - package: notifierConfig.package, - path: notifierConfig.path, - // Only validate manifest.name when user explicitly specified plugin - expectedPluginName: userSpecifiedPlugin, - }); + if (notifierConfig) { + const entry = processExternalPluginConfig( + notifierConfig, + `notifiers.${notifierId}`, + { kind: "notifier", notifierId }, + "notifier", + ); + if (entry) entries.push(entry); } } diff --git a/packages/core/src/plugin-registry.ts b/packages/core/src/plugin-registry.ts index 84de57a16..44f243481 100644 --- a/packages/core/src/plugin-registry.ts +++ b/packages/core/src/plugin-registry.ts @@ -92,7 +92,20 @@ function extractPluginConfig( if (matches) { const sourceId = `projects.${projectId}.${slot}`; - return prepareConfig(slot, name, sourceId, entry as Record, config.configPath); + const prepared = prepareConfig( + slot, + name, + sourceId, + entry as Record, + config.configPath, + ); + + // Tracker and SCM plugins are typically stateless in their global instance. + // To maintain legacy behavior and prevent accidental project-specific leakage + // into singleton built-ins, we return undefined for built-in trackers/scms. + // External plugins (not in BUILTIN_PLUGINS) still receive their cleaned config. + const isBuiltin = BUILTIN_PLUGINS.some((b) => b.slot === slot && b.name === name); + return isBuiltin ? undefined : prepared; } } } @@ -102,6 +115,7 @@ function extractPluginConfig( /** * Internal helper to validate and strip loading metadata from a plugin configuration. + * Reserved fields (plugin, package, path) are used for plugin resolution and stripped. */ function prepareConfig( slot: string, @@ -119,14 +133,16 @@ function prepareConfig( ); } - // If loading via built-in name or npm package, 'path' is a collision. - // We detect this by checking if 'package' is present OR if no 'path' was - // intended for local resolution (i.e. name refers to a built-in). + // If loading via built-in name or npm package, having a 'path' field is ambiguous: + // it could be a local plugin path (for loading) or a plugin config value. + // We reject this to avoid silently stripping a config value the user intended to pass. const isBuiltin = BUILTIN_PLUGINS.some((b) => b.slot === slot && b.name === name); if ((rawConfig.package || isBuiltin) && "path" in rawConfig) { + const loadingMethod = rawConfig.package ? `npm package "${rawConfig.package}"` : `built-in plugin "${name}"`; throw new Error( - `In ${slot} "${sourceId}": "path" is reserved for plugin loading. ` + - `Rename your configuration field to something else (e.g., "apiPath").`, + `In ${slot} "${sourceId}": "path" field conflicts with reserved plugin loading field. ` + + `You're loading via ${loadingMethod}, but also have a "path" field which would be stripped. ` + + `Rename your configuration field to something else (e.g., "apiPath", "webhookPath").`, ); } @@ -135,6 +151,29 @@ function prepareConfig( return configPath ? { ...rest, configPath } : rest; } +/** + * Build an index of external plugin entries by package/path for O(1) lookups. + * Multiple entries can share the same package/path (e.g., multiple projects using same plugin). + */ +function buildExternalPluginIndex( + externalEntries: ExternalPluginEntryRef[] | undefined, +): Map { + const index = new Map(); + if (!externalEntries) return index; + + for (const entry of externalEntries) { + const key = entry.package ? `package:${entry.package}` : `path:${entry.path}`; + const existing = index.get(key); + if (existing) { + existing.push(entry); + } else { + index.set(key, [entry]); + } + } + + return index; +} + /** * Find ALL external plugin entries that match a given plugin config. * Used for manifest.name validation when loading inline tracker/scm/notifier plugins. @@ -145,15 +184,15 @@ function prepareConfig( */ function findAllExternalPluginEntries( plugin: InstalledPluginConfig, - externalEntries: ExternalPluginEntryRef[] | undefined, + externalIndex: Map, ): ExternalPluginEntryRef[] { - if (!externalEntries) return []; - - return externalEntries.filter((entry) => { - if (plugin.package && entry.package === plugin.package) return true; - if (plugin.path && entry.path === plugin.path) return true; - return false; - }); + if (plugin.package) { + return externalIndex.get(`package:${plugin.package}`) ?? []; + } + if (plugin.path) { + return externalIndex.get(`path:${plugin.path}`) ?? []; + } + return []; } /** @@ -400,7 +439,8 @@ export function createPluginRegistry(): PluginRegistry { await this.loadBuiltins(config, importFn); const doImport = importFn ?? ((pkg: string) => import(pkg)); - const externalEntries = config._externalPluginEntries; + // Build index once for O(1) lookups when matching plugins to external entries + const externalIndex = buildExternalPluginIndex(config._externalPluginEntries); for (const plugin of config.plugins ?? []) { if (plugin.enabled === false) continue; @@ -419,7 +459,7 @@ export function createPluginRegistry(): PluginRegistry { // Multiple projects may share the same external plugin, so find ALL matching entries. // We validate and update configs FIRST, before extracting plugin config, because // extractPluginConfig looks up by manifest.name which may differ from the temp name. - const matchingEntries = findAllExternalPluginEntries(plugin, externalEntries); + const matchingEntries = findAllExternalPluginEntries(plugin, externalIndex); for (const externalEntry of matchingEntries) { try { // Validate manifest.name matches expectedPluginName (if specified)