fix(tui): make provider onboarding navigable and escapable (#4763)

[Hmbown]

Closes #4763.

The trap

A returning user on the xAI OAuth route hit a closed loop at startup: an
empty Step 3/4 backdrop → the OAuth modal → back to the empty backdrop.
Ctrl+C was swallowed, so the only exit was Esc-then-Ctrl+C.

What changed

1. The provider list is visible and navigable. open_onboarding_provider_picker
now uses a new ProviderPickerView::new_for_onboarding, which focuses the
persisted route but stays on Stage::List. Key/OAuth entry is reached by
picking a row, never by opening straight into it. /setup and /provider
keep the existing new_for_setup jump-to-key-entry behavior.

2. Esc backs out one stage. ui.rs no longer intercepts Esc on the
picker's behalf. The picker already had correct per-stage Esc handling
(KeyEntry → List, ModelPick → KeyEntry, Confirm → ModelPick); it just never
saw the key. Dismissal from Stage::List emits ProviderPickerDismissed,
whose handler already calls back_from_provider_onboarding — so the
missing-key-recovery semantics at ui.rs:807 are preserved on the final
pop, and the non-mutating "Esc during onboarding never touches config"
invariant still holds (no picker sub-stage Esc mutates config or emits).

3. Ctrl+C quits from anywhere. Key routing during onboarding now goes
through one pure function, onboarding_key_route, which returns Quit for
Ctrl+C in any onboarding state before any modal gets a chance to swallow it.

4. The backdrop tells the truth. The footer said "Press 0-9 to choose,
↑/↓ to move, Enter to continue" next to no list. It now reads "Enter opens
the provider list · Esc goes back · Ctrl+C quits", updated in all 7 locale
files that carry OnboardProviderFooter (zh-Hant.json does not define it).

5. External CLI grant reuse completes onboarding. Found while verifying
the issue's open question about the E path. ProviderPickerExternalConsentConfirmed
persisted consent and switched the provider but never called
complete_provider_picker_onboarding, unlike its siblings
ProviderPickerApplied and ProviderPickerSetupConfirmed. Since the picker
closes on EmitAndClose, a successful grant left onboarding == Provider
with an empty view stack — i.e. it dumped the user back onto the empty
backdrop. This is the second half of the reported loop.

Investigation: why needs_api_key was true

This is working as designed — no fix needed, and I did not change it.

The reporter's ~/.grok/auth.json was fresh (<24h) but that is not the
relevant clock. The entry carries expires_at roughly 6 hours after issue;
the file I inspected showed a 6-hour access-token lifetime with a
refresh_token sitting right beside it.

credentials_valid (xai_oauth.rs:225) accepts a refresh token for
Codewhale-owned storage, but for an external grant it requires
entry_access_token_is_fresh — an access token that has not expired. That
asymmetry is deliberate and load-bearing: read-only external consent never
refreshes, rewrites, or makes network calls against another CLI's file. It
is pinned by the existing tests
expired_read_only_external_credentials_never_refresh_rewrite_or_network
and disabled_external_grok_credentials_cause_zero_external_io.

So: Grok CLI token expires after ~6h → external grant stops counting →
has_api_key_for false → needs_api_key true → onboarding opens. Correct
behavior; the defect was that onboarding was a trap, not that it opened.

I added expired_external_grok_grant_reads_as_missing_key_despite_refresh_token
to pin this so the entry condition stays explainable, asserting both
directions (expired external grant → missing; fresh → ready).

Follow-up worth filing separately: this state deserves an explanation in
the UI ("your reused Grok CLI token expired — run grok login, or use
Codewhale-owned device login"). Right now the user gets a bare provider step
with no hint that a 6-hour timer is what moved. I left it out to keep this
PR scoped.

On the E path specifically (the issue's second open question): it
completes onboarding only when the grant actually yields a working route.
With an already-expired token — the reporter's exact case — pressing E
re-affirms consent that is already recorded, switch_provider fails, and
the user lands back on the backdrop. So the workaround in the issue
("press E to reuse existing Grok CLI credentials") does not work for an
expired token; device login or grok login is required. The backdrop is at
least now honest and escapable in that case.

Tests

Six new tests, all in existing patterns:

Test	Pins
onboarding_catalog_focuses_missing_provider_without_leaving_the_list	picker opens at Stage::List during onboarding
onboarding_escape_walks_key_entry_back_to_the_list_then_dismisses	Esc: key-entry → list → dismiss
onboarding_ctrl_c_quits_even_with_the_provider_picker_on_the_view_stack	Ctrl+C quits with the picker on the stack
onboarding_escape_is_routed_to_the_provider_picker_not_intercepted	Esc reaches the picker; non-onboarding keys still hit the legacy switch
external_grant_reuse_completes_provider_onboarding	a satisfied provider does not return to the provider step
expired_external_grok_grant_reads_as_missing_key_despite_refresh_token	the root trigger, both directions

Verification

PTY reproduction (pty + pyte, 120×40) against the built binary with a
synthetic HOME reproducing the reported config — provider = "xai",
auth_mode = "oauth", read-only grok_cli consent, expired
~/.grok/auth.json:

• Provider step renders the full navigable catalog, focused on xAI.
• Enter on xAI → "OAuth login — xAI"; Esc returns to the list (previously
  popped the whole modal).
• Ctrl+C exits from the list and from inside the OAuth sub-stage — the
  exact reported trap. PTY closes, process exits.
• A/B on one fixture (fresh Grok token, consent granted via E):
  before this change E → empty Step 3/6 backdrop; after, onboarding
  advances to Step 5/6.

Gates, all run on this branch:

• cargo fmt --check — clean
• cargo clippy --workspace --all-features --locked -- -D warnings -A clippy::uninlined_format_args -A clippy::too_many_arguments -A clippy::unnecessary_map_or -A clippy::collapsible_if -A clippy::assertions_on_constants — clean
• cargo test -p codewhale-tui --bin codewhale-tui — 8146 passed, 0 failed, 4 ignored (8140 on main, +6 new)
• ./scripts/release/check-versions.sh — workspace=0.9.1, npm=0.9.1, lockfile in sync

No version bumps, tag moves, or publishing.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HniWoNaNt9K9pkxTrGx1zd

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @Hmbown's task in 3m 15s —— View job

---

Review: PR #4765 — provider onboarding trap fix

Todo list

• Gather context (diff vs origin/main, changed files)
• Review correctness / panics / non-exhaustive matches
• Review provider/model/route safety invariant
• Review reuse & simplification
• Review test coverage
• Review security (secrets, exec policy, input validation)
• Post final review summary

I read the diff against origin/main directly (not just the PR description) — ui.rs, provider_picker.rs, xai_oauth.rs, ui/tests.rs, and the locale files.

Correctness

• onboarding_key_route (ui.rs:807–838) is a clean pure-function extraction. Checked it against the pre-existing generic Ctrl+C handling (ctrl_c_disposition, ui.rs:5818–5850) and the legacy onboarding switch (ui.rs:5036) — the new immediate-shutdown-on-Ctrl+C behavior during onboarding is not a new, more aggressive disposition; it matches what the legacy onboarding switch already did at line 5036-5039. The picker-owning-the-keys case is genuinely new (that's the fix), but it doesn't diverge from established onboarding Ctrl+C semantics.
• top_kind() returns only the topmost view's kind, so if a different modal (Help, SessionPicker, …) is ever stacked above the provider picker during onboarding, onboarding_key_route correctly falls through to Legacy rather than misrouting — same as the code being replaced, so no regression there.
• The ProviderPickerExternalConsentConfirmed fix (ui.rs:13853–13862) mirrors the exact pattern already used by ProviderPickerApplied (13773-13777) and ProviderPickerSetupConfirmed (13798-13809): only calls complete_provider_picker_onboarding when switch_provider returns true. I traced switch_provider's failure path (ui.rs:9851-9899) — on failure it either re-opens a focused picker or leaves a status message, so the expired-token case the PR body describes (switched == false) correctly skips completing onboarding.
• No new unwrap/expect on fallible paths, no non-exhaustive matches — OnboardingKeyRoute is a 3-variant enum fully matched at both call sites.

Provider/model/route safety (EPIC #2608)

No provider/model inference from prefix strings or freeform text anywhere in this diff. Routing is entirely driven by explicit OnboardingState/ModalKind enum state and an explicit ApiProvider argument from the confirmed picker selection. Not applicable here beyond confirming absence.

Reuse & simplification

• new_for_setup_inner factoring (provider_picker.rs:1479-1498) is the right shape — new_for_setup and new_for_onboarding differ by exactly one bool (key_entry_for_missing_auth), no duplicated construction logic.
• Verified /setup and /provider call sites (ui.rs:10863, 13553) were left on new_for_setup unchanged — only open_onboarding_provider_picker moved to new_for_onboarding, exactly as described.
• The match onboarding_key_route(...) replacing the old nested if/pop/continue block is a genuine readability improvement, not just a refactor for its own sake — it's what makes the Ctrl+C fix reachable in one place.

Tests

Six new tests, all pinning real regressions rather than incidental behavior:

• The two onboarding_key_route unit tests (ui/tests.rs) directly exercise the routing table, including the "picker outside onboarding" and "different onboarding stage" negative cases.
• external_grant_reuse_completes_provider_onboarding and the picker-level onboarding_catalog_focuses_missing_provider_without_leaving_the_list / onboarding_escape_walks_key_entry_back_to_the_list_then_dismisses cover the three independent defects described in the PR.
• expired_external_grok_grant_reads_as_missing_key_despite_refresh_token (xai_oauth.rs) is a pure pinning test for pre-existing, unmodified behavior — correctly scoped as "no fix needed" per the PR body, and it asserts both directions (expired → missing, fresh → ready) rather than just one.
• Locale coverage: confirmed all 7 files that define OnboardProviderFooter were updated identically in structure (Enter/Esc/Ctrl+C), and confirmed zh-Hant.json genuinely doesn't define the key (not a missed file).

Security

No secret handling, shell/exec, or input-validation changes in this diff. The external-grant test writes synthetic credentials to a TempDir, not to a real credential path.

Nit (non-blocking)

None worth flagging — this is a tightly scoped fix with the diff matching its description at every claim I checked.

Note: I was unable to run cargo build/cargo test/cargo clippy myself in this sandboxed environment (Bash tool execution is restricted), so I relied on static review of the diff plus tracing the surrounding call graph. The PR body's reported gate results (cargo fmt --check, clippy clean, 8146 passed/0 failed/4 ignored) were not independently re-run here.

[claude]

Claude finished @Hmbown's task in 2m 29s —— View job

---

Review: PR #4765 — provider onboarding trap fix (update on synchronize)

Todo list

• Identify what changed since the last review (dbd5457 → 1d0085c)
• Review the new commit for correctness
• Verify the updated test assertion actually matches runtime behavior (not just restated)
• Confirm no other files changed since the last review
• Post updated summary

What's new since the last review

Only one commit landed after my previous pass: 1d0085c — test(pty): follow current Kimi recovery detail, touching a single line in crates/tui/tests/qa_pty.rs:

-h.wait_for_text("does not import Kimi CLI credentials", BOOT_TIMEOUT)?;
+h.wait_for_text("missing MOONSHOT_API_KEY / KIMI_API_KEY", BOOT_TIMEOUT)?;

No other files changed; crates/tui/src/tui/provider_picker.rs, ui.rs, xai_oauth.rs, and the locale files are unchanged from what I reviewed on dbd5457, and that review still stands (no blocking issues found there).

Verifying the new assertion isn't just a rubber-stamp string match

I traced the actual message-construction path for this scenario rather than trusting the commit message:

• The test's returning_missing_kimi_code_key_opens_picker fixture configures provider = "moonshot" with base_url = "https://api.kimi.com/coding/v1" and no api_key, no OAuth marker, no imported-token config (crates/tui/tests/qa_pty.rs:686-691).
• missing_auth_message (crates/tui/src/tui/provider_picker.rs:1213-1234) special-cases ApiProvider::Moonshot only when provider_config_uses_kimi_imported_token is true — that branch returns "Kimi OAuth is unavailable; configure a Kimi API key", not the string this test checks for. Since this fixture has no imported-token config, that branch is correctly skipped.
• The generic fallback at line 1233, format!("missing {}", provider.env_vars_label()), applies instead. env_vars_label() (crates/tui/src/config.rs:179-181) joins env_vars() with " / ", and Moonshot's env var list is ["MOONSHOT_API_KEY", "KIMI_API_KEY"] (crates/config/src/provider.rs:684) — producing exactly "missing MOONSHOT_API_KEY / KIMI_API_KEY".

So the new assertion matches the actual code path the picker now renders through for this scenario, not just a plausible-looking string. The old string ("does not import Kimi CLI credentials") still exists elsewhere (config.rs:5137, still exercised by other tests), so this wasn't a global find-replace that could have masked a regression — it's a scoped fix to the one test whose rendered surface changed as a side effect of routing this scenario through the picker's row-status message instead of the composer diagnostic. The config-immutability assertion below it (assert_eq! against config_before) is untouched.

No correctness, safety, or test-coverage issues found in this update.

Note: I could not run cargo build/cargo test/cargo clippy in this sandboxed environment (command execution requires approval that isn't available here), so this is a static trace of the message-construction call graph, not an executed test run. The PR body's previously reported gate results were not independently re-verified in this pass.