diff --git a/app/src/lib/copilot/computer-tools.tsx b/app/src/lib/copilot/computer-tools.tsx index c4d93726..c4087b49 100644 --- a/app/src/lib/copilot/computer-tools.tsx +++ b/app/src/lib/copilot/computer-tools.tsx @@ -42,7 +42,14 @@ async function waitForPerson( return "gave up"; } -async function callComputer( +/** + * Exported for the test that covers what a Bot is told when a call is refused. + * + * The distinctions this draws from a status and a body decide the model's next step, and they are + * drawn nowhere else, so they are worth pinning without standing up the tool registrations and the + * runtime around them. + */ +export async function callComputer( botId: string, path: string, /* diff --git a/app/tests/computer-tool-refusals.test.ts b/app/tests/computer-tool-refusals.test.ts new file mode 100644 index 00000000..8404f7fb --- /dev/null +++ b/app/tests/computer-tool-refusals.test.ts @@ -0,0 +1,68 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { callComputer } from "../src/lib/copilot/computer-tools"; + +/** + * What a Bot is told when its action did not happen. + * + * Every refusal reaches the model as this object, and the fields decide what it does next: `staleRefs` + * is the one its own tool description turns into "the page changed, call this again with the new + * refs", so labelling a takeover with it sends the Bot back round the same action against the person + * who just took the browser. The server carries `humanHasControl` for exactly that reason; this is + * the end of that wire, and the only place the two conditions are told apart for the model. + */ + +const realFetch = globalThis.fetch; + +afterEach(() => { + globalThis.fetch = realFetch; +}); + +function serverAnswering(status: number, body: unknown) { + globalThis.fetch = (async () => + new Response(body === undefined ? null : JSON.stringify(body), { + status, + headers: { "content-type": "application/json" }, + })) as unknown as typeof fetch; +} + +describe("a computer call the server refused", () => { + test("a person holding the wheel is reported as that, not as stale refs", async () => { + serverAnswering(409, { + error: "A person has taken control of this computer.", + humanHasControl: true, + }); + + const outcome = await callComputer("bot-1", "/click", { method: "POST" }); + + expect(outcome.ok).toBe(false); + expect(outcome.humanHasControl).toBe(true); + // The instruction that must not be attached: it would send the Bot round again. + expect(outcome.staleRefs).toBeUndefined(); + expect(outcome.reason).toBe("A person has taken control of this computer."); + }); + + test("a stale snapshot is still reported as stale refs", async () => { + // The half that must not move. Losing this would park a Bot with genuinely stale refs waiting for + // a person who is not coming. + serverAnswering(409, { error: "Snapshot 3 is not the current one." }); + + const outcome = await callComputer("bot-1", "/click", { method: "POST" }); + + expect(outcome.ok).toBe(false); + expect(outcome.staleRefs).toBe(true); + expect(outcome.humanHasControl).toBeUndefined(); + }); + + test("a policy refusal is neither", async () => { + serverAnswering(403, { + error: "That is not allowed here.", + rule: 'url.host == "example.com"', + }); + + const outcome = await callComputer("bot-1", "/click", { method: "POST" }); + + expect(outcome.refused).toBe(true); + expect(outcome.staleRefs).toBeUndefined(); + expect(outcome.humanHasControl).toBeUndefined(); + }); +}); diff --git a/server/src/computer/client.ts b/server/src/computer/client.ts index d084cfe7..1b1a12a3 100644 --- a/server/src/computer/client.ts +++ b/server/src/computer/client.ts @@ -49,6 +49,21 @@ export class StaleSnapshotError extends Error { } } +/** + * A person has the wheel, so the Bot's action was not carried out. + * + * Its own condition rather than a stale snapshot, though both arrive as 409, because what the caller + * should do next is the opposite in each case. Stale refs mean take a fresh snapshot and go again; + * this means stop and leave the browser to the person holding it. Told apart by the flag the computer + * puts on the body, which is the only thing in the response that distinguishes them. + */ +export class HumanHasControlError extends Error { + constructor(reason: string) { + super(reason); + this.name = "HumanHasControlError"; + } +} + /** * Transport options used inside the computer gateway. * @@ -206,6 +221,10 @@ function throwMappedError( const detail = typeof body?.error === "string" ? body.error : `HTTP ${status}`; if (status === 409) { + // The computer says which kind of 409 this is. Absent, it is the ordinary one. + if (body?.humanHasControl === true) { + throw new HumanHasControlError(detail); + } throw new StaleSnapshotError(detail); } if (status === 403) { diff --git a/server/src/computer/gateway.ts b/server/src/computer/gateway.ts index 7d5fdac6..672842b3 100644 --- a/server/src/computer/gateway.ts +++ b/server/src/computer/gateway.ts @@ -24,6 +24,7 @@ import { checkComputerAddress } from "./target"; export { ComputerUnavailableError, ElementNotFoundError, + HumanHasControlError, NavigationRefusedError, StaleSnapshotError, WorkspaceRefusedError, diff --git a/server/src/computer/routes.ts b/server/src/computer/routes.ts index b2be2c51..e8a88be6 100644 --- a/server/src/computer/routes.ts +++ b/server/src/computer/routes.ts @@ -10,6 +10,7 @@ import { ComputerUnavailableError, ElementNotFoundError, NavigationRefusedError, + HumanHasControlError, StaleSnapshotError, WorkspaceRefusedError, WorkspaceRequestError, @@ -86,7 +87,7 @@ export function createComputerRoutes( try { return context.json(await gateway.screenshot(context.req.param("botId"))); } catch (error) { - return context.json({ error: describe(error) }, statusFor(error)); + return context.json(errorBody(error), statusFor(error)); } }); @@ -94,7 +95,7 @@ export function createComputerRoutes( try { return context.json(await gateway.read(context.req.param("botId"))); } catch (error) { - return context.json({ error: describe(error) }, statusFor(error)); + return context.json(errorBody(error), statusFor(error)); } }); @@ -129,7 +130,7 @@ export function createComputerRoutes( if (error instanceof NavigationRefusedError) { return context.json({ error: error.message }, 403); } - return context.json({ error: describe(error) }, statusFor(error)); + return context.json(errorBody(error), statusFor(error)); } }); @@ -137,7 +138,7 @@ export function createComputerRoutes( try { return context.json(await gateway.snapshot(context.req.param("botId"))); } catch (error) { - return context.json({ error: describe(error) }, statusFor(error)); + return context.json(errorBody(error), statusFor(error)); } }); @@ -209,7 +210,7 @@ export function createComputerRoutes( try { return context.json(await gateway.control(context.req.param("botId"))); } catch (error) { - return context.json({ error: describe(error) }, statusFor(error)); + return context.json(errorBody(error), statusFor(error)); } }); @@ -248,7 +249,7 @@ export function createComputerRoutes( try { return context.json(await gateway.computers()); } catch (error) { - return context.json({ error: describe(error) }, statusFor(error)); + return context.json(errorBody(error), statusFor(error)); } }); @@ -262,7 +263,7 @@ export function createComputerRoutes( try { return context.json(await gateway.computers()); } catch (error) { - return context.json({ error: describe(error) }, statusFor(error)); + return context.json(errorBody(error), statusFor(error)); } }); @@ -355,7 +356,7 @@ export function createComputerRoutes( } as Parameters[1]), ); } catch (error) { - return context.json({ error: describe(error) }, statusFor(error)); + return context.json(errorBody(error), statusFor(error)); } }); @@ -548,7 +549,7 @@ async function act( if (error instanceof WorkspaceRequestError) { return context.json({ error: error.message }, 400); } - return context.json({ error: describe(error) }, statusFor(error)); + return context.json(errorBody(error), statusFor(error)); } } @@ -588,8 +589,27 @@ function describe(error: unknown): string { * not running (an operator fixes it), the refs are stale (the model fixes it by snapshotting again), * and everything else. Navigation established this; the acting routes follow it. */ +/** + * What a failed computer call looks like to the caller. + * + * One place, because the flag below decides what a Bot does next and a route that renders its own + * error would silently not carry it. The computer marks a takeover on both the acting path and + * navigate, and those two are answered by different code here, so the version of this that lived in + * the acting helper alone fixed one of them. + */ +function errorBody(error: unknown): Record { + return { + error: describe(error), + // Not "the refs are stale, take another snapshot", which is what the surface says without it. + ...(error instanceof HumanHasControlError ? { humanHasControl: true } : {}), + }; +} + function statusFor(error: unknown): 409 | 500 | 503 { if (error instanceof StaleSnapshotError) return 409; + // Same status as a stale snapshot and for the same reason: nothing is broken, the caller has to do + // something else first. What differs is what that something is, which the body carries. + if (error instanceof HumanHasControlError) return 409; // The same answer as a stale snapshot, because it is the same instruction: the refs are wrong, take // another snapshot. Not 503, which says the computer is unavailable and sends an operator hunting a // container that is running perfectly. diff --git a/server/tests/computer-client.test.ts b/server/tests/computer-client.test.ts index 66dd9d2e..f7c0f0eb 100644 --- a/server/tests/computer-client.test.ts +++ b/server/tests/computer-client.test.ts @@ -2,7 +2,9 @@ import { describe, expect, test } from "bun:test"; import { createComputerTransport, ElementNotFoundError, + HumanHasControlError, NavigationRefusedError, + StaleSnapshotError, } from "../src/computer/client"; function clientWith( @@ -191,6 +193,88 @@ describe("acting on an element that is not there", () => { }); /** + * A refusal because a person took the wheel, told apart from stale refs. + * + * Both arrive as 409, and the computer says which is which by putting `humanHasControl` on the body + * (agent-computer/src/index.ts). Mapping every 409 to the same condition throws that away, and the + * surface then tells the model its refs are stale and to take a fresh snapshot: advice that sends a + * Bot round the loop again against a person who has deliberately taken the browser. The status is + * the same either way; what differs is what the caller should do next, which is the only thing the + * distinction is for. + */ +describe("a 409 from the computer", () => { + const refusing = (body: unknown) => + clientWith( + () => + new Response(JSON.stringify(body), { + status: 409, + headers: { "content-type": "application/json" }, + }), + ); + + test("is a person holding the wheel when the computer says so", async () => { + expect( + refusing({ + error: "A person has taken control of this computer.", + humanHasControl: true, + }).click({ ref: "e1", snapshotId: 1 }), + ).rejects.toBeInstanceOf(HumanHasControlError); + }); + + test("is a stale snapshot when it does not", async () => { + // The permissive half, and the one that must not change: an ordinary stale-ref 409 carries no + // flag and keeps its own condition, so the refs-are-stale instruction still reaches the model. + expect( + refusing({ error: "Snapshot 3 is not the current one." }).click({ + ref: "e1", + snapshotId: 1, + }), + ).rejects.toBeInstanceOf(StaleSnapshotError); + }); + + test("is a stale snapshot for every shape that is not the flag", async () => { + // The flag decides an instruction a Bot acts on, so only the computer's own `true` counts. A + // string, a false, or a 409 with nothing on it at all stays the ordinary condition rather than + // parking a Bot to wait for a person who is not there. + for (const body of [ + { error: "no flag" }, + { error: "explicitly not a takeover", humanHasControl: false }, + { error: "a string is not a boolean", humanHasControl: "true" }, + {}, + ]) { + expect( + refusing(body).click({ ref: "e1", snapshotId: 1 }), + ).rejects.toBeInstanceOf(StaleSnapshotError); + } + }); + + test("a 409 with no body at all is still a refusal, not a crash", async () => { + const bodyless = clientWith(() => new Response(null, { status: 409 })); + expect(bodyless.click({ ref: "e1", snapshotId: 1 })).rejects.toBeInstanceOf( + StaleSnapshotError, + ); + }); + + test("carries the reason the computer gave, either way", async () => { + for (const body of [ + { + error: "A person has taken control of this computer.", + humanHasControl: true, + }, + { error: "Snapshot 3 is not the current one." }, + ]) { + try { + await refusing(body).click({ ref: "e1", snapshotId: 1 }); + throw new Error("should have refused"); + } catch (error) { + expect((error as Error).message).toBe(body.error); + } + } + }); +}); + +/** + * Stop has to travel./** * Stop has to travel. * * Pressing Stop aborts the surface's request. That abort is only useful if it reaches the browser: a diff --git a/server/tests/control-refusal-end-to-end.test.ts b/server/tests/control-refusal-end-to-end.test.ts new file mode 100644 index 00000000..0ec79f11 --- /dev/null +++ b/server/tests/control-refusal-end-to-end.test.ts @@ -0,0 +1,174 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import type { MiddlewareHandler } from "hono"; +import type { AuditEventInput, AuditStore } from "../src/audit"; +import type { AppVariables, AuthenticatedActor } from "../src/auth/guards"; +import { createComputerGateway } from "../src/computer/gateway"; +import type { PolicyStore } from "../src/computer/policy-store"; +import type { ComputerProvider } from "../src/computer/provider"; +import { createComputerRoutes } from "../src/computer/routes"; + +/** + * What a Bot is told when a person has taken the wheel, over a real socket. + * + * The computer answers a refused action with `409 { error, humanHasControl: true }`, and the surface + * reads that flag to decide what the model hears next: with it, a person has control; without it, + * `staleRefs`, which renders as "your refs are stale, the page changed, call this again with the new + * ones". So a flag lost in the middle is not a cosmetic loss. It sends a Bot back round the same + * action against somebody who deliberately took the browser, which is the one moment it should stop. + * + * Driven over a socket rather than a stubbed fetch because the property is that a field survives + * being serialised, thrown as an error, and re-serialised by the route. A stub proves the shape this + * process built; only a listener proves what the far side actually sent and what the caller finally + * reads. + */ + +const servers: Array<{ stop: (force?: boolean) => void }> = []; + +afterEach(() => { + for (const server of servers.splice(0)) server.stop(true); +}); + +/** The computer, refusing exactly the way `agent-computer/src/index.ts` does. */ +function computerAnswering(body: unknown, status: number) { + const server = Bun.serve({ + port: 0, + fetch: async () => Response.json(body, { status }), + }); + servers.push(server); + return `http://127.0.0.1:${server.port}`; +} + +function routesFor(baseUrl: string) { + const rows: AuditEventInput[] = []; + const provider: ComputerProvider = { + name: "test", + isolation: "per-bot", + locate: async () => baseUrl, + status: async (botId) => ({ botId, state: "ready" }), + stop: async () => ({ wasRunning: true }), + reset: async () => ({ cleared: true }), + list: async () => [], + }; + const gateway = createComputerGateway({ + provider, + auditStore: { + insert: async (event: AuditEventInput) => { + rows.push(event); + }, + } as unknown as AuditStore, + policy: () => ({ mode: "enforce", deny: [], allow: ["true"] }), + token: "computer-token-for-this-test", + }); + const actor: AuthenticatedActor = { + id: "user-1", + email: "member@openbot.test", + role: "user", + }; + const asActor: MiddlewareHandler<{ Variables: AppVariables }> = async ( + context, + next, + ) => { + context.set("actor", actor); + await next(); + }; + return { + rows, + app: createComputerRoutes( + gateway, + {} as PolicyStore, + asActor, + async () => true, + ), + }; +} + +async function navigate(baseUrl: string) { + const { app } = routesFor(baseUrl); + const response = await app.request("http://openbot.test/bot-1/navigate", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ url: "https://example.com/" }), + }); + return { + status: response.status, + body: (await response.json()) as Record, + }; +} + +async function click(baseUrl: string) { + const { app, rows } = routesFor(baseUrl); + const response = await app.request("http://openbot.test/bot-1/click", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ ref: "e1", snapshotId: 1 }), + }); + return { + status: response.status, + body: (await response.json()) as Record, + rows, + }; +} + +describe("a refusal because a person has the wheel", () => { + test("reaches the caller as one, not as stale refs", async () => { + const { status, body } = await click( + computerAnswering( + { + error: "A person has taken control of this computer.", + humanHasControl: true, + }, + 409, + ), + ); + + expect(status).toBe(409); + expect(body.humanHasControl).toBe(true); + expect(body.error).toBe("A person has taken control of this computer."); + }); + + test("a stale snapshot is still a stale snapshot", async () => { + // The half that must not move. Without this, marking every 409 as a takeover would pass the test + // above while telling a Bot with genuinely stale refs to go and wait for a person who is not there. + const { status, body } = await click( + computerAnswering({ error: "Snapshot 3 is not the current one." }, 409), + ); + + expect(status).toBe(409); + expect(body.humanHasControl).toBeUndefined(); + expect(body.error).toBe("Snapshot 3 is not the current one."); + }); + + test("reaches the caller from navigate too, which is the other place the computer refuses", async () => { + // The computer marks the takeover on its action path and on navigate (agent-computer/src/index.ts + // 938 and 721). Navigate does not go through the shared acting helper, so carrying the flag in + // that helper alone would fix one of the two and leave the other telling a Bot to re-snapshot. + const { status, body } = await navigate( + computerAnswering( + { + error: "A person has taken control of this computer.", + humanHasControl: true, + }, + 409, + ), + ); + + expect(status).toBe(409); + expect(body.humanHasControl).toBe(true); + }); + + test("the refusal is still audited as a refusal", async () => { + // The gateway writes a row when a forwarded action fails. Telling the caller something new must + // not quietly cost the trail the record that the action did not happen. + const { rows } = await click( + computerAnswering( + { + error: "A person has taken control of this computer.", + humanHasControl: true, + }, + 409, + ), + ); + + expect(rows.length).toBeGreaterThan(0); + }); +});