From bb87148eef46f33cd70a125f627b78c88c2e4df9 Mon Sep 17 00:00:00 2001 From: Faisal01011 Date: Sat, 24 Oct 2026 12:54:59 +0530 Subject: [PATCH 1/4] Name recurring failure modes for agents (issue #465) Sections of #465 that turn silent churn into distinct, actionable errors; each follows the existing stable data.reason convention (extension daemons attach reasons, human copy lives in render_error.rs). Section 1 (partial, detection): repeated observation identity failures on the same tab inside a short window now return cdp_failed with the stable reason observe_identity_reload_loop naming likely anti-debugging protection instead of the retry churn, and never suggest a reload (the non-debugger control path itself remains a product decision; see #454). Section 3: the DSH plugin's missing-CLI error points at the plugin's bskPath option in addition to the install URL. Section 5: doctor's 0-browsers check and its waiting line distinguish "not installed" from "installed but not connected" (running window, per-profile enablement, daemon address/port, stale service worker). Section 6: borrowing a tab that already lives in an Agent Window returns invalid_params with the stable reason borrow_agent_window_tab and the user-move guidance from the reference docs, instead of bare invalid-parameters copy. Section 7 (partial): the daemon tombstones why a session id ended (idle timeout, Agent Window closed, browser disconnect) and a later dispatch surfaces a reasoned not_found instead of the generic "session not registered"; idle-stop copy explains --session-idle and that the Agent Window tab closed with the session. The UI confirmation and detach-instead-of-close asks remain product decisions. --- .../__tests__/observe-identity-loop.test.ts | 42 +++++ .../src/tools/__tests__/tabs.test.ts | 9 +- apps/extension/src/tools/observation.ts | 92 +++++++++- apps/extension/src/tools/tabs.ts | 21 ++- .../src/tools/vom/capture-coordinator.ts | 11 +- apps/extension/src/transport/types.ts | 2 + crates/bsk-cli/src/cli/doctor.rs | 26 ++- crates/bsk-cli/src/cli/render_error.rs | 95 ++++++++++ crates/bsk-cli/src/daemon/queue.rs | 77 ++++++-- crates/bsk-cli/src/daemon/sessions.rs | 170 +++++++++++++++++- crates/bsk-cli/src/daemon/start.rs | 7 + crates/bsk-cli/src/daemon/ws.rs | 1 + crates/bsk-cli/src/lib.rs | 12 ++ crates/bsk-cli/tests/per_session_queue.rs | 4 +- .../dsh-plugin-browserskill/src/runner.ts | 4 +- 15 files changed, 541 insertions(+), 32 deletions(-) create mode 100644 apps/extension/src/tools/__tests__/observe-identity-loop.test.ts diff --git a/apps/extension/src/tools/__tests__/observe-identity-loop.test.ts b/apps/extension/src/tools/__tests__/observe-identity-loop.test.ts new file mode 100644 index 000000000..c33979d35 --- /dev/null +++ b/apps/extension/src/tools/__tests__/observe-identity-loop.test.ts @@ -0,0 +1,42 @@ +// 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 with a stable reason. + +import { describe, expect, it } from "vitest"; +import { nextObserveIdentityStreak, OBSERVE_IDENTITY_LOOP_FAILURES } from "../observation"; + +describe("observe identity reload-loop streak", () => { + it("does not flag an isolated failure", () => { + const first = nextObserveIdentityStreak(undefined, 1_000, 120_000); + expect(first.isReloadLoop).toBe(false); + expect(first.streak).toEqual({ count: 1, firstAt: 1_000, lastAt: 1_000 }); + }); + + it("needs consecutive failures inside the window before naming anti-debugging", () => { + const first = nextObserveIdentityStreak(undefined, 0, 120_000); + const second = nextObserveIdentityStreak(first.streak, 3_000, 120_000); + expect(second.isReloadLoop).toBe(false); + expect(second.streak).toEqual({ count: 2, firstAt: 0, lastAt: 3_000 }); + const third = nextObserveIdentityStreak(second.streak, 6_000, 120_000); + expect(third.isReloadLoop).toBe(true); + expect(third.streak.count).toBe(OBSERVE_IDENTITY_LOOP_FAILURES); + }); + + it("failures spread wider than the window restart the streak", () => { + const first = nextObserveIdentityStreak(undefined, 0, 120_000); + const next = nextObserveIdentityStreak(first.streak, 120_001, 120_000); + expect(next.isReloadLoop).toBe(false); + expect(next.streak.count).toBe(1); + // And repeated restarts never accumulate into the list arbitrarily: + // three *far* apart failures stay three separate isolated failures. + const again = nextObserveIdentityStreak(next.streak, 130_000, 120_000); + expect(again.isReloadLoop).toBe(false); + expect(again.streak.count).toBe(2); + }); + + it("defaults the streak window to the published constant", () => { + const first = nextObserveIdentityStreak(undefined, 0); + expect(first.streak.count).toBe(1); + }); +}); diff --git a/apps/extension/src/tools/__tests__/tabs.test.ts b/apps/extension/src/tools/__tests__/tabs.test.ts index e6c882544..6d4a7d755 100644 --- a/apps/extension/src/tools/__tests__/tabs.test.ts +++ b/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 () => { diff --git a/apps/extension/src/tools/observation.ts b/apps/extension/src/tools/observation.ts index 810dc995e..7be4521cb 100644 --- a/apps/extension/src/tools/observation.ts +++ b/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,75 @@ 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 { + count: number; + firstAt: number; + lastAt: number; +} + +const observeIdentityStreaks = new Map(); + +/** 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 when it happens within `windowMs` of the + * streak's first failure; otherwise the counter restarts. + */ +export function nextObserveIdentityStreak( + previous: ObserveIdentityStreak | undefined, + failureAt: number, + windowMs: number = OBSERVE_IDENTITY_LOOP_WINDOW_MS, +): { streak: ObserveIdentityStreak; isReloadLoop: boolean } { + const continues = previous !== undefined && failureAt - previous.firstAt <= windowMs; + const count = continues ? previous!.count + 1 : 1; + const streak: ObserveIdentityStreak = continues + ? { count, firstAt: previous!.firstAt, lastAt: failureAt } + : { count: 1, firstAt: failureAt, lastAt: failureAt }; + return { streak, isReloadLoop: 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. + */ +function observeIdentityReloadLoopError( + ctx: SessionContext, + tabId: number, + err: unknown, +): RpcError | null { + const message = err instanceof Error ? err.message : String(err); + if (!message.includes(OBSERVE_IDENTITY_CHANGED)) return null; + const key = observeIdentityKey(ctx.sessionId, tabId); + const next = nextObserveIdentityStreak(observeIdentityStreaks.get(key), 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 +1177,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 +1205,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 +1238,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 +1282,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 +1314,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, 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); + } } } diff --git a/apps/extension/src/tools/tabs.ts b/apps/extension/src/tools/tabs.ts index a5ff09caa..458f2c649 100644 --- a/apps/extension/src/tools/tabs.ts +++ b/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) { diff --git a/apps/extension/src/tools/vom/capture-coordinator.ts b/apps/extension/src/tools/vom/capture-coordinator.ts index dde388ed0..adcd8acec 100644 --- a/apps/extension/src/tools/vom/capture-coordinator.ts +++ b/apps/extension/src/tools/vom/capture-coordinator.ts @@ -42,6 +42,15 @@ interface TargetBatch { 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( @@ -533,7 +542,7 @@ async function collectObservationFacts( 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])); diff --git a/apps/extension/src/transport/types.ts b/apps/extension/src/transport/types.ts index cd4e07c14..e729e5c13 100644 --- a/apps/extension/src/transport/types.ts +++ b/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" diff --git a/crates/bsk-cli/src/cli/doctor.rs b/crates/bsk-cli/src/cli/doctor.rs index f50e63d40..7e934a5aa 100644 --- a/crates/bsk-cli/src/cli/doctor.rs +++ b/crates/bsk-cli/src/cli/doctor.rs @@ -145,7 +145,11 @@ fn resolve_daemon_state(output: Output) -> DaemonState { }; if wait > Duration::ZERO && output == Output::Human { - eprintln!("waiting for browser extension to connect…"); + // Issue #465 §5: name the two possible causes up front so an + // installed-but-disconnected extension is not read as "missing". + eprintln!( + "waiting for browser extension to connect… (if it is installed: check that a window with the extension enabled is open in the expected profile and that it targets this daemon; reinstall from the store if missing)" + ); } if wait.is_zero() { @@ -671,12 +675,21 @@ fn check_extension_connected(status: Option<&StatusResult>) -> CheckResult { if browsers > 0 { CheckResult::ok(name, format!("{} browser(s) connected", browsers)) } else { + // Issue #465 §5: "0 browsers connected" is true for both "not + // installed" and "installed but not connected" (another or no + // browser window, wrong profile, a different daemon or port, a + // stale extension service worker). Spell both out so the fix + // does not require guessing which one applies. CheckResult::fail( name, "0 browsers connected", format!( - "install the extension from {EXTENSION_STORE_URL} (Chrome) \ - or {EXTENSION_STORE_URL_EDGE} (Edge) and load it in the browser" + "if the extension is not installed, install it from {EXTENSION_STORE_URL} (Chrome) \ + or {EXTENSION_STORE_URL_EDGE} (Edge); \ + if it is already installed, it is not connected to this daemon — keep a window \ + of that browser open with the extension enabled in the profile you are using \ + (the extension is per-profile), point the extension at this daemon's \ + address/port, and reload the extension if it was just updated" ), ) } @@ -1144,6 +1157,13 @@ mod m2_tests { hint.contains(EXTENSION_STORE_URL_EDGE), "hint should include Edge Add-ons URL: {hint}" ); + // Issue #465 §5: the hint must not imply "not installed" is the + // only possibility — an installed-but-disconnected extension is + // the harder case to diagnose. + assert!( + hint.contains("already installed"), + "hint should name the installed-but-disconnected case: {hint}" + ); } #[test] diff --git a/crates/bsk-cli/src/cli/render_error.rs b/crates/bsk-cli/src/cli/render_error.rs index b12122062..290a73c23 100644 --- a/crates/bsk-cli/src/cli/render_error.rs +++ b/crates/bsk-cli/src/cli/render_error.rs @@ -77,6 +77,11 @@ pub mod reason { pub const TRANSFER_OUTCOME_UNKNOWN: &str = "transfer_outcome_unknown"; pub const TRANSFER_TIMEOUT: &str = "transfer_timeout"; pub const SESSION_BUSY: &str = crate::rpc_reason::SESSION_BUSY; + pub const SESSION_IDLE_TIMEOUT: &str = crate::rpc_reason::SESSION_IDLE_TIMEOUT; + pub const SESSION_WINDOW_CLOSED: &str = crate::rpc_reason::SESSION_WINDOW_CLOSED; + pub const BROWSER_DISCONNECTED: &str = crate::rpc_reason::BROWSER_DISCONNECTED; + pub const OBSERVE_IDENTITY_RELOAD_LOOP: &str = crate::rpc_reason::OBSERVE_IDENTITY_RELOAD_LOOP; + pub const BORROW_AGENT_WINDOW_TAB: &str = crate::rpc_reason::BORROW_AGENT_WINDOW_TAB; pub const RECORD_START_PAGE_UNREACHABLE: &str = "record_start_page_unreachable"; } @@ -276,6 +281,41 @@ pub fn info_for_error(code: ErrorCode, data: Option<&serde_json::Value>) -> Rend ), ..base }, + (ErrorCode::CdpFailed, reason::OBSERVE_IDENTITY_RELOAD_LOOP) => RenderInfo { + summary: "the page kept losing observation identity — the site is likely reloading itself while the debugger is attached (anti-debugging)", + hint: Some( + "do not observe again and do not reload this tab; the site rejects the debugger attach itself (see GitHub issues #454 and #465); continue on another page or hand the task back to the user", + ), + exit_code: base.exit_code, + }, + (ErrorCode::InvalidParams, reason::BORROW_AGENT_WINDOW_TAB) => RenderInfo { + summary: "the borrow target already lives inside an Agent Window", + hint: Some( + "a tab the session did not open needs a user move to a user window first; a tab this session opened is already controlled and needs no borrow", + ), + exit_code: base.exit_code, + }, + (ErrorCode::NotFound, reason::SESSION_IDLE_TIMEOUT) => RenderInfo { + summary: "the session stopped after its idle timeout", + hint: Some( + "start a new session with `bsk session start`; run the daemon with a larger `--session-idle` (default 5m) for longer tasks; the Agent Window and any tab it opened were closed with the session", + ), + exit_code: base.exit_code, + }, + (ErrorCode::NotFound, reason::SESSION_WINDOW_CLOSED) => RenderInfo { + summary: "the session ended because the user closed the Agent Window", + hint: Some( + "start a new session; closing the Agent Window ends the session, while the window's stop button only cancels the running action", + ), + exit_code: base.exit_code, + }, + (ErrorCode::NotFound, reason::BROWSER_DISCONNECTED) => RenderInfo { + summary: "the browser was disconnected from the daemon", + hint: Some( + "wait for the extension to reconnect, then start a new session; the session and its Agent Window are gone", + ), + exit_code: base.exit_code, + }, (_, "cooperation_sync_required") => RenderInfo { summary: "shared page control or observation changed", hint: Some( @@ -1048,4 +1088,59 @@ mod tests { assert_eq!(disconnected.exit_code, 2); assert!(!disconnected.hint.unwrap().contains("protocol version")); } + + /// Issue #465: a dead session surfaces *why* it stopped instead of + /// a generic not-found, and the anti-debugging / borrow-target + /// reasons get non-retry copy instead of the `cdp_failed` / + /// `invalid_params` defaults. + #[test] + fn ended_sessions_and_recurring_failures_get_reasoned_copy() { + let idle = info_for_error( + ErrorCode::NotFound, + Some(&serde_json::json!({"reason": crate::rpc_reason::SESSION_IDLE_TIMEOUT})), + ); + assert_eq!(idle.summary, "the session stopped after its idle timeout"); + assert!(idle.hint.unwrap().contains("--session-idle")); + assert_eq!(idle.exit_code, info_for(ErrorCode::NotFound).exit_code); + + let closed = info_for_error( + ErrorCode::NotFound, + Some(&serde_json::json!({"reason": crate::rpc_reason::SESSION_WINDOW_CLOSED})), + ); + assert_eq!( + closed.summary, + "the session ended because the user closed the Agent Window" + ); + + let browser_gone = info_for_error( + ErrorCode::NotFound, + Some(&serde_json::json!({"reason": crate::rpc_reason::BROWSER_DISCONNECTED})), + ); + assert_eq!( + browser_gone.summary, + "the browser was disconnected from the daemon" + ); + + let anti_debug = info_for_error( + ErrorCode::CdpFailed, + Some(&serde_json::json!({"reason": crate::rpc_reason::OBSERVE_IDENTITY_RELOAD_LOOP})), + ); + assert!(anti_debug.summary.contains("anti-debugging")); + // The reporter explicitly asked that this failure must not + // propose reloading the tab; the hint forbids it instead. + let anti_debug_hint = anti_debug.hint.unwrap(); + assert!(anti_debug_hint.contains("do not reload")); + assert!(anti_debug_hint.contains("hand the task back")); + assert_eq!( + anti_debug.exit_code, + info_for(ErrorCode::CdpFailed).exit_code + ); + + let borrow = info_for_error( + ErrorCode::InvalidParams, + Some(&serde_json::json!({"reason": crate::rpc_reason::BORROW_AGENT_WINDOW_TAB})), + ); + assert!(borrow.summary.contains("Agent Window")); + assert!(borrow.hint.unwrap().contains("needs a user move")); + } } diff --git a/crates/bsk-cli/src/daemon/queue.rs b/crates/bsk-cli/src/daemon/queue.rs index 1d28e3743..81b3babf4 100644 --- a/crates/bsk-cli/src/daemon/queue.rs +++ b/crates/bsk-cli/src/daemon/queue.rs @@ -107,9 +107,14 @@ pub struct ToolJob { #[derive(Debug, thiserror::Error)] pub enum DispatchError { /// Session id does not have a queue (either never started or was - /// stopped concurrently with this dispatch). + /// stopped concurrently with this dispatch). `ended` carries the + /// registry's tombstone for why the session stopped (idle timeout, + /// Agent Window closed, browser disconnect), surfaced through + /// [`RpcError.data.reason`]; `None` for ids without a recorded end. #[error("session not registered or already stopped")] - SessionNotFound, + SessionNotFound { + ended: Option, + }, /// Session is draining for `session.stop`; new tool calls must not /// enter behind the queued stop request. #[error("session is stopping")] @@ -140,11 +145,19 @@ impl DispatchError { /// suitable for sending back over the IPC line. pub fn into_rpc(self) -> RpcError { match self { - DispatchError::SessionNotFound => RpcError { - code: ErrorCode::NotFound, - message: "session not registered or already stopped".into(), - data: None, - }, + DispatchError::SessionNotFound { ended } => { + let data = ended.map(|end| { + serde_json::json!({ + "reason": end.reason, + "ended_unix_ms": end.ended_unix_ms, + }) + }); + RpcError { + code: ErrorCode::NotFound, + message: "session not registered or already stopped".into(), + data, + } + } DispatchError::SessionStopping => RpcError { code: ErrorCode::Timeout, message: "session is stopping".into(), @@ -240,6 +253,17 @@ impl ToolQueueRegistry { guard.get(sid).is_some_and(|entry| entry.accepting) } + /// The session-missing [`DispatchError`] for `sid`, enriched with + /// the registry's tombstone when the id recently stopped with a + /// known reason (issue #465: idle timeout / window close / browser + /// disconnect). Without a tombstone this is the same generic + /// not-found as before. + fn session_missing(&self, sid: &SessionId) -> DispatchError { + DispatchError::SessionNotFound { + ended: self.sessions.end_info(sid), + } + } + #[cfg(test)] fn force_busy_for_tests(&self, sid: &SessionId, busy: bool) { let guard = self.queues.lock().expect("tool queue registry poisoned"); @@ -314,7 +338,7 @@ impl ToolQueueRegistry { let (sender, state) = { let guard = self.queues.lock().expect("tool queue registry poisoned"); let Some(entry) = guard.get(sid) else { - return Err(DispatchError::SessionNotFound); + return Err(self.session_missing(sid)); }; if !entry.accepting { return Err(DispatchError::SessionStopping); @@ -361,7 +385,7 @@ impl ToolQueueRegistry { pub fn activity(&self, sid: &SessionId) -> Result { let queues = self.queues.lock().expect("tool queue registry poisoned"); - let entry = queues.get(sid).ok_or(DispatchError::SessionNotFound)?; + let entry = queues.get(sid).ok_or_else(|| self.session_missing(sid))?; let state = entry.state.lock().expect("queue state poisoned"); let mut activity = state.active.clone().unwrap_or(DebugActivity { state: if state.busy { "running" } else { "idle" }.into(), @@ -421,7 +445,7 @@ impl ToolQueueRegistry { { let guard = self.queues.lock().expect("tool queue registry poisoned"); let Some(entry) = guard.get(sid) else { - return Err(DispatchError::SessionNotFound); + return Err(self.session_missing(sid)); }; if !entry.accepting { return Err(DispatchError::SessionStopping); @@ -459,7 +483,7 @@ impl ToolQueueRegistry { let (sender, state) = { let mut guard = self.queues.lock().expect("tool queue registry poisoned"); let Some(entry) = guard.get_mut(sid) else { - return Err(DispatchError::SessionNotFound); + return Err(self.session_missing(sid)); }; if !entry.accepting { return Err(DispatchError::SessionStopping); @@ -1710,6 +1734,37 @@ mod link_error_tests { } } +/// Issue #465 §7: a dispatch against an ended session id turns into a +/// `not_found` RpcError carrying the tombstoned stop reason. +#[cfg(test)] +mod ended_session_dispatch_tests { + use super::{super::sessions::SessionEnd, DispatchError}; + use crate::rpc_reason; + use bsk_protocol::ErrorCode; + + #[test] + fn ended_session_surfaces_reason_and_timestamp() { + let rpc = DispatchError::SessionNotFound { + ended: Some(SessionEnd { + reason: rpc_reason::SESSION_IDLE_TIMEOUT, + ended_unix_ms: 1_760_000_000_000, + }), + } + .into_rpc(); + assert_eq!(rpc.code, ErrorCode::NotFound); + let data = rpc.data.expect("ended session must carry data"); + assert_eq!(data["reason"], rpc_reason::SESSION_IDLE_TIMEOUT); + assert_eq!(data["ended_unix_ms"], 1_760_000_000_000i64); + } + + #[test] + fn unknown_session_defaults_to_the_plain_not_found() { + let rpc = DispatchError::SessionNotFound { ended: None }.into_rpc(); + assert_eq!(rpc.code, ErrorCode::NotFound); + assert_eq!(rpc.data, None); + } +} + fn epoch_ms() -> u64 { std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) diff --git a/crates/bsk-cli/src/daemon/sessions.rs b/crates/bsk-cli/src/daemon/sessions.rs index 5d191debf..fdd92b7e0 100644 --- a/crates/bsk-cli/src/daemon/sessions.rs +++ b/crates/bsk-cli/src/daemon/sessions.rs @@ -75,6 +75,25 @@ pub struct SessionRegistry { last_activity: Mutex>, /// Managed creates whose original extension operation has not settled. starting: Mutex>, + /// Tombstones for recently ended session ids: why they stopped. + /// Read by [`super::queue::ToolQueueRegistry`] when a dispatch hits a + /// dead id, so the CLI can say "idle timeout" / "Agent Window closed" + /// instead of a bare not-found (issue #465 §7). + ended: Mutex>, +} + +/// Upper bound for the [`SessionRegistry::ended`] tombstone map. Session +/// ids are drawn from the small `26^4` space; the cap plus expiry-on-reuse +/// keeps a long-lived daemon from growing it without limit. +const SESSION_END_TOMBSTONE_CAP: usize = 256; + +/// Why a session id was removed. `reason` is a stable wire string +/// (`RpcError.data.reason`); unknown values fall back to code-based +/// copy on older peers. +#[derive(Debug, Clone)] +pub struct SessionEnd { + pub reason: &'static str, + pub ended_unix_ms: i64, } #[derive(Debug)] @@ -132,6 +151,12 @@ impl SessionRegistry { let session_id = session.id.clone(); let mut sessions = self.inner.lock().expect("session registry poisoned"); sessions.insert(session_id.clone(), session); + // The id is alive again: any tombstone from an earlier same-id + // session must stop influencing later dispatch errors. + self.ended + .lock() + .expect("session end registry poisoned") + .remove(&session_id); self.last_activity .lock() .expect("session activity registry poisoned") @@ -161,7 +186,16 @@ impl SessionRegistry { let mut guard = self.inner.lock().expect("session registry poisoned"); for _ in 0..max_attempts { let candidate = SessionId::random(); - if guard.contains_key(&candidate) { + // A tombstoned id still means "recently ended": reusing it + // would immediately look like a crash/stop to clients that + // dispatch against the old id (see `SessionEnd`). + if guard.contains_key(&candidate) + || self + .ended + .lock() + .expect("session end registry poisoned") + .contains_key(&candidate) + { continue; } guard.insert( @@ -237,6 +271,49 @@ impl SessionRegistry { removed } + /// Record why `id` is no longer registered. Read by the dispatch + /// queue when a later tool call hits the dead id (see + /// [`super::queue::ToolQueueRegistry`]) and rendered by the CLI as + /// a distinct "session ended (why)" error instead of a generic + /// not-found. Also removed automatically when the id is reused via + /// [`Self::insert`] or [`Self::reserve_id`]. + pub fn remember_end(&self, id: &SessionId, reason: &'static str) { + let mut guard = self.ended.lock().expect("session end registry poisoned"); + if guard.len() >= SESSION_END_TOMBSTONE_CAP && !guard.contains_key(id) { + // Evict the oldest tombstone so the map stays bounded; + // losing the oldest entry only weakens a very stale hint. + if let Some(oldest) = guard + .iter() + .min_by_key(|(_, end)| end.ended_unix_ms) + .map(|(id, _)| id.clone()) + { + guard.remove(&oldest); + } + } + guard.insert( + id.clone(), + SessionEnd { + reason, + ended_unix_ms: now_ms(), + }, + ); + } + + /// [`Self::remove`] plus [`Self::remember_end`]. + pub fn remove_with_end_reason(&self, id: &SessionId, reason: &'static str) -> Option { + self.remember_end(id, reason); + self.remove(id) + } + + /// The recorded end for `id`, when one still exists. + pub fn end_info(&self, id: &SessionId) -> Option { + self.ended + .lock() + .expect("session end registry poisoned") + .get(id) + .cloned() + } + /// Accept preference updates only from the browser owning the session. pub fn update_interaction( &self, @@ -1029,7 +1106,7 @@ pub async fn stop_session( queues.reopen(session_id); Err(StopSessionError::ExtensionError(err)) } - Err(DispatchError::SessionNotFound) => Err(StopSessionError::NotFound), + Err(DispatchError::SessionNotFound { .. }) => Err(StopSessionError::NotFound), Err(DispatchError::SessionStopping) => Err(StopSessionError::Stopping), Err(DispatchError::SessionBusy) => Err(StopSessionError::SessionBusy), Err(DispatchError::Timeout) => { @@ -1045,17 +1122,24 @@ pub async fn stop_session( /// Drop a session locally (no round-trip) — used when the extension has /// already torn down its side (e.g. window closed by user). Also drops -/// the queue so the worker exits. +/// the queue so the worker exits. `end_reason` tombstones why the id is +/// gone so later dispatches say so (issue #465 §7). pub fn forget_session( sessions: &Arc, queues: &Arc, interrupts: &Arc, session_id: &SessionId, + end_reason: Option<&'static str>, ) -> bool { if let Some(audit) = &sessions.audit { audit.session_ended(&session_id.0, "interrupted"); } - let removed = sessions.remove(session_id).is_some(); + let removed = match end_reason { + Some(reason) => sessions + .remove_with_end_reason(session_id, reason) + .is_some(), + None => sessions.remove(session_id).is_some(), + }; if removed { drop_session_local(queues, interrupts, session_id); } @@ -1348,3 +1432,81 @@ mod link_tests { } } } + +/// Issue #465 §7: when a session stops, dispatches against the dead id +/// must say why it stopped — until the id is reused by a new session. +#[cfg(test)] +mod ended_tombstone_tests { + use super::{super::browsers::BrowserId, Session, SessionId, SessionRegistry}; + use crate::rpc_reason; + + fn live_session(id: &str) -> Session { + Session { + interaction: None, + id: SessionId(id.into()), + browser_id: BrowserId("browser".into()), + agent_window_id: Some(7), + created_at_ms: 1, + } + } + + #[test] + fn tombstone_records_the_stop_reason_until_the_id_is_reused() { + let registry = SessionRegistry::new(); + registry.insert(live_session("abcd")); + + let removed = registry + .remove_with_end_reason(&SessionId("abcd".into()), rpc_reason::SESSION_IDLE_TIMEOUT); + assert!(removed.is_some()); + let end = registry + .end_info(&SessionId("abcd".into())) + .expect("ended session must keep a tombstone"); + assert_eq!(end.reason, rpc_reason::SESSION_IDLE_TIMEOUT); + assert!(end.ended_unix_ms > 0); + + // A new session reusing the id clears the tombstone: later + // dispatch errors must describe the *current* session. + registry.insert(live_session("abcd")); + assert!(registry.end_info(&SessionId("abcd".into())).is_none()); + registry + .remove(&SessionId("abcd".into())) + .expect("live session removed"); + assert!( + registry.end_info(&SessionId("abcd".into())).is_none(), + "remove without a reason must not invent a tombstone" + ); + } + + #[test] + fn remember_end_only_tombstones_stop_paths_that_opt_in() { + let registry = SessionRegistry::new(); + registry.insert(live_session("woff")); + registry.remember_end(&SessionId("woff".into()), rpc_reason::SESSION_WINDOW_CLOSED); + assert_eq!( + registry + .end_info(&SessionId("woff".into())) + .expect("tombstone recorded") + .reason, + rpc_reason::SESSION_WINDOW_CLOSED + ); + registry.remove(&SessionId("woff".into())); + } + + #[test] + fn tombstones_stay_bounded() { + let registry = SessionRegistry::new(); + for i in 0..(256 + 32) { + let id = SessionId(format!("a{i:0>3}")); + registry.remember_end(&id, rpc_reason::BROWSER_DISCONNECTED); + } + let guarded = registry + .ended + .lock() + .expect("session end registry poisoned"); + assert!( + guarded.len() <= 256, + "tombstone map must stay bounded, got {}", + guarded.len() + ); + } +} diff --git a/crates/bsk-cli/src/daemon/start.rs b/crates/bsk-cli/src/daemon/start.rs index da1b86a25..57415820e 100644 --- a/crates/bsk-cli/src/daemon/start.rs +++ b/crates/bsk-cli/src/daemon/start.rs @@ -836,6 +836,9 @@ pub(crate) fn spawn_browser_liveness_reaper( state.tool_queues.remove(&s.id); state.session_interrupts.drop_session(&s.id); state.transfers.release_session(&s.id.0); + state + .sessions + .remember_end(&s.id, crate::rpc_reason::BROWSER_DISCONNECTED); debug!(session = %s.id, "purged session on browser liveness timeout"); } } @@ -883,6 +886,9 @@ pub(crate) fn spawn_session_idle_reaper(state: Arc) -> tokio::task: { Ok(_) => { state.transfers.release_session(&session_id.0); + state + .sessions + .remember_end(&session_id, crate::rpc_reason::SESSION_IDLE_TIMEOUT); info!(session = %session_id, "idle session stopped"); } Err(StopSessionError::SessionBusy | StopSessionError::Stopping) => { @@ -894,6 +900,7 @@ pub(crate) fn spawn_session_idle_reaper(state: Arc) -> tokio::task: &state.tool_queues, &state.session_interrupts, &session_id, + None, ); state.transfers.release_session(&session_id.0); } diff --git a/crates/bsk-cli/src/daemon/ws.rs b/crates/bsk-cli/src/daemon/ws.rs index 6bd895c62..e44a47431 100644 --- a/crates/bsk-cli/src/daemon/ws.rs +++ b/crates/bsk-cli/src/daemon/ws.rs @@ -669,6 +669,7 @@ fn handle_session_window_closed( &state.tool_queues, &state.session_interrupts, &session_id, + Some(crate::rpc_reason::SESSION_WINDOW_CLOSED), ) { state.transfers.release_session(&session_id.0); state.dialog_operations.remove_session(&session_id.0); diff --git a/crates/bsk-cli/src/lib.rs b/crates/bsk-cli/src/lib.rs index 0b9e23881..0b92ad30e 100644 --- a/crates/bsk-cli/src/lib.rs +++ b/crates/bsk-cli/src/lib.rs @@ -5,6 +5,18 @@ pub mod daemon; pub mod ipc_client; pub mod rpc_reason { pub const SESSION_BUSY: &str = "session_busy"; + /// Stable `RpcError.data.reason` values the daemon attaches when a + /// dispatch hits a session id that already ended, so the CLI can + /// render why it vanished instead of a generic not-found. + pub const SESSION_IDLE_TIMEOUT: &str = "session_idle_timeout"; + pub const SESSION_WINDOW_CLOSED: &str = "session_window_closed"; + pub const BROWSER_DISCONNECTED: &str = "browser_disconnected"; + /// Extension-side: repeated observation identity failures on the + /// same tab in a short window — the site reloads itself when the + /// debugger attaches (anti-debugging), see issues #454 / #465. + pub const OBSERVE_IDENTITY_RELOAD_LOOP: &str = "observe_identity_reload_loop"; + /// Extension-side: borrow target already lives in an Agent Window. + pub const BORROW_AGENT_WINDOW_TAB: &str = "borrow_agent_window_tab"; } pub mod skill_install; diff --git a/crates/bsk-cli/tests/per_session_queue.rs b/crates/bsk-cli/tests/per_session_queue.rs index 1493997c3..80190d931 100644 --- a/crates/bsk-cli/tests/per_session_queue.rs +++ b/crates/bsk-cli/tests/per_session_queue.rs @@ -683,11 +683,11 @@ async fn dispatch_returns_session_not_found_for_unknown_session() { ) .await; match result { - Err(DispatchError::SessionNotFound) => {} + Err(DispatchError::SessionNotFound { .. }) => {} other => panic!("expected SessionNotFound, got {other:?}"), } // And the surfaced RpcError carries `not_found`. - let rpc = DispatchError::SessionNotFound.into_rpc(); + let rpc = DispatchError::SessionNotFound { ended: None }.into_rpc(); assert_eq!(rpc.code, ErrorCode::NotFound); handle.shutdown().await; } diff --git a/packages/dsh-plugin-browserskill/src/runner.ts b/packages/dsh-plugin-browserskill/src/runner.ts index fcda9930e..30c1a3493 100644 --- a/packages/dsh-plugin-browserskill/src/runner.ts +++ b/packages/dsh-plugin-browserskill/src/runner.ts @@ -378,7 +378,9 @@ export function bskInstallMessage(bskPath: string): string { return ( `the bsk CLI ("${bskPath}") was not found. BrowserSkill must be installed and on PATH ` + "for browser tools to work — install it from https://github.com/Tencent/BrowserSkill " + - "(see the README install script or `cargo install`), then retry." + "(see the README install script or `cargo install`), then retry. " + + `If bsk is installed at a custom location, set the plugin's \`bskPath\` option ` + + `(currently "${bskPath}") instead of relying on PATH.` ); } From 5f00fd7852e76a69dde67fdda517eff9218de656 Mon Sep 17 00:00:00 2001 From: Faisal01011 Date: Sun, 25 Oct 2026 18:50:32 +0530 Subject: [PATCH 2/4] Address maintainer review of #487: scope the streak per page, share the purge path Review comments by @iuyo5678 (both P2, inline): 1. The observation identity streak keyed only by session+tab carried over between unrelated sites: one transient failure on each of three sites in 120s produced the anti-debugging diagnosis on the third. The streak entry now carries the target URL and only continues on the same page; a navigation or an expired window restarts the count (repeated reloads of the same page are still detected). Rule-level regression tests cover navigating different sites and a genuine loop on the new site after a switch. 2. browser_disconnected was only tombstoned on the liveness-timeout purge. The reconnect-time stale purge and the ordinary socket disconnect cleanup now go through the shared DaemonState::purge_sessions_of_browser (reason + note), and the existing window-close, disconnect and same-instance reconnect integration scenarios assert the surfaced data.reason on a later dispatch. --- .../__tests__/observe-identity-loop.test.ts | 81 +++++++++++++++---- apps/extension/src/tools/observation.ts | 29 ++++--- crates/bsk-cli/src/daemon/start.rs | 14 ++-- crates/bsk-cli/src/daemon/state.rs | 28 ++++++- crates/bsk-cli/src/daemon/ws.rs | 24 +++--- crates/bsk-cli/tests/sessions_ipc.rs | 64 +++++++++++++++ 6 files changed, 193 insertions(+), 47 deletions(-) diff --git a/apps/extension/src/tools/__tests__/observe-identity-loop.test.ts b/apps/extension/src/tools/__tests__/observe-identity-loop.test.ts index c33979d35..bb2e15aed 100644 --- a/apps/extension/src/tools/__tests__/observe-identity-loop.test.ts +++ b/apps/extension/src/tools/__tests__/observe-identity-loop.test.ts @@ -1,42 +1,95 @@ // 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 with a stable reason. +// 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 } 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, 1_000, 120_000); + const first = nextObserveIdentityStreak(undefined, SITE_A, 1_000, 120_000); expect(first.isReloadLoop).toBe(false); - expect(first.streak).toEqual({ count: 1, firstAt: 1_000, lastAt: 1_000 }); + expect(first.streak).toEqual({ + url: SITE_A, + count: 1, + firstAt: 1_000, + lastAt: 1_000, + }); }); - it("needs consecutive failures inside the window before naming anti-debugging", () => { - const first = nextObserveIdentityStreak(undefined, 0, 120_000); - const second = nextObserveIdentityStreak(first.streak, 3_000, 120_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({ count: 2, firstAt: 0, lastAt: 3_000 }); - const third = nextObserveIdentityStreak(second.streak, 6_000, 120_000); + 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, 0, 120_000); - const next = nextObserveIdentityStreak(first.streak, 120_001, 120_000); + 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 repeated restarts never accumulate into the list arbitrarily: - // three *far* apart failures stay three separate isolated failures. - const again = nextObserveIdentityStreak(next.streak, 130_000, 120_000); + // 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, 0); + 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"); + }); }); diff --git a/apps/extension/src/tools/observation.ts b/apps/extension/src/tools/observation.ts index 7be4521cb..e2d0879a3 100644 --- a/apps/extension/src/tools/observation.ts +++ b/apps/extension/src/tools/observation.ts @@ -1092,6 +1092,7 @@ async function buildVomObservation( // --------------------------------------------------------------------------- interface ObserveIdentityStreak { + url: string; count: number; firstAt: number; lastAt: number; @@ -1112,20 +1113,29 @@ function observeIdentityKey(sessionId: string, tabId: number): string { /** * Pure rule so the streak logic is testable without a browser: a failure - * joins the previous streak when it happens within `windowMs` of the - * streak's first failure; otherwise the counter restarts. + * 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 && failureAt - previous.firstAt <= windowMs; - const count = continues ? previous!.count + 1 : 1; + const continues = + previous !== undefined && previous.url === url && failureAt - previous.firstAt <= windowMs; const streak: ObserveIdentityStreak = continues - ? { count, firstAt: previous!.firstAt, lastAt: failureAt } - : { count: 1, firstAt: failureAt, lastAt: failureAt }; - return { streak, isReloadLoop: count >= OBSERVE_IDENTITY_LOOP_FAILURES }; + ? { + 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 }; } /** @@ -1136,12 +1146,13 @@ export function nextObserveIdentityStreak( function observeIdentityReloadLoopError( ctx: SessionContext, tabId: number, + tabUrl: string, err: unknown, ): RpcError | null { const message = err instanceof Error ? err.message : String(err); if (!message.includes(OBSERVE_IDENTITY_CHANGED)) return null; const key = observeIdentityKey(ctx.sessionId, tabId); - const next = nextObserveIdentityStreak(observeIdentityStreaks.get(key), Date.now()); + const next = nextObserveIdentityStreak(observeIdentityStreaks.get(key), tabUrl, Date.now()); observeIdentityStreaks.set(key, next.streak); if (!next.isReloadLoop) return null; return rpcError( @@ -1314,7 +1325,7 @@ 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, err); + const identityLoopError = observeIdentityReloadLoopError(ctx, target.tabId, target.url, err); if (identityLoopError) return identityLoopError; return { code: "cdp_failed", diff --git a/crates/bsk-cli/src/daemon/start.rs b/crates/bsk-cli/src/daemon/start.rs index 57415820e..f58df4571 100644 --- a/crates/bsk-cli/src/daemon/start.rs +++ b/crates/bsk-cli/src/daemon/start.rs @@ -832,15 +832,11 @@ pub(crate) fn spawn_browser_liveness_reaper( idle_secs = client.idle_for().as_secs(), "reaping unresponsive browser (no heartbeat within liveness window)" ); - for s in state.sessions.purge_browser(&client.id) { - state.tool_queues.remove(&s.id); - state.session_interrupts.drop_session(&s.id); - state.transfers.release_session(&s.id.0); - state - .sessions - .remember_end(&s.id, crate::rpc_reason::BROWSER_DISCONNECTED); - debug!(session = %s.id, "purged session on browser liveness timeout"); - } + state.purge_sessions_of_browser( + &client.id, + crate::rpc_reason::BROWSER_DISCONNECTED, + "purged session on browser liveness timeout", + ); } } } diff --git a/crates/bsk-cli/src/daemon/state.rs b/crates/bsk-cli/src/daemon/state.rs index fa4b1fec0..056fc39b1 100644 --- a/crates/bsk-cli/src/daemon/state.rs +++ b/crates/bsk-cli/src/daemon/state.rs @@ -6,7 +6,7 @@ use std::sync::Arc; use tokio::task::JoinHandle; use super::abort::AbortRegistry; -use super::browsers::BrowserRegistry; +use super::browsers::{BrowserId, BrowserRegistry}; use super::file_transfer::TransferRegistry; use super::inflight::ToolInflightRegistry; use super::ipc::IpcHandle; @@ -60,6 +60,32 @@ pub struct DaemonState { pub transfers: Arc, } +impl DaemonState { + /// Forget every session owned by `browser_id`: drop its queue and + /// interrupt state, release its transfers, clear its dialog + /// operations, and tombstone why the ids ended so later dispatches + /// answer "browser disconnected" instead of a bare not-found. + /// + /// Shared by every browser-driven purge (maintainer review of + /// issue #465): the reconnect-time stale purge, the ordinary socket + /// cleanup, and the liveness-timeout reaper. + pub fn purge_sessions_of_browser( + &self, + browser_id: &BrowserId, + end_reason: &'static str, + note: &'static str, + ) { + for session in self.sessions.purge_browser(browser_id) { + self.tool_queues.remove(&session.id); + self.session_interrupts.drop_session(&session.id); + self.transfers.release_session(&session.id.0); + self.dialog_operations.remove_session(&session.id.0); + self.sessions.remember_end(&session.id, end_reason); + tracing::debug!(session = %session.id, note); + } + } +} + impl DaemonState { pub fn new(config: DaemonConfig) -> Self { if crate::cli::human_loop::legacy_help_override_requested() { diff --git a/crates/bsk-cli/src/daemon/ws.rs b/crates/bsk-cli/src/daemon/ws.rs index e44a47431..d30eca820 100644 --- a/crates/bsk-cli/src/daemon/ws.rs +++ b/crates/bsk-cli/src/daemon/ws.rs @@ -316,13 +316,11 @@ pub(super) async fn drive_connection(); let generation = super::browsers::next_browser_generation(); let connected_at_ms = std::time::SystemTime::now() @@ -454,13 +452,11 @@ pub(super) async fn drive_connection Date: Sun, 25 Oct 2026 20:31:55 +0530 Subject: [PATCH 3/4] Support the optional target URL in the identity streak (review P1) ResolvedTargetTab.url is string | undefined, so passing it to the new tabUrl parameter failed the extension type check (frontend CI: pnpm --filter @browser-skill/extension compile). 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, exactly as suggested in review. The helper is exported and unit-tested for the unavailable-URL cases. --- .../__tests__/observe-identity-loop.test.ts | 48 ++++++++++++++++++- apps/extension/src/tools/observation.ts | 10 +++- 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/apps/extension/src/tools/__tests__/observe-identity-loop.test.ts b/apps/extension/src/tools/__tests__/observe-identity-loop.test.ts index bb2e15aed..ec8533f25 100644 --- a/apps/extension/src/tools/__tests__/observe-identity-loop.test.ts +++ b/apps/extension/src/tools/__tests__/observe-identity-loop.test.ts @@ -7,7 +7,11 @@ // third site. import { describe, expect, it } from "vitest"; -import { nextObserveIdentityStreak, OBSERVE_IDENTITY_LOOP_FAILURES } from "../observation"; +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"; @@ -93,3 +97,45 @@ describe("observe identity reload-loop streak", () => { 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[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(); + }); +}); diff --git a/apps/extension/src/tools/observation.ts b/apps/extension/src/tools/observation.ts index e2d0879a3..03fae2e65 100644 --- a/apps/extension/src/tools/observation.ts +++ b/apps/extension/src/tools/observation.ts @@ -1142,15 +1142,21 @@ export function nextObserveIdentityStreak( * 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. */ -function observeIdentityReloadLoopError( +export function observeIdentityReloadLoopError( ctx: SessionContext, tabId: number, - tabUrl: string, + 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); From 3ffa3dd853600c23a321746b04426fcf308fb39b Mon Sep 17 00:00:00 2001 From: Zhang GH Date: Tue, 27 Oct 2026 10:29:34 +0800 Subject: [PATCH 4/4] test: use expect_err in session lifecycle assertions --- crates/bsk-cli/tests/sessions_ipc.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/crates/bsk-cli/tests/sessions_ipc.rs b/crates/bsk-cli/tests/sessions_ipc.rs index 5a07169a7..9e3e31bda 100644 --- a/crates/bsk-cli/tests/sessions_ipc.rs +++ b/crates/bsk-cli/tests/sessions_ipc.rs @@ -1212,8 +1212,7 @@ async fn session_window_closed_event_purges_session() { None, ) .await - .err() - .expect("the dead session must reject dispatches") + .expect_err("the dead session must reject dispatches") .into_rpc(); assert_eq!(err.code, bsk_protocol::ErrorCode::NotFound); assert_eq!( @@ -1260,8 +1259,7 @@ async fn browser_disconnect_purges_sessions() { None, ) .await - .err() - .expect("the dead session must reject dispatches") + .expect_err("the dead session must reject dispatches") .into_rpc(); assert_eq!(err.code, bsk_protocol::ErrorCode::NotFound); assert_eq!( @@ -1435,8 +1433,7 @@ async fn reconnect_with_same_instance_id_purges_stale_sessions_but_keeps_new_bro None, ) .await - .err() - .expect("the dead session must reject dispatches") + .expect_err("the dead session must reject dispatches") .into_rpc(); assert_eq!(err.code, bsk_protocol::ErrorCode::NotFound); assert_eq!(