Conversation
Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: HEAD
📝 WalkthroughWalkthroughThe skill registry now accepts detected agent identifiers through a callback, caches them during refreshes, and uses them to rebuild default sources. Shared detection wiring updates skill discovery, hooks, and MCP tool refresh paths. ChangesSkill registry refresh flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
… just at Registry construction open_default's caller-supplied detected_agents was only computed once at construction time. For mcp_server.rs's process-lifetime-cached Registry, that meant a newly installed agent CLI's skill sources would never be scanned until the whole MCP server process restarted -- before this decoupling, detect_all() ran fresh inside every debounced refresh(). Restore that by having ensure_fresh() take a lazy detect_agents closure, invoked only when a rescan actually happens. Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: task/61 Agentflare-Item: 61
# Conflicts: # Cargo.lock Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: task/61 Agentflare-Item: 61
Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: task/61 Agentflare-Item: 61
Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: task/61 Agentflare-Item: 61
# Conflicts: # Cargo.lock Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: task/61 Agentflare-Item: 61
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components.rs`:
- Around line 200-204: Update the documentation comment above the agent-ID
callback to reference Registry::ensure_fresh, not Registry::open_default, and
describe the callback according to the ensure_fresh contract. Preserve the
explanation that callers provide detected agent IDs and use a throwaway cache
because cross-call version caching is unnecessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d6618c1-0772-4761-8677-823a2ba664d3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/skill-registry/Cargo.tomlcrates/skill-registry/src/load.rssrc/components.rssrc/hook.rssrc/mcp_server.rs
💤 Files with no reviewable changes (1)
- crates/skill-registry/Cargo.toml
| /// Agent IDs detected on this machine, for `skill_registry::Registry::open_default`'s | ||
| /// `detected_agents` param. skill-registry itself has no `agent-registry` dependency | ||
| /// (deliberately decoupled — skill discovery only needs agent IDs, not the version- | ||
| /// detection machinery); every call site collects them the same way, using a | ||
| /// throwaway cache since none of these callers need cross-call version caching. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the detected-agent callback documentation.
The comment describes a detected_agents parameter on Registry::open_default, but the supplied implementation has no such parameter; the callback is passed to Registry::ensure_fresh.
Based on the supplied crates/skill-registry/src/load.rs contract.
Proposed documentation fix
-/// Agent IDs detected on this machine, for `skill_registry::Registry::open_default`'s
-/// `detected_agents` param.
+/// Agent IDs detected on this machine for `skill_registry::Registry::ensure_fresh`'s
+/// `detect_agents` callback.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Agent IDs detected on this machine, for `skill_registry::Registry::open_default`'s | |
| /// `detected_agents` param. skill-registry itself has no `agent-registry` dependency | |
| /// (deliberately decoupled — skill discovery only needs agent IDs, not the version- | |
| /// detection machinery); every call site collects them the same way, using a | |
| /// throwaway cache since none of these callers need cross-call version caching. | |
| /// Agent IDs detected on this machine for `skill_registry::Registry::ensure_fresh`'s | |
| /// `detect_agents` callback. skill-registry itself has no `agent-registry` dependency | |
| /// (deliberately decoupled — skill discovery only needs agent IDs, not the version- | |
| /// detection machinery); every call site collects them the same way, using a | |
| /// throwaway cache since none of these callers need cross-call version caching. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components.rs` around lines 200 - 204, Update the documentation comment
above the agent-ID callback to reference Registry::ensure_fresh, not
Registry::open_default, and describe the callback according to the ensure_fresh
contract. Preserve the explanation that callers provide detected agent IDs and
use a throwaway cache because cross-call version caching is unnecessary.
Summary
skill_registry::Registry::ensure_fresh()calledagent_registry::detect_all()directly, makingagent-registrya hard dependency ofskill-registryjust to get a list of installed agent IDs for source discovery.Registry::open_default()now takesdetected_agents: Vec<String>from the caller instead of detecting them itself.skill-registry'sCargo.tomlno longer depends onagent-registry.Registry::open_default()call sites (src/hook.rs's SessionStart/PromptSubmit skill-preload paths) since this branch forked. Rather than duplicate the "detect installed agents" boilerplate a 3rd/4th time, factored it into one sharedcrate::components::detected_skill_agents()helper used by all 4 call sites.Not the full scope of item #61 (that item vendors a whole skills.rs-style registry/search engine, still in progress under opencode) — this is a small, independent decoupling refactor that happened to branch off the same numbered item.
Test plan
cargo build --workspace --all-featurescleancargo build --features skill-overrides-syncclean (the one feature-gated call site)cargo test --workspace— 727 passed, 0 failedSummary by CodeRabbit