mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-15 14:22:43 +00:00
test: deflake CI and dev-machine flaky tests in bulk (11 tests, 10 files) (#61816)
* 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.
This commit is contained in:
Teknium
GitHub
parent
3a394210ff
commit
1a47769715
11 changed files with 130 additions and 18 deletions
|
|
@ -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():
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
{
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue