refactor: consolidate MCP tools to match CLI from PR #280#292
refactor: consolidate MCP tools to match CLI from PR #280#292carlos-alm merged 4 commits intomainfrom
Conversation
Add Phase 2.7 (Deep Analysis & Graph Enrichment) covering all features shipped since the last review: dataflow analysis, intraprocedural CFG, AST node storage, expanded node/edge types (13 kinds, 9 edge kinds), extractors refactoring, CLI consolidation, interactive viewer, exports command, and normalizeSymbol utility. Update Phase 3 priorities: add unified AST analysis framework as new #1 critical item (5,193 lines of parallel AST walking across complexity/ CFG/dataflow/ast with no shared infrastructure). Update all metrics across Phase 3 items (50 modules, 26K lines, 13 tables, 34 MCP tools). Mark qualified names as partially addressed (parent_id + contains edges). Subsume standalone complexity decomposition into AST framework. Update Phase 4 type definitions with new kind/edge type hierarchies. Mark Phase 9.1 visualization as partially complete (plot command).
- Update extractor line counts from 2,299 to actual 3,023 (verified via wc -l) - Fix individual per-file counts in both ROADMAP.md and architecture.md - Remove duplicate section 3.8 numbering for strikethrough/subsumed item
Remove 3 redundant MCP tools and add 1 new tool to mirror the CLI consolidation done in PR #280: - Remove `explain` tool → fold into `audit` with `quick` param - Remove `hotspots` tool → fold into `triage` with `level` param - Remove `manifesto` tool → fold into `check` with manifesto-mode routing - Add standalone `path` tool (was only accessible via query mode=path) All backing data functions are unchanged. The `query` tool still supports mode=path for backward compatibility. Impact: 1 functions changed, 1 affected
|
Claude finished @carlos-alm's task —— View job Reviewing PR #292: MCP tools consolidation Review Progress
Analysis ResultsPhase 1: Root Cause Analysis ✅Problem Being Solved: This PR consolidates MCP tools to mirror CLI consolidation from PR #280, which removed redundant CLI commands by folding functionality into existing commands. Why This Approach: Maintains consistency between CLI and MCP interfaces. Rather than having separate Architectural Justification: Sound approach - reduces API surface while preserving all functionality. Backward compatibility is maintained via parameter routing. Phase 2: Configuration Change Detection ✅No High-Risk Changes Detected:
Phase 3: Test Integrity ✅Test Coverage Analysis:
Test Quality: Excellent - comprehensive schema validation for all consolidated tools. Phase 4: Code Quality Review ✅Implementation Analysis (src/mcp.js:1206-1352): ✅ if (args.quick) {
result = explainData(args.target, dbPath, { /* options */ });
} else {
const { auditData } = await import('./audit.js');
result = auditData(args.target, dbPath, { /* full options */ });
}✅ if (args.level === 'file' || args.level === 'directory') {
const { hotspotsData } = await import('./structure.js');
// Correctly maps 'risk' sort to 'fan-in' for hotspots
const metric = args.sort === 'risk' ? 'fan-in' : args.sort;
}✅
✅ New Backward Compatibility: Perfect - Code Quality: High - clean separation of concerns, proper lazy imports, consistent error handling. Root Cause AnalysisProblem Being Solved: Maintaining consistency between CLI and MCP interfaces after CLI consolidation in PR #280. The CLI removed redundant commands by folding functionality into existing commands with parameters. Why This Approach: The consolidation reduces API surface area while preserving all functionality. This is a principled refactoring that eliminates duplicate endpoints without losing capabilities. Risk Assessment: Low risk - purely structural changes with comprehensive test coverage and backward compatibility preservation. Critical ConcernsNone identified. This is a well-executed refactoring that:
Final Recommendation
Key Strengths:
Minor Notes:
|
Greptile SummaryThis PR successfully consolidates MCP tools to mirror the CLI changes from PR #280, reducing tool count from 34 to 31 while preserving backward compatibility through parameter-based routing. Key changes:
Implementation highlights:
Testing:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[MCP Client] --> B{Tool Called}
B -->|audit| C{quick param?}
C -->|true| D[explainData]
C -->|false| E[auditData]
B -->|triage| F{level param?}
F -->|function| G[triageData]
F -->|file/directory| H[hotspotsData with<br/>TRIAGE_TO_HOTSPOT<br/>mapping]
B -->|check| I{ref or staged?}
I -->|yes| J[checkData]
I -->|no| K{rules param?}
K -->|true| L[checkData +<br/>manifestoData]
K -->|false| M[manifestoData only]
B -->|path| N[pathData]
H -->|risk→fan-in<br/>complexity→density<br/>churn→coupling<br/>mi→fan-in| O[Valid Hotspot Metric]
Last reviewed commit: a1583cb |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/mcp.js
Outdated
| }); | ||
| if (args.level === 'file' || args.level === 'directory') { | ||
| const { hotspotsData } = await import('./structure.js'); | ||
| const metric = args.sort === 'risk' ? 'fan-in' : args.sort; |
There was a problem hiding this comment.
When sort is 'complexity', 'churn', or 'mi', these metrics don't exist in hotspotsData's HOTSPOT_QUERIES (which only supports 'fan-in', 'fan-out', 'density', 'coupling'). The function will fall back to 'fan-in', silently ignoring the user's sort preference.
| const metric = args.sort === 'risk' ? 'fan-in' : args.sort; | |
| const metric = args.sort === 'risk' ? 'fan-in' : args.sort === 'complexity' ? 'density' : args.sort === 'churn' ? 'coupling' : args.sort; |
When the triage tool delegates to hotspotsData for file/directory level, sort values like 'complexity', 'churn', and 'mi' don't exist in HOTSPOT_QUERIES (which only supports fan-in, fan-out, density, coupling). Previously these silently fell back to fan-in via the || fallback. Add explicit TRIAGE_TO_HOTSPOT mapping: - risk → fan-in (unchanged) - complexity → density (symbol count as structural proxy) - churn → coupling (fan-in + fan-out reflects change propagation) - mi → fan-in (no direct equivalent) - fan-in → fan-in (pass-through via nullish coalescing) Impact: 1 functions changed, 1 affected
|
Claude finished @carlos-alm's task —— View job PR Review: MCP Tools ConsolidationReview Progress
Phase 1: Root Cause Analysis ✅Problem Being Solved: This PR consolidates MCP tools to mirror CLI consolidation from PR #280, which removed redundant CLI commands by folding functionality into existing commands. Why This Approach: Maintains consistency between CLI and MCP interfaces. Rather than having separate Architectural Justification: Sound approach - reduces API surface while preserving all functionality. Backward compatibility is maintained via parameter routing. Phase 2: Configuration Change Detection ✅No High-Risk Changes Detected:
Phase 3: Test Integrity ✅Test Coverage Analysis:
Test Quality: Excellent - comprehensive schema validation for all consolidated tools. Phase 4: Code Quality Review ✅Implementation Analysis (src/mcp.js:1206-1352): ✅ if (args.quick) {
result = explainData(args.target, dbPath, { /* options */ });
} else {
const { auditData } = await import('./audit.js');
result = auditData(args.target, dbPath, { /* full options */ });
}✅ if (args.level === 'file' || args.level === 'directory') {
const { hotspotsData } = await import('./structure.js');
// Correctly maps 'risk' sort to 'fan-in' for hotspots
const metric = args.sort === 'risk' ? 'fan-in' : args.sort;
}
All mapped values ( ✅
✅ New Backward Compatibility: Perfect - Code Quality: High - clean separation of concerns, proper lazy imports, consistent error handling. Root Cause AnalysisProblem Being Solved: Maintaining consistency between CLI and MCP interfaces after CLI consolidation in PR #280. The CLI removed redundant commands by folding functionality into existing commands with parameters. Why This Approach: The consolidation reduces API surface area while preserving all functionality. This is a principled refactoring that eliminates duplicate endpoints without losing capabilities. Risk Assessment: Low risk - purely structural changes with comprehensive test coverage and backward compatibility preservation. Critical ConcernsGreptile False Positive: The metric mapping bug reported by Greptile is incorrect. The Final Recommendation
Key Strengths:
|
Greptile feedback addressedTriage tool metric mapping bug (line 1237): Fixed in a1583cb. Added explicit
Also switched from |
1 participant
Summary
explaintool → folded intoauditwithquickboolean param (callsexplainDatawhen true)hotspotstool → folded intotriagewithlevelenum (function|file|directory); file/directory delegates tohotspotsDatamanifestotool → folded intocheckwith manifesto-mode routing: no ref/staged → runs manifesto rules;rulesflag runs both diff predicates + manifestopathtool withfrom/torequired params (was only accessible viaquerymode=path, which still works for backward compat)Net tool count: 34 → 31 (removed 3, added 1). All backing data functions unchanged.
Mirrors the CLI consolidation from #280 which removed
explain,hotspots,manifesto,path(standalone), andbatch-queryCLI commands.Test plan
npx biome check src/mcp.js tests/unit/mcp.test.js— lint cleannpx vitest run tests/unit/mcp.test.js— 37/37 pass (updatedALL_TOOL_NAMES, addedpath/audit/triage/checkschema tests)npx vitest run tests/unit/— 566/566 passnpx vitest run tests/integration/— 474/474 passnode src/cli.js diff-impact --staged -T— blast radius: 1 function changed (startMCPServer), 1 transitive caller