Keep skill metadata keys that follow a blank line in the block - #1061
PratikDhanave (PratikDhanave) wants to merge 1 commit into
Conversation
The metadata block regex captured ((?:[ \t]+\S.*\n?)+), whose repetition terminates at the first line that is not indented-then-non-space. A blank line is legal and common inside a YAML mapping block, so any metadata key after a blank line was silently dropped - inconsistent with a real YAML parser and with the sibling block-scalar parsing, which tolerate blank lines. Allow blank lines within the captured block (they are ignored by the per-key extraction); a de-dented line still ends the block.
github-actions
Bot
added
area:agent
There was a problem hiding this comment.
🟡 Changes recommended
CRLF-formatted files can still lose metadata keys after a blank line.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates skill frontmatter parsing to preserve metadata keys after blank lines.
Changes:
- Allows blank lines within metadata blocks.
- Adds regression coverage for keys following blank lines.
File summaries
| File | Summary |
|---|---|
agent/skills/fsskills/source.go |
Updates metadata block matching. |
agent/skills/fsskills/source_test.go |
Adds blank-line metadata coverage. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| frontmatterRegex = regexp.MustCompile(`(?ms)\A^---\s*$(.+?)^---\s*$`) | ||
| yamlKeyValueRegex = regexp.MustCompile(`(?m)^([\w-]+)\s*:\s*(?:["'](.+?)["']|(.+?))\s*$`) | ||
| yamlMetadataBlockRegex = regexp.MustCompile(`(?m)^metadata\s*:\s*$\n((?:[ \t]+\S.*\n?)+)`) | ||
| yamlMetadataBlockRegex = regexp.MustCompile(`(?m)^metadata\s*:\s*$\n((?:[ \t]+\S.*\n?|[ \t]*\n)+)`) |
github-actions
Bot
added
kind:code
|
Scope: internal-only (bug fix in unexported regex; no exported API change; parsed metadata values are user-visible output, but no new/changed public contract) Changed Go contract: Upstream evidence reviewed:
Result: findings reported (cross-SDK parity gap, not a Go-side defect) Finding: fix is Go-only; identical bug remains in .NET and PythonThe regex this PR fixes ( This is a good bug fix on its own, but to restore parity maintainers should port the equivalent regex fix upstream (e.g.
|
Problem
In
agent/skills/fsskills/source.go, the metadata block is captured by:The capture group requires every line to be
indentation + non-space. A blank line — legal and common inside a YAML mapping block — matches neither, so the+repetition stops at the first blank line and every metadata key after it is silently dropped. This is inconsistent with a real YAML parser and with the file's own block-scalar parsing, which tolerate blank lines.Example frontmatter:
parses to
{a:1}—bandcare lost.Fix
Allow blank lines within the captured block (
| [ \t]*\n). The per-key extraction (yamlIndentedKeyValueRegex) already ignores blank lines, and a de-dented (non-indented, non-blank) line still ends the block, so no following top-level key is swept in.Test
TestFileSource_MetadataWithBlankLine_KeepsAllKeysparses a metadata block with a blank line between keys and asserts all keys survive. Fails before the fix ({a:1}), passes after; existing metadata tests remain green.