Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion hermes_cli/env_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,17 @@
# config re-parse, and the ASCII sanitization sweep still ran every time.
_APPLIED_HOMES: set[str] = set()

# Cross-process dedup for the BSM status line. ``hermes`` startup spawns
# child Python processes (gateway, TUI server, ACP adapter, ...) that each
# call ``load_hermes_dotenv()`` at import time — _APPLIED_HOMES above is
# module-level state and does not survive a subprocess boundary, so without
# this marker users see the same "BWS_ACCESS_TOKEN is not set" warning 2-3x
# per startup (#32715). We set this env var when we first print the line;
# subprocesses inherit os.environ and skip the print. The work (config
# read, fetch attempt, env injection) still runs in each subprocess — only
# the duplicated stderr noise is suppressed.
_BWS_STATUS_PRINTED_ENV = "_HERMES_BWS_STATUS_PRINTED"


def get_secret_source(env_var: str) -> str | None:
"""Return the label of the secret source that supplied ``env_var``, if any.
Expand All @@ -63,6 +74,10 @@ def reset_secret_source_cache() -> None:
that want to refresh after a config change.
"""
_APPLIED_HOMES.clear()
# Also drop the cross-process print marker so the next call can emit the
# status line again (tests rely on this, and a long-running process that
# explicitly resets state probably wants the next attempt to be visible).
os.environ.pop(_BWS_STATUS_PRINTED_ENV, None)


def format_secret_source_suffix(env_var: str) -> str:
Expand Down Expand Up @@ -136,7 +151,7 @@ def _sanitize_loaded_credentials() -> None:
" This usually means the key was copy-pasted from a PDF, "
"rich-text editor, or web page that substituted lookalike\n"
" Unicode glyphs for ASCII letters. If authentication fails "
"(e.g. \"API key not valid\"), re-copy the key from the\n"
'(e.g. "API key not valid"), re-copy the key from the\n'
" provider's dashboard and run `hermes setup` (or edit the "
".env file in a plain-text editor).",
file=sys.stderr,
Expand Down Expand Up @@ -190,6 +205,7 @@ def _sanitize_env_file_if_needed(path: Path) -> None:
sanitized = _sanitize_env_lines(stripped)
if sanitized != original:
import tempfile

fd, tmp = tempfile.mkstemp(
dir=str(path.parent), suffix=".tmp", prefix=".env_"
)
Expand Down Expand Up @@ -305,6 +321,16 @@ def _apply_external_secret_sources(home_path: Path) -> None:
# came from BSM rather than .env.
for name in result.applied:
_SECRET_SOURCES[name] = "bitwarden"

# Cross-process print dedup: subprocesses inherit os.environ and skip
# re-emitting the same status line. Mark *before* printing so that even
# if multiple sibling subprocesses race past the check, only the first
# one wins — and tests can pre-set the marker to assert the suppression.
if os.environ.get(_BWS_STATUS_PRINTED_ENV):
return
os.environ[_BWS_STATUS_PRINTED_ENV] = "1"

if result.applied:
print(
f" Bitwarden Secrets Manager: applied {len(result.applied)} "
f"secret{'s' if len(result.applied) != 1 else ''} "
Expand Down
103 changes: 97 additions & 6 deletions tests/test_env_loader_secret_sources.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from __future__ import annotations

import os
import sys
from pathlib import Path

Expand Down Expand Up @@ -57,10 +58,7 @@ def test_format_secret_source_suffix_generic_label_for_future_sources():
# Future-proofing: a new secret source (e.g. "vault") should still
# produce a sensible label without needing to edit every call site.
env_loader._SECRET_SOURCES["OPENAI_API_KEY"] = "vault"
assert (
env_loader.format_secret_source_suffix("OPENAI_API_KEY")
== " (from vault)"
)
assert env_loader.format_secret_source_suffix("OPENAI_API_KEY") == " (from vault)"


def test_apply_external_secret_sources_records_bitwarden_origin(tmp_path, monkeypatch):
Expand Down Expand Up @@ -107,18 +105,110 @@ def _fake_apply(**_kwargs):
def test_apply_external_secret_sources_noop_when_disabled(tmp_path, monkeypatch):
"""Disabled Bitwarden config must not touch the source map."""

monkeypatch.setenv("HERMES_HOME", str(tmp_path))
config_path = tmp_path / "config.yaml"
config_path.write_text(
"secrets:\n bitwarden:\n enabled: false\n",
encoding="utf-8",
)

env_loader._apply_external_secret_sources(tmp_path)

assert env_loader.get_secret_source("ANTHROPIC_API_KEY") is None


def test_apply_external_secret_sources_dedupes_across_subprocesses(
tmp_path, monkeypatch, capsys
):
"""``hermes`` startup spawns child Python processes (gateway, TUI, ACP
adapter) that each call ``load_hermes_dotenv()`` at import time. The
in-process ``_APPLIED_HOMES`` guard doesn't survive a subprocess
boundary, so without the cross-process marker users saw the
"BWS_ACCESS_TOKEN is not set" warning 2-3x per startup (#32715).
A pre-set marker in ``os.environ`` (as a child would inherit from its
parent) must suppress the status line entirely.
"""

monkeypatch.setenv("HERMES_HOME", str(tmp_path))
config_path = tmp_path / "config.yaml"
config_path.write_text(
"secrets:\n"
" bitwarden:\n"
" enabled: false\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

def _fake_apply(**_kwargs):
# Mirror the real "BWS_ACCESS_TOKEN unset" failure mode the issue
# reports — that's the noise we're deduping.
return FetchResult(
error=(
"secrets.bitwarden.enabled is true but BWS_ACCESS_TOKEN is "
"not set. Run `hermes secrets bitwarden setup`."
)
)

import agent.secret_sources.bitwarden as bw_module

monkeypatch.setattr(bw_module, "apply_bitwarden_secrets", _fake_apply)

# Simulate a child process: parent already printed the warning and set
# the marker; the inherited environ carries it across the fork/spawn.
monkeypatch.setenv(env_loader._BWS_STATUS_PRINTED_ENV, "1")

env_loader._apply_external_secret_sources(tmp_path)

assert env_loader.get_secret_source("ANTHROPIC_API_KEY") is None
captured = capsys.readouterr()
assert "Bitwarden Secrets Manager" not in captured.err, (
"Cross-process dedup is broken: the status line printed even "
"though the parent process had already set "
f"{env_loader._BWS_STATUS_PRINTED_ENV}=1. Stderr was: {captured.err!r}"
)


def test_apply_external_secret_sources_prints_warning_once_then_sets_marker(
tmp_path, monkeypatch, capsys
):
"""First subprocess to hit the BWS_ACCESS_TOKEN-unset path must print
the warning *and* set the marker so its siblings stay quiet.
"""

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

err_text = (
"secrets.bitwarden.enabled is true but BWS_ACCESS_TOKEN is "
"not set. Run `hermes secrets bitwarden setup`."
)

def _fake_apply(**_kwargs):
return FetchResult(error=err_text)

import agent.secret_sources.bitwarden as bw_module

monkeypatch.setattr(bw_module, "apply_bitwarden_secrets", _fake_apply)

monkeypatch.delenv(env_loader._BWS_STATUS_PRINTED_ENV, raising=False)

env_loader._apply_external_secret_sources(tmp_path)

captured = capsys.readouterr()
assert err_text in captured.err
assert os.environ.get(env_loader._BWS_STATUS_PRINTED_ENV) == "1"


def test_apply_external_secret_sources_dedupes_within_process(tmp_path, monkeypatch):
Expand Down Expand Up @@ -153,6 +243,7 @@ def _fake_apply(**_kwargs):
)

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
Expand Down
Loading