diff --git a/crates/acp_thread/src/connection.rs b/crates/acp_thread/src/connection.rs index e5bb82e9afd..71c46d3e622 100644 --- a/crates/acp_thread/src/connection.rs +++ b/crates/acp_thread/src/connection.rs @@ -310,7 +310,7 @@ pub trait AgentSessionConfigOptions { fn set_config_option( &self, config_id: acp::SessionConfigId, - value: acp::SessionConfigValueId, + value: acp::SessionConfigOptionValue, cx: &mut App, ) -> Task>>; diff --git a/crates/agent_servers/src/acp.rs b/crates/agent_servers/src/acp.rs index ddec4877c7d..708d1c71687 100644 --- a/crates/agent_servers/src/acp.rs +++ b/crates/agent_servers/src/acp.rs @@ -22,7 +22,7 @@ use project::agent_server_store::{ use project::{AgentId, Project}; use remote::remote_client::Interactive; use serde::Deserialize; -use settings::SettingsStore; +use settings::{AgentConfigOptionValue, SettingsStore}; use std::path::PathBuf; use std::process::{ExitStatus, Stdio}; use std::rc::Rc; @@ -408,11 +408,14 @@ pub struct AcpConnection { #[derive(Clone, Default)] struct AcpConnectionDefaults { mode: Rc>>, - config_options: Rc>>, + config_options: Rc>>, } impl AcpConnectionDefaults { - fn new(mode: Option, config_options: HashMap) -> Self { + fn new( + mode: Option, + config_options: HashMap, + ) -> Self { Self { mode: Rc::new(RefCell::new(mode)), config_options: Rc::new(RefCell::new(config_options)), @@ -423,11 +426,15 @@ impl AcpConnectionDefaults { self.mode.borrow().clone() } - fn config_option(&self, config_id: &str) -> Option { + fn config_option(&self, config_id: &str) -> Option { self.config_options.borrow().get(config_id).cloned() } - fn set(&self, mode: Option, config_options: HashMap) { + fn set( + &self, + mode: Option, + config_options: HashMap, + ) { *self.mode.borrow_mut() = mode; *self.config_options.borrow_mut() = config_options; } @@ -627,7 +634,7 @@ pub async fn connect( command: AgentServerCommand, agent_server_store: WeakEntity, default_mode: Option, - default_config_options: HashMap, + default_config_options: HashMap, cx: &mut AsyncApp, ) -> Result> { let conn = AcpConnection::stdio( @@ -736,7 +743,10 @@ fn connect_client_future( ) } -fn client_capabilities_for_agent(agent_id: &AgentId) -> acp::ClientCapabilities { +fn client_capabilities_for_agent( + agent_id: &AgentId, + supports_boolean_config_options: bool, +) -> acp::ClientCapabilities { let mut meta = acp::Meta::from_iter([ ("terminal_output".into(), true.into()), ("terminal-auth".into(), true.into()), @@ -746,13 +756,24 @@ fn client_capabilities_for_agent(agent_id: &AgentId) -> acp::ClientCapabilities meta.insert(PARAMETERIZED_MODEL_PICKER_META_KEY.into(), true.into()); } - acp::ClientCapabilities::new() + let mut capabilities = acp::ClientCapabilities::new() .fs(acp::FileSystemCapabilities::new() .read_text_file(true) .write_text_file(true)) .terminal(true) .auth(acp::AuthCapabilities::new().terminal(true)) - .meta(meta) + .meta(meta); + + if supports_boolean_config_options { + capabilities = capabilities.session( + acp::ClientSessionCapabilities::new().config_options( + acp::SessionConfigOptionsCapabilities::new() + .boolean(acp::BooleanConfigOptionCapabilities::new()), + ), + ); + } + + capabilities } impl AcpConnection { @@ -771,7 +792,7 @@ impl AcpConnection { command: AgentServerCommand, agent_server_store: WeakEntity, default_mode: Option, - default_config_options: HashMap, + default_config_options: HashMap, cx: &mut AsyncApp, ) -> Result { let root_dir = project.read_with(cx, |project, cx| { @@ -942,7 +963,10 @@ impl AcpConnection { let initialize_response = connection .send_request( acp::InitializeRequest::new(ProtocolVersion::V1) - .client_capabilities(client_capabilities_for_agent(&agent_id)) + .client_capabilities(client_capabilities_for_agent( + &agent_id, + cx.update(|cx| cx.has_flag::()), + )) .client_info( acp::Implementation::new("zed", version) .title(release_channel.map(ToOwned::to_owned)), @@ -1255,6 +1279,7 @@ impl AcpConnection { cx: &mut AsyncApp, ) { let id = self.id.clone(); + let apply_boolean_defaults = cx.update(|cx| cx.has_flag::()); let defaults_to_apply: Vec<_> = { let config_opts_ref = config_options.borrow(); config_opts_ref @@ -1262,31 +1287,54 @@ impl AcpConnection { .filter_map(|config_option| { let default_value = self.defaults.config_option(config_option.id.0.as_ref())?; - let is_valid = match &config_option.kind { - acp::SessionConfigKind::Select(select) => match &select.options { - acp::SessionConfigSelectOptions::Ungrouped(options) => options - .iter() - .any(|opt| &*opt.value.0 == default_value.as_str()), - acp::SessionConfigSelectOptions::Grouped(groups) => { - groups.iter().any(|g| { - g.options - .iter() - .any(|opt| &*opt.value.0 == default_value.as_str()) - }) + let value_to_apply = match &config_option.kind { + acp::SessionConfigKind::Select(select) => { + let value_id = default_value.as_value_id()?; + match &select.options { + acp::SessionConfigSelectOptions::Ungrouped(options) => options + .iter() + .any(|opt| &*opt.value.0 == value_id) + .then(|| { + acp::SessionConfigOptionValue::value_id( + value_id.to_string(), + ) + }), + acp::SessionConfigSelectOptions::Grouped(groups) => groups + .iter() + .any(|group| { + group.options.iter().any(|opt| &*opt.value.0 == value_id) + }) + .then(|| { + acp::SessionConfigOptionValue::value_id( + value_id.to_string(), + ) + }), + _ => None, } - _ => false, - }, - _ => false, + } + acp::SessionConfigKind::Boolean(_) if !apply_boolean_defaults => { + return None; + } + acp::SessionConfigKind::Boolean(_) => default_value + .as_bool() + .map(acp::SessionConfigOptionValue::boolean), + _ => None, }; - if is_valid { + if let Some(value_to_apply) = value_to_apply { let initial_value = match &config_option.kind { acp::SessionConfigKind::Select(select) => { - Some(select.current_value.clone()) + acp::SessionConfigOptionValue::value_id( + select.current_value.clone(), + ) } - _ => None, + acp::SessionConfigKind::Boolean(boolean) => { + acp::SessionConfigOptionValue::boolean(boolean.current_value) + } + _ => return None, }; - Some((config_option.id.clone(), default_value, initial_value)) + + Some((config_option.id.clone(), value_to_apply, initial_value)) } else { log::warn!( "`{}` is not a valid value for config option `{}` in {}", @@ -1302,7 +1350,7 @@ impl AcpConnection { for (config_id, default_value, initial_value) in defaults_to_apply { cx.spawn({ - let default_value_id = acp::SessionConfigValueId::new(default_value.clone()); + let default_value_for_request = default_value.clone(); let session_id = session_id.clone(); let config_id_clone = config_id.clone(); let config_opts = config_options.clone(); @@ -1312,19 +1360,29 @@ impl AcpConnection { .send_request(acp::SetSessionConfigOptionRequest::new( session_id, config_id_clone.clone(), - default_value_id, + default_value_for_request, )) .block_task() .await .log_err(); if result.is_none() { - if let Some(initial) = initial_value { - let mut opts = config_opts.borrow_mut(); - if let Some(opt) = opts.iter_mut().find(|o| o.id == config_id_clone) { - if let acp::SessionConfigKind::Select(select) = &mut opt.kind { - select.current_value = initial; + let mut opts = config_opts.borrow_mut(); + if let Some(opt) = opts.iter_mut().find(|o| o.id == config_id_clone) { + match (&mut opt.kind, &initial_value) { + ( + acp::SessionConfigKind::Select(select), + acp::SessionConfigOptionValue::ValueId { value }, + ) => { + select.current_value = value.clone(); } + ( + acp::SessionConfigKind::Boolean(boolean), + acp::SessionConfigOptionValue::Boolean { value }, + ) => { + boolean.current_value = *value; + } + _ => {} } } } @@ -1334,8 +1392,20 @@ impl AcpConnection { let mut opts = config_options.borrow_mut(); if let Some(opt) = opts.iter_mut().find(|o| o.id == config_id) { - if let acp::SessionConfigKind::Select(select) = &mut opt.kind { - select.current_value = acp::SessionConfigValueId::new(default_value); + match (&mut opt.kind, &default_value) { + ( + acp::SessionConfigKind::Select(select), + acp::SessionConfigOptionValue::ValueId { value }, + ) => { + select.current_value = value.clone(); + } + ( + acp::SessionConfigKind::Boolean(boolean), + acp::SessionConfigOptionValue::Boolean { value }, + ) => { + boolean.current_value = *value; + } + _ => {} } } } @@ -2469,11 +2539,12 @@ mod tests { use std::sync::atomic::{AtomicUsize, Ordering}; use super::*; + use feature_flags::FeatureFlag as _; use settings::Settings as _; #[test] fn cursor_client_capabilities_include_parameterized_model_picker_meta() { - let capabilities = client_capabilities_for_agent(&AgentId::new(CURSOR_ID)); + let capabilities = client_capabilities_for_agent(&AgentId::new(CURSOR_ID), false); let meta = capabilities .meta .expect("expected client capabilities meta"); @@ -2488,7 +2559,7 @@ mod tests { #[test] fn non_cursor_client_capabilities_do_not_include_parameterized_model_picker_meta() { - let capabilities = client_capabilities_for_agent(&AgentId::new("codex-acp")); + let capabilities = client_capabilities_for_agent(&AgentId::new("codex-acp"), false); let meta = capabilities .meta .expect("expected client capabilities meta"); @@ -2496,6 +2567,26 @@ mod tests { assert!(!meta.contains_key(PARAMETERIZED_MODEL_PICKER_META_KEY)); } + #[test] + fn client_capabilities_include_boolean_config_options_when_supported() { + let capabilities = client_capabilities_for_agent(&AgentId::new("codex-acp"), true); + + assert!( + capabilities + .session + .and_then(|session| session.config_options) + .and_then(|config_options| config_options.boolean) + .is_some() + ); + } + + #[test] + fn client_capabilities_omit_boolean_config_options_when_unsupported() { + let capabilities = client_capabilities_for_agent(&AgentId::new("codex-acp"), false); + + assert!(capabilities.session.is_none()); + } + #[test] fn terminal_auth_task_builds_spawn_from_prebuilt_command() { let command = AgentServerCommand { @@ -2943,7 +3034,7 @@ mod tests { default_mode: Some("manual".to_string()), default_config_options: HashMap::from_iter([( "mode".to_string(), - "manual".to_string(), + AgentConfigOptionValue::from("manual"), )]), favorite_config_option_values: HashMap::default(), } @@ -2959,8 +3050,13 @@ mod tests { Some(acp::SessionModeId::new("manual")) ); assert_eq!( - harness.connection.defaults.config_option("mode").as_deref(), - Some("manual") + harness + .connection + .defaults + .config_option("mode") + .as_ref() + .and_then(AgentConfigOptionValue::as_value_id), + Some("manual"), ); cx.update(|cx| { @@ -2975,6 +3071,192 @@ mod tests { assert_eq!(harness.connection.defaults.config_option("mode"), None); } + #[gpui::test] + async fn default_config_options_skip_boolean_defaults_when_acp_beta_is_disabled( + cx: &mut gpui::TestAppContext, + ) { + cx.update(|cx| init_settings_with_acp_beta_override(false, cx)); + + let (connection, set_config_requests) = connect_config_defaults_test_agent(cx).await; + connection.defaults.set( + None, + HashMap::from_iter([ + ( + "web_search".to_string(), + AgentConfigOptionValue::Boolean(true), + ), + ("mode".to_string(), AgentConfigOptionValue::from("manual")), + ]), + ); + let config_options = Rc::new(RefCell::new(vec![ + acp::SessionConfigOption::boolean("web_search", "Web Search", false), + acp::SessionConfigOption::select( + "mode", + "Mode", + "auto", + vec![ + acp::SessionConfigSelectOption::new("auto", "Auto"), + acp::SessionConfigSelectOption::new("manual", "Manual"), + ], + ), + ])); + + let mut async_cx = cx.to_async(); + connection.apply_default_config_options( + &acp::SessionId::new("session-config-defaults"), + &config_options, + &mut async_cx, + ); + drop(async_cx); + cx.run_until_parked(); + + let requests = set_config_requests + .lock() + .expect("set config requests mutex poisoned"); + assert_eq!(requests.len(), 1); + assert_eq!(requests[0].config_id, acp::SessionConfigId::new("mode")); + assert_eq!( + requests[0].value, + acp::SessionConfigOptionValue::value_id("manual") + ); + + let options = config_options.borrow(); + assert!( + matches!(&options[0].kind, acp::SessionConfigKind::Boolean(boolean) if !boolean.current_value) + ); + assert!( + matches!(&options[1].kind, acp::SessionConfigKind::Select(select) if select.current_value == acp::SessionConfigValueId::new("manual")) + ); + } + + #[gpui::test] + async fn default_config_options_apply_boolean_defaults_when_acp_beta_is_enabled( + cx: &mut gpui::TestAppContext, + ) { + cx.update(|cx| init_settings_with_acp_beta_override(true, cx)); + + let (connection, set_config_requests) = connect_config_defaults_test_agent(cx).await; + connection.defaults.set( + None, + HashMap::from_iter([( + "web_search".to_string(), + AgentConfigOptionValue::Boolean(true), + )]), + ); + let config_options = Rc::new(RefCell::new(vec![acp::SessionConfigOption::boolean( + "web_search", + "Web Search", + false, + )])); + + let mut async_cx = cx.to_async(); + connection.apply_default_config_options( + &acp::SessionId::new("session-config-defaults"), + &config_options, + &mut async_cx, + ); + drop(async_cx); + cx.run_until_parked(); + + let requests = set_config_requests + .lock() + .expect("set config requests mutex poisoned"); + assert_eq!(requests.len(), 1); + assert_eq!( + requests[0].config_id, + acp::SessionConfigId::new("web_search") + ); + assert_eq!( + requests[0].value, + acp::SessionConfigOptionValue::boolean(true) + ); + + let options = config_options.borrow(); + assert!( + matches!(&options[0].kind, acp::SessionConfigKind::Boolean(boolean) if boolean.current_value) + ); + } + + fn init_settings_with_acp_beta_override(enabled: bool, cx: &mut App) { + let mut store = settings::SettingsStore::test(cx); + store.register_setting::(); + store.update_user_settings(cx, |content| { + content.feature_flags.get_or_insert_default().insert( + AcpBetaFeatureFlag::NAME.to_string(), + if enabled { "on" } else { "off" }.to_string(), + ); + }); + cx.set_global(store); + cx.update_flags(false, Vec::new()); + } + + async fn connect_config_defaults_test_agent( + cx: &mut gpui::TestAppContext, + ) -> ( + AcpConnection, + Arc>>, + ) { + let set_config_requests = Arc::new(Mutex::new(Vec::new())); + let (client_transport, agent_transport) = agent_client_protocol::Channel::duplex(); + + cx.background_spawn( + Agent + .builder() + .name("config-defaults-test-agent") + .on_receive_request( + { + let set_config_requests = set_config_requests.clone(); + async move |req: acp::SetSessionConfigOptionRequest, responder, _cx| { + set_config_requests + .lock() + .expect("set config requests mutex poisoned") + .push(req); + + responder.respond(acp::SetSessionConfigOptionResponse::new(Vec::new())) + } + }, + agent_client_protocol::on_receive_request!(), + ) + .connect_to(agent_transport), + ) + .detach(); + + let (connection_tx, connection_rx) = futures::channel::oneshot::channel(); + let client_io_task = cx.background_spawn(async move { + Client + .builder() + .name("config-defaults-test-client") + .connect_with( + client_transport, + move |connection: ConnectionTo| async move { + connection_tx.send(connection).ok(); + futures::future::pending::>().await + }, + ) + .await + .ok(); + }); + + let client_conn = connection_rx + .await + .expect("failed to receive ACP connection"); + let sessions = Rc::new(RefCell::new(HashMap::default())); + + let connection = cx.update(|cx| { + AcpConnection::new_for_test( + client_conn, + sessions, + acp::AgentCapabilities::default(), + WeakEntity::new_invalid(), + client_io_task, + Task::ready(()), + cx, + ) + }); + + (connection, set_config_requests) + } + #[gpui::test] async fn session_list_delete_sends_session_delete_when_supported( cx: &mut gpui::TestAppContext, @@ -3766,7 +4048,7 @@ impl acp_thread::AgentSessionConfigOptions for AcpSessionConfigOptions { fn set_config_option( &self, config_id: acp::SessionConfigId, - value: acp::SessionConfigValueId, + value: acp::SessionConfigOptionValue, cx: &mut App, ) -> Task>> { let connection = self.connection.clone(); diff --git a/crates/agent_servers/src/agent_servers.rs b/crates/agent_servers/src/agent_servers.rs index 9d65a2a8e29..5983ee88d41 100644 --- a/crates/agent_servers/src/agent_servers.rs +++ b/crates/agent_servers/src/agent_servers.rs @@ -15,7 +15,7 @@ use acp_thread::AgentConnection; use agent_client_protocol::schema::v1 as acp_schema; use anyhow::Result; use gpui::{App, AppContext, Entity, Task}; -use settings::SettingsStore; +use settings::{AgentConfigOptionValue, SettingsStore}; use std::{any::Any, rc::Rc, sync::Arc}; #[cfg(any(test, feature = "test-support"))] @@ -71,14 +71,14 @@ pub trait AgentServer: Send { ) { } - fn default_config_option(&self, _config_id: &str, _cx: &App) -> Option { + fn default_config_option(&self, _config_id: &str, _cx: &App) -> Option { None } fn set_default_config_option( &self, _config_id: &str, - _value_id: Option<&str>, + _value: Option, _fs: Arc, _cx: &mut App, ) { diff --git a/crates/agent_servers/src/custom.rs b/crates/agent_servers/src/custom.rs index 6039d796764..c79ebdc45c0 100644 --- a/crates/agent_servers/src/custom.rs +++ b/crates/agent_servers/src/custom.rs @@ -10,7 +10,7 @@ use project::{ Project, agent_server_store::{AgentId, AllAgentServersSettings}, }; -use settings::{SettingsStore, update_settings_file}; +use settings::{AgentConfigOptionValue, SettingsStore, update_settings_file}; use std::{rc::Rc, sync::Arc}; use ui::IconName; @@ -142,7 +142,7 @@ impl AgentServer for CustomAgentServer { }); } - fn default_config_option(&self, config_id: &str, cx: &App) -> Option { + fn default_config_option(&self, config_id: &str, cx: &App) -> Option { let settings = cx.read_global(|settings: &SettingsStore, _| { settings .get::(None) @@ -152,19 +152,18 @@ impl AgentServer for CustomAgentServer { settings .as_ref() - .and_then(|s| s.default_config_option(config_id).map(|s| s.to_string())) + .and_then(|s| s.default_config_option(config_id).cloned()) } fn set_default_config_option( &self, config_id: &str, - value_id: Option<&str>, + value: Option, fs: Arc, cx: &mut App, ) { let agent_id = self.agent_id(); let config_id = config_id.to_string(); - let value_id = value_id.map(|s| s.to_string()); update_settings_file(fs, cx, move |settings, _cx| { let settings = settings .agent_servers @@ -181,7 +180,7 @@ impl AgentServer for CustomAgentServer { default_config_options, .. } => { - if let Some(value) = value_id.clone() { + if let Some(value) = value { default_config_options.insert(config_id.clone(), value); } else { default_config_options.remove(&config_id); diff --git a/crates/agent_ui/src/config_options.rs b/crates/agent_ui/src/config_options.rs index 405b6c64360..4dcd72cbdf9 100644 --- a/crates/agent_ui/src/config_options.rs +++ b/crates/agent_ui/src/config_options.rs @@ -5,6 +5,7 @@ use agent_client_protocol::schema::v1 as acp; use agent_servers::AgentServer; use collections::HashSet; +use feature_flags::{AcpBetaFeatureFlag, FeatureFlagAppExt as _}; use fs::Fs; use fuzzy::StringMatchCandidate; use gpui::{ @@ -13,10 +14,10 @@ use gpui::{ use ordered_float::OrderedFloat; use picker::popover_menu::PickerPopoverMenu; use picker::{Picker, PickerDelegate}; -use settings::SettingsStore; +use settings::{AgentConfigOptionValue, SettingsStore}; use ui::{ - ElevationIndex, IconButton, KeyBinding, ListItem, ListItemSpacing, PopoverMenuHandle, Tooltip, - prelude::*, + ElevationIndex, IconButton, KeyBinding, ListItem, ListItemSpacing, PopoverMenuHandle, Switch, + SwitchLabelPosition, ToggleState, Tooltip, prelude::*, }; use unicode_segmentation::UnicodeSegmentation; use util::ResultExt as _; @@ -79,7 +80,9 @@ impl ConfigOptionsView { window: &mut Window, cx: &mut Context, ) -> bool { - let Some(config_id) = self.first_config_option_id(category) else { + let Some(config_id) = self.first_config_option_id_matching(category, |option| { + matches!(&option.kind, acp::SessionConfigKind::Select(_)) + }) else { return false; }; @@ -87,11 +90,7 @@ impl ConfigOptionsView { return false; }; - selector.update(cx, |selector, cx| { - selector.toggle_picker(window, cx); - }); - - true + selector.update(cx, |selector, cx| selector.toggle_picker(window, cx)) } pub fn cycle_category_option( @@ -100,17 +99,21 @@ impl ConfigOptionsView { favorites_only: bool, cx: &mut Context, ) -> bool { - let Some(config_id) = self.first_config_option_id(category) else { + let render_boolean_config_options = should_render_boolean_config_options(cx); + let Some(config_id) = self.first_config_option_id_matching(category, |option| { + Self::can_cycle_config_option(option, favorites_only, render_boolean_config_options) + }) else { return false; }; let Some(next_value) = self.next_value_for_config(&config_id, favorites_only, cx) else { return false; }; + let default_value = setting_value_for_config_option_value(&next_value); self.agent_server.set_default_config_option( config_id.0.as_ref(), - Some(next_value.0.as_ref()), + default_value, self.fs.clone(), cx, ); @@ -129,17 +132,30 @@ impl ConfigOptionsView { true } - fn first_config_option_id( + fn first_config_option_id_matching( &self, category: acp::SessionConfigOptionCategory, + predicate: impl Fn(&acp::SessionConfigOption) -> bool, ) -> Option { self.config_options .config_options() .into_iter() - .find(|option| option.category.as_ref() == Some(&category)) + .find(|option| option.category.as_ref() == Some(&category) && predicate(option)) .map(|option| option.id) } + fn can_cycle_config_option( + option: &acp::SessionConfigOption, + favorites_only: bool, + render_boolean_config_options: bool, + ) -> bool { + match &option.kind { + acp::SessionConfigKind::Select(_) => true, + acp::SessionConfigKind::Boolean(_) => !favorites_only && render_boolean_config_options, + _ => false, + } + } + fn selector_for_config_id( &self, config_id: &acp::SessionConfigId, @@ -156,35 +172,57 @@ impl ConfigOptionsView { config_id: &acp::SessionConfigId, favorites_only: bool, cx: &mut Context, - ) -> Option { - let mut options = extract_options(&self.config_options, config_id); - if options.is_empty() { - return None; - } + ) -> Option { + let option = self + .config_options + .config_options() + .into_iter() + .find(|option| &option.id == config_id)?; - if favorites_only { - let favorites = self - .agent_server - .favorite_config_option_value_ids(config_id, cx); - options.retain(|option| favorites.contains(&option.value)); - if options.is_empty() { - return None; + match &option.kind { + acp::SessionConfigKind::Select(_) => { + let mut options = extract_options(&self.config_options, config_id); + if options.is_empty() { + return None; + } + + if favorites_only { + let favorites = self + .agent_server + .favorite_config_option_value_ids(config_id, cx); + options.retain(|option| favorites.contains(&option.value)); + if options.is_empty() { + return None; + } + } + + let current_value = get_current_select_value(&self.config_options, config_id); + let current_index = current_value + .as_ref() + .and_then(|current| options.iter().position(|option| &option.value == current)) + .unwrap_or(usize::MAX); + + let next_index = if current_index == usize::MAX { + 0 + } else { + (current_index + 1) % options.len() + }; + + Some(acp::SessionConfigOptionValue::value_id( + options[next_index].value.clone(), + )) } + acp::SessionConfigKind::Boolean(boolean) => { + if favorites_only || !should_render_boolean_config_options(cx) { + None + } else { + Some(acp::SessionConfigOptionValue::boolean( + !boolean.current_value, + )) + } + } + _ => None, } - - let current_value = get_current_value(&self.config_options, config_id); - let current_index = current_value - .as_ref() - .and_then(|current| options.iter().position(|option| &option.value == current)) - .unwrap_or(usize::MAX); - - let next_index = if current_index == usize::MAX { - 0 - } else { - (current_index + 1) % options.len() - }; - - Some(options[next_index].value.clone()) } fn config_option_ids( @@ -256,8 +294,10 @@ impl Render for ConfigOptionsView { struct ConfigOptionSelector { config_options: Rc, config_id: acp::SessionConfigId, - picker_handle: PopoverMenuHandle>, - picker: Entity>, + agent_server: Rc, + fs: Arc, + picker_handle: Option>>, + picker: Option>>, setting_value: bool, } @@ -270,21 +310,26 @@ impl ConfigOptionSelector { window: &mut Window, cx: &mut Context, ) -> Self { - let option_count = config_options + let current_option = config_options .config_options() - .iter() - .find(|opt| opt.id == config_id) + .into_iter() + .find(|opt| opt.id == config_id); + let option_count = current_option + .as_ref() .map(count_config_options) .unwrap_or(0); + let is_select = current_option + .as_ref() + .is_some_and(|option| matches!(&option.kind, acp::SessionConfigKind::Select(_))); let is_searchable = option_count >= PICKER_THRESHOLD; - let picker = { + let (picker_handle, picker) = if is_select { let config_options = config_options.clone(); let config_id = config_id.clone(); let agent_server = agent_server.clone(); let fs = fs.clone(); - cx.new(move |picker_cx| { + let picker = cx.new(move |picker_cx| { let delegate = ConfigOptionPickerDelegate::new( config_options, config_id, @@ -301,13 +346,18 @@ impl ConfigOptionSelector { } .show_scrollbar(true) .initial_width(rems(20.)) - }) + }); + (Some(PopoverMenuHandle::default()), Some(picker)) + } else { + (None, None) }; Self { config_options, config_id, - picker_handle: PopoverMenuHandle::default(), + agent_server, + fs, + picker_handle, picker, setting_value: false, } @@ -324,8 +374,13 @@ impl ConfigOptionSelector { &self.config_id } - fn toggle_picker(&self, window: &mut Window, cx: &mut Context) { - self.picker_handle.toggle(window, cx); + fn toggle_picker(&self, window: &mut Window, cx: &mut Context) -> bool { + if let Some(picker_handle) = &self.picker_handle { + picker_handle.toggle(window, cx); + true + } else { + false + } } fn current_value_name(&self) -> String { @@ -346,7 +401,10 @@ impl ConfigOptionSelector { self.config_options .config_options() .into_iter() - .find(|option| option.category.as_ref() == Some(category)) + .find(|option| { + option.category.as_ref() == Some(category) + && matches!(&option.kind, acp::SessionConfigKind::Select(_)) + }) .is_some_and(|option| option.id == self.config_id) } @@ -358,7 +416,11 @@ impl ConfigOptionSelector { .disabled(true); }; - let icon = if self.picker_handle.is_deployed() { + let picker_deployed = self + .picker_handle + .as_ref() + .is_some_and(|picker_handle| picker_handle.is_deployed()); + let icon = if picker_deployed { IconName::ChevronUp } else { IconName::ChevronDown @@ -390,88 +452,173 @@ impl Render for ConfigOptionSelector { return div().into_any_element(); }; - let trigger_button = self.render_trigger_button(window, cx); + match &option.kind { + acp::SessionConfigKind::Select(_) => { + let (Some(picker), Some(picker_handle)) = + (self.picker.clone(), self.picker_handle.clone()) + else { + return div().into_any_element(); + }; - let show_category_keybindings = option - .category - .as_ref() - .is_some_and(|category| self.handles_category_keybindings(category)); - let option_category = option.category.clone(); - let option_name = option.name.clone(); - let option_description: Option = option.description.map(Into::into); + let trigger_button = self.render_trigger_button(window, cx); - let tooltip = Tooltip::element(move |_window, cx| { - let mut content = v_flex().gap_1().child(Label::new(option_name.clone())); - if let Some(desc) = option_description.as_ref() { - content = content.child( - Label::new(desc.clone()) - .size(LabelSize::Small) - .color(Color::Muted), - ); + let show_category_keybindings = option + .category + .as_ref() + .is_some_and(|category| self.handles_category_keybindings(category)); + let option_category = option.category.clone(); + let option_name = option.name.clone(); + let option_description: Option = + option.description.clone().map(Into::into); + + let tooltip = Tooltip::element(move |_window, cx| { + let mut content = v_flex().gap_1().child(Label::new(option_name.clone())); + if let Some(desc) = option_description.as_ref() { + content = content.child( + Label::new(desc.clone()) + .size(LabelSize::Small) + .color(Color::Muted), + ); + } + + let action_tooltip_container = |label: &str, keybinding: KeyBinding| { + h_flex() + .pt_1() + .gap_2() + .justify_between() + .border_t_1() + .border_color(cx.theme().colors().border_variant) + .child(Label::new(label)) + .child(keybinding) + }; + + if show_category_keybindings && let Some(category) = &option_category { + match category { + acp::SessionConfigOptionCategory::Mode => { + content = content + .child(action_tooltip_container( + "Change Mode", + KeyBinding::for_action(&ToggleProfileSelector, cx), + )) + .child(action_tooltip_container( + "Cycle Through Modes", + KeyBinding::for_action(&CycleModeSelector, cx), + )); + } + acp::SessionConfigOptionCategory::Model => { + content = content + .child(action_tooltip_container( + "Change Model", + KeyBinding::for_action(&ToggleModelSelector, cx), + )) + .child(action_tooltip_container( + "Cycle Favorite Models", + KeyBinding::for_action(&CycleFavoriteModels, cx), + )); + } + acp::SessionConfigOptionCategory::ThoughtLevel => { + content = content + .child(action_tooltip_container( + "Change Thinking Effort", + KeyBinding::for_action(&ToggleThinkingEffortMenu, cx), + )) + .child(action_tooltip_container( + "Cycle Thinking Effort", + KeyBinding::for_action(&CycleThinkingEffort, cx), + )); + } + _ => {} + } + } + content.into_any() + }); + + PickerPopoverMenu::new( + picker, + trigger_button, + tooltip, + gpui::Anchor::BottomRight, + cx, + ) + .with_handle(picker_handle) + .render(window, cx) + .into_any_element() } - - let action_tooltip_container = |label: &str, keybinding: KeyBinding| { - h_flex() - .pt_1() - .gap_2() - .justify_between() - .border_t_1() - .border_color(cx.theme().colors().border_variant) - .child(Label::new(label)) - .child(keybinding) - }; - - if show_category_keybindings && let Some(category) = &option_category { - match category { - acp::SessionConfigOptionCategory::Mode => { - content = content - .child(action_tooltip_container( - "Change Mode", - KeyBinding::for_action(&ToggleProfileSelector, cx), - )) - .child(action_tooltip_container( - "Cycle Through Modes", - KeyBinding::for_action(&CycleModeSelector, cx), - )); - } - acp::SessionConfigOptionCategory::Model => { - content = content - .child(action_tooltip_container( - "Change Model", - KeyBinding::for_action(&ToggleModelSelector, cx), - )) - .child(action_tooltip_container( - "Cycle Favorite Models", - KeyBinding::for_action(&CycleFavoriteModels, cx), - )); - } - acp::SessionConfigOptionCategory::ThoughtLevel => { - content = content - .child(action_tooltip_container( - "Change Thinking Effort", - KeyBinding::for_action(&ToggleThinkingEffortMenu, cx), - )) - .child(action_tooltip_container( - "Cycle Thinking Effort", - KeyBinding::for_action(&CycleThinkingEffort, cx), - )); - } - _ => {} + acp::SessionConfigKind::Boolean(boolean) => { + if !should_render_boolean_config_options(cx) { + return div().into_any_element(); } - } - content.into_any() - }); - PickerPopoverMenu::new( - self.picker.clone(), - trigger_button, - tooltip, - gpui::Anchor::BottomRight, - cx, - ) - .with_handle(self.picker_handle.clone()) - .render(window, cx) - .into_any_element() + let option_id = option.id.clone(); + let option_name: SharedString = option.name.clone().into(); + let option_description: Option = + option.description.clone().map(Into::into); + let tooltip_name = option_name.clone(); + let tooltip = Tooltip::element(move |_window, _cx| { + let mut content = v_flex().gap_1().child(Label::new(tooltip_name.clone())); + if let Some(desc) = option_description.as_ref() { + content = content.child( + Label::new(desc.clone()) + .size(LabelSize::Small) + .color(Color::Muted), + ); + } + content.into_any() + }); + + let config_id = self.config_id.clone(); + let config_options = self.config_options.clone(); + let agent_server = self.agent_server.clone(); + let fs = self.fs.clone(); + let current_value = boolean.current_value; + let toggle_state = if current_value { + ToggleState::Selected + } else { + ToggleState::Unselected + }; + + h_flex() + .id(ElementId::Name( + format!("config-option-{}", option_id.0).into(), + )) + .pr_1() + .tooltip(tooltip) + .child( + Switch::new( + ElementId::Name(format!("config-option-{}-switch", option_id.0).into()), + toggle_state, + ) + .label(option_name) + .label_position(SwitchLabelPosition::Start) + .label_size(LabelSize::Small) + .disabled(self.setting_value) + .on_click(move |state, _window, cx| { + let next_value = matches!(state, ToggleState::Selected); + agent_server.set_default_config_option( + config_id.0.as_ref(), + Some(AgentConfigOptionValue::Boolean(next_value)), + fs.clone(), + cx, + ); + + let task = config_options.set_config_option( + config_id.clone(), + acp::SessionConfigOptionValue::boolean(next_value), + cx, + ); + + cx.spawn(async move |_| { + if let Err(err) = task.await { + log::error!("Failed to set config option: {:?}", err); + } + }) + .detach(); + }), + ) + .into_any_element() + } + _ => div().into_any_element(), + } } } @@ -516,7 +663,7 @@ impl ConfigOptionPickerDelegate { let all_options = extract_options(&config_options, &config_id); let filtered_entries = options_to_picker_entries(&all_options, &favorites); - let current_value = get_current_value(&config_options, &config_id); + let current_value = get_current_select_value(&config_options, &config_id); let selected_index = current_value .and_then(|current| { filtered_entries.iter().position(|entry| { @@ -554,7 +701,7 @@ impl ConfigOptionPickerDelegate { } fn current_value(&self) -> Option { - get_current_value(&self.config_options, &self.config_id) + get_current_select_value(&self.config_options, &self.config_id) } } @@ -639,13 +786,13 @@ impl PickerDelegate for ConfigOptionPickerDelegate { { self.agent_server.set_default_config_option( self.config_id.0.as_ref(), - Some(option.value.0.as_ref()), + Some(AgentConfigOptionValue::ValueId(option.value.0.to_string())), self.fs.clone(), cx, ); let task = self.config_options.set_config_option( self.config_id.clone(), - option.value.clone(), + acp::SessionConfigOptionValue::value_id(option.value.clone()), cx, ); @@ -816,7 +963,7 @@ fn extract_options( } } -fn get_current_value( +fn get_current_select_value( config_options: &Rc, config_id: &acp::SessionConfigId, ) -> Option { @@ -830,6 +977,24 @@ fn get_current_value( }) } +fn setting_value_for_config_option_value( + value: &acp::SessionConfigOptionValue, +) -> Option { + match value { + acp::SessionConfigOptionValue::ValueId { value } => { + Some(AgentConfigOptionValue::ValueId(value.0.to_string())) + } + acp::SessionConfigOptionValue::Boolean { value } => { + Some(AgentConfigOptionValue::Boolean(*value)) + } + _ => None, + } +} + +fn should_render_boolean_config_options(cx: &App) -> bool { + cx.has_flag::() +} + fn options_to_picker_entries( options: &[ConfigOptionValue], favorites: &HashSet, @@ -945,8 +1110,9 @@ fn count_config_options(option: &acp::SessionConfigOption) -> usize { mod tests { use super::*; use acp_thread::AgentConnection; + use feature_flags::FeatureFlag as _; use fs::FakeFs; - use gpui::TestAppContext; + use gpui::{TestAppContext, UpdateGlobal}; use parking_lot::Mutex; use project::{AgentId, Project}; use std::{any::Any, cell::RefCell}; @@ -988,17 +1154,219 @@ mod tests { assert_eq!( agent_server.saved_defaults.lock().as_slice(), - &[("mode".to_string(), Some("manual".to_string()))] + &[( + "mode".to_string(), + Some(AgentConfigOptionValue::ValueId("manual".to_string())) + )] ); assert_eq!( config_options.set_values.borrow().as_slice(), - &[("mode".to_string(), "manual".to_string())] + &[( + "mode".to_string(), + acp::SessionConfigOptionValue::value_id("manual") + )] ); } + #[gpui::test] + fn cycling_boolean_config_option_saves_selected_value_as_default(cx: &mut TestAppContext) { + let agent_server = Rc::new(TestAgentServer::default()); + let config_options = Rc::new(TestSessionConfigOptions::new(vec![ + acp::SessionConfigOption::boolean("web_search", "Web Search", false) + .category(acp::SessionConfigOptionCategory::ModelConfig), + ])); + let fs: Arc = FakeFs::new(cx.executor()); + + cx.update(|cx| { + init_feature_flag_settings(cx); + set_feature_flag_override(AcpBetaFeatureFlag::NAME, "on", cx); + + let config_options: Rc = config_options.clone(); + let agent_server: Rc = agent_server.clone(); + let fs = fs.clone(); + let view = cx.new(|_| ConfigOptionsView { + config_option_ids: ConfigOptionsView::config_option_ids(&config_options), + config_options, + selectors: Vec::new(), + agent_server, + fs, + _refresh_task: Task::ready(()), + }); + + assert!(view.update(cx, |view, cx| { + view.cycle_category_option(acp::SessionConfigOptionCategory::ModelConfig, false, cx) + })); + }); + + assert_eq!( + agent_server.saved_defaults.lock().as_slice(), + &[( + "web_search".to_string(), + Some(AgentConfigOptionValue::Boolean(true)) + )] + ); + assert_eq!( + config_options.set_values.borrow().as_slice(), + &[( + "web_search".to_string(), + acp::SessionConfigOptionValue::boolean(true) + )] + ); + } + + #[gpui::test] + fn cycling_hidden_boolean_config_option_is_unhandled(cx: &mut TestAppContext) { + let agent_server = Rc::new(TestAgentServer::default()); + let config_options = Rc::new(TestSessionConfigOptions::new(vec![ + acp::SessionConfigOption::boolean("web_search", "Web Search", false) + .category(acp::SessionConfigOptionCategory::ModelConfig), + ])); + let fs: Arc = FakeFs::new(cx.executor()); + + cx.update(|cx| { + init_feature_flag_settings(cx); + set_feature_flag_override(AcpBetaFeatureFlag::NAME, "off", cx); + + let config_options: Rc = config_options.clone(); + let agent_server: Rc = agent_server.clone(); + let fs = fs.clone(); + let view = cx.new(|_| ConfigOptionsView { + config_option_ids: ConfigOptionsView::config_option_ids(&config_options), + config_options, + selectors: Vec::new(), + agent_server, + fs, + _refresh_task: Task::ready(()), + }); + + assert!(!view.update(cx, |view, cx| { + view.cycle_category_option(acp::SessionConfigOptionCategory::ModelConfig, false, cx) + })); + }); + + assert!(agent_server.saved_defaults.lock().is_empty()); + assert!(config_options.set_values.borrow().is_empty()); + } + + #[gpui::test] + fn cycling_category_skips_hidden_boolean_config_option(cx: &mut TestAppContext) { + let agent_server = Rc::new(TestAgentServer::default()); + let config_options = Rc::new(TestSessionConfigOptions::new(vec![ + acp::SessionConfigOption::boolean("web_search", "Web Search", false) + .category(acp::SessionConfigOptionCategory::Model), + acp::SessionConfigOption::select( + "model", + "Model", + "small", + vec![ + acp::SessionConfigSelectOption::new("small", "Small"), + acp::SessionConfigSelectOption::new("large", "Large"), + ], + ) + .category(acp::SessionConfigOptionCategory::Model), + ])); + let fs: Arc = FakeFs::new(cx.executor()); + + cx.update(|cx| { + init_feature_flag_settings(cx); + set_feature_flag_override(AcpBetaFeatureFlag::NAME, "off", cx); + + let config_options: Rc = config_options.clone(); + let agent_server: Rc = agent_server.clone(); + let fs = fs.clone(); + let view = cx.new(|_| ConfigOptionsView { + config_option_ids: ConfigOptionsView::config_option_ids(&config_options), + config_options, + selectors: Vec::new(), + agent_server, + fs, + _refresh_task: Task::ready(()), + }); + + assert!(view.update(cx, |view, cx| { + view.cycle_category_option(acp::SessionConfigOptionCategory::Model, false, cx) + })); + }); + + assert_eq!( + agent_server.saved_defaults.lock().as_slice(), + &[( + "model".to_string(), + Some(AgentConfigOptionValue::ValueId("large".to_string())) + )] + ); + assert_eq!( + config_options.set_values.borrow().as_slice(), + &[( + "model".to_string(), + acp::SessionConfigOptionValue::value_id("large") + )] + ); + } + + #[gpui::test] + fn toggling_category_picker_without_select_config_option_is_unhandled(cx: &mut TestAppContext) { + let agent_server = Rc::new(TestAgentServer::default()); + let config_options = Rc::new(TestSessionConfigOptions::new(vec![ + acp::SessionConfigOption::boolean("web_search", "Web Search", false) + .category(acp::SessionConfigOptionCategory::Model), + ])); + let fs: Arc = FakeFs::new(cx.executor()); + let cx = cx.add_empty_window(); + let view = cx.update({ + move |window, cx| { + let config_options: Rc = config_options; + let agent_server: Rc = agent_server; + cx.new(|cx| ConfigOptionsView::new(config_options, agent_server, fs, window, cx)) + } + }); + + let handled = cx.update(|window, cx| { + view.update(cx, |view, cx| { + view.toggle_category_picker(acp::SessionConfigOptionCategory::Model, window, cx) + }) + }); + + assert!(!handled); + } + + #[gpui::test] + fn boolean_config_option_rendering_is_beta_gated(cx: &mut TestAppContext) { + cx.update(|cx| { + init_feature_flag_settings(cx); + + cx.update_flags(false, Vec::new()); + set_feature_flag_override(AcpBetaFeatureFlag::NAME, "off", cx); + assert!(!should_render_boolean_config_options(cx)); + + set_feature_flag_override(AcpBetaFeatureFlag::NAME, "on", cx); + assert!(should_render_boolean_config_options(cx)); + }); + } + #[derive(Default)] struct TestAgentServer { - saved_defaults: Arc)>>>, + saved_defaults: Arc)>>>, + } + + fn init_feature_flag_settings(cx: &mut App) { + let store = SettingsStore::test(cx); + cx.set_global(store); + SettingsStore::update_global(cx, |store, _| { + store.register_setting::(); + }); + cx.update_flags(false, Vec::new()); + } + + fn set_feature_flag_override(name: &str, value: &str, cx: &mut App) { + SettingsStore::update_global(cx, |store, cx| { + store.update_user_settings(cx, |content| { + content + .feature_flags + .get_or_insert_default() + .insert(name.to_string(), value.to_string()); + }); + }); } impl AgentServer for TestAgentServer { @@ -1026,20 +1394,19 @@ mod tests { fn set_default_config_option( &self, config_id: &str, - value_id: Option<&str>, + value: Option, _fs: Arc, _cx: &mut App, ) { - self.saved_defaults.lock().push(( - config_id.to_string(), - value_id.map(|value| value.to_string()), - )); + self.saved_defaults + .lock() + .push((config_id.to_string(), value)); } } struct TestSessionConfigOptions { options: RefCell>, - set_values: RefCell>, + set_values: RefCell>, } impl TestSessionConfigOptions { @@ -1059,19 +1426,31 @@ mod tests { fn set_config_option( &self, config_id: acp::SessionConfigId, - value: acp::SessionConfigValueId, + value: acp::SessionConfigOptionValue, _cx: &mut App, ) -> Task>> { self.set_values .borrow_mut() - .push((config_id.0.to_string(), value.0.to_string())); + .push((config_id.0.to_string(), value.clone())); let options = { let mut options = self.options.borrow_mut(); - if let Some(option) = options.iter_mut().find(|option| option.id == config_id) - && let acp::SessionConfigKind::Select(select) = &mut option.kind - { - select.current_value = value; + if let Some(option) = options.iter_mut().find(|option| option.id == config_id) { + match (&mut option.kind, value) { + ( + acp::SessionConfigKind::Select(select), + acp::SessionConfigOptionValue::ValueId { value }, + ) => { + select.current_value = value; + } + ( + acp::SessionConfigKind::Boolean(boolean), + acp::SessionConfigOptionValue::Boolean { value }, + ) => { + boolean.current_value = value; + } + _ => {} + } } options.clone() }; diff --git a/crates/agent_ui/src/conversation_view.rs b/crates/agent_ui/src/conversation_view.rs index e3e941a140b..a101920102b 100644 --- a/crates/agent_ui/src/conversation_view.rs +++ b/crates/agent_ui/src/conversation_view.rs @@ -1160,7 +1160,6 @@ impl ConversationView { // Check for config options first // Config options take precedence over legacy mode/model selectors - // (feature flag gating happens at the data layer) let config_options_provider = connection.session_config_options(&session_id, cx); let config_options_view; diff --git a/crates/project/src/agent_server_store.rs b/crates/project/src/agent_server_store.rs index caeb28612ed..89f8b98354f 100644 --- a/crates/project/src/agent_server_store.rs +++ b/crates/project/src/agent_server_store.rs @@ -21,7 +21,7 @@ use rpc::{AnyProtoClient, TypedEnvelope, proto}; use schemars::JsonSchema; use semver::Version; use serde::{Deserialize, Serialize}; -use settings::{RegisterSetting, SettingsStore, update_settings_file}; +use settings::{AgentConfigOptionValue, RegisterSetting, SettingsStore, update_settings_file}; use sha2::{Digest, Sha256}; use url::Url; use util::{ResultExt as _, debug_panic}; @@ -1524,10 +1524,10 @@ pub enum CustomAgentServerSettings { default_mode: Option, /// Default values for session config options. /// - /// This is a map from config option ID to value ID. + /// This is a map from config option ID to the default value for that option. /// /// Default: {} - default_config_options: HashMap, + default_config_options: HashMap, /// Favorited values for session config options. /// /// This is a map from config option ID to a list of favorited value IDs. @@ -1548,10 +1548,10 @@ pub enum CustomAgentServerSettings { default_mode: Option, /// Default values for session config options. /// - /// This is a map from config option ID to value ID. + /// This is a map from config option ID to the default value for that option. /// /// Default: {} - default_config_options: HashMap, + default_config_options: HashMap, /// Favorited values for session config options. /// /// This is a map from config option ID to a list of favorited value IDs. @@ -1576,7 +1576,7 @@ impl CustomAgentServerSettings { } } - pub fn default_config_option(&self, config_id: &str) -> Option<&str> { + pub fn default_config_option(&self, config_id: &str) -> Option<&AgentConfigOptionValue> { match self { CustomAgentServerSettings::Custom { default_config_options, @@ -1585,7 +1585,7 @@ impl CustomAgentServerSettings { | CustomAgentServerSettings::Registry { default_config_options, .. - } => default_config_options.get(config_id).map(|s| s.as_str()), + } => default_config_options.get(config_id), } } diff --git a/crates/settings_content/src/agent.rs b/crates/settings_content/src/agent.rs index e0ac05fcd6d..2e2218c5e30 100644 --- a/crates/settings_content/src/agent.rs +++ b/crates/settings_content/src/agent.rs @@ -667,6 +667,56 @@ impl std::ops::DerefMut for AllAgentServersSettings { } } +#[derive(Deserialize, Serialize, Clone, JsonSchema, MergeFrom, Debug, PartialEq, Eq)] +#[serde(untagged)] +pub enum AgentConfigOptionValue { + ValueId(String), + Boolean(bool), +} + +impl AgentConfigOptionValue { + pub fn as_value_id(&self) -> Option<&str> { + match self { + Self::ValueId(value) => Some(value), + Self::Boolean(_) => None, + } + } + + pub fn as_bool(&self) -> Option { + match self { + Self::Boolean(value) => Some(*value), + Self::ValueId(_) => None, + } + } +} + +impl std::fmt::Display for AgentConfigOptionValue { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::ValueId(value) => formatter.write_str(value), + Self::Boolean(value) => value.fmt(formatter), + } + } +} + +impl From for AgentConfigOptionValue { + fn from(value: String) -> Self { + Self::ValueId(value) + } +} + +impl From<&str> for AgentConfigOptionValue { + fn from(value: &str) -> Self { + Self::ValueId(value.to_string()) + } +} + +impl From for AgentConfigOptionValue { + fn from(value: bool) -> Self { + Self::Boolean(value) + } +} + #[with_fallible_options] #[derive(Deserialize, Serialize, Clone, JsonSchema, MergeFrom, Debug, PartialEq)] #[serde(tag = "type", rename_all = "snake_case")] @@ -687,11 +737,11 @@ pub enum CustomAgentServerSettings { default_mode: Option, /// Default values for session config options. /// - /// This is a map from config option ID to value ID. + /// This is a map from config option ID to the default value for that option. /// /// Default: {} #[serde(default, skip_serializing_if = "HashMap::is_empty")] - default_config_options: HashMap, + default_config_options: HashMap, /// Favorited values for session config options. /// /// This is a map from config option ID to a list of favorited value IDs. @@ -716,11 +766,11 @@ pub enum CustomAgentServerSettings { default_mode: Option, /// Default values for session config options. /// - /// This is a map from config option ID to value ID. + /// This is a map from config option ID to the default value for that option. /// /// Default: {} #[serde(default, skip_serializing_if = "HashMap::is_empty")] - default_config_options: HashMap, + default_config_options: HashMap, /// Favorited values for session config options. /// /// This is a map from config option ID to a list of favorited value IDs. @@ -863,6 +913,46 @@ impl std::fmt::Display for ToolPermissionMode { mod tests { use super::*; + #[test] + fn agent_config_option_value_serializes_value_id_as_string() { + let value = AgentConfigOptionValue::from("manual"); + + assert_eq!( + serde_json::to_value(&value).expect("serialize value id"), + serde_json::json!("manual") + ); + assert_eq!( + serde_json::from_value::(serde_json::json!("manual")) + .expect("deserialize value id"), + AgentConfigOptionValue::ValueId("manual".to_string()) + ); + } + + #[test] + fn agent_config_option_value_serializes_boolean_as_boolean() { + let value = AgentConfigOptionValue::Boolean(true); + + assert_eq!( + serde_json::to_value(&value).expect("serialize boolean"), + serde_json::json!(true) + ); + assert_eq!( + serde_json::from_value::(serde_json::json!(true)) + .expect("deserialize boolean"), + AgentConfigOptionValue::Boolean(true) + ); + } + + #[test] + fn agent_config_option_value_merge_replaces_existing_value() { + use crate::merge_from::MergeFrom as _; + + let mut value = AgentConfigOptionValue::ValueId("manual".to_string()); + value.merge_from(&AgentConfigOptionValue::Boolean(true)); + + assert_eq!(value, AgentConfigOptionValue::Boolean(true)); + } + #[test] fn test_set_tool_default_permission_creates_structure() { let mut settings = AgentSettingsContent::default(); diff --git a/crates/settings_ui/src/pages/external_agents_page.rs b/crates/settings_ui/src/pages/external_agents_page.rs index 3369b58be0b..f05937680ed 100644 --- a/crates/settings_ui/src/pages/external_agents_page.rs +++ b/crates/settings_ui/src/pages/external_agents_page.rs @@ -9,7 +9,9 @@ use gpui::{ }; use itertools::Itertools as _; use project::agent_server_store::{AgentId, AgentServerStore, ExternalAgentSource}; -use settings::{CustomAgentServerSettings, SettingsStore, update_settings_file}; +use settings::{ + AgentConfigOptionValue, CustomAgentServerSettings, SettingsStore, update_settings_file, +}; use ui::{ AiSettingItem, AiSettingItemSource, AiSettingItemStatus, ContextMenu, ContextMenuEntry, Divider, DividerColor, PopoverMenu, Tooltip, prelude::*, @@ -366,7 +368,7 @@ pub(crate) struct CustomAgentForm { /// Advanced fields not surfaced by the form. They're preserved verbatim so /// editing the basic settings doesn't drop a user's hand-written config. default_mode: Option, - default_config_options: HashMap, + default_config_options: HashMap, favorite_config_option_values: HashMap>, /// Stable handles for the Cancel/Save buttons so they can render a focus /// ring. `Filled`/`Subtle` buttons only get a subtle `focus_visible` @@ -812,7 +814,7 @@ struct CustomAgentFormValues { args: String, env: Vec<(String, String)>, default_mode: Option, - default_config_options: HashMap, + default_config_options: HashMap, favorite_config_option_values: HashMap>, } @@ -1157,7 +1159,7 @@ mod tests { let mut values = values(); values.default_mode = Some("ask".into()); values.default_config_options = - HashMap::from_iter([("opt".to_string(), "val".to_string())]); + HashMap::from_iter([("opt".to_string(), AgentConfigOptionValue::from("val"))]); let (_, _, content) = build_settings_from_values(values).unwrap(); match content { @@ -1168,8 +1170,10 @@ mod tests { } => { assert_eq!(default_mode.as_deref(), Some("ask")); assert_eq!( - default_config_options.get("opt").map(String::as_str), - Some("val") + default_config_options + .get("opt") + .and_then(AgentConfigOptionValue::as_value_id), + Some("val"), ); } _ => panic!("expected a custom agent"),