Skip to content

Fix: close SKILL.md frontmatter on an own-line ---, not on any --- substring - #367

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/skill-md-frontmatter-line-anchored-delimiter
Open

Fix: close SKILL.md frontmatter on an own-line ---, not on any --- substring#367
AmaadMartin wants to merge 1 commit into
mainfrom
fix/skill-md-frontmatter-line-anchored-delimiter

Conversation

@AmaadMartin

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    Closes: #issue_number
    Related: #issue_number

  2. Or, if no issue exists, describe the change:
    Problem: parseFrontmatterYaml in core/src/skills/loader.ts located the SKILL.md frontmatter block with content.split('---'). String.prototype.split has no notion of lines, so the first --- anywhere after the opening delimiter — including one inside a frontmatter value — closed the block. Two user-visible failures:

  3. Bogus YAML error. A well-formed SKILL.md with description: "a --- b" truncates to description: "a and js-yaml reports unexpected end of the stream within a double quoted scalar (4:1), wrapped as Invalid YAML in frontmatter: …. The skill cannot be loaded, listed, or validated.

  4. Silent data corruption. When the truncation happens to leave valid YAML, the file loads with silently rewritten fields. ---\nname: t\ndescription: a --- b\nBody has no own-line closing delimiter at all, yet parsed "successfully" to {name: 't', description: 'a'}.

Reachable from every entry point that reads a SKILL.md: loadSkillFromDir, loadSkillFile, validateSkillDir, loadAllSkillsInDir (which logs Skipping invalid skill in '<dir>' and drops the skill) and loadSkillFromZipBuffer. The body side of this bug was already fixed in google#500 (markdown tables survive); the frontmatter side was not.

Solution: Match the leading block with one line-anchored pattern instead of splitting on a substring, so only a --- that occupies a line of its own closes the frontmatter — the contract the docs already describe and that Jekyll, gray-matter and python-frontmatter all implement:

const FRONTMATTER_BLOCK_PATTERN =
  /^---[ \t]*\r?\n([\s\S]*?\r?\n)?---[ \t]*(?:\r?\n|$)/;

Four statements replace the eleven-line split. Design notes:

  • [ \t]* after each --- preserves today's tolerance of a delimiter line with trailing spaces.
  • Making ([\s\S]*?\r?\n)? optional as a whole (rather than the newline optional inside it) is load-bearing: it lets ---\n---\n match while still forcing every candidate closing --- to sit immediately after a line break. The tempting ([\s\S]*?)(?:\r?\n)?--- reintroduces the bug for a value that ends a line with ---, which is why there is a dedicated test for it.
  • (?:\r?\n|$) lets the file end right after the closing delimiter (existing handles empty body test); \r?\n on both delimiters handles CRLF files.
  • No g flag, so the module-level regex carries no lastIndex state across calls. The lazy [\s\S]*? is followed by a required line break plus a literal, so scanning stays linear — no nested quantifiers, no ReDoS surface.
  • Group 1 is undefined only for the empty-frontmatter case, hence ?? ''; yaml.load('') returns undefined, which the existing non-mapping check already rejects, so that degenerate input keeps producing the same error as before.

No new dependency (js-yaml remains the only YAML parser), no signature change, no new public export, and both existing error strings are preserved verbatim.

Behaviour deltas (no previously-valid file becomes invalid):

Input Before After
--- inside a quoted/plain frontmatter value bogus Invalid YAML in frontmatter: …, or silent truncation parses correctly
No own-line closing ---, but a --- inside a value silently returned a truncated mapping SKILL.md frontmatter not properly closed with ---
Indented --- inside a frontmatter block scalar truncated the block stays in the frontmatter
---- (4+ dashes) as the opening line Invalid YAML in frontmatter: … SKILL.md frontmatter not properly closed with ---
Opening --- not followed by a line break (e.g. ---name: x) could parse via the substring split SKILL.md frontmatter not properly closed with ---

The last two rows change only which error is raised for input that was already an error.

Deliberately out of scope (each has its own queued task): the Invalid YAML in frontmatter: wrapping of the non-mapping error, the redundant second FrontmatterSchema.parse in loadSkillFile, and the identical content.split("---", 2) defect in adk-python's _parse_skill_md_content. This change touches only the delimiter-detection lines.

Collision check. Listed all 272 open PRs on the fork; 22 titles mention skills or frontmatter, and 9 of those touch core/src/skills/loader.ts (#282, #283, #284, #263, #262, #242, #239, #310, #361). Grepping every one of those diffs for split('---'), parseFrontmatterYaml and properly closed returns nothing — none of them lands this change. The nearest neighbour is #282 (narrowing the frontmatter error messages), which rewrites the post-parse error handling of the same function; it was written against an older main where that logic still lived in parseSkillMdContent, and it does not touch the delimiter lines. Branched from main (currently fcfd043) rather than stacking, because the two changes are line-disjoint and the delimiter fix should not have to wait on an error-message refactor.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

Ten new it() blocks in core/test/skills/loader_test.ts. No existing test was edited, renamed, weakened, skipped or deleted — the pre-existing suite (in particular handles tables in body, the google#500 body guard, and throws error if frontmatter is not properly closed) is the regression signal and still passes unchanged.

describe('parseSkillMdContent'): keeps a quoted frontmatter value containing --- (the reported repro), keeps a frontmatter value with --- mid-line, keeps a frontmatter value whose line ends with ---, parses frontmatter with CRLF line endings, accepts a closing delimiter with trailing whitespace, throws when the only --- after the frontmatter is inside a value, preserves a --- horizontal rule in the body, still rejects a frontmatter block with no content. Plus loads a skill whose description contains --- (loadSkillFromDir, real files under fs.mkdtemp) and loads a zipped skill whose description contains --- (loadSkillFromZipBuffer, the second independent caller).

npx vitest run --project unit:core core/test/skills/loader_test.ts
  Test Files  1 passed (1)
       Tests  55 passed (55)

Coverage of the new code, measured with --coverage.include='core/src/skills/loader.ts': 100% of the new lines and branches (both arms of !match, both arms of match[1] ?? '', the \r?\n and [ \t]* alternatives, and the $-terminated close via the pre-existing handles empty body). No new uncovered statement or branch appears anywhere in the file.

Proof the tests can fail. Coverage is not proof, so every new test was run against mutated source and confirmed to fail:

  1. Restored the pre-fix content.split('---') implementation verbatim — 7 of 10 fail:

    × keeps a quoted frontmatter value containing ---
      → Invalid YAML in frontmatter: unexpected end of the stream within a double quoted scalar (4:1)
    × keeps a frontmatter value with --- mid-line          → expected 'a' to be 'a --- b'
    × keeps a frontmatter value whose line ends with ---   → expected 'a' to be 'a ---'
    × parses frontmatter with CRLF line endings            → Invalid YAML in frontmatter: unexpected end of the stream within a double quoted scalar (4:1)
    × throws when the only --- after the frontmatter is inside a value → expected [Function] to throw an error
    × loadSkillFromDir > loads a skill whose description contains ---  → Invalid YAML in frontmatter: unexpected end of the stream within a double quoted scalar (4:1)
    × loadSkillFromZipBuffer > loads a zipped skill whose description contains --- → Invalid YAML in frontmatter: unexpected end of the stream within a double quoted scalar (4:1)
    
  2. The remaining three pin preserved behaviour, so each was killed with a mutation of the exact construct it covers:

    • dropped [ \t]* from the closing delimiter → × accepts a closing delimiter with trailing whitespace → SKILL.md frontmatter not properly closed with ---
    • made the group greedy ([\s\S]* instead of [\s\S]*?) → × preserves a --- horizontal rule in the body → Invalid YAML in frontmatter: expected a single document in the stream, but found more
    • changed the fallback to match[1] ?? '{}'× still rejects a frontmatter block with no content → expected […] to throw error including 'Invalid YAML in frontmatter: SKILL.md…' but got 'Invalid YAML in frontmatter: [\n {\n…'

    That last mutation initially survived a looser toThrow('Invalid YAML in frontmatter:') assertion, because the schema-validation failure is wrapped with the same prefix. The assertion was therefore written against the full message, Invalid YAML in frontmatter: SKILL.md frontmatter must be a YAML mapping — matching the exact-message style already used elsewhere in this file.

Integration: npx vitest run --project integration tests/integration/skills → 3 passed, 1 skipped, and one file (skills/script_js/agent_test.ts) fails in this sandbox because its beforeAll runs npm install with no network. That failure reproduces identically on unmodified main, so it is pre-existing and unrelated.

npm run lint passes. npm run format:check passes on both changed files. npm run ts:check reports no error in either changed file; it exits non-zero for pre-existing errors elsewhere in the test tree that reproduce identically on unmodified main.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

Against the built package, importing through the public entry point:

  1. npm ci && npm run build

  2. Create a scratch skill directory and write a SKILL.md whose description is "a --- b" and whose body contains both a markdown table and a --- horizontal rule:

    ---
    name: test-skill
    description: "a --- b"
    ---
    intro
    
    | Column 1 | Column 2 |
    |---|---|
    | Cell 1 | Cell 2 |
    
    ---
    
    more
    
  3. In a scratch script, import {validateSkillDir, loadAllSkillsInDir} from '@google/adk' and assert validateSkillDir(<skillDir>) returns [], loadAllSkillsInDir(<baseDir>) returns the skill, frontmatter.description === 'a --- b', and instructions is byte-identical to the body above.

Result with this change: all assertions hold (validateSkillDir=[] description="a --- b" body byte-identical). Re-running the same script against the pre-fix build fails at the first assertion with:

AssertionError: validateSkillDir problems: Invalid YAML in frontmatter: unexpected end of the stream within a double quoted scalar (4:1)

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

…bstring

parseFrontmatterYaml located the frontmatter block with content.split('---'),
which has no notion of lines: the first '---' anywhere after the opening
delimiter closed the block, including one inside a frontmatter value. A
SKILL.md with description: "a --- b" was rejected with a YAML syntax error,
and a file whose only '---' was inside a value parsed "successfully" into a
silently truncated mapping.

Match the block with a line-anchored pattern instead, so only a '---' that
occupies a line of its own closes the frontmatter. Both existing error
strings and the body handling (including the tables-in-body guard) are
unchanged.
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