From b17927b7aaf0dfc65adc0d798ba82ea4a8bd07d0 Mon Sep 17 00:00:00 2001 From: bcode Date: Wed, 6 May 2026 22:00:04 +0000 Subject: [PATCH] sync: harness 32d8d51 --- UPSTREAM.md | 1 + .../bcode-browser/harness/.github/VOUCHED.td | 2 + packages/bcode-browser/harness/README.md | 2 +- .../harness/src/browser_harness/_ipc.py | 83 ++++- .../harness/src/browser_harness/admin.py | 187 +++++++++-- .../harness/src/browser_harness/daemon.py | 111 ++++++- .../harness/src/browser_harness/helpers.py | 12 +- .../harness/src/browser_harness/run.py | 13 +- .../harness/tests/unit/test_admin.py | 291 ++++++++++++++++- .../harness/tests/unit/test_daemon.py | 295 ++++++++++++++++++ .../harness/tests/unit/test_helpers.py | 47 +++ .../harness/tests/unit/test_ipc.py | 107 +++++++ .../harness/tests/unit/test_run.py | 133 ++++++++ 13 files changed, 1220 insertions(+), 64 deletions(-) create mode 100644 packages/bcode-browser/harness/tests/unit/test_daemon.py create mode 100644 packages/bcode-browser/harness/tests/unit/test_ipc.py diff --git a/UPSTREAM.md b/UPSTREAM.md index 1da96bf6a..8bf228b69 100644 --- a/UPSTREAM.md +++ b/UPSTREAM.md @@ -93,6 +93,7 @@ Each upstream has its own append-only table. Add a row every time you pull. | 2026-04-30 | `997ee45` | `660827d` | bcode | 11 upstream commits (PRs #246, #247, #251, #254, #256, #260). `src/browser_harness/daemon.py`: resolve WS via `/json/version` to avoid stale `DevToolsActivePort` path (PR #260) + report `cdp_disconnected` on stale CDP probe in `connection_status` (PR #254) + cleanup remote browser when daemon startup fails (PR #251). `src/browser_harness/admin.py`: companion changes for the daemon fixes. `tests/unit/test_admin.py`: 7 new tests. New domain skills: `agent-workspace/domain-skills/xiaohongshu/scraping.md` (PR #246), and a top-level `domain-skills/shopify-admin/` tree (PR #247: README, embedded-apps, knowledge-base, polaris-inputs). Note: PR #247 added skills at the top-level `domain-skills/` path, not under `agent-workspace/domain-skills/` as the post-#229 layout would suggest — vendored verbatim to match upstream layout. Doc updates: README operator framing (PR #255), install.md heredoc → `-c` flag (PR #256), profile-sync.md same. All files outside divergences — taken verbatim. Smoke test + 19 admin unit tests pass. Divergences touched: none. | | 2026-05-01 | `660827d` | `013097a` | bcode | 8 upstream commits (PRs #261, #265, #266). `src/browser_harness/daemon.py` (PR #265): split `DevToolsActivePort` into port + ws-path lines and fall back to `ws://127.0.0.1:` when `/json/version` returns 404 (Chrome 147+ disables `/json/*` HTTP discovery on the default user-data-dir). `src/browser_harness/run.py` (PR #266): when no daemon is alive, no local Chrome is listening on 9222/9223 (probed via `/json/version`, not bare TCP), and `BROWSER_USE_API_KEY` is set, auto-bootstrap a cloud daemon. `tests/unit/test_run.py`: 2 new tests for the cloud bootstrap path. PR #261 moved `domain-skills/shopify-admin/` → `agent-workspace/domain-skills/shopify-admin/` upstream — both paths are excluded from the vendored tree per §3, so this rename is a no-op for browsercode (`script/check-harness-diff.sh` filters both via `IGNORED_PATHS_REGEX`). All in protected `src/browser_harness/*.py` + tests — taken verbatim. Smoke test + 23 unit tests pass. Divergences touched: none. | | 2026-05-03 | `013097a` | `59a166f` | bcode | 62 upstream commits. **Helper additions** (PRs #258, #279): `helpers.py` adds `fill_input` (raises on missing element, optional timeout for SPA rendering, dispatches select-all without char event so Cmd/Ctrl+A fires on macOS), `wait_for_element` (prefers `checkVisibility`, falls back to computed style), `wait_for_network_idle`. `tests/unit/test_helpers.py`: +253 lines covering the new helpers. `daemon.py`: discover Dia browser profile on macOS. **Windows IPC hardening** (PR #276): `_ipc.py` adds ping handshake, token auth, atomic port file. **Domain-skills opt-in** (PR #274): `helpers.py` gates auto-injected domain skills behind `BH_DOMAIN_SKILLS=1` (default off). Aligns upstream default with browsercode's exclusion policy — no behavior change for us, but the `BH_DOMAIN_SKILLS` env name is now the canonical knob if we ever decide to ship a curated set. **Cloud bootstrap opt-in** (PR #277): `run.py` makes cloud auto-bootstrap opt-in via `BU_AUTOSPAWN` instead of triggering on any `BROWSER_USE_API_KEY` presence. Plus admin tweaks (`tests/unit/test_admin.py` +10 lines), doc canonicalization (`README.md`, `SKILL.md`, `install.md`, `interaction-skills/profile-sync.md` PR #280), and new top-level scaffolding: `AGENTS.md` (repo orientation for coding agents), `.github/ISSUE_TEMPLATE/{bug-report,feature-request,config}.yml`, `.github/VOUCHED.td`, `docs/allow-remote-debugging.png`. All non-excluded paths taken verbatim. **Excluded paths** (per §3): 14 new domain-skills directories added upstream (aa, alaska, articulate-rise, bigbang-hr, bilibili, BOSS-zhipin, claude-ai, ctrip, flipkart, ly-com, manus, perplexity, wehotel, plus amazon under top-level `domain-skills/`) — skipped. **Divergence update**: `.gitignore` now also includes upstream's new `.idea/` and `.claude/` entries while preserving our `.venv/`. Smoke test (imports + `--version`) clean. Divergences touched: `.gitignore` (extended, same intent). | +| 2026-05-06 | `59a166f` | `32d8d515e` | bcode | 52 upstream commits. **PID-reuse safety in `restart_daemon`** (PR #294): `admin.py` gains `_process_start_time` (Linux `/proc//stat` field 22, macOS `ps -o lstart=`, Windows `GetProcessTimes` via ctypes) + new IPC `identify()` helper. `_ipc.py` hardens `ping` against non-dict/non-positive-pid responses. **`BH_RUNTIME_DIR` / `BH_TMP_DIR` split** (PR #318): `_ipc.py` introduces `BH_RUNTIME_DIR` for the AF_UNIX-sensitive sock/port/pid (104-byte `sun_path` budget) while `BH_TMP_DIR` keeps the long-path-tolerant log/screenshot files. Backward compatible — `BH_RUNTIME_DIR` falls back to `BH_TMP_DIR` then `/tmp`, so our `BH_TMP_DIR=ctx.bhTmpDir` setup in `browser-execute.ts` continues to work unchanged. (Future browsercode improvement: pass `BH_RUNTIME_DIR` separately so a deeper persistent `bhTmpDir` no longer has to fit the AF_UNIX budget. Tracked for ROADMAP follow-up — out of scope for this sync.) **AF_UNIX umask fix** (PR #309): `_ipc.py` sets `umask 0077` around `bind()` to remove the chmod TOCTOU window. **`current_tab` via daemon meta** (PR #305): `helpers.py` resolves the attached `target_id` server-side via the daemon's session meta instead of `Target.getTargetInfo`, fixing the missing-target case after a page nav. **CDP discovery fallback** (PR #292): `daemon.py` falls back to `ws://127.0.0.1:` when `/json/version` returns 404 (Chrome 147+ disables `/json/*` on default user-data-dirs); IPv6 hosts bracketed in the WS URL. **Tab-switch CDP parity** (PR #296): `daemon.py` enables Page/DOM/Runtime/Network on `set_session` to match initial-attach behavior; `helpers.py` filters `wait_for_network_idle` events by `session_id` so a previously-attached background tab doesn't poison idle on the current tab. **Run-time CDP precedence** (PR #300): `run.py` adds `_explicit_cdp_configured()` gate so `BU_CDP_URL` / `BU_CDP_WS` block the cloud auto-bootstrap (was silently overriding user's explicit endpoint and billing for a cloud browser). **Browser discovery additions**: Chrome Canary profile (PR #263, macOS + Windows in `daemon.py`), Brave on Windows (PR #284, `daemon.py`). **README banner** (PR #285): SVG ink-bleed reveal replaces the static R2 PNG. **VOUCHED.td** (PRs #308, #310): two bot/fabricated-profile exclusions. **Excluded paths** (per §3): 8 new domain-skills additions upstream (agentlist, browser-use-cloud, freewheel-mrm, tasksquad-ai, vercel, x — across PRs #281, #282, #283, #288, #301, #302) plus shopify-admin reorg/cleanup — skipped. All in-scope files (`src/browser_harness/*.py`, `tests/unit/*.py`, `README.md`, `.github/VOUCHED.td`) taken verbatim. Two new test files: `tests/unit/test_daemon.py`, `tests/unit/test_ipc.py`. Smoke test: imports ok, `browser-harness --version` → `0.1.0`, `pytest tests/unit/` → 76 passed. Divergences touched: none. | --- diff --git a/packages/bcode-browser/harness/.github/VOUCHED.td b/packages/bcode-browser/harness/.github/VOUCHED.td index b51d0cd73..d0e0cb1bf 100644 --- a/packages/bcode-browser/harness/.github/VOUCHED.td +++ b/packages/bcode-browser/harness/.github/VOUCHED.td @@ -11,3 +11,5 @@ molesza rohitdutt108 shaunandrewjackson1977 +-nandanadileep # Bot +-web-dev0521 # Fabricated profile, bot PRs diff --git a/packages/bcode-browser/harness/README.md b/packages/bcode-browser/harness/README.md index 704b5efd2..ab7f6a5d6 100644 --- a/packages/bcode-browser/harness/README.md +++ b/packages/bcode-browser/harness/README.md @@ -1,4 +1,4 @@ -Browser Harness +Browser Harness # Browser Harness ♞ diff --git a/packages/bcode-browser/harness/src/browser_harness/_ipc.py b/packages/bcode-browser/harness/src/browser_harness/_ipc.py index 71acddd0e..2d2657660 100644 --- a/packages/bcode-browser/harness/src/browser_harness/_ipc.py +++ b/packages/bcode-browser/harness/src/browser_harness/_ipc.py @@ -3,13 +3,22 @@ from pathlib import Path IS_WINDOWS = sys.platform == "win32" -# BH_TMP_DIR set → caller-isolated dir, bare filenames (avoids AF_UNIX sun_path -# overrun: 104 macOS / 108 Linux). Unset → shared tmpdir, "bu-" prefix -# disambiguates daemons. POSIX default is /tmp (gettempdir() returns long -# /var/folders/... on macOS); Windows uses TCP so any tempdir is fine. +# Two caller-supplied dirs: +# BH_RUNTIME_DIR — sock/port/pid. AF_UNIX sun_path is 104 bytes on macOS, so +# the runtime dir must be short. Caller is responsible for keeping it +# within budget. Falls back to BH_TMP_DIR (legacy single-dir callers), +# then to /tmp on POSIX (gettempdir() returns long /var/folders/... on +# macOS — unsafe for AF_UNIX) or tempfile.gettempdir() on Windows (TCP). +# BH_TMP_DIR — screenshots, debug overlays, daemon log. No path-length +# sensitivity; caller can use a deep persistent path. +# When the caller supplies a per-instance dir for either purpose, files use +# bare "bu" stems; otherwise "bu-" disambiguates co-tenants. BH_TMP_DIR = os.environ.get("BH_TMP_DIR") +BH_RUNTIME_DIR = os.environ.get("BH_RUNTIME_DIR") or BH_TMP_DIR _TMP = Path(BH_TMP_DIR or (tempfile.gettempdir() if IS_WINDOWS else "/tmp")) +_RUNTIME = Path(BH_RUNTIME_DIR or (tempfile.gettempdir() if IS_WINDOWS else "/tmp")) _TMP.mkdir(parents=True, exist_ok=True) +_RUNTIME.mkdir(parents=True, exist_ok=True) _NAME_RE = re.compile(r"\A[A-Za-z0-9_-]{1,64}\Z") # Set by serve() on Windows. Daemon's handle() requires every request to carry @@ -25,15 +34,20 @@ def _check(name): # path-traversal guard for BU_NAME return name -def _stem(name): # "bu" when BH_TMP_DIR isolates us, else "bu-" +def _runtime_stem(name): # "bu" when BH_RUNTIME_DIR isolates us, else "bu-" + _check(name) + return "bu" if BH_RUNTIME_DIR else f"bu-{name}" + + +def _tmp_stem(name): # "bu" when BH_TMP_DIR isolates us, else "bu-" _check(name) return "bu" if BH_TMP_DIR else f"bu-{name}" -def log_path(name): return _TMP / f"{_stem(name)}.log" -def pid_path(name): return _TMP / f"{_stem(name)}.pid" -def port_path(name): return _TMP / f"{_stem(name)}.port" # Windows-only: holds {"port","token"} JSON -def _sock_path(name): return _TMP / f"{_stem(name)}.sock" +def log_path(name): return _TMP / f"{_tmp_stem(name)}.log" +def pid_path(name): return _RUNTIME / f"{_runtime_stem(name)}.pid" +def port_path(name): return _RUNTIME / f"{_runtime_stem(name)}.port" # Windows-only: holds {"port","token"} JSON +def _sock_path(name): return _RUNTIME / f"{_runtime_stem(name)}.sock" def _read_port_file(name): @@ -48,7 +62,7 @@ def _read_port_file(name): def sock_addr(name): # display-only, used in log lines if not IS_WINDOWS: return str(_sock_path(name)) port, _ = _read_port_file(name) - return f"127.0.0.1:{port}" if port else f"tcp:{_stem(name)}" + return f"127.0.0.1:{port}" if port else f"tcp:{_runtime_stem(name)}" def spawn_kwargs(): # subprocess.Popen flags so the daemon detaches from this terminal @@ -97,22 +111,63 @@ def ping(name, timeout=1.0): except (FileNotFoundError, ConnectionRefusedError, TimeoutError, socket.timeout, OSError): return False try: - return request(c, token, {"meta": "ping"}).get("pong") is True - except (OSError, ValueError): + resp = request(c, token, {"meta": "ping"}) + # request() returns parsed JSON, which may be any valid value (a list, + # scalar, etc. from a stale or hostile endpoint). Anything that isn't + # a {pong: true} dict counts as "not our daemon" — never .get() blindly. + return isinstance(resp, dict) and resp.get("pong") is True + except (OSError, ValueError, AttributeError): return False finally: try: c.close() except OSError: pass +def identify(name, timeout=1.0): + """Return the live daemon's PID, or None if unreachable. + + Used by restart_daemon() to signal a process whose identity has been + verified end-to-end (live IPC + self-reported PID), instead of trusting + a pid file whose number may have been reused by an unrelated process.""" + try: + c, token = connect(name, timeout=timeout) + except (FileNotFoundError, ConnectionRefusedError, TimeoutError, socket.timeout, OSError): + return None + try: + resp = request(c, token, {"meta": "ping"}) + # request() returns parsed JSON, which may be any valid value (a list, + # scalar, etc. from a stale or hostile endpoint). Anything that isn't + # a {pong: true} dict gets None — never .get() on a non-dict. + if not isinstance(resp, dict) or resp.get("pong") is not True: + return None + pid = resp.get("pid") + # `type(pid) is int` (not isinstance) intentionally rejects bool: in + # Python, isinstance(True, int) is True, so a hostile/buggy daemon + # could reply with {"pid": True} and we'd treat that as PID 1 (init). + # Also reject 0/negatives — os.kill(0, sig) signals every process in + # the calling process group, os.kill(-1, sig) signals every process + # the caller can. Upper bound is 2**31 because C pid_t is typically + # signed 32-bit and a value outside that range makes os.kill() raise + # OverflowError, which would propagate out of restart_daemon() before + # its cleanup. Linux pid_max is also bounded at 2**22 in practice. + return pid if type(pid) is int and 0 < pid < (1 << 31) else None + except (OSError, ValueError, AttributeError): + return None + finally: + try: c.close() + except OSError: pass + + async def serve(name, handler): """Run the server until cancelled. handler(reader, writer) sees the same interface either way.""" global _server_token if not IS_WINDOWS: path = str(_sock_path(name)) if os.path.exists(path): os.unlink(path) - server = await asyncio.start_unix_server(handler, path=path) - os.chmod(path, 0o600) + # umask 0o077 makes bind() create the socket as 0600 — no TOCTOU window before chmod. + old_umask = os.umask(0o077) + try: server = await asyncio.start_unix_server(handler, path=path) + finally: os.umask(old_umask) _server_token = None async with server: await asyncio.Event().wait() return diff --git a/packages/bcode-browser/harness/src/browser_harness/admin.py b/packages/bcode-browser/harness/src/browser_harness/admin.py index 83109c419..b105100a8 100644 --- a/packages/bcode-browser/harness/src/browser_harness/admin.py +++ b/packages/bcode-browser/harness/src/browser_harness/admin.py @@ -1,6 +1,8 @@ import json import os import socket +import subprocess +import sys import tempfile import time import urllib.request @@ -9,6 +11,97 @@ from . import _ipc as ipc +def _process_start_time(pid): + """Opaque process-start-time fingerprint at PID, or None if unavailable. + + Two reads returning the same non-None value mean the PID still refers to + the same process; a different value means the PID was reused. Used by + restart_daemon() to keep the force-kill recovery path working even when + the daemon has already torn down its IPC socket (e.g. during a slow + remote shutdown), without falling back to "trust the pid file" — which + would re-introduce the PID-reuse hazard. + + Linux: /proc//stat field 22 (starttime in clock ticks since boot). + macOS: `ps -o lstart= -p ` (an absolute timestamp string). + Windows: GetProcessTimes via ctypes (FILETIME creation time, 100-ns since 1601). + Anywhere else: returns None; restart_daemon falls back to its strict + identify-only check, which is safer than no check at all. + """ + if type(pid) is not int or pid <= 0: + return None + if sys.platform.startswith("linux"): + try: + with open(f"/proc/{pid}/stat", "rb") as f: + raw = f.read().decode("ascii", errors="replace") + except (FileNotFoundError, PermissionError, OSError): + return None + # Field 2 is `(comm)`; comm can contain spaces and parens, so split off + # everything after the LAST `)` and index from there. + try: + tail = raw[raw.rindex(")") + 2:].split() + return tail[19] # starttime is field 22 (0-indexed: 21 - skipped 2 = 19) + except (ValueError, IndexError): + return None + if sys.platform == "darwin": + try: + out = subprocess.check_output( + ["ps", "-o", "lstart=", "-p", str(pid)], + stderr=subprocess.DEVNULL, timeout=2, + ) + except (subprocess.SubprocessError, OSError): + return None + s = out.decode("ascii", errors="replace").strip() + return s or None + if sys.platform == "win32": + # Windows users running a remote daemon hit the same slow-shutdown + # window as POSIX (stop_remote() PATCHes api.browser-use.com after + # the IPC socket has been torn down). Without a fingerprint here the + # SIGTERM gate can never pass during that window, leaving an orphan + # daemon that may continue to hold a billed cloud browser. Use + # GetProcessTimes via ctypes to read the kernel-reported creation + # time as a 64-bit FILETIME (100-ns intervals since 1601-01-01). + try: + import ctypes + from ctypes import wintypes + except ImportError: + return None + PROCESS_QUERY_LIMITED_INFORMATION = 0x1000 + try: + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + kernel32.OpenProcess.argtypes = [wintypes.DWORD, wintypes.BOOL, wintypes.DWORD] + kernel32.OpenProcess.restype = wintypes.HANDLE + kernel32.GetProcessTimes.argtypes = [ + wintypes.HANDLE, + ctypes.POINTER(wintypes.FILETIME), + ctypes.POINTER(wintypes.FILETIME), + ctypes.POINTER(wintypes.FILETIME), + ctypes.POINTER(wintypes.FILETIME), + ] + kernel32.GetProcessTimes.restype = wintypes.BOOL + kernel32.CloseHandle.argtypes = [wintypes.HANDLE] + kernel32.CloseHandle.restype = wintypes.BOOL + except (OSError, AttributeError): + return None + h = kernel32.OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, False, pid) + if not h: + return None + try: + creation = wintypes.FILETIME() + exit_ft = wintypes.FILETIME() + kernel_ft = wintypes.FILETIME() + user_ft = wintypes.FILETIME() + ok = kernel32.GetProcessTimes( + h, ctypes.byref(creation), ctypes.byref(exit_ft), + ctypes.byref(kernel_ft), ctypes.byref(user_ft), + ) + if not ok: + return None + return (creation.dwHighDateTime << 32) | creation.dwLowDateTime + finally: + kernel32.CloseHandle(h) + return None + + def _load_env(): repo_root = Path(__file__).resolve().parents[2] workspace = Path(os.environ.get("BH_AGENT_WORKSPACE", repo_root / "agent-workspace")).expanduser() @@ -75,15 +168,15 @@ def daemon_alive(name=None): def _daemon_endpoint_names(): - # BH_TMP_DIR isolates one daemon per dir → no filename-prefix discovery, - # just check whether our local endpoint exists. Without BH_TMP_DIR, _TMP - # is the shared default (`/tmp` etc.) and we glob `bu-*.` to find - # every daemon on the machine. + # BH_RUNTIME_DIR isolates one daemon per dir → no filename-prefix discovery, + # just check whether our local endpoint exists. Without BH_RUNTIME_DIR, + # _RUNTIME is the shared default (`/tmp` etc.) and we glob `bu-*.` + # to find every daemon on the machine. suffix = ".port" if ipc.IS_WINDOWS else ".sock" - if ipc.BH_TMP_DIR: - return [NAME] if (ipc._TMP / f"bu{suffix}").exists() else [] + if ipc.BH_RUNTIME_DIR: + return [NAME] if (ipc._RUNTIME / f"bu{suffix}").exists() else [] names = [] - for p in sorted(ipc._TMP.glob(f"bu-*{suffix}")): + for p in sorted(ipc._RUNTIME.glob(f"bu-*{suffix}")): raw = p.name[3:-len(suffix)] try: ipc._check(raw) @@ -187,33 +280,73 @@ def restart_daemon(name=None): Name is historical: callers typically follow this with another `browser-harness` invocation, which auto-spawns a fresh daemon via - ensure_daemon(). The function itself only stops.""" + ensure_daemon(). The function itself only stops. + + Identity is verified via ipc.identify() before any process signal, so + a stale pid file whose number has been reused by an unrelated process + is never SIGTERM'd. If the daemon is unreachable, we just clean up the + pid file and socket and return — never escalate to a kill-by-pid-file. + """ import signal - pid_path = str(ipc.pid_path(name or NAME)) - try: - c, token = ipc.connect(name or NAME, timeout=5.0) - ipc.request(c, token, {"meta": "shutdown"}) - c.close() - except Exception: - pass - try: - pid = int(open(pid_path).read()) - except (FileNotFoundError, ValueError): - pid = None - if pid: + name = name or NAME + pid_path = str(ipc.pid_path(name)) + + # Two pieces of information are tracked separately: + # - daemon_pid: the daemon's self-reported PID, or None. Only daemons + # running this version (or newer) include `pid` in the ping response; + # pre-upgrade daemons return {pong: True} only and yield None here. + # - daemon_alive: whether ANY daemon answers ping. Keeps the shutdown + # IPC path working across upgrades — without it, a still-running + # pre-upgrade daemon would have its socket deleted out from under it + # while the process stayed alive. + daemon_pid = ipc.identify(name, timeout=5.0) + daemon_alive = daemon_pid is not None or ipc.ping(name, timeout=1.0) + # Snapshot the daemon's process start-time as a secondary identity check. + # The IPC socket can disappear before the process exits (e.g. the shutdown + # path tears down the socket and then waits on a slow remote `stop` PATCH), + # so identify() going None partway through is not proof of process death. + # Comparing start-time before SIGTERM lets us recover the original + # force-kill behavior for slow shutdowns without re-opening the + # PID-reuse hole — a reused PID would have a different start-time. + daemon_start = _process_start_time(daemon_pid) + + if daemon_alive: + try: + c, token = ipc.connect(name, timeout=5.0) + ipc.request(c, token, {"meta": "shutdown"}) + c.close() + except Exception: + pass + + if daemon_pid is not None: for _ in range(75): try: - os.kill(pid, 0) + os.kill(daemon_pid, 0) time.sleep(0.2) - except (ProcessLookupError, OSError, SystemError): + except (ProcessLookupError, OSError, SystemError, OverflowError): break else: - try: - os.kill(pid, signal.SIGTERM) - except (ProcessLookupError, OSError, SystemError): - pass - ipc.cleanup_endpoint(name or NAME) + # Re-verify identity before escalating to SIGTERM. Two acceptable + # signals, in priority order: + # 1. ipc.identify() still returns the same PID — daemon's IPC is + # live, daemon is wedged. Safe to kill. + # 2. start-time fingerprint of the original PID is unchanged — + # same process, just slow to exit (e.g. stuck in remote stop). + # The IPC may already be gone; that's expected. + # If neither holds, the PID may have been reused; skip SIGTERM. + verified_pid = ipc.identify(name, timeout=1.0) + same_process = verified_pid == daemon_pid or ( + daemon_start is not None + and _process_start_time(daemon_pid) == daemon_start + ) + if same_process: + try: + os.kill(daemon_pid, signal.SIGTERM) + except (ProcessLookupError, OSError, SystemError, OverflowError): + pass + + ipc.cleanup_endpoint(name) try: os.unlink(pid_path) except FileNotFoundError: diff --git a/packages/bcode-browser/harness/src/browser_harness/daemon.py b/packages/bcode-browser/harness/src/browser_harness/daemon.py index 01edad1ce..077183e73 100644 --- a/packages/bcode-browser/harness/src/browser_harness/daemon.py +++ b/packages/bcode-browser/harness/src/browser_harness/daemon.py @@ -1,5 +1,6 @@ """CDP WS holder + IPC relay (Unix socket on POSIX, TCP loopback on Windows). One daemon per BU_NAME.""" import asyncio, json, os, socket, sys, time, urllib.error, urllib.request +from urllib.parse import urlparse from collections import deque from pathlib import Path @@ -34,6 +35,7 @@ def _load_env_file(p): BUF = 500 PROFILES = [ Path.home() / "Library/Application Support/Google/Chrome", + Path.home() / "Library/Application Support/Google/Chrome Canary", Path.home() / "Library/Application Support/Comet", Path.home() / "Library/Application Support/Arc/User Data", Path.home() / "Library/Application Support/Dia/User Data", @@ -53,11 +55,13 @@ def _load_env_file(p): Path.home() / ".var/app/com.brave.Browser/config/BraveSoftware/Brave-Browser", Path.home() / ".var/app/com.microsoft.Edge/config/microsoft-edge", Path.home() / "AppData/Local/Google/Chrome/User Data", + Path.home() / "AppData/Local/Google/Chrome SxS/User Data", Path.home() / "AppData/Local/Chromium/User Data", Path.home() / "AppData/Local/Microsoft/Edge/User Data", Path.home() / "AppData/Local/Microsoft/Edge Beta/User Data", Path.home() / "AppData/Local/Microsoft/Edge Dev/User Data", Path.home() / "AppData/Local/Microsoft/Edge SxS/User Data", + Path.home() / "AppData/Local/BraveSoftware/Brave-Browser/User Data", ] INTERNAL = ("chrome://", "chrome-untrusted://", "devtools://", "chrome-extension://", "about:") BU_API = "https://api.browser-use.com/api/v3" @@ -76,6 +80,27 @@ async def _silent(coro): pass +def _ws_from_devtools_active_port(http_url: str) -> str | None: + """When /json/version returns 404 (Chrome 147+ default profile), match DevToolsActivePort by port.""" + p = urlparse(http_url) + want_port = str(p.port) if p.port else "" + if not want_port: + return None + host = p.hostname or "127.0.0.1" + if ":" in host: # urlparse strips IPv6 brackets; restore them for the ws:// URL + host = f"[{host}]" + for base in PROFILES: + try: + active = (base / "DevToolsActivePort").read_text().splitlines() + except (FileNotFoundError, NotADirectoryError): + continue + port = active[0].strip() if active else "" + ws_path = active[1].strip() if len(active) > 1 else "" + if port == want_port and ws_path: + return f"ws://{host}:{port}{ws_path}" + return None + + def get_ws_url(): if url := os.environ.get("BU_CDP_WS"): return url @@ -85,9 +110,15 @@ def get_ws_url(): # M144 "Allow remote debugging" dialog and the M136 default-profile lockdown. deadline = time.time() + 30 last_err = None + base_url = url.rstrip("/") while time.time() < deadline: try: - return json.loads(urllib.request.urlopen(f"{url}/json/version", timeout=5).read())["webSocketDebuggerUrl"] + return json.loads(urllib.request.urlopen(f"{base_url}/json/version", timeout=5).read())["webSocketDebuggerUrl"] + except urllib.error.HTTPError as e: + last_err = e + if e.code == 404 and (ws := _ws_from_devtools_active_port(url)): + return ws + time.sleep(1) except Exception as e: last_err = e time.sleep(1) @@ -171,15 +202,32 @@ async def attach_first_page(self): ))["sessionId"] self.target_id = pages[0]["targetId"] log(f"attached {pages[0]['targetId']} ({pages[0].get('url','')[:80]}) session={self.session}") - for d in ("Page", "DOM", "Runtime", "Network"): + await self._enable_default_domains(self.session) + return pages[0] + + async def _enable_default_domains(self, session_id): + """Enable Page/DOM/Runtime/Network on a CDP session. + + Used by both initial attach and set_session (called after switch_tab/ + new_tab). Without this, helpers that depend on Network.* events — + notably wait_for_network_idle() — silently stop receiving events + after a tab switch, because each fresh CDP session starts with all + domains disabled. + + Runs the four enables in parallel via gather so the worst-case time is + bounded by a single CDP round trip rather than four sequential ones — + important on the set_session path, where the helper's IPC socket has + a 5s read timeout. + """ + async def enable_one(d): try: await asyncio.wait_for( - self.cdp.send_raw(f"{d}.enable", session_id=self.session), - timeout=5 + self.cdp.send_raw(f"{d}.enable", session_id=session_id), + timeout=4, ) except Exception as e: - log(f"enable {d}: {e}") - return pages[0] + log(f"enable {d} on {session_id}: {e}") + await asyncio.gather(*(enable_one(d) for d in ("Page", "DOM", "Runtime", "Network"))) async def start(self): self.stop = asyncio.Event() @@ -220,11 +268,25 @@ async def handle(self, req): meta = req.get("meta") # Liveness probe — lets clients confirm the listener is actually this # daemon and not an unrelated process that reused our port post-crash. - if meta == "ping": return {"pong": True} + # `pid` lets restart_daemon() verify the live daemon's identity before + # signaling — protects against SIGTERM-by-stale-pid-file after PID reuse. + if meta == "ping": return {"pong": True, "pid": os.getpid()} if meta == "drain_events": out = list(self.events); self.events.clear() return {"events": out} if meta == "session": return {"session_id": self.session} + if meta == "current_tab": + # Resolve the attached page's target info server-side. Helpers can't + # send Target.getTargetInfo themselves: daemon strips session_id for + # any Target.* method (browser-level call), and without a targetId + # Chrome silently returns the *browser* target. + if not self.target_id: + return {"error": "not_attached"} + try: + info = (await self.cdp.send_raw("Target.getTargetInfo", {"targetId": self.target_id}))["targetInfo"] + except Exception: + return {"error": "cdp_disconnected"} + return {"targetId": info.get("targetId"), "url": info.get("url", ""), "title": info.get("title", "")} if meta == "connection_status": if not self.target_id: return {"error": "not_attached"} @@ -241,12 +303,39 @@ async def handle(self, req): } return {"target_id": self.target_id, "session_id": self.session, "page": page} if meta == "set_session": + old_session = self.session self.session = req.get("session_id") self.target_id = req.get("target_id") or self.target_id - try: - await asyncio.wait_for(self.cdp.send_raw("Page.enable", session_id=self.session), timeout=3) - await asyncio.wait_for(self.cdp.send_raw("Runtime.evaluate", {"expression": "if(!document.title.startsWith('\U0001F7E2'))document.title='\U0001F7E2 '+document.title"}, session_id=self.session), timeout=2) - except Exception: pass + # Run the old-session Network.disable (defense in depth — keeps + # background-tab traffic out of the global event buffer; the + # consumer-side filter in wait_for_network_idle is the actual + # correctness gate) in parallel with the four enables on the new + # session. Different sessions, independent CDP requests. Keeps + # the synchronous reply under the helper's 5s IPC read timeout + # even on a remote daemon — sequentially these would have stacked + # to ~22s worst case. + tasks = [] + if old_session and old_session != self.session: + async def disable_old(): + try: + await asyncio.wait_for( + self.cdp.send_raw("Network.disable", session_id=old_session), + timeout=2, + ) + except Exception: pass + tasks.append(disable_old()) + tasks.append(self._enable_default_domains(self.session)) + await asyncio.gather(*tasks) + # 🟢 tab-marker title prefix is purely cosmetic — fire-and-forget so + # it doesn't add to the synchronous IPC budget. + asyncio.create_task(_silent(asyncio.wait_for( + self.cdp.send_raw( + "Runtime.evaluate", + {"expression": "if(!document.title.startsWith('\U0001F7E2'))document.title='\U0001F7E2 '+document.title"}, + session_id=self.session, + ), + timeout=2, + ))) return {"session_id": self.session} if meta == "pending_dialog": return {"dialog": self.dialog} if meta == "shutdown": self.stop.set(); return {"ok": True} diff --git a/packages/bcode-browser/harness/src/browser_harness/helpers.py b/packages/bcode-browser/harness/src/browser_harness/helpers.py index aa897da99..7e4cf13c1 100644 --- a/packages/bcode-browser/harness/src/browser_harness/helpers.py +++ b/packages/bcode-browser/harness/src/browser_harness/helpers.py @@ -292,8 +292,8 @@ def list_tabs(include_chrome=True): return out def current_tab(): - t = cdp("Target.getTargetInfo").get("targetInfo", {}) - return {"targetId": t.get("targetId"), "url": t.get("url", ""), "title": t.get("title", "")} + r = _send({"meta": "current_tab"}) + return {"targetId": r["targetId"], "url": r["url"], "title": r["title"]} def _mark_tab(): """Prepend 🟢 to tab title so the user can see which tab the agent controls.""" @@ -393,12 +393,20 @@ def wait_for_network_idle(timeout=10.0, idle_ms=500): Useful after form submits, SPA route transitions, and any action that triggers XHR/fetch without a visible DOM change. Builds on drain_events() — no daemon changes. Returns True if idle window reached, False on timeout. + + Events are filtered to the active session — a previously-attached background + tab (e.g. a polling/SSE page the agent switched away from) keeps emitting + Network events into the daemon's global event buffer; without this filter + they would poison the idle check on the current tab. """ deadline = time.time() + timeout last_activity = time.time() inflight = set() + active_session = _send({"meta": "session"}).get("session_id") while time.time() < deadline: for e in drain_events(): + if e.get("session_id") != active_session: + continue method = e.get("method", "") params = e.get("params", {}) if method == "Network.requestWillBeSent": diff --git a/packages/bcode-browser/harness/src/browser_harness/run.py b/packages/bcode-browser/harness/src/browser_harness/run.py index 2c6ac9573..4828bad76 100644 --- a/packages/bcode-browser/harness/src/browser_harness/run.py +++ b/packages/bcode-browser/harness/src/browser_harness/run.py @@ -56,6 +56,15 @@ def _local_chrome_listening(): return False +# BU_CDP_URL / BU_CDP_WS are documented to override local Chrome discovery +# (install.md:58-59), so they must also block cloud auto-bootstrap. Without this +# guard, start_remote_daemon() in admin.py overwrites BU_CDP_WS in the daemon +# env with a cloud WebSocket URL, silently replacing the user's explicit endpoint +# *and* billing them for a cloud browser they never asked for. +def _explicit_cdp_configured(): + return bool(os.environ.get("BU_CDP_URL") or os.environ.get("BU_CDP_WS")) + + def main(): args = sys.argv[1:] if args and args[0] in {"-h", "--help"}: @@ -83,10 +92,12 @@ def main(): print_update_banner() # Auto-bootstrap a cloud browser is opt-in via BU_AUTOSPAWN — BROWSER_USE_API_KEY alone # is not enough, since the key is commonly set for unrelated reasons (profile sync, - # cloud API calls, parent agents managing their own session). + # cloud API calls, parent agents managing their own session). An explicit BU_CDP_URL + # or BU_CDP_WS also blocks the spawn so we honour the precedence install.md promises. if ( not daemon_alive() and not _local_chrome_listening() + and not _explicit_cdp_configured() and os.environ.get("BROWSER_USE_API_KEY") and os.environ.get("BU_AUTOSPAWN") ): diff --git a/packages/bcode-browser/harness/tests/unit/test_admin.py b/packages/bcode-browser/harness/tests/unit/test_admin.py index 70be8afa2..e8b333cd3 100644 --- a/packages/bcode-browser/harness/tests/unit/test_admin.py +++ b/packages/bcode-browser/harness/tests/unit/test_admin.py @@ -50,8 +50,8 @@ def test_stale_websocket_does_not_open_chrome_inspect(): def test_daemon_endpoint_names_discovers_valid_socket_names(tmp_path, monkeypatch): monkeypatch.setattr(admin.ipc, "IS_WINDOWS", False) - monkeypatch.setattr(admin.ipc, "BH_TMP_DIR", None) # shared-tmpdir mode - monkeypatch.setattr(admin.ipc, "_TMP", tmp_path) + monkeypatch.setattr(admin.ipc, "BH_RUNTIME_DIR", None) # shared-tmpdir mode + monkeypatch.setattr(admin.ipc, "_RUNTIME", tmp_path) (tmp_path / "bu-default.sock").touch() (tmp_path / "bu-remote_1.sock").touch() (tmp_path / "bu-invalid.name.sock").touch() @@ -60,20 +60,20 @@ def test_daemon_endpoint_names_discovers_valid_socket_names(tmp_path, monkeypatc assert admin._daemon_endpoint_names() == ["default", "remote_1"] -def test_daemon_endpoint_names_with_bh_tmp_dir_returns_local_name_when_sock_exists(tmp_path, monkeypatch): +def test_daemon_endpoint_names_with_bh_runtime_dir_returns_local_name_when_sock_exists(tmp_path, monkeypatch): monkeypatch.setattr(admin.ipc, "IS_WINDOWS", False) - monkeypatch.setattr(admin.ipc, "BH_TMP_DIR", str(tmp_path)) - monkeypatch.setattr(admin.ipc, "_TMP", tmp_path) + monkeypatch.setattr(admin.ipc, "BH_RUNTIME_DIR", str(tmp_path)) + monkeypatch.setattr(admin.ipc, "_RUNTIME", tmp_path) monkeypatch.setattr(admin, "NAME", "session-xyz") (tmp_path / "bu.sock").touch() assert admin._daemon_endpoint_names() == ["session-xyz"] -def test_daemon_endpoint_names_with_bh_tmp_dir_returns_empty_when_sock_missing(tmp_path, monkeypatch): +def test_daemon_endpoint_names_with_bh_runtime_dir_returns_empty_when_sock_missing(tmp_path, monkeypatch): monkeypatch.setattr(admin.ipc, "IS_WINDOWS", False) - monkeypatch.setattr(admin.ipc, "BH_TMP_DIR", str(tmp_path)) - monkeypatch.setattr(admin.ipc, "_TMP", tmp_path) + monkeypatch.setattr(admin.ipc, "BH_RUNTIME_DIR", str(tmp_path)) + monkeypatch.setattr(admin.ipc, "_RUNTIME", tmp_path) monkeypatch.setattr(admin, "NAME", "session-xyz") assert admin._daemon_endpoint_names() == [] @@ -252,3 +252,278 @@ def fake_browser_use(path, method, body=None): assert calls == [ ("/browsers", "POST", {}), ] + + +# --- restart_daemon: PID-reuse safety --- + +def test_restart_daemon_does_not_signal_when_daemon_unreachable(monkeypatch, tmp_path): + """If ipc.identify() returns None (daemon gone), restart_daemon must NOT + fall back to reading the pid file and SIGTERMing whatever owns that PID — + that's the PID-reuse hazard. It should only clean up files.""" + pid_path = tmp_path / "default.pid" + # A pid file with a PID that, if signaled, would hit an unrelated process. + # The whole point is that we don't read or trust this number. + pid_path.write_text("99999") + + kill_calls = [] + monkeypatch.setattr(admin.os, "kill", lambda pid, sig: kill_calls.append((pid, sig))) + monkeypatch.setattr(admin.ipc, "identify", lambda name, timeout=5.0: None) + monkeypatch.setattr(admin.ipc, "ping", lambda name, timeout=1.0: False) + monkeypatch.setattr(admin.ipc, "pid_path", lambda name: pid_path) + monkeypatch.setattr(admin.ipc, "cleanup_endpoint", lambda name: None) + + # Should not raise, should not signal, should still clean up the pid file. + admin.restart_daemon("default") + + assert kill_calls == [], ( + f"restart_daemon SIGTERM'd a PID despite identify() returning None — " + f"this is the PID-reuse hazard the function is meant to avoid. Calls: {kill_calls}" + ) + assert not pid_path.exists(), "stale pid file should be cleaned up" + + +def test_restart_daemon_signals_pid_returned_by_identify_not_pid_file(monkeypatch, tmp_path): + """The PID we signal must come from the live daemon's self-report, never + from the pid file. If a stale pid file disagrees, the live daemon's PID wins.""" + import signal + + pid_path = tmp_path / "default.pid" + pid_path.write_text("99999") # bogus stale value — must be ignored + + live_pid = 4242 + + kill_calls = [] + def fake_kill(pid, sig): + kill_calls.append((pid, sig)) + # First os.kill(pid, 0) probe: report process is gone so we exit the loop + # without escalating. We just want to see WHICH pid was probed. + if sig == 0: + raise ProcessLookupError + + class FakeIPC: + def __init__(self): + self.shutdown_sent = False + def identify(self, name, timeout=5.0): + return live_pid + def connect(self, name, timeout): + return ("conn", "tok") + def request(self, conn, tok, msg): + if msg.get("meta") == "shutdown": + self.shutdown_sent = True + return {"ok": True} + def pid_path(self, name): + return pid_path + def cleanup_endpoint(self, name): + pass + + fake = FakeIPC() + monkeypatch.setattr(admin.os, "kill", fake_kill) + monkeypatch.setattr(admin.ipc, "identify", fake.identify) + monkeypatch.setattr(admin.ipc, "ping", lambda name, timeout=1.0: True) + monkeypatch.setattr(admin.ipc, "connect", fake.connect) + monkeypatch.setattr(admin.ipc, "request", fake.request) + monkeypatch.setattr(admin.ipc, "pid_path", fake.pid_path) + monkeypatch.setattr(admin.ipc, "cleanup_endpoint", fake.cleanup_endpoint) + + admin.restart_daemon("default") + + assert fake.shutdown_sent, "expected shutdown IPC to be sent" + assert kill_calls, "expected at least one os.kill probe" + pids_signaled = {pid for pid, _ in kill_calls} + assert pids_signaled == {live_pid}, ( + f"restart_daemon must only signal the PID returned by identify(); " + f"signaled pids: {pids_signaled}, expected {{{live_pid}}} (and NOT 99999)" + ) + assert not pid_path.exists() + + +def test_restart_daemon_sends_shutdown_to_pre_upgrade_daemon_without_pid_in_ping(monkeypatch, tmp_path): + """Backward compat: a pre-upgrade daemon's ping reply has {pong:True} but + no `pid` field, so identify() returns None. The shutdown IPC must STILL be + sent (so the daemon exits cleanly), but no os.kill happens (we have no + verified PID to safely signal).""" + pid_path = tmp_path / "default.pid" + pid_path.write_text("99999") # bogus stale value + + kill_calls = [] + shutdown_calls = [] + + def fake_request(conn, tok, msg): + if msg.get("meta") == "shutdown": + shutdown_calls.append(msg) + return {"ok": True} + + monkeypatch.setattr(admin.os, "kill", lambda pid, sig: kill_calls.append((pid, sig))) + monkeypatch.setattr(admin.ipc, "identify", lambda name, timeout=5.0: None) + monkeypatch.setattr(admin.ipc, "ping", lambda name, timeout=1.0: True) # old daemon: alive but no pid + monkeypatch.setattr(admin.ipc, "connect", lambda name, timeout: ("conn", "tok")) + monkeypatch.setattr(admin.ipc, "request", fake_request) + monkeypatch.setattr(admin.ipc, "pid_path", lambda name: pid_path) + monkeypatch.setattr(admin.ipc, "cleanup_endpoint", lambda name: None) + + admin.restart_daemon("default") + + assert shutdown_calls, ( + "restart_daemon must send shutdown IPC to a pre-upgrade daemon even " + "when identify() can't return a PID — otherwise upgrades orphan the " + "old daemon while deleting its socket and pid file." + ) + assert kill_calls == [], ( + f"no os.kill should fire when we don't have a verified PID, " + f"but got: {kill_calls}" + ) + assert not pid_path.exists() + + +def test_restart_daemon_skips_sigterm_if_pid_was_reused_during_wait(monkeypatch, tmp_path): + """A second identify() runs immediately before the SIGTERM. If the daemon + exited and the PID was reused mid-wait, identify() will return None (or a + different PID) and we must NOT signal — that's the PID-reuse race during + the 15s wait window.""" + import signal + + pid_path = tmp_path / "default.pid" + pid_path.write_text("99999") + live_pid = 4242 + + kill_calls = [] + + def fake_kill(pid, sig): + kill_calls.append((pid, sig)) + # All os.kill(pid, 0) probes succeed → loop exhausts → reaches the + # SIGTERM branch. (We're simulating a "wedged" daemon that the wait + # loop can't tell apart from a daemon whose PID got reused.) + + # First identify() call (top of restart_daemon) returns the live PID. + # Second identify() call (right before SIGTERM) returns None — simulating + # the daemon having exited and its PID having been reused by an unrelated + # process. The function must NOT escalate to SIGTERM in that state. + identify_responses = iter([live_pid, None]) + monkeypatch.setattr(admin.os, "kill", fake_kill) + monkeypatch.setattr(admin.ipc, "identify", lambda name, timeout=5.0: next(identify_responses)) + monkeypatch.setattr(admin.ipc, "ping", lambda name, timeout=1.0: True) + monkeypatch.setattr(admin.ipc, "connect", lambda name, timeout: ("conn", "tok")) + monkeypatch.setattr(admin.ipc, "request", lambda conn, tok, msg: {"ok": True}) + monkeypatch.setattr(admin.ipc, "pid_path", lambda name: pid_path) + monkeypatch.setattr(admin.ipc, "cleanup_endpoint", lambda name: None) + # Speed up the wait loop so the test finishes quickly. The loop polls 75 + # times at 0.2s = 15s; with sleep neutralized it runs in microseconds. + monkeypatch.setattr(admin.time, "sleep", lambda _s: None) + + admin.restart_daemon("default") + + sigterms = [(pid, sig) for pid, sig in kill_calls if sig == signal.SIGTERM] + assert sigterms == [], ( + f"restart_daemon issued SIGTERM despite the re-verify identify() " + f"returning None (PID was reused during the 15s wait). Calls: {kill_calls}" + ) + assert not pid_path.exists() + + +def test_restart_daemon_sigterms_via_start_time_fingerprint_when_socket_gone(monkeypatch, tmp_path): + """Slow-shutdown recovery: the daemon's serve() tears down the IPC socket + BEFORE the process exits (the daemon then runs slow cleanup like remote + `stop` PATCH calls that can hang). In that window, identify() returns None + even though the process is still our daemon. SIGTERM must still fire when + the PID's start-time fingerprint hasn't changed since we first identified + it — that's strong evidence of "same process, just slow to exit." + """ + import signal + + pid_path = tmp_path / "default.pid" + pid_path.write_text("99999") + live_pid = 4242 + + kill_calls = [] + + def fake_kill(pid, sig): + kill_calls.append((pid, sig)) + # All os.kill(pid, 0) probes succeed; loop exhausts → SIGTERM gate runs. + + # First identify() returns live_pid. Second identify() returns None — the + # daemon has torn down its IPC during shutdown but the process is still + # finishing up cleanup work, so the start-time fingerprint is unchanged. + identify_responses = iter([live_pid, None]) + # Both _process_start_time() calls return the same fingerprint, signaling + # "still the same process." This is the legitimate-slow-shutdown case. + monkeypatch.setattr(admin, "_process_start_time", lambda pid: "STARTED_AT_X") + monkeypatch.setattr(admin.os, "kill", fake_kill) + monkeypatch.setattr(admin.ipc, "identify", lambda name, timeout=5.0: next(identify_responses)) + monkeypatch.setattr(admin.ipc, "ping", lambda name, timeout=1.0: True) + monkeypatch.setattr(admin.ipc, "connect", lambda name, timeout: ("conn", "tok")) + monkeypatch.setattr(admin.ipc, "request", lambda conn, tok, msg: {"ok": True}) + monkeypatch.setattr(admin.ipc, "pid_path", lambda name: pid_path) + monkeypatch.setattr(admin.ipc, "cleanup_endpoint", lambda name: None) + monkeypatch.setattr(admin.time, "sleep", lambda _s: None) + + admin.restart_daemon("default") + + sigterms = [(pid, sig) for pid, sig in kill_calls if sig == signal.SIGTERM] + assert sigterms == [(live_pid, signal.SIGTERM)], ( + f"slow-shutdown daemon (identify=None but unchanged start-time) must " + f"still receive SIGTERM. signal calls: {kill_calls}" + ) + + +def test_restart_daemon_skips_sigterm_when_start_time_changed_during_wait(monkeypatch, tmp_path): + """If the start-time fingerprint of the original PID has CHANGED, the PID + was reused by another process. Even though identify() also returns None, + we must skip SIGTERM — start-time mismatch is the signal that protects + against killing an unrelated reused-PID process.""" + import signal + + pid_path = tmp_path / "default.pid" + pid_path.write_text("99999") + live_pid = 4242 + + kill_calls = [] + monkeypatch.setattr(admin.os, "kill", lambda pid, sig: kill_calls.append((pid, sig))) + + identify_responses = iter([live_pid, None]) + # First start-time read at top of restart_daemon: "ORIGINAL". + # Second start-time read in the safety gate: "DIFFERENT" — proof of reuse. + start_time_responses = iter(["ORIGINAL", "DIFFERENT"]) + monkeypatch.setattr(admin, "_process_start_time", lambda pid: next(start_time_responses)) + monkeypatch.setattr(admin.ipc, "identify", lambda name, timeout=5.0: next(identify_responses)) + monkeypatch.setattr(admin.ipc, "ping", lambda name, timeout=1.0: True) + monkeypatch.setattr(admin.ipc, "connect", lambda name, timeout: ("conn", "tok")) + monkeypatch.setattr(admin.ipc, "request", lambda conn, tok, msg: {"ok": True}) + monkeypatch.setattr(admin.ipc, "pid_path", lambda name: pid_path) + monkeypatch.setattr(admin.ipc, "cleanup_endpoint", lambda name: None) + monkeypatch.setattr(admin.time, "sleep", lambda _s: None) + + admin.restart_daemon("default") + + sigterms = [(pid, sig) for pid, sig in kill_calls if sig == signal.SIGTERM] + assert sigterms == [], ( + f"start-time mismatch indicates PID reuse — restart_daemon must NOT " + f"SIGTERM. signal calls: {kill_calls}" + ) + + +# --- _process_start_time helper --- + +def test_process_start_time_returns_stable_fingerprint_for_self(): + """The start-time of the current process should be readable on Linux, + macOS, and Windows, and stable across two reads.""" + import os as _os, sys + if sys.platform.startswith("linux") or sys.platform == "darwin" or sys.platform == "win32": + pid = _os.getpid() + first = admin._process_start_time(pid) + second = admin._process_start_time(pid) + assert first is not None, "expected a fingerprint for the current PID" + assert first == second, ( + f"two reads of the same PID should return the same fingerprint; " + f"got {first!r} vs {second!r}" + ) + + +def test_process_start_time_returns_none_for_invalid_pid(): + """Bad inputs (None, 0, negatives, non-int) and PIDs with no live process + must return None rather than raising.""" + for bad in (None, 0, -1, -42, "not-an-int", 1.5, True, False): + assert admin._process_start_time(bad) is None, ( + f"expected None for invalid pid {bad!r}" + ) + # 2**31 - 1 is the largest pid_t; in practice no live process at that PID. + assert admin._process_start_time((1 << 31) - 1) is None diff --git a/packages/bcode-browser/harness/tests/unit/test_daemon.py b/packages/bcode-browser/harness/tests/unit/test_daemon.py new file mode 100644 index 000000000..90c5bc855 --- /dev/null +++ b/packages/bcode-browser/harness/tests/unit/test_daemon.py @@ -0,0 +1,295 @@ +import asyncio + +from browser_harness import daemon + + +class _FakeCDP: + """Records send_raw calls so tests can assert which CDP methods fired.""" + + def __init__(self): + self.calls = [] # list of (method, params, session_id) + + async def send_raw(self, method, params=None, session_id=None): + self.calls.append((method, params, session_id)) + # Set-session/initial-attach paths only need a benign response. + return {} + + +def _fresh_daemon(): + d = daemon.Daemon() + d.cdp = _FakeCDP() + return d + + +def test_set_session_enables_all_four_default_domains_on_new_session(): + """Regression: switch_tab() / new_tab() in helpers.py route through the + `set_session` IPC, which previously only enabled Page on the new + session. With Network disabled, wait_for_network_idle() silently stops + receiving events after a tab switch. Initial attach enables all four + (Page, DOM, Runtime, Network); set_session must enable the same set.""" + d = _fresh_daemon() + new_session = "session-AFTER-switch" + + asyncio.run(d.handle({ + "meta": "set_session", + "session_id": new_session, + "target_id": "target-2", + })) + + enabled_on_new = [ + method for (method, _params, sid) in d.cdp.calls + if sid == new_session and method.endswith(".enable") + ] + assert set(enabled_on_new) == {"Page.enable", "DOM.enable", "Runtime.enable", "Network.enable"}, ( + f"set_session must enable Page/DOM/Runtime/Network on the new session " + f"(parity with initial attach). Got: {enabled_on_new}" + ) + assert d.session == new_session + assert d.target_id == "target-2" + + +def test_set_session_falls_back_to_existing_target_id_when_not_provided(): + """If a caller forgets target_id (passes None), the daemon should keep its + existing target_id rather than overwriting it with None — otherwise + subsequent calls that depend on self.target_id would break.""" + d = _fresh_daemon() + d.target_id = "original-target" + + asyncio.run(d.handle({ + "meta": "set_session", + "session_id": "session-AFTER", + "target_id": None, + })) + + assert d.target_id == "original-target" + assert d.session == "session-AFTER" + + +def test_enable_default_domains_swallows_errors_per_domain(): + """A single domain failing to enable must not prevent the others from + being attempted — that would leave the daemon in a partially-configured + state. Each Domain.enable call has its own try/except inside the helper.""" + class _PartialFailureCDP(_FakeCDP): + async def send_raw(self, method, params=None, session_id=None): + self.calls.append((method, params, session_id)) + if method == "DOM.enable": + raise RuntimeError("simulated DOM failure") + return {} + + d = daemon.Daemon() + d.cdp = _PartialFailureCDP() + + asyncio.run(d._enable_default_domains("session-X")) + + attempted = [m for (m, _p, _s) in d.cdp.calls] + assert "Page.enable" in attempted + assert "DOM.enable" in attempted # attempted, but raised + assert "Runtime.enable" in attempted + assert "Network.enable" in attempted + + +def test_set_session_disables_network_on_old_session_before_enabling_new(): + """When switching tabs, the previous session's Network domain must be + disabled so background tabs (polling, SSE, etc.) stop emitting events + into the global buffer that wait_for_network_idle reads. Initial attach + has no `old_session` so this disable doesn't fire then.""" + d = _fresh_daemon() + d.session = "session-OLD" + d.target_id = "target-OLD" + + asyncio.run(d.handle({ + "meta": "set_session", + "session_id": "session-NEW", + "target_id": "target-NEW", + })) + + disabled = [ + (method, sid) for (method, _params, sid) in d.cdp.calls + if method == "Network.disable" + ] + assert disabled == [("Network.disable", "session-OLD")], ( + f"Network.disable must fire on the old session before re-enabling on " + f"the new one. Got: {disabled}" + ) + + # Sanity: the new session still gets Network.enable. + enabled_on_new = { + method for (method, _p, sid) in d.cdp.calls + if sid == "session-NEW" and method.endswith(".enable") + } + assert "Network.enable" in enabled_on_new + + +def test_set_session_does_not_disable_network_when_no_previous_session(): + """First set_session call (e.g. very early in startup before any attach) + has no old_session — the Network.disable path must be skipped.""" + d = _fresh_daemon() + d.session = None # no prior attach + + asyncio.run(d.handle({ + "meta": "set_session", + "session_id": "session-FIRST", + "target_id": "target-FIRST", + })) + + disables = [m for (m, _p, _s) in d.cdp.calls if m == "Network.disable"] + assert disables == [], ( + f"Network.disable must not fire when there's no previous session " + f"to disable. Got: {disables}" + ) + + +def test_set_session_runs_disable_and_enables_in_parallel(): + """The four Domain.enable calls (plus Network.disable on the old session) + must run concurrently via asyncio.gather, not sequentially. With the old + sequential code, helpers.switch_tab() would block in _send() for up to + ~22s on a slow/remote daemon while the helper's IPC socket has a 5s + read timeout, causing client-side socket timeouts. Verifying that all + five CDP calls reach send_raw before any returns proves parallelization.""" + class _ConcurrencyProbeCDP: + def __init__(self): + self.calls = [] + self.in_flight = 0 + self.max_concurrent = 0 + self.release = None # asyncio.Event, set inside the test loop + + async def send_raw(self, method, params=None, session_id=None): + self.calls.append((method, params, session_id)) + self.in_flight += 1 + self.max_concurrent = max(self.max_concurrent, self.in_flight) + try: + await self.release.wait() + finally: + self.in_flight -= 1 + return {} + + async def run(): + d = daemon.Daemon() + d.cdp = _ConcurrencyProbeCDP() + d.session = "session-OLD" # ensures Network.disable on old fires + d.cdp.release = asyncio.Event() + + handle_task = asyncio.create_task(d.handle({ + "meta": "set_session", + "session_id": "session-NEW", + "target_id": "target-NEW", + })) + # Yield repeatedly until everything that's going to be in-flight is + # in-flight. Cap iterations to avoid hanging if parallelization breaks. + for _ in range(50): + await asyncio.sleep(0) + # 5 = Network.disable on OLD + 4 enables on NEW. + if d.cdp.in_flight >= 5: + break + peak = d.cdp.max_concurrent + d.cdp.release.set() + await handle_task + return peak, d.cdp.calls + + peak, calls = asyncio.run(run()) + assert peak == 5, ( + f"set_session must run disable + 4 enables concurrently via gather " + f"(observed peak in-flight = {peak}; expected 5 = 1 disable on OLD + " + f"4 enables on NEW). Sequential await would peak at 1." + ) + # Sanity: the right calls were made. + methods = sorted({m for (m, _p, _s) in calls}) + assert "Network.disable" in methods + assert {"Page.enable", "DOM.enable", "Runtime.enable", "Network.enable"}.issubset(methods) + + +def test_set_session_first_attach_runs_four_enables_in_parallel(): + """When there's no previous session, the disable path is skipped — only + the four enables run, still in parallel.""" + class _ConcurrencyProbeCDP: + def __init__(self): + self.calls = [] + self.in_flight = 0 + self.max_concurrent = 0 + self.release = None + + async def send_raw(self, method, params=None, session_id=None): + self.calls.append((method, params, session_id)) + self.in_flight += 1 + self.max_concurrent = max(self.max_concurrent, self.in_flight) + try: + await self.release.wait() + finally: + self.in_flight -= 1 + return {} + + async def run(): + d = daemon.Daemon() + d.cdp = _ConcurrencyProbeCDP() + d.session = None # no previous session + d.cdp.release = asyncio.Event() + + handle_task = asyncio.create_task(d.handle({ + "meta": "set_session", + "session_id": "session-FIRST", + "target_id": "target-FIRST", + })) + for _ in range(50): + await asyncio.sleep(0) + if d.cdp.in_flight >= 4: + break + peak = d.cdp.max_concurrent + d.cdp.release.set() + await handle_task + return peak + + peak = asyncio.run(run()) + assert peak == 4, ( + f"first set_session must run 4 enables concurrently " + f"(observed peak = {peak}). No Network.disable should fire." + ) + + +def test_current_tab_meta_passes_attached_target_id(): + """Regression for issue #304: helpers.current_tab() previously sent + Target.getTargetInfo with no targetId. The daemon strips session_id for + Target.* methods, so the call hit the browser-level connection with empty + params, and Chrome returned info about the *browser* target (empty + url/title) instead of the attached page. The daemon now resolves this + server-side using its tracked target_id.""" + class _TargetInfoCDP(_FakeCDP): + async def send_raw(self, method, params=None, session_id=None): + self.calls.append((method, params, session_id)) + if method == "Target.getTargetInfo": + return {"targetInfo": { + "targetId": params["targetId"], + "url": "https://example.com/", + "title": "Example Domain", + "type": "page", + }} + return {} + + d = daemon.Daemon() + d.cdp = _TargetInfoCDP() + d.target_id = "page-target-abc" + + result = asyncio.run(d.handle({"meta": "current_tab"})) + + assert result == { + "targetId": "page-target-abc", + "url": "https://example.com/", + "title": "Example Domain", + } + # The targetId must be passed through — that's the whole point of the fix. + get_info_calls = [(p, s) for (m, p, s) in d.cdp.calls if m == "Target.getTargetInfo"] + assert get_info_calls == [({"targetId": "page-target-abc"}, None)] + + +def test_current_tab_meta_returns_not_attached_when_no_target_id(): + """Without an attached page, current_tab() has no meaningful answer. + Returning {error: not_attached} causes _send() to raise in helpers, which + is the right signal for callers like ensure_real_tab() that wrap the call + in try/except.""" + d = _fresh_daemon() + d.target_id = None + + result = asyncio.run(d.handle({"meta": "current_tab"})) + + assert result == {"error": "not_attached"} + # No CDP call should have been issued. + assert d.cdp.calls == [] diff --git a/packages/bcode-browser/harness/tests/unit/test_helpers.py b/packages/bcode-browser/harness/tests/unit/test_helpers.py index e90602b99..4a45ee07a 100644 --- a/packages/bcode-browser/harness/tests/unit/test_helpers.py +++ b/packages/bcode-browser/harness/tests/unit/test_helpers.py @@ -303,3 +303,50 @@ def fake_send(req): assert result is False + + +def test_wait_for_network_idle_filters_events_to_active_session(): + """Background tabs (e.g. a polling page the agent switched away from) keep + emitting Network events into the daemon's global buffer. The wait must + filter by session_id of the currently-attached tab — otherwise it would + see the background tab's traffic and either fail to return idle or wait + on the wrong tab's requests.""" + active = "session-ACTIVE" + background = "session-BACKGROUND" + + # First /drain_events/ payload: rWS + lF on the BACKGROUND session that we + # must ignore, plus zero events on the active session. With filtering, the + # active session sees no traffic and the idle window can elapse. + events_seq = [ + [ + {"session_id": background, "method": "Network.requestWillBeSent", "params": {"requestId": "bg1"}}, + {"session_id": background, "method": "Network.loadingFinished", "params": {"requestId": "bg1"}}, + ], + [], # second drain — quiet on both sessions; idle window should fire here + ] + drain_idx = 0 + + def fake_send(req): + nonlocal drain_idx + if req.get("meta") == "session": + return {"session_id": active} + if req.get("meta") == "drain_events": + evs = events_seq[min(drain_idx, len(events_seq) - 1)] + drain_idx += 1 + return {"events": evs} + return {} + + with patch("browser_harness.helpers._send", side_effect=fake_send), \ + patch("browser_harness.helpers.time") as mock_time: + start = 1000.0 + # No inflight on active session → idle check uses time.time(). + mock_time.time.side_effect = [start, start, start, start + 0.6, start + 0.6] + mock_time.sleep = lambda _: None + result = helpers.wait_for_network_idle(timeout=5.0, idle_ms=500) + + assert result is True, ( + "wait_for_network_idle must return True even when the BACKGROUND " + "session is busy, as long as the ACTIVE session is idle. Without the " + "session filter, the background rWS/lF pair would have updated " + "last_activity and prevented the idle window from elapsing." + ) diff --git a/packages/bcode-browser/harness/tests/unit/test_ipc.py b/packages/bcode-browser/harness/tests/unit/test_ipc.py new file mode 100644 index 000000000..96e2dbc69 --- /dev/null +++ b/packages/bcode-browser/harness/tests/unit/test_ipc.py @@ -0,0 +1,107 @@ +from browser_harness import _ipc as ipc + + +# --- identify(): ping payload sanitation --- + +class _FakeConn: + def close(self): pass + + +def _patch_identify_response(monkeypatch, response): + """Stub connect() and request() so identify() sees `response` as the JSON + parsed from the daemon's reply, exactly as it would arrive over the wire.""" + monkeypatch.setattr(ipc, "connect", lambda name, timeout=1.0: (_FakeConn(), "tok")) + monkeypatch.setattr(ipc, "request", lambda conn, tok, msg: response) + + +def test_identify_returns_pid_for_well_formed_ping_reply(monkeypatch): + _patch_identify_response(monkeypatch, {"pong": True, "pid": 4242}) + + assert ipc.identify("default", timeout=0.0) == 4242 + + +def test_identify_rejects_boolean_pid(monkeypatch): + """isinstance(True, int) is True in Python; a hostile or buggy daemon + that replies {"pid": True} would otherwise yield PID 1 (init on POSIX), + which os.kill(1, SIGTERM) would target. Reject it explicitly.""" + _patch_identify_response(monkeypatch, {"pong": True, "pid": True}) + + assert ipc.identify("default", timeout=0.0) is None + + +def test_identify_rejects_boolean_false_pid(monkeypatch): + """False is also an int subclass and would yield PID 0.""" + _patch_identify_response(monkeypatch, {"pong": True, "pid": False}) + + assert ipc.identify("default", timeout=0.0) is None + + +def test_identify_returns_none_when_pid_field_missing(monkeypatch): + """Pre-upgrade daemons reply {pong: True} only — no pid. identify must + return None so callers know they have no verified PID to signal, while + still letting alive-checks via ipc.ping() succeed.""" + _patch_identify_response(monkeypatch, {"pong": True}) + + assert ipc.identify("default", timeout=0.0) is None + + +def test_identify_handles_non_dict_ping_payload(monkeypatch): + """request() can deserialize any valid JSON value. A stale or hostile + endpoint replying with a list / scalar / null would crash a naive + resp.get() with AttributeError; identify must absorb that and return None.""" + for payload in ([1, 2, 3], "hello", 42, None): + _patch_identify_response(monkeypatch, payload) + assert ipc.identify("default", timeout=0.0) is None, ( + f"identify() should reject non-dict ping payload: {payload!r}" + ) + + +def test_identify_returns_none_when_pong_is_not_true(monkeypatch): + _patch_identify_response(monkeypatch, {"pong": False, "pid": 4242}) + + assert ipc.identify("default", timeout=0.0) is None + + +def test_identify_rejects_zero_and_negative_pids(monkeypatch): + """os.kill semantics on POSIX: pid=0 signals every process in the calling + process group; pid=-1 signals every process the caller can; pid<-1 signals + the corresponding process group. None of these are valid daemon PIDs and + forwarding any of them to os.kill would be catastrophic.""" + for bad_pid in (0, -1, -42, -99999): + _patch_identify_response(monkeypatch, {"pong": True, "pid": bad_pid}) + assert ipc.identify("default", timeout=0.0) is None, ( + f"identify() must reject non-positive pid {bad_pid!r}" + ) + + +# --- ping(): same payload sanitation --- + +def _patch_ping_response(monkeypatch, response): + monkeypatch.setattr(ipc, "connect", lambda name, timeout=1.0: (_FakeConn(), "tok")) + monkeypatch.setattr(ipc, "request", lambda conn, tok, msg: response) + + +def test_ping_returns_true_for_well_formed_pong(monkeypatch): + _patch_ping_response(monkeypatch, {"pong": True}) + + assert ipc.ping("default", timeout=0.0) is True + + +def test_ping_handles_non_dict_payload(monkeypatch): + """Same regression class as identify(): if a stale or hostile endpoint + replies with a list / scalar / null, ping() must return False rather than + raising AttributeError on resp.get(). restart_daemon() now calls ping() on + the fallback path, so an unhandled raise here would abort cleanup.""" + for payload in ([1, 2, 3], "hello", 42, None): + _patch_ping_response(monkeypatch, payload) + assert ipc.ping("default", timeout=0.0) is False, ( + f"ping() should reject non-dict payload: {payload!r}" + ) + + +def test_ping_returns_false_when_pong_field_is_missing_or_not_true(monkeypatch): + for resp in ({}, {"pong": False}, {"pong": "yes"}, {"pong": 1}): + _patch_ping_response(monkeypatch, resp) + assert ipc.ping("default", timeout=0.0) is False, ( + f"ping() should require pong is exactly True; got: {resp!r}" + ) diff --git a/packages/bcode-browser/harness/tests/unit/test_run.py b/packages/bcode-browser/harness/tests/unit/test_run.py index 31cb4e1d8..abf559cf7 100644 --- a/packages/bcode-browser/harness/tests/unit/test_run.py +++ b/packages/bcode-browser/harness/tests/unit/test_run.py @@ -29,6 +29,139 @@ def test_cloud_bootstrap_on_headless_server(monkeypatch): mock_start.assert_called_once() +def test_explicit_bu_cdp_url_blocks_cloud_bootstrap(monkeypatch): + """BU_CDP_URL is documented to override local Chrome discovery (install.md:58-59), + so it must also block cloud auto-bootstrap. Otherwise start_remote_daemon would + overwrite BU_CDP_WS in the daemon env and silently bill the user for a cloud + browser instead of attaching to their explicit endpoint.""" + monkeypatch.setenv("BU_CDP_URL", "http://127.0.0.1:9333") + monkeypatch.setenv("BROWSER_USE_API_KEY", "test-key") + monkeypatch.setenv("BU_AUTOSPAWN", "1") + with patch.object(sys, "argv", ["browser-harness", "-c", "x = 1"]), \ + patch("browser_harness.run.daemon_alive", return_value=False), \ + patch("browser_harness.run._local_chrome_listening", return_value=False), \ + patch("browser_harness.run.start_remote_daemon") as mock_start, \ + patch("browser_harness.run.ensure_daemon"), \ + patch("browser_harness.run.print_update_banner"): + run.main() + mock_start.assert_not_called() + + +def test_explicit_bu_cdp_ws_blocks_cloud_bootstrap(monkeypatch): + """Same precedence guarantee for BU_CDP_WS — install.md:58 promises it overrides + local Chrome discovery for remote browsers, so cloud auto-bootstrap must defer + to the explicit WebSocket endpoint the caller already chose.""" + monkeypatch.setenv("BU_CDP_WS", "ws://example.test/devtools/browser/abc") + monkeypatch.setenv("BROWSER_USE_API_KEY", "test-key") + monkeypatch.setenv("BU_AUTOSPAWN", "1") + with patch.object(sys, "argv", ["browser-harness", "-c", "x = 1"]), \ + patch("browser_harness.run.daemon_alive", return_value=False), \ + patch("browser_harness.run._local_chrome_listening", return_value=False), \ + patch("browser_harness.run.start_remote_daemon") as mock_start, \ + patch("browser_harness.run.ensure_daemon"), \ + patch("browser_harness.run.print_update_banner"): + run.main() + mock_start.assert_not_called() + + +def test_empty_bu_cdp_url_does_not_block_bootstrap(monkeypatch): + """An env var set to empty string is conventionally treated as unset; the helper + must not let `BU_CDP_URL=""` accidentally suppress cloud bootstrap on the headless + fresh-box path #277 explicitly preserved.""" + monkeypatch.setenv("BU_CDP_URL", "") + monkeypatch.setenv("BROWSER_USE_API_KEY", "test-key") + monkeypatch.setenv("BU_AUTOSPAWN", "1") + with patch.object(sys, "argv", ["browser-harness", "-c", "x = 1"]), \ + patch("browser_harness.run.daemon_alive", return_value=False), \ + patch("browser_harness.run._local_chrome_listening", return_value=False), \ + patch("browser_harness.run.start_remote_daemon") as mock_start, \ + patch("browser_harness.run.ensure_daemon"), \ + patch("browser_harness.run.print_update_banner"): + run.main() + mock_start.assert_called_once() + + +def test_both_bu_cdp_url_and_bu_cdp_ws_set_blocks_bootstrap(monkeypatch): + """When the caller has BOTH endpoints configured (e.g. a parent agent that probes + BU_CDP_URL first and falls back to a known BU_CDP_WS), bootstrap must still defer + — the user has been doubly explicit about their intent.""" + monkeypatch.setenv("BU_CDP_URL", "http://127.0.0.1:9333") + monkeypatch.setenv("BU_CDP_WS", "ws://example.test/devtools/browser/abc") + monkeypatch.setenv("BROWSER_USE_API_KEY", "test-key") + monkeypatch.setenv("BU_AUTOSPAWN", "1") + with patch.object(sys, "argv", ["browser-harness", "-c", "x = 1"]), \ + patch("browser_harness.run.daemon_alive", return_value=False), \ + patch("browser_harness.run._local_chrome_listening", return_value=False), \ + patch("browser_harness.run.start_remote_daemon") as mock_start, \ + patch("browser_harness.run.ensure_daemon"), \ + patch("browser_harness.run.print_update_banner"): + run.main() + mock_start.assert_not_called() + + +def test_explicit_endpoint_does_not_break_daemon_alive_short_circuit(monkeypatch): + """daemon_alive=True must continue to short-circuit auto-bootstrap regardless of + whether an explicit endpoint is configured — re-using a live daemon was the + pre-existing fast path and the precedence guard must not regress it.""" + monkeypatch.setenv("BU_CDP_URL", "http://127.0.0.1:9333") + monkeypatch.setenv("BROWSER_USE_API_KEY", "test-key") + monkeypatch.setenv("BU_AUTOSPAWN", "1") + with patch.object(sys, "argv", ["browser-harness", "-c", "x = 1"]), \ + patch("browser_harness.run.daemon_alive", return_value=True), \ + patch("browser_harness.run._local_chrome_listening", return_value=False), \ + patch("browser_harness.run.start_remote_daemon") as mock_start, \ + patch("browser_harness.run.ensure_daemon"), \ + patch("browser_harness.run.print_update_banner"): + run.main() + mock_start.assert_not_called() + + +def test_explicit_endpoint_does_not_break_local_chrome_short_circuit(monkeypatch): + """If a local Chrome is already listening on 9222/9223 the bootstrap must skip + even when the user *also* set an explicit endpoint pointing somewhere else. + The auto-bootstrap path is for cloud only; routing between local-default and + explicit-non-default endpoints is handled later in daemon.py:get_ws_url().""" + monkeypatch.setenv("BU_CDP_URL", "http://127.0.0.1:9333") + monkeypatch.setenv("BROWSER_USE_API_KEY", "test-key") + monkeypatch.setenv("BU_AUTOSPAWN", "1") + with patch.object(sys, "argv", ["browser-harness", "-c", "x = 1"]), \ + patch("browser_harness.run.daemon_alive", return_value=False), \ + patch("browser_harness.run._local_chrome_listening", return_value=True), \ + patch("browser_harness.run.start_remote_daemon") as mock_start, \ + patch("browser_harness.run.ensure_daemon"), \ + patch("browser_harness.run.print_update_banner"): + run.main() + mock_start.assert_not_called() + + +def test_explicit_cdp_configured_helper_truthy(monkeypatch): + """Direct unit test of the helper: any non-empty BU_CDP_URL or BU_CDP_WS must + return True so the bootstrap guard reads as 'caller has been explicit'.""" + for name, value in [ + ("BU_CDP_URL", "http://127.0.0.1:9333"), + ("BU_CDP_WS", "ws://example.test/devtools/browser/abc"), + ("BU_CDP_URL", "http://[::1]:9333"), # IPv6 host + ("BU_CDP_WS", "wss://cloud.example.com/devtools/browser/x"), # secure WS + ]: + monkeypatch.delenv("BU_CDP_URL", raising=False) + monkeypatch.delenv("BU_CDP_WS", raising=False) + monkeypatch.setenv(name, value) + assert run._explicit_cdp_configured() is True, f"{name}={value!r} should be truthy" + + +def test_explicit_cdp_configured_helper_falsy(monkeypatch): + """Helper must return False for unset, empty-string, or both-unset cases — + those are all 'caller has not chosen an endpoint' from the bootstrap's POV.""" + monkeypatch.delenv("BU_CDP_URL", raising=False) + monkeypatch.delenv("BU_CDP_WS", raising=False) + assert run._explicit_cdp_configured() is False, "both unset" + monkeypatch.setenv("BU_CDP_URL", "") + assert run._explicit_cdp_configured() is False, "BU_CDP_URL empty string" + monkeypatch.delenv("BU_CDP_URL", raising=False) + monkeypatch.setenv("BU_CDP_WS", "") + assert run._explicit_cdp_configured() is False, "BU_CDP_WS empty string" + + def test_local_chrome_listening_rejects_non_chrome(): """A bare TCP listener on 9222/9223 must not fool the probe — only a real /json/version response counts as Chrome."""