Merge pull request #2769 from Hmbown/codex/harvest-2755-provider-auth-rollback

fix(tui): harvest provider auth rollback
2026-06-04 20:16:17 -07:00
parent 4b61462eda 54f7556359
commit 499ec42c5d
5 changed files with 224 additions and 10 deletions
@@ -55,6 +55,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `/config` now reports the canonical `~/.codewhale/settings.toml` path for TUI
  settings while still reading legacy DeepSeek-branded settings fallbacks and
  migrating them into the CodeWhale home on load.
+- Provider switches now roll back transactionally when the first request to a
+  newly selected provider fails authentication: CodeWhale restores the previous
+  provider/model, model-ID passthrough, onboarding/API-key state, runtime
+  config, persisted provider selection, and engine handle so users can return
+  to DeepSeek after a failed Moonshot/Kimi switch (#2754, #2755). Thanks
+  @Dr3259 for the Windows repro and @cyq1017 for the draft fix.
 - `PATCH /v1/threads/{id}` can now update a thread's persisted workspace for
  GUI/runtime clients. Workspace changes reject active turns and evict idle
  cached engines so the next turn starts in the new workspace.
@@ -55,6 +55,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `/config` now reports the canonical `~/.codewhale/settings.toml` path for TUI
  settings while still reading legacy DeepSeek-branded settings fallbacks and
  migrating them into the CodeWhale home on load.
+- Provider switches now roll back transactionally when the first request to a
+  newly selected provider fails authentication: CodeWhale restores the previous
+  provider/model, model-ID passthrough, onboarding/API-key state, runtime
+  config, persisted provider selection, and engine handle so users can return
+  to DeepSeek after a failed Moonshot/Kimi switch (#2754, #2755). Thanks
+  @Dr3259 for the Windows repro and @cyq1017 for the draft fix.
 - `PATCH /v1/threads/{id}` can now update a thread's persisted workspace for
  GUI/runtime clients. Workspace changes reject active turns and evict idle
  cached engines so the next turn starts in the new workspace.
@@ -1216,6 +1216,17 @@ pub struct ToolEvidence {
    pub summary: String,
 }

+#[derive(Debug, Clone)]
+pub(crate) struct PendingProviderSwitch {
+    pub previous_provider: ApiProvider,
+    pub previous_model: String,
+    pub previous_model_ids_passthrough: bool,
+    pub previous_config: Config,
+    pub previous_onboarding: OnboardingState,
+    pub previous_onboarding_needs_api_key: bool,
+    pub previous_api_key_env_only: bool,
+}
+
 /// Global UI state for the TUI.
 #[allow(clippy::struct_excessive_bools)]
 pub struct App {
@@ -1272,6 +1283,9 @@ pub struct App {
    /// True when the active provider/base URL accepts arbitrary model IDs
    /// verbatim rather than DeepSeek-only aliases.
    pub model_ids_passthrough: bool,
+    /// Pending provider transition for transactional rollback when the next
+    /// auth failure indicates the new provider cannot be used.
+    pub pending_provider_switch: Option<PendingProviderSwitch>,
    /// Current reasoning-effort tier for DeepSeek thinking mode.
    /// Cycled via Shift+Tab; initialized from config at startup.
    pub reasoning_effort: ReasoningEffort,
@@ -2067,6 +2081,7 @@ impl App {
            last_effective_model: None,
            api_provider: provider,
            model_ids_passthrough,
+            pending_provider_switch: None,
            reasoning_effort,
            last_effective_reasoning_effort: None,
            workspace,
@@ -117,8 +117,8 @@ use crate::tui::workspace_context;
 use super::key_actions;

 use super::app::{
-    App, AppAction, AppMode, OnboardingState, QueuedMessage, ReasoningEffort, SidebarFocus,
-    StatusToastLevel, SubmitDisposition, TaskPanelEntry, TuiOptions,
+    App, AppAction, AppMode, OnboardingState, PendingProviderSwitch, QueuedMessage,
+    ReasoningEffort, SidebarFocus, StatusToastLevel, SubmitDisposition, TaskPanelEntry, TuiOptions,
    looks_like_slash_command_input, shell_command_from_bang_input,
 };
 use super::approval::{
@@ -1307,6 +1307,7 @@ async fn run_event_loop(
        let mut received_engine_event = false;
        let mut transcript_batch_updated = false;
        let mut queued_to_send: Option<QueuedMessage> = None;
+        let mut respawn_after_provider_rollback: Option<String> = None;
        {
            let mut rx = engine_handle.rx_event.write().await;
            loop {
@@ -1720,6 +1721,7 @@ async fn run_event_loop(
                        }
                        app.is_loading = false;
                        app.dispatch_started_at = None;
+                        app.pending_provider_switch = None;
                        app.offline_mode = false;
                        app.streaming_state.reset();
                        if was_locally_cancelled {
@@ -1986,7 +1988,18 @@ async fn run_event_loop(
                        envelope,
                        recoverable: _,
                    } => {
+                        let rollback_after_auth_failure =
+                            matches!(
+                                envelope.category,
+                                crate::error_taxonomy::ErrorCategory::Authentication
+                            ) && app.pending_provider_switch.is_some();
                        apply_engine_error_to_app(app, envelope);
+                        if rollback_after_auth_failure
+                            && let Some(rollback_warning) =
+                                rollback_provider_after_auth_failure(app, config)
+                        {
+                            respawn_after_provider_rollback = Some(rollback_warning);
+                        }
                    }
                    EngineEvent::Status { message } => {
                        app.status_message = Some(message);
@@ -2406,6 +2419,29 @@ async fn run_event_loop(
                }
            }
        }
+        if let Some(rollback_warning) = respawn_after_provider_rollback {
+            let _ = engine_handle.send(Op::Shutdown).await;
+            let engine_config = build_engine_config(app, config);
+            engine_handle = spawn_engine(engine_config, config);
+            if !app.api_messages.is_empty() {
+                let _ = engine_handle
+                    .send(Op::SyncSession {
+                        session_id: app.current_session_id.clone(),
+                        messages: app.api_messages.clone(),
+                        system_prompt: app.system_prompt.clone(),
+                        system_prompt_override: false,
+                        model: app.model.clone(),
+                        workspace: app.workspace.clone(),
+                    })
+                    .await;
+            }
+            let _ = engine_handle
+                .send(Op::SetCompaction {
+                    config: app.compaction_config(),
+                })
+                .await;
+            app.status_message = Some(rollback_warning);
+        }
        if let Some(index) = app.streaming_message_index {
            let committed = app.streaming_state.commit_text(0);
            if !committed.is_empty() {
@@ -4569,6 +4605,68 @@ pub(crate) fn apply_engine_error_to_app(
    // toast in the footer — that duplicates the transcript entry.
 }

+fn rollback_provider_after_auth_failure(app: &mut App, config: &mut Config) -> Option<String> {
+    let pending = app.pending_provider_switch.take()?;
+    let PendingProviderSwitch {
+        previous_provider,
+        previous_model,
+        previous_model_ids_passthrough,
+        previous_config,
+        previous_onboarding,
+        previous_onboarding_needs_api_key,
+        previous_api_key_env_only,
+    } = pending;
+
+    *config = previous_config;
+    app.api_provider = previous_provider;
+    app.set_model_selection(previous_model.clone());
+    app.provider_models
+        .insert(previous_provider.as_str().to_string(), previous_model);
+    app.model_ids_passthrough = previous_model_ids_passthrough;
+    app.update_model_compaction_budget();
+    app.clear_model_scoped_telemetry();
+    app.offline_mode = false;
+    app.onboarding = previous_onboarding;
+    app.onboarding_needs_api_key = previous_onboarding_needs_api_key;
+    app.api_key_env_only = previous_api_key_env_only;
+
+    let persistence_error = (|| -> anyhow::Result<()> {
+        commands::persist_root_string_key(
+            app.config_path.as_deref(),
+            "provider",
+            previous_provider.as_str(),
+        )?;
+        let mut settings = crate::settings::Settings::load()?;
+        settings.default_provider = Some(previous_provider.as_str().to_string());
+        settings.set_model_for_provider(
+            previous_provider.as_str(),
+            &app.model_selection_for_persistence(),
+        );
+        if matches!(
+            previous_provider,
+            ApiProvider::Deepseek | ApiProvider::DeepseekCN
+        ) {
+            settings.set("default_model", &app.model_selection_for_persistence())?;
+        }
+        settings.save()?;
+        Ok(())
+    })()
+    .err()
+    .map(|err| format!("provider rollback not fully persisted: {err}"));
+
+    Some(match persistence_error {
+        Some(warning) => format!(
+            "Provider switch failed and has been rolled back to {}. {}",
+            previous_provider.as_str(),
+            warning
+        ),
+        None => format!(
+            "Provider switch failed and has been rolled back to {}.",
+            previous_provider.as_str()
+        ),
+    })
+}
+
 fn persist_offline_queue_state(app: &App) {
    if app.queued_messages.is_empty() && app.queued_draft.is_none() {
        persistence_actor::persist(PersistRequest::ClearOfflineQueue);
@@ -5339,10 +5437,17 @@ async fn switch_provider(
 ) {
    let previous_provider = app.api_provider;
    let previous_model = app.model.clone();
-    let previous_provider_str = config.provider.clone();
-    let previous_base_url = config.base_url.clone();
-    let previous_default_text_model = config.default_text_model.clone();
-    let previous_providers = config.providers.clone();
+    let previous_model_ids_passthrough = app.model_ids_passthrough;
+    let previous_config = config.clone();
+    app.pending_provider_switch = Some(PendingProviderSwitch {
+        previous_provider,
+        previous_model: previous_model.clone(),
+        previous_model_ids_passthrough,
+        previous_config: previous_config.clone(),
+        previous_onboarding: app.onboarding,
+        previous_onboarding_needs_api_key: app.onboarding_needs_api_key,
+        previous_api_key_env_only: app.api_key_env_only,
+    });

    config.provider = Some(target.as_str().to_string());
    if matches!(target, ApiProvider::NvidiaNim)
@@ -5368,10 +5473,8 @@ async fn switch_provider(
    }

    if let Err(err) = DeepSeekClient::new(config) {
-        config.provider = previous_provider_str;
-        config.base_url = previous_base_url;
-        config.default_text_model = previous_default_text_model;
-        config.providers = previous_providers;
+        app.pending_provider_switch = None;
+        *config = previous_config;
        app.add_message(HistoryCell::System {
            content: format!(
                "Failed to switch provider to {}: {err}\nProvider unchanged ({}).",
@@ -7300,6 +7300,89 @@ fn recoverable_engine_error_does_not_enter_offline_mode() {
    let _ = ErrorEnvelope::transient("");
 }

+#[tokio::test]
+async fn provider_switch_auth_error_restores_previous_provider_and_model() {
+    use crate::error_taxonomy::ErrorEnvelope;
+
+    let _home = SettingsHomeGuard::new();
+    let mut app = create_test_app();
+    app.api_provider = ApiProvider::Deepseek;
+    app.model = "deepseek-v4-pro".to_string();
+    app.model_ids_passthrough = false;
+    app.onboarding = OnboardingState::None;
+    app.onboarding_needs_api_key = false;
+    app.api_key_env_only = true;
+    let mut engine = mock_engine_handle();
+    let mut config = Config {
+        provider: Some("deepseek".to_string()),
+        api_key: Some("deepseek-key".to_string()),
+        default_text_model: Some("deepseek-v4-pro".to_string()),
+        providers: Some(ProvidersConfig {
+            deepseek: ProviderConfig {
+                api_key: Some("deepseek-key".to_string()),
+                ..Default::default()
+            },
+            moonshot: ProviderConfig {
+                api_key: Some("kimi-key".to_string()),
+                ..Default::default()
+            },
+            ..Default::default()
+        }),
+        ..Default::default()
+    };
+
+    switch_provider(
+        &mut app,
+        &mut engine.handle,
+        &mut config,
+        ApiProvider::Moonshot,
+        Some("kimi-k2.6".to_string()),
+    )
+    .await;
+    assert_eq!(app.api_provider, ApiProvider::Moonshot);
+    assert_eq!(config.provider.as_deref(), Some("moonshot"));
+    assert!(app.pending_provider_switch.is_some());
+
+    apply_engine_error_to_app(
+        &mut app,
+        ErrorEnvelope::fatal_auth("Authentication failed: invalid API key"),
+    );
+    let rollback_status = rollback_provider_after_auth_failure(&mut app, &mut config)
+        .expect("auth failure after provider switch should roll back");
+
+    assert_eq!(app.api_provider, ApiProvider::Deepseek);
+    assert_eq!(app.model, "deepseek-v4-pro");
+    assert!(!app.model_ids_passthrough);
+    assert!(!app.offline_mode);
+    assert_eq!(app.onboarding, OnboardingState::None);
+    assert!(!app.onboarding_needs_api_key);
+    assert!(app.api_key_env_only);
+    assert_eq!(config.provider.as_deref(), Some("deepseek"));
+    assert_eq!(
+        config.default_text_model.as_deref(),
+        Some("deepseek-v4-pro")
+    );
+    let settings = crate::settings::Settings::load().expect("load settings");
+    assert_eq!(settings.default_provider.as_deref(), Some("deepseek"));
+    assert_eq!(
+        settings
+            .provider_models
+            .as_ref()
+            .and_then(|models| models.get("deepseek"))
+            .map(String::as_str),
+        Some("deepseek-v4-pro")
+    );
+    assert_eq!(settings.default_model.as_deref(), Some("deepseek-v4-pro"));
+    assert!(app.pending_provider_switch.is_none());
+    assert!(rollback_status.contains("Provider switch failed"));
+    assert!(
+        app.status_message
+            .as_deref()
+            .map_or(true, |status| !status.contains("Provider switch failed")),
+        "status message is set by the async event loop after engine respawn"
+    );
+}
+
 #[test]
 fn stream_error_marks_active_turn_failed_without_waiting_for_turn_complete() {
    use crate::error_taxonomy::ErrorEnvelope;
@@ -7363,6 +7446,7 @@ fn non_recoverable_engine_error_enters_offline_mode() {
        app.status_message.is_none(),
        "non-recoverable error should NOT set status_message — already in transcript as HistoryCell::Error"
    );
+    assert!(app.pending_provider_switch.is_none());
 }

 #[test]