diff --git a/apps/server/src/features/browser-automation/execution/__tests__/broker.test.ts b/apps/server/src/features/browser-automation/execution/__tests__/broker.test.ts index e4a5efdf6..fdb2dff06 100644 --- a/apps/server/src/features/browser-automation/execution/__tests__/broker.test.ts +++ b/apps/server/src/features/browser-automation/execution/__tests__/broker.test.ts @@ -284,7 +284,7 @@ describe("BrowserAutomationBroker", () => { result: { controlEpoch: 3, readiness: { ready: false, state: "human-control" }, - guidance: "The user has control of the Browser. Do not act until browser_inspect reports ready; the user hands control back.", + guidance: "The user has control of the Browser. browser_inspect can still read any tab. Do not act until browser_inspect reports ready; the user hands control back.", }, }); broker.disconnect(hostSocket); diff --git a/apps/server/src/features/browser-automation/execution/__tests__/broker.user-tab.test.ts b/apps/server/src/features/browser-automation/execution/__tests__/broker.user-tab.test.ts new file mode 100644 index 000000000..3d019d5df --- /dev/null +++ b/apps/server/src/features/browser-automation/execution/__tests__/broker.user-tab.test.ts @@ -0,0 +1,278 @@ +import { describe, expect, it } from "vitest"; +import { + BROWSER_AUTOMATION_CONTRACT_VERSION, + BROWSER_AUTOMATION_OPERATIONS, + type BrowserAutomationHostDispatchTarget, + type BrowserAutomationHostRegistration, + type BrowserAutomationRequest, +} from "@mcode/contracts"; +import type { WebSocket } from "ws"; +import { BrowserAutomationBroker } from "../broker.js"; +import type { BrowserAutomationCredentialClaims } from "../../access/credential-registry.js"; + +const HOST_ID = "user-tab-host"; +const USER_TAB = "user-tab"; +const AGENT_TAB = "agent-tab"; + +type Controller = NonNullable; + +interface Delivery { + readonly dispatch: { readonly request: BrowserAutomationRequest; readonly target: BrowserAutomationHostDispatchTarget }; +} + +function registration(): BrowserAutomationHostRegistration { + return { + contractVersion: BROWSER_AUTOMATION_CONTRACT_VERSION, + hostId: HOST_ID, + runtime: "electron", + desktopInstanceId: `desktop-${HOST_ID}`, + worktreeIdentity: "worktree-a", + workspaceIds: ["workspace-a"], + executorDescriptor: { + runtime: "electron", + operations: [...BROWSER_AUTOMATION_OPERATIONS], + constraints: { maxTabs: 32, maxSnapshotChars: 20_000, maxDiagnostics: 200 }, + capabilityRevision: 1, + }, + capabilities: BROWSER_AUTOMATION_OPERATIONS.map((operation) => ({ operation, available: true })), + maxPendingRequests: 4, + connectedAt: 1, + }; +} + +const scope: BrowserAutomationCredentialClaims = { + credentialId: "credential-thread-a", + providerId: "cursor", + providerSessionId: "provider-thread-a", + mcodeSessionId: "mcode-thread-a", + threadId: "thread-a", + workspaceId: "workspace-a", + worktreeIdentity: "worktree-a", + permissionCapability: "interact", + allowedOperations: ["open", "inspect", "act", "tabs"], + issuedAt: 1, + expiresAt: 100_000, +}; + +function target( + generation: number, + tabId: string, + overrides: Partial = {}, +): BrowserAutomationHostDispatchTarget { + return { + desktopInstanceId: `desktop-${HOST_ID}`, + windowId: 1, + connectionGeneration: generation, + threadId: "thread-a", + tabId, + targetGeneration: 1, + active: false, + focused: false, + lastUsedAt: 1, + ...overrides, + }; +} + +function userHold(epoch: number): Controller { + return { tabId: USER_TAB, controller: "human", controlEpoch: epoch }; +} + +/** A Browser with a user-opened tab and an agent tab in one thread, reachable through one host. */ +function browser(options: { readonly userController?: Controller; readonly focusedTab?: string; readonly agentTab?: boolean } = {}) { + const deliveries: Delivery[] = []; + const broker = new BrowserAutomationBroker({ + now: () => 10, + send: (_socket, channel, data) => { + if (channel === "browserAutomation.request") deliveries.push(data as Delivery); + return true; + }, + }); + const hostSocket = { name: HOST_ID } as unknown as WebSocket; + const generation = broker.registerHost(hostSocket, registration(), { + desktopInstanceId: `desktop-${HOST_ID}`, + worktreeIdentity: "worktree-a", + allowedWorkspaceIds: ["workspace-a"], + }).generation; + const focusedTab = options.focusedTab ?? USER_TAB; + const publish = (userController: Controller | undefined): void => { + broker.updateTargets(hostSocket, HOST_ID, generation, [ + target(generation, USER_TAB, { + active: true, + focused: focusedTab === USER_TAB, + ...(userController ? { controller: userController } : {}), + }), + ...(options.agentTab === false ? [] : [target(generation, AGENT_TAB, { focused: focusedTab === AGENT_TAB, lastUsedAt: 2 })]), + ]); + }; + publish(options.userController); + let sequence = 0; + const send = (operation: BrowserAutomationRequest["operation"], args: Record, expectedControlEpoch = 0) => { + sequence += 1; + const request = { + contractVersion: BROWSER_AUTOMATION_CONTRACT_VERSION, + workspaceId: scope.workspaceId, + threadId: scope.threadId, + providerSessionId: scope.providerSessionId, + providerInstanceId: scope.mcodeSessionId, + requestId: `request-${sequence}`, + sequence, + deadline: 50_000, + expectedControlEpoch, + operation, + args, + } as BrowserAutomationRequest; + return broker.execute(scope, request); + }; + /** Answers the latest delivery the way a host answers an inspect of its target. */ + const answerInspect = (controlEpoch = 0): Delivery => { + const delivery = deliveries.at(-1)!; + const inspected = delivery.dispatch.target; + broker.respond(hostSocket, HOST_ID, generation, { + contractVersion: BROWSER_AUTOMATION_CONTRACT_VERSION, + requestId: delivery.dispatch.request.requestId, + sequence: delivery.dispatch.request.sequence, + ok: true, + result: { + operation: "inspect", + controlEpoch, + // Hosts list the thread's tabs without their holder. + tabs: [target(generation, USER_TAB, { active: true }), target(generation, AGENT_TAB, { lastUsedAt: 2 })], + observationRef: `observation-${delivery.dispatch.request.sequence}`, + snapshot: snapshotOf(inspected.tabId), + }, + }); + return delivery; + }; + return { broker, deliveries, publish, send, answerInspect }; +} + +function snapshotOf(tabId: string) { + const none = { truncated: false as const }; + return { + url: `https://example.test/${tabId}`, + title: `Page in ${tabId}`, + loading: false, + visibleText: `Content of ${tabId}`, + visibleTextTruncation: none, + elements: [], + elementsTruncation: none, + accessibility: [], + accessibilityTruncation: none, + console: [], + consoleTruncation: none, + network: [], + networkTruncation: none, + actions: [], + actionsTruncation: none, + }; +} + +const INSPECT = { includeScreenshot: false, includeDiagnostics: false }; + +describe("BrowserAutomationBroker reading the user's own tab", () => { + it("reads the user's tab when the agent has no tab of its own", async () => { + const { send, answerInspect } = browser({ agentTab: false }); + + const pending = send("inspect", INSPECT); + const delivery = answerInspect(); + + expect(delivery.dispatch.target.tabId).toBe(USER_TAB); + await expect(pending).resolves.toMatchObject({ + ok: true, + result: { snapshot: { url: `https://example.test/${USER_TAB}`, title: `Page in ${USER_TAB}`, visibleText: `Content of ${USER_TAB}` } }, + }); + }); + + it("reads a named tab the user holds without moving the agent off its own tab", async () => { + const { send, answerInspect } = browser({ focusedTab: AGENT_TAB, userController: userHold(2) }); + const sticky = send("inspect", INSPECT); + expect(answerInspect(2).dispatch.target.tabId).toBe(AGENT_TAB); + await sticky; + + const named = send("inspect", { ...INSPECT, tabId: USER_TAB }); + expect(answerInspect(2).dispatch.target.tabId).toBe(USER_TAB); + await expect(named).resolves.toMatchObject({ + ok: true, + result: { target: { tabId: USER_TAB }, snapshot: { visibleText: `Content of ${USER_TAB}` } }, + }); + + const next = send("inspect", INSPECT); + expect(answerInspect(2).dispatch.target.tabId).toBe(AGENT_TAB); + await next; + }); + + it("keeps the user's hold after the agent reads the held tab", async () => { + const { send, answerInspect } = browser({ userController: userHold(2) }); + + const first = send("inspect", { ...INSPECT, tabId: USER_TAB }); + answerInspect(2); + await first; + const second = send("inspect", { ...INSPECT, tabId: USER_TAB }); + answerInspect(2); + + await expect(second).resolves.toMatchObject({ + ok: true, + result: { readiness: { ready: false, state: "human-control" }, tabs: expect.arrayContaining([expect.objectContaining({ tabId: USER_TAB, controller: userHold(2) })]) }, + }); + }); + + it("refuses a named tab that is not in the thread", async () => { + const { send, deliveries } = browser(); + + await expect(send("inspect", { ...INSPECT, tabId: "elsewhere" })).resolves.toMatchObject({ + ok: false, + error: { code: "TAB_UNAVAILABLE", recovery: "inspect" }, + }); + expect(deliveries).toHaveLength(0); + }); + + it("treats a listed tab on a busy host as unavailable for now, not as missing", async () => { + const { send, deliveries } = browser(); + for (let index = 0; index < registration().maxPendingRequests; index += 1) void send("inspect", INSPECT); + + await expect(send("inspect", { ...INSPECT, tabId: USER_TAB })).resolves.toMatchObject({ + ok: false, + error: { code: "TAB_UNAVAILABLE", retryable: true }, + }); + expect(deliveries).toHaveLength(registration().maxPendingRequests); + }); + + it("lists every thread tab with the holder its host published", async () => { + const { send, answerInspect } = browser({ userController: userHold(4) }); + + const pending = send("inspect", { ...INSPECT, tabId: AGENT_TAB }); + answerInspect(4); + + const response = await pending; + expect(response.ok && response.result.operation === "inspect" && response.result.tabs.map((tab) => [tab.tabId, tab.controller?.controller])) + .toEqual([[AGENT_TAB, undefined], [USER_TAB, "human"]]); + }); + + it("reports the Browser as not ready while the user holds another tab in the thread", async () => { + const { send, answerInspect } = browser({ userController: userHold(1) }); + + const pending = send("inspect", { ...INSPECT, tabId: AGENT_TAB }); + answerInspect(1); + + await expect(pending).resolves.toMatchObject({ + ok: true, + result: { readiness: { ready: false, state: "human-control" } }, + }); + }); + + it("refuses an action on the user's held tab and admits it after Hand back", async () => { + const { send, publish, deliveries } = browser({ agentTab: false, userController: userHold(1) }); + const act = { idempotencyKey: "act-key", observationRef: "observation", deadlineMs: 1_000, steps: [{ operation: "press", key: "A", modifiers: [] }] }; + + await expect(send("act", act, 1)).resolves.toMatchObject({ + ok: false, + error: { code: "HUMAN_INTERRUPTED", recovery: "yield_to_user" }, + }); + expect(deliveries).toHaveLength(0); + + publish({ tabId: USER_TAB, controller: "none", controlEpoch: 2 }); + void send("act", { ...act, idempotencyKey: "act-after-hand-back" }, 2); + + expect(deliveries.map(({ dispatch }) => [dispatch.request.operation, dispatch.target.tabId])).toEqual([["act", USER_TAB]]); + }); +}); diff --git a/apps/server/src/features/browser-automation/execution/broker.ts b/apps/server/src/features/browser-automation/execution/broker.ts index eed573f2b..bf1e5c5c4 100644 --- a/apps/server/src/features/browser-automation/execution/broker.ts +++ b/apps/server/src/features/browser-automation/execution/broker.ts @@ -183,8 +183,8 @@ function failure( code: BrowserAutomationErrorCode, message: string, retryable: boolean, + recovery: ReturnType = browserV2Recovery(code), ): BrowserAutomationResponse { - const recovery = browserV2Recovery(code); return { contractVersion: BROWSER_AUTOMATION_CONTRACT_VERSION, requestId: request.requestId, @@ -202,6 +202,11 @@ function failure( }; } +/** A named tab outside the thread is a bad reference, so the agent re-reads the tab list instead of reopening. */ +function unlistedTabFailure(request: BrowserAutomationRequest): BrowserAutomationResponse { + return failure(request, "TAB_UNAVAILABLE", "That tab is not open in this thread; browser_inspect lists the thread's tabs", false, "inspect"); +} + function assignmentKeyParts( providerId: string, providerSessionId: string, @@ -429,9 +434,17 @@ function inspectCapabilities( : capabilities.filter((operation) => operation !== "act"); } -function inspectReadiness(target: BrowserAutomationHostDispatchTarget | undefined): { ready: boolean; state: "ready" | "target-unavailable" | "human-control"; reason?: string } { +/** The thread tab the user holds, if any. ADR-0029 makes the hold thread-wide, so it blocks effects on every tab. */ +function userHeldTarget(host: RegisteredHost, threadId: string): BrowserAutomationHostDispatchTarget | undefined { + return [...host.targets.values()].find((target) => target.threadId === threadId && target.controller?.controller === "human"); +} + +function inspectReadiness( + target: BrowserAutomationHostDispatchTarget | undefined, + userHeld: BrowserAutomationHostDispatchTarget | undefined, +): { ready: boolean; state: "ready" | "target-unavailable" | "human-control"; reason?: string } { if (!target) return { ready: false, state: "target-unavailable", reason: "Visible browser target is unavailable" }; - if (target.controller?.controller === "human") return { ready: false, state: "human-control", reason: "Visible Preview is under human control" }; + if (userHeld) return { ready: false, state: "human-control", reason: "Visible Preview is under human control" }; return { ready: true, state: "ready" }; } @@ -451,19 +464,28 @@ function boundedInspectSnapshot( }; } +/** + * Hosts list a thread's tabs without their holder, so each listed tab takes the + * controller the broker admits effects against. + */ function inspectTabs( responseTabs: BrowserInspectResult["tabs"], target: BrowserAutomationHostDispatchTarget | undefined, + host: RegisteredHost, ): BrowserInspectResult["tabs"] { - return (target ? [target, ...responseTabs.filter((tab) => tab.tabId !== target.tabId)] : responseTabs) as BrowserInspectResult["tabs"]; + const tabs = target ? [target, ...responseTabs.filter((tab) => tab.tabId !== target.tabId)] : responseTabs; + return tabs.map(({ controller: _reported, ...tab }) => { + const controller = host.targets.get(targetKey(tab))?.controller; + return controller ? { ...tab, controller } : tab; + }); } function inspectGuidance( descriptor: RegisteredHost["registration"]["executorDescriptor"], - target: BrowserAutomationHostDispatchTarget | undefined, + userHeld: BrowserAutomationHostDispatchTarget | undefined, capabilities: BrowserAutomationPublicOperation[], ): string { - if (target?.controller?.controller === "human") return "The user has control of the Browser. Do not act until browser_inspect reports ready; the user hands control back."; + if (userHeld) return "The user has control of the Browser. browser_inspect can still read any tab. Do not act until browser_inspect reports ready; the user hands control back."; return `Visible browser executor (${descriptor.runtime}) supports: ${capabilities.join(", ") || "none"}.`; } @@ -478,19 +500,20 @@ function shapeNegotiatedResponse( const observationRef = inspectObservationReference(response.result); const capabilities = inspectCapabilities(negotiated, observationRef); const diagnostics = response.result.diagnostics?.slice(0, descriptor.constraints.maxDiagnostics); + const userHeld = userHeldTarget(pending.host, pending.request.threadId); return { ...response, result: { ...response.result, - readiness: inspectReadiness(pending.target), + readiness: inspectReadiness(pending.target, userHeld), target: pending.target && { threadId: pending.target.threadId, tabId: pending.target.tabId, targetGeneration: pending.target.targetGeneration, sticky: true }, - tabs: inspectTabs(response.result.tabs, pending.target).slice(0, descriptor.constraints.maxTabs), + tabs: inspectTabs(response.result.tabs, pending.target, pending.host).slice(0, descriptor.constraints.maxTabs), snapshot: boundedInspectSnapshot(response.result.snapshot, descriptor.constraints.maxSnapshotChars), ...(diagnostics ? { diagnostics } : {}), ...(observationRef ? { observationRef } : !negotiated.includes("act") ? { observationRef: NodeCrypto.randomUUID() } : {}), capabilities, capabilityRevision: descriptor.capabilityRevision, - guidance: inspectGuidance(descriptor, pending.target, capabilities).slice(0, 4_000), + guidance: inspectGuidance(descriptor, userHeld, capabilities).slice(0, 4_000), }, }; } @@ -1235,6 +1258,8 @@ export class BrowserAutomationBroker { ): RoutingCandidate | undefined { if (request.operation === "open" && request.args.idempotencyKey !== undefined) return undefined; if (request.operation === "tabs") return this.resolveTabsAssignment(claims, request); + // A named read leaves the sticky assignment alone: reading a tab never selects it. + if (request.operation === "inspect" && request.args.tabId) return this.resolveExplicitTarget(claims, request, request.args.tabId); return this.resolveAssignment(claims, request); } @@ -1247,6 +1272,9 @@ export class BrowserAutomationBroker { const host = this.resolveBootstrapHost(claims, request); if (host) return this.executeBootstrap(host, claims.providerId, request); } + if (request.operation === "inspect" && request.args.tabId && !this.threadListsTab(claims, request.args.tabId) && this.resolveSelectedHost(claims)) { + return finishImmediately(unlistedTabFailure(request)); + } const code = this.resolveSelectedHost(claims) ? "TAB_UNAVAILABLE" : "HOST_UNAVAILABLE"; const message = code === "TAB_UNAVAILABLE" ? "No authorized visible browser tab is available" @@ -1622,6 +1650,12 @@ export class BrowserAutomationBroker { return candidates[0]; } + /** Whether any host in scope lists the tab, busy or not, so a busy host is not reported as a missing tab. */ + private threadListsTab(claims: BrowserAutomationCredentialClaims, tabId: string): boolean { + return [...this.hostsBySocket.values()].some((host) => this.matchesScope(host, claims) && + [...host.targets.values()].some((target) => target.threadId === claims.threadId && target.tabId === tabId)); + } + private canAccept( host: RegisteredHost, claims: BrowserAutomationCredentialClaims, @@ -1681,9 +1715,7 @@ export class BrowserAutomationBroker { } private targetDeliveryError(pending: PendingRequest): BrowserAutomationResponse | null { - if (pending.request.operation !== "inspect" && [...pending.host.targets.values()].some((target) => - target.threadId === pending.request.threadId && target.controller?.controller === "human" - )) { + if (pending.request.operation !== "inspect" && userHeldTarget(pending.host, pending.request.threadId)) { return failure(pending.request, "HUMAN_INTERRUPTED", "The user has control of the Browser until Hand back", true); } if (!pending.target) return null; diff --git a/apps/server/src/features/mcode-mcp/__tests__/browser-toolkit.test.ts b/apps/server/src/features/mcode-mcp/__tests__/browser-toolkit.test.ts index dfc435367..01cfb536c 100644 --- a/apps/server/src/features/mcode-mcp/__tests__/browser-toolkit.test.ts +++ b/apps/server/src/features/mcode-mcp/__tests__/browser-toolkit.test.ts @@ -342,6 +342,44 @@ describe("BrowserToolkit", () => { }); }); + it("reads a named tab by passing its tabId to the broker", async () => { + const page = fakeBrowser(); + const { call, requestAt } = setup({ execute: page.execute }); + + await call("browser_inspect", { tabId: "user-tab" }); + + expect(requestAt(0)).toMatchObject({ operation: "inspect", args: { tabId: "user-tab" } }); + }); + + it("lists each tab's holder, counting a user hold on any tab as holding the whole thread", async () => { + const { call, execute } = setup(); + const tab = (tabId: string, controller?: "human" | "agent") => ({ + desktopInstanceId: "desktop", + windowId: 1, + connectionGeneration: 1, + threadId: "thread-a", + tabId, + targetGeneration: 1, + active: tabId === "user-tab", + focused: false, + lastUsedAt: 1, + ...(controller ? { controller: { tabId, controller, controlEpoch: 3 } } : {}), + }); + const listing = (tabs: ReturnType[]) => execute.mockImplementationOnce(async (_claims, input) => responseFor(input, { operation: "inspect", controlEpoch: 3, tabs })); + + listing([tab("agent-tab", "agent"), tab("user-tab")]); + expect(JSON.parse(textOf(await call("browser_inspect"))).tabs).toEqual([ + { tabId: "agent-tab", active: false, focused: false, holder: "agent" }, + { tabId: "user-tab", active: true, focused: false, holder: "none" }, + ]); + + listing([tab("agent-tab"), tab("user-tab", "human")]); + expect(JSON.parse(textOf(await call("browser_inspect"))).tabs).toEqual([ + { tabId: "agent-tab", active: false, focused: false, holder: "user" }, + { tabId: "user-tab", active: true, focused: false, holder: "user" }, + ]); + }); + it("rejects caller-supplied scope, internal fields, and invalid argument shapes before the broker", async () => { const page = fakeBrowser(); const { call, execute } = setup({ execute: page.execute }); diff --git a/apps/server/src/features/mcode-mcp/toolkits/browser-tool-refs.ts b/apps/server/src/features/mcode-mcp/toolkits/browser-tool-refs.ts index 402536350..59ab2e797 100644 --- a/apps/server/src/features/mcode-mcp/toolkits/browser-tool-refs.ts +++ b/apps/server/src/features/mcode-mcp/toolkits/browser-tool-refs.ts @@ -202,14 +202,30 @@ export function advanceBrowserToolLedger( return { controlEpoch: ledger.controlEpoch }; } +/** + * Lists tabs without routing internals or control epochs. ADR-0029 makes a user + * hold thread-wide, and hosts differ on whether they mark every thread tab, so a + * user hold on any tab is reported on all of them. + */ +function projectTabs(tabs: Result<"inspect">["tabs"]): Array<{ tabId: string; active: boolean; focused: boolean; holder: "user" | "agent" | "none" }> { + const userHolds = tabs.some((tab) => tab.controller?.controller === "human"); + return tabs.map(({ tabId, active, focused, controller }) => ({ + tabId, + active, + focused, + holder: userHolds ? "user" : controller?.controller === "agent" ? "agent" : "none", + })); +} + function projectInspect(result: Result<"inspect">, snapshotId: number | undefined): Record { - const { observationRef: _observationRef, controlEpoch: _controlEpoch, snapshot, ...rest } = result; - if (!snapshot) return rest; + const { observationRef: _observationRef, controlEpoch: _controlEpoch, snapshot, tabs, ...rest } = result; + const listed = { ...rest, tabs: projectTabs(tabs) }; + if (!snapshot) return listed; const elements = snapshot.elements.map(({ semanticId: _semanticId, ...element }, index) => ({ ref: elementRef(snapshotId ?? 0, index), ...element, })); - return { ...rest, snapshot: { ...snapshot, elements } }; + return { ...listed, snapshot: { ...snapshot, elements } }; } /** Removes internal bookkeeping from a Browser result and swaps semantic ids for refs. */ diff --git a/apps/server/src/features/mcode-mcp/toolkits/browser-tool-schemas.ts b/apps/server/src/features/mcode-mcp/toolkits/browser-tool-schemas.ts index 68b06c799..bf6d8e6cf 100644 --- a/apps/server/src/features/mcode-mcp/toolkits/browser-tool-schemas.ts +++ b/apps/server/src/features/mcode-mcp/toolkits/browser-tool-schemas.ts @@ -81,7 +81,11 @@ const inputSchemaByOperation: Record = "Returns a snapshot whose elements each carry a ref such as s3e12. Target browser_act with these refs. Each inspection replaces the previous refs.", "Inspect before the first action and again whenever an action says to. Tool discovery does not prove readiness.", "Request includeScreenshot only for visual evidence or when the user asks. Verify every success claim against current page evidence.", + "tabs lists every Browser tab in this thread, including tabs the user opened, with who holds each. Pass a tabId to read that tab, even while the user holds it; reading never takes control.", + "browser_act targets the current tab, not one read by tabId, and refuses refs from another tab. To act on a tab read by tabId, browser_tabs select it while nobody holds the Browser, then inspect again.", "Use only mcode MCP browser tools; Browser-only tasks need no shell commands or local skills/files.", ].join(" "), act: [ diff --git a/apps/web/src/features/preview/automation/services/__tests__/browserSessionDriver.test.ts b/apps/web/src/features/preview/automation/services/__tests__/browserSessionDriver.test.ts index 52df9e839..68e480496 100644 --- a/apps/web/src/features/preview/automation/services/__tests__/browserSessionDriver.test.ts +++ b/apps/web/src/features/preview/automation/services/__tests__/browserSessionDriver.test.ts @@ -415,7 +415,7 @@ describe("BrowserSessionDriver", () => { const inspect = await driver.execute({ connection: { connectionGeneration: 1, capabilityRevision: 1 }, target: { threadId: "thread", tabId: "tab", targetGeneration: 1 }, - request: { contractVersion: 1, requestId: "inspect", sequence: 1, operation: "inspect", expectedControlEpoch: 0 }, + request: { contractVersion: 1, workspaceId: "workspace", requestId: "inspect", sequence: 1, operation: "inspect", expectedControlEpoch: 0 }, } as unknown as BrowserAutomationHostDispatch, new AbortController().signal); const observationRef = (inspect as { result?: { observationRef?: string } }).result?.observationRef; expect(observationRef).toEqual(expect.any(String)); @@ -536,7 +536,7 @@ describe("BrowserSessionDriver", () => { const inspect = await driver.execute({ connection: { connectionGeneration: 1, capabilityRevision: 1 }, target: { threadId: "thread", tabId: "tab", targetGeneration: 1 }, - request: { contractVersion: 1, requestId: "inspect-issued", sequence: 1, operation: "inspect", expectedControlEpoch: 0 }, + request: { contractVersion: 1, workspaceId: "workspace", requestId: "inspect-issued", sequence: 1, operation: "inspect", expectedControlEpoch: 0 }, } as unknown as BrowserAutomationHostDispatch, new AbortController().signal); expect(inspect).toMatchObject({ ok: true, result: { observationRef: "driver-issued" } }); await expect(driver.execute(actDispatch("driver-issued", [{ operation: "click", target: { cssSelector: "#save" } }]), new AbortController().signal)) @@ -544,6 +544,22 @@ describe("BrowserSessionDriver", () => { expect(execute).toHaveBeenCalledTimes(2); }); + it("refuses an act or evaluate that reuses another tab's observation", async () => { + const execute = vi.fn(async (dispatch: BrowserAutomationHostDispatch) => ( + { ok: true, result: { operation: dispatch.request.operation } } as unknown as BrowserAutomationResponse + )); + const driver = new BrowserSessionDriver({ web: { execute }, electron: { execute }, isElectron: () => true, supportedActOperations: ["click"] }); + const userTab = { ...inspectDispatch("read-user-tab"), target: { threadId: "thread", tabId: "user-tab", targetGeneration: 1, windowId: 1 } } as BrowserAutomationHostDispatch; + const read = await driver.execute(userTab, new AbortController().signal); + const observationRef = (read as { result?: { observationRef?: string } }).result?.observationRef; + + await expect(driver.execute(actDispatch(observationRef!, [{ operation: "click", target: { cssSelector: "#save" } }]), new AbortController().signal)) + .resolves.toMatchObject({ ok: false, error: { code: "STALE_TARGET_GENERATION" } }); + await expect(driver.execute(evaluateDispatch(observationRef!), new AbortController().signal)) + .resolves.toMatchObject({ ok: false, error: { code: "STALE_TARGET_GENERATION" } }); + expect(execute.mock.calls.map(([dispatch]) => dispatch.request.operation)).toEqual(["inspect"]); + }); + it("rejects unsupported steps before the first adapter effect", async () => { const execute = vi.fn().mockResolvedValue({ ok: true, result: { operation: "click" } } as unknown as BrowserAutomationResponse); const driver = new BrowserSessionDriver({ web: { execute }, electron: { execute }, isElectron: () => false, supportedActOperations: ["click"] }); @@ -596,7 +612,7 @@ describe("BrowserSessionDriver", () => { return { ok: true, result: { operation: dispatch.request.operation } } as unknown as BrowserAutomationResponse; }); const driver = new BrowserSessionDriver({ web: { execute }, electron: { execute }, isElectron: () => false, getControlRevision: () => controlRevision, supportedActOperations: ["click", "type"] }); - const inspect = await driver.execute({ connection: { connectionGeneration: 1, capabilityRevision: 1 }, target: { threadId: "thread", tabId: "tab", targetGeneration: 1 }, request: { contractVersion: 1, requestId: "inspect", sequence: 1, operation: "inspect", expectedControlEpoch: 0 } } as unknown as BrowserAutomationHostDispatch, new AbortController().signal); + const inspect = await driver.execute({ connection: { connectionGeneration: 1, capabilityRevision: 1 }, target: { threadId: "thread", tabId: "tab", targetGeneration: 1 }, request: { contractVersion: 1, workspaceId: "workspace", requestId: "inspect", sequence: 1, operation: "inspect", expectedControlEpoch: 0 } } as unknown as BrowserAutomationHostDispatch, new AbortController().signal); const observationRef = (inspect as { result: { observationRef: string } }).result.observationRef; const result = await driver.execute(actDispatch(observationRef, [{ operation: "click", target: { cssSelector: "#save" } }, { operation: "type", text: "secret" }]), new AbortController().signal); expect(result).toMatchObject({ ok: true, result: { outcome: "interrupted", effect: "partial", receipts: [{ index: 0, status: "applied" }, { index: 1, status: "interrupted" }] } }); diff --git a/apps/web/src/features/preview/automation/services/browserSessionDriver.ts b/apps/web/src/features/preview/automation/services/browserSessionDriver.ts index cc0432dfe..bba89db0a 100644 --- a/apps/web/src/features/preview/automation/services/browserSessionDriver.ts +++ b/apps/web/src/features/preview/automation/services/browserSessionDriver.ts @@ -610,7 +610,8 @@ export class BrowserSessionDriver { capabilityRevision: number, ): boolean { const current = this.currentObservationBinding(dispatch, capabilityRevision); - return observation.hostRevision === current.hostRevision && + return this.observedDispatchTarget(dispatch, observation) && + observation.hostRevision === current.hostRevision && observation.documentRevision === current.documentRevision && observation.controlRevision === current.controlRevision && observation.capabilityRevision === current.capabilityRevision && @@ -859,12 +860,21 @@ export class BrowserSessionDriver { capabilityRevision: number, ): boolean { const current = this.currentObservationBinding(dispatch, capabilityRevision); - return observation.hostRevision === current.hostRevision && + return this.observedDispatchTarget(dispatch, observation) && + observation.hostRevision === current.hostRevision && observation.documentRevision === current.documentRevision && observation.controlRevision === current.controlRevision && observation.capabilityRevision === current.capabilityRevision; } + /** + * Revisions count per tab, so two tabs can share them. An inspect can read a + * tab other than the one effects go to, so its refs must not reach another tab. + */ + private observedDispatchTarget(dispatch: BrowserAutomationHostDispatch, observation: ObservationRecord): boolean { + return observation.targetKey === observationTargetKey(dispatch.request.workspaceId, dispatch.target.threadId, dispatch.target.tabId); + } + private evaluatePreEffectInterruption( dispatch: BrowserAutomationHostDispatch, observation: ObservationRecord, diff --git a/packages/contracts/src/models/browser-automation.ts b/packages/contracts/src/models/browser-automation.ts index c3ad4072d..191e2127b 100644 --- a/packages/contracts/src/models/browser-automation.ts +++ b/packages/contracts/src/models/browser-automation.ts @@ -993,6 +993,8 @@ export const BrowserAutomationRequestSchema = lazySchema(() => requestVariant("inspect", z.object({ includeScreenshot: z.boolean().default(false), includeDiagnostics: z.boolean().default(false), + /** Reads this thread tab instead of the session's current one, without selecting it. */ + tabId: idSchema.optional(), }).strict()), requestVariant("status", emptyArgs), requestVariant(