diff --git a/webqa_agent/executor/flash/core/mcp_client.py b/webqa_agent/executor/flash/core/mcp_client.py index dbe6fe8..3f8110a 100644 --- a/webqa_agent/executor/flash/core/mcp_client.py +++ b/webqa_agent/executor/flash/core/mcp_client.py @@ -152,11 +152,13 @@ def start(self, startup_timeout_s: float = _DEFAULT_STARTUP_TIMEOUT) -> None: Raises MCPError if anything fails within startup_timeout_s. """ - cmd = [self._command] + self._args - # Resolve command via PATH explicitly for a clearer error message than - # Popen's generic FileNotFoundError. - if shutil.which(self._command) is None and not os.path.isabs(self._command): - raise MCPError(f'{self.name}: command not found on PATH: {self._command!r}') + resolved_command = shutil.which(self._command) + if not resolved_command: + if not os.path.isabs(self._command): + raise MCPError(f'{self.name}: command not found on PATH: {self._command!r}') + resolved_command = self._command + + cmd = [resolved_command] + self._args merged_env = None if self._env: diff --git a/webqa_agent/executor/flash/runner.py b/webqa_agent/executor/flash/runner.py index cd818dd..b45d5f7 100644 --- a/webqa_agent/executor/flash/runner.py +++ b/webqa_agent/executor/flash/runner.py @@ -574,21 +574,27 @@ def _emergency_cleanup() -> None: atexit.register(_emergency_cleanup) - _prev_sigterm: Any = None - _prev_sighup: Any = None - _signals_installed = False - try: - _prev_sigterm = signal.getsignal(signal.SIGTERM) - _prev_sighup = signal.getsignal(signal.SIGHUP) + _previous_signal_handlers: dict[int, Any] = {} + signals_to_install = [signal.SIGTERM] + + sighup = getattr(signal, "SIGHUP", None) + if sighup is not None: + signals_to_install.append(sighup) + try: def _signal_handler(signum: int, frame: Any) -> None: sys.exit(128 + signum) - signal.signal(signal.SIGTERM, _signal_handler) - signal.signal(signal.SIGHUP, _signal_handler) - _signals_installed = True + for signum in signals_to_install: + _previous_signal_handlers[signum] = signal.getsignal(signum) + signal.signal(signum, _signal_handler) except ValueError: - pass + for signum, previous_handler in _previous_signal_handlers.items(): + try: + signal.signal(signum, previous_handler) + except ValueError: + pass + _previous_signal_handlers.clear() engine: Engine | None = None state = _EventLoopState() @@ -918,9 +924,11 @@ def _on_context_overflow(messages: list[dict]) -> list[dict] | None: except Exception: pass atexit.unregister(_emergency_cleanup) - if _signals_installed: - signal.signal(signal.SIGTERM, _prev_sigterm) - signal.signal(signal.SIGHUP, _prev_sighup) + for signum, previous_handler in _previous_signal_handlers.items(): + try: + signal.signal(signum, previous_handler) + except ValueError: + pass def _resolve_cdp_port( diff --git a/webqa_agent/executor/flash/tests/test_mcp_client_start.py b/webqa_agent/executor/flash/tests/test_mcp_client_start.py new file mode 100644 index 0000000..1ee9b10 --- /dev/null +++ b/webqa_agent/executor/flash/tests/test_mcp_client_start.py @@ -0,0 +1,88 @@ +"""Tests for MCPServer.start process command resolution.""" +from __future__ import annotations + +import io +from pathlib import Path + +import pytest + +from webqa_agent.executor.flash.core import mcp_client +from webqa_agent.executor.flash.core.mcp_client import MCPError, MCPServer + + +class _FakeProc: + def __init__(self) -> None: + self.stdin = io.BytesIO() + self.stdout = io.BytesIO() + self.stderr = io.BytesIO() + self.pid = 12345 + + def wait(self, timeout=None): + return 0 + + def terminate(self) -> None: + pass + + def kill(self) -> None: + pass + + +@pytest.fixture +def isolated_start(monkeypatch): + popen_calls = [] + + def fake_popen(cmd, **kwargs): + popen_calls.append((list(cmd), dict(kwargs))) + return _FakeProc() + + monkeypatch.setattr(mcp_client.subprocess, 'Popen', fake_popen) + monkeypatch.setattr(MCPServer, '_read_stdout', lambda self: None) + monkeypatch.setattr(MCPServer, '_drain_stderr', lambda self: None) + monkeypatch.setattr(MCPServer, '_initialize', lambda self, timeout_s: None) + monkeypatch.setattr(MCPServer, '_list_tools', lambda self, timeout_s: []) + return popen_calls + + +def test_start_uses_resolved_command_from_path(isolated_start, monkeypatch): + resolved = r'C:\Users\me\AppData\Roaming\npm\chrome-devtools-mcp.CMD' + monkeypatch.setattr(mcp_client.shutil, 'which', lambda command: resolved) + + server = MCPServer( + name='browser', + command='chrome-devtools-mcp', + args=['--headless'], + ) + server.start() + server.shutdown() + + assert isolated_start[0][0][0] == resolved + assert isolated_start[0][0][1:] == ['--headless'] + + +def test_start_raises_when_non_absolute_command_is_not_found( + isolated_start, monkeypatch, +): + monkeypatch.setattr(mcp_client.shutil, 'which', lambda command: None) + + server = MCPServer(name='browser', command='missing-mcp-server') + with pytest.raises(MCPError, match="command not found on PATH"): + server.start() + + assert isolated_start == [] + + +def test_start_keeps_absolute_command_when_path_lookup_fails( + isolated_start, monkeypatch, tmp_path: Path, +): + absolute_command = str(tmp_path / 'custom-mcp-server') + monkeypatch.setattr(mcp_client.shutil, 'which', lambda command: None) + + server = MCPServer( + name='custom', + command=absolute_command, + args=['--stdio'], + ) + server.start() + server.shutdown() + + assert isolated_start[0][0] == [absolute_command, '--stdio'] diff --git a/webqa_agent/executor/flash/tests/test_runner_extension_points.py b/webqa_agent/executor/flash/tests/test_runner_extension_points.py index 36cf920..4aff58b 100644 --- a/webqa_agent/executor/flash/tests/test_runner_extension_points.py +++ b/webqa_agent/executor/flash/tests/test_runner_extension_points.py @@ -164,6 +164,66 @@ def test_default_mcp_args_include_security_flags(): '--chrome-arg=... form instead') +def _record_signal_calls(monkeypatch): + calls = [] + previous_handlers = {} + + def fake_getsignal(signum): + previous = object() + previous_handlers[signum] = previous + return previous + + def fake_signal(signum, handler): + calls.append((signum, handler)) + + monkeypatch.setattr(runner.signal, 'getsignal', fake_getsignal) + monkeypatch.setattr(runner.signal, 'signal', fake_signal) + return calls, previous_handlers + + +def test_signal_handling_without_sighup_installs_only_sigterm( + patched_runner, monkeypatch, +): + """Windows-like platforms do not expose signal.SIGHUP.""" + monkeypatch.delattr(runner.signal, 'SIGHUP', raising=False) + calls, previous_handlers = _record_signal_calls(monkeypatch) + + runner.run_cc_mini( + 'https://example.com', 'task', + api_key='fake-key', + ) + + sigterm = runner.signal.SIGTERM + assert [signum for signum, _handler in calls] == [sigterm, sigterm] + assert callable(calls[0][1]) + assert calls[1] == (sigterm, previous_handlers[sigterm]) + + +def test_signal_handling_with_sighup_installs_and_restores_both( + patched_runner, monkeypatch, +): + fake_sighup = 99999 + monkeypatch.setattr(runner.signal, 'SIGHUP', fake_sighup, raising=False) + calls, previous_handlers = _record_signal_calls(monkeypatch) + + runner.run_cc_mini( + 'https://example.com', 'task', + api_key='fake-key', + ) + + sigterm = runner.signal.SIGTERM + assert [signum for signum, _handler in calls] == [ + sigterm, + fake_sighup, + sigterm, + fake_sighup, + ] + assert callable(calls[0][1]) + assert callable(calls[1][1]) + assert calls[2] == (sigterm, previous_handlers[sigterm]) + assert calls[3] == (fake_sighup, previous_handlers[fake_sighup]) + + def test_force_start_uses_list_pages_not_tab_opening_tools(patched_runner): """Force-start must be side-effect-free.