Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tusharmath merged 5 commits into from
Mar 31, 2026
+50 −38
Merged

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

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8f83fa5
fix: prevent partial config write when model selection is cancelled d…
Mar 28, 2026
5c3431f
[autofix.ci] apply automated fixes
autofix-ci[bot] Mar 30, 2026
cb10874
[autofix.ci] apply automated fixes (attempt 2/3)
autofix-ci[bot] Mar 30, 2026
df998d6
Merge branch 'main' into fix/2714-atomic-model-provider-selection-on-…
tusharmath Mar 30, 2026
9688afa
Merge branch 'main' into fix/2714-atomic-model-provider-selection-on-…
tusharmath Mar 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 50 additions & 38 deletions crates/forge_main/src/ui.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use convert_case::{Case, Casing};
use forge_api::{
API, AgentId, AnyProvider, ApiKeyRequest, AuthContextRequest, AuthContextResponse, ChatRequest,
ChatResponse, CodeRequest, Conversation, ConversationId, DeviceCodeRequest, Event,
InterruptionReason, Model, ModelId, Provider, ProviderId, TextMessage, UserPrompt,
InterruptionReason, ModelId, Provider, ProviderId, TextMessage, UserPrompt,
};
use forge_app::utils::{format_display_path, truncate_key};
use forge_app::{CommitResult, ToolResolver};
Expand Down Expand Up @@ -127,14 +127,6 @@ impl<A: API + ConsoleWriter + 'static, F: Fn() -> A + Send + Sync> UI<A, F> {
self.spinner.ewrite_ln(title)
}

/// Retrieve available models
async fn get_models(&mut self) -> Result<Vec<Model>> {
self.spinner.start(Some("Loading"))?;
let models = self.api.get_models().await?;
self.spinner.stop(None)?;
Ok(models)
}

/// Helper to get provider for an optional agent, defaulting to the current
/// active agent's provider
async fn get_provider(&self, agent_id: Option<AgentId>) -> Result<Provider<Url>> {
Expand Down Expand Up @@ -2027,7 +2019,9 @@ impl<A: API + ConsoleWriter + 'static, F: Fn() -> A + Send + Sync> UI<A, F> {
) -> Result<Option<ModelId>> {
// Check if provider is set otherwise first ask to select a provider
if self.api.get_default_provider().await.is_err() {
self.on_provider_selection().await?;
if !self.on_provider_selection().await? {
return Ok(None);
}

// Check if a model was already selected during provider activation
// Return None to signal the model selection is complete and message was already
Expand Down Expand Up @@ -2686,15 +2680,16 @@ impl<A: API + ConsoleWriter + 'static, F: Fn() -> A + Send + Sync> UI<A, F> {
Ok(Some(model))
}

async fn on_provider_selection(&mut self) -> Result<()> {
async fn on_provider_selection(&mut self) -> Result<bool> {
// Select a provider
// If no provider was selected (user canceled), return early
let any_provider = match self.select_provider().await? {
Some(provider) => provider,
None => return Ok(()),
None => return Ok(false),
};

self.activate_provider(any_provider).await
self.activate_provider(any_provider).await?;
Ok(true)
}

/// Activates a provider by configuring it if needed, setting it as default,
Expand Down Expand Up @@ -2741,20 +2736,22 @@ impl<A: API + ConsoleWriter + 'static, F: Fn() -> A + Send + Sync> UI<A, F> {
provider: Provider<Url>,
model: Option<ModelId>,
) -> Result<()> {
// Set the provider via API
self.api.set_default_provider(provider.id.clone()).await?;

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

// If a model was pre-selected (e.g. from :model), validate and set it
// directly without prompting
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"),
Expand All @@ -2763,20 +2760,35 @@ impl<A: API + ConsoleWriter + 'static, F: Fn() -> A + Send + Sync> UI<A, F> {
}

// Check if the current model is available for the new provider

let current_model = self.api.get_default_model().await;
if let Some(current_model) = current_model {
let models = self.get_models().await?;
let model_available = models.iter().any(|m| m.id == current_model);

if !model_available {
// Prompt user to select a new model, scoped to the activated provider
self.writeln_title(TitleFormat::info("Please select a new model"))?;
self.on_model_selection(Some(provider.id.clone())).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(());
}
} else {
// No model set, select one now scoped to the activated provider
self.on_model_selection(Some(provider.id.clone())).await?;
}
// 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"),
)?;
Comment on lines 2749 to +2791
Copy link
Copy Markdown
Contributor

@graphite-app graphite-app bot Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fix in Graphite

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


Ok(())
}
Expand Down Expand Up @@ -2883,19 +2895,19 @@ impl<A: API + ConsoleWriter + 'static, F: Fn() -> A + Send + Sync> UI<A, F> {
// Ensure we have a model selected before proceeding with initialization
let active_agent = self.api.get_active_agent().await;

// Validate provider is configured before loading agents
// If provider is set in config but not configured (no credentials), prompt user
// to login
if self.api.get_default_provider().await.is_err() && !self.on_provider_selection().await? {
return Ok(());
}

let mut operating_model = self.get_agent_model(active_agent.clone()).await;
if operating_model.is_none() {
// Use the model returned from selection instead of re-fetching
operating_model = self.on_model_selection(None).await?;
}

// Validate provider is configured before loading agents
// If provider is set in config but not configured (no credentials), prompt user
// to login
if self.api.get_default_provider().await.is_err() {
self.on_provider_selection().await?;
}

if first {
// For chat, we are trying to get active agent or setting it to default.
// So for default values, `/info` doesn't show active provider, model, etc.
Expand Down
Loading