Skip to content

fix browser system prompt inclusion rules #1556

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.

Sign up for GitHub

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

Merged
mrubens merged 4 commits into from
Mar 11, 2025
Merged

fix browser system prompt inclusion rules #1556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
58532bc
Merge pull request #1 from cannuri/update-from-upstream
cannuri Mar 9, 2025
4a54294
Merge remote-tracking branch 'upstream/main'
cannuri Mar 10, 2025
42287ad
Merge pull request #2 from cannuri/feature/update-main
cannuri Mar 10, 2025
a9773b7
fix browser_action system prompt
cannuri Mar 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/core/webview/ClineProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1826,6 +1826,7 @@ export class ClineProvider implements vscode.WebviewViewProvider {
fuzzyMatchThreshold,
experiments,
enableMcpServerCreation,
browserToolEnabled,
} = await this.getState()

// Create diffStrategy based on current model and settings
Expand All @@ -1841,10 +1842,14 @@ export class ClineProvider implements vscode.WebviewViewProvider {

const rooIgnoreInstructions = this.getCurrentCline()?.rooIgnoreController?.getInstructions()

// Determine if browser tools can be used based on model support and user settings
const modelSupportsComputerUse = this.getCurrentCline()?.api.getModel().info.supportsComputerUse ?? false
const canUseBrowserTool = modelSupportsComputerUse && (browserToolEnabled ?? true)

const systemPrompt = await SYSTEM_PROMPT(
this.context,
cwd,
apiConfiguration.openRouterModelInfo?.supportsComputerUse ?? false,
Copy link
Collaborator

@mrubens mrubens Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that is a good find / what was I thinking? 😂

canUseBrowserTool,
mcpEnabled ? this.mcpHub : undefined,
diffStrategy,
browserViewportSize ?? "900x600",
Expand Down
274 changes: 232 additions & 42 deletions src/core/webview/__tests__/ClineProvider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1160,6 +1160,17 @@ describe("ClineProvider", () => {
})

test("passes diffStrategy and diffEnabled to SYSTEM_PROMPT when previewing", async () => {
// Setup Cline instance with mocked api.getModel()
const { Cline } = require("../../Cline")
const mockCline = new Cline()
mockCline.api = {
getModel: jest.fn().mockReturnValue({
id: "claude-3-sonnet",
info: { supportsComputerUse: true },
}),
}
await provider.addClineToStack(mockCline)

// Mock getState to return experimentalDiffStrategy, diffEnabled and fuzzyMatchThreshold
jest.spyOn(provider, "getState").mockResolvedValue({
apiConfiguration: {
Expand All @@ -1176,6 +1187,7 @@ describe("ClineProvider", () => {
diffEnabled: true,
fuzzyMatchThreshold: 0.8,
experiments: experimentDefault,
browserToolEnabled: true,
} as any)

// Mock SYSTEM_PROMPT to verify diffStrategy and diffEnabled are passed
Expand All @@ -1186,34 +1198,37 @@ describe("ClineProvider", () => {
const handler = getMessageHandler()
await handler({ type: "getSystemPrompt", mode: "code" })

// Verify SYSTEM_PROMPT was called with correct arguments
expect(systemPromptSpy).toHaveBeenCalledWith(
expect.anything(), // context
expect.any(String), // cwd
true, // supportsComputerUse
undefined, // mcpHub (disabled)
expect.objectContaining({
// diffStrategy
getToolDescription: expect.any(Function),
}),
"900x600", // browserViewportSize
"code", // mode
{}, // customModePrompts
{ customModes: [] }, // customModes
undefined, // effectiveInstructions
undefined, // preferredLanguage
true, // diffEnabled
experimentDefault,
true,
undefined, // rooIgnoreInstructions
)
// Verify SYSTEM_PROMPT was called
expect(systemPromptSpy).toHaveBeenCalled()

// Get the actual arguments passed to SYSTEM_PROMPT
const callArgs = systemPromptSpy.mock.calls[0]

// Verify key parameters
expect(callArgs[2]).toBe(true) // supportsComputerUse
expect(callArgs[3]).toBeUndefined() // mcpHub (disabled)
expect(callArgs[4]).toHaveProperty("getToolDescription") // diffStrategy
expect(callArgs[5]).toBe("900x600") // browserViewportSize
expect(callArgs[6]).toBe("code") // mode
expect(callArgs[11]).toBe(true) // diffEnabled

// Run the test again to verify it's consistent
await handler({ type: "getSystemPrompt", mode: "code" })
expect(systemPromptSpy).toHaveBeenCalledTimes(2)
})

test("passes diffEnabled: false to SYSTEM_PROMPT when diff is disabled", async () => {
// Setup Cline instance with mocked api.getModel()
const { Cline } = require("../../Cline")
const mockCline = new Cline()
mockCline.api = {
getModel: jest.fn().mockReturnValue({
id: "claude-3-sonnet",
info: { supportsComputerUse: true },
}),
}
await provider.addClineToStack(mockCline)

// Mock getState to return diffEnabled: false
jest.spyOn(provider, "getState").mockResolvedValue({
apiConfiguration: {
Expand All @@ -1230,6 +1245,7 @@ describe("ClineProvider", () => {
fuzzyMatchThreshold: 0.8,
experiments: experimentDefault,
enableMcpServerCreation: true,
browserToolEnabled: true,
} as any)

// Mock SYSTEM_PROMPT to verify diffEnabled is passed as false
Expand All @@ -1240,27 +1256,19 @@ describe("ClineProvider", () => {
const handler = getMessageHandler()
await handler({ type: "getSystemPrompt", mode: "code" })

// Verify SYSTEM_PROMPT was called with diffEnabled: false
expect(systemPromptSpy).toHaveBeenCalledWith(
expect.anything(), // context
expect.any(String), // cwd
true, // supportsComputerUse
undefined, // mcpHub (disabled)
expect.objectContaining({
// diffStrategy
getToolDescription: expect.any(Function),
}),
"900x600", // browserViewportSize
"code", // mode
{}, // customModePrompts
{ customModes: [] }, // customModes
undefined, // effectiveInstructions
undefined, // preferredLanguage
false, // diffEnabled
experimentDefault,
true,
undefined, // rooIgnoreInstructions
)
// Verify SYSTEM_PROMPT was called
expect(systemPromptSpy).toHaveBeenCalled()

// Get the actual arguments passed to SYSTEM_PROMPT
const callArgs = systemPromptSpy.mock.calls[0]

// Verify key parameters
expect(callArgs[2]).toBe(true) // supportsComputerUse
expect(callArgs[3]).toBeUndefined() // mcpHub (disabled)
expect(callArgs[4]).toHaveProperty("getToolDescription") // diffStrategy
expect(callArgs[5]).toBe("900x600") // browserViewportSize
expect(callArgs[6]).toBe("code") // mode
expect(callArgs[11]).toBe(false) // diffEnabled should be false
})

test("uses correct mode-specific instructions when mode is specified", async () => {
Expand Down Expand Up @@ -1299,6 +1307,188 @@ describe("ClineProvider", () => {
expect.any(String),
)
})

// Tests for browser tool support
test("correctly extracts modelSupportsComputerUse from Cline instance", async () => {
// Setup Cline instance with mocked api.getModel()
const { Cline } = require("../../Cline")
const mockCline = new Cline()
mockCline.api = {
getModel: jest.fn().mockReturnValue({
id: "claude-3-sonnet",
info: { supportsComputerUse: true },
}),
}
await provider.addClineToStack(mockCline)

// Mock SYSTEM_PROMPT to verify supportsComputerUse is passed correctly
const systemPromptModule = require("../../prompts/system")
const systemPromptSpy = jest.spyOn(systemPromptModule, "SYSTEM_PROMPT")

// Mock getState to return browserToolEnabled: true
jest.spyOn(provider, "getState").mockResolvedValue({
apiConfiguration: {
apiProvider: "openrouter",
},
browserToolEnabled: true,
mode: "code",
experiments: experimentDefault,
} as any)

// Trigger getSystemPrompt
const handler = getMessageHandler()
await handler({ type: "getSystemPrompt", mode: "code" })

// Verify SYSTEM_PROMPT was called
expect(systemPromptSpy).toHaveBeenCalled()

// Get the actual arguments passed to SYSTEM_PROMPT
const callArgs = systemPromptSpy.mock.calls[0]

// Verify the supportsComputerUse parameter (3rd parameter, index 2)
expect(callArgs[2]).toBe(true)
})

test("correctly handles when model doesn't support computer use", async () => {
// Setup Cline instance with mocked api.getModel() that doesn't support computer use
const { Cline } = require("../../Cline")
const mockCline = new Cline()
mockCline.api = {
getModel: jest.fn().mockReturnValue({
id: "non-computer-use-model",
info: { supportsComputerUse: false },
}),
}
await provider.addClineToStack(mockCline)

// Mock SYSTEM_PROMPT to verify supportsComputerUse is passed correctly
const systemPromptModule = require("../../prompts/system")
const systemPromptSpy = jest.spyOn(systemPromptModule, "SYSTEM_PROMPT")

// Mock getState to return browserToolEnabled: true
jest.spyOn(provider, "getState").mockResolvedValue({
apiConfiguration: {
apiProvider: "openrouter",
},
browserToolEnabled: true,
mode: "code",
experiments: experimentDefault,
} as any)

// Trigger getSystemPrompt
const handler = getMessageHandler()
await handler({ type: "getSystemPrompt", mode: "code" })

// Verify SYSTEM_PROMPT was called
expect(systemPromptSpy).toHaveBeenCalled()

// Get the actual arguments passed to SYSTEM_PROMPT
const callArgs = systemPromptSpy.mock.calls[0]

// Verify the supportsComputerUse parameter (3rd parameter, index 2)
// Even though browserToolEnabled is true, the model doesn't support it
expect(callArgs[2]).toBe(false)
})

test("correctly handles when browserToolEnabled is false", async () => {
// Setup Cline instance with mocked api.getModel() that supports computer use
const { Cline } = require("../../Cline")
const mockCline = new Cline()
mockCline.api = {
getModel: jest.fn().mockReturnValue({
id: "claude-3-sonnet",
info: { supportsComputerUse: true },
}),
}
await provider.addClineToStack(mockCline)

// Mock SYSTEM_PROMPT to verify supportsComputerUse is passed correctly
const systemPromptModule = require("../../prompts/system")
const systemPromptSpy = jest.spyOn(systemPromptModule, "SYSTEM_PROMPT")

// Mock getState to return browserToolEnabled: false
jest.spyOn(provider, "getState").mockResolvedValue({
apiConfiguration: {
apiProvider: "openrouter",
},
browserToolEnabled: false,
mode: "code",
experiments: experimentDefault,
} as any)

// Trigger getSystemPrompt
const handler = getMessageHandler()
await handler({ type: "getSystemPrompt", mode: "code" })

// Verify SYSTEM_PROMPT was called
expect(systemPromptSpy).toHaveBeenCalled()

// Get the actual arguments passed to SYSTEM_PROMPT
const callArgs = systemPromptSpy.mock.calls[0]

// Verify the supportsComputerUse parameter (3rd parameter, index 2)
// Even though model supports it, browserToolEnabled is false
expect(callArgs[2]).toBe(false)
})

test("correctly calculates canUseBrowserTool as combination of model support and setting", async () => {
// Setup Cline instance with mocked api.getModel()
const { Cline } = require("../../Cline")
const mockCline = new Cline()
mockCline.api = {
getModel: jest.fn().mockReturnValue({
id: "claude-3-sonnet",
info: { supportsComputerUse: true },
}),
}
await provider.addClineToStack(mockCline)

// Mock SYSTEM_PROMPT
const systemPromptModule = require("../../prompts/system")
const systemPromptSpy = jest.spyOn(systemPromptModule, "SYSTEM_PROMPT")

// Test all combinations of model support and browserToolEnabled
const testCases = [
{ modelSupports: true, settingEnabled: true, expected: true },
{ modelSupports: true, settingEnabled: false, expected: false },
{ modelSupports: false, settingEnabled: true, expected: false },
{ modelSupports: false, settingEnabled: false, expected: false },
]

for (const testCase of testCases) {
// Reset mocks
systemPromptSpy.mockClear()

// Update mock Cline instance
mockCline.api.getModel = jest.fn().mockReturnValue({
id: "test-model",
info: { supportsComputerUse: testCase.modelSupports },
})

// Mock getState
jest.spyOn(provider, "getState").mockResolvedValue({
apiConfiguration: {
apiProvider: "openrouter",
},
browserToolEnabled: testCase.settingEnabled,
mode: "code",
experiments: experimentDefault,
} as any)

// Trigger getSystemPrompt
const handler = getMessageHandler()
await handler({ type: "getSystemPrompt", mode: "code" })

// Verify SYSTEM_PROMPT was called
expect(systemPromptSpy).toHaveBeenCalled()

// Get the actual arguments passed to SYSTEM_PROMPT
const callArgs = systemPromptSpy.mock.calls[0]

// Verify the supportsComputerUse parameter (3rd parameter, index 2)
expect(callArgs[2]).toBe(testCase.expected)
}
})
})

describe("handleModeSwitch", () => {
Expand Down
Loading