mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
fix(computer_use): address sweeper feedback on --no-overlay subprocess + manifest probe
The hermes-sweeper review #4701565902 (2026-07-15) flagged two consistency issues in `_cua_driver_supports_no_overlay` and one additive-config concern: 1. `cua-backend.py:260` — the `cua-driver --help` support probe inherited the full parent environment. cua-driver is a third-party binary; every other spawn site in this file (manifest probe at `:214`, MCP spawn at `:697`, install probe at `:997`) uses `_sanitize_subprocess_env(cua_driver_child_env())`. The `--help` probe should match. This was a low-impact leak (only help output exits), but inconsistency is the wrong default for a third-party subprocess. 2. `cua_backend.py:238` — when the manifest returned a `command` different from the input `driver_cmd` parameter (e.g. a relocated executable at `/opt/relocated/cua-driver` while the system binary is at `/usr/bin/cua-driver`), the support probe ran against `_CUA_DRIVER_CMD` (the default) instead of the manifest-discovered `command`. Two failure modes: - The wrapper binary supports `--no-overlay` but the system binary doesn't → probe returns False → overlay kept despite capability. - The system binary supports `--no-overlay` but the wrapper doesn't → probe returns True → MCP spawn crashes on the unknown flag. 3. The original commit bumped `_config_version` 31→32 for an additive default (`computer_use.no_overlay: None`). AGENTS.md specifies that additive defaults in existing sections are handled by deep merge and should NOT trigger a version bump. After cherry-picking onto current `origin/main` (which is already at 33), the bump is effectively dropped — resolved to main's 33. Changes: - Add `env=_sanitize_subprocess_env(cua_driver_child_env())` to the `--help` subprocess (with the same import + rationale comment as the manifest probe). - Pass `driver_cmd=command` (or `driver_cmd=driver_cmd` for the fallback path) into `_mcp_args_with_overlay_flag`, so the support probe runs against the binary that will actually be launched. Tests (3 new): - `test_help_probe_passes_sanitized_env` — verifies `subprocess.run` is called with an `env=` kwarg. - `test_manifest_command_drives_support_probe` — verifies the probe runs against the manifest command when it differs from the input driver_cmd. - `test_fallback_uses_input_driver_cmd_for_support_probe` — verifies the fallback path (no command in manifest) uses the input driver_cmd. - `test_probe_distinguishes_support_between_binaries` — sanity check that the lru_cache key on `driver_cmd` prevents cross-binary cache leakage. File-revert negative test confirmed all three of the new "manifest/probe" tests are load-bearing: with the pre-fix code, they fail (probe runs against the default binary instead of the resolved one); with the fix, they pass. 20/20 tests in `tests/computer_use/test_cua_no_overlay.py` green. `TestMcpInvocationResolution` (8/8) still green. Refs: sweeper review #4701565902
This commit is contained in:
David Metcalfe
Brooklyn Nicholson
parent
f7a6c7a6e5
commit
3d84689714
2 changed files with 120 additions and 2 deletions
|
|
@ -103,6 +103,115 @@ class TestDriverSupportsNoOverlay:
|
|||
cua_backend._cua_driver_supports_no_overlay.cache_clear()
|
||||
assert cua_backend._cua_driver_supports_no_overlay("cua-driver") is False
|
||||
|
||||
def test_help_probe_passes_sanitized_env(self):
|
||||
"""The ``--help`` subprocess must not leak provider credentials
|
||||
via the inherited parent environment (third-party binary; same
|
||||
policy as the manifest probe and MCP spawn).
|
||||
"""
|
||||
from unittest.mock import MagicMock
|
||||
with patch("subprocess.run") as mock_run:
|
||||
mock_run.return_value = MagicMock(stdout="--no-overlay in help", stderr="")
|
||||
cua_backend._cua_driver_supports_no_overlay.cache_clear()
|
||||
cua_backend._cua_driver_supports_no_overlay("cua-driver")
|
||||
kwargs = mock_run.call_args.kwargs
|
||||
assert "env" in kwargs, (
|
||||
"subprocess.run was called without env= — cua-driver is a "
|
||||
"third-party binary and must not receive inherited secrets"
|
||||
)
|
||||
# The sanitized env must come from the same helper the MCP
|
||||
# spawn uses, so the policy is consistent across every
|
||||
# cua-driver invocation in this file.
|
||||
assert kwargs["env"] is not None
|
||||
|
||||
|
||||
class TestMcpInvocationUsesResolvedCommand:
|
||||
"""Surface 8 (NousResearch/hermes-agent#47072) + sweeper feedback
|
||||
#4701565902: when the manifest surfaces a relocated executable for
|
||||
``mcp_invocation.command``, the support probe must run against THAT
|
||||
binary, not the system-resolved ``_CUA_DRIVER_CMD``. Otherwise a
|
||||
wrapper/relocation with a different feature set either crashes on
|
||||
the unknown flag (when the probe falsely reports support) or
|
||||
silently keeps an unwanted overlay (when the probe falsely reports
|
||||
no support).
|
||||
"""
|
||||
|
||||
@staticmethod
|
||||
def _fake_run(stdout: str = "", returncode: int = 0):
|
||||
from unittest.mock import MagicMock
|
||||
def _run(*args, **kwargs):
|
||||
proc = MagicMock()
|
||||
proc.stdout = stdout
|
||||
proc.returncode = returncode
|
||||
return proc
|
||||
return _run
|
||||
|
||||
def test_manifest_command_drives_support_probe(self):
|
||||
"""When the manifest returns a distinct command, the support
|
||||
probe runs against the manifest command, not the input
|
||||
``driver_cmd`` parameter.
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
from tools.computer_use.cua_backend import _resolve_mcp_invocation
|
||||
|
||||
manifest = (
|
||||
'{"mcp_invocation":'
|
||||
'{"command":"/opt/relocated/cua-driver","args":["mcp"]}}'
|
||||
)
|
||||
with patch("subprocess.run", new=self._fake_run(stdout=manifest)), \
|
||||
patch.object(cua_backend, "_cua_no_overlay", return_value=True), \
|
||||
patch.object(
|
||||
cua_backend, "_cua_driver_supports_no_overlay",
|
||||
return_value=True,
|
||||
) as mock_probe:
|
||||
cua_backend._cua_driver_supports_no_overlay.cache_clear()
|
||||
cmd, args = _resolve_mcp_invocation("/usr/bin/cua-driver")
|
||||
assert cmd == "/opt/relocated/cua-driver"
|
||||
# The support probe must be called with the manifest-resolved
|
||||
# command, not the input driver_cmd argument.
|
||||
mock_probe.assert_called_with("/opt/relocated/cua-driver")
|
||||
|
||||
def test_fallback_uses_input_driver_cmd_for_support_probe(self):
|
||||
"""When the manifest knows the args but NOT the command, the
|
||||
input ``driver_cmd`` parameter is what gets launched and
|
||||
probed.
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
from tools.computer_use.cua_backend import _resolve_mcp_invocation
|
||||
|
||||
manifest = '{"mcp_invocation":{"args":["mcp"]}}'
|
||||
with patch("subprocess.run", new=self._fake_run(stdout=manifest)), \
|
||||
patch.object(cua_backend, "_cua_no_overlay", return_value=True), \
|
||||
patch.object(
|
||||
cua_backend, "_cua_driver_supports_no_overlay",
|
||||
return_value=True,
|
||||
) as mock_probe:
|
||||
cua_backend._cua_driver_supports_no_overlay.cache_clear()
|
||||
cmd, args = _resolve_mcp_invocation("/my/local/cua-driver")
|
||||
assert cmd == "/my/local/cua-driver"
|
||||
# Fallback path: probe runs against the input driver_cmd.
|
||||
mock_probe.assert_called_with("/my/local/cua-driver")
|
||||
|
||||
def test_probe_distinguishes_support_between_binaries(self):
|
||||
"""Different binaries must produce independent support verdicts.
|
||||
The cache is keyed on ``driver_cmd``; the same cached result
|
||||
must not leak between the system binary and a manifest-relocated
|
||||
one.
|
||||
"""
|
||||
with patch.object(cua_backend, "_cua_no_overlay", return_value=True), \
|
||||
patch.object(
|
||||
cua_backend, "_cua_driver_supports_no_overlay",
|
||||
side_effect=lambda cmd: cmd == "/opt/relocated/cua-driver",
|
||||
):
|
||||
# System binary does NOT support, manifest binary DOES.
|
||||
args = cua_backend._mcp_args_with_overlay_flag(
|
||||
["mcp"], driver_cmd="/usr/bin/cua-driver",
|
||||
)
|
||||
assert "--no-overlay" not in args
|
||||
args = cua_backend._mcp_args_with_overlay_flag(
|
||||
["mcp"], driver_cmd="/opt/relocated/cua-driver",
|
||||
)
|
||||
assert "--no-overlay" in args
|
||||
|
||||
|
||||
class TestMcpArgsOverlayFlag:
|
||||
def test_appended_when_enabled_and_supported(self):
|
||||
|
|
|
|||
|
|
@ -376,8 +376,12 @@ def _resolve_mcp_invocation(
|
|||
if not isinstance(command, str) or not command:
|
||||
# The driver knows the subcommand but didn't surface its own path.
|
||||
# Keep our resolved driver_cmd; the args are still authoritative.
|
||||
return driver_cmd, _mcp_args_with_overlay_flag(args)
|
||||
return command, _mcp_args_with_overlay_flag(args)
|
||||
return driver_cmd, _mcp_args_with_overlay_flag(args, driver_cmd=driver_cmd)
|
||||
# Manifest surfaced a relocated executable — probe THAT binary for
|
||||
# `--no-overlay` support rather than the system-resolved one, so a
|
||||
# wrapper/relocation with a different feature set doesn't crash on
|
||||
# an unknown flag (or silently keep an unwanted overlay).
|
||||
return command, _mcp_args_with_overlay_flag(args, driver_cmd=command)
|
||||
|
||||
|
||||
def _mcp_args_with_overlay_flag(
|
||||
|
|
@ -399,10 +403,15 @@ def _cua_driver_supports_no_overlay(driver_cmd: str) -> bool:
|
|||
would crash the MCP spawn.
|
||||
"""
|
||||
try:
|
||||
# cua-driver is a third-party binary — never hand it provider
|
||||
# API keys via inherited env (same policy as the manifest probe
|
||||
# and MCP spawn; #53503/#55709/#58889 lineage).
|
||||
from tools.environments.local import _sanitize_subprocess_env
|
||||
proc = subprocess.run(
|
||||
[driver_cmd, "--help"],
|
||||
capture_output=True, text=True, timeout=3.0,
|
||||
stdin=subprocess.DEVNULL,
|
||||
env=_sanitize_subprocess_env(cua_driver_child_env()),
|
||||
)
|
||||
help_text = (proc.stdout or "") + (proc.stderr or "")
|
||||
return "--no-overlay" in help_text
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue