diff --git a/Cargo.lock b/Cargo.lock index 825df740..1dbfb626 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1171,6 +1171,7 @@ dependencies = [ "deepseek-secrets", "dirs", "dotenvy", + "flate2", "futures-util", "ignore", "image", @@ -1186,9 +1187,11 @@ dependencies = [ "rustyline 15.0.0", "serde", "serde_json", + "sha2", "shellexpand", "shlex", "starlark", + "tar", "tempfile", "thiserror 2.0.17", "tiny_http", @@ -1597,6 +1600,17 @@ dependencies = [ "winapi", ] +[[package]] +name = "filetime" +version = "0.2.27" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f98844151eee8917efc50bd9e8318cb963ae8b297431495d3f758616ea5c57db" +dependencies = [ + "cfg-if", + "libc", + "libredox", +] + [[package]] name = "find-msvc-tools" version = "0.1.7" @@ -2484,6 +2498,7 @@ checksum = "3d0b95e02c851351f877147b7deea7b1afb1df71b63aa5f8270716e0c5720616" dependencies = [ "bitflags 2.10.0", "libc", + "redox_syscall 0.7.4", ] [[package]] @@ -3063,7 +3078,7 @@ checksum = "2621685985a2ebf1c516881c026032ac7deafcda1a2c9b7850dc81e3dfcb64c1" dependencies = [ "cfg-if", "libc", - "redox_syscall", + "redox_syscall 0.5.18", "smallvec", "windows-link 0.2.1", ] @@ -3458,6 +3473,15 @@ dependencies = [ "bitflags 2.10.0", ] +[[package]] +name = "redox_syscall" +version = "0.7.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f450ad9c3b1da563fb6948a8e0fb0fb9269711c9c73d9ea1de5058c79c8d643a" +dependencies = [ + "bitflags 2.10.0", +] + [[package]] name = "redox_users" version = "0.4.6" @@ -4335,6 +4359,17 @@ dependencies = [ "syn 2.0.114", ] +[[package]] +name = "tar" +version = "0.4.45" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "22692a6476a21fa75fdfc11d452fda482af402c008cdbaf3476414e122040973" +dependencies = [ + "filetime", + "libc", + "xattr", +] + [[package]] name = "tempfile" version = "3.24.0" @@ -5487,6 +5522,16 @@ version = "0.13.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ea6fc2961e4ef194dcbfe56bb845534d0dc8098940c7e5c012a258bfec6701bd" +[[package]] +name = "xattr" +version = "1.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32e45ad4206f6d2479085147f02bc2ef834ac85886624a23575ae137c8aa8156" +dependencies = [ + "libc", + "rustix 1.1.3", +] + [[package]] name = "xdg-home" version = "1.3.0" diff --git a/README.md b/README.md index 9f409554..055082dc 100644 --- a/README.md +++ b/README.md @@ -198,6 +198,23 @@ Full reference: [docs/CONFIGURATION.md](docs/CONFIGURATION.md) --- +## Publishing your own skill + +DeepSeek-TUI can install community skills directly from a GitHub repo, with no +backend service in the loop: + +1. Create a public GitHub repo with a `SKILL.md` at the root containing the + usual `---` frontmatter (`name`, `description`). +2. Multi-skill bundles use `skills//SKILL.md` instead — the installer + picks the first match and names the install after the frontmatter `name`. +3. Push to `main` (or `master`); the installer fetches + `archive/refs/heads/main.tar.gz` and falls back to `master.tar.gz`. +4. Users install via `/skill install github:/` — installs are + gated by the `[network]` policy, validated for path traversal and size, and + placed under `~/.deepseek/skills//`. +5. Submit a PR to the curated `index.json` (default registry) to make the skill + installable by name (`/skill install `) instead of the GitHub spec. + ## Documentation | Doc | Topic | diff --git a/config.example.toml b/config.example.toml index a38f73cf..0d2e719e 100644 --- a/config.example.toml +++ b/config.example.toml @@ -124,6 +124,25 @@ max_subagents = 5 # optional (1-20) # deny = [] # audit = true # one line per call to ~/.deepseek/audit.log +# ───────────────────────────────────────────────────────────────────────────────── +# Skills (#140) +# ───────────────────────────────────────────────────────────────────────────────── +# Settings for the `/skill install ` community-skill installer. +# * registry_url — curated index.json that resolves bare names to +# `github:owner/repo` specs. Override to point at +# a private fork or internal mirror. +# * max_install_size_bytes — per-skill uncompressed size cap. Tarballs that +# exceed this limit are rejected during validation. +# Default: 5 MiB. +# +# `/skill install` is gated by `[network]`. Make sure `github.com` and +# `raw.githubusercontent.com` are reachable (default `prompt` is fine — you'll +# be asked once and can persist) before running it. +# +# [skills] +# registry_url = "https://raw.githubusercontent.com/Hmbown/deepseek-skills/main/index.json" +# max_install_size_bytes = 5_242_880 + # ───────────────────────────────────────────────────────────────────────────────── # TUI # ───────────────────────────────────────────────────────────────────────────────── diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index 9e7b1884..96a5fcee 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -128,10 +128,29 @@ pub struct ConfigToml { /// to a permissive default that mirrors pre-v0.7.0 behavior. #[serde(default)] pub network: Option, + /// Community skill installer settings (#140). Mirrors + /// [`SkillsToml`] from the TUI side; the dispatcher consults + /// `registry_url` when running `deepseek skill install`. + #[serde(default)] + pub skills: Option, #[serde(flatten)] pub extras: BTreeMap, } +/// On-disk schema for the `[skills]` table (#140). See `config.example.toml` +/// for documentation. +#[derive(Debug, Clone, Serialize, Deserialize, Default)] +pub struct SkillsToml { + /// Curated registry index URL. When unset, the TUI falls back to the + /// bundled default (community-curated GitHub raw). + #[serde(default)] + pub registry_url: Option, + /// Per-skill maximum *uncompressed* size in bytes. When unset, the TUI + /// uses 5 MiB. + #[serde(default)] + pub max_install_size_bytes: Option, +} + /// On-disk schema for the `[network]` table (#135). See `config.example.toml` /// for documentation. #[derive(Debug, Clone, Serialize, Deserialize)] diff --git a/crates/tui/Cargo.toml b/crates/tui/Cargo.toml index d52ea218..5e555b70 100644 --- a/crates/tui/Cargo.toml +++ b/crates/tui/Cargo.toml @@ -57,6 +57,9 @@ zeroize = "1.8.2" ignore = "0.4" image = { version = "0.25", default-features = false, features = ["png"] } pdf-extract = "0.7" +tar = "0.4" +flate2 = "1.1" +sha2 = "0.10" [dev-dependencies] wiremock = "0.6" diff --git a/crates/tui/src/commands/mod.rs b/crates/tui/src/commands/mod.rs index 8209aa26..a9a00ae3 100644 --- a/crates/tui/src/commands/mod.rs +++ b/crates/tui/src/commands/mod.rs @@ -322,14 +322,14 @@ pub const COMMANDS: &[CommandInfo] = &[ CommandInfo { name: "skills", aliases: &[], - description: "List available skills", - usage: "/skills", + description: "List local skills (or --remote to browse the curated registry)", + usage: "/skills [--remote]", }, CommandInfo { name: "skill", aliases: &[], - description: "Activate a skill for next message", - usage: "/skill ", + description: "Activate a skill, or install/update/uninstall/trust a community skill", + usage: "/skill |update |uninstall |trust >", }, CommandInfo { name: "review", @@ -409,7 +409,7 @@ pub fn execute(cmd: &str, app: &mut App) -> CommandResult { "init" => init::init(app), // Skills commands - "skills" => skills::list_skills(app), + "skills" => skills::list_skills(app, arg), "skill" => skills::run_skill(app, arg), "review" => review::review(app, arg), diff --git a/crates/tui/src/commands/skills.rs b/crates/tui/src/commands/skills.rs index 913583df..4ae9369b 100644 --- a/crates/tui/src/commands/skills.rs +++ b/crates/tui/src/commands/skills.rs @@ -2,7 +2,12 @@ use std::fmt::Write; +use crate::network_policy::NetworkPolicy; use crate::skills::SkillRegistry; +use crate::skills::install::{ + self, DEFAULT_MAX_SIZE_BYTES, DEFAULT_REGISTRY_URL, InstallOutcome, InstallSource, + RegistryFetchResult, UpdateResult, +}; use crate::tui::app::App; use crate::tui::history::HistoryCell; @@ -21,8 +26,18 @@ fn render_skill_warnings(registry: &SkillRegistry) -> String { out } -/// List all available skills -pub fn list_skills(app: &mut App) -> CommandResult { +/// List all available skills. Pass `--remote` (or `remote`) to fetch the +/// curated registry instead of scanning the local skills directory. +pub fn list_skills(app: &mut App, arg: Option<&str>) -> CommandResult { + if let Some(arg) = arg { + let trimmed = arg.trim(); + if trimmed == "--remote" || trimmed == "remote" { + return list_remote_skills(app); + } + if !trimmed.is_empty() { + return CommandResult::error("Usage: /skills [--remote]"); + } + } let skills_dir = app.skills_dir.clone(); let registry = SkillRegistry::discover(&skills_dir); let warnings = render_skill_warnings(®istry); @@ -61,15 +76,35 @@ pub fn list_skills(app: &mut App) -> CommandResult { CommandResult::message(output) } -/// Run a specific skill - activates skill for next user message +/// Run a specific skill — activates skill for next user message, or +/// dispatches a sub-command (`install`, `update`, `uninstall`, `trust`). pub fn run_skill(app: &mut App, name: Option<&str>) -> CommandResult { - let name = match name { + let raw = match name { Some(n) => n.trim(), None => { - return CommandResult::error("Usage: /skill "); + return CommandResult::error( + "Usage: /skill \n\nSubcommands:\n /skill install >\n /skill update \n /skill uninstall \n /skill trust ", + ); } }; + // Sub-command dispatch happens before the activation path so users can't + // accidentally activate a skill literally named "install". + let mut iter = raw.splitn(2, char::is_whitespace); + let head = iter.next().unwrap_or("").trim(); + let rest = iter.next().unwrap_or("").trim(); + match head { + "install" => return install_skill(app, rest), + "update" => return update_skill(app, rest), + "uninstall" => return uninstall_skill(app, rest), + "trust" => return trust_skill(app, rest), + _ => {} + } + + activate_skill(app, raw) +} + +fn activate_skill(app: &mut App, name: &str) -> CommandResult { // `/skill new` is a friendly alias for `/skill skill-creator`. let name = if name == "new" { "skill-creator" } else { name }; @@ -111,6 +146,214 @@ pub fn run_skill(app: &mut App, name: Option<&str>) -> CommandResult { } } +// ─── /skill install ──────────────────────────────────────────────────────── + +fn install_skill(app: &mut App, spec: &str) -> CommandResult { + if spec.is_empty() { + return CommandResult::error( + "Usage: /skill install >", + ); + } + let source = match InstallSource::parse(spec) { + Ok(s) => s, + Err(err) => return CommandResult::error(format!("Invalid install source: {err}")), + }; + let skills_dir = app.skills_dir.clone(); + let (network, max_size, registry_url) = installer_settings(app); + + let outcome = run_async(async move { + install::install_with_registry( + source, + &skills_dir, + max_size, + &network, + false, + ®istry_url, + ) + .await + }); + + match outcome { + Ok(InstallOutcome::Installed(installed)) => { + let path_str = path_or_default(&installed.path); + CommandResult::message(format!( + "Installed skill '{}' from {}.\nLocation: {}\n\nRun /skills to see it in the list.", + installed.name, spec, path_str + )) + } + Ok(InstallOutcome::NeedsApproval(host)) => { + CommandResult::error(needs_approval_message(&host)) + } + Ok(InstallOutcome::NetworkDenied(host)) => { + CommandResult::error(network_denied_message(&host)) + } + Err(err) => CommandResult::error(format!("Install failed: {err:#}")), + } +} + +// ─── /skill update ───────────────────────────────────────────────────────── + +fn update_skill(app: &mut App, name: &str) -> CommandResult { + if name.is_empty() { + return CommandResult::error("Usage: /skill update "); + } + let skills_dir = app.skills_dir.clone(); + let (network, max_size, registry_url) = installer_settings(app); + let owned_name = name.to_string(); + let outcome = run_async(async move { + install::update_with_registry(&owned_name, &skills_dir, max_size, &network, ®istry_url) + .await + }); + + match outcome { + Ok(UpdateResult::NoChange) => { + CommandResult::message(format!("Skill '{name}': no upstream change.")) + } + Ok(UpdateResult::Updated(installed)) => CommandResult::message(format!( + "Skill '{}' updated. Location: {}", + installed.name, + path_or_default(&installed.path) + )), + Ok(UpdateResult::NeedsApproval(host)) => { + CommandResult::error(needs_approval_message(&host)) + } + Ok(UpdateResult::NetworkDenied(host)) => { + CommandResult::error(network_denied_message(&host)) + } + Err(err) => CommandResult::error(format!("Update failed: {err:#}")), + } +} + +// ─── /skill uninstall ────────────────────────────────────────────────────── + +fn uninstall_skill(app: &mut App, name: &str) -> CommandResult { + if name.is_empty() { + return CommandResult::error("Usage: /skill uninstall "); + } + match install::uninstall(name, &app.skills_dir) { + Ok(()) => CommandResult::message(format!("Removed skill '{name}'.")), + Err(err) => CommandResult::error(format!("Uninstall failed: {err:#}")), + } +} + +// ─── /skill trust ────────────────────────────────────────────────────────── + +fn trust_skill(app: &mut App, name: &str) -> CommandResult { + if name.is_empty() { + return CommandResult::error("Usage: /skill trust "); + } + match install::trust(name, &app.skills_dir) { + Ok(()) => CommandResult::message(format!( + "Marked skill '{name}' as trusted. Tools that consult the .trusted marker may now invoke its scripts/." + )), + Err(err) => CommandResult::error(format!("Trust failed: {err:#}")), + } +} + +// ─── /skills --remote ────────────────────────────────────────────────────── + +/// List skills available in the configured curated registry. +pub fn list_remote_skills(app: &mut App) -> CommandResult { + let (network, _max_size, registry_url) = installer_settings(app); + let registry = run_async(async move { install::fetch_registry(&network, ®istry_url).await }); + match registry { + Ok(RegistryFetchResult::Loaded(doc)) => { + if doc.skills.is_empty() { + return CommandResult::message("Registry is empty."); + } + let mut out = format!("Available remote skills ({}):\n", doc.skills.len()); + out.push_str("─────────────────────────────\n"); + for (name, entry) in &doc.skills { + let _ = writeln!( + out, + " {name} — {} (source: {})", + entry.description.clone().unwrap_or_default(), + entry.source + ); + } + let _ = write!(out, "\nInstall with: /skill install "); + CommandResult::message(out) + } + Ok(RegistryFetchResult::NeedsApproval(host)) => { + CommandResult::error(needs_approval_message(&host)) + } + Ok(RegistryFetchResult::Denied(host)) => { + CommandResult::error(network_denied_message(&host)) + } + Err(err) => CommandResult::error(format!("Failed to fetch registry: {err:#}")), + } +} + +// ─── helpers ─────────────────────────────────────────────────────────────── + +/// Read the active config knobs for the installer. +/// +/// We load `Config::load` on demand because [`App`] does not carry a `Config` +/// field — and loading is cheap (small TOML file) compared to the network +/// round-trip the install/update operation will incur next. If the config +/// fails to parse, we fall back to defaults so the user still gets a +/// network-gated install rather than a silent crash. +fn installer_settings(_app: &App) -> (NetworkPolicy, u64, String) { + let cfg = crate::config::Config::load(None, None).unwrap_or_default(); + let network = cfg + .network + .clone() + .map(|policy| policy.into_runtime()) + .unwrap_or_default(); + let skills_cfg = cfg.skills.as_ref(); + let max_size = skills_cfg + .and_then(|s| s.max_install_size_bytes) + .unwrap_or(DEFAULT_MAX_SIZE_BYTES); + let registry_url = skills_cfg + .and_then(|s| s.registry_url.clone()) + .unwrap_or_else(|| DEFAULT_REGISTRY_URL.to_string()); + (network, max_size, registry_url) +} + +fn run_async(future: F) -> T +where + F: std::future::Future, +{ + // We're on the TUI's thread, which is part of the multi-threaded runtime. + // `block_in_place` + `Handle::current().block_on` is the pattern used by + // `commands/cycle.rs` to bridge sync slash-command handlers back into the + // async ecosystem. + tokio::task::block_in_place(|| tokio::runtime::Handle::current().block_on(future)) +} + +fn path_or_default(path: &std::path::Path) -> String { + path.file_name() + .map(|n| { + // Display with parent so the user sees the full skill location. + // We intentionally use `display()` here because it's just for + // user-facing output, not for path comparisons. + let parent = path + .parent() + .map(|p| p.display().to_string()) + .unwrap_or_default(); + if parent.is_empty() { + n.to_string_lossy().to_string() + } else { + format!("{parent}/{}", n.to_string_lossy()) + } + }) + .unwrap_or_else(|| path.display().to_string()) +} + +fn needs_approval_message(host: &str) -> String { + format!( + "Network policy requires approval for {host}.\n\ + Add it to your allow list with `/network allow {host}` (or set [network].default = \"allow\" in ~/.deepseek/config.toml), then retry." + ) +} + +fn network_denied_message(host: &str) -> String { + format!( + "Network policy denied access to {host}.\n\ + Remove the deny entry from ~/.deepseek/config.toml under [network] or contact your administrator." + ) +} + #[cfg(test)] mod tests { use super::*; @@ -150,7 +393,7 @@ mod tests { fn test_list_skills_empty_directory() { let tmpdir = TempDir::new().unwrap(); let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = list_skills(&mut app); + let result = list_skills(&mut app, None); assert!(result.message.is_some()); let msg = result.message.unwrap(); assert!(msg.contains("No skills found")); @@ -166,13 +409,32 @@ mod tests { "---\nname: test-skill\ndescription: A test skill\n---\nDo something", ); let mut app = create_test_app_with_tmpdir(&tmpdir); - let result = list_skills(&mut app); + let result = list_skills(&mut app, None); assert!(result.message.is_some()); let msg = result.message.unwrap(); assert!(msg.contains("Available skills")); assert!(msg.contains("/test-skill")); } + #[test] + fn test_skill_subcommand_dispatch_install_usage() { + let tmpdir = TempDir::new().unwrap(); + let mut app = create_test_app_with_tmpdir(&tmpdir); + // Empty install spec → usage hint, not invalid-source error. + let result = run_skill(&mut app, Some("install")); + let msg = result.message.unwrap(); + assert!(msg.contains("/skill install"), "got: {msg}"); + } + + #[test] + fn test_skill_subcommand_dispatch_uninstall_missing() { + let tmpdir = TempDir::new().unwrap(); + let mut app = create_test_app_with_tmpdir(&tmpdir); + let result = run_skill(&mut app, Some("uninstall absent-skill")); + let msg = result.message.unwrap(); + assert!(msg.contains("not installed"), "got: {msg}"); + } + #[test] fn test_run_skill_without_name() { let tmpdir = TempDir::new().unwrap(); diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index 7b0a7275..72124bd0 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -429,6 +429,43 @@ pub struct Config { /// to a permissive default that mirrors pre-v0.7.0 behavior. #[serde(default)] pub network: Option, + + /// Community skill installer settings (#140). When absent, installer + /// commands fall back to the bundled defaults + /// ([`crate::skills::install::DEFAULT_REGISTRY_URL`] + + /// [`crate::skills::install::DEFAULT_MAX_SIZE_BYTES`]). + #[serde(default)] + pub skills: Option, +} + +/// `[skills]` table — knobs for the community-skill installer. +#[derive(Debug, Clone, Deserialize, Default)] +pub struct SkillsConfig { + /// Curated registry index. `/skill install ` looks up the spec here. + /// Defaults to [`crate::skills::install::DEFAULT_REGISTRY_URL`]. + #[serde(default)] + pub registry_url: Option, + /// Per-skill maximum *uncompressed* size in bytes. Tarballs that exceed + /// this limit are rejected during validation. Defaults to 5 MiB. + #[serde(default)] + pub max_install_size_bytes: Option, +} + +impl SkillsConfig { + /// Resolve the registry URL with the bundled default. + #[must_use] + pub fn registry_url(&self) -> String { + self.registry_url + .clone() + .unwrap_or_else(|| crate::skills::install::DEFAULT_REGISTRY_URL.to_string()) + } + + /// Resolve the max install size with the bundled default. + #[must_use] + pub fn max_install_size_bytes(&self) -> u64 { + self.max_install_size_bytes + .unwrap_or(crate::skills::install::DEFAULT_MAX_SIZE_BYTES) + } } /// `[network]` table — mirrors `deepseek_config::NetworkPolicyToml` so the live @@ -1384,6 +1421,7 @@ fn merge_config(base: Config, override_cfg: Config) -> Config { features: merge_features(base.features, override_cfg.features), notifications: override_cfg.notifications.or(base.notifications), network: override_cfg.network.or(base.network), + skills: override_cfg.skills.or(base.skills), } } diff --git a/crates/tui/src/skills/install.rs b/crates/tui/src/skills/install.rs new file mode 100644 index 00000000..fe55823c --- /dev/null +++ b/crates/tui/src/skills/install.rs @@ -0,0 +1,1116 @@ +//! Community-skill installer (#140). +//! +//! Pulls user-authored skills from GitHub or direct tarball URLs, validates them +//! against a path-traversal- and size-bounded extractor, and writes them into +//! `//`. No backend service, no auto-execution: every install +//! is gated by the per-domain [`crate::network_policy::NetworkPolicy`] and +//! validation rejects any tarball entry that escapes the destination directory. +//! +//! Public surface: +//! +//! * [`InstallSource`] — `github:owner/repo`, raw URL, or curated registry +//! name. Parsed from a single string with [`InstallSource::parse`]. +//! * [`install`] / [`update`] / [`uninstall`] — async install, atomic update, +//! and clean uninstall. All three preserve a `.installed-from` marker so the +//! bundled `skill-creator` (which lacks the marker) is never touched. +//! * [`InstallOutcome`] — `Installed` / `NeedsApproval(host)` / +//! `NetworkDenied(host)`. The `NeedsApproval` variant is returned without +//! side effects so the caller (slash-command, runtime API, etc.) can route +//! through its own approval flow. +//! +//! # Hard rules +//! +//! * Validation extracts to a temp directory first. The destination path is +//! only created (via atomic rename) once the tarball clears every check. +//! Half-installed skills can never appear on disk. +//! * Path traversal rejection covers both `..` segments and absolute paths. +//! Symlinks inside the archive are also rejected — there's no use case for +//! them in a SKILL.md bundle and they're a notorious foothold for escape. +//! * No `+x` is granted on extracted files. The optional `/skill trust ` +//! command writes a `.trusted` marker; tool-execution gating is a separate +//! concern that lives next to the tool registry. + +use std::fs; +use std::io::{Read, Write}; +use std::path::{Component, Path, PathBuf}; + +use anyhow::{Context, Result, bail}; +use flate2::read::GzDecoder; +use serde::Deserialize; +use sha2::{Digest, Sha256}; +use thiserror::Error; + +use crate::network_policy::{Decision, NetworkPolicy, host_from_url}; + +/// Default registry. Falls back to a community-curated `index.json` hosted on +/// GitHub raw; users can override via `[skills] registry_url` in config.toml. +pub const DEFAULT_REGISTRY_URL: &str = + "https://raw.githubusercontent.com/Hmbown/deepseek-skills/main/index.json"; + +/// Default per-skill size cap (5 MiB). Honored at unpack time so a malicious +/// gzip bomb can't blow up RAM. +pub const DEFAULT_MAX_SIZE_BYTES: u64 = 5 * 1024 * 1024; + +/// File written under each installed skill so [`update`] / [`uninstall`] can +/// recover the original [`InstallSource`] without re-parsing user input. +pub const INSTALLED_FROM_MARKER: &str = ".installed-from"; + +/// File written under each trusted skill. Currently advisory (the install path +/// never auto-runs anything) — the runtime tool-invocation gate consults this +/// marker before executing scripts that ship with the skill. +pub const TRUSTED_MARKER: &str = ".trusted"; + +// ───────────────────────────────────────────────────────────────────────────── +// Source parsing +// ───────────────────────────────────────────────────────────────────────────── + +/// Where a skill is being installed from. See [`InstallSource::parse`] for the +/// accepted spec syntax. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum InstallSource { + /// `github:owner/repo`. Resolved to + /// `https://github.com///archive/refs/heads/main.tar.gz` + /// with a `master.tar.gz` fallback on 404. + GitHubRepo(String), + /// Raw `http(s)://…` tarball URL. Used as-is. + DirectUrl(String), + /// Curated registry lookup key. Looked up via the configured `registry_url`. + Registry(String), +} + +impl InstallSource { + /// Parse a user-supplied spec. Empty / whitespace-only input is rejected. + /// + /// * `github:owner/repo` → [`InstallSource::GitHubRepo`] + /// * `http://` or `https://` prefix → [`InstallSource::DirectUrl`] + /// * anything else → [`InstallSource::Registry`] + pub fn parse(spec: &str) -> Result { + let trimmed = spec.trim(); + if trimmed.is_empty() { + bail!("install source must not be empty"); + } + if let Some(rest) = trimmed.strip_prefix("github:") { + let rest = rest.trim(); + // Reject obviously bogus values up front. We intentionally accept + // case-insensitive owner/repo so `github:Hmbown/Foo` works. + let (owner, repo) = rest.split_once('/').with_context(|| { + format!("github source must be 'github:owner/repo' (got {spec})") + })?; + let owner = owner.trim(); + let repo = repo.trim().trim_end_matches('/'); + if owner.is_empty() || repo.is_empty() { + bail!("github source must be 'github:owner/repo' (got {spec})"); + } + if owner.contains('/') || repo.contains('/') { + bail!("github source must be 'github:owner/repo' (got {spec})"); + } + return Ok(Self::GitHubRepo(format!("{owner}/{repo}"))); + } + if trimmed.starts_with("https://") || trimmed.starts_with("http://") { + return Ok(Self::DirectUrl(trimmed.to_string())); + } + Ok(Self::Registry(trimmed.to_string())) + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Outcome / result types +// ───────────────────────────────────────────────────────────────────────────── + +/// Outcome of an install attempt. +#[derive(Debug)] +pub enum InstallOutcome { + /// The skill was installed (or already present and idempotent). + Installed(InstalledSkill), + /// The host requires user approval before the install can proceed. The + /// caller should surface this through whatever approval pathway it has and + /// retry once approved (typically by adding the host to the policy's + /// allow list). + NeedsApproval(String), + /// The host is denied by network policy. The install is aborted. + NetworkDenied(String), +} + +/// Metadata for a successfully installed skill. +#[derive(Debug, Clone)] +pub struct InstalledSkill { + /// Skill name (taken from SKILL.md frontmatter). + pub name: String, + /// Final on-disk path: `//`. + pub path: PathBuf, + /// SHA-256 over the downloaded tarball bytes. Used by [`update`] to detect + /// upstream changes without re-extracting; also surfaced for telemetry / + /// future signature-verification work. + #[allow(dead_code)] + pub source_checksum: String, +} + +/// Result of an [`update`] call. +#[derive(Debug)] +pub enum UpdateResult { + /// Upstream tarball is byte-identical to the on-disk checksum; no action. + NoChange, + /// Upstream changed and the on-disk install was atomically replaced. + Updated(InstalledSkill), + /// Network policy short-circuited the update. Same semantics as + /// [`InstallOutcome::NeedsApproval`]. + NeedsApproval(String), + /// Network policy denied the update. + NetworkDenied(String), +} + +/// Errors that can happen during install. Most variants are flattened into +/// `anyhow::Error` at the public boundary; this enum is used internally so +/// tests can pattern-match without parsing strings. +#[derive(Debug, Error)] +pub enum InstallError { + #[error("entry escapes destination directory: {0}")] + PathTraversal(String), + #[error("entry is too large; uncompressed total would exceed {limit} bytes")] + OversizedTarball { limit: u64 }, + #[error("missing SKILL.md in archive")] + MissingSkillMd, + #[error("SKILL.md frontmatter missing required field: {0}")] + MissingFrontmatterField(&'static str), + #[error("symlinks are not allowed in skill tarballs")] + SymlinkRejected, + #[error("skill '{0}' is already installed; use update or remove it first")] + AlreadyInstalled(String), + #[error("skill '{0}' was not installed via /skill install (no .installed-from marker)")] + NotInstalledHere(String), +} + +// ───────────────────────────────────────────────────────────────────────────── +// Public API +// ───────────────────────────────────────────────────────────────────────────── + +/// Install a community skill into `skills_dir`. +/// +/// Steps: +/// +/// 1. Resolve `source` to one or more candidate URLs (GitHub adds a +/// `master` fallback after `main`). +/// 2. Consult `network` for the host. `Allow` proceeds; `Deny` returns +/// [`InstallOutcome::NetworkDenied`]; `Prompt` returns +/// [`InstallOutcome::NeedsApproval`] without touching disk. +/// 3. Stream the tarball into a tempfile (capped at `max_size`). +/// 4. Validate the archive (path-traversal, size, no symlinks, SKILL.md present +/// with required frontmatter fields) into a sibling `.tmp/` directory. +/// 5. Atomic-rename `.tmp/` → `/`. +/// 6. Write `.installed-from` and return [`InstalledSkill`]. +/// +/// `update = false` rejects an existing destination. Pass `update = true` +/// from [`update`] to allow replacement. +/// +/// Convenience wrapper over [`install_with_registry`] that uses the bundled +/// [`DEFAULT_REGISTRY_URL`]. Public for downstream consumers (tests, runtime +/// API) even though the slash-command path always goes through +/// [`install_with_registry`] so the user's configured registry wins. +#[allow(dead_code)] +pub async fn install( + source: InstallSource, + skills_dir: &Path, + max_size: u64, + network: &NetworkPolicy, + update: bool, +) -> Result { + install_with_registry( + source, + skills_dir, + max_size, + network, + update, + DEFAULT_REGISTRY_URL, + ) + .await +} + +/// Same as [`install`] but lets the caller override the registry URL. Useful +/// for tests; the slash-command path always uses the configured registry. +pub async fn install_with_registry( + source: InstallSource, + skills_dir: &Path, + max_size: u64, + network: &NetworkPolicy, + update: bool, + registry_url: &str, +) -> Result { + let urls = candidate_urls(&source, network, registry_url).await?; + let urls = match urls { + UrlResolution::Resolved(urls) => urls, + UrlResolution::NeedsApproval(host) => return Ok(InstallOutcome::NeedsApproval(host)), + UrlResolution::Denied(host) => return Ok(InstallOutcome::NetworkDenied(host)), + }; + + // Try each URL in order — GitHub returns 404 for `main` on master-only + // repos, and we don't want to fail the install on that. + let (bytes, source_url) = match download_first_success(&urls, network, max_size).await? { + DownloadOutcome::Bytes { bytes, url } => (bytes, url), + DownloadOutcome::NeedsApproval(host) => return Ok(InstallOutcome::NeedsApproval(host)), + DownloadOutcome::Denied(host) => return Ok(InstallOutcome::NetworkDenied(host)), + }; + + // Compute a checksum before unpacking so [`update`] can detect upstream + // no-op changes without redoing the extract. + let mut hasher = Sha256::new(); + hasher.update(&bytes); + let checksum = format!("{:x}", hasher.finalize()); + + let staged = stage_tarball(&bytes, skills_dir, max_size)?; + + // Move the staged dir into its final location. If `update` is set and the + // destination exists, replace it; otherwise reject. + let final_path = skills_dir.join(&staged.skill_name); + if final_path.exists() { + if !update { + // Clean up the staging dir before returning the error. + let _ = fs::remove_dir_all(&staged.staged_path); + return Err(InstallError::AlreadyInstalled(staged.skill_name).into()); + } + // Best-effort backup-then-replace; on failure we restore the original. + let backup = skills_dir.join(format!("{}.bak", staged.skill_name)); + // If a previous failed update left a stale `.bak/`, drop it. + if backup.exists() { + fs::remove_dir_all(&backup).ok(); + } + fs::rename(&final_path, &backup).with_context(|| { + format!( + "failed to backup existing skill at {}", + final_path.display() + ) + })?; + if let Err(err) = fs::rename(&staged.staged_path, &final_path) { + // Roll back: restore the backup so the user isn't left with an + // empty skill directory. + fs::rename(&backup, &final_path).ok(); + return Err(err).context("failed to install staged skill"); + } + fs::remove_dir_all(&backup).ok(); + } else { + if let Some(parent) = final_path.parent() { + fs::create_dir_all(parent).with_context(|| { + format!("failed to create skills directory {}", parent.display()) + })?; + } + fs::rename(&staged.staged_path, &final_path).context("failed to install staged skill")?; + } + + // Write the marker last so a partial install never leaves a stale + // .installed-from on disk. + let marker_body = serde_json::json!({ + "spec": source_spec_string(&source), + "url": source_url, + "checksum": checksum, + }) + .to_string(); + fs::write(final_path.join(INSTALLED_FROM_MARKER), marker_body).with_context(|| { + format!( + "failed to write {} marker for skill {}", + INSTALLED_FROM_MARKER, staged.skill_name + ) + })?; + + Ok(InstallOutcome::Installed(InstalledSkill { + name: staged.skill_name, + path: final_path, + source_checksum: checksum, + })) +} + +/// Re-fetch a previously installed skill and replace it on disk if the +/// upstream tarball changed. +/// +/// Reads `.installed-from` to recover the original [`InstallSource`], so +/// a skill installed via `/skill install github:foo/bar` can be updated via +/// `/skill update bar` without the user re-typing the spec. +/// +/// Convenience wrapper over [`update_with_registry`]. +#[allow(dead_code)] +pub async fn update( + name: &str, + skills_dir: &Path, + max_size: u64, + network: &NetworkPolicy, +) -> Result { + update_with_registry(name, skills_dir, max_size, network, DEFAULT_REGISTRY_URL).await +} + +/// Same as [`update`] but lets the caller override the registry URL. +pub async fn update_with_registry( + name: &str, + skills_dir: &Path, + max_size: u64, + network: &NetworkPolicy, + registry_url: &str, +) -> Result { + let target = skills_dir.join(name); + let marker_path = target.join(INSTALLED_FROM_MARKER); + if !marker_path.exists() { + return Err(InstallError::NotInstalledHere(name.to_string()).into()); + } + let marker_body = fs::read_to_string(&marker_path) + .with_context(|| format!("failed to read {}", marker_path.display()))?; + let marker: InstalledFromMarker = serde_json::from_str(&marker_body) + .with_context(|| format!("malformed {} for {name}", INSTALLED_FROM_MARKER))?; + + // Re-resolve the URL, taking the existing checksum as a short-circuit hint: + // we still hit the network so the user gets a useful "no upstream change" + // signal, but we skip the unpack step if the bytes match. + let source = InstallSource::parse(&marker.spec)?; + let urls = match candidate_urls(&source, network, registry_url).await? { + UrlResolution::Resolved(urls) => urls, + UrlResolution::NeedsApproval(host) => return Ok(UpdateResult::NeedsApproval(host)), + UrlResolution::Denied(host) => return Ok(UpdateResult::NetworkDenied(host)), + }; + let (bytes, _url) = match download_first_success(&urls, network, max_size).await? { + DownloadOutcome::Bytes { bytes, url } => (bytes, url), + DownloadOutcome::NeedsApproval(host) => return Ok(UpdateResult::NeedsApproval(host)), + DownloadOutcome::Denied(host) => return Ok(UpdateResult::NetworkDenied(host)), + }; + + let mut hasher = Sha256::new(); + hasher.update(&bytes); + let checksum = format!("{:x}", hasher.finalize()); + if checksum == marker.checksum { + return Ok(UpdateResult::NoChange); + } + + // Bytes changed — fall back to the regular install path with `update = true` + // so we get the same atomic-replace semantics. + let outcome = + install_with_registry(source, skills_dir, max_size, network, true, registry_url).await?; + match outcome { + InstallOutcome::Installed(installed) => Ok(UpdateResult::Updated(installed)), + InstallOutcome::NeedsApproval(host) => Ok(UpdateResult::NeedsApproval(host)), + InstallOutcome::NetworkDenied(host) => Ok(UpdateResult::NetworkDenied(host)), + } +} + +/// Remove a community-installed skill. +/// +/// Refuses to touch any directory that doesn't carry the `.installed-from` +/// marker — that's our cue that it's user-owned and not a system skill. +pub fn uninstall(name: &str, skills_dir: &Path) -> Result<()> { + let target = skills_dir.join(name); + if !target.exists() { + bail!("skill '{name}' is not installed at {}", target.display()); + } + if !target.join(INSTALLED_FROM_MARKER).exists() { + return Err(InstallError::NotInstalledHere(name.to_string()).into()); + } + fs::remove_dir_all(&target) + .with_context(|| format!("failed to remove {}", target.display()))?; + Ok(()) +} + +/// Mark a community-installed skill as trusted. Currently a marker file only; +/// callers that wire tool execution against `/scripts/` consult the file +/// before invoking anything. No-op if already trusted. +/// +/// Refuses to mark system skills (no `.installed-from`) so the bundled +/// `skill-creator` doesn't accidentally inherit elevated tool privileges. +pub fn trust(name: &str, skills_dir: &Path) -> Result<()> { + let target = skills_dir.join(name); + if !target.exists() { + bail!("skill '{name}' is not installed at {}", target.display()); + } + if !target.join(INSTALLED_FROM_MARKER).exists() { + return Err(InstallError::NotInstalledHere(name.to_string()).into()); + } + let marker = target.join(TRUSTED_MARKER); + if !marker.exists() { + fs::write( + &marker, + "Skill scripts/ are user-trusted. Delete this file to revoke.\n", + ) + .with_context(|| format!("failed to write {}", marker.display()))?; + } + Ok(()) +} + +/// Fetch the curated registry and return the parsed entries. +/// +/// Honours `network` (skipping the call entirely on Deny / Prompt). +pub async fn fetch_registry( + network: &NetworkPolicy, + registry_url: &str, +) -> Result { + let host = match host_from_url(registry_url) { + Some(host) => host, + None => bail!("invalid registry url: {registry_url}"), + }; + match network.decide(&host) { + Decision::Allow => {} + Decision::Deny => return Ok(RegistryFetchResult::Denied(host)), + Decision::Prompt => return Ok(RegistryFetchResult::NeedsApproval(host)), + } + let body = reqwest::get(registry_url) + .await + .with_context(|| format!("failed to fetch registry {registry_url}"))? + .error_for_status() + .with_context(|| format!("registry {registry_url} returned an error status"))? + .text() + .await + .with_context(|| format!("failed to read registry body from {registry_url}"))?; + let parsed: RegistryDocument = serde_json::from_str(&body) + .with_context(|| format!("failed to parse registry json from {registry_url}"))?; + Ok(RegistryFetchResult::Loaded(parsed)) +} + +// ───────────────────────────────────────────────────────────────────────────── +// Internal helpers +// ───────────────────────────────────────────────────────────────────────────── + +#[derive(Debug, Deserialize)] +struct InstalledFromMarker { + spec: String, + #[serde(default)] + checksum: String, +} + +/// Curated-registry document. The shape is intentionally minimal so adding +/// optional metadata later (homepage, version, signature) is forward-compatible. +#[derive(Debug, Clone, Deserialize)] +pub struct RegistryDocument { + /// Map of skill name → entry. + #[serde(default)] + pub skills: std::collections::BTreeMap, +} + +/// One row in the curated registry. `description` is optional so old indices +/// keep parsing. +#[derive(Debug, Clone, Deserialize)] +pub struct RegistryEntry { + /// Source spec (e.g. `github:owner/repo`). + pub source: String, + /// Optional human-readable description. + #[serde(default)] + pub description: Option, +} + +/// Successful registry fetch result. Same shape as [`InstallOutcome`] for the +/// network-policy outcomes so the caller can drop directly into approval flow. +#[derive(Debug)] +pub enum RegistryFetchResult { + Loaded(RegistryDocument), + NeedsApproval(String), + Denied(String), +} + +enum UrlResolution { + Resolved(Vec), + NeedsApproval(String), + Denied(String), +} + +enum DownloadOutcome { + Bytes { bytes: Vec, url: String }, + NeedsApproval(String), + Denied(String), +} + +/// Resolve the source spec into one or more candidate URLs to try in order. +async fn candidate_urls( + source: &InstallSource, + network: &NetworkPolicy, + registry_url: &str, +) -> Result { + match source { + InstallSource::GitHubRepo(repo) => { + // GitHub's archive endpoint lives on `codeload.github.com` after + // the redirect, but the public URL we hit is `github.com`. Both + // typically appear in user allow lists; we check the canonical + // host. + Ok(UrlResolution::Resolved(vec![ + format!("https://github.com/{repo}/archive/refs/heads/main.tar.gz"), + format!("https://github.com/{repo}/archive/refs/heads/master.tar.gz"), + ])) + } + InstallSource::DirectUrl(url) => Ok(UrlResolution::Resolved(vec![url.clone()])), + InstallSource::Registry(name) => { + match fetch_registry(network, registry_url).await? { + RegistryFetchResult::Loaded(doc) => { + let entry = doc + .skills + .get(name) + .with_context(|| format!("skill '{name}' not found in registry"))? + .clone(); + let inner = InstallSource::parse(&entry.source).with_context(|| { + format!( + "registry entry for '{name}' has invalid source: {}", + entry.source + ) + })?; + // Recurse only one level — registry pointing at registry is + // disallowed to avoid cycles. + if matches!(inner, InstallSource::Registry(_)) { + bail!("registry entry for '{name}' must not point to another registry"); + } + // Reuse this function for the inner source so GitHub fallback + // still applies. + Box::pin(candidate_urls(&inner, network, registry_url)).await + } + RegistryFetchResult::NeedsApproval(host) => Ok(UrlResolution::NeedsApproval(host)), + RegistryFetchResult::Denied(host) => Ok(UrlResolution::Denied(host)), + } + } + } +} + +/// Download the first URL whose host the policy allows and which returns 2xx. +/// Returns `NeedsApproval` if every candidate hit `Prompt`, or `Denied` if every +/// candidate was denied. +async fn download_first_success( + urls: &[String], + network: &NetworkPolicy, + max_size: u64, +) -> Result { + let mut last_status: Option = None; + let mut prompt_host: Option = None; + let mut denied_host: Option = None; + for url in urls { + let host = match host_from_url(url) { + Some(h) => h, + None => bail!("invalid download url: {url}"), + }; + match network.decide(&host) { + Decision::Allow => {} + Decision::Deny => { + denied_host.get_or_insert(host); + continue; + } + Decision::Prompt => { + prompt_host.get_or_insert(host); + continue; + } + } + match download_with_cap(url, max_size).await? { + DownloadAttempt::Bytes(bytes) => { + return Ok(DownloadOutcome::Bytes { + bytes, + url: url.clone(), + }); + } + DownloadAttempt::NotFound(status) => { + last_status = Some(status); + continue; + } + } + } + if let Some(host) = denied_host { + return Ok(DownloadOutcome::Denied(host)); + } + if let Some(host) = prompt_host { + return Ok(DownloadOutcome::NeedsApproval(host)); + } + bail!( + "failed to download skill (last status: {})", + last_status + .map(|s| s.to_string()) + .unwrap_or_else(|| "unknown".to_string()) + ); +} + +enum DownloadAttempt { + Bytes(Vec), + NotFound(reqwest::StatusCode), +} + +/// Stream a URL into memory with a size cap. Aborts on the first read that +/// would push the buffer over `max_size * 4` (the *4 accounts for compression; +/// the unpack step still enforces `max_size` on the *uncompressed* bytes). +async fn download_with_cap(url: &str, max_size: u64) -> Result { + let resp = reqwest::get(url) + .await + .with_context(|| format!("failed to GET {url}"))?; + let status = resp.status(); + if !status.is_success() { + if status == reqwest::StatusCode::NOT_FOUND { + return Ok(DownloadAttempt::NotFound(status)); + } + bail!("download {url} returned {status}"); + } + // Soft cap on the *compressed* download — well above max_size to allow + // for highly compressible payloads but still bounded. + let compressed_cap = max_size.saturating_mul(4); + let bytes = resp + .bytes() + .await + .with_context(|| format!("failed to read body of {url}"))?; + if (bytes.len() as u64) > compressed_cap { + bail!("download {url} exceeds compressed size cap of {compressed_cap} bytes"); + } + Ok(DownloadAttempt::Bytes(bytes.to_vec())) +} + +struct StagedSkill { + skill_name: String, + staged_path: PathBuf, +} + +/// Validate a tarball and extract it into `/.tmp/`. +fn stage_tarball(bytes: &[u8], skills_dir: &Path, max_size: u64) -> Result { + fs::create_dir_all(skills_dir) + .with_context(|| format!("failed to create skills directory {}", skills_dir.display()))?; + + // Two passes: first determine the skill name (and therefore the staged + // dir) by finding the SKILL.md, then extract under that staged dir. + // Both passes share the same archive bytes; we reset by wrapping fresh + // decoders. + + let scan = scan_tarball(bytes, max_size)?; + + // Prepare staged directory. Use a `.tmp` suffix so a crashed install + // never collides with a real name; remove any leftover from a prior + // failed attempt. + let staged_path = skills_dir.join(format!("{}.tmp", scan.skill_name)); + if staged_path.exists() { + fs::remove_dir_all(&staged_path).with_context(|| { + format!( + "failed to clean stale staging dir {}", + staged_path.display() + ) + })?; + } + fs::create_dir_all(&staged_path) + .with_context(|| format!("failed to create staging dir {}", staged_path.display()))?; + + // Second pass — extract. + let result = extract_into(&scan, bytes, &staged_path, max_size); + if let Err(err) = result { + // Cleanup on failure so a half-staged directory doesn't survive. + let _ = fs::remove_dir_all(&staged_path); + return Err(err); + } + + Ok(StagedSkill { + skill_name: scan.skill_name, + staged_path, + }) +} + +struct TarballScan { + /// Skill name from SKILL.md frontmatter. + skill_name: String, + /// Archive prefix to strip from each entry (e.g. `repo-main/`). May be empty. + prefix: String, + /// Sub-directory inside `prefix` that the SKILL.md lives in (`""` if root, + /// or `skills/` for repos that bundle multiple skills). + skill_root: String, +} + +/// First pass: locate SKILL.md, validate frontmatter, compute total size, +/// reject path-traversal / symlink entries. We do not write anything in this +/// pass; that's the second pass's job. +fn scan_tarball(bytes: &[u8], max_size: u64) -> Result { + let cursor = std::io::Cursor::new(bytes); + let gz = GzDecoder::new(cursor); + let mut archive = tar::Archive::new(gz); + + let mut total_size: u64 = 0; + let mut prefix: Option = None; + let mut skill_md_relative: Option<(String, Vec)> = None; + + for entry in archive + .entries() + .context("failed to read tar entries (corrupt archive?)")? + { + let mut entry = entry.context("failed to read tar entry")?; + let header = entry.header().clone(); + let entry_type = header.entry_type(); + if entry_type.is_symlink() || entry_type.is_hard_link() { + return Err(InstallError::SymlinkRejected.into()); + } + let path = entry + .path() + .context("tar entry has invalid path")? + .to_path_buf(); + let path_str = path.to_string_lossy().into_owned(); + if !is_safe_path(&path) { + return Err(InstallError::PathTraversal(path_str).into()); + } + + // Track total size against `max_size` (uncompressed). We honor `header + // .size` rather than streaming-read every file; tar archives are + // self-describing so this is reliable for non-malicious inputs and + // catches the gzip-bomb case. + if let Ok(size) = header.size() { + total_size = total_size.saturating_add(size); + if total_size > max_size { + return Err(InstallError::OversizedTarball { limit: max_size }.into()); + } + } + + // Detect prefix from the first entry. GitHub archives wrap everything + // in `-/`; direct tarballs may have no prefix. We treat + // the first path component as the prefix iff the archive has more than + // one entry under it, but for SKILL.md detection we just strip the + // first component if every entry shares it. + if prefix.is_none() { + if let Some(Component::Normal(first)) = path.components().next() { + let candidate = first.to_string_lossy().into_owned(); + // Only treat the first component as a prefix if it's a + // directory-like (no extension and the path has more + // components). Otherwise leave prefix empty. + if path.components().count() > 1 { + prefix = Some(candidate); + } else { + prefix = Some(String::new()); + } + } else { + prefix = Some(String::new()); + } + } + + // SKILL.md detection. Match either: + // * `/SKILL.md` + // * `/skills//SKILL.md` + if entry_type.is_file() { + let stripped = strip_prefix(&path_str, prefix.as_deref().unwrap_or("")); + if stripped.eq_ignore_ascii_case("SKILL.md") + || stripped.starts_with("skills/") + && stripped.ends_with("/SKILL.md") + && stripped.matches('/').count() == 2 + { + let mut buf = Vec::new(); + entry + .read_to_end(&mut buf) + .context("failed to read SKILL.md from archive")?; + // Prefer the first match — we don't support multi-skill + // archives where a tarball ships several SKILL.mds at once. + if skill_md_relative.is_none() { + skill_md_relative = Some((stripped.to_string(), buf)); + } + } + } + } + + let prefix = prefix.unwrap_or_default(); + let (skill_md_path, skill_md_bytes) = skill_md_relative + .ok_or(InstallError::MissingSkillMd) + .map_err(anyhow::Error::from)?; + + // Parse frontmatter to extract the skill name. We reuse the same parser + // shape as `SkillRegistry::parse_skill` but inline it here so we don't + // depend on the discovery module's private function. + let name = parse_frontmatter_name(&skill_md_bytes)?; + + let skill_root = if skill_md_path == "SKILL.md" { + String::new() + } else { + // strip trailing /SKILL.md + skill_md_path + .strip_suffix("/SKILL.md") + .unwrap_or("") + .to_string() + }; + + Ok(TarballScan { + skill_name: name, + prefix, + skill_root, + }) +} + +fn extract_into(scan: &TarballScan, bytes: &[u8], dest: &Path, max_size: u64) -> Result<()> { + let cursor = std::io::Cursor::new(bytes); + let gz = GzDecoder::new(cursor); + let mut archive = tar::Archive::new(gz); + + let mut total_size: u64 = 0; + let prefix_with_root = if scan.skill_root.is_empty() { + scan.prefix.clone() + } else if scan.prefix.is_empty() { + scan.skill_root.clone() + } else { + format!("{}/{}", scan.prefix, scan.skill_root) + }; + + for entry in archive + .entries() + .context("failed to read tar entries (corrupt archive?)")? + { + let mut entry = entry.context("failed to read tar entry")?; + let header = entry.header().clone(); + let entry_type = header.entry_type(); + if entry_type.is_symlink() || entry_type.is_hard_link() { + return Err(InstallError::SymlinkRejected.into()); + } + let path = entry + .path() + .context("tar entry has invalid path")? + .to_path_buf(); + let path_str = path.to_string_lossy().into_owned(); + if !is_safe_path(&path) { + return Err(InstallError::PathTraversal(path_str).into()); + } + + // Only extract entries that live under our skill root. For simple + // tarballs (`SKILL.md` at root) that's everything; for multi-skill + // repos it's the `skills//` slice. + let stripped = strip_prefix(&path_str, &prefix_with_root).into_owned(); + if stripped.is_empty() && entry_type.is_dir() { + // The root directory itself — already created. + continue; + } + if stripped == path_str && !prefix_with_root.is_empty() { + // Nothing to strip => entry is outside our subtree, skip. + continue; + } + // Defense-in-depth: re-validate the stripped path. + let stripped_path = Path::new(&stripped); + if !is_safe_path(stripped_path) { + return Err(InstallError::PathTraversal(stripped).into()); + } + + let target = dest.join(stripped_path); + // Final paranoia check: ensure the resolved target stays under dest. + // We can't canonicalize (target doesn't exist yet), so we walk + // components one more time after composing. + let target_components: Vec<_> = target.components().collect(); + let dest_components: Vec<_> = dest.components().collect(); + if !target_components.starts_with(dest_components.as_slice()) { + return Err(InstallError::PathTraversal(stripped).into()); + } + + if entry_type.is_dir() { + fs::create_dir_all(&target) + .with_context(|| format!("failed to create dir {}", target.display()))?; + continue; + } + if entry_type.is_file() { + if let Some(parent) = target.parent() { + fs::create_dir_all(parent) + .with_context(|| format!("failed to create dir {}", parent.display()))?; + } + // Read into a buffer so we can enforce `max_size`. Files inside + // a SKILL bundle are small; copying through a buffer is fine. + let mut buf = Vec::new(); + entry + .read_to_end(&mut buf) + .with_context(|| format!("failed to read {}", path.display()))?; + total_size = total_size.saturating_add(buf.len() as u64); + if total_size > max_size { + return Err(InstallError::OversizedTarball { limit: max_size }.into()); + } + let mut out = fs::OpenOptions::new() + .create_new(true) + .write(true) + .open(&target) + .with_context(|| format!("failed to create {}", target.display()))?; + out.write_all(&buf) + .with_context(|| format!("failed to write {}", target.display()))?; + } + } + Ok(()) +} + +/// Ensure a tar path has no `..` segments and is not absolute. +fn is_safe_path(path: &Path) -> bool { + if path.is_absolute() { + return false; + } + for component in path.components() { + match component { + Component::ParentDir => return false, + Component::Prefix(_) | Component::RootDir => return false, + _ => {} + } + } + true +} + +/// Strip a leading directory prefix (e.g. `repo-main/`) from a tarball path. +fn strip_prefix<'a>(path: &'a str, prefix: &str) -> std::borrow::Cow<'a, str> { + if prefix.is_empty() { + return std::borrow::Cow::Borrowed(path); + } + let with_slash = format!("{prefix}/"); + if let Some(rest) = path.strip_prefix(&with_slash) { + std::borrow::Cow::Owned(rest.to_string()) + } else if path == prefix { + std::borrow::Cow::Borrowed("") + } else { + std::borrow::Cow::Borrowed(path) + } +} + +/// Extract `name:` and ensure `description:` exist in the SKILL.md frontmatter. +/// Also verifies the leading `---` fence so we reject malformed files early. +fn parse_frontmatter_name(bytes: &[u8]) -> Result { + let content = std::str::from_utf8(bytes).context("SKILL.md is not valid UTF-8")?; + let trimmed = content.trim_start(); + if !trimmed.starts_with("---") { + bail!("SKILL.md is missing the leading '---' frontmatter fence"); + } + let after_open = &trimmed[3..]; + let close = after_open.find("---").ok_or_else(|| { + anyhow::anyhow!("SKILL.md is missing the closing '---' frontmatter fence") + })?; + let frontmatter = &after_open[..close]; + + let mut name: Option = None; + let mut has_description = false; + for raw in frontmatter.lines() { + let line = raw.trim(); + if line.is_empty() || line.starts_with('#') { + continue; + } + if let Some((key, value)) = line.split_once(':') { + let key = key.trim().to_ascii_lowercase(); + let value = value.trim().to_string(); + match key.as_str() { + "name" if !value.is_empty() => name = Some(value), + "description" if !value.is_empty() => has_description = true, + _ => {} + } + } + } + + let name = name.ok_or(InstallError::MissingFrontmatterField("name"))?; + if !has_description { + return Err(InstallError::MissingFrontmatterField("description").into()); + } + // Sanity check: name must be a single path-safe segment. + if name.contains('/') + || name.contains('\\') + || name == "." + || name == ".." + || name.contains(' ') + { + bail!("SKILL.md `name` must be a single path-safe segment (got '{name}')"); + } + Ok(name) +} + +fn source_spec_string(source: &InstallSource) -> String { + match source { + InstallSource::GitHubRepo(repo) => format!("github:{repo}"), + InstallSource::DirectUrl(url) => url.clone(), + InstallSource::Registry(name) => name.clone(), + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Tests +// ───────────────────────────────────────────────────────────────────────────── + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parse_github_source() { + let s = InstallSource::parse("github:Hmbown/test-skill").unwrap(); + assert_eq!( + s, + InstallSource::GitHubRepo("Hmbown/test-skill".to_string()) + ); + } + + #[test] + fn parse_github_source_rejects_missing_repo() { + let err = InstallSource::parse("github:Hmbown").unwrap_err(); + assert!(err.to_string().contains("github source must"), "got: {err}"); + } + + #[test] + fn parse_github_source_rejects_extra_slashes() { + let err = InstallSource::parse("github:Hmbown/repo/extra").unwrap_err(); + assert!(err.to_string().contains("github source must"), "got: {err}"); + } + + #[test] + fn parse_direct_url_source() { + let s = InstallSource::parse("https://example.com/skill.tar.gz").unwrap(); + assert_eq!( + s, + InstallSource::DirectUrl("https://example.com/skill.tar.gz".to_string()) + ); + let s = InstallSource::parse("http://example.com/skill.tar.gz").unwrap(); + assert_eq!( + s, + InstallSource::DirectUrl("http://example.com/skill.tar.gz".to_string()) + ); + } + + #[test] + fn parse_registry_source() { + let s = InstallSource::parse("my-skill").unwrap(); + assert_eq!(s, InstallSource::Registry("my-skill".to_string())); + } + + #[test] + fn parse_rejects_empty() { + assert!(InstallSource::parse("").is_err()); + assert!(InstallSource::parse(" ").is_err()); + } + + #[test] + fn is_safe_path_rejects_traversal() { + assert!(!is_safe_path(Path::new("../etc/passwd"))); + assert!(!is_safe_path(Path::new("foo/../bar"))); + assert!(!is_safe_path(Path::new("/etc/passwd"))); + assert!(is_safe_path(Path::new("foo/bar/baz"))); + assert!(is_safe_path(Path::new("SKILL.md"))); + } + + #[test] + fn parse_frontmatter_extracts_name() { + let body = b"---\nname: hello\ndescription: greeter\n---\nbody\n"; + assert_eq!(parse_frontmatter_name(body).unwrap(), "hello"); + } + + #[test] + fn parse_frontmatter_missing_name_fails() { + let body = b"---\ndescription: x\n---\n"; + let err = parse_frontmatter_name(body).unwrap_err(); + assert!(format!("{err}").contains("name")); + } + + #[test] + fn parse_frontmatter_missing_description_fails() { + let body = b"---\nname: x\n---\n"; + let err = parse_frontmatter_name(body).unwrap_err(); + assert!(format!("{err}").contains("description")); + } + + #[test] + fn parse_frontmatter_rejects_unsafe_name() { + let body = b"---\nname: ../evil\ndescription: x\n---\n"; + assert!(parse_frontmatter_name(body).is_err()); + + let body = b"---\nname: a name with spaces\ndescription: x\n---\n"; + assert!(parse_frontmatter_name(body).is_err()); + } + + #[test] + fn parse_frontmatter_requires_opening_fence() { + let body = b"name: hello\ndescription: x\n"; + assert!(parse_frontmatter_name(body).is_err()); + } + + #[test] + fn strip_prefix_handles_all_cases() { + assert_eq!(strip_prefix("foo/bar", "foo"), "bar"); + assert_eq!(strip_prefix("foo", "foo"), ""); + assert_eq!(strip_prefix("baz/bar", "foo"), "baz/bar"); + assert_eq!(strip_prefix("foo/bar", ""), "foo/bar"); + } + + #[test] + fn source_spec_string_roundtrips() { + assert_eq!( + source_spec_string(&InstallSource::GitHubRepo("a/b".into())), + "github:a/b" + ); + assert_eq!( + source_spec_string(&InstallSource::DirectUrl("https://x".into())), + "https://x" + ); + assert_eq!( + source_spec_string(&InstallSource::Registry("x".into())), + "x" + ); + } +} diff --git a/crates/tui/src/skills/mod.rs b/crates/tui/src/skills/mod.rs index 46932466..88207e82 100644 --- a/crates/tui/src/skills/mod.rs +++ b/crates/tui/src/skills/mod.rs @@ -1,6 +1,17 @@ //! Skill discovery and registry for local SKILL.md files. +pub mod install; mod system; +// Re-exports kept for documentation parity and downstream consumers; the +// binary itself imports directly from `skills::install`. `#[allow(...)]` +// silences the dead-code warning that fires because no `bin` source path +// references these names through `skills::*`. +#[allow(unused_imports)] +pub use install::{ + DEFAULT_MAX_SIZE_BYTES, DEFAULT_REGISTRY_URL, INSTALLED_FROM_MARKER, InstallOutcome, + InstallSource, InstalledSkill, RegistryDocument, RegistryEntry, RegistryFetchResult, + UpdateResult, +}; pub use system::install_system_skills; use std::fs; diff --git a/crates/tui/tests/skill_install.rs b/crates/tui/tests/skill_install.rs new file mode 100644 index 00000000..4789f7b5 --- /dev/null +++ b/crates/tui/tests/skill_install.rs @@ -0,0 +1,522 @@ +//! Integration tests for the community-skill installer (#140). +//! +//! These tests exercise the full validation pipeline against a tiny in-process +//! HTTP server, so the network gate, download cap, tarball validation, atomic +//! rename, and `.installed-from` marker all run end-to-end. The module is +//! pulled in via `#[path]` includes (matching `integration_mock_llm.rs`) so we +//! get access to private helpers without a separate library crate. + +use std::io::Write; +use std::path::Path; + +use flate2::Compression; +use flate2::write::GzEncoder; +use tempfile::TempDir; +use tiny_http::{Method, Response, Server}; + +// Pull the production source files into this test binary so the test can +// reach `install`'s public surface without a dedicated library crate. +// +// `install.rs` only references `crate::network_policy` so we just need that +// one helper module alongside `install` itself. +#[path = "../src/network_policy.rs"] +mod network_policy; + +#[path = "../src/skills/install.rs"] +#[allow(dead_code)] +mod install; + +use crate::install::{InstallOutcome, InstallSource, UpdateResult}; +use crate::network_policy::{DecisionToml, NetworkPolicy}; + +/// Construct a gzipped tarball from `(path, body)` pairs. Permissions are set +/// to 0o644 so umask differences across platforms don't perturb the bytes. +fn make_tarball(entries: &[(&str, &[u8])]) -> Vec { + let mut gz = GzEncoder::new(Vec::new(), Compression::default()); + { + let mut builder = tar::Builder::new(&mut gz); + for (path, body) in entries { + let mut header = tar::Header::new_gnu(); + header.set_size(body.len() as u64); + header.set_mode(0o644); + header.set_cksum(); + builder + .append_data(&mut header, path, *body) + .expect("append_data"); + } + builder.finish().expect("finish tar"); + } + gz.finish().expect("finish gz") +} + +fn skill_md(name: &str, description: &str) -> Vec { + format!( + "---\nname: {name}\ndescription: {description}\n---\n# {name}\n\nThis is a test skill.\n" + ) + .into_bytes() +} + +fn allow_all_policy() -> NetworkPolicy { + NetworkPolicy { + default: DecisionToml::Allow, + allow: Vec::new(), + deny: Vec::new(), + audit: false, + } +} + +fn deny_all_policy() -> NetworkPolicy { + NetworkPolicy { + default: DecisionToml::Deny, + allow: Vec::new(), + deny: Vec::new(), + audit: false, + } +} + +fn prompt_all_policy() -> NetworkPolicy { + NetworkPolicy { + default: DecisionToml::Prompt, + allow: Vec::new(), + deny: Vec::new(), + audit: false, + } +} + +/// Spawn a tiny HTTP server that serves `bytes` at any path with 200 OK and +/// returns the bound URL. The server replies to *every* request (we re-use it +/// across multiple installs in the same test). +fn spawn_tarball_server( + bytes: Vec, +) -> ( + String, + std::sync::mpsc::Sender<()>, + std::thread::JoinHandle<()>, +) { + let server = Server::http("127.0.0.1:0").expect("bind ephemeral port"); + let url = format!( + "http://{}/skill.tar.gz", + server.server_addr().to_ip().expect("ip addr") + ); + let (shutdown_tx, shutdown_rx) = std::sync::mpsc::channel::<()>(); + let handle = std::thread::spawn(move || { + loop { + // Poll-style with a small recv timeout so we can break out cleanly. + match server.recv_timeout(std::time::Duration::from_millis(100)) { + Ok(Some(req)) => { + if req.method() != &Method::Get { + continue; + } + let response = Response::from_data(bytes.clone()); + let _ = req.respond(response); + } + Ok(None) => {} + Err(_) => break, + } + if shutdown_rx.try_recv().is_ok() { + break; + } + } + }); + (url, shutdown_tx, handle) +} + +fn shutdown(tx: std::sync::mpsc::Sender<()>, handle: std::thread::JoinHandle<()>) { + let _ = tx.send(()); + let _ = handle.join(); +} + +#[tokio::test] +async fn install_happy_path_writes_skill_and_marker() { + let tarball = make_tarball(&[ + ( + "test-skill-main/SKILL.md", + &skill_md("test-skill", "Test skill"), + ), + ("test-skill-main/notes.txt", b"hello world"), + ]); + let (url, tx, handle) = spawn_tarball_server(tarball); + + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + + let outcome = install::install( + InstallSource::DirectUrl(url), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect("install ok"); + + let installed = match outcome { + InstallOutcome::Installed(s) => s, + other => panic!("expected Installed, got {other:?}"), + }; + assert_eq!(installed.name, "test-skill"); + + let installed_dir = tmp.path().join("test-skill"); + assert!(installed_dir.is_dir(), "skill dir created"); + assert!(installed_dir.join("SKILL.md").is_file(), "SKILL.md present"); + assert!( + installed_dir.join("notes.txt").is_file(), + "extra file present" + ); + assert!( + installed_dir.join(install::INSTALLED_FROM_MARKER).is_file(), + ".installed-from marker present" + ); + + shutdown(tx, handle); +} + +#[tokio::test] +async fn install_rejects_path_traversal() { + // `tar::Builder::append_data` rejects `..` itself, so we craft the bad + // entry by writing the raw header bytes via `append`. + let mut gz = GzEncoder::new(Vec::new(), Compression::default()); + { + let mut builder = tar::Builder::new(&mut gz); + let body = skill_md("test-skill", "T"); + let mut hdr = tar::Header::new_gnu(); + hdr.set_size(body.len() as u64); + hdr.set_mode(0o644); + hdr.set_cksum(); + builder + .append_data(&mut hdr, "test-skill-main/SKILL.md", body.as_slice()) + .unwrap(); + + // Path-traversal entry. The `tar` crate's `set_path` rejects `..` + // itself, so we patch the raw 100-byte name field in the header. + let evil_body: &[u8] = b"not gonna happen"; + let mut evil_hdr = tar::Header::new_gnu(); + evil_hdr.set_size(evil_body.len() as u64); + evil_hdr.set_mode(0o644); + // Write a name with a `..` directly into the legacy "name" field. + let bytes = evil_hdr.as_old_mut(); + let evil_name = b"../etc/passwd"; + bytes.name[..evil_name.len()].copy_from_slice(evil_name); + evil_hdr.set_cksum(); + builder.append(&evil_hdr, evil_body).unwrap(); + builder.finish().unwrap(); + } + let tarball = gz.finish().unwrap(); + let (url, tx, handle) = spawn_tarball_server(tarball); + + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + let err = install::install( + InstallSource::DirectUrl(url), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect_err("path traversal must be rejected"); + let msg = format!("{err:#}"); + assert!( + msg.contains("escapes destination"), + "expected path-traversal error, got: {msg}" + ); + + shutdown(tx, handle); +} + +#[tokio::test] +async fn install_rejects_oversized_tarball() { + let big = vec![b'a'; 256 * 1024]; // 256 KiB per file + let mut entries: Vec<(String, Vec)> = Vec::new(); + entries.push(( + "test-skill-main/SKILL.md".to_string(), + skill_md("test-skill", "T"), + )); + for i in 0..50 { + entries.push((format!("test-skill-main/big-{i}.bin"), big.clone())); + } + let entry_refs: Vec<(&str, &[u8])> = entries + .iter() + .map(|(p, b)| (p.as_str(), b.as_slice())) + .collect(); + let tarball = make_tarball(&entry_refs); + let (url, tx, handle) = spawn_tarball_server(tarball); + + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + let small_cap = 1024 * 1024; + let err = install::install( + InstallSource::DirectUrl(url), + tmp.path(), + small_cap, + &policy, + false, + ) + .await + .expect_err("oversized must be rejected"); + let msg = format!("{err:#}"); + assert!( + msg.contains("too large") || msg.contains("exceed"), + "expected size cap error, got: {msg}" + ); + + shutdown(tx, handle); +} + +#[tokio::test] +async fn install_rejects_missing_skill_md() { + let tarball = make_tarball(&[("repo-main/README.md", b"not a skill")]); + let (url, tx, handle) = spawn_tarball_server(tarball); + + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + let err = install::install( + InstallSource::DirectUrl(url), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect_err("missing SKILL.md must be rejected"); + assert!(format!("{err:#}").contains("missing SKILL.md"), "{err:#}"); + + shutdown(tx, handle); +} + +#[tokio::test] +async fn install_rejects_missing_required_frontmatter() { + let tarball = make_tarball(&[("repo-main/SKILL.md", b"---\nname: test\n---\nbody\n")]); + let (url, tx, handle) = spawn_tarball_server(tarball); + + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + let err = install::install( + InstallSource::DirectUrl(url), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect_err("missing description must be rejected"); + assert!(format!("{err:#}").contains("description"), "{err:#}"); + + shutdown(tx, handle); +} + +#[tokio::test] +async fn install_idempotent_then_uninstall_then_reinstall() { + let tarball_bytes = + make_tarball(&[("repo-main/SKILL.md", &skill_md("idem-skill", "Idempotent"))]); + let (url, tx, handle) = spawn_tarball_server(tarball_bytes); + + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + + install::install( + InstallSource::DirectUrl(url.clone()), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect("first install ok"); + + // Second install with `update = false` must reject. + let err = install::install( + InstallSource::DirectUrl(url.clone()), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect_err("second install must reject"); + let msg = format!("{err:#}"); + assert!( + msg.contains("already installed"), + "expected already-installed error, got: {msg}" + ); + + // Uninstall then reinstall. + install::uninstall("idem-skill", tmp.path()).expect("uninstall ok"); + assert!(!tmp.path().join("idem-skill").exists()); + + install::install( + InstallSource::DirectUrl(url), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect("reinstall ok"); + + assert!(tmp.path().join("idem-skill").join("SKILL.md").is_file()); + shutdown(tx, handle); +} + +#[tokio::test] +async fn update_no_change_returns_nochange_without_overwriting() { + let tarball_bytes = + make_tarball(&[("repo-main/SKILL.md", &skill_md("upd-skill", "Update test"))]); + let (url, tx, handle) = spawn_tarball_server(tarball_bytes); + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + + install::install( + InstallSource::DirectUrl(url.clone()), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .unwrap(); + + // Patch the marker so update() re-fetches the same URL. + let marker_path = tmp + .path() + .join("upd-skill") + .join(install::INSTALLED_FROM_MARKER); + let marker_body = std::fs::read_to_string(&marker_path).unwrap(); + let mut marker_json: serde_json::Value = serde_json::from_str(&marker_body).unwrap(); + marker_json["spec"] = serde_json::Value::String(url); + std::fs::write(&marker_path, marker_json.to_string()).unwrap(); + + // Capture mtime so we can confirm SKILL.md wasn't rewritten. + let skill_md_path = tmp.path().join("upd-skill").join("SKILL.md"); + let mtime_before = std::fs::metadata(&skill_md_path) + .unwrap() + .modified() + .unwrap(); + + let result = install::update( + "upd-skill", + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + ) + .await + .expect("update ok"); + assert!(matches!(result, UpdateResult::NoChange)); + + let mtime_after = std::fs::metadata(&skill_md_path) + .unwrap() + .modified() + .unwrap(); + assert_eq!(mtime_before, mtime_after, "SKILL.md must not be rewritten"); + shutdown(tx, handle); +} + +#[tokio::test] +async fn install_with_deny_policy_returns_network_denied() { + let tmp = TempDir::new().unwrap(); + let policy = deny_all_policy(); + let outcome = install::install( + InstallSource::DirectUrl("https://example.invalid/skill.tar.gz".to_string()), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect("policy outcome should be Ok"); + match outcome { + InstallOutcome::NetworkDenied(host) => { + assert!(host.contains("example.invalid"), "got host {host}"); + } + other => panic!("expected NetworkDenied, got {other:?}"), + } + + // Verify the temp dir is untouched. + assert!( + std::fs::read_dir(tmp.path()).unwrap().next().is_none(), + "temp dir must be untouched" + ); +} + +#[tokio::test] +async fn install_with_prompt_policy_returns_needs_approval() { + let tmp = TempDir::new().unwrap(); + let policy = prompt_all_policy(); + let outcome = install::install( + InstallSource::DirectUrl("https://example.invalid/skill.tar.gz".to_string()), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect("policy outcome should be Ok"); + match outcome { + InstallOutcome::NeedsApproval(host) => { + assert!(host.contains("example.invalid"), "got host {host}"); + } + other => panic!("expected NeedsApproval, got {other:?}"), + } + assert!( + std::fs::read_dir(tmp.path()).unwrap().next().is_none(), + "temp dir must be untouched on prompt" + ); +} + +#[tokio::test] +async fn install_rejects_symlink_entry() { + let mut gz = GzEncoder::new(Vec::new(), Compression::default()); + { + let mut builder = tar::Builder::new(&mut gz); + + let body = skill_md("link-skill", "x"); + let mut hdr = tar::Header::new_gnu(); + hdr.set_size(body.len() as u64); + hdr.set_mode(0o644); + hdr.set_cksum(); + builder + .append_data(&mut hdr, "repo-main/SKILL.md", body.as_slice()) + .unwrap(); + + let mut link_hdr = tar::Header::new_gnu(); + link_hdr.set_entry_type(tar::EntryType::Symlink); + link_hdr.set_size(0); + link_hdr.set_mode(0o777); + builder + .append_link(&mut link_hdr, "repo-main/escape", Path::new("/etc/passwd")) + .unwrap(); + builder.finish().unwrap(); + } + let tarball = gz.finish().unwrap(); + let (url, tx, handle) = spawn_tarball_server(tarball); + + let tmp = TempDir::new().unwrap(); + let policy = allow_all_policy(); + let err = install::install( + InstallSource::DirectUrl(url), + tmp.path(), + install::DEFAULT_MAX_SIZE_BYTES, + &policy, + false, + ) + .await + .expect_err("symlinks must be rejected"); + assert!(format!("{err:#}").contains("symlink"), "{err:#}"); + + shutdown(tx, handle); +} + +#[test] +fn uninstall_refuses_system_skill() { + let tmp = TempDir::new().unwrap(); + let dir = tmp.path().join("system-skill"); + std::fs::create_dir_all(&dir).unwrap(); + let mut f = std::fs::File::create(dir.join("SKILL.md")).unwrap(); + f.write_all(b"---\nname: system-skill\ndescription: x\n---\n") + .unwrap(); + // No `.installed-from` marker — looks like a system skill. + + let err = install::uninstall("system-skill", tmp.path()).expect_err("must refuse"); + assert!(format!("{err:#}").contains("not installed via")); + assert!(dir.exists(), "directory must be left alone"); +}