Add git-backed builtin skill registry - #41
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @trotsky1997, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36904a87a9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let output = std::process::Command::new("git") | ||
| .arg("-C") | ||
| .arg(root) | ||
| .arg("rev-parse") | ||
| .arg("HEAD") |
There was a problem hiding this comment.
When skills.registry.path points at a normal subdirectory of some other Git worktree, git -C <path> rev-parse HEAD still succeeds and returns the enclosing repository's commit. That means Ra will load a directory that is not itself a git-backed skill registry and attach misleading provenance, despite the config contract saying unversioned registries are skipped; verify the configured root is the Git toplevel/gitdir before accepting the revision.
Useful? React with 👍 / 👎.
| .to_string_lossy() | ||
| .into_owned(), | ||
| ]; | ||
| load_skills_with_source(&patterns, SkillSource::Registry, Some(revision)) |
There was a problem hiding this comment.
For a dirty registry checkout, this stamps every skill with the current HEAD but then reads SKILL.md files directly from the working tree. If a skill is uncommitted, modified, or untracked, Ra exposes content that cannot be reproduced from the recorded revision, which defeats the new provenance guarantee; either reject dirty registries or read the skill blobs from the recorded commit.
Useful? React with 👍 / 👎.
| let path = materialize_builtin_skill(name, raw) | ||
| .unwrap_or_else(|| PathBuf::from(format!("<builtin>/skills/{name}/SKILL.md"))); |
There was a problem hiding this comment.
If the cache directory is unavailable or unwritable, this fallback still loads the builtin skill but advertises a pseudo path like <builtin>/skills/review/SKILL.md in the model-facing catalog. The read tool only reads filesystem paths, so in cache-restricted environments the model cannot follow the prompt's instruction to load the full builtin skill before acting; keep a readable materialized path or suppress the catalog entry when materialization fails.
Useful? React with 👍 / 👎.
| fn skills_args_have_agent(args: &[String]) -> bool { | ||
| let mut iter = args.iter(); | ||
| while let Some(arg) = iter.next() { | ||
| if arg == "-a" || arg == "--agent" || arg == "--all" || arg.starts_with("--agent=") { |
There was a problem hiding this comment.
When users run ra skills add <repo> --all, --all selects all skills from the package, not an agent target, so skipping the injected --agent codex means whole-collection installs fall back to the upstream CLI's default or interactive agent choice instead of Ra's discovered Codex/project path. Only treat actual agent-target flags as satisfying this check.
Useful? React with 👍 / 👎.
1 participant
Summary
~/.codex/skills/**/SKILL.mdra skills ...passthrough tonpx --yes skills@1.5.9, defaultingaddto--agent codexunless an agent/all option is presentTesting
cargo fmt --checkgit diff --checknode -e "JSON.parse(require('fs').readFileSync('spec/ra-config.schema.json','utf8'))"Could not run local cargo tests or regenerate schema locally because Cargo repeatedly stalled fetching the optional
opentui_rustgit dependency.timeout 180 cargo test --test skills_discoverexited with 124 while updating that dependency.