Repository navigation
Conversation
--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
left a comment
There was a problem hiding this comment.
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:109adds--copyonly onwin32, and:137is the only caller, soinit, bareskillsandskills updateall 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.tsgives 59 passed. Withskills.tsreverted tomain, the linux, darwin andskills updatecases 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/hyperframeslinked to../../.agents/skills/hyperframes, with Cursor and Codex marker dirs, onlinux. The Cursor entry resolves and reads the rightSKILL.md, nothing lands inskipped, and a second run is idempotent. So the chained link~/.cursor/skills/x -> ~/.claude/skills/x -> ~/.agents/skills/xworks with the existingrealpathsafety checks, as the PR says.
Worth knowing before merging:
npx skillsis not pinned. The byte-match with the manifest now depends on upstream still rewritingSKILL.mdonly foreve. If upstream starts rewriting frontmatter in symlink mode again,skills checkwill show fresh installs as outdated, which is the #1744 failure.--copyprotected against that, and this PR removes that protection. That was already true for other upstream behaviour, but this one decides whetherskills checkstays green.- Pointing the mirror at
~/.agents/skillsdirectly, as the issue suggests, would remove the extra hop. That's fine as a follow-up.
— Rames
jrusso1020
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
--copyinstalls 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.
What
hyperframes skills,skills updateandinitstop passing--copytonpx skills addon macOS and Linux. The global install then uses upstream's symlink mode:~/.agents/skills/<skill>;~/.claude/skills/<skill> -> ../../.agents/skills/<skill>.Windows keeps
--copy.Why
With
--copy, every HyperFrames skill is written twice as real files, to~/.claude/skillsand to~/.agents/skills.skills updateoverwrites both.--copywas added in #1744 because the symlink layout then rewrote eachSKILL.md's frontmatter, soskills checkreported a fresh install as outdated. Current upstreamskills(1.7.2, whichnpx skillsresolves to today) only rewritesSKILL.mdfor itseveagent. Symlink-mode installs now byte-match the published bundle.Related work
Fixes #5704.
Refs #1744, which introduced
--copy.The issue also suggests sourcing
mirrorGlobalSkillsfrom~/.agents/skillsrather than~/.claude/skills. This PR leaves that for a follow-up (see How).How
packages/cli/src/commands/skills.ts: the constantGLOBAL_INSTALL_ARGS_TAILbecomesglobalInstallArgsTail(platform), which adds--copyonly onwin32. Windows symlinks need admin or Developer Mode, the same reasonskillsMirror.tscopies 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 comparerealpaths, so they work through the links unchanged. Sourcing the mirror from~/.agents/skillsdirectly would avoid chained links; I left that out to keep this change small.createSymlinkremoves 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
packages/cli/src/commands/skills.test.tsbuilds the expected arguments per platform.linuxanddarwinomit--copyandwin32keeps it, and theskills updatetest asserts no--copyon Linux.npx vitest run src/commands/skills.test.ts src/utils/skillsMirror.test.ts: 59 passed.bun src/cli.ts) andHOMEset to a scratch directory:skills updatecloned from GitHub and installed 10 skills as real files in~/.agents/skills, with relative symlinks in~/.claude/skills.skills checkthen reported "10 current".skillsinstalled the remaining 11;skills checkreported "21 current".~/.claude/skills/hyperframes-corewith a real copy and changed one byte so it read as outdated.skills updatereinstalled it as a symlink, andskills checkreported "21 current".skills checkalso reported 13 hand-symlinked skills as current, sincehashSkillBundlereads through the links.oxfmt --check,oxlint,scripts/check-comment-citations.mjsandscripts/comment-ratchet.mjspass on the changed files.