From d45e2b3731c417bdf060576410e9e4d7594cd6e9 Mon Sep 17 00:00:00 2001 From: ciregenz Date: Mon, 17 Aug 2026 15:48:49 -0700 Subject: [PATCH] [eric] browser: a failed navigation now fails loudly; a reused card that cannot move is replaced instead of answering from the old page --- backend/apps/agents/browser/browser_agent.py | 44 ++++++++++++++++--- frontend/src/shared/browserCommandHandler.ts | 29 +++++++++++- frontend/src/shared/navigationOutcome.test.ts | 41 +++++++++++++++++ frontend/src/shared/navigationOutcome.ts | 18 ++++++++ 4 files changed, 123 insertions(+), 9 deletions(-) create mode 100644 frontend/src/shared/navigationOutcome.test.ts create mode 100644 frontend/src/shared/navigationOutcome.ts diff --git a/backend/apps/agents/browser/browser_agent.py b/backend/apps/agents/browser/browser_agent.py index b3f4ae0f..85c62bc5 100644 --- a/backend/apps/agents/browser/browser_agent.py +++ b/backend/apps/agents/browser/browser_agent.py @@ -1150,8 +1150,25 @@ async def run_browser_agent( nav_result = await execute_browser_tool( "BrowserNavigate", {"url": initial_url}, browser_id, tab_id, ) - logger.info(f"Browser agent {session_id}: navigated to {initial_url}: {nav_result.get('text', nav_result.get('error', ''))}") - preloaded_perception, current_url, preloaded_reads = await p_perceive(initial_url) + p_nav_err = str(nav_result.get("error") or "") if isinstance(nav_result, dict) else "" + if p_nav_err: + # One retry: right after spawn the webview may still be mounting, and that transient must not become a run on the wrong page. + nav_result = await execute_browser_tool( + "BrowserNavigate", {"url": initial_url}, browser_id, tab_id, + ) + p_nav_err = str(nav_result.get("error") or "") if isinstance(nav_result, dict) else "" + if p_nav_err: + # This used to log "navigated to" and run anyway, which is how an agent asked for /wiki/Helium answered about Argon (Haik's field report). + logger.warning(f"Browser agent {session_id}: navigation to {initial_url} FAILED: {p_nav_err[:200]}") + preloaded_perception, current_url, preloaded_reads = await p_perceive("") + preloaded_perception = ( + f"\n\n[IMPORTANT: opening {initial_url} FAILED ({p_nav_err[:200]}). " + f"The page actually loaded right now is {current_url or 'unknown'}. Never answer from this wrong page: " + "retry BrowserNavigate yourself first, and if it still fails, report the navigation failure as your result.]" + ) + preloaded_perception + else: + logger.info(f"Browser agent {session_id}: navigated to {initial_url}: {nav_result.get('text', '')}") + preloaded_perception, current_url, preloaded_reads = await p_perceive(initial_url) elif not p_resumed: # Fresh task on an existing card: perceive the current page to learn its host (for replay) and front-load turn 1 (this path used to start cold). preloaded_perception, current_url, preloaded_reads = await p_perceive("") @@ -3635,11 +3652,24 @@ async def run_browser_agents( ) if url: # a retry starts from the task's entry URL, never the failed attempt's leftover page state + p_nav: dict = {} try: - await execute_browser_tool("BrowserNavigate", {"url": url}, browser_id) - except Exception: - pass - elif os.environ.get("OSW_PRELUDE_TRIM", "1") != "0": + p_nav = await execute_browser_tool("BrowserNavigate", {"url": url}, browser_id) + except Exception as p_nav_exc: + p_nav = {"error": f"{type(p_nav_exc).__name__}: {p_nav_exc}"} + if isinstance(p_nav, dict) and p_nav.get("error"): + # A reused card that cannot move would answer confidently about the OLD page; swap it for a fresh card instead of running on a lie (Haik's field report). + logger.warning( + f"[browser-agent] reused card {browser_id} refused navigation to {url[:80]} " + f"({str(p_nav['error'])[:140]}); evicting it and spawning a fresh card") + DEAD_CARDS.add(browser_id) + await evict_dead_card(dashboard_id, browser_id) + async with p_card_pick_lock: + browser_id = await p_create_browser_card(dashboard_id, url or entry_url, parent_session_id) + ACTIVE_AGENT_CARDS.add(browser_id) + reused = False + task_def.pop("p_reused_note", None) + if not reused and os.environ.get("OSW_PRELUDE_TRIM", "1") != "0": # Poll until the mounting card serves real page text instead of a blind 2s; capped, so the worst case is the old wait plus one probe. p_mount_t0 = time.monotonic() p_mounted = False @@ -3668,7 +3698,7 @@ async def run_browser_agents( await asyncio.sleep(0.4) logger.info(f"[browser-spawn-ack] {browser_id} rebroadcast after silent mount; alive={p_mounted}") logger.info(f"[browser-cold] mount poll {int((time.monotonic() - p_mount_t0) * 1000)}ms for {browser_id}") - else: + elif not reused: await asyncio.sleep(2.0) elif browser_id and not app_mode: ACTIVE_AGENT_CARDS.add(browser_id) diff --git a/frontend/src/shared/browserCommandHandler.ts b/frontend/src/shared/browserCommandHandler.ts index b0126c0f..753f5aa0 100644 --- a/frontend/src/shared/browserCommandHandler.ts +++ b/frontend/src/shared/browserCommandHandler.ts @@ -11,6 +11,7 @@ import { resolveInput } from './resolveUrl'; import { rankAndCapInteractives, type RankItem } from './interactiveRanking'; import { shouldStopWaiting, SETTLE_POLL_MS, settleProbeJs } from './browserSettle'; import { unwrapCdpEval } from './cdpEval'; +import { navigationOutcome, sameDoc } from './navigationOutcome'; import { typeChars, type TypedKeys } from './typeChars'; let initialized = false; @@ -369,6 +370,8 @@ async function handleNavigate(wv: BrowserWebview, params: Record): const raw = params.url as string; if (!raw) return { error: 'url parameter is required' }; const url = resolveInput(raw); + let before = ''; + try { before = wv.getURL(); } catch { /* mid-mount; landed check degrades gracefully */ } // loadURL resolves only on the full 'load' event, which heavy SPAs (LinkedIn, Gmail) hold open with persistent connections long past our timeout even though the page is usable in a second. Return the moment the DOM is ready and let the agent's next wait settle the rest, the way a person clicks before every background request has finished. let removeReady = () => {}; const domReady = new Promise((resolve) => { @@ -376,9 +379,10 @@ async function handleNavigate(wv: BrowserWebview, params: Record): wv.addEventListener('dom-ready', onReady, { once: true }); removeReady = () => wv.removeEventListener('dom-ready', onReady); }); + let aborted = false; const fullyLoaded = wv.loadURL(url).catch((err: any) => { - // A superseded navigation aborts the old load; that's normal, not a failure. - if (err?.message?.includes('ERR_ABORTED')) return; + // ERR_ABORTED can be a benign supersede OR a nav the guest refused outright; the landed-URL check below tells them apart. + if (err?.message?.includes('ERR_ABORTED')) { aborted = true; return; } throw err; }); fullyLoaded.catch(() => {}); // a late load failure shouldn't throw once dom-ready returned @@ -400,6 +404,27 @@ async function handleNavigate(wv: BrowserWebview, params: Record): data_document: true, }; } + // "Navigated to" used to be unconditional, so a wedged guest whose load was silently aborted reported success while still parked on the old page (Haik's 2-of-4 repro). Only claim success once the document provably moved. + if (before && !sameDoc(url, before)) { + let landed = before; + const settleDeadline = Date.now() + (aborted ? 1500 : 400); + for (;;) { + try { landed = wv.getURL(); } catch { /* keep last */ } + if (landed && !sameDoc(landed, before)) break; + if (Date.now() >= settleDeadline) break; + await new Promise((r) => setTimeout(r, 150)); + } + const verdict = navigationOutcome(url, before, landed); + if (verdict.kind === 'stuck') { + return { + error: `Navigation to ${url} did NOT happen; the page is still on ${before}. The webview may be wedged: retry once, and if it fails again report the failure instead of answering from the current page.`, + url: verdict.url, + }; + } + if (verdict.kind === 'redirected') { + return { text: `Navigated to ${landed} (redirected from requested ${url})`, url: landed }; + } + } // Route-count is sampled on the next READ (handleGetText), not here: at navigate-return the SPA's XHRs haven't fired yet, so this would always be ~0. return { text: `Navigated to ${url}`, url }; } diff --git a/frontend/src/shared/navigationOutcome.test.ts b/frontend/src/shared/navigationOutcome.test.ts new file mode 100644 index 00000000..7f588cf6 --- /dev/null +++ b/frontend/src/shared/navigationOutcome.test.ts @@ -0,0 +1,41 @@ +// Haik's repro: 2 of 4 navigations never moved yet all 4 reported OK. These pin the verdict table +// so a nav that leaves the document parked on the old URL can only ever read as a failure. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { navigationOutcome, sameDoc } from './navigationOutcome'; + +test('a landed URL equal to the old page is stuck, never ok', () => { + const v = navigationOutcome('https://en.wikipedia.org/wiki/Helium', 'https://en.wikipedia.org/wiki/Argon', 'https://en.wikipedia.org/wiki/Argon'); + assert.equal(v.kind, 'stuck'); +}); + +test('an empty landed URL (webview gave nothing back) is stuck', () => { + const v = navigationOutcome('https://a.com/x', 'https://a.com/y', ''); + assert.equal(v.kind, 'stuck'); +}); + +test('landing on the requested URL is ok', () => { + const v = navigationOutcome('https://a.com/x', 'https://a.com/y', 'https://a.com/x'); + assert.deepEqual(v, { kind: 'ok', url: 'https://a.com/x' }); +}); + +test('landing elsewhere is reported as a redirect, not a silent ok', () => { + const v = navigationOutcome('https://site.com/settings', 'https://site.com/home', 'https://site.com/login?next=settings'); + assert.equal(v.kind, 'redirected'); + assert.equal(v.url, 'https://site.com/login?next=settings'); +}); + +test('re-navigating to the current page is ok (reload, no moved-document requirement)', () => { + const v = navigationOutcome('https://a.com/x', 'https://a.com/x/', 'https://a.com/x'); + assert.equal(v.kind, 'ok'); +}); + +test('no before URL (mid-mount card) degrades to ok rather than a false failure', () => { + const v = navigationOutcome('https://a.com/x', '', ''); + assert.deepEqual(v, { kind: 'ok', url: 'https://a.com/x' }); +}); + +test('sameDoc ignores only the trailing slash', () => { + assert.equal(sameDoc('https://a.com', 'https://a.com/'), true); + assert.equal(sameDoc('https://a.com/x', 'https://a.com/x#frag'), false); +}); diff --git a/frontend/src/shared/navigationOutcome.ts b/frontend/src/shared/navigationOutcome.ts new file mode 100644 index 00000000..b54aed89 --- /dev/null +++ b/frontend/src/shared/navigationOutcome.ts @@ -0,0 +1,18 @@ +// Pure verdict for one navigation attempt, decided from URLs alone, so "Navigated to" can never +// again be printed while the document is provably still parked on the old page (Haik's field report). +export type NavOutcome = + | { kind: 'ok'; url: string } + | { kind: 'redirected'; url: string; requested: string } + | { kind: 'stuck'; url: string; requested: string }; + +// Trailing-slash-insensitive, so loadURL("https://x.com") over "https://x.com/" doesn't read as "never moved". +export function sameDoc(a: string, b: string): boolean { + return a.replace(/\/$/, '') === b.replace(/\/$/, ''); +} + +export function navigationOutcome(requested: string, before: string, landed: string): NavOutcome { + if (!before || sameDoc(requested, before)) return { kind: 'ok', url: landed || requested }; + if (!landed || sameDoc(landed, before)) return { kind: 'stuck', url: landed || before, requested }; + if (!sameDoc(landed, requested)) return { kind: 'redirected', url: landed, requested }; + return { kind: 'ok', url: landed }; +}