Conversation
`_parse_frontmatter` split every frontmatter line on the first `:`, so a `>` folded or `|` literal block scalar became the value `">"` or `"|"`, its indented continuation lines were parsed as extra top-level keys, and a plain scalar wrapped across lines lost everything after the first line. The skill index then showed the model `- triage: >` instead of the description, with nothing to match a task against and no warning. Teach the parser block scalars, including the `-` and `+` chomping indicators, and indented continuation lines of plain scalars. The parsed values now agree with `yaml.safe_load` on the shapes skill frontmatter uses; PyYAML stays out of the runtime dependencies. A block scalar body must be indented past its key, so a keyless `>` no longer consumes the line that follows it. Descriptions are collapsed to a single line when the skill index is rendered, so a `|` description cannot split one skill entry across several list lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HYuwKi4xwDVSptmiipM5LL
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7286791a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| metadata[parsed_key] = parsed_value | ||
| parsed_value, index = _read_wrapped_plain(body, index, parsed_value, key_indent) | ||
|
|
||
| metadata[parsed_key] = _unquote(parsed_value) |
There was a problem hiding this comment.
When valid frontmatter uses a stripped block such as description: |- whose body is "Use for triage", _read_block_scalar correctly returns the quotes as literal block content, but the unconditional _unquote call removes them. Skills.instructions() therefore exposes a different description to the model than the YAML specifies; only inline quoted scalars should be unquoted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1f8fc54. yaml.safe_load returns '"Use for triage"' for a |- body of "Use for triage", and the parser was returning Use for triage. _unquote now runs only on the inline branch, so block scalar content keeps its quotes. Worth noting it only bit the stripped indicators — with | or > the clip newline made the first/last character check fail, so the quotes survived by accident. Covered for all four indicators in test_quotes_inside_a_block_scalar_are_content.
| while index < len(lines): | ||
| line = lines[index] | ||
| if line.strip() == "" or _indent_width(line) <= key_indent: | ||
| break |
There was a problem hiding this comment.
For a valid wrapped plain description containing a blank line, such as description: Use for triage.\n\n Avoid PR review., YAML retains the later indented text as part of the scalar, but this branch stops at the blank line and the outer parser subsequently discards Avoid PR review.. The rendered skill index therefore loses part of the description, potentially omitting an important trigger or non-goal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1f8fc54. description: Use for triage.\n\n Avoid PR review. gave Use for triage. against yaml.safe_load's Use for triage.\nAvoid PR review. — the continuation line was dropped entirely, since the outer loop then skipped it for having no :. _read_wrapped_plain now folds blank lines into breaks. It only consumes them when an indented continuation actually follows, so a blank line before the next key still ends the scalar — test_wrapped_plain_scalar_stops_at_a_blank_line_before_a_new_key pins that side.
| if end_index is None: | ||
| return {} | ||
|
|
||
| body = lines[1:end_index] |
There was a problem hiding this comment.
When a valid literal or folded description contains an indented --- line, the preliminary delimiter scan treats that content line as the end of frontmatter, so the newly constructed body omits the marker, every subsequent description line, and any later metadata keys. For example, description: |\n first\n ---\n last\nname: triage should retain first\n---\nlast, but the skill index receives only first; the closing delimiter must be recognized only at the frontmatter's top level.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1f8fc54. Your example returned {"description": "first\n"} and lost name completely. The closing scan now matches on line.rstrip() == "---", so only a column-zero marker closes the frontmatter and an indented one stays block content. Pinned in test_indented_document_marker_stays_inside_a_block_scalar.
| elif folded and folded[-1] not in {"", "\n"} and not entry.startswith(" "): | ||
| folded.append(" " + entry) | ||
| else: | ||
| folded.append(entry) |
There was a problem hiding this comment.
For a valid folded scalar such as description: >\n first\n indented\n last, YAML preserves line breaks around the more-indented line and returns first\n indented\nlast\n, whereas this loop concatenates it into first indented last\n. Although Skills.instructions() later normalizes whitespace, the exported LocalDirLazySkillSource.list_skill_metadata() returns this incorrectly reshaped description directly, so callers do not receive the YAML value represented by the skill.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1f8fc54. That case returned first indented last\n against yaml.safe_load's first\n indented\nlast\n. Agreed on why it matters past the index — list_skill_metadata() hands the description to callers directly, so normalizing in instructions() does not cover it. The folded branch now tracks whether each line is blank, more-indented, or normal, and inserts a break around any more-indented run rather than folding it. Covered by test_folded_scalar_keeps_breaks_around_a_more_indented_line.
Follow-up to review feedback on the block scalar parser. Quotes inside a block scalar are content, not a quoted scalar. `_unquote` ran on every value, so a stripped block whose body was `"Use for triage"` lost its quotes; only an inline scalar is unquoted now. A plain scalar continues across a blank line, so `description: a\n\n b` keeps `b` instead of dropping it. Blank lines are consumed only when an indented continuation follows, so a blank line before the next key still ends the scalar. Only a column-zero `---` closes the frontmatter. An indented `---` inside a block scalar used to end the scan early, discarding the rest of the description and every key after it. A folded scalar preserves the breaks around a more-indented line and its leading whitespace, instead of folding everything into one line. `instructions()` normalizes whitespace for the index, but `LocalDirLazySkillSource.list_skill_metadata()` returns the description to callers directly, so the stored value has to be right. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HYuwKi4xwDVSptmiipM5LL
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f8fc54b82
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if entry == "": | ||
| folded.append("\n") | ||
| previous = "blank" | ||
| continue |
There was a problem hiding this comment.
When a folded description has a blank line adjacent to a more-indented line, this branch resets previous to "blank" and loses one of the line breaks that YAML preserves. For example, description: >\n first\n\n note\n last should produce first\n\n note\nlast\n, but the parser returns first\n note\nlast\n; the exported LocalDirLazySkillSource.list_skill_metadata() therefore exposes a reshaped description. The fresh evidence after the earlier more-indented-line fix is this still-failing blank-line variant, which the new adjacent-line regression does not cover.
AGENTS.md reference: AGENTS.md:L113-L116
Useful? React with 👍 / 👎.
| if "-" in header[1:]: | ||
| chomping = "strip" | ||
| elif "+" in header[1:]: | ||
| chomping = "keep" |
There was a problem hiding this comment.
For a valid block header with an inline comment containing - or +, the substring checks treat punctuation in the comment as a chomping indicator. For example, description: > # multi-line text should use clip chomping and return "text\n", but this code selects strip and returns "text"; a + in the comment similarly enables keep unexpectedly. Parse the indicator portion before the YAML comment so the public metadata value is not altered by comment text.
AGENTS.md reference: AGENTS.md:L113-L116
Useful? React with 👍 / 👎.
| if pending_breaks: | ||
| parts.append("\n" * pending_breaks) | ||
| pending_breaks = 0 | ||
| parts.append(line.strip()) |
There was a problem hiding this comment.
When a valid plain description is followed by an indented YAML comment, this loop treats the comment as scalar continuation text. For example, description: Use for triage.\n # maintainer note\nname: triage parses as "Use for triage. # maintainer note" instead of "Use for triage.", so both the exported metadata and the model-facing skill index expose text that YAML designates as a comment. Ignore indented comment-only lines rather than appending them to parts.
AGENTS.md reference: AGENTS.md:L113-L116
Useful? React with 👍 / 👎.
| if block_indent is None: | ||
| block_indent = indent | ||
| if indent < block_indent: | ||
| break |
There was a problem hiding this comment.
For a valid block scalar with an explicit indentation indicator, the parser ignores the digit and instead fixes block_indent from the first nonblank line. For example, description: |2\n emergency triage\n avoid routine review should retain both lines as " emergency triage\navoid routine review\n", but this loop sets the indent to four, stops when it reaches the two-space line, and returns only "emergency triage\n"; the model-facing skill index consequently loses part of the description. Parse the indentation indicator from the header and use it as the scalar's base indentation.
AGENTS.md reference: AGENTS.md:L113-L116
Useful? React with 👍 / 👎.
1 participant
Summary
_parse_frontmattersplits every frontmatter line on the first:and keeps the rest as the value, so a multi-line YAML scalar never survives. Onmain, compared withyaml.safe_loadover the same frontmatter:Both
LocalDirLazySkillSource.list_skill_metadataandSkills._resolve_runtime_metadatabuildSkillMetadata.descriptionfrom this parser, so a skill written with a folded description reaches the model as- triage: > (file: ...)— nothing to match a task against, and nothing warns. Folded descriptions are common inSKILL.mdfiles written for other harnesses.This teaches the parser the shapes that frontmatter actually uses:
>folded and|literal block scalars, including the-and+chomping indicators.description: >with nothing under it no longer swallows the next key.Parsed values now agree with
yaml.safe_loadon every shape above. PyYAML stays out of the runtime dependencies — it is not one today, which is presumably why the parser is hand-rolled, and the issue notes the same.One deliberate divergence from
yaml.safe_load: an unquoted#is kept rather than treated as a comment, matching the current parser.description: use the #triage tagstays intact instead of truncating touse, which seems the better failure mode for a model-facing index.Fixing the parser lets a
|description carry real newlines, and the skill index is one line per skill, soinstructions()now collapses whitespace in the description before rendering. Without that, a literal description would split one entry across several list lines.Test plan
New
tests/sandbox/capabilities/test_skills_frontmatter.pycovers plain, quoted (both styles), empty, folded, literal, all four chomping combinations, paragraph breaks, wrapped plain scalars, comment lines,#inside a value, unterminated frontmatter, block-scalar boundaries, and the four cases from review below. Two tests intest_skills_capability.pyassert the rendered index line for a folded and a literal description end to end.Every one of the new tests fails on
mainand passes with this change.Checked against
yaml.safe_loadover 24 frontmatter shapes: 23 match, the 24th being the#divergence described above.The review pass found four more cases where the parser disagreed with
yaml.safe_load; each is fixed in 1f8fc54 with a test:---inside a block scalar ended the frontmatter scan, discarding the rest of the description and every later key. Only a column-zero marker closes it.LocalDirLazySkillSource.list_skill_metadata()returns the description to callers directly.make formatandmake lintpass.make typecheckfails identically with and without this change — 8 pyright and 10 mypy errors, all inextensions/memory/redis_session.py,tests/mcp/,function_schema.py,models/openai_responses.py,testing/model.py, none in the files touched here. My local resolution picks up newer dependency versions than CI does, becauseexclude-newer = "7 days"inpyproject.tomlis not a value uv can parse as a date:so the cooldown pin is skipped locally.
mypyandpyrightare clean onsrc/agents/sandbox/capabilities/skills.pyand the new tests. A handful of unrelated tests also fail on a clean checkout in this environment (tests/mcp/test_caching.py,tests/mcp/test_mcp_util.py,tests/test_function_tool.py,tests/tracing/test_import_side_effects.py,tests/sandbox/test_run_cwd.py::test_python_skill_uses_absolute_root_from_nested_workdir) for the same reason.Issue number
Closes #5026
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR🤖 Generated with Claude Code
https://claude.ai/code/session_01HYuwKi4xwDVSptmiipM5LL