diff --git a/docs/prd-builtin-skill-registry.md b/docs/prd-builtin-skill-registry.md new file mode 100644 index 0000000..ae6ce43 --- /dev/null +++ b/docs/prd-builtin-skill-registry.md @@ -0,0 +1,101 @@ +# PRD: Builtin Skill Registry And npx Skills Adapter + +## Overview / Problem Statement + +Ra can already discover and invoke project and personal `SKILL.md` files, but it does not yet have a first-class lifecycle for Ra-shipped builtin skills. It also does not expose the package-management workflow users expect from `npx skills`: adding, listing, finding, removing, and updating skills from remote or local sources. + +Ra should not reimplement the `npx skills` ecosystem manager. Instead, Ra should provide a small adapter that forwards skill-management commands to `npx skills`, while keeping Ra's runtime registry responsible for resolving which installed, configured, and builtin skills are active for a session. + +## Goals & Success Metrics + +- Ra ships a deterministic builtin skill registry that can be loaded without network access. +- Ra can load a local clean git checkout as a skill registry and records its HEAD revision as skill provenance. +- Project and global installed skills can shadow Ra builtin skills by command name. +- Users can manage external skills through `ra skills ...` with behavior aligned to `npx skills`. +- Project-level installs default to a path Ra already discovers. +- Global installs are discoverable by Ra after installation. +- Tests cover registry precedence, builtin disable/include/exclude behavior, and `npx skills` command forwarding argument construction. + +## User Personas & Stories + +- As a Ra user, I want useful builtin skills available on a fresh install without manually copying skill files. +- As a project maintainer, I want committed project skills to override generic builtin guidance for the same workflow. +- As a user of the open skills ecosystem, I want to run `ra skills add vercel-labs/agent-skills` and get the same installation semantics as `npx skills`. +- As an operator, I want to disable builtin skills without deleting or modifying Ra-shipped files. + +## Functional Requirements + +| Priority | Requirement | +| --- | --- | +| Must | Add a builtin skill registry loaded from Ra-shipped skill definitions. | +| Must | Treat builtin skills as virtual or embedded source entries, not as installed user files. | +| Must | Resolve active skills with deterministic precedence: explicit config paths, project skills, global skills, local registry skills, then builtin skills. | +| Must | Allow project or global skills to shadow builtin skills with the same slash command name. | +| Must | Expose only the resolved active winner in the model-facing catalog and slash-command map. | +| Must | Add config controls to enable/disable builtin skills and include/exclude builtin names. | +| Must | Support an optional local registry path that must be the root of a git checkout and is skipped if it is not versioned by git. | +| Must | Record the registry HEAD commit for each skill loaded from the local registry. | +| Must | Skip registry loading when the registry checkout has uncommitted or untracked changes, so HEAD provenance describes the loaded content. | +| Must | Preserve progressive disclosure: builtin skill bodies must not be dumped into the startup system prompt. | +| Must | Support `ra skills add/list/find/remove/update/init` by forwarding to `npx skills`. | +| Must | When forwarding `ra skills add` without an explicit `--agent`, default to `--agent codex` so installs land in `.agents/skills/` for project scope. | +| Must | Add global Codex discovery for `~/.codex/skills/**/SKILL.md`, matching `npx skills` global Codex install location. | +| Must | Keep explicit user `--agent` arguments intact and do not inject a default agent when the user already supplied one. | +| Should | Pin the forwarded package invocation to a known `skills` npm version or make the version configurable. | +| Should | Expose `ra skills list --builtin` to inspect Ra-shipped builtin skills and shadowing status. | +| Should | Report shadowed builtin skills in diagnostics or JSON list output. | +| Could | Provide `ra skills disable ` as a config-editing helper that writes a builtin exclude entry. | +| Could | Support an environment variable equivalent to `INSTALL_INTERNAL_SKILLS` for internal builtin skills. | +| Won't | Reimplement remote source parsing, package install, update, or removal logic already provided by `npx skills`. | + +## Non-Functional Requirements + +- Keep session startup deterministic and offline-capable. +- Keep installed skill management separate from runtime skill resolution. +- Avoid writing builtin skills into project or home directories unless explicitly requested by a future export command. +- Preserve existing `.ra/skills`, `.agents/skills`, and `.claude/skills` discovery behavior. +- Avoid surprising command rewriting: only add Ra defaults when the user did not specify the corresponding `npx skills` option. + +## Design Considerations + +`npx skills` is the ecosystem manager. It owns source formats, install/update/remove behavior, project/global scopes, agent target paths, and interactive flows. Ra should use it as a subprocess for management commands. + +Ra's runtime registry has two local sources. The builtin source is embedded with Ra and provides fallback skills. The local registry source is a clean git checkout root, analogous to the GitHub repositories used by `npx skills`, so skill versions are managed by commits, branches, tags, and ordinary git operations. Builtin and registry entries should behave like ordinary skills after resolution, including `disable-model-invocation`, `user-invocable`, runtime tool policy, hooks, model overrides, shell context, and fork behavior. + +## Technical Considerations + +The implementation should extend `src/skills.rs` with a `SkillSource` or equivalent provenance field so resolved entries can distinguish explicit, project, global, registry, and builtin sources. Registry-sourced skills should also carry the registry HEAD revision. `ResourceBundle::prompt_map` and `build_system_prompt` should operate on the resolved active list, not every discovered duplicate. + +Builtin definitions can live in a repo directory such as `skills/.system//SKILL.md` and be embedded at compile time with `include_str!`, or be loaded from a packaged runtime directory. Embedding is safer for single-binary installs; materializing to a read-only cache path may be useful so the existing `read` tool can load full builtin skill bodies during progressive disclosure. + +The optional local registry should default to `~/.ra/skill-registry` when the directory exists. It must be the root of a clean git checkout; Ra should not silently treat an unversioned directory, a subdirectory inside a larger repository, or a dirty worktree as a registry. Ra should not fetch, pull, checkout, or mutate the registry during normal startup. Version changes happen through git commands or future explicit registry-management commands. + +The `ra skills` adapter should shell out to: + +```bash +npx --yes skills@ ... +``` + +For `ra skills add`, if no `--agent`/`-a` appears in user arguments, append `--agent codex`. For all other arguments, preserve ordering and values. The adapter should stream stdout/stderr and return the child exit status. + +## Timeline & Milestones + +| Milestone | Owner | Target | +| --- | --- | --- | +| PRD and issue draft | Agent | Before implementation | +| Registry data model, git provenance, and precedence tests | Agent | First implementation PR | +| Builtin config and discovery integration | Agent | First implementation PR | +| `ra skills` passthrough adapter | Agent | Second implementation PR if scope grows | +| Documentation and CLI examples | Agent | Before merge | + +## Open Questions & Risks + +- Whether builtin skill bodies should be embedded only, materialized to cache for `read`, or exposed through a virtual resource reader. +- Whether future registry management should use `git` directly or redirect to a skills ecosystem command when the source is remote. +- Whether `ra skills list` should default to Ra's resolved registry view or `npx skills list` project view. A pragmatic split is `ra skills list` passthrough and `ra skills registry` for Ra's resolved view. +- `npx skills` is an npm dependency at command time. Users without Node/npm need a clear error and may still rely on builtin and manually installed skills. +- A pinned `skills` version improves reproducibility but may lag ecosystem behavior. A config override can handle this without making default behavior unstable. + +## Appendix + +Reference behavior: `skills@1.5.9` (`npx skills`) supports `add`, `remove`, `list`, `find`, `update`, `init`, project/global scopes, `--agent`, `--skill`, `--copy`, `--all`, and Codex install paths of `.agents/skills/` for project scope and `~/.codex/skills/` for global scope. diff --git a/spec/ra-config.schema.json b/spec/ra-config.schema.json index b714fa5..2c3e7d5 100644 --- a/spec/ra-config.schema.json +++ b/spec/ra-config.schema.json @@ -174,6 +174,38 @@ }, "additionalProperties": false }, + "BuiltinSkillsSection": { + "type": "object", + "properties": { + "enabled": { + "description": "Master switch for Ra-shipped builtin skills. Default true.", + "default": true, + "type": "boolean" + }, + "exclude": { + "description": "Builtin command names to suppress. Always wins over `include`.", + "default": [], + "type": "array", + "items": { + "type": "string" + } + }, + "include": { + "description": "Empty = load the curated builtin set. Non-empty = load only these builtin command names before applying `exclude`.", + "default": [], + "type": "array", + "items": { + "type": "string" + } + }, + "include_internal": { + "description": "Include builtins marked as internal. Default false.", + "default": false, + "type": "boolean" + } + }, + "additionalProperties": false + }, "GraphifySection": { "description": "`[graphify]` — native [Graphify](https://github.com/safishamsi/graphify) support. When enabled, Ra targets a project's `graphify-out/graph.json` (walking cwd → git root), treats Graphify as an agent-owned R2A graph workflow, and registers ensure/impact/update plus native graph query tools.", "type": "object", @@ -692,11 +724,54 @@ }, "additionalProperties": false }, + "SkillRegistrySection": { + "type": "object", + "properties": { + "enabled": { + "description": "Enable loading from a local git-backed skill registry. Default true; no registry is loaded unless `path` is set or the default checkout exists.", + "default": true, + "type": "boolean" + }, + "exclude": { + "description": "Registry command names to suppress. Always wins over `include`.", + "default": [], + "type": "array", + "items": { + "type": "string" + } + }, + "include": { + "description": "Empty = load every discovered registry skill. Non-empty = load only these command names before applying `exclude`.", + "default": [], + "type": "array", + "items": { + "type": "string" + } + }, + "path": { + "description": "Local clean git checkout root containing SKILL.md files. If omitted, Ra checks `~/.ra/skill-registry` and skips it when absent.", + "default": null, + "type": [ + "string", + "null" + ] + } + }, + "additionalProperties": false + }, "SkillsSection": { "type": "object", "properties": { + "builtin": { + "description": "Ra-shipped builtin skill controls. Builtins are the lowest-priority source and can be shadowed by explicit, project, global, or registry skills.", + "allOf": [ + { + "$ref": "#/definitions/BuiltinSkillsSection" + } + ] + }, "discover": { - "description": "Auto-discover skills from Ra-native, universal/cross-agent, and Claude Code layouts: `./.ra/skills/`, `~/.ra/skills/`, `./.agents/skills/`, `~/.agents/skills/`, `./.claude/skills/`, and `~/.claude/skills/`. Default true so project and personal skills work without explicit paths.", + "description": "Auto-discover skills from Ra-native, universal/cross-agent, Codex, and Claude Code layouts: `./.ra/skills/`, `~/.ra/skills/`, `./.agents/skills/`, `~/.agents/skills/`, `~/.codex/skills/`, `./.claude/skills/`, and `~/.claude/skills/`. Default true so project and personal skills work without explicit paths.", "default": true, "type": "boolean" }, @@ -712,6 +787,14 @@ "items": { "type": "string" } + }, + "registry": { + "description": "Optional local skill registry checkout. The registry must be a clean git worktree root; Ra records its HEAD revision as skill provenance.", + "allOf": [ + { + "$ref": "#/definitions/SkillRegistrySection" + } + ] } }, "additionalProperties": false diff --git a/spec/ra.toml.example b/spec/ra.toml.example index bac973c..ef2eb38 100644 --- a/spec/ra.toml.example +++ b/spec/ra.toml.example @@ -73,6 +73,7 @@ builtin = [] # ~/.agents/skills/ — cross-agent global layout # ./.claude/skills/ — Claude Code project skills # ~/.claude/skills/ — Claude Code personal skills +# ~/.codex/skills/ — Codex global skills installed by `npx skills` # Other catalog-style layouts (`skills/.curated/`, …) go in `paths` # explicitly. [skills] @@ -81,6 +82,22 @@ discover = true paths = [] # paths = ["./skills/.curated/**/SKILL.md"] # opt-in extra layouts +[skills.registry] +# Optional clean git checkout root for versioned skill catalogs. Ra loads +# SKILL.md files from this checkout and records HEAD as registry provenance. +enabled = true +# path = "~/.ra/skill-registry" +# include = [] # empty = all registry skills +# exclude = [] + +[skills.builtin] +# Ra-shipped fallback skills. Project/global/registry skills with the same +# command name shadow builtins. +enabled = true +# include = [] # empty = curated builtin set +# exclude = [] +# include_internal = false + # ─── Prompt templates → slash commands ────────────────────────────── [prompts] enabled = true diff --git a/src/config.rs b/src/config.rs index e36a7f2..9c197a2 100644 --- a/src/config.rs +++ b/src/config.rs @@ -146,11 +146,11 @@ pub struct SkillsSection { /// Master switch. Default true. #[serde(default = "default_true")] pub enabled: bool, - /// Auto-discover skills from Ra-native, universal/cross-agent, and - /// Claude Code layouts: `./.ra/skills/`, `~/.ra/skills/`, - /// `./.agents/skills/`, `~/.agents/skills/`, `./.claude/skills/`, - /// and `~/.claude/skills/`. Default true so project and personal - /// skills work without explicit paths. + /// Auto-discover skills from Ra-native, universal/cross-agent, Codex, + /// and Claude Code layouts: `./.ra/skills/`, `~/.ra/skills/`, + /// `./.agents/skills/`, `~/.agents/skills/`, `~/.codex/skills/`, + /// `./.claude/skills/`, and `~/.claude/skills/`. Default true so + /// project and personal skills work without explicit paths. #[serde(default = "default_true")] pub discover: bool, /// Extra glob patterns expanded against `~` and the cwd. Each match @@ -158,6 +158,54 @@ pub struct SkillsSection { /// whatever `discover` finds. #[serde(default)] pub paths: Vec, + /// Ra-shipped builtin skill controls. Builtins are the lowest-priority + /// source and can be shadowed by explicit, project, global, or registry + /// skills. + #[serde(default)] + pub builtin: BuiltinSkillsSection, + /// Optional local skill registry checkout. The registry must be a clean git + /// worktree root; Ra records its HEAD revision as skill provenance. + #[serde(default)] + pub registry: SkillRegistrySection, +} + +#[derive(Debug, Clone, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] +pub struct BuiltinSkillsSection { + /// Master switch for Ra-shipped builtin skills. Default true. + #[serde(default = "default_true")] + pub enabled: bool, + /// Empty = load the curated builtin set. Non-empty = load only these + /// builtin command names before applying `exclude`. + #[serde(default)] + pub include: Vec, + /// Builtin command names to suppress. Always wins over `include`. + #[serde(default)] + pub exclude: Vec, + /// Include builtins marked as internal. Default false. + #[serde(default)] + pub include_internal: bool, +} + +#[derive(Debug, Clone, Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] +pub struct SkillRegistrySection { + /// Enable loading from a local git-backed skill registry. Default true; + /// no registry is loaded unless `path` is set or the default checkout + /// exists. + #[serde(default = "default_true")] + pub enabled: bool, + /// Local clean git checkout root containing SKILL.md files. If omitted, Ra + /// checks `~/.ra/skill-registry` and skips it when absent. + #[serde(default)] + pub path: Option, + /// Empty = load every discovered registry skill. Non-empty = load only + /// these command names before applying `exclude`. + #[serde(default)] + pub include: Vec, + /// Registry command names to suppress. Always wins over `include`. + #[serde(default)] + pub exclude: Vec, } impl Default for SkillsSection { @@ -166,6 +214,30 @@ impl Default for SkillsSection { enabled: true, discover: true, paths: Vec::new(), + builtin: BuiltinSkillsSection::default(), + registry: SkillRegistrySection::default(), + } + } +} + +impl Default for BuiltinSkillsSection { + fn default() -> Self { + Self { + enabled: true, + include: Vec::new(), + exclude: Vec::new(), + include_internal: false, + } + } +} + +impl Default for SkillRegistrySection { + fn default() -> Self { + Self { + enabled: true, + path: None, + include: Vec::new(), + exclude: Vec::new(), } } } @@ -741,6 +813,17 @@ builtin = ["read", "bash"] [skills] paths = ["./skills/**/SKILL.md"] +[skills.registry] +path = "~/.ra/skill-registry" +include = ["review"] +exclude = ["internal-only"] + +[skills.builtin] +enabled = true +include = ["review"] +exclude = ["demo"] +include_internal = true + [prompts] paths = ["./prompts/*.md"] @@ -770,6 +853,16 @@ timeout = 2.0 assert_eq!(cfg.models.len(), 1); assert_eq!(cfg.models[0].name, "pi"); assert_eq!(cfg.tools.builtin, vec!["read", "bash"]); + assert_eq!( + cfg.skills.registry.path.as_deref(), + Some("~/.ra/skill-registry") + ); + assert_eq!(cfg.skills.registry.include, vec!["review"]); + assert_eq!(cfg.skills.registry.exclude, vec!["internal-only"]); + assert!(cfg.skills.builtin.enabled); + assert_eq!(cfg.skills.builtin.include, vec!["review"]); + assert_eq!(cfg.skills.builtin.exclude, vec!["demo"]); + assert!(cfg.skills.builtin.include_internal); assert_eq!(cfg.a2a.serve.http_port, Some(3000)); assert_eq!(cfg.a2a.remote_agents.len(), 1); assert_eq!(cfg.mcp.servers.len(), 1); diff --git a/src/init.rs b/src/init.rs index 64223c3..cc6f4c5 100644 --- a/src/init.rs +++ b/src/init.rs @@ -46,11 +46,27 @@ builtin = [] [skills] # `discover = true` loads project skills from `./.ra/skills/**/SKILL.md` # shared cross-agent skills from `./.agents/skills/**/SKILL.md`, and -# Claude Code skills from `./.claude/skills/**/SKILL.md`. +# Claude Code skills from `./.claude/skills/**/SKILL.md`. Global Codex +# skills installed by `npx skills -g -a codex` are loaded from +# `~/.codex/skills/**/SKILL.md`. enabled = true discover = true paths = [] +[skills.registry] +# Optional clean git checkout root for versioned skill catalogs. +enabled = true +# path = "~/.ra/skill-registry" +# include = [] +# exclude = [] + +[skills.builtin] +# Ra-shipped fallback skills. Project/global/registry skills shadow by name. +enabled = true +# include = [] +# exclude = [] +# include_internal = false + [prompts] enabled = true paths = [] diff --git a/src/main.rs b/src/main.rs index dfcb6f9..5a57314 100644 --- a/src/main.rs +++ b/src/main.rs @@ -70,6 +70,12 @@ enum Cmd { }, /// List saved sessions in the current cwd's bucket. Sessions, + /// Manage external skills through the npx skills ecosystem manager. + Skills { + /// Command and arguments forwarded to `npx skills`. + #[arg(trailing_var_arg = true, allow_hyphen_values = true)] + args: Vec, + }, /// Interactive terminal UI. Requires the `tui` cargo feature /// (and a nightly toolchain — opentui_rust uses edition 2024). Tui, @@ -94,6 +100,7 @@ async fn main() -> anyhow::Result<()> { force, example_skill, }) => return run_init(force, example_skill), + Some(Cmd::Skills { args }) => return run_skills_passthrough(args).await, Some(Cmd::Acp) => RuntimeCmd::Acp, Some(Cmd::Run { prompt }) => RuntimeCmd::Run { prompt }, Some(Cmd::Serve { @@ -127,6 +134,110 @@ async fn main() -> anyhow::Result<()> { } } +async fn run_skills_passthrough(args: Vec) -> anyhow::Result<()> { + let mut forwarded = normalize_skills_args(args); + let status = tokio::process::Command::new("npx") + .arg("--yes") + .arg("skills@1.5.9") + .args(forwarded.drain(..)) + .status() + .await + .map_err(|e| anyhow::anyhow!("failed to run `npx skills`: {e}"))?; + if !status.success() { + std::process::exit(status.code().unwrap_or(1)); + } + Ok(()) +} + +fn normalize_skills_args(mut args: Vec) -> Vec { + let Some(command) = args.first().cloned() else { + return vec!["--help".to_string()]; + }; + if (command == "add" || command == "a") && !skills_args_have_agent(&args[1..]) { + args.push("--agent".to_string()); + args.push("codex".to_string()); + } + args +} + +fn skills_args_have_agent(args: &[String]) -> bool { + for arg in args { + if arg == "-a" || arg == "--agent" || arg == "--all" || arg.starts_with("--agent=") { + return true; + } + if arg == "--" { + break; + } + } + false +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn skills_add_defaults_to_codex_agent() { + let args = normalize_skills_args(vec![ + "add".to_string(), + "vercel-labs/agent-skills".to_string(), + ]); + assert_eq!( + args, + vec![ + "add".to_string(), + "vercel-labs/agent-skills".to_string(), + "--agent".to_string(), + "codex".to_string() + ] + ); + } + + #[test] + fn skills_add_preserves_explicit_agent() { + let args = normalize_skills_args(vec![ + "add".to_string(), + "vercel-labs/agent-skills".to_string(), + "--agent".to_string(), + "claude-code".to_string(), + ]); + assert_eq!( + args, + vec![ + "add".to_string(), + "vercel-labs/agent-skills".to_string(), + "--agent".to_string(), + "claude-code".to_string() + ] + ); + } + + #[test] + fn skills_add_all_preserves_npx_all_semantics() { + let args = normalize_skills_args(vec![ + "add".to_string(), + "vercel-labs/agent-skills".to_string(), + "--all".to_string(), + ]); + assert_eq!( + args, + vec![ + "add".to_string(), + "vercel-labs/agent-skills".to_string(), + "--all".to_string() + ] + ); + } + + #[test] + fn skills_no_command_shows_help() { + assert_eq!( + normalize_skills_args(Vec::new()), + vec!["--help".to_string()] + ); + } +} + fn run_init(force: bool, example_skill: bool) -> anyhow::Result<()> { let cwd = std::env::current_dir()?; let report = ra::init::init_project( diff --git a/src/skills.rs b/src/skills.rs index 6701dc2..b9ace3f 100644 --- a/src/skills.rs +++ b/src/skills.rs @@ -26,6 +26,56 @@ use serde::Deserialize; use std::collections::HashMap; use std::path::{Path, PathBuf}; +const BUILTIN_REVIEW_SKILL: &str = r#"--- +name: review +description: Review local code changes for correctness, regressions, and missing tests. +--- + +# Review Skill + +Use this skill when the user asks for a code review or asks you to inspect a change before merging. + +## Instructions + +- Focus on concrete bugs, behavioral regressions, security issues, and missing tests. +- Ground findings in file and line references where possible. +- Put findings before summaries. +- If no issues are found, say that directly and mention any residual test risk. +"#; + +/// Provenance of a skill after discovery. Lower-priority sources can be +/// shadowed by higher-priority sources with the same command name. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SkillSource { + Explicit, + Project, + Global, + Registry, + Builtin, +} + +impl SkillSource { + fn precedence(self) -> u8 { + match self { + SkillSource::Explicit => 0, + SkillSource::Project => 1, + SkillSource::Global => 2, + SkillSource::Registry => 3, + SkillSource::Builtin => 4, + } + } + + pub fn as_str(self) -> &'static str { + match self { + SkillSource::Explicit => "explicit", + SkillSource::Project => "project", + SkillSource::Global => "global", + SkillSource::Registry => "registry", + SkillSource::Builtin => "builtin", + } + } +} + /// One agent skill following Claude Code / agentskills.io conventions. #[derive(Debug, Clone)] pub struct Skill { @@ -53,6 +103,14 @@ pub struct Skill { pub user_invocable: bool, /// Path to the SKILL.md file on disk. pub path: PathBuf, + /// Where this skill came from before registry resolution. + pub source: SkillSource, + /// Optional version provenance for git-backed registry skills. + pub source_revision: Option, + /// Internal skills are hidden from builtin loading unless explicitly + /// requested. External skills still load normally; this flag mainly + /// preserves `npx skills` metadata for Ra-shipped entries. + pub internal: bool, /// The Markdown body **after** the frontmatter. Not loaded into the /// system prompt at startup (progressive disclosure); the LLM reads /// it on demand. @@ -300,15 +358,25 @@ impl ResourceBundle { // ---------- loaders ----------------------------------------------------- pub fn load_skills(patterns: &[String]) -> Vec { + load_skills_with_source(patterns, SkillSource::Global, None) +} + +fn load_skills_with_source( + patterns: &[String], + source: SkillSource, + source_revision: Option, +) -> Vec { expand_globs(patterns, "skills") .into_iter() - .filter_map(|p| match parse_skill(&p) { - Ok(s) => Some(s), - Err(e) => { - eprintln!("[ra::skills] {}: {e:#}", p.display()); - None - } - }) + .filter_map( + |p| match parse_skill_with_source(&p, source, source_revision.clone()) { + Ok(s) => Some(s), + Err(e) => { + eprintln!("[ra::skills] {}: {e:#}", p.display()); + None + } + }, + ) .collect() } @@ -321,14 +389,237 @@ pub fn default_discover_globs() -> Vec { "~/.ra/skills/**/SKILL.md".to_string(), "./.agents/skills/**/SKILL.md".to_string(), "~/.agents/skills/**/SKILL.md".to_string(), + "~/.codex/skills/**/SKILL.md".to_string(), "./.claude/skills/**/SKILL.md".to_string(), "~/.claude/skills/**/SKILL.md".to_string(), ] } +fn project_skill_globs(cwd: &Path) -> Vec { + discover_project_skill_globs(cwd) +} + +fn global_skill_globs() -> Vec { + vec![ + "~/.ra/skills/**/SKILL.md".to_string(), + "~/.agents/skills/**/SKILL.md".to_string(), + "~/.codex/skills/**/SKILL.md".to_string(), + "~/.claude/skills/**/SKILL.md".to_string(), + ] +} + +fn builtin_skill_docs() -> Vec<(&'static str, &'static str)> { + vec![("review", BUILTIN_REVIEW_SKILL)] +} + +fn load_builtin_skills(config: &crate::config::BuiltinSkillsSection) -> Vec { + if !config.enabled { + return Vec::new(); + } + let include = normalized_name_set(&config.include); + let exclude = normalized_name_set(&config.exclude); + builtin_skill_docs() + .into_iter() + .filter_map(|(name, raw)| { + let command = normalize_name(name); + if !include.is_empty() && !include.contains(&command) { + return None; + } + if exclude.contains(&command) { + return None; + } + let path = materialize_builtin_skill(name, raw) + .unwrap_or_else(|| PathBuf::from(format!("/skills/{name}/SKILL.md"))); + match parse_skill_document(raw, &path, SkillSource::Builtin, None) { + Ok(skill) if skill.internal && !config.include_internal => None, + Ok(skill) => Some(skill), + Err(e) => { + eprintln!("[ra::skills] builtin {name}: {e:#}"); + None + } + } + }) + .collect() +} + +fn materialize_builtin_skill(name: &str, raw: &str) -> Option { + let cache_root = dirs::cache_dir()? + .join("ra") + .join("builtin-skills") + .join(env!("CARGO_PKG_VERSION")) + .join(name); + if let Err(e) = std::fs::create_dir_all(&cache_root) { + eprintln!( + "[ra::skills] failed to create builtin skill cache {}: {e}", + cache_root.display() + ); + return None; + } + let path = cache_root.join("SKILL.md"); + if let Err(e) = std::fs::write(&path, raw) { + eprintln!( + "[ra::skills] failed to write builtin skill cache {}: {e}", + path.display() + ); + return None; + } + Some(path) +} + +fn load_registry_skills(config: &crate::config::SkillRegistrySection) -> Vec { + if !config.enabled { + return Vec::new(); + } + let Some(root) = registry_root(config) else { + return Vec::new(); + }; + if !root.exists() { + return Vec::new(); + } + let Some(revision) = git_registry_revision(&root) else { + eprintln!( + "[ra::skills] registry {} is not a clean git checkout; skipping", + root.display() + ); + return Vec::new(); + }; + let include = normalized_name_set(&config.include); + let exclude = normalized_name_set(&config.exclude); + let patterns = vec![ + root.join("skills/**/SKILL.md") + .to_string_lossy() + .into_owned(), + root.join("skills/.curated/**/SKILL.md") + .to_string_lossy() + .into_owned(), + root.join("skills/.experimental/**/SKILL.md") + .to_string_lossy() + .into_owned(), + root.join("skills/.system/**/SKILL.md") + .to_string_lossy() + .into_owned(), + root.join(".agents/skills/**/SKILL.md") + .to_string_lossy() + .into_owned(), + root.join(".claude/skills/**/SKILL.md") + .to_string_lossy() + .into_owned(), + ]; + load_skills_with_source(&patterns, SkillSource::Registry, Some(revision)) + .into_iter() + .filter(|skill| { + let name = normalize_name(&skill.command_name); + (include.is_empty() || include.contains(&name)) && !exclude.contains(&name) + }) + .collect() +} + +fn registry_root(config: &crate::config::SkillRegistrySection) -> Option { + match config + .path + .as_deref() + .map(str::trim) + .filter(|s| !s.is_empty()) + { + Some(path) => Some(PathBuf::from(shellexpand::tilde(path).to_string())), + None => dirs::home_dir().map(|home| home.join(".ra").join("skill-registry")), + } +} + +fn git_registry_revision(root: &Path) -> Option { + if !git_toplevel_matches(root) || !git_worktree_clean(root) { + return None; + } + let output = std::process::Command::new("git") + .arg("-C") + .arg(root) + .arg("rev-parse") + .arg("HEAD") + .output() + .ok()?; + if !output.status.success() { + return None; + } + let revision = String::from_utf8_lossy(&output.stdout).trim().to_string(); + (!revision.is_empty()).then_some(revision) +} + +fn git_toplevel_matches(root: &Path) -> bool { + let output = std::process::Command::new("git") + .arg("-C") + .arg(root) + .arg("rev-parse") + .arg("--show-toplevel") + .output(); + let Ok(output) = output else { + return false; + }; + if !output.status.success() { + return false; + } + let top = String::from_utf8_lossy(&output.stdout).trim().to_string(); + if top.is_empty() { + return false; + } + let Ok(root) = root.canonicalize() else { + return false; + }; + let Ok(top) = PathBuf::from(top).canonicalize() else { + return false; + }; + top == root +} + +fn git_worktree_clean(root: &Path) -> bool { + let output = std::process::Command::new("git") + .arg("-C") + .arg(root) + .arg("status") + .arg("--porcelain") + .arg("--untracked-files=all") + .output(); + let Ok(output) = output else { + return false; + }; + output.status.success() && output.stdout.is_empty() +} + +fn resolve_skills(mut skills: Vec) -> Vec { + skills.sort_by(|a, b| { + a.source + .precedence() + .cmp(&b.source.precedence()) + .then_with(|| a.command_name.cmp(&b.command_name)) + .then_with(|| a.path.cmp(&b.path)) + }); + let mut seen = std::collections::HashSet::new(); + let mut resolved = Vec::new(); + for skill in skills { + let key = normalize_name(&skill.command_name); + if seen.insert(key) { + resolved.push(skill); + } + } + resolved.sort_by(|a, b| a.command_name.cmp(&b.command_name)); + resolved +} + +fn normalized_name_set(values: &[String]) -> std::collections::HashSet { + values.iter().map(|s| normalize_name(s)).collect() +} + +fn normalize_name(value: &str) -> String { + value.trim().to_ascii_lowercase() +} + pub fn discover_project_skill_globs(cwd: &Path) -> Vec { let mut out = Vec::new(); for dir in project_walk_dirs(cwd) { + out.push( + dir.join(".ra/skills/**/SKILL.md") + .to_string_lossy() + .into_owned(), + ); out.push( dir.join(".agents/skills/**/SKILL.md") .to_string_lossy() @@ -437,12 +728,37 @@ struct Frontmatter { #[allow(dead_code)] paths: Option, shell: Option, + metadata: Option, } +#[derive(Debug, Default, Deserialize)] +struct SkillMetadata { + internal: Option, +} + +#[cfg(test)] fn parse_skill(p: &Path) -> Result { let raw = std::fs::read_to_string(p).context("read")?; + parse_skill_document(&raw, p, SkillSource::Global, None) +} + +fn parse_skill_with_source( + p: &Path, + source: SkillSource, + source_revision: Option, +) -> Result { + let raw = std::fs::read_to_string(p).context("read")?; + parse_skill_document(&raw, p, source, source_revision) +} + +fn parse_skill_document( + raw: &str, + p: &Path, + source: SkillSource, + source_revision: Option, +) -> Result { let (fm_raw, body) = - split_frontmatter(&raw).with_context(|| "missing or malformed YAML frontmatter")?; + split_frontmatter(raw).with_context(|| "missing or malformed YAML frontmatter")?; let fm: Frontmatter = serde_yaml::from_str(fm_raw).with_context(|| "parse YAML frontmatter")?; let command_name = skill_command_name(p)?; let name = fm.name.clone().unwrap_or_else(|| command_name.clone()); @@ -470,6 +786,13 @@ fn parse_skill(p: &Path) -> Result { disable_model_invocation: fm.disable_model_invocation.unwrap_or(false), user_invocable: fm.user_invocable.unwrap_or(true), path: p.to_path_buf(), + source, + source_revision, + internal: fm + .metadata + .as_ref() + .and_then(|m| m.internal) + .unwrap_or(false), body: body.to_string(), }) } @@ -732,18 +1055,30 @@ pub fn build_resource_bundle(config: &crate::config::RaConfig, emit_logs: bool) let mut bundle = ResourceBundle::default(); if config.skills.enabled { - let mut all_patterns: Vec = Vec::new(); + let mut skills = Vec::new(); if config.skills.discover { let cwd = std::env::current_dir().unwrap_or_else(|_| ".".into()); - all_patterns.extend(discover_project_skill_globs(&cwd)); - all_patterns.extend(default_discover_globs()); - } - all_patterns.extend(config.skills.paths.iter().cloned()); - if !all_patterns.is_empty() { - bundle.skills = load_skills(&all_patterns); - if !bundle.skills.is_empty() { - log(&format!("[ra] loaded {} skill(s)", bundle.skills.len())); - } + skills.extend(load_skills_with_source( + &project_skill_globs(&cwd), + SkillSource::Project, + None, + )); + skills.extend(load_skills_with_source( + &global_skill_globs(), + SkillSource::Global, + None, + )); + } + skills.extend(load_skills_with_source( + &config.skills.paths, + SkillSource::Explicit, + None, + )); + skills.extend(load_registry_skills(&config.skills.registry)); + skills.extend(load_builtin_skills(&config.skills.builtin)); + bundle.skills = resolve_skills(skills); + if !bundle.skills.is_empty() { + log(&format!("[ra] loaded {} skill(s)", bundle.skills.len())); } } if config.prompts.enabled { diff --git a/tests/skills_discover.rs b/tests/skills_discover.rs index 909dc5b..103a972 100644 --- a/tests/skills_discover.rs +++ b/tests/skills_discover.rs @@ -7,6 +7,7 @@ use ra::{ skills::{build_resource_bundle, default_discover_globs, load_skills, ResourceBundle}, }; use std::fs; +use std::process::Command; use std::sync::{Mutex, OnceLock}; use tempfile::TempDir; @@ -25,6 +26,46 @@ fn write_skill(root: &std::path::Path, slug: &str, body: &str) { .unwrap(); } +fn git_commit_all(dir: &std::path::Path) -> String { + init_git_repo(dir); + Command::new("git") + .args(["add", "."]) + .current_dir(dir) + .status() + .unwrap(); + Command::new("git") + .args(["commit", "-qm", "add skills"]) + .current_dir(dir) + .status() + .unwrap(); + let output = Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(dir) + .output() + .unwrap(); + assert!(output.status.success()); + String::from_utf8(output.stdout).unwrap().trim().to_string() +} + +fn init_git_repo(dir: &std::path::Path) { + Command::new("git") + .arg("init") + .arg("-q") + .current_dir(dir) + .status() + .unwrap(); + Command::new("git") + .args(["config", "user.email", "ra@example.invalid"]) + .current_dir(dir) + .status() + .unwrap(); + Command::new("git") + .args(["config", "user.name", "Ra Test"]) + .current_dir(dir) + .status() + .unwrap(); +} + #[test] fn default_discovery_finds_ra_and_agents_layouts() { let _guard = cwd_lock(); @@ -38,6 +79,10 @@ fn default_discovery_finds_ra_and_agents_layouts() { std::env::set_current_dir(cwd).unwrap(); let globs = default_discover_globs(); + assert!( + globs.iter().any(|glob| glob.contains("~/.codex/skills")), + "global Codex skills installed by npx skills should be discoverable" + ); let skills = load_skills(&globs); let names: Vec<&str> = skills.iter().map(|s| s.command_name.as_str()).collect(); @@ -50,6 +95,165 @@ fn default_discovery_finds_ra_and_agents_layouts() { } } +#[test] +fn build_resource_bundle_loads_builtin_skills_by_default() { + let _guard = cwd_lock(); + let tmp = TempDir::new().unwrap(); + std::env::set_current_dir(tmp.path()).unwrap(); + + let bundle = build_resource_bundle(&RaConfig::default(), false); + let review = bundle + .skills + .iter() + .find(|skill| skill.command_name == "review") + .expect("builtin review skill should load by default"); + + assert_eq!(review.source.as_str(), "builtin"); + assert!(bundle.prompt_map().contains_key("review")); + assert!(bundle + .build_system_prompt() + .expect("skills prompt") + .contains("Review local code changes")); +} + +#[test] +fn project_skill_shadows_builtin_skill() { + let _guard = cwd_lock(); + let tmp = TempDir::new().unwrap(); + let cwd = tmp.path(); + write_skill( + &cwd.join(".agents/skills"), + "review", + "Project review body.", + ); + std::env::set_current_dir(cwd).unwrap(); + + let bundle = build_resource_bundle(&RaConfig::default(), false); + let review = bundle + .skills + .iter() + .find(|skill| skill.command_name == "review") + .expect("review skill should resolve"); + + assert_eq!(review.source.as_str(), "project"); + assert_eq!(review.body.trim(), "Project review body."); + assert_eq!( + bundle + .skills + .iter() + .filter(|skill| skill.command_name == "review") + .count(), + 1, + "only the active winner should be exposed" + ); +} + +#[test] +fn builtin_skill_exclude_suppresses_builtin() { + let _guard = cwd_lock(); + let tmp = TempDir::new().unwrap(); + std::env::set_current_dir(tmp.path()).unwrap(); + + let mut config = RaConfig::default(); + config.skills.builtin.exclude = vec!["review".to_string()]; + let bundle = build_resource_bundle(&config, false); + + assert!( + !bundle + .skills + .iter() + .any(|skill| skill.command_name == "review"), + "excluded builtin skill should not load" + ); +} + +#[test] +fn git_backed_registry_loads_skills_with_revision() { + let _guard = cwd_lock(); + let tmp = TempDir::new().unwrap(); + let cwd = tmp.path().join("work"); + fs::create_dir_all(&cwd).unwrap(); + let registry = tmp.path().join("registry"); + write_skill( + ®istry.join("skills"), + "plan-review", + "Registry skill body.", + ); + let revision = git_commit_all(®istry); + std::env::set_current_dir(&cwd).unwrap(); + + let mut config = RaConfig::default(); + config.skills.builtin.enabled = false; + config.skills.registry.path = Some(registry.to_string_lossy().into_owned()); + let bundle = build_resource_bundle(&config, false); + let skill = bundle + .skills + .iter() + .find(|skill| skill.command_name == "plan-review") + .expect("registry skill should load"); + + assert_eq!(skill.source.as_str(), "registry"); + assert_eq!(skill.source_revision.as_deref(), Some(revision.as_str())); +} + +#[test] +fn registry_path_must_be_git_worktree_root() { + let _guard = cwd_lock(); + let tmp = TempDir::new().unwrap(); + let parent = tmp.path().join("parent"); + let registry = parent.join("registry"); + write_skill(®istry.join("skills"), "nested-registry", "Nested body."); + git_commit_all(&parent); + + let cwd = tmp.path().join("work"); + fs::create_dir_all(&cwd).unwrap(); + std::env::set_current_dir(&cwd).unwrap(); + + let mut config = RaConfig::default(); + config.skills.builtin.enabled = false; + config.skills.registry.path = Some(registry.to_string_lossy().into_owned()); + let bundle = build_resource_bundle(&config, false); + + assert!( + !bundle + .skills + .iter() + .any(|skill| skill.command_name == "nested-registry"), + "a registry subdirectory inside a parent repo must not be treated as its own versioned registry" + ); +} + +#[test] +fn registry_skips_dirty_or_untracked_content() { + let _guard = cwd_lock(); + let tmp = TempDir::new().unwrap(); + let registry = tmp.path().join("registry"); + fs::create_dir_all(®istry).unwrap(); + init_git_repo(®istry); + write_skill( + ®istry.join("skills"), + "uncommitted-registry", + "Uncommitted body.", + ); + + let cwd = tmp.path().join("work"); + fs::create_dir_all(&cwd).unwrap(); + std::env::set_current_dir(&cwd).unwrap(); + + let mut config = RaConfig::default(); + config.skills.builtin.enabled = false; + config.skills.registry.path = Some(registry.to_string_lossy().into_owned()); + let bundle = build_resource_bundle(&config, false); + + assert!( + !bundle + .skills + .iter() + .any(|skill| skill.command_name == "uncommitted-registry"), + "registry skills must come from committed git content so HEAD provenance is meaningful" + ); +} + #[test] fn claude_skill_optional_frontmatter_uses_fallbacks() { let _guard = cwd_lock();