refactor(skills): render bundled skills from shared templates#217
Merged
Conversation
|
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 (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
Summary by CodeRabbit
WalkthroughAdds agent-aware rendering for bundled skill Markdown using Sequence Diagram(s)sequenceDiagram
participant CLI
participant Embedded as EmbeddedAssets
participant Renderer as agentic-markdown
participant FS as FileSystem
CLI->>Embedded: getBundledSkillFiles(skill, agent)
Embedded->>CLI: BundledSkillFile[] (with contentKind)
loop for each BundledSkillFile
CLI->>Embedded: readBundledSkillFileContent(file)
Embedded->>Renderer: if contentKind == agenticMarkdown, render with agent config
Renderer-->>Embedded: rendered Markdown text
Embedded-->>CLI: file.content (rendered or raw)
CLI->>FS: write(destinationPath, file.content)
FS-->>CLI: write complete
end
Note right of CLI: Tests also call readBundledSkillFileContent to assert rendered output
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: 3
🧹 Nitpick comments (3)
src/application/commands/skills/embedded-assets.ts (1)
101-114: ⚡ Quick winAdd error handling for file read and render operations.
readBundledSkillFileContentperforms I/O and rendering without error handling. IfBun.file().text()fails due to missing files or read permissions, or ifrender()throws due to malformed directives, the error will propagate unhandled.Consider wrapping the operations in try-catch to provide clearer error context, or document that callers are expected to handle errors.
🛡️ Add error handling with context
export async function readBundledSkillFileContent( file: BundledSkillFile, ): Promise<string> { + try { const content = await Bun.file(file.sourcePath).text(); if (file.contentKind === "static") { return content; } return render(content, { agent: file.agentName, agentTitle: bundledSkillAgentTitles[file.agentName], }); + } catch (error) { + throw new Error( + `Failed to read or render bundled skill file ${file.relativePath} for ${file.agentName}: ${error instanceof Error ? error.message : String(error)}`, + { cause: error }, + ); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/embedded-assets.ts` around lines 101 - 114, readBundledSkillFileContent currently performs I/O (Bun.file(file.sourcePath).text()) and rendering (render(...)) without protection; wrap the body of readBundledSkillFileContent in a try-catch that catches any errors from the file read or render call and rethrows a new Error (or reject) that includes clear context such as file.sourcePath, file.contentKind and file.agentName (and bundledSkillAgentTitles[file.agentName] if useful), and include the original error as the cause or append its message so callers get actionable context; ensure you still return the rendered content on success and preserve the same return type.contrib/skills/shared/oo-publish-skill/SKILL.md (1)
88-92: 💤 Low valueClarify the intent of the nested variable instruction.
The text instructs future agents to "replace
<!-- agentic:var agent -->with that host id", but this appears to reference a template variable that should be automatically rendered by agentic-markdown, not manually replaced.If this is meant to guide human readers editing a shared skill file, the phrasing is confusing because the variable directive will already be rendered when the agent reads this skill. If it's meant to instruct the agent to substitute the variable in generated output, it should be clearer.
Consider rephrasing for clarity, or verify this instruction is necessary and correctly expressed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/skills/shared/oo-publish-skill/SKILL.md` around lines 88 - 92, The sentence about replacing `<!-- agentic:var agent -->` is ambiguous; clarify whether this is a human editing instruction or an agent/template directive. Update the text inside the conditional block (the lines around `<!-- agentic:if agent=openclaw|qoderwork -->` and the `<!-- agentic:var agent -->` token) to explicitly state the intent: either say "This token is rendered automatically by agentic-markdown and should not be manually changed" for reader guidance, or say "Agents should substitute the `<!-- agentic:var agent -->` token with the host id when generating output" if it is an agent instruction—choose one and rephrase accordingly so the purpose of `<!-- agentic:var agent -->` is unambiguous.src/application/commands/skills/__tests__/helpers.ts (1)
32-48: ⚡ Quick winDeduplicate bundled file lookup in test helpers.
Lines 32-44 reimplement the same
getBundledSkillFiles(...).find(...)+ missing-file error logic already used bygetBundledSkillSourcePath. Extract one internal resolver to avoid drift.♻️ Suggested refactor
+function getRequiredBundledSkillFile( + skillName: BundledSkillName, + relativePath: string, + agentName: BundledSkillAgentName, +) { + const file = getBundledSkillFiles(skillName, agentName).find(file => + file.relativePath === relativePath, + ); + + if (file === undefined) { + throw new Error( + `Missing bundled skill file: ${agentName}/${skillName}/${relativePath}`, + ); + } + + return file; +} + export function getBundledSkillSourcePath( skillName: BundledSkillName, relativePath: string, agentName: BundledSkillAgentName = "codex", ): string { - const file = getBundledSkillFiles(skillName, agentName).find(file => - file.relativePath === relativePath, - ); - - if (file === undefined) { - throw new Error( - `Missing bundled skill file: ${agentName}/${skillName}/${relativePath}`, - ); - } - - return file.sourcePath; + return getRequiredBundledSkillFile(skillName, relativePath, agentName).sourcePath; } @@ export async function readBundledSkillSourceContent( skillName: BundledSkillName, relativePath: string, agentName: BundledSkillAgentName = "codex", ): Promise<string> { - const file = getBundledSkillFiles(skillName, agentName).find(file => - file.relativePath === relativePath, - ); - - if (file === undefined) { - throw new Error( - `Missing bundled skill file: ${agentName}/${skillName}/${relativePath}`, - ); - } - + const file = getRequiredBundledSkillFile(skillName, relativePath, agentName); return await readBundledSkillFileContent(file); }As per coding guidelines, "When identical logic appears in 2+ files, extract it to a shared module".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/__tests__/helpers.ts` around lines 32 - 48, readBundledSkillSourceContent duplicates the getBundledSkillFiles(...).find(...) + missing-file error logic already implemented in getBundledSkillSourcePath; extract a small shared resolver (e.g., resolveBundledSkillFileByPath or getBundledSkillFile) that accepts (skillName: BundledSkillName, relativePath: string, agentName?: BundledSkillAgentName) and returns the found file or throws the same Error, then replace the finder logic in both readBundledSkillSourceContent and getBundledSkillSourcePath to call that new helper; keep using readBundledSkillFileContent to read the file after resolving it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contrib/skills/shared/oo-find-skills/references/oo-cli-contract.md`:
- Around line 59-60: The contract currently renders `AskUserQuestion` for
`openclaw` due to the agentic condition groups; update the agentic tags so
`openclaw` uses the `request_user_input` variant instead of `AskUserQuestion` —
e.g., adjust the conditional `<!-- agentic:if agent=codex
-->request_user_input<!-- agentic:endif -->` to include `openclaw` (or remove
`openclaw` from the second group that outputs `AskUserQuestion`) so the tool
name for `openclaw` becomes `request_user_input` and matches the
OpenClaw-specific SKILL guidance.
In `@src/application/commands/skills/embedded-assets.ts`:
- Line 1: The import "import { render } from \"agentic-markdown\";" references a
non-existent npm package (agentic-markdown@0.0.2); update the dependency and
import to a valid module or local implementation: either change package.json to
the correct package name/version and run npm install, or replace the import with
the correct module (e.g., an existing package that provides render) or a local
utility (create/export a render function in this repo and import it by relative
path). Ensure the symbol referenced in the file (render) remains available from
the new module or local file so usages in
src/application/commands/skills/embedded-assets.ts continue to work.
In `@src/application/commands/skills/index.test.ts`:
- Line 509: The await call to readBundledSkillSourceContent is executed before
entering the try/finally, so if it throws sandbox.cleanup() won’t run; move the
awaited call into the try block (i.e., perform const expectedSkillContent =
await readBundledSkillSourceContent("oo", "SKILL.md") inside the try where
sandbox is active) so that any exception triggers the finally and calls
sandbox.cleanup(), keeping the surrounding try/finally structure and references
to sandbox.cleanup() intact.
---
Nitpick comments:
In `@contrib/skills/shared/oo-publish-skill/SKILL.md`:
- Around line 88-92: The sentence about replacing `<!-- agentic:var agent -->`
is ambiguous; clarify whether this is a human editing instruction or an
agent/template directive. Update the text inside the conditional block (the
lines around `<!-- agentic:if agent=openclaw|qoderwork -->` and the `<!--
agentic:var agent -->` token) to explicitly state the intent: either say "This
token is rendered automatically by agentic-markdown and should not be manually
changed" for reader guidance, or say "Agents should substitute the `<!--
agentic:var agent -->` token with the host id when generating output" if it is
an agent instruction—choose one and rephrase accordingly so the purpose of `<!--
agentic:var agent -->` is unambiguous.
In `@src/application/commands/skills/__tests__/helpers.ts`:
- Around line 32-48: readBundledSkillSourceContent duplicates the
getBundledSkillFiles(...).find(...) + missing-file error logic already
implemented in getBundledSkillSourcePath; extract a small shared resolver (e.g.,
resolveBundledSkillFileByPath or getBundledSkillFile) that accepts (skillName:
BundledSkillName, relativePath: string, agentName?: BundledSkillAgentName) and
returns the found file or throws the same Error, then replace the finder logic
in both readBundledSkillSourceContent and getBundledSkillSourcePath to call that
new helper; keep using readBundledSkillFileContent to read the file after
resolving it.
In `@src/application/commands/skills/embedded-assets.ts`:
- Around line 101-114: readBundledSkillFileContent currently performs I/O
(Bun.file(file.sourcePath).text()) and rendering (render(...)) without
protection; wrap the body of readBundledSkillFileContent in a try-catch that
catches any errors from the file read or render call and rethrows a new Error
(or reject) that includes clear context such as file.sourcePath,
file.contentKind and file.agentName (and bundledSkillAgentTitles[file.agentName]
if useful), and include the original error as the cause or append its message so
callers get actionable context; ensure you still return the rendered content on
success and preserve the same return type.
🪄 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: 6a139dcb-0de2-4efd-9e9d-813e10191660
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (75)
AGENTS.mdCLAUDE.mdcontrib/ci/npm-packages.test.tscontrib/skills/claude/oo-find-skills/references/oo-cli-contract.mdcontrib/skills/claude/oo-publish-skill/SKILL.mdcontrib/skills/claude/oo/SKILL.mdcontrib/skills/codebuddy/oo-create-skill/SKILL.mdcontrib/skills/codebuddy/oo-find-skills/SKILL.mdcontrib/skills/codebuddy/oo-find-skills/references/oo-cli-contract.mdcontrib/skills/codebuddy/oo-publish-skill/SKILL.mdcontrib/skills/codebuddy/oo/SKILL.mdcontrib/skills/codebuddy/oo/references/auth-and-billing.mdcontrib/skills/codebuddy/oo/references/connector-execution.mdcontrib/skills/codebuddy/oo/references/file-transfer.mdcontrib/skills/codebuddy/oo/references/llm-client.mdcontrib/skills/codebuddy/oo/references/package-execution.mdcontrib/skills/codebuddy/oo/references/search-and-selection.mdcontrib/skills/codebuddy/oo/references/task-lifecycle.mdcontrib/skills/codex/oo-create-skill/SKILL.mdcontrib/skills/codex/oo-find-skills/SKILL.mdcontrib/skills/codex/oo-find-skills/references/oo-cli-contract.mdcontrib/skills/codex/oo-publish-skill/SKILL.mdcontrib/skills/codex/oo/references/auth-and-billing.mdcontrib/skills/codex/oo/references/connector-execution.mdcontrib/skills/codex/oo/references/file-transfer.mdcontrib/skills/codex/oo/references/llm-client.mdcontrib/skills/codex/oo/references/package-execution.mdcontrib/skills/codex/oo/references/search-and-selection.mdcontrib/skills/codex/oo/references/task-lifecycle.mdcontrib/skills/openclaw/oo-create-skill/SKILL.mdcontrib/skills/openclaw/oo-find-skills/SKILL.mdcontrib/skills/openclaw/oo-publish-skill/SKILL.mdcontrib/skills/openclaw/oo/SKILL.mdcontrib/skills/openclaw/oo/references/auth-and-billing.mdcontrib/skills/openclaw/oo/references/connector-execution.mdcontrib/skills/openclaw/oo/references/file-transfer.mdcontrib/skills/openclaw/oo/references/llm-client.mdcontrib/skills/openclaw/oo/references/package-execution.mdcontrib/skills/openclaw/oo/references/search-and-selection.mdcontrib/skills/openclaw/oo/references/task-lifecycle.mdcontrib/skills/qoderwork/oo-create-skill/SKILL.mdcontrib/skills/qoderwork/oo-find-skills/SKILL.mdcontrib/skills/qoderwork/oo-find-skills/references/oo-cli-contract.mdcontrib/skills/qoderwork/oo/SKILL.mdcontrib/skills/qoderwork/oo/references/auth-and-billing.mdcontrib/skills/qoderwork/oo/references/connector-execution.mdcontrib/skills/qoderwork/oo/references/file-transfer.mdcontrib/skills/qoderwork/oo/references/llm-client.mdcontrib/skills/qoderwork/oo/references/package-execution.mdcontrib/skills/qoderwork/oo/references/search-and-selection.mdcontrib/skills/qoderwork/oo/references/task-lifecycle.mdcontrib/skills/shared/oo-create-skill/SKILL.mdcontrib/skills/shared/oo-create-skill/agents/openai.yamlcontrib/skills/shared/oo-find-skills/SKILL.mdcontrib/skills/shared/oo-find-skills/agents/openai.yamlcontrib/skills/shared/oo-find-skills/references/oo-cli-contract.mdcontrib/skills/shared/oo-publish-skill/SKILL.mdcontrib/skills/shared/oo-publish-skill/agents/openai.yamlcontrib/skills/shared/oo/SKILL.mdcontrib/skills/shared/oo/agents/openai.yamlcontrib/skills/shared/oo/references/auth-and-billing.mdcontrib/skills/shared/oo/references/connector-execution.mdcontrib/skills/shared/oo/references/file-transfer.mdcontrib/skills/shared/oo/references/llm-client.mdcontrib/skills/shared/oo/references/package-execution.mdcontrib/skills/shared/oo/references/search-and-selection.mdcontrib/skills/shared/oo/references/task-lifecycle.mdcspell.config.ymlpackage.jsonsrc/application/commands/skills/__tests__/helpers.tssrc/application/commands/skills/embedded-assets.test.tssrc/application/commands/skills/embedded-assets.tssrc/application/commands/skills/index.cli.test.tssrc/application/commands/skills/index.test.tssrc/application/commands/skills/shared.ts
💤 Files with no reviewable changes (48)
- contrib/skills/openclaw/oo/references/package-execution.md
- contrib/skills/codex/oo/references/task-lifecycle.md
- contrib/skills/codex/oo/references/package-execution.md
- contrib/skills/codebuddy/oo/references/llm-client.md
- contrib/skills/codebuddy/oo-publish-skill/SKILL.md
- contrib/skills/openclaw/oo/references/llm-client.md
- contrib/skills/codex/oo-publish-skill/SKILL.md
- contrib/skills/openclaw/oo/references/task-lifecycle.md
- contrib/skills/qoderwork/oo/references/auth-and-billing.md
- contrib/skills/codebuddy/oo/references/file-transfer.md
- contrib/skills/openclaw/oo/references/search-and-selection.md
- contrib/skills/codebuddy/oo/references/package-execution.md
- contrib/skills/qoderwork/oo/references/task-lifecycle.md
- contrib/skills/qoderwork/oo-find-skills/references/oo-cli-contract.md
- contrib/skills/qoderwork/oo-find-skills/SKILL.md
- contrib/skills/codebuddy/oo/references/task-lifecycle.md
- contrib/skills/openclaw/oo-find-skills/SKILL.md
- contrib/skills/codex/oo/references/search-and-selection.md
- contrib/skills/claude/oo-find-skills/references/oo-cli-contract.md
- contrib/skills/codebuddy/oo-find-skills/SKILL.md
- contrib/skills/codebuddy/oo/references/connector-execution.md
- contrib/skills/codex/oo-find-skills/references/oo-cli-contract.md
- contrib/skills/codex/oo-create-skill/SKILL.md
- contrib/skills/codex/oo/references/auth-and-billing.md
- contrib/skills/codex/oo-find-skills/SKILL.md
- contrib/skills/openclaw/oo-publish-skill/SKILL.md
- contrib/skills/qoderwork/oo-create-skill/SKILL.md
- contrib/skills/codex/oo/references/file-transfer.md
- contrib/skills/openclaw/oo-create-skill/SKILL.md
- contrib/skills/qoderwork/oo/references/file-transfer.md
- contrib/skills/openclaw/oo/references/auth-and-billing.md
- contrib/skills/codex/oo/references/connector-execution.md
- contrib/skills/qoderwork/oo/references/llm-client.md
- contrib/skills/openclaw/oo/references/file-transfer.md
- contrib/skills/openclaw/oo/SKILL.md
- contrib/skills/claude/oo-publish-skill/SKILL.md
- contrib/skills/qoderwork/oo/references/package-execution.md
- contrib/skills/codebuddy/oo-create-skill/SKILL.md
- contrib/skills/codebuddy/oo-find-skills/references/oo-cli-contract.md
- contrib/skills/claude/oo/SKILL.md
- contrib/skills/codex/oo/references/llm-client.md
- contrib/skills/codebuddy/oo/SKILL.md
- contrib/skills/qoderwork/oo/references/connector-execution.md
- contrib/skills/codebuddy/oo/references/search-and-selection.md
- contrib/skills/qoderwork/oo/references/search-and-selection.md
- contrib/skills/openclaw/oo/references/connector-execution.md
- contrib/skills/codebuddy/oo/references/auth-and-billing.md
- contrib/skills/qoderwork/oo/SKILL.md
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
1 participant
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.
Bundled Skill Source Consolidation
Requirement
Bundled skills for each agent were previously maintained as separate Markdown
trees under
contrib/skills/<agent>/.... Most of the content was duplicated,which made updates error-prone and required the same wording change to be
propagated across multiple agent-specific files.
The requested architecture change is:
agentic-markdown.agentic-markdownconditionals for host-specific text.agentic-markdownvariables for values such as the target agent id.Example patterns supported by the new source:
oo skills preflight --agent <!-- agentic:var agent -->Architecture Changes
The single source of truth now lives under:
The old duplicated source trees were removed:
src/application/commands/skills/embedded-assets.tsnow imports only sharedskill source files and static Codex agent YAML files from
contrib/skills/shared.Each bundled file is classified as either:
agenticMarkdown: read and rendered throughagentic-markdown.static: copied as-is, currently foragents/openai.yaml.The new
readBundledSkillFileContent(file)function centralizes contentloading. It renders Markdown with:
The install path in
src/application/commands/skills/shared.tswrites renderedcontent instead of copying the raw source file.
Markdown Template Changes
Shared Markdown templates now use
agentic-markdowndirectives for the placeswhere agent-specific copies previously diverged.
Covered differences include:
allowed-toolsfrontmatter.request_user_inputguidance.AskUserQuestionguidance.oo-create-skillreferences to the target agent id and display name.oo-publish-skillhost-specific locate guidance for OpenClaw/QoderWork.Static
agents/openai.yamlfiles remain installed only for Codex agents.Dependency Change
Added runtime dependency:
bun.lockwas updated accordingly.Test Updates
Tests now compare installed bundled skill files against rendered content, not
raw source Markdown.
Key test changes:
embedded-assets.test.tscontrib/skills/shared/<skill>.agentic:directives.index.test.tsandindex.cli.test.tsreadBundledSkillFileContent(...).contrib/ci/npm-packages.test.tssrc/application/commands/skills/__tests__/helpers.tsDocumentation Decision
No
docs/commands*.mdchanges were made. This change affects internal bundledskill source maintenance and installation rendering, not the user-facing CLI
contract, command arguments, options, output shapes, or stable observable
behavior.
Validation
The following checks passed:
bun run lint:fix bun run ts-check bun run testFinal test result:
Review Focus
Please pay particular attention to:
contrib/skills/sharedis now the only intended maintained source.agentic-markdowndirectives.agents/openai.yaml.compared raw source files.
agentic-markdowndependency is acceptable as a runtimedependency for bundled skill installation and compiled binary behavior.