fix: use block-style YAML sequences in SKILL.md frontmatter#580
fix: use block-style YAML sequences in SKILL.md frontmatter#580ademakdogan wants to merge 1 commit intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: a4b477b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical validation issue where generated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue with YAML frontmatter formatting by switching from flow-style to block-style sequences, which is required by the strictyaml parser. The changes are applied consistently across all skill generation templates, and the addition of unit tests to verify this new format is a great improvement. I've found one issue in a new test helper function that could make the tests brittle, which I've commented on.
ademakdogan
force-pushed
the
fix/skill-frontmatter-block-sequences
branch
from
923a1d6 to
bee1af2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue with YAML frontmatter generation by switching from flow-style to block-style sequences, which is required by the strictyaml parser. The changes are applied consistently across all relevant skill generation templates. The addition of unit tests to verify the output format is an excellent improvement that will help prevent future regressions. I've found one potential issue in a test helper function that could make the new tests less reliable, and I've provided a suggestion to make it more robust.
ademakdogan
force-pushed
the
fix/skill-frontmatter-block-sequences
branch
from
bee1af2 to
d45f7fd
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the YAML compatibility issue with strictyaml by converting all generated frontmatter to use block-style sequences instead of flow-style. The changes are clean and directly solve the problem described. The addition of comprehensive unit tests is excellent, as it verifies the fix for various skill types and will prevent future regressions. I have one suggestion to make the new test logic even more robust.
ademakdogan
force-pushed
the
fix/skill-frontmatter-block-sequences
branch
from
d45f7fd to
bf4b91b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly replaces flow-style YAML sequences with block-style sequences in the generated SKILL.md frontmatter to fix validation issues with strictyaml. The changes are applied consistently across service, shared, persona, and recipe skill templates. The addition of unit tests to verify the output format is a great improvement. I've identified one area for improvement in test coverage to make the fix even more robust.
Replace flow sequences (bins: ["gws"], skills: [...]) with block-style sequences in all generated SKILL.md frontmatter templates. Flow sequences are valid YAML but rejected by strictyaml, which the Agent Skills reference implementation (agentskills validate) uses to parse frontmatter. This caused all 93 generated skills to fail validation. Also adds unit tests verifying that service, shared, persona, and recipe skill templates produce block-style sequences only. Fixes googleworkspace#521
ademakdogan
force-pushed
the
fix/skill-frontmatter-block-sequences
branch
from
bf4b91b to
a4b477b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue with YAML flow sequences by converting them to block-style sequences, which is necessary for compatibility with strictyaml. The addition of comprehensive unit tests to validate the frontmatter format is a great improvement. However, I've identified a regression in how empty skill lists are handled for personas and recipes. The new implementation produces a null value instead of an empty list, which could lead to validation failures. I've added two high-severity comments with suggestions to fix this potential bug.
| skills: | ||
| {skills} |
There was a problem hiding this comment.
This will produce skills:\n when required_skills is empty, which is a null value in YAML. The original code produced an empty list []. If the schema expects a list, this will fail validation for any persona without service dependencies.
To fix this, you should handle the empty case in the required_skills generation and adjust this template.
For example, you can modify the required_skills block (lines 762-767) to produce [] for an empty list:
let required_skills = if persona.services.is_empty() {
"[]".to_string()
} else {
persona
.services
.iter()
.map(|s| format!("\n - gws-{}", s))
.collect::<String>()
};With that change, this template part should be on a single line.
| skills: | |
| {skills} | |
| skills: {skills} |
| skills: | ||
| {skills} |
There was a problem hiding this comment.
Similar to the persona skill generation, this will produce skills:\n when required_skills is empty, which is a null value in YAML. The original code produced an empty list []. If the schema expects a list, this will fail validation for any recipe without service dependencies.
To fix this, you should handle the empty case in the required_skills generation and adjust this template.
For example, you can modify the required_skills block (lines 834-839) to produce [] for an empty list:
let required_skills = if recipe.services.is_empty() {
"[]".to_string()
} else {
recipe
.services
.iter()
.map(|s| format!("\n - gws-{}", s))
.collect::<String>()
};With that change, this template part should be on a single line.
| skills: | |
| {skills} | |
| skills: {skills} |
2 participants
Description
Replace flow sequences (
bins: ["gws"],skills: [...]) with block-style sequences in all generated SKILL.md frontmatter templates.Flow sequences are valid YAML but rejected by
strictyaml, which the Agent Skills reference implementation (agentskills validate) uses to parse frontmatter. This caused all 93 generated skills to fail validation.Also adds unit tests verifying that service, shared, persona, and recipe skill templates produce block-style sequences only.
Fixes #521
Dry Run Output:
N/AChecklist:
AGENTS.mdguidelines (no generatedgoogle-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.