Fence browser observations by generation - #63
Conversation
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Missing TSDoc on observation exports
- Added TSDoc comments to all newly exported observation interfaces, the ObservationChangedError class, and cohortKey to satisfy documentation requirements.
Or push these changes by commenting:
@cursor push 0ee770931c
Preview (0ee770931c)
diff --git a/packages/agent/src/translator/browser-observation.ts b/packages/agent/src/translator/browser-observation.ts
--- a/packages/agent/src/translator/browser-observation.ts
+++ b/packages/agent/src/translator/browser-observation.ts
@@ -1,3 +1,4 @@
+/** Minimal accessibility node shape consumed by browser observation rendering. */
export interface AXNode {
nodeId: string;
ignored?: boolean;
@@ -10,11 +11,13 @@
childIds?: string[];
}
+/** Indexes node ref ordinals within role/name cohorts for stable references. */
export interface NthIndex {
index: Map<string, number>;
cohorts: Map<string, number>;
}
+/** Carries frame/session metadata needed to render and trace an observed node. */
export interface RenderContext {
targetId: string;
frameKey: string;
@@ -25,23 +28,27 @@
cursorIds?: ReadonlySet<number>;
}
+/** One rendered observation line plus source node and render context. */
export interface ObservationLine {
text: string;
refNode?: AXNode;
ctx: RenderContext;
}
+/** Stitched frame tree with node lookup and per-frame render context. */
export interface FrameStitch {
byId: Map<string, AXNode>;
roots: string[];
ctx: RenderContext;
}
+/** Flattened observed node entry paired with the context it came from. */
export interface ObservedNode {
node: AXNode;
ctx: RenderContext;
}
+/** Full browser observation snapshot assembled from stitched frame trees. */
export interface BrowserObservation {
targetId: string;
tree: FrameStitch;
@@ -52,6 +59,7 @@
generations: Map<string, number>;
}
+/** Cached presentation artifact derived from a browser observation snapshot. */
export interface BrowserPresentation {
observation: BrowserObservation;
cacheKey: string;
@@ -59,6 +67,7 @@
shape: string;
}
+/** Signals that observation data changed mid-collection and must be retried. */
export class ObservationChangedError extends Error {
constructor(message = "Browser observation changed during collection") {
super(message);
@@ -80,6 +89,7 @@
return { index, cohorts };
}
+/** Builds a stable cohort identifier from a node's role/name pair. */
export function cohortKey(role: string, name: string): string {
return `${role}\u0000${name}`;
}You can send follow-ups to the cloud agent here.
4067b8b to
0c29b34
Compare
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0c29b34. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Missing TSDoc on frame exports
- Added TSDoc comments for the exported frame-collection error class and helper functions to document their purpose and expected usage boundary.
- ✅ Fixed: OOPIF events skip invalidation
- OOPIF invalidation and removal now fall back to deriving frame owners from imported refs when owner mappings are missing, and regression tests cover navigated/detached event paths.
Or push these changes by commenting:
@cursor push e2004c93d0
Preview (e2004c93d0)
diff --git a/packages/agent/src/translator/browser-frame-collection.ts b/packages/agent/src/translator/browser-frame-collection.ts
--- a/packages/agent/src/translator/browser-frame-collection.ts
+++ b/packages/agent/src/translator/browser-frame-collection.ts
@@ -1,5 +1,9 @@
import { CdpProtocolError } from "./cdp";
+/**
+ * Wraps unexpected iframe collection failures with context about which frame
+ * and collection stage failed.
+ */
export class FrameCollectionError extends Error {
constructor(message: string, cause: unknown) {
super(message, { cause });
@@ -7,6 +11,11 @@
}
}
+/**
+ * True when a CDP error is one of the known transient "frame disappeared"
+ * variants that should mark the frame as incomplete instead of failing the
+ * whole observation.
+ */
export function isExpectedFrameCollectionError(
error: unknown,
method: "DOM.describeNode" | "Accessibility.getFullAXTree",
@@ -21,6 +30,10 @@
);
}
+/**
+ * Build a contextual {@link FrameCollectionError} for an unexpected iframe
+ * collection failure.
+ */
export function frameCollectionError(
backendNodeId: number,
frameId: string | undefined,
diff --git a/packages/agent/src/translator/browser.ts b/packages/agent/src/translator/browser.ts
--- a/packages/agent/src/translator/browser.ts
+++ b/packages/agent/src/translator/browser.ts
@@ -142,7 +142,7 @@
if (this.frameTargets.has(sessionTargetId)) {
// The OOPIF tree and any same-process descendants fetched through
// its session share the OOPIF generation key.
- const owner = this.frameOwners.get(sessionTargetId);
+ const owner = this.ownerForFrameTarget(sessionTargetId);
if (owner) {
this.lifecycle.invalidateFrame(owner, sessionTargetId);
if (frame.loaderId) this.lifecycle.recordDocument(sessionTargetId, owner, frame.loaderId);
@@ -168,7 +168,7 @@
const sessionTargetId = this.targetsBySession.get(event.sessionId);
if (!frameId || !sessionTargetId) return;
if (this.frameTargets.has(sessionTargetId)) {
- const owner = this.frameOwners.get(sessionTargetId);
+ const owner = this.ownerForFrameTarget(sessionTargetId);
if (owner) this.lifecycle.invalidateFrame(owner, sessionTargetId);
} else {
this.lifecycle.removeFrame(sessionTargetId, frameId);
@@ -218,7 +218,7 @@
this.targetsBySession.delete(sessionId);
if (!targetId) return;
if (this.frameTargets.has(targetId)) {
- const owner = this.frameOwners.get(targetId);
+ const owner = this.ownerForFrameTarget(targetId);
if (owner) this.lifecycle.removeFrame(owner, targetId);
this.frameSessions.delete(targetId);
this.frameOwners.delete(targetId);
@@ -967,6 +967,17 @@
return entry;
}
+ private ownerForFrameTarget(frameTargetId: string): string | undefined {
+ const mapped = this.frameOwners.get(frameTargetId);
+ if (mapped) return mapped;
+ for (const entry of this.refs.values()) {
+ if (entry.frameId !== frameTargetId) continue;
+ this.frameOwners.set(frameTargetId, entry.targetId);
+ return entry.targetId;
+ }
+ return undefined;
+ }
+
private dropTarget(targetId: string): void {
this.lifecycle.dropTarget(targetId);
this.selfNavigations.delete(targetId);
diff --git a/packages/agent/test/translator-browser.test.ts b/packages/agent/test/translator-browser.test.ts
--- a/packages/agent/test/translator-browser.test.ts
+++ b/packages/agent/test/translator-browser.test.ts
@@ -783,7 +783,27 @@
fake.setSessionTree("session-oop", OOPIF_CHILD);
return fake;
};
+ const importOopifRefWithoutOwner = async () => {
+ const source = setupOopif();
+ const mint = new BrowserExecutor(source.cdp);
+ await snapshotText(mint);
+ const state = mint.exportRefState();
+ const fake = createFakeCdp(OOPIF_PAGE);
+ fake.setIframeFrame(50, "FRAME-OOP");
+ fake.setSessionTree("session-oop", OOPIF_CHILD);
+ const executor = new BrowserExecutor(fake.cdp);
+ executor.importRefState(state);
+ // Simulate a frame session that attached without a parent session id, so
+ // the target->owner mapping is absent until a later observation rebuilds it.
+ fake.emit({
+ method: "Target.attachedToTarget",
+ params: { sessionId: "session-oop", targetInfo: { targetId: "FRAME-OOP", type: "iframe" } },
+ });
+ expect([...refsOf(executor).keys()].sort()).toEqual(["e1", "e2", "e3"]);
+ return { executor, fake };
+ };
+
it("resolves an OOPIF ref's node through the child session but dispatches input on the page session", async () => {
const { cdp, sent } = setupOopif();
const executor = new BrowserExecutor(cdp);
@@ -900,6 +920,16 @@
expect(text).toContain('button "Pay" [e');
});
+ it.each([
+ { label: "Page.frameNavigated", event: { method: "Page.frameNavigated", params: { frame: { id: "FRAME-OOP" } }, sessionId: "session-oop" } },
+ { label: "Page.frameDetached", event: { method: "Page.frameDetached", params: { frameId: "FRAME-OOP", reason: "swap" }, sessionId: "session-oop" } },
+ { label: "Target.detachedFromTarget", event: { method: "Target.detachedFromTarget", params: { sessionId: "session-oop" } } },
+ ] as const)("drops imported OOPIF refs when $label fires before owner mapping is known", async ({ event }) => {
+ const { fake, executor } = await importOopifRefWithoutOwner();
+ fake.emit(event);
+ expect([...refsOf(executor).keys()].sort()).toEqual(["e1", "e2"]);
+ });
+
it("invalidates and releases a same-process frame when it detaches and rotates", async () => {
const root = [
ax({ nodeId: "1", role: "RootWebArea", name: "Page", childIds: ["2"] }),You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 9d41e64. Configure here.


summary
stack
size
542 changed lines: 388 additions, 154 deletions.
testing
npm run typechecknpm test --workspace @onkernel/cua-agent— 170 passed, 16 live tests skippedgit diff --check origin/main...HEADNote
High Risk
Changes core browser ref validity, cross-process persistence, and iframe/OOPIF hit-testing; mistakes could cause stale refs, wrong clicks, or silent mis-targeting after navigation.
Overview
Snapshot and find now share a generation-fenced observation pipeline: AX trees (including stitched iframes) are collected into a structured
BrowserObservation, checked against target metadata and frame generations, and retried (up to three times) onObservationChangedError/ incomplete frames before any refs are minted. Presentation, unchanged-snapshot caching, and ref scoping move to a separate step on top of that stable snapshot.Ref lifecycle is extracted into
RefGenerationLifecyclewith capture/release during collection, per-frame invalidation, and exported state that addsdocuments(loaderIdper ref-owning frame) plusREF_STATE_VERSION. On attach,BrowserDocumentReconcilerreconciles imported identities so cross-process refs stay valid only when the live document matches; legacy or partial identity fails safe (stale) instead of trusting process-local generation alone.browser_fill/browser_scroll_tonow attach (and reconcile) before resolving refs, like click/hover.Iframe handling gains typed
CdpProtocolError, an allow-list for transient collection races vsFrameCollectionErrorfor unexpected failures, incomplete-frame tracking for scoped refs, and OOPIF click/hover coordinate translation via the iframe owner offset. CDP navigation/detach events update document identity and invalidate the correct frame vs whole target.Reviewed by Cursor Bugbot for commit 8287879. Bugbot is set up for automated code reviews on this repo. Configure here.