From 576abb012d1e2ffa569bd2f2ef6b9b3ddf9f69bd Mon Sep 17 00:00:00 2001 From: Alexandre Teixeira <111787685+alteixeira20@users.noreply.github.com> Date: Thu, 1 Oct 2026 20:59:27 +0100 Subject: [PATCH 1/2] feat(browser): deterministic private_browser lifecycle (Wave 5A) Own each agent-browser session as a browser tree: the daemon's POSIX session, its runtime files and its Chrome profile. Timeouts, launch failures, bootstrap recovery, cancellation and shutdown clean that tree and verify nothing survives, instead of killing only the daemon and orphaning Chrome. Per-call cleanup no longer sweeps every Chrome under the runtime TMPDIR. Sessionless calls get an ephemeral browser closed before returning. Actions on one session are serialized. Recovery is bounded by one deadline with at most one retry for local HTML open, and the retry flag is no longer model-visible. Observations after a failed navigation are marked stale. read URL navigates and extracts in one batch because agent-browser has no read command. Results carry a browser_lifecycle receipt with stages, timings, ownership and cleanup evidence. Playwright MCP tool calls are bounded by ODYSSEUS_BROWSER_MCP_CALL_TIMEOUT_S and are not retried. research_navigator now passes timeout_ms. --- .../wave-5a-browser-lifecycle.md | 115 ++++ scripts/generate_env_reference.py | 5 + src/agent_tools/web_tools.py | 448 +++++++++++--- src/browser_lifecycle.py | 397 ++++++++++++ src/mcp_manager.py | 30 + src/research_navigator.py | 2 +- tests/test_browser_lifecycle.py | 581 ++++++++++++++++++ tests/test_private_browser_tool.py | 43 +- website/configuration-reference.md | 9 +- 9 files changed, 1552 insertions(+), 78 deletions(-) create mode 100644 docs/runtime-decomposition/wave-5a-browser-lifecycle.md create mode 100644 src/browser_lifecycle.py create mode 100644 tests/test_browser_lifecycle.py diff --git a/docs/runtime-decomposition/wave-5a-browser-lifecycle.md b/docs/runtime-decomposition/wave-5a-browser-lifecycle.md new file mode 100644 index 000000000..c33f81bf9 --- /dev/null +++ b/docs/runtime-decomposition/wave-5a-browser-lifecycle.md @@ -0,0 +1,115 @@ +# Wave 5A: deterministic browser lifecycle + +Base: `a46eb7f47abaf15c799275f946d7dfe27bdee516`, branch `feature/browser-lifecycle`. +Scope is browser-specific lifecycle only. Request authority, approvals, +TurnContract, generic process containment (Wave 3-S), effects/provenance +(Wave 4), generic process lifecycle (Wave 5B) and runtime decomposition +(Wave 6) are unchanged. + +## Runtimes + +1. `private_browser` (`src/agent_tools/web_tools.py`, `PrivateBrowserTool`) is the + model-facing browser. It runs the `agent-browser` CLI per action. The CLI is a + short-lived client of a detached daemon; the daemon calls `setsid` and + launches Chrome. Identity is `--session ody-`. + Other entry points: `src/research_navigator.py` (`browser_read`), + `scripts/probe_browser_budget.py`, app shutdown in `app.py`. +2. Playwright MCP (`src/builtin_mcp.py`, server `builtin_browser`) is one global + `npx @playwright/mcp --headless --isolated --no-sandbox` stdio server owned by + `src/mcp_manager.py`. Its tools are hidden from the model unless + `private_browser` is disabled or `ODYSSEUS_EXPOSE_RAW_BROWSER_MCP` is set + (`src/agent_loop.py`, `_should_hide_raw_browser_mcp`). The two runtimes share + no code; only Chromium discovery overlaps. + +## Probe evidence (agent-browser 0.27.0, this host) + +- Runtime files live in `AGENT_BROWSER_SOCKET_DIR`, else + `$XDG_RUNTIME_DIR/agent-browser`, else `$HOME/.agent-browser`, as + `.{pid,sock,stream,version,engine}`. The socket path must stay under + about 103 bytes. +- Every Chrome process shares the daemon's POSIX session id (sid == daemon pid). +- `close` removes the daemon, Chrome, the runtime files and the + `agent-browser-chrome-*` profile. +- SIGKILL of the daemon alone (the previous timeout path) left 13 Chrome + processes, the profile, a Chromium temp directory and stale pid/socket files. +- `close` against a session with no daemon bootstraps one. +- A Chrome launch failure ("No usable sandbox", "Chrome exited early") leaves + the daemon alive; `close` cannot reach a browser. +- There is no `read` command ("Unknown command: read"). +- This host blocks the Chromium sandbox for agent-browser. Tests pass + `AGENT_BROWSER_ARGS=--no-sandbox` in the test environment only; production + launch flags are unchanged. + +## Failure modes found and their resolution + +| # | Failure | Resolution | +|---|---------|------------| +| F1 | Cancellation not handled; CLI, daemon and Chrome survived until idle timeout | `execute` catches `CancelledError`, kills every CLI client of the call and cleans the session tree, then re-raises | +| F2 | Timeout/exception killed only the daemon; Chrome reparented and leaked | `browser_lifecycle.force_cleanup` kills the daemon's whole POSIX session, removes runtime files and the profile, and verifies no survivor | +| F3 | Shutdown force-kill used `os.environ` and only the legacy layout | Shutdown uses each session's recorded launch environment, closes only verified live daemons, then force-cleans and verifies | +| F4 | Missing `session_id` used agent-browser's shared `default` session | A sessionless call gets an ephemeral session that is closed and verified before the call returns | +| F5 | Launch failure left the daemon alive | Launch-failure output triggers forced cleanup and a truthful error | +| F6 | Concurrent actions on one session raced one daemon | Per-session `asyncio.Lock` serializes actions | +| F7 | Observation after a failed navigation silently showed the old page | Sessions track navigation generation, page URL and failed navigation; such observations are prefixed with an explicit stale notice and flagged `stale_observation` | +| F8 | Recovery recursed through `execute` with a model-visible retry flag and no overall deadline | One deadline per call (action timeout + 75s); at most one retry, only for local read-only HTML open; model-supplied `_odysseus_browser_retry` is ignored | +| F9 | `research_navigator` passed `timeout`, which the tool ignored | Passes `timeout_ms` | +| F10 | No lifecycle evidence | Every result carries `browser_lifecycle` with stages, timings, ownership, state and cleanup receipt | +| F11 | Pid lookup assumed `/run/user/`; containers without `XDG_RUNTIME_DIR` were never cleaned | Runtime root follows agent-browser's own resolution from the launch environment | +| F12 | Per-call timeout swept every Chrome under the runtime `TMPDIR`, killing other sessions | Per-call cleanup is limited to the session tree; the `TMPDIR` sweep only runs at runtime shutdown | +| F13 | `read` used a command agent-browser does not have | `read URL` runs `open` and `get text body` in one batch; success requires both rows; `read` without URL extracts the current page | +| F14 | Playwright MCP calls had no time bound | `builtin_browser` calls are bounded by `ODYSSEUS_BROWSER_MCP_CALL_TIMEOUT_S` (default 90) and are not retried | + +## Lifecycle model + +Session states: `idle`, `ready`, `navigation_failed`, `reset`, `timed_out`, +`failed`, `launch_failed`, `bootstrap_failed`, `cancelled`, `closed`. Any state +reached by forced cleanup discards the page URL so nothing earlier remains +observable. Ownership is `retained` for a chat session (bounded by +`AGENT_BROWSER_IDLE_TIMEOUT_MS`, default 300000, and cleaned at shutdown) or +`ephemeral` for a sessionless call. + +The `browser_lifecycle` result field: + +```json +{"session": "ody-...", "ownership": "retained", "state": "ready", + "navigation_generation": 2, "page_url": "file:///...", + "stages": [{"stage": "open", "ms": 210, "ok": true, "cold_start": true}], + "elapsed_ms": 230, "cleanup": {"method": "forced", "verified": true, "...": "..."}, + "recovery_attempts": 1, "stale_observation": true, "closed_page_url": "..."} +``` + +Optional keys appear only when relevant. + +## Ownership boundary + +`src/browser_lifecycle.py` holds the browser-specific process attribution. It +claims processes only through the session's own pid file and the daemon's +POSIX session; once the daemon is gone it claims only Chrome process groups +whose root carries an `agent-browser-chrome-*` profile. Without procfs it kills +nothing. `kill_browser_tree` is the single seam to replace with the shared +process-lifecycle primitives from Wave 3-S/5B. + +## Files + +- New: `src/browser_lifecycle.py`, `tests/test_browser_lifecycle.py`, this document. +- Changed: `src/agent_tools/web_tools.py` (`PrivateBrowserTool` and shutdown), + `src/research_navigator.py` (timeout argument), `src/mcp_manager.py` (bounded + `builtin_browser` call), `scripts/generate_env_reference.py` and + `website/configuration-reference.md` (new variable), + `tests/test_private_browser_tool.py` (shutdown and read fakes). +- Not touched: `src/agent_loop.py`, `src/tool_execution.py`, + `src/agent_runtime/authority.py`, approvals, task and background infrastructure. + +## Limitations + +- A retained session's browser is not closed when its chat session is deleted; + it is bounded by the idle timeout and shutdown cleanup. +- The in-process session registry keeps one small record per chat session that + used the browser until shutdown. +- Chromium temp directories outside the profile (`org.chromium.Chromium.*`) are + not attributable to one session and are not removed by forced cleanup. +- Playwright MCP remains one global browser shared by all sessions. A timed-out + call is abandoned but the server is not restarted, because restarting the npx + server requires its owner task in `builtin_mcp.py`. +- The stale-observation notice marks, but does not block, an observation after + a failed navigation. diff --git a/scripts/generate_env_reference.py b/scripts/generate_env_reference.py index c00131574..35f017e03 100644 --- a/scripts/generate_env_reference.py +++ b/scripts/generate_env_reference.py @@ -557,6 +557,11 @@ VARIABLE_NOTES: dict[str, tuple[str, str, str]] = { "Cache directory handed to the browser MCP server, so its npm download " "survives a container rebuild.", ), + "ODYSSEUS_BROWSER_MCP_CALL_TIMEOUT_S": ( + "Browser automation", USER, + "Upper bound in seconds for one browser MCP tool call. A call that exceeds " + "it fails without being retried.", + ), "ODYSSEUS_BROWSER_MCP_REQUIRE_CACHE": ( "Browser automation", USER, "Truthy refuses to start the browser MCP server unless its npm package is " diff --git a/src/agent_tools/web_tools.py b/src/agent_tools/web_tools.py index 26122e3e0..b18757d37 100644 --- a/src/agent_tools/web_tools.py +++ b/src/agent_tools/web_tools.py @@ -1,6 +1,7 @@ import asyncio import base64 import contextlib +import contextvars import inspect import io import json @@ -10,15 +11,18 @@ import signal import shutil import sys import tempfile +import time import html import hashlib import pwd import urllib.parse import urllib.request +import uuid from pathlib import Path from typing import Dict, Any from core import platform_compat +from src import browser_lifecycle from src.constants import MAX_OUTPUT_CHARS PDF_EXTRACT_MAX_BYTES = 80_000_000 @@ -90,6 +94,28 @@ def _scoped_browser_session(namespace: str, session_id: str) -> str: return f"ody-{_bounded_browser_identity(scope)}" +def _browser_namespace(env: dict[str, str] | None) -> str: + return str( + (env or {}).get("ODYSSEUS_BROWSER_NAMESPACE") + or os.getenv("ODYSSEUS_BROWSER_NAMESPACE", "odysseus-ui") + ).strip() or "odysseus-ui" + + +# Browser CLI processes started by the current private_browser call, so a +# cancellation can stop every client it spawned, not only the main command. +_BROWSER_CALL_PROCS: contextvars.ContextVar[list | None] = contextvars.ContextVar( + "_BROWSER_CALL_PROCS", default=None +) + + +async def _spawn_browser_cli(*command, **kwargs): + proc = await asyncio.create_subprocess_exec(*command, **kwargs) + tracked = _BROWSER_CALL_PROCS.get() + if tracked is not None: + tracked.append(proc) + return proc + + def _browser_pid_file_candidates( runtime_dir: Path, namespace: str, session_id: str | None ) -> list[Path]: @@ -2433,15 +2459,30 @@ class PrivateBrowserTool: @staticmethod def _terminate_owned_daemon( env: dict[str, str], session_id: str | None = None - ) -> None: - """Terminate detached agent-browser daemon(s) for this runtime.""" + ) -> dict[str, Any] | None: + """Terminate the detached agent-browser tree owned by one session. - namespace = str( - env.get("ODYSSEUS_BROWSER_NAMESPACE") - or os.getenv("ODYSSEUS_BROWSER_NAMESPACE", "odysseus-ui") - ).strip() or "odysseus-ui" + Killing only the daemon reparents its Chrome children, so the whole + POSIX session the daemon leads is cleaned together with the session's + runtime files and browser profile. Returns the cleanup receipt. + """ + + namespace = _browser_namespace(env) + receipt = None + pid_files = [] + if session_id: + key = _scoped_browser_session(namespace, session_id) + root = browser_lifecycle.runtime_root(env) + receipt = browser_lifecycle.force_cleanup( + root, key, pid_alive=lambda pid: _process_is_alive(pid) + ).as_dict() + pid_files.append(root / f"{key}.pid") runtime_dir = Path(os.getenv("XDG_RUNTIME_DIR") or f"/run/user/{os.getuid()}") - pid_files = _browser_pid_file_candidates(runtime_dir, namespace, session_id) + pid_files = [ + path + for path in _browser_pid_file_candidates(runtime_dir, namespace, session_id) + if path not in pid_files + ] for pid_file in pid_files: try: pid = int(pid_file.read_text().strip()) @@ -2463,6 +2504,7 @@ class PrivateBrowserTool: os.kill(pid, signal.SIGKILL) with contextlib.suppress(FileNotFoundError, PermissionError, OSError): pid_file.unlink() + return receipt @staticmethod def _owned_daemon_exists(env: dict[str, str], session_id: str | None) -> bool: @@ -2475,12 +2517,17 @@ class PrivateBrowserTool: if not session_id: return False - namespace = str( - env.get("ODYSSEUS_BROWSER_NAMESPACE") - or os.getenv("ODYSSEUS_BROWSER_NAMESPACE", "odysseus-ui") - ).strip() or "odysseus-ui" + namespace = _browser_namespace(env) + key = _scoped_browser_session(namespace, session_id) + root = browser_lifecycle.runtime_root(env) + if browser_lifecycle.has_live_daemon( + root, key, pid_alive=lambda pid: _process_is_alive(pid) + ): + return True runtime_dir = Path(os.getenv("XDG_RUNTIME_DIR") or f"/run/user/{os.getuid()}") for pid_file in _browser_pid_file_candidates(runtime_dir, namespace, session_id): + if pid_file == root / f"{key}.pid": + continue try: pid = int(pid_file.read_text().strip()) except (OSError, ValueError): @@ -2553,10 +2600,184 @@ class PrivateBrowserTool: resolved = cls._resolve_workspace_path(raw_path) return resolved.as_uri() + # Time allowed beyond the action timeout for one bounded recovery attempt. + _RECOVERY_BUDGET_S = 75 + _CLOSE_TIMEOUT_S = 10 + async def execute(self, content: str, ctx: dict) -> dict: + """Run one browser action inside its session's lifecycle. + + Actions on one session are serialized. A call without an owning + Odysseus session gets a browser of its own that is closed before the + call returns; it never falls back to agent-browser's shared default + session. Cancellation stops every CLI client the call started and + cleans the session's browser tree, because its state is unknown. + """ + + ctx = dict(ctx) if isinstance(ctx, dict) else {} + session_id = str(ctx.get("session_id") or "").strip() + ephemeral = not session_id + if ephemeral: + session_id = f"ephemeral-{uuid.uuid4().hex}" + ctx["session_id"] = session_id + runtime_env = ctx.get("subproc_env") if isinstance(ctx.get("subproc_env"), dict) else {} + key = _scoped_browser_session(_browser_namespace(runtime_env), session_id) + browser = browser_lifecycle.session_for(key, ephemeral) + clock = browser_lifecycle.StageClock() + procs: list = [] + token = _BROWSER_CALL_PROCS.set(procs) + lock = browser.lock() + acquired = False + try: + await lock.acquire() + acquired = True + result = await self._execute_unlocked(content, ctx, browser=browser, clock=clock) + if ephemeral: + await self._release_session(browser, session_id, clock) + if isinstance(result, dict) and browser.env is not None: + result["browser_lifecycle"] = browser.receipt(clock) + return result + except asyncio.CancelledError: + if acquired: + for proc in procs: + if getattr(proc, "returncode", None) is None: + self._terminate_subprocess(proc) + if browser.env is not None: + self._terminate_owned_daemon(browser.env, session_id) + browser.discarded("cancelled") + raise + except Exception: + if acquired and ephemeral and browser.env is not None: + self._terminate_owned_daemon(browser.env, session_id) + raise + finally: + if acquired: + lock.release() + _BROWSER_CALL_PROCS.reset(token) + if ephemeral: + browser_lifecycle.forget(key) + _ACTIVE_BROWSER_SESSIONS.discard(key) + + async def _release_session( + self, + browser: browser_lifecycle.BrowserSession, + session_id: str, + clock: browser_lifecycle.StageClock, + ) -> None: + """Close a session gracefully, then verify nothing it owned survives.""" + + if browser.env is None: + return + started = time.monotonic() + graceful = False + if self._owned_daemon_exists(browser.env, session_id): + proc = None + try: + proc = await _spawn_browser_cli( + *browser.command_prefix, + "close", + stdout=asyncio.subprocess.DEVNULL, + stderr=asyncio.subprocess.DEVNULL, + env=browser.env, + start_new_session=True, + ) + await asyncio.wait_for(proc.wait(), timeout=self._CLOSE_TIMEOUT_S) + graceful = (proc.returncode or 0) == 0 + except Exception: + if proc is not None: + self._terminate_subprocess(proc) + receipt = self._terminate_owned_daemon(browser.env, session_id) + verified = bool(receipt.get("verified")) if isinstance(receipt, dict) else False + clock.record("close", started, verified or graceful) + clock.extra["cleanup"] = {"graceful_close": graceful, **(receipt or {})} + if browser.page_url: + clock.extra["closed_page_url"] = browser.page_url + browser.discarded("closed") + + def _discard_session( + self, + browser: browser_lifecycle.BrowserSession, + env: dict[str, str], + session_id: str, + clock: browser_lifecycle.StageClock, + state: str, + ) -> None: + """Force-clean a session whose browser state can no longer be trusted.""" + + started = time.monotonic() + receipt = self._terminate_owned_daemon(env, session_id or None) + verified = bool(receipt.get("verified")) if isinstance(receipt, dict) else False + clock.record("forced_cleanup", started, verified, reason=state) + clock.extra["cleanup"] = receipt + browser.discarded(state) + + @staticmethod + def _navigation_target(action: str, args: dict) -> str: + """URL this action navigates the session to, or ``""``.""" + + if action == "read" and any( + str(args.get(key) or "").strip() for key in ("selector", "target", "ref") + ): + return "" + if action in {"open", "read"}: + return str(args.get("url") or "").strip() + if action == "batch" and isinstance(args.get("commands"), list): + target = "" + for command in args["commands"]: + if ( + isinstance(command, list) + and len(command) > 1 + and str(command[0]).lower() in {"open", "goto", "navigate"} + ): + target = str(command[1]).strip() + return target + return "" + + @staticmethod + def _read_page_from_rows(output: str) -> dict[str, Any]: + """Page text from an open + ``get text`` batch, only if both succeeded.""" + + try: + rows = json.loads(output) + except (ValueError, TypeError): + rows = None + if not isinstance(rows, list) or len(rows) != 2 or not all(isinstance(r, dict) for r in rows): + return {"ok": False, "error": "private_browser read returned no structured page result"} + opened, extracted = rows + for row in rows: + if row.get("success") is not True: + return {"ok": False, "error": f"private_browser read failed: {row.get('error') or 'unknown error'}"} + opened_result = opened.get("result") if isinstance(opened.get("result"), dict) else {} + extracted_result = extracted.get("result") if isinstance(extracted.get("result"), dict) else {} + text = extracted_result.get("text") + if not isinstance(text, str): + return {"ok": False, "error": "private_browser read observed no page text"} + url = str(opened_result.get("url") or extracted_result.get("origin") or "") + title = str(opened_result.get("title") or "") + header = "\n".join(part for part in (title, url) if part) + return {"ok": True, "url": url, "text": f"{header}\n\n{text}".strip()} + + @staticmethod + def _navigated_url(output: str) -> str: + """Final URL reported by ``open`` (after redirects), when present.""" + + match = re.search(r"^\s+([a-z][a-z0-9+.-]*:\S+)\s*$", str(output or ""), re.MULTILINE) + return match.group(1) if match else "" + + async def _execute_unlocked( + self, + content: str, + ctx: dict, + *, + browser: browser_lifecycle.BrowserSession, + clock: browser_lifecycle.StageClock, + retry: bool = False, + deadline: float | None = None, + ) -> dict: args, err = self._parse_args(content) if err: return {"error": err, "exit_code": 1} + args.pop("_odysseus_browser_retry", None) action = str(args.get("action") or "").strip().lower() if action not in self._ACTIONS: @@ -2697,8 +2918,23 @@ class PrivateBrowserTool: # only errors emitted by this page. Opening a URL replaces prior page # state anyway; cookies are irrelevant for confined file:// artifacts. session_id = str((ctx or {}).get("session_id") or "").strip() - if verifies_local_html and self._owned_daemon_exists(env, session_id): + browser.bind(env, cmd_prefix) + loop = asyncio.get_running_loop() + if deadline is None: + deadline = loop.time() + timeout_s + self._RECOVERY_BUDGET_S + warm = self._owned_daemon_exists(env, session_id) + if verifies_local_html and warm: + reset_started = time.monotonic() await self._reset_browser_session(cmd_prefix, env, timeout_s) + clock.record("reset", reset_started, True) + browser.discarded("reset") + navigation_url = self._navigation_target(action, command_args) + stale_note = ( + browser.stale_observation_note() + if action in browser_lifecycle.OBSERVATION_ACTIONS and not navigation_url + else "" + ) + command_started = time.monotonic() # agent-browser starts a persistent daemon which can inherit the # client's stdout/stderr descriptors. Pipes therefore never reach @@ -2708,8 +2944,10 @@ class PrivateBrowserTool: # depend on the client process, not its detached daemon. stdout_file = tempfile.TemporaryFile() stderr_file = tempfile.TemporaryFile() + attempt_timeout = max(1.0, min(float(timeout_s), deadline - loop.time())) + proc = None try: - proc = await asyncio.create_subprocess_exec( + proc = await _spawn_browser_cli( *command, stdin=asyncio.subprocess.PIPE if stdin_data is not None else None, stdout=stdout_file, @@ -2719,40 +2957,47 @@ class PrivateBrowserTool: ) await asyncio.wait_for( proc.communicate(stdin_data.encode("utf-8") if stdin_data is not None else None), - timeout=timeout_s, + timeout=attempt_timeout, ) stdout_file.seek(0) stderr_file.seek(0) stdout = stdout_file.read() stderr = stderr_file.read() except asyncio.TimeoutError: - with contextlib.suppress(Exception): - self._terminate_subprocess(proc) - self._terminate_owned_chrome(env) - self._terminate_owned_daemon( - env, str((ctx or {}).get("session_id") or "").strip() or None - ) + if proc is not None: + with contextlib.suppress(Exception): + self._terminate_subprocess(proc) + clock.record(action, command_started, False, cold_start=not warm, failure="timeout") + self._discard_session(browser, env, session_id, clock, "timed_out") # A failed local-page verification can leave agent-browser's # persistent session between a page-error response and the next - # repair attempt. Reopen exactly once after resetting that session; - # never retry mutating browser actions or arbitrary URLs. - if ( - verifies_local_html - and action == "open" - and not args.get("_odysseus_browser_retry") - ): + # repair attempt. The session was cleaned above, so reopen exactly + # once within the call's deadline; never retry mutating browser + # actions or arbitrary URLs. + remaining = deadline - loop.time() + if verifies_local_html and action == "open" and not retry and remaining >= 10: retry_args = dict(args) - retry_args["_odysseus_browser_retry"] = True - retry_args["timeout_ms"] = max(60_000, timeout_s * 1000) - if self._owned_daemon_exists(env, session_id): - await self._reset_browser_session(cmd_prefix, env, timeout_s) - return await self.execute(json.dumps(retry_args), ctx) - return {"error": f"private_browser timed out after {timeout_s}s", "exit_code": 1} + retry_args["timeout_ms"] = int( + min(max(60.0, float(timeout_s)), remaining - 5) * 1000 + ) + clock.extra["recovery_attempts"] = 1 + return await self._execute_unlocked( + json.dumps(retry_args), ctx, + browser=browser, clock=clock, retry=True, deadline=deadline, + ) + return { + "error": f"private_browser timed out after {int(attempt_timeout)}s", + "exit_code": 1, + } except Exception as e: - self._terminate_owned_chrome(env) - self._terminate_owned_daemon( - env, str((ctx or {}).get("session_id") or "").strip() or None - ) + if proc is not None: + with contextlib.suppress(Exception): + self._terminate_subprocess(proc) + clock.record(action, command_started, False, cold_start=not warm, failure=type(e).__name__) + if proc is not None: + # The client reached the daemon, so the session's state is + # unknown. A client that never started left it untouched. + self._discard_session(browser, env, session_id, clock, "failed") return {"error": f"private_browser failed: {type(e).__name__}: {e}", "exit_code": 1} finally: stdout_file.close() @@ -2763,6 +3008,47 @@ class PrivateBrowserTool: combined = out if err_text: combined = f"{combined}\n\n[stderr]\n{err_text}".strip() + command_ok = (proc.returncode or 0) == 0 + read_page = None + if action == "read" and navigation_url: + read_page = self._read_page_from_rows(out) + command_ok = command_ok and read_page.get("ok", False) + clock.record(action, command_started, command_ok, cold_start=not warm) + if not command_ok and browser_lifecycle.LAUNCH_FAILURE_RE.search(combined): + # The browser never became ready. The daemon outlives this failure + # and a later close cannot reach a browser, so clean it here. + self._discard_session(browser, env, session_id, clock, "launch_failed") + return { + "output": combined[:4000], + "error": ( + "private_browser could not launch the browser; no page was " + "opened or observed. The browser session was cleaned up." + ), + "exit_code": 1, + "untrusted_content": True, + } + if navigation_url: + if command_ok: + browser.navigated( + (read_page or {}).get("url") or self._navigated_url(out) or navigation_url + ) + else: + browser.navigation_failed(navigation_url) + if read_page is not None: + if not command_ok: + return { + "output": combined[:4000], + "error": read_page.get("error") or "private_browser read failed; no page text was observed", + "exit_code": 1, + "untrusted_content": True, + } + out = read_page["text"] + combined = out if not err_text else f"{out}\n\n[stderr]\n{err_text}" + elif command_ok and browser.state in {"idle", "closed", "reset"}: + browser.state = "ready" + if stale_note and command_ok: + combined = f"[{stale_note}]\n\n{combined}".strip() + clock.extra["stale_observation"] = True from src.turn_contract import active_turn_contract contract = active_turn_contract() model_choice = getattr(contract, 'routing_experiment', '') == 'recent_model_choice' @@ -2806,13 +3092,15 @@ class PrivateBrowserTool: and action == "open" and (proc.returncode or 0) != 0 and self._retryable_local_open_failure(combined) - and not args.get("_odysseus_browser_retry") + and not retry + and deadline - loop.time() >= 10 ): - self._terminate_owned_chrome(env) - self._terminate_owned_daemon(env, session_id or None) - retry_args = dict(args) - retry_args["_odysseus_browser_retry"] = True - return await self.execute(json.dumps(retry_args), ctx) + self._discard_session(browser, env, session_id, clock, "bootstrap_failed") + clock.extra["recovery_attempts"] = 1 + return await self._execute_unlocked( + json.dumps(args), ctx, + browser=browser, clock=clock, retry=True, deadline=deadline, + ) page_errors = "" if ( verifies_local_html @@ -2893,7 +3181,7 @@ class PrivateBrowserTool: proc = None try: async with asyncio.timeout(max(0, deadline - loop.time())): - proc = await asyncio.create_subprocess_exec( + proc = await _spawn_browser_cli( *cmd_prefix, "batch", "--json", stdin=asyncio.subprocess.PIPE, stdout=asyncio.subprocess.PIPE, @@ -3044,7 +3332,7 @@ class PrivateBrowserTool: """Best-effort reset of state retained by a persistent browser session.""" proc = None try: - proc = await asyncio.create_subprocess_exec( + proc = await _spawn_browser_cli( *cmd_prefix, "close", stdout=asyncio.subprocess.PIPE, @@ -3070,7 +3358,7 @@ class PrivateBrowserTool: """Return bounded JavaScript errors from the current browser page.""" proc = None try: - proc = await asyncio.create_subprocess_exec( + proc = await _spawn_browser_cli( *cmd_prefix, "errors", stdout=asyncio.subprocess.PIPE, @@ -3114,7 +3402,7 @@ class PrivateBrowserTool: return None proc = None try: - proc = await asyncio.create_subprocess_exec( + proc = await _spawn_browser_cli( *command, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, @@ -3331,10 +3619,13 @@ class PrivateBrowserTool: if target: return [*prefix, "get", "text", target], None, None url = str(args.get("url") or "").strip() - command = [*prefix, "read"] + # agent-browser has no `read` command. Navigate and extract in one + # client call so the text is observed after this navigation. if url: - command.append(url) - return command, None, None + return [*prefix, "batch", "--json"], json.dumps( + [["open", url], ["get", "text", "body"]] + ), None + return [*prefix, "get", "text", "body"], None, None if action == "snapshot": return [*prefix, "snapshot"], None, None if action == "find": @@ -3465,7 +3756,13 @@ class PrivateBrowserTool: async def shutdown_private_browser_sessions() -> None: - """Close browser sessions owned by this Odysseus runtime namespace.""" + """Close and verify every browser session this runtime started. + + Each session is closed with the environment it was launched with, so its + runtime directory resolves to the daemon's own. ``close`` is sent only to + a verified live daemon, because against a missing one it bootstraps a new + browser. Forced cleanup of the session's own browser tree always follows. + """ binary = shutil.which("agent-browser") or PrivateBrowserTool._local_agent_browser_binary() command_prefix = [binary] if binary else ( @@ -3474,26 +3771,37 @@ async def shutdown_private_browser_sessions() -> None: sessions = sorted(_ACTIVE_BROWSER_SESSIONS) if not command_prefix or not sessions: return - env = dict(os.environ) - env["HOME"] = str(_service_home()) - env.setdefault("AGENT_BROWSER_IDLE_TIMEOUT_MS", "300000") + base_env = dict(os.environ) + base_env["HOME"] = str(_service_home()) + base_env.setdefault("AGENT_BROWSER_IDLE_TIMEOUT_MS", "300000") try: for session in sessions: - proc = None - try: - proc = await asyncio.create_subprocess_exec( - *command_prefix, "--session", session, "close", - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE, - env=env, - start_new_session=True, - ) - await asyncio.wait_for(proc.communicate(), timeout=20) - except Exception: - if proc is not None: - with contextlib.suppress(Exception): - PrivateBrowserTool._terminate_subprocess(proc) + record = browser_lifecycle.registered(session) + env = record.env if record is not None and record.env is not None else base_env + root = browser_lifecycle.runtime_root(env) + if browser_lifecycle.has_live_daemon( + root, session, pid_alive=lambda pid: _process_is_alive(pid) + ): + proc = None + try: + proc = await asyncio.create_subprocess_exec( + *command_prefix, "--session", session, "close", + stdout=asyncio.subprocess.DEVNULL, + stderr=asyncio.subprocess.DEVNULL, + env=env, + start_new_session=True, + ) + await asyncio.wait_for(proc.communicate(), timeout=20) + except Exception: + if proc is not None: + with contextlib.suppress(Exception): + PrivateBrowserTool._terminate_subprocess(proc) + browser_lifecycle.force_cleanup( + root, session, method="shutdown", + pid_alive=lambda pid: _process_is_alive(pid), + ) + browser_lifecycle.forget(session) finally: _ACTIVE_BROWSER_SESSIONS.difference_update(sessions) - PrivateBrowserTool._terminate_owned_chrome(env) - PrivateBrowserTool._terminate_owned_daemon(env) + PrivateBrowserTool._terminate_owned_chrome(base_env) + PrivateBrowserTool._terminate_owned_daemon(base_env) diff --git a/src/browser_lifecycle.py b/src/browser_lifecycle.py new file mode 100644 index 000000000..689048309 --- /dev/null +++ b/src/browser_lifecycle.py @@ -0,0 +1,397 @@ +"""Lifecycle ownership for private_browser's agent-browser sessions. + +agent-browser runs a short-lived CLI client against a detached daemon. The +daemon calls ``setsid`` and every Chrome process it launches stays in that +POSIX session, so the daemon pid recorded in the session's own pid file +identifies the complete browser tree. Cleanup here is limited to that tree, +the session's runtime files and its ``agent-browser-chrome-*`` profile. + +This is browser-specific ownership only. Generic process containment and +lifecycle primitives belong to the shared process layer; when those exist, +``kill_browser_tree`` is the single seam to replace. +""" + +from __future__ import annotations + +import asyncio +import os +import re +import shutil +import signal +import time +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any, Callable + +from core import platform_compat + +PROFILE_PREFIX = "agent-browser-chrome-" +RUNTIME_SUFFIXES = (".pid", ".sock", ".stream", ".version", ".engine") + +# Output that proves the browser never became ready. The daemon survives such +# a failure and a later ``close`` cannot reach a browser to shut down. +LAUNCH_FAILURE_RE = re.compile( + r"Chrome exited early|DevToolsActivePort|No usable sandbox|" + r"Failed to launch (?:the )?browser|Browser (?:process )?exited before", + re.IGNORECASE, +) + +OBSERVATION_ACTIONS = frozenset( + {"snapshot", "read", "find", "evaluate", "screenshot", "scroll", "wait"} +) + + +def runtime_root(env: dict[str, str] | None) -> Path: + """Directory where agent-browser keeps ``.pid`` and its socket. + + Mirrors agent-browser's own resolution for the environment the daemon is + launched with: an explicit socket directory, then the XDG runtime + directory, then ``$HOME/.agent-browser``. + """ + + source = env or {} + + def _get(name: str) -> str: + return str(source.get(name) or os.environ.get(name) or "").strip() + + socket_dir = _get("AGENT_BROWSER_SOCKET_DIR") + if socket_dir: + return Path(socket_dir) + xdg = _get("XDG_RUNTIME_DIR") + if xdg: + return Path(xdg) / "agent-browser" + home = _get("HOME") or str(Path.home()) + return Path(home) / ".agent-browser" + + +def _read_cmdline(pid: int) -> str | None: + try: + return (platform_compat.PROC_ROOT / str(pid) / "cmdline").read_bytes().replace( + b"\0", b" " + ).decode("utf-8", errors="replace") + except (OSError, UnicodeError): + return None + + +def _read_stat(pid: int) -> tuple[str, int, int] | None: + """Return ``(state, pgid, sid)`` for a pid, or ``None`` when unreadable.""" + + try: + raw = (platform_compat.PROC_ROOT / str(pid) / "stat").read_text() + except (OSError, UnicodeError): + return None + _, _, rest = raw.rpartition(")") + fields = rest.split() + if len(fields) < 4: + return None + try: + return fields[0], int(fields[2]), int(fields[3]) + except ValueError: + return None + + +def _live_pids() -> list[int]: + if not platform_compat.has_procfs(): + return [] + pids = [] + for entry in platform_compat.PROC_ROOT.iterdir(): + if entry.name.isdigit(): + pids.append(int(entry.name)) + return pids + + +def daemon_pid(root: Path, key: str) -> int | None: + """Pid recorded in this session's pid file, whether or not it is alive.""" + + try: + return int((root / f"{key}.pid").read_text().strip()) + except (OSError, ValueError): + return None + + +def is_verified_daemon(pid: int | None) -> bool: + """Whether ``pid`` is a live agent-browser process (requires procfs).""" + + if not pid: + return False + stat = _read_stat(pid) + if stat is not None and stat[0] == "Z": + return False + command_line = _read_cmdline(pid) + return bool(command_line) and "agent-browser" in command_line + + +def browser_tree(leader: int) -> list[int]: + """Processes owned by the browser session whose daemon pid is ``leader``. + + While the daemon is verified alive, every member of its POSIX session is + owned. Once the daemon is gone the pid may be reused, so only Chrome + process groups whose root carries an agent-browser profile are claimed. + """ + + members: list[tuple[int, int]] = [] + for pid in _live_pids(): + stat = _read_stat(pid) + if stat is None or stat[0] == "Z" or stat[2] != leader: + continue + members.append((pid, stat[1])) + if not members: + return [] + if is_verified_daemon(leader): + return sorted(pid for pid, _ in members) + owned_groups = { + pgid for pid, pgid in members if _profile_dirs([pid]) + } + return sorted(pid for pid, pgid in members if pgid in owned_groups) + + +def _profile_dirs(pids: list[int]) -> set[Path]: + profiles: set[Path] = set() + for pid in pids: + for token in (_read_cmdline(pid) or "").split(): + if not token.startswith("--user-data-dir="): + continue + path = Path(token.split("=", 1)[1]) + if path.name.startswith(PROFILE_PREFIX): + profiles.add(path) + return profiles + + +def kill_browser_tree(leader: int, *, settle_s: float = 1.0) -> tuple[list[int], list[int], set[Path]]: + """SIGKILL one browser session tree and wait briefly for it to exit. + + Returns ``(killed, survivors, profile_dirs)``. Synchronous so it can run + from cancellation and shutdown paths without awaiting. + """ + + members = browser_tree(leader) + profiles = _profile_dirs(members) + ordered = [pid for pid in members if pid != leader] + if leader in members: + ordered.append(leader) + killed = [] + for pid in ordered: + try: + os.kill(pid, signal.SIGKILL) + killed.append(pid) + except (ProcessLookupError, PermissionError, OSError): + continue + deadline = time.monotonic() + settle_s + survivors = list(killed) + while survivors and time.monotonic() < deadline: + survivors = [pid for pid in survivors if _alive(pid)] + if survivors: + time.sleep(0.02) + return killed, survivors, profiles + + +def _alive(pid: int) -> bool: + stat = _read_stat(pid) + if stat is None: + return False + return stat[0] != "Z" + + +@dataclass +class CleanupReceipt: + method: str + daemon_pid: int | None = None + killed: int = 0 + survivors: list[int] = field(default_factory=list) + removed_files: list[str] = field(default_factory=list) + removed_profiles: int = 0 + verified: bool = False + note: str = "" + + def as_dict(self) -> dict[str, Any]: + return { + "method": self.method, + "daemon_pid": self.daemon_pid, + "killed": self.killed, + "survivors": list(self.survivors), + "removed_files": list(self.removed_files), + "removed_profiles": self.removed_profiles, + "verified": self.verified, + **({"note": self.note} if self.note else {}), + } + + +def force_cleanup( + root: Path, + key: str, + *, + method: str = "forced", + pid_alive: Callable[[int], bool] = platform_compat.pid_alive, +) -> CleanupReceipt: + """Kill this session's browser tree and remove its owned resources. + + Without procfs nothing can be attributed safely, so live processes are + left alone and only the pid file of a dead daemon is forgotten. + """ + + pid = daemon_pid(root, key) + receipt = CleanupReceipt(method=method, daemon_pid=pid) + if not platform_compat.has_procfs(): + if pid and not pid_alive(pid): + _remove_runtime_files(root, key, receipt) + receipt.verified = True + else: + receipt.note = "procfs unavailable; browser ownership could not be verified" + return receipt + profiles: set[Path] = set() + if pid: + killed, survivors, profiles = kill_browser_tree(pid) + receipt.killed = len(killed) + receipt.survivors = survivors + if not receipt.survivors: + _remove_runtime_files(root, key, receipt) + for profile in profiles: + if profile.name.startswith(PROFILE_PREFIX) and profile.is_dir(): + shutil.rmtree(profile, ignore_errors=True) + if not profile.exists(): + receipt.removed_profiles += 1 + receipt.verified = not receipt.survivors and not (pid and browser_tree(pid)) + return receipt + + +def _remove_runtime_files(root: Path, key: str, receipt: CleanupReceipt) -> None: + for suffix in RUNTIME_SUFFIXES: + path = root / f"{key}{suffix}" + try: + path.unlink() + receipt.removed_files.append(path.name) + except FileNotFoundError: + continue + except OSError: + continue + + +class StageClock: + """Ordered stage timings for one browser call.""" + + def __init__(self) -> None: + self.stages: list[dict[str, Any]] = [] + self.extra: dict[str, Any] = {} + self._start = time.monotonic() + + def record(self, stage: str, started: float, ok: bool, **detail: Any) -> None: + entry = { + "stage": stage, + "ms": int((time.monotonic() - started) * 1000), + "ok": bool(ok), + } + entry.update({k: v for k, v in detail.items() if v not in (None, "")}) + self.stages.append(entry) + + def total_ms(self) -> int: + return int((time.monotonic() - self._start) * 1000) + + +@dataclass +class BrowserSession: + """In-process lifecycle record for one owned agent-browser session.""" + + key: str + ephemeral: bool + root: Path | None = None + env: dict[str, str] | None = field(default=None, repr=False) + command_prefix: list[str] = field(default_factory=list, repr=False) + state: str = "idle" + navigation_generation: int = 0 + page_url: str = "" + failed_navigation_url: str = "" + _lock: asyncio.Lock | None = field(default=None, repr=False) + _lock_loop: Any = field(default=None, repr=False) + + def bind(self, env: dict[str, str], command_prefix: list[str]) -> None: + """Record the environment and CLI prefix the daemon is launched with.""" + + self.env = dict(env) + self.root = runtime_root(env) + self.command_prefix = list(command_prefix) + + def lock(self) -> asyncio.Lock: + loop = asyncio.get_running_loop() + if self._lock is None or self._lock_loop is not loop: + self._lock = asyncio.Lock() + self._lock_loop = loop + return self._lock + + def navigated(self, url: str) -> None: + self.navigation_generation += 1 + self.page_url = url + self.failed_navigation_url = "" + self.state = "ready" + + def navigation_failed(self, url: str) -> None: + self.failed_navigation_url = url + self.state = "navigation_failed" + + def discarded(self, state: str) -> None: + """The browser and its page are gone; nothing earlier is observable.""" + + self.page_url = "" + self.failed_navigation_url = "" + self.state = state + + def stale_observation_note(self) -> str: + if not self.failed_navigation_url: + return "" + shown = self.page_url or "an earlier page" + return ( + f"Browser lifecycle: the most recent navigation to {self.failed_navigation_url} " + f"failed. This observation shows {shown} (navigation " + f"#{self.navigation_generation}), not {self.failed_navigation_url}." + ) + + def receipt(self, clock: StageClock) -> dict[str, Any]: + payload = { + "session": self.key, + "ownership": "ephemeral" if self.ephemeral else "retained", + "state": self.state, + "navigation_generation": self.navigation_generation, + "page_url": self.page_url, + "stages": clock.stages, + "elapsed_ms": clock.total_ms(), + } + payload.update({k: v for k, v in clock.extra.items() if v is not None}) + return payload + + +_SESSIONS: dict[str, BrowserSession] = {} + + +def session_for(key: str, ephemeral: bool) -> BrowserSession: + record = _SESSIONS.get(key) + if record is None: + record = BrowserSession(key=key, ephemeral=ephemeral) + _SESSIONS[key] = record + return record + + +def forget(key: str) -> None: + _SESSIONS.pop(key, None) + + +def registered(key: str) -> BrowserSession | None: + return _SESSIONS.get(key) + + +def has_live_daemon( + root: Path, + key: str, + *, + pid_alive: Callable[[int], bool] = platform_compat.pid_alive, +) -> bool: + """Whether this session has a daemon a ``close`` command could reach. + + Without procfs a live pid from our own pid file is treated as a match, + because answering "no daemon" lets ``close`` bootstrap a fresh browser. + """ + + pid = daemon_pid(root, key) + if not pid: + return False + if not platform_compat.has_procfs(): + return pid_alive(pid) + return is_verified_daemon(pid) diff --git a/src/mcp_manager.py b/src/mcp_manager.py index 1ba563170..21d9e9cad 100644 --- a/src/mcp_manager.py +++ b/src/mcp_manager.py @@ -17,6 +17,18 @@ from src.runtime_paths import get_app_root logger = logging.getLogger(__name__) +BROWSER_MCP_SERVER_ID = "builtin_browser" + + +def browser_mcp_call_timeout() -> float: + """Upper bound for one Playwright MCP tool call, in seconds.""" + + try: + value = float(os.environ.get("ODYSSEUS_BROWSER_MCP_CALL_TIMEOUT_S", "90")) + except ValueError: + return 90.0 + return value if value > 0 else 90.0 + def _format_mcp_connection_error(name: str, command: str = "", args: Optional[List[str]] = None, error: Exception = None) -> str: """Return a user-actionable MCP connection error message.""" args = args or [] @@ -521,6 +533,24 @@ class McpManager: return {"error": f"MCP server not connected: {server_id}", "exit_code": 1} try: + if server_id == BROWSER_MCP_SERVER_ID: + # The shared Playwright browser must not hold a turn forever. + # The call is abandoned, not retried: page state is unknown. + limit = browser_mcp_call_timeout() + try: + return await asyncio.wait_for( + self._do_call(session, tool_name, arguments), timeout=limit + ) + except asyncio.TimeoutError: + logger.warning("Browser MCP call %s timed out after %ss", tool_name, limit) + return { + "error": ( + f"Browser call {tool_name} timed out after {limit:g}s and was " + "not retried. The current page state is unknown; navigate " + "again before relying on any observation." + ), + "exit_code": 1, + } result = await self._do_call(session, tool_name, arguments) except Exception as e: # Auto-reconnect for builtin servers whose subprocess may have died diff --git a/src/research_navigator.py b/src/research_navigator.py index c1f26d7ec..5738e0d4d 100644 --- a/src/research_navigator.py +++ b/src/research_navigator.py @@ -211,7 +211,7 @@ class ResearchNavigator: self._progress({"phase": "navigating", "url": url, "title": url}) tool = PrivateBrowserTool() result = await tool.execute( - json.dumps({"action": "read", "url": url, "timeout": timeout}), + json.dumps({"action": "read", "url": url, "timeout_ms": int(timeout * 1000)}), {"session_id": self.session_id or "research"}, ) output = str(result.get("output") or "").strip() diff --git a/tests/test_browser_lifecycle.py b/tests/test_browser_lifecycle.py new file mode 100644 index 000000000..5b8889130 --- /dev/null +++ b/tests/test_browser_lifecycle.py @@ -0,0 +1,581 @@ +"""Wave 5A browser lifecycle: ownership, cleanup, recovery and freshness.""" + +import asyncio +import json +import os +import shutil +import signal +import tempfile +import time +from pathlib import Path + +import pytest + +from core import platform_compat +import src.agent_tools.web_tools as web_tools +from src import browser_lifecycle +from src.agent_tools.web_tools import PrivateBrowserTool + + +def _fake_proc(root: Path, pid: int, *, ppid: int, pgid: int, sid: int, cmdline: str, state: str = "S") -> None: + entry = root / str(pid) + entry.mkdir(parents=True) + (entry / "stat").write_text(f"{pid} (x y) {state} {ppid} {pgid} {sid} 0 0 0") + (entry / "cmdline").write_bytes(cmdline.replace(" ", "\0").encode()) + + +@pytest.fixture +def fake_procfs(monkeypatch, tmp_path): + proc = tmp_path / "proc" + proc.mkdir() + monkeypatch.setattr(platform_compat, "PROC_ROOT", proc) + killed: list[tuple[int, int]] = [] + + def _kill(pid, sig): + entry = proc / str(pid) + if not entry.exists(): + raise ProcessLookupError(pid) + killed.append((pid, sig)) + shutil.rmtree(entry) + + monkeypatch.setattr(browser_lifecycle.os, "kill", _kill) + return proc, killed + + +def _browser_tree(proc: Path, profile: Path, *, daemon: int = 500, extra_sid: int = 900) -> None: + _fake_proc(proc, daemon, ppid=1, pgid=daemon, sid=daemon, cmdline="/x/agent-browser-linux-x64") + _fake_proc(proc, daemon + 1, ppid=daemon, pgid=daemon + 1, sid=daemon, + cmdline=f"chrome --no-sandbox --user-data-dir={profile}") + _fake_proc(proc, daemon + 2, ppid=daemon + 1, pgid=daemon + 1, sid=daemon, cmdline="chrome --type=renderer") + _fake_proc(proc, extra_sid, ppid=1, pgid=extra_sid, sid=extra_sid, cmdline="chrome --type=renderer") + + +def test_runtime_root_follows_agent_browser_resolution(monkeypatch, tmp_path) -> None: + for name in ("AGENT_BROWSER_SOCKET_DIR", "XDG_RUNTIME_DIR"): + monkeypatch.delenv(name, raising=False) + assert browser_lifecycle.runtime_root({"HOME": str(tmp_path)}) == tmp_path / ".agent-browser" + assert browser_lifecycle.runtime_root( + {"HOME": str(tmp_path), "XDG_RUNTIME_DIR": "/run/x"} + ) == Path("/run/x/agent-browser") + assert browser_lifecycle.runtime_root( + {"XDG_RUNTIME_DIR": "/run/x", "AGENT_BROWSER_SOCKET_DIR": "/s"} + ) == Path("/s") + + +def test_live_daemon_owns_its_whole_session_and_nothing_else(fake_procfs, tmp_path) -> None: + proc, _ = fake_procfs + _browser_tree(proc, tmp_path / "agent-browser-chrome-a") + + assert browser_lifecycle.browser_tree(500) == [500, 501, 502] + + +def test_orphaned_tree_is_claimed_only_through_its_browser_profile(fake_procfs, tmp_path) -> None: + proc, _ = fake_procfs + _browser_tree(proc, tmp_path / "agent-browser-chrome-a") + shutil.rmtree(proc / "500") + # A reused pid's session without an agent-browser profile is not ours. + _fake_proc(proc, 700, ppid=1, pgid=700, sid=500, cmdline="bash") + + assert browser_lifecycle.browser_tree(500) == [501, 502] + + +def test_forced_cleanup_kills_tree_and_removes_owned_resources(fake_procfs, tmp_path) -> None: + proc, killed = fake_procfs + root = tmp_path / "rt" + root.mkdir() + profile = tmp_path / "agent-browser-chrome-a" + profile.mkdir() + (profile / "Default").mkdir() + for suffix in browser_lifecycle.RUNTIME_SUFFIXES: + (root / f"ody-k{suffix}").write_text("500") + (root / "ody-other.pid").write_text("900") + _browser_tree(proc, profile) + + receipt = browser_lifecycle.force_cleanup(root, "ody-k") + + assert [pid for pid, _ in killed] == [501, 502, 500] + assert all(sig == signal.SIGKILL for _, sig in killed) + assert receipt.verified and receipt.killed == 3 and receipt.removed_profiles == 1 + assert not profile.exists() + assert sorted(p.name for p in root.iterdir()) == ["ody-other.pid"] + assert (proc / "900").exists() + + +def test_forced_cleanup_without_procfs_never_kills_unverified_processes(monkeypatch, tmp_path) -> None: + monkeypatch.setattr(platform_compat, "PROC_ROOT", tmp_path / "missing") + monkeypatch.setattr(browser_lifecycle.os, "kill", lambda *a: pytest.fail("killed")) + root = tmp_path / "rt" + root.mkdir() + (root / "ody-k.pid").write_text("4242") + + live = browser_lifecycle.force_cleanup(root, "ody-k", pid_alive=lambda pid: True) + assert not live.verified and (root / "ody-k.pid").exists() + + dead = browser_lifecycle.force_cleanup(root, "ody-k", pid_alive=lambda pid: False) + assert dead.verified and not (root / "ody-k.pid").exists() + + +class _Proc: + """Fake agent-browser CLI client driven by a per-test behaviour.""" + + def __init__(self, command, kwargs, behaviour): + self.command = list(command) + self.kwargs = kwargs + self.behaviour = behaviour + self.returncode = None + self.pid = None + + async def communicate(self, stdin=None): + rc, out = await self.behaviour(self.command) + self.returncode = rc + target = self.kwargs.get("stdout") + if hasattr(target, "write"): + target.write(out.encode()) + return None, None + return out.encode(), b"" + + async def wait(self): + await self.communicate() + return self.returncode + + def kill(self): + self.returncode = -9 + + +@pytest.fixture +def browser_env(monkeypatch, tmp_path): + monkeypatch.setattr(web_tools.shutil, "which", lambda name: "/usr/bin/agent-browser") + monkeypatch.setattr(PrivateBrowserTool, "_AUTO_SCREENSHOT_ACTIONS", set()) + monkeypatch.setattr("src.tool_execution.get_active_workspace", lambda: str(tmp_path)) + monkeypatch.setenv("XDG_RUNTIME_DIR", str(tmp_path / "xdg")) + swept = [] + monkeypatch.setattr(PrivateBrowserTool, "_terminate_owned_chrome", staticmethod(lambda env: swept.append(env))) + cleaned = [] + + def _cleanup(env, session_id=None): + cleaned.append(session_id) + return {"method": "forced", "verified": True} + + monkeypatch.setattr(PrivateBrowserTool, "_terminate_owned_daemon", staticmethod(_cleanup)) + calls = [] + state = {"behaviour": None} + + async def _spawn(*command, **kwargs): + calls.append(list(command)) + return _Proc(command, kwargs, state["behaviour"]) + + monkeypatch.setattr(asyncio, "create_subprocess_exec", _spawn) + return state, calls, cleaned, swept + + +def _run(payload, ctx): + return asyncio.run(PrivateBrowserTool().execute(json.dumps(payload), ctx)) + + +def test_timeout_cleans_only_this_sessions_browser(browser_env) -> None: + state, calls, cleaned, swept = browser_env + + async def _hang(command): + raise asyncio.TimeoutError() + + state["behaviour"] = _hang + result = _run({"action": "open", "url": "https://example.com"}, {"session_id": "s-timeout"}) + + assert result["exit_code"] == 1 and "timed out" in result["error"] + assert cleaned == ["s-timeout"] + assert swept == [], "a per-session timeout must not sweep other sessions' Chrome" + lifecycle = result["browser_lifecycle"] + assert lifecycle["state"] == "timed_out" + assert lifecycle["cleanup"]["verified"] is True + assert [stage["stage"] for stage in lifecycle["stages"]] == ["open", "forced_cleanup"] + assert sum(1 for call in calls if "open" in call) == 1, "remote opens are never retried" + + +def test_launch_failure_is_reported_and_cleaned(browser_env) -> None: + state, _, cleaned, _ = browser_env + + async def _no_sandbox(command): + return 1, ("Chrome exited early (exit code: unknown) without writing DevToolsActivePort\n" + "FATAL: No usable sandbox!") + + state["behaviour"] = _no_sandbox + result = _run({"action": "open", "url": "https://example.com"}, {"session_id": "s-launch"}) + + assert result["exit_code"] == 1 + assert "could not launch the browser" in result["error"] + assert cleaned == ["s-launch"] + assert result["browser_lifecycle"]["state"] == "launch_failed" + assert result["browser_lifecycle"]["navigation_generation"] == 0 + + +def test_observation_after_failed_navigation_is_marked_stale(browser_env) -> None: + state, _, _, _ = browser_env + + async def _behaviour(command): + if command[-2:] == ["open", "https://good.example/"]: + return 0, "✓ Good\n https://good.example/\n" + if "open" in command: + return 1, "net::ERR_NAME_NOT_RESOLVED" + return 0, '- heading "Good page" [ref=e1]' + + state["behaviour"] = _behaviour + ctx = {"session_id": "s-stale"} + opened = _run({"action": "open", "url": "https://good.example/"}, ctx) + assert opened["browser_lifecycle"]["navigation_generation"] == 1 + assert opened["browser_lifecycle"]["page_url"] == "https://good.example/" + + failed = _run({"action": "open", "url": "https://bad.example/"}, ctx) + assert failed["exit_code"] == 1 + assert failed["browser_lifecycle"]["state"] == "navigation_failed" + + observed = _run({"action": "snapshot"}, ctx) + assert observed["output"].startswith("[Browser lifecycle: the most recent navigation to https://bad.example/ failed") + assert "shows https://good.example/ (navigation #1)" in observed["output"] + assert observed["browser_lifecycle"]["stale_observation"] is True + + _run({"action": "open", "url": "https://good.example/"}, ctx) + fresh = _run({"action": "snapshot"}, ctx) + assert not fresh["output"].startswith("[Browser lifecycle") + assert "stale_observation" not in fresh["browser_lifecycle"] + + +def test_sessionless_call_gets_its_own_browser_and_closes_it(browser_env, monkeypatch) -> None: + state, calls, cleaned, _ = browser_env + monkeypatch.setattr(PrivateBrowserTool, "_owned_daemon_exists", staticmethod(lambda env, session: True)) + + async def _ok(command): + return 0, "✓ T\n https://example.com/\n" + + state["behaviour"] = _ok + first = _run({"action": "open", "url": "https://example.com/"}, {}) + second = _run({"action": "open", "url": "https://example.com/"}, {}) + + sessions = [call[call.index("--session") + 1] for call in calls if "--session" in call] + assert all(session.startswith("ody-") for session in sessions) + assert len({sessions[0], sessions[-1]}) == 2, "sessionless calls must not share a browser" + assert any(call[-1] == "close" for call in calls) + assert first["browser_lifecycle"]["ownership"] == "ephemeral" + assert first["browser_lifecycle"]["cleanup"]["graceful_close"] is True + assert first["browser_lifecycle"]["state"] == "closed" + assert len(cleaned) == 2 + assert not web_tools._ACTIVE_BROWSER_SESSIONS.intersection(sessions) + assert not any(browser_lifecycle.registered(s) for s in sessions) + assert second["exit_code"] == 0 + + +def test_actions_on_one_session_are_serialized(browser_env) -> None: + state, _, _, _ = browser_env + active = {"now": 0, "peak": 0} + + async def _slow(command): + active["now"] += 1 + active["peak"] = max(active["peak"], active["now"]) + await asyncio.sleep(0.02) + active["now"] -= 1 + return 0, '- heading "x"' + + state["behaviour"] = _slow + + async def _both(): + tool = PrivateBrowserTool() + await asyncio.gather( + tool.execute(json.dumps({"action": "snapshot"}), {"session_id": "s-lock"}), + tool.execute(json.dumps({"action": "snapshot"}), {"session_id": "s-lock"}), + ) + + asyncio.run(_both()) + assert active["peak"] == 1 + + +def test_cancellation_stops_clients_and_cleans_the_session(browser_env, monkeypatch) -> None: + state, calls, cleaned, _ = browser_env + terminated = [] + + async def _forever(command): + await asyncio.sleep(3600) + + state["behaviour"] = _forever + monkeypatch.setattr( + PrivateBrowserTool, "_terminate_subprocess", + staticmethod(lambda proc: terminated.append(proc.command)), + ) + + async def _cancel(): + task = asyncio.create_task(PrivateBrowserTool().execute( + json.dumps({"action": "open", "url": "https://example.com"}), + {"session_id": "s-cancel"}, + )) + while not calls: + await asyncio.sleep(0.01) + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + + asyncio.run(_cancel()) + + assert terminated and terminated[0][-1] == "https://example.com" + assert cleaned == ["s-cancel"] + key = web_tools._scoped_browser_session("odysseus-ui", "s-cancel") + assert browser_lifecycle.registered(key).state == "cancelled" + + +def test_local_open_recovery_is_single_and_inside_the_deadline(browser_env, monkeypatch, tmp_path) -> None: + state, calls, cleaned, _ = browser_env + page = tmp_path / "page.html" + page.write_text("x") + + async def _hang(command): + raise asyncio.TimeoutError() + + state["behaviour"] = _hang + payload = {"action": "open", "url": "/workspace/page.html", "_odysseus_browser_retry": True} + result = _run(payload, {"session_id": "s-retry"}) + + opens = [call for call in calls if call[-1] == page.as_uri()] + assert len(opens) == 2, "a model-supplied retry flag must not change recovery" + assert result["browser_lifecycle"]["recovery_attempts"] == 1 + assert cleaned == ["s-retry", "s-retry"] + + calls.clear() + monkeypatch.setattr(PrivateBrowserTool, "_RECOVERY_BUDGET_S", 0) + exhausted = _run({"action": "open", "url": "/workspace/page.html", "timeout_ms": 1000}, {"session_id": "s-budget"}) + assert len([call for call in calls if call[-1] == page.as_uri()]) == 1 + assert "recovery_attempts" not in exhausted["browser_lifecycle"] + + +def test_research_reader_passes_its_timeout_to_the_browser(monkeypatch) -> None: + from src.research_navigator import ResearchNavigator + + seen = {} + + async def _execute(self, content, ctx): + seen.update(json.loads(content)) + return {"output": "", "exit_code": 1} + + monkeypatch.setattr(PrivateBrowserTool, "execute", _execute) + navigator = ResearchNavigator.__new__(ResearchNavigator) + navigator._progress = None + navigator.session_id = "r" + asyncio.run(navigator.browser_read("https://example.com", timeout=12)) + + assert seen["timeout_ms"] == 12000 + + +# Real browser: open/extract a local HTML page, then prove the cleanup paths +# leave no process, profile or runtime file behind. + +def _real_browser(): + binary = shutil.which("agent-browser") or PrivateBrowserTool._local_agent_browser_binary() + chrome = web_tools._browser_executable_candidates() + if not binary or not chrome or not platform_compat.has_procfs(): + return None + return binary, str(chrome[0]) + + +REAL = _real_browser() +real_browser = pytest.mark.skipif(REAL is None, reason="agent-browser and Chromium are not installed") + + +@pytest.fixture +def real_runtime(monkeypatch, tmp_path): + # agent-browser's Unix socket path must stay under ~103 bytes. + runtime = Path(tempfile.mkdtemp(prefix="abt", dir="/tmp")) + workspace = tmp_path / "ws" + workspace.mkdir() + monkeypatch.setattr("src.tool_execution.get_active_workspace", lambda: str(workspace)) + monkeypatch.setattr(web_tools.shutil, "which", lambda name: REAL[0] if name == "agent-browser" else shutil.which(name)) + env = { + "XDG_RUNTIME_DIR": str(runtime), + "TMPDIR": str(runtime / "tmp"), + "AGENT_BROWSER_EXECUTABLE_PATH": REAL[1], + "AGENT_BROWSER_IDLE_TIMEOUT_MS": "60000", + # Hosts that restrict unprivileged user namespaces cannot start + # Chrome's sandbox. Test-only; production launch flags are unchanged. + "AGENT_BROWSER_ARGS": "--no-sandbox", + } + (runtime / "tmp").mkdir() + yield workspace, runtime, env + for pid_file in (runtime / "agent-browser").glob("*.pid"): + browser_lifecycle.force_cleanup(runtime / "agent-browser", pid_file.stem) + shutil.rmtree(runtime, ignore_errors=True) + + +def _owned_processes(runtime: Path) -> list[int]: + owned = [] + for entry in platform_compat.PROC_ROOT.iterdir(): + if not entry.name.isdigit(): + continue + try: + text = (entry / "cmdline").read_bytes().decode(errors="replace") + except OSError: + continue + if str(runtime) in text: + owned.append(int(entry.name)) + return owned + + +@real_browser +def test_real_local_page_open_extract_and_ephemeral_cleanup(real_runtime) -> None: + workspace, runtime, env = real_runtime + (workspace / "page.html").write_text( + "Lifecycle

Fresh heading

" + ) + + result = _run( + {"action": "batch", "commands": [["open", "/workspace/page.html"], ["snapshot"]]}, + {"subproc_env": env}, + ) + + assert result["exit_code"] == 0, result + assert "Fresh heading" in result["output"] + lifecycle = result["browser_lifecycle"] + assert lifecycle["ownership"] == "ephemeral" + assert lifecycle["navigation_generation"] == 1 + assert lifecycle["state"] == "closed" and lifecycle["page_url"] == "" + assert lifecycle["closed_page_url"].endswith("/page.html") + assert lifecycle["cleanup"]["verified"] is True + assert [stage["stage"] for stage in lifecycle["stages"]] == ["batch", "close"] + time.sleep(0.5) + assert _owned_processes(runtime) == [] + assert list((runtime / "agent-browser").glob("ody-*")) == [] + assert list((runtime / "tmp").glob("agent-browser-chrome-*")) == [] + + +@real_browser +def test_real_retained_session_survives_then_forced_cleanup_leaves_nothing(real_runtime) -> None: + workspace, runtime, env = real_runtime + (workspace / "a.html").write_text("A

Alpha

") + ctx = {"session_id": "retained", "subproc_env": env} + + opened = _run({"action": "open", "url": "/workspace/a.html"}, ctx) + assert opened["exit_code"] == 0, opened + observed = _run({"action": "snapshot"}, ctx) + assert "Alpha" in observed["output"] + assert observed["browser_lifecycle"]["ownership"] == "retained" + assert _owned_processes(runtime), "a retained session keeps its browser" + + receipt = PrivateBrowserTool._terminate_owned_daemon(dict(os.environ, **env), "retained") + + assert receipt["verified"] is True and receipt["killed"] >= 2 + assert receipt["removed_profiles"] == 1 + assert _owned_processes(runtime) == [] + assert list((runtime / "agent-browser").glob("ody-*")) == [] + + +@real_browser +def test_real_cancellation_leaves_no_browser(real_runtime) -> None: + workspace, runtime, env = real_runtime + (workspace / "slow.html").write_text("S

Slow

") + ctx = {"session_id": "cancelled", "subproc_env": env} + assert _run({"action": "open", "url": "/workspace/slow.html"}, ctx)["exit_code"] == 0 + + async def _cancel_wait(): + task = asyncio.create_task(PrivateBrowserTool().execute( + json.dumps({"action": "wait", "timeout_ms": 30000}), ctx, + )) + await asyncio.sleep(1.5) + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + + asyncio.run(_cancel_wait()) + time.sleep(0.5) + assert _owned_processes(runtime) == [] + assert list((runtime / "agent-browser").glob("ody-*")) == [] + + +def test_browser_mcp_call_is_bounded_and_never_replayed(monkeypatch) -> None: + from src.mcp_manager import McpManager + + manager = McpManager() + calls = [] + + class _Session: + async def call_tool(self, name, arguments): + calls.append(name) + await asyncio.sleep(3600) + + manager._sessions["builtin_browser"] = _Session() + monkeypatch.setenv("ODYSSEUS_BROWSER_MCP_CALL_TIMEOUT_S", "0.05") + + result = asyncio.run(manager.call_tool( + "mcp__builtin_browser__browser_navigate", {"url": "https://example.com"} + )) + + assert result["exit_code"] == 1 + assert "timed out after 0.05s and was not retried" in result["error"] + assert calls == ["browser_navigate"] + + +def test_read_url_navigates_and_extracts_in_one_observation(browser_env) -> None: + state, calls, _, _ = browser_env + + async def _batch(command): + return 0, json.dumps([ + {"command": ["open", "https://example.com/"], "success": True, + "result": {"title": "Example", "url": "https://example.com/final"}}, + {"command": ["get", "text", "body"], "success": True, + "result": {"text": "Example body"}}, + ]) + + state["behaviour"] = _batch + result = _run({"action": "read", "url": "https://example.com/"}, {"session_id": "s-read"}) + + assert calls[-1][-2:] == ["batch", "--json"] + assert result["exit_code"] == 0 + assert result["output"] == "Example\nhttps://example.com/final\n\nExample body" + assert result["browser_lifecycle"]["page_url"] == "https://example.com/final" + + +def test_read_url_without_extracted_text_is_a_failure(browser_env) -> None: + state, _, _, _ = browser_env + + async def _no_text(command): + return 0, json.dumps([ + {"success": True, "result": {"url": "https://example.com/"}}, + {"success": False, "error": "Timeout waiting for body", "result": None}, + ]) + + state["behaviour"] = _no_text + result = _run({"action": "read", "url": "https://example.com/"}, {"session_id": "s-read-fail"}) + + assert result["exit_code"] == 1 + assert "Timeout waiting for body" in result["error"] + assert result["browser_lifecycle"]["state"] == "navigation_failed" + + +@real_browser +def test_real_read_url_extracts_text_after_navigation(real_runtime) -> None: + import functools + import http.server + import threading + + workspace, runtime, env = real_runtime + (workspace / "doc.html").write_text("Doc

Served heading

Body text

") + handler = functools.partial(http.server.SimpleHTTPRequestHandler, directory=str(workspace)) + server = http.server.ThreadingHTTPServer(("127.0.0.1", 0), handler) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + url = f"http://127.0.0.1:{server.server_address[1]}/doc.html" + result = _run({"action": "read", "url": url}, {"subproc_env": env}) + finally: + server.shutdown() + server.server_close() + + assert result["exit_code"] == 0, result + assert result["output"].startswith(f"Doc\n{url}") + assert "Served heading" in result["output"] and "Body text" in result["output"] + assert result["browser_lifecycle"]["closed_page_url"] == url + assert result["browser_lifecycle"]["cleanup"]["verified"] is True + time.sleep(0.5) + assert _owned_processes(runtime) == [] + + +def test_selector_read_is_an_observation_not_a_navigation() -> None: + assert PrivateBrowserTool._navigation_target( + "read", {"selector": "#main", "url": "https://elsewhere.example/"} + ) == "" + assert PrivateBrowserTool._navigation_target( + "batch", {"commands": [["open", "file:///a.html"], ["snapshot"], ["open", "file:///b.html"]]} + ) == "file:///b.html" diff --git a/tests/test_private_browser_tool.py b/tests/test_private_browser_tool.py index b212b2f47..8163c43df 100644 --- a/tests/test_private_browser_tool.py +++ b/tests/test_private_browser_tool.py @@ -951,12 +951,19 @@ def test_private_browser_reuses_host_npx_cache_when_task_home_is_isolated( class _FakeProc: returncode = 0 + def __init__(self, stdout): + self.stdout = stdout + async def communicate(self, stdin=None): - return b"page text", b"" + self.stdout.write(json.dumps([ + {"success": True, "result": {"title": "T", "url": "https://example.com/"}}, + {"success": True, "result": {"text": "page text"}}, + ]).encode()) + return None, None async def _fake_create_subprocess_exec(*command, **kwargs): calls["env"] = kwargs["env"] - return _FakeProc() + return _FakeProc(kwargs["stdout"]) monkeypatch.setattr( asyncio, "create_subprocess_exec", _fake_create_subprocess_exec @@ -1049,7 +1056,7 @@ def test_browser_executable_discovery_supports_chromium_snapshot_cache( assert web_tools._browser_executable_candidates() == [chrome] -def test_private_browser_shutdown_is_namespace_scoped(monkeypatch) -> None: +def test_private_browser_shutdown_is_namespace_scoped(monkeypatch, tmp_path) -> None: calls = {} class _FakeProc: @@ -1058,9 +1065,18 @@ def test_private_browser_shutdown_is_namespace_scoped(monkeypatch) -> None: monkeypatch.setattr(web_tools.shutil, "which", lambda name: "/bin/agent-browser") monkeypatch.setenv("ODYSSEUS_BROWSER_NAMESPACE", "clawmm-test") + monkeypatch.setenv("XDG_RUNTIME_DIR", str(tmp_path)) + monkeypatch.delenv("AGENT_BROWSER_SOCKET_DIR", raising=False) web_tools._ACTIVE_BROWSER_SESSIONS.clear() session = web_tools._scoped_browser_session("clawmm-test", "session-1") web_tools._ACTIVE_BROWSER_SESSIONS.add(session) + (tmp_path / "agent-browser").mkdir() + (tmp_path / "agent-browser" / f"{session}.pid").write_text("7001") + proc = tmp_path / "proc" + (proc / "7001").mkdir(parents=True) + (proc / "7001" / "cmdline").write_bytes(b"agent-browser-linux-x64\0") + monkeypatch.setattr(platform_compat, "PROC_ROOT", proc) + monkeypatch.setattr(web_tools.os, "kill", lambda pid, sig: None) async def _fake_create_subprocess_exec(*command, **kwargs): calls["command"] = command @@ -1076,6 +1092,27 @@ def test_private_browser_shutdown_is_namespace_scoped(monkeypatch) -> None: assert not web_tools._ACTIVE_BROWSER_SESSIONS +def test_private_browser_shutdown_never_bootstraps_a_missing_daemon( + monkeypatch, tmp_path +) -> None: + monkeypatch.setattr(web_tools.shutil, "which", lambda name: "/bin/agent-browser") + monkeypatch.setenv("XDG_RUNTIME_DIR", str(tmp_path)) + monkeypatch.setattr(platform_compat, "PROC_ROOT", tmp_path / "proc") + (tmp_path / "proc").mkdir() + web_tools._ACTIVE_BROWSER_SESSIONS.clear() + web_tools._ACTIVE_BROWSER_SESSIONS.add( + web_tools._scoped_browser_session("odysseus-ui", "never-started") + ) + + async def _no_spawn(*command, **kwargs): + pytest.fail(f"close would start a fresh daemon: {command}") + + monkeypatch.setattr(asyncio, "create_subprocess_exec", _no_spawn) + asyncio.run(shutdown_private_browser_sessions()) + + assert not web_tools._ACTIVE_BROWSER_SESSIONS + + def test_browser_pid_candidates_include_upstream_root_session() -> None: runtime = Path("/run/user/1000") namespace = "clawmm-test" diff --git a/website/configuration-reference.md b/website/configuration-reference.md index 69b3bea60..b5d8b7a6b 100644 --- a/website/configuration-reference.md +++ b/website/configuration-reference.md @@ -21,7 +21,7 @@ described as a switch that turns something off, the read rejects `0`, `false`, `no` and `off` and treats everything else as on. The `Default` column is the value the code falls back to when the variable is unset, quoted from the source. -The source tree reads **108** `ODYSSEUS_*` variables: 78 an operator may want to set, and 30 that are internal - sentinels, fixture switches, capture hooks and development tooling. The internal ones are listed too, in their own section, so this page can be checked against the source mechanically. +The source tree reads **109** `ODYSSEUS_*` variables: 79 an operator may want to set, and 30 that are internal - sentinels, fixture switches, capture hooks and development tooling. The internal ones are listed too, in their own section, so this page can be checked against the source mechanically. > This page is generated. Edit `scripts/generate_env_reference.py` and > re-run it; `tests/test_env_reference.py` enforces that the committed page @@ -86,10 +86,11 @@ The source tree reads **108** `ODYSSEUS_*` variables: 78 an operator may want to | `ODYSSEUS_BROWSER_EXECUTABLE` | `''` | `src/builtin_mcp.py:114` | Absolute path to the Chrome or Chromium binary. Empty searches the usual names, then lets Playwright MCP pick its own browser. | | `ODYSSEUS_BROWSER_ISOLATED` | `'1'` | `src/builtin_mcp.py:139` | Security-relevant. On by default, adding `--isolated` so each browser session starts clean. Set 0, false or no to keep a persistent profile. | | `ODYSSEUS_BROWSER_MCP_CACHE` | `os.path.join(base_dir, 'data', 'local', 'playwright-mcp-cache')` | `src/builtin_mcp.py:229` | Cache directory handed to the browser MCP server, so its npm download survives a container rebuild. | +| `ODYSSEUS_BROWSER_MCP_CALL_TIMEOUT_S` | `'90'` | `src/mcp_manager.py:27` | Upper bound in seconds for one browser MCP tool call. A call that exceeds it fails without being retried. | | `ODYSSEUS_BROWSER_MCP_REQUIRE_CACHE` | `''` | `src/builtin_mcp.py:90` | Truthy refuses to start the browser MCP server unless its npm package is already in the npx cache, instead of installing it at startup. | -| `ODYSSEUS_BROWSER_NAMESPACE` | `'odysseus-ui'` | `src/agent_tools/web_tools.py:2441` (+5 more) | Namespace for the detached agent-browser daemon's pid files, so two runtimes on one machine do not terminate each other's browsers. | +| `ODYSSEUS_BROWSER_NAMESPACE` | `'odysseus-ui'` | `src/agent_tools/web_tools.py:100` (+3 more) | Namespace for the detached agent-browser daemon's pid files, so two runtimes on one machine do not terminate each other's browsers. | | `ODYSSEUS_BROWSER_NO_SANDBOX` | `'1'` | `src/builtin_mcp.py:142` | Security-relevant. On by default, adding `--no-sandbox` because the Docker image cannot use the Chromium sandbox. Set 0, false or no to keep it. | -| `ODYSSEUS_BROWSER_SCREENSHOT_DIR` | *unset* | `src/agent_tools/web_tools.py:3135` | Where private-browser screenshots are written. Falls back to the container path, then the system temp directory. | +| `ODYSSEUS_BROWSER_SCREENSHOT_DIR` | *unset* | `src/agent_tools/web_tools.py:3423` | Where private-browser screenshots are written. Falls back to the container path, then the system temp directory. | ### Container and workspace mounts @@ -256,7 +257,7 @@ reads three ways, because no single pattern covers the codebase: lines, so one read lives inside a string literal. The three passes are not redundancy. A line-based grep for a direct -`os.environ.get("ODYSSEUS_...` call finds 80 of the 108 variables on this +`os.environ.get("ODYSSEUS_...` call finds 81 of the 109 variables on this page. What it misses is reads through an env-reader helper, reads whose call spans more than one line, reads whose variable name is held in a module constant, and reads through a mapping passed in as an argument - which is the From 255bff1f7600e48c6e0e0153f585d0eca9ea5a13 Mon Sep 17 00:00:00 2001 From: Alexandre Teixeira <111787685+alteixeira20@users.noreply.github.com> Date: Thu, 1 Oct 2026 21:03:00 +0100 Subject: [PATCH 2/2] fix(browser): derive batch navigation outcome from command rows A batch whose open succeeded but whose later command failed was recorded as a failed navigation, so a following observation was wrongly labelled stale. Use the per-command rows; when the outcome cannot be determined, treat the page as unknown instead of claiming either result. --- .../wave-5a-browser-lifecycle.md | 9 +++- src/agent_tools/web_tools.py | 41 +++++++++++++++++-- src/browser_lifecycle.py | 18 ++++++++ tests/test_browser_lifecycle.py | 38 +++++++++++++++++ website/configuration-reference.md | 2 +- 5 files changed, 102 insertions(+), 6 deletions(-) diff --git a/docs/runtime-decomposition/wave-5a-browser-lifecycle.md b/docs/runtime-decomposition/wave-5a-browser-lifecycle.md index c33f81bf9..3b02b4457 100644 --- a/docs/runtime-decomposition/wave-5a-browser-lifecycle.md +++ b/docs/runtime-decomposition/wave-5a-browser-lifecycle.md @@ -50,7 +50,7 @@ TurnContract, generic process containment (Wave 3-S), effects/provenance | F4 | Missing `session_id` used agent-browser's shared `default` session | A sessionless call gets an ephemeral session that is closed and verified before the call returns | | F5 | Launch failure left the daemon alive | Launch-failure output triggers forced cleanup and a truthful error | | F6 | Concurrent actions on one session raced one daemon | Per-session `asyncio.Lock` serializes actions | -| F7 | Observation after a failed navigation silently showed the old page | Sessions track navigation generation, page URL and failed navigation; such observations are prefixed with an explicit stale notice and flagged `stale_observation` | +| F7 | Observation after a failed navigation silently showed the old page | Sessions track navigation generation, page URL and failed navigation; such observations are prefixed with an explicit stale notice and flagged `stale_observation`. A batch's navigation outcome comes from its per-command rows; when it cannot be determined the page is treated as unknown | | F8 | Recovery recursed through `execute` with a model-visible retry flag and no overall deadline | One deadline per call (action timeout + 75s); at most one retry, only for local read-only HTML open; model-supplied `_odysseus_browser_retry` is ignored | | F9 | `research_navigator` passed `timeout`, which the tool ignored | Passes `timeout_ms` | | F10 | No lifecycle evidence | Every result carries `browser_lifecycle` with stages, timings, ownership, state and cleanup receipt | @@ -61,7 +61,7 @@ TurnContract, generic process containment (Wave 3-S), effects/provenance ## Lifecycle model -Session states: `idle`, `ready`, `navigation_failed`, `reset`, `timed_out`, +Session states: `idle`, `ready`, `navigation_failed`, `navigation_unknown`, `reset`, `timed_out`, `failed`, `launch_failed`, `bootstrap_failed`, `cancelled`, `closed`. Any state reached by forced cleanup discards the page URL so nothing earlier remains observable. Ownership is `retained` for a chat session (bounded by @@ -113,3 +113,8 @@ process-lifecycle primitives from Wave 3-S/5B. server requires its owner task in `builtin_mcp.py`. - The stale-observation notice marks, but does not block, an observation after a failed navigation. +- Forced cleanup waits synchronously, at most one second, for killed processes + to exit, so it can run from cancellation without awaiting. +- The recovery deadline covers the action and its retry. Post-action + observations (page errors, settled snapshot, screenshot) keep their own + 20 second bounds outside it. diff --git a/src/agent_tools/web_tools.py b/src/agent_tools/web_tools.py index b18757d37..bff68ef66 100644 --- a/src/agent_tools/web_tools.py +++ b/src/agent_tools/web_tools.py @@ -2757,6 +2757,33 @@ class PrivateBrowserTool: header = "\n".join(part for part in (title, url) if part) return {"ok": True, "url": url, "text": f"{header}\n\n{text}".strip()} + @staticmethod + def _batch_navigation_outcome(output: str, command_ok: bool) -> tuple[str, str]: + """Outcome of a batch's last navigation: ``ok``, ``failed`` or ``unknown``. + + A later command failing does not undo a navigation that succeeded, + so the per-command rows decide, not the batch exit status. + """ + + try: + rows = json.loads(output) + except (ValueError, TypeError): + rows = None + if isinstance(rows, list): + for row in reversed(rows): + command = row.get("command") if isinstance(row, dict) else None + if not ( + isinstance(command, list) + and command + and str(command[0]).lower() in {"open", "goto", "navigate"} + ): + continue + if row.get("success") is True: + result = row.get("result") if isinstance(row.get("result"), dict) else {} + return "ok", str(result.get("url") or "") + return "failed", "" + return ("ok", "") if command_ok else ("unknown", "") + @staticmethod def _navigated_url(output: str) -> str: """Final URL reported by ``open`` (after redirects), when present.""" @@ -3028,12 +3055,20 @@ class PrivateBrowserTool: "untrusted_content": True, } if navigation_url: - if command_ok: + outcome, final_url = "ok" if command_ok else "failed", "" + if action == "batch": + outcome, final_url = self._batch_navigation_outcome(out, command_ok) + if outcome == "ok": browser.navigated( - (read_page or {}).get("url") or self._navigated_url(out) or navigation_url + final_url + or (read_page or {}).get("url") + or self._navigated_url(out) + or navigation_url ) - else: + elif outcome == "failed": browser.navigation_failed(navigation_url) + else: + browser.navigation_unknown(navigation_url) if read_page is not None: if not command_ok: return { diff --git a/src/browser_lifecycle.py b/src/browser_lifecycle.py index 689048309..7d266ed63 100644 --- a/src/browser_lifecycle.py +++ b/src/browser_lifecycle.py @@ -300,6 +300,7 @@ class BrowserSession: navigation_generation: int = 0 page_url: str = "" failed_navigation_url: str = "" + navigation_outcome_unknown: bool = False _lock: asyncio.Lock | None = field(default=None, repr=False) _lock_loop: Any = field(default=None, repr=False) @@ -321,22 +322,39 @@ class BrowserSession: self.navigation_generation += 1 self.page_url = url self.failed_navigation_url = "" + self.navigation_outcome_unknown = False self.state = "ready" def navigation_failed(self, url: str) -> None: self.failed_navigation_url = url + self.navigation_outcome_unknown = False self.state = "navigation_failed" + def navigation_unknown(self, url: str) -> None: + """A navigation was attempted but whether it happened is unknown.""" + + self.page_url = "" + self.failed_navigation_url = url + self.navigation_outcome_unknown = True + self.state = "navigation_unknown" + def discarded(self, state: str) -> None: """The browser and its page are gone; nothing earlier is observable.""" self.page_url = "" self.failed_navigation_url = "" + self.navigation_outcome_unknown = False self.state = state def stale_observation_note(self) -> str: if not self.failed_navigation_url: return "" + if self.navigation_outcome_unknown: + return ( + f"Browser lifecycle: the outcome of the most recent navigation to " + f"{self.failed_navigation_url} is unknown. This observation may not " + f"show {self.failed_navigation_url}." + ) shown = self.page_url or "an earlier page" return ( f"Browser lifecycle: the most recent navigation to {self.failed_navigation_url} " diff --git a/tests/test_browser_lifecycle.py b/tests/test_browser_lifecycle.py index 5b8889130..55450de38 100644 --- a/tests/test_browser_lifecycle.py +++ b/tests/test_browser_lifecycle.py @@ -579,3 +579,41 @@ def test_selector_read_is_an_observation_not_a_navigation() -> None: assert PrivateBrowserTool._navigation_target( "batch", {"commands": [["open", "file:///a.html"], ["snapshot"], ["open", "file:///b.html"]]} ) == "file:///b.html" + + +def test_batch_navigation_outcome_comes_from_its_rows(browser_env) -> None: + state, _, _, _ = browser_env + responses = {} + + async def _batch(command): + if command[-2:] == ["batch", "--json"]: + return responses["batch"] + return 0, '- heading "x"' + + state["behaviour"] = _batch + ctx = {"session_id": "s-batch"} + + # The open succeeded; a later click failing must not mark it failed. + responses["batch"] = (1, json.dumps([ + {"command": ["open", "https://a.example/"], "success": True, + "result": {"url": "https://a.example/landing"}}, + {"command": ["click", "@e9"], "success": False, "error": "no element"}, + ])) + result = _run({"action": "batch", "commands": [["open", "https://a.example/"], ["click", "@e9"]]}, ctx) + assert result["browser_lifecycle"]["page_url"] == "https://a.example/landing" + assert result["browser_lifecycle"]["state"] == "ready" + assert "stale_observation" not in _run({"action": "snapshot"}, ctx)["browser_lifecycle"] + + responses["batch"] = (1, json.dumps([ + {"command": ["open", "https://b.example/"], "success": False, "error": "net::ERR"}, + ])) + failed = _run({"action": "batch", "commands": [["open", "https://b.example/"]]}, ctx) + assert failed["browser_lifecycle"]["state"] == "navigation_failed" + note = _run({"action": "snapshot"}, ctx)["output"] + assert "shows https://a.example/landing (navigation #1), not https://b.example/" in note + + responses["batch"] = (1, "daemon connection lost") + _run({"action": "batch", "commands": [["open", "https://c.example/"]]}, ctx) + unknown = _run({"action": "snapshot"}, ctx) + assert "outcome of the most recent navigation to https://c.example/ is unknown" in unknown["output"] + assert unknown["browser_lifecycle"]["page_url"] == "" diff --git a/website/configuration-reference.md b/website/configuration-reference.md index b5d8b7a6b..cf21c8a19 100644 --- a/website/configuration-reference.md +++ b/website/configuration-reference.md @@ -90,7 +90,7 @@ The source tree reads **109** `ODYSSEUS_*` variables: 79 an operator may want to | `ODYSSEUS_BROWSER_MCP_REQUIRE_CACHE` | `''` | `src/builtin_mcp.py:90` | Truthy refuses to start the browser MCP server unless its npm package is already in the npx cache, instead of installing it at startup. | | `ODYSSEUS_BROWSER_NAMESPACE` | `'odysseus-ui'` | `src/agent_tools/web_tools.py:100` (+3 more) | Namespace for the detached agent-browser daemon's pid files, so two runtimes on one machine do not terminate each other's browsers. | | `ODYSSEUS_BROWSER_NO_SANDBOX` | `'1'` | `src/builtin_mcp.py:142` | Security-relevant. On by default, adding `--no-sandbox` because the Docker image cannot use the Chromium sandbox. Set 0, false or no to keep it. | -| `ODYSSEUS_BROWSER_SCREENSHOT_DIR` | *unset* | `src/agent_tools/web_tools.py:3423` | Where private-browser screenshots are written. Falls back to the container path, then the system temp directory. | +| `ODYSSEUS_BROWSER_SCREENSHOT_DIR` | *unset* | `src/agent_tools/web_tools.py:3458` | Where private-browser screenshots are written. Falls back to the container path, then the system temp directory. | ### Container and workspace mounts