feat(skills): support Agent Skills frontmatter - #517
Conversation
|
@alectimison-maker is attempting to deploy a commit to the esokullu's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Review: PR #517 — feat(skills): support Agent Skills frontmatter Overview Teaches the custom-skill importer to recognize the two required fields (name, description) of an Agent Skills SKILL.md YAML frontmatter, via a small hand-rolled parser (parseAgentSkillFrontmatter + parseAgentSkillScalar) added identically to src/chrome/src/agent/skills.js and src/firefox/src/agent/skills.js. Valid frontmatter feeds the routing catalog (name/summary) and is stripped before the Markdown body is loaded; invalid frontmatter is left untouched as plain text. Docs updated in en/fr/zh-CN; three new multi-case tests cover both builds. What's done well
Issues
description: Extract PDF text and fill forms. — is valid YAML (folds to one string) and passes validation here, but the parser accepts it with description = "Extract PDF text and fill forms." only. Worse, the continuation line lives inside the frontmatter block, so it's also stripped from the loaded body: the text is lost from both the catalog and the instructions. This contradicts the PR's stated principle that unsupported forms "remain unmodified text." Suggest either (a) folding continuation lines into the value, or (b) detecting an indented line immediately after name:/description: plain scalars and returning null (reject → keep as text), which fits the conservative design better.
Nits / observations
Test coverage Strong: valid folded scalar, precedence, invalid-name fallback, duplicates, implicit boolean, non-string flow sequence, oversized description, leak assertions into the built prompt, all run against both builds. Gaps worth adding, especially given issue 1:
Verdict Well-scoped, security-conscious change that matches the project's conventions and trust model. I'd ask for issue 1 (multi-line plain scalar data loss) to be fixed or explicitly rejected-as-invalid before merge; issues 2–3 and the extra tests are nice-to-haves. @alectimison-maker |
|
Addressed the review feedback in
Validation: |
There was a problem hiding this comment.
Pull request overview
Adds Agent Skills frontmatter support to custom skill imports.
Changes:
- Parses
nameanddescriptionmetadata in both browser builds. - Strips valid frontmatter before loading instructions.
- Adds parity tests and multilingual documentation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/chrome/src/agent/skills.js |
Implements Chrome parsing and routing integration. |
src/firefox/src/agent/skills.js |
Mirrors the Chrome implementation. |
test/run.js |
Tests parsing, precedence, rejection, and preservation. |
docs/skills.md |
Documents compatibility and limitations. |
docs/architecture.md |
Describes routing and prompt behavior. |
docs/fr/skills.md |
Adds French user documentation. |
docs/fr/architecture.md |
Adds French architecture documentation. |
docs/zh-CN/skills.md |
Adds Chinese user documentation. |
docs/zh-CN/architecture.md |
Adds Chinese architecture documentation. |
Comments suppressed due to low confidence (6)
src/chrome/src/agent/skills.js:148
- This derives indentation from the minimum across all block lines, but YAML derives it from the first non-empty line. Consequently an invalid block such as a 4-space first line followed by a 2-space line (or tab indentation) is accepted and its frontmatter is destructively stripped. Reject under-indented/tab-indented content so unsupported YAML remains ordinary skill text as intended.
const nonEmpty = block.filter((item) => item.trim());
const indent = nonEmpty.length
? Math.min(...nonEmpty.map((item) => item.match(/^\s*/)[0].length))
: 0;
values[key] = cleanText(block.map((item) => item.slice(indent)).join('\n'));
src/firefox/src/agent/skills.js:148
- This derives indentation from the minimum across all block lines, but YAML derives it from the first non-empty line. Consequently an invalid block such as a 4-space first line followed by a 2-space line (or tab indentation) is accepted and its frontmatter is destructively stripped. Reject under-indented/tab-indented content so unsupported YAML remains ordinary skill text as intended.
const nonEmpty = block.filter((item) => item.trim());
const indent = nonEmpty.length
? Math.min(...nonEmpty.map((item) => item.match(/^\s*/)[0].length))
: 0;
values[key] = cleanText(block.map((item) => item.slice(indent)).join('\n'));
src/chrome/src/agent/skills.js:166
- The current Agent Skills validator permits lowercase international names (for example
技能andмой-навык) and NFKC-normalizes them, but this ASCII-only regex rejects those valid skills and leaves their frontmatter in the prompt. Validate Unicode letters/numbers with lowercase and NFKC checks; count code points rather than UTF-16 units so the 64/1024-character limits also match the specification.
const name = cleanSingleLine(values.name);
const description = cleanSingleLine(values.description);
if (
!/^[a-z0-9]+(?:-[a-z0-9]+)*$/.test(name)
|| name.length > 64
|| !description
|| description.length > 1024
src/firefox/src/agent/skills.js:166
- The current Agent Skills validator permits lowercase international names (for example
技能andмой-навык) and NFKC-normalizes them, but this ASCII-only regex rejects those valid skills and leaves their frontmatter in the prompt. Validate Unicode letters/numbers with lowercase and NFKC checks; count code points rather than UTF-16 units so the 64/1024-character limits also match the specification.
const name = cleanSingleLine(values.name);
const description = cleanSingleLine(values.description);
if (
!/^[a-z0-9]+(?:-[a-z0-9]+)*$/.test(name)
|| name.length > 64
|| !description
|| description.length > 1024
src/chrome/src/agent/skills.js:127
- Only known optional Agent Skills fields should be ignored. This branch currently accepts every unknown top-level key, but the official validator rejects unexpected fields; such a file is therefore treated as valid and stripped even though it is not a valid Agent Skill. Reject keys outside
name,description,license,allowed-tools,metadata, andcompatibility.
const key = field[1];
if (key !== 'name' && key !== 'description') continue;
src/firefox/src/agent/skills.js:127
- Only known optional Agent Skills fields should be ignored. This branch currently accepts every unknown top-level key, but the official validator rejects unexpected fields; such a file is therefore treated as valid and stripped even though it is not a valid Agent Skill. Reject keys outside
name,description,license,allowed-tools,metadata, andcompatibility.
const key = field[1];
if (key !== 'name' && key !== 'description') continue;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ? cleanSingleLine(item.sourceUrl || item.path).slice(0, 2048) | ||
| : ''; | ||
| const name = cleanSingleLine(item.name).slice(0, 80) || inferName(content, skills.length); | ||
| const agentSkill = parseAgentSkillFrontmatter(content); |
| ? cleanSingleLine(item.sourceUrl || item.path).slice(0, 2048) | ||
| : ''; | ||
| const name = cleanSingleLine(item.name).slice(0, 80) || inferName(content, skills.length); | ||
| const agentSkill = parseAgentSkillFrontmatter(content); |
| return null; | ||
| } | ||
| } | ||
| return raw.replace(/\s+#.*$/, '').trim() || null; |
| return null; | ||
| } | ||
| } | ||
| return raw.replace(/\s+#.*$/, '').trim() || null; |
|
The parser crosses the frontmatter/body trust boundary, allowing hidden WebBrain metadata and tools. It also diverges from the reference Agent Skills grammar and validation rules, rejecting valid skills while destructively accepting malformed ones. Full review comments:
|
|
Addressed the five review findings in
I also added coverage proving legitimate Validation: |
Summary
SKILL.mdfrontmatter when importing custom skillsnameanddescriptionin the existing routing catalog and remove frontmatter before loading Markdown instructionsMotivation
Agent Skills files otherwise import as plain text, so required frontmatter can become the inferred display name and is repeated in the loaded prompt. Supporting the standard routing fields lets existing
SKILL.mdinstructions work without manual conversion.The parser also sits on a security boundary: fenced
webbrain-skillorwebbrain-toolstext inside a YAML block scalar must remain ordinary frontmatter data and must not grant Ask eligibility, routing intents, or network tools.Design
The dependency-free parser follows the current
agentskills/skills-refbehavior needed by WebBrain:name,description,license,compatibility,metadata, andallowed-toolstrue,123, and dates remain strings, while malformed mapping separators and flow collections reject the parseAfter valid frontmatter is parsed,
webbrain-skillandwebbrain-toolsdiscovery scans onlyagentSkill.body. Re-normalization also rebuilds tools from that body instead of preserving a previously deriveditem.toolsarray. This closes the frontmatter/body trust-boundary bypass while retaining legitimate WebBrain manifests placed in the Markdown body.Existing precedence remains:
webbrain-skillblock overrides the standard routing summary and may explicitly enable supported modes/intentsallowed-toolsnever grants WebBrain tools, permissions, or Ask eligibilityThis avoids adding a YAML/runtime dependency to both browser builds and introduces no network access, permissions, or code execution.
Testing
node test/run.js— 1358 passed; the one failure is the existing upstream release-history mismatch (package.json26.0.1 vs. latestCHANGELOG.mdentry 26.0.0)node test/security/injection-corpus.mjs— 60/60 checks passednode --check src/chrome/src/agent/skills.js— passednode --check src/firefox/src/agent/skills.js— passedgit diff --check— passedRegression coverage includes hidden frontmatter manifests/tools, re-normalization, legitimate body-level tools, StrictYAML string cases, malformed plain scalars, allowed/unknown/duplicate fields, explicit and invalid indentation, empty optional fields, international names, uppercase rejection, and NFKC normalization.
Compatibility and risks
Existing plain-text and body-level
webbrain-skill/webbrain-toolsimports retain their behavior. The parser intentionally implements the bounded Agent Skills frontmatter subset WebBrain consumes instead of adding a general YAML dependency. Residual risk is future divergence if the reference grammar changes.Scope
This does not import Agent Skills directories or fetch
scripts/,references/, orassets/. It does not execute bundled code or interpretallowed-tools; those capabilities require a separate security and packaging design.