Repository navigation
fix(skills): require a line of exactly --- as the SKILL.md frontmatter delimiter - #1136
Open
OsamaAnsar wants to merge 2 commits into
Open
OsamaAnsar wants to merge 2 commits into
OsamaAnsar wants to merge 2 commits into
Conversation
…r delimiter
parseFrontmatterYaml used startsWith('---') and split('---'). A UTF-8 byte
order mark before the opening delimiter made it throw 'SKILL.md must start
with YAML frontmatter (---)', and a '---' inside a value (for example
'description: Use A --- B') ended the frontmatter early, silently
truncating the value and leaking the rest into the body.
Strip a leading U+FEFF and match the opening line, frontmatter and closing
line with a single pattern, accepting LF and CRLF.
Contributor
|
Thanks for the heads-up @OsamaAnsar The changes fit together fine: your parsing fix plus my typed throws. Whichever lands second, I'm happy to do the rebase |
Resolve the conflict in core/src/skills/loader.ts with the typed errors from google#1129: keep the byte order mark and exact --- delimiter parsing, and throw InputValidationError from the two frontmatter checks. Assert the error class in the new error-path tests.
Author
|
Thanks @SHAI-KrishnaDobhal, and no rebase needed. #1129 landed first, so I merged |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
No existing issue found for this (searched open and closed issues and PRs for
SKILL.md,frontmatter,BOM,loader.ts).2. Or, if no issue exists, describe the change:
Problem:
parseFrontmatterYamlincore/src/skills/loader.tsfinds the frontmatter withcontent.startsWith('---')andcontent.split('---'). Two consequences, both reproduced by callingparseSkillMdContent:Out-File/Set-Content -Encoding UTF8; Node'sreadFilekeeps it as a leading U+FEFF) makes the loader throwSKILL.md must start with YAML frontmatter (---), even though the file starts with---.---inside the frontmatter ends it.description: Use A --- B for thingsis silently truncated toUse A, andB for thingsplus the real closing---leak into the skill body.LF and CRLF files without a BOM were already fine.
Solution:
The delimiter in the Agent Skills format is a line of exactly
---.parseFrontmatterYamlnow strips a leading U+FEFF, then matches the opening line, the frontmatter and the closing line with one pattern (FRONTMATTER_PATTERN; LF or CRLF, optional trailing spaces/tabs on the delimiter lines). The body is everything after the closing line, trimmed as before. The two existing error messages are unchanged, and an empty---/---pair still reports "Invalid YAML in frontmatter" (non-mapping) instead of "not properly closed".One behaviour change: a file whose only closing
---sits at the end of a value line (description: A test skill ---) used to load with a truncated value and now fails withSKILL.md frontmatter not properly closed with ---, because that is not a delimiter line.Testing Plan
Unit Tests:
Added 9 test cases to
core/test/skills/loader_test.ts(afrontmatter delimitersblock inparseSkillMdContent, plus aloadSkillFromDirtest that writes a BOM + CRLF SKILL.md to disk).Before/after, calling
parseSkillMdContenton the same inputs:With only the parser change in
loader.tsreverted (typed errors kept), 5 of the new tests fail (both BOM cases, the---inside a value, the value-line---case, and the on-disk BOM test); the other new cases pass either way because they pin behaviour that was already correct. With the change restoredcore/test/skillspasses (3 files, 84 passed, 1 skipped), andcore/test/skillspluscore/test/agentstogether pass (738 passed, 2 skipped).prettier --checkandeslintare clean on the touched files.I did not run the full unit suite for this change. On my Windows machine two unrelated tests (
core/test/utils/file_utils_test.ts,core/test/cli/utils/logs_test.ts) fail withEPERM: operation not permitted, symlink, also on an unmodifiedmain, so I left "All unit tests pass locally" unticked.Manual End-to-End (E2E) Tests:
Not run against a real agent. I exercised
parseSkillMdContentandloadSkillFromDironly.Checklist
Additional context
Overlap with #1129 (typed errors in the skill loader), which merged after this PR was opened: both changed
parseFrontmatterYaml. I mergedmaininto this branch and resolved the one conflict incore/src/skills/loader.tsby keeping both changes. The BOM and exact---delimiter parsing is this PR, and the two frontmatter checks throwInputValidationErroras #1129 introduced. Messages are unchanged, and the error-path tests now also assert the class. Againstmain, the diff inloader.tsis only the parser change.For reference, adk-python
skills/_utils.py(_parse_skill_md_content) also usescontent.startswith("---")andcontent.split("---", 2), so a---inside a value ends the frontmatter there too; this fix goes beyond parity with it. I did not test adk-python itself, and did not check how it handles a BOM.