diff --git a/md/design/common-issues.md b/md/design/common-issues.md index 1e733238..a5f72630 100644 --- a/md/design/common-issues.md +++ b/md/design/common-issues.md @@ -54,6 +54,10 @@ And when two walks over one tree feed each other — a copy, and the comparison Plugin content must not decide where Symposium writes. A name it supplies, such as a skill's `name`, can resolve outside the directory it is joined onto (`Path::join` does not confine it), so it never becomes part of a path unchecked. Validate it with `skills::is_safe_dir_name`, which also covers Windows-specific path forms, rather than with a check of your own. +## A source that reads as empty is not an empty source + +A listing that swallows read errors returns the same empty list for "nothing there" and "could not look", and anything that removes what a source stopped offering turns the second into data loss: a `[[registry]]` path on an unmounted share would make `sync` delete every skill installed from it. So `PackageManager::registry_plugins` fails when a registry cannot be read, `PluginRegistry::unreadable_registries` carries that to `sync`, and `sync` skips its stale-skill cleanup while that list is not empty. Any new pass that removes things because a source stopped offering them belongs behind the same condition. The opposite mistake is as bad: a source that is legitimately absent must not count as unreadable, or cleanup never runs again, which is why the built-in `~/.symposium/plugins/` is left out when nothing exists at that path. + ## Windows portability (tests) The test suite runs on `windows-latest`, where CI passes `--no-fail-fast` so that one failing test binary does not hide the failures in every binary cargo would otherwise skip. A few patterns recur when writing tests that touch paths or scripts: diff --git a/md/design/module-structure.md b/md/design/module-structure.md index f81d20da..6ec5a8a9 100644 --- a/md/design/module-structure.md +++ b/md/design/module-structure.md @@ -42,6 +42,8 @@ Implements `cargo agents init`. Prompts for agents (or accepts `--add-agent`/`-- Implements `cargo agents sync`. Scans workspace dependencies, finds applicable skills from plugin sources, and synchronizes them into each configured agent's skill directory. The core primitive is `sync_skill_dir(source_dir, dest_dir, project_root)`. It copies the entire source directory (not just `SKILL.md`) and is change-aware: it compares source and destination content, only performing the delete-and-recopy when files actually differ, so the disk shows no modifications when nothing changed. Copying and comparing share one traversal, `walk_tree` — they have to agree entry for entry, since a comparison scan that omitted something the copier writes would see a difference on every run and turn each sync into a full recopy. That traversal **follows symlinks** (see [`dir_walk.rs`](#dir_walkrs--following-symlinks-without-looping)), so a linked file installs as a real file and a linked directory as its contents; a broken link and a link back into the tree being copied are reported and skipped rather than failing the sync. A configurable debounce (`sync-debounce-secs`, default 5s, keyed on the `.symposium` marker's mtime) skips even the comparison for recently-synced skills. On each sync, scans every agent's skills parent directory and reaps any marker-bearing subdirectory it didn't install this time, leaving user-managed skills (which lack the marker) untouched. Writes a `.gitignore` with `*` only into individual skill directories (not parent directories like `.claude/` or `.claude/skills/`). Also provides `register_hooks()` for use by `init`, which registers only symposium's own global hook handler — individual plugin hooks are never written into agent configs. +While `PluginRegistry::unreadable_registries` is not empty, `sync` skips that reap and warns once per registry: an unreadable registry offered nothing, so everything it installed would look stale. + One entry point, `sync(sym, deps, update)`. The caller supplies the `WorkspaceDeps`, so the hook pipeline hands over the resolver its other stages already used rather than resolving the workspace a second time. Callers: the CLI's `sync` arm (after the consent prompt), `use_command` once an enablement is recorded, and the hook pipeline's auto-sync. `sync` takes an `UpdateLevel` that it threads into skill resolution (`skills::collect_skills`), controlling how aggressively `source.git` skill groups are re-fetched. Callers choose: the auto-sync path passes `Check` on `SessionStart` (refresh) and `None` otherwise (debounced); the binary's global `--update` flag feeds manual `cargo agents sync`. @@ -50,6 +52,8 @@ One entry point, `sync(sym, deps, update)`. The caller supplies the `WorkspaceDe Loads plugin manifests from the configured registries and validates them into `Plugin`s. Loading goes through the [package-manager layer](#pm--package-managers): `load_registry` asks each **trusted** `PmRegistry` instance (the configured registries — the cargo transport is not a trust root) for its plugins via `active_plugins`. A `PathPm` returns each entry through `entry_plugin`: a `Symposium.toml` manifest read as-is, or a bare `SKILL.md` synthesized into a manifest (`standalone_skill_manifest`); dependency-embedded crate plugins never load here. Refreshing a git registry's content is the `GitPm`'s `refresh` operation, driven by `ensure_registries` (startup) and `sync_registries` (`plugin sync`). `scan_source_dir` remains as the offline form used by the `plugin validate` CLI, which points at an arbitrary directory rather than a configured registry; it walks the same [layout](#pm--package-managers) rules and synthesizes bare skills the same way. +`load_registry` reads each registry through `PmInstance::registry_plugins`, which fails where `active_plugins` would come back empty because the source cannot be read (a `PathPm` whose directory cannot be listed or is itself an entry, and a `GitPm` that has not been fetched). Such a registry is reported and recorded in `PluginRegistry::unreadable_registries` (see [common issues](./common-issues.md#a-source-that-reads-as-empty-is-not-an-empty-source)); `plugin list` and `plugin show` read through the same call. + Validation here turns the raw TOML (parsed into the SDK's `RawPluginManifest`) into: - `Installation` entries (optional `source`, optional `executable`/`script`, optional `args`, plus `requirements` and `install_commands`) collected on `PluginManifest.installations`. Inline installation references on hooks or other installations are *promoted* into synthetic `Installation` entries with derived names (`` for an inline `command`, `__req_` for an inline requirement), so all references in the validated form are plain names. - `Hook` entries with `command: String` (the name of an `Installation`) plus optional hook-level `executable` / `script` / `args`. Validation guarantees at most one of `executable`/`script` is set across hook + installation, and at most one layer sets `args`. diff --git a/md/design/sync-agent-flow.md b/md/design/sync-agent-flow.md index fcc74d6c..69f95c79 100644 --- a/md/design/sync-agent-flow.md +++ b/md/design/sync-agent-flow.md @@ -23,6 +23,8 @@ Scans workspace dependencies, installs applicable skills into agent directories, 7. **Reap stale skills** — across every known agent's skills parent directory, remove any subdirectory that contains the `.symposium` marker but wasn't installed this sync. Directories without the marker (user-managed) are left untouched. + While a registry cannot be read, this step is skipped and a warning names the registry: it offered nothing this time, so everything it installed would look stale. + 8. **Register hooks** — ensure symposium's global hook handler and MCP servers are registered for all configured agents. Unregister hooks for agents no longer in the config. Only symposium's own handler is registered (e.g., `cargo-agents hook claude pre-tool-use`) — individual plugin hooks are never written into agent configs. See [Hooks](./hooks.md) for the dispatch model. ## Marker file diff --git a/md/reference/cargo-agents-sync.md b/md/reference/cargo-agents-sync.md index 4586299c..d968eb2a 100644 --- a/md/reference/cargo-agents-sync.md +++ b/md/reference/cargo-agents-sync.md @@ -24,7 +24,7 @@ Must be run from within a Rust workspace. Performs the following steps: 5. **Mirror workspace skills**: if `agents-syncing` is enabled (default), user-authored skills in `/.agents/skills/` are propagated into the skill directories of any configured agent that doesn't natively use `.agents/skills/` (e.g., `.claude/skills/`, `.kiro/skills/`). See [Workspace skills](../workspace-skills.md). -6. **Clean up stale skills**: scans every agent's skills parent directory and removes any subdirectory containing the `.symposium` marker that wasn't installed (or propagated) this sync. Directories without the marker (user-managed) are left untouched. +6. **Clean up stale skills**: scans every agent's skills parent directory and removes any subdirectory containing the `.symposium` marker that wasn't installed (or propagated) this sync. Directories without the marker (user-managed) are left untouched. If a registry cannot be read (a `path` that is missing, unreadable or itself a plugin, or a `git` registry that has not been fetched yet), sync warns, naming the registry, and skips this step: nothing is removed until it can be read again. 7. **Register hooks**: ensures hooks and MCP servers are registered for all configured agents, at the scope set by `hook-scope`: the user's home directory for `"global"` (the default), the project for `"project"` (MCP servers fall back to the user level for agents without a project-level file). Unregisters hooks for agents no longer in the config. diff --git a/src/config.rs b/src/config.rs index 379cca6f..2ef6f30c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -623,7 +623,9 @@ impl Symposium { }); } - if self.config.defaults.user_plugins { + if self.config.defaults.user_plugins + && fs::symlink_metadata(self.dirs.config_dir.join("plugins")).is_ok() + { configs.push(RegistryConfig { name: "user-plugins".to_string(), git: None, diff --git a/src/help_render.rs b/src/help_render.rs index c60aa6d7..46629ba8 100644 --- a/src/help_render.rs +++ b/src/help_render.rs @@ -251,6 +251,7 @@ mod tests { PluginRegistry { plugins, warnings: vec![], + unreadable_registries: vec![], custom_predicates: crate::plugins::CustomPredicateRegistry::default(), } } diff --git a/src/plugins.rs b/src/plugins.rs index 48d847bf..69e88bfc 100644 --- a/src/plugins.rs +++ b/src/plugins.rs @@ -767,6 +767,7 @@ pub struct PluginRegistry { pub plugins: Vec, /// Non-fatal load warnings for entries that were skipped. pub warnings: Vec, + pub unreadable_registries: Vec, /// Global custom predicate registry. Built from all plugins' `custom_predicates`. pub custom_predicates: CustomPredicateRegistry, } @@ -869,7 +870,7 @@ pub async fn list_plugins(sym: &Symposium) -> Vec { // Trusted instances only: registry entries, not dependency-embedded crates. for inst in pms.instances().filter(|i| i.trusted) { - for p in inst.active_plugins(&[]).await { + for p in inst.registry_plugins().await.unwrap_or_default() { by_registry .entry(inst.name.clone()) .or_default() @@ -908,7 +909,7 @@ pub async fn list_plugins(sym: &Symposium) -> Vec { pub async fn find_plugin(sym: &Symposium, name: &str) -> Option { let pms = sym.detached_managers(); for inst in pms.instances().filter(|i| i.trusted) { - for parsed in inst.active_plugins(&[]).await { + for parsed in inst.registry_plugins().await.unwrap_or_default() { if parsed.manifest.name == name { return Some(parsed); } @@ -1088,13 +1089,17 @@ async fn load_registry_impl( let pms = sym.detached_managers(); let mut plugins = Vec::new(); let mut warnings = Vec::new(); + let mut unreadable_registries = Vec::new(); // Trust roots only: the configured registries (and, below, the workspace). // Dependency-embedded crate plugins are not trust roots — they reach the // active set through discovery / consent and the driver's `load_plugin`, // never here. Each registry instance logs its own load failures. for inst in pms.instances().filter(|i| i.trusted) { - plugins.extend(inst.active_plugins(&[]).await); + match inst.registry_plugins().await { + Some(found) => plugins.extend(found), + None => unreadable_registries.push(inst.name.clone()), + } } if let Some(ws) = workspace { @@ -1111,6 +1116,7 @@ async fn load_registry_impl( PluginRegistry { plugins, warnings, + unreadable_registries, custom_predicates, } } diff --git a/src/pm/git.rs b/src/pm/git.rs index 49bd0050..6d5188e7 100644 --- a/src/pm/git.rs +++ b/src/pm/git.rs @@ -64,6 +64,16 @@ impl PackageManager for GitPm { self.inner.active_plugins(deps).await } + async fn registry_plugins(&self) -> Result> { + let name = &self.name; + if !self.inner.dir.exists() { + anyhow::bail!( + "registry `{name}` has not been fetched yet; run `cargo agents plugin sync {name}`" + ); + } + self.inner.registry_plugins().await + } + async fn load_plugin(&self, id: &PackageId) -> Vec { self.inner.load_plugin(id).await } diff --git a/src/pm/mod.rs b/src/pm/mod.rs index bb515111..68b64e21 100644 --- a/src/pm/mod.rs +++ b/src/pm/mod.rs @@ -87,6 +87,12 @@ pub trait PackageManager { /// plugin is *trusted* is the caller's decision — see [`PmInstance::trusted`]. async fn active_plugins(&self, deps: &[PackageId]) -> Vec; + /// Fails instead of returning an empty list when the source cannot be + /// read, since sync removes what a registry stops offering. + async fn registry_plugins(&self) -> Result> { + Ok(self.active_plugins(&[]).await) + } + /// The plugin(s) a specific id maps to — zero, one, or many. Used for /// `[[plugins]]` chained references and explicitly enabled crates. async fn load_plugin(&self, id: &PackageId) -> Vec; @@ -169,6 +175,20 @@ impl PmInstance { self.validate_all(self.pm.active_plugins(deps).await) } + pub async fn registry_plugins(&self) -> Option> { + match self.pm.registry_plugins().await { + Ok(plugins) => Some(self.validate_all(plugins)), + Err(e) => { + tracing::warn!( + report = %crate::report::ReportEvent::Warning { + message: format!("{e:#}"), + }, + ); + None + } + } + } + /// The plugin(s) an id maps to, validated. pub async fn load_plugin(&self, id: &PackageId) -> Vec { self.validate_all(self.pm.load_plugin(id).await) @@ -365,6 +385,18 @@ mod tests { assert!(reports.is_empty(), "{reports:?}"); } + #[tokio::test] + async fn git_registry_without_its_copy_is_unreadable() { + let tmp = tempfile::tempdir().unwrap(); + let ctx = symposium_install::InstallContext::new(tmp.path().to_path_buf()); + let pm = GitPm::new("reg", "https://github.com/example/reg", false, ctx).unwrap(); + let err = pm.registry_plugins().await.unwrap_err(); + assert!( + err.to_string().contains("cargo agents plugin sync reg"), + "{err}" + ); + } + #[tokio::test] async fn registry_rejects_unknown_pm() { let tmp = tempfile::tempdir().unwrap(); diff --git a/src/pm/path.rs b/src/pm/path.rs index 7c16380e..76f7cfb5 100644 --- a/src/pm/path.rs +++ b/src/pm/path.rs @@ -14,7 +14,7 @@ use std::path::{Path, PathBuf}; -use anyhow::Result; +use anyhow::{Context, Result}; use symposium_install::UpdateLevel; use super::{ @@ -27,7 +27,7 @@ use crate::report::ReportEvent; /// collection of plugin entries. pub struct PathPm { name: String, - dir: PathBuf, + pub(super) dir: PathBuf, } impl PathPm { @@ -51,13 +51,22 @@ impl PackageManager for PathPm { /// plugin applies is decided later by its own predicates. A registry is a /// trust root, so these activate without consent. async fn active_plugins(&self, _deps: &[PackageId]) -> Vec { - let entries = match layout::enumerate(&self.dir) { - Ok(e) => e, - Err(e) => { - tracing::warn!(registry = %self.name, error = %e, "cannot list registry"); - return Vec::new(); - } - }; + self.registry_plugins().await.unwrap_or_else(|e| { + tracing::warn!(error = %format!("{e:#}"), "cannot list registry"); + Vec::new() + }) + } + + async fn registry_plugins(&self) -> Result> { + std::fs::read_dir(&self.dir).with_context(|| { + format!( + "cannot read registry `{}` at {}", + self.name, + crate::output::display_path(&self.dir) + ) + })?; + let entries = layout::enumerate(&self.dir) + .with_context(|| format!("cannot list registry `{}`", self.name))?; let mut out = Vec::new(); for entry in entries { match crate::plugins::entry_plugin(&self.dir, &entry.subpath, &self.name) { @@ -73,7 +82,7 @@ impl PackageManager for PathPm { None => {} } } - out + Ok(out) } /// The entry an id names (`id.name` is the entry's subpath key). diff --git a/src/skills.rs b/src/skills.rs index 6520aea4..19bbcd04 100644 --- a/src/skills.rs +++ b/src/skills.rs @@ -1169,6 +1169,7 @@ mod tests { workspace_member: false, }], warnings: vec![], + unreadable_registries: vec![], custom_predicates: crate::plugins::CustomPredicateRegistry::default(), }; @@ -1228,6 +1229,7 @@ mod tests { workspace_member: false, }], warnings: vec![], + unreadable_registries: vec![], custom_predicates: crate::plugins::CustomPredicateRegistry::default(), }; @@ -1305,6 +1307,7 @@ mod tests { workspace_member: false, }], warnings: vec![], + unreadable_registries: vec![], custom_predicates: crate::plugins::CustomPredicateRegistry::default(), }; @@ -1387,6 +1390,7 @@ mod tests { workspace_member: false, }], warnings: vec![], + unreadable_registries: vec![], custom_predicates: crate::plugins::CustomPredicateRegistry::default(), }; @@ -1470,6 +1474,7 @@ mod tests { workspace_member: false, }], warnings: vec![], + unreadable_registries: vec![], custom_predicates: crate::plugins::CustomPredicateRegistry::default(), }; @@ -1598,6 +1603,7 @@ mod tests { workspace_member: false, }], warnings: vec![], + unreadable_registries: vec![], custom_predicates: crate::plugins::CustomPredicateRegistry::default(), }; diff --git a/src/subcommand_dispatch.rs b/src/subcommand_dispatch.rs index 1515253c..82667a3a 100644 --- a/src/subcommand_dispatch.rs +++ b/src/subcommand_dispatch.rs @@ -232,6 +232,7 @@ mod tests { PluginRegistry { plugins, warnings: vec![], + unreadable_registries: vec![], custom_predicates: crate::plugins::CustomPredicateRegistry::default(), } } diff --git a/src/sync.rs b/src/sync.rs index 71fd434b..e9992c94 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -588,10 +588,11 @@ pub async fn sync(sym: &Symposium, deps: &Arc, update: UpdateLeve // Stale-skill cleanup: scan every agent's skills parent directory (across // all known agents, so we also clean up after agents removed from config) // and remove subdirs containing the marker that we didn't just install. + let skip_cleanup = !registry.unreadable_registries.is_empty(); let mut scanned: BTreeSet = BTreeSet::new(); for &agent in Agent::all() { let parent = skills_parent_dir(agent, &project_root); - if !scanned.insert(parent.clone()) { + if skip_cleanup || !scanned.insert(parent.clone()) { continue; } let Ok(entries) = fs::read_dir(&parent) else { @@ -623,6 +624,15 @@ pub async fn sync(sym: &Symposium, deps: &Arc, update: UpdateLeve } } } + for name in ®istry.unreadable_registries { + tracing::info!( + report = %crate::report::ReportEvent::Warning { + message: format!( + "registry `{name}` could not be read; skipped cleanup of installed skills" + ), + }, + ); + } // Hooks come off at whichever scope they would have been written to; MCP // entries come off at both, since a leftover one keeps pointing the agent diff --git a/tests/fixtures/unreadable-registry0/dot-symposium/config.toml b/tests/fixtures/unreadable-registry0/dot-symposium/config.toml new file mode 100644 index 00000000..16a921ab --- /dev/null +++ b/tests/fixtures/unreadable-registry0/dot-symposium/config.toml @@ -0,0 +1,8 @@ +hook-scope = "project" + +[defaults] +symposium-recommendations = false + +[[registry]] +name = "ext" +path = "$TEST_DIR/ext" diff --git a/tests/fixtures/unreadable-registry0/ext/house-style/SKILL.md b/tests/fixtures/unreadable-registry0/ext/house-style/SKILL.md new file mode 100644 index 00000000..5b5c756b --- /dev/null +++ b/tests/fixtures/unreadable-registry0/ext/house-style/SKILL.md @@ -0,0 +1,7 @@ +--- +name: house-style +description: Style guidance served by a path registry +depends-on: "*" +--- + +Follow the house style. diff --git a/tests/init_sync.rs b/tests/init_sync.rs index 6798c1da..09631546 100644 --- a/tests/init_sync.rs +++ b/tests/init_sync.rs @@ -371,6 +371,61 @@ async fn sync_preserves_user_managed_skills() { .unwrap(); } +#[tokio::test] +async fn sync_keeps_skills_of_a_registry_it_cannot_read() { + let breakages: [fn(&Path); 2] = [ + |ext| std::fs::rename(ext, ext.with_extension("unmounted")).unwrap(), + |ext| std::fs::write(ext.join(symposium::pm::layout::MANIFEST_FILE), "").unwrap(), + ]; + for breakage in breakages { + with_fixture( + TestMode::SimulationOnly, + &["unreadable-registry0", "workspace-empty0"], + async |mut ctx| { + ctx.symposium(&["init", "--add-agent", "claude"]).await?; + ctx.symposium(&["sync"]).await?; + let skills = ctx.workspace_root.as_ref().unwrap().join(".claude/skills"); + assert!(!find_installed_skills(&skills, "house-style").is_empty()); + + breakage(&ctx.tempdir.join("ext")); + let events = ctx.sync_with_report(tracing::Level::INFO).await?; + assert!(!find_installed_skills(&skills, "house-style").is_empty()); + assert!( + events.iter().any(|e| e["kind"] == "warning" + && e["message"] + .as_str() + .is_some_and(|m| m.contains("`ext`") && m.contains("skipped cleanup"))), + "expected a warning naming `ext`, got: {events:?}" + ); + Ok(()) + }, + ) + .await + .unwrap(); + } +} + +#[tokio::test] +async fn sync_cleans_up_without_a_user_plugins_directory() { + with_fixture( + TestMode::SimulationOnly, + &["unreadable-registry0", "workspace-empty0"], + async |mut ctx| { + ctx.symposium(&["init", "--add-agent", "claude"]).await?; + ctx.symposium(&["sync"]).await?; + let skills = ctx.workspace_root.as_ref().unwrap().join(".claude/skills"); + assert!(!find_installed_skills(&skills, "house-style").is_empty()); + + std::fs::remove_dir_all(ctx.tempdir.join("ext/house-style"))?; + ctx.symposium(&["sync"]).await?; + assert!(find_installed_skills(&skills, "house-style").is_empty()); + Ok(()) + }, + ) + .await + .unwrap(); +} + /// Copilot uses vendor-neutral `.agents/skills/` path, not `.claude/skills/`. #[test] fn copilot_uses_vendor_neutral_skill_path() {