From e51d8920fdab7b81d1de6f5bdb736d3c78069cda Mon Sep 17 00:00:00 2001 From: daniel Date: Tue, 1 Sep 2026 04:56:40 +0100 Subject: [PATCH 1/2] feat(plugins): hot-reload agent plugins --- docs/user/agent-plugins.md | 22 +- src/main.rs | 87 ++- src/plugins.rs | 1428 ++++++++++++++++++++++++++++++++++-- src/protocols/acp.rs | 188 ++++- src/protocols/acp/v2.rs | 33 +- src/runtime.rs | 345 +++++++-- src/runtime/tests.rs | 187 ++++- src/tools/mcp.rs | 846 ++++++++++++++++----- 8 files changed, 2825 insertions(+), 311 deletions(-) diff --git a/docs/user/agent-plugins.md b/docs/user/agent-plugins.md index 7e0c42c..db0de2e 100644 --- a/docs/user/agent-plugins.md +++ b/docs/user/agent-plugins.md @@ -1,6 +1,6 @@ # Agent Plugins -Kit can load Agent Plugin packages from a local directory, a checksum-pinned online archive, or a Git repository. Source resolution happens at startup. Kit uses `agentkit-plugins` to validate the resolved package, exposes its valid Agent Skills through the existing `skill` tool, and registers its supported MCP servers. A plugin-only configuration works without `--mcp-config` or `mcp_config`. +Kit can load Agent Plugin packages from a local directory, a checksum-pinned online archive, or a Git repository. Source resolution happens at startup and plugin reload boundaries. Kit uses `agentkit-plugins` to validate each resolved package, exposes its valid Agent Skills through the existing `skill` tool, and registers its supported MCP servers. A plugin-only configuration works without `--mcp-config` or `mcp_config`, including when the session started before any plugins were configured. ## Configure a source @@ -26,19 +26,19 @@ subdir = "agent-plugins/example" Aliases must contain 1–64 lowercase ASCII letters, digits, or single hyphens, with an alphanumeric first and last character. Duplicate plugin manifest names are an error. -A relative `path` is resolved against Kit's working directory, not the configuration directory. An absolute path is used directly. Local packages are validated on every startup and remain mutable local content. +A relative `path` is resolved against Kit's working directory, not the configuration directory. An absolute path is used directly. Local packages are validated at startup and each reload boundary; validated skill generations are snapshotted even though the source package remains mutable local content. For an `archive`, provide the final archive URL. Kit does not translate forge URLs or automatically update archive sources. `sha256` is mandatory and identifies the exact downloaded bytes. HTTPS is required, except that explicit loopback HTTP URLs are accepted for local testing. Redirects from HTTPS must remain HTTPS. URL credentials and fragments are rejected. Kit recognizes ZIP, gzip-compressed tar, and plain tar by content. Archives may contain `plugin.json` at the extraction root or one top-level directory, as forge-generated archives commonly do. `subdir`, when present, is applied below that selected base. Archive paths must be contained relative paths; links, special files, duplicate normalized paths, and extraction-limit violations are rejected. Executable mode bits are not preserved in this release. -For `git`, `url` is required, while `rev` and `subdir` are optional. When `rev` is omitted, Kit fetches the remote's `HEAD`, which selects its default branch. `rev` can be an exact 40-hex SHA-1 commit ID or a safe Git ref name such as `main`, `v1.2.0`, `refs/heads/main`, or `refs/tags/v1.2.0`. Git options, refspecs, revision expressions, control characters, and malformed ref names are rejected. Kit fetches the selected name into a private Kit-controlled ref, resolves it to a full commit ID, and uses that immutable ID for archive validation and the cache. A full commit is the reproducible choice; omitted or named revisions are fetched again on every startup and can select a new commit when the remote ref moves. The URL must be an absolute HTTPS URL without user information, a query, or a fragment. Local, SCP-like, SSH, `git`, file, and external-helper transports are rejected. +For `git`, `url` is required, while `rev` and `subdir` are optional. When `rev` is omitted, Kit fetches the remote's `HEAD`, which selects its default branch. `rev` can be an exact 40-hex SHA-1 commit ID or a safe Git ref name such as `main`, `v1.2.0`, `refs/heads/main`, or `refs/tags/v1.2.0`. Git options, refspecs, revision expressions, control characters, and malformed ref names are rejected. Kit fetches the selected name into a private Kit-controlled ref, resolves it to a full commit ID, and uses that immutable ID for archive validation and the cache. A full commit is the reproducible choice; omitted or named revisions are fetched again at startup and each reload boundary, and can select a new commit when the remote ref moves. The URL must be an absolute HTTPS URL without user information, a query, or a fragment. Local, SCP-like, SSH, `git`, file, and external-helper transports are rejected. Kit invokes the installed `git` executable without a shell. It preserves normal system and user Git configuration so configured noninteractive HTTPS credential helpers can authenticate private repositories, but it disables Git terminal and configured askpass prompts, sets standard GUI credential-helper controls to noninteractive, and rejects credentials in the configured URL. Before network access, Kit verifies that `url.*.insteadOf` configuration did not rewrite the validated origin. System and uncommitted attribute files are disabled with controlled empty files; committed `.gitattributes` remains effective. Git diagnostics and configured URLs are not included in errors or written to temporary files. The selected portable `subdir` is archived with literal path semantics. Kit does not check out a worktree or run repository hooks, filters, Git LFS, or submodules; symlinks and submodules in the selected tree are rejected. ## Plugin MCP servers -Kit supports the Agent Plugin `stdio` and `streamable-http` transports. The deprecated `sse` transport is not supported: Kit skips each SSE server and writes a diagnostic containing the plugin alias and server name to stderr. Other valid servers in that plugin remain available. +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. For `stdio`, Kit materializes the validated portable declaration as follows: @@ -54,9 +54,11 @@ For `stdio`, Kit materializes the validated portable declaration as follows: The location follows the loaded Kit configuration directory, so a config loaded from another directory uses that directory's `plugin-data`. Streamable HTTP uses the validated URL and headers as declared; stdio placeholders are not expanded in HTTP URLs or headers. Plugin HTTP declarations do not add the explicit MCP file's `description`, bearer-token, or OAuth fields. +MCP tools are invoked only through Kit's `tool` meta-tool; exact `mcp_*` names are not exposed as direct compose callables. This keeps every call behind the server operation gate and post-wait fingerprint check. Calls to effective plugin-owned servers also hold a plugin-generation lease; configured, project-local, and command-line overrides retain their independent lifetime. + 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. -## Cache and startup behavior +## Cache and live reload behavior Archive content is downloaded, checked against `sha256`, validated, and extracted atomically under: @@ -74,8 +76,14 @@ A validated full-commit cache entry can be reused without network access. Tags a The cache is local state protected by the permissions of `~/.kit`; it is not a sandbox or a publisher-identity check. Remove a damaged cache entry to force a verified download. The configured archive checksum proves archive-byte integrity, while a Git commit identifies repository content; neither proves who published it. -Resolution or package-validation failures stop startup. Non-fatal package diagnostics are written to stderr with the plugin alias. Supported validated MCP declarations are registered and begin connecting in the background at startup; unsupported SSE declarations produce the skip diagnostic described above. +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. + +A candidate generation is published only after its TOML, sources, packages, diagnostics, MCP declarations, and server names validate. Invalid TOML or an invalid/resolution-failing package leaves both the previous plugin MCP servers and previous plugin skill roots active, and the triggering tool call or prompt returns a bounded diagnostic. Diagnostics that mean a skill or MCP component was skipped or disabled reject the whole candidate generation where Kit can identify them; clearly informational forward-compatibility diagnostics can be accepted. + +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. Kit captures plugin skill metadata, instructions, and resource inventory into an immutable in-memory generation. The live `skill` tool serves metadata and instructions from that generation instead of reparsing writable package or cache paths. Resource paths refer to a per-runtime snapshot tree that Kit makes read-only as defense in depth. + +Reload fingerprints cover the canonical package and data roots plus the complete expanded MCP declaration, so replacing a configured package or declaration reconnects the affected server. Package-change detection covers declarations and files under validated skill directories, including resource contents and inventory; unrelated executable and extension bytes outside those directories do not force a reconnect or skill generation. Kit uses a unique ephemeral skill-snapshot root per runtime, retains only the published and currently staged generations, and removes that root when the runtime is released. One runtime never garbage-collects another runtime's snapshot paths. Archive URLs remain checksum-pinned, so changing remote archive content requires a new configured checksum. -Skill collision precedence is 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. +For mutable path packages on supported Unix platforms, Kit opens each captured file through a no-follow descriptor chain and rechecks directory and file identity around capture. Other platforms use the strongest checks exposed by their standard filesystem APIs. The read-only cache mode and repeated metadata/content checks protect against ordinary concurrent edits and accidental cache mutation; they are not a sandbox or a guarantee against a hostile process running as the same operating-system user. `serve`, `acp`, and `prompt` resolve plugins directly. `tui` validates them before launch, and its built-in Kit server reloads the same global configuration, cache, and plugin MCP declarations. Nested built-in `acp.kit` children receive Kit's configured and explicit MCP paths, project root, and credential settings. They rediscover project `.mcp.json` and reload plugins from the same global Kit configuration, preserving the full MCP precedence order. External ACP profile processes receive standard ACP traffic but do not inherit Kit plugin declarations or Kit MCP configuration unless that external program implements and configures its own equivalent behavior. diff --git a/src/main.rs b/src/main.rs index c245c6b..e31f18e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -231,6 +231,7 @@ struct Config { otel_message_content_max_messages: Option, otel_message_content_max_bytes: Option, mcp_config: Option, + #[allow(dead_code)] #[serde(default)] plugins: BTreeMap, credential_store: Option, @@ -240,6 +241,8 @@ struct Config { subagent: Option, #[serde(skip)] config_dir: PathBuf, + #[serde(skip)] + config_path: Option, } #[derive(Debug, Deserialize)] @@ -258,12 +261,18 @@ impl Config { } fn load(path: &Path) -> io::Result { - let config_dir = absolute_parent(path)?; + let config_path = if path.is_absolute() { + path.to_path_buf() + } else { + env::current_dir()?.join(path) + }; + let config_dir = absolute_parent(&config_path)?; let contents = match fs::read_to_string(path) { Ok(contents) => contents, Err(error) if error.kind() == io::ErrorKind::NotFound => { return Ok(Self { config_dir, + config_path: Some(config_path), ..Self::default() }); } @@ -298,20 +307,26 @@ impl Config { ) })?; config.config_dir = config_dir; + config.config_path = Some(config_path); Ok(config) } - async fn resolve_plugins( + async fn plugin_runtime( &self, runtime_root: &Path, - ) -> Result { - kit::plugins::resolve( - &self.plugins, - runtime_root, - &self.config_dir.join("plugin-cache"), - &self.config_dir.join("plugin-data"), - ) - .await + ) -> Result, String> { + let Some(config_path) = &self.config_path else { + return Ok(None); + }; + Ok(Some( + kit::plugins::PluginRuntime::load( + config_path.clone(), + runtime_root.to_path_buf(), + self.config_dir.join("plugin-cache"), + self.config_dir.join("plugin-data"), + ) + .await?, + )) } fn root(&self, value: Option) -> PathBuf { @@ -871,7 +886,7 @@ async fn main() -> Result<(), Box> { let a2a = config.a2a(a2a); let credential_storage = mcp.credentials.storage(&config)?; let (configured_mcp, explicit_mcp) = mcp.config_paths(&config)?; - let plugins = config.resolve_plugins(&root).await?; + let plugins = config.plugin_runtime(&root).await?; let runtime = match session_id { Some(id) => { kit::Runtime::with_session_provider_credentials_effort_and_openrouter_key( @@ -893,11 +908,7 @@ async fn main() -> Result<(), Box> { openrouter_api_key.as_ref().map(|(key, _)| key.clone()), )?, }; - let runtime = kit::Runtime::with_plugin_skills( - runtime, - plugins.package_roots, - plugins.skill_directories, - )?; + let runtime = kit::Runtime::with_plugin_runtime(runtime, plugins)?; let runtime = kit::Runtime::with_telemetry(runtime, telemetry_settings.clone())?; let (harnesses, default_harness) = config.harnesses()?; let runtime = kit::Runtime::with_acp_harnesses(runtime, harnesses, default_harness)?; @@ -905,7 +916,7 @@ async fn main() -> Result<(), Box> { kit::Runtime::with_mcp_config( runtime, explicit_mcp.as_deref(), - plugins.mcp_plugins, + Vec::new(), true, credential_storage, ) @@ -915,7 +926,7 @@ async fn main() -> Result<(), Box> { runtime, configured_mcp.as_deref(), explicit_mcp.as_deref(), - plugins.mcp_plugins, + Vec::new(), true, credential_storage, ) @@ -963,7 +974,7 @@ async fn main() -> Result<(), Box> { let reasoning_effort = config.reasoning_effort(reasoning_effort); let credential_storage = mcp.credentials.storage(&config)?; let (configured_mcp, explicit_mcp) = mcp.config_paths(&config)?; - let plugins = config.resolve_plugins(&root).await?; + let plugins = config.plugin_runtime(&root).await?; let runtime = match session_id { Some(id) => { kit::Runtime::with_session_provider_credentials_effort_and_openrouter_key( @@ -985,11 +996,7 @@ async fn main() -> Result<(), Box> { openrouter_api_key.as_ref().map(|(key, _)| key.clone()), )?, }; - let runtime = kit::Runtime::with_plugin_skills( - runtime, - plugins.package_roots, - plugins.skill_directories, - )?; + let runtime = kit::Runtime::with_plugin_runtime(runtime, plugins)?; let runtime = kit::Runtime::with_telemetry(runtime, telemetry_settings.clone())?; let runtime = kit::Runtime::with_depth(runtime, subagent_depth)?; let runtime = kit::Runtime::with_subagent_parent_context( @@ -1002,7 +1009,7 @@ async fn main() -> Result<(), Box> { kit::Runtime::with_mcp_config( runtime, explicit_mcp.as_deref(), - plugins.mcp_plugins, + Vec::new(), true, credential_storage, ) @@ -1012,7 +1019,7 @@ async fn main() -> Result<(), Box> { runtime, configured_mcp.as_deref(), explicit_mcp.as_deref(), - plugins.mcp_plugins, + Vec::new(), true, credential_storage, ) @@ -1039,7 +1046,7 @@ async fn main() -> Result<(), Box> { let reasoning_effort = config.reasoning_effort(reasoning_effort); let credential_storage = mcp.credentials.storage(&config)?; let (configured_mcp, explicit_mcp) = mcp.config_paths(&config)?; - let plugins = config.resolve_plugins(&root).await?; + let plugins = config.plugin_runtime(&root).await?; let session_id = resume.clone().unwrap_or_else(kit::session::new_id); let runtime = kit::Runtime::with_session_provider_credentials_effort_and_openrouter_key( @@ -1055,11 +1062,7 @@ async fn main() -> Result<(), Box> { reasoning_effort, openrouter_api_key.as_ref().map(|(key, _)| key.clone()), )?; - let runtime = kit::Runtime::with_plugin_skills( - runtime, - plugins.package_roots, - plugins.skill_directories, - )?; + let runtime = kit::Runtime::with_plugin_runtime(runtime, plugins)?; let runtime = kit::Runtime::with_telemetry(runtime, telemetry_settings.clone())?; let (harnesses, default_harness) = config.harnesses()?; let runtime = kit::Runtime::with_acp_harnesses(runtime, harnesses, default_harness)?; @@ -1067,7 +1070,7 @@ async fn main() -> Result<(), Box> { kit::Runtime::with_mcp_config( runtime, explicit_mcp.as_deref(), - plugins.mcp_plugins, + Vec::new(), false, credential_storage, ) @@ -1077,7 +1080,7 @@ async fn main() -> Result<(), Box> { runtime, configured_mcp.as_deref(), explicit_mcp.as_deref(), - plugins.mcp_plugins, + Vec::new(), false, credential_storage, ) @@ -1107,7 +1110,7 @@ async fn main() -> Result<(), Box> { let a2a = config.a2a(a2a); let credential_storage = mcp.credentials.storage(&config)?; let (_, explicit_mcp) = mcp.config_paths(&config)?; - config.resolve_plugins(&root).await?; + let _ = config.plugin_runtime(&root).await?; kit::tui::run_with_reasoning_effort_and_openrouter_key( &root, &model, @@ -1155,6 +1158,20 @@ mod tests { assert!(error.to_string().contains("unknown field `names`")); } + #[tokio::test] + async fn missing_config_path_can_start_empty_and_reload_plugins_later() { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("missing-config.toml"); + let config = Config::load(&path).unwrap(); + assert_eq!(config.config_path.as_deref(), Some(path.as_path())); + let plugins = config + .plugin_runtime(directory.path()) + .await + .unwrap() + .expect("an exact missing config path still supports live reload"); + assert!(plugins.snapshot().package_roots.is_empty()); + } + #[test] fn config_file_supplies_defaults_and_cli_values_win() { let directory = tempfile::tempdir().unwrap(); diff --git a/src/plugins.rs b/src/plugins.rs index 1ce1dd9..de284d5 100644 --- a/src/plugins.rs +++ b/src/plugins.rs @@ -3,15 +3,16 @@ use std::{ env, ffi::{OsStr, OsString}, fs::{self, File, OpenOptions}, - io::{self, Cursor, Read, Write}, + io::{self, Cursor, Read, Seek, Write}, path::{Component, Path, PathBuf}, process::{Command, Stdio}, - sync::mpsc, + sync::{Arc, RwLock, mpsc}, thread, time::{Duration, Instant, SystemTime}, }; -use agentkit_plugins::{AgentPlugin, PluginMcpServer}; +use agentkit_plugins::{AgentPlugin, PluginDiagnosticKind, PluginMcpServer}; +use agentkit_tool_skills::{Skill, SkillRegistry}; use flate2::read::GzDecoder; use serde::Deserialize; use sha2::{Digest, Sha256}; @@ -91,34 +92,249 @@ pub struct ResolvedPluginMcp { pub struct ResolvedPlugins { pub package_roots: Vec, pub skill_directories: Vec, + pub skills: Vec, pub mcp_plugins: Vec, } +#[derive(Deserialize)] +struct PluginConfigFile { + #[serde(default)] + plugins: BTreeMap, +} + +#[derive(Clone)] +pub struct PluginRuntime { + inner: Arc, +} + +struct PluginRuntimeInner { + config_path: PathBuf, + runtime_root: PathBuf, + cache_root: PathBuf, + skill_cache_root: PathBuf, + data_root: PathBuf, + published: RwLock>, + generation_barrier: Arc>, +} + +#[derive(Debug)] +pub(crate) struct StagedPlugins { + pub(crate) resolved: ResolvedPlugins, +} + +impl PluginRuntime { + pub async fn load( + config_path: PathBuf, + runtime_root: PathBuf, + cache_root: PathBuf, + data_root: PathBuf, + ) -> Result { + let runtime = Self::new( + config_path, + runtime_root, + cache_root, + data_root, + ResolvedPlugins::default(), + ); + let staged = runtime.stage().await?; + runtime.publish(staged.resolved); + Ok(runtime) + } + + pub fn new( + config_path: PathBuf, + runtime_root: PathBuf, + cache_root: PathBuf, + data_root: PathBuf, + initial: ResolvedPlugins, + ) -> Self { + static NEXT_RUNTIME_CACHE: std::sync::atomic::AtomicU64 = + std::sync::atomic::AtomicU64::new(1); + let cache_id = NEXT_RUNTIME_CACHE.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let mut random = [0_u8; 16]; + let suffix = if getrandom::fill(&mut random).is_ok() { + random + .iter() + .map(|byte| format!("{byte:02x}")) + .collect::() + } else { + format!( + "{}-{cache_id}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map_or(0, |duration| duration.as_nanos()) + ) + }; + let skill_cache_root = cache_root.join("runtime-skill-generations").join(suffix); + Self { + inner: Arc::new(PluginRuntimeInner { + config_path, + runtime_root, + cache_root, + skill_cache_root, + data_root, + published: RwLock::new(Arc::new(initial)), + generation_barrier: Arc::new(tokio::sync::RwLock::new(())), + }), + } + } + + pub fn snapshot(&self) -> Arc { + self.inner + .published + .read() + .expect("plugin generation lock poisoned") + .clone() + } + + pub(crate) async fn stage(&self) -> Result { + let contents = match tokio::fs::read_to_string(&self.inner.config_path).await { + Ok(contents) => contents, + Err(error) if error.kind() == io::ErrorKind::NotFound => String::new(), + Err(error) => { + return Err(bounded_diagnostic(format!( + "could not read plugin config {}: {error}", + self.inner.config_path.display() + ))); + } + }; + let configs = if contents.is_empty() { + BTreeMap::new() + } else { + toml::from_str::(&contents) + .map_err(|error| { + bounded_diagnostic(format!( + "invalid plugin config {}: {error}", + self.inner.config_path.display() + )) + })? + .plugins + }; + cleanup_runtime_skill_generations(&self.inner.skill_cache_root, &self.snapshot()); + let resolved = resolve_with_skill_cache( + &configs, + &self.inner.runtime_root, + &self.inner.cache_root, + &self.inner.skill_cache_root, + &self.inner.data_root, + ) + .await + .map_err(bounded_diagnostic)?; + Ok(StagedPlugins { resolved }) + } + + pub(crate) async fn generation_lease(&self) -> tokio::sync::OwnedRwLockReadGuard<()> { + self.inner.generation_barrier.clone().read_owned().await + } + + pub(crate) fn try_generation_lease(&self) -> Option> { + self.inner.generation_barrier.clone().try_read_owned().ok() + } + + pub(crate) async fn generation_writer(&self) -> tokio::sync::OwnedRwLockWriteGuard<()> { + self.inner.generation_barrier.clone().write_owned().await + } + + pub(crate) fn publish(&self, resolved: ResolvedPlugins) { + let mut published = self + .inner + .published + .write() + .expect("plugin generation lock poisoned"); + if resolved_generation_key(&published) != resolved_generation_key(&resolved) { + *published = Arc::new(resolved); + } + let current = published.clone(); + drop(published); + cleanup_runtime_skill_generations(&self.inner.skill_cache_root, ¤t); + } +} + +impl Drop for PluginRuntimeInner { + fn drop(&mut self) { + make_tree_writable(&self.skill_cache_root); + let _ = fs::remove_dir_all(&self.skill_cache_root); + } +} + +fn resolved_generation_key(resolved: &ResolvedPlugins) -> blake3::Hash { + let mut key = blake3::Hasher::new(); + for root in &resolved.package_roots { + key.update(root.as_os_str().as_encoded_bytes()); + key.update(&[0]); + } + for directory in &resolved.skill_directories { + key.update(directory.as_os_str().as_encoded_bytes()); + key.update(&[0]); + } + key.update(format!("{:?}", resolved.mcp_plugins).as_bytes()); + key.finalize() +} + +pub(crate) fn bounded_diagnostic(mut message: String) -> String { + const LIMIT: usize = 2_048; + const ELLIPSIS: &str = "..."; + if message.len() > LIMIT { + let mut end = LIMIT - ELLIPSIS.len(); + while !message.is_char_boundary(end) { + end -= 1; + } + message.truncate(end); + message.push_str(ELLIPSIS); + } + message +} + pub async fn resolve( configs: &BTreeMap, runtime_root: &Path, cache_root: &Path, data_root: &Path, +) -> Result { + resolve_with_skill_cache(configs, runtime_root, cache_root, cache_root, data_root).await +} + +async fn resolve_with_skill_cache( + configs: &BTreeMap, + runtime_root: &Path, + cache_root: &Path, + skill_cache_root: &Path, + data_root: &Path, ) -> Result { let configs = configs.clone(); let runtime_root = runtime_root.to_path_buf(); let cache_root = cache_root.to_path_buf(); + let skill_cache_root = skill_cache_root.to_path_buf(); let data_root = data_root.to_path_buf(); - tokio::task::spawn_blocking(move || { - resolve_blocking(&configs, &runtime_root, &cache_root, &data_root) + let mut resolved = tokio::task::spawn_blocking(move || { + resolve_blocking( + &configs, + &runtime_root, + &cache_root, + &skill_cache_root, + &data_root, + ) }) .await - .map_err(|error| format!("plugin resolver task failed: {error}"))? + .map_err(|error| format!("plugin resolver task failed: {error}"))??; + let registry = SkillRegistry::from_skill_dirs(resolved.skill_directories.clone()) + .discover_skills() + .await; + resolved.skills = registry.skills().into_iter().cloned().collect(); + Ok(resolved) } fn resolve_blocking( configs: &BTreeMap, runtime_root: &Path, cache_root: &Path, + skill_cache_root: &Path, data_root: &Path, ) -> Result { let mut resolved = ResolvedPlugins::default(); let mut manifest_names = BTreeSet::new(); + let mut loaded_generations = Vec::new(); for (alias, config) in configs { validate_alias(alias)?; let root = match config { @@ -144,17 +360,7 @@ fn resolve_blocking( plugin.manifest().name )); } - for diagnostic in plugin.diagnostics() { - let path = diagnostic - .path - .as_deref() - .map(|path| format!(" at {}", path.display())) - .unwrap_or_default(); - eprintln!( - "plugin {alias} ({:?}){path}: {}", - diagnostic.kind, diagnostic.message - ); - } + validate_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| { @@ -184,10 +390,840 @@ fn resolve_blocking( resolved .skill_directories .extend(plugin.skill_directories()); + loaded_generations.push(( + alias.clone(), + plugin.root().to_path_buf(), + plugin_semantic_key(&plugin), + )); + } + let (package_roots, skill_directories) = snapshot_skills( + &resolved.package_roots, + &resolved.skill_directories, + skill_cache_root, + )?; + for (alias, root, expected) in loaded_generations { + let plugin = AgentPlugin::load(&root).map_err(|error| { + format!( + "could not revalidate plugin {alias:?} from {}: {error}", + 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" + )); + } } + resolved.package_roots = package_roots; + resolved.skill_directories = skill_directories; Ok(resolved) } +fn plugin_semantic_key(plugin: &AgentPlugin) -> blake3::Hash { + let mut key = blake3::Hasher::new(); + key.update(plugin.manifest().name.as_bytes()); + key.update(format!("{:?}", plugin.mcp_servers()).as_bytes()); + for skill in plugin.skill_directories() { + key.update(skill.as_os_str().as_encoded_bytes()); + key.update(&[0]); + } + key.finalize() +} + +#[derive(Debug, PartialEq, Eq)] +enum CachedEntry { + Directory, + File(Vec), +} + +fn snapshot_skills( + package_roots: &[PathBuf], + skill_directories: &[PathBuf], + cache_root: &Path, +) -> Result<(Vec, Vec), String> { + if skill_directories.is_empty() { + return Ok((Vec::new(), Vec::new())); + } + let mut inventory = BTreeMap::::new(); + let mut captured_bytes = 0_u64; + for (index, package) in package_roots.iter().enumerate() { + let target = PathBuf::from(index.to_string()); + inventory.insert(target.clone(), CachedEntry::Directory); + capture_snapshot_file( + package, + &package.join("plugin.json"), + &target.join("plugin.json"), + &mut inventory, + &mut captured_bytes, + )?; + } + let expected_skills = skill_directories + .iter() + .map(|directory| { + let (index, package) = owning_package(package_roots, directory).ok_or_else(|| { + format!( + "plugin skill {} is outside its package", + directory.display() + ) + })?; + let relative = directory + .strip_prefix(package) + .expect("owning package is a prefix"); + let target = PathBuf::from(index.to_string()).join(relative); + collect_skill_inventory( + package, + directory, + &PathBuf::from(index.to_string()), + &mut inventory, + &mut captured_bytes, + )?; + Ok(target) + }) + .collect::, String>>()?; + + // A second complete capture rejects in-place edits and inventory changes + // that race the first directory traversal. + let mut verification = BTreeMap::::new(); + let mut verification_bytes = 0_u64; + for (index, package) in package_roots.iter().enumerate() { + let target = PathBuf::from(index.to_string()); + verification.insert(target.clone(), CachedEntry::Directory); + capture_snapshot_file( + package, + &package.join("plugin.json"), + &target.join("plugin.json"), + &mut verification, + &mut verification_bytes, + )?; + } + for directory in skill_directories { + let (index, package) = owning_package(package_roots, directory).ok_or_else(|| { + format!( + "plugin skill {} is outside its package", + directory.display() + ) + })?; + collect_skill_inventory( + package, + directory, + &PathBuf::from(index.to_string()), + &mut verification, + &mut verification_bytes, + )?; + } + if verification != inventory { + return Err("plugin skills changed while their generation was being captured".into()); + } + + let paths = inventory.keys().cloned().collect::>(); + for path in paths { + let mut parent = path.parent(); + while let Some(relative) = parent.filter(|parent| !parent.as_os_str().is_empty()) { + match inventory.get(relative) { + Some(CachedEntry::File(_)) => { + return Err(format!( + "plugin skill inventory uses a file as a directory: {}", + relative.display() + )); + } + Some(CachedEntry::Directory) => {} + None => { + inventory.insert(relative.to_path_buf(), CachedEntry::Directory); + } + } + parent = relative.parent(); + } + } + + let mut fingerprint = blake3::Hasher::new(); + for root in package_roots { + fingerprint.update(root.as_os_str().as_encoded_bytes()); + fingerprint.update(&[0]); + } + for (relative, entry) in &inventory { + fingerprint.update(&(relative.as_os_str().as_encoded_bytes().len() as u64).to_le_bytes()); + fingerprint.update(relative.as_os_str().as_encoded_bytes()); + match entry { + CachedEntry::Directory => { + fingerprint.update(&[0]); + } + CachedEntry::File(bytes) => { + fingerprint.update(&[1]); + fingerprint.update(&(bytes.len() as u64).to_le_bytes()); + fingerprint.update(bytes); + } + } + } + let parent = cache_root.join("skill-generations"); + let destination = parent.join(fingerprint.finalize().to_hex().as_str()); + publish_cached_directory(&destination, "plugin skill generation", |staging| { + write_cached_inventory(staging, &inventory)?; + validate_skill_snapshot(staging, package_roots.len(), &expected_skills).map(|_| ()) + })?; + validate_cached_inventory(&destination, &inventory)?; + let published = validate_skill_snapshot(&destination, package_roots.len(), &expected_skills)?; + make_tree_read_only(&destination)?; + Ok(published) +} + +fn cleanup_runtime_skill_generations(root: &Path, current: &ResolvedPlugins) { + let generations = root.join("skill-generations"); + let current = current + .package_roots + .iter() + .filter_map(|package| package.parent().map(Path::to_path_buf)) + .collect::>(); + let Ok(entries) = fs::read_dir(&generations) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + if !current.contains(&path) { + make_tree_writable(&path); + let _ = fs::remove_dir_all(path); + } + } +} + +fn make_tree_read_only(root: &Path) -> Result<(), String> { + let mut paths = vec![root.to_path_buf()]; + let mut index = 0; + while index < paths.len() { + let path = paths[index].clone(); + index += 1; + if path.is_dir() { + for entry in fs::read_dir(&path).map_err(|error| { + format!( + "could not inspect plugin snapshot {}: {error}", + path.display() + ) + })? { + paths.push( + entry + .map_err(|error| format!("could not inspect plugin snapshot: {error}"))? + .path(), + ); + } + } + } + for path in paths.into_iter().rev() { + let metadata = fs::symlink_metadata(&path).map_err(|error| { + format!( + "could not inspect plugin snapshot {}: {error}", + path.display() + ) + })?; + let permissions = read_only_permissions(&metadata); + fs::set_permissions(&path, permissions).map_err(|error| { + format!( + "could not make plugin snapshot read-only {}: {error}", + path.display() + ) + })?; + } + Ok(()) +} + +fn make_tree_writable(root: &Path) { + let mut paths = vec![root.to_path_buf()]; + let mut index = 0; + while index < paths.len() { + let path = paths[index].clone(); + index += 1; + let Ok(metadata) = fs::symlink_metadata(&path) else { + continue; + }; + if metadata.permissions().readonly() { + let _ = fs::set_permissions(&path, writable_permissions(&metadata)); + } + if metadata.is_dir() + && let Ok(entries) = fs::read_dir(&path) + { + paths.extend(entries.flatten().map(|entry| entry.path())); + } + } +} + +#[cfg(test)] +pub(crate) fn make_tree_writable_for_test(root: &Path) { + make_tree_writable(root); +} + +#[cfg(unix)] +fn read_only_permissions(metadata: &fs::Metadata) -> fs::Permissions { + use std::os::unix::fs::PermissionsExt; + fs::Permissions::from_mode(metadata.permissions().mode() & !0o222) +} + +#[cfg(not(unix))] +fn read_only_permissions(metadata: &fs::Metadata) -> fs::Permissions { + let mut permissions = metadata.permissions(); + permissions.set_readonly(true); + permissions +} + +#[cfg(unix)] +fn writable_permissions(metadata: &fs::Metadata) -> fs::Permissions { + use std::os::unix::fs::PermissionsExt; + fs::Permissions::from_mode(metadata.permissions().mode() | 0o200) +} + +#[cfg(not(unix))] +fn writable_permissions(metadata: &fs::Metadata) -> fs::Permissions { + let mut permissions = metadata.permissions(); + permissions.set_readonly(false); + permissions +} + +fn owning_package<'a>(package_roots: &'a [PathBuf], path: &Path) -> Option<(usize, &'a Path)> { + package_roots + .iter() + .enumerate() + .filter(|(_, package)| path.starts_with(package)) + .max_by_key(|(_, package)| package.as_os_str().as_encoded_bytes().len()) + .map(|(index, package)| (index, package.as_path())) +} + +fn collect_skill_inventory( + package_root: &Path, + directory: &Path, + target_root: &Path, + inventory: &mut BTreeMap, + captured_bytes: &mut u64, +) -> Result<(), String> { + let canonical = directory.canonicalize().map_err(|error| { + format!( + "could not resolve plugin skill {}: {error}", + directory.display() + ) + })?; + if !canonical.starts_with(package_root) { + return Err(format!( + "plugin skill path resolves outside its package: {}", + directory.display() + )); + } + let metadata = fs::symlink_metadata(directory).map_err(|error| { + format!( + "could not inspect plugin skill {}: {error}", + directory.display() + ) + })?; + if metadata.file_type().is_symlink() || !metadata.is_dir() { + return Err(format!( + "plugin skill path is not a real directory: {}", + directory.display() + )); + } + let relative = directory + .strip_prefix(package_root) + .expect("validated skill directory is in its package"); + inventory.insert(target_root.join(relative), CachedEntry::Directory); + let entries = fs::read_dir(directory).map_err(|error| { + format!( + "could not read plugin skill {}: {error}", + directory.display() + ) + })?; + for entry in entries { + let entry = entry.map_err(|error| format!("could not read plugin skill entry: {error}"))?; + let path = entry.path(); + let metadata = fs::symlink_metadata(&path).map_err(|error| { + format!("could not inspect plugin skill {}: {error}", path.display()) + })?; + if metadata.file_type().is_symlink() { + return Err(format!( + "plugin skill path is a symlink: {}", + path.display() + )); + } + if metadata.is_dir() { + collect_skill_inventory(package_root, &path, target_root, inventory, captured_bytes)?; + } else if metadata.is_file() { + let relative = path + .strip_prefix(package_root) + .expect("skill inventory remains under its package"); + capture_snapshot_file( + package_root, + &path, + &target_root.join(relative), + inventory, + captured_bytes, + )?; + } else { + return Err(format!( + "plugin skill path is not a regular file: {}", + path.display() + )); + } + } + let after = fs::symlink_metadata(directory).map_err(|error| { + format!( + "could not recheck plugin skill {}: {error}", + directory.display() + ) + })?; + let canonical_after = directory.canonicalize().map_err(|error| { + format!( + "could not re-resolve plugin skill {}: {error}", + directory.display() + ) + })?; + if !after.is_dir() + || after.file_type().is_symlink() + || !same_file_state(&metadata, &after) + || canonical_after != canonical + { + return Err(format!( + "plugin skill directory changed while it was being captured: {}", + directory.display() + )); + } + Ok(()) +} + +fn capture_snapshot_file( + package_root: &Path, + source: &Path, + target: &Path, + inventory: &mut BTreeMap, + captured_bytes: &mut u64, +) -> Result<(), String> { + let canonical = source.canonicalize().map_err(|error| { + format!( + "could not resolve plugin skill {}: {error}", + source.display() + ) + })?; + if !canonical.starts_with(package_root) { + return Err(format!( + "plugin skill path resolves outside its package: {}", + source.display() + )); + } + let before = fs::symlink_metadata(source).map_err(|error| { + format!( + "could not inspect plugin skill {}: {error}", + source.display() + ) + })?; + if before.file_type().is_symlink() || !before.is_file() { + return Err(format!( + "plugin skill path is not a real file: {}", + source.display() + )); + } + let mut file = open_snapshot_file(package_root, source)?; + let opened = file.metadata().map_err(|error| { + format!( + "could not inspect open plugin skill {}: {error}", + source.display() + ) + })?; + let after = fs::symlink_metadata(source).map_err(|error| { + format!( + "could not recheck plugin skill {}: {error}", + source.display() + ) + })?; + if after.file_type().is_symlink() + || !opened.is_file() + || !after.is_file() + || !same_file(&before, &opened) + || !same_file(&opened, &after) + { + return Err(format!( + "plugin skill changed while it was being captured: {}", + source.display() + )); + } + let mut bytes = Vec::new(); + Read::by_ref(&mut file) + .take(MAX_FILE_BYTES + 1) + .read_to_end(&mut bytes) + .map_err(|error| format!("could not read plugin skill {}: {error}", source.display()))?; + if bytes.len() as u64 > MAX_FILE_BYTES { + return Err(format!( + "plugin skill file exceeds size limit: {}", + source.display() + )); + } + file.rewind().map_err(|error| { + format!( + "could not recheck plugin skill {}: {error}", + source.display() + ) + })?; + let mut second = Vec::new(); + Read::by_ref(&mut file) + .take(MAX_FILE_BYTES + 1) + .read_to_end(&mut second) + .map_err(|error| { + format!( + "could not recheck plugin skill {}: {error}", + source.display() + ) + })?; + let opened_after = file.metadata().map_err(|error| { + format!( + "could not recheck open plugin skill {}: {error}", + source.display() + ) + })?; + let path_after = fs::symlink_metadata(source).map_err(|error| { + format!( + "could not recheck plugin skill {}: {error}", + source.display() + ) + })?; + if bytes != second + || !same_file_state(&before, &opened_after) + || !same_file_state(&opened_after, &path_after) + { + return Err(format!( + "plugin skill changed while it was being captured: {}", + source.display() + )); + } + *captured_bytes = captured_bytes + .checked_add(bytes.len() as u64) + .filter(|bytes| *bytes <= MAX_EXPANDED_BYTES) + .ok_or_else(|| "plugin skills exceed expanded size limit".to_string())?; + inventory.insert(target.to_path_buf(), CachedEntry::File(bytes)); + Ok(()) +} + +#[cfg(unix)] +fn open_snapshot_file(package_root: &Path, source: &Path) -> Result { + use std::{ + ffi::CString, + os::{ + fd::{AsRawFd, FromRawFd, OwnedFd}, + unix::ffi::OsStrExt, + }, + }; + + let relative = source + .strip_prefix(package_root) + .map_err(|_| format!("plugin skill is outside its package: {}", source.display()))?; + let root = CString::new(package_root.as_os_str().as_bytes()) + .map_err(|_| "plugin package path contains a NUL byte".to_string())?; + // SAFETY: `root` is NUL terminated and the returned descriptor is owned. + let descriptor = unsafe { + libc::open( + root.as_ptr(), + libc::O_RDONLY | libc::O_DIRECTORY | libc::O_CLOEXEC | libc::O_NOFOLLOW, + ) + }; + if descriptor < 0 { + return Err(format!( + "could not open plugin package {} without following links: {}", + package_root.display(), + io::Error::last_os_error() + )); + } + // SAFETY: `descriptor` was returned uniquely by `open` above. + let mut current = unsafe { OwnedFd::from_raw_fd(descriptor) }; + let components = relative.components().collect::>(); + for (index, component) in components.iter().enumerate() { + let Component::Normal(component) = component else { + return Err(format!("invalid plugin skill path: {}", source.display())); + }; + let component = CString::new(component.as_bytes()) + .map_err(|_| "plugin skill path contains a NUL byte".to_string())?; + let last = index + 1 == components.len(); + let flags = libc::O_RDONLY + | libc::O_CLOEXEC + | libc::O_NOFOLLOW + | if last { 0 } else { libc::O_DIRECTORY }; + // SAFETY: both the directory descriptor and component C string are valid. + let descriptor = unsafe { libc::openat(current.as_raw_fd(), component.as_ptr(), flags) }; + if descriptor < 0 { + return Err(format!( + "could not open plugin skill {} without following links: {}", + source.display(), + io::Error::last_os_error() + )); + } + // SAFETY: `descriptor` was returned uniquely by `openat` above. + current = unsafe { OwnedFd::from_raw_fd(descriptor) }; + } + Ok(fs::File::from(current)) +} + +#[cfg(not(unix))] +fn open_snapshot_file(_package_root: &Path, source: &Path) -> Result { + OpenOptions::new() + .read(true) + .open(source) + .map_err(|error| format!("could not open plugin skill {}: {error}", source.display())) +} + +fn same_file_state(left: &fs::Metadata, right: &fs::Metadata) -> bool { + same_file(left, right) + && left.len() == right.len() + && left.modified().ok() == right.modified().ok() +} + +#[cfg(unix)] +fn same_file(left: &fs::Metadata, right: &fs::Metadata) -> bool { + use std::os::unix::fs::MetadataExt; + left.dev() == right.dev() && left.ino() == right.ino() +} + +#[cfg(windows)] +fn same_file(left: &fs::Metadata, right: &fs::Metadata) -> bool { + use std::os::windows::fs::MetadataExt; + left.volume_serial_number() == right.volume_serial_number() + && left.file_index() == right.file_index() +} + +#[cfg(not(any(unix, windows)))] +fn same_file(left: &fs::Metadata, right: &fs::Metadata) -> bool { + left.len() == right.len() && left.modified().ok() == right.modified().ok() +} + +fn write_cached_inventory( + root: &Path, + inventory: &BTreeMap, +) -> Result<(), String> { + for (relative, entry) in inventory { + let path = root.join(relative); + match entry { + CachedEntry::Directory => fs::create_dir_all(&path).map_err(|error| { + format!( + "could not create cached plugin directory {}: {error}", + path.display() + ) + })?, + CachedEntry::File(bytes) => { + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).map_err(|error| { + format!( + "could not create cached plugin directory {}: {error}", + parent.display() + ) + })?; + } + fs::write(&path, bytes).map_err(|error| { + format!( + "could not write cached plugin file {}: {error}", + path.display() + ) + })?; + } + } + } + Ok(()) +} + +fn validate_cached_inventory( + root: &Path, + expected: &BTreeMap, +) -> Result<(), String> { + let mut pending = vec![root.to_path_buf()]; + let mut actual = BTreeSet::new(); + while let Some(directory) = pending.pop() { + for entry in fs::read_dir(&directory).map_err(|error| { + format!( + "could not read immutable plugin skill snapshot {}: {error}", + directory.display() + ) + })? { + let entry = entry.map_err(|error| { + format!("could not read immutable plugin skill snapshot entry: {error}") + })?; + let path = entry.path(); + let relative = path.strip_prefix(root).map_err(|_| { + format!( + "immutable plugin skill snapshot entry escaped its root: {}", + path.display() + ) + })?; + let metadata = fs::symlink_metadata(&path).map_err(|error| { + format!( + "could not inspect immutable plugin skill snapshot {}: {error}", + path.display() + ) + })?; + if metadata.file_type().is_symlink() { + return Err(format!( + "immutable plugin skill snapshot contains a symlink: {}", + path.display() + )); + } + if actual.len() >= MAX_ARCHIVE_ENTRIES { + return Err("immutable plugin skill snapshot exceeds entry limit".into()); + } + actual.insert(relative.to_path_buf()); + if metadata.is_dir() { + pending.push(path); + } else if !metadata.is_file() { + return Err(format!( + "immutable plugin skill snapshot entry is not a regular file: {}", + path.display() + )); + } + } + } + if actual != expected.keys().cloned().collect() { + return Err( + "immutable plugin skill snapshot inventory does not match its fingerprint".into(), + ); + } + for (relative, expected_entry) in expected { + let path = root.join(relative); + let metadata = fs::symlink_metadata(&path).map_err(|error| { + format!( + "could not inspect immutable plugin skill snapshot {}: {error}", + path.display() + ) + })?; + match expected_entry { + CachedEntry::Directory if metadata.is_dir() => {} + CachedEntry::File(expected_bytes) if metadata.is_file() => { + if metadata.len() > MAX_FILE_BYTES { + return Err(format!( + "immutable plugin skill snapshot file exceeds size limit: {}", + path.display() + )); + } + let bytes = fs::read(&path).map_err(|error| { + format!( + "could not read immutable plugin skill snapshot {}: {error}", + path.display() + ) + })?; + if &bytes != expected_bytes { + return Err(format!( + "immutable plugin skill snapshot does not match its fingerprint: {}", + path.display() + )); + } + } + _ => { + return Err(format!( + "immutable plugin skill snapshot has the wrong entry type: {}", + path.display() + )); + } + } + } + Ok(()) +} + +fn validate_skill_snapshot( + root: &Path, + package_count: usize, + expected_skills: &BTreeSet, +) -> Result<(Vec, Vec), String> { + let canonical_root = root.canonicalize().map_err(|error| { + format!( + "could not resolve immutable plugin skill snapshot {}: {error}", + root.display() + ) + })?; + let mut package_roots = Vec::new(); + let mut skill_directories = Vec::new(); + let mut actual_skills = BTreeSet::new(); + for index in 0..package_count { + let package = root.join(index.to_string()); + let plugin = AgentPlugin::load(&package).map_err(|error| { + format!( + "could not validate immutable plugin skill snapshot {}: {error}", + package.display() + ) + })?; + validate_plugin_diagnostics(&index.to_string(), &plugin)?; + if !plugin.skill_directories().is_empty() { + package_roots.push(package.clone()); + } + for skill in plugin.skill_directories() { + let relative = skill.strip_prefix(&canonical_root).map_err(|_| { + format!( + "immutable plugin skill escaped its snapshot root: {}", + skill.display() + ) + })?; + actual_skills.insert(relative.to_path_buf()); + skill_directories.push(root.join(relative)); + } + } + if &actual_skills != expected_skills { + return Err("plugin skills changed while the immutable generation was being built".into()); + } + Ok((package_roots, skill_directories)) +} + +fn publish_cached_directory( + destination: &Path, + context: &str, + build: impl FnOnce(&Path) -> Result<(), String>, +) -> Result<(), String> { + match fs::symlink_metadata(destination) { + Ok(metadata) if metadata.file_type().is_dir() => return Ok(()), + Ok(_) => { + return Err(format!( + "{context} cache entry is not a real directory: {}", + destination.display() + )); + } + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => return Err(format!("could not inspect {context} cache entry: {error}")), + } + let parent = destination + .parent() + .ok_or_else(|| format!("{context} cache path has no parent"))?; + fs::create_dir_all(parent).map_err(|error| { + format!( + "could not create {context} cache {}: {error}", + parent.display() + ) + })?; + let mut random = [0u8; 8]; + getrandom::fill(&mut random).map_err(|error| error.to_string())?; + let staging = parent.join(format!( + ".{}-{:x}.tmp", + std::process::id(), + u64::from_le_bytes(random) + )); + fs::create_dir(&staging) + .map_err(|error| format!("could not create {context} staging directory: {error}"))?; + let result = build(&staging).and_then(|()| match fs::rename(&staging, destination) { + Ok(()) => Ok(()), + Err(_) + if fs::symlink_metadata(destination) + .is_ok_and(|metadata| metadata.file_type().is_dir()) => + { + Ok(()) + } + Err(error) => Err(format!("could not publish {context}: {error}")), + }); + if staging.exists() { + let _ = fs::remove_dir_all(&staging); + } + result +} + +fn validate_plugin_diagnostics(alias: &str, plugin: &AgentPlugin) -> Result<(), String> { + 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 + )); + } + Ok(()) +} + fn validate_alias(alias: &str) -> Result<(), String> { let bytes = alias.as_bytes(); if alias.is_empty() @@ -1444,51 +2480,176 @@ fn resolve_archive( cache_root.display() ) })?; + let bytes = cached_archive_bytes( + &url, + &expected, + &cache_root.join("archive-blobs").join(&digest), + )?; let destination = cache_root.join(&digest); - match fs::symlink_metadata(&destination) { - Ok(metadata) if !metadata.file_type().is_dir() => { + if destination.exists() { + verify_cached_archive(&destination, &bytes)?; + } + publish_cached_directory(&destination, "plugin archive", |staging| { + extract_archive(&bytes, staging)?; + let candidate = select_package_root(staging, subdir.as_deref())?; + AgentPlugin::load(&candidate).map_err(|error| { + format!( + "invalid plugin archive package at {}: {error}", + candidate.display() + ) + })?; + Ok(()) + })?; + select_package_root(&destination, subdir.as_deref()) +} + +fn cached_archive_bytes(url: &Url, expected: &[u8; 32], blob: &Path) -> Result, String> { + let bytes = match fs::symlink_metadata(blob) { + Ok(metadata) if metadata.file_type().is_file() && metadata.len() <= MAX_DOWNLOAD_BYTES => { + fs::read(blob).map_err(|error| { + format!( + "could not read cached plugin archive {}: {error}", + blob.display() + ) + })? + } + Ok(_) => { return Err(format!( - "plugin cache entry is not a directory: {}", - destination.display() + "cached plugin archive is not a bounded regular file: {}", + blob.display() )); } - Ok(_) => return validate_archive_cache_entry(&destination, subdir.as_deref()), - Err(error) if error.kind() == io::ErrorKind::NotFound => {} - Err(error) => return Err(format!("could not inspect plugin cache entry: {error}")), - } + Err(error) if error.kind() == io::ErrorKind::NotFound => { + let bytes = download(url)?; + verify_archive_digest(url, expected, &bytes)?; + publish_cached_file(blob, &bytes, "plugin archive blob")?; + return cached_archive_bytes(url, expected, blob); + } + Err(error) => { + return Err(format!( + "could not inspect cached plugin archive {}: {error}", + blob.display() + )); + } + }; + verify_archive_digest(url, expected, &bytes)?; + Ok(bytes) +} - let prefix = format!(".{digest}.staging-"); - let staging_guard = StagingDirectory::create(cache_root, &prefix)?; - let staging = staging_guard.path(); - let bytes = download(&url)?; - let actual = Sha256::digest(&bytes); +fn verify_archive_digest(url: &Url, expected: &[u8; 32], bytes: &[u8]) -> Result<(), String> { + let actual = Sha256::digest(bytes); if actual.as_slice() != expected { return Err(format!("SHA-256 mismatch for plugin archive {url}")); } - extract_archive(&bytes, staging)?; - let candidate = select_package_root(staging, subdir.as_deref())?; - AgentPlugin::load(&candidate).map_err(|error| { + Ok(()) +} + +fn publish_cached_file(destination: &Path, bytes: &[u8], context: &str) -> Result<(), String> { + let parent = destination + .parent() + .ok_or_else(|| format!("{context} cache path has no parent"))?; + fs::create_dir_all(parent).map_err(|error| { format!( - "invalid plugin archive package at {}: {error}", - candidate.display() + "could not create {context} cache {}: {error}", + parent.display() ) })?; - match fs::rename(staging, &destination) { - Ok(()) => {} - Err(_) if validate_archive_cache_entry(&destination, subdir.as_deref()).is_ok() => {} - Err(error) => return Err(format!("could not publish plugin archive: {error}")), - } - validate_archive_cache_entry(&destination, subdir.as_deref()) + let mut random = [0u8; 8]; + getrandom::fill(&mut random).map_err(|error| error.to_string())?; + let staging = parent.join(format!( + ".{}-{:x}.tmp", + std::process::id(), + u64::from_le_bytes(random) + )); + fs::write(&staging, bytes) + .map_err(|error| format!("could not write {context} staging file: {error}"))?; + let result = match fs::rename(&staging, destination) { + Ok(()) => Ok(()), + Err(_) if destination.is_file() => Ok(()), + Err(error) => Err(format!("could not publish {context}: {error}")), + }; + let _ = fs::remove_file(staging); + result } -fn validate_archive_cache_entry( - destination: &Path, - subdir: Option<&Path>, -) -> Result { - let root = select_package_root(destination, subdir)?; - AgentPlugin::load(&root) - .map_err(|error| format!("invalid cached plugin archive package: {error}"))?; - Ok(root) +fn verify_cached_archive(destination: &Path, bytes: &[u8]) -> Result<(), String> { + let parent = destination + .parent() + .ok_or_else(|| "plugin archive cache path has no parent".to_string())?; + let mut random = [0u8; 8]; + getrandom::fill(&mut random).map_err(|error| error.to_string())?; + let staging = parent.join(format!( + ".verify-{}-{:x}.tmp", + std::process::id(), + u64::from_le_bytes(random) + )); + fs::create_dir(&staging).map_err(|error| { + format!("could not create plugin archive verification directory: {error}") + })?; + let result = extract_archive(bytes, &staging).and_then(|()| { + let expected = read_directory_inventory(&staging)?; + let actual = read_directory_inventory(destination)?; + if actual == expected { + Ok(()) + } else { + Err(format!( + "cached plugin archive does not match its SHA-256 source: {}", + destination.display() + )) + } + }); + let _ = fs::remove_dir_all(staging); + result +} + +fn read_directory_inventory(root: &Path) -> Result, String> { + let mut inventory = BTreeMap::new(); + let mut pending = vec![root.to_path_buf()]; + let mut bytes_read = 0_u64; + while let Some(directory) = pending.pop() { + for entry in fs::read_dir(&directory) + .map_err(|error| format!("could not read plugin archive cache: {error}"))? + { + let entry = + entry.map_err(|error| format!("could not read plugin archive entry: {error}"))?; + if inventory.len() >= MAX_ARCHIVE_ENTRIES { + return Err("plugin archive cache exceeds entry limit".into()); + } + let path = entry.path(); + let relative = path + .strip_prefix(root) + .map_err(|_| "plugin archive entry escaped its root".to_string())? + .to_path_buf(); + let metadata = fs::symlink_metadata(&path) + .map_err(|error| format!("could not inspect plugin archive entry: {error}"))?; + if metadata.file_type().is_symlink() { + return Err(format!( + "plugin archive cache contains a symlink: {}", + path.display() + )); + } + if metadata.is_dir() { + inventory.insert(relative, CachedEntry::Directory); + pending.push(path); + } else if metadata.is_file() && metadata.len() <= MAX_FILE_BYTES { + bytes_read = bytes_read + .checked_add(metadata.len()) + .filter(|total| *total <= MAX_EXPANDED_BYTES) + .ok_or_else(|| { + "plugin archive cache exceeds expanded size limit".to_string() + })?; + let bytes = fs::read(&path) + .map_err(|error| format!("could not read plugin archive entry: {error}"))?; + inventory.insert(relative, CachedEntry::File(bytes)); + } else { + return Err(format!( + "plugin archive cache contains an invalid entry: {}", + path.display() + )); + } + } + } + Ok(inventory) } fn validate_download_url(url: &Url) -> Result<(), String> { @@ -1950,6 +3111,169 @@ mod tests { )); } + #[cfg(unix)] + #[test] + fn intermediate_directory_symlink_mutation_is_rejected() { + use std::os::unix::fs::symlink; + + let directory = tempfile::tempdir().unwrap(); + let package = directory.path().join("plugin"); + let original = package.join("skills/live-skill"); + fs::create_dir_all(&original).unwrap(); + fs::write(original.join("SKILL.md"), "safe").unwrap(); + let canonical_package = package.canonicalize().unwrap(); + let source = canonical_package.join("skills/live-skill/SKILL.md"); + assert!( + source + .canonicalize() + .unwrap() + .starts_with(&canonical_package) + ); + + let moved = package.join("skills-original"); + fs::rename(package.join("skills"), &moved).unwrap(); + let outside = directory.path().join("outside/live-skill"); + fs::create_dir_all(&outside).unwrap(); + fs::write(outside.join("SKILL.md"), "outside").unwrap(); + symlink(outside.parent().unwrap(), package.join("skills")).unwrap(); + + let error = open_snapshot_file(&canonical_package, &source).unwrap_err(); + assert!(error.contains("without following links")); + } + + #[tokio::test] + async fn poisoned_unpublished_skill_generation_is_rebuilt() { + let directory = tempfile::tempdir().unwrap(); + let config = directory.path().join("config.toml"); + let package = directory.path().join("plugin"); + let skill = package.join("skills/live-skill"); + fs::create_dir_all(&skill).unwrap(); + fs::write(package.join("plugin.json"), MANIFEST).unwrap(); + fs::write( + skill.join("SKILL.md"), + "---\nname: live-skill\ndescription: Live skill.\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(); + let generation = staged.resolved.package_roots[0] + .parent() + .expect("snapshot package has a generation root"); + let cached_skill = staged.resolved.skill_directories[0].join("SKILL.md"); + assert!(fs::write(&cached_skill, "blocked mutation").is_err()); + make_tree_writable(generation); + fs::write(&cached_skill, "poisoned").unwrap(); + + let rebuilt = runtime.stage().await.unwrap(); + assert_eq!(rebuilt.resolved.skills[0].body, "body"); + assert!(!rebuilt.resolved.skills[0].body.contains("poisoned")); + assert!(runtime.snapshot().skill_directories.is_empty()); + } + + #[tokio::test] + async fn live_runtime_stages_add_remove_and_retains_last_valid_generation() { + let directory = tempfile::tempdir().unwrap(); + let config = directory.path().join("config.toml"); + let package = directory.path().join("plugin"); + let skill = package.join("skills/live-skill"); + fs::create_dir_all(&skill).unwrap(); + fs::write(package.join("plugin.json"), MANIFEST).unwrap(); + fs::write( + skill.join("SKILL.md"), + "---\nname: live-skill\ndescription: Live skill.\n---\nbody\n", + ) + .unwrap(); + fs::write(&config, "").unwrap(); + let runtime = PluginRuntime::load( + config.clone(), + directory.path().to_path_buf(), + directory.path().join("cache"), + directory.path().join("data"), + ) + .await + .unwrap(); + let mcp = crate::tools::mcp::connect_dynamic( + None::<&Path>, + runtime.clone(), + false, + crate::tools::mcp::CredentialStorage::Memory, + ) + .await + .unwrap(); + assert!(runtime.snapshot().package_roots.is_empty()); + + fs::write( + &config, + format!( + "[plugins.live]\nsource = 'path'\npath = '{}'\n", + package.display() + ), + ) + .unwrap(); + mcp.refresh().await.unwrap(); + assert_eq!(runtime.snapshot().package_roots.len(), 1); + assert_eq!(runtime.snapshot().skill_directories.len(), 1); + 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( + skill.join("SKILL.md"), + "---\nname: live-skill\ndescription: Changed skill.\n---\nchanged body\n", + ) + .unwrap(); + assert!(mcp.refresh().await.is_err()); + assert_eq!( + runtime.snapshot().skill_directories[0].join("SKILL.md"), + first_skill + ); + assert!( + !fs::read_to_string(&first_skill) + .unwrap() + .contains("changed body") + ); + + fs::remove_file(package.join("mcp.json")).unwrap(); + mcp.refresh().await.unwrap(); + assert_ne!( + runtime.snapshot().skill_directories[0].join("SKILL.md"), + first_skill + ); + assert!( + fs::read_to_string(runtime.snapshot().skill_directories[0].join("SKILL.md")) + .unwrap() + .contains("changed body") + ); + + fs::write(&config, "[plugins.live\n").unwrap(); + let error = mcp.refresh().await.unwrap_err(); + assert!(error.contains("invalid plugin config")); + assert_eq!(runtime.snapshot().package_roots.len(), 1); + + fs::write(&config, "").unwrap(); + mcp.refresh().await.unwrap(); + assert!(runtime.snapshot().package_roots.is_empty()); + assert!(runtime.snapshot().skill_directories.is_empty()); + } + #[test] fn validates_aliases_digests_and_paths() { assert!(validate_alias("review-tools").is_ok()); @@ -2937,6 +4261,12 @@ mod tests { resolve_archive(&url, &digest, None, cache.path()).unwrap(), root ); + fs::write(root.join("plugin.json"), "poisoned").unwrap(); + assert!( + resolve_archive(&url, &digest, None, cache.path()) + .unwrap_err() + .contains("does not match its SHA-256 source") + ); let other_cache = tempfile::tempdir().unwrap(); let (url, server) = serve_once(bytes); diff --git a/src/protocols/acp.rs b/src/protocols/acp.rs index 0e35302..8b6ac42 100644 --- a/src/protocols/acp.rs +++ b/src/protocols/acp.rs @@ -1123,10 +1123,9 @@ async fn session_actor(actor: SessionActor) { biased; command = commands.recv() => match command { Some(Command::Prompt { request, reply }) => { - let skills = runtime.current_skills().await; - let result = drive_prompt( + let result = drive_runtime_prompt( &session_id, - &skills, + &runtime, &integration, &mut skill_catalog, &mut driver, @@ -1432,6 +1431,7 @@ fn record_acp_loop_failure( } } +#[cfg(test)] #[allow(clippy::too_many_arguments)] async fn drive_prompt( session_id: &agentkit_acp::SessionId, @@ -1459,6 +1459,68 @@ async fn drive_prompt( record_acp_loop_failure(session_id, &error) } })?; + drive_submitted_prompt( + session_id, + integration, + driver, + tasks, + background_jobs, + structured_completion, + ) + .await +} + +#[allow(clippy::too_many_arguments)] +async fn drive_runtime_prompt( + session_id: &agentkit_acp::SessionId, + runtime: &Arc, + integration: &AcpIntegration, + skill_catalog: &mut skill_catalog::SkillCatalogMonitor, + driver: &mut LoopDriver, + request: PromptRequest, + tasks: &TaskManagerHandle, + background_jobs: &BackgroundJobs, + structured_completion: bool, +) -> Result { + if structured_completion { + let _ = settle_background_jobs(tasks, background_jobs).await?; + } + let current = runtime + .current_skills() + .await + .map_err(AcpRuntimeError::Loop)?; + background_jobs.begin_turn(); + let items = integration.input_port().prompt_to_items(&request)?; + skill_catalog + .submit(¤t.skills, items, |items| driver.submit_input(items)) + .map_err(|error| match error { + skill_catalog::SubmitError::Catalog(error) => { + record_acp_runtime_failure(session_id, "skill_catalog", error) + } + skill_catalog::SubmitError::Submit(error) => { + record_acp_loop_failure(session_id, &error) + } + })?; + drop(current); + drive_submitted_prompt( + session_id, + integration, + driver, + tasks, + background_jobs, + structured_completion, + ) + .await +} + +async fn drive_submitted_prompt( + session_id: &agentkit_acp::SessionId, + integration: &AcpIntegration, + driver: &mut LoopDriver, + tasks: &TaskManagerHandle, + background_jobs: &BackgroundJobs, + structured_completion: bool, +) -> Result { let response = match drive_until_pause( session_id, integration, @@ -2751,6 +2813,122 @@ pub(super) mod tests { drain.abort(); } + #[tokio::test] + async fn live_prompt_boundary_refreshes_plugin_skill_catalog() { + let root = tempfile::tempdir().unwrap(); + let config = root.path().join("config.toml"); + std::fs::write(&config, "").unwrap(); + let plugins = crate::plugins::PluginRuntime::load( + config.clone(), + root.path().to_path_buf(), + root.path().join("cache"), + root.path().join("data"), + ) + .await + .unwrap(); + let runtime = Runtime::with_plugin_runtime( + Runtime::new(root.path(), "gpt-5.4").unwrap(), + Some(plugins), + ) + .unwrap(); + let runtime = Runtime::with_mcp_config( + runtime, + None, + Vec::new(), + false, + crate::tools::mcp::CredentialStorage::Memory, + ) + .await + .unwrap(); + let baseline = runtime.current_skills().await.unwrap(); + let mut skill_catalog = skill_catalog::SkillCatalogMonitor::new(&baseline.skills).unwrap(); + drop(baseline); + + let package = root.path().join("plugin"); + let skill = package.join("skills/live-skill"); + std::fs::create_dir_all(&skill).unwrap(); + std::fs::write( + package.join("plugin.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/plugin.schema.json","name":"live-plugin"}"#, + ) + .unwrap(); + std::fs::write( + skill.join("SKILL.md"), + "---\nname: live-skill\ndescription: Live skill.\n---\nbody\n", + ) + .unwrap(); + std::fs::write( + &config, + format!( + "[plugins.live]\nsource = 'path'\npath = '{}'\n", + package.display() + ), + ) + .unwrap(); + + let integration = Arc::new( + AcpIntegration::builder() + .name("plugin-refresh-test") + .approval_resolver(AutoDenyResolver) + .build() + .unwrap(), + ); + let acp_session_id = agentkit_acp::SessionId::new("s-plugin-refresh"); + let agentkit_session_id = AgentkitSessionId::new("s-plugin-refresh"); + let (client, mut messages) = AcpClientHandle::channel(); + integration + .bind_session(AcpSessionBinding::new( + acp_session_id.clone(), + agentkit_session_id.clone(), + client, + )) + .unwrap(); + let drain = tokio::spawn(async move { + while let Some(message) = messages.recv().await { + if let AcpClientMessage::Flush { response } = message { + let _ = response.send(()); + } + } + }); + let turns = Arc::new(AtomicUsize::new(2)); + let notification_items_seen = Arc::new(AtomicUsize::new(0)); + let mut driver = Agent::builder() + .model(ScriptAdapter { + turns, + user_items_seen: Arc::new(AtomicUsize::new(0)), + notification_items_seen: Arc::clone(¬ification_items_seen), + }) + .observer(integration.as_ref().clone()) + .build() + .unwrap() + .start(SessionConfig::new(agentkit_session_id).without_cache()) + .await + .unwrap(); + let task_manager = AsyncTaskManager::new(); + let tasks = task_manager.handle(); + let response = drive_runtime_prompt( + &acp_session_id, + &runtime, + &integration, + &mut skill_catalog, + &mut driver, + PromptRequest::new( + acp_session_id.clone(), + vec![agentkit_acp::ContentBlock::Text( + agentkit_acp::TextContent::new("use the new skill"), + )], + ), + &tasks, + &BackgroundJobs::default(), + false, + ) + .await + .unwrap(); + assert_eq!(response.stop_reason, StopReason::EndTurn); + assert_eq!(notification_items_seen.load(Ordering::SeqCst), 1); + drain.abort(); + } + #[tokio::test] async fn structured_prompt_waits_for_background_completion_and_synthesis() { let turns = Arc::new(AtomicUsize::new(0)); @@ -2958,7 +3136,7 @@ pub(super) mod tests { let mcp_events = test_mcp.subscribe(acp_session_id.to_string()); let root = tempfile::tempdir().unwrap(); let runtime = Runtime::new(root.path(), "gpt-5.4").unwrap(); - let skills = runtime.current_skills().await; + let skills = runtime.current_skills().await.unwrap(); let actor = tokio::spawn(session_actor(SessionActor { session_id: acp_session_id.clone(), runtime, @@ -2968,7 +3146,7 @@ pub(super) mod tests { tasks: tasks.clone(), background_jobs: background_jobs.clone(), structured_completion: false, - skill_catalog: skill_catalog::SkillCatalogMonitor::new(&skills).unwrap(), + skill_catalog: skill_catalog::SkillCatalogMonitor::new(&skills.skills).unwrap(), adapter: SelectableAdapter::new(crate::ProviderKind::OpenAiSubscription, "gpt-5.4") .unwrap(), catalog: Vec::new(), diff --git a/src/protocols/acp/v2.rs b/src/protocols/acp/v2.rs index 9dd485f..5eae6a0 100644 --- a/src/protocols/acp/v2.rs +++ b/src/protocols/acp/v2.rs @@ -993,10 +993,9 @@ async fn session_actor(actor: SessionActor) biased; command = commands.recv() => match command { Some(Command::Prompt(command)) => { - let skills = runtime.current_skills().await; let result = prepare_prompt( &session_id, - &skills, + PromptSkillSource::Runtime(&runtime), &integration, &handle, &mut skill_catalog, @@ -1074,10 +1073,16 @@ async fn session_actor(actor: SessionActor) } } +enum PromptSkillSource<'a> { + #[cfg(test)] + Static(&'a [agentkit_tool_skills::Skill]), + Runtime(&'a Arc), +} + #[allow(clippy::too_many_arguments)] async fn prepare_prompt( session_id: &wire::SessionId, - skills: &[agentkit_tool_skills::Skill], + skill_source: PromptSkillSource<'_>, integration: &AcpIntegration, handle: &AcpSessionHandle, skill_catalog: &mut skill_catalog::SkillCatalogMonitor, @@ -1100,6 +1105,25 @@ async fn prepare_prompt( let _ = reply.send(Err(error)); return Ok(()); } + let current; + let skills = match skill_source { + #[cfg(test)] + PromptSkillSource::Static(skills) => { + current = None; + skills + } + PromptSkillSource::Runtime(runtime) => { + current = match runtime.current_skills().await { + Ok(current) => Some(current), + Err(error) => { + handle.stop_injection_turn(); + let _ = reply.send(Err(AcpRuntimeError::Loop(error))); + return Ok(()); + } + }; + ¤t.as_ref().expect("runtime skills were loaded").skills + } + }; background_jobs.begin_turn(); let prepared = integration.prompt_to_items(&request).and_then(|items| { skill_catalog @@ -1112,6 +1136,7 @@ async fn prepare_prompt( })?; integration.begin_prompt(session_id) }); + drop(current); let user_message_id = match prepared { Ok(message_id) => message_id, Err(error) => { @@ -2706,7 +2731,7 @@ mod tests { let (result, ()) = tokio::join!( prepare_prompt( &session_id, - &[], + PromptSkillSource::Static(&[]), &integration, &handle, &mut skill_catalog, diff --git a/src/runtime.rs b/src/runtime.rs index 4490b3a..2a755a6 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -1,5 +1,5 @@ use std::{ - collections::{HashMap, VecDeque}, + collections::{BTreeSet, HashMap, VecDeque}, fmt::Write as _, path::{Path, PathBuf}, sync::{ @@ -12,7 +12,8 @@ use std::{ use agentkit_acp::{AcpIntegration, AcpRuntimeError}; use agentkit_context::{AgentsMd, ContextLoader}; use agentkit_core::{ - CancellationController, CancellationHandle, FinishReason, Item, ItemKind, Part, + CancellationController, CancellationHandle, FinishReason, Item, ItemKind, MetadataMap, Part, + ToolOutput, ToolResultPart, }; use agentkit_loop::{ Agent, LoopDriver, LoopError, LoopInterrupt, LoopObserver, LoopStep, SessionConfig, @@ -252,6 +253,7 @@ pub struct Runtime { /// Later ACP sessions receive their own persisted ids. session: Mutex, mcp: crate::tools::mcp::McpRuntime, + plugin_runtime: Option, skills: Arc, skill_package_roots: Vec, skill_directories: Vec, @@ -365,6 +367,7 @@ impl Runtime { subagents, session: Mutex::new(SessionSelection::default()), mcp: crate::tools::mcp::empty(), + plugin_runtime: None, skills, skill_package_roots: Vec::new(), skill_directories: Vec::new(), @@ -557,6 +560,28 @@ impl Runtime { Ok(Arc::new(runtime)) } + /// Installs a live Agent Plugin generation shared by MCP and skill tools. + pub fn with_plugin_runtime( + runtime: Arc, + plugins: Option, + ) -> Result, String> { + let Some(plugins) = plugins else { + return Ok(runtime); + }; + let snapshot = plugins.snapshot(); + let mut runtime = Arc::try_unwrap(runtime) + .map_err(|_| "could not configure plugins after runtime was shared".to_string())?; + runtime.skills = build_skill_tools( + &runtime.root, + &snapshot.package_roots, + &snapshot.skill_directories, + ); + runtime.skill_package_roots = snapshot.package_roots.clone(); + runtime.skill_directories = snapshot.skill_directories.clone(); + runtime.plugin_runtime = Some(plugins); + Ok(Arc::new(runtime)) + } + /// Preserves the original single-file behavior. `path` is the explicit MCP /// source and project-local configuration is not discovered. pub async fn with_mcp_config( @@ -566,7 +591,7 @@ impl Runtime { interactive_oauth_enabled: bool, credential_storage: crate::tools::mcp::CredentialStorage, ) -> Result, String> { - if path.is_none() && plugin_mcps.is_empty() { + if path.is_none() && plugin_mcps.is_empty() && runtime.plugin_runtime.is_none() { return Ok(runtime); } let sources = path @@ -666,13 +691,26 @@ impl Runtime { interactive_oauth_enabled: bool, credential_storage: crate::tools::mcp::CredentialStorage, ) -> Result, String> { - let mcp = crate::tools::mcp::connect( - install.sources, - &plugin_mcps, - interactive_oauth_enabled, - credential_storage.clone(), - ) - .await?; + let mcp = match runtime.plugin_runtime.clone() { + Some(plugins) => { + crate::tools::mcp::connect_dynamic( + install.sources, + plugins, + interactive_oauth_enabled, + credential_storage.clone(), + ) + .await? + } + None => { + crate::tools::mcp::connect( + install.sources, + &plugin_mcps, + interactive_oauth_enabled, + credential_storage.clone(), + ) + .await? + } + }; let mut runtime = Arc::try_unwrap(runtime) .map_err(|_| "could not configure MCP after runtime was shared".to_string())?; runtime.mcp = mcp; @@ -724,23 +762,57 @@ impl Runtime { self.compose_with(depth, self.subagents.fresh()) } - fn fresh_skills(&self) -> Arc { - build_skill_tools( - &self.root, - &self.skill_package_roots, - &self.skill_directories, + fn current_skill_paths(&self) -> (Vec, Vec) { + self.plugin_runtime.as_ref().map_or_else( + || { + ( + self.skill_package_roots.clone(), + self.skill_directories.clone(), + ) + }, + |plugins| { + let snapshot = plugins.snapshot(); + ( + snapshot.package_roots.clone(), + snapshot.skill_directories.clone(), + ) + }, ) } - pub(crate) async fn current_skills(&self) -> Vec { - let registry = build_skill_registry( - &self.root, - &self.skill_package_roots, - &self.skill_directories, - ) - .discover_skills() - .await; - registry.skills().into_iter().cloned().collect() + fn fresh_skills(&self) -> Arc { + let (package_roots, skill_directories) = self.current_skill_paths(); + build_skill_tools(&self.root, &package_roots, &skill_directories) + } + + pub(crate) async fn current_skills(&self) -> Result { + if self.plugin_runtime.is_some() { + self.mcp.refresh().await?; + } + let _generation = match &self.plugin_runtime { + Some(plugins) => Some(plugins.generation_lease().await), + None => None, + }; + let registry = build_skill_registry(&self.root, &[], &[]) + .discover_skills() + .await; + let mut skills = registry.skills().into_iter().cloned().collect::>(); + if let Some(plugins) = &self.plugin_runtime { + let mut names = skills + .iter() + .map(|skill| skill.name.clone()) + .collect::>(); + for skill in &plugins.snapshot().skills { + if names.insert(skill.name.clone()) { + skills.push(skill.clone()); + } + } + } + Ok(CurrentSkills { + skills, + _generation, + _plugin_runtime: self.plugin_runtime.clone(), + }) } fn compose_with(&self, depth: usize, subagents: Subagents) -> ComposeOnly { @@ -785,30 +857,30 @@ impl Runtime { .register(Observed::new(ToolSearch::new(self.mcp.clone()))) .register(Observed::new(AuthTool::new(self.mcp.clone()))) .register(Observed::new(McpTool::new(self.mcp.clone()))); - let mut child_specs = children.specs(); - let skill_tools = skills.tool_registry(); - if let Some(skill_tool) = skill_tools.get(&ToolName::new("skill")) { - let frozen_skill_spec = skill_spec_with_open_name( - skill_tool - .current_spec() - .unwrap_or_else(|| skill_tool.spec().clone()), - ); + if let Some(plugins) = &self.plugin_runtime { + let skill_tool: Arc = + Arc::new(DynamicSkillTool::new(self.root.clone(), plugins.clone())); children.register(observe_shared(skill_tool)); - child_specs.push(frozen_skill_spec); + } else { + let skill_tools = skills.tool_registry(); + if let Some(skill_tool) = skill_tools.get(&ToolName::new("skill")) { + children.register(observe_shared(skill_tool)); + } } + let hidden_tools = children.clone(); let compose = ComposeTool::wrap(children) - .with_source(self.mcp.catalog().unadvertised()) .with_config( ComposeConfig::new() .with_max_nested_tool_calls(128) .with_max_result_bytes(MAX_COMPOSE_RESULT_BYTES), ) - .with_backend(HiddenRunletBackend(child_specs)); + .with_backend(HiddenRunletBackend(hidden_tools.clone())); ComposeOnly { backgroundable: BackgroundableCompose::new( compose.clone(), background_jobs, self.root.clone(), + hidden_tools, ), compose, } @@ -828,6 +900,16 @@ impl Runtime { .clone() .ok_or_else(|| "persistent run requires a configured session".to_string())?; let session_id = request.id.clone(); + if self.plugin_runtime.is_some() { + self.mcp.refresh().await.map_err(|error| { + record_runtime_failure( + &session_id, + crate::fatal::Surface::Prompt, + "plugin_refresh", + error, + ) + })?; + } let initial = if request.resume { vec![Item::text(ItemKind::System, self.system_prompt(0))] } else { @@ -948,6 +1030,9 @@ impl Runtime { depth: usize, cancellation: Option, ) -> Result { + if self.plugin_runtime.is_some() { + self.mcp.refresh().await.map_err(LoopError::InvalidState)?; + } let session = format!("run-{}", NEXT_SESSION.fetch_add(1, Ordering::Relaxed)); let transcript = self .initial_transcript(depth) @@ -1068,8 +1153,9 @@ impl Runtime { self.openrouter_api_key.clone(), ) .map_err(AcpRuntimeError::Loop)?; + let current_skills = self.current_skills().await.map_err(AcpRuntimeError::Loop)?; let skills = self.fresh_skills(); - let skill_catalog = self.current_skills().await; + let skill_catalog = current_skills.skills; let compactor = crate::compaction::automatic( adapter.clone(), self.agentkit_telemetry(), @@ -1159,6 +1245,157 @@ impl Runtime { } } +pub(crate) struct CurrentSkills { + pub(crate) skills: Vec, + _generation: Option>, + _plugin_runtime: Option, +} + +struct DynamicSkillTool { + root: PathBuf, + plugins: crate::plugins::PluginRuntime, + spec: ToolSpec, +} + +impl DynamicSkillTool { + fn new(root: PathBuf, plugins: crate::plugins::PluginRuntime) -> Self { + let tool = default_skill_tool(&root); + Self { + root, + plugins, + spec: tool.spec().clone(), + } + } + + fn base(&self) -> Arc { + default_skill_tool(&self.root) + } + + fn requested_name(request: &ToolRequest) -> Result<&str, ToolError> { + request + .input + .get("name") + .and_then(Value::as_str) + .ok_or_else(|| ToolError::InvalidInput("skill name must be a string".into())) + } + + fn base_names(tool: &Arc) -> BTreeSet { + tool.current_spec() + .and_then(|spec| { + spec.input_schema["properties"]["name"]["enum"] + .as_array() + .cloned() + }) + .unwrap_or_default() + .into_iter() + .filter_map(|name| name.as_str().map(str::to_owned)) + .collect() + } + + fn merged_spec(&self) -> Option { + let base = self.base(); + let base_spec = base.current_spec(); + let mut names = Self::base_names(&base); + let snapshot = self.plugins.snapshot(); + let plugin_skills = snapshot + .skills + .iter() + .filter(|skill| names.insert(skill.name.clone())) + .collect::>(); + if names.is_empty() { + return None; + } + let mut spec = base_spec.unwrap_or_else(|| self.spec.clone()); + let mut names = names.into_iter().map(Value::String).collect::>(); + names.sort_by(|left, right| left.as_str().cmp(&right.as_str())); + spec.input_schema["properties"]["name"]["enum"] = Value::Array(names); + if !plugin_skills.is_empty() { + spec.description.push_str("\n\nAgent Plugin skills:\n"); + for skill in plugin_skills { + let _ = writeln!(spec.description, "- {}: {}", skill.name, skill.description); + } + } + Some(spec) + } +} + +fn default_skill_tool(root: &Path) -> Arc { + build_skill_registry(root, &[], &[]) + .tool_registry() + .get(&ToolName::new("skill")) + .expect("default skill registry always provides the skill tool") +} + +#[async_trait] +impl Tool for DynamicSkillTool { + fn spec(&self) -> &ToolSpec { + &self.spec + } + + fn current_spec(&self) -> Option { + let _generation = self.plugins.try_generation_lease()?; + self.merged_spec() + } + + fn proposed_requests( + &self, + request: &ToolRequest, + ) -> Result>, ToolError> { + let _generation = self.plugins.try_generation_lease().ok_or_else(|| { + ToolError::Unavailable( + "plugin generation is being refreshed; retry the skill call".into(), + ) + })?; + let base = self.base(); + if Self::base_names(&base).contains(Self::requested_name(request)?) { + base.proposed_requests(request) + } else { + Ok(Vec::new()) + } + } + + async fn invoke( + &self, + request: ToolRequest, + context: &mut ToolContext<'_>, + ) -> Result { + let _generation = self.plugins.generation_lease().await; + let name = Self::requested_name(&request)?.to_owned(); + let base = self.base(); + if Self::base_names(&base).contains(&name) { + return base.invoke(request, context).await; + } + let snapshot = self.plugins.snapshot(); + let skill = snapshot + .skills + .iter() + .find(|skill| skill.name == name) + .ok_or_else(|| ToolError::InvalidInput(format!("unknown skill: {name}")))?; + let mut response = format!( + "skill: {}\ndir: {}\n\n{}", + skill.name, + skill.base_dir.display(), + skill.body + ); + if !skill.resources.is_empty() { + response.push_str("\n\nresources:\n"); + for resource in &skill.resources { + let _ = writeln!(response, " - {}", resource.display()); + } + } + Ok(ToolResult { + result: ToolResultPart { + call_id: request.call_id, + output: ToolOutput::Text(response), + is_error: false, + metadata: MetadataMap::new(), + }, + duration: None, + metadata: MetadataMap::new(), + }) + } +} + fn build_skill_tools( root: &Path, package_roots: &[PathBuf], @@ -1549,16 +1786,23 @@ struct BackgroundableCompose { spec: ToolSpec, background_jobs: BackgroundJobs, root: PathBuf, + hidden_tools: agentkit_tools_core::ToolRegistry, } impl BackgroundableCompose { - fn new(inner: ComposeTool, background_jobs: BackgroundJobs, root: PathBuf) -> Self { + fn new( + inner: ComposeTool, + background_jobs: BackgroundJobs, + root: PathBuf, + hidden_tools: agentkit_tools_core::ToolRegistry, + ) -> Self { let spec = backgroundable_spec(inner.spec().clone()); Self { inner, spec, background_jobs, root, + hidden_tools, } } @@ -1588,7 +1832,10 @@ impl Tool for BackgroundableCompose { } fn current_spec(&self) -> Option { - self.inner.current_spec().map(backgroundable_spec) + self.inner.current_spec().map(|mut spec| { + spec.description = HiddenRunletBackend(self.hidden_tools.clone()).description(None); + backgroundable_spec(spec) + }) } fn proposed_requests( @@ -1780,7 +2027,23 @@ fn skill_spec_with_open_name(mut spec: ToolSpec) -> ToolSpec { spec } -struct HiddenRunletBackend(Vec); +struct HiddenRunletBackend(agentkit_tools_core::ToolRegistry); + +impl HiddenRunletBackend { + fn specs(&self) -> Vec { + self.0 + .specs() + .into_iter() + .map(|spec| { + if spec.name == ToolName::new("skill") { + skill_spec_with_open_name(spec) + } else { + spec + } + }) + .collect() + } +} #[async_trait] impl ComposeBackend for HiddenRunletBackend { @@ -1794,7 +2057,7 @@ impl ComposeBackend for HiddenRunletBackend { "\n\nHidden callable tools. Each entry includes the exact compact JSON schemas \ used by Runlet for input checking and output typing:", ); - for spec in &self.0 { + for spec in self.specs() { let _ = write!( description, "\n\n- `{}`: {}\n Input JSON schema: `{}`\n Output JSON schema: `{}`", @@ -1812,7 +2075,7 @@ impl ComposeBackend for HiddenRunletBackend { } async fn execute(&self, mut run: BackendRun) -> Result { - run.visible_specs.clone_from(&self.0); + run.visible_specs = self.specs(); RunletBackend.execute(run).await } } diff --git a/src/runtime/tests.rs b/src/runtime/tests.rs index 0ffa73e..ce7988c 100644 --- a/src/runtime/tests.rs +++ b/src/runtime/tests.rs @@ -11,8 +11,8 @@ use agentkit_tools_core::{ use serde_json::{Value, json}; use super::{ - BackgroundJobs, BackgroundableCompose, DetachRegistration, Runtime, SessionRequest, - SessionSelection, background_route, load_initial_transcript, + BackgroundJobs, BackgroundableCompose, DetachRegistration, DynamicSkillTool, Runtime, + SessionRequest, SessionSelection, background_route, load_initial_transcript, }; #[tokio::test] @@ -603,6 +603,152 @@ fn project_skills_take_precedence_over_plugin_skills() { assert!(!catalog.contains("Plugin version.")); } +#[tokio::test] +async fn live_plugin_skill_tool_adds_changes_and_removes_skills() { + let root = tempfile::tempdir().unwrap(); + let config = root.path().join("config.toml"); + std::fs::write(&config, "").unwrap(); + let plugins = crate::plugins::PluginRuntime::load( + config.clone(), + root.path().to_path_buf(), + root.path().join("cache"), + root.path().join("data"), + ) + .await + .unwrap(); + let runtime = Runtime::with_plugin_runtime( + Runtime::new(root.path(), "gpt-5.4").unwrap(), + Some(plugins.clone()), + ) + .unwrap(); + let runtime = Runtime::with_mcp_config( + runtime, + None, + Vec::new(), + false, + crate::tools::mcp::CredentialStorage::Memory, + ) + .await + .unwrap(); + let compose = runtime.compose(0); + let tool = DynamicSkillTool::new(root.path().to_path_buf(), plugins.clone()); + assert!( + !tool + .current_spec() + .map(|spec| spec.input_schema.to_string()) + .unwrap_or_default() + .contains("live-skill") + ); + + let package = root.path().join("plugin"); + let skill = package.join("skills/live-skill"); + std::fs::create_dir_all(&package).unwrap(); + std::fs::write( + package.join("plugin.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/plugin.schema.json","name":"live-plugin"}"#, + ) + .unwrap(); + write_skill(&skill, "live-skill", "Live skill.", "first body"); + std::fs::write( + &config, + format!( + "[plugins.live]\nsource = 'path'\npath = '{}'\n", + package.display() + ), + ) + .unwrap(); + let refreshed = runtime.current_skills().await.unwrap(); + assert!( + refreshed + .skills + .iter() + .any(|skill| skill.name == "live-skill") + ); + drop(refreshed); + assert!( + tool.current_spec() + .unwrap() + .input_schema + .to_string() + .contains("live-skill") + ); + assert!(compose.specs()[0].description.contains("live-skill")); + + let session_id = SessionId::new("session"); + let turn_id = TurnId::new("turn"); + let owned = OwnedToolContext { + session_id: session_id.clone(), + turn_id: turn_id.clone(), + metadata: MetadataMap::new(), + permissions: Arc::new(AllowAllPermissions), + resources: Arc::new(()), + cancellation: None, + execution_scope: None, + approved_request: None, + }; + let invoke = |id: &str| { + ToolRequest::new( + ToolCallId::new(id), + ToolName::new("skill"), + json!({"name": "live-skill"}), + session_id.clone(), + turn_id.clone(), + ) + }; + let first = tool + .invoke(invoke("first"), &mut owned.borrowed()) + .await + .unwrap(); + assert!(format!("{:?}", first.result.output).contains("first body")); + + let published = plugins.snapshot(); + let generation = published.package_roots[0] + .parent() + .expect("snapshot package has a generation root"); + crate::plugins::make_tree_writable_for_test(generation); + std::fs::write( + published.skill_directories[0].join("SKILL.md"), + "poisoned cache body", + ) + .unwrap(); + let still_immutable = tool + .invoke(invoke("cache-poison"), &mut owned.borrowed()) + .await + .unwrap(); + assert!(format!("{:?}", still_immutable.result.output).contains("first body")); + assert!(!format!("{:?}", still_immutable.result.output).contains("poisoned")); + drop(published); + + write_skill(&skill, "live-skill", "Live skill.", "second body"); + let still_first = tool + .invoke(invoke("before-refresh"), &mut owned.borrowed()) + .await + .unwrap(); + assert!(format!("{:?}", still_first.result.output).contains("first body")); + let refreshed = runtime.current_skills().await.unwrap(); + assert!( + refreshed + .skills + .iter() + .any(|skill| skill.body.contains("second body")) + ); + drop(refreshed); + let second = tool + .invoke(invoke("second"), &mut owned.borrowed()) + .await + .unwrap(); + assert!(format!("{:?}", second.result.output).contains("second body")); + + std::fs::write(&config, "").unwrap(); + drop(runtime.current_skills().await.unwrap()); + assert!(!compose.specs()[0].description.contains("live-skill")); + assert!( + tool.invoke(invoke("removed"), &mut owned.borrowed()) + .await + .is_err() + ); +} + #[tokio::test] async fn compose_can_load_a_skill_added_after_its_spec_is_frozen() { let root = tempfile::tempdir().unwrap(); @@ -702,6 +848,43 @@ fn compose_is_the_only_visible_tool_and_documents_mcp_meta_tools() { assert!(!specs[0].description.contains("mcp_filesystem_read_file")); } +#[tokio::test] +async fn exact_mcp_name_cannot_bypass_the_tool_meta_dispatch() { + let root = tempfile::tempdir().unwrap(); + let runtime = Runtime::new(root.path(), "gpt-5.4").unwrap(); + let compose = runtime.compose(0); + assert!(ToolSource::get(&compose.compose, &ToolName::new("mcp_exact_tool")).is_none()); + let session_id = SessionId::new("session"); + let turn_id = TurnId::new("turn"); + let owned = OwnedToolContext { + session_id: session_id.clone(), + turn_id: turn_id.clone(), + metadata: MetadataMap::new(), + permissions: Arc::new(AllowAllPermissions), + resources: Arc::new(()), + cancellation: None, + execution_scope: None, + approved_request: None, + }; + let outcome = compose + .backgroundable + .invoke_outcome( + ToolRequest::new( + ToolCallId::new("call"), + ToolName::new("compose"), + json!({"script": "return mcp_exact_tool({})"}), + session_id, + turn_id, + ), + &mut owned.borrowed(), + ) + .await; + assert!(matches!( + outcome, + ToolExecutionOutcome::FailedBeforeInvocation(_) | ToolExecutionOutcome::Failed(_) + )); +} + #[test] fn maximum_depth_compose_omits_depth_increasing_tools() { let root = tempfile::tempdir().unwrap(); diff --git a/src/tools/mcp.rs b/src/tools/mcp.rs index c1ee10a..84c5c36 100644 --- a/src/tools/mcp.rs +++ b/src/tools/mcp.rs @@ -7,7 +7,7 @@ use std::{ collections::{BTreeMap, BTreeSet}, path::{Path, PathBuf}, sync::{ - Arc, + Arc, Weak, atomic::{AtomicU64, Ordering}, }, time::{Duration, Instant}, @@ -28,7 +28,9 @@ use async_trait::async_trait; use rmcp::transport::auth::AuthorizationManager; use serde::{Deserialize, Deserializer}; use serde_json::{Value, json}; -use tokio::sync::{Mutex, OwnedMutexGuard, RwLock, Semaphore, mpsc, oneshot}; +use tokio::sync::{ + Mutex, OwnedRwLockReadGuard, OwnedRwLockWriteGuard, RwLock, Semaphore, mpsc, oneshot, +}; const CONNECT_TIMEOUT: Duration = Duration::from_secs(20); const DEFAULT_TOOL_TIMEOUT: Duration = Duration::from_secs(60); @@ -136,11 +138,13 @@ struct Inner { oauth_sessions: OAuthSessions, active_replays: ActiveReplays, credential_storage: CredentialStorage, + plugin_source: Option, reload: Mutex, reload_flight: Arc, + reload_epoch: AtomicU64, initialization: Mutex<()>, auth_setup: Mutex<()>, - operations: Mutex>>>, + operations: Mutex>>>, event_routes: EventRoutes, next_event_route: AtomicU64, interactive_oauth_enabled: bool, @@ -153,6 +157,7 @@ struct ServerRecord { oauth: Option, static_authorization: bool, fingerprint: Vec, + plugin_owned: bool, status: ServerStatus, } @@ -161,6 +166,7 @@ struct ReloadState { entries: BTreeMap>, plugins: BTreeMap, plugin_entries: BTreeMap>, + plugin_source: Option, } #[derive(Clone)] @@ -668,6 +674,7 @@ fn prepare_server( oauth: None, static_authorization: false, fingerprint, + plugin_owned: false, status: ServerStatus::Uninitialized, }, )) @@ -703,6 +710,7 @@ fn prepare_server( oauth: server.auth, static_authorization, fingerprint, + plugin_owned: false, status: ServerStatus::Uninitialized, }, )) @@ -793,11 +801,10 @@ fn prepare_plugins( let data = plugin_text(&plugin.data_dir, "data", &plugin.alias)?; for server in &plugin.servers { if matches!(server.transport, PluginMcpTransport::Sse { .. }) { - eprintln!( - "plugin {}: MCP server {:?} uses unsupported SSE transport; skipping", + return Err(format!( + "plugin {}: MCP server {:?} uses unsupported SSE transport", plugin.alias, server.name - ); - continue; + )); } if let Some(owner) = owners.insert(server.name.clone(), plugin.alias.clone()) { return Err(format!( @@ -806,8 +813,6 @@ fn prepare_plugins( )); } let description = format!("{} plugin MCP server", plugin.manifest_name); - let fingerprint = - format!("plugin:{}:{:?}", plugin.alias, server.transport).into_bytes(); let (binding, url, static_authorization) = match &server.transport { PluginMcpTransport::Stdio { command, @@ -901,8 +906,13 @@ fn prepare_plugins( .any(|name| name.eq_ignore_ascii_case("authorization")), ) } - PluginMcpTransport::Sse { .. } => unreachable!("SSE was skipped above"), + PluginMcpTransport::Sse { .. } => unreachable!("SSE was rejected above"), }; + 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(), @@ -914,6 +924,7 @@ fn prepare_plugins( oauth: None, static_authorization, fingerprint, + plugin_owned: true, status: ServerStatus::Uninitialized, }, }, @@ -962,7 +973,40 @@ pub(crate) async fn connect( interactive_oauth_enabled: bool, credential_storage: CredentialStorage, ) -> Result { - let sources = sources.into_config_sources(); + connect_inner( + sources.into_config_sources(), + plugins, + None, + interactive_oauth_enabled, + credential_storage, + ) + .await +} + +pub(crate) async fn connect_dynamic( + sources: impl IntoConfigSources, + plugins: crate::plugins::PluginRuntime, + interactive_oauth_enabled: bool, + credential_storage: CredentialStorage, +) -> Result { + let snapshot = plugins.snapshot(); + connect_inner( + sources.into_config_sources(), + &snapshot.mcp_plugins, + Some(plugins), + interactive_oauth_enabled, + credential_storage, + ) + .await +} + +async fn connect_inner( + sources: Vec, + plugins: &[crate::plugins::ResolvedPluginMcp], + plugin_source: Option, + interactive_oauth_enabled: bool, + credential_storage: CredentialStorage, +) -> Result { let (plugin_prepared, plugin_entries) = prepare_plugins(plugins)?; let mut source_states = Vec::with_capacity(sources.len()); for source in sources { @@ -1007,6 +1051,7 @@ pub(crate) async fn connect( entries, plugins: plugin_prepared, plugin_entries, + plugin_source, }, interactive_oauth_enabled, ); @@ -1026,6 +1071,7 @@ impl McpRuntime { ) -> Self { let catalog = manager.source(); let (oauth_sessions, active_replays) = oauth; + let plugin_source = reload.plugin_source.clone(); Self { inner: Arc::new(Inner { manager: Mutex::new(manager), @@ -1036,8 +1082,10 @@ impl McpRuntime { oauth_sessions, active_replays, credential_storage, + plugin_source, reload: Mutex::new(reload), reload_flight: Arc::new(Semaphore::new(1)), + reload_epoch: AtomicU64::new(0), initialization: Mutex::new(()), auth_setup: Mutex::new(()), operations: Mutex::new(BTreeMap::new()), @@ -1052,17 +1100,18 @@ impl McpRuntime { self.inner.catalog.clone() } - async fn operation_gate(&self, server: &str) -> Arc> { - self.inner - .operations - .lock() - .await - .entry(server.to_string()) - .or_insert_with(|| Arc::new(Mutex::new(()))) - .clone() + async fn operation_gate(&self, server: &str) -> Arc> { + let mut operations = self.inner.operations.lock().await; + operations.retain(|_, gate| gate.strong_count() > 0); + if let Some(gate) = operations.get(server).and_then(Weak::upgrade) { + return gate; + } + let gate = Arc::new(RwLock::new(())); + operations.insert(server.to_string(), Arc::downgrade(&gate)); + gate } - async fn server_for_tool(&self, tool: &str) -> Option<(String, Vec, bool)> { + async fn server_for_tool(&self, tool: &str) -> Option<(String, Vec, bool, bool)> { self.inner .servers .read() @@ -1075,10 +1124,45 @@ impl McpRuntime { server.clone(), record.fingerprint.clone(), serializes_tool_calls(record), + record.plugin_owned, ) }) } + async fn acquire_invocation(&self, tool: &str) -> Result { + let Some((server, fingerprint, serializes, plugin_owned)) = + self.server_for_tool(tool).await + else { + return Err(ToolError::Unavailable(format!( + "MCP tool is no longer available: {tool}" + ))); + }; + let gate = self.operation_gate(&server).await; + let operation = if serializes { + McpOperationGuard::Exclusive(gate.write_owned().await) + } else { + McpOperationGuard::Shared(gate.read_owned().await) + }; + let generation = match (&self.inner.plugin_source, plugin_owned) { + (Some(source), true) => Some(source.generation_lease().await), + _ => None, + }; + let current = self.server_for_tool(tool).await; + if !current.is_some_and(|(current_server, current_fingerprint, _, _)| { + current_server == server && current_fingerprint == fingerprint + }) { + return Err(ToolError::Unavailable( + "MCP configuration changed while the tool was waiting; retry the call".into(), + )); + } + Ok(McpInvocationLease { + server, + fingerprint, + _operation: operation, + _generation: generation, + }) + } + async fn finish_tool_call( &self, server: &str, @@ -1187,124 +1271,186 @@ impl McpRuntime { .collect() } + pub(crate) async fn refresh(&self) -> Result<(), String> { + self.reload_config().await + } + async fn reload_config(&self) -> Result<(), String> { let permit = Arc::clone(&self.inner.reload_flight) .acquire_owned() .await .map_err(|_| "MCP config reload coordinator closed".to_string())?; + let current = self.inner.reload_epoch.load(Ordering::Acquire); let runtime = self.clone(); tokio::spawn(async move { + // The spawned task owns completion so caller cancellation cannot + // strand the reload permit. Every waiter reruns after acquiring the + // permit: a change can arrive after the preceding reload staged its + // inputs but before that reload publishes. let _permit = permit; - runtime.reload_config_inner().await + let result = runtime + .reload_config_inner() + .await + .map_err(crate::plugins::bounded_diagnostic); + runtime + .inner + .reload_epoch + .store(current.wrapping_add(1), Ordering::Release); + result }) .await .map_err(|error| format!("MCP config reload task failed: {error}"))? } async fn reload_config_inner(&self) -> Result<(), String> { - let mut state = self.inner.reload.lock().await; - let mut next_sources = Vec::with_capacity(state.sources.len()); - for current in &state.sources { - next_sources.push(SourceState { - source: current.source.clone(), - raw: read_source(¤t.source).await?, - }); - } - if next_sources - .iter() - .zip(&state.sources) - .all(|(next, current)| next.raw == current.raw) - { - return Ok(()); - } + loop { + // Snapshot the published generation, then perform all config reads, + // downloads, and package resolution without holding runtime locks. + let ( + current_sources, + current_entries, + current_plugins, + current_plugin_entries, + plugin_source, + ) = { + let state = self.inner.reload.lock().await; + ( + state.sources.clone(), + state.entries.clone(), + state.plugins.clone(), + state.plugin_entries.clone(), + state.plugin_source.clone(), + ) + }; + let staged_plugins = match &plugin_source { + Some(source) => Some(source.stage().await?), + None => None, + }; + let (plugin_prepared, plugin_entries) = match &staged_plugins { + Some(staged) => prepare_plugins(&staged.resolved.mcp_plugins)?, + None => (current_plugins, current_plugin_entries), + }; + let mut next_sources = Vec::with_capacity(current_sources.len()); + for current in ¤t_sources { + next_sources.push(SourceState { + source: current.source.clone(), + raw: read_source(¤t.source).await?, + }); + } - // Validate every source before changing live state, then layer them in - // declared precedence order over the immutable plugin baseline. - let (mut prepared, entries) = - prepare_sources(&next_sources, &state.plugins, &state.plugin_entries)?; - validate_server_names(prepared.keys())?; - let changed = state - .entries - .keys() - .chain(entries.keys()) - .filter(|name| state.entries.get(*name) != entries.get(*name)) - .cloned() - .collect::>(); - if changed.is_empty() { - state.sources = next_sources; - return Ok(()); - } - drop(state); - let gates = { - let mut operations = self.inner.operations.lock().await; - changed - .iter() - .map(|name| { - operations - .entry(name.clone()) - .or_insert_with(|| Arc::new(Mutex::new(()))) - .clone() - }) - .collect::>() - }; - let mut operation_guards = Vec::with_capacity(gates.len()); - for gate in &gates { - operation_guards.push( - tokio::time::timeout(CONNECT_TIMEOUT, gate.lock()) + // Configured, project, and explicit files retain their declared + // precedence over the newly staged plugin baseline. + let (mut prepared, entries) = + prepare_sources(&next_sources, &plugin_prepared, &plugin_entries)?; + validate_server_names(prepared.keys())?; + let changed = current_entries + .keys() + .chain(entries.keys()) + .filter(|name| current_entries.get(*name) != entries.get(*name)) + .cloned() + .collect::>(); + let deleted = current_entries + .keys() + .filter(|name| !entries.contains_key(*name)) + .cloned() + .collect::>(); + + let gates = if changed.is_empty() { + Vec::new() + } else { + let mut gates = Vec::with_capacity(changed.len()); + for name in &changed { + gates.push(self.operation_gate(name).await); + } + gates + }; + let mut operation_guards = Vec::with_capacity(gates.len()); + let mut blocked = None; + for gate in &gates { + match gate.clone().try_write_owned() { + Ok(guard) => operation_guards.push(guard), + Err(_) => { + blocked = Some(gate.clone()); + break; + } + } + } + if let Some(gate) = blocked { + drop(operation_guards); + let waited = tokio::time::timeout(CONNECT_TIMEOUT, gate.write_owned()) .await - .map_err(|_| "timed out waiting for an in-flight MCP operation".to_string())?, - ); - } - let mut state = self.inner.reload.lock().await; - let _initialization = self.inner.initialization.lock().await; - for expected in &next_sources { - if read_source(&expected.source).await? != expected.raw { - return Err( - "MCP config changed while reload was waiting; retry the operation".into(), - ); + .map_err(|_| "timed out waiting for an in-flight MCP operation".to_string())?; + drop(waited); + continue; } - } + let generation_writer = match &plugin_source { + Some(source) => Some(source.generation_writer().await), + None => None, + }; + let mut initialization_guard = None; + if !changed.is_empty() { + initialization_guard = Some(self.inner.initialization.lock().await); - { - let mut manager = self.inner.manager.lock().await; - for name in &changed { - let _ = manager.unregister_server(&McpServerId::new(name)).await; - } - for name in &changed { - if let Some(server) = prepared.get(name) { - manager.register_server_with_options( - server.config.clone(), - McpServerOptions::new().with_timeout(CONNECT_TIMEOUT), - ); + { + let mut manager = self.inner.manager.lock().await; + for name in &changed { + let _ = manager.unregister_server(&McpServerId::new(name)).await; + } + for name in &changed { + if let Some(server) = prepared.get(name) { + manager.register_server_with_options( + server.config.clone(), + McpServerOptions::new().with_timeout(CONNECT_TIMEOUT), + ); + } + } + } + { + let mut servers = self.inner.servers.write().await; + for name in &changed { + servers.remove(name); + if let Some(server) = prepared.remove(name) { + servers.insert(name.clone(), server.record); + } + } + } + { + let mut challenges = self.inner.challenges.lock().await; + let mut pending = self.inner.pending.lock().await; + let mut oauth_sessions = self.inner.oauth_sessions.lock().await; + let mut active_replays = self.inner.active_replays.lock().await; + for name in &changed { + challenges.remove(name); + if let Some(pending) = pending.remove(name) { + pending.abort.abort(); + } + oauth_sessions.remove(name); + active_replays.remove(name); + } } } - } - { - let mut servers = self.inner.servers.write().await; - for name in &changed { - servers.remove(name); - if let Some(server) = prepared.remove(name) { - servers.insert(name.clone(), server.record); + + { + let mut state = self.inner.reload.lock().await; + state.sources = next_sources; + state.entries = entries; + state.plugins = plugin_prepared; + state.plugin_entries = plugin_entries; + if let (Some(source), Some(staged)) = (&plugin_source, staged_plugins) { + source.publish(staged.resolved); } } - } - { - let mut challenges = self.inner.challenges.lock().await; - let mut pending = self.inner.pending.lock().await; - let mut oauth_sessions = self.inner.oauth_sessions.lock().await; - let mut active_replays = self.inner.active_replays.lock().await; - for name in &changed { - challenges.remove(name); - if let Some(pending) = pending.remove(name) { - pending.abort.abort(); + drop(initialization_guard); + drop(generation_writer); + if !deleted.is_empty() { + let mut operations = self.inner.operations.lock().await; + for name in deleted { + operations.remove(&name); } - oauth_sessions.remove(name); - active_replays.remove(name); } + drop(operation_guards); + return Ok(()); } - state.sources = next_sources; - state.entries = entries; - Ok(()) } fn spawn_eager_initialization(&self) { @@ -1505,8 +1651,9 @@ impl McpRuntime { async fn authorize(&self, name: &str, session_id: String) -> Result { self.reload_config().await.map_err(ToolError::Unavailable)?; + self.initialize_uninitialized().await; let operation = self.operation_gate(name).await; - let _operation = tokio::time::timeout(CONNECT_TIMEOUT, operation.lock()) + let _operation = tokio::time::timeout(CONNECT_TIMEOUT, operation.write()) .await .map_err(|_| { ToolError::Unavailable(format!( @@ -1518,7 +1665,6 @@ impl McpRuntime { "interactive MCP authentication requires the tui, serve, or acp command".into(), )); } - let _reload = self.inner.reload.lock().await; self.initialize_servers(&[name.to_string()]).await; let _setup = self.inner.auth_setup.lock().await; if let Some(pending) = self.inner.pending.lock().await.get(name).cloned() { @@ -1664,7 +1810,7 @@ impl McpRuntime { ) { let finished = auth::finish(pending).await; let operation = self.operation_gate(&server).await; - let _operation = operation.lock().await; + let _operation = operation.write().await; let _reload = self.inner.reload.lock().await; let current = self .inner @@ -1948,11 +2094,24 @@ pub struct McpTool { spec: ToolSpec, } +#[allow(dead_code)] +enum McpOperationGuard { + Shared(OwnedRwLockReadGuard<()>), + Exclusive(OwnedRwLockWriteGuard<()>), +} + +struct McpInvocationLease { + server: String, + fingerprint: Vec, + _operation: McpOperationGuard, + _generation: Option>, +} + struct ExecutedMcpCall { outcome: ToolExecutionOutcome, replay: ReplayCleanup, server: Option<(String, Vec)>, - _operation: Option>, + _invocation: Option, } impl McpTool { @@ -2081,26 +2240,18 @@ impl McpTool { tool_name: ToolName, args: Value, ) -> ExecutedMcpCall { - let initial_server = self.runtime.server_for_tool(&name).await; - let server_name = initial_server.as_ref().map(|(server, _, _)| server.clone()); - let operation = match initial_server.as_ref() { - Some((server, _, true)) => Some(self.runtime.operation_gate(server).await), - _ => None, - }; - let operation = match operation { - Some(operation) => Some(operation.lock_owned().await), - None => None, + let invocation = match self.runtime.acquire_invocation(&name).await { + Ok(invocation) => invocation, + Err(error) => { + return ExecutedMcpCall { + outcome: ToolExecutionOutcome::FailedBeforeInvocation(error), + replay: ReplayCleanup::new(None, self.runtime.clone(), None), + server: None, + _invocation: None, + }; + } }; - let server = self - .runtime - .server_for_tool(&name) - .await - .filter(|(server, _, _)| { - server_name - .as_ref() - .is_some_and(|expected| server == expected) - }) - .map(|(server, fingerprint, _)| (server, fingerprint)); + let server = Some((invocation.server.clone(), invocation.fingerprint.clone())); let replay = ReplayCleanup::new( match server.as_ref() { Some((server, _)) => self @@ -2132,7 +2283,7 @@ impl McpTool { outcome, replay, server, - _operation: operation, + _invocation: Some(invocation), } } @@ -2141,7 +2292,7 @@ impl McpTool { outcome, replay, server, - _operation, + _invocation, } = call; let succeeded = matches!(outcome, ToolExecutionOutcome::Completed(_)); let error = match &outcome { @@ -2234,6 +2385,7 @@ pub fn empty() -> McpRuntime { entries: BTreeMap::new(), plugins: BTreeMap::new(), plugin_entries: BTreeMap::new(), + plugin_source: None, }, true, ) @@ -2627,7 +2779,12 @@ fn render_search( #[cfg(test)] mod tests { - use std::{collections::BTreeMap, path::Path, sync::Arc, time::Duration}; + use std::{ + collections::BTreeMap, + path::Path, + sync::{Arc, atomic::Ordering}, + time::Duration, + }; use agentkit_core::MetadataMap; use agentkit_mcp::{ @@ -2653,7 +2810,7 @@ mod tests { prepare_config, prepare_plugins, regular_term_score, serializes_tool_calls, validate_server_names, }; - use crate::plugins::ResolvedPluginMcp; + use crate::plugins::{PluginRuntime, ResolvedPluginMcp, ResolvedPlugins}; fn spec(name: &str, description: &str) -> ToolSpec { ToolSpec::new(ToolName::new(name), description, json!({"type": "object"})) @@ -2683,6 +2840,7 @@ mod tests { oauth: None, static_authorization: false, fingerprint: vec![1], + plugin_owned: false, status: ServerStatus::Connected, } } @@ -3048,20 +3206,75 @@ mod tests { let first = runtime.operation_gate("first").await; let same = runtime.operation_gate("first").await; let other = runtime.operation_gate("other").await; - let _held = first.lock().await; + let _held = first.write().await; assert!( - tokio::time::timeout(std::time::Duration::from_millis(25), other.lock()) + tokio::time::timeout(std::time::Duration::from_millis(25), other.write()) .await .is_ok() ); assert!( - tokio::time::timeout(std::time::Duration::from_millis(25), same.lock()) + tokio::time::timeout(std::time::Duration::from_millis(25), same.write()) .await .is_err() ); } + #[tokio::test] + async fn concurrent_safe_invocations_share_the_reload_gate() { + let runtime = super::empty(); + let mut record = connected_oauth_record(); + record.url = None; + runtime + .inner + .servers + .write() + .await + .insert("same".into(), record); + let first = runtime.acquire_invocation("mcp_same_read").await.unwrap(); + let second = tokio::time::timeout( + Duration::from_millis(25), + runtime.acquire_invocation("mcp_same_other"), + ) + .await + .expect("concurrent-safe MCP invocation was serialized") + .unwrap(); + drop((first, second)); + } + + #[tokio::test] + async fn same_name_fingerprint_change_rejects_waiting_invocation() { + let runtime = super::empty(); + runtime + .inner + .servers + .write() + .await + .insert("same".into(), connected_oauth_record()); + let gate = runtime.operation_gate("same").await; + let held = gate.write().await; + let waiting = { + let runtime = runtime.clone(); + tokio::spawn(async move { runtime.acquire_invocation("mcp_same_write").await }) + }; + tokio::time::sleep(Duration::from_millis(25)).await; + runtime + .inner + .servers + .write() + .await + .get_mut("same") + .unwrap() + .fingerprint = vec![2]; + drop(held); + + let error = match waiting.await.unwrap() { + Ok(_) => panic!("waiting invocation accepted a changed fingerprint"), + Err(error) => error, + }; + assert!(error.to_string().contains("configuration changed")); + } + #[tokio::test] async fn rejected_replay_preserves_interactive_auth_recovery() { let runtime = super::empty(); @@ -3109,7 +3322,7 @@ mod tests { .await .unwrap(); let gate = runtime.operation_gate("local").await; - let held = gate.lock().await; + let held = gate.write().await; std::fs::write(&path, r#"{"mcpServers":{"local":{"command":"unused"}}}"#).unwrap(); let reloading = { let runtime = runtime.clone(); @@ -3139,8 +3352,126 @@ mod tests { ) .unwrap(); drop(held); - let error = reloading.await.unwrap().unwrap_err(); - assert!(error.contains("changed while reload was waiting")); + // The staged snapshot may publish after the gate clears, but config I/O + // never runs while runtime or operation locks are held. The next + // boundary observes an edit that arrived during the wait. + reloading.await.unwrap().unwrap(); + runtime.reload_config().await.unwrap(); + } + + #[tokio::test] + async fn queued_reload_rereads_changes_that_arrived_during_the_prior_reload() { + let directory = tempfile::tempdir().unwrap(); + let config = directory.path().join("config.toml"); + let package = directory.path().join("plugin"); + std::fs::create_dir_all(&package).unwrap(); + std::fs::write( + package.join("plugin.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/plugin.schema.json","name":"queued-plugin"}"#, + ) + .unwrap(); + std::fs::write(&config, "").unwrap(); + let plugins = PluginRuntime::new( + config.clone(), + directory.path().to_path_buf(), + directory.path().join("cache"), + directory.path().join("data"), + ResolvedPlugins::default(), + ); + let runtime = super::connect_dynamic( + None::<&Path>, + plugins.clone(), + true, + CredentialStorage::Memory, + ) + .await + .unwrap(); + let write_mcp = |url: &str| { + std::fs::write( + package.join("mcp.json"), + format!( + r#"{{"$schema":"https://agent-plugins.org/schemas/1.0.0/mcp.schema.json","mcpServers":{{"live":{{"type":"streamable-http","url":"{url}"}}}}}}"# + ), + ) + .unwrap(); + }; + write_mcp("https://example.com/first"); + std::fs::write( + &config, + format!( + "[plugins.queued]\nsource = 'path'\npath = '{}'\n", + package.display() + ), + ) + .unwrap(); + + let generation = plugins.generation_lease().await; + let gate = runtime.operation_gate("live").await; + let first = { + let runtime = runtime.clone(); + tokio::spawn(async move { runtime.reload_config().await }) + }; + tokio::time::timeout(Duration::from_secs(1), async { + loop { + if gate.clone().try_read_owned().is_err() { + break; + } + tokio::task::yield_now().await; + } + }) + .await + .expect("first reload did not reach plugin publication"); + + write_mcp("https://example.com/second"); + let second = { + let runtime = runtime.clone(); + tokio::spawn(async move { runtime.reload_config().await }) + }; + drop(generation); + first.await.unwrap().unwrap(); + second.await.unwrap().unwrap(); + + assert_eq!( + runtime.inner.servers.read().await["live"].url.as_deref(), + Some("https://example.com/second") + ); + } + + #[tokio::test] + async fn cancelled_reload_caller_does_not_strand_completion() { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("mcp.json"); + std::fs::write(&path, r#"{"mcpServers":{}}"#).unwrap(); + let runtime = super::connect(Some(&path), &[], true, CredentialStorage::Memory) + .await + .unwrap(); + let gate = runtime.operation_gate("local").await; + let held = gate.write().await; + std::fs::write(&path, r#"{"mcpServers":{"local":{"command":"unused"}}}"#).unwrap(); + let epoch = runtime.inner.reload_epoch.load(Ordering::Acquire); + let caller = { + let runtime = runtime.clone(); + tokio::spawn(async move { runtime.reload_config().await }) + }; + tokio::time::timeout(Duration::from_secs(1), async { + while runtime.inner.reload_flight.available_permits() != 0 { + tokio::task::yield_now().await; + } + }) + .await + .unwrap(); + tokio::time::sleep(Duration::from_millis(25)).await; + caller.abort(); + drop(held); + + tokio::time::timeout(Duration::from_secs(1), async { + while runtime.inner.reload_epoch.load(Ordering::Acquire) == epoch { + tokio::task::yield_now().await; + } + }) + .await + .expect("detached reload completion did not publish its epoch"); + assert!(runtime.inner.servers.read().await.contains_key("local")); runtime.reload_config().await.unwrap(); } @@ -3193,6 +3524,7 @@ mod tests { oauth: None, static_authorization: false, fingerprint: vec![1], + plugin_owned: false, status, } } @@ -3564,36 +3896,26 @@ mod tests { } #[test] - fn plugin_mcp_expands_stdio_paths_and_skips_sse() { + fn plugin_mcp_expands_stdio_paths() { let directory = tempfile::tempdir().unwrap(); let root = directory.path().canonicalize().unwrap(); let plugin = plugin( "tools", directory.path(), - vec![ - PluginMcpServer { - name: "local".into(), - transport: PluginMcpTransport::Stdio { - command: "./bin/server".into(), - args: vec!["${PLUGIN_DATA}/db".into()], - env: std::collections::BTreeMap::from([( - "ROOT_COPY".into(), - "${PLUGIN_ROOT}".into(), - )]), - cwd: Some("${PLUGIN_DATA}/work".into()), - }, - }, - PluginMcpServer { - name: "legacy".into(), - transport: PluginMcpTransport::Sse { - url: "https://example.com/sse".into(), - headers: Default::default(), - }, + vec![PluginMcpServer { + name: "local".into(), + transport: PluginMcpTransport::Stdio { + command: "./bin/server".into(), + args: vec!["${PLUGIN_DATA}/db".into()], + env: std::collections::BTreeMap::from([( + "ROOT_COPY".into(), + "${PLUGIN_ROOT}".into(), + )]), + cwd: Some("${PLUGIN_DATA}/work".into()), }, - ], + }], ); let (prepared, _) = prepare_plugins(&[plugin]).unwrap(); - assert!(!prepared.contains_key("legacy")); let McpTransportBinding::Stdio(transport) = &prepared["local"].config.transport else { panic!("expected stdio transport"); }; @@ -3615,6 +3937,23 @@ mod tests { ); } + #[test] + fn plugin_mcp_rejects_sse_without_partial_publication() { + 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(), + }, + }], + ); + assert!(prepare_plugins(&[plugin]).is_err()); + } + #[test] fn plugin_mcp_preserves_transport_defaults_and_http_literals() { assert_eq!( @@ -3937,6 +4276,36 @@ mod tests { assert_eq!(record.url.as_deref(), Some("https://example.com/mcp")); } + #[tokio::test] + async fn removed_server_operation_gate_is_reclaimed() { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("mcp.json"); + std::fs::write(&path, r#"{"mcpServers":{"removed":{"command":"unused"}}}"#).unwrap(); + let runtime = super::connect(Some(&path), &[], true, CredentialStorage::Memory) + .await + .unwrap(); + drop(runtime.operation_gate("removed").await); + assert!( + runtime + .inner + .operations + .lock() + .await + .contains_key("removed") + ); + + std::fs::write(&path, r#"{"mcpServers":{}}"#).unwrap(); + runtime.reload_config().await.unwrap(); + assert!( + !runtime + .inner + .operations + .lock() + .await + .contains_key("removed") + ); + } + #[tokio::test] async fn plugin_only_mcp_configuration_is_registered() { let directory = tempfile::tempdir().unwrap(); @@ -3957,6 +4326,147 @@ mod tests { assert!(runtime.inner.servers.read().await.contains_key("remote")); } + #[tokio::test] + async fn live_plugins_add_change_fail_closed_and_remove_through_search() { + let directory = tempfile::tempdir().unwrap(); + let config = directory.path().join("config.toml"); + let package = directory.path().join("plugin"); + std::fs::create_dir_all(&package).unwrap(); + std::fs::write( + package.join("plugin.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/plugin.schema.json","name":"live-plugin"}"#, + ) + .unwrap(); + std::fs::write(&config, "").unwrap(); + let plugins = PluginRuntime::new( + config.clone(), + directory.path().to_path_buf(), + directory.path().join("cache"), + directory.path().join("data"), + ResolvedPlugins::default(), + ); + let runtime = super::connect_dynamic( + None::<&Path>, + plugins.clone(), + true, + CredentialStorage::Memory, + ) + .await + .unwrap(); + assert!(runtime.inner.servers.read().await.is_empty()); + + let write_mcp = |name: &str| { + std::fs::write( + package.join("mcp.json"), + format!( + r#"{{"$schema":"https://agent-plugins.org/schemas/1.0.0/mcp.schema.json","mcpServers":{{"{name}":{{"type":"stdio","command":"kit-test-missing-plugin-command"}}}}}}"# + ), + ) + .unwrap(); + }; + write_mcp("first"); + std::fs::write( + &config, + format!( + "[plugins.live]\nsource = 'path'\npath = '{}'\n", + package.display() + ), + ) + .unwrap(); + runtime.search("mcp").await.unwrap(); + assert!(matches!( + runtime.inner.servers.read().await["first"].status, + ServerStatus::Error(_) + )); + + write_mcp("second"); + runtime.search("mcp").await.unwrap(); + let servers = runtime.inner.servers.read().await; + assert!(!servers.contains_key("first")); + assert!(matches!(servers["second"].status, ServerStatus::Error(_))); + drop(servers); + + std::fs::write(&config, "[plugins.live\n").unwrap(); + assert!(runtime.search("mcp").await.is_err()); + assert!(runtime.inner.servers.read().await.contains_key("second")); + assert_eq!(plugins.snapshot().mcp_plugins.len(), 1); + + std::fs::write(&config, "").unwrap(); + runtime.search("mcp").await.unwrap(); + assert!(runtime.inner.servers.read().await.is_empty()); + assert!(plugins.snapshot().mcp_plugins.is_empty()); + } + + #[tokio::test] + async fn live_plugin_baseline_respects_and_recovers_from_explicit_override() { + let directory = tempfile::tempdir().unwrap(); + let config = directory.path().join("config.toml"); + let explicit = directory.path().join("mcp.json"); + let package = directory.path().join("plugin"); + std::fs::create_dir_all(&package).unwrap(); + std::fs::write( + package.join("plugin.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/plugin.schema.json","name":"override-plugin"}"#, + ) + .unwrap(); + std::fs::write(&config, "").unwrap(); + std::fs::write( + &explicit, + r#"{"mcpServers":{"shared":{"command":"explicit","description":"Explicit"}}}"#, + ) + .unwrap(); + let plugins = PluginRuntime::new( + config.clone(), + directory.path().to_path_buf(), + directory.path().join("cache"), + directory.path().join("data"), + ResolvedPlugins::default(), + ); + let runtime = + super::connect_dynamic(Some(&explicit), plugins, true, CredentialStorage::Memory) + .await + .unwrap(); + + std::fs::write( + package.join("mcp.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/mcp.schema.json","mcpServers":{"shared":{"type":"streamable-http","url":"https://example.com/latest"}}}"#, + ) + .unwrap(); + std::fs::write( + &config, + format!( + "[plugins.override]\nsource = 'path'\npath = '{}'\n", + package.display() + ), + ) + .unwrap(); + runtime.reload_config().await.unwrap(); + assert_eq!( + runtime.inner.servers.read().await["shared"].description, + "Explicit" + ); + assert!(!runtime.inner.servers.read().await["shared"].plugin_owned); + + let invocation = runtime.acquire_invocation("mcp_shared_read").await.unwrap(); + std::fs::write( + package.join("mcp.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/mcp.schema.json","mcpServers":{"shared":{"type":"streamable-http","url":"https://example.com/newest"}}}"#, + ) + .unwrap(); + tokio::time::timeout(Duration::from_secs(1), runtime.reload_config()) + .await + .expect("explicit MCP invocation blocked plugin generation publication") + .unwrap(); + drop(invocation); + + std::fs::write(&explicit, r#"{"mcpServers":{}}"#).unwrap(); + runtime.reload_config().await.unwrap(); + let record = runtime.inner.servers.read().await["shared"].clone(); + assert_eq!(record.description, "override-plugin plugin MCP server"); + assert_eq!(record.url.as_deref(), Some("https://example.com/newest")); + assert!(record.plugin_owned); + } + #[tokio::test] async fn explicit_oauth_overrides_do_not_skip_the_initial_connection() { let directory = tempfile::tempdir().unwrap(); From c7b7f1270c86aa0e03cc0ae0ec31b22b4e763c21 Mon Sep 17 00:00:00 2001 From: daniel Date: Tue, 1 Sep 2026 16:29:43 +0100 Subject: [PATCH 2/2] fix(plugins): harden live generation refresh --- src/plugins.rs | 1564 ++++++++++++++++++++++++++++++++++++--- src/protocols/acp/v2.rs | 13 +- src/runtime.rs | 68 +- src/runtime/tests.rs | 73 +- src/tools/mcp.rs | 111 ++- 5 files changed, 1658 insertions(+), 171 deletions(-) diff --git a/src/plugins.rs b/src/plugins.rs index de284d5..824124a 100644 --- a/src/plugins.rs +++ b/src/plugins.rs @@ -113,13 +113,149 @@ struct PluginRuntimeInner { cache_root: PathBuf, skill_cache_root: PathBuf, data_root: PathBuf, - published: RwLock>, + git_mode: GitResolverMode, + published: RwLock, generation_barrier: Arc>, } +#[derive(Clone)] +enum GitResolverMode { + Https, + #[cfg(test)] + Local { + repository: PathBuf, + activity: Arc, + }, +} + +#[cfg(test)] +#[derive(Default)] +struct GitActivity { + probes: std::sync::atomic::AtomicUsize, + fetches: std::sync::atomic::AtomicUsize, + archives: std::sync::atomic::AtomicUsize, +} + +#[derive(Clone)] +struct PublishedPlugins { + resolved: Arc, + source_fingerprint: Option, +} + +#[derive(Clone, Debug, PartialEq, Eq)] +struct PlannedGitRevision { + fetch_revision: GitRevision, + raw_oid: String, + commit_oid: String, +} + +enum GitSourceRevision<'a> { + Unplanned(&'a GitRevision), + Planned(&'a PlannedGitRevision), +} + +#[derive(Clone, Copy, Debug)] +struct SourceFingerprint { + candidate: blake3::Hash, + resolved: blake3::Hash, +} + +#[derive(Clone)] +struct PlannedPathSource { + canonical_root: PathBuf, + tree_fingerprint: blake3::Hash, +} + +#[derive(Clone)] +struct SourcePlan { + candidate_fingerprint: blake3::Hash, + path_sources: BTreeMap, + git_revisions: BTreeMap, +} + +#[derive(Clone, Debug)] +struct ResolvedGitRevision { + raw_oid: String, + commit_oid: String, +} + +#[derive(Debug)] +struct ResolvedGitSource { + root: PathBuf, + revision: ResolvedGitRevision, +} + +struct BlockingResolution { + resolved: ResolvedPlugins, + git_revisions: BTreeMap, +} + #[derive(Debug)] pub(crate) struct StagedPlugins { - pub(crate) resolved: ResolvedPlugins, + pub(crate) resolved: Arc, + source_fingerprint: SourceFingerprint, +} + +impl SourcePlan { + fn verify_path_fingerprints( + &self, + configs: &BTreeMap, + runtime_root: &Path, + ) -> Result<(), String> { + let configured_paths = configs + .values() + .filter(|config| matches!(config, PluginConfig::Path { .. })) + .count(); + if configured_paths != self.path_sources.len() { + return Err("not all staged plugin paths have fingerprints".into()); + } + for (alias, config) in configs { + let PluginConfig::Path { path } = config else { + continue; + }; + let expected = self + .path_sources + .get(alias) + .ok_or_else(|| format!("path fingerprint for plugin {alias:?} was not retained"))?; + let verification_error = + || format!("plugin path for {alias:?} could not be verified after staging"); + let root = resolve_path(path, runtime_root).map_err(|_| verification_error())?; + let actual = local_plugin_tree_fingerprint(&root).map_err(|_| verification_error())?; + if root != expected.canonical_root || actual != expected.tree_fingerprint { + return Err(format!("plugin path for {alias:?} changed during staging")); + } + } + Ok(()) + } + + fn verified_fingerprint( + &self, + resolved: &BTreeMap, + ) -> Result { + if resolved.len() != self.git_revisions.len() { + return Err("not all staged Git revisions were verified".into()); + } + let mut fingerprint = blake3::Hasher::new(); + fingerprint.update(b"kit-plugin-resolved-sources-v1"); + fingerprint.update(self.candidate_fingerprint.as_bytes()); + for (alias, planned) in &self.git_revisions { + let verified = resolved + .get(alias) + .ok_or_else(|| format!("Git revision for plugin {alias:?} was not verified"))?; + if verified.raw_oid != planned.raw_oid || verified.commit_oid != planned.commit_oid { + return Err(format!( + "Git revision for plugin {alias:?} moved during staging" + )); + } + hash_fingerprint_field(&mut fingerprint, alias.as_bytes()); + hash_fingerprint_field(&mut fingerprint, verified.raw_oid.as_bytes()); + hash_fingerprint_field(&mut fingerprint, verified.commit_oid.as_bytes()); + } + Ok(SourceFingerprint { + candidate: self.candidate_fingerprint, + resolved: fingerprint.finalize(), + }) + } } impl PluginRuntime { @@ -137,7 +273,7 @@ impl PluginRuntime { ResolvedPlugins::default(), ); let staged = runtime.stage().await?; - runtime.publish(staged.resolved); + runtime.publish(staged); Ok(runtime) } @@ -174,18 +310,39 @@ impl PluginRuntime { cache_root, skill_cache_root, data_root, - published: RwLock::new(Arc::new(initial)), + git_mode: GitResolverMode::Https, + published: RwLock::new(PublishedPlugins { + resolved: Arc::new(initial), + source_fingerprint: None, + }), generation_barrier: Arc::new(tokio::sync::RwLock::new(())), }), } } + #[cfg(test)] + fn with_local_git_mode(mut self, repository: &Path, activity: Arc) -> Self { + if let Some(inner) = Arc::get_mut(&mut self.inner) { + inner.git_mode = GitResolverMode::Local { + repository: repository.to_path_buf(), + activity, + }; + } + self + } + pub fn snapshot(&self) -> Arc { - self.inner - .published - .read() - .expect("plugin generation lock poisoned") - .clone() + match self.inner.published.read() { + Ok(published) => published.resolved.clone(), + Err(poisoned) => poisoned.into_inner().resolved.clone(), + } + } + + fn published(&self) -> PublishedPlugins { + match self.inner.published.read() { + Ok(published) => published.clone(), + Err(poisoned) => poisoned.into_inner().clone(), + } } pub(crate) async fn stage(&self) -> Result { @@ -212,40 +369,41 @@ impl PluginRuntime { .plugins }; cleanup_runtime_skill_generations(&self.inner.skill_cache_root, &self.snapshot()); - let resolved = resolve_with_skill_cache( + stage_with_skill_cache( &configs, &self.inner.runtime_root, &self.inner.cache_root, &self.inner.skill_cache_root, &self.inner.data_root, + self.inner.git_mode.clone(), + self.published(), ) .await - .map_err(bounded_diagnostic)?; - Ok(StagedPlugins { resolved }) + .map_err(bounded_diagnostic) } pub(crate) async fn generation_lease(&self) -> tokio::sync::OwnedRwLockReadGuard<()> { self.inner.generation_barrier.clone().read_owned().await } - pub(crate) fn try_generation_lease(&self) -> Option> { - self.inner.generation_barrier.clone().try_read_owned().ok() - } - pub(crate) async fn generation_writer(&self) -> tokio::sync::OwnedRwLockWriteGuard<()> { self.inner.generation_barrier.clone().write_owned().await } - pub(crate) fn publish(&self, resolved: ResolvedPlugins) { - let mut published = self - .inner - .published - .write() - .expect("plugin generation lock poisoned"); - if resolved_generation_key(&published) != resolved_generation_key(&resolved) { - *published = Arc::new(resolved); + pub(crate) fn publish(&self, staged: StagedPlugins) { + let mut published = match self.inner.published.write() { + Ok(published) => published, + Err(poisoned) => poisoned.into_inner(), + }; + let unchanged = published + .source_fingerprint + .is_some_and(|current| current.resolved == staged.source_fingerprint.resolved) + && Arc::ptr_eq(&published.resolved, &staged.resolved); + if !unchanged { + published.resolved = staged.resolved; } - let current = published.clone(); + published.source_fingerprint = Some(staged.source_fingerprint); + let current = published.resolved.clone(); drop(published); cleanup_runtime_skill_generations(&self.inner.skill_cache_root, ¤t); } @@ -258,20 +416,6 @@ impl Drop for PluginRuntimeInner { } } -fn resolved_generation_key(resolved: &ResolvedPlugins) -> blake3::Hash { - let mut key = blake3::Hasher::new(); - for root in &resolved.package_roots { - key.update(root.as_os_str().as_encoded_bytes()); - key.update(&[0]); - } - for directory in &resolved.skill_directories { - key.update(directory.as_os_str().as_encoded_bytes()); - key.update(&[0]); - } - key.update(format!("{:?}", resolved.mcp_plugins).as_bytes()); - key.finalize() -} - pub(crate) fn bounded_diagnostic(mut message: String) -> String { const LIMIT: usize = 2_048; const ELLIPSIS: &str = "..."; @@ -307,17 +451,20 @@ async fn resolve_with_skill_cache( let cache_root = cache_root.to_path_buf(); let skill_cache_root = skill_cache_root.to_path_buf(); let data_root = data_root.to_path_buf(); - let mut resolved = tokio::task::spawn_blocking(move || { + let resolution = tokio::task::spawn_blocking(move || { resolve_blocking( &configs, &runtime_root, &cache_root, &skill_cache_root, &data_root, + &GitResolverMode::Https, + None, ) }) .await .map_err(|error| format!("plugin resolver task failed: {error}"))??; + let mut resolved = resolution.resolved; let registry = SkillRegistry::from_skill_dirs(resolved.skill_directories.clone()) .discover_skills() .await; @@ -325,14 +472,95 @@ async fn resolve_with_skill_cache( Ok(resolved) } +enum BlockingStage { + Reused { + resolved: Arc, + source_fingerprint: SourceFingerprint, + }, + Fresh { + resolved: ResolvedPlugins, + source_fingerprint: SourceFingerprint, + }, +} + +async fn stage_with_skill_cache( + configs: &BTreeMap, + runtime_root: &Path, + cache_root: &Path, + skill_cache_root: &Path, + data_root: &Path, + git_mode: GitResolverMode, + published: PublishedPlugins, +) -> Result { + let configs = configs.clone(); + let runtime_root = runtime_root.to_path_buf(); + let cache_root = cache_root.to_path_buf(); + let skill_cache_root = skill_cache_root.to_path_buf(); + let data_root = data_root.to_path_buf(); + let staged = tokio::task::spawn_blocking(move || -> Result { + let source_plan = source_plan(&configs, &runtime_root, &cache_root, &git_mode)?; + if let Some(source_fingerprint) = published + .source_fingerprint + .filter(|fingerprint| fingerprint.candidate == source_plan.candidate_fingerprint) + { + return Ok(BlockingStage::Reused { + resolved: published.resolved, + source_fingerprint, + }); + } + let resolution = resolve_blocking( + &configs, + &runtime_root, + &cache_root, + &skill_cache_root, + &data_root, + &git_mode, + Some(&source_plan), + )?; + source_plan.verify_path_fingerprints(&configs, &runtime_root)?; + let source_fingerprint = source_plan.verified_fingerprint(&resolution.git_revisions)?; + Ok(BlockingStage::Fresh { + resolved: resolution.resolved, + source_fingerprint, + }) + }) + .await + .map_err(|error| format!("plugin resolver task failed: {error}"))??; + match staged { + BlockingStage::Reused { + resolved, + source_fingerprint, + } => Ok(StagedPlugins { + resolved, + source_fingerprint, + }), + BlockingStage::Fresh { + mut resolved, + source_fingerprint, + } => { + let registry = SkillRegistry::from_skill_dirs(resolved.skill_directories.clone()) + .discover_skills() + .await; + resolved.skills = registry.skills().into_iter().cloned().collect(); + Ok(StagedPlugins { + resolved: Arc::new(resolved), + source_fingerprint, + }) + } + } +} + fn resolve_blocking( configs: &BTreeMap, runtime_root: &Path, cache_root: &Path, skill_cache_root: &Path, data_root: &Path, -) -> Result { + git_mode: &GitResolverMode, + source_plan: Option<&SourcePlan>, +) -> Result { let mut resolved = ResolvedPlugins::default(); + let mut verified_git_revisions = BTreeMap::new(); let mut manifest_names = BTreeSet::new(); let mut loaded_generations = Vec::new(); for (alias, config) in configs { @@ -344,9 +572,18 @@ fn resolve_blocking( sha256, subdir, } => resolve_archive(url, sha256, subdir.as_deref(), cache_root)?, - PluginConfig::Git { url, rev, subdir } => { - resolve_git(url, rev.as_deref(), subdir.as_deref(), cache_root)? - } + PluginConfig::Git { url, rev, subdir } => match source_plan { + Some(source_plan) => { + let planned = source_plan.git_revisions.get(alias).ok_or_else(|| { + format!("missing staged Git revision for plugin {alias:?}") + })?; + let verified = + resolve_git_planned(url, subdir.as_deref(), cache_root, git_mode, planned)?; + verified_git_revisions.insert(alias.clone(), verified.revision); + verified.root + } + None => resolve_git(url, rev.as_deref(), subdir.as_deref(), cache_root)?, + }, }; let plugin = AgentPlugin::load(&root).map_err(|error| { format!( @@ -417,7 +654,10 @@ fn resolve_blocking( } resolved.package_roots = package_roots; resolved.skill_directories = skill_directories; - Ok(resolved) + Ok(BlockingResolution { + resolved, + git_revisions: verified_git_revisions, + }) } fn plugin_semantic_key(plugin: &AgentPlugin) -> blake3::Hash { @@ -467,9 +707,13 @@ fn snapshot_skills( directory.display() ) })?; - let relative = directory - .strip_prefix(package) - .expect("owning package is a prefix"); + let relative = directory.strip_prefix(package).map_err(|error| { + format!( + "could not resolve plugin skill {} relative to package {}: {error}", + directory.display(), + package.display() + ) + })?; let target = PathBuf::from(index.to_string()).join(relative); collect_skill_inventory( package, @@ -716,9 +960,13 @@ fn collect_skill_inventory( directory.display() )); } - let relative = directory - .strip_prefix(package_root) - .expect("validated skill directory is in its package"); + let relative = directory.strip_prefix(package_root).map_err(|error| { + format!( + "could not resolve plugin skill {} relative to package {}: {error}", + directory.display(), + package_root.display() + ) + })?; inventory.insert(target_root.join(relative), CachedEntry::Directory); let entries = fs::read_dir(directory).map_err(|error| { format!( @@ -741,9 +989,13 @@ fn collect_skill_inventory( if metadata.is_dir() { collect_skill_inventory(package_root, &path, target_root, inventory, captured_bytes)?; } else if metadata.is_file() { - let relative = path - .strip_prefix(package_root) - .expect("skill inventory remains under its package"); + let relative = path.strip_prefix(package_root).map_err(|error| { + format!( + "could not resolve plugin skill entry {} relative to package {}: {error}", + path.display(), + package_root.display() + ) + })?; capture_snapshot_file( package_root, &path, @@ -1181,16 +1433,10 @@ fn publish_cached_directory( parent.display() ) })?; - let mut random = [0u8; 8]; - getrandom::fill(&mut random).map_err(|error| error.to_string())?; - let staging = parent.join(format!( - ".{}-{:x}.tmp", - std::process::id(), - u64::from_le_bytes(random) - )); - fs::create_dir(&staging) - .map_err(|error| format!("could not create {context} staging directory: {error}"))?; - let result = build(&staging).and_then(|()| match fs::rename(&staging, destination) { + let staging_guard = StagingDirectory::create(parent, ".cache-", context)?; + let staging = staging_guard.path(); + build(staging)?; + match fs::rename(staging, destination) { Ok(()) => Ok(()), Err(_) if fs::symlink_metadata(destination) @@ -1199,11 +1445,7 @@ fn publish_cached_directory( Ok(()) } Err(error) => Err(format!("could not publish {context}: {error}")), - }); - if staging.exists() { - let _ = fs::remove_dir_all(&staging); } - result } fn validate_plugin_diagnostics(alias: &str, plugin: &AgentPlugin) -> Result<(), String> { @@ -1252,6 +1494,265 @@ fn resolve_path(path: &Path, runtime_root: &Path) -> Result { .map_err(|error| format!("could not resolve plugin path {}: {error}", path.display())) } +fn hash_fingerprint_field(hasher: &mut blake3::Hasher, value: &[u8]) { + hasher.update(&(value.len() as u64).to_le_bytes()); + hasher.update(value); +} + +fn source_plan( + configs: &BTreeMap, + runtime_root: &Path, + cache_root: &Path, + git_mode: &GitResolverMode, +) -> Result { + let mut fingerprint = blake3::Hasher::new(); + let mut path_sources = BTreeMap::new(); + let mut git_revisions = BTreeMap::new(); + fingerprint.update(b"kit-plugin-sources-v2"); + for (alias, config) in configs { + validate_alias(alias)?; + hash_fingerprint_field(&mut fingerprint, alias.as_bytes()); + match config { + PluginConfig::Path { path } => { + fingerprint.update(&[0]); + let root = resolve_path(path, runtime_root)?; + hash_fingerprint_field(&mut fingerprint, root.as_os_str().as_encoded_bytes()); + let tree_fingerprint = local_plugin_tree_fingerprint(&root)?; + hash_fingerprint_field(&mut fingerprint, tree_fingerprint.as_bytes()); + path_sources.insert( + alias.clone(), + PlannedPathSource { + canonical_root: root, + tree_fingerprint, + }, + ); + } + PluginConfig::Archive { + url, + sha256, + subdir, + } => { + fingerprint.update(&[1]); + let url = Url::parse(url) + .map_err(|error| format!("invalid plugin archive URL: {error}"))?; + validate_download_url(&url)?; + let digest = parse_sha256(sha256)?; + let subdir = subdir.as_deref().map(validate_relative_path).transpose()?; + hash_fingerprint_field(&mut fingerprint, url.as_str().as_bytes()); + hash_fingerprint_field(&mut fingerprint, &digest); + if let Some(subdir) = subdir { + hash_fingerprint_field(&mut fingerprint, subdir.as_os_str().as_encoded_bytes()); + } else { + hash_fingerprint_field(&mut fingerprint, &[]); + } + } + PluginConfig::Git { url, rev, subdir } => { + fingerprint.update(&[2]); + let url = validate_git_url(url)?; + let revision = rev + .as_deref() + .map(validate_git_revision) + .transpose()? + .unwrap_or(GitRevision::DefaultBranch); + let subdir = subdir.as_deref().map(validate_git_subdir).transpose()?; + hash_fingerprint_field(&mut fingerprint, url.as_str().as_bytes()); + if let Some(subdir) = subdir { + hash_fingerprint_field(&mut fingerprint, subdir.as_os_str().as_encoded_bytes()); + } else { + hash_fingerprint_field(&mut fingerprint, &[]); + } + let planned = match &revision { + GitRevision::Commit(oid) => PlannedGitRevision { + fetch_revision: revision.clone(), + raw_oid: oid.clone(), + commit_oid: oid.clone(), + }, + GitRevision::Ref(_) | GitRevision::DefaultBranch => match git_mode { + GitResolverMode::Https => probe_git_revision( + OsStr::new(url.as_str()), + &sha256_text(url.as_str()), + &revision, + cache_root, + GitProtocol::Https, + &SystemGitRunner::default(), + )?, + #[cfg(test)] + GitResolverMode::Local { + repository, + activity, + } => probe_git_revision( + repository.as_os_str(), + &sha256_text(&repository.to_string_lossy()), + &revision, + cache_root, + GitProtocol::Local, + &TrackingGitRunner { + inner: SystemGitRunner::default(), + activity: activity.clone(), + }, + )?, + }, + }; + fingerprint.update(&[1]); + hash_fingerprint_field(&mut fingerprint, planned.raw_oid.as_bytes()); + hash_fingerprint_field(&mut fingerprint, planned.commit_oid.as_bytes()); + git_revisions.insert(alias.clone(), planned); + } + } + } + Ok(SourcePlan { + candidate_fingerprint: fingerprint.finalize(), + path_sources, + git_revisions, + }) +} + +fn local_plugin_tree_fingerprint(root: &Path) -> Result { + let mut fingerprint = blake3::Hasher::new(); + fingerprint.update(b"kit-plugin-path-tree-v1"); + hash_local_plugin_tree(root, &mut fingerprint)?; + Ok(fingerprint.finalize()) +} + +fn hash_local_plugin_tree(root: &Path, fingerprint: &mut blake3::Hasher) -> Result<(), String> { + let metadata = fs::symlink_metadata(root) + .map_err(|error| format!("could not inspect plugin path {}: {error}", root.display()))?; + if !metadata.file_type().is_dir() { + return Err(format!( + "plugin path is not a real directory: {}", + root.display() + )); + } + let mut pending = vec![PathBuf::new()]; + let mut entries_seen = 0usize; + let mut bytes_seen = 0u64; + while let Some(relative_directory) = pending.pop() { + let directory = root.join(&relative_directory); + let mut entries = fs::read_dir(&directory) + .map_err(|error| { + format!( + "could not read plugin path {}: {error}", + directory.display() + ) + })? + .collect::, _>>() + .map_err(|error| format!("could not read plugin path entry: {error}"))?; + entries.sort_by_key(|entry| entry.file_name()); + let mut child_directories = Vec::new(); + for entry in entries { + entries_seen = entries_seen + .checked_add(1) + .filter(|count| *count <= MAX_ARCHIVE_ENTRIES) + .ok_or_else(|| "plugin path exceeds entry limit".to_string())?; + let relative = relative_directory.join(entry.file_name()); + hash_fingerprint_field(fingerprint, relative.as_os_str().as_encoded_bytes()); + let path = entry.path(); + let before = fs::symlink_metadata(&path).map_err(|error| { + format!("could not inspect plugin path {}: {error}", path.display()) + })?; + if before.file_type().is_dir() { + fingerprint.update(&[0]); + child_directories.push(relative); + } else if before.file_type().is_file() { + fingerprint.update(&[1]); + if before.len() > MAX_FILE_BYTES { + return Err(format!( + "plugin path file exceeds size limit: {}", + path.display() + )); + } + bytes_seen = bytes_seen + .checked_add(before.len()) + .filter(|total| *total <= MAX_EXPANDED_BYTES) + .ok_or_else(|| "plugin path exceeds expanded size limit".to_string())?; + fingerprint.update(&before.len().to_le_bytes()); + let mut file = File::open(&path).map_err(|error| { + format!( + "could not open plugin path file {}: {error}", + path.display() + ) + })?; + let opened = file.metadata().map_err(|error| { + format!( + "could not inspect plugin path file {}: {error}", + path.display() + ) + })?; + if !opened.is_file() + || !same_file(&before, &opened) + || !same_file_state(&before, &opened) + { + return Err(format!( + "plugin path changed while being fingerprinted: {}", + path.display() + )); + } + let mut remaining = before.len(); + let mut buffer = [0u8; 32 * 1024]; + while remaining != 0 { + let limit = usize::try_from(remaining.min(buffer.len() as u64)) + .map_err(|_| "plugin path read limit is too large".to_string())?; + let read = file.read(&mut buffer[..limit]).map_err(|error| { + format!( + "could not read plugin path file {}: {error}", + path.display() + ) + })?; + if read == 0 { + return Err(format!( + "plugin path file changed while being fingerprinted: {}", + path.display() + )); + } + fingerprint.update(&buffer[..read]); + remaining -= read as u64; + } + let mut extra = [0u8; 1]; + if file.read(&mut extra).map_err(|error| { + format!( + "could not recheck plugin path file {}: {error}", + path.display() + ) + })? != 0 + { + return Err(format!( + "plugin path file changed while being fingerprinted: {}", + path.display() + )); + } + let after = fs::symlink_metadata(&path).map_err(|error| { + format!( + "could not recheck plugin path file {}: {error}", + path.display() + ) + })?; + if !after.is_file() + || !same_file(&before, &after) + || !same_file_state(&before, &after) + { + return Err(format!( + "plugin path changed while being fingerprinted: {}", + path.display() + )); + } + } else if before.file_type().is_symlink() { + return Err(format!( + "plugin path fingerprinting does not allow symlinks: {}", + path.display() + )); + } else { + return Err(format!( + "plugin path contains an unsupported entry: {}", + path.display() + )); + } + } + child_directories.reverse(); + pending.extend(child_directories); + } + Ok(()) +} + #[derive(Clone, Debug, PartialEq, Eq)] enum GitRevision { Commit(String), @@ -1537,6 +2038,44 @@ impl GitRunner for SystemGitRunner { } } +#[cfg(test)] +struct TrackingGitRunner { + inner: SystemGitRunner, + activity: Arc, +} + +#[cfg(test)] +impl GitRunner for TrackingGitRunner { + fn run(&self, request: GitRunRequest<'_>) -> Result, GitFailure> { + if request + .args + .iter() + .any(|argument| argument == OsStr::new("--exit-code")) + { + self.activity + .probes + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); + } + if request + .args + .iter() + .any(|argument| argument == OsStr::new("fetch")) + { + self.activity + .fetches + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); + } + self.inner.run(request) + } + + fn archive(&self, request: GitRunRequest<'_>, destination: &Path) -> Result<(), GitFailure> { + self.activity + .archives + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); + self.inner.archive(request, destination) + } +} + fn read_bounded(mut reader: impl Read, limit: u64) -> Result, GitFailure> { let mut bytes = Vec::new(); reader @@ -1654,12 +2193,21 @@ struct StagingDirectory { } impl StagingDirectory { - fn create(parent: &Path, prefix: &str) -> Result { + fn create(parent: &Path, prefix: &str, context: &str) -> Result { cleanup_stale_staging(parent, prefix, STALE_STAGING_AGE)?; - let path = parent.join(format!("{prefix}{}", random_suffix()?)); - create_private_directory(&path) - .map_err(|error| format!("could not create plugin staging directory: {error}"))?; - Ok(Self { path }) + for _ in 0..4 { + let path = parent.join(format!("{prefix}{}", random_suffix()?)); + match create_private_directory(&path) { + Ok(()) => return Ok(Self { path }), + Err(error) if error.kind() == io::ErrorKind::AlreadyExists => continue, + Err(error) => { + return Err(format!( + "could not create {context} staging directory: {error}" + )); + } + } + } + Err(format!("could not allocate {context} staging directory")) } fn path(&self) -> &Path { @@ -1735,19 +2283,58 @@ fn resolve_git( resolve_git_source( OsStr::new(source), &source_key, - &revision, + GitSourceRevision::Unplanned(&revision), subdir.as_deref(), cache_root, GitProtocol::Https, &SystemGitRunner::default(), ) + .map(|resolved| resolved.root) } -#[cfg(test)] -fn resolve_git_local( - repository: &Path, - rev: &str, - subdir: Option<&Path>, +fn resolve_git_planned( + value: &str, + subdir: Option<&str>, + cache_root: &Path, + mode: &GitResolverMode, + planned: &PlannedGitRevision, +) -> Result { + let url = validate_git_url(value)?; + let subdir = subdir.map(validate_git_subdir).transpose()?; + match mode { + GitResolverMode::Https => resolve_git_source( + OsStr::new(url.as_str()), + &sha256_text(url.as_str()), + GitSourceRevision::Planned(planned), + subdir.as_deref(), + cache_root, + GitProtocol::Https, + &SystemGitRunner::default(), + ), + #[cfg(test)] + GitResolverMode::Local { + repository, + activity, + } => resolve_git_source( + repository.as_os_str(), + &sha256_text(&repository.to_string_lossy()), + GitSourceRevision::Planned(planned), + subdir.as_deref(), + cache_root, + GitProtocol::Local, + &TrackingGitRunner { + inner: SystemGitRunner::default(), + activity: activity.clone(), + }, + ), + } +} + +#[cfg(test)] +fn resolve_git_local( + repository: &Path, + rev: &str, + subdir: Option<&Path>, cache_root: &Path, runner: &dyn GitRunner, ) -> Result { @@ -1782,12 +2369,13 @@ fn resolve_git_local_revision( resolve_git_source( repository.as_os_str(), &source_key, - &revision, + GitSourceRevision::Unplanned(&revision), subdir.as_deref(), cache_root, GitProtocol::Local, runner, ) + .map(|resolved| resolved.root) } fn validate_git_url(value: &str) -> Result { @@ -2047,15 +2635,177 @@ fn describe_git_failure(operation: &str, failure: GitFailure) -> String { } } -fn resolve_git_source( +fn probe_git_revision( remote: &OsStr, source_key: &str, revision: &GitRevision, + cache_root: &Path, + protocol: GitProtocol, + runner: &dyn GitRunner, +) -> Result { + let source_root = cache_root.join(GIT_CACHE_VERSION).join(source_key); + fs::create_dir_all(&source_root) + .map_err(|error| format!("could not create Git plugin cache: {error}"))?; + let staging_guard = + StagingDirectory::create(&source_root, ".probe-", "Git plugin revision probe")?; + let staging = staging_guard.path(); + let hooks = staging.join("hooks"); + let attributes = staging.join("attributes"); + fs::create_dir(&hooks) + .map_err(|error| format!("could not create Git plugin revision probe hooks: {error}"))?; + OpenOptions::new() + .write(true) + .create_new(true) + .open(&attributes) + .map_err(|error| { + format!("could not create Git plugin revision probe attributes: {error}") + })?; + let remote_config = match protocol { + GitProtocol::Https => Some( + remote + .to_str() + .ok_or("plugin Git URL must be valid Unicode")?, + ), + #[cfg(test)] + GitProtocol::Local => None, + }; + let git = GitCommandContext { + runner, + protocol, + hooks: &hooks, + attributes: &attributes, + remote: remote_config, + }; + verify_git_effective_url(&git, staging, remote)?; + let references = match revision { + GitRevision::Ref(reference) if reference.starts_with("refs/") => { + vec![reference.clone()] + } + GitRevision::Ref(reference) => vec![ + format!("refs/heads/{reference}"), + format!("refs/tags/{reference}"), + ], + GitRevision::DefaultBranch => vec!["HEAD".to_string()], + GitRevision::Commit(_) => { + return Err("immutable Git commits do not require a revision probe".into()); + } + }; + let queries = references + .iter() + .flat_map(|reference| [reference.clone(), format!("{reference}^{{}}")]) + .collect::>(); + let mut arguments = vec![ + OsString::from("ls-remote"), + OsString::from("--exit-code"), + OsString::from("--"), + remote.to_os_string(), + ]; + arguments.extend(queries.iter().map(OsString::from)); + let argument_refs = arguments + .iter() + .map(OsString::as_os_str) + .collect::>(); + let output = git.run( + "revision probe", + staging, + &argument_refs, + MAX_GIT_DIAGNOSTIC_BYTES, + None, + )?; + parse_git_probe(&output, revision, &references) +} + +fn parse_git_probe( + output: &[u8], + revision: &GitRevision, + references: &[String], +) -> Result { + let mut records = BTreeMap::new(); + for raw in output.split(|byte| *byte == b'\n') { + let record = match raw.strip_suffix(b"\r") { + Some(record) => record, + None => raw, + }; + if record.is_empty() { + continue; + } + let Some(separator) = record.iter().position(|byte| *byte == b'\t') else { + return Err("Git plugin revision probe returned malformed output".into()); + }; + let (oid, reference_with_separator) = record.split_at(separator); + let reference = std::str::from_utf8(&reference_with_separator[1..]) + .map_err(|_| "Git plugin revision probe returned malformed output")?; + let advertised_reference = reference.strip_suffix("^{}").unwrap_or(reference); + if oid.len() != 40 + || !oid.iter().all(u8::is_ascii_hexdigit) + || !references + .iter() + .any(|expected| expected == advertised_reference) + { + return Err("Git plugin revision probe returned an unexpected reference".into()); + } + let oid = std::str::from_utf8(oid) + .map_err(|_| "Git plugin revision probe returned malformed output")? + .to_ascii_lowercase(); + if records.insert(reference.to_string(), oid).is_some() { + return Err("Git plugin revision probe returned a duplicate reference".into()); + } + } + let selected = references + .iter() + .filter(|reference| records.contains_key(*reference)) + .collect::>(); + if selected.len() != 1 { + return Err(match revision { + GitRevision::Ref(reference) if !reference.starts_with("refs/") => { + "Git plugin short revision is missing or ambiguous".into() + } + _ => "Git plugin revision probe did not return exactly one reference".into(), + }); + } + let reference = selected[0]; + let raw_oid = records + .remove(reference) + .ok_or("Git plugin revision probe returned no object ID")?; + let peeled_reference = format!("{reference}^{{}}"); + let commit_oid = records + .remove(&peeled_reference) + .unwrap_or_else(|| raw_oid.clone()); + if !records.is_empty() { + return Err("Git plugin revision probe returned an unexpected reference".into()); + } + let fetch_revision = match revision { + GitRevision::DefaultBranch => { + if reference != "HEAD" { + return Err("Git plugin default revision probe did not return HEAD".into()); + } + GitRevision::DefaultBranch + } + GitRevision::Ref(_) => GitRevision::Ref(reference.clone()), + GitRevision::Commit(_) => { + return Err("immutable Git commits do not require a revision probe".into()); + } + }; + Ok(PlannedGitRevision { + fetch_revision, + raw_oid, + commit_oid, + }) +} + +fn resolve_git_source( + remote: &OsStr, + source_key: &str, + source_revision: GitSourceRevision<'_>, subdir: Option<&Path>, cache_root: &Path, protocol: GitProtocol, runner: &dyn GitRunner, -) -> Result { +) -> Result { + let (revision, expected_revision) = match source_revision { + GitSourceRevision::Unplanned(revision) => (revision, None), + GitSourceRevision::Planned(planned) => (&planned.fetch_revision, Some(planned)), + }; let subdir_text = subdir.map(path_to_git_string).transpose()?; let subdir_key = sha256_text(subdir_text.as_deref().unwrap_or(".")); let source_root = cache_root.join(GIT_CACHE_VERSION).join(source_key); @@ -2067,7 +2817,34 @@ fn resolve_git_source( if let Ok(root) = validate_git_cache_entry(&destination, source_key, oid, &subdir_key, subdir) { - return Ok(root); + return Ok(ResolvedGitSource { + root, + revision: ResolvedGitRevision { + raw_oid: oid.clone(), + commit_oid: oid.clone(), + }, + }); + } + } + + if let Some(planned) = expected_revision + && matches!( + &planned.fetch_revision, + GitRevision::Ref(_) | GitRevision::DefaultBranch + ) + { + let oid = &planned.commit_oid; + let destination = git_cache_destination(&source_root, oid, &subdir_key); + if let Ok(root) = + validate_git_cache_entry(&destination, source_key, oid, &subdir_key, subdir) + { + return Ok(ResolvedGitSource { + root, + revision: ResolvedGitRevision { + raw_oid: planned.raw_oid.clone(), + commit_oid: oid.clone(), + }, + }); } } @@ -2080,7 +2857,7 @@ fn resolve_git_source( #[cfg(test)] GitProtocol::Local => None, }; - let staging_guard = StagingDirectory::create(&source_root, ".staging-")?; + let staging_guard = StagingDirectory::create(&source_root, ".staging-", "Git plugin")?; let staging = staging_guard.path(); let hooks = staging.join("hooks"); let attributes = staging.join("attributes"); @@ -2147,6 +2924,23 @@ fn resolve_git_source( Some(&git_dir.join("objects")), )?; enforce_git_staging_metadata(&git_dir, remote)?; + let fetched_raw = git.run( + "raw object verification", + &git_dir, + &[ + OsStr::new("rev-parse"), + OsStr::new("--verify"), + OsStr::new("--end-of-options"), + OsStr::new(GIT_PRIVATE_FETCH_REF), + ], + 128, + None, + )?; + let fetched_raw = + parse_git_oid(&fetched_raw).ok_or("Git plugin revision is not an object ID")?; + if expected_revision.is_some_and(|expected| fetched_raw != expected.raw_oid) { + return Err("Git plugin revision moved while its generation was being staged".into()); + } let private_commit = format!("{GIT_PRIVATE_FETCH_REF}^{{commit}}"); let fetched = git.run( "commit verification", @@ -2161,6 +2955,9 @@ fn resolve_git_source( None, )?; let fetched = parse_git_oid(&fetched).ok_or("Git plugin revision is not a commit")?; + if expected_revision.is_some_and(|expected| fetched != expected.commit_oid) { + return Err("Git plugin revision moved while its generation was being staged".into()); + } if let GitRevision::Commit(expected) = revision && fetched != *expected { @@ -2172,7 +2969,13 @@ fn resolve_git_source( if let Ok(root) = validate_git_cache_entry(&destination, source_key, &fetched, &subdir_key, subdir) { - return Ok(root); + return Ok(ResolvedGitSource { + root, + revision: ResolvedGitRevision { + raw_oid: fetched_raw, + commit_oid: fetched, + }, + }); } let oid = OsStr::new(&fetched); @@ -2227,7 +3030,14 @@ fn resolve_git_source( .is_ok() => {} Err(error) => return Err(format!("could not publish Git plugin cache: {error}")), } - validate_git_cache_entry(&destination, source_key, &fetched, &subdir_key, subdir) + let root = validate_git_cache_entry(&destination, source_key, &fetched, &subdir_key, subdir)?; + Ok(ResolvedGitSource { + root, + revision: ResolvedGitRevision { + raw_oid: fetched_raw, + commit_oid: fetched, + }, + }) } fn verify_git_effective_url( @@ -2486,8 +3296,20 @@ fn resolve_archive( &cache_root.join("archive-blobs").join(&digest), )?; let destination = cache_root.join(&digest); - if destination.exists() { - verify_cached_archive(&destination, &bytes)?; + match fs::symlink_metadata(&destination) { + Ok(metadata) if metadata.file_type().is_dir() => { + verify_cached_archive(&destination, &bytes)?; + } + Ok(_) => { + return Err(format!( + "plugin archive cache entry is not a real directory: {}", + destination.display() + )); + } + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => { + return Err(format!("could not inspect plugin archive cache: {error}")); + } } publish_cached_directory(&destination, "plugin archive", |staging| { extract_archive(&bytes, staging)?; @@ -2500,6 +3322,7 @@ fn resolve_archive( })?; Ok(()) })?; + verify_cached_archive(&destination, &bytes)?; select_package_root(&destination, subdir.as_deref()) } @@ -2576,18 +3399,11 @@ fn verify_cached_archive(destination: &Path, bytes: &[u8]) -> Result<(), String> let parent = destination .parent() .ok_or_else(|| "plugin archive cache path has no parent".to_string())?; - let mut random = [0u8; 8]; - getrandom::fill(&mut random).map_err(|error| error.to_string())?; - let staging = parent.join(format!( - ".verify-{}-{:x}.tmp", - std::process::id(), - u64::from_le_bytes(random) - )); - fs::create_dir(&staging).map_err(|error| { - format!("could not create plugin archive verification directory: {error}") - })?; - let result = extract_archive(bytes, &staging).and_then(|()| { - let expected = read_directory_inventory(&staging)?; + let staging_guard = + StagingDirectory::create(parent, ".verify-", "plugin archive verification")?; + let staging = staging_guard.path(); + extract_archive(bytes, staging).and_then(|()| { + let expected = read_directory_inventory(staging)?; let actual = read_directory_inventory(destination)?; if actual == expected { Ok(()) @@ -2597,9 +3413,7 @@ fn verify_cached_archive(destination: &Path, bytes: &[u8]) -> Result<(), String> destination.display() )) } - }); - let _ = fs::remove_dir_all(staging); - result + }) } fn read_directory_inventory(root: &Path) -> Result, String> { @@ -3141,6 +3955,87 @@ mod tests { assert!(error.contains("without following links")); } + #[cfg(unix)] + #[test] + fn local_tree_fingerprint_rejects_symlinks() { + use std::os::unix::fs::symlink; + + let directory = tempfile::tempdir().unwrap(); + fs::write(directory.path().join("plugin.json"), MANIFEST).unwrap(); + fs::write(directory.path().join("target"), "body").unwrap(); + symlink("target", directory.path().join("link")).unwrap(); + let mut fingerprint = blake3::Hasher::new(); + let error = hash_local_plugin_tree(directory.path(), &mut fingerprint).unwrap_err(); + assert!(error.contains("does not allow symlinks")); + } + + #[test] + fn path_source_plan_verification_rejects_tree_changes() { + let directory = tempfile::tempdir().unwrap(); + let package = directory.path().join("plugin"); + fs::create_dir(&package).unwrap(); + fs::write(package.join("plugin.json"), MANIFEST).unwrap(); + let mut configs = BTreeMap::new(); + configs.insert( + "local".to_string(), + PluginConfig::Path { + path: package.clone(), + }, + ); + let plan = source_plan( + &configs, + directory.path(), + directory.path(), + &GitResolverMode::Https, + ) + .unwrap(); + + fs::write(package.join("plugin.json"), "changed").unwrap(); + + let error = plan + .verify_path_fingerprints(&configs, directory.path()) + .unwrap_err(); + assert_eq!(error, "plugin path for \"local\" changed during staging"); + } + + #[cfg(unix)] + #[test] + fn path_source_plan_verification_rejects_root_symlink_retargeting() { + use std::os::unix::fs::symlink; + + let directory = tempfile::tempdir().unwrap(); + let first = directory.path().join("first"); + let second = directory.path().join("second"); + fs::create_dir(&first).unwrap(); + fs::create_dir(&second).unwrap(); + fs::write(first.join("plugin.json"), MANIFEST).unwrap(); + fs::write(second.join("plugin.json"), MANIFEST).unwrap(); + let configured = directory.path().join("current"); + symlink(&first, &configured).unwrap(); + let mut configs = BTreeMap::new(); + configs.insert( + "local".to_string(), + PluginConfig::Path { + path: configured.clone(), + }, + ); + let plan = source_plan( + &configs, + directory.path(), + directory.path(), + &GitResolverMode::Https, + ) + .unwrap(); + + fs::remove_file(&configured).unwrap(); + symlink(&second, &configured).unwrap(); + + let error = plan + .verify_path_fingerprints(&configs, directory.path()) + .unwrap_err(); + assert_eq!(error, "plugin path for \"local\" changed during staging"); + } + #[tokio::test] async fn poisoned_unpublished_skill_generation_is_rebuilt() { let directory = tempfile::tempdir().unwrap(); @@ -3274,6 +4169,457 @@ mod tests { assert!(runtime.snapshot().skill_directories.is_empty()); } + #[tokio::test] + async fn unchanged_path_reuses_published_generation_and_edits_refresh_it() { + let directory = tempfile::tempdir().unwrap(); + let config = directory.path().join("config.toml"); + let package = directory.path().join("plugin"); + let skill = package.join("skills/live-skill"); + fs::create_dir_all(&skill).unwrap(); + fs::write(package.join("plugin.json"), MANIFEST).unwrap(); + fs::write( + skill.join("SKILL.md"), + "---\nname: live-skill\ndescription: Live skill.\n---\nfirst\n", + ) + .unwrap(); + fs::write( + &config, + format!( + "[plugins.live]\nsource = 'path'\npath = '{}'\n", + package.display() + ), + ) + .unwrap(); + let runtime = PluginRuntime::load( + config, + directory.path().to_path_buf(), + directory.path().join("cache"), + directory.path().join("data"), + ) + .await + .unwrap(); + let published = runtime.snapshot(); + let unchanged = runtime.stage().await.unwrap(); + assert!(Arc::ptr_eq(&published, &unchanged.resolved)); + + fs::write( + skill.join("SKILL.md"), + "---\nname: live-skill\ndescription: Live skill.\n---\nsecond\n", + ) + .unwrap(); + let changed = runtime.stage().await.unwrap(); + assert!(!Arc::ptr_eq(&published, &changed.resolved)); + assert_eq!(changed.resolved.skills[0].body, "second"); + runtime.publish(changed); + let republished = runtime.snapshot(); + let unchanged_again = runtime.stage().await.unwrap(); + assert!(Arc::ptr_eq(&republished, &unchanged_again.resolved)); + } + + #[tokio::test] + async fn unchanged_archive_reuses_published_generation() { + let bytes = tar_with_file("plugin/plugin.json", MANIFEST.as_bytes()); + let digest = sha256_hex(&bytes); + let (url, server) = serve_once(bytes); + let directory = tempfile::tempdir().unwrap(); + let config = directory.path().join("config.toml"); + fs::write( + &config, + format!("[plugins.archive]\nsource = 'archive'\nurl = '{url}'\nsha256 = '{digest}'\n"), + ) + .unwrap(); + let runtime = PluginRuntime::load( + config, + directory.path().to_path_buf(), + directory.path().join("cache"), + directory.path().join("data"), + ) + .await + .unwrap(); + server.join().unwrap(); + let published = runtime.snapshot(); + let unchanged = runtime.stage().await.unwrap(); + assert!(Arc::ptr_eq(&published, &unchanged.resolved)); + } + + async fn stage_local_git_runtime( + config: PathBuf, + root: &Path, + repository: &Path, + ) -> (PluginRuntime, Arc) { + let activity = Arc::new(GitActivity::default()); + let runtime = PluginRuntime::new( + config, + root.to_path_buf(), + root.join("cache"), + root.join("data"), + ResolvedPlugins::default(), + ) + .with_local_git_mode(repository, activity.clone()); + let staged = runtime.stage().await.unwrap(); + runtime.publish(staged); + (runtime, activity) + } + + #[tokio::test] + async fn full_commit_stage_reuses_published_arc_before_git_resolution() { + let repository = TestRepository::new(); + repository.commit_file("plugin.json", MANIFEST.as_bytes(), "manifest"); + let commit = repository.commit_file( + "skills/live/SKILL.md", + b"---\nname: live\ndescription: Live.\n---\nfirst\n", + "skill", + ); + let directory = tempfile::tempdir().unwrap(); + let config = directory.path().join("config.toml"); + fs::write( + &config, + format!( + "[plugins.commit]\nsource = 'git'\nurl = 'https://plugins.example/repository.git'\nrev = '{commit}'\n" + ), + ) + .unwrap(); + let (runtime, activity) = + stage_local_git_runtime(config, directory.path(), repository.path()).await; + assert_eq!( + activity.fetches.load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + assert_eq!( + activity.archives.load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + let published = runtime.snapshot(); + let unchanged = runtime.stage().await.unwrap(); + assert!(Arc::ptr_eq(&published, &unchanged.resolved)); + assert_eq!( + activity.fetches.load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + assert_eq!( + activity.archives.load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + } + + #[tokio::test] + async fn mutable_default_head_stage_reuses_then_refreshes_on_movement() { + let repository = TestRepository::new(); + repository.commit_file("plugin.json", MANIFEST.as_bytes(), "manifest"); + repository.commit_file( + "skills/live/SKILL.md", + b"---\nname: live\ndescription: Live.\n---\nfirst\n", + "skill", + ); + repository.git(&["branch", "-M", "main"]); + let directory = tempfile::tempdir().unwrap(); + let config = directory.path().join("config.toml"); + fs::write( + &config, + "[plugins.head]\nsource = 'git'\nurl = 'https://plugins.example/repository.git'\n", + ) + .unwrap(); + let (runtime, activity) = + stage_local_git_runtime(config, directory.path(), repository.path()).await; + assert_eq!( + activity.fetches.load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + assert_eq!( + activity.archives.load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + let published = runtime.snapshot(); + let unchanged = runtime.stage().await.unwrap(); + assert!(Arc::ptr_eq(&published, &unchanged.resolved)); + assert_eq!( + activity.fetches.load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + assert_eq!( + activity.archives.load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + + repository.commit_file( + "skills/live/SKILL.md", + b"---\nname: live\ndescription: Live.\n---\nsecond\n", + "move head", + ); + let changed = runtime.stage().await.unwrap(); + assert!(!Arc::ptr_eq(&published, &changed.resolved)); + assert_eq!(changed.resolved.skills[0].body, "second"); + assert_eq!( + activity.fetches.load(std::sync::atomic::Ordering::Relaxed), + 2 + ); + assert_eq!( + activity.archives.load(std::sync::atomic::Ordering::Relaxed), + 2 + ); + runtime.publish(changed); + let republished = runtime.snapshot(); + let unchanged_again = runtime.stage().await.unwrap(); + assert!(Arc::ptr_eq(&republished, &unchanged_again.resolved)); + assert_eq!( + activity.fetches.load(std::sync::atomic::Ordering::Relaxed), + 2 + ); + assert_eq!( + activity.archives.load(std::sync::atomic::Ordering::Relaxed), + 2 + ); + } + + #[tokio::test] + async fn shared_cache_reuses_probed_mutable_head_then_refreshes_on_movement() { + let repository = TestRepository::new(); + repository.commit_file("plugin.json", MANIFEST.as_bytes(), "manifest"); + repository.commit_file( + "skills/live/SKILL.md", + b"---\nname: live\ndescription: Live.\n---\nfirst\n", + "skill", + ); + repository.git(&["branch", "-M", "main"]); + let directory = tempfile::tempdir().unwrap(); + let config = directory.path().join("config.toml"); + fs::write( + &config, + "[plugins.head]\nsource = 'git'\nurl = 'https://plugins.example/repository.git'\n", + ) + .unwrap(); + let (_first_runtime, first_activity) = + stage_local_git_runtime(config.clone(), directory.path(), repository.path()).await; + assert_eq!( + first_activity + .probes + .load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + assert_eq!( + first_activity + .fetches + .load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + assert_eq!( + first_activity + .archives + .load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + + let (runtime, activity) = + stage_local_git_runtime(config, directory.path(), repository.path()).await; + assert_eq!( + activity.probes.load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + assert_eq!( + activity.fetches.load(std::sync::atomic::Ordering::Relaxed), + 0 + ); + assert_eq!( + activity.archives.load(std::sync::atomic::Ordering::Relaxed), + 0 + ); + assert_eq!(runtime.snapshot().skills[0].body, "first"); + let published = runtime.snapshot(); + + repository.commit_file( + "skills/live/SKILL.md", + b"---\nname: live\ndescription: Live.\n---\nsecond\n", + "move head", + ); + let changed = runtime.stage().await.unwrap(); + assert!(!Arc::ptr_eq(&published, &changed.resolved)); + assert_eq!(changed.resolved.skills[0].body, "second"); + assert_eq!( + activity.probes.load(std::sync::atomic::Ordering::Relaxed), + 2 + ); + assert_eq!( + activity.fetches.load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + assert_eq!( + activity.archives.load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + } + + #[tokio::test] + async fn shared_cache_reuses_probed_annotated_tag_without_resolution() { + let repository = TestRepository::new(); + repository.commit_file("plugin.json", MANIFEST.as_bytes(), "manifest"); + let commit = repository.commit_file( + "skills/live/SKILL.md", + b"---\nname: live\ndescription: Live.\n---\nfirst\n", + "skill", + ); + repository.git(&["tag", "-a", "stable", "-m", "stable"]); + assert_ne!(repository.git(&["rev-parse", "stable"]), commit); + let directory = tempfile::tempdir().unwrap(); + let config = directory.path().join("config.toml"); + fs::write( + &config, + "[plugins.tag]\nsource = 'git'\nurl = 'https://plugins.example/repository.git'\nrev = 'refs/tags/stable'\n", + ) + .unwrap(); + let (_first_runtime, first_activity) = + stage_local_git_runtime(config.clone(), directory.path(), repository.path()).await; + assert_eq!( + first_activity + .probes + .load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + assert_eq!( + first_activity + .fetches + .load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + assert_eq!( + first_activity + .archives + .load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + + let (runtime, activity) = + stage_local_git_runtime(config, directory.path(), repository.path()).await; + assert_eq!( + activity.probes.load(std::sync::atomic::Ordering::Relaxed), + 1 + ); + assert_eq!( + activity.fetches.load(std::sync::atomic::Ordering::Relaxed), + 0 + ); + assert_eq!( + activity.archives.load(std::sync::atomic::Ordering::Relaxed), + 0 + ); + assert_eq!(runtime.snapshot().skills[0].body, "first"); + } + + #[test] + fn full_commit_fingerprint_is_stable_without_network() { + let mut configs = BTreeMap::new(); + configs.insert( + "commit".to_string(), + PluginConfig::Git { + url: "https://plugins.example/repository.git".to_string(), + rev: Some("ab".repeat(20)), + subdir: Some("plugin".to_string()), + }, + ); + let directory = tempfile::tempdir().unwrap(); + let fingerprint = |configs: &BTreeMap| { + source_plan( + configs, + directory.path(), + directory.path(), + &GitResolverMode::Https, + ) + .unwrap() + .candidate_fingerprint + }; + let first = fingerprint(&configs); + let second = fingerprint(&configs); + assert_eq!(first, second); + if let Some(PluginConfig::Git { rev, .. }) = configs.get_mut("commit") { + *rev = Some("cd".repeat(20)); + } + assert_ne!(first, fingerprint(&configs)); + } + + #[test] + fn mutable_default_head_probe_changes_only_when_remote_oid_changes() { + let repository = TestRepository::new(); + repository.commit_file("plugin.json", MANIFEST.as_bytes(), "first"); + repository.git(&["branch", "-M", "main"]); + let cache = tempfile::tempdir().unwrap(); + let source_key = sha256_text(&repository.path().to_string_lossy()); + let first = probe_git_revision( + repository.path().as_os_str(), + &source_key, + &GitRevision::DefaultBranch, + cache.path(), + GitProtocol::Local, + &SystemGitRunner::default(), + ) + .unwrap(); + let unchanged = probe_git_revision( + repository.path().as_os_str(), + &source_key, + &GitRevision::DefaultBranch, + cache.path(), + GitProtocol::Local, + &SystemGitRunner::default(), + ) + .unwrap(); + assert_eq!(first, unchanged); + repository.commit_file("version.txt", b"second", "second"); + let changed = probe_git_revision( + repository.path().as_os_str(), + &source_key, + &GitRevision::DefaultBranch, + cache.path(), + GitProtocol::Local, + &SystemGitRunner::default(), + ) + .unwrap(); + assert_ne!(first, changed); + } + + #[test] + fn short_git_probe_rejects_branch_tag_ambiguity() { + let branch = "11".repeat(20); + let tag = "22".repeat(20); + let output = format!("{branch}\trefs/heads/stable\n{tag}\trefs/tags/stable\n"); + let error = parse_git_probe( + output.as_bytes(), + &GitRevision::Ref("stable".into()), + &["refs/heads/stable".into(), "refs/tags/stable".into()], + ) + .unwrap_err(); + assert!(error.contains("ambiguous")); + } + + #[test] + fn planned_annotated_tag_rejects_movement_before_fetch() { + let repository = TestRepository::new(); + let first = repository.commit_file("plugin.json", MANIFEST.as_bytes(), "first"); + repository.git(&["tag", "-a", "stable", "-m", "stable", &first]); + let cache = tempfile::tempdir().unwrap(); + let source_key = sha256_text(&repository.path().to_string_lossy()); + let planned = probe_git_revision( + repository.path().as_os_str(), + &source_key, + &GitRevision::Ref("refs/tags/stable".into()), + cache.path(), + GitProtocol::Local, + &SystemGitRunner::default(), + ) + .unwrap(); + let second = repository.commit_file("version.txt", b"second", "second"); + repository.git(&["tag", "--force", "stable", &second]); + let error = resolve_git_planned( + "https://plugins.example/repository.git", + None, + cache.path(), + &GitResolverMode::Local { + repository: repository.path().to_path_buf(), + activity: Arc::new(GitActivity::default()), + }, + &planned, + ) + .unwrap_err(); + assert!(error.contains("moved while")); + } + #[test] fn validates_aliases_digests_and_paths() { assert!(validate_alias("review-tools").is_ok()); @@ -3725,7 +5071,7 @@ mod tests { let error = resolve_git_source( OsStr::new(remote), &source_key, - &GitRevision::Commit("01".repeat(20)), + GitSourceRevision::Unplanned(&GitRevision::Commit("01".repeat(20))), None, cache.path(), GitProtocol::Https, @@ -3757,9 +5103,15 @@ mod tests { let active_path; { - let staging = StagingDirectory::create(parent.path(), ".staging-").unwrap(); + let staging = StagingDirectory::create(parent.path(), ".staging-", "test").unwrap(); active_path = staging.path().to_path_buf(); assert!(active_path.is_dir()); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = fs::metadata(&active_path).unwrap().permissions().mode(); + assert_eq!(mode & 0o777, 0o700); + } } assert!(!active_path.exists()); } @@ -4124,7 +5476,7 @@ mod tests { let error = resolve_git_source( OsStr::new(url.as_str()), &source_key, - &GitRevision::Ref("refs/tags/stable".into()), + GitSourceRevision::Unplanned(&GitRevision::Ref("refs/tags/stable".into())), None, cache.path(), GitProtocol::Https, diff --git a/src/protocols/acp/v2.rs b/src/protocols/acp/v2.rs index 5eae6a0..aba923f 100644 --- a/src/protocols/acp/v2.rs +++ b/src/protocols/acp/v2.rs @@ -1105,23 +1105,20 @@ async fn prepare_prompt( let _ = reply.send(Err(error)); return Ok(()); } - let current; + let mut current = None; let skills = match skill_source { #[cfg(test)] - PromptSkillSource::Static(skills) => { - current = None; - skills - } + PromptSkillSource::Static(skills) => skills, PromptSkillSource::Runtime(runtime) => { - current = match runtime.current_skills().await { - Ok(current) => Some(current), + let loaded = match runtime.current_skills().await { + Ok(current) => current, Err(error) => { handle.stop_injection_turn(); let _ = reply.send(Err(AcpRuntimeError::Loop(error))); return Ok(()); } }; - ¤t.as_ref().expect("runtime skills were loaded").skills + ¤t.insert(loaded).skills } }; background_jobs.begin_turn(); diff --git a/src/runtime.rs b/src/runtime.rs index 2a755a6..c2d1906 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -254,6 +254,7 @@ pub struct Runtime { session: Mutex, mcp: crate::tools::mcp::McpRuntime, plugin_runtime: Option, + dynamic_skill_tool: Option>, skills: Arc, skill_package_roots: Vec, skill_directories: Vec, @@ -368,6 +369,7 @@ impl Runtime { session: Mutex::new(SessionSelection::default()), mcp: crate::tools::mcp::empty(), plugin_runtime: None, + dynamic_skill_tool: None, skills, skill_package_roots: Vec::new(), skill_directories: Vec::new(), @@ -569,6 +571,10 @@ impl Runtime { return Ok(runtime); }; let snapshot = plugins.snapshot(); + let dynamic_skill_tool: Arc = Arc::new(DynamicSkillTool::new( + runtime.root.clone(), + plugins.clone(), + )?); let mut runtime = Arc::try_unwrap(runtime) .map_err(|_| "could not configure plugins after runtime was shared".to_string())?; runtime.skills = build_skill_tools( @@ -579,6 +585,7 @@ impl Runtime { runtime.skill_package_roots = snapshot.package_roots.clone(); runtime.skill_directories = snapshot.skill_directories.clone(); runtime.plugin_runtime = Some(plugins); + runtime.dynamic_skill_tool = Some(dynamic_skill_tool); Ok(Arc::new(runtime)) } @@ -857,10 +864,8 @@ impl Runtime { .register(Observed::new(ToolSearch::new(self.mcp.clone()))) .register(Observed::new(AuthTool::new(self.mcp.clone()))) .register(Observed::new(McpTool::new(self.mcp.clone()))); - if let Some(plugins) = &self.plugin_runtime { - let skill_tool: Arc = - Arc::new(DynamicSkillTool::new(self.root.clone(), plugins.clone())); - children.register(observe_shared(skill_tool)); + if let Some(skill_tool) = &self.dynamic_skill_tool { + children.register(observe_shared(Arc::clone(skill_tool))); } else { let skill_tools = skills.tool_registry(); if let Some(skill_tool) = skill_tools.get(&ToolName::new("skill")) { @@ -1258,16 +1263,20 @@ struct DynamicSkillTool { } impl DynamicSkillTool { - fn new(root: PathBuf, plugins: crate::plugins::PluginRuntime) -> Self { - let tool = default_skill_tool(&root); - Self { + fn new(root: PathBuf, plugins: crate::plugins::PluginRuntime) -> Result { + let tool = default_skill_tool(&root)?; + let mut spec = tool.spec().clone(); + if let Some(name) = spec.input_schema["properties"]["name"].as_object_mut() { + name.remove("enum"); + } + Ok(Self { root, plugins, - spec: tool.spec().clone(), - } + spec, + }) } - fn base(&self) -> Arc { + fn base(&self) -> Result, String> { default_skill_tool(&self.root) } @@ -1292,38 +1301,41 @@ impl DynamicSkillTool { .collect() } - fn merged_spec(&self) -> Option { - let base = self.base(); - let base_spec = base.current_spec(); - let mut names = Self::base_names(&base); + fn merged_spec(&self) -> ToolSpec { + let base = self.base().ok(); + let base_spec = base.as_ref().and_then(|tool| tool.current_spec()); + let mut names = base.as_ref().map(Self::base_names).unwrap_or_default(); let snapshot = self.plugins.snapshot(); let plugin_skills = snapshot .skills .iter() .filter(|skill| names.insert(skill.name.clone())) .collect::>(); + let mut spec = base_spec.unwrap_or_else(|| self.spec.clone()); if names.is_empty() { - return None; + if let Some(name) = spec.input_schema["properties"]["name"].as_object_mut() { + name.remove("enum"); + } + } else { + let mut names = names.into_iter().map(Value::String).collect::>(); + names.sort_by(|left, right| left.as_str().cmp(&right.as_str())); + spec.input_schema["properties"]["name"]["enum"] = Value::Array(names); } - let mut spec = base_spec.unwrap_or_else(|| self.spec.clone()); - let mut names = names.into_iter().map(Value::String).collect::>(); - names.sort_by(|left, right| left.as_str().cmp(&right.as_str())); - spec.input_schema["properties"]["name"]["enum"] = Value::Array(names); if !plugin_skills.is_empty() { spec.description.push_str("\n\nAgent Plugin skills:\n"); for skill in plugin_skills { let _ = writeln!(spec.description, "- {}: {}", skill.name, skill.description); } } - Some(spec) + spec } } -fn default_skill_tool(root: &Path) -> Arc { +fn default_skill_tool(root: &Path) -> Result, String> { build_skill_registry(root, &[], &[]) .tool_registry() .get(&ToolName::new("skill")) - .expect("default skill registry always provides the skill tool") + .ok_or_else(|| "default skill registry did not provide the skill tool".to_owned()) } #[async_trait] @@ -1333,20 +1345,14 @@ impl Tool for DynamicSkillTool { } fn current_spec(&self) -> Option { - let _generation = self.plugins.try_generation_lease()?; - self.merged_spec() + Some(self.merged_spec()) } fn proposed_requests( &self, request: &ToolRequest, ) -> Result>, ToolError> { - let _generation = self.plugins.try_generation_lease().ok_or_else(|| { - ToolError::Unavailable( - "plugin generation is being refreshed; retry the skill call".into(), - ) - })?; - let base = self.base(); + let base = self.base().map_err(ToolError::Unavailable)?; if Self::base_names(&base).contains(Self::requested_name(request)?) { base.proposed_requests(request) } else { @@ -1361,7 +1367,7 @@ impl Tool for DynamicSkillTool { ) -> Result { let _generation = self.plugins.generation_lease().await; let name = Self::requested_name(&request)?.to_owned(); - let base = self.base(); + let base = self.base().map_err(ToolError::Unavailable)?; if Self::base_names(&base).contains(&name) { return base.invoke(request, context).await; } diff --git a/src/runtime/tests.rs b/src/runtime/tests.rs index ce7988c..622cb02 100644 --- a/src/runtime/tests.rs +++ b/src/runtime/tests.rs @@ -604,7 +604,7 @@ fn project_skills_take_precedence_over_plugin_skills() { } #[tokio::test] -async fn live_plugin_skill_tool_adds_changes_and_removes_skills() { +async fn initially_empty_live_plugin_skill_tool_becomes_actionable_and_tracks_changes() { let root = tempfile::tempdir().unwrap(); let config = root.path().join("config.toml"); std::fs::write(&config, "").unwrap(); @@ -631,14 +631,20 @@ async fn live_plugin_skill_tool_adds_changes_and_removes_skills() { .await .unwrap(); let compose = runtime.compose(0); - let tool = DynamicSkillTool::new(root.path().to_path_buf(), plugins.clone()); + let tool = DynamicSkillTool::new(root.path().to_path_buf(), plugins.clone()).unwrap(); + assert_eq!( + tool.spec().input_schema["properties"]["name"]["type"], + "string" + ); assert!( - !tool - .current_spec() - .map(|spec| spec.input_schema.to_string()) - .unwrap_or_default() - .contains("live-skill") + tool.spec().input_schema["properties"]["name"] + .get("enum") + .is_none() ); + let initial = tool + .current_spec() + .expect("the skill tool remains visible with an empty plugin catalog"); + assert!(!initial.input_schema.to_string().contains("live-skill")); let package = root.path().join("plugin"); let skill = package.join("skills/live-skill"); @@ -672,6 +678,12 @@ async fn live_plugin_skill_tool_adds_changes_and_removes_skills() { .to_string() .contains("live-skill") ); + assert!( + tool.spec().input_schema["properties"]["name"] + .get("enum") + .is_none(), + "the frozen model schema remains open as the catalog changes" + ); assert!(compose.specs()[0].description.contains("live-skill")); let session_id = SessionId::new("session"); @@ -749,6 +761,53 @@ async fn live_plugin_skill_tool_adds_changes_and_removes_skills() { ); } +#[tokio::test] +async fn dynamic_skill_catalog_reads_survive_a_held_generation_writer() { + let root = tempfile::tempdir().unwrap(); + let package = root.path().join("plugin"); + let skill = package.join("skills/plugin-skill"); + std::fs::create_dir_all(&package).unwrap(); + std::fs::write( + package.join("plugin.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/plugin.schema.json","name":"plugin"}"#, + ) + .unwrap(); + write_skill(&skill, "plugin-skill", "Plugin skill.", "plugin body"); + let config = root.path().join("config.toml"); + std::fs::write( + &config, + format!( + "[plugins.live]\nsource = 'path'\npath = '{}'\n", + package.display() + ), + ) + .unwrap(); + let plugins = crate::plugins::PluginRuntime::load( + config, + root.path().to_path_buf(), + root.path().join("cache"), + root.path().join("data"), + ) + .await + .unwrap(); + let tool = DynamicSkillTool::new(root.path().to_path_buf(), plugins.clone()).unwrap(); + let request = ToolRequest::new( + ToolCallId::new("call"), + ToolName::new("skill"), + json!({"name": "plugin-skill"}), + SessionId::new("session"), + TurnId::new("turn"), + ); + + let generation_writer = plugins.generation_writer().await; + let current = tool + .current_spec() + .expect("a writer must not hide the last published skill catalog"); + assert!(current.input_schema.to_string().contains("plugin-skill")); + assert!(tool.proposed_requests(&request).is_ok()); + drop(generation_writer); +} + #[tokio::test] async fn compose_can_load_a_skill_added_after_its_spec_is_frozen() { let root = tempfile::tempdir().unwrap(); diff --git a/src/tools/mcp.rs b/src/tools/mcp.rs index 84c5c36..5d71066 100644 --- a/src/tools/mcp.rs +++ b/src/tools/mcp.rs @@ -1383,28 +1383,33 @@ impl McpRuntime { drop(waited); continue; } - let generation_writer = match &plugin_source { - Some(source) => Some(source.generation_writer().await), - None => None, - }; let mut initialization_guard = None; if !changed.is_empty() { initialization_guard = Some(self.inner.initialization.lock().await); - - { - let mut manager = self.inner.manager.lock().await; - for name in &changed { - let _ = manager.unregister_server(&McpServerId::new(name)).await; - } - for name in &changed { - if let Some(server) = prepared.get(name) { - manager.register_server_with_options( - server.config.clone(), - McpServerOptions::new().with_timeout(CONNECT_TIMEOUT), - ); - } + } + if !changed.is_empty() { + let mut manager = self.inner.manager.lock().await; + for name in &changed { + let _ = manager.unregister_server(&McpServerId::new(name)).await; + } + for name in &changed { + if let Some(server) = prepared.get(name) { + manager.register_server_with_options( + server.config.clone(), + McpServerOptions::new().with_timeout(CONNECT_TIMEOUT), + ); } } + } + + // The generation writer only covers publication. Staging and MCP + // manager initialization above may await without hiding the last + // published skill and MCP generation from readers. + let generation_writer = match &plugin_source { + Some(source) => Some(source.generation_writer().await), + None => None, + }; + if !changed.is_empty() { { let mut servers = self.inner.servers.write().await; for name in &changed { @@ -1437,11 +1442,11 @@ impl McpRuntime { state.plugins = plugin_prepared; state.plugin_entries = plugin_entries; if let (Some(source), Some(staged)) = (&plugin_source, staged_plugins) { - source.publish(staged.resolved); + source.publish(staged); } } - drop(initialization_guard); drop(generation_writer); + drop(initialization_guard); if !deleted.is_empty() { let mut operations = self.inner.operations.lock().await; for name in deleted { @@ -3359,6 +3364,74 @@ mod tests { runtime.reload_config().await.unwrap(); } + #[tokio::test] + async fn plugin_generation_reads_remain_available_while_initialization_is_blocked() { + let directory = tempfile::tempdir().unwrap(); + let config = directory.path().join("config.toml"); + let package = directory.path().join("plugin"); + std::fs::create_dir_all(&package).unwrap(); + std::fs::write( + package.join("plugin.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/plugin.schema.json","name":"blocked-plugin"}"#, + ) + .unwrap(); + std::fs::write( + package.join("mcp.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/mcp.schema.json","mcpServers":{"live":{"type":"streamable-http","url":"https://example.com/mcp"}}}"#, + ) + .unwrap(); + std::fs::write(&config, "").unwrap(); + let plugins = PluginRuntime::new( + config.clone(), + directory.path().to_path_buf(), + directory.path().join("cache"), + directory.path().join("data"), + ResolvedPlugins::default(), + ); + let runtime = super::connect_dynamic( + None::<&Path>, + plugins.clone(), + true, + CredentialStorage::Memory, + ) + .await + .unwrap(); + std::fs::write( + &config, + format!( + "[plugins.blocked]\nsource = 'path'\npath = '{}'\n", + package.display() + ), + ) + .unwrap(); + + let initialization = runtime.inner.initialization.lock().await; + let gate = runtime.operation_gate("live").await; + let reloading = { + let runtime = runtime.clone(); + tokio::spawn(async move { runtime.reload_config().await }) + }; + tokio::time::timeout(Duration::from_secs(1), async { + loop { + if gate.clone().try_read_owned().is_err() { + break; + } + tokio::task::yield_now().await; + } + }) + .await + .expect("reload did not reach the initialization guard"); + + let generation = + tokio::time::timeout(Duration::from_millis(100), plugins.generation_lease()) + .await + .expect("plugin generation reads were blocked by MCP initialization"); + drop(generation); + drop(initialization); + reloading.await.unwrap().unwrap(); + assert_eq!(plugins.snapshot().mcp_plugins.len(), 1); + } + #[tokio::test] async fn queued_reload_rereads_changes_that_arrived_during_the_prior_reload() { let directory = tempfile::tempdir().unwrap();