fix(cli): install skills with --copy so check sees a faithful, correctly-placed set#1744
Conversation
…tly-placed set `hyperframes init` / `skills` / `skills update` shelled out to `skills add` without `--copy`, so the upstream CLI installed via its canonical `.agents/skills` store + per-agent symlinks. That layout re-serialises each SKILL.md's frontmatter, so an installed bundle no longer byte-matches the published manifest — `skills check` reported a freshly-installed set as outdated — and it didn't reliably land in the dir the agent reads (e.g. `.claude/skills`). Pass `--copy` in runSkillsAdd (the single chokepoint every install flows through: bare `skills`, `update`, and `init` via installAllSkills) so real files are written into each agent's skills dir — faithful to the manifest and correctly placed. Verified end-to-end: a fresh-content install now reads all-current in `.claude/skills` (was all-outdated in `agent/skills`). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
miga-heygen
left a comment
There was a problem hiding this comment.
LGTM — clean, well-scoped fix.
What I checked:
-
The chokepoint claim holds.
runSkillsAddis the single spawn point forskills add. Both callers — barehyperframes skills(viainstallAllSkills()with noextraArgs, so the["--all"]default kicks in) andskills update(viainstallAllSkills({ extraArgs: ["--all", "--yes"] })) — now unconditionally get--copyappended at the end. No path through the code installs without it. -
Flag ordering is safe.
--copyappends after the spread ofextraArgs, so theupdatepath produces["skills", "add", "<url>", "--all", "--yes", "--copy"]— all valid positional/flag ordering for the upstreamskillsCLI. -
Tests updated correctly. All three platform variants (linux, darwin, win32) in the
it.eachmatrix now assert--copyin the expected install args. The win32cmd.exepath correctly includes it inside the/d /s /c npx.cmd ...wrapper. -
No runtime side effects. The change doesn't alter env handling (
GIT_CLONE_PROTECTION_ACTIVE), timeout, stdio, error handling, or thestrictpropagation contract. Theskills removeprune path (from #1740) is untouched, as claimed. -
Comment quality. The inline comment in
runSkillsAddis excellent — explains the why (frontmatter re-serialisation breaks byte-matching, symlink layout misplaces files) rather than the what, and ties it back to the observable failure (skills checkfalse positives). Good signal-to-noise ratio.
One minor observation (not blocking): the PR body mentions a docs follow-up for README / docs/guides/skills.mdx to recommend --copy in manual examples — worth tracking as a follow-up issue so it doesn't get lost.
CI is still running at time of review (several checks pending), but the change is a single flag addition to a spawn argv with no new imports, no type changes, and no control-flow alterations — low risk of CI surprises.
Reviewed at 28ff927.
— Miga
jrusso1020
left a comment
There was a problem hiding this comment.
LGTM — no blockers. Small, well-scoped, and the inline comment explains the why clearly. Two things I verified independently:
--copyis a real flag at the pinned version. #1743 just pinned us tovercel-labs/skills@v1.5.13, and--copyexists there —src/cli.ts:141("Copy files instead of symlinking to agent directories"), backed byInstallMode = 'symlink' | 'copy'ininstaller.ts. So this won't break the install path against the version we pin.- Single-chokepoint claim holds.
runSkillsAddis the onlyskills addspawn site in the package, soinit,skills, andskills updateall pick up--copyfrom this one change; the #1740 prune path is untouched.
Good coherence fix for the #1738/#1740 freshness system — real copies are byte-faithful to the published manifest, so skills check stops false-positiving on a freshly-installed set. Tests updated across linux/darwin/win32.
— Jerrai
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed current head 28ff9277.
Specific checks:
packages/cli/src/commands/skills.ts:56-67appends--copyatrunSkillsAdd, the singleskills addchokepoint used by barehyperframes skills,hyperframes initviainstallAllSkills, andhyperframes skills update.packages/cli/src/commands/skills.test.ts:107-149pins the install argv on linux, darwin, and win32, including thecmd.exewrapper path.- I also merge-simulated this PR onto current
main(13b115e, after #1743). The merge is clean and preserves #1743'supdate --source/--dirremoved-detection work; the combined delta is only the intended--copyflag + test expectations. - Verified the upstream
skillsCLI help exposes--copy, so this is not passing an unknown flag.
No blockers. Miga and Rames covered the same chokepoint/flag-validity surface; this is additive confirmation after all CI completed green.
Verdict: APPROVE
Reasoning: The change fixes the install-format mismatch at the only install spawn site, the tests cover platform argv construction, and the branch composes cleanly with the latest main skills-update changes.
— Magi
What
hyperframes init / skills / skills update now pass --copy to the upstream skills add. Added in runSkillsAdd — the single chokepoint every install funnels through (installAllSkills → runSkillsAdd), so all three paths are covered with one change; the skills remove prune path from #1740 is untouched.
Effect: installs now write real files into each agent's skills dir — byte-faithful to the published skills-manifest.json, and in the directory the agent actually reads.
Why
Without --copy, the upstream skills CLI installs via its canonical .agents/skills store plus per-agent symlinks. That layout re-serialises each SKILL.md's frontmatter (drops name:, reflows the folded description: > into a quoted scalar), so an installed bundle no longer byte-matches the manifest. Two user-facing failures fall out — both independent of the skills.sh registry lag (they reproduce on freshly-cloned, fully-current content):
skills check reports a brand-new install as entirely outdated— a permanent false positive;updatecan't clear it (re-install reproduces the same rewritten layout), socheck || updatenever converges.
Skills don't land where the agent reads(e.g.agent/skillsinstead of.claude/skills), so Claude Code may not pick them up.
This is the install-side complement to #1738 / #1740: detection is sound, but the default install format was defeating it.
How
One-line change in runSkillsAdd: append --copy to the skills add argv. --copy copies real files into each target agent's dir instead of symlinking to the shared canonical store — sidestepping the frontmatter re-serialisation. Agent targeting unchanged (--agent claude-code when CLAUDECODE, upstream auto-detect otherwise); --copy only changes how files are written, not where.
Test plan
scenariobefore (no --copy)after (--copy)init, fresh content (forced clone of main)0 current / 19 outdated @ agent/skills19 / 0 @ .claude/skillsinit, real (via skills.sh)0 / 19 @ agent/skills13 / 6 @ .claude/skills — the 6 are genuine skills.sh lag, unrelated
skills.test.tsinstall-arg assertions expect--copy(linux/darwin/win32); suite 10/10
oxlint/oxfmt --check/tsc/fallow auditclean
Docs follow-up: README /docs/guides/skills.mdxnpx skills addexamples should also recommend--copy