diff --git a/amplifier_app_cli/commands/provider.py b/amplifier_app_cli/commands/provider.py index 8b9fb2e..450e114 100644 --- a/amplifier_app_cli/commands/provider.py +++ b/amplifier_app_cli/commands/provider.py @@ -19,17 +19,22 @@ _config_claimed_env_vars, _normalize_id, _preserve_reserved_keys, + _run_provider_login, _secret_env_var_for, _secret_field_id_for, _suggest_instance_env_var, configure_provider, ) +from ..provider_loader import _try_instantiate_provider +from ..provider_loader import get_provider_info from ..provider_loader import get_provider_models +from ..provider_loader import load_provider_class from ..provider_manager import ProviderManager from ..provider_manager import resolve_provider_entry from ..provider_sources import ensure_provider_installed from ..provider_sources import get_effective_provider_sources from ..provider_sources import install_known_providers +from ..provider_sources import is_provider_module_installed from ..ui.item_renderer import ItemRenderer from ..ui.scope import ( is_scope_change_available, @@ -283,8 +288,7 @@ def _prompt_credential_binding( f"model/settings [dim](default)[/dim]" ) console.print( - " [2] Separate credential -- its own env var (different " - "account/org/key)" + " [2] Separate credential -- its own env var (different account/org/key)" ) choice = Prompt.ask(" Choice", choices=["1", "2"], default="1") @@ -1183,6 +1187,109 @@ def provider_models(ctx: click.Context, provider_id: str | None) -> None: console.print(table) +# ============================================================ +# provider login +# ============================================================ + + +@provider.command("login") +@click.argument("provider_id") +@click.pass_context +def provider_login(ctx: click.Context, provider_id: str) -> None: + """Log in to an OAuth-capable provider. + + Runs the same browser-login flow the configuration wizard offers for + a provider that supports it (declares an "auth:*" capability in its + get_info()). Useful to (re)authenticate later, or if login was + skipped/declined during `provider add` or `provider edit`. + + Examples: + amplifier provider login openai-chatgpt + """ + module_id = _normalize_module_id(provider_id) + display = _display_name(module_id) + + if not is_provider_module_installed(module_id): + console.print(f"[red]Provider '{display}' is not installed.[/red]") + console.print( + f"\nInstall it first with: [cyan]amplifier provider install {display}[/cyan]" + ) + ctx.exit(1) + + info = get_provider_info(module_id) + if info is None: + console.print(f"[red]Error: Could not load provider '{display}'.[/red]") + ctx.exit(1) + + capabilities = info.get("capabilities") or [] + if not any(str(c).startswith("auth:") for c in capabilities): + console.print( + f"[yellow]'{display}' uses API-key configuration -- see " + f"`amplifier provider edit {display}`.[/yellow]" + ) + return + + provider_class = load_provider_class(module_id) + if provider_class is None: + console.print(f"[red]Error: Could not load provider '{display}'.[/red]") + ctx.exit(1) + + # Instantiate with the saved config for this instance when one + # exists, so login uses the same connection values the wizard/runtime + # would -- empty dict (not an error) when the provider has never been + # configured yet. + settings = _get_settings() + entry = _find_provider_entry(settings.get_provider_overrides(), provider_id) + stored_config = entry.get("config", {}) if entry else {} + + provider_instance = _try_instantiate_provider(provider_class, stored_config) + if provider_instance is None: + console.print(f"[red]Error: Could not instantiate provider '{display}'.[/red]") + ctx.exit(1) + + if not ( + hasattr(provider_instance, "auth_status") + and hasattr(provider_instance, "login") + ): + console.print( + f"[yellow]'{display}' uses API-key configuration -- see " + f"`amplifier provider edit {display}`.[/yellow]" + ) + return + + try: + status = provider_instance.auth_status() + except Exception as e: + console.print( + f"[red]Could not check login status: {escape_markup(str(e))}[/red]" + ) + ctx.exit(1) + + console.print(f"Current status: [bold]{escape_markup(str(status))}[/bold]") + if status == "authenticated": + console.print(f"[green]✓ Already logged in to {display}[/green]") + return + + success = _run_provider_login(provider_instance) + + try: + final_status = provider_instance.auth_status() + except Exception: + final_status = "unauthenticated" if not success else status + + if success: + console.print( + f"[green]✓ Logged in to {display} " + f"({escape_markup(str(final_status))})[/green]" + ) + else: + console.print( + f"[red]✗ Login failed for {display} " + f"({escape_markup(str(final_status))})[/red]" + ) + ctx.exit(1) + + # ============================================================ # Task 1: provider manage — interactive dashboard # ============================================================ diff --git a/amplifier_app_cli/main.py b/amplifier_app_cli/main.py index 53e246f..bfe2f80 100644 --- a/amplifier_app_cli/main.py +++ b/amplifier_app_cli/main.py @@ -182,6 +182,66 @@ def _ensure_utf8_output() -> None: pass # Not a real Windows console (e.g. some CI/test environments) +def _configure_console_logging() -> None: + """Install a minimal root logging handler if the process has none. + + amplifier-app-cli never calls logging.basicConfig() / logging.config + .dictConfig() / Logger.addHandler() anywhere in the codebase (verified). + With zero handlers on the root logger, any module's + ``logger.warning(..., exc_info=True)`` or ``logger.exception(...)`` + falls through to Python's ``logging.lastResort`` handler, which dumps + the bare message AND the full traceback straight to stderr with no + formatting control. That is exactly how the owner's raw + AuthenticationError traceback leaked to the terminal during + `provider add openai-chatgpt`'s model fetch -- and it is not specific + to that one provider or that one call site; it is a structural gap + that leaks a traceback from *any* module that logs an exception. + + Installs a single stderr ``StreamHandler`` at ``WARNING`` with a plain + ``"%(message)s"`` formatter (no timestamp/logger-name noise -- this is + a CLI, not a service) and, unless verbose/debug output was requested, + a filter that clears ``exc_info``/``exc_text`` on each record so a + logged exception still prints its message but never dumps a raw + traceback into the user's terminal. Verbose/debug mode leaves + tracebacks intact for debugging. + + Must be called from main() before ``_attach_llm_error_filter()``: that + function's primary path (attach to an existing stderr StreamHandler) + only works once a real handler exists -- see its own docstring. + Without this, it fell back to filtering at the root *logger* level, + which is inert for any record emitted by a named child logger (the + normal case): ``Logger.callHandlers()`` walks handlers up the + hierarchy and checks each handler's own filters, but a logger-level + filter is only consulted on the logger that originated the record. + + A no-op if the root logger already has a handler (e.g. under pytest, + or if some future code path configures logging itself) -- this never + overrides an existing setup. + """ + root = logging.getLogger() + if root.handlers: + return + + verbose_requested = any( + arg in ("--verbose", "-v", "--debug") for arg in sys.argv[1:] + ) + + handler = logging.StreamHandler(sys.stderr) + handler.setLevel(logging.WARNING) + handler.setFormatter(logging.Formatter("%(message)s")) + + if not verbose_requested: + + def _suppress_traceback(record: logging.LogRecord) -> bool: + record.exc_info = None + record.exc_text = None + return True + + handler.addFilter(_suppress_traceback) + + root.addHandler(handler) + + def _attach_llm_error_filter() -> None: """Attach the LLM error filter to the stderr StreamHandler at runtime. @@ -3538,10 +3598,12 @@ async def interactive_chat( # tests miss it because they mock _create_prompt_session. try: prompt_session = _create_prompt_session( - get_active_mode=lambda: command_processor.session.coordinator.session_state.get( - "active_mode" + get_active_mode=lambda: ( + command_processor.session.coordinator.session_state.get("active_mode") + ), + get_pinned_provider=lambda: _pinned_provider_name( + command_processor.session ), - get_pinned_provider=lambda: _pinned_provider_name(command_processor.session), ) except _TERMINAL_UNUSABLE_ERRORS as e: _report_terminal_unusable(e, verbose=verbose) @@ -4529,6 +4591,7 @@ def _goal_sigint_handler(signum, frame): def main(): """Main entry point.""" _ensure_utf8_output() + _configure_console_logging() _attach_llm_error_filter() cli() diff --git a/amplifier_app_cli/provider_config_utils.py b/amplifier_app_cli/provider_config_utils.py index 96b60ae..b29eedf 100644 --- a/amplifier_app_cli/provider_config_utils.py +++ b/amplifier_app_cli/provider_config_utils.py @@ -4,6 +4,7 @@ Queries provider modules dynamically for model lists and config fields. """ +import asyncio import logging import os import re @@ -20,8 +21,11 @@ from .key_manager import KeyManager from .lib.settings import AppSettings from .lib.settings import Scope +from .provider_loader import _try_instantiate_provider from .provider_loader import get_provider_info from .provider_loader import get_provider_models +from .provider_loader import list_models_for_instance +from .provider_loader import load_provider_class console = Console() logger = logging.getLogger(__name__) @@ -188,6 +192,161 @@ def _prompt_model_selection( return None +def _run_provider_login(provider: Any) -> bool: + """Drive an already-instantiated provider's login() flow. + + Renders whatever the provider's login() prints (device-code URL, + instructions, etc.) through this module's rich console via a + ``print_fn`` callback -- the provider itself has no console of its + own. Shared between the configuration wizard's login step (see + ``_maybe_login_provider()``) and the standalone + ``amplifier provider login `` command, so the two call sites can + never diverge on how a provider's ``login()`` is invoked. + + Args: + provider: An already-instantiated provider with a ``login`` + attribute (sync or async), taking an optional ``print_fn`` + keyword argument -- see the duck-typed auth contract in + ``_maybe_login_provider()``'s docstring. + + Returns: + True if ``login()`` reported success, False on a graceful + failure. Never raises for a login-specific error -- prints one + yellow message and returns False instead of a raw traceback. + + Note: + Ctrl-C / EOFError during login are deliberately NOT caught here; + they propagate to the caller so the same strict abort semantics + from the model-selection fix apply uniformly (see + configure_provider()'s outer ``except (KeyboardInterrupt, + EOFError)`` handler). + """ + + def _print_fn(message: str) -> None: + console.print(message) + + login_fn = provider.login + try: + if asyncio.iscoroutinefunction(login_fn): + result = asyncio.run(login_fn(print_fn=_print_fn)) + else: + result = login_fn(print_fn=_print_fn) + except (KeyboardInterrupt, EOFError): + raise + except Exception as e: + console.print(f"[yellow]Login failed: {escape(str(e))}[/yellow]") + return False + return bool(result) + + +def _safely_fetch_models_from_instance(provider_id: str, provider: Any) -> list: + """Fetch models from an already-instantiated provider, with the exact + same connectivity/generic-exception safety net + ``_prompt_model_selection()`` uses for its own (self-instantiating) + fetch path -- so a login-time prefetch can never leak a raw + traceback (e.g. an AuthenticationError) the way the pre-fix + onboarding flow did. + """ + try: + return list_models_for_instance(provider) + except (ConnectionError, OSError) as e: + logger.debug(f"Could not connect to provider '{provider_id}': {e}") + return [] + except Exception as e: + console.print( + f"\n [yellow]⚠ Could not fetch models for '{escape(str(provider_id))}':[/yellow]" + f"\n\n {escape(str(e))}\n" + ) + return [] + + +def _maybe_login_provider( + provider_id: str, + info: dict[str, Any], + collected_config: dict[str, Any], +) -> Any | None: + """Offer a one-time interactive login for a provider that declares an + ``"auth:*"`` capability (e.g. ``"auth:oauth-device-code"``), then + return the instantiated provider instance so the caller can reuse it + for model fetching -- avoiding a second, separate instantiation. + + Duck-types ``auth_status()``/``login()`` via ``hasattr`` so this + function -- and configure_provider()'s wizard step that calls it -- + merges and runs safely independent of whichever provider module PR + actually adds those methods and the capability string. A provider + that declares the capability but doesn't (yet) implement the methods + is treated exactly like one with no login flow at all (the instance + is still returned for model-fetch reuse; no login prompt is shown). + + Args: + provider_id: Provider ID (e.g. "openai-chatgpt"). + info: The provider's get_info() dict (from get_provider_info()). + collected_config: Config values collected so far in Phase 1 + (base_url, host, etc.) -- passed through to instantiation so + the same connection values are used as the eventual model + fetch would use. + + Returns: + The instantiated provider instance if one could be created + (regardless of whether login ran, was declined, or failed), or + None if this provider declares no ``"auth:*"`` capability, or if + it could not be instantiated at all. In the None case the caller + must fall back to its normal (already-safe) model-fetch path. + + Note: + Ctrl-C / EOFError raised from the confirmation prompt are + deliberately NOT caught here -- they propagate up to + configure_provider()'s own outer except handler, landing the same + clean "Cancelled." abort as any other prompt in the wizard (see + the model-selection Ctrl-C fix). + """ + capabilities = info.get("capabilities") or [] + if not any(str(c).startswith("auth:") for c in capabilities): + return None + + provider_class = load_provider_class(provider_id) + if provider_class is None: + return None + provider = _try_instantiate_provider(provider_class, collected_config) + if provider is None: + return None + + if not (hasattr(provider, "auth_status") and hasattr(provider, "login")): + return provider + + try: + status = provider.auth_status() + except Exception: + # A broken auth_status() must never crash the wizard -- treat it + # like "no login capability" and let the caller proceed with + # whatever model list it can otherwise get. + return provider + + if status == "authenticated": + return provider + + display_name = info.get("display_name", provider_id) + console.print() + proceed = Confirm.ask( + f"[bold]{escape(str(display_name))}[/bold] requires a one-time " + "browser login. Start it now?", + default=True, + ) + if not proceed: + console.print( + "[yellow]Skipping login -- model list may be limited; run " + f"`amplifier provider login {escape(str(provider_id))}` later[/yellow]" + ) + return provider + + if not _run_provider_login(provider): + console.print( + "[yellow]Skipping login -- model list may be limited; run " + f"`amplifier provider login {escape(str(provider_id))}` later[/yellow]" + ) + return provider + + def _should_show_field(field: dict[str, Any], collected_config: dict[str, Any]) -> bool: """Check if a field should be shown based on show_when conditions. @@ -283,10 +442,18 @@ def _sanitize_env_token(value: str) -> str: return re.sub(r"[^A-Za-z0-9]+", "_", value).strip("_").upper() -def _secret_config_field(module_id: str) -> dict[str, Any] | None: - """Return the provider type's secret ConfigField dict - (``field_type == "secret"``), if any.""" - info = get_provider_info(module_id) +def _secret_config_field_from_info( + info: dict[str, Any] | None, +) -> dict[str, Any] | None: + """Return the secret ConfigField dict (``field_type == "secret"``) from + an already-resolved provider info dict, if any. + + Split out from ``_secret_config_field()`` so callers that must + distinguish "module unresolvable" (``info is None``) from "resolved, + but this provider has no secret field" (e.g. OAuth-based providers) can + call ``get_provider_info()`` once themselves and reuse the result -- + see ``normalize_provider_secrets()``. + """ if not info: return None for field in info.get("config_fields", []): @@ -295,6 +462,12 @@ def _secret_config_field(module_id: str) -> dict[str, Any] | None: return None +def _secret_config_field(module_id: str) -> dict[str, Any] | None: + """Return the provider type's secret ConfigField dict + (``field_type == "secret"``), if any.""" + return _secret_config_field_from_info(get_provider_info(module_id)) + + def _secret_env_var_for(module_id: str) -> str | None: """Default env var of the provider type's secret ConfigField (field_type == 'secret'), i.e. the collision-prone name.""" @@ -310,6 +483,13 @@ def _secret_field_id_for(module_id: str) -> str | None: return field.get("id") if field else None +def _secret_field_id_for_info(info: dict[str, Any] | None) -> str | None: + """Same as ``_secret_field_id_for()``, but from an already-resolved + provider info dict -- see ``_secret_config_field_from_info()``.""" + field = _secret_config_field_from_info(info) + return field.get("id") if field else None + + def _config_claimed_env_vars(settings: AppSettings) -> set[str]: """Env-var names claimed by an existing *configured instance*: names referenced by a ``${VAR}`` placeholder in some scope's provider config @@ -492,8 +672,13 @@ def normalize_provider_secrets( continue module_id = raw_module_id - field_id = _secret_field_id_for(module_id) - if field_id is None: + # Hoisted once so a genuine resolution failure (module can't be + # loaded/instantiated -- e.g. a stale/broken install) and the + # normal case of a provider with no secret ConfigField at all + # (OAuth-based providers) don't both call get_provider_info() and + # don't get conflated into the same loud warning. + provider_info = get_provider_info(module_id) + if provider_info is None: unresolved_label = raw_entry_id or module_id console.print( f"[yellow]\u26a0 Could not resolve provider module " @@ -504,6 +689,21 @@ def normalize_provider_secrets( continue entry_label: str = str(raw_entry_id) if raw_entry_id else module_id + field_id = _secret_field_id_for_info(provider_info) + if field_id is None: + # Not a failure -- this provider type simply has no secret + # ConfigField to scan (e.g. an OAuth-based provider with no + # api_key field at all). Debug-only; never a user-facing + # warning for the normal case. + logger.debug( + "normalize_provider_secrets: provider module '%s' (entry " + "'%s') has no secret ConfigField -- skipping " + "plaintext-secret scan for this entry.", + module_id, + entry_label, + ) + continue + entry_config = entry.get("config") or {} value = entry_config.get(field_id) if not value: @@ -933,13 +1133,48 @@ def configure_provider( existing_config.get("default_model") if existing_config else None ) + # Wizard login step: for a provider that declares an "auth:*" + # capability (e.g. "auth:oauth-device-code"), offer a + # one-time browser login BEFORE fetching models, so a + # first-time user gets the live model catalog instead of a + # stale/empty fallback list. Reuses ONE provider instance for + # both the login check and the model fetch below -- see + # _maybe_login_provider()'s docstring. A provider with no + # "auth:*" capability (the common case today) is unaffected: + # login_provider is None and prefetched_models stays None, so + # _prompt_model_selection() below does its own (already-safe) + # fetch exactly as before. + prefetched_models = None + login_provider = _maybe_login_provider(provider_id, info, collected_config) + if login_provider is not None: + with console.status( + "[dim]Fetching available models...[/dim]", spinner="dots" + ): + prefetched_models = _safely_fetch_models_from_instance( + provider_id, login_provider + ) + # Prompt for model selection # Pass collected_config so providers can connect to real servers for dynamic discovery console.print() console.print("[bold]Default Model[/bold]") selected_model = _prompt_model_selection( - provider_id, default_model, collected_config + provider_id, + default_model, + collected_config, + models=prefetched_models, ) + # None is the strict abort sentinel (Ctrl-C / EOF during the + # Choice prompt -- see _prompt_model_selection's docstring). It + # is NOT the same as "" (user declined to enter a custom model + # name, which is a valid "continue without a model" outcome). + # Without this check, an interrupted model prompt fell through + # to Phase 3, printed "configured", and saved an empty/partial + # config -- matching the outer handler at the bottom of this + # function and the precedent in commands/routing.py. + if selected_model is None: + console.print("\n[dim]Cancelled.[/dim]") + return None if selected_model: collected_config["default_model"] = selected_model diff --git a/amplifier_app_cli/provider_loader.py b/amplifier_app_cli/provider_loader.py index 3f47c14..fffc3e7 100644 --- a/amplifier_app_cli/provider_loader.py +++ b/amplifier_app_cli/provider_loader.py @@ -165,9 +165,36 @@ def get_provider_models( ) return [] - # Check if provider has list_models + return list_models_for_instance(provider) + + +def list_models_for_instance(provider: Any) -> list["ModelInfo"]: + """Call list_models() on an ALREADY-INSTANTIATED provider instance. + + Extracted from get_provider_models() so a caller that already holds a + live provider instance for another reason (e.g. the configuration + wizard's login step, which must reuse one instance across the login + check and the subsequent model fetch -- see + provider_config_utils._maybe_login_provider()) doesn't have to + instantiate a second, separate provider just to list its models. + get_provider_models() itself is unchanged in behavior; it now just + delegates the "call list_models and clean up" part here. + + Args: + provider: An already-instantiated provider object. + + Returns: + List of ModelInfo for available models, empty list if the + instance has no list_models(). + + Raises: + Exception: Re-raises whatever list_models() raises (auth errors, + API errors, connection errors) so callers can display + meaningful error messages -- same contract as + get_provider_models(). + """ if not hasattr(provider, "list_models"): - logger.debug(f"Provider '{provider_id}' does not have list_models()") + logger.debug(f"Provider {provider!r} does not have list_models()") return [] # Call list_models (may be sync or async) via the shared invocation @@ -175,9 +202,11 @@ def get_provider_models( # list_models, async-aware" mechanic the in-session /provider # test|models slash commands use against already-mounted providers, so # the two surfaces can't quietly diverge on how a provider is asked for - # its models. This function still owns instantiation (above) and - # cleanup (below) itself, since only it knows this provider instance is - # disposable -- a mounted session provider must never be closed. + # its models. This function still owns cleanup (below) itself for the + # async path, since only the caller knows whether this instance is + # disposable -- a mounted session provider must never be closed here, + # so callers that pass in a long-lived instance should be aware a + # coroutine list_models() will still trigger a best-effort close(). # Let exceptions propagate - auth errors, API errors, connection errors # should be shown to the user, not silently swallowed list_models_fn = provider.list_models @@ -335,4 +364,9 @@ def get_provider_info(provider_id: str) -> dict[str, Any] | None: return None -__all__ = ["load_provider_class", "get_provider_models", "get_provider_info"] +__all__ = [ + "load_provider_class", + "get_provider_models", + "get_provider_info", + "list_models_for_instance", +] diff --git a/amplifier_app_cli/provider_manager.py b/amplifier_app_cli/provider_manager.py index 412e14f..0773d3e 100644 --- a/amplifier_app_cli/provider_manager.py +++ b/amplifier_app_cli/provider_manager.py @@ -22,6 +22,7 @@ _PROVIDER_DISPLAY_NAMES = { "anthropic": "Anthropic", "openai": "OpenAI", + "openai-chatgpt": "OpenAI ChatGPT", "azure-openai": "Azure OpenAI", "gemini": "Google Gemini", "ollama": "Ollama", diff --git a/amplifier_app_cli/provider_sources.py b/amplifier_app_cli/provider_sources.py index 3913db8..995e3d6 100644 --- a/amplifier_app_cli/provider_sources.py +++ b/amplifier_app_cli/provider_sources.py @@ -27,6 +27,7 @@ "provider-github-copilot": "git+https://github.com/microsoft/amplifier-module-provider-github-copilot@main", "provider-ollama": "git+https://github.com/microsoft/amplifier-module-provider-ollama@main", "provider-openai": "git+https://github.com/microsoft/amplifier-module-provider-openai@main", + "provider-openai-chatgpt": "git+https://github.com/microsoft/amplifier-module-provider-openai-chatgpt@main", "provider-vllm": "git+https://github.com/microsoft/amplifier-module-provider-vllm@main", } diff --git a/tests/test_configure_console_logging.py b/tests/test_configure_console_logging.py new file mode 100644 index 0000000..3902c6d --- /dev/null +++ b/tests/test_configure_console_logging.py @@ -0,0 +1,238 @@ +"""Tests for _configure_console_logging() -- the fix for the traceback-leak +class of onboarding defects. + +amplifier-app-cli never calls logging.basicConfig()/dictConfig()/ +addHandler() anywhere. With zero handlers on the root logger, Python's +logging.lastResort dumps bare messages AND full tracebacks from any +module's `logger.warning(..., exc_info=True)` straight to stderr. This is +exactly how the owner's raw AuthenticationError traceback leaked during +`provider add openai-chatgpt`'s model fetch. + +_configure_console_logging() installs a real stderr handler so: +1. Tracebacks are suppressed by default (unless verbose/debug requested). +2. _attach_llm_error_filter()'s primary "attach to existing stderr + handler" path actually fires, instead of silently falling back to the + inert root-logger-filter path (a logger-level filter never fires for + records emitted by a *child* logger -- see that function's docstring). + +These tests use an ISOLATED Logger instance (patched in place of +logging.getLogger()) rather than the true root logger, because pytest's +own log-capture plugin keeps a handler on the real root logger for the +duration of every test -- manipulating root.handlers directly is not +reliable under pytest. +""" + +import io +import logging +import sys +from unittest.mock import patch + +import pytest + +from amplifier_app_cli.main import _configure_console_logging + + +def _isolated_logger(name: str) -> logging.Logger: + """A fresh, unpropagated Logger standing in for "the root logger" for + a single test, so pytest's own log-capture handler never interferes.""" + logger = logging.Logger(name) + logger.handlers = [] + logger.filters = [] + return logger + + +class TestConfigureConsoleLogging: + """Verify handler installation and no-op-when-already-configured.""" + + def setup_method(self) -> None: + self._orig_argv = sys.argv[:] + + def teardown_method(self) -> None: + sys.argv = self._orig_argv + + def test_installs_stderr_handler_when_none_exists(self) -> None: + fake_root = _isolated_logger("test-installs-handler") + sys.argv = ["amplifier", "provider", "add", "openai-chatgpt"] + + with patch("amplifier_app_cli.main.logging.getLogger", return_value=fake_root): + _configure_console_logging() + + assert len(fake_root.handlers) == 1 + handler = fake_root.handlers[0] + assert isinstance(handler, logging.StreamHandler) + assert handler.stream is sys.stderr + assert handler.level == logging.WARNING + + def test_noop_when_handler_already_present(self) -> None: + fake_root = _isolated_logger("test-noop") + existing = logging.StreamHandler(sys.stderr) + fake_root.handlers = [existing] + sys.argv = ["amplifier", "run"] + + with patch("amplifier_app_cli.main.logging.getLogger", return_value=fake_root): + _configure_console_logging() + + assert fake_root.handlers == [existing], ( + "Must never override an existing logging setup" + ) + + def test_suppresses_traceback_by_default(self) -> None: + """The installed handler must strip exc_info/exc_text from a + record unless verbose/debug was requested on the command line.""" + fake_root = _isolated_logger("test-suppress-default") + sys.argv = ["amplifier", "provider", "add", "openai-chatgpt"] + + with patch("amplifier_app_cli.main.logging.getLogger", return_value=fake_root): + _configure_console_logging() + + handler = fake_root.handlers[0] + try: + raise ValueError("boom") + except ValueError: + record = logging.getLogger( + "amplifier_module_provider_openai_chatgpt" + ).makeRecord( + name="amplifier_module_provider_openai_chatgpt", + level=logging.WARNING, + fn="provider.py", + lno=1, + msg="Could not fetch models: %s", + args=("boom",), + exc_info=sys.exc_info(), + ) + assert record.exc_info is not None # sanity: it started populated + assert handler.filter(record) + assert record.exc_info is None, ( + "Traceback info must be stripped from the record by default" + ) + assert record.exc_text is None + + @pytest.mark.parametrize("flag", ["--verbose", "-v", "--debug"]) + def test_preserves_traceback_when_verbose_requested(self, flag: str) -> None: + fake_root = _isolated_logger(f"test-preserve-{flag}") + sys.argv = ["amplifier", "provider", "add", "openai-chatgpt", flag] + + with patch("amplifier_app_cli.main.logging.getLogger", return_value=fake_root): + _configure_console_logging() + + handler = fake_root.handlers[0] + try: + raise ValueError("boom") + except ValueError: + record = logging.getLogger( + "amplifier_module_provider_openai_chatgpt" + ).makeRecord( + name="amplifier_module_provider_openai_chatgpt", + level=logging.WARNING, + fn="provider.py", + lno=1, + msg="Could not fetch models: %s", + args=("boom",), + exc_info=sys.exc_info(), + ) + assert handler.filter(record) + assert record.exc_info is not None, ( + "Traceback info must be preserved when --verbose/-v/--debug is requested" + ) + + def test_message_only_formatter(self) -> None: + fake_root = _isolated_logger("test-formatter") + sys.argv = ["amplifier"] + + with patch("amplifier_app_cli.main.logging.getLogger", return_value=fake_root): + _configure_console_logging() + + handler = fake_root.handlers[0] + assert handler.formatter is not None + assert handler.formatter._fmt == "%(message)s" + + +class TestConfigureConsoleLoggingEnablesLlmErrorFilterAttachment: + """Integration: once a real handler exists, _attach_llm_error_filter() + must take its primary path (attach to the handler), not the inert + root-logger fallback.""" + + def setup_method(self) -> None: + self._orig_argv = sys.argv[:] + + def teardown_method(self) -> None: + sys.argv = self._orig_argv + + def test_llm_error_filter_attaches_to_handler_not_root(self) -> None: + from amplifier_app_cli.main import _attach_llm_error_filter, _llm_error_filter + + fake_root = _isolated_logger("test-llm-filter-attach") + sys.argv = ["amplifier", "provider", "add", "openai-chatgpt"] + + with patch("amplifier_app_cli.main.logging.getLogger", return_value=fake_root): + _configure_console_logging() + _attach_llm_error_filter() + + assert len(fake_root.handlers) == 1 + handler = fake_root.handlers[0] + assert _llm_error_filter in handler.filters, ( + "LLMErrorLogFilter must land on the real stderr handler " + "installed by _configure_console_logging(), not fall back " + "to the inert root-logger filter path" + ) + assert _llm_error_filter not in fake_root.filters + + +class TestTracebackSuppressionEndToEnd: + """A record actually written through the handler must not contain a + traceback in the emitted output, by default.""" + + def setup_method(self) -> None: + self._orig_argv = sys.argv[:] + + def teardown_method(self) -> None: + sys.argv = self._orig_argv + + def test_logged_exception_reaches_stderr_without_traceback(self) -> None: + fake_root = _isolated_logger("test-e2e-no-verbose") + sys.argv = ["amplifier", "provider", "add", "openai-chatgpt"] + + with patch("amplifier_app_cli.main.logging.getLogger", return_value=fake_root): + _configure_console_logging() + + # Redirect the installed handler's stream to a buffer we can inspect. + buf = io.StringIO() + fake_root.handlers[0].stream = buf + + logger = logging.Logger("amplifier_module_provider_openai_chatgpt") + logger.handlers = [] + logger.parent = fake_root + logger.setLevel(logging.WARNING) + try: + raise RuntimeError("AuthenticationError: invalid api key") + except RuntimeError: + logger.warning("Could not fetch models: %s", "auth failed", exc_info=True) + + output = buf.getvalue() + assert "Could not fetch models: auth failed" in output + assert "Traceback" not in output + assert "RuntimeError" not in output + + def test_logged_exception_reaches_stderr_with_traceback_when_verbose( + self, + ) -> None: + fake_root = _isolated_logger("test-e2e-verbose") + sys.argv = ["amplifier", "provider", "add", "openai-chatgpt", "--verbose"] + + with patch("amplifier_app_cli.main.logging.getLogger", return_value=fake_root): + _configure_console_logging() + + buf = io.StringIO() + fake_root.handlers[0].stream = buf + + logger = logging.Logger("amplifier_module_provider_openai_chatgpt") + logger.handlers = [] + logger.parent = fake_root + logger.setLevel(logging.WARNING) + try: + raise RuntimeError("AuthenticationError: invalid api key") + except RuntimeError: + logger.warning("Could not fetch models: %s", "auth failed", exc_info=True) + + output = buf.getvalue() + assert "Traceback" in output diff --git a/tests/test_discover_providers_clean_install.py b/tests/test_discover_providers_clean_install.py index 90908c1..95b76da 100644 --- a/tests/test_discover_providers_clean_install.py +++ b/tests/test_discover_providers_clean_install.py @@ -182,6 +182,7 @@ def test_provider_display_names_are_correct_for_all_fallbacks(self): expected_names = { "provider-anthropic": "Anthropic", "provider-openai": "OpenAI", + "provider-openai-chatgpt": "OpenAI ChatGPT", "provider-azure-openai": "Azure OpenAI", "provider-chat-completions": "OpenAI-Compatible", "provider-gemini": "Google Gemini", diff --git a/tests/test_normalize_provider_secrets_warning.py b/tests/test_normalize_provider_secrets_warning.py new file mode 100644 index 0000000..dc03a10 --- /dev/null +++ b/tests/test_normalize_provider_secrets_warning.py @@ -0,0 +1,206 @@ +"""Regression tests: normalize_provider_secrets() must not raise a +false-alarm "Could not resolve provider module" warning for a provider that +resolves fine but simply has no secret ConfigField (e.g. an OAuth-based +provider like openai-chatgpt). That warning is reserved for a genuine +resolution failure (get_provider_info() returns None). + +See the owner's broken-onboarding transcript: after a Ctrl-C during +`provider add openai-chatgpt`, the save path still ran +normalize_provider_secrets() and printed a spurious +"warning: Could not resolve provider module ... skipping plaintext-secret scan" +warning for a provider that was never actually unresolvable. +""" + +from unittest.mock import MagicMock, patch + +from amplifier_app_cli.lib.settings import AppSettings, SettingsPaths +from amplifier_app_cli.provider_config_utils import normalize_provider_secrets + + +def _make_settings(tmp_path) -> AppSettings: + paths = SettingsPaths( + global_settings=tmp_path / "global" / "settings.yaml", + project_settings=tmp_path / "project" / "settings.yaml", + local_settings=tmp_path / "local" / "settings.local.yaml", + ) + return AppSettings(paths=paths) + + +def _oauth_provider_info() -> dict: + """A resolvable provider with zero secret ConfigFields -- the normal + shape for an OAuth-based provider (e.g. openai-chatgpt, github-copilot) + that has nothing to scan for a plaintext api_key.""" + return { + "display_name": "OpenAI ChatGPT", + "config_fields": [ + { + "id": "some_non_secret_field", + "display_name": "Something", + "field_type": "text", + } + ], + } + + +def _api_key_provider_info() -> dict: + return { + "display_name": "Test Provider", + "config_fields": [ + { + "id": "api_key", + "display_name": "API Key", + "field_type": "secret", + "env_var": "TEST_PROVIDER_API_KEY", + } + ], + } + + +class TestNormalizeProviderSecretsUnresolvableModule: + """Genuine resolution failure -- must keep the loud, user-facing warning.""" + + def test_warns_when_provider_module_unresolvable(self, tmp_path): + settings = _make_settings(tmp_path) + scope_settings = { + "config": { + "providers": [ + {"module": "provider-does-not-exist", "config": {}}, + ] + } + } + + with ( + patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=None, + ), + patch("amplifier_app_cli.provider_config_utils.console") as mock_console, + ): + normalize_provider_secrets(settings, scope_settings, "global") + + printed = " ".join(str(c) for c in mock_console.print.call_args_list) + assert "Could not resolve provider module" in printed + + +class TestNormalizeProviderSecretsOAuthProviderNoWarning: + """Resolved-but-no-secret-field is the NORMAL case for an OAuth + provider -- must never print the loud warning, only a debug log.""" + + def test_no_console_warning_for_provider_with_no_secret_field(self, tmp_path): + settings = _make_settings(tmp_path) + scope_settings = { + "config": { + "providers": [ + {"module": "provider-openai-chatgpt", "config": {}}, + ] + } + } + + with ( + patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=_oauth_provider_info(), + ), + patch("amplifier_app_cli.provider_config_utils.console") as mock_console, + ): + normalize_provider_secrets(settings, scope_settings, "global") + + printed = " ".join(str(c) for c in mock_console.print.call_args_list) + assert "Could not resolve provider module" not in printed, ( + f"Expected no false-alarm warning, got console output: {printed}" + ) + + def test_debug_logged_for_provider_with_no_secret_field(self, tmp_path, caplog): + import logging + + settings = _make_settings(tmp_path) + scope_settings = { + "config": { + "providers": [ + {"module": "provider-openai-chatgpt", "config": {}}, + ] + } + } + + with ( + patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=_oauth_provider_info(), + ), + patch("amplifier_app_cli.provider_config_utils.console"), + caplog.at_level( + logging.DEBUG, logger="amplifier_app_cli.provider_config_utils" + ), + ): + normalize_provider_secrets(settings, scope_settings, "global") + + assert any( + "no secret ConfigField" in record.message for record in caplog.records + ), f"Expected a debug log, got: {[r.message for r in caplog.records]}" + + def test_get_provider_info_called_once_per_entry(self, tmp_path): + """Hoisting must not cost a second get_provider_info() call for the + same entry (previously implied by _secret_field_id_for() calling it + again internally).""" + settings = _make_settings(tmp_path) + scope_settings = { + "config": { + "providers": [ + {"module": "provider-openai-chatgpt", "config": {}}, + ] + } + } + + with ( + patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=_oauth_provider_info(), + ) as mock_get_info, + patch("amplifier_app_cli.provider_config_utils.console"), + ): + normalize_provider_secrets(settings, scope_settings, "global") + + assert mock_get_info.call_count == 1, ( + f"Expected get_provider_info() called once, got " + f"{mock_get_info.call_count} calls" + ) + + +class TestNormalizeProviderSecretsStillMovesLiteralSecrets: + """Regression guard: the split must not break the actual + plaintext-to-keys.env normalization for providers that DO have a + secret ConfigField.""" + + def test_literal_secret_still_moved_to_placeholder(self, tmp_path): + settings = _make_settings(tmp_path) + scope_settings = { + "config": { + "providers": [ + { + "module": "provider-test", + "config": {"api_key": "sk-literal-secret-value"}, + }, + ] + } + } + + # Patch KeyManager so this test never touches the real, shared + # ~/.amplifier/keys.env on the host running the suite. + mock_key_manager = MagicMock() + with ( + patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=_api_key_provider_info(), + ), + patch( + "amplifier_app_cli.provider_config_utils.KeyManager", + return_value=mock_key_manager, + ), + ): + normalize_provider_secrets(settings, scope_settings, "global") + + entry_config = scope_settings["config"]["providers"][0]["config"] + assert entry_config["api_key"].startswith("${"), ( + f"Expected literal secret rewritten to a placeholder, got: {entry_config}" + ) + mock_key_manager.save_key.assert_called_once() diff --git a/tests/test_openai_chatgpt_registration.py b/tests/test_openai_chatgpt_registration.py new file mode 100644 index 0000000..4e8db3a --- /dev/null +++ b/tests/test_openai_chatgpt_registration.py @@ -0,0 +1,54 @@ +"""Tests for OpenAI ChatGPT well-known (first-class) provider registration. + +At f9a2e15 (see `git log -p -- amplifier_app_cli/provider_sources.py`), +provider-openai-chatgpt had never been added to DEFAULT_PROVIDER_SOURCES -- +confirmed absent, and no commit anywhere in history ever added it. This +made `provider install openai-chatgpt` / `provider add openai-chatgpt` +work only by accident (e.g. a local source override), not as a first-class +well-known provider like anthropic/openai/gemini/etc. + +Deliberately does NOT add a PROVIDER_DEPENDENCIES entry: the +openai-chatgpt module is standalone (verified) and doesn't extend another +provider's class the way azure-openai extends openai. +""" + + +class TestOpenAIChatGPTRegistration: + """Verify OpenAI ChatGPT is registered as a well-known provider.""" + + def test_registered_in_provider_sources(self): + """OpenAI ChatGPT should be in DEFAULT_PROVIDER_SOURCES.""" + from amplifier_app_cli.provider_sources import DEFAULT_PROVIDER_SOURCES + + assert "provider-openai-chatgpt" in DEFAULT_PROVIDER_SOURCES + assert ( + "amplifier-module-provider-openai-chatgpt" + in DEFAULT_PROVIDER_SOURCES["provider-openai-chatgpt"] + ) + + def test_registered_in_display_names(self): + """OpenAI ChatGPT should be in _PROVIDER_DISPLAY_NAMES.""" + from amplifier_app_cli.provider_manager import _PROVIDER_DISPLAY_NAMES + + assert "openai-chatgpt" in _PROVIDER_DISPLAY_NAMES + assert _PROVIDER_DISPLAY_NAMES["openai-chatgpt"] == "OpenAI ChatGPT" + + def test_not_registered_as_a_runtime_dependency(self): + """openai-chatgpt is standalone -- it must not appear in + PROVIDER_DEPENDENCIES (that's reserved for providers that extend + another provider's class at runtime, e.g. azure-openai -> openai).""" + from amplifier_app_cli.provider_sources import PROVIDER_DEPENDENCIES + + assert "provider-openai-chatgpt" not in PROVIDER_DEPENDENCIES + for dependent, deps in PROVIDER_DEPENDENCIES.items(): + assert "provider-openai-chatgpt" not in deps, ( + f"provider-openai-chatgpt should not be a dependency of {dependent}" + ) + + def test_effective_sources_include_openai_chatgpt_with_no_config_manager(self): + """get_effective_provider_sources() with no config_manager should + still surface the well-known default.""" + from amplifier_app_cli.provider_sources import get_effective_provider_sources + + sources = get_effective_provider_sources(None) + assert "provider-openai-chatgpt" in sources diff --git a/tests/test_provider_commands.py b/tests/test_provider_commands.py index ac506f1..e91b8a6 100644 --- a/tests/test_provider_commands.py +++ b/tests/test_provider_commands.py @@ -704,6 +704,55 @@ def test_provider_edit_calls_configure_with_existing(self, tmp_path): len(call_kwargs[0]) > 0 # positional args ) + def test_provider_edit_interrupted_model_selection_does_not_overwrite_default_model( + self, tmp_path + ): + """Regression: Ctrl-C at the model Choice prompt during `provider edit` + must abort the whole wizard -- NOT save a config that is missing the + previously-working default_model. Runs the real configure_provider() + (only get_provider_info and _prompt_model_selection are mocked) so + the fix is exercised end-to-end through the actual edit command.""" + settings = _make_settings(tmp_path) + _seed_provider( + settings, + "provider-anthropic", + {"default_model": "claude-sonnet-4-6"}, + priority=1, + ) + + from amplifier_app_cli.commands.provider import provider + + runner = CliRunner() + with ( + patch( + "amplifier_app_cli.commands.provider._get_settings", + return_value=settings, + ), + patch("amplifier_app_cli.commands.provider._ensure_providers_ready"), + patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value={"display_name": "Anthropic", "config_fields": []}, + ), + patch( + "amplifier_app_cli.provider_config_utils._prompt_model_selection", + return_value=None, + ), + patch("amplifier_app_cli.commands.provider.KeyManager"), + ): + result = runner.invoke(provider, ["edit", "anthropic"]) + + assert result.exit_code == 0, f"Output: {result.output}" + assert "cancelled" in result.output.lower(), ( + f"Expected a cancellation message, got: {result.output}" + ) + + providers = settings.get_scope_provider_overrides("global") + assert len(providers) == 1 + assert providers[0]["config"]["default_model"] == "claude-sonnet-4-6", ( + "Interrupted model selection must not overwrite the existing " + f"default_model, got config: {providers[0]['config']}" + ) + def test_provider_edit_accepts_scope(self, tmp_path): """provider edit --scope project should write the updated entry to project scope.""" settings = _make_settings(tmp_path) diff --git a/tests/test_provider_config_error_handling.py b/tests/test_provider_config_error_handling.py index e293ca2..3847c92 100644 --- a/tests/test_provider_config_error_handling.py +++ b/tests/test_provider_config_error_handling.py @@ -525,6 +525,136 @@ def test_configure_provider_prints_cancelled_on_ctrl_c(self): ) +# ============================================================ +# Regression: model-selection Ctrl-C must abort configure_provider, +# not fall through to "configured" + save (data-corrupting onboarding bug). +# ============================================================ + + +class TestConfigureProviderAbortsOnModelSelectionInterrupt: + """_prompt_model_selection() returning None (Ctrl-C / EOF at the model + Choice prompt) must abort configure_provider() the same way the outer + except (KeyboardInterrupt, EOFError) handler does -- NOT be treated the + same as "" (declined custom model name), which is a valid + continue-without-a-model outcome. + """ + + def _make_mock_provider_info(self): + """Provider info with only a pre-model field, so the model-selection + step is reached deterministically.""" + return { + "display_name": "Test Provider", + "config_fields": [ + { + "id": "api_key", + "display_name": "API Key", + "field_type": "text", + "prompt": "Enter your API key", + "required": True, + } + ], + } + + def test_configure_provider_returns_none_when_model_selection_interrupted(self): + """When _prompt_model_selection() returns None (Ctrl-C at Choice), + configure_provider() must return None -- not a "configured" dict.""" + from amplifier_app_cli.provider_config_utils import configure_provider + + mock_key_manager = MagicMock() + + with ( + patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=self._make_mock_provider_info(), + ), + patch( + "amplifier_app_cli.provider_config_utils.Prompt.ask", + return_value="dummy-api-key", + ), + patch( + "amplifier_app_cli.provider_config_utils._prompt_model_selection", + return_value=None, + ), + patch("amplifier_app_cli.provider_config_utils.console"), + ): + result = configure_provider("test-provider", mock_key_manager) + + assert result is None, ( + f"Expected None when model selection is interrupted, got {result!r}" + ) + + def test_configure_provider_prints_cancelled_when_model_selection_interrupted( + self, + ): + """The abort must be visible to the user, matching the outer + handler's 'Cancelled.' message -- never a silent, empty return.""" + from amplifier_app_cli.provider_config_utils import configure_provider + + mock_key_manager = MagicMock() + mock_console = MagicMock() + + with ( + patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=self._make_mock_provider_info(), + ), + patch( + "amplifier_app_cli.provider_config_utils.Prompt.ask", + return_value="dummy-api-key", + ), + patch( + "amplifier_app_cli.provider_config_utils._prompt_model_selection", + return_value=None, + ), + patch( + "amplifier_app_cli.provider_config_utils.console", + mock_console, + ), + ): + configure_provider("test-provider", mock_key_manager) + + printed_texts = [str(call) for call in mock_console.print.call_args_list] + joined = " ".join(printed_texts) + assert "Cancelled" in joined, ( + f"Expected 'Cancelled' in console output, got: {printed_texts}" + ) + assert "configured" not in joined, ( + "Must not print the ' configured' success message " + f"when the model prompt was interrupted, got: {printed_texts}" + ) + + def test_configure_provider_declined_custom_model_name_still_completes(self): + """ "" (empty string -- user declined to type a custom model name) is + NOT the abort sentinel. configure_provider() must still complete and + return a config dict (without default_model set), matching existing + behavior for providers with no models discovered.""" + from amplifier_app_cli.provider_config_utils import configure_provider + + mock_key_manager = MagicMock() + + with ( + patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=self._make_mock_provider_info(), + ), + patch( + "amplifier_app_cli.provider_config_utils.Prompt.ask", + return_value="dummy-api-key", + ), + patch( + "amplifier_app_cli.provider_config_utils._prompt_model_selection", + return_value="", + ), + patch("amplifier_app_cli.provider_config_utils.console"), + ): + result = configure_provider("test-provider", mock_key_manager) + + assert result is not None, ( + "Declining a custom model name ('') must not abort configuration" + ) + assert "default_model" not in result + + # ============================================================ # Task 3: Spinner wraps model fetching # ============================================================ diff --git a/tests/test_provider_login_command.py b/tests/test_provider_login_command.py new file mode 100644 index 0000000..7263a7f --- /dev/null +++ b/tests/test_provider_login_command.py @@ -0,0 +1,334 @@ +"""Tests for `amplifier provider login `. + +Duck-types auth_status()/login() the same way the wizard's login step +does, so this command works safely independent of the parallel +provider-module PR that's adding those methods to provider-openai-chatgpt. +""" + +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +from click.testing import CliRunner + +from amplifier_app_cli.lib.settings import AppSettings, SettingsPaths + + +def _make_settings(tmp_path: Path) -> AppSettings: + paths = SettingsPaths( + global_settings=tmp_path / "global" / "settings.yaml", + project_settings=tmp_path / "project" / "settings.yaml", + local_settings=tmp_path / "local" / "settings.local.yaml", + ) + return AppSettings(paths=paths) + + +def _seed_provider(settings: AppSettings, module: str, config: dict) -> None: + entry = {"module": module, "config": {**config, "priority": 1}} + with patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=None, + ): + settings.set_provider_override(entry, scope="global") + + +def _oauth_info(display_name: str = "OpenAI ChatGPT") -> dict: + return { + "display_name": display_name, + "capabilities": ["streaming", "auth:oauth-device-code"], + "config_fields": [], + } + + +def _api_key_info() -> dict: + return { + "display_name": "Anthropic", + "capabilities": ["streaming"], + "config_fields": [ + {"id": "api_key", "field_type": "secret", "env_var": "ANTHROPIC_API_KEY"} + ], + } + + +class TestProviderLoginCommandRegistered: + def test_login_command_exists(self): + from amplifier_app_cli.commands.provider import provider + + command_names = [c.name for c in provider.commands.values()] + assert "login" in command_names + + +class TestProviderLoginNotInstalled: + def test_helpful_error_names_provider_install(self, tmp_path): + from amplifier_app_cli.commands.provider import provider + + settings = _make_settings(tmp_path) + runner = CliRunner() + with ( + patch( + "amplifier_app_cli.commands.provider._get_settings", + return_value=settings, + ), + patch( + "amplifier_app_cli.commands.provider.is_provider_module_installed", + return_value=False, + ), + ): + result = runner.invoke(provider, ["login", "openai-chatgpt"]) + + assert result.exit_code != 0 + assert "not installed" in result.output.lower() + assert "provider install" in result.output + + +class TestProviderLoginNoAuthCapability: + def test_clean_error_for_api_key_provider(self, tmp_path): + from amplifier_app_cli.commands.provider import provider + + settings = _make_settings(tmp_path) + runner = CliRunner() + with ( + patch( + "amplifier_app_cli.commands.provider._get_settings", + return_value=settings, + ), + patch( + "amplifier_app_cli.commands.provider.is_provider_module_installed", + return_value=True, + ), + patch( + "amplifier_app_cli.commands.provider.get_provider_info", + return_value=_api_key_info(), + ), + ): + result = runner.invoke(provider, ["login", "anthropic"]) + + assert result.exit_code == 0 + assert "API-key configuration" in result.output + assert "provider edit" in result.output + + +class TestProviderLoginDuckTypeMissingMethods: + def test_clean_error_when_methods_missing_despite_capability(self, tmp_path): + from amplifier_app_cli.commands.provider import provider + + settings = _make_settings(tmp_path) + provider_instance = MagicMock(spec=[]) # no auth_status/login + + runner = CliRunner() + with ( + patch( + "amplifier_app_cli.commands.provider._get_settings", + return_value=settings, + ), + patch( + "amplifier_app_cli.commands.provider.is_provider_module_installed", + return_value=True, + ), + patch( + "amplifier_app_cli.commands.provider.get_provider_info", + return_value=_oauth_info(), + ), + patch( + "amplifier_app_cli.commands.provider.load_provider_class", + return_value=MagicMock, + ), + patch( + "amplifier_app_cli.commands.provider._try_instantiate_provider", + return_value=provider_instance, + ), + ): + result = runner.invoke(provider, ["login", "openai-chatgpt"]) + + assert result.exit_code == 0 + assert "API-key configuration" in result.output + + +class TestProviderLoginAlreadyAuthenticated: + def test_reports_already_logged_in(self, tmp_path): + from amplifier_app_cli.commands.provider import provider + + settings = _make_settings(tmp_path) + provider_instance = MagicMock() + provider_instance.auth_status.return_value = "authenticated" + + runner = CliRunner() + with ( + patch( + "amplifier_app_cli.commands.provider._get_settings", + return_value=settings, + ), + patch( + "amplifier_app_cli.commands.provider.is_provider_module_installed", + return_value=True, + ), + patch( + "amplifier_app_cli.commands.provider.get_provider_info", + return_value=_oauth_info(), + ), + patch( + "amplifier_app_cli.commands.provider.load_provider_class", + return_value=MagicMock, + ), + patch( + "amplifier_app_cli.commands.provider._try_instantiate_provider", + return_value=provider_instance, + ), + ): + result = runner.invoke(provider, ["login", "openai-chatgpt"]) + + assert result.exit_code == 0 + assert "Already logged in" in result.output + provider_instance.login.assert_not_called() + + +class TestProviderLoginUsesSavedConfig: + def test_instantiates_with_saved_config_for_id(self, tmp_path): + from amplifier_app_cli.commands.provider import provider + + settings = _make_settings(tmp_path) + _seed_provider( + settings, "provider-openai-chatgpt", {"some_field": "saved-value"} + ) + provider_instance = MagicMock() + provider_instance.auth_status.return_value = "authenticated" + + runner = CliRunner() + with ( + patch( + "amplifier_app_cli.commands.provider._get_settings", + return_value=settings, + ), + patch( + "amplifier_app_cli.commands.provider.is_provider_module_installed", + return_value=True, + ), + patch( + "amplifier_app_cli.commands.provider.get_provider_info", + return_value=_oauth_info(), + ), + patch( + "amplifier_app_cli.commands.provider.load_provider_class", + return_value=MagicMock, + ), + patch( + "amplifier_app_cli.commands.provider._try_instantiate_provider", + return_value=provider_instance, + ) as mock_instantiate, + ): + result = runner.invoke(provider, ["login", "openai-chatgpt"]) + + assert result.exit_code == 0 + args, _ = mock_instantiate.call_args + assert args[1]["some_field"] == "saved-value" + + def test_instantiates_with_empty_config_when_never_configured(self, tmp_path): + from amplifier_app_cli.commands.provider import provider + + settings = _make_settings(tmp_path) # no seeded provider + provider_instance = MagicMock() + provider_instance.auth_status.return_value = "authenticated" + + runner = CliRunner() + with ( + patch( + "amplifier_app_cli.commands.provider._get_settings", + return_value=settings, + ), + patch( + "amplifier_app_cli.commands.provider.is_provider_module_installed", + return_value=True, + ), + patch( + "amplifier_app_cli.commands.provider.get_provider_info", + return_value=_oauth_info(), + ), + patch( + "amplifier_app_cli.commands.provider.load_provider_class", + return_value=MagicMock, + ), + patch( + "amplifier_app_cli.commands.provider._try_instantiate_provider", + return_value=provider_instance, + ) as mock_instantiate, + ): + result = runner.invoke(provider, ["login", "openai-chatgpt"]) + + assert result.exit_code == 0 + args, _ = mock_instantiate.call_args + assert args[1] == {} + + +class TestProviderLoginRunsLoginFlow: + def test_successful_login_reports_success(self, tmp_path): + from amplifier_app_cli.commands.provider import provider + + settings = _make_settings(tmp_path) + provider_instance = MagicMock() + provider_instance.auth_status.side_effect = ["unauthenticated", "authenticated"] + provider_instance.login = AsyncMock(return_value=True) + + runner = CliRunner() + with ( + patch( + "amplifier_app_cli.commands.provider._get_settings", + return_value=settings, + ), + patch( + "amplifier_app_cli.commands.provider.is_provider_module_installed", + return_value=True, + ), + patch( + "amplifier_app_cli.commands.provider.get_provider_info", + return_value=_oauth_info(), + ), + patch( + "amplifier_app_cli.commands.provider.load_provider_class", + return_value=MagicMock, + ), + patch( + "amplifier_app_cli.commands.provider._try_instantiate_provider", + return_value=provider_instance, + ), + ): + result = runner.invoke(provider, ["login", "openai-chatgpt"]) + + assert result.exit_code == 0 + provider_instance.login.assert_awaited_once() + assert "Logged in to" in result.output + assert "authenticated" in result.output + + def test_failed_login_reports_failure_and_nonzero_exit(self, tmp_path): + from amplifier_app_cli.commands.provider import provider + + settings = _make_settings(tmp_path) + provider_instance = MagicMock() + provider_instance.auth_status.return_value = "unauthenticated" + provider_instance.login = AsyncMock(return_value=False) + + runner = CliRunner() + with ( + patch( + "amplifier_app_cli.commands.provider._get_settings", + return_value=settings, + ), + patch( + "amplifier_app_cli.commands.provider.is_provider_module_installed", + return_value=True, + ), + patch( + "amplifier_app_cli.commands.provider.get_provider_info", + return_value=_oauth_info(), + ), + patch( + "amplifier_app_cli.commands.provider.load_provider_class", + return_value=MagicMock, + ), + patch( + "amplifier_app_cli.commands.provider._try_instantiate_provider", + return_value=provider_instance, + ), + ): + result = runner.invoke(provider, ["login", "openai-chatgpt"]) + + assert result.exit_code != 0 + assert "Login failed" in result.output diff --git a/tests/test_provider_login_wizard_step.py b/tests/test_provider_login_wizard_step.py new file mode 100644 index 0000000..94f24c9 --- /dev/null +++ b/tests/test_provider_login_wizard_step.py @@ -0,0 +1,470 @@ +"""Tests for the wizard login step: _maybe_login_provider(), +_run_provider_login(), and _safely_fetch_models_from_instance() in +provider_config_utils.py, plus their integration into configure_provider(). + +Duck-types auth_status()/login() via hasattr so this PR merges safely +independent of the parallel provider-module PR adding those methods (and +the "auth:oauth-device-code" capability) to provider-openai-chatgpt. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +from amplifier_app_cli.provider_config_utils import ( + _maybe_login_provider, + _run_provider_login, + _safely_fetch_models_from_instance, + configure_provider, +) + + +def _info_with_auth_capability(display_name: str = "OpenAI ChatGPT") -> dict: + return { + "display_name": display_name, + "capabilities": ["streaming", "tools", "auth:oauth-device-code"], + "config_fields": [], + } + + +def _info_without_auth_capability() -> dict: + return { + "display_name": "Anthropic", + "capabilities": ["streaming", "tools"], + "config_fields": [], + } + + +class TestMaybeLoginProviderCapabilityGate: + """No "auth:*" capability -> no instantiation attempt at all.""" + + def test_returns_none_when_no_auth_capability(self): + with patch( + "amplifier_app_cli.provider_config_utils.load_provider_class" + ) as mock_load: + result = _maybe_login_provider( + "anthropic", _info_without_auth_capability(), {} + ) + + assert result is None + mock_load.assert_not_called() + + def test_returns_none_when_provider_class_unloadable(self): + with patch( + "amplifier_app_cli.provider_config_utils.load_provider_class", + return_value=None, + ): + result = _maybe_login_provider( + "openai-chatgpt", _info_with_auth_capability(), {} + ) + assert result is None + + def test_returns_none_when_instantiation_fails(self): + with ( + patch( + "amplifier_app_cli.provider_config_utils.load_provider_class", + return_value=MagicMock, + ), + patch( + "amplifier_app_cli.provider_config_utils._try_instantiate_provider", + return_value=None, + ), + ): + result = _maybe_login_provider( + "openai-chatgpt", _info_with_auth_capability(), {} + ) + assert result is None + + +class TestMaybeLoginProviderDuckTyping: + """Provider declares "auth:*" but doesn't implement auth_status/login + -- must be treated like "no login flow", instance still reusable.""" + + def test_returns_instance_without_prompting_when_methods_missing(self): + provider = MagicMock(spec=[]) # no auth_status/login attributes + + with ( + patch( + "amplifier_app_cli.provider_config_utils.load_provider_class", + return_value=MagicMock, + ), + patch( + "amplifier_app_cli.provider_config_utils._try_instantiate_provider", + return_value=provider, + ), + patch( + "amplifier_app_cli.provider_config_utils.Confirm.ask" + ) as mock_confirm, + ): + result = _maybe_login_provider( + "openai-chatgpt", _info_with_auth_capability(), {} + ) + + assert result is provider + mock_confirm.assert_not_called() + + +class TestMaybeLoginProviderAlreadyAuthenticated: + def test_no_prompt_when_already_authenticated(self): + provider = MagicMock() + provider.auth_status.return_value = "authenticated" + + with ( + patch( + "amplifier_app_cli.provider_config_utils.load_provider_class", + return_value=MagicMock, + ), + patch( + "amplifier_app_cli.provider_config_utils._try_instantiate_provider", + return_value=provider, + ), + patch( + "amplifier_app_cli.provider_config_utils.Confirm.ask" + ) as mock_confirm, + ): + result = _maybe_login_provider( + "openai-chatgpt", _info_with_auth_capability(), {} + ) + + assert result is provider + mock_confirm.assert_not_called() + + def test_broken_auth_status_treated_as_no_login(self): + """A raising auth_status() must never crash the wizard.""" + provider = MagicMock() + provider.auth_status.side_effect = RuntimeError("network down") + + with ( + patch( + "amplifier_app_cli.provider_config_utils.load_provider_class", + return_value=MagicMock, + ), + patch( + "amplifier_app_cli.provider_config_utils._try_instantiate_provider", + return_value=provider, + ), + patch( + "amplifier_app_cli.provider_config_utils.Confirm.ask" + ) as mock_confirm, + ): + result = _maybe_login_provider( + "openai-chatgpt", _info_with_auth_capability(), {} + ) + + assert result is provider + mock_confirm.assert_not_called() + + +class TestMaybeLoginProviderPromptFlow: + """auth_status() != "authenticated" -> the actual prompt/login flow.""" + + def test_decline_prints_skip_message_and_never_calls_login(self): + provider = MagicMock() + provider.auth_status.return_value = "unauthenticated" + + with ( + patch( + "amplifier_app_cli.provider_config_utils.load_provider_class", + return_value=MagicMock, + ), + patch( + "amplifier_app_cli.provider_config_utils._try_instantiate_provider", + return_value=provider, + ), + patch( + "amplifier_app_cli.provider_config_utils.Confirm.ask", + return_value=False, + ), + patch("amplifier_app_cli.provider_config_utils.console") as mock_console, + ): + result = _maybe_login_provider( + "openai-chatgpt", _info_with_auth_capability(), {} + ) + + assert result is provider + provider.login.assert_not_called() + printed = " ".join(str(c) for c in mock_console.print.call_args_list) + assert "Skipping login" in printed + assert "amplifier provider login openai-chatgpt" in printed + + def test_accept_and_successful_login_no_skip_message(self): + provider = MagicMock() + provider.auth_status.return_value = "expired" + provider.login = AsyncMock(return_value=True) + + with ( + patch( + "amplifier_app_cli.provider_config_utils.load_provider_class", + return_value=MagicMock, + ), + patch( + "amplifier_app_cli.provider_config_utils._try_instantiate_provider", + return_value=provider, + ), + patch( + "amplifier_app_cli.provider_config_utils.Confirm.ask", + return_value=True, + ), + patch("amplifier_app_cli.provider_config_utils.console") as mock_console, + ): + result = _maybe_login_provider( + "openai-chatgpt", _info_with_auth_capability(), {} + ) + + assert result is provider + provider.login.assert_awaited_once() + printed = " ".join(str(c) for c in mock_console.print.call_args_list) + assert "Skipping login" not in printed + + def test_accept_but_failed_login_prints_skip_message(self): + provider = MagicMock() + provider.auth_status.return_value = "unauthenticated" + provider.login = AsyncMock(return_value=False) + + with ( + patch( + "amplifier_app_cli.provider_config_utils.load_provider_class", + return_value=MagicMock, + ), + patch( + "amplifier_app_cli.provider_config_utils._try_instantiate_provider", + return_value=provider, + ), + patch( + "amplifier_app_cli.provider_config_utils.Confirm.ask", + return_value=True, + ), + patch("amplifier_app_cli.provider_config_utils.console") as mock_console, + ): + result = _maybe_login_provider( + "openai-chatgpt", _info_with_auth_capability(), {} + ) + + assert result is provider + printed = " ".join(str(c) for c in mock_console.print.call_args_list) + assert "Skipping login" in printed + + +class TestRunProviderLogin: + def test_sync_login_success(self): + provider = MagicMock() + provider.login = MagicMock(return_value=True) + # Ensure iscoroutinefunction sees this as sync + assert _run_provider_login(provider) is True + + def test_sync_login_failure(self): + provider = MagicMock() + provider.login = MagicMock(return_value=False) + assert _run_provider_login(provider) is False + + def test_async_login_success_calls_print_fn(self): + async def fake_login(print_fn=None): + if print_fn: + print_fn("Go to https://example.com/device and enter ABCD-EFGH") + return True + + provider = MagicMock() + provider.login = fake_login + + with patch("amplifier_app_cli.provider_config_utils.console") as mock_console: + result = _run_provider_login(provider) + + assert result is True + printed = " ".join(str(c) for c in mock_console.print.call_args_list) + assert "https://example.com/device" in printed + assert "ABCD-EFGH" in printed + + def test_login_exception_returns_false_no_traceback(self): + provider = MagicMock() + provider.login = MagicMock(side_effect=RuntimeError("device flow expired")) + + with patch("amplifier_app_cli.provider_config_utils.console") as mock_console: + result = _run_provider_login(provider) + + assert result is False + printed = " ".join(str(c) for c in mock_console.print.call_args_list) + assert "Login failed" in printed + assert "Traceback" not in printed + + def test_keyboard_interrupt_propagates(self): + provider = MagicMock() + provider.login = MagicMock(side_effect=KeyboardInterrupt()) + + try: + _run_provider_login(provider) + raised = False + except KeyboardInterrupt: + raised = True + assert raised, "KeyboardInterrupt must propagate, not be swallowed" + + +class TestSafelyFetchModelsFromInstance: + def test_success_returns_models(self): + models = [MagicMock(id="gpt-5")] + with patch( + "amplifier_app_cli.provider_config_utils.list_models_for_instance", + return_value=models, + ): + result = _safely_fetch_models_from_instance("openai-chatgpt", MagicMock()) + assert result == models + + def test_connection_error_returns_empty_list_silently(self): + with ( + patch( + "amplifier_app_cli.provider_config_utils.list_models_for_instance", + side_effect=ConnectionError("down"), + ), + patch("amplifier_app_cli.provider_config_utils.console") as mock_console, + ): + result = _safely_fetch_models_from_instance("openai-chatgpt", MagicMock()) + assert result == [] + mock_console.print.assert_not_called() + + def test_generic_exception_returns_empty_list_with_warning_not_traceback(self): + with ( + patch( + "amplifier_app_cli.provider_config_utils.list_models_for_instance", + side_effect=RuntimeError("AuthenticationError: bad token"), + ), + patch("amplifier_app_cli.provider_config_utils.console") as mock_console, + ): + result = _safely_fetch_models_from_instance("openai-chatgpt", MagicMock()) + assert result == [] + printed = " ".join(str(c) for c in mock_console.print.call_args_list) + assert "Could not fetch models" in printed + assert "Traceback" not in printed + + +class TestConfigureProviderLoginStepIntegration: + """End-to-end through configure_provider(): the login step must never + break a provider with no auth capability (regression guard), and must + drive the prompt + reuse the fetched models for a provider that has one.""" + + def _api_key_info(self): + return { + "display_name": "Test Provider", + "capabilities": ["streaming"], + "config_fields": [ + { + "id": "api_key", + "display_name": "API Key", + "field_type": "text", + "prompt": "Enter your API key", + "required": True, + } + ], + } + + def test_provider_without_auth_capability_unaffected(self): + """Regression guard: a normal (non-OAuth) provider's configure_provider() + flow is byte-for-byte unaffected by the new login step.""" + mock_key_manager = MagicMock() + + with ( + patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=self._api_key_info(), + ), + patch( + "amplifier_app_cli.provider_config_utils.Prompt.ask", + return_value="dummy-api-key", + ), + patch( + "amplifier_app_cli.provider_config_utils._prompt_model_selection", + return_value="model-x", + ) as mock_select, + patch( + "amplifier_app_cli.provider_config_utils.load_provider_class" + ) as mock_load, + patch("amplifier_app_cli.provider_config_utils.console"), + ): + result = configure_provider("test-provider", mock_key_manager) + + assert result is not None + assert result["default_model"] == "model-x" + mock_load.assert_not_called() + # models kwarg must be None (unaffected) when there's no auth capability + _, kwargs = mock_select.call_args + assert kwargs.get("models") is None + + def test_provider_with_auth_capability_prompts_and_reuses_prefetched_models( + self, + ): + oauth_info = _info_with_auth_capability() + oauth_info["config_fields"] = [] + provider_instance = MagicMock() + provider_instance.auth_status.return_value = "unauthenticated" + provider_instance.login = AsyncMock(return_value=True) + prefetched = [MagicMock(id="gpt-5-chatgpt")] + + with ( + patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=oauth_info, + ), + patch( + "amplifier_app_cli.provider_config_utils.load_provider_class", + return_value=MagicMock, + ), + patch( + "amplifier_app_cli.provider_config_utils._try_instantiate_provider", + return_value=provider_instance, + ), + patch( + "amplifier_app_cli.provider_config_utils.list_models_for_instance", + return_value=prefetched, + ), + patch( + "amplifier_app_cli.provider_config_utils.Confirm.ask", + return_value=True, + ), + patch( + "amplifier_app_cli.provider_config_utils._prompt_model_selection", + return_value="gpt-5-chatgpt", + ) as mock_select, + patch("amplifier_app_cli.provider_config_utils.console"), + ): + result = configure_provider("openai-chatgpt", MagicMock()) + + assert result is not None + assert result["default_model"] == "gpt-5-chatgpt" + provider_instance.login.assert_awaited_once() + _, kwargs = mock_select.call_args + assert kwargs.get("models") == prefetched, ( + "The prefetched models from the login-time instance must be " + "passed straight into _prompt_model_selection(), not refetched" + ) + + def test_provider_with_auth_capability_decline_still_completes(self): + oauth_info = _info_with_auth_capability() + oauth_info["config_fields"] = [] + provider_instance = MagicMock() + provider_instance.auth_status.return_value = "unauthenticated" + + with ( + patch( + "amplifier_app_cli.provider_config_utils.get_provider_info", + return_value=oauth_info, + ), + patch( + "amplifier_app_cli.provider_config_utils.load_provider_class", + return_value=MagicMock, + ), + patch( + "amplifier_app_cli.provider_config_utils._try_instantiate_provider", + return_value=provider_instance, + ), + patch( + "amplifier_app_cli.provider_config_utils.Confirm.ask", + return_value=False, + ), + patch( + "amplifier_app_cli.provider_config_utils._prompt_model_selection", + return_value="", + ), + patch("amplifier_app_cli.provider_config_utils.console") as mock_console, + ): + result = configure_provider("openai-chatgpt", MagicMock()) + + assert result is not None, "Declining login must not abort the wizard" + provider_instance.login.assert_not_called() + printed = " ".join(str(c) for c in mock_console.print.call_args_list) + assert "Skipping login" in printed