Skills: deterministic invocation — /<skill-name>, searchable /skills picker, discovery-budget fix#520
Conversation
Skills were model-pulled only: the model had to notice a skill in a
640-byte system-prompt list (~6 skills visible; the rest collapsed into
'…and N more') and call the skill tool on its own. Users could not run a
skill at all — typing /<skill-name> reported 'unknown command'. The two
halves of the fix:
User invocation
- /<skill-name> [args] now runs an installed skill directly: resolved
after builtins and user commands, the skill body is inlined as the
prompt with the typed args appended, and submitted through the same
launch path user commands use.
- /skills lists installed skills with their slash form, live from disk.
Labels are honest: a skill shadowed by a builtin, alias, user command,
or a case-colliding sibling is listed bare with a '(via skill tool)'
note instead of advertising a dead /name.
- The slash palette suggests skills (labeled '(skill)') read through the
same loader dispatch uses, so mid-session installs appear and shadowed
or case-colliding names are never advertised.
- Skill and user-command invocations now apply the plain-prompt run-state
guards: mid-run submissions queue the EXPANDED prompt (the queue flush
submits literal text), compaction-in-flight warns, exiting drops.
- Skills resolve through a lazy plugin-aware loader (default dir merged
with plugin skill roots — the same set the skill tool and the system
prompt see), TTL-cached so palette keystrokes don't re-read SKILL.md
bodies.
Model triggering
- Raise the skills-list budget 640 -> 4096 bytes: the list is the model's
only discovery surface, and the old cap made skills past ~6 invisible —
invocation reliability collapsed exactly for users invested in skills.
- Raise description truncation 100 -> 200 runes: the description carries
the trigger conditions ('use when…'); cutting them is what stopped the
model from firing the skill.
- Strengthen the instruction line: scan the list before acting, call the
matching skill FIRST, don't substitute your own approach.
Replace the static /skills card with the same picker UX /model uses: type-to-filter (ranked search), arrow-key selection, Enter runs the chosen skill immediately. Selection is by the skill's exact name, so skills the slash path cannot reach — names shadowed by a builtin or user command, or names that don't fit a slash token — are still runnable from the picker, where the choice is unambiguous. The text card remains only as the no-skills install hint, and the picker path applies the same run-state guards as slash invocation.
A skill invoked with no request submits only its body — instructions
with no target ("review the PR" — which PR?) — and the model improvises
a target instead of asking, which is especially wrong in a fresh
session. Bare invocations (slash and picker) now append a conditional
note: if the instructions need a target or details not already clear
from the conversation, ask first; self-contained skills still just run.
Invocations that carry a request are unchanged.
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
WalkthroughThis PR adds installed-skill support to the TUI, including cached loading, a ChangesSkills invocation and TUI integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (2)
internal/tui/autocomplete.go (1)
187-193: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNo overall cap on combined suggestion list.
Each source (
matchCommandSuggestionsWithFilter,matchUserCommandSuggestions,matchSkillSuggestions) independently truncates tomaxCommandSuggestions(32), but the mergedouthere isn't re-capped. With skills now added as a third source, the palette can surface up to ~96 rows for a single prefix.💡 Proposed fix to cap the merged result
func (m model) matchCommandSuggestions(token string) []commandSuggestion { out := matchCommandSuggestionsWithFilter(token, func(command commandDefinition) bool { return command.kind != commandSandboxSetup || m.sandboxSetupCommand != nil }) out = append(out, m.matchUserCommandSuggestions(token)...) - return append(out, m.matchSkillSuggestions(token)...) + out = append(out, m.matchSkillSuggestions(token)...) + if len(out) > maxCommandSuggestions { + out = out[:maxCommandSuggestions] + } + return out }🤖 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 `@internal/tui/autocomplete.go` around lines 187 - 193, The merged autocomplete list in matchCommandSuggestions can exceed the intended overall limit because it appends results from matchCommandSuggestionsWithFilter, matchUserCommandSuggestions, and matchSkillSuggestions without reapplying maxCommandSuggestions. Add a final cap after combining all three sources so the returned []commandSuggestion never exceeds the shared maximum, keeping the palette size bounded regardless of how many sources match.internal/tui/skill_commands_test.go (1)
177-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWeak assertion: checks formatted
Desctext, not absence of the shadowedName.This test only fails if a suggestion's
Descexactly equals"Skill: /help"etc. If the default-description format inmatchSkillSuggestions(Line 223) ever changes, this test silently stops verifying the actual invariant (that shadowed names aren't advertised at all), while still passing.✅ Proposed tightening to assert on Name directly
for _, token := range []string{"/hel", "/ses", "/dep"} { for _, s := range m.matchCommandSuggestions(token) { - if s.Desc == "Skill: /help" || s.Desc == "Skill: /sessions" || s.Desc == "Skill: /deploy" { + if s.Name == "/help" || s.Name == "/sessions" || s.Name == "/deploy" { t.Fatalf("shadowed skill advertised for %q: %#v", token, s) } } }🤖 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 `@internal/tui/skill_commands_test.go` around lines 177 - 204, The shadowed-skill test is asserting against the rendered Desc string instead of the suggestion identity, so it can miss regressions if formatting changes. Update TestShadowedSkillNotSuggested to verify that matchCommandSuggestions does not return any suggestion whose Name corresponds to the shadowed skills (/help, /sessions, /deploy), rather than matching on Desc text. Keep the existing token loop and use the suggestion Name field as the invariant, with matchSkillSuggestions/matchCommandSuggestions as the key locations to inspect.
🤖 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 `@internal/tui/skill_commands.go`:
- Around line 65-88: The compactInFlight branch in launchOrDeferExpandedPrompt
loses the original typed invocation because commandUnknown clears the composer
before handleSkillCommand/handleUserCommand runs. Thread the raw invocation
through the skill/user-command dispatch path, and when compactInFlight is true
restore that text back into the composer before returning so the user can re-run
it without retyping; update the relevant call sites around commandUnknown,
invokeSkillByName, and the user command handling to pass the invocation along.
- Around line 169-177: The installedSkills() path is still performing a
synchronous disk refresh through m.loadSkills(), which can block the Bubble Tea
Update flow on cache misses. Update the model/loader wiring so skill refreshes
happen asynchronously or are preloaded outside the TUI hot path, and have
installedSkills() read from a cached snapshot instead of waiting on
internal/cli/app.go’s loader. Keep the behavior for bare sessions intact while
ensuring /skills, autocomplete, and direct skill invocation no longer stall in
model.installedSkills().
---
Nitpick comments:
In `@internal/tui/autocomplete.go`:
- Around line 187-193: The merged autocomplete list in matchCommandSuggestions
can exceed the intended overall limit because it appends results from
matchCommandSuggestionsWithFilter, matchUserCommandSuggestions, and
matchSkillSuggestions without reapplying maxCommandSuggestions. Add a final cap
after combining all three sources so the returned []commandSuggestion never
exceeds the shared maximum, keeping the palette size bounded regardless of how
many sources match.
In `@internal/tui/skill_commands_test.go`:
- Around line 177-204: The shadowed-skill test is asserting against the rendered
Desc string instead of the suggestion identity, so it can miss regressions if
formatting changes. Update TestShadowedSkillNotSuggested to verify that
matchCommandSuggestions does not return any suggestion whose Name corresponds to
the shadowed skills (/help, /sessions, /deploy), rather than matching on Desc
text. Keep the existing token loop and use the suggestion Name field as the
invariant, with matchSkillSuggestions/matchCommandSuggestions as the key
locations to inspect.
🪄 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
Run ID: 99354357-ecba-4d31-b3db-90d9af46e305
📒 Files selected for processing (13)
internal/agent/skills_context_test.gointernal/agent/system_prompt.gointernal/cli/app.gointernal/tui/autocomplete.gointernal/tui/command_views.gointernal/tui/commands.gointernal/tui/model.gointernal/tui/options.gointernal/tui/picker.gointernal/tui/skill_commands.gointernal/tui/skill_commands_test.gointernal/tui/user_commands.gozero-skills
…guard, cap merged suggestions - The compaction-in-flight guard for skill/user-command invocations fired after the dispatch path had already cleared the composer, so the typed "/name args" was lost and had to be retyped; thread the raw invocation through and restore it into the composer (the plain-prompt guard keeps the composer intact by firing before the clear). The picker path has no typed form and restores nothing. - Cap the merged autocomplete list at the shared bound: each source self-caps, but three sources could stack to ~3x the palette limit. - Tighten the shadowed-skill palette test to assert on the advertised Name rather than the fallback Desc wording.
|
Addressed the review: the compaction-guard text-loss is fixed (raw invocation threaded through and restored into the composer, with a test pinning it), the merged suggestion list is now capped at the shared bound, and the shadowed-skill test asserts on the advertised Name. The synchronous cache-miss load is kept as-is with reasoning on the thread — bounded I/O, TTL-limited, consistent with existing patterns. @coderabbitai review |
|
✅ Action performedReview finished.
|
|
@CodeRabbit approve |
✅ Action performedComments resolved and changes approved. |
|
@kevincodex1 check it out |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
- [P2] Remove the committed build artifact
zero-skills
This PR adds a 30 MB Mach-O arm64 executable at the repository root, but the change is otherwise source-level Go/TUI work and the binary is not referenced by the command wiring, package metadata, or release scripts. Keeping a prebuilt platform-specific executable in source history bloats every clone and bypasses the normal build/release provenance for a file that Linux and Windows users cannot run anyway. Please removezero-skillsfrom the PR, and if a local build command produced it, add that generated name to the ignored build-artifact patterns or build into the existing ignored output path instead.
A locally built test binary under this name was accidentally committed (and has been scrubbed from the branch history); ignore the name so it cannot recur.
gnanam1990
force-pushed
the
feat/skill-invocation
branch
from
82b3583 to
c427052
Compare
|
@jatmn addressed — and thanks for the catch, that was a locally built test binary swept in by a broad |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Reviewed the whole thing and it holds up — approving. This solves a real problem (skills that only fired if the model happened to notice them in a truncated list), and the dispatch design is careful.
Precedence is right: parseCommand takes builtins, then commandUnknown falls through handleUserCommand → handleSkillCommand → "unknown," so it's builtin > user command > skill exactly as documented, and a shadowed skill name never reaches dispatch.
The guard refactor is the sharp bit. launchOrDeferExpandedPrompt queues the EXPANDED prompt rather than the raw /name — right call, since the queue flush resubmits literal text, so queuing /name args would have sent it to the model as prose. And routing user commands through the same path (user_commands.go:35) closes the pre-existing hole where a user command could kick off a second concurrent run mid-turn. Good catch folding that in.
The rest is thoughtful too: an empty SKILL.md body surfaces a real error instead of "unknown command," the bare-invocation ask-first note is conditional so self-contained skills still run, and the skills block only renders when skills are installed so skill-less prompts stay byte-identical (the prompt-budget ratchet is untouched). The 640 → 4096 discovery budget is a deliberate, prompt-cache-friendly cost for heavy-skill users — reasonable, since an invisible skill is a broken skill.
On the binary: jatmn's zero-skills flag is resolved — it's out of the tree and /zero-skills is in .gitignore now (that review predates the cleanup commit). Build's clean, tui + agent suites pass here, and CI is green across all nine checks.
Good work. Good to merge.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Independently verified from source, not just the review threads: checked out the branch in an isolated worktree, go build/go vet clean, and -race passes on all 4 touched packages (agent/tui/cli/plugins).
Confirmed directly against code (not just taking the PR body's word for it):
- Precedence (builtin > user command > skill) matches
handleSubmit's actual dispatch order. - The compaction-guard fix is correctly targeted —
commandUnknownclears the composer unconditionally before reaching skill/user-command dispatch (unlike the plain-prompt path, which checkscompactInFlightfirst), so threadingrawthrough to restore it is the right fix, not a workaround. zero-skillsis gone from every commit in the PR's range, not just the tip — checked viagit rev-list+ blob-size scan across the full range.cachedSkillsLoader's TTL isn't defeated by the zero-skills case (checked whethermergeSkillscould returnnilthere — it always inits[]skills.Skill{}, so the cache holds).- Real bonus fix:
handleUserCommandhad no run-state guard at all before this PR — a pre-existing hole where a user command could start a second concurrent run, closed as a side effect.
One non-blocking gap for a follow-up: the merged-suggestion-list cap (autocomplete.go's re-cap after combining command/user-command/skill sources) is correct by inspection but has no regression test constructing a >32-combined-match scenario. All existing tests check functional correctness (shadowing, case-collision) not this scale invariant — worth pinning so a future "simplification" can't silently drop the cap.
Good to merge.
4 participants
Summary
Makes skill invocation deterministic and discoverable. Direct user feedback: "Skill invocations — fix how they get invoked and I would happily keep using it."
Skills were model-pulled only: the model had to notice a skill in a 640-byte system-prompt list (~6 skills visible; the rest collapsed into "…and N more") and decide to call the skill tool on its own. Users could not run a skill at all — typing
/<skill-name>reported "unknown command", because skills and.zero/commandsuser commands were two disconnected systems. The more skills someone installed, the less reliably any of them fired.Three commits, layered:
1.
feat(skills)— deterministic invocation + discovery fixUser invocation
/<skill-name> [args]runs an installed skill directly: resolved after builtins and user commands (precedence: builtin > user command > skill), the skill body is inlined as the prompt with the typed request appended, submitted through the same launch path user commands use.(skill)), read through the same loader dispatch uses — mid-session installs appear without a restart; names shadowed by a builtin/alias/user command or by a case-colliding sibling are never advertised (a shadowed row would be dead at dispatch time)./namewould have sent it to the model as prose), compaction-in-flight warns, exiting drops. This also closes a pre-existing hole where a user command could start a second concurrent run.tui.Options.LoadSkillsclosure backed byplugins.MergedSkillsLoaded— the same merged set (default dir + plugin skill roots) the skill tool and the system prompt resolve against — TTL-cached (2s) so palette keystrokes don't re-readSKILL.mdbodies.Model triggering
2.
feat(tui)—/skillsopens a searchable picker/skillsnow behaves like/model: type-to-filter (ranked search), arrow keys, Enter. Selection fills the composer with/name(cursor at the end) so the user adds their request before submitting — it does not fire the skill blind. Skills whose names can't be typed as a slash command (non-slash shapes, or names shadowed by a builtin/user command) run immediately on selection instead: the picker is the only path that reaches them, and picking from the Skills list is unambiguous. The old text card remains only as the no-skills install hint.3.
feat(tui)— ask-first guard for bare invocationsA skill invoked with no request submits only its body — instructions with no target ("review the PR" — which PR?) — and the model improvised a target instead of asking, which is especially wrong in a fresh session. Bare invocations (slash and picker) now append a conditional note: if the instructions need a target or details not already clear from the conversation, ask first; self-contained skills (e.g. one that reads the staged diff) still just run. Invocations that carry a request are unchanged.
Notes for reviewers
bareSkillInvocationNote) and is only attached whenargs == "".Testing
go build ./...·go vet ./...·gofmtclean ·go test ./... -race→ 74 packages, 0 failures, 0 races (rebased on latestmainincluding fix: Termux/Android support — PRoot scroll, SIGSYS sandbox, build docs #509/feat(openai): forward prompt_cache_key for server-side prefix cache routing #515/feat(agent): guard against over-engineering the solution in the editing discipline #517).Summary by CodeRabbit
/skillscommand with a picker to browse and launch installed skills, including a clear “no skills installed” fallback with an install hint.