From a28b9d55e7b5d9d367f0444e2b18717501651d0d Mon Sep 17 00:00:00 2001 From: Ljy-0827 Date: Tue, 8 Sep 2026 20:04:00 +0800 Subject: [PATCH] fix(vom): surface incomplete iframe geometry in observations --- .../tools/__tests__/geometry-context.test.ts | 14 +++-- .../src/tools/__tests__/observation.test.ts | 51 ++++++++++++++- .../src/tools/geometry/coordinate-types.ts | 18 ++++++ .../src/tools/geometry/frame-context.ts | 41 +++++++----- apps/extension/src/tools/observation.ts | 11 +++- .../src/tools/vom/__tests__/capture.test.ts | 13 ++++ .../tools/vom/__tests__/frame-capture.test.ts | 62 ++++++++++++++++++ apps/extension/src/tools/vom/capture.ts | 63 ++++++++++++++----- apps/extension/src/tools/vom/frame-capture.ts | 3 + 9 files changed, 240 insertions(+), 36 deletions(-) diff --git a/apps/extension/src/tools/__tests__/geometry-context.test.ts b/apps/extension/src/tools/__tests__/geometry-context.test.ts index 5094495..addd227 100644 --- a/apps/extension/src/tools/__tests__/geometry-context.test.ts +++ b/apps/extension/src/tools/__tests__/geometry-context.test.ts @@ -60,15 +60,16 @@ describe("measurement geometry context", () => { width: 1000, height: 1000, }); + if (projection.status !== "available") throw new Error("expected available projection"); const input = snapshotViewportRect([17, 23, 120, 40], source, { x: 0, y: 0 }); - expect(projectSnapshotRect(input!, projection!)).toEqual({ + expect(projectSnapshotRect(input!, projection.projection)).toEqual({ x: 73.75, y: 641.25, width: 150, height: 50, }); const scrolled = snapshotViewportRect([17, 23, 120, 40], source, { x: 0, y: 10 }); - expect(projectSnapshotRect(scrolled!, projection!)?.y).toBe(628.75); + expect(projectSnapshotRect(scrolled!, projection.projection)?.y).toBe(628.75); await context.snapshotProjection(source, 10, [], { width: 1000, height: 1000 }); expect(cdp.calls.mock.calls.filter(([, method]) => method === "DOM.resolveNode")).toHaveLength( 1, @@ -118,7 +119,11 @@ describe("measurement geometry context", () => { width: 100, height: 100, }), - ).toBeNull(); + ).toEqual({ + status: "unavailable", + source: { target, frameId: "child" }, + ownerBackendNodeId: 10, + }); }); it("keeps ancestor clips in target coordinates instead of applying nested offsets twice", async () => { @@ -129,12 +134,13 @@ describe("measurement geometry context", () => { [rectPolygon({ x: 60, y: 620, w: 100, h: 100 })], { width: 1000, height: 1000 }, ); + if (projection.status !== "available") throw new Error("expected available projection"); const input = snapshotViewportRect( [0, 0, 300, 200], { target, frameId: "nested" }, { x: 0, y: 0 }, ); - expect(projectSnapshotRect(input!, projection!)).toEqual({ + expect(projectSnapshotRect(input!, projection.projection)).toEqual({ x: 60, y: 620, width: 100, diff --git a/apps/extension/src/tools/__tests__/observation.test.ts b/apps/extension/src/tools/__tests__/observation.test.ts index 798efe0..8c24651 100644 --- a/apps/extension/src/tools/__tests__/observation.test.ts +++ b/apps/extension/src/tools/__tests__/observation.test.ts @@ -3147,7 +3147,9 @@ describe("handleSnapshot", () => { cssLayoutViewport: { clientWidth: 1000, clientHeight: 800, pageX: 0, pageY: 0 }, }; - function makeFrameAwareDeps() { + function makeFrameAwareDeps( + ownerGeometry: "unavailable" | "available" | "clipped" = "unavailable", + ) { const strings = ["html", "body", "iframe", "button", "title", "Remote", "static", "auto"]; const i = (value: string) => strings.indexOf(value); const styles = [i("static"), i("auto"), i("auto")]; @@ -3227,6 +3229,16 @@ describe("handleSnapshot", () => { ]; const send = vi.fn(async (_tabId: number, method: string) => { if (method === "Page.getLayoutMetrics") return VP_METRICS; + if (ownerGeometry !== "unavailable") { + if (method === "DOM.getBoxModel") { + const x = ownerGeometry === "clipped" ? 10000 : 100; + return { model: { content: [x, 100, x + 400, 100, x + 400, 400, x, 400] } }; + } + if (method === "DOM.resolveNode") return { object: { objectId: "owner" } }; + if (method === "Runtime.callFunctionOn") + return { result: { value: { width: 400, height: 300 } } }; + if (method === "Runtime.releaseObject") return {}; + } if (method === "DOMSnapshot.enable" || method === "Accessibility.enable") return {}; if (method === "DOMSnapshot.captureSnapshot") return snapshot; if (method === "Accessibility.getFullAXTree") return { nodes: mainAx }; @@ -3467,6 +3479,43 @@ describe("handleSnapshot", () => { expect(result.text).toContain("•••"); }); + it.each([ + undefined, + 100, + 0, + ])("reports unavailable iframe geometry within the observation budget %s", async (maxTokens) => { + const result = await captureVomObservation(makeFrameAwareDeps().cdp, 4, "https://example.com", { + maxTokens, + }); + expect(result.text.match(/@warning/g)).toHaveLength(1); + expect(result.text).toContain("iframe geometry incomplete"); + expect(result.text.startsWith("@vom 1\n")).toBe(true); + if (maxTokens === undefined) { + expect(result.truncated).toBe(false); + expect(result.text).toContain("Frame action"); + expect(result.matchNodes.find((node) => node.backendNodeId === 22)).toMatchObject({ + rect: null, + localRect: { x: 20, y: 30, w: 120, h: 40 }, + }); + } + if (maxTokens === 100) expect(Math.ceil(result.text.length / 4)).toBeLessThanOrEqual(maxTokens); + }); + + it.each([ + "available", + "clipped", + ] as const)("does not report successful %s geometry as an observation failure", async (ownerGeometry) => { + const result = await captureVomObservation( + makeFrameAwareDeps(ownerGeometry).cdp, + 4, + "https://example.com", + ); + expect(result.text).not.toContain("@warning"); + const node = result.matchNodes.find((item) => item.backendNodeId === 22); + if (ownerGeometry === "clipped") expect(node?.rect).toBeNull(); + else expect(node?.rect).toEqual({ x: 120, y: 130, w: 120, h: 40 }); + }); + it("keeps frames and rendered refs on the same frame identity", async () => { const deps = makeFrameAwareDeps(); diff --git a/apps/extension/src/tools/geometry/coordinate-types.ts b/apps/extension/src/tools/geometry/coordinate-types.ts index 09a2cec..289ee11 100644 --- a/apps/extension/src/tools/geometry/coordinate-types.ts +++ b/apps/extension/src/tools/geometry/coordinate-types.ts @@ -18,6 +18,24 @@ export interface SnapshotProjection { geometry: GeometryProjection; } +/** A failed owner read is different from a successfully projected, clipped-out rect. */ +export interface UnavailableFrameProjection { + status: "unavailable"; + source: CoordinateOwner; + ownerBackendNodeId: number; +} + +export type SnapshotProjectionResult = + | { status: "available"; projection: SnapshotProjection } + | UnavailableFrameProjection; + +/** Descendants inherit the failed boundary without attempting another owner read. */ +export type FrameProjectionIssue = + | UnavailableFrameProjection + | { status: "blocked"; source: CoordinateOwner; cause: UnavailableFrameProjection }; + +export type FrameProjectionState = SnapshotProjectionResult | FrameProjectionIssue; + /** DOMSnapshot bounds are document-relative CSS pixels, independent of raster DPR. */ export function snapshotViewportRect( bounds: number[], diff --git a/apps/extension/src/tools/geometry/frame-context.ts b/apps/extension/src/tools/geometry/frame-context.ts index 3a81699..be9d54b 100644 --- a/apps/extension/src/tools/geometry/frame-context.ts +++ b/apps/extension/src/tools/geometry/frame-context.ts @@ -13,7 +13,7 @@ import { type Size, } from "../geometry"; import { type CdpRunner, sendToCdpTarget } from "../shared"; -import type { CoordinateOwner, CssViewport, SnapshotProjection } from "./coordinate-types"; +import type { CoordinateOwner, CssViewport, SnapshotProjectionResult } from "./coordinate-types"; export interface LayoutMetrics { cssVisualViewport?: { zoom?: number }; @@ -149,30 +149,41 @@ export class GeometryContext { ownerBackendNodeId: number, ancestorClips: Polygon[], viewport: Size, - ): Promise { + ): Promise { const key = `${cdpTargetKey(source.target)}:${ownerBackendNodeId}`; let promise = this.snapshotOwners.get(key); if (!promise) { promise = this.snapshotOwner(source.target, ownerBackendNodeId); this.snapshotOwners.set(key, promise); } - const owner = await promise; + let owner: { quad: Quad; size: Size } | null; + try { + owner = await promise; + } catch (error) { + if (error && typeof error === "object" && "name" in error && error.name === "AbortError") + throw error; + console.debug("[bsk geometry] frame owner unavailable", error); + owner = null; + } return owner ? { - source, - geometry: { - sourceClips: [], - edges: [ - { - sourceViewport: owner.size, - destinationQuad: owner.quad, - destinationClips: ancestorClips, - }, - ], - topViewport: viewport, + status: "available", + projection: { + source, + geometry: { + sourceClips: [], + edges: [ + { + sourceViewport: owner.size, + destinationQuad: owner.quad, + destinationClips: ancestorClips, + }, + ], + topViewport: viewport, + }, }, } - : null; + : { status: "unavailable", source, ownerBackendNodeId }; } private async snapshotOwner( diff --git a/apps/extension/src/tools/observation.ts b/apps/extension/src/tools/observation.ts index e8726a0..4dca1bd 100644 --- a/apps/extension/src/tools/observation.ts +++ b/apps/extension/src/tools/observation.ts @@ -1133,12 +1133,21 @@ export async function captureVomObservation( captured, hoverProbes.surfaceProbes, ); + // Reserve space using the renderer's character-based token estimate. Like VOM + // headers, this integrity notice remains visible even under a tiny token budget. + const geometryNotice = captured.frameGeometryIssues?.length + ? "@warning iframe geometry incomplete: some frame content has no top-level coordinates.\n" + : ""; const rendered = renderVom(decoratedScene, { maxDepth: options.maxDepth, - maxTokens: options.maxTokens, + maxTokens: + !geometryNotice || options.maxTokens === undefined + ? options.maxTokens + : Math.max(0, options.maxTokens - Math.ceil(geometryNotice.length / 4)), redactValues: options.redactValues, activeRegionPolicy: options.activeRegionPolicy, }); + if (geometryNotice) rendered.text += `\n${geometryNotice.trimEnd()}`; throwIfAborted(options.signal, "observation"); return projectRecordSafeObservation({ rootFrameId: captured.rootFrameId ?? normalizedDocuments[0]?.frameId ?? "root", diff --git a/apps/extension/src/tools/vom/__tests__/capture.test.ts b/apps/extension/src/tools/vom/__tests__/capture.test.ts index 3f26c10..6d95b68 100644 --- a/apps/extension/src/tools/vom/__tests__/capture.test.ts +++ b/apps/extension/src/tools/vom/__tests__/capture.test.ts @@ -1340,6 +1340,7 @@ describe("sibling frame measurement scheduling", () => { expect(fixture.measured()).not.toContain(200); for (const id of [105, 102, 101, 100]) pending.get(id)!(); const captured = await capture; + expect(captured.frameGeometryIssues).toEqual([]); expect(fixture.peak()).toBe(4); expect(fixture.active()).toBe(0); expect(fixture.measured()).toEqual([100, 101, 102, 103, 104, 105, 200]); @@ -1360,6 +1361,18 @@ describe("sibling frame measurement scheduling", () => { throw new Error("owner replaced"); }); const captured = await captureViewModel(fixture.cdp, 4); + expect(captured.frameGeometryIssues).toEqual([ + { + status: "unavailable", + source: { target: { tabId: 4 }, frameId: "frame-1" }, + ownerBackendNodeId: 100, + }, + { + status: "blocked", + source: { target: { tabId: 4 }, frameId: "frame-7" }, + cause: captured.frameGeometryIssues?.[0], + }, + ]); expect(fixture.measured()).not.toContain(200); expect(captured.iframeNodes.get(200)?.[0]).toMatchObject({ tag: "body", diff --git a/apps/extension/src/tools/vom/__tests__/frame-capture.test.ts b/apps/extension/src/tools/vom/__tests__/frame-capture.test.ts index 6bbd224..89512ec 100644 --- a/apps/extension/src/tools/vom/__tests__/frame-capture.test.ts +++ b/apps/extension/src/tools/vom/__tests__/frame-capture.test.ts @@ -50,6 +50,68 @@ function childSnapshot(frameId: string, backendNodeId: number) { } describe("captureFrameData", () => { + it("retains nested owner failures when merging a captured OOPIF", async () => { + const owner = ownerNode(10, 50); + const captured: CapturedViewModel = { + nodes: [owner], + viewport: { width: 1000, height: 800 }, + iframeNodes: new Map(), + frameNodes: new Map([["main", [owner]]]), + rootFrameId: "main", + excludedBackendNodeIds: new Set(), + }; + const child = childSnapshot("child", 101); + const nested = childSnapshot("nested", 201); + const document = child.documents[0]; + const snapshot = { + strings: child.strings, + documents: [ + { + ...document, + nodes: { ...document.nodes, contentDocumentIndex: { index: [1], value: [1] } }, + }, + nested.documents[0], + ], + }; + const reply = async (_target: unknown, method: string) => { + if (method === "Page.getLayoutMetrics") + return { cssLayoutViewport: { clientWidth: 300, clientHeight: 200 } }; + if (method === "DOMSnapshot.captureSnapshot") return snapshot; + if (method === "DOM.getBoxModel") + return { model: { content: [50, 100, 350, 100, 350, 300, 50, 300] } }; + if (method === "DOM.resolveNode") throw new Error("nested owner replaced"); + return {}; + }; + const cdp: CdpRunner = { + send: vi.fn(reply) as CdpRunner["send"], + sendToTarget: vi.fn(reply) as NonNullable, + getFrameGraph: vi.fn(async () => ({ + rootFrameId: "main", + frames: [ + { frameId: "main", target: { tabId: 4 } }, + { + frameId: "child", + parentFrameId: "main", + ownerBackendNodeId: 10, + target: { tabId: 4, sessionId: "child-session" }, + }, + ], + })), + }; + await captureFrameData(cdp, 4, captured); + expect(captured.frameGeometryIssues).toEqual([ + { + status: "unavailable", + source: { target: { tabId: 4, sessionId: "child-session" }, frameId: "nested" }, + ownerBackendNodeId: 101, + }, + ]); + expect(captured.frameNodes?.get("nested")?.[1]).toMatchObject({ + rect: null, + localRect: { x: 10, y: 20, w: 100, h: 40 }, + }); + }); + it("captures and positions multiple OOPIF documents missing from the root snapshot", async () => { const leftOwner = ownerNode(10, 50); const rightOwner = ownerNode(20, 500); diff --git a/apps/extension/src/tools/vom/capture.ts b/apps/extension/src/tools/vom/capture.ts index c341da3..4dddd85 100644 --- a/apps/extension/src/tools/vom/capture.ts +++ b/apps/extension/src/tools/vom/capture.ts @@ -9,8 +9,9 @@ import type { CdpTarget } from "@/browser-driver/frame-graph"; import { evaluateHoverTrigger } from "@/lib/hover-trigger-policy"; import { isOverlayHostNode, OVERLAY_HOST_SELECTOR } from "../../lib/overlay-bridge"; import { + type FrameProjectionIssue, + type FrameProjectionState, projectSnapshotRect, - type SnapshotProjection, snapshotViewportRect, } from "../geometry/coordinate-types"; import { cssViewport, GeometryContext, type LayoutMetrics } from "../geometry/frame-context"; @@ -77,6 +78,8 @@ export interface CapturedViewModel { /** Explicit DOMSnapshot frame ancestry; never inferred from backend node ids. */ frameParentIds?: Map; rootFrameId?: string; + /** Failed boundaries and descendants blocked by them; clipping is not a failure. */ + frameGeometryIssues?: FrameProjectionIssue[]; /** Backend node ids belonging to the agent overlay host + its shadow subtree. */ excludedBackendNodeIds: Set; } @@ -351,7 +354,7 @@ interface ParseDocumentResult { interface FrameContext { frameId?: string; ownerFrameBackendNodeId: number | null; - projection: SnapshotProjection | null; + projection: FrameProjectionState; target: CdpTarget; scrollX: number; scrollY: number; @@ -896,7 +899,9 @@ function parseDocumentNodes( localRect = { x, y, w, h }; } const bounds = - input && context.projection ? projectSnapshotRect(input, context.projection) : null; + input && context.projection.status === "available" + ? projectSnapshotRect(input, context.projection.projection) + : null; rect = bounds ? { x: bounds.x, y: bounds.y, w: bounds.width, h: bounds.height } : null; } paintOrder = dl?.paintOrders?.[li] ?? 0; @@ -964,6 +969,7 @@ function parseDocumentNodes( } interface ParseFrameDocumentsResult { + frameGeometryIssues: FrameProjectionIssue[]; iframeNodes: CapturedIframeNodes; frameOwnerBackendNodeIds: Map; frameParentIds: Map; @@ -979,6 +985,7 @@ async function parseChildFrameDocuments( signal?: AbortSignal, visited = new Set(), ): Promise { + const frameGeometryIssues: FrameProjectionIssue[] = []; const iframeNodes: CapturedIframeNodes = new Map(); const frameOwnerBackendNodeIds = new Map(); const frameParentIds = new Map(); @@ -986,7 +993,13 @@ async function parseChildFrameDocuments( const parentDoc = documents[parentDocIndex]; const cdi = sparseIndexMap(parentDoc?.nodes?.contentDocumentIndex); if (cdi.size === 0) { - return { iframeNodes, frameOwnerBackendNodeIds, frameParentIds, excludedBackendNodeIds }; + return { + iframeNodes, + frameOwnerBackendNodeIds, + frameParentIds, + excludedBackendNodeIds, + frameGeometryIssues, + }; } const parentBackendIds = parentDoc?.nodes?.backendNodeId ?? []; @@ -995,17 +1008,26 @@ async function parseChildFrameDocuments( const childDoc = documents[childDocIndex]; const iframeBackendId = parentBackendIds[nodeArrayIdx]; if (visited.has(childDocIndex) || !childDoc || iframeBackendId === undefined) return []; + const source = { target: parentContext.target, frameId: snapshotFrameId(childDoc, strings) }; + const parent = parentContext.projection; + const projection: FrameProjectionState = + parent.status === "available" + ? { status: "unavailable", source, ownerBackendNodeId: iframeBackendId } + : { status: "blocked", source, cause: parent.status === "blocked" ? parent.cause : parent }; return [ { childDocIndex, childDoc, iframeBackendId, - source: { target: parentContext.target, frameId: snapshotFrameId(childDoc, strings) }, - projection: null as SnapshotProjection | null, + source, + projection: projection as FrameProjectionState, }, ]; }); - const parentProjection = parentContext.projection?.geometry; + const parentProjection = + parentContext.projection.status === "available" + ? parentContext.projection.projection.geometry + : null; if (parentProjection) { // Measure only direct siblings. Recursion starts after these workers finish, // so descendants never hold or recursively acquire a measurement slot. @@ -1014,10 +1036,10 @@ async function parseChildFrameDocuments( ? [edge.destinationQuad, ...(edge.destinationClips ?? [])] : parentProjection.sourceClips; let cursor = 0; - let aborted: unknown; + let failure: unknown; await Promise.all( Array.from({ length: Math.min(4, children.length) }, async () => { - while (cursor < children.length && !signal?.aborted && !aborted) { + while (cursor < children.length && !signal?.aborted && !failure) { const child = children[cursor++]; try { // Owner quads are target-relative; do not apply the parent transform twice. @@ -1028,20 +1050,20 @@ async function parseChildFrameDocuments( parentProjection.topViewport, ); } catch (error) { - if (isAbortError(error)) aborted = error; - else console.debug("[bsk capture] frame owner geometry unavailable", error); + failure ??= error; } } }), ); // Join all active reads, including object cleanup, before propagating abort. - if (aborted) throw aborted; + if (failure) throw failure; } throwIfAborted(signal); // Completion order must not change document traversal or result insertion order. for (const { childDocIndex, childDoc, iframeBackendId, source, projection } of children) { throwIfAborted(signal); + if (projection.status !== "available") frameGeometryIssues.push(projection); const childContext: FrameContext = { frameId: source.frameId, target: source.target, @@ -1069,6 +1091,7 @@ async function parseChildFrameDocuments( signal, nextVisited, ); + frameGeometryIssues.push(...nested.frameGeometryIssues); for (const [nestedFrameId, nestedNodes] of nested.iframeNodes) { iframeNodes.set(nestedFrameId, nestedNodes); } @@ -1081,7 +1104,13 @@ async function parseChildFrameDocuments( for (const id of nested.excludedBackendNodeIds) excludedBackendNodeIds.add(id); } - return { iframeNodes, frameOwnerBackendNodeIds, frameParentIds, excludedBackendNodeIds }; + return { + iframeNodes, + frameOwnerBackendNodeIds, + frameParentIds, + excludedBackendNodeIds, + frameGeometryIssues, + }; } export async function captureViewModel( @@ -1131,8 +1160,11 @@ export async function captureViewModel( ownerFrameBackendNodeId: null, target, projection: { - source: { target, frameId: snapshotFrameId(doc0, strings) }, - geometry: { sourceClips: [], edges: [], topViewport: viewport }, + status: "available", + projection: { + source: { target, frameId: snapshotFrameId(doc0, strings) }, + geometry: { sourceClips: [], edges: [], topViewport: viewport }, + }, }, scrollX, scrollY, @@ -1174,6 +1206,7 @@ export async function captureViewModel( viewport, iframeNodes, frameNodes, + frameGeometryIssues: frameParsed.frameGeometryIssues, frameOwnerBackendNodeIds: frameParsed.frameOwnerBackendNodeIds, frameParentIds: frameParsed.frameParentIds, ...(topContext.frameId ? { rootFrameId: topContext.frameId } : {}), diff --git a/apps/extension/src/tools/vom/frame-capture.ts b/apps/extension/src/tools/vom/frame-capture.ts index 45beeb7..374945a 100644 --- a/apps/extension/src/tools/vom/frame-capture.ts +++ b/apps/extension/src/tools/vom/frame-capture.ts @@ -56,6 +56,9 @@ async function captureMissingFrameDocuments( geometry, target: frame.target, }); + if (child.frameGeometryIssues?.length) { + (captured.frameGeometryIssues ??= []).push(...child.frameGeometryIssues); + } let projection: GeometryProjection | null = null; try { projection = await geometry.targetProjection(frame.frameId);