From 3d846897147ba6ec257cbe930c0f8b855960d0e8 Mon Sep 17 00:00:00 2001 From: David Metcalfe <80915+DavidMetcalfe@users.noreply.github.com> Date: Thu, 16 Jul 2026 21:47:23 -0700 Subject: [PATCH] fix(computer_use): address sweeper feedback on --no-overlay subprocess + manifest probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- tests/computer_use/test_cua_no_overlay.py | 109 ++++++++++++++++++++++ tools/computer_use/cua_backend.py | 13 ++- 2 files changed, 120 insertions(+), 2 deletions(-) diff --git a/tests/computer_use/test_cua_no_overlay.py b/tests/computer_use/test_cua_no_overlay.py index 3629aed448d..207b66ce9d2 100644 --- a/tests/computer_use/test_cua_no_overlay.py +++ b/tests/computer_use/test_cua_no_overlay.py @@ -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): diff --git a/tools/computer_use/cua_backend.py b/tools/computer_use/cua_backend.py index 6d2f6b5c6ff..547604e8448 100644 --- a/tools/computer_use/cua_backend.py +++ b/tools/computer_use/cua_backend.py @@ -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