Skip to content

test: stop 5 config.set tests writing the REAL ~/.hermes/config.yaml - #61225

Closed
slow4cyl wants to merge 2 commits into
NousResearch:mainfrom
slow4cyl:fix/tests-config-set-no-real-home-writes
Closed

slow4cyl wants to merge 2 commits into
NousResearch:mainfrom
slow4cyl:fix/tests-config-set-no-real-home-writes

Conversation

@slow4cyl

@slow4cyl slow4cyl commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

test_config_set_model_{requires_confirmation_for_expensive_model,
explicit_provider_skips_broken_default_init,does_not_leak_inference_provider_env,
records_per_session_override_not_env,switches_agent_without_touching_env}
exercised the full config.set flow with switch_model mocked but let
_persist_model_switch call the real cli.save_config_value — which resolves
the config path from cli._hermes_home captured at import. Run without
HERMES_HOME (any plain 'pytest tests/'), they clobber the live user config:
last writer left model.default=anthropic/claude-sonnet-4.6 +
base_url=https://api.anthropic.com, silently repointing every fleet worker
at an Anthropic endpoint (observed live 2026-07-08 16:38; ~2000 worker
deaths on non-retryable 400s until restored).

Mock cli.save_config_value in all five, same as
test_config_set_model_global_persists already does.

🤖 Generated with Claude Code

@slow4cyl
slow4cyl force-pushed the fix/tests-config-set-no-real-home-writes branch from 65a5c15 to 1e7e9fc Compare July 9, 2026 01:39
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have duplicate This issue or pull request already exists labels Jul 9, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Duplicate of #57182 — both patch the same 5 config.set model tests in tests/test_tui_gateway_server.py with the same monkeypatch.setattr("cli.save_config_value", ...) mock. #57182 (2026-07-02) is the earliest-open fix and additionally adds a conftest tripwire that fails any test writing the repo-root/user config. Marking this as duplicate of the earlier, more complete PR.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the five model-switch tests.

Problems

  • The targeted mocks are correct for the current path: tui_gateway/server.py:2795-2812 calls cli.save_config_value, whose fallback writes repo-root cli-config.yaml when cli._hermes_home/config.yaml is absent (cli.py:3644-3646). The five affected tests remain unmocked on current main, including tests/test_tui_gateway_server.py:3497-3562.
  • This PR does not include a suite-wide regression guard. Earlier open PR #57182 includes the same five mocks plus an autouse tests/conftest.py tripwire for repo-root cli-config.yaml writes; current main has no equivalent guard.

Suggested changes

  • Salvage this patch together with #57182's tripwire, or add equivalent protection so future real save_config_value() calls fail at the offending test.

Automated hermes-sweeper review.

@@ -3526,6 +3526,7 @@ def switch_model(self, **_kwargs):
"hermes_cli.model_switch.switch_model", lambda **_kwargs: result
)
monkeypatch.setattr(server, "_restart_slash_worker", lambda sid, session: None)
monkeypatch.setattr("cli.save_config_value", lambda *_a, **_k: True)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mock fixes this test's real persistence path. Please pair the five targeted mocks with a suite-wide repo-root cli-config.yaml write guard (as in earlier open PR #57182), so future unmocked callers fail at the source.

@teknium1 teknium1 added sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 10, 2026
slow4cyl and others added 2 commits July 10, 2026 11:08
test_config_set_model_{requires_confirmation_for_expensive_model,
explicit_provider_skips_broken_default_init,does_not_leak_inference_provider_env,
records_per_session_override_not_env,switches_agent_without_touching_env}
exercised the full config.set flow with switch_model mocked but let
_persist_model_switch call the real cli.save_config_value — which resolves
the config path from cli._hermes_home captured at import. Run without
HERMES_HOME (any plain 'pytest tests/'), they clobber the live user config:
last writer left model.default=anthropic/claude-sonnet-4.6 +
base_url=https://api.anthropic.com, silently repointing every fleet worker
at an Anthropic endpoint (observed live 2026-07-08 16:38; ~2000 worker
deaths on non-retryable 400s until restored).

Mock cli.save_config_value in all five, same as
test_config_set_model_global_persists already does.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…test guard)

Per review: the five targeted mocks stand; add the missing suite-wide
regression guard so no future test can write the developer's live
config.yaml or the repo-root cli-config.yaml fallback.

Two layers in tests/conftest.py:

1. Call-time interception — _arm_save_config_tripwire() wraps
   cli.save_config_value (marker-tagged, armed process-wide, re-armed
   by the autouse fixture so it survives importlib.reload(cli)) and
   fails the offending test at its call site BEFORE the write lands,
   mirroring the function's own target resolution
   (cli._hermes_home/config.yaml if present, else repo-root
   cli-config.yaml). Writes into per-test tmp homes pass through
   untouched; leaked background threads calling save_config_value
   between tests are blocked too (the failure dies inside that
   thread). cli is deliberately not force-imported (its module-level
   load_hermes_dotenv would mutate os.environ mid-test).
2. Before/after watch (autouse fixture) — snapshot both forbidden
   files around every test; restore the pre-test bytes and fail the
   test in whose window the write landed if anything slips past the
   wrapper (cli imported mid-test, direct YAML writers, subprocesses),
   so one polluting write cannot cascade into later runs.

Forbidden paths resolve at conftest-import time from the PRE-isolation
environment (hermes_constants.get_hermes_home()), i.e. the home pytest
was invoked with — per-test tmp homes are never affected.

tests/test_config_write_guard.py pins both layers: repo-root fallback
blocked, pre-isolation user config blocked (both shapes), tmp-home
writes allowed through to the real implementation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@slow4cyl
slow4cyl force-pushed the fix/tests-config-set-no-real-home-writes branch from 1e7e9fc to 2c422a7 Compare July 10, 2026 17:57
@slow4cyl

Copy link
Copy Markdown
Contributor Author

Added the suite-wide guard as an autouse tripwire in tests/conftest.py — equivalent to #57182's but extended to also cover the live user config.yaml, not just the repo-root cli-config.yaml fallback. Two layers: cli.save_config_value is wrapped process-wide (marker-tagged, reload-safe) to fail an offending test at the call site before the write lands, and a per-test before/after snapshot restores the pre-test bytes and fails the test if anything slips past (cli imported mid-test, direct writers). Both forbidden paths resolve at conftest-import time from the pre-isolation environment, so tests persisting into their own tmp homes are untouched. Verified by temporarily reverting one of the five mocks: the test fails with the tripwire message and the polluting cli-config.yaml is restored; with mocks intact the affected slice's failure set is byte-identical to main. tests/test_config_write_guard.py pins both layers permanently. Rebased onto current main.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks @slow4cyl — closing as resolved on main: same class as #57182 — the repo-root/real-config fallback this guards against was removed from cli.save_config_value, so the tripwire would mirror dead code. Verified empirically post-prune.

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 duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants