From a0db530c59c3d9b00cde3ce47781e55189549041 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Wed, 16 Sep 2026 16:10:20 -0700 Subject: [PATCH] refactor(plugins-mcp): dedupe collectors, guards, and register loops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: zero-behavior-change merges in src/plugins, src/mcp, src/web — one generic plugin-candidate collector, one COMMAND_NAME_PATTERN export, one lane-local isENOENT, warning-resolver delegation to diagnostics helper, one shared register loop, inlined segment matcher. isPluginEnabled kept: tests/unit/plugin-register.test.ts imports it. Verification: bun run typecheck clean; bun test ./src/plugins ./src/mcp ./src/web --randomize --seed 424242 → 659 pass 0 fail (identical to pristine-worktree baseline); bun run check EXIT=0 (full suite 7755 pass 0 fail). --- src/mcp/tool-name.ts | 13 ++---- src/plugins/agent-plugins.ts | 7 +-- src/plugins/data-only-commands.ts | 4 +- src/plugins/data-only.ts | 2 +- src/plugins/frontmatter.ts | 5 ++ src/plugins/loader.ts | 78 ++++++++++++++++++++----------- src/plugins/register.ts | 34 +++++++++----- src/plugins/skill-commands.ts | 4 +- src/plugins/tool-plugins.ts | 21 ++------- src/web/plugin-provider.ts | 20 ++------ 10 files changed, 99 insertions(+), 89 deletions(-) diff --git a/src/mcp/tool-name.ts b/src/mcp/tool-name.ts index f2f333d81..ba1907452 100644 --- a/src/mcp/tool-name.ts +++ b/src/mcp/tool-name.ts @@ -113,19 +113,14 @@ const MCP_READ_ONLY_TOOL_PREFIXES = [ "fetch_", ] as const; -function mcpToolSegmentMatchesPrefix( - segment: string, - prefixes: readonly string[], -): boolean { - return prefixes.some((prefix) => segment.startsWith(prefix)); -} - // Name-prefix fallback when a server omits ToolAnnotations on tools/list. export function isReadOnlyMcpTool(name: string): boolean { const parsed = parseMcpToolName(name); if (parsed === null) return false; const segment = parsed.tool; - if (mcpToolSegmentMatchesPrefix(segment, MCP_MUTATING_TOOL_PREFIXES)) + if (MCP_MUTATING_TOOL_PREFIXES.some((prefix) => segment.startsWith(prefix))) return false; - return mcpToolSegmentMatchesPrefix(segment, MCP_READ_ONLY_TOOL_PREFIXES); + return MCP_READ_ONLY_TOOL_PREFIXES.some((prefix) => + segment.startsWith(prefix), + ); } diff --git a/src/plugins/agent-plugins.ts b/src/plugins/agent-plugins.ts index cc912fa75..bde722ea1 100644 --- a/src/plugins/agent-plugins.ts +++ b/src/plugins/agent-plugins.ts @@ -7,7 +7,7 @@ import type { PluginModule } from "./loader.js"; import type { PluginConfig } from "../config/settings.js"; import { isPluginModuleEnabled } from "./register.js"; import { - pluginWarningSink, + resolvePluginWarningHandler, type PluginLoadDiagnostics, } from "./diagnostics.js"; import { type } from "arktype"; @@ -27,8 +27,9 @@ function resolveAgentProfileWarningHandler( ): (msg: string) => void { if (typeof opts === "function") return opts; if (opts.diagnostics !== undefined) - return pluginWarningSink(opts.diagnostics); - if (opts.onWarning !== undefined) return opts.onWarning; + return resolvePluginWarningHandler({ diagnostics: opts.diagnostics }); + if (opts.onWarning !== undefined) + return resolvePluginWarningHandler({ onWarning: opts.onWarning }); return () => undefined; } diff --git a/src/plugins/data-only-commands.ts b/src/plugins/data-only-commands.ts index a565516e9..e41a9745c 100644 --- a/src/plugins/data-only-commands.ts +++ b/src/plugins/data-only-commands.ts @@ -7,7 +7,7 @@ import { type CommandResult, type SubcommandDefinition, } from "../tui/commands/registry.js"; -import { splitFrontmatter } from "./frontmatter.js"; +import { COMMAND_NAME_PATTERN, splitFrontmatter } from "./frontmatter.js"; // A data-only command plugin declares its slash commands as markdown files, the // same convention Claude Code (`.claude/commands/`), OpenCode @@ -25,8 +25,6 @@ import { splitFrontmatter } from "./frontmatter.js"; // replaced with the args passed to that command level (all args for a flat // command; the args after the subcommand for a namespaced one). -const COMMAND_NAME_PATTERN = /^[a-z0-9]+(-[a-z0-9]+)*$/; - interface LoadedBody { description: string; body: string; diff --git a/src/plugins/data-only.ts b/src/plugins/data-only.ts index d6fb06844..fdfd9bdac 100644 --- a/src/plugins/data-only.ts +++ b/src/plugins/data-only.ts @@ -26,7 +26,7 @@ export interface DataOnlyPlugin { commandPlugin?: CommandPlugin; } -function isENOENT(err: unknown): boolean { +export function isENOENT(err: unknown): boolean { return ( typeof err === "object" && err !== null && diff --git a/src/plugins/frontmatter.ts b/src/plugins/frontmatter.ts index 0a13c7db7..ac7ff6104 100644 --- a/src/plugins/frontmatter.ts +++ b/src/plugins/frontmatter.ts @@ -9,6 +9,11 @@ export interface ParsedMarkdown { body: string; } +// Slash-command names are kebab-case identifiers. Data-only command files and +// skill commands validate the same shape, so the pattern lives here once — +// both modules already import this file for splitFrontmatter. +export const COMMAND_NAME_PATTERN = /^[a-z0-9]+(-[a-z0-9]+)*$/; + // Strip a leading `---\n...\n---` YAML block. Returns { frontmatter, body }. // No frontmatter block -> empty-object frontmatter (an agent/command can // legitimately have none). A present-but-malformed block -> null frontmatter diff --git a/src/plugins/loader.ts b/src/plugins/loader.ts index f2089f0fc..312b49cb5 100644 --- a/src/plugins/loader.ts +++ b/src/plugins/loader.ts @@ -12,12 +12,14 @@ import { pathIsInsideOrEqual } from "../util/path-contain.js"; import { parsePluginManifest, PluginManifestSchema, + type PluginCredentialField, + type PluginKind, type PluginManifest, } from "./manifest.js"; import { NOOP_TELEMETRY, type Telemetry } from "../telemetry/index.js"; import type { PluginLoadReporter } from "../telemetry/product-events.js"; import { runtimePluginLoadReporter } from "../telemetry/singleton.js"; -import { loadDataOnlyPlugin } from "./data-only.js"; +import { isENOENT, loadDataOnlyPlugin } from "./data-only.js"; import { resolvePluginWarningHandler, @@ -76,19 +78,46 @@ export interface PluginModule { shadowedRepoDefaultEnabled?: boolean; } -function isENOENT(err: unknown): boolean { - return ( - typeof err === "object" && - err !== null && - "code" in err && - (err as { code?: unknown }).code === "ENOENT" - ); -} - function errorText(err: unknown): string { return err instanceof Error ? err.message : String(err); } +// Flat candidate behind the tool-plugin and web-provider collectors: both +// select modules by manifest kind + factory key and project the same shape, +// so the per-kind entry points stay one-line typed wrappers. +export interface CollectedPluginCandidate { + id: string; + name: string; + description?: string; + credentials: PluginCredentialField[]; + factory: (options: unknown) => TFactory | Promise; +} + +export function collectPluginCandidates( + modules: PluginModule[], + opts: { + kind: PluginKind; + factoryKey: "createToolPlugin" | "createWebProvider"; + }, +): CollectedPluginCandidate[] { + const out: CollectedPluginCandidate[] = []; + for (const mod of modules) { + if (mod.manifest?.kind !== opts.kind) continue; + const factory = mod[opts.factoryKey]; + if (typeof factory !== "function") continue; + out.push({ + id: mod.manifest.id, + name: mod.manifest.name, + ...(mod.manifest.description !== undefined + ? { description: mod.manifest.description } + : {}), + credentials: mod.manifest.credentials ?? [], + factory: factory as CollectedPluginCandidate["factory"], + }); + } + return out; +} + // Read and validate a manifest.json beside the module. Plugins may declare // their manifest as a JS export (mod.manifest) or a sibling manifest.json file; // this covers the JSON path so plugins that are pure data + commands work too. @@ -427,23 +456,16 @@ export function expandSkipDiagnosticsHandler( return (skip) => diagnostics.warnings.push(formatExpandSkip(skip)); } -/** - * `onSkip` when no diagnostics collector is in play: one explicit stderr - * line, module-private and only reached by an internal caller's own - * deliberate choice (see `resolveExpandSkip` below) — never `expandPluginPath` - * falling back to it on its own. - */ -function stderrExpandSkip(skip: ExpandPluginPathSkip): void { - process.stderr.write(`plugins: ${formatExpandSkip(skip)}\n`); -} - /** Diagnostics when given, else the explicit stderr line — no silent option. */ function resolveExpandSkip( diagnostics?: PluginLoadDiagnostics, ): (skip: ExpandPluginPathSkip) => void { - return diagnostics !== undefined - ? expandSkipDiagnosticsHandler(diagnostics) - : stderrExpandSkip; + const onWarning = resolvePluginWarningHandler( + diagnostics !== undefined + ? { diagnostics } + : { onWarning: stderrPluginWarning }, + ); + return (skip) => onWarning(formatExpandSkip(skip)); } /** @@ -928,11 +950,11 @@ export async function discoverClaudeInstalledPlugins( parsed = JSON.parse(raw); } catch { // Prefer collector when present; else stderrPluginWarning. - resolvePluginWarningHandler( - opts.diagnostics !== undefined - ? { diagnostics: opts.diagnostics } - : { onWarning: stderrPluginWarning }, - )(`failed to parse ${registryPath}`); + (opts.diagnostics !== undefined + ? resolvePluginWarningHandler({ diagnostics: opts.diagnostics }) + : resolvePluginWarningHandler({ onWarning: stderrPluginWarning }))( + `failed to parse ${registryPath}`, + ); return []; } if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { diff --git a/src/plugins/register.ts b/src/plugins/register.ts index eeb18af6e..d7ee9818c 100644 --- a/src/plugins/register.ts +++ b/src/plugins/register.ts @@ -89,28 +89,40 @@ export function registerCommandPlugins( config: Record | (() => Record), ): string[] { const getConfig = typeof config === "function" ? config : () => config; - const registered: string[] = []; - for (const mod of modules) { + return collectEnabledModules(modules, (mod) => { const id = mod.manifest?.id; if (id === undefined || !registerCommandPluginModule(mod, getConfig)) - continue; - if (isPluginModuleEnabled(mod, getConfig())) registered.push(id); - } - return registered; + return undefined; + return isPluginModuleEnabled(mod, getConfig()) ? id : undefined; + }); } export function registerWorkflowPlugins( modules: PluginModule[], config: Record, ): string[] { - const registered: string[] = []; - for (const mod of modules) { - if (!isEnabledWorkflowPlugin(mod, config)) continue; + return collectEnabledModules(modules, (mod) => { + if (!isEnabledWorkflowPlugin(mod, config)) return undefined; const workflowPlugin = mod.workflowPlugin; const id = mod.manifest?.id; - if (workflowPlugin === undefined || id === undefined) continue; + if (workflowPlugin === undefined || id === undefined) return undefined; registerWorkflowPlugin(workflowPlugin); - registered.push(id); + return id; + }); +} + +// Shared register loop: each module is offered to `collect`, which performs +// that kind's registration side effects and returns the manifest id exactly +// when the module counts as registered, or undefined to skip it. One pass in +// module order; per-kind predicates stay at the call sites above. +function collectEnabledModules( + modules: PluginModule[], + collect: (mod: PluginModule) => string | undefined, +): string[] { + const registered: string[] = []; + for (const mod of modules) { + const id = collect(mod); + if (id !== undefined) registered.push(id); } return registered; } diff --git a/src/plugins/skill-commands.ts b/src/plugins/skill-commands.ts index 80b52d92b..e9fc84462 100644 --- a/src/plugins/skill-commands.ts +++ b/src/plugins/skill-commands.ts @@ -5,7 +5,7 @@ import { type CommandDefinition, type CommandResult, } from "../tui/commands/registry.js"; -import { splitFrontmatter } from "./frontmatter.js"; +import { COMMAND_NAME_PATTERN, splitFrontmatter } from "./frontmatter.js"; // Slash is the operator action surface: `/ [args]` sends the skill // body (plus args) to the agent. Convention/internal skills opt out with @@ -15,8 +15,6 @@ import { splitFrontmatter } from "./frontmatter.js"; // skips the skill from `discoverSkills` lazy listing. Explicit `use_skill` / // `resolveSkillBody` still loads the body by name. -const COMMAND_NAME_PATTERN = /^[a-z0-9]+(-[a-z0-9]+)*$/; - // `$ARGUMENTS` (Claude Code convention) interpolates inline when the author used // it; otherwise args append after the body so the skill instructions run against // the user's target (e.g. an issue id). diff --git a/src/plugins/tool-plugins.ts b/src/plugins/tool-plugins.ts index 0db7d60c6..1c8fab316 100644 --- a/src/plugins/tool-plugins.ts +++ b/src/plugins/tool-plugins.ts @@ -1,5 +1,5 @@ import type { ToolPlugin } from "@intx/tools-posix"; -import type { PluginModule } from "./loader.js"; +import { collectPluginCandidates, type PluginModule } from "./loader.js"; import type { PluginConfig } from "../config/settings.js"; import type { PluginCredentialField } from "./manifest.js"; import { scrubSecrets } from "../web/secret-scrub.js"; @@ -22,21 +22,10 @@ export interface ToolPluginCandidate { export function collectToolPlugins( modules: PluginModule[], ): ToolPluginCandidate[] { - const out: ToolPluginCandidate[] = []; - for (const mod of modules) { - if (mod.manifest?.kind !== "tool") continue; - if (typeof mod.createToolPlugin !== "function") continue; - out.push({ - id: mod.manifest.id, - name: mod.manifest.name, - ...(mod.manifest.description !== undefined - ? { description: mod.manifest.description } - : {}), - credentials: mod.manifest.credentials ?? [], - factory: mod.createToolPlugin as ToolPluginCandidate["factory"], - }); - } - return out; + return collectPluginCandidates(modules, { + kind: "tool", + factoryKey: "createToolPlugin", + }); } // A tool plugin adds in-process agent capabilities, so it is wired in only when diff --git a/src/web/plugin-provider.ts b/src/web/plugin-provider.ts index 78968af56..7d86f971c 100644 --- a/src/web/plugin-provider.ts +++ b/src/web/plugin-provider.ts @@ -1,6 +1,7 @@ import type { WebProvider } from "./types.js"; import { scrubSecrets } from "./secret-scrub.js"; import type { PluginModule } from "../plugins/loader.js"; +import { collectPluginCandidates } from "../plugins/loader.js"; import type { PluginConfig } from "../config/settings.js"; import type { PluginCredentialField } from "../plugins/manifest.js"; @@ -18,21 +19,10 @@ export interface WebPluginCandidate { export function collectWebPlugins( modules: PluginModule[], ): WebPluginCandidate[] { - const out: WebPluginCandidate[] = []; - for (const mod of modules) { - if (mod.manifest?.kind !== "web") continue; - if (typeof mod.createWebProvider !== "function") continue; - out.push({ - id: mod.manifest.id, - name: mod.manifest.name, - ...(mod.manifest.description !== undefined - ? { description: mod.manifest.description } - : {}), - credentials: mod.manifest.credentials ?? [], - factory: mod.createWebProvider as WebPluginCandidate["factory"], - }); - } - return out; + return collectPluginCandidates(modules, { + kind: "web", + factoryKey: "createWebProvider", + }); } // Pick the active web plugin: an explicit `web` override wins; otherwise the