From 8c196ed85c724ab592d736a286b7c3a67c05ecb6 Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Fri, 24 Jul 2026 07:50:07 +0200 Subject: [PATCH] test: isolate computer-use approval globals between tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tools/computer_use/tool.py keeps the CLI approval flow in module-globals: _approval_callback plus the per-session unlock stores _always_allow / _session_auto_approve. Any test that installs a callback (or drives CLI init far enough that the real one is registered) and does not reset it poisons every later computer-use test in the same process: * a leaked callback that raises — dead UI infra or a stale two-argument signature (the contract is (action, args, summary)) — becomes verdict='deny' in _request_approval, so dispatch tests fail with an empty backend call list; * a leaked callback that blocks (the real CLI one waits on an answer queue) hangs a single-process run forever. Both are order-dependent: tests/tools/test_computer_use.py passes 220/220 in isolation but shows dispatch failures in single-process full-suite runs (and, with a blocking leak, a permanent hang observed via py-spy inside _request_approval -> callback -> queue.get with no timeout). Fix: an autouse teardown-only fixture resets callback + unlock stores after every test; tests that install their own callback keep it for their own duration. Regression pair included: a 'forgetful' test leaves a stale two-arg callback behind, the next test asserts dispatch still routes to the backend — red without the fixture (1 failed), green with it (225 passed together with the whole computer-use file). --- tests/conftest.py | 35 +++++++++ .../test_computer_use_approval_isolation.py | 75 +++++++++++++++++++ 2 files changed, 110 insertions(+) create mode 100644 tests/tools/test_computer_use_approval_isolation.py diff --git a/tests/conftest.py b/tests/conftest.py index aac654ceb56..8b9488df5a5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1088,3 +1088,38 @@ def _audio_playback_guard(request, monkeypatch): monkeypatch.setattr(_voice, "play_audio_file", _blocked_play_audio_file) yield + + +@pytest.fixture(autouse=True) +def _isolate_computer_use_approval_state(): + """Reset computer-use approval globals after every test. + + ``tools.computer_use.tool`` keeps three module-globals for the CLI + approval flow: ``_approval_callback`` (set by the CLI console on init) + plus the per-session unlock stores ``_always_allow`` / + ``_session_auto_approve``. A test that installs a callback — or drives + CLI init far enough that the real one is registered — and does not reset + it poisons every later computer-use test in the same process: + + * a leaked callback that raises (dead UI/queue infra, or a stale + two-argument signature — the real contract is ``(action, args, + summary)``) turns into ``verdict = "deny"`` in ``_request_approval``, + so dispatch tests fail with an empty backend call list; + * a leaked callback that blocks (the real CLI one waits on an answer + queue) hangs the whole single-process run forever — pytest-timeout is + the only thing that can cut it. + + Both symptoms are order-dependent: the affected files pass in isolation + and only fail in full-suite runs. Teardown-only, so tests that install + their own callback keep it for their own duration. + """ + yield + try: + from tools.computer_use import tool as _cu_tool + + _cu_tool.set_approval_callback(None) + with _cu_tool._approval_lock: + _cu_tool._always_allow.clear() + _cu_tool._session_auto_approve.clear() + except Exception: + pass diff --git a/tests/tools/test_computer_use_approval_isolation.py b/tests/tools/test_computer_use_approval_isolation.py new file mode 100644 index 00000000000..06ce41509bb --- /dev/null +++ b/tests/tools/test_computer_use_approval_isolation.py @@ -0,0 +1,75 @@ +"""Regression: leaked approval callbacks must not poison later tests. + +``tools.computer_use.tool._approval_callback`` and the per-session unlock +stores are module-globals. Without the autouse reset fixture in +``tests/conftest.py``, a test that installs a callback and "forgets" it +changes the behavior of every later computer-use test in the process: +a raising callback becomes ``verdict = "deny"`` (dispatch tests see an +empty backend call list), a blocking callback hangs the run. The pair +below simulates the forgetful test and asserts the next test still sees +default-allow behavior. +""" + +import json + + +def _install_backend(cu_tool): + class _RecordingBackend: + def __init__(self): + self.calls = [] + + def start(self): + pass + + def stop(self): + pass + + def is_available(self): + return True + + def click(self, **kw): + self.calls.append(("click", kw)) + from tools.computer_use.backend import ActionResult + + return ActionResult(ok=True, action="click") + + def capture(self, mode="som", app=None): + from tools.computer_use.backend import CaptureResult + + return CaptureResult( + mode=mode, width=1, height=1, png_b64=None, elements=[], + app="X", window_title="", + ) + + backend = _RecordingBackend() + cu_tool.reset_backend_for_tests() + cu_tool._backend = backend + return backend + + +def test_a_forgets_a_poisoned_approval_callback(): + """Simulates the polluter: installs a callback with the LEGACY + two-argument signature and deliberately does not reset it.""" + from tools.computer_use import tool as cu_tool + + def stale_two_arg_callback(action, args): # wrong arity on purpose + return "approve_once" + + cu_tool.set_approval_callback(stale_two_arg_callback) + # no reset — the autouse fixture must clean this up + + +def test_b_still_dispatches_with_default_allow(): + """Without the isolation fixture this fails: the stale callback raises + (arity), ``_request_approval`` converts that into a deny, and the + backend never sees the click.""" + from tools.computer_use import tool as cu_tool + + backend = _install_backend(cu_tool) + result = cu_tool.handle_computer_use({"action": "click", "element": 3}) + call_names = [c[0] for c in backend.calls] + assert "click" in call_names, ( + f"leaked approval callback poisoned this test: {result!r}" + ) + payload = json.loads(result) if isinstance(result, str) else result + assert not (isinstance(payload, dict) and payload.get("error"))