permission skill loading

agent/tools/skill.go

@@ -9,8 +9,9 @@ import (
 )
 
 // Skill is the model-facing adapter for the core agent skill catalog.
-// It only supplies instructions; regular tools retain their own approval
-// requirements for filesystem or network access.
+// Model-initiated loads require approval because a skill's instructions can
+// influence the rest of the run. Explicit user activation is handled by the
+// session's synthetic skill call and bypasses this adapter.
 type Skill struct{ Catalog *agent.SkillCatalog }
 
 func (t *Skill) Name() string { return "skill" }
@@ -25,6 +26,8 @@ func (t *Skill) Schema() api.ToolFunction {
 	return api.ToolFunction{Name: t.Name(), Description: t.Description(), Parameters: api.ToolFunctionParameters{Type: "object", Properties: props, Required: []string{"name"}}}
 }
 
+func (t *Skill) RequiresApproval(map[string]any) bool { return true }
+
 func (t *Skill) Execute(_ context.Context, _ agent.ToolContext, args map[string]any) (agent.ToolResult, error) {
 	name, ok := args["name"].(string)
 	if !ok {

agent/tools/skill_test.go

@@ -8,9 +8,117 @@ import (
 	"testing"
 
 	"github.com/ollama/ollama/agent"
+	"github.com/ollama/ollama/api"
 )
 
-func TestSkillLoadsCoreCatalogWithoutApproval(t *testing.T) {
+func TestSkillLoadsCoreCatalogWithApproval(t *testing.T) {
+	catalog := testSkillCatalog(t)
+	tool := &Skill{Catalog: catalog}
+	if !agent.ToolRequiresApproval(tool, map[string]any{"name": "release-notes"}) {
+		t.Fatal("model-initiated skill loading should require approval")
+	}
+	result, err := tool.Execute(context.Background(), agent.ToolContext{}, map[string]any{"name": "release-notes"})
+	if err != nil || !strings.Contains(result.Content, "Use concise bullets.") {
+		t.Fatalf("tool result = %#v, %v", result, err)
+	}
+}
+
+func TestModelSkillLoadRequiresApproval(t *testing.T) {
+	for _, tt := range []struct {
+		name        string
+		approval    agent.Approval
+		prompt      bool
+		wantCalls   int
+		wantPrompts int
+		wantResult  string
+	}{
+		{name: "rejected", approval: agent.Approval{Reason: "Skill loading denied."}, prompt: true, wantCalls: 1, wantPrompts: 1, wantResult: "Skill loading denied."},
+		{name: "approved", approval: agent.Approval{Allow: true}, prompt: true, wantCalls: 2, wantPrompts: 1, wantResult: "Use concise bullets."},
+		{name: "headless denied", wantCalls: 1, wantResult: "Tool execution requires approval"},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			catalog := testSkillCatalog(t)
+			args := api.NewToolCallFunctionArguments()
+			args.Set("name", "release-notes")
+			client := &skillTestClient{responses: [][]api.ChatResponse{
+				{{Message: api.Message{Role: "assistant", ToolCalls: []api.ToolCall{{
+					ID:       "call_skill_1",
+					Function: api.ToolCallFunction{Name: "skill", Arguments: args},
+				}}}}},
+				{{Message: api.Message{Role: "assistant", Content: "done"}}},
+			}}
+			var prompter *skillApprovalPrompter
+			var approvalPrompter agent.ApprovalPrompter
+			if tt.prompt {
+				prompter = &skillApprovalPrompter{result: tt.approval}
+				approvalPrompter = prompter
+			}
+			registry := &agent.Registry{}
+			registry.Register(&Skill{Catalog: catalog})
+
+			result, err := (&agent.Session{
+				Client:           client,
+				Tools:            registry,
+				ApprovalPrompter: approvalPrompter,
+			}).Run(context.Background(), agent.RunOptions{
+				Model:       "test",
+				NewMessages: []api.Message{{Role: "user", Content: "load the release-notes skill"}},
+			})
+			if err != nil {
+				t.Fatal(err)
+			}
+			if tt.prompt {
+				if got := len(prompter.requests); got != tt.wantPrompts {
+					t.Fatalf("approval prompts = %d, want %d", got, tt.wantPrompts)
+				}
+				request := prompter.requests[0]
+				if len(request.Calls) != 1 || request.Calls[0].ToolName != "skill" || request.Calls[0].ApprovalScope != "skill" || request.Calls[0].Args["name"] != "release-notes" {
+					t.Fatalf("approval request = %#v", request)
+				}
+			}
+			if got := client.calls; got != tt.wantCalls {
+				t.Fatalf("model calls = %d, want %d", got, tt.wantCalls)
+			}
+			var toolResult string
+			for _, message := range result.Messages {
+				if message.Role == "tool" && message.ToolCallID == "call_skill_1" {
+					toolResult = message.Content
+					break
+				}
+			}
+			if !strings.Contains(toolResult, tt.wantResult) {
+				t.Fatalf("skill tool result = %q, want it to contain %q", toolResult, tt.wantResult)
+			}
+		})
+	}
+}
+
+func TestExplicitSkillActivationBypassesApproval(t *testing.T) {
+	catalog := testSkillCatalog(t)
+	client := &skillTestClient{responses: [][]api.ChatResponse{{{Message: api.Message{Role: "assistant", Content: "done"}}}}}
+	prompter := &skillApprovalPrompter{result: agent.Approval{}}
+	result, err := (&agent.Session{
+		Client:           client,
+		Skills:           catalog,
+		ApprovalPrompter: prompter,
+	}).Run(context.Background(), agent.RunOptions{
+		Model:       "test",
+		NewMessages: []api.Message{{Role: "user", Content: "draft release notes"}},
+		SkillName:   "release-notes",
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(prompter.requests) != 0 {
+		t.Fatalf("explicit activation prompted for approval: %#v", prompter.requests)
+	}
+	if len(result.Messages) != 4 || result.Messages[2].ToolName != "skill" || !strings.Contains(result.Messages[2].Content, "Use concise bullets.") {
+		t.Fatalf("synthetic skill activation = %#v", result.Messages)
+	}
+}
+
+func testSkillCatalog(t *testing.T) *agent.SkillCatalog {
+	t.Helper()
 	dir := t.TempDir()
 	path := filepath.Join(dir, "release-notes")
 	if err := os.Mkdir(path, 0o755); err != nil {
@@ -23,12 +131,33 @@ func TestSkillLoadsCoreCatalogWithoutApproval(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	tool := &Skill{Catalog: catalog}
-	if agent.ToolRequiresApproval(tool, map[string]any{"name": "release-notes"}) {
-		t.Fatal("loading a skill must not change ordinary tool approval semantics")
+	return catalog
 }
-	result, err := tool.Execute(context.Background(), agent.ToolContext{}, map[string]any{"name": "release-notes"})
-	if err != nil || !strings.Contains(result.Content, "Use concise bullets.") {
-		t.Fatalf("tool result = %#v, %v", result, err)
+
+type skillTestClient struct {
+	responses [][]api.ChatResponse
+	calls     int
+}
+
+func (c *skillTestClient) Chat(_ context.Context, _ *api.ChatRequest, fn api.ChatResponseFunc) error {
+	if c.calls >= len(c.responses) {
+		return nil
+	}
+	for _, response := range c.responses[c.calls] {
+		if err := fn(response); err != nil {
+			return err
 		}
 	}
+	c.calls++
+	return nil
+}
+
+type skillApprovalPrompter struct {
+	requests []agent.ApprovalRequest
+	result   agent.Approval
+}
+
+func (p *skillApprovalPrompter) PromptApproval(_ context.Context, request agent.ApprovalRequest) (agent.Approval, error) {
+	p.requests = append(p.requests, request)
+	return p.result, nil
+}

cmd/tui/chat/approval_test.go

@@ -134,6 +134,31 @@ func TestChatApprovalUsesShellNameForPermissionPrompt(t *testing.T) {
 	}
 }
 
+func TestChatApprovalRendersSkillLoad(t *testing.T) {
+	request := coreagent.ApprovalRequest{
+		WorkingDir: "/repo",
+		Calls: []coreagent.ApprovalToolCall{{
+			ToolCallID:    "call-skill-1",
+			ToolName:      "skill",
+			Args:          map[string]any{"name": "release-notes"},
+			ApprovalScope: "skill",
+		}},
+	}
+
+	lines := stripANSI(strings.Join((&chatModel{approvalPrompt: &chatApprovalPrompt{request: request}}).renderApprovalPromptLines(80), "\n"))
+	for _, want := range []string{"name: release-notes", "2. Always allow skill"} {
+		if !strings.Contains(lines, want) {
+			t.Fatalf("skill approval prompt missing %q:\n%s", want, lines)
+		}
+	}
+
+	m := chatModel{}
+	m.upsertApprovalToolEntries(request)
+	if len(m.entries) != 1 || !strings.Contains(stripANSI(toolStatusLine(m.entries[0])), `skill("release-notes") needs approval`) {
+		t.Fatalf("skill approval entry = %#v", m.entries)
+	}
+}
+
 func TestChatApprovalPromptOmitsDuplicateBatchDetails(t *testing.T) {
 	request := coreagent.ApprovalRequest{
 		WorkingDir: "/repo",