fix(config): retain last-known-good config when config.yaml fails to parse

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)
@@ -6875,7 +6889,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)

tests/hermes_cli/test_config.py

@@ -266,6 +266,99 @@ def test_corrupt_symlink_config_not_backed_up(self, tmp_path):
 
             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