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 1/2] 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" From 4e331c09353fb9b37f1be1c1b2763b1215f22cf0 Mon Sep 17 00:00:00 2001 From: sadlilas <11658960+sadlilas@users.noreply.github.com> Date: Mon, 17 Aug 2026 22:14:26 -0700 Subject: [PATCH 2/2] fix(provider-detect): exempt ambient credentials, verify module resolves, surface install failure reason MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Split provider credential table into 'chosen' vs 'ambient' credentials. GitHub Actions injects GITHUB_TOKEN into every job automatically — a platform-provided token is not a user decision. Missing the copilot module should not block the Ollama fallback, so ambient credentials from missing modules are now skipped silently. - Replaced entry-point-name check with is_provider_module_installed() so stranded .dist-info (entry registered but module files missing — common with editable installs) is correctly treated as not installed rather than selected and then failing at import time. - Added optional failures_out parameter to install_known_providers() to make install-failure reasons recoverable. Auto-init runs install with verbose=False/console=None, so real causes (network, bad source override, broken build) previously went only to log and were discarded. The GAP-003 error path now appends actual failure reason when it matches the provider in question. Existing callers unaffected. - Added regression tests: autouse fixture to keep mocked entry-point list current with new resolution check; stranded-entry-point test; TestAmbientCredentialsDoNotBlockFallback covering GITHUB_TOKEN-alone fallback, None return when nothing installed, Copilot selectable when installed, and ANTHROPIC_API_KEY still raising alongside ambient GITHUB_TOKEN (proving carve-out did not silently undo GAP-003). 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 | 47 ++++++++- amplifier_app_cli/provider_env_detect.py | 61 ++++++++++- amplifier_app_cli/provider_sources.py | 18 ++++ tests/test_provider_env_detect.py | 129 +++++++++++++++++++++++ 4 files changed, 248 insertions(+), 7 deletions(-) diff --git a/amplifier_app_cli/commands/init.py b/amplifier_app_cli/commands/init.py index 3d3b68be..355115d5 100644 --- a/amplifier_app_cli/commands/init.py +++ b/amplifier_app_cli/commands/init.py @@ -23,6 +23,7 @@ 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 @@ -269,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, " @@ -277,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() @@ -328,9 +346,30 @@ def auto_init_from_env(console_arg: Console | None = None) -> bool: # 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 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 {e}[/bold red]") + console_arg.print(f"[bold red]\u2717 {escape_markup(message)}[/bold red]") return False except Exception as e: diff --git a/amplifier_app_cli/provider_env_detect.py b/amplifier_app_cli/provider_env_detect.py index 73b0895a..363a763a 100644 --- a/amplifier_app_cli/provider_env_detect.py +++ b/amplifier_app_cli/provider_env_detect.py @@ -3,6 +3,8 @@ 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 PROVIDER_CREDENTIAL_VARS: dict[str, list[str]] = { @@ -14,6 +16,31 @@ "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 @@ -75,9 +102,29 @@ def detect_provider_from_env() -> str | None: 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 @@ -103,8 +150,16 @@ def detect_provider_from_env() -> str | None: # 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: - missing_but_credentialed.append((provider_id, env_vars)) + if provider_id not in AMBIENT_CREDENTIAL_PROVIDERS: + missing_but_credentialed.append((provider_id, env_vars)) continue return provider_id 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 index 26a32eb1..030905e6 100644 --- a/tests/test_provider_env_detect.py +++ b/tests/test_provider_env_detect.py @@ -35,6 +35,28 @@ def _mock_entry_points(names: list[str]): 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 @@ -129,6 +151,37 @@ def test_falls_through_to_second_credentialed_installed_provider(self, monkeypat ): 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 ): @@ -149,3 +202,79 @@ def test_does_not_reach_ollama_when_credentialed_provider_missing( # 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"