feat(providers): remove connected provider from model picker - #1297
Conversation
TheGreatAxios
left a comment
There was a problem hiding this comment.
Review · Request changes
The branch adds provider removal and its settings/auth cleanup path.
Findings
src/tui/runner/settings.ts:560— The advertised OAuth cleanup retry is unreachable afterremoveProfilefails. The first attempt has already deleted the settings entry; the refreshed auth-backed provider then appears only as a residual/ghost row, butdescribeRemoveProviderreturnsnullwhenever that settings entry is absent, so Alt+R is inert andonRemoveProvidercan never reach its orphan-cleanup branch at lines 589–616. Please allow that residual OAuth row to arm removal (or expose another reachable retry) so the notice at line 611 is actionable.
git diff --check 1ba2599...9099b649 passes. GitHub CI is green. Local bun run check passed lint, typecheck, dead-export checking, and build; its guarded suite hit unrelated connected-MCP preflight failures (8,028 pass, 7 fail, 1 error), so I relied on the green CI suite for full-test verification.
TheGreatAxios
left a comment
There was a problem hiding this comment.
Review follow-up · Request changes
Additional findings
-
src/auth/remove-provider.ts:27— OAuth-store ownership is inferred solely from the catalog name prefix. The custom/manual setup path persists the operator's trimmed name without reservingcodex/*orxai/*(src/tui/provider/submit.ts:67-73,168-174), so removing a custom API-key provider named, for example,codex/workalso callsremoveCodexProfile("work")and can delete an unrelated OAuth credential. Please determine credential ownership from provider provenance rather than the user-controlled name, or reserve these namespaces at creation. -
src/tui/product-host.ts:791— Confirming removal leaves the picker interactive while the async settings mutation runs.onRemoveProviderchecks whether the target is live before its first settings-write await (src/tui/runner/settings.ts:619-639), but the operator can press Enter immediately after confirmation and synchronously switch the session to that provider while the write is pending. The callback then deletes the now-live provider and its credential, bypassing the intended live-session guard. Please close/disable the row during removal or re-check the live identity before credential deletion/commit.
TheGreatAxios
left a comment
There was a problem hiding this comment.
Review follow-up · Fix verified
No findings in fix commit b223f4ce.
The residual OAuth row now exposes cleanup only when live source provenance proves OAuth ownership, custom codex/* and xai/* rows cannot authorize credential deletion by name alone, and pending removals block model selection until the async operation settles. Regression tests cover all three cases.
Verification: bun run check passes with 8,072 tests and 0 failures; all GitHub checks pass; git diff --check 9099b649..b223f4ce passes.
Summary
/modelpicker gains an Alt+R row action that deletes the focused provider account with a two-step armed confirm showing the blast radiusVerification
bun run checkpasses (lint, typecheck, dead-exports, build, guarded suite: 8069 pass, 0 fail)src/tui/provider-remove.test.ts,src/auth/remove-provider.test.ts, settings and picker casesFixes #1294