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 <noreply@anthropic.com>
This commit is contained in:
harshitsinghbhandari
parent
ddba72a485
commit
32075b879d
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<string, unknown>)["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<string, unknown>,
|
||||
config.configPath,
|
||||
);
|
||||
}
|
||||
}
|
||||
return undefined;
|
||||
}
|
||||
|
||||
return undefined;
|
||||
|
|
|
|||
|
|
@ -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") */
|
||||
|
|
|
|||
Loading…
Reference in New Issue