From 5a816214a1d3beab037709a0025c7213ea32a516 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Tue, 1 Sep 2026 16:07:45 +1000 Subject: [PATCH 1/5] feat(cli): run and add accept repository, curator, and collection refs skilld.dev prints `npx skilld add gh:owner/repo`, `@login`, and `@login/slug`, but the v3 CLI had no `add` and `run` took one Skill. `skilld run ` now prints an index: one line per Skill with its run command, and loads none of them. `skilld add ` installs every Skill the ref names through the hosted Artifact path `install` uses. `skilld add ` installs one Skill like `install`. Refs parse once into `SkillRef` in skilld-core. Listings come from `/api/skills?owner=`, `/api/curators/{login}`, and `/api/collections/by-author/{login}/{slug}`. Claude-Session: https://claude.ai/code/session_018T67Ndp8FAjnHWthXABbJW --- GLOSSARY.md | 35 ++ README.md | 12 + crates/skilld-command/src/lib.rs | 491 +++++++++++++++++++++++--- crates/skilld-command/src/output.rs | 121 ++++++- crates/skilld-command/src/remote.rs | 243 ++++++++++++- crates/skilld-command/src/run.rs | 4 +- crates/skilld-command/tests/add.rs | 161 +++++++++ crates/skilld-command/tests/remote.rs | 205 ++++++++++- crates/skilld-command/tests/run.rs | 4 +- crates/skilld-core/src/lib.rs | 2 + crates/skilld-core/src/reference.rs | 267 ++++++++++++++ crates/skilld-core/src/remote.rs | 36 +- docs/migrate-v2-to-v3.md | 2 + 13 files changed, 1498 insertions(+), 85 deletions(-) create mode 100644 crates/skilld-command/tests/add.rs create mode 100644 crates/skilld-core/src/reference.rs diff --git a/GLOSSARY.md b/GLOSSARY.md index 72fffd4c..b6bc4b2b 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -22,6 +22,9 @@ Every public export, command, error, route, and document uses these terms. | Source status | lockfile and protocol | published value | skilld CLI, CI | source status | | Update relation | skilld CLI JSON v1 | published value | Agent, developer, CI | update relation | | Agent target | skilld CLI | published configuration | Agent | Agent target | +| Curator | skilld.dev | published route | skilld.dev, skilld CLI | curator | +| Collection | skilld.dev | published route | skilld.dev, skilld CLI | collection | +| Multi-skill ref | `skilld run`, `skilld add` | published argument | developer, Agent | ref | | Identifier | Term | | --- | --- | @@ -29,6 +32,8 @@ Every public export, command, error, route, and document uses these terms. | `skilld run` | transient Skill load | | `skilld run --file` | supporting file read | | `skilld install` | Skill install | +| `skilld add` | multi-skill install | +| `skilld run gh:OWNER/REPOSITORY` | Skill index | | `skilld list` | installed Skills | | `skilld view` | Skill details | | `skilld remove` | Skill removal | @@ -222,6 +227,36 @@ The Rust type for the second is `TransientSkill`, never `SkillRun`. **Casing:** `Agent target` in prose, `AgentTarget` in types. +### Curator + +**Is:** a skilld.dev account that publishes collections at `/@LOGIN`. + +**Use for:** the `@LOGIN` ref and curator profile pages. + +**Never:** author, publisher, maintainer in this sense. + +**Casing:** `Curator` in headings, `curator` in sentences. + +### Collection + +**Is:** a curator's ordered, named list of Skills at `/@LOGIN/SLUG`. + +**Use for:** the `@LOGIN/SLUG` ref and collection pages. + +**Never:** pack, bundle, list, set. + +**Casing:** `Collection` in headings, `collection` in sentences. + +### Multi-skill ref + +**Is:** one `skilld run` or `skilld add` argument that names more than one Skill: `gh:OWNER/REPOSITORY`, `@LOGIN`, or `@LOGIN/SLUG`. + +**Use for:** the argument grammar and its index output. + +**Never:** bulk selector, group source, target. + +**Casing:** `multi-skill ref` in prose, `MultiSkillRef` in Rust. + ### Outdated Skill report **Is:** the per Skill status produced by `skilld outdated`. diff --git a/README.md b/README.md index 16273afc..a0aac6cf 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,14 @@ skilld run skilld:skilld-dev/skills/vue --revision --file references/ap # Install a Skill in the current project skilld install skilld:skilld-dev/skills/vue +# List every Skill a Repository, curator, or collection names +skilld run gh:vuejs/core +skilld run @harlan-zw +skilld run @harlan-zw/vue-nuxt + +# Install every Skill one of those refs names +skilld add @harlan-zw/vue-nuxt + # Inspect installed Skills skilld list skilld view vue @@ -110,6 +118,10 @@ Use `--mode copy` or `--mode symlink` to control target writes. Run `skilld install` without a source to restore the lockfile state. +`skilld run` with a Repository, curator, or collection ref prints an index. +The index has one line per Skill with its run command. It loads no Skill. +`skilld add` installs every Skill the same ref names. It accepts the `skilld install` flags. + ## Artifact delivery The skilld CLI resolves remote Skills through the skilld.dev API. diff --git a/crates/skilld-command/src/lib.rs b/crates/skilld-command/src/lib.rs index 82e6fd4b..845fce69 100644 --- a/crates/skilld-command/src/lib.rs +++ b/crates/skilld-command/src/lib.rs @@ -34,9 +34,9 @@ pub use run::{ use skilld_core::{ AGENT_TARGETS, AgentTargetId, CommitHistory, CommitSha, DomainError, GlobalTargetPath, InstallMode, InstallOperation, InstallRequest, InstallScope, InstallSource, LockedSource, - NotTrackedReason, RemoteSelector, SourceRef, UpdateFailure, UpdateLatestCommit, - UpdateModelError, UpdatePlan, UpdatePlanItem, UpdatePlanV1, UpdateRelation, UpdateRetryAfter, - VERSION, classify_update_comparison, select_target_ids, + MultiSkillRef, NotTrackedReason, RemoteSelector, SkillListing, SkillRef, SourceRef, + UpdateFailure, UpdateLatestCommit, UpdateModelError, UpdatePlan, UpdatePlanItem, UpdatePlanV1, + UpdateRelation, UpdateRetryAfter, VERSION, classify_update_comparison, select_target_ids, }; use skilld_ui::text::is_unsafe_terminal; use skilld_ui::{Detail, Line, Marker, Screen}; @@ -102,13 +102,45 @@ enum Command { )] direct: bool, }, + /// Install every Skill a Repository, curator, or collection names. + #[command( + long_about = "Install every Skill a Repository, curator, or collection names.\n\nGive REF as:\n gh:OWNER/REPOSITORY\n Install every Skill the Repository carries.\n @LOGIN\n Install every Skill the curator's collections name.\n @LOGIN/SLUG\n Install every Skill one collection names.\n Any SOURCE skilld install accepts\n Install that one Skill.\n\nEach Skill installs through the same hosted Artifact path skilld install uses.\nRun skilld run REF first to see the Skills a ref names.", + after_long_help = "Examples:\n npx skilld add gh:skilld-dev/skills\n npx skilld add @harlan-zw/nuxt --agent codex\n npx skilld add skilld:skilld-dev/skills/vue --global" + )] + Add { + /// The Repository, curator, collection, or Skill source to install. + #[arg(value_name = "REF")] + reference: String, + #[arg( + long, + long_help = "Install to your account-level Agent targets. The default is the current project." + )] + global: bool, + #[arg( + long = "agent", + value_name = "AGENT", + long_help = "Select an Agent target. Repeat --agent to select several.\nDefault: every Agent target skilld detects. If skilld detects none, it uses agent.targets." + )] + agents: Vec, + #[arg( + long, + value_name = "MODE", + long_help = "Choose how each Agent target receives the Skill.\nValues: copy, symlink. The default comes from install.mode." + )] + mode: Option, + #[arg( + long, + long_help = "Fetch a public GitHub Repository without going through skilld.dev.\nOnly one Skill source accepts --direct. A direct install records the unverified source status." + )] + direct: bool, + }, /// Load a Skill for this session without installing it. #[command( - long_about = "Load a Skill for this session without installing it.\n\nskilld run prints SKILL.md so the calling Agent follows it now.\nA remote run retains no Skill files. It creates no lockfile entry, Agent target,\nor project file.\n\nskilld names the supporting files and prints none of them.\nUse --file to read one. Remote file reads also require the returned --revision.\nUse skilld install to put supporting files on disk.\n\nGive SOURCE in the same forms skilld install accepts.", - after_long_help = "Examples:\n npx skilld run skilld:skilld-dev/skills/find-skill\n skilld run ./skills/my-skill --file references/api.md\n skilld run github:skilld-dev/skilld/skills/skilld --direct\n skilld run ./skills/my-skill" + long_about = "Load a Skill for this session without installing it.\n\nskilld run prints SKILL.md so the calling Agent follows it now.\nA remote run retains no Skill files. It creates no lockfile entry, Agent target,\nor project file.\n\nskilld names the supporting files and prints none of them.\nUse --file to read one. Remote file reads also require the returned --revision.\nUse skilld install to put supporting files on disk.\n\nGive SOURCE in the same forms skilld install accepts.\n\nGive a ref that names several Skills to list them instead:\n gh:OWNER/REPOSITORY, @LOGIN, or @LOGIN/SLUG\nskilld prints one line per Skill with its run command and loads none of them.", + after_long_help = "Examples:\n npx skilld run skilld:skilld-dev/skills/find-skill\n npx skilld run gh:skilld-dev/skills\n npx skilld run @harlan-zw/nuxt\n skilld run ./skills/my-skill --file references/api.md\n skilld run github:skilld-dev/skilld/skills/skilld --direct\n skilld run ./skills/my-skill" )] Run { - /// The Skill source to load. + /// The Skill source to load, or a ref that names several Skills. #[arg(value_name = "SOURCE")] source: String, #[arg( @@ -227,6 +259,12 @@ pub trait Host { )) } + fn list_skills(&self, _reference: &MultiSkillRef) -> Result { + Err(CommandError::unsupported_host( + "Skill listings are unavailable on this host", + )) + } + fn view(&self, _name: &str, _scope: InstallScope) -> Result { Err(CommandError::unsupported_host( "Skill details are unavailable on this host", @@ -387,6 +425,15 @@ impl CommandError { ) } + fn direct_multi_skill_ref(reference: &MultiSkillRef) -> Self { + Self::usage( + "DIRECT_SOURCE_REQUIRED", + format!( + "--direct needs one Skill source. {reference} names several Skills. Remove --direct, then run the same command again." + ), + ) + } + fn remote_file_revision() -> Self { Self::input( "Remote --file reads require --revision. Run the Skill without --file first. Then repeat this run with the returned revision.", @@ -708,7 +755,8 @@ fn requested_output(args: &[OsString]) -> (bool, bool) { fn display_path(args: &[OsString]) -> String { let commands = [ - "search", "install", "run", "list", "view", "remove", "update", "verify", "auth", "config", + "search", "install", "add", "run", "list", "view", "remove", "update", "verify", "auth", + "config", ]; let mut path = vec!["skilld"]; if let Some(command) = args @@ -781,63 +829,60 @@ fn dispatch( platform: CommandPlatform, ) -> Result { match command { - Command::Install { - source, + Command::Add { + reference, global, agents, mode, direct, } => { - let scope = scope(global); - let operation = match source { - Some(source) => match (direct, InstallSource::parse(&source)) { - (true, InstallSource::Remote(source)) => { - InstallOperation::Install(InstallSource::DirectRemote(source)) - } - (true, InstallSource::DirectRemote(source)) => { - InstallOperation::Install(InstallSource::DirectRemote(source)) - } - (true, InstallSource::Local(_)) => { - return Err(CommandError::direct_local_install_source()); - } - (true, InstallSource::BundledSkilld) => { - return Err(CommandError::direct_bundled_install_source()); + let reference = SkillRef::parse(&reference).map_err(CommandError::remote)?; + let options = InstallOptions::parse(global, &agents, mode.as_deref())?; + match reference { + SkillRef::Skill(source) => install(host, Some(source), options, direct), + SkillRef::Many(reference) if direct => { + Err(CommandError::direct_multi_skill_ref(&reference)) + } + SkillRef::Many(reference) => { + let listing = list_skills(host, &reference)?; + let mut lines = Vec::with_capacity(listing.items.len()); + for (index, item) in listing.items.iter().enumerate() { + let names = host + .install_request(InstallRequest { + operation: InstallOperation::Install(InstallSource::Remote( + item.selector(), + )), + scope: options.scope, + targets: options.targets.clone(), + mode: options.mode, + }) + .map_err(|error| CommandError { + message: format!( + "{}. skilld installed {index} of {} Skills from {reference} before this failure.", + error.message.trim_end_matches('.'), + listing.items.len() + ), + ..error + })?; + lines.extend( + names + .into_iter() + .map(|name| Line::success(format!("Installed Skill {name}."))), + ); } - (false, source) => InstallOperation::Install(source), - }, - None if direct => InstallOperation::DirectRestore, - None => InstallOperation::Restore, - }; - if operation == InstallOperation::Install(InstallSource::BundledSkilld) - && scope != InstallScope::Global - { - return Err(CommandError::input( - "install the skilld-maintained Skill with --global", - )); - } - let targets = agents - .iter() - .map(|agent| AgentTargetId::parse(agent).map_err(CommandError::domain)) - .collect::, _>>()?; - let mode = mode - .as_deref() - .map(InstallMode::parse) - .transpose() - .map_err(CommandError::domain)?; - let names = host.install_request(InstallRequest { - operation, - scope, - targets, - mode, - })?; - let mut lines = names - .into_iter() - .map(|name| Line::success(format!("Installed Skill {name}."))) - .collect::>(); - if direct { - lines.push(Line::hint("Review the unverified Skill before use.")); + Ok(CommandOutput::Screen(Screen::new(lines))) + } } - Ok(CommandOutput::Screen(Screen::new(lines))) + } + Command::Install { + source, + global, + agents, + mode, + direct, + } => { + let options = InstallOptions::parse(global, &agents, mode.as_deref())?; + install(host, source, options, direct) } Command::Run { source, @@ -845,6 +890,22 @@ fn dispatch( revision, direct, } => { + let reference = SkillRef::parse(&source).map_err(CommandError::remote)?; + let source = match reference { + SkillRef::Skill(source) => source, + SkillRef::Many(reference) => { + if direct { + return Err(CommandError::direct_multi_skill_ref(&reference)); + } + if !files.is_empty() || revision.is_some() { + return Err(CommandError::input(format!( + "--file and --revision need one Skill source. {reference} names several Skills. Run skilld run {reference} to list them." + ))); + } + return list_skills(host, &reference) + .map(|listing| CommandOutput::Run(RunOutcome::Index(listing))); + } + }; run::reject_duplicate_files(&files)?; let revision = revision.map(CommitSha::parse).transpose().map_err(|_| { CommandError::input("--revision must use 40 lowercase hexadecimal characters") @@ -995,6 +1056,95 @@ fn dispatch( } } +/// The install flags `skilld install` and `skilld add` share, parsed once. +#[derive(Clone, Debug, Eq, PartialEq)] +struct InstallOptions { + scope: InstallScope, + targets: Vec, + mode: Option, +} + +impl InstallOptions { + fn parse(global: bool, agents: &[String], mode: Option<&str>) -> Result { + let targets = agents + .iter() + .map(|agent| AgentTargetId::parse(agent).map_err(CommandError::domain)) + .collect::, _>>()?; + let mode = mode + .map(InstallMode::parse) + .transpose() + .map_err(CommandError::domain)?; + Ok(Self { + scope: scope(global), + targets, + mode, + }) + } +} + +/// Install one Skill source, or restore the lockfile when SOURCE is absent. +fn install( + host: &H, + source: Option, + options: InstallOptions, + direct: bool, +) -> Result { + let scope = options.scope; + let operation = match source { + Some(source) => match (direct, InstallSource::parse(&source)) { + (true, InstallSource::Remote(source)) => { + InstallOperation::Install(InstallSource::DirectRemote(source)) + } + (true, InstallSource::DirectRemote(source)) => { + InstallOperation::Install(InstallSource::DirectRemote(source)) + } + (true, InstallSource::Local(_)) => { + return Err(CommandError::direct_local_install_source()); + } + (true, InstallSource::BundledSkilld) => { + return Err(CommandError::direct_bundled_install_source()); + } + (false, source) => InstallOperation::Install(source), + }, + None if direct => InstallOperation::DirectRestore, + None => InstallOperation::Restore, + }; + if operation == InstallOperation::Install(InstallSource::BundledSkilld) + && scope != InstallScope::Global + { + return Err(CommandError::input( + "install the skilld-maintained Skill with --global", + )); + } + let names = host.install_request(InstallRequest { + operation, + scope, + targets: options.targets, + mode: options.mode, + })?; + let mut lines = names + .into_iter() + .map(|name| Line::success(format!("Installed Skill {name}."))) + .collect::>(); + if direct { + lines.push(Line::hint("Review the unverified Skill before use.")); + } + Ok(CommandOutput::Screen(Screen::new(lines))) +} + +/// List the Skills a multi-skill ref names. An empty listing is a failure: +/// the caller asked for Skills and got none to act on. +fn list_skills(host: &H, reference: &MultiSkillRef) -> Result { + let listing = host.list_skills(reference)?; + if listing.items.is_empty() { + return Err(CommandError::operation( + "SOURCE_NOT_FOUND", + format!("{reference} names no Skills that skilld.dev lists"), + )); + } + Ok(listing) +} + fn render_view(view: SkillView) -> Result, CommandError> { let source = match view.skill.source { LockedSource::Local { path } => Line::field("Source", format!("local {path}")), @@ -1672,6 +1822,12 @@ impl Host for LocalHost { } } + fn list_skills(&self, reference: &MultiSkillRef) -> Result { + self.remote_provider()? + .list_skills(reference) + .map_err(CommandError::remote) + } + fn view(&self, name: &str, scope: InstallScope) -> Result { let known = self.known_targets(scope)?; let name = skilld_core::SkillName::parse(name.to_owned()).map_err(CommandError::domain)?; @@ -3061,12 +3217,231 @@ mod tests { } } + /// Lists two Skills for any multi-skill ref and records every install. + struct ListingHost { + installs: std::sync::Mutex>, + empty: bool, + } + + impl ListingHost { + fn new() -> Self { + Self { + installs: std::sync::Mutex::new(vec![]), + empty: false, + } + } + + fn requests(&self) -> Vec { + self.installs.lock().unwrap().clone() + } + } + + impl Host for ListingHost { + fn list(&self, _scope: InstallScope) -> Result, CommandError> { + Ok(vec![]) + } + + fn install( + &self, + _source: InstallSource, + _scope: InstallScope, + ) -> Result { + unreachable!("add installs through install_request") + } + + fn install_request(&self, request: InstallRequest) -> Result, CommandError> { + let InstallOperation::Install(InstallSource::Remote(source)) = &request.operation + else { + panic!("expected a hosted install: {:?}", request.operation); + }; + let name = source.rsplit('/').next().unwrap().to_owned(); + self.installs.lock().unwrap().push(request); + Ok(vec![name]) + } + + fn list_skills(&self, reference: &MultiSkillRef) -> Result { + let items = if self.empty { + vec![] + } else { + vec![ + skilld_core::ListedSkill { + name: "vue".to_owned(), + owner: "skilld-dev".to_owned(), + repository: "skills".to_owned(), + description: Some("Build Vue interfaces.".to_owned()), + }, + skilld_core::ListedSkill { + name: "nuxt".to_owned(), + owner: "skilld-dev".to_owned(), + repository: "skills".to_owned(), + description: None, + }, + ] + }; + Ok(SkillListing { + reference: reference.clone(), + items, + }) + } + } + + fn run_plain(host: &impl Host, args: &[&str]) -> (u8, String, String) { + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let result = run(args.iter().copied(), host, &mut stdout, &mut stderr); + ( + result.exit_code, + String::from_utf8(stdout).unwrap(), + String::from_utf8(stderr).unwrap(), + ) + } + + #[test] + fn run_prints_an_index_for_a_multi_skill_ref() { + let host = ListingHost::new(); + let (exit, stdout, stderr) = run_plain(&host, &["skilld", "run", "@harlan-zw/nuxt"]); + + assert_eq!(exit, 0, "{stderr}"); + assert_eq!( + stdout, + "vue\tskilld-dev/skills\tBuild Vue interfaces.\tnpx skilld run skilld:skilld-dev/skills/vue\n\ + nuxt\tskilld-dev/skills\t\tnpx skilld run skilld:skilld-dev/skills/nuxt\n" + ); + assert!(host.requests().is_empty(), "run must install nothing"); + } + + #[test] + fn run_index_json_names_every_skill_and_its_run_command() { + let host = ListingHost::new(); + let (exit, stdout, _) = + run_plain(&host, &["skilld", "run", "gh:skilld-dev/skills", "--json"]); + + assert_eq!(exit, 0); + let json: serde_json::Value = serde_json::from_str(&stdout).unwrap(); + assert_eq!(json["command"], "run"); + assert_eq!(json["data"]["_tag"], "index"); + assert_eq!(json["data"]["kind"], "repository"); + assert_eq!(json["data"]["reference"], "gh:skilld-dev/skills"); + assert_eq!(json["data"]["wroteSkillFiles"], false); + assert_eq!(json["data"]["total"], 2); + assert_eq!( + json["data"]["items"][0]["selector"], + "skilld:skilld-dev/skills/vue" + ); + assert_eq!( + json["data"]["items"][0]["runArgv"], + serde_json::json!(["skilld", "run", "skilld:skilld-dev/skills/vue", "--json"]) + ); + assert_eq!( + json["data"]["items"][1]["description"], + serde_json::Value::Null + ); + assert_eq!( + json["data"]["addArgv"], + serde_json::json!(["skilld", "add", "gh:skilld-dev/skills"]) + ); + } + + #[test] + fn run_rejects_file_direct_and_empty_listings_for_multi_skill_refs() { + let host = ListingHost::new(); + let (exit, stdout, stderr) = run_plain(&host, &["skilld", "run", "@harlan-zw", "--direct"]); + assert_eq!((exit, stdout.as_str()), (2, "")); + assert!(stderr.starts_with("DIRECT_SOURCE_REQUIRED:"), "{stderr}"); + + let (exit, _, stderr) = run_plain( + &host, + &["skilld", "run", "@harlan-zw", "--file", "references/api.md"], + ); + assert_eq!(exit, 2); + assert!( + stderr.contains("--file and --revision need one Skill source"), + "{stderr}" + ); + + let empty = ListingHost { + empty: true, + ..ListingHost::new() + }; + let (exit, stdout, stderr) = run_plain(&empty, &["skilld", "run", "gh:skilld-dev/empty"]); + assert_eq!((exit, stdout.as_str()), (1, "")); + assert_eq!( + stderr, + "SOURCE_NOT_FOUND: gh:skilld-dev/empty names no Skills that skilld.dev lists\n" + ); + } + + #[test] + fn add_installs_every_listed_skill_with_the_shared_install_flags() { + let host = ListingHost::new(); + let (exit, stdout, stderr) = run_plain( + &host, + &[ + "skilld", + "add", + "gh:skilld-dev/skills", + "--global", + "--agent", + "codex", + "--mode", + "symlink", + ], + ); + + assert_eq!(exit, 0, "{stderr}"); + assert_eq!(stdout, "Installed Skill vue.\nInstalled Skill nuxt.\n"); + let requests = host.requests(); + assert_eq!(requests.len(), 2); + for (request, selector) in requests.iter().zip([ + "skilld:skilld-dev/skills/vue", + "skilld:skilld-dev/skills/nuxt", + ]) { + assert_eq!( + request.operation, + InstallOperation::Install(InstallSource::Remote(selector.to_owned())) + ); + assert_eq!(request.scope, InstallScope::Global); + assert_eq!(request.targets, [AgentTargetId::Codex]); + assert_eq!(request.mode, Some(InstallMode::Symlink)); + } + } + + #[test] + fn add_with_one_skill_source_installs_like_install() { + let host = ListingHost::new(); + let (exit, stdout, stderr) = + run_plain(&host, &["skilld", "add", "skilld:skilld-dev/skills/vue"]); + + assert_eq!(exit, 0, "{stderr}"); + assert_eq!(stdout, "Installed Skill vue.\n"); + assert_eq!( + host.requests()[0].operation, + InstallOperation::Install(InstallSource::Remote( + "skilld:skilld-dev/skills/vue".to_owned() + )) + ); + } + + #[test] + fn add_rejects_ambiguous_refs_before_any_request() { + let host = ListingHost::new(); + let (exit, stdout, stderr) = + run_plain(&host, &["skilld", "add", "gh:skilld-dev/skills/vue"]); + + assert_eq!((exit, stdout.as_str()), (2, "")); + assert!( + stderr.contains("gh:OWNER/REPOSITORY names every Skill in a Repository"), + "{stderr}" + ); + assert!(host.requests().is_empty()); + } + #[test] fn public_command_vocabulary_matches_v3() { assert_eq!( command_names(), [ - "search", "install", "run", "list", "view", "remove", "update", "verify", + "search", "install", "add", "run", "list", "view", "remove", "update", "verify", "outdated", "auth", "config" ] ); diff --git a/crates/skilld-command/src/output.rs b/crates/skilld-command/src/output.rs index deaa9e67..d23d1817 100644 --- a/crates/skilld-command/src/output.rs +++ b/crates/skilld-command/src/output.rs @@ -1,6 +1,6 @@ use clap::error::ErrorKind; use serde::Serialize; -use skilld_core::UpdatePlanV1; +use skilld_core::{ListedSkill, SkillListing, UpdatePlanV1}; use skilld_ui::text::{grouped_number, is_unsafe_terminal, sanitize, width, wrap}; use skilld_ui::{Role, paint}; @@ -422,6 +422,17 @@ pub(crate) fn render_run(outcome: &RunOutcome, mode: OutputMode) -> Result render_json_success( + "run", + index_json(listing), + "Skill run output could not be encoded", + ), + (RunOutcome::Index(listing), OutputMode::Plain { .. }) => { + Ok(render_index_plain(listing).into_bytes()) + } + (RunOutcome::Index(listing), OutputMode::Human { color, .. }) => { + Ok(render_index(listing, color).into_bytes()) + } (RunOutcome::Load(skill), _) => { Ok(render_load(skill, colored(mode), command_platform(mode)).into_bytes()) } @@ -457,6 +468,106 @@ const fn command_platform(mode: OutputMode) -> CommandPlatform { } } +/// One line per Skill: the run command, then the description. +fn render_index(listing: &SkillListing, color: bool) -> String { + let reference = sanitize(&listing.reference.canonical()); + let count = listing.items.len(); + let noun = if count == 1 { "Skill" } else { "Skills" }; + let mut out = String::new(); + out.push_str(&paint( + &format!("{reference} names {count} {noun}. skilld loaded none of them."), + Role::Emphasis, + color, + )); + out.push_str("\n\n"); + for item in &listing.items { + out.push_str(&paint(&run_command(item), Role::Emphasis, color)); + if let Some(description) = &item.description { + out.push_str(" "); + out.push_str(&paint(&sanitize(description), Role::Dim, color)); + } + out.push('\n'); + } + out.push('\n'); + out.push_str("Run one command above to load that Skill for this session.\n"); + out.push_str(&format!( + "Install every Skill listed here with {}.\n", + paint( + &format!("npx skilld add {reference}"), + Role::Emphasis, + color + ) + )); + out +} + +/// Tab separated: name, owner/repository, description, run command. +fn render_index_plain(listing: &SkillListing) -> String { + let mut out = String::new(); + for item in &listing.items { + out.push_str(&escape_plain(&item.name)); + out.push('\t'); + out.push_str(&escape_plain(&format!( + "{}/{}", + item.owner, item.repository + ))); + out.push('\t'); + out.push_str(&escape_plain( + item.description.as_deref().unwrap_or_default(), + )); + out.push('\t'); + out.push_str(&escape_plain(&run_command(item))); + out.push('\n'); + } + out +} + +fn run_command(item: &ListedSkill) -> String { + format!("npx skilld run {}", item.selector()) +} + +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] +struct JsonIndexedSkill { + name: String, + owner: String, + repository: String, + description: Option, + selector: String, + run_argv: Vec, +} + +fn index_json(listing: &SkillListing) -> JsonRunData { + JsonRunData::Index { + reference: listing.reference.canonical(), + kind: listing.reference.kind(), + wrote_skill_files: false, + items: listing + .items + .iter() + .map(|item| JsonIndexedSkill { + name: item.name.clone(), + owner: item.owner.clone(), + repository: item.repository.clone(), + description: item.description.clone(), + selector: item.selector(), + run_argv: vec![ + "skilld".to_owned(), + "run".to_owned(), + item.selector(), + "--json".to_owned(), + ], + }) + .collect(), + total: listing.items.len(), + add_argv: vec![ + "skilld".to_owned(), + "add".to_owned(), + listing.reference.canonical(), + ], + } +} + fn render_load(skill: &TransientSkill, color: bool, platform: CommandPlatform) -> String { let mut out = String::new(); out.push_str(&format!( @@ -837,6 +948,14 @@ enum JsonRunData { wrote_skill_files: bool, files: Vec, }, + Index { + reference: String, + kind: &'static str, + wrote_skill_files: bool, + items: Vec, + total: usize, + add_argv: Vec, + }, } #[derive(Serialize)] diff --git a/crates/skilld-command/src/remote.rs b/crates/skilld-command/src/remote.rs index 73764498..4e23e0cb 100644 --- a/crates/skilld-command/src/remote.rs +++ b/crates/skilld-command/src/remote.rs @@ -10,11 +10,11 @@ use base64::{Engine as _, engine::general_purpose::STANDARD}; use serde::{Deserialize, Serialize}; use serde_json::json; use skilld_core::{ - ArtifactAttestation, CommitAuthor, CommitSha, CommitSummary, LockedSource, PreparedFile, - RemoteError, RemoteSelector, RepositoryVisibility, SearchResponse, SourceRef, SourceRequest, - SourceSelector, SourceStatus, TrustedRoot, TrustedRootPin, VerifiedTrustedRoot, - parse_search_response, prepare_unverified_files, verify_artifact, verify_attestation, - verify_trusted_root, + ArtifactAttestation, CommitAuthor, CommitSha, CommitSummary, ListedSkill, LockedSource, + MultiSkillRef, PreparedFile, RemoteError, RemoteSelector, RepositoryVisibility, SearchResponse, + SkillListing, SourceRef, SourceRequest, SourceSelector, SourceStatus, TrustedRoot, + TrustedRootPin, VerifiedTrustedRoot, parse_search_response, prepare_unverified_files, + verify_artifact, verify_attestation, verify_trusted_root, }; use skilld_ui::text::is_unsafe_terminal; use url::Url; @@ -22,6 +22,8 @@ use url::Url; const JSON_LIMIT: usize = 8 * 1024 * 1024; const UPDATE_RESPONSE_LIMIT: usize = 64 * 1024 * 1024; const SEARCH_LIMIT: usize = 1024 * 1024; +const LISTING_LIMIT: usize = 4 * 1024 * 1024; +const LISTING_PAGE: usize = 200; const ARTIFACT_LIMIT: usize = 64 * 1024 * 1024; const DIRECT_BLOB_LIMIT: usize = 12 * 1024 * 1024; const MAX_REDIRECTS: usize = 3; @@ -380,6 +382,15 @@ pub trait RemoteProvider: Send + Sync { &self, comparisons: &[RemoteUpdateComparison], ) -> Result, RemoteError>; + + /// List every Skill a Repository, curator, or collection names. + fn list_skills(&self, reference: &MultiSkillRef) -> Result { + let _ = reference; + Err(RemoteError::new( + "NOT_IMPLEMENTED", + "this remote provider lists no Skills", + )) + } } #[derive(Clone)] @@ -1485,7 +1496,229 @@ impl SkilldRemote { } } +/// One entry a collection names, before the Skills behind it are known. +#[derive(Clone, Debug, Eq, PartialEq)] +struct CollectionEntry { + owner: String, + repository: String, + /// `None` names every Skill the Repository carries. + name: Option, + /// The curator's one-line reason for including it. + reason: Option, +} + +/// `GET /api/skills?owner=` rows. The site returns more fields; skilld reads these. +#[derive(Deserialize)] +struct RegistrySkillPage { + items: Vec, +} + +#[derive(Deserialize)] +struct RegistrySkillRow { + name: String, + owner: String, + repo: String, + description: Option, +} + +/// `GET /api/curators/{login}`: the protocol `CuratorPayload` shape. +#[derive(Deserialize)] +struct CuratorPayload { + collections: Vec, +} + +#[derive(Deserialize)] +struct CollectionSummary { + slug: String, +} + +/// `GET /api/collections/by-author/{login}/{slug}`. The site returns more +/// fields; skilld reads the resolved Skill rows. +#[derive(Deserialize)] +struct CollectionDetail { + skills: Vec, +} + +#[derive(Deserialize)] +struct CollectionSkillRow { + owner: String, + repo: String, + name: Option, + reason: Option, +} + +impl SkilldRemote { + fn service_json Deserialize<'de>>(&self, url: Url) -> Result { + let request = HttpRequest { + method: HttpMethod::Get, + url: url.into(), + headers: vec![], + body: vec![], + response_limit: LISTING_LIMIT, + }; + let response = self.execute(request, AllowedOrigin::Service(self.endpoint.clone()))?; + parse_json(&response.body) + } + + /// Every indexed Skill one GitHub account owns. + fn owner_skills(&self, owner: &str) -> Result, RemoteError> { + let mut url = self.service_url("/api/skills")?; + url.query_pairs_mut() + .append_pair("owner", owner) + .append_pair("limit", &LISTING_PAGE.to_string()); + let page: RegistrySkillPage = self.service_json(url)?; + Ok(page + .items + .into_iter() + .filter(|row| row.owner.eq_ignore_ascii_case(owner)) + .filter_map(|row| { + listed_skill(row.owner, row.repo, row.name, row.description.as_deref()) + }) + .collect()) + } + + /// Every Skill one Repository carries, by name. + fn repository_skills( + &self, + owner: &str, + repository: &str, + ) -> Result, RemoteError> { + let mut items = self + .owner_skills(owner)? + .into_iter() + .filter(|skill| skill.repository.eq_ignore_ascii_case(repository)) + .collect::>(); + items.sort_by(|left, right| left.name.cmp(&right.name)); + Ok(items) + } + + fn collection_entries( + &self, + login: &str, + slug: &str, + ) -> Result, RemoteError> { + let path = format!( + "/api/collections/by-author/{}/{}", + path_segment(login), + path_segment(slug) + ); + let detail: CollectionDetail = + self.service_json(self.service_url(&path)?) + .map_err(|error| { + not_found_as_source( + error, + format!("skilld.dev has no collection @{login}/{slug}"), + ) + })?; + Ok(detail + .skills + .into_iter() + .map(|row| CollectionEntry { + owner: row.owner, + repository: row.repo, + name: row.name, + reason: row.reason, + }) + .collect()) + } + + fn curator_slugs(&self, login: &str) -> Result, RemoteError> { + let path = format!("/api/curators/{}", path_segment(login)); + let curator: CuratorPayload = + self.service_json(self.service_url(&path)?) + .map_err(|error| { + not_found_as_source(error, format!("skilld.dev has no curator @{login}")) + })?; + Ok(curator + .collections + .into_iter() + .map(|collection| collection.slug) + .collect()) + } + + /// Turn collection entries into listed Skills, in collection order. + /// An entry that names one Skill lists it with the curator's reason. + /// An entry that names a Repository lists every Skill it carries. + fn expand_entries( + &self, + entries: Vec, + ) -> Result, RemoteError> { + let mut seen = BTreeSet::new(); + let mut items = Vec::new(); + for entry in entries { + let expanded = match entry.name { + Some(name) => { + listed_skill(entry.owner, entry.repository, name, entry.reason.as_deref()) + .into_iter() + .collect() + } + None => self.repository_skills(&entry.owner, &entry.repository)?, + }; + for skill in expanded { + if seen.insert(skill.selector()) { + items.push(skill); + } + } + } + Ok(items) + } +} + +/// Build one listed Skill from untrusted registry fields. +/// +/// A row outside the selector contract has no `skilld:` selector, so skilld +/// could not run or install it. Listing it would print a command that fails, +/// so the row is dropped. +fn listed_skill( + owner: String, + repository: String, + name: String, + description: Option<&str>, +) -> Option { + let skill = ListedSkill { + owner, + repository, + name, + description: description + .map(|value| sanitize_line(value, 500, "")) + .filter(|value| !value.is_empty()), + }; + RemoteSelector::parse(&skill.selector()) + .is_ok() + .then_some(skill) +} + +fn not_found_as_source(error: RemoteError, message: String) -> RemoteError { + if error.code == "SERVICE_UNAVAILABLE" && error.message.ends_with("HTTP 404") { + RemoteError::new("SOURCE_NOT_FOUND", message) + } else { + error + } +} + impl RemoteProvider for SkilldRemote { + fn list_skills(&self, reference: &MultiSkillRef) -> Result { + let items = match reference { + MultiSkillRef::Repository { owner, repository } => { + self.repository_skills(owner, repository)? + } + MultiSkillRef::Collection { login, slug } => { + self.expand_entries(self.collection_entries(login, slug)?)? + } + MultiSkillRef::Curator { login } => { + let mut entries = Vec::new(); + for slug in self.curator_slugs(login)? { + entries.extend(self.collection_entries(login, &slug)?); + } + self.expand_entries(entries)? + } + }; + Ok(SkillListing { + reference: reference.clone(), + items, + }) + } + fn search(&self, query: &str, limit: u8) -> Result { let query = query.trim(); if query.is_empty() || query.len() > 200 || !(1..=50).contains(&limit) { diff --git a/crates/skilld-command/src/run.rs b/crates/skilld-command/src/run.rs index 9166d4c1..1e8ecfcb 100644 --- a/crates/skilld-command/src/run.rs +++ b/crates/skilld-command/src/run.rs @@ -11,7 +11,7 @@ use std::fs::{self, File}; use std::io::Read; use std::path::{Path, PathBuf}; -use skilld_core::PreparedFile; +use skilld_core::{PreparedFile, SkillListing}; use skilld_ui::text::is_unsafe_terminal; use crate::CommandError; @@ -109,6 +109,8 @@ pub enum FileContent { #[derive(Clone, Debug, Eq, PartialEq)] pub enum RunOutcome { Load(Box), + /// A multi-skill ref: skilld names the Skills and loads none of them. + Index(SkillListing), Files { skill: String, origin: SkillOrigin, diff --git a/crates/skilld-command/tests/add.rs b/crates/skilld-command/tests/add.rs new file mode 100644 index 00000000..f283e694 --- /dev/null +++ b/crates/skilld-command/tests/add.rs @@ -0,0 +1,161 @@ +//! `skilld add` installs every Skill a multi-skill ref names. + +use std::fs; +use std::sync::Arc; + +use sha2::{Digest, Sha256}; +use skilld_command::{ + CommandPlatform, DetectionEnvironment, Host, LocalHost, OutputContext, PreparedRemoteSkill, + RemoteLatestCommit, RemoteProvider, RemoteSourceState, RemoteUpdateComparison, + RemoteUpdateResult, run_with_output, +}; +use skilld_core::{ + CommitSha, InstallScope, ListedSkill, LockedSource, MultiSkillRef, PreparedFile, RemoteError, + RemoteSelector, SearchResponse, SkillListing, SourceSelector, SourceStatus, +}; + +/// Lists two Skills for one Repository and prepares whichever one is asked for. +struct ListingRemote; + +impl RemoteProvider for ListingRemote { + fn list_skills(&self, reference: &MultiSkillRef) -> Result { + assert_eq!( + *reference, + MultiSkillRef::Repository { + owner: "vuejs".to_owned(), + repository: "core".to_owned(), + } + ); + Ok(SkillListing { + reference: reference.clone(), + items: ["vue", "nuxt"] + .into_iter() + .map(|name| ListedSkill { + name: name.to_owned(), + owner: "vuejs".to_owned(), + repository: "core".to_owned(), + description: None, + }) + .collect(), + }) + } + + fn search(&self, _query: &str, _limit: u8) -> Result { + unimplemented!("search is outside an add") + } + + fn prepare( + &self, + selector: &RemoteSelector, + direct: bool, + ) -> Result { + assert!(!direct, "add uses hosted delivery"); + let SourceSelector::NamedSkill { name } = &selector.source().selector else { + panic!("expected a named Skill selector: {selector}"); + }; + let file = PreparedFile { + path: "SKILL.md".to_owned(), + mode: 0o644, + bytes: format!("---\nname: {name}\ndescription: Use {name}.\n---\n\n# {name}\n") + .into_bytes(), + }; + let digest = installed_digest(&file); + Ok(PreparedRemoteSkill { + files: vec![file], + locked_source: LockedSource::Remote { + source: selector.canonical(), + commit_sha: "a".repeat(40), + skill_path: format!("skills/{name}"), + }, + source_status: SourceStatus::Unverified { + content_sha256: digest.clone(), + installed_sha256: digest, + }, + }) + } + + fn prepare_exact( + &self, + _selector: &RemoteSelector, + _expected_commit: &CommitSha, + _direct: bool, + ) -> Result { + unimplemented!("exact preparation is outside an add") + } + + fn source_state( + &self, + _selector: &RemoteSelector, + _artifact_id: &str, + _commit_sha: &str, + ) -> Result { + unimplemented!("source state is outside an add") + } + + fn latest_commit( + &self, + _selector: &RemoteSelector, + _direct: bool, + ) -> Result { + unimplemented!("latest commit is outside an add") + } + + fn compare_updates( + &self, + _comparisons: &[RemoteUpdateComparison], + ) -> Result, RemoteError> { + unimplemented!("update comparison is outside an add") + } +} + +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() +} + +#[test] +fn add_installs_every_skill_the_repository_ref_names() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let host = LocalHost::new(project.clone(), temporary.path().join("global")) + .with_detection_environment(DetectionEnvironment::new(["CLAUDE_CODE".to_owned()])) + .with_remote_provider(Arc::new(ListingRemote)); + + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let result = run_with_output( + ["skilld", "add", "gh:vuejs/core"], + &host, + OutputContext::Plain { + platform: CommandPlatform::Unix, + }, + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 0, "{}", String::from_utf8_lossy(&stderr)); + assert_eq!( + String::from_utf8(stdout).unwrap(), + "Installed Skill vue.\nInstalled Skill nuxt.\n" + ); + assert_eq!(host.list(InstallScope::Project).unwrap(), ["nuxt", "vue"]); + for name in ["vue", "nuxt"] { + assert!( + project + .join(".claude/skills") + .join(name) + .join("SKILL.md") + .exists(), + "{name} reached the Agent target" + ); + } +} diff --git a/crates/skilld-command/tests/remote.rs b/crates/skilld-command/tests/remote.rs index ca8064f4..be85274b 100644 --- a/crates/skilld-command/tests/remote.rs +++ b/crates/skilld-command/tests/remote.rs @@ -18,9 +18,10 @@ use skilld_command::{ use skilld_core::{ AgentTargetId, ArtifactAttestation, ArtifactFile, AttestationSignature, CheckOutcome, CheckResult, CommitAuthor, CommitSha, CommitSummary, InstallMode, InstallOperation, - InstallRequest, InstallScope, InstallSource, LockedSource, PreparedFile, RemoteError, - RemoteSelector, RepositoryVisibility, ResolvedSource, SearchResponse, SignatureAlgorithm, - SourceProvider, SourceStatus, TrustedRootPin, UpdatePlanItem, UpdatePlanV1, UpdateRelation, + InstallRequest, InstallScope, InstallSource, ListedSkill, LockedSource, MultiSkillRef, + PreparedFile, RemoteError, RemoteSelector, RepositoryVisibility, ResolvedSource, + SearchResponse, SignatureAlgorithm, SourceProvider, SourceStatus, TrustedRootPin, + UpdatePlanItem, UpdatePlanV1, UpdateRelation, }; const ROOT_DOMAIN: &[u8] = b"skilld-trusted-key-v1\0"; @@ -388,6 +389,204 @@ fn search_remote(http: Arc) -> SkilldRemote { .with_sleeper(Arc::new(NoSleep)) } +fn listed(owner: &str, repository: &str, name: &str, description: Option<&str>) -> ListedSkill { + ListedSkill { + name: name.to_owned(), + owner: owner.to_owned(), + repository: repository.to_owned(), + description: description.map(str::to_owned), + } +} + +fn registry_page(rows: &[(&str, &str, &str, Option<&str>)]) -> HttpResponse { + let items = rows + .iter() + .map(|(owner, repo, name, description)| { + json!({ + "name": name, + "owner": owner, + "repo": repo, + "description": description, + "stars": 12, + "registryPath": format!("/gh/{owner}/{repo}/{name}"), + }) + }) + .collect::>(); + response( + 200, + serde_json::to_vec(&json!({ "items": items, "total": items.len(), "page": 1 })).unwrap(), + ) +} + +fn request_paths(http: &FakeHttp) -> Vec { + http.requests + .lock() + .unwrap() + .iter() + .map(|request| { + request + .url + .trim_start_matches("http://127.0.0.1:8787") + .to_owned() + }) + .collect() +} + +#[test] +fn a_repository_ref_lists_that_repository_from_the_owner_index() { + let http = Arc::new(FakeHttp::with([registry_page(&[ + ( + "vuejs", + "core", + "vue", + Some("Build Vue interfaces.\nSecond line."), + ), + ("vuejs", "core", "Not A Skill", Some("dropped: no selector")), + ("vuejs", "router", "vue-router", None), + ("vuejs", "core", "composition", None), + ])])); + let remote = search_remote(http.clone()); + + let listing = remote + .list_skills(&MultiSkillRef::Repository { + owner: "vuejs".to_owned(), + repository: "core".to_owned(), + }) + .unwrap(); + + assert_eq!( + listing.items, + [ + listed("vuejs", "core", "composition", None), + listed("vuejs", "core", "vue", Some("Build Vue interfaces.")), + ] + ); + assert_eq!(request_paths(&http), ["/api/skills?owner=vuejs&limit=200"]); +} + +#[test] +fn a_collection_ref_lists_its_skills_in_order_and_expands_repository_entries() { + let http = Arc::new(FakeHttp::with([ + response( + 200, + serde_json::to_vec(&json!({ + "authorLogin": "harlan-zw", + "slug": "nuxt", + "skills": [ + { "position": 0, "owner": "vuejs", "repo": "core", "name": "vue", "reason": "The reactive core." }, + { "position": 1, "owner": "nuxt", "repo": "skills", "name": null, "reason": null }, + { "position": 2, "owner": "vuejs", "repo": "core", "name": "vue", "reason": "Listed twice." }, + ] + })) + .unwrap(), + ), + registry_page(&[ + ("nuxt", "skills", "nuxt", Some("Build Nuxt apps.")), + ("nuxt", "other", "ignored", None), + ]), + ])); + let remote = search_remote(http.clone()); + + let listing = remote + .list_skills(&MultiSkillRef::Collection { + login: "harlan-zw".to_owned(), + slug: "nuxt".to_owned(), + }) + .unwrap(); + + assert_eq!( + listing.items, + [ + listed("vuejs", "core", "vue", Some("The reactive core.")), + listed("nuxt", "skills", "nuxt", Some("Build Nuxt apps.")), + ] + ); + assert_eq!( + request_paths(&http), + [ + "/api/collections/by-author/harlan-zw/nuxt", + "/api/skills?owner=nuxt&limit=200", + ] + ); +} + +#[test] +fn a_curator_ref_lists_every_collection_once() { + let collection = |name: &str| { + response( + 200, + serde_json::to_vec(&json!({ + "skills": [ + { "position": 0, "owner": "vuejs", "repo": "core", "name": name, "reason": null }, + { "position": 1, "owner": "vuejs", "repo": "core", "name": "shared", "reason": null }, + ] + })) + .unwrap(), + ) + }; + let http = Arc::new(FakeHttp::with([ + response( + 200, + serde_json::to_vec(&json!({ + "login": "harlan-zw", + "collections": [ + { "slug": "vue", "name": "Vue", "itemCount": 2 }, + { "slug": "nuxt", "name": "Nuxt", "itemCount": 2 }, + ] + })) + .unwrap(), + ), + collection("vue"), + collection("nuxt"), + ])); + let remote = search_remote(http.clone()); + + let listing = remote + .list_skills(&MultiSkillRef::Curator { + login: "harlan-zw".to_owned(), + }) + .unwrap(); + + assert_eq!( + listing.items, + [ + listed("vuejs", "core", "vue", None), + listed("vuejs", "core", "shared", None), + listed("vuejs", "core", "nuxt", None), + ] + ); + assert_eq!( + request_paths(&http), + [ + "/api/curators/harlan-zw", + "/api/collections/by-author/harlan-zw/vue", + "/api/collections/by-author/harlan-zw/nuxt", + ] + ); +} + +#[test] +fn a_missing_collection_is_a_source_not_found_error() { + let http = Arc::new(FakeHttp::with([response( + 404, + br#"{"error":true,"statusCode":404,"message":"Collection not found"}"#.to_vec(), + )])); + let remote = search_remote(http); + + let error = remote + .list_skills(&MultiSkillRef::Collection { + login: "harlan-zw".to_owned(), + slug: "missing".to_owned(), + }) + .unwrap_err(); + + assert_eq!(error.code, "SOURCE_NOT_FOUND"); + assert_eq!( + error.message, + "skilld.dev has no collection @harlan-zw/missing" + ); +} + fn skilld_selector() -> RemoteSelector { RemoteSelector::parse("skilld:skilld-dev/skilld/skilld").unwrap() } diff --git a/crates/skilld-command/tests/run.rs b/crates/skilld-command/tests/run.rs index cab56dee..26d39e79 100644 --- a/crates/skilld-command/tests/run.rs +++ b/crates/skilld-command/tests/run.rs @@ -201,7 +201,7 @@ fn load(host: &LocalHost) -> Box { .unwrap() { RunOutcome::Load(skill) => skill, - RunOutcome::Files { .. } => panic!("expected a Skill load"), + RunOutcome::Files { .. } | RunOutcome::Index(_) => panic!("expected a Skill load"), } } @@ -219,7 +219,7 @@ fn pull(host: &LocalHost, wanted: &[&str]) -> Vec { .unwrap() { RunOutcome::Files { files, .. } => files, - RunOutcome::Load(_) => panic!("expected supporting files"), + RunOutcome::Load(_) | RunOutcome::Index(_) => panic!("expected supporting files"), } } diff --git a/crates/skilld-core/src/lib.rs b/crates/skilld-core/src/lib.rs index 10d48f12..8219d228 100644 --- a/crates/skilld-core/src/lib.rs +++ b/crates/skilld-core/src/lib.rs @@ -1,4 +1,5 @@ mod lock; +mod reference; mod remote; mod target; mod update; @@ -9,6 +10,7 @@ use std::path::{Path, PathBuf}; pub use lock::{ LockDocument, LockedSkill, LockedSource, LockedTarget, SOURCE_STATUSES, SourceStatus, }; +pub use reference::{ListedSkill, MultiSkillRef, SkillListing, SkillRef}; pub use remote::{ ArtifactAttestation, ArtifactFile, AttestationSignature, CheckOutcome, CheckResult, PreparedFile, RemoteError, RemoteSelector, RepositoryVisibility, ResolvedSource, diff --git a/crates/skilld-core/src/reference.rs b/crates/skilld-core/src/reference.rs new file mode 100644 index 00000000..9207ce6d --- /dev/null +++ b/crates/skilld-core/src/reference.rs @@ -0,0 +1,267 @@ +//! Skill references for `skilld run` and `skilld add`. +//! +//! One argument names either one Skill or a set of Skills. The set forms are +//! the refs skilld.dev prints: a Repository, a curator, or a collection. +//! Parsing happens once here. Every later step trusts the tagged value. + +use std::fmt; + +use crate::remote::{RemoteError, valid_owner, valid_repository}; + +/// One parsed `skilld run` or `skilld add` argument. +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum SkillRef { + /// One Skill, in any form `skilld install` accepts. + Skill(String), + /// A ref that names more than one Skill. + Many(MultiSkillRef), +} + +/// A ref that names a set of Skills. +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum MultiSkillRef { + /// Every Skill one GitHub Repository carries: `gh:OWNER/REPOSITORY`. + Repository { owner: String, repository: String }, + /// Every Skill one curator's collections name: `@LOGIN`. + Curator { login: String }, + /// Every Skill one collection names: `@LOGIN/SLUG`. + Collection { login: String, slug: String }, +} + +/// One Skill a multi-skill ref names, as skilld.dev lists it. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct ListedSkill { + pub name: String, + pub owner: String, + pub repository: String, + pub description: Option, +} + +impl ListedSkill { + /// The hosted selector `skilld run` and `skilld install` accept. + pub fn selector(&self) -> String { + format!("skilld:{}/{}/{}", self.owner, self.repository, self.name) + } +} + +/// Every Skill one multi-skill ref names. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct SkillListing { + pub reference: MultiSkillRef, + pub items: Vec, +} + +impl SkillRef { + /// Sort one argument into a single Skill source or a multi-skill ref. + /// + /// A single Skill source passes through untouched. `skilld install` parses + /// it later, with its own rules for local paths and remote selectors. + pub fn parse(value: &str) -> Result { + let value = value.trim(); + if let Some(rest) = value.strip_prefix('@') { + return parse_handle(rest).map(Self::Many); + } + if let Some(rest) = value.strip_prefix("gh:") { + return parse_repository(rest, true).map(Self::Many); + } + if let Some(rest) = value.strip_prefix("github:") + && rest.matches('/').count() == 1 + { + return parse_repository(rest, false).map(Self::Many); + } + if value.starts_with("npm:") { + return Err(invalid( + "npm: references are not supported. Use gh:OWNER/REPOSITORY for a Repository, or skilld:OWNER/REPOSITORY/SKILL for one Skill.", + )); + } + Ok(Self::Skill(value.to_owned())) + } +} + +impl MultiSkillRef { + /// The kind word skilld prints and emits in JSON. + pub const fn kind(&self) -> &'static str { + match self { + Self::Repository { .. } => "repository", + Self::Curator { .. } => "curator", + Self::Collection { .. } => "collection", + } + } + + /// The shortest form that parses back to this ref. + pub fn canonical(&self) -> String { + match self { + Self::Repository { owner, repository } => format!("gh:{owner}/{repository}"), + Self::Curator { login } => format!("@{login}"), + Self::Collection { login, slug } => format!("@{login}/{slug}"), + } + } +} + +impl fmt::Display for MultiSkillRef { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(&self.canonical()) + } +} + +fn parse_handle(rest: &str) -> Result { + const GUIDANCE: &str = "Use @LOGIN for a curator, or @LOGIN/SLUG for one collection."; + let mut parts = rest.split('/'); + let login = parts.next().unwrap_or_default(); + if !valid_owner(login) { + return Err(invalid(format!("the curator login is invalid. {GUIDANCE}"))); + } + match (parts.next(), parts.next()) { + (None, None) => Ok(MultiSkillRef::Curator { + login: login.to_owned(), + }), + (Some(slug), None) if valid_slug(slug) => Ok(MultiSkillRef::Collection { + login: login.to_owned(), + slug: slug.to_owned(), + }), + (Some(_), None) => Err(invalid(format!( + "the collection slug is invalid. {GUIDANCE}" + ))), + _ => Err(invalid(format!( + "a collection reference has one slug. {GUIDANCE}" + ))), + } +} + +fn parse_repository(rest: &str, short_prefix: bool) -> Result { + const GUIDANCE: &str = + "Use gh:OWNER/REPOSITORY for a Repository, or skilld:OWNER/REPOSITORY/SKILL for one Skill."; + if rest.contains('#') { + return Err(invalid(format!( + "a Repository reference takes no #reference. {GUIDANCE}" + ))); + } + let mut parts = rest.split('/'); + let owner = parts.next().unwrap_or_default(); + let repository = parts + .next() + .map(|value| value.trim_end_matches(".git")) + .unwrap_or_default(); + if parts.next().is_some() { + let prefix = if short_prefix { "gh:" } else { "github:" }; + return Err(invalid(format!( + "{prefix}OWNER/REPOSITORY names every Skill in a Repository. {GUIDANCE}" + ))); + } + if !valid_owner(owner) || !valid_repository(repository) { + return Err(invalid(format!( + "the GitHub Repository owner or name is invalid. {GUIDANCE}" + ))); + } + Ok(MultiSkillRef::Repository { + owner: owner.to_owned(), + repository: repository.to_owned(), + }) +} + +fn valid_slug(value: &str) -> bool { + !value.is_empty() + && value.len() <= 100 + && !value.contains("..") + && value + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'-' | b'_' | b'.')) +} + +fn invalid(message: impl Into) -> RemoteError { + RemoteError::new("INVALID_SOURCE", message) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn many(value: &str) -> MultiSkillRef { + match SkillRef::parse(value).unwrap() { + SkillRef::Many(reference) => reference, + SkillRef::Skill(skill) => panic!("{value} parsed as one Skill: {skill}"), + } + } + + #[test] + fn parses_every_multi_skill_form() { + let repository = MultiSkillRef::Repository { + owner: "skilld-dev".to_owned(), + repository: "skills".to_owned(), + }; + assert_eq!(many("gh:skilld-dev/skills"), repository); + assert_eq!(many("gh:skilld-dev/skills.git"), repository); + assert_eq!(many("github:skilld-dev/skills"), repository); + assert_eq!( + many("@harlan-zw"), + MultiSkillRef::Curator { + login: "harlan-zw".to_owned() + } + ); + assert_eq!( + many("@harlan-zw/nuxt"), + MultiSkillRef::Collection { + login: "harlan-zw".to_owned(), + slug: "nuxt".to_owned(), + } + ); + } + + #[test] + fn single_skill_forms_pass_through_unchanged() { + for value in [ + "skilld:skilld-dev/skills/vue", + "github:skilld-dev/skilld/skills/skilld", + "github:skilld-dev/skilld/skills/skilld#branch:main", + "https://github.com/skilld-dev/skilld/tree/main/skills/skilld", + "./skills/vue", + "/srv/skills/vue", + "skilld", + ] { + assert_eq!( + SkillRef::parse(value).unwrap(), + SkillRef::Skill(value.to_owned()) + ); + } + } + + #[test] + fn rejects_ambiguous_and_unsupported_forms() { + let cases = [ + ( + "gh:skilld-dev/skills/vue", + "names every Skill in a Repository", + ), + ("gh:skilld-dev", "owner or name is invalid"), + ("gh:skilld-dev/skills#branch:main", "takes no #reference"), + ( + "github:skilld-dev/skills#branch:main", + "takes no #reference", + ), + ("@", "curator login is invalid"), + ("@harlan-zw/nuxt/extra", "one slug"), + ("@harlan-zw/", "collection slug is invalid"), + ("@-bad", "curator login is invalid"), + ("npm:vue", "npm: references are not supported"), + ("gh:skilld-dev/skills\u{1b}[0m", "owner or name is invalid"), + ("@harlan\u{202e}zw", "curator login is invalid"), + ]; + for (value, expected) in cases { + let error = SkillRef::parse(value).unwrap_err(); + assert_eq!(error.code, "INVALID_SOURCE", "{value}"); + assert!( + error.message.contains(expected), + "{value}: {}", + error.message + ); + } + } + + #[test] + fn canonical_forms_round_trip() { + for value in ["gh:skilld-dev/skills", "@harlan-zw", "@harlan-zw/nuxt"] { + assert_eq!(many(value).canonical(), value); + } + assert_eq!(many("github:a/b").canonical(), "gh:a/b"); + } +} diff --git a/crates/skilld-core/src/remote.rs b/crates/skilld-core/src/remote.rs index 87e748b8..a7942691 100644 --- a/crates/skilld-core/src/remote.rs +++ b/crates/skilld-core/src/remote.rs @@ -221,22 +221,28 @@ fn split_three(value: &str) -> Result<(&str, &str, &str), RemoteError> { } } -fn validate_source_request(source: &SourceRequest) -> Result<(), RemoteError> { - let valid_owner = !source.owner.is_empty() - && source.owner.len() <= 39 - && !source.owner.starts_with('-') - && !source.owner.ends_with('-') - && source - .owner +/// Whether a value is a valid GitHub account login. +pub(crate) fn valid_owner(value: &str) -> bool { + !value.is_empty() + && value.len() <= 39 + && !value.starts_with('-') + && !value.ends_with('-') + && value .bytes() - .all(|byte| byte.is_ascii_alphanumeric() || byte == b'-'); - let valid_repository = !source.repository.is_empty() - && source.repository.len() <= 100 - && source - .repository + .all(|byte| byte.is_ascii_alphanumeric() || byte == b'-') +} + +/// Whether a value is a valid GitHub Repository name. +pub(crate) fn valid_repository(value: &str) -> bool { + !value.is_empty() + && value.len() <= 100 + && value .bytes() - .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'-' | b'_' | b'.')); - if !valid_owner || !valid_repository { + .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'-' | b'_' | b'.')) +} + +fn validate_source_request(source: &SourceRequest) -> Result<(), RemoteError> { + if !valid_owner(&source.owner) || !valid_repository(&source.repository) { return Err(RemoteError::new( "INVALID_SOURCE", "the GitHub Repository owner or name is invalid", @@ -1098,7 +1104,7 @@ fn validate_relative_path(value: &str, maximum: usize) -> Result<(), RemoteError } } -fn is_unsafe_terminal(character: char) -> bool { +pub(crate) fn is_unsafe_terminal(character: char) -> bool { let code = u32::from(character); character.is_control() || matches!( diff --git a/docs/migrate-v2-to-v3.md b/docs/migrate-v2-to-v3.md index cb3e17d3..16e32529 100644 --- a/docs/migrate-v2-to-v3.md +++ b/docs/migrate-v2-to-v3.md @@ -128,6 +128,8 @@ It cannot restore a v2 lockfile. | v2 command | v3 replacement | | --- | --- | | `skilld add ` | Run `skilld search`, then `skilld run ` to use it once, or `skilld install ` to keep it | +| `skilld add gh:OWNER/REPOSITORY`, `skilld add @LOGIN`, `skilld add @LOGIN/SLUG` | Unchanged. `skilld run` with the same ref lists the Skills first | +| `skilld add npm:PACKAGE` | Not supported. Run `skilld search `, then use the selector | | `skilld update [name]` | `skilld update [name]` | | `skilld info` | `skilld list`, then `skilld view ` | | `skilld login` | `skilld auth login` | From be5d6931314f51aa0dab857a2c299782bb25e398 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Tue, 1 Sep 2026 16:10:04 +1000 Subject: [PATCH 2/5] docs(skill): describe multi-skill refs in the skilld Skill Claude-Session: https://claude.ai/code/session_018T67Ndp8FAjnHWthXABbJW --- skills/skilld/SKILL.md | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/skills/skilld/SKILL.md b/skills/skilld/SKILL.md index f82d648e..18bbc086 100644 --- a/skills/skilld/SKILL.md +++ b/skills/skilld/SKILL.md @@ -1,6 +1,6 @@ --- name: skilld -description: Operate skilld CLI for Skill discovery, use, installation, inspection, updates, authentication, configuration, restoration, and removal. +description: Operate skilld CLI for Skill discovery, use, installation, inspection, updates, authentication, configuration, restoration, and removal, including Repository, curator, and collection refs. --- # Use skilld CLI @@ -75,6 +75,25 @@ A `verified` status covers where the Skill came from. It does not cover what the instructions ask you to do. If the status is `unverified`, tell the user before you follow the Skill. +## List the Skills a Repository, curator, or collection names + +skilld.dev prints refs that name several Skills: + +- `gh:OWNER/REPOSITORY` names every Skill in one Repository. +- `@LOGIN` names every Skill in one curator's collections. +- `@LOGIN/SLUG` names every Skill in one collection. + +Run one of these refs to list its Skills: + +```sh +skilld run @LOGIN/SLUG --json +``` + +The command prints an index and loads no Skill. +Read `data.items` for each Skill's `name`, `owner`, `repository`, `description`, and `selector`. +Run the `data.items[].runArgv` array to load one Skill. +Pick the Skills the current task needs. Do not run every Skill in the index. + ## Choose the source Prefer the exact `skilld:` selector returned by Skill search. @@ -129,6 +148,18 @@ Install this skilld-maintained Skill globally: skilld install skilld --global ``` +Install every Skill a Repository, curator, or collection names: + +```sh +skilld add gh:OWNER/REPOSITORY +skilld add @LOGIN/SLUG --global +``` + +`skilld add` accepts `--global`, `--agent`, and `--mode` like `skilld install`. +It prints one `Installed Skill` line per Skill. +Run `skilld run` with the same ref first, then confirm the list with the user. +`skilld add` with one Skill selector installs that Skill like `skilld install`. + Always use the source selector shown by `skilld search`. After installation, report the Skill name, scope, Agent targets, and source status. From 40d2882efa0c49f1991d16ebc177d67cc5953493 Mon Sep 17 00:00:00 2001 From: Harlan GitHub Agent Date: Wed, 2 Sep 2026 03:37:26 +1000 Subject: [PATCH 3/5] fix(cli): page the owner skill index and correct README quick-start refs --- README.md | 6 ++-- crates/skilld-command/src/remote.rs | 41 ++++++++++++++++++---- crates/skilld-command/tests/remote.rs | 49 +++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 79e7db35..f959ee5c 100644 --- a/README.md +++ b/README.md @@ -94,12 +94,12 @@ skilld run skilld:skilld-dev/skills/vue --revision --file references/ap skilld install skilld:skilld-dev/skills/vue # List every Skill a Repository, curator, or collection names -skilld run gh:vuejs/core +skilld run gh:anthropics/skills skilld run @harlan-zw -skilld run @harlan-zw/vue-nuxt +skilld run @harlan-zw/agent-workflow-stack # Install every Skill one of those refs names -skilld add @harlan-zw/vue-nuxt +skilld add @harlan-zw/agent-workflow-stack # Inspect installed Skills skilld list diff --git a/crates/skilld-command/src/remote.rs b/crates/skilld-command/src/remote.rs index 4e23e0cb..f64c16e9 100644 --- a/crates/skilld-command/src/remote.rs +++ b/crates/skilld-command/src/remote.rs @@ -1511,6 +1511,8 @@ struct CollectionEntry { #[derive(Deserialize)] struct RegistrySkillPage { items: Vec, + total: Option, + pages: Option, } #[derive(Deserialize)] @@ -1562,13 +1564,21 @@ impl SkilldRemote { /// Every indexed Skill one GitHub account owns. fn owner_skills(&self, owner: &str) -> Result, RemoteError> { - let mut url = self.service_url("/api/skills")?; - url.query_pairs_mut() - .append_pair("owner", owner) - .append_pair("limit", &LISTING_PAGE.to_string()); - let page: RegistrySkillPage = self.service_json(url)?; - Ok(page - .items + let first = self.owner_skills_page(owner, 1)?; + let pages = first + .pages + .or_else(|| first.total.map(|total| total.div_ceil(LISTING_PAGE))) + .unwrap_or(1); + let mut rows = first.items; + for page in 2..=pages { + let next = self.owner_skills_page(owner, page)?; + let received = next.items.len(); + rows.extend(next.items); + if received < LISTING_PAGE { + break; + } + } + Ok(rows .into_iter() .filter(|row| row.owner.eq_ignore_ascii_case(owner)) .filter_map(|row| { @@ -1577,6 +1587,23 @@ impl SkilldRemote { .collect()) } + fn owner_skills_page( + &self, + owner: &str, + page: usize, + ) -> Result { + let mut url = self.service_url("/api/skills")?; + { + let mut pairs = url.query_pairs_mut(); + pairs.append_pair("owner", owner); + pairs.append_pair("limit", &LISTING_PAGE.to_string()); + if page > 1 { + pairs.append_pair("page", &page.to_string()); + } + } + self.service_json(url) + } + /// Every Skill one Repository carries, by name. fn repository_skills( &self, diff --git a/crates/skilld-command/tests/remote.rs b/crates/skilld-command/tests/remote.rs index be85274b..1e8cedcc 100644 --- a/crates/skilld-command/tests/remote.rs +++ b/crates/skilld-command/tests/remote.rs @@ -464,6 +464,55 @@ fn a_repository_ref_lists_that_repository_from_the_owner_index() { assert_eq!(request_paths(&http), ["/api/skills?owner=vuejs&limit=200"]); } +#[test] +fn an_owner_index_past_page_one_is_merged_into_the_listing() { + let page_one = response( + 200, + serde_json::to_vec(&json!({ + "items": [{ + "name": "padding", + "owner": "big", + "repo": "other", + "description": null, + "stars": 1, + "registryPath": "/gh/big/other/padding", + }], + "total": 300, + "page": 1, + "pages": 2, + })) + .unwrap(), + ); + let page_two = registry_page(&[ + ("big", "wanted", "zulu", None), + ("big", "wanted", "alpha", Some("On page two.")), + ]); + let http = Arc::new(FakeHttp::with([page_one, page_two])); + let remote = search_remote(http.clone()); + + let listing = remote + .list_skills(&MultiSkillRef::Repository { + owner: "big".to_owned(), + repository: "wanted".to_owned(), + }) + .unwrap(); + + assert_eq!( + listing.items, + [ + listed("big", "wanted", "alpha", Some("On page two.")), + listed("big", "wanted", "zulu", None), + ] + ); + assert_eq!( + request_paths(&http), + [ + "/api/skills?owner=big&limit=200", + "/api/skills?owner=big&limit=200&page=2", + ] + ); +} + #[test] fn a_collection_ref_lists_its_skills_in_order_and_expands_repository_entries() { let http = Arc::new(FakeHttp::with([ From f36329f31fad709ecfe0186a00a0b768e68a43b4 Mon Sep 17 00:00:00 2001 From: Harlan GitHub Agent Date: Wed, 2 Sep 2026 03:50:11 +1000 Subject: [PATCH 4/5] fix(cli): cap owner index paging and deduplicate listed Skills --- crates/skilld-command/src/remote.rs | 9 +++- crates/skilld-command/tests/remote.rs | 67 +++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/crates/skilld-command/src/remote.rs b/crates/skilld-command/src/remote.rs index f64c16e9..bb9d71f6 100644 --- a/crates/skilld-command/src/remote.rs +++ b/crates/skilld-command/src/remote.rs @@ -24,6 +24,10 @@ const UPDATE_RESPONSE_LIMIT: usize = 64 * 1024 * 1024; const SEARCH_LIMIT: usize = 1024 * 1024; const LISTING_LIMIT: usize = 4 * 1024 * 1024; const LISTING_PAGE: usize = 200; +/// Most pages one owner index may span. `pages` is server supplied, so this +/// bounds the loop when the response lies; a partial or empty page also ends +/// paging early. +const MAX_LISTING_PAGES: usize = 25; const ARTIFACT_LIMIT: usize = 64 * 1024 * 1024; const DIRECT_BLOB_LIMIT: usize = 12 * 1024 * 1024; const MAX_REDIRECTS: usize = 3; @@ -1568,7 +1572,8 @@ impl SkilldRemote { let pages = first .pages .or_else(|| first.total.map(|total| total.div_ceil(LISTING_PAGE))) - .unwrap_or(1); + .unwrap_or(1) + .min(MAX_LISTING_PAGES); let mut rows = first.items; for page in 2..=pages { let next = self.owner_skills_page(owner, page)?; @@ -1578,12 +1583,14 @@ impl SkilldRemote { break; } } + let mut seen = BTreeSet::new(); Ok(rows .into_iter() .filter(|row| row.owner.eq_ignore_ascii_case(owner)) .filter_map(|row| { listed_skill(row.owner, row.repo, row.name, row.description.as_deref()) }) + .filter(|skill| seen.insert(skill.selector())) .collect()) } diff --git a/crates/skilld-command/tests/remote.rs b/crates/skilld-command/tests/remote.rs index 1e8cedcc..60c55d76 100644 --- a/crates/skilld-command/tests/remote.rs +++ b/crates/skilld-command/tests/remote.rs @@ -513,6 +513,73 @@ fn an_owner_index_past_page_one_is_merged_into_the_listing() { ); } +/// Serves the same full page for every owner index request, whatever `page` +/// names, so only a client side page cap can end the listing. +#[derive(Default)] +struct RunawayOwnerIndexHttp { + requests: Mutex>, +} + +const RUNAWAY_PAGE_ROWS: usize = 200; +const RUNAWAY_REQUEST_LIMIT: usize = 25; + +impl HttpAdapter for RunawayOwnerIndexHttp { + fn send( + &self, + request: &HttpRequest, + _cancellation: &dyn Cancellation, + _timeout: Option, + ) -> Result { + let mut requests = self.requests.lock().unwrap(); + requests.push(request.url.to_string()); + assert!( + requests.len() <= RUNAWAY_REQUEST_LIMIT, + "the owner index kept paging: {} requests issued", + requests.len() + ); + let mut page = json!({ + "items": (0..RUNAWAY_PAGE_ROWS) + .map(|_| json!({ + "name": "wanted", + "owner": "big", + "repo": "wanted", + "description": null, + "stars": 1, + "registryPath": "/gh/big/wanted/wanted", + })) + .collect::>(), + "total": 20_000_000, + }); + if requests.len() == 1 { + page["pages"] = json!(u64::MAX); + } + Ok(response(200, serde_json::to_vec(&page).unwrap())) + } +} + +#[test] +fn a_runaway_owner_index_stops_paging_and_lists_each_skill_once() { + let http = Arc::new(RunawayOwnerIndexHttp::default()); + let remote = SkilldRemote::new( + http.clone(), + Arc::new(NoTokenProvider), + NativeRemoteConfig::Unconfigured, + ) + .with_endpoint("http://127.0.0.1:8787") + .unwrap() + .with_sleeper(Arc::new(NoSleep)); + + let listing = remote + .list_skills(&MultiSkillRef::Repository { + owner: "big".to_owned(), + repository: "wanted".to_owned(), + }) + .unwrap(); + + assert_eq!(listing.items, [listed("big", "wanted", "wanted", None)]); + assert_eq!(http.requests.lock().unwrap().len(), RUNAWAY_REQUEST_LIMIT); +} + #[test] fn a_collection_ref_lists_its_skills_in_order_and_expands_repository_entries() { let http = Arc::new(FakeHttp::with([ From 7e7acbcc7d8fb670d7aab467323030499d97f944 Mon Sep 17 00:00:00 2001 From: Harlan GitHub Agent Date: Wed, 2 Sep 2026 04:05:37 +1000 Subject: [PATCH 5/5] fix(cli): cap curator listing fanout and memoize repository fetches --- crates/skilld-command/src/remote.rs | 33 ++++++-- crates/skilld-command/tests/remote.rs | 106 ++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 7 deletions(-) diff --git a/crates/skilld-command/src/remote.rs b/crates/skilld-command/src/remote.rs index bb9d71f6..a9c71785 100644 --- a/crates/skilld-command/src/remote.rs +++ b/crates/skilld-command/src/remote.rs @@ -1,4 +1,4 @@ -use std::collections::{BTreeMap, BTreeSet}; +use std::collections::{BTreeMap, BTreeSet, HashMap}; use std::fmt; use std::sync::Arc; #[cfg(not(target_os = "wasi"))] @@ -28,6 +28,10 @@ const LISTING_PAGE: usize = 200; /// bounds the loop when the response lies; a partial or empty page also ends /// paging early. const MAX_LISTING_PAGES: usize = 25; +/// Most server supplied listing rows skilld trusts per response, mirroring +/// `MAX_LISTING_PAGES`: the curator collection list and each collection's +/// entry list are capped here so a malformed response cannot fan out. +const MAX_LISTING_ENTRIES: usize = MAX_LISTING_PAGES; const ARTIFACT_LIMIT: usize = 64 * 1024 * 1024; const DIRECT_BLOB_LIMIT: usize = 12 * 1024 * 1024; const MAX_REDIRECTS: usize = 3; @@ -1611,18 +1615,26 @@ impl SkilldRemote { self.service_json(url) } - /// Every Skill one Repository carries, by name. + /// Every Skill one Repository carries, by name. The owner index fetch is + /// memoized per Repository in `memo` for one listing, so repeated + /// collection entries naming the same Repository cost one fetch. fn repository_skills( &self, owner: &str, repository: &str, + memo: &mut HashMap<(String, String), Vec>, ) -> Result, RemoteError> { + let key = (owner.to_ascii_lowercase(), repository.to_ascii_lowercase()); + if let Some(items) = memo.get(&key) { + return Ok(items.clone()); + } let mut items = self .owner_skills(owner)? .into_iter() .filter(|skill| skill.repository.eq_ignore_ascii_case(repository)) .collect::>(); items.sort_by(|left, right| left.name.cmp(&right.name)); + memo.insert(key, items.clone()); Ok(items) } @@ -1653,6 +1665,7 @@ impl SkilldRemote { name: row.name, reason: row.reason, }) + .take(MAX_LISTING_ENTRIES) .collect()) } @@ -1676,6 +1689,7 @@ impl SkilldRemote { fn expand_entries( &self, entries: Vec, + memo: &mut HashMap<(String, String), Vec>, ) -> Result, RemoteError> { let mut seen = BTreeSet::new(); let mut items = Vec::new(); @@ -1686,7 +1700,7 @@ impl SkilldRemote { .into_iter() .collect() } - None => self.repository_skills(&entry.owner, &entry.repository)?, + None => self.repository_skills(&entry.owner, &entry.repository, memo)?, }; for skill in expanded { if seen.insert(skill.selector()) { @@ -1732,19 +1746,24 @@ fn not_found_as_source(error: RemoteError, message: String) -> RemoteError { impl RemoteProvider for SkilldRemote { fn list_skills(&self, reference: &MultiSkillRef) -> Result { + let mut memo = HashMap::new(); let items = match reference { MultiSkillRef::Repository { owner, repository } => { - self.repository_skills(owner, repository)? + self.repository_skills(owner, repository, &mut memo)? } MultiSkillRef::Collection { login, slug } => { - self.expand_entries(self.collection_entries(login, slug)?)? + self.expand_entries(self.collection_entries(login, slug)?, &mut memo)? } MultiSkillRef::Curator { login } => { let mut entries = Vec::new(); - for slug in self.curator_slugs(login)? { + for slug in self + .curator_slugs(login)? + .into_iter() + .take(MAX_LISTING_ENTRIES) + { entries.extend(self.collection_entries(login, &slug)?); } - self.expand_entries(entries)? + self.expand_entries(entries, &mut memo)? } }; Ok(SkillListing { diff --git a/crates/skilld-command/tests/remote.rs b/crates/skilld-command/tests/remote.rs index 60c55d76..54f43e6d 100644 --- a/crates/skilld-command/tests/remote.rs +++ b/crates/skilld-command/tests/remote.rs @@ -681,6 +681,112 @@ fn a_curator_ref_lists_every_collection_once() { ); } +/// Serves a curator payload with 100 collections, answers every collection +/// detail with one name-less Repository entry, and serves a runaway owner +/// index to every owner, so only client side caps and memoization can end +/// the listing. +#[derive(Default)] +struct RunawayCuratorHttp { + requests: Mutex>, +} + +const RUNAWAY_CURATOR_COLLECTIONS: usize = 100; +const RUNAWAY_CURATOR_PAGE_CAP: usize = 25; + +/// One curator payload, the capped collection details, and one memoized +/// owner index fetch for the repeated entry spanning the capped pages. +const RUNAWAY_CURATOR_REQUEST_LIMIT: usize = 1 + 2 * RUNAWAY_CURATOR_PAGE_CAP; + +impl HttpAdapter for RunawayCuratorHttp { + fn send( + &self, + request: &HttpRequest, + _cancellation: &dyn Cancellation, + _timeout: Option, + ) -> Result { + let mut requests = self.requests.lock().unwrap(); + requests.push(request.url.clone()); + assert!( + requests.len() <= RUNAWAY_CURATOR_REQUEST_LIMIT, + "the curator listing kept issuing requests: {} requests issued", + requests.len() + ); + if request.url.contains("/api/curators/") { + return Ok(response( + 200, + serde_json::to_vec(&json!({ + "login": "curator", + "collections": (0..RUNAWAY_CURATOR_COLLECTIONS) + .map(|index| { + json!({ + "slug": format!("collection-{index}"), + "name": format!("Collection {index}"), + "itemCount": 1, + }) + }) + .collect::>(), + })) + .unwrap(), + )); + } + if request.url.contains("/api/collections/by-author/") { + return Ok(response( + 200, + serde_json::to_vec(&json!({ + "skills": [{ + "position": 0, + "owner": "big", + "repo": "wanted", + "name": null, + "reason": null, + }] + })) + .unwrap(), + )); + } + let page = json!({ + "items": (0..RUNAWAY_PAGE_ROWS) + .map(|_| json!({ + "name": "wanted", + "owner": "big", + "repo": "wanted", + "description": null, + "stars": 1, + "registryPath": "/gh/big/wanted/wanted", + })) + .collect::>(), + "total": 20_000_000, + "pages": u64::MAX, + }); + Ok(response(200, serde_json::to_vec(&page).unwrap())) + } +} + +#[test] +fn a_runaway_curator_stops_fanning_out_and_fetches_one_repository_once() { + let http = Arc::new(RunawayCuratorHttp::default()); + let remote = SkilldRemote::new( + http.clone(), + Arc::new(NoTokenProvider), + NativeRemoteConfig::Unconfigured, + ) + .with_endpoint("http://127.0.0.1:8787") + .unwrap() + .with_sleeper(Arc::new(NoSleep)); + + let listing = remote + .list_skills(&MultiSkillRef::Curator { + login: "curator".to_owned(), + }) + .unwrap(); + + assert_eq!(listing.items, [listed("big", "wanted", "wanted", None)]); + assert_eq!( + http.requests.lock().unwrap().len(), + RUNAWAY_CURATOR_REQUEST_LIMIT + ); +} + #[test] fn a_missing_collection_is_a_source_not_found_error() { let http = Arc::new(FakeHttp::with([response(