refactor: composable MCP tool registry (ROADMAP 3.5)#426
Merged
carlos-alm merged 12 commits intomainfrom Mar 12, 2026
Merged
Conversation
…shared/ modules
queries.js (2289 lines) decomposed into focused modules per roadmap 3.4:
src/shared/normalize.js — normalizeSymbol, kindIcon
src/shared/generators.js — iterListFunctions, iterRoles, iterWhere
src/analysis/symbol-lookup.js — findMatchingNodes, queryNameData, whereData,
listFunctionsData, childrenData
src/analysis/impact.js — impactAnalysisData, fnImpactData, diffImpactData,
diffImpactMermaid
src/analysis/dependencies.js — fileDepsData, fnDepsData, pathData
src/analysis/module-map.js — moduleMapData, statsData
src/analysis/context.js — contextData, explainData
src/analysis/exports.js — exportsData
src/analysis/roles.js — rolesData
queries.js is now a ~36-line barrel re-export for backward compatibility.
All 85 test files pass.
Resolve merge conflicts with main (v3.1.3 version bumps, changelog). Extract duplicated helpers into shared modules per Greptile review: - resolveMethodViaHierarchy → src/shared/hierarchy.js (was duplicated in context.js and dependencies.js) - safePath, readSourceRange, extractSummary, extractSignature, isFileLikeTarget → src/shared/file-utils.js (were in context.js, imported cross-module by exports.js) Impact: 61 functions changed, 138 affected
Address Greptile review: _getFileHash in symbol-lookup.js was identical to the private getFileHash in normalize.js. Export the original and import it instead of maintaining a duplicate. Impact: 2 functions changed, 6 affected
…s.js Deduplicate three identical getFileLines closures (two in context.js, one in exports.js) into a single createFileLinesReader factory in src/shared/file-utils.js. Remove now-unused fs/debug/safePath imports. Impact: 4 functions changed, 3 affected
No consumer imports ALL_SYMBOL_KINDS from this module — all callers import directly from kinds.js. The re-export was dead code that muddied normalize.js's responsibility.
…MAP 3.5)
Replace the 1,470-line src/mcp.js (31 tools in one switch dispatch) with
self-contained tool modules under src/mcp/. Each tool is now a single file
exporting { name, handler }. The original src/mcp.js becomes a 2-line
re-export shim preserving the public API (TOOLS, buildToolList, startMCPServer).
Structure: server.js (MCP lifecycle + dispatch), tool-registry.js (schemas),
middleware.js (pagination helpers), tools/ (31 handler files + barrel).
Adding a new MCP tool = adding a file + one barrel line.
Impact: 38 functions changed, 26 affected
|
Claude finished @carlos-alm's task —— View job PR Review: Composable MCP Tool Registry RefactorTodo List
Root Cause Analysis ✅Problem Being Solved: The PR addresses a legitimate architectural debt issue identified in ROADMAP 3.5. The monolithic 1,470-line Why This Approach: The refactor splits the monolith into self-contained modules following a clean pattern:
This is a well-executed vertical slice refactor that improves maintainability without breaking changes. Configuration Change Detection ✅Findings: No configuration rules relaxed or tests weakened
Test Integrity ✅API Preservation: The refactor maintains perfect backward compatibility:
Code Quality ✅Excellent Refactor Execution: ✅ Clean Modular Structure
✅ Proper Abstraction Layers
✅ Context Injection PatternSmart context injection avoiding circular dependencies: const ctx = { dbPath, getQueries, getDatabase, findDbPath, allowedRepos, MCP_MAX_LIMIT };
const result = await toolEntry.handler(args, ctx);✅ Smart Error Handling
✅ Future-Proof Design
Critical ConcernsNone. This is a textbook example of clean refactoring:
Final Recommendation |
Contributor
Greptile SummaryThis PR successfully completes ROADMAP 3.5: the 1,470-line monolithic Key observations:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant Server as server.js (CallToolRequestSchema)
participant Registry as tool-registry.js (buildToolList)
participant Barrel as tools/index.js (TOOL_HANDLERS Map)
participant Tool as tools/<name>.js (handler)
participant Ctx as ctx (dbPath, getQueries, getDatabase, findDbPath, MCP_MAX_LIMIT)
participant Upstream as src/*.js (queries, export, flow…)
Client->>Server: ListTools request
Server->>Registry: buildToolList(multiRepo)
Registry-->>Server: BASE_TOOLS [+ repo prop + list_repos]
Server-->>Client: { tools: [...] }
Client->>Server: CallTool { name, arguments }
Server->>Server: guard: multiRepo / allowedRepos / list_repos check
Server->>Server: resolve dbPath (registry.js if args.repo)
Server->>Barrel: TOOL_HANDLERS.get(name)
Barrel-->>Server: { name, handler }
Server->>Ctx: build ctx object
Server->>Tool: handler(args, ctx)
Tool->>Upstream: await import('../../<module>.js') [lazy]
Upstream-->>Tool: result
Tool-->>Server: result (plain object) or { content, isError }
Server-->>Client: { content: [{ type:'text', text: JSON }] }
Last reviewed commit: 919185e |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/mcp/tools/export-graph.js
Outdated
Comment on lines
+15
to
+53
| switch (args.format) { | ||
| case 'dot': | ||
| result = exportDOT(db, { fileLevel, limit: exportLimit }); | ||
| break; | ||
| case 'mermaid': | ||
| result = exportMermaid(db, { fileLevel, limit: exportLimit }); | ||
| break; | ||
| case 'json': | ||
| result = exportJSON(db, { | ||
| limit: exportLimit, | ||
| offset: effectiveOffset(args), | ||
| }); | ||
| break; | ||
| case 'graphml': | ||
| result = exportGraphML(db, { fileLevel, limit: exportLimit }); | ||
| break; | ||
| case 'graphson': | ||
| result = exportGraphSON(db, { | ||
| fileLevel, | ||
| limit: exportLimit, | ||
| offset: effectiveOffset(args), | ||
| }); | ||
| break; | ||
| case 'neo4j': | ||
| result = exportNeo4jCSV(db, { fileLevel, limit: exportLimit }); | ||
| break; | ||
| default: | ||
| db.close(); | ||
| return { | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: `Unknown format: ${args.format}. Use dot, mermaid, json, graphml, graphson, or neo4j.`, | ||
| }, | ||
| ], | ||
| isError: true, | ||
| }; | ||
| } | ||
| db.close(); |
Contributor
There was a problem hiding this comment.
Database connection leak on thrown exceptions
The db handle is opened manually and closed only on the happy path (db.close() on line 53) and on the default branch (line 42). If any of the export functions (exportDOT, exportMermaid, exportJSON, etc.) throws an unexpected exception, execution escapes to the server-level catch block in server.js and db.close() is never called. Over many calls in a long-running MCP session, SQLite connections accumulate and are never released.
A try/finally guard is needed:
Suggested change
| switch (args.format) { | |
| case 'dot': | |
| result = exportDOT(db, { fileLevel, limit: exportLimit }); | |
| break; | |
| case 'mermaid': | |
| result = exportMermaid(db, { fileLevel, limit: exportLimit }); | |
| break; | |
| case 'json': | |
| result = exportJSON(db, { | |
| limit: exportLimit, | |
| offset: effectiveOffset(args), | |
| }); | |
| break; | |
| case 'graphml': | |
| result = exportGraphML(db, { fileLevel, limit: exportLimit }); | |
| break; | |
| case 'graphson': | |
| result = exportGraphSON(db, { | |
| fileLevel, | |
| limit: exportLimit, | |
| offset: effectiveOffset(args), | |
| }); | |
| break; | |
| case 'neo4j': | |
| result = exportNeo4jCSV(db, { fileLevel, limit: exportLimit }); | |
| break; | |
| default: | |
| db.close(); | |
| return { | |
| content: [ | |
| { | |
| type: 'text', | |
| text: `Unknown format: ${args.format}. Use dot, mermaid, json, graphml, graphson, or neo4j.`, | |
| }, | |
| ], | |
| isError: true, | |
| }; | |
| } | |
| db.close(); | |
| let result; | |
| try { | |
| switch (args.format) { | |
| case 'dot': | |
| result = exportDOT(db, { fileLevel, limit: exportLimit }); | |
| break; | |
| case 'mermaid': | |
| result = exportMermaid(db, { fileLevel, limit: exportLimit }); | |
| break; | |
| case 'json': | |
| result = exportJSON(db, { | |
| limit: exportLimit, | |
| offset: effectiveOffset(args), | |
| }); | |
| break; | |
| case 'graphml': | |
| result = exportGraphML(db, { fileLevel, limit: exportLimit }); | |
| break; | |
| case 'graphson': | |
| result = exportGraphSON(db, { | |
| fileLevel, | |
| limit: exportLimit, | |
| offset: effectiveOffset(args), | |
| }); | |
| break; | |
| case 'neo4j': | |
| result = exportNeo4jCSV(db, { fileLevel, limit: exportLimit }); | |
| break; | |
| default: | |
| return { | |
| content: [ | |
| { | |
| type: 'text', | |
| text: `Unknown format: ${args.format}. Use dot, mermaid, json, graphml, graphson, or neo4j.`, | |
| }, | |
| ], | |
| isError: true, | |
| }; | |
| } | |
| } finally { | |
| db.close(); | |
| } |
…s handlers Impact: 2 functions changed, 0 affected
…m/optave/codegraph into refactor/queries-analysis-modules
Contributor
Author
|
Fixed the db connection leak in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/mcp.js(31 tools in one switch dispatch) into self-contained tool modules undersrc/mcp/{ name, handler }— adding a new MCP tool = adding a file + one barrel linesrc/mcp.jsbecomes a 2-line re-export shim preserving the public API (TOOLS,buildToolList,startMCPServer), so zero consumer/test changesserver.js(MCP lifecycle + dispatch),tool-registry.js(31 tool schemas),middleware.js(pagination helpers),tools/(31 handler files + barrel)Test plan
biome check— 0 errors, 0 warnings)codegraph cycles)codegraph build && codegraph check)