doesn't handle non-string metadata values correctly. #40
doesn't handle non-string metadata values correctly. #40masukomi wants to merge 2 commits intoagent-ecosystem:mainfrom
Conversation
metadata is 'Arbitrary key-value mapping for additional metadata.' it can be arrays, strings, number, whatever.
masukomi
force-pushed
the
inline_yaml_array_handling
branch
from
095edb5 to
3ef5316
Compare
dacharyc
left a comment
dacharyc
left a comment
There was a problem hiding this comment.
I appreciate the PR, @masukomi - thanks for making the effort on something that looked like a bug fix! But I think your citation of the spec permitting "any" as a metadata field value is based on this frontmatter summary table in the spec; there's more info further down the spec about what the field should contain.
I think skill-validator is correctly enforcing this as a map[string]string, so I'm going to go ahead and close this. But I do appreciate your time and contribution.
| Description string `yaml:"description"` | ||
| License string `yaml:"license"` | ||
| Compatibility string `yaml:"compatibility"` | ||
| Metadata map[string]any `yaml:"metadata"` |
There was a problem hiding this comment.
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.
2 participants
metadata is officially defined as
Thus it can be arrays, strings, number, whatever.
The processing of it was unfairly restrictive.
This PR Fixes that HOWEVER it does introduce one opinionated restriction. This would cause an error of the value of a metadata field was an empty string or null. A null or empty string value wouldn't technically be an error according to the spec but it seems like a bad practice to me.
Let me know if you want me to change this.