feat(agent): adds mcp json backwards compatibility (#2292)

* adds alternative format for mcp server field in agent config

* adjusts migration flow for mcp backwards compatibility

* fixes typo

* implements @Native

* WIP: utilizes type check to ensure invariant

* makes agent generic to prevent api misuse

* allows optional @Builtin prefix

* reverts typestate pattern

* assign agent name to agent during thaw

* adds comment for freeze and thaw

* monomorphisizes mcp server field in agent config

* modifies migration flow to use database for migration status

* updates agent doc comments

* fixes errors from merge

* makes freeze and thaw private

Author: dingfeli

crates/chat-cli/src/cli/agent/legacy/mod.rs

@@ -21,6 +21,11 @@ use crate::os::Os;
 use crate::util::directories;
 
 pub async fn migrate(os: &mut Os) -> eyre::Result<Vec<Agent>> {
+    let has_migrated = os.database.get_has_migrated()?;
+    if has_migrated.is_some_and(|has_migrated| has_migrated) {
+        bail!("Nothing to migrate");
+    }
+
     let legacy_global_context_path = directories::chat_global_context_path(os)?;
     let legacy_global_context: Option<LegacyContextConfig> = 'global: {
         let Ok(content) = os.fs.read(&legacy_global_context_path).await else {
@@ -70,7 +75,12 @@ pub async fn migrate(os: &mut Os) -> eyre::Result<Vec<Agent>> {
         let config_path = directories::chat_legacy_mcp_config(os)?;
         if os.fs.exists(&config_path) {
             match McpServerConfig::load_from_file(os, config_path).await {
-                Ok(config) => Some(config),
+                Ok(mut config) => {
+                    config.mcp_servers.iter_mut().for_each(|(_name, config)| {
+                        config.is_from_legacy_mcp_json = true;
+                    });
+                    Some(config)
+                },
                 Err(e) => {
                     error!("Malformed legacy global mcp config detected: {e}. Skipping mcp migration.");
                     None
@@ -82,6 +92,7 @@ pub async fn migrate(os: &mut Os) -> eyre::Result<Vec<Agent>> {
     };
 
     if legacy_global_context.is_none() && legacy_profiles.is_empty() {
+        os.database.set_has_migrated()?;
         bail!("Nothing to migrate");
     }
 
@@ -113,8 +124,6 @@ pub async fn migrate(os: &mut Os) -> eyre::Result<Vec<Agent>> {
     const DEFAULT_DESC: &str = "This is an agent migrated from global context";
     const PROFILE_DESC: &str = "This is an agent migrated from profile context";
 
-    let has_global_context = legacy_global_context.is_some();
-
     // Migration of global context
     let mut new_agents = vec![];
     if let Some(context) = legacy_global_context {
@@ -192,20 +201,8 @@ pub async fn migrate(os: &mut Os) -> eyre::Result<Vec<Agent>> {
         os.fs.create_dir_all(&global_agent_path).await?;
     }
 
-    let formatted_server_list = mcp_servers
-        .map(|config| {
-            config
-                .mcp_servers
-                .keys()
-                .map(|server_name| format!("@{server_name}"))
-                .collect::<Vec<_>>()
-        })
-        .unwrap_or_default();
-
     for agent in &mut new_agents {
-        agent.tools.extend(formatted_server_list.clone());
-
-        let content = serde_json::to_string_pretty(agent)?;
+        let content = agent.to_str_pretty()?;
         if let Some(path) = agent.path.as_ref() {
             info!("Agent {} peristed in path {}", agent.name, path.to_string_lossy());
             os.fs.write(path, content).await?;
@@ -217,27 +214,7 @@ pub async fn migrate(os: &mut Os) -> eyre::Result<Vec<Agent>> {
         }
     }
 
-    let legacy_profile_config_path = directories::chat_profiles_dir(os)?;
-    let profile_backup_path = legacy_profile_config_path
-        .parent()
-        .ok_or(eyre::eyre!("Failed to obtain profile config parent path"))?
-        .join("profiles.bak");
-    os.fs.rename(legacy_profile_config_path, profile_backup_path).await?;
-
-    if has_global_context {
-        let legacy_global_config_path = directories::chat_global_context_path(os)?;
-        let legacy_global_config_file_name = legacy_global_config_path
-            .file_name()
-            .ok_or(eyre::eyre!("Failed to obtain legacy global config name"))?
-            .to_string_lossy();
-        let global_context_backup_path = legacy_global_config_path
-            .parent()
-            .ok_or(eyre::eyre!("Failed to obtain parent path for global context"))?
-            .join(format!("{}.bak", legacy_global_config_file_name));
-        os.fs
-            .rename(legacy_global_config_path, global_context_backup_path)
-            .await?;
-    }
+    os.database.set_has_migrated()?;
 
     Ok(new_agents)
 }

crates/chat-cli/src/cli/agent/mod.rs

@@ -66,8 +66,30 @@ use crate::util::{
 /// An [Agent] is a declarative way of configuring a given instance of q chat. Currently, it is
 /// impacting q chat in via influenicng [ContextManager] and [ToolManager].
 /// Changes made to [ContextManager] and [ToolManager] do not persist across sessions.
+///
+/// To increase the usability of the agent config, (both from the perspective of CLI and the users
+/// who would need to write these config), the agent config has two states of existence: "cold" and
+/// "warm".
+///
+/// A "cold" state describes the config as it is written. And a "warm" state is an alternate form
+/// of the same config, modified for the convenience of the business logic that relies on it in the
+/// application.
+///
+/// For example, the "cold" state does not require the field of "path" to be populated. This is
+/// because it would be redundant and tedious for user to have to write the path of the file they
+/// had created in said file. This field is thus populated during its parsing.
+///
+/// Another example is the mcp config. To support backwards compatibility of users existing global
+/// mcp.json, we allow users to supply a flag to denote whether they would want to include servers
+/// from the legacy global mcp.json. If this flag exists, we would need to read the legacy mcp
+/// config and merge it with what is in the agent mcp servers field. Conversely, when we write this
+/// config to file, we would want to filter out the servers that belong only in the mcp.json.
+///
+/// Where agents are instantiated from their config, we would need to convert them from "cold" to
+/// "warm".
 #[derive(Debug, Clone, Serialize, Deserialize, Eq, PartialEq, JsonSchema)]
 #[serde(rename_all = "camelCase")]
+#[schemars(description = "An Agent is a declarative way of configuring a given instance of q chat.")]
 pub struct Agent {
     /// Agent names are derived from the file name. Thus they are skipped for
     /// serializing
@@ -105,6 +127,11 @@ pub struct Agent {
     #[serde(default)]
     #[schemars(schema_with = "tool_settings_schema")]
     pub tools_settings: HashMap<ToolSettingTarget, serde_json::Value>,
+    /// Whether or not to include the legacy ~/.aws/amazonq/mcp.json in the agent
+    /// You can reference tools brought in by these servers as just as you would with the servers
+    /// you configure in the mcpServers field in this config
+    #[serde(default)]
+    pub use_legacy_mcp_json: bool,
     #[serde(skip)]
     pub path: Option<PathBuf>,
 }
@@ -116,7 +143,7 @@ impl Default for Agent {
             description: Some("Default agent".to_string()),
             prompt: Default::default(),
             mcp_servers: Default::default(),
-            tools: NATIVE_TOOLS.iter().copied().map(str::to_string).collect::<Vec<_>>(),
+            tools: vec!["*".to_string()],
             tool_aliases: Default::default(),
             allowed_tools: {
                 let mut set = HashSet::<String>::new();
@@ -130,20 +157,83 @@ impl Default for Agent {
                 .collect::<Vec<_>>(),
             hooks: Default::default(),
             tools_settings: Default::default(),
+            use_legacy_mcp_json: true,
             path: None,
         }
     }
 }
 
 impl Agent {
+    /// This function mutates the agent to a state that is writable.
+    /// Practically this means reverting some fields back to their original values as they were
+    /// written in the config.
+    fn freeze(&mut self) -> eyre::Result<()> {
+        let Self { mcp_servers, .. } = self;
+
+        mcp_servers
+            .mcp_servers
+            .retain(|_name, config| !config.is_from_legacy_mcp_json);
+
+        Ok(())
+    }
+
+    /// This function mutates the agent to a state that is usable for runtime.
+    /// Practically this means to convert some of the fields value to their usable counterpart.
+    /// For example, we populate the agent with its file name, convert the mcp array to actual
+    /// mcp config and populate the agent file path.
+    fn thaw(&mut self, path: &Path, global_mcp_config: Option<&McpServerConfig>) -> eyre::Result<()> {
+        let Self { mcp_servers, .. } = self;
+
+        let name = path
+            .file_stem()
+            .ok_or(eyre::eyre!("Missing valid file name"))?
+            .to_string_lossy()
+            .to_string();
+
+        self.name = name.clone();
+
+        if let (true, Some(global_mcp_config)) = (self.use_legacy_mcp_json, global_mcp_config) {
+            let mut stderr = std::io::stderr();
+            for (name, legacy_server) in &global_mcp_config.mcp_servers {
+                if mcp_servers.mcp_servers.contains_key(name) {
+                    let _ = queue!(
+                        stderr,
+                        style::SetForegroundColor(Color::Yellow),
+                        style::Print("WARNING: "),
+                        style::ResetColor,
+                        style::Print("MCP server '"),
+                        style::SetForegroundColor(Color::Green),
+                        style::Print(name),
+                        style::ResetColor,
+                        style::Print(
+                            "' is already configured in agent config. Skipping duplicate from legacy mcp.json.\n"
+                        )
+                    );
+                    continue;
+                }
+                let mut server_clone = legacy_server.clone();
+                server_clone.is_from_legacy_mcp_json = true;
+                mcp_servers.mcp_servers.insert(name.clone(), server_clone);
+            }
+        }
+
+        Ok(())
+    }
+
+    pub fn to_str_pretty(&self) -> eyre::Result<String> {
+        let mut agent_clone = self.clone();
+        agent_clone.freeze()?;
+        Ok(serde_json::to_string_pretty(&agent_clone)?)
+    }
+
     /// Retrieves an agent by name. It does so via first seeking the given agent under local dir,
     /// and falling back to global dir if it does not exist in local.
     pub async fn get_agent_by_name(os: &Os, agent_name: &str) -> eyre::Result<(Agent, PathBuf)> {
         let config_path: Result<PathBuf, PathBuf> = 'config: {
             // local first, and then fall back to looking at global
-            let local_config_dir = directories::chat_local_agent_dir()?.join(agent_name);
+            let local_config_dir = directories::chat_local_agent_dir()?.join(format!("{agent_name}.json"));
             if os.fs.exists(&local_config_dir) {
-                break 'config Ok::<PathBuf, PathBuf>(local_config_dir);
+                break 'config Ok(local_config_dir);
             }
 
             let global_config_dir = directories::chat_global_agent_path(os)?.join(format!("{agent_name}.json"));
@@ -157,23 +247,18 @@ impl Agent {
         match config_path {
             Ok(config_path) => {
                 let content = os.fs.read(&config_path).await?;
-                Ok((serde_json::from_slice::<Agent>(&content)?, config_path))
-            },
-            Err(global_config_dir) if agent_name == "default" => {
-                os.fs
-                    .create_dir_all(
-                        global_config_dir
-                            .parent()
-                            .ok_or(eyre::eyre!("Failed to retrieve global agent config parent path"))?,
-                    )
-                    .await?;
-                os.fs.create_new(&global_config_dir).await?;
-
-                let default_agent = Agent::default();
-                let content = serde_json::to_string_pretty(&default_agent)?;
-                os.fs.write(&global_config_dir, content.as_bytes()).await?;
-
-                Ok((default_agent, global_config_dir))
+                let mut agent = serde_json::from_slice::<Agent>(&content)?;
+
+                let global_mcp_path = directories::chat_legacy_mcp_config(os)?;
+                let global_mcp_config = match McpServerConfig::load_from_file(os, global_mcp_path).await {
+                    Ok(config) => Some(config),
+                    Err(e) => {
+                        tracing::error!("Error loading global mcp json path: {e}.");
+                        None
+                    },
+                };
+                agent.thaw(&config_path, global_mcp_config.as_ref())?;
+                Ok((agent, config_path))
             },
             _ => bail!("Agent {agent_name} does not exist"),
         }
@@ -252,7 +337,8 @@ impl Agents {
             path: Some(agent_path.clone()),
             ..Default::default()
         };
-        let contents = serde_json::to_string_pretty(&agent)
+        let contents = agent
+            .to_str_pretty()
             .map_err(|e| eyre::eyre!("Failed to serialize profile configuration: {}", e))?;
 
         if let Some(parent) = agent_path.parent() {
@@ -307,6 +393,8 @@ impl Agents {
             vec![]
         };
 
+        let mut global_mcp_config = None::<McpServerConfig>;
+
         let mut local_agents = 'local: {
             // We could be launching from the home dir, in which case the global and local agents
             // are the same set of agents. If that is the case, we simply skip this.
@@ -324,7 +412,7 @@ impl Agents {
             let Ok(files) = os.fs.read_dir(path).await else {
                 break 'local Vec::<Agent>::new();
             };
-            load_agents_from_entries(files).await
+            load_agents_from_entries(files, os, &mut global_mcp_config).await
         };
 
         let mut global_agents = 'global: {
@@ -342,7 +430,7 @@ impl Agents {
                     break 'global Vec::<Agent>::new();
                 },
             };
-            load_agents_from_entries(files).await
+            load_agents_from_entries(files, os, &mut global_mcp_config).await
         }
         .into_iter()
         .chain(new_agents)
@@ -385,7 +473,7 @@ impl Agents {
                 },
                 ..Default::default()
             };
-            let Ok(content) = serde_json::to_string_pretty(&example_agent) else {
+            let Ok(content) = example_agent.to_str_pretty() else {
                 error!("Error serializing example agent config");
                 break 'example_config;
             };
@@ -522,8 +610,13 @@ impl Agents {
     }
 }
 
-async fn load_agents_from_entries(mut files: ReadDir) -> Vec<Agent> {
+async fn load_agents_from_entries(
+    mut files: ReadDir,
+    os: &Os,
+    global_mcp_config: &mut Option<McpServerConfig>,
+) -> Vec<Agent> {
     let mut res = Vec::<Agent>::new();
+
     while let Ok(Some(file)) = files.next_entry().await {
         let file_path = &file.path();
         if file_path
@@ -539,6 +632,7 @@ async fn load_agents_from_entries(mut files: ReadDir) -> Vec<Agent> {
                     continue;
                 },
             };
+
             let mut agent = match serde_json::from_slice::<Agent>(&content) {
                 Ok(mut agent) => {
                     agent.path = Some(file_path.clone());
@@ -550,13 +644,34 @@ async fn load_agents_from_entries(mut files: ReadDir) -> Vec<Agent> {
                     continue;
                 },
             };
-            if let Some(name) = Path::new(&file.file_name()).file_stem() {
-                agent.name = name.to_string_lossy().to_string();
-                res.push(agent);
-            } else {
-                let file_path = file_path.to_string_lossy();
-                tracing::error!("Unable to determine agent name from config file at {file_path}, skipping");
+
+            // The agent config could have use_legacy_mcp_json set to true but not have a valid
+            // global mcp.json. We would still need to carry on loading the config.
+            'load_legacy_mcp_json: {
+                if agent.use_legacy_mcp_json && global_mcp_config.is_none() {
+                    let Ok(global_mcp_path) = directories::chat_legacy_mcp_config(os) else {
+                        tracing::error!("Error obtaining legacy mcp json path. Skipping");
+                        break 'load_legacy_mcp_json;
+                    };
+                    let legacy_mcp_config = match McpServerConfig::load_from_file(os, global_mcp_path).await {
+                        Ok(config) => config,
+                        Err(e) => {
+                            tracing::error!("Error loading global mcp json path: {e}. Skipping");
+                            break 'load_legacy_mcp_json;
+                        },
+                    };
+                    global_mcp_config.replace(legacy_mcp_config);
+                }
             }
+
+            if let Err(e) = agent.thaw(file_path, global_mcp_config.as_ref()) {
+                tracing::error!(
+                    "Error transforming agent at {} to usable state: {e}. Skipping",
+                    file_path.display()
+                );
+            };
+
+            res.push(agent);
         }
     }
     res
@@ -806,11 +921,4 @@ mod tests {
         assert!(validate_agent_name("invalid!").is_err());
         assert!(validate_agent_name("invalid space").is_err());
     }
-
-    #[test]
-    fn test_schema_gen() {
-        use schemars::schema_for;
-        let schema = schema_for!(Agent);
-        println!("Schema for agent: {}", serde_json::to_string_pretty(&schema).unwrap());
-    }
 }

crates/chat-cli/src/cli/agent/root_command_args.rs

@@ -163,7 +163,7 @@ pub async fn create_agent(
 
     let prepopulated_content = if let Some(from) = from {
         let agent_to_copy = agents.switch(from.as_str())?;
-        serde_json::to_string_pretty(agent_to_copy)?
+        agent_to_copy.to_str_pretty()?
     } else {
         Default::default()
     };