From 9f941fbfc5f479d26747d13147457138163ab94e Mon Sep 17 00:00:00 2001 From: tic-top <78676563+tic-top@users.noreply.github.com> Date: Wed, 19 Aug 2026 00:17:58 +0000 Subject: [PATCH] feat(cli): let --with-extension name its extension (#11127) Signed-off-by: kirp@umich.edu Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: Douwe M Osinga --- crates/goose-cli/src/cli.rs | 2 +- crates/goose-cli/src/session/builder.rs | 238 +++++++++++++++++- crates/goose-cli/src/session/mod.rs | 137 +++++++++- .../docs/guides/goose-cli-commands.md | 5 +- 4 files changed, 361 insertions(+), 21 deletions(-) diff --git a/crates/goose-cli/src/cli.rs b/crates/goose-cli/src/cli.rs index a240d8227..c0ba3c1b5 100644 --- a/crates/goose-cli/src/cli.rs +++ b/crates/goose-cli/src/cli.rs @@ -176,7 +176,7 @@ pub struct ExtensionOptions { long = "with-extension", value_name = "COMMAND", help = "Add stdio extensions (can be specified multiple times)", - long_help = "Add stdio extensions from full commands with environment variables. Can be specified multiple times. Format: 'ENV1=val1 ENV2=val2 command args...'", + long_help = "Add stdio extensions from full commands with environment variables. Can be specified multiple times. Format: '[name:]ENV1=val1 ENV2=val2 command args...'. Without the optional name, the extension is named after the command, which is the launcher for anything started through one ('npx', 'python', 'uvx', ...); extensions that would end up sharing a name are instead named after their full command line.", action = clap::ArgAction::Append )] pub extensions: Vec, diff --git a/crates/goose-cli/src/session/builder.rs b/crates/goose-cli/src/session/builder.rs index 303372a94..94b4c7fff 100644 --- a/crates/goose-cli/src/session/builder.rs +++ b/crates/goose-cli/src/session/builder.rs @@ -1,9 +1,10 @@ use crate::cli::StreamableHttpOptions; use super::output; -use super::CliSession; +use super::{derive_extension_name_from_command, split_extension_name_prefix, CliSession}; use console::style; use goose::agents::{Agent, Container, ExtensionError}; +use goose::config::extensions::name_to_key; use goose::config::resolve_extensions_for_new_session; use goose::config::{Config, ExtensionConfig, GooseMode}; use goose::model_config::model_config_from_user_config; @@ -12,7 +13,7 @@ use goose::recipe::Recipe; use goose::session::session_manager::SessionType; use goose::session::EnabledExtensionsState; use rustyline::EditMode; -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashMap, HashSet}; use std::process; use std::sync::Arc; use tokio::task::JoinSet; @@ -28,11 +29,68 @@ fn truncate_with_ellipsis(s: &str, max_len: usize) -> String { } } +// Plain String rather than `ExtensionError`: the only variant this function +// ever constructs is `ConfigError(String)`, but clippy's `result_large_err` +// sizes an error type by its largest variant, and `ExtensionError` carries a +// `ClientError`/`ClientInitializeError` far past the 128-byte default +// threshold. Callers wrap this back into `ExtensionError::ConfigError`. +fn disambiguate_stdio_extension_names( + extensions: &mut [(String, ExtensionConfig)], + renameable: &HashSet, +) -> Result<(), String> { + let mut counts: HashMap = HashMap::new(); + // Only entries the caller marked fixed (explicitly named, or not a CLI + // stdio extension at all) can make this an error — a fixed name colliding + // with a renameable one is exactly the case the loop below resolves by + // renaming the renameable side, not a real conflict. + let mut fixed_counts: HashMap = HashMap::new(); + for (idx, (_, config)) in extensions.iter().enumerate() { + let key = config.key(); + *counts.entry(key.clone()).or_default() += 1; + if !renameable.contains(&idx) { + *fixed_counts.entry(key).or_default() += 1; + } + } + + let duplicate_fixed_names = extensions.iter().enumerate().find(|(index, (_, config))| { + !renameable.contains(index) && fixed_counts.get(&config.key()).copied().unwrap_or(0) > 1 + }); + if let Some((_, (_, config))) = duplicate_fixed_names { + return Err(format!( + "extension name '{}' is already in use", + config.name() + )); + } + + let mut taken: HashSet = counts.keys().cloned().collect(); + for (idx, (_, config)) in extensions.iter_mut().enumerate() { + if !renameable.contains(&idx) || counts.get(&config.key()).copied().unwrap_or(0) < 2 { + continue; + } + let ExtensionConfig::Stdio { + name, cmd, args, .. + } = config + else { + continue; + }; + let derived = derive_extension_name_from_command(cmd, args); + let mut candidate = derived.clone(); + let mut suffix = 2; + while candidate.is_empty() || taken.contains(&name_to_key(&candidate)) { + candidate = format!("{}_{}", derived, suffix); + suffix += 1; + } + taken.insert(name_to_key(&candidate)); + *name = candidate; + } + Ok(()) +} + fn parse_cli_flag_extensions( extensions: &[String], streamable_http_extensions: &[StreamableHttpOptions], builtins: &[String], -) -> Vec<(String, ExtensionConfig)> { +) -> Vec<(String, ExtensionConfig, bool)> { let mut extensions_to_load = Vec::new(); for (idx, ext_str) in extensions.iter().enumerate() { @@ -40,7 +98,8 @@ fn parse_cli_flag_extensions( Ok(config) => { let hint = truncate_with_ellipsis(ext_str, EXTENSION_HINT_MAX_LEN); let label = format!("stdio #{}({})", idx + 1, hint); - extensions_to_load.push((label, config)); + let explicitly_named = split_extension_name_prefix(ext_str).0.is_some(); + extensions_to_load.push((label, config, !explicitly_named)); } Err(e) => { eprintln!( @@ -59,13 +118,13 @@ fn parse_cli_flag_extensions( let config = CliSession::parse_streamable_http_extension(&opts.url, opts.timeout); let hint = truncate_with_ellipsis(&opts.url, EXTENSION_HINT_MAX_LEN); let label = format!("http #{}({})", idx + 1, hint); - extensions_to_load.push((label, config)); + extensions_to_load.push((label, config, false)); } for builtin_str in builtins { let configs = CliSession::parse_builtin_extensions(builtin_str); for config in configs { - extensions_to_load.push((config.name(), config)); + extensions_to_load.push((config.name(), config, false)); } } @@ -549,16 +608,34 @@ async fn collect_extension_configs( &session_config.builtins, ); - let mut all: Vec = configured_extensions; + let mut all: Vec<(String, ExtensionConfig)> = configured_extensions + .into_iter() + .map(|config| (config.name(), config)) + .collect(); if !session_config.no_profile && !session_config.resume && recipe_extensions.is_none() { let project_root = std::env::current_dir().ok(); - all.extend(goose::plugins::mcp_servers::enabled_plugin_mcp_servers( - project_root.as_deref(), - )); + all.extend( + goose::plugins::mcp_servers::enabled_plugin_mcp_servers(project_root.as_deref()) + .into_iter() + .map(|config| (config.name(), config)), + ); } - all.extend(cli_flag_extensions.into_iter().map(|(_, cfg)| cfg)); - Ok(all) + let cli_start = all.len(); + let renameable = cli_flag_extensions + .iter() + .enumerate() + .filter_map(|(index, (_, _, renameable))| renameable.then_some(cli_start + index)) + .collect(); + all.extend( + cli_flag_extensions + .into_iter() + .map(|(label, config, _)| (label, config)), + ); + disambiguate_stdio_extension_names(&mut all, &renameable) + .map_err(ExtensionError::ConfigError)?; + + Ok(all.into_iter().map(|(_, config)| config).collect()) } async fn resolve_and_load_extensions( @@ -832,6 +909,143 @@ mod tests { use goose::session::SessionManager; use tempfile::TempDir; + fn stdio_names(extensions: &[&str]) -> Vec { + let parsed = parse_cli_flag_extensions( + &extensions.iter().map(|s| s.to_string()).collect::>(), + &[], + &[], + ); + let renameable = parsed + .iter() + .enumerate() + .filter_map(|(index, (_, _, renameable))| renameable.then_some(index)) + .collect(); + let mut configs: Vec<_> = parsed + .into_iter() + .map(|(label, config, _)| (label, config)) + .collect(); + disambiguate_stdio_extension_names(&mut configs, &renameable).unwrap(); + configs + .into_iter() + .map(|(_, config)| config.name()) + .collect() + } + + #[test] + fn test_colliding_launcher_names_fall_back_to_the_command_line() { + assert_eq!( + stdio_names(&[ + "npx -y @modelcontextprotocol/server-memory", + "npx -y @modelcontextprotocol/server-filesystem", + ]), + vec!["server-memory".to_string(), "server-filesystem".to_string()] + ); + } + + #[test] + fn test_explicit_names_survive_a_collision() { + assert_eq!( + stdio_names(&["python -m word_mcp", "memory:python -m memory_mcp"]), + vec!["python".to_string(), "memory".to_string()] + ); + assert_eq!( + stdio_names(&[ + "word:python -m word_mcp", + "python -m a_mcp", + "python -m b_mcp", + ]), + vec![ + "word".to_string(), + "python_m_a_mcp".to_string(), + "python_m_b_mcp".to_string(), + ] + ); + } + + #[test] + fn test_normalized_names_are_disambiguated() { + assert_eq!( + stdio_names(&["MyTool --server a", "mytool --server b"]), + vec!["mytool_server_a".to_string(), "mytool_server_b".to_string()] + ); + } + + #[test] + fn test_cli_name_is_disambiguated_against_configured_extension() { + let mut extensions = vec![ + ( + "configured".to_string(), + CliSession::parse_stdio_extension("npx configured-server").unwrap(), + ), + ( + "cli".to_string(), + CliSession::parse_stdio_extension("npx cli-server").unwrap(), + ), + ]; + disambiguate_stdio_extension_names(&mut extensions, &HashSet::from([1])).unwrap(); + assert_eq!(extensions[0].1.name(), "npx"); + assert_eq!(extensions[1].1.name(), "npx_cli-server"); + } + + #[test] + fn test_fixed_counted_per_key_not_globally() { + // `duplicate_fixed_names` must count *fixed entries sharing a key*, + // not "is there more than one fixed entry at all" -- otherwise a + // fixed entry with a unique name, sitting alongside a fixed+renameable + // collision on a different key, would wrongly trip the same error. + let mut extensions = vec![ + ( + "configured a".to_string(), + // Fixed, name "word" -- unique, no collision with anything. + CliSession::parse_stdio_extension("word:npx server-a").unwrap(), + ), + ( + "configured b".to_string(), + // Fixed, name "npx" -- collides with the renameable entry below. + CliSession::parse_stdio_extension("npx server-b").unwrap(), + ), + ( + "cli".to_string(), + // Renameable, also defaults to "npx". + CliSession::parse_stdio_extension("npx cli-server").unwrap(), + ), + ]; + disambiguate_stdio_extension_names(&mut extensions, &HashSet::from([2])).unwrap(); + assert_eq!(extensions[0].1.name(), "word"); + assert_eq!(extensions[1].1.name(), "npx"); + assert_eq!(extensions[2].1.name(), "npx_cli-server"); + } + + #[test] + fn test_duplicate_explicit_names_are_rejected() { + let mut extensions = vec![ + ( + "first".to_string(), + CliSession::parse_stdio_extension("memory:npx first").unwrap(), + ), + ( + "second".to_string(), + CliSession::parse_stdio_extension("Memory:npx second").unwrap(), + ), + ]; + let error = + disambiguate_stdio_extension_names(&mut extensions, &HashSet::new()).unwrap_err(); + assert!(error + .to_string() + .contains("extension name 'memory' is already in use")); + } + + #[test] + fn test_identical_commands_still_get_distinct_names() { + assert_eq!( + stdio_names(&["python -m word_mcp", "python -m word_mcp"]), + vec![ + "python_m_word_mcp".to_string(), + "python_m_word_mcp_2".to_string() + ] + ); + } + fn test_config(temp_dir: &TempDir) -> Config { Config::new_with_file_secrets( temp_dir.path().join("config.yaml"), diff --git a/crates/goose-cli/src/session/mod.rs b/crates/goose-cli/src/session/mod.rs index c5321cb31..b4fec8ca7 100644 --- a/crates/goose-cli/src/session/mod.rs +++ b/crates/goose-cli/src/session/mod.rs @@ -70,6 +70,59 @@ const SHELL_STATUS_FALLBACK_WIDTH: usize = 120; const SHELL_STATUS_MAX_LINES: usize = 3; const SHELL_STATUS_RESERVED_WIDTH: usize = 2; +/// Upper bound on a *derived* extension name. Tools are exposed to the model as +/// `{extension}__{tool}`, and several providers cap tool name length, so a name +/// built out of a whole command line has to be clipped. +const DERIVED_EXTENSION_NAME_MAX_LEN: usize = 32; + +pub(crate) fn split_extension_name_prefix(extension_command: &str) -> (Option, &str) { + let Some((candidate, rest)) = extension_command.split_once(':') else { + return (None, extension_command); + }; + let looks_like_name = candidate.chars().count() >= 2 + && candidate + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-'); + if !looks_like_name || rest.trim().is_empty() || rest.starts_with("//") { + return (None, extension_command); + } + (Some(name_to_key(candidate)), rest) +} + +pub(crate) fn derive_extension_name_from_command(cmd: &str, args: &[String]) -> String { + let basename = std::path::Path::new(cmd) + .file_name() + .and_then(|f| f.to_str()) + .unwrap_or(cmd); + let joined = std::iter::once(basename) + .chain(args.iter().map(String::as_str)) + .map(|token| token.trim_start_matches('-')) + .collect::>() + .join("_"); + + let mut key = String::with_capacity(joined.len()); + for c in name_to_key(&joined).chars() { + if c == '_' && key.ends_with('_') { + continue; + } + key.push(c); + } + let key = key.trim_matches('_'); + + if key.chars().count() <= DERIVED_EXTENSION_NAME_MAX_LEN { + return key.to_string(); + } + let tail: String = key + .chars() + .skip(key.chars().count() - DERIVED_EXTENSION_NAME_MAX_LEN) + .collect(); + // Drop the leading fragment of whatever token the clip landed inside. + match tail.split_once('_') { + Some((_, rest)) if !rest.is_empty() => rest.to_string(), + _ => tail, + } +} + fn planner_provider_messages(plan_messages: &Conversation) -> Conversation { // The planner prompt has no turn-context instructions; drop the blocks. let projected_messages: Vec = plan_messages @@ -337,9 +390,14 @@ impl CliSession { } /// Parse a stdio extension command string into an ExtensionConfig - /// Format: "ENV1=val1 ENV2=val2 command args..." + /// Format: "[name:]ENV1=val1 ENV2=val2 command args..." + /// + /// Without the optional `name:` prefix the extension is named after the + /// basename of the command, which is the launcher rather than the server + /// whenever one is used (`npx`, `python -m ...`, `uvx`, ...). pub fn parse_stdio_extension(extension_command: &str) -> Result { - let mut parts = goose::utils::split_command_args(extension_command)?; + let (explicit_name, command) = split_extension_name_prefix(extension_command); + let mut parts = goose::utils::split_command_args(command)?; let mut envs = HashMap::new(); while let Some(part) = parts.first() { @@ -356,11 +414,13 @@ impl CliSession { } let cmd = parts.remove(0); - let name = std::path::Path::new(&cmd) - .file_name() - .and_then(|f| f.to_str()) - .unwrap_or("unnamed") - .to_string(); + let name = explicit_name.unwrap_or_else(|| { + std::path::Path::new(&cmd) + .file_name() + .and_then(|f| f.to_str()) + .unwrap_or("unnamed") + .to_string() + }); Ok(ExtensionConfig::Stdio { name, @@ -2963,6 +3023,69 @@ mod tests { assert!(CliSession::parse_stdio_extension("").is_err()); } + fn stdio_name(input: &str) -> String { + CliSession::parse_stdio_extension(input).unwrap().name() + } + + #[test] + fn test_parse_stdio_extension_explicit_name() { + assert_eq!(stdio_name("word:python -m word_mcp"), "word"); + assert_eq!(stdio_name("Word-One:python -m word_mcp"), "word-one"); + let absolute = CliSession::parse_stdio_extension("memory:/usr/local/bin/mcp").unwrap(); + assert_eq!(absolute.name(), "memory"); + assert!(matches!( + absolute, + ExtensionConfig::Stdio { cmd, .. } if cmd == "/usr/local/bin/mcp" + )); + let config = CliSession::parse_stdio_extension("memory:API_KEY=k npx -y srv").unwrap(); + let ExtensionConfig::Stdio { + name, + cmd, + args, + envs, + .. + } = config + else { + panic!("expected a stdio extension"); + }; + assert_eq!(name, "memory"); + assert_eq!(cmd, "npx"); + assert_eq!(args, vec!["-y".to_string(), "srv".to_string()]); + assert_eq!(envs.get_env().get("API_KEY").map(String::as_str), Some("k")); + } + + #[test_case("C:\\Program Files\\srv.exe --stdio" ; "windows_drive_letter")] + #[test_case("srv://not-a-name" ; "url_like_command")] + #[test_case("npx -y pkg:latest" ; "colon_after_a_space")] + #[test_case("word:" ; "empty_command")] + fn test_split_extension_name_prefix_leaves_command_alone(input: &str) { + let (name, rest) = split_extension_name_prefix(input); + assert_eq!(name, None); + assert_eq!(rest, input); + } + + #[test] + fn test_derive_extension_name_from_command() { + assert_eq!( + derive_extension_name_from_command("python", &["-m".into(), "word_mcp".into()]), + "python_m_word_mcp" + ); + assert_eq!( + derive_extension_name_from_command( + "npx", + &[ + "-y".into(), + "@modelcontextprotocol/server-filesystem".into() + ], + ), + "server-filesystem" + ); + assert_eq!( + derive_extension_name_from_command("/usr/local/bin/srv", &[]), + "srv" + ); + } + #[test] fn test_build_switched_model_config_rebuilds_target_model_settings() { let _guard = env_lock::lock_env([ diff --git a/documentation/docs/guides/goose-cli-commands.md b/documentation/docs/guides/goose-cli-commands.md index 05f8a79e7..c665eefd1 100644 --- a/documentation/docs/guides/goose-cli-commands.md +++ b/documentation/docs/guides/goose-cli-commands.md @@ -215,7 +215,7 @@ Start or resume interactive chat sessions. - **`--max-turns `**: Set the maximum number of turns allowed without user input (default: 1000) **Extension Options:** -- **`--with-extension `**: Add stdio extensions +- **`--with-extension `**: Add stdio extensions. Format: `[name:]ENV1=val1 command args...`. Without the optional `name:`, the extension is named after the command — which is the launcher, not the server, for anything started through one (`npx`, `python -m ...`, `uvx`). That name prefixes every tool the server exposes (`npx__search`), and extensions started by the same launcher would otherwise collide, so goose names colliding ones after their full command line instead. Give an explicit name to control it: `--with-extension "memory:npx -y @modelcontextprotocol/server-memory"` exposes `memory__search`. - **`--with-streamable-http-extension `**: Add remote extensions over Streamable HTTP - **`--with-builtin `**: Enable built-in extensions (e.g., 'developer', 'computercontroller') @@ -244,6 +244,9 @@ goose session --resume --session-id 20251108_2 --fork --edit --history # Start with extensions goose session --with-extension "npx -y @modelcontextprotocol/server-memory" + +# Name an extension explicitly (tools become memory__*, not npx__*) +goose session --with-extension "memory:npx -y @modelcontextprotocol/server-memory" goose session --with-builtin developer goose session --with-streamable-http-extension "http://localhost:8080/mcp"