From 3edd1f097b517321bd286585ad79be6623721cc3 Mon Sep 17 00:00:00 2001 From: pikann22 Date: Sun, 9 Aug 2026 16:18:38 +0000 Subject: [PATCH 1/4] feat: implement getToolContext for plugins to enrich core tool responses --- apps/mcp/src/__tests__/plugin-loader.test.ts | 198 ++++++++++++++++++ apps/mcp/src/plugin-loader.ts | 94 ++++++++- apps/mcp/src/server.ts | 43 +++- docs/plugins/mcp-plugin-system.md | 54 +++++ services/api/internal/domain/plugin/entity.go | 6 + 5 files changed, 388 insertions(+), 7 deletions(-) create mode 100644 apps/mcp/src/__tests__/plugin-loader.test.ts diff --git a/apps/mcp/src/__tests__/plugin-loader.test.ts b/apps/mcp/src/__tests__/plugin-loader.test.ts new file mode 100644 index 00000000..41668719 --- /dev/null +++ b/apps/mcp/src/__tests__/plugin-loader.test.ts @@ -0,0 +1,198 @@ +import { describe, expect, it, vi } from "vitest"; +import { PluginRegistry } from "../plugin-loader.js"; +import type { PacaConfig } from "../types/index.js"; + +const config: PacaConfig = { + apiKey: "test-key", + baseURL: "http://localhost:8080", +}; + +// --------------------------------------------------------------------------- +// PluginRegistry.getToolContext +// --------------------------------------------------------------------------- + +describe("PluginRegistry.getToolContext", () => { + it("returns an empty array when no plugin is loaded", async () => { + const registry = new PluginRegistry([]); + const sections = await registry.getToolContext( + "get_task", + { projectId: "p1", taskId: "t1" }, + config, + ); + expect(sections).toEqual([]); + }); + + it("never calls a plugin that didn't declare a hook for this toolId", async () => { + const getToolContext = vi.fn().mockResolvedValue("should not be called"); + const registry = new PluginRegistry([ + { + pluginId: "com.paca.no-hook", + entry: { tools: [], handleToolCall: vi.fn(), getToolContext }, + toolContextHooks: [], // implements the method but never declared it + }, + ]); + const sections = await registry.getToolContext( + "get_task", + { projectId: "p1", taskId: "t1" }, + config, + ); + expect(sections).toEqual([]); + expect(getToolContext).not.toHaveBeenCalled(); + }); + + it("only calls plugins that declared a hook for the requested toolId", async () => { + const taskHook = vi.fn().mockResolvedValue("## GitHub\nBranch: feat/t1"); + const sprintOnlyHook = vi.fn().mockResolvedValue("should not run for get_task"); + const registry = new PluginRegistry([ + { + pluginId: "com.paca.github", + entry: { tools: [], handleToolCall: vi.fn(), getToolContext: taskHook }, + toolContextHooks: ["get_task"], + }, + { + pluginId: "com.paca.other", + entry: { + tools: [], + handleToolCall: vi.fn(), + getToolContext: sprintOnlyHook, + }, + toolContextHooks: ["list_sprints"], + }, + ]); + + const sections = await registry.getToolContext( + "get_task", + { projectId: "p1", taskId: "t1" }, + config, + ); + expect(sections).toEqual([ + { pluginId: "com.paca.github", text: "## GitHub\nBranch: feat/t1" }, + ]); + expect(taskHook).toHaveBeenCalledTimes(1); + expect(sprintOnlyHook).not.toHaveBeenCalled(); + }); + + it("collects text from every declared plugin that returns a non-empty section", async () => { + const registry = new PluginRegistry([ + { + pluginId: "com.paca.github", + entry: { + tools: [], + handleToolCall: vi.fn(), + getToolContext: vi.fn().mockResolvedValue("## GitHub\nBranch: feat/t1"), + }, + toolContextHooks: ["get_task"], + }, + { + pluginId: "com.paca.checklist", + entry: { + tools: [], + handleToolCall: vi.fn(), + getToolContext: vi.fn().mockResolvedValue("## Checklist\n- [ ] Item"), + }, + toolContextHooks: ["get_task"], + }, + ]); + const sections = await registry.getToolContext( + "get_task", + { projectId: "p1", taskId: "t1" }, + config, + ); + expect(sections).toEqual([ + { pluginId: "com.paca.github", text: "## GitHub\nBranch: feat/t1" }, + { pluginId: "com.paca.checklist", text: "## Checklist\n- [ ] Item" }, + ]); + }); + + it("omits plugins that resolve to null (nothing to contribute)", async () => { + const registry = new PluginRegistry([ + { + pluginId: "com.paca.github", + entry: { + tools: [], + handleToolCall: vi.fn(), + getToolContext: vi.fn().mockResolvedValue(null), + }, + toolContextHooks: ["get_task"], + }, + ]); + const sections = await registry.getToolContext( + "get_task", + { projectId: "p1", taskId: "t1" }, + config, + ); + expect(sections).toEqual([]); + }); + + it("logs and contributes nothing when a declared plugin's module has no getToolContext", async () => { + const registry = new PluginRegistry([ + { + pluginId: "com.paca.mismatched", + entry: { tools: [], handleToolCall: vi.fn() }, // no getToolContext impl + toolContextHooks: ["get_task"], // but manifest declares it + }, + ]); + const sections = await registry.getToolContext( + "get_task", + { projectId: "p1", taskId: "t1" }, + config, + ); + expect(sections).toEqual([]); + }); + + it("swallows a throwing plugin without affecting the others", async () => { + const registry = new PluginRegistry([ + { + pluginId: "com.paca.broken", + entry: { + tools: [], + handleToolCall: vi.fn(), + getToolContext: vi.fn().mockRejectedValue(new Error("boom")), + }, + toolContextHooks: ["get_task"], + }, + { + pluginId: "com.paca.bdd", + entry: { + tools: [], + handleToolCall: vi.fn(), + getToolContext: vi.fn().mockResolvedValue("## BDD\nScenario: X"), + }, + toolContextHooks: ["get_task"], + }, + ]); + const sections = await registry.getToolContext( + "get_task", + { projectId: "p1", taskId: "t1" }, + config, + ); + expect(sections).toEqual([ + { pluginId: "com.paca.bdd", text: "## BDD\nScenario: X" }, + ]); + }); + + it("passes toolId, args, and per-plugin context to getToolContext", async () => { + const getToolContext = vi.fn().mockResolvedValue("## GitHub\n..."); + const registry = new PluginRegistry([ + { + pluginId: "com.paca.github", + entry: { tools: [], handleToolCall: vi.fn(), getToolContext }, + toolContextHooks: ["get_task"], + }, + ]); + await registry.getToolContext( + "get_task", + { projectId: "p1", taskId: "t1" }, + config, + ); + expect(getToolContext).toHaveBeenCalledWith( + "get_task", + { projectId: "p1", taskId: "t1" }, + { + pluginId: "com.paca.github", + baseURL: config.baseURL, + apiKey: config.apiKey, + }, + ); + }); +}); diff --git a/apps/mcp/src/plugin-loader.ts b/apps/mcp/src/plugin-loader.ts index 59c31f54..2a98f827 100644 --- a/apps/mcp/src/plugin-loader.ts +++ b/apps/mcp/src/plugin-loader.ts @@ -27,6 +27,13 @@ interface InstalledPlugin { manifest: { mcp?: { remoteEntryUrl: string; + /** + * Core tool IDs (e.g. "get_task") this plugin's `getToolContext` + * can contribute to. Declared up front so the host only calls + * into plugins that actually registered interest in a given + * tool, instead of invoking every loaded plugin on every call. + */ + toolContextHooks?: string[]; }; }; } @@ -43,6 +50,18 @@ interface PluginMCPEntry { args: Record, context: PluginMCPContext, ): Promise; + /** Optional: contribute additional text to the response of any core tool call. */ + getToolContext?( + toolId: string, + args: Record, + context: PluginMCPContext, + ): Promise; +} + +/** A plugin-contributed section attached to a core tool's response. */ +export interface PluginContextSection { + pluginId: string; + text: string; } interface PluginMCPContext { @@ -60,6 +79,8 @@ interface PluginToolResult { interface LoadedPlugin { pluginId: string; entry: PluginMCPEntry; + /** Core tool IDs this plugin declared (in its manifest) it can add context to. */ + toolContextHooks: string[]; } // ── Registry ────────────────────────────────────────────────────────────────── @@ -74,11 +95,19 @@ export class PluginRegistry { private readonly toolOwner: Map; /** Deduplicated tool definitions contributed by loaded plugins. */ private readonly tools: Tool[]; + /** + * Map from core tool ID → plugins that declared (in their manifest) a + * `getToolContext` hook for it. Built once at load time so a call for a + * tool no plugin cares about costs a single Map lookup, not N plugin + * invocations. + */ + private readonly toolContextOwners: Map; constructor(loaded: LoadedPlugin[]) { this.loaded = loaded; this.toolOwner = new Map(); this.tools = []; + this.toolContextOwners = new Map(); for (const p of loaded) { for (const tool of p.entry.tools) { if (this.toolOwner.has(tool.name)) { @@ -92,6 +121,12 @@ export class PluginRegistry { this.toolOwner.set(tool.name, p.pluginId); this.tools.push(tool); } + + for (const toolId of p.toolContextHooks) { + const owners = this.toolContextOwners.get(toolId) ?? []; + owners.push(p); + this.toolContextOwners.set(toolId, owners); + } } } @@ -138,6 +173,57 @@ export class PluginRegistry { }; } } + + /** + * Collect context sections for a given core tool call, but only from + * plugins that declared a `getToolContext` hook for that exact tool ID + * in their manifest (`mcp.toolContextHooks`). Plugins that didn't + * register interest in `toolId` are never invoked — this is a Map + * lookup, not a fan-out over every loaded plugin. + * + * A registered plugin that throws, or that turns out not to actually + * implement `getToolContext` (manifest/module mismatch), contributes + * nothing — one broken plugin cannot blank out the rest of the response. + */ + async getToolContext( + toolId: string, + args: Record, + config: PacaConfig, + ): Promise { + const candidates = this.toolContextOwners.get(toolId); + if (!candidates || candidates.length === 0) return []; + + const sections: PluginContextSection[] = []; + + await Promise.all( + candidates.map(async (p) => { + if (!p.entry.getToolContext) { + console.error( + `[plugin-loader] Plugin "${p.pluginId}" declared toolContextHooks for "${toolId}" but its module has no getToolContext method`, + ); + return; + } + + const context: PluginMCPContext = { + pluginId: p.pluginId, + baseURL: config.baseURL, + apiKey: config.apiKey, + }; + + try { + const text = await p.entry.getToolContext(toolId, args, context); + if (text) sections.push({ pluginId: p.pluginId, text }); + } catch (error) { + console.error( + `[plugin-loader] Plugin "${p.pluginId}" getToolContext("${toolId}") failed:`, + error, + ); + } + }), + ); + + return sections; + } } // ── Loader ──────────────────────────────────────────────────────────────────── @@ -184,9 +270,13 @@ export async function loadPlugins(config: PacaConfig): Promise { const pluginBaseURL = config.gatewayURL ?? config.baseURL; try { const entry = await loadPluginEntry(plugin.name, url, pluginBaseURL); - loaded.push({ pluginId: plugin.name, entry }); + const toolContextHooks = plugin.manifest.mcp?.toolContextHooks ?? []; + loaded.push({ pluginId: plugin.name, entry, toolContextHooks }); console.error( - `[plugin-loader] Loaded "${plugin.name}" (${entry.tools.length} tool(s))`, + `[plugin-loader] Loaded "${plugin.name}" (${entry.tools.length} tool(s)` + + (toolContextHooks.length > 0 + ? `, context hooks: ${toolContextHooks.join(", ")})` + : ")"), ); } catch (err) { console.error( diff --git a/apps/mcp/src/server.ts b/apps/mcp/src/server.ts index 27f0b81b..00beedf1 100644 --- a/apps/mcp/src/server.ts +++ b/apps/mcp/src/server.ts @@ -37,6 +37,10 @@ export async function createServer(config: PacaConfig): Promise { const docClient = new PacaAPIDocClient(config); const automationClient = new PacaAPIAutomationClient(config); + // Load plugin MCP modules from the Paca API. + // Failures for individual plugins are logged and skipped. + const pluginRegistry = await loadPlugins(config); + const clients = { apiClient, extendedClient, @@ -46,10 +50,6 @@ export async function createServer(config: PacaConfig): Promise { automationClient, }; - // Load plugin MCP modules from the Paca API. - // Failures for individual plugins are logged and skipped. - const pluginRegistry = await loadPlugins(config); - // Fetch agent permissions at startup const permissionMap: PermissionMap = await fetchAgentPermissions(config); @@ -164,7 +164,40 @@ export async function createServer(config: PacaConfig): Promise { } // Fall through to core tool handlers - return handleToolCall(request, clients); + const result = await handleToolCall(request, clients); + + // Let every loaded plugin optionally attach context to this core + // tool's response (e.g. linked GitHub branches on get_task). Skipped + // for error results so a failure isn't buried under unrelated data. + if (!result?.isError) { + const sections = await pluginRegistry.getToolContext( + name, + (args ?? {}) as Record, + config, + ); + if (sections.length > 0) { + const extra = sections.map((s) => s.text).join("\n\n"); + // Merge into the last text block rather than appending a new + // content entry: agents reliably read the one continuous text + // block a core tool returns, but have been observed treating + // a separate trailing block as unrelated/easy to miss — e.g. + // calling github_list_task_branches right after get_task + // despite the branch already being in a second block. + const content = [...result.content]; + const lastIdx = content.length - 1; + if (lastIdx >= 0 && content[lastIdx]?.type === "text") { + content[lastIdx] = { + ...content[lastIdx], + text: `${content[lastIdx].text}\n\n${extra}`, + }; + } else { + content.push({ type: "text", text: extra }); + } + return { ...result, content }; + } + } + + return result; }); return server; diff --git a/docs/plugins/mcp-plugin-system.md b/docs/plugins/mcp-plugin-system.md index 2f3920a6..3a34144e 100644 --- a/docs/plugins/mcp-plugin-system.md +++ b/docs/plugins/mcp-plugin-system.md @@ -101,6 +101,60 @@ const entry: PluginMCPEntry = { export default entry; ``` +## Contributing to any core tool's response (`getToolContext`) + +A plugin can optionally implement `getToolContext` on its `PluginMCPEntry` to +attach additional text to the response of **any** core Paca tool call — not +just `get_task`. This lets an AI client see a plugin's data (linked +branches, checklist items, BDD scenarios, …) inline in whatever core tool it +already called, without separately discovering and calling the plugin's own +tools. + +```ts +const entry: PluginMCPEntry = { + tools: [ /* ... */ ], + async handleToolCall(name, args, context) { /* ... */ }, + + async getToolContext(toolId, args, context) { + if (toolId !== "get_task") return null; // only enrich get_task + const { projectId, taskId } = args as { projectId: string; taskId: string }; + + const api = new PluginAPIClient(context); + const items = await api.pluginGet( + `projects/${projectId}/tasks/${taskId}/items`, + ); + if (items.length === 0) return null; // nothing to add — omit the section + return `## My Plugin\n\n${items.map((i) => `- ${i.title}`).join("\n")}`; + }, +}; +``` + +Notes: + +- `toolId` is the core tool's name (`"get_task"`, `"list_tasks"`, + `"get_project"`, …) — switch on it to decide what, if anything, to add. + See `ALL_TOOLS.md` in `apps/mcp` for the full list of core tools and their + argument shapes. +- `args` is exactly what the AI client passed for that call — the same + shape the core tool itself receives, nothing more. It may not contain + every ID your plugin needs: e.g. `get_task_by_number` has no `taskId`, + only `taskNumber`, so a hook scoped to `"get_task"` won't fire for it. +- Return `null` (or `undefined`) when the plugin has nothing to contribute + for this call. The host omits the section entirely rather than rendering + empty boilerplate on every call — most calls won't touch every plugin. +- The host calls `getToolContext` for every loaded plugin that implements + it, in parallel, after every successful core tool call (skipped when the + core call itself returned an error). Keep it fast and read-only. +- Errors are caught and logged by the host (`[plugin-loader] Plugin "" + getToolContext("") failed: ...`) — a throwing plugin contributes + nothing but cannot break the rest of the response. You don't need your + own try/catch purely for that; add one if you want a specific failure + (e.g. "not configured for this project") to resolve to `null` instead of + logging. +- All plugins' returned text is joined and appended as one additional + content block on the tool result — prefix your text with a heading (e.g. + `## GitHub`) so it reads clearly alongside other plugins' sections. + ## Plugin SDK (`@paca-ai/plugin-sdk-mcp`) The `@paca-ai/plugin-sdk-mcp` package provides: diff --git a/services/api/internal/domain/plugin/entity.go b/services/api/internal/domain/plugin/entity.go index fdcf40f0..901d72ba 100644 --- a/services/api/internal/domain/plugin/entity.go +++ b/services/api/internal/domain/plugin/entity.go @@ -217,6 +217,12 @@ type MCPManifest struct { // The module must be a Node.js-compatible ESM bundle that exports a // PluginMCPEntry as its default export (see @paca-ai/plugin-sdk-mcp). RemoteEntryURL string `json:"remoteEntryUrl"` + // ToolContextHooks lists core tool IDs (e.g. "get_task") this plugin's + // getToolContext can contribute to. The MCP server only calls into a + // plugin for a given tool if that tool ID is declared here — this is + // what lets the host skip plugins that have no interest in a given + // tool call instead of invoking every loaded plugin on every call. + ToolContextHooks []string `json:"toolContextHooks,omitempty"` } // SkillsManifest describes the Agent Skills a plugin contributes. When From a537fe5e4baf8223c3b6e1c7cd5b2703f615a69b Mon Sep 17 00:00:00 2001 From: pikann22 Date: Mon, 10 Aug 2026 05:06:12 +0000 Subject: [PATCH 2/4] feat: add tests for plugin context merging and ensure manifest fields are preserved --- apps/mcp/src/__tests__/server.test.ts | 74 +++++++++++++++ apps/mcp/src/plugin-loader.ts | 19 ++-- apps/mcp/src/server.ts | 53 +++++++---- docs/plugins/mcp-plugin-system.md | 18 ++-- .../postgres/plugin_repository_test.go | 93 +++++++++++++++++++ 5 files changed, 225 insertions(+), 32 deletions(-) create mode 100644 apps/mcp/src/__tests__/server.test.ts create mode 100644 services/api/internal/repository/postgres/plugin_repository_test.go diff --git a/apps/mcp/src/__tests__/server.test.ts b/apps/mcp/src/__tests__/server.test.ts new file mode 100644 index 00000000..4a5bca54 --- /dev/null +++ b/apps/mcp/src/__tests__/server.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it } from "vitest"; +import type { PluginContextSection } from "../plugin-loader.js"; +import { mergePluginContext } from "../server.js"; + +// --------------------------------------------------------------------------- +// mergePluginContext +// --------------------------------------------------------------------------- +// +// Regression coverage for the bug described in the getToolContext PR: a +// plugin's contributed text was originally appended as a separate trailing +// content block, which agents were observed treating as unrelated and +// ignoring (e.g. calling github_list_task_branches right after get_task +// despite the branch already being in a second block). The fix merges into +// the last existing text block instead. + +describe("mergePluginContext", () => { + const section = (text: string, pluginId = "com.paca.github") => + [{ pluginId, text }] satisfies PluginContextSection[]; + + it("merges a single section into the last text block", () => { + const result = { + content: [{ type: "text", text: "# Task: Fix login bug" }], + }; + const merged = mergePluginContext( + result, + section("## GitHub\nBranch: feat/t1"), + ); + expect(merged.content).toHaveLength(1); + expect(merged.content[0]).toEqual({ + type: "text", + text: "# Task: Fix login bug\n\n## GitHub\nBranch: feat/t1", + }); + }); + + it("joins multiple sections in the given order before merging", () => { + const result = { content: [{ type: "text", text: "# Task" }] }; + const merged = mergePluginContext(result, [ + { pluginId: "com.paca.github", text: "## GitHub" }, + { pluginId: "com.paca.checklist", text: "## Checklist" }, + ]); + expect(merged.content[0].text).toBe("# Task\n\n## GitHub\n\n## Checklist"); + }); + + it("appends a new text block when content is empty", () => { + const result = { content: [] }; + const merged = mergePluginContext(result, section("## GitHub")); + expect(merged.content).toEqual([{ type: "text", text: "## GitHub" }]); + }); + + it("appends a new text block when the last block isn't type text", () => { + const result = { + content: [{ type: "image", data: "base64...", mimeType: "image/png" }], + }; + const merged = mergePluginContext(result, section("## GitHub")); + expect(merged.content).toHaveLength(2); + expect(merged.content[1]).toEqual({ type: "text", text: "## GitHub" }); + }); + + it("does not mutate the original result's content array", () => { + const originalContent = [{ type: "text", text: "# Task" }]; + const result = { content: originalContent }; + mergePluginContext(result, section("## GitHub")); + expect(originalContent).toEqual([{ type: "text", text: "# Task" }]); + }); + + it("preserves other fields on the result (e.g. isError: false)", () => { + const result = { + content: [{ type: "text", text: "# Task" }], + isError: false, + }; + const merged = mergePluginContext(result, section("## GitHub")); + expect(merged.isError).toBe(false); + }); +}); diff --git a/apps/mcp/src/plugin-loader.ts b/apps/mcp/src/plugin-loader.ts index 2a98f827..2a55b238 100644 --- a/apps/mcp/src/plugin-loader.ts +++ b/apps/mcp/src/plugin-loader.ts @@ -184,6 +184,12 @@ export class PluginRegistry { * A registered plugin that throws, or that turns out not to actually * implement `getToolContext` (manifest/module mismatch), contributes * nothing — one broken plugin cannot blank out the rest of the response. + * + * Results are returned in `candidates` (i.e. plugin load) order + * regardless of which plugin's call resolves first — the requests + * themselves still run concurrently via `Promise.all`, only the + * ordering of the resolved output is fixed, so a slow plugin can't + * shuffle the sections a caller sees on one call vs. the next. */ async getToolContext( toolId: string, @@ -193,15 +199,13 @@ export class PluginRegistry { const candidates = this.toolContextOwners.get(toolId); if (!candidates || candidates.length === 0) return []; - const sections: PluginContextSection[] = []; - - await Promise.all( - candidates.map(async (p) => { + const results = await Promise.all( + candidates.map(async (p): Promise => { if (!p.entry.getToolContext) { console.error( `[plugin-loader] Plugin "${p.pluginId}" declared toolContextHooks for "${toolId}" but its module has no getToolContext method`, ); - return; + return null; } const context: PluginMCPContext = { @@ -212,17 +216,18 @@ export class PluginRegistry { try { const text = await p.entry.getToolContext(toolId, args, context); - if (text) sections.push({ pluginId: p.pluginId, text }); + return text ? { pluginId: p.pluginId, text } : null; } catch (error) { console.error( `[plugin-loader] Plugin "${p.pluginId}" getToolContext("${toolId}") failed:`, error, ); + return null; } }), ); - return sections; + return results.filter((s): s is PluginContextSection => s !== null); } } diff --git a/apps/mcp/src/server.ts b/apps/mcp/src/server.ts index 00beedf1..65d81dad 100644 --- a/apps/mcp/src/server.ts +++ b/apps/mcp/src/server.ts @@ -17,7 +17,7 @@ import { hasPermission, type PermissionMap, } from "./permissions.js"; -import { loadPlugins } from "./plugin-loader.js"; +import { loadPlugins, type PluginContextSection } from "./plugin-loader.js"; import { getAllTools, handleToolCall } from "./tools/index.js"; import type { PacaConfig } from "./types/index.js"; @@ -176,24 +176,7 @@ export async function createServer(config: PacaConfig): Promise { config, ); if (sections.length > 0) { - const extra = sections.map((s) => s.text).join("\n\n"); - // Merge into the last text block rather than appending a new - // content entry: agents reliably read the one continuous text - // block a core tool returns, but have been observed treating - // a separate trailing block as unrelated/easy to miss — e.g. - // calling github_list_task_branches right after get_task - // despite the branch already being in a second block. - const content = [...result.content]; - const lastIdx = content.length - 1; - if (lastIdx >= 0 && content[lastIdx]?.type === "text") { - content[lastIdx] = { - ...content[lastIdx], - text: `${content[lastIdx].text}\n\n${extra}`, - }; - } else { - content.push({ type: "text", text: extra }); - } - return { ...result, content }; + return mergePluginContext(result, sections); } } @@ -202,3 +185,35 @@ export async function createServer(config: PacaConfig): Promise { return server; } + +/** + * Merge plugin-contributed context sections into a core tool's result. + * + * Merges into the *last* text content block rather than appending a new + * content entry: agents reliably read the one continuous text block a core + * tool returns, but have been observed treating a separate trailing block + * as unrelated/easy to miss — e.g. calling github_list_task_branches right + * after get_task despite the branch already being in a second block. Falls + * back to appending a new text block when the result has no existing text + * block to merge into (e.g. empty content array). + * + * Exported for testing; `sections` is expected to be non-empty — callers + * should skip invoking this when there's nothing to merge. + */ +export function mergePluginContext( + result: any, + sections: PluginContextSection[], +): any { + const extra = sections.map((s) => s.text).join("\n\n"); + const content = [...result.content]; + const lastIdx = content.length - 1; + if (lastIdx >= 0 && content[lastIdx]?.type === "text") { + content[lastIdx] = { + ...content[lastIdx], + text: `${content[lastIdx].text}\n\n${extra}`, + }; + } else { + content.push({ type: "text", text: extra }); + } + return { ...result, content }; +} diff --git a/docs/plugins/mcp-plugin-system.md b/docs/plugins/mcp-plugin-system.md index 3a34144e..9fbfd0fc 100644 --- a/docs/plugins/mcp-plugin-system.md +++ b/docs/plugins/mcp-plugin-system.md @@ -142,18 +142,24 @@ Notes: - Return `null` (or `undefined`) when the plugin has nothing to contribute for this call. The host omits the section entirely rather than rendering empty boilerplate on every call — most calls won't touch every plugin. -- The host calls `getToolContext` for every loaded plugin that implements - it, in parallel, after every successful core tool call (skipped when the - core call itself returned an error). Keep it fast and read-only. +- The host only calls `getToolContext` on plugins that declared `toolId` in + their manifest's `mcp.toolContextHooks` — implementing the method alone + isn't enough, you also need the manifest declaration or the hook never + fires. Declared plugins are called in parallel, after every successful + core tool call (skipped when the core call itself returned an error). + Keep it fast and read-only. - Errors are caught and logged by the host (`[plugin-loader] Plugin "" getToolContext("") failed: ...`) — a throwing plugin contributes nothing but cannot break the rest of the response. You don't need your own try/catch purely for that; add one if you want a specific failure (e.g. "not configured for this project") to resolve to `null` instead of logging. -- All plugins' returned text is joined and appended as one additional - content block on the tool result — prefix your text with a heading (e.g. - `## GitHub`) so it reads clearly alongside other plugins' sections. +- All plugins' returned text is joined (in manifest-declared plugin order) + and merged into the tool result's last text block — not appended as a + separate content entry — so the AI client sees task detail and plugin + context as one continuous passage instead of a trailing block it can + ignore. Prefix your text with a heading (e.g. `## GitHub`) so it reads + clearly alongside other plugins' sections. ## Plugin SDK (`@paca-ai/plugin-sdk-mcp`) diff --git a/services/api/internal/repository/postgres/plugin_repository_test.go b/services/api/internal/repository/postgres/plugin_repository_test.go new file mode 100644 index 00000000..6cec15cc --- /dev/null +++ b/services/api/internal/repository/postgres/plugin_repository_test.go @@ -0,0 +1,93 @@ +package postgres + +import ( + "slices" + "testing" + "time" + + "github.com/google/uuid" + + plugindom "github.com/Paca-AI/api/internal/domain/plugin" +) + +// TestPluginToModel_RoundTrip_PreservesMCPManifestFields guards against a +// regression class this package has already hit once: Create/Update +// re-marshal the *typed* PluginManifest struct into the DB's JSONB column +// rather than storing the raw plugin.json bytes, so any manifest field not +// mirrored in the Go struct is silently dropped on install/update instead of +// erroring. ToolContextHooks was added to MCPManifest specifically to fix +// one such drop; this test exercises the exact conversion path +// (pluginToModel -> pluginFromModel) Create/Update/FindByID all go through, +// so a future field added to the wrong place fails a test instead of +// silently vanishing in production. +func TestPluginToModel_RoundTrip_PreservesMCPManifestFields(t *testing.T) { + original := &plugindom.Plugin{ + ID: uuid.New(), + Name: "com.paca.github", + Version: "1.0.0", + Manifest: plugindom.PluginManifest{ + ID: "com.paca.github", + Version: "1.0.0", + MCP: &plugindom.MCPManifest{ + RemoteEntryURL: "https://example.com/entry.js", + ToolContextHooks: []string{"get_task", "list_tasks"}, + }, + }, + Enabled: true, + InstalledAt: time.Now().UTC().Truncate(time.Second), + UpdatedAt: time.Now().UTC().Truncate(time.Second), + } + + model, err := pluginToModel(original) + if err != nil { + t.Fatalf("pluginToModel: %v", err) + } + + roundTripped, err := pluginFromModel(model) + if err != nil { + t.Fatalf("pluginFromModel: %v", err) + } + + if roundTripped.Manifest.MCP == nil { + t.Fatal("MCP manifest was dropped on round trip") + } + if roundTripped.Manifest.MCP.RemoteEntryURL != original.Manifest.MCP.RemoteEntryURL { + t.Errorf("RemoteEntryURL = %q, want %q", + roundTripped.Manifest.MCP.RemoteEntryURL, original.Manifest.MCP.RemoteEntryURL) + } + if !slices.Equal(roundTripped.Manifest.MCP.ToolContextHooks, original.Manifest.MCP.ToolContextHooks) { + t.Errorf("ToolContextHooks = %v, want %v", + roundTripped.Manifest.MCP.ToolContextHooks, original.Manifest.MCP.ToolContextHooks) + } +} + +// TestPluginToModel_RoundTrip_NilMCPManifest ensures a manifest with no mcp +// block at all (most plugins) round-trips to nil rather than a zero-value +// struct, since callers branch on `Manifest.MCP == nil`. +func TestPluginToModel_RoundTrip_NilMCPManifest(t *testing.T) { + original := &plugindom.Plugin{ + ID: uuid.New(), + Name: "com.paca.no-mcp", + Version: "1.0.0", + Manifest: plugindom.PluginManifest{ + ID: "com.paca.no-mcp", + Version: "1.0.0", + }, + InstalledAt: time.Now().UTC().Truncate(time.Second), + UpdatedAt: time.Now().UTC().Truncate(time.Second), + } + + model, err := pluginToModel(original) + if err != nil { + t.Fatalf("pluginToModel: %v", err) + } + + roundTripped, err := pluginFromModel(model) + if err != nil { + t.Fatalf("pluginFromModel: %v", err) + } + + if roundTripped.Manifest.MCP != nil { + t.Errorf("MCP = %+v, want nil", roundTripped.Manifest.MCP) + } +} From c5771a06931cf9b4e7cff2b0fb995b08e48c1d1d Mon Sep 17 00:00:00 2001 From: pikann22 Date: Mon, 10 Aug 2026 05:54:14 +0000 Subject: [PATCH 3/4] feat: add validation for toolContextHooks in PluginRegistry to ensure getToolContext method exists --- apps/mcp/src/plugin-loader.ts | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/apps/mcp/src/plugin-loader.ts b/apps/mcp/src/plugin-loader.ts index 2a55b238..eeb01631 100644 --- a/apps/mcp/src/plugin-loader.ts +++ b/apps/mcp/src/plugin-loader.ts @@ -122,6 +122,15 @@ export class PluginRegistry { this.tools.push(tool); } + if (p.toolContextHooks.length > 0 && !p.entry.getToolContext) { + // Manifest/module mismatch is static once the entry is loaded — + // warn once here rather than on every matching tool call. + console.error( + `[plugin-loader] Plugin "${p.pluginId}" declared toolContextHooks (${p.toolContextHooks.join(", ")}) but its module has no getToolContext method`, + ); + continue; + } + for (const toolId of p.toolContextHooks) { const owners = this.toolContextOwners.get(toolId) ?? []; owners.push(p); @@ -181,9 +190,10 @@ export class PluginRegistry { * register interest in `toolId` are never invoked — this is a Map * lookup, not a fan-out over every loaded plugin. * - * A registered plugin that throws, or that turns out not to actually - * implement `getToolContext` (manifest/module mismatch), contributes - * nothing — one broken plugin cannot blank out the rest of the response. + * A registered plugin that throws contributes nothing — one broken + * plugin cannot blank out the rest of the response. A manifest/module + * mismatch (declared but not implemented) is filtered out and logged + * once at load time (see the constructor), so it never reaches here. * * Results are returned in `candidates` (i.e. plugin load) order * regardless of which plugin's call resolves first — the requests @@ -201,12 +211,11 @@ export class PluginRegistry { const results = await Promise.all( candidates.map(async (p): Promise => { - if (!p.entry.getToolContext) { - console.error( - `[plugin-loader] Plugin "${p.pluginId}" declared toolContextHooks for "${toolId}" but its module has no getToolContext method`, - ); - return null; - } + // Guaranteed non-null: only plugins that pass this check at + // load time are ever added to toolContextOwners. Narrows the + // optional method for TS rather than guarding against a case + // that can occur here. + if (!p.entry.getToolContext) return null; const context: PluginMCPContext = { pluginId: p.pluginId, From d0061d0d61a47c3718a3a1b457b2f86593311cff Mon Sep 17 00:00:00 2001 From: pikann22 Date: Mon, 10 Aug 2026 06:08:57 +0000 Subject: [PATCH 4/4] fix: improve automation run status check to avoid race conditions --- services/api/test/e2e/automation_engine_test.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/services/api/test/e2e/automation_engine_test.go b/services/api/test/e2e/automation_engine_test.go index 7395e9d9..d826939e 100644 --- a/services/api/test/e2e/automation_engine_test.go +++ b/services/api/test/e2e/automation_engine_test.go @@ -2686,13 +2686,11 @@ func TestE2EAutomationEngine_SprintStartedTriggerConditionAndUpdateSprint(t *tes t.Fatalf("expected the sprint's goal to be updated to \"kickoff\", got %q", goal) } - runs := listAutomationRunsViaAPI(t, env, ownerClient, ownerToken, projID, automationID) - if len(runs) != 1 { - t.Fatalf("expected exactly one recorded run, got %d", len(runs)) - } - if status, _ := runs[0]["status"].(string); status != "completed" { - t.Fatalf("expected the run to finalize as completed, got %q", status) - } + // The sprint's goal field and the run's own status are two separate + // writes; waitForSprintField only proves the former landed, so the run + // can still legitimately be "running" for a moment after. Poll for the + // run status instead of checking it once, to avoid racing that write. + waitForAutomationRunStatus(t, env, ownerClient, ownerToken, projID, automationID, "completed", 10*time.Second) } // TestE2EAutomationEngine_TaskTriggeredCompleteSprintViaTaskSprintID fires a