Skip to content

refactor(skills): split package into focused files#2571

Merged
dgageot merged 3 commits intodocker:mainfrom
dgageot:board/simplifying-skills-package-without-break-aff91d91
Apr 28, 2026
Merged

refactor(skills): split package into focused files#2571
dgageot merged 3 commits intodocker:mainfrom
dgageot:board/simplifying-skills-package-without-break-aff91d91

Conversation

@dgageot
Copy link
Copy Markdown
Member

@dgageot dgageot commented Apr 28, 2026

Summary

Internal refactor of pkg/skills to make the package easier to read and maintain. No public API or behavior change.

Why

pkg/skills/skills.go had grown to 402 lines doing five different jobs (types, public API, local discovery, frontmatter parsing, helpers). The precedence rules for local skill loading were buried in nested for-loops. A few one-line wrappers (loadRemoteSkillsloadRemoteSkillsWithCache, defaultCache()) added indirection without value.

What changed

Three small commits:

  1. refactor(skills): inline trivial wrappers and use maps.Values

    • Merge loadRemoteSkills wrapper + defaultCache() helper.
    • Merge loadSkillsRecursive + walkSkillsRecursive.
    • Use slices.Collect(maps.Values(...)) for map → slice conversions.
    • Drop redundant os.Stat in loadSkillsFromDir.
  2. refactor(skills): split package into focused files

    • skills.go (402 → 82 lines): public API only — Skill type + Load.
    • local.go (new): filesystem discovery, with a declarative localSearchPaths() table that makes the precedence rules (codex → claude → agents → project) easy to read at a glance. findGitRoot+projectSearchDirs share a single walk-up.
    • frontmatter.go (new): YAML-ish frontmatter parsing, split into parseTopLevelLine and parseIndentedLine.
    • Load now sorts its result by Name for deterministic output.
    • Load avoids the double map → slice → map round-trip via loadLocalSkillsInto.
    • Skill.IsFork uses a value receiver for consistency.
    • isHTTPSource extracted from Load.
  3. fix(skills): tighten review nits from refactor

    • Load: add FilePath tiebreaker to the sort. slices.SortedFunc uses pdqsort which is unstable, so two skills sharing a Name (e.g. local + remote both exposing foo) would have non-deterministic relative order. FilePath is unique → fully deterministic output.
    • Clarify symlink behavior in loadSkillsFlat (skips symlinks explicitly) and loadSkillsRecursive (filepath.WalkDir does not follow symlinks; the visited map is defensive against bind mounts and symlinked roots, not symlinks inside the tree).

File sizes (before → after)

File Before After
skills.go 402 82
local.go 232
frontmatter.go 122
remote.go 153 136

Validation

  • mise lint — clean (golangci-lint + custom cop + go mod tidy --diff)
  • mise test — all packages pass
  • Public API unchanged: Skill, Skill.IsFork, Load, ExpandCommands keep the same signatures
dgageot added 3 commits April 28, 2026 11:27
- Merge loadRemoteSkills wrapper + defaultCache helper

- Merge loadSkillsRecursive + walkSkillsRecursive

- Use slices.Collect(maps.Values(...)) for map -> slice conversions

- Drop redundant os.Stat in loadSkillsFromDir

No public API or behavior change.

Assisted-By: docker-agent
skills.go (402 -> 82 lines) now only holds the public API: the Skill type

and the Load entry point. The implementation is split into focused files:

- local.go: filesystem skill discovery, with a declarative localSearchPaths

  table that makes the precedence rules (codex, claude, agents, project) easy

  to read at a glance. findGitRoot+projectSearchDirs share a single walk-up.

- frontmatter.go: YAML-ish frontmatter parsing, split into parseTopLevelLine

  and parseIndentedLine for clarity.

Other small simplifications:

- Load now sorts its result by name for deterministic output.

- Load avoids the double map -> slice -> map round-trip via loadLocalSkillsInto.

- Skill.IsFork uses a value receiver for consistency with the rest of the API.

- isHTTPSource extracted from Load.

No public API or behavior change; all existing tests pass.

Assisted-By: docker-agent
Two small follow-ups from reviewing the refactor:

- Load: add a FilePath tiebreaker to the sort. slices.SortedFunc uses

  pdqsort which is unstable, so two skills sharing a Name (e.g. a local

  and a remote source both exposing 'foo') would otherwise have

  non-deterministic relative ordering across runs. FilePath is unique,

  so this gives Load a fully deterministic output.

- local.go: clarify the symlink behavior of loadSkillsFlat (skips

  symlinks explicitly) and loadSkillsRecursive (filepath.WalkDir does

  not follow symlinks; the visited map is defensive against bind

  mounts and symlinked roots, not symlinks inside the tree).

Assisted-By: docker-agent
@dgageot dgageot requested a review from a team as a code owner April 28, 2026 11:25
@dgageot dgageot merged commit cf40a5c into docker:main Apr 28, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants