#2710

fix: refuse link-only skill folders and keep locked folders on update

Opensmorimotowants to merge intofrom

Why

A skill folder made of symlinks into another folder, such as packages/cli/.cursor/skills/sentry-cli in getsentry/toolkit, installs as an empty directory. The copier skips every link that leaves the skill directory (#2589, #2652), discovery still reads the SKILL.md through the link, and the installer reports success and locks the hash of nothing. skills update then made it worse for a lock entry that pointed at the real folder. Discovery never reaches that folder, so update read the entry as a relocation to the link folder and overwrote the real files with the same empty copy. The goal is an honest failure that leaves the existing install and the lock entry alone.

What changed

  • installSkillForAgent checks, before it removes the previous install, that SKILL.md will be copied. A missing, broken, or outward-linking SKILL.md fails that target with SKILLS_SKILL_MD_NOT_COPYABLE, and add exits non-zero when a target fails, as its JSON mode already did.
  • discoverSkills takes knownSkillDirs, and both update scopes pass the locked folders, so a skill that still lives at its locked path is current rather than relocated.

Scope

  • One outward link among real files still installs with a warning, as before. Only a SKILL.md that would not be copied fails the install.
  • skills use shares the copier and still materializes such a skill as an empty folder. Left for a follow-up.
  • A lock entry that already points at the link folder now fails on update with the reason instead of printing "Updated". Reinstall from the real folder to recover.

Tradeoffs

The check runs before the copy instead of staging the copy and swapping it in. That keeps the existing install intact without new filesystem semantics, and the copier and the check share one link rule.

Blast Radius

Installs from clones, local paths, sync, and restore. A folder whose SKILL.md is missing or links outside the skill now fails per target instead of installing an empty folder. add now exits non-zero after any failed target, whatever the reason, where it used to exit zero after printing the failure. Update for GitHub and generic Git sources now also checks the locked folders, so a lock entry can no longer move to a same-name folder while its own folder still exists.

Verification

  • pnpm dev add getsentry/toolkit --skill sentry-cli -y -a cursor fails with the SKILL.md reason, exit code 1, no folder, no lock entry.
  • With the lock at packages/cli/plugins/sentry-cli/skills/sentry-cli/SKILL.md and its 43 files on disk, skills check and skills update -y report up to date and leave the files and the lock unchanged.
  • With the old broken state (lock at the link folder, empty install), skills update -y fails with the reason, exit code 1, lock unchanged.
  • The first commit adds the tests. On main, the two refusal cases in tests/installer-skill-md-symlink.test.ts, all three cases in tests/update-locked-path.test.ts, and the knownSkillDirs case in tests/full-depth-discovery.test.ts fail. The third installer case, a real SKILL.md beside an outward references link, passes on main by design and still passes.
  • pnpm type-check and pnpm format:check pass. The full suite on this machine has 14 failures, all of which also fail on clean main (Cursor agent detection under an agent session, invalid-local-lock, and 5 s timeouts under load). The timeout files pass when run alone.