Skip to content

Honor configured providers during login and model discovery - #362

Merged
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
fix/provider-config-instantiation
Oct 1, 2026
Merged

Brian Krabach (bkrabach) merged 1 commit into
mainfrom
fix/provider-config-instantiation

Conversation

@bkrabach

@bkrabach Brian Krabach (bkrabach) commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Provider setup collected settings but discarded them before constructing the provider for login or model discovery. A user-selected authentication mode or credential file could therefore silently use the provider's default account instead. The shared loader now binds the constructor once, passes an independent copy of the selected configuration and declared connection arguments, and preserves validation errors instead of retrying with defaults.

provider login also resolves an exact configured instance before its module and rejects ambiguous module-only matches rather than choosing an account by priority. Authentication choices, credential paths, and consent remain provider-owned metadata and behavior; this change adds no provider-specific authentication code.

Validation: 76 focused loader, wizard, login, model-discovery, and optional-field tests passed, including an independent review rerun. Regression cases cover nested configuration isolation, constructor error preservation, optional server arguments, metadata-only discovery, configured login/catalog parity, and named-account ambiguity. The new tests reproduced 13 failures on the previous loader before the fix. No real authentication, paid inference, or installed-host changes were used.

Cross-repository acceptance also passed six hermetic cases against the current ChatGPT provider candidate's real metadata, constructor, login dispatch, and catalog dispatch. Both connection modes, default/custom credential paths, plan host identity/application name, settings persistence and reconfiguration, exact named-instance login alongside a second same-module account, legacy alias normalization, and invalid-mode rejection were exercised. Only remote authentication/catalog boundaries were replaced; network access was blocked. These external compatibility tests do not add a provider dependency to the CLI test suite.

@bkrabach

Copy link
Copy Markdown
Collaborator Author

Coordinated review result: no blocker found in this PR

Reviewed commit faaac1b262249eafcedcd468c2396a6856dc2285 together with provider #18 at 7130487b3f25ac9765bd99ce9c1a728f70d02752 and Unified #281 at fec5d145d4ff3cde5fe519d2306d5467c5c482c7. This assessment applies to those commits, not subsequent updates.

The earlier CLI/provider integration problems are resolved:

  1. Signature-bound construction carries an independent copy of selected configuration and does not retry with defaults after a constructor validation/runtime failure (provider_loader.py:327-367).
  2. Provider-declared mode/path/plan-host/application fields survive reconfiguration. Four interactive wizard cases used the installed provider's real metadata, constructor, login wrapper and catalog dispatch: Codex/Plan, each with default/custom paths. Only prompts and underlying auth/network services were mocked. The selected mode reached the correct catalog endpoint.
  3. Exact named-instance login resolves before the module installation check, and an ambiguous module-only login is rejected instead of choosing an account by priority (commands/provider.py:1226-1249). Combined edit/login probes confirmed the stored configuration is retained.

Verification: 227 targeted CLI tests passed in the combined isolated Python 3.13 / Core 2.0.1 environment. Provider defaults remain chatgpt_codex; Plan is explicit. Fable independently reviewed the source/probe evidence and likewise found the earlier constructor/configuration/named-login defects resolved; it did not execute tests.

The remaining coordinated-review requests are provider local metadata handling (#18) and Unified abandoned candidate cleanup / capped test-message UI (#281), not regressions introduced by this generic loader change. Real OAuth and paid inference were not tested, so this is not a live-service acceptance claim or a merge action.

@bkrabach
Brian Krabach (bkrabach) merged commit cef71e0 into main Oct 1, 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.

1 participant