From ed89a9f883f81e2f3519616ee977850f2a9fe208 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Thu, 27 Aug 2026 04:25:50 -0700 Subject: [PATCH] feat(provider): reuse-or-separate credential flow for multi-instance providers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When adding a second instance of the same provider type, the wizard's env-var-collision check unconditionally forced a distinct credential name, with no way to intentionally share a credential across instances (e.g. two Anthropic instances -- one pinned to Opus, one to Sonnet -- that both authenticate with the same account/key). This offers a reuse-or-separate choice instead, defaulting to reuse: - Reuse (default): binds the new instance to the SAME ${ENV_VAR} as the existing instance. The secret field is never prompted for or overwritten -- only the placeholder is persisted, so both instances resolve the same stored credential at runtime. - Separate: derives/validates a distinct env var (unchanged from today's collision flow) and may be left blank to persist the placeholder UNSET for runtime injection (shell / CI / DTU passthrough), or populated through the existing secure-storage flow (KeyManager.save_key), unchanged. - Editing an existing instance recovers its binding from the stored placeholder, including "shared" when another configured instance still references the same env var -- the reuse invariant survives edits, not just the initial add. - Non-interactive mode carries the same semantics: an explicit binding (shared or separate) is exempt from the existing collision fail-loud guard, since it is intentional rather than an accidental default-name collision. - Runtime hardening: a provider instance's required secret placeholder that resolves to unset now fails loudly before session mount, instead of silently expanding to "" and letting the provider module fall back to its own ambient credential (which would route a "separate" instance through a DIFFERENT account's key). Optional/keyless secret fields are unaffected. No settings schema, amplifier-core, or provider-module contract changes -- this is app-CLI wizard/runtime policy only. Tests cover all four spec paths (shared / separate / unset-for-runtime- injection / edit-preservation) plus the non-interactive parity and runtime fallback-hardening cases. Full suite: 1469 passed, 1 skipped, 13 deselected, 1 xfailed. This design was independently validated across 10 reference implementations produced in an internal first-turn evaluation of this exact issue; this change follows the most consistent design among them (a named CredentialDecision/binding-mode contract) and additionally folds in the runtime unset-fallback hardening one of those implementations added after adversarial review. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_app_cli/commands/provider.py | 193 +++++++-- amplifier_app_cli/provider_config_utils.py | 88 +++- amplifier_app_cli/runtime/config.py | 83 ++++ tests/test_provider_instance_credentials.py | 456 +++++++++++++++++++- tests/test_runtime_credential_validation.py | 276 ++++++++++++ 5 files changed, 1030 insertions(+), 66 deletions(-) create mode 100644 tests/test_runtime_credential_validation.py diff --git a/amplifier_app_cli/commands/provider.py b/amplifier_app_cli/commands/provider.py index c3269d90..6d27725b 100644 --- a/amplifier_app_cli/commands/provider.py +++ b/amplifier_app_cli/commands/provider.py @@ -3,7 +3,7 @@ import os import re import time -from typing import Any, cast +from typing import Any, NamedTuple, cast import click from rich.console import Console @@ -162,25 +162,50 @@ def _find_claimed_env_var_owner(settings: AppSettings, env_var: str) -> str: return "an existing instance" -def _prompt_env_var_collision( - settings: AppSettings, +class CredentialDecision(NamedTuple): + """How a provider instance binds to its credential env var. + + ``env_var_overrides``: ``{type_default_env_var: instance_env_var}`` rename + map threaded into ``configure_provider`` (empty ⇒ use the type default). + + ``binding_modes``: ``{resolved_env_var: "shared" | "separate"}`` threaded + into ``configure_provider`` so a *shared* credential is never prompted + for or overwritten, and a *separate* one may be left unset for runtime + injection. Empty ⇒ today's behavior (no per-instance binding decision). + """ + + env_var_overrides: dict[str, str] + binding_modes: dict[str, str] + + +def _count_instances_using_env_var(settings: AppSettings, env_var: str) -> int: + """Number of configured provider instances (across all merged scopes) + whose config references ``${env_var}``. ``>= 2`` means the credential is + genuinely shared, so editing any one of them must preserve the shared + binding rather than overwrite the common secret. + """ + placeholder = f"${{{env_var}}}" + count = 0 + for p in settings.get_provider_overrides(): + config = p.get("config", {}) + if isinstance(config, dict) and placeholder in config.values(): + count += 1 + return count + + +def _prompt_separate_env_var( key_manager: KeyManager, module_id: str, instance_id: str | None, default_name: str, claimed: set[str], ) -> str: - """Interactive resolution for a same-default-name credential collision - (design §5.2 step 4). Returns the chosen, validated, unclaimed env var - name for this instance. Raises (EOFError, KeyboardInterrupt) on cancel. + """Derive, prefill, prompt for and validate a *distinct* credential env + var for a same-type instance that opted for a separate credential + (design §5.2 step 4). Returns the chosen, validated, unclaimed name. + Raises (EOFError, KeyboardInterrupt) on cancel, or ValueError if the id + can't produce a usable, non-colliding suggestion (§5.4.2). """ - owner = _find_claimed_env_var_owner(settings, default_name) - console.print( - f"\n [yellow]{module_id} already uses {default_name} " - f"(instance '{owner}'). This instance needs its own credential " - f"source.[/yellow]" - ) - suggested = _suggest_instance_env_var(module_id, instance_id or module_id, claimed) # Exact-name prefill only (design §5.4.3) -- a single os.environ.get() @@ -225,18 +250,72 @@ def _prompt_env_var_collision( return chosen_name +def _prompt_credential_binding( + settings: AppSettings, + key_manager: KeyManager, + module_id: str, + instance_id: str | None, + default_name: str, + claimed: set[str], +) -> CredentialDecision: + """Interactive fork for a same-type instance whose type-default + credential env var is already claimed: reuse the existing credential + (default) or configure a separate one. + + * Reuse -> bind to ``default_name``; never prompt for or overwrite its + value (``binding_modes = {default_name: "shared"}``). + * Separate -> derive/validate a distinct name and rename to it + (``env_var_overrides = {default_name: chosen}``, + ``binding_modes = {chosen: "separate"}``). + + Raises (EOFError, KeyboardInterrupt) on cancel, or ValueError if the id + can't produce a usable, non-colliding suggestion for the separate path. + """ + owner = _find_claimed_env_var_owner(settings, default_name) + console.print( + f"\n [bold]Another '{module_id}' instance already uses " + f"{default_name} (instance '{owner}').[/bold]" + ) + console.print(" How should this instance authenticate?") + console.print( + f" [1] Reuse {default_name} -- same credential, different " + f"model/settings [dim](default)[/dim]" + ) + console.print( + " [2] Separate credential -- its own env var (different " + "account/org/key)" + ) + choice = Prompt.ask(" Choice", choices=["1", "2"], default="1") + + if choice == "1": + # Reuse: bind to the existing credential; the secret prompt is + # skipped entirely and the stored value is left untouched. + console.print(f" [dim]This instance will reuse {default_name}.[/dim]") + return CredentialDecision({}, {default_name: "shared"}) + + chosen_name = _prompt_separate_env_var( + key_manager, module_id, instance_id, default_name, claimed + ) + return CredentialDecision({default_name: chosen_name}, {chosen_name: "separate"}) + + def _resolve_env_var_overrides( settings: AppSettings, key_manager: KeyManager, module_id: str, instance_id: str | None, -) -> dict[str, str]: - """Resolve the `env_var_overrides` map for a new provider instance - (design §5.2). Empty dict means "use the type default" -- no collision - detected. May raise (EOFError, KeyboardInterrupt) on user cancel, or - ValueError if the instance id cannot produce a usable, non-colliding - suggestion (design §5.4.2) -- caller decides how to react (re-prompt vs. - exit). +) -> CredentialDecision: + """Resolve the credential binding for a NEW provider instance (design + §5.2). Returns an empty decision when no collision is detected -- the + first instance of a type keeps today's no-extra-prompt UX. When the + type-default name is already claimed, forks to reuse-vs-separate via + ``_prompt_credential_binding`` (offer to reuse -- the default -- or + configure a separate credential). + + May raise (EOFError, KeyboardInterrupt) on user cancel, or ValueError if + the instance id cannot produce a usable, non-colliding suggestion for a + separate credential (design §5.4.2) -- caller decides how to react + (re-prompt vs. exit). Collision detection asks "does another *configured instance* already own this name?", so it uses ``_config_claimed_env_vars`` (${VAR} references @@ -252,38 +331,56 @@ def _resolve_env_var_overrides( claimed = _config_claimed_env_vars(settings) default_name = _secret_env_var_for(module_id) if not default_name or default_name not in claimed: - return {} + return CredentialDecision({}, {}) - chosen_name = _prompt_env_var_collision( + return _prompt_credential_binding( settings, key_manager, module_id, instance_id, default_name, claimed ) - return {default_name: chosen_name} def _recover_env_var_override( - module_id: str, existing_config: dict[str, Any] | None -) -> dict[str, str]: - """Recover an instance's existing credential env var from its stored + settings: AppSettings, + module_id: str, + instance_id: str | None, + existing_config: dict[str, Any] | None, +) -> CredentialDecision: + """Recover an instance's existing credential binding from its stored placeholder so re-configuring it doesn't reset to the type default and silently re-collide with another instance (design §5.3 -- the "must not miss" fix). + + If the recovered env var is shared by another configured instance, mark + it ``"shared"`` so editing this instance never prompts for or overwrites + the common secret (the reuse invariant must survive edits, not just the + initial add). """ if not existing_config: - return {} + return CredentialDecision({}, {}) default_name = _secret_env_var_for(module_id) field_id = _secret_field_id_for(module_id) if not default_name or not field_id: - return {} + return CredentialDecision({}, {}) raw_value = existing_config.get(field_id) - if ( + if not ( isinstance(raw_value, str) and raw_value.startswith("${") and raw_value.endswith("}") ): - current_name = raw_value[2:-1] - if current_name and current_name != default_name: - return {default_name: current_name} - return {} + return CredentialDecision({}, {}) + + current_name = raw_value[2:-1] + if not current_name: + return CredentialDecision({}, {}) + + overrides: dict[str, str] = {} + if current_name != default_name: + overrides = {default_name: current_name} + + binding_modes: dict[str, str] = {} + if _count_instances_using_env_var(settings, current_name) >= 2: + binding_modes = {current_name: "shared"} + + return CredentialDecision(overrides, binding_modes) @click.group() @@ -479,7 +576,7 @@ def provider_add(ctx: click.Context, provider_type: str | None, scope: str) -> N # name in one pass. key_manager = KeyManager() try: - env_var_overrides = _resolve_env_var_overrides( + credential = _resolve_env_var_overrides( settings, key_manager, module_id, instance_id ) except ValueError as e: @@ -494,7 +591,8 @@ def provider_add(ctx: click.Context, provider_type: str | None, scope: str) -> N config = configure_provider( module_id, key_manager, - env_var_overrides=env_var_overrides, + env_var_overrides=credential.env_var_overrides, + credential_binding_modes=credential.binding_modes, settings=settings, ) except (click.Abort, click.ClickException): @@ -819,8 +917,11 @@ def provider_edit(name: str, scope: str) -> None: # Bug 3 (design §5.3, "must not miss"): recover the instance's existing # credential env var from its stored placeholder so re-configuring it # doesn't reset to the type default and silently re-collide. - env_var_overrides = _recover_env_var_override( - module_id, existing_config if isinstance(existing_config, dict) else None + credential = _recover_env_var_override( + settings, + module_id, + entry.get("id"), + existing_config if isinstance(existing_config, dict) else None, ) # Run configure_provider with existing config as defaults @@ -829,7 +930,8 @@ def provider_edit(name: str, scope: str) -> None: module_id, key_manager, existing_config=existing_config, - env_var_overrides=env_var_overrides, + env_var_overrides=credential.env_var_overrides, + credential_binding_modes=credential.binding_modes, settings=settings, ) @@ -1251,7 +1353,7 @@ def _manage_add_provider(settings: AppSettings, scope: Scope = "global") -> None # name in one pass. key_manager = KeyManager() try: - env_var_overrides = _resolve_env_var_overrides( + credential = _resolve_env_var_overrides( settings, key_manager, module_id, instance_id ) except ValueError as e: @@ -1266,7 +1368,8 @@ def _manage_add_provider(settings: AppSettings, scope: Scope = "global") -> None config = configure_provider( module_id, key_manager, - env_var_overrides=env_var_overrides, + env_var_overrides=credential.env_var_overrides, + credential_binding_modes=credential.binding_modes, settings=settings, ) except (click.Abort, KeyboardInterrupt, EOFError): @@ -1363,8 +1466,11 @@ def _manage_edit_provider( # Bug 3 (design §5.3, "must not miss"): recover the instance's existing # credential env var from its stored placeholder so re-configuring it # doesn't reset to the type default and silently re-collide. - env_var_overrides = _recover_env_var_override( - module_id, existing_config if isinstance(existing_config, dict) else None + credential = _recover_env_var_override( + settings, + module_id, + entry.get("id"), + existing_config if isinstance(existing_config, dict) else None, ) key_manager = KeyManager() @@ -1373,7 +1479,8 @@ def _manage_edit_provider( module_id, key_manager, existing_config=existing_config, - env_var_overrides=env_var_overrides, + env_var_overrides=credential.env_var_overrides, + credential_binding_modes=credential.binding_modes, settings=settings, ) except (click.Abort, KeyboardInterrupt, EOFError): diff --git a/amplifier_app_cli/provider_config_utils.py b/amplifier_app_cli/provider_config_utils.py index 400703d3..61223157 100644 --- a/amplifier_app_cli/provider_config_utils.py +++ b/amplifier_app_cli/provider_config_utils.py @@ -506,6 +506,7 @@ def _prompt_for_field( collected_config: dict[str, Any], existing_config: dict[str, Any] | None = None, env_var_overrides: dict[str, str] | None = None, + credential_binding_modes: dict[str, str] | None = None, ) -> tuple[str, Any]: """Prompt user for a single config field value. @@ -518,6 +519,22 @@ def _prompt_for_field( instance_env_var}``, resolved by the caller (design §5.2/§5.3) when a same-type instance needs a distinct credential name from the provider type's declared default. + credential_binding_modes: Optional map of ``{resolved_env_var: mode}`` + where ``mode`` is ``"shared"`` or ``"separate"``. Resolved by the + caller when adding/editing a same-type instance: + + * ``"shared"`` -- this instance reuses an existing instance's + credential env var. The secret field is NEVER prompted for or + overwritten; only its ``${VAR}`` placeholder is persisted, so + both instances resolve the same secret at runtime. + * ``"separate"`` -- this instance owns a distinct credential env + var. It is prompted as usual, but may be left blank to persist + the placeholder UNSET for runtime injection (shell / CI / DTU + passthrough) instead of hard-failing on a required secret. + + Mechanism only: this function does not decide the mode, it just + honors whatever it is handed. Fields with no matching entry keep + today's behavior exactly (fully backward compatible). Returns: Tuple of (field_id, value) @@ -531,6 +548,7 @@ def _prompt_for_field( if declared_env_var else declared_env_var ) + binding_mode = (credential_binding_modes or {}).get(env_var) if env_var else None default = field.get("default") required = field.get("required", True) @@ -592,6 +610,17 @@ def _prompt_for_field( # No choices defined, fall through to text if field_type == "secret": + # Shared credential binding: this instance reuses another instance's + # credential env var. Never prompt for or overwrite its stored value + # -- just persist the placeholder so both instances resolve the same + # secret at runtime. + if env_var and binding_mode == "shared": + console.print( + f" [dim]Reusing existing credential {env_var}; its stored " + f"value is left untouched.[/dim]" + ) + return field_id, f"${{{env_var}}}" + prompt_suffix = " (press Enter to keep existing)" if existing_value else "" value = Prompt.ask(f"{prompt_text}{prompt_suffix}", password=True, default="") @@ -606,6 +635,17 @@ def _prompt_for_field( if existing_value: console.print("[green]✓ Using existing[/green]") return field_id, f"${{{env_var}}}" if env_var else existing_value + # Separate credential binding left blank: persist the placeholder + # UNSET so the value can be injected from the environment at runtime + # (shell / CI / DTU passthrough) instead of hard-failing. Scoped to an + # explicit per-instance binding -- a first-instance/normal required + # secret still errors, preserving today's setup guardrail. + if env_var and binding_mode == "separate": + console.print( + f" [yellow]No value entered; {env_var} will be resolved from " + f"the environment at runtime.[/yellow]" + ) + return field_id, f"${{{env_var}}}" if required: console.print("[red]Error: Required field[/red]") raise ValueError(f"{field['display_name']} is required") @@ -640,6 +680,7 @@ def configure_provider( existing_config: dict[str, Any] | None = None, non_interactive: bool = False, env_var_overrides: dict[str, str] | None = None, + credential_binding_modes: dict[str, str] | None = None, settings: AppSettings | None = None, ) -> dict[str, Any] | None: """Configure a provider using its self-declared config_fields. @@ -664,6 +705,11 @@ def configure_provider( when a same-type instance needs a distinct credential name. Mechanism only -- this function does not compute collisions, it just uses whatever name it is handed. + credential_binding_modes: Optional map of ``{resolved_env_var: mode}`` + (``"shared"`` or ``"separate"``) threaded into ``_prompt_for_field`` + so a reused credential env var is never re-prompted/overwritten and + a separate one may be left unset for runtime injection. Mechanism + only -- the caller decides the mode. settings: Optional AppSettings, used only to detect a same-type credential collision in ``non_interactive`` mode and fail loudly instead of silently reusing the type default (design §5.4.5). @@ -738,13 +784,19 @@ def configure_provider( if declared else declared ) + mode = ( + (credential_binding_modes or {}).get(env_var) if env_var else None + ) # Fail loud instead of silently reusing the type default when # it's already claimed by another instance (design §5.4.5). + # An explicit per-instance binding (shared reuse or separate) + # is intentional, so it is exempt from this guard. if ( settings is not None and declared and env_var == declared and declared not in (env_var_overrides or {}) + and mode is None and declared in _claimed_env_vars(settings) ): raise ValueError( @@ -754,7 +806,13 @@ def configure_provider( f"env_var_overrides mapping for this instance " f"instead of relying on the type default." ) - if env_var and os.environ.get(env_var): + if mode is not None: + # Explicit per-instance binding: persist the placeholder + # whether or not the value is set in the environment. A + # separate binding may be intentionally unset for runtime + # injection; a shared one reuses an existing instance's key. + collected_config[field_id] = f"${{{env_var}}}" + elif env_var and os.environ.get(env_var): collected_config[field_id] = f"${{{env_var}}}" elif existing_config and field_id in existing_config: collected_config[field_id] = existing_config[field_id] @@ -764,7 +822,12 @@ def configure_provider( # Prompt for the field (pass existing_config for defaults) field_id, value = _prompt_for_field( - field, key_manager, collected_config, existing_config, env_var_overrides + field, + key_manager, + collected_config, + existing_config, + env_var_overrides, + credential_binding_modes, ) if value is not None: collected_config[field_id] = value @@ -832,13 +895,19 @@ def configure_provider( if declared else declared ) + mode = ( + (credential_binding_modes or {}).get(env_var) if env_var else None + ) # Fail loud instead of silently reusing the type default when # it's already claimed by another instance (design §5.4.5). + # An explicit per-instance binding (shared reuse or separate) + # is intentional, so it is exempt from this guard. if ( settings is not None and declared and env_var == declared and declared not in (env_var_overrides or {}) + and mode is None and declared in _claimed_env_vars(settings) ): raise ValueError( @@ -848,7 +917,13 @@ def configure_provider( f"env_var_overrides mapping for this instance " f"instead of relying on the type default." ) - if env_var and os.environ.get(env_var): + if mode is not None: + # Explicit per-instance binding: persist the placeholder + # whether or not the value is set in the environment. A + # separate binding may be intentionally unset for runtime + # injection; a shared one reuses an existing instance's key. + collected_config[field_id] = f"${{{env_var}}}" + elif env_var and os.environ.get(env_var): collected_config[field_id] = f"${{{env_var}}}" elif existing_config and field_id in existing_config: collected_config[field_id] = existing_config[field_id] @@ -858,7 +933,12 @@ def configure_provider( # Prompt for the field (pass existing_config for defaults) field_id, value = _prompt_for_field( - field, key_manager, collected_config, existing_config, env_var_overrides + field, + key_manager, + collected_config, + existing_config, + env_var_overrides, + credential_binding_modes, ) if value is not None: collected_config[field_id] = value diff --git a/amplifier_app_cli/runtime/config.py b/amplifier_app_cli/runtime/config.py index 032a9e49..85358841 100644 --- a/amplifier_app_cli/runtime/config.py +++ b/amplifier_app_cli/runtime/config.py @@ -16,6 +16,7 @@ from ..lib.merge_utils import merge_module_items from ..lib.merge_utils import merge_tool_configs from ..lib.merge_utils import _normalize_module_entry +from ..provider_loader import get_provider_info if TYPE_CHECKING: @@ -305,6 +306,15 @@ def _on_progress(action: str, detail: str) -> None: if console: console.print(f"[dim]Bundle '{bundle_name}' prepared successfully[/dim]") + # Fail loud (before mount) on an unresolved *required* credential + # placeholder, instead of letting env-var expansion silently turn it + # into "" and letting the provider module fall back to its own ambient + # credential -- see _validate_provider_credentials for why this matters + # for the reuse-or-separate multi-instance flow. + raw_providers = bundle_config.get("providers") + if isinstance(raw_providers, list): + _validate_provider_credentials(raw_providers) + # Expand environment variables # IMPORTANT: Must expand BEFORE syncing to mount_plan, so ${ANTHROPIC_API_KEY} etc. become actual values bundle_config = expand_env_vars(bundle_config) @@ -842,6 +852,79 @@ def _merge_module_lists( ENV_PATTERN = re.compile(r"\$\{([^}:]+)(?::([^}]*))?}") +def _validate_provider_credentials(providers: list[Any]) -> None: + """Fail loudly, before session mount, when a provider instance's + configured credential placeholder resolves to nothing. + + Why this exists: ``expand_env_vars`` (below) treats an unset ``${VAR}`` + as an empty string. Several provider modules treat an empty/absent + ``api_key`` config value as "not configured" and fall back to their own + canonical ambient env var (e.g. ``OPENAI_API_KEY``). For a *separate* + per-instance credential binding left unset for runtime injection (see + the reuse-or-separate wizard flow in ``commands/provider.py`` / + ``provider_config_utils.py``, design doc §5.2), that fallback silently + routes the instance through a *different* account's key -- exactly the + wrong-account failure the reuse-or-separate flow exists to prevent. + Raising here turns that into a clear, actionable error at session start + instead of a silent cross-account credential mixup. + + Only enforced for fields the provider declares as + ``field_type == "secret"`` AND ``required`` (default True): optional / + keyless secrets (e.g. a local Chat Completions server with no API key) + are intentionally left alone, and a placeholder with an inline + ``${VAR:-default}`` default is also left alone (the default already + covers "unset"). + + App-CLI policy only -- no core, provider-contract, or settings-schema + changes. When provider metadata can't be loaded (custom/removed + provider, import error, etc.), validation is skipped for that entry -- + consistent with how the rest of this module already treats a missing + ``get_provider_info()`` result. + """ + for entry in providers: + if not isinstance(entry, dict): + continue + module_id = entry.get("module") + config = entry.get("config") + if not isinstance(module_id, str) or not isinstance(config, dict): + continue + + info = get_provider_info(module_id) + if not info: + continue + + for field in info.get("config_fields") or []: + if not isinstance(field, dict) or field.get("field_type") != "secret": + continue + if not field.get("required", True): + continue + + field_id = field.get("id") + if not field_id: + continue + raw_value = config.get(field_id) + if not isinstance(raw_value, str): + continue + + match = ENV_PATTERN.fullmatch(raw_value) + if not match: + continue + var_name, default = match.group(1), match.group(2) + if default is not None: + # An inline default already covers "unset" -- not our concern. + continue + if os.environ.get(var_name): + continue + + label = entry.get("id") or module_id + raise ValueError( + f"Credential environment variable {var_name} for provider " + f"'{label}' is not set. Set it before starting the " + f"session; Amplifier will not fall back to another " + f"provider's credential." + ) + + def expand_env_vars(config: dict[str, Any]) -> dict[str, Any]: """Expand ${VAR} references within configuration values.""" diff --git a/tests/test_provider_instance_credentials.py b/tests/test_provider_instance_credentials.py index ff02ef5f..0d7ec9a6 100644 --- a/tests/test_provider_instance_credentials.py +++ b/tests/test_provider_instance_credentials.py @@ -725,6 +725,408 @@ def _fake_configure_provider(module_id, key_manager, **kwargs): assert by_id["anthropic-opus"]["config"]["api_key"] == "${ANTHROPIC_API_KEY}" +# ============================================================ +# Reuse-vs-separate credential binding for same-type instances +# (offer to reuse an existing credential env var, or configure a +# separate one). Covers the shared, separate, unset, and +# edit-preservation paths. +# ============================================================ + + +class TestReuseVsSeparateCredentialBinding: + def test_reuse_choice_shares_env_var_and_never_prompts_for_secret( + self, tmp_path, monkeypatch + ): + """Choosing "reuse" (default) binds the new instance to the existing + credential env var, adds NO rename, and marks it "shared" so the + secret is never prompted for or overwritten.""" + monkeypatch.setattr(Path, "home", lambda: tmp_path) + from amplifier_app_cli.commands.provider import _resolve_env_var_overrides + + settings = _make_settings(tmp_path) + _seed_provider( + settings, + "provider-anthropic", + {"api_key": "${ANTHROPIC_API_KEY}"}, + provider_id="anthropic-opus", + scope="global", + ) + mock_key_manager = MagicMock() + + with ( + patch( + "amplifier_app_cli.commands.provider._secret_env_var_for", + return_value="ANTHROPIC_API_KEY", + ), + patch( + "amplifier_app_cli.commands.provider.Prompt.ask", + # Only the fork prompt is asked; "1" = reuse. No name prompt. + side_effect=["1"], + ) as mock_ask, + ): + decision = _resolve_env_var_overrides( + settings, mock_key_manager, "provider-anthropic", "anthropic-sonnet" + ) + + assert decision.env_var_overrides == {} + assert decision.binding_modes == {"ANTHROPIC_API_KEY": "shared"} + mock_ask.assert_called_once() + + def test_reuse_is_the_default_choice(self, tmp_path, monkeypatch): + """Pressing Enter (empty input) at the fork prompt must default to + reuse -- design requirement: reuse is the DEFAULT offer.""" + monkeypatch.setattr(Path, "home", lambda: tmp_path) + from amplifier_app_cli.commands.provider import _prompt_credential_binding + + settings = _make_settings(tmp_path) + _seed_provider( + settings, + "provider-anthropic", + {"api_key": "${ANTHROPIC_API_KEY}"}, + provider_id="anthropic-opus", + scope="global", + ) + mock_key_manager = MagicMock() + + with patch( + "amplifier_app_cli.commands.provider.Prompt.ask" + ) as mock_ask: + mock_ask.return_value = "1" # simulate default accepted + decision = _prompt_credential_binding( + settings, + mock_key_manager, + "provider-anthropic", + "anthropic-sonnet", + "ANTHROPIC_API_KEY", + {"ANTHROPIC_API_KEY"}, + ) + + # The prompt itself must declare "1" (reuse) as its default. + _, kwargs = mock_ask.call_args + assert kwargs.get("default") == "1" + assert decision.binding_modes == {"ANTHROPIC_API_KEY": "shared"} + + def test_separate_choice_derives_distinct_env_var(self, tmp_path, monkeypatch): + """Choosing "separate" derives a distinct env var and marks it + "separate" (secure-storage path unchanged: the secret field is + still prompted normally).""" + monkeypatch.setattr(Path, "home", lambda: tmp_path) + from amplifier_app_cli.commands.provider import _resolve_env_var_overrides + + settings = _make_settings(tmp_path) + _seed_provider( + settings, + "provider-anthropic", + {"api_key": "${ANTHROPIC_API_KEY}"}, + provider_id="anthropic-opus", + scope="global", + ) + mock_key_manager = MagicMock() + mock_key_manager.has_key.return_value = False + mock_key_manager.has_stored_key.return_value = False + + with ( + patch( + "amplifier_app_cli.commands.provider._secret_env_var_for", + return_value="ANTHROPIC_API_KEY", + ), + patch( + "amplifier_app_cli.commands.provider.Prompt.ask", + side_effect=["2", "ANTHROPIC_FABLE_API_KEY"], + ), + ): + decision = _resolve_env_var_overrides( + settings, mock_key_manager, "provider-anthropic", "anthropic-fable" + ) + + assert decision.env_var_overrides == { + "ANTHROPIC_API_KEY": "ANTHROPIC_FABLE_API_KEY" + } + assert decision.binding_modes == {"ANTHROPIC_FABLE_API_KEY": "separate"} + + def test_shared_binding_never_prompts_for_or_overwrites_secret(self): + """`_prompt_for_field` with binding_mode="shared" must return the + placeholder immediately -- no secret Prompt.ask call, no + key_manager.save_key call.""" + from amplifier_app_cli.provider_config_utils import _prompt_for_field + + field = { + "id": "api_key", + "display_name": "API Key", + "field_type": "secret", + "prompt": "Enter your API key", + "env_var": "ANTHROPIC_API_KEY", + "required": True, + } + mock_key_manager = MagicMock() + + with patch( + "amplifier_app_cli.provider_config_utils.Prompt.ask" + ) as mock_ask: + field_id, value = _prompt_for_field( + field, + mock_key_manager, + collected_config={}, + existing_config=None, + credential_binding_modes={"ANTHROPIC_API_KEY": "shared"}, + ) + + assert field_id == "api_key" + assert value == "${ANTHROPIC_API_KEY}" + mock_ask.assert_not_called() + mock_key_manager.save_key.assert_not_called() + + def test_separate_binding_left_blank_persists_unset_placeholder(self): + """A "separate" binding left blank at the prompt must persist the + ${VAR} placeholder UNSET for runtime injection -- NOT raise, and + NOT call key_manager.save_key (no value to save).""" + from amplifier_app_cli.provider_config_utils import _prompt_for_field + + field = { + "id": "api_key", + "display_name": "API Key", + "field_type": "secret", + "prompt": "Enter your API key", + "env_var": "ANTHROPIC_WORK_API_KEY", + "required": True, + } + mock_key_manager = MagicMock() + + with patch( + "amplifier_app_cli.provider_config_utils.Prompt.ask", + return_value="", + ): + field_id, value = _prompt_for_field( + field, + mock_key_manager, + collected_config={}, + existing_config=None, + credential_binding_modes={"ANTHROPIC_WORK_API_KEY": "separate"}, + ) + + assert field_id == "api_key" + assert value == "${ANTHROPIC_WORK_API_KEY}" + mock_key_manager.save_key.assert_not_called() + + def test_separate_binding_can_still_use_secure_storage(self): + """A "separate" binding is NOT forced blank -- entering a value at + the prompt still saves it through the existing secure-storage flow + (KeyManager.save_key), unchanged.""" + from amplifier_app_cli.provider_config_utils import _prompt_for_field + + field = { + "id": "api_key", + "display_name": "API Key", + "field_type": "secret", + "prompt": "Enter your API key", + "env_var": "ANTHROPIC_WORK_API_KEY", + "required": True, + } + mock_key_manager = MagicMock() + + with patch( + "amplifier_app_cli.provider_config_utils.Prompt.ask", + return_value="sk-separate-value", + ): + field_id, value = _prompt_for_field( + field, + mock_key_manager, + collected_config={}, + existing_config=None, + credential_binding_modes={"ANTHROPIC_WORK_API_KEY": "separate"}, + ) + + assert field_id == "api_key" + assert value == "${ANTHROPIC_WORK_API_KEY}" + mock_key_manager.save_key.assert_called_once_with( + "ANTHROPIC_WORK_API_KEY", "sk-separate-value" + ) + + def test_edit_preserves_shared_binding_no_prompt_no_overwrite(self, tmp_path): + """Editing ONE of two instances that share a credential env var must + recover binding_modes={"shared"} so the edit never prompts for or + overwrites the common secret (the reuse invariant must survive + edits, not just the initial add).""" + from amplifier_app_cli.commands.provider import _manage_edit_provider + + settings = _make_settings(tmp_path) + scope_data = { + "config": { + "providers": [ + { + "module": "provider-anthropic", + "id": "anthropic-opus", + "config": { + "default_model": "claude-opus", + "api_key": "${ANTHROPIC_API_KEY}", + "priority": 1, + }, + }, + { + "module": "provider-anthropic", + "id": "anthropic-sonnet", + "config": { + "default_model": "claude-sonnet", + "api_key": "${ANTHROPIC_API_KEY}", + "priority": 2, + }, + }, + ] + } + } + settings._write_scope("global", scope_data) + + providers = settings.get_provider_overrides() + idx = next( + i for i, p in enumerate(providers, 1) if p.get("id") == "anthropic-sonnet" + ) + + captured: dict = {} + + def _fake_configure_provider(module_id, key_manager, **kwargs): + captured["env_var_overrides"] = kwargs.get("env_var_overrides") + captured["credential_binding_modes"] = kwargs.get( + "credential_binding_modes" + ) + return { + "default_model": "claude-sonnet", + "api_key": "${ANTHROPIC_API_KEY}", + } + + with ( + patch( + "amplifier_app_cli.commands.provider.configure_provider", + side_effect=_fake_configure_provider, + ), + patch("amplifier_app_cli.commands.provider.KeyManager"), + patch( + "amplifier_app_cli.commands.provider._secret_env_var_for", + return_value="ANTHROPIC_API_KEY", + ), + patch( + "amplifier_app_cli.commands.provider._secret_field_id_for", + return_value="api_key", + ), + ): + _manage_edit_provider(settings, f"e{idx}", providers, scope="global") + + assert captured["env_var_overrides"] == {} + assert captured["credential_binding_modes"] == {"ANTHROPIC_API_KEY": "shared"} + + # The shared value on both instances is untouched. + raw = settings.get_scope_provider_overrides("global") + by_id = {p["id"]: p for p in raw} + assert by_id["anthropic-sonnet"]["config"]["api_key"] == "${ANTHROPIC_API_KEY}" + assert by_id["anthropic-opus"]["config"]["api_key"] == "${ANTHROPIC_API_KEY}" + + def test_non_interactive_shared_binding_persists_placeholder_when_unset(self): + """Non-interactive parity: a "shared" binding persists the + placeholder even when the shared env var happens to be unset in + THIS process's environment (it reuses the other instance's stored + value at runtime, so it must not be treated as missing).""" + from amplifier_app_cli.provider_config_utils import configure_provider + + mock_key_manager = MagicMock() + with patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=_mock_provider_info(), + ): + config = configure_provider( + "provider-anthropic", + mock_key_manager, + non_interactive=True, + credential_binding_modes={"ANTHROPIC_API_KEY": "shared"}, + ) + + assert config is not None + assert config["api_key"] == "${ANTHROPIC_API_KEY}" + + def test_non_interactive_separate_binding_persists_placeholder_when_unset(self): + """Non-interactive parity: a "separate" binding persists the + placeholder unset for runtime injection instead of silently + skipping the field.""" + from amplifier_app_cli.provider_config_utils import configure_provider + + mock_key_manager = MagicMock() + with patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=_mock_provider_info(env_var="ANTHROPIC_WORK_API_KEY"), + ): + config = configure_provider( + "provider-anthropic", + mock_key_manager, + non_interactive=True, + credential_binding_modes={"ANTHROPIC_WORK_API_KEY": "separate"}, + ) + + assert config is not None + assert config["api_key"] == "${ANTHROPIC_WORK_API_KEY}" + + def test_non_interactive_explicit_binding_exempt_from_fail_loud(self, tmp_path): + """An explicit binding mode (shared or separate) is intentional, so + it must be exempt from the §5.4.5 non-interactive collision + fail-loud guard even when the type default is already claimed.""" + from amplifier_app_cli.provider_config_utils import configure_provider + + settings = _make_settings(tmp_path) + _seed_provider( + settings, + "provider-anthropic", + {"api_key": "${ANTHROPIC_API_KEY}"}, + provider_id="anthropic-opus", + scope="global", + ) + mock_key_manager = MagicMock() + + with patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=_mock_provider_info(), + ): + # No exception, even though ANTHROPIC_API_KEY is already claimed + # by anthropic-opus -- the "shared" mode makes this intentional. + config = configure_provider( + "provider-anthropic", + mock_key_manager, + non_interactive=True, + settings=settings, + credential_binding_modes={"ANTHROPIC_API_KEY": "shared"}, + ) + + assert config is not None + assert config["api_key"] == "${ANTHROPIC_API_KEY}" + + def test_first_instance_blank_required_secret_still_errors(self, monkeypatch): + """Scope guard: WITHOUT an explicit per-instance binding mode, a + blank required secret still errors (today's first-time-setup + guardrail is unchanged).""" + from amplifier_app_cli.provider_config_utils import _prompt_for_field + + # Use a name guaranteed unset in the ambient environment so the test + # exercises the "no existing value" required-field path deterministically. + monkeypatch.delenv("UNSET_FIRST_INSTANCE_API_KEY", raising=False) + field = { + "id": "api_key", + "display_name": "API Key", + "field_type": "secret", + "prompt": "Enter your API key", + "env_var": "UNSET_FIRST_INSTANCE_API_KEY", + "required": True, + } + mock_key_manager = MagicMock() + + with patch( + "amplifier_app_cli.provider_config_utils.Prompt.ask", + return_value="", + ): + with pytest.raises(ValueError): + _prompt_for_field( + field, + mock_key_manager, + collected_config={}, + existing_config=None, + ) + + # ============================================================ # Silent-clobber race regression (§5.4.1 fix): a still-unnormalized # literal secret for an EXISTING instance must not let a brand-new @@ -901,15 +1303,20 @@ def test_prefill_uses_live_env_value_when_set(self, tmp_path, monkeypatch): ), patch( "amplifier_app_cli.commands.provider.Prompt.ask", - return_value="ANTHROPIC_FABLE_API_KEY", + # 1st prompt = reuse-vs-separate fork (choose separate); + # 2nd prompt = the per-instance env var name. + side_effect=["2", "ANTHROPIC_FABLE_API_KEY"], ) as mock_ask, ): - overrides = _resolve_env_var_overrides( + decision = _resolve_env_var_overrides( settings, mock_key_manager, "provider-anthropic", "anthropic-fable" ) - assert overrides == {"ANTHROPIC_API_KEY": "ANTHROPIC_FABLE_API_KEY"} - # default= kwarg on the prompt call should be the derived suggestion + assert decision.env_var_overrides == { + "ANTHROPIC_API_KEY": "ANTHROPIC_FABLE_API_KEY" + } + assert decision.binding_modes == {"ANTHROPIC_FABLE_API_KEY": "separate"} + # default= kwarg on the LAST (name) prompt should be the derived suggestion _, kwargs = mock_ask.call_args assert kwargs.get("default") == "ANTHROPIC_FABLE_API_KEY" @@ -938,14 +1345,16 @@ def test_no_prefill_note_when_env_var_not_set(self, tmp_path, monkeypatch): ), patch( "amplifier_app_cli.commands.provider.Prompt.ask", - return_value="ANTHROPIC_FABLE_API_KEY", + side_effect=["2", "ANTHROPIC_FABLE_API_KEY"], ), ): - overrides = _resolve_env_var_overrides( + decision = _resolve_env_var_overrides( settings, mock_key_manager, "provider-anthropic", "anthropic-fable" ) - assert overrides == {"ANTHROPIC_API_KEY": "ANTHROPIC_FABLE_API_KEY"} + assert decision.env_var_overrides == { + "ANTHROPIC_API_KEY": "ANTHROPIC_FABLE_API_KEY" + } # ============================================================ @@ -982,7 +1391,7 @@ def test_warns_and_reuses_keys_env_only_leftover(self, tmp_path, monkeypatch): ), patch( "amplifier_app_cli.commands.provider.Prompt.ask", - return_value="ANTHROPIC_FABLE_API_KEY", + side_effect=["2", "ANTHROPIC_FABLE_API_KEY"], ), patch("amplifier_app_cli.commands.provider.console") as mock_console, ): @@ -1035,15 +1444,17 @@ def test_real_key_manager_leftover_is_reused_not_rejected( ), patch( "amplifier_app_cli.commands.provider.Prompt.ask", - return_value="ANTHROPIC_FABLE_API_KEY", + side_effect=["2", "ANTHROPIC_FABLE_API_KEY"], ), patch("amplifier_app_cli.commands.provider.console") as mock_console, ): - overrides = _resolve_env_var_overrides( + decision = _resolve_env_var_overrides( settings, key_manager, "provider-anthropic", "anthropic-fable" ) - assert overrides == {"ANTHROPIC_API_KEY": "ANTHROPIC_FABLE_API_KEY"} + assert decision.env_var_overrides == { + "ANTHROPIC_API_KEY": "ANTHROPIC_FABLE_API_KEY" + } printed = " ".join(str(c) for c in mock_console.print.call_args_list) assert "stored credential" in printed.lower() assert "already in use by another" not in printed.lower() @@ -1077,14 +1488,15 @@ def test_first_instance_gets_no_collision_prompt(self, tmp_path, monkeypatch): ), patch("amplifier_app_cli.commands.provider.Prompt.ask") as mock_ask, ): - overrides = _resolve_env_var_overrides( + decision = _resolve_env_var_overrides( settings, MagicMock(), "provider-anthropic", None ) - assert overrides == {}, ( + assert decision.env_var_overrides == {}, ( "A keys.env-only leftover is owned by no instance -- the first " "instance of the type must use the type default (§5.2 step 3)." ) + assert decision.binding_modes == {} mock_ask.assert_not_called() def test_second_instance_still_collides(self, tmp_path, monkeypatch): @@ -1112,15 +1524,18 @@ def test_second_instance_still_collides(self, tmp_path, monkeypatch): ), patch( "amplifier_app_cli.commands.provider.Prompt.ask", - return_value="ANTHROPIC_FABLE_API_KEY", + side_effect=["2", "ANTHROPIC_FABLE_API_KEY"], ) as mock_ask, ): - overrides = _resolve_env_var_overrides( + decision = _resolve_env_var_overrides( settings, mock_key_manager, "provider-anthropic", "anthropic-fable" ) - assert overrides == {"ANTHROPIC_API_KEY": "ANTHROPIC_FABLE_API_KEY"} - mock_ask.assert_called_once() + assert decision.env_var_overrides == { + "ANTHROPIC_API_KEY": "ANTHROPIC_FABLE_API_KEY" + } + # Collision path entered: fork prompt (choice) + separate-name prompt. + assert mock_ask.call_count == 2 def test_provider_add_cli_writes_entry_despite_stale_key( self, tmp_path, monkeypatch @@ -1369,8 +1784,11 @@ def test_provider_add_cli_exits_on_degenerate_suggestion(self, tmp_path): return_value="ANTHROPIC_API_KEY", ), ): - # instance_id "---" sanitizes to an empty suffix. - result = runner.invoke(provider, ["add", "anthropic"], input="---\n") + # instance_id "---" sanitizes to an empty suffix. Choose the + # SEPARATE credential path ("2") so the degenerate id reaches + # _suggest_instance_env_var and fails loudly (the reuse path + # never derives a name, so it wouldn't exercise this guard). + result = runner.invoke(provider, ["add", "anthropic"], input="---\n2\n") assert result.exit_code != 0 diff --git a/tests/test_runtime_credential_validation.py b/tests/test_runtime_credential_validation.py new file mode 100644 index 00000000..4a69148d --- /dev/null +++ b/tests/test_runtime_credential_validation.py @@ -0,0 +1,276 @@ +"""Tests for the runtime credential-fallback hardening. + +Covers `_validate_provider_credentials()` (amplifier_app_cli.runtime.config), +added alongside the reuse-or-separate multi-instance credential wizard +(docs/designs/provider-instance-credentials.md). Without this guard, an +unresolved *separate* credential placeholder expands to "" at runtime and +several provider modules treat that as "not configured", silently falling +back to their own canonical ambient env var -- routing the "separate" +instance through the WRONG account's key. This must fail loudly instead. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from amplifier_app_cli.runtime.config import _validate_provider_credentials + + +def _provider_info(env_var: str = "OPENAI_API_KEY", required: bool = True) -> dict: + return { + "display_name": "OpenAI", + "config_fields": [ + { + "id": "api_key", + "display_name": "API Key", + "field_type": "secret", + "prompt": "Enter your API key", + "env_var": env_var, + "required": required, + } + ], + } + + +class TestValidateProviderCredentials: + def test_unset_separate_binding_fails_loud(self, monkeypatch): + """The core hardening case: a required secret field whose ${VAR} + placeholder resolves to nothing must raise BEFORE session mount, + instead of silently letting expand_env_vars turn it into "" (which + would let the provider module fall back to its own ambient var -- + e.g. a *different* account's OPENAI_API_KEY).""" + monkeypatch.delenv("OPENAI_WORK_API_KEY", raising=False) + monkeypatch.setenv("OPENAI_API_KEY", "sk-shared-account-key") + + providers = [ + { + "module": "provider-openai", + "id": "openai-work", + "config": {"api_key": "${OPENAI_WORK_API_KEY}"}, + } + ] + + with patch( + "amplifier_app_cli.runtime.config.get_provider_info", + return_value=_provider_info(env_var="OPENAI_WORK_API_KEY"), + ), pytest.raises(ValueError, match="OPENAI_WORK_API_KEY"): + _validate_provider_credentials(providers) + + def test_set_credential_does_not_raise(self, monkeypatch): + """A required secret field whose env var IS set must pass silently.""" + monkeypatch.setenv("OPENAI_WORK_API_KEY", "sk-distinct-value") + + providers = [ + { + "module": "provider-openai", + "id": "openai-work", + "config": {"api_key": "${OPENAI_WORK_API_KEY}"}, + } + ] + + with patch( + "amplifier_app_cli.runtime.config.get_provider_info", + return_value=_provider_info(env_var="OPENAI_WORK_API_KEY"), + ): + _validate_provider_credentials(providers) # must not raise + + def test_shared_binding_with_set_var_does_not_raise(self, monkeypatch): + """Two instances sharing the SAME credential env var (the "reuse" + path) must not raise as long as that shared var is set -- this is + the normal, intended shared-credential configuration.""" + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-shared-value") + + providers = [ + { + "module": "provider-anthropic", + "id": "anthropic-opus", + "config": {"api_key": "${ANTHROPIC_API_KEY}"}, + }, + { + "module": "provider-anthropic", + "id": "anthropic-sonnet", + "config": {"api_key": "${ANTHROPIC_API_KEY}"}, + }, + ] + + with patch( + "amplifier_app_cli.runtime.config.get_provider_info", + return_value=_provider_info(env_var="ANTHROPIC_API_KEY"), + ): + _validate_provider_credentials(providers) # must not raise + + def test_optional_keyless_secret_field_unset_does_not_raise(self, monkeypatch): + """A secret field declared `required=False` (e.g. a local/keyless + Chat Completions server) must be left alone even when unset -- + the fail-loud guard is scoped to REQUIRED credential fields only.""" + monkeypatch.delenv("LOCAL_SERVER_API_KEY", raising=False) + + providers = [ + { + "module": "provider-chat-completions", + "id": "local-server", + "config": {"api_key": "${LOCAL_SERVER_API_KEY}"}, + } + ] + + with patch( + "amplifier_app_cli.runtime.config.get_provider_info", + return_value=_provider_info(env_var="LOCAL_SERVER_API_KEY", required=False), + ): + _validate_provider_credentials(providers) # must not raise + + def test_missing_provider_metadata_skips_validation(self, monkeypatch): + """When provider metadata can't be loaded (custom/removed provider, + import error, etc.) validation is skipped for that entry rather + than blocking session start -- consistent with how the rest of + this module treats a missing get_provider_info() result.""" + monkeypatch.delenv("SOME_UNSET_VAR", raising=False) + + providers = [ + { + "module": "provider-custom-thing", + "id": "custom", + "config": {"api_key": "${SOME_UNSET_VAR}"}, + } + ] + + with patch( + "amplifier_app_cli.runtime.config.get_provider_info", + return_value=None, + ): + _validate_provider_credentials(providers) # must not raise + + def test_literal_value_is_not_validated(self, monkeypatch): + """A literal (non-placeholder) config value is untouched by this + guard -- it isn't an unresolved env var reference at all.""" + providers = [ + { + "module": "provider-openai", + "id": "openai-work", + "config": {"api_key": "sk-literal-value-not-a-placeholder"}, + } + ] + + with patch( + "amplifier_app_cli.runtime.config.get_provider_info", + return_value=_provider_info(env_var="OPENAI_WORK_API_KEY"), + ): + _validate_provider_credentials(providers) # must not raise + + def test_inline_default_placeholder_is_not_validated(self, monkeypatch): + """A placeholder carrying an inline default (${VAR:default}) already + has its own "unset" handling via expand_env_vars -- this guard only + concerns itself with bare ${VAR} placeholders.""" + monkeypatch.delenv("OPENAI_WORK_API_KEY", raising=False) + + providers = [ + { + "module": "provider-openai", + "id": "openai-work", + "config": {"api_key": "${OPENAI_WORK_API_KEY:not-needed}"}, + } + ] + + with patch( + "amplifier_app_cli.runtime.config.get_provider_info", + return_value=_provider_info(env_var="OPENAI_WORK_API_KEY"), + ): + _validate_provider_credentials(providers) # must not raise + + def test_multiple_providers_identifies_the_failing_instance(self, monkeypatch): + """With several configured providers, the error must name the + instance and env var actually at fault, not a generic message.""" + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-fine") + monkeypatch.delenv("OPENAI_WORK_API_KEY", raising=False) + + providers = [ + { + "module": "provider-anthropic", + "id": "anthropic-opus", + "config": {"api_key": "${ANTHROPIC_API_KEY}"}, + }, + { + "module": "provider-openai", + "id": "openai-work", + "config": {"api_key": "${OPENAI_WORK_API_KEY}"}, + }, + ] + + def _info(module_id: str): + if module_id == "provider-anthropic": + return _provider_info(env_var="ANTHROPIC_API_KEY") + return _provider_info(env_var="OPENAI_WORK_API_KEY") + + with patch( + "amplifier_app_cli.runtime.config.get_provider_info", + side_effect=_info, + ), pytest.raises(ValueError) as exc_info: + _validate_provider_credentials(providers) + + assert "OPENAI_WORK_API_KEY" in str(exc_info.value) + assert "openai-work" in str(exc_info.value) + + def test_non_dict_or_non_module_entries_are_skipped(self): + """Malformed provider entries must not crash the guard.""" + providers = [ + "not-a-dict", + {"config": {"api_key": "${SOMETHING}"}}, # no "module" + {"module": 123, "config": {"api_key": "${SOMETHING}"}}, # bad module type + {"module": "provider-x", "config": "not-a-dict"}, # bad config type + ] + _validate_provider_credentials(providers) # must not raise + + +# ============================================================ +# Integration: the guard actually runs inside resolve_bundle_config(), +# before expand_env_vars() would otherwise silently launder the unset +# placeholder into "". +# ============================================================ + + +class TestValidateProviderCredentialsIntegration: + @pytest.mark.asyncio + async def test_resolve_bundle_config_raises_for_unset_separate_credential( + self, monkeypatch + ): + from amplifier_app_cli.runtime.config import resolve_bundle_config + + monkeypatch.delenv("OPENAI_WORK_API_KEY", raising=False) + monkeypatch.setenv("OPENAI_API_KEY", "sk-shared-account-key") + + mount_plan = { + "providers": [ + { + "module": "provider-openai", + "id": "openai-work", + "config": {"api_key": "${OPENAI_WORK_API_KEY}"}, + } + ], + } + mock_prepared = MagicMock() + mock_prepared.mount_plan = mount_plan + mock_prepared.bundle.load_agent_metadata = MagicMock() + + settings = MagicMock() + settings.get_config_overrides.return_value = {} + settings.get_provider_overrides.return_value = [] + settings.get_tool_overrides.return_value = [] + settings.get_notification_hook_overrides.return_value = [] + settings.get_routing_config.return_value = None + settings.get_source_overrides.return_value = {} + settings.get_module_sources.return_value = {} + settings.get_bundle_sources.return_value = {} + + with ( + patch( + "amplifier_app_cli.lib.bundle_loader.prepare.load_and_prepare_bundle", + new_callable=AsyncMock, + return_value=mock_prepared, + ), + patch("amplifier_app_cli.paths.get_bundle_search_paths", return_value=[]), + patch("amplifier_app_cli.lib.bundle_loader.AppBundleDiscovery"), + patch( + "amplifier_app_cli.runtime.config.get_provider_info", + return_value=_provider_info(env_var="OPENAI_WORK_API_KEY"), + ),pytest.raises(ValueError, match="OPENAI_WORK_API_KEY") + ): + await resolve_bundle_config(bundle_name="test", app_settings=settings)