Tell a Bot a person has the wheel, not that its refs are stale

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,
   /*

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();
+  });
+});

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) {

server/src/computer/gateway.ts

@@ -24,6 +24,7 @@ import { checkComputerAddress } from "./target";
 export {
   ComputerUnavailableError,
   ElementNotFoundError,
+  HumanHasControlError,
   NavigationRefusedError,
   StaleSnapshotError,
   WorkspaceRefusedError,

server/src/computer/routes.ts

@@ -10,6 +10,7 @@ import {
   ComputerUnavailableError,
   ElementNotFoundError,
   NavigationRefusedError,
+  HumanHasControlError,
   StaleSnapshotError,
   WorkspaceRefusedError,
   WorkspaceRequestError,
@@ -86,15 +87,15 @@ 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));
     }
   });
 
   routes.get("/:botId/read", async (context) => {
     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,15 +130,15 @@ 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));
     }
   });
 
   routes.post("/:botId/snapshot", async (context) => {
     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<typeof gateway.humanInput>[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<string, unknown> {
+  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.

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