mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
Each provider setup function unconditionally cleared selected_model, so re-running the wizard with "Keep current provider? Yes" would lose the model name, forcing the user to re-select it every time. Now only clears selected_model when the backend actually changes (old model may be invalid for the new provider). When keeping the same provider, the model is preserved and Step 4 shows the "Keep current model" prompt. Co-authored-by: Claude Opus 4.6 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
200aed16cd
commit
56b7218897
+54
-8
@@ -1037,10 +1037,11 @@ impl SetupWizard {
|
||||
|
||||
/// Anthropic OAuth setup: extract token from `claude login` credentials.
|
||||
async fn setup_anthropic_oauth(&mut self) -> Result<(), SetupError> {
|
||||
self.settings.llm_backend = Some("anthropic".to_string());
|
||||
if self.settings.selected_model.is_some() {
|
||||
// Clear model only when switching providers (old model may be invalid)
|
||||
if self.settings.llm_backend.as_deref() != Some("anthropic") {
|
||||
self.settings.selected_model = None;
|
||||
}
|
||||
self.settings.llm_backend = Some("anthropic".to_string());
|
||||
|
||||
// Try to extract existing OAuth token from Claude Code credentials
|
||||
if let Some(token) = crate::config::ClaudeCodeConfig::extract_oauth_token() {
|
||||
@@ -1134,10 +1135,11 @@ impl SetupWizard {
|
||||
other => other,
|
||||
});
|
||||
|
||||
self.settings.llm_backend = Some(backend.to_string());
|
||||
if self.settings.selected_model.is_some() {
|
||||
// Clear model only when switching providers (old model may be invalid)
|
||||
if self.settings.llm_backend.as_deref() != Some(backend) {
|
||||
self.settings.selected_model = None;
|
||||
}
|
||||
self.settings.llm_backend = Some(backend.to_string());
|
||||
|
||||
// Check env var first
|
||||
if let Ok(existing) = std::env::var(env_var) {
|
||||
@@ -1196,10 +1198,11 @@ impl SetupWizard {
|
||||
&mut self,
|
||||
def: &crate::llm::ProviderDefinition,
|
||||
) -> Result<(), SetupError> {
|
||||
self.settings.llm_backend = Some(def.id.clone());
|
||||
if self.settings.selected_model.is_some() {
|
||||
// Clear model only when switching providers (old model may be invalid)
|
||||
if self.settings.llm_backend.as_deref() != Some(&def.id) {
|
||||
self.settings.selected_model = None;
|
||||
}
|
||||
self.settings.llm_backend = Some(def.id.clone());
|
||||
|
||||
let default_url = self
|
||||
.settings
|
||||
@@ -1234,10 +1237,11 @@ impl SetupWizard {
|
||||
secret_name: &str,
|
||||
display_name: &str,
|
||||
) -> Result<(), SetupError> {
|
||||
self.settings.llm_backend = Some(backend_id.to_string());
|
||||
if self.settings.selected_model.is_some() {
|
||||
// Clear model only when switching providers (old model may be invalid)
|
||||
if self.settings.llm_backend.as_deref() != Some(backend_id) {
|
||||
self.settings.selected_model = None;
|
||||
}
|
||||
self.settings.llm_backend = Some(backend_id.to_string());
|
||||
|
||||
let existing_url = self
|
||||
.settings
|
||||
@@ -3521,6 +3525,48 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// Regression test for #600: re-running provider setup for the same backend
|
||||
/// must NOT clear selected_model. Only switching to a different backend should.
|
||||
#[test]
|
||||
fn test_same_provider_preserves_selected_model() {
|
||||
let mut wizard = SetupWizard::new();
|
||||
wizard.settings.llm_backend = Some("ollama".to_string());
|
||||
wizard.settings.selected_model = Some("llama3".to_string());
|
||||
|
||||
// Simulate re-entering the same provider -- model should survive
|
||||
// (This is the check that each setup_* function now performs)
|
||||
if wizard.settings.llm_backend.as_deref() != Some("ollama") {
|
||||
wizard.settings.selected_model = None;
|
||||
}
|
||||
wizard.settings.llm_backend = Some("ollama".to_string());
|
||||
|
||||
assert_eq!(
|
||||
wizard.settings.selected_model.as_deref(),
|
||||
Some("llama3"),
|
||||
"model should be preserved when re-selecting the same provider"
|
||||
);
|
||||
}
|
||||
|
||||
/// Regression test for #600: switching to a different provider must clear
|
||||
/// selected_model since the old model may not be valid for the new backend.
|
||||
#[test]
|
||||
fn test_different_provider_clears_selected_model() {
|
||||
let mut wizard = SetupWizard::new();
|
||||
wizard.settings.llm_backend = Some("ollama".to_string());
|
||||
wizard.settings.selected_model = Some("llama3".to_string());
|
||||
|
||||
// Simulate switching to a different provider -- model should be cleared
|
||||
if wizard.settings.llm_backend.as_deref() != Some("openai") {
|
||||
wizard.settings.selected_model = None;
|
||||
}
|
||||
wizard.settings.llm_backend = Some("openai".to_string());
|
||||
|
||||
assert!(
|
||||
wizard.settings.selected_model.is_none(),
|
||||
"model should be cleared when switching providers"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_run_provider_setup_no_setup_hint() {
|
||||
// A provider with setup: None should not error. It should set the
|
||||
|
||||
Reference in New Issue
Block a user