From 15ddc79e75870dcc7baee48fa28004c85ffc7260 Mon Sep 17 00:00:00 2001 From: Gnanam Date: Sun, 5 Jul 2026 18:22:10 +0530 Subject: [PATCH 1/5] feat(skills): make skill invocation deterministic and discoverable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 / reported 'unknown command'. The two halves of the fix: User invocation - / [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. --- internal/agent/skills_context_test.go | 35 ++- internal/agent/system_prompt.go | 25 +- internal/cli/app.go | 32 +++ internal/tui/autocomplete.go | 52 +++- internal/tui/command_views.go | 83 ++++++ internal/tui/commands.go | 8 + internal/tui/model.go | 11 + internal/tui/options.go | 9 +- internal/tui/skill_commands.go | 121 +++++++++ internal/tui/skill_commands_test.go | 360 ++++++++++++++++++++++++++ internal/tui/user_commands.go | 5 +- 11 files changed, 725 insertions(+), 16 deletions(-) create mode 100644 internal/tui/skill_commands.go create mode 100644 internal/tui/skill_commands_test.go diff --git a/internal/agent/skills_context_test.go b/internal/agent/skills_context_test.go index 87073bb6..11228828 100644 --- a/internal/agent/skills_context_test.go +++ b/internal/agent/skills_context_test.go @@ -7,14 +7,18 @@ import ( ) func TestSkillsContextCapsLongList(t *testing.T) { - skills := make([]SkillInfo, 0, 40) - for i := 0; i < 40; i++ { + // 60 skills with realistic trigger-rich descriptions (~180 chars each, under + // the 200-rune truncation) total well past the 4096-byte list budget, so the + // overflow summary must kick in while the block stays bounded. + longDesc := strings.Repeat("use when the request touches deployments, release notes, or verification; ", 2) + "number " + skills := make([]SkillInfo, 0, 60) + for i := 0; i < 60; i++ { n := strconv.Itoa(i) - skills = append(skills, SkillInfo{Name: "skill-" + n, Description: "does something useful, number " + n}) + skills = append(skills, SkillInfo{Name: "skill-" + n, Description: longDesc + n}) } got := skillsContext(Options{Skills: skills}) - if len(got) > 1200 { - t.Fatalf("skills block should stay bounded, got %d bytes:\n%s", len(got), got) + if len(got) > skillsContextListBudget+1000 { + t.Fatalf("skills block should stay bounded near the %d-byte budget, got %d bytes:\n%s", skillsContextListBudget, len(got), got) } if !strings.Contains(got, "more (call skill") { t.Fatalf("expected an overflow summary line, got:\n%s", got) @@ -25,6 +29,27 @@ func TestSkillsContextCapsLongList(t *testing.T) { } } +// A realistic mid-size skill set (20 skills, trigger-rich descriptions) must be +// listed in FULL — no overflow summary. This pins the fix for the old 640-byte +// budget, under which skills past ~#6 were invisible to the model and therefore +// never triggered. +func TestSkillsContextListsRealisticSetInFull(t *testing.T) { + desc := "Use when the user asks about deployments, release notes, or pre-merge verification runs." + skills := make([]SkillInfo, 0, 20) + for i := 0; i < 20; i++ { + skills = append(skills, SkillInfo{Name: "skill-" + strconv.Itoa(i), Description: desc}) + } + got := skillsContext(Options{Skills: skills}) + if strings.Contains(got, "more (call skill") { + t.Fatalf("20 described skills must all be listed without overflow, got:\n%s", got) + } + for i := 0; i < 20; i++ { + if !strings.Contains(got, "- skill-"+strconv.Itoa(i)+":") { + t.Fatalf("skill-%d missing from the list:\n%s", i, got) + } + } +} + func TestSkillsContext(t *testing.T) { if got := skillsContext(Options{}); got != "" { t.Fatalf("no skills should yield an empty section, got %q", got) diff --git a/internal/agent/system_prompt.go b/internal/agent/system_prompt.go index 5cf2431e..1a492695 100644 --- a/internal/agent/system_prompt.go +++ b/internal/agent/system_prompt.go @@ -191,10 +191,19 @@ func specialistDelegationContext(options Options) string { // when no skills are installed, so a skill-less run reproduces the previous prompt // byte-for-byte. // skillsContextListBudget bounds the bytes spent listing individual skills so a -// workspace with dozens of them can't bloat every turn's prompt. Skills past the -// budget are summarized as a count rather than dropped — the model can still load -// any of them by name (an unknown name returns the full list from the skill tool). -const skillsContextListBudget = 640 +// workspace with an extreme number of them can't bloat every turn's prompt. +// Skills past the budget are summarized as a count rather than dropped — the +// model can still load any of them by name (an unknown name returns the full +// list from the skill tool). +// +// The budget is deliberately generous — roughly 18 skills with maximally long +// (200-rune, ASCII) descriptions, or 40+ with typical terser ones; non-ASCII +// descriptions consume it faster since the budget counts bytes. This list is +// the model's ONLY discovery surface, so a skill squeezed out of it effectively +// never triggers. The previous 640-byte cap silently broke invocation for +// anyone with more than ~6 skills — the model cannot match a request against a +// skill it cannot see. +const skillsContextListBudget = 4096 func skillsContext(options Options) string { if len(options.Skills) == 0 { @@ -202,7 +211,7 @@ func skillsContext(options Options) string { } var b strings.Builder b.WriteString("\n") - b.WriteString("Reusable, on-demand instruction sets you can load with the skill tool. When a request matches one, call skill with its exact name to pull in its guidance before acting — do not guess names or skip a relevant skill.\n") + b.WriteString("Reusable, on-demand instruction sets you can load with the skill tool. Before acting on a request, scan this list; when the request matches a skill's name or description, call skill with its exact name FIRST and follow the loaded guidance — do not guess names, do not skip a matching skill, and do not substitute your own approach for its instructions.\n") listed, spent, omitted := 0, 0, 0 for _, info := range options.Skills { name := strings.TrimSpace(info.Name) @@ -232,9 +241,11 @@ func skillsContext(options Options) string { } // truncateForSkillLine keeps a skill's one-line description short so a single -// verbose description can't dominate the skills-list budget. +// verbose description can't dominate the skills-list budget. The cap is roomy on +// purpose: the description carries the skill's trigger conditions ("use when…"), +// and truncating those is what stops the model from ever invoking the skill. func truncateForSkillLine(desc string) string { - const maxDescRunes = 100 + const maxDescRunes = 200 runes := []rune(desc) if len(runes) <= maxDescRunes { return desc diff --git a/internal/cli/app.go b/internal/cli/app.go index 3c5ba68f..28639ce8 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -12,6 +12,7 @@ import ( "os/exec" "path/filepath" "strings" + "sync" "time" "github.com/Gitlawb/zero/internal/agent" @@ -782,6 +783,14 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a Specialists: specialistRuntime.specialists, Skills: pluginActivation.skillInfos(deps.skillsDir()), }, + // LoadSkills backs /skills and direct / invocation in the TUI. + // It resolves against the same merged set (default dir + plugin skill + // roots) as the skill tool and the system-prompt list, re-read per use so + // newly installed skills work without a restart. + LoadSkills: cachedSkillsLoader(func() []skills.Skill { + merged, _ := plugins.MergedSkillsLoaded(deps.skillsDir(), pluginActivation.skillRoots) + return merged + }), PermissionMode: permissionMode, Notify: resolved.Notify, KeyBindings: resolved.KeyBindings, @@ -1277,3 +1286,26 @@ Flags: `) return err } + +// cachedSkillsLoader memoizes a skills loader for a short interval. The TUI +// calls the loader from autocomplete on every "/x" keystroke; without a cache +// each keystroke would re-read every SKILL.md body. Two seconds is fresh enough +// that a newly installed skill still shows up "immediately" while typing. +func cachedSkillsLoader(load func() []skills.Skill) func() []skills.Skill { + var ( + mu sync.Mutex + at time.Time + cached []skills.Skill + ) + const ttl = 2 * time.Second + return func() []skills.Skill { + mu.Lock() + defer mu.Unlock() + if cached != nil && time.Since(at) < ttl { + return cached + } + cached = load() + at = time.Now() + return cached + } +} diff --git a/internal/tui/autocomplete.go b/internal/tui/autocomplete.go index f2d37733..074c78db 100644 --- a/internal/tui/autocomplete.go +++ b/internal/tui/autocomplete.go @@ -188,7 +188,57 @@ func (m model) matchCommandSuggestions(token string) []commandSuggestion { out := matchCommandSuggestionsWithFilter(token, func(command commandDefinition) bool { return command.kind != commandSandboxSetup || m.sandboxSetupCommand != nil }) - return append(out, m.matchUserCommandSuggestions(token)...) + out = append(out, m.matchUserCommandSuggestions(token)...) + return append(out, m.matchSkillSuggestions(token)...) +} + +// matchSkillSuggestions returns installed skills whose slash name has the typed +// prefix, so skills are discoverable and invocable from the palette like user +// commands. Precedence (builtin > user command > skill) is enforced here by +// skipping any skill whose name is claimed by a builtin (or alias) or a user +// command — unlike the builtin/user pair, a shadowed skill row would be dead at +// dispatch time, so it must not be advertised at all. +func (m model) matchSkillSuggestions(token string) []commandSuggestion { + prefix := strings.ToLower(strings.TrimSpace(strings.TrimPrefix(token, "/"))) + if prefix == "" || m.loadSkills == nil { + return nil + } + taken := map[string]bool{} + for _, command := range commandDefinitions { + taken[strings.TrimPrefix(command.name, "/")] = true + for _, alias := range command.aliases { + taken[strings.TrimPrefix(alias, "/")] = true + } + } + for _, cmd := range m.userCommands { + taken[cmd.Name] = true + } + var out []commandSuggestion + // Read through the loader (not the startup snapshot in agentOptions) so the + // palette matches what dispatch will actually resolve — a skill installed or + // removed mid-session appears/disappears without a restart. + for _, skill := range m.installedSkills() { + name := skillSlashName(skill.Name) + if name == "" || taken[name] || !strings.HasPrefix(name, prefix) { + continue + } + // Two skills whose names collide after lowercasing (e.g. "Deploy" and + // "deploy") map to one slash name; only the first — the one dispatch will + // run — may be advertised, or the row's description and the executed + // instructions silently diverge. + taken[name] = true + desc := strings.TrimSpace(skill.Description) + if desc == "" { + desc = "Skill: /" + name + } else { + desc += " (skill)" + } + out = append(out, commandSuggestion{Name: "/" + name, Desc: desc}) + if len(out) >= maxCommandSuggestions { + break + } + } + return out } // matchUserCommandSuggestions returns file-sourced /commands whose name has the diff --git a/internal/tui/command_views.go b/internal/tui/command_views.go index f9a99f86..8e52dbcd 100644 --- a/internal/tui/command_views.go +++ b/internal/tui/command_views.go @@ -698,3 +698,86 @@ func (m model) debugText() string { }}, }) } + +// truncateSkillListDescription keeps a /skills row readable: the command card +// hard-truncates each rendered line at card width, so a very long trigger-rich +// description is cut deliberately (with an ellipsis) rather than arbitrarily. +func truncateSkillListDescription(desc string) string { + desc = strings.TrimSpace(desc) + const maxRunes = 140 + runes := []rune(desc) + if len(runes) <= maxRunes { + return desc + } + return strings.TrimSpace(string(runes[:maxRunes])) + "…" +} + +// skillsText renders the /skills listing: every installed skill (default dir +// merged with plugin skill roots), its slash-invocation form when the name fits +// a slash token, and how to run one. Skills are re-read on each invocation so a +// skill installed mid-session shows up without a restart. +func (m model) skillsText() string { + installed := m.installedSkills() + if len(installed) == 0 { + return renderCommandOutput(commandOutput{ + Title: "Skills", + Status: commandStatusInfo, + Sections: []commandSection{{ + Lines: []string{"No skills installed."}, + }}, + Hints: []string{ + "install one: create //SKILL.md (see `zero skills`)", + }, + }) + } + // A "/name" label is shown ONLY when typing it would actually run the skill: + // names shadowed by a builtin (or alias), a user command, or an earlier skill + // with the same lowercased name — and names that don't fit a slash token — + // are listed bare with a "(via skill tool)" note instead, so the listing + // never advertises a dead invocation. + taken := map[string]bool{} + for _, command := range commandDefinitions { + taken[strings.TrimPrefix(command.name, "/")] = true + for _, alias := range command.aliases { + taken[strings.TrimPrefix(alias, "/")] = true + } + } + for _, cmd := range m.userCommands { + taken[cmd.Name] = true + } + lines := make([]string, 0, len(installed)) + for _, skill := range installed { + name := strings.TrimSpace(skill.Name) + if name == "" { + continue + } + slash := skillSlashName(name) + invocable := slash != "" && !taken[slash] + if slash != "" { + taken[slash] = true + } + label := name + " -" + if invocable { + label = "/" + slash + " -" + } + if desc := truncateSkillListDescription(skill.Description); desc != "" { + label += " " + desc + } + if !invocable { + label += " (via skill tool)" + } + lines = append(lines, strings.TrimSuffix(label, " -")) + } + return renderCommandOutput(commandOutput{ + Title: "Skills", + Status: commandStatusOK, + Sections: []commandSection{{ + Title: "Installed", + Lines: lines, + }}, + Hints: []string{ + "run one directly: / [request]", + "the model can also load skills on demand via the skill tool", + }, + }) +} diff --git a/internal/tui/commands.go b/internal/tui/commands.go index b7edab32..0890a6b4 100644 --- a/internal/tui/commands.go +++ b/internal/tui/commands.go @@ -46,6 +46,7 @@ const ( commandCopy commandExport commandNew + commandSkills commandUnknown ) @@ -131,6 +132,13 @@ var commandDefinitions = []commandDefinition{ description: "List registered tools.", kind: commandTools, }, + { + name: "/skills", + usage: "/skills", + group: commandGroupTools, + description: "List installed skills; run one directly with / [args].", + kind: commandSkills, + }, { name: "/context", usage: "/context", diff --git a/internal/tui/model.go b/internal/tui/model.go index c27406e1..16abf63e 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -29,6 +29,7 @@ import ( "github.com/Gitlawb/zero/internal/providers/providerio" "github.com/Gitlawb/zero/internal/sandbox" "github.com/Gitlawb/zero/internal/sessions" + "github.com/Gitlawb/zero/internal/skills" "github.com/Gitlawb/zero/internal/streamjson" "github.com/Gitlawb/zero/internal/tools" "github.com/Gitlawb/zero/internal/usage" @@ -62,6 +63,7 @@ type model struct { ctx context.Context cwd string userCommands []usercommands.Command // file-sourced /commands (.zero/commands) + loadSkills func() []skills.Skill // lazy installed-skills loader for /skills + / userConfigPath string doctorUserConfigPath string projectConfigPath string @@ -735,6 +737,7 @@ func newModel(ctx context.Context, options Options) model { cwd: cwd, swarmDoneAt: map[string]time.Time{}, userCommands: loadedUserCommands, + loadSkills: options.LoadSkills, composerCursorVisible: true, userConfigPath: options.UserConfigPath, doctorUserConfigPath: doctorUserConfigPath, @@ -3845,6 +3848,9 @@ func (m model) handleSubmit() (tea.Model, tea.Cmd) { case commandTools: m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: m.toolsText()}) return m, nil + case commandSkills: + m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: m.skillsText()}) + return m, nil case commandMCP: if strings.TrimSpace(command.text) == "" { return m.openMCPManager(), nil @@ -4040,6 +4046,11 @@ func (m model) handleSubmit() (tea.Model, tea.Cmd) { if next, cmd, handled := m.handleUserCommand(command.text); handled { return next, cmd } + // Then an installed skill: "/ [args]" runs the skill directly + // (deterministic invocation, vs waiting for the model to pull it in). + if next, cmd, handled := m.handleSkillCommand(command.text); handled { + return next, cmd + } m.transcript = reduceTranscript(m.transcript, transcriptAction{ kind: actionAppendError, text: "unknown command: " + command.text, diff --git a/internal/tui/options.go b/internal/tui/options.go index c43f3910..e1a98d5e 100644 --- a/internal/tui/options.go +++ b/internal/tui/options.go @@ -13,6 +13,7 @@ import ( "github.com/Gitlawb/zero/internal/providermodeldiscovery" "github.com/Gitlawb/zero/internal/sandbox" "github.com/Gitlawb/zero/internal/sessions" + "github.com/Gitlawb/zero/internal/skills" "github.com/Gitlawb/zero/internal/tools" "github.com/Gitlawb/zero/internal/usage" "github.com/Gitlawb/zero/internal/zeroruntime" @@ -48,7 +49,13 @@ type Options struct { SessionCompactor SessionCompactor PrService *PrService - AgentOptions agent.Options + AgentOptions agent.Options + // LoadSkills returns the installed skills (default skills dir merged with any + // plugin skill roots), bodies included, for /skills and direct / + // invocation. Called lazily per use so newly installed skills are picked up + // without a restart. Nil means the session has no skills wiring (skills stay + // model-pulled via the skill tool only). + LoadSkills func() []skills.Skill PermissionMode agent.PermissionMode ReasoningEffort modelregistry.ReasoningEffort ResponseStyle string diff --git a/internal/tui/skill_commands.go b/internal/tui/skill_commands.go new file mode 100644 index 00000000..ab4478d5 --- /dev/null +++ b/internal/tui/skill_commands.go @@ -0,0 +1,121 @@ +package tui + +import ( + "strings" + + tea "charm.land/bubbletea/v2" + + "github.com/Gitlawb/zero/internal/skills" +) + +// Skill slash invocation: "/ [args]" runs an installed skill +// directly, mirroring user commands (.zero/commands). Skills were previously +// model-pulled only (the skill tool), which made invocation a matter of model +// discretion; typing the skill's name makes it deterministic. Precedence is +// builtin > user command > skill: parseCommand resolves builtins first, and the +// commandUnknown fallback tries user commands before skills. + +// handleSkillCommand resolves a "/name args" that wasn't a builtin or a user +// command against the installed skills. On a match it launches a normal agent +// turn whose prompt inlines the skill body (its instructions) followed by the +// typed args, returning handled=true. handled=false means no skill matched, so +// the caller falls through to "unknown command". +func (m model) handleSkillCommand(raw string) (model, tea.Cmd, bool) { + name, args := splitUserCommand(raw) + if name == "" { + return m, nil, false + } + skill, ok := m.lookupSkillCommand(name) + if !ok { + return m, nil, false + } + body := strings.TrimSpace(skill.Content) + if body == "" { + // The name matched a real skill, so falling through to "unknown command" + // would mislead; surface the actual problem instead. + m.transcript = reduceTranscript(m.transcript, transcriptAction{ + kind: actionAppendError, + text: "skill /" + name + " has an empty SKILL.md body (" + skill.Path + ")", + }) + return m, nil, true + } + prompt := body + if args != "" { + // Mirror usercommands.Expand's no-placeholder behavior: the skill body is + // the guidance, the typed args are the specific request it applies to. + prompt += "\n\n" + args + } + return m.launchOrDeferExpandedPrompt(prompt) +} + +// launchOrDeferExpandedPrompt applies the same run-state guards the plain +// commandPrompt path has to an expanded (user command / skill) prompt: while +// exiting nothing may start, a prompt submitted mid-run is queued for the next +// turn, and compaction-in-flight warns instead of racing the compactor. The +// EXPANDED prompt is what gets queued — the queue flush path resubmits text as +// a literal prompt, so queuing the raw "/name args" would send it to the model +// as prose instead of re-dispatching it. +func (m model) launchOrDeferExpandedPrompt(prompt string) (model, tea.Cmd, bool) { + if m.exiting { + return m, nil, true + } + if m.pending { + return m.queueMessage(prompt), nil, true + } + if m.compactInFlight { + m.transcript = reduceTranscript(m.transcript, transcriptAction{ + kind: actionAppendSystem, + text: "Compact\nstatus: warning\nCompaction is running. Re-run the command when it finishes.", + }) + return m, nil, true + } + next, teaCmd := m.launchPrompt(prompt) + return next, teaCmd, true +} + +// lookupSkillCommand returns the installed skill whose slash name matches the +// given (lowercased) name. Linear scan over a fresh load — skills are re-read +// per invocation so a skill installed mid-session is invocable without a +// restart, and the body is never held on the model. +func (m model) lookupSkillCommand(name string) (skills.Skill, bool) { + name = strings.ToLower(strings.TrimSpace(name)) + if name == "" { + return skills.Skill{}, false + } + for _, skill := range m.installedSkills() { + if skillSlashName(skill.Name) == name { + return skill, true + } + } + return skills.Skill{}, false +} + +// installedSkills returns the session's installed skills (default dir merged +// with plugin skill roots) via the injected loader, or nil when the session has +// no skills wiring (e.g. bare test models). +func (m model) installedSkills() []skills.Skill { + if m.loadSkills == nil { + return nil + } + return m.loadSkills() +} + +// skillSlashName maps a skill's frontmatter name to its slash-command form: +// lowercased, and only if it fits the slash-token shape (letters, digits, +// dot/underscore/hyphen — a superset of user-command names, since skill names +// are free-form frontmatter). Returns "" for names that cannot be typed as a +// /command (e.g. containing spaces); those skills remain loadable by the model +// via the skill tool and are still listed by /skills. +func skillSlashName(name string) string { + name = strings.ToLower(strings.TrimSpace(name)) + if name == "" { + return "" + } + for _, r := range name { + valid := (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-' || r == '_' || r == '.' + if !valid { + return "" + } + } + return name +} diff --git a/internal/tui/skill_commands_test.go b/internal/tui/skill_commands_test.go new file mode 100644 index 00000000..0c170abe --- /dev/null +++ b/internal/tui/skill_commands_test.go @@ -0,0 +1,360 @@ +package tui + +import ( + "context" + "strings" + "testing" + + tea "charm.land/bubbletea/v2" + + "github.com/Gitlawb/zero/internal/agent" + "github.com/Gitlawb/zero/internal/skills" +) + +// newSkillTestModel builds a model with an injected skills loader (and the +// matching palette metadata), no filesystem involved. +func newSkillTestModel(t *testing.T, installed ...skills.Skill) model { + t.Helper() + infos := make([]agent.SkillInfo, 0, len(installed)) + for _, s := range installed { + infos = append(infos, agent.SkillInfo{Name: s.Name, Description: s.Description}) + } + return newModel(context.Background(), Options{ + Cwd: t.TempDir(), + LoadSkills: func() []skills.Skill { return installed }, + AgentOptions: agent.Options{Skills: infos}, + }) +} + +func submitInput(t *testing.T, m model, input string) model { + t.Helper() + m.input.SetValue(input) + updated, _ := m.Update(testKey(tea.KeyEnter)) + return updated.(model) +} + +func TestSkillCommandInvokesAndSubmits(t *testing.T) { + m := newSkillTestModel(t, skills.Skill{ + Name: "deploy-checks", + Description: "Run the pre-deploy verification suite.", + Content: "Run every verification step and summarize failures.", + Path: "/skills/deploy-checks/SKILL.md", + }) + + next := submitInput(t, m, "/deploy-checks ship v2 to production") + + if transcriptContains(next.transcript, "unknown command") { + t.Fatalf("an installed skill must not be 'unknown', got %#v", next.transcript) + } + if !transcriptContains(next.transcript, "Run every verification step and summarize failures.") { + t.Fatalf("skill body should be inlined into the prompt, got %#v", next.transcript) + } + if !transcriptContains(next.transcript, "ship v2 to production") { + t.Fatalf("typed args should follow the skill body, got %#v", next.transcript) + } +} + +func TestSkillCommandWithoutArgs(t *testing.T) { + m := newSkillTestModel(t, skills.Skill{Name: "reviewer", Content: "Review the diff for correctness."}) + + next := submitInput(t, m, "/reviewer") + + if !transcriptContains(next.transcript, "Review the diff for correctness.") { + t.Fatalf("bare skill invocation should submit the body, got %#v", next.transcript) + } +} + +// A skill's frontmatter name is free-form; invocation matches its lowercased +// slash form. +func TestSkillCommandNameIsCaseInsensitive(t *testing.T) { + m := newSkillTestModel(t, skills.Skill{Name: "Deploy-Checks", Content: "Check things."}) + + next := submitInput(t, m, "/deploy-checks") + + if !transcriptContains(next.transcript, "Check things.") { + t.Fatalf("uppercase frontmatter name should be invocable lowercased, got %#v", next.transcript) + } +} + +// Precedence: a user command (.zero/commands) shadows a same-named skill. +func TestUserCommandShadowsSkill(t *testing.T) { + root := t.TempDir() + writeUserCommand(t, root, "greet.md", "Say hello from the user command.") + m := newModel(context.Background(), Options{ + Cwd: root, + LoadSkills: func() []skills.Skill { return []skills.Skill{{Name: "greet", Content: "Skill body must not run."}} }, + }) + + next := submitInput(t, m, "/greet") + + if !transcriptContains(next.transcript, "Say hello from the user command.") { + t.Fatalf("user command should win the name collision, got %#v", next.transcript) + } + if transcriptContains(next.transcript, "Skill body must not run.") { + t.Fatalf("shadowed skill must not run, got %#v", next.transcript) + } +} + +// An empty skill body is a real, named match — surface the problem instead of +// falling through to a misleading "unknown command". +func TestSkillCommandEmptyBody(t *testing.T) { + m := newSkillTestModel(t, skills.Skill{Name: "hollow", Path: "/skills/hollow/SKILL.md"}) + + next := submitInput(t, m, "/hollow") + + if transcriptContains(next.transcript, "unknown command") { + t.Fatalf("an empty skill must not be 'unknown', got %#v", next.transcript) + } + if !transcriptContains(next.transcript, "empty SKILL.md body") { + t.Fatalf("empty-body error should be surfaced, got %#v", next.transcript) + } +} + +func TestUnknownSlashStillUnknownWithSkillsInstalled(t *testing.T) { + m := newSkillTestModel(t, skills.Skill{Name: "reviewer", Content: "Review."}) + + next := submitInput(t, m, "/definitelynotaskill") + + if !transcriptContains(next.transcript, "unknown command") { + t.Fatalf("non-matching name should still be unknown, got %#v", next.transcript) + } +} + +func TestSkillSlashName(t *testing.T) { + cases := map[string]string{ + "deploy-checks": "deploy-checks", + "Deploy-Checks": "deploy-checks", + " pdf_tools ": "pdf_tools", + "v2.migrate": "v2.migrate", + "has space": "", + "emoji✨": "", + "": "", + "slash/inside": "", + "tab\tseparated": "", + } + for in, want := range cases { + if got := skillSlashName(in); got != want { + t.Errorf("skillSlashName(%q) = %q, want %q", in, got, want) + } + } +} + +func TestSkillAppearsInAutocomplete(t *testing.T) { + m := newSkillTestModel(t, skills.Skill{Name: "deploy-checks", Description: "Run the pre-deploy suite.", Content: "x"}) + + got := m.matchCommandSuggestions("/dep") + found := false + for _, s := range got { + if s.Name == "/deploy-checks" { + found = true + if s.Desc != "Run the pre-deploy suite. (skill)" { + t.Fatalf("skill suggestion should be labeled, got %q", s.Desc) + } + } + } + if !found { + t.Fatalf("skill /deploy-checks should appear for '/dep', got %#v", got) + } +} + +// A skill shadowed by a builtin (or its alias) or a user command would be dead +// at dispatch time, so the palette must not advertise it. +func TestShadowedSkillNotSuggested(t *testing.T) { + root := t.TempDir() + writeUserCommand(t, root, "deploy.md", "User deploy.") + installed := []skills.Skill{ + {Name: "help", Content: "x"}, // builtin collision + {Name: "sessions", Content: "x"}, // builtin ALIAS collision (/resume alias) + {Name: "deploy", Content: "x"}, // user-command collision + } + infos := make([]agent.SkillInfo, 0, len(installed)) + for _, s := range installed { + infos = append(infos, agent.SkillInfo{Name: s.Name}) + } + m := newModel(context.Background(), Options{ + Cwd: root, + LoadSkills: func() []skills.Skill { return installed }, + AgentOptions: agent.Options{Skills: infos}, + }) + + 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" { + t.Fatalf("shadowed skill advertised for %q: %#v", token, s) + } + } + } +} + +// Invalid slash shapes (spaces etc.) and a bare "/" must not surface skills. +func TestSkillSuggestionEdgeCases(t *testing.T) { + m := newSkillTestModel(t, skills.Skill{Name: "has space", Description: "not invocable", Content: "x"}) + + if got := m.matchSkillSuggestions("/has"); len(got) != 0 { + t.Fatalf("invalid slash-shape skill must not be suggested, got %#v", got) + } + if got := m.matchSkillSuggestions("/"); got != nil { + t.Fatalf("bare '/' should not list skills, got %#v", got) + } +} + +func TestSkillsCommandResolves(t *testing.T) { + command, ok := resolveCommand("/skills") + if !ok || command.kind != commandSkills { + t.Fatalf("resolveCommand(/skills) = %#v, %v", command, ok) + } +} + +func TestSkillsCommandListsInstalled(t *testing.T) { + m := newSkillTestModel(t, + skills.Skill{Name: "deploy-checks", Description: "Run the pre-deploy suite.", Content: "x"}, + skills.Skill{Name: "has space", Description: "tool-only skill", Content: "x"}, + ) + + next := submitInput(t, m, "/skills") + + if !transcriptContains(next.transcript, "/deploy-checks - Run the pre-deploy suite.") { + t.Fatalf("installed skill should be listed with its slash form, got %#v", next.transcript) + } + // A non-slash-invocable name is still listed, without the leading slash. + if !transcriptContains(next.transcript, "has space - tool-only skill") { + t.Fatalf("non-invocable skill should still be listed, got %#v", next.transcript) + } + if !transcriptContains(next.transcript, "/") { + t.Fatalf("usage hint missing, got %#v", next.transcript) + } +} + +func TestSkillsCommandEmptyState(t *testing.T) { + m := newModel(context.Background(), Options{Cwd: t.TempDir()}) + + next := submitInput(t, m, "/skills") + + if !transcriptContains(next.transcript, "No skills installed.") { + t.Fatalf("empty state should say no skills, got %#v", next.transcript) + } +} + +// Run-state guards: an expanded skill prompt must not race an active run — it +// queues (as the EXPANDED prompt, since the queue flush submits literal text). +func TestSkillInvocationQueuedWhileRunPending(t *testing.T) { + m := newSkillTestModel(t, skills.Skill{Name: "reviewer", Content: "Review the diff."}) + m.pending = true + + next, _, handled := m.handleSkillCommand("/reviewer focus on tests") + if !handled { + t.Fatal("skill must be handled even while a run is pending") + } + if next.queuedMessage == "" || !strings.Contains(next.queuedMessage, "Review the diff.") { + t.Fatalf("expanded prompt should be queued, got %q", next.queuedMessage) + } + if !strings.Contains(next.queuedMessage, "focus on tests") { + t.Fatalf("queued prompt should carry the args, got %q", next.queuedMessage) + } +} + +func TestSkillInvocationDroppedWhileExiting(t *testing.T) { + m := newSkillTestModel(t, skills.Skill{Name: "reviewer", Content: "Review."}) + m.exiting = true + + next, cmd, handled := m.handleSkillCommand("/reviewer") + if !handled || cmd != nil || next.queuedMessage != "" { + t.Fatalf("exiting must swallow the invocation: handled=%v cmd=%v queued=%q", handled, cmd, next.queuedMessage) + } +} + +func TestSkillInvocationWarnsDuringCompaction(t *testing.T) { + m := newSkillTestModel(t, skills.Skill{Name: "reviewer", Content: "Review."}) + m.compactInFlight = true + + next, _, handled := m.handleSkillCommand("/reviewer") + if !handled { + t.Fatal("skill must be handled during compaction") + } + if !transcriptContains(next.transcript, "Compaction is running") { + t.Fatalf("compaction warning expected, got %#v", next.transcript) + } +} + +// The same guard now protects user commands (previously they could start a +// second concurrent run). +func TestUserCommandQueuedWhileRunPending(t *testing.T) { + root := t.TempDir() + writeUserCommand(t, root, "greet.md", "Say hello to $1.") + m := newModel(context.Background(), Options{Cwd: root}) + m.pending = true + + next, _, handled := m.handleUserCommand("/greet world") + if !handled { + t.Fatal("user command must be handled while pending") + } + if !strings.Contains(next.queuedMessage, "Say hello to world.") { + t.Fatalf("expanded user-command prompt should be queued, got %q", next.queuedMessage) + } +} + +// Two skills whose names collide after lowercasing map to one slash name; only +// the one dispatch will run may be advertised. +func TestCaseCollidingSkillsAdvertisedOnce(t *testing.T) { + m := newSkillTestModel(t, + skills.Skill{Name: "Deploy", Description: "wins", Content: "a"}, + skills.Skill{Name: "deploy", Description: "loses", Content: "b"}, + ) + + rows := 0 + for _, s := range m.matchSkillSuggestions("/dep") { + if s.Name == "/deploy" { + rows++ + if !strings.Contains(s.Desc, "wins") { + t.Fatalf("advertised row must describe the skill dispatch runs, got %q", s.Desc) + } + } + } + if rows != 1 { + t.Fatalf("case-colliding skills must yield exactly one /deploy row, got %d", rows) + } +} + +// /skills must not print a "/name" invocation label for a skill that a builtin +// or user command shadows — typing it would never run the skill. +func TestSkillsCommandShadowedSkillListedBare(t *testing.T) { + m := newSkillTestModel(t, skills.Skill{Name: "help", Description: "shadowed by the builtin", Content: "x"}) + + next := submitInput(t, m, "/skills") + + if transcriptContains(next.transcript, "/help - shadowed") { + t.Fatalf("shadowed skill must not carry a slash label, got %#v", next.transcript) + } + if !transcriptContains(next.transcript, "help - shadowed by the builtin (via skill tool)") { + t.Fatalf("shadowed skill should be listed bare with the tool note, got %#v", next.transcript) + } +} + +func TestSkillsCommandLongDescriptionTruncated(t *testing.T) { + long := strings.Repeat("very long trigger description ", 12) // ~360 chars + m := newSkillTestModel(t, skills.Skill{Name: "wordy", Description: long, Content: "x"}) + + next := submitInput(t, m, "/skills") + + if !transcriptContains(next.transcript, "…") { + t.Fatalf("long description should be truncated with an ellipsis, got %#v", next.transcript) + } +} + +// The palette reads through the loader, so a skill installed mid-session (the +// loader's next result) appears without a restart. +func TestSkillPaletteReadsThroughLoader(t *testing.T) { + installed := []skills.Skill{} + m := newModel(context.Background(), Options{ + Cwd: t.TempDir(), + LoadSkills: func() []skills.Skill { return installed }, + }) + if got := m.matchSkillSuggestions("/lat"); len(got) != 0 { + t.Fatalf("no skills installed yet, got %#v", got) + } + installed = append(installed, skills.Skill{Name: "late-arrival", Description: "installed mid-session", Content: "x"}) + got := m.matchSkillSuggestions("/lat") + if len(got) != 1 || got[0].Name != "/late-arrival" { + t.Fatalf("mid-session skill should appear via the loader, got %#v", got) + } +} diff --git a/internal/tui/user_commands.go b/internal/tui/user_commands.go index 5ebe64e5..864c9718 100644 --- a/internal/tui/user_commands.go +++ b/internal/tui/user_commands.go @@ -30,8 +30,9 @@ func (m model) handleUserCommand(raw string) (model, tea.Cmd, bool) { if strings.TrimSpace(prompt) == "" { return m, nil, false } - next, teaCmd := m.launchPrompt(prompt) - return next, teaCmd, true + // Same run-state guards as a plain prompt: a user command invoked mid-run is + // queued (as its expanded prompt), not raced into a second concurrent turn. + return m.launchOrDeferExpandedPrompt(prompt) } // lookupUserCommand returns the loaded user command with the given (lowercased) From f6e298ad20d2fb161f3752de884c3afa6a8ce8fb Mon Sep 17 00:00:00 2001 From: Gnanam Date: Sun, 5 Jul 2026 18:52:14 +0530 Subject: [PATCH 2/5] feat(tui): /skills opens a searchable skill picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/tui/autocomplete.go | 11 +--- internal/tui/command_views.go | 79 ++---------------------- internal/tui/model.go | 11 ++++ internal/tui/picker.go | 30 +++++++++ internal/tui/skill_commands.go | 62 +++++++++++++++++++ internal/tui/skill_commands_test.go | 95 ++++++++++++++++++----------- 6 files changed, 171 insertions(+), 117 deletions(-) diff --git a/internal/tui/autocomplete.go b/internal/tui/autocomplete.go index 074c78db..f190603d 100644 --- a/internal/tui/autocomplete.go +++ b/internal/tui/autocomplete.go @@ -203,16 +203,7 @@ func (m model) matchSkillSuggestions(token string) []commandSuggestion { if prefix == "" || m.loadSkills == nil { return nil } - taken := map[string]bool{} - for _, command := range commandDefinitions { - taken[strings.TrimPrefix(command.name, "/")] = true - for _, alias := range command.aliases { - taken[strings.TrimPrefix(alias, "/")] = true - } - } - for _, cmd := range m.userCommands { - taken[cmd.Name] = true - } + taken := m.takenSlashNames() var out []commandSuggestion // Read through the loader (not the startup snapshot in agentOptions) so the // palette matches what dispatch will actually resolve — a skill installed or diff --git a/internal/tui/command_views.go b/internal/tui/command_views.go index 8e52dbcd..f92aa671 100644 --- a/internal/tui/command_views.go +++ b/internal/tui/command_views.go @@ -699,85 +699,18 @@ func (m model) debugText() string { }) } -// truncateSkillListDescription keeps a /skills row readable: the command card -// hard-truncates each rendered line at card width, so a very long trigger-rich -// description is cut deliberately (with an ellipsis) rather than arbitrarily. -func truncateSkillListDescription(desc string) string { - desc = strings.TrimSpace(desc) - const maxRunes = 140 - runes := []rune(desc) - if len(runes) <= maxRunes { - return desc - } - return strings.TrimSpace(string(runes[:maxRunes])) + "…" -} - -// skillsText renders the /skills listing: every installed skill (default dir -// merged with plugin skill roots), its slash-invocation form when the name fits -// a slash token, and how to run one. Skills are re-read on each invocation so a -// skill installed mid-session shows up without a restart. +// skillsText is the /skills fallback when NO skills are installed — an install +// hint. With skills present /skills opens the searchable skill picker instead +// (see newSkillPicker), matching how /model works. func (m model) skillsText() string { - installed := m.installedSkills() - if len(installed) == 0 { - return renderCommandOutput(commandOutput{ - Title: "Skills", - Status: commandStatusInfo, - Sections: []commandSection{{ - Lines: []string{"No skills installed."}, - }}, - Hints: []string{ - "install one: create //SKILL.md (see `zero skills`)", - }, - }) - } - // A "/name" label is shown ONLY when typing it would actually run the skill: - // names shadowed by a builtin (or alias), a user command, or an earlier skill - // with the same lowercased name — and names that don't fit a slash token — - // are listed bare with a "(via skill tool)" note instead, so the listing - // never advertises a dead invocation. - taken := map[string]bool{} - for _, command := range commandDefinitions { - taken[strings.TrimPrefix(command.name, "/")] = true - for _, alias := range command.aliases { - taken[strings.TrimPrefix(alias, "/")] = true - } - } - for _, cmd := range m.userCommands { - taken[cmd.Name] = true - } - lines := make([]string, 0, len(installed)) - for _, skill := range installed { - name := strings.TrimSpace(skill.Name) - if name == "" { - continue - } - slash := skillSlashName(name) - invocable := slash != "" && !taken[slash] - if slash != "" { - taken[slash] = true - } - label := name + " -" - if invocable { - label = "/" + slash + " -" - } - if desc := truncateSkillListDescription(skill.Description); desc != "" { - label += " " + desc - } - if !invocable { - label += " (via skill tool)" - } - lines = append(lines, strings.TrimSuffix(label, " -")) - } return renderCommandOutput(commandOutput{ Title: "Skills", - Status: commandStatusOK, + Status: commandStatusInfo, Sections: []commandSection{{ - Title: "Installed", - Lines: lines, + Lines: []string{"No skills installed."}, }}, Hints: []string{ - "run one directly: / [request]", - "the model can also load skills on demand via the skill tool", + "install one: create //SKILL.md (see `zero skills`)", }, }) } diff --git a/internal/tui/model.go b/internal/tui/model.go index 16abf63e..3406d282 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -3733,6 +3733,11 @@ func (m model) choosePicker() (tea.Model, tea.Cmd) { if text != "" { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: text}) } + case pickerSkill: + // Fill the composer with "/name " so the user adds their request before + // submitting (a bare second Enter runs it without one); names the slash + // path cannot reach run immediately instead. + m, cmd = m.chooseSkillFromPicker(item) case pickerTheme: // The hovered palette is already live from the preview; handleThemeCommand // records the choice (m.themeMode) and re-applies it, and reports the switch. @@ -3849,6 +3854,12 @@ func (m model) handleSubmit() (tea.Model, tea.Cmd) { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: m.toolsText()}) return m, nil case commandSkills: + // With skills installed, /skills opens a searchable picker (like /model); + // the text card remains only as the no-skills install hint. + if picker := m.newSkillPicker(); picker != nil { + m.picker = picker + return m, nil + } m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: m.skillsText()}) return m, nil case commandMCP: diff --git a/internal/tui/picker.go b/internal/tui/picker.go index cda22a65..39384b2a 100644 --- a/internal/tui/picker.go +++ b/internal/tui/picker.go @@ -24,6 +24,7 @@ const ( pickerEffort pickerSession pickerTheme + pickerSkill ) // pickerItem is one selectable row: Label is shown, Value is passed to the @@ -225,6 +226,35 @@ func (m model) newModelPicker() *commandPicker { return &commandPicker{kind: pickerModel, title: "Choose a model", items: items, allItems: append([]pickerItem{}, items...), selected: 0} } +// newSkillPicker lists the installed skills for browse-and-run: type to filter, +// arrow keys to select, Enter runs the chosen skill immediately. Selection is by +// the skill's EXACT name (item.Value), so even skills the slash path cannot +// reach — names shadowed by a builtin or user command, or names that don't fit +// a slash token — are runnable here: picking from the Skills picker is +// unambiguous. Returns nil when no skills are installed (the caller falls back +// to the install-hint card). +func (m model) newSkillPicker() *commandPicker { + installed := m.installedSkills() + items := make([]pickerItem, 0, len(installed)) + for _, skill := range installed { + name := strings.TrimSpace(skill.Name) + if name == "" { + continue + } + label := name + if slash := skillSlashName(name); slash != "" { + // Show the slash form where one exists — it doubles as documentation + // of the direct "/name [args]" invocation. + label = "/" + slash + } + items = append(items, pickerItem{Label: label, Value: name, Meta: strings.TrimSpace(skill.Description)}) + } + if len(items) == 0 { + return nil + } + return &commandPicker{kind: pickerSkill, title: "pick a skill · ⏎ fills /name — add your request", items: items, allItems: append([]pickerItem{}, items...), selected: 0} +} + // modelPickerProviders returns the providers to list in /model: all saved // providers, falling back to the active profile when none were threaded in. func (m model) modelPickerProviders() []config.ProviderProfile { diff --git a/internal/tui/skill_commands.go b/internal/tui/skill_commands.go index ab4478d5..4fb9ad43 100644 --- a/internal/tui/skill_commands.go +++ b/internal/tui/skill_commands.go @@ -73,6 +73,68 @@ func (m model) launchOrDeferExpandedPrompt(prompt string) (model, tea.Cmd, bool) return next, teaCmd, true } +// takenSlashNames returns every slash token already claimed by a builtin +// command (or alias) or a user command — the names a typed "/x" would dispatch +// to before ever reaching a skill. +func (m model) takenSlashNames() map[string]bool { + taken := map[string]bool{} + for _, command := range commandDefinitions { + taken[strings.TrimPrefix(command.name, "/")] = true + for _, alias := range command.aliases { + taken[strings.TrimPrefix(alias, "/")] = true + } + } + for _, cmd := range m.userCommands { + taken[cmd.Name] = true + } + return taken +} + +// chooseSkillFromPicker handles Enter on a skill-picker row. Most skills want a +// request ("review WHICH pr?"), so selection fills the composer with +// "/name " — cursor at the end — for the user to complete and submit (Enter +// again runs it bare). Skills whose names cannot be typed as a slash command +// (non-slash shapes, or names shadowed by a builtin or user command) fall back +// to running immediately: the picker is the only path that reaches them, and an +// unfillable composer would be a dead end. +func (m model) chooseSkillFromPicker(item pickerItem) (model, tea.Cmd) { + slash := skillSlashName(item.Value) + if slash == "" || m.takenSlashNames()[slash] { + return m.invokeSkillByName(item.Value) + } + m.input.SetValue("/" + slash + " ") + m.input.CursorEnd() + return m, nil +} + +// invokeSkillByName runs the installed skill with the given EXACT raw name (the +// skill-picker path). The same run-state guards as slash invocation apply; the +// picker offers no args affordance, so the prompt is the skill body alone. +func (m model) invokeSkillByName(name string) (model, tea.Cmd) { + for _, skill := range m.installedSkills() { + if strings.TrimSpace(skill.Name) != name { + continue + } + body := strings.TrimSpace(skill.Content) + if body == "" { + m.transcript = reduceTranscript(m.transcript, transcriptAction{ + kind: actionAppendError, + text: "skill " + name + " has an empty SKILL.md body (" + skill.Path + ")", + }) + return m, nil + } + next, teaCmd, _ := m.launchOrDeferExpandedPrompt(body) + return next, teaCmd + } + // The picker row came from a slightly older load (TTL cache) and the skill + // has since been removed. + m.transcript = reduceTranscript(m.transcript, transcriptAction{ + kind: actionAppendError, + text: "skill " + name + " is no longer installed", + }) + return m, nil +} + // lookupSkillCommand returns the installed skill whose slash name matches the // given (lowercased) name. Linear scan over a fresh load — skills are re-read // per invocation so a skill installed mid-session is invocable without a diff --git a/internal/tui/skill_commands_test.go b/internal/tui/skill_commands_test.go index 0c170abe..20c83138 100644 --- a/internal/tui/skill_commands_test.go +++ b/internal/tui/skill_commands_test.go @@ -205,7 +205,8 @@ func TestSkillsCommandResolves(t *testing.T) { } } -func TestSkillsCommandListsInstalled(t *testing.T) { +// With skills installed, /skills opens a searchable picker (like /model). +func TestSkillsCommandOpensPicker(t *testing.T) { m := newSkillTestModel(t, skills.Skill{Name: "deploy-checks", Description: "Run the pre-deploy suite.", Content: "x"}, skills.Skill{Name: "has space", Description: "tool-only skill", Content: "x"}, @@ -213,15 +214,67 @@ func TestSkillsCommandListsInstalled(t *testing.T) { next := submitInput(t, m, "/skills") - if !transcriptContains(next.transcript, "/deploy-checks - Run the pre-deploy suite.") { - t.Fatalf("installed skill should be listed with its slash form, got %#v", next.transcript) + if next.picker == nil || next.picker.kind != pickerSkill { + t.Fatalf("/skills should open the skill picker, got %#v", next.picker) } - // A non-slash-invocable name is still listed, without the leading slash. - if !transcriptContains(next.transcript, "has space - tool-only skill") { - t.Fatalf("non-invocable skill should still be listed, got %#v", next.transcript) + labels := map[string]string{} + for _, item := range next.picker.items { + labels[item.Label] = item.Value } - if !transcriptContains(next.transcript, "/") { - t.Fatalf("usage hint missing, got %#v", next.transcript) + if labels["/deploy-checks"] != "deploy-checks" { + t.Fatalf("slash-invocable skill should show its slash form, got %#v", labels) + } + // A non-slash-shaped name is still selectable (bare label, exact-name value). + if labels["has space"] != "has space" { + t.Fatalf("non-slash skill should be listed bare but selectable, got %#v", labels) + } +} + +// Enter on a picker row fills the composer with "/name " so the user can add +// their request (which PR? which file?) before submitting — it must NOT fire +// the skill with no request attached. +func TestSkillPickerEnterFillsComposer(t *testing.T) { + m := newSkillTestModel(t, skills.Skill{Name: "reviewer", Content: "Review the diff for correctness."}) + + next := submitInput(t, m, "/skills") + if next.picker == nil { + t.Fatal("picker should be open") + } + updated, _ := next.Update(testKey(tea.KeyEnter)) + after := updated.(model) + + if after.picker != nil { + t.Fatal("picker should close on Enter") + } + if got := after.input.Value(); got != "/reviewer " { + t.Fatalf("composer should be filled with the invocation, got %q", got) + } + if transcriptContains(after.transcript, "Review the diff for correctness.") { + t.Fatalf("skill must not run before the user submits, got %#v", after.transcript) + } + + // A second Enter submits the (bare) invocation and runs the skill. + final, _ := after.Update(testKey(tea.KeyEnter)) + done := final.(model) + if !transcriptContains(done.transcript, "Review the diff for correctness.") { + t.Fatalf("submitting the filled composer should run the skill, got %#v", done.transcript) + } +} + +// Picker selection is by exact name, so a skill shadowed by a builtin (dead on +// the slash path) is still runnable from the picker. +func TestSkillPickerRunsBuiltinShadowedSkill(t *testing.T) { + m := newSkillTestModel(t, skills.Skill{Name: "help", Description: "shadowed", Content: "Shadowed skill body."}) + + next := submitInput(t, m, "/skills") + if next.picker == nil { + t.Fatal("picker should be open") + } + updated, _ := next.Update(testKey(tea.KeyEnter)) + after := updated.(model) + + if !transcriptContains(after.transcript, "Shadowed skill body.") { + t.Fatalf("picker must run the shadowed skill directly, got %#v", after.transcript) } } @@ -315,32 +368,6 @@ func TestCaseCollidingSkillsAdvertisedOnce(t *testing.T) { } } -// /skills must not print a "/name" invocation label for a skill that a builtin -// or user command shadows — typing it would never run the skill. -func TestSkillsCommandShadowedSkillListedBare(t *testing.T) { - m := newSkillTestModel(t, skills.Skill{Name: "help", Description: "shadowed by the builtin", Content: "x"}) - - next := submitInput(t, m, "/skills") - - if transcriptContains(next.transcript, "/help - shadowed") { - t.Fatalf("shadowed skill must not carry a slash label, got %#v", next.transcript) - } - if !transcriptContains(next.transcript, "help - shadowed by the builtin (via skill tool)") { - t.Fatalf("shadowed skill should be listed bare with the tool note, got %#v", next.transcript) - } -} - -func TestSkillsCommandLongDescriptionTruncated(t *testing.T) { - long := strings.Repeat("very long trigger description ", 12) // ~360 chars - m := newSkillTestModel(t, skills.Skill{Name: "wordy", Description: long, Content: "x"}) - - next := submitInput(t, m, "/skills") - - if !transcriptContains(next.transcript, "…") { - t.Fatalf("long description should be truncated with an ellipsis, got %#v", next.transcript) - } -} - // The palette reads through the loader, so a skill installed mid-session (the // loader's next result) appears without a restart. func TestSkillPaletteReadsThroughLoader(t *testing.T) { From 3259ea3f6a2ba6b5d39b78bbe2f732ef32a7316d Mon Sep 17 00:00:00 2001 From: Gnanam Date: Sun, 5 Jul 2026 19:21:40 +0530 Subject: [PATCH 3/5] feat(tui): ask-first guard for bare skill invocations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/tui/skill_commands.go | 26 ++++++++++++++++++++------ internal/tui/skill_commands_test.go | 17 +++++++++++++++++ 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/internal/tui/skill_commands.go b/internal/tui/skill_commands.go index 4fb9ad43..887680b3 100644 --- a/internal/tui/skill_commands.go +++ b/internal/tui/skill_commands.go @@ -39,13 +39,27 @@ func (m model) handleSkillCommand(raw string) (model, tea.Cmd, bool) { }) return m, nil, true } - prompt := body + return m.launchOrDeferExpandedPrompt(skillInvocationPrompt(body, args)) +} + +// bareSkillInvocationNote is appended when a skill is invoked with no request. +// The body alone is instructions with no target ("review the PR" — which PR?), +// and without this note the model improvises one instead of asking. The wording +// is conditional so self-contained skills (no target needed) still just run. +const bareSkillInvocationNote = "The user invoked this skill directly without providing a request. " + + "If these instructions need a target or details that are not already clear from the conversation " + + "(which pull request, file, branch, topic, …), ask for them first — do not guess or pick one yourself. " + + "If the instructions are self-contained, proceed." + +// skillInvocationPrompt builds the agent prompt for a skill invocation: the +// skill body (its instructions), then either the user's request or — for a bare +// invocation — the ask-first note above. Mirrors usercommands.Expand's +// no-placeholder behavior for the args case. +func skillInvocationPrompt(body, args string) string { if args != "" { - // Mirror usercommands.Expand's no-placeholder behavior: the skill body is - // the guidance, the typed args are the specific request it applies to. - prompt += "\n\n" + args + return body + "\n\n" + args } - return m.launchOrDeferExpandedPrompt(prompt) + return body + "\n\n" + bareSkillInvocationNote } // launchOrDeferExpandedPrompt applies the same run-state guards the plain @@ -123,7 +137,7 @@ func (m model) invokeSkillByName(name string) (model, tea.Cmd) { }) return m, nil } - next, teaCmd, _ := m.launchOrDeferExpandedPrompt(body) + next, teaCmd, _ := m.launchOrDeferExpandedPrompt(skillInvocationPrompt(body, "")) return next, teaCmd } // The picker row came from a slightly older load (TTL cache) and the skill diff --git a/internal/tui/skill_commands_test.go b/internal/tui/skill_commands_test.go index 20c83138..23284d33 100644 --- a/internal/tui/skill_commands_test.go +++ b/internal/tui/skill_commands_test.go @@ -62,6 +62,23 @@ func TestSkillCommandWithoutArgs(t *testing.T) { if !transcriptContains(next.transcript, "Review the diff for correctness.") { t.Fatalf("bare skill invocation should submit the body, got %#v", next.transcript) } + // A bare invocation carries the ask-first note: instructions with no target + // must make the model ask ("which PR?") instead of improvising one. + if !transcriptContains(next.transcript, "ask for them first") { + t.Fatalf("bare invocation should carry the clarify-first note, got %#v", next.transcript) + } +} + +// An invocation WITH a request must not carry the ask-first note — the user +// already said what to apply the skill to. +func TestSkillCommandWithArgsHasNoClarifyNote(t *testing.T) { + m := newSkillTestModel(t, skills.Skill{Name: "reviewer", Content: "Review the diff."}) + + next := submitInput(t, m, "/reviewer PR 484 reconnect changes") + + if transcriptContains(next.transcript, "ask for them first") { + t.Fatalf("args invocation must not carry the clarify note, got %#v", next.transcript) + } } // A skill's frontmatter name is free-form; invocation matches its lowercased From fce9569362961fae7df93e610b4e8df7641f6011 Mon Sep 17 00:00:00 2001 From: Gnanam Date: Sun, 5 Jul 2026 19:46:03 +0530 Subject: [PATCH 4/5] =?UTF-8?q?fix(tui):=20review=20follow-ups=20=E2=80=94?= =?UTF-8?q?=20restore=20typed=20invocation=20on=20compaction=20guard,=20ca?= =?UTF-8?q?p=20merged=20suggestions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- internal/tui/autocomplete.go | 8 +++++++- internal/tui/skill_commands.go | 16 ++++++++++++---- internal/tui/skill_commands_test.go | 12 ++++++++++-- internal/tui/user_commands.go | 2 +- 4 files changed, 30 insertions(+), 8 deletions(-) diff --git a/internal/tui/autocomplete.go b/internal/tui/autocomplete.go index f190603d..56a443d9 100644 --- a/internal/tui/autocomplete.go +++ b/internal/tui/autocomplete.go @@ -189,7 +189,13 @@ func (m model) matchCommandSuggestions(token string) []commandSuggestion { 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)...) + // Each source caps itself, but the merged list must honor the shared cap too + // (three sources could otherwise stack up to ~3x the palette bound). + if len(out) > maxCommandSuggestions { + out = out[:maxCommandSuggestions] + } + return out } // matchSkillSuggestions returns installed skills whose slash name has the typed diff --git a/internal/tui/skill_commands.go b/internal/tui/skill_commands.go index 887680b3..364e51b6 100644 --- a/internal/tui/skill_commands.go +++ b/internal/tui/skill_commands.go @@ -39,7 +39,7 @@ func (m model) handleSkillCommand(raw string) (model, tea.Cmd, bool) { }) return m, nil, true } - return m.launchOrDeferExpandedPrompt(skillInvocationPrompt(body, args)) + return m.launchOrDeferExpandedPrompt(raw, skillInvocationPrompt(body, args)) } // bareSkillInvocationNote is appended when a skill is invoked with no request. @@ -68,8 +68,12 @@ func skillInvocationPrompt(body, args string) string { // turn, and compaction-in-flight warns instead of racing the compactor. The // EXPANDED prompt is what gets queued — the queue flush path resubmits text as // a literal prompt, so queuing the raw "/name args" would send it to the model -// as prose instead of re-dispatching it. -func (m model) launchOrDeferExpandedPrompt(prompt string) (model, tea.Cmd, bool) { +// as prose instead of re-dispatching it. raw is the typed invocation; on the +// compaction warning it is restored into the composer for an easy re-submit +// (the commandUnknown dispatch cleared the composer before we got here, unlike +// the plain-prompt path whose guard fires before the clear). Empty raw (the +// picker path has no typed form) restores nothing. +func (m model) launchOrDeferExpandedPrompt(raw, prompt string) (model, tea.Cmd, bool) { if m.exiting { return m, nil, true } @@ -77,6 +81,10 @@ func (m model) launchOrDeferExpandedPrompt(prompt string) (model, tea.Cmd, bool) return m.queueMessage(prompt), nil, true } if m.compactInFlight { + if strings.TrimSpace(raw) != "" { + m.input.SetValue(raw) + m.input.CursorEnd() + } m.transcript = reduceTranscript(m.transcript, transcriptAction{ kind: actionAppendSystem, text: "Compact\nstatus: warning\nCompaction is running. Re-run the command when it finishes.", @@ -137,7 +145,7 @@ func (m model) invokeSkillByName(name string) (model, tea.Cmd) { }) return m, nil } - next, teaCmd, _ := m.launchOrDeferExpandedPrompt(skillInvocationPrompt(body, "")) + next, teaCmd, _ := m.launchOrDeferExpandedPrompt("", skillInvocationPrompt(body, "")) return next, teaCmd } // The picker row came from a slightly older load (TTL cache) and the skill diff --git a/internal/tui/skill_commands_test.go b/internal/tui/skill_commands_test.go index 23284d33..17437201 100644 --- a/internal/tui/skill_commands_test.go +++ b/internal/tui/skill_commands_test.go @@ -194,9 +194,12 @@ func TestShadowedSkillNotSuggested(t *testing.T) { AgentOptions: agent.Options{Skills: infos}, }) + // Assert on the advertised NAME with a skill-marked Desc, not on the exact + // fallback Desc format — the invariant is that no skill row carries a + // builtin/user-command name, regardless of description wording. 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" { + for _, s := range m.matchSkillSuggestions(token) { + if s.Name == "/help" || s.Name == "/sessions" || s.Name == "/deploy" { t.Fatalf("shadowed skill advertised for %q: %#v", token, s) } } @@ -344,6 +347,11 @@ func TestSkillInvocationWarnsDuringCompaction(t *testing.T) { if !transcriptContains(next.transcript, "Compaction is running") { t.Fatalf("compaction warning expected, got %#v", next.transcript) } + // The typed invocation is restored for an easy re-submit (the dispatch path + // cleared the composer before the guard could fire). + if got := next.input.Value(); got != "/reviewer" { + t.Fatalf("typed invocation should be restored into the composer, got %q", got) + } } // The same guard now protects user commands (previously they could start a diff --git a/internal/tui/user_commands.go b/internal/tui/user_commands.go index 864c9718..23adcc34 100644 --- a/internal/tui/user_commands.go +++ b/internal/tui/user_commands.go @@ -32,7 +32,7 @@ func (m model) handleUserCommand(raw string) (model, tea.Cmd, bool) { } // Same run-state guards as a plain prompt: a user command invoked mid-run is // queued (as its expanded prompt), not raced into a second concurrent turn. - return m.launchOrDeferExpandedPrompt(prompt) + return m.launchOrDeferExpandedPrompt(raw, prompt) } // lookupUserCommand returns the loaded user command with the given (lowercased) From c427052f9b358c8739020c0946f449b03701f4dc Mon Sep 17 00:00:00 2001 From: Gnanam Date: Sun, 5 Jul 2026 20:49:24 +0530 Subject: [PATCH 5/5] chore: ignore zero-skills local build artifact 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. --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index b3880564..22df498e 100644 --- a/.gitignore +++ b/.gitignore @@ -37,6 +37,7 @@ report.[0-9]_.[0-9]_.[0-9]_.[0-9]_.json /zero /zero.exe +/zero-skills /snake-game.html # Superpowers skill artifacts (brainstorm mockups, specs, plans) — local only