From 546c70aeca9d7c6948a7eb1277c0b761235aeb1c Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Tue, 17 Mar 2026 16:07:26 +0100 Subject: [PATCH] agent: Allow all settings in the thread to apply immediately (#51762) Before we kind of had it mixed: you could change thinking but not model etc. We brute forced a solution by disabling, but it seems better to just allow all of them to be updated whenever we would build the request. Release Notes: - N/A --- crates/agent/src/tests/mod.rs | 107 ++++++++++++++++++ crates/agent/src/thread.rs | 56 ++++----- .../src/conversation_view/thread_view.rs | 21 +--- crates/agent_ui/src/model_selector_popover.rs | 48 ++------ crates/agent_ui/src/profile_selector.rs | 63 ++++------- 5 files changed, 169 insertions(+), 126 deletions(-) diff --git a/crates/agent/src/tests/mod.rs b/crates/agent/src/tests/mod.rs index 534b70aaee5cb62a0f343d5adf0ef7b196e49d94..7482ffda5854525f78efb2fcd3fd8cc9f757e3be 100644 --- a/crates/agent/src/tests/mod.rs +++ b/crates/agent/src/tests/mod.rs @@ -6478,3 +6478,110 @@ async fn test_streaming_tool_error_waits_for_prior_tools_to_complete(cx: &mut Te ] ); } + +#[gpui::test] +async fn test_mid_turn_model_and_settings_refresh(cx: &mut TestAppContext) { + let ThreadTest { + model, thread, fs, .. + } = setup(cx, TestModel::Fake).await; + let fake_model_a = model.as_fake(); + + thread.update(cx, |thread, _cx| { + thread.add_tool(EchoTool); + thread.add_tool(DelayTool); + }); + + // Set up two profiles: profile-a has both tools, profile-b has only DelayTool. + fs.insert_file( + paths::settings_file(), + json!({ + "agent": { + "profiles": { + "profile-a": { + "name": "Profile A", + "tools": { + EchoTool::NAME: true, + DelayTool::NAME: true, + } + }, + "profile-b": { + "name": "Profile B", + "tools": { + DelayTool::NAME: true, + } + } + } + } + }) + .to_string() + .into_bytes(), + ) + .await; + cx.run_until_parked(); + + thread.update(cx, |thread, cx| { + thread.set_profile(AgentProfileId("profile-a".into()), cx); + thread.set_thinking_enabled(false, cx); + }); + + // Send a message — first iteration starts with model A, profile-a, thinking off. + thread + .update(cx, |thread, cx| { + thread.send(UserMessageId::new(), ["test mid-turn refresh"], cx) + }) + .unwrap(); + cx.run_until_parked(); + + // Verify first request has both tools and thinking disabled. + let completions = fake_model_a.pending_completions(); + assert_eq!(completions.len(), 1); + let first_tools = tool_names_for_completion(&completions[0]); + assert_eq!(first_tools, vec![DelayTool::NAME, EchoTool::NAME]); + assert!(!completions[0].thinking_allowed); + + // Model A responds with an echo tool call. + fake_model_a.send_last_completion_stream_event(LanguageModelCompletionEvent::ToolUse( + LanguageModelToolUse { + id: "tool_1".into(), + name: "echo".into(), + raw_input: r#"{"text":"hello"}"#.into(), + input: json!({"text": "hello"}), + is_input_complete: true, + thought_signature: None, + }, + )); + fake_model_a.end_last_completion_stream(); + + // Before the next iteration runs, switch to profile-b (only DelayTool), + // swap in a new model, and enable thinking. + let fake_model_b = Arc::new(FakeLanguageModel::with_id_and_thinking( + "test-provider", + "model-b", + "Model B", + true, + )); + thread.update(cx, |thread, cx| { + thread.set_profile(AgentProfileId("profile-b".into()), cx); + thread.set_model(fake_model_b.clone() as Arc, cx); + thread.set_thinking_enabled(true, cx); + }); + + // Run until parked — processes the echo tool call, loops back, picks up + // the new model/profile/thinking, and makes a second request to model B. + cx.run_until_parked(); + + // The second request should have gone to model B. + let model_b_completions = fake_model_b.pending_completions(); + assert_eq!( + model_b_completions.len(), + 1, + "second request should go to model B" + ); + + // Profile-b only has DelayTool, so echo should be gone. + let second_tools = tool_names_for_completion(&model_b_completions[0]); + assert_eq!(second_tools, vec![DelayTool::NAME]); + + // Thinking should now be enabled. + assert!(model_b_completions[0].thinking_allowed); +} diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 55fdace2cfea1dd77be507cb06f0a9d4b6634cf7..2f6eee28b6ab7311bde461170e56704daa5b7b9f 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -12,8 +12,7 @@ use feature_flags::{FeatureFlagAppExt as _, StreamingEditFileToolFeatureFlag}; use agent_client_protocol as acp; use agent_settings::{ - AgentProfileId, AgentProfileSettings, AgentSettings, SUMMARIZE_THREAD_DETAILED_PROMPT, - SUMMARIZE_THREAD_PROMPT, + AgentProfileId, AgentSettings, SUMMARIZE_THREAD_DETAILED_PROMPT, SUMMARIZE_THREAD_PROMPT, }; use anyhow::{Context as _, Result, anyhow}; use chrono::{DateTime, Utc}; @@ -1771,11 +1770,6 @@ impl Thread { self.flush_pending_message(cx); self.cancel(cx).detach(); - let model = self.model.clone().context("No language model configured")?; - let profile = AgentSettings::get_global(cx) - .profiles - .get(&self.profile_id) - .context("Profile not found")?; let (events_tx, events_rx) = mpsc::unbounded::>(); let event_stream = ThreadEventStream(events_tx); let message_ix = self.messages.len().saturating_sub(1); @@ -1783,20 +1777,15 @@ impl Thread { let (cancellation_tx, mut cancellation_rx) = watch::channel(false); self.running_turn = Some(RunningTurn { event_stream: event_stream.clone(), - tools: self.enabled_tools(profile, &model, cx), + tools: self.enabled_tools(cx), cancellation_tx, streaming_tool_inputs: HashMap::default(), _task: cx.spawn(async move |this, cx| { log::debug!("Starting agent turn execution"); - let turn_result = Self::run_turn_internal( - &this, - model, - &event_stream, - cancellation_rx.clone(), - cx, - ) - .await; + let turn_result = + Self::run_turn_internal(&this, &event_stream, cancellation_rx.clone(), cx) + .await; // Check if we were cancelled - if so, cancel() already took running_turn // and we shouldn't touch it (it might be a NEW turn now) @@ -1838,7 +1827,6 @@ impl Thread { async fn run_turn_internal( this: &WeakEntity, - model: Arc, event_stream: &ThreadEventStream, mut cancellation_rx: watch::Receiver, cx: &mut AsyncApp, @@ -1846,8 +1834,15 @@ impl Thread { let mut attempt = 0; let mut intent = CompletionIntent::UserPrompt; loop { - let request = - this.update(cx, |this, cx| this.build_completion_request(intent, cx))??; + // Re-read the model and refresh tools on each iteration so that + // mid-turn changes (e.g. the user switches model, toggles tools, + // or changes profile) take effect between tool-call rounds. + let (model, request) = this.update(cx, |this, cx| { + let model = this.model.clone().context("No language model configured")?; + this.refresh_turn_tools(cx); + let request = this.build_completion_request(intent, cx)?; + anyhow::Ok((model, request)) + })??; telemetry::event!( "Agent Thread Completion", @@ -2700,12 +2695,13 @@ impl Thread { Ok(request) } - fn enabled_tools( - &self, - profile: &AgentProfileSettings, - model: &Arc, - cx: &App, - ) -> BTreeMap> { + fn enabled_tools(&self, cx: &App) -> BTreeMap> { + let Some(model) = self.model.as_ref() else { + return BTreeMap::new(); + }; + let Some(profile) = AgentSettings::get_global(cx).profiles.get(&self.profile_id) else { + return BTreeMap::new(); + }; fn truncate(tool_name: &SharedString) -> SharedString { if tool_name.len() > MAX_TOOL_NAME_LENGTH { let mut truncated = tool_name.to_string(); @@ -2786,6 +2782,13 @@ impl Thread { tools } + fn refresh_turn_tools(&mut self, cx: &App) { + let tools = self.enabled_tools(cx); + if let Some(turn) = self.running_turn.as_mut() { + turn.tools = tools; + } + } + fn tool(&self, name: &str) -> Option> { self.running_turn.as_ref()?.tools.get(name).cloned() } @@ -3029,7 +3032,8 @@ struct RunningTurn { /// The current event stream for the running turn. Used to report a final /// cancellation event if we cancel the turn. event_stream: ThreadEventStream, - /// The tools that were enabled for this turn. + /// The tools that are enabled for the current iteration of the turn. + /// Refreshed at the start of each iteration via `refresh_turn_tools`. tools: BTreeMap>, /// Sender to signal tool cancellation. When cancel is called, this is /// set to true so all tools can detect user-initiated cancellation. diff --git a/crates/agent_ui/src/conversation_view/thread_view.rs b/crates/agent_ui/src/conversation_view/thread_view.rs index fd76c34402a5ba71a8e5feea03dc6398e6a43b4b..4c1e1e6419cd6bd5d68d988ff69c7198b437e6bb 100644 --- a/crates/agent_ui/src/conversation_view/thread_view.rs +++ b/crates/agent_ui/src/conversation_view/thread_view.rs @@ -2707,14 +2707,6 @@ impl ThreadView { return div().into_any_element(); } - let is_generating = self.thread.read(cx).status() != ThreadStatus::Idle; - if let Some(model_selector) = &self.model_selector { - model_selector.update(cx, |selector, _| selector.set_disabled(is_generating)); - } - if let Some(profile_selector) = &self.profile_selector { - profile_selector.update(cx, |selector, _| selector.set_disabled(is_generating)); - } - let focus_handle = self.message_editor.focus_handle(cx); let editor_bg_color = cx.theme().colors().editor_background; let editor_expanded = self.editor_expanded; @@ -3264,7 +3256,6 @@ impl ThreadView { return None; } - let is_generating = self.thread.read(cx).status() != ThreadStatus::Idle; let thinking = thread.thinking_enabled(); let (tooltip_label, icon, color) = if thinking { @@ -3286,13 +3277,8 @@ impl ThreadView { let thinking_toggle = IconButton::new("thinking-mode", icon) .icon_size(IconSize::Small) .icon_color(color) - .disabled(is_generating) - .tooltip(move |window, cx| { - if is_generating { - Tooltip::text("Disabled until generation is done")(window, cx) - } else { - Tooltip::for_action_in(tooltip_label, &ToggleThinkingMode, &focus_handle, cx) - } + .tooltip(move |_, cx| { + Tooltip::for_action_in(tooltip_label, &ToggleThinkingMode, &focus_handle, cx) }) .on_click(cx.listener(move |this, _, _window, cx| { if let Some(thread) = this.as_native_thread(cx) { @@ -3324,7 +3310,6 @@ impl ThreadView { let right_btn = self.render_effort_selector( model.supported_effort_levels(), thread.thinking_effort().cloned(), - is_generating, cx, ); @@ -3339,7 +3324,6 @@ impl ThreadView { &self, supported_effort_levels: Vec, selected_effort: Option, - disabled: bool, cx: &Context, ) -> impl IntoElement { let weak_self = cx.weak_entity(); @@ -3408,7 +3392,6 @@ impl ThreadView { PopoverMenu::new("effort-selector") .trigger_with_tooltip( ButtonLike::new_rounded_right("effort-selector-trigger") - .disabled(disabled) .selected_style(ButtonStyle::Tinted(TintColor::Accent)) .child(Label::new(label).size(LabelSize::Small).color(label_color)) .child(Icon::new(icon).size(IconSize::XSmall).color(Color::Muted)), diff --git a/crates/agent_ui/src/model_selector_popover.rs b/crates/agent_ui/src/model_selector_popover.rs index 74ebd78ba61681325cc4905be8d577b225e50e92..75ef5ab8cc907c0ffbce370c846bb1a1b651e938 100644 --- a/crates/agent_ui/src/model_selector_popover.rs +++ b/crates/agent_ui/src/model_selector_popover.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use acp_thread::{AgentModelIcon, AgentModelInfo, AgentModelSelector}; use fs::Fs; -use gpui::{AnyView, Entity, FocusHandle}; +use gpui::{Entity, FocusHandle}; use picker::popover_menu::PickerPopoverMenu; use ui::{PopoverMenuHandle, Tooltip, prelude::*}; @@ -13,7 +13,6 @@ use crate::{ModelSelector, model_selector::acp_model_selector}; pub struct ModelSelectorPopover { selector: Entity, menu_handle: PopoverMenuHandle, - disabled: bool, } impl ModelSelectorPopover { @@ -31,18 +30,10 @@ impl ModelSelectorPopover { acp_model_selector(selector, agent_server, fs, focus_handle.clone(), window, cx) }), menu_handle, - disabled: false, } } - pub fn set_disabled(&mut self, disabled: bool) { - self.disabled = disabled; - } - pub fn toggle(&self, window: &mut Window, cx: &mut Context) { - if self.disabled { - return; - } self.menu_handle.toggle(window, cx); } @@ -51,9 +42,6 @@ impl ModelSelectorPopover { } pub fn cycle_favorite_models(&self, window: &mut Window, cx: &mut Context) { - if self.disabled { - return; - } self.selector.update(cx, |selector, cx| { selector.delegate.cycle_favorite_models(window, cx); }); @@ -73,33 +61,25 @@ impl Render for ModelSelectorPopover { let (color, icon) = if self.menu_handle.is_deployed() { (Color::Accent, IconName::ChevronUp) - } else if self.disabled { - (Color::Disabled, IconName::ChevronDown) } else { (Color::Muted, IconName::ChevronDown) }; let show_cycle_row = selector.delegate.favorites_count() > 1; - let disabled = self.disabled; - let tooltip: Box AnyView> = if disabled { - Box::new(Tooltip::text("Disabled until generation is done")) - } else { - Box::new(Tooltip::element({ - move |_, _cx| { - ModelSelectorTooltip::new() - .show_cycle_row(show_cycle_row) - .into_any_element() - } - })) - }; + let tooltip = Tooltip::element({ + move |_, _cx| { + ModelSelectorTooltip::new() + .show_cycle_row(show_cycle_row) + .into_any_element() + } + }); PickerPopoverMenu::new( self.selector.clone(), Button::new("active-model", model_name) .label_size(LabelSize::Small) .color(color) - .disabled(self.disabled) .when_some(model_icon, |this, icon| { this.start_icon( match icon { @@ -110,17 +90,7 @@ impl Render for ModelSelectorPopover { .size(IconSize::XSmall), ) }) - .end_icon( - Icon::new(icon) - .map(|this| { - if self.disabled { - this.color(Color::Disabled) - } else { - this.color(Color::Muted) - } - }) - .size(IconSize::XSmall), - ), + .end_icon(Icon::new(icon).color(Color::Muted).size(IconSize::XSmall)), tooltip, gpui::Corner::BottomRight, cx, diff --git a/crates/agent_ui/src/profile_selector.rs b/crates/agent_ui/src/profile_selector.rs index 661f887b53116094b5a8694bf93b21389bd9f58b..1bad3c45e4dece2397a2e026d659fd0fad043a24 100644 --- a/crates/agent_ui/src/profile_selector.rs +++ b/crates/agent_ui/src/profile_selector.rs @@ -34,7 +34,6 @@ pub trait ProfileProvider { pub struct ProfileSelector { profiles: AvailableProfiles, pending_refresh: bool, - disabled: bool, fs: Arc, provider: Arc, picker: Option>>, @@ -58,7 +57,6 @@ impl ProfileSelector { Self { profiles: AgentProfile::available_profiles(cx), pending_refresh: false, - disabled: false, fs, provider, picker: None, @@ -72,19 +70,7 @@ impl ProfileSelector { self.picker_handle.clone() } - pub fn set_disabled(&mut self, disabled: bool) { - self.disabled = disabled; - } - - pub fn is_disabled(&self) -> bool { - self.disabled - } - pub fn cycle_profile(&mut self, cx: &mut Context) { - if self.disabled { - return; - } - if !self.provider.profiles_supported(cx) { return; } @@ -189,38 +175,31 @@ impl Render for ProfileSelector { }; let trigger_button = Button::new("profile-selector", selected_profile) - .disabled(self.disabled) .label_size(LabelSize::Small) .color(Color::Muted) .end_icon(Icon::new(icon).size(IconSize::XSmall).color(Color::Muted)); - let disabled = self.disabled; - - let tooltip: Box AnyView> = if disabled { - Box::new(Tooltip::text("Disabled until generation is done")) - } else { - Box::new(Tooltip::element({ - move |_window, cx| { - let container = || h_flex().gap_1().justify_between(); - v_flex() - .gap_1() - .child( - container() - .child(Label::new("Change Profile")) - .child(KeyBinding::for_action(&ToggleProfileSelector, cx)), - ) - .child( - container() - .pt_1() - .border_t_1() - .border_color(cx.theme().colors().border_variant) - .child(Label::new("Cycle Through Profiles")) - .child(KeyBinding::for_action(&CycleModeSelector, cx)), - ) - .into_any() - } - })) - }; + let tooltip: Box AnyView> = Box::new(Tooltip::element({ + move |_window, cx| { + let container = || h_flex().gap_1().justify_between(); + v_flex() + .gap_1() + .child( + container() + .child(Label::new("Change Profile")) + .child(KeyBinding::for_action(&ToggleProfileSelector, cx)), + ) + .child( + container() + .pt_1() + .border_t_1() + .border_color(cx.theme().colors().border_variant) + .child(Label::new("Cycle Through Profiles")) + .child(KeyBinding::for_action(&CycleModeSelector, cx)), + ) + .into_any() + } + })); PickerPopoverMenu::new( picker,