Skip to content

fix: harden /browse lifecycle and ref safety#5

Open
morluto wants to merge 9 commits intogarrytan:mainfrom
morluto:morl/browse-correctness-patch
Open

fix: harden /browse lifecycle and ref safety#5
morluto wants to merge 9 commits intogarrytan:mainfrom
morluto:morl/browse-correctness-patch

Conversation

@morluto
Copy link
Contributor

@morluto morluto commented Mar 12, 2026

What This PR Does

  • hardens the /browse daemon lifecycle so stop and restart complete deterministically, startup is serialized, recreated state files do not wedge shutdown, and concurrent shells do not race a replacement daemon
  • switches snapshot refs from global live locators to tab-scoped frozen ElementHandles so refs do not leak across tabs or drift after SPA rerenders
  • fixes snapshot correctness around filtered and skipped nodes, including contiguous ref numbering and stable nth() bookkeeping for -d and -c
  • fixes browse command semantics for explicit-origin cookies, empty-string fill and select, quoted accessible names, and same-URL network attribution

Context

The original /browse implementation had a few correctness problems that were acceptable in a single-shell happy path but broke down under real agent usage:

  • live locator refs could silently retarget a different element after SPA DOM changes or tab switches
  • stop and restart could report success while the daemon was still in an ambiguous shutdown state
  • concurrent CLI calls could race startup or replacement-daemon creation
  • network attribution by URL could mix up concurrent requests to the same endpoint

The common theme is that wrong-target behavior is worse than an ordinary failure because it looks successful while mutating the wrong tab, element, or daemon state.

Implementation Overview

1. Snapshot refs and DOM targeting

  • BrowserManager now stores snapshot refs per tab as frozen ElementHandles instead of one global Locator map
  • stale or detached handles are normalized back into the same clear run snapshot guidance
  • snapshot now resolves a concrete handle before storing a ref, so refs only exist for resolvable nodes
  • ref numbering stays contiguous even when some accessibility nodes are skipped
  • skipped nodes from -d, -c, or -i still advance same-role/name occurrence tracking when they affect later nth() disambiguation

2. Daemon lifecycle and startup serialization

  • stop and restart return their HTTP response first, then schedule shutdown on the next event-loop turn
  • the CLI waits for the old PID to actually exit before considering stop or restart complete
  • startup is serialized with a lock file so parallel CLI invocations do not spawn duplicate daemons
  • the daemon keeps its state file until actual exit so concurrent shells do not mistake a shutting-down server for a dead one
  • recreated state files no longer block shutdown completion
  • user-agent settings persist across restart via a settings file next to the state file

3. Command and capture correctness

  • cookie accepts an explicit origin so it works before first navigation instead of failing on about:blank
  • fill and select now accept explicit empty-string values
  • snapshot parsing preserves escaped quotes in accessible names
  • network capture uses Playwright Request identity instead of reverse-scanning by URL, so same-URL requests keep the right status/size/duration pairing

Reviewer Guide

Suggested review order:

  1. browse/src/cli.ts, browse/src/server.ts, browse/src/meta-commands.ts
    Lifecycle, startup locking, stop/restart semantics, state ownership
  2. browse/src/browser-manager.ts, browse/src/snapshot.ts
    Ref model migration, stale-ref handling, request-identity tracking
  3. browse/src/write-commands.ts, browse/src/read-commands.ts
    Ref consumers and command-semantic fixes
  4. browse/test/commands.test.ts, browse/test/snapshot.test.ts
    Regression coverage for each failure mode fixed here

Risk / Compatibility

  • This intentionally changes /browse ref semantics: stale refs now fail instead of silently drifting to a different element
  • The daemon lifecycle is stricter: stop and restart wait for a real shutdown boundary rather than treating socket drop as success

Verification

  • bun test browse/test/commands.test.ts browse/test/snapshot.test.ts
  • ./setup sanity check on the merged branch

morluto and others added 7 commits March 12, 2026 17:54
Clean up browse daemon lifecycle, freeze refs per tab, and add regression coverage for restart, cookie, network, and snapshot edge cases.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wait for the requested daemon pid to exit instead of treating raw state-file existence as part of shutdown completion.
@morluto morluto changed the title fix: harden /browse lifecycle and ref safety fix: harden /browse lifecycle, refs, and setup flow Mar 12, 2026
…ness-patch

# Conflicts:
#	CHANGELOG.md
#	browse/src/cli.ts
@morluto morluto changed the title fix: harden /browse lifecycle, refs, and setup flow fix: harden /browse lifecycle and ref safety Mar 12, 2026
@vasiliyk
Copy link

@dependabot close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants