Browse Source

Fix browser tool visibility in system prompt preview (#1840)

fix sys prompt browser visibility
cannuri 11 months ago
parent
commit
92dd8e36da
2 changed files with 107 additions and 43 deletions
  1. 20 4
      src/core/webview/ClineProvider.ts
  2. 87 39
      src/core/webview/__tests__/ClineProvider.test.ts

+ 20 - 4
src/core/webview/ClineProvider.ts

@@ -35,7 +35,7 @@ import {
 import { HistoryItem } from "../../shared/HistoryItem"
 import { ApiConfigMeta, ExtensionMessage } from "../../shared/ExtensionMessage"
 import { checkoutDiffPayloadSchema, checkoutRestorePayloadSchema, WebviewMessage } from "../../shared/WebviewMessage"
-import { Mode, PromptComponent, defaultModeSlug, ModeConfig } from "../../shared/modes"
+import { Mode, PromptComponent, defaultModeSlug, ModeConfig, getModeBySlug, getGroupName } from "../../shared/modes"
 import { checkExistKey } from "../../shared/checkExistApiConfig"
 import { EXPERIMENT_IDS, experiments as Experiments, experimentDefault, ExperimentId } from "../../shared/experiments"
 import { formatLanguage } from "../../shared/language"
@@ -2060,9 +2060,25 @@ export class ClineProvider extends EventEmitter<ClineProviderEvents> implements
 
 			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)
+			// Determine if browser tools can be used based on model support, mode, and user settings
+			let modelSupportsComputerUse = false
+
+			// Create a temporary API handler to check if the model supports computer use
+			// This avoids relying on an active Cline instance which might not exist during preview
+			try {
+				const tempApiHandler = buildApiHandler(apiConfiguration)
+				modelSupportsComputerUse = tempApiHandler.getModel().info.supportsComputerUse ?? false
+			} catch (error) {
+				console.error("Error checking if model supports computer use:", error)
+			}
+
+			// Check if the current mode includes the browser tool group
+			const modeConfig = getModeBySlug(mode, customModes)
+			const modeSupportsBrowser = modeConfig?.groups.some((group) => getGroupName(group) === "browser") ?? false
+
+			// Only enable browser tools if the model supports it, the mode includes browser tools,
+			// and browser tools are enabled in settings
+			const canUseBrowserTool = modelSupportsComputerUse && modeSupportsBrowser && (browserToolEnabled ?? true)
 
 			const systemPrompt = await SYSTEM_PROMPT(
 				this.context,

+ 87 - 39
src/core/webview/__tests__/ClineProvider.test.ts

@@ -1344,29 +1344,27 @@ describe("ClineProvider", () => {
 		})
 
 		// 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 = {
+		test("correctly determines model support for computer use without Cline instance", async () => {
+			// Mock buildApiHandler to return an API handler with supportsComputerUse: true
+			const { buildApiHandler } = require("../../../api")
+			;(buildApiHandler as jest.Mock).mockImplementation(() => ({
 				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
+			// Mock getState to return browserToolEnabled: true and a mode that supports browser
 			jest.spyOn(provider, "getState").mockResolvedValue({
 				apiConfiguration: {
 					apiProvider: "openrouter",
 				},
 				browserToolEnabled: true,
-				mode: "code",
+				mode: "code", // code mode includes browser tool group
 				experiments: experimentDefault,
 			} as any)
 
@@ -1385,16 +1383,14 @@ describe("ClineProvider", () => {
 		})
 
 		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 = {
+			// Mock buildApiHandler to return an API handler with supportsComputerUse: false
+			const { buildApiHandler } = require("../../../api")
+			;(buildApiHandler as jest.Mock).mockImplementation(() => ({
 				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")
@@ -1426,16 +1422,14 @@ describe("ClineProvider", () => {
 		})
 
 		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 = {
+			// Mock buildApiHandler to return an API handler with supportsComputerUse: true
+			const { buildApiHandler } = require("../../../api")
+			;(buildApiHandler as jest.Mock).mockImplementation(() => ({
 				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")
@@ -1466,38 +1460,92 @@ describe("ClineProvider", () => {
 			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 = {
+		test("correctly handles when mode doesn't include browser tool group", async () => {
+			// Mock buildApiHandler to return an API handler with supportsComputerUse: true
+			const { buildApiHandler } = require("../../../api")
+			;(buildApiHandler as jest.Mock).mockImplementation(() => ({
 				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 a mode that doesn't include browser tool group
+			jest.spyOn(provider, "getState").mockResolvedValue({
+				apiConfiguration: {
+					apiProvider: "openrouter",
+				},
+				browserToolEnabled: true,
+				mode: "custom-mode-without-browser", // Custom mode without browser tool group
+				experiments: experimentDefault,
+			} as any)
+
+			// Mock getModeBySlug to return a mode without browser tool group
+			const modesModule = require("../../../shared/modes")
+			jest.spyOn(modesModule, "getModeBySlug").mockReturnValue({
+				slug: "custom-mode-without-browser",
+				name: "Custom Mode",
+				roleDefinition: "Custom role",
+				groups: ["read", "edit"], // No browser group
+			})
+
+			// Trigger getSystemPrompt
+			const handler = getMessageHandler()
+			await handler({ type: "getSystemPrompt", mode: "custom-mode-without-browser" })
+
+			// 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 and browserToolEnabled is true, the mode doesn't include browser tool group
+			expect(callArgs[2]).toBe(false)
+		})
+
+		test("correctly calculates canUseBrowserTool based on all three conditions", async () => {
+			// Mock buildApiHandler
+			const { buildApiHandler } = require("../../../api")
 
 			// Mock SYSTEM_PROMPT
 			const systemPromptModule = require("../../prompts/system")
 			const systemPromptSpy = jest.spyOn(systemPromptModule, "SYSTEM_PROMPT")
 
-			// Test all combinations of model support and browserToolEnabled
+			// Mock getModeBySlug
+			const modesModule = require("../../../shared/modes")
+
+			// Test all combinations of model support, mode 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 },
+				{ modelSupports: true, modeSupports: true, settingEnabled: true, expected: true },
+				{ modelSupports: true, modeSupports: true, settingEnabled: false, expected: false },
+				{ modelSupports: true, modeSupports: false, settingEnabled: true, expected: false },
+				{ modelSupports: false, modeSupports: true, settingEnabled: true, expected: false },
+				{ modelSupports: false, modeSupports: 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 buildApiHandler to return appropriate model support
+				;(buildApiHandler as jest.Mock).mockImplementation(() => ({
+					getModel: jest.fn().mockReturnValue({
+						id: "test-model",
+						info: { supportsComputerUse: testCase.modelSupports },
+					}),
+				}))
+
+				// Mock getModeBySlug to return appropriate mode support
+				jest.spyOn(modesModule, "getModeBySlug").mockReturnValue({
+					slug: "test-mode",
+					name: "Test Mode",
+					roleDefinition: "Test role",
+					groups: testCase.modeSupports ? ["read", "browser"] : ["read"],
 				})
 
 				// Mock getState
@@ -1506,13 +1554,13 @@ describe("ClineProvider", () => {
 						apiProvider: "openrouter",
 					},
 					browserToolEnabled: testCase.settingEnabled,
-					mode: "code",
+					mode: "test-mode",
 					experiments: experimentDefault,
 				} as any)
 
 				// Trigger getSystemPrompt
 				const handler = getMessageHandler()
-				await handler({ type: "getSystemPrompt", mode: "code" })
+				await handler({ type: "getSystemPrompt", mode: "test-mode" })
 
 				// Verify SYSTEM_PROMPT was called
 				expect(systemPromptSpy).toHaveBeenCalled()