fix(skills): treat aliased agent homes as one skill host - #336
Conversation
Two supported agents can point at the same physical skills directory, for example `~/.claude/skills` symlinked to `~/.agents/skills`. Host resolution treated them as separate install targets, so publishing a registry skill copied into that one directory from two concurrent tasks: one removed the target while the other was writing it, and the second copy failed with `EEXIST: file already exists, mkdir <path>`. Startup synchronization carried the same race, where it only surfaced as a logged warning. Hosts are now collapsed by the real path of their skills directory, falling back to the resolved home directory while that directory does not exist yet, so an aliased home collapses before its first publication. The always-provisioned universal host yields to a concrete agent sharing its directory, because bundled skill content is rendered per agent and the concrete agent's rendering is the more specific one. Signed-off-by: Kevin Cui <bh@bugs.cc>
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughManaged skill host discovery now deduplicates hosts that resolve to the same physical skills directory. Concrete agents take precedence over the universal host. The resolution logic falls back to home-directory or literal paths when real paths are unavailable. Tests cover symbolic links, aliased homes, host precedence, concurrent installation, and shared skill content. English and Chinese documentation describe the synchronization behavior. Sequence Diagram(s)sequenceDiagram
participant ManagedSkillHosts
participant FileSystem
participant SkillInstaller
ManagedSkillHosts->>FileSystem: resolve shared skills directories
FileSystem-->>ManagedSkillHosts: return physical or fallback paths
ManagedSkillHosts->>SkillInstaller: provide deduplicated hosts
SkillInstaller-->>ManagedSkillHosts: install the skill once per shared directory
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/application/commands/skills/index.test.ts`:
- Around line 430-462: Extract the repeated registry fetcher mock into a local
factory function at the bottom of index.test.ts, reusing the existing metadata,
archive, and download-count response behavior. Parameterize the factory with an
optional request observer and update both test setups around the existing
fetcher definitions to call it instead of duplicating the mock logic.
- Around line 20-24: Remove the local createDirectorySymbolicLinkForTest
declaration from the test module and rely on the imported helper from
"./__tests__/helpers.ts"; preserve the existing call sites to use the shared
implementation.
In `@src/application/commands/skills/managed-skill-hosts.test.ts`:
- Around line 47-50: Extract the repeated temporary-directory and environment
setup from the managed-host tests into a local factory function at the bottom of
the test file. Have the factory create the root directory, build the
HOME/USERPROFILE environment object, and return the root directory, environment,
and cleanup function; update the affected tests to use this factory.
🪄 Autofix
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: 1e77ad8e-e350-4b9c-b2d8-c455c30f5c6f
📒 Files selected for processing (6)
docs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/skills/__tests__/helpers.tssrc/application/commands/skills/index.test.tssrc/application/commands/skills/managed-skill-hosts.test.tssrc/application/commands/skills/managed-skill-hosts.ts
The registry fetcher mock and the managed-host temporary directory setup were each written out twice, and the project's test guidance puts repeated setup in a local factory at the bottom of the file. The fetcher factory takes an optional request sink so the one test that asserts the request sequence keeps doing so. Signed-off-by: Kevin Cui <bh@bugs.cc>
oo skills add <package>failed withEEXIST: file already exists, mkdir ~/.claude/skills/<skill>on machines where two agent homes point at the same skills directory, such as~/.claude/skillssymlinked to~/.agents/skills. Host resolution reported them as two install targets, and publication fans out over targets concurrently, so both tasks removed and recopied that one directory at the same time. Whichever copy lost the race hit anEEXISTfrom the directory the other had just created.oo-cli/src/application/commands/skills/registry-skill-publication.ts
Lines 91 to 103 in 2529e32
Hosts are now collapsed by the real path of their skills directory. A host without one yet falls back to its resolved home directory, so an aliased home collapses before its first publication, and a path that cannot be resolved at all stays separate as before. When an alias does collide, the always-provisioned universal host yields to the concrete agent sharing its directory, because bundled skill content is rendered per agent and the concrete agent's rendering is the more specific one.
Startup synchronization ran the same race, where it only surfaced as a logged warning, and the single resolver change covers it. The visible side effect is that
oo skills infonow reports a shared directory under one agent instead of listing the same physical copy twice, which docs/commands.md and its Chinese counterpart now describe. The new end-to-end test installs a registry skill into two aliased homes and exits 1 on the old resolver.