diff --git a/CLAUDE.md b/CLAUDE.md index 50b74c21..e9761f15 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -24,7 +24,7 @@ cargo clippy --workspace --all-targets -- -D warnings ## Product boundary -The native `skilld` CLI searches, installs, lists, views, removes, upgrades, and verifies Skills. +The native `skilld` CLI searches, installs, lists, views, removes, updates, and verifies Skills. It also manages account authentication and Agent target configuration. The skilld CLI contains no Skill generation logic or Agent runtime. diff --git a/Cargo.lock b/Cargo.lock index 34489142..b2a5bb44 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1708,6 +1708,7 @@ dependencies = [ "sha2 0.11.0", "skilld-core", "tempfile", + "unicode-width", "url", ] @@ -1727,10 +1728,12 @@ dependencies = [ name = "skilld-native" version = "3.0.0-beta.1" dependencies = [ + "nix", "skilld-auth", "skilld-command", "skilld-core", "tempfile", + "terminal_size", "ureq", "url", ] @@ -1836,6 +1839,16 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "terminal_size" +version = "0.4.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "230a1b821ccbd75b185820a1f1ff7b14d21da1e442e22c0863ea5f08771a8874" +dependencies = [ + "rustix", + "windows-sys 0.61.2", +] + [[package]] name = "time" version = "0.3.44" @@ -1951,7 +1964,7 @@ checksum = "f2f6fb2847f6742cd76af783a2a2c49e9375d0a111c7bef6f71cd9e738c72d6e" dependencies = [ "memoffset", "tempfile", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -1960,6 +1973,12 @@ version = "1.0.24" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e6e4313cd5fcd3dad5cafa179702e2b244f760991f45397d14d4ebf38247da75" +[[package]] +name = "unicode-width" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b4ac048d71ede7ee76d585517add45da530660ef4390e49b098733c6e897f254" + [[package]] name = "untrusted" version = "0.9.0" diff --git a/Cargo.toml b/Cargo.toml index 757471f4..0ef036df 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,6 +29,8 @@ skilld-command = { path = "crates/skilld-command" } skilld-core = { path = "crates/skilld-core" } subtle = "2.6.1" tempfile = "3.27.0" +terminal_size = "0.4.4" +unicode-width = "0.2.2" url = "2.5.8" wit-bindgen = "0.60.0" zeroize = "1.9.0" diff --git a/GLOSSARY.md b/GLOSSARY.md index 55e20f4f..b08d58f8 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -19,6 +19,7 @@ Every public export, command, error, route, and document uses these terms. | Artifact attestation | `skilld.dev/api/v1` | published protocol | skilld CLI | attestation | | Check result | `skilld.dev/api/v1` | published protocol | skilld CLI, developer | check result | | 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 | | Identifier | Term | @@ -28,7 +29,8 @@ Every public export, command, error, route, and document uses these terms. | `skilld list` | installed Skills | | `skilld view` | Skill details | | `skilld remove` | Skill removal | -| `skilld upgrade` | Skill upgrade | +| `skilld update` | Skill update | +| `skilld update --check --json` | update relation check | | `skilld verify` | source verification | | `skilld install skilld --global` | global skilld Skill install | | `skilld auth login` | account login | @@ -94,7 +96,7 @@ None recorded. ### skilld CLI -**Is:** the Rust command line interface that searches, installs, upgrades, and removes Skills. +**Is:** the Rust command line interface that searches, installs, updates, and removes Skills. **Use for:** the command product and its manager logic. @@ -182,6 +184,16 @@ None recorded. **Casing:** `Source status` in headings, `sourceStatus` in identifiers. +### Update relation + +**Is:** the Git relationship between an installed Skill commit and its current source commit. + +**Use for:** `current`, `available`, `behind`, `diverged`, `pinned`, `notTracked`, or `unavailable` JSON values. + +**Never:** upgrade status, version status, release status. + +**Casing:** `Update relation` in headings, `relation` in JSON. + ### Agent target **Is:** an Agent installation destination managed by skilld. diff --git a/README.md b/README.md index bc0f864f..a67dc745 100644 --- a/README.md +++ b/README.md @@ -49,7 +49,10 @@ skilld view vue # Keep Skills current skilld verify vue -skilld upgrade vue +skilld update vue + +# Check update relations for an Agent or CI +skilld update --check --json # Remove a Skill skilld remove vue diff --git a/crates/skilld-command/Cargo.toml b/crates/skilld-command/Cargo.toml index 4e95069b..7438b487 100644 --- a/crates/skilld-command/Cargo.toml +++ b/crates/skilld-command/Cargo.toml @@ -26,6 +26,8 @@ skilld-core.workspace = true tempfile.workspace = true +unicode-width.workspace = true + url = "2.5.7" [target.'cfg(not(target_os = "wasi"))'.dependencies] diff --git a/crates/skilld-command/src/lib.rs b/crates/skilld-command/src/lib.rs index 4100a15b..3f6be267 100644 --- a/crates/skilld-command/src/lib.rs +++ b/crates/skilld-command/src/lib.rs @@ -1,5 +1,6 @@ mod config; mod local_store; +mod output; mod remote; use std::collections::BTreeSet; @@ -16,14 +17,22 @@ pub use local_store::{ AllowTransaction, LocalStore, ResolvedTarget, SkillView, StoreError, TargetInstall, TransactionGate, }; +pub use output::OutputContext; pub use remote::{ Cancellation, HeaderValue, HttpAdapter, HttpHeader, HttpMethod, HttpRequest, HttpResponse, NativeRemoteConfig, NeverCancelled, NoTokenProvider, PreparedRemoteSkill, RemoteProvider, RemoteSourceState, SecretValue, SkilldRemote, Sleeper, ThreadSleeper, TokenProvider, }; use skilld_core::{ - AGENT_TARGETS, AgentTargetId, DomainError, GlobalTargetPath, InstallMode, InstallOperation, - InstallRequest, InstallScope, InstallSource, LockedSource, VERSION, select_target_ids, + AGENT_TARGETS, AgentTargetId, CommitSha, DomainError, GlobalTargetPath, InstallMode, + InstallOperation, InstallRequest, InstallScope, InstallSource, LockedSource, NotTrackedReason, + SourceRef, UpdateCheckV1, UpdateFailure, UpdateLatestCommit, UpdateModelError, UpdatePlan, + UpdatePlanItem, UpdateRelation, VERSION, select_target_ids, +}; + +use output::{ + OutputMode, SearchItem, SearchOutcome, render_error, render_search, render_update_check, + resolve_mode, }; #[derive(Debug, Parser)] @@ -34,6 +43,12 @@ use skilld_core::{ disable_help_subcommand = true )] pub struct Cli { + /// Output stable JSON for Agents and automation. + #[arg(long, global = true, conflicts_with = "plain")] + json: bool, + /// Output stable text without terminal formatting. + #[arg(long, global = true, conflicts_with = "json")] + plain: bool, #[command(subcommand)] command: Command, } @@ -71,8 +86,13 @@ enum Command { #[arg(long)] global: bool, }, - /// Upgrade installed Skills. - Upgrade { skill: Option }, + /// Update installed Skills. + Update { + skill: Option, + /// Check update relations without changing files. + #[arg(long)] + check: bool, + }, /// Verify a Skill source. Verify { skill: Option }, /// Manage account authentication. @@ -132,7 +152,7 @@ pub trait Host { )) } - fn search(&self, _query: &str) -> Result, CommandError> { + fn search(&self, _query: &str) -> Result { Err(CommandError::unsupported_host( "Skill search is unavailable on this host", )) @@ -144,9 +164,15 @@ pub trait Host { )) } - fn upgrade(&self, _name: Option<&str>) -> Result, CommandError> { + fn update(&self, _name: Option<&str>) -> Result, CommandError> { + Err(CommandError::unsupported_host( + "Skill update is unavailable on this host", + )) + } + + fn update_check(&self, _name: Option<&str>) -> Result { Err(CommandError::unsupported_host( - "Skill upgrade is unavailable on this host", + "Skill update checks are unavailable on this host", )) } @@ -187,75 +213,90 @@ pub trait Host { } } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum CommandErrorKind { + Usage, + Operation, +} + #[derive(Clone, Debug, Eq, PartialEq)] pub struct CommandError { + pub kind: CommandErrorKind, pub code: &'static str, pub message: String, } impl CommandError { - pub fn unsupported_host(message: impl Into) -> Self { + pub fn usage(code: &'static str, message: impl Into) -> Self { Self { - code: "UNSUPPORTED_HOST", + kind: CommandErrorKind::Usage, + code, message: message.into(), } } - pub fn service(message: impl Into) -> Self { + pub fn operation(code: &'static str, message: impl Into) -> Self { Self { - code: "SERVICE_UNAVAILABLE", + kind: CommandErrorKind::Operation, + code, message: message.into(), } } + pub fn unsupported_host(message: impl Into) -> Self { + Self::operation("UNSUPPORTED_HOST", message) + } + + pub fn service(message: impl Into) -> Self { + Self::operation("SERVICE_UNAVAILABLE", message) + } + pub fn not_implemented(message: impl Into) -> Self { - Self { - code: "NOT_IMPLEMENTED", - message: message.into(), - } + Self::operation("NOT_IMPLEMENTED", message) } pub fn input(message: impl Into) -> Self { - Self { - code: "INVALID_SOURCE", - message: message.into(), - } + Self::usage("INVALID_SOURCE", message) } pub fn config(message: impl Into) -> Self { - Self { - code: "INVALID_CONFIG", - message: message.into(), - } + Self::usage("INVALID_CONFIG", message) } pub fn filesystem(message: impl Into) -> Self { - Self { - code: "SERVICE_UNAVAILABLE", - message: message.into(), - } + Self::operation("SERVICE_UNAVAILABLE", message) } pub fn domain(error: DomainError) -> Self { - Self { - code: error.code(), - message: error.to_string(), - } + Self::usage(error.code(), error.to_string()) } pub fn store(error: StoreError) -> Self { - Self { - code: error.code(), - message: error.to_string(), - } + Self::operation(error.code(), error.to_string()) } pub fn remote(error: skilld_core::RemoteError) -> Self { + let kind = if matches!( + error.code, + "INVALID_SEARCH" | "INVALID_SOURCE" | "DIRECT_SOURCE_REQUIRED" + ) { + CommandErrorKind::Usage + } else { + CommandErrorKind::Operation + }; Self { + kind, code: error.code, message: error.message, } } + + fn exit_code(&self) -> u8 { + match self.kind { + CommandErrorKind::Usage => 2, + CommandErrorKind::Operation => 1, + } + } } impl fmt::Display for CommandError { @@ -271,6 +312,12 @@ pub struct CommandResult { pub exit_code: u8, } +enum CommandOutput { + Lines(Vec), + Search(SearchOutcome), + UpdateCheck(UpdateCheckV1), +} + pub fn run(args: I, host: &H, stdout: &mut O, stderr: &mut E) -> CommandResult where I: IntoIterator, @@ -279,7 +326,31 @@ where O: Write, E: Write, { - let cli = match Cli::try_parse_from(args) { + run_with_output(args, host, OutputContext::Plain, stdout, stderr) +} + +pub fn run_with_output( + args: I, + host: &H, + context: OutputContext, + stdout: &mut O, + stderr: &mut E, +) -> CommandResult +where + I: IntoIterator, + T: Into + Clone, + H: Host, + O: Write, + E: Write, +{ + let args = args.into_iter().map(Into::into).collect::>(); + let (requested_json, requested_plain) = requested_output(&args); + let requested_mode = if requested_json && requested_plain { + OutputMode::Plain + } else { + resolve_mode(requested_json, requested_plain, context) + }; + let cli = match Cli::try_parse_from(&args) { Ok(cli) => cli, Err(error) => { let display = matches!( @@ -287,8 +358,33 @@ where ErrorKind::DisplayHelp | ErrorKind::DisplayVersion ); let target: &mut dyn Write = if display { stdout } else { stderr }; - if write!(target, "{error}").is_err() { - return CommandResult { exit_code: 2 }; + let rendered = if requested_mode == OutputMode::JsonV1 { + if display { + output::render_display( + error.kind(), + &display_path(&args), + error.to_string().trim(), + ) + } else { + let message = error + .to_string() + .lines() + .next() + .unwrap_or("invalid command arguments") + .trim_start_matches("error: ") + .to_owned(); + render_error( + &CommandError::usage("INVALID_ARGUMENT", message), + requested_mode, + ) + } + } else { + error.to_string().into_bytes() + }; + if target.write_all(&rendered).is_err() { + return CommandResult { + exit_code: if display { 1 } else { 2 }, + }; } return CommandResult { exit_code: if display { 0 } else { 2 }, @@ -296,20 +392,123 @@ where } }; + let mode = resolve_mode(cli.json, cli.plain, context); + if matches!(&cli.command, Command::Update { check: true, .. }) && mode != OutputMode::JsonV1 { + let error = CommandError::usage("UNSUPPORTED_OUTPUT", "Skill update checks need --json"); + if stderr.write_all(&render_error(&error, mode)).is_err() { + return CommandResult { exit_code: 2 }; + } + return CommandResult { exit_code: 2 }; + } + let supports_json = matches!(&cli.command, Command::Search { .. }) + || matches!(&cli.command, Command::Update { check: true, .. }); + if mode == OutputMode::JsonV1 && !supports_json { + let error = CommandError::usage( + "UNSUPPORTED_OUTPUT", + "JSON output is available for Skill search and update checks", + ); + if stderr.write_all(&render_error(&error, mode)).is_err() { + return CommandResult { exit_code: 2 }; + } + return CommandResult { exit_code: 2 }; + } + match dispatch(cli.command, host) { - Ok(lines) => { + Ok(CommandOutput::Lines(lines)) => { + let mut bytes = Vec::new(); for line in lines { - if writeln!(stdout, "{line}").is_err() { - return CommandResult { exit_code: 2 }; - } + bytes.extend_from_slice(line.as_bytes()); + bytes.push(b'\n'); } - CommandResult { exit_code: 0 } + write_success(&bytes, mode, stdout, stderr) } + Ok(CommandOutput::Search(outcome)) => match render_search(&outcome, mode) { + Ok(bytes) => write_success(&bytes, mode, stdout, stderr), + Err(error) => { + if stderr.write_all(&render_error(&error, mode)).is_err() { + return CommandResult { + exit_code: error.exit_code(), + }; + } + CommandResult { + exit_code: error.exit_code(), + } + } + }, + Ok(CommandOutput::UpdateCheck(outcome)) => match render_update_check(&outcome, mode) { + Ok(bytes) => write_success(&bytes, mode, stdout, stderr), + Err(error) => { + if stderr.write_all(&render_error(&error, mode)).is_err() { + return CommandResult { + exit_code: error.exit_code(), + }; + } + CommandResult { + exit_code: error.exit_code(), + } + } + }, Err(error) => { - if writeln!(stderr, "{error}").is_err() { - return CommandResult { exit_code: 2 }; + if stderr.write_all(&render_error(&error, mode)).is_err() { + return CommandResult { + exit_code: error.exit_code(), + }; } - CommandResult { exit_code: 2 } + CommandResult { + exit_code: error.exit_code(), + } + } + } +} + +fn requested_output(args: &[OsString]) -> (bool, bool) { + let mut json = false; + let mut plain = false; + for argument in args.iter().skip(1) { + if argument == "--" { + break; + } + if argument == "--json" { + json = true; + } else if argument == "--plain" { + plain = true; + } + } + (json, plain) +} + +fn display_path(args: &[OsString]) -> String { + let commands = [ + "search", "install", "list", "view", "remove", "update", "verify", "auth", "config", + ]; + let mut path = vec!["skilld"]; + if let Some(command) = args + .iter() + .skip(1) + .filter_map(|argument| argument.to_str()) + .find(|argument| commands.contains(argument)) + { + path.push(command); + } + path.join(" ") +} + +fn write_success( + bytes: &[u8], + mode: OutputMode, + stdout: &mut O, + stderr: &mut E, +) -> CommandResult { + match stdout.write_all(bytes) { + Ok(()) => CommandResult { exit_code: 0 }, + Err(error) if error.kind() == std::io::ErrorKind::BrokenPipe => { + CommandResult { exit_code: 0 } + } + Err(_) => { + let error = + CommandError::operation("OUTPUT_WRITE_FAILED", "Skill output could not be written"); + let _ = stderr.write_all(&render_error(&error, mode)); + CommandResult { exit_code: 1 } } } } @@ -337,7 +536,7 @@ pub fn run_stdio_probe( } } -fn dispatch(command: Command, host: &H) -> Result, CommandError> { +fn dispatch(command: Command, host: &H) -> Result { match command { Command::Install { source, @@ -391,70 +590,88 @@ fn dispatch(command: Command, host: &H) -> Result, CommandE if direct { lines.push("Review the unverified Skill before use.".to_owned()); } - Ok(lines) + Ok(CommandOutput::Lines(lines)) + } + Command::List { global } => host.list(scope(global)).map(CommandOutput::Lines), + Command::View { skill, global } => { + render_view(host.view(&skill, scope(global))?).map(CommandOutput::Lines) } - Command::List { global } => host.list(scope(global)), - Command::View { skill, global } => render_view(host.view(&skill, scope(global))?), Command::Remove { skill, global } => { host.remove(&skill, scope(global))?; - Ok(vec![format!("Removed Skill {skill}.")]) + Ok(CommandOutput::Lines(vec![format!( + "Removed Skill {skill}." + )])) } Command::Auth { command: AuthCommand::Status, - } => Ok(vec![if host.auth_status()? { + } => Ok(CommandOutput::Lines(vec![if host.auth_status()? { "Authenticated.".to_owned() } else { "Not authenticated.".to_owned() - }]), + }])), Command::Auth { command: AuthCommand::Login, } => { host.auth_login()?; - Ok(vec!["Authentication started.".to_owned()]) + Ok(CommandOutput::Lines(vec![ + "Authentication started.".to_owned(), + ])) } Command::Auth { command: AuthCommand::Logout, } => { host.auth_logout()?; - Ok(vec!["Logged out.".to_owned()]) + Ok(CommandOutput::Lines(vec!["Logged out.".to_owned()])) } Command::Config { command: ConfigCommand::Get { key }, - } => Ok(vec![host.config_get(&key)?]), + } => Ok(CommandOutput::Lines(vec![host.config_get(&key)?])), Command::Config { command: ConfigCommand::Set { key, value }, } => { host.config_set(&key, &value)?; - Ok(vec![format!("Set {key}.")]) + Ok(CommandOutput::Lines(vec![format!("Set {key}.")])) } Command::Config { command: ConfigCommand::List, - } => host.config_list(), - Command::Search { query } => host - .search(&query.join(" "))? - .into_iter() - .map(|result| { - let selector = result.selector().map_err(CommandError::remote)?; - let description = result - .description - .unwrap_or_default() - .chars() - .map(|character| { - if character.is_control() { - ' ' - } else { - character - } + } => host.config_list().map(CommandOutput::Lines), + Command::Search { query } => { + let query = query.join(" ").trim().to_owned(); + if query.is_empty() || query.len() > 200 { + return Err(CommandError::usage( + "INVALID_SEARCH", + "Skill search needs a query up to 200 bytes", + )); + } + let response = host.search(&query)?; + let items = response + .items + .into_iter() + .map(|result| { + let selector = result.selector().map_err(CommandError::remote)?; + Ok(SearchItem { + name: result.name, + selector: selector.to_string(), + description: result.description, + stargazer_count: result.stargazer_count, }) - .collect::(); - Ok(format!( - "{}\t{}\t{}\t{} stars", - result.name, selector, description, result.stargazer_count - )) - }) - .collect(), - Command::Upgrade { skill } => host.upgrade(skill.as_deref()), - Command::Verify { skill } => host.verify(skill.as_deref()), + }) + .collect::, CommandError>>()?; + Ok(CommandOutput::Search(SearchOutcome { + query, + items, + total: response.total, + })) + } + Command::Update { skill, check } => { + if check { + host.update_check(skill.as_deref()) + .map(CommandOutput::UpdateCheck) + } else { + host.update(skill.as_deref()).map(CommandOutput::Lines) + } + } + Command::Verify { skill } => host.verify(skill.as_deref()).map(CommandOutput::Lines), } } @@ -729,6 +946,144 @@ impl LocalHost { }) } + fn update_relation( + &self, + skill: &skilld_core::LockedSkill, + ) -> Result { + let (source, locked_commit_sha) = match &skill.source { + LockedSource::Local { .. } => { + return Ok(UpdateRelation::NotTracked { + reason: NotTrackedReason::Local, + }); + } + LockedSource::BundledSkilld => { + return Ok(UpdateRelation::NotTracked { + reason: NotTrackedReason::Bundled, + }); + } + LockedSource::Remote { + source, commit_sha, .. + } => ( + source, + CommitSha::parse(commit_sha.clone()).map_err(update_model_error)?, + ), + }; + + let selector = match skilld_core::RemoteSelector::parse(source) { + Ok(selector) => selector, + Err(error) => { + return Ok(unavailable_update( + locked_commit_sha, + UpdateLatestCommit::Unknown, + error.code, + error.message, + )); + } + }; + if let Some(SourceRef::Commit { value }) = &selector.source().r#ref { + let pinned_commit_sha = match CommitSha::parse(value.clone()) { + Ok(commit_sha) => commit_sha, + Err(error) => { + return Ok(unavailable_update( + locked_commit_sha, + UpdateLatestCommit::Unknown, + "INVALID_SOURCE", + error.to_string(), + )); + } + }; + if pinned_commit_sha != locked_commit_sha { + return Ok(unavailable_update( + locked_commit_sha, + UpdateLatestCommit::Known { + commit_sha: pinned_commit_sha, + }, + "INVALID_LOCKFILE", + "the locked commit differs from its source selector", + )); + } + return Ok(UpdateRelation::Pinned { + commit_sha: locked_commit_sha, + }); + } + + let artifact_id = match &skill.source_status { + skilld_core::SourceStatus::Verified { artifact_id, .. } => artifact_id, + skilld_core::SourceStatus::Unverified { .. } => { + return Ok(unavailable_update( + locked_commit_sha, + UpdateLatestCommit::Unknown, + "UNVERIFIED_SOURCE", + "run an explicit --direct install to update this Skill", + )); + } + skilld_core::SourceStatus::Local { .. } => { + return Ok(unavailable_update( + locked_commit_sha, + UpdateLatestCommit::Unknown, + "INVALID_LOCKFILE", + "the remote Skill has a local source status", + )); + } + }; + let provider = match self.remote_provider() { + Ok(provider) => provider, + Err(error) => { + return Ok(unavailable_update( + locked_commit_sha, + UpdateLatestCommit::Unknown, + error.code, + error.message, + )); + } + }; + let state = match provider.source_state(&selector, artifact_id, locked_commit_sha.as_str()) + { + Ok(state) => state, + Err(error) => { + return Ok(unavailable_update( + locked_commit_sha, + UpdateLatestCommit::Unknown, + error.code, + error.message, + )); + } + }; + match state { + RemoteSourceState::Current => Ok(UpdateRelation::Current { + commit_sha: locked_commit_sha, + }), + RemoteSourceState::Stale { + current_commit_sha, .. + } => { + let latest_commit_sha = match CommitSha::parse(current_commit_sha) { + Ok(commit_sha) => commit_sha, + Err(error) => { + return Ok(unavailable_update( + locked_commit_sha, + UpdateLatestCommit::Unknown, + "INVALID_RESPONSE", + error.to_string(), + )); + } + }; + if latest_commit_sha == locked_commit_sha { + return Ok(UpdateRelation::Current { + commit_sha: locked_commit_sha, + }); + } + Ok(unavailable_update( + locked_commit_sha, + UpdateLatestCommit::Known { + commit_sha: latest_commit_sha, + }, + "COMPARISON_UNAVAILABLE", + "skilld.dev does not provide Git comparison data", + )) + } + } + } + fn install_remote( &self, source: &str, @@ -766,13 +1121,13 @@ impl LocalHost { let store = self.store(request.scope); let names = store.list(&known).map_err(CommandError::store)?; if names.is_empty() { - return Err(CommandError { - code: "LOCKFILE_NOT_FOUND", - message: format!( + return Err(CommandError::operation( + "LOCKFILE_NOT_FOUND", + format!( "no installed Skills exist in {} scope", request.scope.as_str() ), - }); + )); } let mut restored = Vec::new(); for name in names { @@ -843,12 +1198,11 @@ impl LocalHost { skilld_core::SourceStatus::Verified { .. } => false, skilld_core::SourceStatus::Unverified { .. } if direct => true, _ => { - return Err(CommandError { - code: "UNVERIFIED_SOURCE", - message: - "run skilld install --direct to restore an unverified Skill" - .to_owned(), - }); + return Err(CommandError::operation( + "UNVERIFIED_SOURCE", + "run skilld install --direct to restore an unverified Skill" + .to_owned(), + )); } }; let selector = skilld_core::RemoteSelector::parse(&source) @@ -980,7 +1334,7 @@ impl Host for LocalHost { .logout() } - fn search(&self, query: &str) -> Result, CommandError> { + fn search(&self, query: &str) -> Result { self.remote_provider()? .search(query, 20) .map_err(CommandError::remote) @@ -997,10 +1351,9 @@ impl Host for LocalHost { let view = store .verify_content(&name, &known) .map_err(|error| match error { - StoreError::Conflict(message) => CommandError { - code: "CONTENT_CHANGED", - message, - }, + StoreError::Conflict(message) => { + CommandError::operation("CONTENT_CHANGED", message) + } error => CommandError::store(error), })?; match (&view.skill.source, &view.skill.source_status) { @@ -1021,21 +1374,18 @@ impl Host for LocalHost { lines.push(format!("Verified Skill {}.", name.as_str())); } RemoteSourceState::Stale { .. } => { - return Err(CommandError { - code: "SOURCE_STALE", - message: format!( - "Skill {} has a newer or changed source", - name.as_str() - ), - }); + return Err(CommandError::operation( + "SOURCE_STALE", + format!("Skill {} has a newer or changed source", name.as_str()), + )); } } } (_, skilld_core::SourceStatus::Unverified { .. }) => { - return Err(CommandError { - code: "UNVERIFIED_SOURCE", - message: format!("Skill {} has an unverified source", name.as_str()), - }); + return Err(CommandError::operation( + "UNVERIFIED_SOURCE", + format!("Skill {} has an unverified source", name.as_str()), + )); } _ => lines.push(format!("Checked local Skill {}.", name.as_str())), } @@ -1043,12 +1393,12 @@ impl Host for LocalHost { Ok(lines) } - fn upgrade(&self, requested: Option<&str>) -> Result, CommandError> { + fn update(&self, requested: Option<&str>) -> Result, CommandError> { let scope = InstallScope::Project; let known = self.known_targets(scope)?; let store = self.store(scope); let names = selected_names(&store, &known, requested)?; - let mut upgraded = Vec::new(); + let mut updated = Vec::new(); for name in names { let skill_name = skilld_core::SkillName::parse(name.clone()).map_err(CommandError::domain)?; @@ -1062,10 +1412,10 @@ impl Host for LocalHost { view.skill.source_status, skilld_core::SourceStatus::Verified { .. } ) { - return Err(CommandError { - code: "UNVERIFIED_SOURCE", - message: format!("Skill {name} needs another explicit --direct install"), - }); + return Err(CommandError::operation( + "UNVERIFIED_SOURCE", + format!("Skill {name} needs another explicit --direct install"), + )); } let selector = skilld_core::RemoteSelector::parse(source).map_err(CommandError::remote)?; @@ -1077,10 +1427,10 @@ impl Host for LocalHost { let staged_name = skilld_core::SkillName::from_source(staged.path()).map_err(CommandError::domain)?; if staged_name != skill_name { - return Err(CommandError { - code: "SOURCE_MISMATCH", - message: format!("the upgraded Skill name changed from {name}"), - }); + return Err(CommandError::operation( + "SOURCE_MISMATCH", + format!("the updated Skill name changed from {name}"), + )); } let targets = view .skill @@ -1111,9 +1461,27 @@ impl Host for LocalHost { &known, ) .map_err(CommandError::store)?; - upgraded.push(format!("Upgraded Skill {name}.")); + updated.push(format!("Updated Skill {name}.")); + } + Ok(updated) + } + + fn update_check(&self, requested: Option<&str>) -> Result { + let scope = InstallScope::Project; + let known = self.known_targets(scope)?; + let store = self.store(scope); + let names = selected_names(&store, &known, requested)?; + let mut items = Vec::with_capacity(names.len()); + for name in names { + let skill_name = skilld_core::SkillName::parse(name).map_err(CommandError::domain)?; + let view = store + .view(&skill_name, &known) + .map_err(CommandError::store)?; + let relation = self.update_relation(&view.skill)?; + items.push(UpdatePlanItem::new(skill_name, relation)); } - Ok(upgraded) + let plan = UpdatePlan::new(items).map_err(update_model_error)?; + Ok(UpdateCheckV1::new(plan)) } } @@ -1183,6 +1551,23 @@ fn selected_names( } } +fn unavailable_update( + locked_commit_sha: CommitSha, + latest_commit: UpdateLatestCommit, + code: impl Into, + message: impl Into, +) -> UpdateRelation { + UpdateRelation::Unavailable { + locked_commit_sha, + latest_commit, + failure: UpdateFailure::new(code, message), + } +} + +fn update_model_error(error: UpdateModelError) -> CommandError { + CommandError::operation("INVALID_LOCKFILE", error.to_string()) +} + fn detects_environment(agent: AgentTargetId, environment: &DetectionEnvironment) -> bool { match agent { AgentTargetId::ClaudeCode => [ @@ -1343,12 +1728,31 @@ mod tests { assert_eq!( command_names(), [ - "search", "install", "list", "view", "remove", "upgrade", "verify", "auth", - "config" + "search", "install", "list", "view", "remove", "update", "verify", "auth", "config" ] ); } + #[test] + fn upgrade_is_not_a_command_alias() { + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let result = run( + ["skilld", "upgrade"], + &RecordingHost, + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 2); + assert!(stdout.is_empty()); + assert!( + String::from_utf8(stderr) + .unwrap() + .contains("unrecognized subcommand 'upgrade'") + ); + } + #[test] fn global_skilld_install_uses_the_target_contract() { let mut stdout = Vec::new(); diff --git a/crates/skilld-command/src/output.rs b/crates/skilld-command/src/output.rs new file mode 100644 index 00000000..767b27ec --- /dev/null +++ b/crates/skilld-command/src/output.rs @@ -0,0 +1,388 @@ +use clap::error::ErrorKind; +use serde::Serialize; +use skilld_core::UpdateCheckV1; +use unicode_width::{UnicodeWidthChar, UnicodeWidthStr}; + +use crate::{CommandError, CommandErrorKind}; + +const JSON_SCHEMA_VERSION: u8 = 1; +const MIN_WIDTH: u16 = 20; +const MAX_WIDTH: u16 = 240; + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum OutputContext { + HumanTerminal { width: u16, color: bool }, + Plain, +} + +impl OutputContext { + pub fn auto( + stdout_is_terminal: bool, + active_agent: bool, + ci: bool, + no_color: bool, + term_is_dumb: bool, + width: u16, + ) -> Self { + if active_agent || ci || !stdout_is_terminal { + return Self::Plain; + } + Self::HumanTerminal { + width: width.clamp(MIN_WIDTH, MAX_WIDTH), + color: !no_color && !term_is_dumb, + } + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum OutputMode { + Human { width: u16, color: bool }, + Plain, + JsonV1, +} + +pub(crate) fn resolve_mode(json: bool, plain: bool, context: OutputContext) -> OutputMode { + if json { + OutputMode::JsonV1 + } else if plain { + OutputMode::Plain + } else { + match context { + OutputContext::HumanTerminal { width, color } => OutputMode::Human { width, color }, + OutputContext::Plain => OutputMode::Plain, + } + } +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub(crate) struct SearchOutcome { + pub query: String, + pub items: Vec, + pub total: u64, +} + +#[derive(Clone, Debug, Eq, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub(crate) struct SearchItem { + pub name: String, + pub selector: String, + pub description: Option, + pub stargazer_count: u64, +} + +pub(crate) fn render_search( + outcome: &SearchOutcome, + mode: OutputMode, +) -> Result, CommandError> { + match mode { + OutputMode::Human { width, color } => Ok(render_human(outcome, width, color).into_bytes()), + OutputMode::Plain => Ok(render_plain(outcome).into_bytes()), + OutputMode::JsonV1 => render_json_success( + "search", + JsonSearchData { + query: &outcome.query, + items: &outcome.items, + total: outcome.total, + }, + "Skill search output could not be encoded", + ), + } +} + +pub(crate) fn render_display(kind: ErrorKind, path: &str, text: &str) -> Vec { + let (command, data) = if kind == ErrorKind::DisplayVersion { + ( + "version", + JsonDisplayData::Version { + name: "skilld", + version: skilld_core::VERSION, + }, + ) + } else { + ("help", JsonDisplayData::Help { path, text }) + }; + render_json_success(command, data, "display output could not be encoded") + .unwrap_or_else(|_| b"OUTPUT_RENDER_FAILED: display output could not be encoded\n".to_vec()) +} + +pub(crate) fn render_update_check( + outcome: &UpdateCheckV1, + mode: OutputMode, +) -> Result, CommandError> { + if mode != OutputMode::JsonV1 { + return Err(CommandError::service( + "Skill update check output needs JSON mode", + )); + } + render_json_success( + "update", + outcome, + "Skill update check output could not be encoded", + ) +} + +fn render_json_success( + command: &'static str, + data: T, + encoding_error: &'static str, +) -> Result, CommandError> { + serde_json::to_vec(&JsonSuccess { + schema_version: JSON_SCHEMA_VERSION, + tag: "Success", + command, + data, + notices: Vec::new(), + }) + .map(|mut bytes| { + bytes.push(b'\n'); + bytes + }) + .map_err(|_| CommandError::service(encoding_error)) +} + +pub(crate) fn render_error(error: &CommandError, mode: OutputMode) -> Vec { + if mode == OutputMode::JsonV1 { + return serde_json::to_vec(&JsonFailure { + schema_version: JSON_SCHEMA_VERSION, + tag: match error.kind { + CommandErrorKind::Usage => "UsageError", + CommandErrorKind::Operation => "OperationError", + }, + error: JsonError { + code: error.code, + message: &error.message, + }, + }) + .map(|mut bytes| { + bytes.push(b'\n'); + bytes + }) + .unwrap_or_else(|_| b"OUTPUT_RENDER_FAILED: error output could not be encoded\n".to_vec()); + } + format!("{error}\n").into_bytes() +} + +fn render_plain(outcome: &SearchOutcome) -> String { + let mut output = String::new(); + for item in &outcome.items { + output.push_str(&escape_plain(&item.name)); + output.push('\t'); + output.push_str(&escape_plain(&item.selector)); + output.push('\t'); + output.push_str(&item.stargazer_count.to_string()); + output.push('\t'); + output.push_str(&escape_plain( + item.description.as_deref().unwrap_or_default(), + )); + output.push('\n'); + } + output +} + +fn render_human(outcome: &SearchOutcome, width: u16, color: bool) -> String { + let width = usize::from(width); + let mut output = String::new(); + let heading = format!("Skill search {}", terminal_text(&outcome.query)); + for line in wrap(&heading, width) { + output.push_str(&styled(&line, "\u{1b}[1m\u{1b}[36m", color)); + output.push('\n'); + } + let shown = outcome.items.len(); + output.push_str(&format!( + "{} of {} {}\n", + shown, + outcome.total, + if outcome.total == 1 { + "Skill" + } else { + "Skills" + } + )); + + if outcome.items.is_empty() { + output.push('\n'); + let empty = format!("No Skills found for {}.", terminal_text(&outcome.query)); + for line in wrap(&empty, width) { + output.push_str(&line); + output.push('\n'); + } + output.push_str("Try a shorter search.\n"); + return output; + } + + for item in &outcome.items { + output.push('\n'); + let name = terminal_text(&item.name); + let stars = format!("{} stars", grouped_number(item.stargazer_count)); + if 2 + display_width(&name) + 2 + display_width(&stars) <= width { + let gap = width - 2 - display_width(&name) - display_width(&stars); + output.push_str(" "); + output.push_str(&styled(&name, "\u{1b}[1m", color)); + output.push_str(&" ".repeat(gap)); + output.push_str(&styled(&stars, "\u{1b}[33m", color)); + output.push('\n'); + } else { + for line in wrap(&name, width.saturating_sub(2)) { + output.push_str(" "); + output.push_str(&styled(&line, "\u{1b}[1m", color)); + output.push('\n'); + } + output.push_str(" "); + output.push_str(&styled(&stars, "\u{1b}[33m", color)); + output.push('\n'); + } + + if let Some(description) = &item.description { + for line in wrap(&terminal_text(description), width.saturating_sub(2)) { + output.push_str(" "); + output.push_str(&line); + output.push('\n'); + } + } + let install = format!("Install: skilld install {}", terminal_text(&item.selector)); + for line in wrap(&install, width.saturating_sub(2)) { + output.push_str(" "); + output.push_str(&styled(&line, "\u{1b}[2m", color)); + output.push('\n'); + } + } + output +} + +fn escape_plain(value: &str) -> String { + let mut output = String::new(); + for character in value.chars() { + match character { + '\\' => output.push_str("\\\\"), + '\t' => output.push_str("\\t"), + '\r' => output.push_str("\\r"), + '\n' => output.push_str("\\n"), + character if character.is_control() => { + output.push_str(&format!("\\u{{{:04X}}}", u32::from(character))); + } + character => output.push(character), + } + } + output +} + +fn terminal_text(value: &str) -> String { + value + .chars() + .map(|character| { + if character.is_control() { + ' ' + } else { + character + } + }) + .collect() +} + +fn wrap(value: &str, width: usize) -> Vec { + let width = width.max(1); + let mut lines = Vec::new(); + let mut current = String::new(); + + for word in value.split_whitespace() { + let separator = usize::from(!current.is_empty()); + if display_width(¤t) + separator + display_width(word) > width && !current.is_empty() + { + lines.push(std::mem::take(&mut current)); + } + if !current.is_empty() { + current.push(' '); + } + if display_width(word) <= width { + current.push_str(word); + } else { + let mut chunk = String::new(); + for character in word.chars() { + let character_width = UnicodeWidthChar::width(character).unwrap_or(0); + if !chunk.is_empty() && display_width(&chunk) + character_width > width { + lines.push(std::mem::take(&mut chunk)); + } + chunk.push(character); + } + current = chunk; + } + } + if !current.is_empty() || lines.is_empty() { + lines.push(current); + } + lines +} + +fn display_width(value: &str) -> usize { + UnicodeWidthStr::width(value) +} + +fn grouped_number(value: u64) -> String { + let digits = value.to_string(); + let mut output = String::new(); + for (index, character) in digits.chars().enumerate() { + if index > 0 && (digits.len() - index) % 3 == 0 { + output.push(','); + } + output.push(character); + } + output +} + +fn styled(value: &str, style: &str, color: bool) -> String { + if color { + format!("{style}{value}\u{1b}[0m") + } else { + value.to_owned() + } +} + +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] +struct JsonSuccess { + schema_version: u8, + #[serde(rename = "_tag")] + tag: &'static str, + command: &'static str, + data: T, + notices: Vec, +} + +#[derive(Serialize)] +#[serde(untagged)] +enum JsonDisplayData<'a> { + Help { + path: &'a str, + text: &'a str, + }, + Version { + name: &'static str, + version: &'static str, + }, +} + +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] +struct JsonSearchData<'a> { + query: &'a str, + items: &'a [SearchItem], + total: u64, +} + +#[derive(Serialize)] +struct JsonNotice; + +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] +struct JsonFailure<'a> { + schema_version: u8, + #[serde(rename = "_tag")] + tag: &'static str, + error: JsonError<'a>, +} + +#[derive(Serialize)] +struct JsonError<'a> { + code: &'a str, + message: &'a str, +} diff --git a/crates/skilld-command/src/remote.rs b/crates/skilld-command/src/remote.rs index a108efd1..a6b1f66d 100644 --- a/crates/skilld-command/src/remote.rs +++ b/crates/skilld-command/src/remote.rs @@ -9,7 +9,7 @@ use serde::Deserialize; use serde_json::json; use skilld_core::{ ArtifactAttestation, LockedSource, PreparedFile, RemoteError, RemoteSelector, - RepositoryVisibility, SearchResult, SourceRef, SourceRequest, SourceSelector, SourceStatus, + RepositoryVisibility, SearchResponse, SourceRef, SourceRequest, SourceSelector, SourceStatus, TrustedRoot, TrustedRootPin, VerifiedTrustedRoot, parse_search_response, prepare_unverified_files, verify_artifact, verify_attestation, verify_trusted_root, }; @@ -250,7 +250,7 @@ pub enum RemoteSourceState { } pub trait RemoteProvider: Send + Sync { - fn search(&self, query: &str, limit: u8) -> Result, RemoteError>; + fn search(&self, query: &str, limit: u8) -> Result; fn prepare( &self, @@ -786,7 +786,7 @@ impl SkilldRemote { } impl RemoteProvider for SkilldRemote { - fn search(&self, query: &str, limit: u8) -> Result, RemoteError> { + fn search(&self, query: &str, limit: u8) -> Result { let query = query.trim(); if query.is_empty() || query.len() > 200 || !(1..=50).contains(&limit) { return Err(RemoteError::new( @@ -806,7 +806,7 @@ impl RemoteProvider for SkilldRemote { response_limit: SEARCH_LIMIT, }; let response = self.execute(request, AllowedOrigin::Service(self.endpoint.clone()))?; - parse_search_response(&response.body).map(|response| response.items) + parse_search_response(&response.body) } fn prepare( diff --git a/crates/skilld-command/tests/output.rs b/crates/skilld-command/tests/output.rs new file mode 100644 index 00000000..a80e2206 --- /dev/null +++ b/crates/skilld-command/tests/output.rs @@ -0,0 +1,473 @@ +use skilld_command::{CommandError, Host, OutputContext, run_with_output}; +use skilld_core::{ + InstallScope, InstallSource, SearchResponse, SearchResult, SourceProvider, SourceRequest, + SourceSelector, +}; +use std::io::{self, Write}; +use unicode_width::UnicodeWidthStr; + +#[derive(Clone)] +struct SearchHost { + response: Result, +} + +impl Host for SearchHost { + fn list(&self, _scope: InstallScope) -> Result, CommandError> { + unreachable!("list is outside this test") + } + + fn install( + &self, + _source: InstallSource, + _scope: InstallScope, + ) -> Result { + unreachable!("install is outside this test") + } + + fn search(&self, _query: &str) -> Result { + self.response.clone() + } +} + +fn response() -> SearchResponse { + SearchResponse { + items: vec![SearchResult { + name: "grill-me".to_owned(), + description: Some( + "A focused Skill description that wraps cleanly on a narrow terminal.".to_owned(), + ), + source: SourceRequest { + provider: SourceProvider::Github, + owner: "mattpocock".to_owned(), + repository: "skills".to_owned(), + selector: SourceSelector::NamedSkill { + name: "grill-me".to_owned(), + }, + r#ref: None, + }, + stargazer_count: 227_068, + }], + total: 14, + } +} + +fn run(args: &[&str], context: OutputContext) -> (u8, String, String) { + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let result = run_with_output( + args, + &SearchHost { + response: Ok(response()), + }, + context, + &mut stdout, + &mut stderr, + ); + ( + result.exit_code, + String::from_utf8(stdout).unwrap(), + String::from_utf8(stderr).unwrap(), + ) +} + +#[test] +fn json_search_returns_one_versioned_document() { + let (exit, stdout, stderr) = run( + &["skilld", "search", "grill", "--json"], + OutputContext::auto(true, true, false, false, false, 80), + ); + let global_form = run( + &["skilld", "--json", "search", "grill"], + OutputContext::auto(true, true, false, false, false, 80), + ); + + assert_eq!(exit, 0); + assert!(stderr.is_empty()); + assert_eq!( + serde_json::from_str::(&stdout).unwrap(), + serde_json::json!({ + "schemaVersion": 1, + "_tag": "Success", + "command": "search", + "data": { + "query": "grill", + "items": [{ + "name": "grill-me", + "selector": "skilld:mattpocock/skills/grill-me", + "description": "A focused Skill description that wraps cleanly on a narrow terminal.", + "stargazerCount": 227068 + }], + "total": 14 + }, + "notices": [] + }) + ); + assert!(stdout.ends_with('\n')); + assert_eq!(global_form, (exit, stdout, stderr)); +} + +#[test] +fn json_help_and_version_return_versioned_documents() { + let root_help = run(&["skilld", "--json", "--help"], OutputContext::Plain); + let search_help = run( + &["skilld", "search", "--json", "--help"], + OutputContext::Plain, + ); + let version = run(&["skilld", "--json", "--version"], OutputContext::Plain); + + assert_eq!(root_help.0, 0); + assert!(root_help.2.is_empty()); + let root = serde_json::from_str::(&root_help.1).unwrap(); + assert_eq!(root["command"], "help"); + assert_eq!(root["data"]["path"], "skilld"); + let search = serde_json::from_str::(&search_help.1).unwrap(); + assert_eq!(search["command"], "help"); + assert_eq!(search["data"]["path"], "skilld search"); + let version = serde_json::from_str::(&version.1).unwrap(); + assert_eq!(version["command"], "version"); + assert_eq!(version["data"]["name"], "skilld"); + assert_eq!(version["data"]["version"], env!("CARGO_PKG_VERSION")); +} + +#[test] +fn non_terminal_and_ci_output_are_stable_plain_records() { + let expected = concat!( + "grill-me\tskilld:mattpocock/skills/grill-me\t227068\t", + "A focused Skill description that wraps cleanly on a narrow terminal.\n" + ); + + let non_terminal = run( + &["skilld", "search", "grill"], + OutputContext::auto(false, false, false, false, false, 80), + ); + let ci_with_tty = run( + &["skilld", "search", "grill"], + OutputContext::auto(true, false, true, false, false, 120), + ); + + assert_eq!(non_terminal, (0, expected.to_owned(), String::new())); + assert_eq!(ci_with_tty, non_terminal); +} + +#[test] +fn human_terminal_is_formatted_without_an_explicit_machine_flag() { + let agent_with_tty = run( + &["skilld", "search", "grill"], + OutputContext::auto(true, false, false, false, false, 120), + ); + + assert_eq!(agent_with_tty.0, 0); + assert!(agent_with_tty.1.contains("Skill search")); + assert!(agent_with_tty.1.contains('\u{1b}')); + assert!(agent_with_tty.2.is_empty()); +} + +#[test] +fn active_agent_terminal_is_plain_without_an_explicit_machine_flag() { + let result = run( + &["skilld", "search", "grill"], + OutputContext::auto(true, true, false, false, false, 120), + ); + + assert_eq!( + result, + ( + 0, + concat!( + "grill-me\tskilld:mattpocock/skills/grill-me\t227068\t", + "A focused Skill description that wraps cleanly on a narrow terminal.\n" + ) + .to_owned(), + String::new() + ) + ); +} + +#[test] +fn explicit_plain_overrides_a_human_terminal() { + let (_, stdout, stderr) = run( + &["skilld", "search", "grill", "--plain"], + OutputContext::auto(true, false, false, false, false, 120), + ); + + assert!(stderr.is_empty()); + assert_eq!( + stdout, + concat!( + "grill-me\tskilld:mattpocock/skills/grill-me\t227068\t", + "A focused Skill description that wraps cleanly on a narrow terminal.\n" + ) + ); +} + +#[test] +fn plain_search_escapes_record_delimiters() { + let mut response = response(); + response.items[0].description = Some("first\nsecond\tvalue\u{1b}".to_owned()); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run_with_output( + ["skilld", "search", "grill", "--plain"], + &SearchHost { + response: Ok(response), + }, + OutputContext::auto(true, false, false, false, false, 80), + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 0); + assert!(stderr.is_empty()); + assert_eq!( + String::from_utf8(stdout).unwrap(), + "grill-me\tskilld:mattpocock/skills/grill-me\t227068\tfirst\\nsecond\\tvalue\\u{001B}\n" + ); +} + +#[test] +fn human_search_is_polished_and_respects_terminal_width() { + let (_, stdout, stderr) = run( + &["skilld", "search", "grill"], + OutputContext::auto(true, false, false, true, false, 40), + ); + + assert!(stderr.is_empty()); + assert!(stdout.contains("Skill search")); + assert!(stdout.contains("1 of 14 Skills")); + assert!(stdout.contains("227,068 stars")); + assert!(stdout.contains("skilld:mattpocock/skills/grill-me")); + assert!(stdout.contains("Install: skilld install")); + assert!( + stdout + .lines() + .all(|line| UnicodeWidthStr::width(line) <= 40) + ); + assert!(!stdout.contains('\u{1b}')); +} + +#[test] +fn human_empty_search_names_the_query_and_suggests_a_next_step() { + let mut response = response(); + response.items.clear(); + response.total = 0; + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run_with_output( + ["skilld", "search", "grill"], + &SearchHost { + response: Ok(response), + }, + OutputContext::auto(true, false, false, true, false, 40), + &mut stdout, + &mut stderr, + ); + let stdout = String::from_utf8(stdout).unwrap(); + + assert_eq!(result.exit_code, 0); + assert!(stderr.is_empty()); + assert!(stdout.contains("No Skills found for grill.")); + assert!(stdout.contains("Try a shorter search.")); +} + +#[test] +fn human_search_uses_display_cells_and_sanitizes_terminal_controls() { + let mut response = response(); + response.items[0].name = "\u{6280}\u{80fd}\u{1f642}".to_owned(); + response.items[0].description = + Some("\u{6f22}\u{5b57}\u{1f642} cafe\u{301} \u{1b}[31mred".to_owned()); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run_with_output( + ["skilld", "search", "grill"], + &SearchHost { + response: Ok(response), + }, + OutputContext::auto(true, false, false, true, false, 20), + &mut stdout, + &mut stderr, + ); + let stdout = String::from_utf8(stdout).unwrap(); + + assert_eq!(result.exit_code, 0); + assert!(stderr.is_empty()); + assert!(stdout.contains("\u{6280}\u{80fd}\u{1f642}")); + assert!(stdout.contains("\u{6f22}\u{5b57}\u{1f642}")); + assert!(stdout.contains("cafe\u{301}")); + assert!(!stdout.contains('\u{1b}')); + assert!( + stdout + .lines() + .all(|line| UnicodeWidthStr::width(line) <= 20) + ); +} + +#[test] +fn broken_pipe_is_a_successful_search_exit() { + let mut stdout = BrokenPipeWriter; + let mut stderr = Vec::new(); + + let result = run_with_output( + ["skilld", "search", "grill", "--plain"], + &SearchHost { + response: Ok(response()), + }, + OutputContext::Plain, + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 0); + assert!(stderr.is_empty()); +} + +#[test] +fn color_capabilities_change_ansi_only() { + let (_, colored, _) = run( + &["skilld", "search", "grill"], + OutputContext::auto(true, false, false, false, false, 100), + ); + let (_, no_color, _) = run( + &["skilld", "search", "grill"], + OutputContext::auto(true, false, false, true, false, 100), + ); + let (_, dumb_terminal, _) = run( + &["skilld", "search", "grill"], + OutputContext::auto(true, false, false, false, true, 100), + ); + + assert!(colored.contains('\u{1b}')); + assert_eq!(strip_ansi(&colored), no_color); + assert_eq!(no_color, dumb_terminal); +} + +#[test] +fn conflicting_output_flags_fail_before_search() { + let (exit, stdout, stderr) = run( + &["skilld", "search", "grill", "--json", "--plain"], + OutputContext::auto(false, false, false, false, false, 80), + ); + + assert_eq!(exit, 2); + assert!(stdout.is_empty()); + assert!(stderr.contains("cannot be used with")); +} + +#[test] +fn json_search_parse_errors_are_tagged_usage_errors() { + let after = run( + &["skilld", "search", "grill", "--json", "--unknown"], + OutputContext::Plain, + ); + let before = run( + &["skilld", "--json", "search", "grill", "--unknown"], + OutputContext::Plain, + ); + + assert_eq!(after.0, 2); + assert!(after.1.is_empty()); + assert_eq!( + serde_json::from_str::(&after.2).unwrap()["_tag"], + "UsageError" + ); + assert_eq!(before, after); +} + +#[test] +fn empty_json_search_is_a_tagged_usage_error() { + let (exit, stdout, stderr) = run(&["skilld", "search", "--json"], OutputContext::Plain); + + assert_eq!(exit, 2); + assert!(stdout.is_empty()); + let error = serde_json::from_str::(&stderr).unwrap(); + assert_eq!(error["_tag"], "UsageError"); + assert_eq!(error["error"]["code"], "INVALID_SEARCH"); +} + +#[test] +fn json_search_errors_are_tagged_and_written_to_stderr() { + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let result = run_with_output( + ["skilld", "search", "grill", "--json"], + &SearchHost { + response: Err(CommandError::service("Skill search timed out")), + }, + OutputContext::auto(false, false, false, false, false, 80), + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 1); + assert!(stdout.is_empty()); + assert_eq!( + serde_json::from_slice::(&stderr).unwrap(), + serde_json::json!({ + "schemaVersion": 1, + "_tag": "OperationError", + "error": { + "code": "SERVICE_UNAVAILABLE", + "message": "Skill search timed out" + } + }) + ); +} + +#[test] +fn stdout_failures_report_an_operation_error() { + let mut stdout = WriteErrorWriter; + let mut stderr = Vec::new(); + + let result = run_with_output( + ["skilld", "search", "grill", "--json"], + &SearchHost { + response: Ok(response()), + }, + OutputContext::Plain, + &mut stdout, + &mut stderr, + ); + + assert_eq!(result.exit_code, 1); + let error = serde_json::from_slice::(&stderr).unwrap(); + assert_eq!(error["_tag"], "OperationError"); + assert_eq!(error["error"]["code"], "OUTPUT_WRITE_FAILED"); +} + +fn strip_ansi(value: &str) -> String { + [ + "\u{1b}[1m", + "\u{1b}[2m", + "\u{1b}[36m", + "\u{1b}[33m", + "\u{1b}[0m", + ] + .into_iter() + .fold(value.to_owned(), |value, code| value.replace(code, "")) +} + +struct BrokenPipeWriter; + +impl Write for BrokenPipeWriter { + fn write(&mut self, _buffer: &[u8]) -> io::Result { + Err(io::Error::from(io::ErrorKind::BrokenPipe)) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } +} + +struct WriteErrorWriter; + +impl Write for WriteErrorWriter { + fn write(&mut self, _buffer: &[u8]) -> io::Result { + Err(io::Error::other("write failed")) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } +} diff --git a/crates/skilld-command/tests/remote.rs b/crates/skilld-command/tests/remote.rs index fa1864ac..ab984036 100644 --- a/crates/skilld-command/tests/remote.rs +++ b/crates/skilld-command/tests/remote.rs @@ -17,7 +17,8 @@ use skilld_core::{ AgentTargetId, ArtifactAttestation, ArtifactFile, AttestationSignature, CheckOutcome, CheckResult, InstallMode, InstallOperation, InstallRequest, InstallScope, InstallSource, LockedSource, PreparedFile, RemoteError, RemoteSelector, RepositoryVisibility, ResolvedSource, - SearchResult, SignatureAlgorithm, SourceProvider, SourceStatus, TrustedRootPin, + SearchResponse, SignatureAlgorithm, SourceProvider, SourceStatus, TrustedRootPin, + UpdateCheckV1, UpdateLatestCommit, UpdateRelation, }; const ROOT_DOMAIN: &[u8] = b"skilld-trusted-key-v1\0"; @@ -331,9 +332,10 @@ fn search_uses_only_the_v1_skilld_route_and_retries_a_bounded_failure() { ])); let remote = search_remote(http.clone()); - let results = remote.search("vue testing", 20).unwrap(); + let response = remote.search("vue testing", 20).unwrap(); - assert_eq!(results[0].name, "vue-testing"); + assert_eq!(response.items[0].name, "vue-testing"); + assert_eq!(response.total, 1); let requests = http.requests.lock().unwrap(); assert_eq!(requests.len(), 2); assert!(requests.iter().all(|request| { @@ -759,11 +761,10 @@ impl FakeProvider { } impl RemoteProvider for FakeProvider { - fn search(&self, _query: &str, _limit: u8) -> Result, RemoteError> { + fn search(&self, _query: &str, _limit: u8) -> Result { skilld_core::parse_search_response(include_bytes!( "../../../contracts/fixtures/v1/skill-search.json" )) - .map(|response| response.items) } fn prepare( @@ -856,7 +857,7 @@ fn verify_reports_changed_bytes_and_stale_sources() { } #[test] -fn remote_install_verify_and_failed_upgrade_use_the_normal_transaction() { +fn remote_install_verify_and_failed_update_use_the_normal_transaction() { let temporary = tempfile::tempdir().unwrap(); let project = temporary.path().join("project"); let data = temporary.path().join("data"); @@ -881,7 +882,7 @@ fn remote_install_verify_and_failed_upgrade_use_the_normal_transaction() { *provider.content.lock().unwrap() = b"---\nname: example\ndescription: second\n---\n".to_vec(); *provider.fail_prepare.lock().unwrap() = true; - let error = host.upgrade(Some("example")).unwrap_err(); + let error = host.update(Some("example")).unwrap_err(); assert_eq!(error.code, "CHECK_BLOCKED"); assert_eq!( @@ -890,6 +891,64 @@ fn remote_install_verify_and_failed_upgrade_use_the_normal_transaction() { ); } +#[test] +fn update_check_keeps_an_uncompared_commit_unavailable() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let provider = provider("---\nname: example\ndescription: first\n---\n"); + let host = LocalHost::new(project, temporary.path().join("data")) + .with_remote_provider(provider.clone()); + host.install_request(InstallRequest { + operation: skilld_core::InstallOperation::Install(InstallSource::Remote( + "skilld:skilld-dev/skills/example".to_owned(), + )), + scope: InstallScope::Project, + targets: vec![AgentTargetId::Codex], + mode: Some(InstallMode::Copy), + }) + .unwrap(); + *provider.stale.lock().unwrap() = true; + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run( + ["skilld", "update", "example", "--check", "--json"], + &host, + &mut stdout, + &mut stderr, + ); + let mut global_stdout = Vec::new(); + let mut global_stderr = Vec::new(); + let global_result = run( + ["skilld", "--json", "update", "example", "--check"], + &host, + &mut global_stdout, + &mut global_stderr, + ); + let outcome: serde_json::Value = serde_json::from_slice(&stdout).unwrap(); + let check: UpdateCheckV1 = serde_json::from_value(outcome["data"].clone()).unwrap(); + + assert_eq!(result.exit_code, 0); + assert!(stderr.is_empty()); + assert_eq!(global_result.exit_code, 0); + assert!(global_stderr.is_empty()); + assert_eq!(global_stdout, stdout); + assert_eq!(outcome["schemaVersion"], 1); + assert_eq!(outcome["_tag"], "Success"); + assert_eq!(outcome["command"], "update"); + assert_eq!(outcome["notices"], serde_json::json!([])); + assert!(matches!( + check.items()[0].relation(), + UpdateRelation::Unavailable { + latest_commit: UpdateLatestCommit::Known { commit_sha }, + failure, + .. + } if commit_sha.as_str() == "ffffffffffffffffffffffffffffffffffffffff" + && failure.code == "COMPARISON_UNAVAILABLE" + )); +} + #[test] fn cli_direct_install_marks_review_as_required() { let temporary = tempfile::tempdir().unwrap(); @@ -1030,7 +1089,7 @@ fn cli_plain_restore_rejects_an_unverified_source_with_the_recovery_command() { &mut stderr, ); - assert_eq!(restored.exit_code, 2); + assert_eq!(restored.exit_code, 1); assert!(stdout.is_empty()); assert_eq!( String::from_utf8(stderr).unwrap(), diff --git a/crates/skilld-core/src/lib.rs b/crates/skilld-core/src/lib.rs index 77ec89a8..92c81b6b 100644 --- a/crates/skilld-core/src/lib.rs +++ b/crates/skilld-core/src/lib.rs @@ -1,6 +1,7 @@ mod lock; mod remote; mod target; +mod update; use std::fmt; use std::path::{Path, PathBuf}; @@ -20,6 +21,10 @@ use serde::{Deserialize, Serialize}; pub use target::{ AGENT_TARGETS, AgentTarget, AgentTargetId, GlobalTargetPath, TargetSelection, select_target_ids, }; +pub use update::{ + CommitSha, NotTrackedReason, UpdateCheckV1, UpdateFailure, UpdateLatestCommit, + UpdateModelError, UpdatePlan, UpdatePlanItem, UpdateRelation, classify_update_comparison, +}; pub const VERSION: &str = env!("CARGO_PKG_VERSION"); @@ -48,7 +53,8 @@ impl InstallMode { } } -#[derive(Clone, Debug, Eq, Ord, PartialEq, PartialOrd)] +#[derive(Clone, Debug, Deserialize, Eq, Ord, PartialEq, PartialOrd, Serialize)] +#[serde(try_from = "String", into = "String")] pub struct SkillName(String); impl SkillName { @@ -91,6 +97,20 @@ impl fmt::Display for SkillName { } } +impl TryFrom for SkillName { + type Error = DomainError; + + fn try_from(value: String) -> Result { + Self::parse(value) + } +} + +impl From for String { + fn from(value: SkillName) -> Self { + value.0 + } +} + #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum InstallScope { Project, diff --git a/crates/skilld-core/src/update.rs b/crates/skilld-core/src/update.rs new file mode 100644 index 00000000..1c8609d1 --- /dev/null +++ b/crates/skilld-core/src/update.rs @@ -0,0 +1,247 @@ +use std::fmt; +use std::num::NonZeroU64; + +use serde::{Deserialize, Deserializer, Serialize}; + +use crate::SkillName; + +#[derive(Clone, Debug, Deserialize, Eq, Ord, PartialEq, PartialOrd, Serialize)] +#[serde(try_from = "String", into = "String")] +pub struct CommitSha(String); + +impl CommitSha { + pub fn parse(value: impl Into) -> Result { + let value = value.into(); + let valid = value.len() == 40 + && value + .bytes() + .all(|byte| byte.is_ascii_digit() || (b'a'..=b'f').contains(&byte)); + valid + .then_some(Self(value.clone())) + .ok_or(UpdateModelError::InvalidCommitSha(value)) + } + + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl TryFrom for CommitSha { + type Error = UpdateModelError; + + fn try_from(value: String) -> Result { + Self::parse(value) + } +} + +impl From for String { + fn from(value: CommitSha) -> Self { + value.0 + } +} + +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +#[serde(deny_unknown_fields, rename_all = "camelCase")] +pub struct UpdateFailure { + pub code: String, + pub message: String, +} + +impl UpdateFailure { + pub fn new(code: impl Into, message: impl Into) -> Self { + Self { + code: code.into(), + message: message.into(), + } + } +} + +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +#[serde( + deny_unknown_fields, + tag = "_tag", + rename_all = "camelCase", + rename_all_fields = "camelCase" +)] +pub enum UpdateLatestCommit { + Known { commit_sha: CommitSha }, + Unknown, +} + +#[derive(Clone, Copy, Debug, Deserialize, Eq, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub enum NotTrackedReason { + Local, + Bundled, +} + +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +#[serde( + deny_unknown_fields, + tag = "_tag", + rename_all = "camelCase", + rename_all_fields = "camelCase" +)] +pub enum UpdateRelation { + Current { + commit_sha: CommitSha, + }, + Available { + locked_commit_sha: CommitSha, + latest_commit_sha: CommitSha, + ahead_by: NonZeroU64, + }, + Behind { + locked_commit_sha: CommitSha, + latest_commit_sha: CommitSha, + behind_by: NonZeroU64, + }, + Diverged { + locked_commit_sha: CommitSha, + latest_commit_sha: CommitSha, + ahead_by: NonZeroU64, + behind_by: NonZeroU64, + }, + Pinned { + commit_sha: CommitSha, + }, + NotTracked { + reason: NotTrackedReason, + }, + Unavailable { + locked_commit_sha: CommitSha, + latest_commit: UpdateLatestCommit, + failure: UpdateFailure, + }, +} + +pub fn classify_update_comparison( + locked_commit_sha: CommitSha, + latest_commit_sha: CommitSha, + ahead_by: u64, + behind_by: u64, +) -> Result { + match (locked_commit_sha == latest_commit_sha, ahead_by, behind_by) { + (true, 0, 0) => Ok(UpdateRelation::Current { + commit_sha: locked_commit_sha, + }), + (false, ahead_by, 0) if ahead_by > 0 => Ok(UpdateRelation::Available { + locked_commit_sha, + latest_commit_sha, + ahead_by: NonZeroU64::new(ahead_by).expect("ahead count is non-zero"), + }), + (false, 0, behind_by) if behind_by > 0 => Ok(UpdateRelation::Behind { + locked_commit_sha, + latest_commit_sha, + behind_by: NonZeroU64::new(behind_by).expect("behind count is non-zero"), + }), + (false, ahead_by, behind_by) if ahead_by > 0 && behind_by > 0 => { + Ok(UpdateRelation::Diverged { + locked_commit_sha, + latest_commit_sha, + ahead_by: NonZeroU64::new(ahead_by).expect("ahead count is non-zero"), + behind_by: NonZeroU64::new(behind_by).expect("behind count is non-zero"), + }) + } + _ => Err(UpdateModelError::InvalidComparison), + } +} + +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +#[serde(deny_unknown_fields, rename_all = "camelCase")] +pub struct UpdatePlanItem { + name: SkillName, + relation: UpdateRelation, +} + +impl UpdatePlanItem { + pub fn new(name: SkillName, relation: UpdateRelation) -> Self { + Self { name, relation } + } + + pub const fn name(&self) -> &SkillName { + &self.name + } + + pub const fn relation(&self) -> &UpdateRelation { + &self.relation + } +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct UpdatePlan { + items: Vec, +} + +impl UpdatePlan { + pub fn new(mut items: Vec) -> Result { + items.sort_by(|left, right| left.name.cmp(&right.name)); + if let Some(duplicate) = items + .windows(2) + .find(|pair| pair[0].name == pair[1].name) + .map(|pair| pair[0].name.to_string()) + { + return Err(UpdateModelError::DuplicateSkill(duplicate)); + } + Ok(Self { items }) + } + + pub fn items(&self) -> &[UpdatePlanItem] { + &self.items + } +} + +#[derive(Clone, Debug, Eq, PartialEq, Serialize)] +#[serde(deny_unknown_fields, rename_all = "camelCase")] +pub struct UpdateCheckV1 { + items: Vec, +} + +impl UpdateCheckV1 { + pub fn new(plan: UpdatePlan) -> Self { + Self { items: plan.items } + } + + pub fn items(&self) -> &[UpdatePlanItem] { + &self.items + } +} + +#[derive(Deserialize)] +#[serde(deny_unknown_fields, rename_all = "camelCase")] +struct UpdateCheckV1Wire { + items: Vec, +} + +impl<'de> Deserialize<'de> for UpdateCheckV1 { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let wire = UpdateCheckV1Wire::deserialize(deserializer)?; + UpdatePlan::new(wire.items) + .map(Self::new) + .map_err(serde::de::Error::custom) + } +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum UpdateModelError { + DuplicateSkill(String), + InvalidCommitSha(String), + InvalidComparison, +} + +impl fmt::Display for UpdateModelError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::DuplicateSkill(name) => { + write!(formatter, "duplicate Skill in update plan: {name}") + } + Self::InvalidCommitSha(_) => formatter.write_str("invalid Git commit in update plan"), + Self::InvalidComparison => formatter.write_str("invalid Git update comparison"), + } + } +} + +impl std::error::Error for UpdateModelError {} diff --git a/crates/skilld-core/tests/update.rs b/crates/skilld-core/tests/update.rs new file mode 100644 index 00000000..d75ca05e --- /dev/null +++ b/crates/skilld-core/tests/update.rs @@ -0,0 +1,122 @@ +use std::num::NonZeroU64; + +use skilld_core::{ + CommitSha, SkillName, UpdateCheckV1, UpdateLatestCommit, UpdateModelError, UpdatePlan, + UpdatePlanItem, UpdateRelation, classify_update_comparison, +}; + +fn sha(value: char) -> CommitSha { + CommitSha::parse(value.to_string().repeat(40)).unwrap() +} + +#[test] +fn comparison_counts_classify_every_git_relation() { + let locked = sha('1'); + let latest = sha('2'); + + assert_eq!( + classify_update_comparison(locked.clone(), locked.clone(), 0, 0).unwrap(), + UpdateRelation::Current { + commit_sha: locked.clone(), + } + ); + assert_eq!( + classify_update_comparison(locked.clone(), latest.clone(), 3, 0).unwrap(), + UpdateRelation::Available { + locked_commit_sha: locked.clone(), + latest_commit_sha: latest.clone(), + ahead_by: NonZeroU64::new(3).unwrap(), + } + ); + assert_eq!( + classify_update_comparison(locked.clone(), latest.clone(), 0, 2).unwrap(), + UpdateRelation::Behind { + locked_commit_sha: locked.clone(), + latest_commit_sha: latest.clone(), + behind_by: NonZeroU64::new(2).unwrap(), + } + ); + assert_eq!( + classify_update_comparison(locked.clone(), latest.clone(), 4, 2).unwrap(), + UpdateRelation::Diverged { + locked_commit_sha: locked, + latest_commit_sha: latest, + ahead_by: NonZeroU64::new(4).unwrap(), + behind_by: NonZeroU64::new(2).unwrap(), + } + ); +} + +#[test] +fn impossible_comparison_counts_are_rejected() { + assert_eq!( + classify_update_comparison(sha('1'), sha('2'), 0, 0), + Err(UpdateModelError::InvalidComparison) + ); + assert_eq!( + classify_update_comparison(sha('1'), sha('1'), 1, 0), + Err(UpdateModelError::InvalidComparison) + ); +} + +#[test] +fn update_plan_sorts_skills_and_rejects_duplicates() { + let first = UpdatePlanItem::new( + SkillName::parse("zeta").unwrap(), + UpdateRelation::Pinned { + commit_sha: sha('1'), + }, + ); + let second = UpdatePlanItem::new( + SkillName::parse("alpha").unwrap(), + UpdateRelation::Current { + commit_sha: sha('2'), + }, + ); + let plan = UpdatePlan::new(vec![first, second]).unwrap(); + + assert_eq!( + plan.items() + .iter() + .map(|item| item.name().as_str()) + .collect::>(), + ["alpha", "zeta"] + ); + assert_eq!( + UpdatePlan::new(vec![ + UpdatePlanItem::new( + SkillName::parse("alpha").unwrap(), + UpdateRelation::Pinned { + commit_sha: sha('1'), + }, + ), + UpdatePlanItem::new( + SkillName::parse("alpha").unwrap(), + UpdateRelation::Current { + commit_sha: sha('2'), + }, + ), + ]), + Err(UpdateModelError::DuplicateSkill("alpha".to_owned())) + ); +} + +#[test] +fn v1_check_fixture_covers_the_published_relations() { + let bytes = include_bytes!("../../../tests/fixtures/v3-rust/v1/update-check.json"); + let fixture: serde_json::Value = serde_json::from_slice(bytes).unwrap(); + let check: UpdateCheckV1 = serde_json::from_value(fixture["data"].clone()).unwrap(); + + assert_eq!(fixture["schemaVersion"], 1); + assert_eq!(fixture["_tag"], "Success"); + assert_eq!(fixture["command"], "update"); + assert_eq!(check.items().len(), 7); + assert!(matches!( + check.items()[6].relation(), + UpdateRelation::Unavailable { + latest_commit: UpdateLatestCommit::Known { .. }, + .. + } + )); + assert_eq!(serde_json::to_value(check).unwrap(), fixture["data"]); +} diff --git a/crates/skilld-native/Cargo.toml b/crates/skilld-native/Cargo.toml index f88e8e4c..c7727ae2 100644 --- a/crates/skilld-native/Cargo.toml +++ b/crates/skilld-native/Cargo.toml @@ -24,6 +24,11 @@ skilld-core.workspace = true tempfile.workspace = true +terminal_size.workspace = true + ureq = { version = "=3.1.4", default-features = false, features = [ "rustls" ] } url.workspace = true + +[target.'cfg(unix)'.dev-dependencies] +nix = { version = "0.29.0", features = [ "term" ] } diff --git a/crates/skilld-native/src/main.rs b/crates/skilld-native/src/main.rs index 895368de..bc4dba4e 100644 --- a/crates/skilld-native/src/main.rs +++ b/crates/skilld-native/src/main.rs @@ -2,6 +2,7 @@ mod embedded_skill; mod native_auth; use std::env; +use std::io::IsTerminal; use std::path::PathBuf; use std::process::ExitCode; use std::sync::Arc; @@ -9,11 +10,15 @@ use std::sync::Arc; use embedded_skill::EmbeddedSkilld; use native_auth::NativeAccount; use skilld_command::{ - DetectionEnvironment, LocalHost, NativeRemoteConfig, SkilldRemote, TargetRoots, run, - run_stdio_probe, + CommandError, DetectionEnvironment, Host, LocalHost, NativeRemoteConfig, OutputContext, + SkilldRemote, TargetRoots, run_stdio_probe, run_with_output, +}; +use skilld_core::{ + InstallScope, InstallSource, SearchResponse, SearchResult, SourceProvider, SourceRequest, + SourceSelector, TrustedRootPin, }; -use skilld_core::TrustedRootPin; use skilld_native::NativeHttpAdapter; +use terminal_size::Width; fn main() -> ExitCode { if env::var_os("SKILLD_PROBE_STDIO").as_deref() == Some(std::ffi::OsStr::new("1")) { @@ -23,6 +28,9 @@ fn main() -> ExitCode { let result = run_stdio_probe(&mut stdin, &mut stdout, &mut stderr); return ExitCode::from(result.exit_code); } + if env::var_os("SKILLD_PROBE_SEARCH_OUTPUT").as_deref() == Some(std::ffi::OsStr::new("1")) { + return run_search_output_probe(); + } let project_root = match env::current_dir() { Ok(path) => path, @@ -32,10 +40,11 @@ fn main() -> ExitCode { } }; let global_root = global_root(); + let detection = detection_environment(); let account = Arc::new(NativeAccount::new()); let host = LocalHost::new(project_root, global_root) .with_target_roots(target_roots()) - .with_detection_environment(detection_environment()) + .with_detection_environment(detection.clone()) .with_bundled_provider(Arc::new(EmbeddedSkilld::new())) .with_account_provider(account.clone()) .with_remote_provider(Arc::new(SkilldRemote::new( @@ -46,10 +55,76 @@ fn main() -> ExitCode { let mut stdout = std::io::stdout().lock(); let mut stderr = std::io::stderr().lock(); - let result = run(env::args_os(), &host, &mut stdout, &mut stderr); + let output = OutputContext::auto( + stdout.is_terminal(), + active_agent_detected(), + environment_enabled("CI"), + environment_present("NO_COLOR"), + env::var("TERM").is_ok_and(|term| term.eq_ignore_ascii_case("dumb")), + terminal_width(), + ); + let result = run_with_output(env::args_os(), &host, output, &mut stdout, &mut stderr); ExitCode::from(result.exit_code) } +fn run_search_output_probe() -> ExitCode { + let mut stdout = std::io::stdout().lock(); + let mut stderr = std::io::stderr().lock(); + let output = OutputContext::auto( + stdout.is_terminal(), + active_agent_detected(), + environment_enabled("CI"), + environment_present("NO_COLOR"), + env::var("TERM").is_ok_and(|term| term.eq_ignore_ascii_case("dumb")), + terminal_width(), + ); + let mut args = vec!["skilld", "search", "output"]; + if env::args_os().any(|argument| argument == "--json") { + args.push("--json"); + } + if env::args_os().any(|argument| argument == "--plain") { + args.push("--plain"); + } + let result = run_with_output(args, &SearchOutputProbe, output, &mut stdout, &mut stderr); + ExitCode::from(result.exit_code) +} + +struct SearchOutputProbe; + +impl Host for SearchOutputProbe { + fn list(&self, _scope: InstallScope) -> Result, CommandError> { + unreachable!("list is outside the search output probe") + } + + fn install( + &self, + _source: InstallSource, + _scope: InstallScope, + ) -> Result { + unreachable!("install is outside the search output probe") + } + + fn search(&self, _query: &str) -> Result { + Ok(SearchResponse { + items: vec![SearchResult { + name: "output-probe".to_owned(), + description: Some("Checks native terminal output.".to_owned()), + source: SourceRequest { + provider: SourceProvider::Github, + owner: "skilld-dev".to_owned(), + repository: "skilld".to_owned(), + selector: SourceSelector::NamedSkill { + name: "output-probe".to_owned(), + }, + r#ref: None, + }, + stargazer_count: 1, + }], + total: 1, + }) + } +} + fn native_remote_config() -> NativeRemoteConfig { match ( option_env!("SKILLD_ROOT_KEY_ID"), @@ -106,6 +181,29 @@ fn detection_environment() -> DetectionEnvironment { ) } +fn active_agent_detected() -> bool { + const SIGNALS: [&str; 17] = [ + "CLAUDE_CODE", + "CLAUDECODE", + "CLAUDE_CODE_ENTRYPOINT", + "CURSOR_SESSION", + "CURSOR_TRACE_ID", + "WINDSURF_SESSION", + "CLINE_TASK_ID", + "CLINE_ACTIVE", + "COPILOT_RUN_APP", + "GEMINI_CLI", + "GOOSE_SESSION", + "AGENT_SESSION_ID", + "AMP_SESSION", + "OPENCODE_SESSION", + "OPENCODE_SESSION_ID", + "ROO_SESSION", + "ANTIGRAVITY_CLI_ALIAS", + ]; + SIGNALS.iter().any(|name| environment_enabled(name)) +} + fn global_root() -> PathBuf { if let Some(path) = env::var_os("SKILLD_DATA_DIR") { return PathBuf::from(path); @@ -118,3 +216,26 @@ fn global_root() -> PathBuf { } PathBuf::from(".skilld") } + +fn environment_enabled(name: &str) -> bool { + env::var(name).is_ok_and(|value| { + !value.is_empty() && value != "0" && !value.eq_ignore_ascii_case("false") + }) +} + +fn environment_present(name: &str) -> bool { + env::var_os(name).is_some_and(|value| !value.is_empty()) +} + +fn terminal_width() -> u16 { + terminal_size::terminal_size_of(std::io::stdout()) + .map(|(Width(width), _)| width) + .filter(|width| (20..=240).contains(width)) + .or_else(|| { + env::var("COLUMNS") + .ok() + .and_then(|value| value.parse().ok()) + .filter(|width| (20..=240).contains(width)) + }) + .unwrap_or(80) +} diff --git a/crates/skilld-native/src/native_auth.rs b/crates/skilld-native/src/native_auth.rs index b1143227..8b953a2d 100644 --- a/crates/skilld-native/src/native_auth.rs +++ b/crates/skilld-native/src/native_auth.rs @@ -203,10 +203,7 @@ fn command_auth_error(error: AuthError) -> CommandError { AuthErrorKind::UnsupportedCapability => "UNSUPPORTED_HOST", _ => "SERVICE_UNAVAILABLE", }; - CommandError { - code, - message: error.message().to_owned(), - } + CommandError::operation(code, error.message()) } fn remote_auth_error(error: AuthError) -> RemoteError { diff --git a/crates/skilld-native/tests/cli.rs b/crates/skilld-native/tests/cli.rs index 24d720eb..98ddabcf 100644 --- a/crates/skilld-native/tests/cli.rs +++ b/crates/skilld-native/tests/cli.rs @@ -1,7 +1,37 @@ use std::fs; +#[cfg(unix)] +use std::fs::File; +#[cfg(unix)] +use std::io::Read; use std::path::{Path, PathBuf}; +#[cfg(unix)] +use std::process::Stdio; use std::process::{Command, Output}; +#[cfg(unix)] +use nix::pty::{Winsize, openpty}; + +const DETECTION_SIGNALS: [&str; 18] = [ + "CLAUDE_CODE", + "CLAUDECODE", + "CLAUDE_CODE_ENTRYPOINT", + "CLAUDE_CONFIG_DIR", + "CURSOR_SESSION", + "CURSOR_TRACE_ID", + "WINDSURF_SESSION", + "CLINE_TASK_ID", + "CLINE_ACTIVE", + "COPILOT_RUN_APP", + "GEMINI_CLI", + "GOOSE_SESSION", + "AGENT_SESSION_ID", + "AMP_SESSION", + "OPENCODE_SESSION", + "OPENCODE_SESSION_ID", + "ROO_SESSION", + "ANTIGRAVITY_CLI_ALIAS", +]; + fn binary() -> PathBuf { PathBuf::from(env!("CARGO_BIN_EXE_skilld")) } @@ -11,26 +41,6 @@ fn fixture() -> PathBuf { } fn run(project: &Path, data: &Path, home: &Path, args: &[&str]) -> Output { - const SIGNALS: [&str; 18] = [ - "CLAUDE_CODE", - "CLAUDECODE", - "CLAUDE_CODE_ENTRYPOINT", - "CLAUDE_CONFIG_DIR", - "CURSOR_SESSION", - "CURSOR_TRACE_ID", - "WINDSURF_SESSION", - "CLINE_TASK_ID", - "CLINE_ACTIVE", - "COPILOT_RUN_APP", - "GEMINI_CLI", - "GOOSE_SESSION", - "AGENT_SESSION_ID", - "AMP_SESSION", - "OPENCODE_SESSION", - "OPENCODE_SESSION_ID", - "ROO_SESSION", - "ANTIGRAVITY_CLI_ALIAS", - ]; let mut command = Command::new(binary()); command .current_dir(project) @@ -38,12 +48,72 @@ fn run(project: &Path, data: &Path, home: &Path, args: &[&str]) -> Output { .env("HOME", home) .env("XDG_CONFIG_HOME", home.join(".config")) .args(args); - for signal in SIGNALS { + for signal in DETECTION_SIGNALS { command.env_remove(signal); } command.output().unwrap() } +#[cfg(unix)] +fn run_output_probe_in_pty(signal: (&str, &str), width: u16) -> String { + let pair = openpty( + Some(&Winsize { + ws_row: 24, + ws_col: width, + ws_xpixel: 0, + ws_ypixel: 0, + }), + None, + ) + .unwrap(); + let mut master = File::from(pair.master); + let slave = File::from(pair.slave); + let slave_stdout = slave.try_clone().unwrap(); + let slave_stderr = slave.try_clone().unwrap(); + let mut command = Command::new(binary()); + for name in DETECTION_SIGNALS { + command.env_remove(name); + } + let mut child = command + .env_remove("CI") + .env_remove("COLUMNS") + .env("NO_COLOR", "1") + .env("TERM", "xterm-256color") + .env("SKILLD_PROBE_SEARCH_OUTPUT", "1") + .env(signal.0, signal.1) + .stdin(Stdio::from(slave)) + .stdout(Stdio::from(slave_stdout)) + .stderr(Stdio::from(slave_stderr)) + .spawn() + .unwrap(); + drop(command); + + let status = child.wait().unwrap(); + let mut output = String::new(); + let _ = master.read_to_string(&mut output); + assert!(status.success()); + output.replace("\r\n", "\n") +} + +#[cfg(unix)] +#[test] +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("Skill search")); +} + +#[cfg(unix)] +#[test] +fn config_directory_alone_keeps_human_output_in_a_terminal() { + let output = run_output_probe_in_pty(("CLAUDE_CONFIG_DIR", "/tmp/claude-config"), 40); + + assert!(output.contains("Skill search output")); + assert!(output.contains("Install: skilld install")); + assert!(output.lines().all(|line| line.chars().count() <= 40)); +} + #[test] fn version_reports_the_rust_package_version() { let output = Command::new(binary()).arg("--version").output().unwrap(); @@ -286,7 +356,7 @@ fn native_auth_status_surfaces_an_unavailable_os_credential_store() { .output() .unwrap(); - assert_eq!(output.status.code(), Some(2)); + assert_eq!(output.status.code(), Some(1)); assert_eq!( String::from_utf8(output.stderr).unwrap(), "SERVICE_UNAVAILABLE: The OS keychain operation failed.\n" diff --git a/crates/skilld-native/tests/http.rs b/crates/skilld-native/tests/http.rs index 79c9d16f..dee52a16 100644 --- a/crates/skilld-native/tests/http.rs +++ b/crates/skilld-native/tests/http.rs @@ -33,8 +33,9 @@ fn native_http_adapter_reaches_the_v1_skill_search_route() { .with_endpoint(&format!("http://{address}")) .unwrap(); - let results = remote.search("testing", 20).unwrap(); + let response = remote.search("testing", 20).unwrap(); - assert_eq!(results[0].name, "vue-testing"); + assert_eq!(response.items[0].name, "vue-testing"); + assert_eq!(response.total, 1); server.join().unwrap(); } diff --git a/crates/skilld-wasi/src/lib.rs b/crates/skilld-wasi/src/lib.rs index fa0a6cee..0a8e6d9e 100644 --- a/crates/skilld-wasi/src/lib.rs +++ b/crates/skilld-wasi/src/lib.rs @@ -1,7 +1,9 @@ use std::env; use std::path::PathBuf; -use skilld_command::{CommandError, Host, LocalHost, run, run_stdio_probe}; +use skilld_command::{ + CommandError, Host, LocalHost, OutputContext, run_stdio_probe, run_with_output, +}; use skilld_core::{InstallScope, InstallSource}; wit_bindgen::generate!({ @@ -37,7 +39,13 @@ impl Guest for SkilldComponent { let host = WasiHost { local }; let mut stdout = std::io::stdout().lock(); let mut stderr = std::io::stderr().lock(); - let result = run(env::args_os(), &host, &mut stdout, &mut stderr); + let result = run_with_output( + env::args_os(), + &host, + OutputContext::Plain, + &mut stdout, + &mut stderr, + ); result.exit_code.into() } } diff --git a/docs/migrate-v2-to-v3.md b/docs/migrate-v2-to-v3.md index 98af919d..edc036b5 100644 --- a/docs/migrate-v2-to-v3.md +++ b/docs/migrate-v2-to-v3.md @@ -111,7 +111,7 @@ It cannot restore a v2 lockfile. | v2 command | v3 replacement | | --- | --- | | `skilld add ` | Run `skilld search`, then `skilld install ` | -| `skilld update [name]` | `skilld upgrade [name]` | +| `skilld update [name]` | `skilld update [name]` | | `skilld info` | `skilld list`, then `skilld view ` | | `skilld login` | `skilld auth login` | | `skilld whoami` | `skilld auth status` | diff --git a/skills/skilld/SKILL.md b/skills/skilld/SKILL.md index 6c8da409..2022b177 100644 --- a/skills/skilld/SKILL.md +++ b/skills/skilld/SKILL.md @@ -1,6 +1,6 @@ --- name: skilld -description: Search, view, install, upgrade, verify, and remove Skills with skilld CLI, including private repository access. +description: Search, view, install, update, verify, and remove Skills with skilld CLI, including private repository access. --- # Use skilld CLI @@ -12,12 +12,19 @@ Use skilld CLI to search for and install Skills. Run a focused search: ```sh -skilld search +skilld search --json ``` -Read the result names and descriptions before choosing. +Read `data.items` before choosing a Skill. +Use each item's `selector` for install. Refine the query when several Skills cover different tasks. +Always use `--json` when an Agent runs Skill search. +Check the exit code before reading stdout. +If search fails, read the tagged JSON error from stderr. +Use `--plain` only when another command needs stable text. +Never parse formatted terminal output. + ## Install a Skill Install the selector returned by search into the detected Agent target: @@ -57,11 +64,18 @@ Use `view` to show one Skill's path, source status, and Agent targets. ## Maintain installed Skills ```sh -skilld upgrade +skilld update +skilld update --check --json skilld verify skilld remove ``` -Use `upgrade` to install a newer Artifact. +Use `update --check --json` to inspect update relations without changing files. +Read each `data.items[].relation._tag` before changing files. +Use `update ` only when the relation is `available`. +Treat `current`, `pinned`, and `notTracked` as no action. +If the relation is `behind` or `diverged`, ask before changing files. +If the relation is `unavailable`, report `failure.code` and `failure.message`. +Treat `unavailable` as unknown. Do not infer a newer commit. Use `verify` to check the installed bytes and source status. Use `remove` only when the request names the Skill to remove. diff --git a/tests/fixtures/v3-rust/v1/update-check.json b/tests/fixtures/v3-rust/v1/update-check.json new file mode 100644 index 00000000..3e27f108 --- /dev/null +++ b/tests/fixtures/v3-rust/v1/update-check.json @@ -0,0 +1,74 @@ +{ + "schemaVersion": 1, + "_tag": "Success", + "command": "update", + "data": { + "items": [ + { + "name": "available-skill", + "relation": { + "_tag": "available", + "lockedCommitSha": "1111111111111111111111111111111111111111", + "latestCommitSha": "2222222222222222222222222222222222222222", + "aheadBy": 3 + } + }, + { + "name": "behind-skill", + "relation": { + "_tag": "behind", + "lockedCommitSha": "2222222222222222222222222222222222222222", + "latestCommitSha": "1111111111111111111111111111111111111111", + "behindBy": 2 + } + }, + { + "name": "current-skill", + "relation": { + "_tag": "current", + "commitSha": "1111111111111111111111111111111111111111" + } + }, + { + "name": "diverged-skill", + "relation": { + "_tag": "diverged", + "lockedCommitSha": "1111111111111111111111111111111111111111", + "latestCommitSha": "2222222222222222222222222222222222222222", + "aheadBy": 4, + "behindBy": 2 + } + }, + { + "name": "local-skill", + "relation": { + "_tag": "notTracked", + "reason": "local" + } + }, + { + "name": "pinned-skill", + "relation": { + "_tag": "pinned", + "commitSha": "1111111111111111111111111111111111111111" + } + }, + { + "name": "unavailable-skill", + "relation": { + "_tag": "unavailable", + "lockedCommitSha": "1111111111111111111111111111111111111111", + "latestCommit": { + "_tag": "known", + "commitSha": "2222222222222222222222222222222222222222" + }, + "failure": { + "code": "COMPARISON_UNAVAILABLE", + "message": "Git comparison is unavailable." + } + } + } + ] + }, + "notices": [] +}