fix: prevent partial config write when model selection is cancelled d…

[ashprakasan]

Summary

Problem
When switching providers via /login or forge provider login, if the user cancelled model selection after authenticating, the new provider was written to config but the model was never updated. This left the config in an invalid state — the active provider (e.g. Anthropic) had no compatible model set, while the previous model (e.g. gpt-4) still remained from the old provider (e.g. OpenAI).

Root Cause
In finalize_provider_activation, set_default_provider was called unconditionally at the top of the function — before model selection was confirmed. The return value of on_model_selection (Option) was also being discarded via ?, so a cancellation (Ok(None)) silently fell through without rolling back the provider write.

Fix
Two changes in finalize_provider_activation:

• Deferred set_default_provider to the end of the function, after model selection is confirmed
• Captured the return value of on_model_selection and added an early return if None, preserving the existing config untouched

The model compatibility check was also updated to use get_all_provider_models() scoped to the new provider instead of get_models(), since get_models() fetches from the currently active provider — which would be incorrect now that set_default_provider is deferred.

Manual Testing
Tested using the debug binary (./target/debug/forge):

• Started with provider = OpenAI, model = gpt-4
• Ran forge provider login, selected Anthropic, entered API key, confirmed as default provider
• When prompted to select a model, cancelled
• Verified config with forge config get provider and forge config get model
• Both still showed OpenAI and gpt-4 — existing config preserved correctly

Why No Automated Test
finalize_provider_activation calls on_model_selection → select_model → ForgeWidget::select, which requires a real TTY and is not mockable in the current architecture. This is noted in the existing test module comment. A follow-up PR will introduce the necessary abstractions (either a trait over ForgeWidget or extraction of pure logic) to make this flow unit-testable. Fixes #2714

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ tusharmath
❌ Aiswarya Prakasan

---

Aiswarya Prakasan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[graphite-app]

Unused API call to get_all_provider_models() that is immediately discarded. The result is fetched again at line 2775 inside the match branch. This causes an unnecessary duplicate network/database call.

// Remove this line - it's redundant

Spotted by Graphite



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

Critical logic bug: When model is Some, the provider is set twice - once at line 2752 and again unconditionally at line 2793. After setting the provider and model in the pre-selected path (lines 2752-2762), the function continues to the model selection check and provider-setting code instead of returning early. This defeats the purpose of the fix.

if let Some(model) = model {
    let model_id = self
        .validate_model(model.as_str(), Some(&provider.id))
        .await?;

    //set provider
    self.api.set_default_provider(provider.id.clone()).await?;

    self.writeln_title(
        TitleFormat::action(format!("{}", provider.id))
            .sub_title("is now the default provider"),
    )?;

    //set model
    self.api.set_default_model(model_id.clone()).await?;
    self.writeln_title(
        TitleFormat::action(model_id.as_str()).sub_title("is now the default model"),
    )?;
    
    return Ok(()); // ADD THIS - early return to prevent double provider set
}

Suggested change

	self.writeln_title(
	TitleFormat::action(format!("{}", provider.id))
	.sub_title("is now the default provider"),
	)?;
	//set model
	self.api.set_default_model(model_id.clone()).await?;
	self.writeln_title(
	TitleFormat::action(model_id.as_str()).sub_title("is now the default model"),
	)?;
	return Ok(());
	}
	// Check if the current model is available for the new provider
	let current_model = self.api.get_default_model().await;
	let _provider_models = self.api.get_all_provider_models().await?;
	let needs_model_selection = match current_model {
	None => true,
	Some(current_model) => {
	let provider_models = self.api.get_all_provider_models().await?;
	!provider_models
	.iter()
	.find(|pm| pm.provider_id == provider.id)
	.map(|pm| pm.models.iter().any(|m| m.id == current_model))
	.unwrap_or(false)
	}
	};
	if needs_model_selection {
	self.writeln_title(TitleFormat::info("Please select a new model"))?;
	let selected = self.on_model_selection(Some(provider.id.clone())).await?;
	if selected.is_none() {
	// User cancelled — preserve existing config untouched
	return Ok(());
	}
	}
	// Only reaches here if model is confirmed — safe to write provider now
	self.api.set_default_provider(provider.id.clone()).await?;
	self.writeln_title(
	TitleFormat::action(format!("{}", provider.id))
	.sub_title("is now the default provider"),
	)?;

Spotted by Graphite



Is this helpful? React 👍 or 👎 to let us know.