Skip to content

fix(skills): close SKILL.md frontmatter on a delimiter line - #7743

Closed
lisry-1 wants to merge 1 commit into
google:mainfrom
lisry-1:fix/skill-frontmatter-delimiter
Closed

lisry-1 wants to merge 1 commit into
google:mainfrom
lisry-1:fix/skill-frontmatter-delimiter

Conversation

@lisry-1

@lisry-1 lisry-1 commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes #7725.

_parse_skill_md_content split SKILL.md with content.split("---", 2), so the
frontmatter ended at the first occurrence of --- anywhere in the file
rather than at the closing delimiter line. Any --- inside a frontmatter value
therefore truncated the frontmatter:

  • description: Compare two options --- pros, cons and a verdict. loaded as
    Compare two options, and the tail plus the real closing delimiter leaked
    into instructions. The model then chooses skills from a truncated
    description.
  • description: "Draft release notes --- never publish them." failed to load
    with Invalid YAML in frontmatter: while scanning a quoted scalar, although
    the file is valid.
  • description: | block scalars holding a --- rule line dropped every key
    after them (license, metadata, allowed-tools, including
    metadata.adk_additional_tools) silently, since the parser never saw them.

Because directory, zip, GCS and SkillToolset loading all go through this one
function, every loader was affected.

The fix

_split_frontmatter now locates the delimiters by line, which is how
YAML itself delimits a document:

  • The file must open with a line holding only --- (trailing whitespace
    tolerated).
  • The frontmatter closes at the first unindented line holding only ---,
    so a --- indented inside a block scalar stays content, per YAML's rule that
    block content is indented past its key.
  • Line breaks are recognised as \r\n, \r or \n, so a file saved with
    foreign line endings parses exactly like an LF one.

Error messages are unchanged, so callers matching on them keep working, and the
public load_skill_* API and Frontmatter model are untouched.

Testing plan

Unit tests added in tests/unittests/skills/test__utils.py:

Test Covers
test_frontmatter_value_may_contain_delimiter (3 cases) unquoted, quoted and trailing --- inside a value
test_frontmatter_block_scalar_may_contain_delimiter_line description: | with a --- line, keys after it preserved
test_body_may_start_with_delimiter_line a body opening with a Markdown rule (no-regression guard)
test_frontmatter_tolerates_foreign_line_endings (\r\n, \r) CRLF and CR files
test_parse_skill_md_content_requires_the_delimiter_lines missing opening / closing delimiter still raises, same messages
test__load_skill_from_zip_bytes_keeps_delimiter_in_value the same defect through the archive loader

Before/after evidence (same test selection, source reverted vs. patched):

revert control:  6 failed, 2 passed
with the fix:    9 passed

Full suites, Windows 11 / Python 3.13, runtime deps only:

tests/unittests/skills:   13 failed, 91 passed  ->  13 failed, 100 passed

The 13 failures are identical before and after the change (verified with a
sorted failure-set diff). They are environment-bound: 12 GCS tests need
google-cloud-storage, and test__load_skill_from_dir compares file:// URI
spellings that differ on Windows.

tests/unittests/tools/test_skill_toolset.py + test_skill_path_traversal.py:
5 failed, 336 passed; the 5 failures are shell-symlink tests that cannot run
in this sandbox, unrelated to frontmatter parsing.

Lint/format: pyink --check, isort --check-only, ruff check all clean on
the two touched files.

Notes for reviewers

  • Only the parsing boundary changed; no public signature, model or error string
    moved.
  • I deliberately did not touch two adjacent gaps, to keep this PR to one
    concern: a UTF-8 BOM before --- still fails to load, and a lone
    ---name: x header (no newline) is now rejected rather than mis-parsed.
    Happy to file follow-ups if you want them.

@google-cla

google-cla Bot commented Oct 26, 2026

Copy link
Copy Markdown

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.

…so a

delimiter inside a value ended the frontmatter early: a description was cut
short, a quoted value failed to parse, and every key after a `|` block scalar
was dropped. Locate the delimiter by line instead, as YAML does, and accept
CRLF and CR line breaks.

Fixes google#7725.
@lisry-1
lisry-1 force-pushed the fix/skill-frontmatter-delimiter branch from bb9d98c to faaa549 Compare October 26, 2026 15:13
@lisry-1

lisry-1 commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Closing this as superseded: #7726 landed the same fix on main in 25105b4 ("fix: match SKILL.md frontmatter delimiters as whole lines"), which is why this branch now conflicts. No action needed on my side.

For the record, I verified the landed implementation against the cases I had written tests for — a --- inside an unquoted and a quoted value, a --- line indented inside a | block scalar, a body that opens with a --- rule, and CRLF/CR files — and all of them pass on main, so there is nothing this branch would add beyond 25105b4.

Thanks @alphasafal for the quick turnaround on #7725.

@lisry-1 lisry-1 closed this Oct 27, 2026
@lisry-1
lisry-1 deleted the fix/skill-frontmatter-delimiter branch October 27, 2026 09:31
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.

SKILL.md frontmatter is cut at the first "---" inside a value (description truncated or skill fails to load)

2 participants