From 693aa2cdc644fb1e8362a42058d0afb719adec94 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Wed, 12 Aug 2026 17:30:25 -0700 Subject: [PATCH] fix: GAP-003 - refuse to silently fall back to Ollama when a credentialed provider's module is missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-platform behaviour change (not Windows-specific). Affects every Linux/macOS/WSL/Windows user of `detect_provider_from_env()` / `auto_init_from_env()` (the non-interactive auto-configure path used when stdin is not a TTY: CI, Docker, shadow environments). Old behaviour: `detect_provider_from_env()` treated "this provider's module isn't installed" identically to "no credentials set for this provider" -- both cases just `continue`d past the provider in the priority loop. A user with a valid `ANTHROPIC_API_KEY` set, but whose `provider-anthropic` module was not installed (or failed to install), silently fell through to the credential-free Ollama fallback. That choice got persisted to settings.yaml, so it wasn't even a one-time mistake -- every subsequent run kept using Ollama, with no error and no mention that a real API key was ever seen and discarded. The symptom the user actually saw was a `ConnectionError` against a local Ollama server they never set up, which is a much harder thing to debug than "you're missing a package." New behaviour: if a provider has all of its required credential env vars present but its module is not installed/importable, that is recorded and blocks the Ollama fallback. If no other candidate provider is both credentialed and installed, `detect_provider_from_env()` raises `CredentialedProviderModuleMissingError` naming the provider, the env vars that were found, and the fix (`amplifier provider install `). `auto_init_from_env()` catches this specifically and prints a loud, specific error instead of quietly "succeeding" onto the wrong backend. Nothing is persisted, so the next run gets a real second chance once the module is installed. Unaffected: the genuine no-cloud-credentials case still lands on Ollama quietly, exactly as before (covered by `test_no_credentials_falls_through_to_ollama`). Unaffected: a higher-priority provider with a missing module no longer blocks a lower-priority provider that IS both credentialed and installed -- that one is still selected (`test_falls_through_to_second_credentialed_installed_provider`). Why fail loud instead of silently substituting a different provider: the user made an explicit choice by setting a specific provider's credentials. Silently overriding that choice with Ollama is a correctness bug dressed up as graceful degradation -- it changes which backend runs, which model answers, and (for anyone who assumed their cloud key was in effect) can send prompts to the wrong place entirely. An explicit, actionable error that names the exact fix is strictly better than a misleading downstream connection failure. Test evidence: 7 new tests in tests/test_provider_env_detect.py exercise detect_provider_from_env() directly (entry_points patched, not the whole function mocked away) across every branch: no credentials/no installed providers -> None; no credentials + Ollama installed -> quiet Ollama fallback (unaffected case, regression guard); credentials + module installed -> that provider; credentials + module missing (with and without Ollama available) -> raises; higher-priority module missing but lower-priority both credentialed and installed -> lower-priority one still selected; decisive regression guard asserting the function must never return "provider-ollama" once a credentialed-but-missing provider was seen. Reverting just the detect_provider_from_env() logic back to the old "treat missing module same as missing credentials" behavior (keeping the exception class defined so imports still resolve) makes exactly the 3 tests targeting the new behavior fail with "DID NOT RAISE" / "provider-ollama" == "provider-ollama", confirming they exercise the new code path and not just the pre-existing one. Full suite: 1308 passed, 1 skipped, 13 deselected, 1 xfailed (this branch only carries GAP-003 + tests, so the count is smaller than main's ~1316 -- expected). ruff clean on all three changed files. Extracted from microsoft/amplifier-app-cli#259, which bundles this GAP-003 fix together with four unrelated fixes (GAP-020/023/027/021) across 15 commits and 19 files under a "Windows compatibility gaps" label. This change is not Windows-gated and needs review on its own terms as a default-behaviour change for every platform. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_app_cli/commands/init.py | 17 ++- amplifier_app_cli/provider_env_detect.py | 95 ++++++++++++-- tests/test_provider_env_detect.py | 151 +++++++++++++++++++++++ 3 files changed, 251 insertions(+), 12 deletions(-) create mode 100644 tests/test_provider_env_detect.py diff --git a/amplifier_app_cli/commands/init.py b/amplifier_app_cli/commands/init.py index 67efeedb..3d3b68be 100644 --- a/amplifier_app_cli/commands/init.py +++ b/amplifier_app_cli/commands/init.py @@ -18,7 +18,10 @@ ) from ..provider_config_utils import configure_provider from ..provider_manager import ProviderManager -from ..provider_env_detect import detect_provider_from_env +from ..provider_env_detect import ( + CredentialedProviderModuleMissingError, + detect_provider_from_env, +) from ..provider_sources import install_known_providers from .routing import _discover_matrix_files from .routing import _get_configured_provider_types @@ -318,6 +321,18 @@ def auto_init_from_env(console_arg: Console | None = None) -> bool: ) return True + except CredentialedProviderModuleMissingError as e: + # GAP-003: environment credentials were found for a real provider, + # but its module isn't usable. This must be loud and specific -- + # NOT collapsed into the generic warning below, and NOT allowed to + # silently fall through to Ollama (detect_provider_from_env() never + # returns "provider-ollama" once this exception is raised, so there + # is nothing to silently configure here; we just report and stop). + logger.error(f"Auto-init: {e}") + if console_arg: + console_arg.print(f"[bold red]\u2717 {e}[/bold red]") + return False + except Exception as e: logger.warning(f"Auto-init failed: {e}") if console_arg: diff --git a/amplifier_app_cli/provider_env_detect.py b/amplifier_app_cli/provider_env_detect.py index f5f97509..73b0895a 100644 --- a/amplifier_app_cli/provider_env_detect.py +++ b/amplifier_app_cli/provider_env_detect.py @@ -3,7 +3,6 @@ import os from importlib.metadata import entry_points - # Known credential env vars for each provider # Module name -> list of env vars that indicate the provider is configured PROVIDER_CREDENTIAL_VARS: dict[str, list[str]] = { @@ -16,34 +15,108 @@ } +class CredentialedProviderModuleMissingError(RuntimeError): + """Raised by detect_provider_from_env() when environment credentials + point at a provider whose module is not installed/importable. + + GAP-003: previously, "module not installed" and "no credentials set" + were treated identically -- both simply `continue`d past the provider + in the priority loop, falling through to the Ollama fallback (or to + None) with no distinction and no diagnostic. That silently discarded a + real, valid API key: a user with `ANTHROPIC_API_KEY` set but a + not-yet-installed (or install-failed) `provider-anthropic` module got + auto-configured onto Ollama instead, then hit a misleading + `ConnectionError` against a local server that was never running, with + nothing telling them their real key was ever seen. + + These are very different situations and must not be handled the same + way. "No credentials for this provider" is silence-worthy -- there is + nothing to report. "Credentials are present but the module can't be + used" is a loud, actionable condition: the user has a working key for + a provider that Amplifier chose not to use, for a reason it can name. + + This exception is that loud condition. Catching it and reporting it + (see `auto_init_from_env`) replaces the silent fall-through -- it does + NOT replace the legitimate case of a user with genuinely no cloud + credentials landing on Ollama, which still happens quietly and + correctly when this exception is never raised. + """ + + def __init__(self, provider_id: str, env_vars: list[str]): + self.provider_id = provider_id + self.env_vars = env_vars + display = provider_id.removeprefix("provider-") + vars_str = " and ".join(env_vars) + super().__init__( + f"Found credentials for {display} ({vars_str}) but the " + f"'{provider_id}' module is not installed or could not be " + f"imported. Run 'amplifier provider install {display}' (or " + f"'amplifier provider add') to fix this. Refusing to silently " + f"fall back to a different provider you didn't configure." + ) + + def detect_provider_from_env() -> str | None: """Detect configured provider from environment variables. Checks installed provider modules against known credential env vars. - Returns the first provider that has credentials configured. + Returns the first provider that has credentials configured AND whose + module is actually installed. + + Raises: + CredentialedProviderModuleMissingError: if a provider has all of + its credential env vars set but its module is not installed. + This must never be treated the same as "no credentials" -- + see the exception's docstring for why (GAP-003). Returns: - module_id if a provider's credentials are found, None otherwise. + module_id if a provider's credentials are found and its module is + installed; "provider-ollama" if nothing else matched and Ollama's + module is installed (the genuinely-no-cloud-credentials case); + None otherwise. """ # Get installed provider modules eps = entry_points(group="amplifier.modules") installed_providers = {ep.name for ep in eps if ep.name.startswith("provider-")} + # Providers whose credentials ARE fully present in the environment but + # whose module is NOT installed. Recorded rather than silently skipped + # (GAP-003) -- these must block the Ollama fallback, not fall through + # to it, because falling through would discard a real, valid key with + # no indication it was ever seen. + missing_but_credentialed: list[tuple[str, list[str]]] = [] + # Check each known provider (in priority order) for credentials for provider_id, env_vars in PROVIDER_CREDENTIAL_VARS.items(): - # Skip if provider not installed - if provider_id not in installed_providers: + # Providers with no required credentials (like ollama) are handled + # by the dedicated check below, not by this credential loop. + if not env_vars: continue - # Skip providers with no required credentials (like ollama) - if not env_vars: + # No credentials set for this provider at all -- genuinely nothing + # to report, move on to the next candidate. + if not all(os.environ.get(var) for var in env_vars): continue - # Check if ALL required env vars are set - if all(os.environ.get(var) for var in env_vars): - return provider_id + # Credentials ARE present. If the module isn't installed, this is + # the GAP-003 condition: record it and keep checking lower-priority + # providers (one of them may be both credentialed and installed), + # but never silently fall through to Ollama once anything has been + # recorded here. + if provider_id not in installed_providers: + missing_but_credentialed.append((provider_id, env_vars)) + continue + + return provider_id + + if missing_but_credentialed: + provider_id, env_vars = missing_but_credentialed[0] + raise CredentialedProviderModuleMissingError(provider_id, env_vars) - # Check for ollama last (since it doesn't require credentials) + # Check for ollama last (since it doesn't require credentials) -- only + # reached when no provider anywhere in PROVIDER_CREDENTIAL_VARS had + # credentials set. This is the genuinely-no-cloud-credentials case and + # must stay quiet and correct. if "provider-ollama" in installed_providers: return "provider-ollama" diff --git a/tests/test_provider_env_detect.py b/tests/test_provider_env_detect.py new file mode 100644 index 00000000..26a32eb1 --- /dev/null +++ b/tests/test_provider_env_detect.py @@ -0,0 +1,151 @@ +"""Tests for amplifier_app_cli.provider_env_detect. + +GAP-003: `detect_provider_from_env()` must distinguish two very different +situations that were previously handled identically: + +1. A provider has NO credentials in the environment at all -- silence is + correct, fall through to the next candidate (and eventually Ollama). +2. A provider DOES have credentials in the environment, but its module is + not installed/importable -- this must be loud + (`CredentialedProviderModuleMissingError`), not a silent fall-through + to a different, unrequested provider. + +These tests exercise `detect_provider_from_env()` directly (not mocked), +with `entry_points` patched to control which provider modules appear +"installed", so the real priority-loop logic is under test. +""" + +from unittest.mock import MagicMock, patch + +import pytest +from amplifier_app_cli.provider_env_detect import ( + PROVIDER_CREDENTIAL_VARS, + CredentialedProviderModuleMissingError, + detect_provider_from_env, +) + + +def _mock_entry_points(names: list[str]): + """Build a fake entry_points() return value with the given module names.""" + eps = [] + for name in names: + ep = MagicMock() + ep.name = name + eps.append(ep) + return eps + + +def _clear_all_provider_env_vars(monkeypatch): + """Strip every credential env var this module knows about, so tests are + isolated from whatever happens to be set in the ambient environment + (e.g. a real GITHUB_TOKEN or ANTHROPIC_API_KEY on the machine running + the suite).""" + for env_vars in PROVIDER_CREDENTIAL_VARS.values(): + for var in env_vars: + monkeypatch.delenv(var, raising=False) + + +class TestDetectProviderFromEnvNoCredentials: + """The genuinely-no-cloud-credentials case must stay quiet and correct.""" + + def test_no_env_vars_no_installed_providers_returns_none(self, monkeypatch): + _clear_all_provider_env_vars(monkeypatch) + with patch( + "amplifier_app_cli.provider_env_detect.entry_points", + return_value=_mock_entry_points([]), + ): + assert detect_provider_from_env() is None + + def test_no_credentials_falls_through_to_ollama(self, monkeypatch): + """No cloud credentials set, but provider-ollama IS installed -> + quietly select Ollama. This is the legitimate case the fix must + not disturb.""" + _clear_all_provider_env_vars(monkeypatch) + with patch( + "amplifier_app_cli.provider_env_detect.entry_points", + return_value=_mock_entry_points(["provider-ollama"]), + ): + assert detect_provider_from_env() == "provider-ollama" + + +class TestDetectProviderFromEnvCredentialedAndInstalled: + """The normal, working case: credentials present, module installed.""" + + def test_anthropic_credentials_and_module_installed(self, monkeypatch): + monkeypatch.setenv("ANTHROPIC_API_KEY", "dummy-not-a-real-key") + with patch( + "amplifier_app_cli.provider_env_detect.entry_points", + return_value=_mock_entry_points(["provider-anthropic", "provider-ollama"]), + ): + assert detect_provider_from_env() == "provider-anthropic" + + +class TestDetectProviderFromEnvCredentialedButModuleMissing: + """GAP-003: the fixed behavior. Credentials present, module NOT + installed -- must raise loudly, never silently pick Ollama.""" + + def test_raises_instead_of_falling_back_to_ollama(self, monkeypatch): + monkeypatch.setenv("ANTHROPIC_API_KEY", "dummy-not-a-real-key") + with patch( + "amplifier_app_cli.provider_env_detect.entry_points", + # provider-anthropic is NOT in this list -- module missing. + # provider-ollama IS installed -- this is exactly the shape + # that used to silently produce "provider-ollama". + return_value=_mock_entry_points(["provider-ollama"]), + ): + with pytest.raises(CredentialedProviderModuleMissingError) as excinfo: + detect_provider_from_env() + + assert excinfo.value.provider_id == "provider-anthropic" + assert "ANTHROPIC_API_KEY" in str(excinfo.value) + assert "provider-anthropic" in str(excinfo.value) + + def test_raises_even_when_ollama_not_installed_either(self, monkeypatch): + """Same defect, no Ollama fallback available at all (would have + previously returned None with no explanation).""" + monkeypatch.setenv("ANTHROPIC_API_KEY", "dummy-not-a-real-key") + with ( + patch( + "amplifier_app_cli.provider_env_detect.entry_points", + return_value=_mock_entry_points([]), + ), + pytest.raises(CredentialedProviderModuleMissingError), + ): + detect_provider_from_env() + + def test_falls_through_to_second_credentialed_installed_provider(self, monkeypatch): + """If a higher-priority provider's module is missing but a + lower-priority provider is both credentialed AND installed, that + lower-priority provider should still be selected -- the missing + higher-priority one is recorded but doesn't block a real, + installed, working alternative.""" + monkeypatch.setenv("ANTHROPIC_API_KEY", "dummy-not-a-real-key") + monkeypatch.setenv("OPENAI_API_KEY", "dummy-not-a-real-key") + with patch( + "amplifier_app_cli.provider_env_detect.entry_points", + # anthropic (higher priority) missing; openai (lower priority) + # installed and credentialed. + return_value=_mock_entry_points(["provider-openai"]), + ): + assert detect_provider_from_env() == "provider-openai" + + def test_does_not_reach_ollama_when_credentialed_provider_missing( + self, monkeypatch + ): + """Decisive regression guard for the exact GAP-003 symptom: with + ANTHROPIC_API_KEY set and provider-anthropic's module missing, + the function must never return "provider-ollama" even though + Ollama's module is installed.""" + monkeypatch.setenv("ANTHROPIC_API_KEY", "dummy-not-a-real-key") + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + with ( + patch( + "amplifier_app_cli.provider_env_detect.entry_points", + return_value=_mock_entry_points(["provider-ollama"]), + ), + pytest.raises(CredentialedProviderModuleMissingError), + ): + result = detect_provider_from_env() + # Should never get here, but if the exception handling + # regresses, fail loudly on the actual returned value too. + assert result != "provider-ollama"