refactor: integrate gray-matter for frontmatter parsing and enhance skill output#116
Merged
Conversation
…ce validation warnings
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThis pull request updates skill authoring, initialization, publishing, and validation to derive and populate presentation metadata (metadata.title and metadata.icon) when absent, serialize SKILL.md frontmatter using gray-matter, change publish output to print the canonical storage directory plus per-target linked/copied messages, and emit non-fatal warnings for missing metadata fields. Tests, docs, i18n entries, and package dependencies were updated to reflect these behaviors. Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (oo skills init)
participant Resolver as Workflow Resolver
participant Serializer as gray-matter
participant FS as Filesystem
participant Publisher as Publisher (symlink/copy)
participant Output as CLI Output
CLI->>Resolver: resolve workflow purpose & package metadata
Resolver-->>CLI: derived title & icon (if user omitted)
CLI->>Serializer: build structured frontmatter object
Serializer->>FS: write SKILL.md (YAML frontmatter)
CLI->>Publisher: publish to targets (decide symlink or copy)
Publisher->>FS: create link or copy per target
Publisher-->>CLI: per-target mode & path
CLI->>Output: print canonical storage path
CLI->>Output: print per-target "linked"/"copied" messages
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/application/commands/skills/init.test.ts (1)
245-246: Assert the rollback error output too.This now only proves that
stdoutstayed empty. The test can still pass if the command fails for a different reason before or after the publish step, so it no longer pins the scenario to the expected stderr failure path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/init.test.ts` around lines 245 - 246, The test currently only asserts result.exitCode and stdout; add an assertion to verify stderr contains the rollback error to ensure the failure happened during publish rollback. Update the assertions after the command run (where result, exitCode and stdout are checked) to also assert result.stderr includes the expected rollback message (or a regex like /rollback/i) that your publish/rollback path emits so the test pins the failure to the rollback error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/application/commands/skills/validate.ts`:
- Around line 147-153: The code currently treats malformed presentation metadata
as missing; instead, validate parsedMatter.data.metadata strictly and fail when
it's invalid. Locate parsedMatter.data.metadata and the assignments to
metadataIcon/metadataTitle (the isRecord checks around metadata.icon and
metadata.title) and replace them with explicit validation: if metadata is not an
object (isRecord) or metadata.icon is not a non-empty string or metadata.title
is not a non-empty string, throw or return a validation error (causing skills
validate to fail) rather than returning undefined; apply the same strict checks
to the other occurrence of metadata handling later in the file (the block around
the second isRecord usage). Ensure error messages identify which field is
malformed (metadata, metadata.icon, or metadata.title).
- Around line 106-112: The current validation only checks type of the
frontmatter.description variable and allows empty or whitespace-only strings;
update the validation in validate.ts (the block using the description constant)
to ensure description.trim().length > 0 (or equivalent) and return the same
error object when the description is missing or empty/whitespace-only so empty
descriptions are rejected by the skills validate flow.
---
Nitpick comments:
In `@src/application/commands/skills/init.test.ts`:
- Around line 245-246: The test currently only asserts result.exitCode and
stdout; add an assertion to verify stderr contains the rollback error to ensure
the failure happened during publish rollback. Update the assertions after the
command run (where result, exitCode and stdout are checked) to also assert
result.stderr includes the expected rollback message (or a regex like
/rollback/i) that your publish/rollback path emits so the test pins the failure
to the rollback error path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2d225ab-b74a-47b0-bf2a-067cb0493b66
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
contrib/skills/claude/oo-create-skill/SKILL.mdcontrib/skills/codex/oo-create-skill/SKILL.mdcontrib/skills/openclaw/oo-create-skill/SKILL.mddocs/commands.mddocs/commands.zh-CN.mdpackage.jsonsrc/application/commands/skills/embedded-assets.test.tssrc/application/commands/skills/init.test.tssrc/application/commands/skills/init.tssrc/application/commands/skills/validate.test.tssrc/application/commands/skills/validate.tssrc/i18n/catalog.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/application/commands/skills/validate.test.ts (1)
68-358:⚠️ Potential issue | 🔴 CriticalMultiple critical bugs prevent tests from passing: inverted icon validation logic, mismatched error messages, and metadata requirement conflict.
The implementation has three critical issues:
Inverted icon validation condition (Line 124): The code reads
if (!isDefined(iconError))which is backwards. When icon is undefined,validateSkillIconValue()returns undefined (lines 186–187), triggering the error path with an undefined message. This should beif (isDefined(iconError))to match the title validation pattern at line 134.Metadata field is required, but tests expect it optional: Line 165–167 rejects any YAML without
metadata:as an object error. However, tests at lines 85 and 116 omit the metadata field entirely and expect only warnings for missing icon/title. The parser should allow missing metadata and let warnings handle it instead.Error message text mismatches:
- Tests expect
"Frontmatter metadata.title cannot be empty."(lines 229, 257) but implementation returns"Frontmatter metadata.title must be a non-empty string."(line 201)- Tests expect
"Frontmatter metadata.title must be a string."(line 286) but implementation only checksisNonEmptyString()and never produces a type-mismatch errorThese contract gaps cause failures at lines 85, 116, 145, 201, 229, 257, and 286.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/validate.test.ts` around lines 68 - 358, The icon validation logic in validateSkillDirectory is inverted: change the condition using validateSkillIconValue() from if (!isDefined(iconError)) to if (isDefined(iconError)) so real errors are reported; make metadata optional by allowing metadata === undefined (do not immediately reject non-present metadata) and instead emit warnings for missing metadata.icon and metadata.title; adjust the title validation in validateSkillDirectory (and any helper isNonEmptyString checks) to first check type and return "Frontmatter metadata.title must be a string." for non-string values, then return "Frontmatter metadata.title cannot be empty." for empty/whitespace-only strings (matching tests), and ensure the icon error message remains "Frontmatter metadata.icon must be a non-empty string." where applicable.
🧹 Nitpick comments (1)
src/application/commands/skills/validate.test.ts (1)
10-358: Extract repeated SKILL.md setup into a local test helper.There is heavy copy-paste for temp dir creation, SKILL.md writing, and cleanup across many tests. Consolidating this will reduce maintenance cost and make future schema-case additions safer.
♻️ Refactor sketch
+async function withSkillFrontmatter( + directoryName: string, + frontmatterLines: string[], + run: (skillDirectoryPath: string) => Promise<void>, +): Promise<void> { + const rootDirectory = await createTemporaryDirectory("oo-skills-validate"); + const skillDirectoryPath = join(rootDirectory, directoryName); + try { + await mkdir(skillDirectoryPath, { recursive: true }); + await Bun.write( + join(skillDirectoryPath, "SKILL.md"), + ["---", ...frontmatterLines, "---", ""].join("\n"), + ); + await run(skillDirectoryPath); + } finally { + await rm(rootDirectory, { force: true, recursive: true }); + } +}- test("warns about missing metadata icon only", async () => { - const rootDirectory = await createTemporaryDirectory("oo-skills-validate"); - const skillDirectoryPath = join(rootDirectory, "missing-icon-skill"); - try { - await mkdir(skillDirectoryPath, { recursive: true }); - await Bun.write( - join(skillDirectoryPath, "SKILL.md"), - [ - "---", - "name: missing-icon-skill", - "description: Use this skill for a workflow.", - "metadata:", - " title: Missing Icon Skill", - "---", - "", - ].join("\n"), - ); - await expect(validateSkillDirectory(skillDirectoryPath)).resolves.toEqual({ - warnings: ["Warning: Frontmatter metadata.icon is missing."], - }); - } finally { - await rm(rootDirectory, { force: true, recursive: true }); - } - }); + test("warns about missing metadata icon only", async () => { + await withSkillFrontmatter( + "missing-icon-skill", + [ + "name: missing-icon-skill", + "description: Use this skill for a workflow.", + "metadata:", + " title: Missing Icon Skill", + ], + async (skillDirectoryPath) => { + await expect(validateSkillDirectory(skillDirectoryPath)).resolves.toEqual({ + warnings: ["Warning: Frontmatter metadata.icon is missing."], + }); + }, + ); + });As per coding guidelines,
**/*.test.{ts,tsx}: “Extract repeated setup (mocks, stubs, setup objects) into local factory functions in test files; avoid copy-pasting test setup.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/validate.test.ts` around lines 10 - 358, Extract the repeated temp-dir + SKILL.md creation and cleanup into a local test helper (e.g., a function createSkillTestDir or makeSkillDir) used by tests that call createTemporaryDirectory, mkdir, Bun.write, and rm; the helper should accept the directory name and SKILL.md contents (or a small options object for name/description/metadata), create the skillDirectoryPath, write SKILL.md, and return { skillDirectoryPath, cleanup } (or ensure cleanup is invoked in finally blocks). Replace direct uses of createTemporaryDirectory, Bun.write(SKILL.md), and createCliSandbox/sandbox.run wiring to instead call the helper in tests that assert validateSkillDirectory or run the CLI, keeping validateSkillDirectory, createCliSandbox, and sandbox.run usages unchanged except for using the returned path/cleanup. Ensure tests that rely on specific SKILL.md variations pass content through the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/application/commands/skills/validate.test.ts`:
- Around line 68-358: The icon validation logic in validateSkillDirectory is
inverted: change the condition using validateSkillIconValue() from if
(!isDefined(iconError)) to if (isDefined(iconError)) so real errors are
reported; make metadata optional by allowing metadata === undefined (do not
immediately reject non-present metadata) and instead emit warnings for missing
metadata.icon and metadata.title; adjust the title validation in
validateSkillDirectory (and any helper isNonEmptyString checks) to first check
type and return "Frontmatter metadata.title must be a string." for non-string
values, then return "Frontmatter metadata.title cannot be empty." for
empty/whitespace-only strings (matching tests), and ensure the icon error
message remains "Frontmatter metadata.icon must be a non-empty string." where
applicable.
---
Nitpick comments:
In `@src/application/commands/skills/validate.test.ts`:
- Around line 10-358: Extract the repeated temp-dir + SKILL.md creation and
cleanup into a local test helper (e.g., a function createSkillTestDir or
makeSkillDir) used by tests that call createTemporaryDirectory, mkdir,
Bun.write, and rm; the helper should accept the directory name and SKILL.md
contents (or a small options object for name/description/metadata), create the
skillDirectoryPath, write SKILL.md, and return { skillDirectoryPath, cleanup }
(or ensure cleanup is invoked in finally blocks). Replace direct uses of
createTemporaryDirectory, Bun.write(SKILL.md), and createCliSandbox/sandbox.run
wiring to instead call the helper in tests that assert validateSkillDirectory or
run the CLI, keeping validateSkillDirectory, createCliSandbox, and sandbox.run
usages unchanged except for using the returned path/cleanup. Ensure tests that
rely on specific SKILL.md variations pass content through the helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b941a5c-4c87-4afe-8188-297982e9a5c8
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
cspell.config.ymldocs/commands.mddocs/commands.zh-CN.mdpackage.jsonsrc/application/commands/skills/validate.test.tssrc/application/commands/skills/validate.ts
✅ Files skipped from review due to trivial changes (3)
- package.json
- cspell.config.yml
- docs/commands.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/application/commands/skills/validate.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/application/commands/skills/validate.ts`:
- Around line 98-109: The code accepts whitespace-only names because it only
checks isNonEmptyString(name); fix by trimming the input before validation:
compute const trimmedName = name.trim() and use trimmedName in the emptiness
check and when calling validateSkillNameValue (i.e., replace uses of name with
trimmedName in the validation blocks around the
isNonEmptyString/validateSkillNameValue calls), and apply the same change to the
analogous block at the later occurrence (lines around the second check noted in
the review).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3e7e98b-67e8-4769-b257-d07725ed81d8
📒 Files selected for processing (1)
src/application/commands/skills/validate.ts
…rove error messages
This was referenced Apr 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
grap-matterto handle frontmatter, simplifying a large amount of code.oo skills init.oo-create-skill.