From 06a4546cbc5a5b231330082197e33012b1821725 Mon Sep 17 00:00:00 2001 From: Andrey Shiryaev <7615137+shoom1@users.noreply.github.com> Date: Sat, 18 Jul 2026 00:17:50 -0400 Subject: [PATCH] fix(settings): API key export overwrites env; drop multi-tenant docs claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding 7 (countered): export_api_keys_to_env() set provider env vars only when absent, so in a process with two settings instances the first manager's export silently pinned credentials for every later one — while config.py advertised SettingsContext for multi-tenant use. The reviewer's fix (pass credentials to every provider client) fights ADK 1.x: AnthropicLlm/Gemini construct SDK clients from env internally. Instead: - export now OVERWRITES env from the settings instance. This is precedence-consistent: the key fields bind only via their env alias (validation_alias, no populate_by_name — constructor kwargs and JSON files never set them), so divergence only means the process env changed after this instance loaded, and the configured value must win. Unset keys still leave the environment untouched. - config.py no longer claims multi-tenant isolation for SettingsContext; it documents that credentials are process-global env vars. Claude-Session: https://claude.ai/code/session_01SwkKXQpy3JLEqrPkQA8QRv --- src/agentic_cli/config.py | 8 ++++++- src/agentic_cli/workflow/settings.py | 20 +++++++++++++--- tests/test_config.py | 35 ++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 4 deletions(-) diff --git a/src/agentic_cli/config.py b/src/agentic_cli/config.py index 9287a8d..1d5da9f 100644 --- a/src/agentic_cli/config.py +++ b/src/agentic_cli/config.py @@ -10,11 +10,17 @@ set_settings(my_settings) settings = get_settings() - 2. Context-based (isolated contexts, multi-tenant): + 2. Context-based (isolated settings lookup, e.g. tests or per-request + overrides): with SettingsContext(my_settings): # Code here sees my_settings via get_settings() settings = get_settings() # Returns my_settings + Note: isolation covers settings *lookup* only. API credentials are + exported to process-global env vars at manager initialization + (provider SDKs read them from the environment), so a SettingsContext + does not isolate credentials between contexts in one process. + Settings Loading Priority (highest to lowest): 1. Environment variables (AGENTIC_* prefix) 2. Project config (./.{app_name}/settings.json) diff --git a/src/agentic_cli/workflow/settings.py b/src/agentic_cli/workflow/settings.py index a154a86..852970d 100644 --- a/src/agentic_cli/workflow/settings.py +++ b/src/agentic_cli/workflow/settings.py @@ -652,11 +652,25 @@ def set_thinking_effort(self, effort: str) -> None: object.__setattr__(self, "thinking_effort", effort) def export_api_keys_to_env(self) -> None: - """Export API keys to environment variables.""" + """Export configured API keys to provider environment variables. + + Provider SDKs used by the orchestrators (ADK's AnthropicLlm/Gemini, + LangChain clients) read credentials from process env vars. The export + OVERWRITES the env from this settings instance: the key fields bind + only via their env alias (real env vars are the highest-priority + source), so settings and env diverge only when the process env + changed after this instance loaded — e.g. an earlier manager's + export, or a key loaded from a class-specific env_file — and then + this instance's configured value must win. (The previous + set-if-absent guard let the first exporting manager pin credentials + for every later one.) A key unset in settings leaves the environment + untouched. Credentials are process-global; SettingsContext does not + isolate them. + """ import os - if self.google_api_key and not os.environ.get("GOOGLE_API_KEY"): + if self.google_api_key: os.environ["GOOGLE_API_KEY"] = self.google_api_key - if self.anthropic_api_key and not os.environ.get("ANTHROPIC_API_KEY"): + if self.anthropic_api_key: os.environ["ANTHROPIC_API_KEY"] = self.anthropic_api_key diff --git a/tests/test_config.py b/tests/test_config.py index 054733e..a1338ef 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -248,6 +248,41 @@ def test_export_api_keys_to_env(self, temp_workspace: Path): else: os.environ.pop("GOOGLE_API_KEY", None) + def test_export_overwrites_stale_env_key(self, temp_workspace: Path, monkeypatch): + """Export re-asserts this instance's key over a mutated env var. + + The key fields bind only via their env alias, so settings and env + diverge when the process env changed after this instance loaded — + exactly what an earlier manager's export does in a two-settings + process. The previous set-if-absent guard then silently kept the + first manager's credentials for every later one. + """ + monkeypatch.setenv("ANTHROPIC_API_KEY", "current-key") + settings = BaseSettings(workspace_dir=temp_workspace) + assert settings.anthropic_api_key == "current-key" + + # Another settings instance exported its key in the meantime + monkeypatch.setenv("ANTHROPIC_API_KEY", "stale-key") + + settings.export_api_keys_to_env() + + assert os.environ["ANTHROPIC_API_KEY"] == "current-key" + + def test_export_leaves_env_when_setting_unset( + self, temp_workspace: Path, monkeypatch, tmp_path: Path + ): + """A key unset in settings never clobbers an externally-set env var.""" + monkeypatch.setenv("HOME", str(tmp_path)) # no user config leakage + monkeypatch.delenv("GOOGLE_API_KEY", raising=False) + settings = BaseSettings(workspace_dir=temp_workspace) + assert settings.google_api_key is None + + monkeypatch.setenv("GOOGLE_API_KEY", "external-key") + + settings.export_api_keys_to_env() + + assert os.environ["GOOGLE_API_KEY"] == "external-key" + class TestSettingsContext: """Tests for context-based settings management."""