feat(skills)!: migrate legacy canonical skill layout#96
Merged
Conversation
Canonical skills now live under dedicated bundled and registry roots. Installing skills removes legacy directories first so stale copies cannot conflict with the new layout after users upgrade. BREAKING CHANGE: existing registry skill installations under the legacy canonical layout are deleted during migration and must be reinstalled with `oo skills install <packageName>`. Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbit
WalkthroughThis PR refactors the canonical directory structure for bundled and published skills, introducing a new nested layout where bundled skills are stored under Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 4
🧹 Nitpick comments (3)
src/application/commands/skills/legacy-canonical-migration.ts (1)
134-156:readDirectoryIfPresentover-fetches; prefer EAFP.The function calls
readdir(directoryPath)purely as an existence probe and discards the entries. Per coding guidelines, "Use EAFP over LBYL for file operations; attempt the operation and catch errors rather than pre-checking existence." You could either:
- Replace with a lightweight
stat()probe, or- Drop the probe entirely and call
removePathdirectly (it already swallows ENOENT), emitting theRemoved legacy canonical skill directorylog only when the path existed (e.g., via a pre-lstat/statcheck inside the removal loop or by propagating a boolean fromremovePath).As per coding guidelines: "Use EAFP (Easier to Ask for Forgiveness than Permission) over LBYL (Look Before You Leap) for file operations; attempt the operation and catch errors rather than pre-checking existence."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/legacy-canonical-migration.ts` around lines 134 - 156, The helper readDirectoryIfPresent currently calls readdir(directoryPath) just to probe existence and discards the entries (LBYL); change it to follow EAFP by either 1) replacing the probe with a lightweight lstat/stat check (use fs.lstat or fs.stat) to determine existence before logging/removal, or 2) drop the pre-check entirely and call removePath(directoryPath) directly (removePath already swallows ENOENT) and only emit the "Removed legacy canonical skill directory" log when removePath indicates the path was removed (propagate a boolean or perform a minimal lstat inside removePath). Update references to readDirectoryIfPresent, readdir, removePath, isNodeNotFoundError, and logger.warn accordingly so we no longer over-fetch directory entries.src/application/commands/skills/legacy-canonical-migration.test.ts (2)
164-200: Consider also asserting that theclaude-skillsfile remains untouched.The "logs a warning and continues" test writes a regular file at
legacyClaudePathto trigger an inspection failure, then verifies the healthyopenclaw-skillscandidate is still removed. It would be slightly stronger to also assert that the file atlegacyClaudePathis not deleted (migration should only log and skip), which locks in the swallow-and-continue contract more explicitly.💚 Suggested addition
await expect(stat(legacyOpenClawPath)).rejects.toMatchObject({ code: "ENOENT", }); + await expect(stat(legacyClaudePath)).resolves.toMatchObject({ + isFile: expect.any(Function), + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/legacy-canonical-migration.test.ts` around lines 164 - 200, Add an assertion that the file at legacyClaudePath remains untouched after calling migrateLegacyCanonicalSkillLayout: after verifying openclaw-skills was removed and after logCapture.read(), assert the file still exists and/or its contents equal "not a directory\n" (e.g. await expect(stat(legacyClaudePath)).resolves.toBeDefined() or await expect(Bun.file(legacyClaudePath).text()).resolves.toEqual("not a directory\n")). Reference the existing legacyClaudePath variable and the migrateLegacyCanonicalSkillLayout invocation and place this check before the final cleanup block so the test ensures the claude-skills file is not deleted by the migration.
240-243: Nit:serializeJsonPathdrops the surrounding quotes but not the inner ones for edge cases.
JSON.stringify(path).slice(1, -1)is fine for typical filesystem paths but breaks if a path ever contains a"(it would leave an unbalanced escaped quote in the slice output). For current test inputs this isn't reachable, so it's just a robustness note for future changes — e.g., serialize then pass the full quoted form totoContainto sidestep the slice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/legacy-canonical-migration.test.ts` around lines 240 - 243, serializeJsonPath currently strips the leading/trailing quotes with JSON.stringify(path).slice(1, -1), which breaks if the path contains a double-quote; change serializeJsonPath to return the full JSON.stringify(path) (preserving proper escaping) and update all test assertions that call serializeJsonPath (e.g., usages passed to toContain) to expect the quoted/escaped form (i.e., pass the returned value from serializeJsonPath directly into toContain or adjust the assertion to match the quoted string).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/commands.md`:
- Around line 297-302: Update the migration note to accurately reflect behavior
and scope: state that cleanup occurs on the next or each `oo skills install`
(instead of "first run after upgrading") or document that the implementation can
be gated behind a marker file (e.g., a migration-flag) to ensure one-time
execution; explicitly warn that the cleanup will remove any child directory
under the config-dir `skills/` other than `bundled/` and `registry/` and advise
users to back up custom content, or change the described behavior to only delete
directories that contain a parseable `.oo-metadata.json` (i.e., treat only
validated oo skills as removable). Ensure the note references `oo skills
install`, the `skills/` directory, `bundled/` and `registry/`, and
`.oo-metadata.json` so readers know the exact scope and destructive risk.
In `@src/application/commands/skills/index.test.ts`:
- Around line 1440-1496: The test doesn't assert that the legacy bundled skills
child (legacyCodexBundledPath) is removed by the migration; add an assertion
after running sandbox.run(["skills", "install"]) that the legacy bundled path
has been deleted (e.g., await
expect(stat(legacyCodexBundledPath)).rejects.toMatchObject({ code: "ENOENT" })),
or instead create and assert removal of a non-colliding legacy marker (e.g.,
create join(configDirectoryPath, "skills", "chatgpt-legacy") using
resolveBundledSkillMetadataFilePath/renderSkillMetadataJson before running
install and then expect stat(...) to reject) so the test verifies the
legacySkillsChild cleanup behavior referenced by the migration logic.
In `@src/application/commands/skills/install.ts`:
- Around line 55-56: The migration function migrateLegacyCanonicalSkillLayout is
being invoked on every skills install; change it so the handler only runs the
migration once by adding a persisted marker check: in the install command
handler, before calling migrateLegacyCanonicalSkillLayout(context), read a small
persisted flag (e.g., config/store key like "legacyCanonicalMigrationDone") and
only invoke migrateLegacyCanonicalSkillLayout if the flag is absent, then set
the flag after a successful run; ensure the flag is stored in the same
persistent store used by the app (or a stable config file) so subsequent
installs skip the migration.
In `@src/application/commands/skills/legacy-canonical-migration.ts`:
- Around line 102-132: readLegacySkillsChildren currently returns all
directory/symlink entries under codex skills (except bundled/registry) which
later leads to unconditional removePath deletions; change it to only return
entries that contain a parseable .oo-metadata.json (i.e., replicate the
ownership check used by the uninstall flow) by, for each candidate name from
readLegacySkillsChildren, checking for the presence and successful parse of
join(skillsDirectoryPath, name, ".oo-metadata.json") before including it in the
result so only oo-managed legacy skill folders are returned and later removed.
---
Nitpick comments:
In `@src/application/commands/skills/legacy-canonical-migration.test.ts`:
- Around line 164-200: Add an assertion that the file at legacyClaudePath
remains untouched after calling migrateLegacyCanonicalSkillLayout: after
verifying openclaw-skills was removed and after logCapture.read(), assert the
file still exists and/or its contents equal "not a directory\n" (e.g. await
expect(stat(legacyClaudePath)).resolves.toBeDefined() or await
expect(Bun.file(legacyClaudePath).text()).resolves.toEqual("not a
directory\n")). Reference the existing legacyClaudePath variable and the
migrateLegacyCanonicalSkillLayout invocation and place this check before the
final cleanup block so the test ensures the claude-skills file is not deleted by
the migration.
- Around line 240-243: serializeJsonPath currently strips the leading/trailing
quotes with JSON.stringify(path).slice(1, -1), which breaks if the path contains
a double-quote; change serializeJsonPath to return the full JSON.stringify(path)
(preserving proper escaping) and update all test assertions that call
serializeJsonPath (e.g., usages passed to toContain) to expect the
quoted/escaped form (i.e., pass the returned value from serializeJsonPath
directly into toContain or adjust the assertion to match the quoted string).
In `@src/application/commands/skills/legacy-canonical-migration.ts`:
- Around line 134-156: The helper readDirectoryIfPresent currently calls
readdir(directoryPath) just to probe existence and discards the entries (LBYL);
change it to follow EAFP by either 1) replacing the probe with a lightweight
lstat/stat check (use fs.lstat or fs.stat) to determine existence before
logging/removal, or 2) drop the pre-check entirely and call
removePath(directoryPath) directly (removePath already swallows ENOENT) and only
emit the "Removed legacy canonical skill directory" log when removePath
indicates the path was removed (propagate a boolean or perform a minimal lstat
inside removePath). Update references to readDirectoryIfPresent, readdir,
removePath, isNodeNotFoundError, and logger.warn accordingly so we no longer
over-fetch directory entries.
🪄 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: 1e11ffcd-bb45-416c-9c95-f2d3b97573be
📒 Files selected for processing (9)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/skills/bundled-skill-paths.tssrc/application/commands/skills/index.test.tssrc/application/commands/skills/install.tssrc/application/commands/skills/legacy-canonical-migration.test.tssrc/application/commands/skills/legacy-canonical-migration.tssrc/application/commands/skills/managed-skill-paths.test.tssrc/application/commands/skills/managed-skill-paths.ts
Comment on lines
+297
to
+302
| - Migration: on first run after upgrading, `oo skills install` removes legacy | ||
| canonical directories left over from earlier releases (`claude-skills/`, | ||
| `openclaw-skills/`, and any Codex-bundled or registry skill directory that | ||
| lived directly under `skills/`). Bundled skills are rebuilt automatically in | ||
| the new layout; previously-installed published skills must be reinstalled | ||
| with `oo skills install <packageName>`. |
There was a problem hiding this comment.
Clarify migration scope and destructiveness.
Two user-facing concerns worth addressing:
- The note says "on first run after upgrading," but the implementation runs the cleanup on every
oo skills install(idempotently). Minor accuracy issue — either rephrase to "on nextoo skills install" or gate the migration behind a marker. - The cleanup deletes any child directory under
<config-dir>/skills/other thanbundled/andregistry/, without inspecting.oo-metadata.json. Users who placed custom, non-oo content under that path (e.g., hand-crafted skills, backups) will lose it silently. Consider documenting this scope explicitly so users can back up before upgrading, or limiting deletion to entries that contain a parseable.oo-metadata.json.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/commands.md` around lines 297 - 302, Update the migration note to
accurately reflect behavior and scope: state that cleanup occurs on the next or
each `oo skills install` (instead of "first run after upgrading") or document
that the implementation can be gated behind a marker file (e.g., a
migration-flag) to ensure one-time execution; explicitly warn that the cleanup
will remove any child directory under the config-dir `skills/` other than
`bundled/` and `registry/` and advise users to back up custom content, or change
the described behavior to only delete directories that contain a parseable
`.oo-metadata.json` (i.e., treat only validated oo skills as removable). Ensure
the note references `oo skills install`, the `skills/` directory, `bundled/` and
`registry/`, and `.oo-metadata.json` so readers know the exact scope and
destructive risk.
Comment on lines
+1440
to
+1496
| test("migrates legacy canonical skill layout on install", async () => { | ||
| const sandbox = await createCliSandbox(); | ||
| const codexHomeDirectory = resolveCodexHomeDirectory(sandbox.env); | ||
| const ooSkillDirectoryPath = join(codexHomeDirectory, "skills", "oo"); | ||
| const storePaths = resolveStorePaths({ | ||
| appName: APP_NAME, | ||
| env: sandbox.env, | ||
| platform: process.platform, | ||
| }); | ||
| const configDirectoryPath = join(storePaths.settingsFilePath, ".."); | ||
| const legacyCodexBundledPath = join(configDirectoryPath, "skills", "oo"); | ||
| const legacyRegistryPath = join(configDirectoryPath, "skills", "chatgpt"); | ||
| const legacyClaudeRoot = join(configDirectoryPath, "claude-skills"); | ||
| const legacyOpenClawRoot = join(configDirectoryPath, "openclaw-skills"); | ||
| const newOoCanonicalPath = resolveBundledSkillCanonicalDirectoryPath( | ||
| storePaths.settingsFilePath, | ||
| "oo", | ||
| ); | ||
|
|
||
| try { | ||
| await mkdir(codexHomeDirectory, { recursive: true }); | ||
| await mkdir(legacyCodexBundledPath, { recursive: true }); | ||
| await Bun.write( | ||
| resolveBundledSkillMetadataFilePath(legacyCodexBundledPath), | ||
| renderSkillMetadataJson({ version: "0.0.1" }), | ||
| ); | ||
| await mkdir(legacyRegistryPath, { recursive: true }); | ||
| await Bun.write( | ||
| join(legacyRegistryPath, ".oo-metadata.json"), | ||
| renderSkillMetadataJson({ packageName: "foo", version: "0.0.2" }), | ||
| ); | ||
| await mkdir(join(legacyClaudeRoot, "oo"), { recursive: true }); | ||
| await mkdir(join(legacyOpenClawRoot, "oo"), { recursive: true }); | ||
|
|
||
| const result = await sandbox.run(["skills", "install"], { | ||
| version: "9.9.9", | ||
| }); | ||
|
|
||
| expect(result.exitCode).toBe(0); | ||
| expect(result.stderr).toBe(""); | ||
| await expect(stat(legacyClaudeRoot)).rejects.toMatchObject({ | ||
| code: "ENOENT", | ||
| }); | ||
| await expect(stat(legacyOpenClawRoot)).rejects.toMatchObject({ | ||
| code: "ENOENT", | ||
| }); | ||
| await expect(stat(legacyRegistryPath)).rejects.toMatchObject({ | ||
| code: "ENOENT", | ||
| }); | ||
| expect(await realpath(ooSkillDirectoryPath)).toBe( | ||
| await realpath(newOoCanonicalPath), | ||
| ); | ||
| } | ||
| finally { | ||
| await sandbox.cleanup(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Minor test gap: legacyCodexBundledPath removal is not asserted.
The test sets up legacyCodexBundledPath = join(configDirectoryPath, "skills", "oo") at line 1450 with a stale .oo-metadata.json, but only asserts ENOENT for legacyClaudeRoot, legacyOpenClawRoot, and legacyRegistryPath. Since the migration's "legacySkillsChild" cleanup is one of the most behaviorally sensitive paths (it deletes bare children under skills/), consider asserting it here too.
💚 Suggested addition
await expect(stat(legacyRegistryPath)).rejects.toMatchObject({
code: "ENOENT",
});
+ await expect(stat(legacyCodexBundledPath)).rejects.toMatchObject({
+ code: "ENOENT",
+ });
expect(await realpath(ooSkillDirectoryPath)).toBe(Note: after install, <config>/skills/oo will exist again as the symlinked target, so this assertion would only be valid if placed before reinstall or if asserting on a different legacy entry name. An alternative is to use a third-party name (e.g., chatgpt-legacy) as the bundled legacy marker to verify generic child removal without collision against the reinstalled bundled skill.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/application/commands/skills/index.test.ts` around lines 1440 - 1496, The
test doesn't assert that the legacy bundled skills child
(legacyCodexBundledPath) is removed by the migration; add an assertion after
running sandbox.run(["skills", "install"]) that the legacy bundled path has been
deleted (e.g., await
expect(stat(legacyCodexBundledPath)).rejects.toMatchObject({ code: "ENOENT" })),
or instead create and assert removal of a non-colliding legacy marker (e.g.,
create join(configDirectoryPath, "skills", "chatgpt-legacy") using
resolveBundledSkillMetadataFilePath/renderSkillMetadataJson before running
install and then expect stat(...) to reject) so the test verifies the
legacySkillsChild cleanup behavior referenced by the migration logic.
Comment on lines
55
to
+56
| handler: async (input, context) => { | ||
| await migrateLegacyCanonicalSkillLayout(context); |
There was a problem hiding this comment.
Migration runs on every install, not just post-upgrade.
migrateLegacyCanonicalSkillLayout(context) executes at the start of every skills install invocation. This is functionally harmless because the migration is idempotent (no legacy paths ⇒ no-op, proven by legacy-canonical-migration.test.ts), but it diverges from the docs that describe a "first run after upgrading" semantic. Either the docs should be loosened to "on install (idempotent cleanup)" or the migration should be gated behind a persisted marker so it only runs once.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/application/commands/skills/install.ts` around lines 55 - 56, The
migration function migrateLegacyCanonicalSkillLayout is being invoked on every
skills install; change it so the handler only runs the migration once by adding
a persisted marker check: in the install command handler, before calling
migrateLegacyCanonicalSkillLayout(context), read a small persisted flag (e.g.,
config/store key like "legacyCanonicalMigrationDone") and only invoke
migrateLegacyCanonicalSkillLayout if the flag is absent, then set the flag after
a successful run; ensure the flag is stored in the same persistent store used by
the app (or a stable config file) so subsequent installs skip the migration.
Comment on lines
+102
to
+132
| async function readLegacySkillsChildren( | ||
| configDirectoryPath: string, | ||
| logger: Pick<Logger, "warn">, | ||
| ): Promise<string[]> { | ||
| const skillsDirectoryPath = join(configDirectoryPath, codexSkillsDirectoryName); | ||
|
|
||
| try { | ||
| const entries = await readdir(skillsDirectoryPath, { withFileTypes: true }); | ||
|
|
||
| return entries | ||
| .filter(entry => entry.isDirectory() || entry.isSymbolicLink()) | ||
| .map(entry => entry.name) | ||
| .filter(name => | ||
| name !== canonicalBundledSkillsDirectoryName | ||
| && name !== canonicalRegistrySkillsDirectoryName, | ||
| ); | ||
| } | ||
| catch (error) { | ||
| if (!isNodeNotFoundError(error)) { | ||
| logger.warn( | ||
| { | ||
| err: error, | ||
| path: skillsDirectoryPath, | ||
| }, | ||
| "Failed to inspect legacy canonical skills directory.", | ||
| ); | ||
| } | ||
|
|
||
| return []; | ||
| } | ||
| } |
There was a problem hiding this comment.
Migration deletes non-oo content under <config>/skills/ without a metadata check.
readLegacySkillsChildren returns every directory/symlink child except bundled and registry, and each is then unconditionally removePath'd. There is no .oo-metadata.json ownership check analogous to the one enforced during uninstall (see docs/commands.md ownership rule for bundled/registry). Consequences:
- A user who previously placed unrelated content under
<config>/skills/<name>/(e.g., manual experiments, external tooling output, backups) will lose it silently on the nextoo skills install. - The PR description only calls out deletion of legacy registry installations, but behavior is broader.
Recommend filtering candidates to only those containing a parseable .oo-metadata.json (matching the existing ownership semantics elsewhere in this module). This keeps the migration targeted at oo-managed legacy state and aligns with the BREAKING CHANGE scope as described.
♻️ Sketch
- return entries
- .filter(entry => entry.isDirectory() || entry.isSymbolicLink())
- .map(entry => entry.name)
- .filter(name =>
- name !== canonicalBundledSkillsDirectoryName
- && name !== canonicalRegistrySkillsDirectoryName,
- );
+ const candidateNames = entries
+ .filter(entry => entry.isDirectory() || entry.isSymbolicLink())
+ .map(entry => entry.name)
+ .filter(name =>
+ name !== canonicalBundledSkillsDirectoryName
+ && name !== canonicalRegistrySkillsDirectoryName,
+ );
+ // Only treat directories owned by oo (i.e. with parseable metadata)
+ // as legacy canonical candidates; preserves unrelated user content.
+ const ownedNames: string[] = [];
+ for (const name of candidateNames) {
+ if (await hasOoMetadata(join(skillsDirectoryPath, name))) {
+ ownedNames.push(name);
+ }
+ }
+ return ownedNames;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/application/commands/skills/legacy-canonical-migration.ts` around lines
102 - 132, readLegacySkillsChildren currently returns all directory/symlink
entries under codex skills (except bundled/registry) which later leads to
unconditional removePath deletions; change it to only return entries that
contain a parseable .oo-metadata.json (i.e., replicate the ownership check used
by the uninstall flow) by, for each candidate name from
readLegacySkillsChildren, checking for the presence and successful parse of
join(skillsDirectoryPath, name, ".oo-metadata.json") before including it in the
result so only oo-managed legacy skill folders are returned and later removed.
This was referenced May 9, 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.
Canonical skills now live under dedicated bundled and registry roots. Installing skills removes legacy directories first so stale copies cannot conflict with the new layout after users upgrade.
BREAKING CHANGE: existing registry skill installations under the legacy canonical layout are deleted during migration and must be reinstalled with
oo skills install <packageName>.