From b2ad9030051e4b53664572e055163edffc3b44da Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 2 Jul 2026 20:18:22 +0200 Subject: [PATCH] Fix browser tool ergonomics followups --- .../src/server/agent/mcp-server.test.ts | 56 ++++++++++++++++++- .../src/server/browser-tools/tools.test.ts | 28 ++++++++++ .../server/src/server/browser-tools/tools.ts | 47 +++++++++++----- 3 files changed, 115 insertions(+), 16 deletions(-) diff --git a/packages/server/src/server/agent/mcp-server.test.ts b/packages/server/src/server/agent/mcp-server.test.ts index b3578f46a..871503eaf 100644 --- a/packages/server/src/server/agent/mcp-server.test.ts +++ b/packages/server/src/server/agent/mcp-server.test.ts @@ -39,6 +39,7 @@ import { PARENT_AGENT_ID_LABEL } from "@getpaseo/protocol/agent-labels"; const REPO_CWD = resolvePath("/tmp/repo"); const TARGET_CWD = resolvePath("/tmp/target"); +const BROWSER_WORKSPACE_ID = "wks_browser_tools"; interface LooseSafeParseResult { success: boolean; @@ -583,7 +584,11 @@ describe("browser MCP tools", () => { it("keeps browser tools registered when browser tools are disabled", async () => { const { agentManager, agentStorage, spies } = createTestDeps(); - spies.agentManager.getAgent.mockReturnValue({ id: "agent-1", cwd: REPO_CWD }); + spies.agentManager.getAgent.mockReturnValue({ + id: "agent-1", + cwd: REPO_CWD, + workspaceId: BROWSER_WORKSPACE_ID, + }); const execute = vi.fn().mockResolvedValue({ requestId: "req-browser-disabled", ok: false, @@ -609,6 +614,7 @@ describe("browser MCP tools", () => { expect(execute).toHaveBeenCalledWith({ agentId: "agent-1", cwd: REPO_CWD, + workspaceId: BROWSER_WORKSPACE_ID, command: { command: "list_tabs", args: {} }, }); expect(response.structuredContent).toEqual({ @@ -618,13 +624,17 @@ describe("browser MCP tools", () => { message: "Browser tools are disabled.", retryable: false, }, - context: { agentId: "agent-1", cwd: REPO_CWD }, + context: { agentId: "agent-1", cwd: REPO_CWD, workspaceId: BROWSER_WORKSPACE_ID }, }); }); it("wires browser tools through the browser tools broker", async () => { const { agentManager, agentStorage, spies } = createTestDeps(); - spies.agentManager.getAgent.mockReturnValue({ id: "agent-1", cwd: REPO_CWD }); + spies.agentManager.getAgent.mockReturnValue({ + id: "agent-1", + cwd: REPO_CWD, + workspaceId: BROWSER_WORKSPACE_ID, + }); const execute = vi.fn().mockResolvedValue({ requestId: "req-browser-tabs", ok: true, @@ -645,6 +655,7 @@ describe("browser MCP tools", () => { expect(execute).toHaveBeenCalledWith({ agentId: "agent-1", cwd: REPO_CWD, + workspaceId: BROWSER_WORKSPACE_ID, command: { command: "list_tabs", args: {} }, }); expect(response.content).toEqual([ @@ -656,6 +667,45 @@ describe("browser MCP tools", () => { expect(response.structuredContent).toEqual({ ok: true, result: { command: "list_tabs", tabs: [] }, + context: { agentId: "agent-1", cwd: REPO_CWD, workspaceId: BROWSER_WORKSPACE_ID }, + }); + }); + + it("tells browser callers without a workspace how to proceed before broker execution", async () => { + const { agentManager, agentStorage, spies } = createTestDeps(); + spies.agentManager.getAgent.mockReturnValue({ id: "agent-1", cwd: REPO_CWD }); + const execute = vi.fn().mockResolvedValue({ + requestId: "req-browser-tabs", + ok: true, + result: { command: "list_tabs", tabs: [] }, + }); + const server = await createAgentMcpServer({ + agentManager, + agentStorage, + providerSnapshotManager: createOpenCodeManager().manager, + browserToolsBroker: { execute } as never, + callerAgentId: "agent-1", + logger, + }); + const tool = registeredTool(server, "browser_list_tabs"); + + const response = await tool.handler({}); + + expect(execute).not.toHaveBeenCalled(); + expect(response.content).toEqual([ + { + type: "text", + text: "This browser tool needs a workspace. Start the agent from a Paseo workspace before calling browser_new_tab or browser_list_tabs.", + }, + ]); + expect(response.structuredContent).toEqual({ + ok: false, + error: { + code: "browser_denied", + message: + "This browser tool needs a workspace. Start the agent from a Paseo workspace before calling browser_new_tab or browser_list_tabs.", + retryable: false, + }, context: { agentId: "agent-1", cwd: REPO_CWD }, }); }); diff --git a/packages/server/src/server/browser-tools/tools.test.ts b/packages/server/src/server/browser-tools/tools.test.ts index ad69100a9..37ddb8115 100644 --- a/packages/server/src/server/browser-tools/tools.test.ts +++ b/packages/server/src/server/browser-tools/tools.test.ts @@ -216,6 +216,24 @@ describe("registerBrowserTools", () => { input: { url: "example.com/x" }, expected: { url: "http://example.com/x" }, }, + { + name: "navigate accepts a single-label host with a port as http", + toolName: "browser_navigate", + input: { browserId: BROWSER_ID, url: "devbox:8080" }, + expected: { browserId: BROWSER_ID, url: "http://devbox:8080" }, + }, + { + name: "navigate accepts an IPv6 host with a port as http", + toolName: "browser_navigate", + input: { browserId: BROWSER_ID, url: "[::1]:5173" }, + expected: { browserId: BROWSER_ID, url: "http://[::1]:5173" }, + }, + { + name: "navigate trims whitespace around a URL", + toolName: "browser_navigate", + input: { browserId: BROWSER_ID, url: " https://example.com/x " }, + expected: { browserId: BROWSER_ID, url: "https://example.com/x" }, + }, { name: "navigate keeps https URLs unchanged", toolName: "browser_navigate", @@ -246,6 +264,16 @@ describe("registerBrowserTools", () => { toolName: "browser_new_tab", input: { url: "file:///tmp/index.html" }, }, + { + name: "navigate rejects invalid ports", + toolName: "browser_navigate", + input: { browserId: BROWSER_ID, url: "devbox:99999" }, + }, + { + name: "navigate rejects URLs with spaces", + toolName: "browser_navigate", + input: { browserId: BROWSER_ID, url: "dev box:8080" }, + }, ])("$name", ({ toolName, input }) => { const harness = new BrowserToolHarness(); diff --git a/packages/server/src/server/browser-tools/tools.ts b/packages/server/src/server/browser-tools/tools.ts index 5ab16945b..6e3b612fa 100644 --- a/packages/server/src/server/browser-tools/tools.ts +++ b/packages/server/src/server/browser-tools/tools.ts @@ -63,8 +63,8 @@ export interface RegisterBrowserToolsOptions { const HTTP_URL_ONLY_MESSAGE = "URL must use http/https only"; const WORKSPACE_CONTEXT_MESSAGE = "This browser tool needs a workspace. Start the agent from a Paseo workspace before calling browser_new_tab or browser_list_tabs."; -const SCHEMELESS_HTTP_URL_PATTERN = - /^(?:localhost|(?:\d{1,3}\.){3}\d{1,3}|\[[0-9a-f:.]+\]|[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?(?:\.[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?)+)(?::\d{1,5})?(?:[/?#].*)?$/i; +const URL_WHITESPACE_PATTERN = /\s/; +const NON_HTTP_EXPLICIT_SCHEME_PATTERN = /^(?!https?:\/\/)[a-z][a-z0-9+.-]*:\/\//i; const BrowserToolContextSchema = z.object({ agentId: z.string().optional(), @@ -453,15 +453,36 @@ export function registerBrowserTools(options: RegisterBrowserToolsOptions): void }, ); - for (const name of ["browser_back", "browser_forward", "browser_reload"] as const) { - const command = name.replace("browser_", "") as "back" | "forward" | "reload"; + for (const toolConfig of [ + { + name: "browser_back", + command: "back", + title: "Browser back", + description: + "Go back in a Paseo desktop browser tab. Use browserId from browser_new_tab or browser_list_tabs.", + }, + { + name: "browser_forward", + command: "forward", + title: "Browser forward", + description: + "Go forward in a Paseo desktop browser tab. Use browserId from browser_new_tab or browser_list_tabs.", + }, + { + name: "browser_reload", + command: "reload", + title: "Browser reload", + description: + "Reload a Paseo desktop browser tab. Use browserId from browser_new_tab or browser_list_tabs.", + }, + ] as const) { options.registerTool( - name, + toolConfig.name, { - title: `Browser ${command}`, - description: `${command} a Paseo desktop browser tab. Use browserId from browser_new_tab or browser_list_tabs.`, + title: toolConfig.title, + description: toolConfig.description, inputSchema: { browserId: BrowserAutomationBrowserIdSchema }, - outputSchema: BrowserToolOutputs[command], + outputSchema: BrowserToolOutputs[toolConfig.command], }, async ({ browserId }) => { const context = resolveBrowserToolContext(options); @@ -471,7 +492,7 @@ export function registerBrowserTools(options: RegisterBrowserToolsOptions): void ...(context.workspaceId ? { workspaceId: context.workspaceId } : {}), command: { - command, + command: toolConfig.command, args: { browserId, }, @@ -943,12 +964,12 @@ function normalizeHttpUrlInput(value: string): string | null { return isValidHttpUrl(value) ? value : null; } - if (SCHEMELESS_HTTP_URL_PATTERN.test(value)) { - const normalized = `http://${value}`; - return isValidHttpUrl(normalized) ? normalized : null; + if (URL_WHITESPACE_PATTERN.test(value) || NON_HTTP_EXPLICIT_SCHEME_PATTERN.test(value)) { + return null; } - return null; + const normalized = `http://${value}`; + return isValidHttpUrl(normalized) ? normalized : null; } function isValidHttpUrl(value: string): boolean {