diff --git a/amplifier_app_cli/commands/init.py b/amplifier_app_cli/commands/init.py index 67efeed..3d3b68b 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 f5f9750..73b0895 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 0000000..26a32eb --- /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"