feat: implement getToolContext for plugins to enrich core tool responses - #387
Conversation
There was a problem hiding this comment.
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.getToolContextplus a manifest-declaredmcp.toolContextHooksgate;PluginRegistryindexes 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\nand merged into the result's last text block (new text block when the last block isn't text), skipped onisErrorresults (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;
tscbuild passes. - Docs section in
docs/plugins/mcp-plugin-system.md;MCPManifest.ToolContextHookson 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 viaconsole.erroron 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.pushyields completion order, not manifest declaration order, so the joined output varies call to call when multiple plugins have a section. Sorting by declaration order (orpluginId) 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 theserver.tscomment about agents missing trailing blocks).
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ No new issues found. Prior feedback is cleanly addressed; the incremental delta (testing + doc + ordering fixes) is solid.
Reviewed changes
Since the prior pullfrog review (3edd1f0), the new commit (a537fe5) makes these changes:
- Extracted the core-tool enrichment merge into an exported pure
mergePluginContextfunction inserver.tsand added a 6-caseserver.test.tscovering merge-into-last-text-block, empty-content and non-text-last-block fallbacks, non-mutation, and result-field preservation — directly addressing the previously untested glue. - Made
PluginRegistry.getToolContextreturn sections in plugin-load (declaration) order regardless of which plugin's call resolves first, fixing the earlier nondeterministic-ordering nitpick. - Corrected
docs/plugins/mcp-plugin-system.mdto state that the manifestmcp.toolContextHooksdeclaration is required for a hook to fire, and that sections are merged into the last text block rather than appended — resolving the misdocumented activation contract. - Added two Go round-trip regression tests in
plugin_repository_test.goguarding the typed-manifest→JSONB serialization path (ToolContextHookspreservation, and nil-MCP manifest staying nil).
The new section-ordering refactor keeps concurrency (Promise.all) while fixing output order — a clean resolution of the earlier nondeterminism concern. I verified locally: the 14 new MCP Vitest cases pass and both Go round-trip tests pass.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
… getToolContext method exists
There was a problem hiding this comment.
✅ No new issues found. The new commit cleanly resolves the last remaining nitpick from the prior review.
Reviewed changes
Since the prior pullfrog review (a537fe5), the new commit (c5771a0) makes these changes:
- Moved the manifest/module-mismatch check (plugin declares
toolContextHooksbut its module exports nogetToolContext) out of the per-call path and into thePluginRegistryconstructor: it's now logged once at load time viaconsole.error, and the mismatched plugin is filtered out oftoolContextOwners(thecontinueleaves the plugin's tools registered, only its context hooks are dropped). - Simplified
getToolContextaccordingly — the per-callif (!p.entry.getToolContext)path is now only TS narrowing, since only plugins that pass the load-time check are ever added totoolContextOwners.
This directly addresses the earlier nitpick about a console.error firing on every matching tool call for a statically-known mismatch. I verified locally: all 8 plugin-loader and 6 server Vitest cases pass (the mismatch case confirms the plugin contributes nothing and the error is logged once at construction), and tsc --noEmit is clean. Working tree clean.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ No new issues found. The incremental delta is a clean, low-risk test-only robustness fix.
Reviewed changes
The new commit (d0061d0) makes this change since the prior pullfrog review (c5771a0):
- In
TestE2EAutomationEngine_SprintStartedTriggerConditionAndUpdateSprint, replaced the one-shotruns[0]["status"] == "completed"assertion with the existingwaitForAutomationRunStatuspolling helper (10s timeout), eliminating a race between the goal-field write and the run-status write (services/api/test/e2e/automation_engine_test.go).
The helper is already used across many sibling tests, listAutomationRunsViaAPI remains used elsewhere, and no dangling references are left behind. The change is self-contained and consistent.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

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_taskand has no idea a GitHub branch or PR is linked to that task unless it separately callsgithub_list_task_branches/github_list_task_prs. This lets the GitHub (or BDD, Checklist, …) plugin attach that info directly to theget_taskresponse instead.What changed
apps/mcp/src/plugin-loader.ts—PluginRegistrynow builds aMap<toolId, LoadedPlugin[]>at load time from each plugin's manifest-declaredmcp.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 theCallToolRequestSchemahandler: 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— addedToolContextHooks []stringtoMCPManifest. 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 unrecognizedplugin.jsonfield never survives an install/update round-trip.docs/plugins/mcp-plugin-system.md— documents thegetToolContextcontract, the manifest opt-in, and theget_task_by_numbercaveat (its args have notaskId, onlytaskNumber, so a hook scoped toget_taskwon'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_taskcall 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 calledgithub_list_task_branchesanyway 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, soget_taskreturns one continuous passage — task detail followed directly by## GitHub— instead of a result an agent can partially ignore.Test plan
apps/mcp:tsc --noEmitclean,vitest run— 503/503 passservices/api:go build ./...,go vet ./..., andgo test ./internal/domain/plugin/... ./internal/repository/postgres/... ./internal/transport/http/...all passmcpservice, confirmed 0 compile errors, confirmed the compiledbuild/server.js(what the sandboxed agent'snode /mcp/build/index.jsactually runs) has the fixagent_conversation_eventson a real conversation thatget_taskreturns a merged## GitHubsection for a task with a linked branchDepends on
com.paca.github,com.paca.bdd, andcom.paca.checklistplugin repos are updated separately to declaretoolContextHooksin their manifests and implementgetToolContext— those changes ship independently and require@paca-ai/plugin-sdk-mcp@^0.2.0.