Add plugin-defined MCP tools#988
Conversation
🦋 Changeset detectedLatest commit: b0200a4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Scope checkThis PR changes 683 lines across 26 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
|
/review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid implementation overall — capability gating, hex-encoded tool names to avoid plugin-id collisions, admin scope + ADMIN role enforced before dispatch, and good test coverage (capability filter, sandbox-not-loaded guard, scope check, ID collision). The delegation to handlePluginApiRoute is a clean reuse strategy.
Findings are mostly DX/footgun concerns at the definePlugin boundary plus a couple of manifest cross-validation gaps. No correctness or auth bugs that block merge.
Notes:
- Trusted (
definePlugin) plugins skip several validations that sandboxed plugins get frompluginManifestSchema— tool-name shape, route existence, and capability declaration. Silent filtering vs. early errors. getPluginRouteMetafalls back to{ public: false }for any route on an existing sandboxed plugin, so MCP tools pointing at nonexistent sandboxed routes register fine and fail at call time. The catch-all route has the same behavior, so this matches existing semantics — flagging only as DX context.- One coverage gap: no test for
requireRole(ADMIN)rejecting a non-admin user holding anadmin-scoped token (the defense-in-depth branch). - Sandboxed plugin MCP tools have no input schema in
ManifestMcpToolEntry, so they all fall back toz.record(z.string(), z.unknown())— MCP clients lose introspection. Future work, but worth tracking.
|
Review submitted successfully (review ID 4270626524) with 6 line-anchored comments and a summary body. Key findings:
No blocking issues — auth/correctness paths check out. |
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Excellent work. One thing that would be good would be if there was a way we could provide a schema for tool inputs so we can validate and describe them. While we can't serialise a Zod schema directly, maybe a JSON schema would work?
Can you include docs in the PR too, as it will also make it easier to review.
Agreed on both. I’ll add docs in this PR, and I’ll look at adding a constrained I’ll keep the JSON Schema subset conservative: object root, No |
|
Done. I added docs for plugin MCP tools and added manifest-side |
|
/review |
There was a problem hiding this comment.
Reviewing on top of the existing rounds (ask-bonk + ascorbic). Most of the substantive concerns from the earlier passes — capability cross-check, route cross-check, tool-name shape validation, JSON Schema introspection, dual-check coverage, admin-only intent comment, runtime dedup — have been addressed. A few remaining things worth a look, all DX/robustness rather than auth/correctness:
- One bad
inputSchemafrom a trusted plugin can take down the whole MCP endpoint becausejsonSchemaObjectToZodis called inline duringcreateMcpServerand has no isolation. Trusted plugins skip the Zod manifest validation that sandboxed plugins get, so the JSON Schema is unvalidated untilz.fromJSONSchemathrows. pluginToolNamecollision detection looks for__within a single segment, but two distinct valid scoped IDs can still collapse to the same generated name when a dash sits on either side of the slash boundary (@a-/bvs@a/-bboth →a___b__foo). Narrow but real.- No dedicated unit test file for
pluginToolName— the integration tests cover the happy paths but the ambiguous-segment / hash-fallback branches aren't directly exercised.
No blocking issues from me.
| inputSchema: | ||
| tool.input ?? | ||
| (tool.inputSchema | ||
| ? jsonSchemaObjectToZod(tool.inputSchema) |
There was a problem hiding this comment.
Robustness: one bad plugin inputSchema breaks the whole MCP endpoint.
jsonSchemaObjectToZod is called inline here, and z.fromJSONSchema throws on unsupported / malformed schemas. For sandboxed plugins this is fine — the manifest Zod schema (manifestJsonSchema) rejects bad shapes at parse time. For trusted plugins coming through definePlugin, there is no runtime JSON Schema validation: define-plugin.ts checks tool name shape, route existence, and capability, but never validates tool.inputSchema (the TS type is enforced at compile time only).
So a single trusted plugin author who hand-writes an inputSchema with, say, $ref, oneOf, or a misspelled type causes z.fromJSONSchema to throw during createMcpServer. The outer try/catch in api/mcp.ts then returns a JSON-RPC 500 for the whole endpoint — every other built-in tool (content, schema, media, etc.) becomes unreachable, not just the offending plugin tool.
Two cheap mitigations:
- Wrap the conversion and skip the tool on failure:
| ? jsonSchemaObjectToZod(tool.inputSchema) | |
| inputSchema: | |
| tool.input ?? | |
| (tool.inputSchema | |
| ? safeJsonSchemaObjectToZod(tool.inputSchema, tool.pluginId, tool.name) | |
| : z.record(z.string(), z.unknown())), |
where safeJsonSchemaObjectToZod try/catches and returns the permissive fallback (with a console.warn so the author has a signal). The route handler still validates server-side via PluginRouteHandler.invoke, so the security boundary is unchanged.
- Or validate
inputSchemaindefinePluginfor trusted plugins (parse it with the same Zod schema used for manifests). That way bad schemas fail loudly at config time, like the other validations added in this PR.
Low-frequency in practice, but the blast radius (entire MCP server down) makes it worth handling explicitly.
| .split(FORWARD_SLASH_PATTERN) | ||
| .map((segment) => segment.replace(PLUGIN_ID_DASH_PATTERN, "_")); | ||
| const readablePluginId = readableSegments.join("__"); | ||
| if (readableSegments.some((segment) => AMBIGUOUS_PLUGIN_SEGMENT_PATTERN.test(segment))) { |
There was a problem hiding this comment.
Narrow collision case the __ check misses.
This catches consecutive __ within a single segment (e.g. foo--bar → segment foo__bar), but the segments are joined with __, so two distinct scoped IDs where a dash sits adjacent to the slash boundary still collapse to the same generated name:
@a-/b→ segments["a-", "b"]→["a_", "b"]→a___b@a/-b→ segments["a", "-b"]→["a", "_b"]→a___b
Neither segment contains __ on its own, so the hash fallback is not triggered. Both plugins end up with a___b__<tool>, and the dedup in createMcpServer (registeredToolNames) silently drops the second one — meaning whichever plugin loaded first wins.
It requires fairly unusual plugin IDs (SIMPLE_ID/SCOPED_ID does allow leading/trailing dashes inside segments), so this is low-severity, but the existing check could be tightened to look at the joined readable id instead:
| if (readableSegments.some((segment) => AMBIGUOUS_PLUGIN_SEGMENT_PATTERN.test(segment))) { | |
| const readablePluginId = readableSegments.join("__"); | |
| if (/_{3,}/.test(readablePluginId) || readableSegments.some((segment) => AMBIGUOUS_PLUGIN_SEGMENT_PATTERN.test(segment))) { | |
| return `${readablePluginId}__${stableToolNameHash(pluginId)}__${toolName}`; | |
| } |
(Or just hash unconditionally for any plugin id whose readable form isn't reversibly recoverable.)
| hash = Math.imul(hash, 0x01000193); | ||
| } | ||
| return (hash >>> 0).toString(16).padStart(8, "0"); | ||
| } |
There was a problem hiding this comment.
Worth a small dedicated unit test file (tests/unit/mcp/plugin-tool-name.test.ts) to lock the contract in directly. The integration test in plugin-tools.test.ts covers the happy paths and one of the ambiguous cases, but the hash-fallback branch, leading-@ stripping, and edge cases like @a-/b vs @a/-b aren't directly asserted — a future refactor could change the generation scheme without breaking any test.
|
Review submitted successfully (review ID 4303523122) with three line-anchored comments and a summary body. Key findings (all non-blocking, DX/robustness rather than auth/correctness):
Prior reviewers' concerns (capability/route cross-validation, JSON Schema introspection, dual-check coverage, runtime dedup, admin-only intent comment, real-implementation testing) all look addressed. |
2 participants
What does this PR do?
Adds the approved plugin-defined MCP tool surface from Discussion #850:
mcp:toolsas an opt-in plugin capabilitymcpToolsmetadata to plugin manifests, descriptors, bundle extraction, and runtime plugin wiringADMINrole for plugin MCP tool callsRelated discussion: #850
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
pnpm typecheckpnpm lint(exits 0; reports 39 existing warnings, 0 errors)pnpm testgit diff --checkpnpm --filter emdash test -- tests/integration/mcp/plugin-tools.test.ts tests/unit/plugins/adapt-sandbox-entry.test.ts tests/unit/plugins/manifest-schema.test.ts tests/unit/plugins/define-plugin.test.ts tests/unit/cli/bundle-utils.test.tspnpm --filter @emdash-cms/registry-cli test -- bundle-utils.test.tsAdversarial review was run twice with RepoPrompt agent_run. The first pass found two issues around unloaded sandboxed tools and plugin ID tool-name collisions; both were fixed and the second pass found no blocking issues.