fix(core,tui): support directory-based skills, tilde paths, and dynamic tui autocomplete - #401
fix(core,tui): support directory-based skills, tilde paths, and dynamic tui autocomplete#401bobbyunknown wants to merge 1 commit into
Conversation
…ui autocomplete - scan subdirectories containing SKILL.md and skill.md - expand tilde (~) in configured skill paths - add global ~/.agents/skills/ to search locations - populate discovered skills into TUI slash command autocomplete and palette - pass arguments to skills without template placeholders
Kuberwastaken
left a comment
There was a problem hiding this comment.
Thanks for this — the underlying problem is real and I want the fix. commands_from_discovered_skills() in crates/commands/src/lib.rs has zero callers, so skills have only ever been reachable through the execute_command fallback and never showed up in autocomplete or the palette. And you're right that directory-based skills are the inconsistency: docs/plugins.md already documents skill dirs as "each must contain a SKILL.md", but skill_discovery.rs only ever looked at flat *.md. Good catch.
A few things before this can land.
1. Please drop the reformatting. The diff reads as +4525/−3205, but when I normalize both sides with rustfmt the actual change is about 387 lines:
| file | as shown | actual |
|---|---|---|
commands/src/lib.rs |
+2017/−1877 | +23/−11 |
core/src/skill_discovery.rs |
+612/−405 | +208/−19 |
tui/src/app.rs |
+1221/−466 | +110/−3 |
cli/src/main.rs |
+591/−409 | +9/−0 |
tui/messages/markdown_enhanced.rs |
+84/−48 | +2/−2 |
Two of those files are CRLF on main and got rewritten to LF, so they show up with no context lines at all. This is deliberate on our side — see the note in .github/workflows/ci.yml: the tree is intentionally not fmt-clean (CRLF files and pre-existing drift), which is why cargo fmt --check is advisory rather than enforced. Could you rebuild the branch touching only the lines you actually changed, and leave line endings alone? markdown_enhanced.rs can drop out entirely — its only real change is removing two & before format!, which is unrelated to this PR.
This matters beyond tidiness: the reformatting turns every other queued PR touching cli/main.rs, app.rs, and commands/lib.rs into a manual rebase.
2. Two bugs in resolve_path. On Windows, ~\.agents\skills doesn't match strip_prefix("~/") and then find('/') returns None, so it falls through to returning bare $HOME — which means we'd scan the user's entire home directory for skills on every launch. Worth using std::path::is_separator so both separators work. Separately, ~alice/skills currently resolves to $HOME/skills, silently dropping the username; better to leave ~user paths unexpanded than to resolve them to the wrong home. The new test_resolve_path only covers ~/, so neither case is caught.
3. The early return in scan_dir. If a directory contains its own SKILL.md, the function returns just that one skill and skips everything else in the directory. That bites fetch_git_skills, which calls scan_dir(&repo_dir) — a skills repo with a root SKILL.md plus sibling .md files would lose the siblings. Could it collect rather than return early?
4. Discovery in App::new is blocking. discover_skills walks from cwd to the filesystem root, and fetch_git_skills shells out to a synchronous git clone. On a first launch with skills.urls configured, the terminal will freeze with no feedback. Could this move off the construction path, or at least skip the git fetch there? Relatedly, refresh_prompt_input now calls all_slash_commands() on every keystroke, which rebuilds ~60 String pairs with an O(n·m) dedup each time — worth caching the built list and only rebuilding when discovered_skills changes.
5. One thing to flag in the description. The args-append change lands on TemplateCommand too, not just SkillCommand — so existing user-defined [commands] templates without placeholders will start getting args appended. I think that's the right behavior, but it's a change to an existing feature and should be called out. Minor: the guard checks $ARGUMENTS and $1 but not $2, so a $2-only template takes the append branch and leaves $2 unsubstituted.
Also heads-up on #399 — it adds its own discover_skills call in App::new for the status-bar count. Once your discovered_skills field exists it can just read .len() off it, so we should sequence those two (yours first).
Sequencing: I'm landing #403 (the app.rs → app/ module split) first, since every open TUI PR needs a revision round anyway. When you rebuild the branch, please target the new layout — the TUI half of this will live in app/ modules (autocomplete around app/prompt_handling, fields in app/mod.rs). Happy to help you re-target if anything's unclear. The core logic is good — mostly asking for a clean diff and the path fixes.
|
Rebased on post-#403 main with a clean diff (no reformatting, no CRLF/LF churn). 5 files, +266/-22 lines. 1. Reformatting dropped. All changes preserve original line endings. 2. 3. 4. Discovery off 5. TemplateCommand args-append: Unchanged from the original PR —
|
…g changes - scan_dir now detects SKILL.md/skill.md in subdirectories (e.g. skills/<name>/SKILL.md) - parse_skill_file uses parent dir name when file stem is 'skill' (case-insensitive) - hidden files/dirs starting with '.' are skipped during directory scans - App gains discovered_skills field and refresh_discovered_skills/rebuild_command_palette/rebuild_help_overlay methods - main.rs calls refresh after /move, config updates, and ConfigChangeMessage - all_slash_commands merges built-in, custom templates, and discovered skills - CRLF line endings preserved in skill_discovery.rs closes Kuberwastaken#401
|
This PR has merge conflicts with main due to the The rebase preserves CRLF line endings in Closing in favor of #409. |
…utocomplete - scan_dir now handles subdirectories with SKILL.md (directory-based skills) - parse_skill_file uses parent dir name when file stem is 'skill' - resolve_path expands ~ and ~/ paths, leaves ~user unexpanded, handles both / and backslash separators for Windows - discover_skills adds ~/.agents/skills/ as a global source - commands_from_discovered_skills is now wired into autocomplete via all_slash_command_names, so skills appear in the command palette - Discovered slash commands cached on App and merged at prompt update time, not re-discovered per keystroke - No CRLF/LF churn: all changes preserve original line endings Addresses review feedback on PR Kuberwastaken#401: 1. Rebased on post-Kuberwastaken#403 main, no reformatting 2. resolve_path handles ~user paths (left unexpanded) and Windows separators 3. scan_dir collects rather than returning early on SKILL.md 4. Discovery runs once at startup (not in App::new, not per keystroke) 5. all_slash_command_names cached, not rebuilt per keystroke
Summary
This PR addresses issues with skill discovery and TUI slash command handling:
scan_dirnow discovers skills housed in subdirectories withSKILL.md(orskill.md), as well as directories pointing directly to a skill folder. It also falls back to the parent directory name whenSKILL.mddoes not specifyname:in frontmatter.skills.paths(e.g.~/.agents/skills) now expand~to the user's home directory.~/.agents/skills/to default global discovery locations.PromptInputStatesuggestion list,Command Palette(Ctrl+K), and help overlay to dynamically include discovered skills and custom templates alongside built-in commands.$ARGUMENTSor$1.Testing
claurst-corefor subdirectory scanning, parent directory naming, path resolution, and agent skill locations.cargo check --workspacepassedcargo clippy --workspace --all-targets -- -D warningspassed (0 warnings)cargo test --workspacepassed (all 658+ unit and integration tests)