diff --git a/docs/user/agent-plugins.md b/docs/user/agent-plugins.md index d28eb32..59ed640 100644 --- a/docs/user/agent-plugins.md +++ b/docs/user/agent-plugins.md @@ -51,7 +51,7 @@ Kit does not run repository hooks or filters, fetch Git LFS content, or initiali ## Plugin MCP servers -Kit supports the Agent Plugin `stdio` and `streamable-http` transports. The deprecated `sse` transport is not supported. Kit rejects a plugin generation containing an SSE declaration instead of publishing only part of that generation. +Kit supports the Agent Plugin `stdio` and `streamable-http` transports. The deprecated `sse` transport is not supported. Kit skips an SSE declaration, or a declaration with an unsupported `cwd`, a `./` command outside the plugin root, or an invalid header, and still loads the plugin's other servers. For `stdio`, Kit materializes the validated portable declaration as follows: @@ -69,7 +69,7 @@ The location follows the loaded Kit configuration directory, so a config loaded Invoke plugin MCP tools through Kit's `tool` dispatcher, using names returned by `tool_search`. Exact `mcp_*` names are not direct compose callables. -Supported MCP server names must be unique across plugins. If two plugins declare the same supported server name, startup fails and identifies both aliases. A same-named entry in configured, project-local, or command-line MCP JSON intentionally overrides the plugin server. Kit live-reloads every named file before `tool_search` and `auth`; changing an override replaces it, and removing it restores the next lower configured or plugin server without restarting Kit. An invalid file edit fails the current call and preserves the last valid combined configuration. +If two plugins declare the same MCP server name, the plugin with the lexically first alias keeps it and later declarations are skipped. A skipped invalid declaration still reserves its name, so a later plugin cannot take the server over. A same-named entry in configured, project-local, or command-line MCP JSON intentionally overrides the plugin server. Kit live-reloads every named file before `tool_search` and `auth`; changing an override replaces it, and removing it restores the next lower configured or plugin server without restarting Kit. An invalid file edit fails the current call and preserves the last valid combined configuration. ## Cache and live reload behavior @@ -91,7 +91,7 @@ The cache is local state protected by the permissions of `~/.kit`; it is not a s Resolution or package-validation failures stop startup. In a live session, Kit rereads the exact Kit config file used at startup and re-resolves path, archive, and Git sources before `tool_search`, `auth`, and each ACP v1 or v2 user-prompt boundary. Added, changed, and removed plugin MCP servers are settled before `tool_search` returns. Plugin skill additions, skill-content or resource-inventory changes, and removals are visible to the live `skill` tool; ACP sessions announce catalog changes at the next user-prompt boundary. Reload does not create an unsolicited model turn. -If a live reload fails, fix the reported configuration or package error and retry the triggering tool call or prompt. The previous plugin MCP servers and skills remain active until a complete valid replacement is available. A package with a skipped or disabled skill or MCP component can reject the entire reload; do not assume the remaining components were installed. +If a live reload fails, fix the reported configuration or package error and retry the triggering tool call or prompt. The previous plugin MCP servers and skills remain active until a complete valid replacement is available. Invalid skills, skills directories, `mcp.json` files, and MCP server declarations are skipped individually while the rest of the package loads. Package-validation failures, such as an invalid `plugin.json`, and plugin files or data directories that are not available on disk reject the reload and keep the previous generation. Skill collision precedence remains project skills, then user skills, then plugins in lexical alias order. Only immediate valid plugin skill directories approved by the package validator are exposed; nested `SKILL.md` files are not recursively added. Skill instructions and resource paths come from a read-only snapshot, so editing a package does not change an already loaded skill in place. diff --git a/docs/user/mcp.md b/docs/user/mcp.md index e7964a4..0ca397d 100644 --- a/docs/user/mcp.md +++ b/docs/user/mcp.md @@ -76,7 +76,7 @@ Files are not the only installation route. An Agent Plugin can contribute MCP se ## Agent Plugin MCP configuration -Validated Agent Plugins can contribute `stdio` and `streamable-http` servers. Deprecated `sse` servers are skipped with a stderr diagnostic naming the plugin and server. Supported server names must be unique across plugins; a collision stops startup and identifies both plugin aliases. +Validated Agent Plugins can contribute `stdio` and `streamable-http` servers. Deprecated `sse` servers and other invalid plugin server declarations are skipped, and the plugin's remaining servers still load. When plugins collide on a server name, the lexically first plugin alias keeps the server and later declarations are skipped. Plugin stdio declarations use the canonical plugin package as `PLUGIN_ROOT` and a persistent `/plugin-data/` directory as `PLUGIN_DATA`. Kit injects both environment variables and replaces every occurrence of `${PLUGIN_ROOT}` and `${PLUGIN_DATA}` in stdio arguments and plugin-supplied environment values. A `./` command is resolved beneath the plugin root. An omitted `cwd` leaves the transport default unchanged and inherits Kit's working directory; `./...` is plugin-root-relative; and `${PLUGIN_ROOT}` or `${PLUGIN_DATA}`, optionally with a validated contained suffix, selects that directory. Kit creates missing data-rooted working directories. These stdio placeholders are not expanded in streamable HTTP URLs or headers. @@ -382,8 +382,7 @@ Persistent stores restore OpenAI credentials when Kit starts and restore MCP OAu - **`could not read MCP config ...`**: verify the configured `mcp_config` or `--mcp-config` path and permissions; named files are required. Root `.mcp.json` is optional; creating it takes effect on the next `tool_search` or `auth` call. - **`invalid MCP config ...`**: validate JSON syntax, the exact `mcpServers` spelling, field types, and field names such as `bearerToken`, `clientId`, and `clientMetadataUrl`. Unknown fields are rejected. - **`MCP server names must not be empty`**, **`has an empty command`**, or **`has an empty URL`**: give every entry a non-blank name and its transport a non-blank `command` or `url`. -- **`MCP server ... is declared by both plugins ...`**: rename one plugin server or disable one of the colliding plugins. Explicit-file entries may override plugin servers, but plugin/plugin collisions are errors. -- **Plugin SSE skip diagnostic**: migrate that declaration to `streamable-http`; SSE is deprecated and Kit intentionally leaves that server unavailable. +- **Plugin server missing**: an SSE declaration, an unsupported `cwd`, a `./` command outside the plugin root, or a name already declared by a lexically earlier plugin alias leaves that server unavailable. Migrate SSE to `streamable-http` and fix or rename the declaration. - **MCP server status `error`**: Kit tolerates initialization failures—startup continues and the per-server diagnostic is included in `tool_search`. Run a failing stdio command directly to check that it exists and speaks MCP over stdio; for HTTP, check DNS, HTTPS, proxy/firewall access, authentication settings, headers, and the endpoint URL. The connection timeout is 20 seconds. ### OAuth and authentication errors diff --git a/docs/user/security-limits-and-troubleshooting.md b/docs/user/security-limits-and-troubleshooting.md index 7685005..fc75ddc 100644 --- a/docs/user/security-limits-and-troubleshooting.md +++ b/docs/user/security-limits-and-troubleshooting.md @@ -129,7 +129,7 @@ Provider context windows, model token limits, child-agent turn limits, remote ra ### MCP tools are missing or authentication fails -1. Confirm that the server comes from a configured, valid Agent Plugin, configured `mcp_config`, root `.mcp.json`, or `--mcp-config`. Plugin-only operation needs no file; project discovery checks only the canonical runtime root. An SSE plugin server is skipped with a diagnostic; use `streamable-http` instead. A duplicate supported server name across two plugins stops startup, while an explicit same-named entry intentionally overrides a plugin server. +1. Confirm that the server comes from a configured, valid Agent Plugin, configured `mcp_config`, root `.mcp.json`, or `--mcp-config`. Plugin-only operation needs no file; project discovery checks only the canonical runtime root. An SSE or otherwise invalid plugin server declaration is skipped; use `streamable-http` and check its `command` and `cwd`. When two plugins declare the same server name, only the lexically first plugin alias provides it, while an explicit same-named entry intentionally overrides a plugin server. 2. Search with `tool_search`; it waits for background server initialization to settle, and the exact query `mcp` compactly lists configured servers and reports any cap-driven tail omission. Inspect statuses: `authenticated`, `authentication_required`, `pending`, or `error`. An `error` result includes the initialization diagnostic. Invoke only tool names returned by the search. 3. For `authentication_required`, call `auth` with the exact server name in a `kit tui`, `kit serve`, or `kit acp` session, open its URL, and complete the browser flow within 10 minutes. Kit connects the server and resumes the originating ACP session automatically. If a configured `bearerToken` or `Authorization` header is rejected, Kit reports an error instead and does not replace that static credential with inferred OAuth; update or remove it first. 4. In one-shot `kit prompt`, use a long-lived command to authenticate or configure persistent credentials there for later one-shot use. diff --git a/src/plugins.rs b/src/plugins.rs index 2369d0e..c11c11c 100644 --- a/src/plugins.rs +++ b/src/plugins.rs @@ -573,7 +573,7 @@ fn resolve_blocking( plugin.manifest().name )); } - validate_plugin_diagnostics(alias, &plugin)?; + log_plugin_diagnostics(alias, &plugin); if !plugin.mcp_servers().is_empty() { let data_dir = data_root.join(&plugin.manifest().name); fs::create_dir_all(&data_dir).map_err(|error| { @@ -622,7 +622,6 @@ fn resolve_blocking( root.display() ) })?; - validate_plugin_diagnostics(&alias, &plugin)?; if plugin_semantic_key(&plugin) != expected { return Err(format!( "plugin {alias:?} changed while its generation was being staged" @@ -1321,7 +1320,6 @@ fn validate_skill_snapshot( package.display() ) })?; - validate_plugin_diagnostics(&index.to_string(), &plugin)?; if !plugin.skill_directories().is_empty() { package_roots.push(package.clone()); } @@ -1404,22 +1402,19 @@ fn require_skill_directories(directories: &[PathBuf]) -> Result<(), String> { Ok(()) } -fn validate_plugin_diagnostics(alias: &str, plugin: &AgentPlugin) -> Result<(), String> { +fn log_plugin_diagnostics(alias: &str, plugin: &AgentPlugin) { for diagnostic in plugin.diagnostics() { if matches!(diagnostic.kind, PluginDiagnosticKind::UnknownManifestField) { continue; } - let path = diagnostic - .path - .as_deref() - .map(|path| format!(" at {}", path.display())) - .unwrap_or_default(); - return Err(format!( - "plugin {alias} ({:?}){path}: {}", - diagnostic.kind, diagnostic.message - )); + tracing::warn!( + plugin = alias, + kind = ?diagnostic.kind, + path = ?diagnostic.path, + "{}", + diagnostic.message + ); } - Ok(()) } fn validate_alias(alias: &str) -> Result<(), String> { @@ -4130,6 +4125,50 @@ mod tests { assert!(runtime.snapshot().skill_directories.is_empty()); } + #[tokio::test] + async fn nonconforming_skill_is_skipped_without_failing_plugin() { + let directory = tempfile::tempdir().unwrap(); + let config = directory.path().join("config.toml"); + let package = directory.path().join("plugin"); + let valid = package.join("skills/live-skill"); + let invalid = package.join("skills/broken-skill"); + fs::create_dir_all(&valid).unwrap(); + fs::create_dir_all(&invalid).unwrap(); + fs::write(package.join("plugin.json"), MANIFEST).unwrap(); + fs::write( + valid.join("SKILL.md"), + "---\nname: live-skill\ndescription: Live skill.\n---\nbody\n", + ) + .unwrap(); + fs::write( + invalid.join("SKILL.md"), + "---\nname: other-name\ndescription: Mismatched.\n---\nbody\n", + ) + .unwrap(); + fs::write( + &config, + format!( + "[plugins.live]\nsource = 'path'\npath = '{}'\n", + package.display() + ), + ) + .unwrap(); + let runtime = PluginRuntime::new( + config, + directory.path().to_path_buf(), + directory.path().join("cache"), + directory.path().join("data"), + ResolvedPlugins::default(), + ); + + let staged = runtime.stage().await.unwrap(); + + assert_eq!(staged.resolved.skills.len(), 1); + assert_eq!(staged.resolved.skills[0].body, "body"); + assert_eq!(staged.resolved.skill_directories.len(), 1); + assert!(staged.resolved.skill_directories[0].ends_with("skills/live-skill")); + } + #[tokio::test] async fn live_runtime_stages_add_remove_and_retains_last_valid_generation() { let directory = tempfile::tempdir().unwrap(); @@ -4176,11 +4215,7 @@ mod tests { let first_skill = runtime.snapshot().skill_directories[0].join("SKILL.md"); assert!(fs::read_to_string(&first_skill).unwrap().contains("body")); - write!( - fs::File::create(package.join("mcp.json")).unwrap(), - "{{\"$schema\":\"https://agent-plugins.org/schemas/1.0.0/mcp.schema.json\",\"mcpServers\":{{}},\"extra\":true}}" - ) - .unwrap(); + fs::write(package.join("plugin.json"), "{").unwrap(); fs::write( skill.join("SKILL.md"), "---\nname: live-skill\ndescription: Changed skill.\n---\nchanged body\n", @@ -4197,7 +4232,7 @@ mod tests { .contains("changed body") ); - fs::remove_file(package.join("mcp.json")).unwrap(); + fs::write(package.join("plugin.json"), MANIFEST).unwrap(); mcp.refresh().await.unwrap(); assert_ne!( runtime.snapshot().skill_directories[0].join("SKILL.md"), diff --git a/src/tools/mcp.rs b/src/tools/mcp.rs index 3dd9b5b..605febc 100644 --- a/src/tools/mcp.rs +++ b/src/tools/mcp.rs @@ -35,7 +35,7 @@ use agentkit_mcp::{ McpServerConfig, McpServerId, McpServerManager, McpServerOptions, McpTransportBinding, StdioTransportConfig, StreamableHttpTransportConfig, }; -use agentkit_plugins::PluginMcpTransport; +use agentkit_plugins::{PluginMcpServer, PluginMcpTransport}; use agentkit_tools_core::{ BasicToolExecutor, CatalogReader, Tool, ToolContext, ToolError, ToolExecutionOutcome, ToolExecutionScope, ToolExecutor, ToolName, ToolRequest, ToolResult, ToolSource, ToolSpec, @@ -1309,38 +1309,64 @@ fn expand_plugin_value(value: &str, root: &str, data: &str) -> String { output } -fn contained_plugin_path(path: PathBuf, root: &Path, context: &str) -> Result { +enum PluginServerError { + Invalid(String), + Unavailable(String), +} + +impl PluginServerError { + fn from_io(error: &std::io::Error, message: String) -> Self { + match error.kind() { + std::io::ErrorKind::NotADirectory => Self::Invalid(message), + _ => Self::Unavailable(message), + } + } +} + +fn contained_plugin_path( + path: PathBuf, + root: &Path, + context: &str, +) -> Result { let mut ancestor = path.clone(); let mut suffix = Vec::new(); loop { match crate::resilient_fs::symlink_metadata(&ancestor) { Ok(_) => break, Err(error) if error.kind() == std::io::ErrorKind::NotFound => { - let component = ancestor - .file_name() - .ok_or_else(|| format!("plugin MCP {context} path has no existing ancestor"))?; + let component = ancestor.file_name().ok_or_else(|| { + PluginServerError::Invalid(format!( + "plugin MCP {context} path has no existing ancestor" + )) + })?; suffix.push(component.to_os_string()); ancestor.pop(); } Err(error) => { - return Err(format!( - "could not inspect plugin MCP {context} path {}: {error}", - ancestor.display() + return Err(PluginServerError::from_io( + &error, + format!( + "could not inspect plugin MCP {context} path {}: {error}", + ancestor.display() + ), )); } } } let mut resolved = crate::resilient_fs::canonicalize(&ancestor).map_err(|error| { - format!( - "could not resolve plugin MCP {context} path {}: {error}", - ancestor.display() + PluginServerError::from_io( + &error, + format!( + "could not resolve plugin MCP {context} path {}: {error}", + ancestor.display() + ), ) })?; if !resolved.starts_with(root) { - return Err(format!( + return Err(PluginServerError::Invalid(format!( "plugin MCP {context} path resolves outside {}", root.display() - )); + ))); } for component in suffix.into_iter().rev() { resolved.push(component); @@ -1358,150 +1384,179 @@ fn prepare_plugins( let root = plugin_text(&plugin.root, "root", &plugin.alias)?; let data = plugin_text(&plugin.data_dir, "data", &plugin.alias)?; for server in &plugin.servers { - if let Some(owner) = owners.insert(server.name.clone(), plugin.alias.clone()) { - return Err(format!( - "MCP server {:?} is declared by both plugins {owner:?} and {:?}", - server.name, plugin.alias - )); + if let Some(owner) = owners.get(&server.name) { + tracing::warn!( + server = %server.name, + plugin = %plugin.alias, + owner = %owner, + "skipping plugin MCP server already declared by another plugin" + ); + continue; } - let description = format!("{} plugin MCP server", plugin.manifest_name); - let (binding, url, static_authorization) = match &server.transport { - PluginMcpTransport::Stdio { - command, - args, - env, - cwd, - } => { - let command = if let Some(command) = command.strip_prefix("./") { - let command = contained_plugin_path( - plugin.root.join(command), - &plugin.root, - "command", - )?; - plugin_text(&command, "command", &plugin.alias)? - } else { - command.clone() - }; - let mut transport = StdioTransportConfig::new(command); - transport.args = args - .iter() - .map(|value| expand_plugin_value(value, &root, &data)) - .collect(); - transport.env = env - .iter() - .map(|(name, value)| { - (name.clone(), expand_plugin_value(value, &root, &data)) - }) - .chain([ - ("PLUGIN_ROOT".to_owned(), root.clone()), - ("PLUGIN_DATA".to_owned(), data.clone()), - ]) - .collect(); - transport.cwd = match cwd.as_deref() { - Some("${PLUGIN_ROOT}") => Some(plugin.root.clone()), - Some("${PLUGIN_DATA}") => Some(plugin.data_dir.clone()), - Some(cwd) if cwd.starts_with("${PLUGIN_ROOT}/") => { - Some(contained_plugin_path( - plugin.root.join(&cwd["${PLUGIN_ROOT}/".len()..]), - &plugin.root, - "cwd", - )?) - } - Some(cwd) if cwd.starts_with("${PLUGIN_DATA}/") => { - let directory = contained_plugin_path( - plugin.data_dir.join(&cwd["${PLUGIN_DATA}/".len()..]), - &plugin.data_dir, - "data cwd", - )?; - crate::resilient_fs::create_dir_all(&directory).map_err(|error| { - format!( - "could not create cwd for plugin {:?} MCP server {:?} at {}: {error}", - plugin.alias, server.name, directory.display() - ) - })?; - Some(crate::resilient_fs::canonicalize(&directory).map_err(|error| { - format!( - "could not resolve cwd for plugin {:?} MCP server {:?}: {error}", - plugin.alias, server.name - ) - })?) - } - Some(cwd) if cwd.starts_with("./") => Some(contained_plugin_path( - plugin.root.join(&cwd[2..]), - &plugin.root, - "cwd", - )?), - Some(cwd) => { - return Err(format!( - "plugin {:?} MCP server {:?} has unsupported cwd {cwd:?}", - plugin.alias, server.name - )); - } - None => None, - }; - crate::resilient_fs::global() - .require_disk(&plugin.root) - .map_err(|error| { - format!("plugin MCP files are not available on disk: {error}") - })?; - crate::resilient_fs::global() - .require_disk(&plugin.data_dir) - .map_err(|error| { - format!("plugin MCP data directory is not available on disk: {error}") - })?; - (McpTransportBinding::Stdio(transport), None, false) + owners.insert(server.name.clone(), plugin.alias.clone()); + match prepare_plugin_server(plugin, server, &root, &data) { + Ok(server_prepared) => { + entries.insert( + server.name.clone(), + server_prepared.record.fingerprint.clone(), + ); + prepared.insert(server.name.clone(), server_prepared); } - PluginMcpTransport::StreamableHttp { url, headers } => { - let mut transport = StreamableHttpTransportConfig::new(url.clone()); - for (name, value) in headers { - transport = transport - .with_header(name.as_str(), value.as_str()) - .map_err(|error| { - format!("invalid plugin MCP config for {}: {error}", server.name) - })?; - } - ( - McpTransportBinding::StreamableHttp(transport), - Some(url.clone()), - headers - .keys() - .any(|name| name.eq_ignore_ascii_case("authorization")), + Err(PluginServerError::Invalid(error)) => tracing::warn!( + server = %server.name, + plugin = %plugin.alias, + %error, + "skipping plugin MCP server" + ), + Err(PluginServerError::Unavailable(error)) => return Err(error), + } + } + } + Ok((prepared, entries)) +} + +fn prepare_plugin_server( + plugin: &crate::plugins::ResolvedPluginMcp, + server: &PluginMcpServer, + root: &str, + data: &str, +) -> Result { + let description = format!("{} plugin MCP server", plugin.manifest_name); + let (binding, url, static_authorization) = match &server.transport { + PluginMcpTransport::Stdio { + command, + args, + env, + cwd, + } => { + let command = if let Some(command) = command.strip_prefix("./") { + let command = + contained_plugin_path(plugin.root.join(command), &plugin.root, "command")?; + plugin_text(&command, "command", &plugin.alias) + .map_err(PluginServerError::Invalid)? + } else { + command.clone() + }; + let mut transport = StdioTransportConfig::new(command); + transport.args = args + .iter() + .map(|value| expand_plugin_value(value, root, data)) + .collect(); + transport.env = env + .iter() + .map(|(name, value)| (name.clone(), expand_plugin_value(value, root, data))) + .chain([ + ("PLUGIN_ROOT".to_owned(), root.to_owned()), + ("PLUGIN_DATA".to_owned(), data.to_owned()), + ]) + .collect(); + transport.cwd = match cwd.as_deref() { + Some("${PLUGIN_ROOT}") => Some(plugin.root.clone()), + Some("${PLUGIN_DATA}") => Some(plugin.data_dir.clone()), + Some(cwd) if cwd.starts_with("${PLUGIN_ROOT}/") => Some(contained_plugin_path( + plugin.root.join(&cwd["${PLUGIN_ROOT}/".len()..]), + &plugin.root, + "cwd", + )?), + Some(cwd) if cwd.starts_with("${PLUGIN_DATA}/") => { + let directory = contained_plugin_path( + plugin.data_dir.join(&cwd["${PLUGIN_DATA}/".len()..]), + &plugin.data_dir, + "data cwd", + )?; + crate::resilient_fs::create_dir_all(&directory).map_err(|error| { + PluginServerError::Unavailable(format!( + "could not create cwd for plugin {:?} MCP server {:?} at {}: {error}", + plugin.alias, + server.name, + directory.display() + )) + })?; + Some( + crate::resilient_fs::canonicalize(&directory).map_err(|error| { + PluginServerError::Unavailable(format!( + "could not resolve cwd for plugin {:?} MCP server {:?}: {error}", + plugin.alias, server.name + )) + })?, ) } - PluginMcpTransport::Sse { .. } => { - return Err(format!( - "plugin {}: MCP server {:?} uses unsupported SSE transport", + Some(cwd) if cwd.starts_with("./") => Some(contained_plugin_path( + plugin.root.join(&cwd[2..]), + &plugin.root, + "cwd", + )?), + Some(cwd) => { + return Err(PluginServerError::Invalid(format!( + "plugin {:?} MCP server {:?} has unsupported cwd {cwd:?}", plugin.alias, server.name - )); + ))); } + None => None, }; - let fingerprint = format!( - "plugin:v2:{:?}:{:?}:{:?}:{:?}:{binding:?}:{url:?}:{static_authorization}", - plugin.alias, plugin.manifest_name, plugin.root, plugin.data_dir + crate::resilient_fs::global() + .require_disk(&plugin.root) + .map_err(|error| { + PluginServerError::Unavailable(format!( + "plugin MCP files are not available on disk: {error}" + )) + })?; + crate::resilient_fs::global() + .require_disk(&plugin.data_dir) + .map_err(|error| { + PluginServerError::Unavailable(format!( + "plugin MCP data directory is not available on disk: {error}" + )) + })?; + (McpTransportBinding::Stdio(transport), None, false) + } + PluginMcpTransport::StreamableHttp { url, headers } => { + let mut transport = StreamableHttpTransportConfig::new(url.clone()); + for (name, value) in headers { + transport = transport + .with_header(name.as_str(), value.as_str()) + .map_err(|error| { + PluginServerError::Invalid(format!( + "invalid plugin MCP config for {}: {error}", + server.name + )) + })?; + } + ( + McpTransportBinding::StreamableHttp(transport), + Some(url.clone()), + headers + .keys() + .any(|name| name.eq_ignore_ascii_case("authorization")), ) - .into_bytes(); - entries.insert(server.name.clone(), fingerprint.clone()); - prepared.insert( - server.name.clone(), - PreparedServer { - config: McpServerConfig::new(server.name.clone(), binding), - record: ServerRecord { - description, - url, - oauth: None, - static_authorization, - fingerprint, - plugin_owned: true, - catalog_stale: false, - catalog_error: None, - subscription: None, - status: ServerStatus::Uninitialized, - }, - }, - ); } - } - Ok((prepared, entries)) + PluginMcpTransport::Sse { .. } => { + return Err(PluginServerError::Invalid(format!( + "plugin {}: MCP server {:?} uses unsupported SSE transport", + plugin.alias, server.name + ))); + } + }; + let fingerprint = format!( + "plugin:v2:{:?}:{:?}:{:?}:{:?}:{binding:?}:{url:?}:{static_authorization}", + plugin.alias, plugin.manifest_name, plugin.root, plugin.data_dir + ) + .into_bytes(); + Ok(PreparedServer { + config: McpServerConfig::new(server.name.clone(), binding), + record: ServerRecord { + description, + url, + oauth: None, + static_authorization, + fingerprint, + plugin_owned: true, + catalog_stale: false, + catalog_error: None, + subscription: None, + status: ServerStatus::Uninitialized, + }, + }) } async fn read_source(source: &ConfigSource) -> Result>, String> { @@ -7248,20 +7303,31 @@ mod tests { } #[test] - fn plugin_mcp_rejects_sse_without_partial_publication() { + fn plugin_mcp_skips_sse_and_keeps_sibling_servers() { let directory = tempfile::tempdir().unwrap(); let plugin = plugin( "tools", directory.path(), - vec![PluginMcpServer { - name: "legacy".into(), - transport: PluginMcpTransport::Sse { - url: "https://example.com/sse".into(), - headers: Default::default(), + vec![ + PluginMcpServer { + name: "legacy".into(), + transport: PluginMcpTransport::Sse { + url: "https://example.com/sse".into(), + headers: Default::default(), + }, }, - }], + PluginMcpServer { + name: "remote".into(), + transport: PluginMcpTransport::StreamableHttp { + url: "https://example.com/mcp".into(), + headers: Default::default(), + }, + }, + ], ); - assert!(prepare_plugins(&[plugin]).is_err()); + let (prepared, entries) = prepare_plugins(&[plugin]).unwrap(); + assert_eq!(prepared.keys().collect::>(), ["remote"]); + assert_eq!(entries.keys().collect::>(), ["remote"]); } #[test] @@ -7314,7 +7380,7 @@ mod tests { } #[test] - fn duplicate_plugin_mcp_names_are_rejected() { + fn duplicate_plugin_mcp_names_keep_first_plugin() { let first = tempfile::tempdir().unwrap(); let second = tempfile::tempdir().unwrap(); let server = || PluginMcpServer { @@ -7324,17 +7390,107 @@ mod tests { headers: Default::default(), }, }; - let error = match prepare_plugins(&[ + let (prepared, _) = prepare_plugins(&[ plugin("first", first.path(), vec![server()]), plugin("second", second.path(), vec![server()]), - ]) { - Ok(_) => panic!("duplicate plugin MCP names were accepted"), - Err(error) => error, + ]) + .unwrap(); + assert_eq!(prepared.len(), 1); + assert_eq!( + prepared["shared"].record.description, + "first-manifest plugin MCP server" + ); + } + + #[test] + fn invalid_first_plugin_mcp_declaration_keeps_name_reserved() { + let first = tempfile::tempdir().unwrap(); + let second = tempfile::tempdir().unwrap(); + let (prepared, entries) = prepare_plugins(&[ + plugin( + "first", + first.path(), + vec![PluginMcpServer { + name: "shared".into(), + transport: PluginMcpTransport::Stdio { + command: "server".into(), + args: Vec::new(), + env: Default::default(), + cwd: Some("/elsewhere".into()), + }, + }], + ), + plugin( + "second", + second.path(), + vec![PluginMcpServer { + name: "shared".into(), + transport: PluginMcpTransport::StreamableHttp { + url: "https://example.com/mcp".into(), + headers: Default::default(), + }, + }], + ), + ]) + .unwrap(); + assert!(!prepared.contains_key("shared")); + assert!(!entries.contains_key("shared")); + } + + #[test] + fn plugin_mcp_skips_file_ancestor_paths_and_keeps_siblings() { + let directory = tempfile::tempdir().unwrap(); + std::fs::write(directory.path().join("bin"), "not a directory").unwrap(); + let stdio = |name: &str, command: &str, cwd: Option<&str>| PluginMcpServer { + name: name.into(), + transport: PluginMcpTransport::Stdio { + command: command.into(), + args: Vec::new(), + env: Default::default(), + cwd: cwd.map(Into::into), + }, }; - assert!( - error.contains("both plugins \"first\" and \"second\""), - "{error}" + let plugin = plugin( + "tools", + directory.path(), + vec![ + stdio("bad-cwd", "server", Some("${PLUGIN_ROOT}/bin/work")), + stdio("bad-command", "./bin/run", None), + PluginMcpServer { + name: "sibling".into(), + transport: PluginMcpTransport::StreamableHttp { + url: "https://example.com/mcp".into(), + headers: Default::default(), + }, + }, + ], + ); + let (prepared, _) = prepare_plugins(&[plugin]).unwrap(); + assert_eq!(prepared.keys().collect::>(), ["sibling"]); + } + + #[cfg(unix)] + #[test] + fn unavailable_plugin_mcp_data_cwd_fails_preparation() { + let directory = tempfile::tempdir().unwrap(); + let plugin = plugin( + "tools", + directory.path(), + vec![PluginMcpServer { + name: "local".into(), + transport: PluginMcpTransport::Stdio { + command: "server".into(), + args: Vec::new(), + env: Default::default(), + cwd: Some("${PLUGIN_DATA}/work".into()), + }, + }], ); + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&plugin.data_dir, std::fs::Permissions::from_mode(0o555)).unwrap(); + let result = prepare_plugins(std::slice::from_ref(&plugin)); + std::fs::set_permissions(&plugin.data_dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + assert!(result.is_err()); } #[tokio::test]