Skip to content

fix(tests): stop config.set model tests writing cli-config.yaml into the repo root - #57182

Closed
lEWFkRAD wants to merge 1 commit into
NousResearch:mainfrom
lEWFkRAD:fix/tests-repo-root-cli-config-pollution
Closed

fix(tests): stop config.set model tests writing cli-config.yaml into the repo root#57182
lEWFkRAD wants to merge 1 commit into
NousResearch:mainfrom
lEWFkRAD:fix/tests-repo-root-cli-config-pollution

Conversation

@lEWFkRAD

@lEWFkRAD lEWFkRAD commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Stops the test suite from silently writing a cli-config.yaml into the repo root of the developer's checkout, and adds a tripwire so any future offender fails loudly by name.

cli.save_config_value() writes the user config at cli._hermes_home / 'config.yaml' only if it already exists, otherwise it falls back to the project config — cli-config.yaml next to cli.py, i.e. the repo root. The suite's hermetic fixture points HERMES_HOME at a fresh empty per-test tempdir, so during tests the user config never exists and any real save_config_value() call lands in the repo root. The file is gitignored (invisible in git status) and is read back as the project-config fallback by every later run, silently breaking tests/hermes_cli/test_ignore_user_config_flags.py and tests/test_hermes_state.py.

Five tests in tests/test_tui_gateway_server.py hit this: they drive a real /model switch through server.handle_request without patching cli.save_config_value — and a plain /model (no flag) persists globally by default (resolve_persist_behaviormodel.persist_switch_by_default defaults True), so _persist_model_switch ran the real save with model.default: anthropic/claude-sonnet-4.6, provider: anthropic, base_url: https://api.anthropic.com.

Related Issue

Fixes #57147

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • tests/test_tui_gateway_server.py — patch cli.save_config_value in the five offending tests (test_config_set_model_requires_confirmation_for_expensive_model, test_config_set_model_explicit_provider_skips_broken_default_init, test_config_set_model_does_not_leak_inference_provider_env, test_config_set_model_records_per_session_override_not_env, test_config_set_model_switches_agent_without_touching_env), matching the existing pattern in test_config_set_model_global_persists.
  • tests/conftest.py — new autouse _guard_repo_root_cli_config fixture: fails any test that creates or mutates the repo-root cli-config.yaml, and restores the pre-test state so one offender can't cascade into unrelated failures. (This tripwire is how the five tests above were identified.)

How to Test

  1. On a clean checkout without this PR: python -m pytest tests/test_tui_gateway_server.py -q → an untracked cli-config.yaml appears at the repo root afterwards (it's gitignored — check with ls, not git status).
  2. With this PR: same command → 303 passed, no cli-config.yaml.
  3. Revert one of the five test patches while keeping the conftest guard → the guard fails that exact test with a message naming the file and the written content.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — tests/test_tui_gateway_server.py is fully green with this change (303 passed, 0 errors, repo root clean afterwards), and a full-suite run via scripts/run_tests_parallel.py on native Windows (together with the Windows: parallel test runner children share the console process group — one os.kill(pid, 0) probe broadcasts KeyboardInterrupt across the whole run #57145 fix) ends with no repo-root cli-config.yaml. Remaining full-suite failures are pre-existing native-Windows failures that reproduce under plain python -m pytest on unmodified main — none involve the new guard.
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features) — the conftest tripwire is the regression guard
  • I've tested on my platform: Windows 11 Pro (native, no WSL), CPython 3.11.15

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — the fixture docstring documents the fallback trap
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — pure-pathlib guard, platform-independent
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

…the repo root

Five tests in tests/test_tui_gateway_server.py drive a real /model
switch through server.handle_request without patching
cli.save_config_value. A plain /model persists globally by default
(model.persist_switch_by_default → True), so _persist_model_switch ran
the real save — and because the hermetic per-test HERMES_HOME tempdir
never contains a config.yaml, save_config_value fell through to its
project-config fallback and wrote cli-config.yaml into the developer's
repo root:

    model:
      default: anthropic/claude-sonnet-4.6
      provider: anthropic
      base_url: https://api.anthropic.com

The file is gitignored, so the pollution is invisible in git status,
and later runs read it back as the project-config fallback — silently
breaking tests/hermes_cli/test_ignore_user_config_flags.py and
tests/test_hermes_state.py.

- Patch cli.save_config_value in the five offending tests
  (test_config_set_model_requires_confirmation_for_expensive_model,
  test_config_set_model_explicit_provider_skips_broken_default_init,
  test_config_set_model_does_not_leak_inference_provider_env,
  test_config_set_model_records_per_session_override_not_env,
  test_config_set_model_switches_agent_without_touching_env), matching
  the existing pattern in test_config_set_model_global_persists.
- Add an autouse tripwire fixture in tests/conftest.py that fails any
  test that creates or mutates the repo-root cli-config.yaml, restoring
  the pre-test state so one offender can't cascade into unrelated
  failures runs later.

Found during the PR NousResearch#57066 / issue NousResearch#57068 triage.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the test pollution. The premise remains valid on current main: cli.py:3646-3657 selects repo-root cli-config.yaml when the isolated home lacks config.yaml; hermes_cli/model_switch.py:416-443 makes a plain model switch persistent by default; and tui_gateway/server.py:3037-3038 then invokes _persist_model_switch.

The five targeted tests still exercise successful plain config.set model switches without mocking that persistence path (tests/test_tui_gateway_server.py:3623-3688, 3744-3789, 3832-3890, 3893-3957, 3960-4067). The existing explicit-global test already establishes the same mock pattern at tests/test_tui_gateway_server.py:3721-3741. The added autouse fixture provides a behavior-focused guard for future direct writes and restores detected pollution before reporting the offending test.

GitHub reports the PR cleanly mergeable against current main. This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks @lEWFkRAD — closing as resolved on main: cli.save_config_value now resolves get_hermes_home()/config.yaml live with no repo-root cli-config.yaml fallback. Verified empirically: /model persistence tests create no repo-root file and leave the real config byte-identical. Your issue #57147 documented the class correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test suite writes cli-config.yaml into the repo root (gitignored, silently breaks later config tests)

3 participants