Fix browser tool ergonomics followups
This commit is contained in:
@@ -39,6 +39,7 @@ import { PARENT_AGENT_ID_LABEL } from "@getpaseo/protocol/agent-labels";
|
|||||||
|
|
||||||
const REPO_CWD = resolvePath("/tmp/repo");
|
const REPO_CWD = resolvePath("/tmp/repo");
|
||||||
const TARGET_CWD = resolvePath("/tmp/target");
|
const TARGET_CWD = resolvePath("/tmp/target");
|
||||||
|
const BROWSER_WORKSPACE_ID = "wks_browser_tools";
|
||||||
|
|
||||||
interface LooseSafeParseResult {
|
interface LooseSafeParseResult {
|
||||||
success: boolean;
|
success: boolean;
|
||||||
@@ -583,7 +584,11 @@ describe("browser MCP tools", () => {
|
|||||||
|
|
||||||
it("keeps browser tools registered when browser tools are disabled", async () => {
|
it("keeps browser tools registered when browser tools are disabled", async () => {
|
||||||
const { agentManager, agentStorage, spies } = createTestDeps();
|
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({
|
const execute = vi.fn().mockResolvedValue({
|
||||||
requestId: "req-browser-disabled",
|
requestId: "req-browser-disabled",
|
||||||
ok: false,
|
ok: false,
|
||||||
@@ -609,6 +614,7 @@ describe("browser MCP tools", () => {
|
|||||||
expect(execute).toHaveBeenCalledWith({
|
expect(execute).toHaveBeenCalledWith({
|
||||||
agentId: "agent-1",
|
agentId: "agent-1",
|
||||||
cwd: REPO_CWD,
|
cwd: REPO_CWD,
|
||||||
|
workspaceId: BROWSER_WORKSPACE_ID,
|
||||||
command: { command: "list_tabs", args: {} },
|
command: { command: "list_tabs", args: {} },
|
||||||
});
|
});
|
||||||
expect(response.structuredContent).toEqual({
|
expect(response.structuredContent).toEqual({
|
||||||
@@ -618,13 +624,17 @@ describe("browser MCP tools", () => {
|
|||||||
message: "Browser tools are disabled.",
|
message: "Browser tools are disabled.",
|
||||||
retryable: false,
|
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 () => {
|
it("wires browser tools through the browser tools broker", async () => {
|
||||||
const { agentManager, agentStorage, spies } = createTestDeps();
|
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({
|
const execute = vi.fn().mockResolvedValue({
|
||||||
requestId: "req-browser-tabs",
|
requestId: "req-browser-tabs",
|
||||||
ok: true,
|
ok: true,
|
||||||
@@ -645,6 +655,7 @@ describe("browser MCP tools", () => {
|
|||||||
expect(execute).toHaveBeenCalledWith({
|
expect(execute).toHaveBeenCalledWith({
|
||||||
agentId: "agent-1",
|
agentId: "agent-1",
|
||||||
cwd: REPO_CWD,
|
cwd: REPO_CWD,
|
||||||
|
workspaceId: BROWSER_WORKSPACE_ID,
|
||||||
command: { command: "list_tabs", args: {} },
|
command: { command: "list_tabs", args: {} },
|
||||||
});
|
});
|
||||||
expect(response.content).toEqual([
|
expect(response.content).toEqual([
|
||||||
@@ -656,6 +667,45 @@ describe("browser MCP tools", () => {
|
|||||||
expect(response.structuredContent).toEqual({
|
expect(response.structuredContent).toEqual({
|
||||||
ok: true,
|
ok: true,
|
||||||
result: { command: "list_tabs", tabs: [] },
|
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 },
|
context: { agentId: "agent-1", cwd: REPO_CWD },
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -216,6 +216,24 @@ describe("registerBrowserTools", () => {
|
|||||||
input: { url: "example.com/x" },
|
input: { url: "example.com/x" },
|
||||||
expected: { url: "http://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",
|
name: "navigate keeps https URLs unchanged",
|
||||||
toolName: "browser_navigate",
|
toolName: "browser_navigate",
|
||||||
@@ -246,6 +264,16 @@ describe("registerBrowserTools", () => {
|
|||||||
toolName: "browser_new_tab",
|
toolName: "browser_new_tab",
|
||||||
input: { url: "file:///tmp/index.html" },
|
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 }) => {
|
])("$name", ({ toolName, input }) => {
|
||||||
const harness = new BrowserToolHarness();
|
const harness = new BrowserToolHarness();
|
||||||
|
|
||||||
|
|||||||
@@ -63,8 +63,8 @@ export interface RegisterBrowserToolsOptions {
|
|||||||
const HTTP_URL_ONLY_MESSAGE = "URL must use http/https only";
|
const HTTP_URL_ONLY_MESSAGE = "URL must use http/https only";
|
||||||
const WORKSPACE_CONTEXT_MESSAGE =
|
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.";
|
"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 =
|
const URL_WHITESPACE_PATTERN = /\s/;
|
||||||
/^(?: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 NON_HTTP_EXPLICIT_SCHEME_PATTERN = /^(?!https?:\/\/)[a-z][a-z0-9+.-]*:\/\//i;
|
||||||
|
|
||||||
const BrowserToolContextSchema = z.object({
|
const BrowserToolContextSchema = z.object({
|
||||||
agentId: z.string().optional(),
|
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) {
|
for (const toolConfig of [
|
||||||
const command = name.replace("browser_", "") as "back" | "forward" | "reload";
|
{
|
||||||
|
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(
|
options.registerTool(
|
||||||
name,
|
toolConfig.name,
|
||||||
{
|
{
|
||||||
title: `Browser ${command}`,
|
title: toolConfig.title,
|
||||||
description: `${command} a Paseo desktop browser tab. Use browserId from browser_new_tab or browser_list_tabs.`,
|
description: toolConfig.description,
|
||||||
inputSchema: { browserId: BrowserAutomationBrowserIdSchema },
|
inputSchema: { browserId: BrowserAutomationBrowserIdSchema },
|
||||||
outputSchema: BrowserToolOutputs[command],
|
outputSchema: BrowserToolOutputs[toolConfig.command],
|
||||||
},
|
},
|
||||||
async ({ browserId }) => {
|
async ({ browserId }) => {
|
||||||
const context = resolveBrowserToolContext(options);
|
const context = resolveBrowserToolContext(options);
|
||||||
@@ -471,7 +492,7 @@ export function registerBrowserTools(options: RegisterBrowserToolsOptions): void
|
|||||||
...(context.workspaceId ? { workspaceId: context.workspaceId } : {}),
|
...(context.workspaceId ? { workspaceId: context.workspaceId } : {}),
|
||||||
|
|
||||||
command: {
|
command: {
|
||||||
command,
|
command: toolConfig.command,
|
||||||
args: {
|
args: {
|
||||||
browserId,
|
browserId,
|
||||||
},
|
},
|
||||||
@@ -943,12 +964,12 @@ function normalizeHttpUrlInput(value: string): string | null {
|
|||||||
return isValidHttpUrl(value) ? value : null;
|
return isValidHttpUrl(value) ? value : null;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (SCHEMELESS_HTTP_URL_PATTERN.test(value)) {
|
if (URL_WHITESPACE_PATTERN.test(value) || NON_HTTP_EXPLICIT_SCHEME_PATTERN.test(value)) {
|
||||||
const normalized = `http://${value}`;
|
return null;
|
||||||
return isValidHttpUrl(normalized) ? normalized : null;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
return null;
|
const normalized = `http://${value}`;
|
||||||
|
return isValidHttpUrl(normalized) ? normalized : null;
|
||||||
}
|
}
|
||||||
|
|
||||||
function isValidHttpUrl(value: string): boolean {
|
function isValidHttpUrl(value: string): boolean {
|
||||||
|
|||||||
Reference in New Issue
Block a user