diff --git a/src/agents/tools/computer-tool.node-resolution.test.ts b/src/agents/tools/computer-tool.node-resolution.test.ts new file mode 100644 index 000000000000..845d72ea977b --- /dev/null +++ b/src/agents/tools/computer-tool.node-resolution.test.ts @@ -0,0 +1,225 @@ +/** + * computer tool node-resolution tests. + * + * Cover which paired node a call binds to: capability eligibility, explicit + * node selectors, and the id-before-display-name precedence that keeps input + * off the wrong machine. + */ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const listNodesMock = vi.fn(); +const callGatewayToolMock = vi.fn(); +const sleepMock = vi.hoisted(() => vi.fn()); +const TINY_PNG_BASE64 = + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+/p9sAAAAASUVORK5CYII="; + +vi.mock("./nodes-utils.js", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, listNodes: listNodesMock }; +}); + +vi.mock("./gateway.js", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, callGatewayTool: callGatewayToolMock }; +}); + +vi.mock("../utils/sleep.js", () => ({ sleep: sleepMock })); + +const { createComputerTool } = await import("./computer-tool.js"); + +function macComputerNode(overrides?: Record) { + return { + nodeId: "mac-1", + displayName: "Studio", + platform: "macos", + connected: true, + commands: ["screen.snapshot", "computer.act"], + ...overrides, + }; +} + +function screenshotPayload(screenIndex = 0, base64 = TINY_PNG_BASE64) { + return { + payload: { + format: "png", + base64, + displayFrameId: `display-${screenIndex}-frame`, + width: 1280, + height: 800, + screenIndex, + }, + }; +} + +describe("createComputerTool node resolution", () => { + beforeEach(() => { + listNodesMock.mockReset(); + callGatewayToolMock.mockReset(); + sleepMock.mockReset(); + sleepMock.mockResolvedValue(undefined); + }); + + it("errors when no computer-capable node is connected", async () => { + listNodesMock.mockResolvedValue([ + macComputerNode({ connected: false }), + { nodeId: "phone", platform: "ios", connected: true, commands: [] }, + ]); + const tool = createComputerTool({ modelHasVision: true }); + await expect(tool.execute("call", { action: "screenshot" })).rejects.toThrow( + /no connected computer-capable node/, + ); + expect(callGatewayToolMock).not.toHaveBeenCalled(); + }); + + it.each(["windows", "linux"])("resolves and executes on a capable %s node", async (platform) => { + const nodeId = `${platform}-1`; + listNodesMock.mockResolvedValue([ + { + nodeId, + displayName: `${platform} desktop`, + platform, + connected: true, + commands: ["computer.act", "screen.snapshot"], + }, + ]); + callGatewayToolMock.mockResolvedValue(screenshotPayload()); + const tool = createComputerTool({ modelHasVision: true }); + + await expect(tool.execute("call", { action: "type", text: "hello" })).resolves.toBeDefined(); + expect(callGatewayToolMock).toHaveBeenCalledWith( + "node.invoke", + expect.anything(), + expect.objectContaining({ nodeId, command: "computer.act" }), + { signal: undefined }, + ); + }); + + it("rejects a named node that is not computer-capable", async () => { + listNodesMock.mockResolvedValue([ + { nodeId: "mac-2", platform: "macos", connected: true, commands: ["screen.snapshot"] }, + ]); + const tool = createComputerTool({ modelHasVision: true }); + await expect(tool.execute("call", { action: "screenshot", node: "mac-2" })).rejects.toThrow( + /not computer-capable/, + ); + }); + + it("reports the eligible node ids when an exact id names an ineligible machine", async () => { + listNodesMock.mockResolvedValue([ + macComputerNode({ nodeId: "mac-disabled", commands: ["screen.snapshot"] }), + macComputerNode({ nodeId: "mac-ready" }), + ]); + const tool = createComputerTool({ modelHasVision: true }); + await expect( + tool.execute("call", { action: "screenshot", node: "mac-disabled" }), + ).rejects.toThrow(/node "mac-disabled" is not computer-capable.*eligible node ids: mac-ready/); + expect(callGatewayToolMock).not.toHaveBeenCalled(); + }); + + it("never redirects an ineligible exact id to an eligible node with that display name", async () => { + listNodesMock.mockResolvedValue([ + macComputerNode({ + nodeId: "requested-desktop", + displayName: "Disabled", + commands: ["screen.snapshot"], + }), + macComputerNode({ nodeId: "mac-ready", displayName: "requested-desktop" }), + ]); + const tool = createComputerTool({ modelHasVision: true }); + await expect( + tool.execute("call", { action: "screenshot", node: "requested-desktop" }), + ).rejects.toThrow(/node "requested-desktop" is not computer-capable/); + expect(callGatewayToolMock).not.toHaveBeenCalled(); + }); + + it("rejects a case-insensitive ineligible id before an eligible display-name match", async () => { + listNodesMock.mockResolvedValue([ + macComputerNode({ + nodeId: "Requested-Desktop", + displayName: "Disabled", + commands: ["screen.snapshot"], + }), + macComputerNode({ nodeId: "mac-ready", displayName: "requested-desktop" }), + ]); + const tool = createComputerTool({ modelHasVision: true }); + await expect( + tool.execute("call", { action: "screenshot", node: "requested-desktop" }), + ).rejects.toThrow(/is not computer-capable/); + expect(callGatewayToolMock).not.toHaveBeenCalled(); + }); + + it("rejects an ambiguous eligible display-name match", async () => { + listNodesMock.mockResolvedValue([ + macComputerNode({ nodeId: "mac-a", displayName: "Shared Desktop" }), + macComputerNode({ nodeId: "mac-b", displayName: "Shared Desktop" }), + ]); + const tool = createComputerTool({ modelHasVision: true }); + await expect( + tool.execute("call", { action: "screenshot", node: "Shared Desktop" }), + ).rejects.toThrow( + /ambiguous node: Shared Desktop.*node=mac-a.*node=mac-b.*eligible computer-capable node ids: mac-a, mac-b/, + ); + expect(callGatewayToolMock).not.toHaveBeenCalled(); + }); + + it("resolves an eligible node by display name", async () => { + listNodesMock.mockResolvedValue([ + macComputerNode({ nodeId: "mac-other", displayName: "Other Desktop" }), + macComputerNode({ nodeId: "mac-ready", displayName: "Studio Desktop" }), + ]); + callGatewayToolMock.mockResolvedValue(screenshotPayload()); + const tool = createComputerTool({ modelHasVision: true }); + + await expect( + tool.execute("call", { action: "screenshot", node: "Studio Desktop" }), + ).resolves.toBeDefined(); + expect(callGatewayToolMock).toHaveBeenCalledWith( + "node.invoke", + expect.anything(), + expect.objectContaining({ nodeId: "mac-ready", command: "screen.snapshot" }), + { signal: undefined }, + ); + }); + + it("selects an exact eligible id over an ineligible display-name collision", async () => { + listNodesMock.mockResolvedValue([ + macComputerNode({ nodeId: "mac-ready", displayName: "Studio" }), + macComputerNode({ nodeId: "mac-off", displayName: "mac-ready", commands: [] }), + ]); + callGatewayToolMock.mockResolvedValue(screenshotPayload()); + const tool = createComputerTool({ modelHasVision: true }); + + await expect( + tool.execute("call", { action: "screenshot", node: "mac-ready" }), + ).resolves.toBeDefined(); + expect(callGatewayToolMock).toHaveBeenCalledWith( + "node.invoke", + expect.anything(), + expect.objectContaining({ nodeId: "mac-ready", command: "screen.snapshot" }), + { signal: undefined }, + ); + }); + + it("requires an explicit node when several computer-capable nodes are connected", async () => { + listNodesMock.mockResolvedValue([ + macComputerNode({ nodeId: "mac-a" }), + macComputerNode({ nodeId: "mac-b" }), + ]); + const tool = createComputerTool({ modelHasVision: true }); + await expect(tool.execute("call", { action: "screenshot" })).rejects.toThrow( + /multiple computer-capable nodes connected; pass node explicitly: mac-a, mac-b/, + ); + expect(callGatewayToolMock).not.toHaveBeenCalled(); + }); + + it("rejects a node advertising computer.act without screen.snapshot", async () => { + listNodesMock.mockResolvedValue([ + { nodeId: "desktop-1", platform: "windows", connected: true, commands: ["computer.act"] }, + ]); + const tool = createComputerTool({ modelHasVision: true }); + await expect(tool.execute("call", { action: "screenshot", node: "desktop-1" })).rejects.toThrow( + /advertising computer\.act and screen\.snapshot/, + ); + expect(callGatewayToolMock).not.toHaveBeenCalled(); + }); +}); diff --git a/src/agents/tools/computer-tool.test.ts b/src/agents/tools/computer-tool.test.ts index fec1d1cd12eb..8b8503d29500 100644 --- a/src/agents/tools/computer-tool.test.ts +++ b/src/agents/tools/computer-tool.test.ts @@ -1,7 +1,8 @@ /** * computer tool tests. * - * Cover the computer.act wire mapping and node resolution / arming behavior. + * Cover the computer.act wire mapping, frame binding, and arming behavior. + * Node selection lives in computer-tool.node-resolution.test.ts. */ import { createHash } from "node:crypto"; import { beforeEach, describe, expect, it, vi } from "vitest"; @@ -226,7 +227,7 @@ describe("createComputerTool schema", () => { }); }); -describe("createComputerTool node resolution", () => { +describe("createComputerTool execution", () => { beforeEach(() => { listNodesMock.mockReset(); callGatewayToolMock.mockReset(); @@ -355,62 +356,6 @@ describe("createComputerTool node resolution", () => { }, ); - it("errors when no computer-capable node is connected", async () => { - listNodesMock.mockResolvedValue([ - macComputerNode({ connected: false }), - { nodeId: "phone", platform: "ios", connected: true, commands: [] }, - ]); - const tool = createComputerTool({ modelHasVision: true }); - await expect(tool.execute("call", { action: "screenshot" })).rejects.toThrow( - /no connected computer-capable node/, - ); - expect(callGatewayToolMock).not.toHaveBeenCalled(); - }); - - it.each(["windows", "linux"])("resolves and executes on a capable %s node", async (platform) => { - const nodeId = `${platform}-1`; - listNodesMock.mockResolvedValue([ - { - nodeId, - displayName: `${platform} desktop`, - platform, - connected: true, - commands: ["computer.act", "screen.snapshot"], - }, - ]); - callGatewayToolMock.mockResolvedValue(screenshotPayload()); - const tool = createComputerTool({ modelHasVision: true }); - - await expect(tool.execute("call", { action: "type", text: "hello" })).resolves.toBeDefined(); - expect(callGatewayToolMock).toHaveBeenCalledWith( - "node.invoke", - expect.anything(), - expect.objectContaining({ nodeId, command: "computer.act" }), - { signal: undefined }, - ); - }); - - it("rejects a named node that is not computer-capable", async () => { - listNodesMock.mockResolvedValue([ - { nodeId: "mac-2", platform: "macos", connected: true, commands: ["screen.snapshot"] }, - ]); - const tool = createComputerTool({ modelHasVision: true }); - await expect(tool.execute("call", { action: "screenshot", node: "mac-2" })).rejects.toThrow( - /not computer-capable/, - ); - }); - - it("rejects a node advertising computer.act without screen.snapshot", async () => { - listNodesMock.mockResolvedValue([ - { nodeId: "desktop-1", platform: "windows", connected: true, commands: ["computer.act"] }, - ]); - const tool = createComputerTool({ modelHasVision: true }); - await expect(tool.execute("call", { action: "screenshot", node: "desktop-1" })).rejects.toThrow( - /advertising computer\.act and screen\.snapshot/, - ); - expect(callGatewayToolMock).not.toHaveBeenCalled(); - }); - it("captures a screenshot through screen.snapshot and keeps it model-only", async () => { listNodesMock.mockResolvedValue([macComputerNode()]); callGatewayToolMock.mockResolvedValue(screenshotPayload()); diff --git a/src/agents/tools/computer-tool.ts b/src/agents/tools/computer-tool.ts index 3add9c307d03..6967a3396f8b 100644 --- a/src/agents/tools/computer-tool.ts +++ b/src/agents/tools/computer-tool.ts @@ -325,13 +325,13 @@ function isEligibleComputerNode(node: NodeListNode): boolean { const NOT_COMPUTER_CAPABLE_HINT = "enable Computer Control in the OpenClaw app and approve the pairing update"; -function nodeMatchesQuery(node: NodeListNode, query: string): boolean { - const lowered = query.toLowerCase(); - return ( - node.nodeId === query || - node.nodeId.toLowerCase() === lowered || - node.displayName?.toLowerCase() === lowered - ); +function formatEligibleComputerNodeIds(nodes: NodeListNode[]): string { + return nodes.length > 0 + ? nodes + .map((node) => node.nodeId) + .toSorted() + .join(", ") + : "none"; } async function resolveComputerNode( @@ -343,26 +343,38 @@ async function resolveComputerNode( const eligible = nodes.filter(isEligibleComputerNode); const trimmed = query?.trim(); if (trimmed) { - // Shared resolver: prefers exact node ids and rejects ambiguous - // display-name collisions, so control never lands on the wrong machine. - let nodeId: string; - try { - nodeId = resolveNodeIdFromList(eligible, trimmed, false); - } catch (err) { - const ineligible = nodes.find((node) => nodeMatchesQuery(node, trimmed)); - if (ineligible && !isEligibleComputerNode(ineligible)) { + // Stable ids outrank human-facing names across the full machine set, and the + // precedence check matches case-insensitively because display-name resolution + // below is case-insensitive: an ineligible id that differs only by case must + // never fall through to another eligible machine's name. Exact case wins first. + const lowerTrimmed = trimmed.toLowerCase(); + const exactNode = + nodes.find((node) => node.nodeId === trimmed) ?? + nodes.find((node) => node.nodeId.toLowerCase() === lowerTrimmed); + if (exactNode) { + if (!isEligibleComputerNode(exactNode)) { throw new Error( - `node "${trimmed}" is not computer-capable (needs a connected node advertising ${COMPUTER_ACT_COMMAND} and ${SCREEN_SNAPSHOT_COMMAND}; ${NOT_COMPUTER_CAPABLE_HINT})`, - { cause: err }, + `node "${trimmed}" is not computer-capable (needs a connected node advertising ${COMPUTER_ACT_COMMAND} and ${SCREEN_SNAPSHOT_COMMAND}; ${NOT_COMPUTER_CAPABLE_HINT}; ` + + `eligible node ids: ${formatEligibleComputerNodeIds(eligible)})`, ); } - throw err instanceof Error ? err : new Error(String(err)); + return exactNode; } - const match = eligible.find((node) => node.nodeId === nodeId); - if (!match) { - throw new Error(`node not found: ${trimmed}`); + // Shared resolver: rejects ambiguous display-name collisions, so control + // never lands on the wrong machine. + try { + const nodeId = resolveNodeIdFromList(eligible, trimmed, false); + const match = eligible.find((node) => node.nodeId === nodeId); + if (match) { + return match; + } + } catch (err) { + throw new Error( + `${formatErrorMessage(err)} (eligible computer-capable node ids: ${formatEligibleComputerNodeIds(eligible)})`, + { cause: err }, + ); } - return match; + throw new Error(`node not found: ${trimmed}`); } if (eligible.length === 1) { const node = eligible.at(0);