skills: group index by category with category descriptions - #640
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughSkills now receive directory-derived categories and optional descriptions. Prompt rendering groups and sorts skills by category. Channel, branch, and worker templates display category metadata and require broader skill discovery. Skill listing responses now include categories. ChangesCategorized skills
Estimated code review effort: 3 (Moderate) | ~25 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 |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| set.skills.insert(skill.name.to_lowercase(), skill); | ||
| } | ||
| for (cat, desc) in descriptions { | ||
| set.category_descriptions.entry(cat).or_insert(desc); |
There was a problem hiding this comment.
The new description map should follow the same precedence as the skills themselves. With or_insert, an instance category description wins even when the workspace defines the same category and its skills override instance skills.
| set.category_descriptions.entry(cat).or_insert(desc); | |
| set.category_descriptions.insert(cat, desc); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/prompts/engine.rs (1)
128-138: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEscape all dynamic values before rendering the XML skill index.
The extensionless MiniJinja registrations disable auto-escaping. Installed and user-created skills can provide values that change the prompt structure or inject instructions. Escape
category.name,category.description,skill.name, andskill.descriptionin all three skill templates.🤖 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/prompts/engine.rs` around lines 128 - 138, Update the three skill templates to XML-escape category.name, category.description, skill.name, and skill.description before rendering, since the registrations in src/prompts/engine.rs (lines 128-138) disable auto-escaping; apply the changes in prompts/en/fragments/skills_branch.md.j2 (lines 8-16), prompts/en/fragments/skills_channel.md.j2 (lines 12-20), and prompts/en/fragments/skills_worker.md.j2 (lines 8-16), with no direct change required in engine.rs.
🧹 Nitpick comments (1)
src/skills.rs (1)
170-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse complete variable names for category metadata.
Rename
cattocategoryanddesctodescription. This also avoids the nesteddescshadowing in Lines 580-583.As per coding guidelines, “Don't abbreviate variable names. Use
queuenotq,messagenotmsg,channelnotch.”Also applies to: 183-184, 580-583
🤖 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/skills.rs` around lines 170 - 171, Rename the category metadata loop variables in the descriptions handling and the corresponding locations at lines 183-184 and 580-583: change cat to category and desc to description, updating all references consistently to eliminate shadowing and comply with complete variable naming.Source: Coding guidelines
🤖 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/skills.rs`:
- Around line 170-184: Update the workspace category-description merge in the
skill-loading flow to overwrite existing entries rather than retaining them, so
workspace descriptions take precedence over instance descriptions. Change the
descriptions loop in the workspace-skills block while leaving skill merging and
lower-precedence behavior unchanged.
- Around line 164-166: Update the skill-loading branches around
load_skills_from_dir and the other let Ok(...) sites at the referenced locations
to handle Err results instead of silently discarding them. Log directory-read,
file-read, frontmatter-parse, and index.md loading errors with the relevant
source/path context, matching the error-reporting approach used by
load_skill_into while preserving successful loading behavior.
---
Outside diff comments:
In `@src/prompts/engine.rs`:
- Around line 128-138: Update the three skill templates to XML-escape
category.name, category.description, skill.name, and skill.description before
rendering, since the registrations in src/prompts/engine.rs (lines 128-138)
disable auto-escaping; apply the changes in
prompts/en/fragments/skills_branch.md.j2 (lines 8-16),
prompts/en/fragments/skills_channel.md.j2 (lines 12-20), and
prompts/en/fragments/skills_worker.md.j2 (lines 8-16), with no direct change
required in engine.rs.
---
Nitpick comments:
In `@src/skills.rs`:
- Around line 170-171: Rename the category metadata loop variables in the
descriptions handling and the corresponding locations at lines 183-184 and
580-583: change cat to category and desc to description, updating all references
consistently to eliminate shadowing and comply with complete variable naming.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a6e13daa-1422-4389-8f14-85bf0c4d6968
📒 Files selected for processing (8)
prompts/en/fragments/skills_branch.md.j2prompts/en/fragments/skills_channel.md.j2prompts/en/fragments/skills_worker.md.j2src/api/skills.rssrc/prompts/engine.rssrc/skills.rssrc/skills/builtin.rssrc/tools/skills_list.rs
| && let Ok((skills, descriptions)) = | ||
| load_skills_from_dir(instance_skills_dir, SkillSource::Instance).await | ||
| { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle loader errors instead of discarding them.
These let Ok(...) branches discard directory-read, file-read, and frontmatter-parse errors. A failed source load silently omits all skills from that source. A failed index.md load silently omits its category description.
Log these errors at minimum, as load_skill_into already does for individual skills.
As per coding guidelines, “Don't silently discard errors. No let _ = on Results. Handle them, log them, or propagate them.”
Also applies to: 177-179, 580-584
🤖 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/skills.rs` around lines 164 - 166, Update the skill-loading branches
around load_skills_from_dir and the other let Ok(...) sites at the referenced
locations to handle Err results instead of silently discarding them. Log
directory-read, file-read, frontmatter-parse, and index.md loading errors with
the relevant source/path context, matching the error-reporting approach used by
load_skill_into while preserving successful loading behavior.
Source: Coding guidelines
| for (cat, desc) in descriptions { | ||
| set.category_descriptions.entry(cat).or_insert(desc); | ||
| } | ||
| } | ||
|
|
||
| // Workspace skills (highest precedence, overrides instance) | ||
| if workspace_skills_dir.is_dir() | ||
| && let Ok(skills) = | ||
| && let Ok((skills, descriptions)) = | ||
| load_skills_from_dir(workspace_skills_dir, SkillSource::Workspace).await | ||
| { | ||
| for skill in skills { | ||
| set.skills.insert(skill.name.to_lowercase(), skill); | ||
| } | ||
| for (cat, desc) in descriptions { | ||
| set.category_descriptions.entry(cat).or_insert(desc); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Apply workspace precedence to category descriptions.
or_insert retains the instance description when the workspace defines the same category. This conflicts with the documented Builtin < Instance < Workspace precedence. A workspace skill can therefore render with an instance index.md description.
Overwrite existing entries when merging workspace descriptions.
🤖 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/skills.rs` around lines 170 - 184, Update the workspace
category-description merge in the skill-loading flow to overwrite existing
entries rather than retaining them, so workspace descriptions take precedence
over instance descriptions. Change the descriptions loop in the workspace-skills
block while leaving skill merging and lower-precedence behavior unchanged.
1 participant
Phase 1 of the bundled skills index shape (from
docs/design-docs/bundled-skills.md§ "The index is grouped by category").Four changes:
Category on every struct.
Skill, bothSkillInfotypes (public + API + prompts), andSkillsListEntryall carrycategory: String. Derived from the directory path at load time — no new frontmatter field.Category captured during loading.
load_skills_from_diralready scanned two levels deep for category directories but discarded the name. Now it threads the category throughload_skill_into→load_skill→Skill. Top-level skills get"general". Categoryindex.mdfiles are parsed for adescriptionfield and passed through to template rendering.Grouped rendering.
SkillCategoryGroupin the prompts engine groups skills by category, sorted alphabetically. All three fragments (skills_channel,skills_branch,skills_worker) render as nested<category>elements instead of flat<skill>lists.Firmer load directive. The preamble in all three fragments now instructs: scan before acting, read any partially-relevant skill, and prefer reading an unnecessary skill over missing established procedure.