From 32075b879d85fdd96d58b823da5b36d1560cd7a5 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Fri, 3 Apr 2026 22:09:24 +0530 Subject: [PATCH] fix: address illegalcall review comments - Fix bad temp name for non-standard packages: use full packageName instead of last hyphen-segment to avoid collisions between packages like "custom-tracker-plugin" and "my-custom-plugin" - Fix disabled top-level plugin blocking inline loading: when an existing plugin entry has `enabled: false`, it's now set to `true` when there's an inline tracker/scm/notifier reference for the same package/path - Create validatePluginConfigFields helper to deduplicate the TrackerConfigSchema, SCMConfigSchema, and NotifierConfigSchema validation logic (was duplicated thrice) - Fix multi-project external plugin order-dependency: tracker and SCM plugins now receive no project-level config at create() time, making them consistent with built-in plugins. Project-specific config is passed per-call via ProjectConfig argument, avoiding first-project wins behavior - Document post-validation invariant for plugin field: added documentation clarifying that `plugin` is always populated after validateConfig() to help downstream consumers understand they can safely assume non-null after validation Co-Authored-By: Claude Opus 4.5 --- packages/core/src/config.ts | 100 ++++++++++++--------------- packages/core/src/plugin-registry.ts | 35 ++-------- packages/core/src/types.ts | 15 ++++ 3 files changed, 67 insertions(+), 83 deletions(-) diff --git a/packages/core/src/config.ts b/packages/core/src/config.ts index 9fed53af8..0da3db105 100644 --- a/packages/core/src/config.ts +++ b/packages/core/src/config.ts @@ -50,6 +50,32 @@ function inferScmPlugin(project: { // ZOD SCHEMAS // ============================================================================= +/** + * Common validation for plugin config fields (tracker, scm, notifier). + * Must have either plugin (for built-ins) or package/path (for external plugins). + * Cannot have both package and path. + */ +function validatePluginConfigFields( + value: { plugin?: string; package?: string; path?: string }, + ctx: z.RefinementCtx, + configType: string, +): void { + // Must have either plugin or package/path + if (!value.plugin && !value.package && !value.path) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `${configType} config requires either 'plugin' (for built-ins) or 'package'/'path' (for external plugins)`, + }); + } + // Cannot have both package and path + if (value.package && value.path) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `${configType} config cannot have both 'package' and 'path' - use one or the other`, + }); + } +} + const ReactionConfigSchema = z.object({ auto: z.boolean().default(true), action: z.enum(["send-to-agent", "notify", "auto-merge"]).default("notify"), @@ -68,22 +94,7 @@ const TrackerConfigSchema = z path: z.string().optional(), }) .passthrough() - .superRefine((value, ctx) => { - // Must have either plugin or package/path - if (!value.plugin && !value.package && !value.path) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - message: "Tracker config requires either 'plugin' (for built-ins) or 'package'/'path' (for external plugins)", - }); - } - // Cannot have both package and path - if (value.package && value.path) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - message: "Tracker config cannot have both 'package' and 'path' - use one or the other", - }); - } - }); + .superRefine((value, ctx) => validatePluginConfigFields(value, ctx, "Tracker")); const SCMConfigSchema = z .object({ @@ -103,22 +114,7 @@ const SCMConfigSchema = z .optional(), }) .passthrough() - .superRefine((value, ctx) => { - // Must have either plugin or package/path - if (!value.plugin && !value.package && !value.path) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - message: "SCM config requires either 'plugin' (for built-ins) or 'package'/'path' (for external plugins)", - }); - } - // Cannot have both package and path - if (value.package && value.path) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - message: "SCM config cannot have both 'package' and 'path' - use one or the other", - }); - } - }); + .superRefine((value, ctx) => validatePluginConfigFields(value, ctx, "SCM")); const NotifierConfigSchema = z .object({ @@ -127,22 +123,7 @@ const NotifierConfigSchema = z path: z.string().optional(), }) .passthrough() - .superRefine((value, ctx) => { - // Must have either plugin or package/path - if (!value.plugin && !value.package && !value.path) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - message: "Notifier config requires either 'plugin' (for built-ins) or 'package'/'path' (for external plugins)", - }); - } - // Cannot have both package and path - if (value.package && value.path) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - message: "Notifier config cannot have both 'package' and 'path' - use one or the other", - }); - } - }); + .superRefine((value, ctx) => validatePluginConfigFields(value, ctx, "Notifier")); const AgentPermissionSchema = z .enum(["permissionless", "default", "auto-edit", "suggest", "skip"]) @@ -328,10 +309,10 @@ function generateTempPluginName(pkg?: string, path?: string): string { return prefixMatch[1]; } - // 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; + // Non-standard package name (doesn't follow ao-plugin convention): use the full package name + // to avoid collisions. "plugin" from "custom-tracker-plugin" would collide with other packages + // that also end in "-plugin". The temp name is replaced with manifest.name after loading anyway. + return packageName; } // Handle local paths: use the basename @@ -450,10 +431,21 @@ function mergeExternalPlugins( if (plugin.path) seen.add(`path:${plugin.path}`); } - // Add external entries that aren't already present + // Add external entries that aren't already present, or enable if disabled for (const entry of externalEntries) { const key = entry.package ? `package:${entry.package}` : `path:${entry.path}`; - if (seen.has(key)) continue; + if (seen.has(key)) { + // If the existing plugin is disabled but there's an inline reference, enable it + const existingPlugin = plugins.find( + (p) => + (entry.package && p.package === entry.package) || + (entry.path && p.path === entry.path), + ); + if (existingPlugin && existingPlugin.enabled === false) { + existingPlugin.enabled = true; + } + continue; + } seen.add(key); // Generate a temporary name - will be replaced with manifest.name during loading diff --git a/packages/core/src/plugin-registry.ts b/packages/core/src/plugin-registry.ts index c41d8bd92..e05e395ff 100644 --- a/packages/core/src/plugin-registry.ts +++ b/packages/core/src/plugin-registry.ts @@ -81,36 +81,13 @@ function extractPluginConfig( } // 2. Handle Tracker and SCM Slots (Project-level) + // Tracker and SCM plugins are typically stateless singletons that receive + // project-specific config per-call (via ProjectConfig argument), not at create() time. + // This applies to BOTH built-in and external plugins to avoid order-dependent + // behavior when multiple projects share the same plugin but have different configs. + // Return undefined so plugins are initialized without project-specific config. if (slot === "tracker" || slot === "scm") { - for (const [projectId, project] of Object.entries(config.projects)) { - const entry = slot === "tracker" ? project.tracker : project.scm; - if (!entry || typeof entry !== "object") continue; - - const configuredPlugin = (entry as Record)["plugin"]; - const hasExplicitPlugin = typeof configuredPlugin === "string" && configuredPlugin.length > 0; - const matches = hasExplicitPlugin ? configuredPlugin === name : false; - - if (matches) { - // 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. - // Check isBuiltin BEFORE prepareConfig to avoid unnecessary validation errors. - const isBuiltin = BUILTIN_PLUGINS.some((b) => b.slot === slot && b.name === name); - if (isBuiltin) { - return undefined; - } - - // External plugins (not in BUILTIN_PLUGINS) receive their cleaned config. - const sourceId = `projects.${projectId}.${slot}`; - return prepareConfig( - slot, - name, - sourceId, - entry as Record, - config.configPath, - ); - } - } + return undefined; } return undefined; diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 300397e0f..f42ec1c63 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -1176,6 +1176,11 @@ export interface TrackerConfig { * Plugin name (manifest.name). Required when using built-in plugins. * Optional when `package` or `path` is specified (will be inferred from manifest). * When both plugin and package/path are specified, manifest.name must match plugin. + * + * POST-VALIDATION INVARIANT: After validateConfig(), this field is ALWAYS populated. + * Either from user input, inferred from repo (github/gitlab), or auto-generated from + * package/path via generateTempPluginName(). The optional typing exists for raw config + * input before validation. Downstream code can safely assume non-null after validation. */ plugin?: string; /** npm package name for external plugins (e.g. "@acme/ao-plugin-tracker-jira") */ @@ -1191,6 +1196,11 @@ export interface SCMConfig { * Plugin name (manifest.name). Required when using built-in plugins. * Optional when `package` or `path` is specified (will be inferred from manifest). * When both plugin and package/path are specified, manifest.name must match plugin. + * + * POST-VALIDATION INVARIANT: After validateConfig(), this field is ALWAYS populated. + * Either from user input, inferred from repo (github/gitlab), or auto-generated from + * package/path via generateTempPluginName(). The optional typing exists for raw config + * input before validation. Downstream code can safely assume non-null after validation. */ plugin?: string; /** npm package name for external plugins (e.g. "@acme/ao-plugin-scm-bitbucket") */ @@ -1216,6 +1226,11 @@ export interface NotifierConfig { * Plugin name (manifest.name). Required when using built-in plugins. * Optional when `package` or `path` is specified (will be inferred from manifest). * When both plugin and package/path are specified, manifest.name must match plugin. + * + * POST-VALIDATION INVARIANT: After validateConfig(), this field is ALWAYS populated. + * Either from user input or auto-generated from package/path via generateTempPluginName(). + * The optional typing exists for raw config input before validation. + * Downstream code can safely assume non-null after validation. */ plugin?: string; /** npm package name for external plugins (e.g. "@acme/ao-plugin-notifier-teams") */