Skip to content

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

Merged
pikann merged 4 commits into
masterfrom
feature/implement-get-tool-context-for-plugin
Aug 10, 2026
Merged

feat: implement getToolContext for plugins to enrich core tool responses#387
pikann merged 4 commits 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) | 𝕏

Comment thread docs/plugins/mcp-plugin-system.md Outdated

@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.

✅ 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 mergePluginContext function in server.ts and added a 6-case server.test.ts covering 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.getToolContext return 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.md to state that the manifest mcp.toolContextHooks declaration 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.go guarding the typed-manifest→JSONB serialization path (ToolContextHooks preservation, 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.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@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.

✅ 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 toolContextHooks but its module exports no getToolContext) out of the per-call path and into the PluginRegistry constructor: it's now logged once at load time via console.error, and the mismatched plugin is filtered out of toolContextOwners (the continue leaves the plugin's tools registered, only its context hooks are dropped).
  • Simplified getToolContext accordingly — the per-call if (!p.entry.getToolContext) path is now only TS narrowing, since only plugins that pass the load-time check are ever added to toolContextOwners.

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.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@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.

✅ 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-shot runs[0]["status"] == "completed" assertion with the existing waitForAutomationRunStatus polling 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.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pikann
pikann merged commit ce9502b into master Aug 10, 2026
6 checks passed
@pikann
pikann deleted the feature/implement-get-tool-context-for-plugin branch August 10, 2026 06:15
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