diff --git a/hermes_cli/config.py b/hermes_cli/config.py index f17c6a0a55059..ab54b596ef15c 100644 --- a/hermes_cli/config.py +++ b/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) diff --git a/tests/hermes_cli/test_config_env_ref_parity.py b/tests/hermes_cli/test_config_env_ref_parity.py new file mode 100644 index 0000000000000..be39849f68c28 --- /dev/null +++ b/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