Skip to content

fix(cli): install global skills in symlink mode outside Windows - #5706

Open
kumanday wants to merge 1 commit into
heygen-com:mainfrom
kumanday:fix/cli-skills-symlink-install
Open

kumanday wants to merge 1 commit into
heygen-com:mainfrom
kumanday:fix/cli-skills-symlink-install

Conversation

@kumanday

Copy link
Copy Markdown

What

hyperframes skills, skills update and init stop passing --copy to npx skills add on macOS and Linux. The global install then uses upstream's symlink mode:

  • real files in ~/.agents/skills/<skill>;
  • a relative symlink ~/.claude/skills/<skill> -> ../../.agents/skills/<skill>.

Windows keeps --copy.

Why

With --copy, every HyperFrames skill is written twice as real files, to ~/.claude/skills and to ~/.agents/skills.

  • Drift between agents. Claude Code reads the first store; Codex and Pi read the second. A local edit or fix to one copy changes behavior in one agent only. Nothing reports the mismatch until the next skills update overwrites both.
  • Double storage for every installed skill.
  • Mixed layouts. Users who manage other skills with the upstream default get two layouts in the same directory, and each update replaces their links with copies.

--copy was added in #1744 because the symlink layout then rewrote each SKILL.md's frontmatter, so skills check reported a fresh install as outdated. Current upstream skills (1.7.2, which npx skills resolves to today) only rewrites SKILL.md for its eve agent. Symlink-mode installs now byte-match the published bundle.

Related work

Fixes #5704.

Refs #1744, which introduced --copy.

The issue also suggests sourcing mirrorGlobalSkills from ~/.agents/skills rather than ~/.claude/skills. This PR leaves that for a follow-up (see How).

How

  • packages/cli/src/commands/skills.ts: the constant GLOBAL_INSTALL_ARGS_TAIL becomes globalInstallArgsTail(platform), which adds --copy only on win32. Windows symlinks need admin or Developer Mode, the same reason skillsMirror.ts copies there.
  • packages/cli/src/utils/skillsMirror.ts: comments only. The mirror still reads from the Claude store. In symlink mode each entry there is a link, and the mirror's symlink checks compare realpaths, so they work through the links unchanged. Sourcing the mirror from ~/.agents/skills directly would avoid chained links; I left that out to keep this change small.
  • Migration needs no code. Upstream's createSymlink removes a real directory at the link path before linking, so an existing copy-mode skill turns into a link the next time it's reinstalled.

Test plan

  • Unit tests added/updated: packages/cli/src/commands/skills.test.ts builds the expected arguments per platform. linux and darwin omit --copy and win32 keeps it, and the skills update test asserts no --copy on Linux. npx vitest run src/commands/skills.test.ts src/utils/skillsMirror.test.ts: 59 passed.
  • Manual testing performed, with the patched CLI (bun src/cli.ts) and HOME set to a scratch directory:
    • skills update cloned from GitHub and installed 10 skills as real files in ~/.agents/skills, with relative symlinks in ~/.claude/skills. skills check then reported "10 current".
    • Bare skills installed the remaining 11; skills check reported "21 current".
    • Migration: I replaced ~/.claude/skills/hyperframes-core with a real copy and changed one byte so it read as outdated. skills update reinstalled it as a symlink, and skills check reported "21 current".
    • On a real machine, skills check also reported 13 hand-symlinked skills as current, since hashSkillBundle reads through the links.
  • oxfmt --check, oxlint, scripts/check-comment-citations.mjs and scripts/comment-ratchet.mjs pass on the changed files.
  • Documentation updated: no docs describe the install layout beyond the changelog entry for fix(cli): install skills with --copy so check sees a faithful, correctly-placed set #1744.
  • Comments follow CONTRIBUTING.md "Comments"

--copy wrote every skill twice as real files, to ~/.claude/skills and
~/.agents/skills, so an edit to one copy changed one agent and not the other.
Current upstream skills keeps symlink-mode files byte-identical to the source,
which removes the reason for --copy added in heygen-com#1744. Outside Windows the install
now keeps real files in ~/.agents/skills and a relative link per skill in
~/.claude/skills. Windows keeps --copy, matching the mirror's fallback.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The code is correct and I have no change requests. The open question is whether we want this default change: it reverses the --copy choice from #1744, so on macOS and Linux every global install becomes one real copy in ~/.agents/skills plus a link from ~/.claude/skills. That needs a maintainer decision, not a code fix.

What I checked:

  • packages/cli/src/commands/skills.ts:109 adds --copy only on win32, and :137 is the only caller, so init, bare skills and skills update all pick it up. No other code path passes --copy. The only remaining mentions are in the changelog and release notes, which describe history.
  • Tests: vitest run src/commands/skills.test.ts src/utils/skillsMirror.test.ts gives 59 passed. With skills.ts reverted to main, the linux, darwin and skills update cases fail (3 of 40), so the tests pin the new behaviour.
  • The mirror now reads from a store whose entries are links (skillsMirror.ts:285, :295). I ran a throwaway test: ~/.claude/skills/hyperframes linked to ../../.agents/skills/hyperframes, with Cursor and Codex marker dirs, on linux. The Cursor entry resolves and reads the right SKILL.md, nothing lands in skipped, and a second run is idempotent. So the chained link ~/.cursor/skills/x -> ~/.claude/skills/x -> ~/.agents/skills/x works with the existing realpath safety checks, as the PR says.

Worth knowing before merging:

  • npx skills is not pinned. The byte-match with the manifest now depends on upstream still rewriting SKILL.md only for eve. If upstream starts rewriting frontmatter in symlink mode again, skills check will show fresh installs as outdated, which is the #1744 failure. --copy protected against that, and this PR removes that protection. That was already true for other upstream behaviour, but this one decides whether skills check stays green.
  • Pointing the mirror at ~/.agents/skills directly, as the issue suggests, would remove the extra hop. That's fine as a follow-up.

— Rames

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The head hasn't changed since my last review at 568c05ba, and the code is still correct. Two things have moved since then.

First, the branch now conflicts with main. I ran a trial merge against current main (8a84bedd): skills.ts and skillsMirror.ts merge cleanly, and the conflict is only in packages/cli/src/commands/skills.test.ts, where #6168 and #5651 reworked the same argument tables. Could you rebase and carry globalArgsTail(platform) into the new test cases? The --copy block in skills.ts on main is still the one this PR replaces, so the change itself still applies.

Second, the question from my last review still stands and is for the maintainers rather than you: this reverses the --copy default from #1744 on macOS and Linux. npx skills isn't pinned, so whether a fresh install still reads as current in skills check now depends on upstream continuing to leave SKILL.md alone outside eve. If we're happy with that trade, this is good to go after the rebase.

Need: yes for the drift between the two stores (#5704), pending a maintainer call on the default · Size: ok · Reuse: follows the same win32 copy fallback skillsMirror.ts already uses · Simpler: already minimal; sourcing the mirror from ~/.agents/skills is a fine follow-up

— Rames

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Adding to Rames's two reviews here; he already covered the mirror, the tests and the rebase.

I checked the main claim against the current upstream package, since the whole change rests on it. In skills 1.7.4 (the latest published, a little newer than the 1.7.2 you tested), SKILL.md is only rewritten for the eve agent. Symlink-mode installs for claude-code and universal leave it byte-for-byte. Two other upstream behaviours also work in this PR's favour:

  • An existing real directory at the link path is removed before linking, so old --copy installs migrate on the next update, as you described.
  • If creating the link fails (for example on a filesystem without symlinks), upstream falls back to copying rather than erroring.

One small note: on Windows, upstream links directories with junctions, which don't need admin or Developer Mode. So the reason given in the comment at skills.ts:104 doesn't quite apply to upstream's install step. Keeping --copy on Windows is a fine conservative choice; the comment could just say that instead.

The rest is as Rames said: please rebase onto main (the conflict is only in skills.test.ts). The switch away from --copy is a maintainer call. CI hasn't run on this head yet.

Verdict: COMMENT
Reasoning: The code is correct and the upstream behaviour it relies on checks out at the latest version, but the branch conflicts with main, CI has not run, and the default change still needs a maintainer decision.

This branch has not been deployed

No deployments
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.

skills: install globally in symlink mode now that upstream skills keeps files byte-identical

3 participants