diff --git a/strix/tools/browser/browser_manager.py b/strix/tools/browser/browser_manager.py index c6a02a4a..4cd86345 100644 --- a/strix/tools/browser/browser_manager.py +++ b/strix/tools/browser/browser_manager.py @@ -92,12 +92,6 @@ class BrowserSession: await self.browser.start() - # [info] Really annoying discovery: despite being isolated in - # different internal sessions, browser use does not - # *actually* isolate the cookies and internals. - # - # [fix] We use CDP browser contexts manually to make the - # sessions unique per CDP connection. Nothing groundbreaking cdp = self.browser.cdp_client ctx = await cdp.send.Target.createBrowserContext(params={"disposeOnDetach": True}) self.browser_context_id = ctx["browserContextId"] @@ -116,13 +110,16 @@ class BrowserSession: target_id = await _scoped(new_window=True) await self.browser.get_or_create_cdp_session(target_id, focus=True) - async def close(self) -> None: + async def dispose_context(self) -> None: if self.browser_context_id and self.browser and self.browser.is_cdp_connected: with contextlib.suppress(Exception): - # [fix] properly dispose the session on closure. await self.browser.cdp_client.send.Target.disposeBrowserContext( params={"browserContextId": self.browser_context_id}, ) + self.browser_context_id = None + + async def close(self) -> None: + await self.dispose_context() await _close_browser(self.browser) self.browser = None diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 0bd2d96d..77771e19 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -39,7 +39,6 @@ def _preflight(): _preflight() -# in pretty mode, silence all loggers and suppress pytest's own terminal output if ui.is_pretty(): for _name in ( "strix.tests.integration", @@ -147,10 +146,12 @@ def browsers(agent_state, request): if "error" in result: pytest.fail(f"Browser launch failed for {aid}: {result}") - yield [Browser(aid) for aid in agent_ids] + yield [Browser(aid, agent_state) for aid in agent_ids] for aid in agent_ids: - _manager.sessions.pop(aid, None) + session = _manager.remove(aid) + if session: + _run_in_bg(session.dispose_context()) set_current_agent_id(_SESSION_AGENT_ID) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 8c16b024..559426c0 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -18,8 +18,9 @@ def setup_screenshots_dir(): class Browser: - def __init__(self, agent_id): + def __init__(self, agent_id, agent_state=None): self._agent_id = agent_id + self._agent_state = agent_state def __getattr__(self, action): import asyncio @@ -32,12 +33,17 @@ class Browser: def call(**kwargs): async def _run(): set_current_agent_id(self._agent_id) - return await browser_action(agent_state=None, action=action, **kwargs) + return await browser_action( + agent_state=self._agent_state, + action=action, + **kwargs, + ) future = asyncio.run_coroutine_threadsafe(_run(), _bg_loop) result = future.result(timeout=120) if "error" in result: Fail(result).error(result["error"]) + _strip_screenshot(result, _caller_test_name()) return result return call @@ -62,20 +68,20 @@ def _caller_test_name(): return "unknown" -def _save_screenshot(result, name): - b64 = result.get("screenshot") +def _strip_screenshot(result, name): + b64 = result.pop("screenshot", None) if not b64 or not isinstance(b64, str) or len(b64) < 100: - return None + return path = SCREENSHOTS_DIR / f"{name}.png" path.write_bytes(base64.b64decode(b64)) - return str(path) + result["screenshot_path"] = str(path) class Fail: def __init__(self, result=None): self._result = result self._name = _caller_test_name() - self._screenshot = _save_screenshot(result, self._name) if result else None + self._screenshot = result.get("screenshot_path") if result else None def expected(self, value): self._expected = value diff --git a/tests/integration/test_browser_actions.py b/tests/integration/test_browser_actions.py index 9bc733a3..cc0fb604 100644 --- a/tests/integration/test_browser_actions.py +++ b/tests/integration/test_browser_actions.py @@ -1,5 +1,4 @@ -import base64 -import binascii +from pathlib import Path import pytest @@ -89,15 +88,13 @@ def test_screenshot(browser): browser.navigate(url="https://example.com") result = browser.screenshot() - screenshot = result.get("screenshot", "") - ui.log(f"screenshot → {len(screenshot)} bytes base64") + path = result.get("screenshot_path") + ui.log(f"screenshot → {path}") - if len(screenshot) <= 100: - Fail(result).expected("> 100 bytes").got(f"{len(screenshot)} bytes") - try: - base64.b64decode(screenshot) - except (ValueError, binascii.Error) as e: - Fail(result).error(f"invalid base64: {e}") + if not path: + Fail(result).error("no screenshot saved") + elif not Path(path).exists(): + Fail(result).error(f"screenshot file missing: {path}") def test_input(browser): diff --git a/tests/integration/test_browser_isolation.py b/tests/integration/test_browser_isolation.py index ed00674c..6d32e6bc 100644 --- a/tests/integration/test_browser_isolation.py +++ b/tests/integration/test_browser_isolation.py @@ -1,13 +1,13 @@ import pytest from . import console as ui -from .helpers import Fail, act_parallel +from .helpers import Browser, Fail, act_parallel pytestmark = [pytest.mark.integration, pytest.mark.browsers(2)] -def test_session_objects_are_distinct(browsers): +def test_session_objects_are_distinct(browsers: list[Browser]) -> None: from strix.tools.browser.browser_manager import _manager a, b = browsers @@ -25,7 +25,7 @@ def test_session_objects_are_distinct(browsers): ) -def test_parallel_navigate_isolated(browsers): +def test_parallel_navigate_isolated(browsers: list[Browser]) -> None: a, b = browsers ui.status("isolation → navigating both agents concurrently") result_a, result_b = act_parallel( @@ -55,7 +55,7 @@ def test_parallel_navigate_isolated(browsers): Fail(state_b).expected("agent B still on iana.org").got(state_b.get("url")) -def test_cookie_isolation(browsers): +def test_cookie_isolation(browsers: list[Browser]) -> None: a, b = browsers a.navigate(url="https://example.com") a.evaluate(code="document.cookie = 'agent=A; path=/'") @@ -76,7 +76,7 @@ def test_cookie_isolation(browsers): Fail(result_a).expected("cookie 'agent=A' present").got(cookie_a) -def test_concurrent_mixed_actions(browsers): +def test_concurrent_mixed_actions(browsers: list[Browser]) -> None: a, b = browsers a.navigate(url="https://example.com") b.navigate(url="https://www.iana.org") @@ -94,3 +94,56 @@ def test_concurrent_mixed_actions(browsers): for r in results: if "error" in r: Fail(r).error(r["error"]) + + +def test_session_close_does_not_affect_other(browsers: list[Browser]) -> None: + a, b = browsers + a.navigate(url="https://example.com") + b.navigate(url="https://www.iana.org") + + a.close_browser() + ui.log("agent A closed") + + result = b.navigate(url="https://example.com") + ui.log(f"agent B navigation result: {result}") + if "error" in result: + Fail(result).error(f"agent B broken after A closed: {result['error']}") + if "example.com" not in result.get("url", ""): + Fail(result).expected("url containing 'example.com'").got(result.get("url")) + + +def test_relaunch_parallel_no_side_effects(browsers: list[Browser]) -> None: + a, b = browsers + a.navigate(url="https://example.com") + b.navigate(url="https://www.iana.org") + + # close A, verify B is unaffected + a.close_browser() + b_check = b.screenshot() + if "iana.org" not in b_check.get("url", ""): + Fail(b_check).expected("agent B still on iana.org").got(b_check.get("url")) + + # relaunch A + a.launch() + a.navigate(url="https://example.com") + + # parallel actions on both after relaunch + results = act_parallel( + [ + (a, {"action": "evaluate", "code": "document.title"}), + (b, {"action": "evaluate", "code": "document.title"}), + (a, {"action": "screenshot"}), + (b, {"action": "screenshot"}), + ] + ) + + for r in results: + if "error" in r: + Fail(r).error(r["error"]) + + # verify no cross-contamination after relaunch (use url, titles vary) + a_state, b_state = results[2], results[3] + if "example.com" not in a_state.get("url", ""): + Fail(a_state).expected("agent A on example.com").got(a_state.get("url")) + if "iana.org" not in b_state.get("url", ""): + Fail(b_state).expected("agent B on iana.org").got(b_state.get("url"))