feat: support claude skills#62
Conversation
Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbit
WalkthroughThis PR extends bundled skills support from Codex-only to support multiple agents (Codex and Claude Code). Key changes include: introducing multi-agent path resolution with new functions for Claude home directory detection, updating embedded asset management to track skills per agent, refactoring bundled skill installation/uninstallation/synchronization to handle multiple target hosts, updating documentation and UI messages to remove "Codex" branding, and adding formal skill workflow specifications (SKILL.md and cli-contract.md files) for oo and oo-find-skills with detailed command syntax and control flow definitions. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (3)
src/application/commands/skills/bundled-skill-paths.ts (1)
26-42: Centralize the per-agent mapping.The same
codex/claudekey set is now spread across multipleswitchblocks for home resolution, canonical-root resolution, and ownership-file resolution. A single config record would keep those values in sync and make future agent additions one edit instead of three.As per coding guidelines,
Consolidate multiple switch/map structures that share the same keys into a single configuration object (data-driven over parallel mappings).Also applies to: 51-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/bundled-skill-paths.ts` around lines 26 - 42, Centralize the per-agent mapping by introducing a single configuration record (e.g., a const agentConfig: Record<BundledSkillAgentName, { directoryName: string, ... }>) that maps each BundledSkillAgentName ("claude", "codex", etc.) to its directory/metadata, then refactor resolveClaudeHomeDirectory, resolveCodexHomeDirectory and resolveBundledSkillHomeDirectory to read from that config instead of using switch blocks; do the same replacement for the canonical-root and ownership-file resolution functions so all agent-specific values come from the single config object.src/application/commands/skills/embedded-assets.ts (1)
107-111: Build this source path withjoin().Hard-coding
contrib/skills/${agentName}/${skillName}bakes POSIX separators into a path helper. Usenode:pathhere so Windows callers and snapshots get a native path form.As per coding guidelines, `Never assume POSIX path separators in code, tests, snapshots, or assertions; use node:path helpers (join(), resolve(), relative()) for path construction`.♻️ Suggested change
+import { join } from "node:path"; + export function getBundledSkillSourceDirectory( skillName: BundledSkillName, agentName: BundledSkillAgentName = "codex", ): string { - return `contrib/skills/${agentName}/${skillName}`; + return join("contrib", "skills", agentName, skillName); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/embedded-assets.ts` around lines 107 - 111, The function getBundledSkillSourceDirectory hard-codes POSIX separators; change it to build the path with node's path join to produce native separators on Windows and POSIX: import join from "node:path" (or use path.join) and return join("contrib", "skills", agentName, skillName) instead of the template literal; keep the same function name, signature, and return type.docs/commands.zh-CN.md (1)
190-199: Keep the command reference user-facing.These additions document private canonical directories,
.oo-metadata.json, andagents/openai.yaml. The supported host locations and observable install/uninstall behavior belong here; the internal staging/layout details should move to developer-facing docs so the CLI contract stays stable.As per coding guidelines,
Documentation under docs/commands*.md must describe only user-facing CLI contract (command purpose, arguments, options, stable output shapes, externally observable behavior) without internal implementation details such as validator order, AJV usage, schema patching, or internal lint mechanics.Also applies to: 208-210, 254-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/commands.zh-CN.md` around lines 190 - 199, The docs section currently exposes internal staging/layout details (private canonical directories, `.oo-metadata.json`, `agents/openai.yaml`, and internal canonical release paths) that should be moved to developer-facing docs; update the text in the commands reference to only state user-facing observable behavior: list supported host install locations (e.g., `${CODEX_HOME:-~/.codex}/skills/<skill-id>` and `~/.claude/skills/<skill-id>`), and describe what users can observe during install/uninstall (where published skills end up and any visible CLI outputs), and remove or relocate mentions of internal canonical directories and internal release paths (the lines calling out `<config-dir>/skills/<skill-id>`, `<config-dir>/claude-skills/<skill-id>`, `.oo-metadata.json`, and `agents/openai.yaml`); apply the same change to the other affected blocks referenced (the sections around 208-210 and 254-259).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contrib/skills/claude/oo-find-skills/SKILL.md`:
- Line 7: The chooser that selects AskUserQuestion (referenced in the
interactive chooser around the chooser block) is unreachable because
allowed-tools only contains "Bash(oo *)"; update the allowed-tools entry to also
include AskUserQuestion (or a regex that matches it, e.g., "AskUserQuestion" or
"AskUserQuestion(.*)") so the chooser can invoke AskUserQuestion instead of
falling back to plain-text; modify the allowed-tools array to include that
symbol and verify the chooser block (the interactive chooser at lines 114-117)
can select it.
In `@src/application/commands/skills/shared.ts`:
- Around line 318-325: The current check using
resolveAvailableBundledSkillHostInstallations drops agents whose host home no
longer exists and then throws createMissingBundledSkillHostError, preventing
uninstall of managed canonical copies; update the resolver or add an alternative
resolution path so uninstall logic always includes the agent-specific canonical
installation path even when the host root is missing: modify
resolveAvailableBundledSkillHostInstallations (and the caller logic that
currently throws createMissingBundledSkillHostError) to return entries for
canonical agent paths regardless of whether the host home exists (or provide a
new function like resolveBundledSkillHostInstallationsAllowMissingHost) and use
that in the uninstall flow so cleanup still runs when ~/.codex or ~/.claude is
deleted (also apply the same change for the related code around the 490-523
range).
---
Nitpick comments:
In `@docs/commands.zh-CN.md`:
- Around line 190-199: The docs section currently exposes internal
staging/layout details (private canonical directories, `.oo-metadata.json`,
`agents/openai.yaml`, and internal canonical release paths) that should be moved
to developer-facing docs; update the text in the commands reference to only
state user-facing observable behavior: list supported host install locations
(e.g., `${CODEX_HOME:-~/.codex}/skills/<skill-id>` and
`~/.claude/skills/<skill-id>`), and describe what users can observe during
install/uninstall (where published skills end up and any visible CLI outputs),
and remove or relocate mentions of internal canonical directories and internal
release paths (the lines calling out `<config-dir>/skills/<skill-id>`,
`<config-dir>/claude-skills/<skill-id>`, `.oo-metadata.json`, and
`agents/openai.yaml`); apply the same change to the other affected blocks
referenced (the sections around 208-210 and 254-259).
In `@src/application/commands/skills/bundled-skill-paths.ts`:
- Around line 26-42: Centralize the per-agent mapping by introducing a single
configuration record (e.g., a const agentConfig: Record<BundledSkillAgentName, {
directoryName: string, ... }>) that maps each BundledSkillAgentName ("claude",
"codex", etc.) to its directory/metadata, then refactor
resolveClaudeHomeDirectory, resolveCodexHomeDirectory and
resolveBundledSkillHomeDirectory to read from that config instead of using
switch blocks; do the same replacement for the canonical-root and ownership-file
resolution functions so all agent-specific values come from the single config
object.
In `@src/application/commands/skills/embedded-assets.ts`:
- Around line 107-111: The function getBundledSkillSourceDirectory hard-codes
POSIX separators; change it to build the path with node's path join to produce
native separators on Windows and POSIX: import join from "node:path" (or use
path.join) and return join("contrib", "skills", agentName, skillName) instead of
the template literal; keep the same function name, signature, and 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: 504d0e42-27e4-43ab-b653-3ce84f7e44e6
⛔ Files ignored due to path filters (2)
src/application/commands/skills/__snapshots__/index.cli.test.ts.snapis excluded by!**/*.snapsrc/application/commands/skills/config/__snapshots__/set.cli.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (22)
README-ZH_CN.mdREADME.mdcontrib/skills/claude/oo-find-skills/SKILL.mdcontrib/skills/claude/oo-find-skills/references/oo-cli-contract.mdcontrib/skills/claude/oo/SKILL.mdcontrib/skills/claude/oo/references/oo-cli-contract.mddocs/commands.mddocs/commands.zh-CN.mdsrc/application/bootstrap/run-cli.test.tssrc/application/commands/skills/__tests__/helpers.tssrc/application/commands/skills/bundled-skill-model.tssrc/application/commands/skills/bundled-skill-observation.test.tssrc/application/commands/skills/bundled-skill-observation.tssrc/application/commands/skills/bundled-skill-paths.tssrc/application/commands/skills/config/set.test.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.tssrc/application/commands/skills/update.test.tssrc/i18n/catalog.ts
| Search the published OOMOL skill catalog, compare candidate skills, and | ||
| install chosen results using the oo CLI. Do not use for generic skill | ||
| discovery outside the oo or OOMOL ecosystem. | ||
| allowed-tools: ["Bash(oo *)"] |
There was a problem hiding this comment.
AskUserQuestion is unreachable with the current allowlist.
Line 7 only whitelists Bash(oo *), so the chooser on Lines 114-117 can never use AskUserQuestion and this skill will always drop to the plain-text fallback. If the interactive chooser is intended, it needs to be added to allowed-tools.
Also applies to: 114-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contrib/skills/claude/oo-find-skills/SKILL.md` at line 7, The chooser that
selects AskUserQuestion (referenced in the interactive chooser around the
chooser block) is unreachable because allowed-tools only contains "Bash(oo *)";
update the allowed-tools entry to also include AskUserQuestion (or a regex that
matches it, e.g., "AskUserQuestion" or "AskUserQuestion(.*)") so the chooser can
invoke AskUserQuestion instead of falling back to plain-text; modify the
allowed-tools array to include that symbol and verify the chooser block (the
interactive chooser at lines 114-117) can select it.
| const installations = await resolveAvailableBundledSkillHostInstallations( | ||
| context, | ||
| skillName, | ||
| ); | ||
| const installedSkillDirectoryExists = await directoryExists(skillDirectoryPath); | ||
| const installedSkillMetadata = installedSkillDirectoryExists | ||
| ? await readInstalledBundledSkillMetadata(skillDirectoryPath) | ||
| : undefined; | ||
|
|
||
| if (!canUninstallManagedBundledSkillInstallation({ | ||
| installedDirectoryExists: installedSkillDirectoryExists, | ||
| installedDirectoryManaged: installedSkillMetadata !== undefined, | ||
| })) { | ||
| context.logger.warn( | ||
| { | ||
| path: skillDirectoryPath, | ||
| skillName, | ||
| }, | ||
| "Bundled Codex skill uninstall skipped because no managed installation was found.", | ||
|
|
||
| if (installations.length === 0) { | ||
| throw createMissingBundledSkillHostError(context.env); | ||
| } |
There was a problem hiding this comment.
Uninstall stops working once a host home is removed.
Lines 503-505 drop any agent whose home directory no longer exists, and Lines 323-325 turn that into errors.skills.noSupportedBundledSkillHosts. That makes the managed canonical copies impossible to uninstall after a user deletes ~/.codex or ~/.claude, and it also skips cleanup for a missing host when another host still exists. Uninstall needs a resolver that always includes the agent-specific canonical path, even if the host root is gone.
Also applies to: 490-523
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/application/commands/skills/shared.ts` around lines 318 - 325, The
current check using resolveAvailableBundledSkillHostInstallations drops agents
whose host home no longer exists and then throws
createMissingBundledSkillHostError, preventing uninstall of managed canonical
copies; update the resolver or add an alternative resolution path so uninstall
logic always includes the agent-specific canonical installation path even when
the host root is missing: modify resolveAvailableBundledSkillHostInstallations
(and the caller logic that currently throws createMissingBundledSkillHostError)
to return entries for canonical agent paths regardless of whether the host home
exists (or provide a new function like
resolveBundledSkillHostInstallationsAllowMissingHost) and use that in the
uninstall flow so cleanup still runs when ~/.codex or ~/.claude is deleted (also
apply the same change for the related code around the 490-523 range).
1 participant
No description provided.