diff --git a/apps/extension/src/session-manager/__tests__/surface-capture-store.test.ts b/apps/extension/src/session-manager/__tests__/surface-capture-store.test.ts new file mode 100644 index 0000000..fdd3364 --- /dev/null +++ b/apps/extension/src/session-manager/__tests__/surface-capture-store.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it } from "vitest"; +import { type SurfaceCaptureInput, SurfaceCaptureStore } from "../surface-capture-store"; + +function input(): SurfaceCaptureInput { + return { + sessionId: "aa11", + tabId: 7, + navigationIdentity: "navigation", + surface: { + ref: "e3", + frameId: "main", + backendNodeId: 99, + observationGeneration: 2, + }, + topViewportRect: { x: 10, y: 20, w: 100, h: 50 }, + imageWidth: 200, + imageHeight: 100, + viewportSignature: "viewport", + frameProjectionSignature: "frame", + }; +} + +describe("SurfaceCaptureStore", () => { + it("creates metadata-only, short-lived captures and consumes them once", () => { + let now = 1_000; + const store = new SurfaceCaptureStore({ + now: () => now, + ttlMs: 500, + createId: () => "sc_test", + }); + const capture = store.create(input()); + + expect(capture).toMatchObject({ + id: "sc_test", + createdAt: 1_000, + expiresAt: 1_500, + consumed: false, + }); + expect(capture).not.toHaveProperty("imageBase64"); + expect(store.consume("sc_test")).toMatchObject({ ok: true }); + expect(store.consume("sc_test")).toEqual({ ok: false, reason: "consumed" }); + + now = 1_600; + expect(store.size()).toBe(0); + }); + + it("rejects expired and unknown captures", () => { + let now = 1_000; + const store = new SurfaceCaptureStore({ + now: () => now, + ttlMs: 10, + createId: () => "sc_expired", + }); + store.create(input()); + now = 1_010; + + expect(store.consume("sc_expired")).toEqual({ ok: false, reason: "expired" }); + expect(store.consume("sc_missing")).toEqual({ ok: false, reason: "not_found" }); + }); + + it("clears every capture on session cleanup", () => { + const store = new SurfaceCaptureStore({ createId: () => "sc_clear" }); + store.create(input()); + store.clear(); + expect(store.size()).toBe(0); + }); +}); diff --git a/apps/extension/src/session-manager/manager.ts b/apps/extension/src/session-manager/manager.ts index 97982e3..d02e4f3 100644 --- a/apps/extension/src/session-manager/manager.ts +++ b/apps/extension/src/session-manager/manager.ts @@ -1,10 +1,12 @@ import { AGENT_WINDOW_HOME, type AgentWindowApi, chromeAgentWindowApi } from "./agent-window"; import { RefStore } from "./ref-store"; +import { SurfaceCaptureStore } from "./surface-capture-store"; export interface SessionContext { sessionId: string; agentWindowId: number; refStore: RefStore; + surfaceCaptures: SurfaceCaptureStore; borrowedTabs: Map; /** * Tabs explicitly claimed by the agent because it created them. This @@ -210,6 +212,7 @@ export class SessionManager { sessionId, agentWindowId: windowId, refStore: new RefStore(), + surfaceCaptures: new SurfaceCaptureStore({ now: this.now }), borrowedTabs: new Map(), // The home tab is the session's first explicit claim. Every other // tab remains free until `tab_create` or `tab_borrow` identifies it diff --git a/apps/extension/src/session-manager/surface-capture-store.ts b/apps/extension/src/session-manager/surface-capture-store.ts new file mode 100644 index 0000000..10985e0 --- /dev/null +++ b/apps/extension/src/session-manager/surface-capture-store.ts @@ -0,0 +1,100 @@ +import type { Rect } from "@browser-skill/vom"; + +export const SURFACE_CAPTURE_TTL_MS = 30_000; + +export interface SurfaceCapture { + id: string; + sessionId: string; + tabId: number; + navigationIdentity: string; + surface: { + ref: string; + frameId?: string; + backendNodeId: number; + observationGeneration: number; + }; + topViewportRect: Rect; + imageWidth: number; + imageHeight: number; + viewportSignature: string; + frameProjectionSignature: string; + createdAt: number; + expiresAt: number; + consumed: boolean; +} + +export type SurfaceCaptureInput = Omit< + SurfaceCapture, + "id" | "createdAt" | "expiresAt" | "consumed" +>; + +export type SurfaceCaptureConsumeResult = + | { ok: true; capture: SurfaceCapture } + | { ok: false; reason: "not_found" | "expired" | "consumed" }; + +export interface SurfaceCaptureStoreOptions { + now?: () => number; + ttlMs?: number; + createId?: () => string; +} + +function randomCaptureId(): string { + const bytes = crypto.getRandomValues(new Uint8Array(12)); + return `sc_${Array.from(bytes, (byte) => byte.toString(16).padStart(2, "0")).join("")}`; +} + +/** Per-session, metadata-only store for short-lived screenshot coordinate transactions. */ +export class SurfaceCaptureStore { + private readonly captures = new Map(); + private readonly now: () => number; + private readonly ttlMs: number; + private readonly createId: () => string; + + constructor(options: SurfaceCaptureStoreOptions = {}) { + this.now = options.now ?? Date.now; + this.ttlMs = options.ttlMs ?? SURFACE_CAPTURE_TTL_MS; + this.createId = options.createId ?? randomCaptureId; + } + + create(input: SurfaceCaptureInput): SurfaceCapture { + this.purgeExpired(); + const createdAt = this.now(); + const capture: SurfaceCapture = { + ...input, + id: this.createId(), + createdAt, + expiresAt: createdAt + this.ttlMs, + consumed: false, + }; + this.captures.set(capture.id, capture); + return capture; + } + + consume(id: string): SurfaceCaptureConsumeResult { + const capture = this.captures.get(id); + if (!capture) return { ok: false, reason: "not_found" }; + if (capture.expiresAt <= this.now()) { + this.captures.delete(id); + return { ok: false, reason: "expired" }; + } + if (capture.consumed) return { ok: false, reason: "consumed" }; + capture.consumed = true; + return { ok: true, capture }; + } + + clear(): void { + this.captures.clear(); + } + + size(): number { + this.purgeExpired(); + return this.captures.size; + } + + private purgeExpired(): void { + const now = this.now(); + for (const [id, capture] of this.captures) { + if (capture.expiresAt <= now) this.captures.delete(id); + } + } +} diff --git a/apps/extension/src/tools/__tests__/observation.test.ts b/apps/extension/src/tools/__tests__/observation.test.ts index 2850ae3..562dc7a 100644 --- a/apps/extension/src/tools/__tests__/observation.test.ts +++ b/apps/extension/src/tools/__tests__/observation.test.ts @@ -392,6 +392,7 @@ describe("handleScreenshot", () => { }; expect(clip.clip).toMatchObject({ x: 25, y: 30, width: 50, height: 40 }); expect(res).toMatchObject({ width: 100, height: 80 }); + expect(res.capture?.id).toMatch(/^sc_/); }); it("routes OOPIF surface geometry through its child session and captures top coordinates", async () => { diff --git a/apps/extension/src/tools/__tests__/session.test.ts b/apps/extension/src/tools/__tests__/session.test.ts index e87e01a..dedaeaa 100644 --- a/apps/extension/src/tools/__tests__/session.test.ts +++ b/apps/extension/src/tools/__tests__/session.test.ts @@ -278,12 +278,23 @@ describe("handleSessionStop with auto-return", () => { expect(aw.remove).not.toHaveBeenCalled(); }); - it("clears the RefStore before window teardown", async () => { + it("clears refs and Surface captures before window teardown", async () => { const aw = fakeAgentWindow([100]); const sm = new SessionManager({ agentWindow: aw }); const ctx = await sm.start("aa11"); // Insert a fake ref so we can verify clear() ran. ctx.refStore.set("e1", 123, { tabId: 7 }); + ctx.surfaceCaptures.create({ + sessionId: "aa11", + tabId: 7, + navigationIdentity: "navigation", + surface: { ref: "e1", backendNodeId: 123, observationGeneration: 0 }, + topViewportRect: { x: 0, y: 0, w: 100, h: 50 }, + imageWidth: 100, + imageHeight: 50, + viewportSignature: "viewport", + frameProjectionSignature: "frame", + }); const state: FakeState = { tabs: new Map(), windowsClosed: new Set(), @@ -292,6 +303,7 @@ describe("handleSessionStop with auto-return", () => { const { tabs, windows } = makeApis(state); await handleSessionStop(sm, { session_id: "aa11" }, { tabManagement: { tabs, windows } }); expect(ctx.refStore.isEmpty()).toBe(true); + expect(ctx.surfaceCaptures.size()).toBe(0); }); it.each([ diff --git a/apps/extension/src/tools/__tests__/snapshot-ref.test.ts b/apps/extension/src/tools/__tests__/snapshot-ref.test.ts index 60c0d95..fb697da 100644 --- a/apps/extension/src/tools/__tests__/snapshot-ref.test.ts +++ b/apps/extension/src/tools/__tests__/snapshot-ref.test.ts @@ -26,12 +26,14 @@ describe("lookupSnapshotRef", () => { refKey: "e3", kind: "dom", capabilities: ["interact", "screenshot"], + generation: 0, }); expect(lookupSnapshotRef(ctx, "e3", 4)).toEqual({ backendNodeId: 1234, refKey: "e3", kind: "dom", capabilities: ["interact", "screenshot"], + generation: 0, }); }); @@ -68,6 +70,7 @@ describe("resolveSnapshotRef", () => { cdpSessionId: "child-session", kind: "dom" as const, capabilities: ["interact", "screenshot"] as const, + generation: 0, }; expect(lookupSnapshotRef(ctx, "@e3", 4)).toEqual(expected); expect(resolveSnapshotRef(ctx, "@e3", 4)).toEqual(expected); @@ -115,6 +118,7 @@ describe("resolveSnapshotRef", () => { refKey: "e3", kind: "dom", capabilities: ["interact", "screenshot"], + generation: 0, }); }); diff --git a/apps/extension/src/tools/__tests__/surface-coordinate.test.ts b/apps/extension/src/tools/__tests__/surface-coordinate.test.ts new file mode 100644 index 0000000..609ea8f --- /dev/null +++ b/apps/extension/src/tools/__tests__/surface-coordinate.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from "vitest"; +import { + mapImagePointToViewport, + pointInRegion, + sameRect, + surfaceVisibleRect, +} from "../surface-coordinate"; + +describe("surface coordinate mapping", () => { + it("maps capture image pixels through the actual screenshot dimensions", () => { + expect(mapImagePointToViewport({ x: 100, y: 50, w: 400, h: 200 }, 800, 400, 200, 100)).toEqual({ + x: 200, + y: 100, + }); + }); + + it("rejects invalid image coordinates and dimensions", () => { + const rect = { x: 0, y: 0, w: 100, h: 50 }; + expect(mapImagePointToViewport(rect, 100, 50, -1, 0)).toBeNull(); + expect(mapImagePointToViewport(rect, 100, 50, 100, 0)).toBeNull(); + expect(mapImagePointToViewport(rect, 100, 50, 0, 50)).toBeNull(); + expect(mapImagePointToViewport(rect, 100, 50, Number.NaN, 0)).toBeNull(); + expect(mapImagePointToViewport(rect, 100, 50, Number.POSITIVE_INFINITY, 0)).toBeNull(); + expect(mapImagePointToViewport(rect, 0, 50, 0, 0)).toBeNull(); + }); + + it("intersects live and observed visible regions and compares them strictly", () => { + const clipped = surfaceVisibleRect( + { x: 0, y: 0, width: 200, height: 100 }, + { x: 25, y: 30, w: 50, h: 40 }, + ); + expect(clipped).toEqual({ x: 25, y: 30, w: 50, h: 40 }); + expect(sameRect(clipped as NonNullable, { x: 25, y: 30, w: 50, h: 40 })).toBe( + true, + ); + expect(sameRect(clipped as NonNullable, { x: 26, y: 30, w: 50, h: 40 })).toBe( + false, + ); + }); + + it("checks the projected visible region rather than only its bounding box", () => { + const triangle = [ + [ + { x: 0, y: 0 }, + { x: 100, y: 0 }, + { x: 0, y: 100 }, + ], + ]; + expect(pointInRegion({ x: 20, y: 20 }, triangle)).toBe(true); + expect(pointInRegion({ x: 90, y: 90 }, triangle)).toBe(false); + }); +}); diff --git a/apps/extension/src/tools/__tests__/surface-point-action.test.ts b/apps/extension/src/tools/__tests__/surface-point-action.test.ts new file mode 100644 index 0000000..ce5bc80 --- /dev/null +++ b/apps/extension/src/tools/__tests__/surface-point-action.test.ts @@ -0,0 +1,313 @@ +import { describe, expect, it, vi } from "vitest"; +import { SessionManager } from "@/session-manager/manager"; +import type { CdpRunner } from "../shared"; +import { captureSurfaceEnvironment, handleSurfacePointClick } from "../surface-point-action"; + +function fakeAgentWindow() { + return { + create: vi.fn(async () => 100), + remove: vi.fn(async () => {}), + ensureActiveTab: vi.fn(async () => {}), + }; +} + +function fakeBrowser() { + let loaderId = "loader-1"; + let pageY = 0; + let quad = [10, 20, 110, 20, 110, 60, 10, 60]; + let dispatchFails = false; + const sent: Array<{ method: string; params?: object }> = []; + const cdp: CdpRunner = { + send: vi.fn(async (_tabId: number, method: string, params?: object) => { + sent.push({ method, params }); + if (method === "Page.getFrameTree") { + return { + frameTree: { frame: { id: "main", loaderId, url: "https://fixture.test/" } }, + } as never; + } + if (method === "Page.getLayoutMetrics") { + return { + cssLayoutViewport: { clientWidth: 800, clientHeight: 600, pageX: 0, pageY }, + cssVisualViewport: { + clientWidth: 800, + clientHeight: 600, + pageX: 0, + pageY, + scale: 1, + }, + } as never; + } + if (method === "DOM.getContentQuads") return { quads: [quad] } as never; + if (method === "Input.dispatchMouseEvent") { + if (dispatchFails) throw new Error("input failed"); + return {} as never; + } + throw new Error(`unexpected CDP call ${method}`); + }) as CdpRunner["send"], + trackSessionTab: vi.fn(), + }; + const tabsApi = { + get: vi.fn( + async (tabId: number) => ({ id: tabId, windowId: 100, active: true }) as chrome.tabs.Tab, + ), + query: vi.fn(async () => [{ id: 4, windowId: 100, active: true } as chrome.tabs.Tab]), + }; + return { + cdp, + tabsApi, + sent, + setLoaderId: (value: string) => { + loaderId = value; + }, + setPageY: (value: number) => { + pageY = value; + }, + setQuad: (value: number[]) => { + quad = value; + }, + failDispatch: () => { + dispatchFails = true; + }, + }; +} + +async function setupCapture() { + const manager = new SessionManager({ agentWindow: fakeAgentWindow() }); + const context = await manager.start("aa11"); + context.refStore.set("e3", 99, { + tabId: 4, + kind: "surface", + visibleRect: { x: 10, y: 20, w: 100, h: 40 }, + }); + const browser = fakeBrowser(); + const environment = await captureSurfaceEnvironment(browser.cdp, 4, undefined); + if ("code" in environment) throw new Error(environment.message); + const entry = context.refStore.resolveEntry("e3"); + if (!entry) throw new Error("missing ref"); + const capture = context.surfaceCaptures.create({ + sessionId: "aa11", + tabId: 4, + navigationIdentity: environment.navigationIdentity, + surface: { + ref: "e3", + backendNodeId: 99, + observationGeneration: entry.generation, + }, + topViewportRect: { x: 10, y: 20, w: 100, h: 40 }, + imageWidth: 200, + imageHeight: 80, + viewportSignature: environment.viewportSignature, + frameProjectionSignature: environment.frameProjectionSignature, + }); + return { manager, context, browser, capture }; +} + +function pointParams(captureId: string) { + return { + session_id: "aa11", + ref: "@e3", + capture_id: captureId, + image_x: 50, + image_y: 20, + }; +} + +describe("Surface screenshot-bound point click", () => { + it("maps one fresh capture coordinate and dispatches exactly one trusted click", async () => { + const { manager, browser, capture } = await setupCapture(); + const result = await handleSurfacePointClick(manager, pointParams(capture.id), browser); + + if ("code" in result) throw new Error(JSON.stringify(result)); + expect(result).toMatchObject({ + used_ref: "e3", + capture_id: capture.id, + image_x: 50, + image_y: 20, + x: 35, + y: 30, + }); + expect(browser.sent.filter((call) => call.method === "Input.dispatchMouseEvent")).toHaveLength( + 3, + ); + + const repeated = await handleSurfacePointClick(manager, pointParams(capture.id), browser); + expect(repeated).toMatchObject({ + code: "permission_denied", + data: { reason: "surface_capture_consumed" }, + }); + }); + + it("rejects a new observation generation before sending input", async () => { + const { manager, context, browser, capture } = await setupCapture(); + context.refStore.replace([ + [ + "e3", + { + backendNodeId: 99, + tabId: 4, + kind: "surface" as const, + visibleRect: { x: 10, y: 20, w: 100, h: 40 }, + }, + ], + ]); + + const result = await handleSurfacePointClick(manager, pointParams(capture.id), browser); + expect(result).toMatchObject({ + code: "permission_denied", + data: { reason: "surface_capture_stale" }, + }); + expect(browser.sent.some((call) => call.method === "Input.dispatchMouseEvent")).toBe(false); + }); + + it("rejects navigation, scroll, and visible geometry changes", async () => { + for (const mutate of [ + (browser: ReturnType) => browser.setLoaderId("loader-2"), + (browser: ReturnType) => browser.setPageY(20), + (browser: ReturnType) => + browser.setQuad([11, 20, 111, 20, 111, 60, 11, 60]), + ]) { + const { manager, browser, capture } = await setupCapture(); + mutate(browser); + const result = await handleSurfacePointClick(manager, pointParams(capture.id), browser); + expect(result).toMatchObject({ + code: "permission_denied", + data: { reason: "surface_capture_stale" }, + }); + expect(browser.sent.some((call) => call.method === "Input.dispatchMouseEvent")).toBe(false); + } + }); + + it("consumes the capture when coordinates or input dispatch fail", async () => { + const invalid = await setupCapture(); + const outside = await handleSurfacePointClick( + invalid.manager, + { ...pointParams(invalid.capture.id), image_x: 200 }, + invalid.browser, + ); + expect(outside).toMatchObject({ + code: "invalid_params", + data: { reason: "surface_coordinate_invalid" }, + }); + expect( + await handleSurfacePointClick( + invalid.manager, + pointParams(invalid.capture.id), + invalid.browser, + ), + ).toMatchObject({ data: { reason: "surface_capture_consumed" } }); + + const failed = await setupCapture(); + failed.browser.failDispatch(); + expect( + await handleSurfacePointClick(failed.manager, pointParams(failed.capture.id), failed.browser), + ).toMatchObject({ code: "cdp_failed" }); + expect( + await handleSurfacePointClick(failed.manager, pointParams(failed.capture.id), failed.browser), + ).toMatchObject({ data: { reason: "surface_capture_consumed" } }); + }); + + it("maps image coordinates through an OOPIF frame projection", async () => { + const manager = new SessionManager({ agentWindow: fakeAgentWindow() }); + const context = await manager.start("aa11"); + context.refStore.set("e3", 99, { + tabId: 4, + frameId: "child", + cdpSessionId: "child-session", + kind: "surface", + visibleRect: { x: 120, y: 110, w: 100, h: 40 }, + }); + const graph = { + rootFrameId: "main", + frames: [ + { frameId: "main", target: { tabId: 4 } }, + { + frameId: "child", + parentFrameId: "main", + ownerBackendNodeId: 77, + target: { tabId: 4, sessionId: "child-session" }, + }, + ], + }; + const sent: Array<{ method: string; params?: object }> = []; + const cdp: CdpRunner = { + send: vi.fn(async (_tabId: number, method: string, params?: object) => { + sent.push({ method, params }); + if (method === "Page.getFrameTree") { + return { + frameTree: { + frame: { id: "main", loaderId: "loader-1", url: "https://fixture.test/" }, + }, + } as never; + } + if (method === "Page.getLayoutMetrics") { + return { + cssLayoutViewport: { clientWidth: 800, clientHeight: 600, pageX: 0, pageY: 0 }, + cssVisualViewport: { + clientWidth: 800, + clientHeight: 600, + pageX: 0, + pageY: 0, + scale: 1, + }, + } as never; + } + if (method === "DOM.getBoxModel") { + return { model: { content: [100, 100, 300, 100, 300, 200, 100, 200] } } as never; + } + if (method === "Input.dispatchMouseEvent") return {} as never; + throw new Error(`unexpected root CDP call ${method}`); + }) as CdpRunner["send"], + sendToTarget: vi.fn(async (_target, method) => { + if (method === "DOM.getContentQuads") { + return { quads: [[20, 10, 120, 10, 120, 50, 20, 50]] } as never; + } + if (method === "Page.getLayoutMetrics") { + return { cssLayoutViewport: { clientWidth: 200, clientHeight: 100 } } as never; + } + throw new Error(`unexpected child CDP call ${method}`); + }) as CdpRunner["sendToTarget"], + getFrameGraph: vi.fn(async () => graph), + trackSessionTab: vi.fn(), + }; + const tabsApi = { + get: vi.fn(async () => ({ id: 4, windowId: 100, active: true }) as chrome.tabs.Tab), + query: vi.fn(async () => [{ id: 4, windowId: 100, active: true } as chrome.tabs.Tab]), + }; + const environment = await captureSurfaceEnvironment(cdp, 4, "child"); + if ("code" in environment) throw new Error(environment.message); + const entry = context.refStore.resolveEntry("e3"); + if (!entry) throw new Error("missing ref"); + const capture = context.surfaceCaptures.create({ + sessionId: "aa11", + tabId: 4, + navigationIdentity: environment.navigationIdentity, + surface: { + ref: "e3", + frameId: "child", + backendNodeId: 99, + observationGeneration: entry.generation, + }, + topViewportRect: { x: 120, y: 110, w: 100, h: 40 }, + imageWidth: 200, + imageHeight: 80, + viewportSignature: environment.viewportSignature, + frameProjectionSignature: environment.frameProjectionSignature, + }); + + const result = await handleSurfacePointClick( + manager, + { + session_id: "aa11", + ref: "@e3", + capture_id: capture.id, + image_x: 100, + image_y: 40, + }, + { cdp, tabsApi }, + ); + + if ("code" in result) throw new Error(JSON.stringify(result)); + expect(result).toMatchObject({ x: 170, y: 130 }); + expect(sent.filter((call) => call.method === "Input.dispatchMouseEvent")).toHaveLength(3); + }); +}); diff --git a/apps/extension/src/tools/interaction.ts b/apps/extension/src/tools/interaction.ts index abce158..45a02a3 100644 --- a/apps/extension/src/tools/interaction.ts +++ b/apps/extension/src/tools/interaction.ts @@ -44,6 +44,7 @@ import { resolveTargetTab, } from "./shared"; import { resolveSnapshotRef } from "./snapshot-ref"; +import { handleSurfacePointClick } from "./surface-point-action"; export interface InteractionDeps { cdp: CdpRunner; @@ -247,6 +248,11 @@ export async function handleClick( params: ClickParams, deps: InteractionDeps = getDefaultDeps(), ): Promise { + const hasSurfacePointParams = + params.capture_id !== undefined || params.image_x !== undefined || params.image_y !== undefined; + if (hasSurfacePointParams) { + return handleSurfacePointClick(manager, params, deps); + } const ctxOrErr = lookupSession(manager, params, "click"); if (isRpcError(ctxOrErr)) return ctxOrErr; const ctx = ctxOrErr; diff --git a/apps/extension/src/tools/observation.ts b/apps/extension/src/tools/observation.ts index e37531d..a864454 100644 --- a/apps/extension/src/tools/observation.ts +++ b/apps/extension/src/tools/observation.ts @@ -254,7 +254,7 @@ async function captureElementScreenshot( if (!dims) { return { code: "cdp_failed", message: "Page.captureScreenshot returned invalid PNG data" }; } - return { image_base64, width: dims.width, height: dims.height }; + return { image_base64, width: dims.width, height: dims.height, topViewportRect }; } catch (err) { return { code: "cdp_failed", diff --git a/apps/extension/src/tools/session.ts b/apps/extension/src/tools/session.ts index b6fe38f..9e802df 100644 --- a/apps/extension/src/tools/session.ts +++ b/apps/extension/src/tools/session.ts @@ -271,6 +271,7 @@ export async function handleSessionStop( // Step 2: clear the per-session RefStore (review M6/M7 parity). ctx.refStore.clear(); + ctx.surfaceCaptures.clear(); clearRecordingForSession(params.session_id); diff --git a/apps/extension/src/tools/snapshot-ref.ts b/apps/extension/src/tools/snapshot-ref.ts index 19e3a25..a1c5a10 100644 --- a/apps/extension/src/tools/snapshot-ref.ts +++ b/apps/extension/src/tools/snapshot-ref.ts @@ -16,6 +16,7 @@ export interface SnapshotRefLookup { visibleRect?: Rect; kind: RefTargetKind; capabilities: RefCapability[]; + generation: number; } function refEntryForTab(ctx: SessionContext, refKey: string, tabId: number) { @@ -44,6 +45,7 @@ export function lookupSnapshotRef( ...(entry.visibleRect ? { visibleRect: entry.visibleRect } : {}), kind: entry.kind, capabilities: entry.capabilities, + generation: entry.generation, }; } diff --git a/apps/extension/src/tools/surface-coordinate.ts b/apps/extension/src/tools/surface-coordinate.ts new file mode 100644 index 0000000..ced1a90 --- /dev/null +++ b/apps/extension/src/tools/surface-coordinate.ts @@ -0,0 +1,88 @@ +import type { Rect } from "@browser-skill/vom"; +import type { Point, Region, ViewportRect } from "./geometry"; + +const RECT_EPSILON = 0.01; + +export function surfaceVisibleRect( + liveRect: ViewportRect, + observedRect: Rect | undefined, +): Rect | null { + const observed = observedRect ?? { + x: liveRect.x, + y: liveRect.y, + w: liveRect.width, + h: liveRect.height, + }; + const x = Math.max(liveRect.x, observed.x); + const y = Math.max(liveRect.y, observed.y); + const right = Math.min(liveRect.x + liveRect.width, observed.x + observed.w); + const bottom = Math.min(liveRect.y + liveRect.height, observed.y + observed.h); + return right > x && bottom > y ? { x, y, w: right - x, h: bottom - y } : null; +} + +export function sameRect(a: Rect, b: Rect): boolean { + return ( + Math.abs(a.x - b.x) <= RECT_EPSILON && + Math.abs(a.y - b.y) <= RECT_EPSILON && + Math.abs(a.w - b.w) <= RECT_EPSILON && + Math.abs(a.h - b.h) <= RECT_EPSILON + ); +} + +export function mapImagePointToViewport( + rect: Rect, + imageWidth: number, + imageHeight: number, + imageX: number, + imageY: number, +): Point | null { + if ( + !Number.isFinite(imageX) || + !Number.isFinite(imageY) || + !Number.isSafeInteger(imageWidth) || + !Number.isSafeInteger(imageHeight) || + imageWidth <= 0 || + imageHeight <= 0 || + imageX < 0 || + imageY < 0 || + imageX >= imageWidth || + imageY >= imageHeight || + !Number.isFinite(rect.x) || + !Number.isFinite(rect.y) || + !Number.isFinite(rect.w) || + !Number.isFinite(rect.h) || + rect.w <= 0 || + rect.h <= 0 + ) { + return null; + } + return { + x: rect.x + (imageX / imageWidth) * rect.w, + y: rect.y + (imageY / imageHeight) * rect.h, + }; +} + +function pointInPolygon(point: Point, polygon: Point[]): boolean { + for (let index = 0; index < polygon.length; index += 1) { + const a = polygon[index]; + const b = polygon[(index + 1) % polygon.length]; + const cross = (point.x - a.x) * (b.y - a.y) - (point.y - a.y) * (b.x - a.x); + const withinX = point.x >= Math.min(a.x, b.x) - 1e-9 && point.x <= Math.max(a.x, b.x) + 1e-9; + const withinY = point.y >= Math.min(a.y, b.y) - 1e-9 && point.y <= Math.max(a.y, b.y) + 1e-9; + if (Math.abs(cross) <= 1e-9 && withinX && withinY) return true; + } + let inside = false; + for (let index = 0, previous = polygon.length - 1; index < polygon.length; previous = index++) { + const a = polygon[index]; + const b = polygon[previous]; + const intersects = + a.y > point.y !== b.y > point.y && + point.x < ((b.x - a.x) * (point.y - a.y)) / (b.y - a.y) + a.x; + if (intersects) inside = !inside; + } + return inside; +} + +export function pointInRegion(point: Point, region: Region): boolean { + return region.some((polygon) => polygon.length >= 3 && pointInPolygon(point, polygon)); +} diff --git a/apps/extension/src/tools/surface-point-action.ts b/apps/extension/src/tools/surface-point-action.ts new file mode 100644 index 0000000..e61b661 --- /dev/null +++ b/apps/extension/src/tools/surface-point-action.ts @@ -0,0 +1,320 @@ +import type { CdpFrameGraph } from "@/browser-driver/frame-graph"; +import type { SessionManager } from "@/session-manager/manager"; +import type { ClickParams, ClickResult, RpcError } from "@/transport/types"; +import { attachDialogs, markDialogCursor } from "./dialogs"; +import { rpcError } from "./errors"; +import { resolveNodeGeometry } from "./frame-geometry"; +import { + type CdpRunner, + type ChromeTabsApi, + enforceAgentWindow, + isRpcError, + lookupSession, + normaliseRef, + resolveTargetTab, +} from "./shared"; +import { resolveSnapshotRef } from "./snapshot-ref"; +import { + mapImagePointToViewport, + pointInRegion, + sameRect, + surfaceVisibleRect, +} from "./surface-coordinate"; + +export interface SurfaceCaptureEnvironment { + navigationIdentity: string; + viewportSignature: string; + frameProjectionSignature: string; +} + +export interface SurfacePointActionDeps { + cdp: CdpRunner; + tabsApi: ChromeTabsApi; + signal?: AbortSignal; + bypassOverlay?: (tabId: number, enabled: boolean) => Promise; +} + +function stale(message: string): RpcError { + return rpcError("permission_denied", "surface_capture_stale", message); +} + +function finiteObject(value: unknown): unknown { + if (Array.isArray(value)) return value.map(finiteObject); + if (typeof value !== "object" || value === null) return value; + return Object.fromEntries( + Object.entries(value) + .filter( + ([, entry]) => entry !== undefined && (typeof entry !== "number" || Number.isFinite(entry)), + ) + .sort(([a], [b]) => a.localeCompare(b)) + .map(([key, entry]) => [key, finiteObject(entry)]), + ); +} + +function framePathSignature(graph: CdpFrameGraph, frameId: string | undefined): string | null { + if (!frameId) return "top"; + const frames = new Map(graph.frames.map((frame) => [frame.frameId, frame])); + const path: Array<{ + frameId: string; + parentFrameId?: string; + ownerBackendNodeId?: number; + targetSessionId?: string; + }> = []; + const seen = new Set(); + let current = frames.get(frameId); + while (current) { + if (seen.has(current.frameId)) return null; + seen.add(current.frameId); + path.push({ + frameId: current.frameId, + ...(current.parentFrameId ? { parentFrameId: current.parentFrameId } : {}), + ...(current.ownerBackendNodeId !== undefined + ? { ownerBackendNodeId: current.ownerBackendNodeId } + : {}), + ...(current.target.sessionId ? { targetSessionId: current.target.sessionId } : {}), + }); + if (!current.parentFrameId) return JSON.stringify(path); + const parent = frames.get(current.parentFrameId); + if (!parent) return null; + current = parent; + } + return null; +} + +export async function captureSurfaceEnvironment( + cdp: CdpRunner, + tabId: number, + frameId: string | undefined, +): Promise { + try { + const [tree, metrics, graph] = await Promise.all([ + cdp.send<{ + frameTree?: { frame?: { id?: string; loaderId?: string; url?: string } }; + }>(tabId, "Page.getFrameTree", {}), + cdp.send>(tabId, "Page.getLayoutMetrics", {}), + frameId && cdp.getFrameGraph ? cdp.getFrameGraph(tabId) : Promise.resolve(null), + ]); + const root = tree.frameTree?.frame; + if (!root?.id || !root.loaderId) { + return { code: "cdp_failed", message: "could not identify the current navigation" }; + } + const frameProjectionSignature = frameId + ? graph + ? framePathSignature(graph, frameId) + : null + : "top"; + if (!frameProjectionSignature) { + return { code: "cdp_failed", message: `could not identify frame projection for ${frameId}` }; + } + return { + navigationIdentity: JSON.stringify({ id: root.id, loaderId: root.loaderId, url: root.url }), + viewportSignature: JSON.stringify( + finiteObject({ + cssLayoutViewport: metrics.cssLayoutViewport, + cssVisualViewport: metrics.cssVisualViewport, + }), + ), + frameProjectionSignature, + }; + } catch (error) { + return { code: "cdp_failed", message: error instanceof Error ? error.message : String(error) }; + } +} + +function validatePointParams(params: ClickParams): RpcError | null { + if (!params.capture_id || typeof params.capture_id !== "string") { + return { code: "invalid_params", message: "surface point click requires capture_id" }; + } + if (typeof params.image_x !== "number" || !Number.isFinite(params.image_x)) { + return { code: "invalid_params", message: "image_x must be a finite number" }; + } + if (typeof params.image_y !== "number" || !Number.isFinite(params.image_y)) { + return { code: "invalid_params", message: "image_y must be a finite number" }; + } + if (!params.ref || params.selector) { + return { + code: "invalid_params", + message: "surface point click requires one Surface ref and does not accept a selector", + }; + } + if ((params.button ?? "left") !== "left" || (params.click_count ?? 1) !== 1) { + return { + code: "invalid_params", + message: "surface point click currently supports one left click only", + }; + } + if (params.modifiers && params.modifiers.length > 0) { + return { code: "invalid_params", message: "surface point click does not accept modifiers" }; + } + return null; +} + +function captureConsumeError(reason: "not_found" | "expired" | "consumed"): RpcError { + const code = reason === "not_found" ? "not_found" : "permission_denied"; + return rpcError(code, `surface_capture_${reason}`, `surface capture is ${reason}`); +} + +function aborted(signal: AbortSignal | undefined): RpcError | null { + return signal?.aborted ? { code: "cancelled", message: "surface point click aborted" } : null; +} + +export async function handleSurfacePointClick( + manager: SessionManager, + params: ClickParams, + deps: SurfacePointActionDeps, +): Promise { + const invalid = validatePointParams(params); + if (invalid) return invalid; + const ctxOrError = lookupSession(manager, params, "click"); + if (isRpcError(ctxOrError)) return ctxOrError; + const ctx = ctxOrError; + const earlyAbort = aborted(deps.signal); + if (earlyAbort) return earlyAbort; + const target = await resolveTargetTab(manager, ctx, params.tab_id, deps.tabsApi); + if (isRpcError(target)) return target; + const denied = enforceAgentWindow(ctx, target, "click"); + if (denied) return denied; + + const consumed = ctx.surfaceCaptures.consume(params.capture_id as string); + if (!consumed.ok) return captureConsumeError(consumed.reason); + const capture = consumed.capture; + const postConsumeAbort = aborted(deps.signal); + if (postConsumeAbort) return postConsumeAbort; + + if (capture.sessionId !== ctx.sessionId || capture.tabId !== target.tabId) { + return stale("surface capture belongs to a different session or tab"); + } + if (normaliseRef(params.ref as string) !== capture.surface.ref) { + return stale("surface ref does not match the capture"); + } + const node = resolveSnapshotRef(ctx, params.ref as string, target.tabId, "screenshot"); + if (isRpcError(node)) return stale(node.message); + if ( + node.kind !== "surface" || + node.backendNodeId !== capture.surface.backendNodeId || + node.frameId !== capture.surface.frameId || + node.generation !== capture.surface.observationGeneration + ) { + return stale("surface observation generation or node identity changed"); + } + + deps.cdp.trackSessionTab?.(ctx.sessionId, target.tabId); + try { + await deps.cdp.ensureAttachedToUrl?.(target.tabId, target.url); + } catch (error) { + return { code: "cdp_failed", message: error instanceof Error ? error.message : String(error) }; + } + const environment = await captureSurfaceEnvironment(deps.cdp, target.tabId, node.frameId); + if (isRpcError(environment)) return environment; + if ( + environment.navigationIdentity !== capture.navigationIdentity || + environment.viewportSignature !== capture.viewportSignature || + environment.frameProjectionSignature !== capture.frameProjectionSignature + ) { + return stale("navigation, viewport, zoom, scroll, or frame projection changed"); + } + + const geometry = await resolveNodeGeometry( + deps.cdp, + target.tabId, + { + target: { + tabId: target.tabId, + ...(node.cdpSessionId ? { sessionId: node.cdpSessionId } : {}), + }, + backendNodeId: node.backendNodeId, + ...(node.frameId ? { frameId: node.frameId } : {}), + }, + { scrollIntoView: false }, + ); + if (isRpcError(geometry)) return stale(geometry.message); + const currentRect = surfaceVisibleRect(geometry.topBounds, node.visibleRect); + if (!currentRect || !sameRect(currentRect, capture.topViewportRect)) { + return stale("surface visible region changed"); + } + const point = mapImagePointToViewport( + capture.topViewportRect, + capture.imageWidth, + capture.imageHeight, + params.image_x as number, + params.image_y as number, + ); + if (!point || !pointInRegion(point, geometry.topVisibleRegions)) { + return rpcError( + "invalid_params", + "surface_coordinate_invalid", + "image coordinate is outside the captured Surface", + ); + } + + const dialogCursor = markDialogCursor(deps.cdp, target.tabId); + let bypassEnabled = false; + let pressed = false; + const release = () => + deps.cdp.send(target.tabId, "Input.dispatchMouseEvent", { + type: "mouseReleased", + x: point.x, + y: point.y, + button: "left", + clickCount: 1, + modifiers: 0, + }); + try { + if (deps.bypassOverlay) { + await deps.bypassOverlay(target.tabId, true); + bypassEnabled = true; + } + await deps.cdp.send(target.tabId, "Input.dispatchMouseEvent", { + type: "mouseMoved", + x: point.x, + y: point.y, + modifiers: 0, + }); + const beforePressAbort = aborted(deps.signal); + if (beforePressAbort) return beforePressAbort; + await deps.cdp.send(target.tabId, "Input.dispatchMouseEvent", { + type: "mousePressed", + x: point.x, + y: point.y, + button: "left", + clickCount: 1, + modifiers: 0, + }); + pressed = true; + const afterPressAbort = aborted(deps.signal); + if (afterPressAbort) { + await release(); + pressed = false; + return afterPressAbort; + } + await release(); + pressed = false; + } catch (error) { + if (pressed) { + try { + await release(); + } catch (releaseError) { + console.debug("[bsk surface-point] best-effort mouse release failed", releaseError); + } + } + return { code: "cdp_failed", message: error instanceof Error ? error.message : String(error) }; + } finally { + if (bypassEnabled && deps.bypassOverlay) { + try { + await deps.bypassOverlay(target.tabId, false); + } catch (error) { + console.debug("[bsk surface-point] overlay bypass disable failed", error); + } + } + } + + return attachDialogs(deps.cdp, target.tabId, dialogCursor, { + tab_id: target.tabId, + used_ref: capture.surface.ref, + x: point.x, + y: point.y, + capture_id: capture.id, + image_x: params.image_x, + image_y: params.image_y, + }); +} diff --git a/apps/extension/src/transport/types.ts b/apps/extension/src/transport/types.ts index 713dba1..5bdbe80 100644 --- a/apps/extension/src/transport/types.ts +++ b/apps/extension/src/transport/types.ts @@ -49,6 +49,11 @@ export type RpcErrorReason = | "download_capture_failed" | "transfer_outcome_unknown" | "transfer_timeout" + | "surface_capture_not_found" + | "surface_capture_expired" + | "surface_capture_consumed" + | "surface_capture_stale" + | "surface_coordinate_invalid" | "cleanup_failed"; export type TransferEffectState = "none" | "committed" | "unknown"; diff --git a/crates/bsk-cli/skill/SKILL.md b/crates/bsk-cli/skill/SKILL.md index c6ba9e6..09a8b9e 100644 --- a/crates/bsk-cli/skill/SKILL.md +++ b/crates/bsk-cli/skill/SKILL.md @@ -76,12 +76,24 @@ is cheaper and more precise. When `bsk observe` renders `@eN surface ... [visual-only; requires=image-understanding; ...]`, the ref identifies rendered canvas content that is not represented by the text observation, not an interactable DOM control. -Keep using any reliable DOM/AX text and controls that appear alongside it. If you can actually -inspect image output, use `bsk screenshot --ref @eN --session ` to obtain the visible crop. If -you lack multimodal or image-reading capability, do not take a screenshot and pretend to know its -contents, and never guess coordinates; tell the user that they need to switch to a model with -image-understanding capability. Do not pass a visual surface ref to click, fill, hover, or select; -those commands intentionally reject screenshot-only refs. +Keep using any reliable DOM/AX text and controls that appear alongside it. If you lack multimodal or +image-reading capability, do not take a screenshot and pretend to know its contents, and never guess +coordinates; tell the user that they need to switch to a model with image-understanding capability. + +If you can actually inspect image output, use `bsk screenshot --ref @eN --session ` to obtain +the visible crop and its short-lived, single-use Surface capture id. Only when the task requires a +point action and the image provides a clear target, bind that exact screenshot to the click command: + +```bash +bsk click @eN --capture --image-x --image-y --session +``` + +Coordinates are pixels in the returned capture image, not viewport CSS coordinates. Re-observing, +navigating, scrolling, resizing, zooming, changing Frame projection, expiry, or reusing the capture +makes the action fail; take a new Surface screenshot instead of adjusting or retrying coordinates. +A point click does not reveal Canvas semantics and must not be followed by an assumed fill/press +workflow. Without all three capture arguments, visual Surface refs remain screenshot-only and +click/fill/hover/select reject them. Escalate page reading only as needed: @@ -135,6 +147,7 @@ Required flags that are easy to get wrong: bsk fill --value bsk select --value bsk screenshot --out bsk emulate --device bsk upload --file bsk download --out +bsk click --capture --image-x --image-y ``` `select` matches an option's `value` attribute, not its visible label. Device preset ids are diff --git a/crates/bsk-cli/src/cli/interaction.rs b/crates/bsk-cli/src/cli/interaction.rs index 075f9ac..e58b220 100644 --- a/crates/bsk-cli/src/cli/interaction.rs +++ b/crates/bsk-cli/src/cli/interaction.rs @@ -136,6 +136,18 @@ pub struct ClickArgs { #[arg(long, default_value = "30s", value_parser = parse_timeout_ms)] pub timeout: u32, + + /// Capture id returned by `bsk screenshot --ref `. + #[arg(long)] + pub capture: Option, + + /// X coordinate in capture-image pixels. Requires `--capture` and `--image-y`. + #[arg(long = "image-x", allow_hyphen_values = true)] + pub image_x: Option, + + /// Y coordinate in capture-image pixels. Requires `--capture` and `--image-x`. + #[arg(long = "image-y", allow_hyphen_values = true)] + pub image_y: Option, } pub fn dispatch_click(args: ClickArgs, format: Format) -> Result<(), CliError> { @@ -147,6 +159,14 @@ pub fn dispatch_click(args: ClickArgs, format: Format) -> Result<(), CliError> { )?; let modifiers = parse_modifiers(&args.modifiers) .map_err(|e| CliError::Local(anyhow::anyhow!("--modifiers: {e}")))?; + let point_arg_count = usize::from(args.capture.is_some()) + + usize::from(args.image_x.is_some()) + + usize::from(args.image_y.is_some()); + if point_arg_count != 0 && point_arg_count != 3 { + return Err(CliError::Local(anyhow::anyhow!( + "--capture, --image-x, and --image-y must be given together" + ))); + } let params = ClickParams { session_id: args.session.clone(), ref_, @@ -160,6 +180,9 @@ pub fn dispatch_click(args: ClickArgs, format: Format) -> Result<(), CliError> { Some(modifiers) }, timeout_ms: Some(args.timeout), + capture_id: args.capture.clone(), + image_x: args.image_x, + image_y: args.image_y, }; let reply: ClickResult = call( info.sock_path, @@ -183,6 +206,14 @@ pub fn dispatch_click(args: ClickArgs, format: Format) -> Result<(), CliError> { "click ok tab={} target={target} at=({}, {})", reply.tab_id, reply.x, reply.y ); + if let Some(capture_id) = &reply.capture_id { + println!( + "capture={} image=({}, {}) coordinate_space=capture-image-pixel", + capture_id, + reply.image_x.unwrap_or_default(), + reply.image_y.unwrap_or_default() + ); + } print_dialog_summaries(&reply.dialogs); } } diff --git a/crates/bsk-cli/src/cli/render_error.rs b/crates/bsk-cli/src/cli/render_error.rs index 7f7aee5..c58b33d 100644 --- a/crates/bsk-cli/src/cli/render_error.rs +++ b/crates/bsk-cli/src/cli/render_error.rs @@ -65,6 +65,11 @@ pub mod reason { pub const DOWNLOAD_CAPTURE_FAILED: &str = "download_capture_failed"; pub const TRANSFER_OUTCOME_UNKNOWN: &str = "transfer_outcome_unknown"; pub const TRANSFER_TIMEOUT: &str = "transfer_timeout"; + pub const SURFACE_CAPTURE_NOT_FOUND: &str = "surface_capture_not_found"; + pub const SURFACE_CAPTURE_EXPIRED: &str = "surface_capture_expired"; + pub const SURFACE_CAPTURE_CONSUMED: &str = "surface_capture_consumed"; + pub const SURFACE_CAPTURE_STALE: &str = "surface_capture_stale"; + pub const SURFACE_COORDINATE_INVALID: &str = "surface_coordinate_invalid"; pub const SESSION_BUSY: &str = crate::rpc_reason::SESSION_BUSY; pub const RECORD_START_PAGE_UNREACHABLE: &str = "record_start_page_unreachable"; } @@ -303,7 +308,7 @@ pub fn info_for_error(code: ErrorCode, data: Option<&serde_json::Value>) -> Rend (ErrorCode::PermissionDenied, reason::REF_CAPABILITY_DENIED) => RenderInfo { summary: "snapshot ref does not support this operation", hint: Some( - "visual surface refs are screenshot-only; inspect `bsk screenshot --ref --session ` with image-understanding capability", + "visual Surface refs require image understanding; inspect `bsk screenshot --ref --session `, then bind its capture id and image coordinates to `bsk click`", ), exit_code: base.exit_code, }, @@ -349,6 +354,41 @@ pub fn info_for_error(code: ErrorCode, data: Option<&serde_json::Value>) -> Rend ), exit_code: base.exit_code, }, + (ErrorCode::NotFound, reason::SURFACE_CAPTURE_NOT_FOUND) => RenderInfo { + summary: "surface capture transaction was not found", + hint: Some( + "take a fresh surface screenshot and use its capture id with coordinates from that image", + ), + exit_code: base.exit_code, + }, + (ErrorCode::PermissionDenied, reason::SURFACE_CAPTURE_EXPIRED) => RenderInfo { + summary: "surface capture transaction has expired", + hint: Some( + "take a fresh surface screenshot, then click promptly with that capture id and image coordinates", + ), + exit_code: base.exit_code, + }, + (ErrorCode::PermissionDenied, reason::SURFACE_CAPTURE_CONSUMED) => RenderInfo { + summary: "surface capture transaction was already used", + hint: Some( + "each capture id is single-use; take a fresh surface screenshot before another point action", + ), + exit_code: base.exit_code, + }, + (ErrorCode::PermissionDenied, reason::SURFACE_CAPTURE_STALE) => RenderInfo { + summary: "page state changed since the surface screenshot", + hint: Some( + "take a fresh surface screenshot and choose coordinates from the new image before retrying", + ), + exit_code: base.exit_code, + }, + (ErrorCode::InvalidParams, reason::SURFACE_COORDINATE_INVALID) => RenderInfo { + summary: "surface image coordinate is invalid", + hint: Some( + "use finite pixel coordinates inside the captured image; its top-left corner is (0, 0)", + ), + exit_code: base.exit_code, + }, ( ErrorCode::CdpFailed | ErrorCode::Timeout | ErrorCode::InvalidParams, reason::RECORD_START_PAGE_UNREACHABLE, @@ -578,7 +618,7 @@ mod tests { let info = info_for_error(ErrorCode::PermissionDenied, Some(&data)); assert_eq!(info.summary, "snapshot ref does not support this operation"); assert!( - info.hint.unwrap().contains("screenshot-only"), + info.hint.unwrap().contains("capture id"), "expected visual-surface-specific hint" ); assert_eq!(info.exit_code, 1); @@ -684,6 +724,25 @@ mod tests { assert!(info.summary.contains("timed out after browser dispatch")); } + #[test] + fn surface_capture_stale_requests_a_fresh_screenshot() { + let data = serde_json::json!({ "reason": reason::SURFACE_CAPTURE_STALE }); + let info = info_for_error(ErrorCode::PermissionDenied, Some(&data)); + assert_eq!( + info.summary, + "page state changed since the surface screenshot" + ); + assert!(info.hint.unwrap().contains("fresh surface screenshot")); + } + + #[test] + fn surface_coordinate_invalid_explains_image_pixel_bounds() { + let data = serde_json::json!({ "reason": reason::SURFACE_COORDINATE_INVALID }); + let info = info_for_error(ErrorCode::InvalidParams, Some(&data)); + assert_eq!(info.summary, "surface image coordinate is invalid"); + assert!(info.hint.unwrap().contains("top-left corner is (0, 0)")); + } + #[test] fn ref_not_found_overrides_not_found_copy() { let data = serde_json::json!({ "reason": reason::REF_NOT_FOUND }); diff --git a/crates/bsk-cli/src/cli/screenshot.rs b/crates/bsk-cli/src/cli/screenshot.rs index fe981e0..eb1a50a 100644 --- a/crates/bsk-cli/src/cli/screenshot.rs +++ b/crates/bsk-cli/src/cli/screenshot.rs @@ -59,7 +59,7 @@ fn run(sock: PathBuf, args: ScreenshotArgs, format: Format) -> Result<(), CliErr .map_err(CliError::Local)?; match format { Format::Json => { - let json = serde_json::json!({ + let mut json = serde_json::json!({ "tab_id": reply.tab_id, "width": reply.width, "height": reply.height, @@ -67,6 +67,10 @@ fn run(sock: PathBuf, args: ScreenshotArgs, format: Format) -> Result<(), CliErr "path": out_path.to_string_lossy(), "byte_size": bytes.len(), }); + if let Some(capture) = &reply.capture { + json["capture"] = serde_json::to_value(capture) + .map_err(|e| CliError::Local(anyhow::anyhow!(e)))?; + } println!( "{}", serde_json::to_string_pretty(&json) @@ -75,6 +79,12 @@ fn run(sock: PathBuf, args: ScreenshotArgs, format: Format) -> Result<(), CliErr } Format::Human => { println!("{}", out_path.display()); + if let Some(capture) = &reply.capture { + println!( + "capture={} surface={} coordinate_space={} expires_at={}", + capture.id, capture.surface_ref, capture.coordinate_space, capture.expires_at + ); + } print_dialog_summaries(&reply.dialogs); } } diff --git a/crates/bsk-cli/tests/cli_parse.rs b/crates/bsk-cli/tests/cli_parse.rs index 7a52f65..bcbbeca 100644 --- a/crates/bsk-cli/tests/cli_parse.rs +++ b/crates/bsk-cli/tests/cli_parse.rs @@ -237,6 +237,29 @@ fn parses_click_count_alias() { assert_eq!(args.click_count, 2); } +#[test] +fn parses_surface_capture_image_coordinates() { + let cli = parse(&[ + "bsk", + "click", + "@e1", + "--session", + "s1", + "--capture", + "sc_test", + "--image-x", + "160.5", + "--image-y", + "-1", + ]); + let Command::Click(args) = cli.command else { + panic!("expected click command"); + }; + assert_eq!(args.capture.as_deref(), Some("sc_test")); + assert_eq!(args.image_x, Some(160.5)); + assert_eq!(args.image_y, Some(-1.0)); +} + #[test] fn parses_hover_with_settle() { let cli = parse(&[ diff --git a/crates/bsk-cli/tests/tools_ipc.rs b/crates/bsk-cli/tests/tools_ipc.rs index 4d8d2bc..b4c4ce9 100644 --- a/crates/bsk-cli/tests/tools_ipc.rs +++ b/crates/bsk-cli/tests/tools_ipc.rs @@ -238,6 +238,7 @@ async fn screenshot_returns_image_base64_with_dimensions() { height: 600, format: "png".into(), tab_id: 7, + capture: None, dialogs: vec![], }) .unwrap(), @@ -286,6 +287,7 @@ async fn screenshot_forwards_ref_to_extension() { height: 60, format: "png".into(), tab_id: 7, + capture: None, dialogs: vec![], }) .unwrap(), diff --git a/crates/bsk-cli/tests/tools_m7_ipc.rs b/crates/bsk-cli/tests/tools_m7_ipc.rs index ee74b5e..9346c46 100644 --- a/crates/bsk-cli/tests/tools_m7_ipc.rs +++ b/crates/bsk-cli/tests/tools_m7_ipc.rs @@ -354,6 +354,9 @@ async fn click_round_trips_ref_and_modifiers() { used_selector: None, x: 12.5, y: 34.0, + capture_id: None, + image_x: None, + image_y: None, dialogs: vec![], }) .unwrap(), @@ -373,6 +376,9 @@ async fn click_round_trips_ref_and_modifiers() { click_count: Some(1), modifiers: Some(vec![KeyModifier::Ctrl, KeyModifier::Shift]), timeout_ms: Some(5_000), + capture_id: None, + image_x: None, + image_y: None, }, ) .await @@ -610,6 +616,9 @@ async fn m7_tools_propagate_extension_errors() { click_count: None, modifiers: None, timeout_ms: Some(5_000), + capture_id: None, + image_x: None, + image_y: None, }, ) .await diff --git a/crates/bsk-protocol/schema/tool_click_params.json b/crates/bsk-protocol/schema/tool_click_params.json index aaa8f28..12646aa 100644 --- a/crates/bsk-protocol/schema/tool_click_params.json +++ b/crates/bsk-protocol/schema/tool_click_params.json @@ -16,6 +16,13 @@ } ] }, + "capture_id": { + "description": "Short-lived capture id returned by a visual Surface screenshot.", + "type": [ + "string", + "null" + ] + }, "click_count": { "description": "Number of consecutive mouse presses (double-click = 2).", "type": [ @@ -25,6 +32,22 @@ "format": "uint32", "minimum": 1.0 }, + "image_x": { + "description": "X coordinate in the capture image's pixel coordinate space.", + "type": [ + "number", + "null" + ], + "format": "double" + }, + "image_y": { + "description": "Y coordinate in the capture image's pixel coordinate space.", + "type": [ + "number", + "null" + ], + "format": "double" + }, "modifiers": { "type": [ "array", diff --git a/crates/bsk-protocol/schema/tool_click_result.json b/crates/bsk-protocol/schema/tool_click_result.json index 3ee6f75..10059ba 100644 --- a/crates/bsk-protocol/schema/tool_click_result.json +++ b/crates/bsk-protocol/schema/tool_click_result.json @@ -8,6 +8,12 @@ "y" ], "properties": { + "capture_id": { + "type": [ + "string", + "null" + ] + }, "dialogs": { "description": "Native JS dialogs observed and auto-handled during this call.", "type": "array", @@ -15,6 +21,20 @@ "$ref": "#/definitions/JavaScriptDialogInfo" } }, + "image_x": { + "type": [ + "number", + "null" + ], + "format": "double" + }, + "image_y": { + "type": [ + "number", + "null" + ], + "format": "double" + }, "tab_id": { "type": "integer", "format": "int64" diff --git a/crates/bsk-protocol/schema/tool_screenshot_result.json b/crates/bsk-protocol/schema/tool_screenshot_result.json index e3c089f..63a88b7 100644 --- a/crates/bsk-protocol/schema/tool_screenshot_result.json +++ b/crates/bsk-protocol/schema/tool_screenshot_result.json @@ -8,6 +8,16 @@ "tab_id" ], "properties": { + "capture": { + "anyOf": [ + { + "$ref": "#/definitions/SurfaceCaptureInfo" + }, + { + "type": "null" + } + ] + }, "dialogs": { "type": "array", "items": { @@ -109,6 +119,31 @@ "prompt", "beforeunload" ] + }, + "SurfaceCaptureInfo": { + "type": "object", + "required": [ + "coordinate_space", + "expires_at", + "id", + "surface_ref" + ], + "properties": { + "coordinate_space": { + "type": "string" + }, + "expires_at": { + "type": "integer", + "format": "uint64", + "minimum": 0.0 + }, + "id": { + "type": "string" + }, + "surface_ref": { + "type": "string" + } + } } } } diff --git a/crates/bsk-protocol/src/tools/interaction.rs b/crates/bsk-protocol/src/tools/interaction.rs index 2d2e808..11a5a7d 100644 --- a/crates/bsk-protocol/src/tools/interaction.rs +++ b/crates/bsk-protocol/src/tools/interaction.rs @@ -67,6 +67,15 @@ pub struct ClickParams { #[serde(default, skip_serializing_if = "Option::is_none")] #[schemars(range(min = 1))] pub timeout_ms: Option, + /// Short-lived capture id returned by a visual Surface screenshot. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub capture_id: Option, + /// X coordinate in the capture image's pixel coordinate space. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub image_x: Option, + /// Y coordinate in the capture image's pixel coordinate space. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub image_y: Option, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] @@ -80,6 +89,12 @@ pub struct ClickResult { /// agents can correlate with a follow-up `tool.screenshot`. pub x: f64, pub y: f64, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub capture_id: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub image_x: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub image_y: Option, /// Native JS dialogs observed and auto-handled during this call. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub dialogs: Vec, @@ -288,6 +303,9 @@ mod tests { click_count: Some(1), modifiers: Some(vec![KeyModifier::Ctrl]), timeout_ms: Some(5_000), + capture_id: Some("sc_test".into()), + image_x: Some(160.0), + image_y: Some(90.0), }; let v = serde_json::to_value(&p).unwrap(); assert_eq!(v.get("ref").and_then(|v| v.as_str()), Some("@e3")); diff --git a/crates/bsk-protocol/src/tools/observation.rs b/crates/bsk-protocol/src/tools/observation.rs index 9cbfc8d..be8d00a 100644 --- a/crates/bsk-protocol/src/tools/observation.rs +++ b/crates/bsk-protocol/src/tools/observation.rs @@ -233,10 +233,20 @@ pub struct ScreenshotResult { /// Always `"png"` in v0.1; reserved for future JPEG / WebP support. pub format: String, pub tab_id: i64, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub capture: Option, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub dialogs: Vec, } +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +pub struct SurfaceCaptureInfo { + pub id: String, + pub surface_ref: String, + pub coordinate_space: String, + pub expires_at: u64, +} + #[cfg(test)] mod tests { use super::*; @@ -318,6 +328,12 @@ mod tests { height: 600, format: "png".into(), tab_id: 7, + capture: Some(SurfaceCaptureInfo { + id: "sc_test".into(), + surface_ref: "@e3".into(), + coordinate_space: "capture-image-pixel".into(), + expires_at: 1_700_000_030_000, + }), dialogs: vec![], }; let v = serde_json::to_value(&r).unwrap(); diff --git a/packages/dsh-plugin-browserskill/src/tools.ts b/packages/dsh-plugin-browserskill/src/tools.ts index be99780..420736e 100644 --- a/packages/dsh-plugin-browserskill/src/tools.ts +++ b/packages/dsh-plugin-browserskill/src/tools.ts @@ -563,7 +563,8 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): name: "interact.click", description: "Click an element in the session's active tab. Target is a snapshot ref (@e3) from the " + - "last browser_inspect observation, or a CSS selector.", + "last browser_inspect observation, or a CSS selector. A visual Surface requires the " + + "captureId and image coordinates returned by browser_inspect action=screenshot.", parameters: { target: { type: "string", @@ -580,6 +581,18 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): type: "integer", description: "Number of consecutive presses (double-click = 2).", }, + captureId: { + type: "string", + description: "Short-lived Surface capture id returned by browser_screenshot.", + }, + imageX: { + type: "number", + description: "X coordinate in capture-image pixels; requires captureId and imageY.", + }, + imageY: { + type: "number", + description: "Y coordinate in capture-image pixels; requires captureId and imageX.", + }, }, output: { schema: { @@ -590,6 +603,7 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): tabId: { type: "integer", required: true }, x: { type: "number", required: true }, y: { type: "number", required: true }, + captureId: { type: "string" }, }, }, render: (_args, value) => [ @@ -601,17 +615,41 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): }, async execute(args, exec) { if (args.target.trim().length === 0) throw new Error("target must be a non-empty string"); + const pointArgCount = + Number(args.captureId !== undefined) + + Number(args.imageX !== undefined) + + Number(args.imageY !== undefined); + if (pointArgCount !== 0 && pointArgCount !== 3) { + throw new Error("captureId, imageX, and imageY must be given together"); + } const sessionId = registry.resolve(args.session, "browser_interact(action=click)"); const cmdArgs = ["click", "--session", sessionId]; if (args.button !== undefined) cmdArgs.push("--button", args.button); if (args.clickCount !== undefined) cmdArgs.push("--click-count", String(args.clickCount)); + if (args.captureId !== undefined) { + cmdArgs.push( + "--capture", + args.captureId, + "--image-x", + String(args.imageX), + "--image-y", + String(args.imageY), + ); + } cmdArgs.push(args.target); const reply = (await runBsk(deps, exec, cmdArgs, "click", sessionId)) as { tab_id: number; x: number; y: number; + capture_id?: string; + }; + return { + session: sessionId, + tabId: reply.tab_id, + x: reply.x, + y: reply.y, + ...(reply.capture_id !== undefined ? { captureId: reply.capture_id } : {}), }; - return { session: sessionId, tabId: reply.tab_id, x: reply.x, y: reply.y }; }, presentCall: (args) => ({ card: "terminal", @@ -780,6 +818,8 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): width: { type: "integer", required: true }, height: { type: "integer", required: true }, byteSize: { type: "integer", required: true }, + captureId: { type: "string" }, + captureExpiresAt: { type: "integer" }, image: { type: "object", additionalProperties: false, @@ -803,7 +843,11 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): value.image !== undefined ? `[session ${value.session}] screenshot of tab ${value.tabId} (${value.width}x${value.height}px)` : `[session ${value.session}] screenshot saved to ${value.path} (${value.width}x${value.height}px, ${value.byteSize} bytes) — this deployment cannot inline images; read the file to view it`; - const blocks: ContentBlock[] = [{ type: "text", text }]; + const captureText = + value.captureId !== undefined + ? ` Surface capture ${value.captureId} uses capture-image-pixel coordinates and expires at ${value.captureExpiresAt}.` + : ""; + const blocks: ContentBlock[] = [{ type: "text", text: text + captureText }]; if (value.image !== undefined) { blocks.push({ type: "image", @@ -842,6 +886,7 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): height: number; path: string; byte_size: number; + capture?: { id: string; expires_at: number }; }; writtenPath = reply.path; const data = await readFile(reply.path); @@ -855,6 +900,9 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar): width: reply.width, height: reply.height, byteSize: reply.byte_size, + ...(reply.capture !== undefined + ? { captureId: reply.capture.id, captureExpiresAt: reply.capture.expires_at } + : {}), ...(ref !== undefined ? { image: { diff --git a/packages/dsh-plugin-browserskill/tests/tools.test.ts b/packages/dsh-plugin-browserskill/tests/tools.test.ts index 7faf3fa..e023ad6 100644 --- a/packages/dsh-plugin-browserskill/tests/tools.test.ts +++ b/packages/dsh-plugin-browserskill/tests/tools.test.ts @@ -499,6 +499,41 @@ describe("interaction tools", () => { ]); }); + it("interact.click forwards a complete Surface capture coordinate transaction", async () => { + const { tools, calls } = setup({ + ...responses, + click: { + tab_id: 7, + used_ref: "e1", + x: 110, + y: 70, + capture_id: "sc_test", + image_x: 200, + image_y: 100, + }, + }); + await startSession(tools); + const click = tools.get("interact.click"); + const value = await click?.execute( + { target: "@e1", captureId: "sc_test", imageX: 200, imageY: 100 }, + makeExec(), + ); + + expect(value).toMatchObject({ captureId: "sc_test", x: 110, y: 70 }); + expect(calls[1].args).toEqual([ + "click", + "--session", + "s1", + "--capture", + "sc_test", + "--image-x", + "200", + "--image-y", + "100", + "@e1", + ]); + }); + it("interact.fill passes value and --no-clear", async () => { const { tools, calls } = setup(responses); await startSession(tools); @@ -923,6 +958,32 @@ describe("inspect.screenshot", () => { expect(rendered?.every((block) => block.type === "text")).toBe(true); }); + it("returns Surface capture metadata alongside the screenshot", async () => { + const path = pngFile(); + const { tools } = setup({ + "session start": START_REPLY("s1"), + screenshot: { + tab_id: 7, + width: 800, + height: 600, + format: "png", + path, + byte_size: 8, + capture: { id: "sc_test", expires_at: 1700000030000 }, + }, + }); + await startSession(tools); + const screenshot = tools.get("browser_screenshot"); + const value = await screenshot?.execute({ ref: "@e1" }, makeExec()); + + expect(value).toMatchObject({ + captureId: "sc_test", + captureExpiresAt: 1700000030000, + }); + const rendered = screenshot?.output.render({ session: "s1" }, value as never); + expect(rendered?.[0]).toMatchObject({ type: "text", text: expect.stringContaining("sc_test") }); + }); + it("commits the image through the attachment store on an image-capable route", async () => { const path = pngFile(); const services = { diff --git a/skill/SKILL.md b/skill/SKILL.md index c6ba9e6..09a8b9e 100644 --- a/skill/SKILL.md +++ b/skill/SKILL.md @@ -76,12 +76,24 @@ is cheaper and more precise. When `bsk observe` renders `@eN surface ... [visual-only; requires=image-understanding; ...]`, the ref identifies rendered canvas content that is not represented by the text observation, not an interactable DOM control. -Keep using any reliable DOM/AX text and controls that appear alongside it. If you can actually -inspect image output, use `bsk screenshot --ref @eN --session ` to obtain the visible crop. If -you lack multimodal or image-reading capability, do not take a screenshot and pretend to know its -contents, and never guess coordinates; tell the user that they need to switch to a model with -image-understanding capability. Do not pass a visual surface ref to click, fill, hover, or select; -those commands intentionally reject screenshot-only refs. +Keep using any reliable DOM/AX text and controls that appear alongside it. If you lack multimodal or +image-reading capability, do not take a screenshot and pretend to know its contents, and never guess +coordinates; tell the user that they need to switch to a model with image-understanding capability. + +If you can actually inspect image output, use `bsk screenshot --ref @eN --session ` to obtain +the visible crop and its short-lived, single-use Surface capture id. Only when the task requires a +point action and the image provides a clear target, bind that exact screenshot to the click command: + +```bash +bsk click @eN --capture --image-x --image-y --session +``` + +Coordinates are pixels in the returned capture image, not viewport CSS coordinates. Re-observing, +navigating, scrolling, resizing, zooming, changing Frame projection, expiry, or reusing the capture +makes the action fail; take a new Surface screenshot instead of adjusting or retrying coordinates. +A point click does not reveal Canvas semantics and must not be followed by an assumed fill/press +workflow. Without all three capture arguments, visual Surface refs remain screenshot-only and +click/fill/hover/select reject them. Escalate page reading only as needed: @@ -135,6 +147,7 @@ Required flags that are easy to get wrong: bsk fill --value bsk select --value bsk screenshot --out bsk emulate --device bsk upload --file bsk download --out +bsk click --capture --image-x --image-y ``` `select` matches an option's `value` attribute, not its visible label. Device preset ids are