-
Notifications
You must be signed in to change notification settings - Fork 10.6k
refactor(core): harden skill frontmatter parsing #16705
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This commit adds a 'hardening' pass on top of the initial fix for issue #16323: - Use regex for more flexible name and description field matching - Support missing space after colons in frontmatter (e.g. 'name:foo') - Support indented field names in frontmatter - Add safety check for missing body in SKILL.md - Add comprehensive test cases for frontmatter edge cases
Summary of ChangesHello @NTaylorMullen, 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 significantly improves the robustness and flexibility of the skill frontmatter parsing mechanism. By adopting more adaptive matching techniques and enhancing error handling, the system can now gracefully process a wider range of user formatting styles, preventing issues that might arise from non-standard input. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
abhipatel12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Size Change: +74 B (0%) Total Size: 23.1 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively hardens the frontmatter parsing for skills by using regular expressions to handle formatting variations like leading whitespace and missing spaces after colons, and adds a crucial safety check to prevent crashes when a skill file lacks a body. The accompanying tests are comprehensive and cover the new edge cases well. To further improve the parser's resilience, consider making the key matching case-insensitive for name and description fields.
| if (line.startsWith('name:')) { | ||
| name = line.substring(5).trim(); | ||
| // Match "name:" at the start of the line (optional whitespace) | ||
| const nameMatch = line.match(/^\s*name:\s*(.*)$/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex for matching the name field is case-sensitive. To be more resilient to user formatting variations, as is the goal of this refactor, consider making it case-insensitive. A user might write Name: which would currently fail to be parsed by this fallback parser.
| const nameMatch = line.match(/^\s*name:\s*(.*)$/); | |
| const nameMatch = line.match(/^\s*name:\s*(.*)$/i); |
| if (line.startsWith('description:')) { | ||
| const descLines = [line.substring(12).trim()]; | ||
| // Match "description:" at the start of the line (optional whitespace) | ||
| const descMatch = line.match(/^\s*description:\s*(.*)$/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex for matching the description field is case-sensitive. To be more resilient to user formatting variations, consider making it case-insensitive. A user might write Description: which would currently fail to be parsed by this fallback parser.
| const descMatch = line.match(/^\s*description:\s*(.*)$/); | |
| const descMatch = line.match(/^\s*description:\s*(.*)$/i); |
Summary
This PR adds a 'hardening' pass on top of the recently merged fix for issue #16323. It makes the simple frontmatter parser more resilient to variations in user formatting.
Details
name:foo).SKILL.mdfile contains frontmatter but no subsequent content.Related Issues
Related to #16323
How to Validate
Run the updated test suite:
Verified that all 13 tests pass, including the 6 new edge case scenarios.
Pre-Merge Checklist