From 9dba520c44654d9777401a658392287e1b96353f Mon Sep 17 00:00:00 2001 From: Lorenzo Delgado Date: Thu, 17 Sep 2026 12:09:53 +0200 Subject: [PATCH] feat(ampup): add skills subcommand to install amp agent skills Agent skills in the amp repository are not release assets and link docs that live elsewhere in the repository, so ampup reads them from the source tarball of a git ref and bundles those links into self-contained skill directories. - Add `ampup skills add|list|update|remove` for Claude Code, Codex, and Cursor, in the project or `--global`, defaulting to the skills of the active ampd version and pinnable with `--ref` or `--pr ` (the pull request head commit) - Copy files linked from `SKILL.md` frontmatter `metadata.references`/`assets` into `references/` and `assets/`, and record repo, ref, and commit in an `.amp-skills.json` lock file so skills ampup did not install are never touched - Validate skill names as single path components so a tampered lock file or `--skill` value cannot reach outside the skills directory, and record the lock entry before installing so interrupted installs are retried - Print results to stdout as text or `--json`, status to stderr, confirm `remove` unless `--force`, and print error causes from the top-level handler - Add `GitHubClient::resolve_commit`, `resolve_pull_request_head`, and `download_source_tarball`, which streams the tarball to an anonymous temporary file read in two passes, plus typed `thiserror` errors and end-to-end tests against a mock GitHub API Signed-off-by: Lorenzo Delgado --- Cargo.lock | 178 +++- ampup/Cargo.toml | 8 + ampup/src/commands.rs | 1 + ampup/src/commands/skills.rs | 1422 +++++++++++++++++++++++++++++ ampup/src/github.rs | 251 ++++- ampup/src/lib.rs | 1 + ampup/src/main.rs | 173 +++- ampup/src/skills.rs | 590 ++++++++++++ ampup/src/skills/agent.rs | 108 +++ ampup/src/skills/archive.rs | 229 +++++ ampup/src/skills/frontmatter.rs | 192 ++++ ampup/src/skills/lock.rs | 208 +++++ ampup/src/skills/skill_name.rs | 131 +++ ampup/src/tests/it_skills.rs | 1044 +++++++++++++++++++++ ampup/src/tests/mod.rs | 1 + ampup/src/ui.rs | 20 + docs/code/apps-cli.md | 15 +- docs/features/app-ampup-skills.md | 170 ++++ docs/features/app-ampup.md | 10 + 19 files changed, 4728 insertions(+), 24 deletions(-) create mode 100644 ampup/src/commands/skills.rs create mode 100644 ampup/src/skills.rs create mode 100644 ampup/src/skills/agent.rs create mode 100644 ampup/src/skills/archive.rs create mode 100644 ampup/src/skills/frontmatter.rs create mode 100644 ampup/src/skills/lock.rs create mode 100644 ampup/src/skills/skill_name.rs create mode 100644 ampup/src/tests/it_skills.rs create mode 100644 docs/features/app-ampup-skills.md diff --git a/Cargo.lock b/Cargo.lock index 5760403..d889556 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -22,13 +22,28 @@ dependencies = [ "reqwest", "semver", "serde", + "serde-saphyr", + "serde_json", "sha2", "tar", "tempfile", + "thiserror 2.0.18", "tokio", + "tokio-util", "vergen-gitcl", ] +[[package]] +name = "annotate-snippets" +version = "0.12.16" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f211a51805bc641f3ad5b7664c77d2547af685cc33b4cd8d31964027a46f13f1" +dependencies = [ + "anstyle", + "memchr", + "unicode-width", +] + [[package]] name = "anstream" version = "0.6.21" @@ -85,6 +100,12 @@ version = "1.0.101" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5f0e0fee31ef5ed1ba1316088939cea399010ed7731dba877ed44aeb407a75ea" +[[package]] +name = "arraydeque" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d902e3d592a523def97af8f317b08ce16b7ab854c1985a0c671e6f15cebc236" + [[package]] name = "atomic-waker" version = "1.1.2" @@ -213,7 +234,7 @@ dependencies = [ "heck", "proc-macro2", "quote", - "syn", + "syn 2.0.115", ] [[package]] @@ -276,6 +297,12 @@ version = "0.8.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" +[[package]] +name = "core_detect" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7f8f80099a98041a3d1622845c271458a2d73e688351bf3cb999266764b81d48" + [[package]] name = "cpufeatures" version = "0.2.17" @@ -325,7 +352,7 @@ dependencies = [ "proc-macro2", "quote", "strsim", - "syn", + "syn 2.0.115", ] [[package]] @@ -336,7 +363,7 @@ checksum = "fc34b93ccb385b40dc71c6fceac4b2ad23662c7eeb248cf10d529b7e055b6ead" dependencies = [ "darling_core", "quote", - "syn", + "syn 2.0.115", ] [[package]] @@ -366,7 +393,7 @@ dependencies = [ "darling", "proc-macro2", "quote", - "syn", + "syn 2.0.115", ] [[package]] @@ -376,7 +403,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ab63b0e2bf4d5928aff72e83a7dace85d7bba5fe12dcc3c5a572d78caffd3f3c" dependencies = [ "derive_builder_core", - "syn", + "syn 2.0.115", ] [[package]] @@ -409,7 +436,7 @@ checksum = "97369cbbc041bc366949bc74d34658d6cda5621039731c6310521892a3a20ae0" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.115", ] [[package]] @@ -424,6 +451,30 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "34aa73646ffb006b8f5147f3dc182bd4bcb190227ce861fc4a4844bf8e3cb2c0" +[[package]] +name = "encoding_rs" +version = "0.8.41" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7b5ef0006ac9ab233c38522f5ae99cae3625151de8f706cacee1cba4b8e2832a" +dependencies = [ + "cfg-if", + "core_detect", + "multiversion", + "multiversion_no_op", + "rustversion", + "scopeguard", + "simdutf8", +] + +[[package]] +name = "encoding_rs_io" +version = "0.1.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fba3fe847045ecff794b9c138293a80db914678c453ad63fbf0c6a9eb6e00b22" +dependencies = [ + "encoding_rs", +] + [[package]] name = "equivalent" version = "1.0.2" @@ -564,7 +615,7 @@ checksum = "162ee34ebcb7c64a8abebc059ce0fee27c2262618d7b60ed8faf72fef13c3650" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.115", ] [[package]] @@ -647,6 +698,16 @@ dependencies = [ "wasip3", ] +[[package]] +name = "granit-parser" +version = "1.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e20f99e46474f56bd905c56e817ebddcf377a611f94c53ac4649e4d3fa3c0cd0" +dependencies = [ + "arraydeque", + "smallvec", +] + [[package]] name = "hashbrown" version = "0.15.5" @@ -1035,12 +1096,54 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "multiversion" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b4ca4bea16ffc3f443cf7d866912118196bfef4c6a1556ca00f9f9b00bb43f7c" +dependencies = [ + "multiversion-macros", +] + +[[package]] +name = "multiversion-macros" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0d416831a7317ef4b08bee00b69cbbb9c8763da7959a7026244d6266869f9c83" +dependencies = [ + "proc-macro2", + "quote", + "rustversion", + "syn 3.0.6", +] + +[[package]] +name = "multiversion_no_op" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "743fb55ba31b18fb1ecef6bdc9aa2743314978ac084044301a7eee33fb99a20d" + +[[package]] +name = "nohash-hasher" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2bf50223579dc7cdcfb3bfcacf7069ff68243f8c363f62ffa99cf000a6b9c451" + [[package]] name = "num-conv" version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cf97ec579c3c42f953ef76dbf8d55ac91fb219dde70e49aa4a6b7d74e9919050" +[[package]] +name = "num-traits" +version = "0.2.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "071dfc062690e90b734c0b2273ce72ad0ffa95f0c74596bc250dcfd960262841" +dependencies = [ + "autocfg", +] + [[package]] name = "num_threads" version = "0.1.7" @@ -1140,7 +1243,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "479ca8adacdd7ce8f1fb39ce9ecccbfe93a3f1344b3d0d97f20bc0196208f62b" dependencies = [ "proc-macro2", - "syn", + "syn 2.0.115", ] [[package]] @@ -1489,6 +1592,23 @@ dependencies = [ "serde_derive", ] +[[package]] +name = "serde-saphyr" +version = "1.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b8050abb251097357e24aff63ba2c52a6309ecb7d23a5474023df960a02694d8" +dependencies = [ + "annotate-snippets", + "base64", + "encoding_rs_io", + "granit-parser", + "nohash-hasher", + "num-traits", + "serde_core", + "smallvec", + "zmij", +] + [[package]] name = "serde_core" version = "1.0.228" @@ -1506,7 +1626,7 @@ checksum = "d540f220d3187173da220f885ab66608367b6574e925011a9353e4badda91d79" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.115", ] [[package]] @@ -1563,6 +1683,12 @@ version = "0.3.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3a219298ac11a56ea9a6d2120044824d6f01aeb034955e7af7bc16858527deea" +[[package]] +name = "simdutf8" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e3a9fe34e3e7a50316060351f37187a3f546bce95496156754b601a5fa71b76e" + [[package]] name = "slab" version = "0.4.12" @@ -1614,6 +1740,17 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "syn" +version = "3.0.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8593e8e72159ed2257d083c7a454a85cbf854f37a0966d8d483aff8c8a3ebcee" +dependencies = [ + "proc-macro2", + "quote", + "unicode-ident", +] + [[package]] name = "sync_wrapper" version = "1.0.2" @@ -1631,7 +1768,7 @@ checksum = "728a70f3dbaf5bab7f0c4b1ac8d7ae5ea60a4b5549c8a5914361c99147a709d2" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.115", ] [[package]] @@ -1684,7 +1821,7 @@ checksum = "4fee6c4efc90059e10f81e6d42c60a18f76588c3d74cb83a0b242a2b6c7504c1" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.115", ] [[package]] @@ -1695,7 +1832,7 @@ checksum = "ebc4ee7f67670e9b64d05fa4253e753e016c6c95ff35b89b7941d6b856dec1d5" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.115", ] [[package]] @@ -1780,7 +1917,7 @@ checksum = "af407857209536a95c8e56f8231ef2c2e2aff839b22e07a1ffcbc617e9db9fa5" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.115", ] [[package]] @@ -1802,6 +1939,7 @@ dependencies = [ "bytes", "futures-core", "futures-sink", + "futures-util", "pin-project-lite", "tokio", ] @@ -2063,7 +2201,7 @@ dependencies = [ "bumpalo", "proc-macro2", "quote", - "syn", + "syn 2.0.115", "wasm-bindgen-shared", ] @@ -2419,7 +2557,7 @@ dependencies = [ "heck", "indexmap", "prettyplease", - "syn", + "syn 2.0.115", "wasm-metadata", "wit-bindgen-core", "wit-component", @@ -2435,7 +2573,7 @@ dependencies = [ "prettyplease", "proc-macro2", "quote", - "syn", + "syn 2.0.115", "wit-bindgen-core", "wit-bindgen-rust", ] @@ -2512,7 +2650,7 @@ checksum = "b659052874eb698efe5b9e8cf382204678a0086ebf46982b79d6ca3182927e5d" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.115", "synstructure", ] @@ -2533,7 +2671,7 @@ checksum = "4122cd3169e94605190e77839c9a40d40ed048d305bfdc146e7df40ab0f3e517" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.115", ] [[package]] @@ -2553,7 +2691,7 @@ checksum = "d71e5d6e06ab090c67b5e44993ec16b72dcbaabc526db883a360057678b48502" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.115", "synstructure", ] @@ -2593,7 +2731,7 @@ checksum = "eadce39539ca5cb3985590102671f2567e659fca9666581ad3411d59207951f3" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.115", ] [[package]] diff --git a/ampup/Cargo.toml b/ampup/Cargo.toml index cbef488..fc84824 100644 --- a/ampup/Cargo.toml +++ b/ampup/Cargo.toml @@ -26,10 +26,15 @@ reqwest = { version = "0.13", default-features = false, features = [ ] } semver = { version = "1.0.18", features = ["serde"] } serde = { version = "1.0", features = ["derive"] } +serde-saphyr = "1.3" +serde_json = "1.0" sha2 = "0.10" tar = "0.4" tempfile = "3.13.0" +thiserror = "2.0" tokio = { version = "1.36.0", features = [ + "fs", + "io-util", "macros", "parking_lot", "rt-multi-thread", @@ -37,5 +42,8 @@ tokio = { version = "1.36.0", features = [ "test-util", ] } +[dev-dependencies] +tokio-util = { version = "0.7", features = ["rt"] } + [build-dependencies] vergen-gitcl = { version = "9.0.0", features = ["build"] } diff --git a/ampup/src/commands.rs b/ampup/src/commands.rs index b64901c..9ca803d 100644 --- a/ampup/src/commands.rs +++ b/ampup/src/commands.rs @@ -3,6 +3,7 @@ pub mod build; pub mod init; pub mod install; pub mod list; +pub mod skills; pub mod uninstall; pub mod update; pub mod use_version; diff --git a/ampup/src/commands/skills.rs b/ampup/src/commands/skills.rs new file mode 100644 index 0000000..8a3ffb2 --- /dev/null +++ b/ampup/src/commands/skills.rs @@ -0,0 +1,1422 @@ +//! The `ampup skills` commands: add, list, update, and remove agent skills. +//! +//! A skill ampup did not install (one missing from the lock file) is never +//! overwritten, updated, or removed. Each command writes its result to stdout +//! (as text, or as JSON with `--json`) and all status and warnings to stderr. + +use std::{ + collections::BTreeSet, + num::NonZeroU32, + path::{Path, PathBuf}, + time::Duration, +}; + +use anyhow::{Context, Result}; +use dialoguer::{Confirm, MultiSelect, theme::ColorfulTheme}; + +use crate::{ + DEFAULT_REPO, + config::Config, + github::GitHubClient, + skills::{ + self, Agent, InstallSkillError, InstalledSkill, LoadSkillsError, LoadSkillsLockError, + SaveSkillsLockError, Scope, Skill, SkillName, SkillsDir, SkillsLock, UninstallSkillError, + }, + token, ui, +}; + +/// Deadline for resolving a git ref or pull request to a commit. +const RESOLVE_COMMIT_TIMEOUT: Duration = Duration::from_secs(30); + +/// Deadline for downloading the repository source tarball, which holds the +/// whole repository. +const SOURCE_TARBALL_TIMEOUT: Duration = Duration::from_secs(600); + +/// Where skills are downloaded from, shared by the commands that download. +#[derive(Debug, clap::Args)] +pub struct SkillsSourceArgs { + /// Git tag, branch, or commit to install from (defaults to the active ampd version) + #[arg(long = "ref", value_name = "REF", conflicts_with = "pr")] + pub git_ref: Option, + + /// Pull request number to install from, at the current head of its branch + #[arg(long, value_name = "NUMBER")] + pub pr: Option, + + /// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp) + #[arg(long, env = "AMP_DIR")] + pub install_dir: Option, + + /// GitHub repository in format "owner/repo" + #[arg(long, default_value_t = DEFAULT_REPO.to_string())] + pub repo: String, + + /// GitHub token for private repository access (defaults to $GITHUB_TOKEN) + #[arg(long, env = "GITHUB_TOKEN", hide_env = true)] + pub github_token: Option, +} + +impl SkillsSourceArgs { + /// The ref the `--ref` and `--pr` flags select, which clap keeps mutually + /// exclusive; neither means the active ampd version. + pub fn source_ref(&self) -> SourceRef { + match (&self.git_ref, self.pr) { + (Some(git_ref), _) => SourceRef::Git(git_ref.clone()), + (None, Some(number)) => SourceRef::PullRequest(number), + (None, None) => SourceRef::ActiveVersion, + } + } +} + +/// The point in the repository history to download skills from. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SourceRef { + /// The tag of the active ampd version, so skills match the installed binaries. + ActiveVersion, + /// A tag, branch, or commit. + Git(String), + /// The current head commit of a pull request. + PullRequest(NonZeroU32), +} + +/// Which skills a command acts on. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SkillSelection { + /// Exactly these skills. + Named(Vec), + /// Every skill. + All, + /// Ask the user to pick. + Prompt, +} + +impl SkillSelection { + /// The selection the `--skill` and `--all` flags describe: `--all` wins, + /// then named skills, and neither means prompting. + pub fn from_flags(skill_names: Vec, all: bool) -> Self { + if all { + Self::All + } else if skill_names.is_empty() { + Self::Prompt + } else { + Self::Named(skill_names) + } + } +} + +/// List the skills available at a git ref, without installing them. +/// +/// # Errors +/// +/// Fails when the ampd config or GitHub client cannot be created, the skills +/// cannot be listed (see [`ListAvailableSkillsError`]), or the result cannot be +/// printed. +pub async fn list_available(source: SkillsSourceArgs, json: bool) -> Result<()> { + let source_ref = source.source_ref(); + let config = Config::new(source.install_dir)?; + let github = GitHubClient::new( + source.repo, + token::resolve_github_token(source.github_token), + )?; + + let available = list_available_skills(&github, &config, source_ref).await?; + print_result(&available, json) +} + +/// Install skills. +/// +/// # Errors +/// +/// Fails when the ampd config, GitHub client, or install location cannot be +/// created, the skills cannot be installed (see [`AddSkillsError`]), or the +/// result cannot be printed. +pub async fn add( + source: SkillsSourceArgs, + selection: SkillSelection, + agents: Vec, + scope: Scope, + json: bool, +) -> Result<()> { + let source_ref = source.source_ref(); + let config = Config::new(source.install_dir)?; + let github = GitHubClient::new( + source.repo, + token::resolve_github_token(source.github_token), + )?; + let location = SkillsLocation::create(scope)?; + + let changes = add_skills(&github, &config, &location, source_ref, selection, agents).await?; + print_result(&changes, json) +} + +/// List the skills installed by ampup. +/// +/// # Errors +/// +/// Fails when the install location cannot be determined, a lock file cannot be +/// read (see [`ListInstalledSkillsError`]), or the result cannot be printed. +pub async fn list(agents: Vec, scope: Scope, json: bool) -> Result<()> { + let location = SkillsLocation::create(scope)?; + + // Reading lock files is blocking file I/O + let list_task = tokio::task::spawn_blocking(move || list_installed_skills(&location, &agents)); + let installed = list_task.await.context("Skill listing task failed")??; + + if installed.dirs.is_empty() { + ui::info_stderr!("No skills installed by ampup"); + ui::detail_stderr!("Run 'ampup skills add' to install skills"); + } + print_result(&installed, json) +} + +/// Reinstall installed skills from a git ref. +/// +/// # Errors +/// +/// Fails when the ampd config, GitHub client, or install location cannot be +/// created, the skills cannot be updated (see [`UpdateSkillsError`]), or the +/// result cannot be printed. +pub async fn update( + source: SkillsSourceArgs, + skill_names: Vec, + agents: Vec, + scope: Scope, + json: bool, +) -> Result<()> { + let source_ref = source.source_ref(); + let config = Config::new(source.install_dir)?; + let github = GitHubClient::new( + source.repo, + token::resolve_github_token(source.github_token), + )?; + let location = SkillsLocation::create(scope)?; + + let changes = + update_skills(&github, &config, &location, source_ref, skill_names, agents).await?; + print_result(&changes, json) +} + +/// Remove skills installed by ampup, after confirming unless `force` is set. +/// +/// # Errors +/// +/// Fails when the install location cannot be determined, the skills cannot be +/// removed (see [`RemoveSkillsError`]), or the result cannot be printed. +pub async fn remove( + selection: SkillSelection, + agents: Vec, + scope: Scope, + force: bool, + json: bool, +) -> Result<()> { + let location = SkillsLocation::create(scope)?; + + // Removing skills is blocking file I/O and may prompt the user + let remove_task = + tokio::task::spawn_blocking(move || remove_skills(&location, selection, &agents, force)); + let changes = remove_task.await.context("Skill removal task failed")??; + + print_result(&changes, json) +} + +/// List the skills available at a git ref, using the given client and config. +/// +/// # Errors +/// +/// See [`ListAvailableSkillsError`]. +// Split from `list_available` so tests can inject a client pointed at a mock server +pub(crate) async fn list_available_skills( + github: &GitHubClient, + config: &Config, + source_ref: SourceRef, +) -> Result { + let resolved_ref = resolve_source_ref(config, source_ref) + .map_err(ListAvailableSkillsError::ResolveSourceRef)?; + let snapshot = download_snapshot(github, resolved_ref) + .await + .map_err(ListAvailableSkillsError::Download)?; + + // Decompressing and parsing the tarball is CPU-bound + let load_task = tokio::task::spawn_blocking(move || { + let skills = load_snapshot_skills(&snapshot)?; + Ok(AvailableSkills { + repo: snapshot.repo, + git_ref: snapshot.git_ref, + commit: snapshot.commit, + skills: skills + .into_iter() + .map(|skill| AvailableSkill { + name: skill.name, + description: skill.description, + }) + .collect(), + }) + }); + + load_task + .await + .map_err(ListAvailableSkillsError::LoadTask)? + .map_err(ListAvailableSkillsError::LoadSkills) +} + +/// Errors from listing the skills available at a git ref +#[derive(Debug, thiserror::Error)] +pub(crate) enum ListAvailableSkillsError { + /// No ref could be determined + #[error("Failed to determine the ref to list skills from")] + ResolveSourceRef(#[source] ResolveSourceRefError), + + /// The repository snapshot could not be downloaded + #[error("Failed to download skills")] + Download(#[source] DownloadSnapshotError), + + /// The task reading the downloaded skills panicked or was cancelled + #[error("Skill loading task failed")] + LoadTask(#[source] tokio::task::JoinError), + + /// The downloaded snapshot has no readable skills + #[error("Failed to load skills")] + LoadSkills(#[source] LoadSnapshotSkillsError), +} + +/// Install skills using the given client, config, and install location. +/// +/// # Errors +/// +/// See [`AddSkillsError`]. +// Split from `add` so tests can inject a client pointed at a mock server +pub(crate) async fn add_skills( + github: &GitHubClient, + config: &Config, + location: &SkillsLocation, + source_ref: SourceRef, + selection: SkillSelection, + agents: Vec, +) -> Result { + let resolved_ref = + resolve_source_ref(config, source_ref).map_err(AddSkillsError::ResolveSourceRef)?; + let snapshot = download_snapshot(github, resolved_ref) + .await + .map_err(AddSkillsError::Download)?; + + // Parsing the tarball, prompting, and writing files all block + let location = location.clone(); + let install_task = tokio::task::spawn_blocking(move || { + install_from_snapshot(&snapshot, selection, agents, &location) + }); + + install_task + .await + .map_err(AddSkillsError::InstallTask)? + .map_err(AddSkillsError::Install) +} + +/// Errors from installing skills +#[derive(Debug, thiserror::Error)] +pub(crate) enum AddSkillsError { + /// No ref could be determined + #[error("Failed to determine the ref to install skills from")] + ResolveSourceRef(#[source] ResolveSourceRefError), + + /// The repository snapshot could not be downloaded + /// + /// Nothing has been written when this occurs. + #[error("Failed to download skills")] + Download(#[source] DownloadSnapshotError), + + /// The task installing the skills panicked or was cancelled + #[error("Skill install task failed")] + InstallTask(#[source] tokio::task::JoinError), + + /// The skills could not be installed + /// + /// Skills installed before the failure stay installed and recorded in the + /// lock file. Safe to retry: the next run replaces them. + #[error("Failed to install skills")] + Install(#[source] InstallFromSnapshotError), +} + +/// List the skills installed by ampup at the given install location. +/// +/// # Errors +/// +/// See [`ListInstalledSkillsError`]. +pub(crate) fn list_installed_skills( + location: &SkillsLocation, + agents: &[Agent], +) -> Result { + let agents = agents_or_all(agents); + + let mut dirs = Vec::new(); + for skills_dir in location.skills_dirs(&agents) { + let lock = SkillsLock::load(&skills_dir.path).map_err(ListInstalledSkillsError)?; + if lock.skills.is_empty() { + continue; + } + + dirs.push(InstalledSkillsDir { + skills_dir: location.display(&skills_dir), + agents: skills_dir.agents, + skills: lock + .skills + .into_iter() + .map(|(name, installed)| InstalledSkillEntry { name, installed }) + .collect(), + }); + } + + Ok(InstalledSkills { dirs }) +} + +/// A skills lock file could not be read while listing installed skills +#[derive(Debug, thiserror::Error)] +#[error("Failed to read installed skills")] +pub(crate) struct ListInstalledSkillsError(#[source] LoadSkillsLockError); + +/// Reinstall installed skills using the given client, config, and install +/// location. +/// +/// # Errors +/// +/// See [`UpdateSkillsError`]. +// Split from `update` so tests can inject a client pointed at a mock server +pub(crate) async fn update_skills( + github: &GitHubClient, + config: &Config, + location: &SkillsLocation, + source_ref: SourceRef, + skill_names: Vec, + agents: Vec, +) -> Result { + let agents = agents_or_all(&agents); + + // Check for something to update before downloading anything + let check_location = location.clone(); + let check_agents = agents.clone(); + let check_names = skill_names.clone(); + let check_task = tokio::task::spawn_blocking(move || { + has_installed_skills(&check_location, &check_agents, &check_names) + }); + let has_skills_to_update = check_task + .await + .map_err(UpdateSkillsError::CheckTask)? + .map_err(UpdateSkillsError::CheckInstalled)?; + if !has_skills_to_update { + ui::info_stderr!("No installed skills to update"); + return Ok(SkillChanges::default()); + } + + let resolved_ref = + resolve_source_ref(config, source_ref).map_err(UpdateSkillsError::ResolveSourceRef)?; + let snapshot = download_snapshot(github, resolved_ref) + .await + .map_err(UpdateSkillsError::Download)?; + + // Parsing the tarball and writing files both block + let location = location.clone(); + let update_task = tokio::task::spawn_blocking(move || { + update_from_snapshot(&snapshot, &skill_names, &agents, &location) + }); + + update_task + .await + .map_err(UpdateSkillsError::UpdateTask)? + .map_err(UpdateSkillsError::Update) +} + +/// Errors from updating installed skills +#[derive(Debug, thiserror::Error)] +pub(crate) enum UpdateSkillsError { + /// The task checking for installed skills panicked or was cancelled + #[error("Installed skills check task failed")] + CheckTask(#[source] tokio::task::JoinError), + + /// A skills lock file could not be read while checking for installed skills + /// + /// Nothing has been downloaded or written when this occurs. + #[error("Failed to read installed skills")] + CheckInstalled(#[source] LoadSkillsLockError), + + /// No ref could be determined + #[error("Failed to determine the ref to update skills to")] + ResolveSourceRef(#[source] ResolveSourceRefError), + + /// The repository snapshot could not be downloaded + /// + /// Nothing has been written when this occurs. + #[error("Failed to download skills")] + Download(#[source] DownloadSnapshotError), + + /// The task updating the skills panicked or was cancelled + #[error("Skill update task failed")] + UpdateTask(#[source] tokio::task::JoinError), + + /// The skills could not be updated + /// + /// Skills updated before the failure keep their new version. Safe to retry. + #[error("Failed to update skills")] + Update(#[source] UpdateFromSnapshotError), +} + +/// Remove skills installed by ampup at the given install location, after +/// confirming unless `force` is set. +/// +/// A declined confirmation removes nothing and returns no changes. +/// +/// # Errors +/// +/// See [`RemoveSkillsError`]. +pub(crate) fn remove_skills( + location: &SkillsLocation, + selection: SkillSelection, + agents: &[Agent], + force: bool, +) -> Result { + let agents = agents_or_all(agents); + let skills_dirs = location.skills_dirs(&agents); + + let mut installed_names = BTreeSet::new(); + for skills_dir in &skills_dirs { + let lock = SkillsLock::load(&skills_dir.path).map_err(RemoveSkillsError::LoadLock)?; + installed_names.extend(lock.skills.into_keys()); + } + if installed_names.is_empty() { + ui::info_stderr!("No skills installed by ampup"); + return Ok(SkillChanges::default()); + } + + let installed_names: Vec = installed_names.into_iter().collect(); + let names_to_remove: Vec = match selection { + SkillSelection::All => installed_names, + SkillSelection::Named(skill_names) => { + let mut names = Vec::new(); + for name in skill_names { + if installed_names.contains(&name) { + names.push(name); + } else { + ui::warn!("Skipping {}: not installed by ampup", name); + } + } + names + } + SkillSelection::Prompt => { + let items: Vec = installed_names.iter().map(SkillName::to_string).collect(); + let selected_indexes = prompt_multi_select("Select skills to remove", &items) + .map_err(RemoveSkillsError::SelectSkills)?; + selected_indexes + .into_iter() + .map(|index| installed_names[index].clone()) + .collect() + } + }; + if names_to_remove.is_empty() { + ui::info_stderr!("No skills selected"); + return Ok(SkillChanges::default()); + } + + if !force { + let names: Vec = names_to_remove.iter().map(SkillName::to_string).collect(); + let prompt = format!( + "{} will no longer be available to your agents. Remove?", + names.join(", ") + ); + let is_confirmed = Confirm::with_theme(&ColorfulTheme::default()) + .with_prompt(prompt) + .default(false) + .interact() + .map_err(RemoveSkillsError::Confirm)?; + if !is_confirmed { + ui::info_stderr!("Nothing removed"); + return Ok(SkillChanges::default()); + } + } + + let mut changes = SkillChanges::default(); + for skills_dir in &skills_dirs { + let mut lock = SkillsLock::load(&skills_dir.path).map_err(RemoveSkillsError::LoadLock)?; + + for name in &names_to_remove { + if !lock.skills.contains_key(name) { + continue; + } + + skills::uninstall_skill(&skills_dir.path, name) + .map_err(RemoveSkillsError::UninstallSkill)?; + lock.skills.remove(name); + lock.save(&skills_dir.path) + .map_err(RemoveSkillsError::SaveLock)?; + + changes.changes.push(SkillChange { + action: SkillAction::Removed, + skill: name.clone(), + skills_dir: location.display(skills_dir), + agents: skills_dir.agents.clone(), + source: None, + }); + } + } + + Ok(changes) +} + +/// Errors from removing installed skills +/// +/// Skills removed before a failure stay removed, and the lock file reflects +/// them. Safe to retry. +#[derive(Debug, thiserror::Error)] +pub(crate) enum RemoveSkillsError { + /// A skills lock file could not be read + /// + /// Occurs, among other causes, when the lock file names a skill that is not + /// a valid directory name. Nothing is removed in that case. + #[error("Failed to read installed skills")] + LoadLock(#[source] LoadSkillsLockError), + + /// The prompt for picking skills failed + /// + /// Occurs when stdin is not a terminal; pass `--skill` or `--all` instead. + #[error("Failed to get skill selection")] + SelectSkills(#[source] dialoguer::Error), + + /// The confirmation prompt failed + /// + /// Occurs when stdin is not a terminal; pass `--force` to skip the prompt. + #[error("Failed to get confirmation")] + Confirm(#[source] dialoguer::Error), + + /// A skill could not be deleted + #[error("Failed to remove skill")] + UninstallSkill(#[source] UninstallSkillError), + + /// A lock file could not be updated after removing a skill + #[error("Failed to update skills lock file")] + SaveLock(#[source] SaveSkillsLockError), +} + +/// The skills available at a git ref, as printed by `ampup skills add --list`. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] +pub(crate) struct AvailableSkills { + /// GitHub repository in format `owner/repo`. + pub repo: String, + /// The ref the skills were read from. + #[serde(rename = "ref")] + pub git_ref: String, + /// The commit the ref resolved to. + pub commit: String, + /// The skills, sorted by name. + pub skills: Vec, +} + +/// One skill available at a git ref. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] +pub(crate) struct AvailableSkill { + pub name: SkillName, + pub description: String, +} + +impl std::fmt::Display for AvailableSkills { + /// Formats each skill as its name followed by an indented description. + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + for (index, skill) in self.skills.iter().enumerate() { + if index > 0 { + writeln!(f)?; + } + write!( + f, + "{}\n {}", + ui::version(&skill.name), + console::style(&skill.description).dim() + )?; + } + Ok(()) + } +} + +/// The skills ampup installed, grouped by skills directory, as printed by +/// `ampup skills list`. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] +pub(crate) struct InstalledSkills { + /// Skills directories that hold at least one skill installed by ampup. + pub dirs: Vec, +} + +/// The skills ampup installed in one skills directory. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] +pub(crate) struct InstalledSkillsDir { + /// The directory as the user would type it (relative, or under `~`). + pub skills_dir: String, + /// The agents that read from this directory. + pub agents: Vec, + /// The installed skills, sorted by name. + pub skills: Vec, +} + +/// One installed skill and where it came from. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] +pub(crate) struct InstalledSkillEntry { + pub name: SkillName, + #[serde(flatten)] + pub installed: InstalledSkill, +} + +impl std::fmt::Display for InstalledSkills { + /// Formats each directory with its agents, then one indented line per + /// skill with its ref and short commit. + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + for (index, dir) in self.dirs.iter().enumerate() { + if index > 0 { + writeln!(f)?; + } + write!( + f, + "{} ({})", + ui::path(&dir.skills_dir), + agent_names(&dir.agents) + )?; + for skill in &dir.skills { + write!( + f, + "\n {} {} ({})", + skill.name, + skill.installed.git_ref, + short_commit(&skill.installed.commit) + )?; + } + } + Ok(()) + } +} + +/// What a command changed, as printed by `ampup skills add`, `update`, and +/// `remove`. +#[derive(Debug, Clone, Default, PartialEq, Eq, serde::Serialize)] +pub(crate) struct SkillChanges { + /// One entry per skill per skills directory, in the order they happened. + pub changes: Vec, +} + +/// A skill installed, updated, or removed in one skills directory. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)] +pub(crate) struct SkillChange { + pub action: SkillAction, + pub skill: SkillName, + /// The directory as the user would type it (relative, or under `~`). + pub skills_dir: String, + /// The agents that read from this directory. + pub agents: Vec, + /// Where the skill now comes from; absent for removals. + #[serde(skip_serializing_if = "Option::is_none")] + pub source: Option, +} + +/// The kind of change made to a skill. +#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize)] +#[serde(rename_all = "lowercase")] +pub(crate) enum SkillAction { + Installed, + Updated, + Removed, +} + +impl std::fmt::Display for SkillChanges { + /// Formats one checkmark line per change, such as + /// `✓ Installed amp-query in .claude/skills (claude-code)`. + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + for (index, change) in self.changes.iter().enumerate() { + if index > 0 { + writeln!(f)?; + } + let verb = match change.action { + SkillAction::Installed => "Installed", + SkillAction::Updated => "Updated", + SkillAction::Removed => "Removed", + }; + let preposition = match change.action { + SkillAction::Installed | SkillAction::Updated => "in", + SkillAction::Removed => "from", + }; + write!( + f, + "{} {} {} {} {} ({})", + console::style("✓").green().bold(), + verb, + ui::version(&change.skill), + preposition, + ui::path(&change.skills_dir), + agent_names(&change.agents) + )?; + } + Ok(()) + } +} + +/// Where skills commands install to: a scope and the directory it is relative +/// to (the project root, or the home directory for global skills). +// The CLI entry points derive it from the current and home directories; tests +// point it at a temporary directory +#[derive(Debug, Clone)] +pub(crate) struct SkillsLocation { + pub scope: Scope, + pub base_dir: PathBuf, +} + +impl SkillsLocation { + /// Project skills are relative to the current directory, global skills to + /// the home directory. + fn create(scope: Scope) -> Result { + let base_dir = match scope { + Scope::Global => { + // A missing HOME is expected on Windows, where USERPROFILE holds + // the home directory; the error that matters is both being missing + let home = std::env::var("HOME") + .or_else(|_| std::env::var("USERPROFILE")) + .context("Could not determine home directory")?; + home.into() + } + Scope::Project => { + std::env::current_dir().context("Could not determine current directory")? + } + }; + + Ok(Self { scope, base_dir }) + } + + fn skills_dirs(&self, agents: &[Agent]) -> Vec { + skills::skills_dirs(agents, self.scope, &self.base_dir) + } + + /// Show a skills directory the way the user would type it: relative to the + /// project, or under `~` for global skills. + fn display(&self, skills_dir: &SkillsDir) -> String { + // Skills directories are always built from `base_dir`, so this only + // falls back to the absolute path if that ever changes + let Ok(relative_dir) = skills_dir.path.strip_prefix(&self.base_dir) else { + return skills_dir.path.display().to_string(); + }; + + match self.scope { + Scope::Project => relative_dir.display().to_string(), + Scope::Global => Path::new("~").join(relative_dir).display().to_string(), + } + } +} + +/// The repository at one commit, downloaded but not yet parsed. +struct SourceSnapshot { + repo: String, + /// The ref as recorded in the lock file, such as `v0.1.0` or `pr/42`. + git_ref: String, + commit: String, + /// The gzipped source tarball, in an anonymous temporary file that the + /// operating system deletes once it is closed. + tarball: std::fs::File, +} + +/// Resolve a ref to a commit and download the repository at that commit. +async fn download_snapshot( + github: &GitHubClient, + resolved_ref: ResolvedRef, +) -> Result { + let git_ref = resolved_ref.label(); + ui::info_stderr!( + "Fetching skills from {} at {}", + github.repo(), + ui::version(&git_ref) + ); + + // Download by commit, so the recorded commit is exactly what was installed + // even if the ref moves in between + let resolution = match &resolved_ref { + ResolvedRef::Git(git_ref) => { + tokio::time::timeout(RESOLVE_COMMIT_TIMEOUT, github.resolve_commit(git_ref)).await + } + ResolvedRef::PullRequest(number) => { + tokio::time::timeout( + RESOLVE_COMMIT_TIMEOUT, + github.resolve_pull_request_head(*number), + ) + .await + } + }; + let commit = match resolution { + Ok(Ok(commit)) => commit, + Ok(Err(err)) => return Err(DownloadSnapshotError::ResolveCommit(err)), + // `Elapsed` carries nothing beyond the limit, which the variant records + Err(_) => { + return Err(DownloadSnapshotError::ResolveCommitTimeout( + RESOLVE_COMMIT_TIMEOUT, + )); + } + }; + + let tarball = tempfile::tempfile().map_err(DownloadSnapshotError::CreateTarballFile)?; + // The clone shares the underlying file, so what is written through it can be + // read back from `tarball` + let tarball_writer = tarball + .try_clone() + .map_err(DownloadSnapshotError::CreateTarballFile)?; + let mut tarball_writer = tokio::fs::File::from_std(tarball_writer); + + let download = tokio::time::timeout( + SOURCE_TARBALL_TIMEOUT, + github.download_source_tarball(&commit, &mut tarball_writer), + ) + .await; + match download { + Ok(Ok(())) => {} + Ok(Err(err)) => return Err(DownloadSnapshotError::DownloadTarball(err)), + // `Elapsed` carries nothing beyond the limit, which the variant records + Err(_) => { + return Err(DownloadSnapshotError::DownloadTarballTimeout( + SOURCE_TARBALL_TIMEOUT, + )); + } + } + + Ok(SourceSnapshot { + repo: github.repo().to_string(), + git_ref, + commit, + tarball, + }) +} + +/// Errors from downloading a repository snapshot +#[derive(Debug, thiserror::Error)] +pub(crate) enum DownloadSnapshotError { + /// The git ref or pull request could not be resolved to a commit + /// + /// Possible causes: + /// - The ref or pull request does not exist + /// - The repository is private and no GitHub token is available + /// - GitHub rate limiting + #[error("Failed to resolve the ref to a commit")] + ResolveCommit(#[source] anyhow::Error), + + /// Resolving the git ref or pull request took longer than the deadline + #[error("Resolving the ref timed out after {} seconds", .0.as_secs())] + ResolveCommitTimeout(Duration), + + /// The temporary file for the source tarball could not be created + /// + /// Possible causes: + /// - The temporary directory (`$TMPDIR`) is missing or not writable + #[error("Failed to create a temporary file for the source tarball")] + CreateTarballFile(#[source] std::io::Error), + + /// The source tarball could not be downloaded + /// + /// Possible causes: + /// - Network failure or GitHub error + /// - The temporary directory ran out of space + #[error("Failed to download source tarball")] + DownloadTarball(#[source] anyhow::Error), + + /// Downloading the source tarball took longer than the deadline + #[error("Downloading the source tarball timed out after {} seconds", .0.as_secs())] + DownloadTarballTimeout(Duration), +} + +/// Read the skills of a downloaded snapshot, requiring at least one. +fn load_snapshot_skills(snapshot: &SourceSnapshot) -> Result, LoadSnapshotSkillsError> { + let skills = + skills::load_skills(&snapshot.tarball).map_err(LoadSnapshotSkillsError::LoadSkills)?; + + if skills.is_empty() { + return Err(LoadSnapshotSkillsError::NoSkillsInRef { + repo: snapshot.repo.clone(), + git_ref: snapshot.git_ref.clone(), + }); + } + + Ok(skills) +} + +/// Errors from reading the skills of a downloaded snapshot +#[derive(Debug, thiserror::Error)] +pub(crate) enum LoadSnapshotSkillsError { + /// The skills in the snapshot could not be read + #[error("Failed to read skills from the repository")] + LoadSkills(#[source] LoadSkillsError), + + /// The snapshot has no skills + /// + /// Occurs for refs from before skills were added to the repository, such + /// as older releases. + #[error( + "No skills found in {repo} at {git_ref}; try a newer ref with --ref or --pr " + )] + NoSkillsInRef { repo: String, git_ref: String }, +} + +/// Select skills and agents, then install the skills into each skills +/// directory, recording each in the lock file first. +fn install_from_snapshot( + snapshot: &SourceSnapshot, + selection: SkillSelection, + agents: Vec, + location: &SkillsLocation, +) -> Result { + let available_skills = + load_snapshot_skills(snapshot).map_err(InstallFromSnapshotError::LoadSkills)?; + + let selected_skills = select_skills_to_add(&available_skills, selection, snapshot) + .map_err(InstallFromSnapshotError::SelectSkills)?; + if selected_skills.is_empty() { + ui::info_stderr!("No skills selected"); + return Ok(SkillChanges::default()); + } + + let agents = select_agents(agents).map_err(InstallFromSnapshotError::SelectAgents)?; + if agents.is_empty() { + ui::info_stderr!("No agents selected"); + return Ok(SkillChanges::default()); + } + + let installed = InstalledSkill { + repo: snapshot.repo.clone(), + git_ref: snapshot.git_ref.clone(), + commit: snapshot.commit.clone(), + }; + + let mut changes = SkillChanges::default(); + for skills_dir in location.skills_dirs(&agents) { + let mut lock = + SkillsLock::load(&skills_dir.path).map_err(InstallFromSnapshotError::LoadLock)?; + + for skill in &selected_skills { + // Never overwrite a skill the user wrote or installed another way + let is_installed_by_ampup = lock.skills.contains_key(&skill.name); + if skills_dir.path.join(skill.name.as_str()).exists() && !is_installed_by_ampup { + ui::warn!( + "Skipping {}: it already exists in {} and was not installed by ampup", + skill.name, + ui::path(location.display(&skills_dir)) + ); + continue; + } + + // Record first, so an interrupted install is retried rather than + // skipped as a skill the user wrote + lock.skills.insert(skill.name.clone(), installed.clone()); + lock.save(&skills_dir.path) + .map_err(InstallFromSnapshotError::SaveLock)?; + skills::install_skill(&skills_dir.path, skill) + .map_err(InstallFromSnapshotError::InstallSkill)?; + + changes.changes.push(SkillChange { + action: SkillAction::Installed, + skill: skill.name.clone(), + skills_dir: location.display(&skills_dir), + agents: skills_dir.agents.clone(), + source: Some(installed.clone()), + }); + } + } + + Ok(changes) +} + +/// Errors from installing skills out of a downloaded snapshot +#[derive(Debug, thiserror::Error)] +pub(crate) enum InstallFromSnapshotError { + /// The snapshot's skills could not be read + /// + /// Nothing has been written when this occurs. + #[error("Failed to load skills")] + LoadSkills(#[source] LoadSnapshotSkillsError), + + /// The requested skills could not be selected + /// + /// Nothing has been written when this occurs. + #[error("Failed to select skills")] + SelectSkills(#[source] SelectSkillsError), + + /// The prompt for picking agents failed + /// + /// Occurs when stdin is not a terminal; pass `--agent` instead. + #[error("Failed to get agent selection")] + SelectAgents(#[source] dialoguer::Error), + + /// A skills lock file could not be read + #[error("Failed to read installed skills")] + LoadLock(#[source] LoadSkillsLockError), + + /// A skills lock file could not be written before installing a skill + #[error("Failed to update skills lock file")] + SaveLock(#[source] SaveSkillsLockError), + + /// A skill could not be installed + /// + /// The skill is already recorded in the lock file, so running the command + /// again replaces whatever was left behind. + #[error("Failed to install skill")] + InstallSkill(#[source] InstallSkillError), +} + +/// Reinstall the requested installed skills from a downloaded snapshot. +fn update_from_snapshot( + snapshot: &SourceSnapshot, + skill_names: &[SkillName], + agents: &[Agent], + location: &SkillsLocation, +) -> Result { + let available_skills = + load_snapshot_skills(snapshot).map_err(UpdateFromSnapshotError::LoadSkills)?; + + let installed = InstalledSkill { + repo: snapshot.repo.clone(), + git_ref: snapshot.git_ref.clone(), + commit: snapshot.commit.clone(), + }; + + let mut changes = SkillChanges::default(); + for skills_dir in location.skills_dirs(agents) { + let mut lock = + SkillsLock::load(&skills_dir.path).map_err(UpdateFromSnapshotError::LoadLock)?; + + let installed_names: Vec = lock + .skills + .keys() + .filter(|name| is_requested(name, skill_names)) + .cloned() + .collect(); + + for name in installed_names { + let Some(skill) = available_skills.iter().find(|skill| skill.name == name) else { + ui::warn!( + "Skipping {}: not found in {} at {}", + name, + snapshot.repo, + ui::version(&snapshot.git_ref) + ); + continue; + }; + + // Record first, matching installs: a failed update is retried by + // the next run + lock.skills.insert(name.clone(), installed.clone()); + lock.save(&skills_dir.path) + .map_err(UpdateFromSnapshotError::SaveLock)?; + skills::install_skill(&skills_dir.path, skill) + .map_err(UpdateFromSnapshotError::InstallSkill)?; + + changes.changes.push(SkillChange { + action: SkillAction::Updated, + skill: name, + skills_dir: location.display(&skills_dir), + agents: skills_dir.agents.clone(), + source: Some(installed.clone()), + }); + } + } + + Ok(changes) +} + +/// Errors from reinstalling skills out of a downloaded snapshot +#[derive(Debug, thiserror::Error)] +pub(crate) enum UpdateFromSnapshotError { + /// The snapshot's skills could not be read + /// + /// Nothing has been written when this occurs. + #[error("Failed to load skills")] + LoadSkills(#[source] LoadSnapshotSkillsError), + + /// A skills lock file could not be read + #[error("Failed to read installed skills")] + LoadLock(#[source] LoadSkillsLockError), + + /// A skills lock file could not be written before updating a skill + #[error("Failed to update skills lock file")] + SaveLock(#[source] SaveSkillsLockError), + + /// A skill could not be reinstalled + /// + /// The lock file already records the new version, so running the command + /// again replaces whatever was left behind. + #[error("Failed to update skill")] + InstallSkill(#[source] InstallSkillError), +} + +/// Whether any of the requested skills is installed by ampup for the agents. +fn has_installed_skills( + location: &SkillsLocation, + agents: &[Agent], + skill_names: &[SkillName], +) -> Result { + for skills_dir in location.skills_dirs(agents) { + let lock = SkillsLock::load(&skills_dir.path)?; + if lock + .skills + .keys() + .any(|name| is_requested(name, skill_names)) + { + return Ok(true); + } + } + Ok(false) +} + +/// Turn the requested ref into one that can be downloaded, using the active +/// ampd version when none was given so skills match the installed binaries. +fn resolve_source_ref( + config: &Config, + source_ref: SourceRef, +) -> Result { + match source_ref { + SourceRef::Git(git_ref) => Ok(ResolvedRef::Git(git_ref)), + SourceRef::PullRequest(number) => Ok(ResolvedRef::PullRequest(number)), + SourceRef::ActiveVersion => match config.current_version() { + Ok(Some(version)) => Ok(ResolvedRef::Git(version)), + Ok(None) => Err(ResolveSourceRefError::NoActiveVersion), + Err(err) => Err(ResolveSourceRefError::ReadActiveVersion(err)), + }, + } +} + +/// Errors from determining the ref to download skills from +#[derive(Debug, thiserror::Error)] +pub(crate) enum ResolveSourceRefError { + /// The active ampd version file could not be read + #[error("Failed to read the active ampd version")] + ReadActiveVersion(#[source] anyhow::Error), + + /// Neither `--ref` nor `--pr` was given and no ampd version is active + /// + /// Occurs before `ampup install` has run, or after the active version was + /// uninstalled. + #[error( + "No active ampd version to match skills against; run 'ampup install' or pass --ref or --pr " + )] + NoActiveVersion, +} + +/// A ref that can be resolved to a commit on GitHub. +enum ResolvedRef { + Git(String), + PullRequest(NonZeroU32), +} + +impl ResolvedRef { + /// The ref as shown to the user and recorded in the lock file: the git ref + /// itself, or `pr/` for a pull request. + fn label(&self) -> String { + match self { + Self::Git(git_ref) => git_ref.clone(), + Self::PullRequest(number) => format!("pr/{}", number), + } + } +} + +fn select_skills_to_add<'a>( + available_skills: &'a [Skill], + selection: SkillSelection, + snapshot: &SourceSnapshot, +) -> Result, SelectSkillsError> { + let skill_names = match selection { + SkillSelection::All => return Ok(available_skills.iter().collect()), + SkillSelection::Prompt => { + let items: Vec = available_skills + .iter() + .map(|skill| skill.name.to_string()) + .collect(); + let selected_indexes = prompt_multi_select("Select skills to install", &items) + .map_err(SelectSkillsError::Prompt)?; + return Ok(selected_indexes + .into_iter() + .map(|index| &available_skills[index]) + .collect()); + } + SkillSelection::Named(skill_names) => skill_names, + }; + + let mut selected_skills = Vec::new(); + for name in skill_names { + let Some(skill) = available_skills.iter().find(|skill| skill.name == name) else { + return Err(SelectSkillsError::SkillNotFound { + name, + repo: snapshot.repo.clone(), + git_ref: snapshot.git_ref.clone(), + available: available_skills + .iter() + .map(|skill| skill.name.clone()) + .collect(), + }); + }; + selected_skills.push(skill); + } + + Ok(selected_skills) +} + +/// Errors from selecting the skills to install +#[derive(Debug, thiserror::Error)] +pub(crate) enum SelectSkillsError { + /// The prompt for picking skills failed + /// + /// Occurs when stdin is not a terminal; pass `--skill` or `--all` instead. + #[error("Failed to get skill selection")] + Prompt(#[source] dialoguer::Error), + + /// A skill requested with `--skill` does not exist at the ref + /// + /// No skill is installed when this occurs, including valid ones requested + /// alongside it. + #[error( + "Skill {name} not found in {repo} at {git_ref}; available skills: {}", + skill_list(available) + )] + SkillNotFound { + name: SkillName, + repo: String, + git_ref: String, + available: Vec, + }, +} + +/// Use the given agents, or ask the user to pick some. +fn select_agents(agents: Vec) -> Result, dialoguer::Error> { + if !agents.is_empty() { + return Ok(agents); + } + + let items: Vec = Agent::ALL + .iter() + .map(|agent| agent.name().to_string()) + .collect(); + let selected_indexes = prompt_multi_select("Select agents to install for", &items)?; + + Ok(selected_indexes + .into_iter() + .map(|index| Agent::ALL[index]) + .collect()) +} + +/// Commands that only act on installed skills default to every agent. +fn agents_or_all(agents: &[Agent]) -> Vec { + if agents.is_empty() { + Agent::ALL.to_vec() + } else { + agents.to_vec() + } +} + +/// Ask the user to pick any number of items, and return the indexes they +/// picked. +fn prompt_multi_select(prompt: &str, items: &[String]) -> Result, dialoguer::Error> { + MultiSelect::with_theme(&ColorfulTheme::default()) + .with_prompt(format!("{} (space to toggle, enter to confirm)", prompt)) + .items(items) + .interact() +} + +/// Print a command's result to stdout, as JSON or as its human form. +fn print_result(result: &T, json: bool) -> Result<()> +where + T: serde::Serialize + std::fmt::Display, +{ + if json { + let text = serde_json::to_string_pretty(result).context("Failed to serialize result")?; + println!("{text}"); + return Ok(()); + } + + // An empty result has already been explained on stderr + let text = result.to_string(); + if !text.is_empty() { + println!("{text}"); + } + Ok(()) +} + +fn agent_names(agents: &[Agent]) -> String { + let names: Vec<&str> = agents.iter().map(|agent| agent.name()).collect(); + names.join(", ") +} + +fn skill_list(names: &[SkillName]) -> String { + let names: Vec<&str> = names.iter().map(SkillName::as_str).collect(); + names.join(", ") +} + +/// An empty filter means every installed skill. +fn is_requested(name: &SkillName, skill_names: &[SkillName]) -> bool { + skill_names.is_empty() || skill_names.contains(name) +} + +fn short_commit(commit: &str) -> &str { + commit.get(..7).unwrap_or(commit) +} + +#[cfg(test)] +mod tests { + use anyhow::Result; + + use super::*; + + fn change(action: SkillAction, skill: &str) -> Result { + Ok(SkillChange { + action, + skill: skill.parse()?, + skills_dir: ".claude/skills".to_string(), + agents: vec![Agent::ClaudeCode], + source: None, + }) + } + + #[test] + fn skill_changes_display_with_install_and_removal_renders_one_line_each() -> Result<()> { + //* Given + console::set_colors_enabled(false); + let changes = SkillChanges { + changes: vec![ + change(SkillAction::Installed, "amp-query")?, + change(SkillAction::Removed, "amp-setup")?, + ], + }; + + //* When + let text = changes.to_string(); + + //* Then + assert_eq!( + text, + "✓ Installed amp-query in .claude/skills (claude-code)\n\ + ✓ Removed amp-setup from .claude/skills (claude-code)", + "each change should render as one checkmark line" + ); + + Ok(()) + } + + #[test] + fn skill_changes_display_with_no_changes_renders_nothing() { + //* Given + let changes = SkillChanges::default(); + + //* When + let text = changes.to_string(); + + //* Then + assert_eq!(text, "", "an empty result should print nothing to stdout"); + } + + #[test] + fn skill_selection_from_flags_with_all_and_names_selects_all() -> Result<()> { + //* Given + let skill_names = vec!["amp-query".parse()?]; + + //* When + let selection = SkillSelection::from_flags(skill_names, true); + + //* Then + assert_eq!( + selection, + SkillSelection::All, + "--all should win over --skill" + ); + + Ok(()) + } +} diff --git a/ampup/src/github.rs b/ampup/src/github.rs index 0e09120..3587865 100644 --- a/ampup/src/github.rs +++ b/ampup/src/github.rs @@ -1,8 +1,9 @@ -use std::sync::Arc; +use std::{num::NonZeroU32, sync::Arc}; use anyhow::{Context, Result}; use futures::StreamExt; use serde::Deserialize; +use tokio::io::AsyncWriteExt as _; use crate::{DEFAULT_REPO, DEFAULT_SELF_REPO, rate_limiter::GitHubRateLimiter}; @@ -45,6 +46,20 @@ pub enum GitHubError { retry_after_secs: u64, has_token: bool, }, + /// A git ref (tag, branch, or commit) does not exist. GitHub answers 404 or + /// 422; without a token this can also mean the repository is private. + RefNotFound { + repo: String, + git_ref: String, + has_token: bool, + }, + /// A pull request number does not exist. Without a token this can also + /// mean the repository is private. + PullRequestNotFound { + repo: String, + number: NonZeroU32, + has_token: bool, + }, } impl std::fmt::Display for GitHubError { @@ -164,6 +179,39 @@ impl std::fmt::Display for GitHubError { writeln!(f, " Try: export GITHUB_TOKEN=$(gh auth token)")?; } } + Self::RefNotFound { + repo, + git_ref, + has_token, + } => { + writeln!(f, "Git ref not found")?; + writeln!(f, " Repository: {}", repo)?; + writeln!(f, " Ref: {}", git_ref)?; + writeln!(f)?; + if *has_token { + writeln!(f, " No tag, branch, or commit with this name exists.")?; + writeln!(f, " Try: --ref ")?; + } else { + writeln!(f, " The repository is private or requires authentication.")?; + writeln!(f, " Try: export GITHUB_TOKEN=$(gh auth token)")?; + } + } + Self::PullRequestNotFound { + repo, + number, + has_token, + } => { + writeln!(f, "Pull request not found")?; + writeln!(f, " Repository: {}", repo)?; + writeln!(f, " Pull request: #{}", number)?; + writeln!(f)?; + if *has_token { + writeln!(f, " No pull request with this number exists.")?; + } else { + writeln!(f, " The repository is private or requires authentication.")?; + writeln!(f, " Try: export GITHUB_TOKEN=$(gh auth token)")?; + } + } } Ok(()) } @@ -237,6 +285,16 @@ struct Release { assets: Vec, } +#[derive(Debug, Deserialize)] +struct PullRequest { + head: PullRequestHead, +} + +#[derive(Debug, Deserialize)] +struct PullRequestHead { + sha: String, +} + #[derive(Debug, Deserialize)] struct Asset { id: u64, @@ -257,6 +315,9 @@ pub struct GitHubClient { token: Option, /// Base URL for API requests (either custom API or GitHub API) api: String, + /// Base URL for repository source requests (commits and tarballs), which + /// the ampup API does not proxy + source_api: String, rate_limiter: Arc, } @@ -274,6 +335,7 @@ impl GitHubClient { .context("Failed to create request client")?; let api = release_api_base(&repo); + let source_api = format!("{}/repos/{}", GITHUB_API_URL, repo); let rate_limiter = Arc::new(GitHubRateLimiter::new(github_token.is_some())); @@ -282,6 +344,7 @@ impl GitHubClient { repo, token: github_token, api, + source_api, rate_limiter, }) } @@ -302,11 +365,17 @@ impl GitHubClient { client, repo: "test/repo".to_string(), token: github_token, + source_api: api_base.clone(), api: api_base, rate_limiter, }) } + /// GitHub repository in format `owner/repo` + pub fn repo(&self) -> &str { + &self.repo + } + /// Get the latest release version pub async fn get_latest_version(&self) -> Result { let release = self.get_latest_release().await?; @@ -537,6 +606,186 @@ impl GitHubClient { Ok(release) } + /// Resolve a git ref (tag, branch, or commit) to its full commit SHA. + /// + /// # Errors + /// + /// Fails with [`GitHubError::RefNotFound`] when the ref does not exist, + /// [`GitHubError::AuthFailed`] on 401 or 403, [`GitHubError::RateLimited`] + /// when rate limited, and [`GitHubError::HttpError`] on any other failed + /// status or when the request cannot be sent. + pub async fn resolve_commit(&self, git_ref: &str) -> Result { + let url = format!("{}/commits/{}", self.source_api, git_ref); + + let response = self + .send_with_rate_limit( + || { + self.client + .get(&url) + .header(reqwest::header::ACCEPT, "application/vnd.github.sha") + }, + "Failed to resolve git ref", + ) + .await?; + + let status = response.status(); + match status { + // GitHub answers 422 when the ref does not name a commit + reqwest::StatusCode::NOT_FOUND | reqwest::StatusCode::UNPROCESSABLE_ENTITY => { + return Err(GitHubError::RefNotFound { + repo: self.repo.clone(), + git_ref: git_ref.to_string(), + has_token: self.token.is_some(), + } + .into()); + } + reqwest::StatusCode::UNAUTHORIZED | reqwest::StatusCode::FORBIDDEN => { + return Err(GitHubError::AuthFailed { + status_code: status.as_u16(), + repo: self.repo.clone(), + url, + } + .into()); + } + _ if !status.is_success() => { + // The status is the error being reported; the body is extra + // detail, and failing to read it must not hide the status + let body = response.text().await.unwrap_or_default(); + return Err(GitHubError::HttpError { + repo: self.repo.clone(), + status_code: status.as_u16(), + url, + body, + } + .into()); + } + _ => {} + } + + let commit = response + .text() + .await + .context("Failed to read commit SHA response")?; + + Ok(commit.trim().to_string()) + } + + /// Resolve a pull request to the commit at the head of its branch. + /// + /// This works for pull requests from forks and for closed or merged pull + /// requests, because GitHub keeps their head commits reachable from the + /// base repository. + /// + /// # Errors + /// + /// Fails with [`GitHubError::PullRequestNotFound`] when the pull request + /// does not exist, [`GitHubError::AuthFailed`] on 401 or 403, + /// [`GitHubError::RateLimited`] when rate limited, + /// [`GitHubError::HttpError`] on any other failed status or when the + /// request cannot be sent, and when the response is not a pull request. + pub async fn resolve_pull_request_head(&self, number: NonZeroU32) -> Result { + let url = format!("{}/pulls/{}", self.source_api, number); + + let response = self + .send_with_rate_limit(|| self.client.get(&url), "Failed to fetch pull request") + .await?; + + let status = response.status(); + match status { + reqwest::StatusCode::NOT_FOUND => { + return Err(GitHubError::PullRequestNotFound { + repo: self.repo.clone(), + number, + has_token: self.token.is_some(), + } + .into()); + } + reqwest::StatusCode::UNAUTHORIZED | reqwest::StatusCode::FORBIDDEN => { + return Err(GitHubError::AuthFailed { + status_code: status.as_u16(), + repo: self.repo.clone(), + url, + } + .into()); + } + _ if !status.is_success() => { + // The status is the error being reported; the body is extra + // detail, and failing to read it must not hide the status + let body = response.text().await.unwrap_or_default(); + return Err(GitHubError::HttpError { + repo: self.repo.clone(), + status_code: status.as_u16(), + url, + body, + } + .into()); + } + _ => {} + } + + let pull_request: PullRequest = response + .json() + .await + .context("Failed to parse pull request response")?; + + Ok(pull_request.head.sha) + } + + /// Download the gzipped source tarball of the repository at a commit into + /// `destination`, writing it chunk by chunk as it arrives. + /// + /// The tarball holds the whole repository, so it is streamed to a file + /// instead of being held in memory. + /// + /// # Errors + /// + /// Fails with [`GitHubError::DownloadFailed`] on a failed status, + /// [`GitHubError::RateLimited`] when rate limited, or when the request, the + /// body stream, or a write to `destination` fails. `destination` may hold a + /// partial tarball after a failure. + pub async fn download_source_tarball( + &self, + commit: &str, + destination: &mut tokio::fs::File, + ) -> Result<()> { + let url = format!("{}/tarball/{}", self.source_api, commit); + + let response = self + .send_with_rate_limit( + || self.client.get(&url), + "Failed to download source tarball", + ) + .await?; + + if !response.status().is_success() { + return Err(GitHubError::DownloadFailed { + repo: self.repo.clone(), + asset_name: "source tarball".to_string(), + status_code: response.status().as_u16(), + url, + } + .into()); + } + + let mut stream = response.bytes_stream(); + while let Some(chunk) = stream.next().await { + let chunk = chunk.context("Error while downloading source tarball")?; + destination + .write_all(&chunk) + .await + .context("Failed to write source tarball")?; + } + + // `tokio::fs::File` buffers writes, so data not flushed here could be + // missing when the file is read back + destination + .flush() + .await + .context("Failed to write source tarball")?; + + Ok(()) + } + /// Download a release asset by name. pub async fn download_release_asset(&self, version: &str, asset_name: &str) -> Result> { let release = self.get_tagged_release(version).await?; diff --git a/ampup/src/lib.rs b/ampup/src/lib.rs index 6610a61..63d8751 100644 --- a/ampup/src/lib.rs +++ b/ampup/src/lib.rs @@ -10,6 +10,7 @@ pub mod platform; pub mod progress; pub mod rate_limiter; pub mod shell; +pub mod skills; pub mod token; pub mod updater; pub mod version_manager; diff --git a/ampup/src/main.rs b/ampup/src/main.rs index bf6de88..2f81192 100644 --- a/ampup/src/main.rs +++ b/ampup/src/main.rs @@ -1,4 +1,11 @@ -use ampup::{DEFAULT_DOWNLOAD_JOBS, DEFAULT_REPO, DEFAULT_SELF_REPO, commands}; +use ampup::{ + DEFAULT_DOWNLOAD_JOBS, DEFAULT_REPO, DEFAULT_SELF_REPO, + commands::{ + self, + skills::{SkillSelection, SkillsSourceArgs}, + }, + skills::{Agent, Scope, SkillName}, +}; use console::style; /// The ampd installer and version manager @@ -152,6 +159,12 @@ enum Commands { jobs: usize, }, + /// Install and manage agent skills from the amp repository + Skills { + #[command(subcommand)] + command: SkillsCommands, + }, + /// Manage the ampup executable #[command(name = "self")] SelfCmd { @@ -166,6 +179,103 @@ enum Commands { }, } +#[derive(Debug, clap::Subcommand)] +enum SkillsCommands { + /// Install skills (default: the skills of the active ampd version) + Add { + /// Skill to install; repeat for several. Prompts when neither --skill nor --all is given + #[arg(short, long = "skill", value_name = "NAME")] + skills: Vec, + + /// Install every skill + #[arg(long, conflicts_with = "skills")] + all: bool, + + /// List the available skills without installing them + #[arg(short, long, conflicts_with_all = ["skills", "all", "agents", "global"])] + list: bool, + + /// Agent to install for; repeat for several. Prompts when not given + #[arg(short, long = "agent", value_name = "AGENT")] + agents: Vec, + + /// Install in the home directory instead of the current project + #[arg(short, long)] + global: bool, + + /// Print the result as JSON + #[arg(long)] + json: bool, + + #[command(flatten)] + source: SkillsSourceArgs, + }, + + /// List skills installed by ampup + List { + /// Only list skills for this agent; repeat for several (default: all agents) + #[arg(short, long = "agent", value_name = "AGENT")] + agents: Vec, + + /// List skills in the home directory instead of the current project + #[arg(short, long)] + global: bool, + + /// Print the result as JSON + #[arg(long)] + json: bool, + }, + + /// Reinstall skills installed by ampup (default: from the active ampd version) + Update { + /// Skill to update; repeat for several (default: all installed skills) + #[arg(short, long = "skill", value_name = "NAME")] + skills: Vec, + + /// Only update skills for this agent; repeat for several (default: all agents) + #[arg(short, long = "agent", value_name = "AGENT")] + agents: Vec, + + /// Update skills in the home directory instead of the current project + #[arg(short, long)] + global: bool, + + /// Print the result as JSON + #[arg(long)] + json: bool, + + #[command(flatten)] + source: SkillsSourceArgs, + }, + + /// Remove skills installed by ampup + Remove { + /// Skill to remove; repeat for several. Prompts when neither --skill nor --all is given + #[arg(short, long = "skill", value_name = "NAME")] + skills: Vec, + + /// Remove every skill installed by ampup + #[arg(long, conflicts_with = "skills")] + all: bool, + + /// Only remove skills for this agent; repeat for several (default: all agents) + #[arg(short, long = "agent", value_name = "AGENT")] + agents: Vec, + + /// Remove skills from the home directory instead of the current project + #[arg(short, long)] + global: bool, + + /// Skip the confirmation prompt + #[arg(short, long)] + force: bool, + + /// Print the result as JSON + #[arg(long)] + json: bool, + }, +} + #[derive(Debug, clap::Subcommand)] enum SelfCommands { /// Update ampup itself to the latest version @@ -246,6 +356,11 @@ async fn main() { if let Err(e) = run().await { // Print the error with some custom formatting eprintln!("{} {}", style("✗").red().bold(), e); + // Typed errors describe one level each and carry their cause as a + // source, so print the whole chain + for cause in e.chain().skip(1) { + eprintln!(" {} {}", style("Caused by:").dim(), cause); + } std::process::exit(1); } } @@ -322,6 +437,52 @@ async fn run() -> anyhow::Result<()> { commands::install::run(install_dir, repo, github_token, None, arch, platform, jobs) .await?; } + Some(Commands::Skills { command }) => match command { + SkillsCommands::Add { + skills, + all, + list, + agents, + global, + json, + source, + } => { + if list { + commands::skills::list_available(source, json).await?; + } else { + let selection = SkillSelection::from_flags(skills, all); + commands::skills::add(source, selection, agents, scope_of(global), json) + .await?; + } + } + SkillsCommands::List { + agents, + global, + json, + } => { + commands::skills::list(agents, scope_of(global), json).await?; + } + SkillsCommands::Update { + skills, + agents, + global, + json, + source, + } => { + commands::skills::update(source, skills, agents, scope_of(global), json).await?; + } + SkillsCommands::Remove { + skills, + all, + agents, + global, + force, + json, + } => { + let selection = SkillSelection::from_flags(skills, all); + commands::skills::remove(selection, agents, scope_of(global), force, json).await?; + } + }, Some(Commands::SelfCmd { command }) => match command { SelfCommands::Update { repo, github_token } => { commands::update::run(repo, github_token).await?; @@ -382,3 +543,13 @@ async fn run() -> anyhow::Result<()> { Ok(()) } + +/// The `--global` flag of the skills commands selects the home directory +/// instead of the current project. +fn scope_of(global: bool) -> Scope { + if global { + Scope::Global + } else { + Scope::Project + } +} diff --git a/ampup/src/skills.rs b/ampup/src/skills.rs new file mode 100644 index 0000000..b030b40 --- /dev/null +++ b/ampup/src/skills.rs @@ -0,0 +1,590 @@ +//! Agent skills distributed in the amp repository. +//! +//! Skills live in the repository's `skills/` directory and are not published as +//! release assets, so they are read from the repository source tarball of a git +//! ref. A skill's `SKILL.md` frontmatter can link files that live elsewhere in +//! the repository, such as feature docs and JSON schemas. Those files are +//! copied into the skill's `references/` and `assets/` directories on install, +//! so the installed skill is self-contained. + +use std::{ + collections::{BTreeMap, BTreeSet}, + fs::File, + path::{Path, PathBuf}, +}; + +use fs_err as fs; + +mod agent; +mod archive; +mod frontmatter; +mod lock; +mod skill_name; + +// Only tests inspect the lock file by name +#[cfg(test)] +pub(crate) use self::lock::LOCK_FILE_NAME; +pub use self::{ + agent::{Agent, Scope}, + skill_name::{InvalidSkillNameError, SkillName}, +}; +pub(crate) use self::{ + archive::{ArchiveFile, ReadFilesError}, + frontmatter::{Frontmatter, ParseFrontmatterError}, + lock::{InstalledSkill, LoadSkillsLockError, SaveSkillsLockError, SkillsLock}, +}; + +/// Repository directory that holds one subdirectory per skill. +const SKILLS_REPO_DIR: &str = "skills/"; + +/// The file that makes a directory a skill. +const SKILL_FILE_NAME: &str = "SKILL.md"; + +/// Skill directory that receives files linked under `metadata.references`. +const REFERENCES_DIR: &str = "references"; + +/// Skill directory that receives files linked under `metadata.assets`. +const ASSETS_DIR: &str = "assets"; + +/// A skill ready to install, with its linked files already copied in. +#[derive(Debug, Clone)] +pub(crate) struct Skill { + /// Skill name, which is also its directory name. + pub name: SkillName, + /// What the skill does and when to use it. + pub description: String, + /// Files keyed by their path inside the skill directory (e.g. + /// `SKILL.md`, `references/sql.md`). + pub files: BTreeMap, +} + +/// Read every skill from a repository source tarball file. +/// +/// A skill is a `skills//` directory containing a `SKILL.md`. Files +/// linked from the frontmatter `metadata` are copied into the skill as +/// `references/` and `assets/`. +/// +/// # Errors +/// +/// Fails when the tarball cannot be read, a skill directory name is not a +/// valid [`SkillName`], a `SKILL.md` is not UTF-8 or has invalid frontmatter, +/// or the frontmatter links a path that is not in the repository. See +/// [`LoadSkillsError`]. +pub(crate) fn load_skills(tarball: &File) -> Result, LoadSkillsError> { + let skills_dir_files = + archive::read_files(tarball, archive::Selection::UnderDir(SKILLS_REPO_DIR)) + .map_err(LoadSkillsError::ReadSkillsDir)?; + + let mut files_by_skill: BTreeMap> = BTreeMap::new(); + for (repo_path, file) in skills_dir_files { + let Some(path_in_skills_dir) = repo_path.strip_prefix(SKILLS_REPO_DIR) else { + continue; + }; + + // Files directly inside `skills/`, such as README.md, belong to no skill + let Some((skill_name, path_in_skill)) = path_in_skills_dir.split_once('/') else { + continue; + }; + + files_by_skill + .entry(skill_name.to_string()) + .or_default() + .insert(path_in_skill.to_string(), file); + } + + // Parse every SKILL.md first, so all linked files are read in one pass + let mut skills = Vec::new(); + let mut linked_paths = BTreeSet::new(); + for (dir_name, files) in files_by_skill { + let Some(skill_md) = files.get(SKILL_FILE_NAME) else { + continue; + }; + + let name: SkillName = match dir_name.parse() { + Ok(name) => name, + Err(err) => return Err(LoadSkillsError::InvalidSkillDir(err)), + }; + let skill_md = match std::str::from_utf8(&skill_md.contents) { + Ok(skill_md) => skill_md, + Err(err) => { + return Err(LoadSkillsError::SkillMdNotUtf8 { + skill: name, + source: err, + }); + } + }; + let frontmatter: Frontmatter = match skill_md.parse() { + Ok(frontmatter) => frontmatter, + Err(err) => { + return Err(LoadSkillsError::InvalidFrontmatter { + skill: name, + source: err, + }); + } + }; + + linked_paths.extend(frontmatter.references.iter().cloned()); + linked_paths.extend(frontmatter.assets.iter().cloned()); + + skills.push((name, frontmatter, files)); + } + + let linked_files = archive::read_files(tarball, archive::Selection::Paths(&linked_paths)) + .map_err(LoadSkillsError::ReadLinkedFiles)?; + + let mut loaded_skills = Vec::new(); + for (name, frontmatter, mut files) in skills { + copy_linked_files( + &mut files, + &name, + REFERENCES_DIR, + &frontmatter.references, + &linked_files, + ) + .map_err(LoadSkillsError::LinkedFileNotFound)?; + copy_linked_files( + &mut files, + &name, + ASSETS_DIR, + &frontmatter.assets, + &linked_files, + ) + .map_err(LoadSkillsError::LinkedFileNotFound)?; + + loaded_skills.push(Skill { + name, + description: frontmatter.description, + files, + }); + } + + Ok(loaded_skills) +} + +/// Errors from reading skills out of a repository source tarball +#[derive(Debug, thiserror::Error)] +pub(crate) enum LoadSkillsError { + /// The files under `skills/` could not be read from the tarball + #[error("Failed to read the skills directory from the source tarball")] + ReadSkillsDir(#[source] ReadFilesError), + + /// A skill directory's name is not a valid skill name + /// + /// Occurs when the repository has a skill directory whose name contains a + /// backslash, which could not be installed as a single directory. + #[error("Invalid skill directory in the repository")] + InvalidSkillDir(#[source] InvalidSkillNameError), + + /// A skill's `SKILL.md` is not valid UTF-8 + #[error("skills/{skill}/SKILL.md is not valid UTF-8")] + SkillMdNotUtf8 { + skill: SkillName, + #[source] + source: std::str::Utf8Error, + }, + + /// A skill's `SKILL.md` frontmatter could not be parsed + #[error("Invalid frontmatter in skills/{skill}/SKILL.md")] + InvalidFrontmatter { + skill: SkillName, + #[source] + source: ParseFrontmatterError, + }, + + /// The files linked from skill frontmatter could not be read from the tarball + #[error("Failed to read linked files from the source tarball")] + ReadLinkedFiles(#[source] ReadFilesError), + + /// A skill links a file that does not exist + /// + /// No skill is returned, so nothing is installed with broken links. + #[error("A skill links a file that does not exist")] + LinkedFileNotFound(#[source] LinkedFileNotFoundError), +} + +/// A skills directory and the agents that read from it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct SkillsDir { + /// Absolute path of the directory skills are installed into. + pub path: PathBuf, + /// The agents that read skills from this directory. + pub agents: Vec, +} + +/// Group agents by the skills directory they read from. +/// +/// Agents that share a directory (such as Codex and Cursor in a project) get a +/// single entry, so each directory is installed into only once. +pub(crate) fn skills_dirs(agents: &[Agent], scope: Scope, base_dir: &Path) -> Vec { + let mut dirs: Vec = Vec::new(); + + for &agent in agents { + let path = agent::skills_dir(agent, scope, base_dir); + + match dirs.iter_mut().find(|dir| dir.path == path) { + Some(dir) => { + if !dir.agents.contains(&agent) { + dir.agents.push(agent); + } + } + None => dirs.push(SkillsDir { + path, + agents: vec![agent], + }), + } + } + + dirs +} + +/// Install a skill into `/`, replacing any previous +/// copy. +/// +/// # Errors +/// +/// Fails when the skills directory, the staged skill files, or their +/// permissions cannot be written, when the previous copy cannot be removed, or +/// when the staged skill cannot be moved into place. See [`InstallSkillError`]. +pub(crate) fn install_skill(skills_dir: &Path, skill: &Skill) -> Result<(), InstallSkillError> { + fs::create_dir_all(skills_dir).map_err(InstallSkillError::CreateSkillsDir)?; + + // Write the skill to a hidden staging directory next to its final location, + // so agents never load a half-written skill + let staging_dir = tempfile::Builder::new() + .prefix(".amp-skill-") + .tempdir_in(skills_dir) + .map_err(InstallSkillError::CreateStagingDir)?; + + for (path_in_skill, file) in &skill.files { + let file_path = staging_dir.path().join(path_in_skill); + if let Some(parent) = file_path.parent() { + fs::create_dir_all(parent).map_err(InstallSkillError::CreateFileDir)?; + } + fs::write(&file_path, &file.contents).map_err(InstallSkillError::WriteFile)?; + + #[cfg(unix)] + if file.is_executable { + use std::os::unix::fs::PermissionsExt as _; + fs::set_permissions(&file_path, std::fs::Permissions::from_mode(0o755)) + .map_err(InstallSkillError::SetExecutable)?; + } + } + + uninstall_skill(skills_dir, &skill.name).map_err(InstallSkillError::RemovePreviousCopy)?; + + let skill_dir = skills_dir.join(skill.name.as_str()); + + // `keep()` stops the TempDir from deleting the directory on drop, so it has + // to be cleaned up by hand if the rename fails + let staged_path = staging_dir.keep(); + if let Err(err) = fs::rename(&staged_path, &skill_dir) { + // The rename error is the one the user needs; if this cleanup also + // fails, a hidden `.amp-skill-*` directory is left behind, which agents + // ignore and the next install does not trip over + let _ = fs::remove_dir_all(&staged_path); + return Err(InstallSkillError::MoveIntoPlace { + skill_dir, + source: err, + }); + } + + Ok(()) +} + +/// Errors from installing a skill into a skills directory +/// +/// The previous copy of the skill is removed just before the new one is moved +/// in, so a failure in [`Self::MoveIntoPlace`] leaves the skill uninstalled; +/// running the install again is safe. +#[derive(Debug, thiserror::Error)] +pub(crate) enum InstallSkillError { + /// The skills directory could not be created + #[error("Failed to create skills directory")] + CreateSkillsDir(#[source] std::io::Error), + + /// The hidden staging directory could not be created + #[error("Failed to create skill staging directory")] + CreateStagingDir(#[source] std::io::Error), + + /// A directory inside the staged skill could not be created + #[error("Failed to create skill directory")] + CreateFileDir(#[source] std::io::Error), + + /// A skill file could not be written to the staging directory + #[error("Failed to write skill file")] + WriteFile(#[source] std::io::Error), + + /// A skill script could not be made executable + #[error("Failed to make skill file executable")] + SetExecutable(#[source] std::io::Error), + + /// The previously installed copy could not be removed + #[error("Failed to remove the previous copy of the skill")] + RemovePreviousCopy(#[source] UninstallSkillError), + + /// The staged skill could not be moved into the skills directory + #[error("Failed to move skill into {}", skill_dir.display())] + MoveIntoPlace { + skill_dir: PathBuf, + #[source] + source: std::io::Error, + }, +} + +/// Delete `/` if it exists. +/// +/// # Errors +/// +/// Fails when the skill's metadata cannot be read for a reason other than it +/// not existing, or when the skill cannot be deleted. See +/// [`UninstallSkillError`]. +pub(crate) fn uninstall_skill( + skills_dir: &Path, + skill_name: &SkillName, +) -> Result<(), UninstallSkillError> { + let skill_dir = skills_dir.join(skill_name.as_str()); + + // `symlink_metadata` does not follow symlinks, so a symlinked skill is + // unlinked instead of deleting the directory it points to + let metadata = match fs::symlink_metadata(&skill_dir) { + Ok(metadata) => metadata, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(err) => return Err(UninstallSkillError::ReadMetadata(err)), + }; + + if metadata.is_dir() { + fs::remove_dir_all(&skill_dir).map_err(UninstallSkillError::RemoveDir)?; + } else { + fs::remove_file(&skill_dir).map_err(UninstallSkillError::RemoveFile)?; + } + + Ok(()) +} + +/// Errors from deleting an installed skill +#[derive(Debug, thiserror::Error)] +pub(crate) enum UninstallSkillError { + /// The skill's metadata could not be read + /// + /// Possible causes: + /// - Permission denied on the skills directory + #[error("Failed to read skill metadata")] + ReadMetadata(#[source] std::io::Error), + + /// The skill directory could not be deleted + /// + /// Some of its files may already be gone. + #[error("Failed to remove skill directory")] + RemoveDir(#[source] std::io::Error), + + /// The skill was a file or symlink and could not be deleted + #[error("Failed to remove skill")] + RemoveFile(#[source] std::io::Error), +} + +/// Copy linked repository files into `/` inside a skill. +/// +/// A link keeps only the file name, so `docs/feat/sql.md` listed under +/// `metadata.references` becomes `references/sql.md`. +fn copy_linked_files( + skill_files: &mut BTreeMap, + skill_name: &SkillName, + link_dir: &str, + repo_paths: &[String], + linked_files: &BTreeMap, +) -> Result<(), LinkedFileNotFoundError> { + for repo_path in repo_paths { + let Some(file) = linked_files.get(repo_path) else { + return Err(LinkedFileNotFoundError { + skill: skill_name.clone(), + path: repo_path.clone(), + }); + }; + + let file_name = match repo_path.rsplit_once('/') { + Some((_, file_name)) => file_name, + None => repo_path, + }; + + skill_files.insert(format!("{}/{}", link_dir, file_name), file.clone()); + } + + Ok(()) +} + +/// A skill's frontmatter links a repository path that does not exist +/// +/// Occurs when `metadata.references` or `metadata.assets` in a `SKILL.md` lists +/// a path that was moved or deleted in the repository. +#[derive(Debug, thiserror::Error)] +#[error("Skill {skill} links {path}, which does not exist in the repository")] +pub(crate) struct LinkedFileNotFoundError { + pub skill: SkillName, + pub path: String, +} + +#[cfg(test)] +mod tests { + use anyhow::Result; + use tempfile::TempDir; + + use super::*; + + /// Write a GitHub-style source tarball, with every file under a top-level + /// `owner-repo-commit` directory, to a temporary file. + fn source_tarball(files: &[(&str, &str)]) -> Result { + let encoder = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::default()); + let mut builder = tar::Builder::new(encoder); + + for (repo_path, contents) in files { + let mut header = tar::Header::new_gnu(); + header.set_size(u64::try_from(contents.len())?); + header.set_mode(0o644); + header.set_cksum(); + builder.append_data( + &mut header, + format!("edgeandnode-amp-0123abc/{}", repo_path), + contents.as_bytes(), + )?; + } + + let encoder = builder.into_inner()?; + let tarball = encoder.finish()?; + + let mut file = tempfile::tempfile()?; + std::io::Write::write_all(&mut file, &tarball)?; + Ok(file) + } + + fn file_text(skill: &Skill, path_in_skill: &str) -> Option { + skill + .files + .get(path_in_skill) + .map(|file| String::from_utf8_lossy(&file.contents).into_owned()) + } + + #[test] + fn skills_dirs_with_all_agents_in_project_scope_groups_shared_dirs() { + //* Given + let agents = [Agent::ClaudeCode, Agent::Codex, Agent::Cursor]; + let base_dir = Path::new("/work/project"); + + //* When + let dirs = skills_dirs(&agents, Scope::Project, base_dir); + + //* Then + assert_eq!( + dirs, + vec![ + SkillsDir { + path: "/work/project/.claude/skills".into(), + agents: vec![Agent::ClaudeCode], + }, + SkillsDir { + path: "/work/project/.agents/skills".into(), + agents: vec![Agent::Codex, Agent::Cursor], + }, + ], + "codex and cursor should share the .agents/skills directory" + ); + } + + #[test] + fn load_skills_with_linked_files_copies_them_into_skill() -> Result<()> { + //* Given + let tarball = source_tarball(&[ + ("skills/README.md", "# Skills\n"), + ( + "skills/amp-providers/SKILL.md", + "---\nname: amp-providers\ndescription: Providers.\nmetadata:\n references: docs/feat/provider.md\n assets: docs/schemas/evm-rpc.spec.json\n---\n", + ), + ("skills/amp-providers/references/workflow-setup.md", "setup"), + ("docs/feat/provider.md", "provider doc"), + ("docs/schemas/evm-rpc.spec.json", "{}"), + ("docs/feat/unrelated.md", "unrelated"), + ])?; + + //* When + let skills = load_skills(&tarball)?; + + //* Then + assert_eq!(skills.len(), 1, "only directories with SKILL.md are skills"); + let skill = &skills[0]; + assert_eq!( + skill.name.as_str(), + "amp-providers", + "the skill name should come from its directory" + ); + assert_eq!( + skill.files.keys().collect::>(), + vec![ + "SKILL.md", + "assets/evm-rpc.spec.json", + "references/provider.md", + "references/workflow-setup.md", + ], + "linked files should sit next to the skill's own files" + ); + assert_eq!( + file_text(skill, "references/provider.md").as_deref(), + Some("provider doc"), + "linked reference should hold the repository file contents" + ); + + Ok(()) + } + + #[test] + fn install_skill_with_previous_copy_replaces_its_files() -> Result<()> { + //* Given + let skills_dir = TempDir::new()?; + let stale_file = skills_dir.path().join("amp-query/references/stale.md"); + fs::create_dir_all(skills_dir.path().join("amp-query/references"))?; + fs::write(&stale_file, "stale")?; + + let tarball = source_tarball(&[( + "skills/amp-query/SKILL.md", + "---\nname: amp-query\ndescription: Query.\n---\n", + )])?; + let skills = load_skills(&tarball)?; + + //* When + install_skill(skills_dir.path(), &skills[0])?; + + //* Then + assert!( + skills_dir.path().join("amp-query/SKILL.md").exists(), + "new skill files should be installed" + ); + assert!( + !stale_file.exists(), + "files from the previous copy should be gone" + ); + + Ok(()) + } + + #[test] + fn load_skills_with_missing_linked_file_fails() -> Result<()> { + //* Given + let tarball = source_tarball(&[( + "skills/amp-query/SKILL.md", + "---\nname: amp-query\ndescription: Query.\nmetadata:\n references: docs/feat/missing.md\n---\n", + )])?; + + //* When + let result = load_skills(&tarball); + + //* Then + assert!( + matches!( + &result, + Err(LoadSkillsError::LinkedFileNotFound(LinkedFileNotFoundError { path, .. })) + if path == "docs/feat/missing.md" + ), + "error should name the missing file, got: {result:?}" + ); + + Ok(()) + } +} diff --git a/ampup/src/skills/agent.rs b/ampup/src/skills/agent.rs new file mode 100644 index 0000000..89adfd2 --- /dev/null +++ b/ampup/src/skills/agent.rs @@ -0,0 +1,108 @@ +//! The coding agents ampup installs skills for, and where each one reads them. + +use std::path::{Path, PathBuf}; + +/// The directory an agent reads skills from in a scope. +/// +/// Several agents can share a directory: Codex and Cursor both read project +/// skills from the agent-neutral `.agents/skills`. +/// +/// `base_dir` is the project root for [`Scope::Project`], and the user's home +/// directory for [`Scope::Global`]. +pub fn skills_dir(agent: Agent, scope: Scope, base_dir: &Path) -> PathBuf { + let relative_dir = match (agent, scope) { + (Agent::ClaudeCode, Scope::Project) => ".claude/skills", + (Agent::ClaudeCode, Scope::Global) => ".claude/skills", + (Agent::Codex, Scope::Project) => ".agents/skills", + (Agent::Codex, Scope::Global) => ".codex/skills", + (Agent::Cursor, Scope::Project) => ".agents/skills", + (Agent::Cursor, Scope::Global) => ".cursor/skills", + }; + base_dir.join(relative_dir) +} + +/// A coding agent that ampup can install skills for. +/// +/// Each agent reads skills from its own directory, both inside a project and in +/// the user's home directory. +#[derive(Debug, Clone, Copy, PartialEq, Eq, clap::ValueEnum, serde::Serialize)] +#[serde(rename_all = "kebab-case")] +pub enum Agent { + /// Claude Code + ClaudeCode, + /// OpenAI Codex + Codex, + /// Cursor + Cursor, +} + +impl Agent { + /// All supported agents, in the order they are listed to the user. + pub const ALL: [Self; 3] = [Self::ClaudeCode, Self::Codex, Self::Cursor]; + + /// The agent name as accepted by the `--agent` flag. + pub fn name(self) -> &'static str { + match self { + Self::ClaudeCode => "claude-code", + Self::Codex => "codex", + Self::Cursor => "cursor", + } + } +} + +impl std::fmt::Display for Agent { + /// Formats the agent as its `--agent` flag value. + /// + /// ``` + /// assert_eq!(ampup::skills::Agent::ClaudeCode.to_string(), "claude-code"); + /// ``` + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.name()) + } +} + +/// Where skills are installed. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Scope { + /// Inside the current project, shared with everyone working on it. + Project, + /// In the user's home directory, available in every project. + Global, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn skills_dir_with_codex_in_project_scope_uses_agents_dir() { + //* Given + let project_dir = Path::new("/work/project"); + + //* When + let dir = skills_dir(Agent::Codex, Scope::Project, project_dir); + + //* Then + assert_eq!( + dir, + PathBuf::from("/work/project/.agents/skills"), + "codex project skills should live in the agent-neutral .agents directory" + ); + } + + #[test] + fn skills_dir_with_codex_in_global_scope_uses_codex_dir() { + //* Given + let home_dir = Path::new("/home/user"); + + //* When + let dir = skills_dir(Agent::Codex, Scope::Global, home_dir); + + //* Then + assert_eq!( + dir, + PathBuf::from("/home/user/.codex/skills"), + "codex global skills should live in the .codex directory" + ); + } +} diff --git a/ampup/src/skills/archive.rs b/ampup/src/skills/archive.rs new file mode 100644 index 0000000..157462b --- /dev/null +++ b/ampup/src/skills/archive.rs @@ -0,0 +1,229 @@ +//! Reading selected files out of a GitHub repository source tarball. +//! +//! Entry paths that are not plain relative paths (`..`, absolute paths) are +//! dropped, so nothing read from a tarball can resolve outside the repository. + +use std::{ + collections::{BTreeMap, BTreeSet}, + fs::File, + io::{BufReader, Read as _, Seek as _, SeekFrom}, + path::{Component, Path}, +}; + +/// A regular file read from a repository source tarball. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ArchiveFile { + /// The file's bytes. + pub contents: Vec, + /// Whether any execute bit is set, so scripts stay runnable. + pub is_executable: bool, +} + +/// Which files to read from a source tarball. +/// +/// A repository tarball holds the whole repository, so only the needed files +/// are kept in memory. +pub enum Selection<'a> { + /// Every file under this directory, for example `"skills/"`. + UnderDir(&'a str), + /// Exactly these repository paths. + Paths(&'a BTreeSet), +} + +impl Selection<'_> { + fn contains(&self, repo_path: &str) -> bool { + match self { + Self::UnderDir(dir) => repo_path.starts_with(dir), + Self::Paths(paths) => paths.contains(repo_path), + } + } +} + +/// Read the selected regular files from a GitHub source tarball (`.tar.gz`). +/// +/// GitHub wraps the repository in a single top-level directory named after the +/// repository and commit (e.g. `owner-repo-0123abc/`). That directory is +/// stripped, so the returned map is keyed by repository path (e.g. +/// `skills/amp-query/SKILL.md`). Directories, symlinks, and other entry types +/// are skipped. +/// +/// Reads from the start of `tarball` every time, so the same file can be read +/// once per selection. +/// +/// # Errors +/// +/// Fails when the file cannot be rewound, the tarball is not valid gzip or tar, +/// or an entry's header or contents cannot be read. See [`ReadFilesError`]. +pub fn read_files( + tarball: &File, + selection: Selection, +) -> Result, ReadFilesError> { + // `&File` implements `Read` and `Seek`, so a shared reference is enough to + // rewind the file and read it again + let mut reader = tarball; + reader + .seek(SeekFrom::Start(0)) + .map_err(ReadFilesError::Rewind)?; + + let decoder = flate2::read::GzDecoder::new(BufReader::new(reader)); + let mut archive = tar::Archive::new(decoder); + + let mut files = BTreeMap::new(); + + let entries = archive.entries().map_err(ReadFilesError::Entries)?; + for entry in entries { + let mut entry = entry.map_err(ReadFilesError::Entry)?; + if !entry.header().entry_type().is_file() { + continue; + } + + let entry_path = entry + .path() + .map_err(ReadFilesError::EntryPath)? + .into_owned(); + let Some(repo_path) = repo_path_of(&entry_path) else { + continue; + }; + if !selection.contains(&repo_path) { + continue; + } + + let mode = match entry.header().mode() { + Ok(mode) => mode, + Err(err) => { + return Err(ReadFilesError::EntryMode { + path: repo_path, + source: err, + }); + } + }; + + let mut contents = Vec::new(); + if let Err(err) = entry.read_to_end(&mut contents) { + return Err(ReadFilesError::EntryContents { + path: repo_path, + source: err, + }); + } + + files.insert( + repo_path, + ArchiveFile { + contents, + is_executable: mode & 0o111 != 0, + }, + ); + } + + Ok(files) +} + +/// Errors from reading files out of a source tarball +#[derive(Debug, thiserror::Error)] +pub enum ReadFilesError { + /// The tarball file could not be rewound to its start + #[error("Failed to rewind source tarball file")] + Rewind(#[source] std::io::Error), + + /// The tarball's entry list could not be opened + /// + /// Occurs when the download is not a gzip-compressed tar archive. + #[error("Failed to read source tarball")] + Entries(#[source] std::io::Error), + + /// An entry header could not be read + /// + /// Possible causes: + /// - The download was truncated + /// - The archive is corrupted + #[error("Failed to read source tarball entry")] + Entry(#[source] std::io::Error), + + /// An entry's path could not be decoded + #[error("Source tarball entry has an invalid path")] + EntryPath(#[source] std::io::Error), + + /// A selected entry's file mode could not be parsed from its header + #[error("Source tarball entry {path} has an invalid mode")] + EntryMode { + path: String, + #[source] + source: std::io::Error, + }, + + /// A selected entry's contents could not be read + /// + /// Occurs when the archive ends before the size its header declares. + #[error("Failed to read {path} from source tarball")] + EntryContents { + path: String, + #[source] + source: std::io::Error, + }, +} + +/// Turn a tarball entry path into a repository path by dropping the top-level +/// directory. +/// +/// Returns `None` for entries outside that directory, and for paths that are +/// not plain relative paths (such as `..` or absolute paths), so a malformed +/// tarball can never make ampup write outside the skills directory. +fn repo_path_of(entry_path: &Path) -> Option { + let mut components = entry_path.components(); + + // GitHub names the top-level directory after the repository and commit, so + // its name carries nothing to check + let Some(Component::Normal(_)) = components.next() else { + return None; + }; + + let mut parts = Vec::new(); + for component in components { + let Component::Normal(part) = component else { + return None; + }; + parts.push(part.to_str()?); + } + + if parts.is_empty() { + return None; + } + + Some(parts.join("/")) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn repo_path_of_with_top_level_dir_strips_it() { + //* Given + let entry_path = Path::new("owner-repo-0123abc/skills/amp-query/SKILL.md"); + + //* When + let repo_path = repo_path_of(entry_path); + + //* Then + assert_eq!( + repo_path.as_deref(), + Some("skills/amp-query/SKILL.md"), + "the top-level tarball directory should be stripped" + ); + } + + #[test] + fn repo_path_of_with_parent_dir_component_returns_none() { + //* Given + let entry_path = Path::new("owner-repo-0123abc/skills/../../etc/passwd"); + + //* When + let repo_path = repo_path_of(entry_path); + + //* Then + assert_eq!( + repo_path, None, + "paths escaping the repository should be ignored" + ); + } +} diff --git a/ampup/src/skills/frontmatter.rs b/ampup/src/skills/frontmatter.rs new file mode 100644 index 0000000..2c2ddc7 --- /dev/null +++ b/ampup/src/skills/frontmatter.rs @@ -0,0 +1,192 @@ +//! Parsing the YAML frontmatter of a `SKILL.md` file. + +/// The parts of a `SKILL.md` frontmatter that ampup needs. +/// +/// Parse one from a whole `SKILL.md` file with [`str::parse`]. The frontmatter +/// is the block between the opening `---` line and the next `---` line. See the +/// [Agent Skills specification](https://agentskills.io/specification). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Frontmatter { + /// What the skill does and when to use it. + pub description: String, + /// Repository paths of files the skill links as `references/`. + pub references: Vec, + /// Repository paths of files the skill links as `assets/`. + pub assets: Vec, +} + +impl std::str::FromStr for Frontmatter { + type Err = ParseFrontmatterError; + + fn from_str(skill_md: &str) -> Result { + let mut lines = skill_md.lines(); + + let Some(first_line) = lines.next() else { + return Err(ParseFrontmatterError::Empty); + }; + if first_line.trim_end() != "---" { + return Err(ParseFrontmatterError::MissingOpeningDelimiter); + } + + let mut yaml = String::new(); + let mut is_terminated = false; + for line in lines { + if line.trim_end() == "---" { + is_terminated = true; + break; + } + yaml.push_str(line); + yaml.push('\n'); + } + if !is_terminated { + return Err(ParseFrontmatterError::MissingClosingDelimiter); + } + + let raw: RawFrontmatter = match serde_saphyr::from_str(&yaml) { + Ok(raw) => raw, + Err(err) => return Err(ParseFrontmatterError::InvalidYaml(Box::new(err))), + }; + + Ok(Self { + description: raw.description, + references: split_paths(&raw.metadata.references), + assets: split_paths(&raw.metadata.assets), + }) + } +} + +/// Errors from parsing a `SKILL.md` frontmatter block +#[derive(Debug, thiserror::Error)] +pub enum ParseFrontmatterError { + /// The file has no content + #[error("File is empty")] + Empty, + + /// The file does not start with a `---` line + /// + /// Occurs for a `SKILL.md` without frontmatter, which the Agent Skills + /// specification requires. + #[error("File does not start with a '---' frontmatter block")] + MissingOpeningDelimiter, + + /// The frontmatter block has no closing `---` line + #[error("Frontmatter block is not closed with '---'")] + MissingClosingDelimiter, + + /// The frontmatter block is not valid YAML, or a field has the wrong type + /// + /// Occurs, for example, when a `metadata` value is a list instead of the + /// space-separated string the specification requires. + // Boxed because the YAML error is large and would bloat every `Result` + // carrying this error + #[error("Frontmatter is not valid YAML")] + InvalidYaml(#[source] Box), +} + +#[derive(serde::Deserialize)] +struct RawFrontmatter { + #[serde(default)] + description: String, + #[serde(default)] + metadata: RawMetadata, +} + +/// The specification requires `metadata` values to be strings, so each linked +/// file list is a single space-separated string. +#[derive(Default, serde::Deserialize)] +struct RawMetadata { + #[serde(default)] + references: String, + #[serde(default)] + assets: String, +} + +fn split_paths(paths: &str) -> Vec { + paths.split_whitespace().map(str::to_string).collect() +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn from_str_with_linked_files_returns_space_separated_paths() + -> Result<(), ParseFrontmatterError> { + //* Given + let skill_md = "---\n\ + name: amp-providers\n\ + description: Configure providers.\n\ + metadata:\n \ + references: docs/feat/provider.md docs/feat/provider-evm-rpc.md\n \ + assets: docs/schemas/providers/evm-rpc.spec.json\n\ + ---\n\ + \n\ + # Providers\n"; + + //* When + let frontmatter = skill_md.parse::()?; + + //* Then + assert_eq!( + frontmatter, + Frontmatter { + description: "Configure providers.".to_string(), + references: vec![ + "docs/feat/provider.md".to_string(), + "docs/feat/provider-evm-rpc.md".to_string(), + ], + assets: vec!["docs/schemas/providers/evm-rpc.spec.json".to_string()], + }, + "linked files should be split on whitespace" + ); + + Ok(()) + } + + #[test] + fn from_str_without_metadata_returns_no_linked_files() -> Result<(), ParseFrontmatterError> { + //* Given + let skill_md = "---\nname: amp-system\ndescription: Inspect the system.\n---\n# System\n"; + + //* When + let frontmatter = skill_md.parse::()?; + + //* Then + assert!( + frontmatter.references.is_empty() && frontmatter.assets.is_empty(), + "a skill without metadata should link no files" + ); + + Ok(()) + } + + #[test] + fn from_str_without_frontmatter_block_fails() { + //* Given + let skill_md = "# Just a heading\n"; + + //* When + let result = skill_md.parse::(); + + //* Then + assert!( + matches!(result, Err(ParseFrontmatterError::MissingOpeningDelimiter)), + "a SKILL.md without frontmatter should be rejected" + ); + } + + #[test] + fn from_str_with_unclosed_frontmatter_block_fails() { + //* Given + let skill_md = "---\nname: amp-system\n"; + + //* When + let result = skill_md.parse::(); + + //* Then + assert!( + matches!(result, Err(ParseFrontmatterError::MissingClosingDelimiter)), + "an unclosed frontmatter block should be rejected" + ); + } +} diff --git a/ampup/src/skills/lock.rs b/ampup/src/skills/lock.rs new file mode 100644 index 0000000..95ab14e --- /dev/null +++ b/ampup/src/skills/lock.rs @@ -0,0 +1,208 @@ +//! The lock file recording which skills ampup installed in a skills directory. +//! +//! Skills missing from the lock file were not installed by ampup, and ampup +//! never updates or removes them. + +use std::{ + collections::BTreeMap, + path::{Path, PathBuf}, +}; + +use fs_err as fs; + +use super::SkillName; + +/// Name of the file that records which skills ampup installed in a directory. +pub const LOCK_FILE_NAME: &str = ".amp-skills.json"; + +/// The skills ampup installed in one skills directory. +/// +/// Stored as [`LOCK_FILE_NAME`] inside that directory. Skills that are not +/// listed here were not installed by ampup, and ampup never updates or removes +/// them. +#[derive(Debug, Default, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +pub struct SkillsLock { + /// Installed skills, keyed by skill name. + /// + /// Keys are validated on load, so a tampered lock file cannot name a path + /// outside the skills directory. + pub skills: BTreeMap, +} + +/// Where an installed skill was downloaded from. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +pub struct InstalledSkill { + /// GitHub repository in format `owner/repo`. + pub repo: String, + /// The ref the user asked for (tag, branch, or commit). + #[serde(rename = "ref")] + pub git_ref: String, + /// The commit the ref pointed to at install time. + pub commit: String, +} + +impl SkillsLock { + /// Load the lock file from a skills directory. + /// + /// Returns an empty lock when the file does not exist. + /// + /// # Errors + /// + /// Fails when the file cannot be read, is not valid JSON, or names a skill + /// that is not a valid [`SkillName`]. See [`LoadSkillsLockError`]. + pub fn load(skills_dir: &Path) -> Result { + let lock_path = skills_dir.join(LOCK_FILE_NAME); + if !lock_path.exists() { + return Ok(Self::default()); + } + + let contents = fs::read_to_string(&lock_path).map_err(LoadSkillsLockError::Read)?; + serde_json::from_str(&contents).map_err(|err| LoadSkillsLockError::Parse { + path: lock_path, + source: err, + }) + } + + /// Save the lock file to a skills directory. + /// + /// Deletes the file instead when no skills are left. + /// + /// # Errors + /// + /// Fails when the skills directory or the file cannot be written, or the + /// file cannot be deleted. See [`SaveSkillsLockError`]. + pub fn save(&self, skills_dir: &Path) -> Result<(), SaveSkillsLockError> { + let lock_path = skills_dir.join(LOCK_FILE_NAME); + + if self.skills.is_empty() { + if lock_path.exists() { + fs::remove_file(&lock_path).map_err(SaveSkillsLockError::Remove)?; + } + return Ok(()); + } + + let mut contents = + serde_json::to_string_pretty(self).map_err(SaveSkillsLockError::Serialize)?; + contents.push('\n'); + + fs::create_dir_all(skills_dir).map_err(SaveSkillsLockError::CreateSkillsDir)?; + fs::write(&lock_path, contents).map_err(SaveSkillsLockError::Write)?; + + Ok(()) + } +} + +/// Errors from loading a skills lock file +#[derive(Debug, thiserror::Error)] +pub enum LoadSkillsLockError { + /// The lock file exists but could not be read + /// + /// Possible causes: + /// - Permission denied + /// - The path is a directory + #[error("Failed to read skills lock file")] + Read(#[source] std::io::Error), + + /// The lock file is not valid JSON, or names an invalid skill + /// + /// Occurs when the file was edited by hand, including a skill name that is + /// `..` or contains a path separator. Nothing is removed or overwritten + /// when this happens. + #[error("Invalid skills lock file {}", path.display())] + Parse { + path: PathBuf, + #[source] + source: serde_json::Error, + }, +} + +/// Errors from saving a skills lock file +#[derive(Debug, thiserror::Error)] +pub enum SaveSkillsLockError { + /// The lock could not be serialized to JSON + #[error("Failed to serialize skills lock file")] + Serialize(#[source] serde_json::Error), + + /// The skills directory holding the lock file could not be created + #[error("Failed to create skills directory")] + CreateSkillsDir(#[source] std::io::Error), + + /// The lock file could not be written + #[error("Failed to write skills lock file")] + Write(#[source] std::io::Error), + + /// The empty lock file could not be deleted + #[error("Failed to remove skills lock file")] + Remove(#[source] std::io::Error), +} + +#[cfg(test)] +mod tests { + use anyhow::Result; + use tempfile::TempDir; + + use super::*; + + #[test] + fn load_with_saved_lock_returns_same_skills() -> Result<()> { + //* Given + let skills_dir = TempDir::new()?; + let mut lock = SkillsLock::default(); + lock.skills.insert( + "amp-query".parse()?, + InstalledSkill { + repo: "edgeandnode/amp".to_string(), + git_ref: "v0.1.0".to_string(), + commit: "0123456789abcdef".to_string(), + }, + ); + lock.save(skills_dir.path())?; + + //* When + let loaded = SkillsLock::load(skills_dir.path())?; + + //* Then + assert_eq!(loaded, lock, "loaded lock should match the saved lock"); + + Ok(()) + } + + #[test] + fn save_with_no_skills_removes_lock_file() -> Result<()> { + //* Given + let skills_dir = TempDir::new()?; + let lock_path = skills_dir.path().join(LOCK_FILE_NAME); + fs::write(&lock_path, r#"{"skills":{}}"#)?; + + //* When + SkillsLock::default().save(skills_dir.path())?; + + //* Then + assert!( + !lock_path.exists(), + "an empty lock should not leave a lock file behind" + ); + + Ok(()) + } + + #[test] + fn load_with_path_traversal_skill_name_fails() -> Result<()> { + //* Given + let skills_dir = TempDir::new()?; + let tampered_lock = + r#"{"skills":{"../..":{"repo":"edgeandnode/amp","ref":"v0.1.0","commit":"0123"}}}"#; + fs::write(skills_dir.path().join(LOCK_FILE_NAME), tampered_lock)?; + + //* When + let result = SkillsLock::load(skills_dir.path()); + + //* Then + assert!( + matches!(result, Err(LoadSkillsLockError::Parse { .. })), + "a lock file naming a path outside the skills directory must be rejected" + ); + + Ok(()) + } +} diff --git a/ampup/src/skills/skill_name.rs b/ampup/src/skills/skill_name.rs new file mode 100644 index 0000000..6b44fcc --- /dev/null +++ b/ampup/src/skills/skill_name.rs @@ -0,0 +1,131 @@ +//! The name of a skill, which is also the name of its directory. +//! +//! Skill names come from outside ampup (the repository tarball, the lock file, +//! and `--skill` flags) and are joined onto a skills directory before files are +//! written or deleted. A [`SkillName`] is always a single plain path component, +//! so it can never point outside the skills directory. + +/// A validated skill name: a single path component, never `.`, `..`, or a path +/// with separators. +/// +/// Parse one with [`str::parse`]; that is the only way to construct it. +#[derive( + Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, serde::Serialize, serde::Deserialize, +)] +#[serde(try_from = "String", into = "String")] +pub struct SkillName(String); + +impl SkillName { + /// The name as a string slice. + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl std::str::FromStr for SkillName { + type Err = InvalidSkillNameError; + + fn from_str(name: &str) -> Result { + let is_single_component = + !name.is_empty() && name != "." && name != ".." && !name.contains(['/', '\\']); + + if !is_single_component { + return Err(InvalidSkillNameError { + name: name.to_string(), + }); + } + + Ok(Self(name.to_string())) + } +} + +impl TryFrom for SkillName { + type Error = InvalidSkillNameError; + + fn try_from(name: String) -> Result { + name.parse() + } +} + +impl From for String { + fn from(name: SkillName) -> Self { + name.0 + } +} + +impl std::fmt::Display for SkillName { + /// Formats the name as written in the repository. + /// + /// ``` + /// let name: ampup::skills::SkillName = "amp-query".parse().unwrap(); + /// assert_eq!(name.to_string(), "amp-query"); + /// ``` + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.0) + } +} + +/// A string that is not a valid skill name +/// +/// Occurs when a name is empty, `.`, `..`, or contains a path separator, so +/// using it as a directory name could reach outside the skills directory. +#[derive(Debug, thiserror::Error)] +#[error("Invalid skill name '{name}': a skill name must be a single directory name")] +pub struct InvalidSkillNameError { + /// The rejected name. + pub name: String, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn from_str_with_plain_name_succeeds() { + //* Given + let name = "amp-query"; + + //* When + let result = name.parse::(); + + //* Then + assert_eq!( + result + .map(|skill_name| skill_name.to_string()) + .ok() + .as_deref(), + Some("amp-query"), + "a plain directory name should be a valid skill name" + ); + } + + #[test] + fn from_str_with_parent_dir_fails() { + //* Given + let name = ".."; + + //* When + let result = name.parse::(); + + //* Then + assert!( + result.is_err(), + "'..' must be rejected so it cannot escape the skills directory" + ); + } + + #[test] + fn from_str_with_path_separator_fails() { + //* Given + let name = "../outside"; + + //* When + let result = name.parse::(); + + //* Then + assert!( + result.is_err(), + "a name with a separator must be rejected so it cannot escape the skills directory" + ); + } +} diff --git a/ampup/src/tests/it_skills.rs b/ampup/src/tests/it_skills.rs new file mode 100644 index 0000000..93d2711 --- /dev/null +++ b/ampup/src/tests/it_skills.rs @@ -0,0 +1,1044 @@ +//! Integration tests for the `ampup skills` commands. +//! +//! Each test drives a command end to end — resolve the git ref, download the +//! repository source tarball, read the skills, copy linked files, and write +//! the skills directories and lock file — against an in-process mock GitHub +//! server, so no network is required. Tests call the command functions that +//! take an injected GitHub client and install location, which are exactly what +//! the CLI entry points call after building those from flags. + +use std::{ + num::NonZeroU32, + path::{Path, PathBuf}, +}; + +use anyhow::Result; +use flate2::{Compression, write::GzEncoder}; +use fs_err as fs; +use tar::{Builder, Header}; +use tempfile::TempDir; +use tokio_util::task::AbortOnDropHandle; + +use super::{fixtures::TempInstallDir, mock_github}; +use crate::{ + commands::skills::{ + self, AddSkillsError, DownloadSnapshotError, InstallFromSnapshotError, + LoadSnapshotSkillsError, RemoveSkillsError, ResolveSourceRefError, SelectSkillsError, + SkillAction, SkillSelection, SkillsLocation, SourceRef, + }, + config::Config, + github::{GitHubClient, GitHubError}, + skills::{ + Agent, InstalledSkill, LOCK_FILE_NAME, LinkedFileNotFoundError, LoadSkillsError, Scope, + SkillName, SkillsLock, + }, +}; + +/// Commit the mock GitHub API resolves `v1.0.0` to. +const COMMIT_V1: &str = "1111111111111111111111111111111111111111"; + +/// Commit the mock GitHub API resolves `v2.0.0` to. +const COMMIT_V2: &str = "2222222222222222222222222222222222222222"; + +/// Head commit of the mock pull request. +const COMMIT_PR: &str = "4242424242424242424242424242424242424242"; + +/// Number of the mock pull request. +const PR_NUMBER: u32 = 42; + +/// Repository name reported by a mock GitHub client. +const MOCK_REPO: &str = "test/repo"; + +mod list_available_skills { + use super::*; + + #[tokio::test] + async fn list_available_skills_with_ref_returns_names_and_descriptions() -> Result<()> { + //* Given + let env = TestEnv::create()?; + let (github, _server) = start_mock_github(vec![MockRef::v1()]).await?; + + //* When + let available = skills::list_available_skills( + &github, + &env.config()?, + SourceRef::Git("v1.0.0".to_string()), + ) + .await?; + + //* Then + let skills: Vec<(&str, &str)> = available + .skills + .iter() + .map(|skill| (skill.name.as_str(), skill.description.as_str())) + .collect(); + assert_eq!( + skills, + vec![ + ("amp-query", "Query Amp data."), + ("amp-setup", "Set up Amp.") + ], + "every skill should be listed with its frontmatter description" + ); + assert!( + !env.project_path(".claude").exists() && !env.project_path(".agents").exists(), + "listing should not install anything" + ); + + Ok(()) + } +} + +mod add_skills { + use super::*; + + #[tokio::test] + async fn add_skills_without_ref_installs_active_version() -> Result<()> { + //* Given + let env = TestEnv::create()?; + env.set_active_version("v1.0.0")?; + let (github, _server) = start_mock_github(vec![MockRef::v1()]).await?; + + //* When + let changes = skills::add_skills( + &github, + &env.config()?, + &env.project(), + SourceRef::ActiveVersion, + named(&["amp-query"])?, + vec![Agent::ClaudeCode], + ) + .await?; + + //* Then + let skills_dir = env.project_path(".claude/skills"); + assert_eq!( + files_under(&skills_dir)?, + vec![ + LOCK_FILE_NAME, + "amp-query/SKILL.md", + "amp-query/assets/evm-rpc.spec.json", + "amp-query/references/app-ampsql.md", + "amp-query/references/evm-rpc.md", + "amp-query/references/workflow-decoding.md", + "amp-query/scripts/check.sh", + ], + "only the selected skill should be installed, with its linked files in references/ and assets/" + ); + assert_eq!( + fs::read_to_string(skills_dir.join("amp-query/references/app-ampsql.md"))?, + "ampsql doc v1.0.0", + "linked files should come from the active version" + ); + assert_eq!( + SkillsLock::load(&skills_dir)? + .skills + .get(&"amp-query".parse()?), + Some(&installed_from("v1.0.0", COMMIT_V1)), + "the lock should record the ref and the commit it resolved to" + ); + assert_eq!( + changes + .changes + .iter() + .map(|change| ( + change.action, + change.skill.as_str(), + change.skills_dir.as_str() + )) + .collect::>(), + vec![(SkillAction::Installed, "amp-query", ".claude/skills")], + "the result should report the installed skill and where it went" + ); + + Ok(()) + } + + #[tokio::test] + async fn add_skills_with_all_for_codex_and_cursor_uses_shared_dir() -> Result<()> { + //* Given + let env = TestEnv::create()?; + let (github, _server) = start_mock_github(vec![MockRef::v1()]).await?; + + //* When + skills::add_skills( + &github, + &env.config()?, + &env.project(), + SourceRef::Git("v1.0.0".to_string()), + SkillSelection::All, + vec![Agent::Codex, Agent::Cursor], + ) + .await?; + + //* Then + let skills_dir = env.project_path(".agents/skills"); + assert_eq!( + installed_names(&skills_dir)?, + vec!["amp-query", "amp-setup"], + "every skill should be installed, and README.md is not a skill" + ); + assert!( + skills_dir.join("amp-setup/SKILL.md").exists() + && skills_dir.join("amp-query/SKILL.md").exists(), + "codex and cursor should share the project .agents/skills directory" + ); + assert!( + !env.project_path(".claude").exists(), + "agents that were not selected should get nothing" + ); + + Ok(()) + } + + #[tokio::test] + async fn add_skills_with_global_scope_installs_in_home_dir() -> Result<()> { + //* Given + let env = TestEnv::create()?; + let home_dir = TempDir::new()?; + let (github, _server) = start_mock_github(vec![MockRef::v1()]).await?; + let global = SkillsLocation { + scope: Scope::Global, + base_dir: home_dir.path().to_path_buf(), + }; + + //* When + skills::add_skills( + &github, + &env.config()?, + &global, + SourceRef::Git("v1.0.0".to_string()), + named(&["amp-setup"])?, + vec![Agent::Cursor], + ) + .await?; + + //* Then + assert!( + home_dir + .path() + .join(".cursor/skills/amp-setup/SKILL.md") + .exists(), + "global cursor skills should be installed in ~/.cursor/skills" + ); + assert!( + !home_dir.path().join(".agents").exists(), + "the project-only .agents directory should not be used for global skills" + ); + + Ok(()) + } + + #[tokio::test] + async fn add_skills_with_explicit_ref_installs_that_ref() -> Result<()> { + //* Given + let env = TestEnv::create()?; + env.set_active_version("v1.0.0")?; + let (github, _server) = start_mock_github(vec![MockRef::v1(), MockRef::v2()]).await?; + + //* When + skills::add_skills( + &github, + &env.config()?, + &env.project(), + SourceRef::Git("v2.0.0".to_string()), + named(&["amp-setup"])?, + vec![Agent::ClaudeCode], + ) + .await?; + + //* Then + let skills_dir = env.project_path(".claude/skills"); + assert_eq!( + fs::read_to_string(skills_dir.join("amp-setup/SKILL.md"))?, + "---\nname: amp-setup\ndescription: Set up Amp.\n---\n# Setup v2.0.0\n", + "--ref should win over the active ampd version" + ); + assert_eq!( + SkillsLock::load(&skills_dir)? + .skills + .get(&"amp-setup".parse()?), + Some(&installed_from("v2.0.0", COMMIT_V2)), + "the lock should record the explicit ref" + ); + + Ok(()) + } + + #[tokio::test] + async fn add_skills_with_pull_request_installs_its_head_commit() -> Result<()> { + //* Given + let env = TestEnv::create()?; + env.set_active_version("v1.0.0")?; + let (github, _server) = + start_mock_github(vec![MockRef::v1(), MockRef::pull_request()]).await?; + let pull_request = SourceRef::PullRequest(NonZeroU32::try_from(PR_NUMBER)?); + + //* When + skills::add_skills( + &github, + &env.config()?, + &env.project(), + pull_request, + named(&["amp-query"])?, + vec![Agent::ClaudeCode], + ) + .await?; + + //* Then + let skills_dir = env.project_path(".claude/skills"); + assert_eq!( + fs::read_to_string(skills_dir.join("amp-query/references/app-ampsql.md"))?, + "ampsql doc pull request", + "skills should come from the pull request head, not the active version" + ); + assert_eq!( + SkillsLock::load(&skills_dir)? + .skills + .get(&"amp-query".parse()?), + Some(&installed_from("pr/42", COMMIT_PR)), + "the lock should record the pull request and its head commit" + ); + + Ok(()) + } + + #[tokio::test] + async fn add_skills_with_existing_user_skill_leaves_it_untouched() -> Result<()> { + //* Given + let env = TestEnv::create()?; + let user_skill_md = env.project_path(".claude/skills/amp-setup/SKILL.md"); + fs::create_dir_all(env.project_path(".claude/skills/amp-setup"))?; + fs::write(&user_skill_md, "my own amp-setup skill")?; + let (github, _server) = start_mock_github(vec![MockRef::v1()]).await?; + + //* When + skills::add_skills( + &github, + &env.config()?, + &env.project(), + SourceRef::Git("v1.0.0".to_string()), + SkillSelection::All, + vec![Agent::ClaudeCode], + ) + .await?; + + //* Then + assert_eq!( + fs::read_to_string(&user_skill_md)?, + "my own amp-setup skill", + "a skill ampup did not install must never be overwritten" + ); + assert_eq!( + installed_names(&env.project_path(".claude/skills"))?, + vec!["amp-query"], + "the other skills should still be installed, and the skipped one not recorded" + ); + + Ok(()) + } + + #[cfg(unix)] + #[tokio::test] + async fn add_skills_with_executable_script_keeps_it_executable() -> Result<()> { + use std::os::unix::fs::PermissionsExt as _; + + //* Given + const EXECUTE_BITS: u32 = 0o111; + let env = TestEnv::create()?; + let (github, _server) = start_mock_github(vec![MockRef::v1()]).await?; + + //* When + skills::add_skills( + &github, + &env.config()?, + &env.project(), + SourceRef::Git("v1.0.0".to_string()), + named(&["amp-query"])?, + vec![Agent::ClaudeCode], + ) + .await?; + + //* Then + let skills_dir = env.project_path(".claude/skills"); + let script_mode = fs::metadata(skills_dir.join("amp-query/scripts/check.sh"))? + .permissions() + .mode(); + let doc_mode = fs::metadata(skills_dir.join("amp-query/SKILL.md"))? + .permissions() + .mode(); + assert_ne!( + script_mode & EXECUTE_BITS, + 0, + "skill scripts should stay runnable" + ); + assert_eq!( + doc_mode & EXECUTE_BITS, + 0, + "skill docs should not become executable" + ); + + Ok(()) + } + + #[tokio::test] + async fn add_skills_without_ref_or_active_version_fails() -> Result<()> { + //* Given + let env = TestEnv::create()?; + // No mock server: the command must fail before making any request + let github = GitHubClient::with_api_base("http://127.0.0.1:1".to_string(), None)?; + + //* When + let result = skills::add_skills( + &github, + &env.config()?, + &env.project(), + SourceRef::ActiveVersion, + named(&["amp-setup"])?, + vec![Agent::ClaudeCode], + ) + .await; + + //* Then + assert!( + matches!( + result, + Err(AddSkillsError::ResolveSourceRef( + ResolveSourceRefError::NoActiveVersion + )) + ), + "error should explain that no ampd version is active, got: {result:?}" + ); + + Ok(()) + } + + #[tokio::test] + async fn add_skills_with_unknown_ref_fails_without_writing() -> Result<()> { + //* Given + let env = TestEnv::create()?; + let (github, _server) = start_mock_github(vec![MockRef::v1()]).await?; + + //* When + let result = skills::add_skills( + &github, + &env.config()?, + &env.project(), + SourceRef::Git("v9.9.9".to_string()), + named(&["amp-setup"])?, + vec![Agent::ClaudeCode], + ) + .await; + + //* Then + let is_ref_not_found = match &result { + Err(AddSkillsError::Download(DownloadSnapshotError::ResolveCommit(err))) => matches!( + err.downcast_ref::(), + Some(GitHubError::RefNotFound { git_ref, .. }) if git_ref == "v9.9.9" + ), + _ => false, + }; + assert!( + is_ref_not_found, + "error should name the missing ref, got: {result:?}" + ); + assert!( + !env.project_path(".claude").exists(), + "nothing should be written when the ref does not exist" + ); + + Ok(()) + } + + #[tokio::test] + async fn add_skills_with_unknown_pull_request_fails_without_writing() -> Result<()> { + //* Given + let env = TestEnv::create()?; + let (github, _server) = start_mock_github(vec![MockRef::pull_request()]).await?; + let unknown_pull_request = SourceRef::PullRequest(NonZeroU32::try_from(999)?); + + //* When + let result = skills::add_skills( + &github, + &env.config()?, + &env.project(), + unknown_pull_request, + named(&["amp-setup"])?, + vec![Agent::ClaudeCode], + ) + .await; + + //* Then + let is_pull_request_not_found = match &result { + Err(AddSkillsError::Download(DownloadSnapshotError::ResolveCommit(err))) => matches!( + err.downcast_ref::(), + Some(GitHubError::PullRequestNotFound { number, .. }) if number.get() == 999 + ), + _ => false, + }; + assert!( + is_pull_request_not_found, + "error should name the missing pull request, got: {result:?}" + ); + assert!( + !env.project_path(".claude").exists(), + "nothing should be written when the pull request does not exist" + ); + + Ok(()) + } + + #[tokio::test] + async fn add_skills_with_unknown_skill_name_fails_without_installing() -> Result<()> { + //* Given + let env = TestEnv::create()?; + let (github, _server) = start_mock_github(vec![MockRef::v1()]).await?; + + //* When + let result = skills::add_skills( + &github, + &env.config()?, + &env.project(), + SourceRef::Git("v1.0.0".to_string()), + named(&["amp-setup", "amp-typo"])?, + vec![Agent::ClaudeCode], + ) + .await; + + //* Then + assert!( + matches!( + &result, + Err(AddSkillsError::Install(InstallFromSnapshotError::SelectSkills( + SelectSkillsError::SkillNotFound { name, available, .. } + ))) if name.as_str() == "amp-typo" && available.len() == 2 + ), + "error should name the missing skill and list the available ones, got: {result:?}" + ); + assert!( + !env.project_path(".claude").exists(), + "valid skills in the same request should not be installed either" + ); + + Ok(()) + } + + #[tokio::test] + async fn add_skills_with_broken_linked_file_fails_without_installing() -> Result<()> { + //* Given + let env = TestEnv::create()?; + let mut files = amp_repo("v1.0.0"); + files.retain(|file| file.path != "docs/feat/app-ampsql.md"); + let (github, _server) = start_mock_github(vec![MockRef { + git_ref: "v1.0.0", + commit: COMMIT_V1, + files, + pull_request: None, + }]) + .await?; + + //* When + let result = skills::add_skills( + &github, + &env.config()?, + &env.project(), + SourceRef::Git("v1.0.0".to_string()), + SkillSelection::All, + vec![Agent::ClaudeCode], + ) + .await; + + //* Then + assert!( + matches!( + &result, + Err(AddSkillsError::Install(InstallFromSnapshotError::LoadSkills( + LoadSnapshotSkillsError::LoadSkills(LoadSkillsError::LinkedFileNotFound( + LinkedFileNotFoundError { skill, path } + )) + ))) if skill.as_str() == "amp-query" && path == "docs/feat/app-ampsql.md" + ), + "error should name the skill and the missing file, got: {result:?}" + ); + assert!( + !env.project_path(".claude").exists(), + "no skill should be installed with broken links" + ); + + Ok(()) + } +} + +mod list_installed_skills { + use super::*; + + #[tokio::test] + async fn list_installed_skills_with_installed_skills_groups_them_by_dir() -> Result<()> { + //* Given + let env = TestEnv::create()?; + let (github, _server) = start_mock_github(vec![MockRef::v1()]).await?; + install_v1_for_claude_code(&env, &github, &["amp-query", "amp-setup"]).await?; + + //* When + let installed = skills::list_installed_skills(&env.project(), &[])?; + + //* Then + let dirs: Vec<(&str, Vec<&str>)> = installed + .dirs + .iter() + .map(|dir| { + let names = dir.skills.iter().map(|skill| skill.name.as_str()).collect(); + (dir.skills_dir.as_str(), names) + }) + .collect(); + assert_eq!( + dirs, + vec![(".claude/skills", vec!["amp-query", "amp-setup"])], + "only directories with installed skills should be listed" + ); + + Ok(()) + } +} + +mod update_skills { + use super::*; + + #[tokio::test] + async fn update_skills_without_ref_reinstalls_active_version() -> Result<()> { + //* Given + let env = TestEnv::create()?; + let mut files_v2 = amp_repo("v2.0.0"); + files_v2.retain(|file| !file.path.ends_with("workflow-decoding.md")); + let (github, _server) = start_mock_github(vec![ + MockRef::v1(), + MockRef { + git_ref: "v2.0.0", + commit: COMMIT_V2, + files: files_v2, + pull_request: None, + }, + ]) + .await?; + install_v1_for_claude_code(&env, &github, &["amp-query"]).await?; + env.set_active_version("v2.0.0")?; + + //* When + skills::update_skills( + &github, + &env.config()?, + &env.project(), + SourceRef::ActiveVersion, + Vec::new(), + Vec::new(), + ) + .await?; + + //* Then + let skills_dir = env.project_path(".claude/skills"); + assert_eq!( + fs::read_to_string(skills_dir.join("amp-query/references/app-ampsql.md"))?, + "ampsql doc v2.0.0", + "linked files should be refreshed from the new active version" + ); + assert!( + !skills_dir + .join("amp-query/references/workflow-decoding.md") + .exists(), + "files dropped from the skill in the new version should be removed" + ); + assert_eq!( + SkillsLock::load(&skills_dir)? + .skills + .get(&"amp-query".parse()?), + Some(&installed_from("v2.0.0", COMMIT_V2)), + "the lock should record the new ref and commit" + ); + + Ok(()) + } + + #[tokio::test] + async fn update_skills_with_skill_filter_updates_only_that_skill() -> Result<()> { + //* Given + let env = TestEnv::create()?; + let (github, _server) = start_mock_github(vec![MockRef::v1(), MockRef::v2()]).await?; + install_v1_for_claude_code(&env, &github, &["amp-query", "amp-setup"]).await?; + + //* When + skills::update_skills( + &github, + &env.config()?, + &env.project(), + SourceRef::Git("v2.0.0".to_string()), + skill_names(&["amp-query"])?, + Vec::new(), + ) + .await?; + + //* Then + let lock = SkillsLock::load(&env.project_path(".claude/skills"))?; + assert_eq!( + lock.skills.get(&"amp-query".parse()?), + Some(&installed_from("v2.0.0", COMMIT_V2)), + "the requested skill should be updated" + ); + assert_eq!( + lock.skills.get(&"amp-setup".parse()?), + Some(&installed_from("v1.0.0", COMMIT_V1)), + "other installed skills should stay at their version" + ); + + Ok(()) + } +} + +mod remove_skills { + use super::*; + + #[tokio::test] + async fn remove_skills_with_skill_name_removes_only_that_skill() -> Result<()> { + //* Given + let env = TestEnv::create()?; + let (github, _server) = start_mock_github(vec![MockRef::v1()]).await?; + install_v1_for_claude_code(&env, &github, &["amp-query", "amp-setup"]).await?; + + //* When + skills::remove_skills(&env.project(), named(&["amp-query"])?, &[], true)?; + + //* Then + let skills_dir = env.project_path(".claude/skills"); + assert!( + !skills_dir.join("amp-query").exists(), + "the removed skill's directory should be deleted" + ); + assert!( + skills_dir.join("amp-setup/SKILL.md").exists(), + "other skills should stay installed" + ); + assert_eq!( + installed_names(&skills_dir)?, + vec!["amp-setup"], + "the removed skill should be dropped from the lock" + ); + + Ok(()) + } + + #[tokio::test] + async fn remove_skills_with_all_keeps_user_skills() -> Result<()> { + //* Given + let env = TestEnv::create()?; + let (github, _server) = start_mock_github(vec![MockRef::v1()]).await?; + install_v1_for_claude_code(&env, &github, &["amp-query", "amp-setup"]).await?; + let user_skill_md = env.project_path(".claude/skills/my-skill/SKILL.md"); + fs::create_dir_all(env.project_path(".claude/skills/my-skill"))?; + fs::write(&user_skill_md, "my own skill")?; + + //* When + skills::remove_skills(&env.project(), SkillSelection::All, &[], true)?; + + //* Then + assert_eq!( + files_under(&env.project_path(".claude/skills"))?, + vec!["my-skill/SKILL.md"], + "only ampup skills and the lock file should be removed" + ); + + Ok(()) + } + + #[test] + fn remove_skills_with_tampered_lock_fails_without_deleting() -> Result<()> { + //* Given + let env = TestEnv::create()?; + let project_file = env.project_path("Cargo.toml"); + fs::write(&project_file, "[workspace]")?; + // A lock file committed to the project that names the project root + fs::create_dir_all(env.project_path(".claude/skills"))?; + fs::write( + env.project_path(".claude/skills").join(LOCK_FILE_NAME), + r#"{"skills":{"../..":{"repo":"test/repo","ref":"v1.0.0","commit":"1111"}}}"#, + )?; + + //* When + let result = skills::remove_skills(&env.project(), SkillSelection::All, &[], true); + + //* Then + assert!( + matches!(result, Err(RemoveSkillsError::LoadLock(_))), + "a lock file naming a path outside the skills directory must be rejected, got: {result:?}" + ); + assert!( + project_file.exists(), + "nothing outside the skills directory should be deleted" + ); + + Ok(()) + } +} + +/// A file in the mock amp repository. +struct RepoFile { + path: String, + contents: String, + is_executable: bool, +} + +impl RepoFile { + fn new(path: &str, contents: impl Into) -> Self { + Self { + path: path.to_string(), + contents: contents.into(), + is_executable: false, + } + } + + fn executable(path: &str, contents: impl Into) -> Self { + Self { + path: path.to_string(), + contents: contents.into(), + is_executable: true, + } + } +} + +/// The amp repository at `version`, shaped like the real one: two skills, a +/// README that is not a skill, feature docs and schemas that `amp-query` links +/// from its frontmatter, and a doc nothing links. +/// +/// `version` is written into the file contents so tests can tell which ref a +/// file was installed from. +fn amp_repo(version: &str) -> Vec { + vec![ + RepoFile::new("skills/README.md", "# Amp User Skills\n"), + RepoFile::new( + "skills/amp-setup/SKILL.md", + format!("---\nname: amp-setup\ndescription: Set up Amp.\n---\n# Setup {version}\n"), + ), + RepoFile::new( + "skills/amp-query/SKILL.md", + format!( + "---\n\ + name: amp-query\n\ + description: Query Amp data.\n\ + metadata:\n \ + references: docs/feat/app-ampsql.md docs/schemas/tables/evm-rpc.md\n \ + assets: docs/schemas/providers/evm-rpc.spec.json\n\ + ---\n\ + # Query {version}\n" + ), + ), + RepoFile::new( + "skills/amp-query/references/workflow-decoding.md", + format!("decoding workflow {version}"), + ), + RepoFile::executable("skills/amp-query/scripts/check.sh", "#!/bin/sh\necho ok\n"), + RepoFile::new("docs/feat/app-ampsql.md", format!("ampsql doc {version}")), + RepoFile::new("docs/schemas/tables/evm-rpc.md", "evm-rpc tables"), + RepoFile::new("docs/schemas/providers/evm-rpc.spec.json", "{}"), + RepoFile::new("docs/feat/unrelated.md", "not linked by any skill"), + ] +} + +/// Build the gzipped tarball GitHub serves for a commit: every file sits under +/// a top-level `owner-repo-` directory. +fn source_tarball(commit: &str, files: &[RepoFile]) -> Result> { + let mut builder = Builder::new(GzEncoder::new(Vec::new(), Compression::default())); + + for file in files { + let mode = if file.is_executable { 0o755 } else { 0o644 }; + + let mut header = Header::new_gnu(); + header.set_size(u64::try_from(file.contents.len())?); + header.set_mode(mode); + header.set_cksum(); + + let entry_path = format!("edgeandnode-amp-{}/{}", &commit[..7], file.path); + builder.append_data(&mut header, entry_path, file.contents.as_bytes())?; + } + + Ok(builder.into_inner()?.finish()?) +} + +/// A git ref the mock GitHub API knows: it resolves to `commit`, whose source +/// tarball holds `files`. +struct MockRef { + git_ref: &'static str, + commit: &'static str, + files: Vec, + /// Pull request whose head is this ref, served by the pulls API. + pull_request: Option, +} + +impl MockRef { + /// `v1.0.0` of the mock amp repository. + fn v1() -> Self { + Self { + git_ref: "v1.0.0", + commit: COMMIT_V1, + files: amp_repo("v1.0.0"), + pull_request: None, + } + } + + /// The head branch of pull request [`PR_NUMBER`] in the mock amp repository. + fn pull_request() -> Self { + Self { + git_ref: "feature/new-skills", + commit: COMMIT_PR, + files: amp_repo("pull request"), + pull_request: Some(PR_NUMBER), + } + } + + /// `v2.0.0` of the mock amp repository. + fn v2() -> Self { + Self { + git_ref: "v2.0.0", + commit: COMMIT_V2, + files: amp_repo("v2.0.0"), + pull_request: None, + } + } +} + +/// Start a mock GitHub API serving the given refs, and return a client pointed +/// at it. Unknown refs, pull requests, and commits get a 404. +/// +/// The server stops when the returned handle is dropped at the end of the test. +async fn start_mock_github(refs: Vec) -> Result<(GitHubClient, AbortOnDropHandle<()>)> { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await?; + let addr = listener.local_addr()?; + + let mut routes = Vec::new(); + for mock_ref in refs { + let tarball = source_tarball(mock_ref.commit, &mock_ref.files)?; + routes.push(mock_github::Route::ok( + format!("commits/{}", mock_ref.git_ref), + mock_ref.commit.as_bytes().to_vec(), + )); + routes.push(mock_github::Route::ok( + format!("tarball/{}", mock_ref.commit), + tarball, + )); + if let Some(number) = mock_ref.pull_request { + let pull_request_json = format!(r#"{{"head":{{"sha":"{}"}}}}"#, mock_ref.commit); + routes.push(mock_github::Route::ok( + format!("pulls/{}", number), + pull_request_json.into_bytes(), + )); + } + } + let server = AbortOnDropHandle::new(mock_github::start(listener, routes)); + + let github = GitHubClient::with_api_base(format!("http://{addr}"), None)?; + Ok((github, server)) +} + +/// A mock amp installation plus an empty project directory to install skills +/// into. +struct TestEnv { + amp_dir: TempInstallDir, + project_dir: TempDir, +} + +impl TestEnv { + fn create() -> Result { + Ok(Self { + amp_dir: TempInstallDir::new()?, + project_dir: TempDir::new()?, + }) + } + + /// Mark an ampd version as active, as `ampup install` or `ampup use` would. + fn set_active_version(&self, version: &str) -> Result<()> { + fs::write(self.amp_dir.current_version_file(), version)?; + Ok(()) + } + + fn config(&self) -> Result { + Config::new(Some(self.amp_dir.path().to_path_buf())) + } + + /// Install into the project, as `ampup skills` without `--global` does. + fn project(&self) -> SkillsLocation { + SkillsLocation { + scope: Scope::Project, + base_dir: self.project_dir.path().to_path_buf(), + } + } + + /// A path inside the project directory. + fn project_path(&self, relative_path: &str) -> PathBuf { + self.project_dir.path().join(relative_path) + } +} + +/// Parse skill names, as clap does for `--skill` flags. +fn skill_names(names: &[&str]) -> Result> { + let mut skill_names = Vec::new(); + for name in names { + skill_names.push(name.parse()?); + } + Ok(skill_names) +} + +/// A selection of exactly the named skills, as `--skill` flags produce. +fn named(names: &[&str]) -> Result { + Ok(SkillSelection::Named(skill_names(names)?)) +} + +/// Every file under `dir`, as sorted `/`-separated paths relative to `dir`. +fn files_under(dir: &Path) -> Result> { + let mut files = Vec::new(); + let mut dirs_to_visit = vec![dir.to_path_buf()]; + + while let Some(current_dir) = dirs_to_visit.pop() { + for entry in fs::read_dir(¤t_dir)? { + let path = entry?.path(); + if path.is_dir() { + dirs_to_visit.push(path); + } else { + let relative_path = path.strip_prefix(dir)?; + files.push(relative_path.to_string_lossy().replace('\\', "/")); + } + } + } + + files.sort(); + Ok(files) +} + +/// Names of the skills recorded in the lock file of `skills_dir`, sorted. +fn installed_names(skills_dir: &Path) -> Result> { + let lock = SkillsLock::load(skills_dir)?; + Ok(lock.skills.keys().map(SkillName::to_string).collect()) +} + +fn installed_from(git_ref: &str, commit: &str) -> InstalledSkill { + InstalledSkill { + repo: MOCK_REPO.to_string(), + git_ref: git_ref.to_string(), + commit: commit.to_string(), + } +} + +/// Install skills at `v1.0.0` for Claude Code in the project, as a starting +/// point for list, update, and remove tests. +async fn install_v1_for_claude_code( + env: &TestEnv, + github: &GitHubClient, + names: &[&str], +) -> Result<()> { + skills::add_skills( + github, + &env.config()?, + &env.project(), + SourceRef::Git("v1.0.0".to_string()), + named(names)?, + vec![Agent::ClaudeCode], + ) + .await?; + Ok(()) +} diff --git a/ampup/src/tests/mod.rs b/ampup/src/tests/mod.rs index c96016d..b21b505 100644 --- a/ampup/src/tests/mod.rs +++ b/ampup/src/tests/mod.rs @@ -1,4 +1,5 @@ mod fixtures; mod it_adbc; mod it_ampup; +mod it_skills; pub(crate) mod mock_github; diff --git a/ampup/src/ui.rs b/ampup/src/ui.rs index 7b88982..57d683f 100644 --- a/ampup/src/ui.rs +++ b/ampup/src/ui.rs @@ -28,8 +28,28 @@ macro_rules! detail { }; } +/// Print an info message with a cyan arrow to stderr +/// +/// For commands whose stdout carries only their result, such as `--json` output. +macro_rules! info_stderr { + ($($arg:tt)*) => { + eprintln!("{} {}", console::style("→").cyan(), format!($($arg)*)) + }; +} + +/// Print a dimmed detail message (indented) to stderr +/// +/// For commands whose stdout carries only their result, such as `--json` output. +macro_rules! detail_stderr { + ($($arg:tt)*) => { + eprintln!(" {}", console::style(format!($($arg)*)).dim()) + }; +} + pub(crate) use detail; +pub(crate) use detail_stderr; pub(crate) use info; +pub(crate) use info_stderr; pub(crate) use success; pub(crate) use warning as warn; diff --git a/docs/code/apps-cli.md b/docs/code/apps-cli.md index 89e1d1d..2973796 100644 --- a/docs/code/apps-cli.md +++ b/docs/code/apps-cli.md @@ -59,6 +59,8 @@ These patterns ensure: **REQUIRED**: Results and informational messages go to **stdout** via `success!`, `info!`, and `detail!` macros. Warnings go to **stderr** via `warn!`. Errors are handled by the top-level error handler which outputs to **stderr**. +**Exception — `ampup skills`**: the skills commands follow the amp CLI rule instead. Their result is the only thing on stdout, printed as text or as JSON with `--json` by one shared print path (`print_result` in `ampup/src/commands/skills.rs`). Status goes to stderr through `info_stderr!` and `detail_stderr!`, and warnings through `warn!`. + ### 3. Styled Terminal Output **REQUIRED**: All styled output uses the `console` crate for colors, bold text, and dimming. The UI macros and styling helpers provide consistent formatting. @@ -83,6 +85,8 @@ All macros are defined in `ampup/src/ui.rs`: | `info!` | stdout | `→` cyan | Normal | | `warn!` | stderr | `⚠` yellow bold | Normal | | `detail!` | stdout | 2-space indent | Dimmed | +| `info_stderr!` | stderr | `→` cyan | Normal | +| `detail_stderr!` | stderr | 2-space indent | Dimmed | **Usage examples**: @@ -122,14 +126,19 @@ ui::info!("Installation directory: {}", ui::path("/home/user/.amp")); **Location**: `ampup/src/main.rs` -The top-level error handler catches all errors and formats them with a red bold `✗` prefix to stderr: +The top-level error handler catches all errors and formats them with a red bold `✗` prefix to stderr, followed by one `Caused by:` line per error in its source chain: ```rust #[tokio::main] async fn main() { if let Err(e) = run().await { - // Print the error with custom formatting + // Print the error with some custom formatting eprintln!("{} {}", style("✗").red().bold(), e); + // Typed errors describe one level each and carry their cause as a + // source, so print the whole chain + for cause in e.chain().skip(1) { + eprintln!(" {} {}", style("Caused by:").dim(), cause); + } std::process::exit(1); } } @@ -145,6 +154,8 @@ async fn main() { - Implement `std::error::Error` trait - Convert to `anyhow::Error` via `.into()` +**Exception — `ampup skills`**: the skills code (`ampup/src/skills.rs`, `ampup/src/skills/`, `ampup/src/commands/skills.rs`) declares its errors with `#[derive(Debug, thiserror::Error)]`, one error type per fallible function, declared right after it, with `#[source]` chains and messages that describe a single level. The top-level handler prints the chain. + ### Error Propagation **REQUIRED**: Use `anyhow::Result` for all fallible functions and propagate errors with `.context()` for additional context: diff --git a/docs/features/app-ampup-skills.md b/docs/features/app-ampup-skills.md new file mode 100644 index 0000000..7e56787 --- /dev/null +++ b/docs/features/app-ampup-skills.md @@ -0,0 +1,170 @@ +--- +name: "app-ampup-skills" +description: "ampup skills add, list, update, and remove for installing amp agent skills into Claude Code, Codex, and Cursor skill directories, with linked references and assets. Load when asking about agent skills, installing skills, skill versions, or the .amp-skills.json lock file" +type: "feature" +status: "experimental" +components: "app:ampup" +--- + +# ampup skills - Agent Skills Installer + +## Summary + +`ampup skills` installs the agent skills that live in the amp repository's `skills/` directory into the skill directories of coding agents, in the current project or in the home directory. Skills are not published as release assets, so ampup reads them from the repository source at a git ref, which defaults to the active ampd version so skills match the installed binaries. Each skill's `SKILL.md` frontmatter can link feature docs and schemas from elsewhere in the repository; ampup copies those into the skill's `references/` and `assets/` directories so the installed skill is self-contained, following the [Agent Skills specification](https://agentskills.io/specification). + +## Table of Contents + +1. [Key Concepts](#key-concepts) +2. [Architecture](#architecture) +3. [Configuration](#configuration) +4. [Usage](#usage) +5. [Implementation](#implementation) +6. [Limitations](#limitations) +7. [References](#references) + +## Key Concepts + +- **Skill**: A `skills//` directory in the amp repository that contains a `SKILL.md` +- **Linked files**: Repository paths listed in the `SKILL.md` frontmatter `metadata`, as space-separated strings. Paths under `references` are installed as `references/`, paths under `assets` as `assets/` +- **Git ref**: The tag, branch, or commit skills are installed from (`--ref`), or a pull request (`--pr `) at the current head of its branch, which also works for pull requests from forks. Defaults to the active ampd version (the `.version` file). A pull request is recorded as `pr/` +- **Scope**: `project` installs relative to the current directory; `global` (`--global`) installs relative to the home directory +- **Lock file**: `.amp-skills.json` in each skills directory, recording the repository, ref, and commit of every skill ampup installed there. Skills missing from it are never updated or removed by ampup +- **Skill name**: A skill directory name, always a single path component. Names from the tarball, the lock file, and `--skill` that are empty, `.`, `..`, or contain a path separator are rejected, so no operation can reach outside the skills directory + +## Architecture + +### Agent Skill Directories + +| Agent | `--agent` | Project | Global | +|-------|-----------|---------|--------| +| Claude Code | `claude-code` | `.claude/skills/` | `~/.claude/skills/` | +| Codex | `codex` | `.agents/skills/` | `~/.codex/skills/` | +| Cursor | `cursor` | `.agents/skills/` | `~/.cursor/skills/` | + +Agents that share a directory (Codex and Cursor in a project) are installed into once. + +### Install Flow + +1. User runs `ampup skills add [--ref ]` +2. Resolve the ref: `--ref`, `--pr`, or the active ampd version +3. Resolve GitHub token (explicit `--github-token` → `gh auth token` → unauthenticated) +4. Resolve the ref to a commit (a pull request resolves to its head commit), then stream the repository source tarball of that commit to an anonymous temporary file +5. Read every `skills//` directory with a `SKILL.md`, parse its frontmatter, and copy linked files into `references/` and `assets/`. A linked path that does not exist fails the install +6. Select skills (`--skill`, `--all`, or an interactive prompt) and agents (`--agent` or an interactive prompt) +7. For each skills directory, record the skill's repository, ref, and commit in `.amp-skills.json` first, so an interrupted install is retried rather than skipped on the next run. Then write each skill to a hidden staging directory, then move it into `/`, replacing a previous copy installed by ampup. An existing skill directory that ampup did not install is skipped with a warning + +### Communication + +``` +ampup → GitHub API (commits) # Resolve the git ref to a commit +ampup → GitHub API (pulls) # Resolve a pull request to its head commit +ampup → GitHub API (tarball) # Download the repository source +``` + +## Configuration + +| Variable | Default | Description | +|----------|---------|-------------| +| `AMP_DIR` | `$XDG_CONFIG_HOME/.amp` or `$HOME/.amp` | Base installation directory, read for the active ampd version | +| `GITHUB_TOKEN` | (none) | GitHub token for private repository access and API rate limits | + +## Usage + +### Install Skills + +```bash +# Pick skills and agents interactively, from the active ampd version +ampup skills add + +# List the available skills without installing +ampup skills add --list + +# Install specific skills for Claude Code +ampup skills add --skill amp-setup --skill amp-query --agent claude-code + +# Install every skill for all agents from a specific ref +ampup skills add --all -a claude-code -a codex -a cursor --ref v0.1.0 + +# Try the skills of a pull request before it merges +ampup skills add --all --agent claude-code --pr 3087 + +# Install in the home directory +ampup skills add --all --agent claude-code --global + +# Print what was installed as JSON +ampup skills add --all --agent claude-code --json +``` + +### List Installed Skills + +```bash +# Skills installed by ampup in the current project +ampup skills list + +# Skills installed by ampup in the home directory, for one agent +ampup skills list --global --agent codex + +# As JSON +ampup skills list --json +``` + +Shows each skills directory with its agents, and every installed skill with its ref and commit. + +### Update Skills + +```bash +# Reinstall every installed skill from the active ampd version +ampup skills update + +# Update one skill to a branch +ampup skills update --skill amp-query --ref main + +# Update every installed skill to the latest head of a pull request +ampup skills update --pr 3087 +``` + +Skills that no longer exist at the ref are skipped with a warning. + +### Remove Skills + +```bash +# Pick installed skills to remove interactively +ampup skills remove + +# Remove specific skills +ampup skills remove --skill amp-query + +# Remove every skill installed by ampup, for Cursor only, without confirming +ampup skills remove --all --agent cursor --force +``` + +`remove` asks for confirmation unless `--force` is given; declining removes nothing and exits successfully. `list`, `update`, and `remove` act on all agents unless `--agent` is given. + +### Output + +Every `skills` command prints its result, and only its result, to stdout: text by default, or JSON with `--json` (`add --list` prints the available skills, `list` the installed skills grouped by directory, and `add`, `update`, and `remove` the changes they made). Progress, status, and warnings go to stderr, so `--json` output can be piped. Errors print the failing step followed by its causes. + +## Implementation + +### Source Files + +- `ampup/src/commands/skills.rs` - `add`, `list`, `update`, and `remove` commands, including ref defaulting, interactive selection and confirmation, download deadlines, and text and JSON results +- `ampup/src/skills.rs` - Loading skills from a source tarball, copying linked files, installing and removing skill directories +- `ampup/src/skills/agent.rs` - Supported agents and their project and global skill directories +- `ampup/src/skills/archive.rs` - Reading selected files from a GitHub source tarball +- `ampup/src/skills/frontmatter.rs` - `SKILL.md` frontmatter parsing +- `ampup/src/skills/lock.rs` - `.amp-skills.json` lock file +- `ampup/src/skills/skill_name.rs` - Validated skill names +- `ampup/src/github.rs` - Git ref resolution and source tarball download +- `ampup/src/tests/it_skills.rs` - End-to-end tests of listing, adding, updating, and removing skills against a mock GitHub API + +## Limitations + +- The default ref is the active ampd version, so it fails for refs that do not contain `skills/` (releases published before skills were added) and for custom build names; use `--ref` +- Replacing a skill removes the previous copy before moving the new one in, so a failed move leaves the skill uninstalled until the command is re-run +- `update` without `--ref` or `--pr` uses the active ampd version, not the ref each skill was installed from, so skills installed from a branch or pull request move to the release unless the ref is passed again +- The whole repository tarball is downloaded to read the skills, streamed to a temporary file rather than held in memory; resolving the ref times out after 30 seconds and the download after 10 minutes + +## References + +- [app-ampup](app-ampup.md) - Base: ampup version manager and installer diff --git a/docs/features/app-ampup.md b/docs/features/app-ampup.md index 0f1038e..0dcf0dc 100644 --- a/docs/features/app-ampup.md +++ b/docs/features/app-ampup.md @@ -127,6 +127,15 @@ ampup update -j 8 ampup install ``` +### Agent Skills + +```bash +# Install amp agent skills into the current project +ampup skills add +``` + +See [app-ampup-skills](app-ampup-skills.md) for installing, listing, updating, and removing skills. + ### Self-Update ```bash @@ -236,6 +245,7 @@ Each command defaults to the active amp version and accepts `--version` to targe ``` ampup → GitHub Releases API # Download pre-built binaries ampup → GitHub API (tags, PRs) # Fetch source for builds +ampup → GitHub API (tarball) # Fetch agent skills from source ampup → ampup.sh/install # Installation script download ```