From 4b17ed1cd75628fddc459d47029a0401fc85161e Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Fri, 21 Aug 2026 23:42:33 +1000 Subject: [PATCH 1/2] feat(cli): report outdated and unmanaged Skills --- GLOSSARY.md | 11 + crates/skilld-command/src/lib.rs | 151 ++++++++- crates/skilld-command/src/local_store.rs | 2 +- crates/skilld-command/src/outdated.rs | 125 ++++++++ crates/skilld-command/tests/outdated.rs | 376 +++++++++++++++++++++++ crates/skilld-command/tests/remote.rs | 4 +- 6 files changed, 659 insertions(+), 10 deletions(-) create mode 100644 crates/skilld-command/src/outdated.rs create mode 100644 crates/skilld-command/tests/outdated.rs diff --git a/GLOSSARY.md b/GLOSSARY.md index 55e20f4f..f7d6b443 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -30,6 +30,7 @@ Every public export, command, error, route, and document uses these terms. | `skilld remove` | Skill removal | | `skilld upgrade` | Skill upgrade | | `skilld verify` | source verification | +| `skilld outdated` | outdated Skill report | | `skilld install skilld --global` | global skilld Skill install | | `skilld auth login` | account login | | `skilld auth status` | account authentication status | @@ -192,6 +193,16 @@ None recorded. **Casing:** `Agent target` in prose, `AgentTarget` in types. +### Outdated Skill report + +**Is:** the per Skill status produced by `skilld outdated`. + +**Use for:** current, outdated, unverified, local, and unmanaged Skill states. + +**Never:** version check, drift report, health check. + +**Casing:** `Outdated Skill report` in prose, `outdated` in commands. + ## Banned | Never | Use instead | Why | diff --git a/crates/skilld-command/src/lib.rs b/crates/skilld-command/src/lib.rs index 4100a15b..c55f98b3 100644 --- a/crates/skilld-command/src/lib.rs +++ b/crates/skilld-command/src/lib.rs @@ -1,8 +1,9 @@ mod config; mod local_store; +mod outdated; mod remote; -use std::collections::BTreeSet; +use std::collections::{BTreeMap, BTreeSet}; use std::ffi::OsString; use std::fmt; use std::fs; @@ -72,9 +73,20 @@ enum Command { global: bool, }, /// Upgrade installed Skills. - Upgrade { skill: Option }, + Upgrade { + skill: Option, + /// Upgrade Skills in the global scope. + #[arg(long)] + global: bool, + }, /// Verify a Skill source. Verify { skill: Option }, + /// Report outdated and unmanaged Skills. + Outdated { + /// Scan every Agent target on this system. + #[arg(long)] + system: bool, + }, /// Manage account authentication. Auth { #[command(subcommand)] @@ -144,12 +156,22 @@ pub trait Host { )) } - fn upgrade(&self, _name: Option<&str>) -> Result, CommandError> { + fn upgrade( + &self, + _name: Option<&str>, + _scope: InstallScope, + ) -> Result, CommandError> { Err(CommandError::unsupported_host( "Skill upgrade is unavailable on this host", )) } + fn outdated(&self, _system: bool) -> Result, CommandError> { + Err(CommandError::unsupported_host( + "Outdated Skill reports are unavailable on this host", + )) + } + fn config_get(&self, _key: &str) -> Result { Err(CommandError::unsupported_host( "configuration is unavailable on this host", @@ -453,8 +475,9 @@ fn dispatch(command: Command, host: &H) -> Result, CommandE )) }) .collect(), - Command::Upgrade { skill } => host.upgrade(skill.as_deref()), + Command::Upgrade { skill, global } => host.upgrade(skill.as_deref(), scope(global)), Command::Verify { skill } => host.verify(skill.as_deref()), + Command::Outdated { system } => host.outdated(system), } } @@ -1043,8 +1066,11 @@ impl Host for LocalHost { Ok(lines) } - fn upgrade(&self, requested: Option<&str>) -> Result, CommandError> { - let scope = InstallScope::Project; + fn upgrade( + &self, + requested: Option<&str>, + scope: InstallScope, + ) -> Result, CommandError> { let known = self.known_targets(scope)?; let store = self.store(scope); let names = selected_names(&store, &known, requested)?; @@ -1115,6 +1141,115 @@ impl Host for LocalHost { } Ok(upgraded) } + + fn outdated(&self, system: bool) -> Result, CommandError> { + let scopes = if system { + vec![InstallScope::Project, InstallScope::Global] + } else { + vec![InstallScope::Project] + }; + let mut lines = Vec::new(); + let mut managed = BTreeMap::>::new(); + let mut store_roots = Vec::new(); + let mut scan = Vec::new(); + for scope in scopes { + let known = self.known_targets(scope)?; + let store = self.store(scope); + let names = store.list(&known).map_err(CommandError::store)?; + for name in names { + let skill_name = + skilld_core::SkillName::parse(name.clone()).map_err(CommandError::domain)?; + let view = store + .view(&skill_name, &known) + .map_err(CommandError::store)?; + let mut paths = vec![view.canonical_path.clone()]; + for locked in &view.skill.targets { + if let Some(target) = known.iter().find(|target| target.agent == locked.agent) { + paths.push(target.root.join(name.as_str())); + } + } + managed + .entry(name.clone()) + .or_default() + .extend(paths.iter().cloned()); + lines.extend(self.report_outdated_view(&view, scope)?); + } + if system { + store_roots.push(store.root().to_path_buf()); + scan.push((scope, known)); + } + } + if system { + for skill in outdated::scan_unmanaged(&scan, &store_roots, &managed) { + let candidate = match self.search_candidate(&skill.name) { + Ok(candidate) => candidate, + Err(error) => { + lines.push(outdated::render_search_failure(&skill, &error.message)); + continue; + } + }; + lines.extend(outdated::render_unmanaged(&skill, candidate.as_deref())); + } + } + if lines.is_empty() { + lines.push("No installed Skills found.".to_owned()); + } + Ok(lines) + } +} + +impl LocalHost { + fn report_outdated_view( + &self, + view: &SkillView, + scope: InstallScope, + ) -> Result, CommandError> { + let name = &view.name; + let global = if scope == InstallScope::Global { + " --global" + } else { + "" + }; + match (&view.skill.source, &view.skill.source_status) { + ( + LockedSource::Remote { + source, commit_sha, .. + }, + skilld_core::SourceStatus::Verified { artifact_id, .. }, + ) => { + let selector = + skilld_core::RemoteSelector::parse(source).map_err(CommandError::remote)?; + match self + .remote_provider()? + .source_state(&selector, artifact_id, commit_sha) + .map_err(CommandError::remote)? + { + RemoteSourceState::Current => Ok(vec![format!("Current Skill {name}.")]), + RemoteSourceState::Stale { .. } => Ok(vec![format!( + "Outdated Skill {name}. Run skilld upgrade {name}{global}." + )]), + } + } + (LockedSource::Remote { source, .. }, skilld_core::SourceStatus::Unverified { .. }) => { + Ok(vec![format!( + "Unverified Skill {name}. Run skilld install {source} --direct{global} to upgrade it." + )]) + } + _ => Ok(vec![format!("Local Skill {name}.")]), + } + } + + fn search_candidate(&self, name: &str) -> Result, CommandError> { + let results = self + .remote_provider()? + .search(name, 5) + .map_err(CommandError::remote)?; + Ok(results + .iter() + .find(|result| result.name == name) + .and_then(|result| result.selector().ok()) + .map(|selector| selector.canonical())) + } } struct StagedRemote { @@ -1343,8 +1478,8 @@ mod tests { assert_eq!( command_names(), [ - "search", "install", "list", "view", "remove", "upgrade", "verify", "auth", - "config" + "search", "install", "list", "view", "remove", "upgrade", "verify", "outdated", + "auth", "config" ] ); } diff --git a/crates/skilld-command/src/local_store.rs b/crates/skilld-command/src/local_store.rs index 23a04834..f34365bc 100644 --- a/crates/skilld-command/src/local_store.rs +++ b/crates/skilld-command/src/local_store.rs @@ -1346,7 +1346,7 @@ fn fs_error(error: io::Error) -> StoreError { } } -fn normalize_path(path: &Path) -> PathBuf { +pub(crate) fn normalize_path(path: &Path) -> PathBuf { let mut normalized = PathBuf::new(); for component in path.components() { match component { diff --git a/crates/skilld-command/src/outdated.rs b/crates/skilld-command/src/outdated.rs new file mode 100644 index 00000000..3315a619 --- /dev/null +++ b/crates/skilld-command/src/outdated.rs @@ -0,0 +1,125 @@ +use std::collections::{BTreeMap, BTreeSet}; +use std::fs; +use std::path::PathBuf; + +use skilld_core::{AgentTargetId, InstallScope, SkillName}; + +use crate::ResolvedTarget; +use crate::local_store::normalize_path; + +pub(crate) struct UnmanagedSkill { + pub name: String, + pub path: PathBuf, + pub scope: InstallScope, + pub agents: Vec, +} + +pub(crate) fn scan_unmanaged( + scan: &[(InstallScope, Vec)], + store_roots: &[PathBuf], + managed: &BTreeMap>, +) -> Vec { + let stores = store_roots + .iter() + .filter_map(|root| fs::canonicalize(root).ok()) + .collect::>(); + let mut scanned_roots = BTreeSet::new(); + let mut by_path = BTreeMap::new(); + for (scope, targets) in scan { + for target in targets { + if !scanned_roots.insert(normalize_path(&target.root)) { + continue; + } + let Ok(entries) = fs::read_dir(&target.root) else { + continue; + }; + for entry in entries.flatten() { + let Some(name) = entry.file_name().to_str().map(ToOwned::to_owned) else { + continue; + }; + if SkillName::parse(name.clone()).is_err() + || !entry.path().join("SKILL.md").is_file() + { + continue; + } + let Some(canonical) = fs::canonicalize(entry.path()).ok() else { + continue; + }; + if stores.iter().any(|root| canonical.starts_with(root)) { + continue; + } + let managed = managed.get(&name).is_some_and(|paths| { + paths.iter().any(|path| { + normalize_path(path) == normalize_path(&entry.path()) + || fs::canonicalize(path).is_ok_and(|resolved| resolved == canonical) + }) + }); + if managed { + continue; + } + let skill = by_path + .entry(canonical.clone()) + .or_insert_with(|| UnmanagedSkill { + name: name.clone(), + path: canonical, + scope: *scope, + agents: Vec::new(), + }); + if !skill.agents.contains(&target.agent) { + skill.agents.push(target.agent); + } + } + } + } + let mut skills = by_path.into_values().collect::>(); + skills.sort_by(|left, right| left.name.cmp(&right.name).then(left.path.cmp(&right.path))); + skills +} + +pub(crate) fn render_search_failure(skill: &UnmanagedSkill, message: &str) -> String { + format!( + "Unmanaged Skill {} ({}). Skill search unavailable: {message}.", + skill.name, + agent_list(skill) + ) +} + +fn agent_list(skill: &UnmanagedSkill) -> String { + skill + .agents + .iter() + .map(|agent| agent.as_str()) + .collect::>() + .join(", ") +} + +pub(crate) fn render_unmanaged(skill: &UnmanagedSkill, candidate: Option<&str>) -> Vec { + let agents = agent_list(skill); + let Some(candidate) = candidate else { + return vec![format!( + "Unmanaged Skill {} ({agents}). No Repository match found.", + skill.name + )]; + }; + let global = if skill.scope == InstallScope::Global { + " --global" + } else { + "" + }; + let agent_flags = skill + .agents + .iter() + .map(|agent| format!("--agent {}", agent.as_str())) + .collect::>() + .join(" "); + vec![ + format!( + "Unmanaged Skill {} ({agents}). Candidate source {candidate}.", + skill.name + ), + format!( + "Delete {}, then run skilld install {candidate}{global} {agent_flags}.", + skill.path.display() + ), + ] +} diff --git a/crates/skilld-command/tests/outdated.rs b/crates/skilld-command/tests/outdated.rs new file mode 100644 index 00000000..22f82469 --- /dev/null +++ b/crates/skilld-command/tests/outdated.rs @@ -0,0 +1,376 @@ +use std::fs; +use std::path::Path; +use std::sync::{Arc, Mutex}; + +use sha2::{Digest, Sha256}; +use skilld_command::{ + Host, LocalHost, PreparedRemoteSkill, RemoteProvider, RemoteSourceState, run, +}; +use skilld_core::{ + AgentTargetId, InstallMode, InstallOperation, InstallRequest, InstallScope, InstallSource, + LockedSource, PreparedFile, RemoteError, RemoteSelector, SearchResult, SourceProvider, + SourceRequest, SourceSelector, SourceStatus, +}; + +struct Provider { + content: Mutex>, + stale: Mutex, + search_results: Mutex>, + fail_search: Mutex, +} + +impl Provider { + fn new(content: &str) -> Self { + Self { + content: Mutex::new(content.as_bytes().to_vec()), + stale: Mutex::new(false), + search_results: Mutex::new(vec![]), + fail_search: Mutex::new(false), + } + } + + fn search_result(name: &str) -> SearchResult { + SearchResult { + name: name.to_owned(), + description: None, + source: SourceRequest { + provider: SourceProvider::Github, + owner: "acme".to_owned(), + repository: "skills".to_owned(), + selector: SourceSelector::NamedSkill { + name: name.to_owned(), + }, + r#ref: None, + }, + stargazer_count: 0, + } + } +} + +fn installed_digest(file: &PreparedFile) -> String { + let mut hasher = Sha256::new(); + hasher.update((file.path.len() as u64).to_be_bytes()); + hasher.update(file.path.as_bytes()); + hasher.update((file.bytes.len() as u64).to_be_bytes()); + hasher.update(&file.bytes); + hasher + .finalize() + .iter() + .map(|byte| format!("{byte:02x}")) + .collect() +} + +impl RemoteProvider for Provider { + fn search(&self, _query: &str, _limit: u8) -> Result, RemoteError> { + if *self.fail_search.lock().unwrap() { + return Err(RemoteError::new( + "INVALID_RESPONSE", + "Skill search returned invalid JSON", + )); + } + Ok(self.search_results.lock().unwrap().clone()) + } + + fn prepare( + &self, + selector: &RemoteSelector, + _direct: bool, + ) -> Result { + let bytes = self.content.lock().unwrap().clone(); + let file = PreparedFile { + path: "SKILL.md".to_owned(), + mode: 0o644, + bytes, + }; + let digest = installed_digest(&file); + Ok(PreparedRemoteSkill { + files: vec![file], + locked_source: LockedSource::Remote { + source: selector.canonical(), + commit_sha: "0123456789abcdef0123456789abcdef01234567".to_owned(), + skill_path: "skills/example".to_owned(), + }, + source_status: SourceStatus::Verified { + artifact_id: format!("sha256:{digest}"), + content_sha256: digest.clone(), + installed_sha256: digest, + attestation_key_id: "test-key".to_owned(), + }, + }) + } + + fn source_state( + &self, + _selector: &RemoteSelector, + _artifact_id: &str, + _commit_sha: &str, + ) -> Result { + Ok(if *self.stale.lock().unwrap() { + RemoteSourceState::Stale { + current_artifact_id: "sha256:new".to_owned(), + current_commit_sha: "ffffffffffffffffffffffffffffffffffffffff".to_owned(), + } + } else { + RemoteSourceState::Current + }) + } +} + +fn install_project(host: &LocalHost, selector: &str) { + host.install_request(InstallRequest { + operation: InstallOperation::Install(InstallSource::Remote(selector.to_owned())), + scope: InstallScope::Project, + targets: vec![AgentTargetId::Codex], + mode: Some(InstallMode::Copy), + }) + .unwrap(); +} + +fn install_global(host: &LocalHost, selector: &str) { + host.install_request(InstallRequest { + operation: InstallOperation::Install(InstallSource::Remote(selector.to_owned())), + scope: InstallScope::Global, + targets: vec![AgentTargetId::Codex], + mode: Some(InstallMode::Copy), + }) + .unwrap(); +} + +fn unmanaged_skill(home: &Path, agent_dir: &str, name: &str) { + let directory = home.join(agent_dir).join("skills").join(name); + fs::create_dir_all(&directory).unwrap(); + fs::write( + directory.join("SKILL.md"), + format!("---\nname: {name}\ndescription: unmanaged\n---\n"), + ) + .unwrap(); +} + +#[test] +fn outdated_reports_current_and_stale_project_skills() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let provider = Arc::new(Provider::new( + "---\nname: example\ndescription: first\n---\n", + )); + let host = LocalHost::new(project.clone(), temporary.path().join("data")) + .with_remote_provider(provider.clone()); + install_project(&host, "skilld:skilld-dev/skills/example"); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let current = run(["skilld", "outdated"], &host, &mut stdout, &mut stderr); + + assert_eq!(current.exit_code, 0); + assert_eq!( + String::from_utf8(stdout.clone()).unwrap(), + "Current Skill example.\n" + ); + *provider.stale.lock().unwrap() = true; + stdout.clear(); + stderr.clear(); + + let outdated = run(["skilld", "outdated"], &host, &mut stdout, &mut stderr); + + assert_eq!(outdated.exit_code, 0); + assert_eq!( + String::from_utf8(stdout).unwrap(), + "Outdated Skill example. Run skilld upgrade example.\n" + ); +} + +#[test] +fn outdated_system_reports_a_stale_global_skill_with_the_global_upgrade() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let provider = Arc::new(Provider::new( + "---\nname: example\ndescription: first\n---\n", + )); + let host = LocalHost::new(project, temporary.path().join("data")) + .with_remote_provider(provider.clone()); + install_global(&host, "skilld:skilld-dev/skills/example"); + *provider.stale.lock().unwrap() = true; + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run( + ["skilld", "outdated", "--system"], + &host, + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 0); + assert_eq!( + String::from_utf8(stdout).unwrap(), + "Outdated Skill example. Run skilld upgrade example --global.\n" + ); +} + +#[test] +fn outdated_system_links_unmanaged_skills_to_a_repository() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let provider = Arc::new(Provider::new("---\nname: example\n---\n")); + *provider.search_results.lock().unwrap() = vec![Provider::search_result("vue-testing")]; + let home = temporary.path(); + unmanaged_skill(home, ".claude", "vue-testing"); + let host = LocalHost::new(project, home.join("data")).with_remote_provider(provider); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run( + ["skilld", "outdated", "--system"], + &host, + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 0); + let output = String::from_utf8(stdout).unwrap(); + let expected = format!( + "Unmanaged Skill vue-testing (claude-code). Candidate source skilld:acme/skills/vue-testing.\nDelete {}, then run skilld install skilld:acme/skills/vue-testing --global --agent claude-code.\n", + home.join(".claude/skills/vue-testing").display() + ); + assert_eq!(output, expected); +} + +#[test] +fn outdated_system_reports_unmanaged_skills_without_a_match() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let provider = Arc::new(Provider::new("---\nname: example\n---\n")); + *provider.search_results.lock().unwrap() = vec![Provider::search_result("vue-testing")]; + let home = temporary.path(); + unmanaged_skill(home, ".agents", "private-skill"); + let host = LocalHost::new(project, home.join("data")).with_remote_provider(provider); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run( + ["skilld", "outdated", "--system"], + &host, + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 0); + assert_eq!( + String::from_utf8(stdout).unwrap(), + "Unmanaged Skill private-skill (codex). No Repository match found.\n" + ); +} + +#[test] +fn outdated_system_surfaces_a_search_failure_and_keeps_scanning() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let provider = Arc::new(Provider::new("---\nname: example\n---\n")); + *provider.search_results.lock().unwrap() = vec![Provider::search_result("vue-testing")]; + *provider.fail_search.lock().unwrap() = true; + let home = temporary.path(); + unmanaged_skill(home, ".claude", "vue-testing"); + unmanaged_skill(home, ".agents", "other-skill"); + let host = LocalHost::new(project, home.join("data")).with_remote_provider(provider); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run( + ["skilld", "outdated", "--system"], + &host, + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 0); + assert_eq!( + String::from_utf8(stdout).unwrap(), + "Unmanaged Skill other-skill (codex). Skill search unavailable: Skill search returned invalid JSON.\nUnmanaged Skill vue-testing (claude-code). Skill search unavailable: Skill search returned invalid JSON.\n" + ); +} + +#[test] +fn outdated_system_reports_a_managed_skill_once() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let provider = Arc::new(Provider::new( + "---\nname: example\ndescription: first\n---\n", + )); + let host = LocalHost::new(project.clone(), temporary.path().join("data")) + .with_remote_provider(provider); + install_project(&host, "skilld:skilld-dev/skills/example"); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run( + ["skilld", "outdated", "--system"], + &host, + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 0); + assert_eq!( + String::from_utf8(stdout).unwrap(), + "Current Skill example.\n" + ); + assert!(project.join(".skills/example/SKILL.md").exists()); + assert!(project.join(".agents/skills/example/SKILL.md").exists()); +} + +#[test] +fn outdated_without_installed_skills_reports_none() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let host = LocalHost::new(project, temporary.path().join("data")); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run(["skilld", "outdated"], &host, &mut stdout, &mut stderr); + + assert_eq!(result.exit_code, 0); + assert_eq!( + String::from_utf8(stdout).unwrap(), + "No installed Skills found.\n" + ); +} + +#[test] +fn upgrade_global_upgrades_a_global_skill() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let data = temporary.path().join("data"); + let provider = Arc::new(Provider::new( + "---\nname: example\ndescription: first\n---\n", + )); + let host = LocalHost::new(project, data.clone()).with_remote_provider(provider.clone()); + install_global(&host, "skilld:skilld-dev/skills/example"); + *provider.content.lock().unwrap() = b"---\nname: example\ndescription: second\n---\n".to_vec(); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run( + ["skilld", "upgrade", "example", "--global"], + &host, + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 0); + assert_eq!( + String::from_utf8(stdout).unwrap(), + "Upgraded Skill example.\n" + ); + assert_eq!( + fs::read_to_string(data.join("skills/example/SKILL.md")).unwrap(), + "---\nname: example\ndescription: second\n---\n" + ); +} diff --git a/crates/skilld-command/tests/remote.rs b/crates/skilld-command/tests/remote.rs index fa1864ac..aa603805 100644 --- a/crates/skilld-command/tests/remote.rs +++ b/crates/skilld-command/tests/remote.rs @@ -881,7 +881,9 @@ fn remote_install_verify_and_failed_upgrade_use_the_normal_transaction() { *provider.content.lock().unwrap() = b"---\nname: example\ndescription: second\n---\n".to_vec(); *provider.fail_prepare.lock().unwrap() = true; - let error = host.upgrade(Some("example")).unwrap_err(); + let error = host + .upgrade(Some("example"), InstallScope::Project) + .unwrap_err(); assert_eq!(error.code, "CHECK_BLOCKED"); assert_eq!( From e36fa32991ac08c05c9961d3a97e04de791ec965 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Fri, 21 Aug 2026 23:53:29 +1000 Subject: [PATCH 2/2] fix(cli): keep the outdated report useful under failures - A corrupt or unreadable lockfile reports one line per scope and skips that scope scan instead of aborting, so managed copies are never advised for deletion. - A source state network failure reports one line per Skill instead of aborting the whole report. - Agents sharing one directory, Codex and Amp on .agents/skills, are grouped so recovery commands target every Agent that reads the Skill. - Unverified recovery keeps the locked Agent targets. - The bundled skilld Skill reports its own source line. - Repository candidates show their star count, and an invalid candidate selector reports a search failure instead of no match. --- crates/skilld-command/src/lib.rs | 114 +++++++++----- crates/skilld-command/src/outdated.rs | 140 ++++++++++------- crates/skilld-command/tests/outdated.rs | 191 +++++++++++++++++++++++- 3 files changed, 342 insertions(+), 103 deletions(-) diff --git a/crates/skilld-command/src/lib.rs b/crates/skilld-command/src/lib.rs index c55f98b3..600f122a 100644 --- a/crates/skilld-command/src/lib.rs +++ b/crates/skilld-command/src/lib.rs @@ -1141,7 +1141,6 @@ impl Host for LocalHost { } Ok(upgraded) } - fn outdated(&self, system: bool) -> Result, CommandError> { let scopes = if system { vec![InstallScope::Project, InstallScope::Global] @@ -1155,13 +1154,31 @@ impl Host for LocalHost { for scope in scopes { let known = self.known_targets(scope)?; let store = self.store(scope); - let names = store.list(&known).map_err(CommandError::store)?; + let names = match store.list(&known) { + Ok(names) => names, + Err(error) => { + // Without a readable lockfile, managed copies cannot be told from unmanaged ones. + lines.push(format!( + "Skill store unavailable in {} scope: {}", + scope.as_str(), + CommandError::store(error).message + )); + continue; + } + }; for name in names { let skill_name = skilld_core::SkillName::parse(name.clone()).map_err(CommandError::domain)?; - let view = store - .view(&skill_name, &known) - .map_err(CommandError::store)?; + let view = match store.view(&skill_name, &known) { + Ok(view) => view, + Err(error) => { + lines.push(format!( + "Skill {name} details unavailable: {}", + CommandError::store(error).message + )); + continue; + } + }; let mut paths = vec![view.canonical_path.clone()]; for locked in &view.skill.targets { if let Some(target) = known.iter().find(|target| target.agent == locked.agent) { @@ -1172,7 +1189,7 @@ impl Host for LocalHost { .entry(name.clone()) .or_default() .extend(paths.iter().cloned()); - lines.extend(self.report_outdated_view(&view, scope)?); + lines.extend(self.report_outdated_view(&view, scope)); } if system { store_roots.push(store.root().to_path_buf()); @@ -1181,14 +1198,15 @@ impl Host for LocalHost { } if system { for skill in outdated::scan_unmanaged(&scan, &store_roots, &managed) { - let candidate = match self.search_candidate(&skill.name) { - Ok(candidate) => candidate, + match self.search_candidate(&skill.name) { + Ok(Some(candidate)) => { + lines.extend(outdated::render_unmanaged(&skill, Some(&candidate))) + } + Ok(None) => lines.extend(outdated::render_unmanaged(&skill, None)), Err(error) => { lines.push(outdated::render_search_failure(&skill, &error.message)); - continue; } - }; - lines.extend(outdated::render_unmanaged(&skill, candidate.as_deref())); + } } } if lines.is_empty() { @@ -1199,11 +1217,7 @@ impl Host for LocalHost { } impl LocalHost { - fn report_outdated_view( - &self, - view: &SkillView, - scope: InstallScope, - ) -> Result, CommandError> { + fn report_outdated_view(&self, view: &SkillView, scope: InstallScope) -> Vec { let name = &view.name; let global = if scope == InstallScope::Global { " --global" @@ -1217,41 +1231,65 @@ impl LocalHost { }, skilld_core::SourceStatus::Verified { artifact_id, .. }, ) => { - let selector = - skilld_core::RemoteSelector::parse(source).map_err(CommandError::remote)?; - match self - .remote_provider()? - .source_state(&selector, artifact_id, commit_sha) - .map_err(CommandError::remote)? - { - RemoteSourceState::Current => Ok(vec![format!("Current Skill {name}.")]), - RemoteSourceState::Stale { .. } => Ok(vec![format!( - "Outdated Skill {name}. Run skilld upgrade {name}{global}." - )]), + let state = skilld_core::RemoteSelector::parse(source) + .map_err(CommandError::remote) + .and_then(|selector| { + self.remote_provider()? + .source_state(&selector, artifact_id, commit_sha) + .map_err(CommandError::remote) + }); + match state { + Ok(RemoteSourceState::Current) => { + vec![format!("Current Skill {name}.")] + } + Ok(RemoteSourceState::Stale { .. }) => { + vec![format!( + "Outdated Skill {name}. Run skilld upgrade {name}{global}." + )] + } + Err(error) => { + vec![format!( + "Source state unavailable for Skill {name}: {}.", + error.message + )] + } } } (LockedSource::Remote { source, .. }, skilld_core::SourceStatus::Unverified { .. }) => { - Ok(vec![format!( - "Unverified Skill {name}. Run skilld install {source} --direct{global} to upgrade it." - )]) + let agents = view + .skill + .targets + .iter() + .map(|locked| locked.agent) + .collect::>(); + let agent_flags = outdated::agent_flags(&agents); + vec![format!( + "Unverified Skill {name}. Run skilld install {source} --direct{global}{agent_flags} to upgrade it." + )] } - _ => Ok(vec![format!("Local Skill {name}.")]), + (LockedSource::BundledSkilld, _) => vec![format!("skilld-maintained Skill {name}.")], + _ => vec![format!("Local Skill {name}.")], } } - fn search_candidate(&self, name: &str) -> Result, CommandError> { + fn search_candidate( + &self, + name: &str, + ) -> Result, CommandError> { let results = self .remote_provider()? .search(name, 5) .map_err(CommandError::remote)?; - Ok(results - .iter() - .find(|result| result.name == name) - .and_then(|result| result.selector().ok()) - .map(|selector| selector.canonical())) + let Some(result) = results.into_iter().find(|result| result.name == name) else { + return Ok(None); + }; + let selector = result.selector().map_err(CommandError::remote)?; + Ok(Some(outdated::SkillCandidate { + selector: selector.canonical(), + stargazer_count: result.stargazer_count, + })) } } - struct StagedRemote { _directory: tempfile::TempDir, skill: PathBuf, diff --git a/crates/skilld-command/src/outdated.rs b/crates/skilld-command/src/outdated.rs index 3315a619..19679e04 100644 --- a/crates/skilld-command/src/outdated.rs +++ b/crates/skilld-command/src/outdated.rs @@ -1,4 +1,4 @@ -use std::collections::{BTreeMap, BTreeSet}; +use std::collections::BTreeMap; use std::fs; use std::path::PathBuf; @@ -14,6 +14,11 @@ pub(crate) struct UnmanagedSkill { pub agents: Vec, } +pub(crate) struct SkillCandidate { + pub selector: String, + pub stargazer_count: u64, +} + pub(crate) fn scan_unmanaged( scan: &[(InstallScope, Vec)], store_roots: &[PathBuf], @@ -23,55 +28,63 @@ pub(crate) fn scan_unmanaged( .iter() .filter_map(|root| fs::canonicalize(root).ok()) .collect::>(); - let mut scanned_roots = BTreeSet::new(); - let mut by_path = BTreeMap::new(); + let mut roots = BTreeMap::)>::new(); for (scope, targets) in scan { for target in targets { - if !scanned_roots.insert(normalize_path(&target.root)) { + let entry = roots + .entry(normalize_path(&target.root)) + .or_insert_with(|| (*scope, Vec::new())); + if !entry.1.contains(&target.agent) { + entry.1.push(target.agent); + } + } + } + let mut by_path = BTreeMap::new(); + for (root, (scope, agents)) in roots { + let Ok(entries) = fs::read_dir(&root) else { + continue; + }; + for entry in entries.flatten() { + let Some(name) = entry.file_name().to_str().map(ToOwned::to_owned) else { + continue; + }; + if SkillName::parse(name.clone()).is_err() || !entry.path().join("SKILL.md").is_file() { continue; } - let Ok(entries) = fs::read_dir(&target.root) else { + let Some(canonical) = fs::canonicalize(entry.path()).ok() else { continue; }; - for entry in entries.flatten() { - let Some(name) = entry.file_name().to_str().map(ToOwned::to_owned) else { - continue; - }; - if SkillName::parse(name.clone()).is_err() - || !entry.path().join("SKILL.md").is_file() - { - continue; - } - let Some(canonical) = fs::canonicalize(entry.path()).ok() else { - continue; - }; - if stores.iter().any(|root| canonical.starts_with(root)) { - continue; - } - let managed = managed.get(&name).is_some_and(|paths| { - paths.iter().any(|path| { - normalize_path(path) == normalize_path(&entry.path()) - || fs::canonicalize(path).is_ok_and(|resolved| resolved == canonical) - }) + if stores.iter().any(|root| canonical.starts_with(root)) { + continue; + } + let managed = managed.get(&name).is_some_and(|paths| { + paths.iter().any(|path| { + normalize_path(path) == normalize_path(&entry.path()) + || fs::canonicalize(path).is_ok_and(|resolved| resolved == canonical) + }) + }); + if managed { + continue; + } + let skill = by_path + .entry(canonical.clone()) + .or_insert_with(|| UnmanagedSkill { + name: name.clone(), + path: canonical, + scope, + agents: Vec::new(), }); - if managed { - continue; - } - let skill = by_path - .entry(canonical.clone()) - .or_insert_with(|| UnmanagedSkill { - name: name.clone(), - path: canonical, - scope: *scope, - agents: Vec::new(), - }); - if !skill.agents.contains(&target.agent) { - skill.agents.push(target.agent); + for agent in &agents { + if !skill.agents.contains(agent) { + skill.agents.push(*agent); } } } } let mut skills = by_path.into_values().collect::>(); + for skill in &mut skills { + skill.agents.sort_by_key(|agent| agent.as_str()); + } skills.sort_by(|left, right| left.name.cmp(&right.name).then(left.path.cmp(&right.path))); skills } @@ -84,16 +97,10 @@ pub(crate) fn render_search_failure(skill: &UnmanagedSkill, message: &str) -> St ) } -fn agent_list(skill: &UnmanagedSkill) -> String { - skill - .agents - .iter() - .map(|agent| agent.as_str()) - .collect::>() - .join(", ") -} - -pub(crate) fn render_unmanaged(skill: &UnmanagedSkill, candidate: Option<&str>) -> Vec { +pub(crate) fn render_unmanaged( + skill: &UnmanagedSkill, + candidate: Option<&SkillCandidate>, +) -> Vec { let agents = agent_list(skill); let Some(candidate) = candidate else { return vec![format!( @@ -106,20 +113,37 @@ pub(crate) fn render_unmanaged(skill: &UnmanagedSkill, candidate: Option<&str>) } else { "" }; - let agent_flags = skill - .agents - .iter() - .map(|agent| format!("--agent {}", agent.as_str())) - .collect::>() - .join(" "); + let agent_flags = agent_flags(&skill.agents); vec![ format!( - "Unmanaged Skill {} ({agents}). Candidate source {candidate}.", - skill.name + "Unmanaged Skill {} ({agents}). Candidate source {}, {} stars.", + skill.name, candidate.selector, candidate.stargazer_count ), format!( - "Delete {}, then run skilld install {candidate}{global} {agent_flags}.", - skill.path.display() + "Delete {}, then run skilld install {}{global}{agent_flags}.", + skill.path.display(), + candidate.selector ), ] } + +pub(crate) fn agent_flags(agents: &[AgentTargetId]) -> String { + if agents.is_empty() { + return String::new(); + } + let flags = agents + .iter() + .map(|agent| format!("--agent {}", agent.as_str())) + .collect::>() + .join(" "); + format!(" {flags}") +} + +fn agent_list(skill: &UnmanagedSkill) -> String { + skill + .agents + .iter() + .map(|agent| agent.as_str()) + .collect::>() + .join(", ") +} diff --git a/crates/skilld-command/tests/outdated.rs b/crates/skilld-command/tests/outdated.rs index 22f82469..202a7131 100644 --- a/crates/skilld-command/tests/outdated.rs +++ b/crates/skilld-command/tests/outdated.rs @@ -15,6 +15,7 @@ use skilld_core::{ struct Provider { content: Mutex>, stale: Mutex, + fail_state: Mutex, search_results: Mutex>, fail_search: Mutex, } @@ -24,6 +25,7 @@ impl Provider { Self { content: Mutex::new(content.as_bytes().to_vec()), stale: Mutex::new(false), + fail_state: Mutex::new(false), search_results: Mutex::new(vec![]), fail_search: Mutex::new(false), } @@ -74,7 +76,7 @@ impl RemoteProvider for Provider { fn prepare( &self, selector: &RemoteSelector, - _direct: bool, + direct: bool, ) -> Result { let bytes = self.content.lock().unwrap().clone(); let file = PreparedFile { @@ -90,11 +92,18 @@ impl RemoteProvider for Provider { commit_sha: "0123456789abcdef0123456789abcdef01234567".to_owned(), skill_path: "skills/example".to_owned(), }, - source_status: SourceStatus::Verified { - artifact_id: format!("sha256:{digest}"), - content_sha256: digest.clone(), - installed_sha256: digest, - attestation_key_id: "test-key".to_owned(), + source_status: if direct { + SourceStatus::Unverified { + content_sha256: digest.clone(), + installed_sha256: digest, + } + } else { + SourceStatus::Verified { + artifact_id: format!("sha256:{digest}"), + content_sha256: digest.clone(), + installed_sha256: digest, + attestation_key_id: "test-key".to_owned(), + } }, }) } @@ -105,6 +114,12 @@ impl RemoteProvider for Provider { _artifact_id: &str, _commit_sha: &str, ) -> Result { + if *self.fail_state.lock().unwrap() { + return Err(RemoteError::new( + "SERVICE_UNAVAILABLE", + "the remote service returned HTTP 503", + )); + } Ok(if *self.stale.lock().unwrap() { RemoteSourceState::Stale { current_artifact_id: "sha256:new".to_owned(), @@ -232,7 +247,7 @@ fn outdated_system_links_unmanaged_skills_to_a_repository() { assert_eq!(result.exit_code, 0); let output = String::from_utf8(stdout).unwrap(); let expected = format!( - "Unmanaged Skill vue-testing (claude-code). Candidate source skilld:acme/skills/vue-testing.\nDelete {}, then run skilld install skilld:acme/skills/vue-testing --global --agent claude-code.\n", + "Unmanaged Skill vue-testing (claude-code). Candidate source skilld:acme/skills/vue-testing, 0 stars.\nDelete {}, then run skilld install skilld:acme/skills/vue-testing --global --agent claude-code.\n", home.join(".claude/skills/vue-testing").display() ); assert_eq!(output, expected); @@ -374,3 +389,165 @@ fn upgrade_global_upgrades_a_global_skill() { "---\nname: example\ndescription: second\n---\n" ); } + +#[test] +fn outdated_survives_a_source_state_failure() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let provider = Arc::new(Provider::new( + "---\nname: example\ndescription: first\n---\n", + )); + *provider.fail_state.lock().unwrap() = true; + let host = LocalHost::new(project, temporary.path().join("data")) + .with_remote_provider(provider.clone()); + install_project(&host, "skilld:skilld-dev/skills/example"); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run(["skilld", "outdated"], &host, &mut stdout, &mut stderr); + + assert_eq!(result.exit_code, 0); + assert_eq!( + String::from_utf8(stdout).unwrap(), + "Source state unavailable for Skill example: the remote service returned HTTP 503.\n" + ); +} + +#[test] +fn outdated_system_survives_a_corrupt_global_store() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let home = temporary.path(); + let data = home.join("data"); + fs::create_dir_all(data.join("skills")).unwrap(); + fs::write(data.join("skills/skilld-lock.yaml"), "not json").unwrap(); + unmanaged_skill(&project, ".agents", "unmanaged-project"); + unmanaged_skill(home, ".claude", "hidden-global"); + let provider = Arc::new(Provider::new("---\nname: example\n---\n")); + let host = LocalHost::new(project, data.clone()).with_remote_provider(provider); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run( + ["skilld", "outdated", "--system"], + &host, + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 0); + let output = String::from_utf8(stdout).unwrap(); + assert!( + output.starts_with("Skill store unavailable in global scope: "), + "expected a store failure line, got: {output}" + ); + assert!( + output.contains( + "Unmanaged Skill unmanaged-project (amp, codex). No Repository match found.\n" + ) + ); + assert!( + !output.contains("hidden-global"), + "a scope with an unreadable lockfile must not report its Skills: {output}" + ); +} + +#[test] +fn outdated_system_groups_agents_sharing_one_directory() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let home = temporary.path(); + unmanaged_skill(&project, ".agents", "shared"); + let provider = Arc::new(Provider::new("---\nname: example\n---\n")); + let host = LocalHost::new(project, home.join("data")).with_remote_provider(provider); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run( + ["skilld", "outdated", "--system"], + &host, + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 0); + let output = String::from_utf8(stdout).unwrap(); + assert!( + output.contains("Unmanaged Skill shared (amp, codex). No Repository match found.\n"), + "expected both agents sharing .agents/skills, got: {output}" + ); +} + +#[test] +fn outdated_gives_the_direct_recovery_for_unverified_skills() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let provider = Arc::new(Provider::new( + "---\nname: example\ndescription: direct\n---\n", + )); + let host = + LocalHost::new(project, temporary.path().join("data")).with_remote_provider(provider); + host.install_request(InstallRequest { + operation: InstallOperation::Install(InstallSource::DirectRemote( + "github:skilld-dev/skills/skills/example".to_owned(), + )), + scope: InstallScope::Project, + targets: vec![AgentTargetId::Codex], + mode: Some(InstallMode::Copy), + }) + .unwrap(); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run(["skilld", "outdated"], &host, &mut stdout, &mut stderr); + + assert_eq!(result.exit_code, 0); + assert_eq!( + String::from_utf8(stdout).unwrap(), + "Unverified Skill example. Run skilld install github:skilld-dev/skills/skills/example --direct --agent codex to upgrade it.\n" + ); +} + +#[test] +fn outdated_reports_the_bundled_skill_by_its_source() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let bundled = temporary.path().join("bundled").join("skilld"); + fs::create_dir_all(&bundled).unwrap(); + fs::write( + bundled.join("SKILL.md"), + "---\nname: skilld\ndescription: bundled\n---\n", + ) + .unwrap(); + let provider = Arc::new(Provider::new("---\nname: example\n---\n")); + let host = LocalHost::new(project, temporary.path().join("data")) + .with_remote_provider(provider) + .with_bundled_skill(bundled); + host.install_request(InstallRequest { + operation: InstallOperation::Install(InstallSource::BundledSkilld), + scope: InstallScope::Global, + targets: vec![AgentTargetId::Codex], + mode: Some(InstallMode::Copy), + }) + .unwrap(); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run( + ["skilld", "outdated", "--system"], + &host, + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 0); + assert_eq!( + String::from_utf8(stdout).unwrap(), + "skilld-maintained Skill skilld.\n" + ); +}