Skip to content

doesn't handle non-string metadata values correctly. #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
masukomi wants to merge 2 commits into from
+79 −18
Closed

doesn't handle non-string metadata values correctly. #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f6470b7
metadata value processing was too restrictive
masukomi Mar 21, 2026
3ef5316
added a test to ensure metadata with float value considered valid
masukomi Mar 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions skill/skill.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ var _ yaml.Unmarshaler = (*AllowedTools)(nil)

// Frontmatter represents the parsed YAML frontmatter of a SKILL.md file.
type Frontmatter struct {
Name string `yaml:"name"`
Description string `yaml:"description"`
License string `yaml:"license"`
Compatibility string `yaml:"compatibility"`
Metadata map[string]string `yaml:"metadata"`
AllowedTools AllowedTools `yaml:"allowed-tools"`
Name string `yaml:"name"`
Description string `yaml:"description"`
License string `yaml:"license"`
Compatibility string `yaml:"compatibility"`
Metadata map[string]any `yaml:"metadata"`
Copy link
Copy Markdown
Member

@dacharyc dacharyc Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line you cite as rationale for this becoming any is not the complete picture. In the expanded metadata field description, the list contains this bullet:

A map from string keys to string values

So if you're encountering non-string values in the metadata field, they're correctly failing validation.

AllowedTools AllowedTools `yaml:"allowed-tools"`
}

// AllowedTools handles the type ambiguity in the allowed-tools field.
Expand Down
26 changes: 24 additions & 2 deletions skill/skill_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,28 @@ func TestLoad(t *testing.T) {
}
})

t.Run("metadata with sequence value parses without error", func(t *testing.T) {
dir := t.TempDir()
content := "---\nname: test\ndescription: desc\nmetadata:\n author: alice\n tags: [foo, bar, baz]\n---\nBody\n"
if err := os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(content), 0o644); err != nil {
t.Fatal(err)
}
s, err := Load(dir)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if s.Frontmatter.Metadata["author"] != "alice" {
t.Errorf("metadata[author] = %v, want %q", s.Frontmatter.Metadata["author"], "alice")
}
tags, ok := s.Frontmatter.Metadata["tags"].([]any)
if !ok {
t.Fatalf("metadata[tags] = %T, want []any", s.Frontmatter.Metadata["tags"])
}
if len(tags) != 3 {
t.Errorf("metadata[tags] len = %d, want 3", len(tags))
}
})

t.Run("metadata parsing", func(t *testing.T) {
dir := t.TempDir()
content := "---\nname: test\ndescription: desc\nmetadata:\n author: alice\n version: \"1.0\"\n---\nBody\n"
Expand All @@ -248,10 +270,10 @@ func TestLoad(t *testing.T) {
t.Fatalf("unexpected error: %v", err)
}
if s.Frontmatter.Metadata["author"] != "alice" {
t.Errorf("metadata[author] = %q, want %q", s.Frontmatter.Metadata["author"], "alice")
t.Errorf("metadata[author] = %v, want %q", s.Frontmatter.Metadata["author"], "alice")
}
if s.Frontmatter.Metadata["version"] != "1.0" {
t.Errorf("metadata[version] = %q, want %q", s.Frontmatter.Metadata["version"], "1.0")
t.Errorf("metadata[version] = %v, want %q", s.Frontmatter.Metadata["version"], "1.0")
}
})
}
Expand Down
17 changes: 10 additions & 7 deletions structure/frontmatter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,20 +68,23 @@ func CheckFrontmatter(s *skill.Skill, opts Options) []types.Result {

// Check optional metadata
if s.RawFrontmatter["metadata"] != nil {
// Verify it's a map[string]string
// Verify it's a map whose values are non-nil, non-empty
if m, ok := s.RawFrontmatter["metadata"].(map[string]any); ok {
allStrings := true
valid := true
for k, v := range m {
if _, ok := v.(string); !ok {
results = append(results, ctx.Errorf("metadata[%q] value must be a string", k))
allStrings = false
if v == nil {
results = append(results, ctx.Errorf("metadata[%q] value must not be null", k))
valid = false
} else if s, ok := v.(string); ok && s == "" {
results = append(results, ctx.Errorf("metadata[%q] value must not be empty", k))
valid = false
}
}
if allStrings {
if valid {
results = append(results, ctx.Passf("metadata: (%d entries)", len(m)))
}
} else {
results = append(results, ctx.Error("metadata must be a map of string keys to string values"))
results = append(results, ctx.Error("metadata must be a map"))
}
}

Expand Down
42 changes: 39 additions & 3 deletions structure/frontmatter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,20 +373,56 @@ func TestCheckFrontmatter_Metadata(t *testing.T) {
requireResultContaining(t, results, types.Pass, "metadata: (2 entries)")
})

t.Run("metadata with non-string value", func(t *testing.T) {
t.Run("metadata with integer value", func(t *testing.T) {
s := makeSkill("/tmp/my-skill", "my-skill", "desc")
s.RawFrontmatter["metadata"] = map[string]any{
"count": 42,
}
results := CheckFrontmatter(s, Options{})
requireResultContaining(t, results, types.Error, "metadata[\"count\"] value must be a string")
requireResultContaining(t, results, types.Pass, "metadata: (1 entries)")
})

t.Run("metadata with float value", func(t *testing.T) {
s := makeSkill("/tmp/my-skill", "my-skill", "desc")
s.RawFrontmatter["metadata"] = map[string]any{
"score": 3.14,
}
results := CheckFrontmatter(s, Options{})
requireResultContaining(t, results, types.Pass, "metadata: (1 entries)")
})

t.Run("metadata with sequence value", func(t *testing.T) {
s := makeSkill("/tmp/my-skill", "my-skill", "desc")
s.RawFrontmatter["metadata"] = map[string]any{
"tags": []any{"foo", "bar"},
}
results := CheckFrontmatter(s, Options{})
requireResultContaining(t, results, types.Pass, "metadata: (1 entries)")
})

t.Run("metadata with null value", func(t *testing.T) {
s := makeSkill("/tmp/my-skill", "my-skill", "desc")
s.RawFrontmatter["metadata"] = map[string]any{
"key": nil,
}
results := CheckFrontmatter(s, Options{})
requireResultContaining(t, results, types.Error, "metadata[\"key\"] value must not be null")
})

t.Run("metadata with empty string value", func(t *testing.T) {
s := makeSkill("/tmp/my-skill", "my-skill", "desc")
s.RawFrontmatter["metadata"] = map[string]any{
"key": "",
}
results := CheckFrontmatter(s, Options{})
requireResultContaining(t, results, types.Error, "metadata[\"key\"] value must not be empty")
})

t.Run("metadata not a map", func(t *testing.T) {
s := makeSkill("/tmp/my-skill", "my-skill", "desc")
s.RawFrontmatter["metadata"] = "not a map"
results := CheckFrontmatter(s, Options{})
requireResult(t, results, types.Error, "metadata must be a map of string keys to string values")
requireResult(t, results, types.Error, "metadata must be a map")
})
}

Expand Down
Loading