diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index 1a9c37cbf2..a2fd8c5eee 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -125,6 +125,10 @@ type Server struct { // keyed by chat ID and invalidated when the agent changes. workspaceMCPToolsCache sync.Map // uuid.UUID -> *cachedWorkspaceMCPTools + // skillsCache caches discovered skill metadata per chat so + // we avoid re-scanning .agents/skills/ on every turn. + skillsCache sync.Map // uuid.UUID -> *cachedSkills + usageTracker *workspacestats.UsageTracker clock quartz.Clock @@ -172,6 +176,86 @@ type cachedWorkspaceMCPTools struct { tools []workspacesdk.MCPToolInfo } +// cachedSkills stores discovered skill metadata from a workspace +// agent, keyed by the agent ID that provided them. +type cachedSkills struct { + agentID uuid.UUID + skills []chattool.SkillMeta +} + +// discoverWorkspaceSkills returns cached skill metadata for a chat +// or discovers them fresh using the provided agent connection. The +// result is cached per chat+agent so subsequent turns skip the +// filesystem scan. +func (p *Server) discoverWorkspaceSkills( + ctx context.Context, + chatID uuid.UUID, + agent database.WorkspaceAgent, + conn workspacesdk.AgentConn, + logger slog.Logger, +) []chattool.SkillMeta { + // Check cache first. + if cached, ok := p.skillsCache.Load(chatID); ok { + if entry, ok2 := cached.(*cachedSkills); ok2 { + if entry.agentID == agent.ID { + return entry.skills + } + } + } + + dir := agent.ExpandedDirectory + if dir == "" { + dir = agent.Directory + } + discovered, err := chattool.DiscoverSkills(ctx, conn, dir) + if err != nil { + logger.Warn(ctx, "failed to discover skills", + slog.Error(err)) + return nil + } + // Cache the result. Unlike MCP tools, an empty skills + // list is a valid stable state (the workspace simply has + // no skills), so we always cache. + p.skillsCache.Store(chatID, &cachedSkills{ + agentID: agent.ID, + skills: discovered, + }) + return discovered +} + +// loadCachedWorkspaceContext checks the MCP tools and skills caches +// for the given chat and agent. Returns non-nil tools when the MCP +// cache hits, which signals the caller to skip the slow discovery +// path. Skills may also be populated from the skills cache. +func (p *Server) loadCachedWorkspaceContext( + chatID uuid.UUID, + agent database.WorkspaceAgent, + getConn func(context.Context) (workspacesdk.AgentConn, error), +) ([]fantasy.AgentTool, []chattool.SkillMeta) { + cached, ok := p.workspaceMCPToolsCache.Load(chatID) + if !ok { + return nil, nil + } + entry, ok := cached.(*cachedWorkspaceMCPTools) + if !ok || entry.agentID != agent.ID { + return nil, nil + } + + var tools []fantasy.AgentTool + for _, t := range entry.tools { + tools = append(tools, chattool.NewWorkspaceMCPTool(t, getConn)) + } + + var skills []chattool.SkillMeta + if sc, ok := p.skillsCache.Load(chatID); ok { + if se, ok := sc.(*cachedSkills); ok && se.agentID == agent.ID { + skills = se.skills + } + } + + return tools, skills +} + type turnWorkspaceContext struct { server *Server chatStateMu *sync.Mutex @@ -2572,6 +2656,7 @@ func (p *Server) cleanupStreamIfIdle(chatID uuid.UUID, state *chatStreamState) { if !state.buffering && len(state.subscribers) == 0 { p.chatStreams.Delete(chatID) p.workspaceMCPToolsCache.Delete(chatID) + p.skillsCache.Delete(chatID) } } @@ -3804,6 +3889,7 @@ func (p *Server) runChat( mcpTools []fantasy.AgentTool mcpCleanup func() workspaceMCPTools []fantasy.AgentTool + skills []chattool.SkillMeta ) // Check if instruction files need to be (re-)persisted. // This happens when no context-file parts exist yet, or when @@ -3872,21 +3958,14 @@ func (p *Server) runChat( // agent (ensureWorkspaceAgent is free when already // loaded). This avoids a per-turn latest-build DB // query on the common subsequent-turn path. - if agent, err := workspaceCtx.getWorkspaceAgent(ctx); err == nil { - if cached, ok := p.workspaceMCPToolsCache.Load(chat.ID); ok { - entry, ok := cached.(*cachedWorkspaceMCPTools) - if ok && entry.agentID == agent.ID { - for _, t := range entry.tools { - workspaceMCPTools = append(workspaceMCPTools, - chattool.NewWorkspaceMCPTool(t, workspaceCtx.getWorkspaceConn), - ) - } - return nil - } + agent, agentErr := workspaceCtx.getWorkspaceAgent(ctx) + if agentErr == nil { + if workspaceMCPTools, skills = p.loadCachedWorkspaceContext( + chat.ID, agent, workspaceCtx.getWorkspaceConn, + ); workspaceMCPTools != nil { + return nil } - } - - // Cache miss, agent changed, or no cache — validate + } // Cache miss, agent changed, or no cache: validate // that the workspace still has a live agent before // attempting a dial. workspaceMCPCtx, cancel := context.WithTimeout( @@ -3895,12 +3974,11 @@ func (p *Server) runChat( ) defer cancel() - _, _, agentErr := workspaceCtx.workspaceAgentIDForConn( - workspaceMCPCtx, - ) + _, _, agentErr = workspaceCtx.workspaceAgentIDForConn(workspaceMCPCtx) if agentErr != nil { if xerrors.Is(agentErr, errChatHasNoWorkspaceAgent) { p.workspaceMCPToolsCache.Delete(chat.ID) + p.skillsCache.Delete(chat.ID) return nil } logger.Warn(ctx, "failed to resolve workspace agent for MCP tools", @@ -3908,13 +3986,22 @@ func (p *Server) runChat( return nil } - // Fetch fresh tools from the workspace agent. + // Discover skills and MCP tools using the + // same conn to avoid a second dial attempt. conn, connErr := workspaceCtx.getWorkspaceConn(workspaceMCPCtx) if connErr != nil { logger.Warn(ctx, "failed to get workspace conn for MCP tools", slog.Error(connErr)) return nil } + + agent, agentErr = workspaceCtx.getWorkspaceAgent(workspaceMCPCtx) + if agentErr == nil { + skills = p.discoverWorkspaceSkills( + workspaceMCPCtx, chat.ID, agent, conn, logger, + ) + } + toolsResp, listErr := conn.ListMCPTools(workspaceMCPCtx) if listErr != nil { logger.Warn(ctx, "failed to list workspace MCP tools", @@ -3963,6 +4050,9 @@ func (p *Server) runChat( if instruction != "" { prompt = chatprompt.InsertSystem(prompt, instruction) } + if skillIndex := chattool.FormatSkillIndex(skills); skillIndex != "" { + prompt = chatprompt.InsertSystem(prompt, skillIndex) + } if resolvedUserPrompt != "" { prompt = chatprompt.InsertSystem(prompt, resolvedUserPrompt) } @@ -4341,6 +4431,20 @@ 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), + ) + } + // Append tools from external MCP servers. These appear // after the built-in tools so the LLM sees them as // additional capabilities. @@ -4430,6 +4534,9 @@ func (p *Server) runChat( if instruction != "" { reloadedPrompt = chatprompt.InsertSystem(reloadedPrompt, instruction) } + if skillIndex := chattool.FormatSkillIndex(skills); skillIndex != "" { + reloadedPrompt = chatprompt.InsertSystem(reloadedPrompt, skillIndex) + } reloadUserPrompt := p.resolveUserPrompt(reloadCtx, chat.OwnerID) if reloadUserPrompt != "" { reloadedPrompt = chatprompt.InsertSystem(reloadedPrompt, reloadUserPrompt) diff --git a/coderd/x/chatd/chattool/skill.go b/coderd/x/chatd/chattool/skill.go new file mode 100644 index 0000000000..757cf96d14 --- /dev/null +++ b/coderd/x/chatd/chattool/skill.go @@ -0,0 +1,511 @@ +package chattool + +import ( + "context" + "fmt" + "io" + "path" + "regexp" + "strings" + + "charm.land/fantasy" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/codersdk/workspacesdk" +) + +const ( + agentsSkillsDir = ".agents/skills" + skillMetaFile = "SKILL.md" + maxSkillMetaBytes = 64 * 1024 + maxSkillFileBytes = 512 * 1024 +) + +// skillNamePattern validates kebab-case skill names. Each segment +// must start with a lowercase letter or digit, and segments are +// separated by single hyphens. +var skillNamePattern = regexp.MustCompile( + `^[a-z0-9]+(-[a-z0-9]+)*$`, +) + +// markdownCommentRe strips HTML comments from skill bodies so +// they don't leak into the prompt. Matches the same pattern +// used by instruction.go in the parent package. +var markdownCommentRe = regexp.MustCompile(``) + +// SkillMeta is the frontmatter from a SKILL.md discovered in a +// workspace. It carries just enough information to list the skill +// in the prompt index without reading the full body. +type SkillMeta struct { + Name string + Description string + // Dir is the absolute path to the skill directory inside + // the workspace filesystem. + Dir string +} + +// SkillContent is the full body of a skill, loaded on demand +// when the model calls read_skill. +type SkillContent struct { + SkillMeta + // Body is the markdown content after the frontmatter + // delimiters have been stripped. + Body string + // Files lists relative paths of supporting files in the + // skill directory (everything except SKILL.md itself). + Files []string +} + +// DiscoverSkills walks the .agents/skills directory inside the +// workspace and returns metadata for every valid skill it finds. +// Missing directories or individual read errors are silently +// skipped so that a partially broken skills tree never blocks the +// conversation. +func DiscoverSkills( + ctx context.Context, + conn workspacesdk.AgentConn, + workingDir string, +) ([]SkillMeta, error) { + skillsDirPath := path.Join(workingDir, agentsSkillsDir) + + lsResp, err := conn.LS(ctx, "", workspacesdk.LSRequest{ + Path: []string{skillsDirPath}, + Relativity: workspacesdk.LSRelativityRoot, + }) + if err != nil { + // The skills directory is entirely optional. Return + // nil for any error so skill discovery never blocks + // the conversation. + return nil, nil + } + + var skills []SkillMeta + for _, entry := range lsResp.Contents { + if !entry.IsDir { + continue + } + + metaPath := path.Join( + entry.AbsolutePathString, skillMetaFile, + ) + reader, _, err := conn.ReadFile( + ctx, metaPath, 0, maxSkillMetaBytes+1, + ) + if err != nil { + // The directory may have been removed between the + // LS and this read, or it simply lacks a SKILL.md. + // Any error is non-fatal. + continue + } + raw, err := io.ReadAll(io.LimitReader(reader, maxSkillMetaBytes+1)) + reader.Close() + if err != nil { + continue + } + + // Silently truncate oversized metadata files so a + // single large file cannot exhaust memory. + if int64(len(raw)) > maxSkillMetaBytes { + raw = raw[:maxSkillMetaBytes] + } + + name, description, _, err := parseSkillFrontmatter( + string(raw), + ) + if err != nil { + continue + } + + // The directory name must match the declared name so + // skill references are unambiguous. + if name != entry.Name { + continue + } + if !skillNamePattern.MatchString(name) { + continue + } + + skills = append(skills, SkillMeta{ + Name: name, + Description: description, + Dir: entry.AbsolutePathString, + }) + } + + return skills, nil +} + +// parseSkillFrontmatter extracts name, description, and the +// markdown body from a SKILL.md file. The frontmatter uses a +// simple `key: value` format between `---` delimiters, and no +// full YAML parser is needed. +func parseSkillFrontmatter( + content string, +) (name, description, body string, err error) { + content = strings.TrimPrefix(content, "\xef\xbb\xbf") + lines := strings.Split(content, "\n") + if len(lines) == 0 || strings.TrimSpace(lines[0]) != "---" { + return "", "", "", xerrors.New( + "missing opening frontmatter delimiter", + ) + } + + closingIdx := -1 + for i := 1; i < len(lines); i++ { + if strings.TrimSpace(lines[i]) == "---" { + closingIdx = i + break + } + } + if closingIdx < 0 { + return "", "", "", xerrors.New( + "missing closing frontmatter delimiter", + ) + } + + for _, line := range lines[1:closingIdx] { + key, value, ok := strings.Cut(line, ":") + if !ok { + continue + } + key = strings.TrimSpace(key) + value = strings.TrimSpace(value) + // Strip surrounding quotes from YAML string values. + if len(value) >= 2 { + if (value[0] == '"' && value[len(value)-1] == '"') || + (value[0] == '\'' && value[len(value)-1] == '\'') { + value = value[1 : len(value)-1] + } + } + switch strings.ToLower(key) { + case "name": + name = value + case "description": + description = value + } + } + + if name == "" { + return "", "", "", xerrors.New( + "frontmatter missing required 'name' field", + ) + } + + // Everything after the closing delimiter is the body. + body = strings.Join(lines[closingIdx+1:], "\n") + body = markdownCommentRe.ReplaceAllString(body, "") + body = strings.TrimSpace(body) + + return name, description, body, nil +} + +// 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 { + return "" + } + + var b strings.Builder + _, _ = b.WriteString("\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", + ) + for _, s := range skills { + _, _ = b.WriteString("- ") + _, _ = b.WriteString(s.Name) + if s.Description != "" { + _, _ = b.WriteString(": ") + _, _ = b.WriteString(s.Description) + } + _, _ = b.WriteString("\n") + } + _, _ = b.WriteString("") + return b.String() +} + +// LoadSkillBody reads the full SKILL.md for a discovered skill +// and lists the supporting files in its directory. The caller +// should have already obtained the SkillMeta from DiscoverSkills. +func LoadSkillBody( + ctx context.Context, + conn workspacesdk.AgentConn, + skill SkillMeta, +) (SkillContent, error) { + metaPath := path.Join(skill.Dir, skillMetaFile) + + reader, _, err := conn.ReadFile( + ctx, metaPath, 0, maxSkillMetaBytes+1, + ) + if err != nil { + return SkillContent{}, xerrors.Errorf( + "read skill body: %w", err, + ) + } + raw, err := io.ReadAll(io.LimitReader(reader, maxSkillMetaBytes+1)) + reader.Close() + if err != nil { + return SkillContent{}, xerrors.Errorf( + "read skill body bytes: %w", err, + ) + } + + if int64(len(raw)) > maxSkillMetaBytes { + raw = raw[:maxSkillMetaBytes] + } + + _, _, body, err := parseSkillFrontmatter(string(raw)) + if err != nil { + return SkillContent{}, xerrors.Errorf( + "parse skill frontmatter: %w", err, + ) + } + + // List supporting files so the model knows what it can + // request via read_skill_file. + lsResp, err := conn.LS(ctx, "", workspacesdk.LSRequest{ + Path: []string{skill.Dir}, + Relativity: workspacesdk.LSRelativityRoot, + }) + if err != nil { + return SkillContent{}, xerrors.Errorf( + "list skill directory: %w", err, + ) + } + + var files []string + for _, entry := range lsResp.Contents { + if entry.Name == skillMetaFile { + continue + } + name := entry.Name + if entry.IsDir { + name += "/" + } + files = append(files, name) + } + + return SkillContent{ + SkillMeta: skill, + Body: body, + Files: files, + }, nil +} + +// LoadSkillFile reads a supporting file from a skill's directory. +// The relativePath is validated to prevent directory traversal and +// access to hidden files. +func LoadSkillFile( + ctx context.Context, + conn workspacesdk.AgentConn, + skill SkillMeta, + relativePath string, +) (string, error) { + if err := validateSkillFilePath(relativePath); err != nil { + return "", err + } + + fullPath := path.Join(skill.Dir, relativePath) + + reader, _, err := conn.ReadFile( + ctx, fullPath, 0, maxSkillFileBytes+1, + ) + if err != nil { + return "", xerrors.Errorf( + "read skill file: %w", err, + ) + } + raw, err := io.ReadAll(io.LimitReader(reader, maxSkillFileBytes+1)) + reader.Close() + if err != nil { + return "", xerrors.Errorf( + "read skill file bytes: %w", err, + ) + } + + if int64(len(raw)) > maxSkillFileBytes { + raw = raw[:maxSkillFileBytes] + } + + return string(raw), nil +} + +// validateSkillFilePath rejects paths that could escape the skill +// directory or access hidden files. Only forward-relative, +// non-hidden paths are allowed. +func validateSkillFilePath(p string) error { + if p == "" { + return xerrors.New("path is required") + } + if strings.HasPrefix(p, "/") { + return xerrors.New( + "absolute paths are not allowed", + ) + } + for _, component := range strings.Split(p, "/") { + if component == ".." { + return xerrors.New( + "path traversal is not allowed", + ) + } + if strings.HasPrefix(component, ".") { + return xerrors.New( + "hidden file components are not allowed", + ) + } + } + return nil +} + +// ReadSkillOptions configures the read_skill and read_skill_file +// tools. +type ReadSkillOptions struct { + GetWorkspaceConn func(context.Context) (workspacesdk.AgentConn, error) + GetSkills func() []SkillMeta +} + +// 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."` +} + +// ReadSkill returns an AgentTool that reads the full instructions +// for a skill by name. The model should call this before +// following any skill's instructions. +func ReadSkill(options ReadSkillOptions) fantasy.AgentTool { + return fantasy.NewAgentTool( + "read_skill", + "Read the full instructions for a skill by name. "+ + "Returns the SKILL.md body and a list of "+ + "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 + } + + conn, err := options.GetWorkspaceConn(ctx) + if err != nil { + return fantasy.NewTextErrorResponse( + err.Error(), + ), nil + } + + content, err := LoadSkillBody(ctx, conn, skill) + if err != nil { + return fantasy.NewTextErrorResponse( + err.Error(), + ), nil + } + + return toolResponse(map[string]any{ + "name": content.Name, + "body": content.Body, + "files": content.Files, + }), nil + }, + ) +} + +// ReadSkillFileArgs are the parameters accepted by +// read_skill_file. +type ReadSkillFileArgs struct { + Name string `json:"name" description:"The kebab-case name of the skill."` + Path string `json:"path" description:"Relative path to a file in the skill directory (e.g. roles/security-reviewer.md)."` +} + +// ReadSkillFile returns an AgentTool that reads a supporting file +// from a skill's directory. +func ReadSkillFile(options ReadSkillOptions) fantasy.AgentTool { + return fantasy.NewAgentTool( + "read_skill_file", + "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", + ), nil + } + if args.Path == "" { + return fantasy.NewTextErrorResponse( + "path is required", + ), nil + } + + skill, ok := findSkill(options.GetSkills, args.Name) + if !ok { + return fantasy.NewTextErrorResponse( + fmt.Sprintf("skill %q not found", args.Name), + ), nil + } + + // Validate the path early so we reject bad + // inputs before dialing the workspace agent. + if err := validateSkillFilePath(args.Path); err != nil { + return fantasy.NewTextErrorResponse( + err.Error(), + ), nil + } + + conn, err := options.GetWorkspaceConn(ctx) + if err != nil { + return fantasy.NewTextErrorResponse( + err.Error(), + ), nil + } + + content, err := LoadSkillFile( + ctx, conn, skill, args.Path, + ) + if err != nil { + return fantasy.NewTextErrorResponse( + err.Error(), + ), nil + } + + return toolResponse(map[string]any{ + "content": content, + }), nil + }, + ) +} + +// findSkill looks up a skill by name in the current skill list. +func findSkill( + getSkills func() []SkillMeta, + name string, +) (SkillMeta, bool) { + if getSkills == nil { + return SkillMeta{}, false + } + for _, s := range getSkills() { + if s.Name == name { + return s, true + } + } + return SkillMeta{}, false +} diff --git a/coderd/x/chatd/chattool/skill_test.go b/coderd/x/chatd/chattool/skill_test.go new file mode 100644 index 0000000000..66e57099fe --- /dev/null +++ b/coderd/x/chatd/chattool/skill_test.go @@ -0,0 +1,688 @@ +package chattool_test + +import ( + "context" + "io" + "strings" + "testing" + + "charm.land/fantasy" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/x/chatd/chattool" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/codersdk/workspacesdk" + "github.com/coder/coder/v2/codersdk/workspacesdk/agentconnmock" +) + +// validSkillMD returns a valid SKILL.md with the given name and +// description. +func validSkillMD(name, description string) string { + return "---\nname: " + name + "\ndescription: " + description + "\n---\n\n# Instructions\n\nDo the thing.\n" +} + +func TestDiscoverSkills(t *testing.T) { + t.Parallel() + + t.Run("FindsSkillsInWorkspace", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + // List the skills directory: returns two skill dirs. + conn.EXPECT().LS(gomock.Any(), "", gomock.Any()).DoAndReturn( + func(_ context.Context, _ string, req workspacesdk.LSRequest) (workspacesdk.LSResponse, error) { + require.Equal(t, []string{"/work/.agents/skills"}, req.Path) + return workspacesdk.LSResponse{ + Contents: []workspacesdk.LSFile{ + {Name: "my-skill", IsDir: true, AbsolutePathString: "/work/.agents/skills/my-skill"}, + {Name: "other-skill", IsDir: true, AbsolutePathString: "/work/.agents/skills/other-skill"}, + }, + }, nil + }, + ) + + // Read SKILL.md for my-skill. + conn.EXPECT().ReadFile( + gomock.Any(), + "/work/.agents/skills/my-skill/SKILL.md", + int64(0), + int64(64*1024+1), + ).Return( + io.NopCloser(strings.NewReader(validSkillMD("my-skill", "first skill"))), + "text/markdown", + nil, + ) + + // Read SKILL.md for other-skill. + conn.EXPECT().ReadFile( + gomock.Any(), + "/work/.agents/skills/other-skill/SKILL.md", + int64(0), + int64(64*1024+1), + ).Return( + io.NopCloser(strings.NewReader(validSkillMD("other-skill", "second skill"))), + "text/markdown", + nil, + ) + + skills, err := chattool.DiscoverSkills(context.Background(), conn, "/work") + require.NoError(t, err) + require.Len(t, skills, 2) + assert.Equal(t, "my-skill", skills[0].Name) + assert.Equal(t, "first skill", skills[0].Description) + assert.Equal(t, "other-skill", skills[1].Name) + }) + + t.Run("SkillsDirMissing", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + conn.EXPECT().LS(gomock.Any(), "", gomock.Any()).Return( + workspacesdk.LSResponse{}, + codersdk.NewTestError(404, "POST", "/api/v0/list-directory"), + ) + + skills, err := chattool.DiscoverSkills(context.Background(), conn, "/work") + require.NoError(t, err) + require.Empty(t, skills) + }) + + t.Run("SkipsMissingSKILLmd", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + conn.EXPECT().LS(gomock.Any(), "", gomock.Any()).Return( + workspacesdk.LSResponse{ + Contents: []workspacesdk.LSFile{ + {Name: "broken", IsDir: true, AbsolutePathString: "/work/.agents/skills/broken"}, + }, + }, nil, + ) + + // SKILL.md doesn't exist. + conn.EXPECT().ReadFile( + gomock.Any(), + "/work/.agents/skills/broken/SKILL.md", + int64(0), + int64(64*1024+1), + ).Return( + nil, "", + codersdk.NewTestError(404, "GET", "/api/v0/read-file"), + ) + + skills, err := chattool.DiscoverSkills(context.Background(), conn, "/work") + require.NoError(t, err) + require.Empty(t, skills) + }) + + t.Run("SkipsInvalidFrontmatter", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + conn.EXPECT().LS(gomock.Any(), "", gomock.Any()).Return( + workspacesdk.LSResponse{ + Contents: []workspacesdk.LSFile{ + {Name: "bad", IsDir: true, AbsolutePathString: "/work/.agents/skills/bad"}, + }, + }, nil, + ) + + // No frontmatter delimiters. + conn.EXPECT().ReadFile( + gomock.Any(), gomock.Any(), int64(0), gomock.Any(), + ).Return( + io.NopCloser(strings.NewReader("just some markdown")), + "text/markdown", + nil, + ) + + skills, err := chattool.DiscoverSkills(context.Background(), conn, "/work") + require.NoError(t, err) + require.Empty(t, skills) + }) + + t.Run("SkipsMismatchedDirName", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + conn.EXPECT().LS(gomock.Any(), "", gomock.Any()).Return( + workspacesdk.LSResponse{ + Contents: []workspacesdk.LSFile{ + {Name: "dir-name", IsDir: true, AbsolutePathString: "/work/.agents/skills/dir-name"}, + }, + }, nil, + ) + + // name in frontmatter doesn't match dir name. + conn.EXPECT().ReadFile( + gomock.Any(), gomock.Any(), int64(0), gomock.Any(), + ).Return( + io.NopCloser(strings.NewReader(validSkillMD("different-name", "desc"))), + "text/markdown", + nil, + ) + + skills, err := chattool.DiscoverSkills(context.Background(), conn, "/work") + require.NoError(t, err) + require.Empty(t, skills) + }) + + t.Run("SkipsNonKebabCase", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + conn.EXPECT().LS(gomock.Any(), "", gomock.Any()).Return( + workspacesdk.LSResponse{ + Contents: []workspacesdk.LSFile{ + {Name: "UPPER", IsDir: true, AbsolutePathString: "/work/.agents/skills/UPPER"}, + }, + }, nil, + ) + + conn.EXPECT().ReadFile( + gomock.Any(), gomock.Any(), int64(0), gomock.Any(), + ).Return( + io.NopCloser(strings.NewReader(validSkillMD("UPPER", "bad name"))), + "text/markdown", + nil, + ) + + skills, err := chattool.DiscoverSkills(context.Background(), conn, "/work") + require.NoError(t, err) + require.Empty(t, skills) + }) + + t.Run("SkipsNonDirectories", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + conn.EXPECT().LS(gomock.Any(), "", gomock.Any()).Return( + workspacesdk.LSResponse{ + Contents: []workspacesdk.LSFile{ + {Name: "README.md", IsDir: false, AbsolutePathString: "/work/.agents/skills/README.md"}, + }, + }, nil, + ) + + skills, err := chattool.DiscoverSkills(context.Background(), conn, "/work") + require.NoError(t, err) + require.Empty(t, skills) + }) + + t.Run("QuotedDescription", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + conn.EXPECT().LS(gomock.Any(), "", gomock.Any()).Return( + workspacesdk.LSResponse{ + Contents: []workspacesdk.LSFile{ + {Name: "my-skill", IsDir: true, AbsolutePathString: "/work/.agents/skills/my-skill"}, + }, + }, nil, + ) + + // Description uses YAML-style quotes. + md := "---\nname: my-skill\ndescription: \"A quoted description\"\n---\n\nBody.\n" + conn.EXPECT().ReadFile( + gomock.Any(), gomock.Any(), int64(0), gomock.Any(), + ).Return( + io.NopCloser(strings.NewReader(md)), + "text/markdown", + nil, + ) + + skills, err := chattool.DiscoverSkills(context.Background(), conn, "/work") + require.NoError(t, err) + require.Len(t, skills, 1) + assert.Equal(t, "A quoted description", skills[0].Description) + }) + + t.Run("OversizedSKILLmdTruncated", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + conn.EXPECT().LS(gomock.Any(), "", gomock.Any()).Return( + workspacesdk.LSResponse{ + Contents: []workspacesdk.LSFile{ + {Name: "big-skill", IsDir: true, AbsolutePathString: "/work/.agents/skills/big-skill"}, + }, + }, nil, + ) + + // Build a SKILL.md larger than 64KB. The frontmatter is + // at the start so it survives truncation. + bigBody := strings.Repeat("x", 70*1024) + md := "---\nname: big-skill\ndescription: large\n---\n" + bigBody + conn.EXPECT().ReadFile( + gomock.Any(), gomock.Any(), int64(0), gomock.Any(), + ).Return( + io.NopCloser(strings.NewReader(md)), + "text/markdown", + nil, + ) + + skills, err := chattool.DiscoverSkills(context.Background(), conn, "/work") + require.NoError(t, err) + // The skill should still be discovered since the + // frontmatter fits within the truncation limit. + require.Len(t, skills, 1) + assert.Equal(t, "big-skill", skills[0].Name) + }) + + t.Run("BOMHandled", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + conn.EXPECT().LS(gomock.Any(), "", gomock.Any()).Return( + workspacesdk.LSResponse{ + Contents: []workspacesdk.LSFile{ + {Name: "bom-skill", IsDir: true, AbsolutePathString: "/work/.agents/skills/bom-skill"}, + }, + }, nil, + ) + + // UTF-8 BOM prefix before the frontmatter. + md := "\xef\xbb\xbf---\nname: bom-skill\ndescription: has BOM\n---\n\nBody.\n" + conn.EXPECT().ReadFile( + gomock.Any(), gomock.Any(), int64(0), gomock.Any(), + ).Return( + io.NopCloser(strings.NewReader(md)), + "text/markdown", + nil, + ) + + skills, err := chattool.DiscoverSkills(context.Background(), conn, "/work") + require.NoError(t, err) + require.Len(t, skills, 1) + assert.Equal(t, "bom-skill", skills[0].Name) + }) +} + +func TestFormatSkillIndex(t *testing.T) { + t.Parallel() + + t.Run("Empty", func(t *testing.T) { + t.Parallel() + assert.Empty(t, chattool.FormatSkillIndex(nil)) + }) + + t.Run("RendersIndex", 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") + }) +} + +func TestLoadSkillBody(t *testing.T) { + t.Parallel() + + t.Run("ReturnsBodyAndFiles", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + skill := chattool.SkillMeta{ + Name: "my-skill", + Description: "desc", + Dir: "/work/.agents/skills/my-skill", + } + + // Read the full SKILL.md. + conn.EXPECT().ReadFile( + gomock.Any(), + "/work/.agents/skills/my-skill/SKILL.md", + int64(0), + int64(64*1024+1), + ).Return( + io.NopCloser(strings.NewReader(validSkillMD("my-skill", "desc"))), + "text/markdown", + nil, + ) + + // List supporting files. + conn.EXPECT().LS(gomock.Any(), "", gomock.Any()).Return( + workspacesdk.LSResponse{ + Contents: []workspacesdk.LSFile{ + {Name: "SKILL.md"}, + {Name: "helper.md"}, + {Name: "roles", IsDir: true}, + }, + }, nil, + ) + + content, err := chattool.LoadSkillBody(context.Background(), conn, skill) + require.NoError(t, err) + assert.Contains(t, content.Body, "Do the thing.") + assert.Equal(t, []string{"helper.md", "roles/"}, content.Files) + }) +} + +func TestLoadSkillFile(t *testing.T) { + t.Parallel() + + t.Run("ValidFile", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + skill := chattool.SkillMeta{ + Name: "my-skill", + Dir: "/work/.agents/skills/my-skill", + } + + conn.EXPECT().ReadFile( + gomock.Any(), + "/work/.agents/skills/my-skill/roles/reviewer.md", + int64(0), + int64(512*1024+1), + ).Return( + io.NopCloser(strings.NewReader("review instructions")), + "text/markdown", + nil, + ) + + content, err := chattool.LoadSkillFile( + context.Background(), conn, skill, "roles/reviewer.md", + ) + require.NoError(t, err) + assert.Equal(t, "review instructions", content) + }) + + t.Run("PathTraversalRejected", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + skill := chattool.SkillMeta{ + Name: "my-skill", + Dir: "/work/.agents/skills/my-skill", + } + + _, err := chattool.LoadSkillFile( + context.Background(), conn, skill, "../../etc/passwd", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "traversal") + }) + + t.Run("AbsolutePathRejected", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + skill := chattool.SkillMeta{ + Name: "my-skill", + Dir: "/work/.agents/skills/my-skill", + } + + _, err := chattool.LoadSkillFile( + context.Background(), conn, skill, "/etc/passwd", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "absolute") + }) + + t.Run("HiddenFileRejected", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + skill := chattool.SkillMeta{ + Name: "my-skill", + Dir: "/work/.agents/skills/my-skill", + } + + _, err := chattool.LoadSkillFile( + context.Background(), conn, skill, ".git/config", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "hidden") + }) + + t.Run("EmptyPathRejected", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + skill := chattool.SkillMeta{ + Name: "my-skill", + Dir: "/work/.agents/skills/my-skill", + } + + _, err := chattool.LoadSkillFile( + context.Background(), conn, skill, "", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "required") + }) + + t.Run("OversizedFileTruncated", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + skill := chattool.SkillMeta{ + Name: "my-skill", + Dir: "/work/.agents/skills/my-skill", + } + + // Build a file that exceeds maxSkillFileBytes (512KB). + bigContent := strings.Repeat("x", 512*1024+100) + + conn.EXPECT().ReadFile( + gomock.Any(), + "/work/.agents/skills/my-skill/large.txt", + int64(0), + int64(512*1024+1), + ).Return( + io.NopCloser(strings.NewReader(bigContent)), + "text/plain", + nil, + ) + + content, err := chattool.LoadSkillFile( + context.Background(), conn, skill, "large.txt", + ) + require.NoError(t, err) + assert.Equal(t, 512*1024, len(content), + "content should be truncated to maxSkillFileBytes") + }) +} + +func TestReadSkillTool(t *testing.T) { + t.Parallel() + + t.Run("ValidSkill", 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{ + Contents: []workspacesdk.LSFile{ + {Name: "SKILL.md"}, + }, + }, nil, + ) + + tool := chattool.ReadSkill(chattool.ReadSkillOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return conn, nil + }, + GetSkills: func() []chattool.SkillMeta { return skills }, + }) + + 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, "Do the thing.") + }) + + t.Run("UnknownSkill", func(t *testing.T) { + t.Parallel() + + tool := chattool.ReadSkill(chattool.ReadSkillOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + t.Fatal("unexpected call to GetWorkspaceConn") + return nil, xerrors.New("unreachable") + }, + GetSkills: func() []chattool.SkillMeta { return nil }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "read_skill", + Input: `{"name":"nonexistent"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, "not found") + }) + + t.Run("EmptyName", func(t *testing.T) { + t.Parallel() + + tool := chattool.ReadSkill(chattool.ReadSkillOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + t.Fatal("unexpected call to GetWorkspaceConn") + return nil, xerrors.New("unreachable") + }, + GetSkills: func() []chattool.SkillMeta { return nil }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "read_skill", + Input: `{"name":""}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, "required") + }) +} + +func TestReadSkillFileTool(t *testing.T) { + t.Parallel() + + t.Run("ValidFile", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + conn := agentconnmock.NewMockAgentConn(ctrl) + + skills := []chattool.SkillMeta{{ + Name: "my-skill", + Dir: "/work/.agents/skills/my-skill", + }} + + conn.EXPECT().ReadFile( + gomock.Any(), + "/work/.agents/skills/my-skill/roles/reviewer.md", + int64(0), + int64(512*1024+1), + ).Return( + io.NopCloser(strings.NewReader("reviewer guide")), + "text/markdown", + nil, + ) + + tool := chattool.ReadSkillFile(chattool.ReadSkillOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return conn, nil + }, + GetSkills: func() []chattool.SkillMeta { return skills }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "read_skill_file", + Input: `{"name":"my-skill","path":"roles/reviewer.md"}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) + assert.Contains(t, resp.Content, "reviewer guide") + }) + + t.Run("TraversalRejected", func(t *testing.T) { + t.Parallel() + + skills := []chattool.SkillMeta{{ + Name: "my-skill", + Dir: "/work/.agents/skills/my-skill", + }} + + tool := chattool.ReadSkillFile(chattool.ReadSkillOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + t.Fatal("unexpected call to GetWorkspaceConn") + return nil, xerrors.New("unreachable") + }, + GetSkills: func() []chattool.SkillMeta { return skills }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "read_skill_file", + Input: `{"name":"my-skill","path":"../../etc/passwd"}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Contains(t, resp.Content, "traversal") + }) +}