From 4d24f357a03b8bd2c9644e69b784c481ec0c5914 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Thu, 3 Sep 2026 12:17:52 +0530 Subject: [PATCH] fix(config): strict version check also refuses non-mapping config roots A config.yaml whose top-level value is a list or scalar parses fine, so the strict check_config_version(raise_on_parse_error=True) from #101778 still returned (0, latest) and migrate_config() proceeded: sanitize_env_file() rewrote .env, then save_config()'s fail-closed guard raised RuntimeError. Raise InvalidUserConfigError up front for that shape too, so the "no side effect before the invalid config is surfaced" guarantee holds for both invalid-config shapes. Tolerant callers are unchanged. Tests: parametrize the two #101778 regression tests over malformed-yaml and list-root; assert the tolerant call still does not raise. Also make the test_update_autostash check_config_version mock kwarg-tolerant, matching the author's fix in test_config.py. --- hermes_cli/config.py | 8 ++++++++ tests/hermes_cli/test_config.py | 22 ++++++++++++++++------ tests/hermes_cli/test_update_autostash.py | 2 +- 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 9fe2dfb806..dd0c9f6ea8 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -2306,6 +2306,14 @@ def check_config_version(*, raise_on_parse_error: bool = False) -> Tuple[int, in return latest, latest if not isinstance(config, dict): + # A list/scalar root parses fine but is just as unusable as broken + # YAML: save_config() would refuse it later, after .env was already + # rewritten. Strict callers must see it up front too. + if raise_on_parse_error: + raise InvalidUserConfigError( + f"Cannot inspect {config_path}: config.yaml top-level value must be " + f"a mapping, got {type(config).__name__}" + ) config = {} current = _coerce_config_version(config.get("_config_version")) return current, latest diff --git a/tests/hermes_cli/test_config.py b/tests/hermes_cli/test_config.py index 7944a57ab0..186cd97882 100644 --- a/tests/hermes_cli/test_config.py +++ b/tests/hermes_cli/test_config.py @@ -786,24 +786,34 @@ class TestConfigVersionDetection: assert load_config()["_config_version"] == DEFAULT_CONFIG["_config_version"] assert check_config_version() == (0, DEFAULT_CONFIG["_config_version"]) - def test_strict_check_rejects_malformed_yaml(self, tmp_path): + _INVALID_CONFIG_CASES = [ + pytest.param(b"model: [unterminated\n", "not valid YAML", id="malformed-yaml"), + pytest.param(b"- just_a_list\n", "must be a mapping", id="list-root"), + ] + + @pytest.mark.parametrize("config_bytes, match", _INVALID_CONFIG_CASES) + def test_strict_check_rejects_invalid_config(self, tmp_path, config_bytes, match): config_path = tmp_path / "config.yaml" - config_path.write_text("model: [unterminated\n", encoding="utf-8") + config_path.write_bytes(config_bytes) with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): - with pytest.raises(InvalidUserConfigError, match="not valid YAML"): + with pytest.raises(InvalidUserConfigError, match=match): check_config_version(raise_on_parse_error=True) + # Tolerant callers keep the historical non-raising behavior. + check_config_version() - def test_migration_rejects_malformed_yaml_before_sanitizing_env(self, tmp_path): + @pytest.mark.parametrize("config_bytes, match", _INVALID_CONFIG_CASES) + def test_migration_rejects_invalid_config_before_sanitizing_env( + self, tmp_path, config_bytes, match + ): config_path = tmp_path / "config.yaml" - config_bytes = b"model: [unterminated\n" config_path.write_bytes(config_bytes) env_path = tmp_path / ".env" env_bytes = b"OPENAI_API_KEY=test-without-final-newline" env_path.write_bytes(env_bytes) with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): - with pytest.raises(InvalidUserConfigError, match="not valid YAML"): + with pytest.raises(InvalidUserConfigError, match=match): migrate_config(interactive=False, quiet=True) assert config_path.read_bytes() == config_bytes diff --git a/tests/hermes_cli/test_update_autostash.py b/tests/hermes_cli/test_update_autostash.py index f7147dfa5c..b86cd898da 100644 --- a/tests/hermes_cli/test_update_autostash.py +++ b/tests/hermes_cli/test_update_autostash.py @@ -91,7 +91,7 @@ def _setup_update_mocks(monkeypatch, tmp_path): monkeypatch.setattr(hermes_main, "_restore_stashed_changes", lambda *a, **kw: True) monkeypatch.setattr(hermes_config, "get_missing_env_vars", lambda required_only=True: []) monkeypatch.setattr(hermes_config, "get_missing_config_fields", lambda: []) - monkeypatch.setattr(hermes_config, "check_config_version", lambda: (5, 5)) + monkeypatch.setattr(hermes_config, "check_config_version", lambda **_kwargs: (5, 5)) monkeypatch.setattr(hermes_config, "migrate_config", lambda **kw: {"env_added": [], "config_added": []}) monkeypatch.setattr(hermes_main, "_upgrade_pip_before_lazy_refresh", lambda *a, **kw: None) monkeypatch.setattr(hermes_main, "_refresh_active_lazy_features", lambda *a, **kw: True)