feat(config): resolve ${env:VAR} SecretRefs in config.yaml, matching MCP config (#69267)

MCP server config already resolves Cursor-style ${env:VAR} references
(mcp_tool._env_ref_name); config.yaml's expander treated the same shape
as a literal string — a confusing half-support.  _expand_env_vars() now
strips the env: prefix and resolves identically, _env_ref_snapshot()
tracks the ref under the REAL var name (preserving the #58514 cache-
invalidation contract), and refs with a non-env source prefix
(bitwarden:/vault:/file:) warn with a pointer to the secrets: block
instead of being silently treated as a variable named 'bitwarden:FOO'.

Salvaged from PR #59516 — the audit-CLI half and the main() exit-code
change were out of scope and are not included.

Co-authored-by: andynguyendk <35395190+andynguyendk@users.noreply.github.com>

hermes_cli/config.py

@@ -6672,19 +6672,76 @@ def _strip_dotted_keys(cfg: dict, dotted_keys: set) -> Tuple[dict, set]:
     return cfg, stripped
 
 
+def _env_expand_match(m: re.Match) -> str:
+    """Expand one ``${...}`` config reference.
+
+    Two accepted shapes, matching what MCP server config already resolves
+    (``tools/mcp_tool.py::_env_ref_name``):
+
+    * ``${VAR}`` — legacy bare name, resolved via ``os.environ``.
+    * ``${env:VAR}`` — Cursor-style SecretRef, same resolution after the
+      ``env:`` prefix is stripped.  Before this, the prefixed form worked in
+      MCP config but stayed a literal string in config.yaml — a confusing
+      half-support.
+
+    Other SecretRef sources (``file:``, ``bitwarden:``, ``vault:``, ...)
+    are NOT resolved here — external secret backends inject their values
+    into the environment at startup (the ``secrets:`` block), so a config
+    ref only ever needs the env shape.  Unknown prefixes warn once and stay
+    verbatim so callers can detect them.
+    """
+    raw = m.group(0)
+    inner = m.group(1).strip()
+    if inner.startswith("env:"):
+        name = inner[len("env:"):].strip()
+        if not name:
+            return raw
+        val = os.environ.get(name)
+        if val is not None:
+            return val
+        logger.warning(
+            "Config ref %r: %s is not set (check ~/.hermes/.env); "
+            "keeping the literal placeholder", raw, name,
+        )
+        return raw
+    if ":" in inner and re.match(r"^[a-z][a-z0-9_-]*:", inner):
+        # Looks like a SecretRef with a non-env source.  Values from vault
+        # backends arrive via the secrets: block as env vars — point there
+        # instead of silently treating "bitwarden:FOO" as a var named
+        # "bitwarden:FOO".
+        logger.warning(
+            "Config ref %r uses source %r which is not resolvable in "
+            "config.yaml — external secret sources inject env vars at "
+            "startup, so reference the variable as ${env:NAME} instead",
+            raw, inner.split(":", 1)[0],
+        )
+        return raw
+    # Legacy ``${VAR}`` — bare name.
+    return os.environ.get(inner, raw)
+
+
+def _env_ref_var_name(ref: str) -> Optional[str]:
+    """Normalize a ``${...}`` body to the env-var name it reads, or None
+    when the ref uses a non-env source and never touches the environment."""
+    ref = ref.strip()
+    if ref.startswith("env:"):
+        name = ref[len("env:"):].strip()
+        return name or None
+    if ":" in ref and re.match(r"^[a-z][a-z0-9_-]*:", ref):
+        return None
+    return ref
+
+
 def _expand_env_vars(obj):
-    """Recursively expand ``${VAR}`` references in config values.
+    """Recursively expand ``${VAR}`` / ``${env:VAR}`` references in config
+    values.
 
     Only string values are processed; dict keys, numbers, booleans, and
     None are left untouched.  Unresolved references (variable not in
     ``os.environ``) are kept verbatim so callers can detect them.
     """
     if isinstance(obj, str):
-        return re.sub(
-            r"\${([^}]+)}",
-            lambda m: os.environ.get(m.group(1), m.group(0)),
-            obj,
-        )
+        return re.sub(r"\${([^}]+)}", _env_expand_match, obj)
     if isinstance(obj, dict):
         return {k: _expand_env_vars(v) for k, v in obj.items()}
     if isinstance(obj, list):
@@ -6693,8 +6750,8 @@ def _expand_env_vars(obj):
 
 
 def _env_ref_snapshot(obj, snapshot=None):
-    """Map every ``${VAR}`` name referenced in config values to its current
-    ``os.environ`` value (``None`` when unset).
+    """Map every ``${VAR}`` / ``${env:VAR}`` name referenced in config values
+    to its current ``os.environ`` value (``None`` when unset).
 
     Stored alongside cached ``load_config()`` results so a cache hit can
     detect that the cached expansion was made against a *different*
@@ -6702,12 +6759,18 @@ def _env_ref_snapshot(obj, snapshot=None):
     ``load_hermes_dotenv()`` populated the process env, or an env var
     rotated in-process after the first load. File mtime/size alone cannot
     see either case (#58514).
+
+    ``${env:VAR}`` refs are tracked under the real variable name; refs
+    with a non-env source prefix never read the environment, so they are
+    excluded from the snapshot.
     """
     if snapshot is None:
         snapshot = {}
     if isinstance(obj, str):
-        for name in re.findall(r"\${([^}]+)}", obj):
-            snapshot[name] = os.environ.get(name)
+        for raw in re.findall(r"\${([^}]+)}", obj):
+            name = _env_ref_var_name(raw)
+            if name is not None:
+                snapshot[name] = os.environ.get(name)
     elif isinstance(obj, dict):
         for value in obj.values():
             _env_ref_snapshot(value, snapshot)

tests/hermes_cli/test_config_env_ref_parity.py

@@ -0,0 +1,99 @@
+"""Config `${env:VAR}` SecretRef parity (salvaged from PR #59516).
+
+`${env:VAR}` already resolved in MCP server config (mcp_tool._env_ref_name);
+config.yaml's expander treated it as a literal.  These tests pin the parity
+plus the cache-snapshot tracking and the non-env-source warning behavior.
+"""
+from __future__ import annotations
+
+import pytest
+
+from hermes_cli.config import (
+    _env_ref_snapshot,
+    _env_ref_var_name,
+    _expand_env_vars,
+)
+
+
+def test_bare_ref_still_expands(monkeypatch):
+    monkeypatch.setenv("PARITY_VAR", "val-bare")
+    assert _expand_env_vars("x-${PARITY_VAR}-y") == "x-val-bare-y"
+
+
+def test_env_prefixed_ref_expands(monkeypatch):
+    monkeypatch.setenv("PARITY_VAR", "val-prefixed")
+    assert _expand_env_vars("${env:PARITY_VAR}") == "val-prefixed"
+
+
+def test_env_prefixed_ref_unset_stays_verbatim(monkeypatch):
+    monkeypatch.delenv("PARITY_MISSING", raising=False)
+    assert _expand_env_vars("${env:PARITY_MISSING}") == "${env:PARITY_MISSING}"
+
+
+def test_empty_env_ref_stays_verbatim():
+    assert _expand_env_vars("${env:}") == "${env:}"
+
+
+def test_non_env_source_stays_verbatim_with_warning(caplog):
+    import logging
+    with caplog.at_level(logging.WARNING, logger="hermes_cli.config"):
+        out = _expand_env_vars("${bitwarden:MY_KEY}")
+    assert out == "${bitwarden:MY_KEY}"
+    assert any("env:NAME" in r.message for r in caplog.records)
+
+
+def test_nested_structures_expand(monkeypatch):
+    monkeypatch.setenv("PARITY_VAR", "v")
+    cfg = {"a": ["${env:PARITY_VAR}", {"b": "${PARITY_VAR}"}], "n": 3}
+    out = _expand_env_vars(cfg)
+    assert out == {"a": ["v", {"b": "v"}], "n": 3}
+
+
+def test_value_containing_colon_is_not_a_source_ref(monkeypatch):
+    """URL-ish or uppercase-colon refs are legacy bare names, not sources —
+    only a lowercase ident prefix counts as a SecretRef source."""
+    monkeypatch.delenv("MY:WEIRD", raising=False)
+    # Uppercase before ':' → treated as a bare (unset) var, kept verbatim,
+    # no misleading source warning.
+    assert _expand_env_vars("${MY:WEIRD}") == "${MY:WEIRD}"
+
+
+# ---------------------------------------------------------------------------
+# _env_ref_var_name + snapshot tracking
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.parametrize("ref,expected", [
+    ("PLAIN_VAR", "PLAIN_VAR"),
+    ("env:PLAIN_VAR", "PLAIN_VAR"),
+    ("env: SPACED ", "SPACED"),
+    ("env:", None),
+    ("bitwarden:KEY", None),
+    ("vault:path/to/key", None),
+])
+def test_env_ref_var_name(ref, expected):
+    assert _env_ref_var_name(ref) == expected
+
+
+def test_snapshot_tracks_env_prefixed_under_real_name(monkeypatch):
+    monkeypatch.setenv("PARITY_SNAP", "s1")
+    snap = _env_ref_snapshot({"k": "${env:PARITY_SNAP}"})
+    assert snap == {"PARITY_SNAP": "s1"}
+
+
+def test_snapshot_excludes_non_env_sources(monkeypatch):
+    snap = _env_ref_snapshot({"k": "${bitwarden:KEY}", "j": "${PARITY_SNAP2}"})
+    assert "bitwarden:KEY" not in snap
+    assert "KEY" not in snap
+    assert "PARITY_SNAP2" in snap
+
+
+def test_snapshot_detects_rotation_for_env_prefixed(monkeypatch):
+    """The #58514 cache-invalidation contract must hold for ${env:VAR} refs:
+    the snapshot records the value under the REAL var name, so a rotation
+    changes the snapshot."""
+    monkeypatch.setenv("PARITY_ROT", "before")
+    snap1 = _env_ref_snapshot({"k": "${env:PARITY_ROT}"})
+    monkeypatch.setenv("PARITY_ROT", "after")
+    snap2 = _env_ref_snapshot({"k": "${env:PARITY_ROT}"})
+    assert snap1 != snap2