diff --git a/browser/agent-harness/cli_anything/browser/README.md b/browser/agent-harness/cli_anything/browser/README.md index 74381add23..b722768170 100644 --- a/browser/agent-harness/cli_anything/browser/README.md +++ b/browser/agent-harness/cli_anything/browser/README.md @@ -241,10 +241,25 @@ Run: `npx @apireno/domshell --version` (first run downloads the package) - Ensure Chrome is running - Install DOMShell extension from Chrome Web Store - Check that DOMShell is enabled in Chrome +- Verify `DOMSHELL_TOKEN` matches the token used by the running DOMShell server ### Commands hang on first use First `npx` call downloads DOMShell package (10-50 MB). Subsequent calls are faster. Use `--daemon` mode for persistent connection. +If the server is unreachable or auth token is wrong, MCP calls now fail with timeout instead of hanging forever. +Set the per-tool-call timeout (seconds) via: + +```bash +export CLI_ANYTHING_BROWSER_MCP_TIMEOUT=20 +``` + +Session initialization (spawning `npx`/`domshell-proxy` and completing the MCP handshake) +can legitimately take longer than a regular tool call, so it has its own timeout: + +```bash +export CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT=120 +``` + ## Architecture This CLI follows the [CLI-Anything harness methodology](https://github.com/HKUDS/CLI-Anything/tree/main/cli-anything-plugin/HARNESS.md): diff --git a/browser/agent-harness/cli_anything/browser/__init__.py b/browser/agent-harness/cli_anything/browser/__init__.py index ebd15be9da..d4b40d44a5 100644 --- a/browser/agent-harness/cli_anything/browser/__init__.py +++ b/browser/agent-harness/cli_anything/browser/__init__.py @@ -4,4 +4,4 @@ Accessibility Tree exposed through DOMShell's MCP server. """ -__version__ = "1.0.0" +__version__ = "1.0.1" diff --git a/browser/agent-harness/cli_anything/browser/browser_cli.py b/browser/agent-harness/cli_anything/browser/browser_cli.py index 264a1f9c9e..c99917a14c 100644 --- a/browser/agent-harness/cli_anything/browser/browser_cli.py +++ b/browser/agent-harness/cli_anything/browser/browser_cli.py @@ -381,7 +381,7 @@ def repl(): global _repl_mode _repl_mode = True - skin = ReplSkin("browser", version="1.0.0") + skin = ReplSkin("browser", version="1.0.1") skin.print_banner() pt_session = skin.create_prompt_session() diff --git a/browser/agent-harness/cli_anything/browser/skills/SKILL.md b/browser/agent-harness/cli_anything/browser/skills/SKILL.md index 73b9e4d4a2..0185fed9c9 100644 --- a/browser/agent-harness/cli_anything/browser/skills/SKILL.md +++ b/browser/agent-harness/cli_anything/browser/skills/SKILL.md @@ -158,6 +158,7 @@ The CLI provides clear error messages for common issues: - **npx not found**: Install Node.js from https://nodejs.org/ - **DOMShell not found**: Run `npx @apireno/domshell --version` - **MCP call failed**: Install DOMShell Chrome extension +- **MCP timeout / hanging calls**: Verify `DOMSHELL_TOKEN`, ensure DOMShell server is running, and optionally tune timeouts via `CLI_ANYTHING_BROWSER_MCP_TIMEOUT` (default: 20s, per tool call) and `CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT` (default: 120s, session initialize) Check `is_available()` return value before running commands. diff --git a/browser/agent-harness/cli_anything/browser/tests/test_core.py b/browser/agent-harness/cli_anything/browser/tests/test_core.py index 18c722582a..80c29f4988 100644 --- a/browser/agent-harness/cli_anything/browser/tests/test_core.py +++ b/browser/agent-harness/cli_anything/browser/tests/test_core.py @@ -7,7 +7,7 @@ """ import pytest -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, patch from cli_anything.browser.core.session import Session from cli_anything.browser.core import page, fs diff --git a/browser/agent-harness/cli_anything/browser/tests/test_domshell_backend.py b/browser/agent-harness/cli_anything/browser/tests/test_domshell_backend.py index 2b75fe0097..9d7612370f 100644 --- a/browser/agent-harness/cli_anything/browser/tests/test_domshell_backend.py +++ b/browser/agent-harness/cli_anything/browser/tests/test_domshell_backend.py @@ -127,6 +127,23 @@ def test_cd_relative_quoted(mock_call): assert mock_call.call_args.args[0] == "cd main" +def test_cd_restores_tracked_cwd_after_timeout(): + sess = _make_session(working_dir="/main") + mock_call = AsyncMock(side_effect=[ + backend.MCPToolTimeoutError("cd timed out"), + _make_result("restored\n[lane: 1]"), + ]) + + with patch.object(backend, "_call_execute", mock_call): + with pytest.raises(backend.MCPToolTimeoutError, match="cd timed out"): + backend.cd("/dialog", session=sess) + + assert [item.args[0] for item in mock_call.call_args_list] == [ + "cd %here%/dialog", + "cd %here%/main", + ] + + @patch.object(backend, "_call_execute", new_callable=AsyncMock) def test_cat_absolute_path_uses_three_separate_calls(mock_call): """`cat /main/btn`: anchor at tab root → cat main/btn → restore.""" @@ -1335,3 +1352,883 @@ def test_use_daemon_positional_on_type_text(mock_call): call("focus input", True, session=sess), call("type hello", True, session=sess), ] + + +# ── MCP init vs. tool-call timeout split ───────────────────────────── +# +# CLI_ANYTHING_BROWSER_MCP_TIMEOUT bounds individual tool calls (default 20s); +# CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT bounds session.initialize() / +# daemon initialize() (default 120s), since a handshake can legitimately +# take much longer than a single tool call without indicating a hang. + + +def test_tool_timeout_default_value(monkeypatch): + """Tool-call timeout defaults to 20s when env var is unset.""" + monkeypatch.delenv("CLI_ANYTHING_BROWSER_MCP_TIMEOUT", raising=False) + assert backend._get_tool_timeout_seconds() == 20.0 + + +def test_tool_timeout_invalid_env_falls_back_to_default(monkeypatch): + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_TIMEOUT", "not-a-number") + assert backend._get_tool_timeout_seconds() == 20.0 + + +def test_tool_timeout_is_clamped_to_minimum(monkeypatch): + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_TIMEOUT", "0") + assert backend._get_tool_timeout_seconds() == 1.0 + + +def test_init_timeout_default_value(monkeypatch): + """Init timeout defaults to 120s when env var is unset.""" + monkeypatch.delenv("CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT", raising=False) + assert backend._get_init_timeout_seconds() == 120.0 + + +def test_init_timeout_invalid_env_falls_back_to_default(monkeypatch): + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT", "not-a-number") + assert backend._get_init_timeout_seconds() == 120.0 + + +def test_init_timeout_is_clamped_to_minimum(monkeypatch): + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT", "0") + assert backend._get_init_timeout_seconds() == 1.0 + + +def test_init_timeout_independent_of_tool_timeout(monkeypatch): + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_TIMEOUT", "5") + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT", "120") + assert backend._get_tool_timeout_seconds() == 5.0 + assert backend._get_init_timeout_seconds() == 120.0 + + +def test_await_with_timeout_passes_fast_calls(monkeypatch): + """Fast operations should complete without timeout errors.""" + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_TIMEOUT", "5") + + async def _fast(): + return {"ok": True} + + import asyncio as _aio + result = _aio.run(backend._await_with_timeout(_fast(), "unit-test")) + assert result == {"ok": True} + + +def test_await_with_timeout_runs_operation_in_calling_task(): + async def _run(): + import asyncio as _aio + + calling_task = _aio.current_task() + + async def _operation(): + return _aio.current_task() + + operation_task = await backend._await_with_timeout( + _operation(), "unit-test", 1 + ) + assert operation_task is calling_task + + import asyncio as _aio + _aio.run(_run()) + + +def test_await_with_timeout_raises_on_timeout(monkeypatch): + """Slow operations should raise an actionable MCPToolTimeoutError.""" + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_TIMEOUT", "1") + + async def _slow(): + import asyncio as _aio + await _aio.sleep(2) + return {"ok": True} + + import asyncio as _aio + + with pytest.raises(RuntimeError, match="timed out") as exc_info: + _aio.run(backend._await_with_timeout(_slow(), "unit-test")) + message = str(exc_info.value) + assert "CLI_ANYTHING_BROWSER_MCP_TIMEOUT" in message + assert "CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT" in message + + +def test_call_execute_uses_init_timeout_for_session_initialize(monkeypatch): + """Non-daemon _call_execute must await session.initialize() with the + init timeout, not the (much shorter) tool-call timeout.""" + monkeypatch.setenv("DOMSHELL_TOKEN", "test-token") + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_TIMEOUT", "5") + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT", "120") + + recorded_timeouts = [] + + async def _fake_await_with_timeout(coro, operation, timeout_seconds=None): + if hasattr(coro, "close"): + coro.close() + recorded_timeouts.append((operation, timeout_seconds)) + return _make_result("✓\n[lane: 1]") + + class _DummyMcpSession: + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, tb): + return False + + def initialize(self): + return object() + + def call_tool(self, _tool_name, _arguments): + return object() + + class _DummyStdio: + async def __aenter__(self): + return object(), object() + + async def __aexit__(self, exc_type, exc, tb): + return False + + with patch.object(backend, "stdio_client", return_value=_DummyStdio()), \ + patch.object(backend, "ClientSession", return_value=_DummyMcpSession()), \ + patch.object(backend, "_build_server_args", return_value=[]), \ + patch.object(backend, "_await_with_timeout", side_effect=_fake_await_with_timeout): + import asyncio as _aio + _aio.run(backend._call_execute("ls /", session=Session())) + + assert ("session initialize", 120.0) in recorded_timeouts + assert ("domshell_execute", 5.0) in recorded_timeouts + + +def test_start_daemon_uses_init_timeout(monkeypatch): + """_start_daemon must await daemon initialize() with the init timeout.""" + monkeypatch.setenv("DOMSHELL_TOKEN", "test-token") + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_TIMEOUT", "5") + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT", "120") + + recorded_timeouts = [] + + async def _fake_await_with_timeout(coro, operation, timeout_seconds=None): + if hasattr(coro, "close"): + coro.close() + recorded_timeouts.append((operation, timeout_seconds)) + return None + + class _DummyMcpSession: + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, tb): + return False + + def initialize(self): + return object() + + class _DummyStdio: + async def __aenter__(self): + return object(), object() + + async def __aexit__(self, exc_type, exc, tb): + return False + + original_daemon = backend._daemon_session + try: + backend._daemon_session = None + with patch.object(backend, "stdio_client", return_value=_DummyStdio()), \ + patch.object(backend, "ClientSession", return_value=_DummyMcpSession()), \ + patch.object(backend, "_build_server_args", return_value=[]), \ + patch.object(backend, "_await_with_timeout", side_effect=_fake_await_with_timeout): + import asyncio as _aio + _aio.run(backend._start_daemon()) + + assert recorded_timeouts == [("daemon initialize", 120.0)] + finally: + backend._daemon_session = original_daemon + + +def test_call_execute_captures_timeout_once_per_operation(monkeypatch): + """Timeouts are captured once per _call_execute invocation and passed + explicitly, rather than re-reading env vars on every await within the + same logical operation (which could pick up a mid-operation mutation).""" + monkeypatch.setenv("DOMSHELL_TOKEN", "test-token") + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_TIMEOUT", "5") + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT", "120") + + recorded_timeouts = [] + + async def _fake_await_with_timeout(coro, operation, timeout_seconds=None): + if hasattr(coro, "close"): + coro.close() + # Simulate the env var mutating mid-operation (e.g. another + # thread/test). A per-call env read would pick this up; a + # captured-once value would not. + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_TIMEOUT", "999") + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT", "999") + recorded_timeouts.append((operation, timeout_seconds)) + return _make_result("✓\n[lane: 1]") + + class _DummyMcpSession: + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, tb): + return False + + def initialize(self): + return object() + + def call_tool(self, _tool_name, _arguments): + return object() + + class _DummyStdio: + async def __aenter__(self): + return object(), object() + + async def __aexit__(self, exc_type, exc, tb): + return False + + with patch.object(backend, "stdio_client", return_value=_DummyStdio()), \ + patch.object(backend, "ClientSession", return_value=_DummyMcpSession()), \ + patch.object(backend, "_build_server_args", return_value=[]), \ + patch.object(backend, "_await_with_timeout", side_effect=_fake_await_with_timeout): + import asyncio as _aio + _aio.run(backend._call_execute("ls /", session=Session())) + + # Both awaits within this single _call_execute invocation must use the + # timeouts captured before the env var was mutated mid-operation. + assert recorded_timeouts == [ + ("session initialize", 120.0), + ("domshell_execute", 5.0), + ] + + +def test_call_execute_bounds_context_exit_after_tool_timeout(monkeypatch): + """A wedged context exit must not hide the original tool timeout.""" + monkeypatch.setenv("DOMSHELL_TOKEN", "test-token") + monkeypatch.setattr(backend, "DAEMON_STOP_TIMEOUT_SECONDS", 0.01) + exit_started = [] + + class _DummyMcpSession: + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, tb): + exit_started.append("session") + import asyncio as _aio + await _aio.sleep(0.5) + + def initialize(self): + return object() + + def call_tool(self, _tool_name, _arguments): + return object() + + class _DummyStdio: + async def __aenter__(self): + return object(), object() + + async def __aexit__(self, exc_type, exc, tb): + exit_started.append("stdio") + import asyncio as _aio + await _aio.sleep(0.5) + + async def _fake_await_with_timeout(coro, operation, timeout_seconds=None): + if hasattr(coro, "close"): + coro.close() + if operation == "domshell_execute": + raise backend.MCPToolTimeoutError("tool timed out") + return None + + async def _run(): + import asyncio as _aio + with pytest.raises(backend.MCPToolTimeoutError, match="tool timed out"): + await _aio.wait_for( + backend._call_execute("ls /", session=Session()), + timeout=0.2, + ) + + with patch.object(backend, "stdio_client", return_value=_DummyStdio()), \ + patch.object(backend, "ClientSession", return_value=_DummyMcpSession()), \ + patch.object(backend, "_build_server_args", return_value=[]), \ + patch.object(backend, "_await_with_timeout", side_effect=_fake_await_with_timeout): + import asyncio as _aio + _aio.run(_run()) + + assert exit_started == ["session", "stdio"] + + +def test_close_transport_exits_contexts_in_entering_task(): + """AnyIO-backed MCP contexts must exit in the task that entered them.""" + import asyncio as _aio + + class _TaskBoundContext: + entered_task = None + exited_task = None + + async def __aenter__(self): + self.entered_task = _aio.current_task() + return self + + async def __aexit__(self, exc_type, exc, tb): + self.exited_task = _aio.current_task() + return False + + async def _run(): + real_wait_for = _aio.wait_for + + async def _scheduled_wait_for(awaitable, timeout): + return await real_wait_for(_aio.create_task(awaitable), timeout) + + session = _TaskBoundContext() + client = _TaskBoundContext() + await client.__aenter__() + await session.__aenter__() + with patch.object( + backend.asyncio, "wait_for", new=_scheduled_wait_for + ): + await backend._close_daemon_transport(session, client) + assert session.exited_task is session.entered_task + assert client.exited_task is client.entered_task + + _aio.run(_run()) + + +def test_close_transport_preserves_anyio_cancel_scope_order(): + """Cleanup timeout must not nest a cancel scope inside the MCP context.""" + import asyncio as _aio + import anyio + + class _AnyioContext: + closed = False + + async def __aenter__(self): + self.task_group = anyio.create_task_group() + await self.task_group.__aenter__() + return self + + async def __aexit__(self, exc_type, exc, tb): + await self.task_group.__aexit__(exc_type, exc, tb) + self.closed = True + + async def _run(): + context = _AnyioContext() + await context.__aenter__() + await backend._close_daemon_transport(context, None) + assert context.closed is True + + _aio.run(_run()) + + +@pytest.mark.parametrize( + "operation", + ["ls", "cat", "grep", "click", "focus-error", "type"], +) +def test_restore_failure_does_not_mask_primary_result(operation): + sess = _make_session(working_dir="/main") + success = _make_result("primary result\n[lane: 1]") + restore_timeout = backend.MCPToolTimeoutError("restore timed out") + + if operation == "focus-error": + focus_error = _make_result("Error: focus failed\n[lane: 1]") + responses = [_make_result("anchored\n[lane: 1]"), focus_error, restore_timeout] + elif operation == "type": + responses = [ + _make_result("anchored\n[lane: 1]"), + _make_result("focused\n[lane: 1]"), + success, + restore_timeout, + ] + else: + responses = [ + _make_result("anchored\n[lane: 1]"), + success, + restore_timeout, + ] + + with patch.object( + backend, "_call_execute", AsyncMock(side_effect=responses) + ): + if operation == "ls": + result = backend.ls("/main", session=sess) + elif operation == "cat": + result = backend.cat("/main/button", session=sess) + elif operation == "grep": + result = backend.grep("needle", path="/main", session=sess) + elif operation == "click": + result = backend.click("/main/button", session=sess) + else: + result = backend.type_text("/main/input", "hello", session=sess) + + if operation == "focus-error": + assert "focus failed" in result["error"] + elif operation in {"ls", "grep"}: + assert "primary result" in result["raw"] + else: + assert "primary result" in result["output"] + + +@pytest.mark.parametrize("blocked_stage", ["stdio", "session"]) +def test_call_execute_bounds_context_entry(monkeypatch, blocked_stage): + monkeypatch.setenv("DOMSHELL_TOKEN", "test-token") + monkeypatch.setattr(backend, "_get_init_timeout_seconds", lambda: 0.01) + + class _Stdio: + async def __aenter__(self): + if blocked_stage == "stdio": + import asyncio as _aio + await _aio.sleep(0.5) + return object(), object() + + async def __aexit__(self, exc_type, exc, tb): + return False + + class _Session: + async def __aenter__(self): + if blocked_stage == "session": + import asyncio as _aio + await _aio.sleep(0.5) + return self + + async def __aexit__(self, exc_type, exc, tb): + return False + + async def _run(): + import asyncio as _aio + with pytest.raises( + backend.MCPToolTimeoutError, match=f"{blocked_stage}.*enter" + ): + await _aio.wait_for( + backend._call_execute("ls /", session=Session()), + timeout=0.2, + ) + + with patch.object(backend, "stdio_client", return_value=_Stdio()), \ + patch.object(backend, "ClientSession", return_value=_Session()), \ + patch.object(backend, "_build_server_args", return_value=[]): + import asyncio as _aio + _aio.run(_run()) + + +@pytest.mark.parametrize("blocked_stage", ["stdio", "session"]) +def test_start_daemon_bounds_context_entry(monkeypatch, blocked_stage): + monkeypatch.setattr(backend, "_get_init_timeout_seconds", lambda: 0.01) + + class _Stdio: + async def __aenter__(self): + if blocked_stage == "stdio": + import asyncio as _aio + await _aio.sleep(0.5) + return object(), object() + + async def __aexit__(self, exc_type, exc, tb): + return False + + class _Session: + async def __aenter__(self): + if blocked_stage == "session": + import asyncio as _aio + await _aio.sleep(0.5) + return self + + async def __aexit__(self, exc_type, exc, tb): + return False + + async def _run(): + import asyncio as _aio + with pytest.raises(RuntimeError, match=f"{blocked_stage}.*enter"): + await _aio.wait_for(backend._start_daemon(), timeout=0.2) + + original_state = ( + backend._daemon_session, + backend._daemon_read, + backend._daemon_write, + backend._daemon_client_context, + ) + try: + backend._daemon_session = None + backend._daemon_read = None + backend._daemon_write = None + backend._daemon_client_context = None + with patch.object(backend, "stdio_client", return_value=_Stdio()), \ + patch.object(backend, "ClientSession", return_value=_Session()), \ + patch.object(backend, "_build_server_args", return_value=[]): + import asyncio as _aio + _aio.run(_run()) + finally: + ( + backend._daemon_session, + backend._daemon_read, + backend._daemon_write, + backend._daemon_client_context, + ) = original_state + + +def test_daemon_timeout_is_not_retried_and_resets_daemon(): + """A daemon-mode timeout must not be retried against the same tool + call, but must reset the daemon so later commands can recover.""" + + class _DummyDaemonSession: + def call_tool(self, _tool_name, _arguments): + return object() + + async def _fake_await_with_timeout(coro, operation, timeout_seconds=None): + if hasattr(coro, "close"): + coro.close() + raise backend.MCPToolTimeoutError("timed out") + + original_daemon = backend._daemon_session + try: + backend._daemon_session = _DummyDaemonSession() + with patch.object(backend, "_await_with_timeout", side_effect=_fake_await_with_timeout), \ + patch.object(backend, "_stop_daemon", new_callable=AsyncMock) as mock_stop, \ + patch.object(backend, "stdio_client") as mock_stdio: + import asyncio as _aio + with pytest.raises(RuntimeError, match="timed out"): + _aio.run(backend._call_execute("click /", use_daemon=True, session=Session())) + + mock_stop.assert_awaited_once() + mock_stdio.assert_not_called() + finally: + backend._daemon_session = original_daemon + + +def test_daemon_non_timeout_error_falls_back_to_non_daemon_mode(monkeypatch): + """A non-timeout daemon RuntimeError should stop the daemon and fall + back to a fresh non-daemon call once (not propagate directly).""" + monkeypatch.setenv("DOMSHELL_TOKEN", "test-token") + + class _BrokenDaemonSession: + def call_tool(self, _tool_name, _arguments): + raise RuntimeError("loop mismatch") + + class _DummyMcpSession: + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, tb): + return False + + def initialize(self): + return object() + + def call_tool(self, _tool_name, _arguments): + return object() + + class _DummyStdio: + async def __aenter__(self): + return object(), object() + + async def __aexit__(self, exc_type, exc, tb): + return False + + fallback_result = _make_result("✓\n[lane: 1]") + + async def _fake_await_with_timeout(coro, operation, timeout_seconds=None): + if hasattr(coro, "close"): + coro.close() + return fallback_result + + original_daemon = backend._daemon_session + try: + backend._daemon_session = _BrokenDaemonSession() + with patch.object(backend, "stdio_client", return_value=_DummyStdio()), \ + patch.object(backend, "ClientSession", return_value=_DummyMcpSession()), \ + patch.object(backend, "_build_server_args", return_value=[]), \ + patch.object(backend, "_await_with_timeout", side_effect=_fake_await_with_timeout), \ + patch.object(backend, "_stop_daemon", new_callable=AsyncMock) as mock_stop: + import asyncio as _aio + result = _aio.run( + backend._call_execute("click /", use_daemon=True, session=Session()) + ) + + assert result is fallback_result + mock_stop.assert_awaited_once() + finally: + backend._daemon_session = original_daemon + + + +# ── _start_daemon: entered contexts must be closed on init failure ─── +# +# _start_daemon manually drives stdio_client and ClientSession as +# entered context managers (not via `async with`) so it can hold them +# open for the life of the daemon. If `initialize()` times out or +# raises after both contexts are entered, the previous code only +# nulled the module globals — it never called `__aexit__` on either +# context, orphaning the underlying npx/DOMShell subprocess and stdio +# pipes. This must close both entered contexts (best-effort, bounded) +# before re-raising, without masking the original failure. + + +def test_start_daemon_closes_contexts_on_initialize_failure(monkeypatch): + """A daemon-initialize failure must exit both the entered + ClientSession and the entered stdio_client context — not just null + the globals — so the underlying npx/DOMShell process isn't + orphaned.""" + monkeypatch.setenv("DOMSHELL_TOKEN", "test-token") + monkeypatch.setenv("CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT", "120") + + session_exit_calls = [] + stdio_exit_calls = [] + + class _DummyMcpSession: + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, tb): + session_exit_calls.append((exc_type, exc)) + return False + + def initialize(self): + return object() + + class _DummyStdio: + async def __aenter__(self): + return object(), object() + + async def __aexit__(self, exc_type, exc, tb): + stdio_exit_calls.append((exc_type, exc)) + return False + + async def _fake_await_with_timeout(coro, operation, timeout_seconds=None): + if hasattr(coro, "close"): + coro.close() + raise backend.MCPToolTimeoutError("daemon initialize timed out") + + original_daemon = backend._daemon_session + original_context = backend._daemon_client_context + try: + backend._daemon_session = None + backend._daemon_client_context = None + with patch.object(backend, "stdio_client", return_value=_DummyStdio()), \ + patch.object(backend, "ClientSession", return_value=_DummyMcpSession()), \ + patch.object(backend, "_build_server_args", return_value=[]), \ + patch.object(backend, "_await_with_timeout", side_effect=_fake_await_with_timeout): + import asyncio as _aio + with pytest.raises(RuntimeError, match="Failed to start DOMShell daemon"): + _aio.run(backend._start_daemon()) + + # Both entered contexts must have been exited exactly once. + assert len(session_exit_calls) == 1 + assert len(stdio_exit_calls) == 1 + # Globals still end up cleared (pre-existing behavior, preserved). + assert backend._daemon_session is None + assert backend._daemon_read is None + assert backend._daemon_write is None + assert backend._daemon_client_context is None + finally: + backend._daemon_session = original_daemon + backend._daemon_client_context = original_context + + +def test_start_daemon_closes_stdio_context_when_session_never_entered(monkeypatch): + """If ClientSession.__aenter__ itself raises (session never fully + entered), the already-entered stdio_client context must still be + exited.""" + monkeypatch.setenv("DOMSHELL_TOKEN", "test-token") + + stdio_exit_calls = [] + + class _BrokenMcpSession: + async def __aenter__(self): + raise RuntimeError("session enter failed") + + async def __aexit__(self, exc_type, exc, tb): + return False + + class _DummyStdio: + async def __aenter__(self): + return object(), object() + + async def __aexit__(self, exc_type, exc, tb): + stdio_exit_calls.append((exc_type, exc)) + return False + + original_daemon = backend._daemon_session + original_context = backend._daemon_client_context + try: + backend._daemon_session = None + backend._daemon_client_context = None + with patch.object(backend, "stdio_client", return_value=_DummyStdio()), \ + patch.object(backend, "ClientSession", return_value=_BrokenMcpSession()), \ + patch.object(backend, "_build_server_args", return_value=[]): + import asyncio as _aio + with pytest.raises(RuntimeError, match="Failed to start DOMShell daemon"): + _aio.run(backend._start_daemon()) + + assert len(stdio_exit_calls) == 1 + assert backend._daemon_client_context is None + finally: + backend._daemon_session = original_daemon + backend._daemon_client_context = original_context + + +# ── Absolute-path wrappers: restore must be attempted even when the +# operation raises (not just on normal return) ───────────────────── +# +# ls/cat/click/grep/type_text all anchor a `cd` before the real +# operation and restore afterward. Previously the restore call only +# ran on normal return — an operation that raised (e.g. an +# MCPToolTimeoutError) skipped it entirely, leaving the DOMShell +# lane's cwd stuck at the anchor and silently corrupting every later +# relative-path call against that lane. The fix must still let the +# original exception propagate (the restore is best-effort — its own +# failure must never mask the operation's failure). + + +@patch.object(backend, "_call_execute", new_callable=AsyncMock) +def test_ls_absolute_restores_after_op_raises(mock_call): + sess = _make_session(working_dir="/main") + mock_call.side_effect = [ + _make_result("✓ Entered\n[lane: 1]"), # anchor ok + backend.MCPToolTimeoutError("ls timed out"), # op raises + _make_result("✓\n[lane: 1]"), # restore + ] + with pytest.raises(backend.MCPToolTimeoutError, match="ls timed out"): + backend.ls("/main", session=sess) + assert mock_call.call_count == 3 + assert mock_call.call_args_list[0].args[0] == "cd %here%/main" + assert mock_call.call_args_list[2].args[0] == "cd %here%/main" + + +@patch.object(backend, "_call_execute", new_callable=AsyncMock) +def test_cat_absolute_restores_after_op_raises(mock_call): + sess = _make_session(working_dir="/") + mock_call.side_effect = [ + _make_result("✓ Entered\n[lane: 1]"), + backend.MCPToolTimeoutError("cat timed out"), + _make_result("✓\n[lane: 1]"), + ] + with pytest.raises(backend.MCPToolTimeoutError, match="cat timed out"): + backend.cat("/main/btn", session=sess) + assert mock_call.call_count == 3 + assert mock_call.call_args_list[2].args[0] == "cd %here%" + + +@patch.object(backend, "_call_execute", new_callable=AsyncMock) +def test_click_absolute_restores_after_op_raises(mock_call): + sess = _make_session(working_dir="/main") + mock_call.side_effect = [ + _make_result("✓ Entered\n[lane: 1]"), + backend.MCPToolTimeoutError("click timed out"), + _make_result("✓\n[lane: 1]"), + ] + with pytest.raises(backend.MCPToolTimeoutError, match="click timed out"): + backend.click("/main/button[0]", session=sess) + assert mock_call.call_count == 3 + assert mock_call.call_args_list[2].args[0] == "cd %here%/main" + + +@patch.object(backend, "_call_execute", new_callable=AsyncMock) +def test_grep_rooted_absolute_restores_after_op_raises(mock_call): + mock_call.side_effect = [ + _make_result("✓ Entered\n[lane: 1]"), + backend.MCPToolTimeoutError("grep timed out"), + _make_result("✓\n[lane: 1]"), + ] + with pytest.raises(backend.MCPToolTimeoutError, match="grep timed out"): + backend.grep( + "Login", path="/main", prev="/", session=_make_session(working_dir="/"), + ) + assert mock_call.call_count == 3 + assert mock_call.call_args_list[2].args[0] == "cd %here%" + + +@patch.object(backend, "_call_execute", new_callable=AsyncMock) +def test_type_text_absolute_restores_after_focus_raises(mock_call): + """focus itself raising (not just returning an error dict) must + still trigger the restore — the anchor already moved the lane + cwd.""" + sess = _make_session(working_dir="/main") + mock_call.side_effect = [ + _make_result("✓ Entered\n[lane: 1]"), # anchor ok + backend.MCPToolTimeoutError("focus timed out"), # focus raises + _make_result("✓\n[lane: 1]"), # restore + ] + with pytest.raises(backend.MCPToolTimeoutError, match="focus timed out"): + backend.type_text("/main/input", "hello", session=sess) + assert mock_call.call_count == 3 + assert mock_call.call_args_list[2].args[0] == "cd %here%/main" + + +@patch.object(backend, "_call_execute", new_callable=AsyncMock) +def test_type_text_absolute_restores_after_type_raises(mock_call): + """type itself raising after a successful focus must still trigger + the restore — the lane cwd was moved by the anchor and must not + stay parked there.""" + sess = _make_session(working_dir="/main") + mock_call.side_effect = [ + _make_result("✓ Entered\n[lane: 1]"), # anchor ok + _make_result("✓ Focused\n[lane: 1]"), # focus ok + backend.MCPToolTimeoutError("type timed out"), # type raises + _make_result("✓\n[lane: 1]"), # restore + ] + with pytest.raises(backend.MCPToolTimeoutError, match="type timed out"): + backend.type_text("/main/input", "hello", session=sess) + assert mock_call.call_count == 4 + assert mock_call.call_args_list[3].args[0] == "cd %here%/main" + + +@patch.object(backend, "_call_execute", new_callable=AsyncMock) +def test_ls_absolute_restore_failure_does_not_mask_op_error(mock_call): + """If both the operation and the best-effort restore fail, the + original operation error must be what propagates — not the + restore's own failure.""" + sess = _make_session(working_dir="/main") + mock_call.side_effect = [ + _make_result("✓ Entered\n[lane: 1]"), + backend.MCPToolTimeoutError("ls timed out"), + RuntimeError("restore also failed"), + ] + with pytest.raises(backend.MCPToolTimeoutError, match="ls timed out"): + backend.ls("/main", session=sess) + assert mock_call.call_count == 3 + + +@pytest.mark.parametrize( + ("invoke", "anchor_cmd", "restore_cmd"), + [ + pytest.param( + lambda sess: backend.ls("/main", session=sess), + "cd %here%/main", + "cd %here%/main", + id="ls", + ), + pytest.param( + lambda sess: backend.cat("/main/btn", session=sess), + "cd %here%", + "cd %here%/main", + id="cat", + ), + pytest.param( + lambda sess: backend.grep("Login", path="/main", session=sess), + "cd %here%/main", + "cd %here%/main", + id="grep", + ), + pytest.param( + lambda sess: backend.click("/main/button[0]", session=sess), + "cd %here%", + "cd %here%/main", + id="click", + ), + pytest.param( + lambda sess: backend.type_text("/main/input", "hello", session=sess), + "cd %here%", + "cd %here%/main", + id="type-text", + ), + ], +) +def test_absolute_wrappers_restore_after_anchor_timeout( + invoke, anchor_cmd, restore_cmd, +): + sess = _make_session(working_dir="/main") + mock_call = AsyncMock(side_effect=[ + backend.MCPToolTimeoutError("anchor timed out"), + _make_result("restored\n[lane: 1]"), + ]) + + with patch.object(backend, "_call_execute", mock_call): + with pytest.raises(backend.MCPToolTimeoutError, match="anchor timed out"): + invoke(sess) + + assert [item.args[0] for item in mock_call.call_args_list] == [ + anchor_cmd, + restore_cmd, + ] diff --git a/browser/agent-harness/cli_anything/browser/utils/domshell_backend.py b/browser/agent-harness/cli_anything/browser/utils/domshell_backend.py index d344a2b3f5..c0b2141192 100644 --- a/browser/agent-harness/cli_anything/browser/utils/domshell_backend.py +++ b/browser/agent-harness/cli_anything/browser/utils/domshell_backend.py @@ -25,7 +25,7 @@ import subprocess import shutil import threading -from typing import Any, Optional +from typing import Any, Awaitable, Optional from mcp import ClientSession, StdioServerParameters from mcp.client.stdio import stdio_client @@ -38,6 +38,13 @@ # DOMSHELL_TOKEN — auth token (required, must match the running server) # DOMSHELL_PORT — MCP HTTP port of the running server (default: 3001) DEFAULT_SERVER_CMD = "npx" +DEFAULT_TOOL_TIMEOUT_SECONDS = 20.0 +DEFAULT_INIT_TIMEOUT_SECONDS = 120.0 +DAEMON_STOP_TIMEOUT_SECONDS = 5.0 + + +class MCPToolTimeoutError(RuntimeError): + """Raised when a DOMShell MCP operation exceeds configured timeout.""" def _build_server_args() -> list[str]: @@ -57,6 +64,92 @@ def _build_server_args() -> list[str]: "--token", token, ] + +def _get_tool_timeout_seconds() -> float: + """Read MCP tool-call timeout (seconds) from env with safe bounds.""" + raw = os.environ.get( + "CLI_ANYTHING_BROWSER_MCP_TIMEOUT", + str(DEFAULT_TOOL_TIMEOUT_SECONDS), + ) + try: + parsed = float(raw) + except (TypeError, ValueError): + return DEFAULT_TOOL_TIMEOUT_SECONDS + return max(1.0, parsed) + + +def _get_init_timeout_seconds() -> float: + """Read MCP session-initialize timeout (seconds) from env with safe bounds. + + Initialize can legitimately take much longer than a regular tool call + (e.g. spawning npx/domshell-proxy and completing the MCP handshake), so it + is configured separately from CLI_ANYTHING_BROWSER_MCP_TIMEOUT. + """ + raw = os.environ.get( + "CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT", + str(DEFAULT_INIT_TIMEOUT_SECONDS), + ) + try: + parsed = float(raw) + except (TypeError, ValueError): + return DEFAULT_INIT_TIMEOUT_SECONDS + return max(1.0, parsed) + + +def _same_task_timeout(timeout_seconds: float) -> Any: + """Return a timeout context that does not move work to another task.""" + if hasattr(asyncio, "timeout"): + return asyncio.timeout(timeout_seconds) + from async_timeout import timeout + return timeout(timeout_seconds) + + +def _timeout_error(operation: str, timeout_seconds: float) -> str: + return ( + "DOMShell MCP request timed out after " + f"{timeout_seconds:.1f}s during {operation}. " + "Verify DOMSHELL_TOKEN and that the DOMShell server is reachable. " + "Adjust CLI_ANYTHING_BROWSER_MCP_TIMEOUT for tool calls or " + "CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT for initialization." + ) + + +async def _await_with_timeout( + coro: Awaitable[Any], + operation: str, + timeout_seconds: Optional[float] = None, +) -> Any: + """Await an MCP operation with timeout and actionable error text. + + Callers that need to distinguish init vs. tool-call timeouts (or capture + the timeout once for a whole operation) should pass timeout_seconds + explicitly; it otherwise defaults to the tool-call timeout. + """ + if timeout_seconds is None: + timeout_seconds = _get_tool_timeout_seconds() + try: + async with _same_task_timeout(timeout_seconds): + return await coro + except asyncio.TimeoutError as e: + raise MCPToolTimeoutError( + _timeout_error(operation, timeout_seconds) + ) from e + + +async def _enter_context_with_timeout( + context: Any, + operation: str, + timeout_seconds: float, +) -> Any: + """Enter an AnyIO-backed context with a timeout in the current task.""" + try: + async with _same_task_timeout(timeout_seconds): + return await context.__aenter__() + except asyncio.TimeoutError as e: + raise MCPToolTimeoutError( + _timeout_error(operation, timeout_seconds) + ) from e + # Daemon mode: persistent MCP connection _daemon_session: Optional[ClientSession] = None _daemon_read: Optional[Any] = None @@ -299,6 +392,39 @@ def _restore_cwd_cmd(session: Any) -> str: return f"cd {_q(stripped)}" if stripped else f"cd {_here_path('')}" +def _best_effort_restore( + restore_cmd: str, use_daemon: bool, session: Any, +) -> None: + """Attempt a restore-cwd call without masking the primary result. + + Absolute-path wrappers (ls/cat/click/grep/type_text) anchor a `cd` + before their real operation and restore afterward. If the operation + raises or returns, restore must still be attempted so the lane's cwd + does not stay parked at the anchor. Restore errors are logged and + swallowed because cleanup must not replace the operation's result. + """ + try: + asyncio.run(_call_execute(restore_cmd, use_daemon, session=session)) + except Exception: + log.warning( + "DOMShell lane cwd restore failed", + exc_info=True, + ) + + +def _anchor_with_restore_on_error( + anchor_cmd: str, restore_cmd: str, use_daemon: bool, session: Any, +) -> Any: + """Run an anchor command and restore cwd if it raises.""" + try: + return asyncio.run( + _call_execute(anchor_cmd, use_daemon, session=session) + ) + except Exception: + _best_effort_restore(restore_cmd, use_daemon, session) + raise + + def _anchor_path_cmd(deeper: str = "") -> str: """Build a single ``cd %here%[/]`` command line, shell-quoted. @@ -545,11 +671,18 @@ async def _call_execute( else: arguments["group_id"] = "new" + # Capture timeouts once per operation so env mutations mid-call (e.g. from + # another thread) can't apply inconsistent timeouts across the awaits below. + tool_timeout = _get_tool_timeout_seconds() + init_timeout = _get_init_timeout_seconds() + if use_daemon and _daemon_session is not None: # Use persistent daemon connection try: - result = await _daemon_session.call_tool( - "domshell_execute", arguments + result = await _await_with_timeout( + _daemon_session.call_tool("domshell_execute", arguments), + "domshell_execute (daemon)", + tool_timeout, ) _capture_lane(session, result) # Daemon-level capture on first daemon-no-session call so @@ -564,6 +697,10 @@ async def _call_execute( if captured: _daemon_lane_id = captured return result + except MCPToolTimeoutError: + # Do not auto-retry this call, but reset daemon for future recovery. + await _stop_daemon() + raise except Exception as e: # Daemon died — log diagnosability and fall back to spawning # a fresh server below. Silent swallow was making the daemon @@ -580,36 +717,83 @@ async def _call_execute( args=_build_server_args() ) + client_context = None + session_context = None try: - async with stdio_client(server_params) as (read, write): - async with ClientSession(read, write) as mcp_session: - await mcp_session.initialize() - result = await mcp_session.call_tool( - "domshell_execute", arguments - ) - _capture_lane(session, result) - # See daemon-path capture above for rationale — same - # logic applies on the fresh-spawn fall-back path, - # including the lock-guarded test-and-set. - if use_daemon and session is None: - with _daemon_lane_lock: - if _daemon_lane_id is None: - captured = _extract_lane_id(result) - if captured: - _daemon_lane_id = captured - return result + client_context = stdio_client(server_params) + read, write = await _enter_context_with_timeout( + client_context, "stdio client enter", init_timeout + ) + session_context = ClientSession(read, write) + mcp_session = await _enter_context_with_timeout( + session_context, "session enter", init_timeout + ) + await _await_with_timeout( + mcp_session.initialize(), "session initialize", init_timeout + ) + result = await _await_with_timeout( + mcp_session.call_tool("domshell_execute", arguments), + "domshell_execute", + tool_timeout, + ) + _capture_lane(session, result) + # See daemon-path capture above for rationale — same + # logic applies on the fresh-spawn fall-back path, + # including the lock-guarded test-and-set. + if use_daemon and session is None: + with _daemon_lane_lock: + if _daemon_lane_id is None: + captured = _extract_lane_id(result) + if captured: + _daemon_lane_id = captured + return result + except RuntimeError as e: + # Pass through MCPToolTimeoutError unchanged so callers can recover; + # but wrap other RuntimeErrors with actionable context below. + if isinstance(e, MCPToolTimeoutError): + raise + raise RuntimeError( + f"DOMShell MCP call failed: {e}\n" + f"Ensure Chrome is running with DOMShell extension installed.\n" + f"Chrome Web Store: https://chromewebstore.google.com/detail/domshell" + ) from e except Exception as e: raise RuntimeError( f"DOMShell MCP call failed: {e}\n" f"Ensure Chrome is running with DOMShell extension installed.\n" f"Chrome Web Store: https://chromewebstore.google.com/detail/domshell" ) from e + finally: + await _close_daemon_transport(session_context, client_context) # NOTE: Known limitation - Daemon mode uses asyncio.run() per tool call (in sync wrappers). # Each asyncio.run() creates a new event loop. Async IO objects created in one loop # (like the daemon session) may have issues when accessed from subsequent calls that # create new loops. This is a documented limitation for v1; future work should use # a single long-lived event loop (e.g., background thread + run_coroutine_threadsafe). +async def _close_daemon_transport(session: Any, client_context: Any) -> None: + """Best-effort, bounded exit of entered MCP contexts. + + Shared by per-command calls and daemon startup/shutdown. Each + ``__aexit__`` runs in the task that entered the AnyIO-backed context + and is bounded by ``DAEMON_STOP_TIMEOUT_SECONDS``. Failures are + swallowed so cleanup cannot hide the operation result. Either argument + may be ``None`` and is skipped. + """ + if session is not None: + try: + async with _same_task_timeout(DAEMON_STOP_TIMEOUT_SECONDS): + await session.__aexit__(None, None, None) + except (Exception, asyncio.TimeoutError): + pass + if client_context is not None: + try: + async with _same_task_timeout(DAEMON_STOP_TIMEOUT_SECONDS): + await client_context.__aexit__(None, None, None) + except (Exception, asyncio.TimeoutError): + pass + + async def _start_daemon() -> bool: """Start persistent daemon mode. @@ -631,13 +815,28 @@ async def _start_daemon() -> bool: try: # Store the context manager so we can properly clean it up later + init_timeout = _get_init_timeout_seconds() _daemon_client_context = stdio_client(server_params) - _daemon_read, _daemon_write = await _daemon_client_context.__aenter__() + _daemon_read, _daemon_write = await _enter_context_with_timeout( + _daemon_client_context, "daemon stdio enter", init_timeout + ) _daemon_session = ClientSession(_daemon_read, _daemon_write) - await _daemon_session.__aenter__() - await _daemon_session.initialize() + await _enter_context_with_timeout( + _daemon_session, "daemon session enter", init_timeout + ) + await _await_with_timeout( + _daemon_session.initialize(), + "daemon initialize", + init_timeout, + ) return True except Exception as e: + # An initialize() timeout/error can land after the stdio + # transport and/or the ClientSession have already been + # __aenter__'d. Exiting them here (best-effort, bounded) is + # required — otherwise the underlying npx/DOMShell subprocess + # and stdio pipes are orphaned, not just the Python globals. + await _close_daemon_transport(_daemon_session, _daemon_client_context) _daemon_session = None _daemon_read = None _daemon_write = None @@ -646,23 +845,22 @@ async def _start_daemon() -> bool: async def _stop_daemon() -> None: - """Stop persistent daemon mode.""" + """Stop persistent daemon mode. + + Cleanup is bounded to a fixed timeout (DAEMON_STOP_TIMEOUT_SECONDS) to + avoid hanging if the daemon transport is wedged. Not configurable via + env var: this is an internal safety bound, not a user-tunable setting. + """ global _daemon_session, _daemon_read, _daemon_write, _daemon_client_context if _daemon_session is None: return - try: - await _daemon_session.__aexit__(None, None, None) - if _daemon_client_context: - await _daemon_client_context.__aexit__(None, None, None) - except Exception: - pass # Ignore cleanup errors - finally: - _daemon_session = None - _daemon_read = None - _daemon_write = None - _daemon_client_context = None + await _close_daemon_transport(_daemon_session, _daemon_client_context) + _daemon_session = None + _daemon_read = None + _daemon_write = None + _daemon_client_context = None def daemon_started() -> bool: @@ -710,17 +908,24 @@ def ls(path: str = "/", use_daemon: bool = False, *, session: Any = None) -> dic # wrong-target results. Three separate _call_execute calls so # we can _is_error-gate after the anchor and skip the operation # cleanly. All share the persisted lane via session. - anchor = asyncio.run(_call_execute( - _anchor_path_cmd(translated), use_daemon, session=session, - )) + restore_cmd = _restore_cwd_cmd(session) + anchor = _anchor_with_restore_on_error( + _anchor_path_cmd(translated), + restore_cmd, + use_daemon, + session, + ) if _is_error(anchor): return _parse_execute_result(anchor, "ls") - op = asyncio.run(_call_execute("ls", use_daemon, session=session)) - # Best-effort restore — ls already ran; restore failure is - # cosmetic (next harness cd corrects any drift). - asyncio.run(_call_execute( - _restore_cwd_cmd(session), use_daemon, session=session, - )) + try: + op = asyncio.run(_call_execute("ls", use_daemon, session=session)) + except Exception: + # ls raised (e.g. a timeout) instead of returning an error + # dict — restore must still be attempted so the lane cwd + # doesn't stay parked at the anchor. + _best_effort_restore(restore_cmd, use_daemon, session) + raise + _best_effort_restore(restore_cmd, use_daemon, session) return _parse_execute_result(op, "ls") if translated: op = asyncio.run(_call_execute( @@ -757,10 +962,10 @@ def cd(path: str, use_daemon: bool = False, *, session: Any = None) -> dict: "output": "cd: /missing: No such directory"} """ translated, is_absolute = _translate_path(path) - # cd is the one wrapper where the operation IS the new state — no - # following operation needs the anchored cwd, so no split-and-check - # and no restore. Absolute targets anchor via `cd %here%/` so - # the result is independent of the lane's current cwd. + # cd is the one wrapper where the operation IS the new state, so no + # restore follows a successful call. Absolute targets anchor via + # `cd %here%/` so the result is independent of the lane's + # current cwd. if is_absolute: command = _anchor_path_cmd(translated) elif translated: @@ -768,7 +973,12 @@ def cd(path: str, use_daemon: bool = False, *, session: Any = None) -> dict: else: # Bare/empty `cd` → back to tab root. command = _anchor_path_cmd("") - result = asyncio.run(_call_execute(command, use_daemon, session=session)) + if session is not None: + result = _anchor_with_restore_on_error( + command, _restore_cwd_cmd(session), use_daemon, session + ) + else: + result = asyncio.run(_call_execute(command, use_daemon, session=session)) return _parse_execute_result(result, "cd") @@ -802,17 +1012,23 @@ def cat(path: str, use_daemon: bool = False, *, session: Any = None) -> dict: # otherwise run relative cat, restore. Anchor success is # load-bearing — without it cat resolves the relative path # against the wrong cwd. - anchor = asyncio.run(_call_execute( - _anchor_path_cmd(""), use_daemon, session=session, - )) + restore_cmd = _restore_cwd_cmd(session) + anchor = _anchor_with_restore_on_error( + _anchor_path_cmd(""), + restore_cmd, + use_daemon, + session, + ) if _is_error(anchor): return _parse_execute_result(anchor, "cat") - op = asyncio.run(_call_execute( - f"cat {_q(translated)}", use_daemon, session=session, - )) - asyncio.run(_call_execute( - _restore_cwd_cmd(session), use_daemon, session=session, - )) + try: + op = asyncio.run(_call_execute( + f"cat {_q(translated)}", use_daemon, session=session, + )) + except Exception: + _best_effort_restore(restore_cmd, use_daemon, session) + raise + _best_effort_restore(restore_cmd, use_daemon, session) return _parse_execute_result(op, "cat") op = asyncio.run(_call_execute( f"cat {_q(translated)}", use_daemon, session=session, @@ -923,15 +1139,21 @@ def grep( else _anchor_path_cmd("") ) - anchor = asyncio.run(_call_execute(anchor_cmd, use_daemon, session=session)) + anchor = _anchor_with_restore_on_error( + anchor_cmd, restore_cmd, use_daemon, session, + ) if _is_error(anchor): return _parse_execute_result(anchor, "grep") # `-r` preserves the pre-migration recursive default (see unrooted # branch above for the full rationale). - op = asyncio.run(_call_execute( - f"grep -r {_q(pattern)}", use_daemon, session=session, - )) - asyncio.run(_call_execute(restore_cmd, use_daemon, session=session)) + try: + op = asyncio.run(_call_execute( + f"grep -r {_q(pattern)}", use_daemon, session=session, + )) + except Exception: + _best_effort_restore(restore_cmd, use_daemon, session) + raise + _best_effort_restore(restore_cmd, use_daemon, session) return _parse_execute_result(op, "grep") @@ -961,17 +1183,23 @@ def click(path: str, use_daemon: bool = False, *, session: Any = None) -> dict: # otherwise click the relative path, restore. Anchor success is # load-bearing — clicking the wrong element if cwd has drifted # could trigger an unintended action. - anchor = asyncio.run(_call_execute( - _anchor_path_cmd(""), use_daemon, session=session, - )) + restore_cmd = _restore_cwd_cmd(session) + anchor = _anchor_with_restore_on_error( + _anchor_path_cmd(""), + restore_cmd, + use_daemon, + session, + ) if _is_error(anchor): return _parse_execute_result(anchor, "click") - op = asyncio.run(_call_execute( - f"click {_q(translated)}", use_daemon, session=session, - )) - asyncio.run(_call_execute( - _restore_cwd_cmd(session), use_daemon, session=session, - )) + try: + op = asyncio.run(_call_execute( + f"click {_q(translated)}", use_daemon, session=session, + )) + except Exception: + _best_effort_restore(restore_cmd, use_daemon, session) + raise + _best_effort_restore(restore_cmd, use_daemon, session) return _parse_execute_result(op, "click") op = asyncio.run(_call_execute( f"click {_q(translated)}", use_daemon, session=session, @@ -1128,7 +1356,6 @@ def type_text( "(Daemon mode shares the persistent connection's lane " "automatically and doesn't need session.)" ) - translated_path, is_absolute = _translate_path(path) if not translated_path: raise ValueError( @@ -1150,37 +1377,49 @@ def type_text( # we can _is_error-check, then type, then restore as a separate # best-effort call. All four share the persisted lane via session. if is_absolute: - anchor_result = asyncio.run(_call_execute( - _anchor_path_cmd(""), use_daemon, session=session, - )) + restore_cmd = _restore_cwd_cmd(session) + anchor_result = _anchor_with_restore_on_error( + _anchor_path_cmd(""), + restore_cmd, + use_daemon, + session, + ) if _is_error(anchor_result): # Anchor failed — we never moved, so no restore is needed. return _parse_execute_result(anchor_result, "focus") - focus_result = asyncio.run(_call_execute( - f"focus {_q(translated_path)}", use_daemon, session=session, - )) + try: + focus_result = asyncio.run(_call_execute( + f"focus {_q(translated_path)}", use_daemon, session=session, + )) + except Exception: + # focus raised (e.g. a timeout) instead of returning an error + # dict. Only relevant to restore when we actually moved (the + # absolute path branch) — mirrors the _is_error restore below. + if is_absolute: + _best_effort_restore(restore_cmd, use_daemon, session) + raise if _is_error(focus_result): # Focus failed — restore cwd before returning so the lane # doesn't stay parked at the anchor (only relevant when we # actually moved, i.e. the absolute path branch). if is_absolute: - asyncio.run(_call_execute( - _restore_cwd_cmd(session), use_daemon, session=session, - )) + _best_effort_restore(restore_cmd, use_daemon, session) return _parse_execute_result(focus_result, "focus") - type_result = asyncio.run(_call_execute( - f"type {_q(text)}", use_daemon, session=session, - )) + try: + type_result = asyncio.run(_call_execute( + f"type {_q(text)}", use_daemon, session=session, + )) + except Exception: + # type raised after a successful focus — the anchor already + # moved the lane cwd, so restore must still be attempted. + if is_absolute: + _best_effort_restore(restore_cmd, use_daemon, session) + raise if is_absolute: - # Best-effort restore — type already succeeded, so a restore - # failure is cosmetic. The next harness cd will correct any - # drift. - asyncio.run(_call_execute( - _restore_cwd_cmd(session), use_daemon, session=session, - )) + _best_effort_restore(restore_cmd, use_daemon, session) return _parse_execute_result(type_result, "type") diff --git a/browser/agent-harness/setup.py b/browser/agent-harness/setup.py index 0c14a3ea4b..c3e1d205c7 100644 --- a/browser/agent-harness/setup.py +++ b/browser/agent-harness/setup.py @@ -13,7 +13,7 @@ def read_readme(): setup( name="cli-anything-browser", - version="1.0.0", + version="1.0.1", author="CLI Anything Contributors", description="CLI harness for browser automation via DOMShell MCP server", @@ -32,6 +32,7 @@ def read_readme(): python_requires=">=3.10", install_requires=[ + "async-timeout>=4.0,<6.0; python_version < '3.11'", "click>=8.1,<9.0", "prompt-toolkit>=3.0,<4.0", "mcp>=0.1.0,<1.0.0", diff --git a/registry.json b/registry.json index d3ac62d637..9ef42335e4 100644 --- a/registry.json +++ b/registry.json @@ -198,7 +198,7 @@ { "name": "browser", "display_name": "Browser", - "version": "1.0.0", + "version": "1.0.1", "description": "Browser automation via DOMShell MCP server. Maps Chrome's Accessibility Tree to a virtual filesystem for agent-native navigation.", "requires": "Node.js, npx, Chrome + DOMShell extension", "homepage": "https://github.com/apireno/DOMShell", diff --git a/skills/cli-anything-browser/SKILL.md b/skills/cli-anything-browser/SKILL.md index 73b9e4d4a2..0185fed9c9 100644 --- a/skills/cli-anything-browser/SKILL.md +++ b/skills/cli-anything-browser/SKILL.md @@ -158,6 +158,7 @@ The CLI provides clear error messages for common issues: - **npx not found**: Install Node.js from https://nodejs.org/ - **DOMShell not found**: Run `npx @apireno/domshell --version` - **MCP call failed**: Install DOMShell Chrome extension +- **MCP timeout / hanging calls**: Verify `DOMSHELL_TOKEN`, ensure DOMShell server is running, and optionally tune timeouts via `CLI_ANYTHING_BROWSER_MCP_TIMEOUT` (default: 20s, per tool call) and `CLI_ANYTHING_BROWSER_MCP_INIT_TIMEOUT` (default: 120s, session initialize) Check `is_available()` return value before running commands.