fix(skills): copy skills for non-symlink agents#123
Merged
Conversation
Some supported agents ignore skills when the installed target is a symlink. Centralizing the publication policy keeps bundled, registry, update, startup sync, and local init paths aligned while retaining symlinks only for agents known to support them. Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbitRelease Notes
WalkthroughThis pull request refactors the skill publication mechanism across multiple commands and introduces documentation updates. The changes establish a centralized function 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
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/application/commands/skills/index.test.ts (1)
1421-1436: Avoid hardcoding host-mode groups inline in this integration test.The linked/copied host arrays duplicate the centralized publication policy. Prefer deriving expected mode per host via
resolveManagedSkillPublicationMode(...)and asserting behavior accordingly.♻️ Suggested refactor
+import { resolveManagedSkillPublicationMode } from "./managed-skill-publication.ts";- for (const linkedSkillDirectoryPath of [ - codexSkillDirectoryPath, - claudeSkillDirectoryPath, - qoderWorkSkillDirectoryPath, - ]) { ... } - - for (const copiedSkillDirectoryPath of [ - hermesSkillDirectoryPath, - codeBuddySkillDirectoryPath, - workBuddySkillDirectoryPath, - openClawSkillDirectoryPath, - ]) { ... } +const installedByAgent = { + codex: codexSkillDirectoryPath, + claude: claudeSkillDirectoryPath, + hermes: hermesSkillDirectoryPath, + codebuddy: codeBuddySkillDirectoryPath, + workbuddy: workBuddySkillDirectoryPath, + openclaw: openClawSkillDirectoryPath, + qoderwork: qoderWorkSkillDirectoryPath, +} as const; + +for (const [agentName, installedPath] of Object.entries(installedByAgent)) { + if (resolveManagedSkillPublicationMode(agentName as keyof typeof installedByAgent) === "symlink-or-copy") { + expect(await realpath(installedPath)).toBe(canonicalSkillRealPath); + continue; + } + expect(await realpath(installedPath)).not.toBe(canonicalSkillRealPath); + expect((await lstat(installedPath)).isSymbolicLink()).toBeFalse(); +}Based on learnings: Applies to
**/*.test.{ts,tsx}: When extracting a shared utility from production code, also replace any test helpers or inline expressions that duplicate the same logic in test files.🤖 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 1421 - 1436, The test currently hardcodes which hosts are "linked" vs "copied" (the arrays containing codexSkillDirectoryPath, claudeSkillDirectoryPath, qoderWorkSkillDirectoryPath, etc. and hermesSkillDirectoryPath, codeBuddySkillDirectoryPath, workBuddySkillDirectoryPath, openClawSkillDirectoryPath) which duplicates the publication policy; instead, import and call resolveManagedSkillPublicationMode(hostIdentifier) for each host and derive the expected behavior (linked => expect realpath(...) toBe(canonicalSkillRealPath), copied => expect realpath(...) not toBe(canonicalSkillRealPath) or other assertions used), replacing the two hardcoded for-loops so the test asserts per-host behavior based on resolveManagedSkillPublicationMode instead of inline arrays.src/application/commands/skills/update.test.ts (1)
277-305: Extract duplicated update fetcher/setup into a local test factory.The new Hermes copy-mode test repeats the same package-info/tarball stubbing pattern already used in this file, which makes fixture updates error-prone.
♻️ Suggested refactor
+function createOpenAiUpdateFetcher(version: string, skillMarkdown: string) { + return async (input: RequestInfo | URL, init?: RequestInit) => { + const request = toRequest(input, init); + + if (request.url.includes("/package-info/")) { + return new Response(JSON.stringify({ + packageName: "openai", + version, + skills: [{ description: "Chat with a model", name: "chatgpt", title: "ChatGPT" }], + })); + } + + if (request.url.endsWith(`/openai/-/meta/openai-${version}.tgz`)) { + return new Response(await createRegistrySkillArchiveBytes({ + "package/package/skills/chatgpt/SKILL.md": skillMarkdown, + })); + } + + throw new Error(`Unexpected request: ${request.url}`); + }; +}- fetcher: async (input, init) => { ... } + fetcher: createOpenAiUpdateFetcher("0.0.4", "# ChatGPT fresh\n")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/update.test.ts` around lines 277 - 305, The test duplicates the package-info/tarball fetcher setup used elsewhere; extract that repeated logic into a local factory helper (e.g., createUpdateFetcher or makeUpdateSandbox) inside this test file and have sandbox.run use the returned fetcher/setup instead of inlining it. Locate where sandbox.run(["skills", "update", "chatgpt"], { fetcher: ... }) is used and move the Response stubs (the package-info JSON and the openai-0.0.4.tgz branch that calls createRegistrySkillArchiveBytes with "package/package/skills/chatgpt/SKILL.md") into the new factory function, then call that factory from each test to supply the fetcher (and any other repeated setup) to avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/application/commands/skills/index.test.ts`:
- Around line 1421-1436: The test currently hardcodes which hosts are "linked"
vs "copied" (the arrays containing codexSkillDirectoryPath,
claudeSkillDirectoryPath, qoderWorkSkillDirectoryPath, etc. and
hermesSkillDirectoryPath, codeBuddySkillDirectoryPath,
workBuddySkillDirectoryPath, openClawSkillDirectoryPath) which duplicates the
publication policy; instead, import and call
resolveManagedSkillPublicationMode(hostIdentifier) for each host and derive the
expected behavior (linked => expect realpath(...) toBe(canonicalSkillRealPath),
copied => expect realpath(...) not toBe(canonicalSkillRealPath) or other
assertions used), replacing the two hardcoded for-loops so the test asserts
per-host behavior based on resolveManagedSkillPublicationMode instead of inline
arrays.
In `@src/application/commands/skills/update.test.ts`:
- Around line 277-305: The test duplicates the package-info/tarball fetcher
setup used elsewhere; extract that repeated logic into a local factory helper
(e.g., createUpdateFetcher or makeUpdateSandbox) inside this test file and have
sandbox.run use the returned fetcher/setup instead of inlining it. Locate where
sandbox.run(["skills", "update", "chatgpt"], { fetcher: ... }) is used and move
the Response stubs (the package-info JSON and the openai-0.0.4.tgz branch that
calls createRegistrySkillArchiveBytes with
"package/package/skills/chatgpt/SKILL.md") into the new factory function, then
call that factory from each test to supply the fetcher (and any other repeated
setup) to avoid duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af68e80b-1b14-43d6-af4a-4033cff9a054
📒 Files selected for processing (11)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/skills/auto-sync.tssrc/application/commands/skills/index.test.tssrc/application/commands/skills/init.test.tssrc/application/commands/skills/init.tssrc/application/commands/skills/managed-skill-publication.test.tssrc/application/commands/skills/managed-skill-publication.tssrc/application/commands/skills/registry-skill-publication.tssrc/application/commands/skills/shared.tssrc/application/commands/skills/update.test.ts
This was referenced May 6, 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.
Some supported agents ignore skills when the installed target is a symlink. Centralizing the publication policy keeps bundled, registry, update, startup sync, and local init paths aligned while retaining symlinks only for agents known to support them.