refactor(mcp): collapse tool wiring into a single registry#28
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Refactors the MCP tool wiring in server/mcp.ts to use a single tool registry entry per tool, consolidating schema, visibility rules, dispatch, and (for execution tools) permission enforcement into one place to reduce multi-site edits.
Changes:
- Introduces a
ToolDefregistry (TOOLS) withunlock+handlerand aTOOLS_BY_NAMElookup map. - Replaces the hardcoded
selectToolNamesvisibility chain with a sharedisUnlocked/unlockedToolsfilter used by both listing and selection. - Replaces the
dispatchToolswitch with registry-based dispatch and a sharedexecuteToolhandler for execution tools.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function executeTool(principal: Principal, args: Record<string, unknown>, def: ToolDef) { | ||
| return execute(principal, def.name, def.unlock as Permission, args) | ||
| } |
There was a problem hiding this comment.
Addressed in 25cf6e7. Note this thread reviewed the first commit (e25c751): the unlock: 'always' | 'accessible' | Permission sentinel union is gone — visibility is now a visible predicate and execution tools carry an optional permission set via requires(), so a sentinel string can no longer reach the permission path. The residual non-null assertion (def.permission!) is now narrowed through a toolPermission(def) guard that throws a clear "misconfigured" error and removes the cast.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Shared handler for the execution tools (query/write_data/run_ddl). The disjoint permission to | ||
| // enforce is the tool's own `unlock`, so it lives in one place — the registry entry. | ||
| function executeTool(principal: Principal, args: Record<string, unknown>, def: ToolDef) { | ||
| return execute(principal, def.name, def.unlock as Permission, args) | ||
| } |
There was a problem hiding this comment.
as Permission cast in executeTool silently allows any future ToolDef with unlock: 'always' or unlock: 'accessible' to pass those sentinel strings directly into the permission enforcement path. TypeScript won't catch the misuse, and the resulting runtime error (Permission denied: ... requires 'always' permission) will be confusing. A narrow overload or a dedicated ExecuteToolDef type with unlock: Permission would make this invariant compiler-enforced.
| // Shared handler for the execution tools (query/write_data/run_ddl). The disjoint permission to | |
| // enforce is the tool's own `unlock`, so it lives in one place — the registry entry. | |
| function executeTool(principal: Principal, args: Record<string, unknown>, def: ToolDef) { | |
| return execute(principal, def.name, def.unlock as Permission, args) | |
| } | |
| // Shared handler for the execution tools (query/write_data/run_ddl). The disjoint permission to | |
| // enforce is the tool's own `unlock`, so it lives in one place — the registry entry. | |
| // `unlock` must be a Permission here; a sentinel ('always'/'accessible') would produce a | |
| // confusing runtime error. Narrow the type so the compiler enforces the invariant. | |
| interface ExecuteToolDef extends ToolDef { | |
| unlock: Permission | |
| } | |
| function executeTool(principal: Principal, args: Record<string, unknown>, def: ExecuteToolDef) { | |
| return execute(principal, def.name, def.unlock, args) | |
| } |
There was a problem hiding this comment.
Addressed in 25cf6e7. The as Permission cast no longer exists (this reviewed e25c751, before the sentinel union was replaced by a visible predicate + optional permission). On the suggested ExecuteToolDef extends ToolDef narrowing: dispatchTool invokes every handler through the generic ToolDef['handler'] signature, so a handler typed to the narrower subtype won't cleanly assign under strictFunctionTypes. Instead, toolPermission(def) fails fast with a clear message if an execution tool is wired without a permission.
e25c751 to
08ad3e4
Compare
08ad3e4 to
635bac6
Compare
Each MCP tool was defined across four places — the TOOLS map, the
hardcoded selectToolNames visibility chain, the dispatchTool switch, and
the expectedPerm literal in each execute() call — with the permission
level restated in three of them. Adding or changing a tool meant editing
all four in lockstep.
Replace them with one ToolDef per tool: { name, description, inputSchema,
unlock, handler }. `unlock` is declared once and drives both visibility
and enforcement:
- 'always' → always advertised (list_connections)
- 'accessible' → needs >=1 reachable connection (catalog browsing)
- a Permission → advertised when held on >=1 connection, and reused as
the disjoint permission execute() enforces
selectToolNames/listToolsFor share one isUnlocked filter; dispatchTool is
a TOOLS_BY_NAME lookup instead of a switch; the three execution tools
share one executeTool handler that reads the enforced permission from
def.unlock. No behavior change — exported signatures preserved, all 32
mcp tests pass, tsc clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
635bac6 to
538d41b
Compare
Address PR review: the registry's TOOLS_BY_NAME map would silently overwrite on a duplicate `name`, and executeTool/explainQuery asserted `def.permission!`. Build the map with an explicit duplicate check, and narrow the permission through a `toolPermission()` guard that throws a clear "misconfigured" error instead of letting `undefined` flow into the enforcement path. Both are registry-construction invariants, caught at module load / call time rather than as opaque downstream errors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
Each MCP tool was defined across four places that had to stay in lockstep:
TOOLSmap (schema)selectToolNamesvisibility chaindispatchToolswitchexpectedPermliteral in eachexecute(...)callThe permission level was restated in three of them, so adding or changing a tool meant a four-site edit.
Change
Collapse all four into one descriptor per tool, where visibility is a predicate over the agent's access:
Registry entries:
requires(perm)returns{ visible: a => a.union.has(perm), permission: perm }, so an execution tool's permission is written once and drives both visibility and enforcement (executeTool/explainQueryreaddef.permission). This avoids the earlier overloaded single field that mixed access-tier sentinels with real permission values — honest about the fact that permissions are disjoint, not hierarchical.Supporting mechanics:
selectToolNames/listToolsForshare onevisibleTools(access)filterdispatchToolis aTOOLS_BY_NAMElookup instead of a switchBehavior
No behavior change. Dispatch stays by-name (visibility filtering remains in
tools/list; each handler re-checks its permission, so an unadvertised tool called directly stays gated — fail-closed intact). Exported signaturesselectToolNames/dispatchTool/Principalare unchanged.Verification
tests/mcp.test.ts— 32/32 pass, unchangedtsc --noEmit— cleanAdding a future tool is now a single array entry.
🤖 Generated with Claude Code