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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user