mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-28 05:16:23 -06:00
fix(agents): resolve computer node ids against the full node list first (#112503)
resolveComputerNode passed only the eligible (computer-capable) nodes to the shared resolver, so an explicit exact node id belonging to an ineligible device was never scored. Resolution could then fall through to display-name matching and select a different eligible machine whose display name equaled the requested id, observing and acting on the wrong desktop. Mirror the mobile_ui-tool fix: match an exact node id against the full node list first, reject an ineligible exact match with the eligible ids listed, and only then resolve by display name among eligible nodes (still rejecting ambiguous collisions, now with eligible ids in the error). Split the node-selection tests into computer-tool.node-resolution.test.ts to keep the main test file under max-lines.
This commit is contained in:
Peter Steinberger
committed by
GitHub
GitHub
parent
b83af8157b
commit
d5d2d63ead
@@ -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<typeof import("./nodes-utils.js")>();
|
||||
return { ...actual, listNodes: listNodesMock };
|
||||
});
|
||||
|
||||
vi.mock("./gateway.js", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("./gateway.js")>();
|
||||
return { ...actual, callGatewayTool: callGatewayToolMock };
|
||||
});
|
||||
|
||||
vi.mock("../utils/sleep.js", () => ({ sleep: sleepMock }));
|
||||
|
||||
const { createComputerTool } = await import("./computer-tool.js");
|
||||
|
||||
function macComputerNode(overrides?: Record<string, unknown>) {
|
||||
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();
|
||||
});
|
||||
});
|
||||
@@ -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());
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user