diff --git a/agent/copilot_acp_client.py b/agent/copilot_acp_client.py index 759ebaceb7..42be04395d 100644 --- a/agent/copilot_acp_client.py +++ b/agent/copilot_acp_client.py @@ -74,8 +74,16 @@ def _resolve_args() -> list[str]: return shlex.split(raw) -def _acp_supported(command: str, args: list[str]) -> bool: - """Return True iff ``command`` accepts the ACP args we'd pass. +# Probe verdicts cached per binary path so repeated prompts against a +# CLI that supports --acp pay the ~50ms --help cost exactly once per +# process. Only definitive verdicts (True/False) are cached; an +# inconclusive probe (binary missing, --help crashed or timed out) is +# not cached so a CLI installed mid-session is picked up. +_ACP_PROBE_CACHE: dict[str, bool] = {} + + +def _acp_supported(command: str, args: list[str]) -> bool | None: + """Tri-state probe: does ``command`` accept the ACP args we'd pass? Different CLI versions support different transports. The GitHub Copilot CLI (`@github/copilot`, late 2025+) ships with ``--acp``; @@ -86,21 +94,38 @@ def _acp_supported(command: str, args: list[str]) -> bool: ``child_timeout_seconds`` (default 600s) waiting for stdout that never arrives. - This probe fires once per ACP prompt, runs in ~50ms, and - short-circuits to a clear error before any spawn happens. + Returns: + - ``True`` — help text advertises ``--acp``; safe to spawn. + - ``False`` — help ran cleanly but ``--acp`` is absent; spawning + would hang, so the caller should fast-fail with a clear error. + - ``None`` — inconclusive (binary missing, --help failed or + timed out). The caller must fall through to the normal spawn + path, which surfaces the existing "Could not start Copilot ACP + command" error with full context. + + Only probes when ``--acp`` is actually among ``args``: a custom + HERMES_COPILOT_ACP_ARGS transport is the operator's business. """ + if "--acp" not in args: + return True + cached = _ACP_PROBE_CACHE.get(command) + if cached is not None: + return cached try: probe = subprocess.run( [command, "--help"], capture_output=True, text=True, timeout=5, ) except (FileNotFoundError, subprocess.TimeoutExpired, OSError): - return False + return None if probe.returncode != 0: - return False + # --help itself failed; can't tell anything about --acp. + return None # Match ``--acp`` as a flag in the help text; tolerate spacing and # variants like ``[--acp]``. - return bool(re.search(r"(?:^|\s)--acp(?:\s|=|\]|\b)", probe.stdout)) + verdict = bool(re.search(r"(?:^|[\s\[])--acp(?:[\s=\],]|$)", probe.stdout, re.MULTILINE)) + _ACP_PROBE_CACHE[command] = verdict + return verdict def _resolve_home_dir() -> str: @@ -537,7 +562,10 @@ class CopilotACPClient: # ACP loop waits the full ``child_timeout_seconds`` (default 600s) # for stdout that never arrives. The probe costs ~50ms and turns # a 600s silent hang into a 280ms clear error. - if not _acp_supported(self._acp_command, self._acp_args): + # ``None`` (inconclusive probe — e.g. binary missing) falls + # through to the spawn below, which raises the established + # "Could not start Copilot ACP command" error. + if _acp_supported(self._acp_command, self._acp_args) is False: preview = " ".join(self._acp_args[:3]) if self._acp_args else "(none)" raise RuntimeError( f"ACP transport not supported by '{self._acp_command}': " diff --git a/tests/agent/test_copilot_acp_client.py b/tests/agent/test_copilot_acp_client.py index a6b366c9c9..100dca67f4 100644 --- a/tests/agent/test_copilot_acp_client.py +++ b/tests/agent/test_copilot_acp_client.py @@ -211,9 +211,13 @@ def test_run_prompt_preserves_real_home_when_profile_home_available(monkeypatch, captured = {} client = _make_home_client(tmp_path) - with _patch("agent.copilot_acp_client.subprocess.Popen", side_effect=_fake_popen_capture(captured)): - with pytest.raises(RuntimeError, match="Could not start Copilot ACP command"): - client._run_prompt("hello", timeout_seconds=1) + # Hermeticity: the --acp support probe (PR #87308) calls subprocess.run + # before Popen; stub it inconclusive so no real CLI on the host box can + # flip the resolution this test asserts. + with _patch("agent.copilot_acp_client.subprocess.run", side_effect=FileNotFoundError): + with _patch("agent.copilot_acp_client.subprocess.Popen", side_effect=_fake_popen_capture(captured)): + with pytest.raises(RuntimeError, match="Could not start Copilot ACP command"): + client._run_prompt("hello", timeout_seconds=1) assert captured["kwargs"]["env"]["HOME"] == str(real_home) assert captured["kwargs"]["env"]["HERMES_REAL_HOME"] == str(real_home) @@ -226,9 +230,90 @@ def test_run_prompt_passes_home_when_parent_env_is_clean(monkeypatch, tmp_path): captured = {} client = _make_home_client(tmp_path) - with _patch("agent.copilot_acp_client.subprocess.Popen", side_effect=_fake_popen_capture(captured)): - with pytest.raises(RuntimeError, match="Could not start Copilot ACP command"): - client._run_prompt("hello", timeout_seconds=1) + # Hermeticity: the --acp support probe (PR #87308) calls subprocess.run + # before Popen; stub it inconclusive so no real CLI on the host box can + # flip the resolution this test asserts. + with _patch("agent.copilot_acp_client.subprocess.run", side_effect=FileNotFoundError): + with _patch("agent.copilot_acp_client.subprocess.Popen", side_effect=_fake_popen_capture(captured)): + with pytest.raises(RuntimeError, match="Could not start Copilot ACP command"): + client._run_prompt("hello", timeout_seconds=1) assert "env" in captured["kwargs"] assert captured["kwargs"]["env"]["HOME"] + + +# ── --acp support probe tests (PR #87308 / issue #87309) ──────────── + +import subprocess as _subprocess + +from agent.copilot_acp_client import _ACP_PROBE_CACHE, _acp_supported + + +@pytest.fixture(autouse=True) +def _clear_probe_cache(): + _ACP_PROBE_CACHE.clear() + yield + _ACP_PROBE_CACHE.clear() + + +def _completed(returncode=0, stdout=""): + return _subprocess.CompletedProcess(args=[], returncode=returncode, stdout=stdout, stderr="") + + +def test_probe_true_when_help_advertises_acp(): + with _patch( + "agent.copilot_acp_client.subprocess.run", + return_value=_completed(stdout="Usage: copilot [--acp] [--stdio]"), + ): + assert _acp_supported("copilot", ["--acp", "--stdio"]) is True + + +def test_probe_false_when_help_lacks_acp_and_run_prompt_fast_fails(tmp_path): + client = _make_home_client(tmp_path) + with _patch( + "agent.copilot_acp_client.subprocess.run", + return_value=_completed(stdout="Usage: claude [--print] [--model]"), + ): + with pytest.raises(RuntimeError, match="ACP transport not supported"): + client._run_prompt("hello", timeout_seconds=1) + + +def test_probe_inconclusive_falls_through_to_spawn_error(tmp_path): + """Missing binary: probe must NOT mask the established spawn error.""" + client = _make_home_client(tmp_path) + with _patch( + "agent.copilot_acp_client.subprocess.run", + side_effect=FileNotFoundError("copilot not found"), + ): + with _patch( + "agent.copilot_acp_client.subprocess.Popen", + side_effect=FileNotFoundError("copilot not found"), + ): + with pytest.raises(RuntimeError, match="Could not start Copilot ACP command"): + client._run_prompt("hello", timeout_seconds=1) + + +def test_probe_result_cached_per_binary_path(): + with _patch( + "agent.copilot_acp_client.subprocess.run", + return_value=_completed(stdout="Usage: copilot [--acp]"), + ) as run_mock: + assert _acp_supported("copilot", ["--acp"]) is True + assert _acp_supported("copilot", ["--acp"]) is True + assert run_mock.call_count == 1 + + +def test_probe_inconclusive_not_cached(): + with _patch( + "agent.copilot_acp_client.subprocess.run", + side_effect=FileNotFoundError, + ) as run_mock: + assert _acp_supported("copilot", ["--acp"]) is None + assert _acp_supported("copilot", ["--acp"]) is None + assert run_mock.call_count == 2 # inconclusive verdicts retry + + +def test_probe_skipped_for_custom_args_without_acp(): + with _patch("agent.copilot_acp_client.subprocess.run") as run_mock: + assert _acp_supported("mycli", ["--custom-transport"]) is True + run_mock.assert_not_called()