Repository navigation
fix(skills): match SKILL.md frontmatter delimiters as whole lines - #7726
alphasafal wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
_parse_skill_md_content split the file on the first "---" anywhere, so a "---" inside a frontmatter value ended the frontmatter early. An unquoted description was cut short and the rest leaked into the instructions, a quoted one failed to load with "Invalid YAML in frontmatter", and keys after a block scalar were dropped. Match the opening and closing "---" as whole lines instead.
7c05c2b to
7792c76
Compare
BichengWang
left a comment
BichengWang
left a comment
There was a problem hiding this comment.
Reviewed 7792c76. I reproduced all three new description cases failing with the base parser, then ran the skills tests excluding GCS: 92 passed, 16 deselected. I also checked directory and ZIP loading with CRLF, an EOF closing delimiter, trailing delimiter whitespace, and a horizontal rule in the body; all preserved the description and instructions. Malformed closing delimiters still raise the expected error.
No blocking findings. The line anchors correctly leave indented --- inside a block scalar alone. I couldn't run the GCS tests because the optional storage dependency isn't available in the offline cache; I didn't run the full suite.
|
Thanks @BichengWang for the thorough review, especially the extra CRLF, EOF-delimiter and ZIP checks. To close the gap you mentioned, I ran the full skills suite with |
Merge #7726 A `---` inside a frontmatter value no longer ends the frontmatter. The closing delimiter must now be a line of only `---` (trailing spaces allowed): a SKILL.md whose closing `---` has other text on the same line, which was previously accepted with that text moved into the instructions, now raises ValueError. Fixes #7725 PiperOrigin-RevId: 1007645341
|
Thank you @alphasafal for your contribution! 🎉 Your changes have been successfully imported and merged via Copybara in commit 25105b4. Closing this PR as the changes are now in the main branch. |
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):
Problem:
_parse_skill_md_contentsplit SKILL.md withcontent.split("---", 2), so the frontmatter ended at the first---anywhere, including inside a value. An unquoteddescription: Compare A --- Bwas cut toCompare Awith the rest leaking into the instructions, a quoted one failed withInvalid YAML in frontmatter, and keys after a|block scalar containing a---line were dropped.Solution:
Match the opening and closing delimiters as whole
---lines with a module-level regex (_FRONTMATTER_PATTERN). The existing checks and error messages are unchanged: a file not starting with---and a frontmatter with no closing---line raise the sameValueErrors as before. CRLF line endings are handled. A closing---with other text after it on the same line is no longer accepted as the delimiter, which matches how YAML frontmatter is normally delimited.Testing Plan
Unit Tests:
tests/unittests/skills, not the full suite).New tests in
tests/unittests/skills/test__utils.py:test__load_skill_from_dir_dashes_inside_frontmatter_value(unquoted, quoted and block-scalar values containing---): all three fail onmainand pass with this change.test__load_skill_from_dir_crlf_frontmatter: guards Windows line endings.isort 8.0.1 and pyink 25.12 report no changes on the edited files.
Manual End-to-End (E2E) Tests:
The reproduction from #7725 with
google.adk.skills.load_skill_from_dir:Checklist
Additional context
All skill loaders go through this function (
load_skill_from_dir, zip loading, GCS loading), so the fix covers each of them.