Skip to content

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

Closed
andreinknv wants to merge 1 commit into
colbymchenry:mainfrom
andreinknv:refactor/mcp-tool-registry
Closed

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

Conversation

@andreinknv
Copy link
Copy Markdown
Contributor

@andreinknv andreinknv commented Apr 27, 2026

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.


Summary

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.

What's new

File Purpose
src/mcp/tool-types.ts (NEW) Extracted ToolDefinition, ToolResult, PropertySchema, projectPathProperty so per-tool files import without circular deps.
src/mcp/tools/types.ts (NEW) ToolModule, HandlerKey string union, ToolHandlerLike interface — ToolHandler now implements ToolHandlerLike, making dispatch fully compile-time-checked.
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.
src/mcp/tools/registry.ts (NEW) Static-import barrel + lookup helpers + derived tools[] array.
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.
src/mcp/index.ts Tool-existence check switched from linear tools.find() scan to O(1) getToolModule() Map lookup.

Tests

387/387 pass. 7 new in __tests__/mcp-tool-registry.test.ts:

  • Definitions well-formed (name shape, description length)
  • handlerKey follows handle<UpperCase> convention
  • 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 — 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 enforcedToolHandler 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 the smoke test 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.

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

PR Change
#110 review-context Rebases to 1 new file + 2 lines in registry + 1 method on ToolHandler + 1 line in HandlerKey
#112 centrality+churn Same shape for codegraph_hotspots
#114 config-refs Same shape for codegraph_config
#115 sql-refs Same shape for codegraph_sql

Each 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).

Test plan

  • tsc --noEmit clean
  • npm test — 387/387 passing
  • End-to-end dispatch smoke test exercises real handler binding
  • Reviewer agent: REQUEST_CHANGES → APPROVE after fixes (with new test)
  • Backward compat: all existing exports preserved

🤖 Generated with Claude Code

…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>
@andreinknv
Copy link
Copy Markdown
Contributor Author

Part of a coordinated 4-PR refactor that unblocks the open PR backlog. See #120 for the full merge-order guide explaining how this PR fits into the picture and how each open language/feature PR rebases onto the new pattern.

@andreinknv
Copy link
Copy Markdown
Contributor Author

Maintainer review checklist (5-min path)

This refactor is behavior-preserving. Below is the shortest path to verify that without re-reading the patch.

1. What changed structurally

  • New src/mcp/tool-types.ts (39 lines) — extracted shared types so per-tool files import without circular dep
  • New src/mcp/tools/types.ts (50 lines) — ToolModule, HandlerKey union, ToolHandlerLike structural type
  • New src/mcp/tools/<name>.ts × 9 — one file per tool, ~25-30 lines each, all the tool definitions previously inlined in tools.ts
  • New src/mcp/tools/registry.ts (65 lines) — alphabetised barrel export
  • Shrunk src/mcp/tools.ts (-200 lines net): inline definitions and the case-switch deleted; execute() does this[mod.handlerKey](args) with compile-time-checked dispatch via implements ToolHandlerLike

2. What stays exactly the same — verify in seconds

Public surface File:line Status
ToolDefinition, ToolResult types src/mcp/tools.ts:20 re-exported from tool-types.ts
tools[] array consumed by list_tools src/mcp/tools.ts:32 now derived from registry, same shape, same order via alpha sort
getExploreBudget(fileCount) src/mcp/tools.ts:42 unmoved
ToolHandler class src/mcp/tools.ts:71 same methods, now public instead of private so the structural interface can match

Existing imports import { ToolDefinition, ToolResult, tools, ToolHandler } from './tools' keep working unchanged. Grep confirms no consumer touches the formerly-private handler methods.

3. Invariants now enforced by tests (__tests__/mcp-tool-registry.test.ts, 7 tests)

  • ✅ Every registered handlerKey resolves to a real method on ToolHandler — adding a tool with a typo'd handler name fails CI loudly
  • ✅ The exported tools[] exactly mirrors the registry — drift between the two surfaces is impossible
  • ✅ All 9 canonical tools registered (regression guard against accidental deletion)
  • ✅ End-to-end dispatch smoke: execute('codegraph_status', {}) reaches the real handler body — guards against this-binding regressions in the dynamic dispatch
  • execute() returns the standard error result for unknown tools

4. Reviewer pass — caught and fixed before this comment

  1. ToolHandlerLike was defined but unused → ToolHandler implements ToolHandlerLike now (compile-time dispatch check; eliminates the unknown cast)
  2. No e2e dispatch test → added (test Codegraphe does not work under Node v24. #5 above)
  3. MCPServer.handleToolsCall did a linear tools.find() while execute() used a Map → switched to getToolModule() for parity
  4. Redundant .slice() in registry.ts removed (map() already returns fresh)

5. Mergeability

If checklist sections 1-4 look right, this is safe to land. Happy to walk through any specific file in more detail.

🤖 Comment template generated with Claude Code

andreinknv added a commit to andreinknv/codegraph that referenced this pull request Apr 28, 2026
…om a diff

Adds a new MCP tool that takes a unified diff and returns structured
review context for an LLM-driven PR reviewer. Codegraph becomes a
substrate for Greptile/CodeRabbit-style products without itself doing
the synthesis.

## What it returns

Per changed file:
  - status (added / modified / deleted / renamed)
  - hunks (line ranges)
  - affected symbols (line-range overlap with hunks)
  - tests covering the file (via tests-edges; graceful no-op if absent)

Per affected symbol:
  - signature, docstring
  - top-N callers, top-N callees
  - impact-radius node count

## Components

- src/review/diff-parser.ts: pure unified-diff parser
- src/review/index.ts: buildReviewContext + co-change warnings
- src/index.ts: CodeGraph.buildReviewContext public API
- src/mcp/tools/review-context.ts: ToolModule (post-colbymchenry#117 form)
- src/mcp/tools.ts: handleReviewContext + serializeReviewContextWithinCap
  (progressive-trim JSON serializer that keeps output <= MAX_OUTPUT_LENGTH
  while preserving JSON parseability)
- __tests__/review-context.test.ts: 25 tests

Output is JSON; the LLM consumer does the synthesis.

This is the post-colbymchenry#117 (per-tool MCP registry) form — the original
PR's case-switch dispatch is replaced by a ToolModule entry plus
a 'handleReviewContext' HandlerKey.

Tests that exercise co-change warnings include a graceful inline
ALTER TABLE for commit_count, since that column lands with colbymchenry#112's
centrality+churn migration. After colbymchenry#112 lands the ALTER becomes a
no-op (column already exists).
andreinknv added a commit to andreinknv/codegraph that referenced this pull request Apr 28, 2026
…om a diff

Adds a new MCP tool that takes a unified diff and returns structured
review context for an LLM-driven PR reviewer. Codegraph becomes a
substrate for Greptile/CodeRabbit-style products without itself doing
the synthesis.

Per changed file:
  - status (added / modified / deleted / renamed)
  - hunks (line ranges)
  - affected symbols (line-range overlap with hunks)
  - tests covering the file (via tests-edges; graceful no-op if absent)

Per affected symbol:
  - signature, docstring
  - top-N callers, top-N callees
  - impact-radius node count

- src/review/diff-parser.ts: pure unified-diff parser
- src/review/index.ts: buildReviewContext + co-change warnings
- src/index.ts: CodeGraph.buildReviewContext public API
- src/mcp/tools/review-context.ts: ToolModule (post-colbymchenry#117 form)
- src/mcp/tools.ts: handleReviewContext + serializeReviewContextWithinCap
  (progressive-trim JSON serializer that keeps output <= MAX_OUTPUT_LENGTH
  while preserving JSON parseability)
- __tests__/review-context.test.ts: 25 tests

Output is JSON; the LLM consumer does the synthesis.

This is the post-colbymchenry#117 (per-tool MCP registry) form — the original
PR's case-switch dispatch is replaced by a ToolModule entry plus
a 'handleReviewContext' HandlerKey.

Tests that exercise co-change warnings include a graceful inline
ALTER TABLE for commit_count, since that column lands with colbymchenry#112's
centrality+churn migration. After colbymchenry#112 lands the ALTER becomes a
no-op (column already exists).
@colbymchenry
Copy link
Copy Markdown
Owner

Closing — this is well-engineered but I'm not adopting the per-file MCP tool organization. The refactor's primary value is reducing conflict surface for stacked future PRs (cited #110/#112/#114/#115 all from this same flood), and I've been declining most of those on independent grounds. Without those PRs merging, the refactor is net file-count growth for organizational change I didn't ask for.

If I decide later that I want per-file MCP tool organization, I'd rather scope and design that myself. Thanks for the work.

colbymchenry added a commit that referenced this pull request May 8, 2026
Adds a universal tool-selection playbook surfaced by MCP clients
(Claude Code, Cursor, opencode, LangChain, OpenAI Agent SDK) in the
agent's system prompt automatically. Without this, agents have to
infer tool composition from individual tool descriptions and tend to
walk callers manually instead of reaching for codegraph_impact, etc.

Scoped tight: only the 9 tools that exist on main today
(search/context/callers/callees/impact/node/explore/files/status), no
"(when present)" references to unmerged tools, no per-language
guidance. ~40 lines of useful guidance.

Salvaged from #121, which bundled the instructions with #117's MCP
tool-registry refactor and referenced many tools that don't exist on
main.

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