feat(init): sync installed skills with the manifest - #8556
domitriusclark wants to merge 7 commits into
Conversation
Removes unedited deprecated skills, migrates unedited copies under a prior name to the current name, and cleans up staging and backup directories left by an interrupted install. Edited copies are kept and reported; --reset-context replaces, migrates or deletes them. A directory that stops early still reports what it finished. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The occupant check now scans the directory itself, so a case twin without SKILL.md can no longer be migrated over on reset, and the renamed path never forces. Staging directories younger than ten minutes are left to the run that owns them, and the assembled tree must match the manifest tree_hash before it is swapped in. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Drops the duplicate classification (an unknown directory is left alone either way), the restore-a-backup branch (a missing skill is reinstalled and the backup then removed as an unedited release), and scopes the --reset-context hint to copies reset would act on. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
commit: |
| await fs.mkdir(path.dirname(output), { recursive: true }) | ||
| await fs.writeFile(output, bytes, { mode: executable.has(file) ? 0o755 : 0o644 }) | ||
| } | ||
| const stagedHash = await hashSkillTree(staged) |
There was a problem hiding this comment.
| } | ||
| return skill | ||
| } | ||
| const stillUnedited = async (name: string, skill: ManifestSkill): Promise<boolean> => |
There was a problem hiding this comment.
[consider] Nothing tests this re-check. Every edited fixture is already marked modified at classification and gets kept before this runs, so stillUnedited could always return true and the suite would still pass.
It's the guard behind "re-hashed right before the rm" in the description, so a regression here would go unnoticed.
There was a problem hiding this comment.
Added in 10c72c4: a stale skill's download writes an edit into netlify-legacy mid-run, and the deprecated copy is kept with the edit intact. The record was unedited at classification, so only the re-hash can produce that outcome.
| await writeSkill(skillsDir, '.netlify-skill-netlify-deploy-Ab12Cd', { 'SKILL.md': '# half written\n' }) | ||
| await writeSkill(skillsDir, 'netlify-functions.old-123-0123456789ab', FUNCTIONS_V1) | ||
| await writeSkill(skillsDir, 'netlify-deploy.old-123-0123456789ab', DEPLOY.files, DEPLOY.executable) | ||
| await writeSkill(skillsDir, '.netlify-skill-someone-else-Ab12Cd', { 'SKILL.md': '# not ours\n' }) |
There was a problem hiding this comment.
[consider] This one survives because it's fresh, not because someone-else isn't a skill name. The known-name check on staging leftovers could be dropped and this test would still pass.
There was a problem hiding this comment.
Fixed in 10c72c4. The someone-else staging dir is aged past the guard too, so the manifest-name check is what protects it.
| const { actions } = await syncSkills({ host: HOST, directory: skillsDir, manifest, reset: true }) | ||
|
|
||
| await expect(readFile(join(skillsDir, 'Netlify-Deploy', 'notes.md'), 'utf8')).resolves.toBe('# keep me\n') | ||
| await expect(listDirectories(skillsDir)).resolves.toContain('netlify-cli-and-deploy') |
There was a problem hiding this comment.
[blocking] This only holds on a case-insensitive filesystem, so it fails both Linux unit jobs.
On Linux Netlify-Deploy isn't a twin, so reset installs netlify-deploy beside it and migrates the edited prior-name copy, which is what the description says reset does. The code looks right here; the assertion assumes macOS.
There was a problem hiding this comment.
Fixed in 10c72c4. The test probes the tmp filesystem first; on Linux it asserts the migrate path you described, on macOS the occupant path.
…e' into claude/ex-3055-skills-sync-manifest
--reset-context no longer forces over an exact-name directory that has no SKILL.md. A backup is removed only once its skill is present again and it has aged past the staging guard. A prior name that differs from the current name only by case is renamed in place instead of being reported as an occupant every run. Inode 0 is no longer treated as a match. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e' into claude/ex-3055-skills-sync-manifest
Windows returns readdir entries in a different order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
EX-3055. Stacked on #8555, so review that first.
#8555 makes
netlify initinstall the Netlify skills and refresh outdated copies, but it leaves copies under deprecated or prior names in place and gives the user no way to put an edited skill back to the shipped release. This PR adds the rest of the sync: re-runninginitdeletes an unedited deprecated skill, migrates an unedited copy under a prior name to its current name, and cleans up what an interrupted run left behind. Anything the user edited is kept and listed, and--reset-contextreplaces, migrates or deletes it.This is the first path that deletes directories in a user's repo. A directory is removed only when its tree hash matches a release we shipped, re-hashed right before the
rm, or under--reset-contextwhen it sits at a manifest name. Nothing else is touched: unknown directories, symlinks, and a directory that differs from a skill name only by case on macOS are all left alone, with or without the flag.Changes
src/utils/init/agent-skills.ts:syncSkillstakesreset. New rules per directory:--reset-context.--reset-context.--reset-context. A symlink or file at the skill's own name is replaced too; only the link is removed, never its target..netlify-skill-<name>-*staging directories older than ten minutes and<name>.old-*backups that hash to a shipped release are removed when<name>is in the manifest. Younger staging directories belong to a run that may still be writing them.Netlify-Deploynext tonetlify-deploy) is never renamed away, with or withoutSKILL.md. The assembled staging tree must hash to the manifesttree_hashbefore it is swapped in. A directory withoutSKILL.mdunder a prior or deprecated name is ignored. Every hash call passes the manifest'sexecutableset, so the Windows path from feat(init): install Netlify agent skills by default #8555 holds here.reset,renamedandremovedalongsideadded,updatedandfailed. Kept and failed copies are listed, with a--reset-contexthint only when reset would act on one of them.src/commands/init/index.ts,init.ts: the--reset-contextflag, passed throughInitExtraOptions;resetContextis added to the analytics payload and thesites_agentSkillsSetupevent.devandwatchare unchanged.docs/commands/init.md: regenerated with the new flag.Testing
tests/unit/utils/init/agent-skills.test.ts(54 tests,fetchstubbed): deprecated removed by exact and by prior name, edited deprecated kept then removed on reset, an edit made mid-run (while another skill downloads) keeps the deprecated copy; prior name migrated, removed as superseded, kept when either copy is edited, two prior names in one run; modified kept then reset, symlink reset without touching its target; leftover cleanup incl. a fresh staging directory left alone, an aged staging directory under a non-manifest name left alone, an edited backup kept; staged tree-hash mismatch refused; case twin with and withoutSKILL.mdnever replaced, with assertions for both case-sensitive and case-insensitive filesystems; an exact-name directory withoutSKILL.mdleft alone on reset; a backup kept while its skill is missing and removed once it is back; a prior name differing only by case renamed in place; a failed download recorded next to what landed; summary and hint lines.tests/integration/commands/init/init.test.ts: the mock skills host now takes a manifest spec; shared init helpers hoisted. New test runsinitagainst a stale, an edited and a deprecated copy (1 updated, 1 removed, 1 kept), theninit --reset-context(1 reset). 9 tests pass.npm run test:unit(717 tests),npm run typecheck,npm run lintandnpm run format:checkare clean.Known gaps
netlify initruns in the same repo at once are not serialized. The age guard and the staged hash check turn that race into a failed install rather than a corrupt one; the next run completes it.🤖 Generated with Claude Code