diff --git a/CHANGELOG.md b/CHANGELOG.md index 07a185a18..99e163589 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,15 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A person's Stop is recorded as a stop, not as a failed action + +Pressing Stop mid-action aborts the request, and the gateway wrote that outcome beside the decision +row as a failure — the same `computer.action_failed` type it writes when a computer is unreachable or +times out. The row's message already said the action was stopped, but anything counting failures by +type, the natural way to watch for outages, read every Stop as one. A stop now writes its own type, +`computer.action_stopped`: the action did not happen and nothing broke. The audit page already groups +it with the other did-not-happen outcomes, and a policy dry-run skips it the way it skips a failure, +since both sit beside a decision row that is already scored. ### A malformed `DATABASE_URL` is refused without printing the password `DATABASE_URL` is taken apart before it reaches Bun, and the string most likely to fail that parse is diff --git a/app/src/lib/audit/outcome.ts b/app/src/lib/audit/outcome.ts index a114211c0..0fb5970d9 100644 --- a/app/src/lib/audit/outcome.ts +++ b/app/src/lib/audit/outcome.ts @@ -56,6 +56,8 @@ export const REFUSED_EVENT_TYPES = [ */ export const DID_NOT_HAPPEN_EVENT_TYPES = [ "computer.action_failed", + /** A person pressed Stop mid-action: the action did not happen, and nothing broke. */ + "computer.action_stopped", "agent.stream_stalled", /** A hop that was accepted, ran out of attempts, and never became the other Bot's turn. */ "agent.handoff_failed", diff --git a/server/src/audit.ts b/server/src/audit.ts index e057138dc..2b5357c41 100644 --- a/server/src/audit.ts +++ b/server/src/audit.ts @@ -194,6 +194,10 @@ export const auditEventTypes = [ // Permitted by policy, attempted, and did not succeed. Its own type because "allowed" reads as // "happened", and a trail that cannot tell those apart misleads exactly when it matters most. "computer.action_failed", + // Permitted, attempted, and stopped mid-action by a person pressing Stop. Kept apart from + // `action_failed` because a stop is not an outage: a count of failures that folds in every Stop + // reports a broken computer where somebody simply changed their mind. + "computer.action_stopped", // A person taking the wheel and giving it back. Recorded as a period rather than as keystrokes: the // useful fact for an investigator is that a human drove this browser between these two times, and // logging every click a person made would bury it while telling nobody anything. diff --git a/server/src/computer/client.ts b/server/src/computer/client.ts index a322de1e9..bac3890a8 100644 --- a/server/src/computer/client.ts +++ b/server/src/computer/client.ts @@ -9,6 +9,22 @@ export class ComputerUnavailableError extends Error { } } +/** + * A person pressed Stop and the action was aborted mid-flight. + * + * A subclass of `ComputerUnavailableError` on purpose: everything downstream that catches an + * unavailable computer to tell the model still catches this unchanged. What it adds is a type the + * gateway can see, so the audit row it writes is `computer.action_stopped` rather than + * `computer.action_failed` -- a stop is not an outage, and a count of failures that includes every + * Stop reports one where there was none. + */ +export class ComputerStoppedError extends ComputerUnavailableError { + constructor(reason: string) { + super(reason); + this.name = "ComputerStoppedError"; + } +} + /** The requested element is not on the current page. */ export class ElementNotFoundError extends Error { constructor(reason: string) { @@ -125,7 +141,7 @@ export function createComputerTransport( timeoutMsOverride?: number, ): Promise { if (caller?.aborted) { - throw new ComputerUnavailableError("The action was stopped."); + throw new ComputerStoppedError("The action was stopped."); } /* @@ -161,11 +177,11 @@ export function createComputerTransport( * The signal is handed to fetch precisely so a Stop can land mid-flight, and a fetch aborted * that way rejects with an AbortError, which is neither a TimeoutError nor a computer that is * not running. Both of the other answers are statements about the infrastructure, and this - * message is not only read by the model: the gateway writes it into the action's audit row as - * `failure`, so a person pressing Stop was recorded as an outage. + * message is not only read by the model: the gateway writes it into the action's audit row, + * and the type below is what keeps that row a stop rather than an outage. */ if (caller?.aborted) { - throw new ComputerUnavailableError("The action was stopped."); + throw new ComputerStoppedError("The action was stopped."); } throw new ComputerUnavailableError( error instanceof Error && error.name === "TimeoutError" diff --git a/server/src/computer/gateway.ts b/server/src/computer/gateway.ts index 99f0cf094..ffe66ab2d 100644 --- a/server/src/computer/gateway.ts +++ b/server/src/computer/gateway.ts @@ -19,6 +19,7 @@ */ import { type AuditStore, recordAuditEvent } from "../audit"; import { + ComputerStoppedError, ComputerUnavailableError, createComputerTransport, StaleSnapshotError, @@ -603,6 +604,10 @@ export function createComputerGateway( pageUrl, decision, failure: error instanceof Error ? error.message : "The action failed.", + // A person pressing Stop mid-action is not the computer failing. The message still says so; + // this keeps the row's type a stop, so a count of failed actions does not read every Stop as + // an outage. + ...(error instanceof ComputerStoppedError ? { stopped: true } : {}), }); throw error; } @@ -1075,16 +1080,22 @@ async function write( command?: string; /** Set only when a permitted action was attempted and did not succeed. */ failure?: string; + /** Set when that non-success was a person pressing Stop, so the row is typed a stop, not a failure. */ + stopped?: boolean; }, ) { await recordAuditEvent(auditStore, { // A failure is its own kind of event, not a variant of "allowed": the whole point of the extra row // is that a reader can tell an action that happened from one that was permitted and then did not. - eventType: entry.failure - ? "computer.action_failed" - : entry.decision.allowed - ? "computer.action_allowed" - : "computer.action_refused", + // A stop is a third kind again: the action did not happen, but nothing broke, so it is neither a + // failure to be counted as an outage nor an action that was carried out. + eventType: entry.stopped + ? "computer.action_stopped" + : entry.failure + ? "computer.action_failed" + : entry.decision.allowed + ? "computer.action_allowed" + : "computer.action_refused", targetType: "computer", targetId: entry.botId, // Only ever a real users row. The audit table has a foreign key to it, so writing the local diff --git a/server/src/computer/policy-dry-run.ts b/server/src/computer/policy-dry-run.ts index c79972d05..46f3347cc 100644 --- a/server/src/computer/policy-dry-run.ts +++ b/server/src/computer/policy-dry-run.ts @@ -148,7 +148,13 @@ export function dryRunAgainstHistory( }; for (const event of events) { - if (event.eventType === "computer.action_failed") continue; + // Both carry a decision row already in this history, so scoring the outcome row would count the + // action twice; a stop is skipped for the same reason a failure is. + if ( + event.eventType === "computer.action_failed" || + event.eventType === "computer.action_stopped" + ) + continue; const context = contextFromAuditPayload(event.payload); if (!context) continue; report.scanned += 1; diff --git a/server/tests/computer-client.test.ts b/server/tests/computer-client.test.ts index ef25d877b..ce9059eec 100644 --- a/server/tests/computer-client.test.ts +++ b/server/tests/computer-client.test.ts @@ -1,5 +1,7 @@ import { describe, expect, test } from "bun:test"; import { + ComputerStoppedError, + ComputerUnavailableError, createComputerTransport, ElementNotFoundError, HumanHasControlError, @@ -310,9 +312,18 @@ describe("the caller's Stop", () => { const stop = new AbortController(); stop.abort(); - expect( - client.click({ ref: "e1", snapshotId: 1 }, stop.signal), - ).rejects.toBeDefined(); + // A stop, and a distinguishable one: `ComputerStoppedError` so the gateway types the audit row + // `computer.action_stopped` rather than counting a person's Stop as a failed action. It stays a + // subclass of `ComputerUnavailableError`, so everything catching an unavailable computer to tell + // the model is unaffected. + const error = await client + .click({ ref: "e1", snapshotId: 1 }, stop.signal) + .then( + () => null, + (reason) => reason, + ); + expect(error).toBeInstanceOf(ComputerStoppedError); + expect(error).toBeInstanceOf(ComputerUnavailableError); expect(called).toBe(false); }); diff --git a/server/tests/computer-gateway.test.ts b/server/tests/computer-gateway.test.ts index 75518a69d..89e6dbadc 100644 --- a/server/tests/computer-gateway.test.ts +++ b/server/tests/computer-gateway.test.ts @@ -435,6 +435,30 @@ describe("the computer gateway", () => { expect(rows[0]?.payload.element).toBeUndefined(); }); + test("a permitted action stopped mid-flight is recorded as a stop, not a failure", async () => { + // A person pressing Stop is not the computer failing. The row still says so in its message, but + // its TYPE is `computer.action_stopped`, so a count of `action_failed` rows — the natural way to + // measure outages — does not read every Stop as one. + const { gateway, rows } = await gatewayWith(PERMISSIVE); + const stop = new AbortController(); + stop.abort(); + + await expect( + gateway.runCommand( + "bot-1", + ACTOR, + { command: "cat secrets.txt" }, + stop.signal, + ), + ).rejects.toThrow(/stopped/); + + // The decision that permitted it, then the outcome — a stop, not a failure. + expect(rows).toHaveLength(2); + expect(rows[0]?.eventType).toBe("computer.action_allowed"); + expect(rows[1]?.eventType).toBe("computer.action_stopped"); + expect(rows[1]?.payload.failure).toContain("stopped"); + }); + test("a command the policy refuses is recorded and never reaches the computer", async () => { const { gateway, calls, rows } = await gatewayWith({ ...PERMISSIVE,