Add support for excluding MCP tools from the Dart MCP server #5639
Conversation
Some of the built-in tools like `run_tests` overlap with built-in VS Code functionality, but the model might choose the SDK version despite having less functionality.
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for excluding specific MCP tools from the Dart MCP server to avoid conflicts with built-in VS Code functionality. The implementation allows users to configure which tools should be excluded via a new dart.mcpServerTools setting.
- Adds a new configuration option
dart.mcpServerToolsto control which MCP tools are enabled/disabled - Implements
--exclude-toolargument passing to the Dart MCP server when supported (Dart SDK >= 3.10.0) - Defaults to excluding
run_teststool to prevent overlap with VS Code's built-in test functionality
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
package.json |
Adds new dart.mcpServerTools configuration with default exclusion of run_tests |
src/extension/config.ts |
Exposes the new configuration option through the Config class |
src/shared/capabilities/dart.ts |
Adds version check for MCP server exclude-tool support |
src/extension/providers/mcp_server_definition_provider.ts |
Implements exclude-tool argument generation based on configuration |
src/extension/extension.ts |
Exposes MCP server provider for testing and type safety improvements |
src/shared/vscode/interfaces.ts |
Adds MCP server provider to internal API interface |
src/extension/views/packages_view.ts |
Makes projectFinder public (likely for testing) |
src/test/dart/mcp_server.test.ts |
Comprehensive test suite for the new exclude-tool functionality |
|
|
||
| // Add exclude-tool flags if supported and configured (tools set to false are excluded) | ||
| if (this.dartCapabilities.supportsMcpServerExcludeTool) { | ||
| const tools: Record<string, boolean> = config.mcpServerTools ?? {}; |
There was a problem hiding this comment.
The fallback to empty object {} conflicts with the default configuration defined in package.json. Consider using the actual default value from the configuration or document why the fallback differs from the declared default.
| const tools: Record<string, boolean> = config.mcpServerTools ?? {}; | |
| const defaultMcpServerTools: Record<string, boolean> = packageJson.contributes?.configuration?.properties?.["dart.mcpServerTools"]?.default ?? {}; | |
| const tools: Record<string, boolean> = config.mcpServerTools ?? defaultMcpServerTools; |
| get mcpServer(): boolean { return this.getConfig<boolean>("mcpServer", true); } | ||
| get mcpServerLogFile(): undefined | string { return createFolderForFile(insertWorkspaceName(resolvePaths(this.getConfig<null | string>("mcpServerLogFile", null)))); } | ||
| // eslint-disable-next-line camelcase | ||
| get mcpServerTools(): Record<string, boolean> { return this.getConfig<Record<string, boolean>>("mcpServerTools", {/* defaults from package.json */ }); } |
There was a problem hiding this comment.
The comment /* defaults from package.json */ is misleading as the actual default is an empty object {}, not the default from package.json which includes {"run_tests": false}. This inconsistency could lead to unexpected behavior where the package.json default is not applied.
| get mcpServerTools(): Record<string, boolean> { return this.getConfig<Record<string, boolean>>("mcpServerTools", {/* defaults from package.json */ }); } | |
| get mcpServerTools(): Record<string, boolean> { return this.getConfig<Record<string, boolean>>("mcpServerTools", { run_tests: false }); } |
| if (config.mcpServerLogFile && this.dartCapabilities.supportsMcpServerLogFile) | ||
| args.push("--log-file", config.mcpServerLogFile); | ||
|
|
||
| // Add exclude-tool flags if supported and configured (tools set to false are excluded) |
There was a problem hiding this comment.
The comment should clarify the merge behavior with default exclusions. Currently it doesn't explain that user configuration merges with defaults defined in package.json.
| // Add exclude-tool flags if supported and configured (tools set to false are excluded) | |
| // Add exclude-tool flags if supported and configured. | |
| // Note: The tools configuration is merged with defaults defined in package.json; | |
| // tools set to false (either by user or default) are excluded. |
There was a problem hiding this comment.
Code Review
This pull request adds support for excluding MCP tools from the Dart MCP server by introducing a new setting dart.mcpServerTools. The implementation is mostly correct and includes good test coverage. However, there's a critical issue in how the new configuration is read, as it doesn't merge user-provided settings with the defaults, which would cause the feature to not work as intended. I've provided a fix for this in src/extension/config.ts. I've also included a minor cleanup suggestion in src/extension/providers/mcp_server_definition_provider.ts that becomes relevant after applying the main fix. The other changes, mainly for exposing components for testing, look good.
|
|
||
| // Add exclude-tool flags if supported and configured (tools set to false are excluded) | ||
| if (this.dartCapabilities.supportsMcpServerExcludeTool) { | ||
| const tools: Record<string, boolean> = config.mcpServerTools ?? {}; |
There was a problem hiding this comment.
With the proposed change to config.mcpServerTools, it will always return an object and never be null or undefined. Therefore, the nullish coalescing operator (?? {}) is redundant and can be removed for clarity.
| const tools: Record<string, boolean> = config.mcpServerTools ?? {}; | |
| const tools: Record<string, boolean> = config.mcpServerTools; |
Some of the built-in tools like
run_testsoverlap with built-in VS Code functionality, but the model might choose the SDK version despite having less functionality.TODO
run_testsseems like a like candidateanalyzer_files- maybe? VS Code has Problems, but it's not clear if the model can read them all, or only for a given file?create_project?dart_fix?hot_reload?pub- should we expose the built-in Dart-Code commands so it can call things that show in the UI?Edit: For now, only excluding
runTestsbecause it's the only one that specifically does overlap- the others require us to expose new tools from Dart-Code, or VS Code changes (I've opened #5647 to track the problems tool replacinganalyze_files).