feat(skill-registry): skill routing/management epic — FTS5, negation, bandit ranking, pack/hub lifecycle - #302
Merged
Merged
Conversation
…checks Strips copyright/SPDX headers and blanks out generated files before detect_over_engineering runs, so noise lines never reach the check and generated files are skipped entirely rather than false-positiving on their own boilerplate. Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: item-54-regex-pre-filter
pre_filter stripped noise/copyright/generated lines entirely, shifting line numbers for every finding detect_over_engineering reports after a stripped line. Blank filtered lines instead of removing them so the line count — and every downstream finding's line number — stays aligned with the original source. Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: item-54-regex-pre-filter
…al, fusion/decay/budget, proactive advisory Agentflare-Agent: 1 Agentflare-Branch: epic/skill-routing-management
… layer Agentflare-Agent: 1 Agentflare-Branch: epic/skill-routing-management
- neg_text FTS5 column weight was +2.0 (positive weights only boost
relevance in SQLite bm25(), they cannot penalize) so a skill's own
"do not use for X" clause could still outrank a genuine match for X
when the neg_text field was short and dense. Set weight to 0.0
(verified empirically against both the original and an adversarial
fixture) and added a regression test for the adversarial case.
- run_export / run_hub push used list_all_names() (bare names only)
then split_once('/') to recover source, which never matches --
every exported/pushed skill got source="unknown", and ambiguous
bare-name lookups could abort the whole export. Added
list_all_name_source_pairs() and load via the qualified
"source:name" form instead.
- hub.rs's two closures failed the project's actual CI gate (clippy
--all-features -D warnings: result_large_err on ureq::Error, 272
bytes) though `cargo check` alone missed it. Boxed the error type
and fixed two other epic-scoped clippy lints (identical_op in the
eval harness, single-pattern match in the Export CLI arm,
collapsible-if in skill_proactive.rs). The only remaining clippy
errors are two dead-code functions in daemon_autostart.rs that are
pre-existing on master, unrelated to this epic.
- Documented (not silently hidden) that save_settings() has no
caller yet -- Task 6's snooze/dismiss write path is unbuilt, only
the read side exists.
Task 8 as committed does not touch gateway_registry at all (spec
scoped it there); what shipped is DB integrity-check/repair and hub
HTTP retry, a different and narrower reliability layer. Task 7's
`skill import`/hub push-pull is a JSON-bundle format, not the
git-repo-clone + Levenshtein-dedup + hub-tag design the spec
described. Both should stay open rather than close on these commits.
cargo test --workspace (728 passed), cargo clippy --workspace
--all-features -- -D warnings (clean except the two pre-existing
daemon_autostart.rs items), cargo fmt clean.
Agentflare-Agent: claude-code_2-1-216_agent
Agentflare-Branch: epic/skill-routing-management
…tion length find_skills_budget summed r.description.len() as the token-budget proxy -- a skill with a one-line description but a huge SKILL.md body barely counted against the budget, defeating the whole point of capping by real content cost. Added est_tokens to RankedSkill (threaded through from SkillHit, which already carries it), and changed the budget accumulator to use it. Replaced the old test (which only exercised the same flawed description-length proxy) with one where description length and real cost diverge, so it actually catches this class of regression. cargo test --workspace and clippy --workspace --all-features -D warnings both clean (aside from the two pre-existing daemon_autostart.rs dead-code items on master, unrelated to this epic). Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: epic/skill-routing-management
… broken db.rs params!, bogus beta_sample bandit_alpha/bandit_beta were added to SkillEntry and SkillHit without updating every construction site (load.rs, pack.rs, search.rs tests, cli/skill.rs export/hub-push, registry-fallback SkillHit) — the crate and the main binary's test build did not compile. db.rs::rebuild had an orphaned .as_ref().map(...) chain (syntax error) with SQL placeholder order that didn't match the params! order. beta_sample used a fabricated rejection-sampling formula (denom reused the same log-density twice, plus an unused u2/E) instead of a real algorithm. Replaced with the standard Gamma-ratio construction using the actual Marsaglia-Tsang method for Gamma(shape,1). Verified: cargo build --workspace --all-features and cargo test --workspace both clean (the 2 hook.rs session_start failures seen under full-suite parallel execution are pre-existing global-state test-isolation flakiness, unrelated to this file set — reproduced passing in isolation on both pre- and post-fix trees). Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: epic/skill-routing-management
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ 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 (6)
📝 WalkthroughWalkthroughThe PR expands the skill registry with richer metadata, ranking feedback, bundle transfer, hub synchronization, evaluation, proactive suggestions, token budgets, and MCP activation wrapping. It also adds generated-code filtering to over-engineering detection and ignores transient reference clones. ChangesSkill platform
Code-analysis filtering
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: epic/skill-routing-management
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
crates/skill-registry/src/hub.rs (1)
83-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
http_errflag / manual status check is dead code given ureq's default error handling.By default, ureq converts any non-2xx response into
Err(ureq::Error::Status(code, response))before the call returns — confirmed by ureq's docs: "A response was successfully received but had status code >= 400." This meanssend_string(&json)?already short-circuits via?for anystatus >= 400, so thelet status = resp.status(); if status >= 400 { http_err = ...; }branch below can never actually execute — theOk(resp)path is only reachable for 2xx responses. The final(Ok(()), Some(msg))match arm is likewise unreachable.The end result is still correct (4xx/5xx surface through
with_retry's ownErrpath andretryable()), but thehttp_errmechanism is confusing dead code that misrepresents the actual control flow to future readers.♻️ Proposed cleanup
pub fn push_bundle(hub_url: &str, bundle: &SkillBundle) -> Result<(), HubError> { let url = format!("{}/skills/bundle", hub_url.trim_end_matches('/')); let json = bundle .to_json() .map_err(|e| HubError::Serde(e.to_string()))?; - // Use a flag rather than a retryable error: 4xx/5xx from the hub is not - // likely to succeed on retry, but timeouts and transport errors are. - let mut http_err: Option<String> = None; - let success = with_retry("push", &mut || { - let resp = ureq::put(&url) + with_retry("push", &mut || { + ureq::put(&url) .set("Content-Type", "application/json") .set("User-Agent", "agentflare-skill-registry/0.1") - .send_string(&json)?; - let status = resp.status(); - if status >= 400 { - http_err = Some(format!("PUT {url} returned {status}")); - // Return OK to suppress retry — we'll check the flag after. - return Ok(()); - } - Ok(()) - }); - match (&success, &http_err) { - (Err(e), _) => return Err(HubError::Http(e.to_string())), - (Ok(()), Some(msg)) => return Err(HubError::Http(msg.clone())), - (Ok(()), None) => {} - } - Ok(()) + .send_string(&json) + .map(|_| ()) + }) }🤖 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/hub.rs` around lines 83 - 101, Remove the unused http_err flag, manual response-status check, and unreachable (Ok(()), Some(msg)) match arm around the with_retry("push", ...) call. Rely on send_string(&json)? and the existing Err(e) handling to propagate non-2xx responses, while preserving the successful (Ok(()), None) completion path.crates/skill-registry/src/sources.rs (1)
66-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNegation marker list is too broad and will misclassify legitimate content.
"skip"is included as a bare negation marker (\b(do not use|skip|not for|never use)\b). This word is extremely common in legitimate, non-negating skill descriptions/bodies (e.g. "how to skip flaky tests", "use this to skip boilerplate setup"). Any occurrence of "skip" as a standalone word will cause everything from that point onward to be misclassified intoneg_text, silently dropping real positive content fromdescription/bodyand feeding it into the (much lower-weighted)neg_textFTS column instead — degrading search relevance for skills that legitimately use this word.Consider requiring more context around "skip" (e.g. "skip if", "skip for", "do not... skip") or dropping it from the marker list in favor of more specific negation phrasing.
🤖 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 66 - 82, Update NEGATION_RE used by split_negation to stop treating standalone “skip” as a negation marker; remove it or constrain it to an explicitly negating phrase such as “skip if” or “skip for”. Preserve detection of the remaining specific negation markers and ensure legitimate content containing “skip” remains in the positive portion.src/skill_proactive.rs (1)
49-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTracking the unbuilt write path. The
#[allow(dead_code)]+ TODO onsave_settingsdocuments that noskill snooze/dismiss caller exists yet, so the read side (skill_overrides) can never be populated through the app. Want me to open an issue to track wiring up the snooze/dismiss write command so thisallowcan be removed?🤖 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/skill_proactive.rs` around lines 49 - 61, Track wiring the unbuilt snooze/dismiss command to call save_settings, enabling skill_overrides to be populated through the app. Add a follow-up issue for this missing CLI/MCP write path, while retaining the current #[allow(dead_code)] and TODO until a caller is implemented.src/cli/skill.rs (1)
599-599: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
"claude-code"isn't a recognized convention agent id.
default_sourcesonly conditionally adds dirs forcodex/cursor/opencode; the claude sources are always added regardless. So passing&["claude-code".to_string()]has no effect. If the intent was to include other detected agents' skills on pull, detect them (asensure_freshdoes) or drop the misleading argument.🤖 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/cli/skill.rs` at line 599, Update the default_sources call in the pull flow to stop passing the unsupported "claude-code" agent ID, or replace it with agent IDs actually detected by the same logic used in ensure_fresh. Ensure the argument accurately controls inclusion of conditional agent skill directories.
🤖 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 `@crates/flare-code/src/sub_skills.rs`:
- Around line 90-95: Update generated-file detection in the relevant sub-skill
function to scan a sufficiently bounded header region instead of only the first
five lines, so markers immediately after longer license banners are recognized
and the entire generated file is skipped. Add a boundary-case test covering a
generated marker on line 6 or later, preserving existing behavior for
non-generated files.
- Around line 79-80: Update the NOISE_PATTERNS regex so its final `--` comment
alternative matches both Copyright and SPDX headers, while preserving the
existing handling for `//` and `#` comments.
In `@crates/skill-registry/src/db.rs`:
- Around line 20-38: Update open_or_repair and its repair path to propagate
failures from removing the database instead of ignoring them, so a failed
deletion returns an error rather than reopening the corrupted file. Centralize
cleanup for db_path and its SQLite -wal and -shm sidecars, then recreate the
database only after cleanup succeeds while preserving the existing
integrity-check and open-failure triggers.
In `@crates/skill-registry/src/hub.rs`:
- Around line 62-103: Configure explicit read and write timeouts for the ureq
requests in pull_bundle and push_bundle, using a shared configured Agent or
equivalent timeout settings. Ensure both GET and PUT requests use these timeouts
so stalled hub connections cannot block indefinitely before with_retry handles
failures.
In `@crates/skill-registry/src/search.rs`:
- Around line 25-45: Correct apply_usage_decay so stale skills are penalized
under SQLite FTS5’s negative-score ascending sort convention: adjust scores
toward zero rather than multiplying negative scores by a factor above 1. Update
usage_decay_penalizes_stale_skills to use realistic negative scores such as -5.0
and verify recently used skills rank ahead of stale ones.
In `@src/cli/skill.rs`:
- Around line 573-585: Update run_import to merge imported entries with the
existing skill index before calling db::rebuild, matching the merge-then-rebuild
flow used by hub pull instead of replacing all rows. Preserve valid source paths
or otherwise ensure imported entries remain loadable and are not overwritten by
the next filesystem refresh; reuse the established hub-pull merge logic and
symbols rather than synthetic import paths.
- Around line 595-599: Update the source base passed to
skill_registry::sources::default_sources in the surrounding local skill
discovery flow to use the user’s home directory rather than
dirs::data_local_dir(). Preserve the existing temporary-directory fallback and
current-directory handling, ensuring .claude/.codex paths resolve under the
actual home directory.
---
Nitpick comments:
In `@crates/skill-registry/src/hub.rs`:
- Around line 83-101: Remove the unused http_err flag, manual response-status
check, and unreachable (Ok(()), Some(msg)) match arm around the
with_retry("push", ...) call. Rely on send_string(&json)? and the existing
Err(e) handling to propagate non-2xx responses, while preserving the successful
(Ok(()), None) completion path.
In `@crates/skill-registry/src/sources.rs`:
- Around line 66-82: Update NEGATION_RE used by split_negation to stop treating
standalone “skip” as a negation marker; remove it or constrain it to an
explicitly negating phrase such as “skip if” or “skip for”. Preserve detection
of the remaining specific negation markers and ensure legitimate content
containing “skip” remains in the positive portion.
In `@src/cli/skill.rs`:
- Line 599: Update the default_sources call in the pull flow to stop passing the
unsupported "claude-code" agent ID, or replace it with agent IDs actually
detected by the same logic used in ensure_fresh. Ensure the argument accurately
controls inclusion of conditional agent skill directories.
In `@src/skill_proactive.rs`:
- Around line 49-61: Track wiring the unbuilt snooze/dismiss command to call
save_settings, enabling skill_overrides to be populated through the app. Add a
follow-up issue for this missing CLI/MCP write path, while retaining the current
#[allow(dead_code)] and TODO until a caller is implemented.
🪄 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: 5d15de4b-5abb-4efa-a8b5-cbe61bc3d423
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.gitignorecrates/flare-code/Cargo.tomlcrates/flare-code/src/sub_skills.rscrates/skill-registry/Cargo.tomlcrates/skill-registry/src/db.rscrates/skill-registry/src/hub.rscrates/skill-registry/src/lib.rscrates/skill-registry/src/load.rscrates/skill-registry/src/pack.rscrates/skill-registry/src/search.rscrates/skill-registry/src/sources.rssrc/cli/skill.rssrc/hook.rssrc/main.rssrc/mcp_server.rssrc/mcp_server/types.rssrc/skill_detect.rssrc/skill_proactive.rs
Comment on lines
+79
to
+80
| static NOISE_PATTERNS: LazyLock<Regex> = LazyLock::new(|| { | ||
| Regex::new(r"(?m)^\s*(//\s*(Copyright|SPDX)|#\s*(Copyright|SPDX)|\-\-\s*Copyright)").unwrap() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match SPDX headers in -- comments.
Line 80 handles -- Copyright but not -- SPDX-License-Identifier, leaving that supported comment style unfiltered. Include SPDX in the final alternative.
Proposed fix
- Regex::new(r"(?m)^\s*(//\s*(Copyright|SPDX)|#\s*(Copyright|SPDX)|\-\-\s*Copyright)").unwrap()
+ Regex::new(r"(?m)^\s*(//\s*(Copyright|SPDX)|#\s*(Copyright|SPDX)|--\s*(Copyright|SPDX))").unwrap()📝 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.
Suggested change
| static NOISE_PATTERNS: LazyLock<Regex> = LazyLock::new(|| { | |
| Regex::new(r"(?m)^\s*(//\s*(Copyright|SPDX)|#\s*(Copyright|SPDX)|\-\-\s*Copyright)").unwrap() | |
| Regex::new(r"(?m)^\s*(//\s*(Copyright|SPDX)|#\s*(Copyright|SPDX)|--\s*(Copyright|SPDX))").unwrap() |
🤖 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/flare-code/src/sub_skills.rs` around lines 79 - 80, Update the
NOISE_PATTERNS regex so its final `--` comment alternative matches both
Copyright and SPDX headers, while preserving the existing handling for `//` and
`#` comments.
Comment on lines
+90
to
+95
| if text | ||
| .lines() | ||
| .take(5) | ||
| .any(|line| GENERATED_MARKERS.is_match(line)) | ||
| { | ||
| return String::new(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not limit generated-file detection to five lines.
A five-line license banner followed by a generated marker on Line 6 is not skipped; only the marker line is removed and generated code is still analyzed. Scan a sufficiently bounded header region and add this boundary-case test.
Also applies to: 263-274
🤖 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/flare-code/src/sub_skills.rs` around lines 90 - 95, Update
generated-file detection in the relevant sub-skill function to scan a
sufficiently bounded header region instead of only the first five lines, so
markers immediately after longer license banners are recognized and the entire
generated file is skipped. Add a boundary-case test covering a generated marker
on line 6 or later, preserving existing behavior for non-generated files.
Comment on lines
+20
to
+38
| /// Open DB with repair: if integrity check fails or open fails, delete the DB | ||
| /// file and create a fresh one. | ||
| pub fn open_or_repair(db_path: &Path) -> rusqlite::Result<Connection> { | ||
| match open_db(db_path) { | ||
| Ok(conn) => { | ||
| if integrity_check(&conn).is_some() { | ||
| drop(conn); | ||
| let _ = std::fs::remove_file(db_path); | ||
| open_db(db_path) | ||
| } else { | ||
| Ok(conn) | ||
| } | ||
| } | ||
| Err(_) => { | ||
| let _ = std::fs::remove_file(db_path); | ||
| open_db(db_path) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
open_or_repair can silently return a still-corrupted connection, and doesn't clean up WAL/SHM sidecars.
Two related gaps in the repair path:
let _ = std::fs::remove_file(db_path);ignores the removal's result. If it fails (e.g. permission denied, file locked by another process), the corrupted file is left in place and the subsequentopen_db(db_path)simply reopens the same corrupted database —open_or_repairthen returnsOk(conn)for a connection that was never actually repaired, with no error signaled to the caller.- Only the main
db_pathfile is removed. WAL-mode databases also have-wal/-shmsidecar files; if corruption originates in a partially-written WAL, leaving those files behind while recreating the main file is unnecessarily risky/untidy for a function whose entire purpose is a clean rebuild.
🛡️ Proposed fix
pub fn open_or_repair(db_path: &Path) -> rusqlite::Result<Connection> {
match open_db(db_path) {
Ok(conn) => {
if integrity_check(&conn).is_some() {
drop(conn);
- let _ = std::fs::remove_file(db_path);
+ remove_db_and_sidecars(db_path)?;
open_db(db_path)
} else {
Ok(conn)
}
}
Err(_) => {
- let _ = std::fs::remove_file(db_path);
+ remove_db_and_sidecars(db_path)?;
open_db(db_path)
}
}
}
+
+fn remove_db_and_sidecars(db_path: &Path) -> rusqlite::Result<()> {
+ let path = db_path.to_string_lossy().to_string();
+ if db_path.exists() {
+ std::fs::remove_file(db_path)
+ .map_err(|e| rusqlite::Error::SqliteFailure(
+ rusqlite::ffi::Error::new(rusqlite::ffi::SQLITE_CANTOPEN),
+ Some(format!("failed to remove {path}: {e}")),
+ ))?;
+ }
+ let _ = std::fs::remove_file(format!("{path}-wal"));
+ let _ = std::fs::remove_file(format!("{path}-shm"));
+ Ok(())
+}📝 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.
Suggested change
| /// Open DB with repair: if integrity check fails or open fails, delete the DB | |
| /// file and create a fresh one. | |
| pub fn open_or_repair(db_path: &Path) -> rusqlite::Result<Connection> { | |
| match open_db(db_path) { | |
| Ok(conn) => { | |
| if integrity_check(&conn).is_some() { | |
| drop(conn); | |
| let _ = std::fs::remove_file(db_path); | |
| open_db(db_path) | |
| } else { | |
| Ok(conn) | |
| } | |
| } | |
| Err(_) => { | |
| let _ = std::fs::remove_file(db_path); | |
| open_db(db_path) | |
| } | |
| } | |
| } | |
| /// Open DB with repair: if integrity check fails or open fails, delete the DB | |
| /// file and create a fresh one. | |
| pub fn open_or_repair(db_path: &Path) -> rusqlite::Result<Connection> { | |
| match open_db(db_path) { | |
| Ok(conn) => { | |
| if integrity_check(&conn).is_some() { | |
| drop(conn); | |
| remove_db_and_sidecars(db_path)?; | |
| open_db(db_path) | |
| } else { | |
| Ok(conn) | |
| } | |
| } | |
| Err(_) => { | |
| remove_db_and_sidecars(db_path)?; | |
| open_db(db_path) | |
| } | |
| } | |
| } | |
| fn remove_db_and_sidecars(db_path: &Path) -> rusqlite::Result<()> { | |
| let path = db_path.to_string_lossy().to_string(); | |
| if db_path.exists() { | |
| std::fs::remove_file(db_path).map_err(|e| { | |
| rusqlite::Error::SqliteFailure( | |
| rusqlite::ffi::Error::new(rusqlite::ffi::SQLITE_CANTOPEN), | |
| Some(format!("failed to remove {path}: {e}")), | |
| ) | |
| })?; | |
| } | |
| let _ = std::fs::remove_file(format!("{path}-wal")); | |
| let _ = std::fs::remove_file(format!("{path}-shm")); | |
| Ok(()) | |
| } |
🤖 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/db.rs` around lines 20 - 38, Update open_or_repair
and its repair path to propagate failures from removing the database instead of
ignoring them, so a failed deletion returns an error rather than reopening the
corrupted file. Centralize cleanup for db_path and its SQLite -wal and -shm
sidecars, then recreate the database only after cleanup succeeds while
preserving the existing integrity-check and open-failure triggers.
Comment on lines
+62
to
+103
| /// Pull a SkillBundle from a remote hub URL (GET /skills/bundle). | ||
| pub fn pull_bundle(hub_url: &str) -> Result<SkillBundle, HubError> { | ||
| let url = format!("{}/skills/bundle", hub_url.trim_end_matches('/')); | ||
| let body = with_retry("pull", &mut || -> Result<String, Box<ureq::Error>> { | ||
| ureq::get(&url) | ||
| .set("User-Agent", "agentflare-skill-registry/0.1") | ||
| .call()? | ||
| .into_string() | ||
| .map_err(|e| Box::new(ureq::Error::from(e))) | ||
| })?; | ||
| SkillBundle::from_json(&body).map_err(|e| HubError::Serde(e.to_string())) | ||
| } | ||
|
|
||
| /// Push a SkillBundle to a remote hub (PUT /skills/bundle). | ||
| pub fn push_bundle(hub_url: &str, bundle: &SkillBundle) -> Result<(), HubError> { | ||
| let url = format!("{}/skills/bundle", hub_url.trim_end_matches('/')); | ||
| let json = bundle | ||
| .to_json() | ||
| .map_err(|e| HubError::Serde(e.to_string()))?; | ||
| // Use a flag rather than a retryable error: 4xx/5xx from the hub is not | ||
| // likely to succeed on retry, but timeouts and transport errors are. | ||
| let mut http_err: Option<String> = None; | ||
| let success = with_retry("push", &mut || { | ||
| let resp = ureq::put(&url) | ||
| .set("Content-Type", "application/json") | ||
| .set("User-Agent", "agentflare-skill-registry/0.1") | ||
| .send_string(&json)?; | ||
| let status = resp.status(); | ||
| if status >= 400 { | ||
| http_err = Some(format!("PUT {url} returned {status}")); | ||
| // Return OK to suppress retry — we'll check the flag after. | ||
| return Ok(()); | ||
| } | ||
| Ok(()) | ||
| }); | ||
| match (&success, &http_err) { | ||
| (Err(e), _) => return Err(HubError::Http(e.to_string())), | ||
| (Ok(()), Some(msg)) => return Err(HubError::Http(msg.clone())), | ||
| (Ok(()), None) => {} | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== hub.rs ==\n'
sed -n '1,180p' crates/skill-registry/src/hub.rs
printf '\n== ureq dependency ==\n'
grep -nE '^\s*ureq\s*=' -n Cargo.toml Cargo.lock crates/skill-registry/Cargo.toml 2>/dev/null || true
printf '\n== any explicit timeout usage in skill-registry ==\n'
grep -RIn --exclude-dir target --exclude-dir node_modules -E '\.timeout(_read|_write)?\(|timeout_read|timeout_write|timeout_connect' crates/skill-registry || trueRepository: getappz/agentflare
Length of output: 4709
🌐 Web query:
ureq default timeout_read timeout_write no timeout requests may block forever read timeout docs
💡 Result:
In the ureq library, a Rust HTTP client, the default behavior for all timeout settings is no timeout (None) [1][2]. Consequently, requests may block indefinitely while waiting for socket reads, writes, or other operations, as the library relies on the underlying OS socket behavior [3][1][4]. To prevent requests from blocking forever, you must explicitly configure timeouts using an AgentBuilder or ConfigBuilder [3][1][5]. Key points regarding timeouts in ureq: - Default Behavior: All timeout configurations default to None, meaning no explicit timeout is enforced by the library [1][2]. - Blocking I/O: Ureq uses blocking I/O, which means each concurrent request typically requires an OS thread [6][4]. Because there is no internal timeout, a hung connection may result in an OS thread being blocked indefinitely [3][4]. - Configuration: You can set various timeouts (e.g., connect, read, write, global) when creating an Agent [3][1]. For example, using AgentBuilder or ConfigBuilder methods like timeout_read or timeout_global allows you to define explicit durations [3][1]. - OS Interaction: Even when timeouts are not set, requests may eventually fail if the underlying operating system gives up on the TCP connection (often after several minutes), though this is controlled by the OS and not by ureq [7]. For applications where preventing indefinite blocking is critical, it is highly recommended to configure a global or per-call timeout [1].
Citations:
- 1: https://docs.rs/ureq/latest/ureq/config/struct.ConfigBuilder.html
- 2: https://docs.rs/ureq/latest/ureq/enum.Error.html
- 3: https://docs.rs/ureq/2.1.1/ureq/struct.AgentBuilder.html
- 4: https://docs.rs/ureq/^2
- 5: Timeout for connecting to a host algesten/ureq#595
- 6: https://github.com/algesten/ureq
- 7: Err(Timeout(Global)) can be returned even when no timeouts are set algesten/ureq#1024
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, os, re, pathlib
root = pathlib.Path('.')
for p in [root/'Cargo.toml', root/'Cargo.lock', root/'crates/skill-registry/Cargo.toml']:
if p.exists():
print(f"\n== {p} ==")
txt = p.read_text()
if p.name == 'Cargo.lock':
# Print ureq entries around occurrence
for m in re.finditer(r'(?m)^name = "ureq"$', txt):
start = txt.rfind('\n[[package]]', 0, m.start())
end = txt.find('\n[[package]]', m.start()+1)
print(txt[start+1:end if end!=-1 else None][:2000])
else:
for line in txt.splitlines():
if 'ureq' in line:
print(line)
PYRepository: getappz/agentflare
Length of output: 1018
Add an explicit read/write timeout to hub requests. pull_bundle and push_bundle use ureq’s default agent, which leaves read/write timeouts unset, so a stalled hub can block a thread indefinitely before retry/backoff has a chance to help. Reuse a configured Agent with timeout_read/timeout_write or a global timeout here.
🤖 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/hub.rs` around lines 62 - 103, Configure explicit
read and write timeouts for the ureq requests in pull_bundle and push_bundle,
using a shared configured Agent or equivalent timeout settings. Ensure both GET
and PUT requests use these timeouts so stalled hub connections cannot block
indefinitely before with_retry handles failures.
Comment on lines
+25
to
+45
| /// Exponential decay half-life in seconds (30 days). | ||
| const DECAY_HALF_LIFE: f64 = 30.0 * 86400.0; | ||
|
|
||
| /// Apply usage-decay penalty: skills used more recently rank slightly higher | ||
| /// than equally-relevant stale ones. `now` is seconds since epoch. | ||
| fn apply_usage_decay(hits: &mut [SkillHit], now: i64) { | ||
| for h in hits.iter_mut() { | ||
| if h.last_used_at == 0 { | ||
| continue; | ||
| } | ||
| let elapsed = (now - h.last_used_at) as f64; | ||
| if elapsed <= 0.0 { | ||
| continue; | ||
| } | ||
| // decay = 2^(-elapsed / half_life) → 1.0 when just used, → 0.0 when ancient | ||
| let decay = (-elapsed / DECAY_HALF_LIFE).exp2(); | ||
| // Penalty: at most 30% of the raw score, scaled by decay. | ||
| // Newest: score * 1.0. Oldest: score * (1.0 + 0.3). | ||
| h.score = h.score + h.score * 0.3 * (1.0 - decay); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
apply_usage_decay inverts ranking on real (negative) bm25 scores — stale skills get boosted, not penalized.
search()'s SQL query returns bm25(...) as score, and per SQLite's FTS5 convention (and the file's own ORDER BY score ascending), matching rows get negative scores where more negative = more relevant. apply_usage_decay computes:
h.score = h.score + h.score * 0.3 * (1.0 - decay);For a negative h.score, multiplying by a factor in [1.0, 1.3] (as decay shrinks toward 0 for stale skills) makes the score more negative, not less — which under this system's sort convention means the stale skill ranks better, not worse. This is the exact opposite of the stated intent ("skills used more recently rank slightly higher... than equally-relevant stale ones").
The regression test usage_decay_penalizes_stale_skills doesn't catch this because it seeds an artificial positive score: 1.0 on both hits — for a positive number the same formula does grow it in the "expected" direction, which masks the sign bug that only manifests on the real, negative bm25-derived scores actually passed in from search() (line 130's apply_usage_decay(&mut rows, now), fed directly from r.get(8)? = bm25(...)).
🐛 Proposed fix
// decay = 2^(-elapsed / half_life) → 1.0 when just used, → 0.0 when ancient
let decay = (-elapsed / DECAY_HALF_LIFE).exp2();
- // Penalty: at most 30% of the raw score, scaled by decay.
- // Newest: score * 1.0. Oldest: score * (1.0 + 0.3).
- h.score = h.score + h.score * 0.3 * (1.0 - decay);
+ // bm25 scores are negative (more negative = more relevant), so
+ // penalizing staleness means pushing the score toward zero
+ // regardless of sign, not scaling its magnitude up.
+ h.score += h.score.abs() * 0.3 * (1.0 - decay);Also worth strengthening the existing test to use a realistic negative score (e.g. score: -5.0) so it would have caught this.
Also applies to: 126-130
🤖 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/search.rs` around lines 25 - 45, Correct
apply_usage_decay so stale skills are penalized under SQLite FTS5’s
negative-score ascending sort convention: adjust scores toward zero rather than
multiplying negative scores by a factor above 1. Update
usage_decay_penalizes_stale_skills to use realistic negative scores such as -5.0
and verify recently used skills rank ahead of stale ones.
Comment on lines
+573
to
+585
| fn run_import(path: &str) -> Result<usize, Box<dyn std::error::Error>> { | ||
| let json = std::fs::read_to_string(path)?; | ||
| let mut bundle = skill_registry::SkillBundle::from_json(&json)?; | ||
| let deduped = bundle.dedup(); | ||
| if deduped > 0 { | ||
| eprintln!("note: removed {deduped} duplicate entries during import"); | ||
| } | ||
| let db_path = crate::paths::skills_db_path(); | ||
| let mut conn = skill_registry::db::open_db(&db_path)?; | ||
| let entries = bundle.to_entries(Path::new("import")); | ||
| skill_registry::db::rebuild(&mut conn, &entries)?; | ||
| Ok(entries.len()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- files ---'
git ls-files 'src/cli/skill.rs' 'src/**/skill*.rs' 'src/**/db*.rs' 'src/**/paths*.rs' | sed 's#^`#-` #'
echo '--- locate relevant symbols ---'
rg -n "fn run_import|fn run_hub|ensure_fresh|load\(|rebuild\(|skills_fts|DELETE FROM skills|DELETE FROM skills_fts|hub pull|pull" src/cli/skill.rs src -g '*.rs'
echo '--- file sizes ---'
wc -l src/cli/skill.rs || true
wc -l src/**/*.rs 2>/dev/null | sort -n | tail -n 20 || true
echo '--- run_import context ---'
sed -n '540,640p' src/cli/skill.rs
echo '--- rebuild implementation ---'
# try to find db module file paths first
rg -n "pub fn rebuild|fn rebuild|struct .*Db|open_db|to_entries|load\(" src -g '*.rs'Repository: getappz/agentflare
Length of output: 25135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate skill_registry definitions ---'
rg -n "mod skill_registry|crate::skill_registry|skill_registry::" src Cargo.toml -g '*.rs' -g 'Cargo.toml'
echo '--- find candidate source files ---'
git ls-files | rg 'skill_registry|skill|bundle|registry|paths|db\.rs|lib\.rs$|mod\.rs$'
echo '--- inspect cli imports and top of skill.rs ---'
sed -n '1,120p' src/cli/skill.rs
echo '--- inspect maybe-relevant modules if present ---'
for f in $(git ls-files | rg 'skill_registry|skill.*registry|registry.*skill|bundle|skills'); do
echo "### $f"
wc -l "$f"
doneRepository: getappz/agentflare
Length of output: 11367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- outline db.rs ---'
ast-grep outline crates/skill-registry/src/db.rs --view expanded || true
echo '--- outline load.rs ---'
ast-grep outline crates/skill-registry/src/load.rs --view expanded || true
echo '--- outline sources.rs ---'
ast-grep outline crates/skill-registry/src/sources.rs --view expanded || true
echo '--- inspect db.rs ---'
sed -n '1,260p' crates/skill-registry/src/db.rs
echo '--- inspect load.rs (first half) ---'
sed -n '1,240p' crates/skill-registry/src/load.rs
echo '--- inspect sources.rs around SkillEntry and bundle conversion ---'
rg -n "struct SkillEntry|fn to_entries|struct SkillBundle|dedup|path:" crates/skill-registry/src/sources.rs crates/skill-registry/src/lib.rs
sed -n '1,260p' crates/skill-registry/src/sources.rsRepository: getappz/agentflare
Length of output: 32178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate SkillBundle and to_entries ---'
rg -n "struct SkillBundle|impl SkillBundle|fn to_entries|to_entries\\(" crates/skill-registry/src -g '*.rs'
echo '--- inspect bundle conversion and load tests ---'
sed -n '260,380p' crates/skill-registry/src/sources.rs
sed -n '240,340p' crates/skill-registry/src/load.rsRepository: getappz/agentflare
Length of output: 7494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- inspect pack.rs ---'
sed -n '1,260p' crates/skill-registry/src/pack.rs
echo '--- inspect import-related tests in pack.rs ---'
sed -n '160,240p' crates/skill-registry/src/pack.rs
echo '--- quick path-behavior probe (read-only) ---'
python3 - <<'PY'
from pathlib import Path
base = Path("import")
p = base / "foo" / "SKILL.md"
print("synthetic path:", p)
print("exists:", p.exists())
PYRepository: getappz/agentflare
Length of output: 7089
Import full-replaces the skills DB db::rebuild clears skills and skills_fts before inserting, so skill import drops whatever is already indexed instead of merging. The bundle entries also get synthetic paths like import/<name>/SKILL.md, so load() will try to read files that do not exist, and the next filesystem refresh will overwrite these imported rows. If that is intentional, keep it; otherwise import should follow the same merge-then-rebuild flow as hub pull.
🤖 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/cli/skill.rs` around lines 573 - 585, Update run_import to merge imported
entries with the existing skill index before calling db::rebuild, matching the
merge-then-rebuild flow used by hub pull instead of replacing all rows. Preserve
valid source paths or otherwise ensure imported entries remain loadable and are
not overwritten by the next filesystem refresh; reuse the established hub-pull
merge logic and symbols rather than synthetic import paths.
Comment on lines
+595
to
+599
| let home = dirs::data_local_dir().unwrap_or_else(std::env::temp_dir); | ||
| let cwd = std::env::current_dir().ok(); | ||
| let cwd = cwd.as_deref().unwrap_or(Path::new(".")); | ||
| let sources = | ||
| skill_registry::sources::default_sources(&home, cwd, &["claude-code".to_string()]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant function and surrounding call sites.
rg -n "default_sources\\(|data_local_dir\\(|home_dir\\(|ensure_fresh\\(" src/cli/skill.rs src -g '!target' -g '!dist' -g '!build' || true
# Show the relevant slice around the cited lines.
sed -n '560,640p' src/cli/skill.rs
# Show the default_sources implementation if present in the repo.
rg -n "fn default_sources|default_sources\\(" src -g '!target' -g '!dist' -g '!build' || trueRepository: getappz/agentflare
Length of output: 5550
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map candidate files first.
ast-grep outline src/paths.rs --view expanded || true
ast-grep outline src/cli/skill.rs --view expanded || true
# Find the default_sources implementation.
rg -n "default_sources|skills\"|claude|codex|home_dir\\(|data_local_dir\\(" src -g '!target' -g '!dist' -g '!build' || true
# Read the relevant helper file in a narrow slice.
sed -n '1,140p' src/paths.rs
# If default_sources is in a separate file, try to locate it directly.
fd -a "sources.rs" src || true
fd -a "skill_registry" src || trueRepository: getappz/agentflare
Length of output: 41508
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any other uses of the same source scan pattern and the home/data-local split.
rg -n "scan_sources\\(|default_sources\\(|\\.claude.*skills|\\.codex.*skills|skills.db" src -g '!target' -g '!dist' -g '!build' || true
# Show the hub/pull and freshening paths together for comparison.
sed -n '380,620p' src/cli/skill.rs
# Show any tests around hub or local skill scanning if present.
sed -n '710,840p' src/cli/skill.rsRepository: getappz/agentflare
Length of output: 13174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for docs/comments/tests describing where skill sources live.
rg -n "skills|skill source|\\.claude/skills|\\.codex/skills|local skills|default_sources" src README.md docs -g '!target' -g '!dist' -g '!build' || true
# Show the nearby tests in cli/skill.rs if any mention local skill discovery.
sed -n '740,830p' src/cli/skill.rsRepository: getappz/agentflare
Length of output: 11633
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '110,140p' src/cli/skill.rsRepository: getappz/agentflare
Length of output: 1186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Identify where skill_registry comes from.
rg -n 'name = "skill-registry"|skill_registry|skill-registry' Cargo.toml Cargo.lock . -g '!target' -g '!dist' -g '!build' || true
# List top-level files that might vendor the dependency or point to it.
git ls-files | rg '(^|/)skill(-|_)?registry|Cargo.lock|Cargo.toml$' || trueRepository: getappz/agentflare
Length of output: 7463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline crates/skill-registry/src/sources.rs --view expanded || true
sed -n '1,260p' crates/skill-registry/src/sources.rsRepository: getappz/agentflare
Length of output: 11975
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the default_sources implementation and its tests.
sed -n '278,560p' crates/skill-registry/src/sources.rs
# Also inspect any Registry freshness code in the crate for comparison.
rg -n "ensure_fresh|default_sources\\(" crates/skill-registry/src -g '!target' -g '!dist' -g '!build' || trueRepository: getappz/agentflare
Length of output: 10787
Use the home directory for local skill discovery default_sources builds .claude/.codex paths from this base, so dirs::data_local_dir() scans ~/.local/share/.claude/skills and misses the user's actual local skills, skewing scan.entries and net_new.
🐛 Proposed fix
- let home = dirs::data_local_dir().unwrap_or_else(std::env::temp_dir);
+ let home = dirs::home_dir().unwrap_or_else(std::env::temp_dir);📝 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.
Suggested change
| let home = dirs::data_local_dir().unwrap_or_else(std::env::temp_dir); | |
| let cwd = std::env::current_dir().ok(); | |
| let cwd = cwd.as_deref().unwrap_or(Path::new(".")); | |
| let sources = | |
| skill_registry::sources::default_sources(&home, cwd, &["claude-code".to_string()]); | |
| let home = dirs::home_dir().unwrap_or_else(std::env::temp_dir); | |
| let cwd = std::env::current_dir().ok(); | |
| let cwd = cwd.as_deref().unwrap_or(Path::new(".")); | |
| let sources = | |
| skill_registry::sources::default_sources(&home, cwd, &["claude-code".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 `@src/cli/skill.rs` around lines 595 - 599, Update the source base passed to
skill_registry::sources::default_sources in the surrounding local skill
discovery flow to use the user’s home directory rather than
dirs::data_local_dir(). Preserve the existing temporary-directory fallback and
current-directory handling, ensuring .claude/.codex paths resolve under the
actual home directory.
…management # Conflicts: # Cargo.lock # crates/flare-code/src/sub_skills.rs Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: epic/skill-routing-management
…management Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: epic/skill-routing-management
getappz
enabled auto-merge (squash)
July 22, 2026 05:32
Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: epic/skill-routing-management
…management Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: epic/skill-routing-management
…management # Conflicts: # Cargo.lock Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: epic/skill-routing-management
…d by the upstream API change skill_proactive.rs and cli/skill.rs's eval command both call Registry::ensure_fresh(), which task/61's landed fix changed to take a detect_agents closure. Neither file exists on that branch, so its own build never caught these two call sites. Agentflare-Agent: claude-code_2-1-217_agent Agentflare-Branch: epic/skill-routing-management
…management Agentflare-Agent: claude-code_2-1-217_agent Agentflare-Branch: epic/skill-routing-management
Agentflare-Agent: claude-code_2-1-217_agent Agentflare-Branch: epic/skill-routing-management
4 tasks
getappz
added a commit
that referenced
this pull request
Jul 22, 2026
gc_orphans: retry remove_dir_all with exponential backoff, fall back to cmd /c rmdir, detect locking process via handle64.exe. cleanup-branches.sh: retry git worktree remove on Permission denied with backoff; distinguish dirty vs locked; warn and fall through on lock so branch cleanup is not blocked. Item #302: git worktree remove --force .worktrees/ fails with Permission denied on Windows when rust-analyzer holds file handles. Agentflare-Agent: 1 Agentflare-Branch: feat/worktree-audit-294
5 tasks
getappz
added a commit
that referenced
this pull request
Jul 25, 2026
…#322) remove_worktree_dir's existing retry loop and cmd /c rmdir fallback both only clear a transient in-use lock (e.g. item #302's rust-analyzer case). Neither touches a genuine ACL denial, which is item #267's actual failure mode: cargo's own target/*/.fingerprint/* files can end up ACL-restricted, not merely open, and no amount of retrying clears that. Adds one more fallback before giving up: icacls /grant <user>:F /T resets ownership access recursively, then one final remove_dir_all attempt. No-op (and thus never destructive) when the real problem was actually an in-use lock the earlier retries already cleared. Considered switching to git-parsec's shared_cache symlink strategy instead (share target/ across worktrees so there's nothing per-worktree to get ACL-locked in the first place) -- rejected: this codebase already hit and fixed the exact correctness bug that would reintroduce (item #139, cargo #12516/#14053/#7740 -- a shared CARGO_TARGET_DIR's fingerprint hash omits the worktree path, so two worktrees of different branches silently reuse each other's stale local crate artifacts). isolate_worktree_target_dir's per-worktree isolation is deliberate, not an oversight; sccache (item #133) already covers the safe part of cross-worktree cache sharing (registry deps, hash-keyed). Agentflare-Agent: claude-code_2-1-219_agent Agentflare-Branch: task/267 Agentflare-Item: 267
5 tasks
getappz
added a commit
that referenced
this pull request
Jul 28, 2026
…sk 9 provisioning (#354) * feat(skill): finish EPIC #272 Task 6 write-path + Task 9 provisioning Task 6 (proactive advisory): proactive_suggestions()/settings read-path already existed (PR #302) but had no way to actually set a snooze or dismiss - save_settings() was dead code. Add `skill snooze <name> [--days N]` and `skill dismiss <name>` wired to it. Task 9 (repo stack -> auto provisioning): new `skill provision <path>` subcommand. Detects stack via manifest files (Cargo.toml/package.json/ tsconfig.json/pyproject.toml/requirements.txt/go.mod), ranks indexed skills against it via BM25 search, prints a dry-run report (skills, confidence, token cost) with zero DB writes, and on --yes re-tags the matched entries under source "provisioned:<repo>" via the existing scan+rebuild entry-creation path. * fix(skill): address CodeRabbit findings on provision/snooze - rank_candidates picked the highest bm25 score as "best" and sorted descending; bm25 is negative-is-better (search.rs sorts ASC on the same raw value), so this had it backwards. Flip to min-selection + ascending sort, add confidence_pct() for a bounded 0-100 display value instead of the broken score.min(1.0)*100.0. - run_provision --yes rebuilt the DB from a fresh flat-dir scan only, and rebuild() is full-replace -- silently dropping any other provisioned:*/imported:*/hub:* DB-only rows on every run. Merge existing DB rows (via list_all_name_source_pairs + load, same reconstruction run_export already uses) before rebuilding, mirroring HubAction::Pull's merge-before-rebuild shape. - Use crate::components::detected_skill_agents() instead of a hardcoded ["claude-code"] source list. - snooze() clamped days to >=0 but not the upper bound; saturating_add from a large --days no longer risks i64 overflow. - New adversarial rank_candidates test (two same-tag matches of different strength) -- the previous test had only one candidate per tag, so it couldn't have caught the sign inversion.
getappz
added a commit
that referenced
this pull request
Aug 21, 2026
…g-column schema drift apply_schema() hand-rolled CREATE TABLE IF NOT EXISTS DDL, which is a no-op against an existing table -- any skills.db created before #302 added body/neg_text/last_used_at/bandit_alpha/bandit_beta stayed permanently stuck without them, while the FTS5 triggers added later (#347) reference old.body/new.body etc. and throw "no such column: old.body" the first time a DELETE or qualifying UPDATE fires (e.g. rebuild()'s DELETE FROM skills). Reproduced live via skill_detect against a real skills.db from before #302. Migrate to agentflare-db-kit's open_file/open_memory with a real migration list, matching the pattern already used by agentflare-backend/agentflare-store/agentflare-artifacts/flare-docs/ flare-workflow/agentflare-jobs: 0001_initial replays the original (#92) narrow schema, 0002_ranking_and_fts adds the ranking columns via a migration hook (ALTER TABLE ADD COLUMN isn't idempotent, so it's guarded by a PRAGMA table_info check first) plus the external-content FTS5 table and sync triggers, drop-and-recreated unconditionally so it's correct regardless of which pre-migration shape the database was in. Verified against a copy of the actual broken skills.db from this machine in addition to the new unit test that reproduces the bug synthetically. Agentflare-Agent: claude-code_2-1-237_agent Agentflare-Branch: task/519-fix-skill-registry-adopt-agentflare-db-k Agentflare-Item: 519
getappz
added a commit
that referenced
this pull request
Aug 21, 2026
…g-column schema drift (#572) * fix(skill-registry): adopt agentflare-db-kit migrations to fix missing-column schema drift apply_schema() hand-rolled CREATE TABLE IF NOT EXISTS DDL, which is a no-op against an existing table -- any skills.db created before #302 added body/neg_text/last_used_at/bandit_alpha/bandit_beta stayed permanently stuck without them, while the FTS5 triggers added later (#347) reference old.body/new.body etc. and throw "no such column: old.body" the first time a DELETE or qualifying UPDATE fires (e.g. rebuild()'s DELETE FROM skills). Reproduced live via skill_detect against a real skills.db from before #302. Migrate to agentflare-db-kit's open_file/open_memory with a real migration list, matching the pattern already used by agentflare-backend/agentflare-store/agentflare-artifacts/flare-docs/ flare-workflow/agentflare-jobs: 0001_initial replays the original (#92) narrow schema, 0002_ranking_and_fts adds the ranking columns via a migration hook (ALTER TABLE ADD COLUMN isn't idempotent, so it's guarded by a PRAGMA table_info check first) plus the external-content FTS5 table and sync triggers, drop-and-recreated unconditionally so it's correct regardless of which pre-migration shape the database was in. Verified against a copy of the actual broken skills.db from this machine in addition to the new unit test that reproduces the bug synthetically. Agentflare-Agent: claude-code_2-1-237_agent Agentflare-Branch: task/519-fix-skill-registry-adopt-agentflare-db-k Agentflare-Item: 519 * fix(skill-registry): open_or_repair must not delete the db on migration/SchemaAhead errors CodeRabbit review on PR #572: the Err(_) arm deleted db_path for every open_db failure, including db_kit::open::Error::Migration (a real bug in a migration -- deleting the file would silently destroy skill_impressions/ranking state the filesystem can't reconstruct, hiding the bug instead of surfacing it) and Error::SchemaAhead (means a newer build already migrated this file; its own error message says not to touch it by hand, let alone delete it). Now only deletes-and-retries on Error::Sqlite -- a genuine open/read failure (corruption, not a valid SQLite file). Migration/SchemaAhead propagate as real errors. Added regression tests for both paths, plus documented FTS_AND_TRIGGERS as frozen migration-0002 history per the review's nitpick. Agentflare-Agent: claude-code_2-1-237_agent Agentflare-Branch: task/519-fix-skill-registry-adopt-agentflare-db-k Agentflare-Item: 519 * fix(skill-registry): don't treat a failed integrity_check query as corruption integrity_check() previously folded a real corruption verdict from SQLite and a failure to even run the check (locked db, permission denied, I/O error) into the same Some(String), and open_or_repair() deleted the file on either. A transient failure to run the check is not evidence of corruption -- propagate it instead of destroying the database. Agentflare-Agent: claude-code_2-1-237_agent Agentflare-Branch: task/519-fix-skill-registry-adopt-agentflare-db-k Agentflare-Item: 519 * chore: regenerate Cargo.lock Agentflare-Agent: claude-code_2-1-237_agent Agentflare-Branch: task/519-fix-skill-registry-adopt-agentflare-db-k Agentflare-Item: 519
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Adopts feature-rich skill routing/management into
crates/skill-registry+src/skill_detect.rs(agentflare item #272, 8 child tasks):neg_textexclusion signal for search rankingskill evalharnessbandit_alpha/bandit_betaper skill, sampled via Marsaglia-Tsang and blended into the bm25 score)skill install/skill searchCLIReview notes
Self-reviewed via cavecrew-reviewer + manual verification (findings: agentflare
mcp__flare__review, comment on item #282). Found and fixed a compile-blocking bug beyond the initial pass:bandit_alpha/bandit_betawere added toSkillEntry/SkillHitwithout updating 10 of 12 construction sites — neither the skill-registry crate nor the main binary's test build compiled. Also fixed a brokenparams!macro indb.rs::rebuild(syntax error + wrong SQL placeholder order) and replaced a fabricated Beta-sampling formula inbeta_samplewith the real Marsaglia-Tsang Gamma-ratio construction.Known open scope gaps (tracked, not blocking this PR): Task 7 (import) shipped a JSON-bundle format instead of the spec'd git-clone importer; Task 8 (reliability) shipped DB integrity-check/repair instead of the spec'd
gateway_registrydispatch reliability layer; Task 6's settings write-path has no caller yet.Test plan
cargo build --workspace --all-features— cleancargo test --workspace— 726/728 (2 pre-existinghook.rssession_start_*failures are global-env-var test-isolation flakiness under parallel execution, unrelated to this diff — reproduced passing in isolation on both pre- and post-fix trees)cargo clippy -p agentflare-skill-registry --all-targets— cleanSummary by CodeRabbit