From d03d2d5c6242013707bc92dcfcf8dd932b3717e1 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Sat, 29 Aug 2026 20:45:50 -0700 Subject: [PATCH 1/6] fix(provider): abort configure_provider() on Ctrl-C at model selection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The owner's transcript: `provider add openai-chatgpt` raised a raw AuthenticationError during model fetch, fell back to a stale model list, the user pressed Ctrl-C at the "Choice" prompt -- and the wizard printed "OpenAI ChatGPT configured" anyway and saved an empty/partial config. Root cause: _prompt_model_selection() already returns None on KeyboardInterrupt/EOFError (provider_config_utils.py:187-188, documented at :93-94), but its caller in configure_provider() only checked `if selected_model:` (provider_config_utils.py:943) -- a falsy check that treats the strict abort sentinel (None) the same as "" (user declined to type a custom model name, a valid continue-without-a-model outcome). An interrupted model prompt fell straight through to Phase 3, printed the "configured" success message, and returned a config dict instead of None. Fix: disambiguate the sentinel at the call site. `selected_model is None` now aborts configure_provider() immediately with the same "Cancelled." message the outer `except (KeyboardInterrupt, EOFError)` handler prints (:984-986), matching the precedent in commands/routing.py's model-picker (`if selected is None: return None`). `""` still falls through to "no model set, continue" as before. This single fix lands the abort on all three call sites that already correctly check `if config is None:` after calling configure_provider(): `provider add` (commands/provider.py:607), the manage-loop's add flow (:1403), and `provider edit` (:951) -- the last of which was silently overwriting a working default_model with a config that had none. Regression tests: - configure_provider() returns None (not a dict) when model selection is interrupted, and prints "Cancelled." without ever printing "configured" - "" (declined custom name) still completes configuration, unaffected - `provider edit` end-to-end: an interrupted model prompt no longer overwrites an existing default_model in settings 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_app_cli/provider_config_utils.py | 11 ++ tests/test_provider_commands.py | 49 +++++++ tests/test_provider_config_error_handling.py | 130 +++++++++++++++++++ 3 files changed, 190 insertions(+) diff --git a/amplifier_app_cli/provider_config_utils.py b/amplifier_app_cli/provider_config_utils.py index 96b60ae2..5d638060 100644 --- a/amplifier_app_cli/provider_config_utils.py +++ b/amplifier_app_cli/provider_config_utils.py @@ -940,6 +940,17 @@ def configure_provider( selected_model = _prompt_model_selection( provider_id, default_model, collected_config ) + # 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/tests/test_provider_commands.py b/tests/test_provider_commands.py index ac506f11..e91b8a60 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 e293ca28..3847c921 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 # ============================================================ From 0b2b2daa7a1b18371d708c27a8ed0f956963d075 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Sat, 29 Aug 2026 20:49:08 -0700 Subject: [PATCH 2/6] fix(provider): stop false "Could not resolve provider module" warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The owner's transcript ended with a spurious "⚠ Could not resolve provider module ... skipping plaintext-secret scan" warning, even though the provider module in question resolved just fine. Root cause: normalize_provider_secrets() (provider_config_utils.py) printed that warning whenever _secret_field_id_for(module_id) returned None -- but that helper returns None for TWO different reasons that were conflated: 1. The module genuinely can't be resolved/instantiated (a real failure). 2. The module resolves fine but simply declares no field_type == "secret" ConfigField at all -- the normal, expected shape for any OAuth-based provider (e.g. openai-chatgpt, github-copilot) that has no api_key to scan for. This is not a failure; it's just nothing to do. Fix: call get_provider_info(module_id) once per entry and branch on it directly. `None` (case 1) keeps the loud, user-facing yellow warning unchanged. A resolved info with no secret field (case 2) now only logs at debug level -- no console output. Added _secret_config_field_from_info()/_secret_field_id_for_info() so the already-fetched info dict is reused instead of calling get_provider_info() a second time via the existing module_id-based helpers (which stay in place, unchanged, for their other callers). Regression tests (new tests/test_normalize_provider_secrets_warning.py): - genuine resolution failure still prints the warning - a resolved OAuth-shaped provider prints no warning, only a debug log - get_provider_info() is called exactly once per entry - literal secrets are still correctly moved to keys.env (unaffected) 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_app_cli/provider_config_utils.py | 53 ++++- ...test_normalize_provider_secrets_warning.py | 206 ++++++++++++++++++ 2 files changed, 253 insertions(+), 6 deletions(-) create mode 100644 tests/test_normalize_provider_secrets_warning.py diff --git a/amplifier_app_cli/provider_config_utils.py b/amplifier_app_cli/provider_config_utils.py index 5d638060..da277e7e 100644 --- a/amplifier_app_cli/provider_config_utils.py +++ b/amplifier_app_cli/provider_config_utils.py @@ -283,10 +283,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 +303,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 +324,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 +513,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 +530,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: diff --git a/tests/test_normalize_provider_secrets_warning.py b/tests/test_normalize_provider_secrets_warning.py new file mode 100644 index 00000000..dc03a102 --- /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() From 51c79a5b96e585054724ec0cc18cd1f73aa8bdef Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Sat, 29 Aug 2026 20:52:08 -0700 Subject: [PATCH 3/6] fix(main): install a real console logging handler at startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The owner's transcript showed a raw AuthenticationError traceback dumped to the terminal during `provider add openai-chatgpt`'s model fetch. Root cause: 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 `.exception(...)`) falls through to Python's `logging.lastResort`, which prints the bare message AND the full traceback straight to stderr with no formatting control -- for any module, not just one provider's model-fetch path. This also explains why `_attach_llm_error_filter()` (main.py:185-204) was inert in practice: its primary path attaches the filter to an existing stderr StreamHandler, but with no handlers configured it always fell back to `root.addFilter(...)` -- a logger-level filter, which `Logger.callHandlers()` never consults for records emitted by a *child* logger (it walks handlers up the hierarchy and checks each handler's own filters, but only checks the *originating* logger's filters, not the root's). So the fallback silently did nothing for the normal case of a named `logging.getLogger(__name__)` call anywhere else in the codebase. Fix: added `_configure_console_logging()`, called from `main()` before `_attach_llm_error_filter()`. If the root logger has no handlers, installs one stderr StreamHandler at WARNING with a plain "%(message)s" formatter, and -- unless `--verbose`/`-v`/`--debug` is present on the command line -- 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. This also makes `_attach_llm_error_filter()`'s primary (correct) path fire for the first time, since a real stderr handler now exists to attach to. No-op if a handler already exists (e.g. under pytest). Tests (tests/test_configure_console_logging.py): handler installed only when none exists; traceback suppressed by default and preserved under --verbose/-v/--debug; formatter is message-only; the LLMErrorLogFilter now attaches to the real handler instead of the inert root-logger fallback; end-to-end -- a logged exception reaches stderr with its message but without "Traceback"/exception-type text, by default. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_app_cli/main.py | 69 ++++++- tests/test_configure_console_logging.py | 238 ++++++++++++++++++++++++ 2 files changed, 304 insertions(+), 3 deletions(-) create mode 100644 tests/test_configure_console_logging.py diff --git a/amplifier_app_cli/main.py b/amplifier_app_cli/main.py index 53e246fc..bfe2f807 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/tests/test_configure_console_logging.py b/tests/test_configure_console_logging.py new file mode 100644 index 00000000..3902c6de --- /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 From a7c6f16821b02165ac14bf08928f901a3ba78283 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Sat, 29 Aug 2026 20:53:21 -0700 Subject: [PATCH 4/6] feat(provider): register openai-chatgpt as a first-class well-known provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The owner's transcript shows `provider add openai-chatgpt` (and an earlier `provider install openai-chatgpt`) succeeding, but at f9a2e15 provider-openai-chatgpt was never present in DEFAULT_PROVIDER_SOURCES -- confirmed by `git log -p -- amplifier_app_cli/provider_sources.py`, which shows no commit anywhere in this repo's history ever adding it (searched `git log --all --oneline | grep -i chatgpt`: no results). So it was never a well-known, first-class provider the way anthropic/openai/gemini/ azure-openai/github-copilot/ollama/vllm/chat-completions are; anything that worked did so via a source override, not this registry. Adds: - "provider-openai-chatgpt" -> the module's git source, alphabetically placed in DEFAULT_PROVIDER_SOURCES (provider_sources.py) - "openai-chatgpt" -> "OpenAI ChatGPT" in _PROVIDER_DISPLAY_NAMES (provider_manager.py) Deliberately does NOT add a PROVIDER_DEPENDENCIES entry: the module is standalone (verified) and does not extend another provider's class the way azure-openai extends openai. Tests (tests/test_openai_chatgpt_registration.py): registered in both well-known dicts with the correct display name and source URL; absent from PROVIDER_DEPENDENCIES (as neither a dependent nor a dependency); surfaced by get_effective_provider_sources() with no config_manager. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_app_cli/provider_manager.py | 1 + amplifier_app_cli/provider_sources.py | 1 + .../test_discover_providers_clean_install.py | 1 + tests/test_openai_chatgpt_registration.py | 54 +++++++++++++++++++ 4 files changed, 57 insertions(+) create mode 100644 tests/test_openai_chatgpt_registration.py diff --git a/amplifier_app_cli/provider_manager.py b/amplifier_app_cli/provider_manager.py index 412e14f0..0773d3e9 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 3913db81..995e3d63 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_discover_providers_clean_install.py b/tests/test_discover_providers_clean_install.py index 90908c14..95b76dad 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_openai_chatgpt_registration.py b/tests/test_openai_chatgpt_registration.py new file mode 100644 index 00000000..4e8db3a1 --- /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 From 5bb9ea1b08463a3e3646ac0aba18d8ae52b9ab7f Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Sat, 29 Aug 2026 20:58:26 -0700 Subject: [PATCH 5/6] feat(provider): wizard login step for OAuth-capable providers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the "top-notch onboarding" login step to configure_provider(): for a provider that declares an "auth:*" capability in get_info() (e.g. the parallel provider PR's planned "auth:oauth-device-code" for openai-chatgpt), offer a one-time browser login BEFORE fetching models -- so a first-time user gets the live model catalog instead of the stale fallback list the owner's transcript hit. Placement: after the pre-model Phase-1 fields, before the "Default Model" header -- so it only runs on the interactive, non-azure path (the azure-openai deployment_name path and non_interactive mode are unaffected, matching their existing semantics). Duck-typed via hasattr(auth_status)/hasattr(login), per this PR's constraint to merge safely independent of the parallel provider-module PR: a provider that declares the capability but doesn't yet implement the methods is treated exactly like one with no login flow at all. Reuses ONE provider instance for both the login check and the model fetch (matching provider_loader.py:161's existing _try_instantiate_provider call for the model phase): extracted the "call list_models and clean up" half of get_provider_models() into list_models_for_instance(), so a caller that already holds a live instance (this wizard step) doesn't instantiate a second, separate one just to list models. Safety: the login-time model prefetch is wrapped in the exact same connectivity/generic-exception safety net _prompt_model_selection() uses for its own fetch (_safely_fetch_models_from_instance()) -- so this can never reintroduce the raw-traceback bug this whole PR is about. Ctrl-C at the login confirmation prompt propagates to configure_provider()'s own outer except handler, landing the same clean "Cancelled." abort as any other prompt (commit 1). Decline or failure prints exactly one yellow line ("Skipping login -- model list may be limited; run `amplifier provider login ` later") and continues gracefully. Factored _run_provider_login() as the shared login-driving helper (sync or async login(), print_fn rendering through this module's rich console) -- reused by the `amplifier provider login ` subcommand in the next commit, so the two call sites can't diverge on how login() is invoked. Tests (tests/test_provider_login_wizard_step.py, 20 cases): capability gate (no auth:* -> no instantiation attempt at all); duck-typing fallback when methods are missing; already-authenticated skips the prompt; a broken auth_status() never crashes the wizard; accept/decline/failure paths and their exact console messages; _run_provider_login() sync/async/ exception/KeyboardInterrupt-propagation; _safely_fetch_models_from_instance() success/ConnectionError/generic-exception; and full configure_provider() integration proving (a) a provider with no auth capability is completely unaffected (regression guard) and (b) a provider with the capability passes its login-time prefetched models straight into _prompt_model_selection() rather than fetching twice. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_app_cli/provider_config_utils.py | 185 +++++++- amplifier_app_cli/provider_loader.py | 46 +- tests/test_provider_login_wizard_step.py | 470 +++++++++++++++++++++ 3 files changed, 694 insertions(+), 7 deletions(-) create mode 100644 tests/test_provider_login_wizard_step.py diff --git a/amplifier_app_cli/provider_config_utils.py b/amplifier_app_cli/provider_config_utils.py index da277e7e..b29eedf0 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. @@ -974,12 +1133,36 @@ 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 diff --git a/amplifier_app_cli/provider_loader.py b/amplifier_app_cli/provider_loader.py index 3f47c14c..fffc3e71 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/tests/test_provider_login_wizard_step.py b/tests/test_provider_login_wizard_step.py new file mode 100644 index 00000000..94f24c92 --- /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 From b25d28bd1464efe7a8d19e58c4b9c4959e443b3b Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Sat, 29 Aug 2026 21:00:49 -0700 Subject: [PATCH 6/6] feat(provider): `amplifier provider login ` subcommand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a standalone way to (re)authenticate an OAuth-capable provider outside the configuration wizard -- for a first login after declining it during `provider add`/`provider edit`, or to re-authenticate after a token expires. Resolves the provider (a helpful, non-crashing error naming `provider install ` if it isn't installed yet), fetches get_info() and checks for an "auth:*" capability, then instantiates it with the saved config for that instance id when one exists in settings (empty dict otherwise -- never configuring is not an error for a provider that's about to be logged in for the first time). Duck-types auth_status()/login() the same way the wizard's login step does (commit 5) -- a provider that declares no "auth:*" capability at all, or declares one but doesn't (yet) implement the methods, gets the same clean message: "'' uses API-key configuration -- see `amplifier provider edit `." Reports the current auth_status(), skips straight to "Already logged in" when already authenticated, and otherwise drives the login via the shared _run_provider_login() helper (commit 5) -- the wizard and this command can never diverge on how login() is invoked. Reports the resulting status and exits non-zero on failure. Tests (tests/test_provider_login_command.py, 9 cases, stub provider classes throughout): command registration; not-installed error names `provider install`; no-auth-capability and duck-type-missing-methods both give the clean "API-key configuration" message; already-authenticated short-circuits without calling login(); instantiation uses the saved per-instance config when present and {} when the provider was never configured; successful and failed login report the right message and exit code. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_app_cli/commands/provider.py | 111 +++++++- tests/test_provider_login_command.py | 334 +++++++++++++++++++++++++ 2 files changed, 443 insertions(+), 2 deletions(-) create mode 100644 tests/test_provider_login_command.py diff --git a/amplifier_app_cli/commands/provider.py b/amplifier_app_cli/commands/provider.py index 8b9fb2e1..450e114f 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/tests/test_provider_login_command.py b/tests/test_provider_login_command.py new file mode 100644 index 00000000..7263a7f2 --- /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