fix: stop the delete guard breaking the documented layout, and follow symlinks when checking containment - #28
Merged
Conversation
Three live-reproduced ways pluginpack could destroy or refuse to manage files,
found by a 1.0-readiness review. All three were invisible to a green 189-test
suite because no test ever mutated a documented layout and then pruned it.
CHK-036 — the README's Recommended Shape survived exactly one build. The delete
guard unconditionally protected <root>/plugins, which is where that layout
writes every target's output, so deleting or renaming a skill made build, prune,
and clean all refuse. The only escape was --force, which also disables real
source protection, training the habit that made CHK-039 destructive. This was a
regression from the previous review's own fix. The guard now protects the
*discovered* source plugin directories under the default root, plus source.skills,
source.partials, and an explicitly configured source.plugins root. Reuses
config.ts's single definition of "is a source plugin dir" rather than a second
copy, since two definitions of that predicate could drift.
CHK-039 — one letter of case defeated the guard entirely. isProtectedDeletion
compared exact strings while fs.rm resolves case-insensitively on APFS and NTFS,
so `source: { skills: "Skills" }` against a real `skills/` — a typo the OS
forgives, no manifest editing, no --force — let clean delete the source tree.
Comparison is now case- and NFC-folded. That over-protects on a case-sensitive
host, which is the correct direction for a guard whose job is refusing to delete.
CHK-040 — a symlinked *intermediate* directory escaped containment on both the
write and delete paths. path.resolve is lexical and the symlink check inspected
only the final entry, which is an ordinary file, so `<outDir>/link/file` passed
the string test and landed wherever `link` pointed: build overwrote a file
outside outDir, clean deleted it, then crashed ENOTDIR leaving the repo
permanently un-cleanable. Both paths now resolve symlinks via a shared
resolveInside helper.
Note on resolveInside: it resolves BOTH sides. Resolving only the target and
comparing against a lexical root rejects every ordinary write whenever any
ancestor of the output directory is itself a symlink — /tmp is one on macOS —
and does so exactly when the output directory does not yet exist, i.e. on a
first build. I hit that false positive mid-change; the symmetry is load-bearing,
not stylistic.
Also wraps fs.rm failures with the path, the outDir, and the note that the
manifest still lists everything so a re-run is safe once the cause is fixed.
Tests: a "documented layouts stay operable through a full lifecycle" block runs
build -> delete a skill -> rebuild -> prune -> clean across three real layouts
(README Recommended Shape, README outDir "." shape, init scaffold shape). That
is the test shape absent when this class of bug shipped twice. Plus symlink
write/delete containment, the legitimate symlinked-outDir case, and the case-only
guard bypass.
All four fixes are mutation-verified: reverting each turns the corresponding test
red. Reverting CHK-036 fails 2 of the 3 lifecycle layouts and not the
init/dist one — which is why the scaffold layout hid this.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CHK-036 asked for the refusal to name "the specific source path the managed path resolved inside"; the first commit only listed the refused paths, which does not tell the user what the collision was with. isProtectedDeletion returned a boolean, so the message had nothing to name. Replaced with protectingRoot, which returns the matched root. Useful side effect: when the root came from a mis-cased config value, the message echoes it back — "resolves inside <root>/Skills" while the real directory is skills/ — which points straight at the typo that CHK-039 is about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The config-typo test only means anything on a case-insensitive filesystem: on Linux `source.skills: "Skills"` against a real `skills/` does not resolve at all, so build correctly fails earlier with "Root skills source directory is missing" and the test would fail in CI on ubuntu for the wrong reason. Split into two: - A platform-independent test that puts the case variant in the MANIFEST while source.skills is correctly cased. That exercises the fold comparison itself rather than the filesystem, and holds on any host. - The end-to-end config-typo version, skipped unless the host filesystem is actually case-insensitive, probed by asking whether this test file exists under an upper-cased name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
Three live-reproduced data-loss or breakage paths from the 1.0-readiness review, all invisible to a green 189-test suite.
CHK-036 — the README's own Recommended Shape survived exactly one build
The delete guard unconditionally protected
<root>/plugins, which is where that layout writes every target's output. So:The only escape was
--force, which also disables real source protection — so the bug trained users into the habit that makes CHK-039 destructive. This was a regression from the previous review's fix for the opposite problem.The guard now protects the discovered source plugin directories under the default root, plus
source.skills,source.partials, and an explicitly configuredsource.plugins(when you name a directory as source, the whole directory is protected). It reusesconfig.ts's single definition of "is a source plugin dir" rather than a second copy — two definitions of that predicate drifting is the same class of bug as the rest of this PR.After: all four steps pass.
CHK-039 — one letter of case defeated the guard, no
--forceisProtectedDeletioncompared exact strings whilefs.rmresolves case-insensitively on APFS and NTFS.source: { skills: "Skills" }against a realskills/— a config typo the OS silently forgives — letcleandelete the source tree andremoveEmptyParentstake the directory too. Comparison is now case- and NFC-folded. This over-protects on a case-sensitive host, which is the right direction for a guard whose job is refusing to delete.CHK-040 — a symlinked intermediate directory escaped containment, config-only
path.resolveis lexical, and the symlink check inspected only the final entry — an ordinary file. So<outDir>/link/filepassed the string test and landed whereverlinkpointed:buildoverwrote a user file outsideoutDir,cleandeleted it, then crashedENOTDIRleaving the repo permanently un-cleanable including under--force. Both paths now resolve symlinks through a sharedresolveInside.One detail worth reviewer attention:
resolveInsideresolves both sides. Resolving only the target and comparing against a lexical root rejects every ordinary write whenever any ancestor of the output directory is itself a symlink —/tmpis one on macOS — and does so exactly when the output directory doesn't exist yet, i.e. on a first build. I hit that false positive mid-change; the symmetry is load-bearing, not stylistic.Also wraps
fs.rmfailures with the path, the outDir, and a note that the manifest still lists everything, so re-running is safe once the cause is fixed (CHK-072 groundwork).Tests
197 total, up from 189. The important addition is a "documented layouts stay operable through a full lifecycle" block: build → delete a skill → rebuild → prune → clean, across three real layouts (README Recommended Shape, README
outDir: "."shape,initscaffold shape). That is the test shape that was missing when this class of bug shipped twice — every prior layout test did one fresh build and never mutated or pruned.Plus symlink write/delete containment, the legitimate symlinked-
outDircase (which must keep working), and the case-only guard bypass.Verification
Every fix is mutation-verified — reverting each turns the corresponding test red:
pluginsrootwriteArtifactremoveManagedPathThat the
init/distlayout passes even with CHK-036 reintroduced is itself the explanation for how this hid: the scaffolded layout dodges the bug, so only the recommended one breaks.Each original reproduction was re-run against the fix and no longer reproduces.
🤖 Generated with Claude Code