Skip to content

fix: onboarding defects (Ctrl-C data corruption, false secret-scan warning, traceback leaks) + openai-chatgpt first-class + login wizard - #287

Merged
Brian Krabach (bkrabach) merged 6 commits into
mainfrom
fix/onboarding-defects-and-openai-chatgpt-first-class
Aug 30, 2026
Merged

fix: onboarding defects (Ctrl-C data corruption, false secret-scan warning, traceback leaks) + openai-chatgpt first-class + login wizard#287
Brian Krabach (bkrabach) merged 6 commits into
mainfrom
fix/onboarding-defects-and-openai-chatgpt-first-class

Conversation

@bkrabach

Copy link
Copy Markdown
Collaborator

The owner's broken onboarding transcript

$ amplifier provider add openai-chatgpt
...model fetch...
Traceback (most recent call last):
  ...
amplifier_module_provider_openai_chatgpt.AuthenticationError: ...
  (raw traceback dumped to terminal)
...falls back to a stale/empty model list...
Choice [1/2/3]: ^C
✓ OpenAI ChatGPT configured
⚠ Could not resolve provider module '...' for entry '...' -- skipping plaintext-secret scan for this entry.

Four distinct defects, all hit in one provider add openai-chatgpt run, plus
provider-openai-chatgpt was never actually registered as a well-known
provider. This PR fixes all four, registers the provider, and adds the
onboarding improvement (wizard login step) the transcript was missing.

Fix 1 (highest severity, data-corrupting): Ctrl-C during model selection silently "succeeded"

Root cause: _prompt_model_selection() already returns None on
KeyboardInterrupt/EOFError (provider_config_utils.py:187-188, documented at
:93-94). Its caller inside configure_provider() only checked
if selected_model: -- 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 "✓ ... configured", and returned a config dict
instead of None.

Fix: disambiguate at the call site: selected_model is None now aborts
configure_provider() immediately with the same "Cancelled." message the
outer except (KeyboardInterrupt, EOFError) handler already 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" unchanged.

This single fix (inside configure_provider() itself) lands the abort on
all three call sites that already correctly check if config is None: ...
after calling it: 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.

Fix 2: false "Could not resolve provider module" warning

Root cause: normalize_provider_secrets() printed a loud yellow warning
whenever _secret_field_id_for(module_id) returned None -- but that's the
return value for TWO different situations: a genuine resolution failure, and
(equally) a provider that resolves fine but simply declares no
field_type == "secret" ConfigField at all -- the normal shape for any
OAuth-based provider (openai-chatgpt, github-copilot, ...).

Fix: call get_provider_info(module_id) once per entry and branch on it
directly. None keeps the loud warning (genuine failure). A resolved info
with no secret field now only logs at debug level. Added
_secret_config_field_from_info()/_secret_field_id_for_info() so the
already-fetched info dict is reused instead of a second get_provider_info()
call.

Fix 3: the traceback-leak class (kills it for ALL providers, not just this one)

Root cause: amplifier-app-cli configures no logging anywhere --
verified zero basicConfig()/dictConfig()/addHandler() calls in the
codebase. With no root handlers, 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 raw AuthenticationError leaked during the model fetch above -- and it
would leak the same way for any provider, any logged exception.

This also explains why _attach_llm_error_filter() (main.py:185-204) was
inert: its primary path attaches to an existing stderr StreamHandler, but
with none 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 only checks the originating
logger's own filters, not the root's, while walking handlers up the
hierarchy).

Fix: added _configure_console_logging(), called from main() before
_attach_llm_error_filter(). If root has no handlers, installs one stderr
StreamHandler at WARNING with a plain "%(message)s" formatter, and --
unless --verbose/-v/--debug is on the command line -- a filter that
clears exc_info/exc_text 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.

Fix 4: provider-openai-chatgpt was never a well-known/first-class provider

git log -p -- amplifier_app_cli/provider_sources.py and
git log --all --oneline | grep -i chatgpt (searched at f9a2e15, and
re-verified on this branch): no commit anywhere in history ever added it to
DEFAULT_PROVIDER_SOURCES. Added it there (alphabetically) and
"openai-chatgpt" -> "OpenAI ChatGPT" to _PROVIDER_DISPLAY_NAMES.
Deliberately no PROVIDER_DEPENDENCIES entry -- the module is standalone
(verified), unlike e.g. azure-openai extending openai.

The onboarding improvement: wizard login step + provider login

For a provider that declares an "auth:*" capability in get_info()
(e.g. "auth:oauth-device-code"), configure_provider() now offers a
one-time browser login before fetching models, so a first-time user
gets the live model catalog instead of the stale/empty fallback list the
transcript above hit. Decline or failure prints one graceful yellow line
and continues (never a traceback); Ctrl-C aborts cleanly via fix 1's
mechanism. Also added amplifier provider login <id> to (re)authenticate
later, sharing the same login-driving helper as the wizard step.

The duck-typed auth contract (why this merges safely on its own)

A parallel provider-module PR is adding auth_status() -> "authenticated" | "expired" | "unauthenticated", async login(print_fn=None) -> bool, and an
"auth:oauth-device-code" capability in get_info() to
provider-openai-chatgpt. This PR never assumes those exist -- it checks
get_info()["capabilities"] for an "auth:"-prefixed string, then
hasattr(provider, "auth_status")/hasattr(provider, "login") before using
them. A provider that declares the capability but doesn't (yet) implement
the methods is treated exactly like one with no login flow at all (instance
still reused for model-fetch efficiency; no crash, no login prompt).
The login step activates fully once the provider PR merges -- no
follow-up change needed here.

Quality

  • Fail-before/pass-after evidence for fixes 1 and 2 (stash-compared against
    this branch's own fix commits).
  • Full suite: 1508 passed baseline -> 1560 passed / 0 failed after (52 net
    new regression tests, zero regressions).
  • ruff format/ruff check clean on every touched file (this repo's own
    toolchain and config -- CI here runs pytest -q only, no lint job).
  • One pre-existing self-audit test (test_discover_providers_clean_install .py::...test_provider_display_names_are_correct_for_all_fallbacks) is
    updated in the same commit that added the new provider, per its own
    "keep me in sync" comment.

Conventional commits, one fix/feature per commit, ordered as designed.

🤖 Generated with Amplifier

Co-Authored-By: Amplifier 240397093+microsoft-amplifier@users.noreply.github.com

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>
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>
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>
…rovider

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>
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 <id>` 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 <id>` 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>
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 <id>` 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: "'<display>' uses API-key configuration -- see
`amplifier provider edit <id>`." 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>
@bkrabach

Copy link
Copy Markdown
Collaborator Author

All CI checks are green (license/cla, pytest across ubuntu/macos/windows x py3.11/3.12, and the integration job). This PR is self-authored by a maintainer session and is being merged with admin privileges at the repository owner's direct instruction, per the documented maintainer admin-merge pattern (checks-green-first; never merge over a failing check). Branch review-gate (REVIEW_REQUIRED) is bypassed via --admin for this reason; branch is not deleted on merge.

@bkrabach
Brian Krabach (bkrabach) merged commit 7f45c52 into main Aug 30, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants