Skip to content

feat: implement getToolContext for plugins to enrich core tool responses - #387

Open
pikann wants to merge 1 commit into
masterfrom
feature/implement-get-tool-context-for-plugin
Open

feat: implement getToolContext for plugins to enrich core tool responses#387
pikann wants to merge 1 commit into
masterfrom
feature/implement-get-tool-context-for-plugin

Conversation

@pikann

@pikann pikann commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements the host side of getToolContext (SDK contract added in Paca-AI/plugin-sdk-mcp#1, released as @paca-ai/plugin-sdk-mcp@0.2.0): lets a plugin attach additional text to the response of any core MCP tool call, declared per-tool in its manifest.

Motivating case: an AI agent calls get_task and has no idea a GitHub branch or PR is linked to that task unless it separately calls github_list_task_branches/github_list_task_prs. This lets the GitHub (or BDD, Checklist, …) plugin attach that info directly to the get_task response instead.

What changed

  • apps/mcp/src/plugin-loader.tsPluginRegistry now builds a Map<toolId, LoadedPlugin[]> at load time from each plugin's manifest-declared mcp.toolContextHooks. getToolContext(toolId, args, config) is a map lookup, not a fan-out: a plugin that didn't declare a given tool ID is never invoked for it.
  • apps/mcp/src/server.ts — single interception point in the CallToolRequestSchema handler: after any core tool call resolves (and isn't itself an error), it asks the registry for context sections and merges them into the tool result. This covers every core tool automatically, with no per-tool wiring.
  • services/api/internal/domain/plugin/entity.go — added ToolContextHooks []string to MCPManifest. Without this, the field would've been silently dropped: the plugin repository re-marshals the typed manifest struct into the DB's JSONB column rather than storing the raw JSON, so an unrecognized plugin.json field never survives an install/update round-trip.
  • docs/plugins/mcp-plugin-system.md — documents the getToolContext contract, the manifest opt-in, and the get_task_by_number caveat (its args have no taskId, only taskNumber, so a hook scoped to get_task won't fire for it).
  • apps/mcp/src/__tests__/plugin-loader.test.ts (new) — covers the map-based dispatch: a plugin is never called for a tool ID it didn't declare, multiple declared plugins' sections are collected in order, a throwing plugin doesn't affect others, a manifest/module mismatch (declared but not implemented) degrades gracefully.

A bug found (and fixed) while verifying this end-to-end

Traced a real agent conversation's get_task call against a task with a linked GitHub branch. The plugin's context was correctly generated and attached — but as a separate trailing content block, not merged into the main text. The agent didn't treat that block as part of the task's context and called github_list_task_branches anyway right after, defeating the point.

Fix (in server.ts): merge plugin-contributed text into the last existing text block instead of appending a new content array entry, so get_task returns one continuous passage — task detail followed directly by ## GitHub — instead of a result an agent can partially ignore.

Test plan

  • apps/mcp: tsc --noEmit clean, vitest run — 503/503 pass
  • services/api: go build ./..., go vet ./..., and go test ./internal/domain/plugin/... ./internal/repository/postgres/... ./internal/transport/http/... all pass
  • Verified live against the dev stack: restarted the mcp service, confirmed 0 compile errors, confirmed the compiled build/server.js (what the sandboxed agent's node /mcp/build/index.js actually runs) has the fix
  • Verified via agent_conversation_events on a real conversation that get_task returns a merged ## GitHub section for a task with a linked branch

Depends on

com.paca.github, com.paca.bdd, and com.paca.checklist plugin repos are updated separately to declare toolContextHooks in their manifests and implement getToolContext — those changes ship independently and require @paca-ai/plugin-sdk-mcp@^0.2.0.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The feature's activation contract is misdocumented: a manifest mcp.toolContextHooks declaration is required for a hook to ever fire, yet it's absent from the manifest example and one note claims the host calls every plugin that implements getToolContext — the opposite of the shipped behavior. Plugin authors following the docs end-to-end will ship hooks that silently never run.

Reviewed changes

  • PluginMCPEntry.getToolContext plus a manifest-declared mcp.toolContextHooks gate; PluginRegistry indexes owners per core tool ID once at load and fans out (Promise.all) only to declaring plugins, isolating throwing/mismatched ones (apps/mcp/src/plugin-loader.ts).
  • Core-tool enrichment: after a successful core call, plugin sections are joined with \n\n and merged into the result's last text block (new text block when the last block isn't text), skipped on isError results (apps/mcp/src/server.ts).
  • 8 Vitest cases pinning no-hook, per-tool filtering, null omission, manifest/module mismatch, and throwing-plugin behaviors. All 503 MCP tests pass; Biome clean; tsc build passes.
  • Docs section in docs/plugins/mcp-plugin-system.md; MCPManifest.ToolContextHooks on the Go side with matching camelCase JSON tag.

ℹ️ Enrichment wiring in server.ts is untested

The registry method gets thorough unit coverage, but the code that actually wires the feature into tool execution has none: the isError skip, the merge-into-last-text-block (with the push-new-block fallback), and proof that getToolContext is invoked with the core tool's name/args are all uncovered. apps/mcp has no server.test.ts at all, so once a plugin declares a hook, a regression in this glue (e.g. a mis-shapen content) would silently affect every matching core tool call.

Technical details
# Test the core-tool enrichment glue

## Affected sites
- apps/mcp/src/server.ts:186-196 — the merge-into-last-text-block / push-new-block logic
- apps/mcp/src/server.ts:172-177 — the isError skip + getToolContext invocation

## Required outcome
- Coverage that a successful core result gets plugin text merged into its last text block; an `isError: true` result never reaches `getToolContext`; a last block that isn't text gets a new text block appended; empty plugin sections leave the result untouched.

## Suggested approach (optional)
- Extract the merge into a small exported pure function (e.g. `appendPluginSections(result, sections)`) and unit-test it directly, sidestepping the need for a full `Server` instance.

ℹ️ Nitpicks

  • apps/mcp/src/plugin-loader.ts:200-204 — the manifest/module-mismatch case is logged via console.error on every matching tool call. The mismatch is static (known once the entry is imported); warn once at load time instead of per call.
  • Section order is nondeterministic: Promise.all + sections.push yields completion order, not manifest declaration order, so the joined output varies call to call when multiple plugins have a section. Sorting by declaration order (or pluginId) would make output stable.
  • docs/plugins/mcp-plugin-system.md:154-156 — "joined and appended as one additional content block" contradicts the implementation, which merges into the last text block (deliberately, per the server.ts comment about agents missing trailing blocks).

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

- 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The host only invokes getToolContext for plugins whose manifest lists the exact tool ID in mcp.toolContextHooksPluginRegistry builds that gate at load and the "never calls a plugin that didn't declare a hook" test enforces it. Two doc defects follow: the "Plugin Manifest" example (above) still shows only remoteEntryUrl, so the required field is undiscoverable, and this bullet ("every loaded plugin that implements it") states the opposite of the shipped, manifest-gated behavior. A plugin author following the docs end-to-end gets a silently dead hook — no error, no context, ever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant