diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 8499377f..79330667 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -49,6 +49,10 @@ jobs: - run: pnpm test:js - name: Rust public API contract run: cargo test -p skilld-core --test api_contract + - name: Doctor discovery, migration, and removal + run: | + cargo test -p skilld-command --test doctor --test doctor_metadata --test doctor_actions + cargo test -p skilld-native --test doctor_ui - name: Rust tests if: github.event_name == 'push' run: cargo test --workspace diff --git a/.pnpm-store/v11/index.db b/.pnpm-store/v11/index.db new file mode 100644 index 00000000..7f9770bd Binary files /dev/null and b/.pnpm-store/v11/index.db differ diff --git a/Cargo.lock b/Cargo.lock index 1e7065c4..eface24d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -290,6 +290,16 @@ dependencies = [ "piper", ] +[[package]] +name = "bstr" +version = "1.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6bb31b46c14244e20ee9984b11bf5c992b91fb6939fea616e3512c8baecdbe5f" +dependencies = [ + "memchr", + "serde_core", +] + [[package]] name = "bumpalo" version = "3.20.3" @@ -892,6 +902,19 @@ dependencies = [ "r-efi", ] +[[package]] +name = "globset" +version = "0.4.20" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "07c34a9410465b45bd9787443bc7370f37735bad04b0f0cd57ff1a3186c98988" +dependencies = [ + "aho-corasick", + "bstr", + "log", + "regex-automata", + "regex-syntax", +] + [[package]] name = "hashbrown" version = "0.16.1" @@ -1978,6 +2001,7 @@ dependencies = [ "clap", "ed25519-dalek", "fs4", + "globset", "miniz_oxide", "pulldown-cmark", "serde", diff --git a/GLOSSARY.md b/GLOSSARY.md index 9213a614..3845f921 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -66,6 +66,7 @@ Every public export, command, error, route, and document uses these terms. | `skilld verify` | source verification | | `skilld outdated` | outdated Skill report | | `skilld outdated --all` | system-wide outdated Skill report | +| `skilld doctor` | Skill discovery and cleanup | | `skilld install skilld --global` | global skilld Skill install | | `skilld auth login` | account login | | `SKILLD_NO_WEEKLY` | weekly notice opt-out | diff --git a/README.md b/README.md index 5b9d02f3..cfd3357c 100644 --- a/README.md +++ b/README.md @@ -356,6 +356,11 @@ skilld update --check --json skilld update skilld verify +# Sweep Skill files and review cleanup actions +skilld doctor +skilld doctor ./apps --json +skilld doctor --exclude '**/archive/**' + # Remove a Skill skilld remove ``` @@ -364,6 +369,55 @@ Project installs update `.skills/skilld-lock.yaml` and the selected Agent target Use `--global` (or `-g`) for account-level Agent targets. Use `--mode copy` or `--mode symlink` to control target writes. +### Review Skill files + +`skilld doctor` scans your home directory and opens a terminal UI. +Pass roots to narrow the scan. Use `--json` or `--plain` for a read-only report. +The default scan skips links and ownership metadata outside your home directory. +Pass explicit roots when you need those locations. +Progress shows the current phase, directory counts, and elapsed time. +The scan includes hidden `.claude/skills` directories and lists `CLAUDE.md` and `.claude/commands` files separately. + +Default exclude globs prune dependencies, builds, caches, plugin staging, sessions, backups, and fixtures before reading their children. +The scan skips general `~/.local` discovery, including worker homes and cached bundles. +Configured Agent Skill directories and the configured skilld store remain included within the requested scan roots. +Skill symlinks can still resolve into `~/.local`. Their neighboring directories are not scanned. +Configured roots still honor exclude globs. Use `--include-excluded` for a broad `.local` scan. +Git worktrees are skipped, including explicit roots and links into worktrees. +Main checkouts and submodules remain included. +Use `--include-excluded` or `--include-worktrees` when you need those files. +Repeat `--exclude GLOB` to add exclusions. Git internals remain excluded. +Source and plugin files retain their owners. + +The first screen recommends reviewing skills.sh migration, unknown installs, ownership problems, and duplicate copies. +Open a recommendation with Enter, choose a project or folder, then choose a Skill. Escape goes back. +Managed installs, plugin Skills, and source directories have separate groups. +Press Tab to cycle through all paths, symlinks, and directories. +Details show symlink destinations and the paths of identical copies. Symlinks are not duplicate copies. +The Symlinks view includes Skills reached through linked parent folders and identifies the folder link. +Counts distinguish physical Skills from linked paths. One Skill can appear in both path views. +Filters only change the view. Cleanup reviews all targets for the selected install. +Choose a Skill, then press `m` to migrate or `d` to remove. +Review affected Agent targets before pressing Enter. Escape cancels. +Press `/` to filter by name, owner, or path. Left and right switch focus between the list and details. +Press `p` for scan problems, `n` for the full notice, or `?` for help. +After cleanup, quitting prints the completed action and backup path to your shell. +Migration keeps the scope and copy or link mode of each observed target. +It verifies source contents, preserves recorded branches, and requests approval for Skill behaviors. +When the recorded Git tree matches the installed files, migration can also replace them with the displayed source commit. +If provenance cannot establish the replacement, migration stops before changing files. + +Removal backs up the selected targets and removes only their skills.sh lockfile entry. +Original targets, lock metadata, and `recovery.json` remain under `.skilld-doctor-backups` beside the skilld store. +Unknown Agent targets can also be removed after review. +Declared skilld installs, plugin files, and source directories require their existing owner's workflow. + +`--check-sources` checks up to 20 source candidates through skilld.dev. +An exact directory match includes supporting files and executable modes. +A name match alone never establishes provenance. +The report retains scan problems. Exit code 1 means some files could not be checked. +Historical commit searches and project-to-global deduplication are outside this command's current actions. + ### Read the registry These commands read skilld.dev and need no account: @@ -470,6 +524,7 @@ The skilld-maintained Skills: - [`update-package-skill`](./skills/update-package-skill): update that Skill after a release, testing only what changed - [`generate-project-skill`](./skills/generate-project-skill): draft a Skill from a project you maintain - [`review-skill`](./skills/review-skill): review a Skill before you publish it +- [`skilld-tui-design`](./skills/skilld-tui-design): design and verify terminal output and interactive CLI flows When you run these Skills directly, you see every instruction and review every change. diff --git a/crates/skilld-command/Cargo.toml b/crates/skilld-command/Cargo.toml index 697a1d7a..a1459002 100644 --- a/crates/skilld-command/Cargo.toml +++ b/crates/skilld-command/Cargo.toml @@ -16,6 +16,7 @@ base64.workspace = true clap.workspace = true +globset = "0.4" miniz_oxide = "0.9.1" pulldown-cmark = { version = "0.13.4", default-features = false } diff --git a/crates/skilld-command/src/discover.rs b/crates/skilld-command/src/discover.rs index ffaf0875..667fbc6d 100644 --- a/crates/skilld-command/src/discover.rs +++ b/crates/skilld-command/src/discover.rs @@ -411,8 +411,11 @@ fn skill_detail_lines( lines.push(Line::linked_field("Skill page", page.clone(), page)); } if let Some(selector) = skill_selector(&summary.owner, &summary.repository, &summary.name) { - lines.push(Line::field("Run", run_command(&selector, platform))); - lines.push(Line::field("Install", install_command(&selector, platform))); + lines.push(Line::command_field("Run", run_command(&selector, platform))); + lines.push(Line::command_field( + "Install", + install_command(&selector, platform), + )); } Ok(lines) } diff --git a/crates/skilld-command/src/doctor.rs b/crates/skilld-command/src/doctor.rs new file mode 100644 index 00000000..c257c552 --- /dev/null +++ b/crates/skilld-command/src/doctor.rs @@ -0,0 +1,1012 @@ +//! Read-only discovery. Paths remain observations until an action rechecks them. +use std::collections::{BTreeMap, BTreeSet}; +use std::fs; +use std::path::{Path, PathBuf}; + +use clap::Args; +use globset::{Glob, GlobSet, GlobSetBuilder}; +use serde::Serialize; +use skilld_core::{AGENT_TARGETS, AgentTargetId, SkillName}; + +use crate::doctor_metadata::{Fingerprint, ForeignRecord, fingerprint, read_foreign_lock}; +use crate::{CommandError, ResolvedTarget}; + +/// Pruned before reading children. Hidden Agent target directories stay visible. +pub const DEFAULT_EXCLUDES: &[&str] = &[ + "**/node_modules/**", + "**/.git/**", + "**/.hg/**", + "**/.svn/**", + "**/target/**", + "**/dist/**", + "**/.nuxt/**", + "**/.output/**", + "**/.next/**", + "**/coverage/**", + "**/__pycache__/**", + "**/.venv/**", + "**/.cache/**", + "**/.npm/**", + "**/.bun/**", + "**/.pnpm-store/**", + "**/.local/share/pnpm/**", + "**/.cargo/registry/**", + "**/.rustup/**", + "**/.codex/.tmp/**", + "**/.codex/sessions/**", + "**/.codex/archived_sessions/**", + "**/.claude/projects/**", + "**/.claude/debug/**", + "**/.claude/session-env/**", + "**/.claude/plugins/cache/**", + "**/.codex/plugins/cache/**", + "**/.claude/plugins/marketplaces/**", + "**/.codex/plugins/marketplaces/**", + "**/.skilld/repos/**", + "**/.skilld/references/**", + "**/.skilld/llm-cache/**", + "**/.local/share/harlan-github-agent/**", + "**/.local/share/Trash/**", + "**/scratch/**", + "**/backups/**", + "**/Backups/**", + "**/*-backup*/**", + "**/.skilld-doctor-backups/**", + "**/.data/content/**", + "**/data/trees/**", + "**/.steam/**", + "**/.local/share/Steam/**", + "**/.mozilla/**", + "**/.config/google-chrome/**", + "**/.ssh/**", + "**/.gnupg/**", + "**/.dev-browser/**", + "**/.artifacts/**", + "**/test/fixtures/**", + "**/tests/fixtures/**", + "**/evals/**", + "**/evals-opencode/**", + "**/runtime/mysql/**", + "**/.config/chromium/**", + "**/.local/share/containers/**", + "**/.local/share/uv/**", + "**/go/pkg/mod/**", + "**/.gradle/caches/**", + "**/.docker/**", + "**/.yarn/**", + "**/.intellijPlatform/**", + "**/.unlighthouse/**", + "**/kv-dump/**", + "**/*.trickplay/**", + "**/google-cloud-sdk/**", + "**/.gemini/tmp/**", + "**/.gemini/history/**", + "**/.gemini/antigravity-browser-profile/**", + "**/.config/*/User/History/**", + "**/.config/google-chrome-*/**", + "**/.config/cef_user_data/**", + "**/.config/JetBrains/**/chrome-user-data/**", + "**/.config/JetBrains/**/jdbc-drivers/**", + "**/.config/JetBrains/**/javascript/nodejs/**", +]; + +const WORKTREE_EXCLUDES: &[&str] = &["**/.claude/worktrees/**", "**/.worktrees/**"]; + +#[derive(Clone, Debug, Args)] +pub struct DoctorOptions { + /// Scan these roots. The default is your home directory. + #[arg(value_name = "ROOT")] + pub roots: Vec, + /// Prune paths matching this glob. Repeat for several patterns. + #[arg(long, value_name = "GLOB")] + pub exclude: Vec, + /// Include dependency, cache, staging, and backup directories. + #[arg(long)] + pub include_excluded: bool, + /// Include Git worktrees and their staging directories, including explicit roots. + #[arg(long)] + pub include_worktrees: bool, + /// Stop descending after this many directories. + #[arg(long, default_value_t = 20, value_parser = clap::value_parser!(u16).range(1..=64))] + pub max_depth: u16, + /// Check source candidates through skilld.dev. Limited to 20 Skills per scan. + #[arg(long)] + pub check_sources: bool, +} + +impl DoctorOptions { + pub fn for_root(root: &Path) -> Self { + Self { + roots: vec![root.to_path_buf()], + exclude: vec![], + include_excluded: false, + include_worktrees: false, + max_depth: 20, + check_sources: false, + } + } +} + +#[derive(Clone, Debug)] +pub struct ScanContext { + pub home: PathBuf, + pub global_store: PathBuf, + pub global_targets: Vec, + pub skills_sh_global_lock: PathBuf, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum ScanPhase { + Discover, + Fingerprint { completed: usize, total: usize }, + Sources { completed: usize, total: usize }, +} + +#[derive(Clone, Debug)] +pub struct ScanProgress { + pub phase: ScanPhase, + pub current_path: PathBuf, + pub visited_directories: usize, + pub skipped_directories: usize, + pub found_skills: usize, + pub problems: usize, +} + +impl ScanProgress { + pub fn from_report( + report: &DoctorReport, + phase: ScanPhase, + current_path: PathBuf, + found_skills: usize, + ) -> Self { + Self { + phase, + current_path, + visited_directories: report.visited_directories, + skipped_directories: report.skipped_directories, + found_skills, + problems: report.problems.len(), + } + } +} + +#[derive(Clone, Debug, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct SkillLocation { + pub path: PathBuf, + /// The direct link destination, as written. A linked parent does not make this path a link. + pub symlink_target: Option, + /// The nearest linked parent, stopping at home when it is an ancestor. + pub linked_parent: Option, + pub agent: Option, + /// None means global scope. Source directories have no Agent target. + pub project_root: Option, +} + +#[derive(Clone, Debug, Serialize)] +pub struct LinkedParent { + pub path: PathBuf, + pub target: PathBuf, +} + +impl SkillLocation { + pub fn is_linked(&self) -> bool { + self.symlink_target.is_some() || self.linked_parent.is_some() + } +} + +#[derive(Clone, Debug, Serialize)] +#[serde( + tag = "_tag", + rename_all = "camelCase", + rename_all_fields = "camelCase" +)] +pub enum DoctorOwner { + SkillsSh { record: ForeignRecord }, + Skilld { store: PathBuf }, + Plugin, + Source, + Unknown, + Unavailable { message: String }, +} + +impl DoctorOwner { + pub fn label(&self) -> &'static str { + match self { + Self::SkillsSh { .. } => "skills.sh", + Self::Skilld { .. } => "skilld", + Self::Plugin => "Plugin", + Self::Source => "Source", + Self::Unknown => "Unknown", + Self::Unavailable { .. } => "Unavailable", + } + } +} + +#[derive(Clone, Debug, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct SourceMatch { + pub source: String, + pub commit: Option, + pub result: String, +} + +#[derive(Clone, Debug, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct DoctorSkill { + pub name: String, + pub canonical_path: PathBuf, + pub paths: Vec, + pub owner: DoctorOwner, + pub fingerprint: Option, + pub duplicates: Vec, + pub source_match: Option, +} + +#[derive(Clone, Debug, Serialize)] +pub struct ScanProblem { + pub path: PathBuf, + pub message: String, +} + +#[derive(Clone, Debug, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct DoctorReport { + pub roots: Vec, + pub excludes: Vec, + pub skills: Vec, + pub claude_files: Vec, + pub problems: Vec, + pub visited_directories: usize, + pub skipped_directories: usize, +} + +fn io_error(path: &Path, error: impl std::fmt::Display) -> CommandError { + CommandError::filesystem(format!("Cannot read {}: {error}", path.display())) +} + +fn excludes(options: &DoctorOptions) -> Result<(Vec, GlobSet), CommandError> { + let mut patterns = if options.include_excluded { + vec!["**/.git/**".into()] + } else { + DEFAULT_EXCLUDES.iter().map(|s| (*s).to_owned()).collect() + }; + if !options.include_worktrees { + patterns.extend( + WORKTREE_EXCLUDES + .iter() + .map(|pattern| (*pattern).to_owned()), + ); + } + patterns.extend(options.exclude.clone()); + let mut builder = GlobSetBuilder::new(); + for pattern in &patterns { + builder.add( + Glob::new(pattern).map_err(|e| { + CommandError::config(format!("Invalid exclude glob {pattern}: {e}")) + })?, + ); + // A trailing /** must prune the directory itself, before readdir. + if let Some(parent) = pattern.strip_suffix("/**") { + builder.add(Glob::new(parent).map_err(|e| CommandError::config(e.to_string()))?); + } + } + Ok(( + patterns, + builder + .build() + .map_err(|e| CommandError::config(e.to_string()))?, + )) +} + +fn location(path: &Path, context: &ScanContext) -> std::io::Result { + let symlink_target = if fs::symlink_metadata(path)?.file_type().is_symlink() { + Some(fs::read_link(path)?) + } else { + None + }; + let parent = path.parent().unwrap_or(path); + let mut linked_parent = None; + for ancestor in parent.ancestors() { + if fs::symlink_metadata(ancestor)?.file_type().is_symlink() { + linked_parent = Some(LinkedParent { + path: ancestor.to_owned(), + target: fs::read_link(ancestor)?, + }); + break; + } + if ancestor == context.home { + break; + } + } + if let Some(target) = context.global_targets.iter().find(|t| t.root == parent) { + return Ok(SkillLocation { + path: path.to_owned(), + symlink_target, + linked_parent, + agent: Some(target.agent), + project_root: None, + }); + } + if parent == context.home.join(".codex/skills") { + return Ok(SkillLocation { + path: path.to_owned(), + symlink_target, + linked_parent, + agent: Some(AgentTargetId::Codex), + project_root: None, + }); + } + for target in AGENT_TARGETS { + // Unprefixed source directories are not evidence of an Agent installation. + if !target.project_skills_dir.starts_with('.') { + continue; + } + if parent.ends_with(target.project_skills_dir) { + let mut root = parent; + for _ in Path::new(target.project_skills_dir).components() { + root = root.parent().unwrap_or(root); + } + return Ok(SkillLocation { + path: path.to_owned(), + symlink_target, + linked_parent, + agent: Some(target.id), + project_root: Some(root.to_owned()), + }); + } + } + Ok(SkillLocation { + path: path.to_owned(), + symlink_target, + linked_parent, + agent: None, + project_root: None, + }) +} + +/// A Git file alone can also name a submodule or a separate Git directory. +/// Linked worktrees have a shared repository recorded in their admin directory. +fn linked_worktree(path: &Path) -> std::io::Result { + let marker = path.join(".git"); + match fs::metadata(&marker) { + Ok(metadata) if metadata.is_file() => {} + Ok(_) => return Ok(false), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(false), + Err(error) => return Err(error), + } + let marker = fs::read_to_string(marker)?; + let Some(directory) = marker.trim().strip_prefix("gitdir: ") else { + return Ok(false); + }; + let admin = path.join(directory); + match fs::metadata(admin.join("commondir")) { + Ok(metadata) => Ok(metadata.is_file()), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(false), + Err(error) => Err(error), + } +} + +fn worktree_ancestor(path: &Path) -> std::io::Result { + for parent in path.ancestors() { + if parent.file_name().is_some_and(|name| name == ".worktrees") + || (parent.file_name().is_some_and(|name| name == "worktrees") + && parent + .parent() + .and_then(Path::file_name) + .is_some_and(|name| name == ".claude")) + { + return Ok(true); + } + if linked_worktree(parent)? { + return Ok(true); + } + } + Ok(false) +} + +fn prune_worktree(result: std::io::Result, path: &Path, report: &mut DoctorReport) -> bool { + match result { + Ok(false) => false, + Ok(true) => { + report.skipped_directories += 1; + true + } + Err(error) => { + report.skipped_directories += 1; + report.problems.push(ScanProblem { + path: path.to_owned(), + message: format!("Cannot check Git worktree metadata: {error}"), + }); + true + } + } +} + +fn target_root(path: &Path, context: &ScanContext) -> bool { + context.global_targets.iter().any(|t| t.root == path) + || path == context.home.join(".codex/skills") + || AGENT_TARGETS + .iter() + .any(|t| t.project_skills_dir.starts_with('.') && path.ends_with(t.project_skills_dir)) +} + +pub fn scan(options: &DoctorOptions, context: &ScanContext) -> Result { + scan_with_progress(options, context, &mut |_| {}) +} + +pub fn scan_with_progress( + options: &DoctorOptions, + context: &ScanContext, + progress: &mut impl FnMut(ScanProgress), +) -> Result { + let (patterns, excluded) = excludes(options)?; + let home_boundary = if options.roots.is_empty() { + Some(fs::canonicalize(&context.home).map_err(|e| io_error(&context.home, e))?) + } else { + None + }; + let roots = if options.roots.is_empty() { + vec![context.home.clone()] + } else { + options.roots.clone() + }; + let roots = roots + .into_iter() + .map(|p| { + fs::metadata(&p).map_err(|e| io_error(&p, e))?; + std::path::absolute(&p).map_err(|e| io_error(&p, e)) + }) + .collect::, _>>()?; + let local_root = context.home.join(".local"); + let known_local_roots: BTreeSet = context + .global_targets + .iter() + .map(|target| target.root.clone()) + .chain(std::iter::once(context.global_store.clone())) + .filter(|path| path.starts_with(&local_root)) + .collect(); + // Only the configured roots bypass broad app-data pruning. All glob exclusions still apply. + let skip_local = |path: &Path| { + !options.include_excluded + && path.starts_with(&local_root) + && !known_local_roots.iter().any(|root| path.starts_with(root)) + }; + let mut report = DoctorReport { + roots: roots.clone(), + excludes: patterns, + skills: vec![], + claude_files: vec![], + problems: vec![], + visited_directories: 0, + skipped_directories: 0, + }; + let mut pending = roots.iter().map(|p| (p.clone(), 0)).collect::>(); + if !options.include_excluded { + report.excludes.push(format!( + "{}/**", + local_root.to_string_lossy().replace('\\', "/") + )); + for path in &known_local_roots { + // Do not expand an explicitly narrowed scan into another scope. + if !roots.iter().any(|root| path.starts_with(root)) { + continue; + } + if path.ancestors().any(|ancestor| excluded.is_match(ancestor)) { + report.skipped_directories += 1; + continue; + } + match fs::symlink_metadata(path) { + Ok(_) => pending.push((path.clone(), 0)), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => report.problems.push(ScanProblem { + path: path.clone(), + message: format!("Cannot inspect configured Skill root: {error}"), + }), + } + } + } + progress(ScanProgress::from_report( + &report, + ScanPhase::Discover, + roots[0].clone(), + 0, + )); + let mut visited = BTreeSet::new(); + let mut found = BTreeSet::new(); + while let Some((path, depth)) = pending.pop() { + if excluded.is_match(&path) || skip_local(&path) { + report.skipped_directories += 1; + continue; + } + if depth == 0 + && let Some(home) = &home_boundary + { + match fs::canonicalize(&path) { + Ok(real) if real.starts_with(home) => {} + Ok(_) => { + report.skipped_directories += 1; + report.problems.push(ScanProblem { path, message: "Link points outside the home directory. Pass its root explicitly to scan it.".into() }); + continue; + } + Err(error) => { + report.problems.push(ScanProblem { + path, + message: format!("Cannot resolve scan root: {error}"), + }); + continue; + } + } + } + if !options.include_worktrees { + let worktree = if depth == 0 { + fs::canonicalize(&path).and_then(|canonical| worktree_ancestor(&canonical)) + } else { + linked_worktree(&path) + }; + if prune_worktree(worktree, &path, &mut report) { + continue; + } + } + if !visited.insert(path.clone()) { + continue; + } + if report.visited_directories >= 250_000 || found.len() >= 5_000 { + report.problems.push(ScanProblem { + path, + message: "Scan limit reached. Choose narrower roots.".into(), + }); + break; + } + report.visited_directories += 1; + if report.visited_directories.is_multiple_of(128) { + progress(ScanProgress::from_report( + &report, + ScanPhase::Discover, + path.clone(), + found.len(), + )); + } + let entries = match fs::read_dir(&path) { + Ok(entries) => entries, + Err(e) => { + report.problems.push(ScanProblem { + path, + message: e.to_string(), + }); + continue; + } + }; + let mut is_skill = false; + let mut children = vec![]; + for entry in entries { + let entry = match entry { + Ok(e) => e, + Err(e) => { + report.problems.push(ScanProblem { + path: path.clone(), + message: e.to_string(), + }); + continue; + } + }; + let child = entry.path(); + if excluded.is_match(&child) || skip_local(&child) { + report.skipped_directories += 1; + continue; + } + let kind = match entry.file_type() { + Ok(t) => t, + Err(e) => { + report.problems.push(ScanProblem { + path: child, + message: e.to_string(), + }); + continue; + } + }; + if kind.is_symlink() + && let Some(home) = &home_boundary + && let Ok(real) = fs::canonicalize(&child) + && !real.starts_with(home) + { + report.skipped_directories += 1; + if target_root(&path, context) || target_root(&child, context) { + report.problems.push(ScanProblem { path: child, message: "Link points outside the home directory. Pass its root explicitly to scan it.".into() }); + } + continue; + } + if entry.file_name() == "SKILL.md" && (kind.is_file() || kind.is_symlink()) { + is_skill = true; + } + if kind.is_file() + && (entry.file_name() == "CLAUDE.md" + || (path.ends_with(".claude/commands") + && child.extension().is_some_and(|e| e == "md"))) + { + report.claude_files.push(child.clone()); + } + if kind.is_dir() { + children.push(child); + } else if kind.is_symlink() && target_root(&path, context) { + match fs::canonicalize(&child) { + Ok(real) + if !options.include_worktrees + && prune_worktree(worktree_ancestor(&real), &child, &mut report) => {} + Ok(real) if real.join("SKILL.md").is_file() => { + found.insert(child); + } + Ok(_) => {} + Err(e) => report.problems.push(ScanProblem { + path: child, + message: format!("Broken Agent target: {e}"), + }), + } + } else if kind.is_symlink() && target_root(&child, context) { + // A target root can itself be a symlink. Only inspect this known boundary. + let real = match fs::canonicalize(&child) { + Ok(real) => real, + Err(error) => { + report.problems.push(ScanProblem { + path: child, + message: format!("Broken Agent target root: {error}"), + }); + continue; + } + }; + if !options.include_worktrees + && prune_worktree(worktree_ancestor(&real), &child, &mut report) + { + continue; + } + match fs::read_dir(&child) { + Ok(entries) => { + for e in entries { + match e { + Ok(e) if !excluded.is_match(e.path()) => { + let real = match fs::canonicalize(e.path()) { + Ok(real) => real, + Err(error) => { + report.problems.push(ScanProblem { + path: e.path(), + message: format!("Broken Agent target: {error}"), + }); + continue; + } + }; + if let Some(home) = &home_boundary + && !real.starts_with(home) + { + report.skipped_directories += 1; + report.problems.push(ScanProblem { path: e.path(), message: "Link points outside the home directory. Pass its root explicitly to scan it.".into() }); + continue; + } + if !options.include_worktrees + && prune_worktree( + worktree_ancestor(&real), + &e.path(), + &mut report, + ) + { + continue; + } + if e.path().join("SKILL.md").is_file() { + found.insert(e.path()); + } + } + Ok(_) => {} + Err(e) => report.problems.push(ScanProblem { + path: child.clone(), + message: e.to_string(), + }), + } + } + } + Err(e) => report.problems.push(ScanProblem { + path: child, + message: e.to_string(), + }), + } + } + } + if is_skill { + found.insert(path.clone()); + // A repository can ship SKILL.md at its root and still contain Agent targets. + if !path.join(".git").exists() && !path.join("package.json").is_file() { + continue; + } + } + if depth == options.max_depth { + if !children.is_empty() { + report.problems.push(ScanProblem { + path, + message: "Maximum scan depth reached.".into(), + }); + } + } else { + pending.extend(children.into_iter().map(|p| (p, depth + 1))); + } + } + let mut by_canonical: BTreeMap> = BTreeMap::new(); + for path in found { + match fs::canonicalize(&path) { + Ok(canonical) + if home_boundary + .as_ref() + .is_some_and(|home| !canonical.starts_with(home)) => + { + report.skipped_directories += 1; + report.problems.push(ScanProblem { path, message: "Link points outside the home directory. Pass its root explicitly to scan it.".into() }); + } + Ok(canonical) => match location(&path, context) { + Ok(location) => by_canonical.entry(canonical).or_default().push(location), + Err(error) => report.problems.push(ScanProblem { + path, + message: format!("Cannot read Skill location: {error}"), + }), + }, + Err(e) => report.problems.push(ScanProblem { + path, + message: e.to_string(), + }), + } + } + let mut locks: BTreeMap> = BTreeMap::new(); + let total = by_canonical.len(); + for (canonical, mut paths) in by_canonical { + progress(ScanProgress::from_report( + &report, + ScanPhase::Fingerprint { + completed: report.skills.len(), + total, + }, + canonical.clone(), + total, + )); + paths.sort_by(|a, b| a.path.cmp(&b.path)); + let name = paths + .iter() + .find(|p| p.agent.is_some()) + .unwrap_or(&paths[0]) + .path + .file_name() + .unwrap_or_default() + .to_string_lossy() + .into_owned(); + if SkillName::parse(name.clone()).is_err() { + report.problems.push(ScanProblem { + path: canonical.clone(), + message: "Invalid Skill directory name.".into(), + }); + } + let owner = owner( + &canonical, + &name, + &paths, + context, + home_boundary.as_deref(), + &mut locks, + &mut report.problems, + ); + let fingerprint = if canonical.join(".git").exists() && matches!(owner, DoctorOwner::Source) + { + None + } else { + match fingerprint(&canonical) { + Ok(f) => Some(f), + Err(e) => { + report.problems.push(ScanProblem { + path: canonical.clone(), + message: e.to_string(), + }); + None + } + } + }; + report.skills.push(DoctorSkill { + name, + canonical_path: canonical, + paths, + owner, + fingerprint, + duplicates: vec![], + source_match: None, + }); + } + let mut hashes: BTreeMap> = BTreeMap::new(); + for skill in &report.skills { + if let Some(f) = &skill.fingerprint { + hashes + .entry(f.sha256.clone()) + .or_default() + .push(skill.canonical_path.clone()); + } + } + for skill in &mut report.skills { + if let Some(f) = &skill.fingerprint { + skill.duplicates = hashes[&f.sha256] + .iter() + .filter(|p| **p != skill.canonical_path) + .cloned() + .collect(); + } + } + report.skills.sort_by(|a, b| { + a.owner + .label() + .cmp(b.owner.label()) + .then(a.name.cmp(&b.name)) + .then(a.canonical_path.cmp(&b.canonical_path)) + }); + report.claude_files.sort(); + report.claude_files.dedup(); + progress(ScanProgress::from_report( + &report, + ScanPhase::Fingerprint { + completed: total, + total, + }, + roots[0].clone(), + total, + )); + Ok(report) +} + +fn owner( + canonical: &Path, + name: &str, + paths: &[SkillLocation], + context: &ScanContext, + home_boundary: Option<&Path>, + locks: &mut BTreeMap>, + problems: &mut Vec, +) -> DoctorOwner { + if canonical.components().any(|p| p.as_os_str() == "plugins") + || canonical.starts_with(context.home.join(".codex/skills/.system")) + { + return DoctorOwner::Plugin; + } + let stores = paths + .iter() + .filter(|p| p.agent.is_some() && p.project_root.is_none()) + .map(|_| context.global_store.clone()) + .chain( + paths + .iter() + .filter_map(|p| p.project_root.as_ref().map(|r| r.join(".skills"))), + ) + .chain(canonical.ancestors().take(2).map(Path::to_path_buf)) + .collect::>(); + for store in stores { + let lock = store.join("skilld-lock.yaml"); + if let Some(owner) = outside_metadata(&lock, context, home_boundary, problems) { + return owner; + } + if !lock.is_file() { + continue; + } + if store.join(name) == canonical { + return DoctorOwner::Skilld { store }; + } + match fs::read(&lock).map_err(|e| e.to_string()).and_then(|b| { + serde_json::from_slice::(&b).map_err(|e| e.to_string()) + }) { + Ok(v) if v["skills"].get(name).is_some() => return DoctorOwner::Skilld { store }, + Ok(_) => {} + Err(message) => { + problems.push(ScanProblem { + path: lock, + message: message.clone(), + }); + return DoctorOwner::Unavailable { message }; + } + } + } + for location in paths.iter().filter(|p| p.agent.is_some()) { + let global = location.project_root.is_none(); + let lock = location.project_root.as_ref().map_or_else( + || context.skills_sh_global_lock.clone(), + |r| r.join("skills-lock.json"), + ); + if let Some(owner) = outside_metadata(&lock, context, home_boundary, problems) { + return owner; + } + if !locks.contains_key(&lock) { + let records = if fs::symlink_metadata(&lock).is_ok() { + match read_foreign_lock(&lock, global) { + Ok(r) => r, + Err(e) => { + let message = e.to_string(); + problems.push(ScanProblem { + path: lock.clone(), + message: message.clone(), + }); + return DoctorOwner::Unavailable { message }; + } + } + } else { + vec![] + }; + locks.insert(lock.clone(), records); + } + if let Some(record) = locks[&lock].iter().find(|r| r.name == name) { + return DoctorOwner::SkillsSh { + record: record.clone(), + }; + } + } + if paths.iter().any(|p| p.agent.is_some()) { + DoctorOwner::Unknown + } else { + DoctorOwner::Source + } +} + +fn outside_metadata( + path: &Path, + context: &ScanContext, + home_boundary: Option<&Path>, + problems: &mut Vec, +) -> Option { + let home = home_boundary?; + if path.starts_with(&context.home) + && fs::canonicalize(path).map_or(true, |real| real.starts_with(home)) + { + return None; + } + let message = "Ownership metadata is outside the home directory. Pass scan roots explicitly to inspect it.".to_owned(); + problems.push(ScanProblem { + path: path.to_owned(), + message: message.clone(), + }); + Some(DoctorOwner::Unavailable { message }) +} + +pub fn render_plain(report: &DoctorReport) -> String { + use skilld_ui::text::sanitize; + let mut text = format!( + "{} Skills. {} directories scanned. {} directories excluded.\n", + report.skills.len(), + report.visited_directories, + report.skipped_directories + ); + let mut last = ""; + for skill in &report.skills { + let label = skill.owner.label(); + if label != last { + text.push_str(&format!("\n{label}\n")); + last = label; + } + text.push_str(&format!( + " {} {} {} Agent paths{}\n", + sanitize(&skill.name), + sanitize(&skill.canonical_path.display().to_string()), + skill.paths.iter().filter(|p| p.agent.is_some()).count(), + if skill.duplicates.is_empty() { + "" + } else { + " identical copies" + } + )); + if let Some(source) = &skill.source_match { + text.push_str(&format!( + " {}: {}\n", + sanitize(&source.source), + sanitize(&source.result) + )); + } + } + text.push_str(&format!( + "\n{} Claude instruction files. {} scan problems.\n", + report.claude_files.len(), + report.problems.len() + )); + for p in &report.problems { + text.push_str(&format!( + " {}: {}\n", + sanitize(&p.path.display().to_string()), + sanitize(&p.message) + )); + } + text +} diff --git a/crates/skilld-command/src/doctor_actions.rs b/crates/skilld-command/src/doctor_actions.rs new file mode 100644 index 00000000..0b35c0b1 --- /dev/null +++ b/crates/skilld-command/src/doctor_actions.rs @@ -0,0 +1,678 @@ +//! Reviewed, reversible changes. Discovery never calls this module's apply path. +use std::collections::{BTreeMap, BTreeSet}; +use std::fs; +use std::io::Write; +use std::path::{Path, PathBuf}; + +use crate::doctor::{ + DoctorOptions, DoctorOwner, DoctorReport, DoctorSkill, ScanContext, ScanPhase, ScanProgress, + SourceMatch, +}; +use crate::doctor_metadata::{ForeignRecord, fingerprint, foreign_lock_without}; +use crate::{ + CommandError, LocalHost, LocalStore, PreparedRemoteSkill, ResolvedTarget, TargetInstall, +}; +use serde::Serialize; +use skilld_core::{InstallMode, InstallScope, LockedSource, RemoteSelector, SkillName}; + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum DoctorAction { + Migrate, + Remove, +} + +#[derive(Clone, Debug, Serialize)] +pub struct ActionPreview { + pub action: String, + pub name: String, + pub paths: Vec, + pub source: Option, + pub commit: Option, + pub changes_content: bool, + pub behaviors: Vec, +} + +pub struct DoctorPlan { + pub preview: ActionPreview, + action: DoctorAction, + copies: Vec, + paths: Vec, + observed_paths: Vec<(PathBuf, PathBuf, Option)>, + absent_targets: Vec, + record: Option, + store: LocalStore, + targets: Vec, + prepared: Option<(PreparedRemoteSkill, crate::StagedRemote)>, + allowed_behaviors: Vec, +} + +#[derive(Debug)] +pub struct DoctorApplied { + pub backup: PathBuf, + pub message: String, +} + +fn failure(message: impl Into) -> CommandError { + CommandError::config(message) +} + +fn same_record(a: &ForeignRecord, b: &ForeignRecord) -> bool { + a.lockfile == b.lockfile && a.name == b.name +} + +fn source_selector(record: &ForeignRecord) -> Result { + if record.source_type != "github" { + return Err(failure( + "Migration requires a GitHub source. Keep or remove this Skill.", + )); + } + let mut source = if let Some(path) = &record.skill_path { + let path = path.strip_suffix("/SKILL.md").unwrap_or(path); + let path = if path == "SKILL.md" { "." } else { path }; + format!("github:{}/{}", record.source, path) + } else { + format!("{}/{}", record.source, record.name) + }; + if let Some(reference) = &record.revision { + source.push('#'); + source.push_str(reference); + } + RemoteSelector::parse(&source).map_err(CommandError::remote) +} + +impl LocalHost { + pub fn doctor_context(&self) -> Result { + let lock = std::env::var_os("XDG_STATE_HOME") + .filter(|s| !s.is_empty()) + .map(|p| PathBuf::from(p).join("skills/.skill-lock.json")) + .unwrap_or_else(|| self.target_roots.home.join(".agents/.skill-lock.json")); + let mut global_targets = self.known_targets(InstallScope::Global)?; + global_targets.push( + ResolvedTarget::new( + skilld_core::AgentTargetId::Codex, + self.target_roots.home.join(".codex/skills"), + ) + .map_err(CommandError::store)?, + ); + Ok(ScanContext { + home: self.target_roots.home.clone(), + global_store: self.global_root.join("skills"), + global_targets, + skills_sh_global_lock: lock, + }) + } + + pub fn doctor_scan(&self, options: &DoctorOptions) -> Result { + self.doctor_scan_with_progress(options, &mut |_| {}) + } + + pub fn doctor_scan_with_progress( + &self, + options: &DoctorOptions, + progress: &mut impl FnMut(ScanProgress), + ) -> Result { + let mut report = + crate::doctor::scan_with_progress(options, &self.doctor_context()?, progress)?; + if options.check_sources { + let mut checked = 0; + let total = report + .skills + .iter() + .filter(|s| matches!(s.owner, DoctorOwner::SkillsSh { .. } | DoctorOwner::Unknown)) + .count() + .min(20); + let mut status = ScanProgress::from_report( + &report, + ScanPhase::Sources { + completed: 0, + total, + }, + PathBuf::new(), + report.skills.len(), + ); + for skill in &mut report.skills { + if checked >= 20 { + break; + } + if !matches!( + skill.owner, + DoctorOwner::SkillsSh { .. } | DoctorOwner::Unknown + ) { + continue; + } + status.phase = ScanPhase::Sources { + completed: checked, + total, + }; + status.current_path = skill.canonical_path.clone(); + progress(status.clone()); + checked += 1; + let selector = match &skill.owner { + DoctorOwner::SkillsSh { record } => source_selector(record), + DoctorOwner::Unknown => { + let result = self + .remote_provider()? + .search(&skill.name, 5) + .map_err(CommandError::remote); + match result { + Ok(results) => { + let matches = results + .items + .into_iter() + .filter(|r| r.name == skill.name) + .collect::>(); + if matches.len() != 1 { + skill.source_match = Some(SourceMatch { + source: skill.name.clone(), + commit: None, + result: "No unique repository candidate.".into(), + }); + continue; + } + matches[0].selector().map_err(CommandError::remote) + } + Err(e) => { + skill.source_match = Some(SourceMatch { + source: skill.name.clone(), + commit: None, + result: e.to_string(), + }); + continue; + } + } + } + _ => continue, + }; + let result = selector.and_then(|selector| { + let prepared = self + .remote_provider()? + .prepare(&selector, false) + .map_err(CommandError::remote)?; + let staged = crate::materialize_remote(&prepared)?; + let current = fingerprint(staged.path())?; + Ok((prepared, current)) + }); + skill.source_match = Some(match result { + Ok((prepared, current)) => { + let (source, commit) = match prepared.locked_source { + LockedSource::Remote { + source, commit_sha, .. + } => (source, Some(commit_sha)), + _ => (skill.name.clone(), None), + }; + SourceMatch { + source, + commit, + result: if skill + .fingerprint + .as_ref() + .is_some_and(|f| f.git_tree == current.git_tree) + { + "Exact directory match.".into() + } else { + "Source contents differ. Review before replacement.".into() + }, + } + } + Err(e) => SourceMatch { + source: skill.name.clone(), + commit: None, + result: e.to_string(), + }, + }); + } + } + Ok(report) + } + + pub fn doctor_plan( + &self, + report: &DoctorReport, + index: usize, + action: DoctorAction, + ) -> Result { + let selected = report + .skills + .get(index) + .ok_or_else(|| failure("Select a Skill first."))?; + let record = match &selected.owner { + DoctorOwner::SkillsSh { record } => Some(record.clone()), + DoctorOwner::Unknown if action == DoctorAction::Remove => None, + _ => { + return Err(failure( + "This action requires a skills.sh install or an unknown Agent target.", + )); + } + }; + let copies = report + .skills + .iter() + .filter(|s| match (&record, &s.owner) { + (Some(a), DoctorOwner::SkillsSh { record: b }) => same_record(a, b), + (None, _) => s.canonical_path == selected.canonical_path, + _ => false, + }) + .cloned() + .collect::>(); + let locations = copies + .iter() + .flat_map(|s| s.paths.iter()) + .filter(|p| p.agent.is_some()) + .collect::>(); + let first = locations + .first() + .ok_or_else(|| failure("The Skill has no Agent target to change."))?; + let scope_root = first.project_root.clone(); + if locations.iter().any(|p| p.project_root != scope_root) { + return Err(failure( + "These paths span different scopes. Scan and change one scope at a time.", + )); + } + let store = LocalStore::new( + scope_root + .as_ref() + .map_or_else(|| self.global_root.join("skills"), |p| p.join(".skills")), + ); + if fs::symlink_metadata(store.root().join(&selected.name)).is_ok() { + return Err(failure( + "skilld already contains this name. Resolve that installation first.", + )); + } + // A metadata entry cannot justify changing an Agent target outside the scan. + let all_targets = if let Some(root) = &scope_root { + skilld_core::AGENT_TARGETS + .iter() + .map(|t| ResolvedTarget::new(t.id, root.join(t.project_skills_dir))) + .collect::, _>>() + .map_err(CommandError::store)? + } else { + self.doctor_context()?.global_targets + }; + let mut paths = BTreeSet::new(); + let mut observed_paths = vec![]; + let mut targets = BTreeMap::new(); + let managed_targets = if scope_root.is_some() { + all_targets.clone() + } else { + self.known_targets(InstallScope::Global)? + }; + for location in &locations { + let parent = location + .path + .parent() + .ok_or_else(|| failure("The Agent target has no parent."))?; + let real_parent = + fs::canonicalize(parent).map_err(|e| CommandError::filesystem(e.to_string()))?; + let filename = location + .path + .file_name() + .ok_or_else(|| failure("The Agent target has no Skill name."))?; + if action == DoctorAction::Migrate && filename != selected.name.as_str() { + return Err(failure( + "An Agent target uses another name. Migration cannot rename it.", + )); + } + let path = real_parent.join(filename); + let agent = location.agent.expect("filtered Agent target"); + if action == DoctorAction::Migrate + && !managed_targets + .iter() + .any(|t| t.agent == agent && t.root == parent && t.root == real_parent) + { + return Err(failure( + "Migration requires a standard Agent target root. Keep or remove this installation.", + )); + } + if let Some(previous) = targets.get(&agent) { + let previous: &TargetInstall = previous; + if action == DoctorAction::Migrate && previous.target.root != real_parent { + return Err(failure( + "One Agent has multiple target roots. Resolve them separately.", + )); + } + } + let mode = if fs::symlink_metadata(&path) + .map_err(|e| CommandError::filesystem(e.to_string()))? + .file_type() + .is_symlink() + { + InstallMode::Symlink + } else { + InstallMode::Copy + }; + let link = if mode == InstallMode::Symlink { + Some(fs::read_link(&path).map_err(|e| CommandError::filesystem(e.to_string()))?) + } else { + None + }; + observed_paths.push((location.path.clone(), path.clone(), link)); + targets.insert( + agent, + TargetInstall { + target: ResolvedTarget::new(agent, real_parent).map_err(CommandError::store)?, + mode, + }, + ); + paths.insert(path); + } + let mut absent_targets = vec![store.root().join(&selected.name)]; + for target in all_targets { + let path = target.root.join(&selected.name); + if fs::symlink_metadata(&path).is_ok() { + let real = fs::canonicalize(&target.root) + .map_err(|e| CommandError::filesystem(e.to_string()))? + .join(&selected.name); + if !paths.contains(&real) { + return Err(failure(format!( + "Another Agent target was outside this scan: {}. Scan the complete scope first.", + path.display() + ))); + } + } else { + absent_targets.push(path); + } + } + if let Some(record) = &record { + foreign_lock_without(&record.lockfile, record)?; + let document: serde_json::Value = serde_json::from_slice( + &fs::read(&record.lockfile).map_err(|e| CommandError::filesystem(e.to_string()))?, + ) + .map_err(|e| failure(e.to_string()))?; + if document["skills"][&record.name]["subagents"] + .as_array() + .is_some_and(|v| !v.is_empty()) + { + return Err(failure( + "This Skill has subagent targets. Use skills.sh to change those targets first.", + )); + } + } + let mut preview = ActionPreview { + action: if action == DoctorAction::Migrate { + "Migrate to skilld" + } else { + "Remove" + } + .into(), + name: selected.name.clone(), + paths: paths.iter().cloned().collect(), + source: None, + commit: None, + changes_content: false, + behaviors: vec![], + }; + let mut allowed_behaviors = vec![]; + let prepared = if action == DoctorAction::Migrate { + let record = record.as_ref().expect("migration has a record"); + let selected_hash = selected + .fingerprint + .as_ref() + .ok_or_else(|| failure("The Skill directory could not be read."))?; + if copies.iter().any(|s| { + s.fingerprint + .as_ref() + .is_none_or(|f| f.git_tree != selected_hash.git_tree) + }) { + return Err(failure( + "Installed copies differ. Keep them or remove selected copies before migration.", + )); + } + let selector = source_selector(record)?; + let prepared = self + .remote_provider()? + .prepare(&selector, false) + .map_err(CommandError::remote)?; + if prepared + .skill_name() + .map_err(CommandError::remote)? + .as_str() + != selected.name + { + return Err(failure( + "The source uses another Skill name. Migration cannot rename Agent targets.", + )); + } + let staged = crate::materialize_remote(&prepared)?; + let current = fingerprint(staged.path())?; + preview.changes_content = selected_hash.git_tree != current.git_tree; + // Project computedHash uses locale-specific JS ordering. Exact remote bytes + // establish identity without pretending that hash has Git tree semantics. + if preview.changes_content + && record.expected_tree.as_ref() != Some(&selected_hash.git_tree) + { + return Err(failure( + "Source contents differ and the installed directory does not match a recorded Git tree. Migration stopped.", + )); + } + let behaviors = skilld_core::detect_behaviors(&prepared.files); + let held = crate::held_behaviors(&behaviors, &[], &[]); + preview.behaviors = held.iter().map(|b| crate::describe_behavior(b)).collect(); + allowed_behaviors = held.iter().map(|b| b.id.to_owned()).collect(); + if let LockedSource::Remote { + source, commit_sha, .. + } = &prepared.locked_source + { + preview.source = Some(source.clone()); + preview.commit = Some(commit_sha.clone()); + } + Some((prepared, staged)) + } else { + None + }; + Ok(DoctorPlan { + preview, + action, + copies, + paths: paths.into_iter().collect(), + observed_paths, + absent_targets, + record, + store, + targets: targets.into_values().collect(), + prepared, + allowed_behaviors, + }) + } +} + +impl DoctorPlan { + pub fn apply(self) -> Result { + for path in &self.absent_targets { + match fs::symlink_metadata(path) { + Ok(_) => { + return Err(failure( + "An installation appeared after review. Scan it again.", + )); + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => return Err(CommandError::filesystem(e.to_string())), + } + } + if let Some((prepared, _)) = &self.prepared { + crate::gate_new_behaviors( + |_| Ok(crate::BehaviorDecision::Unavailable), + &self.preview.name, + crate::BehaviorChange::Install, + &skilld_core::detect_behaviors(&prepared.files), + &[], + &self.allowed_behaviors, + &prepared.behavior_readings, + )?; + } + for (observed, physical, link) in &self.observed_paths { + let parent = fs::canonicalize(observed.parent().expect("planned parent")) + .map_err(|e| CommandError::filesystem(e.to_string()))?; + if parent.join(observed.file_name().expect("planned name")) != *physical { + return Err(failure( + "An Agent target root changed after review. Scan it again.", + )); + } + let metadata = fs::symlink_metadata(physical) + .map_err(|e| CommandError::filesystem(e.to_string()))?; + let current_link = if metadata.file_type().is_symlink() { + Some(fs::read_link(physical).map_err(|e| CommandError::filesystem(e.to_string()))?) + } else { + None + }; + if current_link != *link { + return Err(failure( + "An Agent target link changed after review. Scan it again.", + )); + } + } + for skill in &self.copies { + let expected = skill + .fingerprint + .as_ref() + .ok_or_else(|| failure("The Skill directory could not be read."))?; + for location in skill.paths.iter().filter(|p| p.agent.is_some()) { + let real = fs::canonicalize(&location.path) + .map_err(|e| CommandError::filesystem(e.to_string()))?; + if real != skill.canonical_path || fingerprint(&real)? != *expected { + return Err(failure("The Skill changed after review. Scan it again.")); + } + } + } + let foreign_new = if let Some(r) = &self.record { + Some(foreign_lock_without(&r.lockfile, r)?) + } else { + None + }; + let parent = self + .store + .root() + .parent() + .ok_or_else(|| failure("The Skill store has no parent."))?; + let backup_root = parent.join(".skilld-doctor-backups"); + fs::create_dir_all(&backup_root).map_err(|e| CommandError::filesystem(e.to_string()))?; + let backup = tempfile::Builder::new() + .prefix("action-") + .tempdir_in(&backup_root) + .map_err(|e| CommandError::filesystem(e.to_string()))? + .keep(); + let moves = self + .paths + .iter() + .enumerate() + .map(|(i, p)| (p.clone(), backup.join(format!("target-{i}")))) + .collect::>(); + let receipt = serde_json::json!({"version":1,"action":self.preview.action,"name":self.preview.name,"paths":moves,"foreignLock":self.record.as_ref().map(|r|&r.lockfile)}); + fs::write( + backup.join("recovery.json"), + serde_json::to_vec_pretty(&receipt) + .map_err(|e| CommandError::filesystem(e.to_string()))?, + ) + .map_err(|e| CommandError::filesystem(e.to_string()))?; + if let Some(r) = &self.record { + fs::copy(&r.lockfile, backup.join("skills-lock.json")) + .map_err(|e| CommandError::filesystem(e.to_string()))?; + } + let known = self + .targets + .iter() + .map(|t| t.target.clone()) + .collect::>(); + let mut moved = vec![]; + let mut installed = false; + let result = (|| { + for (original, saved) in &moves { + fs::rename(original, saved).map_err(|e| { + CommandError::filesystem(format!("Cannot back up {}: {e}", original.display())) + })?; + moved.push((original, saved)); + } + if let Some((prepared, staged)) = &self.prepared { + let outcome = self.store.install_from_with_status( + staged.path(), + prepared.locked_source.clone(), + prepared.source_status.clone(), + &self.targets, + &known, + ); + // A cleanup error can follow a committed installation. Roll it back too. + installed = outcome.is_ok() + || outcome + .as_ref() + .is_err_and(|e| e.code() == "COMMITTED_CLEANUP_PENDING"); + outcome.map_err(CommandError::store)?; + installed = true; + let name = + SkillName::parse(self.preview.name.clone()).map_err(CommandError::domain)?; + self.store + .verify_content(&name, &known) + .map_err(CommandError::store)?; + } + if let (Some(record), Some(bytes)) = (&self.record, &foreign_new) { + foreign_lock_without(&record.lockfile, record)?; + atomic_write(&record.lockfile, bytes)?; + } + Ok(()) + })(); + if let Err(error) = result { + if installed { + let name = + SkillName::parse(self.preview.name.clone()).map_err(CommandError::domain)?; + self.store.remove(&name, &known).map_err(|rollback| { + CommandError::filesystem(format!( + "{error}. Rollback failed: {rollback}. Recovery files: {}", + backup.display() + )) + })?; + } + for (original, saved) in moved.into_iter().rev() { + if fs::symlink_metadata(original).is_ok() { + return Err(CommandError::filesystem(format!( + "{error}. A target changed during rollback. Recovery files: {}", + backup.display() + ))); + } + fs::rename(saved, original).map_err(|rollback| { + CommandError::filesystem(format!( + "{error}. Rollback failed: {rollback}. Recovery files: {}", + backup.display() + )) + })?; + } + return Err(error); + } + Ok(DoctorApplied { + backup, + message: format!( + "{}: {}", + if self.action == DoctorAction::Migrate { + "Migrated" + } else { + "Removed" + }, + self.preview.name + ), + }) + } +} + +fn atomic_write(path: &Path, bytes: &[u8]) -> Result<(), CommandError> { + let parent = path + .parent() + .ok_or_else(|| failure("The lockfile has no parent."))?; + let mut staged = tempfile::NamedTempFile::new_in(parent) + .map_err(|e| CommandError::filesystem(e.to_string()))?; + staged + .as_file() + .set_permissions( + fs::metadata(path) + .map_err(|e| CommandError::filesystem(e.to_string()))? + .permissions(), + ) + .map_err(|e| CommandError::filesystem(e.to_string()))?; + staged + .write_all(bytes) + .map_err(|e| CommandError::filesystem(e.to_string()))?; + staged + .as_file() + .sync_all() + .map_err(|e| CommandError::filesystem(e.to_string()))?; + staged + .persist(path) + .map_err(|e| CommandError::filesystem(e.to_string()))?; + Ok(()) +} diff --git a/crates/skilld-command/src/doctor_metadata.rs b/crates/skilld-command/src/doctor_metadata.rs new file mode 100644 index 00000000..934ff36a --- /dev/null +++ b/crates/skilld-command/src/doctor_metadata.rs @@ -0,0 +1,569 @@ +use std::fs::{self, File, Metadata}; +use std::io::Read; +use std::path::{Component, Path, PathBuf}; + +use serde::{Deserialize, Serialize}; +use serde_json::Value; +use sha1::Sha1; +use sha2::{Digest, Sha256}; + +use crate::CommandError; + +const MAX_DEPTH: usize = 32; +const MAX_FILES: usize = 10_000; +const MAX_BYTES: u64 = 64 * 1024 * 1024; +const MAX_LOCK_BYTES: u64 = 4 * 1024 * 1024; + +#[derive(Clone, Debug, Eq, PartialEq, Serialize)] +pub struct Fingerprint { + pub sha256: String, + pub git_tree: String, + pub files: usize, + pub bytes: u64, +} + +#[derive(Clone, Debug, Serialize)] +pub struct ForeignRecord { + pub lockfile: PathBuf, + pub name: String, + pub source: String, + pub source_type: String, + pub skill_path: Option, + pub revision: Option, + pub expected_tree: Option, + pub expected_content: Option, + pub lock_sha256: String, + pub global: bool, +} + +/// Hash the complete Skill tree, excluding only `.git` entries. +/// +/// The SHA256 includes framed names, file kinds, permissions and link targets. +/// The Git SHA1 uses Git's tree encoding and omits empty directories. +/// Internal links remain links. Absolute and escaping links are rejected. +pub fn fingerprint(root: &Path) -> Result { + let metadata = fs::symlink_metadata(root).map_err(tree_io)?; + if !metadata.is_dir() || metadata.file_type().is_symlink() { + return Err(unsupported_tree()); + } + let mut budget = Budget { + files: 0, + bytes: 0, + entries: 0, + }; + let resolved_root = fs::canonicalize(root).map_err(tree_io)?; + let tree = hash_directory(root, &resolved_root, 0, &mut budget)?; + Ok(Fingerprint { + sha256: hex(&tree.sha256), + git_tree: hex(&tree.git_tree), + files: budget.files, + bytes: budget.bytes, + }) +} + +struct Budget { + files: usize, + bytes: u64, + entries: usize, +} + +struct TreeHash { + sha256: Vec, + git_tree: Vec, + has_git_entries: bool, +} + +fn hash_directory( + path: &Path, + root: &Path, + depth: usize, + budget: &mut Budget, +) -> Result { + if depth > MAX_DEPTH { + return Err(scan_limit()); + } + let before = fs::symlink_metadata(path).map_err(tree_io)?; + if !before.is_dir() || before.file_type().is_symlink() { + return Err(unsupported_tree()); + } + let mut entries = Vec::new(); + for entry in fs::read_dir(path).map_err(tree_io)? { + let entry = entry.map_err(tree_io)?; + if entry.file_name() == ".git" { + continue; + } + budget.entries += 1; + if budget.entries > MAX_FILES * 2 { + return Err(scan_limit()); + } + let metadata = fs::symlink_metadata(entry.path()).map_err(tree_io)?; + let mut sort_key = entry.file_name().as_encoded_bytes().to_vec(); + if metadata.is_dir() { + sort_key.push(b'/'); + } + entries.push((sort_key, entry, metadata)); + } + entries.sort_by(|left, right| left.0.cmp(&right.0)); + let mut digest = Sha256::new(); + frame(&mut digest, b"skilld-doctor-tree-v1"); + frame(&mut digest, &permission_mode(&before).to_be_bytes()); + let mut git_body = Vec::new(); + for (_, entry, metadata) in entries { + let name = entry.file_name(); + frame(&mut digest, name.as_encoded_bytes()); + frame(&mut digest, &permission_mode(&metadata).to_be_bytes()); + let (git_mode, object) = if metadata.is_dir() { + frame(&mut digest, b"directory"); + let child = hash_directory(&entry.path(), root, depth + 1, budget)?; + frame(&mut digest, &child.sha256); + if !child.has_git_entries { + continue; + } + ("40000", child.git_tree) + } else { + budget.files += 1; + if budget.files > MAX_FILES { + return Err(scan_limit()); + } + let (kind, mode, content) = if metadata.file_type().is_symlink() { + let target = fs::read_link(entry.path()).map_err(tree_io)?; + check_link(&target, depth)?; + // Resolve location only. Never read or recurse through link contents. + let resolved = fs::canonicalize(entry.path()).map_err(|_| unsupported_tree())?; + if !resolved.starts_with(root) { + return Err(unsupported_tree()); + } + let bytes = target.as_os_str().as_encoded_bytes().to_vec(); + add_bytes(budget, bytes.len() as u64)?; + (b"symlink".as_slice(), "120000", bytes) + } else if metadata.is_file() { + if metadata.len() > MAX_BYTES - budget.bytes { + return Err(scan_limit()); + } + let content = + read_regular(&entry.path(), &metadata, MAX_BYTES - budget.bytes, tree_io)?; + add_bytes(budget, content.len() as u64)?; + let mode = if permission_mode(&metadata) & 0o100 != 0 { + "100755" + } else { + "100644" + }; + (b"file".as_slice(), mode, content) + } else { + return Err(unsupported_tree()); + }; + ensure_unchanged(&entry.path(), &metadata, tree_io)?; + frame(&mut digest, kind); + frame(&mut digest, &content); + (mode, git_object("blob", &content)) + }; + git_body.extend_from_slice(git_mode.as_bytes()); + git_body.push(b' '); + git_body.extend_from_slice(name.as_encoded_bytes()); + git_body.push(0); + git_body.extend_from_slice(&object); + } + ensure_unchanged(path, &before, tree_io)?; + Ok(TreeHash { + sha256: digest.finalize().to_vec(), + git_tree: git_object("tree", &git_body), + has_git_entries: !git_body.is_empty(), + }) +} + +fn add_bytes(budget: &mut Budget, bytes: u64) -> Result<(), CommandError> { + if bytes > MAX_BYTES - budget.bytes { + return Err(scan_limit()); + } + budget.bytes += bytes; + Ok(()) +} + +fn check_link(target: &Path, parent_depth: usize) -> Result<(), CommandError> { + let mut depth = parent_depth; + for component in target.components() { + match component { + Component::Normal(_) => depth += 1, + Component::CurDir => {} + Component::ParentDir if depth > 0 => depth -= 1, + _ => return Err(unsupported_tree()), + } + } + Ok(()) +} + +fn frame(hash: &mut Sha256, bytes: &[u8]) { + hash.update((bytes.len() as u64).to_be_bytes()); + hash.update(bytes); +} + +fn git_object(kind: &str, body: &[u8]) -> Vec { + let mut hash = Sha1::new(); + hash.update(format!("{kind} {}\0", body.len())); + hash.update(body); + hash.finalize().to_vec() +} + +fn hex(bytes: &[u8]) -> String { + bytes.iter().map(|byte| format!("{byte:02x}")).collect() +} + +#[cfg(unix)] +fn permission_mode(metadata: &Metadata) -> u32 { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o7777 +} + +#[cfg(not(unix))] +fn permission_mode(metadata: &Metadata) -> u32 { + if metadata.permissions().readonly() { + 0o444 + } else { + 0o644 + } +} + +fn same_metadata(left: &Metadata, right: &Metadata) -> bool { + let same = left.file_type() == right.file_type() + && left.len() == right.len() + && left.modified().ok() == right.modified().ok() + && permission_mode(left) == permission_mode(right); + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + same && left.dev() == right.dev() + && left.ino() == right.ino() + && left.ctime() == right.ctime() + && left.ctime_nsec() == right.ctime_nsec() + } + #[cfg(not(unix))] + { + same + } +} + +fn ensure_unchanged( + path: &Path, + before: &Metadata, + error: fn(std::io::Error) -> CommandError, +) -> Result<(), CommandError> { + let after = fs::symlink_metadata(path).map_err(error)?; + if !same_metadata(before, &after) { + return Err(CommandError::operation( + "DOCTOR_CONTENT_CHANGED", + "Skill files changed during the scan. Run doctor again.", + )); + } + Ok(()) +} + +fn read_regular( + path: &Path, + before: &Metadata, + limit: u64, + error: fn(std::io::Error) -> CommandError, +) -> Result, CommandError> { + let file = File::open(path).map_err(error)?; + let opened = file.metadata().map_err(error)?; + if !opened.is_file() || !same_metadata(before, &opened) { + return Err(CommandError::operation( + "DOCTOR_CONTENT_CHANGED", + "Skill files changed during the scan. Run doctor again.", + )); + } + let mut bytes = Vec::new(); + file.take(limit + 1) + .read_to_end(&mut bytes) + .map_err(error)?; + if bytes.len() as u64 > limit { + return Err(scan_limit()); + } + ensure_unchanged(path, before, error)?; + Ok(bytes) +} + +/// Read known skills.sh formats without exposing URLs or arbitrary metadata. +pub fn read_foreign_lock(path: &Path, global: bool) -> Result, CommandError> { + let bytes = read_lock_bytes(path)?; + let document = parse_lock(&bytes, global)?; + records_from_document(path, global, &bytes, &document) +} + +/// Prepare a foreign lockfile with exactly one selected Skill removed. +/// The caller owns atomic replacement and rollback across both installers. +pub fn foreign_lock_without(path: &Path, record: &ForeignRecord) -> Result, CommandError> { + if path != record.lockfile { + return Err(stale_lock()); + } + let bytes = read_lock_bytes(path)?; + if hex(&Sha256::digest(&bytes)) != record.lock_sha256 { + return Err(stale_lock()); + } + let mut document = parse_lock(&bytes, record.global)?; + let skills = document + .get_mut("skills") + .and_then(Value::as_object_mut) + .ok_or_else(invalid_lock)?; + if skills.remove(&record.name).is_none() { + return Err(stale_lock()); + } + let mut output = serde_json::to_vec_pretty(&document).map_err(|_| invalid_lock())?; + output.push(b'\n'); + Ok(output) +} + +fn read_lock_bytes(path: &Path) -> Result, CommandError> { + let metadata = fs::symlink_metadata(path).map_err(lock_io)?; + if !metadata.is_file() || metadata.file_type().is_symlink() || metadata.len() > MAX_LOCK_BYTES { + return Err(invalid_lock()); + } + read_regular(path, &metadata, MAX_LOCK_BYTES, lock_io) +} + +fn parse_lock(bytes: &[u8], global: bool) -> Result { + let UniqueValue(document) = serde_json::from_slice(bytes).map_err(|_| invalid_lock())?; + if document.get("version").and_then(Value::as_u64) != Some(if global { 3 } else { 1 }) + || document.get("skills").and_then(Value::as_object).is_none() + { + return Err(invalid_lock()); + } + Ok(document) +} + +fn records_from_document( + path: &Path, + global: bool, + bytes: &[u8], + document: &Value, +) -> Result, CommandError> { + let skills = document + .get("skills") + .and_then(Value::as_object) + .ok_or_else(invalid_lock)?; + if skills.len() > MAX_FILES { + return Err(invalid_lock()); + } + let lock_sha256 = hex(&Sha256::digest(bytes)); + skills + .iter() + .map(|(name, entry)| { + if !safe_component(name) || !entry.is_object() { + return Err(invalid_lock()); + } + let source = required_text(entry, "source", 4096)?; + let source_type = required_text(entry, "sourceType", 64)?; + if !safe_component(&source_type) + || !source_without_credentials(&source) + || (source_type == "github" && !github_source(&source)) + { + return Err(invalid_lock()); + } + let skill_path = optional_text(entry, "skillPath", 4096)?; + if skill_path + .as_ref() + .is_some_and(|path| !relative_source_path(path)) + { + return Err(invalid_lock()); + } + let revision = optional_text(entry, "ref", 1024)?; + let raw_hash = optional_text( + entry, + if global { + "skillFolderHash" + } else { + "computedHash" + }, + 64, + )?; + if raw_hash.as_ref().is_some_and(|hash| { + !hash.bytes().all(|byte| byte.is_ascii_hexdigit()) + || (!matches!(hash.len(), 40 | 64)) + || (!global && hash.len() != 64) + }) { + return Err(invalid_lock()); + } + let expected_tree = if global && source_type == "github" { + raw_hash.clone().filter(|hash| hash.len() == 40) + } else { + None + }; + let expected_content = raw_hash.filter(|hash| hash.len() == 64); + Ok(ForeignRecord { + lockfile: path.to_owned(), + name: name.clone(), + source, + source_type, + skill_path, + revision, + expected_tree, + expected_content, + lock_sha256: lock_sha256.clone(), + global, + }) + }) + .collect() +} + +fn required_text(entry: &Value, field: &str, limit: usize) -> Result { + optional_text(entry, field, limit)?.ok_or_else(invalid_lock) +} + +fn optional_text(entry: &Value, field: &str, limit: usize) -> Result, CommandError> { + match entry.get(field) { + None | Some(Value::Null) => Ok(None), + Some(Value::String(value)) if value.is_empty() => Ok(None), + Some(Value::String(value)) + if value.len() <= limit + && value.trim() == value + && !value.chars().any(char::is_control) => + { + Ok(Some(value.clone())) + } + _ => Err(invalid_lock()), + } +} + +fn safe_component(value: &str) -> bool { + !value.is_empty() + && value.len() <= 255 + && value != "." + && value != ".." + && value + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'.' | b'_' | b'-')) +} + +fn github_source(value: &str) -> bool { + let parts: Vec<_> = value.split('/').collect(); + parts.len() == 2 && parts.iter().all(|part| safe_component(part)) +} + +fn relative_source_path(value: &str) -> bool { + !value.contains(['\\', ':']) + && value + .split('/') + .all(|part| !part.is_empty() && part != "." && part != "..") +} + +fn source_without_credentials(source: &str) -> bool { + if source.contains("://") { + return url::Url::parse(source).is_ok_and(|url| { + url.username().is_empty() + && url.password().is_none() + && url.query().is_none() + && url.fragment().is_none() + }); + } + true +} + +/// Duplicate keys make surgical edits ambiguous. Reject them at every depth. +struct UniqueValue(Value); + +impl<'de> Deserialize<'de> for UniqueValue { + fn deserialize>(deserializer: D) -> Result { + struct Visitor; + impl<'de> serde::de::Visitor<'de> for Visitor { + type Value = UniqueValue; + + fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str("JSON with unique object keys") + } + + fn visit_map>( + self, + mut map: A, + ) -> Result { + let mut values = serde_json::Map::new(); + while let Some((key, UniqueValue(value))) = + map.next_entry::()? + { + if values.insert(key, value).is_some() { + return Err(serde::de::Error::custom("duplicate object key")); + } + } + Ok(UniqueValue(Value::Object(values))) + } + + fn visit_seq>( + self, + mut sequence: A, + ) -> Result { + let mut values = Vec::new(); + while let Some(UniqueValue(value)) = sequence.next_element()? { + values.push(value); + } + Ok(UniqueValue(Value::Array(values))) + } + + fn visit_str(self, value: &str) -> Result { + Ok(UniqueValue(Value::String(value.to_owned()))) + } + + fn visit_bool(self, value: bool) -> Result { + Ok(UniqueValue(Value::Bool(value))) + } + + fn visit_i64(self, value: i64) -> Result { + Ok(UniqueValue(Value::Number(value.into()))) + } + + fn visit_u64(self, value: u64) -> Result { + Ok(UniqueValue(Value::Number(value.into()))) + } + + fn visit_f64(self, value: f64) -> Result { + serde_json::Number::from_f64(value) + .map(|number| UniqueValue(Value::Number(number))) + .ok_or_else(|| serde::de::Error::custom("invalid number")) + } + + fn visit_unit(self) -> Result { + Ok(UniqueValue(Value::Null)) + } + } + deserializer.deserialize_any(Visitor) + } +} + +fn invalid_lock() -> CommandError { + CommandError::operation( + "DOCTOR_INVALID_LOCKFILE", + "The skills.sh lockfile has an unsupported format or invalid Skill record.", + ) +} + +fn stale_lock() -> CommandError { + CommandError::operation( + "DOCTOR_STALE_LOCKFILE", + "The skills.sh lockfile changed. Run doctor again before changing Skills.", + ) +} + +fn unsupported_tree() -> CommandError { + CommandError::operation( + "DOCTOR_UNSUPPORTED_TREE", + "The Skill contains an unsupported file or a link outside its directory.", + ) +} + +fn scan_limit() -> CommandError { + CommandError::operation( + "DOCTOR_SCAN_LIMIT", + "The Skill exceeds the scan limit for depth, files, or bytes.", + ) +} + +fn tree_io(error: std::io::Error) -> CommandError { + CommandError::operation( + "DOCTOR_READ_FAILED", + format!("Cannot read the Skill directory: {error}"), + ) +} + +fn lock_io(error: std::io::Error) -> CommandError { + CommandError::operation( + "DOCTOR_READ_FAILED", + format!("Cannot read the skills.sh lockfile: {error}"), + ) +} diff --git a/crates/skilld-command/src/lib.rs b/crates/skilld-command/src/lib.rs index 2ee159a0..91b8da70 100644 --- a/crates/skilld-command/src/lib.rs +++ b/crates/skilld-command/src/lib.rs @@ -2,6 +2,9 @@ mod account; mod config; mod dependencies; mod discover; +pub mod doctor; +pub mod doctor_actions; +pub mod doctor_metadata; pub use dependencies::{ExternalReference, external_references}; mod local_store; mod outdated; @@ -23,6 +26,7 @@ use std::io::{Read, Write}; use std::path::{Path, PathBuf}; use std::sync::Arc; +use clap::builder::styling::{AnsiColor, Styles}; use clap::{CommandFactory, Parser, Subcommand, error::ErrorKind}; pub use config::{ConfigStore, LocalConfig}; pub use local_store::{ @@ -62,7 +66,7 @@ use output::{ /// An Agent without the skilld Skill reads `--help` first. It names every command /// that answers in JSON; a test keeps it in step with `supports_json`. -const JSON_COMMANDS_HELP: &str = "Agents: add --json to sync, search, run, update --check, view with a registry ref,\nbrowse, trending, tracks, index, curators, account, like, unlike, likes, watch,\nunwatch, watches, changes, stars, collection, and tokens. Read data when _tag is Success.\nOther commands answer in text. Add --plain to them for stable text."; +const JSON_COMMANDS_HELP: &str = "Agents: add --json to doctor, sync, search, run, update --check, view with a registry ref,\nbrowse, trending, tracks, index, curators, account, like, unlike, likes, watch,\nunwatch, watches, changes, stars, collection, and tokens. Read data when _tag is Success.\nOther commands answer in text. Add --plain to them for stable text."; const DIRECT_SOURCE_GUIDANCE: &str = "--direct requires a github:OWNER/REPOSITORY/SKILL_PATH source or a GitHub tree URL. Remove --direct, then run the same command again."; @@ -87,6 +91,11 @@ pub struct Cli { #[derive(Debug, Subcommand)] enum Command { + /// Sweep Skill files and review source ownership, copies, and cleanup actions. + Doctor { + #[command(flatten)] + options: doctor::DoctorOptions, + }, /// Sync declared Skills and their required Skills to Agent targets. Sync { /// Read declarations from this JSON file. @@ -565,6 +574,14 @@ pub struct InstalledSkill { } pub trait Host { + fn doctor( + &self, + _options: &doctor::DoctorOptions, + ) -> Result { + Err(CommandError::unsupported_host( + "Skill discovery is unavailable on this host", + )) + } fn sync(&self, _request: SyncRequest) -> Result { Err(CommandError::unsupported_host( "Skill sync is unavailable on this host", @@ -974,7 +991,21 @@ where }) } +pub fn interactive_doctor_requested( + args: I, +) -> Result, clap::Error> +where + I: IntoIterator, + T: Into + Clone, +{ + Cli::try_parse_from(args).map(|cli| match cli.command { + Command::Doctor { options } if !cli.json && !cli.plain => Some(options), + _ => None, + }) +} + enum CommandOutput { + Doctor(doctor::DoctorReport), Sync(SyncReport), Screen(Screen), /// One public API answer: JSON carries the answer, text carries the screen. @@ -1066,7 +1097,7 @@ where ) } } else if display || matches!(requested_mode, OutputMode::Human { .. }) { - terminal_safe_clap_text(&args, error.kind()).into_bytes() + terminal_safe_clap_text(&args, error.kind(), requested_mode).into_bytes() } else { let message = error .to_string() @@ -1109,9 +1140,32 @@ where } match dispatch(cli.command, host, context.platform()) { + Ok(CommandOutput::Doctor(report)) => { + let bytes = if mode == OutputMode::JsonV1 { + output::render_api( + "doctor", + &serde_json::to_value(&report).expect("serializable report"), + ) + } else { + Ok(doctor::render_plain(&report).into_bytes()) + }; + match bytes { + Ok(bytes) => write_success_with_exit( + &bytes, + mode, + stdout, + stderr, + u8::from(!report.problems.is_empty()), + ), + Err(error) => { + let _ = stderr.write_all(&render_error(&error, mode)); + CommandResult { exit_code: 2 } + } + } + } Ok(CommandOutput::Screen(screen)) => { let bytes = match mode { - OutputMode::Human { color, .. } => screen.render_human(color), + OutputMode::Human { color, width, .. } => screen.render_human_width(color, width), OutputMode::Plain { .. } | OutputMode::JsonV1 => screen.render_plain(), }; write_success(bytes.as_bytes(), mode, stdout, stderr) @@ -1119,7 +1173,9 @@ where Ok(CommandOutput::Sync(report)) => { let screen = Screen::new(report.lines()); let bytes = match mode { - OutputMode::Human { color, .. } => screen.render_human(color).into_bytes(), + OutputMode::Human { color, width, .. } => { + screen.render_human_width(color, width).into_bytes() + } OutputMode::Plain { .. } => screen.render_plain().into_bytes(), OutputMode::JsonV1 => { match output::render_api("sync", &serde_json::json!(report)) { @@ -1137,7 +1193,9 @@ where } Ok(CommandOutput::Api(output)) => { let bytes = match mode { - OutputMode::Human { color, .. } => output.human.render_human(color).into_bytes(), + OutputMode::Human { color, width, .. } => { + output.human.render_human_width(color, width).into_bytes() + } OutputMode::Plain { .. } => output.plain.into_bytes(), OutputMode::JsonV1 => match output::render_api(output.command, &output.data) { Ok(bytes) => bytes, @@ -1153,7 +1211,7 @@ where } Ok(CommandOutput::IncompleteScreen(screen)) => { let bytes = match mode { - OutputMode::Human { color, .. } => screen.render_human(color), + OutputMode::Human { color, width, .. } => screen.render_human_width(color, width), OutputMode::Plain { .. } | OutputMode::JsonV1 => screen.render_plain(), }; write_success_with_exit(bytes.as_bytes(), mode, stdout, stderr, 1) @@ -1264,10 +1322,33 @@ fn v2_command(mut args: Vec) -> V2Command { V2Command::Current(args) } -fn terminal_safe_clap_text(args: &[OsString], expected_kind: ErrorKind) -> String { +fn terminal_safe_clap_text( + args: &[OsString], + expected_kind: ErrorKind, + mode: OutputMode, +) -> String { + let styles = Styles::styled() + .header(AnsiColor::Cyan.on_default().bold()) + .usage(AnsiColor::Cyan.on_default().bold()) + .literal(AnsiColor::Cyan.on_default().bold()) + .error(AnsiColor::Red.on_default().bold()) + .invalid(AnsiColor::Yellow.on_default()) + .valid(AnsiColor::Green.on_default()); + let mut command = Cli::command().styles(styles); + let color = matches!(mode, OutputMode::Human { color: true, .. }); + if let OutputMode::Human { width, .. } = mode { + command = command.term_width(usize::from(width)); + } let safe_args = args.iter().map(terminal_safe_argument); - let text = match Cli::try_parse_from(safe_args) { - Err(error) if error.kind() == expected_kind => error.to_string(), + let text = match command.try_get_matches_from(safe_args) { + Err(error) if error.kind() == expected_kind => { + if color { + // Arguments are escaped before Clap formats them. Only Clap's + // own styles may contain terminal control sequences here. + return error.render().ansi().to_string(); + } + error.to_string() + } _ => "error: invalid command arguments\n\nFor more information, try '--help'.\n".to_owned(), }; let mut safe = String::new(); @@ -1319,7 +1400,8 @@ fn requested_output(args: &[OsString]) -> (bool, bool) { /// Whether one command can answer `--json`. fn supports_json(command: &Command) -> bool { match command { - Command::Sync { .. } + Command::Doctor { .. } + | Command::Sync { .. } | Command::Search { .. } | Command::Run { .. } | Command::Update { check: true, .. } @@ -1448,6 +1530,7 @@ fn dispatch( platform: CommandPlatform, ) -> Result { match command { + Command::Doctor { options } => host.doctor(&options).map(CommandOutput::Doctor), Command::Sync { manifest, check, @@ -1642,7 +1725,16 @@ fn dispatch( Ok(CommandOutput::Run(outcome)) } Command::List { global } => host.list(scope(global)).map(|names| { - CommandOutput::Screen(Screen::new(names.into_iter().map(Line::item).collect())) + let label = if global { "global" } else { "project" }; + CommandOutput::Screen( + Screen::with_header( + format!("Installed Skills · {label} · {}", names.len()), + names.into_iter().map(Line::item).collect(), + ) + .with_empty_hint( + "No Skills installed here. Find Skills with skilld search .", + ), + ) }), Command::View { skill, global } => match discover::registry_ref(&skill)? { None => render_view(host.view(&skill, scope(global))?) @@ -3165,6 +3257,12 @@ impl LocalHost { } impl Host for LocalHost { + fn doctor( + &self, + options: &doctor::DoctorOptions, + ) -> Result { + self.doctor_scan(options) + } fn sync(&self, request: SyncRequest) -> Result { sync::sync(self, request) } diff --git a/crates/skilld-command/src/output.rs b/crates/skilld-command/src/output.rs index 8c36204f..c7ab0a18 100644 --- a/crates/skilld-command/src/output.rs +++ b/crates/skilld-command/src/output.rs @@ -237,7 +237,11 @@ pub(crate) fn render_error(error: &CommandError, mode: OutputMode) -> Vec { .unwrap_or_else(|_| b"OUTPUT_RENDER_FAILED: error output could not be encoded\n".to_vec()); } match mode { - OutputMode::Human { color, .. } => { + OutputMode::Human { + color, + width: columns, + .. + } => { let message = sanitize(&error.message); let code = sanitize(error.code); let mut out = format!( @@ -246,8 +250,28 @@ pub(crate) fn render_error(error: &CommandError, mode: OutputMode) -> Vec { skilld_ui::paint(&message, skilld_ui::Role::Emphasis, color), skilld_ui::paint(&format!("({code})"), skilld_ui::Role::Dim, color), ); + if width(&message) + width(&code) + 5 > usize::from(columns) { + out.clear(); + for (index, line) in wrap(&message, usize::from(columns).saturating_sub(2)) + .iter() + .enumerate() + { + let prefix = if index == 0 { + paint("✗ ", Role::Error, color) + } else { + " ".to_owned() + }; + out.push_str(&format!("{prefix}{}\n", paint(line, Role::Emphasis, color))); + } + for line in wrap(&format!("({code})"), usize::from(columns).saturating_sub(2)) { + out.push_str(&format!(" {}\n", paint(&line, Role::Dim, color))); + } + } if let Some(next_step) = &error.next_step { - out.push_str(&format!(" Next step: {}\n", sanitize(next_step))); + out.push_str(&format!("\n {}\n", paint("Next step", Role::Brand, color))); + for line in wrap(&sanitize(next_step), usize::from(columns).saturating_sub(2)) { + out.push_str(&format!(" {line}\n")); + } } out.into_bytes() } @@ -308,16 +332,21 @@ fn render_human( output.push('\n'); } let shown = outcome.items.len(); - output.push_str(&format!( - "{} of {} {}\n", - shown, - outcome.total, - if outcome.total == 1 { - "Skill" - } else { - "Skills" - } + output.push_str(&paint( + &format!( + "{} of {} {}", + shown, + outcome.total, + if outcome.total == 1 { + "Skill" + } else { + "Skills" + } + ), + Role::Dim, + color, )); + output.push('\n'); if outcome.items.is_empty() { output.push('\n'); @@ -343,7 +372,7 @@ fn render_human( let meta = format!( "{} · {}", paint(&slug, Role::Dim, color), - paint(&stars, Role::Warn, color) + paint(&stars, Role::Accent, color) ); let meta_width = width(&slug) + 3 + width(&stars); if 2 + width(&name) + 2 + meta_width <= columns { @@ -486,8 +515,8 @@ pub(crate) fn render_run(outcome: &RunOutcome, mode: OutputMode) -> Result { Ok(render_index_plain(listing).into_bytes()) } - (RunOutcome::Index(listing), OutputMode::Human { color, .. }) => { - Ok(render_index(listing, color).into_bytes()) + (RunOutcome::Index(listing), OutputMode::Human { color, width, .. }) => { + Ok(render_index(listing, color, width).into_bytes()) } (RunOutcome::Load(skill), _) => { Ok(render_load(skill, colored(mode), command_platform(mode)).into_bytes()) @@ -525,22 +554,29 @@ const fn command_platform(mode: OutputMode) -> CommandPlatform { } /// One line per Skill: the run command, then the description. -fn render_index(listing: &SkillListing, color: bool) -> String { +fn render_index(listing: &SkillListing, color: bool, columns: u16) -> String { let reference = sanitize(&listing.reference.canonical()); let count = listing.items.len(); let noun = if count == 1 { "Skill" } else { "Skills" }; let mut out = String::new(); out.push_str(&paint( &format!("{reference} names {count} {noun}. skilld loaded none of them."), - Role::Emphasis, + Role::Brand, color, )); out.push_str("\n\n"); for item in &listing.items { - out.push_str(&paint(&run_command(item), Role::Emphasis, color)); + out.push_str(&skilld_ui::paint_command(&run_command(item), color)); + out.push('\n'); if let Some(description) = &item.description { - out.push_str(" "); - out.push_str(&paint(&sanitize(description), Role::Dim, color)); + for line in wrap( + &sanitize(description), + usize::from(columns).saturating_sub(2), + ) { + out.push_str(" "); + out.push_str(&paint(&line, Role::Dim, color)); + out.push('\n'); + } } out.push('\n'); } @@ -548,11 +584,7 @@ fn render_index(listing: &SkillListing, color: bool) -> String { out.push_str("Run one command above to load that Skill for this session.\n"); out.push_str(&format!( "Install every Skill listed here with {}.\n", - paint( - &format!("npx skilld add {reference}"), - Role::Emphasis, - color - ) + paint(&format!("npx skilld add {reference}"), Role::Brand, color) )); out } @@ -683,14 +715,14 @@ fn render_load(skill: &TransientSkill, color: bool, platform: CommandPlatform) - } out.push('\n'); - out.push_str(&paint("--- SKILL.md ---", Role::Dim, color)); + out.push_str(&paint("--- SKILL.md ---", Role::Brand, color)); out.push('\n'); let instructions = safe_terminal_text(&skill.instructions); out.push_str(&instructions); if !instructions.ends_with('\n') { out.push('\n'); } - out.push_str(&paint("--- end of SKILL.md ---", Role::Dim, color)); + out.push_str(&paint("--- end of SKILL.md ---", Role::Brand, color)); out.push('\n'); out.push('\n'); diff --git a/crates/skilld-command/tests/doctor.rs b/crates/skilld-command/tests/doctor.rs new file mode 100644 index 00000000..37808c4d --- /dev/null +++ b/crates/skilld-command/tests/doctor.rs @@ -0,0 +1,648 @@ +use skilld_command::ResolvedTarget; +use skilld_command::doctor::{DoctorOptions, ScanContext, ScanPhase, scan, scan_with_progress}; +use skilld_core::AgentTargetId; +use std::{fs, path::Path}; + +fn skill(root: &Path, path: &str, text: &str) { + let path = root.join(path); + fs::create_dir_all(&path).unwrap(); + fs::write( + path.join("SKILL.md"), + format!("---\nname: example\ndescription: Test.\n---\n{text}"), + ) + .unwrap(); +} + +fn context(home: &Path) -> ScanContext { + ScanContext { + home: home.to_path_buf(), + global_store: home.join(".skilld/skills"), + global_targets: vec![ + ResolvedTarget::new(AgentTargetId::ClaudeCode, home.join(".claude/skills")).unwrap(), + ResolvedTarget::new(AgentTargetId::Codex, home.join(".agents/skills")).unwrap(), + ], + skills_sh_global_lock: home.join(".agents/.skill-lock.json"), + } +} + +#[test] +fn sweep_keeps_hidden_claude_skills_and_prunes_dependency_and_session_trees() { + let home = tempfile::tempdir().unwrap(); + for path in [ + "app/.claude/skills/example", + ".claude/skills/global", + "app/node_modules/pkg/skills/dependency", + ".codex/.tmp/plugins/staged", + ".claude/projects/session/skills/old", + ".config/Code/User/History/old/skills/cached", + "app/.intellijPlatform/sandbox/skills/generated", + "app/.data/kv-dump/skills/generated", + "media/movie.trickplay/skills/generated", + ".gemini/history/session/skills/old", + "google-cloud-sdk/lib/skills/bundled", + ] { + skill(home.path(), path, "Original"); + } + fs::write(home.path().join("app/CLAUDE.md"), "Project instructions").unwrap(); + let report = scan(&DoctorOptions::for_root(home.path()), &context(home.path())).unwrap(); + assert_eq!(report.skills.len(), 2); + assert!(report.skills.iter().any(|s| { + s.paths + .iter() + .any(|p| p.path == home.path().join("app/.claude/skills/example")) + })); + assert_eq!(report.claude_files, vec![home.path().join("app/CLAUDE.md")]); + assert!(report.skipped_directories >= 3); +} + +#[test] +fn exclusions_prune_before_reading_and_invalid_globs_fail() { + let home = tempfile::tempdir().unwrap(); + skill(home.path(), "app/.claude/skills/keep", "Keep"); + skill(home.path(), "archive/app/.claude/skills/skip", "Skip"); + let mut options = DoctorOptions::for_root(home.path()); + options.exclude.push("**/archive/**".into()); + let report = scan(&options, &context(home.path())).unwrap(); + assert_eq!(report.skills.len(), 1); + options.exclude.push("[".into()); + assert!(scan(&options, &context(home.path())).is_err()); +} + +#[test] +fn duplicates_compare_supporting_files_not_only_instructions() { + let home = tempfile::tempdir().unwrap(); + for name in ["one", "two", "three"] { + skill(home.path(), &format!("app/.claude/skills/{name}"), "Same"); + } + fs::write( + home.path().join("app/.claude/skills/three/reference.md"), + "Different", + ) + .unwrap(); + let report = scan(&DoctorOptions::for_root(home.path()), &context(home.path())).unwrap(); + let one = report.skills.iter().find(|s| s.name == "one").unwrap(); + assert_eq!(one.duplicates.len(), 1); + let three = report.skills.iter().find(|s| s.name == "three").unwrap(); + assert!(three.duplicates.is_empty()); +} + +#[cfg(unix)] +#[test] +fn links_share_one_skill_and_broken_links_are_reported_without_following_loops() { + use std::os::unix::fs::symlink; + let home = tempfile::tempdir().unwrap(); + skill(home.path(), ".agents/skills/example", "Same"); + fs::create_dir_all(home.path().join(".claude/skills")).unwrap(); + symlink( + home.path().join(".agents/skills/example"), + home.path().join(".claude/skills/example"), + ) + .unwrap(); + symlink( + home.path().join("missing"), + home.path().join(".claude/skills/broken"), + ) + .unwrap(); + symlink(home.path(), home.path().join("loop")).unwrap(); + let report = scan(&DoctorOptions::for_root(home.path()), &context(home.path())).unwrap(); + assert_eq!(report.skills.len(), 1); + assert_eq!(report.skills[0].paths.len(), 2); + assert!(report.problems.iter().any(|p| p.path.ends_with("broken"))); +} + +#[cfg(unix)] +#[test] +fn excluded_skill_links_are_not_read_and_explicit_target_roots_keep_their_scope() { + use std::os::unix::fs::symlink; + let home = tempfile::tempdir().unwrap(); + skill(home.path(), "source/skip", "Same"); + fs::create_dir_all(home.path().join("app/.claude")).unwrap(); + symlink( + home.path().join("source"), + home.path().join("app/.claude/skills"), + ) + .unwrap(); + let mut options = DoctorOptions::for_root(&home.path().join("app/.claude/skills")); + let report = scan(&options, &context(home.path())).unwrap(); + assert_eq!( + report.skills[0].paths[0].agent, + Some(AgentTargetId::ClaudeCode) + ); + options.exclude.push("**/skip/**".into()); + assert!( + scan(&options, &context(home.path())) + .unwrap() + .skills + .is_empty() + ); +} + +#[cfg(unix)] +#[test] +fn default_scan_stays_inside_home_even_with_external_agent_targets() { + let home = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + skill(home.path(), "app/.claude/skills/local", "Local"); + skill(outside.path(), "skills/external", "External"); + fs::create_dir_all(home.path().join(".claude/skills")).unwrap(); + std::os::unix::fs::symlink( + outside.path().join("skills/external"), + home.path().join(".claude/skills/external"), + ) + .unwrap(); + let mut ctx = context(home.path()); + ctx.global_targets + .push(ResolvedTarget::new(AgentTargetId::Cursor, outside.path().join("skills")).unwrap()); + let mut options = DoctorOptions::for_root(home.path()); + options.roots.clear(); + let report = scan(&options, &ctx).unwrap(); + assert_eq!( + report + .skills + .iter() + .map(|s| s.name.as_str()) + .collect::>(), + vec!["local"] + ); + assert_eq!(report.roots, vec![home.path().to_path_buf()]); + assert!( + report + .problems + .iter() + .any(|p| p.message.contains("outside")) + ); + options.roots.push(outside.path().to_path_buf()); + let explicit = scan(&options, &ctx).unwrap(); + assert_eq!(explicit.skills[0].name, "external"); +} + +#[test] +fn progress_reports_discovery_then_actual_content_totals() { + let home = tempfile::tempdir().unwrap(); + skill(home.path(), "app/.claude/skills/example", "Local"); + let mut events = vec![]; + let report = scan_with_progress( + &DoctorOptions::for_root(home.path()), + &context(home.path()), + &mut |p| events.push(p), + ) + .unwrap(); + assert_eq!(events[0].phase, ScanPhase::Discover); + assert!(events.iter().any(|p| p.phase + == ScanPhase::Fingerprint { + completed: 0, + total: 1 + })); + let last = events.last().unwrap(); + assert_eq!( + last.phase, + ScanPhase::Fingerprint { + completed: 1, + total: 1 + } + ); + assert_eq!(last.found_skills, report.skills.len()); + assert_eq!(last.visited_directories, report.visited_directories); +} + +#[cfg(unix)] +#[test] +fn home_scan_reports_external_metadata_without_classifying_the_skill_as_unmanaged() { + let home = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + skill(home.path(), ".claude/skills/example", "Local"); + let mut ctx = context(home.path()); + ctx.global_store = outside.path().join("store"); + fs::create_dir_all(&ctx.global_store).unwrap(); + fs::write( + ctx.global_store.join("skilld-lock.yaml"), + "invalid external metadata", + ) + .unwrap(); + let mut options = DoctorOptions::for_root(home.path()); + options.roots.clear(); + let report = scan(&options, &ctx).unwrap(); + assert!(matches!( + report.skills[0].owner, + skilld_command::doctor::DoctorOwner::Unavailable { .. } + )); + assert!( + report + .problems + .iter() + .any(|p| p.message.contains("outside the home directory")) + ); +} + +#[cfg(unix)] +#[test] +fn home_scan_checks_children_of_linked_agent_roots_before_inspection() { + let home = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + skill(outside.path(), "external", "External"); + skill(home.path(), "targets/local", "Local"); + fs::create_dir_all(home.path().join(".claude")).unwrap(); + std::os::unix::fs::symlink( + home.path().join("targets"), + home.path().join(".claude/skills"), + ) + .unwrap(); + std::os::unix::fs::symlink( + outside.path().join("external"), + home.path().join("targets/external"), + ) + .unwrap(); + let mut options = DoctorOptions::for_root(home.path()); + options.roots.clear(); + let report = scan(&options, &context(home.path())).unwrap(); + assert_eq!(report.skills.len(), 1); + assert_eq!(report.skills[0].name, "local"); + assert!( + report + .problems + .iter() + .any(|p| p.path.ends_with(".claude/skills/external") && p.message.contains("outside")) + ); +} + +fn linked_worktree(home: &Path, path: &str) { + let root = home.join(path); + let admin = home + .join("main/.git/worktrees") + .join(path.replace('/', "-")); + fs::create_dir_all(&root).unwrap(); + fs::create_dir_all(&admin).unwrap(); + fs::write(admin.join("commondir"), "../..\n").unwrap(); + fs::write(root.join(".git"), format!("gitdir: {}\n", admin.display())).unwrap(); +} + +#[test] +fn worktree_roots_and_their_children_need_explicit_opt_in() { + let home = tempfile::tempdir().unwrap(); + linked_worktree(home.path(), "feature"); + skill(home.path(), "feature/.claude/skills/feature", "Feature"); + skill(home.path(), "main/.claude/skills/main", "Main"); + for root in [ + home.path().to_path_buf(), + home.path().join("feature"), + home.path().join("feature/.claude/skills"), + ] { + let mut options = DoctorOptions::for_root(&root); + let report = scan(&options, &context(home.path())).unwrap(); + assert!(report.skills.iter().all(|skill| skill.name != "feature")); + assert!(report.skipped_directories > 0); + options.include_worktrees = true; + let report = scan(&options, &context(home.path())).unwrap(); + assert!(report.skills.iter().any(|skill| skill.name == "feature")); + } + let report = scan(&DoctorOptions::for_root(home.path()), &context(home.path())).unwrap(); + assert!(report.skills.iter().any(|skill| skill.name == "main")); +} + +#[test] +fn ordinary_git_files_do_not_hide_submodules_or_separate_git_directories() { + let home = tempfile::tempdir().unwrap(); + for (root, admin) in [ + ("submodule", "main/.git/modules/submodule"), + ("separate", "metadata/separate"), + ] { + skill(home.path(), &format!("{root}/.claude/skills/{root}"), root); + fs::create_dir_all(home.path().join(admin)).unwrap(); + fs::write( + home.path().join(root).join(".git"), + format!("gitdir: {}\n", home.path().join(admin).display()), + ) + .unwrap(); + } + let report = scan(&DoctorOptions::for_root(home.path()), &context(home.path())).unwrap(); + assert_eq!( + report + .skills + .iter() + .map(|skill| skill.name.as_str()) + .collect::>(), + ["separate", "submodule"] + ); +} + +#[test] +fn named_worktree_containers_follow_the_worktree_flag() { + let home = tempfile::tempdir().unwrap(); + skill( + home.path(), + ".claude/worktrees/feature/.claude/skills/claude", + "Claude", + ); + skill( + home.path(), + "app/.worktrees/feature/.claude/skills/hidden", + "Hidden", + ); + for include_excluded in [false, true] { + let mut options = DoctorOptions::for_root(home.path()); + options.include_excluded = include_excluded; + assert!( + scan(&options, &context(home.path())) + .unwrap() + .skills + .is_empty() + ); + options.include_worktrees = true; + assert_eq!( + scan(&options, &context(home.path())).unwrap().skills.len(), + 2 + ); + } +} + +#[cfg(unix)] +#[test] +fn explicit_worktree_aliases_and_agent_links_cannot_bypass_exclusion() { + let home = tempfile::tempdir().unwrap(); + linked_worktree(home.path(), "feature"); + skill(home.path(), "feature/.claude/skills/feature", "Feature"); + std::os::unix::fs::symlink( + home.path().join("feature/.claude/skills"), + home.path().join("alias"), + ) + .unwrap(); + fs::create_dir_all(home.path().join(".claude/skills")).unwrap(); + std::os::unix::fs::symlink( + home.path().join("feature/.claude/skills/feature"), + home.path().join(".claude/skills/feature"), + ) + .unwrap(); + for root in [home.path().to_path_buf(), home.path().join("alias")] { + let mut options = DoctorOptions::for_root(&root); + assert!( + scan(&options, &context(home.path())) + .unwrap() + .skills + .is_empty() + ); + options.include_worktrees = true; + assert_eq!( + scan(&options, &context(home.path())).unwrap().skills.len(), + 1 + ); + } +} + +#[cfg(unix)] +#[test] +fn link_observations_keep_the_raw_destination_and_agent_scope() { + let home = tempfile::tempdir().unwrap(); + skill(home.path(), "source/example", "Source"); + fs::create_dir_all(home.path().join(".claude/skills")).unwrap(); + std::os::unix::fs::symlink( + "../../source/example", + home.path().join(".claude/skills/example"), + ) + .unwrap(); + let report = scan(&DoctorOptions::for_root(home.path()), &context(home.path())).unwrap(); + let skill = &report.skills[0]; + let link = skill + .paths + .iter() + .find(|location| location.agent == Some(AgentTargetId::ClaudeCode)) + .unwrap(); + assert_eq!( + link.symlink_target.as_deref(), + Some(Path::new("../../source/example")) + ); + assert_eq!(link.project_root, None); + let directory = skill + .paths + .iter() + .find(|location| location.agent.is_none()) + .unwrap(); + assert_eq!(directory.symlink_target, None); + let json = serde_json::to_value(link).unwrap(); + assert_eq!(json["symlinkTarget"], "../../source/example"); +} + +#[cfg(unix)] +#[test] +fn a_linked_agent_root_does_not_turn_ordinary_children_into_symlinks() { + let home = tempfile::tempdir().unwrap(); + skill(home.path(), "source/example", "Source"); + fs::create_dir_all(home.path().join("app/.claude")).unwrap(); + std::os::unix::fs::symlink( + home.path().join("source"), + home.path().join("app/.claude/skills"), + ) + .unwrap(); + let report = scan(&DoctorOptions::for_root(home.path()), &context(home.path())).unwrap(); + let location = report.skills[0] + .paths + .iter() + .find(|location| location.agent == Some(AgentTargetId::ClaudeCode)) + .unwrap(); + assert_eq!(location.symlink_target, None); + assert_eq!( + location.project_root.as_deref(), + Some(home.path().join("app").as_path()) + ); + assert_eq!( + location.path, + home.path().join("app/.claude/skills/example") + ); +} + +#[test] +fn a_relative_git_admin_path_still_identifies_a_linked_worktree() { + let home = tempfile::tempdir().unwrap(); + linked_worktree(home.path(), "feature"); + fs::write( + home.path().join("feature/.git"), + "gitdir: ../main/.git/worktrees/feature\n", + ) + .unwrap(); + skill(home.path(), "feature/.claude/skills/feature", "Feature"); + let options = DoctorOptions::for_root(&home.path().join("feature")); + assert!( + scan(&options, &context(home.path())) + .unwrap() + .skills + .is_empty() + ); +} + +#[cfg(unix)] +#[test] +fn broken_links_under_linked_roots_are_reported_even_when_worktrees_are_included() { + let home = tempfile::tempdir().unwrap(); + fs::create_dir_all(home.path().join("source")).unwrap(); + fs::create_dir_all(home.path().join(".claude")).unwrap(); + std::os::unix::fs::symlink( + home.path().join("source"), + home.path().join(".claude/skills"), + ) + .unwrap(); + std::os::unix::fs::symlink("missing", home.path().join("source/broken")).unwrap(); + for include_worktrees in [false, true] { + let mut options = DoctorOptions::for_root(home.path()); + options.include_worktrees = include_worktrees; + let report = scan(&options, &context(home.path())).unwrap(); + assert!( + report + .problems + .iter() + .any(|p| p.path == home.path().join(".claude/skills/broken") + && p.message.contains("Broken Agent target")) + ); + } +} + +#[test] +fn local_app_data_is_pruned_but_configured_skill_roots_are_scanned() { + let home = tempfile::tempdir().unwrap(); + skill(home.path(), ".local/share/cache/noise", "Cache"); + skill( + home.path(), + ".local/share/worker-home/.agents/skills/worker", + "Worker", + ); + skill(home.path(), ".local/share/skilld/skills/managed", "Managed"); + skill( + home.path(), + ".local/share/custom-agent/skills/installed", + "Installed", + ); + skill( + home.path(), + ".local/share/custom-agent/skills/node_modules/ignored", + "Dependency", + ); + let mut ctx = context(home.path()); + ctx.global_store = home.path().join(".local/share/skilld/skills"); + ctx.global_targets.push( + ResolvedTarget::new( + AgentTargetId::Cursor, + home.path().join(".local/share/custom-agent/skills"), + ) + .unwrap(), + ); + let mut options = DoctorOptions::for_root(home.path()); + options.roots.clear(); + let report = scan(&options, &ctx).unwrap(); + let mut names: Vec<_> = report.skills.iter().map(|s| s.name.as_str()).collect(); + names.sort_unstable(); + assert_eq!(names, vec!["installed", "managed"]); + assert!( + report + .skills + .iter() + .find(|s| s.name == "installed") + .unwrap() + .paths + .iter() + .any(|p| p.agent == Some(AgentTargetId::Cursor)) + ); + options.exclude.push("**/custom-agent/**".into()); + let excluded = scan(&options, &ctx).unwrap(); + assert_eq!( + excluded + .skills + .iter() + .map(|s| s.name.as_str()) + .collect::>(), + vec!["managed"] + ); + options.exclude.clear(); + options.exclude.push( + home.path() + .join(".local/share/custom-agent") + .display() + .to_string() + .replace('\\', "/"), + ); + let excluded_parent = scan(&options, &ctx).unwrap(); + assert_eq!( + excluded_parent + .skills + .iter() + .map(|s| s.name.as_str()) + .collect::>(), + vec!["managed"] + ); + options.exclude.clear(); + options.include_excluded = true; + let all = scan(&options, &ctx).unwrap(); + for name in ["noise", "worker", "managed", "installed", "ignored"] { + assert!(all.skills.iter().any(|s| s.name == name), "{name}"); + } + skill(home.path(), "project/.claude/skills/project", "Project"); + options = DoctorOptions::for_root(&home.path().join("project")); + let narrow = scan(&options, &ctx).unwrap(); + assert_eq!( + narrow + .skills + .iter() + .map(|s| s.name.as_str()) + .collect::>(), + vec!["project"] + ); +} + +#[cfg(unix)] +#[test] +fn skill_links_keep_local_referents_without_sweeping_their_neighbors() { + let home = tempfile::tempdir().unwrap(); + skill(home.path(), ".local/share/bundle/active", "Active"); + skill(home.path(), ".local/share/bundle/unused", "Unused"); + fs::create_dir_all(home.path().join(".claude/skills")).unwrap(); + std::os::unix::fs::symlink( + "../../.local/share/bundle/active", + home.path().join(".claude/skills/active"), + ) + .unwrap(); + let report = scan(&DoctorOptions::for_root(home.path()), &context(home.path())).unwrap(); + assert_eq!(report.skills.len(), 1); + assert_eq!(report.skills[0].name, "active"); + assert_eq!( + report.skills[0].canonical_path, + home.path().join(".local/share/bundle/active") + ); + assert!(report.skills[0].paths[0].is_linked()); +} + +#[cfg(unix)] +#[test] +fn configured_local_root_cannot_traverse_outside_home() { + let home = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + skill(outside.path(), "external", "Outside"); + fs::create_dir_all(home.path().join(".local/share")).unwrap(); + let linked = home.path().join(".local/share/agent"); + std::os::unix::fs::symlink(outside.path(), &linked).unwrap(); + let mut ctx = context(home.path()); + ctx.global_targets + .push(ResolvedTarget::new(AgentTargetId::Cursor, linked).unwrap()); + let mut options = DoctorOptions::for_root(home.path()); + options.roots.clear(); + let report = scan(&options, &ctx).unwrap(); + assert!(report.skills.is_empty()); + assert!( + report + .problems + .iter() + .any(|p| p.message.contains("outside the home directory")) + ); +} + +#[test] +fn unreadable_git_metadata_is_reported_before_reading_skill_contents() { + let home = tempfile::tempdir().unwrap(); + skill(home.path(), "feature/.claude/skills/feature", "Feature"); + fs::write(home.path().join("feature/.git"), [0xff]).unwrap(); + let report = scan(&DoctorOptions::for_root(home.path()), &context(home.path())).unwrap(); + assert!(report.skills.is_empty()); + assert!( + report + .problems + .iter() + .any(|problem| problem.path.ends_with("feature") + && problem.message.contains("Git worktree metadata")) + ); +} diff --git a/crates/skilld-command/tests/doctor_actions.rs b/crates/skilld-command/tests/doctor_actions.rs new file mode 100644 index 00000000..ffaefa4d --- /dev/null +++ b/crates/skilld-command/tests/doctor_actions.rs @@ -0,0 +1,626 @@ +use serde_json::json; +use sha2::{Digest, Sha256}; +use skilld_command::doctor::DoctorOptions; +use skilld_command::doctor_actions::DoctorAction; +use skilld_command::doctor_metadata::fingerprint; +use skilld_command::{ + Host, LocalHost, PreparedRemoteSkill, RemoteLatestCommit, RemoteProvider, RemoteSourceState, + RemoteUpdateComparison, RemoteUpdateResult, +}; +use skilld_core::{ + CommitSha, LockedSource, PreparedFile, RemoteError, RemoteSelector, SearchResponse, + SourceStatus, +}; +use std::{fs, path::Path, sync::Arc}; + +struct Remote { + text: String, +} +impl RemoteProvider for Remote { + fn search(&self, _: &str, _: u8) -> Result { + unreachable!() + } + fn prepare( + &self, + selector: &RemoteSelector, + direct: bool, + ) -> Result { + assert!(!direct); + let file = PreparedFile { + path: "SKILL.md".into(), + mode: 0o644, + bytes: self.text.as_bytes().to_vec(), + }; + let mut digest = Sha256::new(); + digest.update((file.path.len() as u64).to_be_bytes()); + digest.update(file.path.as_bytes()); + digest.update((file.bytes.len() as u64).to_be_bytes()); + digest.update(&file.bytes); + Ok(PreparedRemoteSkill { + files: vec![file], + locked_source: LockedSource::Remote { + source: selector.canonical(), + commit_sha: "1".repeat(40), + skill_path: if selector.canonical().ends_with("/.") { + "." + } else { + "skills/example" + } + .into(), + }, + source_status: SourceStatus::Verified { + artifact_id: format!("sha256:{}", "a".repeat(64)), + content_sha256: "a".repeat(64), + installed_sha256: digest + .finalize() + .iter() + .map(|b| format!("{b:02x}")) + .collect(), + attestation_key_id: "fixture".into(), + }, + page_url: None, + omitted_files: vec![], + behavior_readings: vec![], + }) + } + fn prepare_exact( + &self, + _: &RemoteSelector, + _: &CommitSha, + _: bool, + ) -> Result { + unreachable!() + } + fn source_state( + &self, + _: &RemoteSelector, + _: &str, + _: &str, + ) -> Result { + unreachable!() + } + fn latest_commit( + &self, + _: &RemoteSelector, + _: bool, + ) -> Result { + unreachable!() + } + fn compare_updates( + &self, + _: &[RemoteUpdateComparison], + ) -> Result, RemoteError> { + unreachable!() + } +} +const TEXT: &str = "---\nname: example\ndescription: Test.\n---\nRead the project.\n"; +fn fixture() -> (tempfile::TempDir, LocalHost) { + let root = tempfile::tempdir().unwrap(); + fs::create_dir_all(root.path().join("app")).unwrap(); + fs::create_dir_all(root.path().join(".agents/skills/example")).unwrap(); + fs::write(root.path().join(".agents/skills/example/SKILL.md"), TEXT).unwrap(); + let tree = fingerprint(&root.path().join(".agents/skills/example")) + .unwrap() + .git_tree; + fs::write(root.path().join(".agents/.skill-lock.json"),json!({"version":3,"dismissed":{"notice":true},"skills":{"example":{"source":"owner/repository","sourceType":"github","skillPath":"skills/example/SKILL.md","skillFolderHash":tree},"unrelated":{"source":"owner/other","sourceType":"github","skillFolderHash":""}}}).to_string()).unwrap(); + let host = LocalHost::new(root.path().join("app"), root.path().join(".skilld")) + .with_remote_provider(Arc::new(Remote { text: TEXT.into() })); + (root, host) +} +fn lock(root: &Path) -> serde_json::Value { + serde_json::from_slice(&fs::read(root.join(".agents/.skill-lock.json")).unwrap()).unwrap() +} + +#[test] +fn removal_backs_up_files_and_preserves_unrelated_foreign_metadata() { + let (root, host) = fixture(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + let done = host + .doctor_plan(&report, 0, DoctorAction::Remove) + .unwrap() + .apply() + .unwrap(); + assert!(!root.path().join(".agents/skills/example").exists()); + assert_eq!( + fs::read_to_string(done.backup.join("target-0/SKILL.md")).unwrap(), + TEXT + ); + let value = lock(root.path()); + assert!(value["skills"].get("example").is_none()); + assert!(value["skills"].get("unrelated").is_some()); + assert_eq!(value["dismissed"]["notice"], true); +} + +#[test] +fn changed_supporting_file_rejects_a_reviewed_removal() { + let (root, host) = fixture(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + let plan = host.doctor_plan(&report, 0, DoctorAction::Remove).unwrap(); + fs::write( + root.path().join(".agents/skills/example/new.md"), + "Keep this edit", + ) + .unwrap(); + assert!(plan.apply().is_err()); + assert!(lock(root.path())["skills"].get("example").is_some()); + assert!(root.path().join(".agents/skills/example/new.md").is_file()); +} + +#[test] +fn concurrent_lock_edit_rejects_a_reviewed_removal() { + let (root, host) = fixture(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + let plan = host.doctor_plan(&report, 0, DoctorAction::Remove).unwrap(); + let mut value = lock(root.path()); + value["newField"] = json!("keep"); + fs::write( + root.path().join(".agents/.skill-lock.json"), + value.to_string(), + ) + .unwrap(); + assert!(plan.apply().is_err()); + assert!( + root.path() + .join(".agents/skills/example/SKILL.md") + .is_file() + ); + assert_eq!(lock(root.path())["newField"], "keep"); +} + +#[cfg(unix)] +#[test] +fn migration_preserves_claude_link_and_canonical_copy_then_transfers_ownership() { + let (root, host) = fixture(); + fs::create_dir_all(root.path().join(".claude/skills")).unwrap(); + std::os::unix::fs::symlink( + root.path().join(".agents/skills/example"), + root.path().join(".claude/skills/example"), + ) + .unwrap(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + let plan = host.doctor_plan(&report, 0, DoctorAction::Migrate).unwrap(); + assert!(!plan.preview.changes_content); + let done = plan.apply().unwrap(); + assert_eq!( + fs::read_to_string(root.path().join(".claude/skills/example/SKILL.md")).unwrap(), + TEXT + ); + assert!( + fs::symlink_metadata(root.path().join(".claude/skills/example")) + .unwrap() + .file_type() + .is_symlink() + ); + assert!( + !fs::symlink_metadata(root.path().join(".agents/skills/example")) + .unwrap() + .file_type() + .is_symlink() + ); + let managed: serde_json::Value = serde_json::from_slice( + &fs::read(root.path().join(".skilld/skills/skilld-lock.yaml")).unwrap(), + ) + .unwrap(); + assert_eq!( + managed["skills"]["example"]["source"]["commit_sha"], + "1".repeat(40) + ); + assert_eq!( + managed["skills"]["example"]["targets"] + .as_array() + .unwrap() + .len(), + 2 + ); + assert!(lock(root.path())["skills"].get("example").is_none()); + assert!(done.backup.join("skills-lock.json").is_file()); +} + +#[test] +fn migration_does_not_overwrite_unverified_content() { + let (root, host) = fixture(); + fs::write( + root.path().join(".agents/skills/example/local.md"), + "Local edit", + ) + .unwrap(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + assert!(host.doctor_plan(&report, 0, DoctorAction::Migrate).is_err()); + assert!( + root.path() + .join(".agents/skills/example/local.md") + .is_file() + ); +} + +#[test] +fn installation_failure_restores_all_foreign_files() { + let (root, host) = fixture(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + let plan = host.doctor_plan(&report, 0, DoctorAction::Migrate).unwrap(); + fs::create_dir_all(root.path().join(".skilld/skills")).unwrap(); + fs::write( + root.path().join(".skilld/skills/skilld-lock.yaml"), + "invalid concurrent state", + ) + .unwrap(); + assert!(plan.apply().is_err()); + assert_eq!( + fs::read_to_string(root.path().join(".agents/skills/example/SKILL.md")).unwrap(), + TEXT + ); + assert!(lock(root.path())["skills"].get("example").is_some()); +} + +#[cfg(unix)] +#[test] +fn differently_named_alias_never_removes_an_unrelated_target() { + let (root, host) = fixture(); + fs::remove_file(root.path().join(".agents/.skill-lock.json")).unwrap(); + fs::create_dir_all(root.path().join(".claude/skills/example")).unwrap(); + fs::write( + root.path().join(".claude/skills/example/SKILL.md"), + "Unrelated personal Skill", + ) + .unwrap(); + std::os::unix::fs::symlink( + root.path().join(".agents/skills/example"), + root.path().join(".claude/skills/alias"), + ) + .unwrap(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + let index = report + .skills + .iter() + .position(|s| s.canonical_path == root.path().join(".agents/skills/example")) + .unwrap(); + let result = host.doctor_plan(&report, index, DoctorAction::Remove); + assert!(result.is_err()); + assert_eq!( + fs::read_to_string(root.path().join(".claude/skills/example/SKILL.md")).unwrap(), + "Unrelated personal Skill" + ); +} + +#[cfg(unix)] +#[test] +fn retargeted_agent_root_rejects_stale_physical_paths() { + use std::os::unix::fs::symlink; + let (root, host) = fixture(); + for dir in ["old", "new"] { + fs::create_dir_all(root.path().join(dir)).unwrap(); + symlink( + root.path().join(".agents/skills/example"), + root.path().join(dir).join("example"), + ) + .unwrap(); + } + fs::create_dir_all(root.path().join(".claude")).unwrap(); + symlink(root.path().join("old"), root.path().join(".claude/skills")).unwrap(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + let plan = host.doctor_plan(&report, 0, DoctorAction::Remove).unwrap(); + fs::remove_file(root.path().join(".claude/skills")).unwrap(); + symlink(root.path().join("new"), root.path().join(".claude/skills")).unwrap(); + fs::remove_file(root.path().join("old/example")).unwrap(); + fs::create_dir(root.path().join("old/example")).unwrap(); + fs::write(root.path().join("old/example/keep.md"), "Unrelated").unwrap(); + assert!(plan.apply().is_err()); + assert!(root.path().join("old/example/keep.md").is_file()); + assert!(lock(root.path())["skills"].get("example").is_some()); +} + +#[test] +fn migration_preserves_recorded_source_ref() { + let (root, host) = fixture(); + let mut value = lock(root.path()); + value["skills"]["example"]["ref"] = json!("release/v1"); + fs::write( + root.path().join(".agents/.skill-lock.json"), + value.to_string(), + ) + .unwrap(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + let plan = host.doctor_plan(&report, 0, DoctorAction::Migrate).unwrap(); + assert!( + plan.preview + .source + .as_deref() + .unwrap() + .contains("release/v1") + ); +} + +#[test] +fn unscanned_subagent_targets_block_foreign_lock_removal() { + let (root, host) = fixture(); + let mut value = lock(root.path()); + value["skills"]["example"]["subagents"] = json!(["research"]); + fs::write( + root.path().join(".agents/.skill-lock.json"), + value.to_string(), + ) + .unwrap(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + assert!(host.doctor_plan(&report, 0, DoctorAction::Remove).is_err()); + assert!(lock(root.path())["skills"].get("example").is_some()); +} + +#[test] +fn newly_added_agent_target_rejects_stale_removal() { + let (root, host) = fixture(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + let plan = host.doctor_plan(&report, 0, DoctorAction::Remove).unwrap(); + fs::create_dir_all(root.path().join(".claude/skills/example")).unwrap(); + fs::write(root.path().join(".claude/skills/example/SKILL.md"), TEXT).unwrap(); + assert!(plan.apply().is_err()); + assert!( + root.path() + .join(".agents/skills/example/SKILL.md") + .is_file() + ); + assert!(lock(root.path())["skills"].get("example").is_some()); +} + +#[test] +fn newly_added_managed_skill_rejects_stale_migration() { + let (root, host) = fixture(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + let plan = host.doctor_plan(&report, 0, DoctorAction::Migrate).unwrap(); + fs::create_dir_all(root.path().join(".skilld/skills/example")).unwrap(); + fs::write( + root.path().join(".skilld/skills/example/SKILL.md"), + "Keep this", + ) + .unwrap(); + assert!(plan.apply().is_err()); + assert_eq!( + fs::read_to_string(root.path().join(".skilld/skills/example/SKILL.md")).unwrap(), + "Keep this" + ); + assert!( + root.path() + .join(".agents/skills/example/SKILL.md") + .is_file() + ); +} + +#[test] +fn migration_accepts_a_repository_root_skill() { + let (root, host) = fixture(); + let mut value = lock(root.path()); + value["skills"]["example"]["skillPath"] = json!("SKILL.md"); + value["skills"]["example"]["source"] = json!("owner/example"); + fs::write( + root.path().join(".agents/.skill-lock.json"), + value.to_string(), + ) + .unwrap(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + let plan = host.doctor_plan(&report, 0, DoctorAction::Migrate).unwrap(); + assert!(plan.preview.source.as_deref().unwrap().ends_with("/.")); + plan.apply().unwrap(); + assert!( + root.path() + .join(".skilld/skills/example/SKILL.md") + .is_file() + ); + host.remove("example", skilld_core::InstallScope::Global) + .unwrap(); + assert!(!root.path().join(".agents/skills/example").exists()); +} + +#[test] +fn legacy_codex_target_can_be_removed_but_cannot_be_migrated() { + let (root, host) = fixture(); + fs::create_dir_all(root.path().join(".codex/skills")).unwrap(); + fs::rename( + root.path().join(".agents/skills/example"), + root.path().join(".codex/skills/example"), + ) + .unwrap(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + assert!(host.doctor_plan(&report, 0, DoctorAction::Migrate).is_err()); + host.doctor_plan(&report, 0, DoctorAction::Remove) + .unwrap() + .apply() + .unwrap(); + assert!(!root.path().join(".codex/skills/example").exists()); +} + +#[test] +fn source_check_reports_exact_contents_and_resolved_commit_without_mutation() { + let (root, host) = fixture(); + let before = fs::read(root.path().join(".agents/.skill-lock.json")).unwrap(); + let mut options = DoctorOptions::for_root(root.path()); + options.check_sources = true; + let report = host.doctor_scan(&options).unwrap(); + let matched = report.skills[0].source_match.as_ref().unwrap(); + assert_eq!(matched.result, "Exact directory match."); + assert_eq!( + matched.commit.as_deref(), + Some("1111111111111111111111111111111111111111") + ); + assert_eq!( + fs::read(root.path().join(".agents/.skill-lock.json")).unwrap(), + before + ); + assert!(!root.path().join(".skilld/skills").exists()); +} + +#[cfg(unix)] +fn linked_source_fixture() -> (tempfile::TempDir, LocalHost) { + let (root, host) = fixture(); + fs::remove_file(root.path().join(".agents/.skill-lock.json")).unwrap(); + fs::create_dir_all(root.path().join("source")).unwrap(); + fs::rename( + root.path().join(".agents/skills/example"), + root.path().join("source/example"), + ) + .unwrap(); + fs::create_dir_all(root.path().join(".claude/skills")).unwrap(); + std::os::unix::fs::symlink( + "../../source/example", + root.path().join(".claude/skills/example"), + ) + .unwrap(); + (root, host) +} + +#[cfg(unix)] +#[test] +fn removing_only_an_agent_link_preserves_its_source_directory() { + let (root, host) = linked_source_fixture(); + let source = root.path().join("source/example"); + let link = root.path().join(".claude/skills/example"); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + let index = report + .skills + .iter() + .position(|s| s.canonical_path == source) + .unwrap(); + let plan = host + .doctor_plan(&report, index, DoctorAction::Remove) + .unwrap(); + assert_eq!(plan.preview.paths, vec![link.clone()]); + let done = plan.apply().unwrap(); + assert!(fs::symlink_metadata(&link).is_err()); + assert_eq!(fs::read_to_string(source.join("SKILL.md")).unwrap(), TEXT); + assert_eq!( + fs::read_link(done.backup.join("target-0")).unwrap(), + Path::new("../../source/example") + ); +} + +#[cfg(unix)] +#[test] +fn retargeting_a_direct_skill_link_rejects_the_reviewed_removal() { + let (root, host) = linked_source_fixture(); + let source = root.path().join("source/example"); + let link = root.path().join(".claude/skills/example"); + let other = root.path().join("source/other"); + fs::create_dir(&other).unwrap(); + fs::write(other.join("SKILL.md"), TEXT).unwrap(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + let index = report + .skills + .iter() + .position(|s| s.canonical_path == source) + .unwrap(); + let plan = host + .doctor_plan(&report, index, DoctorAction::Remove) + .unwrap(); + fs::remove_file(&link).unwrap(); + std::os::unix::fs::symlink("../../source/other", &link).unwrap(); + let error = plan.apply().unwrap_err(); + assert!(error.to_string().contains("link changed after review")); + assert_eq!( + fs::read_link(&link).unwrap(), + Path::new("../../source/other") + ); + assert_eq!(fs::read_to_string(source.join("SKILL.md")).unwrap(), TEXT); + assert_eq!(fs::read_to_string(other.join("SKILL.md")).unwrap(), TEXT); +} + +#[cfg(unix)] +#[test] +fn agent_root_aliases_backup_the_same_physical_skill_only_once() { + let (root, host) = fixture(); + fs::create_dir_all(root.path().join(".claude")).unwrap(); + std::os::unix::fs::symlink("../.agents/skills", root.path().join(".claude/skills")).unwrap(); + let physical = root.path().join(".agents/skills/example"); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + let index = report + .skills + .iter() + .position(|s| s.canonical_path == physical) + .unwrap(); + let plan = host + .doctor_plan(&report, index, DoctorAction::Remove) + .unwrap(); + assert_eq!(plan.preview.paths, vec![physical.clone()]); + let done = plan.apply().unwrap(); + assert!(fs::symlink_metadata(physical).is_err()); + assert_eq!( + fs::read_to_string(done.backup.join("target-0/SKILL.md")).unwrap(), + TEXT + ); + assert!(fs::symlink_metadata(done.backup.join("target-1")).is_err()); + assert_eq!( + fs::read_link(root.path().join(".claude/skills")).unwrap(), + Path::new("../.agents/skills") + ); + assert!(lock(root.path())["skills"].get("example").is_none()); +} + +#[cfg(unix)] +#[test] +fn removing_a_child_through_a_linked_root_preserves_the_root_link() { + let (root, host) = fixture(); + fs::remove_file(root.path().join(".agents/.skill-lock.json")).unwrap(); + fs::remove_dir_all(root.path().join(".agents/skills/example")).unwrap(); + let shared = root.path().join("shared"); + let physical = shared.join("example"); + fs::create_dir_all(&physical).unwrap(); + fs::write(physical.join("SKILL.md"), TEXT).unwrap(); + fs::write(shared.join("keep.md"), "Keep this sibling").unwrap(); + fs::create_dir_all(root.path().join(".claude")).unwrap(); + let linked_root = root.path().join(".claude/skills"); + std::os::unix::fs::symlink("../shared", &linked_root).unwrap(); + let report = host + .doctor_scan(&DoctorOptions::for_root(root.path())) + .unwrap(); + let index = report + .skills + .iter() + .position(|s| s.canonical_path == physical) + .unwrap(); + let plan = host + .doctor_plan(&report, index, DoctorAction::Remove) + .unwrap(); + assert_eq!(plan.preview.paths, vec![physical.clone()]); + let done = plan.apply().unwrap(); + assert!(fs::symlink_metadata(physical).is_err()); + assert_eq!(fs::read_link(&linked_root).unwrap(), Path::new("../shared")); + assert_eq!( + fs::read_to_string(shared.join("keep.md")).unwrap(), + "Keep this sibling" + ); + assert_eq!( + fs::read_to_string(done.backup.join("target-0/SKILL.md")).unwrap(), + TEXT + ); +} diff --git a/crates/skilld-command/tests/doctor_metadata.rs b/crates/skilld-command/tests/doctor_metadata.rs new file mode 100644 index 00000000..326ef797 --- /dev/null +++ b/crates/skilld-command/tests/doctor_metadata.rs @@ -0,0 +1,288 @@ +use std::fs; +use std::path::Path; +use std::process::Command; + +use serde_json::{Value, json}; +use skilld_command::doctor_metadata::{fingerprint, foreign_lock_without, read_foreign_lock}; + +fn skill(root: &Path) { + fs::create_dir_all(root.join("references")).unwrap(); + fs::write( + root.join("SKILL.md"), + "---\nname: example\n---\nInstructions.\n", + ) + .unwrap(); + fs::write(root.join("references/check.md"), "supporting file").unwrap(); +} + +fn global_lock(path: &Path) { + fs::write(path, serde_json::to_vec(&json!({ + "version": 3, + "skills": { + "example": {"source":"owner/repository", "sourceType":"github", "skillPath":"skills/example/SKILL.md", "ref":"release/v1", "skillFolderHash":"1234567890123456789012345678901234567890", "extra":42}, + "other": {"source":"owner/other", "sourceType":"github", "skillFolderHash":""} + }, + "dismissed":{"findSkillsPrompt":true}, + "lastSelectedAgents":["claude-code"], + "future":{"nested":[1,2,3]} + })).unwrap()).unwrap(); +} + +#[test] +fn fingerprint_matches_git_tree_and_tracks_supporting_files() { + let temp = tempfile::tempdir().unwrap(); + skill(temp.path()); + fs::write(temp.path().join("references.txt"), "ordering").unwrap(); + fs::create_dir(temp.path().join("empty")).unwrap(); + let result = fingerprint(temp.path()).unwrap(); + for args in [["init", "--quiet"].as_slice(), ["add", "."].as_slice()] { + assert!( + Command::new("git") + .args(args) + .current_dir(temp.path()) + .status() + .unwrap() + .success() + ); + } + let git = Command::new("git") + .arg("write-tree") + .current_dir(temp.path()) + .output() + .unwrap(); + assert!(git.status.success()); + assert_eq!( + result.git_tree, + String::from_utf8(git.stdout).unwrap().trim() + ); + assert_eq!(result.files, 3); + assert_eq!( + fingerprint(temp.path()).unwrap(), + result, + "ignore Git metadata" + ); + fs::write(temp.path().join("references/check.md"), "changed").unwrap(); + let changed = fingerprint(temp.path()).unwrap(); + assert_ne!(changed.sha256, result.sha256); + assert_ne!(changed.git_tree, result.git_tree); +} + +#[test] +fn fingerprint_counts_empty_directories_and_frames_names_and_bytes() { + let temp = tempfile::tempdir().unwrap(); + fs::write(temp.path().join("a"), "bc").unwrap(); + let before = fingerprint(temp.path()).unwrap(); + fs::remove_file(temp.path().join("a")).unwrap(); + fs::write(temp.path().join("ab"), "c").unwrap(); + let renamed = fingerprint(temp.path()).unwrap(); + assert_ne!(before.sha256, renamed.sha256); + fs::create_dir(temp.path().join("empty")).unwrap(); + let with_empty = fingerprint(temp.path()).unwrap(); + assert_ne!(with_empty.sha256, renamed.sha256); + assert_eq!(with_empty.git_tree, renamed.git_tree); +} + +#[cfg(unix)] +#[test] +fn fingerprint_tracks_permissions_and_links_without_following_them() { + use std::os::unix::fs::{PermissionsExt, symlink}; + let temp = tempfile::tempdir().unwrap(); + skill(temp.path()); + let before = fingerprint(temp.path()).unwrap(); + fs::set_permissions( + temp.path().join("SKILL.md"), + fs::Permissions::from_mode(0o755), + ) + .unwrap(); + let executable = fingerprint(temp.path()).unwrap(); + assert_ne!(before.sha256, executable.sha256); + assert_ne!(before.git_tree, executable.git_tree); + fs::set_permissions( + temp.path().join("SKILL.md"), + fs::Permissions::from_mode(0o750), + ) + .unwrap(); + let permissions = fingerprint(temp.path()).unwrap(); + assert_ne!(permissions.sha256, executable.sha256); + assert_eq!(permissions.git_tree, executable.git_tree); + symlink("references", temp.path().join("linked")).unwrap(); + let linked = fingerprint(temp.path()).unwrap(); + assert_eq!(linked.files, permissions.files + 1); + assert_eq!(linked.bytes, permissions.bytes + "references".len() as u64); + assert_ne!(linked.sha256, permissions.sha256); + symlink("../../outside", temp.path().join("escape")).unwrap(); + assert_eq!( + fingerprint(temp.path()).unwrap_err().code, + "DOCTOR_UNSUPPORTED_TREE" + ); +} + +#[test] +fn fingerprint_limits_depth_and_bytes() { + let temp = tempfile::tempdir().unwrap(); + let oversized = fs::File::create(temp.path().join("large")).unwrap(); + oversized.set_len(64 * 1024 * 1024 + 1).unwrap(); + assert_eq!( + fingerprint(temp.path()).unwrap_err().code, + "DOCTOR_SCAN_LIMIT" + ); + fs::remove_file(temp.path().join("large")).unwrap(); + let mut nested = temp.path().to_owned(); + for _ in 0..33 { + nested.push("nested"); + fs::create_dir(&nested).unwrap(); + } + assert_eq!( + fingerprint(temp.path()).unwrap_err().code, + "DOCTOR_SCAN_LIMIT" + ); +} + +#[test] +fn fingerprint_limits_file_count() { + let temp = tempfile::tempdir().unwrap(); + for index in 0..10_001 { + fs::write(temp.path().join(index.to_string()), "").unwrap(); + } + assert_eq!( + fingerprint(temp.path()).unwrap_err().code, + "DOCTOR_SCAN_LIMIT" + ); +} + +#[cfg(unix)] +#[test] +fn fingerprint_rejects_links_that_escape_through_another_link() { + use std::os::unix::fs::symlink; + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("skill"); + fs::create_dir_all(root.join("nested")).unwrap(); + fs::write(temp.path().join("outside"), "private").unwrap(); + symlink(".", root.join("nested/alias")).unwrap(); + symlink("alias/../../outside", root.join("nested/escape")).unwrap(); + assert_eq!( + fingerprint(&root).unwrap_err().code, + "DOCTOR_UNSUPPORTED_TREE" + ); +} + +#[test] +fn foreign_lock_rejects_duplicate_json_fields() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join(".skill-lock.json"); + fs::write(&path, br#"{"version":3,"skills":{},"skills":{"example":{"source":"owner/repo","sourceType":"github"}}}"#).unwrap(); + assert_eq!( + read_foreign_lock(&path, true).unwrap_err().code, + "DOCTOR_INVALID_LOCKFILE" + ); +} + +#[test] +fn foreign_lock_keeps_scoped_package_sources() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("skills-lock.json"); + fs::write(&path, serde_json::to_vec(&json!({"version":1,"skills":{"example":{"source":"@scope/package","sourceType":"node_modules","computedHash":"a".repeat(64)}}})).unwrap()).unwrap(); + let records = read_foreign_lock(&path, false).unwrap(); + assert_eq!(records[0].source, "@scope/package"); +} + +#[test] +fn foreign_lock_removes_one_record_and_preserves_unrelated_fields() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join(".skill-lock.json"); + global_lock(&path); + let original: Value = serde_json::from_slice(&fs::read(&path).unwrap()).unwrap(); + let records = read_foreign_lock(&path, true).unwrap(); + let record = records + .iter() + .find(|record| record.name == "example") + .unwrap(); + assert_eq!(record.source, "owner/repository"); + assert_eq!( + record.skill_path.as_deref(), + Some("skills/example/SKILL.md") + ); + assert_eq!(record.revision.as_deref(), Some("release/v1")); + assert_eq!( + record.expected_tree.as_deref(), + Some("1234567890123456789012345678901234567890") + ); + let output: Value = + serde_json::from_slice(&foreign_lock_without(&path, record).unwrap()).unwrap(); + let mut expected = original.clone(); + expected["skills"] + .as_object_mut() + .unwrap() + .remove("example"); + assert_eq!(output, expected); + assert_eq!( + serde_json::from_slice::(&fs::read(&path).unwrap()).unwrap(), + original + ); + fs::write(&path, b"{}").unwrap(); + assert_eq!( + foreign_lock_without(&path, record).unwrap_err().code, + "DOCTOR_STALE_LOCKFILE" + ); +} + +#[test] +fn project_content_hash_is_distinct_from_git_tree() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("skills-lock.json"); + let hash = "a".repeat(64); + fs::write(&path, serde_json::to_vec(&json!({"version":1,"skills":{"example":{"source":"owner/repository","sourceType":"github","computedHash":hash}}})).unwrap()).unwrap(); + let records = read_foreign_lock(&path, false).unwrap(); + assert_eq!(records[0].expected_content.as_deref(), Some(hash.as_str())); + assert_eq!(records[0].expected_tree, None); + assert!(!records[0].global); +} + +#[test] +fn foreign_lock_rejects_malformed_versions_records_and_paths() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join(".skill-lock.json"); + for input in [ + json!({"version":4,"skills":{}}), + json!({"version":2,"skills":{}}), + json!({"version":3,"skills":[]}), + json!({"version":3,"skills":{"../escape":{"source":"owner/repository","sourceType":"github"}}}), + json!({"version":3,"skills":{"example":{"source":"owner/repository","sourceType":"github","skillPath":"../SKILL.md"}}}), + json!({"version":3,"skills":{"example":{"source":"https://token@example.com/repo","sourceType":"git"}}}), + json!({"version":3,"skills":{"example":{"source":"owner/repository","sourceType":"github","skillFolderHash":"wrong"}}}), + json!({"version":3,"skills":{"example":{"sourceType":"github"}}}), + ] { + fs::write(&path, serde_json::to_vec(&input).unwrap()).unwrap(); + assert_eq!( + read_foreign_lock(&path, true).unwrap_err().code, + "DOCTOR_INVALID_LOCKFILE", + "{input}" + ); + } + fs::write(&path, b"{invalid").unwrap(); + assert_eq!( + read_foreign_lock(&path, true).unwrap_err().code, + "DOCTOR_INVALID_LOCKFILE" + ); +} + +#[cfg(unix)] +#[test] +fn foreign_lock_rejects_symlink_files() { + use std::os::unix::fs::symlink; + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join(".skill-lock.json"); + global_lock(&path); + let records = read_foreign_lock(&path, true).unwrap(); + let actual = temp.path().join("actual"); + fs::rename(&path, &actual).unwrap(); + symlink(&actual, &path).unwrap(); + assert_eq!( + read_foreign_lock(&path, true).unwrap_err().code, + "DOCTOR_INVALID_LOCKFILE" + ); + assert_eq!( + foreign_lock_without(&path, &records[0]).unwrap_err().code, + "DOCTOR_INVALID_LOCKFILE" + ); +} diff --git a/crates/skilld-command/tests/help_output.rs b/crates/skilld-command/tests/help_output.rs new file mode 100644 index 00000000..e0e1a54f --- /dev/null +++ b/crates/skilld-command/tests/help_output.rs @@ -0,0 +1,119 @@ +use skilld_command::{ + CommandError, CommandPlatform, Host, InstalledSkill, OutputContext, run_with_output, +}; +use skilld_core::{InstallScope, InstallSource}; + +struct HelpHost; +impl Host for HelpHost { + fn list(&self, _: InstallScope) -> Result, CommandError> { + unreachable!("help never calls the host") + } + fn install(&self, _: InstallSource, _: InstallScope) -> Result { + unreachable!("help never calls the host") + } +} + +fn run(args: &[&str], context: OutputContext) -> (u8, String, String) { + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let result = run_with_output(args, &HelpHost, context, &mut stdout, &mut stderr); + ( + result.exit_code, + String::from_utf8(stdout).unwrap(), + String::from_utf8(stderr).unwrap(), + ) +} +fn human(color: bool) -> OutputContext { + OutputContext::HumanTerminal { + width: 80, + color, + platform: CommandPlatform::Unix, + } +} +fn strip_styles(text: &str) -> String { + let mut characters = text.chars(); + let mut plain = String::new(); + while let Some(character) = characters.next() { + if character == '\x1b' { + assert_eq!(characters.next(), Some('[')); + for code in characters.by_ref() { + if code == 'm' { + break; + } + } + } else { + plain.push(character); + } + } + plain +} + +#[test] +fn root_subcommand_and_nested_help_share_colour_without_changing_words() { + for args in [ + vec!["skilld", "--help"], + vec!["skilld", "doctor", "--help"], + vec!["skilld", "auth", "login", "--help"], + ] { + let coloured = run(&args, human(true)); + let monochrome = run(&args, human(false)); + assert_eq!(coloured.0, 0); + assert!(coloured.2.is_empty()); + assert!(coloured.1.contains('\x1b'), "{args:?}: {}", coloured.1); + assert!(!monochrome.1.contains('\x1b')); + assert_eq!(strip_styles(&coloured.1), monochrome.1); + } +} + +#[test] +fn usage_errors_are_styled_on_stderr_with_hostile_arguments_escaped() { + let args = ["skilld", "search", "--evil\x1b[2J\nflag"]; + let coloured = run(&args, human(true)); + let monochrome = run(&args, human(false)); + assert_eq!(coloured.0, 2); + assert!(coloured.1.is_empty()); + assert!(coloured.2.contains('\x1b')); + assert!(!coloured.2.contains("\x1b[2J")); + assert!(coloured.2.contains("\\u{001B}")); + assert_eq!(strip_styles(&coloured.2), monochrome.2); +} + +#[test] +fn plain_and_json_help_keep_their_machine_contracts() { + let plain = run( + &["skilld", "auth", "login", "--plain", "--help"], + human(true), + ); + assert_eq!(plain.0, 0); + assert!(plain.2.is_empty()); + assert!(!plain.1.contains('\x1b')); + let json = run( + &["skilld", "auth", "login", "--json", "--help"], + human(true), + ); + assert_eq!(json.0, 0); + assert!(json.2.is_empty()); + assert!(!json.1.contains('\x1b')); + let envelope: serde_json::Value = serde_json::from_str(&json.1).unwrap(); + assert_eq!(envelope["command"], "help"); + assert_eq!(envelope["data"]["path"], "skilld auth"); +} + +#[test] +fn terminal_capabilities_disable_help_styles() { + for (no_color, dumb) in [(true, false), (false, true)] { + let context = OutputContext::auto( + true, + false, + false, + no_color, + dumb, + 80, + CommandPlatform::Unix, + ); + let output = run(&["skilld", "--help"], context); + assert_eq!(output.0, 0); + assert!(!output.1.contains('\x1b')); + assert!(output.1.contains("Usage:")); + } +} diff --git a/crates/skilld-command/tests/output.rs b/crates/skilld-command/tests/output.rs index de47a13f..1b2db155 100644 --- a/crates/skilld-command/tests/output.rs +++ b/crates/skilld-command/tests/output.rs @@ -15,7 +15,7 @@ struct SearchHost { impl Host for SearchHost { fn list(&self, _scope: InstallScope) -> Result, CommandError> { - unreachable!("list is outside this test") + Ok(Vec::new()) } fn install( @@ -77,6 +77,58 @@ const PLAIN: OutputContext = OutputContext::Plain { platform: CommandPlatform::Unix, }; +#[test] +fn empty_installed_list_explains_scope_and_discovery_without_machine_records() { + let context = OutputContext::HumanTerminal { + width: 80, + color: false, + platform: CommandPlatform::Unix, + }; + let (exit, human, errors) = run(&["skilld", "list", "--global"], context); + assert_eq!(exit, 0); + assert!(errors.is_empty()); + assert!(human.contains("Installed Skills · global · 0")); + assert!(human.contains("No Skills installed here.")); + assert!(human.contains("skilld search ")); + let (_, plain, _) = run(&["skilld", "list", "--global", "--plain"], context); + assert!(plain.is_empty()); +} + +#[test] +fn narrow_errors_keep_diagnostics_and_action_on_stderr() { + let host = SearchHost { + response: Err(CommandError { + kind: skilld_command::CommandErrorKind::Operation, + code: "SERVICE_UNAVAILABLE", + message: "The registry did not answer the Skill search request.".to_owned(), + next_step: Some("Check your connection. Try the search again.".to_owned()), + }), + }; + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let result = run_with_output( + ["skilld", "search", "design"], + &host, + OutputContext::HumanTerminal { + width: 24, + color: false, + platform: CommandPlatform::Unix, + }, + &mut stdout, + &mut stderr, + ); + let stderr = String::from_utf8(stderr).unwrap(); + assert_eq!(result.exit_code, 1); + assert!(stdout.is_empty()); + assert!(stderr.contains("SERVICE_UNAVAILABLE")); + assert!(stderr.contains("Next step")); + assert!( + stderr + .lines() + .all(|line| UnicodeWidthStr::width(line) <= 24) + ); +} + fn auto( stdout_is_terminal: bool, active_agent: bool, @@ -687,6 +739,7 @@ fn strip_ansi(value: &str) -> String { "\u{1b}[2m", "\u{1b}[36m", "\u{1b}[33m", + "\u{1b}[94m", "\u{1b}[0m", ] .into_iter() diff --git a/crates/skilld-native/src/behavior_prompt.rs b/crates/skilld-native/src/behavior_prompt.rs index fee3304b..e68dd21c 100644 --- a/crates/skilld-native/src/behavior_prompt.rs +++ b/crates/skilld-native/src/behavior_prompt.rs @@ -4,13 +4,13 @@ //! Only a person at a terminal sees it. An Agent, a pipe, or CI gets the //! stopped run and its `--allow` command instead. -use std::io::{self, BufRead, Write}; +use std::io::{self, BufRead, IsTerminal, Write}; use skilld_command::{ BEHAVIOR_CAVEAT, BehaviorConfirmer, BehaviorDecision, CommandError, describe_behavior, }; use skilld_core::Behavior; -use skilld_ui::text::sanitize; +use skilld_ui::{Role, text::sanitize, theme::paint}; /// Asks on standard error and reads one answer from standard input. pub struct TtyBehaviorConfirmer { @@ -33,11 +33,15 @@ impl BehaviorConfirmer for TtyBehaviorConfirmer { behaviors: &[&Behavior], ) -> Result { (self.before_ask)(); - ask( + let color = io::stderr().is_terminal() + && std::env::var_os("NO_COLOR").is_none() + && std::env::var("TERM").is_ok_and(|term| term != "dumb"); + ask_styled( &mut io::stdin().lock(), &mut io::stderr().lock(), skill, behaviors, + color, ) } } @@ -49,15 +53,41 @@ pub fn ask( skill: &str, behaviors: &[&Behavior], ) -> Result { - let mut question = format!( - "The Skill {} needs your approval for these behaviors:\n", - sanitize(skill) + ask_styled(input, output, skill, behaviors, false) +} + +/// Ask the same approval question with optional terminal styles. +pub fn ask_styled( + input: &mut impl BufRead, + output: &mut impl Write, + skill: &str, + behaviors: &[&Behavior], + color: bool, +) -> Result { + let mut question = paint( + &format!( + "The Skill {} needs your approval for these behaviors:", + sanitize(skill) + ), + Role::Warn, + color, ); + question.push('\n'); for behavior in behaviors { - question.push_str(&format!(" {}\n", sanitize(&describe_behavior(behavior)))); + question.push_str(&paint( + &format!(" {}", sanitize(&describe_behavior(behavior))), + Role::Emphasis, + color, + )); + question.push('\n'); } question.push_str(BEHAVIOR_CAVEAT); - question.push_str("\nContinue with this Skill? [y/N] "); + question.push('\n'); + question.push_str(&paint( + "Continue with this Skill? [y/N] ", + Role::Brand, + color, + )); output .write_all(question.as_bytes()) .and_then(|()| output.flush()) @@ -117,4 +147,24 @@ mod tests { assert!(question.contains("Runs code downloaded from the network: SKILL.md:7")); assert!(question.ends_with("[y/N] ")); } + + #[test] + fn styled_approval_preserves_the_default_and_cleans_remote_text() { + let behavior = remote_code(); + let mut output = Vec::new(); + let decision = ask_styled( + &mut "\n".as_bytes(), + &mut output, + "demo\u{1b}[2J", + &[&behavior], + true, + ) + .unwrap(); + let question = String::from_utf8(output).unwrap(); + assert_eq!(decision, BehaviorDecision::Declined); + assert!(question.contains("Runs code downloaded from the network: SKILL.md:7")); + assert!(question.contains("[y/N]")); + assert!(!question.contains("demo\u{1b}[2J")); + assert!(question.ends_with(skilld_ui::theme::RESET)); + } } diff --git a/crates/skilld-native/src/doctor_ui.rs b/crates/skilld-native/src/doctor_ui.rs new file mode 100644 index 00000000..42347341 --- /dev/null +++ b/crates/skilld-native/src/doctor_ui.rs @@ -0,0 +1,1247 @@ +use crate::terminal_theme::{selection, tone}; +use crate::update_ui::{InteractiveUpdateError, NativeTerminalLifecycle, with_restored_terminal}; +use crossterm::event::{self, Event, KeyCode, KeyEventKind}; +use ratatui::backend::{CrosstermBackend, TestBackend}; +use ratatui::layout::{Constraint, Layout, Rect}; +use ratatui::style::{Color, Modifier}; +use ratatui::text::{Line, Span}; +use ratatui::widgets::{Block, Borders, Gauge, List, ListItem, ListState, Paragraph, Wrap}; +use skilld_command::doctor::{ + DoctorOptions, DoctorOwner, DoctorReport, DoctorSkill, ScanPhase, ScanProgress, +}; +use skilld_command::doctor_actions::{ActionPreview, DoctorAction, DoctorApplied, DoctorPlan}; +use skilld_command::{CommandError, LocalHost}; +use skilld_ui::text::sanitize; +use std::cell::Cell; +use std::collections::BTreeSet; +use std::io::{self, Write}; +use std::path::PathBuf; +use std::sync::{Arc, mpsc}; +use std::time::{Duration, Instant}; + +const ACCENT: Color = Color::Cyan; +const MUTED: Color = Color::Reset; +fn in_group(skill: &DoctorSkill, owner: &str) -> bool { + if owner == "Duplicates" { + !skill.duplicates.is_empty() + } else { + skill.owner.label() == owner + } +} + +fn skill_count(count: usize) -> String { + count_label(count, "Skill") +} + +fn count_label(count: usize, noun: &str) -> String { + format!("{count} {noun}{}", if count == 1 { "" } else { "s" }) +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum WorkState { + Scanning, + Ready, + Preparing, + Applying, +} +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Screen { + Skills, + Problems, + Help, + Notice, +} + +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub enum Browse { + #[default] + Owners, + Locations { + owner: &'static str, + }, + Skills { + owner: &'static str, + location: Option, + }, +} + +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub enum PathFilter { + #[default] + All, + Symlinks, + Directories, +} + +impl PathFilter { + fn label(self) -> &'static str { + match self { + Self::All => "All paths", + Self::Symlinks => "Symlinks", + Self::Directories => "Directories", + } + } +} + +pub enum Entry { + Group { + label: String, + skills: Vec, + next: Browse, + }, + Skill(usize), +} + +fn locations(skill: &DoctorSkill, filter: PathFilter) -> BTreeSet> { + skill + .paths + .iter() + .filter(|p| match filter { + PathFilter::All => true, + PathFilter::Symlinks => p.is_linked(), + PathFilter::Directories => !p.is_linked(), + }) + .map(|p| { + p.project_root.clone().or_else(|| { + if p.agent.is_some() { + None + } else { + p.path.parent().map(PathBuf::from) + } + }) + }) + .collect() +} + +pub struct Model { + pub report: Option, + pub selected: usize, + pub browse: Browse, + pub path_filter: PathFilter, + pub preview: Option, + pub message: String, + pub work: WorkState, + pub screen: Screen, + pub detail_scroll: u16, + pub scroll_limit: Cell, + pub progress: Option, + pub roots: Vec, + pub elapsed: Duration, + pub color: bool, + pub filter: String, + pub editing_filter: bool, + pub details_focused: bool, + pub history: Vec, +} + +impl Default for Model { + fn default() -> Self { + Self { + report: None, + selected: 0, + browse: Browse::default(), + path_filter: PathFilter::default(), + preview: None, + message: "Scanning Skill files. Press q to cancel.".into(), + work: WorkState::Scanning, + screen: Screen::Skills, + detail_scroll: 0, + scroll_limit: Cell::new(0), + progress: None, + roots: vec![], + elapsed: Duration::ZERO, + color: true, + filter: String::new(), + editing_filter: false, + details_focused: false, + history: vec![], + } + } +} + +impl Model { + pub fn visible(&self) -> Vec { + self.report + .as_ref() + .map(|r| { + r.skills + .iter() + .enumerate() + .filter(|(_, s)| { + let scopes = locations(s, self.path_filter); + !scopes.is_empty() + && match &self.browse { + Browse::Owners => true, + Browse::Locations { owner } => in_group(s, owner), + Browse::Skills { owner, location } => { + in_group(s, owner) && scopes.contains(location) + } + } + && (self.filter.is_empty() + || format!( + "{} {} {} {}", + s.name, + s.owner.label(), + s.canonical_path.display(), + s.paths + .iter() + .map(|p| p.path.display().to_string()) + .collect::>() + .join(" ") + ) + .to_lowercase() + .contains(&self.filter.to_lowercase())) + }) + .map(|(i, _)| i) + .collect() + }) + .unwrap_or_default() + } + pub fn entries(&self) -> Vec { + let visible = self.visible(); + let Some(report) = &self.report else { + return vec![]; + }; + match &self.browse { + Browse::Owners => [ + ("skills.sh", "Review skills.sh migration"), + ("Unknown", "Inspect unknown installs"), + ("Unavailable", "Resolve ownership problems"), + ("Duplicates", "Review duplicate copies"), + ("skilld", "Browse skilld installs"), + ("Plugin", "Browse plugin Skills"), + ("Source", "Browse source directories"), + ] + .into_iter() + .filter_map(|(owner, label)| { + let skills: Vec<_> = visible + .iter() + .copied() + .filter(|&i| in_group(&report.skills[i], owner)) + .collect(); + (!skills.is_empty()).then(|| Entry::Group { + label: label.into(), + skills, + next: Browse::Locations { owner }, + }) + }) + .collect(), + Browse::Locations { owner } => { + let scopes: BTreeSet<_> = visible + .iter() + .flat_map(|&i| locations(&report.skills[i], self.path_filter)) + .collect(); + scopes + .into_iter() + .map(|location| Entry::Group { + label: location.as_ref().map_or_else( + || "Global Agent targets".into(), + |p| p.display().to_string(), + ), + skills: visible + .iter() + .copied() + .filter(|&i| { + locations(&report.skills[i], self.path_filter).contains(&location) + }) + .collect(), + next: Browse::Skills { owner, location }, + }) + .collect() + } + Browse::Skills { .. } => visible.into_iter().map(Entry::Skill).collect(), + } + } + pub fn selected_skill(&self) -> Option { + match self.entries().get(self.selected) { + Some(Entry::Skill(i)) => Some(*i), + _ => None, + } + } + pub fn open_group(&mut self) { + if self.work != WorkState::Ready || self.preview.is_some() { + return; + } + if let Some(Entry::Group { next, .. }) = self.entries().into_iter().nth(self.selected) { + self.history.push(self.selected); + self.browse = next; + self.reset_selection(); + } + } + pub fn back(&mut self) { + if self.screen != Screen::Skills { + self.screen = Screen::Skills; + } else if self.preview.take().is_some() { + // Keep the Skill selected after cancelling its action review. + } else if !self.filter.is_empty() { + self.filter.clear(); + } else { + self.browse = match self.browse { + Browse::Skills { owner, .. } => Browse::Locations { owner }, + _ => Browse::Owners, + }; + self.reset_selection(); + self.selected = self + .history + .pop() + .unwrap_or(0) + .min(self.entries().len().saturating_sub(1)); + } + self.detail_scroll = 0; + } + fn reset_selection(&mut self) { + self.selected = 0; + self.detail_scroll = 0; + self.details_focused = false; + } + pub fn move_selection(&mut self, down: bool) { + if self.preview.is_some() || self.details_focused || self.screen != Screen::Skills { + self.detail_scroll = if down { + self.detail_scroll + .saturating_add(1) + .min(self.scroll_limit.get()) + } else { + self.detail_scroll.saturating_sub(1) + }; + return; + } + let len = self.entries().len(); + if len > 0 { + self.selected = if down { + (self.selected + 1) % len + } else { + self.selected.checked_sub(1).unwrap_or(len - 1) + }; + } + self.detail_scroll = 0; + } + pub fn can_review_action(&self, width: u16, height: u16) -> bool { + width >= 60 + && height >= 18 + && self.work == WorkState::Ready + && self.screen == Screen::Skills + && self.preview.is_none() + && !self.editing_filter + && self.selected_skill().is_some() + } + pub fn toggle_group(&mut self) { + if self.work == WorkState::Ready && self.preview.is_none() { + self.path_filter = match self.path_filter { + PathFilter::All => PathFilter::Symlinks, + PathFilter::Symlinks => PathFilter::Directories, + PathFilter::Directories => PathFilter::All, + }; + self.browse = Browse::Owners; + self.history.clear(); + self.reset_selection(); + } + } +} + +pub fn view(frame: &mut ratatui::Frame<'_>, model: &Model) { + if frame.area().width < 60 || frame.area().height < 18 { + frame.render_widget( + Paragraph::new(if model.work == WorkState::Applying { + "skilld doctor\n\nApplying action. Wait for the result.\nResize to at least 60 columns and 18 rows." + } else { + "skilld doctor\n\nResize to at least 60 columns and 18 rows.\nq quit" + }) + .style(tone(model.color, ACCENT)), + frame.area(), + ); + return; + } + let rows = Layout::vertical([ + Constraint::Length(2), + Constraint::Min(4), + Constraint::Length(4), + ]) + .split(frame.area()); + let counts = model + .report + .as_ref() + .map(|r| { + format!( + "{} · {} · {}", + skill_count(r.skills.len()), + count_label( + r.skills + .iter() + .flat_map(|s| &s.paths) + .filter(|p| p.is_linked()) + .count(), + "linked path" + ), + count_label(r.problems.len(), "problem") + ) + }) + .unwrap_or_default(); + let scope = model + .roots + .iter() + .map(|p| sanitize(&p.display().to_string())) + .collect::>() + .join(", "); + frame.render_widget( + Paragraph::new(vec![ + Line::from(vec![ + Span::styled( + "skilld doctor", + tone(model.color, ACCENT).add_modifier(Modifier::BOLD), + ), + Span::raw(format!(" {counts}")), + ]), + Line::styled( + if model.work == WorkState::Scanning { + format!("Scan: {scope}") + } else { + let trail = match &model.browse { + Browse::Owners => "Overview".into(), + Browse::Locations { owner } => format!("Overview > {owner}"), + Browse::Skills { owner, location } => format!( + "Overview > {owner} > {}", + location.as_ref().map_or_else( + || "Global Agent targets".into(), + |p| p.display().to_string() + ) + ), + }; + format!("{} | {trail}", model.path_filter.label()) + }, + tone(model.color, MUTED), + ), + ]), + rows[0], + ); + if model.work == WorkState::Scanning { + loading(frame, model, rows[1]); + frame.render_widget( + Paragraph::new(vec![ + Line::raw("Scanning reads files. Cleanup actions need your review."), + Line::styled("q cancel", tone(model.color, ACCENT)), + ]), + rows[2], + ); + return; + } + let cols = if model.preview.is_some() || model.screen != Screen::Skills { + vec![Rect::new(rows[1].x, rows[1].y, 0, 0), rows[1]] + } else if frame.area().width < 76 { + Layout::vertical([Constraint::Percentage(40), Constraint::Percentage(60)]) + .split(rows[1]) + .to_vec() + } else { + Layout::horizontal([Constraint::Percentage(45), Constraint::Percentage(55)]) + .split(rows[1]) + .to_vec() + }; + let visible = model.visible(); + let entries = model.entries(); + let items = entries + .iter() + .map(|entry| match entry { + Entry::Group { + label, + skills, + next, + } => { + let short = match next { + Browse::Skills { + location: Some(p), .. + } => p + .file_name() + .map_or_else(|| label.clone(), |n| n.to_string_lossy().into_owned()), + _ => label.clone(), + }; + ListItem::new(vec![ + Line::styled( + sanitize(&short), + tone(model.color, ACCENT).add_modifier(Modifier::BOLD), + ), + Line::styled( + format!(" {} · Enter to review", skill_count(skills.len())), + tone(model.color, MUTED), + ), + ]) + } + Entry::Skill(i) => { + let s = &model.report.as_ref().expect("visible report").skills[*i]; + ListItem::new(Line::from(vec![ + Span::styled( + if s.duplicates.is_empty() { + "" + } else { + "Duplicate · " + }, + tone(model.color, Color::Yellow), + ), + Span::raw(sanitize(&s.name)), + ])) + } + }) + .collect::>(); + let mut state = ListState::default().with_selected(if entries.is_empty() { + None + } else { + Some(model.selected.min(entries.len() - 1)) + }); + frame.render_stateful_widget( + List::new(items) + .block( + Block::default() + .borders(Borders::ALL) + .border_style(tone( + model.color, + if model.details_focused { MUTED } else { ACCENT }, + )) + .title(format!( + " {}{} · {} ", + if !model.details_focused && model.preview.is_none() { + "> " + } else { + "" + }, + match model.browse { + Browse::Owners => "Recommendations", + Browse::Locations { .. } => "Projects / folders", + Browse::Skills { .. } => "Skills", + }, + entries.len() + )), + ) + .highlight_symbol("> ") + .highlight_style(selection()), + cols[0], + &mut state, + ); + let (title, details) = if model.screen == Screen::Help { + ( + "Help", + "Open a recommendation, then a folder, then a Skill. + +Up/down or j/k: select or scroll +Left/right: focus list or details +Enter: open group; Esc: back +Tab: All paths / Symlinks / Directories +/: filter Skills; Enter: finish; Esc: clear +PgUp/PgDn: scroll details +m: review migration; d: review removal +p: scan problems; n: full notice; ?: help; Esc: back +r: scan again; q or Ctrl-C: quit + +Actions always require a separate review. +While applying, wait for the result. +A Skill can appear in both path views. +Linked paths include links in parent folders." + .into(), + ) + } else if model.screen == Screen::Notice { + ("Latest notice", model.message.clone()) + } else if model.screen == Screen::Problems { + let text = model + .report + .as_ref() + .map(|report| { + report + .problems + .iter() + .map(|p| format!("{}\n{}\n", p.path.display(), p.message)) + .collect::>() + .join("\n") + }) + .unwrap_or_default(); + ( + "Scan problems", + if text.is_empty() { + "No scan problems. Esc returns to Skills.".into() + } else { + text + }, + ) + } else if let Some(p) = &model.preview { + let mut text = format!("{}: {}\n\n", p.action, p.name); + if let Some(source) = &p.source { + text.push_str(&format!( + "Source: {source}\nCommit: {}\n", + p.commit.as_deref().unwrap_or("unknown") + )); + } + if p.changes_content { + text.push_str( + "\nThis migration replaces contents with the source commit shown above.\n", + ); + } + if !p.behaviors.is_empty() { + text.push_str("\nSkill behaviors needing approval:\n"); + for behavior in &p.behaviors { + text.push_str(&format!("{behavior}\n")); + } + text.push_str("Enter approves these behaviors and applies the action.\n"); + } + text.push_str("\nAffected Agent targets:\n"); + for path in &p.paths { + text.push_str(&format!("{}\n", path.display())); + } + text.push_str("\nOriginal files and lock metadata will be backed up.\nEnter applies this action. Esc cancels.\nUp/down scroll all affected paths."); + ("Review action", text) + } else if let Some(Entry::Group { + label, + skills, + next, + }) = entries.get(model.selected) + { + let report = model.report.as_ref().expect("group report"); + let duplicate_count = skills + .iter() + .filter(|&&i| !report.skills[i].duplicates.is_empty()) + .count(); + let linked_count = skills + .iter() + .filter(|&&i| report.skills[i].paths.iter().any(|p| p.is_linked())) + .count(); + let linked_paths = skills + .iter() + .flat_map(|&i| &report.skills[i].paths) + .filter(|p| p.is_linked()) + .count(); + let linked_skills = skill_count(linked_count); + let description = match next { + Browse::Locations { owner: "skills.sh" } => { + "Installed through skills.sh. Review migration or removal for each Skill." + } + Browse::Locations { owner: "Unknown" } => { + "No installation record found. Inspect each Skill before removing it." + } + Browse::Locations { + owner: "Unavailable", + } => "Ownership could not be checked. Press p to inspect scan problems.", + Browse::Locations { + owner: "Duplicates", + } => { + "These directories contain identical files. Compare paths, then use each owner's removal workflow. Doctor reviews all targets for an install." + } + Browse::Locations { owner: "skilld" } => { + "Managed by skilld. Use skilld update or remove for these Skills." + } + Browse::Locations { owner: "Plugin" } => { + "Managed by plugins. Use the plugin manager to make changes." + } + Browse::Locations { owner: "Source" } => { + "Source directories. No cleanup action applies here." + } + _ => "Skills found in this project or folder. A Skill may appear in several locations.", + }; + ( + "Recommended next step", + format!( + "{label}\n\n{description}\n\nEnter opens this group.\n\n{}\n{linked_paths} linked paths across {linked_skills}\n{duplicate_count} with identical copies\n\nFilters only change this view.\nActions review all targets for the install.\nRecommendations can include the same Skill.\n\nScan: {scope}", + skill_count(skills.len()) + ), + ) + } else if let Some(i) = model.selected_skill() { + let s = &model.report.as_ref().expect("visible report").skills[i]; + let mut text = format!( + "{}\nOwner: {}\n\nFilters only change this view.\nActions review all targets for the install.\n\n{}\n", + s.name, + s.owner.label(), + s.canonical_path.display() + ); + if let DoctorOwner::SkillsSh { record } = &s.owner { + text.push_str(&format!( + "\nSource: {}\nLockfile: {}\n", + record.source, + record.lockfile.display() + )); + } + if let Some(f) = &s.fingerprint { + text.push_str(&format!( + "\n{} · {} bytes\nGit tree: {}\n", + count_label(f.files, "file"), + f.bytes, + f.git_tree + )); + } + text.push_str("\nDirectories:\n"); + if !s.paths.iter().any(|p| !p.is_linked()) { + text.push_str("None observed\n"); + } + for p in s.paths.iter().filter(|p| !p.is_linked()) { + text.push_str(&format!("{}\n", p.path.display())); + } + text.push_str("\nSymlinks:\n"); + let mut links = 0; + for p in &s.paths { + if !p.is_linked() { + continue; + } + links += 1; + text.push_str(&format!("{}\n", p.path.display())); + if let Some(target) = &p.symlink_target { + text.push_str(&format!(" -> {}\n", target.display())); + } + if let Some(parent) = &p.linked_parent { + text.push_str(&format!( + "Via linked folder:\n{}\n -> {}\n", + parent.path.display(), + parent.target.display() + )); + } + } + if links == 0 { + text.push_str("None\n"); + } + if !s.duplicates.is_empty() { + text.push_str(&format!( + "\nDuplicate: {} identical directory copies.\n", + s.duplicates.len() + )); + for path in &s.duplicates { + text.push_str(&format!("{}\n", path.display())); + } + text.push_str("Symlinks to this Skill are not duplicate copies.\n"); + } + if let Some(m) = &s.source_match { + text.push_str(&format!( + "\n{}\nCommit: {}\n{}\n", + m.source, + m.commit.as_deref().unwrap_or("unknown"), + m.result + )); + } + text.push_str(match &s.owner { + DoctorOwner::SkillsSh { .. } => { + "\nActions: m migrate to skilld · d remove.\nReview affected paths before applying." + } + DoctorOwner::Unknown => "\nNo installation record found.\nAction: d review removal.", + DoctorOwner::Skilld { .. } => "\nManaged by skilld. Use skilld update or remove.", + DoctorOwner::Plugin => "\nManaged by a plugin. Change it through its plugin manager.", + DoctorOwner::Source => "\nSource directory. No cleanup action applies here.", + DoctorOwner::Unavailable { .. } => { + "\nOwnership could not be checked. Resolve the scan problem first." + } + }); + ("Skill details", text) + } else { + ( + "Skill details", + if model.filter.is_empty() { + "No Skills in this group. Press Tab to switch groups." + } else { + "No Skills match this filter. Press / to change it." + } + .into(), + ) + }; + let limit = bounded_scroll(&details, cols[1], u16::MAX); + model.scroll_limit.set(limit); + frame.render_widget( + Paragraph::new(details.lines().map(sanitize).collect::>().join("\n")) + .block( + Block::default() + .borders(Borders::ALL) + .title(format!( + " {}{title} ", + if model.details_focused + || model.preview.is_some() + || model.screen != Screen::Skills + { + "> " + } else { + "" + } + )) + .border_style(tone( + model.color, + if model.preview.is_some() { + Color::Yellow + } else if model.details_focused { + ACCENT + } else { + MUTED + }, + )), + ) + .wrap(Wrap { trim: false }) + .scroll((model.detail_scroll.min(limit), 0)), + cols[1], + ); + let actions = model + .selected_skill() + .map(|i| &model.report.as_ref().expect("visible report").skills[i].owner); + let help = if model.editing_filter { + "type to filter enter done esc clear" + } else if model.work == WorkState::Applying { + "Applying action. Wait for the result." + } else if model.work == WorkState::Preparing { + "Preparing action q quit" + } else if model.screen != Screen::Skills { + "↑/↓ scroll esc back q quit" + } else if model.preview.is_some() { + "enter apply esc cancel ↑/↓ scroll" + } else if !matches!(model.browse, Browse::Skills { .. }) { + "enter open esc back / filter tab paths ? help q quit" + } else { + match actions { + Some(DoctorOwner::SkillsSh { .. }) => { + "m migrate d remove esc back / filter ? help q quit" + } + Some(DoctorOwner::Unknown) => "d remove esc back / filter ? help q quit", + _ => "esc back / filter tab paths ? help q quit", + } + }; + frame.render_widget( + Paragraph::new(vec![ + Line::styled( + sanitize(&model.message), + tone( + model.color, + if model.editing_filter { + ACCENT + } else { + Color::Reset + }, + ), + ), + Line::styled(help, tone(model.color, ACCENT)), + Line::styled( + if model.work == WorkState::Applying || model.work == WorkState::Preparing { + format!("{}s elapsed", model.elapsed.as_secs()) + } else if model.preview.is_some() { + "PgUp/PgDn scroll q quit".into() + } else if model.screen != Screen::Skills { + "PgUp/PgDn scroll".into() + } else if model.filter.is_empty() && !model.editing_filter { + "↑/↓ move ←/→ focus PgUp/PgDn scroll p problems n notice r rescan".into() + } else { + format!( + "Filter: {} {} matching / edit esc clear", + sanitize(&model.filter), + visible.len() + ) + }, + tone(model.color, MUTED), + ), + ]), + rows[2], + ); +} + +fn loading(frame: &mut ratatui::Frame<'_>, model: &Model, area: Rect) { + let block = Block::bordered() + .title(" Scan progress ") + .border_style(tone(model.color, ACCENT)); + let inner = block.inner(area); + frame.render_widget(block, area); + let phase = model + .progress + .as_ref() + .map_or(ScanPhase::Discover, |p| p.phase); + let (label, count) = match phase { + ScanPhase::Discover => ("Finding Skill files", None), + ScanPhase::Fingerprint { completed, total } => { + ("Checking Skill contents", Some((completed, total))) + } + ScanPhase::Sources { completed, total } => { + ("Checking source candidates", Some((completed, total))) + } + }; + let spinner = ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"] + [(model.elapsed.as_millis() / 100 % 10) as usize]; + let rows = Layout::vertical([ + Constraint::Length(2), + Constraint::Length(2), + Constraint::Min(3), + ]) + .split(inner); + frame.render_widget( + Paragraph::new(format!( + "{spinner} {label} {}s elapsed", + model.elapsed.as_secs() + )) + .style(tone(model.color, ACCENT).add_modifier(Modifier::BOLD)), + rows[0], + ); + if let Some((completed, total)) = count { + frame.render_widget( + Gauge::default() + .ratio(if total == 0 { + 1.0 + } else { + (completed as f64 / total as f64).min(1.0) + }) + .label(format!("{completed} / {total} Skills")) + .gauge_style(tone(model.color, ACCENT)), + rows[1], + ); + } else { + frame.render_widget( + Paragraph::new("Finding directories first. The total is not known yet.") + .style(tone(model.color, MUTED)), + rows[1], + ); + } + let mut lines = vec![]; + if let Some(p) = &model.progress { + lines.extend([ + Line::from(vec![ + Span::styled( + format!("{} Skills found", p.found_skills), + tone(model.color, Color::Green), + ), + Span::raw(format!(" {} directories scanned", p.visited_directories)), + ]), + Line::styled( + format!( + "{} excluded {} problems", + p.skipped_directories, p.problems + ), + tone( + model.color, + if p.problems == 0 { + MUTED + } else { + Color::Yellow + }, + ), + ), + Line::raw(""), + Line::styled("Current directory", tone(model.color, MUTED)), + Line::raw(sanitize(&p.current_path.display().to_string())), + ]); + } + frame.render_widget(Paragraph::new(lines).wrap(Wrap { trim: false }), rows[2]); +} + +fn bounded_scroll(text: &str, area: Rect, requested: u16) -> u16 { + let width = area.width.saturating_sub(2).max(1) as usize; + let height = area.height.saturating_sub(2) as usize; + let lines: usize = text + .lines() + .map(|line| { + unicode_width::UnicodeWidthStr::width(line) + .max(1) + .div_ceil(width) + }) + .sum(); + requested.min(lines.saturating_sub(height).min(u16::MAX as usize) as u16) +} + +pub fn render_snapshot(model: &Model, width: u16, height: u16) -> String { + let mut terminal = + ratatui::Terminal::new(TestBackend::new(width, height)).expect("test terminal"); + terminal.draw(|f| view(f, model)).expect("render"); + let buffer = terminal.backend().buffer(); + (0..height) + .map(|y| { + (0..width) + .map(|x| buffer[(x, y)].symbol()) + .collect::() + .trim_end() + .to_owned() + }) + .collect::>() + .join("\n") +} + +enum Job { + Progress(ScanProgress), + Scan(Result), + Plan(Result, CommandError>), + Applied(Result), +} + +pub fn run_doctor( + host: Arc, + options: DoctorOptions, +) -> Result<(), InteractiveUpdateError> { + let roots = if options.roots.is_empty() { + vec![ + host.doctor_context() + .map_err(|e| InteractiveUpdateError::new(e.code, e.message))? + .home, + ] + } else { + options.roots.clone() + }; + let summaries = with_restored_terminal(NativeTerminalLifecycle, || { + let mut terminal = + ratatui::Terminal::new(CrosstermBackend::new(io::stdout())).map_err(terminal_error)?; + let (tx, rx) = mpsc::channel(); + start_scan(host.clone(), options.clone(), tx.clone()); + let mut model = Model { + roots, + color: std::env::var_os("NO_COLOR").is_none() + && std::env::var("TERM").is_ok_and(|v| v != "dumb"), + ..Model::default() + }; + let mut started = Instant::now(); + let mut plan = None; + let mut summaries = Vec::new(); + let mut previous_path = None; + let mut completed_message = None; + loop { + while let Ok(job) = rx.try_recv() { + model.work = WorkState::Ready; + match job { + Job::Progress(progress) => { + model.work = WorkState::Scanning; + model.progress = Some(progress); + } + Job::Scan(result) => { + model.progress = None; + match result { + Ok(report) => { + model.message = if let Some(message) = completed_message.take() { + message + } else if report.problems.is_empty() { + "Choose a recommendation to review.".into() + } else { + format!( + "{}. Press p for paths and reasons.", + count_label(report.problems.len(), "scan problem") + ) + }; + model.report = Some(report); + model.selected = previous_path + .take() + .and_then(|path| { + model.visible().iter().position(|&index| { + model.report.as_ref().unwrap().skills[index] + .canonical_path + == path + }) + }) + .unwrap_or(0); + } + Err(e) => model.message = e.to_string(), + } + } + Job::Plan(result) => match result { + Ok(p) => { + let mut cancelled = false; + while event::poll(Duration::ZERO).map_err(terminal_error)? { + if let Event::Key(key) = event::read().map_err(terminal_error)? { + if key.kind != KeyEventKind::Press { + continue; + } + if key.code == KeyCode::Char('q') + || (key.code == KeyCode::Char('c') + && key.modifiers.contains(event::KeyModifiers::CONTROL)) + { + return Ok(summaries); + } + cancelled |= key.code == KeyCode::Esc; + } + } + if cancelled { + model.message = "Action review cancelled. No files changed.".into(); + continue; + } + model.preview = Some(p.preview.clone()); + model.detail_scroll = 0; + model.message = "Review every affected target before applying.".into(); + plan = Some(p); + } + Err(e) => model.message = e.to_string(), + }, + Job::Applied(result) => { + model.work = WorkState::Ready; + model.preview = None; + match result { + Ok(done) => { + model.message = + format!("{}. Backup: {}", done.message, done.backup.display()); + summaries.push(model.message.clone()); + completed_message = Some(model.message.clone()); + model.work = WorkState::Scanning; + started = Instant::now(); + start_scan(host.clone(), options.clone(), tx.clone()); + } + Err(e) => { + model.message = e.to_string(); + summaries.push(model.message.clone()); + } + } + } + } + } + if model.work != WorkState::Ready { + model.elapsed = started.elapsed(); + } + terminal.draw(|f| view(f, &model)).map_err(terminal_error)?; + if !event::poll(Duration::from_millis(100)).map_err(terminal_error)? { + continue; + } + let Event::Key(key) = event::read().map_err(terminal_error)? else { + continue; + }; + if key.kind != KeyEventKind::Press { + continue; + } + if model.editing_filter && !key.modifiers.contains(event::KeyModifiers::CONTROL) { + match key.code { + KeyCode::Esc => { + model.filter.clear(); + model.editing_filter = false; + } + KeyCode::Enter => model.editing_filter = false, + KeyCode::Backspace => { + model.filter.pop(); + } + KeyCode::Char(c) => model.filter.push(c), + _ => {} + } + model.selected = 0; + model.detail_scroll = 0; + continue; + } + if (key.code == KeyCode::Char('q') + || (key.code == KeyCode::Char('c') + && key.modifiers.contains(event::KeyModifiers::CONTROL))) + && model.work != WorkState::Applying + { + return Ok(summaries); + } + if model.work != WorkState::Ready { + continue; + } + let size = terminal.size().map_err(terminal_error)?; + if size.width < 60 || size.height < 18 { + continue; + } + if model.screen != Screen::Skills + && !matches!( + key.code, + KeyCode::Esc + | KeyCode::Up + | KeyCode::Down + | KeyCode::PageUp + | KeyCode::PageDown + | KeyCode::Char('j' | 'k' | '?' | 'p' | 'n') + ) + { + continue; + } + match key.code { + KeyCode::Down | KeyCode::Char('j') => model.move_selection(true), + KeyCode::Up | KeyCode::Char('k') => model.move_selection(false), + KeyCode::PageDown => { + model.detail_scroll = model + .detail_scroll + .saturating_add(size.height.saturating_sub(8)) + .min(model.scroll_limit.get()) + } + KeyCode::PageUp => { + model.detail_scroll = model + .detail_scroll + .min(model.scroll_limit.get()) + .saturating_sub(size.height.saturating_sub(8)) + } + KeyCode::Left => model.details_focused = false, + KeyCode::Right => model.details_focused = true, + KeyCode::Char('/') if model.preview.is_none() => model.editing_filter = true, + KeyCode::Tab => model.toggle_group(), + KeyCode::Char('?') if model.preview.is_none() => { + model.screen = Screen::Help; + model.detail_scroll = 0; + } + KeyCode::Char('p') if model.preview.is_none() => { + model.screen = Screen::Problems; + model.detail_scroll = 0; + } + KeyCode::Char('n') if model.preview.is_none() => { + model.screen = Screen::Notice; + model.detail_scroll = 0; + } + KeyCode::Esc => { + plan = None; + model.back(); + } + KeyCode::Enter + if model.work == WorkState::Ready && model.screen == Screen::Skills => + { + if let Some(p) = plan.take() { + previous_path = model.selected_skill().map(|i| { + model.report.as_ref().unwrap().skills[i] + .canonical_path + .clone() + }); + model.work = WorkState::Applying; + started = Instant::now(); + model.message = "Applying reviewed action. Keep this terminal open.".into(); + let tx = tx.clone(); + std::thread::spawn(move || { + let _ = tx.send(Job::Applied(p.apply())); + }); + } else { + model.open_group(); + } + } + KeyCode::Char('r') if model.preview.is_none() => { + previous_path = model.selected_skill().map(|i| { + model.report.as_ref().unwrap().skills[i] + .canonical_path + .clone() + }); + model.work = WorkState::Scanning; + started = Instant::now(); + start_scan(host.clone(), options.clone(), tx.clone()); + } + KeyCode::Char('m' | 'd') if model.can_review_action(size.width, size.height) => { + if let (Some(report), Some(index)) = + (model.report.clone(), model.selected_skill()) + { + let owner = &report.skills[index].owner; + if !matches!(owner, DoctorOwner::SkillsSh { .. } | DoctorOwner::Unknown) + || (key.code == KeyCode::Char('m') + && !matches!(owner, DoctorOwner::SkillsSh { .. })) + { + continue; + } + let action = if key.code == KeyCode::Char('m') { + DoctorAction::Migrate + } else { + DoctorAction::Remove + }; + model.work = WorkState::Preparing; + started = Instant::now(); + model.message = "Preparing action and checking source contents.".into(); + let host = host.clone(); + let tx = tx.clone(); + std::thread::spawn(move || { + let _ = tx.send(Job::Plan( + host.doctor_plan(&report, index, action).map(Box::new), + )); + }); + } + } + _ => {} + } + } + })?; + let mut output = io::stdout().lock(); + for summary in summaries { + writeln!(output, "{}", sanitize(&summary)).map_err(terminal_error)?; + } + Ok(()) +} + +fn start_scan(host: Arc, options: DoctorOptions, tx: mpsc::Sender) { + std::thread::spawn(move || { + let mut last = Instant::now(); + let mut phase = None; + let result = host.doctor_scan_with_progress(&options, &mut |progress| { + let current = std::mem::discriminant(&progress.phase); + if phase != Some(current) || last.elapsed() >= Duration::from_millis(80) { + // A closed receiver means the person quit the read-only scan. + let _ = tx.send(Job::Progress(progress)); + last = Instant::now(); + phase = Some(current); + } + }); + let _ = tx.send(Job::Scan(result)); + }); +} +fn terminal_error(error: io::Error) -> InteractiveUpdateError { + InteractiveUpdateError::new("TERMINAL_UNAVAILABLE", error.to_string()) +} diff --git a/crates/skilld-native/src/lib.rs b/crates/skilld-native/src/lib.rs index 48c4aeae..2ead04b9 100644 --- a/crates/skilld-native/src/lib.rs +++ b/crates/skilld-native/src/lib.rs @@ -11,7 +11,10 @@ use url::Url; pub mod behavior_prompt; #[cfg(not(target_os = "wasi"))] +pub mod doctor_ui; +#[cfg(not(target_os = "wasi"))] pub mod select_ui; +mod terminal_theme; pub mod update_ui; #[cfg(not(target_os = "wasi"))] pub mod upgrade; diff --git a/crates/skilld-native/src/main.rs b/crates/skilld-native/src/main.rs index 0592d2d5..c41f7ea9 100644 --- a/crates/skilld-native/src/main.rs +++ b/crates/skilld-native/src/main.rs @@ -55,6 +55,15 @@ fn main() -> ExitCode { return ExitCode::SUCCESS; } let args = env::args_os().collect::>(); + let doctor = skilld_command::interactive_doctor_requested(args.clone()) + .ok() + .flatten() + .filter(|_| { + std::io::stdin().is_terminal() + && std::io::stdout().is_terminal() + && !active_agent_detected() + && !environment_enabled("CI") + }); let upgrade_session = match start_upgrade(&args) { UpgradeStartup::Continue(session) => session, UpgradeStartup::Exit(exit) => return exit, @@ -89,7 +98,7 @@ fn main() -> ExitCode { terminal_width(), CommandPlatform::current(), ); - let label = if interactive { + let label = if interactive || doctor.is_some() { None } else { status::status_label(args.iter().map(|arg| arg.to_string_lossy())) @@ -171,6 +180,16 @@ fn main() -> ExitCode { }; let host = Arc::new(host); + if let Some(options) = doctor { + return match skilld_native::doctor_ui::run_doctor(host, options) { + Ok(()) => ExitCode::SUCCESS, + Err(error) => { + eprintln!("{error}"); + ExitCode::from(2) + } + }; + } + if interactive { let interactive_host = Arc::new(CommandInteractiveUpdateHost::new(host)); return match run_interactive_update(interactive_host, !environment_present("NO_COLOR")) { diff --git a/crates/skilld-native/src/select_ui.rs b/crates/skilld-native/src/select_ui.rs index 9b3daed0..258ebf8c 100644 --- a/crates/skilld-native/src/select_ui.rs +++ b/crates/skilld-native/src/select_ui.rs @@ -6,11 +6,12 @@ use std::io; +use crate::terminal_theme::{selection, tone}; use crossterm::event::{self, Event, KeyCode, KeyEvent, KeyEventKind, KeyModifiers}; use ratatui::Terminal; use ratatui::backend::{CrosstermBackend, TestBackend}; use ratatui::layout::{Constraint, Layout}; -use ratatui::style::{Color, Modifier, Style}; +use ratatui::style::{Color, Modifier}; use ratatui::text::{Line, Span}; use ratatui::widgets::{List, ListItem, ListState, Paragraph}; use skilld_command::{CommandError, SkillChooser}; @@ -40,6 +41,8 @@ pub enum PickerKey { ToggleAll, Confirm, Cancel, + /// Quit without preserving or clearing an active filter first. + Quit, /// Start typing a filter. FilterStart, /// One character typed into the filter. @@ -163,6 +166,7 @@ impl PickerModel { } } PickerKey::Confirm => self.outcome = Some(PickerOutcome::Chose(self.chosen())), + PickerKey::Quit => self.outcome = Some(PickerOutcome::Cancelled), PickerKey::Cancel => { // Escape backs out of the filter first. It cancels the picker // only when there is nothing to back out of. @@ -248,6 +252,10 @@ pub fn run_skill_picker( with_restored_terminal(NativeTerminalLifecycle, || { let backend = CrosstermBackend::new(io::stdout()); let mut terminal = Terminal::new(backend).map_err(terminal_lost)?; + // Input buffered for the previous command is not installation consent. + while event::poll(Duration::ZERO).map_err(terminal_lost)? { + let _ = event::read().map_err(terminal_lost)?; + } loop { terminal .draw(|frame| view(frame, &model, color)) @@ -259,6 +267,13 @@ pub fn run_skill_picker( && let Event::Key(event) = event::read().map_err(terminal_lost)? && let Some(key) = picker_key(event, model.filtering()) { + let area = terminal.size().map_err(terminal_lost)?; + if area.width < 20 || area.height < 5 { + if matches!(key, PickerKey::Cancel | PickerKey::Quit) { + model.update(PickerKey::Quit); + } + continue; + } model.update(key); } } @@ -271,6 +286,10 @@ const MARK_FREE: &str = "\u{25cb}"; const CURSOR: &str = "\u{276f} "; fn view(frame: &mut ratatui::Frame<'_>, model: &PickerModel, color: bool) { + if frame.area().width < 20 || frame.area().height < 5 { + frame.render_widget(Paragraph::new("Resize to 20x5\nEsc cancel"), frame.area()); + return; + } let areas = Layout::vertical([ Constraint::Length(2), Constraint::Min(1), @@ -284,17 +303,18 @@ fn view(frame: &mut ratatui::Frame<'_>, model: &PickerModel, color: bool) { frame.render_widget( Paragraph::new(Line::from(vec![ Span::styled( - "No Skill matches ", - Style::default().fg(theme(color, Color::DarkGray)), - ), - Span::styled( - sanitize(model.filter()), - Style::default().fg(theme(color, Color::Yellow)), + if model.choices().is_empty() { + "No Skills in this ref." + } else { + "No Skill matches " + }, + tone(color, Color::DarkGray), ), + Span::styled(sanitize(model.filter()), tone(color, Color::Yellow)), ])), areas[1], ); - frame.render_widget(footer(model, color), areas[2]); + frame.render_widget(footer(model, color, frame.area().width), areas[2]); return; } // The name column is as wide as the longest name, so every description @@ -303,7 +323,8 @@ fn view(frame: &mut ratatui::Frame<'_>, model: &PickerModel, color: bool) { .iter() .map(|position| UnicodeWidthStr::width(model.choices()[*position].label.as_str())) .max() - .unwrap_or(0); + .unwrap_or(0) + .min(width.saturating_sub(UnicodeWidthStr::width(CURSOR) + 4)); let items = visible .iter() .map(|position| row(&model.choices()[*position], name_width, width, color)) @@ -312,41 +333,37 @@ fn view(frame: &mut ratatui::Frame<'_>, model: &PickerModel, color: bool) { frame.render_stateful_widget( List::new(items) .highlight_symbol(CURSOR) - .highlight_style(Style::default().add_modifier(Modifier::BOLD)), + .highlight_style(selection()), areas[1], &mut state, ); - frame.render_widget(footer(model, color), areas[2]); + frame.render_widget(footer(model, color, frame.area().width), areas[2]); } /// The ref, the counts, and one blank line. fn header(model: &PickerModel, color: bool) -> Paragraph<'static> { let (chosen, total) = model.counts(); let chosen_style = if chosen == 0 { - Style::default().fg(theme(color, Color::DarkGray)) + tone(color, Color::DarkGray) } else { - Style::default() - .fg(theme(color, Color::Green)) - .add_modifier(Modifier::BOLD) + tone(color, Color::Green).add_modifier(Modifier::BOLD) }; let shown = model.visible().len(); let mut counts = vec![ Span::styled( sanitize(model.reference()), - Style::default() - .fg(theme(color, Color::Cyan)) - .add_modifier(Modifier::BOLD), + tone(color, Color::Cyan).add_modifier(Modifier::BOLD), ), Span::styled( format!(" names {total} Skills. "), - Style::default().fg(theme(color, Color::Reset)), + tone(color, Color::Reset), ), Span::styled(format!("{chosen} chosen"), chosen_style), ]; if shown != total { counts.push(Span::styled( format!(", {shown} shown"), - Style::default().fg(theme(color, Color::DarkGray)), + tone(color, Color::DarkGray), )); } Paragraph::new(vec![Line::from(counts), filter_line(model, color)]) @@ -358,22 +375,14 @@ fn filter_line(model: &PickerModel, color: bool) -> Line<'static> { return Line::from(String::new()); } let mut spans = vec![ - Span::styled( - "filter ", - Style::default().fg(theme(color, Color::DarkGray)), - ), + Span::styled("filter ", tone(color, Color::DarkGray)), Span::styled( sanitize(model.filter()), - Style::default() - .fg(theme(color, Color::Yellow)) - .add_modifier(Modifier::BOLD), + tone(color, Color::Yellow).add_modifier(Modifier::BOLD), ), ]; if model.filtering() { - spans.push(Span::styled( - "\u{2588}", - Style::default().fg(theme(color, Color::Yellow)), - )); + spans.push(Span::styled("\u{2588}", tone(color, Color::Yellow))); } Line::from(spans) } @@ -383,21 +392,16 @@ fn row(choice: &SkillChoice, name_width: usize, width: usize, color: bool) -> Li let mark = if choice.selected { Span::styled( MARK_CHOSEN, - Style::default() - .fg(theme(color, Color::Green)) - .add_modifier(Modifier::BOLD), + tone(color, Color::Green).add_modifier(Modifier::BOLD), ) } else { - Span::styled( - MARK_FREE, - Style::default().fg(theme(color, Color::DarkGray)), - ) + Span::styled(MARK_FREE, tone(color, Color::DarkGray)) }; - let name = sanitize(&choice.label); + let name = truncate(&sanitize(&choice.label), name_width); let name_style = if choice.selected { - Style::default().fg(theme(color, Color::Reset)) + tone(color, Color::Reset) } else { - Style::default().fg(theme(color, Color::Gray)) + tone(color, Color::Gray) }; let padding = name_width.saturating_sub(UnicodeWidthStr::width(name.as_str())); let mut spans = vec![ @@ -416,7 +420,7 @@ fn row(choice: &SkillChoice, name_width: usize, width: usize, color: bool) -> Li if room > 1 { spans.push(Span::styled( truncate(&clean(description), room), - Style::default().fg(theme(color, Color::DarkGray)), + tone(color, Color::DarkGray), )); } } @@ -426,12 +430,32 @@ fn row(choice: &SkillChoice, name_width: usize, width: usize, color: bool) -> Li /// The key hints. Each key is lit, each verb stays dim. /// /// Typing a filter takes the letter keys, so the hints say what is left. -fn footer(model: &PickerModel, color: bool) -> Paragraph<'static> { - let key = Style::default() - .fg(theme(color, Color::Cyan)) - .add_modifier(Modifier::BOLD); - let text = Style::default().fg(theme(color, Color::DarkGray)); - let hints = if model.filtering() { +fn footer(model: &PickerModel, color: bool, width: u16) -> Paragraph<'static> { + let key = tone(color, Color::Cyan).add_modifier(Modifier::BOLD); + let text = tone(color, Color::DarkGray); + if width < 50 { + let lines = if model.choices().is_empty() { + ["No Skills to install", "Esc cancel"] + } else if model.filtering() { + ["Enter done Esc clear", "↑/↓ move Ctrl+C quit"] + } else if width < 30 { + ["Enter install q quit", "Space choose / filter"] + } else { + [ + "↑/↓ move Space choose a all", + "Enter install / filter q quit", + ] + }; + return Paragraph::new( + lines + .into_iter() + .map(|line| Line::styled(line, key)) + .collect::>(), + ); + } + let hints = if model.choices().is_empty() { + vec![("esc", " cancel")] + } else if model.filtering() { vec![ ("type", " to filter "), ("enter", " keep it "), @@ -459,12 +483,17 @@ fn footer(model: &PickerModel, color: bool) -> Paragraph<'static> { spans.push(Span::styled(name, key)); spans.push(Span::styled(verb, text)); } - Paragraph::new(vec![Line::from(String::new()), Line::from(spans)]) -} - -/// Colors are off under NO_COLOR, so every style falls back to the default. -const fn theme(color: bool, value: Color) -> Color { - if color { value } else { Color::Reset } + Paragraph::new(vec![ + Line::styled( + if model.filtering() { + "↑/↓ move Ctrl+C cancel" + } else { + "↑/↓ move j/k move q cancel Ctrl+C cancel" + }, + text, + ), + Line::from(spans), + ]) } /// One line of text, with runs of whitespace collapsed. @@ -526,7 +555,7 @@ fn picker_key(event: KeyEvent, filtering: bool) -> Option { return None; } if event.modifiers.contains(KeyModifiers::CONTROL) && event.code == KeyCode::Char('c') { - return Some(PickerKey::Cancel); + return Some(PickerKey::Quit); } if filtering { return match event.code { @@ -550,7 +579,8 @@ fn picker_key(event: KeyEvent, filtering: bool) -> Option { KeyCode::Char('a') => Some(PickerKey::ToggleAll), KeyCode::Char('/') => Some(PickerKey::FilterStart), KeyCode::Enter => Some(PickerKey::Confirm), - KeyCode::Char('q') | KeyCode::Esc => Some(PickerKey::Cancel), + KeyCode::Char('q') => Some(PickerKey::Quit), + KeyCode::Esc => Some(PickerKey::Cancel), _ => None, } } diff --git a/crates/skilld-native/src/terminal_theme.rs b/crates/skilld-native/src/terminal_theme.rs new file mode 100644 index 00000000..ca965ad9 --- /dev/null +++ b/crates/skilld-native/src/terminal_theme.rs @@ -0,0 +1,21 @@ +//! Terminal palette styles for screens that inherit the user's background. + +use ratatui::style::{Color, Modifier, Style}; + +pub(crate) fn tone(color: bool, foreground: Color) -> Style { + if !color { + return Style::default(); + } + match foreground { + Color::Gray | Color::DarkGray => Style::default().add_modifier(Modifier::DIM), + foreground => Style::default().fg(foreground), + } +} + +pub(crate) fn selection() -> Style { + Style::default() + .fg(Color::Reset) + .bg(Color::Reset) + .remove_modifier(Modifier::DIM) + .add_modifier(Modifier::REVERSED | Modifier::BOLD) +} diff --git a/crates/skilld-native/src/update_ui.rs b/crates/skilld-native/src/update_ui.rs index dc520a35..0719220c 100644 --- a/crates/skilld-native/src/update_ui.rs +++ b/crates/skilld-native/src/update_ui.rs @@ -7,6 +7,7 @@ use std::sync::{Arc, Mutex}; use std::thread; use std::time::Duration; +use crate::terminal_theme::{selection, tone}; use crossterm::cursor::{Hide, Show}; use crossterm::event::{self, Event, KeyCode, KeyEvent, KeyEventKind, KeyModifiers}; use crossterm::execute; @@ -16,7 +17,7 @@ use crossterm::terminal::{ use ratatui::Terminal; use ratatui::backend::{CrosstermBackend, TestBackend}; use ratatui::layout::{Constraint, Layout, Rect}; -use ratatui::style::{Color, Modifier, Style}; +use ratatui::style::{Color, Modifier}; use ratatui::text::{Line, Text}; use ratatui::widgets::{Block, List, ListItem, ListState, Paragraph, Tabs}; use skilld_command::{CommandError, Host}; @@ -893,6 +894,9 @@ fn update_key(model: &mut Model, key: KeyInput, effects: &mut Vec) { model.expanded_help = !model.expanded_help; return; } + if model.width < 20 || model.height < 7 { + return; + } if let Phase::FailedLoad(_) = &model.phase { if key == KeyInput::Retry { model.phase = Phase::LoadingCandidates; @@ -1004,6 +1008,10 @@ fn update_key(model: &mut Model, key: KeyInput, effects: &mut Vec) { } pub fn view(frame: &mut ratatui::Frame<'_>, model: &Model, color: bool) { + if frame.area().width < 20 || frame.area().height < 7 { + frame.render_widget(Paragraph::new("Resize to 20x7\nq cancel"), frame.area()); + return; + } let help_height = if model.expanded_help { 3 } else { 1 }; let areas = Layout::vertical([ Constraint::Length(1), @@ -1015,23 +1023,20 @@ pub fn view(frame: &mut ratatui::Frame<'_>, model: &Model, color: bool) { .split(frame.area()); frame.render_widget( - Paragraph::new("skilld update").style( - Style::default() - .fg(theme(color, Color::Cyan)) - .add_modifier(Modifier::BOLD), - ), + Paragraph::new("skilld update") + .style(tone(color, Color::Cyan).add_modifier(Modifier::BOLD)), areas[0], ); - render_tabs(frame, model, color, areas[1]); + render_tabs(frame, model, areas[1]); render_body(frame, model, color, areas[2]); frame.render_widget(Paragraph::new(status_text(model)), areas[3]); frame.render_widget( - Paragraph::new(help_text(model)).style(Style::default().fg(theme(color, Color::DarkGray))), + Paragraph::new(help_text(model)).style(tone(color, Color::DarkGray)), areas[4], ); } -fn render_tabs(frame: &mut ratatui::Frame<'_>, model: &Model, color: bool, area: Rect) { +fn render_tabs(frame: &mut ratatui::Frame<'_>, model: &Model, area: Rect) { let outdated = format!( "Outdated {}", model @@ -1050,11 +1055,7 @@ fn render_tabs(frame: &mut ratatui::Frame<'_>, model: &Model, color: bool, area: Tab::Outdated => 0, Tab::Commits => 1, }) - .highlight_style( - Style::default() - .fg(theme(color, Color::Cyan)) - .add_modifier(Modifier::BOLD), - ); + .highlight_style(selection()); frame.render_widget(tabs, area); } @@ -1075,7 +1076,7 @@ fn render_body(frame: &mut ratatui::Frame<'_>, model: &Model, color: bool, area: Line::from("The update plan could not load."), Line::from(format!("{}: {}", error.code, error.message)), ])) - .style(Style::default().fg(theme(color, Color::Red))), + .style(tone(color, Color::Red)), area, ); } @@ -1136,7 +1137,7 @@ fn render_outdated(frame: &mut ratatui::Frame<'_>, model: &Model, color: bool, a Line::from(truncate(&format!("⚠ {name}: {}", error.code), width)), Line::from(truncate(&format!(" {}", error.message), width)), ])) - .style(Style::default().fg(theme(color, Color::Red))) + .style(tone(color, Color::Red)) } } }) @@ -1144,11 +1145,7 @@ fn render_outdated(frame: &mut ratatui::Frame<'_>, model: &Model, color: bool, a let list = List::new(items) .block(Block::bordered().title("Select Skills to update")) .highlight_symbol("> ") - .highlight_style( - Style::default() - .fg(theme(color, Color::Cyan)) - .add_modifier(Modifier::BOLD), - ); + .highlight_style(selection()); let mut state = ListState::default().with_selected( (!model.rows.is_empty()).then_some(model.cursor.saturating_sub(model.viewport)), ); @@ -1183,9 +1180,7 @@ fn render_commits(frame: &mut ratatui::Frame<'_>, model: &Model, color: bool, ar ), width, ), - Style::default() - .fg(theme(color, Color::Cyan)) - .add_modifier(Modifier::BOLD), + tone(color, Color::Cyan).add_modifier(Modifier::BOLD), )); match model.commits.get(id) { Some(CommitState::NotRequested) => { @@ -1200,7 +1195,7 @@ fn render_commits(frame: &mut ratatui::Frame<'_>, model: &Model, color: bool, ar Some(CommitState::Failed(error)) => { lines.push(Line::styled( truncate(&format!(" {}: {}", error.code, error.message), width), - Style::default().fg(theme(color, Color::Red)), + tone(color, Color::Red), )); } Some(CommitState::Ready(page)) => { @@ -1232,7 +1227,7 @@ fn render_commits(frame: &mut ratatui::Frame<'_>, model: &Model, color: bool, ar ), width, ), - Style::default().fg(theme(color, Color::Yellow)), + tone(color, Color::Yellow), )); } } @@ -1372,7 +1367,11 @@ fn bindings(model: &Model) -> Vec { }, Binding { keys: "q", - action: "cancel", + action: if matches!(model.phase, Phase::Applying) { + "exit after update" + } else { + "cancel" + }, }, ]); bindings @@ -1391,10 +1390,6 @@ fn help_text(model: &Model) -> String { } } -fn theme(color: bool, value: Color) -> Color { - if color { value } else { Color::Reset } -} - fn short_sha(value: &str) -> &str { value.get(..7).unwrap_or(value) } diff --git a/crates/skilld-native/src/upgrade_ui.rs b/crates/skilld-native/src/upgrade_ui.rs index 0440d78c..b12fb513 100644 --- a/crates/skilld-native/src/upgrade_ui.rs +++ b/crates/skilld-native/src/upgrade_ui.rs @@ -7,25 +7,17 @@ use crossterm::event::{self, Event, KeyCode, KeyEventKind, KeyModifiers}; use ratatui::Terminal; use ratatui::backend::{CrosstermBackend, TestBackend}; use ratatui::layout::{Constraint, Layout}; -use ratatui::style::{Color, Modifier, Style}; +use ratatui::style::{Color, Modifier}; use ratatui::text::{Line, Span}; use ratatui::widgets::{Block, List, ListItem, ListState, Paragraph, Wrap}; +use crate::terminal_theme::{selection, tone}; use crate::update_ui::{InteractiveUpdateError, NativeTerminalLifecycle, with_restored_terminal}; -const ACCENT: Color = Color::Rgb(103, 232, 249); -const SUCCESS: Color = Color::Rgb(134, 239, 172); -const MUTED: Color = Color::Rgb(148, 163, 184); -const COMMAND: Color = Color::Rgb(253, 224, 71); -const SELECTED: Color = Color::Rgb(22, 78, 99); - -fn tone(color: bool, foreground: Color) -> Style { - if color { - Style::default().fg(foreground) - } else { - Style::default() - } -} +const ACCENT: Color = Color::Cyan; +const SUCCESS: Color = Color::Green; +const MUTED: Color = Color::DarkGray; +const COMMAND: Color = Color::Cyan; #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum UpgradeChoice { @@ -143,6 +135,10 @@ pub fn ask( } _ => continue, }; + let area = terminal.size().map_err(terminal_error)?; + if (area.width < 20 || area.height < 8) && key != UpgradeKey::Cancel { + continue; + } if let Some(choice) = model.update(key) { return Ok(choice); } @@ -158,6 +154,41 @@ fn view( action: &str, color: bool, ) { + if frame.area().width < 20 || frame.area().height < 8 { + frame.render_widget(Paragraph::new("Resize to 20x8\nEsc continue"), frame.area()); + return; + } + if frame.area().width < 50 || frame.area().height < 12 { + let areas = Layout::vertical([ + Constraint::Length(1), + Constraint::Length(2), + Constraint::Min(3), + Constraint::Length(2), + ]) + .split(frame.area()); + frame.render_widget( + Paragraph::new(format!("Upgrade: {current} -> {latest}")).style(tone(color, ACCENT)), + areas[0], + ); + frame.render_stateful_widget( + List::new(["Upgrade now", "Not now", "Don't remind me for this version"]) + .highlight_symbol("> ") + .highlight_style(selection()), + areas[2], + &mut ListState::default().with_selected(Some(model.cursor)), + ); + frame.render_widget( + Paragraph::new(action) + .wrap(Wrap { trim: true }) + .style(tone(color, COMMAND)), + areas[1], + ); + frame.render_widget( + Paragraph::new("↑/↓ choose Enter confirm\nEsc continue q/Ctrl+C continue"), + areas[3], + ); + return; + } let areas = Layout::vertical([ Constraint::Length(7), Constraint::Length(3), @@ -197,14 +228,7 @@ fn view( ListItem::new("Don't remind me for this version"), ]) .highlight_symbol("> ") - .highlight_style(if color { - Style::default() - .fg(ACCENT) - .bg(SELECTED) - .add_modifier(Modifier::BOLD) - } else { - Style::default().add_modifier(Modifier::BOLD) - }); + .highlight_style(selection()); frame.render_stateful_widget( options, areas[1], @@ -267,6 +291,12 @@ fn snapshot_text(terminal: &Terminal, color: bool) -> String { line.push_str("\x1b[0m"); line.push_str(&ansi_color(cell.fg, false)); line.push_str(&ansi_color(cell.bg, true)); + if cell.modifier.contains(Modifier::DIM) { + line.push_str("\x1b[2m"); + } + if cell.modifier.contains(Modifier::REVERSED) { + line.push_str("\x1b[7m"); + } if cell.modifier.contains(Modifier::BOLD) { line.push_str("\x1b[1m"); } diff --git a/crates/skilld-native/tests/doctor_ui.rs b/crates/skilld-native/tests/doctor_ui.rs new file mode 100644 index 00000000..2c8d4540 --- /dev/null +++ b/crates/skilld-native/tests/doctor_ui.rs @@ -0,0 +1,409 @@ +use skilld_command::ResolvedTarget; +use skilld_command::doctor::{DoctorOptions, ScanContext, ScanPhase, ScanProgress, scan}; +use skilld_command::doctor_actions::ActionPreview; +use skilld_core::AgentTargetId; +use skilld_native::doctor_ui::{Browse, Model, PathFilter, Screen, WorkState, render_snapshot}; +use std::fs; + +#[test] +fn loading_screen_explains_work_instead_of_showing_an_empty_result() { + let snapshot = render_snapshot(&Model::default(), 100, 26); + assert!(snapshot.contains("Finding Skill files")); + assert!(snapshot.contains("q cancel")); + assert!(!snapshot.contains("No Skills in this group")); +} + +#[test] +fn loading_progress_shows_scope_counts_elapsed_and_respects_no_color() { + let mut model = Model { + roots: vec!["/home/test".into()], + elapsed: std::time::Duration::from_secs(12), + progress: Some(ScanProgress { + phase: ScanPhase::Fingerprint { + completed: 3, + total: 12, + }, + current_path: "/home/test/.claude/skills/example".into(), + visited_directories: 128, + skipped_directories: 7, + found_skills: 12, + problems: 0, + }), + ..Model::default() + }; + let snapshot = render_snapshot(&model, 100, 26); + for text in [ + "Scan: /home/test", + "Checking Skill contents", + "3 / 12 Skills", + "12s elapsed", + "128 directories scanned", + "7 excluded", + ] { + assert!(snapshot.contains(text), "{text}"); + } + let mut terminal = ratatui::Terminal::new(ratatui::backend::TestBackend::new(100, 26)).unwrap(); + terminal + .draw(|f| skilld_native::doctor_ui::view(f, &model)) + .unwrap(); + assert!( + terminal + .backend() + .buffer() + .content + .iter() + .any(|c| c.fg == ratatui::style::Color::Cyan) + ); + model.color = false; + terminal + .draw(|f| skilld_native::doctor_ui::view(f, &model)) + .unwrap(); + assert!( + terminal + .backend() + .buffer() + .content + .iter() + .all(|c| c.fg == ratatui::style::Color::Reset && c.bg == ratatui::style::Color::Reset) + ); +} + +#[test] +fn doctor_groups_skills_sh_and_previews_removal_without_changing_files() { + let root = tempfile::tempdir().unwrap(); + let path = root.path().join(".agents/skills/example"); + fs::create_dir_all(&path).unwrap(); + fs::write( + path.join("SKILL.md"), + "---\nname: example\ndescription: Read files.\n---\nRead the project.", + ) + .unwrap(); + fs::write(root.path().join(".agents/.skill-lock.json"),r#"{"version":3,"skills":{"example":{"source":"owner/repository","sourceType":"github","skillFolderHash":""}}}"#).unwrap(); + let context = ScanContext { + home: root.path().into(), + global_store: root.path().join(".skilld/skills"), + global_targets: vec![ + ResolvedTarget::new(AgentTargetId::Codex, root.path().join(".agents/skills")).unwrap(), + ], + skills_sh_global_lock: root.path().join(".agents/.skill-lock.json"), + }; + let report = scan(&DoctorOptions::for_root(root.path()), &context).unwrap(); + let mut model = Model { + report: Some(report), + work: WorkState::Ready, + ..Model::default() + }; + let overview = render_snapshot(&model, 110, 26); + assert!(overview.contains("Review skills.sh migration")); + assert!(!model.can_review_action(110, 26)); + model.open_group(); + assert!(render_snapshot(&model, 110, 26).contains("Global Agent targets")); + model.open_group(); + let snapshot = render_snapshot(&model, 110, 26); + assert!(snapshot.contains("example")); + assert!(snapshot.contains("m migrate")); + assert!(snapshot.contains("d remove")); + let owner_line = snapshot + .lines() + .find(|line| line.contains("Owner: skills.sh")) + .unwrap(); + assert!(!owner_line.contains("example")); + model.filter = "missing".into(); + assert!(model.visible().is_empty()); + assert!(render_snapshot(&model, 110, 26).contains("No Skills match")); + model.filter = "example".into(); + assert_eq!(model.visible().len(), 1); + model.preview = Some(ActionPreview { + action: "Remove".into(), + name: "example".into(), + paths: vec![path.clone()], + source: None, + commit: None, + changes_content: false, + behaviors: vec![], + }); + let review = render_snapshot(&model, 110, 26); + assert!(review.contains("Review action")); + assert!(review.contains("esc cancel")); + assert!(path.join("SKILL.md").exists()); + let narrow = render_snapshot(&model, 40, 12); + assert!(narrow.contains("Resize")); +} + +#[test] +fn applying_never_promises_cancellation_and_filter_keeps_error_visible() { + let mut model = Model { + work: WorkState::Applying, + filter: "example".into(), + message: "Could not read affected files".into(), + ..Model::default() + }; + let output = render_snapshot(&model, 80, 24); + assert!(output.contains("Wait for the result")); + assert!(!output.contains("q cancel")); + assert!(output.contains("Could not read affected files")); + model.work = WorkState::Ready; + let output = render_snapshot(&model, 80, 24); + assert!(output.contains("Could not read affected files")); + assert!(output.contains("Filter: example")); +} + +#[test] +fn narrow_review_uses_full_content_area_and_scroll_cannot_hide_all_content() { + let model = Model { + work: WorkState::Ready, + preview: Some(ActionPreview { + action: "Remove".into(), + name: "example".into(), + paths: vec!["/home/test/.agents/skills/example".into()], + source: None, + commit: None, + changes_content: false, + behaviors: vec![], + }), + ..Model::default() + }; + let output = render_snapshot(&model, 80, 24); + assert!(output.contains("Affected Agent targets:")); + assert!(output.contains("/home/test/.agents/skills/example")); + assert!(output.contains("Enter applies")); + let model = Model { + detail_scroll: u16::MAX, + ..model + }; + assert!(render_snapshot(&model, 80, 24).contains("Enter applies")); +} + +#[test] +fn help_is_readable_and_focus_survives_color_removal() { + let model = Model { + work: WorkState::Ready, + screen: Screen::Help, + color: false, + ..Model::default() + }; + let output = render_snapshot(&model, 80, 24); + assert!(output.contains("> Help")); + assert!(output.contains("Ctrl-C")); + assert!(output.contains("Esc: back")); +} + +#[test] +fn actions_are_unavailable_while_working_filtering_or_outside_skill_view() { + let mut model = Model { + work: WorkState::Ready, + ..Model::default() + }; + assert!(!model.can_review_action(80, 24)); + assert!(!model.can_review_action(59, 24)); + assert!(!model.can_review_action(80, 17)); + for work in [ + WorkState::Scanning, + WorkState::Preparing, + WorkState::Applying, + ] { + model.work = work; + assert!(!model.can_review_action(80, 24)); + } + model.work = WorkState::Ready; + model.screen = Screen::Help; + assert!(!model.can_review_action(80, 24)); + model.screen = Screen::Skills; + model.editing_filter = true; + assert!(!model.can_review_action(80, 24)); +} + +#[test] +fn problems_show_paths_and_reasons_inside_the_terminal() { + let model = Model { + work: WorkState::Ready, + screen: Screen::Problems, + report: Some(skilld_command::doctor::DoctorReport { + roots: vec![], + excludes: vec![], + skills: vec![], + claude_files: vec![], + problems: vec![skilld_command::doctor::ScanProblem { + path: "/home/test/restricted".into(), + message: "Permission denied".into(), + }], + visited_directories: 1, + skipped_directories: 0, + }), + ..Model::default() + }; + let output = render_snapshot(&model, 80, 24); + assert!(output.contains("/home/test/restricted")); + assert!(output.contains("Permission denied")); + assert!(output.contains("esc back")); +} + +#[test] +fn scrolling_at_the_end_can_immediately_return_to_previous_content() { + let mut model = Model { + work: WorkState::Ready, + screen: Screen::Help, + ..Model::default() + }; + let before = render_snapshot(&model, 60, 18); + for _ in 0..1000 { + model.move_selection(true); + } + let end = render_snapshot(&model, 60, 18); + assert_ne!(before, end); + model.move_selection(false); + assert_ne!(render_snapshot(&model, 60, 18), end); +} + +#[test] +fn shrinking_during_application_keeps_wait_instructions_truthful() { + let model = Model { + work: WorkState::Applying, + ..Model::default() + }; + let output = render_snapshot(&model, 50, 12); + assert!(output.contains("Wait for the result")); + assert!(!output.contains("q quit")); +} + +#[test] +fn full_notice_keeps_long_errors_and_recovery_paths_reachable() { + let mut model = Model { + work: WorkState::Ready, + screen: Screen::Notice, + message: format!( + "Removed: example. Backup: /home/test/{}recovery.json", + "long-directory/".repeat(80) + ), + ..Model::default() + }; + assert!(render_snapshot(&model, 80, 24).contains("Removed: example")); + for _ in 0..1000 { + model.move_selection(true); + } + assert!(render_snapshot(&model, 80, 24).contains("recovery.json")); +} + +fn scan_fixture(root: &std::path::Path) -> skilld_command::doctor::DoctorReport { + scan( + &DoctorOptions::for_root(root), + &ScanContext { + home: root.into(), + global_store: root.join(".skilld/skills"), + global_targets: vec![], + skills_sh_global_lock: root.join(".agents/.skill-lock.json"), + }, + ) + .unwrap() +} + +#[test] +fn recommendations_reduce_large_results_and_require_drilldown_before_actions() { + let root = tempfile::tempdir().unwrap(); + for project in ["first", "second"] { + for index in 0..40 { + let path = root + .path() + .join(format!("{project}/.claude/skills/skill-{index}")); + fs::create_dir_all(&path).unwrap(); + fs::write( + path.join("SKILL.md"), + format!("# Skill {index}\nRead {project}.\n"), + ) + .unwrap(); + } + } + let mut model = Model { + report: Some(scan_fixture(root.path())), + work: WorkState::Ready, + ..Model::default() + }; + assert_eq!(model.visible().len(), 80); + assert_eq!(model.entries().len(), 1); + assert!(render_snapshot(&model, 100, 26).contains("Inspect unknown installs")); + assert!(!model.can_review_action(100, 26)); + model.open_group(); + assert_eq!(model.entries().len(), 2); + assert!(!model.can_review_action(100, 26)); + model.move_selection(true); + model.open_group(); + assert_eq!(model.entries().len(), 40); + assert!(model.can_review_action(100, 26)); + let selected = model.selected_skill().unwrap(); + assert!( + model.report.as_ref().unwrap().skills[selected] + .canonical_path + .starts_with(root.path().join("second")) + ); + model.back(); + assert_eq!(model.entries().len(), 2); + model.back(); + assert_eq!(model.entries().len(), 1); +} + +#[cfg(unix)] +#[test] +fn symlink_filter_keeps_alias_scope_and_duplicate_details_name_real_copies() { + let root = tempfile::tempdir().unwrap(); + let source = root.path().join("catalog/example"); + let copy = root.path().join("project/.claude/skills/copied"); + let alias = root.path().join("project/.claude/skills/alias-only-name"); + for path in [&source, ©] { + fs::create_dir_all(path).unwrap(); + fs::write(path.join("SKILL.md"), "# Example\nRead files.\n").unwrap(); + } + std::os::unix::fs::symlink(&source, &alias).unwrap(); + let mut model = Model { + report: Some(scan_fixture(root.path())), + work: WorkState::Ready, + ..Model::default() + }; + assert_eq!(model.visible().len(), 2); + model.toggle_group(); + assert_eq!(model.path_filter, PathFilter::Symlinks); + assert_eq!(model.visible().len(), 1); + model.filter = "alias-only-name".into(); + assert_eq!(model.visible().len(), 1); + model.open_group(); + model.open_group(); + let snapshot = render_snapshot(&model, 180, 60); + assert!(snapshot.contains("Symlinks:")); + assert!(snapshot.contains("alias-only-name")); + assert!(snapshot.contains(&format!("-> {}", source.display()))); + assert!(snapshot.contains("Duplicate: 1 identical directory copies.")); + assert!(snapshot.contains(©.display().to_string())); + model.filter.clear(); + model.toggle_group(); + assert_eq!(model.path_filter, PathFilter::Directories); + assert_eq!(model.visible().len(), 2); + assert_eq!(model.browse, Browse::Owners); +} + +#[cfg(unix)] +#[test] +fn linked_parent_paths_are_in_the_symlink_view_and_explain_the_parent_link() { + let root = tempfile::tempdir().unwrap(); + let source = root.path().join("catalog/example"); + fs::create_dir_all(&source).unwrap(); + fs::write(source.join("SKILL.md"), "# Example\nRead files.\n").unwrap(); + let agent = root.path().join("project/.claude"); + fs::create_dir_all(&agent).unwrap(); + std::os::unix::fs::symlink(root.path().join("catalog"), agent.join("skills")).unwrap(); + let mut model = Model { + report: Some(scan_fixture(root.path())), + work: WorkState::Ready, + ..Model::default() + }; + model.toggle_group(); + assert_eq!(model.visible().len(), 1); + model.open_group(); + model.open_group(); + let snapshot = render_snapshot(&model, 180, 60); + assert!(snapshot.contains("Via linked folder:")); + assert!(snapshot.contains(&agent.join("skills").display().to_string())); + assert!(snapshot.contains(&format!("-> {}", root.path().join("catalog").display()))); + model.toggle_group(); + model.open_group(); + let directories = render_snapshot(&model, 180, 60); + assert!(!directories.contains("project/.claude")); +} diff --git a/crates/skilld-native/tests/select_ui.rs b/crates/skilld-native/tests/select_ui.rs index 307b5c48..fdfde6ad 100644 --- a/crates/skilld-native/tests/select_ui.rs +++ b/crates/skilld-native/tests/select_ui.rs @@ -123,7 +123,7 @@ fn the_picker_aligns_names_and_cuts_a_description_that_does_not_fit() { " \u{25cb} pinia-skilld Intuitive, type safe and flexible Store for\u{2026}", "", "", - "", + "↑/↓ move j/k move q cancel Ctrl+C cancel", "space choose a all / filter enter install esc cancel", ] .join("\n") @@ -226,9 +226,38 @@ fn a_filter_that_matches_nothing_says_so() { "No Skill matches zz", "", "", - "", + "↑/↓ move Ctrl+C cancel", "type to filter enter keep it esc clear", ] .join("\n") ); } + +#[test] +fn a_long_skill_name_leaves_a_visible_cut_marker() { + let model = named(&["a-very-long-skill-name-that-does-not-fit-the-terminal"]); + let frame = render_snapshot(&model, 24, 7, false); + assert!(frame.lines().nth(2).unwrap().contains('…'), "{frame}"); +} + +#[test] +fn an_empty_listing_is_distinct_from_a_filter_without_matches() { + let model = named(&[]); + let frame = render_snapshot(&model, 60, 7, false); + assert!(frame.contains("No Skills in this ref."), "{frame}"); + assert!(!frame.contains("enter install"), "{frame}"); +} + +#[test] +fn an_unusable_picker_explains_how_to_recover() { + let frame = render_snapshot(&model(), 12, 3, false); + assert!(frame.contains("Resize"), "{frame}"); +} + +#[test] +fn quitting_a_filter_cancels_the_whole_picker() { + let mut model = model(); + typing(&mut model, "alpha"); + model.update(PickerKey::Quit); + assert_eq!(model.outcome(), Some(&PickerOutcome::Cancelled)); +} diff --git a/crates/skilld-native/tests/update_ui.rs b/crates/skilld-native/tests/update_ui.rs index 54505816..3a33e9f7 100644 --- a/crates/skilld-native/tests/update_ui.rs +++ b/crates/skilld-native/tests/update_ui.rs @@ -567,3 +567,31 @@ fn command_host_reports_one_atomic_failure_for_every_selected_skill() { .collect::>() ); } + +#[test] +fn a_hidden_update_form_cannot_apply_selected_skills() { + let model = browsing_model(12, 3); + let transition = update(model, Message::Key(KeyInput::Apply)); + assert!(transition.effects.is_empty()); + assert!(render_snapshot(&transition.model, false).contains("Resize")); +} + +#[test] +fn cancelling_a_hidden_update_form_still_exits() { + let model = browsing_model(12, 3); + let transition = update(model, Message::Key(KeyInput::Cancel)); + assert!(matches!(transition.effects.as_slice(), [Effect::Exit(_)])); +} + +#[test] +fn resizing_restores_visible_update_controls() { + let model = browsing_model(12, 3); + let transition = update( + model, + Message::Resized { + width: 80, + height: 24, + }, + ); + assert!(render_snapshot(&transition.model, false).contains("enter update")); +} diff --git a/crates/skilld-native/tests/upgrade_flow.rs b/crates/skilld-native/tests/upgrade_flow.rs index a4f3f5c4..187af4ae 100644 --- a/crates/skilld-native/tests/upgrade_flow.rs +++ b/crates/skilld-native/tests/upgrade_flow.rs @@ -143,7 +143,7 @@ fn restarting_with_a_cached_release_prompts_and_runs_the_package_manager() { let fixture = Fixture::new(0); let (success, output) = fixture.terminal(&["list", "--global"], Some(b"\r"), None); assert!(success, "{output}"); - assert!(output.contains("\x1b[38;2;"), "{output}"); + assert!(output.contains("\x1b[38;"), "{output}"); assert!( output.contains("Upgrade complete. Run your command again."), "{output}" diff --git a/crates/skilld-native/tests/upgrade_ui.rs b/crates/skilld-native/tests/upgrade_ui.rs index fb41ae30..02066479 100644 --- a/crates/skilld-native/tests/upgrade_ui.rs +++ b/crates/skilld-native/tests/upgrade_ui.rs @@ -67,8 +67,20 @@ fn color_output_resets_styles_and_plain_banners_have_no_escape_codes() { ); let banner = render_banner("3.6.7", "3.7.0", "Restart skilld to upgrade.", 80, true); println!("COLOR_PREVIEW_START\n{prompt}\n\n{banner}\nCOLOR_PREVIEW_END"); + assert!( + prompt.contains("\x1b[7m"), + "The selection must reverse inherited colors." + ); + assert!( + prompt.contains("\x1b[2m"), + "Secondary text must dim the inherited foreground." + ); for output in [&prompt, &banner] { - assert!(output.contains("\x1b[38;2;"), "{output}"); + assert!( + !output.contains(";2;"), + "RGB overrides the terminal palette: {output}" + ); + assert!(output.contains("\x1b[38;5;6m"), "{output}"); assert!(output.lines().all(|line| line.ends_with("\x1b[0m"))); } let plain = render_banner("3.6.7", "3.7.0", "Restart skilld to upgrade.", 40, false); @@ -92,3 +104,18 @@ fn the_prompt_fits_a_narrow_terminal() { "{output}" ); } + +#[test] +fn a_short_prompt_keeps_the_action_and_consent_visible() { + let output = render_snapshot("3.6.7", "3.7.0", "Run skilld upgrade", 40, 8, false); + assert!(output.contains("Run skilld upgrade"), "{output}"); + assert!(output.contains("Upgrade now"), "{output}"); + assert!(output.contains("Enter"), "{output}"); + assert!(output.contains("Esc"), "{output}"); +} + +#[test] +fn an_unusable_prompt_does_not_hide_consent_behind_a_partial_form() { + let output = render_snapshot("3.6.7", "3.7.0", "Run skilld upgrade", 12, 3, false); + assert!(output.contains("Resize"), "{output}"); +} diff --git a/crates/skilld-ui/src/screen.rs b/crates/skilld-ui/src/screen.rs index 70b5dd7c..48836879 100644 --- a/crates/skilld-ui/src/screen.rs +++ b/crates/skilld-ui/src/screen.rs @@ -4,7 +4,7 @@ //! humans. Plain mode prints the text untouched for machines; Human mode adds //! glyphs, theme roles, aligned fields, and optional hyperlinks. -use crate::text::{pad_to, width}; +use crate::text::{pad_to, sanitize, width}; use crate::theme::{Role, paint}; /// The success glyph prefixing completed work. @@ -60,6 +60,8 @@ pub struct Screen { pub header: Option, /// The output body. pub lines: Vec, + /// Human-only guidance when this screen has no records. + pub empty_hint: Option, } impl Screen { @@ -68,6 +70,7 @@ impl Screen { Self { header: None, lines, + empty_hint: None, } } @@ -76,9 +79,16 @@ impl Screen { Self { header: Some(header.into()), lines, + empty_hint: None, } } + /// Explain an empty result without adding records to Plain output. + pub fn with_empty_hint(mut self, hint: impl Into) -> Self { + self.empty_hint = Some(hint.into()); + self + } + /// The machine rendering: one exact line per record, heading excluded. pub fn render_plain(&self) -> String { let mut output = String::new(); @@ -91,14 +101,28 @@ impl Screen { /// The terminal rendering with glyphs, theme roles, and aligned fields. pub fn render_human(&self, color: bool) -> String { + self.render_human_width(color, u16::MAX) + } + + /// Fit prose and metadata to the terminal. Commands stay on one line, + /// so copying a command never adds a newline inside a quoted argument. + pub fn render_human_width(&self, color: bool, columns: u16) -> String { + let columns = usize::from(columns).max(20); let mut output = String::new(); if let Some(header) = &self.header { - output.push_str(&paint(header, Role::Brand, color)); + output.push_str(&wrapped(header, "", "", columns, Role::Brand, color)); output.push('\n'); if !self.lines.is_empty() { output.push('\n'); } } + if self.lines.is_empty() + && let Some(hint) = &self.empty_hint + { + output.push('\n'); + output.push_str(&wrapped(hint, "", "", columns, Role::Dim, color)); + output.push('\n'); + } let label_width = self .lines .iter() @@ -106,8 +130,14 @@ impl Screen { .map(width) .max() .unwrap_or(0); - for line in &self.lines { - output.push_str(&line.render_human(color, label_width)); + for (index, line) in self.lines.iter().enumerate() { + if index > 0 + && matches!(line.kind, LineKind::Record { .. } | LineKind::Group { .. }) + && !output.ends_with("\n\n") + { + output.push('\n'); + } + output.push_str(&line.render_human_width(color, label_width, columns)); output.push('\n'); } output @@ -147,6 +177,8 @@ pub enum LineKind { value: String, /// A terminal hyperlink target for the value, used when color is on. url: Option, + /// Commands keep their copyable shape; other values wrap. + command: bool, }, /// One titled row with indented detail rows underneath. The Plain text /// may span several sentences joined by newlines; Human renders the @@ -296,6 +328,7 @@ impl Line { label: label.into(), value: value.into(), url: None, + command: false, }, } } @@ -314,6 +347,22 @@ impl Line { label, value, url: Some(url.into()), + command: false, + }, + } + } + + /// A copyable command shown beside its label in Human mode. + pub fn command_field(label: impl Into, value: impl Into) -> Self { + let label = label.into(); + let value = value.into(); + Self { + plain: format!("{label}: {value}"), + kind: LineKind::Field { + label, + value, + url: None, + command: true, }, } } @@ -369,21 +418,62 @@ impl Line { } } + #[cfg(test)] fn render_human(&self, color: bool, label_width: usize) -> String { + self.render_human_width(color, label_width, usize::from(u16::MAX)) + } + + fn render_human_width(&self, color: bool, label_width: usize, columns: usize) -> String { match &self.kind { - LineKind::Plain => self.plain.clone(), - LineKind::Item => paint(&self.plain, Role::Emphasis, color), - LineKind::Success => glyphed(GLYPH_SUCCESS, Role::Success, &self.plain, color), - LineKind::Warn => glyphed(GLYPH_WARN, Role::Warn, &self.plain, color), - LineKind::Error => glyphed(GLYPH_ERROR, Role::Error, &self.plain, color), - LineKind::Hint => paint(&self.plain, Role::Dim, color), - LineKind::Field { label, value, url } => { - let label = pad_to(label, label_width); - let value = match url { - Some(url) if color => hyperlink(value, url), - _ => value.clone(), - }; - format!("{}: {value}", paint(&label, Role::Dim, color)) + LineKind::Plain => wrap(&sanitize(&self.plain), columns).join("\n"), + LineKind::Item => wrapped(&self.plain, "", "", columns, Role::Emphasis, color), + LineKind::Success => glyphed(GLYPH_SUCCESS, Role::Success, &self.plain, color, columns), + LineKind::Warn => glyphed(GLYPH_WARN, Role::Warn, &self.plain, color, columns), + LineKind::Error => glyphed(GLYPH_ERROR, Role::Error, &self.plain, color, columns), + LineKind::Hint => wrapped(&self.plain, "", "", columns, Role::Dim, color), + LineKind::Field { + label, + value, + url, + command, + } => { + let label = sanitize(label); + let value = sanitize(value); + if *command { + return format!( + "{}: {}", + paint(&pad_to(&label, label_width), Role::Dim, color), + crate::paint_command(&value, color) + ); + } + let prefix = format!("{}: ", pad_to(&label, label_width)); + let stacked = width(&prefix) + 12 > columns; + let indent = if stacked { 2 } else { width(&prefix) }; + let mut lines = Vec::new(); + if stacked { + lines.push(paint(&format!("{label}:"), Role::Dim, color)); + } + for (index, line) in wrap(&value, columns.saturating_sub(indent)) + .iter() + .enumerate() + { + let start = if index == 0 && !stacked { + format!( + "{}: ", + paint(&pad_to(&label, label_width), Role::Dim, color) + ) + } else { + " ".repeat(indent) + }; + let value = match url { + Some(url) if color => { + hyperlink(&paint(line, Role::Accent, color), &sanitize(url)) + } + _ => line.clone(), + }; + lines.push(format!("{start}{value}")); + } + lines.join("\n") } LineKind::Record { marker, @@ -391,9 +481,31 @@ impl Line { status, details, } => { - let mut output = format!("{} {title}", marker.paint_glyph(color)); + let mut output = wrapped( + title, + &format!("{} ", marker.paint_glyph(color)), + " ", + columns.saturating_sub(2), + Role::Emphasis, + color, + ); if let Some(status) = status { - output.push_str(&format!(" {}", paint(status, Role::Dim, color))); + if width(title) + width(status) + 4 <= columns { + output.push_str(&format!( + " {}", + paint(&sanitize(status), marker.role(), color) + )); + } else { + output.push('\n'); + output.push_str(&wrapped( + status, + " ", + " ", + columns.saturating_sub(2), + marker.role(), + color, + )); + } } let label_width = details .iter() @@ -402,13 +514,29 @@ impl Line { .unwrap_or(0); for detail in details { let label = pad_to(detail.label, label_width); + let raw = sanitize(&detail.value); let value = match detail.kind { - DetailKind::Plain => detail.value.clone(), - DetailKind::Command => crate::spans::paint_command(&detail.value, color), - DetailKind::Path => paint(&detail.value, Role::Dim, color), + DetailKind::Plain => raw.clone(), + DetailKind::Command => crate::spans::paint_command(&raw, color), + DetailKind::Path => paint(&raw, Role::Dim, color), }; output.push('\n'); - output.push_str(&format!(" {label} {value}")); + let prefix = format!(" {label} "); + if detail.kind == DetailKind::Command || width(&prefix) + width(&raw) <= columns + { + output.push_str(&format!("{}{value}", paint(&prefix, Role::Dim, color))); + } else { + output.push_str(&paint(&format!(" {}", detail.label), Role::Dim, color)); + output.push('\n'); + let body = wrap(&raw, columns.saturating_sub(4)); + output.push_str( + &body + .iter() + .map(|line| format!(" {line}")) + .collect::>() + .join("\n"), + ); + } } output } @@ -417,14 +545,47 @@ impl Line { heading, items, } => { - let mut output = format!("{} {heading}", marker.paint_glyph(color)); + let mut output = wrapped( + heading, + &format!("{} ", marker.paint_glyph(color)), + " ", + columns.saturating_sub(2), + Role::Emphasis, + color, + ); let name_width = items.iter().map(|(name, _)| width(name)).max().unwrap_or(0); for (name, meta) in items { output.push('\n'); - let name = pad_to(name, name_width); - output.push_str(&format!(" {name}")); + let name = pad_to(&sanitize(name), name_width.min(columns.saturating_sub(4))); + if width(&name) + 2 <= columns { + output.push_str(&format!(" {}", paint(&name, Role::Emphasis, color))); + } else { + output.push_str(&wrapped( + &name, + " ", + " ", + columns.saturating_sub(2), + Role::Emphasis, + color, + )); + } if !meta.is_empty() { - output.push_str(&format!(" {}", paint(meta, Role::Dim, color))); + if width(&name) + width(meta) + 4 <= columns { + output.push_str(&format!( + " {}", + paint(&sanitize(meta), Role::Dim, color) + )); + } else { + output.push('\n'); + output.push_str(&wrapped( + meta, + " ", + " ", + columns.saturating_sub(4), + Role::Dim, + color, + )); + } } } output @@ -433,8 +594,43 @@ impl Line { } } -fn glyphed(glyph: &str, role: Role, text: &str, color: bool) -> String { - format!("{} {text}", paint(glyph, role, color)) +fn glyphed(glyph: &str, role: Role, text: &str, color: bool, columns: usize) -> String { + let lines = wrap(&sanitize(text), columns.saturating_sub(2)); + lines + .iter() + .enumerate() + .map(|(index, line)| { + if index == 0 { + format!("{} {line}", paint(glyph, role, color)) + } else { + format!(" {line}") + } + }) + .collect::>() + .join("\n") +} + +fn wrapped(text: &str, first: &str, rest: &str, columns: usize, role: Role, color: bool) -> String { + wrap(&sanitize(text), columns) + .iter() + .enumerate() + .map(|(index, line)| { + format!( + "{}{}", + if index == 0 { first } else { rest }, + paint(line, role, color) + ) + }) + .collect::>() + .join("\n") +} + +fn wrap(value: &str, columns: usize) -> Vec { + if width(value) <= columns { + vec![value.to_owned()] + } else { + crate::text::wrap(value, columns) + } } /// Wrap `value` in an OSC 8 terminal hyperlink. @@ -452,6 +648,69 @@ pub(crate) fn has_hyperlink(value: &str) -> bool { mod tests { use super::{Detail, Line, Marker, Screen, has_hyperlink}; + #[test] + fn narrow_screens_wrap_metadata_without_losing_paths_or_actions() { + let path = "/home/harlan/projects/very-long-project/.agents/skills/example"; + let command = "skilld install 'owner/my skill/example' --global"; + let screen = Screen::with_header( + "Installed Skills", + vec![ + Line::field("Source status", "unverified"), + Line::field("Path", path), + Line::record( + Marker::Warn, + "review example", + "example", + Some("unverified source".to_owned()), + vec![ + Detail::path("files", path), + Detail::command("install", command), + ], + ), + ], + ); + let rendered = screen.render_human_width(false, 24); + assert!( + rendered + .lines() + .filter(|line| !line.contains(command)) + .all(|line| crate::text::width(line) <= 24) + ); + assert!(rendered.contains(command)); + assert!(rendered.contains("unverified")); + assert!(rendered.replace(char::is_whitespace, "").contains(path)); + assert!(screen.render_plain().contains(path)); + } + + #[test] + fn empty_screens_explain_next_steps_only_to_humans() { + let screen = Screen::with_header("Installed Skills · project", Vec::new()) + .with_empty_hint("No Skills installed here. Find Skills with skilld search ."); + assert_eq!(screen.render_plain(), ""); + let human = screen.render_human_width(false, 40); + assert!(human.contains("No Skills installed here.")); + assert!(human.contains("skilld search ")); + assert!(!human.contains('\u{1b}')); + } + + #[test] + fn human_values_cannot_emit_terminal_controls() { + let screen = Screen::with_header( + "Skills\u{1b}[2J", + vec![ + Line::field("Path", "a\u{1b}[2Jb"), + Line::record( + Marker::Note, + "plain", + "title\u{1b}[2J", + Some("state\u{1b}[2J".to_owned()), + vec![Detail::path("files", "c\u{1b}[2J")], + ), + ], + ); + assert!(!screen.render_human_width(false, 80).contains('\u{1b}')); + } + #[test] fn plain_rendering_matches_the_machine_contract() { let screen = Screen::new(vec![ diff --git a/crates/skilld-ui/src/spans.rs b/crates/skilld-ui/src/spans.rs index 1ed9673a..bd2ff4a5 100644 --- a/crates/skilld-ui/src/spans.rs +++ b/crates/skilld-ui/src/spans.rs @@ -80,7 +80,32 @@ fn span_text(span: &Span) -> &str { /// Paint a command string in one step. pub fn paint_command(command: &str, color: bool) -> String { - paint_spans(&command_spans(command), color) + if !color { + return command.to_owned(); + } + let spans = command_spans(command); + let mut spans = spans.iter(); + let mut output = String::new(); + let mut cursor = 0; + while cursor < command.len() { + let rest = &command[cursor..]; + let whitespace = rest.starts_with(char::is_whitespace); + let end = rest + .char_indices() + .find(|(_, character)| character.is_whitespace() != whitespace) + .map_or(rest.len(), |(index, _)| index); + let text = &rest[..end]; + if whitespace { + output.push_str(text); + } else { + match spans.next() { + Some(Span::Styled(_, role)) => output.push_str(&paint(text, *role, true)), + _ => output.push_str(text), + } + } + cursor += end; + } + output } #[cfg(test)] @@ -116,6 +141,10 @@ mod tests { paint_command("skilld install foo --agent codex", false), "skilld install foo --agent codex" ); + assert_eq!( + paint_command("skilld install 'my skill' --agent codex", false), + "skilld install 'my skill' --agent codex" + ); } #[test] diff --git a/crates/skilld-ui/src/theme.rs b/crates/skilld-ui/src/theme.rs index 02a847b5..0d334a64 100644 --- a/crates/skilld-ui/src/theme.rs +++ b/crates/skilld-ui/src/theme.rs @@ -13,6 +13,8 @@ pub enum Role { Brand, /// Bold. Emphasis inside body text. Emphasis, + /// Bright blue. Informational values and links, without a status implication. + Accent, /// Green. Completed work. Success, /// Yellow. Attention: outdated or degraded results. @@ -29,6 +31,7 @@ impl Role { match self { Self::Brand => "\u{1b}[1m\u{1b}[36m", Self::Emphasis => "\u{1b}[1m", + Self::Accent => "\u{1b}[94m", Self::Success => "\u{1b}[32m", Self::Warn => "\u{1b}[33m", Self::Error => "\u{1b}[31m", diff --git a/packages/harness/test/unit/skills.test.ts b/packages/harness/test/unit/skills.test.ts index 399b3aa0..2a6af1df 100644 --- a/packages/harness/test/unit/skills.test.ts +++ b/packages/harness/test/unit/skills.test.ts @@ -70,6 +70,7 @@ describe('skilld-maintained Skills', () => { 'generate-project-skill', 'review-skill', 'skilld', + 'skilld-tui-design', ]) }) }) diff --git a/skills/skilld-maintained-skills.json b/skills/skilld-maintained-skills.json index ebfd664c..8d4c2f1c 100644 --- a/skills/skilld-maintained-skills.json +++ b/skills/skilld-maintained-skills.json @@ -3,5 +3,6 @@ "update-package-skill", "generate-project-skill", "review-skill", - "skilld" + "skilld", + "skilld-tui-design" ] diff --git a/skills/skilld-tui-design/SKILL.md b/skills/skilld-tui-design/SKILL.md new file mode 100644 index 00000000..056fa95d --- /dev/null +++ b/skills/skilld-tui-design/SKILL.md @@ -0,0 +1,100 @@ +--- +name: skilld-tui-design +description: Designs and verifies skilld CLI output, terminal pickers, and full-screen flows. Use for terminal colours, loading feedback, keyboard controls, layout, errors, or end-to-end CLI UX reviews. +license: MIT +--- + +# Design skilld CLI interactions + +Build the user's whole command journey, including failure, cancellation, and the return to their shell. +Read [source decisions](references/source-review.md) for the two reviewed Skills and their exact revisions. + +## Establish the contract + +Read the project's `GLOSSARY.md`, `COPY.md`, and `VISION.md` when present. +Keep canonical nouns and strings. Use short sentences with one idea each. +Inspect the command parser, output renderer, and terminal lifecycle before changing behavior. +Classify each affected surface as static output, a picker, a prompt, or a full-screen view. +Include command help, parser errors, startup notices, and progress indicators in the inventory. +List its starting state, successful outcome, expected failures, and cancellation behavior. +Keep the existing JSON envelope, streams, and exit codes unless the task changes that contract. + +Use the existing shared renderer and semantic palette. +Avoid adding a second framework or per-screen theme. +Start with `crates/skilld-ui`, `crates/skilld-command/src/output.rs`, and `crates/skilld-native/src`. +Inspect command help and routing in `crates/skilld-command/src/lib.rs`. + +## Put the decision first + +Static output starts with the requested result or the failed action. +Group repeated records consistently. Place supporting metadata below each record. +Use spacing and headings before adding boxes. +Keep commands copyable. Preserve whitespace inside quoted arguments. +Wrap prose and paths by terminal cells. Preserve the original value in machine output. +Give empty results a useful next action based on available commands. + +Pickers show the question, query, matching count, selected item, and valid keys. +Full-screen views keep scope and current state visible. +For large inventories, start with recommendations and counts. Open projects or folders before listing individual Skills. +Use words for findings. Label identical copies as duplicates and show their paths; separate symlinks from directory copies. +State when a filter changes only the view. An action review must still list every affected target. +Spend rows on the user's decision. Do not reserve space for decorative panels. +If a pane cannot show complete content, provide reachable scrolling or a detail view. +At smaller sizes, simplify the layout before hiding required information. +If the screen is unusable, block hidden actions and show the required size. + +## Use colour with independent cues + +Inherit the terminal's foreground and background for body text. +Share roles for brand, action, secondary information, success, warning, error, focus, and selection. +Use one action accent across screens. Reserve status colours for actual status. +Show ownership as a label, without implying safety or success. +Pair colour with words, markers, bold text, or inverse selection. +Make pane focus visible when colour is disabled. +Honor `NO_COLOR`, `TERM=dumb`, and existing output capability detection. +Never claim contrast across unknown terminal themes from one screenshot. + +## Make state and input agree + +Use explicit states for loading, browsing, editing, review, applying, completion, and failure. +Prefer tagged states carrying their required data over overlapping pending flags. +Keep filesystem and network work outside the input loop. +If operations can overlap, identify their results so stale work cannot replace the current view. +Derive available actions and footer hints from the same state rules. +Keep query editing separate from global shortcuts. Typing `q` in a query must remain possible. +Make Escape predictable: cancel editing or review before leaving the whole screen. +Preserve selection by stable identity when refreshing data. +Bound scrolling and expose position when the remaining content is otherwise unclear. +Provide discoverable help when the current footer cannot explain all controls. + +## Explain waiting and outcomes + +Show the operation, scope, elapsed time, and available cancellation action. +When the total is unknown, show activity and observed counts without a percentage. +When the total is known, show completed and total work. +Keep previous results readable during refresh when they remain relevant. Label them as previous results if refresh fails. +Keep errors visible until the user changes state or dismisses them. +Do not replace an error with a filter label or a generic loading message. +Describe partial completion explicitly. Preserve recovery and backup paths. + +Before a destructive change, show the exact action, item, paths, and content replacement. +Require deliberate confirmation of that specific plan. +Discard stale activation keys before entering consent states. Preserve cancellation requests. +If writes cannot stop safely, defer exit and say so. Never display a cancellation key that does nothing. +Restore the terminal on success, failure, and cancellation. +After leaving the alternate screen, print a durable summary of completed changes and recovery paths. +Provenance describes source evidence. It never proves that instructions are safe. + +## Verify the whole journey + +Run focused behavior tests for changed transitions and regressions. +Use the real renderer for layout tests. Do not test source text or private implementation shape. +Inspect actual terminal captures, including colour and monochrome. +Cover normal and narrow sizes, short screens, Unicode names, long paths, and resize during confirmation. +Exercise loading, empty results, errors, filtering, review, cancellation, completion, and return to the shell. +Use disposable fixtures for migration, removal, and restoration. +Check plain and JSON output separately for escape sequences, stream changes, and exit codes. +Run the repository's required checks after integration. + +Report what changed and what was exercised. Identify any live path that remains untested. +Do not treat passing snapshots as proof of keyboard behavior or terminal restoration. diff --git a/skills/skilld-tui-design/references/source-review.md b/skills/skilld-tui-design/references/source-review.md new file mode 100644 index 00000000..04f4c137 --- /dev/null +++ b/skills/skilld-tui-design/references/source-review.md @@ -0,0 +1,54 @@ +# Source decisions + +This Skill contains original guidance for skilld CLI work. +Two upstream Skills informed the design. Their application scaffolds and scripts were not incorporated. + +## tui-design + +Author: Diego Marino. +Repository: `diegomarino/tui-design`. +Revision: `57c199f22dbf04f057c279431d45346cc1fd5dcd`. +[Reviewed SKILL.md](https://github.com/diegomarino/tui-design/blob/57c199f22dbf04f057c279431d45346cc1fd5dcd/skills/tui-design/SKILL.md). +License: MIT, declared in the reviewed Skill frontmatter. + +Adopted ideas: + +- Classify static output, pickers, and full-screen interactions separately. +- Use semantic colours with independent selection and status cues. +- Design around terminal cells and inspect actual rendered evidence. +- Preserve selection and distinguish unknown totals from measured progress. +- Treat plain output and terminal restoration as product behavior. + +Excluded prescriptions: + +- Mandatory concept counts, large token schemas, and subjective craft scores. +- Python mock tooling and copied starter applications. +- Forced dark themes or universal shortcut sets. +- Mandatory NDJSON, additive-only schemas, and errors-last ordering. + +These prescriptions conflict with skilld's existing contracts or add work without improving the user's decision. + +## tui-development + +Author: NVIDIA. +Repository: `NVIDIA/OpenShell`. +Revision: `fb8f6c0885127fe4ca29993739131a3bdbaed477`. +[Reviewed SKILL.md](https://github.com/NVIDIA/OpenShell/blob/fb8f6c0885127fe4ca29993739131a3bdbaed477/.agents/skills/tui-development/SKILL.md). +License: [Apache-2.0 at the reviewed revision](https://github.com/NVIDIA/OpenShell/blob/fb8f6c0885127fe4ca29993739131a3bdbaed477/LICENSE). + +Adopted ideas: + +- Keep long operations outside the input loop. +- Represent lifecycle states explicitly and route background results through events. +- Make focus, valid actions, scope, and outcomes visible. +- Review file changes before applying them. + +Excluded prescriptions: + +- OpenShell branding, topology, deployment commands, and splash screens. +- Mandatory Tokio, unbounded channels, and fixed network timeouts. +- Independent pending flags and forced interruption during writes. + +The synthesis uses skilld's current host boundaries, shared rendering, and terminal lifecycle. +It adds durable shell summaries and actual terminal journey verification beyond the upstream build guidance. +It introduces no deployment, privilege, or installation requirements. diff --git a/skills/skilld/SKILL.md b/skills/skilld/SKILL.md index 67d8c89e..69673f7d 100644 --- a/skills/skilld/SKILL.md +++ b/skills/skilld/SKILL.md @@ -16,6 +16,7 @@ Use `--json` with `search`, `run`, and `update --check`. Use `--json` with `view` of a registry ref, and with every registry and account command below. Their `data` is the skilld.dev answer. A command whose answer has no body returns `data: null`. Use `--json` with `sync` and `sync --check` for declared Skills. +Use `doctor --json` for read-only Skill discovery and cleanup findings. The remaining commands do not support JSON output. Use `--plain` when another command needs stable text. @@ -421,6 +422,26 @@ Use `outdated` for stale, unverified, local, and unmanaged Skill reports. Read every proposed command before using it. Never delete an unmanaged Skill unless the user names it for removal. +## Review Skill files across projects + +Run `skilld doctor --json` to scan the user's home directory. +Pass explicit roots to narrow the scan or inspect files outside home. +The default scan excludes dependencies, caches, generated files, backups, and Git worktrees. +Use `--exclude GLOB` to prune more paths. +Use `--include-excluded` or `--include-worktrees` only when those files matter to the task. + +Read owner labels, duplicate paths, source matches, and scan problems separately. +Exit code `1` with success data means some files could not be checked. +Use `--check-sources` for up to 20 source candidates through skilld.dev. +A name match alone never establishes provenance. + +The interactive `skilld doctor` separates skills.sh installs with Tab. +The user can review migration with `m` or removal with `d`. +Each action shows affected paths and requires confirmation. +Backups include original files, lock metadata, and recovery instructions. +Use the existing update workflow for skilld installs. +Historical commit searches and project-to-global deduplication are outside Doctor's current actions. + ## Manage account authentication Check account authentication before starting login: diff --git a/tests/loader/native-packages.test.mjs b/tests/loader/native-packages.test.mjs index 23a2ecf0..1b5c0ca4 100644 --- a/tests/loader/native-packages.test.mjs +++ b/tests/loader/native-packages.test.mjs @@ -1,6 +1,6 @@ import assert from 'node:assert/strict' import { execFile } from 'node:child_process' -import { chmod, mkdir, mkdtemp, readdir, rm, writeFile } from 'node:fs/promises' +import { chmod, cp, mkdir, mkdtemp, readdir, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { promisify } from 'node:util' @@ -76,12 +76,16 @@ it('accepts the executable from a packed platform package', async () => { const spec = nativePackageSpecs.find(candidate => candidate.directory === 'cli-linux-x64-gnu') await writeNativePackage(root, spec) await writeFile(join(root, spec.directory, 'LICENSE'), 'MIT\n') + const staging = join(root, 'stage') + await mkdir(join(staging, 'package'), { recursive: true }) + await cp(join(root, spec.directory), join(staging, 'package'), { recursive: true }) await mkdir(output) - await execFileAsync('npm', [ - 'pack', - join(root, spec.directory), - '--pack-destination', - output, + await execFileAsync('tar', [ + '-czf', + join(output, `${spec.packageName}-3.0.0-test.0.tgz`), + '-C', + staging, + 'package', ]) const filename = (await readdir(output)).find(path => path.endsWith('.tgz')) assert.ok(filename)