fix(acp): make --acp probe tri-state, cached, and mock-safe
Salvage hardening on top of #87308 (thanks @Dudeman456): - Tri-state verdict: inconclusive probes (binary missing, --help failed/timed out) return None and fall through to the normal spawn path, preserving the established 'Could not start Copilot ACP command' error instead of masking it. This also fixes the two test_copilot_acp_client HOME-env regressions that went red on the PR: their mocked-Popen path was intercepted by the new unmocked subprocess.run probe. - Cache definitive verdicts per binary path so CLIs that DO support --acp pay the ~50ms --help cost once per process, not per prompt. - Skip the probe entirely when custom ACP args don't include --acp. - Fix the help-text regex: the old pattern never matched '[--acp]' (leading '[' is neither start-of-string nor whitespace) and \b after 'p' matched '--acpfoo'. - Hermeticity: stub subprocess.run in the two HOME-env tests; add 6 probe-specific tests (fast-fail, fall-through, caching, skip).
This commit is contained in:
Teknium
@@ -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}': "
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user