Skip to content

fix(skill): accept YAML block scalars in SKILL.md frontmatter#76

Merged
andybons merged 2 commits into
mainfrom
fix/skill-frontmatter-block-scalars
Jul 13, 2026
Merged

fix(skill): accept YAML block scalars in SKILL.md frontmatter#76
andybons merged 2 commits into
mainfrom
fix/skill-frontmatter-block-scalars

Conversation

@andybons

Copy link
Copy Markdown
Contributor

Summary

The Agent-Skills SKILL.md frontmatter parser (skill/frontmatter.go) only
understood single-line key: value scalars. A YAML block scalar
description: | (literal) or description: > (folded), the standard way to
write a long multi-line description and valid YAML — was rejected with
unexpected indented line in frontmatter. Because discovery fails loudly on
the first Prompt, one skill with a block-scalar description takes the whole
session down.

Fix

parseFrontmatter now recognizes a block-scalar header (|/> with an
optional +/- chomping indicator) and collects the following more-indented
lines as the value: dedented, joined with newlines (literal) or spaces
(folded), ending at the next non-indented key or end of frontmatter. Trailing
blank lines are dropped. Description length limits still apply to the assembled
value. The explicit indentation-indicator form (|2) is intentionally out of
scope.

Testing

TestLoadBlockScalarDescription (red-verified before the fix) covers literal,
folded, strip-chomped, and block-value-before-another-key cases. Full skill
package tests pass; go build ./... and go vet ./skill/ clean.

The frontmatter parser only handled single-line `key: value` scalars, so a
`description: |` (literal) or `description: >` (folded) block — valid YAML,
and the common way to write a long multi-line description — was rejected with
"unexpected indented line in frontmatter", which fails skill discovery on the
first Prompt and takes the whole session down.

Parse block scalars: collect the following more-indented lines as the value,
dedented, joined with newlines (literal) or spaces (folded), ending at the
next non-indented key or end of frontmatter, with an optional +/- chomping
indicator accepted. Description length validation still applies to the
assembled value. The explicit indentation-indicator form ("|2") stays out of
scope. TDD: literal, folded, chomped, and block-before-next-key cases.
@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown
📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 2m 31s —— View job


Code review — PR #76: accept YAML block scalars in SKILL.md frontmatter

  • Read changed files (skill/frontmatter.go, skill/skill_test.go)
  • Read surrounding context (skill/skill.go package doc + validation)
  • Check parser correctness (dedent, folded/literal join, chomping, dup keys)
  • Build & run skill tests — ok, go vet ./skill/ clean
  • Post review

Solid, well-scoped fix. The parser is careful: the min(indent, leading) dedent can't slice into real characters, trailing blanks are stripped, the folded/literal joins match the common cases, duplicate-key detection still fires, and the block content stays safely inside the frontmatter fence (splitFrontmatter only treats a non-indented --- as the closing delimiter, and block content is always indented). Tests are red-verified per the PR description and cover literal, folded, strip-chomped, and block-before-next-key. Nice.

One real issue and two minor notes.

🟠 The package doc now contradicts the code (skill/skill.go:28-33)

The doc comment still declares block scalars deliberately unsupported:

// Deliberately unsupported YAML constructs (any use is an error or is treated
// as a plain string, never interpreted): block scalars (| and >), flow
// collections ([a, b] / {k: v}), multi-line values, anchors/aliases, tags, ...

This PR makes | and > first-class, so this paragraph — and the "multi-line values" mention right after it — is now false. Since the package deliberately hand-rolls its YAML subset and documents exactly what it supports, the doc is the spec here; leaving it stale will mislead the next reader/reviewer into thinking block scalars aren't handled. Please move block scalars (| and >) (and "multi-line values") out of the unsupported list and add a bullet to the supported-constructs section (~lines 20-26), e.g. "literal (|) / folded (>) block scalars with optional -/+ chomping, dedented and joined; explicit indentation indicators like |2 are out of scope."

Fix this →

🟡 Minor — + (keep) chomping is accepted but not honored (frontmatter.go:114-171)

blockScalarIndicator accepts +, but parseBlockScalar unconditionally strips trailing blank lines (lines 152-154), so |+/>+ behave identically to clip/strip. That's harmless for skill descriptions (no one wants trailing newlines in a description), and the PR scopes chomping out — just flagging that the code advertises acceptance of a form it doesn't fully implement. Either honor keep, or drop + from the accepted set so an author isn't misled. A one-line comment noting "+ is accepted but treated as clip" would also suffice.

🟡 Minor — optional test for the block-scalar → length-limit path

The PR says "Description length limits still apply to the assembled value," which is true (the block value flows through fields["description"] into validate). Consider one small test asserting that a block scalar whose joined length exceeds 1024 runes is rejected — it locks in that a block scalar can't smuggle past the length guard, which is the exact class of regression this parser branch could introduce later.


Nothing here is blocking correctness; the fix itself is good. The doc update is the one I'd want before merge so the package's self-description stays accurate.

…h test

Address PR #76 review:
- The package doc listed block scalars and multi-line values as deliberately
  unsupported, which the parser now contradicts. Move them to the supported-
  constructs list with the exact semantics (dedent, literal/folded join, |2 out
  of scope, + normalized to clip); the package doc is this package's spec.
- Note at blockScalarIndicator that the +/- chomping indicator is accepted for
  a valid header but + (keep) is normalized to clip, matching parseBlockScalar.
- Add TestLoadBlockScalarDescriptionLengthLimit: a block scalar whose joined
  value exceeds 1024 runes is still rejected, locking the value through the
  same length guard as inline scalars.
@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown

Claude finished @andybons's task in 1m 39s —— View job


Code review — PR #76: accept YAML block scalars in SKILL.md frontmatter

  • Read changed files & surrounding context
  • Verify previous review findings were addressed
  • Check parser correctness (dedent, folded/literal, chomping, length limit)
  • Build & run skill tests — ok, go vet ./skill/ clean
  • Post review

Re-review of the follow-up commit (docs(skill): document block-scalar support…). All three items from the prior round are resolved, and I found no new issues.

Prior findings — all addressed:

  1. 🟠 Stale package doc — Fixed. skill/skill.go:25-30 now documents literal/folded block scalars as a supported construct, and block scalars / multi-line values are removed from the "Deliberately unsupported" list (lines 34-38). The doc-as-spec is accurate again.
  2. 🟡 + chomping accepted but not honored — Addressed via documentation. blockScalarIndicator (frontmatter.go:109-115) and the package doc both now state plainly that + is accepted for header validity but normalized to clip since trailing blanks are always dropped. Reader is no longer misled.
  3. 🟡 Length-limit test — Added. TestLoadBlockScalarDescriptionLengthLimit (skill_test.go) asserts a joined block value of 1201 runes is rejected with a description-naming error, locking in that a block scalar can't smuggle past the 1024-rune guard.

Fresh pass on the parser — still clean:

  • Dedent is safe: line[min(indent, leading):] (frontmatter.go:152) can never slice into real characters when a continuation line is less-indented than the first.
  • Folded join (157-170) correctly emits a newline for blank lines and a single space between consecutive non-blank lines; literal join uses \n. Trailing blanks stripped before joining.
  • Block content is always indented, so splitFrontmatter (which only treats a non-indented --- as the closing fence) can't be fooled into ending the frontmatter early.
  • Duplicate-key detection still fires on the block-scalar path (92-95).

Build/test: go build ./... clean, go vet ./skill/ clean, go test ./skill/ok.

LGTM. Nothing blocking; ready to merge from my side.

@andybons andybons merged commit 55429b0 into main Jul 13, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant