mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-29 18:46:59 +00:00
fix(tests): stop the test suite speaking aloud and launching a browser
Running the suite could take over the machine it ran on. Two real effects, both now closed: - **The suite spoke through the speakers.** Once any test drove the `voice.toggle` RPC with `action="tts"`, the handler set `HERMES_VOICE_TTS=1` in the *live process environment*, and the flag outlived that test. Every later test that drove a turn to completion then fed its final response text to `hermes_cli.voice.speak_text` on a background thread - real synthesis, real playback, no API key needed (the default `edge` provider is keyless). A developer heard the fixture string "partial answer complete" out loud. Because the flag is set from inside the process, `scripts/run_tests.sh`'s `env -i` never protected against this. `tests/conftest.py` now blanks `HERMES_VOICE`/`HERMES_VOICE_TTS` per test, and a new autouse `_audio_playback_guard` stubs `speak_text` and its playback binding outright, so the speakers stay shut even inside the test that sets the flag itself. `@pytest.mark.real_audio_playback` opts out. - **The suite launched Chrome.** `tests/tools/test_browser_supervisor.py` spawned a real browser on any machine with Chrome on `PATH`. Its docstring promised a `HERMES_E2E_BROWSER=1` gate that existed nowhere in the code. That gate is now real, and the file is marked `integration` so the default marker filter excludes it. `scripts/run_tests.sh` forwards `HERMES_E2E_BROWSER` so the documented manual run still works. Miscellanea - `tests/test_audio_playback_guard.py`: regression cover for both defences, driving the real `voice.toggle` handler rather than a stand-in.
This commit is contained in:
parent
c911a5f10f
commit
63be8ce863
4 changed files with 258 additions and 7 deletions
|
|
@ -119,6 +119,7 @@ exec env -i \
|
|||
LC_ALL=C.UTF-8 \
|
||||
PYTHONHASHSEED=0 \
|
||||
${HERMES_RUN_SLOW_PET_TESTS:+HERMES_RUN_SLOW_PET_TESTS="$HERMES_RUN_SLOW_PET_TESTS"} \
|
||||
${HERMES_E2E_BROWSER:+HERMES_E2E_BROWSER="$HERMES_E2E_BROWSER"} \
|
||||
${EXTRA_PYTHONPATH:+PYTHONPATH="$EXTRA_PYTHONPATH"} \
|
||||
${EXTRA_PYTEST_PLUGINS:+PYTEST_PLUGINS="$EXTRA_PYTEST_PLUGINS"} \
|
||||
"$PYTHON" "$SCRIPT_DIR/run_tests_parallel.py" "$@"
|
||||
|
|
|
|||
|
|
@ -170,6 +170,16 @@ def _looks_like_credential(name: str) -> bool:
|
|||
# HERMES_* vars that change test behavior by being set. Unset all of these
|
||||
# unconditionally — individual tests that need them set do so explicitly.
|
||||
_HERMES_BEHAVIORAL_VARS = frozenset({
|
||||
# Voice/TTS runtime flags. ``tui_gateway/server.py`` reads these straight
|
||||
# off ``os.environ`` at call time (``_voice_mode_enabled`` /
|
||||
# ``_voice_tts_enabled``) and, on every completed turn, hands the turn's
|
||||
# final response text to ``hermes_cli.voice.speak_text`` — real synthesis,
|
||||
# real playback, out of the developer's speakers. Blank them per-test so a
|
||||
# leak (from the shell, or from an earlier test that drove the
|
||||
# ``voice.toggle`` RPC, which writes ``os.environ`` directly) cannot carry
|
||||
# into the next test. See ``_audio_playback_guard`` for the second layer.
|
||||
"HERMES_VOICE",
|
||||
"HERMES_VOICE_TTS",
|
||||
"HERMES_YOLO_MODE",
|
||||
"HERMES_INTERACTIVE",
|
||||
"HERMES_QUIET",
|
||||
|
|
@ -583,6 +593,51 @@ def _wal_is_usable() -> bool:
|
|||
return False
|
||||
|
||||
|
||||
# ── Audio-playback guard ───────────────────────────────────────────────────
|
||||
#
|
||||
# Same class of incident as the live-system guard above, different primitive:
|
||||
# a test run spoke the string "partial answer complete" out of the developer's
|
||||
# speakers. That string is a test fixture
|
||||
# (``tests/test_tui_gateway_server.py``'s fake ``final_response``), and the
|
||||
# route it took is fully in-process — no leaked shell variable required:
|
||||
#
|
||||
# 1. ``test_voice_toggle_tts_branch_also_carries_record_key`` drives the
|
||||
# ``voice.toggle`` RPC with ``action="tts"``. The handler
|
||||
# (``tui_gateway/server.py``) flips the flag by writing the *real*
|
||||
# process environment: ``os.environ["HERMES_VOICE_TTS"] = "1"``. The
|
||||
# test's ``monkeypatch.delenv(..., raising=False)`` records no undo entry
|
||||
# (pytest only records an undo when the key was present), so the "1"
|
||||
# survives teardown and persists for the rest of the pytest process.
|
||||
# 2. Any later test in that process that drives a turn to completion hits
|
||||
# the TTS dispatch in ``prompt.submit``, which checks
|
||||
# ``_voice_tts_enabled()`` — now true — and fires
|
||||
# ``hermes_cli.voice.speak_text(final_response)`` on a daemon thread.
|
||||
# 3. ``speak_text`` needs no API key to be audible: ``tools/tts_tool.py``
|
||||
# defaults to the ``edge`` provider, which is keyless.
|
||||
#
|
||||
# Because the flag is set from *inside* the process, ``scripts/run_tests.sh``'s
|
||||
# ``env -i`` does not help, and neither does env-blanking on its own — the
|
||||
# hermetic fixture blanks at test setup, and step 1 re-sets it mid-test. So we
|
||||
# also intercept the primitive that does the damage, exactly as the
|
||||
# live-system guard intercepts ``os.kill`` rather than trusting every caller
|
||||
# to mock it:
|
||||
#
|
||||
# • ``hermes_cli.voice.speak_text`` — the synth+playback entry point both
|
||||
# gateway call sites late-import, so patching the module attribute catches
|
||||
# them wherever they import it from.
|
||||
# • ``hermes_cli.voice.play_audio_file`` — the module-level binding
|
||||
# ``speak_text`` actually plays through. Patching the binding inside
|
||||
# ``hermes_cli.voice`` (not ``tools.voice_mode``) keeps the real function
|
||||
# available to the tests that legitimately exercise it with a mocked
|
||||
# audio backend (``tests/tools/test_voice_mode.py``).
|
||||
#
|
||||
# Config cannot re-open this hole: the ``tts:`` section of ``config.yaml``
|
||||
# only selects *which* provider speaks, never *whether* to speak — that gate
|
||||
# is the env var alone.
|
||||
|
||||
_AUDIO_GUARD_BYPASS_MARK = "real_audio_playback"
|
||||
|
||||
|
||||
def pytest_configure(config): # noqa: D401 — pytest hook
|
||||
"""Register markers used by hermetic conftest."""
|
||||
config.addinivalue_line(
|
||||
|
|
@ -597,6 +652,12 @@ def pytest_configure(config): # noqa: D401 — pytest hook
|
|||
"SQLite WAL mode; skipped on builds where Hermes falls back to "
|
||||
"journal_mode=DELETE for the WAL-reset bug.",
|
||||
)
|
||||
config.addinivalue_line(
|
||||
"markers",
|
||||
f"{_AUDIO_GUARD_BYPASS_MARK}: bypass the audio-playback guard (only "
|
||||
"for tests that genuinely need real TTS synthesis and speaker "
|
||||
"playback — there are none in the default suite).",
|
||||
)
|
||||
|
||||
# The pyproject addopts pin ``--timeout-method=signal`` relies on
|
||||
# ``signal.SIGALRM``, which does not exist on Windows — pytest-timeout
|
||||
|
|
@ -977,3 +1038,44 @@ def _live_system_guard(request, monkeypatch):
|
|||
pass
|
||||
|
||||
yield
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _audio_playback_guard(request, monkeypatch):
|
||||
"""Stub TTS synthesis + speaker playback for every test.
|
||||
|
||||
See the block comment above for the incident this closes. Defence in
|
||||
depth behind ``_HERMES_BEHAVIORAL_VARS``: the env blanking stops the flag
|
||||
leaking *between* tests, this stops the speakers ever opening even when a
|
||||
test sets the flag *itself* (which the ``voice.toggle`` RPC handler does,
|
||||
by writing ``os.environ`` directly).
|
||||
|
||||
Deliberately silent rather than raising: unlike a stray ``os.kill``, a
|
||||
stray ``speak_text`` is dispatched on a daemon thread whose exception
|
||||
nobody would ever see, so a hard failure would neither stop the test nor
|
||||
surface. Silence is the whole point. Tests that genuinely want real audio
|
||||
can opt out with ``@pytest.mark.real_audio_playback``.
|
||||
"""
|
||||
if request.node.get_closest_marker(_AUDIO_GUARD_BYPASS_MARK):
|
||||
yield
|
||||
return
|
||||
|
||||
try:
|
||||
import hermes_cli.voice as _voice
|
||||
except Exception:
|
||||
# Optional audio deps missing — nothing importable to speak with.
|
||||
yield
|
||||
return
|
||||
|
||||
def _blocked_speak_text(text, *args, **kwargs):
|
||||
return None
|
||||
|
||||
def _blocked_play_audio_file(path, *args, **kwargs):
|
||||
return False
|
||||
|
||||
if hasattr(_voice, "speak_text"):
|
||||
monkeypatch.setattr(_voice, "speak_text", _blocked_speak_text)
|
||||
if hasattr(_voice, "play_audio_file"):
|
||||
monkeypatch.setattr(_voice, "play_audio_file", _blocked_play_audio_file)
|
||||
|
||||
yield
|
||||
|
|
|
|||
126
tests/test_audio_playback_guard.py
Normal file
126
tests/test_audio_playback_guard.py
Normal file
|
|
@ -0,0 +1,126 @@
|
|||
"""Regression tests for the test-suite audio-playback guard.
|
||||
|
||||
The incident: a plain test run spoke the words "partial answer complete" out
|
||||
of a developer's speakers. That string is a fake ``final_response`` from
|
||||
``tests/test_tui_gateway_server.py``. The route was entirely in-process — no
|
||||
leaked shell variable, so ``scripts/run_tests.sh``'s ``env -i`` was no help:
|
||||
|
||||
1. A test drives the ``voice.toggle`` RPC with ``action="tts"``. The
|
||||
handler flips the flag by writing the real process environment:
|
||||
``os.environ["HERMES_VOICE_TTS"] = "1"``.
|
||||
2. The flag outlives that test (``monkeypatch.delenv`` on an *absent* key
|
||||
records no undo entry), so every later test in the process sees it.
|
||||
3. Any later test that drives a turn to completion hits the TTS dispatch in
|
||||
``prompt.submit``, which calls ``hermes_cli.voice.speak_text`` on a
|
||||
daemon thread with the turn's final response text.
|
||||
4. ``speak_text`` needs no API key to be audible — ``tools/tts_tool.py``
|
||||
defaults to the keyless ``edge`` provider.
|
||||
|
||||
Two independent defences in ``tests/conftest.py``, one test each below:
|
||||
|
||||
* ``_HERMES_BEHAVIORAL_VARS`` now blanks ``HERMES_VOICE`` /
|
||||
``HERMES_VOICE_TTS`` at every test setup, so step 2 cannot cross a test
|
||||
boundary.
|
||||
* ``_audio_playback_guard`` stubs ``speak_text`` outright, so step 3 stays
|
||||
silent even *within* the test that set the flag itself.
|
||||
|
||||
The second is the load-bearing one: env blanking alone cannot stop code under
|
||||
test from re-setting the variable mid-test.
|
||||
"""
|
||||
|
||||
import os
|
||||
import sys
|
||||
import types
|
||||
|
||||
import pytest
|
||||
|
||||
from tui_gateway import server
|
||||
|
||||
|
||||
def test_voice_toggle_still_leaks_the_env_var_but_speech_is_stubbed(monkeypatch):
|
||||
"""The dangerous primitive is neutralised even when the flag IS set.
|
||||
|
||||
This reproduces the incident's first step for real — it drives the same
|
||||
``voice.toggle`` RPC that the original culprit test does, and asserts the
|
||||
handler really does write ``os.environ`` (so the guard is being tested
|
||||
against live behaviour, not a straw man). It then walks the second step:
|
||||
``speak_text`` is called exactly as ``prompt.submit`` calls it, with the
|
||||
exact string the user heard — and must not reach the TTS backend.
|
||||
"""
|
||||
monkeypatch.setattr(server, "_load_cfg", lambda: {"voice": {}})
|
||||
monkeypatch.setitem(
|
||||
sys.modules,
|
||||
"tools.voice_mode",
|
||||
types.SimpleNamespace(
|
||||
check_voice_requirements=lambda: {"available": True, "details": ""}
|
||||
),
|
||||
)
|
||||
monkeypatch.setenv("HERMES_VOICE", "1")
|
||||
|
||||
resp = server.dispatch(
|
||||
{"id": "tts", "method": "voice.toggle", "params": {"action": "tts"}}
|
||||
)
|
||||
|
||||
# The handler mutates the real process environment. This is the leak;
|
||||
# it is upstream behaviour we are guarding against, not asserting away.
|
||||
assert resp["result"]["tts"] is True
|
||||
assert os.environ.get("HERMES_VOICE_TTS") == "1"
|
||||
assert server._voice_tts_enabled() is True
|
||||
|
||||
# Any call into the TTS backend from here on would be real synthesis and
|
||||
# real playback. Stand a recorder in front of it — a *recorder*, not a
|
||||
# raising stub, because ``speak_text`` wraps its whole body in
|
||||
# ``except Exception`` and would swallow an AssertionError, quietly
|
||||
# turning this test green whether or not the guard is doing anything.
|
||||
import tools.tts_tool as tts_tool
|
||||
|
||||
calls = []
|
||||
monkeypatch.setattr(
|
||||
tts_tool,
|
||||
"text_to_speech_tool",
|
||||
lambda *args, **kwargs: calls.append((args, kwargs)),
|
||||
)
|
||||
|
||||
# Called exactly as tui_gateway/server.py's prompt.submit completion path
|
||||
# calls it (a late `from hermes_cli.voice import speak_text`), with the
|
||||
# exact fixture string that came out of the speakers.
|
||||
from hermes_cli.voice import speak_text
|
||||
|
||||
assert speak_text("partial answer complete") is None
|
||||
assert calls == [], (
|
||||
"audio guard breached: speak_text reached the TTS backend — on an "
|
||||
"unguarded run this is real synthesis through the keyless 'edge' "
|
||||
"provider, played through the speakers"
|
||||
)
|
||||
|
||||
|
||||
def test_voice_env_does_not_leak_into_the_next_test():
|
||||
"""Second defence: the flag the previous test set must not have survived.
|
||||
|
||||
Ordering matters — this test only means anything because it runs after the
|
||||
one above, in the same process, which left ``HERMES_VOICE_TTS=1`` set.
|
||||
Before ``HERMES_VOICE``/``HERMES_VOICE_TTS`` were added to
|
||||
``_HERMES_BEHAVIORAL_VARS``, this assertion failed.
|
||||
"""
|
||||
assert "HERMES_VOICE_TTS" not in os.environ
|
||||
assert "HERMES_VOICE" not in os.environ
|
||||
assert server._voice_tts_enabled() is False
|
||||
assert server._voice_mode_enabled() is False
|
||||
|
||||
|
||||
def test_guard_can_be_opted_out_of_explicitly():
|
||||
"""The stub is a guard, not a lobotomy — the real function is reachable."""
|
||||
import hermes_cli.voice as voice
|
||||
|
||||
assert voice.speak_text.__name__ == "_blocked_speak_text"
|
||||
|
||||
|
||||
@pytest.mark.real_audio_playback
|
||||
def test_bypass_marker_restores_the_real_speak_text():
|
||||
"""``@pytest.mark.real_audio_playback`` hands back the real primitive.
|
||||
|
||||
Asserts identity only — it does not call it, which would speak aloud.
|
||||
"""
|
||||
import hermes_cli.voice as voice
|
||||
|
||||
assert voice.speak_text.__name__ == "speak_text"
|
||||
|
|
@ -6,10 +6,24 @@ Exercises the supervisor end-to-end against a real local Chrome
|
|||
works, since mock-CDP unit tests can only prove the happy paths we
|
||||
thought to model.
|
||||
|
||||
Run manually:
|
||||
scripts/run_tests.sh tests/tools/test_browser_supervisor.py
|
||||
These tests spawn a **real Chrome process** on the machine running them.
|
||||
They are therefore opt-in, twice over:
|
||||
|
||||
Automated: skipped in CI unless ``HERMES_E2E_BROWSER=1`` is set.
|
||||
* ``@pytest.mark.integration`` — excluded by the default
|
||||
``addopts = "-m 'not integration'"`` in ``pyproject.toml``, so a bare
|
||||
``pytest`` cannot launch a browser on a developer's desktop by accident.
|
||||
* ``HERMES_E2E_BROWSER=1`` — the env gate this docstring has always claimed.
|
||||
It previously existed only in this prose: nothing read the variable, and
|
||||
the sole real gate was "is a Chrome binary on PATH", which is true on most
|
||||
desktops and on ``ubuntu-latest``. Now it is enforced.
|
||||
|
||||
Run manually:
|
||||
HERMES_E2E_BROWSER=1 scripts/run_tests.sh -m integration \\
|
||||
tests/tools/test_browser_supervisor.py
|
||||
|
||||
(``scripts/run_tests.sh`` runs under ``env -i`` and forwards
|
||||
``HERMES_E2E_BROWSER`` explicitly; ``-m integration`` overrides the default
|
||||
marker filter.)
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
|
@ -17,6 +31,7 @@ from __future__ import annotations
|
|||
import asyncio
|
||||
import base64
|
||||
import json
|
||||
import os
|
||||
import shutil
|
||||
import subprocess
|
||||
import tempfile
|
||||
|
|
@ -25,10 +40,17 @@ import time
|
|||
import pytest
|
||||
|
||||
|
||||
pytestmark = pytest.mark.skipif(
|
||||
not shutil.which("google-chrome") and not shutil.which("chromium"),
|
||||
reason="Chrome/Chromium not installed",
|
||||
)
|
||||
pytestmark = [
|
||||
pytest.mark.integration,
|
||||
pytest.mark.skipif(
|
||||
os.environ.get("HERMES_E2E_BROWSER", "").strip() != "1",
|
||||
reason="real-browser E2E: set HERMES_E2E_BROWSER=1 to opt in",
|
||||
),
|
||||
pytest.mark.skipif(
|
||||
not shutil.which("google-chrome") and not shutil.which("chromium"),
|
||||
reason="Chrome/Chromium not installed",
|
||||
),
|
||||
]
|
||||
|
||||
|
||||
def _find_chrome() -> str:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue