From 41ac47c456905369439ec532cd8251cd6b207534 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sat, 22 Aug 2026 08:39:03 -0500 Subject: [PATCH 1/2] Tell a Bot a person has the wheel, not that its refs are stale The computer refuses an action with `409 { error, humanHasControl: true }` when somebody has taken the browser, and the surface already branches on that flag: with it the model is told a person has control, without it it is told `staleRefs`, which its own tool description turns into "the page changed, call this again and use the new refs". The flag did not survive the server. Every 409 became a `StaleSnapshotError`, which carries only a message, so the branch in the surface was unreachable and a Bot refused by a takeover was sent back round the same action against the person who had just taken the browser. A takeover is now its own condition through the transport and the route. The status is unchanged, 409 either way, because nothing is broken in either case; what differs is what the caller should do next, and that is the only thing the distinction is for. An ordinary stale-ref 409 still maps as it did. Told apart by the flag rather than by the message, because the message is the computer's prose and matching on it would break the moment the wording changed. --- app/src/lib/copilot/computer-tools.tsx | 9 +- app/tests/computer-tool-refusals.test.ts | 65 +++++++ server/src/computer/client.ts | 19 ++ server/src/computer/gateway.ts | 1 + server/src/computer/routes.ts | 38 +++- server/tests/computer-client.test.ts | 84 +++++++++ .../tests/control-refusal-end-to-end.test.ts | 174 ++++++++++++++++++ 7 files changed, 380 insertions(+), 10 deletions(-) create mode 100644 app/tests/computer-tool-refusals.test.ts create mode 100644 server/tests/control-refusal-end-to-end.test.ts 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..2e5ed90f --- /dev/null +++ b/app/tests/computer-tool-refusals.test.ts @@ -0,0 +1,65 @@ +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..3a52536a 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); + }); +}); From dc1d7e4fd5cc10f2cb1868036697d591bedd22c5 Mon Sep 17 00:00:00 2001 From: David McKay Date: Sat, 22 Aug 2026 08:11:54 -0700 Subject: [PATCH 2/2] Format two test files the way biome does --- app/tests/computer-tool-refusals.test.ts | 5 ++++- server/tests/computer-client.test.ts | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/app/tests/computer-tool-refusals.test.ts b/app/tests/computer-tool-refusals.test.ts index 2e5ed90f..8404f7fb 100644 --- a/app/tests/computer-tool-refusals.test.ts +++ b/app/tests/computer-tool-refusals.test.ts @@ -54,7 +54,10 @@ describe("a computer call the server refused", () => { }); test("a policy refusal is neither", async () => { - serverAnswering(403, { error: "That is not allowed here.", rule: "url.host == \"example.com\"" }); + serverAnswering(403, { + error: "That is not allowed here.", + rule: 'url.host == "example.com"', + }); const outcome = await callComputer("bot-1", "/click", { method: "POST" }); diff --git a/server/tests/computer-client.test.ts b/server/tests/computer-client.test.ts index 3a52536a..f7c0f0eb 100644 --- a/server/tests/computer-client.test.ts +++ b/server/tests/computer-client.test.ts @@ -250,9 +250,9 @@ describe("a 409 from the computer", () => { 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); + expect(bodyless.click({ ref: "e1", snapshotId: 1 })).rejects.toBeInstanceOf( + StaleSnapshotError, + ); }); test("carries the reason the computer gave, either way", async () => {