diff --git a/CLAUDE.md b/CLAUDE.md index e9761f15..9031a80a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -24,9 +24,12 @@ cargo clippy --workspace --all-targets -- -D warnings ## Product boundary -The native `skilld` CLI searches, installs, lists, views, removes, updates, and verifies Skills. +The native `skilld` CLI searches, runs, installs, lists, views, removes, updates, and verifies Skills. It also manages account authentication and Agent target configuration. +`skilld run` loads a transient Skill: it prints the Skill and installs nothing. +The calling Agent follows the instructions. The CLI never executes them. + The skilld CLI contains no Skill generation logic or Agent runtime. `skilld-harness` runs visible skilld-maintained Skills for generation and review. diff --git a/CONTEXT.md b/CONTEXT.md index 74b49184..9bd8cc0a 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -6,7 +6,7 @@ Use the terms in `GLOSSARY.md` exactly. - The skilld CLI is Rust. - skilld-maintained Skills support direct Agent generation, review, search, and install. -- `skilld install skilld --global` installs search and install guidance for Agents. +- `skilld install skilld --global` installs search, run, and install guidance for Agents. - Harness is the JavaScript `skilld-harness` package. - `skilld.dev` resolves Repositories into Artifacts with attestations. diff --git a/GLOSSARY.md b/GLOSSARY.md index 986dd5f1..72fffd4c 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -9,6 +9,7 @@ Every public export, command, error, route, and document uses these terms. | Term | Export or owner | Stability | Consumers | Customer word | | --- | --- | --- | --- | --- | | Skill | Agent Skills specification | external standard | Agent, skilld CLI, Harness | Skill | +| transient Skill | `skilld run` | published command | Agent, developer | transient Skill | | skilld-maintained Skill | `skills/*` | published asset | Agent, Harness | skilld-maintained Skill | | skilld CLI | `skilld` | published CLI | developer, CI | skilld CLI | | Harness | `skilld-harness` | published package | application, CI | Harness | @@ -25,6 +26,8 @@ Every public export, command, error, route, and document uses these terms. | Identifier | Term | | --- | --- | | `skilld search` | Skill search | +| `skilld run` | transient Skill load | +| `skilld run --file` | supporting file read | | `skilld install` | Skill install | | `skilld list` | installed Skills | | `skilld view` | Skill details | @@ -72,7 +75,10 @@ flowchart LR Collisions -None recorded. +`Skill run` and `transient Skill` sound alike and mean different things. +A Skill run is one Harness execution. +A transient Skill is one Skill that `skilld run` loads for a session. +The Rust type for the second is `TransientSkill`, never `SkillRun`. ## Terms @@ -96,6 +102,16 @@ None recorded. **Casing:** `skilld-maintained Skill` in prose. +### transient Skill + +**Is:** a Skill that `skilld run` loads for the current Agent session. + +**Use for:** any Skill used without an install. A remote transient Skill never reaches disk. + +**Never:** ephemeral skill, temporary install, one-off install, Skill run. + +**Casing:** `transient Skill` in prose, `TransientSkill` in Rust. + ### skilld CLI **Is:** the Rust command line interface that searches, installs, updates, and removes Skills. diff --git a/README.md b/README.md index 678fa85f..16273afc 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ [![npm downloads](https://img.shields.io/npm/dm/skilld?color=yellow)](https://npm.chart.dev/skilld) [![license](https://img.shields.io/npm/l/skilld?color=yellow)](https://github.com/skilld-dev/skilld/blob/main/LICENSE) -Search, install, and keep Agent Skills current. +Search, run, install, and keep Agent Skills current. skilld v3 has two products: @@ -22,7 +22,13 @@ npm install --global skilld The npm package selects a native executable for the current system. It has no JavaScript CLI engine or JavaScript fallback. -Install the skilld-maintained Skill for your Agent: +Ask your Agent to run the skilld-maintained Skill for the current session: + +```sh +skilld run skilld +``` + +Install it only when you want your Agent to keep it across sessions: ```sh skilld install skilld --global @@ -34,12 +40,52 @@ Use `--agent` when you want an explicit Agent target: skilld install skilld --global --agent codex ``` +## Run a Skill without installing it + +`skilld run` is the default way to use a Skill. +It prints `SKILL.md` to stdout. +Ask your Agent to run the command and follow the printed instructions. +If you run it yourself, pass the output to your Agent. + +```sh +npx skilld run skilld:skilld-dev/skills/vue +``` + +A remote run writes no Skill files. +It creates no lockfile entry, Agent target, project file, or Skill cache. +The command retains no Skill files after it exits. + +skilld names the supporting files a Skill carries and prints none of them. +Read one when the instructions call for it: + +```sh +npx skilld run skilld:skilld-dev/skills/vue --revision --file references/api.md +``` + +Use the revision and file-read command from the initial output. +skilld never prints executable or binary files. +A Skill that must run its own script needs an install. + +Install the Skill when you want it in every session: + +```sh +skilld install skilld:skilld-dev/skills/vue +``` + +An install writes files. Ask the user first. + ## Use the skilld CLI ```sh # Find a Skill skilld search vue +# Run a Skill for this session only +skilld run skilld:skilld-dev/skills/vue + +# Read one supporting file that Skill carries +skilld run skilld:skilld-dev/skills/vue --revision --file references/api.md + # Install a Skill in the current project skilld install skilld:skilld-dev/skills/vue @@ -84,6 +130,7 @@ The API does not expose private storage addresses. ### Direct mode `--direct` fetches a public GitHub Repository without the skilld.dev API. +Explicit GitHub selectors use hosted Artifact delivery unless you add `--direct`. ```sh skilld install github:skilld-dev/skilld/skills/skilld --direct --agent codex @@ -95,6 +142,9 @@ The user reviews the Skill before use. Direct mode never handles private Repositories. It never falls back to skilld.dev. +Generated commands use POSIX shell quoting on Unix. +They use PowerShell quoting on Windows. + ## Source status - `verified`: skilld checked a skilld.dev Artifact and its attestation. diff --git a/crates/skilld-command/src/lib.rs b/crates/skilld-command/src/lib.rs index 641cf73f..6729f0aa 100644 --- a/crates/skilld-command/src/lib.rs +++ b/crates/skilld-command/src/lib.rs @@ -4,6 +4,7 @@ mod outdated; pub use outdated::{NoOutdatedProgress, OutdatedProgress, ancestor_roots}; mod output; mod remote; +mod run; use std::collections::{BTreeMap, BTreeSet}; use std::ffi::OsString; @@ -19,7 +20,7 @@ pub use local_store::{ AllowTransaction, LocalStore, PreparedStoreUpdate, ResolvedTarget, SkillView, StoreError, TargetInstall, TransactionGate, }; -pub use output::OutputContext; +pub use output::{CommandPlatform, OutputContext}; pub use remote::{ Cancellation, HeaderValue, HttpAdapter, HttpHeader, HttpMethod, HttpRequest, HttpResponse, NativeRemoteConfig, NeverCancelled, NoTokenProvider, PreparedRemoteSkill, @@ -27,18 +28,22 @@ pub use remote::{ RemoteProvider, RemoteSourceState, RemoteUpdateComparison, RemoteUpdateResult, SecretValue, SkilldRemote, Sleeper, ThreadSleeper, TokenProvider, }; +pub use run::{ + FileContent, FileKind, PulledFile, RunOutcome, SkillOrigin, SupportingFile, TransientSkill, +}; use skilld_core::{ AGENT_TARGETS, AgentTargetId, CommitHistory, CommitSha, DomainError, GlobalTargetPath, InstallMode, InstallOperation, InstallRequest, InstallScope, InstallSource, LockedSource, - NotTrackedReason, SourceRef, UpdateFailure, UpdateLatestCommit, UpdateModelError, UpdatePlan, - UpdatePlanItem, UpdatePlanV1, UpdateRelation, UpdateRetryAfter, VERSION, - classify_update_comparison, select_target_ids, + NotTrackedReason, RemoteSelector, SourceRef, UpdateFailure, UpdateLatestCommit, + UpdateModelError, UpdatePlan, UpdatePlanItem, UpdatePlanV1, UpdateRelation, UpdateRetryAfter, + VERSION, classify_update_comparison, select_target_ids, }; +use skilld_ui::text::is_unsafe_terminal; use skilld_ui::{Detail, Line, Marker, Screen}; use output::{ - OutputMode, SearchItem, SearchOutcome, render_error, render_search, render_update_check, - resolve_mode, + OutputMode, SearchItem, SearchOutcome, render_error, render_run, render_search, + render_update_check, resolve_mode, screen_message, shell_command, }; const DIRECT_SOURCE_GUIDANCE: &str = "--direct requires a github:OWNER/REPOSITORY/SKILL_PATH source or a GitHub tree URL. Remove --direct, then run the same command again."; @@ -47,7 +52,7 @@ const DIRECT_SOURCE_GUIDANCE: &str = "--direct requires a github:OWNER/REPOSITOR #[command( name = "skilld", version = VERSION, - about = "Search, install, and keep Skills current", + about = "Search, run, install, and keep Skills current", disable_help_subcommand = true )] pub struct Cli { @@ -67,7 +72,7 @@ enum Command { Search { query: Vec }, /// Install a Skill, or restore the Skills recorded in your lockfile. #[command( - long_about = "Install a Skill, or restore the Skills recorded in your lockfile.\n\nGive SOURCE as:\n skilld:OWNER/REPOSITORY/SKILL\n Install a hosted Artifact.\n github:OWNER/REPOSITORY/SKILL_PATH\n github:OWNER/REPOSITORY/SKILL_PATH#branch:BRANCH\n github:OWNER/REPOSITORY/SKILL_PATH#tag:TAG\n github:OWNER/REPOSITORY/SKILL_PATH#commit:SHA\n https://github.com/OWNER/REPOSITORY/tree/REF/SKILL_PATH\n Public GitHub Repository paths. Each one requires --direct.\n ./RELATIVE_PATH or ABSOLUTE_PATH\n Install a local Skill.\n skilld\n Install the skilld-maintained Skill with --global.\n\nRun skilld install without SOURCE to restore .skills/skilld-lock.yaml.\nVerified remote Skills restore the exact locked Git commit.", + long_about = "Install a Skill, or restore the Skills recorded in your lockfile.\n\nGive SOURCE as:\n skilld:OWNER/REPOSITORY/SKILL\n Install a hosted Artifact.\n github:OWNER/REPOSITORY/SKILL_PATH\n github:OWNER/REPOSITORY/SKILL_PATH#branch:BRANCH\n github:OWNER/REPOSITORY/SKILL_PATH#tag:TAG\n github:OWNER/REPOSITORY/SKILL_PATH#commit:SHA\n https://github.com/OWNER/REPOSITORY/tree/REF/SKILL_PATH\n Install a hosted Artifact from an explicit GitHub selector.\n Add --direct to fetch a public GitHub Repository instead.\n ./RELATIVE_PATH or ABSOLUTE_PATH\n Install a local Skill.\n skilld\n Install the skilld-maintained Skill with --global.\n\nRun skilld install without SOURCE to restore .skills/skilld-lock.yaml.\nVerified remote Skills restore the exact locked Git commit.", after_long_help = "Examples:\n skilld install skilld:skilld-dev/skills/find-skill --agent codex\n skilld install github:skilld-dev/skilld/skills/skilld --direct --agent codex\n skilld install" )] Install { @@ -93,7 +98,35 @@ enum Command { mode: Option, #[arg( long, - long_help = "Fetch a public GitHub Repository without going through skilld.dev.\nGive a github: source or a GitHub tree URL.\nA direct install records the unverified source status." + long_help = "Fetch a public GitHub Repository without going through skilld.dev.\nGive an explicit github: source or a GitHub tree URL.\nWithout --direct, these selectors use hosted Artifact delivery.\nA direct install records the unverified source status." + )] + direct: bool, + }, + /// Load a Skill for this session without installing it. + #[command( + long_about = "Load a Skill for this session without installing it.\n\nskilld run prints SKILL.md so the calling Agent follows it now.\nA remote run retains no Skill files. It creates no lockfile entry, Agent target,\nor project file.\n\nskilld names the supporting files and prints none of them.\nUse --file to read one. Remote file reads also require the returned --revision.\nUse skilld install to put supporting files on disk.\n\nGive SOURCE in the same forms skilld install accepts.", + after_long_help = "Examples:\n npx skilld run skilld:skilld-dev/skills/find-skill\n skilld run ./skills/my-skill --file references/api.md\n skilld run github:skilld-dev/skilld/skills/skilld --direct\n skilld run ./skills/my-skill" + )] + Run { + /// The Skill source to load. + #[arg(value_name = "SOURCE")] + source: String, + #[arg( + long = "file", + value_name = "PATH", + long_help = "Read one supporting file the Skill carries. Repeat --file for several.\nGive the path exactly as the Skill inventory reports it.\nRemote reads require --revision. Local and bundled reads do not.\nskilld never prints executable or binary files. Install the Skill to use one." + )] + files: Vec, + #[arg( + long, + value_name = "COMMIT", + requires = "files", + long_help = "Read supporting files from one exact remote Git commit.\nUse the revision that an earlier skilld run returned." + )] + revision: Option, + #[arg( + long, + long_help = "Fetch a public GitHub Repository without going through skilld.dev.\nGive a github: source or a GitHub tree URL.\nA direct run carries the unverified source status." )] direct: bool, }, @@ -183,6 +216,17 @@ pub trait Host { self.install(source, request.scope).map(|name| vec![name]) } + fn run_skill( + &self, + _source: InstallSource, + _files: &[String], + _revision: Option<&CommitSha>, + ) -> Result { + Err(CommandError::unsupported_host( + "Skill runs are unavailable on this host", + )) + } + fn view(&self, _name: &str, _scope: InstallScope) -> Result { Err(CommandError::unsupported_host( "Skill details are unavailable on this host", @@ -226,7 +270,7 @@ pub trait Host { )) } - fn outdated(&self, _all: bool) -> Result, CommandError> { + fn outdated(&self, _all: bool, _platform: CommandPlatform) -> Result, CommandError> { Err(CommandError::unsupported_host( "Outdated Skill reports are unavailable on this host", )) @@ -315,20 +359,40 @@ impl CommandError { Self::usage("INVALID_SOURCE", message) } - fn direct_local_source() -> Self { + fn direct_local_install_source() -> Self { Self::usage( "DIRECT_SOURCE_REQUIRED", "--direct cannot install a local Skill. Remove --direct, then run the same command again.", ) } - fn direct_bundled_source() -> Self { + fn direct_bundled_install_source() -> Self { Self::usage( "DIRECT_SOURCE_REQUIRED", "--direct cannot install the skilld-maintained Skill. Run skilld install skilld --global instead", ) } + fn direct_local_run_source() -> Self { + Self::usage( + "DIRECT_SOURCE_REQUIRED", + "--direct cannot run a local Skill. Remove --direct, then run the same command again.", + ) + } + + fn direct_bundled_run_source() -> Self { + Self::usage( + "DIRECT_SOURCE_REQUIRED", + "--direct cannot run the skilld-maintained Skill. Run skilld run skilld without --direct.", + ) + } + + fn remote_file_revision() -> Self { + Self::input( + "Remote --file reads require --revision. Run the Skill without --file first. Then repeat this run with the returned revision.", + ) + } + pub fn config(message: impl Into) -> Self { Self::usage("INVALID_CONFIG", message) } @@ -402,6 +466,7 @@ enum CommandOutput { Screen(Screen), Search(SearchOutcome), UpdateCheck(UpdatePlanV1), + Run(RunOutcome), } pub fn run(args: I, host: &H, stdout: &mut O, stderr: &mut E) -> CommandResult @@ -412,7 +477,15 @@ where O: Write, E: Write, { - run_with_output(args, host, OutputContext::Plain, stdout, stderr) + run_with_output( + args, + host, + OutputContext::Plain { + platform: CommandPlatform::current(), + }, + stdout, + stderr, + ) } pub fn run_with_output( @@ -432,7 +505,9 @@ where 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 + OutputMode::Plain { + platform: context.platform(), + } } else { resolve_mode(requested_json, requested_plain, context) }; @@ -464,8 +539,18 @@ where requested_mode, ) } + } else if display || matches!(requested_mode, OutputMode::Human { .. }) { + terminal_safe_clap_text(&args, error.kind()).into_bytes() } else { - error.to_string().into_bytes() + let message = error + .to_string() + .trim() + .trim_start_matches("error: ") + .to_owned(); + render_error( + &CommandError::usage("INVALID_ARGUMENT", message), + requested_mode, + ) }; if target.write_all(&rendered).is_err() { return CommandResult { @@ -487,11 +572,12 @@ where return CommandResult { exit_code: 2 }; } let supports_json = matches!(&cli.command, Command::Search { .. }) + || matches!(&cli.command, Command::Run { .. }) || 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", + "JSON output is available for Skill search, Skill runs and update checks", ); if stderr.write_all(&render_error(&error, mode)).is_err() { return CommandResult { exit_code: 2 }; @@ -499,11 +585,11 @@ where return CommandResult { exit_code: 2 }; } - match dispatch(cli.command, host) { + match dispatch(cli.command, host, context.platform()) { Ok(CommandOutput::Screen(screen)) => { let bytes = match mode { OutputMode::Human { color, .. } => screen.render_human(color), - OutputMode::Plain | OutputMode::JsonV1 => screen.render_plain(), + OutputMode::Plain { .. } | OutputMode::JsonV1 => screen.render_plain(), }; write_success(bytes.as_bytes(), mode, stdout, stderr) } @@ -520,6 +606,19 @@ where } } }, + Ok(CommandOutput::Run(outcome)) => match render_run(&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) => { let exit_code = if outcome.is_incomplete() { @@ -555,6 +654,42 @@ where } } +fn terminal_safe_clap_text(args: &[OsString], expected_kind: ErrorKind) -> String { + let safe_args = args.iter().map(terminal_safe_argument); + let text = match Cli::try_parse_from(safe_args) { + Err(error) if error.kind() == expected_kind => error.to_string(), + _ => "error: invalid command arguments\n\nFor more information, try '--help'.\n".to_owned(), + }; + let mut safe = String::new(); + for character in text.chars() { + match character { + '\n' | '\t' => safe.push(character), + '\r' => safe.push_str("\\r"), + character if is_unsafe_terminal(character) => { + safe.push_str(&format!("\\u{{{:04X}}}", u32::from(character))); + } + character => safe.push(character), + } + } + safe +} + +fn terminal_safe_argument(argument: &OsString) -> OsString { + let mut safe = String::new(); + for character in argument.to_string_lossy().chars() { + match character { + '\n' => safe.push_str("\\n"), + '\r' => safe.push_str("\\r"), + '\t' => safe.push_str("\\t"), + character if is_unsafe_terminal(character) => { + safe.push_str(&format!("\\u{{{:04X}}}", u32::from(character))); + } + character => safe.push(character), + } + } + OsString::from(safe) +} + fn requested_output(args: &[OsString]) -> (bool, bool) { let mut json = false; let mut plain = false; @@ -573,7 +708,7 @@ fn requested_output(args: &[OsString]) -> (bool, bool) { fn display_path(args: &[OsString]) -> String { let commands = [ - "search", "install", "list", "view", "remove", "update", "verify", "auth", "config", + "search", "install", "run", "list", "view", "remove", "update", "verify", "auth", "config", ]; let mut path = vec!["skilld"]; if let Some(command) = args @@ -640,7 +775,11 @@ pub fn run_stdio_probe( } } -fn dispatch(command: Command, host: &H) -> Result { +fn dispatch( + command: Command, + host: &H, + platform: CommandPlatform, +) -> Result { match command { Command::Install { source, @@ -659,10 +798,10 @@ fn dispatch(command: Command, host: &H) -> Result { - return Err(CommandError::direct_local_source()); + return Err(CommandError::direct_local_install_source()); } (true, InstallSource::BundledSkilld) => { - return Err(CommandError::direct_bundled_source()); + return Err(CommandError::direct_bundled_install_source()); } (false, source) => InstallOperation::Install(source), }, @@ -700,6 +839,53 @@ fn dispatch(command: Command, host: &H) -> Result { + run::reject_duplicate_files(&files)?; + let revision = revision.map(CommitSha::parse).transpose().map_err(|_| { + CommandError::input("--revision must use 40 lowercase hexadecimal characters") + })?; + let source = match (direct, InstallSource::parse(&source)) { + (true, InstallSource::Remote(source) | InstallSource::DirectRemote(source)) => { + InstallSource::DirectRemote(source) + } + (true, InstallSource::Local(_)) => { + return Err(CommandError::direct_local_run_source()); + } + (true, InstallSource::BundledSkilld) => { + return Err(CommandError::direct_bundled_run_source()); + } + (false, source) => source, + }; + if !files.is_empty() + && revision.is_none() + && matches!( + source, + InstallSource::Remote(_) | InstallSource::DirectRemote(_) + ) + { + return Err(CommandError::remote_file_revision()); + } + if revision.is_some() + && !matches!( + source, + InstallSource::Remote(_) | InstallSource::DirectRemote(_) + ) + { + return Err(CommandError::input( + "--revision requires a remote Skill source", + )); + } + Ok(CommandOutput::Run(host.run_skill( + source, + &files, + revision.as_ref(), + )?)) + } Command::List { global } => host.list(scope(global)).map(|names| { CommandOutput::Screen(Screen::new(names.into_iter().map(Line::item).collect())) }), @@ -804,7 +990,7 @@ fn dispatch(command: Command, host: &H) -> Result host - .outdated(all) + .outdated(all, platform) .map(|lines| CommandOutput::Screen(Screen::new(lines))), } } @@ -813,10 +999,10 @@ 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, .. } => match github_url(&source) { - Some(url) => Line::linked_field("Source", source, url), - None => Line::field("Source", source), - }, + LockedSource::Remote { source, .. } => { + let selector = RemoteSelector::parse(&source).map_err(CommandError::remote)?; + Line::linked_field("Source", selector.canonical(), github_url(&selector)?) + } }; let targets = if view.skill.targets.is_empty() { "none".to_owned() @@ -837,15 +1023,15 @@ fn render_view(view: SkillView) -> Result, CommandError> { ]) } -/// A GitHub repository URL for a remote Skill source, when the source names -/// one. -fn github_url(source: &str) -> Option { - let body = source.split_once(':')?.1; - let mut segments = body.split('/'); - let owner = segments.next()?; - let repository = segments.next()?; - (!owner.is_empty() && !repository.is_empty()) - .then(|| format!("https://github.com/{owner}/{repository}")) +/// 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 { @@ -891,6 +1077,7 @@ impl TargetRoots { } pub trait BundledSkillProvider: Send + Sync { + fn skilld_run_files(&self) -> Result, CommandError>; fn skilld_source(&self) -> Result; } @@ -906,6 +1093,10 @@ struct DirectoryBundledSkillProvider { } impl BundledSkillProvider for DirectoryBundledSkillProvider { + fn skilld_run_files(&self) -> Result, CommandError> { + run::read_local(&self.path).map(|(_, files)| files) + } + fn skilld_source(&self) -> Result { Ok(self.path.clone()) } @@ -1128,6 +1319,161 @@ impl LocalHost { Ok(name.to_string()) } + fn run_remote( + &self, + source: &str, + direct: bool, + wanted: &[String], + expected_revision: Option<&CommitSha>, + ) -> Result { + let selector = skilld_core::RemoteSelector::parse(source).map_err(CommandError::remote)?; + if let (Some(revision), Some(SourceRef::Commit { value })) = + (expected_revision, &selector.source().r#ref) + && value != revision.as_str() + { + return Err(CommandError::operation( + "SOURCE_MISMATCH", + "the source commit does not match --revision", + )); + } + let provider = self.remote_provider()?; + let prepared = match expected_revision { + Some(revision) => provider.prepare_exact(&selector, revision, direct), + None => provider.prepare(&selector, direct), + } + .map_err(CommandError::remote)?; + let LockedSource::Remote { + source: locked_source, + commit_sha, + skill_path, + } = &prepared.locked_source + else { + return Err(CommandError::operation( + "SOURCE_MISMATCH", + "the prepared Skill has no remote revision", + )); + }; + let revision = CommitSha::parse(commit_sha.clone()).map_err(|_| { + CommandError::operation( + "SOURCE_MISMATCH", + "the prepared Skill has an invalid remote revision", + ) + })?; + if skill_path.contains('#') { + return Err(CommandError::input( + "the attested Skill source path cannot contain #", + )); + } + if expected_revision.is_some_and(|expected| expected != &revision) { + return Err(CommandError::operation( + "SOURCE_MISMATCH", + "the prepared Skill changed its exact revision", + )); + } + let locked_selector = + skilld_core::RemoteSelector::parse(locked_source).map_err(CommandError::remote)?; + let exact_source = skilld_core::RemoteSelector::parse(&format!( + "github:{}/{}/{}#commit:{}", + locked_selector.source().owner, + locked_selector.source().repository, + skill_path, + revision.as_str(), + )) + .map_err(CommandError::remote)? + .canonical(); + let source_status = prepared.source_status.as_str(); + let (name, _, files) = + skilld_core::prepare_unverified_files(prepared.files).map_err(CommandError::remote)?; + let name = name.as_str().to_owned(); + let origin = SkillOrigin::Remote { + source: selector.canonical(), + exact_source, + direct, + }; + if !wanted.is_empty() { + return Ok(RunOutcome::Files { + files: run::pull_files(&name, &files, wanted)?, + skill: name, + origin, + source_status, + revision: Some(revision.as_str().to_owned()), + }); + } + Ok(RunOutcome::Load(Box::new(TransientSkill { + instructions: run::read_instructions(&files)?, + files: run::supporting_files(&files), + name, + origin, + source_status, + revision: Some(revision.as_str().to_owned()), + }))) + } + + fn run_directory( + &self, + source: InstallSource, + wanted: &[String], + ) -> Result { + let (path, _) = self.resolve_source(source)?; + // The output names this directory to the Agent, so give it the real + // path rather than the one the user typed. + let path = path.canonicalize().unwrap_or(path); + let (name, files) = run::read_local(&path)?; + let origin = SkillOrigin::Local { root: path }; + if !wanted.is_empty() { + return Ok(RunOutcome::Files { + files: run::pull_files(&name, &files, wanted)?, + skill: name, + origin, + source_status: "local", + revision: None, + }); + } + Ok(RunOutcome::Load(Box::new(TransientSkill { + instructions: run::read_instructions(&files)?, + files: run::supporting_files(&files), + name, + origin, + source_status: "local", + revision: None, + }))) + } + + fn run_bundled(&self, wanted: &[String]) -> Result { + let provider = self.bundled_skill.as_ref().ok_or_else(|| { + CommandError::service( + "the bundled skilld-maintained Skill is unavailable in this build", + ) + })?; + let (name, _, files) = skilld_core::prepare_unverified_files(provider.skilld_run_files()?) + .map_err(CommandError::remote)?; + if name.as_str() != "skilld" { + return Err(CommandError::operation( + "SOURCE_MISMATCH", + "the bundled Skill must declare the name skilld", + )); + } + let name = name.as_str().to_owned(); + let origin = SkillOrigin::Bundled; + if !wanted.is_empty() { + return Ok(RunOutcome::Files { + files: run::pull_files(&name, &files, wanted)?, + skill: name, + origin, + source_status: "local", + revision: None, + }); + } + Ok(RunOutcome::Load(Box::new(TransientSkill { + instructions: run::read_instructions(&files)?, + files: run::supporting_files(&files), + name, + origin, + source_status: "local", + revision: None, + }))) + } + fn restore(&self, request: &InstallRequest, direct: bool) -> Result, CommandError> { let (targets, known) = if request.targets.is_empty() { (None, self.known_targets(request.scope)?) @@ -1299,6 +1645,33 @@ impl Host for LocalHost { } } + fn run_skill( + &self, + source: InstallSource, + files: &[String], + revision: Option<&CommitSha>, + ) -> Result { + run::reject_duplicate_files(files)?; + if !files.is_empty() + && revision.is_none() + && matches!( + source, + InstallSource::Remote(_) | InstallSource::DirectRemote(_) + ) + { + return Err(CommandError::remote_file_revision()); + } + match source { + InstallSource::Remote(source) => self.run_remote(&source, false, files, revision), + InstallSource::DirectRemote(source) => self.run_remote(&source, true, files, revision), + InstallSource::BundledSkilld if revision.is_none() => self.run_bundled(files), + source if revision.is_none() => self.run_directory(source, files), + _ => Err(CommandError::input( + "--revision requires a remote Skill source", + )), + } + } + fn view(&self, name: &str, scope: InstallScope) -> Result { let known = self.known_targets(scope)?; let name = skilld_core::SkillName::parse(name.to_owned()).map_err(CommandError::domain)?; @@ -1762,7 +2135,7 @@ impl Host for LocalHost { Ok(UpdatePlanV1::new(plan)) } - fn outdated(&self, all: bool) -> Result, CommandError> { + fn outdated(&self, all: bool, platform: CommandPlatform) -> Result, CommandError> { let scopes = if all { vec![InstallScope::Project, InstallScope::Global] } else { @@ -1781,11 +2154,12 @@ impl Host for LocalHost { let names = match store.list(&known) { Ok(names) => names, Err(error) => { + let message = screen_message(&CommandError::store(error).message); // Without a readable lockfile, managed copies cannot be told from unmanaged ones. lines.push(Line::error(format!( "Skill store unavailable in {} scope: {}", scope.as_str(), - CommandError::store(error).message + message ))); if all { // The ancestor scan must not report Skills this scope cannot verify. @@ -1800,9 +2174,9 @@ impl Host for LocalHost { let view = match store.view(&skill_name, &known) { Ok(view) => view, Err(error) => { + let message = screen_message(&CommandError::store(error).message); lines.push(Line::error(format!( - "Skill {name} details unavailable: {}", - CommandError::store(error).message + "Skill {name} details unavailable: {message}" ))); continue; } @@ -1854,7 +2228,7 @@ impl Host for LocalHost { } for (view, scope) in &views { progress.checking(&view.name); - lines.extend(self.report_outdated_view(view, *scope)); + lines.extend(self.report_outdated_view(view, *scope, platform)); } if all { #[cfg(not(target_os = "wasi"))] @@ -1872,9 +2246,13 @@ impl Host for LocalHost { skill, Some(&candidate), &self.project_root, + platform, )), Ok(None) => no_match.push(skill), - Err(error) => failures.entry(error.message).or_default().push(skill), + Err(error) => failures + .entry(screen_message(&error.message)) + .or_default() + .push(skill), } } lines.extend(outdated::render_no_match(&no_match)); @@ -2296,13 +2674,13 @@ fn update_apply_failure(name: &str, outcome: RemoteComparisonOutcome) -> Command } impl LocalHost { - fn report_outdated_view(&self, view: &SkillView, scope: InstallScope) -> Vec { + fn report_outdated_view( + &self, + view: &SkillView, + scope: InstallScope, + platform: CommandPlatform, + ) -> Vec { let name = &view.name; - let global = if scope == InstallScope::Global { - " --global" - } else { - "" - }; match (&view.skill.source, &view.skill.source_status) { ( LockedSource::Remote { @@ -2328,7 +2706,12 @@ impl LocalHost { )] } Ok(RemoteSourceState::Stale { .. }) => { - let update = format!("skilld update {name}{global}"); + let mut argv = + vec!["skilld".to_owned(), "update".to_owned(), name.to_owned()]; + if scope == InstallScope::Global { + argv.push("--global".to_owned()); + } + let update = shell_command(&argv, platform); vec![Line::record( Marker::Warn, format!("Outdated Skill {name}. Run {update}."), @@ -2338,15 +2721,13 @@ impl LocalHost { )] } Err(error) => { + let message = screen_message(&error.message); vec![Line::record( Marker::Error, - format!( - "Source state unavailable for Skill {name}: {}.", - error.message - ), + format!("Source state unavailable for Skill {name}: {message}."), name, Some("source unavailable".to_owned()), - vec![Detail::plain("error", error.message.clone())], + vec![Detail::plain("error", message)], )] } } @@ -2358,8 +2739,13 @@ impl LocalHost { .iter() .map(|locked| locked.agent) .collect::>(); - let agent_flags = outdated::agent_flags(&agents); - let install = format!("skilld install {source} --direct{global}{agent_flags}"); + let install = outdated::install_command( + source, + true, + scope == InstallScope::Global, + &agents, + platform, + ); vec![Line::record( Marker::Warn, format!("Unverified Skill {name}. Run {install} to update it."), @@ -2680,8 +3066,8 @@ mod tests { assert_eq!( command_names(), [ - "search", "install", "list", "view", "remove", "update", "verify", "outdated", - "auth", "config" + "search", "install", "run", "list", "view", "remove", "update", "verify", + "outdated", "auth", "config" ] ); } diff --git a/crates/skilld-command/src/local_store.rs b/crates/skilld-command/src/local_store.rs index 811fba43..6d26738e 100644 --- a/crates/skilld-command/src/local_store.rs +++ b/crates/skilld-command/src/local_store.rs @@ -10,8 +10,8 @@ use std::time::{SystemTime, UNIX_EPOCH}; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use skilld_core::{ - AgentTargetId, InstallMode, LockDocument, LockedSkill, LockedSource, LockedTarget, SkillName, - SourceStatus, + AgentTargetId, CommitSha, InstallMode, LockDocument, LockedSkill, LockedSource, LockedTarget, + RemoteSelector, SkillName, SourceRef, SourceSelector, SourceStatus, }; const JOURNAL_NAME: &str = ".skilld-transaction"; @@ -143,6 +143,56 @@ pub struct LocalStore { root: PathBuf, } +fn normalize_locked_source(source: &mut LockedSource) -> Result<(), StoreError> { + let LockedSource::Remote { + source, + commit_sha, + skill_path, + } = source + else { + return Ok(()); + }; + let selector = RemoteSelector::parse(source).map_err(|error| { + StoreError::InvalidLockfile(format!("the remote Skill source is invalid: {error}")) + })?; + let commit = CommitSha::parse(commit_sha.clone()).map_err(|error| { + StoreError::InvalidLockfile(format!("the remote Skill commit is invalid: {error}")) + })?; + if let Some(SourceRef::Commit { value }) = &selector.source().r#ref + && value != commit.as_str() + { + return Err(StoreError::InvalidLockfile( + "the remote Skill source commit does not match its locked commit".to_owned(), + )); + } + let exact = RemoteSelector::parse(&format!( + "github:{}/{}/{}#commit:{}", + selector.source().owner, + selector.source().repository, + skill_path, + commit.as_str(), + )) + .map_err(|error| { + StoreError::InvalidLockfile(format!("the remote Skill path is invalid: {error}")) + })?; + let SourceSelector::Path { path } = &exact.source().selector else { + return Err(StoreError::InvalidLockfile( + "the remote Skill path is invalid".to_owned(), + )); + }; + if let SourceSelector::Path { path: source_path } = &selector.source().selector + && source_path != path + { + return Err(StoreError::InvalidLockfile( + "the remote Skill source path does not match its locked path".to_owned(), + )); + } + *source = selector.canonical(); + *commit_sha = commit.as_str().to_owned(); + skill_path.clone_from(path); + Ok(()) +} + impl LocalStore { pub fn new(root: PathBuf) -> Self { Self { @@ -794,7 +844,7 @@ impl LocalStore { )); } let bytes = fs::read(&path).map_err(fs_error)?; - let document: LockDocument = serde_json::from_slice(&bytes).map_err(|_| { + let mut document: LockDocument = serde_json::from_slice(&bytes).map_err(|_| { StoreError::InvalidLockfile("the Skill lockfile is not valid JSON".to_owned()) })?; if document.version != 1 { @@ -803,9 +853,10 @@ impl LocalStore { document.version ))); } - for name in document.skills.keys() { + for (name, skill) in &mut document.skills { SkillName::parse(name.clone()) .map_err(|error| StoreError::InvalidLockfile(error.to_string()))?; + normalize_locked_source(&mut skill.source)?; } Ok(document) } @@ -1240,7 +1291,7 @@ fn hash_skill_tree(root: &Path) -> Result { Ok(hex(&hasher.finalize())) } -fn validate_skill_files(root: &Path) -> Result<(), StoreError> { +pub(crate) fn validate_skill_files(root: &Path) -> Result<(), StoreError> { let metadata = fs::symlink_metadata(root).map_err(fs_error)?; if metadata.file_type().is_symlink() || !metadata.is_dir() { return Err(StoreError::InvalidSource( @@ -1259,7 +1310,7 @@ fn validate_skill_files(root: &Path) -> Result<(), StoreError> { Ok(()) } -fn validate_skill_source(root: &Path) -> Result<(), StoreError> { +pub(crate) fn validate_skill_source(root: &Path) -> Result<(), StoreError> { validate_skill_files(root)?; let directory_name = SkillName::from_source(root) .map_err(|error| StoreError::InvalidSource(error.to_string()))?; diff --git a/crates/skilld-command/src/outdated.rs b/crates/skilld-command/src/outdated.rs index 44ccdc8c..f0b70204 100644 --- a/crates/skilld-command/src/outdated.rs +++ b/crates/skilld-command/src/outdated.rs @@ -8,6 +8,7 @@ use skilld_ui::{Detail, Line, Marker}; use crate::ResolvedTarget; use crate::local_store::normalize_path; +use crate::output::{CommandPlatform, shell_command}; pub trait OutdatedProgress: Send + Sync { fn found(&self, _line: &str) {} @@ -188,18 +189,19 @@ pub(crate) fn render_unmanaged( skill: &UnmanagedSkill, candidate: Option<&SkillCandidate>, display_base: &Path, + platform: CommandPlatform, ) -> Vec { let agents = agent_list(skill); let Some(candidate) = candidate else { return vec![]; }; - let global = if skill.scope == InstallScope::Global { - " --global" - } else { - "" - }; - let agent_flags = agent_flags(&skill.agents); - let install = format!("skilld install {}{global}{agent_flags}", candidate.selector); + let install = install_command( + &candidate.selector, + false, + skill.scope == InstallScope::Global, + &skill.agents, + platform, + ); let plain = format!( "Unmanaged Skill {} ({agents}). Candidate source {}, {} stars.\nDelete {}, then run {install}.", skill.name, @@ -224,16 +226,25 @@ pub(crate) fn render_unmanaged( )] } -pub(crate) fn agent_flags(agents: &[AgentTargetId]) -> String { - if agents.is_empty() { - return String::new(); +pub(crate) fn install_command( + source: &str, + direct: bool, + global: bool, + agents: &[AgentTargetId], + platform: CommandPlatform, +) -> String { + let mut argv = vec!["skilld".to_owned(), "install".to_owned(), source.to_owned()]; + if direct { + argv.push("--direct".to_owned()); } - let flags = agents - .iter() - .map(|agent| format!("--agent {}", agent.as_str())) - .collect::>() - .join(" "); - format!(" {flags}") + if global { + argv.push("--global".to_owned()); + } + for agent in agents { + argv.push("--agent".to_owned()); + argv.push(agent.as_str().to_owned()); + } + shell_command(&argv, platform) } fn agent_list(skill: &UnmanagedSkill) -> String { diff --git a/crates/skilld-command/src/output.rs b/crates/skilld-command/src/output.rs index 149fd84a..deaa9e67 100644 --- a/crates/skilld-command/src/output.rs +++ b/crates/skilld-command/src/output.rs @@ -1,22 +1,51 @@ use clap::error::ErrorKind; use serde::Serialize; use skilld_core::UpdatePlanV1; -use skilld_ui::text::{grouped_number, sanitize, width, wrap}; +use skilld_ui::text::{grouped_number, is_unsafe_terminal, sanitize, width, wrap}; use skilld_ui::{Role, paint}; +use crate::run::{FileContent, PulledFile, RunOutcome, SkillOrigin, TransientSkill}; 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 CommandPlatform { + Unix, + WindowsPowerShell, +} + +impl CommandPlatform { + pub const fn current() -> Self { + if cfg!(windows) { + Self::WindowsPowerShell + } else { + Self::Unix + } + } +} + #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum OutputContext { - HumanTerminal { width: u16, color: bool }, - Plain, + HumanTerminal { + width: u16, + color: bool, + platform: CommandPlatform, + }, + Plain { + platform: CommandPlatform, + }, } impl OutputContext { + pub const fn platform(self) -> CommandPlatform { + match self { + Self::HumanTerminal { platform, .. } | Self::Plain { platform } => platform, + } + } + pub fn auto( stdout_is_terminal: bool, active_agent: bool, @@ -24,21 +53,29 @@ impl OutputContext { no_color: bool, term_is_dumb: bool, width: u16, + platform: CommandPlatform, ) -> Self { if active_agent || ci || !stdout_is_terminal { - return Self::Plain; + return Self::Plain { platform }; } Self::HumanTerminal { width: width.clamp(MIN_WIDTH, MAX_WIDTH), color: !no_color && !term_is_dumb, + platform, } } } #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub(crate) enum OutputMode { - Human { width: u16, color: bool }, - Plain, + Human { + width: u16, + color: bool, + platform: CommandPlatform, + }, + Plain { + platform: CommandPlatform, + }, JsonV1, } @@ -46,11 +83,21 @@ pub(crate) fn resolve_mode(json: bool, plain: bool, context: OutputContext) -> O if json { OutputMode::JsonV1 } else if plain { - OutputMode::Plain + OutputMode::Plain { + platform: context.platform(), + } } else { match context { - OutputContext::HumanTerminal { width, color } => OutputMode::Human { width, color }, - OutputContext::Plain => OutputMode::Plain, + OutputContext::HumanTerminal { + width, + color, + platform, + } => OutputMode::Human { + width, + color, + platform, + }, + OutputContext::Plain { platform } => OutputMode::Plain { platform }, } } } @@ -76,8 +123,12 @@ pub(crate) fn render_search( 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::Human { + width, + color, + platform, + } => Ok(render_human(outcome, width, color, platform).into_bytes()), + OutputMode::Plain { .. } => Ok(render_plain(outcome).into_bytes()), OutputMode::JsonV1 => render_json_success( "search", JsonSearchData { @@ -161,18 +212,34 @@ pub(crate) fn render_error(error: &CommandError, mode: OutputMode) -> Vec { .unwrap_or_else(|_| b"OUTPUT_RENDER_FAILED: error output could not be encoded\n".to_vec()); } match mode { - OutputMode::Human { color, .. } => format!( - "{} {} {}\n", - skilld_ui::paint("✗", skilld_ui::Role::Error, color), - skilld_ui::paint(&error.message, skilld_ui::Role::Emphasis, color), - skilld_ui::paint(&format!("({})", error.code), skilld_ui::Role::Dim, color), + OutputMode::Human { color, .. } => { + let message = sanitize(&error.message); + let code = sanitize(error.code); + format!( + "{} {} {}\n", + skilld_ui::paint("✗", skilld_ui::Role::Error, color), + skilld_ui::paint(&message, skilld_ui::Role::Emphasis, color), + skilld_ui::paint(&format!("({code})"), skilld_ui::Role::Dim, color), + ) + .into_bytes() + } + OutputMode::Plain { .. } => format!( + "{}: {}\n", + escape_plain(error.code), + escape_plain(&error.message) ) .into_bytes(), - OutputMode::Plain => format!("{error}\n").into_bytes(), OutputMode::JsonV1 => unreachable!("JSON errors return early"), } } +pub(crate) fn screen_message(value: &str) -> String { + sanitize(value) + .split_whitespace() + .collect::>() + .join(" ") +} + fn render_plain(outcome: &SearchOutcome) -> String { let mut output = String::new(); for item in &outcome.items { @@ -190,7 +257,12 @@ fn render_plain(outcome: &SearchOutcome) -> String { output } -fn render_human(outcome: &SearchOutcome, terminal_width: u16, color: bool) -> String { +fn render_human( + outcome: &SearchOutcome, + terminal_width: u16, + color: bool, + platform: CommandPlatform, +) -> String { let columns = usize::from(terminal_width); let mut output = String::new(); let heading = format!("Skill search {}", sanitize(&outcome.query)); @@ -250,12 +322,13 @@ fn render_human(outcome: &SearchOutcome, terminal_width: u16, color: bool) -> St output.push('\n'); } } - let install = format!("skilld install {}", sanitize(&item.selector)); - for line in wrap(&install, columns.saturating_sub(2)) { - output.push_str(" "); - output.push_str(&skilld_ui::paint_command(&line, color)); - output.push('\n'); - } + let run = shell_command( + &["skilld".to_owned(), "run".to_owned(), item.selector.clone()], + platform, + ); + output.push_str(" "); + output.push_str(&skilld_ui::paint_command(&run, color)); + output.push('\n'); } output } @@ -268,7 +341,7 @@ fn escape_plain(value: &str) -> String { '\t' => output.push_str("\\t"), '\r' => output.push_str("\\r"), '\n' => output.push_str("\\n"), - character if character.is_control() => { + character if is_unsafe_terminal(character) => { output.push_str(&format!("\\u{{{:04X}}}", u32::from(character))); } character => output.push(character), @@ -326,3 +399,520 @@ struct JsonError<'a> { code: &'a str, message: &'a str, } + +/// Render one transient Skill load, or the supporting files an Agent asked for. +pub(crate) fn render_run(outcome: &RunOutcome, mode: OutputMode) -> Result, CommandError> { + match (outcome, mode) { + (RunOutcome::Load(skill), OutputMode::JsonV1) => render_json_success( + "run", + load_json(skill), + "Skill run output could not be encoded", + ), + ( + RunOutcome::Files { + skill, + origin, + source_status, + revision, + files, + }, + OutputMode::JsonV1, + ) => render_json_success( + "run", + files_json(skill, origin, source_status, revision.as_deref(), files), + "Skill run output could not be encoded", + ), + (RunOutcome::Load(skill), _) => { + Ok(render_load(skill, colored(mode), command_platform(mode)).into_bytes()) + } + ( + RunOutcome::Files { + skill, + origin, + source_status, + revision, + files, + }, + _, + ) => Ok(render_files( + skill, + origin, + source_status, + revision.as_deref(), + files, + colored(mode), + ) + .into_bytes()), + } +} + +const fn colored(mode: OutputMode) -> bool { + matches!(mode, OutputMode::Human { color: true, .. }) +} + +const fn command_platform(mode: OutputMode) -> CommandPlatform { + match mode { + OutputMode::Human { platform, .. } | OutputMode::Plain { platform } => platform, + OutputMode::JsonV1 => unreachable!(), + } +} + +fn render_load(skill: &TransientSkill, color: bool, platform: CommandPlatform) -> String { + let mut out = String::new(); + out.push_str(&format!( + "{}\n", + paint( + &format!( + "skilld loaded the transient Skill {} for this session.", + sanitize(&skill.name) + ), + Role::Emphasis, + color + ) + )); + + match &skill.origin { + SkillOrigin::Bundled => { + out.push_str("This skilld-maintained Skill is bundled with the skilld CLI.\n"); + out.push_str("skilld wrote no Skill files.\n"); + out.push_str(&field("Source", "skilld-maintained Skill", color)); + } + SkillOrigin::Remote { source, .. } => { + 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(&field("Source", source, color)); + } + SkillOrigin::Local { root } => { + out.push_str("This Skill already sits on disk. skilld wrote no Skill files.\n"); + out.push_str(&field("Source", &root.display().to_string(), color)); + } + } + if let Some(revision) = &skill.revision { + out.push_str(&field("Revision", revision, color)); + } + out.push_str(&field("Source status", skill.source_status, color)); + out.push_str(source_status_caution(skill.source_status)); + + out.push('\n'); + out.push_str(&paint("--- SKILL.md ---", Role::Dim, color)); + out.push('\n'); + let instructions = safe_terminal_text(&skill.instructions); + out.push_str(&instructions); + if !instructions.ends_with('\n') { + out.push('\n'); + } + out.push_str(&paint("--- end of SKILL.md ---", Role::Dim, color)); + out.push('\n'); + + out.push('\n'); + out.push_str("Follow these instructions now.\n"); + out.push_str(&render_inventory(skill, color, platform)); + out.push_str(&render_install_guidance(&skill.origin, color, platform)); + out +} + +fn render_inventory(skill: &TransientSkill, color: bool, platform: CommandPlatform) -> String { + if skill.files.is_empty() { + return String::new(); + } + let mut out = String::new(); + out.push_str("The instructions may name a supporting file. skilld printed none of them.\n"); + if let SkillOrigin::Local { root } = &skill.origin { + out.push_str(&format!( + "Read one from {}, or use --file to print it here.\n", + sanitize(&root.display().to_string()) + )); + } else { + out.push_str("Use --file to read the ones you need.\n"); + } + out.push('\n'); + out.push_str(&paint( + &format!("Supporting files ({}):", skill.files.len()), + Role::Emphasis, + color, + )); + out.push('\n'); + for file in &skill.files { + out.push_str(&format!( + " {} {} bytes {}\n", + sanitize(&file.path), + grouped_number(file.size), + file.kind.as_str() + )); + if file.kind.is_readable() { + out.push_str(&format!( + " {}\n", + paint( + &shell_command( + &read_argv(&skill.origin, skill.revision.as_deref(), &file.path, false,), + platform + ), + Role::Brand, + color, + ) + )); + } else { + out.push_str(&format!( + " skilld will not print this {} file. Install the Skill to use it.\n", + file.kind.as_str() + )); + } + } + out.push('\n'); + out +} + +fn render_files( + skill: &str, + origin: &SkillOrigin, + source_status: &str, + revision: Option<&str>, + files: &[PulledFile], + color: bool, +) -> String { + let mut out = String::new(); + out.push_str(&format!( + "{}\n", + paint( + &format!( + "skilld read supporting files from the transient Skill {}.", + sanitize(skill) + ), + Role::Emphasis, + color, + ) + )); + out.push_str(&origin_field(origin, color)); + if let Some(revision) = revision { + out.push_str(&field("Revision", revision, color)); + } + out.push_str(&field("Source status", source_status, color)); + out.push_str(source_status_caution(source_status)); + out.push('\n'); + for file in files { + let path = sanitize(&file.path); + out.push_str(&field("File", &path, color)); + out.push_str(&field( + "Size", + &format!("{} bytes", grouped_number(file.size)), + color, + )); + out.push_str(&field("Kind", file.kind.as_str(), color)); + match &file.content { + FileContent::Text(text) => { + out.push('\n'); + out.push_str(&paint(&format!("--- {path} ---"), Role::Dim, color)); + out.push('\n'); + let text = safe_terminal_text(text); + out.push_str(&text); + if !text.ends_with('\n') { + out.push('\n'); + } + out.push_str(&paint(&format!("--- end of {path} ---"), Role::Dim, color)); + out.push('\n'); + } + FileContent::Withheld { reason } => { + out.push_str(&format!( + "skilld did not print this file, because {reason}.\n" + )); + out.push_str("Install the Skill to put this file on disk.\n"); + } + } + out.push('\n'); + } + out +} + +fn render_install_guidance(origin: &SkillOrigin, color: bool, platform: CommandPlatform) -> String { + let mut out = String::new(); + out.push_str(&paint("To keep this Skill:", Role::Emphasis, color)); + out.push('\n'); + if matches!(origin, SkillOrigin::Bundled) { + out.push_str(&format!( + " {}\n", + shell_command(&install_argv(origin, true), platform) + )); + out.push_str("This keeps the Skill for every project.\n"); + out.push_str( + "Ask the user before you install. An install writes files they did not request.\n", + ); + out.push('\n'); + out.push_str(&field("Find another Skill", "skilld search ", color)); + out.push_str(&field("List installed Skills", "skilld list", color)); + out.push_str(&field("Update installed Skills", "skilld update", color)); + return out; + } + out.push_str(&format!( + " {}\n", + shell_command(&install_argv(origin, false), platform) + )); + out.push_str(&format!( + " {}\n", + shell_command(&install_argv(origin, true), platform) + )); + out.push_str("The first writes the Skill into this project and records it in the lockfile.\n"); + out.push_str("The second keeps it for every project.\n"); + out.push_str( + "Ask the user before you install. An install writes files they did not request.\n", + ); + out.push('\n'); + out.push_str(&field("Find another Skill", "skilld search ", color)); + out.push_str(&field("List installed Skills", "skilld list", color)); + out.push_str(&field("Update installed Skills", "skilld update", color)); + out +} + +fn read_argv(origin: &SkillOrigin, revision: Option<&str>, path: &str, json: bool) -> Vec { + let mut argv = vec![ + "skilld".to_owned(), + "run".to_owned(), + source_argument(origin), + ]; + if matches!(origin, SkillOrigin::Remote { direct: true, .. }) { + argv.push("--direct".to_owned()); + } + if let Some(revision) = revision { + argv.push("--revision".to_owned()); + argv.push(revision.to_owned()); + } + argv.push(format!("--file={path}")); + if json { + argv.push("--json".to_owned()); + } + argv +} + +fn install_argv(origin: &SkillOrigin, global: bool) -> Vec { + let mut argv = vec![ + "skilld".to_owned(), + "install".to_owned(), + install_source_argument(origin), + ]; + if matches!(origin, SkillOrigin::Remote { direct: true, .. }) { + argv.push("--direct".to_owned()); + } + if global { + argv.push("--global".to_owned()); + } + argv +} + +fn source_argument(origin: &SkillOrigin) -> String { + match origin { + SkillOrigin::Bundled => "skilld".to_owned(), + SkillOrigin::Remote { exact_source, .. } => exact_source.clone(), + SkillOrigin::Local { root } => root.display().to_string(), + } +} + +fn install_source_argument(origin: &SkillOrigin) -> String { + match origin { + SkillOrigin::Bundled => "skilld".to_owned(), + SkillOrigin::Remote { exact_source, .. } => exact_source.clone(), + SkillOrigin::Local { root } => root.display().to_string(), + } +} + +pub(crate) fn shell_command(argv: &[String], platform: CommandPlatform) -> String { + argv.iter() + .map(|argument| shell_quote(argument, platform)) + .collect::>() + .join(" ") +} + +fn shell_quote(argument: &str, platform: CommandPlatform) -> String { + let portable = !argument.is_empty() + && argument.bytes().all(|byte| { + byte.is_ascii_alphanumeric() + || match platform { + CommandPlatform::Unix => b"_@%+=:,./-".contains(&byte), + CommandPlatform::WindowsPowerShell => b"_./:-".contains(&byte), + } + }); + if portable { + return argument.to_owned(); + } + match platform { + CommandPlatform::Unix => format!("'{}'", argument.replace('\'', "'\\''")), + CommandPlatform::WindowsPowerShell => format!("'{}'", argument.replace('\'', "''")), + } +} + +/// 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" + } + "unverified" => "skilld did not check this source. Read this Skill before you follow it.\n", + _ => "Read this Skill before you follow it.\n", + } +} + +fn field(label: &str, value: &str, color: bool) -> String { + format!("{}: {}\n", paint(label, Role::Dim, color), sanitize(value)) +} + +fn origin_field(origin: &SkillOrigin, color: bool) -> String { + match origin { + SkillOrigin::Bundled => field("Source", "skilld-maintained Skill", color), + SkillOrigin::Remote { source, .. } => field("Source", source, color), + SkillOrigin::Local { root } => field("Source", &root.display().to_string(), color), + } +} + +fn safe_terminal_text(value: &str) -> String { + value + .chars() + .filter(|character| !is_unsafe_terminal(*character) || matches!(character, '\n' | '\t')) + .collect() +} + +#[derive(Serialize)] +#[serde(tag = "_tag", rename_all = "lowercase")] +enum JsonOrigin { + Bundled { source: &'static str }, + Remote { source: String, direct: bool }, + 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::Local { root } => JsonOrigin::Local { + root: root.display().to_string(), + }, + } +} + +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] +struct JsonSupportingFile { + path: String, + kind: &'static str, + size: u64, + readable: bool, + #[serde(skip_serializing_if = "Option::is_none")] + read_argv: Option>, +} + +#[derive(Serialize)] +struct JsonInstallArgv { + #[serde(skip_serializing_if = "Option::is_none")] + project: Option>, + global: Vec, +} + +#[derive(Serialize)] +#[serde( + tag = "_tag", + rename_all = "lowercase", + rename_all_fields = "camelCase" +)] +enum JsonRunData { + Load { + name: String, + origin: JsonOrigin, + source_status: &'static str, + source_caution: &'static str, + revision: Option, + wrote_skill_files: bool, + instructions: String, + files: Vec, + install_argv: JsonInstallArgv, + }, + Files { + name: String, + origin: JsonOrigin, + source_status: &'static str, + source_caution: &'static str, + revision: Option, + wrote_skill_files: bool, + files: Vec, + }, +} + +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] +struct JsonPulledFile { + path: String, + kind: &'static str, + size: u64, + content: JsonFileContent, +} + +#[derive(Serialize)] +#[serde(tag = "_tag", rename_all = "lowercase")] +enum JsonFileContent { + Text { value: String }, + Withheld { reason: &'static str }, +} + +fn load_json(skill: &TransientSkill) -> JsonRunData { + JsonRunData::Load { + name: skill.name.clone(), + origin: origin_json(&skill.origin), + source_status: skill.source_status, + source_caution: source_status_caution(skill.source_status).trim_end(), + revision: skill.revision.clone(), + wrote_skill_files: false, + instructions: skill.instructions.clone(), + files: skill + .files + .iter() + .map(|file| JsonSupportingFile { + path: file.path.clone(), + kind: file.kind.as_str(), + size: file.size, + readable: file.kind.is_readable(), + read_argv: file + .kind + .is_readable() + .then(|| read_argv(&skill.origin, skill.revision.as_deref(), &file.path, true)), + }) + .collect(), + install_argv: JsonInstallArgv { + project: (!matches!(skill.origin, SkillOrigin::Bundled)) + .then(|| install_argv(&skill.origin, false)), + global: install_argv(&skill.origin, true), + }, + } +} + +fn files_json( + skill: &str, + origin: &SkillOrigin, + source_status: &'static str, + revision: Option<&str>, + files: &[PulledFile], +) -> JsonRunData { + JsonRunData::Files { + name: skill.to_owned(), + origin: origin_json(origin), + source_status, + source_caution: source_status_caution(source_status).trim_end(), + revision: revision.map(str::to_owned), + wrote_skill_files: false, + files: files + .iter() + .map(|file| JsonPulledFile { + path: file.path.clone(), + kind: file.kind.as_str(), + size: file.size, + content: match &file.content { + FileContent::Text(text) => JsonFileContent::Text { + value: text.clone(), + }, + FileContent::Withheld { reason } => JsonFileContent::Withheld { reason }, + }, + }) + .collect(), + } +} diff --git a/crates/skilld-command/src/remote.rs b/crates/skilld-command/src/remote.rs index 3e0b11e1..3d4518c7 100644 --- a/crates/skilld-command/src/remote.rs +++ b/crates/skilld-command/src/remote.rs @@ -16,6 +16,7 @@ use skilld_core::{ parse_search_response, prepare_unverified_files, verify_artifact, verify_attestation, verify_trusted_root, }; +use skilld_ui::text::is_unsafe_terminal; use url::Url; const JSON_LIMIT: usize = 8 * 1024 * 1024; @@ -1090,6 +1091,15 @@ impl SkilldRemote { if !valid_sha(&commit.sha) || !valid_sha(&commit.commit.tree.sha) { return Err(invalid_github()); } + if matches!( + &source.r#ref, + Some(SourceRef::Commit { value }) if value != &commit.sha + ) { + return Err(RemoteError::new( + "SOURCE_MISMATCH", + "GitHub resolved a different commit than requested", + )); + } Ok((repository_url, skill_path.clone(), commit)) } @@ -1494,6 +1504,14 @@ impl RemoteProvider for SkilldRemote { expected_commit: &CommitSha, direct: bool, ) -> Result { + if let Some(SourceRef::Commit { value }) = &selector.source().r#ref + && value != expected_commit.as_str() + { + return Err(RemoteError::new( + "SOURCE_MISMATCH", + "the source commit does not match the expected commit", + )); + } let mut source = selector.source().clone(); source.r#ref = Some(SourceRef::Commit { value: expected_commit.as_str().to_owned(), @@ -1921,11 +1939,6 @@ fn sanitize_line(value: &str, maximum: usize, fallback: &str) -> String { value.chars().take(maximum).collect() } -fn is_unsafe_terminal(character: char) -> bool { - let code = u32::from(character); - character.is_control() || matches!(code, 0x200E | 0x200F | 0x202A..=0x202E | 0x2066..=0x2069) -} - fn valid_timestamp(value: &str) -> bool { !value.is_empty() && value.len() <= 64 diff --git a/crates/skilld-command/src/run.rs b/crates/skilld-command/src/run.rs new file mode 100644 index 00000000..9166d4c1 --- /dev/null +++ b/crates/skilld-command/src/run.rs @@ -0,0 +1,384 @@ +//! Transient Skill loads. +//! +//! `skilld run` hands the calling Agent a Skill now. A remote run retains no +//! Skill files and creates no lockfile entry, Agent target, or project file. +//! +//! The initial load names supporting files without printing their content. +//! The Agent reads only the files that the instructions name. + +use std::collections::BTreeSet; +use std::fs::{self, File}; +use std::io::Read; +use std::path::{Path, PathBuf}; + +use skilld_core::PreparedFile; +use skilld_ui::text::is_unsafe_terminal; + +use crate::CommandError; + +/// The instructions file every Skill carries. +pub const INSTRUCTIONS_FILE: &str = "SKILL.md"; + +const MAX_LOCAL_DEPTH: usize = 8; +const MAX_LOCAL_FILES: usize = 512; +const MAX_LOCAL_BYTES: u64 = 64 * 1024 * 1024; + +/// Where a transient Skill came from, and what that means for its files. +/// +/// A local Skill already sits on the user's disk, so its files carry a path. +/// Bundled and remote Skills have no path to give. +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum SkillOrigin { + Bundled, + Local { + root: PathBuf, + }, + Remote { + source: String, + exact_source: String, + direct: bool, + }, +} + +/// How skilld can hand one supporting file to an Agent. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum FileKind { + /// UTF-8 text. skilld prints it on request. + Text, + /// Marked executable by its author. skilld never prints it. + Executable, + /// Not valid UTF-8. skilld never prints it. + Binary, +} + +impl FileKind { + pub const fn as_str(self) -> &'static str { + match self { + Self::Text => "text", + Self::Executable => "executable", + Self::Binary => "binary", + } + } + + /// Whether skilld will print this file's bytes. + pub const fn is_readable(self) -> bool { + matches!(self, Self::Text) + } +} + +/// One supporting file, named but not delivered. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct SupportingFile { + pub path: String, + pub kind: FileKind, + pub size: u64, +} + +/// One transient Skill: loaded for this session, recorded nowhere. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct TransientSkill { + pub name: String, + pub instructions: String, + pub origin: SkillOrigin, + /// `verified`, `local`, or `unverified`. + pub source_status: &'static str, + /// The exact remote Git commit. Local and bundled Skills have no revision. + pub revision: Option, + pub files: Vec, +} + +/// One supporting file the Agent asked for. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct PulledFile { + pub path: String, + pub kind: FileKind, + pub size: u64, + pub content: FileContent, +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum FileContent { + Text(String), + /// skilld holds the bytes but will not print them. + Withheld { + reason: &'static str, + }, +} + +/// What one `skilld run` invocation produced. +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum RunOutcome { + Load(Box), + Files { + skill: String, + origin: SkillOrigin, + source_status: &'static str, + revision: Option, + files: Vec, + }, +} + +pub(crate) fn reject_duplicate_files(wanted: &[String]) -> Result<(), CommandError> { + if wanted + .iter() + .any(|path| path.chars().any(is_unsafe_terminal)) + { + return Err(CommandError::input( + "--file paths cannot contain terminal formatting characters", + )); + } + let mut unique = BTreeSet::new(); + if wanted.iter().any(|path| !unique.insert(path)) { + return Err(CommandError::input( + "each --file path must appear only once", + )); + } + Ok(()) +} + +/// Read the SKILL.md text out of a file set. +pub fn read_instructions(files: &[PreparedFile]) -> Result { + let file = instructions_file(files)?; + decode(&file.bytes).ok_or_else(|| { + CommandError::operation("INVALID_ARTIFACT", "the SKILL.md file is not valid UTF-8") + }) +} + +fn instructions_file(files: &[PreparedFile]) -> Result<&PreparedFile, CommandError> { + files + .iter() + .find(|file| file.path == INSTRUCTIONS_FILE) + .ok_or_else(|| { + CommandError::operation("INVALID_ARTIFACT", "the Skill has no SKILL.md file") + }) +} + +/// Describe every supporting file a Skill carries, without delivering one. +pub fn supporting_files(files: &[PreparedFile]) -> Vec { + files + .iter() + .filter(|file| file.path != INSTRUCTIONS_FILE) + .map(|file| { + let kind = classify(file); + SupportingFile { + path: file.path.clone(), + kind, + size: file.bytes.len() as u64, + } + }) + .collect() +} + +/// Hand over the supporting files the Agent named. +/// +/// An unknown path fails the whole run. A path skilld will not print comes back +/// withheld, so the Agent learns the file exists and learns why it did not get it. +pub fn pull_files( + skill: &str, + files: &[PreparedFile], + wanted: &[String], +) -> Result, CommandError> { + wanted + .iter() + .map(|path| { + if path == INSTRUCTIONS_FILE { + return Err(CommandError::input( + "SKILL.md arrives with every run. Drop --file SKILL.md.", + )); + } + let file = files + .iter() + .find(|file| &file.path == path) + .ok_or_else(|| { + CommandError::operation( + "SOURCE_NOT_FOUND", + format!("the Skill {skill} carries no file at {path}"), + ) + })?; + let kind = classify(file); + Ok(PulledFile { + path: file.path.clone(), + kind, + size: file.bytes.len() as u64, + content: match kind { + FileKind::Text => decode(&file.bytes).map_or( + FileContent::Withheld { + reason: "the file is not valid UTF-8", + }, + FileContent::Text, + ), + FileKind::Executable => FileContent::Withheld { + reason: "the Skill marks this file executable", + }, + FileKind::Binary => FileContent::Withheld { + reason: "the file is not valid UTF-8", + }, + }, + }) + }) + .collect() +} + +/// Read a Skill that already sits on disk. +/// +/// A local Skill needs no delivery decision. The user owns these files already. +pub fn read_local(path: &Path) -> Result<(String, Vec), CommandError> { + let path_text = path + .to_str() + .ok_or_else(|| invalid_local("Skill paths must use UTF-8"))?; + reject_local_path_controls(path_text)?; + crate::local_store::validate_skill_files(path).map_err(CommandError::store)?; + let mut inventory = Vec::new(); + let mut total = 0; + collect_local_metadata(path, Path::new(""), 0, &mut inventory, &mut total)?; + crate::local_store::validate_skill_source(path).map_err(CommandError::store)?; + let name = skilld_core::SkillName::from_source(path) + .map_err(CommandError::domain)? + .to_string(); + inventory.sort_by(|left, right| left.relative.cmp(&right.relative)); + let files = inventory + .into_iter() + .map(read_local_file) + .collect::, _>>()?; + Ok((name, files)) +} + +struct LocalFile { + path: PathBuf, + relative: String, + mode: u32, + size: u64, +} + +fn collect_local_metadata( + root: &Path, + relative: &Path, + depth: usize, + files: &mut Vec, + total: &mut u64, +) -> Result<(), CommandError> { + if depth > MAX_LOCAL_DEPTH { + return Err(too_large("the local Skill exceeds its depth limit")); + } + let entries = fs::read_dir(root.join(relative)).map_err(|error| { + CommandError::operation( + "SOURCE_NOT_FOUND", + format!("cannot read the Skill directory: {error}"), + ) + })?; + for entry in entries { + let entry = entry.map_err(|error| { + CommandError::filesystem(format!("cannot read a Skill file: {error}")) + })?; + let name = entry.file_name(); + let name = name + .to_str() + .ok_or_else(|| invalid_local("Skill paths must use UTF-8"))?; + reject_local_path_controls(name)?; + let child = relative.join(name); + let file_type = entry.file_type().map_err(|error| { + CommandError::filesystem(format!("cannot read a Skill file: {error}")) + })?; + if file_type.is_symlink() { + return Err(invalid_local("local Skill sources cannot contain links")); + } + let metadata = entry.metadata().map_err(|error| { + CommandError::filesystem(format!("cannot read a Skill file: {error}")) + })?; + if metadata.is_dir() { + collect_local_metadata(root, &child, depth + 1, files, total)?; + continue; + } + if !metadata.is_file() { + return Err(invalid_local( + "local Skill sources can contain only files and directories", + )); + } + if files.len() >= MAX_LOCAL_FILES { + return Err(too_large("the local Skill exceeds its file limit")); + } + *total = total + .checked_add(metadata.len()) + .ok_or_else(|| too_large("the local Skill exceeds its content limit"))?; + if *total > MAX_LOCAL_BYTES { + return Err(too_large("the local Skill exceeds its content limit")); + } + files.push(LocalFile { + path: entry.path(), + relative: child + .to_str() + .ok_or_else(|| invalid_local("Skill paths must use UTF-8"))? + .replace('\\', "/"), + mode: local_mode(&metadata), + size: metadata.len(), + }); + } + Ok(()) +} + +fn read_local_file(file: LocalFile) -> Result { + let input = File::open(&file.path) + .map_err(|error| CommandError::filesystem(format!("cannot read a Skill file: {error}")))?; + let mut bytes = Vec::new(); + input + .take(file.size.saturating_add(1)) + .read_to_end(&mut bytes) + .map_err(|error| CommandError::filesystem(format!("cannot read a Skill file: {error}")))?; + if bytes.len() as u64 != file.size { + return Err(invalid_local( + "a local Skill file changed while skilld read it", + )); + } + Ok(PreparedFile { + path: file.relative, + mode: file.mode, + bytes, + }) +} + +fn invalid_local(message: &'static str) -> CommandError { + CommandError::operation("INVALID_SOURCE", message) +} + +fn reject_local_path_controls(value: &str) -> Result<(), CommandError> { + if value.chars().any(is_unsafe_terminal) { + return Err(invalid_local( + "local Skill paths cannot contain terminal formatting characters", + )); + } + Ok(()) +} + +fn too_large(message: &'static str) -> CommandError { + CommandError::operation("SKILL_TOO_LARGE", message) +} + +#[cfg(unix)] +fn local_mode(metadata: &fs::Metadata) -> u32 { + use std::os::unix::fs::PermissionsExt; + if metadata.permissions().mode() & 0o111 == 0 { + 0o644 + } else { + 0o755 + } +} + +#[cfg(not(unix))] +fn local_mode(_metadata: &fs::Metadata) -> u32 { + 0o644 +} + +fn classify(file: &PreparedFile) -> FileKind { + if file.mode & 0o111 != 0 { + return FileKind::Executable; + } + if std::str::from_utf8(&file.bytes).is_ok() { + FileKind::Text + } else { + FileKind::Binary + } +} + +fn decode(bytes: &[u8]) -> Option { + String::from_utf8(bytes.to_vec()).ok() +} diff --git a/crates/skilld-command/tests/outdated.rs b/crates/skilld-command/tests/outdated.rs index d4a0aaa6..70dd952e 100644 --- a/crates/skilld-command/tests/outdated.rs +++ b/crates/skilld-command/tests/outdated.rs @@ -4,11 +4,12 @@ use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; use std::time::Duration; +use serde_json::json; use sha2::{Digest, Sha256}; use skilld_command::{ - Host, LocalHost, PreparedRemoteSkill, RemoteComparisonAccess, RemoteComparisonOutcome, - RemoteComparisonRelation, RemoteLatestCommit, RemoteProvider, RemoteSourceState, - RemoteUpdateComparison, RemoteUpdateResult, run, + CommandPlatform, Host, LocalHost, OutputContext, PreparedRemoteSkill, RemoteComparisonAccess, + RemoteComparisonOutcome, RemoteComparisonRelation, RemoteLatestCommit, RemoteProvider, + RemoteSourceState, RemoteUpdateComparison, RemoteUpdateResult, run, run_with_output, }; use skilld_core::{ AgentTargetId, CommitAuthor, CommitHistory, CommitSha, CommitSummary, InstallMode, @@ -21,8 +22,10 @@ struct Provider { content: Mutex>, stale: Mutex, fail_state: Mutex, + state_failure_message: Mutex, search_results: Mutex>, fail_search: Mutex, + search_failure_message: Mutex, search_calls: AtomicUsize, search_in_flight: AtomicUsize, search_max_in_flight: Mutex, @@ -35,8 +38,10 @@ impl Provider { content: Mutex::new(content.as_bytes().to_vec()), stale: Mutex::new(false), fail_state: Mutex::new(false), + state_failure_message: Mutex::new("the remote service returned HTTP 503".to_owned()), search_results: Mutex::new(vec![]), fail_search: Mutex::new(false), + search_failure_message: Mutex::new("Skill search returned invalid JSON".to_owned()), search_calls: std::sync::atomic::AtomicUsize::new(0), search_in_flight: std::sync::atomic::AtomicUsize::new(0), search_max_in_flight: Mutex::new(0), @@ -97,7 +102,7 @@ impl RemoteProvider for Provider { if *self.fail_search.lock().unwrap() { return Err(RemoteError::new( "INVALID_RESPONSE", - "Skill search returned invalid JSON", + self.search_failure_message.lock().unwrap().clone(), )); } let items = self.search_results.lock().unwrap().clone(); @@ -122,12 +127,16 @@ impl RemoteProvider for Provider { bytes, }; let digest = installed_digest(&file); + let skill_path = match &selector.source().selector { + SourceSelector::Path { path } => path.clone(), + SourceSelector::NamedSkill { name } => format!("skills/{name}"), + }; Ok(PreparedRemoteSkill { files: vec![file], locked_source: LockedSource::Remote { source: selector.canonical(), commit_sha: "0123456789abcdef0123456789abcdef01234567".to_owned(), - skill_path: "skills/example".to_owned(), + skill_path, }, source_status: if direct { SourceStatus::Unverified { @@ -154,7 +163,7 @@ impl RemoteProvider for Provider { if *self.fail_state.lock().unwrap() { return Err(RemoteError::new( "SERVICE_UNAVAILABLE", - "the remote service returned HTTP 503", + self.state_failure_message.lock().unwrap().clone(), )); } Ok(if *self.stale.lock().unwrap() { @@ -253,6 +262,24 @@ fn install_global(host: &LocalHost, selector: &str) { .unwrap(); } +fn install_direct_project(host: &LocalHost, selector: &str) { + host.install_request(InstallRequest { + operation: InstallOperation::Install(InstallSource::DirectRemote(selector.to_owned())), + scope: InstallScope::Project, + targets: vec![AgentTargetId::Codex], + mode: Some(InstallMode::Copy), + }) + .unwrap(); +} + +fn replace_project_locked_source(project: &Path, source: &str) { + let path = project.join(".skills/skilld-lock.yaml"); + let mut document = + serde_json::from_slice::(&fs::read(&path).unwrap()).unwrap(); + document["skills"]["example"]["source"]["source"] = json!(source); + fs::write(path, serde_json::to_vec_pretty(&document).unwrap()).unwrap(); +} + fn unmanaged_skill(home: &Path, agent_dir: &str, name: &str) { let directory = home.join(agent_dir).join("skills").join(name); fs::create_dir_all(&directory).unwrap(); @@ -297,6 +324,165 @@ fn outdated_reports_current_and_stale_project_skills() { ); } +#[test] +fn view_and_outdated_reject_terminal_formatting_from_a_remote_lock_source() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let host = LocalHost::new(project.clone(), temporary.path().join("data")).with_remote_provider( + Arc::new(Provider::new( + "---\nname: example\ndescription: direct\n---\n", + )), + ); + install_direct_project(&host, "github:skilld-dev/skills/skills/example"); + replace_project_locked_source( + &project, + "github:skilld-dev/skills/skills/example\u{1b}]8;;https://evil.invalid\u{1b}\\\u{202e}", + ); + + for context in [ + OutputContext::Plain { + platform: CommandPlatform::Unix, + }, + OutputContext::HumanTerminal { + width: 80, + color: false, + platform: CommandPlatform::Unix, + }, + ] { + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let view = run_with_output( + ["skilld", "view", "example"], + &host, + context, + &mut stdout, + &mut stderr, + ); + let stderr = String::from_utf8(stderr).unwrap(); + + assert_eq!(view.exit_code, 1); + assert!(stdout.is_empty()); + assert!(stderr.contains("INVALID_LOCKFILE")); + assert!(!stderr.contains('\u{1b}')); + assert!(!stderr.contains('\u{202e}')); + + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let outdated = run_with_output( + ["skilld", "outdated"], + &host, + context, + &mut stdout, + &mut stderr, + ); + let stdout = String::from_utf8(stdout).unwrap(); + + assert_eq!(outdated.exit_code, 0); + assert!(stderr.is_empty()); + assert!(stdout.contains("Skill store unavailable in project scope")); + assert!(!stdout.contains('\u{1b}')); + assert!(!stdout.contains('\u{202e}')); + } +} + +#[test] +fn view_and_outdated_preserve_valid_metacharacters_as_quoted_data() { + 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(Arc::new( + Provider::new("---\nname: example\ndescription: direct\n---\n"), + )); + let source = r#"github:skilld-dev/skills/skills/o'hare$("quoted")"#; + install_direct_project(&host, source); + + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let plain_view = run_with_output( + ["skilld", "view", "example", "--plain"], + &host, + OutputContext::Plain { + platform: CommandPlatform::Unix, + }, + &mut stdout, + &mut stderr, + ); + + assert_eq!(plain_view.exit_code, 0); + assert!(stderr.is_empty()); + assert!( + String::from_utf8(stdout) + .unwrap() + .contains(&format!("Source: {source}\n")) + ); + + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let view = run_with_output( + ["skilld", "view", "example"], + &host, + OutputContext::HumanTerminal { + width: 80, + color: true, + platform: CommandPlatform::Unix, + }, + &mut stdout, + &mut stderr, + ); + let stdout = String::from_utf8(stdout).unwrap(); + + 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}\\")); + + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let plain = run_with_output( + ["skilld", "outdated", "--plain"], + &host, + OutputContext::Plain { + platform: CommandPlatform::Unix, + }, + &mut stdout, + &mut stderr, + ); + + assert_eq!(plain.exit_code, 0); + assert!(stderr.is_empty()); + assert_eq!( + String::from_utf8(stdout).unwrap(), + concat!( + "Unverified Skill example. Run skilld install ", + "'github:skilld-dev/skills/skills/o'\\''hare$(\"quoted\")' ", + "--direct --agent codex to update it.\n" + ) + ); + + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let human = run_with_output( + ["skilld", "outdated"], + &host, + OutputContext::HumanTerminal { + width: 80, + color: false, + platform: CommandPlatform::WindowsPowerShell, + }, + &mut stdout, + &mut stderr, + ); + let stdout = String::from_utf8(stdout).unwrap(); + + assert_eq!(human.exit_code, 0); + assert!(stderr.is_empty()); + assert!(stdout.contains( + r#"skilld install 'github:skilld-dev/skills/skills/o''hare$("quoted")' --direct --agent codex"# + )); +} + #[test] fn outdated_all_reports_a_stale_global_skill_with_the_global_update() { let temporary = tempfile::tempdir().unwrap(); @@ -411,6 +597,58 @@ fn outdated_all_surfaces_a_search_failure_and_keeps_scanning() { ); } +#[test] +fn outdated_search_failures_are_single_line_and_terminal_safe() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let provider = Arc::new(Provider::new("---\nname: example\n---\n")); + *provider.fail_search.lock().unwrap() = true; + *provider.search_failure_message.lock().unwrap() = + "request\u{1b}[31m\u{0085}\u{202e}\rforged\nline".to_owned(); + unmanaged_skill(temporary.path(), ".agents", "search-failure"); + let host = + LocalHost::new(project, temporary.path().join("data")).with_remote_provider(provider); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let plain = run( + ["skilld", "outdated", "--all"], + &host, + &mut stdout, + &mut stderr, + ); + + assert_eq!(plain.exit_code, 0); + assert!(stderr.is_empty()); + assert_eq!( + String::from_utf8(stdout.clone()).unwrap(), + "Skill search unavailable for 1 Skill (search-failure (codex)): request [31m forged line.\n" + ); + stdout.clear(); + let human = run_with_output( + ["skilld", "outdated", "--all"], + &host, + OutputContext::HumanTerminal { + width: 80, + color: false, + platform: CommandPlatform::Unix, + }, + &mut stdout, + &mut stderr, + ); + + assert_eq!(human.exit_code, 0); + assert!(stderr.is_empty()); + assert_eq!( + String::from_utf8(stdout).unwrap(), + concat!( + "⚠ Skill search unavailable: request [31m forged line\n", + " search-failure codex\n" + ) + ); +} + #[test] fn outdated_all_reports_a_managed_skill_once() { let temporary = tempfile::tempdir().unwrap(); @@ -516,6 +754,55 @@ fn outdated_survives_a_source_state_failure() { ); } +#[test] +fn outdated_source_state_failures_are_single_line_and_terminal_safe() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + fs::create_dir_all(&project).unwrap(); + let provider = Arc::new(Provider::new( + "---\nname: example\ndescription: first\n---\n", + )); + let host = LocalHost::new(project, temporary.path().join("data")) + .with_remote_provider(provider.clone()); + install_project(&host, "skilld:skilld-dev/skills/example"); + *provider.fail_state.lock().unwrap() = true; + *provider.state_failure_message.lock().unwrap() = + "request\u{1b}[31m\u{0085}\u{202e}\rforged\nline".to_owned(); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let plain = run(["skilld", "outdated"], &host, &mut stdout, &mut stderr); + + assert_eq!(plain.exit_code, 0); + assert!(stderr.is_empty()); + assert_eq!( + String::from_utf8(stdout.clone()).unwrap(), + "Source state unavailable for Skill example: request [31m forged line.\n" + ); + stdout.clear(); + let human = run_with_output( + ["skilld", "outdated"], + &host, + OutputContext::HumanTerminal { + width: 80, + color: false, + platform: CommandPlatform::Unix, + }, + &mut stdout, + &mut stderr, + ); + + assert_eq!(human.exit_code, 0); + assert!(stderr.is_empty()); + assert_eq!( + String::from_utf8(stdout).unwrap(), + concat!( + "✗ example source unavailable\n", + " error request [31m forged line\n" + ) + ); +} + #[test] fn outdated_all_survives_a_corrupt_global_store() { let temporary = tempfile::tempdir().unwrap(); diff --git a/crates/skilld-command/tests/output.rs b/crates/skilld-command/tests/output.rs index b7a5048f..354a2570 100644 --- a/crates/skilld-command/tests/output.rs +++ b/crates/skilld-command/tests/output.rs @@ -1,7 +1,7 @@ -use skilld_command::{CommandError, Host, OutputContext, run_with_output}; +use skilld_command::{CommandError, CommandPlatform, Host, OutputContext, run_with_output}; use skilld_core::{ - InstallScope, InstallSource, SearchResponse, SearchResult, SourceProvider, SourceRequest, - SourceSelector, + InstallScope, InstallSource, RemoteError, SearchResponse, SearchResult, SourceProvider, + SourceRequest, SourceSelector, }; use std::io::{self, Write}; use unicode_width::UnicodeWidthStr; @@ -70,15 +70,38 @@ fn run(args: &[&str], context: OutputContext) -> (u8, String, String) { ) } +const PLAIN: OutputContext = OutputContext::Plain { + platform: CommandPlatform::Unix, +}; + +fn auto( + stdout_is_terminal: bool, + active_agent: bool, + ci: bool, + no_color: bool, + term_is_dumb: bool, + width: u16, +) -> OutputContext { + OutputContext::auto( + stdout_is_terminal, + active_agent, + ci, + no_color, + term_is_dumb, + width, + CommandPlatform::Unix, + ) +} + #[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), + auto(true, true, false, false, false, 80), ); let global_form = run( &["skilld", "--json", "search", "grill"], - OutputContext::auto(true, true, false, false, false, 80), + auto(true, true, false, false, false, 80), ); assert_eq!(exit, 0); @@ -108,12 +131,10 @@ fn json_search_returns_one_versioned_document() { #[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); + let root_help = run(&["skilld", "--json", "--help"], PLAIN); + let search_help = run(&["skilld", "search", "--json", "--help"], PLAIN); + let run_help = run(&["skilld", "run", "--json", "--help"], PLAIN); + let version = run(&["skilld", "--json", "--version"], PLAIN); assert_eq!(root_help.0, 0); assert!(root_help.2.is_empty()); @@ -123,12 +144,48 @@ fn json_help_and_version_return_versioned_documents() { let search = serde_json::from_str::(&search_help.1).unwrap(); assert_eq!(search["command"], "help"); assert_eq!(search["data"]["path"], "skilld search"); + let run = serde_json::from_str::(&run_help.1).unwrap(); + assert_eq!(run["command"], "help"); + assert_eq!(run["data"]["path"], "skilld run"); 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 help_explains_primary_flow_remote_file_revisions_and_direct_delivery() { + let root = run(&["skilld", "--help"], PLAIN); + let install = run(&["skilld", "install", "--help"], PLAIN); + let run_help = run(&["skilld", "run", "--help"], PLAIN); + + assert_eq!(root.0, 0); + assert!(root.2.is_empty()); + assert!( + root.1 + .contains("Search, run, install, and keep Skills current") + ); + assert_eq!(install.0, 0); + assert!(install.2.is_empty()); + assert!( + install + .1 + .contains("Install a hosted Artifact from an explicit GitHub selector.") + ); + assert!( + install + .1 + .contains("Add --direct to fetch a public GitHub Repository instead.") + ); + assert_eq!(run_help.0, 0); + assert!(run_help.2.is_empty()); + assert!( + run_help + .1 + .contains("Remote file reads also require the returned --revision.") + ); +} + #[test] fn non_terminal_and_ci_output_are_stable_plain_records() { let expected = concat!( @@ -138,11 +195,11 @@ fn non_terminal_and_ci_output_are_stable_plain_records() { let non_terminal = run( &["skilld", "search", "grill"], - OutputContext::auto(false, false, false, false, false, 80), + auto(false, false, false, false, false, 80), ); let ci_with_tty = run( &["skilld", "search", "grill"], - OutputContext::auto(true, false, true, false, false, 120), + auto(true, false, true, false, false, 120), ); assert_eq!(non_terminal, (0, expected.to_owned(), String::new())); @@ -153,7 +210,7 @@ fn non_terminal_and_ci_output_are_stable_plain_records() { 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), + auto(true, false, false, false, false, 120), ); assert_eq!(agent_with_tty.0, 0); @@ -166,7 +223,7 @@ fn human_terminal_is_formatted_without_an_explicit_machine_flag() { 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), + auto(true, true, false, false, false, 120), ); assert_eq!( @@ -187,7 +244,7 @@ fn active_agent_terminal_is_plain_without_an_explicit_machine_flag() { fn explicit_plain_overrides_a_human_terminal() { let (_, stdout, stderr) = run( &["skilld", "search", "grill", "--plain"], - OutputContext::auto(true, false, false, false, false, 120), + auto(true, false, false, false, false, 120), ); assert!(stderr.is_empty()); @@ -203,7 +260,7 @@ fn explicit_plain_overrides_a_human_terminal() { #[test] fn plain_search_escapes_record_delimiters() { let mut response = response(); - response.items[0].description = Some("first\nsecond\tvalue\u{1b}".to_owned()); + response.items[0].description = Some("first\nsecond\tvalue\u{1b}\u{202e}".to_owned()); let mut stdout = Vec::new(); let mut stderr = Vec::new(); @@ -212,7 +269,7 @@ fn plain_search_escapes_record_delimiters() { &SearchHost { response: Ok(response), }, - OutputContext::auto(true, false, false, false, false, 80), + auto(true, false, false, false, false, 80), &mut stdout, &mut stderr, ); @@ -221,7 +278,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}\n" + "grill-me\tskilld:mattpocock/skills/grill-me\t227068\tfirst\\nsecond\\tvalue\\u{001B}\\u{202E}\n" ); } @@ -229,7 +286,7 @@ fn plain_search_escapes_record_delimiters() { fn human_search_is_polished_and_respects_terminal_width() { let (_, stdout, stderr) = run( &["skilld", "search", "grill"], - OutputContext::auto(true, false, false, true, false, 40), + auto(true, false, false, true, false, 40), ); assert!(stderr.is_empty()); @@ -237,15 +294,51 @@ fn human_search_is_polished_and_respects_terminal_width() { assert!(stdout.contains("1 of 14 Skills")); assert!(stdout.contains("227,068 stars")); assert!(stdout.contains("skilld:mattpocock/skills/grill-me")); - assert!(stdout.contains("skilld install")); + assert!(stdout.contains("skilld run")); + assert!(stdout.contains("skilld:mattpocock/skills/grill-me")); + assert!(!stdout.contains("skilld install")); assert!( stdout .lines() + .filter(|line| !line.trim_start().starts_with("skilld run ")) .all(|line| UnicodeWidthStr::width(line) <= 40) ); assert!(!stdout.contains('\u{1b}')); } +#[test] +fn human_search_keeps_the_run_command_on_one_line() { + let mut response = response(); + let name = "a-very-long-skill-name".to_owned(); + response.items[0].name.clone_from(&name); + response.items[0].source.selector = SourceSelector::NamedSkill { name }; + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + let result = run_with_output( + ["skilld", "search", "grill"], + &SearchHost { + response: Ok(response), + }, + OutputContext::HumanTerminal { + width: 20, + color: false, + platform: CommandPlatform::Unix, + }, + &mut stdout, + &mut stderr, + ); + let stdout = String::from_utf8(stdout).unwrap(); + + assert_eq!(result.exit_code, 0); + assert!(stderr.is_empty()); + assert!( + stdout + .lines() + .any(|line| line == " skilld run skilld:mattpocock/skills/a-very-long-skill-name") + ); +} + #[test] fn human_empty_search_names_the_query_and_suggests_a_next_step() { let mut response = response(); @@ -259,7 +352,7 @@ fn human_empty_search_names_the_query_and_suggests_a_next_step() { &SearchHost { response: Ok(response), }, - OutputContext::auto(true, false, false, true, false, 40), + auto(true, false, false, true, false, 40), &mut stdout, &mut stderr, ); @@ -272,11 +365,11 @@ fn human_empty_search_names_the_query_and_suggests_a_next_step() { } #[test] -fn human_search_uses_display_cells_and_sanitizes_terminal_controls() { +fn human_search_uses_display_cells_and_sanitizes_terminal_formatting() { 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()); + Some("\u{6f22}\u{5b57}\u{1f642} cafe\u{301} \u{1b}[31mred\u{202e}forged".to_owned()); let mut stdout = Vec::new(); let mut stderr = Vec::new(); @@ -285,7 +378,7 @@ fn human_search_uses_display_cells_and_sanitizes_terminal_controls() { &SearchHost { response: Ok(response), }, - OutputContext::auto(true, false, false, true, false, 20), + auto(true, false, false, true, false, 20), &mut stdout, &mut stderr, ); @@ -297,9 +390,11 @@ fn human_search_uses_display_cells_and_sanitizes_terminal_controls() { assert!(stdout.contains("\u{6f22}\u{5b57}\u{1f642}")); assert!(stdout.contains("cafe\u{301}")); assert!(!stdout.contains('\u{1b}')); + assert!(!stdout.contains('\u{202e}')); assert!( stdout .lines() + .filter(|line| !line.trim_start().starts_with("skilld run ")) .all(|line| UnicodeWidthStr::width(line) <= 20) ); } @@ -314,7 +409,7 @@ fn broken_pipe_is_a_successful_search_exit() { &SearchHost { response: Ok(response()), }, - OutputContext::Plain, + PLAIN, &mut stdout, &mut stderr, ); @@ -327,15 +422,15 @@ fn broken_pipe_is_a_successful_search_exit() { fn color_capabilities_change_ansi_only() { let (_, colored, _) = run( &["skilld", "search", "grill"], - OutputContext::auto(true, false, false, false, false, 100), + auto(true, false, false, false, false, 100), ); let (_, no_color, _) = run( &["skilld", "search", "grill"], - OutputContext::auto(true, false, false, true, false, 100), + auto(true, false, false, true, false, 100), ); let (_, dumb_terminal, _) = run( &["skilld", "search", "grill"], - OutputContext::auto(true, false, false, false, true, 100), + auto(true, false, false, false, true, 100), ); assert!(colored.contains('\u{1b}')); @@ -347,7 +442,7 @@ fn color_capabilities_change_ansi_only() { 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), + auto(false, false, false, false, false, 80), ); assert_eq!(exit, 2); @@ -357,14 +452,8 @@ fn conflicting_output_flags_fail_before_search() { #[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, - ); + let after = run(&["skilld", "search", "grill", "--json", "--unknown"], PLAIN); + let before = run(&["skilld", "--json", "search", "grill", "--unknown"], PLAIN); assert_eq!(after.0, 2); assert!(after.1.is_empty()); @@ -375,9 +464,71 @@ fn json_search_parse_errors_are_tagged_usage_errors() { assert_eq!(before, after); } +#[test] +fn human_parse_errors_keep_clap_guidance_without_untrusted_terminal_formatting() { + let hostile = "--unknown\u{1b}[31m\u{0085}\u{202e}\rforged\nline"; + let (exit, stdout, stderr) = run( + &["skilld", "search", "grill", hostile], + OutputContext::HumanTerminal { + width: 80, + color: false, + platform: CommandPlatform::Unix, + }, + ); + + assert_eq!(exit, 2); + assert!(stdout.is_empty()); + assert!(stderr.contains("unexpected argument")); + assert!(stderr.contains("Usage:")); + assert!(stderr.contains("\\u{001B}")); + assert!(stderr.contains("\\u{0085}")); + assert!(stderr.contains("\\u{202E}")); + assert!(stderr.contains("\\rforged\\nline")); + assert!(!stderr.contains('\u{1b}')); + assert!(!stderr.contains('\u{0085}')); + assert!(!stderr.contains('\u{202e}')); + assert!(!stderr.contains('\r')); +} + +#[test] +fn plain_parse_errors_escape_record_delimiters_and_terminal_formatting() { + let hostile = "--unknown\u{1b}[31m\u{0085}\u{202e}\rforged\nline"; + let (exit, stdout, stderr) = run(&["skilld", "search", "grill", hostile, "--plain"], PLAIN); + + assert_eq!(exit, 2); + assert!(stdout.is_empty()); + assert_eq!(stderr.lines().count(), 1); + assert!(stderr.contains("INVALID_ARGUMENT:")); + assert!(stderr.contains("\\u{0085}")); + assert!(stderr.contains("\\u{202E}")); + assert!(stderr.contains("\\rforged\\nline")); + assert!(!stderr.contains('\u{1b}')); + assert!(!stderr.contains('\u{0085}')); + assert!(!stderr.contains('\u{202e}')); + assert!(!stderr.contains('\r')); +} + +#[test] +fn json_parse_errors_keep_the_typed_error_contract_for_untrusted_arguments() { + let hostile = "--unknown\u{1b}[31m\u{0085}\u{202e}\rforged\nline"; + let (exit, stdout, stderr) = run(&["skilld", "search", "grill", hostile, "--json"], PLAIN); + + assert_eq!(exit, 2); + assert!(stdout.is_empty()); + assert_eq!(stderr.lines().count(), 1); + let error = serde_json::from_str::(&stderr).unwrap(); + assert_eq!(error["schemaVersion"], 1); + assert_eq!(error["_tag"], "UsageError"); + assert_eq!(error["error"]["code"], "INVALID_ARGUMENT"); + assert_eq!( + error["error"]["message"], + "unexpected argument '--unknown\u{0085}\u{202e}\rforged" + ); +} + #[test] fn empty_json_search_is_a_tagged_usage_error() { - let (exit, stdout, stderr) = run(&["skilld", "search", "--json"], OutputContext::Plain); + let (exit, stdout, stderr) = run(&["skilld", "search", "--json"], PLAIN); assert_eq!(exit, 2); assert!(stdout.is_empty()); @@ -395,7 +546,7 @@ fn json_search_errors_are_tagged_and_written_to_stderr() { &SearchHost { response: Err(CommandError::service("Skill search timed out")), }, - OutputContext::auto(false, false, false, false, false, 80), + auto(false, false, false, false, false, 80), &mut stdout, &mut stderr, ); @@ -415,6 +566,65 @@ fn json_search_errors_are_tagged_and_written_to_stderr() { ); } +#[test] +fn remote_errors_are_terminal_safe_and_json_preserves_the_original_text() { + let code = "REMOTE\nCODE\u{202e}"; + let message = "line one\u{1b}[31m\u{0085}\u{202e}\rforged\nline two"; + let host = SearchHost { + response: Err(CommandError::remote(RemoteError::new(code, message))), + }; + let mut human_stdout = Vec::new(); + let mut human_stderr = Vec::new(); + let mut plain_stdout = Vec::new(); + let mut plain_stderr = Vec::new(); + let mut json_stdout = Vec::new(); + let mut json_stderr = Vec::new(); + + let human = run_with_output( + ["skilld", "search", "grill"], + &host, + OutputContext::HumanTerminal { + width: 80, + color: false, + platform: CommandPlatform::Unix, + }, + &mut human_stdout, + &mut human_stderr, + ); + let plain = run_with_output( + ["skilld", "search", "grill", "--plain"], + &host, + PLAIN, + &mut plain_stdout, + &mut plain_stderr, + ); + let json = run_with_output( + ["skilld", "search", "grill", "--json"], + &host, + PLAIN, + &mut json_stdout, + &mut json_stderr, + ); + + assert_eq!(human.exit_code, 1); + assert!(human_stdout.is_empty()); + assert_eq!( + String::from_utf8(human_stderr).unwrap(), + "✗ line one [31m forged line two (REMOTE CODE )\n" + ); + assert_eq!(plain.exit_code, 1); + assert!(plain_stdout.is_empty()); + assert_eq!( + String::from_utf8(plain_stderr).unwrap(), + "REMOTE\\nCODE\\u{202E}: line one\\u{001B}[31m\\u{0085}\\u{202E}\\rforged\\nline two\n" + ); + assert_eq!(json.exit_code, 1); + assert!(json_stdout.is_empty()); + let json_error = serde_json::from_slice::(&json_stderr).unwrap(); + assert_eq!(json_error["error"]["code"], code); + assert_eq!(json_error["error"]["message"], message); +} + #[test] fn stdout_failures_report_an_operation_error() { let mut stdout = WriteErrorWriter; @@ -425,7 +635,7 @@ fn stdout_failures_report_an_operation_error() { &SearchHost { response: Ok(response()), }, - OutputContext::Plain, + PLAIN, &mut stdout, &mut stderr, ); diff --git a/crates/skilld-command/tests/remote.rs b/crates/skilld-command/tests/remote.rs index 5da64fce..9245cd88 100644 --- a/crates/skilld-command/tests/remote.rs +++ b/crates/skilld-command/tests/remote.rs @@ -1310,6 +1310,95 @@ fn direct_github_access_resolves_an_exact_public_commit_without_tokens() { })); } +#[test] +fn prepare_exact_rejects_a_conflicting_selector_commit_before_http() { + let selector_commit = "0123456789abcdef0123456789abcdef01234567"; + let expected_commit = CommitSha::parse("ffffffffffffffffffffffffffffffffffffffff").unwrap(); + let http = Arc::new(FakeHttp::default()); + let remote = SkilldRemote::new( + http.clone(), + Arc::new(NoTokenProvider), + NativeRemoteConfig::Unconfigured, + ); + let selector = RemoteSelector::parse(&format!( + "github:skilld-dev/skills/skills/example#commit:{selector_commit}" + )) + .unwrap(); + + let error = remote + .prepare_exact(&selector, &expected_commit, true) + .unwrap_err(); + + assert_eq!(error.code, "SOURCE_MISMATCH"); + assert!(http.requests.lock().unwrap().is_empty()); +} + +#[test] +fn prepare_exact_keeps_a_matching_selector_commit_as_provenance() { + let commit = "0123456789abcdef0123456789abcdef01234567"; + let expected_commit = CommitSha::parse(commit).unwrap(); + let (pin, responses) = verified_remote_responses(); + let http = Arc::new(FakeHttp::with(responses)); + let remote = SkilldRemote::new( + http.clone(), + Arc::new(NoTokenProvider), + NativeRemoteConfig::Pinned(pin), + ) + .with_endpoint("http://127.0.0.1:8787") + .unwrap() + .with_sleeper(Arc::new(NoSleep)); + let selector = RemoteSelector::parse(&format!( + "github:skilld-dev/skills/skills/example#commit:{commit}" + )) + .unwrap(); + + let prepared = remote + .prepare_exact(&selector, &expected_commit, false) + .unwrap(); + + assert!(matches!( + prepared.locked_source, + LockedSource::Remote { + ref source, + ref commit_sha, + .. + } if source == &selector.canonical() && commit_sha == commit + )); + assert_eq!(http.requests.lock().unwrap().len(), 4); +} + +#[test] +fn direct_github_access_rejects_a_commit_response_that_changed_the_requested_commit() { + let requested = "0123456789abcdef0123456789abcdef01234567"; + let returned = "ffffffffffffffffffffffffffffffffffffffff"; + let tree = "89abcdef0123456789abcdef0123456789abcdef"; + let http = Arc::new(FakeHttp::with([ + response( + 200, + br#"{"private":false,"default_branch":"main"}"#.to_vec(), + ), + response( + 200, + format!(r#"{{"sha":"{returned}","commit":{{"tree":{{"sha":"{tree}"}}}}}}"#), + ), + ])); + let remote = SkilldRemote::new( + http.clone(), + Arc::new(NoTokenProvider), + NativeRemoteConfig::Unconfigured, + ) + .with_sleeper(Arc::new(NoSleep)); + let selector = RemoteSelector::parse(&format!( + "github:skilld-dev/skills/skills/example#commit:{requested}" + )) + .unwrap(); + + let error = remote.prepare(&selector, true).unwrap_err(); + + assert_eq!(error.code, "SOURCE_MISMATCH"); + assert_eq!(http.requests.lock().unwrap().len(), 2); +} + #[test] fn direct_install_error_gives_an_agent_an_exact_recovery() { let remote = SkilldRemote::new( diff --git a/crates/skilld-command/tests/run.rs b/crates/skilld-command/tests/run.rs new file mode 100644 index 00000000..cab56dee --- /dev/null +++ b/crates/skilld-command/tests/run.rs @@ -0,0 +1,1396 @@ +use std::fs::{self, File}; +use std::path::{Path, PathBuf}; +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; + +use skilld_command::{ + BundledSkillProvider, CommandError, CommandPlatform, FileContent, FileKind, Host, LocalHost, + OutputContext, PreparedRemoteSkill, RemoteLatestCommit, RemoteProvider, RemoteSourceState, + RemoteUpdateComparison, RemoteUpdateResult, RunOutcome, SkillOrigin, run_with_output, +}; +use skilld_core::{ + CommitSha, InstallSource, LockedSource, PreparedFile, RemoteError, RemoteSelector, + SearchResponse, SourceStatus, +}; + +const INSTRUCTIONS: &[u8] = + b"---\nname: vue\ndescription: Build Vue interfaces.\n---\n\n# Use Vue\n"; + +const MAX_TEST_DEPTH: usize = 9; + +struct StubRemote { + calls: AtomicUsize, + exact_calls: AtomicUsize, + files: Vec, + skill_path: String, +} + +impl StubRemote { + fn new(files: Vec) -> Self { + Self { + calls: AtomicUsize::new(0), + exact_calls: AtomicUsize::new(0), + files, + skill_path: "skills/vue".to_owned(), + } + } + + fn with_skill_path(mut self, skill_path: &str) -> Self { + self.skill_path = skill_path.to_owned(); + self + } + + fn prepared(&self, commit_sha: String) -> PreparedRemoteSkill { + PreparedRemoteSkill { + files: self.files.clone(), + locked_source: LockedSource::Remote { + source: "skilld:vuejs/core/vue".to_owned(), + commit_sha, + skill_path: self.skill_path.clone(), + }, + source_status: SourceStatus::Unverified { + content_sha256: "b".repeat(64), + installed_sha256: "c".repeat(64), + }, + } + } +} + +fn file(path: &str, mode: u32, bytes: &[u8]) -> PreparedFile { + PreparedFile { + path: path.to_owned(), + mode, + bytes: bytes.to_vec(), + } +} + +fn skill_files() -> Vec { + vec![ + file("SKILL.md", 0o644, INSTRUCTIONS), + file( + "references/api.md", + 0o644, + b"# The Vue API surface\n\nEvery reactive primitive.\n", + ), + file("scripts/check.mjs", 0o755, b"#!/usr/bin/env node\nrun()\n"), + ] +} + +impl RemoteProvider for StubRemote { + fn search(&self, _query: &str, _limit: u8) -> Result { + unimplemented!("search is out of scope for a run") + } + + fn prepare( + &self, + _selector: &RemoteSelector, + _direct: bool, + ) -> Result { + self.calls.fetch_add(1, Ordering::SeqCst); + Ok(self.prepared("a".repeat(40))) + } + + fn prepare_exact( + &self, + selector: &RemoteSelector, + expected_commit: &CommitSha, + _direct: bool, + ) -> Result { + let _ = selector; + self.exact_calls.fetch_add(1, Ordering::SeqCst); + Ok(self.prepared(expected_commit.as_str().to_owned())) + } + + fn source_state( + &self, + _selector: &RemoteSelector, + _artifact_id: &str, + _commit_sha: &str, + ) -> Result { + unimplemented!("source state is out of scope for a run") + } + + fn latest_commit( + &self, + _selector: &RemoteSelector, + _direct: bool, + ) -> Result { + unimplemented!("latest commit is out of scope for a run") + } + + fn compare_updates( + &self, + _comparisons: &[RemoteUpdateComparison], + ) -> Result, RemoteError> { + unimplemented!("update comparison is out of scope for a run") + } +} + +struct Fixture { + _temporary: tempfile::TempDir, + project: PathBuf, + global: PathBuf, + host: LocalHost, + remote: Arc, +} + +fn remote_fixture(files: Vec) -> Fixture { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + let global = temporary.path().join("global"); + fs::create_dir_all(&project).unwrap(); + let remote = Arc::new(StubRemote::new(files)); + let host = LocalHost::new(project.clone(), global.clone()).with_remote_provider(remote.clone()); + Fixture { + _temporary: temporary, + project, + global, + host, + remote, + } +} + +fn remote_fixture_with_skill_path(files: Vec, skill_path: &str) -> Fixture { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + let global = temporary.path().join("global"); + fs::create_dir_all(&project).unwrap(); + let remote = Arc::new(StubRemote::new(files).with_skill_path(skill_path)); + let host = LocalHost::new(project.clone(), global.clone()).with_remote_provider(remote.clone()); + Fixture { + _temporary: temporary, + project, + global, + host, + remote, + } +} + +fn run_cli(host: &H, args: Vec) -> (u8, String, String) { + run_cli_on(host, args, CommandPlatform::Unix) +} + +fn run_cli_on( + host: &H, + args: Vec, + platform: CommandPlatform, +) -> (u8, String, String) { + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let result = run_with_output( + args, + host, + OutputContext::Plain { platform }, + &mut stdout, + &mut stderr, + ); + ( + result.exit_code, + String::from_utf8(stdout).unwrap(), + String::from_utf8(stderr).unwrap(), + ) +} + +fn load(host: &LocalHost) -> Box { + match host + .run_skill( + InstallSource::Remote("skilld:vuejs/core/vue".to_owned()), + &[], + None, + ) + .unwrap() + { + RunOutcome::Load(skill) => skill, + RunOutcome::Files { .. } => panic!("expected a Skill load"), + } +} + +fn pull(host: &LocalHost, wanted: &[&str]) -> Vec { + let wanted = wanted + .iter() + .map(|path| (*path).to_owned()) + .collect::>(); + match host + .run_skill( + InstallSource::Remote("skilld:vuejs/core/vue".to_owned()), + &wanted, + Some(&CommitSha::parse("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap()), + ) + .unwrap() + { + RunOutcome::Files { files, .. } => files, + RunOutcome::Load(_) => panic!("expected supporting files"), + } +} + +fn tree(root: &Path) -> Vec { + let Ok(entries) = fs::read_dir(root) else { + return vec![]; + }; + entries + .filter_map(Result::ok) + .flat_map(|entry| { + let path = entry.path(); + if path.is_dir() { + tree(&path) + } else { + vec![path] + } + }) + .collect() +} + +#[test] +fn a_remote_run_writes_nothing_to_disk() { + let fixture = remote_fixture(skill_files()); + + let skill = load(&fixture.host); + + assert_eq!(skill.name, "vue"); + assert!(skill.instructions.contains("# Use Vue")); + assert_eq!(tree(&fixture.project), Vec::::new()); + assert_eq!(tree(&fixture.global), Vec::::new()); + assert!(!fixture.global.join("runs").exists()); +} + +#[test] +fn a_remote_run_names_supporting_files_without_printing_them() { + let fixture = remote_fixture(vec![ + file("SKILL.md", 0o644, INSTRUCTIONS), + file("references/api.md", 0o644, b"secret-supporting-prompt\n"), + file("scripts/check.mjs", 0o755, b"#!/usr/bin/env node\nrun()\n"), + ]); + + let (_, stdout, stderr) = 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(&stdout).unwrap(); + let files = output["data"]["files"].as_array().unwrap(); + + let paths = files + .iter() + .map(|file| file["path"].as_str().unwrap()) + .collect::>(); + assert_eq!(paths, ["references/api.md", "scripts/check.mjs"]); + assert!(stderr.is_empty()); + assert!(!stdout.contains("secret-supporting-prompt")); + assert!(!stdout.contains("env node")); +} + +#[test] +fn an_executable_supporting_file_is_never_readable() { + let fixture = remote_fixture(skill_files()); + + let skill = load(&fixture.host); + + let script = skill + .files + .iter() + .find(|file| file.path == "scripts/check.mjs") + .unwrap(); + assert_eq!(script.kind, FileKind::Executable); + assert!(!script.kind.is_readable()); +} + +#[test] +fn pulling_a_text_file_returns_its_content() { + let fixture = remote_fixture(skill_files()); + + let pulled = pull(&fixture.host, &["references/api.md"]); + + assert_eq!(pulled.len(), 1); + assert_eq!( + pulled[0].content, + FileContent::Text("# The Vue API surface\n\nEvery reactive primitive.\n".to_owned()) + ); + assert_eq!(tree(&fixture.global), Vec::::new()); +} + +#[test] +fn pulling_an_executable_withholds_it() { + let fixture = remote_fixture(skill_files()); + + let pulled = pull(&fixture.host, &["scripts/check.mjs"]); + + assert_eq!( + pulled[0].content, + FileContent::Withheld { + reason: "the Skill marks this file executable" + } + ); +} + +#[test] +fn pulling_an_unknown_file_fails() { + let fixture = remote_fixture(skill_files()); + + let error = fixture + .host + .run_skill( + InstallSource::Remote("skilld:vuejs/core/vue".to_owned()), + &["references/nope.md".to_owned()], + Some(&CommitSha::parse("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap()), + ) + .unwrap_err(); + + assert_eq!(error.code, "SOURCE_NOT_FOUND"); +} + +#[test] +fn a_local_run_reports_the_directory_it_read() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + let skill = project.join("my-skill"); + fs::create_dir_all(skill.join("references")).unwrap(); + fs::write( + skill.join("SKILL.md"), + "---\nname: my-skill\ndescription: Test fixture.\n---\n\n# Do the thing\n", + ) + .unwrap(); + fs::write(skill.join("references/api.md"), "# Notes\n").unwrap(); + let host = LocalHost::new(project.clone(), temporary.path().join("global")); + + let RunOutcome::Load(loaded) = host + .run_skill(InstallSource::Local(skill.clone()), &[], None) + .unwrap() + else { + panic!("expected a Skill load") + }; + + assert_eq!(loaded.name, "my-skill"); + assert_eq!(loaded.source_status, "local"); + assert!(matches!(loaded.origin, SkillOrigin::Local { .. })); + assert!(!project.join(".skills").exists()); +} + +#[test] +fn a_local_run_reports_a_directory_without_instructions() { + let temporary = tempfile::tempdir().unwrap(); + let empty = temporary.path().join("empty"); + fs::create_dir_all(&empty).unwrap(); + let host = LocalHost::new( + temporary.path().to_path_buf(), + temporary.path().join("global"), + ); + + let error = host + .run_skill(InstallSource::Local(empty), &[], None) + .unwrap_err(); + + assert_eq!(error.code, "INVALID_SOURCE"); +} + +struct TrackingBundled { + source: PathBuf, + source_calls: AtomicUsize, +} + +impl BundledSkillProvider for TrackingBundled { + fn skilld_run_files(&self) -> Result, CommandError> { + Ok(vec![ + file( + "SKILL.md", + 0o644, + b"---\nname: skilld\ndescription: Test bundled Skill.\n---\n\n# Use skilld\n", + ), + file("references/api.md", 0o644, b"# API\n"), + ]) + } + + fn skilld_source(&self) -> Result { + self.source_calls.fetch_add(1, Ordering::SeqCst); + Ok(self.source.clone()) + } +} + +#[test] +fn bundled_run_preserves_identity_without_install_staging() { + let temporary = tempfile::tempdir().unwrap(); + let provider = Arc::new(TrackingBundled { + source: temporary.path().join("must-not-materialize"), + source_calls: AtomicUsize::new(0), + }); + let host = LocalHost::new( + temporary.path().join("project"), + temporary.path().join("global"), + ) + .with_bundled_provider(provider.clone()); + + let (exit, stdout, stderr) = run_cli( + &host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld".to_owned(), + "--json".to_owned(), + ], + ); + let output: serde_json::Value = serde_json::from_str(&stdout).unwrap(); + + assert_eq!(exit, 0); + assert!(stderr.is_empty()); + assert_eq!(provider.source_calls.load(Ordering::SeqCst), 0); + assert_eq!(output["data"]["origin"]["_tag"], "bundled"); + assert_eq!(output["data"]["origin"]["source"], "skilld"); + assert_eq!( + output["data"]["files"][0]["readArgv"], + serde_json::json!([ + "skilld", + "run", + "skilld", + "--file=references/api.md", + "--json" + ]) + ); + assert!(output["data"]["installArgv"]["project"].is_null()); + assert_eq!( + output["data"]["installArgv"]["global"], + serde_json::json!(["skilld", "install", "skilld", "--global"]) + ); + let read_argv = output["data"]["files"][0]["readArgv"] + .as_array() + .unwrap() + .iter() + .map(|value| value.as_str().unwrap().to_owned()) + .collect::>(); + let (read_exit, read_stdout, read_error) = run_cli(&host, read_argv); + let read: serde_json::Value = serde_json::from_str(&read_stdout).unwrap(); + + assert_eq!(read_exit, 0); + assert!(read_error.is_empty()); + assert_eq!(read["data"]["origin"]["_tag"], "bundled"); + assert_eq!(read["data"]["files"][0]["content"]["value"], "# API\n"); + + let (plain_exit, plain, plain_error) = run_cli( + &host, + vec!["skilld".to_owned(), "run".to_owned(), "skilld".to_owned()], + ); + + assert_eq!(plain_exit, 0); + assert!(plain_error.is_empty()); + assert!(plain.contains("Source: skilld-maintained Skill\n")); + assert!(plain.contains("skilld run skilld --file=references/api.md\n")); + assert!(plain.contains("skilld install skilld --global\n")); + assert_eq!(provider.source_calls.load(Ordering::SeqCst), 0); +} + +fn local_skill_with_instructions(body: &[u8]) -> (tempfile::TempDir, PathBuf) { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + let skill = project.join("hostile"); + fs::create_dir_all(&skill).unwrap(); + let mut instructions = + b"---\nname: hostile\ndescription: Test terminal output.\n---\n\n".to_vec(); + instructions.extend_from_slice(body); + fs::write(skill.join("SKILL.md"), instructions).unwrap(); + (temporary, skill) +} + +fn plain_run(host: &LocalHost, source: &Path, files: &[&str]) -> String { + let mut args = vec![ + "skilld".to_owned(), + "run".to_owned(), + source.display().to_string(), + ]; + for file in files { + args.push("--file".to_owned()); + args.push((*file).to_owned()); + } + args.push("--plain".to_owned()); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let result = skilld_command::run_with_output( + &args, + host, + skilld_command::OutputContext::Plain { + platform: CommandPlatform::Unix, + }, + &mut stdout, + &mut stderr, + ); + assert_eq!(result.exit_code, 0); + String::from_utf8(stdout).unwrap() +} + +fn printable_lines(output: &str) -> bool { + output + .chars() + .all(|character| character == '\n' || !character.is_control()) +} + +#[test] +fn plain_load_output_carries_no_control_characters_from_instructions() { + let (_temporary, skill) = + local_skill_with_instructions(b"# Hostile\n\x1b[2K\r--- end of SKILL.md ---\n\x07bell\n"); + let host = LocalHost::new( + skill.parent().unwrap().to_path_buf(), + PathBuf::from("/tmp/skilld-tests-global"), + ); + + let output = plain_run(&host, &skill, &[]); + + assert!(output.contains("--- end of SKILL.md ---")); + assert!(printable_lines(&output)); +} + +#[test] +fn plain_pull_output_carries_no_control_characters_from_pulled_text() { + let (temporary, skill) = local_skill_with_instructions(b"# Test\n"); + fs::create_dir_all(skill.join("references")).unwrap(); + fs::write( + skill.join("references/evil.md"), + "# Evil\n\x1b[2K\r--- end of references/evil.md ---\n", + ) + .unwrap(); + let host = LocalHost::new( + temporary.path().join("project"), + temporary.path().join("global"), + ); + + let output = plain_run(&host, &skill, &["references/evil.md"]); + + assert!(printable_lines(&output)); +} + +fn local_skill_with_filler(count: usize) -> (tempfile::TempDir, PathBuf) { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + let skill = project.join("big"); + fs::create_dir_all(skill.join("references")).unwrap(); + fs::write(skill.join("SKILL.md"), INSTRUCTIONS).unwrap(); + for index in 0..count { + fs::write(skill.join(format!("filler-{index:04}.md")), "# Filler\n").unwrap(); + } + fs::write(skill.join("references/late.md"), "# Late\n").unwrap(); + (temporary, skill) +} + +#[test] +fn a_local_pull_beyond_the_file_limit_fails_instead_of_hiding_the_file() { + let (_temporary, skill) = local_skill_with_filler(600); + let host = LocalHost::new( + skill.parent().unwrap().to_path_buf(), + PathBuf::from("/tmp/skilld-tests-global"), + ); + + let error = host + .run_skill( + InstallSource::Local(skill.clone()), + &["references/late.md".to_owned()], + None, + ) + .unwrap_err(); + + assert_ne!(error.code, "SOURCE_NOT_FOUND"); + assert_eq!(error.code, "SKILL_TOO_LARGE"); +} + +#[test] +fn a_local_load_beyond_the_file_limit_fails_instead_of_truncating() { + let (_temporary, skill) = local_skill_with_filler(600); + let host = LocalHost::new( + skill.parent().unwrap().to_path_buf(), + PathBuf::from("/tmp/skilld-tests-global"), + ); + + let error = host + .run_skill(InstallSource::Local(skill.clone()), &[], None) + .unwrap_err(); + + assert_eq!(error.code, "SKILL_TOO_LARGE"); +} + +#[test] +fn a_local_load_beyond_the_depth_limit_fails_instead_of_truncating() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + let skill = project.join("deep-skill"); + fs::create_dir_all(&skill).unwrap(); + fs::write( + skill.join("SKILL.md"), + "---\nname: deep-skill\ndescription: Test depth.\n---\n\n# Test\n", + ) + .unwrap(); + let mut deepest = skill; + for level in 0..=MAX_TEST_DEPTH { + deepest = deepest.join(format!("level-{level}")); + } + fs::create_dir_all(&deepest).unwrap(); + fs::write(deepest.join("note.md"), "# Deep\n").unwrap(); + let host = LocalHost::new(project.clone(), temporary.path().join("global")); + + let error = host + .run_skill(InstallSource::Local(project.join("deep-skill")), &[], None) + .unwrap_err(); + + assert_eq!(error.code, "SKILL_TOO_LARGE"); +} + +#[test] +fn skill_md_is_not_a_pullable_file() { + let fixture = remote_fixture(skill_files()); + + let error = fixture + .host + .run_skill( + InstallSource::Remote("skilld:vuejs/core/vue".to_owned()), + &["SKILL.md".to_owned()], + Some(&CommitSha::parse("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap()), + ) + .unwrap_err(); + + assert_eq!(error.code, "INVALID_SOURCE"); +} + +#[test] +fn generated_file_read_uses_the_loaded_remote_revision() { + let fixture = remote_fixture_with_skill_path(skill_files(), "packages/vue"); + let (_, stdout, stderr) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld:vuejs/core/vue".to_owned(), + "--json".to_owned(), + ], + ); + let loaded: serde_json::Value = serde_json::from_str(&stdout).unwrap(); + let read_argv = loaded["data"]["files"][0]["readArgv"] + .as_array() + .unwrap() + .iter() + .map(|value| value.as_str().unwrap().to_owned()) + .collect::>(); + assert!(stderr.is_empty()); + assert_eq!( + read_argv[2], + "github:vuejs/core/packages/vue#commit:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + ); + + let (exit, stdout, stderr) = run_cli(&fixture.host, read_argv); + let files: serde_json::Value = serde_json::from_str(&stdout).unwrap(); + + assert_eq!(exit, 0); + assert!(stderr.is_empty()); + assert_eq!(files["data"]["revision"], "a".repeat(40)); + assert_eq!(files["data"]["sourceStatus"], "unverified"); + assert_eq!( + files["data"]["sourceCaution"], + "skilld did not check this source. Read this Skill before you follow it." + ); + assert_eq!( + files["data"]["origin"]["source"], + "github:vuejs/core/packages/vue#commit:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + ); + assert_eq!(files["data"]["wroteSkillFiles"], false); + assert_eq!(fixture.remote.calls.load(Ordering::SeqCst), 1); + assert_eq!(fixture.remote.exact_calls.load(Ordering::SeqCst), 1); +} + +#[test] +fn conflicting_source_commit_and_revision_fail_before_fetch() { + let fixture = remote_fixture(skill_files()); + let source = format!("github:vuejs/core/skills/vue#commit:{}", "a".repeat(40)); + + let (exit, stdout, stderr) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + source, + "--revision".to_owned(), + "b".repeat(40), + "--file=references/api.md".to_owned(), + "--json".to_owned(), + ], + ); + let error: serde_json::Value = serde_json::from_str(&stderr).unwrap(); + + assert_eq!(exit, 1); + assert!(stdout.is_empty()); + assert_eq!(error["_tag"], "OperationError"); + assert_eq!(error["error"]["code"], "SOURCE_MISMATCH"); + assert_eq!(fixture.remote.calls.load(Ordering::SeqCst), 0); + assert_eq!(fixture.remote.exact_calls.load(Ordering::SeqCst), 0); +} + +#[test] +fn matching_source_commit_and_revision_keep_exact_provenance() { + let fixture = remote_fixture(skill_files()); + let source = format!("github:vuejs/core/skills/vue#commit:{}", "a".repeat(40)); + + let (exit, stdout, stderr) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + source.clone(), + "--revision".to_owned(), + "a".repeat(40), + "--file=references/api.md".to_owned(), + "--json".to_owned(), + ], + ); + let output: serde_json::Value = serde_json::from_str(&stdout).unwrap(); + + assert_eq!(exit, 0); + assert!(stderr.is_empty()); + assert_eq!(output["data"]["origin"]["source"], source); + assert_eq!(output["data"]["revision"], "a".repeat(40)); + assert_eq!(fixture.remote.calls.load(Ordering::SeqCst), 0); + assert_eq!(fixture.remote.exact_calls.load(Ordering::SeqCst), 1); +} + +#[test] +fn remote_file_read_without_revision_fails_before_fetch() { + let fixture = remote_fixture(skill_files()); + + let (exit, stdout, stderr) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld:vuejs/core/vue".to_owned(), + "--file".to_owned(), + "references/api.md".to_owned(), + "--json".to_owned(), + ], + ); + let error: serde_json::Value = serde_json::from_str(&stderr).unwrap(); + + assert_eq!(exit, 2); + assert!(stdout.is_empty()); + assert_eq!(error["error"]["code"], "INVALID_SOURCE"); + assert_eq!( + error["error"]["message"], + "Remote --file reads require --revision. Run the Skill without --file first. Then repeat this run with the returned revision." + ); + assert_eq!(fixture.remote.calls.load(Ordering::SeqCst), 0); + assert_eq!(fixture.remote.exact_calls.load(Ordering::SeqCst), 0); + + let error = fixture + .host + .run_skill( + InstallSource::Remote("skilld:vuejs/core/vue".to_owned()), + &["references/api.md".to_owned()], + None, + ) + .unwrap_err(); + + assert_eq!(error.code, "INVALID_SOURCE"); + assert_eq!(fixture.remote.calls.load(Ordering::SeqCst), 0); +} + +#[test] +fn remote_run_rejects_a_hash_in_the_attested_skill_path() { + let fixture = remote_fixture_with_skill_path(skill_files(), "skills/vue#archive"); + + let (exit, stdout, stderr) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld:vuejs/core/vue".to_owned(), + "--json".to_owned(), + ], + ); + let error: serde_json::Value = serde_json::from_str(&stderr).unwrap(); + + assert_eq!(exit, 2); + assert!(stdout.is_empty()); + assert_eq!(error["error"]["code"], "INVALID_SOURCE"); +} + +#[test] +fn json_run_is_compact_typed_and_uses_argument_arrays() { + let fixture = remote_fixture(skill_files()); + + let (_, stdout, stderr) = 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(&stdout).unwrap(); + + assert!(stderr.is_empty()); + assert!(!stdout.contains("\n \"")); + assert_eq!(output["_tag"], "Success"); + assert_eq!(output["notices"], serde_json::json!([])); + assert_eq!(output["data"]["_tag"], "load"); + assert_eq!(output["data"]["revision"], "a".repeat(40)); + assert_eq!(output["data"]["wroteSkillFiles"], false); + assert_eq!( + output["data"]["files"][0]["readArgv"], + serde_json::json!([ + "skilld", + "run", + "github:vuejs/core/skills/vue#commit:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "--revision", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "--file=references/api.md", + "--json" + ]) + ); + assert_eq!( + output["data"]["installArgv"]["project"], + serde_json::json!([ + "skilld", + "install", + "github:vuejs/core/skills/vue#commit:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + ]) + ); + assert_eq!( + output["data"]["installArgv"]["global"], + serde_json::json!([ + "skilld", + "install", + "github:vuejs/core/skills/vue#commit:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "--global" + ]) + ); +} + +#[test] +fn remote_install_guidance_pins_the_reviewed_path_and_commit() { + let fixture = remote_fixture(skill_files()); + + let (_, plain, plain_error) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld:vuejs/core/vue".to_owned(), + ], + ); + let (_, json, json_error) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "github:vuejs/core/catalog/vue".to_owned(), + "--direct".to_owned(), + "--json".to_owned(), + ], + ); + let output: serde_json::Value = serde_json::from_str(&json).unwrap(); + let exact = format!("github:vuejs/core/skills/vue#commit:{}", "a".repeat(40)); + + assert!(plain_error.is_empty()); + assert!(json_error.is_empty()); + assert!(plain.contains(&format!("skilld install '{exact}'\n"))); + assert!(plain.contains(&format!("skilld install '{exact}' --global\n"))); + assert_eq!( + output["data"]["installArgv"]["project"], + serde_json::json!(["skilld", "install", exact.clone(), "--direct"]) + ); + assert_eq!( + output["data"]["installArgv"]["global"], + serde_json::json!(["skilld", "install", exact, "--direct", "--global"]) + ); +} + +#[test] +fn plain_and_json_run_outputs_handle_metacharacter_paths_as_data() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project with $(printf injected)"); + let skill = project.join("my-skill"); + fs::create_dir_all(&skill).unwrap(); + fs::write( + skill.join("SKILL.md"), + "---\nname: my-skill\ndescription: Test fixture.\n---\n\n# Do the thing\n", + ) + .unwrap(); + fs::write(skill.join("-$(printf injected).md"), "# Notes\n").unwrap(); + let host = LocalHost::new(project.clone(), temporary.path().join("global")); + + let (_, plain, plain_error) = run_cli( + &host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + skill.display().to_string(), + "--plain".to_owned(), + ], + ); + let (_, json, json_error) = run_cli( + &host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + skill.display().to_string(), + "--json".to_owned(), + ], + ); + let output: serde_json::Value = serde_json::from_str(&json).unwrap(); + + assert!(plain_error.is_empty()); + assert!(json_error.is_empty()); + assert!(plain.contains("'--file=-$(printf injected).md'")); + assert!(plain.contains(&format!("skilld install '{}'", skill.display()))); + assert_eq!( + output["data"]["files"][0]["readArgv"], + serde_json::json!([ + "skilld", + "run", + skill.display().to_string(), + "--file=-$(printf injected).md", + "--json" + ]) + ); + let read_argv = output["data"]["files"][0]["readArgv"] + .as_array() + .unwrap() + .iter() + .map(|value| value.as_str().unwrap().to_owned()) + .collect::>(); + let (exit, read_stdout, read_stderr) = run_cli(&host, read_argv); + let read: serde_json::Value = serde_json::from_str(&read_stdout).unwrap(); + assert_eq!(exit, 0); + assert!(read_stderr.is_empty()); + assert_eq!(read["data"]["files"][0]["content"]["value"], "# Notes\n"); +} + +#[test] +fn generated_commands_quote_apostrophes_for_the_declared_platform() { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project's & $(printf injected)"); + let skill = project.join("my-skill"); + fs::create_dir_all(&skill).unwrap(); + fs::write( + skill.join("SKILL.md"), + "---\nname: my-skill\ndescription: Test fixture.\n---\n\n# Test\n", + ) + .unwrap(); + fs::write( + skill.join("reference's & $(printf injected).md"), + "# Notes\n", + ) + .unwrap(); + let host = LocalHost::new(project, temporary.path().join("global")); + let args = vec![ + "skilld".to_owned(), + "run".to_owned(), + skill.display().to_string(), + "--plain".to_owned(), + ]; + + let (_, unix, unix_error) = run_cli_on(&host, args.clone(), CommandPlatform::Unix); + let (_, powershell, powershell_error) = + run_cli_on(&host, args, CommandPlatform::WindowsPowerShell); + let mut human_stdout = Vec::new(); + let mut human_stderr = Vec::new(); + let human_result = run_with_output( + ["skilld", "run", skill.to_str().unwrap()], + &host, + OutputContext::HumanTerminal { + width: 120, + color: false, + platform: CommandPlatform::WindowsPowerShell, + }, + &mut human_stdout, + &mut human_stderr, + ); + let human = String::from_utf8(human_stdout).unwrap(); + + assert!(unix_error.is_empty()); + assert!(powershell_error.is_empty()); + assert_eq!(human_result.exit_code, 0); + assert!(human_stderr.is_empty()); + assert!(unix.contains("project'\\''s & $(printf injected)")); + assert!(unix.contains("'--file=reference'\\''s & $(printf injected).md'")); + assert!(powershell.contains("project''s & $(printf injected)")); + assert!(powershell.contains("'--file=reference''s & $(printf injected).md'")); + assert!(!powershell.contains("'\\''")); + assert!(human.contains("'--file=reference''s & $(printf injected).md'")); +} + +#[cfg(unix)] +#[test] +fn local_run_rejects_terminal_formatting_in_the_source_root_without_stdout() { + for control in ['\n', '\t', '\u{0085}', '\u{202e}'] { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join(format!("project{control}forged")); + let skill = project.join("my-skill"); + write_local_skill(&skill, "my-skill"); + let host = LocalHost::new( + temporary.path().join("project"), + temporary.path().join("global"), + ); + + let (exit, stdout, stderr) = run_cli( + &host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + skill.display().to_string(), + "--plain".to_owned(), + ], + ); + + assert_eq!(exit, 1); + assert!(stdout.is_empty()); + assert!(printable_lines(&stderr)); + } +} + +#[cfg(unix)] +#[test] +fn local_run_rejects_terminal_formatting_in_file_names_without_stdout() { + for control in ['\n', '\t', '\u{0085}', '\u{202e}'] { + let temporary = tempfile::tempdir().unwrap(); + let skill = temporary.path().join("my-skill"); + write_local_skill(&skill, "my-skill"); + fs::write( + skill.join(format!("reference{control}forged.md")), + "# Notes\n", + ) + .unwrap(); + let host = LocalHost::new( + temporary.path().join("project"), + temporary.path().join("global"), + ); + + let (exit, stdout, stderr) = run_cli( + &host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + skill.display().to_string(), + "--plain".to_owned(), + ], + ); + + assert_eq!(exit, 1); + assert!(stdout.is_empty()); + assert!(printable_lines(&stderr)); + } +} + +#[test] +fn plain_run_removes_terminal_formatting_but_json_preserves_text() { + let instructions = "---\nname: vue\ndescription: Test.\n---\n\n# Start\n\u{1b}[2JCSI\n\u{1b}]0;forged\u{7}OSC\tkept\n\u{202e}reordered\n"; + let supporting = + "before\u{1b}[31mred\u{1b}[0m\n\u{1b}]8;;https://example.com\u{7}link\n\u{2067}isolated\n"; + let fixture = remote_fixture(vec![ + file("SKILL.md", 0o644, instructions.as_bytes()), + file("references/api.md", 0o644, supporting.as_bytes()), + ]); + + let (_, loaded_plain, _) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld:vuejs/core/vue".to_owned(), + ], + ); + let (_, pulled_plain, _) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld:vuejs/core/vue".to_owned(), + "--revision".to_owned(), + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_owned(), + "--file".to_owned(), + "references/api.md".to_owned(), + ], + ); + let (_, loaded_json, _) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld:vuejs/core/vue".to_owned(), + "--json".to_owned(), + ], + ); + let json: serde_json::Value = serde_json::from_str(&loaded_json).unwrap(); + let (_, pulled_json, _) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld:vuejs/core/vue".to_owned(), + "--revision".to_owned(), + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_owned(), + "--file".to_owned(), + "references/api.md".to_owned(), + "--json".to_owned(), + ], + ); + let pulled_json: serde_json::Value = serde_json::from_str(&pulled_json).unwrap(); + + assert!( + loaded_plain + .chars() + .chain(pulled_plain.chars()) + .all(|character| !character.is_control() || matches!(character, '\n' | '\t')) + ); + assert!(!loaded_plain.contains('\u{202e}')); + assert!(!pulled_plain.contains('\u{2067}')); + assert_eq!(json["data"]["instructions"], instructions); + assert_eq!( + pulled_json["data"]["files"][0]["content"]["value"], + supporting + ); +} + +#[test] +fn a_plain_file_read_reports_provenance_and_unverified_status() { + let fixture = remote_fixture(skill_files()); + + let (_, stdout, stderr) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld:vuejs/core/vue".to_owned(), + "--revision".to_owned(), + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_owned(), + "--file".to_owned(), + "references/api.md".to_owned(), + ], + ); + + assert!(stderr.is_empty()); + assert!(stdout.contains("Source: skilld:vuejs/core/vue\n")); + assert!(stdout.contains(&format!("Revision: {}\n", "a".repeat(40)))); + assert!(stdout.contains("Source status: unverified\n")); + assert!(stdout.contains("skilld did not check this source.")); +} + +#[test] +fn duplicate_file_requests_fail_before_remote_content_is_loaded() { + let fixture = remote_fixture(skill_files()); + + let (exit, stdout, stderr) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld:vuejs/core/vue".to_owned(), + "--file".to_owned(), + "references/api.md".to_owned(), + "--file".to_owned(), + "references/api.md".to_owned(), + "--json".to_owned(), + ], + ); + let error: serde_json::Value = serde_json::from_str(&stderr).unwrap(); + + assert_eq!(exit, 2); + assert!(stdout.is_empty()); + assert_eq!(error["error"]["code"], "INVALID_SOURCE"); + assert_eq!(fixture.remote.calls.load(Ordering::SeqCst), 0); +} + +#[test] +fn remote_file_requests_reject_c1_control_characters_before_fetch() { + let fixture = remote_fixture(skill_files()); + + let (exit, stdout, stderr) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld:vuejs/core/vue".to_owned(), + "--revision".to_owned(), + "a".repeat(40), + "--file=references/api\u{0085}forged.md".to_owned(), + "--json".to_owned(), + ], + ); + let error: serde_json::Value = serde_json::from_str(&stderr).unwrap(); + + assert_eq!(exit, 2); + assert!(stdout.is_empty()); + assert_eq!(error["error"]["code"], "INVALID_SOURCE"); + assert_eq!(fixture.remote.calls.load(Ordering::SeqCst), 0); + assert_eq!(fixture.remote.exact_calls.load(Ordering::SeqCst), 0); +} + +#[test] +fn remote_file_requests_reject_bidi_formatting_before_fetch() { + let fixture = remote_fixture(skill_files()); + + let (exit, stdout, stderr) = run_cli( + &fixture.host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld:vuejs/core/vue".to_owned(), + "--revision".to_owned(), + "a".repeat(40), + "--file=references/api\u{202e}forged.md".to_owned(), + "--json".to_owned(), + ], + ); + let error: serde_json::Value = serde_json::from_str(&stderr).unwrap(); + + assert_eq!(exit, 2); + assert!(stdout.is_empty()); + assert_eq!(error["error"]["code"], "INVALID_SOURCE"); + assert_eq!(fixture.remote.calls.load(Ordering::SeqCst), 0); + assert_eq!(fixture.remote.exact_calls.load(Ordering::SeqCst), 0); +} + +#[test] +fn direct_run_errors_use_run_recovery() { + let temporary = tempfile::tempdir().unwrap(); + let host = LocalHost::new( + temporary.path().join("project"), + temporary.path().join("global"), + ); + + let local = run_cli( + &host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "./my-skill".to_owned(), + "--direct".to_owned(), + "--json".to_owned(), + ], + ); + let bundled = run_cli( + &host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + "skilld".to_owned(), + "--direct".to_owned(), + "--json".to_owned(), + ], + ); + let local_error: serde_json::Value = serde_json::from_str(&local.2).unwrap(); + let bundled_error: serde_json::Value = serde_json::from_str(&bundled.2).unwrap(); + + assert_eq!(local.0, 2); + assert_eq!( + local_error["error"]["message"], + "--direct cannot run a local Skill. Remove --direct, then run the same command again." + ); + assert_eq!(bundled.0, 2); + assert_eq!( + bundled_error["error"]["message"], + "--direct cannot run the skilld-maintained Skill. Run skilld run skilld without --direct." + ); +} + +fn write_local_skill(path: &Path, frontmatter_name: &str) { + fs::create_dir_all(path).unwrap(); + fs::write( + path.join("SKILL.md"), + format!("---\nname: {frontmatter_name}\ndescription: Test.\n---\n\n# Test\n"), + ) + .unwrap(); +} + +fn local_run_error(path: &Path) -> serde_json::Value { + let host = LocalHost::new( + path.parent().unwrap().to_path_buf(), + path.parent().unwrap().join("global"), + ); + let (exit, stdout, stderr) = run_cli( + &host, + vec![ + "skilld".to_owned(), + "run".to_owned(), + path.display().to_string(), + "--json".to_owned(), + ], + ); + assert_eq!(exit, 1); + assert!(stdout.is_empty()); + serde_json::from_str(&stderr).unwrap() +} + +#[test] +fn a_local_run_rejects_frontmatter_name_drift() { + let temporary = tempfile::tempdir().unwrap(); + let skill = temporary.path().join("my-skill"); + write_local_skill(&skill, "another-skill"); + + let error = local_run_error(&skill); + + assert_eq!(error["error"]["code"], "INVALID_SOURCE"); +} + +#[test] +fn a_local_run_rejects_excess_depth_without_a_partial_inventory() { + let temporary = tempfile::tempdir().unwrap(); + let skill = temporary.path().join("my-skill"); + write_local_skill(&skill, "my-skill"); + let mut deep = skill.clone(); + for index in 0..9 { + deep.push(format!("level-{index}")); + } + fs::create_dir_all(&deep).unwrap(); + fs::write(deep.join("secret.md"), "never partially returned").unwrap(); + + let error = local_run_error(&skill); + + assert_eq!(error["error"]["code"], "SKILL_TOO_LARGE"); +} + +#[test] +fn a_local_run_rejects_excess_files_without_a_partial_inventory() { + let temporary = tempfile::tempdir().unwrap(); + let skill = temporary.path().join("my-skill"); + write_local_skill(&skill, "my-skill"); + for index in 0..512 { + fs::write(skill.join(format!("file-{index}.md")), "x").unwrap(); + } + + let error = local_run_error(&skill); + + assert_eq!(error["error"]["code"], "SKILL_TOO_LARGE"); +} + +#[test] +fn a_local_run_rejects_a_sparse_file_before_reading_its_bytes() { + let temporary = tempfile::tempdir().unwrap(); + let skill = temporary.path().join("my-skill"); + write_local_skill(&skill, "my-skill"); + File::create(skill.join("huge.bin")) + .unwrap() + .set_len(64 * 1024 * 1024 + 1) + .unwrap(); + + let error = local_run_error(&skill); + + assert_eq!(error["error"]["code"], "SKILL_TOO_LARGE"); +} + +#[cfg(unix)] +#[test] +fn a_local_run_rejects_non_utf8_paths_and_links() { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + use std::os::unix::fs::symlink; + + let temporary = tempfile::tempdir().unwrap(); + let non_utf8 = temporary.path().join("non-utf8"); + write_local_skill(&non_utf8, "non-utf8"); + fs::write(non_utf8.join(OsString::from_vec(vec![0xff])), "x").unwrap(); + let linked = temporary.path().join("linked"); + write_local_skill(&linked, "linked"); + symlink(linked.join("SKILL.md"), linked.join("copy.md")).unwrap(); + + let non_utf8_error = local_run_error(&non_utf8); + let link_error = local_run_error(&linked); + + assert_eq!(non_utf8_error["error"]["code"], "INVALID_SOURCE"); + assert_eq!(link_error["error"]["code"], "INVALID_SOURCE"); +} diff --git a/crates/skilld-core/src/remote.rs b/crates/skilld-core/src/remote.rs index 6ee0566f..87e748b8 100644 --- a/crates/skilld-core/src/remote.rs +++ b/crates/skilld-core/src/remote.rs @@ -56,9 +56,20 @@ pub enum RemoteSelector { impl RemoteSelector { pub fn parse(value: &str) -> Result { + if value.chars().any(is_unsafe_terminal) { + return Err(RemoteError::new( + "INVALID_SOURCE", + "the remote selector cannot contain terminal formatting characters", + )); + } let value = value.trim(); if let Some(rest) = value.strip_prefix("skilld:") { - let (owner, repository, name) = split_three(rest)?; + let (source, reference) = rest + .split_once('#') + .map_or((rest, None), |(source, value)| { + (source, Some(parse_source_ref(value))) + }); + let (owner, repository, name) = split_three(source)?; let request = SourceRequest { provider: SourceProvider::Github, owner: owner.to_owned(), @@ -66,7 +77,7 @@ impl RemoteSelector { selector: SourceSelector::NamedSkill { name: name.to_owned(), }, - r#ref: None, + r#ref: reference, }; validate_source_request(&request)?; return Ok(Self::Skilld(request)); @@ -232,6 +243,10 @@ fn validate_source_request(source: &SourceRequest) -> Result<(), RemoteError> { )); } match &source.selector { + SourceSelector::Path { path } if path.contains('#') => Err(RemoteError::new( + "INVALID_SOURCE", + "the Skill source path cannot contain #", + )), SourceSelector::Path { path } => validate_relative_path(path, 1024), SourceSelector::NamedSkill { name } => SkillName::parse(name.clone()) .map(|_| ()) @@ -243,6 +258,7 @@ fn validate_source_request(source: &SourceRequest) -> Result<(), RemoteError> { if value.is_empty() || value.len() > 255 || value.contains(['\0', '\\']) + || value.chars().any(is_unsafe_terminal) || value.starts_with('-') { return Err(RemoteError::new( @@ -1065,8 +1081,8 @@ fn validate_relative_path(value: &str, maximum: usize) -> Result<(), RemoteError let path = Path::new(value); let valid = !value.is_empty() && value.len() <= maximum - && !value.contains(['\\', '\0', ':']) - && !value.bytes().any(|byte| byte < b' ' || byte == 0x7f) + && !value.contains(['\\', ':']) + && !value.chars().any(is_unsafe_terminal) && !path.is_absolute() && path.components().all(|component| { matches!(component, Component::Normal(_)) @@ -1082,6 +1098,15 @@ fn validate_relative_path(value: &str, maximum: usize) -> Result<(), RemoteError } } +fn is_unsafe_terminal(character: char) -> bool { + let code = u32::from(character); + character.is_control() + || matches!( + code, + 0x061C | 0x200E..=0x200F | 0x202A..=0x202E | 0x2066..=0x2069 + ) +} + fn valid_path_part(part: &str) -> bool { if part.is_empty() || part.ends_with(['.', ' ']) { return false; @@ -1177,6 +1202,14 @@ mod tests { fn parses_canonical_skilld_and_github_selectors() { let skilld = RemoteSelector::parse("skilld:skilld-dev/skills/vue-testing").unwrap(); assert_eq!(skilld.canonical(), "skilld:skilld-dev/skills/vue-testing"); + let exact_skilld = RemoteSelector::parse( + "skilld:skilld-dev/skills/vue-testing#commit:0123456789abcdef0123456789abcdef01234567", + ) + .unwrap(); + assert_eq!( + exact_skilld.canonical(), + "skilld:skilld-dev/skills/vue-testing#commit:0123456789abcdef0123456789abcdef01234567" + ); let github = RemoteSelector::parse( "github:skilld-dev/skills/skills/vue-testing#commit:0123456789abcdef0123456789abcdef01234567", ) diff --git a/crates/skilld-core/tests/remote.rs b/crates/skilld-core/tests/remote.rs index ccf7c4e3..2a3a4052 100644 --- a/crates/skilld-core/tests/remote.rs +++ b/crates/skilld-core/tests/remote.rs @@ -4,13 +4,40 @@ use serde::Serialize; use sha2::{Digest, Sha256}; use skilld_core::{ ArtifactAttestation, ArtifactFile, AttestationSignature, CheckOutcome, CheckResult, - RepositoryVisibility, ResolvedSource, SignatureAlgorithm, SourceProvider, TrustedKey, - TrustedKeyStatus, TrustedRoot, TrustedRootPin, verify_artifact, verify_trusted_root, + PreparedFile, RemoteSelector, RepositoryVisibility, ResolvedSource, SignatureAlgorithm, + SourceProvider, TrustedKey, TrustedKeyStatus, TrustedRoot, TrustedRootPin, + prepare_unverified_files, verify_artifact, verify_trusted_root, }; const ROOT_DOMAIN: &[u8] = b"skilld-trusted-key-v1\0"; const ATTESTATION_DOMAIN: &[u8] = b"skilld-attestation-v1\0"; +#[test] +fn public_remote_selectors_reject_control_characters_in_branch_and_tag_refs() { + for selector in [ + "github:skilld-dev/skills/skills/example#branch:main\nforged", + "github:skilld-dev/skills/skills/example#tag:v1\tforged", + "github:skilld-dev/skills/skills/example#branch:main\u{0085}forged", + "github:skilld-dev/skills/skills/example#tag:v1\n", + ] { + let error = RemoteSelector::parse(selector).unwrap_err(); + + assert_eq!(error.code, "INVALID_SOURCE"); + } +} + +#[test] +fn public_remote_selectors_reject_bidi_formatting_characters() { + for selector in [ + "github:skilld-dev/skills/skills/\u{202e}example", + "github:skilld-dev/skills/skills/example#branch:main\u{2067}forged", + ] { + let error = RemoteSelector::parse(selector).unwrap_err(); + + assert_eq!(error.code, "INVALID_SOURCE"); + } +} + #[derive(Serialize)] #[serde(rename_all = "camelCase")] struct KeyStatement<'a> { @@ -136,6 +163,15 @@ fn attestation( archive: &[u8], files: Vec, signing_key: &SigningKey, +) -> ArtifactAttestation { + attestation_at_path(archive, files, signing_key, "skills/example") +} + +fn attestation_at_path( + archive: &[u8], + files: Vec, + signing_key: &SigningKey, + skill_path: &str, ) -> ArtifactAttestation { let content_sha256 = hex(&Sha256::digest(archive)); let artifact_id = format!("sha256:{content_sha256}"); @@ -147,7 +183,7 @@ fn attestation( visibility: RepositoryVisibility::Public, commit_sha: "0123456789abcdef0123456789abcdef01234567".to_owned(), tree_sha: "89abcdef0123456789abcdef0123456789abcdef".to_owned(), - skill_path: "skills/example".to_owned(), + skill_path: skill_path.to_owned(), }; let checks = vec![CheckResult { name: "path-policy".to_owned(), @@ -224,6 +260,119 @@ fn verifies_exact_statements_root_signatures_and_ustar_files() { assert_eq!(verified.files[0].bytes, skill); } +#[test] +fn rejects_a_hash_in_the_attested_skill_path_but_allows_it_in_supporting_files() { + let skill = b"---\nname: example\ndescription: fixture\n---\n"; + let supporting = b"# Fragment\n"; + let archive = archive(&[ + ("SKILL.md", 0o644, skill, b'0'), + ("references/topic#part.md", 0o644, supporting, b'0'), + ]); + let files = vec![ + file("SKILL.md", 0o644, skill), + file("references/topic#part.md", 0o644, supporting), + ]; + let (root, pin, signing_key) = trusted_root(); + let root = verify_trusted_root(root, &pin).unwrap(); + + let valid = verify_artifact( + attestation(&archive, files.clone(), &signing_key), + &root, + &archive, + ) + .unwrap(); + let error = verify_artifact( + attestation_at_path(&archive, files, &signing_key, "skills/example#archive"), + &root, + &archive, + ) + .unwrap_err(); + + assert_eq!(valid.files[1].path, "references/topic#part.md"); + assert_eq!(error.code, "INVALID_SOURCE"); +} + +#[test] +fn unverified_files_reject_c1_control_characters_in_paths() { + let files = vec![ + PreparedFile { + path: "SKILL.md".to_owned(), + mode: 0o644, + bytes: b"---\nname: example\n---\n".to_vec(), + }, + PreparedFile { + path: "references/api\u{0085}forged.md".to_owned(), + mode: 0o644, + bytes: b"# API\n".to_vec(), + }, + ]; + + let error = prepare_unverified_files(files).unwrap_err(); + + assert_eq!(error.code, "INVALID_PATH"); +} + +#[test] +fn unverified_files_reject_bidi_formatting_characters_in_paths() { + let files = vec![ + PreparedFile { + path: "SKILL.md".to_owned(), + mode: 0o644, + bytes: b"---\nname: example\n---\n".to_vec(), + }, + PreparedFile { + path: "references/api\u{202e}forged.md".to_owned(), + mode: 0o644, + bytes: b"# API\n".to_vec(), + }, + ]; + + let error = prepare_unverified_files(files).unwrap_err(); + + assert_eq!(error.code, "INVALID_PATH"); +} + +#[test] +fn verified_artifacts_reject_bidi_formatting_in_source_and_file_paths() { + let skill = b"---\nname: example\n---\n"; + let supporting = b"# API\n"; + let valid_archive = archive(&[("SKILL.md", 0o644, skill, b'0')]); + let bidi_archive = archive(&[ + ("SKILL.md", 0o644, skill, b'0'), + ("references/api\u{202e}forged.md", 0o644, supporting, b'0'), + ]); + let (root, pin, signing_key) = trusted_root(); + let root = verify_trusted_root(root, &pin).unwrap(); + + let source_error = verify_artifact( + attestation_at_path( + &valid_archive, + vec![file("SKILL.md", 0o644, skill)], + &signing_key, + "skills/\u{202e}example", + ), + &root, + &valid_archive, + ) + .unwrap_err(); + let file_error = verify_artifact( + attestation( + &bidi_archive, + vec![ + file("SKILL.md", 0o644, skill), + file("references/api\u{202e}forged.md", 0o644, supporting), + ], + &signing_key, + ), + &root, + &bidi_archive, + ) + .unwrap_err(); + + assert_eq!(source_error.code, "INVALID_PATH"); + assert_eq!(file_error.code, "INVALID_PATH"); +} + #[test] fn rejects_an_outer_field_that_differs_from_the_signed_statement() { let skill = b"---\nname: example\n---\n"; diff --git a/crates/skilld-native/src/embedded_skill.rs b/crates/skilld-native/src/embedded_skill.rs index 84d51cc4..d206291b 100644 --- a/crates/skilld-native/src/embedded_skill.rs +++ b/crates/skilld-native/src/embedded_skill.rs @@ -3,6 +3,7 @@ use std::path::PathBuf; use std::sync::Mutex; use skilld_command::{BundledSkillProvider, CommandError}; +use skilld_core::PreparedFile; use tempfile::TempDir; const SKILLD_SKILL: &[u8] = include_bytes!("../../../skills/skilld/SKILL.md"); @@ -20,6 +21,14 @@ impl EmbeddedSkilld { } impl BundledSkillProvider for EmbeddedSkilld { + fn skilld_run_files(&self) -> Result, CommandError> { + Ok(vec![PreparedFile { + path: "SKILL.md".to_owned(), + mode: 0o644, + bytes: SKILLD_SKILL.to_vec(), + }]) + } + fn skilld_source(&self) -> Result { let mut directory = self.directory.lock().map_err(|_| { CommandError::service("the bundled Skill workspace lock is unavailable") diff --git a/crates/skilld-native/src/main.rs b/crates/skilld-native/src/main.rs index 1c372709..050ce9c4 100644 --- a/crates/skilld-native/src/main.rs +++ b/crates/skilld-native/src/main.rs @@ -11,8 +11,9 @@ use std::sync::Arc; use embedded_skill::EmbeddedSkilld; use native_auth::NativeAccount; use skilld_command::{ - CommandError, DetectionEnvironment, Host, LocalHost, NativeRemoteConfig, OutputContext, - SkilldRemote, TargetRoots, interactive_update_requested, run_stdio_probe, run_with_output, + CommandError, CommandPlatform, DetectionEnvironment, Host, LocalHost, NativeRemoteConfig, + OutputContext, SkilldRemote, TargetRoots, interactive_update_requested, run_stdio_probe, + run_with_output, }; use skilld_core::{ InstallScope, InstallSource, SearchResponse, SearchResult, SourceProvider, SourceRequest, @@ -121,6 +122,7 @@ fn main() -> ExitCode { environment_present("NO_COLOR"), env::var("TERM").is_ok_and(|term| term.eq_ignore_ascii_case("dumb")), terminal_width(), + CommandPlatform::current(), ); let label = status::status_label(args.iter().map(|arg| arg.to_string_lossy())); let status = match label { @@ -143,6 +145,7 @@ fn run_search_output_probe() -> ExitCode { environment_present("NO_COLOR"), env::var("TERM").is_ok_and(|term| term.eq_ignore_ascii_case("dumb")), terminal_width(), + CommandPlatform::current(), ); let mut args = vec!["skilld", "search", "output"]; if env::args_os().any(|argument| argument == "--json") { diff --git a/crates/skilld-native/src/status.rs b/crates/skilld-native/src/status.rs index ca2f5226..fe5cfe09 100644 --- a/crates/skilld-native/src/status.rs +++ b/crates/skilld-native/src/status.rs @@ -188,6 +188,7 @@ where match subcommand.as_deref() { Some("search") => Some("Searching"), Some("install") => Some("Installing"), + Some("run") => Some("Loading"), Some("view") => Some("Loading"), Some("verify") => Some("Verifying"), Some("update") => Some("Updating"), @@ -259,6 +260,7 @@ impl skilld_command::OutdatedProgress for OutdatedProgressLine { #[cfg(test)] mod tests { use super::{GatedStderr, OutputContext, StatusLine, frame_line, status_label}; + use skilld_command::CommandPlatform; use std::io::Write; use std::sync::Mutex; use std::thread; @@ -359,6 +361,8 @@ mod tests { assert_eq!(status_label(args), Some("Searching")); let args = ["skilld", "install", "skilld:owner/repo/skill"]; assert_eq!(status_label(args), Some("Installing")); + let args = ["skilld", "run", "skilld:owner/repo/skill"]; + assert_eq!(status_label(args), Some("Loading")); let args = ["skilld", "list"]; assert_eq!(status_label(args), None); let args = ["skilld"]; @@ -368,7 +372,12 @@ mod tests { #[test] fn plain_contexts_get_no_status_line() { assert!(matches!( - StatusLine::for_terminal("Searching", OutputContext::Plain), + StatusLine::for_terminal( + "Searching", + OutputContext::Plain { + platform: CommandPlatform::Unix, + }, + ), line if line.is_disabled() )); assert!(matches!( @@ -376,7 +385,8 @@ mod tests { "Searching", OutputContext::HumanTerminal { width: 80, - color: true + color: true, + platform: CommandPlatform::Unix, } ), line if !line.is_disabled() diff --git a/crates/skilld-native/src/update_ui.rs b/crates/skilld-native/src/update_ui.rs index e9e09f54..cc7ea820 100644 --- a/crates/skilld-native/src/update_ui.rs +++ b/crates/skilld-native/src/update_ui.rs @@ -22,6 +22,7 @@ use ratatui::widgets::{Block, List, ListItem, ListState, Paragraph, Tabs}; use skilld_command::{CommandError, Host}; use skilld_core::{CommitHistory, UpdatePlanItem, UpdatePlanV1, UpdateRelation, UpdateRetryAfter}; use skilld_ui::spinner; +use skilld_ui::text::sanitize; use skilld_ui::time::relative_time; use unicode_width::{UnicodeWidthChar, UnicodeWidthStr}; use url::Url; @@ -1399,16 +1400,7 @@ fn short_sha(value: &str) -> &str { } fn clean_text(value: &str) -> String { - value - .chars() - .map(|character| { - if character.is_control() { - ' ' - } else { - character - } - }) - .collect::() + sanitize(value) .split_whitespace() .collect::>() .join(" ") diff --git a/crates/skilld-native/tests/cli.rs b/crates/skilld-native/tests/cli.rs index 91098d3a..1e9726db 100644 --- a/crates/skilld-native/tests/cli.rs +++ b/crates/skilld-native/tests/cli.rs @@ -153,8 +153,12 @@ 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("skilld install")); - assert!(output.lines().all(|line| line.chars().count() <= 40)); + assert!(output.contains("skilld run skilld:skilld-dev/skilld/output-probe")); + assert!( + output + .lines() + .all(|line| { line.contains("skilld run ") || line.chars().count() <= 40 }) + ); } #[test] diff --git a/crates/skilld-native/tests/update_ui.rs b/crates/skilld-native/tests/update_ui.rs index ee072600..0f7f7df5 100644 --- a/crates/skilld-native/tests/update_ui.rs +++ b/crates/skilld-native/tests/update_ui.rs @@ -134,11 +134,11 @@ fn view_reflows_at_narrow_width_and_keeps_the_cursor_in_the_viewport() { } #[test] -fn view_preserves_unicode_width_and_removes_terminal_controls() { +fn view_preserves_unicode_width_and_removes_terminal_formatting() { let model = update( Model::new(40, 12), Message::CandidatesLoaded(Ok(vec![candidate( - "技能🙂 cafe\u{301}\u{1b}[31m", + "技能🙂 cafe\u{301}\u{1b}[31m\u{202e}forged", "skilld-dev/skills", 3, )])), @@ -149,6 +149,7 @@ fn view_preserves_unicode_width_and_removes_terminal_controls() { assert!(plain.contains("技能🙂 cafe\u{301}"), "{plain:?}"); assert!(!plain.contains('\u{1b}')); + assert!(!plain.contains('\u{202e}')); assert_eq!(plain, colored); assert!(plain.lines().all(|line| UnicodeWidthStr::width(line) <= 40)); } diff --git a/crates/skilld-ui/src/text.rs b/crates/skilld-ui/src/text.rs index 096a400c..5c172f87 100644 --- a/crates/skilld-ui/src/text.rs +++ b/crates/skilld-ui/src/text.rs @@ -2,13 +2,22 @@ use unicode_width::{UnicodeWidthChar, UnicodeWidthStr}; -/// Replace terminal control characters with spaces so untrusted text can -/// never move a cursor or forge an escape sequence. +/// Whether a character can change terminal state or visual text order. +pub fn is_unsafe_terminal(character: char) -> bool { + let code = u32::from(character); + character.is_control() + || matches!( + code, + 0x061C | 0x200E..=0x200F | 0x202A..=0x202E | 0x2066..=0x2069 + ) +} + +/// Replace unsafe terminal characters with spaces. pub fn sanitize(value: &str) -> String { value .chars() .map(|character| { - if character.is_control() { + if is_unsafe_terminal(character) { ' ' } else { character @@ -129,9 +138,15 @@ mod tests { }; #[test] - fn sanitize_replaces_control_characters() { + fn sanitize_replaces_unsafe_terminal_characters() { assert_eq!(sanitize("a\u{1b}[31mb"), "a [31mb"); assert_eq!(sanitize("a\tb"), "a b"); + for character in [ + '\u{061c}', '\u{200e}', '\u{200f}', '\u{202a}', '\u{202b}', '\u{202c}', '\u{202d}', + '\u{202e}', '\u{2066}', '\u{2067}', '\u{2068}', '\u{2069}', + ] { + assert_eq!(sanitize(&format!("left{character}right")), "left right"); + } } #[test] diff --git a/docs/adr/0001-v3-product-boundaries.md b/docs/adr/0001-v3-product-boundaries.md index 6c2ca5a5..a4a4e8ae 100644 --- a/docs/adr/0001-v3-product-boundaries.md +++ b/docs/adr/0001-v3-product-boundaries.md @@ -54,4 +54,8 @@ Users may request explicit direct remote access with an unverified source status Strict CI rejects unverified remote sources. -`skilld install skilld --global` installs the skilld-maintained Skill for search and install guidance. +`skilld install skilld --global` installs the skilld-maintained Skill for search, run, and install guidance. +`skilld run ` prints a Skill for the current session. +It retains no remote Skill files after the command exits. +It creates no lockfile entry, Agent target, project file, or Skill cache. +It never executes the Skill, so the no Agent runtime boundary holds. diff --git a/docs/migrate-v2-to-v3.md b/docs/migrate-v2-to-v3.md index edc036b5..cb3e17d3 100644 --- a/docs/migrate-v2-to-v3.md +++ b/docs/migrate-v2-to-v3.md @@ -68,9 +68,15 @@ skilld auth status You only need an account for private repository access. -## 4. Install the global skilld Skill +## 4. Run the skilld Skill -Install search and install guidance for your Agent: +Ask your Agent to load search, run, and install guidance for the current session: + +```sh +skilld run skilld +``` + +Install it globally only when you want your Agent to keep it across sessions: ```sh skilld install skilld --global --agent codex @@ -84,15 +90,26 @@ You may omit `--agent` after configuring or detecting a target. v3 stores canonical project Skills under `.skills/`. It writes project state to `.skills/skilld-lock.yaml`. -Search first, then install the returned selector: +Search first, then run the returned selector: ```sh skilld search vue -skilld install skilld:skilld-dev/skills/vue +skilld run skilld:skilld-dev/skills/vue ``` Use the exact selector printed by your search result. +`skilld run` loads a transient Skill for the current Agent session. +It writes no project files or lockfile state. + +Install the Skill only when you want to keep it: + +```sh +skilld install skilld:skilld-dev/skills/vue +``` + +An install writes project files and records lockfile state. + An existing v2 Skill in an Agent target is unmanaged v3 state. v3 returns `TARGET_CONFLICT` instead of replacing it. @@ -110,7 +127,7 @@ It cannot restore a v2 lockfile. | v2 command | v3 replacement | | --- | --- | -| `skilld add ` | Run `skilld search`, then `skilld install ` | +| `skilld add ` | Run `skilld search`, then `skilld run ` to use it once, or `skilld install ` to keep it | | `skilld update [name]` | `skilld update [name]` | | `skilld info` | `skilld list`, then `skilld view ` | | `skilld login` | `skilld auth login` | @@ -138,10 +155,12 @@ Harness enforces output limits, structure checks, and atomic promotion. An Agent run does not claim that Harness checks passed. CI should remain strict and use Harness. -The `skilld install --direct` flag serves another purpose. -It reads a public GitHub repository without skilld.dev Artifact delivery. -The installed Skill receives the `unverified` source status. -It never handles private repositories. +`skilld install --direct` and `skilld run --direct` serve another purpose. +They read a public GitHub repository without skilld.dev Artifact delivery. +Explicit GitHub selectors use Artifact delivery unless you add `--direct`. +A direct install records the `unverified` source status. +A direct run reports the same status but writes no files. +Direct mode never handles private repositories. ## Release requirements diff --git a/package.json b/package.json index 625b26e5..effccbed 100644 --- a/package.json +++ b/package.json @@ -3,7 +3,7 @@ "type": "module", "version": "3.0.0-beta.1", "packageManager": "pnpm@11.21.0", - "description": "Search, install, and keep Skills current", + "description": "Search, run, install, and keep Skills current", "author": { "name": "Harlan Wilton", "email": "harlan@harlanzw.com", diff --git a/skills/skilld/SKILL.md b/skills/skilld/SKILL.md index 2022b177..429f81b6 100644 --- a/skills/skilld/SKILL.md +++ b/skills/skilld/SKILL.md @@ -1,11 +1,13 @@ --- name: skilld -description: Search, view, install, update, verify, and remove Skills with skilld CLI, including private repository access. +description: Search, run, view, install, update, verify, and remove Skills with skilld CLI, including private repository access. --- # Use skilld CLI -Use skilld CLI to search for and install Skills. +Use skilld CLI to search for, run, and install Skills. + +Run a Skill first. Install a Skill only when the user asks to keep it. ## Search for a Skill @@ -16,7 +18,7 @@ skilld search --json ``` Read `data.items` before choosing a Skill. -Use each item's `selector` for install. +Use each item's `selector` for a Skill run. Refine the query when several Skills cover different tasks. Always use `--json` when an Agent runs Skill search. @@ -25,8 +27,48 @@ If search fails, read the tagged JSON error from stderr. Use `--plain` only when another command needs stable text. Never parse formatted terminal output. +## Run a Skill + +Run the selector returned by search: + +```sh +skilld run --json +``` + +The command prints SKILL.md and writes no Skill files. +It retains no remote Skill files after the command exits. +Read the printed SKILL.md, then follow it for the current task. +Prefer `skilld run` for a one-off task. + +Read `data.files` for each supporting file's path, kind, and size. +The initial load prints no supporting file content. +Read one only when the instructions name it: + +```sh +skilld run --revision --file --json +``` + +Run the exact `data.files[].readArgv` array when possible. +It contains the source, exact revision, file path, and `--json`. +Repeat `--file` to read several files in one command. + +Check `data.files[].readable` before you ask for a file. +A file with `readable: false` never prints. +Its `kind` is `executable` or `binary`. +Tell the user the Skill needs an install to use that file. + +Report which Skill you ran and that skilld wrote no Skill files. +Read `data.sourceStatus`, `data.origin`, and `data.revision`. +A `verified` status covers where the Skill came from. +It does not cover what the instructions ask you to do. +If the status is `unverified`, tell the user before you follow the Skill. + ## Install a Skill +Install a Skill when the user wants it in every session. +Install a Skill when it must run its own script. +Ask the user before you install. An install writes files they did not request. + Install the selector returned by search into the detected Agent target: ```sh