Repository navigation
fix(skills): hot-remove deleted skills by id, not directory name (#417) - #419
ALDL (victor-commit) wants to merge 1 commit into
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 54 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to No actionable deletion-path issue remains; the change is ready to merge after normal checks. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
The change meets issue #417. The Deleted handler obtains the deleted skill directory from path.parent(). It uses DashMap::retain to compare that directory with each skill's stored source_path.… |
| Out of Scope Changes check | |
The reviewed change is limited to the WatchEvent::Deleted hot-reload removal logic and related removal logging. The change directly supports issue #417. No unrelated behavior or files are identified… |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. |
| Description Check | |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly describes the hot-reload fix for deleted skills and identifies the mismatch between skill IDs and directory names. |
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
The WatchEvent::Deleted handler removed skills using the lowercased directory name, but skills are stored under an id derived from the SKILL.toml `name` (lowercased, spaces->hyphens, non-alphanumeric stripped). When they differ the removal missed and the stale skill stayed in the DashMap. Match on the stored `source_path` instead. Fixes CortexLM#417
56db971 to
a145e3d
Compare
Deleting a skill's
SKILL.tomldoesn't reliably unload the skill.The
WatchEvent::Deletedhandler removes by the lowercased directory name, but skills are stored under an id derived from thenamefield inSKILL.toml(lowercased, spaces → hyphens, non-alphanumerics stripped — seeparser.rs). So a skill incode_review/withname = "Code Review"is stored ascode-review, while delete tries to removecode_reviewand misses — the stale skill stays in the map.Each
Skillalready keeps itssource_path(the skill directory), so this switches the delete to match on that viaretain, removing the entry by its real id regardless of the folder name.I checked this against the data model (the insert sites,
source_path, and the id generation inparser.rs); haven't compiled it locally.Fixes #417