-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix: respect enableMcpServerCreation setting in fetch_instructions tool #6608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| import { describe, it, expect, vi, beforeEach } from "vitest" | ||
| import { fetchInstructions } from "../instructions" | ||
| import { createMCPServerInstructions } from "../create-mcp-server" | ||
| import { createModeInstructions } from "../create-mode" | ||
| import { McpHub } from "../../../../services/mcp/McpHub" | ||
| import { DiffStrategy } from "../../../../shared/tools" | ||
| import * as vscode from "vscode" | ||
|
|
||
| // Mock the imported modules | ||
| vi.mock("../create-mcp-server", () => ({ | ||
| createMCPServerInstructions: vi.fn(), | ||
| })) | ||
|
|
||
| vi.mock("../create-mode", () => ({ | ||
| createModeInstructions: vi.fn(), | ||
| })) | ||
|
|
||
| describe("fetchInstructions", () => { | ||
| const mockMcpHub = {} as McpHub | ||
| const mockDiffStrategy = {} as DiffStrategy | ||
| const mockContext = {} as vscode.ExtensionContext | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| }) | ||
|
|
||
| describe("create_mcp_server", () => { | ||
| it("should return MCP server instructions when enableMcpServerCreation is true", async () => { | ||
| const mockInstructions = "MCP server creation instructions" | ||
| vi.mocked(createMCPServerInstructions).mockResolvedValue(mockInstructions) | ||
|
|
||
| const result = await fetchInstructions("create_mcp_server", { | ||
| mcpHub: mockMcpHub, | ||
| diffStrategy: mockDiffStrategy, | ||
| enableMcpServerCreation: true, | ||
| }) | ||
|
|
||
| expect(result).toBe(mockInstructions) | ||
| expect(createMCPServerInstructions).toHaveBeenCalledWith(mockMcpHub, mockDiffStrategy) | ||
| }) | ||
|
|
||
| it("should return MCP server instructions when enableMcpServerCreation is undefined (default true)", async () => { | ||
| const mockInstructions = "MCP server creation instructions" | ||
| vi.mocked(createMCPServerInstructions).mockResolvedValue(mockInstructions) | ||
|
|
||
| const result = await fetchInstructions("create_mcp_server", { | ||
| mcpHub: mockMcpHub, | ||
| diffStrategy: mockDiffStrategy, | ||
| // enableMcpServerCreation is undefined | ||
| }) | ||
|
|
||
| expect(result).toBe(mockInstructions) | ||
| expect(createMCPServerInstructions).toHaveBeenCalledWith(mockMcpHub, mockDiffStrategy) | ||
| }) | ||
|
|
||
| it("should return disabled message when enableMcpServerCreation is false", async () => { | ||
| const result = await fetchInstructions("create_mcp_server", { | ||
| mcpHub: mockMcpHub, | ||
| diffStrategy: mockDiffStrategy, | ||
| enableMcpServerCreation: false, | ||
| }) | ||
|
|
||
| expect(result).toBe( | ||
| "MCP server creation is currently disabled. This feature can be enabled in the settings.", | ||
| ) | ||
| expect(createMCPServerInstructions).not.toHaveBeenCalled() | ||
| }) | ||
| }) | ||
|
|
||
| describe("create_mode", () => { | ||
| it("should return mode creation instructions", async () => { | ||
| const mockInstructions = "Mode creation instructions" | ||
| vi.mocked(createModeInstructions).mockResolvedValue(mockInstructions) | ||
|
|
||
| const result = await fetchInstructions("create_mode", { | ||
| context: mockContext, | ||
| }) | ||
|
|
||
| expect(result).toBe(mockInstructions) | ||
| expect(createModeInstructions).toHaveBeenCalledWith(mockContext) | ||
| }) | ||
|
|
||
| it("should not be affected by enableMcpServerCreation setting", async () => { | ||
| const mockInstructions = "Mode creation instructions" | ||
| vi.mocked(createModeInstructions).mockResolvedValue(mockInstructions) | ||
|
|
||
| const result = await fetchInstructions("create_mode", { | ||
| context: mockContext, | ||
| enableMcpServerCreation: false, | ||
| }) | ||
|
|
||
| expect(result).toBe(mockInstructions) | ||
| expect(createModeInstructions).toHaveBeenCalledWith(mockContext) | ||
| }) | ||
| }) | ||
|
|
||
| describe("unknown task", () => { | ||
| it("should return empty string for unknown task", async () => { | ||
| const result = await fetchInstructions("unknown_task", { | ||
| mcpHub: mockMcpHub, | ||
| diffStrategy: mockDiffStrategy, | ||
| }) | ||
|
|
||
| expect(result).toBe("") | ||
| expect(createMCPServerInstructions).not.toHaveBeenCalled() | ||
| expect(createModeInstructions).not.toHaveBeenCalled() | ||
| }) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,11 +8,16 @@ interface InstructionsDetail { | |
| mcpHub?: McpHub | ||
| diffStrategy?: DiffStrategy | ||
| context?: vscode.ExtensionContext | ||
| enableMcpServerCreation?: boolean | ||
| } | ||
|
|
||
| export async function fetchInstructions(text: string, detail: InstructionsDetail): Promise<string> { | ||
| switch (text) { | ||
| case "create_mcp_server": { | ||
| // Check if MCP server creation is enabled | ||
| if (detail.enableMcpServerCreation === false) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| return "MCP server creation is currently disabled. This feature can be enabled in the settings." | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||
| } | ||
| return await createMCPServerInstructions(detail.mcpHub, detail.diffStrategy) | ||
| } | ||
| case "create_mode": { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,7 @@ export async function fetchInstructionsTool( | |
| return | ||
| } | ||
|
|
||
| // Bow fetch the content and provide it to the agent. | ||
| // Now fetch the content and provide it to the agent. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch on fixing the typo! Also, the implementation correctly retrieves the setting from the provider state with a sensible default value. |
||
| const provider = cline.providerRef.deref() | ||
| const mcpHub = provider?.getMcpHub() | ||
|
|
||
|
|
@@ -46,7 +46,12 @@ export async function fetchInstructionsTool( | |
|
|
||
| const diffStrategy = cline.diffStrategy | ||
| const context = provider?.context | ||
| const content = await fetchInstructions(task, { mcpHub, diffStrategy, context }) | ||
|
|
||
| // Get the enableMcpServerCreation setting from provider state | ||
| const state = await provider?.getState() | ||
| const enableMcpServerCreation = state?.enableMcpServerCreation ?? true | ||
|
|
||
| const content = await fetchInstructions(task, { mcpHub, diffStrategy, context, enableMcpServerCreation }) | ||
|
|
||
| if (!content) { | ||
| pushToolResult(formatResponse.toolError(`Invalid instructions request: ${task}`)) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent test coverage! You've covered all three scenarios:
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.