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 00000000..ec8533f2 --- /dev/null +++ b/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[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/__tests__/tabs.test.ts b/apps/extension/src/tools/__tests__/tabs.test.ts index e6c88254..6d4a7d75 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 810dc995..03fae2e6 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,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(); + +/** 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); + } } } diff --git a/apps/extension/src/tools/tabs.ts b/apps/extension/src/tools/tabs.ts index a5ff09ca..458f2c64 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 dde388ed..adcd8ace 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 cd4e07c1..e729e5c1 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 f50e63d4..7e934a5a 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 b1212206..290a73c2 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 1d28e374..81b3babf 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 5d191deb..fdd92b7e 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 da1b86a2..f58df457 100644 --- a/crates/bsk-cli/src/daemon/start.rs +++ b/crates/bsk-cli/src/daemon/start.rs @@ -832,12 +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); - 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", + ); } } } @@ -883,6 +882,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 +896,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/state.rs b/crates/bsk-cli/src/daemon/state.rs index fa4b1fec..056fc39b 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 6bd895c6..d30eca82 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 {} + 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/crates/bsk-cli/tests/sessions_ipc.rs b/crates/bsk-cli/tests/sessions_ipc.rs index c09ac0a6..9e3e31bd 100644 --- a/crates/bsk-cli/tests/sessions_ipc.rs +++ b/crates/bsk-cli/tests/sessions_ipc.rs @@ -1200,6 +1200,26 @@ async fn session_window_closed_event_purges_session() { wait_for_no_sessions(&state).await; + // Issue #465 (maintainer review): a dispatch against the ended id + // must surface why the session stopped, not a generic not-found. + let err = state + .tool_queues + .dispatch( + &bsk::daemon::sessions::SessionId("zzzz".into()), + Method::ToolTabList, + serde_json::json!({"session_id": "zzzz"}), + Duration::from_secs(1), + None, + ) + .await + .expect_err("the dead session must reject dispatches") + .into_rpc(); + assert_eq!(err.code, bsk_protocol::ErrorCode::NotFound); + assert_eq!( + err.data.unwrap()["reason"], + bsk::rpc_reason::SESSION_WINDOW_CLOSED + ); + let _ = ws.close(None).await; handle.shutdown().await; } @@ -1227,6 +1247,26 @@ async fn browser_disconnect_purges_sessions() { drop(ws); wait_for_no_sessions(&state).await; + // Issue #465 (maintainer review, P2): the ordinary socket-cleanup + // purge must tombstone the same reason as the other disconnect paths. + let err = state + .tool_queues + .dispatch( + &bsk::daemon::sessions::SessionId("zzzz".into()), + Method::ToolTabList, + serde_json::json!({"session_id": "zzzz"}), + Duration::from_secs(1), + None, + ) + .await + .expect_err("the dead session must reject dispatches") + .into_rpc(); + assert_eq!(err.code, bsk_protocol::ErrorCode::NotFound); + assert_eq!( + err.data.unwrap()["reason"], + bsk::rpc_reason::BROWSER_DISCONNECTED + ); + handle.shutdown().await; } @@ -1380,6 +1420,27 @@ async fn reconnect_with_same_instance_id_purges_stale_sessions_but_keeps_new_bro "stale sessions must not survive reconnect" ); + // Issue #465 (maintainer review, P2): the same-instance reconnect + // purge must tombstone the same reason as the ordinary disconnect + // path, so a later dispatch explains instead of guessing. + let err = state + .tool_queues + .dispatch( + &bsk::daemon::sessions::SessionId("xxxx".into()), + Method::ToolTabList, + serde_json::json!({"session_id": "xxxx"}), + Duration::from_secs(1), + None, + ) + .await + .expect_err("the dead session must reject dispatches") + .into_rpc(); + assert_eq!(err.code, bsk_protocol::ErrorCode::NotFound); + assert_eq!( + err.data.unwrap()["reason"], + bsk::rpc_reason::BROWSER_DISCONNECTED + ); + let _ = ws_b.close(None).await; handle.shutdown().await; } diff --git a/packages/dsh-plugin-browserskill/src/runner.ts b/packages/dsh-plugin-browserskill/src/runner.ts index fcda9930..30c1a349 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.` ); }