feat(skills): publish registry skills to all hosts#110
Merged
Conversation
Registry skills now follow the same host-aware installation model as bundled skills. This keeps Codex, Claude Code, and OpenClaw targets consistent during install, update, and uninstall, while rejecting unmanaged same-name directories before publication. Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbit
WalkthroughThis PR converts skills command handling from a Codex-focused model to a multi-host "AI Agent Skills" model (Codex, Claude Code, OpenClaw). It adds a managed-skill-hosts abstraction and exports Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI Command
participant Resolver as Host Resolver
participant Registry as Registry/Package
participant Validator as Validator
participant Publisher as Publisher
participant FS as File System
User->>CLI: oo skills install <skill>
CLI->>Resolver: resolveAvailableManagedSkillHosts(env)
Resolver-->>CLI: list of ManagedSkillHosts
CLI->>Registry: fetch package metadata & tarball
Registry-->>CLI: skill metadata & archive
loop for each ManagedSkillHost
CLI->>Validator: validateRegistrySkillPublicationTargets(hostInstallations)
alt target exists & lacks managed metadata
Validator-->>CLI: throw CliUserError (name conflict)
else
Validator-->>CLI: ok to publish
end
end
loop for each ManagedSkillHost
CLI->>Publisher: publishPreparedRegistrySkillPublication(hostInstallations)
alt agentName == "openclaw"
Publisher->>FS: copy files to installedSkillDirectoryPath
else
Publisher->>FS: symlink-or-copy to installedSkillDirectoryPath
end
Publisher->>FS: write .oo-metadata.json
Publisher-->>CLI: RegistrySkillPublicationResult (agentName, status)
end
CLI->>User: report per-host publication results
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/commands.zh-CN.md (1)
285-316:⚠️ Potential issue | 🟡 MinorKeep this reference at the CLI contract level.
These new bullets document canonical storage layout and symlink/copy publication mechanics. That is internal implementation detail rather than stable user-visible behavior, so it makes the command reference harder to keep accurate and accidentally turns internals into compatibility promises.
As per coding guidelines,
docs/commands*.mdshould describe only user-facing CLI contract (purpose, arguments, options, output shapes, observable behavior), not internal implementation details.Also applies to: 337-360
🤖 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 285 - 316, The new bullets reveal internal implementation (e.g., "canonical 目录", explicit paths like `<config-dir>/skills/bundled/<agent>/<skill-id>`, "安装方式" describing symlink vs copy, and hidden file `.oo-metadata.json`) which shouldn't appear in CLI contract docs; remove or rewrite those bullets so docs only state user-facing, observable behavior (what the command does, required flags like `--skill`/`--all -y`, that auth is sent, upgrade/rollback effects) without internal layout/path or implementation details, and move any necessary implementation specifics to internal/developer docs; apply the same edits to the corresponding section referenced (lines 337-360) so both places only present stable CLI contract information.src/application/commands/skills/registry-skill-install.ts (1)
386-449:⚠️ Potential issue | 🟠 MajorDon't route unmanaged targets through the overwrite-confirmation flow.
readRegistrySkillInstallStatus()collapses “directory exists but is not oo-managed” into the genericconflictstate, so explicit installs either prompt for overwrite or fail witherrors.skills.install.confirmationRequiredin non-TTY mode.validateRegistrySkillPublicationTargets()later rejects those same targets anyway, so this branch can never lead to a successful install. Unmanaged host targets should surfaceerrors.skills.nameConflictdirectly here.Also applies to: 488-525
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/registry-skill-install.ts` around lines 386 - 449, The conflict branch in filterConfirmedSkillNames should not route unmanaged targets through the overwrite confirmation flow; instead detect unmanaged targets (using the existing validateRegistrySkillPublicationTargets helper or equivalent validation logic) immediately after readRegistrySkillInstallStatus returns "conflict" and throw a CliUserError with "errors.skills.nameConflict" (including the name) for unmanaged targets; only if the target is a managed conflict proceed with interactive confirmation as currently implemented. Apply the same change to the analogous block later (lines noted in the review).
🧹 Nitpick comments (2)
src/application/commands/skills/update.test.ts (1)
110-207: Add OpenClaw coverage to the new multi-host update regression.This test now proves the Codex + Claude path, but
skills updatealso republishes to OpenClaw. Since OpenClaw uses a different publication mode from the symlink-first hosts, a host-specific update regression can still pass this suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/update.test.ts` around lines 110 - 207, The test only covers Codex and Claude hosts but omits OpenClaw, so add OpenClaw to the setup and assertions: create openClaw home via resolveOpenClawHomeDirectory(sandbox.env), set openclawInstalledSkillDirectoryPath = join(openClawHomeDirectory, "skills", "chatgpt"), mkdir that directory in the same Promise.all that makes codex/claude dirs, write an initial SKILL.md and metadata (renderSkillMetadataJson version "0.0.3") to that OpenClaw path before running sandbox.run, then update the expected stdout to include the OpenClaw update message (similar to the Codex/Claude lines) and add assertions that resolveManagedSkillMetadataFilePath(openclawInstalledSkillDirectoryPath) equals the "0.0.4" metadata and that SKILL.md contents at join(openclawInstalledSkillDirectoryPath, "SKILL.md") contain "# ChatGPT fresh" (mirror the checks used for claude/codex).src/application/commands/skills/registry-skill-publication.ts (1)
109-142: Consider reporting all conflicting targets, not just the first one.The validation function uses
find()at line 130-132, which stops at the first unmanaged target. If multiple hosts have conflicting unmanaged directories, the user will only see one error at a time. This could lead to a frustrating fix-and-retry cycle.Consider collecting all conflicts and reporting them together so users can resolve all issues in one pass.
♻️ Proposed fix to report all conflicts
- const unmanagedTarget = targetStates.find( - target => target.installedDirectoryExists && target.metadata === undefined, + const unmanagedTargets = targetStates.filter( + target => target.installedDirectoryExists && target.metadata === undefined, ); - if (unmanagedTarget === undefined) { + if (unmanagedTargets.length === 0) { return; } throw new CliUserError("errors.skills.nameConflict", 1, { name: options.skillName, - path: unmanagedTarget.installation.installedSkillDirectoryPath, + path: unmanagedTargets + .map(t => t.installation.installedSkillDirectoryPath) + .join(", "), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/registry-skill-publication.ts` around lines 109 - 142, The current validateRegistrySkillPublicationTargets function only reports the first unmanaged target because it uses find(); change this to collect all unmanaged targets by filtering targetStates for entries where installedDirectoryExists is true and metadata is undefined (e.g., const unmanagedTargets = targetStates.filter(...)), and if unmanagedTargets.length > 0 throw the CliUserError including all conflicting installation paths (map unmanagedTargets to their installation.installedSkillDirectoryPath) and any other useful metadata (like names) so the error payload reports all conflicts at once instead of a single path. Ensure you update the thrown error payload in validateRegistrySkillPublicationTargets accordingly.
🤖 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/update.ts`:
- Around line 271-308: The merge currently treats entries with metadata:
undefined as managed; update mergeManagedSkillInstallationsByName (and the same
logic at the other occurrence) to exclude entries whose metadata is exactly
undefined so only parseable .oo-metadata.json counts as managed, and when
performing the host fallback check (in readKnownManagedSkillInstallations flow /
resolveSelectedManagedSkills) use readManagedSkillMetadata() to re-read/validate
host metadata instead of relying on fileExists() so corrupted installs aren’t
treated as managed.
---
Outside diff comments:
In `@docs/commands.zh-CN.md`:
- Around line 285-316: The new bullets reveal internal implementation (e.g.,
"canonical 目录", explicit paths like
`<config-dir>/skills/bundled/<agent>/<skill-id>`, "安装方式" describing symlink vs
copy, and hidden file `.oo-metadata.json`) which shouldn't appear in CLI
contract docs; remove or rewrite those bullets so docs only state user-facing,
observable behavior (what the command does, required flags like `--skill`/`--all
-y`, that auth is sent, upgrade/rollback effects) without internal layout/path
or implementation details, and move any necessary implementation specifics to
internal/developer docs; apply the same edits to the corresponding section
referenced (lines 337-360) so both places only present stable CLI contract
information.
In `@src/application/commands/skills/registry-skill-install.ts`:
- Around line 386-449: The conflict branch in filterConfirmedSkillNames should
not route unmanaged targets through the overwrite confirmation flow; instead
detect unmanaged targets (using the existing
validateRegistrySkillPublicationTargets helper or equivalent validation logic)
immediately after readRegistrySkillInstallStatus returns "conflict" and throw a
CliUserError with "errors.skills.nameConflict" (including the name) for
unmanaged targets; only if the target is a managed conflict proceed with
interactive confirmation as currently implemented. Apply the same change to the
analogous block later (lines noted in the review).
---
Nitpick comments:
In `@src/application/commands/skills/registry-skill-publication.ts`:
- Around line 109-142: The current validateRegistrySkillPublicationTargets
function only reports the first unmanaged target because it uses find(); change
this to collect all unmanaged targets by filtering targetStates for entries
where installedDirectoryExists is true and metadata is undefined (e.g., const
unmanagedTargets = targetStates.filter(...)), and if unmanagedTargets.length > 0
throw the CliUserError including all conflicting installation paths (map
unmanagedTargets to their installation.installedSkillDirectoryPath) and any
other useful metadata (like names) so the error payload reports all conflicts at
once instead of a single path. Ensure you update the thrown error payload in
validateRegistrySkillPublicationTargets accordingly.
In `@src/application/commands/skills/update.test.ts`:
- Around line 110-207: The test only covers Codex and Claude hosts but omits
OpenClaw, so add OpenClaw to the setup and assertions: create openClaw home via
resolveOpenClawHomeDirectory(sandbox.env), set
openclawInstalledSkillDirectoryPath = join(openClawHomeDirectory, "skills",
"chatgpt"), mkdir that directory in the same Promise.all that makes codex/claude
dirs, write an initial SKILL.md and metadata (renderSkillMetadataJson version
"0.0.3") to that OpenClaw path before running sandbox.run, then update the
expected stdout to include the OpenClaw update message (similar to the
Codex/Claude lines) and add assertions that
resolveManagedSkillMetadataFilePath(openclawInstalledSkillDirectoryPath) equals
the "0.0.4" metadata and that SKILL.md contents at
join(openclawInstalledSkillDirectoryPath, "SKILL.md") contain "# ChatGPT fresh"
(mirror the checks used for claude/codex).
🪄 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: 837cc0b7-f737-49e1-b92c-baecb1217997
⛔ Files ignored due to path filters (3)
src/application/bootstrap/__snapshots__/run-cli.test.ts.snapis excluded by!**/*.snapsrc/application/commands/config/__snapshots__/index.cli.test.ts.snapis excluded by!**/*.snapsrc/application/commands/skills/__snapshots__/index.cli.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/skills/index.test.tssrc/application/commands/skills/list.tssrc/application/commands/skills/managed-skill-hosts.tssrc/application/commands/skills/registry-skill-install.tssrc/application/commands/skills/registry-skill-publication.tssrc/application/commands/skills/shared.tssrc/application/commands/skills/update.test.tssrc/application/commands/skills/update.tssrc/i18n/catalog.ts
Treat installed skills whose metadata cannot be parsed or has no managed version as unmanaged during skills update. This prevents broken local installations from being selected during all-target updates. Include host names in missing package metadata errors so explicit update failures identify every affected host. Signed-off-by: Kevin Cui <bh@bugs.cc>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/application/commands/skills/update.ts (1)
513-522: Reuse resolved host-installation sets instead of recomputing them.
resolveManagedSkillHostInstallations(...)is recomputed multiple times for the same skill in the same update flow. Cache per-skill host installations once inprepareRegistrySkillGroupUpdateand pass them through to validation/preparation/current checks.As per coding guidelines "Compute an expression once and store in a variable for reuse, especially inside
switchstatements and loops (no duplicate computations)".Also applies to: 539-549, 566-569, 608-611
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/update.ts` around lines 513 - 522, prepareRegistrySkillGroupUpdate is recomputing resolveManagedSkillHostInstallations(...) for the same skill multiple times; cache the resolved host-installation set per skill inside prepareRegistrySkillGroupUpdate and pass that cached value into the downstream checks (e.g., into isRegistrySkillCurrentInAllHosts and the validation/preparation/current-check routines) instead of calling resolveManagedSkillHostInstallations repeatedly. Update signatures or local call sites so the cached installations map (keyed by skill.name) is used for all occurrences in the update flow (including the blocks that currently call resolveManagedSkillHostInstallations for validation, preparation and current-state checks) to eliminate duplicate computation.
🤖 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/update.ts`:
- Around line 317-337: The current merge conflates skills with the same
skill.name but different non-empty metadata.packageName values by merging
hostNames into a single skillsByName entry; change the logic in the update loop
that uses mergeManagedSkillHostNames, existingSkill, skill and skillsByName so
that when both existingSkill.metadata.packageName and skill.metadata.packageName
are defined and are different you do NOT merge them into one entry — instead
create distinct entries (e.g., key by a composite identifier like
`${skill.name}::${packageName}` or otherwise append the packageName to the map
key) so each package-specific skill keeps its own hostNames and metadata;
preserve the existing behavior only when existing packageName is undefined and
incoming packageName is defined as currently implemented.
---
Nitpick comments:
In `@src/application/commands/skills/update.ts`:
- Around line 513-522: prepareRegistrySkillGroupUpdate is recomputing
resolveManagedSkillHostInstallations(...) for the same skill multiple times;
cache the resolved host-installation set per skill inside
prepareRegistrySkillGroupUpdate and pass that cached value into the downstream
checks (e.g., into isRegistrySkillCurrentInAllHosts and the
validation/preparation/current-check routines) instead of calling
resolveManagedSkillHostInstallations repeatedly. Update signatures or local call
sites so the cached installations map (keyed by skill.name) is used for all
occurrences in the update flow (including the blocks that currently call
resolveManagedSkillHostInstallations for validation, preparation and
current-state checks) to eliminate duplicate computation.
🪄 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: 1270bdd6-6243-4665-acb7-f4faa0b4ff57
📒 Files selected for processing (5)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/skills/update.test.tssrc/application/commands/skills/update.tssrc/i18n/catalog.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/application/commands/skills/update.test.ts
- docs/commands.md
This was referenced Apr 28, 2026
This was referenced May 15, 2026
This was referenced May 23, 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
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.
Registry skills now follow the same host-aware installation model as bundled skills. This keeps Codex, Claude Code, and OpenClaw targets consistent during install, update, and uninstall, while rejecting unmanaged same-name directories before publication.