From b99bc6cc3c6ebe6711b4571e9dcfacef2430c262 Mon Sep 17 00:00:00 2001 From: ciregenz Date: Tue, 28 Jul 2026 11:02:27 -0700 Subject: [PATCH] [eric] browser: make a read wait until the page stops growing, not just until it looks long enough --- backend/apps/agents/browser/browser_agent.py | 16 ++--- .../agents/browser/browser_read_script.py | 25 +++++-- backend/tests/test_browser_read_script.py | 69 ++++++++++++++++++- backend/tests/test_browser_session_import.py | 16 ++--- 4 files changed, 103 insertions(+), 23 deletions(-) diff --git a/backend/apps/agents/browser/browser_agent.py b/backend/apps/agents/browser/browser_agent.py index ad2493fb..c96152ef 100644 --- a/backend/apps/agents/browser/browser_agent.py +++ b/backend/apps/agents/browser/browser_agent.py @@ -130,7 +130,7 @@ p_bridge_known_absent: set[str] = set() # Sites whose sign-in we already borrowed into the partition. Only SUCCESSFUL borrows are recorded, # so a site the user signs into later still gets picked up without a restart; the re-probe that # costs is a keychain-free row count, so re-asking is cheap. -p_signin_borrowed: set[str] = set() +signin_borrowed: set[str] = set() def parse_bridge_result(result: dict) -> object: @@ -255,7 +255,7 @@ def p_summarize_action(tool_name: str, tool_input: dict) -> str: return p_summ_step(stype, ti) if stype else "" -async def p_borrow_signin_before_nav(url: str, browser_id: str) -> None: +async def borrow_signin_before_nav(url: str, browser_id: str) -> None: """Load the user's own sign-in for a site BEFORE its first page load, so the very first request already carries it. @@ -270,11 +270,11 @@ async def p_borrow_signin_before_nav(url: str, browser_id: str) -> None: if not browser_session_import.is_enabled(load_settings()): return domain = browser_session_import.site_domain(url) - if not domain or domain in p_signin_borrowed: + if not domain or domain in signin_borrowed: return if not browser_session_import.has_importable_session(domain): return - p_signin_borrowed.add(domain) + signin_borrowed.add(domain) await browser_session_import.import_signin(domain, browser_id) except Exception as exc: logger.info(f"[session-import] pre-nav borrow skipped: {type(exc).__name__}") @@ -285,7 +285,7 @@ async def execute_browser_tool( ) -> dict: """Execute a browser tool via ws_manager directly (no MCP/HTTP round-trip).""" if tool_name == "BrowserNavigate": - await p_borrow_signin_before_nav(str((tool_input or {}).get("url") or ""), browser_id) + await borrow_signin_before_nav(str((tool_input or {}).get("url") or ""), browser_id) # One greppable line naming the actual buttons/keys/selectors this call drives, # so a run reads as "key:ArrowRight x5" rather than an opaque tool name. Fires # for action tools only (reads stay quiet) and ungated so web runs get it too. @@ -844,11 +844,11 @@ async def try_borrow_signin(domain: str, browser_id: str, tab_id: str, url: str) return False # Already borrowed at the door and STILL standing at a wall, so the session did not take. # Re-importing the same values would change nothing; this one needs a human. - if domain in p_signin_borrowed: + if domain in signin_borrowed: return False if not browser_session_import.has_importable_session(domain): return False - p_signin_borrowed.add(domain) + signin_borrowed.add(domain) result = await browser_session_import.import_signin(domain, browser_id) if not result.ok: return False @@ -3187,7 +3187,7 @@ async def run_browser_agents( # instant it mounts, with no BrowserNavigate to hook, so this is the only moment the # very first request can already carry the session. Outside the pick lock: it can touch # the keychain, and holding a lock across that would serialize every card creation. - await p_borrow_signin_before_nav(host_src, "") + await borrow_signin_before_nav(host_src, "") async with p_card_pick_lock: browser_id = find_reusable_card(dashboard_id, host_src, parent_session_id) if browser_id: diff --git a/backend/apps/agents/browser/browser_read_script.py b/backend/apps/agents/browser/browser_read_script.py index 5ba44d47..7e4456cb 100644 --- a/backend/apps/agents/browser/browser_read_script.py +++ b/backend/apps/agents/browser/browser_read_script.py @@ -23,8 +23,16 @@ P_TEXT_TIMEOUT_S = 8.0 P_AUX_TIMEOUT_S = 12.0 # Prestage's click often lands here while the SPA is still hydrating (measured: a # LinkedIn profile read 184 chars right after the click); wait out the render, bounded. -P_THIN_RETRIES = 3 P_THIN_SETTLE_S = 1.2 +# Crossing the char floor is NOT the same as being finished rendering: a hydrating SPA clears 500 +# chars on nav and footer chrome long before the content lands. Taking that first passing read hands +# the aux a half-drawn page, and it answers confidently from what IS there, so nothing declines and +# the INSUFFICIENT retry below never fires. A confident wrong answer is the one outcome worse than +# just running the loop, so the page has to prove it stopped growing: two reads in a row within this +# much of each other. Costs one extra read plus one short settle on a path that already takes ~7-10s. +P_STABLE_GROWTH = 0.05 +P_STABLE_SETTLE_S = 0.4 +MAX_READS = 4 # A long-enough-but-still-rendering page reads as INSUFFICIENT (measured: profile # passed 500 chars with the headline section missing); one settle + re-read + re-ask. P_INSUFFICIENT_RETRIES = 1 @@ -71,15 +79,22 @@ async def run_read_script( from backend.apps.agents.core.aux_llm import safe_resp_text async def p_page_text() -> tuple: - for attempt in range(P_THIN_RETRIES): + """Page text, but only once two consecutive reads agree it has stopped growing.""" + prev = -1 + text, url = "", "" + for attempt in range(MAX_READS): r = await asyncio.wait_for( execute_tool("BrowserGetText", {}, browser_id, tab_id), timeout=P_TEXT_TIMEOUT_S) text = str(r.get("text") or "") if isinstance(r, dict) and "error" not in r else "" url = str(r.get("url") or "") if isinstance(r, dict) else "" - if len(text) >= P_MIN_PAGE_CHARS: + if len(text) >= P_MIN_PAGE_CHARS and 0 <= prev <= len(text) <= prev * (1 + P_STABLE_GROWTH): return text, url - await asyncio.sleep(P_THIN_SETTLE_S) - return "", "" + # Still thin waits longer than merely still-growing: one is a page that has not + # started, the other is one about to finish. + thin = len(text) < P_MIN_PAGE_CHARS + prev = len(text) + await asyncio.sleep(P_THIN_SETTLE_S if thin else P_STABLE_SETTLE_S) + return (text, url) if len(text) >= P_MIN_PAGE_CHARS else ("", "") for ask in range(1 + P_INSUFFICIENT_RETRIES): page, p_live_url = await p_page_text() diff --git a/backend/tests/test_browser_read_script.py b/backend/tests/test_browser_read_script.py index bd9b95d8..63377327 100644 --- a/backend/tests/test_browser_read_script.py +++ b/backend/tests/test_browser_read_script.py @@ -14,8 +14,14 @@ class Resp: class Aux: - def __init__(self, text): self.txt = text; self.messages = self; self.calls = 0 - async def create(self, **kw): self.calls += 1; return Resp(self.txt) + def __init__(self, text): + self.txt = text; self.messages = self; self.calls = 0; self.last_page = "" + + async def create(self, **kw): + self.calls += 1 + # Keep what the aux was actually shown: which page reached it IS the hydration contract. + self.last_page = str(kw.get("messages", [{}])[0].get("content", "")) + return Resp(self.txt) def tool_returning(text): @@ -28,6 +34,65 @@ def tool_returning(text): PAGE = "Tyler Chen\nHe/Him ยท 1st\nSomething Here\nIrvine, California\nEntrepreneurs First\n" + ("filler " * 200) +def tool_returning_sequence(pages): + """A page that changes between reads, like an SPA finishing its render.""" + seen = [] + + async def run_tool(name, params, browser_id, tab_id): + assert name == "BrowserGetText" + seen.append(len(seen)) + return {"text": pages[min(len(seen) - 1, len(pages) - 1)]} + run_tool.seen = seen + return run_tool + + +# Chrome that clears the 500-char floor while the actual content is still missing. This is the +# shape that made the bug invisible: it is long enough to look like a real page. +CHROME_ONLY = ("Home Feed My Network Jobs Messaging Notifications Me Work " + "Skip to main content Keyboard shortcuts Close jump menu " * 12) +HYDRATED = CHROME_ONLY + "\nTyler Chen\nSomething Here\nEntrepreneurs First\n" + ("filler " * 200) + + +def test_waits_for_the_page_to_stop_growing(monkeypatch): + """The false-clean: a hydrating SPA crosses the char floor on nav and footer chrome long before + the content lands. Answering from that first passing read produces a CONFIDENT WRONG answer, + because the aux reports what it can see and nothing declines, so the INSUFFICIENT retry never + fires. Two reads have to agree the page stopped growing before the aux sees anything.""" + monkeypatch.setattr(rs, "P_THIN_SETTLE_S", 0) + monkeypatch.setattr(rs, "P_STABLE_SETTLE_S", 0) + aux = Aux("His title is \"Something Here\".") + tool = tool_returning_sequence([CHROME_ONLY, HYDRATED, HYDRATED]) + out = asyncio.run(rs.run_read_script(aux, "m", "find tyler chen's title", "b1", "t1", tool)) + + assert out == "His title is \"Something Here\"." + assert len(tool.seen) >= 3, "must re-read until two reads agree, not answer off the first" + sent = aux.last_page + assert "Tyler Chen" in sent, "the aux must be handed the HYDRATED page, not the chrome-only one" + + +def test_a_settled_page_still_answers_without_extra_waiting(monkeypatch): + """The guard must not turn every read into a slow read: a page that is already done answers as + soon as two reads agree, which is immediately.""" + monkeypatch.setattr(rs, "P_THIN_SETTLE_S", 0) + monkeypatch.setattr(rs, "P_STABLE_SETTLE_S", 0) + aux = Aux("answer") + tool = tool_returning_sequence([HYDRATED, HYDRATED]) + assert asyncio.run(rs.run_read_script(aux, "m", "q", "b1", "t1", tool)) == "answer" + assert len(tool.seen) == 2, "a settled page costs exactly one confirming re-read" + + +def test_a_page_that_never_settles_still_answers_from_the_last_read(monkeypatch): + """Something that keeps streaming forever (a live feed) must not fail closed to the loop just + for being busy; after the read budget we use the fullest page we got.""" + monkeypatch.setattr(rs, "P_THIN_SETTLE_S", 0) + monkeypatch.setattr(rs, "P_STABLE_SETTLE_S", 0) + grows = [HYDRATED + ("more " * 200 * i) for i in range(1, 8)] + aux = Aux("answer") + tool = tool_returning_sequence(grows) + assert asyncio.run(rs.run_read_script(aux, "m", "q", "b1", "t1", tool)) == "answer" + assert len(tool.seen) == rs.MAX_READS + + def test_flag_gate(monkeypatch): monkeypatch.delenv("OSW_READ_SCRIPT", raising=False) assert rs.read_script_enabled() is False diff --git a/backend/tests/test_browser_session_import.py b/backend/tests/test_browser_session_import.py index b5b3a2dc..6aa1ee7a 100644 --- a/backend/tests/test_browser_session_import.py +++ b/backend/tests/test_browser_session_import.py @@ -189,7 +189,7 @@ async def test_borrow_happens_at_the_door_not_only_at_the_wall(monkeypatch): from backend.apps.agents.browser import browser_agent seen = [] - browser_agent.p_signin_borrowed.discard("x.com") + browser_agent.signin_borrowed.discard("x.com") monkeypatch.setattr(browser_agent.browser_session_import, "is_enabled", lambda s: True) monkeypatch.setattr(browser_agent.browser_session_import, "has_importable_session", lambda d: True) @@ -198,13 +198,13 @@ async def test_borrow_happens_at_the_door_not_only_at_the_wall(monkeypatch): return si.SessionImportResult(outcome="imported", domain=domain, entries_applied=3) monkeypatch.setattr(browser_agent.browser_session_import, "import_signin", fake_import) - await browser_agent.p_borrow_signin_before_nav("https://x.com/compose/post", "b1") + await browser_agent.borrow_signin_before_nav("https://x.com/compose/post", "b1") assert seen == ["x.com"], "navigating to a site must borrow its sign-in first" # Second navigate to the same site must not re-read the user's browser. - await browser_agent.p_borrow_signin_before_nav("https://x.com/home", "b1") + await browser_agent.borrow_signin_before_nav("https://x.com/home", "b1") assert seen == ["x.com"], "a borrowed site must not be re-imported on every navigate" - browser_agent.p_signin_borrowed.discard("x.com") + browser_agent.signin_borrowed.discard("x.com") @pytest.mark.asyncio @@ -212,11 +212,11 @@ async def test_pre_nav_borrow_respects_the_opt_in(monkeypatch): """The door is the busiest path in the whole agent, so the gate has to hold there too.""" from backend.apps.agents.browser import browser_agent - browser_agent.p_signin_borrowed.discard("x.com") + browser_agent.signin_borrowed.discard("x.com") monkeypatch.setattr(browser_agent.browser_session_import, "is_enabled", lambda s: False) monkeypatch.setattr(browser_agent.browser_session_import, "has_importable_session", lambda d: pytest.fail("must not probe the user's browser while opted out")) - await browser_agent.p_borrow_signin_before_nav("https://x.com/home", "b1") + await browser_agent.borrow_signin_before_nav("https://x.com/home", "b1") @pytest.mark.asyncio @@ -226,14 +226,14 @@ async def test_wall_handoff_asks_a_human_once_the_door_borrow_did_not_take(monke silently returning True here would skip the prompt and strand the run.""" from backend.apps.agents.browser import browser_agent - browser_agent.p_signin_borrowed.add("acme.example") + browser_agent.signin_borrowed.add("acme.example") monkeypatch.setattr(browser_agent.browser_session_import, "is_enabled", lambda s: True) monkeypatch.setattr(browser_agent.browser_session_import, "import_signin", lambda d, b: pytest.fail("must not re-import the same values")) try: assert await browser_agent.try_borrow_signin("acme.example", "b1", "", "") is False finally: - browser_agent.p_signin_borrowed.discard("acme.example") + browser_agent.signin_borrowed.discard("acme.example") def test_import_timeout_outlasts_the_hidden_window_warm():