Skip to content

fix(cli): parse-failure guard must not refuse valid empty {} config - #65975

Closed
ruangraung wants to merge 1 commit into
NousResearch:mainfrom
ruangraung:fix/config-parse-guard-empty-dict
Closed

ruangraung wants to merge 1 commit into
NousResearch:mainfrom
ruangraung:fix/config-parse-guard-empty-dict

Conversation

@ruangraung

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a regression in #14276's config-parse-failure guard. The original guard added by @Societus used if _existing_size > 0 and not read_raw_config() to detect parse failures — but read_raw_config() returns {} for both a genuinely unparseable file and a valid empty {} config. The guard cannot tell them apart, so it incorrectly blocks legitimate writes to a fresh empty config.

This PR replaces the emptiness check with a real inline YAML parse using fast_safe_load (the same parser read_raw_config() uses). The new guard refuses to write only when the file is non-empty and parsing actually raises a yaml.YAMLError. A valid empty {} parses cleanly and passes straight through. Applied to both save_config() and set_config_value().

Related Issue

Directly addresses the CI regression introduced in #14276. The original bug (class of config-write-unsafe errors) is tracked in #58781 (closed P1). An alternative comprehensive approach is tracked in #62232.

Type of Change

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

Changes Made

  • hermes_cli/config.py — Added inline-parse guard in save_config() at line 7403 (after require_readable_config_before_write, before read_raw_config()) and in set_config_value() at line 8425 (same
    position). The guard calls fast_safe_load() directly; if it raises, the file is genuinely unparseable and the write is refused with a warning. A valid {} document parses without error and is allowed through.
  • tests/hermes_cli/test_save_config_wipe_guard.py — 8 tests covering valid empty {}, valid non-empty, unparseable garbage (unclosed bracket), and missing file, for both save_config() and set_config_value().

How to Test

# Step 1: Run guard-specific tests (8 tests)
pytest tests/hermes_cli/test_save_config_wipe_guard.py -v

# Step 2: Run the three dashboard profile tests that regressed in #14276 (59 tests)
pytest tests/hermes_cli/test_web_server_profile_unification.py \
       tests/hermes_cli/test_web_server_messaging_profiles.py \
       tests/hermes_cli/test_web_server_skills_profiles.py -v

Checklist

Code

Hermes Agent v0.18.2 (2026.7.7.2) · upstream 42bd4368
Install method: git
Python: 3.11.15
OpenAI SDK: 2.24.0

Documentation & Housekeeping

  • N/A — pure bug fix, no config key changes, no doc updates needed
  • N/A — no config keys changed
  • N/A — no architecture changes
  • N/A — pure Python I/O + YAML parsing, no OS-specific code
  • N/A — no tool behavior changes

Screenshots / Logs

Guard tests — 8/8 pass:

$ .venv/bin/python -m pytest tests/hermes_cli/test_save_config_wipe_guard.py -v
============================= test session starts ==============================
platform linux -- Python 3.13.5, pytest-9.0.2, pluggy-1.6.0
rootdir: /home/username/hermes-agent-pr
configfile: pyproject.toml
collected 8 items

tests/hermes_cli/test_save_config_wipe_guard.py ........ [100%]
============================== 8 passed in 0.21s ==============================

Dashboard profile tests (regressed by #14276's guard) — 59/59 pass:

$ .venv/bin/python -m pytest \
    tests/hermes_cli/test_web_server_profile_unification.py \
    tests/hermes_cli/test_web_server_messaging_profiles.py \
    tests/hermes_cli/test_web_server_skills_profiles.py -v

[59 tests — all PASSED]
============================== 59 passed in 13.95s ============================

Guard blocking a genuinely unparseable config (the save_config warning):

WARNING  hermes_cli.config:config.py:7419 Refusing to save config:
  config.yaml is non-empty (12 bytes) but failed to parse.
  Writing now would wipe all non-default sections. Fix the YAML or retry.

Guard blocking set_config_value on the same unparseable file:

WARNING  hermes_cli.config:config.py:8425 Refusing to set config value:
  config.yaml is non-empty (12 bytes) but failed to parse.
  Writing now would wipe all non-default sections. Fix the YAML or retry.

Pre-existing CI failure from #14276 run 29447346642 (for reference — these are the failures this PR fixes):

Slice 4: tests/hermes_cli/test_web_server_profile_unification.py → KeyError: 'platforms' (guard refused to write valid {} config)

Slice 5: tests/hermes_cli/test_web_server_messaging_profiles.py → assert telegram["enabled"] is True (config was wiped)

Slice 8: tests/hermes_cli/test_web_server_skills_profiles.py → toolset/skills scoping (config was wiped)

All three slices are green on main (run 29464923384), confirming the regression was introduced by the guard in #14276.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good. No obvious issues found.


Reviewed by Hermes Agent

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 16, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Comment

Summary

Fix: deduplicates model-switch markers in gateway. Ensures only the latest marker is retained in history.

Clean fix. No security concerns.


Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for identifying the empty-{} ambiguity. The underlying malformed-YAML overwrite path is still present on current main: read_raw_config() returns {} after a parser exception at hermes_cli/config.py:6924-6929, and save_config() subsequently writes that replacement at hermes_cli/config.py:7416 and hermes_cli/config.py:7472-7476.

Problems

  • The same read-then-write pattern remains in hermes_cli/auth.py:6664-6698; provider updates can still replace malformed YAML. A two-site guard does not close the config-write bug class.
  • A valid but structurally invalid root ([] or a scalar) parses without raising. read_raw_config() turns it into {} at hermes_cli/config.py:6931-6932, so a parse-exception-only guard still permits save_config() to replace it.
  • Commit 368c91f9280aeb97e5988e311391946060860097 also adds an unrelated .local-hooks/ .gitignore rule.

Suggested changes

  • Put parsed-mapping validation on the shared config-write path and cover the auth writer.
  • Add scalar/list-root preservation coverage and remove the unrelated ignore rule.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit label Jul 18, 2026
…es non-mapping roots

read_raw_config() collapses both a parse exception AND a top-level list/scalar
root to {}, so save_config / set_config_value could wipe every non-default
section on the next write. The previous commit only caught exceptions, leaving
list/scalar roots to slip through.

- Extract _refuse_write_if_unparsable() as the single chokepoint (config.py),
  used by save_config(), set_config_value(), and the auth provider writer so the
  guard cannot be skipped via a different write path.
- Refuse when the existing file parses to a non-dict root (list/scalar), not only
  on a parse error; a genuinely valid empty {} still passes through.
- Cover the auth.py provider writer (_update_config_for_provider) with the same guard.
- Drop the unrelated .local-hooks/ .gitignore rule added in 368c91f.
- Extend tests: list-root, scalar-root, valid-empty-{} pass, and auth-writer-refuses.

refs NousResearch#65975
@ruangraung
ruangraung force-pushed the fix/config-parse-guard-empty-dict branch from 368c91f to 998c28d Compare July 18, 2026 21:58
@ruangraung

ruangraung commented Jul 18, 2026 •

Copy link
Copy Markdown
Contributor Author

@teknium1 thanks for the thorough review, and fair call on the empty-{} blind spot. Reworked and force-pushed (368c91f → 998c28d, rebased onto current origin/main). All three problems and both suggestions are covered:

Shared guard + the auth path. You're right that a two-site guard doesn't close the class, so I collapsed it into one chokepoint, _refuse_write_if_unparsable() (config.py:7017), and routed every config write through it: save_config (config.py:7515), set_config_value (config.py:8507), and the provider writer _update_config_for_provider (auth.py:6664, right before its atomic_yaml_write at auth.py:6701). Malformed YAML can't slip through the auth path anymore.

Non-dict roots. Exactly the trap you flagged: read_raw_config() coerces []/scalar to {} at config.py:6987-6988, so an exception-only guard still let save_config clobber it. The new guard does an explicit isinstance(parsed, dict) check (config.py:7048), list/scalar roots get refused, a legit empty {} still goes through.

.local-hooks/ gitignore rule. Removed. The committed PR shows zero .gitignore changes; the working tree now matches origin/main (upstream had already dropped it in the 279-commit rebase window).

Tests. Extended test_save_config_wipe_guard.py (+112 lines, 15 total): list/scalar roots refused on both save_config and set_config_value, valid-empty-{} passes, auth-writer refuses + passes. All 15 green under scripts/run_tests.sh.

CI green now on 998c28d (23 passed, 1 neutral, 9 skipped). Given the blast-broad label I kept the change fail-closed and additive, happy to tighten anything else later if needed.

@kshitijk4poor

Copy link
Copy Markdown
Contributor

Following up on your coordination comment — thanks for the constructive approach, and you were right that the two PRs together close the whole family.

The set/unset side merged via #96169 (salvage of #71385, authorship preserved). During review we widened the shared guard itself: require_readable_config_before_write now parses the file and refuses unparseable / non-mapping YAML for every caller — which transitively covers the paths this PR targeted: save_config() (guard runs at hermes_cli/config.py:4012 before the write) and both auth provider writers (_update_config_for_provider, _reset_config_provider — they call the guard before atomic_yaml_write). Verified E2E: save_config on a corrupt config now refuses and snapshots a .corrupt.*.bak instead of rewriting with defaults.

That makes this PR's mechanism (_refuse_write_if_unparsable as a second parallel guard) redundant on current main, so closing it — but the diagnosis here (the read_raw_config() {}-collapse ambiguity, and the empty-{} false-refuse in #14276's approach) directly shaped the merged design, and your coverage analysis of the auth writer paths was used to verify the widened guard. Credited in #96169's PR body. Thanks for the careful work and the coordination offer.

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

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants