diff --git a/amplifier_app_cli/commands/init.py b/amplifier_app_cli/commands/init.py index 67efeedb..355115d5 100644 --- a/amplifier_app_cli/commands/init.py +++ b/amplifier_app_cli/commands/init.py @@ -18,8 +18,12 @@ ) 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 ..utils.error_format import escape_markup from .routing import _discover_matrix_files from .routing import _get_configured_provider_types from .routing import _load_all_matrices @@ -266,6 +270,10 @@ def auto_init_from_env(console_arg: Console | None = None) -> bool: Returns True if a provider was configured, False otherwise. This is best-effort — failures are logged but never raised. """ + # Declared before the try block so the except handlers can always read it, + # including when the failure happens before the install step runs. + install_failures: list[tuple[str, str]] = [] + try: logger.info( "Non-interactive environment detected, " @@ -274,8 +282,21 @@ def auto_init_from_env(console_arg: Console | None = None) -> bool: config = create_config_manager() - # Install providers quietly - install_known_providers(config_manager=config, console=None, verbose=False) + # Install providers quietly. + # + # Capture why any install failed. This runs with verbose=False and + # console=None, so nothing is printed and the reason would otherwise + # only reach the log -- yet an install failure here is the most likely + # cause of the "module is not installed" condition detected on the very + # next line. Holding onto the reason lets that report name the actual + # cause instead of just the symptom. + install_failures: list[tuple[str, str]] = [] + install_known_providers( + config_manager=config, + console=None, + verbose=False, + failures_out=install_failures, + ) # Detect provider from environment module_id = detect_provider_from_env() @@ -318,6 +339,39 @@ 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). + # If the install attempt for this exact provider failed earlier in + # this same call, that failure IS the explanation. Report it next to + # the symptom rather than making the user re-run and go log-diving to + # discover it -- the message otherwise says "not installed" while the + # process that just tried to install it knew precisely why it didn't. + message = str(e) + install_reason = next( + ( + reason + for module_id, reason in install_failures + if module_id == e.provider_id + ), + None, + ) + if install_reason: + message = ( + f"{message}\n" + f" The install attempt for {e.provider_id} failed with: " + f"{install_reason}" + ) + + logger.error(f"Auto-init: {message}") + if console_arg: + console_arg.print(f"[bold red]\u2717 {escape_markup(message)}[/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..363a763a 100644 --- a/amplifier_app_cli/provider_env_detect.py +++ b/amplifier_app_cli/provider_env_detect.py @@ -3,6 +3,7 @@ import os from importlib.metadata import entry_points +from .provider_sources import is_provider_module_installed # Known credential env vars for each provider # Module name -> list of env vars that indicate the provider is configured @@ -15,35 +16,162 @@ "provider-ollama": [], # Ollama doesn't require credentials } +# Providers whose credential env vars are typically injected by the *platform* +# rather than deliberately exported by a user. +# +# Every other entry above is a variable somebody consciously set. Exporting +# ANTHROPIC_API_KEY is an explicit statement of intent, and that intent is +# exactly what makes silently overriding it (GAP-003) worth raising on. +# GITHUB_TOKEN is not that: GitHub Actions injects it into *every* job +# automatically, so its presence carries no user intent whatsoever. +# +# Treating it as intent is actively harmful. The GAP-003 raise conditions +# (credential present + module absent) are satisfied by default in any GitHub +# Actions run that doesn't happen to have provider-github-copilot installed -- +# which is most of them. Without this carve-out, the fix converts a working +# Ollama fallback into a hard failure across workflows nobody touched, and the +# error tells them to install a provider they never asked for. +# +# These providers are still selected normally when their module IS installed. +# An ambient credential simply cannot escalate a *missing* module into an +# error, because there is no user decision being overridden. +AMBIENT_CREDENTIAL_PROVIDERS: frozenset[str] = frozenset( + { + "provider-github-copilot", + } +) + + +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 + # Get installed provider modules. + # + # A registered entry point is necessary but NOT sufficient evidence that a + # provider is usable. Providers are installed editable, so anything that + # removes the module cache while leaving site-packages intact (notably + # `amplifier reset --remove cache` on a non-`uv tool` install) strands the + # `.dist-info` -- and therefore the entry point -- pointing at a directory + # that no longer exists. Such a provider still advertises itself but fails + # to import. + # + # That stranded state is exactly the one this fix exists to diagnose, and + # reading raw entry-point names would miss it: the provider would look + # installed, get selected, and fail later at import time with an error that + # says nothing about credentials. Worse, the message this module emits + # ("the module is not installed or could not be imported") would be a claim + # it never actually verified. `is_provider_module_installed()` resolves the + # entry point's module, so both the selection and the diagnostic are true. eps = entry_points(group="amplifier.modules") - installed_providers = {ep.name for ep in eps if ep.name.startswith("provider-")} + installed_providers = { + ep.name + for ep in eps + if ep.name.startswith("provider-") and is_provider_module_installed(ep.name) + } + + # 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. + # + # Exception: providers whose credentials are ambient (see + # AMBIENT_CREDENTIAL_PROVIDERS). A platform-injected token is not a + # user decision, so a missing module for one of them overrides + # nothing and must not be escalated. Skip it the way a provider with + # no credentials at all is skipped -- silently, leaving the Ollama + # fallback reachable. + if provider_id not in installed_providers: + if provider_id not in AMBIENT_CREDENTIAL_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/amplifier_app_cli/provider_sources.py b/amplifier_app_cli/provider_sources.py index 0e9f0467..3913db81 100644 --- a/amplifier_app_cli/provider_sources.py +++ b/amplifier_app_cli/provider_sources.py @@ -349,6 +349,7 @@ def install_known_providers( console: Console | None = None, verbose: bool = True, force: bool = False, + failures_out: list[tuple[str, str]] | None = None, ) -> list[str]: """Install known provider modules that are not already present. @@ -371,6 +372,17 @@ def install_known_providers( console: Optional Rich console for progress display verbose: Whether to show progress messages force: Reinstall providers even if they are already installed + failures_out: Optional list to receive ``(module_id, reason)`` pairs for + providers that failed to install. Purely additive -- the return + value is unchanged, so existing callers need no update. + + Without this, the reason a provider failed to install is written to + the log and then discarded. That matters downstream: when auto-init + later reports "the module is not installed", the actual cause (a + network failure, a bad source override, a broken build) is already + gone, leaving the user to guess. Callers that intend to explain a + missing provider can pass a list here and surface the real reason + alongside the symptom. Returns: List of provider module IDs that are available after this call @@ -447,6 +459,12 @@ def install_known_providers( f"\n[yellow]Warning: {len(failed)} provider(s) failed to install[/yellow]" ) + # Hand the failure reasons to a caller that asked for them. Everything + # else about this function's contract is unchanged -- callers that don't + # pass `failures_out` see identical behavior. + if failures_out is not None: + failures_out.extend(failed) + # Refresh Python's view of installed packages so they're immediately importable. # Without this, the current Python process won't see packages installed via subprocess. # This must be thorough - just invalidate_caches() is not enough for subprocess installs. diff --git a/tests/test_provider_env_detect.py b/tests/test_provider_env_detect.py new file mode 100644 index 00000000..030905e6 --- /dev/null +++ b/tests/test_provider_env_detect.py @@ -0,0 +1,280 @@ +"""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 + + +@pytest.fixture(autouse=True) +def _entry_points_resolve_by_default(): + """Treat every mocked entry point as a module that actually resolves. + + `detect_provider_from_env()` no longer trusts a bare entry-point name -- + it confirms the entry point's module still imports, because a stranded + `.dist-info` can advertise a provider whose files are gone. That check + consults the real interpreter, which would otherwise make every mocked + provider in this file look uninstalled regardless of the entry-point list + each test sets up. + + Patching it True by default preserves each test's intent: the mocked + entry-point list IS the set of installed providers. Tests that care about + the stranded case patch this again locally, and the inner patch wins. + """ + with patch( + "amplifier_app_cli.provider_env_detect.is_provider_module_installed", + return_value=True, + ): + yield + + +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_stranded_entry_point_is_treated_as_missing(self, monkeypatch): + """An entry point that exists but whose module no longer imports must + count as missing, not installed. + + Providers are installed editable, so removing the module cache while + leaving site-packages intact strands the `.dist-info` -- the provider + still advertises an entry point pointing at a directory that is gone. + Reading entry-point names alone would select this provider and fail + later at import time with an error that never mentions credentials. + """ + 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", + # The entry point IS registered -- this is the stranded case. + return_value=_mock_entry_points( + ["provider-anthropic", "provider-ollama"] + ), + ), + patch( + "amplifier_app_cli.provider_env_detect.is_provider_module_installed", + # ...but its module does not resolve. + side_effect=lambda name: name != "provider-anthropic", + ), + pytest.raises(CredentialedProviderModuleMissingError) as excinfo, + ): + detect_provider_from_env() + + assert excinfo.value.provider_id == "provider-anthropic" + + 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" + + +class TestAmbientCredentialsDoNotBlockFallback: + """GITHUB_TOKEN is injected by the platform, not chosen by the user, so + it must not escalate a missing module into a hard failure. + + GitHub Actions sets GITHUB_TOKEN in every job automatically. Combined + with the non-TTY environment that triggers auto-init, that satisfies + every GAP-003 raise condition by default in any workflow that doesn't + happen to have provider-github-copilot installed -- which is most of + them. Without the carve-out, a fix aimed at protecting a user's + deliberately-set API key instead breaks CI runs nobody touched, and + tells them to install a provider they never asked for. + + Note these tests deliberately set GITHUB_TOKEN *after* clearing the + environment. The shared `_clear_all_provider_env_vars()` helper strips + every var in PROVIDER_CREDENTIAL_VARS -- GITHUB_TOKEN included -- so a + test relying on it alone can never observe this behavior, which is + exactly why the regression went unnoticed. + """ + + def test_github_token_alone_still_falls_back_to_ollama(self, monkeypatch): + """The CI shape: ambient GITHUB_TOKEN, Copilot module absent, + Ollama installed. Must select Ollama quietly rather than raise.""" + _clear_all_provider_env_vars(monkeypatch) + monkeypatch.setenv("GITHUB_TOKEN", "ghs-ambient-ci-token") + with patch( + "amplifier_app_cli.provider_env_detect.entry_points", + # provider-github-copilot deliberately absent. + return_value=_mock_entry_points(["provider-ollama"]), + ): + assert detect_provider_from_env() == "provider-ollama" + + def test_github_token_alone_returns_none_without_ollama(self, monkeypatch): + """Same ambient token, nothing installed at all. Must return None -- + the pre-existing 'nothing configured' path -- not raise.""" + _clear_all_provider_env_vars(monkeypatch) + monkeypatch.setenv("GITHUB_TOKEN", "ghs-ambient-ci-token") + with patch( + "amplifier_app_cli.provider_env_detect.entry_points", + return_value=_mock_entry_points([]), + ): + assert detect_provider_from_env() is None + + def test_github_copilot_still_selected_when_module_installed(self, monkeypatch): + """The carve-out must not disable the provider. When the module IS + installed, a GITHUB_TOKEN-credentialed Copilot is still selectable.""" + _clear_all_provider_env_vars(monkeypatch) + monkeypatch.setenv("GITHUB_TOKEN", "ghs-ambient-ci-token") + with patch( + "amplifier_app_cli.provider_env_detect.entry_points", + return_value=_mock_entry_points( + ["provider-github-copilot", "provider-ollama"] + ), + ): + assert detect_provider_from_env() == "provider-github-copilot" + + def test_user_set_credential_still_raises_alongside_ambient_token( + self, monkeypatch + ): + """The carve-out is scoped to the ambient var only. A deliberately-set + ANTHROPIC_API_KEY must still raise even when GITHUB_TOKEN is also + present -- otherwise the CI fix would silently undo GAP-003.""" + _clear_all_provider_env_vars(monkeypatch) + monkeypatch.setenv("GITHUB_TOKEN", "ghs-ambient-ci-token") + 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-ollama"]), + ), + pytest.raises(CredentialedProviderModuleMissingError) as excinfo, + ): + detect_provider_from_env() + + assert excinfo.value.provider_id == "provider-anthropic"