diff --git a/crates/skilld-command/src/lib.rs b/crates/skilld-command/src/lib.rs index c2e1b083..976ada89 100644 --- a/crates/skilld-command/src/lib.rs +++ b/crates/skilld-command/src/lib.rs @@ -3,6 +3,7 @@ mod local_store; mod outdated; pub use outdated::{NoOutdatedProgress, OutdatedProgress, ancestor_roots}; mod output; +mod provenance; mod remote; mod run; @@ -21,6 +22,8 @@ pub use local_store::{ TargetInstall, TransactionGate, }; pub use output::{CommandPlatform, OutputContext}; +pub use provenance::RemoteProvenance; +use provenance::source_status_caution; pub use remote::{ Cancellation, HeaderValue, HttpAdapter, HttpHeader, HttpMethod, HttpRequest, HttpResponse, NativeRemoteConfig, NeverCancelled, NoRemoteProgress, NoTokenProvider, PreparedRemoteSkill, @@ -34,9 +37,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, parse_agent_targets, select_target_ids, + NotTrackedReason, SourceRef, UpdateFailure, UpdateLatestCommit, UpdateModelError, UpdatePlan, + UpdatePlanItem, UpdatePlanV1, UpdateRelation, UpdateRetryAfter, VERSION, + classify_update_comparison, parse_agent_targets, select_target_ids, }; use skilld_ui::text::is_unsafe_terminal; use skilld_ui::{Detail, Line, Marker, Screen}; @@ -201,12 +204,28 @@ enum ConfigCommand { List, } +/// One Skill an install wrote, with the source the lockfile now records. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct InstalledSkill { + pub name: String, + pub source: LockedSource, + /// `verified`, `local`, or `unverified`. + pub source_status: &'static str, +} + pub trait Host { fn list(&self, scope: InstallScope) -> Result, CommandError>; - fn install(&self, source: InstallSource, scope: InstallScope) -> Result; + fn install( + &self, + source: InstallSource, + scope: InstallScope, + ) -> Result; - fn install_request(&self, request: InstallRequest) -> Result, CommandError> { + fn install_request( + &self, + request: InstallRequest, + ) -> Result, CommandError> { if !request.targets.is_empty() || request.mode.is_some() { return Err(CommandError::unsupported_host( "Agent target selection is unavailable on this host", @@ -217,7 +236,7 @@ pub trait Host { "lockfile restore is unavailable", )); }; - self.install(source, request.scope).map(|name| vec![name]) + self.install(source, request.scope).map(|skill| vec![skill]) } fn run_skill( @@ -825,18 +844,15 @@ fn dispatch( .map(InstallMode::parse) .transpose() .map_err(CommandError::domain)?; - let names = host.install_request(InstallRequest { + let installed = 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.")); + let mut lines = Vec::new(); + for skill in &installed { + lines.extend(render_installed(skill)?); } Ok(CommandOutput::Screen(Screen::new(lines))) } @@ -959,6 +975,8 @@ fn dispatch( name: result.name, selector: selector.to_string(), description: result.description, + owner: result.source.owner, + repository: result.source.repository, stargazer_count: result.stargazer_count, }) }) @@ -996,15 +1014,48 @@ fn dispatch( } } -fn render_view(view: SkillView) -> Result, CommandError> { - let source = match view.skill.source { - LockedSource::Local { path } => Line::field("Source", format!("local {path}")), - LockedSource::BundledSkilld => Line::field("Source", "skilld-maintained Skill"), - LockedSource::Remote { source, .. } => { - let selector = RemoteSelector::parse(&source).map_err(CommandError::remote)?; - Line::linked_field("Source", selector.canonical(), github_url(&selector)?) +/// Say who published the Skill, where its bytes came from, and what the +/// status means. Every status gets its meaning; a remote source gets the +/// exact SKILL.md on GitHub. +fn render_installed(skill: &InstalledSkill) -> Result, CommandError> { + let provenance = RemoteProvenance::from_locked(&skill.source)?; + let mut lines = vec![Line::success(format!("Installed Skill {}.", skill.name))]; + if let Some(provenance) = &provenance { + lines.push(Line::item(provenance.headline(&skill.name))); + } + lines.push(source_line(&skill.source, provenance.as_ref())); + lines.push(Line::field("Source status", skill.source_status)); + lines.extend( + source_status_caution(skill.source_status) + .lines() + .map(Line::hint), + ); + if let Some(provenance) = &provenance { + lines.push(Line::linked_field( + "Read it first", + provenance.source_url.clone(), + provenance.source_url.clone(), + )); + } + Ok(lines) +} + +/// The `Source` row. A remote source links to its exact SKILL.md when the +/// terminal supports hyperlinks. +fn source_line(source: &LockedSource, provenance: Option<&RemoteProvenance>) -> Line { + match (source, provenance) { + (LockedSource::Local { path }, _) => Line::field("Source", format!("local {path}")), + (LockedSource::BundledSkilld, _) => Line::field("Source", "skilld-maintained Skill"), + (LockedSource::Remote { source, .. }, Some(provenance)) => { + Line::linked_field("Source", source, provenance.source_url.clone()) } - }; + (LockedSource::Remote { source, .. }, None) => Line::field("Source", source), + } +} + +fn render_view(view: SkillView) -> Result, CommandError> { + let provenance = RemoteProvenance::from_locked(&view.skill.source)?; + let source = source_line(&view.skill.source, provenance.as_ref()); let targets = if view.skill.targets.is_empty() { "none".to_owned() } else { @@ -1024,17 +1075,6 @@ fn render_view(view: SkillView) -> Result, CommandError> { ]) } -/// Build a GitHub repository URL from a parsed remote selector. -fn github_url(selector: &RemoteSelector) -> Result { - let mut url = url::Url::parse("https://github.com/") - .map_err(|_| CommandError::service("the GitHub Repository URL could not be built"))?; - url.path_segments_mut() - .map_err(|_| CommandError::service("the GitHub Repository URL could not be built"))? - .push(&selector.source().owner) - .push(&selector.source().repository); - Ok(url.into()) -} - fn scope(global: bool) -> InstallScope { if global { InstallScope::Global @@ -1324,7 +1364,7 @@ impl LocalHost { scope: InstallScope, targets: &[TargetInstall], known: &[ResolvedTarget], - ) -> Result { + ) -> Result { let selector = skilld_core::RemoteSelector::parse(source).map_err(CommandError::remote)?; let prepared = self .remote_provider()? @@ -1341,7 +1381,25 @@ impl LocalHost { known, ) .map_err(CommandError::store)?; - Ok(name.to_string()) + self.installed(scope, &name, known) + } + + /// Read back what the lockfile recorded for one installed Skill. + fn installed( + &self, + scope: InstallScope, + name: &skilld_core::SkillName, + known: &[ResolvedTarget], + ) -> Result { + let view = self + .store(scope) + .view(name, known) + .map_err(CommandError::store)?; + Ok(InstalledSkill { + name: view.name, + source: view.skill.source, + source_status: view.skill.source_status.as_str(), + }) } fn run_remote( @@ -1406,6 +1464,12 @@ impl LocalHost { )) .map_err(CommandError::remote)? .canonical(); + let provenance = RemoteProvenance::new( + locked_selector.source().owner.as_str(), + locked_selector.source().repository.as_str(), + skill_path.as_str(), + revision.as_str(), + )?; let source_status = prepared.source_status.as_str(); let (name, _, files) = skilld_core::prepare_unverified_files(prepared.files).map_err(CommandError::remote)?; @@ -1414,6 +1478,7 @@ impl LocalHost { source: selector.canonical(), exact_source, direct, + provenance: Box::new(provenance), }; if !wanted.is_empty() { return Ok(RunOutcome::Files { @@ -1499,7 +1564,11 @@ impl LocalHost { }))) } - fn restore(&self, request: &InstallRequest, direct: bool) -> Result, CommandError> { + fn restore( + &self, + request: &InstallRequest, + direct: bool, + ) -> Result, CommandError> { let (targets, known) = if request.targets.is_empty() { (None, self.known_targets(request.scope)?) } else { @@ -1621,7 +1690,7 @@ impl LocalHost { .map_err(CommandError::store)?; } } - restored.push(name); + restored.push(self.installed(request.scope, &skill_name, &known)?); } Ok(restored) } @@ -1633,7 +1702,11 @@ impl Host for LocalHost { self.store(scope).list(&known).map_err(CommandError::store) } - fn install(&self, source: InstallSource, scope: InstallScope) -> Result { + fn install( + &self, + source: InstallSource, + scope: InstallScope, + ) -> Result { self.install_request(InstallRequest { operation: InstallOperation::Install(source), scope, @@ -1645,7 +1718,10 @@ impl Host for LocalHost { .ok_or_else(|| CommandError::service("Skill install returned no result")) } - fn install_request(&self, request: InstallRequest) -> Result, CommandError> { + fn install_request( + &self, + request: InstallRequest, + ) -> Result, CommandError> { let source = match request.operation.clone() { InstallOperation::Restore => return self.restore(&request, false), InstallOperation::DirectRestore => return self.restore(&request, true), @@ -1655,17 +1731,17 @@ impl Host for LocalHost { match source { InstallSource::Remote(source) => self .install_remote(&source, false, request.scope, &targets, &known) - .map(|name| vec![name]), + .map(|skill| vec![skill]), InstallSource::DirectRemote(source) => self .install_remote(&source, true, request.scope, &targets, &known) - .map(|name| vec![name]), + .map(|skill| vec![skill]), source => { let (source, locked_source) = self.resolve_source(source)?; let name = self .store(request.scope) .install_from(&source, locked_source, &targets, &known) .map_err(CommandError::store)?; - Ok(vec![name.to_string()]) + Ok(vec![self.installed(request.scope, &name, &known)?]) } } } @@ -3101,20 +3177,31 @@ mod tests { &self, source: InstallSource, scope: InstallScope, - ) -> Result { + ) -> Result { assert_eq!(source, InstallSource::BundledSkilld); assert_eq!(scope, InstallScope::Global); - Ok("skilld".to_owned()) + Ok(bundled_skilld()) } - fn install_request(&self, request: InstallRequest) -> Result, CommandError> { + fn install_request( + &self, + request: InstallRequest, + ) -> Result, CommandError> { assert_eq!( request.operation, InstallOperation::Install(InstallSource::BundledSkilld) ); assert_eq!(request.scope, InstallScope::Global); assert_eq!(request.targets, [AgentTargetId::Codex]); - Ok(vec!["skilld".to_owned()]) + Ok(vec![bundled_skilld()]) + } + } + + fn bundled_skilld() -> InstalledSkill { + InstalledSkill { + name: "skilld".to_owned(), + source: LockedSource::BundledSkilld, + source_status: "local", } } @@ -3165,7 +3252,12 @@ mod tests { assert_eq!(result.exit_code, 0); assert_eq!( String::from_utf8(stdout).unwrap(), - "Installed Skill skilld.\n" + concat!( + "Installed Skill skilld.\n", + "Source: skilld-maintained Skill\n", + "Source status: local\n", + "Read this Skill before you follow it.\n", + ) ); assert!(stderr.is_empty()); } diff --git a/crates/skilld-command/src/output.rs b/crates/skilld-command/src/output.rs index deaa9e67..47d2fd8f 100644 --- a/crates/skilld-command/src/output.rs +++ b/crates/skilld-command/src/output.rs @@ -4,6 +4,7 @@ use skilld_core::UpdatePlanV1; use skilld_ui::text::{grouped_number, is_unsafe_terminal, sanitize, width, wrap}; use skilld_ui::{Role, paint}; +use crate::provenance::{RemoteProvenance, source_status_caution}; use crate::run::{FileContent, PulledFile, RunOutcome, SkillOrigin, TransientSkill}; use crate::{CommandError, CommandErrorKind}; @@ -115,9 +116,18 @@ pub(crate) struct SearchItem { pub name: String, pub selector: String, pub description: Option, + pub owner: String, + pub repository: String, pub stargazer_count: u64, } +impl SearchItem { + /// `owner/repository`, the way GitHub names it. + fn slug(&self) -> String { + format!("{}/{}", self.owner, self.repository) + } +} + pub(crate) fn render_search( outcome: &SearchOutcome, mode: OutputMode, @@ -247,6 +257,8 @@ fn render_plain(outcome: &SearchOutcome) -> String { output.push('\t'); output.push_str(&escape_plain(&item.selector)); output.push('\t'); + output.push_str(&escape_plain(&item.slug())); + output.push('\t'); output.push_str(&item.stargazer_count.to_string()); output.push('\t'); output.push_str(&escape_plain( @@ -296,13 +308,20 @@ fn render_human( for item in &outcome.items { output.push('\n'); let name = sanitize(&item.name); + let slug = sanitize(&item.slug()); let stars = format!("{} stars", grouped_number(item.stargazer_count)); - if 2 + width(&name) + 2 + width(&stars) <= columns { - let gap = columns - 2 - width(&name) - width(&stars); + let meta = format!( + "{} · {}", + paint(&slug, Role::Dim, color), + paint(&stars, Role::Warn, color) + ); + let meta_width = width(&slug) + 3 + width(&stars); + if 2 + width(&name) + 2 + meta_width <= columns { + let gap = columns - 2 - width(&name) - meta_width; output.push_str(" "); output.push_str(&paint(&name, Role::Emphasis, color)); output.push_str(&" ".repeat(gap)); - output.push_str(&paint(&stars, Role::Warn, color)); + output.push_str(&meta); output.push('\n'); } else { for line in wrap(&name, columns.saturating_sub(2)) { @@ -310,9 +329,11 @@ fn render_human( output.push_str(&paint(&line, Role::Emphasis, color)); output.push('\n'); } - output.push_str(" "); - output.push_str(&paint(&stars, Role::Warn, color)); - output.push('\n'); + for line in wrap(&format!("{slug} · {stars}"), columns.saturating_sub(2)) { + output.push_str(" "); + output.push_str(&paint(&line, Role::Dim, color)); + output.push('\n'); + } } if let Some(description) = &item.description { @@ -477,9 +498,17 @@ fn render_load(skill: &TransientSkill, color: bool, platform: CommandPlatform) - out.push_str("skilld wrote no Skill files.\n"); out.push_str(&field("Source", "skilld-maintained Skill", color)); } - SkillOrigin::Remote { source, .. } => { + SkillOrigin::Remote { + source, provenance, .. + } => { out.push_str("skilld retained no Skill files.\n"); out.push_str("It created no lockfile entry, Agent target, or project file.\n"); + out.push_str(&paint( + &sanitize(&provenance.headline(&skill.name)), + Role::Emphasis, + color, + )); + out.push('\n'); out.push_str(&field("Source", source, color)); } SkillOrigin::Local { root } => { @@ -492,6 +521,7 @@ fn render_load(skill: &TransientSkill, color: bool, platform: CommandPlatform) - } out.push_str(&field("Source status", skill.source_status, color)); out.push_str(source_status_caution(skill.source_status)); + out.push_str(&read_it_first(&skill.origin, color)); out.push('\n'); out.push_str(&paint("--- SKILL.md ---", Role::Dim, color)); @@ -588,6 +618,7 @@ fn render_files( } out.push_str(&field("Source status", source_status, color)); out.push_str(source_status_caution(source_status)); + out.push_str(&read_it_first(origin, color)); out.push('\n'); for file in files { let path = sanitize(&file.path); @@ -738,17 +769,14 @@ fn shell_quote(argument: &str, platform: CommandPlatform) -> String { } } -/// State what the status covers, on every status. -/// -/// A verified Artifact proves where the bytes came from. It says nothing about -/// what the instructions ask an Agent to do, and the output must not imply it. -fn source_status_caution(status: &str) -> &'static str { - match status { - "verified" => { - "skilld checked where this Skill came from, not what it asks you to do.\nRead it before you follow it.\n" +/// Point at the exact SKILL.md on GitHub. Local and bundled Skills sit on +/// disk already, so they get no link. +fn read_it_first(origin: &SkillOrigin, color: bool) -> String { + match origin { + SkillOrigin::Remote { provenance, .. } => { + field("Read it first", &provenance.source_url, color) } - "unverified" => "skilld did not check this source. Read this Skill before you follow it.\n", - _ => "Read this Skill before you follow it.\n", + SkillOrigin::Bundled | SkillOrigin::Local { .. } => String::new(), } } @@ -773,19 +801,51 @@ fn safe_terminal_text(value: &str) -> String { #[derive(Serialize)] #[serde(tag = "_tag", rename_all = "lowercase")] +#[serde(rename_all_fields = "camelCase")] enum JsonOrigin { - Bundled { source: &'static str }, - Remote { source: String, direct: bool }, - Local { root: String }, + Bundled { + source: &'static str, + }, + Remote { + source: String, + direct: bool, + owner: String, + repository: String, + skill_path: String, + commit: String, + source_url: String, + }, + Local { + root: String, + }, } fn origin_json(origin: &SkillOrigin) -> JsonOrigin { match origin { SkillOrigin::Bundled => JsonOrigin::Bundled { source: "skilld" }, - SkillOrigin::Remote { source, direct, .. } => JsonOrigin::Remote { - source: source.clone(), - direct: *direct, - }, + SkillOrigin::Remote { + source, + direct, + provenance, + .. + } => { + let RemoteProvenance { + owner, + repository, + skill_path, + commit_sha, + source_url, + } = provenance.as_ref(); + JsonOrigin::Remote { + source: source.clone(), + direct: *direct, + owner: owner.clone(), + repository: repository.clone(), + skill_path: skill_path.clone(), + commit: commit_sha.clone(), + source_url: source_url.clone(), + } + } SkillOrigin::Local { root } => JsonOrigin::Local { root: root.display().to_string(), }, diff --git a/crates/skilld-command/src/provenance.rs b/crates/skilld-command/src/provenance.rs new file mode 100644 index 00000000..9297d4b6 --- /dev/null +++ b/crates/skilld-command/src/provenance.rs @@ -0,0 +1,115 @@ +//! Where a remote Skill came from: the human, the Repository, and the exact file. +//! +//! Every surface that shows a remote Skill points at the SKILL.md the author +//! committed. The lockfile records the Repository, path, and commit; this module +//! turns those into one line a person can read and one URL they can open. + +use skilld_core::{LockedSource, RemoteSelector}; + +use crate::CommandError; + +/// The Repository, path, and commit that one remote Skill was read from. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct RemoteProvenance { + pub owner: String, + pub repository: String, + pub skill_path: String, + pub commit_sha: String, + /// The SKILL.md file at the exact commit, on github.com. + pub source_url: String, +} + +impl RemoteProvenance { + pub fn new( + owner: impl Into, + repository: impl Into, + skill_path: impl Into, + commit_sha: impl Into, + ) -> Result { + let owner = owner.into(); + let repository = repository.into(); + let skill_path = skill_path.into(); + let commit_sha = commit_sha.into(); + let source_url = source_url(&owner, &repository, &skill_path, &commit_sha)?; + Ok(Self { + owner, + repository, + skill_path, + commit_sha, + source_url, + }) + } + + /// Read the provenance a lockfile entry recorded. Local and bundled + /// Skills have no remote source, so they carry none. + pub fn from_locked(source: &LockedSource) -> Result, CommandError> { + let LockedSource::Remote { + source, + commit_sha, + skill_path, + } = source + else { + return Ok(None); + }; + let selector = RemoteSelector::parse(source).map_err(CommandError::remote)?; + Self::new( + selector.source().owner.as_str(), + selector.source().repository.as_str(), + skill_path.as_str(), + commit_sha.as_str(), + ) + .map(Some) + } + + /// `owner/repository`, the way GitHub names it. + pub fn slug(&self) -> String { + format!("{}/{}", self.owner, self.repository) + } + + /// The first seven characters of the commit, for reading. The URL keeps the full commit. + pub fn short_commit(&self) -> &str { + self.commit_sha.get(..7).unwrap_or(&self.commit_sha) + } + + /// One line: the Skill, the human who published it, and the commit. + pub fn headline(&self, name: &str) -> String { + format!("{name} · {} @ {}", self.slug(), self.short_commit()) + } +} + +/// State what the status covers, on every status. +/// +/// A verified Artifact proves where the bytes came from. It says nothing about +/// what the instructions ask an Agent to do, and the output must not imply it. +pub fn source_status_caution(status: &str) -> &'static str { + match status { + "verified" => { + "skilld checked where this Skill came from, not what it asks you to do.\nRead it before you follow it.\n" + } + "unverified" => "skilld did not check this source. Read this Skill before you follow it.\n", + _ => "Read this Skill before you follow it.\n", + } +} + +fn source_url( + owner: &str, + repository: &str, + skill_path: &str, + commit_sha: &str, +) -> Result { + let failed = || CommandError::service("the GitHub Skill URL could not be built"); + let mut url = url::Url::parse("https://github.com/").map_err(|_| failed())?; + { + let mut segments = url.path_segments_mut().map_err(|_| failed())?; + segments + .push(owner) + .push(repository) + .push("blob") + .push(commit_sha); + for segment in skill_path.split('/').filter(|segment| !segment.is_empty()) { + segments.push(segment); + } + segments.push(crate::run::INSTRUCTIONS_FILE); + } + Ok(url.into()) +} diff --git a/crates/skilld-command/src/run.rs b/crates/skilld-command/src/run.rs index 9166d4c1..b6da7c6e 100644 --- a/crates/skilld-command/src/run.rs +++ b/crates/skilld-command/src/run.rs @@ -15,6 +15,7 @@ use skilld_core::PreparedFile; use skilld_ui::text::is_unsafe_terminal; use crate::CommandError; +use crate::provenance::RemoteProvenance; /// The instructions file every Skill carries. pub const INSTRUCTIONS_FILE: &str = "SKILL.md"; @@ -37,6 +38,8 @@ pub enum SkillOrigin { source: String, exact_source: String, direct: bool, + /// The Repository, path, and commit the bytes came from. + provenance: Box, }, } diff --git a/crates/skilld-command/tests/agent_targets.rs b/crates/skilld-command/tests/agent_targets.rs index 932a7edf..478f4409 100644 --- a/crates/skilld-command/tests/agent_targets.rs +++ b/crates/skilld-command/tests/agent_targets.rs @@ -71,7 +71,15 @@ fn every_project_signal_selects_the_matching_agent_target() { }) .unwrap(); - assert_eq!(names, ["example"], "{}", agent.as_str()); + assert_eq!( + names + .iter() + .map(|skill| skill.name.as_str()) + .collect::>(), + ["example"], + "{}", + agent.as_str() + ); assert!( project.join(skills_dir).join("example/SKILL.md").exists(), "{}", @@ -215,6 +223,7 @@ fn a_first_party_skills_directory_alone_does_not_select_openclaw() { }) .unwrap(); + let names: Vec<&str> = names.iter().map(|skill| skill.name.as_str()).collect(); assert_eq!(names, ["example"]); let skills = fs::read_dir(project.join("skills")) .unwrap() diff --git a/crates/skilld-command/tests/outdated.rs b/crates/skilld-command/tests/outdated.rs index e01c5289..66fb42f2 100644 --- a/crates/skilld-command/tests/outdated.rs +++ b/crates/skilld-command/tests/outdated.rs @@ -436,7 +436,13 @@ fn view_and_outdated_preserve_valid_metacharacters_as_quoted_data() { assert_eq!(view.exit_code, 0); assert!(stderr.is_empty()); assert!(stdout.contains(source)); - assert!(stdout.contains("\u{1b}]8;;https://github.com/skilld-dev/skills\u{1b}\\")); + assert!( + stdout.contains(concat!( + "\u{1b}]8;;https://github.com/skilld-dev/skills/blob/", + "0123456789abcdef0123456789abcdef01234567/skills/o'hare$(%22quoted%22)/SKILL.md\u{1b}\\" + )), + "{stdout:?}" + ); let mut stdout = Vec::new(); let mut stderr = Vec::new(); diff --git a/crates/skilld-command/tests/output.rs b/crates/skilld-command/tests/output.rs index 354a2570..d695c94b 100644 --- a/crates/skilld-command/tests/output.rs +++ b/crates/skilld-command/tests/output.rs @@ -1,4 +1,6 @@ -use skilld_command::{CommandError, CommandPlatform, Host, OutputContext, run_with_output}; +use skilld_command::{ + CommandError, CommandPlatform, Host, InstalledSkill, OutputContext, run_with_output, +}; use skilld_core::{ InstallScope, InstallSource, RemoteError, SearchResponse, SearchResult, SourceProvider, SourceRequest, SourceSelector, @@ -20,7 +22,7 @@ impl Host for SearchHost { &self, _source: InstallSource, _scope: InstallScope, - ) -> Result { + ) -> Result { unreachable!("install is outside this test") } @@ -118,6 +120,8 @@ fn json_search_returns_one_versioned_document() { "name": "grill-me", "selector": "skilld:mattpocock/skills/grill-me", "description": "A focused Skill description that wraps cleanly on a narrow terminal.", + "owner": "mattpocock", + "repository": "skills", "stargazerCount": 227068 }], "total": 14 @@ -189,7 +193,7 @@ fn help_explains_primary_flow_remote_file_revisions_and_direct_delivery() { #[test] fn non_terminal_and_ci_output_are_stable_plain_records() { let expected = concat!( - "grill-me\tskilld:mattpocock/skills/grill-me\t227068\t", + "grill-me\tskilld:mattpocock/skills/grill-me\tmattpocock/skills\t227068\t", "A focused Skill description that wraps cleanly on a narrow terminal.\n" ); @@ -231,7 +235,7 @@ fn active_agent_terminal_is_plain_without_an_explicit_machine_flag() { ( 0, concat!( - "grill-me\tskilld:mattpocock/skills/grill-me\t227068\t", + "grill-me\tskilld:mattpocock/skills/grill-me\tmattpocock/skills\t227068\t", "A focused Skill description that wraps cleanly on a narrow terminal.\n" ) .to_owned(), @@ -251,7 +255,7 @@ fn explicit_plain_overrides_a_human_terminal() { assert_eq!( stdout, concat!( - "grill-me\tskilld:mattpocock/skills/grill-me\t227068\t", + "grill-me\tskilld:mattpocock/skills/grill-me\tmattpocock/skills\t227068\t", "A focused Skill description that wraps cleanly on a narrow terminal.\n" ) ); @@ -278,7 +282,7 @@ fn plain_search_escapes_record_delimiters() { assert!(stderr.is_empty()); assert_eq!( String::from_utf8(stdout).unwrap(), - "grill-me\tskilld:mattpocock/skills/grill-me\t227068\tfirst\\nsecond\\tvalue\\u{001B}\\u{202E}\n" + "grill-me\tskilld:mattpocock/skills/grill-me\tmattpocock/skills\t227068\tfirst\\nsecond\\tvalue\\u{001B}\\u{202E}\n" ); } @@ -292,6 +296,7 @@ fn human_search_is_polished_and_respects_terminal_width() { assert!(stderr.is_empty()); assert!(stdout.contains("Skill search")); assert!(stdout.contains("1 of 14 Skills")); + assert!(stdout.contains("mattpocock/skills")); assert!(stdout.contains("227,068 stars")); assert!(stdout.contains("skilld:mattpocock/skills/grill-me")); assert!(stdout.contains("skilld run")); diff --git a/crates/skilld-command/tests/remote.rs b/crates/skilld-command/tests/remote.rs index ca8064f4..846b4a74 100644 --- a/crates/skilld-command/tests/remote.rs +++ b/crates/skilld-command/tests/remote.rs @@ -2249,7 +2249,10 @@ fn remote_install_verify_and_failed_update_use_the_normal_transaction() { mode: Some(InstallMode::Copy), }; - assert_eq!(host.install_request(request).unwrap(), ["example"]); + let installed = host.install_request(request).unwrap(); + assert_eq!(installed.len(), 1); + assert_eq!(installed[0].name, "example"); + assert_eq!(installed[0].source_status, "verified"); assert_eq!( host.verify(Some("example")) .unwrap() @@ -2357,6 +2360,45 @@ fn update_check_carries_the_exact_comparison_and_commit_history() { ); } +#[test] +fn cli_install_shows_the_author_the_source_status_and_the_exact_skill_file() { + 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")) + .with_remote_provider(provider("---\nname: example\n---\n")); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run( + [ + "skilld", + "install", + "skilld:skilld-dev/skills/example", + "--agent", + "codex", + ], + &host, + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 0); + assert_eq!( + String::from_utf8(stdout).unwrap(), + concat!( + "Installed Skill example.\n", + "example · skilld-dev/skills @ 0123456\n", + "Source: skilld:skilld-dev/skills/example\n", + "Source status: verified\n", + "skilld checked where this Skill came from, not what it asks you to do.\n", + "Read it before you follow it.\n", + "Read it first: https://github.com/skilld-dev/skills/blob/0123456789abcdef0123456789abcdef01234567/skills/example/SKILL.md\n", + ) + ); + assert!(stderr.is_empty()); +} + #[test] fn cli_direct_install_marks_review_as_required() { let temporary = tempfile::tempdir().unwrap(); @@ -2384,7 +2426,14 @@ fn cli_direct_install_marks_review_as_required() { assert_eq!(result.exit_code, 0); assert_eq!( String::from_utf8(stdout).unwrap(), - "Installed Skill example.\nReview the unverified Skill before use.\n" + concat!( + "Installed Skill example.\n", + "example · skilld-dev/skills @ 0123456\n", + "Source: github:skilld-dev/skills/skills/example\n", + "Source status: unverified\n", + "skilld did not check this source. Read this Skill before you follow it.\n", + "Read it first: https://github.com/skilld-dev/skills/blob/0123456789abcdef0123456789abcdef01234567/skills/example/SKILL.md\n", + ) ); assert!(stderr.is_empty()); } @@ -2428,7 +2477,14 @@ fn cli_direct_restore_uses_the_locked_commit() { assert_eq!(restored.exit_code, 0); assert_eq!( String::from_utf8(stdout).unwrap(), - "Installed Skill example.\nReview the unverified Skill before use.\n" + concat!( + "Installed Skill example.\n", + "example · skilld-dev/skills @ 0123456\n", + "Source: github:skilld-dev/skills/skills/example#commit:0123456789abcdef0123456789abcdef01234567\n", + "Source status: unverified\n", + "skilld did not check this source. Read this Skill before you follow it.\n", + "Read it first: https://github.com/skilld-dev/skills/blob/0123456789abcdef0123456789abcdef01234567/skills/example/SKILL.md\n", + ) ); assert!(stderr.is_empty()); assert_eq!( @@ -2547,7 +2603,15 @@ fn cli_verified_restore_keeps_artifact_delivery() { assert_eq!(restored.exit_code, 0); assert_eq!( String::from_utf8(stdout).unwrap(), - "Installed Skill example.\n" + concat!( + "Installed Skill example.\n", + "example · skilld-dev/skills @ 0123456\n", + "Source: skilld:skilld-dev/skills/example#commit:0123456789abcdef0123456789abcdef01234567\n", + "Source status: verified\n", + "skilld checked where this Skill came from, not what it asks you to do.\n", + "Read it before you follow it.\n", + "Read it first: https://github.com/skilld-dev/skills/blob/0123456789abcdef0123456789abcdef01234567/skills/example/SKILL.md\n", + ) ); assert!(stderr.is_empty()); assert_eq!( diff --git a/crates/skilld-command/tests/run.rs b/crates/skilld-command/tests/run.rs index cab56dee..5503bb74 100644 --- a/crates/skilld-command/tests/run.rs +++ b/crates/skilld-command/tests/run.rs @@ -1394,3 +1394,78 @@ fn a_local_run_rejects_non_utf8_paths_and_links() { assert_eq!(non_utf8_error["error"]["code"], "INVALID_SOURCE"); assert_eq!(link_error["error"]["code"], "INVALID_SOURCE"); } + +#[test] +fn a_remote_run_names_the_author_and_links_the_exact_skill_file() { + let fixture = remote_fixture(skill_files()); + let commit = "a".repeat(40); + let url = format!("https://github.com/vuejs/core/blob/{commit}/skills/vue/SKILL.md"); + + let (exit, plain, plain_error) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld:vuejs/core/vue".to_owned(), + "--plain".to_owned(), + ], + ); + let (_, json, json_error) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld:vuejs/core/vue".to_owned(), + "--json".to_owned(), + ], + ); + let output: serde_json::Value = serde_json::from_str(&json).unwrap(); + + assert_eq!(exit, 0); + assert!(plain_error.is_empty()); + assert!(json_error.is_empty()); + assert!(plain.contains("\nvue · vuejs/core @ aaaaaaa\n"), "{plain}"); + assert!( + plain.contains( + "Source status: unverified\nskilld did not check this source. Read this Skill before you follow it.\n" + ), + "{plain}" + ); + assert!( + plain.contains(&format!("Read it first: {url}\n")), + "{plain}" + ); + assert_eq!(output["data"]["origin"]["owner"], "vuejs"); + assert_eq!(output["data"]["origin"]["repository"], "core"); + assert_eq!(output["data"]["origin"]["skillPath"], "skills/vue"); + assert_eq!(output["data"]["origin"]["commit"], commit); + assert_eq!(output["data"]["origin"]["sourceUrl"], url); + assert_eq!(output["data"]["sourceStatus"], "unverified"); +} + +#[test] +fn a_remote_file_read_links_the_exact_skill_file() { + let fixture = remote_fixture(skill_files()); + let commit = "a".repeat(40); + + let (exit, plain, error) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld:vuejs/core/vue".to_owned(), + "--revision".to_owned(), + commit.clone(), + "--file=references/api.md".to_owned(), + "--plain".to_owned(), + ], + ); + + assert_eq!(exit, 0, "{error}"); + assert!( + plain.contains(&format!( + "Read it first: https://github.com/vuejs/core/blob/{commit}/skills/vue/SKILL.md\n" + )), + "{plain}" + ); +} diff --git a/crates/skilld-native/src/main.rs b/crates/skilld-native/src/main.rs index c0204b39..a4257616 100644 --- a/crates/skilld-native/src/main.rs +++ b/crates/skilld-native/src/main.rs @@ -11,9 +11,9 @@ use std::sync::Arc; use embedded_skill::EmbeddedSkilld; use native_auth::NativeAccount; use skilld_command::{ - CommandError, CommandPlatform, DetectionEnvironment, Host, LocalHost, NativeRemoteConfig, - OutputContext, SkilldRemote, TargetRoots, interactive_update_requested, run_stdio_probe, - run_with_output, + CommandError, CommandPlatform, DetectionEnvironment, Host, InstalledSkill, LocalHost, + NativeRemoteConfig, OutputContext, SkilldRemote, TargetRoots, interactive_update_requested, + run_stdio_probe, run_with_output, }; use skilld_core::{ InstallScope, InstallSource, SearchResponse, SearchResult, SourceProvider, SourceRequest, @@ -177,7 +177,7 @@ impl Host for SearchOutputProbe { &self, _source: InstallSource, _scope: InstallScope, - ) -> Result { + ) -> Result { unreachable!("install is outside the search output probe") } diff --git a/crates/skilld-native/tests/cli.rs b/crates/skilld-native/tests/cli.rs index fcaabc56..221b2bb1 100644 --- a/crates/skilld-native/tests/cli.rs +++ b/crates/skilld-native/tests/cli.rs @@ -154,7 +154,11 @@ fn run_empty_interactive_update_in_pty(project: &Path, data: &Path, home: &Path) fn active_agent_signal_uses_plain_output_in_a_terminal() { let output = run_output_probe_in_pty(("AGENT_SESSION_ID", "test-session"), 40); - assert!(output.contains("output-probe\tskilld:skilld-dev/skilld/output-probe\t1\t")); + assert!( + output.contains( + "output-probe\tskilld:skilld-dev/skilld/output-probe\tskilld-dev/skilld\t1\t" + ) + ); assert!(!output.contains("Skill search")); } @@ -510,7 +514,15 @@ fn local_install_list_view_and_remove_use_project_state() { ); assert_eq!( String::from_utf8(install.stdout).unwrap(), - "Installed Skill local-skill.\n" + format!( + concat!( + "Installed Skill local-skill.\n", + "Source: local {}\n", + "Source status: local\n", + "Read this Skill before you follow it.\n", + ), + fixture().display() + ) ); assert_eq!( fs::read_to_string(project.join(".skills/local-skill/SKILL.md")).unwrap(), diff --git a/crates/skilld-native/tests/update_ui.rs b/crates/skilld-native/tests/update_ui.rs index 0f7f7df5..54505816 100644 --- a/crates/skilld-native/tests/update_ui.rs +++ b/crates/skilld-native/tests/update_ui.rs @@ -4,7 +4,7 @@ use std::panic::{AssertUnwindSafe, catch_unwind}; use std::rc::Rc; use std::sync::{Arc, Mutex}; -use skilld_command::{CommandError, Host}; +use skilld_command::{CommandError, Host, InstalledSkill}; use skilld_core::{ CommitAuthor, CommitHistory, CommitSha, CommitSummary as CoreCommitSummary, InstallScope, InstallSource, SkillName, UpdateFailure, UpdatePlan, UpdatePlanItem, UpdatePlanV1, @@ -410,7 +410,7 @@ impl Host for PlanHost { &self, _source: InstallSource, _scope: InstallScope, - ) -> Result { + ) -> Result { unreachable!("install is outside this test") }