fix(cli): reject corrupt config in noninteractive runs - #81988
Conversation
DavidMetcalfe
left a comment
There was a problem hiding this comment.
Thanks for the fast, well-scoped fix. The guard placement and test coverage look solid:
- Profile resolution runs at import time (
_apply_profile_override()), beforecmd_chat, so the guard reads the profileconfig.yaml— the exact path from the issue. - All non-interactive CLI paths pass through the guard: top-level oneshot (
-z),-q/-Qquery,--quiet, and directcmd_chatcallers (_prepare_agent_startupat main.py:10960/10984/12597 + top ofcmd_chat). - The guard uses
fast_safe_load, the same parser_load_config_impluses, so parse semantics match the normal load path. - Safe mode is double-covered:
_apply_safe_modealready setsHERMES_IGNORE_USER_CONFIG=1(main.py:10898), and the guard also passes the flag explicitly.
One substantive gap to consider — follow-up, not a blocker for this PR:
- Gateway-scheduled agent runs (Hermes' own cron jobs, not shell-level
hermes chat -Q) execute in-process and never pass throughcmd_chat/_prepare_agent_startup. A corrupt config at gateway startup still falls back to defaults for those jobs — the same spend class as the issue, one layer up. Worth a follow-up that fails closed at gateway startup (or refuses to schedule) when the profile config is unparseable; the gateway's last-known-good in-process retention also means config edits stay silently ignored until restart.
Nits:
-q ""(empty query): the guard treatsquery is not Noneas non-interactive, whileinteractive_promptuses truthiness (not query). An empty-string query would now fail closed instead of dropping to the interactive prompt. Degenerate usage, but the two checks disagree — considerbool(getattr(args, "query", None))for consistency.- Coverage suggestions: (a) env-var-only bypass (
HERMES_IGNORE_USER_CONFIG=1, no CLI flag) isn't exercised — the test fixture deletes it; (b) a Namespace-reuse case (same args object, config repaired between calls) for the_noninteractive_config_validatedflag. PermissionError/unreadable config now fails closed too — I think that's the right default (silently running with defaults is the bug), just noting it's a behavior change beyond parse errors.
Scope notes for the issue's expected behaviors: fail-closed exit + ERROR + backup covers #1 and #4; credential-ingestion logging (#3) is moot here since the guard exits before provider/credential init; the paid silent default (#2) is a separate maintainer policy decision (#64635) unaffected by this PR.
The fix is solid — exit 2, ERROR log, and backup behavior all match the issue's ask. @embwl0x nice work.
2ce4a73 to
80798a7
Compare
|
@DavidMetcalfe Thanks for the careful review. Updated current head
The focused regression file passes all 16 tests after rebasing onto current main, and the publish gate passed again. I agree the in-process gateway/cron path is a distinct follow-up boundary rather than part of this CLI-scoped patch. |
|
This was generated by AI during triage. Summary: Problems:
Solution: Checked against |
80798a7 to
ca9a55e
Compare
|
Addressed in ca9a55e. I removed Regression coverage now exercises all three cases. I rebased onto current main and reran the guard, single-query finalization, argument propagation, and relaunch suites: 45 tests passed; ruff and |
Summary
--ignore-user-configbehavior intactWhy
A fresh process currently falls back to defaults after a config parse failure. If the profile
.envcontainsOPENROUTER_API_KEY, a noninteractive run can then resolve OpenRouter and silently incur spend against a provider the broken config never selected.The guard runs before plugin discovery or provider initialization and covers the full parser, Termux fast path, and direct
cmd_chatcallers.Verification
tests/hermes_climatrix: 4,610 passed; one unrelated macOS service-manager mode test was flaky and passed standalone on both exact base and this brancherrors.log, corrupt file preserved and backed up, no provider initialization or API callruff,git diff --check, publish-range gitleaks, and metadata checks passedFixes #81952