From fe25806a6bfa8318d56a3e1a4065abf04d619fb2 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Thu, 9 Jul 2026 16:36:03 -0700 Subject: [PATCH] fix(config): retain last-known-good config when config.yaml fails to parse (#60591) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Port from openai/codex#31188: a parse failure in a policy-bearing config file must not silently replace the effective policy with an empty/default one. Codex's load_exec_policy_with_warning replaced the whole exec policy with Policy::empty() when a .rules file failed to parse, silently dropping managed prompt/forbidden rules; the fix preserves the managed policy while still warning. Hermes had the same bug shape in load_config(): a YAML parse error made _load_config_impl() fall through to DEFAULT_CONFIG, dropping every user override — including approvals.deny rules, which are documented to block commands even under --yolo. In a long-running gateway, a user mid-editing config.yaml into broken YAML silently disarmed their own deny rules on the next load. Now, when the process has a last successfully loaded config for that path (_LAST_EXPANDED_CONFIG_BY_PATH), a parse failure keeps serving it (cached under the corrupt file's signature so the broken file isn't re-parsed) and the warning says edits are being ignored until the YAML is fixed. Fresh processes with no last-known-good keep the existing DEFAULT_CONFIG fallback and warning. E2E-verified: deny rule 'curl*evil.com*' still blocks after mid-process corruption; fixed file reloads normally; fresh-process fallback unchanged. --- hermes_cli/config.py | 68 +++++++++++++++++++++--- tests/hermes_cli/test_config.py | 93 +++++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 8 deletions(-) diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 678cdb869af..153fc5fb606 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -93,7 +93,9 @@ def _backup_corrupt_config(config_path: Path) -> Optional[Path]: return None -def _warn_config_parse_failure(config_path: Path, exc: Exception) -> None: +def _warn_config_parse_failure( + config_path: Path, exc: Exception, *, fallback: str = "defaults" +) -> None: """Surface a config.yaml parse failure to user, log, and stderr. A YAML parse error in ``~/.hermes/config.yaml`` causes ``load_config()`` @@ -110,6 +112,11 @@ def _warn_config_parse_failure(config_path: Path, exc: Exception) -> None: timestamped ``.bak`` (best-effort) so the user's recoverable content survives any later rewrite of ``config.yaml`` by the setup wizard or ``hermes config set``. + + ``fallback`` selects the message wording: ``"defaults"`` (fresh process, + nothing else to serve) or ``"last-known-good"`` (in-process retention of + the previously loaded config — see the codex#31188 port in + ``_load_config_impl``). """ try: st = config_path.stat() @@ -122,12 +129,19 @@ def _warn_config_parse_failure(config_path: Path, exc: Exception) -> None: backup_path = _backup_corrupt_config(config_path) - msg = ( - f"Failed to parse {config_path}: {exc}. " - f"Falling back to default config — every user override " - f"(auxiliary providers, fallback chain, model settings) is being IGNORED. " - f"Fix the YAML and restart." - ) + if fallback == "last-known-good": + msg = ( + f"Failed to parse {config_path}: {exc}. " + f"Keeping the previously loaded config for this process — " + f"edits to config.yaml are being IGNORED until the YAML is fixed." + ) + else: + msg = ( + f"Failed to parse {config_path}: {exc}. " + f"Falling back to default config — every user override " + f"(auxiliary providers, fallback chain, model settings) is being IGNORED. " + f"Fix the YAML and restart." + ) if backup_path is not None: msg += f" A copy of the corrupted file was saved to {backup_path}." logger.warning(msg) @@ -6936,7 +6950,45 @@ def _load_config_impl(*, want_deepcopy: bool) -> Dict[str, Any]: config = _deep_merge(config, user_config) except Exception as e: - _warn_config_parse_failure(config_path, e) + # Last-known-good fallback (port of openai/codex#31188's + # invariant: a parse failure in a policy/config file must not + # silently replace the effective policy with an empty/default + # one). Falling through to DEFAULT_CONFIG here drops EVERY user + # override — including security-critical ``approvals.deny`` + # rules, which are supposed to block commands even under yolo. + # A long-running gateway whose user mid-edits config.yaml into + # broken YAML would silently lose those rules on the next load. + # Within a running process we still have the last successfully + # loaded config — keep serving it until the file is fixed. + # Fresh processes with no last-known-good keep the existing + # DEFAULT_CONFIG fallback. + lkg = _LAST_EXPANDED_CONFIG_BY_PATH.get(path_key) + _warn_config_parse_failure( + config_path, + e, + fallback="last-known-good" if lkg is not None else "defaults", + ) + if lkg is not None: + # save_config() stores the pre-expansion normalized dict + # (env-ref templates preserved); the load path stores the + # expanded one. Expand defensively — idempotent when the + # stored value is already expanded. + from typing import cast as _cast + lkg_copy: Dict[str, Any] = _cast( + Dict[str, Any], _expand_env_vars(copy.deepcopy(lkg)) + ) + if cache_sig is not None: + # Cache under the corrupt file's signature (empty env + # snapshot: always valid) so repeated loads don't + # re-parse the broken file; fixing the file changes the + # signature and triggers a normal reload. + _empty_env: Dict[str, Optional[str]] = {} + _LOAD_CONFIG_CACHE[path_key] = ( + cache_sig[0], cache_sig[1], + cache_sig[2], cache_sig[3], + lkg_copy, _empty_env, + ) + return copy.deepcopy(lkg_copy) if want_deepcopy else lkg_copy normalized = _normalize_root_model_keys(_normalize_max_turns_config(config)) expanded = _expand_env_vars(normalized) diff --git a/tests/hermes_cli/test_config.py b/tests/hermes_cli/test_config.py index 06bc395214f..dfc1c457cd3 100644 --- a/tests/hermes_cli/test_config.py +++ b/tests/hermes_cli/test_config.py @@ -266,6 +266,99 @@ class TestLoadConfigParseFailure: assert not list(tmp_path.glob("config.yaml.corrupt.*.bak")) + def test_last_known_good_retained_within_process(self, tmp_path, capsys): + """Port of openai/codex#31188's invariant: a parse failure must not + silently replace the effective config (policy included) with + defaults when the process already loaded a good config. + + Scenario: long-running gateway, user mid-edits config.yaml into + broken YAML. Before this fix the next load_config() dropped every + override — including ``approvals.deny`` security rules. Now the + last successfully loaded config keeps being served until the file + parses again. + """ + import time + from hermes_cli import config as cfg_mod + cfg_mod._CONFIG_PARSE_WARNED.clear() + + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + cfg = tmp_path / "config.yaml" + cfg.write_text( + "model:\n default: test/custom-model\n" + "approvals:\n deny:\n - 'curl*evil.com*'\n" + ) + + good = load_config() + assert good["model"]["default"] == "test/custom-model" + assert good["approvals"]["deny"] == ["curl*evil.com*"] + capsys.readouterr() + + # Corrupt the file (mtime must change to bust the cache) + time.sleep(0.05) + cfg.write_text("approvals:\n deny: [unclosed\n :::bad {{{\n") + + after = load_config() + # Last-known-good retained — NOT defaults + assert after["model"]["default"] == "test/custom-model" + assert after["approvals"]["deny"] == ["curl*evil.com*"] + # Warning says we kept the previous config, not defaults + err = capsys.readouterr().err + assert "previously loaded config" in err + + def test_last_known_good_recovers_after_fix(self, tmp_path): + """Fixing the YAML picks up the new content on the next load.""" + import time + from hermes_cli import config as cfg_mod + cfg_mod._CONFIG_PARSE_WARNED.clear() + + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + cfg = tmp_path / "config.yaml" + cfg.write_text("model:\n default: test/first\n") + assert load_config()["model"]["default"] == "test/first" + + time.sleep(0.05) + cfg.write_text("\tbroken:\n") + assert load_config()["model"]["default"] == "test/first" + + time.sleep(0.05) + cfg.write_text("model:\n default: test/second\n") + assert load_config()["model"]["default"] == "test/second" + + def test_fresh_process_still_falls_back_to_defaults(self, tmp_path): + """With no last-known-good (fresh process for this path), a broken + config still falls back to DEFAULT_CONFIG as before.""" + from hermes_cli import config as cfg_mod + cfg_mod._CONFIG_PARSE_WARNED.clear() + + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + (tmp_path / "config.yaml").write_text("\tbroken:\n") + # No prior good load for this path in _LAST_EXPANDED_CONFIG_BY_PATH + cfg_mod._LAST_EXPANDED_CONFIG_BY_PATH.pop( + str(tmp_path / "config.yaml"), None + ) + config = load_config() + assert config["model"] == DEFAULT_CONFIG["model"] + + def test_last_known_good_cached_no_rewarn_spam(self, tmp_path, capsys): + """Repeated loads of the same broken file serve the cached LKG and + don't re-warn (dedup on mtime/size still applies).""" + import time + from hermes_cli import config as cfg_mod + cfg_mod._CONFIG_PARSE_WARNED.clear() + + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + cfg = tmp_path / "config.yaml" + cfg.write_text("model:\n default: test/custom\n") + load_config() + time.sleep(0.05) + cfg.write_text("\tbroken:\n") + + load_config() + capsys.readouterr() + second = load_config() + assert second["model"]["default"] == "test/custom" + assert capsys.readouterr().err == "" + class TestSaveAndLoadRoundtrip: @staticmethod