performed review and fixed

This commit is contained in:
harshitsinghbhandari 2026-04-03 01:23:37 +05:30 committed by Gaurav Bhola
parent 23cb1ee7a8
commit ee7f08ada7
4 changed files with 124 additions and 98 deletions

View File

@ -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)`);
}
}

View File

@ -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

View File

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

View File

@ -92,7 +92,20 @@ function extractPluginConfig(
if (matches) {
const sourceId = `projects.${projectId}.${slot}`;
return prepareConfig(slot, name, sourceId, entry as Record<string, unknown>, config.configPath);
const prepared = prepareConfig(
slot,
name,
sourceId,
entry as Record<string, unknown>,
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<string, ExternalPluginEntryRef[]> {
const index = new Map<string, ExternalPluginEntryRef[]>();
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<string, ExternalPluginEntryRef[]>,
): 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)