diff --git a/coderd/userskills_test.go b/coderd/userskills_test.go index 744572496f..e3419b9f00 100644 --- a/coderd/userskills_test.go +++ b/coderd/userskills_test.go @@ -667,8 +667,7 @@ func userSkillMarkdown(name string, description string, body string) string { func requireSDKErrorStatus(t *testing.T, err error, status int, msgAndArgs ...any) *codersdk.Error { t.Helper() require.Error(t, err, msgAndArgs...) - var sdkErr *codersdk.Error - require.ErrorAs(t, err, &sdkErr, msgAndArgs...) + sdkErr := coderdtest.SDKError(t, err) require.Equal(t, status, sdkErr.StatusCode(), msgAndArgs...) return sdkErr } diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index ff89b8002c..92620e43aa 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -34,6 +34,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/pubsub" coderdpubsub "github.com/coder/coder/v2/coderd/pubsub" + "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/coderd/util/xjson" "github.com/coder/coder/v2/coderd/webpush" @@ -51,6 +52,7 @@ import ( "github.com/coder/coder/v2/coderd/x/chatd/chattool" "github.com/coder/coder/v2/coderd/x/chatd/internal/agentselect" "github.com/coder/coder/v2/coderd/x/chatd/mcpclient" + skillspkg "github.com/coder/coder/v2/coderd/x/skills" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/workspacesdk" "github.com/coder/quartz" @@ -6476,6 +6478,31 @@ type systemPromptBehaviorContext struct { isRootChat bool } +func workspaceSkillsForResolution(workspaceSkills []chattool.SkillMeta) []skillspkg.Skill { + if len(workspaceSkills) == 0 { + return nil + } + resolved := make([]skillspkg.Skill, 0, len(workspaceSkills)) + for _, skill := range workspaceSkills { + resolved = append(resolved, skillspkg.Skill{ + Name: skill.Name, + Description: skill.Description, + Source: skillspkg.SourceWorkspace, + }) + } + return resolved +} + +func mergeTurnSkills( + personalSkills []skillspkg.Skill, + workspaceSkills []chattool.SkillMeta, +) []skillspkg.ResolvedSkill { + return skillspkg.MergeSkills( + personalSkills, + workspaceSkillsForResolution(workspaceSkills), + ) +} + // buildSystemPrompt applies system-level prompt injections in the // canonical order. It is used by both the initial prompt assembly // and the ReloadMessages callback to keep them in sync. @@ -6483,7 +6510,7 @@ func buildSystemPrompt( prompt []fantasy.Message, subagentInstruction string, instruction string, - skills []chattool.SkillMeta, + resolvedSkills []skillspkg.ResolvedSkill, userPrompt string, behaviorContext systemPromptBehaviorContext, ) []fantasy.Message { @@ -6493,7 +6520,7 @@ func buildSystemPrompt( if instruction != "" { prompt = chatprompt.InsertSystem(prompt, instruction) } - if skillIndex := chattool.FormatSkillIndex(skills); skillIndex != "" { + if skillIndex := chattool.FormatResolvedSkillIndex(resolvedSkills); skillIndex != "" { prompt = chatprompt.InsertSystem(prompt, skillIndex) } if userPrompt != "" { @@ -6517,6 +6544,34 @@ func buildSystemPrompt( return prompt } +func removeSkillIndexMessages(prompt []fantasy.Message) []fantasy.Message { + out := make([]fantasy.Message, 0, len(prompt)) + removed := false + for _, message := range prompt { + if isSkillIndexMessage(message) { + removed = true + continue + } + out = append(out, message) + } + if !removed { + return prompt + } + return out +} + +func isSkillIndexMessage(message fantasy.Message) bool { + if message.Role != fantasy.MessageRoleSystem || len(message.Content) != 1 { + return false + } + textPart, ok := fantasy.AsMessagePart[fantasy.TextPart](message.Content[0]) + if !ok { + return false + } + text := strings.TrimSpace(textPart.Text) + return strings.HasPrefix(text, chattool.AvailableSkillsOpenTag+"\n") && strings.HasSuffix(text, chattool.AvailableSkillsCloseTag) +} + type rootChatToolsOptions struct { chat database.Chat modelConfigID uuid.UUID @@ -6559,6 +6614,88 @@ func (p *Server) loadPlanModeInstructions( return fetched } +func userSkillContext(ctx context.Context, userID uuid.UUID) context.Context { + actor := rbac.Subject{ + Type: rbac.SubjectTypeUser, + ID: userID.String(), + Roles: rbac.RoleIdentifiers{rbac.RoleMember()}, + Scope: rbac.ScopeAll, + }.WithCachedASTValue() + // Chat turns run asynchronously after admission, so the original request + // actor may no longer be available when a worker loads personal skills. + // We synthesize the chat owner as a member instead of reusing that actor. + // Hardcoding RoleMember is safe because dbauthz enforces + // ResourceUserSkill.WithOwner(userID), so this actor cannot read any other + // user's skills regardless of role. Org scoping is not needed because + // personal skills are user-scoped, not org-scoped. + //nolint:gocritic // The synthetic actor is intentional for the reasons above. + return dbauthz.As(ctx, actor) +} + +func (p *Server) fetchPersonalSkillMetadata( + ctx context.Context, + userID uuid.UUID, + logger slog.Logger, +) []skillspkg.Skill { + rows, err := p.db.ListUserSkillMetadataByUserID(userSkillContext(ctx, userID), userID) + // See package coderd/x/skills (doc.go) for why metadata fetch failures + // intentionally degrade to an empty personal-skill list instead of + // failing the chat turn. + if err != nil { + logger.Warn(ctx, "failed to load personal skill metadata", + slog.F("owner_id", userID), + slog.Error(err), + ) + return nil + } + + personalSkills := make([]skillspkg.Skill, 0, len(rows)) + for _, row := range rows { + personalSkills = append(personalSkills, skillspkg.Skill{ + Name: row.Name, + Description: row.Description, + Source: skillspkg.SourcePersonal, + }) + } + return personalSkills +} + +func (p *Server) loadPersonalSkillBody( + ctx context.Context, + userID uuid.UUID, + name string, +) (skillspkg.ParsedSkill, error) { + row, err := p.db.GetUserSkillByUserIDAndName( + userSkillContext(ctx, userID), + database.GetUserSkillByUserIDAndNameParams{ + UserID: userID, + Name: name, + }, + ) + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + return skillspkg.ParsedSkill{}, skillspkg.ErrSkillNotFound + } + p.logger.Error(ctx, "load personal skill body failed", + slog.F("user_id", userID), + slog.F("name", name), + slog.Error(err), + ) + return skillspkg.ParsedSkill{}, xerrors.Errorf("load personal skill body: %w", err) + } + + parsed, err := skillspkg.ParsePersonalSkillMarkdown([]byte(row.Content)) + if err != nil { + p.logger.Error(ctx, "parse personal skill body failed", + slog.F("user_id", userID), + slog.F("name", name), + slog.Error(err), + ) + return skillspkg.ParsedSkill{}, xerrors.Errorf("parse personal skill body: %w", err) + } + return parsed, nil +} + func (p *Server) appendRootChatTools( ctx context.Context, tools []fantasy.AgentTool, @@ -7035,7 +7172,8 @@ func (p *Server) runChat( mcpTools []fantasy.AgentTool mcpCleanup func() workspaceMCPTools []fantasy.AgentTool - skills []chattool.SkillMeta + workspaceSkills []chattool.SkillMeta + personalSkills []skillspkg.Skill ) // Check if instruction files need to be (re-)persisted. // This happens when no context-file parts exist yet, or when @@ -7092,7 +7230,7 @@ func (p *Server) runChat( return workspaceCtx.getWorkspaceConn(instructionCtx) }, ) - skills = selectSkillMetasForInstructionRefresh( + workspaceSkills = selectSkillMetasForInstructionRefresh( persistedSkills, discoveredSkills, uuid.NullUUID{UUID: currentWorkspaceAgentID, Valid: hasCurrentWorkspaceAgent}, @@ -7112,8 +7250,12 @@ func (p *Server) runChat( // re-injected via InsertSystem after compaction drops // those messages. No workspace dial needed. instruction = instructionFromContextFiles(messages) - skills = persistedSkills + workspaceSkills = persistedSkills } + g2.Go(func() error { + personalSkills = p.fetchPersonalSkillMetadata(ctx, chat.OwnerID, logger) + return nil + }) g2.Go(func() error { resolvedUserPrompt = p.resolveUserPrompt(ctx, chat.OwnerID) return nil @@ -7153,11 +7295,19 @@ func (p *Server) runChat( if !isRootChat { subagentInstruction = defaultSubagentInstruction } + resolvedSkillsFor := func(workspaceSkills []chattool.SkillMeta) []skillspkg.ResolvedSkill { + return mergeTurnSkills(personalSkills, workspaceSkills) + } + resolveSkillAlias := func(alias string) (skillspkg.ResolvedSkill, error) { + return skillspkg.Lookup(resolvedSkillsFor(workspaceSkills), alias) + } + initialResolvedSkills := resolvedSkillsFor(workspaceSkills) + injectedSkillIndex := chattool.FormatResolvedSkillIndex(initialResolvedSkills) prompt = buildSystemPrompt( prompt, subagentInstruction, instruction, - skills, + initialResolvedSkills, resolvedUserPrompt, systemPromptBehaviorContext{ planMode: currentPlanMode, @@ -7559,7 +7709,7 @@ func (p *Server) runChat( workspaceCtx: &workspaceCtx, workspaceMu: &workspaceMu, instruction: &instruction, - skills: &skills, + skills: &workspaceSkills, resolvePlanPath: resolvePlanPathForTools, storeFile: storeChatAttachment, isPlanModeTurn: isPlanModeTurn, @@ -7567,19 +7717,43 @@ func (p *Server) runChat( }) } - // Append skill tools when the workspace has skills. - if len(skills) > 0 { - skillOpts := chattool.ReadSkillOptions{ - GetWorkspaceConn: workspaceCtx.getWorkspaceConn, - GetSkills: func() []chattool.SkillMeta { - return skills - }, - } - tools = append(tools, - chattool.ReadSkill(skillOpts), - chattool.ReadSkillFile(skillOpts), - ) + skillOpts := chattool.ReadSkillOptions{ + GetWorkspaceConn: workspaceCtx.getWorkspaceConn, + GetSkills: func() []chattool.SkillMeta { + return workspaceSkills + }, + ResolveAlias: resolveSkillAlias, + LoadPersonalSkillBody: func(ctx context.Context, name string) (skillspkg.ParsedSkill, error) { + return p.loadPersonalSkillBody(ctx, chat.OwnerID, name) + }, } + appendCurrentSkillTools := func(current []fantasy.AgentTool) ([]fantasy.AgentTool, bool) { + if len(personalSkills) == 0 && len(workspaceSkills) == 0 { + return current, false + } + + updated := current + changed := false + appendTool := func(tool fantasy.AgentTool) { + name := tool.Info().Name + if slices.ContainsFunc(current, func(existing fantasy.AgentTool) bool { + return existing.Info().Name == name + }) { + return + } + if !changed { + updated = slices.Clone(current) + changed = true + } + updated = append(updated, tool) + } + appendTool(chattool.ReadSkill(skillOpts)) + if len(workspaceSkills) > 0 { + appendTool(chattool.ReadSkillFile(skillOpts)) + } + return updated, changed + } + tools, _ = appendCurrentSkillTools(tools) if advisorRuntime != nil { tools = append(tools, chatadvisor.Tool(chatadvisor.ToolOptions{ Runtime: advisorRuntime, @@ -7853,14 +8027,16 @@ func (p *Server) runChat( } reloadedSkills := skillsFromParts(reloadedMsgs) if len(reloadedSkills) == 0 { - reloadedSkills = skills + reloadedSkills = workspaceSkills } + reloadedResolvedSkills := resolvedSkillsFor(reloadedSkills) + injectedSkillIndex = chattool.FormatResolvedSkillIndex(reloadedResolvedSkills) reloadUserPrompt := p.resolveUserPrompt(reloadCtx, chat.OwnerID) reloadedPrompt = buildSystemPrompt( reloadedPrompt, subagentInstruction, reloadedInstruction, - reloadedSkills, + reloadedResolvedSkills, reloadUserPrompt, systemPromptBehaviorContext{ planMode: currentPlanMode, @@ -7894,6 +8070,8 @@ func (p *Server) runChat( chainModeActive = false }, PrepareTools: func(currentTools []fantasy.AgentTool) []fantasy.AgentTool { + updatedTools, toolsChanged := appendCurrentSkillTools(currentTools) + // Mid-turn workspace MCP discovery for chats that bind a // workspace via create_workspace or start_workspace after the // turn has already started. The top-of-turn discovery path is @@ -7907,10 +8085,16 @@ func (p *Server) runChat( // always a cache hit. The primer's bounded wait means the // dial fallback here only runs when priming itself failed. if workspaceMCPDiscovered || isExploreSubagent { + if toolsChanged { + return updatedTools + } return nil } snapshot := workspaceCtx.currentChatSnapshot() if !snapshot.WorkspaceID.Valid { + if toolsChanged { + return updatedTools + } return nil } discovered := p.discoverWorkspaceMCPTools( @@ -7925,10 +8109,13 @@ func (p *Server) runChat( // plus one ListMCPTools RPC, both fast against a live // conn. The primer's 30s budget applies to its own // loop only. + if toolsChanged { + return updatedTools + } return nil } workspaceMCPDiscovered = true - return append(slices.Clone(currentTools), discovered...) + return append(slices.Clone(updatedTools), discovered...) }, PrepareMessages: func(msgs []fantasy.Message) []fantasy.Message { // Skip the snapshot update when chain mode is active; @@ -7939,13 +8126,21 @@ func (p *Server) runChat( if !chainModeActive { setAdvisorPromptSnapshot(msgs) } - if instructionInjected || instruction == "" { - return nil + result := msgs + changed := false + if !instructionInjected && instruction != "" { + instructionInjected = true + result = chatprompt.InsertSystem(result, instruction) + changed = true } - instructionInjected = true - result := chatprompt.InsertSystem(msgs, instruction) - if skillIndex := chattool.FormatSkillIndex(skills); skillIndex != "" { + if skillIndex := chattool.FormatResolvedSkillIndex(resolvedSkillsFor(workspaceSkills)); skillIndex != "" && skillIndex != injectedSkillIndex { + result = removeSkillIndexMessages(result) result = chatprompt.InsertSystem(result, skillIndex) + injectedSkillIndex = skillIndex + changed = true + } + if !changed { + return nil } if !chainModeActive { setAdvisorPromptSnapshot(result) diff --git a/coderd/x/chatd/chatd_internal_test.go b/coderd/x/chatd/chatd_internal_test.go index e18e0574cd..4eb7d393f5 100644 --- a/coderd/x/chatd/chatd_internal_test.go +++ b/coderd/x/chatd/chatd_internal_test.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "encoding/json" + "strings" "sync" "sync/atomic" "testing" @@ -23,12 +24,15 @@ import ( "github.com/coder/coder/v2/coderd/database/dbmock" dbpubsub "github.com/coder/coder/v2/coderd/database/pubsub" coderdpubsub "github.com/coder/coder/v2/coderd/pubsub" + "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/x/chatd/chaterror" "github.com/coder/coder/v2/coderd/x/chatd/chatloop" openaicomputeruse "github.com/coder/coder/v2/coderd/x/chatd/chatopenai/computeruse" + "github.com/coder/coder/v2/coderd/x/chatd/chatprompt" "github.com/coder/coder/v2/coderd/x/chatd/chatprovider" "github.com/coder/coder/v2/coderd/x/chatd/chattest" "github.com/coder/coder/v2/coderd/x/chatd/chattool" + skillspkg "github.com/coder/coder/v2/coderd/x/skills" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/workspacesdk" "github.com/coder/coder/v2/codersdk/workspacesdk/agentconnmock" @@ -3092,6 +3096,336 @@ func TestSkillsFromParts(t *testing.T) { }) } +func TestPersonalSkillsInSystemPrompt(t *testing.T) { + t.Parallel() + + prompt := buildSystemPrompt( + nil, + "", + "", + mergeTurnSkills( + []skillspkg.Skill{{ + Name: "personal-review", + Description: "Personal review process", + Source: skillspkg.SourcePersonal, + }}, + nil, + ), + "", + systemPromptBehaviorContext{}, + ) + + text := systemPromptText(t, prompt) + require.Contains(t, text, "") + require.Contains(t, text, "- personal-review: Personal review process") + require.NotContains(t, text, `"skill"`) +} + +func TestPersonalAndWorkspaceSkillCollisionInSystemPrompt(t *testing.T) { + t.Parallel() + + resolved := mergeTurnSkills( + []skillspkg.Skill{{ + Name: "deploy", + Description: "Personal deployment process", + Source: skillspkg.SourcePersonal, + }}, + []chattool.SkillMeta{{ + Name: "deploy", + Description: "Workspace deployment process", + Dir: "/skills/deploy", + }}, + ) + prompt := buildSystemPrompt( + nil, + "", + "", + resolved, + "", + systemPromptBehaviorContext{}, + ) + + text := systemPromptText(t, prompt) + require.Contains(t, text, "") + require.Contains(t, text, "- personal/deploy: Personal deployment process") + require.Contains(t, text, "- workspace/deploy: Workspace deployment process") + require.NotContains(t, text, "\n- deploy: ") + require.NotContains(t, text, "\n- deploy\n") + + personal, err := skillspkg.Lookup(resolved, "personal/deploy") + require.NoError(t, err) + require.Equal(t, "deploy", personal.Name) + require.Equal(t, skillspkg.SourcePersonal, personal.Source) + + workspace, err := skillspkg.Lookup(resolved, "workspace/deploy") + require.NoError(t, err) + require.Equal(t, "deploy", workspace.Name) + require.Equal(t, skillspkg.SourceWorkspace, workspace.Source) + + _, err = skillspkg.Lookup(resolved, "deploy") + require.ErrorIs(t, err, skillspkg.ErrSkillAmbiguous) + require.ErrorContains(t, err, "personal/deploy") + require.ErrorContains(t, err, "workspace/deploy") +} + +func TestSkillIndexRefreshReplacesStaleAliases(t *testing.T) { + t.Parallel() + + initialResolved := mergeTurnSkills( + []skillspkg.Skill{{ + Name: "deploy", + Description: "Personal deployment process", + Source: skillspkg.SourcePersonal, + }}, + nil, + ) + prompt := buildSystemPrompt( + []fantasy.Message{{ + Role: fantasy.MessageRoleUser, + Content: []fantasy.MessagePart{ + fantasy.TextPart{Text: "Create a workspace."}, + }, + }}, + "", + "", + initialResolved, + "", + systemPromptBehaviorContext{}, + ) + + mergedIndex := chattool.FormatResolvedSkillIndex(mergeTurnSkills( + []skillspkg.Skill{{ + Name: "deploy", + Description: "Personal deployment process", + Source: skillspkg.SourcePersonal, + }}, + []chattool.SkillMeta{{ + Name: "deploy", + Description: "Workspace deployment process", + Dir: "/skills/deploy", + }}, + )) + prompt = removeSkillIndexMessages(prompt) + prompt = chatprompt.InsertSystem(prompt, mergedIndex) + + text := systemPromptText(t, prompt) + require.Equal(t, 1, strings.Count(text, "")) + require.NotContains(t, text, "\n- deploy: Personal deployment process") + require.Contains(t, text, "- personal/deploy: Personal deployment process") + require.Contains(t, text, "- workspace/deploy: Workspace deployment process") +} + +func requireUserSkillContextActor(ctx context.Context, t *testing.T, userID uuid.UUID) { + t.Helper() + actor, ok := dbauthz.ActorFromContext(ctx) + require.True(t, ok) + require.Equal(t, rbac.SubjectTypeUser, actor.Type) + require.Equal(t, userID.String(), actor.ID) + require.Equal(t, rbac.RoleIdentifiers{rbac.RoleMember()}, actor.Roles) +} + +func TestFetchPersonalSkillMetadata(t *testing.T) { + t.Parallel() + + t.Run("Success", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug) + server := &Server{db: db} + userID := uuid.New() + + db.EXPECT().ListUserSkillMetadataByUserID(gomock.Any(), userID).DoAndReturn( + func(ctx context.Context, gotUserID uuid.UUID) ([]database.ListUserSkillMetadataByUserIDRow, error) { + requireUserSkillContextActor(ctx, t, userID) + require.Equal(t, userID, gotUserID) + return []database.ListUserSkillMetadataByUserIDRow{{ + UserID: userID, + Name: "personal-review", + Description: "Personal review process", + }}, nil + }, + ) + + got := server.fetchPersonalSkillMetadata(context.Background(), userID, logger) + require.Equal(t, []skillspkg.Skill{{ + Name: "personal-review", + Description: "Personal review process", + Source: skillspkg.SourcePersonal, + }}, got) + }) + + t.Run("ListFailure", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + sink := testutil.NewFakeSink(t) + logger := sink.Logger().Leveled(slog.LevelDebug) + server := &Server{db: db} + userID := uuid.New() + + db.EXPECT().ListUserSkillMetadataByUserID(gomock.Any(), userID).Return(nil, xerrors.New("boom")) + + got := server.fetchPersonalSkillMetadata(context.Background(), userID, logger) + require.Empty(t, got) + warns := sink.Entries(func(e slog.SinkEntry) bool { + return e.Level == slog.LevelWarn && strings.Contains(e.Message, "personal skill metadata") + }) + require.NotEmpty(t, warns) + }) +} + +func TestLoadPersonalSkillBody(t *testing.T) { + t.Parallel() + + t.Run("ParsesCurrentContent", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + server := &Server{db: db} + userID := uuid.New() + params := database.GetUserSkillByUserIDAndNameParams{ + UserID: userID, + Name: "personal-review", + } + + db.EXPECT().GetUserSkillByUserIDAndName(gomock.Any(), params).DoAndReturn( + func(ctx context.Context, gotParams database.GetUserSkillByUserIDAndNameParams) (database.UserSkill, error) { + requireUserSkillContextActor(ctx, t, userID) + require.Equal(t, params, gotParams) + return database.UserSkill{ + UserID: userID, + Name: "personal-review", + Content: "---\nname: personal-review\ndescription: Personal review process\n---\n\nUpdated instructions.\n", + }, nil + }, + ) + + got, err := server.loadPersonalSkillBody(context.Background(), userID, "personal-review") + require.NoError(t, err) + require.Equal(t, "personal-review", got.Name) + require.Equal(t, "Personal review process", got.Description) + require.Equal(t, skillspkg.SourcePersonal, got.Source) + require.Contains(t, got.Body, "Updated instructions.") + }) + + t.Run("DeletedSkill", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + server := &Server{db: db} + userID := uuid.New() + params := database.GetUserSkillByUserIDAndNameParams{ + UserID: userID, + Name: "missing-skill", + } + + db.EXPECT().GetUserSkillByUserIDAndName(gomock.Any(), params).DoAndReturn( + func(ctx context.Context, gotParams database.GetUserSkillByUserIDAndNameParams) (database.UserSkill, error) { + requireUserSkillContextActor(ctx, t, userID) + require.Equal(t, params, gotParams) + return database.UserSkill{}, sql.ErrNoRows + }, + ) + + _, err := server.loadPersonalSkillBody(context.Background(), userID, "missing-skill") + require.ErrorIs(t, err, skillspkg.ErrSkillNotFound) + }) + + t.Run("DatabaseError", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + sink := testutil.NewFakeSink(t) + server := &Server{db: db, logger: sink.Logger()} + userID := uuid.New() + params := database.GetUserSkillByUserIDAndNameParams{ + UserID: userID, + Name: "error-skill", + } + dbErr := xerrors.New("database unavailable") + + db.EXPECT().GetUserSkillByUserIDAndName(gomock.Any(), params).DoAndReturn( + func(ctx context.Context, gotParams database.GetUserSkillByUserIDAndNameParams) (database.UserSkill, error) { + requireUserSkillContextActor(ctx, t, userID) + require.Equal(t, params, gotParams) + return database.UserSkill{}, dbErr + }, + ) + + _, err := server.loadPersonalSkillBody(context.Background(), userID, "error-skill") + + require.ErrorContains(t, err, "load personal skill body") + require.ErrorIs(t, err, dbErr) + entries := sink.Entries(func(e slog.SinkEntry) bool { + return e.Level == slog.LevelError && e.Message == "load personal skill body failed" + }) + require.Len(t, entries, 1) + requireFieldValue(t, entries[0], "error", dbErr) + }) + + t.Run("ParseError", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + sink := testutil.NewFakeSink(t) + server := &Server{db: db, logger: sink.Logger()} + userID := uuid.New() + params := database.GetUserSkillByUserIDAndNameParams{ + UserID: userID, + Name: "broken-skill", + } + + db.EXPECT().GetUserSkillByUserIDAndName(gomock.Any(), params).DoAndReturn( + func(ctx context.Context, gotParams database.GetUserSkillByUserIDAndNameParams) (database.UserSkill, error) { + requireUserSkillContextActor(ctx, t, userID) + require.Equal(t, params, gotParams) + return database.UserSkill{ + UserID: userID, + Name: "broken-skill", + Content: "---\nname: broken-skill\ndescription: Broken\n---\n\n \n", + }, nil + }, + ) + + _, err := server.loadPersonalSkillBody(context.Background(), userID, "broken-skill") + + require.ErrorContains(t, err, "parse personal skill body") + require.ErrorIs(t, err, skillspkg.ErrSkillBodyRequired) + entries := sink.Entries(func(e slog.SinkEntry) bool { + return e.Level == slog.LevelError && e.Message == "parse personal skill body failed" + }) + require.Len(t, entries, 1) + requireFieldValue(t, entries[0], "user_id", userID) + requireFieldValue(t, entries[0], "name", "broken-skill") + }) +} + +func systemPromptText(t *testing.T, prompt []fantasy.Message) string { + t.Helper() + + var b strings.Builder + for _, msg := range prompt { + if msg.Role != fantasy.MessageRoleSystem { + continue + } + for _, part := range msg.Content { + textPart, ok := fantasy.AsMessagePart[fantasy.TextPart](part) + if ok { + _, _ = b.WriteString(textPart.Text) + _, _ = b.WriteString("\n") + } + } + } + return b.String() +} + func TestContextFileAgentID(t *testing.T) { t.Parallel() diff --git a/coderd/x/chatd/chattool/skill.go b/coderd/x/chatd/chattool/skill.go index bafdbf6415..93e6a943b1 100644 --- a/coderd/x/chatd/chattool/skill.go +++ b/coderd/x/chatd/chattool/skill.go @@ -11,12 +11,18 @@ import ( "charm.land/fantasy" "golang.org/x/xerrors" + skillspkg "github.com/coder/coder/v2/coderd/x/skills" "github.com/coder/coder/v2/codersdk/workspacesdk" ) const ( maxSkillMetaBytes = workspacesdk.MaxSkillMetaBytes maxSkillFileBytes = 512 * 1024 + + // AvailableSkillsOpenTag is the XML start tag for the skill index block. + AvailableSkillsOpenTag = "" + // AvailableSkillsCloseTag is the XML end tag for the skill index block. + AvailableSkillsCloseTag = "" ) // SkillMeta is the frontmatter from a skill meta file discovered in a @@ -45,32 +51,78 @@ type SkillContent struct { Files []string } -// FormatSkillIndex renders an XML block listing all discovered -// skills. This block is injected into the system prompt so the -// model knows which skills are available and how to load them. -func FormatSkillIndex(skills []SkillMeta) string { - if len(skills) == 0 { +// FormatResolvedSkillIndex renders an XML block listing all source-aware +// skills. Aliases are the names the model should pass to the skill tools. +func FormatResolvedSkillIndex(resolved []skillspkg.ResolvedSkill) string { + if len(resolved) == 0 { + return "" + } + + entries := make([]skillIndexEntry, 0, len(resolved)) + hasQualifiedAlias := false + hasWorkspaceSkill := false + for _, s := range resolved { + entries = append(entries, skillIndexEntry{ + Alias: s.Alias, + Description: s.Description, + }) + if s.Source == skillspkg.SourceWorkspace { + hasWorkspaceSkill = true + } + if s.Alias == skillspkg.QualifiedAlias(s.Source, s.Name) { + hasQualifiedAlias = true + } + } + return renderSkillIndex(entries, skillIndexFormatOptions{ + includeQualifiedAliasInstruction: hasQualifiedAlias, + includeReadSkillFileInstruction: hasWorkspaceSkill, + }) +} + +type skillIndexEntry struct { + Alias string + Description string +} + +type skillIndexFormatOptions struct { + includeQualifiedAliasInstruction bool + includeReadSkillFileInstruction bool +} + +func renderSkillIndex(entries []skillIndexEntry, opts skillIndexFormatOptions) string { + if len(entries) == 0 { return "" } var b strings.Builder - _, _ = b.WriteString("\n") + _, _ = b.WriteString(AvailableSkillsOpenTag + "\n") _, _ = b.WriteString( "Use read_skill to load a skill's full instructions " + - "before following them.\n" + - "Use read_skill_file to read supporting files " + - "referenced by a skill.\n\n", + "before following them.\n", ) - for _, s := range skills { + if opts.includeReadSkillFileInstruction { + _, _ = b.WriteString( + "Use read_skill_file to read supporting files " + + "referenced by a workspace skill.\n", + ) + } + if opts.includeQualifiedAliasInstruction { + _, _ = b.WriteString( + "When a skill is listed as personal/name or workspace/name, " + + "pass that qualified alias to read_skill.\n", + ) + } + _, _ = b.WriteString("\n") + for _, s := range entries { _, _ = b.WriteString("- ") - _, _ = b.WriteString(s.Name) + _, _ = b.WriteString(s.Alias) if s.Description != "" { _, _ = b.WriteString(": ") _, _ = b.WriteString(s.Description) } _, _ = b.WriteString("\n") } - _, _ = b.WriteString("") + _, _ = b.WriteString(AvailableSkillsCloseTag) return b.String() } @@ -216,11 +268,14 @@ const DefaultSkillMetaFile = "SKILL.md" type ReadSkillOptions struct { GetWorkspaceConn func(context.Context) (workspacesdk.AgentConn, error) GetSkills func() []SkillMeta + + ResolveAlias func(string) (skillspkg.ResolvedSkill, error) + LoadPersonalSkillBody func(context.Context, string) (skillspkg.ParsedSkill, error) } // ReadSkillArgs are the parameters accepted by read_skill. type ReadSkillArgs struct { - Name string `json:"name" description:"The kebab-case name of the skill to read."` + Name string `json:"name" description:"The name or qualified alias of the skill to read."` } // ReadSkill returns an AgentTool that reads the full instructions @@ -234,44 +289,51 @@ func ReadSkill(options ReadSkillOptions) fantasy.AgentTool { "supporting files. Use read_skill before "+ "following a skill's instructions.", func(ctx context.Context, args ReadSkillArgs, _ fantasy.ToolCall) (fantasy.ToolResponse, error) { - if options.GetWorkspaceConn == nil { - return fantasy.NewTextErrorResponse( - "workspace connection resolver is not configured", - ), nil - } if args.Name == "" { return fantasy.NewTextErrorResponse( "name is required", ), nil } - skill, ok := findSkill(options.GetSkills, args.Name) - if !ok { - return fantasy.NewTextErrorResponse( - fmt.Sprintf("skill %q not found", args.Name), - ), nil + resolved, err := resolveSkillAlias(options, args.Name) + if err != nil { + return skillResolveErrorResponse(args.Name, err), nil } - conn, err := options.GetWorkspaceConn(ctx) - if err != nil { - return fantasy.NewTextErrorResponse( - err.Error(), - ), nil + switch resolved.Source { + case skillspkg.SourcePersonal: + if options.LoadPersonalSkillBody == nil { + return fantasy.NewTextErrorResponse( + "personal skill loader is not configured", + ), nil + } + content, err := options.LoadPersonalSkillBody(ctx, resolved.Name) + if err != nil { + if xerrors.Is(err, skillspkg.ErrSkillNotFound) { + return skillNotFoundResponse(args.Name), nil + } + return fantasy.NewTextErrorResponse( + fmt.Sprintf("failed to load personal skill %q", args.Name), + ), nil + } + return toolResponse(map[string]any{ + "name": args.Name, + "body": content.Body, + "files": []string{}, + }), nil + case skillspkg.SourceWorkspace: + content, response, ok := readWorkspaceSkillBody(ctx, options, args.Name, resolved.Name) + if ok { + return response, nil + } + return toolResponse(map[string]any{ + "name": args.Name, + "body": content.Body, + "files": nonNilFiles(content.Files), + }), nil + default: + return skillNotFoundResponse(args.Name), nil } - - // Load the skill body from the workspace agent, - // respecting a custom meta file name if set. - content, err := LoadSkillBody(ctx, conn, skill, cmp.Or(skill.MetaFile, DefaultSkillMetaFile)) - if err != nil { - return fantasy.NewTextErrorResponse( - err.Error(), - ), nil - } - return toolResponse(map[string]any{ - "name": content.Name, - "body": content.Body, - "files": content.Files, - }), nil }, ) } @@ -279,7 +341,7 @@ func ReadSkill(options ReadSkillOptions) fantasy.AgentTool { // ReadSkillFileArgs are the parameters accepted by // read_skill_file. type ReadSkillFileArgs struct { - Name string `json:"name" description:"The kebab-case name of the skill."` + Name string `json:"name" description:"The name or qualified alias of the skill to read."` Path string `json:"path" description:"Relative path to a file in the skill directory (e.g. roles/security-reviewer.md)."` } @@ -291,11 +353,6 @@ func ReadSkillFile(options ReadSkillOptions) fantasy.AgentTool { "Read a supporting file from a skill's directory "+ "(e.g. roles/security-reviewer.md).", func(ctx context.Context, args ReadSkillFileArgs, _ fantasy.ToolCall) (fantasy.ToolResponse, error) { - if options.GetWorkspaceConn == nil { - return fantasy.NewTextErrorResponse( - "workspace connection resolver is not configured", - ), nil - } if args.Name == "" { return fantasy.NewTextErrorResponse( "name is required", @@ -307,12 +364,23 @@ func ReadSkillFile(options ReadSkillOptions) fantasy.AgentTool { ), nil } - skill, ok := findSkill(options.GetSkills, args.Name) - if !ok { + resolved, err := resolveSkillAlias(options, args.Name) + if err != nil { + return skillResolveErrorResponse(args.Name, err), nil + } + if resolved.Source == skillspkg.SourcePersonal { return fantasy.NewTextErrorResponse( - fmt.Sprintf("skill %q not found", args.Name), + "read_skill_file is not supported for personal skills (no supporting files)", ), nil } + if resolved.Source != skillspkg.SourceWorkspace { + return skillNotFoundResponse(args.Name), nil + } + + skill, ok := findSkill(options.GetSkills, resolved.Name) + if !ok { + return skillNotFoundResponse(args.Name), nil + } // Validate the path early so we reject bad // inputs before dialing the workspace agent. @@ -322,6 +390,11 @@ func ReadSkillFile(options ReadSkillOptions) fantasy.AgentTool { ), nil } + if options.GetWorkspaceConn == nil { + return fantasy.NewTextErrorResponse( + "workspace connection resolver is not configured", + ), nil + } conn, err := options.GetWorkspaceConn(ctx) if err != nil { return fantasy.NewTextErrorResponse( @@ -345,6 +418,78 @@ func ReadSkillFile(options ReadSkillOptions) fantasy.AgentTool { ) } +func resolveSkillAlias(options ReadSkillOptions, name string) (skillspkg.ResolvedSkill, error) { + if options.ResolveAlias != nil { + return options.ResolveAlias(name) + } + + skill, ok := findSkill(options.GetSkills, name) + if !ok { + return skillspkg.ResolvedSkill{}, skillspkg.ErrSkillNotFound + } + return skillspkg.ResolvedSkill{ + Skill: skillspkg.Skill{ + Name: skill.Name, + Description: skill.Description, + Source: skillspkg.SourceWorkspace, + }, + Alias: skill.Name, + }, nil +} + +func readWorkspaceSkillBody( + ctx context.Context, + options ReadSkillOptions, + requestedName string, + canonicalName string, +) (SkillContent, fantasy.ToolResponse, bool) { + skill, ok := findSkill(options.GetSkills, canonicalName) + if !ok { + return SkillContent{}, skillNotFoundResponse(requestedName), true + } + if options.GetWorkspaceConn == nil { + return SkillContent{}, fantasy.NewTextErrorResponse( + "workspace connection resolver is not configured", + ), true + } + + conn, err := options.GetWorkspaceConn(ctx) + if err != nil { + return SkillContent{}, fantasy.NewTextErrorResponse(err.Error()), true + } + + content, err := LoadSkillBody(ctx, conn, skill, cmp.Or(skill.MetaFile, DefaultSkillMetaFile)) + if err != nil { + return SkillContent{}, fantasy.NewTextErrorResponse(err.Error()), true + } + return content, fantasy.ToolResponse{}, false +} + +func skillResolveErrorResponse(name string, err error) fantasy.ToolResponse { + if xerrors.Is(err, skillspkg.ErrSkillNotFound) { + return skillNotFoundResponse(name) + } + if xerrors.Is(err, skillspkg.ErrSkillAmbiguous) { + return fantasy.NewTextErrorResponse(err.Error()) + } + return fantasy.NewTextErrorResponse( + fmt.Sprintf("failed to resolve skill %q", name), + ) +} + +func skillNotFoundResponse(name string) fantasy.ToolResponse { + return fantasy.NewTextErrorResponse( + fmt.Sprintf("skill %q not found", name), + ) +} + +func nonNilFiles(files []string) []string { + if files == nil { + return []string{} + } + return files +} + // findSkill looks up a skill by name in the current skill list. func findSkill( getSkills func() []SkillMeta, diff --git a/coderd/x/chatd/chattool/skill_test.go b/coderd/x/chatd/chattool/skill_test.go index f4697131f7..2505fe8083 100644 --- a/coderd/x/chatd/chattool/skill_test.go +++ b/coderd/x/chatd/chattool/skill_test.go @@ -2,6 +2,7 @@ package chattool_test import ( "context" + "encoding/json" "io" "strings" "testing" @@ -13,6 +14,7 @@ import ( "golang.org/x/xerrors" "github.com/coder/coder/v2/coderd/x/chatd/chattool" + skillspkg "github.com/coder/coder/v2/coderd/x/skills" "github.com/coder/coder/v2/codersdk/workspacesdk" "github.com/coder/coder/v2/codersdk/workspacesdk/agentconnmock" ) @@ -23,27 +25,101 @@ func validSkillMD(name, description string) string { return "---\nname: " + name + "\ndescription: " + description + "\n---\n\n# Instructions\n\nDo the thing.\n" } -func TestFormatSkillIndex(t *testing.T) { +func responseName(t *testing.T, resp fantasy.ToolResponse) string { + t.Helper() + + var payload struct { + Name string `json:"name"` + } + require.NoError(t, json.Unmarshal([]byte(resp.Content), &payload)) + return payload.Name +} + +func TestFormatResolvedSkillIndex(t *testing.T) { t.Parallel() t.Run("Empty", func(t *testing.T) { t.Parallel() - assert.Empty(t, chattool.FormatSkillIndex(nil)) + assert.Empty(t, chattool.FormatResolvedSkillIndex(nil)) }) - t.Run("RendersIndex", func(t *testing.T) { + t.Run("PersonalOnly", func(t *testing.T) { t.Parallel() - skills := []chattool.SkillMeta{ - {Name: "alpha", Description: "First"}, - {Name: "beta", Description: "Second"}, - } - idx := chattool.FormatSkillIndex(skills) - assert.Contains(t, idx, "") - assert.Contains(t, idx, "- alpha: First") - assert.Contains(t, idx, "- beta: Second") - assert.Contains(t, idx, "") - assert.Contains(t, idx, "read_skill") + idx := chattool.FormatResolvedSkillIndex([]skillspkg.ResolvedSkill{{ + Skill: skillspkg.Skill{ + Name: "personal-review", + Description: "Personal review process", + Source: skillspkg.SourcePersonal, + }, + Alias: "personal-review", + }}) + assert.Contains(t, idx, "- personal-review: Personal review process") + assert.NotContains(t, idx, "read_skill_file") + assert.NotContains(t, idx, "qualified alias") + }) + + t.Run("WorkspaceOnlyMatchesLegacy", func(t *testing.T) { + t.Parallel() + + resolved := []skillspkg.ResolvedSkill{{ + Skill: skillspkg.Skill{ + Name: "deep-review", + Description: "Review", + Source: skillspkg.SourceWorkspace, + }, + Alias: "deep-review", + }} + assert.Equal(t, + "\n"+ + "Use read_skill to load a skill's full instructions before following them.\n"+ + "Use read_skill_file to read supporting files referenced by a workspace skill.\n"+ + "\n"+ + "- deep-review: Review\n"+ + "", + chattool.FormatResolvedSkillIndex(resolved), + ) + }) + + t.Run("MixedNonColliding", func(t *testing.T) { + t.Parallel() + + idx := chattool.FormatResolvedSkillIndex([]skillspkg.ResolvedSkill{ + { + Skill: skillspkg.Skill{ + Name: "personal-review", + Description: "Personal review process", + Source: skillspkg.SourcePersonal, + }, + Alias: "personal-review", + }, + { + Skill: skillspkg.Skill{ + Name: "deep-review", + Description: "Workspace review process", + Source: skillspkg.SourceWorkspace, + }, + Alias: "deep-review", + }, + }) + assert.Contains(t, idx, "- personal-review: Personal review process") + assert.Contains(t, idx, "- deep-review: Workspace review process") + assert.Contains(t, idx, "read_skill_file") + assert.NotContains(t, idx, "personal/personal-review") + assert.NotContains(t, idx, "workspace/deep-review") + }) + + t.Run("CollidingNames", func(t *testing.T) { + t.Parallel() + + resolved := skillspkg.MergeSkills( + []skillspkg.Skill{{Name: "review", Description: "Personal", Source: skillspkg.SourcePersonal}}, + []skillspkg.Skill{{Name: "review", Description: "Workspace", Source: skillspkg.SourceWorkspace}}, + ) + idx := chattool.FormatResolvedSkillIndex(resolved) + assert.Contains(t, idx, "- personal/review: Personal") + assert.Contains(t, idx, "- workspace/review: Workspace") + assert.Contains(t, idx, "pass that qualified alias to read_skill") }) } @@ -277,6 +353,326 @@ func TestReadSkillTool(t *testing.T) { assert.Contains(t, resp.Content, "Do the thing.") }) + t.Run("PersonalSkill", func(t *testing.T) { + t.Parallel() + + tool := chattool.ReadSkill(chattool.ReadSkillOptions{ + ResolveAlias: func(alias string) (skillspkg.ResolvedSkill, error) { + require.Equal(t, "my-skill", alias) + return skillspkg.ResolvedSkill{ + Skill: skillspkg.Skill{ + Name: "my-skill", + Description: "test", + Source: skillspkg.SourcePersonal, + }, + Alias: "my-skill", + }, nil + }, + LoadPersonalSkillBody: func(context.Context, string) (skillspkg.ParsedSkill, error) { + return skillspkg.ParsedSkill{ + Skill: skillspkg.Skill{ + Name: "my-skill", + Description: "test", + Source: skillspkg.SourcePersonal, + }, + Body: "Personal instructions.", + }, nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "read_skill", + Input: `{"name":"my-skill"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.Contains(t, resp.Content, "Personal instructions.") + assert.Contains(t, resp.Content, `"files":[]`) + }) + + t.Run("PersonalQualifiedAliasPreservesAlias", func(t *testing.T) { + t.Parallel() + + var loadedName string + tool := chattool.ReadSkill(chattool.ReadSkillOptions{ + ResolveAlias: func(alias string) (skillspkg.ResolvedSkill, error) { + require.Equal(t, "personal/my-skill", alias) + return skillspkg.ResolvedSkill{ + Skill: skillspkg.Skill{ + Name: "my-skill", + Description: "test", + Source: skillspkg.SourcePersonal, + }, + Alias: "personal/my-skill", + }, nil + }, + LoadPersonalSkillBody: func(_ context.Context, name string) (skillspkg.ParsedSkill, error) { + loadedName = name + return skillspkg.ParsedSkill{ + Skill: skillspkg.Skill{ + Name: "my-skill", + Description: "test", + Source: skillspkg.SourcePersonal, + }, + Body: "Personal instructions.", + }, nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "read_skill", + Input: `{"name":"personal/my-skill"}`, + }) + + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.Equal(t, "personal/my-skill", responseName(t, resp)) + assert.Equal(t, "my-skill", loadedName) + }) + + t.Run("WorkspaceQualifiedAlias", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + skills := []chattool.SkillMeta{{ + Name: "my-skill", + Description: "test", + Dir: "/work/.agents/skills/my-skill", + }} + + conn.EXPECT().ReadFile( + gomock.Any(), gomock.Any(), int64(0), gomock.Any(), + ).Return( + io.NopCloser(strings.NewReader(validSkillMD("my-skill", "test"))), + "text/markdown", + nil, + ) + conn.EXPECT().LS(gomock.Any(), "", gomock.Any()).Return( + workspacesdk.LSResponse{}, nil, + ) + + tool := chattool.ReadSkill(chattool.ReadSkillOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return conn, nil + }, + GetSkills: func() []chattool.SkillMeta { return skills }, + ResolveAlias: func(alias string) (skillspkg.ResolvedSkill, error) { + require.Equal(t, "workspace/my-skill", alias) + return skillspkg.ResolvedSkill{ + Skill: skillspkg.Skill{ + Name: "my-skill", + Description: "test", + Source: skillspkg.SourceWorkspace, + }, + Alias: "workspace/my-skill", + }, nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "read_skill", + Input: `{"name":"workspace/my-skill"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.Equal(t, "workspace/my-skill", responseName(t, resp)) + assert.Contains(t, resp.Content, "Do the thing.") + }) + + t.Run("CollisionAliasRoundTrip", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + workspaceSkills := []chattool.SkillMeta{{ + Name: "deploy", + Description: "workspace deploy", + Dir: "/work/.agents/skills/deploy", + }} + + conn.EXPECT().ReadFile( + gomock.Any(), gomock.Any(), int64(0), gomock.Any(), + ).Return( + io.NopCloser(strings.NewReader(validSkillMD("deploy", "workspace deploy"))), + "text/markdown", + nil, + ) + conn.EXPECT().LS(gomock.Any(), "", gomock.Any()).Return( + workspacesdk.LSResponse{}, nil, + ) + + resolveAlias := func(alias string) (skillspkg.ResolvedSkill, error) { + switch alias { + case "personal/deploy": + return skillspkg.ResolvedSkill{ + Skill: skillspkg.Skill{ + Name: "deploy", + Description: "personal deploy", + Source: skillspkg.SourcePersonal, + }, + Alias: "personal/deploy", + }, nil + case "workspace/deploy": + return skillspkg.ResolvedSkill{ + Skill: skillspkg.Skill{ + Name: "deploy", + Description: "workspace deploy", + Source: skillspkg.SourceWorkspace, + }, + Alias: "workspace/deploy", + }, nil + default: + return skillspkg.ResolvedSkill{}, skillspkg.ErrSkillNotFound + } + } + tool := chattool.ReadSkill(chattool.ReadSkillOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return conn, nil + }, + GetSkills: func() []chattool.SkillMeta { return workspaceSkills }, + ResolveAlias: resolveAlias, + LoadPersonalSkillBody: func(_ context.Context, name string) (skillspkg.ParsedSkill, error) { + require.Equal(t, "deploy", name) + return skillspkg.ParsedSkill{ + Skill: skillspkg.Skill{ + Name: "deploy", + Description: "personal deploy", + Source: skillspkg.SourcePersonal, + }, + Body: "Personal deploy instructions.", + }, nil + }, + }) + + workspaceResp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "read_skill", + Input: `{"name":"workspace/deploy"}`, + }) + require.NoError(t, err) + assert.False(t, workspaceResp.IsError) + workspaceName := responseName(t, workspaceResp) + assert.Equal(t, "workspace/deploy", workspaceName) + workspaceResolved, err := resolveAlias(workspaceName) + require.NoError(t, err) + assert.Equal(t, skillspkg.SourceWorkspace, workspaceResolved.Source) + + personalResp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-2", + Name: "read_skill", + Input: `{"name":"personal/deploy"}`, + }) + require.NoError(t, err) + assert.False(t, personalResp.IsError) + personalName := responseName(t, personalResp) + assert.Equal(t, "personal/deploy", personalName) + personalResolved, err := resolveAlias(personalName) + require.NoError(t, err) + assert.Equal(t, skillspkg.SourcePersonal, personalResolved.Source) + + _, err = resolveAlias("deploy") + require.ErrorIs(t, err, skillspkg.ErrSkillNotFound) + }) + + t.Run("MissingPersonalSkill", func(t *testing.T) { + t.Parallel() + + tool := chattool.ReadSkill(chattool.ReadSkillOptions{ + ResolveAlias: func(alias string) (skillspkg.ResolvedSkill, error) { + return skillspkg.ResolvedSkill{ + Skill: skillspkg.Skill{Name: alias, Source: skillspkg.SourcePersonal}, + Alias: alias, + }, nil + }, + LoadPersonalSkillBody: func(context.Context, string) (skillspkg.ParsedSkill, error) { + return skillspkg.ParsedSkill{}, skillspkg.ErrSkillNotFound + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "read_skill", + Input: `{"name":"missing-skill"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, `skill "missing-skill" not found`) + }) + + t.Run("PersonalSkillLoaderErrorIsSanitized", func(t *testing.T) { + t.Parallel() + + tool := chattool.ReadSkill(chattool.ReadSkillOptions{ + ResolveAlias: func(alias string) (skillspkg.ResolvedSkill, error) { + return skillspkg.ResolvedSkill{ + Skill: skillspkg.Skill{Name: alias, Source: skillspkg.SourcePersonal}, + Alias: alias, + }, nil + }, + LoadPersonalSkillBody: func(context.Context, string) (skillspkg.ParsedSkill, error) { + return skillspkg.ParsedSkill{}, xerrors.New("synthetic private storage failure") + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "read_skill", + Input: `{"name":"my-skill"}`, + }) + + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, `failed to load personal skill "my-skill"`) + assert.NotContains(t, resp.Content, "synthetic private storage failure") + }) + + t.Run("ResolveAliasErrorIsSanitized", func(t *testing.T) { + t.Parallel() + + tool := chattool.ReadSkill(chattool.ReadSkillOptions{ + ResolveAlias: func(string) (skillspkg.ResolvedSkill, error) { + return skillspkg.ResolvedSkill{}, xerrors.New("synthetic private resolver failure") + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "read_skill", + Input: `{"name":"my-skill"}`, + }) + + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, `failed to resolve skill "my-skill"`) + assert.NotContains(t, resp.Content, "synthetic private resolver failure") + }) + + t.Run("AmbiguousLookupSurfacesAliases", func(t *testing.T) { + t.Parallel() + + tool := chattool.ReadSkill(chattool.ReadSkillOptions{ + ResolveAlias: ambiguousResolveAliasForTest, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "read_skill", + Input: `{"name":"deploy"}`, + }) + + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, "skill lookup is ambiguous") + assert.Contains(t, resp.Content, "personal/deploy") + assert.Contains(t, resp.Content, "workspace/deploy") + }) + t.Run("UnknownSkill", func(t *testing.T) { t.Parallel() @@ -320,6 +716,19 @@ func TestReadSkillTool(t *testing.T) { }) } +func ambiguousResolveAliasForTest(alias string) (skillspkg.ResolvedSkill, error) { + return skillspkg.Lookup([]skillspkg.ResolvedSkill{ + { + Skill: skillspkg.Skill{Name: "deploy", Source: skillspkg.SourcePersonal}, + Alias: "personal/deploy", + }, + { + Skill: skillspkg.Skill{Name: "deploy", Source: skillspkg.SourceWorkspace}, + Alias: "workspace/deploy", + }, + }, alias) +} + func TestReadSkillFileTool(t *testing.T) { t.Parallel() @@ -362,6 +771,48 @@ func TestReadSkillFileTool(t *testing.T) { assert.Contains(t, resp.Content, "reviewer guide") }) + t.Run("PersonalSkillUnsupported", func(t *testing.T) { + t.Parallel() + + tool := chattool.ReadSkillFile(chattool.ReadSkillOptions{ + ResolveAlias: func(alias string) (skillspkg.ResolvedSkill, error) { + return skillspkg.ResolvedSkill{ + Skill: skillspkg.Skill{Name: alias, Source: skillspkg.SourcePersonal}, + Alias: alias, + }, nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "read_skill_file", + Input: `{"name":"my-skill","path":"helper.md"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, "not supported for personal skills") + }) + + t.Run("AmbiguousLookupSurfacesAliases", func(t *testing.T) { + t.Parallel() + + tool := chattool.ReadSkillFile(chattool.ReadSkillOptions{ + ResolveAlias: ambiguousResolveAliasForTest, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "read_skill_file", + Input: `{"name":"deploy","path":"helper.md"}`, + }) + + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, "skill lookup is ambiguous") + assert.Contains(t, resp.Content, "personal/deploy") + assert.Contains(t, resp.Content, "workspace/deploy") + }) + t.Run("TraversalRejected", func(t *testing.T) { t.Parallel()