Name recurring failure modes for agents (issue #465)

apps/extension/src/tools/__tests__/observe-identity-loop.test.ts

@@ -0,0 +1,141 @@
+// Issue #465 §1 / #454: pages with anti-debugging protection reload
+// themselves whenever the debugger attaches, so repeated reads keep
+// failing with the same identity error. The streak rule must name the
+// condition only after demonstrated repetition — and only on the same
+// page: maintainer review added that navigating the same tab to three
+// sites (one transient failure each) must not falsely diagnose the
+// third site.
+
+import { describe, expect, it } from "vitest";
+import {
+  nextObserveIdentityStreak,
+  OBSERVE_IDENTITY_LOOP_FAILURES,
+  observeIdentityReloadLoopError,
+} from "../observation";
+import { OBSERVE_IDENTITY_CHANGED } from "../vom/capture-coordinator";
+
+const SITE_A = "https://site-a.example/search";
+const SITE_B = "https://site-b.example/search";
+const SITE_C = "https://site-c.example/search";
+
+describe("observe identity reload-loop streak", () => {
+  it("does not flag an isolated failure", () => {
+    const first = nextObserveIdentityStreak(undefined, SITE_A, 1_000, 120_000);
+    expect(first.isReloadLoop).toBe(false);
+    expect(first.streak).toEqual({
+      url: SITE_A,
+      count: 1,
+      firstAt: 1_000,
+      lastAt: 1_000,
+    });
+  });
+
+  it("needs consecutive same-page failures inside the window before naming anti-debugging", () => {
+    const first = nextObserveIdentityStreak(undefined, SITE_A, 0, 120_000);
+    const second = nextObserveIdentityStreak(first.streak, SITE_A, 3_000, 120_000);
+    expect(second.isReloadLoop).toBe(false);
+    expect(second.streak).toEqual({
+      url: SITE_A,
+      count: 2,
+      firstAt: 0,
+      lastAt: 3_000,
+    });
+    const third = nextObserveIdentityStreak(second.streak, SITE_A, 6_000, 120_000);
+    expect(third.isReloadLoop).toBe(true);
+    expect(third.streak.count).toBe(OBSERVE_IDENTITY_LOOP_FAILURES);
+  });
+
+  it("a navigation to another site restarts the streak (regression: one transient failure on each of three sites must never diagnose site three)", () => {
+    let next = nextObserveIdentityStreak(undefined, SITE_A, 0, 120_000);
+    expect(next.isReloadLoop).toBe(false);
+    next = nextObserveIdentityStreak(next.streak, SITE_B, 1_000, 120_000);
+    expect(next.isReloadLoop).toBe(false);
+    const third = nextObserveIdentityStreak(next.streak, SITE_C, 2_000, 120_000);
+    // The third site has failed only once; the diagnosis must never carry
+    // across navigations.
+    expect(third.isReloadLoop).toBe(false);
+    expect(third.streak).toEqual({
+      url: SITE_C,
+      count: 1,
+      firstAt: 2_000,
+      lastAt: 2_000,
+    });
+  });
+
+  it("failures spread wider than the window restart the streak", () => {
+    const first = nextObserveIdentityStreak(undefined, SITE_A, 0, 120_000);
+    const next = nextObserveIdentityStreak(first.streak, SITE_A, 120_001, 120_000);
+    expect(next.isReloadLoop).toBe(false);
+    expect(next.streak.count).toBe(1);
+    // And a later failure still counts alone, so slow pages never trigger it.
+    const again = nextObserveIdentityStreak(next.streak, SITE_A, 130_000, 120_000);
+    expect(again.isReloadLoop).toBe(false);
+    expect(again.streak.count).toBe(2);
+  });
+
+  it("repeated reloads of the same page keep being detected after site switches", () => {
+    // The streak carries per page: two failures on site A are forgotten
+    // on navigation, then a genuine loop on the new site is diagnosed
+    // once three failures accumulate on *it*.
+    let next = nextObserveIdentityStreak(undefined, SITE_A, 0, 120_000);
+    next = nextObserveIdentityStreak(next.streak, SITE_A, 1_000, 120_000);
+    next = nextObserveIdentityStreak(next.streak, SITE_B, 2_000, 120_000); // navigate away
+    expect(next.isReloadLoop).toBe(false); // previous site's failures are not carried
+    next = nextObserveIdentityStreak(next.streak, SITE_B, 3_000, 120_000);
+    expect(next.isReloadLoop).toBe(false);
+    next = nextObserveIdentityStreak(next.streak, SITE_B, 4_000, 120_000);
+    expect(next.isReloadLoop).toBe(true);
+    expect(next.streak.url).toBe(SITE_B);
+  });
+
+  it("defaults the streak window to the published constant", () => {
+    const first = nextObserveIdentityStreak(undefined, SITE_A, 0);
+    expect(first.streak.count).toBe(1);
+  });
+
+  it("keeps the stable anti-debugging marker importable for detection", () => {
+    expect(OBSERVE_IDENTITY_CHANGED).toContain("observation document identity changed");
+  });
+});
+
+// Handler-level plumbing (maintainer review, P1): `ResolvedTargetTab.url`
+// is optional, and a failure on a tab whose URL is unavailable must not
+// count toward a page-specific streak — the original error is preserved.
+describe("observe identity reload-loop error plumbing", () => {
+  const ctxOf = (sessionId: string) =>
+    ({ sessionId }) as unknown as Parameters<typeof observeIdentityReloadLoopError>[0];
+  const identityFailure = () => new Error(OBSERVE_IDENTITY_CHANGED);
+
+  it("never counts failures with an unavailable URL toward a streak", () => {
+    const ctx = ctxOf("sess-nourl");
+    // If these were counted anywhere shared, the third failure would flip
+    // to the anti-debugging error; each must instead round-trip to the
+    // plain identity-changed cdp_failed path.
+    for (const url of [undefined, "", undefined]) {
+      expect(observeIdentityReloadLoopError(ctx, 7, url, identityFailure())).toBeNull();
+    }
+  });
+
+  it("still names the loop once a URL is known, with the page-scoped reason", () => {
+    const ctx = ctxOf("sess-withurl");
+    expect(observeIdentityReloadLoopError(ctx, 11, SITE_A, identityFailure())).toBeNull();
+    expect(observeIdentityReloadLoopError(ctx, 11, SITE_A, identityFailure())).toBeNull();
+    const third = observeIdentityReloadLoopError(ctx, 11, SITE_A, identityFailure());
+    expect(third).not.toBeNull();
+    expect(third?.code).toBe("cdp_failed");
+    expect(third?.data?.reason).toBe("observe_identity_reload_loop");
+    expect(third?.message).toContain(OBSERVE_IDENTITY_CHANGED);
+  });
+
+  it("an unavailable URL does not seed a later same-tab streak", () => {
+    const ctx = ctxOf("sess-seed");
+    expect(observeIdentityReloadLoopError(ctx, 5, undefined, identityFailure())).toBeNull();
+    // The tab resolves to a URL afterwards: only its own failures count.
+    expect(observeIdentityReloadLoopError(ctx, 5, SITE_B, identityFailure())).toBeNull();
+  });
+
+  it("keeps unrelated errors untouched", () => {
+    const ctx = ctxOf("sess-other");
+    expect(observeIdentityReloadLoopError(ctx, 3, SITE_C, new Error("boom"))).toBeNull();
+  });
+});

apps/extension/src/tools/__tests__/tabs.test.ts

@@ -829,7 +829,14 @@ describe("handleTabBorrow", () => {
       { session_id: "aa11", tab_id: 7 },
       { tabs: api, approveBorrow: async () => true },
     );
-    expect(res).toMatchObject({ code: "invalid_params" });
+    // Issue #465 §6: the borrow rejection carries a stable reason so the
+    // CLI explains that a user must move the tab out of the Agent Window
+    // first, instead of falling back to generic invalid-parameters copy.
+    expect(res).toMatchObject({
+      code: "invalid_params",
+      data: { reason: "borrow_agent_window_tab" },
+    });
+    expect((res as { message?: unknown }).message).toContain("already lives in the Agent Window");
   });
 
   it("refuses to borrow a tab already held by another session", async () => {

apps/extension/src/tools/observation.ts

@@ -70,7 +70,11 @@ import {
 import { lookupRefTarget, resolveSnapshotRef } from "./snapshot-ref";
 import { captureControlledViewport } from "./viewport-screenshot";
 import { type CapturedNode, type CapturedSurfaceProbe, probeHoverSurfaces } from "./vom/capture";
-import { semanticCapture, withObservationFacts } from "./vom/capture-coordinator";
+import {
+  OBSERVE_IDENTITY_CHANGED,
+  semanticCapture,
+  withObservationFacts,
+} from "./vom/capture-coordinator";
 import type { FrameDocument as CapturedFrameDocument } from "./vom/frame-document";
 import { withOverlayBypass } from "./vom/hover-perception";
 import { probeTooltipNames } from "./vom/name-enrichment";
@@ -1078,6 +1082,92 @@ async function buildVomObservation(
   });
 }
 
+// ---------------------------------------------------------------------------
+// Anti-debugging reload-loop detection (issues #454 / #465). Sites that
+// detect the debugger attach reload themselves, so every repeated read
+// fails with the same identity error while the page keeps churning.
+// After consecutive identity failures on the same tab inside a short
+// window we name the condition with a stable reason instead of letting
+// the agent retry (and never point it at a reload).
+// ---------------------------------------------------------------------------
+
+interface ObserveIdentityStreak {
+  url: string;
+  count: number;
+  firstAt: number;
+  lastAt: number;
+}
+
+const observeIdentityStreaks = new Map<string, ObserveIdentityStreak>();
+
+/** Identity failures spread wider than this never count as a reload loop. */
+export const OBSERVE_IDENTITY_LOOP_WINDOW_MS = 120_000;
+
+/** Consecutive identity failures before the failure is named anti-debugging. */
+export const OBSERVE_IDENTITY_LOOP_FAILURES = 3;
+
+/** Identity-failure bookkeeping key for one session's tab. */
+function observeIdentityKey(sessionId: string, tabId: number): string {
+  return `${sessionId}:${tabId}`;
+}
+
+/**
+ * Pure rule so the streak logic is testable without a browser: a failure
+ * joins the previous streak only when it is on the **same page** (identical
+ * URL) and happens within `windowMs` of the streak's first failure; a
+ * navigation to another site or an expired window restarts the counter
+ * (maintainer review of issue #465: one transient failure each on three
+ * different sites must not falsely diagnose the third site).
+ */
+export function nextObserveIdentityStreak(
+  previous: ObserveIdentityStreak | undefined,
+  url: string,
+  failureAt: number,
+  windowMs: number = OBSERVE_IDENTITY_LOOP_WINDOW_MS,
+): { streak: ObserveIdentityStreak; isReloadLoop: boolean } {
+  const continues =
+    previous !== undefined && previous.url === url && failureAt - previous.firstAt <= windowMs;
+  const streak: ObserveIdentityStreak = continues
+    ? {
+        url,
+        count: previous!.count + 1,
+        firstAt: previous!.firstAt,
+        lastAt: failureAt,
+      }
+    : { url, count: 1, firstAt: failureAt, lastAt: failureAt };
+  return { streak, isReloadLoop: streak.count >= OBSERVE_IDENTITY_LOOP_FAILURES };
+}
+
+/**
+ * Turn a repeated observation identity failure into the distinct
+ * anti-debugging error; returns null while the failure still looks
+ * transient so the normal `cdp_failed` path applies.
+ *
+ * The streak is page-scoped, so an observation whose target could not
+ * be resolved to a URL never counts toward one — that failure keeps
+ * the plain identity-changed error (maintainer review of issue #465,
+ * P1: `ResolvedTargetTab.url` is optional). Exported for tests.
+ */
+export function observeIdentityReloadLoopError(
+  ctx: SessionContext,
+  tabId: number,
+  tabUrl: string | undefined,
+  err: unknown,
+): RpcError | null {
+  const message = err instanceof Error ? err.message : String(err);
+  if (!message.includes(OBSERVE_IDENTITY_CHANGED)) return null;
+  if (!tabUrl) return null;
+  const key = observeIdentityKey(ctx.sessionId, tabId);
+  const next = nextObserveIdentityStreak(observeIdentityStreaks.get(key), tabUrl, Date.now());
+  observeIdentityStreaks.set(key, next.streak);
+  if (!next.isReloadLoop) return null;
+  return rpcError(
+    "cdp_failed",
+    "observe_identity_reload_loop",
+    `${OBSERVE_IDENTITY_CHANGED} — the page lost observation identity on ${next.streak.count} consecutive reads (the site is likely reloading itself when the debugger attaches; anti-debugging)`,
+  );
+}
+
 async function handleVomObservation(
   manager: SessionManager,
   params: SnapshotParams | ObserveParams,
@@ -1104,6 +1194,11 @@ async function handleVomObservation(
   if (denied) return denied;
   const dialogCursor = markDialogCursor(deps.cdp, target.tabId);
 
+  // Anti-debugging reload-loop bookkeeping: the streak is cleared the
+  // moment any read of this tab succeeds (see the helper above).
+  const identityKey = observeIdentityKey(ctx.sessionId, target.tabId);
+  let readSucceeded = false;
+
   try {
     throwIfAborted(signal, toolName);
     deps.cdp.trackSessionTab?.(ctx.sessionId, target.tabId);
@@ -1127,7 +1222,9 @@ async function handleVomObservation(
         { cursor, maxTokens: params.max_tokens },
         signal,
       );
-      return isRpcError(page) ? page : attachDialogs(deps.cdp, target.tabId, dialogCursor, page);
+      if (isRpcError(page)) return page;
+      readSucceeded = true;
+      return attachDialogs(deps.cdp, target.tabId, dialogCursor, page);
     }
     clearObservationContinuation(ctx.refStore);
     const documentRevision = ctx.refStore.documentRevision(target.tabId);
@@ -1158,6 +1255,7 @@ async function handleVomObservation(
         signal,
       );
       if (isRpcError(page)) return page;
+      readSucceeded = true;
       return attachDialogs(deps.cdp, target.tabId, dialogCursor, {
         ...page,
         ...(observation.hoverProbe?.performed
@@ -1201,6 +1299,7 @@ async function handleVomObservation(
         ] as const;
       }),
     );
+    readSucceeded = true;
     return attachDialogs(deps.cdp, target.tabId, dialogCursor, {
       text: observation.text,
       ref_count: observation.refs.length,
@@ -1232,11 +1331,17 @@ async function handleVomObservation(
     // Fresh observations clear their predecessor before capture. Continuation owns
     // invalidation; cancellation must preserve its unpublished page for retry.
     if (isAbortError(err)) return cancelled(toolName);
+    const identityLoopError = observeIdentityReloadLoopError(ctx, target.tabId, target.url, err);
+    if (identityLoopError) return identityLoopError;
     return {
       code: "cdp_failed",
       message: err instanceof Error ? err.message : String(err),
       ...readTimeoutDetails(err),
     };
+  } finally {
+    if (readSucceeded) {
+      observeIdentityStreaks.delete(identityKey);
+    }
   }
 }
 

apps/extension/src/tools/tabs.ts

@@ -756,13 +756,20 @@ async function validateBorrowTarget(
       `tab_borrow: tab ${tabId} is already controlled by session ${owner}`,
     );
   if (tab.windowId === ctx.agentWindowId) {
-    return {
-      code: "invalid_params",
-      message:
-        ctx.remote && !isAgentControlledTab(ctx, tabId)
-          ? `tab_borrow: tab ${tabId} is not authorized and already lives in the Agent Window; move it to a regular browser window, then borrow it`
-          : `tab_borrow: tab ${tabId} already lives in the Agent Window`,
-    };
+    // Issue #465 §6: both branches mean "already inside an Agent Window",
+    // which the reference docs describe as needing a user move to a user
+    // window before borrowing; a tab the session itself opened is already
+    // controlled, so no borrow is needed at all. The stable reason lets
+    // the CLI render this instead of the generic invalid-parameters copy.
+    const lives =
+      ctx.remote && !isAgentControlledTab(ctx, tabId)
+        ? "not authorized and already lives in the Agent Window; a user must move it to a regular browser window, then borrow it"
+        : "already lives in the Agent Window — an unowned tab needs a user move to a user window before borrowing; a tab this session opened is already controlled and needs no borrow";
+    return rpcError(
+      "invalid_params",
+      "borrow_agent_window_tab",
+      `tab_borrow: tab ${tabId} ${lives}`,
+    );
   }
   for (const s of manager.list()) {
     if (s.sessionId !== ctx.sessionId && s.agentWindowId === tab.windowId) {

apps/extension/src/tools/vom/capture-coordinator.ts

@@ -42,6 +42,15 @@ interface TargetBatch<T extends FrameOwnedAxNode> {
   rootFrameId?: string;
 }
 
+/**
+ * Stable message for an observation whose root document identity could
+ * not be (re)verified. `observation.ts` matches on this marker to detect
+ * the anti-debugging reload loop, where a site reloads itself as soon
+ * as the debugger attaches (GitHub issues #454 and #465).
+ */
+export const OBSERVE_IDENTITY_CHANGED =
+  "observation document identity changed or could not be verified; observe again";
+
 /** A fixed worker pool scoped to one capture; each worker issues at most one
  * collection request at a time. Geometry has its own measurement-phase bound. */
 async function collectTasks<T>(
@@ -533,7 +542,7 @@ async function collectObservationFacts<T extends FrameOwnedAxNode>(
     throw new Error(
       ownershipInvalid.has(rootFrameId)
         ? "observation root document ownership is ambiguous"
-        : "observation document identity changed or could not be verified; observe again",
+        : OBSERVE_IDENTITY_CHANGED,
     );
   const frames = currentFrames.filter((frame) => !invalid.has(frame.frameId));
   const frameById = new Map(frames.map((frame) => [frame.frameId, frame]));

apps/extension/src/transport/types.ts

@@ -66,6 +66,8 @@ export type RpcErrorReason =
   | "tab_not_active"
   | "restricted_tab_url"
   | "cdp_extension_access_denied"
+  | "observe_identity_reload_loop"
+  | "borrow_agent_window_tab"
   | "borrow_conflict"
   | "incognito_borrow_disabled"
   | "borrow_in_progress"