Merge pull request #2571 from dgageot/board/simplifying-skills-package-without-break-aff91d91

refactor(skills): split package into focused files

Author: dgageot

pkg/skills/frontmatter.go

@@ -0,0 +1,122 @@
+package skills
+
+import "strings"
+
+// parseFrontmatter extracts and parses the YAML-like frontmatter from a
+// markdown file. Instead of using a full YAML parser (which rejects unquoted
+// colons in values), we do simple line-by-line key: value splitting on the
+// first ": ". This is more robust for the simple frontmatter format used by
+// skill files.
+func parseFrontmatter(content string) (Skill, bool) {
+	content = strings.ReplaceAll(content, "\r\n", "\n")
+	content = strings.ReplaceAll(content, "\r", "\n")
+
+	if !strings.HasPrefix(content, "---") {
+		return Skill{}, false
+	}
+
+	endIndex := strings.Index(content[3:], "\n---")
+	if endIndex == -1 {
+		return Skill{}, false
+	}
+
+	block := content[4 : endIndex+3]
+
+	var skill Skill
+	var currentKey string // tracks multi-line keys like "metadata" or "allowed-tools"
+
+	for line := range strings.SplitSeq(block, "\n") {
+		// Indented lines belong to the current multi-line key.
+		if line != "" && (line[0] == ' ' || line[0] == '\t') {
+			parseIndentedLine(&skill, currentKey, strings.TrimSpace(line))
+			continue
+		}
+
+		currentKey = ""
+		key, value, ok := splitKeyValue(line)
+		if !ok {
+			continue
+		}
+
+		if multi := parseTopLevelLine(&skill, key, value); multi {
+			currentKey = key
+		}
+	}
+
+	return skill, true
+}
+
+// parseTopLevelLine assigns a frontmatter key/value to the right Skill field.
+// It returns true if the key opens a multi-line block whose subsequent
+// indented lines must be collected by parseIndentedLine.
+func parseTopLevelLine(skill *Skill, key, value string) bool {
+	switch key {
+	case "name":
+		skill.Name = unquote(value)
+	case "description":
+		skill.Description = unquote(value)
+	case "license":
+		skill.License = unquote(value)
+	case "compatibility":
+		skill.Compatibility = unquote(value)
+	case "context":
+		skill.Context = unquote(value)
+	case "model":
+		skill.Model = unquote(value)
+	case "metadata":
+		// metadata is always multi-line.
+		return true
+	case "allowed-tools":
+		if value == "" {
+			// Block form: subsequent indented "- item" lines.
+			return true
+		}
+		// Inline comma-separated list.
+		for item := range strings.SplitSeq(value, ",") {
+			if t := unquote(strings.TrimSpace(item)); t != "" {
+				skill.AllowedTools = append(skill.AllowedTools, t)
+			}
+		}
+	}
+	return false
+}
+
+// parseIndentedLine accumulates an indented child line into the multi-line
+// block identified by parentKey.
+func parseIndentedLine(skill *Skill, parentKey, line string) {
+	switch parentKey {
+	case "metadata":
+		if k, v, ok := splitKeyValue(line); ok {
+			if skill.Metadata == nil {
+				skill.Metadata = make(map[string]string)
+			}
+			skill.Metadata[k] = unquote(v)
+		}
+	case "allowed-tools":
+		if item, ok := strings.CutPrefix(line, "- "); ok {
+			skill.AllowedTools = append(skill.AllowedTools, unquote(strings.TrimSpace(item)))
+		}
+	}
+}
+
+// splitKeyValue splits a line on the first ": " into key and value.
+// It also handles bare "key:" (no value) for keys that introduce a block.
+func splitKeyValue(line string) (string, string, bool) {
+	if key, value, ok := strings.Cut(line, ": "); ok {
+		return key, value, true
+	}
+	if strings.HasSuffix(line, ":") {
+		return line[:len(line)-1], "", true
+	}
+	return "", "", false
+}
+
+// unquote strips matching surrounding single or double quotes.
+func unquote(s string) string {
+	if len(s) >= 2 {
+		if (s[0] == '"' && s[len(s)-1] == '"') || (s[0] == '\'' && s[len(s)-1] == '\'') {
+			return s[1 : len(s)-1]
+		}
+	}
+	return s
+}

pkg/skills/local.go

@@ -0,0 +1,241 @@
+package skills
+
+import (
+	"cmp"
+	"io/fs"
+	"os"
+	"path/filepath"
+	"slices"
+
+	"github.com/docker/docker-agent/pkg/paths"
+)
+
+const skillFile = "SKILL.md"
+
+// localSearchPath describes one directory to scan for local skills, and
+// whether subdirectories should be walked recursively (Codex/agents format)
+// or only as immediate children of the search root (Claude format).
+type localSearchPath struct {
+	dir       string
+	recursive bool
+}
+
+// localSearchPaths returns every directory to scan for local skills, in
+// load order. Entries appearing later in the list override skills with the
+// same name from earlier entries:
+//
+//  1. Global directories (under $HOME).
+//  2. .claude/skills under cwd (Claude project format, flat).
+//  3. .agents/skills from the git root down to cwd (closest wins).
+func localSearchPaths() []localSearchPath {
+	var searchPaths []localSearchPath
+
+	if home := paths.GetHomeDir(); home != "" {
+		searchPaths = append(searchPaths,
+			localSearchPath{filepath.Join(home, ".codex", "skills"), true},
+			localSearchPath{filepath.Join(home, ".claude", "skills"), false},
+			localSearchPath{filepath.Join(home, ".agents", "skills"), true},
+		)
+	}
+
+	cwd, err := os.Getwd()
+	if err != nil {
+		return searchPaths
+	}
+
+	searchPaths = append(searchPaths, localSearchPath{filepath.Join(cwd, ".claude", "skills"), false})
+	for _, dir := range projectSearchDirs(cwd) {
+		searchPaths = append(searchPaths, localSearchPath{filepath.Join(dir, ".agents", "skills"), false})
+	}
+	return searchPaths
+}
+
+// loadLocalSkillsInto populates skillMap with every local skill, with later
+// search paths overriding earlier ones.
+func loadLocalSkillsInto(skillMap map[string]Skill) {
+	for _, p := range localSearchPaths() {
+		for _, skill := range loadSkillsFromDir(p.dir, p.recursive) {
+			skillMap[skill.Name] = skill
+		}
+	}
+}
+
+// projectSearchDirs returns directories from the enclosing git root down to
+// cwd (inclusive), ordered from root to cwd. If cwd is not inside a git
+// repository, it returns just cwd.
+//
+// The ordering matters: later (deeper) entries override skills from parents,
+// so a project subdirectory can shadow a skill defined at the repo root.
+func projectSearchDirs(cwd string) []string {
+	abs, err := filepath.Abs(cwd)
+	if err != nil {
+		return []string{cwd}
+	}
+
+	// Walk up from cwd, recording each dir, until we either hit a git
+	// marker or run out of parents. This collects findGitRoot's result
+	// and the dir list in a single traversal.
+	var dirs []string
+	var gitRoot string
+	for current := abs; ; {
+		dirs = append(dirs, current)
+		if hasGitMarker(current) {
+			gitRoot = current
+			break
+		}
+		parent := filepath.Dir(current)
+		if parent == current {
+			break
+		}
+		current = parent
+	}
+
+	if gitRoot == "" {
+		return []string{abs}
+	}
+
+	slices.Reverse(dirs)
+	return dirs
+}
+
+// hasGitMarker reports whether dir contains a .git directory or .git file
+// (the latter is used by worktrees and submodules).
+func hasGitMarker(dir string) bool {
+	info, err := os.Stat(filepath.Join(dir, ".git"))
+	if err != nil {
+		return false
+	}
+	return info.IsDir() || info.Mode().IsRegular()
+}
+
+// findGitRoot finds the enclosing git repository root, or returns "" if dir
+// is not inside a git repository. Kept as a thin wrapper for test coverage
+// and callers that don't need the full directory list.
+func findGitRoot(dir string) string {
+	for current := dir; ; {
+		if hasGitMarker(current) {
+			return current
+		}
+		parent := filepath.Dir(current)
+		if parent == current {
+			return ""
+		}
+		current = parent
+	}
+}
+
+// loadSkillsFromDir loads skills from a directory.
+// If recursive is true, it walks all subdirectories looking for SKILL.md.
+// If recursive is false, it only looks at immediate subdirectories.
+func loadSkillsFromDir(dir string, recursive bool) []Skill {
+	if recursive {
+		return loadSkillsRecursive(dir)
+	}
+	return loadSkillsFlat(dir)
+}
+
+// loadSkillsFlat loads skills from immediate subdirectories only (Claude
+// format). Symlinks are explicitly skipped here because flat-mode skills are
+// expected to live as plain directories under the search root.
+func loadSkillsFlat(dir string) []Skill {
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		return nil
+	}
+
+	var skills []Skill
+	for _, entry := range entries {
+		if !entry.IsDir() || isHidden(entry) || isSymlink(entry) {
+			continue
+		}
+		path := filepath.Join(dir, entry.Name(), skillFile)
+		if skill, ok := loadSkillFile(path, entry.Name()); ok {
+			skills = append(skills, skill)
+		}
+	}
+	return skills
+}
+
+// loadSkillsRecursive walks dir for SKILL.md files (Codex format).
+//
+// filepath.WalkDir does not follow symlinks inside the tree (it uses
+// fs.DirEntry which is Lstat-based), so a symlink-to-directory has
+// IsDir() == false and is naturally not entered. The visited map is
+// defensive: it catches the (rare) cases where the same real directory
+// is reachable via two non-symlink paths — e.g. Linux bind mounts, or
+// when the search root itself is a symlink whose target also appears
+// as a sibling deeper in the tree.
+func loadSkillsRecursive(dir string) []Skill {
+	visited := make(map[string]bool)
+	if realDir, err := filepath.EvalSymlinks(dir); err == nil {
+		visited[realDir] = true
+	}
+
+	var skills []Skill
+	_ = filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error {
+		if err != nil {
+			return nil
+		}
+
+		if d.IsDir() {
+			if path == dir {
+				return nil
+			}
+			if isHidden(d) {
+				return fs.SkipDir
+			}
+			// De-duplicate by real path so symlink cycles are skipped.
+			realPath, err := filepath.EvalSymlinks(path)
+			if err == nil {
+				if visited[realPath] {
+					return fs.SkipDir
+				}
+				visited[realPath] = true
+			}
+			return nil
+		}
+
+		if d.Name() != skillFile {
+			return nil
+		}
+		dirName := filepath.Base(filepath.Dir(path))
+		if skill, ok := loadSkillFile(path, dirName); ok {
+			skills = append(skills, skill)
+		}
+		return nil
+	})
+	return skills
+}
+
+// loadSkillFile reads and parses a SKILL.md file. dirName is used as the
+// skill name when the frontmatter does not declare one.
+func loadSkillFile(path, dirName string) (Skill, bool) {
+	content, err := os.ReadFile(path)
+	if err != nil {
+		return Skill{}, false
+	}
+
+	skill, ok := parseFrontmatter(string(content))
+	if !ok {
+		return Skill{}, false
+	}
+
+	skill.Name = cmp.Or(skill.Name, dirName)
+	if skill.Name == "" || skill.Description == "" {
+		return Skill{}, false
+	}
+
+	skill.FilePath = path
+	skill.BaseDir = filepath.Dir(path)
+	skill.Local = true
+	return skill, true
+}
+
+func isHidden(entry fs.DirEntry) bool {
+	name := entry.Name()
+	return name != "" && name[0] == '.'
+}
+
+func isSymlink(entry fs.DirEntry) bool {
+	return entry.Type()&os.ModeSymlink != 0
+}

pkg/skills/remote.go

@@ -10,8 +10,6 @@ import (
 	"path/filepath"
 	"regexp"
 	"strings"
-
-	"github.com/docker/docker-agent/pkg/paths"
 )
 
 // remoteIndex represents the index.json served at /.well-known/skills/index.json
@@ -25,19 +23,11 @@ type remoteSkillEntry struct {
 	Files       []string `json:"files"`
 }
 
-func defaultCache() *diskCache {
-	return newDiskCache(filepath.Join(paths.GetCacheDir(), "skills"))
-}
-
-// loadRemoteSkills fetches skills from a remote URL per the well-known skills discovery spec.
-// It fetches /.well-known/skills/index.json, then prefetches all listed files
-// into a disk cache so the agent can read them without network requests during
-// task execution.
-func loadRemoteSkills(baseURL string) []Skill {
-	return loadRemoteSkillsWithCache(context.Background(), baseURL, defaultCache())
-}
-
-func loadRemoteSkillsWithCache(ctx context.Context, baseURL string, cache *diskCache) []Skill {
+// loadRemoteSkills fetches skills from a remote URL per the well-known skills
+// discovery spec. It fetches /.well-known/skills/index.json, then prefetches
+// all listed files into the given disk cache so the agent can read them
+// without network requests during task execution.
+func loadRemoteSkills(ctx context.Context, baseURL string, cache *diskCache) []Skill {
 	baseURL = strings.TrimRight(baseURL, "/")
 	indexURL := baseURL + "/.well-known/skills/index.json"