From f286fb130bf83a07c8bc25516b5ee410db827f7c Mon Sep 17 00:00:00 2001 From: devrimcavusoglu Date: Wed, 19 Aug 2026 15:07:23 +0300 Subject: [PATCH 1/3] Add install.exclude frontmatter to keep companion files out of platform copies (#103) skill install copied the whole skill tree, so eval corpora and fixtures landed in every agent's context. Model an author-owned `install.exclude` list of path.Match globs on Skill/frontmatter; match each relative path and its leading directories in skill.MatchExclude; thread the patterns through a new platform.InstallOptions argument on Platform.Install so copyDir prunes matched files and directories. The registry always keeps the full directory. skill validate errors on malformed, absolute, or `..` patterns and anything matching SKILL.md, and warns when a pattern matches nothing or excludes a body-referenced file. Closes #103 Co-Authored-By: Claude Fable 5 --- AGENTS.md | 18 ++++- CHANGELOG.md | 12 ++++ docs/concepts/platform-adapters.md | 4 +- docs/concepts/skill-format.md | 23 ++++++- docs/reference/commands.md | 2 + docs/reference/validation.md | 8 +++ internal/cli/skill_install.go | 5 +- internal/cli/skill_test.go | 104 +++++++++++++++++++++++++++++ internal/platform/adapter.go | 4 +- internal/platform/helpers.go | 24 +++++-- internal/platform/platform.go | 14 +++- internal/platform/platform_test.go | 88 +++++++++++++++++++++--- internal/skill/folder.go | 84 +++++++++++++++++++++++ internal/skill/folder_test.go | 76 +++++++++++++++++++++ internal/skill/manifest.go | 3 + internal/skill/skill.go | 15 +++++ internal/skill/validator.go | 43 ++++++++++++ internal/skill/validator_test.go | 67 +++++++++++++++++++ 18 files changed, 568 insertions(+), 26 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index dc00d3f..daacbbd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -76,9 +76,10 @@ test isolation. Every platform is one row in `Specs` (`internal/platform/spec.go`) driving a single generic `Adapter` — adding a platform means a `Spec` row plus a `Type` constant, never a per-platform Go file. Installing is a directory - copy *today*, but pure-copy is **not** an invariant: a `Spec` may grow - declarative post-install behavior (companion files, per-platform - destination overrides) where a platform's loader demands it. Any such + copy filtered by the skill's own `install.exclude` ([#103]) *today*, but + pure-copy is **not** an invariant: a `Spec` may grow declarative + post-install behavior (companion files, per-platform destination + overrides) where a platform's loader demands it. Any such behavior belongs in the spec table as data, not as a hand-written per-platform code path, and `Uninstall` must reverse whatever `Install` produced. Open work depending on this: [#99], [#101], [#47]. @@ -155,6 +156,8 @@ metadata: type: agent platform: codex-cli date: "2025-07-15T10:30:00Z" +install: # optional; shapes the platform copy only + exclude: [eval, fixtures/*] # kept in the registry, not installed --- ## Overview @@ -199,6 +202,15 @@ hand-edited tags still match. `skill list`, `skill install`, and semantics for all three; on install/uninstall the filter is mutually exclusive with positional names. +**`install.exclude`** ([#103]) lists `path.Match` globs, relative to the skill +directory, that `skill install` leaves out of the platform copy (the registry +keeps everything). A pattern matches a path or any leading directory of it, +so a bare directory name excludes its subtree; `SKILL.md` is never excluded. +Matching lives in `skill.MatchExclude` (`internal/skill/folder.go`) and is +applied by `platform.copyDir` via `InstallOptions.Exclude`; `skill validate` +errors on malformed patterns and warns on no-match / excluded-but-referenced +files. + **Unmodeled keys pass through.** The fields above are the keys skern models; any other key — top-level, `metadata.*`, or nested in `metadata.author` / a `modified-by` entry — lands in the matching `Extra` inline map diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a390fb..2c6e581 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 swallowed). `uninstall --tag` narrows the group to what is actually installed on the platform and skips the rest. `--enforce-budget` counts the resolved group. ([#102]) +- **`install.exclude` frontmatter — keep companion files out of platform + copies.** A skill can now list `path.Match` globs (relative to its + directory) under `install.exclude` for assets that belong in the + registry but not in an agent's context — evaluation corpora, fixtures, + scratch. `skern skill install` skips them; the registry keeps everything. + A bare directory name excludes its subtree (`eval`), `fixtures/*` its + children, `*.draft.md` top-level files by suffix; `SKILL.md` can never be + excluded. `skill validate` errors on malformed, absolute, or `..` patterns + and warns when a pattern matches nothing or excludes a file the body + references. Platform adapters take an `InstallOptions` argument on + `Install`. ([#103]) ### Changed @@ -91,6 +102,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#98]: https://github.com/devrimcavusoglu/skern/pull/98 [#100]: https://github.com/devrimcavusoglu/skern/issues/100 [#102]: https://github.com/devrimcavusoglu/skern/issues/102 +[#103]: https://github.com/devrimcavusoglu/skern/issues/103 [#104]: https://github.com/devrimcavusoglu/skern/issues/104 ## [v0.3.1] — 2026-05-13 diff --git a/docs/concepts/platform-adapters.md b/docs/concepts/platform-adapters.md index fdccc84..b7caf90 100644 --- a/docs/concepts/platform-adapters.md +++ b/docs/concepts/platform-adapters.md @@ -6,9 +6,9 @@ Platform adapters bridge the skill registry with agent runtimes. Each adapter kn When you run `skern skill install`, the adapter: -1. Reads the `SKILL.md` from the skern registry +1. Reads the skill from the skern registry (its `SKILL.md` and companion files) 2. Creates the platform-specific skill directory -3. Copies the `SKILL.md` into the target location +3. Copies the skill directory into the target location — minus any paths the skill's frontmatter lists under [`install.exclude`](/concepts/skill-format#install-time-exclusions) (eval corpora, fixtures, scratch), which stay in the registry only 4. The agent runtime discovers the skill on its next invocation ## Supported Platforms diff --git a/docs/concepts/skill-format.md b/docs/concepts/skill-format.md index 681941d..d5c43b5 100644 --- a/docs/concepts/skill-format.md +++ b/docs/concepts/skill-format.md @@ -55,6 +55,7 @@ The main technique or pattern (before/after for techniques). | `metadata.author.platform` | No | Platform name (e.g. `claude-code`) — used when `type: agent` | | `metadata.version` | No | Semantic version (e.g. `1.0.0`); defaults to `0.0.1` on `skill create` | | `metadata.modified-by` | No | Append-only modification history (set via `skern skill edit --modified-by`) | +| `install.exclude` | No | Glob patterns (relative to the skill directory) for files and directories that stay in the registry but are **not** copied on `skern skill install`. See [Install-time exclusions](#install-time-exclusions). | `name` and `description` are the only hard requirements. The rest help discovery, validation, and provenance. @@ -98,10 +99,30 @@ my-skill/ └── template.json ``` -When a skill is installed to a platform, the **entire directory** is copied. The `scripts/` directory is language-agnostic — skills can include Python, shell, JavaScript, or any other scripts. The agent decides which language is appropriate. +When a skill is installed to a platform, the directory is copied — minus anything the skill's `install.exclude` rules out (below). The `scripts/` directory is language-agnostic — skills can include Python, shell, JavaScript, or any other scripts. The agent decides which language is appropriate. `skern skill validate ` checks that files referenced in the body (via backticks or markdown links) actually exist in the skill directory. Missing references produce **warnings**, not errors. `skern skill show ` lists every file bundled with the skill. +### Install-time exclusions + +A skill directory often carries assets that belong in the registry but not in a platform's skill directory — evaluation corpora, fixtures, development scratch. Because host agents load or index everything under an installed skill, that material is dead weight in the agent's context on every project. The skill author lists what to leave behind in frontmatter: + +```yaml +install: + exclude: + - eval # a directory: excludes eval/ and everything beneath it + - fixtures/* # direct children of fixtures/ (and their subtrees) + - "*.draft.md" # top-level files by suffix (globs do not cross "/") +``` + +Rules: + +- Patterns are slash-separated paths relative to the skill directory, using Go `path.Match` syntax (`*`, `?`, `[…]`). There is no `**`; a pattern matches a path when it matches the full relative path **or any leading directory of it**, which is what makes a bare directory name exclude its whole subtree. A trailing `/` or leading `./` is ignored. +- `SKILL.md` can never be excluded; absolute paths, `..` segments, and malformed globs are validation **errors**. +- The registry always keeps the full directory — `skill create --from-template`, `skill import`, and `skill show --files` are unaffected. Only the copy made by `skern skill install` is trimmed. +- `skern skill validate` **warns** when a pattern matches no file in the skill directory, and when a file the body references (`` `references/guide.md` ``) is excluded — it would exist in the registry but be missing from every installed copy. +- `skern skill diff` compares manifests (frontmatter and body), not file trees, so excluded files never show up as drift. + ## Creating Skills Use `skern skill create` to scaffold a new skill: diff --git a/docs/reference/commands.md b/docs/reference/commands.md index b081f67..d969985 100644 --- a/docs/reference/commands.md +++ b/docs/reference/commands.md @@ -310,6 +310,8 @@ Instead of names, select a group with the same filters [`skill list`](#skern-ski The response includes a top-level `capacity` block reporting the platform's installed-skill count after the operation, the threshold for that scope, and remaining headroom. +The whole skill directory is copied except paths the skill's frontmatter lists under `install.exclude` — see [Skill Format › Install-time exclusions](/concepts/skill-format#install-time-exclusions). The registry copy is never trimmed. + **Flags:** | Flag | Default | Description | diff --git a/docs/reference/validation.md b/docs/reference/validation.md index 9d9546f..2fc103f 100644 --- a/docs/reference/validation.md +++ b/docs/reference/validation.md @@ -42,6 +42,10 @@ If `allowed-tools` is set, no entry may be empty. `metadata.version` must be a valid semver string (`MAJOR.MINOR.PATCH`). +### Install Exclusions + +Every `install.exclude` entry must be a non-empty, relative, well-formed glob (`path.Match` syntax) with no `..` segment, and must not match `SKILL.md`. See [Skill Format › Install-time exclusions](/concepts/skill-format#install-time-exclusions). + ## Warnings ### Folder Integrity @@ -50,6 +54,10 @@ When the body references files (backtick-enclosed paths like `` `scripts/run.py` Inline code spans and URLs are excluded from this check (no false positives from `` `--flag` `` or `https://…`). +### Install Exclusions + +Two warnings relate to `install.exclude`: a pattern that matches no file in the skill directory (usually a typo, or stale after a rename), and a body-referenced file that an exclude pattern would drop — it exists in the registry but would be missing from every installed copy. + ## Hints Stylistic suggestions adapted from [writing-skills](/writing-skills). They never block creation or validation. diff --git a/internal/cli/skill_install.go b/internal/cli/skill_install.go index 4af1a7b..c90a19e 100644 --- a/internal/cli/skill_install.go +++ b/internal/cli/skill_install.go @@ -98,7 +98,7 @@ installed-skill count would meet or exceed the per-platform threshold (see for _, name := range names { entry := output.SkillActionEntry{Skill: name} - _, skillDir, getErr := reg.Get(name, scopeVal) + s, skillDir, getErr := reg.Get(name, scopeVal) if getErr != nil { entry.Error = fmt.Sprintf("not registered in skern (%s scope) — run 'skern skill create' or 'skern skill import' first, or 'skern skill list' to see available skills", scope) entries = append(entries, entry) @@ -110,7 +110,8 @@ installed-skill count would meet or exceed the per-platform threshold (see _ = p.Uninstall(name, scopeVal) } - if installErr := p.Install(skillDir, name, scopeVal); installErr != nil { + opts := platform.InstallOptions{Exclude: s.Install.Exclude} + if installErr := p.Install(skillDir, name, scopeVal, opts); installErr != nil { entry.Error = installErr.Error() } else { entry.Success = true diff --git a/internal/cli/skill_test.go b/internal/cli/skill_test.go index 10f1505..7159696 100644 --- a/internal/cli/skill_test.go +++ b/internal/cli/skill_test.go @@ -1495,3 +1495,107 @@ Body. assert.Equal(t, "needs git", parsed.Extra["compatibility"]) assert.Equal(t, []any{"plan", "review"}, parsed.Metadata.Extra["phases"]) } + +// #103: a skill's install.exclude keeps companion directories in the +// registry but out of every platform copy; the block itself round-trips +// through create --from-template, and validate flags bad patterns. +func TestSkillInstall_HonorsInstallExclude(t *testing.T) { + cc, _, projectReg := testRegistryWithDirs(t) + home := t.TempDir() + project := t.TempDir() + withTestDetector(t, cc, home, project) + + srcDir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(srcDir, "SKILL.md"), []byte(`--- +name: demo.skill +description: Use when checking install exclusions. +install: + exclude: + - eval + - "*.draft.md" +metadata: + version: "0.1.0" +--- + +## Overview + +See `+"`references/guide.md`"+`. +`), 0o644)) + for _, rel := range []string{"eval/s1.md", "eval/nested/s2.md", "references/guide.md", "notes.draft.md"} { + p := filepath.Join(srcDir, filepath.FromSlash(rel)) + require.NoError(t, os.MkdirAll(filepath.Dir(p), 0o755)) + require.NoError(t, os.WriteFile(p, []byte("x"), 0o644)) + } + + _, err := runCmd(t, cc, "skill", "create", "demo.skill", "--from-template", srcDir, "--scope", "project") + require.NoError(t, err) + + // The install block survives the registry write (needs #100's modeling). + regManifest := filepath.Join(projectReg, "demo.skill", "SKILL.md") + parsed := requireParsedManifest(t, regManifest) + assert.Equal(t, []string{"eval", "*.draft.md"}, parsed.Install.Exclude) + raw, err := os.ReadFile(regManifest) + require.NoError(t, err) + assert.Contains(t, string(raw), "install:\n exclude:\n - eval\n - '*.draft.md'\n") + // Registry keeps everything. + for _, rel := range []string{"eval/s1.md", "eval/nested/s2.md", "notes.draft.md"} { + _, err := os.Stat(filepath.Join(projectReg, "demo.skill", filepath.FromSlash(rel))) + require.NoError(t, err, "registry must keep %s", rel) + } + + _, err = runCmd(t, cc, "skill", "install", "demo.skill", "--platform", "claude-code", "--scope", "project") + require.NoError(t, err) + + dest := filepath.Join(project, ".claude", "skills", "demo.skill") + for _, want := range []string{"SKILL.md", "references/guide.md"} { + _, err := os.Stat(filepath.Join(dest, filepath.FromSlash(want))) + assert.NoError(t, err, "%s should be installed", want) + } + for _, gone := range []string{"eval", "eval/s1.md", "eval/nested/s2.md", "notes.draft.md"} { + _, err := os.Stat(filepath.Join(dest, filepath.FromSlash(gone))) + assert.True(t, os.IsNotExist(err), "%s should be excluded from the platform copy", gone) + } + + // diff compares manifests (not trees), so the installed copy is identical. + out, err := runCmd(t, cc, "skill", "diff", "demo.skill", "--platform", "claude-code", "--scope", "project", "--json") + require.NoError(t, err) + var diff output.SkillDiffResult + require.NoError(t, json.Unmarshal([]byte(out), &diff)) + assert.True(t, diff.Identical) + + // validate: clean skill passes; the two patterns match files so no warning. + out, err = runCmd(t, cc, "skill", "validate", "demo.skill", "--scope", "project") + require.NoError(t, err) + assert.NotContains(t, out, "install.exclude") +} + +func TestSkillValidate_InstallExcludeErrors(t *testing.T) { + cc, _, projectReg := testRegistryWithDirs(t) + dir := filepath.Join(projectReg, "bad-exclude") + require.NoError(t, os.MkdirAll(filepath.Join(dir, "eval"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "eval", "s1.md"), []byte("x"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(`--- +name: bad-exclude +description: Use when testing exclude validation. +install: + exclude: ["../escape", "SKILL.md", "eval", "ghost"] +metadata: + author: + name: a + type: human + version: "0.1.0" +--- + +## Overview + +Body text long enough to avoid lint noise about brevity in this validation test. +`), 0o644)) + + out, err := runCmd(t, cc, "skill", "validate", "bad-exclude", "--scope", "project", "--json") + require.Error(t, err, "errors in install.exclude must fail validation") + assert.Contains(t, out, `must not contain \"..\"`) + assert.Contains(t, out, "must always be installed") + assert.Contains(t, out, `exclude pattern \"ghost\" matches no file`) + // "eval" is valid and matches: neither error nor warning mentions it. + assert.NotContains(t, out, `\"eval\"`) +} diff --git a/internal/platform/adapter.go b/internal/platform/adapter.go index 872db85..afb3d30 100644 --- a/internal/platform/adapter.go +++ b/internal/platform/adapter.go @@ -58,8 +58,8 @@ func (a *Adapter) ProjectSkillsDir() string { } // Install implements Platform. -func (a *Adapter) Install(skillDir, skillName string, scope skill.Scope) error { - return installSkill(skillDir, skillName, a.skillsDir(scope)) +func (a *Adapter) Install(skillDir, skillName string, scope skill.Scope, opts InstallOptions) error { + return installSkill(skillDir, skillName, a.skillsDir(scope), opts) } // Uninstall implements Platform. diff --git a/internal/platform/helpers.go b/internal/platform/helpers.go index 3c283dd..283225a 100644 --- a/internal/platform/helpers.go +++ b/internal/platform/helpers.go @@ -5,13 +5,16 @@ import ( "io" "os" "path/filepath" + + "github.com/devrimcavusoglu/skern/internal/skill" ) const manifestFile = "SKILL.md" -// installSkill copies a skill directory into the target base directory. -// It returns an error if the skill already exists at the destination. -func installSkill(sourceDir, skillName, targetBaseDir string) error { +// installSkill copies a skill directory into the target base directory, +// skipping paths matched by opts.Exclude. It returns an error if the skill +// already exists at the destination. +func installSkill(sourceDir, skillName, targetBaseDir string, opts InstallOptions) error { destDir := filepath.Join(targetBaseDir, skillName) if _, err := os.Stat(destDir); err == nil { @@ -22,7 +25,8 @@ func installSkill(sourceDir, skillName, targetBaseDir string) error { return fmt.Errorf("creating skills directory: %w", err) } - if err := copyDir(sourceDir, destDir); err != nil { + skip := func(rel string) bool { return skill.MatchExclude(opts.Exclude, rel) } + if err := copyDir(sourceDir, destDir, skip); err != nil { // Clean up on failure _ = os.RemoveAll(destDir) return fmt.Errorf("copying skill: %w", err) @@ -67,8 +71,10 @@ func listInstalledSkills(targetBaseDir string) ([]string, error) { return names, nil } -// copyDir recursively copies a directory tree from src to dst. -func copyDir(src, dst string) error { +// copyDir recursively copies a directory tree from src to dst. Entries whose +// src-relative path satisfies skip (nil means "copy everything") are not +// copied; a skipped directory is pruned from the walk entirely. +func copyDir(src, dst string, skip func(rel string) bool) error { return filepath.WalkDir(src, func(path string, d os.DirEntry, err error) error { if err != nil { return err @@ -78,6 +84,12 @@ func copyDir(src, dst string) error { if err != nil { return err } + if rel != "." && skip != nil && skip(rel) { + if d.IsDir() { + return filepath.SkipDir + } + return nil + } target := filepath.Join(dst, rel) if d.IsDir() { diff --git a/internal/platform/platform.go b/internal/platform/platform.go index 2e48eaf..eba0cbd 100644 --- a/internal/platform/platform.go +++ b/internal/platform/platform.go @@ -21,6 +21,15 @@ const ( TypeContinue Type = "continue" ) +// InstallOptions shapes how a skill is copied onto a platform. The zero +// value copies the whole skill directory. +type InstallOptions struct { + // Exclude holds the skill's `install.exclude` glob patterns; matching + // files and directories stay in the registry but are not copied. See + // skill.MatchExclude for the rules. + Exclude []string +} + // Platform defines the interface that each platform adapter must implement. type Platform interface { // Name returns the platform type identifier. @@ -31,8 +40,9 @@ type Platform interface { UserSkillsDir() string // ProjectSkillsDir returns the absolute path to the project-level skills directory. ProjectSkillsDir() string - // Install copies a skill from the registry into the platform's skills directory. - Install(skillDir string, skillName string, scope skill.Scope) error + // Install copies a skill from the registry into the platform's skills + // directory, honoring opts.Exclude. + Install(skillDir string, skillName string, scope skill.Scope, opts InstallOptions) error // Uninstall removes a skill from the platform's skills directory. Uninstall(skillName string, scope skill.Scope) error // InstalledSkills returns the names of skills installed for the given scope. diff --git a/internal/platform/platform_test.go b/internal/platform/platform_test.go index a12b7a2..72fe7ca 100644 --- a/internal/platform/platform_test.go +++ b/internal/platform/platform_test.go @@ -136,7 +136,7 @@ func TestAdapter_RoundTrip(t *testing.T) { a := New(s.Name, home, project) // User scope - require.NoError(t, a.Install(skillDir, "round-trip", skill.ScopeUser)) + require.NoError(t, a.Install(skillDir, "round-trip", skill.ScopeUser, InstallOptions{})) userInstalled := filepath.Join(home, s.UserDir, "round-trip", "SKILL.md") _, err := os.Stat(userInstalled) require.NoError(t, err, "expected SKILL.md at %s", userInstalled) @@ -150,7 +150,7 @@ func TestAdapter_RoundTrip(t *testing.T) { assert.True(t, os.IsNotExist(err)) // Project scope - require.NoError(t, a.Install(skillDir, "round-trip", skill.ScopeProject)) + require.NoError(t, a.Install(skillDir, "round-trip", skill.ScopeProject, InstallOptions{})) projectInstalled := filepath.Join(project, s.ProjectDir, "round-trip", "SKILL.md") _, err = os.Stat(projectInstalled) require.NoError(t, err, "expected SKILL.md at %s", projectInstalled) @@ -166,9 +166,9 @@ func TestAdapter_Install_Duplicate(t *testing.T) { skillDir := createSkillDir(t, registry, "dup") a := New(TypeClaudeCode, home, t.TempDir()) - require.NoError(t, a.Install(skillDir, "dup", skill.ScopeUser)) + require.NoError(t, a.Install(skillDir, "dup", skill.ScopeUser, InstallOptions{})) - err := a.Install(skillDir, "dup", skill.ScopeUser) + err := a.Install(skillDir, "dup", skill.ScopeUser, InstallOptions{}) require.Error(t, err) assert.Contains(t, err.Error(), "already installed") } @@ -202,7 +202,7 @@ func TestAdapter_SharedProjectDir(t *testing.T) { require.Equal(t, cursor.ProjectSkillsDir(), gemini.ProjectSkillsDir(), "sanity: cursor and gemini-cli should share .agents/skills/") - require.NoError(t, cursor.Install(skillDir, "shared", skill.ScopeProject)) + require.NoError(t, cursor.Install(skillDir, "shared", skill.ScopeProject, InstallOptions{})) // Both adapters report the skill because they read the same directory. cursorList, err := cursor.InstalledSkills(skill.ScopeProject) @@ -322,10 +322,10 @@ func TestFullLifecycle(t *testing.T) { codex := New(TypeCodexCLI, home, project) // Install to Claude Code - require.NoError(t, claude.Install(skillDir, "lifecycle-skill", skill.ScopeUser)) + require.NoError(t, claude.Install(skillDir, "lifecycle-skill", skill.ScopeUser, InstallOptions{})) // Install to Codex CLI - require.NoError(t, codex.Install(skillDir, "lifecycle-skill", skill.ScopeUser)) + require.NoError(t, codex.Install(skillDir, "lifecycle-skill", skill.ScopeUser, InstallOptions{})) // List installed on both claudeSkills, err := claude.InstalledSkills(skill.ScopeUser) @@ -367,7 +367,7 @@ func TestCopyDir(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(src, "file1.txt"), []byte("hello"), 0o644)) require.NoError(t, os.WriteFile(filepath.Join(src, "sub", "file2.txt"), []byte("world"), 0o644)) - require.NoError(t, copyDir(src, dst)) + require.NoError(t, copyDir(src, dst, nil)) // Verify structure data1, err := os.ReadFile(filepath.Join(dst, "file1.txt")) @@ -397,3 +397,75 @@ func TestListInstalledSkills_SkipsNonSkillDirs(t *testing.T) { require.NoError(t, err) assert.Equal(t, []string{"valid-skill"}, names) } + +// #103: install honors InstallOptions.Exclude — matched files and whole +// directories stay out of the platform copy while the source is untouched. +func TestAdapter_Install_Exclude(t *testing.T) { + home := t.TempDir() + registry := t.TempDir() + skillDir := createSkillDir(t, registry, "lean") + mk := func(rel string) { + p := filepath.Join(skillDir, filepath.FromSlash(rel)) + require.NoError(t, os.MkdirAll(filepath.Dir(p), 0o755)) + require.NoError(t, os.WriteFile(p, []byte("x"), 0o644)) + } + mk("eval/s1.md") + mk("eval/deep/s2.md") + mk("fixtures/a.json") + mk("fixtures/keep/b.json") + mk("references/notes.md") + mk("scratch.test.md") + + a := New(TypeClaudeCode, home, t.TempDir()) + opts := InstallOptions{Exclude: []string{"eval", "fixtures/*", "*.test.md"}} + require.NoError(t, a.Install(skillDir, "lean", skill.ScopeUser, opts)) + + dest := filepath.Join(home, ".claude", "skills", "lean") + for _, want := range []string{"SKILL.md", "references/notes.md"} { + _, err := os.Stat(filepath.Join(dest, filepath.FromSlash(want))) + assert.NoError(t, err, "%s should be installed", want) + } + for _, gone := range []string{"eval", "eval/s1.md", "eval/deep/s2.md", "fixtures/a.json", "fixtures/keep", "scratch.test.md"} { + _, err := os.Stat(filepath.Join(dest, filepath.FromSlash(gone))) + assert.True(t, os.IsNotExist(err), "%s should be excluded", gone) + } + // fixtures/* prunes children but the (now empty) directory itself is not matched. + _, err := os.Stat(filepath.Join(dest, "fixtures")) + assert.NoError(t, err) + + // Source untouched. + for _, src := range []string{"eval/s1.md", "fixtures/a.json", "scratch.test.md"} { + _, err := os.Stat(filepath.Join(skillDir, filepath.FromSlash(src))) + require.NoError(t, err) + } + + // Zero options still copies everything. + b := New(TypeCodexCLI, home, t.TempDir()) + require.NoError(t, b.Install(skillDir, "lean", skill.ScopeUser, InstallOptions{})) + _, err = os.Stat(filepath.Join(home, ".agents", "skills", "lean", "eval", "s1.md")) + assert.NoError(t, err) +} + +func TestCopyDir_SkipFuncPrunesDirectories(t *testing.T) { + src := t.TempDir() + dst := filepath.Join(t.TempDir(), "out") + require.NoError(t, os.MkdirAll(filepath.Join(src, "keep"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(src, "drop", "nested"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(src, "keep", "a.txt"), []byte("a"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(src, "drop", "nested", "b.txt"), []byte("b"), 0o644)) + + var visited []string + skip := func(rel string) bool { + visited = append(visited, filepath.ToSlash(rel)) + return filepath.ToSlash(rel) == "drop" + } + require.NoError(t, copyDir(src, dst, skip)) + + _, err := os.Stat(filepath.Join(dst, "keep", "a.txt")) + require.NoError(t, err) + _, err = os.Stat(filepath.Join(dst, "drop")) + assert.True(t, os.IsNotExist(err)) + // The skipped directory's children were never visited (SkipDir pruned them). + assert.NotContains(t, visited, "drop/nested") + assert.NotContains(t, visited, "drop/nested/b.txt") +} diff --git a/internal/skill/folder.go b/internal/skill/folder.go index b0cc197..5ce5a1a 100644 --- a/internal/skill/folder.go +++ b/internal/skill/folder.go @@ -1,7 +1,9 @@ package skill import ( + "fmt" "io/fs" + "path" "path/filepath" "regexp" "strings" @@ -94,3 +96,85 @@ func ExtractFileReferences(body string) []string { return refs } + +// ValidateExcludePattern checks one `install.exclude` entry. Patterns are +// slash-separated paths relative to the skill directory using path.Match +// syntax (`*`, `?`, `[...]`; no `**`). Rejected: empty, absolute, any `..` +// segment, a malformed glob, and anything that would match SKILL.md itself. +func ValidateExcludePattern(pattern string) error { + p := normalizeExcludePattern(pattern) + if p == "" { + return fmt.Errorf("exclude pattern must not be empty") + } + if strings.HasPrefix(p, "/") || filepath.IsAbs(pattern) || strings.HasPrefix(pattern, "\\") { + return fmt.Errorf("exclude pattern %q must be relative to the skill directory", pattern) + } + for _, seg := range strings.Split(p, "/") { + if seg == ".." { + return fmt.Errorf("exclude pattern %q must not contain \"..\"", pattern) + } + } + if _, err := path.Match(p, "x"); err != nil { + return fmt.Errorf("exclude pattern %q is not a valid glob: %v", pattern, err) + } + if ok, _ := path.Match(p, ManifestFile); ok { + return fmt.Errorf("exclude pattern %q would exclude %s, which must always be installed", pattern, ManifestFile) + } + return nil +} + +// normalizeExcludePattern trims whitespace, converts backslashes to slashes, +// and strips a leading "./" and any trailing "/" (a directory pattern +// written as "eval/" means the same as "eval"). +func normalizeExcludePattern(pattern string) string { + p := strings.TrimSpace(pattern) + p = strings.ReplaceAll(p, "\\", "/") + p = strings.TrimPrefix(p, "./") + p = strings.TrimRight(p, "/") + return p +} + +// MatchExclude reports whether rel — a slash-separated path relative to the +// skill directory — is excluded by any of patterns. A pattern matches a path +// when path.Match matches the full relative path or any of its leading +// directory prefixes, so `eval` excludes `eval/` and everything beneath it, +// `fixtures/*` excludes the direct children of fixtures/, and `*.test.md` +// excludes top-level files with that suffix. SKILL.md is never excluded. +func MatchExclude(patterns []string, rel string) bool { + rel = filepath.ToSlash(rel) + if rel == "" || rel == "." || rel == ManifestFile { + return false + } + for _, raw := range patterns { + p := normalizeExcludePattern(raw) + if p == "" { + continue + } + // Walk the path prefixes: "a/b/c" -> "a", "a/b", "a/b/c". + for i := 0; i <= len(rel); i++ { + if i == len(rel) || rel[i] == '/' { + if ok, _ := path.Match(p, rel[:i]); ok { + return true + } + } + } + } + return false +} + +// ExcludedFiles returns the relative paths under skillDir (excluding +// SKILL.md) that MatchExclude would drop for patterns, in walk order. Used by +// the validator to warn about patterns that match nothing. +func ExcludedFiles(skillDir string, patterns []string) ([]string, error) { + files, err := ListFiles(skillDir) + if err != nil { + return nil, err + } + var out []string + for _, f := range files { + if MatchExclude(patterns, f) { + out = append(out, f) + } + } + return out, nil +} diff --git a/internal/skill/folder_test.go b/internal/skill/folder_test.go index 28ab82a..ed1f448 100644 --- a/internal/skill/folder_test.go +++ b/internal/skill/folder_test.go @@ -244,3 +244,79 @@ func TestValidateFolder(t *testing.T) { }) } } + +func TestMatchExclude(t *testing.T) { + cases := []struct { + name string + patterns []string + rel string + want bool + }{ + {"dir name excludes subtree", []string{"eval"}, "eval/s1.md", true}, + {"dir name excludes nested subtree", []string{"eval"}, "eval/deep/s1.md", true}, + {"dir name excludes the dir itself", []string{"eval"}, "eval", true}, + {"trailing slash is equivalent", []string{"eval/"}, "eval/s1.md", true}, + {"leading ./ is equivalent", []string{"./eval"}, "eval/s1.md", true}, + {"no partial name match", []string{"eval"}, "evaluation/notes.md", false}, + {"children glob", []string{"fixtures/*"}, "fixtures/a.json", true}, + {"children glob prunes nested too", []string{"fixtures/*"}, "fixtures/sub/a.json", true}, + {"children glob does not match the dir", []string{"fixtures/*"}, "fixtures", false}, + {"suffix glob at top level", []string{"*.test.md"}, "a.test.md", true}, + {"suffix glob does not cross slashes", []string{"*.test.md"}, "sub/a.test.md", false}, + {"nested suffix glob", []string{"sub/*.test.md"}, "sub/a.test.md", true}, + {"SKILL.md never excluded", []string{"*"}, "SKILL.md", false}, + {"star excludes everything else", []string{"*"}, "notes.md", true}, + {"multiple patterns, any matches", []string{"a", "b"}, "b/x", true}, + {"empty patterns match nothing", nil, "eval/s1.md", false}, + {"blank pattern ignored", []string{" "}, "eval/s1.md", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, MatchExclude(tc.patterns, tc.rel)) + }) + } +} + +func TestValidateExcludePattern(t *testing.T) { + for _, ok := range []string{"eval", "eval/", "fixtures/*", "*.test.md", "a/b/c", "./scratch", "[abc]*"} { + assert.NoError(t, ValidateExcludePattern(ok), ok) + } + bad := map[string]string{ + "": "must not be empty", + " ": "must not be empty", + "/abs": "must be relative", + "../up": `must not contain ".."`, + "a/../b": `must not contain ".."`, + "[unclosed": "not a valid glob", + "SKILL.md": "must always be installed", + "*.md": "must always be installed", + "SKILL.[m]d": "must always be installed", + `\\server\\x`: "must be relative", + } + for pattern, wantMsg := range bad { + err := ValidateExcludePattern(pattern) + require.Error(t, err, "pattern %q", pattern) + assert.Contains(t, err.Error(), wantMsg, "pattern %q", pattern) + } +} + +func TestExcludedFiles(t *testing.T) { + dir := t.TempDir() + mk := func(rel string) { + p := filepath.Join(dir, filepath.FromSlash(rel)) + require.NoError(t, os.MkdirAll(filepath.Dir(p), 0o755)) + require.NoError(t, os.WriteFile(p, []byte("x"), 0o644)) + } + mk("SKILL.md") + mk("eval/s1.md") + mk("eval/s2.md") + mk("references/a.md") + + got, err := ExcludedFiles(dir, []string{"eval"}) + require.NoError(t, err) + assert.ElementsMatch(t, []string{filepath.FromSlash("eval/s1.md"), filepath.FromSlash("eval/s2.md")}, got) + + got, err = ExcludedFiles(dir, []string{"nope"}) + require.NoError(t, err) + assert.Empty(t, got) +} diff --git a/internal/skill/manifest.go b/internal/skill/manifest.go index b92c8ca..65fab23 100644 --- a/internal/skill/manifest.go +++ b/internal/skill/manifest.go @@ -21,6 +21,7 @@ type frontmatter struct { Tags []string `yaml:"tags,omitempty"` AllowedTools []string `yaml:"allowed-tools,omitempty"` Metadata Metadata `yaml:"metadata"` + Install InstallConfig `yaml:"install,omitempty"` Extra map[string]any `yaml:",inline"` } @@ -90,6 +91,7 @@ func ParseManifestFromBytes(data []byte) (*Skill, error) { Tags: f.Tags, AllowedTools: f.AllowedTools, Metadata: f.Metadata, + Install: f.Install, Extra: f.Extra, Body: body, }, nil @@ -123,6 +125,7 @@ func WriteManifest(s *Skill, path string) error { Tags: s.Tags, AllowedTools: s.AllowedTools, Metadata: s.Metadata, + Install: s.Install, Extra: s.Extra, } diff --git a/internal/skill/skill.go b/internal/skill/skill.go index a75b1ec..4b68d13 100644 --- a/internal/skill/skill.go +++ b/internal/skill/skill.go @@ -59,6 +59,20 @@ type Metadata struct { Extra map[string]any `yaml:",inline" json:"-"` } +// InstallConfig is the author-owned `install:` frontmatter block controlling +// how a skill is copied onto a platform. The registry always keeps the whole +// skill directory; these settings only shape the installed copy. +type InstallConfig struct { + // Exclude lists glob patterns (relative to the skill directory, slash + // separated) for files and directories that stay in the registry but are + // not copied on install — evaluation corpora, fixtures, development + // scratch. See MatchExclude for the matching rules. + Exclude []string `yaml:"exclude,omitempty" json:"exclude,omitempty"` +} + +// IsZero lets yaml omit an empty install block on write. +func (c InstallConfig) IsZero() bool { return len(c.Exclude) == 0 } + // Skill represents an Agent Skill with frontmatter and body content. // // Extra carries every top-level frontmatter key skern does not model @@ -70,6 +84,7 @@ type Skill struct { Tags []string `yaml:"tags,omitempty" json:"tags,omitempty"` AllowedTools []string `yaml:"allowed-tools,omitempty" json:"allowed_tools,omitempty"` Metadata Metadata `yaml:"metadata" json:"metadata"` + Install InstallConfig `yaml:"install,omitempty" json:"install,omitempty"` Extra map[string]any `yaml:",inline" json:"-"` Body string `yaml:"-" json:"-"` } diff --git a/internal/skill/validator.go b/internal/skill/validator.go index 61cbc89..1794899 100644 --- a/internal/skill/validator.go +++ b/internal/skill/validator.go @@ -40,11 +40,28 @@ func Validate(s *Skill) []ValidationIssue { issues = append(issues, validateTags(s.Tags)...) issues = append(issues, validateAllowedTools(s.AllowedTools)...) issues = append(issues, validateMetadata(s.Metadata)...) + issues = append(issues, validateInstall(s.Install)...) issues = append(issues, lintStyle(s)...) return issues } +// validateInstall checks the `install.exclude` patterns: every entry must be +// a relative, well-formed glob that cannot match SKILL.md. +func validateInstall(c InstallConfig) []ValidationIssue { + var issues []ValidationIssue + for _, pattern := range c.Exclude { + if err := ValidateExcludePattern(pattern); err != nil { + issues = append(issues, ValidationIssue{ + Field: "install.exclude", + Severity: SeverityError, + Message: err.Error(), + }) + } + } + return issues +} + // HasErrors returns true if any issues have error severity. func HasErrors(issues []ValidationIssue) bool { for _, issue := range issues { @@ -145,6 +162,32 @@ func ValidateFolder(s *Skill, skillDir string) []ValidationIssue { Severity: SeverityWarning, Message: fmt.Sprintf("referenced file %q not found in skill directory", ref), }) + continue + } + // The file exists in the registry but would be missing from every + // installed copy — almost certainly a mistake in install.exclude. + if MatchExclude(s.Install.Exclude, ref) { + issues = append(issues, ValidationIssue{ + Field: "install.exclude", + Severity: SeverityWarning, + Message: fmt.Sprintf("referenced file %q is excluded from install by install.exclude", ref), + }) + } + } + + // A pattern that matches nothing on disk is usually a typo (or stale + // after a rename) — worth a warning, not an error. + for _, pattern := range s.Install.Exclude { + if ValidateExcludePattern(pattern) != nil { + continue // reported as an error by Validate + } + matched, err := ExcludedFiles(skillDir, []string{pattern}) + if err == nil && len(matched) == 0 { + issues = append(issues, ValidationIssue{ + Field: "install.exclude", + Severity: SeverityWarning, + Message: fmt.Sprintf("exclude pattern %q matches no file in the skill directory", pattern), + }) } } diff --git a/internal/skill/validator_test.go b/internal/skill/validator_test.go index fc0cb29..8e829a4 100644 --- a/internal/skill/validator_test.go +++ b/internal/skill/validator_test.go @@ -1,6 +1,8 @@ package skill import ( + "os" + "path/filepath" "strings" "testing" @@ -479,3 +481,68 @@ func TestScaffold_FreshSkillProducesNoLintHints(t *testing.T) { issues := lintStyle(s) assert.Empty(t, issues, "freshly scaffolded skill must produce zero lint hints") } + +func TestValidate_InstallExclude(t *testing.T) { + base := func(exclude ...string) *Skill { + return &Skill{ + Name: "lean-skill", + Description: "Use when you need to complete tasks with step-by-step guidance", + Body: "## Overview\n\nUse this skill to do things carefully and well with plenty of words here.\n", + Metadata: Metadata{Author: Author{Name: "a", Type: "human"}, Version: "0.1.0"}, + Install: InstallConfig{Exclude: exclude}, + } + } + issuesFor := func(s *Skill) []ValidationIssue { + var out []ValidationIssue + for _, i := range Validate(s) { + if i.Field == "install.exclude" { + out = append(out, i) + } + } + return out + } + + assert.Empty(t, issuesFor(base())) + assert.Empty(t, issuesFor(base("eval", "fixtures/*", "*.test.md"))) + + got := issuesFor(base("eval", "../up", "SKILL.md", "[x")) + require.Len(t, got, 3) + for _, i := range got { + assert.Equal(t, SeverityError, i.Severity) + } + assert.True(t, HasErrors(got)) +} + +func TestValidateFolder_InstallExcludeWarnings(t *testing.T) { + dir := t.TempDir() + mk := func(rel, content string) { + p := filepath.Join(dir, filepath.FromSlash(rel)) + require.NoError(t, os.MkdirAll(filepath.Dir(p), 0o755)) + require.NoError(t, os.WriteFile(p, []byte(content), 0o644)) + } + mk("SKILL.md", "x") + mk("eval/s1.md", "x") + mk("references/guide.md", "x") + + s := &Skill{ + Name: "lean-skill", + Description: "Use when testing.", + Body: "See `references/guide.md` for details.\n", + Install: InstallConfig{Exclude: []string{"eval", "references", "nothing-here", "[bad"}}, + } + issues := ValidateFolder(s, dir) + + var msgs []string + for _, i := range issues { + assert.Equal(t, SeverityWarning, i.Severity) + msgs = append(msgs, i.Message) + } + joined := strings.Join(msgs, "\n") + assert.Contains(t, joined, `referenced file "references/guide.md" is excluded from install`) + assert.Contains(t, joined, `exclude pattern "nothing-here" matches no file`) + // "eval" matches eval/s1.md: no warning. The malformed pattern is an + // error for Validate, not a folder warning. + assert.NotContains(t, joined, `"eval" matches no file`) + assert.NotContains(t, joined, "[bad") + assert.Len(t, issues, 2) +} From c029a0cd7cafc08600f5e43179f43428b02784c4 Mon Sep 17 00:00:00 2001 From: devrimcavusoglu Date: Wed, 19 Aug 2026 16:11:39 +0300 Subject: [PATCH 2/3] Address review: diff install.exclude, pass through install.* extras, refuse bad patterns at install - skill diff reports a changed install.exclude list (it went blind once install became a modeled key) and diffs install.* extras. - InstallConfig gets an inline Extra map (+ collision guard) so other keys under install: round-trip like every other unmodeled key; `exclude` accepts a bare string as a one-element list. - skill install refuses a skill whose patterns fail validation instead of silently copying what path.Match could not parse; `**` is rejected outright rather than degrading to `*`. - Only the literal SKILL.md is an error: wildcards that also match it are legal since MatchExclude never excludes the manifest. Backslashes are left alone (path.Match escape), validator field carries [index] for multi-entry lists. - Docs: no `--files` flag; exact matching/escape/compat rules; roadmap line dropped; json tag omitzero. Co-Authored-By: Claude Fable 5 --- AGENTS.md | 14 +++-- CHANGELOG.md | 15 +++-- docs/concepts/skill-format.md | 9 +-- docs/reference/validation.md | 2 +- internal/cli/skill_diff.go | 9 +++ internal/cli/skill_install.go | 11 +++- internal/cli/skill_test.go | 100 ++++++++++++++++++++++++++++++- internal/skill/folder.go | 38 +++++++++--- internal/skill/folder_test.go | 24 ++++++-- internal/skill/manifest.go | 4 ++ internal/skill/skill.go | 36 +++++++++-- internal/skill/validator.go | 8 ++- internal/skill/validator_test.go | 14 ++++- 13 files changed, 242 insertions(+), 42 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index daacbbd..89fd7f4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -205,11 +205,14 @@ exclusive with positional names. **`install.exclude`** ([#103]) lists `path.Match` globs, relative to the skill directory, that `skill install` leaves out of the platform copy (the registry keeps everything). A pattern matches a path or any leading directory of it, -so a bare directory name excludes its subtree; `SKILL.md` is never excluded. -Matching lives in `skill.MatchExclude` (`internal/skill/folder.go`) and is -applied by `platform.copyDir` via `InstallOptions.Exclude`; `skill validate` -errors on malformed patterns and warns on no-match / excluded-but-referenced -files. +so a bare directory name excludes its subtree; `**` is rejected; `SKILL.md` +is never excluded (so `*` is legal). Matching lives in `skill.MatchExclude` +(`internal/skill/folder.go`) and is applied by `platform.copyDir` via +`InstallOptions.Exclude`; `skill validate` errors on malformed patterns and +warns on no-match / excluded-but-referenced files; `skill install` refuses a +skill whose patterns fail validation; `skill diff` reports a changed list +under `install.exclude`. Other keys under `install:` pass through via +`InstallConfig.Extra`. **Unmodeled keys pass through.** The fields above are the keys skern models; any other key — top-level, `metadata.*`, or nested in `metadata.author` / @@ -341,7 +344,6 @@ Everything planned is a tracked issue; this list is a map, not a commitment. **Correctness and ergonomics (near-term)** -- [#103] — exclude companion directories (eval corpora, fixtures) from install **Adapter model** — all three need the declarative-hook mechanism from design decision 3; settle the mechanism once rather than special-casing each. diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c6e581..85af999 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,11 +39,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 registry but not in an agent's context — evaluation corpora, fixtures, scratch. `skern skill install` skips them; the registry keeps everything. A bare directory name excludes its subtree (`eval`), `fixtures/*` its - children, `*.draft.md` top-level files by suffix; `SKILL.md` can never be - excluded. `skill validate` errors on malformed, absolute, or `..` patterns - and warns when a pattern matches nothing or excludes a file the body - references. Platform adapters take an `InstallOptions` argument on - `Install`. ([#103]) + children, `*.draft.md` top-level files by suffix; `**` is rejected; + `SKILL.md` is never excluded (so `*` means "everything else"). + `skill validate` errors on malformed, absolute, `..`, or `**` patterns and + warns when a pattern matches nothing or excludes a file the body + references; `skill install` refuses a skill whose patterns fail + validation; `skill diff` reports a changed exclude list. Platform adapters + take an `InstallOptions` argument on `Install`. Compatibility: `install` + is now a modeled frontmatter key — a top-level `install:` that is not a + mapping fails to parse, and other keys under `install:` pass through like + any unmodeled key. ([#103]) ### Changed diff --git a/docs/concepts/skill-format.md b/docs/concepts/skill-format.md index d5c43b5..fb3e42d 100644 --- a/docs/concepts/skill-format.md +++ b/docs/concepts/skill-format.md @@ -117,11 +117,12 @@ install: Rules: -- Patterns are slash-separated paths relative to the skill directory, using Go `path.Match` syntax (`*`, `?`, `[…]`). There is no `**`; a pattern matches a path when it matches the full relative path **or any leading directory of it**, which is what makes a bare directory name exclude its whole subtree. A trailing `/` or leading `./` is ignored. -- `SKILL.md` can never be excluded; absolute paths, `..` segments, and malformed globs are validation **errors**. -- The registry always keeps the full directory — `skill create --from-template`, `skill import`, and `skill show --files` are unaffected. Only the copy made by `skern skill install` is trimmed. +- Patterns are slash-separated paths relative to the skill directory, using Go `path.Match` syntax (`*`, `?`, `[…]`; `\` escapes a metacharacter). `*` does not cross `/`, and `**` is **not** supported (it is rejected, not silently treated as `*`); a pattern matches a path when it matches the full relative path **or any leading directory of it**, which is what makes a bare directory name exclude its whole subtree. A trailing `/` or leading `./` is ignored. `exclude: eval` (a bare string) is accepted as shorthand for a one-element list. +- `SKILL.md` is never excluded, whatever the patterns say — so `*` or `*.md` are legal and mean "everything except `SKILL.md`". Naming `SKILL.md` literally, absolute paths, `..` segments, `**`, and malformed globs are validation **errors**, and `skern skill install` refuses a skill whose patterns fail validation rather than silently copying what the author meant to leave out. +- The registry always keeps the full directory — `skill create --from-template` and `skill import` copy everything, and `skill show` lists every file. Only the copy made by `skern skill install` is trimmed. (A pattern such as `fixtures/*` prunes the directory's contents; the now-empty `fixtures/` itself is still created.) - `skern skill validate` **warns** when a pattern matches no file in the skill directory, and when a file the body references (`` `references/guide.md` ``) is excluded — it would exist in the registry but be missing from every installed copy. -- `skern skill diff` compares manifests (frontmatter and body), not file trees, so excluded files never show up as drift. +- `skern skill diff` compares manifests (frontmatter and body), not file trees, so excluded files never show up as drift — but a change to the `install.exclude` list itself *is* reported (`install.exclude`), since it changes what the next install copies. +- `install` is a skern-modeled key: other keys under it (`install.mode`, …) round-trip like any other unmodeled key, but a top-level `install:` that is not a mapping (e.g. `install: pip install foo`) is a parse error. ## Creating Skills diff --git a/docs/reference/validation.md b/docs/reference/validation.md index 2fc103f..c950c14 100644 --- a/docs/reference/validation.md +++ b/docs/reference/validation.md @@ -44,7 +44,7 @@ If `allowed-tools` is set, no entry may be empty. ### Install Exclusions -Every `install.exclude` entry must be a non-empty, relative, well-formed glob (`path.Match` syntax) with no `..` segment, and must not match `SKILL.md`. See [Skill Format › Install-time exclusions](/concepts/skill-format#install-time-exclusions). +Every `install.exclude` entry must be a non-empty, relative, well-formed glob (`path.Match` syntax) with no `..` or `**` segment, and must not name `SKILL.md` literally (wildcards that merely also match it are fine — `SKILL.md` is never excluded). With more than one entry the field is reported as `install.exclude[N]`. `skern skill install` refuses a skill whose patterns fail these checks. See [Skill Format › Install-time exclusions](/concepts/skill-format#install-time-exclusions). ## Warnings diff --git a/internal/cli/skill_diff.go b/internal/cli/skill_diff.go index 153b0bb..770f160 100644 --- a/internal/cli/skill_diff.go +++ b/internal/cli/skill_diff.go @@ -186,9 +186,18 @@ func compareSkills(a *skill.Skill, nameA, sourceA string, b *skill.Skill, nameB, fields = append(fields, output.FieldDiff{Field: "modified-by", Left: modA, Right: modB}) } + // install.exclude decides what an install copies; a change here is the + // signal to reinstall, so it must show up. + exA := strings.Join(a.Install.Exclude, ", ") + exB := strings.Join(b.Install.Exclude, ", ") + if exA != exB { + fields = append(fields, output.FieldDiff{Field: "install.exclude", Left: exA, Right: exB}) + } + // Unmodeled keys round-trip through skern (see skill.Skill.Extra), so // they are part of what an install or edit can change — diff them too. fields = append(fields, extraDiffs("", a.Extra, b.Extra)...) + fields = append(fields, extraDiffs("install.", a.Install.Extra, b.Install.Extra)...) fields = append(fields, extraDiffs("metadata.", a.Metadata.Extra, b.Metadata.Extra)...) fields = append(fields, extraDiffs("author.", a.Metadata.Author.Extra, b.Metadata.Author.Extra)...) diff --git a/internal/cli/skill_install.go b/internal/cli/skill_install.go index c90a19e..51f2962 100644 --- a/internal/cli/skill_install.go +++ b/internal/cli/skill_install.go @@ -26,7 +26,8 @@ func newSkillInstallCmd() *cobra.Command { Each skill must already exist in skern's registry — create it with 'skern skill create' or pull it in with 'skern skill import' before installing. Install copies -the registered skill into the platform's skill directory; the registry copy is +the registered skill directory into the platform's skill directory, minus any +paths the skill's frontmatter lists under install.exclude; the registry copy is left untouched. Each invocation targets exactly one platform — agents are expected to specify @@ -105,6 +106,14 @@ installed-skill count would meet or exceed the per-platform threshold (see continue } + // A malformed exclude pattern would silently install what the + // author meant to leave out; refuse this skill instead. + if vErr := skill.ValidateExcludePatterns(s.Install.Exclude); vErr != nil { + entry.Error = fmt.Sprintf("%v (fix the skill's frontmatter; 'skern skill validate %s' lists every problem)", vErr, name) + entries = append(entries, entry) + continue + } + if force { // Best-effort cleanup: skill may not be installed yet. _ = p.Uninstall(name, scopeVal) diff --git a/internal/cli/skill_test.go b/internal/cli/skill_test.go index 7159696..8b32cb3 100644 --- a/internal/cli/skill_test.go +++ b/internal/cli/skill_test.go @@ -1533,7 +1533,7 @@ See `+"`references/guide.md`"+`. // The install block survives the registry write (needs #100's modeling). regManifest := filepath.Join(projectReg, "demo.skill", "SKILL.md") parsed := requireParsedManifest(t, regManifest) - assert.Equal(t, []string{"eval", "*.draft.md"}, parsed.Install.Exclude) + assert.Equal(t, skill.StringList{"eval", "*.draft.md"}, parsed.Install.Exclude) raw, err := os.ReadFile(regManifest) require.NoError(t, err) assert.Contains(t, string(raw), "install:\n exclude:\n - eval\n - '*.draft.md'\n") @@ -1594,8 +1594,104 @@ Body text long enough to avoid lint noise about brevity in this validation test. out, err := runCmd(t, cc, "skill", "validate", "bad-exclude", "--scope", "project", "--json") require.Error(t, err, "errors in install.exclude must fail validation") assert.Contains(t, out, `must not contain \"..\"`) - assert.Contains(t, out, "must always be installed") + assert.Contains(t, out, "is always installed") assert.Contains(t, out, `exclude pattern \"ghost\" matches no file`) // "eval" is valid and matches: neither error nor warning mentions it. assert.NotContains(t, out, `\"eval\"`) } + +// An invalid install.exclude pattern must not be silently ignored by +// install (path.Match would just return false and copy the file the author +// meant to leave out) — the skill is refused with a per-skill error. +func TestSkillInstall_RefusesInvalidExcludePattern(t *testing.T) { + cc, _, projectReg := testRegistryWithDirs(t) + home := t.TempDir() + project := t.TempDir() + withTestDetector(t, cc, home, project) + + dir := filepath.Join(projectReg, "bad-glob") + require.NoError(t, os.MkdirAll(filepath.Join(dir, "eval"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "eval", "s1.md"), []byte("x"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(`--- +name: bad-glob +description: Use when testing refused installs. +install: + exclude: ["[eval", "ok-dir"] +--- + +Body. +`), 0o644)) + + out, err := runCmd(t, cc, "skill", "install", "bad-glob", "--platform", "claude-code", "--scope", "project", "--json") + require.Error(t, err) + var result output.SkillInstallResult + require.NoError(t, json.Unmarshal([]byte(out), &result)) + require.Len(t, result.Skills, 1) + assert.False(t, result.Skills[0].Success) + assert.Contains(t, result.Skills[0].Error, "install.exclude[0]") + assert.Contains(t, result.Skills[0].Error, "not a valid glob") + _, statErr := os.Stat(filepath.Join(project, ".claude", "skills", "bad-glob")) + assert.True(t, os.IsNotExist(statErr), "nothing may be installed when a pattern is invalid") +} + +// Changing install.exclude changes what an install copies, so diff must +// report it (it is a modeled key now, not an Extra one). Unmodeled keys +// under install: round-trip and diff like any other extra. +func TestSkillDiff_InstallExcludeAndExtras(t *testing.T) { + cc, userDir, _ := testRegistryWithDirs(t) + write := func(name, install string) { + dir := filepath.Join(userDir, name) + require.NoError(t, os.MkdirAll(dir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(`--- +name: `+name+` +description: Use when diffing install blocks. +`+install+`metadata: + author: + name: alice + type: human + version: "0.1.0" +--- + +Body. +`), 0o644)) + } + write("inst-a", "install:\n exclude: [eval]\n mode: strict\n") + write("inst-b", "install:\n exclude: eval\n") + + // Scalar shorthand parses as a one-element list. + b := requireParsedManifest(t, filepath.Join(userDir, "inst-b", "SKILL.md")) + assert.Equal(t, skill.StringList{"eval"}, b.Install.Exclude) + a := requireParsedManifest(t, filepath.Join(userDir, "inst-a", "SKILL.md")) + assert.Equal(t, "strict", a.Install.Extra["mode"]) + + // Identical exclude lists, differing extra key under install:. + out, err := runCmd(t, cc, "skill", "diff", "inst-a", "inst-b", "--json") + require.NoError(t, err) + var result output.SkillDiffResult + require.NoError(t, json.Unmarshal([]byte(out), &result)) + fieldMap := map[string]output.FieldDiff{} + for _, f := range result.Fields { + fieldMap[f.Field] = f + } + assert.NotContains(t, fieldMap, "install.exclude") + require.Contains(t, fieldMap, "install.mode") + assert.Equal(t, "strict", fieldMap["install.mode"].Left) + + // Edit the exclude list on one side: reported under install.exclude, and + // the extra key survives the rewrite. + _, err = runCmd(t, cc, "skill", "edit", "inst-a", "--description", "Use when diffing install blocks (edited).") + require.NoError(t, err) + a = requireParsedManifest(t, filepath.Join(userDir, "inst-a", "SKILL.md")) + assert.Equal(t, "strict", a.Install.Extra["mode"], "unmodeled install.* keys must survive edit") + write("inst-b", "install:\n exclude: [eval, fixtures/*]\n") + out, err = runCmd(t, cc, "skill", "diff", "inst-a", "inst-b", "--json") + require.NoError(t, err) + require.NoError(t, json.Unmarshal([]byte(out), &result)) + fieldMap = map[string]output.FieldDiff{} + for _, f := range result.Fields { + fieldMap[f.Field] = f + } + require.Contains(t, fieldMap, "install.exclude") + assert.Equal(t, "eval", fieldMap["install.exclude"].Left) + assert.Equal(t, "eval, fixtures/*", fieldMap["install.exclude"].Right) +} diff --git a/internal/skill/folder.go b/internal/skill/folder.go index 5ce5a1a..ca40a51 100644 --- a/internal/skill/folder.go +++ b/internal/skill/folder.go @@ -99,36 +99,56 @@ func ExtractFileReferences(body string) []string { // ValidateExcludePattern checks one `install.exclude` entry. Patterns are // slash-separated paths relative to the skill directory using path.Match -// syntax (`*`, `?`, `[...]`; no `**`). Rejected: empty, absolute, any `..` -// segment, a malformed glob, and anything that would match SKILL.md itself. +// syntax (`*`, `?`, `[...]`, `\` escapes a metacharacter; no `**`). +// Rejected: empty, absolute, any `..` segment, a `**` segment, a malformed +// glob, and the literal SKILL.md. (A wildcard that merely also matches +// SKILL.md — `*`, `*.md` — is allowed: MatchExclude never excludes the +// manifest, so such a pattern means "everything except SKILL.md".) func ValidateExcludePattern(pattern string) error { p := normalizeExcludePattern(pattern) if p == "" { return fmt.Errorf("exclude pattern must not be empty") } - if strings.HasPrefix(p, "/") || filepath.IsAbs(pattern) || strings.HasPrefix(pattern, "\\") { + if strings.HasPrefix(p, "/") || filepath.IsAbs(pattern) { return fmt.Errorf("exclude pattern %q must be relative to the skill directory", pattern) } for _, seg := range strings.Split(p, "/") { if seg == ".." { return fmt.Errorf("exclude pattern %q must not contain \"..\"", pattern) } + if strings.Contains(seg, "**") { + return fmt.Errorf("exclude pattern %q: \"**\" is not supported (a bare directory name already excludes its whole subtree; `*` does not cross \"/\")", pattern) + } } if _, err := path.Match(p, "x"); err != nil { return fmt.Errorf("exclude pattern %q is not a valid glob: %v", pattern, err) } - if ok, _ := path.Match(p, ManifestFile); ok { - return fmt.Errorf("exclude pattern %q would exclude %s, which must always be installed", pattern, ManifestFile) + if p == ManifestFile { + return fmt.Errorf("exclude pattern %q names %s, which is always installed", pattern, ManifestFile) + } + return nil +} + +// ValidateExcludePatterns validates every pattern and returns the first +// error, prefixed with the pattern's index for multi-entry lists. +func ValidateExcludePatterns(patterns []string) error { + for i, p := range patterns { + if err := ValidateExcludePattern(p); err != nil { + if len(patterns) > 1 { + return fmt.Errorf("install.exclude[%d]: %w", i, err) + } + return fmt.Errorf("install.exclude: %w", err) + } } return nil } -// normalizeExcludePattern trims whitespace, converts backslashes to slashes, -// and strips a leading "./" and any trailing "/" (a directory pattern -// written as "eval/" means the same as "eval"). +// normalizeExcludePattern trims whitespace and strips a leading "./" and any +// trailing "/" (a directory pattern written as "eval/" means the same as +// "eval"). Backslashes are left alone: patterns are slash-separated and `\` +// is path.Match's escape character. func normalizeExcludePattern(pattern string) string { p := strings.TrimSpace(pattern) - p = strings.ReplaceAll(p, "\\", "/") p = strings.TrimPrefix(p, "./") p = strings.TrimRight(p, "/") return p diff --git a/internal/skill/folder_test.go b/internal/skill/folder_test.go index ed1f448..3ab6bb6 100644 --- a/internal/skill/folder_test.go +++ b/internal/skill/folder_test.go @@ -278,7 +278,10 @@ func TestMatchExclude(t *testing.T) { } func TestValidateExcludePattern(t *testing.T) { - for _, ok := range []string{"eval", "eval/", "fixtures/*", "*.test.md", "a/b/c", "./scratch", "[abc]*"} { + // `*` / `*.md` are legal: SKILL.md is protected at match time, so they + // mean "everything except SKILL.md". `\*.md` is path.Match's escape for + // a literal star, not a separator. + for _, ok := range []string{"eval", "eval/", "fixtures/*", "*.test.md", "a/b/c", "./scratch", "[abc]*", "*", "*.md", "SKILL.[m]d", `\*.md`, "./SKILL.md/x"} { assert.NoError(t, ValidateExcludePattern(ok), ok) } bad := map[string]string{ @@ -288,16 +291,27 @@ func TestValidateExcludePattern(t *testing.T) { "../up": `must not contain ".."`, "a/../b": `must not contain ".."`, "[unclosed": "not a valid glob", - "SKILL.md": "must always be installed", - "*.md": "must always be installed", - "SKILL.[m]d": "must always be installed", - `\\server\\x`: "must be relative", + "SKILL.md": "is always installed", + "./SKILL.md/": "is always installed", + "**/x": `"**" is not supported`, + "a/**": `"**" is not supported`, + "a**b": `"**" is not supported`, } for pattern, wantMsg := range bad { err := ValidateExcludePattern(pattern) require.Error(t, err, "pattern %q", pattern) assert.Contains(t, err.Error(), wantMsg, "pattern %q", pattern) } + + // ValidateExcludePatterns prefixes the index only for multi-entry lists. + assert.NoError(t, ValidateExcludePatterns(nil)) + err := ValidateExcludePatterns([]string{"ok", "[x"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "install.exclude[1]") + err = ValidateExcludePatterns([]string{"[x"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "install.exclude: ") + assert.NotContains(t, err.Error(), "[0]") } func TestExcludedFiles(t *testing.T) { diff --git a/internal/skill/manifest.go b/internal/skill/manifest.go index 65fab23..d17dea9 100644 --- a/internal/skill/manifest.go +++ b/internal/skill/manifest.go @@ -34,6 +34,7 @@ var ( modeledMetadataKeys = yamlKeys(reflect.TypeOf(Metadata{})) modeledAuthorKeys = yamlKeys(reflect.TypeOf(Author{})) modeledModifiedByKeys = yamlKeys(reflect.TypeOf(ModifiedByEntry{})) + modeledInstallKeys = yamlKeys(reflect.TypeOf(InstallConfig{})) ) // yamlKeys returns the set of YAML keys yaml.v3 would use for a struct type: @@ -118,6 +119,9 @@ func WriteManifest(s *Skill, path string) error { return err } } + if err := checkExtraCollisions("install", s.Install.Extra, modeledInstallKeys); err != nil { + return err + } fm := frontmatter{ Name: s.Name, diff --git a/internal/skill/skill.go b/internal/skill/skill.go index 4b68d13..da6ef54 100644 --- a/internal/skill/skill.go +++ b/internal/skill/skill.go @@ -5,6 +5,8 @@ import ( "fmt" "regexp" "strings" + + "gopkg.in/yaml.v3" ) // Scope represents where a skill is stored. @@ -66,12 +68,38 @@ type InstallConfig struct { // Exclude lists glob patterns (relative to the skill directory, slash // separated) for files and directories that stay in the registry but are // not copied on install — evaluation corpora, fixtures, development - // scratch. See MatchExclude for the matching rules. - Exclude []string `yaml:"exclude,omitempty" json:"exclude,omitempty"` + // scratch. See MatchExclude for the matching rules. A single string is + // accepted in YAML as shorthand for a one-element list. + Exclude StringList `yaml:"exclude,omitempty" json:"exclude,omitempty"` + // Extra carries unmodeled keys under install: (see Metadata.Extra). + Extra map[string]any `yaml:",inline" json:"-"` } // IsZero lets yaml omit an empty install block on write. -func (c InstallConfig) IsZero() bool { return len(c.Exclude) == 0 } +func (c InstallConfig) IsZero() bool { return len(c.Exclude) == 0 && len(c.Extra) == 0 } + +// StringList is a []string that also accepts a bare scalar in YAML +// (`exclude: eval` means `exclude: [eval]`). It always marshals as a +// sequence. +type StringList []string + +// UnmarshalYAML implements yaml.Unmarshaler. +func (l *StringList) UnmarshalYAML(node *yaml.Node) error { + if node.Kind == yaml.ScalarNode { + var one string + if err := node.Decode(&one); err != nil { + return err + } + *l = StringList{one} + return nil + } + var many []string + if err := node.Decode(&many); err != nil { + return err + } + *l = StringList(many) + return nil +} // Skill represents an Agent Skill with frontmatter and body content. // @@ -84,7 +112,7 @@ type Skill struct { Tags []string `yaml:"tags,omitempty" json:"tags,omitempty"` AllowedTools []string `yaml:"allowed-tools,omitempty" json:"allowed_tools,omitempty"` Metadata Metadata `yaml:"metadata" json:"metadata"` - Install InstallConfig `yaml:"install,omitempty" json:"install,omitempty"` + Install InstallConfig `yaml:"install,omitempty" json:"install,omitzero"` Extra map[string]any `yaml:",inline" json:"-"` Body string `yaml:"-" json:"-"` } diff --git a/internal/skill/validator.go b/internal/skill/validator.go index 1794899..b7ddf14 100644 --- a/internal/skill/validator.go +++ b/internal/skill/validator.go @@ -50,10 +50,14 @@ func Validate(s *Skill) []ValidationIssue { // a relative, well-formed glob that cannot match SKILL.md. func validateInstall(c InstallConfig) []ValidationIssue { var issues []ValidationIssue - for _, pattern := range c.Exclude { + for i, pattern := range c.Exclude { if err := ValidateExcludePattern(pattern); err != nil { + field := "install.exclude" + if len(c.Exclude) > 1 { + field = fmt.Sprintf("install.exclude[%d]", i) + } issues = append(issues, ValidationIssue{ - Field: "install.exclude", + Field: field, Severity: SeverityError, Message: err.Error(), }) diff --git a/internal/skill/validator_test.go b/internal/skill/validator_test.go index 8e829a4..6ea4207 100644 --- a/internal/skill/validator_test.go +++ b/internal/skill/validator_test.go @@ -495,7 +495,7 @@ func TestValidate_InstallExclude(t *testing.T) { issuesFor := func(s *Skill) []ValidationIssue { var out []ValidationIssue for _, i := range Validate(s) { - if i.Field == "install.exclude" { + if strings.HasPrefix(i.Field, "install.exclude") { out = append(out, i) } } @@ -505,11 +505,19 @@ func TestValidate_InstallExclude(t *testing.T) { assert.Empty(t, issuesFor(base())) assert.Empty(t, issuesFor(base("eval", "fixtures/*", "*.test.md"))) - got := issuesFor(base("eval", "../up", "SKILL.md", "[x")) - require.Len(t, got, 3) + // Wildcards that also match SKILL.md are fine (it is never excluded). + assert.Empty(t, issuesFor(base("*", "*.md"))) + + got := issuesFor(base("eval", "../up", "SKILL.md", "[x", "a/**/b")) + require.Len(t, got, 4) for _, i := range got { assert.Equal(t, SeverityError, i.Severity) + assert.Regexp(t, `^install\.exclude\[\d\]$`, i.Field, "multi-entry lists carry the index") } + // A single bad entry is reported without an index. + single := issuesFor(base("[x")) + require.Len(t, single, 1) + assert.Equal(t, "install.exclude", single[0].Field) assert.True(t, HasErrors(got)) } From 2d9f568c950c4644f05e27934e5f4dc805950a8f Mon Sep 17 00:00:00 2001 From: devrimcavusoglu Date: Wed, 19 Aug 2026 16:13:30 +0300 Subject: [PATCH 3/3] Restore skill import after restack; drop emptied roadmap heading #108 removed the then-unused skill import from skill_install.go and #109 needs it again for ValidateExcludePatterns. The four near-term roadmap entries are all closed by this stack, so the group heading goes too. Co-Authored-By: Claude Fable 5 --- AGENTS.md | 3 --- internal/cli/skill_install.go | 1 + 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 89fd7f4..81b4f8f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -342,9 +342,6 @@ request. Everything planned is a tracked issue; this list is a map, not a commitment. -**Correctness and ergonomics (near-term)** - - **Adapter model** — all three need the declarative-hook mechanism from design decision 3; settle the mechanism once rather than special-casing each. diff --git a/internal/cli/skill_install.go b/internal/cli/skill_install.go index 51f2962..d2f66ce 100644 --- a/internal/cli/skill_install.go +++ b/internal/cli/skill_install.go @@ -7,6 +7,7 @@ import ( "github.com/devrimcavusoglu/skern/internal/output" "github.com/devrimcavusoglu/skern/internal/platform" "github.com/devrimcavusoglu/skern/internal/registry" + "github.com/devrimcavusoglu/skern/internal/skill" "github.com/spf13/cobra" )