mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
test: isolate computer-use approval globals between tests
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).
This commit is contained in:
parent
33fe1cc9e5
commit
8c196ed85c
2 changed files with 110 additions and 0 deletions
|
|
@ -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
|
||||
|
|
|
|||
75
tests/tools/test_computer_use_approval_isolation.py
Normal file
75
tests/tools/test_computer_use_approval_isolation.py
Normal file
|
|
@ -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"))
|
||||
Loading…
Add table
Add a link
Reference in a new issue