fix: respect enableMcpServerCreation setting in fetch_instructions tool #6608
Conversation
- Modified fetchInstructions to check enableMcpServerCreation setting before returning MCP server creation instructions - Updated fetchInstructionsTool to pass the setting from provider state - Added comprehensive tests to verify the fix - When setting is disabled, fetch_instructions with 'create_mcp_server' now returns a message indicating the feature is disabled Fixes #6607
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| switch (text) { | ||
| case "create_mcp_server": { | ||
| // Check if MCP server creation is enabled | ||
| if (detail.enableMcpServerCreation === false) { |
There was a problem hiding this comment.
Nice work on the implementation! The check for correctly prevents MCP server creation when disabled. The default behavior (undefined = true) maintains backward compatibility, which is important.
| case "create_mcp_server": { | ||
| // Check if MCP server creation is enabled | ||
| if (detail.enableMcpServerCreation === false) { | ||
| return "MCP server creation is currently disabled. This feature can be enabled in the settings." |
There was a problem hiding this comment.
The error message is clear and helpful. Could we consider adding more specific guidance about where to find this setting? Something like:
Though the current message works well too!
| } | ||
|
|
||
| // Bow fetch the content and provide it to the agent. | ||
| // Now fetch the content and provide it to the agent. |
There was a problem hiding this comment.
Good catch on fixing the typo! Also, the implementation correctly retrieves the setting from the provider state with a sensible default value.
| expect(createMCPServerInstructions).toHaveBeenCalledWith(mockMcpHub, mockDiffStrategy) | ||
| }) | ||
|
|
||
| it("should return disabled message when enableMcpServerCreation is false", async () => { |
There was a problem hiding this comment.
Excellent test coverage! You've covered all three scenarios:
- Setting explicitly true
- Setting undefined (default true)
- Setting explicitly false
The tests are well-structured and verify both the return values and mock function calls. Consider adding an integration test for to verify the end-to-end flow, though the current unit tests provide solid coverage.
hannesrudolph
added
the
Issue/PR - Triage
|
Closing as duplicate of #6613 |
This PR fixes an issue where MCP server creation was attempted even when the 'Enable MCP Server Creation' setting was disabled.
When the enableMcpServerCreation setting is unchecked (disabled), the AI could still attempt to create an MCP server by using the fetch_instructions tool with 'create_mcp_server' parameter. This bypassed the setting check that was only present in the system prompt generation.
Changes:
Fixes #6607
Important
Fixes issue in
fetch_instructionstool to respectenableMcpServerCreationsetting, preventing MCP server creation when disabled.fetchInstructionsininstructions.tsnow checksenableMcpServerCreationbefore returning MCP server instructions.fetchInstructionsToolretrievesenableMcpServerCreationfrom provider state and passes it tofetchInstructions.enableMcpServerCreationis false.instructions.test.tsto verify behavior whenenableMcpServerCreationis true, false, or undefined.fetchInstructionsTool.tscomment.This description was created by
for 58133f8. You can customize this summary. It will automatically update as commits are pushed.