From 1a477697156cd50a3c2fc5d6a10af6ca89f02d06 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Thu, 9 Jul 2026 20:03:11 -0700 Subject: [PATCH] test: deflake CI and dev-machine flaky tests in bulk (11 tests, 10 files) (#61816) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test: deflake CI and dev-machine flaky tests in bulk Fixes ten distinct flake sources found by mining recent CI failures and running the full suite on a dev machine with real user state: CI-observed races: - tests/conftest.py live-system guard: allow signal 0 (pure liveness probe) through _guarded_kill/_guarded_killpg. psutil.pid_exists() probes a just-killed grandchild reparented to init; the subtree check fails for it and the guard RuntimeError'd test_entire_tree_is_sigkilled_not_just_parent intermittently on unrelated PRs. Hermeticity flakes (fail on dev machines with real state, pass on CI): - agent/coding_context.py: _marker_root() now skips the shared temp root (tempfile.gettempdir()) like it skips $HOME — a stray /tmp/package.json flipped every tmp_path test into the coding posture (9 failures in test_coding_context.py). - test_agent_guardrails.py: pin MAX_CONCURRENT_CHILDREN=3 via autouse monkeypatch instead of freezing the user's real config value at import time (import-time vs call-time config mismatch). - test_web_tools_config.py: TestCheckWebApiKey now neutralizes the ddgs package probe and registry providers — the optional ddgs package in a dev venv lit up the fallback backend. - test_credential_pool.py: block claude_code/hermes-oauth credential autodiscovery in the two pool-merge tests that assert exact id lists (a real ~/.claude/.credentials.json seeded an extra entry). - test_modal_sandbox_fixes.py: clear _permanent_approved / _session_approved — the user's real command_allowlist silently approved the guard-escalation commands under test. - test_setup_irc.py: stub prompt_checklist to select only the IRC row; the non-TTY cancel fallback re-ran the real configured platforms' interactive setup_fn, which hit input() under captured stdin. - test_doctor.py: TestGitHubTokenCheck now patches the module-level HERMES_HOME constant (the file's established pattern) instead of only setenv — doctor was running PRAGMA integrity_check against the real multi-GB state.db and blowing the 300s per-file budget. Latent atexit-duplication (same _enter_buffered_busy class as #34217): - test_undo_command.py: drop importlib.reload(tui_gateway.server) in fixture teardown; reload re-registers the module's atexit hooks. - test_session_platform_resolution.py: drop per-test reload of tui_gateway.server; every resolver reads env at call time. * test: sentinel model value in ignore-user-config fallback assertion With HERMES_IGNORE_USER_CONFIG=1, load_cli_config() falls back to the repo-root cli-config.yaml (untracked, gitignored). On a dev machine that file can legitimately set the same popular model the test hardcoded (anthropic/claude-sonnet-4.6), flipping the != assertion locally while CI (no cli-config.yaml) stayed green. Use an impossible sentinel model name instead. --- agent/coding_context.py | 11 +++++++++- tests/agent/test_credential_pool.py | 8 +++++++ tests/conftest.py | 10 +++++++++ tests/hermes_cli/test_doctor.py | 20 ++++++++++++++--- .../test_ignore_user_config_flags.py | 18 ++++++++++----- tests/hermes_cli/test_setup_irc.py | 12 ++++++++++ tests/run_agent/test_agent_guardrails.py | 22 ++++++++++++++++--- tests/tools/test_modal_sandbox_fixes.py | 20 +++++++++++++++++ tests/tools/test_web_tools_config.py | 8 +++++++ .../test_session_platform_resolution.py | 10 ++++++--- tests/tui_gateway/test_undo_command.py | 9 ++++++-- 11 files changed, 130 insertions(+), 18 deletions(-) diff --git a/agent/coding_context.py b/agent/coding_context.py index 00f6d996d47..db38ab3daa8 100644 --- a/agent/coding_context.py +++ b/agent/coding_context.py @@ -56,6 +56,7 @@ import logging import os import re import subprocess +import tempfile from dataclasses import dataclass from pathlib import Path from typing import Any, Optional @@ -412,10 +413,18 @@ def _marker_root(cwd: Path) -> Optional[Path]: """ current = cwd.resolve() home = _home() + # Shared world-writable temp roots are never project roots: a stray + # manifest in /tmp (left by any process) must not flip every session + # whose cwd lives under the temp dir into the coding posture. Same + # reasoning as the $HOME skip below. + try: + temp_root = Path(tempfile.gettempdir()).resolve() + except Exception: + temp_root = None for depth, parent in enumerate([current, *current.parents]): if depth > 6: break - if parent == home: + if parent == home or (temp_root is not None and parent == temp_root): continue for marker in _PROJECT_MARKERS: if (parent / marker).exists(): diff --git a/tests/agent/test_credential_pool.py b/tests/agent/test_credential_pool.py index d9252a7829c..ae28580d858 100644 --- a/tests/agent/test_credential_pool.py +++ b/tests/agent/test_credential_pool.py @@ -3050,6 +3050,11 @@ def test_codex_oauth_nonterminal_refresh_does_not_quarantine(tmp_path, monkeypat def test_persist_preserves_concurrent_disk_only_entry(tmp_path, monkeypatch): """Regression for #19566: stale rotation writes keep concurrent entries.""" monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hermes")) + # Block external-credential autodiscovery: a real ~/.claude/.credentials.json + # on a dev machine would seed an extra claude_code entry and break the + # exact-id assertions below (passes on CI where no such file exists). + monkeypatch.setattr("agent.anthropic_adapter.read_hermes_oauth_credentials", lambda: None) + monkeypatch.setattr("agent.anthropic_adapter.read_claude_code_credentials", lambda: None) _write_auth_store( tmp_path, { @@ -3111,6 +3116,9 @@ def test_persist_preserves_concurrent_disk_only_entry(tmp_path, monkeypatch): def test_remove_index_does_not_resurrect_via_disk_merge(tmp_path, monkeypatch): monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hermes")) + # Block external-credential autodiscovery (see note in the test above). + monkeypatch.setattr("agent.anthropic_adapter.read_hermes_oauth_credentials", lambda: None) + monkeypatch.setattr("agent.anthropic_adapter.read_claude_code_credentials", lambda: None) _write_auth_store( tmp_path, { diff --git a/tests/conftest.py b/tests/conftest.py index 5606300e5dc..bdef8bae944 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -616,6 +616,13 @@ def _live_system_guard(request, monkeypatch): real_kill = _os.kill def _guarded_kill(pid, sig, *args, **kwargs): + # Signal 0 is a pure liveness probe — it cannot terminate anything. + # psutil.pid_exists() uses os.kill(pid, 0) on POSIX, and probing a + # just-killed grandchild that was reparented to init (zombie with a + # foreign parent chain) must not trip the guard. Flaked in CI on + # test_entire_tree_is_sigkilled_not_just_parent. + if int(sig) == 0: + return real_kill(pid, sig, *args, **kwargs) if _is_own_subtree(int(pid)): return real_kill(pid, sig, *args, **kwargs) raise RuntimeError( @@ -641,6 +648,9 @@ def _live_system_guard(request, monkeypatch): own_pgid = _os.getpgrp() def _guarded_killpg(pgid, sig, *args, **kwargs): + # Signal 0 is a pure liveness probe — never destructive. + if int(sig) == 0: + return real_killpg(pgid, sig, *args, **kwargs) if int(pgid) == own_pgid or _is_own_subtree(int(pgid)): return real_killpg(pgid, sig, *args, **kwargs) raise RuntimeError( diff --git a/tests/hermes_cli/test_doctor.py b/tests/hermes_cli/test_doctor.py index b4c961e2619..ba4e3846e8f 100644 --- a/tests/hermes_cli/test_doctor.py +++ b/tests/hermes_cli/test_doctor.py @@ -858,10 +858,24 @@ def test_run_doctor_opencode_go_skips_invalid_models_probe(monkeypatch, tmp_path class TestGitHubTokenCheck: """Tests for GitHub token / gh auth detection in doctor.""" + @staticmethod + def _isolate_home(monkeypatch, home): + """Point doctor at the temp HERMES_HOME. + + ``run_doctor`` reads the module-level ``HERMES_HOME`` constant (cached + at import time), NOT the env var — so ``setenv("HERMES_HOME")`` alone + leaves doctor probing the REAL ~/.hermes. On a dev machine with a + large state.db that meant a multi-minute ``PRAGMA integrity_check`` + that blew the 300s per-file budget and killed the whole file. + """ + monkeypatch.setattr(doctor_mod, "HERMES_HOME", home) + monkeypatch.setattr(doctor_mod, "_DHH", str(home)) + monkeypatch.setenv("HERMES_HOME", str(home)) + def test_no_token_and_not_gh_authenticated_shows_warn(self, monkeypatch, tmp_path): home = tmp_path / ".hermes" home.mkdir(parents=True, exist_ok=True) - monkeypatch.setenv("HERMES_HOME", str(home)) + self._isolate_home(monkeypatch, home) monkeypatch.setenv("PATH", "/nonexistent") # gh not found from hermes_cli.doctor import run_doctor @@ -878,7 +892,7 @@ class TestGitHubTokenCheck: def test_token_env_present_shows_ok(self, monkeypatch, tmp_path): home = tmp_path / ".hermes" home.mkdir(parents=True, exist_ok=True) - monkeypatch.setenv("HERMES_HOME", str(home)) + self._isolate_home(monkeypatch, home) monkeypatch.setenv("GITHUB_TOKEN", "ghp_test123") monkeypatch.setenv("PATH", "/nonexistent") # gh not found @@ -895,7 +909,7 @@ class TestGitHubTokenCheck: def test_gh_authenticated_without_env_token_shows_ok(self, monkeypatch, tmp_path): home = tmp_path / ".hermes" home.mkdir(parents=True, exist_ok=True) - monkeypatch.setenv("HERMES_HOME", str(home)) + self._isolate_home(monkeypatch, home) # No GITHUB_TOKEN or GH_TOKEN monkeypatch.delenv("GITHUB_TOKEN", raising=False) monkeypatch.delenv("GH_TOKEN", raising=False) diff --git a/tests/hermes_cli/test_ignore_user_config_flags.py b/tests/hermes_cli/test_ignore_user_config_flags.py index 60738779321..28aea6a18de 100644 --- a/tests/hermes_cli/test_ignore_user_config_flags.py +++ b/tests/hermes_cli/test_ignore_user_config_flags.py @@ -48,6 +48,12 @@ class TestIgnoreUserConfigEnvGate: """ def _write_user_config(self, tmp_path, model_default): + # NOTE: the model value is a sentinel that can never appear in a real + # config. With HERMES_IGNORE_USER_CONFIG=1, load_cli_config() falls + # back to the repo-root ``cli-config.yaml`` (untracked, gitignored) — + # on a dev machine that file can legitimately set the same popular + # model this test previously used ("anthropic/claude-sonnet-4.6"), + # making the != assertion flip locally while passing on CI. config_yaml = textwrap.dedent( f""" model: @@ -66,13 +72,13 @@ class TestIgnoreUserConfigEnvGate: return cli.load_cli_config def test_user_config_loaded_when_flag_unset(self, tmp_path, monkeypatch): - self._write_user_config(tmp_path, "anthropic/claude-sonnet-4.6") + self._write_user_config(tmp_path, "test-vendor/ignore-user-config-sentinel") load_cli_config = self._reload_cli(monkeypatch, tmp_path) cfg = load_cli_config() # User config value wins - assert cfg["model"]["default"] == "anthropic/claude-sonnet-4.6" + assert cfg["model"]["default"] == "test-vendor/ignore-user-config-sentinel" assert cfg["agent"]["system_prompt"] == "from user config" def test_user_config_skipped_when_flag_set(self, tmp_path, monkeypatch): @@ -81,7 +87,7 @@ class TestIgnoreUserConfigEnvGate: The built-in default ``model.default`` is empty string (no user override), and the user's ``agent.system_prompt`` is not seen. """ - self._write_user_config(tmp_path, "anthropic/claude-sonnet-4.6") + self._write_user_config(tmp_path, "test-vendor/ignore-user-config-sentinel") monkeypatch.setenv("HERMES_IGNORE_USER_CONFIG", "1") load_cli_config = self._reload_cli(monkeypatch, tmp_path) @@ -93,18 +99,18 @@ class TestIgnoreUserConfigEnvGate: # User-set model.default MUST NOT leak through — either the built-in # default ("" or unset) or a project-level fallback, but never the # user's value - assert cfg["model"].get("default", "") != "anthropic/claude-sonnet-4.6" + assert cfg["model"].get("default", "") != "test-vendor/ignore-user-config-sentinel" def test_flag_ignored_when_set_to_other_value(self, tmp_path, monkeypatch): """Only the literal value "1" activates the bypass, matching the yolo pattern.""" - self._write_user_config(tmp_path, "anthropic/claude-sonnet-4.6") + self._write_user_config(tmp_path, "test-vendor/ignore-user-config-sentinel") monkeypatch.setenv("HERMES_IGNORE_USER_CONFIG", "true") # not "1" load_cli_config = self._reload_cli(monkeypatch, tmp_path) cfg = load_cli_config() # "true" != "1", so user config IS loaded - assert cfg["model"]["default"] == "anthropic/claude-sonnet-4.6" + assert cfg["model"]["default"] == "test-vendor/ignore-user-config-sentinel" class TestIgnoreRulesEnvGate: diff --git a/tests/hermes_cli/test_setup_irc.py b/tests/hermes_cli/test_setup_irc.py index 31b263fec35..e7418507655 100644 --- a/tests/hermes_cli/test_setup_irc.py +++ b/tests/hermes_cli/test_setup_irc.py @@ -231,6 +231,18 @@ class TestIRCGatewaySetupFreshInstall: monkeypatch.setattr(setup_mod, "prompt_yes_no", lambda *a, **kw: False) monkeypatch.setattr(setup_mod, "prompt_choice", lambda *a, **kw: 0) + # Select ONLY the IRC row. Without this, the non-TTY checklist + # falls back to its cancel value (the pre-selected "configured" + # platforms) — on a dev machine with real platforms configured + # that runs their interactive setup_fn, which calls input() and + # dies under captured stdin. IRC's setup_fn is a no-op lambda. + monkeypatch.setattr( + setup_mod, + "prompt_checklist", + lambda title, items, pre=None: [ + i for i, item in enumerate(items) if "IRC" in item + ], + ) monkeypatch.setattr(gateway_mod, "supports_systemd_services", lambda: False) monkeypatch.setattr(gateway_mod, "is_macos", lambda: False) monkeypatch.setattr(gateway_mod, "_is_service_installed", lambda: False) diff --git a/tests/run_agent/test_agent_guardrails.py b/tests/run_agent/test_agent_guardrails.py index eb89cdda9c0..bcec4e3d7c8 100644 --- a/tests/run_agent/test_agent_guardrails.py +++ b/tests/run_agent/test_agent_guardrails.py @@ -8,10 +8,26 @@ Covers three static methods on AIAgent (inspired by PR #1321 — @alireza78a): import types -from run_agent import AIAgent -from tools.delegate_tool import _get_max_concurrent_children +import pytest -MAX_CONCURRENT_CHILDREN = _get_max_concurrent_children() +from run_agent import AIAgent + +# Pin the concurrency limit instead of reading the runtime config. +# _cap_delegate_task_calls() resolves _get_max_concurrent_children() at CALL +# time (inside a per-test hermetic HERMES_HOME), but this module previously +# froze the value at IMPORT time — before the hermetic fixture ran — so a +# developer machine with delegation.max_concurrent_children in the real +# ~/.hermes/config.yaml saw a different limit at import vs call and the +# truncation tests failed locally while passing on CI. +MAX_CONCURRENT_CHILDREN = 3 + + +@pytest.fixture(autouse=True) +def _pin_max_concurrent_children(monkeypatch): + monkeypatch.setattr( + "tools.delegate_tool._get_max_concurrent_children", + lambda: MAX_CONCURRENT_CHILDREN, + ) # --------------------------------------------------------------------------- diff --git a/tests/tools/test_modal_sandbox_fixes.py b/tests/tools/test_modal_sandbox_fixes.py index 3bccb3e07d1..dddfe134edb 100644 --- a/tests/tools/test_modal_sandbox_fixes.py +++ b/tests/tools/test_modal_sandbox_fixes.py @@ -350,6 +350,7 @@ class TestDockerHostBindApproval: def test_isolated_docker_keeps_fast_path(self, monkeypatch): """Isolated Docker still bypasses dangerous-command approval.""" import tools.approval as A + self._isolate_approval_state(monkeypatch) monkeypatch.setenv("HERMES_EXEC_ASK", "1") monkeypatch.setattr( "tools.tirith_security.check_command_security", @@ -358,9 +359,26 @@ class TestDockerHostBindApproval: has_host_access=False) assert res["approved"] is True + @staticmethod + def _isolate_approval_state(monkeypatch): + """Clear approval state that leaks in from the real user config. + + ``tools.approval`` loads ``command_allowlist`` into module-level + ``_permanent_approved`` at import time. This file imports + ``tools.terminal_tool`` at module level (collection time — BEFORE the + hermetic HERMES_HOME fixture runs), so on a dev machine whose real + config permanently allowlists e.g. "delete in root path" the guard + under test silently approves and the assertions flip. CI never has + such an allowlist, making this a local-only flake. + """ + import tools.approval as A + monkeypatch.setattr(A, "_permanent_approved", set()) + monkeypatch.setattr(A, "_session_approved", {}) + def test_host_bound_docker_requires_approval(self, monkeypatch): """Host-bound Docker dangerous command escalates instead of bypassing.""" import tools.approval as A + self._isolate_approval_state(monkeypatch) monkeypatch.setenv("HERMES_EXEC_ASK", "1") monkeypatch.setattr( "tools.tirith_security.check_command_security", @@ -374,6 +392,7 @@ class TestDockerHostBindApproval: def test_execute_code_isolated_docker_keeps_fast_path(self, monkeypatch): """Isolated Docker execute_code still bypasses the guard.""" import tools.approval as A + self._isolate_approval_state(monkeypatch) monkeypatch.setenv("HERMES_EXEC_ASK", "1") res = A.check_execute_code_guard("import os", "docker", has_host_access=False) @@ -382,6 +401,7 @@ class TestDockerHostBindApproval: def test_execute_code_host_bound_docker_requires_approval(self, monkeypatch): """Host-bound Docker execute_code does not get the container fast-path.""" import tools.approval as A + self._isolate_approval_state(monkeypatch) monkeypatch.setenv("HERMES_EXEC_ASK", "1") res = A.check_execute_code_guard( "import os; os.system('rm -rf /workspace')", "docker", diff --git a/tests/tools/test_web_tools_config.py b/tests/tools/test_web_tools_config.py index 535f930e080..5fb88807f54 100644 --- a/tests/tools/test_web_tools_config.py +++ b/tests/tools/test_web_tools_config.py @@ -548,6 +548,14 @@ class TestCheckWebApiKey: self._managed_patchers = [ patch("tools.web_tools.managed_nous_tools_enabled", return_value=True), patch("tools.managed_tool_gateway.managed_nous_tools_enabled", return_value=True), + # ddgs availability is package-presence driven and the plugin + # registry can hold an available ddgs provider. Neutralize both + # fallback surfaces so this class only exercises env-key/gateway + # resolution — otherwise these tests flip on machines where the + # optional ``ddgs`` package is installed (dev venvs) vs CI. + patch("tools.web_tools._ddgs_package_importable", return_value=False), + patch("agent.web_search_registry.get_active_search_provider", return_value=None), + patch("agent.web_search_registry.get_active_extract_provider", return_value=None), ] for p in self._managed_patchers: p.start() diff --git a/tests/tui_gateway/test_session_platform_resolution.py b/tests/tui_gateway/test_session_platform_resolution.py index da241530604..dd2b6eda85c 100644 --- a/tests/tui_gateway/test_session_platform_resolution.py +++ b/tests/tui_gateway/test_session_platform_resolution.py @@ -17,14 +17,18 @@ The resolver helper is import-safe (no heavy module side effects) so it can be unit-tested without spinning up the full gateway. """ -import importlib - import pytest def _reload_resolver(): + # Plain import — every resolver under test reads the env at CALL time, so + # no reload is needed. importlib.reload(tui_gateway.server) would + # re-register the module's atexit hooks (thread-pool shutdown + + # _shutdown_sessions) on every test; duplicated hooks race the stderr + # buffer at interpreter shutdown (Fatal Python error: + # _enter_buffered_busy) — same flake class as PR #34217. Name kept for + # the existing call sites. import tui_gateway.server as _srv - importlib.reload(_srv) return _srv diff --git a/tests/tui_gateway/test_undo_command.py b/tests/tui_gateway/test_undo_command.py index fd1dbca5905..cd0f7e5c4e0 100644 --- a/tests/tui_gateway/test_undo_command.py +++ b/tests/tui_gateway/test_undo_command.py @@ -43,11 +43,16 @@ def server(hermes_home): ): mod = importlib.import_module("tui_gateway.server") yield mod + # Reset module-level session state without re-importing. importlib.reload + # would re-register the module's atexit hooks; duplicated hooks race the + # stderr buffer at interpreter shutdown (Fatal Python error: + # _enter_buffered_busy) — same class as PR #34217. mod._sessions.clear() mod._pending.clear() mod._answers.clear() - mod._methods.clear() - importlib.reload(mod) + # NOTE: _methods is intentionally NOT cleared — it's populated at import + # time and would only repopulate via reload. + mod._db = None @pytest.fixture()