fix(skills): align skills API with TUI command multi-directory discovery#2285
Conversation
- Use discover_for_workspace_and_dir() instead of SkillRegistry::discover() to search all skill directories (workspace + global), matching TUI /skills - Add is_bundled field to SkillEntry for built-in skill identification - Add directories field to SkillsResponse showing all search paths - Use skill.path instead of constructing path from skills_dir + name - Update set_skill_enabled to use the same discovery logic
There was a problem hiding this comment.
Code Review
This pull request updates the runtime API to support multiple skill directories and bundled skills. It adds a directories field and an is_bundled flag to the skills response, and updates skill discovery to search across the workspace. A review comment points out that the directories list returned in the API response might omit a custom skills_dir if it is not already part of the default workspace directories, and suggests appending it to ensure consistency with the actual search paths.
| let registry = | ||
| crate::skills::discover_for_workspace_and_dir(&state.workspace, &skills_dir); | ||
| let skill_state = state.skill_state.lock().await; | ||
| let directories = crate::skills::skills_directories(&state.workspace); |
There was a problem hiding this comment.
The directories list returned in SkillsResponse is populated using crate::skills::skills_directories(&state.workspace). However, discover_for_workspace_and_dir also searches skills_dir (the primary configured/resolved skills directory) and appends it to the search paths if it is not already present.
If a user has configured a custom skills_dir that is not in the default workspace/global candidate list, it will be searched for skills, but it will be missing from the directories field in the API response.
To ensure the returned directories list accurately reflects all directories that were actually searched, we should append skills_dir to directories if it is a valid directory and not already present, matching the behavior of discover_for_workspace_and_dir.
let mut directories = crate::skills::skills_directories(&state.workspace);
if skills_dir.is_dir() && !directories.contains(&skills_dir) {
directories.push(skills_dir.clone());
}|
Thanks for aligning the skills API with the TUI multi-directory discovery path. I am keeping this open because the API response can still become inconsistent: Please mirror the same append logic when building |
|
Thanks @gaord — I pushed a maintainer update that keeps your multi-directory skills API direction and closes the two review gaps. What changed:
Local verification on the updated head:
Really useful parity fix; this should be ready once the refreshed PR checks come back. |
|
One more small maintainer pass before merge: I fixed the |
Summary
The
/v1/skillsAPI endpoint only searched a single directory (resolve_skills_dir), while the TUI's/skillscommand usesdiscover_for_workspace_and_dir()to search multiple directories (workspace-local + global). This means the API would miss skills installed in~/.claude/skills,~/.agents/skills, workspace.agents/skills, etc.Changes
discover_for_workspace_and_dir()in bothlist_skillsandset_skill_enabledhandlers, matching the TUI/skillscommand behavioris_bundledfield toSkillEntry— identifies built-in skills viais_bundled_skill_name(), allowing clients to distinguish user skills from bundled skillsdirectoriesfield toSkillsResponse— returns all searched directories so clients know where skills come fromskill.pathinstead of constructing path fromskills_dir + name, since skills may come from different directoriesSkillRegistryimportBefore
{ "directory": "/Users/user/.codewhale/skills", "warnings": [], "skills": [ { "name": "delegate", "description": "...", "path": "/Users/user/.codewhale/skills/delegate/SKILL.md", "enabled": true } ] }Only skills under
~/.codewhale/skillswere discoverable.After
{ "directory": "/Users/user/.codewhale/skills", "directories": [ "/Users/user/project/.agents/skills", "/Users/user/.agents/skills", "/Users/user/.claude/skills", "/Users/user/.codewhale/skills", "/Users/user/.deepseek/skills" ], "warnings": [], "skills": [ { "name": "delegate", "description": "...", "path": "/Users/user/.codewhale/skills/delegate/SKILL.md", "enabled": true, "is_bundled": true }, { "name": "my-custom-skill", "description": "...", "path": "/Users/user/.claude/skills/my-custom-skill/SKILL.md", "enabled": true, "is_bundled": false } ] }Skills from all configured directories are now discoverable, matching TUI behavior.
Test plan
/v1/skillsreturns skills from~/.claude/skills,~/.agents/skills, etc.is_bundledistruefor built-in skills (delegate, documents, pdf, etc.)directorieslists all existing skill directoriesset_skill_enabledworks for skills from any directoryset_skill_enabledreturns 404 for non-existent skillsGreptile Summary
This PR aligns the
/v1/skillsREST API with the TUI/skillscommand by switching from single-directory discovery to the fulldiscover_for_workspace_and_dirmulti-directory strategy. It also addsis_bundledper entry and adirectoriesarray to the response.list_skillsandset_skill_enablednow use the newdiscover_skills_for_runtime_apihelper, which mirrors the TUI's workspace + global search order and appends the configuredskills_dirwhen it is outside the standard set.is_bundledfield: intended to distinguish first-party bundled skills from user skills; computed viaskill_entry_is_bundledwhich checks name againstBUNDLED_SKILLSand path against the resolvedskills_dir— but see the inline comment for a correctness issue with that path.directoriesfield: reports every directory that was actually searched, built with the same append-if-missing logic used internally by discovery.Confidence Score: 4/5
Safe to merge with one fix:
is_bundledwill be wrong for bundled skills in any workspace that has a local.agents/skillsorskillsdirectory.The multi-directory discovery wiring is correct and well-tested. The one defect is in
skill_entry_is_bundled: it compares skill paths againstresolve_skills_dir(), which prefers workspace-local directories, but bundled skills are always installed toconfig.skills_dir(). The mismatch means the newis_bundledfield will silently returnfalsefor every genuine bundled skill whenever a workspace-local skills directory is present.crates/tui/src/runtime_api.rs — specifically the
skill_entry_is_bundledcall inlist_skillsand theskills_dirargument passed to it.Important Files Changed
/v1/skillsAPI with multi-directory discovery; addsis_bundledanddirectoriesfields to response.is_bundledlogic incorrectly usesresolve_skills_dir()(which may be workspace-local) instead of the globalconfig.skills_dir()where bundled skills are actually installed, causing bundled skills to be misreported when a workspace-local skills directory is present.discover_for_workspace_dirs_and_dirto extract a newpub(crate) discover_from_directorieshelper; a clean, minimal change with no issues.Sequence Diagram
sequenceDiagram participant Client participant list_skills participant resolve_skills_dir participant skills_search_directories participant discover_from_directories participant skill_entry_is_bundled Client->>list_skills: GET /v1/skills list_skills->>resolve_skills_dir: config, workspace resolve_skills_dir-->>list_skills: skills_dir (workspace-local or config.skills_dir()) list_skills->>skills_search_directories: workspace, skills_dir skills_search_directories->>skills_search_directories: skills_directories(workspace) + append skills_dir if new skills_search_directories-->>list_skills: directories[] list_skills->>discover_from_directories: directories.clone() discover_from_directories-->>list_skills: SkillRegistry (merged, first-match-wins) loop for each skill list_skills->>skill_entry_is_bundled: skill, skills_dir (should be config.skills_dir()) skill_entry_is_bundled-->>list_skills: is_bundled bool end list_skills-->>Client: SkillsResponseReviews (3): Last reviewed commit: "fix(runtime): identify bundled skill ent..." | Re-trigger Greptile