diff --git a/Makefile b/Makefile index 5e599a01..dbde6695 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: help install dev-install format lint type-check test test-cov clean pre-commit setup-dev +.PHONY: help install dev-install format lint type-check test test-cov clean pre-commit setup-dev integration help: @echo "Available commands:" @@ -14,8 +14,10 @@ help: @echo " check-all - Run all code quality checks" @echo "" @echo "Testing:" - @echo " test - Run tests with pytest" - @echo " test-cov - Run tests with coverage reporting" + @echo " test - Run tests with pytest" + @echo " test-cov - Run tests with coverage reporting" + @echo " integration - Run integration tests (verbose)" + @echo " PRETTY=1 integration - Run integration tests (clean TUI)" @echo "" @echo "Development:" @echo " pre-commit - Run pre-commit hooks on all files" @@ -70,6 +72,15 @@ test-cov: @echo "βœ… Tests with coverage complete!" @echo "πŸ“Š Coverage report generated in htmlcov/" +integration: +ifdef PRETTY + poetry run pytest tests/integration/ -m integration --no-cov --no-header -q --tb=no -p no:logging -s --pretty +else + @echo "πŸ§ͺ Running integration tests..." + poetry run pytest tests/integration/ -v -s --log-cli-level=INFO -m integration --no-cov + @echo "βœ… Integration tests complete!" +endif + pre-commit: @echo "πŸ”§ Running pre-commit hooks..." uv run pre-commit run --all-files diff --git a/pyproject.toml b/pyproject.toml index 7e8982a1..d1f52d36 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -359,6 +359,7 @@ asyncio_mode = "auto" asyncio_default_fixture_loop_scope = "session" markers = [ "integration: end-to-end tests requiring Docker and network access", + "browsers(n): launch n isolated browser sessions for the test", ] [tool.coverage.run] diff --git a/strix/tools/browser/browser_actions.py b/strix/tools/browser/browser_actions.py index 69618542..4e689e31 100644 --- a/strix/tools/browser/browser_actions.py +++ b/strix/tools/browser/browser_actions.py @@ -156,7 +156,7 @@ async def _run_browser_tool( metadata: dict[str, Any] | None = None, ) -> Any: if not session.browser.is_cdp_connected: - await session.browser.start() + await session.start() llm, _ = _build_llm(metadata=metadata) tools = Tools() diff --git a/strix/tools/browser/browser_manager.py b/strix/tools/browser/browser_manager.py index 5b88d03d..c6a02a4a 100644 --- a/strix/tools/browser/browser_manager.py +++ b/strix/tools/browser/browser_manager.py @@ -60,6 +60,7 @@ class BrowserSession: __slots__ = ( "auth_token", "browser", + "browser_context_id", "cdp_url", "local", "profile_directory", @@ -83,9 +84,45 @@ class BrowserSession: self.auth_token = auth_token self.local = local self.profile_directory = profile_directory + self.browser_context_id: str | None = None self.task_count = 0 + async def start(self) -> None: + from cdp_use.cdp.target.commands import CreateTargetParameters + + 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"] + + async def _scoped( + url: str = "about:blank", background: bool = False, new_window: bool = False + ) -> str: + params = CreateTargetParameters( + url=url, background=background, browserContextId=self.browser_context_id + ) + if new_window: + params["newWindow"] = True + return (await cdp.send.Target.createTarget(params=params))["targetId"] # type: ignore[no-any-return] + + self.browser._cdp_create_new_page = _scoped + target_id = await _scoped(new_window=True) + await self.browser.get_or_create_cdp_session(target_id, focus=True) + async def close(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}, + ) await _close_browser(self.browser) self.browser = None @@ -164,7 +201,9 @@ async def _launch_browser(api_url: str, agent_id: str, auth_token: str = "") -> task.add_done_callback(_manager.background_tasks.discard) return session - return _manager.create(agent_id, browser, api_url, ws_url, auth_token=auth_token) + session = _manager.create(agent_id, browser, api_url, ws_url, auth_token=auth_token) + await session.start() + return session async def _launch_local_browser( diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index ac5630a9..0bd2d96d 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -131,19 +131,32 @@ def agent_state(sandbox_info): ) -@pytest.fixture(scope="session") -def browser_session(agent_state): - set_current_agent_id(_SESSION_AGENT_ID) - ui.status("Launching browser…") - result = _run_in_bg(browser_action(action="launch", agent_state=agent_state)) - if "error" in result: - pytest.fail(f"Browser launch failed: {result}") - ui.log(f"Browser ready: mode={result.get('mode')}") - yield agent_state +@pytest.fixture +def browsers(agent_state, request): from strix.tools.browser.browser_manager import _manager - _manager.sessions.pop(_SESSION_AGENT_ID, None) - _bg_loop.call_soon_threadsafe(_bg_loop.stop) + from .helpers import Browser + + marker = request.node.get_closest_marker("browsers") + count = marker.args[0] if marker else 1 + + agent_ids = [f"{_SESSION_AGENT_ID}-{i}" for i in range(count)] + for aid in agent_ids: + set_current_agent_id(aid) + result = _run_in_bg(browser_action(action="launch", agent_state=agent_state)) + if "error" in result: + pytest.fail(f"Browser launch failed for {aid}: {result}") + + yield [Browser(aid) for aid in agent_ids] + + for aid in agent_ids: + _manager.sessions.pop(aid, None) + set_current_agent_id(_SESSION_AGENT_ID) + + +@pytest.fixture +def browser(browsers): + return browsers[0] def pytest_runtest_logstart(nodeid, location): @@ -164,10 +177,3 @@ def pytest_runtest_logreport(report): @pytest.fixture(autouse=True) def _set_agent_context(): set_current_agent_id(_SESSION_AGENT_ID) - - -@pytest.fixture -def browser(browser_session): - from .helpers import Browser - - return Browser(browser_session) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index f7fbc5c6..8c16b024 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -6,7 +6,6 @@ from pathlib import Path from pytest_check import check from . import console as ui -from .conftest import _run_in_bg SCREENSHOTS_DIR = Path(__file__).parent / "screenshots" @@ -19,20 +18,24 @@ def setup_screenshots_dir(): class Browser: - def __init__(self, agent_state): - self._state = agent_state + def __init__(self, agent_id): + self._agent_id = agent_id def __getattr__(self, action): + import asyncio + from strix.tools.browser.browser_actions import browser_action + from strix.tools.context import set_current_agent_id + + from .conftest import _bg_loop def call(**kwargs): - result = _run_in_bg( - browser_action( - action=action, - agent_state=self._state, - **kwargs, - ) - ) + async def _run(): + set_current_agent_id(self._agent_id) + return await browser_action(agent_state=None, 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"]) return result @@ -40,6 +43,18 @@ class Browser: return call +def act_parallel(tasks): + from concurrent.futures import ThreadPoolExecutor + + def _run_one(browser, kwargs): + action = kwargs.pop("action") + return getattr(browser, action)(**kwargs) + + with ThreadPoolExecutor(max_workers=len(tasks)) as pool: + futures = [pool.submit(_run_one, b, dict(kw)) for b, kw in tasks] + return [f.result(timeout=120) for f in futures] + + def _caller_test_name(): for frame in inspect.stack(): if frame.function.startswith("test_"): diff --git a/tests/integration/test_browser_actions.py b/tests/integration/test_browser_actions.py index 0aa27dd7..9bc733a3 100644 --- a/tests/integration/test_browser_actions.py +++ b/tests/integration/test_browser_actions.py @@ -2,7 +2,6 @@ import base64 import binascii import pytest -from pytest_check import check from . import console as ui from .helpers import Fail, setup_screenshots_dir @@ -18,9 +17,10 @@ def test_navigate(browser): result = browser.navigate(url="https://example.com") ui.log(f"navigate β†’ url={result.get('url')} title={result.get('title')}") - with check: - check.is_in("example.com", result.get("url", "").lower()) - check.is_in("example", result.get("title", "").lower()) + if "example.com" not in result.get("url", "").lower(): + Fail(result).expected("url containing 'example.com'").got(result.get("url")) + if "example" not in result.get("title", "").lower(): + Fail(result).expected("title containing 'example'").got(result.get("title")) def test_click(browser): diff --git a/tests/integration/test_browser_isolation.py b/tests/integration/test_browser_isolation.py new file mode 100644 index 00000000..ed00674c --- /dev/null +++ b/tests/integration/test_browser_isolation.py @@ -0,0 +1,96 @@ +import pytest + +from . import console as ui +from .helpers import Fail, act_parallel + + +pytestmark = [pytest.mark.integration, pytest.mark.browsers(2)] + + +def test_session_objects_are_distinct(browsers): + from strix.tools.browser.browser_manager import _manager + + a, b = browsers + session_a = _manager.sessions.get(a._agent_id) + session_b = _manager.sessions.get(b._agent_id) + + if not session_a or not session_b: + Fail().error("one or both sessions missing from manager") + return + if session_a is session_b: + Fail().error("both agents share the same session object") + if session_a.browser_context_id == session_b.browser_context_id: + Fail().expected("different browser_context_ids").got( + f"A={session_a.browser_context_id}, B={session_b.browser_context_id}" + ) + + +def test_parallel_navigate_isolated(browsers): + a, b = browsers + ui.status("isolation β†’ navigating both agents concurrently") + result_a, result_b = act_parallel( + [ + (a, {"action": "navigate", "url": "https://example.com"}), + (b, {"action": "navigate", "url": "https://www.iana.org"}), + ] + ) + ui.log(f"agent A β†’ {result_a.get('url')}") + ui.log(f"agent B β†’ {result_b.get('url')}") + + if "example.com" not in result_a.get("url", ""): + Fail(result_a).expected("url containing 'example.com'").got(result_a.get("url")) + if "iana.org" not in result_b.get("url", ""): + Fail(result_b).expected("url containing 'iana.org'").got(result_b.get("url")) + + state_a, state_b = act_parallel( + [ + (a, {"action": "screenshot"}), + (b, {"action": "screenshot"}), + ] + ) + + if "example.com" not in state_a.get("url", ""): + Fail(state_a).expected("agent A still on example.com").got(state_a.get("url")) + if "iana.org" not in state_b.get("url", ""): + Fail(state_b).expected("agent B still on iana.org").got(state_b.get("url")) + + +def test_cookie_isolation(browsers): + a, b = browsers + a.navigate(url="https://example.com") + a.evaluate(code="document.cookie = 'agent=A; path=/'") + + b.navigate(url="https://example.com") + result_b = b.evaluate(code="document.cookie") + cookie_b = str(result_b.get("result", "")) + ui.log(f"agent B cookies: {cookie_b}") + + if "agent=A" in cookie_b: + Fail(result_b).expected("no cookie leakage").got(cookie_b) + + result_a = a.evaluate(code="document.cookie") + cookie_a = str(result_a.get("result", "")) + ui.log(f"agent A cookies: {cookie_a}") + + if "agent=A" not in cookie_a: + Fail(result_a).expected("cookie 'agent=A' present").got(cookie_a) + + +def test_concurrent_mixed_actions(browsers): + a, b = browsers + a.navigate(url="https://example.com") + b.navigate(url="https://www.iana.org") + + ui.status("isolation β†’ mixed actions concurrently") + results = act_parallel( + [ + (a, {"action": "scroll", "direction": "down", "amount": 3}), + (b, {"action": "screenshot"}), + (a, {"action": "screenshot"}), + (b, {"action": "evaluate", "code": "document.title"}), + ] + ) + + for r in results: + if "error" in r: + Fail(r).error(r["error"])