diff --git a/hermes_cli/env_loader.py b/hermes_cli/env_loader.py index c5e95a24dbcf..082bc89e688b 100644 --- a/hermes_cli/env_loader.py +++ b/hermes_cli/env_loader.py @@ -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. @@ -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: @@ -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, @@ -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_" ) @@ -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 ''} " diff --git a/tests/test_env_loader_secret_sources.py b/tests/test_env_loader_secret_sources.py index 91c9d4c6e4f5..efc863c98d0d 100644 --- a/tests/test_env_loader_secret_sources.py +++ b/tests/test_env_loader_secret_sources.py @@ -7,6 +7,7 @@ from __future__ import annotations +import os import sys from pathlib import Path @@ -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): @@ -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): @@ -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