agent_servers: Use correct default settings (#51136)

Ben Brandt created 1 month ago

These are edge cases, but there are a few ways you can get into a state
where you are setting favorites for registry agents and we don't have
the setting yet. This prioritizes `type: registry` for agents that we
have in the registry, especially the previous built-ins.

Release Notes:

- N/A

Change summary

crates/agent_servers/src/custom.rs         | 334 +++++++++++++++++------
crates/project/src/agent_registry_store.rs |  16 +
2 files changed, 266 insertions(+), 84 deletions(-)

Detailed changes

crates/agent_servers/src/custom.rs

@@ -84,19 +84,12 @@ impl AgentServer for CustomAgentServer {
         let config_id = config_id.to_string();
         let value_id = value_id.to_string();
 
-        update_settings_file(fs, cx, move |settings, _| {
+        update_settings_file(fs, cx, move |settings, cx| {
             let settings = settings
                 .agent_servers
                 .get_or_insert_default()
                 .entry(name.to_string())
-                .or_insert_with(|| settings::CustomAgentServerSettings::Extension {
-                    default_model: None,
-                    default_mode: None,
-                    env: Default::default(),
-                    favorite_models: Vec::new(),
-                    default_config_options: Default::default(),
-                    favorite_config_option_values: Default::default(),
-                });
+                .or_insert_with(|| default_settings_for_agent(&name, cx));
 
             match settings {
                 settings::CustomAgentServerSettings::Custom {
@@ -132,19 +125,12 @@ impl AgentServer for CustomAgentServer {
 
     fn set_default_mode(&self, mode_id: Option<acp::SessionModeId>, fs: Arc<dyn Fs>, cx: &mut App) {
         let name = self.name();
-        update_settings_file(fs, cx, move |settings, _| {
+        update_settings_file(fs, cx, move |settings, cx| {
             let settings = settings
                 .agent_servers
                 .get_or_insert_default()
                 .entry(name.to_string())
-                .or_insert_with(|| settings::CustomAgentServerSettings::Extension {
-                    default_model: None,
-                    default_mode: None,
-                    env: Default::default(),
-                    favorite_models: Vec::new(),
-                    default_config_options: Default::default(),
-                    favorite_config_option_values: Default::default(),
-                });
+                .or_insert_with(|| default_settings_for_agent(&name, cx));
 
             match settings {
                 settings::CustomAgentServerSettings::Custom { default_mode, .. }
@@ -171,19 +157,12 @@ impl AgentServer for CustomAgentServer {
 
     fn set_default_model(&self, model_id: Option<acp::ModelId>, fs: Arc<dyn Fs>, cx: &mut App) {
         let name = self.name();
-        update_settings_file(fs, cx, move |settings, _| {
+        update_settings_file(fs, cx, move |settings, cx| {
             let settings = settings
                 .agent_servers
                 .get_or_insert_default()
                 .entry(name.to_string())
-                .or_insert_with(|| settings::CustomAgentServerSettings::Extension {
-                    default_model: None,
-                    default_mode: None,
-                    env: Default::default(),
-                    favorite_models: Vec::new(),
-                    default_config_options: Default::default(),
-                    favorite_config_option_values: Default::default(),
-                });
+                .or_insert_with(|| default_settings_for_agent(&name, cx));
 
             match settings {
                 settings::CustomAgentServerSettings::Custom { default_model, .. }
@@ -222,19 +201,12 @@ impl AgentServer for CustomAgentServer {
         cx: &App,
     ) {
         let name = self.name();
-        update_settings_file(fs, cx, move |settings, _| {
+        update_settings_file(fs, cx, move |settings, cx| {
             let settings = settings
                 .agent_servers
                 .get_or_insert_default()
                 .entry(name.to_string())
-                .or_insert_with(|| settings::CustomAgentServerSettings::Extension {
-                    default_model: None,
-                    default_mode: None,
-                    env: Default::default(),
-                    favorite_models: Vec::new(),
-                    default_config_options: Default::default(),
-                    favorite_config_option_values: Default::default(),
-                });
+                .or_insert_with(|| default_settings_for_agent(&name, cx));
 
             let favorite_models = match settings {
                 settings::CustomAgentServerSettings::Custom {
@@ -282,19 +254,12 @@ impl AgentServer for CustomAgentServer {
         let name = self.name();
         let config_id = config_id.to_string();
         let value_id = value_id.map(|s| s.to_string());
-        update_settings_file(fs, cx, move |settings, _| {
+        update_settings_file(fs, cx, move |settings, cx| {
             let settings = settings
                 .agent_servers
                 .get_or_insert_default()
                 .entry(name.to_string())
-                .or_insert_with(|| settings::CustomAgentServerSettings::Extension {
-                    default_model: None,
-                    default_mode: None,
-                    env: Default::default(),
-                    favorite_models: Vec::new(),
-                    default_config_options: Default::default(),
-                    favorite_config_option_values: Default::default(),
-                });
+                .or_insert_with(|| default_settings_for_agent(&name, cx));
 
             match settings {
                 settings::CustomAgentServerSettings::Custom {
@@ -332,45 +297,27 @@ impl AgentServer for CustomAgentServer {
             .unwrap_or_else(|| name.clone());
         let default_mode = self.default_mode(cx);
         let default_model = self.default_model(cx);
-        let is_previous_built_in =
-            matches!(name.as_ref(), CLAUDE_AGENT_NAME | CODEX_NAME | GEMINI_NAME);
-        let (default_config_options, is_registry_agent) =
-            cx.read_global(|settings: &SettingsStore, _| {
-                let agent_settings = settings
-                    .get::<AllAgentServersSettings>(None)
-                    .get(self.name().as_ref());
-
-                let is_registry = agent_settings
-                    .map(|s| {
-                        matches!(
-                            s,
-                            project::agent_server_store::CustomAgentServerSettings::Registry { .. }
-                        )
-                    })
-                    .unwrap_or(false);
-
-                let config_options = agent_settings
-                    .map(|s| match s {
-                        project::agent_server_store::CustomAgentServerSettings::Custom {
-                            default_config_options,
-                            ..
-                        }
-                        | project::agent_server_store::CustomAgentServerSettings::Extension {
-                            default_config_options,
-                            ..
-                        }
-                        | project::agent_server_store::CustomAgentServerSettings::Registry {
-                            default_config_options,
-                            ..
-                        } => default_config_options.clone(),
-                    })
-                    .unwrap_or_default();
-
-                (config_options, is_registry)
-            });
-
-        // Intermediate step to allow for previous built-ins to also be triggered if they aren't in settings yet.
-        let is_registry_agent = is_registry_agent || is_previous_built_in;
+        let is_registry_agent = is_registry_agent(&name, cx);
+        let default_config_options = cx.read_global(|settings: &SettingsStore, _| {
+            settings
+                .get::<AllAgentServersSettings>(None)
+                .get(self.name().as_ref())
+                .map(|s| match s {
+                    project::agent_server_store::CustomAgentServerSettings::Custom {
+                        default_config_options,
+                        ..
+                    }
+                    | project::agent_server_store::CustomAgentServerSettings::Extension {
+                        default_config_options,
+                        ..
+                    }
+                    | project::agent_server_store::CustomAgentServerSettings::Registry {
+                        default_config_options,
+                        ..
+                    } => default_config_options.clone(),
+                })
+                .unwrap_or_default()
+        });
 
         if is_registry_agent {
             if let Some(registry_store) = project::AgentRegistryStore::try_global(cx) {
@@ -458,3 +405,222 @@ fn api_key_for_gemini_cli(cx: &mut App) -> Task<Result<String>> {
         )
     })
 }
+
+fn is_registry_agent(name: &str, cx: &App) -> bool {
+    let is_previous_built_in = matches!(name, CLAUDE_AGENT_NAME | CODEX_NAME | GEMINI_NAME);
+    let is_in_registry = project::AgentRegistryStore::try_global(cx)
+        .map(|store| store.read(cx).agent(name).is_some())
+        .unwrap_or(false);
+    let is_settings_registry = cx.read_global(|settings: &SettingsStore, _| {
+        settings
+            .get::<AllAgentServersSettings>(None)
+            .get(name)
+            .is_some_and(|s| {
+                matches!(
+                    s,
+                    project::agent_server_store::CustomAgentServerSettings::Registry { .. }
+                )
+            })
+    });
+    is_previous_built_in || is_in_registry || is_settings_registry
+}
+
+fn default_settings_for_agent(name: &str, cx: &App) -> settings::CustomAgentServerSettings {
+    if is_registry_agent(name, cx) {
+        settings::CustomAgentServerSettings::Registry {
+            default_model: None,
+            default_mode: None,
+            env: Default::default(),
+            favorite_models: Vec::new(),
+            default_config_options: Default::default(),
+            favorite_config_option_values: Default::default(),
+        }
+    } else {
+        settings::CustomAgentServerSettings::Extension {
+            default_model: None,
+            default_mode: None,
+            env: Default::default(),
+            favorite_models: Vec::new(),
+            default_config_options: Default::default(),
+            favorite_config_option_values: Default::default(),
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use collections::HashMap;
+    use gpui::TestAppContext;
+    use project::agent_registry_store::{
+        AgentRegistryStore, RegistryAgent, RegistryAgentMetadata, RegistryNpxAgent,
+    };
+    use settings::Settings as _;
+
+    fn init_test(cx: &mut TestAppContext) {
+        cx.update(|cx| {
+            let settings_store = SettingsStore::test(cx);
+            cx.set_global(settings_store);
+        });
+    }
+
+    fn init_registry_with_agents(cx: &mut TestAppContext, agent_ids: &[&str]) {
+        let agents: Vec<RegistryAgent> = agent_ids
+            .iter()
+            .map(|id| {
+                let id = SharedString::from(id.to_string());
+                RegistryAgent::Npx(RegistryNpxAgent {
+                    metadata: RegistryAgentMetadata {
+                        id: id.clone(),
+                        name: id.clone(),
+                        description: SharedString::from(""),
+                        version: SharedString::from("1.0.0"),
+                        repository: None,
+                        icon_path: None,
+                    },
+                    package: id,
+                    args: Vec::new(),
+                    env: HashMap::default(),
+                })
+            })
+            .collect();
+        cx.update(|cx| {
+            AgentRegistryStore::init_test_global(cx, agents);
+        });
+    }
+
+    fn set_agent_server_settings(
+        cx: &mut TestAppContext,
+        entries: Vec<(&str, settings::CustomAgentServerSettings)>,
+    ) {
+        cx.update(|cx| {
+            AllAgentServersSettings::override_global(
+                project::agent_server_store::AllAgentServersSettings(
+                    entries
+                        .into_iter()
+                        .map(|(name, settings)| (name.to_string(), settings.into()))
+                        .collect(),
+                ),
+                cx,
+            );
+        });
+    }
+
+    #[gpui::test]
+    fn test_previous_builtins_are_registry(cx: &mut TestAppContext) {
+        init_test(cx);
+        cx.update(|cx| {
+            assert!(is_registry_agent(CLAUDE_AGENT_NAME, cx));
+            assert!(is_registry_agent(CODEX_NAME, cx));
+            assert!(is_registry_agent(GEMINI_NAME, cx));
+        });
+    }
+
+    #[gpui::test]
+    fn test_unknown_agent_is_not_registry(cx: &mut TestAppContext) {
+        init_test(cx);
+        cx.update(|cx| {
+            assert!(!is_registry_agent("my-custom-agent", cx));
+        });
+    }
+
+    #[gpui::test]
+    fn test_agent_in_registry_store_is_registry(cx: &mut TestAppContext) {
+        init_test(cx);
+        init_registry_with_agents(cx, &["some-new-registry-agent"]);
+        cx.update(|cx| {
+            assert!(is_registry_agent("some-new-registry-agent", cx));
+            assert!(!is_registry_agent("not-in-registry", cx));
+        });
+    }
+
+    #[gpui::test]
+    fn test_agent_with_registry_settings_type_is_registry(cx: &mut TestAppContext) {
+        init_test(cx);
+        set_agent_server_settings(
+            cx,
+            vec![(
+                "agent-from-settings",
+                settings::CustomAgentServerSettings::Registry {
+                    env: HashMap::default(),
+                    default_mode: None,
+                    default_model: None,
+                    favorite_models: Vec::new(),
+                    default_config_options: HashMap::default(),
+                    favorite_config_option_values: HashMap::default(),
+                },
+            )],
+        );
+        cx.update(|cx| {
+            assert!(is_registry_agent("agent-from-settings", cx));
+        });
+    }
+
+    #[gpui::test]
+    fn test_agent_with_extension_settings_type_is_not_registry(cx: &mut TestAppContext) {
+        init_test(cx);
+        set_agent_server_settings(
+            cx,
+            vec![(
+                "my-extension-agent",
+                settings::CustomAgentServerSettings::Extension {
+                    env: HashMap::default(),
+                    default_mode: None,
+                    default_model: None,
+                    favorite_models: Vec::new(),
+                    default_config_options: HashMap::default(),
+                    favorite_config_option_values: HashMap::default(),
+                },
+            )],
+        );
+        cx.update(|cx| {
+            assert!(!is_registry_agent("my-extension-agent", cx));
+        });
+    }
+
+    #[gpui::test]
+    fn test_default_settings_for_builtin_agent(cx: &mut TestAppContext) {
+        init_test(cx);
+        cx.update(|cx| {
+            assert!(matches!(
+                default_settings_for_agent(CODEX_NAME, cx),
+                settings::CustomAgentServerSettings::Registry { .. }
+            ));
+            assert!(matches!(
+                default_settings_for_agent(CLAUDE_AGENT_NAME, cx),
+                settings::CustomAgentServerSettings::Registry { .. }
+            ));
+            assert!(matches!(
+                default_settings_for_agent(GEMINI_NAME, cx),
+                settings::CustomAgentServerSettings::Registry { .. }
+            ));
+        });
+    }
+
+    #[gpui::test]
+    fn test_default_settings_for_extension_agent(cx: &mut TestAppContext) {
+        init_test(cx);
+        cx.update(|cx| {
+            assert!(matches!(
+                default_settings_for_agent("some-extension-agent", cx),
+                settings::CustomAgentServerSettings::Extension { .. }
+            ));
+        });
+    }
+
+    #[gpui::test]
+    fn test_default_settings_for_agent_in_registry(cx: &mut TestAppContext) {
+        init_test(cx);
+        init_registry_with_agents(cx, &["new-registry-agent"]);
+        cx.update(|cx| {
+            assert!(matches!(
+                default_settings_for_agent("new-registry-agent", cx),
+                settings::CustomAgentServerSettings::Registry { .. }
+            ));
+            assert!(matches!(
+                default_settings_for_agent("not-in-registry", cx),
+                settings::CustomAgentServerSettings::Extension { .. }
+            ));
+        });
+    }
+}

crates/project/src/agent_registry_store.rs

@@ -147,6 +147,22 @@ impl AgentRegistryStore {
             .map(|store| store.0.clone())
     }
 
+    #[cfg(any(test, feature = "test-support"))]
+    pub fn init_test_global(cx: &mut App, agents: Vec<RegistryAgent>) -> Entity<Self> {
+        let fs: Arc<dyn Fs> = fs::FakeFs::new(cx.background_executor().clone());
+        let store = cx.new(|_cx| Self {
+            fs,
+            http_client: http_client::FakeHttpClient::with_404_response(),
+            agents,
+            is_fetching: false,
+            fetch_error: None,
+            pending_refresh: None,
+            last_refresh: None,
+        });
+        cx.set_global(GlobalAgentRegistryStore(store.clone()));
+        store
+    }
+
     pub fn agents(&self) -> &[RegistryAgent] {
         &self.agents
     }