Implementated selective disable of tools to spare with the schema size#1023
Implementated selective disable of tools to spare with the schema size#1023kecsap wants to merge 5 commits intooptave:mainfrom
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
Greptile SummaryAdds a Confidence Score: 5/5Safe to merge; only P2 style findings remain after addressing previous thread issues. No P0 or P1 issues found. The one inline comment is a P2 style suggestion about applying the same No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[startMCPServer] --> B[loadConfig]
B --> C{config.mcp.disabledTools?}
C -- yes --> D[buildToolList\nmultiRepo, disabledTools]
C -- no --> E[buildToolList\nmultiRepo, undefined]
D --> F[buildDisabledToolSet\nnormalize: trim + lowercase + strip prefix]
F --> G[Filter BASE_TOOLS\nexclude disabled]
E --> G
G --> H{multiRepo?}
H -- yes --> I[Add REPO_PROP to each tool\n+ conditionally add LIST_REPOS_TOOL]
H -- no --> J[Return baseTools]
I --> K[enabledTools]
J --> K
K --> L[enabledToolNames = new Set of tool names]
L --> M[tools/list handler\nreturns enabledTools]
L --> N[tools/call handler]
N --> O{enabledToolNames.has name?}
O -- no --> P[Return isError: Unknown tool]
O -- yes --> Q[validateMultiRepoAccess\nthen dispatch to TOOL_HANDLERS]
Reviews (5): Last reviewed commit: "Merge branch 'main' into main" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| validateMultiRepoAccess(multiRepo, name, args); | ||
|
|
||
| if (!enabledToolNames.has(name)) { | ||
| return { content: [{ type: 'text', text: `Unknown tool: ${name}` }], isError: true }; | ||
| } |
There was a problem hiding this comment.
validateMultiRepoAccess runs before disabled-tool guard
validateMultiRepoAccess is called on line 171 before the enabledToolNames check on line 173. In multi-repo mode, if a caller omits the required repo argument when invoking a disabled tool, the handler will throw a multi-repo access error instead of returning the expected "Unknown tool: <name>" response. Swapping the two checks keeps the disabled-tool path clean.
| validateMultiRepoAccess(multiRepo, name, args); | |
| if (!enabledToolNames.has(name)) { | |
| return { content: [{ type: 'text', text: `Unknown tool: ${name}` }], isError: true }; | |
| } | |
| if (!enabledToolNames.has(name)) { | |
| return { content: [{ type: 'text', text: `Unknown tool: ${name}` }], isError: true }; | |
| } | |
| validateMultiRepoAccess(multiRepo, name, args); |
| mcp: { | ||
| defaults: McpDefaults; | ||
| disabledTools: string[]; | ||
| }; |
There was a problem hiding this comment.
disabledTools typed as required but treated optionally at runtime
disabledTools: string[] is non-optional in CodegraphConfig, yet server.ts guards it with config.mcp?.disabledTools ? … : undefined. Any user-supplied config that includes an mcp block without disabledTools (e.g., configs predating this PR) will satisfy the type only because deep-merge with DEFAULTS fills the gap — but if the merge is ever a shallow spread the field can be absent at runtime while TypeScript believes it is always present. Making it optional (disabledTools?: string[]) would align the type with the actual runtime handling and avoid surprising TypeScript errors for callers that construct a partial CodegraphConfig.
| mcp: { | |
| defaults: McpDefaults; | |
| disabledTools: string[]; | |
| }; | |
| mcp: { | |
| defaults: McpDefaults; | |
| disabledTools?: string[]; | |
| }; |
|
I have read the CLA Document and I hereby sign the CLA |
- Reorder createCallToolHandler so the disabled-tool check runs before validateMultiRepoAccess, giving config-disabled tools a consistent "Unknown tool" response regardless of multi-repo state. - Mark CodegraphConfig.mcp.disabledTools optional to match the runtime guard (config.mcp?.disabledTools) and tolerate partial configs. - Update list_repos single-repo test to reflect the unified "Unknown tool" path. - Apply biome formatting to normalizeToolName.
|
Addressed Greptile review feedback in 34da34a:
|
- README: short subsection under Configuration with example and link. - docs/guides/mcp-tool-filtering.md: extensive guide covering motivation, name normalization, runtime behavior, full tool catalog, and recipes.
Per review feedback, the previous guide was too narrowly scoped. Replace with docs/guides/configuration.md covering every key in DEFAULTS: file selection, build, query, embeddings, llm, search, ci, manifesto, check, coChange, analysis, community, structure, risk, display, mcp. README points to the new guide and keeps the short MCP tool-filtering example for the PR's headline feature.
|
Claude finished @carlos-alm's task —— View job PR Review: Selective MCP Tool DisablingI've reviewed the PR context and implementation based on the available information from Greptile's review and the comments. Here's my comprehensive analysis: Todo List:
SummaryThis PR implements selective MCP tool disabling via a ✅ Greptile Feedback ResolutionBased on the comments from carlos-alm in commit 34da34a, the Greptile review feedback appears to have been addressed:
🔍 Additional Review ObservationsPositive Design Aspects
Architecture ReviewThe implementation follows the established patterns in the codebase:
Security & Robustness
🎯 Minor Suggestions
✅ Code Quality AssessmentThe implementation demonstrates:
🚀 RecommendationAPPROVED - This is a well-implemented feature that solves a real problem (schema size for small-context models) while maintaining code quality and following established patterns. The Greptile feedback has been properly addressed, and the implementation is production-ready. The feature will be particularly valuable for users working with context-constrained models who need to carefully manage their tool schema size. |
Codegraph Impact Analysis7 functions changed → 6 callers affected across 3 files
|
|
Hi @kecsap — thanks for the contribution! The code itself is in great shape (all tests + Greptile review pass), but two required CI checks are failing because of repo conventions documented in CONTRIBUTING.md: 1.
|
2 participants
Closes #1022