feat: skill registry MCP — skill_search + skill_load - #92
Merged
Conversation
…n and shadow pairing
…meout, persistent registry, shadow est_tokens - db.rs: rebuild() uses INSERT OR IGNORE so one duplicate (name, source) no longer rolls back the whole rebuild and disables skill_search/skill_load; skips the fts mirror insert when a row is ignored (tx.changes() == 0). - db.rs: open_db() sets a 5s busy_timeout and WAL journal mode on the shared skills.db to avoid SQLITE_BUSY collisions across concurrent agentflare processes. - mcp_server.rs: AgentflareMcp now owns a persisted, mutex-guarded Registry instead of opening a fresh one per call, so ensure_fresh()'s 60s debounce actually applies instead of doing a full rescan + rebuild on every call. - sources.rs: shadow pairing copies the shadow's est_tokens onto the kept origin entry, since skill_load serves the shadow body by default.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds a new ChangesSkill Registry Feature
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/skill-registry/src/sources.rs (1)
86-91: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the provenance regex instead of recompiling per call.
shadow_originis invoked once for everyclaude-userentry during each scan, andRegex::newrecompiles the same static pattern each time. Hoist it into aLazyLock(oronce_cell) so compilation happens once.♻️ Proposed change
+use std::sync::LazyLock; + +static SHADOW_RE: LazyLock<regex::Regex> = + LazyLock::new(|| regex::Regex::new(r"compressed-from:\s+(.+?)\s+\d+B").unwrap()); + /// `<!-- compressed-from: <path> <N>B → <M>B, <date> -->` in the first 2000 chars. fn shadow_origin(path: &Path) -> Option<PathBuf> { let text = std::fs::read_to_string(path).ok()?; let head: String = text.chars().take(2000).collect(); - let re = regex::Regex::new(r"compressed-from:\s+(.+?)\s+\d+B").unwrap(); - re.captures(&head).map(|c| PathBuf::from(c[1].to_string())) + SHADOW_RE.captures(&head).map(|c| PathBuf::from(c[1].to_string())) }🤖 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 `@crates/skill-registry/src/sources.rs` around lines 86 - 91, The shadow_origin function is recompiling the same regex on every call, which adds unnecessary overhead during scans. Hoist the compressed-from pattern into a static cached regex using LazyLock or once_cell, then reuse that compiled value inside shadow_origin instead of calling Regex::new each time.src/mcp_server.rs (1)
422-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
skill_loadtest should use an isolated registry path.AgentflareMcp::default()still reacheswith_fresh_registryhere, so this test opens/creates the sharedskills.dband refreshes it on disk. Inject the DB path so the test can use a temp file instead of mutating shared state.🤖 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/mcp_server.rs` around lines 422 - 432, The `skill_load_unknown_name_reports_not_found_with_search_hint` test is still using `AgentflareMcp::default()`, which points at the shared registry and can mutate `skills.db` during the run. Update the test to construct `AgentflareMcp` with an injected temporary DB path instead of relying on `with_fresh_registry`, so `skill_load` and related registry access use isolated state. Use the existing `AgentflareMcp`/registry initialization path to wire in the temp file and keep the assertion on the `skill_search` hint unchanged.
🤖 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.
Nitpick comments:
In `@crates/skill-registry/src/sources.rs`:
- Around line 86-91: The shadow_origin function is recompiling the same regex on
every call, which adds unnecessary overhead during scans. Hoist the
compressed-from pattern into a static cached regex using LazyLock or once_cell,
then reuse that compiled value inside shadow_origin instead of calling
Regex::new each time.
In `@src/mcp_server.rs`:
- Around line 422-432: The
`skill_load_unknown_name_reports_not_found_with_search_hint` test is still using
`AgentflareMcp::default()`, which points at the shared registry and can mutate
`skills.db` during the run. Update the test to construct `AgentflareMcp` with an
injected temporary DB path instead of relying on `with_fresh_registry`, so
`skill_load` and related registry access use isolated state. Use the existing
`AgentflareMcp`/registry initialization path to wire in the temp file and keep
the assertion on the `skill_search` hint unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f29cbf0-7696-44a0-a634-9aae638d42fa
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlcrates/skill-registry/Cargo.tomlcrates/skill-registry/src/db.rscrates/skill-registry/src/frontmatter.rscrates/skill-registry/src/lib.rscrates/skill-registry/src/load.rscrates/skill-registry/src/search.rscrates/skill-registry/src/sources.rscrates/skill-registry/tests/golden_queries.rssrc/mcp_server.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
New
crates/skill-registry(MCP-free) + two MCP tools on the existing agentflare server, letting any agent search the machine's installed skills and load one on demand — compressed shadow copies served by default.source:namequalified names (last-colon split), ambiguity errors list candidates, unknown names hint at skill_search.<!-- compressed-from: ... -->provenance markers; est_tokens reflects the body actually served.skills.db(data_local_dir), transactional full-replace rebuild, WAL + busy_timeout for concurrent agents, dup-tolerant inserts.Testing
Notes
Deferred nice-to-haves: path normalization in shadow-marker matching, 2000-char marker scan window, poisoned-lock recovery.
Summary by CodeRabbit