fix(secrets): only apply external secrets once per HERMES_HOME per process (#32271)
`load_hermes_dotenv()` is called at module-import time from cli.py, hermes_cli/main.py, run_agent.py, trajectory_compressor.py, gateway/run.py, tui_gateway/server.py, acp_adapter/entry.py, and a few others. Each call triggered `_apply_external_secret_sources()`, which re-parsed config, re-fetched from Bitwarden Secrets Manager (its own 300s cache mostly absorbed this), re-ran the ASCII sanitization sweep, and reprinted Bitwarden Secrets Manager: applied N secret(s) (...) to stderr. Users saw the status line 3-5x per CLI startup. Guard the function with a process-level set of HERMES_HOME paths that have already had external secrets applied. Subsequent calls for the same home_path are no-ops. `reset_secret_source_cache()` lets tests (and any future long-running consumer that wants to refresh after a config change) force a re-pull.
This commit is contained in:
@ -29,6 +29,15 @@ _WARNED_KEYS: set[str] = set()
|
|||||||
# the .env case and they don't know Bitwarden is wired up).
|
# the .env case and they don't know Bitwarden is wired up).
|
||||||
_SECRET_SOURCES: dict[str, str] = {}
|
_SECRET_SOURCES: dict[str, str] = {}
|
||||||
|
|
||||||
|
# HERMES_HOME paths we've already pulled external secrets for during this
|
||||||
|
# process. ``load_hermes_dotenv()`` is called at module-import time from
|
||||||
|
# several hot modules (cli.py, hermes_cli/main.py, run_agent.py,
|
||||||
|
# trajectory_compressor.py, gateway/run.py, ...), so without this guard the
|
||||||
|
# Bitwarden status line gets printed 3-5x per startup. Bitwarden's own
|
||||||
|
# in-process cache prevents redundant network calls, but the print, the
|
||||||
|
# config re-parse, and the ASCII sanitization sweep still ran every time.
|
||||||
|
_APPLIED_HOMES: set[str] = set()
|
||||||
|
|
||||||
|
|
||||||
def get_secret_source(env_var: str) -> str | None:
|
def get_secret_source(env_var: str) -> str | None:
|
||||||
"""Return the label of the secret source that supplied ``env_var``, if any.
|
"""Return the label of the secret source that supplied ``env_var``, if any.
|
||||||
@ -43,6 +52,19 @@ def get_secret_source(env_var: str) -> str | None:
|
|||||||
return _SECRET_SOURCES.get(env_var)
|
return _SECRET_SOURCES.get(env_var)
|
||||||
|
|
||||||
|
|
||||||
|
def reset_secret_source_cache() -> None:
|
||||||
|
"""Forget which HERMES_HOME paths have already had external secrets applied.
|
||||||
|
|
||||||
|
The first call to ``_apply_external_secret_sources(home_path)`` in a
|
||||||
|
process pulls from Bitwarden (or other configured backend), records the
|
||||||
|
applied keys in ``_SECRET_SOURCES``, and remembers ``home_path`` so
|
||||||
|
subsequent calls in the same process are no-ops. Call this to force the
|
||||||
|
next call to re-pull — useful for tests, and for long-running processes
|
||||||
|
that want to refresh after a config change.
|
||||||
|
"""
|
||||||
|
_APPLIED_HOMES.clear()
|
||||||
|
|
||||||
|
|
||||||
def format_secret_source_suffix(env_var: str) -> str:
|
def format_secret_source_suffix(env_var: str) -> str:
|
||||||
"""Return a human-readable suffix like ``" (from Bitwarden)"`` or ``""``.
|
"""Return a human-readable suffix like ``" (from Bitwarden)"`` or ``""``.
|
||||||
|
|
||||||
@ -232,7 +254,21 @@ def _apply_external_secret_sources(home_path: Path) -> None:
|
|||||||
locate the access token) but BEFORE the rest of Hermes reads
|
locate the access token) but BEFORE the rest of Hermes reads
|
||||||
``os.environ`` for credentials. Any failure here is logged and
|
``os.environ`` for credentials. Any failure here is logged and
|
||||||
swallowed — external secret sources must never block startup.
|
swallowed — external secret sources must never block startup.
|
||||||
|
|
||||||
|
Idempotent within a process: subsequent calls for the same
|
||||||
|
``home_path`` are no-ops. ``load_hermes_dotenv()`` runs at import
|
||||||
|
time from several hot modules (cli.py, hermes_cli/main.py,
|
||||||
|
run_agent.py, trajectory_compressor.py, ...), so without this guard
|
||||||
|
the Bitwarden status line would print 3-5x per CLI startup. Use
|
||||||
|
``reset_secret_source_cache()`` if you need to force a re-pull
|
||||||
|
(tests, future ``hermes secrets bitwarden sync`` from a long-running
|
||||||
|
process).
|
||||||
"""
|
"""
|
||||||
|
home_key = str(Path(home_path).resolve())
|
||||||
|
if home_key in _APPLIED_HOMES:
|
||||||
|
return
|
||||||
|
_APPLIED_HOMES.add(home_key)
|
||||||
|
|
||||||
try:
|
try:
|
||||||
cfg = _load_secrets_config(home_path)
|
cfg = _load_secrets_config(home_path)
|
||||||
except Exception: # noqa: BLE001 — config errors must not block startup
|
except Exception: # noqa: BLE001 — config errors must not block startup
|
||||||
|
|||||||
@ -22,10 +22,12 @@ from hermes_cli import env_loader # noqa: E402
|
|||||||
|
|
||||||
@pytest.fixture(autouse=True)
|
@pytest.fixture(autouse=True)
|
||||||
def _reset_sources():
|
def _reset_sources():
|
||||||
"""Each test starts with a clean source map."""
|
"""Each test starts with a clean source map and applied-home guard."""
|
||||||
env_loader._SECRET_SOURCES.clear()
|
env_loader._SECRET_SOURCES.clear()
|
||||||
|
env_loader.reset_secret_source_cache()
|
||||||
yield
|
yield
|
||||||
env_loader._SECRET_SOURCES.clear()
|
env_loader._SECRET_SOURCES.clear()
|
||||||
|
env_loader.reset_secret_source_cache()
|
||||||
|
|
||||||
|
|
||||||
def test_get_secret_source_returns_none_for_untracked_var():
|
def test_get_secret_source_returns_none_for_untracked_var():
|
||||||
@ -117,3 +119,57 @@ def test_apply_external_secret_sources_noop_when_disabled(tmp_path, monkeypatch)
|
|||||||
env_loader._apply_external_secret_sources(tmp_path)
|
env_loader._apply_external_secret_sources(tmp_path)
|
||||||
|
|
||||||
assert env_loader.get_secret_source("ANTHROPIC_API_KEY") is None
|
assert env_loader.get_secret_source("ANTHROPIC_API_KEY") is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_apply_external_secret_sources_dedupes_within_process(tmp_path, monkeypatch):
|
||||||
|
"""``load_hermes_dotenv()`` is called at module-import time from several
|
||||||
|
hot modules (cli.py, hermes_cli/main.py, run_agent.py, ...). The
|
||||||
|
Bitwarden status line previously printed once per call — 3-5x per
|
||||||
|
startup. The applied-home guard must short-circuit subsequent calls
|
||||||
|
so the heavy work (config re-parse, Bitwarden lookup, status print)
|
||||||
|
runs exactly once per HERMES_HOME per process.
|
||||||
|
"""
|
||||||
|
|
||||||
|
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||||
|
config_path = tmp_path / "config.yaml"
|
||||||
|
config_path.write_text(
|
||||||
|
"secrets:\n"
|
||||||
|
" bitwarden:\n"
|
||||||
|
" enabled: true\n"
|
||||||
|
" project_id: test-project\n"
|
||||||
|
" access_token_env: BWS_ACCESS_TOKEN\n",
|
||||||
|
encoding="utf-8",
|
||||||
|
)
|
||||||
|
|
||||||
|
from agent.secret_sources.bitwarden import FetchResult
|
||||||
|
|
||||||
|
call_count = {"n": 0}
|
||||||
|
|
||||||
|
def _fake_apply(**_kwargs):
|
||||||
|
call_count["n"] += 1
|
||||||
|
return FetchResult(
|
||||||
|
secrets={"ANTHROPIC_API_KEY": "sk-ant-test"},
|
||||||
|
applied=["ANTHROPIC_API_KEY"],
|
||||||
|
)
|
||||||
|
|
||||||
|
import agent.secret_sources.bitwarden as bw_module
|
||||||
|
monkeypatch.setattr(bw_module, "apply_bitwarden_secrets", _fake_apply)
|
||||||
|
|
||||||
|
# Five calls in a row, simulating module-import-time invocations from
|
||||||
|
# cli.py, hermes_cli/main.py, run_agent.py, trajectory_compressor.py,
|
||||||
|
# gateway/run.py. Only the first should actually call the backend.
|
||||||
|
for _ in range(5):
|
||||||
|
env_loader._apply_external_secret_sources(tmp_path)
|
||||||
|
|
||||||
|
assert call_count["n"] == 1, (
|
||||||
|
"Bitwarden backend was called {} time(s); expected exactly 1 — "
|
||||||
|
"the applied-home guard is broken.".format(call_count["n"])
|
||||||
|
)
|
||||||
|
|
||||||
|
# Source tracking still works after dedup.
|
||||||
|
assert env_loader.get_secret_source("ANTHROPIC_API_KEY") == "bitwarden"
|
||||||
|
|
||||||
|
# reset_secret_source_cache() forces a fresh pull on the next call.
|
||||||
|
env_loader.reset_secret_source_cache()
|
||||||
|
env_loader._apply_external_secret_sources(tmp_path)
|
||||||
|
assert call_count["n"] == 2
|
||||||
|
|||||||
Reference in New Issue
Block a user