From e1aa3ededdfd272c5e1e0d6503c0a2cc37726427 Mon Sep 17 00:00:00 2001 From: daniel Date: Sun, 25 Oct 2026 10:57:10 +0000 Subject: [PATCH 1/3] fix(plugins): skip invalid plugin components instead of failing Plugin diagnostics and per-server MCP preparation errors now log a warning and skip the affected skill or server. Duplicate plugin MCP server names keep the first plugin in alias order. --- src/plugins.rs | 75 ++++++++--- src/tools/mcp.rs | 339 +++++++++++++++++++++++++---------------------- 2 files changed, 237 insertions(+), 177 deletions(-) diff --git a/src/plugins.rs b/src/plugins.rs index 2369d0ea..c11c11c3 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 3dd9b5bc..531d28f8 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, @@ -1358,152 +1358,167 @@ 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) - } - 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")), - ) - } - PluginMcpTransport::Sse { .. } => { - return Err(format!( - "plugin {}: MCP server {:?} uses unsupported SSE transport", - plugin.alias, server.name - )); + match prepare_plugin_server(plugin, server, &root, &data) { + Ok(server_prepared) => { + owners.insert(server.name.clone(), plugin.alias.clone()); + entries.insert( + server.name.clone(), + server_prepared.record.fingerprint.clone(), + ); + prepared.insert(server.name.clone(), server_prepared); } - }; - let fingerprint = format!( - "plugin:v2:{:?}:{:?}:{:?}:{:?}:{binding:?}:{url:?}:{static_authorization}", - plugin.alias, plugin.manifest_name, plugin.root, plugin.data_dir - ) - .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, - }, - }, - ); + Err(error) => tracing::warn!( + server = %server.name, + plugin = %plugin.alias, + %error, + "skipping plugin MCP server" + ), + } } } 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)? + } 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| { + 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) + } + 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")), + ) + } + PluginMcpTransport::Sse { .. } => { + return Err(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> { if let Some(bytes) = &source.session_bytes { return Ok(Some(bytes.to_vec())); @@ -7248,20 +7263,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 +7340,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,16 +7350,15 @@ 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, - }; - assert!( - error.contains("both plugins \"first\" and \"second\""), - "{error}" + ]) + .unwrap(); + assert_eq!(prepared.len(), 1); + assert_eq!( + prepared["shared"].record.description, + "first-manifest plugin MCP server" ); } From 78fc7e996bdd6159a8f1d974cd378e36210d3f05 Mon Sep 17 00:00:00 2001 From: daniel Date: Sun, 25 Oct 2026 11:27:41 +0000 Subject: [PATCH 2/3] fix(mcp): keep disk failures fatal and reserve plugin server names Unavailable plugin files or data directories still fail preparation instead of silently removing a server. A plugin MCP name is reserved before preparation so an invalid first declaration cannot transfer ownership to a later plugin. Docs describe component-level skipping. --- docs/user/agent-plugins.md | 6 +- docs/user/mcp.md | 5 +- .../security-limits-and-troubleshooting.md | 2 +- src/tools/mcp.rs | 198 +++++++++++++----- 4 files changed, 146 insertions(+), 65 deletions(-) diff --git a/docs/user/agent-plugins.md b/docs/user/agent-plugins.md index d28eb320..59ed6406 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 e7964a46..0ca397da 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 76850055..fc75ddc0 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/tools/mcp.rs b/src/tools/mcp.rs index 531d28f8..fc72f5ca 100644 --- a/src/tools/mcp.rs +++ b/src/tools/mcp.rs @@ -1309,38 +1309,49 @@ 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), +} + +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!( + return Err(PluginServerError::Unavailable(format!( "could not inspect plugin MCP {context} path {}: {error}", ancestor.display() - )); + ))); } } } let mut resolved = crate::resilient_fs::canonicalize(&ancestor).map_err(|error| { - format!( + PluginServerError::Unavailable(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); @@ -1367,21 +1378,22 @@ fn prepare_plugins( ); continue; } + owners.insert(server.name.clone(), plugin.alias.clone()); match prepare_plugin_server(plugin, server, &root, &data) { Ok(server_prepared) => { - owners.insert(server.name.clone(), plugin.alias.clone()); entries.insert( server.name.clone(), server_prepared.record.fingerprint.clone(), ); prepared.insert(server.name.clone(), server_prepared); } - Err(error) => tracing::warn!( + Err(PluginServerError::Invalid(error)) => tracing::warn!( server = %server.name, plugin = %plugin.alias, %error, "skipping plugin MCP server" ), + Err(PluginServerError::Unavailable(error)) => return Err(error), } } } @@ -1393,7 +1405,7 @@ fn prepare_plugin_server( server: &PluginMcpServer, root: &str, data: &str, -) -> Result { +) -> Result { let description = format!("{} plugin MCP server", plugin.manifest_name); let (binding, url, static_authorization) = match &server.transport { PluginMcpTransport::Stdio { @@ -1405,7 +1417,8 @@ fn prepare_plugin_server( 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)? + plugin_text(&command, "command", &plugin.alias) + .map_err(PluginServerError::Invalid)? } else { command.clone() }; @@ -1422,54 +1435,63 @@ fn prepare_plugin_server( ("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| { - format!( + 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() - ) + 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, - }; + 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 + )) + })?, + ) + } + 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, + }; crate::resilient_fs::global() .require_disk(&plugin.root) - .map_err(|error| format!("plugin MCP files are not available on disk: {error}"))?; + .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| { - format!("plugin MCP data directory is not available on disk: {error}") + PluginServerError::Unavailable(format!( + "plugin MCP data directory is not available on disk: {error}" + )) })?; (McpTransportBinding::Stdio(transport), None, false) } @@ -1479,7 +1501,10 @@ fn prepare_plugin_server( transport = transport .with_header(name.as_str(), value.as_str()) .map_err(|error| { - format!("invalid plugin MCP config for {}: {error}", server.name) + PluginServerError::Invalid(format!( + "invalid plugin MCP config for {}: {error}", + server.name + )) })?; } ( @@ -1491,10 +1516,10 @@ fn prepare_plugin_server( ) } PluginMcpTransport::Sse { .. } => { - return Err(format!( + return Err(PluginServerError::Invalid(format!( "plugin {}: MCP server {:?} uses unsupported SSE transport", plugin.alias, server.name - )); + ))); } }; let fingerprint = format!( @@ -7362,6 +7387,63 @@ mod tests { ); } + #[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 unavailable_plugin_mcp_data_cwd_fails_preparation() { + let directory = tempfile::tempdir().unwrap(); + let mut 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()), + }, + }], + ); + let blocked = plugin.root.join("blocked"); + std::fs::write(&blocked, "").unwrap(); + plugin.data_dir = blocked; + assert!(prepare_plugins(&[plugin]).is_err()); + } + #[tokio::test] async fn ordered_sources_merge_and_optional_deletion_reveals_lower_layers() { let directory = tempfile::tempdir().unwrap(); From bf4f852da73ebf93aab3c65acdfcddc5e5f7fc0b Mon Sep 17 00:00:00 2001 From: daniel Date: Sun, 25 Oct 2026 12:02:05 +0000 Subject: [PATCH 3/3] fix(mcp): skip plugin MCP paths through package files A plugin command or cwd whose ancestor is a regular file is a package defect and skips only that server. --- src/tools/mcp.rs | 75 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 62 insertions(+), 13 deletions(-) diff --git a/src/tools/mcp.rs b/src/tools/mcp.rs index fc72f5ca..605febc2 100644 --- a/src/tools/mcp.rs +++ b/src/tools/mcp.rs @@ -1314,6 +1314,15 @@ enum PluginServerError { 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, @@ -1334,18 +1343,24 @@ fn contained_plugin_path( ancestor.pop(); } Err(error) => { - return Err(PluginServerError::Unavailable(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| { - PluginServerError::Unavailable(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(PluginServerError::Invalid(format!( @@ -7422,10 +7437,43 @@ mod tests { 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), + }, + }; + 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 mut plugin = plugin( + let plugin = plugin( "tools", directory.path(), vec![PluginMcpServer { @@ -7438,10 +7486,11 @@ mod tests { }, }], ); - let blocked = plugin.root.join("blocked"); - std::fs::write(&blocked, "").unwrap(); - plugin.data_dir = blocked; - assert!(prepare_plugins(&[plugin]).is_err()); + 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]