[Faisal-Fayaz]

Summary

Tracks sections of #465 that turn silent churn into distinct, actionable errors. Each change follows the existing stable data.reason convention: the extension and daemon attach reasons, human copy lives in crates/bsk-cli/src/cli/render_error.rs, and older peers ignore unknown reasons and fall back to code-based copy.

Deliberately out of scope (product/architecture decisions, not a contributed patch): the non-debugger control path (#454), the shared co-driving mode (§2), the Windows Job-Object daemon supervisor (§4), and the UI confirmation / detach-instead-of-close asks (§7).

Changes by issue section

§1 (partial, detection) — anti-debugging pages stop looking like a retry loop

• A site that reloads itself whenever the debugger attaches makes every read fail with the same identity error, so agents retry forever (and the previous hint even suggested reload, which makes the page worse each time).
• apps/extension/src/tools/observation.ts tracks consecutive identity failures per session+tab. After 3 consecutive failures within 120 s, the error is returned as cdp_failed with stable reason observe_identity_reload_loop and copy that names the likely anti-debugging condition, forbids further retries and reloads, and says to hand the task back. The message text is unchanged for earlier failures, and any successful read clears the streak.
• apps/extension/src/tools/vom/capture-coordinator.ts exports the marker message so detection does not rely on string duplication.

§3 — the DSH plugin points at bskPath, not only "install it"

• packages/dsh-plugin-browserskill/src/runner.ts — the missing-CLI error now mentions the plugin's own bskPath option (echoing its current value) in addition to the install URL.

§5 — bsk doctor distinguishes "not installed" from "installed but not connected"

• bsk doctor's 0 browsers connected check and its waiting line now cover both cases: install from the stores (URLs unchanged and still asserted by tests), or check a running window with the extension enabled in the expected profile (enablement is per-profile), the extension targeting this daemon's address/port, and a stale service worker after an update.

§6 — the borrow rejection carries the reference-doc guidance

• Borrowing a tab that already lives in an Agent Window used to return bare invalid_params with no reason. It now returns stable reason borrow_agent_window_tab with the guidance from packages/dsh-plugin-browserskill/skill/references/tabs-and-profiles.md: an unowned tab needs a user move to a user window first; a tab the session opened is already controlled and needs no borrow.

§7 (partial) — sessions say why they stopped

• SessionRegistry now tombstones recent session ends: SessionEnd { reason, ended_unix_ms } (bounded at 256, cleared when an id is reused, and reserve_id avoids tombstoned ids).
  • idle reaper → session_idle_timeout, Agent Window close → session_window_closed, browser liveness purge → browser_disconnected.
• A later dispatch against a dead id returns not_found with data.reason + ended_unix_ms instead of the generic "session not registered or already stopped". The idle-stop copy explains --session-idle (default 5m, see browser session idle time too short. 5 minutes ? Make it longer or user configurable #456) and that the Agent Window and its tab closed with the session, matching the measured behavior in the issue.

Tests

• cargo check/clippy --locked, cargo fmt --check
• cargo test --locked --lib (461 passed) — includes new tombstone lifecycle, bounded-cap, enriched-into_rpc, and reason-rendering tests
• --test per_session_queue (11 passed), updated for the SessionNotFound { ended } shape
• Extension: new observe-identity-loop.test.ts (streak rule), updated borrow test asserting the reason and message
• DSH plugin: 25 files / 560 tests pass, typecheck clean
• pnpm lint clean (biome, stylelint, plugin checks)

Note: daemon-lifecycle integration fixtures (auto_spawn/auto_update*) did not run in my sandbox because their fixtures require a release build/ports; the affected code paths are covered by the unit tests above.

References

• [Windows/DSH] Anti-debugging pages are unusable: observe fails with "observation document identity changed", plus other setup pitfalls #465 (this issue's §§1,3,5,6,7), [Feature] Quieter attach mode for pages with console-consumer anti-debugging (skip Runtime/Log domains) #454 (non-debugger attach remains open)

[iuyo5678]

Thanks for working on this. The more specific diagnostics should help users recover from disconnected extensions, invalid borrow targets, and unexpectedly ended sessions. The scope of this PR makes sense to me.

I reviewed e91dcd4 and found two issues I would address before merging: the identity-failure streak can carry over between unrelated pages, and the new browser-disconnect reason is missing from the ordinary disconnect/reconnect cleanup paths. Details and reproduction conditions are in the inline comments.

Validation: 460 Rust unit tests passed (1 ignored), 46 existing session/queue integration tests passed, and 180 relevant extension tests passed. Three additional regression probes reproduced the issues below: one extension scenario and two WebSocket lifecycle scenarios.

[iuyo5678]

[P2] Reset or scope the failure streak when the target page changes

The key only contains the session and tab IDs, so navigating the same tab to another site keeps the previous site's failures. I reproduced this in a handler-level test by changing the tab URL and invalidating its refs between three observations: one simulated transient identity failure on each of three different sites, all within 120 seconds, causes the third site to receive observe_identity_reload_loop.

That site has only failed once, but the new CLI hint tells the agent to stop observing and hand the task back. Following the suggestion to try another page in the same tab can therefore carry the false diagnosis into that page too.

Could we reset or scope the streak when the target page changes, while preserving detection of repeated reloads of the same page? A regression test covering navigation between different sites would help keep this distinction intact.

[Faisal-Fayaz]

Fixed in 93e711a. The streak entry now carries the observed target URL and only continues when the URL is unchanged (previous.url === url); a navigation — or an expired window — restarts the count from 1, while repeated failures on the same page are still counted (OBSERVE_IDENTITY_LOOP_FAILURES = 3, unchanged). The handler passes target.url (the same page identity captureVomObservation keys off), and the success path clears the entry.

Regression tests added in src/tools/__tests__/observe-identity-loop.test.ts, rule-level (same shape as the pure helper you reproduced through):

• your scenario: transient failure on each of three sites within the window → site C never receives observe_identity_reload_loop (count resets to 1 on url change),
• distinction you asked to preserve: after navigating away, a new site is not diagnosed until three of its own failures accumulate, and a genuine reload loop on one site is still detected,
• unchanged window semantics (older-than-window failures also restart).

[iuyo5678]

[P2] Record the disconnect reason on the ordinary WebSocket cleanup paths too

This records browser_disconnected for the liveness timeout, but the two other purge_browser paths in ws.rs do not record an end reason: the replacement handshake around line 319 and ordinary socket-disconnect cleanup around line 456.

I extended the existing disconnect and same-instance reconnect integration scenarios to inspect the subsequent missing-session error. Both return not_found without data.reason, so those common cases still produce the generic "session not registered or already stopped" diagnostic.

Could we record the reason consistently for all browser-driven session purges, preferably through a shared cleanup path, and cover both scenarios with assertions on the returned reason?

[Faisal-Fayaz]

Fixed in 93e711a, via the shared path you suggested: DaemonState::purge_sessions_of_browser(browser_id, end_reason, note) now owns the loop (purge rows → drop queue → drop interrupts → release transfers → tombstone why each id ended), and all three browser-driven purges call it — the reconnect-time stale purge (the replacement handshake), the ordinary socket-disconnect cleanup, and the liveness-timeout reaper, each keeping its own log note.

Assertions added to the existing scenarios in tests/sessions_ipc.rs; each now dispatches against the purged session id afterward and expects not_found with the surfaced reason:

• session_window_closed_event_purges_session → session_window_closed,
• browser_disconnect_purges_sessions → browser_disconnected,
• reconnect_with_same_instance_id_purges_stale_sessions_but_keeps_new_browser → browser_disconnected.

All reasons come from bsk::rpc_reason constants.

[iuyo5678]

Thanks for addressing the feedback. I rechecked 5f00fd7 and confirmed that both previous findings are resolved: failures no longer carry over between different target URLs, and ordinary disconnects and same-instance reconnects now preserve browser_disconnected. All three previously failing regression probes pass.

Validation: 518 Rust tests and 185 relevant extension tests passed (1 Rust test ignored). There is one remaining type-check error introduced by the new URL argument, detailed inline. Please address that before merging. I did not find any other blocking issues in this re-review.

[iuyo5678]

[P1] Handle the optional target URL before passing it to the streak helper

ResolvedTargetTab.url is string | undefined, while the new tabUrl parameter of observeIdentityReloadLoopError requires string. This call now fails the extension's TypeScript check:

pnpm --filter @browser-skill/extension compile

src/tools/observation.ts(1328,81): error TS2345:
Argument of type 'string | undefined' is not assignable to parameter of type 'string'.

The frontend CI workflow runs this command, so the passing unit tests do not cover this merge blocker. I also checked the previous observation implementation in the same environment, where the type check passes.

Could we narrow the URL before calling the helper, or explicitly support a missing URL? For example, when the URL is unavailable, preserve the original error without counting that failure toward a page-specific streak. This would keep the fix type-safe without assuming that every resolved tab has a URL.

[Faisal-Fayaz]

Fixed in 74c0951, following the "explicitly support a missing URL" option: tabUrl is now string | undefined and the helper keeps the plain identity-changed error (no streak counting at all) whenever the target could not be resolved to a URL — including an empty one, since an empty string cannot distinguish between pages either.

Rationale for not counting rather than falling back to a shared key: bucketing URL-less failures together on one tab would recreate exactly the false carry-over fixed for the three-sites scenario in 93e711a, just with "" instead of a real page. A real reload loop always resolves a URL in practice (it reloads the page it observed), so the diagnosis is not lost.

Verified with the command you quoted: pnpm --filter @browser-skill/extension compile passes again. The helper is now exported for tests; added cases in observe-identity-loop.test.ts assert that consecutive unavailable-URL failures (including "") never produce observe_identity_reload_loop, that a tab with a resolved URL is unaffected, and that an unavailable URL does not seed a later same-tab streak. pnpm lint exit 0.