From 7577fa2ffaae56af3cd8fbd4b646cb0ab8f4bbab Mon Sep 17 00:00:00 2001 From: devrimcavusoglu Date: Mon, 15 Jun 2026 12:09:14 +0300 Subject: [PATCH 1/4] Add namespaced --category filter to skern skill list (#96) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a repeatable, domain-agnostic `--category category:value` flag to `skern skill list` for narrowing a skill set by structured tag dimensions (e.g. `lang:python`, `topic:testing`) without skern knowing what any category means. The existing flat `--tag` filter is unchanged; the two compose with AND. Semantics (the two design questions the issue left open): - Untagged / category-absent handling: strict by default — a skill with no tag in a requested category is excluded. `--include-untagged` opts into "absent = applies to all". A category the skill *does* declare must still match a requested value even with the flag. - Combination: OR within a category, AND across categories. Values can be supplied as repeated flags (`--category lang:python --category lang:go`) or a comma list (`--category lang:python,go`); the two forms are equivalent. The matcher (`matchesCategories`) sits beside `hasTag` in skill_helpers.go; `parseCategoryFilters` validates flag input and returns a ValidationError (exit code 2) for a missing colon, empty category, or empty value. Matching is case-insensitive, consistent with `hasTag`. Flat tags (no colon) are never categorical and remain `--tag` territory. Tests cover the parser, the matcher (OR/AND, strict vs include-untagged, zero-tag and flat-tag edge cases, case-insensitivity), and the end-to-end `--json` contract including --tag/--category composition and the exit-code-2 path. Docs updated in reference/commands.md. Co-Authored-By: Claude Fable 5 --- docs/reference/commands.md | 13 ++- internal/cli/skill_helpers.go | 86 ++++++++++++++ internal/cli/skill_list.go | 18 ++- internal/cli/skill_test.go | 211 ++++++++++++++++++++++++++++++++++ 4 files changed, 324 insertions(+), 4 deletions(-) diff --git a/docs/reference/commands.md b/docs/reference/commands.md index 127f913..dca2fe2 100644 --- a/docs/reference/commands.md +++ b/docs/reference/commands.md @@ -174,9 +174,20 @@ skern skill list [--scope user|project|all] [flags] | Flag | Default | Description | |------|---------|-------------| | `--scope` | `all` | `user`, `project`, or `all` | -| `--tag` | — | Filter results to skills with this tag | +| `--tag` | — | Filter results to skills with this flat tag (exact, case-insensitive) | +| `--category` | — | Filter by a namespaced `category:value` tag (repeatable) | +| `--include-untagged` | `false` | Treat a skill with no tag in a requested category as matching that category | | `--with-platforms` | `false` | Include `installed_on` per skill (the detected platforms where the skill is installed at the same scope) | +### Categorical-tag filtering (`--category`) + +`--category` narrows the list by structured `category:value` tags (e.g. `lang:python`, `topic:testing`). It is fully category-agnostic — the namespace is whatever precedes the first `:`; skern never enumerates known categories. Flat tags with no colon are not categorical and are matched by `--tag` instead. + +- **Repeatable, with comma-lists:** `--category lang:python --category lang:go` and `--category lang:python,go` are equivalent. +- **OR within a category, AND across categories:** `--category lang:python,go --category topic:testing` matches skills tagged (`lang:python` **or** `lang:go`) **and** `topic:testing`. +- **Strict by default:** a skill that carries no tag in a requested category is excluded. Pass `--include-untagged` to treat a category-absent skill as applying to all values of that category. A category the skill *does* declare must still match a requested value even with `--include-untagged`. +- Matching is case-insensitive. `--tag` and `--category` compose with AND. Malformed input (`--category value` with no colon, an empty category, or an empty value) exits with code 2. + Also runs pairwise overlap detection across all listed skills and appends a "Potential duplicates" section when matches are found (score >= 0.6). In `--json` mode they appear in the `duplicates` array. Skills that cannot be parsed are reported as parse warnings rather than silently skipped — text mode prints `WARNING:` lines, `--json` mode populates the `parse_warnings` array. diff --git a/internal/cli/skill_helpers.go b/internal/cli/skill_helpers.go index 76fa214..1be9218 100644 --- a/internal/cli/skill_helpers.go +++ b/internal/cli/skill_helpers.go @@ -184,6 +184,92 @@ func hasTag(tags []string, tag string) bool { return false } +// parseCategoryFilters converts repeated --category flags into a namespace -> +// requested-values map. Each flag value has the form "category:value" and may +// carry a comma-separated value list ("lang:python,go"). Namespaces and values +// are lowercased so matching is case-insensitive, consistent with hasTag. +// +// Malformed input is a ValidationError (exit code 2): a value with no colon, +// an empty category name, or an empty value. Flat tags (no colon) are a +// different surface — they belong to --tag, not --category. +func parseCategoryFilters(raw []string) (map[string][]string, error) { + filters := map[string][]string{} + for _, entry := range raw { + ns, valStr, found := strings.Cut(entry, ":") + if !found { + return nil, &ValidationError{Message: fmt.Sprintf("invalid --category %q: expected format \"category:value\"", entry)} + } + ns = strings.ToLower(strings.TrimSpace(ns)) + if ns == "" { + return nil, &ValidationError{Message: fmt.Sprintf("invalid --category %q: category name must not be empty", entry)} + } + for _, v := range strings.Split(valStr, ",") { + v = strings.ToLower(strings.TrimSpace(v)) + if v == "" { + return nil, &ValidationError{Message: fmt.Sprintf("invalid --category %q: value must not be empty", entry)} + } + filters[ns] = append(filters[ns], v) + } + } + return filters, nil +} + +// matchesCategories reports whether a skill's tags satisfy the requested +// category filters. Semantics: OR within a category (any requested value +// matches), AND across categories (every requested category must be satisfied). +// +// A skill is "category-absent" for a namespace when none of its tags carry that +// namespace. By default an absent category fails the match (strict). When +// includeUntagged is set, an absent category is treated as "applies to all" and +// passes — but a category the skill *does* declare must still match a requested +// value. An empty filter set matches everything. +func matchesCategories(tags []string, filters map[string][]string, includeUntagged bool) bool { + if len(filters) == 0 { + return true + } + + // Index the skill's categorical tags as namespace -> set of values. + // Flat tags (no colon) and malformed tags (empty namespace/value) are + // not categorical and are ignored here. + skillCats := map[string]map[string]bool{} + for _, t := range tags { + ns, val, found := strings.Cut(t, ":") + if !found { + continue + } + ns = strings.ToLower(strings.TrimSpace(ns)) + val = strings.ToLower(strings.TrimSpace(val)) + if ns == "" || val == "" { + continue + } + if skillCats[ns] == nil { + skillCats[ns] = map[string]bool{} + } + skillCats[ns][val] = true + } + + for ns, wanted := range filters { + have, present := skillCats[ns] + if !present { + if includeUntagged { + continue + } + return false + } + matched := false + for _, w := range wanted { + if have[w] { + matched = true + break + } + } + if !matched { + return false + } + } + return true +} + // resolveSkill finds a skill by name, searching the specified scope or both scopes. func resolveSkill(reg *registry.Registry, name, scopeStr string) (*skill.Skill, string, skill.Scope, error) { if scopeStr != "" { diff --git a/internal/cli/skill_list.go b/internal/cli/skill_list.go index d9bd359..154ba09 100644 --- a/internal/cli/skill_list.go +++ b/internal/cli/skill_list.go @@ -12,9 +12,11 @@ import ( func newSkillListCmd() *cobra.Command { var ( - scope string - tag string - withPlatforms bool + scope string + tag string + categories []string + includeUntagged bool + withPlatforms bool ) cmd := &cobra.Command{ @@ -28,6 +30,11 @@ func newSkillListCmd() *cobra.Command { return err } + categoryFilters, err := parseCategoryFilters(categories) + if err != nil { + return err + } + var skillResults []output.SkillResult var discovered []registry.DiscoveredSkill @@ -88,6 +95,9 @@ func newSkillListCmd() *cobra.Command { if tag != "" && !hasTag(d.Skill.Tags, tag) { continue } + if !matchesCategories(d.Skill.Tags, categoryFilters, includeUntagged) { + continue + } r := toDiscoveredSkillResult(d) if files, err := skill.ListFiles(d.Path); err == nil && len(files) > 0 { r.Files = files @@ -146,6 +156,8 @@ func newSkillListCmd() *cobra.Command { cmd.Flags().StringVar(&scope, "scope", "all", "skill scope (user, project, or all)") cmd.Flags().StringVar(&tag, "tag", "", "filter skills by tag") + cmd.Flags().StringArrayVar(&categories, "category", nil, "filter by namespaced tag \"category:value\" (repeatable; comma-lists values; OR within a category, AND across categories)") + cmd.Flags().BoolVar(&includeUntagged, "include-untagged", false, "treat a skill with no tag in a requested category as matching that category") cmd.Flags().BoolVar(&withPlatforms, "with-platforms", false, "include the list of detected platforms each skill is installed on") return cmd diff --git a/internal/cli/skill_test.go b/internal/cli/skill_test.go index 8767737..ddc2be0 100644 --- a/internal/cli/skill_test.go +++ b/internal/cli/skill_test.go @@ -1010,6 +1010,217 @@ func TestSkillList_FilterByTag(t *testing.T) { assert.True(t, names["tool-c"]) } +// --- categorical-tag filter (#96) --- + +func TestParseCategoryFilters(t *testing.T) { + tests := []struct { + name string + raw []string + want map[string][]string + wantErr bool + }{ + {name: "empty", raw: nil, want: map[string][]string{}}, + {name: "single", raw: []string{"lang:python"}, want: map[string][]string{"lang": {"python"}}}, + { + name: "comma list folds into one namespace", + raw: []string{"lang:python,go"}, + want: map[string][]string{"lang": {"python", "go"}}, + }, + { + name: "repeated same namespace accumulates", + raw: []string{"lang:python", "lang:go"}, + want: map[string][]string{"lang": {"python", "go"}}, + }, + { + name: "distinct namespaces", + raw: []string{"lang:python", "topic:testing"}, + want: map[string][]string{"lang": {"python"}, "topic": {"testing"}}, + }, + { + name: "lowercased", + raw: []string{"Lang:Python"}, + want: map[string][]string{"lang": {"python"}}, + }, + {name: "no colon", raw: []string{"python"}, wantErr: true}, + {name: "empty namespace", raw: []string{":python"}, wantErr: true}, + {name: "empty value", raw: []string{"lang:"}, wantErr: true}, + {name: "empty value in comma list", raw: []string{"lang:python,"}, wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := parseCategoryFilters(tt.raw) + if tt.wantErr { + require.Error(t, err) + var ve *ValidationError + assert.ErrorAs(t, err, &ve, "malformed --category must be a ValidationError (exit code 2)") + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestMatchesCategories(t *testing.T) { + tests := []struct { + name string + tags []string + filters map[string][]string + includeUntagged bool + want bool + }{ + {name: "empty filter matches everything", tags: []string{"lang:go"}, filters: map[string][]string{}, want: true}, + {name: "single match", tags: []string{"lang:python"}, filters: map[string][]string{"lang": {"python"}}, want: true}, + {name: "single miss", tags: []string{"lang:go"}, filters: map[string][]string{"lang": {"python"}}, want: false}, + { + name: "OR within category", + tags: []string{"lang:go"}, + filters: map[string][]string{"lang": {"python", "go"}}, + want: true, + }, + { + name: "AND across categories satisfied", + tags: []string{"lang:python", "topic:testing"}, + filters: map[string][]string{"lang": {"python"}, "topic": {"testing"}}, + want: true, + }, + { + name: "AND across categories one missing value fails", + tags: []string{"lang:python", "topic:docs"}, + filters: map[string][]string{"lang": {"python"}, "topic": {"testing"}}, + want: false, + }, + { + name: "category absent fails by default", + tags: []string{"lang:python"}, + filters: map[string][]string{"topic": {"testing"}}, + want: false, + }, + { + name: "category absent passes with includeUntagged", + tags: []string{"lang:python"}, + filters: map[string][]string{"topic": {"testing"}}, + includeUntagged: true, + want: true, + }, + { + name: "includeUntagged still requires a present category to match", + tags: []string{"lang:go", "topic:docs"}, + filters: map[string][]string{"lang": {"python"}, "topic": {"docs"}}, + includeUntagged: true, + want: false, + }, + { + name: "zero tags fails by default", + tags: nil, + filters: map[string][]string{"lang": {"python"}}, + want: false, + }, + { + name: "zero tags passes with includeUntagged", + tags: nil, + filters: map[string][]string{"lang": {"python"}}, + includeUntagged: true, + want: true, + }, + { + name: "flat tag is not categorical", + tags: []string{"python", "lang:go"}, + filters: map[string][]string{"lang": {"python"}}, + want: false, + }, + { + name: "case-insensitive match", + tags: []string{"Lang:Python"}, + filters: map[string][]string{"lang": {"python"}}, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, matchesCategories(tt.tags, tt.filters, tt.includeUntagged)) + }) + } +} + +// TestSkillList_FilterByCategory covers the --json contract for the categorical +// filter end to end: OR within a category, AND across categories, and the +// strict-by-default untagged handling. +func TestSkillList_FilterByCategory(t *testing.T) { + cc := testRegistry(t) + + mk := func(name, desc, tags string) { + t.Helper() + _, err := runCmd(t, cc, "skill", "create", name, "--description", desc, "--tags", tags) + require.NoError(t, err) + } + mk("py-test", "Python testing", "lang:python,topic:testing") + mk("py-docs", "Python docs", "lang:python,topic:docs") + mk("go-test", "Go testing", "lang:go,topic:testing") + mk("untyped", "No categories", "misc") + + listNames := func(t *testing.T, args ...string) map[string]bool { + t.Helper() + out, err := runCmd(t, cc, append([]string{"skill", "list"}, args...)...) + require.NoError(t, err) + var result output.SkillListResult + require.NoError(t, json.Unmarshal([]byte(out), &result)) + assert.Equal(t, len(result.Skills), result.Count) + names := map[string]bool{} + for _, s := range result.Skills { + names[s.Name] = true + } + return names + } + + // Single category value. + got := listNames(t, "--category", "lang:python", "--json") + assert.Equal(t, map[string]bool{"py-test": true, "py-docs": true}, got) + + // OR within a category. + got = listNames(t, "--category", "lang:python,go", "--json") + assert.Equal(t, map[string]bool{"py-test": true, "py-docs": true, "go-test": true}, got) + + // AND across categories. + got = listNames(t, "--category", "lang:python", "--category", "topic:testing", "--json") + assert.Equal(t, map[string]bool{"py-test": true}, got) + + // Strict by default: a skill with no tag in the category is excluded. + got = listNames(t, "--category", "topic:testing", "--json") + assert.Equal(t, map[string]bool{"py-test": true, "go-test": true}, got) + + // --include-untagged: category-absent skills now match that category. + got = listNames(t, "--category", "topic:testing", "--include-untagged", "--json") + assert.Equal(t, map[string]bool{"py-test": true, "go-test": true, "untyped": true}, got) +} + +func TestSkillList_FilterByCategory_Invalid(t *testing.T) { + cc := testRegistry(t) + _, err := runCmd(t, cc, "skill", "create", "x", "--description", "X", "--tags", "lang:go") + require.NoError(t, err) + + _, err = runCmd(t, cc, "skill", "list", "--category", "python", "--json") + require.Error(t, err) + var ve *ValidationError + assert.ErrorAs(t, err, &ve) +} + +// TestSkillList_TagAndCategory confirms --tag and --category compose (AND). +func TestSkillList_TagAndCategory(t *testing.T) { + cc := testRegistry(t) + _, err := runCmd(t, cc, "skill", "create", "a", "--description", "A", "--tags", "featured,lang:go") + require.NoError(t, err) + _, err = runCmd(t, cc, "skill", "create", "b", "--description", "B", "--tags", "lang:go") + require.NoError(t, err) + + out, err := runCmd(t, cc, "skill", "list", "--tag", "featured", "--category", "lang:go", "--json") + require.NoError(t, err) + var result output.SkillListResult + require.NoError(t, json.Unmarshal([]byte(out), &result)) + require.Equal(t, 1, result.Count) + assert.Equal(t, "a", result.Skills[0].Name) +} + // TestSkillList_WithPlatforms verifies that --with-platforms enriches each // skill entry with the list of platforms where the skill is currently // installed, scoped to the registry skill's scope. From 86f6c8c8e36c4729d10ebc7d51c1396e773089bc Mon Sep 17 00:00:00 2001 From: devrimcavusoglu Date: Tue, 21 Jul 2026 10:04:08 +0300 Subject: [PATCH 2/4] Address review nits: align tag normalization, reject comma namespaces, dedup values, test text path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four non-blocking review nits from PR #98, all addressed: - hasTag now trims surrounding whitespace on stored tags, matching the normalization matchesCategories already applied — the two filters share one convention. - A comma in the category name (e.g. --category ",lang:python") is now a ValidationError instead of a silently never-matching namespace, with a hint that comma-separated value lists go after the colon. - Duplicate values within a namespace are deduplicated at parse time (lang:python,python → ["python"]). - Added a text-output (non-JSON) test for the category filter path, plus parser cases for the new error and dedup behavior and a hasTag normalization test. Co-Authored-By: Claude Fable 5 --- docs/reference/commands.md | 2 +- internal/cli/skill_helpers.go | 22 ++++++++++++++++------ internal/cli/skill_test.go | 29 +++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/docs/reference/commands.md b/docs/reference/commands.md index dca2fe2..ab42f01 100644 --- a/docs/reference/commands.md +++ b/docs/reference/commands.md @@ -186,7 +186,7 @@ skern skill list [--scope user|project|all] [flags] - **Repeatable, with comma-lists:** `--category lang:python --category lang:go` and `--category lang:python,go` are equivalent. - **OR within a category, AND across categories:** `--category lang:python,go --category topic:testing` matches skills tagged (`lang:python` **or** `lang:go`) **and** `topic:testing`. - **Strict by default:** a skill that carries no tag in a requested category is excluded. Pass `--include-untagged` to treat a category-absent skill as applying to all values of that category. A category the skill *does* declare must still match a requested value even with `--include-untagged`. -- Matching is case-insensitive. `--tag` and `--category` compose with AND. Malformed input (`--category value` with no colon, an empty category, or an empty value) exits with code 2. +- Matching is case-insensitive. `--tag` and `--category` compose with AND. Malformed input (`--category value` with no colon, an empty or comma-containing category name, or an empty value) exits with code 2. Also runs pairwise overlap detection across all listed skills and appends a "Potential duplicates" section when matches are found (score >= 0.6). In `--json` mode they appear in the `duplicates` array. diff --git a/internal/cli/skill_helpers.go b/internal/cli/skill_helpers.go index 1be9218..aac6018 100644 --- a/internal/cli/skill_helpers.go +++ b/internal/cli/skill_helpers.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "slices" "strings" "github.com/devrimcavusoglu/skern/internal/output" @@ -173,11 +174,13 @@ func formatSearchResults(query string, results []output.SkillResult) string { return b.String() } -// hasTag checks if a tag list contains the given tag (case-insensitive). +// hasTag checks if a tag list contains the given tag (case-insensitive, +// ignoring surrounding whitespace — the same normalization matchesCategories +// applies to stored tags). func hasTag(tags []string, tag string) bool { - t := strings.ToLower(tag) + t := strings.ToLower(strings.TrimSpace(tag)) for _, v := range tags { - if strings.ToLower(v) == t { + if strings.ToLower(strings.TrimSpace(v)) == t { return true } } @@ -190,8 +193,10 @@ func hasTag(tags []string, tag string) bool { // are lowercased so matching is case-insensitive, consistent with hasTag. // // Malformed input is a ValidationError (exit code 2): a value with no colon, -// an empty category name, or an empty value. Flat tags (no colon) are a -// different surface — they belong to --tag, not --category. +// an empty category name, a category name containing a comma (comma-separated +// value lists go after the colon), or an empty value. Flat tags (no colon) are +// a different surface — they belong to --tag, not --category. Duplicate values +// within a namespace are deduplicated. func parseCategoryFilters(raw []string) (map[string][]string, error) { filters := map[string][]string{} for _, entry := range raw { @@ -203,12 +208,17 @@ func parseCategoryFilters(raw []string) (map[string][]string, error) { if ns == "" { return nil, &ValidationError{Message: fmt.Sprintf("invalid --category %q: category name must not be empty", entry)} } + if strings.Contains(ns, ",") { + return nil, &ValidationError{Message: fmt.Sprintf("invalid --category %q: category name must not contain a comma (a comma-separated value list goes after the colon)", entry)} + } for _, v := range strings.Split(valStr, ",") { v = strings.ToLower(strings.TrimSpace(v)) if v == "" { return nil, &ValidationError{Message: fmt.Sprintf("invalid --category %q: value must not be empty", entry)} } - filters[ns] = append(filters[ns], v) + if !slices.Contains(filters[ns], v) { + filters[ns] = append(filters[ns], v) + } } } return filters, nil diff --git a/internal/cli/skill_test.go b/internal/cli/skill_test.go index ddc2be0..6955128 100644 --- a/internal/cli/skill_test.go +++ b/internal/cli/skill_test.go @@ -1041,8 +1041,14 @@ func TestParseCategoryFilters(t *testing.T) { raw: []string{"Lang:Python"}, want: map[string][]string{"lang": {"python"}}, }, + { + name: "duplicate values deduped", + raw: []string{"lang:python,python", "lang:Python"}, + want: map[string][]string{"lang": {"python"}}, + }, {name: "no colon", raw: []string{"python"}, wantErr: true}, {name: "empty namespace", raw: []string{":python"}, wantErr: true}, + {name: "comma in category name", raw: []string{",lang:python"}, wantErr: true}, {name: "empty value", raw: []string{"lang:"}, wantErr: true}, {name: "empty value in comma list", raw: []string{"lang:python,"}, wantErr: true}, } @@ -1143,6 +1149,14 @@ func TestMatchesCategories(t *testing.T) { } } +// hasTag and matchesCategories share one normalization convention: +// case-insensitive, surrounding whitespace ignored on stored tags. +func TestHasTag_TrimAndCase(t *testing.T) { + assert.True(t, hasTag([]string{" Featured "}, "featured")) + assert.True(t, hasTag([]string{"featured"}, " FEATURED ")) + assert.False(t, hasTag([]string{"feat"}, "featured")) +} + // TestSkillList_FilterByCategory covers the --json contract for the categorical // filter end to end: OR within a category, AND across categories, and the // strict-by-default untagged handling. @@ -1194,6 +1208,21 @@ func TestSkillList_FilterByCategory(t *testing.T) { assert.Equal(t, map[string]bool{"py-test": true, "go-test": true, "untyped": true}, got) } +// TestSkillList_FilterByCategory_TextOutput drives the text (non-JSON) +// rendering path with a category filter. +func TestSkillList_FilterByCategory_TextOutput(t *testing.T) { + cc := testRegistry(t) + _, err := runCmd(t, cc, "skill", "create", "py-skill", "--description", "Python skill", "--tags", "lang:python") + require.NoError(t, err) + _, err = runCmd(t, cc, "skill", "create", "go-skill", "--description", "Go skill", "--tags", "lang:go") + require.NoError(t, err) + + out, err := runCmd(t, cc, "skill", "list", "--category", "lang:python") + require.NoError(t, err) + assert.Contains(t, out, "py-skill") + assert.NotContains(t, out, "go-skill") +} + func TestSkillList_FilterByCategory_Invalid(t *testing.T) { cc := testRegistry(t) _, err := runCmd(t, cc, "skill", "create", "x", "--description", "X", "--tags", "lang:go") From d1e3d4c92dee8f5cd33f808dd3e97b3ddc0cb308 Mon Sep 17 00:00:00 2001 From: devrimcavusoglu Date: Tue, 21 Jul 2026 10:15:44 +0300 Subject: [PATCH 3/4] Enforce tag charset: alphanumeric segments joined by hyphens, single category colon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tags previously had no validation at all — any string passed through --tags or frontmatter was stored as-is. Now a tag must be alphanumeric segments joined by hyphens ("code-review"), optionally namespaced as "category:value" with a single colon ("lang:python", "topic:ci-cd"). Uppercase stays allowed since tag matching is case-insensitive. - skill.ValidateTag holds the rule; skill.Validate reports violations as errors (so `skern skill validate` catches hand-edited frontmatter). - `skill create --tags` enforces it up front as a ValidationError (exit code 2), mirroring the existing name check, and trims each tag so "a, b" comma-lists with spaces normalize cleanly. - `skill edit` has no tags flag, so create + validate cover every entry point. Tests: ValidateTag table (18 cases), create rejection e2e, and an e2e confirming hyphenated flat/categorical tags flow through --tag and --category filters with space-after-comma input. Docs updated in commands.md and skill-format.md. Co-Authored-By: Claude Fable 5 --- docs/concepts/skill-format.md | 2 +- docs/reference/commands.md | 2 +- internal/cli/skill_create.go | 9 +++++++++ internal/cli/skill_test.go | 29 ++++++++++++++++++++++++++ internal/skill/skill.go | 21 +++++++++++++++++++ internal/skill/skill_test.go | 38 +++++++++++++++++++++++++++++++++++ internal/skill/validator.go | 17 ++++++++++++++++ 7 files changed, 116 insertions(+), 2 deletions(-) diff --git a/docs/concepts/skill-format.md b/docs/concepts/skill-format.md index ce0c2d0..28ebe46 100644 --- a/docs/concepts/skill-format.md +++ b/docs/concepts/skill-format.md @@ -48,7 +48,7 @@ The main technique or pattern (before/after for techniques). |-------|----------|-------------| | `name` | Yes | Skill name matching `[a-z0-9]+([.-][a-z0-9]+)*`, 1-64 chars. Hyphens and dots are both valid separators (`code-review`, `myorg.bootstrap`). Must equal the directory name. | | `description` | Yes | What the skill does — start with "Use when…". Max 1024 chars. | -| `tags` | No | List of classification tags | +| `tags` | No | List of classification tags. Alphanumeric segments joined by hyphens (`code-review`), optionally namespaced as `category:value` with a single colon (`lang:python`, `topic:ci-cd`). Matching is case-insensitive. | | `allowed-tools` | No | Tools the skill may use. No empty entries. | | `metadata.author.name` | No | Author name | | `metadata.author.type` | No | `human` or `agent` | diff --git a/docs/reference/commands.md b/docs/reference/commands.md index ab42f01..3403578 100644 --- a/docs/reference/commands.md +++ b/docs/reference/commands.md @@ -81,7 +81,7 @@ skern skill create [flags] | `--author` | — | Author name | | `--author-type` | `human` | `human` or `agent` | | `--author-platform` | — | Platform name (e.g. `claude-code`) — used with `agent` author type | -| `--tags` | — | Comma-separated list of tags | +| `--tags` | — | Comma-separated list of tags. Each tag is alphanumeric segments joined by hyphens, optionally namespaced as `category:value` (single colon); anything else exits with code 2 | | `--version` | `0.0.1` | Initial semver version | | `--scope` | `user` | `user` or `project` | | `--force` | `false` | Bypass overlap block | diff --git a/internal/cli/skill_create.go b/internal/cli/skill_create.go index 33ee300..68160aa 100644 --- a/internal/cli/skill_create.go +++ b/internal/cli/skill_create.go @@ -38,6 +38,15 @@ func newSkillCreateCmd() *cobra.Command { return &ValidationError{Message: err.Error()} } + // Normalize --tags (tolerate space after commas) and enforce the + // tag charset up front, like the name check above. + for i, tg := range tags { + tags[i] = strings.TrimSpace(tg) + if err := skill.ValidateTag(tags[i]); err != nil { + return &ValidationError{Message: err.Error()} + } + } + scopeVal, err := parseScope(scope) if err != nil { return err diff --git a/internal/cli/skill_test.go b/internal/cli/skill_test.go index 6955128..af3a0b4 100644 --- a/internal/cli/skill_test.go +++ b/internal/cli/skill_test.go @@ -1250,6 +1250,35 @@ func TestSkillList_TagAndCategory(t *testing.T) { assert.Equal(t, "a", result.Skills[0].Name) } +// TestSkillCreate_InvalidTag enforces the tag charset at the create boundary: +// alphanumeric segments joined by hyphens, at most one category:value colon. +func TestSkillCreate_InvalidTag(t *testing.T) { + cc := testRegistry(t) + for _, bad := range []string{"my_tag", "my tag", "a:b:c", "-tag", "tag-", "c++"} { + _, err := runCmd(t, cc, "skill", "create", "x", "--description", "X", "--tags", bad) + require.Error(t, err, "tag %q should be rejected", bad) + var ve *ValidationError + assert.ErrorAs(t, err, &ve, "invalid tag must be a ValidationError (exit code 2)") + } +} + +// TestSkillList_HyphenatedTags confirms hyphenated tags work end to end through +// both filters, and that space after a comma in --tags is normalized away. +func TestSkillList_HyphenatedTags(t *testing.T) { + cc := testRegistry(t) + _, err := runCmd(t, cc, "skill", "create", "review-helper", "--description", "Review helper", + "--tags", "topic:code-review, this-is-another") + require.NoError(t, err) + + out, err := runCmd(t, cc, "skill", "list", "--category", "topic:code-review", "--tag", "this-is-another", "--json") + require.NoError(t, err) + var result output.SkillListResult + require.NoError(t, json.Unmarshal([]byte(out), &result)) + require.Equal(t, 1, result.Count) + assert.Equal(t, "review-helper", result.Skills[0].Name) + assert.Equal(t, []string{"topic:code-review", "this-is-another"}, result.Skills[0].Tags) +} + // TestSkillList_WithPlatforms verifies that --with-platforms enriches each // skill entry with the list of platforms where the skill is currently // installed, scoped to the registry skill's scope. diff --git a/internal/skill/skill.go b/internal/skill/skill.go index 12a373a..a4008cf 100644 --- a/internal/skill/skill.go +++ b/internal/skill/skill.go @@ -4,6 +4,7 @@ package skill import ( "fmt" "regexp" + "strings" ) // Scope represents where a skill is stored. @@ -56,6 +57,26 @@ type Skill struct { Body string `yaml:"-" json:"-"` } +// tagPartRegex validates one side of a tag: alphanumeric segments joined by +// hyphens. Uppercase is allowed because tag matching is case-insensitive. +var tagPartRegex = regexp.MustCompile(`^[a-zA-Z0-9]+(-[a-zA-Z0-9]+)*$`) + +// ValidateTag checks that a tag is either a flat tag ("code-review") or a +// categorical tag ("lang:python", "topic:ci-cd"): alphanumeric segments joined +// by hyphens, with at most one colon separating category and value. +func ValidateTag(tag string) error { + parts := strings.Split(tag, ":") + if len(parts) > 2 { + return fmt.Errorf("tag %q is invalid: at most one \":\" (separating category and value) is allowed", tag) + } + for _, p := range parts { + if !tagPartRegex.MatchString(p) { + return fmt.Errorf("tag %q is invalid: tags may only contain alphanumeric characters and hyphens (segments joined by hyphens, optionally as \"category:value\")", tag) + } + } + return nil +} + // ValidateName checks that a skill name matches the required pattern. func ValidateName(name string) error { if len(name) == 0 { diff --git a/internal/skill/skill_test.go b/internal/skill/skill_test.go index ed080d8..599aec0 100644 --- a/internal/skill/skill_test.go +++ b/internal/skill/skill_test.go @@ -51,3 +51,41 @@ func TestValidateName(t *testing.T) { }) } } + +func TestValidateTag(t *testing.T) { + tests := []struct { + name string + input string + wantErr bool + }{ + {"single word", "python", false}, + {"hyphenated", "this-is-a-tag", false}, + {"numbers", "web3", false}, + {"uppercase allowed (matching is case-insensitive)", "Featured", false}, + {"categorical", "lang:python", false}, + {"categorical hyphenated both sides", "code-topic:code-review", false}, + {"categorical uppercase", "Lang:Python", false}, + {"empty", "", true}, + {"underscore", "my_tag", true}, + {"space", "my tag", true}, + {"special chars", "c++", true}, + {"leading hyphen", "-tag", true}, + {"trailing hyphen", "tag-", true}, + {"double hyphen", "my--tag", true}, + {"two colons", "a:b:c", true}, + {"empty category", ":python", true}, + {"empty value", "lang:", true}, + {"bare colon", ":", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateTag(tt.input) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/internal/skill/validator.go b/internal/skill/validator.go index c7fecae..61cbc89 100644 --- a/internal/skill/validator.go +++ b/internal/skill/validator.go @@ -37,6 +37,7 @@ func Validate(s *Skill) []ValidationIssue { issues = append(issues, validateName(s.Name)...) issues = append(issues, validateDescription(s.Description)...) issues = append(issues, validateBody(s.Body)...) + issues = append(issues, validateTags(s.Tags)...) issues = append(issues, validateAllowedTools(s.AllowedTools)...) issues = append(issues, validateMetadata(s.Metadata)...) issues = append(issues, lintStyle(s)...) @@ -100,6 +101,22 @@ func validateBody(body string) []ValidationIssue { return nil } +// validateTags checks each tag against the tag charset rule. Surrounding +// whitespace is tolerated (the tag filters trim it too). +func validateTags(tags []string) []ValidationIssue { + var issues []ValidationIssue + for i, tag := range tags { + if err := ValidateTag(strings.TrimSpace(tag)); err != nil { + issues = append(issues, ValidationIssue{ + Field: "tags", + Severity: SeverityError, + Message: fmt.Sprintf("tags[%d]: %s", i, err), + }) + } + } + return issues +} + func validateAllowedTools(tools []string) []ValidationIssue { var issues []ValidationIssue for i, tool := range tools { From ea454cbfab20f322139b707dc448904d770b8abc Mon Sep 17 00:00:00 2001 From: devrimcavusoglu Date: Tue, 21 Jul 2026 10:20:39 +0300 Subject: [PATCH 4/4] Restrict tags to lowercase Uppercase is now rejected at write time so stored tags have a single canonical form, matching the lowercase-only rule nameRegex already applies to skill names. The tag *filters* stay case-insensitive: a query may be typed in any case (`--category TOPIC:Code-Review` matches a stored `topic:code-review`), and skills carrying legacy hand-edited uppercase tags still match. Only the write boundary is tightened. Co-Authored-By: Claude Fable 5 --- docs/concepts/skill-format.md | 2 +- docs/reference/commands.md | 2 +- internal/cli/skill_test.go | 11 ++++++++--- internal/skill/skill.go | 15 +++++++++------ internal/skill/skill_test.go | 6 ++++-- 5 files changed, 23 insertions(+), 13 deletions(-) diff --git a/docs/concepts/skill-format.md b/docs/concepts/skill-format.md index 28ebe46..368b315 100644 --- a/docs/concepts/skill-format.md +++ b/docs/concepts/skill-format.md @@ -48,7 +48,7 @@ The main technique or pattern (before/after for techniques). |-------|----------|-------------| | `name` | Yes | Skill name matching `[a-z0-9]+([.-][a-z0-9]+)*`, 1-64 chars. Hyphens and dots are both valid separators (`code-review`, `myorg.bootstrap`). Must equal the directory name. | | `description` | Yes | What the skill does — start with "Use when…". Max 1024 chars. | -| `tags` | No | List of classification tags. Alphanumeric segments joined by hyphens (`code-review`), optionally namespaced as `category:value` with a single colon (`lang:python`, `topic:ci-cd`). Matching is case-insensitive. | +| `tags` | No | List of classification tags. Lowercase alphanumeric segments joined by hyphens (`code-review`), optionally namespaced as `category:value` with a single colon (`lang:python`, `topic:ci-cd`). Filter matching is case-insensitive. | | `allowed-tools` | No | Tools the skill may use. No empty entries. | | `metadata.author.name` | No | Author name | | `metadata.author.type` | No | `human` or `agent` | diff --git a/docs/reference/commands.md b/docs/reference/commands.md index 3403578..1393630 100644 --- a/docs/reference/commands.md +++ b/docs/reference/commands.md @@ -81,7 +81,7 @@ skern skill create [flags] | `--author` | — | Author name | | `--author-type` | `human` | `human` or `agent` | | `--author-platform` | — | Platform name (e.g. `claude-code`) — used with `agent` author type | -| `--tags` | — | Comma-separated list of tags. Each tag is alphanumeric segments joined by hyphens, optionally namespaced as `category:value` (single colon); anything else exits with code 2 | +| `--tags` | — | Comma-separated list of tags. Each tag is lowercase alphanumeric segments joined by hyphens, optionally namespaced as `category:value` (single colon); anything else exits with code 2 | | `--version` | `0.0.1` | Initial semver version | | `--scope` | `user` | `user` or `project` | | `--force` | `false` | Bypass overlap block | diff --git a/internal/cli/skill_test.go b/internal/cli/skill_test.go index af3a0b4..d5a7d7b 100644 --- a/internal/cli/skill_test.go +++ b/internal/cli/skill_test.go @@ -1136,6 +1136,8 @@ func TestMatchesCategories(t *testing.T) { want: false, }, { + // New tags are validated lowercase, but the matcher stays + // case-insensitive so hand-edited frontmatter still matches. name: "case-insensitive match", tags: []string{"Lang:Python"}, filters: map[string][]string{"lang": {"python"}}, @@ -1150,7 +1152,9 @@ func TestMatchesCategories(t *testing.T) { } // hasTag and matchesCategories share one normalization convention: -// case-insensitive, surrounding whitespace ignored on stored tags. +// case-insensitive, surrounding whitespace ignored on stored tags. Stored tags +// are validated lowercase on write, so this covers hand-edited frontmatter and +// lets a query be typed in any case. func TestHasTag_TrimAndCase(t *testing.T) { assert.True(t, hasTag([]string{" Featured "}, "featured")) assert.True(t, hasTag([]string{"featured"}, " FEATURED ")) @@ -1251,10 +1255,11 @@ func TestSkillList_TagAndCategory(t *testing.T) { } // TestSkillCreate_InvalidTag enforces the tag charset at the create boundary: -// alphanumeric segments joined by hyphens, at most one category:value colon. +// lowercase alphanumeric segments joined by hyphens, at most one +// category:value colon. func TestSkillCreate_InvalidTag(t *testing.T) { cc := testRegistry(t) - for _, bad := range []string{"my_tag", "my tag", "a:b:c", "-tag", "tag-", "c++"} { + for _, bad := range []string{"my_tag", "my tag", "a:b:c", "-tag", "tag-", "c++", "Featured", "lang:Python"} { _, err := runCmd(t, cc, "skill", "create", "x", "--description", "X", "--tags", bad) require.Error(t, err, "tag %q should be rejected", bad) var ve *ValidationError diff --git a/internal/skill/skill.go b/internal/skill/skill.go index a4008cf..abffd2d 100644 --- a/internal/skill/skill.go +++ b/internal/skill/skill.go @@ -57,13 +57,16 @@ type Skill struct { Body string `yaml:"-" json:"-"` } -// tagPartRegex validates one side of a tag: alphanumeric segments joined by -// hyphens. Uppercase is allowed because tag matching is case-insensitive. -var tagPartRegex = regexp.MustCompile(`^[a-zA-Z0-9]+(-[a-zA-Z0-9]+)*$`) +// tagPartRegex validates one side of a tag: lowercase alphanumeric segments +// joined by hyphens, matching the shape rule nameRegex applies to names. +var tagPartRegex = regexp.MustCompile(`^[a-z0-9]+(-[a-z0-9]+)*$`) // ValidateTag checks that a tag is either a flat tag ("code-review") or a -// categorical tag ("lang:python", "topic:ci-cd"): alphanumeric segments joined -// by hyphens, with at most one colon separating category and value. +// categorical tag ("lang:python", "topic:ci-cd"): lowercase alphanumeric +// segments joined by hyphens, with at most one colon separating category and +// value. Uppercase is rejected so stored tags have a single canonical form; +// the tag *filters* remain case-insensitive, so a query may be typed in any +// case and skills with legacy hand-edited uppercase tags still match. func ValidateTag(tag string) error { parts := strings.Split(tag, ":") if len(parts) > 2 { @@ -71,7 +74,7 @@ func ValidateTag(tag string) error { } for _, p := range parts { if !tagPartRegex.MatchString(p) { - return fmt.Errorf("tag %q is invalid: tags may only contain alphanumeric characters and hyphens (segments joined by hyphens, optionally as \"category:value\")", tag) + return fmt.Errorf("tag %q is invalid: tags must be lowercase alphanumeric segments joined by hyphens, optionally as \"category:value\"", tag) } } return nil diff --git a/internal/skill/skill_test.go b/internal/skill/skill_test.go index 599aec0..7dd980f 100644 --- a/internal/skill/skill_test.go +++ b/internal/skill/skill_test.go @@ -61,11 +61,13 @@ func TestValidateTag(t *testing.T) { {"single word", "python", false}, {"hyphenated", "this-is-a-tag", false}, {"numbers", "web3", false}, - {"uppercase allowed (matching is case-insensitive)", "Featured", false}, {"categorical", "lang:python", false}, {"categorical hyphenated both sides", "code-topic:code-review", false}, - {"categorical uppercase", "Lang:Python", false}, {"empty", "", true}, + {"uppercase", "Featured", true}, + {"all caps", "FEATURED", true}, + {"uppercase in category", "Lang:python", true}, + {"uppercase in value", "lang:Python", true}, {"underscore", "my_tag", true}, {"space", "my tag", true}, {"special chars", "c++", true},