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 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..56a443d9 100644 --- a/internal/tui/autocomplete.go +++ b/internal/tui/autocomplete.go @@ -188,7 +188,54 @@ 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)...) + 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 +// 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 := 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 + // 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..f92aa671 100644 --- a/internal/tui/command_views.go +++ b/internal/tui/command_views.go @@ -698,3 +698,19 @@ func (m model) debugText() string { }}, }) } + +// 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 { + return renderCommandOutput(commandOutput{ + Title: "Skills", + Status: commandStatusInfo, + Sections: []commandSection{{ + Lines: []string{"No skills installed."}, + }}, + Hints: []string{ + "install one: create //SKILL.md (see `zero skills`)", + }, + }) +} 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..3406d282 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, @@ -3730,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. @@ -3845,6 +3853,15 @@ 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: + // 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: if strings.TrimSpace(command.text) == "" { return m.openMCPManager(), nil @@ -4040,6 +4057,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/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 new file mode 100644 index 00000000..364e51b6 --- /dev/null +++ b/internal/tui/skill_commands.go @@ -0,0 +1,205 @@ +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 + } + return m.launchOrDeferExpandedPrompt(raw, 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 != "" { + return body + "\n\n" + args + } + return body + "\n\n" + bareSkillInvocationNote +} + +// 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. 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 + } + if m.pending { + 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.", + }) + return m, nil, true + } + next, teaCmd := m.launchPrompt(prompt) + 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("", skillInvocationPrompt(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 +// 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..17437201 --- /dev/null +++ b/internal/tui/skill_commands_test.go @@ -0,0 +1,412 @@ +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 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 +// 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}, + }) + + // 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.matchSkillSuggestions(token) { + if s.Name == "/help" || s.Name == "/sessions" || s.Name == "/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) + } +} + +// 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"}, + ) + + next := submitInput(t, m, "/skills") + + if next.picker == nil || next.picker.kind != pickerSkill { + t.Fatalf("/skills should open the skill picker, got %#v", next.picker) + } + labels := map[string]string{} + for _, item := range next.picker.items { + labels[item.Label] = item.Value + } + 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) + } +} + +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 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 +// 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) + } +} + +// 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..23adcc34 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(raw, prompt) } // lookupUserCommand returns the loaded user command with the given (lowercased)