Skip to content

refactor: per-tool MCP registry — eliminate tools[] + case-switch conflicts#19

Open
mschreib28 wants to merge 1 commit into
mainfrom
upstream/refactor/mcp-tool-registry
Open

refactor: per-tool MCP registry — eliminate tools[] + case-switch conflicts#19
mschreib28 wants to merge 1 commit into
mainfrom
upstream/refactor/mcp-tool-registry

Conversation

@mschreib28
Copy link
Copy Markdown
Owner

Reviewers: for a 5-minute review path that surfaces only what to verify, see the review checklist comment below. The original description is preserved here for full technical detail.\n\n---\n\n## Summary\n\nToday every PR adding an MCP tool conflicts on the same two shared lists in src/mcp/tools.ts: the tools[] array (the list_tools surface) and the case switch in execute(). After this refactor:\n\nAdding a new MCP tool:\n\n1. Drop a file at src/mcp/tools/<name>.ts exporting a <NAME>_TOOL: ToolModule (definition + handlerKey).\n2. Add one import line and one array entry to src/mcp/tools/registry.ts.\n3. Implement handle<Name>(args) on ToolHandler in tools.ts and add the new key to HandlerKey in tools/types.ts.\n\nStep 3 is the only remaining "shared method on a single class" conflict surface. Extracting handler bodies into per-tool files (making step 3 also a single-file addition) is left as a follow-up — the cost/benefit favors landing this incremental win now.\n\n## What's new\n\n| File | Purpose |\n|---|---|\n| src/mcp/tool-types.ts (NEW) | Extracted ToolDefinition, ToolResult, PropertySchema, projectPathProperty so per-tool files import without circular deps. |\n| src/mcp/tools/types.ts (NEW) | ToolModule, HandlerKey string union, ToolHandlerLike interface — ToolHandler now implements ToolHandlerLike, making dispatch fully compile-time-checked. |\n| src/mcp/tools/<name>.ts × 9 (NEW) | One file per existing tool: callees, callers, context, explore, files, impact, node, search, status. ~25-30 lines each. |\n| src/mcp/tools/registry.ts (NEW) | Static-import barrel + lookup helpers + derived tools[] array. |\n| src/mcp/tools.ts | ~200 lines deleted (inline types + tools[] array). execute() rewrites the case-switch to a registry lookup with type-safe this[mod.handlerKey](args) dispatch. private async handle* → public async handle* to match the interface. |\n| src/mcp/index.ts | Tool-existence check switched from linear tools.find() scan to O(1) getToolModule() Map lookup. |\n\n## Tests\n\n387/387 pass. 7 new in __tests__/mcp-tool-registry.test.ts:\n\n- Definitions well-formed (name shape, description length)\n- handlerKey follows handle<UpperCase> convention\n- Every registered handlerKey resolves to a real method on ToolHandler\n- Exported tools[] exactly mirrors the registry\n- Canonical 9 main-line tools regression guard\n- execute() unknown-tool error path\n- End-to-end dispatch smoke test: execute('codegraph_status', {}) reaches the real handler body — would fail loudly if the dynamic dispatch chain ever breaks\n\n## Reviewer pass\n\nIndependent reviewer ran once. 2 REQUEST_CHANGES + 2 INFO addressed:\n\n1. ToolHandlerLike was defined but never enforcedToolHandler now implements ToolHandlerLike. Eliminates the (this as unknown as Record<...>) cast in execute(); dispatch is fully compile-time-checked.\n2. No end-to-end dispatch test — added the smoke test above.\n3. MCPServer.handleToolsCall used a linear tools.find() scan while execute() used Map lookup — switched to getToolModule() for parity.\n4. Removed redundant .slice() in registry.ts.\n\n## Backward compat\n\nsrc/mcp/tools.ts still re-exports ToolDefinition, ToolResult, the mutable tools[] array, ToolHandler, and getExploreBudget. Every existing consumer (import { ToolDefinition, ToolResult, tools, ToolHandler } from './tools') keeps working unchanged.\n\n## Affected open PRs\n\n| PR | Change |\n|---|---|\n| colbymchenry#110 review-context | Rebases to 1 new file + 2 lines in registry + 1 method on ToolHandler + 1 line in HandlerKey |\n| colbymchenry#112 centrality+churn | Same shape for codegraph_hotspots |\n| colbymchenry#114 config-refs | Same shape for codegraph_config |\n| colbymchenry#115 sql-refs | Same shape for codegraph_sql |\n\nEach goes from a 4-way conflict (tools[] + case + handler + helpers) down to a 1-way conflict (HandlerKey + handler method on ToolHandler, both in tools.ts).\n\n## Test plan\n\n- [x] tsc --noEmit clean\n- [x] npm test — 387/387 passing\n- [x] End-to-end dispatch smoke test exercises real handler binding\n- [x] Reviewer agent: REQUEST_CHANGES → APPROVE after fixes (with new test)\n- [x] Backward compat: all existing exports preserved\n\n🤖 Generated with Claude Code\n\n


Copied from colbymchenry/codegraph#117

…flicts

Today every PR adding an MCP tool conflicts on the same two
shared lists in src/mcp/tools.ts: the tools[] array (the
list_tools surface) and the case switch in execute(). After this
refactor:

  Adding a new MCP tool:
  1. Drop a file at src/mcp/tools/<name>.ts exporting a
     <NAME>_TOOL: ToolModule (definition + handlerKey).
  2. Add one import line and one array entry to
     src/mcp/tools/registry.ts.
  3. Implement handle<Name>(args) on ToolHandler in tools.ts and
     add the new key to HandlerKey in tools/types.ts.

Step 3 is the only remaining "shared method on a single class"
conflict surface. Extracting handler bodies into per-tool files
(making step 3 also a single-file addition) is left as a
follow-up — the cost/benefit favors landing this incremental win
now and finishing the body extraction once language and migration
refactors land.

## What's new

- **src/mcp/tool-types.ts** — extracted ToolDefinition, ToolResult,
  PropertySchema, projectPathProperty into a shared module so
  per-tool files can import without circular dependency.
- **src/mcp/tools/types.ts** — ToolModule interface, HandlerKey
  string union, and ToolHandlerLike (a structural type that
  ToolHandler now `implements`, providing compile-time guarantee
  that every HandlerKey maps to a real method).
- **src/mcp/tools/<name>.ts × 9** — one file per existing tool
  (callees, callers, context, explore, files, impact, node, search,
  status). Each ~25-30 lines: import + definition literal +
  handlerKey reference.
- **src/mcp/tools/registry.ts** — static-import barrel, sorted
  alphabetically. Exports getToolModules(), getToolModule(name),
  and the derived `tools[]` array.
- **src/mcp/tools.ts** — ~200 lines deleted from the top
  (inline types + tools[] array + projectPathProperty).
  execute()'s case-switch replaced with a registry lookup +
  type-safe `this[mod.handlerKey](args)` dispatch (now compile-
  time-checked thanks to `implements ToolHandlerLike`).
  All `private async handle*` methods now public to match the
  interface. errorResult/textResult also public for the same reason.
- **src/mcp/index.ts** — MCPServer's tool-existence check switched
  from a linear `tools.find()` scan to the O(1) `getToolModule()`
  Map lookup, eliminating two parallel lookup paths.

## Tests

387/387 pass. **7 new tests** in __tests__/mcp-tool-registry.test.ts:
- Definitions are well-formed (name shape, description length).
- handlerKey shape (`handle<UpperCase>`).
- Every registered handlerKey resolves to a real method on
  ToolHandler.
- Exported `tools[]` exactly mirrors the registry.
- Canonical 9 main-line tools regression guard.
- execute() unknown-tool error path.
- **End-to-end dispatch smoke test**: execute('codegraph_status', {})
  reaches the real handler body (no broken `this` binding) — would
  fail loudly if the dynamic dispatch chain ever breaks.

## Reviewer pass

Independent reviewer ran once. 2 REQUEST_CHANGES + 2 INFO addressed:

1. ToolHandlerLike was defined but never enforced —
   ToolHandler now `implements ToolHandlerLike`. Eliminates the
   `(this as unknown as Record<...>)` cast in execute(); dispatch
   is fully compile-time-checked.
2. No end-to-end dispatch test — added one (see Tests above).
3. MCPServer.handleToolsCall used a linear `tools.find()` scan
   while execute() used Map lookup — switched to getToolModule()
   for parity.
4. Removed redundant .slice() in registry.ts (map() already
   returns a fresh array).

## Backward compat

src/mcp/tools.ts still re-exports ToolDefinition, ToolResult, the
mutable `tools[]` array, ToolHandler, and getExploreBudget. Every
existing consumer (`import { ToolDefinition, ToolResult, tools,
ToolHandler } from './tools'`) keeps working unchanged.

## Affected open PRs

- colbymchenry#110 (review-context): rebases to 1 new file in tools/ + 2
  lines in registry.ts + 1 method on ToolHandler + 1 line in
  HandlerKey.
- colbymchenry#112 (centrality+churn): same shape for the codegraph_hotspots
  tool.
- colbymchenry#114 (config-refs): same shape for codegraph_config.
- colbymchenry#115 (sql-refs): same shape for codegraph_sql.

Each goes from 4-way conflict (tools[] + case + handler + helpers)
down to 1-way conflict (HandlerKey + handler method on ToolHandler,
both in tools.ts).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

2 participants