diff --git a/agent/skill_activation_test.go b/agent/skill_activation_test.go index df6a2d17f..2d6f186bc 100644 --- a/agent/skill_activation_test.go +++ b/agent/skill_activation_test.go @@ -24,7 +24,7 @@ func testSkillCatalog(t *testing.T) *SkillCatalog { if err := os.Mkdir(path, 0o755); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(path, "SKILL.md"), []byte("Use concise bullets."), 0o644); err != nil { + if err := os.WriteFile(filepath.Join(path, "SKILL.md"), []byte("---\nname: release-notes\ndescription: Draft release notes.\n---\nUse concise bullets."), 0o644); err != nil { t.Fatal(err) } catalog, err := DiscoverSkills(dir) diff --git a/agent/skills.go b/agent/skills.go index 90f088b04..16200f94e 100644 --- a/agent/skills.go +++ b/agent/skills.go @@ -9,6 +9,8 @@ import ( "regexp" "sort" "strings" + + "gopkg.in/yaml.v3" ) const ( @@ -22,7 +24,7 @@ const ( maxSkillBytes = 1 << 20 ) -var skillName = regexp.MustCompile(`^[a-z0-9][a-z0-9_-]{0,63}$`) +var skillName = regexp.MustCompile(`^[a-z0-9]+(?:-[a-z0-9]+)*$`) // SkillsDir returns the canonical runtime-owned skill directory. func SkillsDir() (string, error) { @@ -160,7 +162,8 @@ func LoadDefaultSkills(projectDir string) (*SkillCatalog, error) { for _, root := range roots { sub, err := DiscoverSkills(root.path) if err != nil { - return nil, fmt.Errorf("discover skills in %s: %w", root.path, err) + catalog.diagnostics = append(catalog.diagnostics, fmt.Errorf("discover skills in %s: %w", root.path, err)) + continue } catalog.diagnostics = append(catalog.diagnostics, sub.diagnostics...) for _, skill := range sub.skills { @@ -285,17 +288,27 @@ func parseSkill(path, directoryName string) (Skill, error) { if instructions == "" { return Skill{}, fmt.Errorf("skill %q: %s is empty", directoryName, skillFilename) } - skill := Skill{Name: directoryName, Path: path} - if strings.HasPrefix(instructions, "---\n") || strings.HasPrefix(instructions, "---\r\n") { - metadata, body, err := skillFrontMatter(instructions) - if err != nil { - return Skill{}, fmt.Errorf("skill %q: %w", directoryName, err) - } - // The directory name is the canonical skill name; a front matter "name" - // field is accepted for compatibility but is not required to match. - skill.Description = metadata["description"] - instructions = body + if !strings.HasPrefix(instructions, "---\n") && !strings.HasPrefix(instructions, "---\r\n") { + return Skill{}, fmt.Errorf("skill %q: missing YAML front matter", directoryName) } + metadata, body, err := skillFrontMatter(instructions) + if err != nil { + return Skill{}, fmt.Errorf("skill %q: %w", directoryName, err) + } + if metadata.Name == "" { + return Skill{}, fmt.Errorf("skill %q: front matter requires name", directoryName) + } + if metadata.Description == "" { + return Skill{}, fmt.Errorf("skill %q: front matter requires description", directoryName) + } + if !skillName.MatchString(metadata.Name) { + return Skill{}, fmt.Errorf("skill %q: invalid front matter name %q", directoryName, metadata.Name) + } + if metadata.Name != directoryName { + return Skill{}, fmt.Errorf("skill %q: front matter name %q must match directory name", directoryName, metadata.Name) + } + skill := Skill{Name: metadata.Name, Description: metadata.Description, Path: path} + instructions = body if strings.TrimSpace(instructions) == "" { return Skill{}, fmt.Errorf("skill %q: instructions are empty", directoryName) } @@ -303,25 +316,28 @@ func parseSkill(path, directoryName string) (Skill, error) { return skill, nil } -func skillFrontMatter(input string) (map[string]string, string, error) { +type skillFrontMatterMetadata struct { + Name string `yaml:"name"` + Description string `yaml:"description"` + Metadata map[string]any `yaml:"metadata"` +} + +func skillFrontMatter(input string) (skillFrontMatterMetadata, string, error) { input = strings.ReplaceAll(input, "\r\n", "\n") lines := strings.Split(input, "\n") if len(lines) < 3 || lines[0] != "---" { - return nil, "", errors.New("invalid front matter") + return skillFrontMatterMetadata{}, "", errors.New("invalid front matter") } - metadata := make(map[string]string) for i := 1; i < len(lines); i++ { if lines[i] == "---" { + var metadata skillFrontMatterMetadata + if err := yaml.Unmarshal([]byte(strings.Join(lines[1:i], "\n")), &metadata); err != nil { + return skillFrontMatterMetadata{}, "", fmt.Errorf("parse YAML front matter: %w", err) + } + metadata.Name = strings.TrimSpace(metadata.Name) + metadata.Description = strings.TrimSpace(metadata.Description) return metadata, strings.Join(lines[i+1:], "\n"), nil } - key, value, ok := strings.Cut(lines[i], ":") - if !ok || strings.TrimSpace(key) == "" || strings.TrimSpace(value) == "" { - return nil, "", fmt.Errorf("invalid front matter line %q", lines[i]) - } - if _, exists := metadata[key]; exists { - return nil, "", fmt.Errorf("duplicate front matter field %q", key) - } - metadata[key] = strings.Trim(strings.TrimSpace(value), `"`) } - return nil, "", errors.New("front matter is not closed") + return skillFrontMatterMetadata{}, "", errors.New("front matter is not closed") } diff --git a/agent/skills_test.go b/agent/skills_test.go index 6f15961d4..4a41a3029 100644 --- a/agent/skills_test.go +++ b/agent/skills_test.go @@ -13,6 +13,9 @@ func writeCatalogSkill(t *testing.T, dir, name, content string) { if err := os.MkdirAll(path, 0o755); err != nil { t.Fatal(err) } + if !strings.HasPrefix(content, "---") { + content = "---\nname: " + name + "\ndescription: Test skill.\n---\n" + content + } if err := os.WriteFile(filepath.Join(path, skillFilename), []byte(content), 0o644); err != nil { t.Fatal(err) } @@ -20,7 +23,7 @@ func writeCatalogSkill(t *testing.T, dir, name, content string) { func TestDiscoverAndLoadSkills(t *testing.T) { dir := t.TempDir() - writeCatalogSkill(t, dir, "release-notes", "---\nname: release-notes\ndescription: Draft concise release notes.\n---\n# Release notes\n\nUse short bullets.") + writeCatalogSkill(t, dir, "release-notes", "---\nname: release-notes\ndescription: Draft concise release notes.\nmetadata:\n author: Ollama\n labels:\n - release\n - docs\n---\n# Release notes\n\nUse short bullets.") catalog, err := DiscoverSkills(dir) if err != nil { t.Fatal(err) @@ -44,20 +47,28 @@ func TestDiscoverAndLoadSkills(t *testing.T) { func TestDiscoverSkillsSkipsMalformedEntries(t *testing.T) { dir := t.TempDir() writeCatalogSkill(t, dir, "valid", "do the useful thing") - // A mismatched front matter "name" no longer rejects a skill: the directory - // name is canonical, so this loads fine. - writeCatalogSkill(t, dir, "mismatched", "---\nname: whatever\ndescription: still loads\n---\nbody") + writeCatalogSkill(t, dir, "mismatched", "---\nname: whatever\ndescription: wrong name\n---\nbody") // Genuinely malformed front matter (a line without a key:value pair) is still rejected. writeCatalogSkill(t, dir, "broken", "---\nname: broken\ndescription\n---\nnope") + writeCatalogSkill(t, dir, "missing-name", "---\ndescription: missing name\n---\nbody") + writeCatalogSkill(t, dir, "missing-description", "---\nname: missing-description\n---\nbody") + writeCatalogSkill(t, dir, "bad-name", "---\nname: bad_name\ndescription: invalid name\n---\nbody") + writeCatalogSkill(t, dir, "under_score", "---\nname: under_score\ndescription: invalid directory\n---\nbody") + if err := os.MkdirAll(filepath.Join(dir, "no-front-matter"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "no-front-matter", skillFilename), []byte("body"), 0o644); err != nil { + t.Fatal(err) + } catalog, err := DiscoverSkills(dir) if err != nil { t.Fatal(err) } - if got, want := len(catalog.List()), 2; got != want { + if got, want := len(catalog.List()), 1; got != want { t.Fatalf("valid skills = %d, want %d", got, want) } - if got := len(catalog.Diagnostics()); got != 1 { - t.Fatalf("diagnostics = %d, want 1", got) + if got, want := len(catalog.Diagnostics()), 7; got != want { + t.Fatalf("diagnostics = %d, want %d: %#v", got, want, catalog.Diagnostics()) } if _, err := catalog.Load("broken"); err == nil || !strings.Contains(err.Error(), "not found") { t.Fatalf("load broken error = %v", err) @@ -70,7 +81,7 @@ func TestDiscoverSkillsSkipsMalformedEntries(t *testing.T) { func TestDiscoverSkillsFollowsSymlinks(t *testing.T) { dir := t.TempDir() target := t.TempDir() - writeCatalogSkill(t, target, "shared", "---\ndescription: From a linked repo.\n---\nshared instructions") + writeCatalogSkill(t, target, "shared", "---\nname: shared\ndescription: From a linked repo.\n---\nshared instructions") if err := os.Symlink(filepath.Join(target, "shared"), filepath.Join(dir, "shared")); err != nil { t.Skipf("symlink not supported: %v", err) } @@ -87,6 +98,38 @@ func TestDiscoverSkillsFollowsSymlinks(t *testing.T) { } } +func TestLoadDefaultSkillsContinuesAfterBadRoot(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + project := t.TempDir() + writeCatalogSkill(t, filepath.Join(project, ".ollama", "skills"), "release-notes", "project instructions") + + badRoot := filepath.Join(t.TempDir(), "not-a-directory") + if err := os.WriteFile(badRoot, []byte("not a directory"), 0o644); err != nil { + t.Fatal(err) + } + t.Setenv(SkillsDirEnv, badRoot) + + catalog, err := LoadDefaultSkills(project) + if err != nil { + t.Fatal(err) + } + if _, err := catalog.Load("release-notes"); err != nil { + t.Fatalf("valid skill was hidden by bad root: %v", err) + } + var foundDiagnostic bool + for _, diagnostic := range catalog.Diagnostics() { + if strings.Contains(diagnostic.Error(), badRoot) { + foundDiagnostic = true + break + } + } + if !foundDiagnostic { + t.Fatalf("diagnostics = %#v, want bad root %q", catalog.Diagnostics(), badRoot) + } +} + func TestSkillsDirUsesOverrideAndXDG(t *testing.T) { base := t.TempDir() @@ -167,7 +210,7 @@ func TestSkillContentListsDirectoryAndResources(t *testing.T) { if err := os.MkdirAll(filepath.Join(skillDir, "references"), 0o755); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("Handle PDFs."), 0o644); err != nil { + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: pdf-processing\ndescription: Handle PDFs.\n---\nHandle PDFs."), 0o644); err != nil { t.Fatal(err) } if err := os.WriteFile(filepath.Join(skillDir, "scripts", "extract.py"), []byte("#!/usr/bin/env python3"), 0o755); err != nil { diff --git a/agent/tools/skill_test.go b/agent/tools/skill_test.go index 84ec0e695..c99515504 100644 --- a/agent/tools/skill_test.go +++ b/agent/tools/skill_test.go @@ -16,7 +16,7 @@ func TestSkillLoadsCoreCatalogWithoutApproval(t *testing.T) { if err := os.Mkdir(path, 0o755); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(path, "SKILL.md"), []byte("Use concise bullets."), 0o644); err != nil { + if err := os.WriteFile(filepath.Join(path, "SKILL.md"), []byte("---\nname: release-notes\ndescription: Draft release notes.\n---\nUse concise bullets."), 0o644); err != nil { t.Fatal(err) } catalog, err := agent.DiscoverSkills(dir) diff --git a/cmd/agent_tui.go b/cmd/agent_tui.go index a31cecc4d..2424dc5d0 100644 --- a/cmd/agent_tui.go +++ b/cmd/agent_tui.go @@ -227,8 +227,6 @@ func GenerateAgentTUI(cmd *cobra.Command, client *api.Client, opts agentTUIOptio for _, diagnostic := range skillCatalog.Diagnostics() { fmt.Fprintf(os.Stderr, "\033[1mwarning:\033[0m ignored invalid agent skill: %v\n", diagnostic) } - skillContext := skillCatalog.SystemContext() - var registry *coreagent.Registry registryForModel := func(ctx context.Context, model string) *coreagent.Registry { return agentToolsRegistry(ctx, client, model, skillCatalog) @@ -236,7 +234,7 @@ func GenerateAgentTUI(cmd *cobra.Command, client *api.Client, opts agentTUIOptio if opts.Model != "" { registry = agentToolsRegistry(cmd.Context(), client, opts.Model, skillCatalog) } - systemPrompt := agentSystemPromptWithWorkingDir(opts.Model, opts.System, skillContext, cwd) + systemPrompt := agentSystemPromptWithWorkingDir(opts.Model, opts.System, agentSkillSystemContext(skillCatalog, registry, opts.ToolsDisabled), cwd) _, err = agentchat.Run(cmd.Context(), agentchat.Options{ Model: opts.Model, @@ -253,8 +251,8 @@ func GenerateAgentTUI(cmd *cobra.Command, client *api.Client, opts agentTUIOptio OnModelSelected: func(_ context.Context, model string) error { return config.SetLastModel(model) }, - SystemPromptForModel: func(ctx context.Context, model string, registry *coreagent.Registry) string { - return agentSystemPromptWithWorkingDir(model, agentSystemFromShow(ctx, client, model), skillContext, cwd) + SystemPromptForModel: func(ctx context.Context, model string, registry *coreagent.Registry, toolsDisabled bool) string { + return agentSystemPromptWithWorkingDir(model, agentSystemFromShow(ctx, client, model), agentSkillSystemContext(skillCatalog, registry, toolsDisabled), cwd) }, Skills: skillCatalog, SystemPrompt: systemPrompt, @@ -294,6 +292,16 @@ func GenerateAgentTUI(cmd *cobra.Command, client *api.Client, opts agentTUIOptio return err } +func agentSkillSystemContext(catalog *coreagent.SkillCatalog, registry *coreagent.Registry, toolsDisabled bool) string { + if toolsDisabled || registry == nil { + return "" + } + if _, ok := registry.Get("skill"); !ok { + return "" + } + return catalog.SystemContext() +} + func selectAgentModel(ctx context.Context, client *api.Client, current string) (string, error) { models, err := agentModelOptions(ctx, client) if err != nil { diff --git a/cmd/agent_tui_test.go b/cmd/agent_tui_test.go index 2a117fd55..5c6d86815 100644 --- a/cmd/agent_tui_test.go +++ b/cmd/agent_tui_test.go @@ -12,6 +12,7 @@ import ( "github.com/spf13/cobra" coreagent "github.com/ollama/ollama/agent" + agenttools "github.com/ollama/ollama/agent/tools" "github.com/ollama/ollama/api" "github.com/ollama/ollama/cmd/config" agentchat "github.com/ollama/ollama/cmd/tui/chat" @@ -70,6 +71,32 @@ func TestAgentSystemPromptIncludesSkillCatalog(t *testing.T) { } } +func TestAgentSkillSystemContextRequiresAvailableEnabledSkillTool(t *testing.T) { + dir := t.TempDir() + if err := os.Mkdir(filepath.Join(dir, "release-notes"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "release-notes", "SKILL.md"), []byte("---\nname: release-notes\ndescription: Draft releases.\n---\nUse bullets."), 0o644); err != nil { + t.Fatal(err) + } + catalog, err := coreagent.DiscoverSkills(dir) + if err != nil { + t.Fatal(err) + } + registry := &coreagent.Registry{} + registry.Register(&agenttools.Skill{Catalog: catalog}) + + if got := agentSkillSystemContext(catalog, registry, false); !strings.Contains(got, "release-notes: Draft releases.") { + t.Fatalf("enabled skill context = %q", got) + } + if got := agentSkillSystemContext(catalog, registry, true); got != "" { + t.Fatalf("disabled tools should omit skill context, got %q", got) + } + if got := agentSkillSystemContext(catalog, &coreagent.Registry{}, false); got != "" { + t.Fatalf("unavailable skill tool should omit skill context, got %q", got) + } +} + func TestAgentSelectionItemsUseLaunchSections(t *testing.T) { items := agentSelectionItems([]agentchat.ModelOption{ {Name: "glm-5.2:cloud", Description: "cloud", Recommended: true, Cloud: true}, diff --git a/cmd/tui/chat/chat.go b/cmd/tui/chat/chat.go index 027434184..948105464 100644 --- a/cmd/tui/chat/chat.go +++ b/cmd/tui/chat/chat.go @@ -60,7 +60,7 @@ type Options struct { MultiModalForModel func(context.Context, string) bool ModelOptions func(context.Context) ([]ModelOption, error) OnModelSelected func(context.Context, string) error - SystemPromptForModel func(context.Context, string, *coreagent.Registry) string + SystemPromptForModel func(context.Context, string, *coreagent.Registry, bool) string ApprovalPrompter coreagent.ApprovalPrompter EventSinks []coreagent.EventSink AllowAllTools bool diff --git a/cmd/tui/chat/input.go b/cmd/tui/chat/input.go index 13ff5ae8b..bdfe3be37 100644 --- a/cmd/tui/chat/input.go +++ b/cmd/tui/chat/input.go @@ -74,11 +74,6 @@ func (m *chatModel) handleSubmit() (tea.Model, tea.Cmd) { return *m, nil } _, _, hasSlashCommand := slashCommandInvocation(input) - if !hasSlashCommand { - if _, _, ok := m.skillSlashInvocation(input); ok { - hasSlashCommand = true - } - } if (m.running || m.compacting) && !hasSlashCommand { m.status = "wait for current response" return *m, nil @@ -227,6 +222,9 @@ func (m *chatModel) handleToolsCommand(args string) (tea.Model, tea.Cmd) { m.opts.ToolsDisabled = true m.status = "tools off" } + if m.opts.SystemPromptForModel != nil { + m.opts.SystemPrompt = m.opts.SystemPromptForModel(m.ctx, m.opts.Model, m.opts.Tools, m.opts.ToolsDisabled) + } return *m, nil } diff --git a/cmd/tui/chat/input_test.go b/cmd/tui/chat/input_test.go index 910708c2b..4443cae32 100644 --- a/cmd/tui/chat/input_test.go +++ b/cmd/tui/chat/input_test.go @@ -569,6 +569,46 @@ func TestSkillSlashCommandPromptBecomesUserMessage(t *testing.T) { } } +func TestChatSkillSubmitWhileActiveRunKeepsActiveState(t *testing.T) { + catalog := writeTestSkillCatalog(t) + for _, state := range []struct { + name string + running bool + compacting bool + }{ + {name: "running", running: true}, + {name: "compacting", compacting: true}, + } { + t.Run(state.name, func(t *testing.T) { + events := make(chan tea.Msg) + cancel := func() {} + m := chatModel{ + opts: Options{Skills: catalog}, + input: []rune("/release-notes draft notes"), + running: state.running, + compacting: state.compacting, + events: events, + cancel: cancel, + } + + updated, cmd := m.handleSubmit() + if cmd != nil { + t.Fatal("skill submit should not start another run while active") + } + got := updated.(chatModel) + if got.events != events || got.cancel == nil || got.running != state.running || got.compacting != state.compacting { + t.Fatalf("active run state changed: %#v", got) + } + if string(got.input) != "/release-notes draft notes" { + t.Fatalf("input = %q, want skill invocation preserved", got.input) + } + if got.status != "wait for current response" { + t.Fatalf("status = %q", got.status) + } + }) + } +} + func writeTestSkillCatalog(t *testing.T) *coreagent.SkillCatalog { t.Helper() dir := t.TempDir() @@ -720,6 +760,35 @@ func TestChatToolsCommandTogglesToolRegistry(t *testing.T) { } } +func TestChatToolsCommandRefreshesCapabilityAwareSystemPrompt(t *testing.T) { + registry := &coreagent.Registry{} + registry.Register(chatTestTool{}) + m := chatModel{ + ctx: context.Background(), + opts: Options{ + Model: "test", + Tools: registry, + SystemPromptForModel: func(_ context.Context, _ string, _ *coreagent.Registry, disabled bool) string { + if disabled { + return "tools disabled" + } + return "tools enabled" + }, + }, + } + + updated, _ := m.handleToolsCommand("") + m = updated.(chatModel) + if m.opts.SystemPrompt != "tools disabled" { + t.Fatalf("system prompt = %q, want disabled prompt", m.opts.SystemPrompt) + } + updated, _ = m.handleToolsCommand("") + m = updated.(chatModel) + if m.opts.SystemPrompt != "tools enabled" { + t.Fatalf("system prompt = %q, want enabled prompt", m.opts.SystemPrompt) + } +} + func TestChatToolsCommandUsage(t *testing.T) { m := chatModel{input: []rune("/tools off")} diff --git a/cmd/tui/chat/modals.go b/cmd/tui/chat/modals.go index 2ca0f923a..7ee55dcb3 100644 --- a/cmd/tui/chat/modals.go +++ b/cmd/tui/chat/modals.go @@ -213,7 +213,7 @@ func (m *chatModel) applyModelSelection(modelName string, persist bool) error { m.opts.Tools = m.opts.ToolRegistryForModel(m.ctx, modelName) } if m.opts.SystemPromptForModel != nil { - m.opts.SystemPrompt = m.opts.SystemPromptForModel(m.ctx, modelName, m.opts.Tools) + m.opts.SystemPrompt = m.opts.SystemPromptForModel(m.ctx, modelName, m.opts.Tools, m.opts.ToolsDisabled) } if m.opts.MultiModalForModel != nil { ctx := m.ctx diff --git a/cmd/tui/chat/modals_test.go b/cmd/tui/chat/modals_test.go index 646207044..f51b6ed32 100644 --- a/cmd/tui/chat/modals_test.go +++ b/cmd/tui/chat/modals_test.go @@ -289,7 +289,7 @@ func TestChatModelPickerFiltersAndSwitchesModel(t *testing.T) { } return 262144 }, - SystemPromptForModel: func(ctx context.Context, model string, registry *coreagent.Registry) string { + SystemPromptForModel: func(ctx context.Context, model string, registry *coreagent.Registry, toolsDisabled bool) string { if model != "qwen3.5:cloud" { t.Fatalf("system prompt model = %q, want qwen3.5:cloud", model) } @@ -397,7 +397,7 @@ func TestChatModelSwitchNextRunKeepsHistory(t *testing.T) { opts: Options{ Model: "llama3.2", Client: client, - SystemPromptForModel: func(_ context.Context, model string, _ *coreagent.Registry) string { + SystemPromptForModel: func(_ context.Context, model string, _ *coreagent.Registry, _ bool) string { return "system for " + model }, },