diff --git a/agent/credential_pool.py b/agent/credential_pool.py index b02514e990..0d9776a397 100644 --- a/agent/credential_pool.py +++ b/agent/credential_pool.py @@ -1158,10 +1158,22 @@ def _seed_from_singletons(provider: str, entries: List[PooledCredential]) -> Tup def _seed_from_env(provider: str, entries: List[PooledCredential]) -> Tuple[bool, Set[str]]: changed = False active_sources: Set[str] = set() + # Honour user suppression — `hermes auth remove ` for an + # env-seeded credential marks the env: source as suppressed so it + # won't be re-seeded from the user's shell environment or ~/.hermes/.env. + # Without this gate the removal is silently undone on the next + # load_pool() call whenever the var is still exported by the shell. + try: + from hermes_cli.auth import is_source_suppressed as _is_source_suppressed + except ImportError: + def _is_source_suppressed(_p, _s): # type: ignore[misc] + return False if provider == "openrouter": token = os.getenv("OPENROUTER_API_KEY", "").strip() if token: source = "env:OPENROUTER_API_KEY" + if _is_source_suppressed(provider, source): + return changed, active_sources active_sources.add(source) changed |= _upsert_entry( entries, @@ -1198,6 +1210,8 @@ def _seed_from_env(provider: str, entries: List[PooledCredential]) -> Tuple[bool if not token: continue source = f"env:{env_var}" + if _is_source_suppressed(provider, source): + continue active_sources.add(source) auth_type = AUTH_TYPE_OAUTH if provider == "anthropic" and not token.startswith("sk-ant-api") else AUTH_TYPE_API_KEY base_url = env_url or pconfig.inference_base_url diff --git a/hermes_cli/auth_commands.py b/hermes_cli/auth_commands.py index 30e5182949..4fe5f3f2e4 100644 --- a/hermes_cli/auth_commands.py +++ b/hermes_cli/auth_commands.py @@ -152,6 +152,22 @@ def auth_add_command(args) -> None: pool = load_pool(provider) + # Clear any env: suppressions for this provider — re-adding a + # credential is a strong signal the user wants auth for this provider + # re-enabled. Matches the Codex device_code re-link pattern below. + if not provider.startswith(CUSTOM_POOL_PREFIX): + try: + from hermes_cli.auth import ( + _load_auth_store, + unsuppress_credential_source, + ) + suppressed = _load_auth_store().get("suppressed_sources", {}) + for src in list(suppressed.get(provider, []) or []): + if src.startswith("env:"): + unsuppress_credential_source(provider, src) + except Exception: + pass + if requested_type == AUTH_TYPE_API_KEY: token = (getattr(args, "api_key", None) or "").strip() if not token: @@ -339,14 +355,56 @@ def auth_remove_command(args) -> None: print(f"Removed {provider} credential #{index} ({removed.label})") # If this was an env-seeded credential, also clear the env var from .env - # so it doesn't get re-seeded on the next load_pool() call. + # so it doesn't get re-seeded on the next load_pool() call. If the env + # var is also (or only) exported by the user's shell/systemd, .env + # cleanup alone is not enough — the next process to call load_pool() + # will re-read os.environ and resurrect the entry. Suppress the + # env: source so _seed_from_env() skips it, and tell the user + # where the shell-level copy is still living so they can remove it. if removed.source.startswith("env:"): + import os as _os env_var = removed.source[len("env:"):] if env_var: - from hermes_cli.config import remove_env_value + from hermes_cli.config import get_env_path, remove_env_value + from hermes_cli.auth import suppress_credential_source + + # Detect whether the var lives in .env, the shell env, or both, + # BEFORE remove_env_value() mutates os.environ. + env_in_process = bool(_os.getenv(env_var)) + env_in_dotenv = False + try: + env_path = get_env_path() + if env_path.exists(): + env_in_dotenv = any( + line.strip().startswith(f"{env_var}=") + for line in env_path.read_text(errors="replace").splitlines() + ) + except OSError: + pass + shell_exported = env_in_process and not env_in_dotenv + cleared = remove_env_value(env_var) if cleared: print(f"Cleared {env_var} from .env") + suppress_credential_source(provider, removed.source) + if shell_exported: + print( + f"Note: {env_var} is still set in your shell environment " + f"(not in ~/.hermes/.env)." + ) + print( + " Unset it there (shell profile, systemd EnvironmentFile, " + "launchd plist, etc.) or it will keep being visible to Hermes." + ) + print( + f" The pool entry is now suppressed — Hermes will ignore " + f"{env_var} until you run `hermes auth add {provider}`." + ) + else: + print( + f"Suppressed env:{env_var} — it will not be re-seeded even " + f"if the variable is re-exported later." + ) # If this was a singleton-seeded credential (OAuth device_code, hermes_pkce), # clear the underlying auth store / credential file so it doesn't get diff --git a/tests/hermes_cli/test_auth_commands.py b/tests/hermes_cli/test_auth_commands.py index 5b0d9062b9..a017185573 100644 --- a/tests/hermes_cli/test_auth_commands.py +++ b/tests/hermes_cli/test_auth_commands.py @@ -1011,3 +1011,177 @@ def test_seed_from_singletons_respects_codex_suppression(tmp_path, monkeypatch): # Verify the auth store was NOT modified (no auto-import happened) after = json.loads((hermes_home / "auth.json").read_text()) assert "openai-codex" not in after.get("providers", {}) + + +def test_auth_remove_env_seeded_suppresses_shell_exported_var(tmp_path, monkeypatch, capsys): + """`hermes auth remove xai 1` must stick even when the env var is exported + by the shell (not written into ~/.hermes/.env). Before PR for #13371 the + removal silently restored on next load_pool() because _seed_from_env() + re-read os.environ. Now env: is suppressed in auth.json. + """ + hermes_home = tmp_path / "hermes" + hermes_home.mkdir(parents=True, exist_ok=True) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + + # Simulate shell export (NOT written to .env) + monkeypatch.setenv("XAI_API_KEY", "sk-xai-shell-export") + (hermes_home / ".env").write_text("") + + _write_auth_store( + tmp_path, + { + "version": 1, + "credential_pool": { + "xai": [{ + "id": "env-1", + "label": "XAI_API_KEY", + "auth_type": "api_key", + "priority": 0, + "source": "env:XAI_API_KEY", + "access_token": "sk-xai-shell-export", + "base_url": "https://api.x.ai/v1", + }] + }, + }, + ) + + from types import SimpleNamespace + from hermes_cli.auth_commands import auth_remove_command + auth_remove_command(SimpleNamespace(provider="xai", target="1")) + + # Suppression marker written + after = json.loads((hermes_home / "auth.json").read_text()) + assert "env:XAI_API_KEY" in after.get("suppressed_sources", {}).get("xai", []) + + # Diagnostic printed pointing at the shell + out = capsys.readouterr().out + assert "still set in your shell environment" in out + assert "Cleared XAI_API_KEY from .env" not in out # wasn't in .env + + # Fresh simulation: shell re-exports, reload pool + monkeypatch.setenv("XAI_API_KEY", "sk-xai-shell-export") + from agent.credential_pool import load_pool + pool = load_pool("xai") + assert not pool.has_credentials(), "pool must stay empty — env:XAI_API_KEY suppressed" + + +def test_auth_remove_env_seeded_dotenv_only_no_shell_hint(tmp_path, monkeypatch, capsys): + """When the env var lives only in ~/.hermes/.env (not the shell), the + shell-hint should NOT be printed — avoid scaring the user about a + non-existent shell export. + """ + hermes_home = tmp_path / "hermes" + hermes_home.mkdir(parents=True, exist_ok=True) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + + # Key ONLY in .env, shell must not have it + monkeypatch.delenv("DEEPSEEK_API_KEY", raising=False) + (hermes_home / ".env").write_text("DEEPSEEK_API_KEY=sk-ds-only\n") + # Mimic load_env() populating os.environ + monkeypatch.setenv("DEEPSEEK_API_KEY", "sk-ds-only") + + _write_auth_store( + tmp_path, + { + "version": 1, + "credential_pool": { + "deepseek": [{ + "id": "env-1", + "label": "DEEPSEEK_API_KEY", + "auth_type": "api_key", + "priority": 0, + "source": "env:DEEPSEEK_API_KEY", + "access_token": "sk-ds-only", + }] + }, + }, + ) + + from types import SimpleNamespace + from hermes_cli.auth_commands import auth_remove_command + auth_remove_command(SimpleNamespace(provider="deepseek", target="1")) + + out = capsys.readouterr().out + assert "Cleared DEEPSEEK_API_KEY from .env" in out + assert "still set in your shell environment" not in out + assert (hermes_home / ".env").read_text().strip() == "" + + +def test_auth_add_clears_env_suppression_for_provider(tmp_path, monkeypatch): + """Re-adding a credential via `hermes auth add ` clears any + env: suppression marker — strong signal the user wants auth back. + Matches the Codex device_code re-link behaviour. + """ + hermes_home = tmp_path / "hermes" + hermes_home.mkdir(parents=True, exist_ok=True) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + monkeypatch.delenv("XAI_API_KEY", raising=False) + + _write_auth_store( + tmp_path, + { + "version": 1, + "providers": {}, + "suppressed_sources": {"xai": ["env:XAI_API_KEY"]}, + }, + ) + + from types import SimpleNamespace + from hermes_cli.auth import is_source_suppressed + from hermes_cli.auth_commands import auth_add_command + + assert is_source_suppressed("xai", "env:XAI_API_KEY") is True + auth_add_command(SimpleNamespace( + provider="xai", auth_type="api_key", + api_key="sk-xai-manual", label="manual", + )) + assert is_source_suppressed("xai", "env:XAI_API_KEY") is False + + +def test_seed_from_env_respects_env_suppression(tmp_path, monkeypatch): + """_seed_from_env() must skip env: sources that the user suppressed + via `hermes auth remove`. This is the gate that prevents shell-exported + keys from resurrecting removed credentials. + """ + hermes_home = tmp_path / "hermes" + hermes_home.mkdir(parents=True, exist_ok=True) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + monkeypatch.setenv("XAI_API_KEY", "sk-xai-shell-export") + + (hermes_home / "auth.json").write_text(json.dumps({ + "version": 1, + "providers": {}, + "suppressed_sources": {"xai": ["env:XAI_API_KEY"]}, + })) + + from agent.credential_pool import _seed_from_env + + entries = [] + changed, active = _seed_from_env("xai", entries) + assert changed is False + assert entries == [] + assert active == set() + + +def test_seed_from_env_respects_openrouter_suppression(tmp_path, monkeypatch): + """OpenRouter is the special-case branch in _seed_from_env; verify it + honours suppression too. + """ + hermes_home = tmp_path / "hermes" + hermes_home.mkdir(parents=True, exist_ok=True) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + monkeypatch.setenv("OPENROUTER_API_KEY", "sk-or-shell-export") + + (hermes_home / "auth.json").write_text(json.dumps({ + "version": 1, + "providers": {}, + "suppressed_sources": {"openrouter": ["env:OPENROUTER_API_KEY"]}, + })) + + from agent.credential_pool import _seed_from_env + + entries = [] + changed, active = _seed_from_env("openrouter", entries) + assert changed is False + assert entries == [] + assert active == set()