fix(config): preserve env refs when save_config rewrites config (#11892)

Co-authored-by: binhnt92 <84617813+binhnt92@users.noreply.github.com>

hermes_cli/config.py

@@ -12,6 +12,7 @@ This module provides:
 - hermes config wizard   - Re-run setup wizard
 """
 
+import copy
 import os
 import platform
 import re
@@ -26,6 +27,7 @@ from typing import Dict, Any, Optional, List, Tuple
 
 _IS_WINDOWS = platform.system() == "Windows"
 _ENV_VAR_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$")
+_LAST_EXPANDED_CONFIG_BY_PATH: Dict[str, Any] = {}
 # Env var names written to .env that aren't in OPTIONAL_ENV_VARS
 # (managed by setup/provider flows directly).
 _EXTRA_ENV_KEYS = frozenset({
@@ -2635,6 +2637,85 @@ def _expand_env_vars(obj):
     return obj
 
 
+def _items_by_unique_name(items):
+    """Return a name-indexed dict only when all items have unique string names."""
+    if not isinstance(items, list):
+        return None
+    indexed = {}
+    for item in items:
+        if not isinstance(item, dict) or not isinstance(item.get("name"), str):
+            return None
+        name = item["name"]
+        if name in indexed:
+            return None
+        indexed[name] = item
+    return indexed
+
+
+def _preserve_env_ref_templates(current, raw, loaded_expanded=None):
+    """Restore raw ``${VAR}`` templates when a value is otherwise unchanged.
+
+    ``load_config()`` expands env refs for runtime use. When a caller later
+    persists that config after modifying some unrelated setting, keep the
+    original on-disk template instead of writing the expanded plaintext
+    secret back to ``config.yaml``.
+
+    Prefer preserving the raw template when ``current`` still matches either
+    the value previously returned by ``load_config()`` for this config path or
+    the current environment expansion of ``raw``. This handles env-var
+    rotation between load and save while still treating mixed literal/template
+    string edits as caller-owned once their rendered value diverges.
+    """
+    if isinstance(current, str) and isinstance(raw, str) and re.search(r"\${[^}]+}", raw):
+        if current == raw:
+            return raw
+        if isinstance(loaded_expanded, str) and current == loaded_expanded:
+            return raw
+        if _expand_env_vars(raw) == current:
+            return raw
+        return current
+
+    if isinstance(current, dict) and isinstance(raw, dict):
+        return {
+            key: _preserve_env_ref_templates(
+                value,
+                raw.get(key),
+                loaded_expanded.get(key) if isinstance(loaded_expanded, dict) else None,
+            )
+            for key, value in current.items()
+        }
+
+    if isinstance(current, list) and isinstance(raw, list):
+        # Prefer matching named config objects (e.g. custom_providers) by name
+        # so harmless reordering doesn't drop the original template. If names
+        # are duplicated, fall back to positional matching instead of silently
+        # shadowing one entry.
+        current_by_name = _items_by_unique_name(current)
+        raw_by_name = _items_by_unique_name(raw)
+        loaded_by_name = _items_by_unique_name(loaded_expanded)
+        if current_by_name is not None and raw_by_name is not None:
+            return [
+                _preserve_env_ref_templates(
+                    item,
+                    raw_by_name.get(item.get("name")),
+                    loaded_by_name.get(item.get("name")) if loaded_by_name is not None else None,
+                )
+                for item in current
+            ]
+        return [
+            _preserve_env_ref_templates(
+                item,
+                raw[index] if index < len(raw) else None,
+                loaded_expanded[index]
+                if isinstance(loaded_expanded, list) and index < len(loaded_expanded)
+                else None,
+            )
+            for index, item in enumerate(current)
+        ]
+
+    return current
+
+
 def _normalize_root_model_keys(config: Dict[str, Any]) -> Dict[str, Any]:
     """Move stale root-level provider/base_url into model section.
 
@@ -2702,7 +2783,6 @@ def read_raw_config() -> Dict[str, Any]:
 
 def load_config() -> Dict[str, Any]:
     """Load configuration from ~/.hermes/config.yaml."""
-    import copy
     ensure_hermes_home()
     config_path = get_config_path()
     
@@ -2723,8 +2803,11 @@ def load_config() -> Dict[str, Any]:
             config = _deep_merge(config, user_config)
         except Exception as e:
             print(f"Warning: Failed to load config: {e}")
-    
-    return _expand_env_vars(_normalize_root_model_keys(_normalize_max_turns_config(config)))
+
+    normalized = _normalize_root_model_keys(_normalize_max_turns_config(config))
+    expanded = _expand_env_vars(normalized)
+    _LAST_EXPANDED_CONFIG_BY_PATH[str(config_path)] = copy.deepcopy(expanded)
+    return expanded
 
 
 _SECURITY_COMMENT = """
@@ -2833,7 +2916,15 @@ def save_config(config: Dict[str, Any]):
 
     ensure_hermes_home()
     config_path = get_config_path()
-    normalized = _normalize_root_model_keys(_normalize_max_turns_config(config))
+    current_normalized = _normalize_root_model_keys(_normalize_max_turns_config(config))
+    normalized = current_normalized
+    raw_existing = _normalize_root_model_keys(_normalize_max_turns_config(read_raw_config()))
+    if raw_existing:
+        normalized = _preserve_env_ref_templates(
+            normalized,
+            raw_existing,
+            _LAST_EXPANDED_CONFIG_BY_PATH.get(str(config_path)),
+        )
 
     # Build optional commented-out sections for features that are off by
     # default or only relevant when explicitly configured.
@@ -2851,6 +2942,7 @@ def save_config(config: Dict[str, Any]):
         extra_content="".join(parts) if parts else None,
     )
     _secure_file(config_path)
+    _LAST_EXPANDED_CONFIG_BY_PATH[str(config_path)] = copy.deepcopy(current_normalized)
 
 
 def load_env() -> Dict[str, str]:

tests/cli/test_cli_save_config_value.py

@@ -64,6 +64,24 @@ class TestSaveConfigValueAtomic:
         result = yaml.safe_load(config_env.read_text())
         assert result["display"]["skin"] == "ares"
 
+    def test_preserves_env_ref_templates_in_unrelated_fields(self, config_env):
+        """The /model --global persistence path must not inline env-backed secrets."""
+        config_env.write_text(yaml.dump({
+            "custom_providers": [{
+                "name": "tuzi",
+                "api_key": "${TU_ZI_API_KEY}",
+                "model": "claude-opus-4-6",
+            }],
+            "model": {"default": "test-model", "provider": "openrouter"},
+        }))
+
+        from cli import save_config_value
+        save_config_value("model.default", "doubao-pro")
+
+        result = yaml.safe_load(config_env.read_text())
+        assert result["model"]["default"] == "doubao-pro"
+        assert result["custom_providers"][0]["api_key"] == "${TU_ZI_API_KEY}"
+
     def test_file_not_truncated_on_error(self, config_env, monkeypatch):
         """If atomic_yaml_write raises, the original file is untouched."""
         original_content = config_env.read_text()

tests/hermes_cli/test_config_env_refs.py

@@ -0,0 +1,169 @@
+import textwrap
+
+from hermes_cli.config import load_config, save_config
+
+
+def _write_config(tmp_path, body: str):
+    (tmp_path / "config.yaml").write_text(textwrap.dedent(body), encoding="utf-8")
+
+
+def _read_config(tmp_path) -> str:
+    return (tmp_path / "config.yaml").read_text(encoding="utf-8")
+
+
+def test_save_config_preserves_env_refs_on_unrelated_change(monkeypatch, tmp_path):
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    monkeypatch.setenv("TU_ZI_API_KEY", "sk-realsecret")
+    monkeypatch.setenv("ALT_SECRET", "alt-secret")
+    _write_config(
+        tmp_path,
+        """\
+        custom_providers:
+          - name: tuzi
+            base_url: https://api.tu-zi.com
+            api_key: ${TU_ZI_API_KEY}
+            headers:
+              Authorization: Bearer ${ALT_SECRET}
+            model: claude-opus-4-6
+        model:
+          default: claude-opus-4-6
+        """,
+    )
+
+    config = load_config()
+    config["model"]["default"] = "doubao-pro"
+    save_config(config)
+
+    saved = _read_config(tmp_path)
+    assert "api_key: ${TU_ZI_API_KEY}" in saved
+    assert "Authorization: Bearer ${ALT_SECRET}" in saved
+    assert "sk-realsecret" not in saved
+    assert "alt-secret" not in saved
+
+
+def test_save_config_preserves_unresolved_env_refs(monkeypatch, tmp_path):
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    monkeypatch.delenv("MISSING_SECRET", raising=False)
+    _write_config(
+        tmp_path,
+        """\
+        custom_providers:
+          - name: unresolved
+            api_key: ${MISSING_SECRET}
+            model: claude-opus-4-6
+        model:
+          default: claude-opus-4-6
+        """,
+    )
+
+    config = load_config()
+    config["display"]["compact"] = True
+    save_config(config)
+
+    assert "api_key: ${MISSING_SECRET}" in _read_config(tmp_path)
+
+
+def test_save_config_allows_intentional_secret_value_change(monkeypatch, tmp_path):
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    monkeypatch.setenv("TU_ZI_API_KEY", "sk-old-secret")
+    _write_config(
+        tmp_path,
+        """\
+        custom_providers:
+          - name: tuzi
+            api_key: ${TU_ZI_API_KEY}
+            model: claude-opus-4-6
+        model:
+          default: claude-opus-4-6
+        """,
+    )
+
+    config = load_config()
+    config["custom_providers"][0]["api_key"] = "sk-new-secret"
+    save_config(config)
+
+    saved = _read_config(tmp_path)
+    assert "api_key: sk-new-secret" in saved
+    assert "${TU_ZI_API_KEY}" not in saved
+
+
+def test_save_config_preserves_template_when_env_rotates_after_load(monkeypatch, tmp_path):
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    monkeypatch.setenv("TU_ZI_API_KEY", "sk-old-secret")
+    _write_config(
+        tmp_path,
+        """\
+        custom_providers:
+          - name: tuzi
+            api_key: ${TU_ZI_API_KEY}
+            model: claude-opus-4-6
+        model:
+          default: claude-opus-4-6
+        """,
+    )
+
+    config = load_config()
+    monkeypatch.setenv("TU_ZI_API_KEY", "sk-rotated-secret")
+    config["model"]["default"] = "doubao-pro"
+    save_config(config)
+
+    saved = _read_config(tmp_path)
+    assert "api_key: ${TU_ZI_API_KEY}" in saved
+    assert "sk-old-secret" not in saved
+    assert "sk-rotated-secret" not in saved
+
+
+def test_save_config_keeps_edited_partial_template_strings_literal(monkeypatch, tmp_path):
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    monkeypatch.setenv("ALT_SECRET", "alt-secret")
+    _write_config(
+        tmp_path,
+        """\
+        custom_providers:
+          - name: tuzi
+            headers:
+              Authorization: Bearer ${ALT_SECRET}
+            model: claude-opus-4-6
+        model:
+          default: claude-opus-4-6
+        """,
+    )
+
+    config = load_config()
+    config["custom_providers"][0]["headers"]["Authorization"] = "Token alt-secret"
+    save_config(config)
+
+    saved = _read_config(tmp_path)
+    assert "Authorization: Token alt-secret" in saved
+    assert "Authorization: Bearer ${ALT_SECRET}" not in saved
+
+
+def test_save_config_falls_back_to_positional_matching_for_duplicate_names(monkeypatch, tmp_path):
+    monkeypatch.setenv("HERMES_HOME", str(tmp_path))
+    monkeypatch.setenv("FIRST_SECRET", "first-secret")
+    monkeypatch.setenv("SECOND_SECRET", "second-secret")
+    _write_config(
+        tmp_path,
+        """\
+        custom_providers:
+          - name: duplicate
+            api_key: ${FIRST_SECRET}
+            model: claude-opus-4-6
+          - name: duplicate
+            api_key: ${SECOND_SECRET}
+            model: doubao-pro
+        model:
+          default: claude-opus-4-6
+        """,
+    )
+
+    config = load_config()
+    config["display"]["compact"] = True
+    save_config(config)
+
+    saved = _read_config(tmp_path)
+    assert saved.count("name: duplicate") == 2
+    assert "api_key: ${FIRST_SECRET}" in saved
+    assert "api_key: ${SECOND_SECRET}" in saved
+    assert "first-secret" not in saved
+    assert "second-secret" not in saved