fix: GAP-003 — refuse to silently fall back to Ollama when a credentialed provider's module is missing

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:

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"
 

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"