From 63be8ce863e103280adc3fb1430b100f0747c683 Mon Sep 17 00:00:00 2001 From: obelisk-complex <265670482+obelisk-complex@users.noreply.github.com> Date: Sun, 12 Jul 2026 11:49:00 -0600 Subject: [PATCH] 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. --- scripts/run_tests.sh | 1 + tests/conftest.py | 102 ++++++++++++++++++++ tests/test_audio_playback_guard.py | 126 +++++++++++++++++++++++++ tests/tools/test_browser_supervisor.py | 36 +++++-- 4 files changed, 258 insertions(+), 7 deletions(-) create mode 100644 tests/test_audio_playback_guard.py diff --git a/scripts/run_tests.sh b/scripts/run_tests.sh index ff7e5cf470a9..68237a5fd9ca 100755 --- a/scripts/run_tests.sh +++ b/scripts/run_tests.sh @@ -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" "$@" diff --git a/tests/conftest.py b/tests/conftest.py index 2ae674cfa404..c77d87bc6498 100644 --- a/tests/conftest.py +++ b/tests/conftest.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 diff --git a/tests/test_audio_playback_guard.py b/tests/test_audio_playback_guard.py new file mode 100644 index 000000000000..79d5d8d35b06 --- /dev/null +++ b/tests/test_audio_playback_guard.py @@ -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" diff --git a/tests/tools/test_browser_supervisor.py b/tests/tools/test_browser_supervisor.py index 9274145e8142..a9ab1c579f24 100644 --- a/tests/tools/test_browser_supervisor.py +++ b/tests/tools/test_browser_supervisor.py @@ -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: