feat(plugins): let plugins expose MCP tools - #1752
Conversation
TREK already runs an MCP server with ~200 built-in tools, but plugins had no way into it. A plugin can now declare `mcpTools` and have them advertised alongside the built-ins, so an assistant connected over MCP can call into it. This is a top-level definition section like `routes`/`jobs`, not a provider hook: the declarations are reported once at load and served from memory when a session starts (registerTools is synchronous), while handlers run per call over the existing supervisor RPC as `invoke.mcpTool`. Design notes: - Tools are namespaced `plugin_<pluginId>_<name>`. Plugin ids carry no underscore and tool names no dash, so cross-plugin collisions are impossible; a name already taken by a built-in is skipped rather than allowed to win. - Schemas cross the process boundary as plain JSON Schema (the SDK stays dependency-free) and are converted to Zod host-side, since registerTool takes no raw JSON Schema. The conversion is deliberately permissive — it is what arguments are validated against, so anything unrecognized becomes z.unknown() rather than rejecting a call the plugin would have handled. Handlers receive input unvalidated and are documented to check it. - Two gates: the plugin needs `mcp:tools` (enforced host-side in the supervisor, like the hook:* grants, since the child reports declarations knowing nothing about grants), and the session's token needs the new `plugins:use` OAuth scope. One scope covers all plugin tools — what any tool can reach is already bounded by that plugin's own grants, acting as the token's user. - Handlers run with the calling user bound (like a route) on a 30 s timeout; throws, timeouts and mid-session deactivation come back as MCP tool errors. - Activating or deactivating a plugin invalidates live MCP sessions, but only when the advertised surface actually changed (liketrek#1414). Declarations are capped and sanitized before they are ever advertised (16 tools per plugin, name/title/description/schema limits) and anything over a cap is dropped whole rather than truncated into a tool whose description no longer matches what it does.
The MCP SDK does validate tools/call arguments against the converted schema before the handler runs — permissively, since json-schema-to-zod turns anything it half-understands into an unconstrained value. So the honest claim is 'validation is not guaranteed', not 'input arrives unvalidated': the mock host is deliberately MORE permissive than production, never the reverse. Reword the SDK docs, mock-host driver, and wiki to match, and refresh the stale scope count (30 across 15) while touching MCP-Scopes.md anyway.
|
Hi, I will put that on hold for now. I'm renovating the backend, and the MCP and plugin part is still not finished. I'll let you know once everything's good backend wise. Once it's good, we'll be able to approach such kind of feature. Either way, please review carefully the contribution guidelines, while, yes this is a good idea, if everyone push some features that has not been discussed beforehand we'll then just start closing them without even looking at the submitted PRs. |
|
Understood, make sense! |
|
Architecturally this is the cleanest of the plugin PRs I have open — declaration and handler kept apart, grants enforced host-side, handoff instead of a cycle, and the tests go at the parts that actually matter. I'm not going to review it properly yet, and that's on us rather than on you: @jubnl's note from 30 July still stands — the MCP layer and the plugin runtime are both mid-rebuild, and this PR marries exactly those two. Reviewing it against a target that's still moving would waste your time and mine. No deadline on this one — it's blocked by our work, not by yours. I'll come back to you when the rebuild has settled. Two things to be aware of for when we pick it up, so they don't come as a surprise:
Also, for future work: CONTRIBUTING asks for a pitch on Discord before large features. Not a problem this time — but for something of this size it saves you the risk of building against a subsystem we're about to change. Thanks for the patience. |
mauriceboe
left a comment
There was a problem hiding this comment.
Thanks for the patience on this one, and sorry it took a while to come back to you. The feature is one I want: letting a plugin publish its own MCP tools is the logical next step after the RPC surface, and the way you gated it (a mcp:tools plugin permission enforced host side, plus a separate OAuth scope, plus host side normalisation and caps on what comes back) is the right instinct.
The problem is timing, not craft. The branch dates from 29 July and dev has moved about 200 commits since, most of them straight through the code this PR touches. So the six conflicts understate the situation.
What actually changed underneath
server/src/services/no longer exists.server/src/nest/plugins/plugin-runtime.service.tsimports from it twice in this branch (../../services/notifications/channelRegistryand../../services/apiKeyCrypto). Both moved into the container: the channel registry isnest/notifications/channel-registry.ts, and the crypto helper isnest/common/crypto/apiKeyCrypto.ts. There is now an ESLintno-restricted-importsrule that fails the build on anyservices/path, so this is not something a merge resolution can paper over.- The MCP tool registrars are gone.
server/src/mcp/tools.tsin this branch imports eight modules that no longer exist (./tools/journey,mapsWeather,notifications,atlas,places,collections,transports,prompts). All 199 tools and all 4 prompts are decorated@Tool/@Prompthandlers on<domain>/<domain>.mcp.tsnow, andmcp/tools.tsis down to the registry handoff. Resolving that conflict by keeping your side breaks the build. - The permission lists are generated.
plugin-sdk/src/manifest.tsandplugin-sdk/src/permissions.tsused to carry hand maintained arrays; those come from@trek/sharedand the generator now. Three of your six conflicts disappear if you drop the hunk instead of merging it, then runnode --import tsx server/scripts/gen-plugin-facts.tsonce.plugin-sdk/src/generated/host-facts.tsandshared/src/plugin-permissions.tsare not regenerated in the branch either, so thePlugin Permission Factsjob fails as it stands. PluginsModulewas split into four (PluginsRuntimeModule,PluginContributionsModule,PluginOAuthModuleand a composition root), and there is a boot time coverage check that refuses to start if a domain owning part of the wire surface is not reachable. Worth readingserver/src/nest/plugins/host/rpc-kit/README.mdbefore you rewire anything.
One ordering bug that survives the rebase
After the merge, registerPluginTools runs before registry.attach(), which inverts the shadowing guarantee the PR documents: plugin tools would be registered first and builtins would win or lose depending on SDK behaviour rather than on our rule. Decide that order deliberately and pin it with a test. (For what it is worth, no builtin tool name starts with plugin, so nothing is being shadowed today. It is the invariant I care about, not a live incident.)
Also: the coverage gate for src/nest/plugins/** is no longer a flat 80. It sits at 86/81/78/89, and invokeMcpTool and syncMcpToolSurface are currently untested.
Three decisions before the rework, not after
I do not want you to spend a day rebasing and then find out one of these goes the other way:
- Should
mcpToolsbe a public field on the plugin definition, or should it go through the same decorator route the rest of the host surface took? The RPC side deliberately moved away from hand declared maps. - The hand rolled JSON Schema to Zod converter. I would rather depend on something existing or restrict the accepted schema subset than maintain a converter.
- The
plugins:usescope: whether plugin tools are one scope or inherit per plugin granularity.
Answer those on the PR and I will confirm before you touch the code. Sorry to ask for a second pass on something that was already finished once; the ground genuinely moved.
|
Following up on my own review: I am closing this rather than asking you to rebase it, and I think that is the friendlier answer. Everything I listed above comes down to the same thing, that the branch predates the NestJS rework. That rework is now essentially done: So: please open a fresh PR from current Two things to do before you write any of it, so nothing is wasted:
Thanks for the work, and sorry the ground moved this much under an open PR. Ping me on the new one and I will get to it quickly. |
|
Fair call, I would rather rewrite against the current tree than drag 79 files across it. Answers before I touch anything. 1. The host side is where you are right and the PR is wrong. Your call: that hook in nest-mcp, or the plugins domain keeps its own pass with the order pinned by a test? I prefer the first but it is your package. 2. The converter. I think it can be deleted rather than replaced. The trade: 3. No questions on the rest: I will move Tell me which way on 1 and 2 and I will start. |
Plugins can now put their own tools on TREK's MCP server. A
mcpToolssection onthe plugin definition (same shape as
routes/jobs) declares name, descriptionand a JSON schema; the handler stays in the plugin child and runs as the calling
user with a 30s budget. Assistants see them as
plugin_<id>_<name>next to thebuilt-ins.
Two gates: the admin grants the plugin the new
mcp:toolspermission, and thesession token carries the new
plugins:useOAuth scope (full-access tokensqualify). The permission grants no data access of its own — a tool still only
reaches what the plugin's other grants allow, membership-checked against the
calling user.
Worth knowing:
anything over a cap is dropped whole rather than truncated into a tool that no
longer matches its description
converter doesn't understand become "unconstrained" instead of rejecting calls
the plugin would have handled; handlers are documented to validate their own input
tool surface actually changed
mcpTool()driver so authors can test handlers without arunning TREK
Wiki updated (Plugin-Development, Cookbook, Permissions, MCP-Scopes,
MCP-Tools-and-Resources) and the new scope/permission strings translated in all
24 locales. Typechecks and unit tests green across server, plugin-sdk and client.