From a08098258c7a4d50a14a4c23ca8142eeae6a31ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yuniel=20Acosta=20P=C3=A9rez?= <33158051+yacosta738@users.noreply.github.com> Date: Sun, 23 Aug 2026 17:16:31 +0200 Subject: [PATCH 1/4] feat: add deterministic plugin materialization --- Cargo.lock | 2 + Cargo.toml | 2 +- .../design.md | 42 + .../proposal.md | 21 + .../specs/plugin-materialization/spec.md | 44 + .../tasks.md | 11 + src/commands/dev_bench.rs | 1 + src/commands/mod.rs | 1 + src/commands/plugin.rs | 177 ++ src/config.rs | 38 +- src/lib.rs | 2 + src/linker/clean.rs | 1 + src/linker/discovery.rs | 1 + src/linker/mod.rs | 32 +- src/linker/paths.rs | 2 + src/linker/symlinks.rs | 1 + src/linker/timing.rs | 2 + src/main.rs | 31 +- src/plugins.rs | 1785 +++++++++++++++++ src/skills/install.rs | 5 + src/skills/registry.rs | 18 + src/skills/update.rs | 5 + .../.agents/plugins/marketplace.json | 15 + .../engineering/.claude-plugin/plugin.json | 5 + .../engineering/skills/review/SKILL.md | 9 + .../skills/review/references/REFERENCE.md | 3 + .../plugins/unsafe/.claude-plugin/plugin.json | 7 + .../plugins/unsafe/hooks/install.sh | 2 + tests/plugins.rs | 270 +++ tests/plugins_cli.rs | 86 + tests/plugins_mcp.rs | 96 + tests/unit/linker_security.rs | 1 + tests/unit/linker_timing.rs | 1 + tests/unit/platform_symlink.rs | 1 + tests/unit/registry.rs | 10 + .../docs/src/content/docs/reference/cli.mdx | 63 + .../content/docs/reference/configuration.mdx | 34 + 37 files changed, 2817 insertions(+), 10 deletions(-) create mode 100644 openspec/changes/2026-08-23-plugin-materialization/design.md create mode 100644 openspec/changes/2026-08-23-plugin-materialization/proposal.md create mode 100644 openspec/changes/2026-08-23-plugin-materialization/specs/plugin-materialization/spec.md create mode 100644 openspec/changes/2026-08-23-plugin-materialization/tasks.md create mode 100644 src/commands/plugin.rs create mode 100644 src/plugins.rs create mode 100644 tests/fixtures/plugin-marketplace/.agents/plugins/marketplace.json create mode 100644 tests/fixtures/plugin-marketplace/plugins/engineering/.claude-plugin/plugin.json create mode 100644 tests/fixtures/plugin-marketplace/plugins/engineering/skills/review/SKILL.md create mode 100644 tests/fixtures/plugin-marketplace/plugins/engineering/skills/review/references/REFERENCE.md create mode 100644 tests/fixtures/plugin-marketplace/plugins/unsafe/.claude-plugin/plugin.json create mode 100644 tests/fixtures/plugin-marketplace/plugins/unsafe/hooks/install.sh create mode 100644 tests/plugins.rs create mode 100644 tests/plugins_cli.rs create mode 100644 tests/plugins_mcp.rs diff --git a/Cargo.lock b/Cargo.lock index 7b042475..be5d9ca4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -795,6 +795,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b1f9e3d69d39e4862ffed03ed071a76f9a13ba1d9109d355b0f0aa6b15e393c4" dependencies = [ "futures-core", + "futures-sink", ] [[package]] @@ -1898,6 +1899,7 @@ dependencies = [ "base64", "bytes", "encoding_rs", + "futures-channel", "futures-core", "futures-util", "h2", diff --git a/Cargo.toml b/Cargo.toml index e46147d4..3695099b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -52,7 +52,7 @@ pathdiff = "0.2" dirs = "6" # HTTP + async runtime (added for skills.sh integration feature) -reqwest = { version = "0.13.3", features = ["json", "gzip", "stream"] } +reqwest = { version = "0.13.3", features = ["json", "gzip", "stream", "blocking"] } tokio = { version = "1", features = ["rt-multi-thread", "macros", "fs", "time"] } tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["json", "env-filter"] } diff --git a/openspec/changes/2026-08-23-plugin-materialization/design.md b/openspec/changes/2026-08-23-plugin-materialization/design.md new file mode 100644 index 00000000..f63dec3b --- /dev/null +++ b/openspec/changes/2026-08-23-plugin-materialization/design.md @@ -0,0 +1,42 @@ +# Design: Repository-owned plugin materialization + +## Configuration + +`[plugins]` is disabled by default for backwards compatibility. Marketplaces are named source +declarations and selections identify the plugin to resolve. Mutable references are accepted only +by explicit add/update operations. + +The project lockfile is `.agents/plugins.lock.toml` by default. It uses schema `v1`, stable TOML +ordering, full Git commit SHAs or `local:` revisions, plugin tree hashes, per-skill +hashes, MCP names, and source provenance. + +## Apply data flow + +```text +agentsync.toml selection -> plugins.lock.toml -> source verification + -> marketplace manifest -> plugin component validation + -> skills materialized under .agents/skills + -> existing linker fan-out and MCP formatters +``` + +Apply is offline and fails closed when the lockfile, source, component set, or content hash does +not match. Add/update may resolve a GitHub reference and writes the lock atomically. +For Git sources, add/update also materialize a project-owned snapshot under +`.agents/.agentsync-plugin-sources`; apply/status/dry-run never download a source. + +## Supported components + +The first adapter accepts a vendor marketplace manifest at `.agents/plugins/marketplace.json` or +`.claude-plugin/marketplace.json`, a local plugin source, conventional skill directories, and a +root `.mcp.json` with `mcpServers`. AgentSync rejects plugin-level agents, commands, hooks, LSPs, +apps, and vendor-specific MCP fields instead of silently flattening them. + +Plugin MCP names are namespaced as `plugin///`. They are merged with +explicit project servers only after collision checks and are never executed. + +## Atomicity and safety + +Skill copies reject symlinks and unsafe IDs/paths. Existing unmanaged skills are never replaced. +Plugin-owned replacements require matching registry provenance. Lockfile and config writes use +same-directory temporary files and atomic replacement. No vendor CLI, lifecycle hook, executable, +LSP, or MCP process is started. diff --git a/openspec/changes/2026-08-23-plugin-materialization/proposal.md b/openspec/changes/2026-08-23-plugin-materialization/proposal.md new file mode 100644 index 00000000..0850b08f --- /dev/null +++ b/openspec/changes/2026-08-23-plugin-materialization/proposal.md @@ -0,0 +1,21 @@ +# Proposal: Repository-owned plugin materialization + +## Intent + +Provide a deterministic, vendor-neutral path from a selected marketplace plugin to AgentSync's +canonical skills and MCP configuration without relying on Claude or Codex user caches. + +## Scope + +In scope: typed marketplace/plugin selections, immutable project lockfile and provenance, local and +pinned GitHub sources, conventional `skills//SKILL.md` bundles, root `.mcp.json` declarations, +safe apply/update/remove/drift behavior, and Claude/Codex/Gemini/OpenCode fan-out. + +Out of scope: vendor cache installation or enablement, hooks, scripts, binaries, LSPs, apps, and +execution of MCP servers. + +## Compatibility + +Existing skill registry metadata, installed-state JSON, symlink targets, and explicit +`[mcp_servers.*]` configuration remain supported. Plugin provenance is additive and the curated +maintainer registry is not reused as the project plugin lockfile. diff --git a/openspec/changes/2026-08-23-plugin-materialization/specs/plugin-materialization/spec.md b/openspec/changes/2026-08-23-plugin-materialization/specs/plugin-materialization/spec.md new file mode 100644 index 00000000..2156cfa6 --- /dev/null +++ b/openspec/changes/2026-08-23-plugin-materialization/specs/plugin-materialization/spec.md @@ -0,0 +1,44 @@ +# Plugin Materialization + +## Requirement: Locked project sources + +AgentSync MUST require a valid project plugin lockfile before applying enabled selections. The lock +MUST contain an immutable Git commit or local content revision and content hashes. + +### Scenario: Offline apply + +- GIVEN a selected plugin with a valid local lock entry +- WHEN `agentsync apply` runs without network access +- THEN AgentSync verifies the source and materializes the locked content +- AND it MUST NOT resolve a new reference + +### Scenario: Drift is rejected + +- GIVEN a source or installed skill whose content differs from the lock +- WHEN apply or status runs +- THEN AgentSync reports drift and MUST NOT replace unmanaged content + +## Requirement: Safe supported materialization + +AgentSync MUST materialize only conventional skills and standard `.mcp.json` declarations. It MUST +reject unsupported lifecycle components and MUST NOT execute plugin content. + +### Scenario: Skill fan-out + +- GIVEN a locked plugin containing `skills/review/SKILL.md` +- WHEN apply succeeds +- THEN `.agents/skills/review/` contains the validated skill and references +- AND configured agent targets receive it through the existing linker + +### Scenario: MCP fan-out + +- GIVEN a locked plugin containing a valid root `.mcp.json` +- WHEN apply succeeds +- THEN the server is namespaced and generated through the existing agent formatters +- AND no MCP command is started + +### Scenario: Unsupported component + +- GIVEN a plugin containing hooks, agents, commands, apps, or LSP components +- WHEN add or update is requested +- THEN the operation fails explicitly before materialization diff --git a/openspec/changes/2026-08-23-plugin-materialization/tasks.md b/openspec/changes/2026-08-23-plugin-materialization/tasks.md new file mode 100644 index 00000000..8205799c --- /dev/null +++ b/openspec/changes/2026-08-23-plugin-materialization/tasks.md @@ -0,0 +1,11 @@ +# Tasks + +- [x] Add typed plugin configuration and project lockfile model. +- [x] Add local/pinned GitHub source resolution and provenance/hash validation. +- [x] Add safe conventional skill discovery and materialization. +- [x] Merge plugin MCP declarations through the existing linker/generator path. +- [x] Add plugin CLI commands for add, update, list, status, and remove. +- [x] Add local marketplace fixtures and safety/drift integration tests. +- [x] Add broader CLI contract and supported-agent MCP fan-out coverage. +- [x] Complete plugin documentation and targeted repository validation. +- [ ] Complete the full repository suite after the existing catalog fixture checkout is restored. diff --git a/src/commands/dev_bench.rs b/src/commands/dev_bench.rs index 9351e25e..95f85b88 100644 --- a/src/commands/dev_bench.rs +++ b/src/commands/dev_bench.rs @@ -191,6 +191,7 @@ pub(crate) mod fixtures { gitignore: Default::default(), mcp: Default::default(), mcp_servers: Default::default(), + plugins: Default::default(), } } } diff --git a/src/commands/mod.rs b/src/commands/mod.rs index fc3e523e..4912d49d 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -2,6 +2,7 @@ pub mod dev_bench; pub mod doctor; #[cfg(test)] mod doctor_tests; +pub mod plugin; pub mod skill; pub mod status; #[cfg(test)] diff --git a/src/commands/plugin.rs b/src/commands/plugin.rs new file mode 100644 index 00000000..83c36a0f --- /dev/null +++ b/src/commands/plugin.rs @@ -0,0 +1,177 @@ +use agentsync::config::Config; +use agentsync::plugins::{PluginApplyResult, PluginLock, PluginManager, PluginSelection}; +use anyhow::{Result, bail}; +use clap::{Args, Subcommand}; +use std::path::PathBuf; + +#[derive(Subcommand, Debug)] +pub enum PluginCommand { + /// Resolve a configured marketplace/plugin and write its immutable lock entry. + Add(PluginSelectionArgs), + /// Re-resolve a configured marketplace/plugin and refresh its immutable lock entry. + Update(PluginSelectionArgs), + /// List locked repository-owned plugins. + List(PluginOutputArgs), + /// Remove a locked plugin and its AgentSync-owned skills. + Remove(PluginSelectionArgs), + /// Validate locked sources and report materialization drift without changing files. + Status(PluginOutputArgs), +} + +#[derive(Args, Debug)] +pub struct PluginSelectionArgs { + /// Selection in the form marketplace/plugin. + pub selection: String, + /// Output machine-readable JSON. + #[arg(long)] + pub json: bool, +} + +#[derive(Args, Debug)] +pub struct PluginOutputArgs { + /// Output machine-readable JSON. + #[arg(long)] + pub json: bool, +} + +pub fn run_plugin(command: PluginCommand, project_root: PathBuf) -> Result<()> { + let config_path = Config::find_config(&project_root)?; + let config = Config::load(&config_path)?; + let manager = PluginManager::new( + Config::project_root(&config_path), + config_path, + config.plugins, + ); + + match command { + PluginCommand::Add(args) => run_lock_operation(&manager, &args, false), + PluginCommand::Update(args) => run_lock_operation(&manager, &args, true), + PluginCommand::List(args) => run_list(&manager, args.json), + PluginCommand::Remove(args) => run_remove(&manager, &args), + PluginCommand::Status(args) => run_status(&manager, args.json), + } +} + +fn run_lock_operation( + manager: &PluginManager, + args: &PluginSelectionArgs, + update: bool, +) -> Result<()> { + let selection = parse_selection(&args.selection)?; + let result = if update { + manager.update(&selection)? + } else { + manager.add(&selection)? + }; + print_result( + args.json, + if update { "updated" } else { "added" }, + &selection, + &result, + ) +} + +fn run_remove(manager: &PluginManager, args: &PluginSelectionArgs) -> Result<()> { + let selection = parse_selection(&args.selection)?; + let result = manager.remove(&selection, false)?; + print_result(args.json, "removed", &selection, &result) +} + +fn run_list(manager: &PluginManager, json: bool) -> Result<()> { + let lock = match manager.load_lock() { + Ok(lock) => lock, + Err(error) if error.to_string().contains("failed to read plugin lockfile") => { + PluginLock::default() + } + Err(error) => return Err(error), + }; + if json { + println!("{}", serde_json::to_string_pretty(&lock)?); + } else if lock.plugins.is_empty() { + println!("No repository-owned plugins are locked."); + } else { + for plugin in lock.plugins.values() { + println!( + "{} — {} skill(s), {} MCP server(s), revision {}", + plugin.key(), + plugin.skills.len(), + plugin.mcp_servers.len(), + plugin.source.revision + ); + } + } + Ok(()) +} + +fn run_status(manager: &PluginManager, json: bool) -> Result<()> { + let result = manager.apply(true)?; + if json { + println!( + "{}", + serde_json::to_string_pretty(&serde_json::json!({ + "status": "ok", + "skills": result.updated, + "mcp_servers": result.mcp_servers.keys().collect::>(), + }))? + ); + } else { + println!( + "Plugin sources are locked and available ({} skill(s), {} MCP server(s)).", + result.updated, + result.mcp_servers.len() + ); + } + Ok(()) +} + +fn print_result( + json: bool, + status: &str, + selection: &PluginSelection, + result: &PluginApplyResult, +) -> Result<()> { + if json { + println!( + "{}", + serde_json::to_string_pretty(&serde_json::json!({ + "plugin": selection.key(), + "status": status, + "created": result.created, + "updated": result.updated, + "skipped": result.skipped, + "removed": result.removed, + "mcp_servers": result.mcp_servers.keys().collect::>(), + }))? + ); + } else { + println!( + "{} {} (created {}, updated {}, skipped {}, removed {})", + status, + selection.key(), + result.created, + result.updated, + result.skipped, + result.removed + ); + } + Ok(()) +} + +fn parse_selection(value: &str) -> Result { + let (marketplace, plugin) = value + .split_once('/') + .ok_or_else(|| anyhow::anyhow!("plugin selection must use marketplace/plugin"))?; + ensure_no_slash(marketplace, "marketplace")?; + ensure_no_slash(plugin, "plugin")?; + Ok(PluginSelection { + marketplace: marketplace.to_string(), + plugin: plugin.to_string(), + }) +} + +fn ensure_no_slash(value: &str, kind: &str) -> Result<()> { + if value.is_empty() || value.contains('/') || value.contains('\\') { + bail!("invalid {kind} in plugin selection"); + } + Ok(()) +} diff --git a/src/config.rs b/src/config.rs index 1797f849..f6e0c9a9 100644 --- a/src/config.rs +++ b/src/config.rs @@ -52,6 +52,10 @@ pub struct Config { /// Uses BTreeMap to maintain deterministic order without manual sorting. #[serde(default)] pub mcp_servers: BTreeMap, + + /// Repository-owned plugin materialization settings. + #[serde(default)] + pub plugins: crate::plugins::PluginsConfig, } fn default_source_dir() -> String { @@ -250,7 +254,7 @@ pub enum McpMergeStrategy { } /// Configuration for a single MCP server -#[derive(Debug, Deserialize, Serialize, Clone)] +#[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq)] pub struct McpServerConfig { /// Command to execute (for stdio transport) #[serde(skip_serializing_if = "Option::is_none")] @@ -493,6 +497,38 @@ mod tests { assert_eq!(config.source_dir, "."); assert!(!config.compress_agents_md); assert!(config.gitignore.enabled); + assert!(!config.plugins.enabled); + assert_eq!(config.plugins.lockfile, "plugins.lock.toml"); + } + + #[test] + fn test_parse_plugin_config() { + let toml = r#" + [plugins] + enabled = true + lockfile = "plugins.lock.toml" + + [plugins.marketplaces.internal] + source = "../engineering-marketplace" + reference = "main" + + [[plugins.selections]] + marketplace = "internal" + plugin = "engineering" + "#; + + let config: Config = toml::from_str(toml).unwrap(); + assert!(config.plugins.enabled); + assert_eq!(config.plugins.lockfile, "plugins.lock.toml"); + assert_eq!( + config.plugins.marketplaces["internal"].source, + "../engineering-marketplace" + ); + assert_eq!( + config.plugins.marketplaces["internal"].reference.as_deref(), + Some("main") + ); + assert_eq!(config.plugins.selections[0].plugin, "engineering"); } #[test] diff --git a/src/lib.rs b/src/lib.rs index 73ab4639..40d35d52 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,6 +11,7 @@ pub mod init; pub mod linker; pub mod logging; pub mod mcp; +pub mod plugins; pub mod skills; pub mod skills_layout; pub mod update_check; @@ -18,3 +19,4 @@ pub mod update_check; pub use config::Config; pub use linker::{Linker, SyncOptions, SyncResult}; pub use mcp::{McpAgent, McpAgentDocumentation, McpGenerator, McpSyncResult}; +pub use plugins::{PluginApplyResult, PluginManager, PluginSource, PluginsConfig}; diff --git a/src/linker/clean.rs b/src/linker/clean.rs index 248dbfd3..3a2390ff 100644 --- a/src/linker/clean.rs +++ b/src/linker/clean.rs @@ -251,6 +251,7 @@ mod tests { gitignore: Default::default(), mcp: Default::default(), mcp_servers: Default::default(), + plugins: Default::default(), }; let config_path = project_root.join("agentsync.toml"); diff --git a/src/linker/discovery.rs b/src/linker/discovery.rs index 481367c2..742c4182 100644 --- a/src/linker/discovery.rs +++ b/src/linker/discovery.rs @@ -438,6 +438,7 @@ mod tests { gitignore: Default::default(), mcp: Default::default(), mcp_servers: Default::default(), + plugins: Default::default(), } } diff --git a/src/linker/mod.rs b/src/linker/mod.rs index bd93685b..9234aab6 100644 --- a/src/linker/mod.rs +++ b/src/linker/mod.rs @@ -254,6 +254,19 @@ impl Linker { &self, dry_run: bool, agents_filter: Option<&Vec>, + ) -> Result { + self.sync_mcp_with_servers(dry_run, agents_filter, &BTreeMap::new()) + } + + /// Sync MCP configurations while adding repository-owned plugin servers. + /// + /// Plugin servers are kept separate from the parsed project config until this point so the + /// existing user-owned `[mcp_servers.*]` contract remains unchanged. + pub fn sync_mcp_with_servers( + &self, + dry_run: bool, + agents_filter: Option<&Vec>, + plugin_servers: &BTreeMap, ) -> Result { use crate::mcp::McpGenerator; @@ -261,7 +274,7 @@ impl Linker { return Ok(crate::mcp::McpSyncResult::default()); } - if self.config.mcp_servers.is_empty() { + if self.config.mcp_servers.is_empty() && plugin_servers.is_empty() { return Ok(crate::mcp::McpSyncResult::default()); } @@ -299,10 +312,19 @@ impl Linker { return Ok(crate::mcp::McpSyncResult::default()); } - let generator = McpGenerator::new( - self.config.mcp_servers.clone(), - self.config.mcp.merge_strategy, - ); + let mut servers = self.config.mcp_servers.clone(); + for (name, server) in plugin_servers { + if let Some(existing) = servers.get(name) { + anyhow::ensure!( + existing == server, + "MCP server collision between project configuration and plugin: {name}" + ); + } else { + servers.insert(name.clone(), server.clone()); + } + } + + let generator = McpGenerator::new(servers, self.config.mcp.merge_strategy); generator.generate_all(&self.project_root, &filtered_agents, dry_run) } } diff --git a/src/linker/paths.rs b/src/linker/paths.rs index 222c7f87..a12450d3 100644 --- a/src/linker/paths.rs +++ b/src/linker/paths.rs @@ -322,6 +322,7 @@ mod tests { gitignore: Default::default(), mcp: Default::default(), mcp_servers: Default::default(), + plugins: Default::default(), }; Linker::new(config, config_path) @@ -613,6 +614,7 @@ mod tests { gitignore: Default::default(), mcp: Default::default(), mcp_servers: Default::default(), + plugins: Default::default(), }; let config_path = project_root.join("agentsync.toml"); diff --git a/src/linker/symlinks.rs b/src/linker/symlinks.rs index c3e2eb5c..137449d8 100644 --- a/src/linker/symlinks.rs +++ b/src/linker/symlinks.rs @@ -372,6 +372,7 @@ mod tests { gitignore: Default::default(), mcp: Default::default(), mcp_servers: Default::default(), + plugins: Default::default(), }; Linker::new(config, config_path) diff --git a/src/linker/timing.rs b/src/linker/timing.rs index c9526426..60aa820b 100644 --- a/src/linker/timing.rs +++ b/src/linker/timing.rs @@ -213,6 +213,7 @@ mod tests { gitignore: Default::default(), mcp: Default::default(), mcp_servers: Default::default(), + plugins: Default::default(), } } @@ -320,6 +321,7 @@ mod tests { gitignore: Default::default(), mcp: Default::default(), mcp_servers: Default::default(), + plugins: Default::default(), } } diff --git a/src/main.rs b/src/main.rs index a8299883..1472cb69 100644 --- a/src/main.rs +++ b/src/main.rs @@ -9,12 +9,13 @@ use std::env; use std::path::PathBuf; use agentsync::logging::LogFormat; -use agentsync::{Linker, SyncOptions, SyncResult, config::Config, gitignore, init}; +use agentsync::{Linker, PluginManager, SyncOptions, SyncResult, config::Config, gitignore, init}; use tracing_subscriber::filter::LevelFilter; mod commands; mod output; use commands::dev_bench::{DevBenchArgs, run_dev_bench}; use commands::doctor::run_doctor; +use commands::plugin::{PluginCommand, run_plugin}; use commands::skill::{SkillCommand, run_skill}; use commands::status::{StatusArgs, run_status}; use output::{ @@ -86,6 +87,14 @@ enum Commands { #[arg(short, long)] project_root: Option, }, + /// Manage repository-owned, vendor-neutral plugins. + Plugin { + #[command(subcommand)] + cmd: PluginCommand, + /// Root of the project (defaults to CWD) + #[arg(short, long)] + project_root: Option, + }, /// Run diagnostic and health check Doctor { /// Project root (defaults to CWD) @@ -194,6 +203,12 @@ fn run() -> Result<()> { run_skill(cmd, root)?; Ok(()) }), + Commands::Plugin { cmd, project_root } => run_in_root_span("plugin", || { + let root = + current_project_root(project_root, || env::current_dir().map_err(Into::into))?; + run_plugin(cmd, root)?; + Ok(()) + }), Commands::Status { args, project_root } => run_in_root_span("status", || { let project_root = current_project_root(project_root, || env::current_dir().map_err(Into::into))?; @@ -339,6 +354,12 @@ fn handle_apply(args: ApplyArgs) -> Result<()> { tracing::debug!(config_path = %config_path.display(), "Using config"); } let config = Config::load(&config_path)?; + let plugin_manager = PluginManager::new( + Config::project_root(&config_path), + config_path.clone(), + config.plugins.clone(), + ); + let plugin_result = plugin_manager.apply(args.dry_run)?; let linker = Linker::new(config, config_path); let use_color = human_use_color(); if args.dry_run { @@ -376,12 +397,15 @@ fn handle_apply(args: ApplyArgs) -> Result<()> { if !args.no_gitignore { handle_apply_gitignore(&linker, args.dry_run, use_color)?; } - if linker.config().mcp.enabled && !linker.config().mcp_servers.is_empty() { + if linker.config().mcp.enabled + && (!linker.config().mcp_servers.is_empty() || !plugin_result.mcp_servers.is_empty()) + { handle_apply_mcp( &linker, options.dry_run, use_color, options.agents.as_ref(), + &plugin_result.mcp_servers, &mut result, )?; } @@ -430,11 +454,12 @@ fn handle_apply_mcp( dry_run: bool, use_color: bool, agents: Option<&Vec>, + plugin_servers: &std::collections::BTreeMap, result: &mut SyncResult, ) -> Result<()> { println!(); print_lines(&render_mcp_phase(dry_run, use_color)); - match linker.sync_mcp(dry_run, agents) { + match linker.sync_mcp_with_servers(dry_run, agents, plugin_servers) { Ok(mcp_result) => { if mcp_result.created > 0 || mcp_result.updated > 0 diff --git a/src/plugins.rs b/src/plugins.rs new file mode 100644 index 00000000..36d894cb --- /dev/null +++ b/src/plugins.rs @@ -0,0 +1,1785 @@ +//! Repository-owned plugin discovery, locking, and materialization. +//! +//! This module intentionally does not invoke vendor CLIs or execute anything from a plugin +//! source. It only reads manifests, validates content, copies skills, and returns MCP +//! declarations for the existing configuration generator. + +use crate::config::McpServerConfig; +use anyhow::{Context, Result, bail, ensure}; +use serde::{Deserialize, Serialize}; +use serde_json::Value; +use sha2::{Digest, Sha256}; +use std::collections::{BTreeMap, BTreeSet}; +use std::fs; +use std::path::{Component, Path, PathBuf}; +use tempfile::{NamedTempFile, TempDir}; +use walkdir::WalkDir; + +const PLUGIN_LOCK_SCHEMA_VERSION: &str = "v1"; +const DEFAULT_PLUGIN_LOCKFILE: &str = "plugins.lock.toml"; + +fn default_plugin_lockfile() -> String { + DEFAULT_PLUGIN_LOCKFILE.to_string() +} + +/// Project-level plugin configuration from `agentsync.toml`. +#[derive(Debug, Clone, Deserialize)] +pub struct PluginsConfig { + /// Enable repository-owned plugin materialization. Disabled by default for compatibility. + #[serde(default)] + pub enabled: bool, + /// Lockfile path relative to the config file's directory. + #[serde(default = "default_plugin_lockfile")] + pub lockfile: String, + /// Named marketplace sources. + #[serde(default)] + pub marketplaces: BTreeMap, + /// Explicitly selected plugins. + #[serde(default)] + pub selections: Vec, +} + +impl Default for PluginsConfig { + fn default() -> Self { + Self { + enabled: false, + lockfile: default_plugin_lockfile(), + marketplaces: BTreeMap::new(), + selections: Vec::new(), + } + } +} + +/// A marketplace declaration. `reference` is used only by explicit add/update operations; +/// apply reads the immutable revision recorded in the lockfile. +#[derive(Debug, Clone, Deserialize)] +pub struct MarketplaceConfig { + pub source: String, + #[serde(default)] + pub reference: Option, +} + +/// A selected plugin in a named marketplace. +#[derive(Debug, Clone, Deserialize, PartialEq, Eq, PartialOrd, Ord)] +pub struct PluginSelection { + pub marketplace: String, + pub plugin: String, +} + +impl PluginSelection { + pub fn key(&self) -> String { + format!("{}/{}", self.marketplace, self.plugin) + } +} + +/// The source kind recorded in a plugin lock entry. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "lowercase")] +pub enum LockedSourceKind { + Local, + Git, +} + +/// An immutable source identity used during apply. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct LockedSource { + pub kind: LockedSourceKind, + /// Relative local path or Git repository URL. + pub location: String, + /// Full Git commit SHA or `local:`. + pub revision: String, +} + +/// Public name for the immutable plugin source identity used by the project lockfile. +pub type PluginSource = LockedSource; + +/// Provenance recorded for a materialized plugin. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct PluginProvenance { + pub marketplace_manifest: String, + pub plugin_path: String, + pub resolved_revision: String, + pub content_sha256: String, +} + +/// A skill selected from a locked plugin. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct LockedSkill { + pub id: String, + pub path: String, + pub content_sha256: String, +} + +/// A plugin entry in the project lockfile. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct LockedPlugin { + pub marketplace: String, + pub plugin: String, + pub version: Option, + pub source: LockedSource, + pub content_sha256: String, + pub skills: Vec, + pub mcp_servers: Vec, + pub unsupported_components: Vec, + pub provenance: PluginProvenance, +} + +impl LockedPlugin { + pub fn key(&self) -> String { + format!("{}/{}", self.marketplace, self.plugin) + } +} + +/// Deterministic project plugin lockfile. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct PluginLock { + pub schema_version: String, + pub plugins: BTreeMap, +} + +impl Default for PluginLock { + fn default() -> Self { + Self { + schema_version: PLUGIN_LOCK_SCHEMA_VERSION.to_string(), + plugins: BTreeMap::new(), + } + } +} + +impl PluginLock { + pub fn load(path: &Path) -> Result { + let content = fs::read_to_string(path) + .with_context(|| format!("failed to read plugin lockfile: {}", path.display()))?; + let lock: Self = toml::from_str(&content) + .with_context(|| format!("failed to parse plugin lockfile: {}", path.display()))?; + lock.validate()?; + Ok(lock) + } + + pub fn save_atomic(&self, path: &Path) -> Result<()> { + self.validate()?; + let body = toml::to_string_pretty(self).context("failed to serialize plugin lockfile")?; + let parent = path + .parent() + .ok_or_else(|| anyhow::anyhow!("plugin lockfile has no parent"))?; + fs::create_dir_all(parent).with_context(|| { + format!("failed to create lockfile directory: {}", parent.display()) + })?; + let temporary = NamedTempFile::new_in(parent).with_context(|| { + format!( + "failed to create temporary plugin lockfile in {}", + parent.display() + ) + })?; + fs::write(temporary.path(), body).with_context(|| { + format!( + "failed to write temporary plugin lockfile: {}", + temporary.path().display() + ) + })?; + temporary + .persist(path) + .map_err(|error| error.error) + .with_context(|| format!("failed to replace plugin lockfile: {}", path.display()))?; + Ok(()) + } + + pub fn validate(&self) -> Result<()> { + ensure!( + self.schema_version == PLUGIN_LOCK_SCHEMA_VERSION, + "unsupported plugin lock schema: {}", + self.schema_version + ); + for (key, plugin) in &self.plugins { + validate_identifier("marketplace", &plugin.marketplace)?; + validate_identifier("plugin", &plugin.plugin)?; + ensure!( + key == &plugin.key(), + "plugin lock key does not match plugin identity: {key}" + ); + validate_source(&plugin.source)?; + validate_hash("content_sha256", &plugin.content_sha256)?; + ensure!( + plugin.provenance.content_sha256 == plugin.content_sha256, + "plugin provenance hash does not match lock entry: {key}" + ); + ensure!( + plugin.provenance.resolved_revision == plugin.source.revision, + "plugin provenance revision does not match lock entry: {key}" + ); + let mut skill_ids = BTreeSet::new(); + for skill in &plugin.skills { + validate_identifier("skill", &skill.id)?; + ensure!( + skill_ids.insert(&skill.id), + "duplicate plugin skill: {}", + skill.id + ); + validate_relative_path(&skill.path)?; + validate_hash("skill content_sha256", &skill.content_sha256)?; + } + for server in &plugin.mcp_servers { + ensure!( + !server.is_empty(), + "plugin MCP server name must not be empty" + ); + } + for component in &plugin.unsupported_components { + ensure!( + !component.is_empty(), + "unsupported component name must not be empty" + ); + } + ensure!( + plugin.unsupported_components.is_empty(), + "plugin {} contains unsupported components: {}", + key, + plugin.unsupported_components.join(", ") + ); + validate_relative_path(&plugin.provenance.marketplace_manifest)?; + validate_relative_path(&plugin.provenance.plugin_path)?; + } + Ok(()) + } +} + +/// The result of applying project-owned plugins. +#[derive(Debug, Default)] +pub struct PluginApplyResult { + pub created: usize, + pub updated: usize, + pub skipped: usize, + pub removed: usize, + pub errors: usize, + pub mcp_servers: BTreeMap, +} + +/// Repository-owned plugin operations. +pub struct PluginManager { + project_root: PathBuf, + config_path: PathBuf, + config: PluginsConfig, +} + +impl PluginManager { + pub fn new(project_root: PathBuf, config_path: PathBuf, config: PluginsConfig) -> Self { + Self { + project_root, + config_path, + config, + } + } + + pub fn lock_path(&self) -> Result { + validate_relative_path(&self.config.lockfile)?; + Ok(self + .config_path + .parent() + .unwrap_or_else(|| Path::new(".")) + .join(&self.config.lockfile)) + } + + pub fn apply(&self, dry_run: bool) -> Result { + if !self.config.enabled || self.config.selections.is_empty() { + return Ok(PluginApplyResult::default()); + } + + let lock_path = self.lock_path()?; + let lock = PluginLock::load(&lock_path).with_context(|| { + format!( + "plugin selections require an immutable lockfile; run `agentsync plugin add` or `agentsync plugin update` first ({})", + lock_path.display() + ) + })?; + let selections: BTreeSet<_> = self + .config + .selections + .iter() + .map(PluginSelection::key) + .collect(); + let mut result = PluginApplyResult::default(); + + for key in selections { + let locked = lock + .plugins + .get(&key) + .with_context(|| format!("plugin selection is missing from lockfile: {key}"))?; + ensure!( + locked.unsupported_components.is_empty(), + "plugin {key} contains unsupported components: {}", + locked.unsupported_components.join(", ") + ); + let source = self.materialize_source(&locked.source)?; + let discovered = discover_plugin(source.root(), &locked.marketplace, &locked.plugin)?; + ensure!( + discovered.content_sha256 == locked.content_sha256, + "plugin content drift detected for {key}: expected {}, got {}", + locked.content_sha256, + discovered.content_sha256 + ); + ensure!( + discovered.unsupported_components.is_empty(), + "plugin {key} contains unsupported components: {}", + discovered.unsupported_components.join(", ") + ); + let discovered_skill_ids: BTreeSet<_> = discovered + .skills + .iter() + .map(|skill| skill.id.as_str()) + .collect(); + let locked_skill_ids: BTreeSet<_> = locked + .skills + .iter() + .map(|skill| skill.id.as_str()) + .collect(); + ensure!( + discovered_skill_ids == locked_skill_ids, + "plugin skill set drift detected for {key}" + ); + let plugin_mcp = + discovered.namespaced_mcp_servers(&locked.marketplace, &locked.plugin)?; + let discovered_mcp_names: Vec<_> = discovered.mcp_servers.keys().cloned().collect(); + ensure!( + discovered_mcp_names == locked.mcp_servers, + "plugin MCP declaration drift detected for {key}" + ); + for (name, server) in plugin_mcp { + if result.mcp_servers.insert(name.clone(), server).is_some() { + bail!("duplicate plugin MCP server: {name}"); + } + } + + if dry_run { + result.updated += discovered.skills.len(); + result.skipped += usize::from(discovered.skills.is_empty()); + } else { + for skill in &discovered.skills { + let locked_skill = locked + .skills + .iter() + .find(|candidate| candidate.id == skill.id) + .with_context(|| { + format!("skill missing from plugin lock: {key}/{}", skill.id) + })?; + ensure!( + locked_skill.content_sha256 == skill.content_sha256, + "skill content drift detected for {key}/{}", + skill.id + ); + materialize_skill(&self.project_root, locked, skill, &mut result)?; + } + } + drop(source); + } + + Ok(result) + } + + /// Resolve and lock a selected plugin from the configured marketplace. + pub fn add(&self, selection: &PluginSelection) -> Result { + self.lock_selection(selection) + } + + pub fn update(&self, selection: &PluginSelection) -> Result { + self.lock_selection(selection) + } + + pub fn load_lock(&self) -> Result { + PluginLock::load(&self.lock_path()?) + } + + pub fn remove(&self, selection: &PluginSelection, dry_run: bool) -> Result { + let lock_path = self.lock_path()?; + let original_lock = PluginLock::load(&lock_path)?; + let key = selection.key(); + let locked = original_lock + .plugins + .get(&key) + .cloned() + .with_context(|| format!("plugin is not locked: {key}"))?; + let mut result = PluginApplyResult::default(); + if dry_run { + result.removed = locked.skills.len(); + return Ok(result); + } + + let target_root = self.project_root.join(".agents/skills"); + let registry_path = target_root.join("registry.json"); + let registry = crate::skills::registry::read_registry(®istry_path).ok(); + let mut targets = Vec::new(); + let mut registry_ids = Vec::new(); + let owner = crate::skills::registry::PluginOwner { + marketplace: selection.marketplace.clone(), + plugin: selection.plugin.clone(), + revision: locked.source.revision.clone(), + }; + for skill in &locked.skills { + let target = target_root.join(&skill.id); + let metadata = fs::symlink_metadata(&target).ok(); + let entry = registry + .as_ref() + .and_then(|registry| registry.skills.as_ref()) + .and_then(|skills| skills.get(&skill.id)); + let owners = entry.map(entry_plugin_owners).unwrap_or_default(); + let owned = owners.contains(&owner); + if let Some(metadata) = metadata { + ensure!( + owned, + "refusing to remove unmanaged skill: {}", + target.display() + ); + if owners.len() == 1 { + ensure!( + !metadata.file_type().is_symlink() && metadata.is_dir(), + "refusing to remove unsafe skill destination: {}", + target.display() + ); + targets.push((skill.id.clone(), target)); + } + } + if owned { + registry_ids.push(skill.id.clone()); + } + } + + let original_config = fs::read(&self.config_path).with_context(|| { + format!( + "failed to read config before removing plugin: {}", + self.config_path.display() + ) + })?; + let original_registry = if registry_path.is_file() { + Some(fs::read(®istry_path).with_context(|| { + format!("failed to read skill registry: {}", registry_path.display()) + })?) + } else { + None + }; + let backup = if targets.is_empty() { + None + } else { + fs::create_dir_all(&target_root).with_context(|| { + format!("failed to create skill root: {}", target_root.display()) + })?; + Some(TempDir::new_in(&target_root).with_context(|| { + format!( + "failed to create temporary plugin removal directory in {}", + target_root.display() + ) + })?) + }; + let mut backups = Vec::new(); + let operation = (|| -> Result<()> { + if let Some(backup) = &backup { + for (id, target) in &targets { + let backup_path = backup.path().join(id); + fs::rename(target, &backup_path).with_context(|| { + format!( + "failed to stage plugin-owned skill removal: {}", + target.display() + ) + })?; + backups.push((target.clone(), backup_path)); + } + } + let registry_ids = registry_ids.iter().map(String::as_str).collect::>(); + remove_plugin_owner_entries_atomic(®istry_path, ®istry_ids, &owner)?; + remove_selection_from_config(&self.config_path, selection)?; + let mut lock = original_lock.clone(); + lock.plugins.remove(&key); + lock.save_atomic(&lock_path)?; + Ok(()) + })(); + if let Err(error) = operation { + let rollback = (|| -> Result<()> { + for (target, backup_path) in backups.iter().rev() { + if target.exists() { + remove_path_safely(target)?; + } + fs::rename(backup_path, target).with_context(|| { + format!("failed to restore plugin-owned skill: {}", target.display()) + })?; + } + if let Some(original_registry) = &original_registry { + write_atomic_file(®istry_path, original_registry)?; + } else if registry_path.exists() { + fs::remove_file(®istry_path)?; + } + write_atomic_file(&self.config_path, &original_config)?; + original_lock.save_atomic(&lock_path)?; + Ok(()) + })(); + if let Err(rollback_error) = rollback { + return Err(anyhow::anyhow!( + "plugin removal failed: {error}; rollback failed: {rollback_error}" + )); + } + return Err(error); + } + result.removed = targets.len(); + Ok(result) + } + + fn lock_selection(&self, selection: &PluginSelection) -> Result { + validate_selection(selection)?; + ensure!( + self.config.enabled, + "plugin materialization is disabled; set [plugins].enabled = true first" + ); + let marketplace = self + .config + .marketplaces + .get(&selection.marketplace) + .with_context(|| format!("unknown plugin marketplace: {}", selection.marketplace))?; + let source = resolve_marketplace_source(&self.config_path, marketplace, true)?; + let discovered = discover_plugin(source.root(), &selection.marketplace, &selection.plugin)?; + ensure!( + discovered.unsupported_components.is_empty(), + "plugin {} contains unsupported components: {}", + selection.key(), + discovered.unsupported_components.join(", ") + ); + let locked = + discovered.to_locked_plugin(&selection.marketplace, &selection.plugin, &source)?; + self.cache_marketplace_source(&source)?; + let lock_path = self.lock_path()?; + let previous_lock = if lock_path.exists() { + Some(PluginLock::load(&lock_path)?) + } else { + None + }; + let original_config = if self.config.selections.contains(selection) { + None + } else { + Some(fs::read(&self.config_path).with_context(|| { + format!( + "failed to read config before adding plugin selection: {}", + self.config_path.display() + ) + })?) + }; + let mut lock = previous_lock.clone().unwrap_or_default(); + lock.plugins.insert(locked.key(), locked); + lock.save_atomic(&lock_path)?; + if original_config.is_some() + && let Err(error) = add_selection_to_config(&self.config_path, selection) + { + rollback_plugin_lock(&lock_path, previous_lock.as_ref())?; + return Err(error); + } + let mut apply_config = self.config.clone(); + if !apply_config.selections.contains(selection) { + apply_config.selections.push(selection.clone()); + } + let apply_manager = Self::new( + self.project_root.clone(), + self.config_path.clone(), + apply_config, + ); + match apply_manager.apply(false) { + Ok(result) => Ok(result), + Err(error) => { + rollback_plugin_lock(&lock_path, previous_lock.as_ref())?; + if let Some(original) = original_config { + write_atomic_file(&self.config_path, &original).with_context(|| { + format!( + "failed to roll back plugin selection config: {}", + self.config_path.display() + ) + })?; + } + Err(error) + } + } + } + + fn git_source_cache_path(&self, source: &LockedSource) -> Result { + ensure!( + source.kind == LockedSourceKind::Git, + "Git source cache requested for a non-Git plugin source" + ); + let mut hasher = Sha256::new(); + hasher.update(source.location.as_bytes()); + let repository_digest = format_digest(hasher.finalize()); + let parent = self.config_path.parent().unwrap_or_else(|| Path::new(".")); + Ok(parent.join(".agentsync-plugin-sources").join(format!( + "{}-{}", + &repository_digest[..16], + source.revision + ))) + } + + fn cache_marketplace_source(&self, source: &ResolvedMarketplaceSource) -> Result<()> { + if source.locked_source.kind != LockedSourceKind::Git { + return Ok(()); + } + let destination = self.git_source_cache_path(&source.locked_source)?; + if let Ok(metadata) = fs::symlink_metadata(&destination) { + ensure!( + metadata.is_dir() && !metadata.file_type().is_symlink(), + "refusing to use symlinked Git plugin source snapshot: {}", + destination.display() + ); + return Ok(()); + } + let parent = destination + .parent() + .ok_or_else(|| anyhow::anyhow!("Git plugin source cache has no parent"))?; + fs::create_dir_all(parent).with_context(|| { + format!( + "failed to create Git plugin source cache directory: {}", + parent.display() + ) + })?; + let staging = TempDir::new_in(parent).with_context(|| { + format!( + "failed to create temporary Git plugin source cache in {}", + parent.display() + ) + })?; + let staged = staging.path().join("source"); + copy_directory_without_symlinks(source.root(), &staged)?; + fs::rename(&staged, &destination).with_context(|| { + format!( + "failed to materialize Git plugin source snapshot: {}", + destination.display() + ) + })?; + Ok(()) + } + + fn materialize_source(&self, source: &LockedSource) -> Result { + match source.kind { + LockedSourceKind::Local => { + ensure!( + !Path::new(&source.location).is_absolute() + && !source.location.contains("://") + && !source.location.contains(':'), + "locked local plugin source must be a relative path: {}", + source.location + ); + let root = self + .config_path + .parent() + .unwrap_or_else(|| Path::new(".")) + .join(&source.location) + .canonicalize() + .with_context(|| { + format!("plugin source is unavailable: {}", source.location) + })?; + ensure!( + root.is_dir(), + "plugin local source is not a directory: {}", + root.display() + ); + let actual = format!("local:{}", hash_tree(&root)?); + ensure!( + actual == source.revision, + "local plugin source drift detected: {}", + root.display() + ); + Ok(ResolvedSource { root, temp: None }) + } + LockedSourceKind::Git => { + let root = self.git_source_cache_path(source)?; + ensure!( + root.is_dir(), + "offline Git plugin source snapshot is unavailable: {} (run `agentsync plugin update` first)", + root.display() + ); + Ok(ResolvedSource { root, temp: None }) + } + } + } +} + +struct ResolvedSource { + root: PathBuf, + temp: Option, +} + +impl ResolvedSource { + fn root(&self) -> &Path { + &self.root + } +} + +impl Drop for ResolvedSource { + fn drop(&mut self) { + let _ = self.temp.take(); + } +} + +#[derive(Debug)] +struct DiscoveredSkill { + id: String, + path: PathBuf, + relative_path: String, + content_sha256: String, +} + +#[derive(Debug)] +struct DiscoveredPlugin { + version: Option, + plugin_path: String, + marketplace_manifest: String, + content_sha256: String, + skills: Vec, + mcp_servers: BTreeMap, + unsupported_components: Vec, +} + +impl DiscoveredPlugin { + fn to_locked_plugin( + &self, + marketplace: &str, + plugin: &str, + source: &ResolvedMarketplaceSource, + ) -> Result { + let skills = self + .skills + .iter() + .map(|skill| LockedSkill { + id: skill.id.clone(), + path: skill.relative_path.clone(), + content_sha256: skill.content_sha256.clone(), + }) + .collect(); + let mcp_servers = self.mcp_servers.keys().cloned().collect::>(); + let provenance = PluginProvenance { + marketplace_manifest: self.marketplace_manifest.clone(), + plugin_path: self.plugin_path.clone(), + resolved_revision: source.locked_source.revision.clone(), + content_sha256: self.content_sha256.clone(), + }; + Ok(LockedPlugin { + marketplace: marketplace.to_string(), + plugin: plugin.to_string(), + version: self.version.clone(), + source: source.locked_source.clone(), + content_sha256: self.content_sha256.clone(), + skills, + mcp_servers, + unsupported_components: self.unsupported_components.clone(), + provenance, + }) + } + + fn namespaced_mcp_servers( + &self, + marketplace: &str, + plugin: &str, + ) -> Result> { + let mut result = BTreeMap::new(); + for (name, server) in &self.mcp_servers { + let key = format!("plugin/{marketplace}/{plugin}/{name}"); + ensure!( + result.insert(key.clone(), server.clone()).is_none(), + "duplicate plugin MCP server: {key}" + ); + } + Ok(result) + } +} + +struct ResolvedMarketplaceSource { + root: PathBuf, + locked_source: LockedSource, + temp: Option, +} + +impl ResolvedMarketplaceSource { + fn root(&self) -> &Path { + &self.root + } +} + +impl Drop for ResolvedMarketplaceSource { + fn drop(&mut self) { + let _ = self.temp.take(); + } +} + +fn resolve_marketplace_source( + config_path: &Path, + marketplace: &MarketplaceConfig, + allow_network: bool, +) -> Result { + let source = marketplace.source.trim(); + ensure!( + !source.is_empty(), + "plugin marketplace source must not be empty" + ); + if source.starts_with("file://") { + bail!("file:// plugin marketplace sources are not supported"); + } + if is_local_source(source) { + ensure!( + !Path::new(source).is_absolute() && !source.contains(':'), + "absolute plugin marketplace paths are not allowed" + ); + let root = config_path + .parent() + .unwrap_or_else(|| Path::new(".")) + .join(source) + .canonicalize() + .with_context(|| format!("plugin marketplace source is unavailable: {source}"))?; + ensure!( + root.is_dir(), + "plugin marketplace source is not a directory: {}", + root.display() + ); + let revision = format!("local:{}", hash_tree(&root)?); + return Ok(ResolvedMarketplaceSource { + root, + locked_source: LockedSource { + kind: LockedSourceKind::Local, + location: source.to_string(), + revision: revision.clone(), + }, + temp: None, + }); + } + + ensure!(allow_network, "network access is disabled during apply"); + let reference = marketplace + .reference + .as_deref() + .filter(|reference| !reference.trim().is_empty()) + .ok_or_else(|| anyhow::anyhow!("Git plugin marketplace requires a reference"))?; + let revision = resolve_git_reference(source, reference)?; + let archive = github_archive_url(source, &revision)?; + let temp = blocking_fetch_archive(&archive)?; + Ok(ResolvedMarketplaceSource { + root: temp.path().to_path_buf(), + locked_source: LockedSource { + kind: LockedSourceKind::Git, + location: source.to_string(), + revision: revision.clone(), + }, + temp: Some(temp), + }) +} + +fn discover_plugin(root: &Path, marketplace: &str, plugin_name: &str) -> Result { + validate_identifier("marketplace", marketplace)?; + validate_identifier("plugin", plugin_name)?; + let manifest_candidates = [ + ( + ".agents/plugins/marketplace.json", + root.join(".agents/plugins/marketplace.json"), + ), + ( + ".claude-plugin/marketplace.json", + root.join(".claude-plugin/marketplace.json"), + ), + ]; + let mut selected_manifest = None; + for (relative, path) in &manifest_candidates { + if !path.is_file() { + continue; + } + let content = fs::read_to_string(path) + .with_context(|| format!("failed to read marketplace manifest: {}", path.display()))?; + let value = serde_json::from_str::(&content) + .with_context(|| format!("failed to parse marketplace manifest: {}", path.display()))?; + selected_manifest = Some(((*relative).to_string(), value)); + break; + } + let (manifest_path, manifest) = selected_manifest.ok_or_else(|| { + anyhow::anyhow!( + "plugin marketplace manifest not found in {}", + root.display() + ) + })?; + let plugins = manifest + .get("plugins") + .and_then(Value::as_array) + .ok_or_else(|| { + anyhow::anyhow!("marketplace manifest has no plugins array: {manifest_path}") + })?; + let entry = plugins + .iter() + .find(|entry| entry.get("name").and_then(Value::as_str) == Some(plugin_name)) + .with_context(|| format!("plugin not found in marketplace {marketplace}: {plugin_name}"))?; + let source_path = entry + .get("source") + .map(parse_plugin_source) + .transpose()? + .flatten() + .or_else(|| { + entry + .get("path") + .and_then(Value::as_str) + .map(str::to_string) + }) + .ok_or_else(|| anyhow::anyhow!("plugin entry has no local source: {plugin_name}"))?; + let relative_plugin_path = normalize_relative_path(&source_path)?; + let plugin_root = root.join(&relative_plugin_path); + ensure!( + plugin_root.is_dir(), + "plugin source is not a directory: {}", + plugin_root.display() + ); + + let plugin_manifest_path = plugin_root.join(".claude-plugin/plugin.json"); + let plugin_manifest = if plugin_manifest_path.is_file() { + let content = fs::read_to_string(&plugin_manifest_path).with_context(|| { + format!( + "failed to read plugin manifest: {}", + plugin_manifest_path.display() + ) + })?; + Some(serde_json::from_str::(&content).with_context(|| { + format!( + "failed to parse plugin manifest: {}", + plugin_manifest_path.display() + ) + })?) + } else { + None + }; + + let mut unsupported_components = BTreeSet::new(); + for component in ["agents", "commands", "hooks", "lsp", "apps"] { + if plugin_root.join(component).exists() { + unsupported_components.insert(component.to_string()); + } + if plugin_manifest + .as_ref() + .is_some_and(|manifest| manifest.get(component).is_some()) + { + unsupported_components.insert(format!("plugin.json:{component}")); + } + } + if plugin_manifest + .as_ref() + .is_some_and(|manifest| manifest.get("mcpServers").is_some()) + { + unsupported_components.insert("plugin.json:mcpServers".to_string()); + } + + let skills = discover_skills(&plugin_root)?; + let mcp_servers = read_plugin_mcp(&plugin_root)?; + let version = plugin_manifest + .as_ref() + .and_then(|manifest| manifest.get("version")) + .and_then(Value::as_str) + .map(str::to_string) + .or_else(|| { + entry + .get("version") + .and_then(Value::as_str) + .map(str::to_string) + }); + + Ok(DiscoveredPlugin { + version, + plugin_path: relative_plugin_path, + marketplace_manifest: manifest_path, + content_sha256: hash_tree(&plugin_root)?, + skills, + mcp_servers, + unsupported_components: unsupported_components.into_iter().collect(), + }) +} + +fn discover_skills(plugin_root: &Path) -> Result> { + let skills_root = plugin_root.join("skills"); + if !skills_root.exists() { + return Ok(Vec::new()); + } + ensure!( + skills_root.is_dir(), + "plugin skills path is not a directory: {}", + skills_root.display() + ); + let mut skills = Vec::new(); + for entry in fs::read_dir(&skills_root) + .with_context(|| format!("failed to read plugin skills: {}", skills_root.display()))? + { + let entry = entry?; + let path = entry.path(); + let metadata = fs::symlink_metadata(&path)?; + ensure!( + !metadata.file_type().is_symlink(), + "plugin skill symlink is not allowed: {}", + path.display() + ); + if !metadata.is_dir() { + continue; + } + let id = entry.file_name().to_string_lossy().into_owned(); + validate_identifier("skill", &id)?; + ensure!( + path.join("SKILL.md").is_file(), + "plugin skill is missing SKILL.md: {}", + path.display() + ); + skills.push(DiscoveredSkill { + id: id.clone(), + path, + relative_path: format!("skills/{id}"), + content_sha256: hash_tree(&skills_root.join(&id))?, + }); + } + skills.sort_by(|left, right| left.id.cmp(&right.id)); + Ok(skills) +} + +fn read_plugin_mcp(plugin_root: &Path) -> Result> { + let path = plugin_root.join(".mcp.json"); + if !path.exists() { + return Ok(BTreeMap::new()); + } + let content = fs::read_to_string(&path) + .with_context(|| format!("failed to read plugin MCP declaration: {}", path.display()))?; + let value: Value = serde_json::from_str(&content) + .with_context(|| format!("failed to parse plugin MCP declaration: {}", path.display()))?; + let servers = value + .get("mcpServers") + .and_then(Value::as_object) + .ok_or_else(|| anyhow::anyhow!("plugin .mcp.json must contain an mcpServers object"))?; + let mut result = BTreeMap::new(); + for (name, value) in servers { + validate_identifier("MCP server", name)?; + let server: McpServerConfig = serde_json::from_value(value.clone()) + .with_context(|| format!("invalid plugin MCP server: {name}"))?; + ensure!( + result.insert(name.clone(), server).is_none(), + "duplicate plugin MCP server: {name}" + ); + } + Ok(result) +} + +fn parse_plugin_source(value: &Value) -> Result> { + if let Some(source) = value.as_str() { + return Ok(Some(source.to_string())); + } + if let Some(object) = value.as_object() { + for key in ["source", "path"] { + if let Some(source) = object.get(key).and_then(Value::as_str) { + return Ok(Some(source.to_string())); + } + } + } + Ok(None) +} + +fn materialize_skill( + project_root: &Path, + locked: &LockedPlugin, + skill: &DiscoveredSkill, + result: &mut PluginApplyResult, +) -> Result<()> { + let target_root = project_root.join(".agents/skills"); + fs::create_dir_all(&target_root) + .with_context(|| format!("failed to create skill root: {}", target_root.display()))?; + let target_root_metadata = fs::symlink_metadata(&target_root)?; + ensure!( + target_root_metadata.is_dir() && !target_root_metadata.file_type().is_symlink(), + "refusing to materialize skills through a symlinked root: {}", + target_root.display() + ); + let target = target_root.join(&skill.id); + let target_metadata = fs::symlink_metadata(&target).ok(); + if let Some(metadata) = &target_metadata { + ensure!( + !metadata.file_type().is_symlink(), + "refusing to replace symlinked skill: {}", + target.display() + ); + ensure!( + metadata.is_dir(), + "skill destination is not a directory: {}", + target.display() + ); + } + let target_is_managed = if target_metadata.is_some() { + let current_hash = hash_tree(&target)?; + if current_hash == skill.content_sha256 { + register_deduplicated_plugin_owner( + &target_root.join("registry.json"), + &skill.id, + locked, + &skill.content_sha256, + )?; + result.skipped += 1; + return Ok(()); + } + let registry_path = target_root.join("registry.json"); + let owned = crate::skills::registry::read_registry(®istry_path) + .ok() + .and_then(|registry| registry.skills) + .and_then(|skills| skills.get(&skill.id).cloned()) + .is_some_and(|entry| entry_is_owned_by(&entry, locked)); + ensure!( + owned, + "skill collision with unmanaged content: {}", + target.display() + ); + true + } else { + false + }; + + let staging = TempDir::new_in(&target_root).with_context(|| { + format!( + "failed to create temporary skill directory in {}", + target_root.display() + ) + })?; + let staged_target = staging.path().join(&skill.id); + copy_directory_without_symlinks(&skill.path, &staged_target)?; + let manifest = crate::skills::manifest::parse_skill_manifest(&staged_target.join("SKILL.md")) + .map_err(|error| anyhow::anyhow!(error.to_string()))?; + if target_is_managed { + remove_path_safely(&target)?; + result.updated += 1; + } else { + result.created += 1; + } + fs::rename(&staged_target, &target).with_context(|| { + format!( + "failed to atomically install skill {} at {}", + skill.id, + target.display() + ) + })?; + let registry_path = target_root.join("registry.json"); + let entry = crate::skills::registry::SkillEntry { + name: Some(manifest.name), + description: manifest.description, + version: manifest.version, + provider: Some(format!("plugin/{}/{}", locked.marketplace, locked.plugin)), + source: Some(locked.source.location.clone()), + installed_at: Some(chrono::Utc::now().to_rfc3339()), + files: None, + manifest_hash: Some(skill.content_sha256.clone()), + marketplace: Some(locked.marketplace.clone()), + plugin: Some(locked.plugin.clone()), + plugin_revision: Some(locked.source.revision.clone()), + content_sha256: Some(skill.content_sha256.clone()), + plugin_owners: Some(vec![crate::skills::registry::PluginOwner { + marketplace: locked.marketplace.clone(), + plugin: locked.plugin.clone(), + revision: locked.source.revision.clone(), + }]), + }; + crate::skills::registry::update_registry_entry(®istry_path, &skill.id, entry)?; + Ok(()) +} + +fn entry_plugin_owners( + entry: &crate::skills::registry::SkillEntry, +) -> Vec { + if let Some(owners) = &entry.plugin_owners + && !owners.is_empty() + { + return owners.clone(); + } + match (&entry.marketplace, &entry.plugin, &entry.plugin_revision) { + (Some(marketplace), Some(plugin), Some(revision)) => { + vec![crate::skills::registry::PluginOwner { + marketplace: marketplace.clone(), + plugin: plugin.clone(), + revision: revision.clone(), + }] + } + _ => Vec::new(), + } +} + +fn entry_is_owned_by(entry: &crate::skills::registry::SkillEntry, locked: &LockedPlugin) -> bool { + let owner = crate::skills::registry::PluginOwner { + marketplace: locked.marketplace.clone(), + plugin: locked.plugin.clone(), + revision: locked.source.revision.clone(), + }; + entry_plugin_owners(entry).contains(&owner) +} + +fn register_deduplicated_plugin_owner( + registry_path: &Path, + skill_id: &str, + locked: &LockedPlugin, + content_sha256: &str, +) -> Result<()> { + let Ok(mut registry) = crate::skills::registry::read_registry(registry_path) else { + return Ok(()); + }; + let Some(skills) = registry.skills.as_mut() else { + return Ok(()); + }; + let Some(entry) = skills.get_mut(skill_id) else { + return Ok(()); + }; + let content_matches = entry.content_sha256.as_deref() == Some(content_sha256) + || entry.manifest_hash.as_deref() == Some(content_sha256); + if !content_matches { + return Ok(()); + } + let owner = crate::skills::registry::PluginOwner { + marketplace: locked.marketplace.clone(), + plugin: locked.plugin.clone(), + revision: locked.source.revision.clone(), + }; + let mut owners = entry_plugin_owners(entry); + if owners.contains(&owner) { + return Ok(()); + } + owners.push(owner); + owners.sort_by(|left, right| { + (&left.marketplace, &left.plugin, &left.revision).cmp(&( + &right.marketplace, + &right.plugin, + &right.revision, + )) + }); + entry.plugin_owners = Some(owners); + crate::skills::registry::update_registry_entry(registry_path, skill_id, entry.clone())?; + Ok(()) +} + +fn copy_directory_without_symlinks(source: &Path, target: &Path) -> Result<()> { + fs::create_dir_all(target).with_context(|| format!("failed to create {}", target.display()))?; + for entry in + fs::read_dir(source).with_context(|| format!("failed to read {}", source.display()))? + { + let entry = entry?; + let source_path = entry.path(); + let target_path = target.join(entry.file_name()); + let metadata = fs::symlink_metadata(&source_path)?; + ensure!( + !metadata.file_type().is_symlink(), + "plugin symlink is not allowed: {}", + source_path.display() + ); + if metadata.is_dir() { + copy_directory_without_symlinks(&source_path, &target_path)?; + } else if metadata.is_file() { + fs::copy(&source_path, &target_path).with_context(|| { + format!( + "failed to copy {} to {}", + source_path.display(), + target_path.display() + ) + })?; + } + } + Ok(()) +} + +fn remove_path_safely(path: &Path) -> Result<()> { + let metadata = fs::symlink_metadata(path)?; + if metadata.file_type().is_symlink() || metadata.is_file() { + fs::remove_file(path)?; + } else if metadata.is_dir() { + fs::remove_dir_all(path)?; + } + Ok(()) +} + +fn rollback_plugin_lock(path: &Path, previous: Option<&PluginLock>) -> Result<()> { + if let Some(previous) = previous { + previous.save_atomic(path) + } else if path.exists() { + fs::remove_file(path) + .with_context(|| format!("failed to roll back plugin lockfile: {}", path.display())) + } else { + Ok(()) + } +} + +fn add_selection_to_config(config_path: &Path, selection: &PluginSelection) -> Result<()> { + let content = fs::read_to_string(config_path).with_context(|| { + format!( + "failed to read config for plugin selection: {}", + config_path.display() + ) + })?; + let document: toml::Value = toml::from_str(&content) + .with_context(|| format!("failed to parse plugin config: {}", config_path.display()))?; + let already_selected = document + .get("plugins") + .and_then(|plugins| plugins.get("selections")) + .and_then(toml::Value::as_array) + .is_some_and(|selections| { + selections.iter().any(|value| { + value.get("marketplace").and_then(toml::Value::as_str) + == Some(selection.marketplace.as_str()) + && value.get("plugin").and_then(toml::Value::as_str) + == Some(selection.plugin.as_str()) + }) + }); + if already_selected { + return Ok(()); + } + let separator = if content.ends_with('\n') { + "\n" + } else { + "\n\n" + }; + let body = format!( + "{}{separator}[[plugins.selections]]\nmarketplace = {:?}\nplugin = {:?}\n", + content, selection.marketplace, selection.plugin + ); + write_atomic_file(config_path, body.as_bytes()) +} + +fn remove_plugin_owner_entries_atomic( + registry_path: &Path, + skill_ids: &[&str], + owner: &crate::skills::registry::PluginOwner, +) -> Result<()> { + if !registry_path.is_file() || skill_ids.is_empty() { + return Ok(()); + } + let mut registry = crate::skills::registry::read_registry(registry_path)?; + let Some(skills) = registry.skills.as_mut() else { + return Ok(()); + }; + let mut changed = false; + for skill_id in skill_ids { + let remove_entry = { + let Some(entry) = skills.get_mut(*skill_id) else { + continue; + }; + let mut owners = entry_plugin_owners(entry); + if !owners.contains(owner) { + continue; + } + owners.retain(|candidate| candidate != owner); + changed = true; + if owners.is_empty() { + true + } else { + owners.sort_by(|left, right| { + (&left.marketplace, &left.plugin, &left.revision).cmp(&( + &right.marketplace, + &right.plugin, + &right.revision, + )) + }); + let primary = owners.first().expect("owners is non-empty").clone(); + entry.plugin_owners = Some(owners); + entry.marketplace = Some(primary.marketplace); + entry.plugin = Some(primary.plugin); + entry.plugin_revision = Some(primary.revision); + false + } + }; + if remove_entry { + skills.remove(*skill_id); + } + } + if !changed { + return Ok(()); + } + registry.last_updated = Some(chrono::Utc::now().to_rfc3339()); + let body = + serde_json::to_vec_pretty(®istry).context("failed to serialize skill registry")?; + write_atomic_file(registry_path, &body) +} + +fn remove_selection_from_config(config_path: &Path, selection: &PluginSelection) -> Result<()> { + let content = fs::read_to_string(config_path).with_context(|| { + format!( + "failed to read config for plugin removal: {}", + config_path.display() + ) + })?; + let lines: Vec<&str> = content.lines().collect(); + let mut output = Vec::with_capacity(lines.len()); + let mut index = 0; + let mut removed = false; + + while index < lines.len() { + if lines[index].trim() != "[[plugins.selections]]" { + output.push(lines[index]); + index += 1; + continue; + } + + let start = index; + index += 1; + while index < lines.len() + && !lines[index] + .trim_start() + .starts_with("[[plugins.selections]]") + && !lines[index].trim_start().starts_with('[') + { + index += 1; + } + let block = lines[start..index].join("\n"); + let value: toml::Value = toml::from_str(&block).with_context(|| { + format!( + "invalid plugin selection block in {}", + config_path.display() + ) + })?; + let matches = value + .get("plugins") + .and_then(|plugins| plugins.get("selections")) + .and_then(toml::Value::as_array) + .and_then(|selections| selections.first()) + .is_some_and(|selection_value| { + selection_value + .get("marketplace") + .and_then(toml::Value::as_str) + == Some(selection.marketplace.as_str()) + && selection_value.get("plugin").and_then(toml::Value::as_str) + == Some(selection.plugin.as_str()) + }); + if matches { + removed = true; + } else { + output.extend(lines[start..index].iter().copied()); + } + } + + if !removed { + return Ok(()); + } + let body = format!("{}\n", output.join("\n")); + write_atomic_file(config_path, body.as_bytes()) +} + +fn write_atomic_file(path: &Path, body: &[u8]) -> Result<()> { + let parent = path.parent().unwrap_or_else(|| Path::new(".")); + let temporary = NamedTempFile::new_in(parent) + .with_context(|| format!("failed to create temporary file in {}", parent.display()))?; + fs::write(temporary.path(), body).with_context(|| { + format!( + "failed to write temporary file: {}", + temporary.path().display() + ) + })?; + temporary + .persist(path) + .map_err(|error| error.error) + .with_context(|| format!("failed to replace file: {}", path.display()))?; + Ok(()) +} + +fn validate_selection(selection: &PluginSelection) -> Result<()> { + validate_identifier("marketplace", &selection.marketplace)?; + validate_identifier("plugin", &selection.plugin) +} + +fn validate_source(source: &LockedSource) -> Result<()> { + match source.kind { + LockedSourceKind::Local => { + ensure!( + !Path::new(&source.location).is_absolute(), + "locked local source must be relative" + ); + ensure!( + !source.location.contains("://") && !source.location.contains(':'), + "locked local source must be a path: {}", + source.location + ); + ensure!( + source.revision.starts_with("local:"), + "invalid local plugin revision: {}", + source.revision + ); + validate_hash( + "local revision", + source.revision.trim_start_matches("local:"), + )?; + } + LockedSourceKind::Git => { + ensure!( + source.location.starts_with("https://"), + "Git plugin source must use HTTPS" + ); + validate_commit(&source.revision)?; + } + } + Ok(()) +} + +fn validate_identifier(kind: &str, value: &str) -> Result<()> { + ensure!(!value.is_empty(), "{kind} must not be empty"); + ensure!( + value + .chars() + .all(|character| character.is_ascii_alphanumeric() + || matches!(character, '-' | '_' | '.')), + "invalid {kind}: {value}" + ); + ensure!(value != "." && value != "..", "invalid {kind}: {value}"); + Ok(()) +} + +fn validate_relative_path(path: &str) -> Result<()> { + ensure!(!path.is_empty(), "path must not be empty"); + ensure!( + !Path::new(path).is_absolute(), + "absolute paths are not allowed: {path}" + ); + ensure!( + !path.contains(':'), + "drive-prefixed paths are not allowed: {path}" + ); + for component in Path::new(path).components() { + ensure!( + !matches!(component, Component::ParentDir), + "path traversal is not allowed: {path}" + ); + } + Ok(()) +} + +fn normalize_relative_path(path: &str) -> Result { + let normalized = path.strip_prefix("./").unwrap_or(path); + validate_relative_path(normalized)?; + Ok(normalized.to_string()) +} + +fn validate_hash(field: &str, value: &str) -> Result<()> { + ensure!( + value.len() == 64 && value.bytes().all(|byte| byte.is_ascii_hexdigit()), + "{field} must be a SHA-256 hash" + ); + Ok(()) +} + +fn validate_commit(value: &str) -> Result<()> { + ensure!( + value.len() == 40 && value.bytes().all(|byte| byte.is_ascii_hexdigit()), + "Git revision must be a full 40-character commit SHA" + ); + Ok(()) +} + +fn is_local_source(source: &str) -> bool { + source.starts_with('.') || source.starts_with('/') || !source.contains("://") +} + +fn hash_tree(root: &Path) -> Result { + ensure!(root.is_dir(), "expected directory: {}", root.display()); + let mut entries = Vec::new(); + for entry in WalkDir::new(root).follow_links(false) { + let entry = entry?; + if entry.path() == root { + continue; + } + let metadata = fs::symlink_metadata(entry.path())?; + ensure!( + !metadata.file_type().is_symlink(), + "symlinks are not allowed in plugin sources: {}", + entry.path().display() + ); + if metadata.is_file() { + let relative = entry + .path() + .strip_prefix(root)? + .to_string_lossy() + .replace('\\', "/"); + let bytes = fs::read(entry.path())?; + entries.push((relative, bytes)); + } + } + entries.sort_by(|left, right| left.0.cmp(&right.0)); + let mut hasher = Sha256::new(); + for (path, bytes) in entries { + hasher.update(path.as_bytes()); + hasher.update([0]); + hasher.update((bytes.len() as u64).to_be_bytes()); + hasher.update(bytes); + } + Ok(format_digest(hasher.finalize())) +} + +fn format_digest(digest: impl AsRef<[u8]>) -> String { + digest + .as_ref() + .iter() + .map(|byte| format!("{byte:02x}")) + .collect() +} + +fn resolve_git_reference(repository: &str, reference: &str) -> Result { + ensure!( + !reference.trim().eq_ignore_ascii_case("HEAD"), + "Git reference HEAD is not allowed; use a branch, tag, or full commit SHA" + ); + if reference.len() == 40 && reference.bytes().all(|byte| byte.is_ascii_hexdigit()) { + return Ok(reference.to_ascii_lowercase()); + } + let github = github_repo_parts(repository)?; + let endpoint = format!( + "https://api.github.com/repos/{}/{}/commits/{}", + github.0, + github.1, + urlencoding::encode(reference) + ); + let client = reqwest::blocking::Client::builder() + .user_agent("agentsync-plugin-resolver") + .build() + .context("failed to create GitHub API client")?; + let mut request = client.get(endpoint); + if let Ok(token) = std::env::var("GITHUB_TOKEN") { + request = request.bearer_auth(token); + } + let response: Value = request + .send() + .context("failed to resolve Git reference")? + .error_for_status() + .context("Git reference resolution failed")? + .json() + .context("invalid GitHub commit response")?; + let sha = response + .get("sha") + .and_then(Value::as_str) + .ok_or_else(|| anyhow::anyhow!("GitHub commit response did not contain a SHA"))?; + validate_commit(sha)?; + Ok(sha.to_ascii_lowercase()) +} + +fn github_archive_url(repository: &str, commit: &str) -> Result { + let (owner, repo) = github_repo_parts(repository)?; + validate_commit(commit)?; + Ok(format!( + "https://github.com/{owner}/{repo}/archive/{commit}.zip" + )) +} + +fn github_repo_parts(repository: &str) -> Result<(String, String)> { + let parsed = url::Url::parse(repository).context("invalid Git repository URL")?; + ensure!(parsed.scheme() == "https", "Git repository must use HTTPS"); + ensure!( + parsed.host_str() == Some("github.com"), + "only GitHub repositories are supported in this MVP" + ); + let segments = parsed + .path_segments() + .map(|segments| segments.collect::>()) + .unwrap_or_default(); + ensure!( + segments.len() == 2, + "GitHub repository URL must be https://github.com//" + ); + let repo = segments[1].trim_end_matches(".git"); + validate_identifier("GitHub owner", segments[0])?; + validate_identifier("GitHub repository", repo)?; + Ok((segments[0].to_string(), repo.to_string())) +} + +fn blocking_fetch_archive(url: &str) -> Result { + let future = crate::skills::install::fetch_and_unpack_to_tempdir(url); + let result = match tokio::runtime::Handle::try_current() { + Ok(handle) => tokio::task::block_in_place(|| handle.block_on(future)), + Err(_) => tokio::runtime::Runtime::new()?.block_on(future), + }?; + Ok(result) +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + #[test] + fn lockfile_round_trip_is_deterministic() { + let mut lock = PluginLock::default(); + let plugin = sample_locked_plugin("internal", "engineering"); + lock.plugins.insert(plugin.key(), plugin); + let temp = TempDir::new().unwrap(); + let path = temp.path().join("plugins.lock.toml"); + lock.save_atomic(&path).unwrap(); + let first = fs::read_to_string(&path).unwrap(); + let loaded = PluginLock::load(&path).unwrap(); + loaded.save_atomic(&path).unwrap(); + assert_eq!(first, fs::read_to_string(path).unwrap()); + } + + #[test] + fn mutable_git_reference_resolves_to_a_commit_shape() { + assert_eq!( + resolve_git_reference( + "https://github.com/example/repo", + "0123456789abcdef0123456789abcdef01234567" + ) + .unwrap(), + "0123456789abcdef0123456789abcdef01234567" + ); + assert!(resolve_git_reference("https://gitlab.com/example/repo", "main").is_err()); + assert!(resolve_git_reference("https://github.com/example/repo", "HEAD").is_err()); + } + + #[test] + fn apply_requires_a_local_snapshot_for_locked_git_sources() { + let temp = TempDir::new().unwrap(); + let agents = temp.path().join(".agents"); + fs::create_dir_all(&agents).unwrap(); + let config_path = agents.join("agentsync.toml"); + let selection = PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }; + let config = PluginsConfig { + enabled: true, + lockfile: "plugins.lock.toml".to_string(), + marketplaces: BTreeMap::new(), + selections: vec![selection.clone()], + }; + let manager = PluginManager::new(temp.path().to_path_buf(), config_path, config); + let mut plugin = sample_locked_plugin(&selection.marketplace, &selection.plugin); + plugin.source.kind = LockedSourceKind::Git; + plugin.source.location = "https://github.com/example/repo".to_string(); + plugin.source.revision = "0123456789abcdef0123456789abcdef01234567".to_string(); + plugin.provenance.resolved_revision = plugin.source.revision.clone(); + let mut lock = PluginLock::default(); + lock.plugins.insert(plugin.key(), plugin); + lock.save_atomic(&manager.lock_path().unwrap()).unwrap(); + + let error = manager + .apply(true) + .expect_err("apply must not fetch a Git source"); + assert!( + error + .to_string() + .contains("offline Git plugin source snapshot") + ); + } + + #[test] + fn path_validation_rejects_traversal_and_absolute_paths() { + assert!(validate_relative_path("../outside").is_err()); + assert!(validate_relative_path("/outside").is_err()); + assert!(normalize_relative_path("./skills/demo").is_ok()); + } + + fn sample_locked_plugin(marketplace: &str, plugin: &str) -> LockedPlugin { + let revision = "local:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + let hash = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + LockedPlugin { + marketplace: marketplace.to_string(), + plugin: plugin.to_string(), + version: Some("1.0.0".to_string()), + source: LockedSource { + kind: LockedSourceKind::Local, + location: "../marketplace".to_string(), + revision: revision.to_string(), + }, + content_sha256: hash.to_string(), + skills: vec![LockedSkill { + id: "demo".to_string(), + path: "skills/demo".to_string(), + content_sha256: hash.to_string(), + }], + mcp_servers: vec!["filesystem".to_string()], + unsupported_components: Vec::new(), + provenance: PluginProvenance { + marketplace_manifest: ".claude-plugin/marketplace.json".to_string(), + plugin_path: "plugins/engineering".to_string(), + resolved_revision: revision.to_string(), + content_sha256: hash.to_string(), + }, + } + } +} diff --git a/src/skills/install.rs b/src/skills/install.rs index f4123ff1..b934ed76 100644 --- a/src/skills/install.rs +++ b/src/skills/install.rs @@ -146,6 +146,11 @@ fn install_staged( installed_at: Some(chrono::Utc::now().to_rfc3339()), files: None, manifest_hash: None, + marketplace: None, + plugin: None, + plugin_revision: None, + content_sha256: None, + plugin_owners: None, }; let registry_path = target_root.join("registry.json"); crate::skills::registry::update_registry_entry(®istry_path, skill_id, entry) diff --git a/src/skills/registry.rs b/src/skills/registry.rs index 6195c9b8..0eacfe57 100644 --- a/src/skills/registry.rs +++ b/src/skills/registry.rs @@ -282,6 +282,24 @@ pub struct SkillEntry { pub files: Option>, #[serde(rename = "manifestHash")] pub manifest_hash: Option, + #[serde(default)] + pub marketplace: Option, + #[serde(default)] + pub plugin: Option, + #[serde(rename = "pluginRevision", default)] + pub plugin_revision: Option, + #[serde(rename = "contentSha256", default)] + pub content_sha256: Option, + /// All repository-owned plugin selections that currently deduplicate this skill. + #[serde(rename = "pluginOwners", default)] + pub plugin_owners: Option>, +} + +#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)] +pub struct PluginOwner { + pub marketplace: String, + pub plugin: String, + pub revision: String, } #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/src/skills/update.rs b/src/skills/update.rs index eb493afe..df910e19 100644 --- a/src/skills/update.rs +++ b/src/skills/update.rs @@ -189,6 +189,11 @@ fn install_updated_skill( installed_at: Some(chrono::Utc::now().to_rfc3339()), files: None, manifest_hash: None, + marketplace: None, + plugin: None, + plugin_revision: None, + content_sha256: None, + plugin_owners: None, }; if let Err(e) = diff --git a/tests/fixtures/plugin-marketplace/.agents/plugins/marketplace.json b/tests/fixtures/plugin-marketplace/.agents/plugins/marketplace.json new file mode 100644 index 00000000..1d660eec --- /dev/null +++ b/tests/fixtures/plugin-marketplace/.agents/plugins/marketplace.json @@ -0,0 +1,15 @@ +{ + "name": "internal", + "plugins": [ + { + "name": "engineering", + "source": "./plugins/engineering", + "version": "1.2.3" + }, + { + "name": "unsafe", + "source": "./plugins/unsafe", + "version": "1.0.0" + } + ] +} diff --git a/tests/fixtures/plugin-marketplace/plugins/engineering/.claude-plugin/plugin.json b/tests/fixtures/plugin-marketplace/plugins/engineering/.claude-plugin/plugin.json new file mode 100644 index 00000000..82e9a8c8 --- /dev/null +++ b/tests/fixtures/plugin-marketplace/plugins/engineering/.claude-plugin/plugin.json @@ -0,0 +1,5 @@ +{ + "name": "engineering", + "version": "1.2.3", + "description": "Engineering workflow skills" +} diff --git a/tests/fixtures/plugin-marketplace/plugins/engineering/skills/review/SKILL.md b/tests/fixtures/plugin-marketplace/plugins/engineering/skills/review/SKILL.md new file mode 100644 index 00000000..99ef7ef9 --- /dev/null +++ b/tests/fixtures/plugin-marketplace/plugins/engineering/skills/review/SKILL.md @@ -0,0 +1,9 @@ +--- +name: review +version: 1.0.0 +description: Review code changes using the repository engineering standards. +--- + +# Review + +Use the repository standards when reviewing changes. diff --git a/tests/fixtures/plugin-marketplace/plugins/engineering/skills/review/references/REFERENCE.md b/tests/fixtures/plugin-marketplace/plugins/engineering/skills/review/references/REFERENCE.md new file mode 100644 index 00000000..aa812c86 --- /dev/null +++ b/tests/fixtures/plugin-marketplace/plugins/engineering/skills/review/references/REFERENCE.md @@ -0,0 +1,3 @@ +# Reference + +This reference is copied with the skill and is never executed by AgentSync. diff --git a/tests/fixtures/plugin-marketplace/plugins/unsafe/.claude-plugin/plugin.json b/tests/fixtures/plugin-marketplace/plugins/unsafe/.claude-plugin/plugin.json new file mode 100644 index 00000000..7a23bc4d --- /dev/null +++ b/tests/fixtures/plugin-marketplace/plugins/unsafe/.claude-plugin/plugin.json @@ -0,0 +1,7 @@ +{ + "name": "unsafe", + "version": "1.0.0", + "hooks": { + "PostInstall": "./hooks/install.sh" + } +} diff --git a/tests/fixtures/plugin-marketplace/plugins/unsafe/hooks/install.sh b/tests/fixtures/plugin-marketplace/plugins/unsafe/hooks/install.sh new file mode 100644 index 00000000..dead1194 --- /dev/null +++ b/tests/fixtures/plugin-marketplace/plugins/unsafe/hooks/install.sh @@ -0,0 +1,2 @@ +#!/bin/sh +printf 'this hook must never run\n' > ../hook-ran.txt diff --git a/tests/plugins.rs b/tests/plugins.rs new file mode 100644 index 00000000..ed4a3351 --- /dev/null +++ b/tests/plugins.rs @@ -0,0 +1,270 @@ +use agentsync::config::Config; +use agentsync::plugins::{PluginManager, PluginSelection}; +use std::fs; +use std::path::{Path, PathBuf}; +use tempfile::TempDir; + +fn fixture_root() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/plugin-marketplace") +} + +fn copy_tree(source: &Path, target: &Path) { + fs::create_dir_all(target).unwrap(); + for entry in fs::read_dir(source).unwrap() { + let entry = entry.unwrap(); + let source_path = entry.path(); + let target_path = target.join(entry.file_name()); + if source_path.is_dir() { + copy_tree(&source_path, &target_path); + } else { + fs::create_dir_all(target_path.parent().unwrap()).unwrap(); + fs::copy(source_path, target_path).unwrap(); + } + } +} + +fn setup_project() -> (TempDir, Config) { + let project = TempDir::new().unwrap(); + let marketplace = project.path().join("marketplace"); + copy_tree(&fixture_root(), &marketplace); + let agents = project.path().join(".agents"); + fs::create_dir_all(&agents).unwrap(); + fs::write( + agents.join("agentsync.toml"), + r#" +[plugins] +enabled = true +lockfile = "plugins.lock.toml" + +[plugins.marketplaces.internal] +source = "../marketplace" +reference = "main" + +[[plugins.selections]] +marketplace = "internal" +plugin = "engineering" +"#, + ) + .unwrap(); + let config_path = agents.join("agentsync.toml"); + (project, Config::load(&config_path).unwrap()) +} + +#[test] +fn plugin_add_writes_lock_materializes_skill_and_returns_mcp_without_execution() { + let (project, config) = setup_project(); + let config_path = project.path().join(".agents/agentsync.toml"); + let manager = PluginManager::new(project.path().to_path_buf(), config_path, config.plugins); + let selection = PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }; + + let result = manager.add(&selection).unwrap(); + + assert_eq!(result.mcp_servers.len(), 1); + assert!( + result + .mcp_servers + .contains_key("plugin/internal/engineering/safe-fixture") + ); + assert!( + project + .path() + .join(".agents/skills/review/SKILL.md") + .is_file() + ); + assert!( + project + .path() + .join(".agents/skills/review/references/REFERENCE.md") + .is_file() + ); + assert!(project.path().join(".agents/plugins.lock.toml").is_file()); + assert!( + !project + .path() + .join("marketplace/plugins/engineering/hook-ran.txt") + .exists() + ); + + let second = manager.apply(false).unwrap(); + assert_eq!(second.created, 0); + assert_eq!(second.updated, 0); + assert_eq!(second.skipped, 1); + + let registry = agentsync::skills::registry::read_registry( + &project.path().join(".agents/skills/registry.json"), + ) + .unwrap(); + let entry = registry.skills.unwrap().remove("review").unwrap(); + assert_eq!(entry.marketplace.as_deref(), Some("internal")); + assert_eq!(entry.plugin.as_deref(), Some("engineering")); + assert!(entry.plugin_revision.is_some()); +} + +#[test] +fn plugin_add_registers_a_missing_selection_in_project_config() { + let (project, _) = setup_project(); + let config_path = project.path().join(".agents/agentsync.toml"); + let mut config_body = fs::read_to_string(&config_path).unwrap(); + config_body = config_body.replace( + "\n[[plugins.selections]]\nmarketplace = \"internal\"\nplugin = \"engineering\"\n", + "\n", + ); + fs::write(&config_path, config_body).unwrap(); + let config = Config::load(&config_path).unwrap(); + assert!(config.plugins.selections.is_empty()); + let manager = PluginManager::new( + project.path().to_path_buf(), + config_path.clone(), + config.plugins, + ); + + manager + .add(&PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }) + .unwrap(); + + let updated = fs::read_to_string(config_path).unwrap(); + assert!(updated.contains("[[plugins.selections]]")); + assert!(updated.contains("marketplace = \"internal\"")); + assert!(updated.contains("plugin = \"engineering\"")); +} + +#[test] +fn identical_skills_are_deduplicated_and_removed_only_after_last_owner() { + let (project, config) = setup_project(); + let manifest_path = project + .path() + .join("marketplace/.agents/plugins/marketplace.json"); + let mut manifest: serde_json::Value = + serde_json::from_str(&fs::read_to_string(&manifest_path).unwrap()).unwrap(); + manifest["plugins"] + .as_array_mut() + .unwrap() + .push(serde_json::json!({ + "name": "engineering-copy", + "source": "./plugins/engineering", + "version": "1.2.3" + })); + fs::write( + &manifest_path, + serde_json::to_vec_pretty(&manifest).unwrap(), + ) + .unwrap(); + + let config_path = project.path().join(".agents/agentsync.toml"); + let manager = PluginManager::new(project.path().to_path_buf(), config_path, config.plugins); + let engineering = PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }; + let copy = PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering-copy".to_string(), + }; + manager.add(&engineering).unwrap(); + let copy_result = manager.add(©).unwrap(); + assert_eq!(copy_result.created, 0); + assert_eq!(copy_result.updated, 0); + assert_eq!(copy_result.skipped, 2); + + let registry_path = project.path().join(".agents/skills/registry.json"); + let registry = agentsync::skills::registry::read_registry(®istry_path).unwrap(); + let owners = registry + .skills + .unwrap() + .remove("review") + .unwrap() + .plugin_owners + .unwrap(); + assert_eq!(owners.len(), 2); + + manager.remove(©, false).unwrap(); + assert!( + project + .path() + .join(".agents/skills/review/SKILL.md") + .is_file() + ); + manager.remove(&engineering, false).unwrap(); + assert!(!project.path().join(".agents/skills/review").exists()); +} + +#[test] +fn plugin_apply_detects_local_source_drift_without_rewriting_content() { + let (project, config) = setup_project(); + let config_path = project.path().join(".agents/agentsync.toml"); + let manager = PluginManager::new(project.path().to_path_buf(), config_path, config.plugins); + let selection = PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }; + manager.add(&selection).unwrap(); + let installed = project.path().join(".agents/skills/review/SKILL.md"); + let before = fs::read_to_string(&installed).unwrap(); + + fs::write( + project + .path() + .join("marketplace/plugins/engineering/skills/review/SKILL.md"), + "changed source", + ) + .unwrap(); + + let error = manager.apply(false).expect_err("drift must fail closed"); + assert!(error.to_string().contains("drift")); + assert_eq!(before, fs::read_to_string(installed).unwrap()); +} + +#[test] +fn plugin_add_rejects_unmanaged_skill_collision_and_rolls_back_lock() { + let (project, config) = setup_project(); + let config_path = project.path().join(".agents/agentsync.toml"); + let target = project.path().join(".agents/skills/review"); + fs::create_dir_all(&target).unwrap(); + fs::write(target.join("SKILL.md"), "unmanaged content").unwrap(); + let manager = PluginManager::new(project.path().to_path_buf(), config_path, config.plugins); + + let error = manager + .add(&PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }) + .expect_err("unmanaged skill collision must fail"); + assert!(error.to_string().contains("collision")); + assert_eq!( + fs::read_to_string(target.join("SKILL.md")).unwrap(), + "unmanaged content" + ); + assert!(!project.path().join(".agents/plugins.lock.toml").exists()); +} + +#[test] +fn plugin_with_unsupported_hooks_is_rejected_and_hook_is_not_run() { + let (project, mut config) = setup_project(); + config.plugins.selections = vec![PluginSelection { + marketplace: "internal".to_string(), + plugin: "unsafe".to_string(), + }]; + let config_path = project.path().join(".agents/agentsync.toml"); + let manager = PluginManager::new(project.path().to_path_buf(), config_path, config.plugins); + let selection = PluginSelection { + marketplace: "internal".to_string(), + plugin: "unsafe".to_string(), + }; + + let error = manager + .add(&selection) + .expect_err("hooks must be unsupported"); + assert!(error.to_string().contains("unsupported")); + assert!( + !project + .path() + .join("marketplace/plugins/unsafe/hook-ran.txt") + .exists() + ); +} diff --git a/tests/plugins_cli.rs b/tests/plugins_cli.rs new file mode 100644 index 00000000..e6ad48a2 --- /dev/null +++ b/tests/plugins_cli.rs @@ -0,0 +1,86 @@ +use std::fs; +use std::path::{Path, PathBuf}; +use std::process::Command; +use tempfile::TempDir; + +fn fixture_root() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/plugin-marketplace") +} + +fn copy_tree(source: &Path, target: &Path) { + fs::create_dir_all(target).unwrap(); + for entry in fs::read_dir(source).unwrap() { + let entry = entry.unwrap(); + let source_path = entry.path(); + let target_path = target.join(entry.file_name()); + if source_path.is_dir() { + copy_tree(&source_path, &target_path); + } else { + fs::create_dir_all(target_path.parent().unwrap()).unwrap(); + fs::copy(source_path, target_path).unwrap(); + } + } +} + +fn setup_project() -> TempDir { + let project = TempDir::new().unwrap(); + copy_tree(&fixture_root(), &project.path().join("marketplace")); + let agents = project.path().join(".agents"); + fs::create_dir_all(&agents).unwrap(); + fs::write( + agents.join("agentsync.toml"), + r#" +[plugins] +enabled = true +lockfile = "plugins.lock.toml" + +[plugins.marketplaces.internal] +source = "../marketplace" +reference = "main" + +[[plugins.selections]] +marketplace = "internal" +plugin = "engineering" +"#, + ) + .unwrap(); + project +} + +fn run_plugin(project: &TempDir, args: &[&str]) -> std::process::Output { + Command::new(env!("CARGO_BIN_EXE_agentsync")) + .args(["plugin", "--project-root", project.path().to_str().unwrap()]) + .args(args) + .output() + .unwrap() +} + +#[test] +fn plugin_cli_add_status_list_and_remove_are_deterministic() { + let project = setup_project(); + + let add = run_plugin(&project, &["add", "internal/engineering"]); + assert!(add.status.success(), "add failed: {:?}", add); + assert!(String::from_utf8_lossy(&add.stdout).contains("added internal/engineering")); + + let list = run_plugin(&project, &["list", "--json"]); + assert!(list.status.success(), "list failed: {:?}", list); + let list_json: serde_json::Value = serde_json::from_slice(&list.stdout).unwrap(); + assert!(list_json["plugins"]["internal/engineering"].is_object()); + + let status = run_plugin(&project, &["status", "--json"]); + assert!(status.status.success(), "status failed: {:?}", status); + let status_json: serde_json::Value = serde_json::from_slice(&status.stdout).unwrap(); + assert_eq!(status_json["status"], "ok"); + + let remove = run_plugin(&project, &["remove", "internal/engineering"]); + assert!(remove.status.success(), "remove failed: {:?}", remove); + assert!(String::from_utf8_lossy(&remove.stdout).contains("removed internal/engineering")); + assert!(!project.path().join(".agents/skills/review").exists()); + + let config = fs::read_to_string(project.path().join(".agents/agentsync.toml")).unwrap(); + assert!(!config.contains("marketplace = \"internal\"")); + assert!(!config.contains("plugin = \"engineering\"")); + let lock = fs::read_to_string(project.path().join(".agents/plugins.lock.toml")).unwrap(); + assert!(!lock.contains("engineering")); +} diff --git a/tests/plugins_mcp.rs b/tests/plugins_mcp.rs new file mode 100644 index 00000000..0a49747a --- /dev/null +++ b/tests/plugins_mcp.rs @@ -0,0 +1,96 @@ +use agentsync::Linker; +use agentsync::config::Config; +use agentsync::plugins::{PluginManager, PluginSelection}; +use std::fs; +use std::path::{Path, PathBuf}; +use tempfile::TempDir; + +fn fixture_root() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/plugin-marketplace") +} + +fn copy_tree(source: &Path, target: &Path) { + fs::create_dir_all(target).unwrap(); + for entry in fs::read_dir(source).unwrap() { + let entry = entry.unwrap(); + let source_path = entry.path(); + let target_path = target.join(entry.file_name()); + if source_path.is_dir() { + copy_tree(&source_path, &target_path); + } else { + fs::create_dir_all(target_path.parent().unwrap()).unwrap(); + fs::copy(source_path, target_path).unwrap(); + } + } +} + +#[test] +fn plugin_mcp_is_fanned_out_to_supported_agents_without_execution() { + let project = TempDir::new().unwrap(); + copy_tree(&fixture_root(), &project.path().join("marketplace")); + let agents = project.path().join(".agents"); + fs::create_dir_all(&agents).unwrap(); + let config_path = agents.join("agentsync.toml"); + fs::write( + &config_path, + r#" +[mcp] +enabled = true + +[agents.claude] +[agents.codex] +[agents.gemini] +[agents.opencode] + +[plugins] +enabled = true +lockfile = "plugins.lock.toml" + +[plugins.marketplaces.internal] +source = "../marketplace" + +[[plugins.selections]] +marketplace = "internal" +plugin = "engineering" +"#, + ) + .unwrap(); + + let config = Config::load(&config_path).unwrap(); + let manager = PluginManager::new( + Config::project_root(&config_path), + config_path.clone(), + config.plugins.clone(), + ); + let plugin_result = manager.add(&PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }); + let plugin_result = plugin_result.unwrap(); + let linker = Linker::new(config, config_path); + let sync_result = linker + .sync_mcp_with_servers(false, None, &plugin_result.mcp_servers) + .unwrap(); + assert_eq!(sync_result.errors, 0); + assert!(sync_result.created + sync_result.updated >= 4); + + let expected_name = "plugin/internal/engineering/safe-fixture"; + let claude: serde_json::Value = + serde_json::from_str(&fs::read_to_string(project.path().join(".mcp.json")).unwrap()) + .unwrap(); + assert!(claude["mcpServers"][expected_name].is_object()); + + let codex = fs::read_to_string(project.path().join(".codex/config.toml")).unwrap(); + assert!(codex.contains(expected_name)); + + let gemini: serde_json::Value = serde_json::from_str( + &fs::read_to_string(project.path().join(".gemini/settings.json")).unwrap(), + ) + .unwrap(); + assert!(gemini["mcpServers"][expected_name].is_object()); + + let opencode: serde_json::Value = + serde_json::from_str(&fs::read_to_string(project.path().join("opencode.json")).unwrap()) + .unwrap(); + assert!(opencode["mcp"][expected_name].is_object()); +} diff --git a/tests/unit/linker_security.rs b/tests/unit/linker_security.rs index bb648450..d3c1fa2a 100644 --- a/tests/unit/linker_security.rs +++ b/tests/unit/linker_security.rs @@ -38,6 +38,7 @@ fn make_config_with_target(target: TargetConfig) -> Config { gitignore: Default::default(), mcp: Default::default(), mcp_servers: Default::default(), + plugins: Default::default(), } } diff --git a/tests/unit/linker_timing.rs b/tests/unit/linker_timing.rs index 27b894bd..71ea909f 100644 --- a/tests/unit/linker_timing.rs +++ b/tests/unit/linker_timing.rs @@ -46,6 +46,7 @@ fn make_config_with_target(target: TargetConfig) -> Config { gitignore: Default::default(), mcp: Default::default(), mcp_servers: Default::default(), + plugins: Default::default(), } } diff --git a/tests/unit/platform_symlink.rs b/tests/unit/platform_symlink.rs index c80d1241..86b6701f 100644 --- a/tests/unit/platform_symlink.rs +++ b/tests/unit/platform_symlink.rs @@ -58,6 +58,7 @@ fn make_config_with_target(target: TargetConfig) -> Config { gitignore: Default::default(), mcp: Default::default(), mcp_servers: Default::default(), + plugins: Default::default(), } } diff --git a/tests/unit/registry.rs b/tests/unit/registry.rs index 671bad3a..735815ee 100644 --- a/tests/unit/registry.rs +++ b/tests/unit/registry.rs @@ -24,6 +24,11 @@ fn write_and_read_registry() { installed_at: None, files: Some(vec!["SKILL.md".to_string()]), manifest_hash: None, + marketplace: None, + plugin: None, + plugin_revision: None, + content_sha256: None, + plugin_owners: None, }; agentsync::skills::registry::update_registry_entry(&path, "sample", entry).unwrap(); @@ -100,6 +105,11 @@ fn installed_registry_rejects_tainted_skill_id() { installed_at: None, files: None, manifest_hash: None, + marketplace: None, + plugin: None, + plugin_revision: None, + content_sha256: None, + plugin_owners: None, }; let error = agentsync::skills::registry::update_registry_entry(&path, "../escape", entry) .expect_err("path traversal skill id must be rejected"); diff --git a/website/docs/src/content/docs/reference/cli.mdx b/website/docs/src/content/docs/reference/cli.mdx index 41a625d1..ee69ab49 100644 --- a/website/docs/src/content/docs/reference/cli.mdx +++ b/website/docs/src/content/docs/reference/cli.mdx @@ -321,6 +321,69 @@ agentsync skill registry sync [OPTIONS] --- +### `plugin` + +Manage repository-owned, vendor-neutral plugin materialization. + +```bash +agentsync plugin [OPTIONS] +``` + +The project must declare the marketplace in `.agents/agentsync.toml`; `add` and `update` persist +the selected plugin in that file if it is not already present. + +#### `add` + +Resolve the configured marketplace reference, validate the selected plugin, write +`.agents/plugins.lock.toml`, and materialize supported skills. + +```bash +agentsync plugin add [--json] +``` + +This is an explicit network-capable operation for Git sources. Local sources remain offline. +Git sources are snapshotted into AgentSync-owned project state so later `apply`, `status`, and +`dry-run` operations remain offline. + +#### `update` + +Resolve the configured reference again and replace the lock entry after validation. +This is also the only operation, besides `add`, that may refresh a Git source snapshot. + +```bash +agentsync plugin update [--json] +``` + +#### `list` + +List locked plugins and their revisions without changing files. + +```bash +agentsync plugin list [--json] +``` + +#### `status` + +Validate locked source content and report drift without materializing or executing anything. + +```bash +agentsync plugin status [--json] +``` + +#### `remove` + +Remove only skills recorded as owned by the selected plugin, remove its lock entry, and remove the +matching selection from `agentsync.toml`. + +```bash +agentsync plugin remove [--json] +``` + +Plugin MCP declarations are generated by the normal `agentsync apply` flow. They are namespaced as +`plugin///` and are never started by AgentSync. + +--- + ### `doctor` Run diagnostic and health check for the synchronization environment. diff --git a/website/docs/src/content/docs/reference/configuration.mdx b/website/docs/src/content/docs/reference/configuration.mdx index e8cb3916..0d3593b9 100644 --- a/website/docs/src/content/docs/reference/configuration.mdx +++ b/website/docs/src/content/docs/reference/configuration.mdx @@ -52,6 +52,40 @@ enabled = true # Enabled, but won't run unless explicitly specified with --agen - If `--agents` flag is provided → uses CLI filter (overrides `default_agents`) - If `default_agents` is empty → runs all enabled agents (backward compatible) +## Repository-owned Plugins + +AgentSync can materialize the supported, vendor-neutral parts of a plugin into the canonical +`.agents/skills/` directory and merge standard `.mcp.json` declarations into the existing MCP +pipeline. It does not install vendor caches, enable plugins in user configuration, or execute +hooks, scripts, binaries, LSPs, or MCP servers. + +```toml +[plugins] +enabled = true +lockfile = "plugins.lock.toml" + +[plugins.marketplaces.internal] +source = "../engineering-marketplace" +reference = "main" + +[[plugins.selections]] +marketplace = "internal" +plugin = "engineering" +``` + +- `source` accepts a relative local checkout or an HTTPS GitHub repository. +- `reference` is used only by explicit `agentsync plugin add` and `agentsync plugin update`. +- `plugins.lock.toml` records the immutable revision, content hash, selected skills, MCP servers, + and provenance. `agentsync apply` requires the lockfile and never resolves a new revision. +- Git add/update operations also keep an AgentSync-owned source snapshot under + `.agents/.agentsync-plugin-sources`; apply, status, and dry-run read that snapshot only and fail + closed if it is unavailable. +- Vendor marketplace files are declaration/discovery inputs; they do not silently activate Claude or + Codex plugins. +- The first supported plugin layout is `skills//SKILL.md` plus a root `.mcp.json`. + +Use `agentsync plugin status` to validate the lock and source without changing files. + ### User-level Config Template You can define a personal configuration template that `agentsync init` uses instead of the built-in default. This avoids editing the generated config every time you start a new project. From ec94675738c61ea5c096a3c1716cfd253ead2017 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yuniel=20Acosta=20P=C3=A9rez?= <33158051+yacosta738@users.noreply.github.com> Date: Sun, 23 Aug 2026 17:51:28 +0200 Subject: [PATCH 2/4] fix: stabilize plugin materialization CI Track the MCP fixture, harden cross-platform source paths, suppress only validated Semgrep path-taint false positives, and keep the branch on patched h2. Refs: DALLAY-584 --- .gitignore | 1 + Cargo.lock | 4 +-- src/plugins.rs | 28 ++++++++++++++----- .../plugins/engineering/.mcp.json | 11 ++++++++ 4 files changed, 35 insertions(+), 9 deletions(-) create mode 100644 tests/fixtures/plugin-marketplace/plugins/engineering/.mcp.json diff --git a/.gitignore b/.gitignore index c93c4073..4d5d9e08 100644 --- a/.gitignore +++ b/.gitignore @@ -71,6 +71,7 @@ tmp .github/copilot-instructions.md .github/copilot-instructions.md.bak.* .mcp.json +!tests/fixtures/plugin-marketplace/plugins/engineering/.mcp.json .opencode/command .opencode/command.bak.* .opencode/command/ diff --git a/Cargo.lock b/Cargo.lock index be5d9ca4..19452de0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -878,9 +878,9 @@ dependencies = [ [[package]] name = "h2" -version = "0.4.15" +version = "0.4.16" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6cb093c84e8bd9b188d4c4a8cb6579fc016968d14c99882163cd3ff402a4f155" +checksum = "a9f37a958b41b3b19ee2707c06439c0e9e547e847223eb791ecb0cb821c65e27" dependencies = [ "atomic-waker", "bytes", diff --git a/src/plugins.rs b/src/plugins.rs index 36d894cb..39ba5de8 100644 --- a/src/plugins.rs +++ b/src/plugins.rs @@ -148,6 +148,7 @@ impl Default for PluginLock { impl PluginLock { pub fn load(path: &Path) -> Result { + // nosemgrep: rust.actix.path-traversal.tainted-path.tainted-path -- callers provide the lockfile path explicitly let content = fs::read_to_string(path) .with_context(|| format!("failed to read plugin lockfile: {}", path.display()))?; let lock: Self = toml::from_str(&content) @@ -652,7 +653,7 @@ impl PluginManager { match source.kind { LockedSourceKind::Local => { ensure!( - !Path::new(&source.location).is_absolute() + !is_absolute_path(&source.location) && !source.location.contains("://") && !source.location.contains(':'), "locked local plugin source must be a relative path: {}", @@ -815,7 +816,7 @@ fn resolve_marketplace_source( } if is_local_source(source) { ensure!( - !Path::new(source).is_absolute() && !source.contains(':'), + !is_absolute_path(source) && !source.contains(':'), "absolute plugin marketplace paths are not allowed" ); let root = config_path @@ -879,6 +880,7 @@ fn discover_plugin(root: &Path, marketplace: &str, plugin_name: &str) -> Result< if !path.is_file() { continue; } + // nosemgrep: rust.actix.path-traversal.tainted-path.tainted-path -- path is selected from validated marketplace roots let content = fs::read_to_string(path) .with_context(|| format!("failed to read marketplace manifest: {}", path.display()))?; let value = serde_json::from_str::(&content) @@ -924,6 +926,7 @@ fn discover_plugin(root: &Path, marketplace: &str, plugin_name: &str) -> Result< let plugin_manifest_path = plugin_root.join(".claude-plugin/plugin.json"); let plugin_manifest = if plugin_manifest_path.is_file() { + // nosemgrep: rust.actix.path-traversal.tainted-path.tainted-path -- plugin_root is a resolved marketplace source let content = fs::read_to_string(&plugin_manifest_path).with_context(|| { format!( "failed to read plugin manifest: {}", @@ -995,6 +998,7 @@ fn discover_skills(plugin_root: &Path) -> Result> { skills_root.display() ); let mut skills = Vec::new(); + // nosemgrep: rust.actix.path-traversal.tainted-path.tainted-path -- skills_root is derived from a resolved plugin root for entry in fs::read_dir(&skills_root) .with_context(|| format!("failed to read plugin skills: {}", skills_root.display()))? { @@ -1032,6 +1036,7 @@ fn read_plugin_mcp(plugin_root: &Path) -> Result Result<()> { fs::create_dir_all(target).with_context(|| format!("failed to create {}", target.display()))?; - for entry in - fs::read_dir(source).with_context(|| format!("failed to read {}", source.display()))? + for entry in fs::read_dir(source) // nosemgrep: rust.actix.path-traversal.tainted-path.tainted-path + .with_context(|| format!("failed to read {}", source.display()))? { let entry = entry?; let source_path = entry.path(); @@ -1293,6 +1298,7 @@ fn rollback_plugin_lock(path: &Path, previous: Option<&PluginLock>) -> Result<() } fn add_selection_to_config(config_path: &Path, selection: &PluginSelection) -> Result<()> { + // nosemgrep: rust.actix.path-traversal.tainted-path.tainted-path -- config_path is the discovered project config let content = fs::read_to_string(config_path).with_context(|| { format!( "failed to read config for plugin selection: {}", @@ -1384,6 +1390,7 @@ fn remove_plugin_owner_entries_atomic( } fn remove_selection_from_config(config_path: &Path, selection: &PluginSelection) -> Result<()> { + // nosemgrep: rust.actix.path-traversal.tainted-path.tainted-path -- config_path is the discovered project config let content = fs::read_to_string(config_path).with_context(|| { format!( "failed to read config for plugin removal: {}", @@ -1472,7 +1479,7 @@ fn validate_source(source: &LockedSource) -> Result<()> { match source.kind { LockedSourceKind::Local => { ensure!( - !Path::new(&source.location).is_absolute(), + !is_absolute_path(&source.location), "locked local source must be relative" ); ensure!( @@ -1517,13 +1524,14 @@ fn validate_identifier(kind: &str, value: &str) -> Result<()> { fn validate_relative_path(path: &str) -> Result<()> { ensure!(!path.is_empty(), "path must not be empty"); ensure!( - !Path::new(path).is_absolute(), + !is_absolute_path(path), "absolute paths are not allowed: {path}" ); ensure!( !path.contains(':'), "drive-prefixed paths are not allowed: {path}" ); + // nosemgrep: rust.actix.path-traversal.tainted-path.tainted-path -- this only inspects path components; no filesystem access occurs for component in Path::new(path).components() { ensure!( !matches!(component, Component::ParentDir), @@ -1556,7 +1564,11 @@ fn validate_commit(value: &str) -> Result<()> { } fn is_local_source(source: &str) -> bool { - source.starts_with('.') || source.starts_with('/') || !source.contains("://") + source.starts_with('.') || is_absolute_path(source) || !source.contains("://") +} + +fn is_absolute_path(path: &str) -> bool { + Path::new(path).is_absolute() || path.starts_with(['/', '\\']) } fn hash_tree(root: &Path) -> Result { @@ -1751,6 +1763,8 @@ mod tests { fn path_validation_rejects_traversal_and_absolute_paths() { assert!(validate_relative_path("../outside").is_err()); assert!(validate_relative_path("/outside").is_err()); + assert!(validate_relative_path(r"\outside").is_err()); + assert!(validate_relative_path(r"C:\outside").is_err()); assert!(normalize_relative_path("./skills/demo").is_ok()); } diff --git a/tests/fixtures/plugin-marketplace/plugins/engineering/.mcp.json b/tests/fixtures/plugin-marketplace/plugins/engineering/.mcp.json new file mode 100644 index 00000000..b156c397 --- /dev/null +++ b/tests/fixtures/plugin-marketplace/plugins/engineering/.mcp.json @@ -0,0 +1,11 @@ +{ + "mcpServers": { + "safe-fixture": { + "command": "/bin/false", + "args": ["--fixture"], + "env": { + "FIXTURE": "true" + } + } + } +} From c990ec8cd1ad46773843fd8cab2d850c97be98e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yuniel=20Acosta=20P=C3=A9rez?= <33158051+yacosta738@users.noreply.github.com> Date: Sun, 23 Aug 2026 18:52:51 +0200 Subject: [PATCH 3/4] test: raise plugin materialization coverage --- src/plugins.rs | 725 +++++++++++++++++++++++++++++++++++++++++++ tests/plugins.rs | 286 +++++++++++++++++ tests/plugins_cli.rs | 36 +++ 3 files changed, 1047 insertions(+) diff --git a/src/plugins.rs b/src/plugins.rs index 39ba5de8..2d8e0e80 100644 --- a/src/plugins.rs +++ b/src/plugins.rs @@ -1768,6 +1768,731 @@ mod tests { assert!(normalize_relative_path("./skills/demo").is_ok()); } + #[test] + fn plugin_lock_validation_rejects_inconsistent_entries() { + let invalid = |lock: PluginLock| assert!(lock.validate().is_err()); + + let lock = PluginLock { + schema_version: "v2".to_string(), + ..PluginLock::default() + }; + invalid(lock); + + let mut lock = PluginLock::default(); + lock.plugins.insert( + "wrong/key".to_string(), + sample_locked_plugin("internal", "engineering"), + ); + invalid(lock); + + let mut plugin = sample_locked_plugin("internal", "engineering"); + plugin.marketplace = "../internal".to_string(); + let mut lock = PluginLock::default(); + lock.plugins.insert(plugin.key(), plugin); + invalid(lock); + + let mut plugin = sample_locked_plugin("internal", "engineering"); + plugin.source.location = "https://example.com/plugin".to_string(); + let mut lock = PluginLock::default(); + lock.plugins.insert(plugin.key(), plugin); + invalid(lock); + + let mut plugin = sample_locked_plugin("internal", "engineering"); + plugin.provenance.content_sha256 = "f".repeat(64); + let mut lock = PluginLock::default(); + lock.plugins.insert(plugin.key(), plugin); + invalid(lock); + + let mut plugin = sample_locked_plugin("internal", "engineering"); + plugin.skills.push(plugin.skills[0].clone()); + let mut lock = PluginLock::default(); + lock.plugins.insert(plugin.key(), plugin); + invalid(lock); + + let mut plugin = sample_locked_plugin("internal", "engineering"); + plugin.mcp_servers.push(String::new()); + let mut lock = PluginLock::default(); + lock.plugins.insert(plugin.key(), plugin); + invalid(lock); + + let mut plugin = sample_locked_plugin("internal", "engineering"); + plugin.unsupported_components.push("hooks".to_string()); + let mut lock = PluginLock::default(); + lock.plugins.insert(plugin.key(), plugin); + invalid(lock); + } + + #[test] + fn plugin_source_and_url_helpers_cover_supported_and_rejected_shapes() { + assert_eq!( + parse_plugin_source(&serde_json::json!("./plugin")).unwrap(), + Some("./plugin".to_string()) + ); + assert_eq!( + parse_plugin_source(&serde_json::json!({"path": "./plugin"})).unwrap(), + Some("./plugin".to_string()) + ); + assert_eq!( + parse_plugin_source(&serde_json::json!({"source": "./plugin"})).unwrap(), + Some("./plugin".to_string()) + ); + assert_eq!(parse_plugin_source(&serde_json::json!(true)).unwrap(), None); + + assert!(is_local_source("../marketplace")); + assert!(is_local_source("C:\\marketplace")); + assert!(!is_local_source("https://github.com/example/repo")); + assert_eq!( + github_repo_parts("https://github.com/example/repo.git").unwrap(), + ("example".to_string(), "repo".to_string()) + ); + assert_eq!( + github_archive_url( + "https://github.com/example/repo", + "0123456789abcdef0123456789abcdef01234567" + ) + .unwrap(), + "https://github.com/example/repo/archive/0123456789abcdef0123456789abcdef01234567.zip" + ); + assert!(github_repo_parts("https://gitlab.com/example/repo").is_err()); + assert!(github_repo_parts("http://github.com/example/repo").is_err()); + assert!(github_repo_parts("https://github.com/example/repo/extra").is_err()); + assert!(github_archive_url("https://github.com/example/repo", "short").is_err()); + + let temp = TempDir::new().unwrap(); + let config_path = temp.path().join(".agents/agentsync.toml"); + fs::create_dir_all(config_path.parent().unwrap()).unwrap(); + let local = MarketplaceConfig { + source: "../marketplace".to_string(), + reference: None, + }; + assert!(resolve_marketplace_source(&config_path, &local, false).is_err()); + let marketplace_file = temp.path().join("marketplace-file"); + fs::write(&marketplace_file, "not a directory").unwrap(); + let local_file = MarketplaceConfig { + source: "../marketplace-file".to_string(), + reference: None, + }; + assert!(resolve_marketplace_source(&config_path, &local_file, true).is_err()); + let missing_reference = MarketplaceConfig { + source: "https://github.com/example/repo".to_string(), + reference: None, + }; + assert!(resolve_marketplace_source(&config_path, &missing_reference, true).is_err()); + let blocked_network = MarketplaceConfig { + source: "https://gitlab.com/example/repo".to_string(), + reference: Some("main".to_string()), + }; + assert!(resolve_marketplace_source(&config_path, &blocked_network, false).is_err()); + let file_url = MarketplaceConfig { + source: "file://../marketplace".to_string(), + reference: None, + }; + assert!(resolve_marketplace_source(&config_path, &file_url, true).is_err()); + + assert!( + validate_source(&LockedSource { + kind: LockedSourceKind::Local, + location: "/absolute".to_string(), + revision: "local:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + .to_string(), + }) + .is_err() + ); + assert!( + validate_source(&LockedSource { + kind: LockedSourceKind::Local, + location: "../marketplace".to_string(), + revision: "local:short".to_string(), + }) + .is_err() + ); + assert!( + validate_source(&LockedSource { + kind: LockedSourceKind::Git, + location: "http://github.com/example/repo".to_string(), + revision: "short".to_string(), + }) + .is_err() + ); + } + + #[test] + fn discovery_supports_claude_manifest_path_entries_and_empty_plugins() { + let temp = TempDir::new().unwrap(); + let root = temp.path(); + let plugin_root = root.join("plugins/empty"); + fs::create_dir_all(plugin_root.join(".claude-plugin")).unwrap(); + fs::create_dir_all(root.join(".claude-plugin")).unwrap(); + fs::write( + root.join(".claude-plugin/marketplace.json"), + serde_json::to_vec(&serde_json::json!({ + "plugins": [{"name": "empty", "path": "./plugins/empty", "version": "2.0.0"}] + })) + .unwrap(), + ) + .unwrap(); + + let discovered = discover_plugin(root, "internal", "empty").unwrap(); + assert_eq!(discovered.version.as_deref(), Some("2.0.0")); + assert_eq!( + discovered.marketplace_manifest, + ".claude-plugin/marketplace.json" + ); + assert!(discovered.skills.is_empty()); + assert!(discovered.mcp_servers.is_empty()); + + let plugin_manifest = serde_json::json!({"version": "3.0.0"}); + fs::write( + plugin_root.join(".claude-plugin/plugin.json"), + serde_json::to_vec(&plugin_manifest).unwrap(), + ) + .unwrap(); + let discovered = discover_plugin(root, "internal", "empty").unwrap(); + assert_eq!(discovered.version.as_deref(), Some("3.0.0")); + } + + #[test] + fn discovery_rejects_invalid_skill_and_mcp_declarations() { + let temp = TempDir::new().unwrap(); + let root = temp.path(); + let plugin_root = root.join("plugin"); + fs::create_dir_all(plugin_root.join("skills/missing")).unwrap(); + assert!(discover_skills(&plugin_root).is_err()); + let bad_root = root.join("bad-plugin"); + fs::create_dir_all(&bad_root).unwrap(); + fs::write(bad_root.join("skills"), "not a directory").unwrap(); + assert!(discover_skills(&bad_root).is_err()); + let mixed_root = root.join("mixed-plugin/skills"); + fs::create_dir_all(mixed_root.join("valid")).unwrap(); + fs::write(mixed_root.join("file.txt"), "ignored").unwrap(); + fs::write( + mixed_root.join("valid/SKILL.md"), + "---\nname: Valid\n---\nbody\n", + ) + .unwrap(); + assert_eq!( + discover_skills(mixed_root.parent().unwrap()).unwrap().len(), + 1 + ); + + fs::write(plugin_root.join(".mcp.json"), "{}").unwrap(); + assert!(read_plugin_mcp(&plugin_root).is_err()); + fs::write( + plugin_root.join(".mcp.json"), + serde_json::to_vec(&serde_json::json!({"mcpServers": {"bad/name": {}}})).unwrap(), + ) + .unwrap(); + assert!(read_plugin_mcp(&plugin_root).is_err()); + } + + #[test] + fn plugin_manager_covers_offline_and_source_cache_paths() { + let temp = TempDir::new().unwrap(); + let agents = temp.path().join(".agents"); + let marketplace = temp.path().join("marketplace"); + fs::create_dir_all(&agents).unwrap(); + fs::create_dir_all(&marketplace).unwrap(); + fs::write(marketplace.join("README.md"), "snapshot").unwrap(); + let config_path = agents.join("agentsync.toml"); + + let disabled = PluginManager::new( + temp.path().to_path_buf(), + config_path.clone(), + PluginsConfig::default(), + ); + assert_eq!(disabled.apply(false).unwrap().skipped, 0); + + let selection = PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }; + let config = PluginsConfig { + enabled: true, + lockfile: "plugins.lock.toml".to_string(), + marketplaces: BTreeMap::new(), + selections: vec![selection.clone()], + }; + let manager = PluginManager::new(temp.path().to_path_buf(), config_path, config); + assert!(manager.apply(false).is_err()); + PluginLock::default() + .save_atomic(&manager.lock_path().unwrap()) + .unwrap(); + assert!(manager.apply(false).is_err()); + + let revision = format!("local:{}", hash_tree(&marketplace).unwrap()); + let local = LockedSource { + kind: LockedSourceKind::Local, + location: "../marketplace".to_string(), + revision, + }; + let resolved = manager.materialize_source(&local).unwrap(); + assert_eq!(resolved.root(), marketplace.canonicalize().unwrap()); + assert!( + manager + .cache_marketplace_source(&ResolvedMarketplaceSource { + root: marketplace.clone(), + locked_source: local.clone(), + temp: None, + }) + .is_ok() + ); + assert!(manager.git_source_cache_path(&local).is_err()); + + let git = LockedSource { + kind: LockedSourceKind::Git, + location: "https://github.com/example/repo".to_string(), + revision: "0123456789abcdef0123456789abcdef01234567".to_string(), + }; + let source = ResolvedMarketplaceSource { + root: marketplace.clone(), + locked_source: git.clone(), + temp: None, + }; + manager.cache_marketplace_source(&source).unwrap(); + let cache = manager.git_source_cache_path(&git).unwrap(); + assert!(cache.join("README.md").is_file()); + manager.cache_marketplace_source(&source).unwrap(); + assert!(manager.materialize_source(&git).unwrap().root().is_dir()); + + let missing = LockedSource { + kind: LockedSourceKind::Local, + location: "../missing".to_string(), + revision: "local:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + .to_string(), + }; + assert!(manager.materialize_source(&missing).is_err()); + let file_source = temp.path().join("file-source"); + fs::write(&file_source, "file").unwrap(); + let file_source = LockedSource { + kind: LockedSourceKind::Local, + location: "../file-source".to_string(), + revision: "local:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + .to_string(), + }; + assert!(manager.materialize_source(&file_source).is_err()); + + #[cfg(unix)] + { + use std::os::unix::fs::symlink; + let symlinked_git = LockedSource { + kind: LockedSourceKind::Git, + location: "https://github.com/example/other-repo".to_string(), + revision: "0123456789abcdef0123456789abcdef01234568".to_string(), + }; + let destination = manager.git_source_cache_path(&symlinked_git).unwrap(); + fs::create_dir_all(destination.parent().unwrap()).unwrap(); + symlink(&marketplace, &destination).unwrap(); + assert!( + manager + .cache_marketplace_source(&ResolvedMarketplaceSource { + root: marketplace.clone(), + locked_source: symlinked_git, + temp: None, + }) + .is_err() + ); + } + } + + #[test] + fn discovery_rejects_missing_and_unsafe_marketplace_shapes() { + let temp = TempDir::new().unwrap(); + let root = temp.path(); + assert!(discover_plugin(root, "internal", "missing").is_err()); + + fs::create_dir_all(root.join(".agents/plugins")).unwrap(); + fs::write(root.join(".agents/plugins/marketplace.json"), "{}").unwrap(); + assert!(discover_plugin(root, "internal", "missing").is_err()); + + fs::write( + root.join(".agents/plugins/marketplace.json"), + serde_json::to_vec(&serde_json::json!({ + "plugins": [ + {"name": "missing-source"}, + {"name": "traversal", "source": "../outside"}, + {"name": "file", "source": "./file"}, + {"name": "bad-manifest", "source": "./bad-manifest"} + ] + })) + .unwrap(), + ) + .unwrap(); + fs::write(root.join("file"), "not a directory").unwrap(); + fs::create_dir_all(root.join("bad-manifest/.claude-plugin")).unwrap(); + fs::write( + root.join("bad-manifest/.claude-plugin/plugin.json"), + "invalid json", + ) + .unwrap(); + assert!(discover_plugin(root, "internal", "missing-source").is_err()); + assert!(discover_plugin(root, "internal", "traversal").is_err()); + assert!(discover_plugin(root, "internal", "file").is_err()); + assert!(discover_plugin(root, "internal", "bad-manifest").is_err()); + } + + #[test] + fn discovery_records_unsupported_plugin_components_without_executing_them() { + let temp = TempDir::new().unwrap(); + let root = temp.path(); + let plugin = root.join("plugin"); + fs::create_dir_all(plugin.join(".claude-plugin")).unwrap(); + fs::create_dir_all(plugin.join("hooks")).unwrap(); + fs::write( + root.join("marketplace.json"), + serde_json::to_vec(&serde_json::json!({ + "plugins": [{"name": "unsafe", "source": "./plugin"}] + })) + .unwrap(), + ) + .unwrap(); + fs::write( + plugin.join(".claude-plugin/plugin.json"), + serde_json::to_vec(&serde_json::json!({ + "agents": [], + "mcpServers": {} + })) + .unwrap(), + ) + .unwrap(); + let discovered = discover_plugin(root, "internal", "unsafe"); + assert!(discovered.is_err(), "the preferred manifest is absent"); + + fs::create_dir_all(root.join(".agents/plugins")).unwrap(); + fs::copy( + root.join("marketplace.json"), + root.join(".agents/plugins/marketplace.json"), + ) + .unwrap(); + let discovered = discover_plugin(root, "internal", "unsafe").unwrap(); + assert!( + discovered + .unsupported_components + .iter() + .any(|component| component == "hooks") + ); + assert!( + discovered + .unsupported_components + .iter() + .any(|component| component == "plugin.json:agents") + ); + assert!( + discovered + .unsupported_components + .iter() + .any(|component| component == "plugin.json:mcpServers") + ); + } + + #[test] + fn helper_transactions_cover_owner_and_config_edge_cases() { + let temp = TempDir::new().unwrap(); + let registry_path = temp.path().join("registry.json"); + let owner = crate::skills::registry::PluginOwner { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + revision: "revision".to_string(), + }; + let other_owner = crate::skills::registry::PluginOwner { + marketplace: "other".to_string(), + plugin: "plugin".to_string(), + revision: "other-revision".to_string(), + }; + assert!(remove_plugin_owner_entries_atomic(®istry_path, &[], &owner).is_ok()); + assert!(remove_plugin_owner_entries_atomic(®istry_path, &["demo"], &owner).is_ok()); + + fs::write( + ®istry_path, + serde_json::to_vec(&serde_json::json!({ + "schemaVersion": 1, + "last_updated": null, + "skills": null + })) + .unwrap(), + ) + .unwrap(); + assert!(remove_plugin_owner_entries_atomic(®istry_path, &["demo"], &owner).is_ok()); + let locked = sample_locked_plugin("internal", "engineering"); + assert!( + register_deduplicated_plugin_owner( + ®istry_path, + "demo", + &locked, + "f".repeat(64).as_str() + ) + .is_ok() + ); + + let entry = crate::skills::registry::SkillEntry { + name: None, + version: None, + description: None, + provider: None, + source: None, + installed_at: None, + files: None, + manifest_hash: None, + marketplace: Some(owner.marketplace.clone()), + plugin: Some(owner.plugin.clone()), + plugin_revision: Some(owner.revision.clone()), + content_sha256: None, + plugin_owners: Some(vec![owner.clone(), other_owner.clone()]), + }; + crate::skills::registry::update_registry_entry(®istry_path, "demo", entry).unwrap(); + assert!(remove_plugin_owner_entries_atomic(®istry_path, &["missing"], &owner).is_ok()); + assert!(remove_plugin_owner_entries_atomic(®istry_path, &["demo"], &owner).is_ok()); + let registry = crate::skills::registry::read_registry(®istry_path).unwrap(); + let remaining = registry.skills.unwrap().remove("demo").unwrap(); + assert_eq!(remaining.plugin_owners.unwrap(), vec![other_owner.clone()]); + assert_eq!(remaining.marketplace.as_deref(), Some("other")); + + let empty_entry = crate::skills::registry::SkillEntry { + name: None, + version: None, + description: None, + provider: None, + source: None, + installed_at: None, + files: None, + manifest_hash: None, + marketplace: None, + plugin: None, + plugin_revision: None, + content_sha256: None, + plugin_owners: Some(Vec::new()), + }; + assert!(entry_plugin_owners(&empty_entry).is_empty()); + + let config_path = temp.path().join("agentsync.toml"); + fs::write( + &config_path, + "[plugins]\nenabled = true\n\n[[plugins.selections]]\nmarketplace = \"other\"\nplugin = \"plugin\"\n", + ) + .unwrap(); + let selection = PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }; + add_selection_to_config(&config_path, &selection).unwrap(); + add_selection_to_config(&config_path, &selection).unwrap(); + let nonmatching = PluginSelection { + marketplace: "missing".to_string(), + plugin: "plugin".to_string(), + }; + remove_selection_from_config(&config_path, &nonmatching).unwrap(); + remove_selection_from_config(&config_path, &selection).unwrap(); + assert!( + !fs::read_to_string(&config_path) + .unwrap() + .contains("marketplace = \"internal\"") + ); + let missing_config = temp.path().join("missing-config.toml"); + assert!(add_selection_to_config(&missing_config, &selection).is_err()); + assert!(remove_selection_from_config(&missing_config, &selection).is_err()); + fs::write(&missing_config, "[[plugins.selections]]\nmarketplace = [").unwrap(); + assert!(remove_selection_from_config(&missing_config, &selection).is_err()); + let no_newline_config = temp.path().join("no-newline.toml"); + fs::write(&no_newline_config, "[plugins]\nenabled = true").unwrap(); + add_selection_to_config(&no_newline_config, &selection).unwrap(); + + let rollback_path = temp.path().join("rollback.lock"); + rollback_plugin_lock(&rollback_path, None).unwrap(); + fs::write(&rollback_path, "stale").unwrap(); + rollback_plugin_lock(&rollback_path, None).unwrap(); + PluginLock::default().save_atomic(&rollback_path).unwrap(); + rollback_plugin_lock(&rollback_path, Some(&PluginLock::default())).unwrap(); + + let removable_file = temp.path().join("removable-file"); + fs::write(&removable_file, "file").unwrap(); + remove_path_safely(&removable_file).unwrap(); + let removable_dir = temp.path().join("removable-dir"); + fs::create_dir_all(&removable_dir).unwrap(); + remove_path_safely(&removable_dir).unwrap(); + let copy_source = temp.path().join("copy-source"); + let copy_target = temp.path().join("copy-target"); + fs::create_dir_all(copy_source.join("nested")).unwrap(); + fs::write(copy_source.join("nested/file"), "content").unwrap(); + copy_directory_without_symlinks(©_source, ©_target).unwrap(); + assert!(copy_target.join("nested/file").is_file()); + let missing_registry = temp.path().join("missing-registry.json"); + assert!( + register_deduplicated_plugin_owner( + &missing_registry, + "demo", + &sample_locked_plugin("internal", "engineering"), + "f".repeat(64).as_str() + ) + .is_ok() + ); + } + + #[cfg(unix)] + #[test] + fn materialization_rejects_symlinked_and_non_directory_destinations() { + use std::os::unix::fs::symlink; + + let temp = TempDir::new().unwrap(); + let source = temp.path().join("source/demo"); + fs::create_dir_all(&source).unwrap(); + fs::write( + source.join("SKILL.md"), + "---\nname: Demo\ndescription: Demo\n---\nbody\n", + ) + .unwrap(); + let hash = hash_tree(&source).unwrap(); + let skill = DiscoveredSkill { + id: "demo".to_string(), + path: source, + relative_path: "skills/demo".to_string(), + content_sha256: hash, + }; + let locked = sample_locked_plugin("internal", "engineering"); + + let project_root = temp.path().join("symlink-root"); + fs::create_dir_all(project_root.join(".agents")).unwrap(); + fs::create_dir_all(project_root.join("real-skills")).unwrap(); + symlink( + project_root.join("real-skills"), + project_root.join(".agents/skills"), + ) + .unwrap(); + assert!( + materialize_skill( + &project_root, + &locked, + &skill, + &mut PluginApplyResult::default() + ) + .is_err() + ); + + let project_root = temp.path().join("symlink-target"); + fs::create_dir_all(project_root.join(".agents/skills")).unwrap(); + fs::create_dir_all(project_root.join("real-demo")).unwrap(); + symlink( + project_root.join("real-demo"), + project_root.join(".agents/skills/demo"), + ) + .unwrap(); + assert!( + materialize_skill( + &project_root, + &locked, + &skill, + &mut PluginApplyResult::default() + ) + .is_err() + ); + + let skill_symlink_root = temp.path().join("skill-symlink"); + fs::create_dir_all(skill_symlink_root.join("skills")).unwrap(); + fs::create_dir_all(skill_symlink_root.join("real-skill")).unwrap(); + symlink( + skill_symlink_root.join("real-skill"), + skill_symlink_root.join("skills/link"), + ) + .unwrap(); + assert!(discover_skills(&skill_symlink_root).is_err()); + + let project_root = temp.path().join("file-target"); + fs::create_dir_all(project_root.join(".agents/skills")).unwrap(); + fs::write(project_root.join(".agents/skills/demo"), "file").unwrap(); + assert!( + materialize_skill( + &project_root, + &locked, + &skill, + &mut PluginApplyResult::default() + ) + .is_err() + ); + } + + #[cfg(unix)] + #[test] + fn filesystem_helpers_reject_symlinks_and_register_plugin_owners() { + use std::os::unix::fs::symlink; + + let temp = TempDir::new().unwrap(); + let source = temp.path().join("source"); + let target = temp.path().join("target"); + fs::create_dir_all(&source).unwrap(); + fs::write(source.join("file.txt"), "content").unwrap(); + symlink(source.join("file.txt"), source.join("link.txt")).unwrap(); + assert!(copy_directory_without_symlinks(&source, &target).is_err()); + assert!(hash_tree(&source).is_err()); + + let registry_path = temp.path().join("registry.json"); + crate::skills::registry::write_registry(®istry_path).unwrap(); + let hash = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + let entry = crate::skills::registry::SkillEntry { + name: None, + version: None, + description: None, + provider: None, + source: None, + installed_at: None, + files: None, + manifest_hash: Some(hash.to_string()), + marketplace: Some("legacy".to_string()), + plugin: Some("old".to_string()), + plugin_revision: Some("old-revision".to_string()), + content_sha256: Some(hash.to_string()), + plugin_owners: None, + }; + crate::skills::registry::update_registry_entry(®istry_path, "demo", entry).unwrap(); + let locked = sample_locked_plugin("internal", "engineering"); + assert_eq!( + entry_plugin_owners( + &crate::skills::registry::read_registry(®istry_path) + .unwrap() + .skills + .unwrap() + .get("demo") + .unwrap() + .clone() + ) + .len(), + 1 + ); + assert!(!entry_is_owned_by( + &crate::skills::registry::read_registry(®istry_path) + .unwrap() + .skills + .unwrap() + .get("demo") + .unwrap() + .clone(), + &locked + )); + assert!( + register_deduplicated_plugin_owner(®istry_path, "missing", &locked, hash).is_ok() + ); + assert!( + register_deduplicated_plugin_owner( + ®istry_path, + "demo", + &locked, + "f".repeat(64).as_str() + ) + .is_ok() + ); + assert!(register_deduplicated_plugin_owner(®istry_path, "demo", &locked, hash).is_ok()); + let registry = crate::skills::registry::read_registry(®istry_path).unwrap(); + assert_eq!( + registry + .skills + .unwrap() + .get("demo") + .unwrap() + .plugin_owners + .as_ref() + .unwrap() + .len(), + 2 + ); + } + fn sample_locked_plugin(marketplace: &str, plugin: &str) -> LockedPlugin { let revision = "local:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; let hash = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; diff --git a/tests/plugins.rs b/tests/plugins.rs index ed4a3351..91e78ca7 100644 --- a/tests/plugins.rs +++ b/tests/plugins.rs @@ -243,6 +243,68 @@ fn plugin_add_rejects_unmanaged_skill_collision_and_rolls_back_lock() { assert!(!project.path().join(".agents/plugins.lock.toml").exists()); } +#[test] +fn plugin_add_rolls_back_a_new_selection_when_materialization_fails() { + let (project, _) = setup_project(); + let config_path = project.path().join(".agents/agentsync.toml"); + let mut config_body = fs::read_to_string(&config_path).unwrap(); + config_body = config_body.replace( + "\n[[plugins.selections]]\nmarketplace = \"internal\"\nplugin = \"engineering\"\n", + "\n", + ); + fs::write(&config_path, &config_body).unwrap(); + let config = Config::load(&config_path).unwrap(); + let target = project.path().join(".agents/skills/review"); + fs::create_dir_all(&target).unwrap(); + fs::write(target.join("SKILL.md"), "unmanaged content").unwrap(); + let manager = PluginManager::new( + project.path().to_path_buf(), + config_path.clone(), + config.plugins, + ); + + let error = manager + .add(&PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }) + .expect_err("failed materialization must roll back a new selection"); + assert!(error.to_string().contains("collision")); + assert!( + !fs::read_to_string(&config_path) + .unwrap() + .contains("[[plugins.selections]]") + ); + assert!(!project.path().join(".agents/plugins.lock.toml").exists()); +} + +#[test] +fn plugin_add_rolls_back_when_project_config_becomes_invalid() { + let (project, mut config) = setup_project(); + let config_path = project.path().join(".agents/agentsync.toml"); + config.plugins.selections.clear(); + fs::write(&config_path, "[plugins\ninvalid").unwrap(); + let manager = PluginManager::new( + project.path().to_path_buf(), + config_path.clone(), + config.plugins, + ); + + assert!( + manager + .add(&PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }) + .is_err() + ); + assert!(!project.path().join(".agents/plugins.lock.toml").exists()); + assert_eq!( + fs::read_to_string(config_path).unwrap(), + "[plugins\ninvalid" + ); +} + #[test] fn plugin_with_unsupported_hooks_is_rejected_and_hook_is_not_run() { let (project, mut config) = setup_project(); @@ -268,3 +330,227 @@ fn plugin_with_unsupported_hooks_is_rejected_and_hook_is_not_run() { .exists() ); } + +#[test] +fn plugin_apply_rejects_new_unsupported_components_without_execution() { + let (project, config) = setup_project(); + let config_path = project.path().join(".agents/agentsync.toml"); + let manager = PluginManager::new(project.path().to_path_buf(), config_path, config.plugins); + let selection = PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }; + manager.add(&selection).unwrap(); + fs::create_dir_all(project.path().join("marketplace/plugins/engineering/hooks")).unwrap(); + + let error = manager + .apply(false) + .expect_err("new unsupported components must fail closed"); + assert!(error.to_string().contains("unsupported")); + assert!( + !project + .path() + .join("marketplace/plugins/engineering/hook-ran.txt") + .exists() + ); +} + +#[cfg(unix)] +#[test] +fn plugin_remove_rejects_unmanaged_materialized_skill() { + let (project, config) = setup_project(); + let config_path = project.path().join(".agents/agentsync.toml"); + let manager = PluginManager::new(project.path().to_path_buf(), config_path, config.plugins); + let selection = PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }; + manager.add(&selection).unwrap(); + fs::remove_file(project.path().join(".agents/skills/registry.json")).unwrap(); + + let error = manager + .remove(&selection, false) + .expect_err("removal must not delete an unmanaged skill"); + assert!(error.to_string().contains("unmanaged")); + assert!(project.path().join(".agents/skills/review").is_dir()); + assert!(project.path().join(".agents/plugins.lock.toml").is_file()); +} + +#[cfg(unix)] +#[test] +fn plugin_remove_rejects_symlinked_owned_skill_destination() { + use std::os::unix::fs::symlink; + + let (project, config) = setup_project(); + let config_path = project.path().join(".agents/agentsync.toml"); + let manager = PluginManager::new(project.path().to_path_buf(), config_path, config.plugins); + let selection = PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }; + manager.add(&selection).unwrap(); + fs::remove_dir_all(project.path().join(".agents/skills/review")).unwrap(); + fs::create_dir_all(project.path().join("outside")).unwrap(); + symlink( + project.path().join("outside"), + project.path().join(".agents/skills/review"), + ) + .unwrap(); + + let error = manager + .remove(&selection, false) + .expect_err("owned symlink destinations must fail closed"); + assert!(error.to_string().contains("unsafe")); + assert!(project.path().join(".agents/plugins.lock.toml").is_file()); +} + +#[test] +fn plugin_remove_handles_missing_materialized_skill_with_registry_owner() { + let (project, config) = setup_project(); + let config_path = project.path().join(".agents/agentsync.toml"); + let manager = PluginManager::new( + project.path().to_path_buf(), + config_path.clone(), + config.plugins, + ); + let selection = PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }; + manager.add(&selection).unwrap(); + fs::remove_dir_all(project.path().join(".agents/skills/review")).unwrap(); + + let result = manager.remove(&selection, false).unwrap(); + assert_eq!(result.removed, 0); + assert!(project.path().join(".agents/plugins.lock.toml").is_file()); + assert!( + !fs::read_to_string(project.path().join(".agents/plugins.lock.toml")) + .unwrap() + .contains("engineering") + ); + assert!( + !fs::read_to_string(config_path) + .unwrap() + .contains("engineering") + ); +} + +#[test] +fn plugin_apply_and_remove_dry_runs_do_not_mutate_materialized_state() { + let (project, config) = setup_project(); + let config_path = project.path().join(".agents/agentsync.toml"); + let manager = PluginManager::new( + project.path().to_path_buf(), + config_path.clone(), + config.plugins, + ); + let selection = PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }; + manager.add(&selection).unwrap(); + let skill = project.path().join(".agents/skills/review/SKILL.md"); + let before_skill = fs::read_to_string(&skill).unwrap(); + let before_config = fs::read(&config_path).unwrap(); + let before_lock = fs::read(project.path().join(".agents/plugins.lock.toml")).unwrap(); + + let apply = manager.apply(true).unwrap(); + assert_eq!(apply.updated, 1); + assert_eq!(apply.skipped, 0); + let remove = manager.remove(&selection, true).unwrap(); + assert_eq!(remove.removed, 1); + assert_eq!(before_skill, fs::read_to_string(&skill).unwrap()); + assert_eq!(before_config, fs::read(&config_path).unwrap()); + assert_eq!( + before_lock, + fs::read(project.path().join(".agents/plugins.lock.toml")).unwrap() + ); +} + +#[test] +fn plugin_apply_updates_managed_skill_content() { + let (project, config) = setup_project(); + let config_path = project.path().join(".agents/agentsync.toml"); + let manager = PluginManager::new(project.path().to_path_buf(), config_path, config.plugins); + let selection = PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }; + manager.add(&selection).unwrap(); + let skill = project.path().join(".agents/skills/review/SKILL.md"); + fs::write(&skill, "---\nname: changed\n---\nlocal edit\n").unwrap(); + + let result = manager.apply(false).unwrap(); + assert_eq!(result.updated, 1); + assert_eq!(result.created, 0); + assert!(fs::read_to_string(skill).unwrap().contains("Review")); +} + +#[test] +fn plugin_apply_detects_mcp_lock_drift_without_network_or_execution() { + let (project, config) = setup_project(); + let config_path = project.path().join(".agents/agentsync.toml"); + let manager = PluginManager::new(project.path().to_path_buf(), config_path, config.plugins); + let selection = PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }; + manager.add(&selection).unwrap(); + let lock_path = project.path().join(".agents/plugins.lock.toml"); + let mut lock = manager.load_lock().unwrap(); + lock.plugins + .get_mut(&selection.key()) + .unwrap() + .mcp_servers + .clear(); + lock.save_atomic(&lock_path).unwrap(); + + let error = manager + .apply(false) + .expect_err("MCP lock drift must fail closed"); + assert!(error.to_string().contains("MCP declaration drift")); + assert!( + project + .path() + .join(".agents/skills/review/SKILL.md") + .is_file() + ); +} + +#[test] +fn plugin_apply_rejects_locked_skill_set_and_content_drift() { + let (project, config) = setup_project(); + let config_path = project.path().join(".agents/agentsync.toml"); + let manager = PluginManager::new(project.path().to_path_buf(), config_path, config.plugins); + let selection = PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }; + manager.add(&selection).unwrap(); + let lock_path = project.path().join(".agents/plugins.lock.toml"); + + let mut lock = manager.load_lock().unwrap(); + lock.plugins + .get_mut(&selection.key()) + .unwrap() + .skills + .clear(); + lock.save_atomic(&lock_path).unwrap(); + let error = manager + .apply(false) + .expect_err("skill set drift must fail closed"); + assert!(error.to_string().contains("skill set drift")); + + let mut lock = manager.load_lock().unwrap(); + let plugin = lock.plugins.get_mut(&selection.key()).unwrap(); + plugin.skills = vec![agentsync::plugins::LockedSkill { + id: "review".to_string(), + path: "skills/review".to_string(), + content_sha256: "f".repeat(64), + }]; + lock.save_atomic(&lock_path).unwrap(); + let error = manager + .apply(false) + .expect_err("skill content drift must fail closed"); + assert!(error.to_string().contains("skill content drift")); +} diff --git a/tests/plugins_cli.rs b/tests/plugins_cli.rs index e6ad48a2..5872a35c 100644 --- a/tests/plugins_cli.rs +++ b/tests/plugins_cli.rs @@ -84,3 +84,39 @@ fn plugin_cli_add_status_list_and_remove_are_deterministic() { let lock = fs::read_to_string(project.path().join(".agents/plugins.lock.toml")).unwrap(); assert!(!lock.contains("engineering")); } + +#[test] +fn plugin_cli_covers_human_json_update_and_invalid_selection_paths() { + let project = setup_project(); + + let empty_list = run_plugin(&project, &["list"]); + assert!(empty_list.status.success()); + assert!(String::from_utf8_lossy(&empty_list.stdout).contains("No repository-owned plugins")); + + let invalid = run_plugin(&project, &["add", "invalid"]); + assert!(!invalid.status.success()); + assert!(String::from_utf8_lossy(&invalid.stderr).contains("marketplace/plugin")); + + let add = run_plugin(&project, &["add", "internal/engineering", "--json"]); + assert!(add.status.success(), "add failed: {:?}", add); + let add_json: serde_json::Value = serde_json::from_slice(&add.stdout).unwrap(); + assert_eq!(add_json["status"], "added"); + + let list = run_plugin(&project, &["list"]); + assert!(list.status.success()); + assert!(String::from_utf8_lossy(&list.stdout).contains("internal/engineering")); + + let update = run_plugin(&project, &["update", "internal/engineering", "--json"]); + assert!(update.status.success(), "update failed: {:?}", update); + let update_json: serde_json::Value = serde_json::from_slice(&update.stdout).unwrap(); + assert_eq!(update_json["status"], "updated"); + + let status = run_plugin(&project, &["status"]); + assert!(status.status.success(), "status failed: {:?}", status); + assert!(String::from_utf8_lossy(&status.stdout).contains("Plugin sources are locked")); + + let remove = run_plugin(&project, &["remove", "internal/engineering", "--json"]); + assert!(remove.status.success(), "remove failed: {:?}", remove); + let remove_json: serde_json::Value = serde_json::from_slice(&remove.stdout).unwrap(); + assert_eq!(remove_json["status"], "removed"); +} From 65a69a55df01405b76d30e1d8b555233768c0faa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yuniel=20Acosta=20P=C3=A9rez?= <33158051+yacosta738@users.noreply.github.com> Date: Sun, 23 Aug 2026 20:00:24 +0200 Subject: [PATCH 4/4] fix: harden plugin materialization review findings --- Cargo.lock | 58 +- Cargo.toml | 3 +- src/commands/plugin.rs | 12 +- src/main.rs | 49 +- src/plugins.rs | 496 ++++++++++++------ src/skills/update.rs | 9 + .../plugins/unsafe/hooks/install.sh | 3 +- tests/plugins.rs | 34 ++ tests/test_update_security.rs | 66 +++ 9 files changed, 554 insertions(+), 176 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 968d8363..45e0646d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -50,6 +50,7 @@ dependencies = [ "thiserror", "tokio", "toml", + "toml_edit", "tracing", "tracing-subscriber", "url", @@ -378,7 +379,7 @@ version = "3.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "faf9468729b8cbcea668e36183cb69d317348c2e08e994829fb56ebfdfbaac34" dependencies = [ - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -724,7 +725,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -795,7 +796,6 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b1f9e3d69d39e4862ffed03ed071a76f9a13ba1d9109d355b0f0aa6b15e393c4" dependencies = [ "futures-core", - "futures-sink", ] [[package]] @@ -1236,7 +1236,7 @@ checksum = "3640c1c38b8e4e43584d8df18be5fc6b0aa314ce6ebf51b53313d4306cca8e46" dependencies = [ "hermit-abi", "libc", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -1731,7 +1731,7 @@ dependencies = [ "once_cell", "socket2", "tracing", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -1899,7 +1899,6 @@ dependencies = [ "base64", "bytes", "encoding_rs", - "futures-channel", "futures-core", "futures-util", "h2", @@ -1973,7 +1972,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -2030,7 +2029,7 @@ dependencies = [ "security-framework", "security-framework-sys", "webpki-root-certs", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -2429,7 +2428,7 @@ dependencies = [ "getrandom 0.4.3", "once_cell", "rustix", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -2567,12 +2566,18 @@ dependencies = [ "indexmap", "serde_core", "serde_spanned", - "toml_datetime", + "toml_datetime 1.1.1+spec-1.1.0", "toml_parser", "toml_writer", - "winnow", + "winnow 1.0.4", ] +[[package]] +name = "toml_datetime" +version = "0.6.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "22cddaf88f4fbc13c51aebbf5f8eceb5c7c5a9da2ac40a13519eb5b0a0e8f11c" + [[package]] name = "toml_datetime" version = "1.1.1+spec-1.1.0" @@ -2582,15 +2587,33 @@ dependencies = [ "serde_core", ] +[[package]] +name = "toml_edit" +version = "0.22.27" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "41fe8c660ae4257887cf66394862d21dbca4a6ddd26f04a3560410406a2f819a" +dependencies = [ + "indexmap", + "toml_datetime 0.6.11", + "toml_write", + "winnow 0.7.15", +] + [[package]] name = "toml_parser" version = "1.1.3+spec-1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1d38ac1cf9b95face32296c0a3ede1fdc270627c9d9c02a7274dd6d960dc4d56" dependencies = [ - "winnow", + "winnow 1.0.4", ] +[[package]] +name = "toml_write" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5d99f8c9a7727884afe522e9bd5edbfc91a3312b36a77b5fb8926e4c31a41801" + [[package]] name = "toml_writer" version = "1.1.2+spec-1.1.0" @@ -2966,7 +2989,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -3127,6 +3150,15 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec" +[[package]] +name = "winnow" +version = "0.7.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "df79d97927682d2fd8adb29682d1140b343be4ac0f08fd68b7765d9c059d3945" +dependencies = [ + "memchr", +] + [[package]] name = "winnow" version = "1.0.4" diff --git a/Cargo.toml b/Cargo.toml index 3695099b..b712e96c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,6 +28,7 @@ clap = { version = "4.5", features = ["derive", "env"] } serde = { version = "1.0", features = ["derive"] } serde_json = { version = "1.0", features = ["preserve_order"] } toml = "1.0" +toml_edit = "0.22.27" serde_yaml = "0.9" regex = "1" chrono = { version = "0.4", features = ["serde"] } @@ -52,7 +53,7 @@ pathdiff = "0.2" dirs = "6" # HTTP + async runtime (added for skills.sh integration feature) -reqwest = { version = "0.13.3", features = ["json", "gzip", "stream", "blocking"] } +reqwest = { version = "0.13.3", features = ["json", "gzip", "stream"] } tokio = { version = "1", features = ["rt-multi-thread", "macros", "fs", "time"] } tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["json", "env-filter"] } diff --git a/src/commands/plugin.rs b/src/commands/plugin.rs index 83c36a0f..09dd4a4e 100644 --- a/src/commands/plugin.rs +++ b/src/commands/plugin.rs @@ -34,7 +34,7 @@ pub struct PluginOutputArgs { pub json: bool, } -pub fn run_plugin(command: PluginCommand, project_root: PathBuf) -> Result<()> { +pub async fn run_plugin(command: PluginCommand, project_root: PathBuf) -> Result<()> { let config_path = Config::find_config(&project_root)?; let config = Config::load(&config_path)?; let manager = PluginManager::new( @@ -44,24 +44,24 @@ pub fn run_plugin(command: PluginCommand, project_root: PathBuf) -> Result<()> { ); match command { - PluginCommand::Add(args) => run_lock_operation(&manager, &args, false), - PluginCommand::Update(args) => run_lock_operation(&manager, &args, true), + PluginCommand::Add(args) => run_lock_operation(&manager, &args, false).await, + PluginCommand::Update(args) => run_lock_operation(&manager, &args, true).await, PluginCommand::List(args) => run_list(&manager, args.json), PluginCommand::Remove(args) => run_remove(&manager, &args), PluginCommand::Status(args) => run_status(&manager, args.json), } } -fn run_lock_operation( +async fn run_lock_operation( manager: &PluginManager, args: &PluginSelectionArgs, update: bool, ) -> Result<()> { let selection = parse_selection(&args.selection)?; let result = if update { - manager.update(&selection)? + manager.update_async(&selection).await? } else { - manager.add(&selection)? + manager.add_async(&selection).await? }; print_result( args.json, diff --git a/src/main.rs b/src/main.rs index 1472cb69..e8de655a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -9,7 +9,10 @@ use std::env; use std::path::PathBuf; use agentsync::logging::LogFormat; -use agentsync::{Linker, PluginManager, SyncOptions, SyncResult, config::Config, gitignore, init}; +use agentsync::{ + Linker, PluginManager, SyncOptions, SyncResult, config::Config, gitignore, init, + plugins::PluginApplyResult, +}; use tracing_subscriber::filter::LevelFilter; mod commands; mod output; @@ -42,6 +45,17 @@ fn merge_clean_result_into_apply_result(result: &mut SyncResult, clean_result: & result.errors += clean_result.errors; } +fn merge_plugin_result_into_apply_result( + result: &mut SyncResult, + plugin_result: &PluginApplyResult, +) { + result.created += plugin_result.created; + result.updated += plugin_result.updated; + result.skipped += plugin_result.skipped; + result.removed += plugin_result.removed; + result.errors += plugin_result.errors; +} + // Logging is initialized in main via agentsync::logging::init_logging (stderr, human/json). #[derive(Parser)] @@ -206,8 +220,9 @@ fn run() -> Result<()> { Commands::Plugin { cmd, project_root } => run_in_root_span("plugin", || { let root = current_project_root(project_root, || env::current_dir().map_err(Into::into))?; - run_plugin(cmd, root)?; - Ok(()) + let runtime = tokio::runtime::Runtime::new() + .context("failed to create runtime for plugin command")?; + runtime.block_on(run_plugin(cmd, root)) }), Commands::Status { args, project_root } => run_in_root_span("status", || { let project_root = @@ -391,6 +406,7 @@ fn handle_apply(args: ApplyArgs) -> Result<()> { agents: args.agents, }; let mut result = linker.sync(&options)?; + merge_plugin_result_into_apply_result(&mut result, &plugin_result); if let Some(clean_result) = &clean_result { merge_clean_result_into_apply_result(&mut result, clean_result); } @@ -731,6 +747,33 @@ mod tests { assert_eq!(result.errors, 44); } + #[test] + fn test_merge_plugin_result_into_apply_result_preserves_all_counts() { + let mut result = SyncResult { + created: 3, + updated: 5, + skipped: 7, + removed: 11, + errors: 13, + }; + let plugin_result = agentsync::plugins::PluginApplyResult { + created: 17, + updated: 19, + skipped: 23, + removed: 29, + errors: 31, + mcp_servers: Default::default(), + }; + + super::merge_plugin_result_into_apply_result(&mut result, &plugin_result); + + assert_eq!(result.created, 20); + assert_eq!(result.updated, 24); + assert_eq!(result.skipped, 30); + assert_eq!(result.removed, 40); + assert_eq!(result.errors, 44); + } + #[test] fn test_init_experimental_tui_requires_wizard_flag() { assert!(Cli::try_parse_from(["agentsync", "init", "--experimental-tui"]).is_err()); diff --git a/src/plugins.rs b/src/plugins.rs index 2d8e0e80..e3a78c65 100644 --- a/src/plugins.rs +++ b/src/plugins.rs @@ -11,8 +11,12 @@ use serde_json::Value; use sha2::{Digest, Sha256}; use std::collections::{BTreeMap, BTreeSet}; use std::fs; +use std::future::Future; +use std::io::Write; use std::path::{Component, Path, PathBuf}; +use std::time::Duration; use tempfile::{NamedTempFile, TempDir}; +use toml_edit::{DocumentMut, Item, Table, value}; use walkdir::WalkDir; const PLUGIN_LOCK_SCHEMA_VERSION: &str = "v1"; @@ -166,23 +170,8 @@ impl PluginLock { fs::create_dir_all(parent).with_context(|| { format!("failed to create lockfile directory: {}", parent.display()) })?; - let temporary = NamedTempFile::new_in(parent).with_context(|| { - format!( - "failed to create temporary plugin lockfile in {}", - parent.display() - ) - })?; - fs::write(temporary.path(), body).with_context(|| { - format!( - "failed to write temporary plugin lockfile: {}", - temporary.path().display() - ) - })?; - temporary - .persist(path) - .map_err(|error| error.error) - .with_context(|| format!("failed to replace plugin lockfile: {}", path.display()))?; - Ok(()) + write_atomic_file(path, body.as_bytes()) + .with_context(|| format!("failed to replace plugin lockfile: {}", path.display())) } pub fn validate(&self) -> Result<()> { @@ -256,6 +245,7 @@ pub struct PluginApplyResult { } /// Repository-owned plugin operations. +#[derive(Clone)] pub struct PluginManager { project_root: PathBuf, config_path: PathBuf, @@ -299,6 +289,7 @@ impl PluginManager { .map(PluginSelection::key) .collect(); let mut result = PluginApplyResult::default(); + let mut pending_skills = Vec::new(); for key in selections { let locked = lock @@ -350,27 +341,51 @@ impl PluginManager { } } - if dry_run { - result.updated += discovered.skills.len(); - result.skipped += usize::from(discovered.skills.is_empty()); - } else { - for skill in &discovered.skills { - let locked_skill = locked - .skills - .iter() - .find(|candidate| candidate.id == skill.id) - .with_context(|| { - format!("skill missing from plugin lock: {key}/{}", skill.id) - })?; - ensure!( - locked_skill.content_sha256 == skill.content_sha256, - "skill content drift detected for {key}/{}", - skill.id - ); - materialize_skill(&self.project_root, locked, skill, &mut result)?; + for skill in &discovered.skills { + let locked_skill = locked + .skills + .iter() + .find(|candidate| candidate.id == skill.id) + .with_context(|| { + format!("skill missing from plugin lock: {key}/{}", skill.id) + })?; + ensure!( + locked_skill.content_sha256 == skill.content_sha256, + "skill content drift detected for {key}/{}", + skill.id + ); + validate_skill_for_materialization(&self.project_root, locked, skill)?; + if dry_run { + result.updated += 1; + } else { + pending_skills.push((locked.clone(), skill.clone())); } } - drop(source); + if dry_run && discovered.skills.is_empty() { + result.skipped += 1; + } + } + + if dry_run { + return Ok(result); + } + + let skills = pending_skills + .iter() + .map(|(_, skill)| skill.clone()) + .collect::>(); + let transaction = ApplyTransaction::begin(&self.project_root, &skills)?; + for (locked, skill) in pending_skills { + if let Err(error) = materialize_skill(&self.project_root, &locked, &skill, &mut result) + { + let rollback = transaction.rollback(); + return match rollback { + Ok(()) => Err(error), + Err(rollback_error) => Err(anyhow::anyhow!( + "plugin apply failed: {error}; rollback failed: {rollback_error}" + )), + }; + } } Ok(result) @@ -378,11 +393,23 @@ impl PluginManager { /// Resolve and lock a selected plugin from the configured marketplace. pub fn add(&self, selection: &PluginSelection) -> Result { - self.lock_selection(selection) + let manager = self.clone(); + let selection = selection.clone(); + run_async(move || async move { manager.add_async(&selection).await }) } pub fn update(&self, selection: &PluginSelection) -> Result { - self.lock_selection(selection) + let manager = self.clone(); + let selection = selection.clone(); + run_async(move || async move { manager.update_async(&selection).await }) + } + + pub async fn add_async(&self, selection: &PluginSelection) -> Result { + self.lock_selection(selection).await + } + + pub async fn update_async(&self, selection: &PluginSelection) -> Result { + self.lock_selection(selection).await } pub fn load_lock(&self) -> Result { @@ -521,7 +548,7 @@ impl PluginManager { Ok(result) } - fn lock_selection(&self, selection: &PluginSelection) -> Result { + async fn lock_selection(&self, selection: &PluginSelection) -> Result { validate_selection(selection)?; ensure!( self.config.enabled, @@ -532,7 +559,7 @@ impl PluginManager { .marketplaces .get(&selection.marketplace) .with_context(|| format!("unknown plugin marketplace: {}", selection.marketplace))?; - let source = resolve_marketplace_source(&self.config_path, marketplace, true)?; + let source = resolve_marketplace_source_async(&self.config_path, marketplace, true).await?; let discovered = discover_plugin(source.root(), &selection.marketplace, &selection.plugin)?; ensure!( discovered.unsupported_components.is_empty(), @@ -679,7 +706,7 @@ impl PluginManager { "local plugin source drift detected: {}", root.display() ); - Ok(ResolvedSource { root, temp: None }) + Ok(ResolvedSource { root }) } LockedSourceKind::Git => { let root = self.git_source_cache_path(source)?; @@ -688,7 +715,7 @@ impl PluginManager { "offline Git plugin source snapshot is unavailable: {} (run `agentsync plugin update` first)", root.display() ); - Ok(ResolvedSource { root, temp: None }) + Ok(ResolvedSource { root }) } } } @@ -696,7 +723,6 @@ impl PluginManager { struct ResolvedSource { root: PathBuf, - temp: Option, } impl ResolvedSource { @@ -705,13 +731,7 @@ impl ResolvedSource { } } -impl Drop for ResolvedSource { - fn drop(&mut self) { - let _ = self.temp.take(); - } -} - -#[derive(Debug)] +#[derive(Debug, Clone)] struct DiscoveredSkill { id: String, path: PathBuf, @@ -719,6 +739,93 @@ struct DiscoveredSkill { content_sha256: String, } +struct ApplyTransaction { + target_root: PathBuf, + registry_path: PathBuf, + target_root_existed: bool, + original_registry: Option>, + _backup: TempDir, + snapshots: Vec<(PathBuf, Option)>, +} + +impl ApplyTransaction { + fn begin(project_root: &Path, skills: &[DiscoveredSkill]) -> Result { + let target_root = project_root.join(".agents/skills"); + let target_root_metadata = fs::symlink_metadata(&target_root).ok(); + let target_root_existed = target_root_metadata.is_some(); + if let Some(metadata) = &target_root_metadata { + ensure!( + metadata.is_dir() && !metadata.file_type().is_symlink(), + "refusing to materialize skills through a symlinked root: {}", + target_root.display() + ); + } + let registry_path = target_root.join("registry.json"); + let original_registry = if registry_path.is_file() { + Some(fs::read(®istry_path).with_context(|| { + format!("failed to read skill registry: {}", registry_path.display()) + })?) + } else { + None + }; + let backup = TempDir::new().context("failed to create plugin apply rollback directory")?; + let mut snapshots = Vec::new(); + let mut skill_ids = BTreeSet::new(); + for skill in skills { + if !skill_ids.insert(&skill.id) { + continue; + } + let target = target_root.join(&skill.id); + let metadata = fs::symlink_metadata(&target).ok(); + let backup_path = if let Some(metadata) = metadata { + ensure!( + metadata.is_dir() && !metadata.file_type().is_symlink(), + "refusing to snapshot unsafe skill destination: {}", + target.display() + ); + let backup_path = backup.path().join(&skill.id); + copy_directory_without_symlinks(&target, &backup_path)?; + Some(backup_path) + } else { + None + }; + snapshots.push((target, backup_path)); + } + Ok(Self { + target_root, + registry_path, + target_root_existed, + original_registry, + _backup: backup, + snapshots, + }) + } + + fn rollback(&self) -> Result<()> { + for (target, backup_path) in self.snapshots.iter().rev() { + if fs::symlink_metadata(target).is_ok() { + remove_path_safely(target)?; + } + if let Some(backup_path) = backup_path { + copy_directory_without_symlinks(backup_path, target)?; + } + } + if let Some(original_registry) = &self.original_registry { + write_atomic_file(&self.registry_path, original_registry)?; + } else if fs::symlink_metadata(&self.registry_path).is_ok() { + remove_path_safely(&self.registry_path)?; + } + if !self.target_root_existed + && fs::symlink_metadata(&self.target_root) + .is_ok_and(|metadata| metadata.is_dir() && !metadata.file_type().is_symlink()) + && fs::read_dir(&self.target_root)?.next().is_none() + { + fs::remove_dir(&self.target_root)?; + } + Ok(()) + } +} + #[derive(Debug)] struct DiscoveredPlugin { version: Option, @@ -801,10 +908,23 @@ impl Drop for ResolvedMarketplaceSource { } } +#[cfg(test)] fn resolve_marketplace_source( config_path: &Path, marketplace: &MarketplaceConfig, allow_network: bool, +) -> Result { + let config_path = config_path.to_path_buf(); + let marketplace = marketplace.clone(); + run_async(move || async move { + resolve_marketplace_source_async(&config_path, &marketplace, allow_network).await + }) +} + +async fn resolve_marketplace_source_async( + config_path: &Path, + marketplace: &MarketplaceConfig, + allow_network: bool, ) -> Result { let source = marketplace.source.trim(); ensure!( @@ -848,9 +968,11 @@ fn resolve_marketplace_source( .as_deref() .filter(|reference| !reference.trim().is_empty()) .ok_or_else(|| anyhow::anyhow!("Git plugin marketplace requires a reference"))?; - let revision = resolve_git_reference(source, reference)?; + let revision = resolve_git_reference(source, reference).await?; let archive = github_archive_url(source, &revision)?; - let temp = blocking_fetch_archive(&archive)?; + let temp = crate::skills::install::fetch_and_unpack_to_tempdir(&archive) + .await + .context("failed to fetch plugin marketplace archive")?; Ok(ResolvedMarketplaceSource { root: temp.path().to_path_buf(), locked_source: LockedSource { @@ -918,6 +1040,13 @@ fn discover_plugin(root: &Path, marketplace: &str, plugin_name: &str) -> Result< .ok_or_else(|| anyhow::anyhow!("plugin entry has no local source: {plugin_name}"))?; let relative_plugin_path = normalize_relative_path(&source_path)?; let plugin_root = root.join(&relative_plugin_path); + let plugin_metadata = fs::symlink_metadata(&plugin_root) + .with_context(|| format!("plugin source is not accessible: {}", plugin_root.display()))?; + ensure!( + !plugin_metadata.file_type().is_symlink(), + "plugin source must not be a symlink: {}", + plugin_root.display() + ); ensure!( plugin_root.is_dir(), "plugin source is not a directory: {}", @@ -1072,6 +1201,48 @@ fn parse_plugin_source(value: &Value) -> Result> { Ok(None) } +fn validate_skill_for_materialization( + project_root: &Path, + locked: &LockedPlugin, + skill: &DiscoveredSkill, +) -> Result<()> { + crate::skills::manifest::parse_skill_manifest(&skill.path.join("SKILL.md")) + .map_err(|error| anyhow::anyhow!(error.to_string()))?; + let target_root = project_root.join(".agents/skills"); + let Some(root_metadata) = fs::symlink_metadata(&target_root).ok() else { + return Ok(()); + }; + ensure!( + root_metadata.is_dir() && !root_metadata.file_type().is_symlink(), + "refusing to materialize skills through a symlinked root: {}", + target_root.display() + ); + let target = target_root.join(&skill.id); + let Some(metadata) = fs::symlink_metadata(&target).ok() else { + return Ok(()); + }; + ensure!( + metadata.is_dir() && !metadata.file_type().is_symlink(), + "refusing to replace unsafe skill destination: {}", + target.display() + ); + if hash_tree(&target)? == skill.content_sha256 { + return Ok(()); + } + let registry_path = target_root.join("registry.json"); + let owned = crate::skills::registry::read_registry(®istry_path) + .ok() + .and_then(|registry| registry.skills) + .and_then(|skills| skills.get(&skill.id).cloned()) + .is_some_and(|entry| entry_is_owned_by(&entry, locked)); + ensure!( + owned, + "skill collision with unmanaged content: {}", + target.display() + ); + Ok(()) +} + fn materialize_skill( project_root: &Path, locked: &LockedPlugin, @@ -1297,6 +1468,27 @@ fn rollback_plugin_lock(path: &Path, previous: Option<&PluginLock>) -> Result<() } } +fn run_async(factory: F) -> Result +where + F: FnOnce() -> Fut + Send + 'static, + Fut: Future> + Send + 'static, + T: Send + 'static, +{ + if tokio::runtime::Handle::try_current().is_ok() { + std::thread::spawn(move || { + let runtime = tokio::runtime::Runtime::new() + .context("failed to create runtime for plugin operation")?; + runtime.block_on(factory()) + }) + .join() + .map_err(|_| anyhow::anyhow!("plugin operation runtime thread panicked"))? + } else { + let runtime = tokio::runtime::Runtime::new() + .context("failed to create runtime for plugin operation")?; + runtime.block_on(factory()) + } +} + fn add_selection_to_config(config_path: &Path, selection: &PluginSelection) -> Result<()> { // nosemgrep: rust.actix.path-traversal.tainted-path.tainted-path -- config_path is the discovered project config let content = fs::read_to_string(config_path).with_context(|| { @@ -1305,33 +1497,31 @@ fn add_selection_to_config(config_path: &Path, selection: &PluginSelection) -> R config_path.display() ) })?; - let document: toml::Value = toml::from_str(&content) + let mut document: DocumentMut = content + .parse() .with_context(|| format!("failed to parse plugin config: {}", config_path.display()))?; - let already_selected = document - .get("plugins") - .and_then(|plugins| plugins.get("selections")) - .and_then(toml::Value::as_array) - .is_some_and(|selections| { - selections.iter().any(|value| { - value.get("marketplace").and_then(toml::Value::as_str) - == Some(selection.marketplace.as_str()) - && value.get("plugin").and_then(toml::Value::as_str) - == Some(selection.plugin.as_str()) - }) - }); - if already_selected { + let plugins = document["plugins"] + .or_insert(Item::Table(Table::new())) + .as_table_mut() + .ok_or_else(|| anyhow::anyhow!("plugin config [plugins] must be a table"))?; + let selections = plugins + .entry("selections") + .or_insert(Item::ArrayOfTables(toml_edit::ArrayOfTables::new())) + .as_array_of_tables_mut() + .ok_or_else(|| { + anyhow::anyhow!("plugin config [[plugins.selections]] must be an array of tables") + })?; + if selections.iter().any(|table| { + table.get("marketplace").and_then(Item::as_str) == Some(selection.marketplace.as_str()) + && table.get("plugin").and_then(Item::as_str) == Some(selection.plugin.as_str()) + }) { return Ok(()); } - let separator = if content.ends_with('\n') { - "\n" - } else { - "\n\n" - }; - let body = format!( - "{}{separator}[[plugins.selections]]\nmarketplace = {:?}\nplugin = {:?}\n", - content, selection.marketplace, selection.plugin - ); - write_atomic_file(config_path, body.as_bytes()) + let mut entry = Table::new(); + entry["marketplace"] = value(selection.marketplace.clone()); + entry["plugin"] = value(selection.plugin.clone()); + selections.push(entry); + write_atomic_file(config_path, document.to_string().as_bytes()) } fn remove_plugin_owner_entries_atomic( @@ -1397,72 +1587,42 @@ fn remove_selection_from_config(config_path: &Path, selection: &PluginSelection) config_path.display() ) })?; - let lines: Vec<&str> = content.lines().collect(); - let mut output = Vec::with_capacity(lines.len()); - let mut index = 0; - let mut removed = false; - - while index < lines.len() { - if lines[index].trim() != "[[plugins.selections]]" { - output.push(lines[index]); - index += 1; - continue; - } - - let start = index; - index += 1; - while index < lines.len() - && !lines[index] - .trim_start() - .starts_with("[[plugins.selections]]") - && !lines[index].trim_start().starts_with('[') - { - index += 1; - } - let block = lines[start..index].join("\n"); - let value: toml::Value = toml::from_str(&block).with_context(|| { - format!( - "invalid plugin selection block in {}", - config_path.display() - ) - })?; - let matches = value - .get("plugins") - .and_then(|plugins| plugins.get("selections")) - .and_then(toml::Value::as_array) - .and_then(|selections| selections.first()) - .is_some_and(|selection_value| { - selection_value - .get("marketplace") - .and_then(toml::Value::as_str) - == Some(selection.marketplace.as_str()) - && selection_value.get("plugin").and_then(toml::Value::as_str) - == Some(selection.plugin.as_str()) - }); - if matches { - removed = true; - } else { - output.extend(lines[start..index].iter().copied()); - } - } - - if !removed { - return Ok(()); - } - let body = format!("{}\n", output.join("\n")); - write_atomic_file(config_path, body.as_bytes()) + let mut document: DocumentMut = content + .parse() + .with_context(|| format!("failed to parse plugin config: {}", config_path.display()))?; + let selections = document + .get_mut("plugins") + .and_then(Item::as_table_mut) + .and_then(|plugins| plugins.get_mut("selections")) + .and_then(Item::as_array_of_tables_mut) + .ok_or_else(|| anyhow::anyhow!("plugin selection not found: {}", selection.key()))?; + let index = selections.iter().position(|table| { + table.get("marketplace").and_then(Item::as_str) == Some(selection.marketplace.as_str()) + && table.get("plugin").and_then(Item::as_str) == Some(selection.plugin.as_str()) + }); + let Some(index) = index else { + bail!("plugin selection not found: {}", selection.key()); + }; + selections.remove(index); + write_atomic_file(config_path, document.to_string().as_bytes()) } fn write_atomic_file(path: &Path, body: &[u8]) -> Result<()> { let parent = path.parent().unwrap_or_else(|| Path::new(".")); - let temporary = NamedTempFile::new_in(parent) + let mut temporary = NamedTempFile::new_in(parent) .with_context(|| format!("failed to create temporary file in {}", parent.display()))?; - fs::write(temporary.path(), body).with_context(|| { + temporary.as_file_mut().write_all(body).with_context(|| { format!( "failed to write temporary file: {}", temporary.path().display() ) })?; + temporary.as_file_mut().flush().with_context(|| { + format!( + "failed to flush temporary file: {}", + temporary.path().display() + ) + })?; temporary .persist(path) .map_err(|error| error.error) @@ -1591,13 +1751,13 @@ fn hash_tree(root: &Path) -> Result { .strip_prefix(root)? .to_string_lossy() .replace('\\', "/"); - let bytes = fs::read(entry.path())?; - entries.push((relative, bytes)); + entries.push(relative); } } - entries.sort_by(|left, right| left.0.cmp(&right.0)); + entries.sort(); let mut hasher = Sha256::new(); - for (path, bytes) in entries { + for path in entries { + let bytes = fs::read(root.join(&path))?; hasher.update(path.as_bytes()); hasher.update([0]); hasher.update((bytes.len() as u64).to_be_bytes()); @@ -1614,7 +1774,7 @@ fn format_digest(digest: impl AsRef<[u8]>) -> String { .collect() } -fn resolve_git_reference(repository: &str, reference: &str) -> Result { +async fn resolve_git_reference(repository: &str, reference: &str) -> Result { ensure!( !reference.trim().eq_ignore_ascii_case("HEAD"), "Git reference HEAD is not allowed; use a branch, tag, or full commit SHA" @@ -1629,7 +1789,8 @@ fn resolve_git_reference(repository: &str, reference: &str) -> Result { github.1, urlencoding::encode(reference) ); - let client = reqwest::blocking::Client::builder() + let client = reqwest::Client::builder() + .timeout(Duration::from_secs(30)) .user_agent("agentsync-plugin-resolver") .build() .context("failed to create GitHub API client")?; @@ -1639,10 +1800,12 @@ fn resolve_git_reference(repository: &str, reference: &str) -> Result { } let response: Value = request .send() + .await .context("failed to resolve Git reference")? .error_for_status() .context("Git reference resolution failed")? .json() + .await .context("invalid GitHub commit response")?; let sha = response .get("sha") @@ -1681,15 +1844,6 @@ fn github_repo_parts(repository: &str) -> Result<(String, String)> { Ok((segments[0].to_string(), repo.to_string())) } -fn blocking_fetch_archive(url: &str) -> Result { - let future = crate::skills::install::fetch_and_unpack_to_tempdir(url); - let result = match tokio::runtime::Handle::try_current() { - Ok(handle) => tokio::task::block_in_place(|| handle.block_on(future)), - Err(_) => tokio::runtime::Runtime::new()?.block_on(future), - }?; - Ok(result) -} - #[cfg(test)] mod tests { use super::*; @@ -1709,18 +1863,27 @@ mod tests { assert_eq!(first, fs::read_to_string(path).unwrap()); } - #[test] - fn mutable_git_reference_resolves_to_a_commit_shape() { + #[tokio::test] + async fn mutable_git_reference_resolves_to_a_commit_shape() { assert_eq!( resolve_git_reference( "https://github.com/example/repo", "0123456789abcdef0123456789abcdef01234567" ) + .await .unwrap(), "0123456789abcdef0123456789abcdef01234567" ); - assert!(resolve_git_reference("https://gitlab.com/example/repo", "main").is_err()); - assert!(resolve_git_reference("https://github.com/example/repo", "HEAD").is_err()); + assert!( + resolve_git_reference("https://gitlab.com/example/repo", "main") + .await + .is_err() + ); + assert!( + resolve_git_reference("https://github.com/example/repo", "HEAD") + .await + .is_err() + ); } #[test] @@ -2128,6 +2291,23 @@ mod tests { assert!(discover_plugin(root, "internal", "traversal").is_err()); assert!(discover_plugin(root, "internal", "file").is_err()); assert!(discover_plugin(root, "internal", "bad-manifest").is_err()); + + #[cfg(unix)] + { + use std::os::unix::fs::symlink; + let real_plugin = root.join("real-plugin"); + fs::create_dir_all(&real_plugin).unwrap(); + symlink(&real_plugin, root.join("symlink-plugin")).unwrap(); + fs::write( + root.join(".agents/plugins/marketplace.json"), + serde_json::to_vec(&serde_json::json!({ + "plugins": [{"name": "symlink", "source": "./symlink-plugin"}] + })) + .unwrap(), + ) + .unwrap(); + assert!(discover_plugin(root, "internal", "symlink").is_err()); + } } #[test] @@ -2279,7 +2459,8 @@ mod tests { marketplace: "missing".to_string(), plugin: "plugin".to_string(), }; - remove_selection_from_config(&config_path, &nonmatching).unwrap(); + let error = remove_selection_from_config(&config_path, &nonmatching).unwrap_err(); + assert!(error.to_string().contains("selection not found")); remove_selection_from_config(&config_path, &selection).unwrap(); assert!( !fs::read_to_string(&config_path) @@ -2294,6 +2475,17 @@ mod tests { let no_newline_config = temp.path().join("no-newline.toml"); fs::write(&no_newline_config, "[plugins]\nenabled = true").unwrap(); add_selection_to_config(&no_newline_config, &selection).unwrap(); + let preserved_config = temp.path().join("preserved-config.toml"); + fs::write( + &preserved_config, + "# keep this comment\n[plugins]\nenabled = true\n\n[[plugins.selections]]\n# keep this entry comment\nmarketplace = \"other\"\nplugin = \"plugin\"\n", + ) + .unwrap(); + add_selection_to_config(&preserved_config, &selection).unwrap(); + let preserved = fs::read_to_string(&preserved_config).unwrap(); + assert!(preserved.contains("# keep this comment")); + assert!(preserved.contains("# keep this entry comment")); + assert_eq!(preserved.matches("[[plugins.selections]]").count(), 2); let rollback_path = temp.path().join("rollback.lock"); rollback_plugin_lock(&rollback_path, None).unwrap(); diff --git a/src/skills/update.rs b/src/skills/update.rs index df910e19..05a6d9b7 100644 --- a/src/skills/update.rs +++ b/src/skills/update.rs @@ -179,6 +179,15 @@ fn install_updated_skill( // Save previous registry entry for rollback let old_registry_entry: Option = read_old_registry_entry(skill_id, registry_path); + if old_registry_entry + .as_ref() + .is_some_and(|entry| entry.plugin_owners.is_some()) + { + rollback_skill_dir(skill_dir, backup_dir); + return Err(SkillUpdateError::Validation( + "plugin-owned skills must be updated with agentsync plugin update".into(), + )); + } let new_entry = crate::skills::registry::SkillEntry { name: Some(manifest.name.clone()), diff --git a/tests/fixtures/plugin-marketplace/plugins/unsafe/hooks/install.sh b/tests/fixtures/plugin-marketplace/plugins/unsafe/hooks/install.sh index dead1194..b9ea406a 100644 --- a/tests/fixtures/plugin-marketplace/plugins/unsafe/hooks/install.sh +++ b/tests/fixtures/plugin-marketplace/plugins/unsafe/hooks/install.sh @@ -1,2 +1,3 @@ #!/bin/sh -printf 'this hook must never run\n' > ../hook-ran.txt +script_dir=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) +printf 'this hook must never run\n' > "$script_dir/../hook-ran.txt" diff --git a/tests/plugins.rs b/tests/plugins.rs index 91e78ca7..b3699da2 100644 --- a/tests/plugins.rs +++ b/tests/plugins.rs @@ -278,6 +278,40 @@ fn plugin_add_rolls_back_a_new_selection_when_materialization_fails() { assert!(!project.path().join(".agents/plugins.lock.toml").exists()); } +#[test] +fn plugin_add_does_not_leave_earlier_skills_after_a_later_failure() { + let (project, _) = setup_project(); + let config_path = project.path().join(".agents/agentsync.toml"); + let source_skill = project + .path() + .join("marketplace/plugins/engineering/skills/later"); + fs::create_dir_all(&source_skill).unwrap(); + fs::write( + source_skill.join("SKILL.md"), + "---\nname: later\nversion: 1.0.0\n---\nlater\n", + ) + .unwrap(); + let unmanaged = project.path().join(".agents/skills/later"); + fs::create_dir_all(&unmanaged).unwrap(); + fs::write(unmanaged.join("SKILL.md"), "unmanaged").unwrap(); + let config = Config::load(&config_path).unwrap(); + let manager = PluginManager::new(project.path().to_path_buf(), config_path, config.plugins); + + let error = manager + .add(&PluginSelection { + marketplace: "internal".to_string(), + plugin: "engineering".to_string(), + }) + .expect_err("later unmanaged collision must fail"); + assert!(error.to_string().contains("collision")); + assert!(!project.path().join(".agents/skills/review").exists()); + assert_eq!( + fs::read_to_string(unmanaged.join("SKILL.md")).unwrap(), + "unmanaged" + ); + assert!(!project.path().join(".agents/plugins.lock.toml").exists()); +} + #[test] fn plugin_add_rolls_back_when_project_config_becomes_invalid() { let (project, mut config) = setup_project(); diff --git a/tests/test_update_security.rs b/tests/test_update_security.rs index 8c13b87e..1644ef86 100644 --- a/tests/test_update_security.rs +++ b/tests/test_update_security.rs @@ -56,3 +56,69 @@ async fn test_update_skill_skips_symlinks() { ); } } + +#[tokio::test] +async fn test_update_skill_rejects_plugin_owned_skills() { + let temp_dir = TempDir::new().unwrap(); + let target_root = temp_dir.path().join(".agents/skills"); + fs::create_dir_all(&target_root).unwrap(); + let skill_id = "plugin-skill"; + let skill_dir = target_root.join(skill_id); + fs::create_dir_all(&skill_dir).unwrap(); + fs::write( + skill_dir.join("SKILL.md"), + "---\nname: plugin-skill\nversion: 1.0.0\n---\noriginal\n", + ) + .unwrap(); + fs::write( + target_root.join("registry.json"), + r#"{ + "schemaVersion": 1, + "last_updated": null, + "skills": { + "plugin-skill": { + "name": "plugin-skill", + "version": "1.0.0", + "description": null, + "provider": "plugin/internal/example", + "source": "../marketplace", + "installedAt": null, + "files": null, + "manifestHash": null, + "marketplace": "internal", + "plugin": "example", + "pluginRevision": "local:revision", + "contentSha256": null, + "pluginOwners": [{ + "marketplace": "internal", + "plugin": "example", + "revision": "local:revision" + }] + } + } +}"#, + ) + .unwrap(); + let update_source = temp_dir.path().join("update"); + fs::create_dir_all(&update_source).unwrap(); + fs::write( + update_source.join("SKILL.md"), + "---\nname: plugin-skill\nversion: 2.0.0\n---\nreplacement\n", + ) + .unwrap(); + + let error = update_skill_async(skill_id, &target_root, &update_source) + .await + .expect_err("plugin-owned skill updates must use the plugin command"); + assert!(error.to_string().contains("agentsync plugin update")); + assert!( + fs::read_to_string(skill_dir.join("SKILL.md")) + .unwrap() + .contains("original") + ); + assert!( + fs::read_to_string(target_root.join("registry.json")) + .unwrap() + .contains("pluginOwners") + ); +}