Skip to content

fix(skills): treat aliased agent homes as one skill host - #336

Merged
BlackHole1 merged 2 commits into
mainfrom
fix/aliased-agent-skill-hosts
Aug 11, 2026
Merged

fix(skills): treat aliased agent homes as one skill host#336
BlackHole1 merged 2 commits into
mainfrom
fix/aliased-agent-skill-hosts

Conversation

@BlackHole1

Copy link
Copy Markdown
Member

oo skills add <package> failed with EEXIST: file already exists, mkdir ~/.claude/skills/<skill> on machines where two agent homes point at the same skills directory, such as ~/.claude/skills symlinked 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 an EEXIST from the directory the other had just created.

return Promise.all(
preparedPublication.hostInstallations.map(async (installation) => {
await publishBundledSkillInstallation({
canonicalSkillDirectoryPath: preparedPublication.canonicalSkillDirectoryPath,
installedSkillDirectoryPath: installation.installedSkillDirectoryPath,
});
return {
agentName: installation.agentName,
path: installation.installedSkillDirectoryPath,
};
}),
);

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 info now 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.

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>
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3ae4085b-9439-4967-98b2-9f9e036f60ce

📥 Commits

Reviewing files that changed from the base of the PR and between a72cb31 and 38edc6c.

📒 Files selected for processing (2)
  • src/application/commands/skills/index.test.ts
  • src/application/commands/skills/managed-skill-hosts.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/application/commands/skills/index.test.ts
  • src/application/commands/skills/managed-skill-hosts.test.ts

Summary by CodeRabbit

  • New Features

    • Skill synchronization now recognizes agents sharing the same physical skills directory and installs each skill only once.
    • Skills are reported under the most specific detected agent, while universal hosts yield to concrete agents.
  • Bug Fixes

    • Improved detection for shared and linked skill directories.
    • Prevented duplicate installations during concurrent setup.
  • Documentation

    • Documented shared-directory synchronization behavior in English and Chinese.

Walkthrough

Managed 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
Loading
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title uses the required type(scope): subject format, uses English, and accurately describes the skill-host aliasing fix.
Description check ✅ Passed The description clearly explains the installation failure, the resolver fix, its behavior, documentation updates, and regression coverage.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/aliased-agent-skill-hosts

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 2529e32 and a72cb31.

📒 Files selected for processing (6)
  • docs/commands.md
  • docs/commands.zh-CN.md
  • src/application/commands/skills/__tests__/helpers.ts
  • src/application/commands/skills/index.test.ts
  • src/application/commands/skills/managed-skill-hosts.test.ts
  • src/application/commands/skills/managed-skill-hosts.ts

Comment thread src/application/commands/skills/index.test.ts
Comment thread src/application/commands/skills/index.test.ts Outdated
Comment thread src/application/commands/skills/managed-skill-hosts.test.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>
@BlackHole1
BlackHole1 merged commit 349ca8d into main Aug 11, 2026
7 checks passed
@BlackHole1
BlackHole1 deleted the fix/aliased-agent-skill-hosts branch August 11, 2026 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant