From 76e08ebea517c15e393912504a924026b7923255 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 23 Sep 2026 08:35:24 -0700 Subject: [PATCH] test: restored tests exercise their claim instead of passing vacuously Independent review of #120220 found restored tests that could pass without reaching the code they name. Each fix below was A/B'd against a sabotaged production line (red) and the real one (green). - test_clarify_progress_leak: the fake agent slept 2x0.35 s as its only sync. It now waits (<= 5 s) for the progress task's first bubble; the queue is FIFO, so a leaked clarify bubble is what lands first. Reverting the clarify skip in run_turn_runner goes red on the leaked JSON. - test_desktop_lifecycle_windows_live holder-scan case: the kanban row was rebuilt after the serve kill, re-reading the dead pid; NoSuchProcess was swallowed into an empty scan and returned False without reaching the classifier. Rows are now snapshotted while both processes live; a classifier that accepts everything goes red (the old version stayed green). - *_windows_live (control socket, desktop lifecycle, plan reconciliation, shim fail-closed): fixed time.sleep settles become bounded waits on the real condition (argv visible to psutil, pipe gone, handle released) and child readline() calls get a 60 s bound. - test_gateway_proc_fallback BSD ps case: drop the is_windows stub and mark it linux_only like its neighbour; a revert to `ps -A eww` goes red. - test_delivery_ledger_single_connection counts only connections to the ledger's own database, not every sqlite3.connect in the process; pruning on a second connection goes red. - test_codex_models: the hardcoded gpt-5.6 list becomes a relationship to DEFAULT_CODEX_MODELS and the forward-compat table; adding a -pro slug to either goes red. - group-chat-view inline-code: the renderer stub wrapped the code in .aui-md, which the generic inline-code rule themes on its own, so the room's data-slot rule was never tested. Without the wrapper, renaming the room's data-slot goes red. --- .../group-chat-view.inline-code.test.tsx | 22 ++++---- tests/gateway/test_clarify_progress_leak.py | 29 +++++++++-- .../test_control_socket_windows_live.py | 43 +++++++++++++--- .../test_delivery_ledger_single_connection.py | 10 +++- tests/hermes_cli/test_codex_models.py | 33 +++++++----- .../test_desktop_lifecycle_windows_live.py | 51 +++++++++++++------ .../hermes_cli/test_gateway_proc_fallback.py | 4 +- .../test_plan_reconciliation_windows_live.py | 24 +++++++-- .../test_shim_fail_closed_windows_live.py | 40 +++++++++++++-- 9 files changed, 193 insertions(+), 63 deletions(-) diff --git a/apps/desktop/src/plugins/hermes-bots/group-chat-view.inline-code.test.tsx b/apps/desktop/src/plugins/hermes-bots/group-chat-view.inline-code.test.tsx index b9dc2e124c..6e58b8e197 100644 --- a/apps/desktop/src/plugins/hermes-bots/group-chat-view.inline-code.test.tsx +++ b/apps/desktop/src/plugins/hermes-bots/group-chat-view.inline-code.test.tsx @@ -8,14 +8,12 @@ import { afterEach, beforeAll, expect, it, vi } from 'vitest' import { translateBots } from './i18n-test-helper' -// Room bodies render through the shell's message renderer, whose markdown -// root is `aui-md prose` — the same container the 1:1 chat uses, minus the -// `[data-slot='aui_assistant-message-content']` ancestor that scopes the -// themed inline-code rule in styles.css. Without a room-owned hook the room's -// `` falls through to Tailwind Typography's fixed near-black ink, which -// is invisible on every dark theme (#114086). The stubs below emit the two -// shapes the room can produce (renderer path, raw Streamdown fallback) so the -// real stylesheet's cascade decides, not a regex over the source text. +// Without a room-owned hook the room's `` falls through to Tailwind +// Typography's fixed near-black ink, which is invisible on every dark theme +// (#114086). The renderer stub emits a bare `` with NO `.aui-md` +// wrapper, so only the room's own `[data-slot='group-chat-message-content']` +// rule in the real stylesheet can theme it: the cascade decides, not a regex +// over the source text. vi.mock('@hermes/plugin-sdk', async () => { const { pluginSdkMock, createGroupGateway } = await import('./group-test-utils') const base = await pluginSdkMock(createGroupGateway().host) @@ -43,11 +41,9 @@ vi.mock('@hermes/plugin-sdk', async () => { DialogTitle: () => null, Input: () => null, MessageTextContent: ({ text }: { text: string }) => ( -
-

- set {text} first -

-
+

+ set {text} first +

), Tip: ({ children }: { children: ReactNode }) => children, relativeTime: () => 'now', diff --git a/tests/gateway/test_clarify_progress_leak.py b/tests/gateway/test_clarify_progress_leak.py index da057c24f6..9763467c38 100644 --- a/tests/gateway/test_clarify_progress_leak.py +++ b/tests/gateway/test_clarify_progress_leak.py @@ -12,7 +12,7 @@ the rendered interactive prompt on Slack. import importlib import sys -import time +import threading import types import pytest @@ -29,6 +29,13 @@ class ProgressCaptureAdapter(BasePlatformAdapter): super().__init__(PlatformConfig(enabled=True, token="***"), platform) self.sent = [] self.edits = [] + # Set on the first send/edit. The fake agent (on an executor thread) + # waits on it, so the turn cannot end before the progress task has + # rendered its first bubble. + self.delivered = threading.Event() + + def _record(self, content): + self.delivered.set() async def connect(self, *, is_reconnect: bool = False) -> bool: return True @@ -38,10 +45,12 @@ class ProgressCaptureAdapter(BasePlatformAdapter): async def send(self, chat_id, content, reply_to=None, metadata=None) -> SendResult: self.sent.append({"chat_id": chat_id, "content": content}) + self._record(content) return SendResult(success=True, message_id="m-1") async def edit_message(self, chat_id, message_id, content) -> SendResult: self.edits.append({"chat_id": chat_id, "message_id": message_id, "content": content}) + self._record(content) return SendResult(success=True, message_id=message_id) async def send_typing(self, chat_id, metadata=None) -> None: @@ -55,7 +64,15 @@ class ProgressCaptureAdapter(BasePlatformAdapter): class ClarifyThenToolAgent: - """Emits a clarify tool.started (with raw args) then a normal tool.""" + """Emits a clarify tool.started (with raw args) then a normal tool, and + returns only once the progress task has delivered its first bubble. + + Both events are queued before that wait, and the queue drains FIFO, so the + first bubble is the clarify one if clarify leaks and the terminal one if + not. A timeout means nothing drained and fails the test. + """ + + adapter = None def __init__(self, **kwargs): self.tool_progress_callback = kwargs.get("tool_progress_callback") @@ -70,9 +87,9 @@ class ClarifyThenToolAgent: "Which environment?", {"question": "Which environment?", "choices": ["staging", "production"]}, ) - time.sleep(0.35) cb("tool.started", "terminal", "pwd", {}) - time.sleep(0.35) + if not type(self).adapter.delivered.wait(timeout=5.0): + raise AssertionError("progress task never delivered a bubble") return {"final_response": "done", "messages": [], "api_calls": 1} @@ -127,6 +144,7 @@ async def test_clarify_tool_never_renders_progress_bubble(monkeypatch, tmp_path, """ adapter = ProgressCaptureAdapter() runner = _make_runner(adapter) + monkeypatch.setattr(ClarifyThenToolAgent, "adapter", adapter) gateway_run = _install_fakes(monkeypatch, mode) monkeypatch.setattr(gateway_run, "_hermes_home", tmp_path) @@ -152,5 +170,6 @@ async def test_clarify_tool_never_renders_progress_bubble(monkeypatch, tmp_path, # No clarify progress line at all (verb "Asking" / tool name). assert "clarify" not in all_content assert "Asking" not in all_content - # The unrelated terminal tool still renders progress normally. + # The unrelated terminal tool still renders progress normally (and proves + # the no-leak asserts above ran against a drained queue). assert "pwd" in all_content diff --git a/tests/gateway/test_control_socket_windows_live.py b/tests/gateway/test_control_socket_windows_live.py index 723a98fef5..42c3c524e9 100644 --- a/tests/gateway/test_control_socket_windows_live.py +++ b/tests/gateway/test_control_socket_windows_live.py @@ -21,8 +21,10 @@ from __future__ import annotations import json import os +import queue import subprocess import sys +import threading import time from pathlib import Path @@ -32,6 +34,35 @@ pytestmark = pytest.mark.windows_only PROJECT_ROOT = Path(__file__).resolve().parents[2] + +def _wait_until(predicate, timeout: float = 15.0, interval: float = 0.05) -> bool: + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + if predicate(): + return True + time.sleep(interval) + return bool(predicate()) + + +def _readline(stream, timeout: float = 60.0) -> str: + """``stream.readline()`` bounded by *timeout* (a hung child fails, not hangs).""" + got: queue.Queue = queue.Queue() + threading.Thread(target=lambda: got.put(stream.readline()), daemon=True).start() + try: + return got.get(timeout=timeout) + except queue.Empty: + return "" + + +def _argv_visible(pid: int, marker: str) -> bool: + """True once the process table shows *pid* with *marker* in its argv.""" + import psutil + + try: + return marker in " ".join(psutil.Process(pid).cmdline()) + except (psutil.NoSuchProcess, psutil.AccessDenied): + return False + _CHILD_CODE = r""" import asyncio, os, sys sys.path.insert(0, sys.argv[1]) @@ -65,10 +96,10 @@ def live_server(tmp_path: Path): text=True, cwd=str(PROJECT_ROOT), ) - line = proc.stdout.readline().strip() + line = _readline(proc.stdout).strip() if not line.startswith("SERVER_STARTED"): err = proc.stderr.read() if proc.poll() is not None else "" - proc.kill() + _kill_tree(proc) pytest.fail(f"pipe server child failed to start: {line!r} {err}") server_pid = int(line.split()[1]) yield proc, home, server_pid @@ -126,9 +157,10 @@ def test_pipe_gone_after_kill_falls_back(live_server, monkeypatch): assert identify_gateway(home, timeout=5.0) is not None _kill_tree(proc) - time.sleep(0.5) - assert identify_gateway(home, timeout=2.0) is None + # taskkill /T returns once the tree is signalled; the pipe disappears when + # the kernel tears the server's handles down. + assert _wait_until(lambda: identify_gateway(home, timeout=0.5) is None) # Consumer falls back to the state file. That file is a claim, not an # identity: its sha classifies a row only when live_gateway_pid_for_home @@ -163,8 +195,7 @@ def test_pipe_gone_after_kill_falls_back(live_server, monkeypatch): stderr=subprocess.DEVNULL, ) try: - time.sleep(0.5) - assert standin.poll() is None + assert _wait_until(lambda: _argv_visible(standin.pid, "gateway")), "stand-in argv never visible" _write_state(standin.pid) fleet = ur.collect_fleet_versions() assert len(fleet) == 1, fleet diff --git a/tests/gateway/test_delivery_ledger_single_connection.py b/tests/gateway/test_delivery_ledger_single_connection.py index 450132dd90..049861e71f 100644 --- a/tests/gateway/test_delivery_ledger_single_connection.py +++ b/tests/gateway/test_delivery_ledger_single_connection.py @@ -7,17 +7,23 @@ second connection opened after the first one closed. from __future__ import annotations import sqlite3 +from pathlib import Path from gateway import delivery_ledger as dl def test_recording_a_reply_does_not_open_a_second_connection(tmp_path, monkeypatch): - monkeypatch.setattr(dl, "_db_path", lambda: tmp_path / "state.db") + db = tmp_path / "state.db" + monkeypatch.setattr(dl, "_db_path", lambda: db) real_connect = sqlite3.connect opened: list[str] = [] def counting_connect(*args, **kwargs): - opened.append(str(args[0]) if args else str(kwargs.get("database"))) + target = args[0] if args else kwargs.get("database") + # Only the ledger's own database counts: unrelated sqlite users in the + # process (other modules, background threads) must not move the tally. + if Path(str(target)).resolve() == db.resolve(): + opened.append(str(target)) return real_connect(*args, **kwargs) monkeypatch.setattr(sqlite3, "connect", counting_connect) diff --git a/tests/hermes_cli/test_codex_models.py b/tests/hermes_cli/test_codex_models.py index efd6c48f0a..e73ad6c636 100644 --- a/tests/hermes_cli/test_codex_models.py +++ b/tests/hermes_cli/test_codex_models.py @@ -2,29 +2,34 @@ import json from unittest.mock import patch from hermes_cli.codex_models import ( + _FORWARD_COMPAT_TEMPLATE_MODELS, + DEFAULT_CODEX_MODELS, get_codex_model_ids, ) -CHATGPT_REJECTED_CODEX_PRO_SLUGS = { - "gpt-5.6-sol-pro", - "gpt-5.6-terra-pro", - "gpt-5.6-luna-pro", -} +def _pro_slugs(model_ids): + return [m for m in model_ids if m.removesuffix("-900k").endswith("-pro")] -def test_curated_codex_fallback_excludes_chatgpt_rejected_pro_slugs(monkeypatch): - """OAuth fallback retains real models but never synthesizes rejected ones.""" - retained_models = {"gpt-5.6-sol", "gpt-5.6-terra", "gpt-5.6-luna"} +def test_codex_catalog_never_offers_chatgpt_rejected_pro_slugs(monkeypatch, tmp_path): + """The ChatGPT Codex OAuth backend 400s every ``-pro`` slug (#52492), so + neither the offline fallback nor forward-compat synthesis over a live + catalog may offer one, while the fallback still keeps every curated model.""" + monkeypatch.setenv("CODEX_HOME", str(tmp_path)) # no config.toml default, no cache + offline = get_codex_model_ids() + assert set(DEFAULT_CODEX_MODELS) <= set(offline) + assert _pro_slugs(offline) == [] + # Live discovery returning only template slugs fires every forward-compat + # synthesis rule; none of what it adds may be -pro. + templates = list(dict.fromkeys(t for _, ts in _FORWARD_COMPAT_TEMPLATE_MODELS for t in ts)) monkeypatch.setattr( - "hermes_cli.codex_models._fetch_models_from_api", - lambda access_token: ["gpt-5.5"], + "hermes_cli.codex_models._fetch_models_from_api", lambda access_token: templates ) - model_ids = get_codex_model_ids(access_token="codex-access-token") - - assert retained_models.issubset(model_ids) - assert CHATGPT_REJECTED_CODEX_PRO_SLUGS.isdisjoint(model_ids) + live = get_codex_model_ids(access_token="codex-access-token") + assert {synthetic for synthetic, _ in _FORWARD_COMPAT_TEMPLATE_MODELS} <= set(live) + assert _pro_slugs(live) == [] diff --git a/tests/hermes_cli/test_desktop_lifecycle_windows_live.py b/tests/hermes_cli/test_desktop_lifecycle_windows_live.py index 6930032ac6..c77a2b1b2b 100644 --- a/tests/hermes_cli/test_desktop_lifecycle_windows_live.py +++ b/tests/hermes_cli/test_desktop_lifecycle_windows_live.py @@ -31,6 +31,25 @@ pytestmark = pytest.mark.windows_only PROJECT_ROOT = Path(__file__).resolve().parents[2] +def _wait_until(predicate, timeout: float = 15.0, interval: float = 0.05) -> bool: + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + if predicate(): + return True + time.sleep(interval) + return bool(predicate()) + + +def _argv_visible(pid: int, marker: str) -> bool: + """True once the process table shows *pid* with *marker* in its argv.""" + import psutil + + try: + return marker in " ".join(psutil.Process(pid).cmdline()) + except (psutil.NoSuchProcess, psutil.AccessDenied): + return False + + @pytest.fixture() def sleeper(): procs: list[subprocess.Popen] = [] @@ -42,8 +61,7 @@ def sleeper(): stderr=subprocess.DEVNULL, ) procs.append(p) - time.sleep(0.5) - assert p.poll() is None + assert _wait_until(lambda: _argv_visible(p.pid, "time.sleep(120)")), "sleeper argv never visible" return p yield _spawn @@ -98,8 +116,7 @@ def test_live_supervised_serve_suppresses_cold_start(sleeper, monkeypatch, tmp_p # Dead serve → ownership drops → plan returns. serve.kill() serve.wait() - time.sleep(0.5) - assert update_cmd._desktop_owns_gateway_lifecycle() is False + assert _wait_until(lambda: update_cmd._desktop_owns_gateway_lifecycle() is False) token = update_cmd._pause_windows_gateways_for_update() assert token is not None and token.get("cold_start_if_installed") is True @@ -116,27 +133,29 @@ def test_holder_scan_fallback_respects_token_classifier(sleeper, monkeypatch, tm # Lookalike from the #90778 class — must NOT confer ownership. kanban_like = sleeper("-m", "hermes_cli.main", "kanban", "--preserve-cache") - def fake_holders(): - import psutil + import psutil - out = [] - for p in (serve_like, kanban_like): - proc = psutil.Process(p.pid) - out.append((p.pid, proc.name(), " ".join(proc.cmdline()))) - return out + # Snapshot both holder rows while both processes are alive. Building the + # kanban row later (after the serve kill) would re-read the dead serve pid, + # raise NoSuchProcess and hand the fallback an empty scan, which returns + # False without ever reaching the token classifier. + serve_row, kanban_row = ( + (p.pid, psutil.Process(p.pid).name(), " ".join(psutil.Process(p.pid).cmdline())) + for p in (serve_like, kanban_like) + ) + assert "--preserve-cache" in kanban_row[2] monkeypatch.setattr( - "hermes_cli.main._detect_venv_python_processes", fake_holders + "hermes_cli.main._detect_venv_python_processes", lambda: [serve_row, kanban_row] ) - # serve-shaped holder with a live parent (us) → owns assert update_cmd._desktop_owns_gateway_lifecycle() is True - # Only the kanban lookalike left → classifier rejects → does not own + # Only the (still live) kanban lookalike left → classifier rejects → does not own serve_like.kill() serve_like.wait() + assert kanban_like.poll() is None monkeypatch.setattr( - "hermes_cli.main._detect_venv_python_processes", - lambda: [fake_holders()[1]], + "hermes_cli.main._detect_venv_python_processes", lambda: [kanban_row] ) assert update_cmd._desktop_owns_gateway_lifecycle() is False diff --git a/tests/hermes_cli/test_gateway_proc_fallback.py b/tests/hermes_cli/test_gateway_proc_fallback.py index 6a70da35f1..8ce5f93255 100644 --- a/tests/hermes_cli/test_gateway_proc_fallback.py +++ b/tests/hermes_cli/test_gateway_proc_fallback.py @@ -114,16 +114,18 @@ class TestProcFallback: mock_ps.assert_not_called() # /proc dir existed, so ps not called +@pytest.mark.linux_only class TestPsFallbackBsdCompat: """The ps fallback must use flags BSD/macOS ps accepts (#73626, #74075). ``ps -A eww`` fails on macOS (BSD ``e`` is not the procps flag), which made gateway discovery silently return nothing whenever /proc is absent. + Linux-only like ``TestProcFallback``: the real host selects the POSIX arm + and only /proc's absence is faked, to force the ps rung. """ def test_ps_fallback_uses_bsd_compatible_flags_and_columns(self): with ( - patch("hermes_cli.gateway.is_windows", return_value=False), patch("os.path.isdir", side_effect=lambda p: p != "/proc"), patch("hermes_cli.gateway._get_ancestor_pids", return_value=set()), patch("subprocess.run") as mock_run, diff --git a/tests/hermes_cli/test_plan_reconciliation_windows_live.py b/tests/hermes_cli/test_plan_reconciliation_windows_live.py index 3f6e8825ca..dce4b7e321 100644 --- a/tests/hermes_cli/test_plan_reconciliation_windows_live.py +++ b/tests/hermes_cli/test_plan_reconciliation_windows_live.py @@ -25,6 +25,25 @@ import pytest pytestmark = [pytest.mark.windows_only, pytest.mark.spawns_gateway_lookalike] +def _wait_until(predicate, timeout: float = 15.0, interval: float = 0.05) -> bool: + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + if predicate(): + return True + time.sleep(interval) + return bool(predicate()) + + +def _argv_visible(pid: int, marker: str) -> bool: + """True once the process table shows *pid* with *marker* in its argv.""" + import psutil + + try: + return marker in " ".join(psutil.Process(pid).cmdline()) + except (psutil.NoSuchProcess, psutil.AccessDenied): + return False + + def test_plan_reconciliation_live_windows(tmp_path, monkeypatch): home = tmp_path / ".hermes" home.mkdir() @@ -43,9 +62,8 @@ def test_plan_reconciliation_live_windows(tmp_path, monkeypatch): stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, ) try: - time.sleep(0.5) - assert child.poll() is None - assert foreign.poll() is None + assert _wait_until(lambda: _argv_visible(child.pid, "gateway")), "stand-in argv never visible" + assert _wait_until(lambda: _argv_visible(foreign.pid, "time.sleep(120)")), "sleeper never visible" import psutil diff --git a/tests/hermes_cli/test_shim_fail_closed_windows_live.py b/tests/hermes_cli/test_shim_fail_closed_windows_live.py index 8bff93bb86..982ccc45ee 100644 --- a/tests/hermes_cli/test_shim_fail_closed_windows_live.py +++ b/tests/hermes_cli/test_shim_fail_closed_windows_live.py @@ -13,8 +13,10 @@ the real rename attempt hitting the real sharing violation. from __future__ import annotations import os +import queue import subprocess import sys +import threading import time from pathlib import Path @@ -25,6 +27,36 @@ pytestmark = pytest.mark.windows_only PROJECT_ROOT = Path(__file__).resolve().parents[2] + +def _wait_until(predicate, timeout: float = 15.0, interval: float = 0.05) -> bool: + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + if predicate(): + return True + time.sleep(interval) + return bool(predicate()) + + +def _readline(stream, timeout: float = 60.0) -> str: + """``stream.readline()`` bounded by *timeout* (a hung child fails, not hangs).""" + got: queue.Queue = queue.Queue() + threading.Thread(target=lambda: got.put(stream.readline()), daemon=True).start() + try: + return got.get(timeout=timeout) + except queue.Empty: + return "" + + +def _rename_round_trips(path: Path) -> bool: + """True once no handle blocks a rename of *path* (the holder is gone).""" + probe = path.with_name(path.name + ".probe") + try: + os.rename(path, probe) + except OSError: + return False + os.rename(probe, path) + return True + # Child that opens a file with GENERIC_READ and NO FILE_SHARE_DELETE — # the exact sharing mode a running .exe image / desktop backend exhibits. _HOLDER_CODE = r""" @@ -55,7 +87,7 @@ def held_shim(tmp_path: Path): stdout=subprocess.PIPE, text=True, ) - line = holder.stdout.readline().strip() + line = _readline(holder.stdout).strip() if line != "HOLDING": holder.kill() pytest.fail(f"lock-holder child failed: {line!r}") @@ -129,10 +161,12 @@ def test_release_then_strict_quarantine_succeeds(tmp_path, monkeypatch): stdout=subprocess.PIPE, text=True, ) - assert holder.stdout.readline().strip() == "HOLDING" + assert _readline(holder.stdout).strip() == "HOLDING" holder.kill() holder.wait() - time.sleep(0.3) # handle teardown + # The handle is released when the kernel tears the (possibly trampolined) + # holder down, not when wait() returns. + assert _wait_until(lambda: _rename_round_trips(scripts / "hermes.exe")), "holder handle never released" install_ran: list = [] monkeypatch.setattr(