From 5ecff6c6d37af1488f3de7ffac064beab59fbb4f Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Thu, 6 Aug 2026 09:38:37 -0700 Subject: [PATCH 1/3] fix: harden destructive-path containment (CHK-036/039/040) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 /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 `/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) --- src/build.ts | 2 +- src/cleanup.ts | 4 +- src/config.ts | 30 +++++++ src/fs.ts | 67 +++++++++++++- src/managed.ts | 93 +++++++++++++++---- tests/destructive.test.ts | 182 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 352 insertions(+), 26 deletions(-) diff --git a/src/build.ts b/src/build.ts index e3ba7fe..6a63f2b 100644 --- a/src/build.ts +++ b/src/build.ts @@ -26,7 +26,7 @@ export async function build(options: BuildOptions = {}): Promise { const targets = options.target ? [options.target] : targetNames.filter((target) => project.config.targets[target]); - const guard = buildDeleteGuard( + const guard = await buildDeleteGuard( project.rootDir, project.config, project.configPath, diff --git a/src/cleanup.ts b/src/cleanup.ts index 3878e05..0721e28 100644 --- a/src/cleanup.ts +++ b/src/cleanup.ts @@ -26,7 +26,7 @@ export async function prune( } = {}, ): Promise { const project = await loadProjectConfig(options.cwd, options.configPath); - const guard = buildDeleteGuard( + const guard = await buildDeleteGuard( project.rootDir, project.config, project.configPath, @@ -58,7 +58,7 @@ export async function clean( } = {}, ): Promise { const project = await loadProjectConfig(options.cwd, options.configPath); - const guard = buildDeleteGuard( + const guard = await buildDeleteGuard( project.rootDir, project.config, project.configPath, diff --git a/src/config.ts b/src/config.ts index 15f3d37..f9d1797 100644 --- a/src/config.ts +++ b/src/config.ts @@ -161,6 +161,36 @@ async function discoverSourcePlugins( return plugins; } +/** + * Directories under `sourceRoot` that are actually source plugins. + * + * The delete guard needs this to protect source without protecting generated + * output: the recommended layout writes every target's output under + * `plugins//`, which is also the default source-plugin root, so + * protecting that root wholesale refuses to prune the tool's own output. A + * shallow filesystem scan is deliberate — `clean` must keep working when the + * config or source tree no longer loads. + */ +export async function listSourcePluginDirs( + sourceRoot: string, +): Promise { + if (!(await exists(sourceRoot))) { + return []; + } + const dirs: string[] = []; + const entries = await fs.readdir(sourceRoot, { withFileTypes: true }); + for (const entry of entries) { + if (!entry.isDirectory() || entry.name.startsWith(".")) { + continue; + } + const dir = path.join(sourceRoot, entry.name); + if (await isSourcePluginDir(dir)) { + dirs.push(dir); + } + } + return dirs; +} + /** * A source plugin dir declares a manifest or has at least one component dir. * This keeps generated target output (e.g. plugins/cursor/ in a single-repo diff --git a/src/fs.ts b/src/fs.ts index 1667756..3e7c0eb 100644 --- a/src/fs.ts +++ b/src/fs.ts @@ -19,6 +19,61 @@ export async function walkFiles(dir: string): Promise { return entries.sort(); } +/** Whether `candidate` is `root` itself or sits beneath it, comparing already-resolved paths. */ +export function isInside(root: string, candidate: string): boolean { + return candidate === root || candidate.startsWith(`${root}${path.sep}`); +} + +/** + * Resolves `target` with symlinks followed, or returns `null` if the result + * escapes `root`. + * + * `path.resolve` is purely lexical, so a prefix test against it is satisfied by + * a path whose *intermediate* directory is a symlink pointing elsewhere — + * `/link/file` passes the string check while landing wherever `link` + * points. Checking the final entry with `lstat` does not help either, since the + * final entry is an ordinary file. + * + * Both sides go through the same resolution, which matters more than it looks: + * 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 precisely when the output + * directory does not exist yet, i.e. on a first build. + */ +export async function resolveInside( + root: string, + target: string, +): Promise { + const realRoot = await realpathDeep(root); + const realTarget = await realpathDeep(target); + return isInside(realRoot, realTarget) ? realTarget : null; +} + +/** + * `fs.realpath` for a path that may not exist yet: resolves symlinks on the + * deepest ancestor that does exist, then re-attaches the missing tail. + */ +async function realpathDeep(target: string): Promise { + const missing: string[] = []; + let current = path.resolve(target); + for (;;) { + try { + const real = await fs.realpath(current); + return path.join(real, ...missing.reverse()); + } catch (error) { + if (!isNotFoundError(error)) { + throw error; + } + const parent = path.dirname(current); + if (parent === current) { + return path.resolve(target); + } + missing.push(path.basename(current)); + current = parent; + } + } +} + /** Writes every file in `files` under `outDir`, refusing to write outside it. */ export async function writeArtifact( outDir: string, @@ -27,14 +82,18 @@ export async function writeArtifact( const resolvedOut = path.resolve(outDir); for (const [relativePath, value] of files) { const destination = path.resolve(outDir, relativePath); - if ( - destination !== resolvedOut && - !destination.startsWith(resolvedOut + path.sep) - ) { + if (!isInside(resolvedOut, destination)) { throw new Error( `Refusing to write outside the output directory: ${relativePath}`, ); } + // The lexical check above cannot see through a symlinked intermediate + // directory, so confirm the real destination too, before creating anything. + if ((await resolveInside(resolvedOut, destination)) === null) { + throw new Error( + `Refusing to write through a symlink that leaves the output directory: ${relativePath}`, + ); + } await fs.mkdir(path.dirname(destination), { recursive: true }); await fs.writeFile(destination, value); } diff --git a/src/managed.ts b/src/managed.ts index 458b6e7..56a3b87 100644 --- a/src/managed.ts +++ b/src/managed.ts @@ -1,6 +1,7 @@ import { promises as fs } from "node:fs"; import path from "node:path"; -import { json, toPosix } from "./fs.js"; +import { isInside, json, resolveInside, toPosix } from "./fs.js"; +import { listSourcePluginDirs } from "./config.js"; import type { Artifact, CleanupEntry, @@ -134,23 +135,37 @@ export async function cleanManagedFiles( return { target, outDir, entries }; } -/** Builds the guard that stops prune/clean from deleting paths inside the config's source tree. */ -export function buildDeleteGuard( +/** + * Builds the guard that stops prune/clean from deleting the config or a source tree. + * + * `source.plugins` gets different treatment depending on whether it was set: + * when the user names a directory as their source-plugin root, that whole + * directory is protected. When they don't, the default root is `plugins/` — + * which is also where the recommended layout writes every target's *output*, so + * protecting it wholesale made the documented layout refuse to prune its own + * generated files after any source file was removed. Under the default, protect + * only the directories that really are source plugins. + */ +export async function buildDeleteGuard( rootDir: string, config: PluginpackConfig, configPath: string, force?: boolean, -): DeleteGuard { +): Promise { const protectedRoots: string[] = []; if (config.source?.skills) { protectedRoots.push(path.resolve(rootDir, config.source.skills)); } - // Mirrors loadConfig's default in src/config.ts so the source-plugin - // discovery root is always protected, whether or not it's written out - // explicitly in config. - protectedRoots.push( - path.resolve(rootDir, config.source?.plugins ?? "plugins"), - ); + if (config.source?.partials) { + protectedRoots.push(path.resolve(rootDir, config.source.partials)); + } + if (config.source?.plugins) { + protectedRoots.push(path.resolve(rootDir, config.source.plugins)); + } else { + protectedRoots.push( + ...(await listSourcePluginDirs(path.resolve(rootDir, "plugins"))), + ); + } return { protectedRoots, configPath: path.resolve(configPath), force }; } @@ -177,17 +192,44 @@ function assertNoProtectedDeletions( ); } +/** + * Whether deleting `relativePath` would reach into the config or a source tree. + * + * Comparison is case- and normalization-folded, not exact. On a case-insensitive + * filesystem (APFS, NTFS) `fs.rm` resolves `Skills/x` and `skills/x` to the same + * file, so an exact-match guard can be walked straight past by one letter of + * case — reachable from a single typo in `source.skills` that the OS forgives, + * or a case-only rename in git history. Folding over-protects on a + * case-sensitive host, which is the correct direction for a guard whose job is + * refusing to delete. + */ function isProtectedDeletion( outDir: string, relativePath: string, guard: DeleteGuard, ): boolean { const absolute = path.resolve(outDir, normalizeManagedPath(relativePath)); - if (guard.configPath && absolute === guard.configPath) { + if (guard.configPath && pathsEqual(absolute, guard.configPath)) { return true; } return guard.protectedRoots.some( - (root) => absolute === root || absolute.startsWith(`${root}${path.sep}`), + (root) => pathsEqual(absolute, root) || isUnder(absolute, root), + ); +} + +/** Case- and normalization-folded form, for comparing paths the filesystem treats as equal. */ +function fold(value: string): string { + return value.normalize("NFC").toLowerCase(); +} + +function pathsEqual(a: string, b: string): boolean { + return a === b || fold(a) === fold(b); +} + +function isUnder(candidate: string, root: string): boolean { + return ( + candidate.startsWith(`${root}${path.sep}`) || + fold(candidate).startsWith(`${fold(root)}${path.sep}`) ); } @@ -218,13 +260,9 @@ async function removeManagedPath( const root = path.resolve(outDir); const normalized = normalizeManagedPath(relativePath); const destination = path.resolve(root, normalized); - if (destination !== root && !destination.startsWith(`${root}${path.sep}`)) { + if (!isInside(root, destination)) { throw new Error(`Managed path escapes output directory: ${relativePath}`); } - // Defense in depth: fs.rm on a symlink unlinks the symlink itself rather - // than following it, so this isn't currently exploitable — but refuse - // outright if the entry is a symlink pointing outside `root`, rather than - // relying on that fs.rm behavior remaining true forever. let stats; try { stats = await fs.lstat(destination); @@ -239,13 +277,30 @@ async function removeManagedPath( path.dirname(destination), await fs.readlink(destination), ); - if (target !== root && !target.startsWith(`${root}${path.sep}`)) { + if (!isInside(root, target)) { throw new Error( `Refusing to remove a symlink pointing outside the output directory: ${relativePath}`, ); } } - await fs.rm(destination, { force: true }); + // The checks above are lexical, or inspect only the final entry. Neither sees + // a path whose *intermediate* directory is a symlink pointing elsewhere: the + // string test passes and the final entry is an ordinary file. Resolve the + // real destination as the catch-all before deleting. + if ((await resolveInside(root, destination)) === null) { + throw new Error( + `Refusing to remove a path that resolves outside the output directory: ${relativePath}`, + ); + } + try { + await fs.rm(destination, { force: true }); + } catch (error) { + throw new Error( + `Failed to remove managed path "${relativePath}" under ${outDir}: ${(error as Error).message}. ` + + `The managed manifest still lists every path, so re-running is safe once the cause is fixed.`, + { cause: error }, + ); + } await removeEmptyParents(path.dirname(destination), root); } diff --git a/tests/destructive.test.ts b/tests/destructive.test.ts index 9fef9c9..931e70c 100644 --- a/tests/destructive.test.ts +++ b/tests/destructive.test.ts @@ -463,6 +463,188 @@ describe("build writes every target before pruning any target", () => { }); }); +describe("the documented layouts stay operable through a full lifecycle", () => { + // The shape of test that was missing when this class of bug shipped twice: + // every other layout test does one fresh build and never mutates, prunes, or + // cleans. A guard false positive is invisible until something goes stale. + const layouts: Array<[string, string]> = [ + [ + "README Recommended Shape (output under plugins//, source.plugins unset)", + ` antigravity: { + outDir: "plugins/antigravity", + plugins: { acme: { from: ["core"] } } + }`, + ], + [ + "README single-repo-root shape (outDir '.', output under a pluginRoot)", + ` claude: { + outDir: ".", + pluginRoot: "plugins/claude", + plugins: { acme: { from: ["core"] } } + }`, + ], + [ + "init scaffold shape (dist/)", + ` claude: { + outDir: "dist/claude", + plugins: { acme: { from: ["core"] } } + }`, + ], + ]; + + for (const [label, targetsBlock] of layouts) { + it(`survives build -> delete a skill -> rebuild -> prune -> clean: ${label}`, async () => { + const created = await makeProject( + targetsBlock, + { + skills: { + alpha: { + "SKILL.md": "---\nname: alpha\ndescription: Alpha.\n---\n\nA.\n", + }, + beta: { + "SKILL.md": "---\nname: beta\ndescription: Beta.\n---\n\nB.\n", + }, + }, + }, + `source: { skills: "skills", rootPlugin: { id: "core" } },`, + ); + const root = created.baseDir; + + await build({ cwd: root }); + // A user removes a skill — the step that used to break everything after. + await rm(path.join(root, "skills/beta"), { + recursive: true, + force: true, + }); + + await expect(build({ cwd: root })).resolves.toBeDefined(); + await expect(prune({ cwd: root })).resolves.toBeDefined(); + await expect(clean({ cwd: root })).resolves.toBeDefined(); + + // The source tree is untouched by any of it. + await access(path.join(root, "skills/alpha/SKILL.md")); + await access(path.join(root, "pluginpack.config.ts")); + }); + } + + it("still refuses to delete real source when a target writes into the source tree", async () => { + const created = await makeProject( + ` antigravity: { + outDir: ".", + plugins: { acme: { from: ["core"], path: "skills/generated" } } + }`, + { + skills: { + alpha: { + "SKILL.md": "---\nname: alpha\ndescription: Alpha.\n---\n\nA.\n", + }, + }, + }, + `source: { skills: "skills", rootPlugin: { id: "core" } },`, + ); + const root = created.baseDir; + await build({ cwd: root }); + + await expect(clean({ cwd: root })).rejects.toThrow( + /Refusing to clean .* that resolve inside your source tree or config/, + ); + await access(path.join(root, "skills/alpha/SKILL.md")); + }); + + it("refuses when source.skills differs from the real directory only by case", async () => { + // A config typo a case-insensitive filesystem forgives: the build succeeds + // against `skills/` while the guard was told `Skills/`. Exact-match + // comparison let clean walk straight into the real source tree. + const created = await makeProject( + ` antigravity: { + outDir: ".", + plugins: { acme: { from: ["core"], path: "skills/generated" } } + }`, + { + skills: { + alpha: { + "SKILL.md": "---\nname: alpha\ndescription: Alpha.\n---\n\nA.\n", + }, + }, + }, + `source: { skills: "Skills", rootPlugin: { id: "core" } },`, + ); + const root = created.baseDir; + await build({ cwd: root }); + + await expect(clean({ cwd: root })).rejects.toThrow( + /Refusing to clean .* that resolve inside your source tree or config/, + ); + await access(path.join(root, "skills/alpha/SKILL.md")); + }); +}); + +describe("containment follows symlinks instead of trusting the path string", () => { + async function projectWithSymlinkedOutputDir(): Promise<{ + root: string; + outside: string; + }> { + const created = await makeProject( + ` antigravity: { + outDir: "out", + plugins: { acme: { from: ["demo"], path: "shared" } } + }`, + ); + const root = created.baseDir; + const outside = path.join(root, "outside-the-output-dir"); + await mkdir(outside, { recursive: true }); + await writeFile(path.join(outside, "plugin.json"), "USER FILE\n"); + await mkdir(path.join(root, "out"), { recursive: true }); + // An intermediate segment of the output path is a symlink pointing away. + await symlink(outside, path.join(root, "out/shared")); + return { root, outside }; + } + + it("refuses to write through a symlinked intermediate directory", async () => { + const { root, outside } = await projectWithSymlinkedOutputDir(); + + await expect(build({ cwd: root })).rejects.toThrow( + /Refusing to write through a symlink that leaves the output directory/, + ); + expect(await readFile(path.join(outside, "plugin.json"), "utf8")).toBe( + "USER FILE\n", + ); + }); + + it("refuses to delete through a symlinked intermediate directory", async () => { + const { root, outside } = await projectWithSymlinkedOutputDir(); + // A manifest naming a path whose intermediate segment is the symlink. Every + // segment is a safe relative path, so normalizeManagedPath permits it. + await writeManifest(root, "out", "antigravity", ["shared/plugin.json"]); + + await expect(clean({ cwd: root, target: "antigravity" })).rejects.toThrow( + /Refusing to remove a path that resolves outside the output directory/, + ); + await access(path.join(outside, "plugin.json")); + }); + + it("still writes and cleans normally when the output directory itself is a symlink", async () => { + // The legitimate case: outDir is a symlink. Resolving both sides means this + // keeps working rather than being caught as an escape. + const created = await makeProject( + ` antigravity: { + outDir: "linked-out", + plugins: { acme: { from: ["demo"] } } + }`, + ); + const root = created.baseDir; + const real = path.join(root, "real-out"); + await mkdir(real, { recursive: true }); + await symlink(real, path.join(root, "linked-out")); + + await expect(build({ cwd: root })).resolves.toBeDefined(); + await access(path.join(real, "acme/plugin.json")); + await expect( + clean({ cwd: root, target: "antigravity" }), + ).resolves.toBeDefined(); + }); +}); + describe("clean refuses paths another target's manifest also claims", () => { // cursor's outDir contains claude's, so a cursor manifest can name a file // inside dist/claude without any "../" — the shape a pre-collision-check From 0e5c1b38cd7e7aa7496efc174ffd6be4a87d9c55 Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Thu, 6 Aug 2026 10:06:28 -0700 Subject: [PATCH 2/3] fix: name the protected root in the delete-guard refusal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 /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) --- src/managed.ts | 27 +++++++++++++++++---------- tests/destructive.test.ts | 16 ++++++++++++++++ 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/src/managed.ts b/src/managed.ts index 56a3b87..ea1dcbe 100644 --- a/src/managed.ts +++ b/src/managed.ts @@ -178,22 +178,27 @@ function assertNoProtectedDeletions( if (!guard || guard.force) { return; } - const blocked = paths.filter((file) => - isProtectedDeletion(outDir, file, guard), - ); + const blocked = paths + .map((file) => ({ file, root: protectingRoot(outDir, file, guard) })) + .filter( + (entry): entry is { file: string; root: string } => entry.root !== null, + ); if (blocked.length === 0) { return; } throw new Error( `Refusing to ${command} ${blocked.length} path(s) that resolve inside your source tree or config:\n` + - `${blocked.map((file) => ` ${file}`).join("\n")}\n` + + `${blocked.map(({ file, root }) => ` ${file} -> resolves inside ${root}`).join("\n")}\n` + `This usually means a target outDir overlaps source.skills/source.plugins. ` + `Fix the config, or re-run with --force to delete anyway.`, ); } /** - * Whether deleting `relativePath` would reach into the config or a source tree. + * The protected root `relativePath` resolves inside, or `null` if it is safe to + * delete. Returns the matching root rather than a boolean so the refusal can + * name what it collided with — "resolves inside " is actionable in a way + * that a bare list of refused paths is not. * * Comparison is case- and normalization-folded, not exact. On a case-insensitive * filesystem (APFS, NTFS) `fs.rm` resolves `Skills/x` and `skills/x` to the same @@ -203,17 +208,19 @@ function assertNoProtectedDeletions( * case-sensitive host, which is the correct direction for a guard whose job is * refusing to delete. */ -function isProtectedDeletion( +function protectingRoot( outDir: string, relativePath: string, guard: DeleteGuard, -): boolean { +): string | null { const absolute = path.resolve(outDir, normalizeManagedPath(relativePath)); if (guard.configPath && pathsEqual(absolute, guard.configPath)) { - return true; + return guard.configPath; } - return guard.protectedRoots.some( - (root) => pathsEqual(absolute, root) || isUnder(absolute, root), + return ( + guard.protectedRoots.find( + (root) => pathsEqual(absolute, root) || isUnder(absolute, root), + ) ?? null ); } diff --git a/tests/destructive.test.ts b/tests/destructive.test.ts index 931e70c..0b043c2 100644 --- a/tests/destructive.test.ts +++ b/tests/destructive.test.ts @@ -307,6 +307,22 @@ describe("the delete guard protects a source tree from clean, not just prune", ( /Refusing to clean 2 path\(s\)[\s\S]*skills\/demo\/SKILL\.md[\s\S]*skills\/demo\/OTHER\.md/, ); }); + + it("names the protected root each blocked path resolved inside", async () => { + // A bare list of refused paths does not tell the user what the collision + // was with. Naming the root does — and when the root came from a mis-cased + // config value, seeing it echoed back points straight at the typo. + const created = await overlappingProject(); + const root = created.baseDir; + await build({ cwd: root, target: "cursor" }); + await writeManifest(root, ".", "cursor", ["skills/demo/SKILL.md"]); + + await expect(clean({ cwd: root, target: "cursor" })).rejects.toThrow( + new RegExp( + `skills/demo/SKILL\\.md -> resolves inside ${root.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}/skills`, + ), + ); + }); }); describe("clean handles targets and manifests that are not there", () => { From be980e5efb56f64c8de61662593ff8c960177593 Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Thu, 6 Aug 2026 10:11:22 -0700 Subject: [PATCH 3/3] test: make the case-sensitivity guard tests platform-honest 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) --- tests/destructive.test.ts | 60 +++++++++++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 6 deletions(-) diff --git a/tests/destructive.test.ts b/tests/destructive.test.ts index 0b043c2..be3116d 100644 --- a/tests/destructive.test.ts +++ b/tests/destructive.test.ts @@ -1,3 +1,4 @@ +import { existsSync } from "node:fs"; import { access, mkdir, @@ -7,6 +8,7 @@ import { writeFile, } from "node:fs/promises"; import path from "node:path"; +import { fileURLToPath } from "node:url"; import { Project, type ProjectArgs } from "fixturify-project"; import { afterEach, describe, expect, it } from "vitest"; import { build } from "../src/build.js"; @@ -82,6 +84,18 @@ const cursorTarget = ` cursor: { plugins: { demo: { from: ["demo"], components: ["skills"] } } }`; +/** + * Whether this host's filesystem is case-insensitive (APFS, NTFS). A couple of + * guard behaviours only exist on such a host, and asserting them on a + * case-sensitive one would test the filesystem rather than the guard. + */ +const caseInsensitiveFs = existsSync( + path.join( + path.dirname(fileURLToPath(import.meta.url)), + "DESTRUCTIVE.TEST.TS", + ), +); + /** Rewrites a target's managed manifest, the on-disk record prune/clean/diff act on. */ async function writeManifest( root: string, @@ -567,14 +581,15 @@ describe("the documented layouts stay operable through a full lifecycle", () => await access(path.join(root, "skills/alpha/SKILL.md")); }); - it("refuses when source.skills differs from the real directory only by case", async () => { - // A config typo a case-insensitive filesystem forgives: the build succeeds - // against `skills/` while the guard was told `Skills/`. Exact-match - // comparison let clean walk straight into the real source tree. + it("refuses a managed path differing from a protected root only by case", async () => { + // Tests the guard's comparison directly, independent of how the host + // filesystem resolves case: the manifest names `Skills/...` while + // source.skills is `skills`. On a case-insensitive host fs.rm would resolve + // these to the same file, so an exact-match guard deletes real source. const created = await makeProject( ` antigravity: { outDir: ".", - plugins: { acme: { from: ["core"], path: "skills/generated" } } + plugins: { acme: { from: ["core"], path: "generated" } } }`, { skills: { @@ -583,16 +598,49 @@ describe("the documented layouts stay operable through a full lifecycle", () => }, }, }, - `source: { skills: "Skills", rootPlugin: { id: "core" } },`, + `source: { skills: "skills", rootPlugin: { id: "core" } },`, ); const root = created.baseDir; await build({ cwd: root }); + await writeManifest(root, ".", "antigravity", ["Skills/alpha/SKILL.md"]); await expect(clean({ cwd: root })).rejects.toThrow( /Refusing to clean .* that resolve inside your source tree or config/, ); await access(path.join(root, "skills/alpha/SKILL.md")); }); + + it.skipIf(!caseInsensitiveFs)( + "refuses when source.skills differs from the real directory only by case", + async () => { + // The end-to-end version of the above, and the realistic trigger: a config + // typo that a case-insensitive filesystem forgives, so the build succeeds + // against `skills/` while the guard was told `Skills/`. Only meaningful on + // a host that resolves the mismatch — on a case-sensitive one the build + // correctly fails earlier with "Root skills source directory is missing". + const created = await makeProject( + ` antigravity: { + outDir: ".", + plugins: { acme: { from: ["core"], path: "skills/generated" } } + }`, + { + skills: { + alpha: { + "SKILL.md": "---\nname: alpha\ndescription: Alpha.\n---\n\nA.\n", + }, + }, + }, + `source: { skills: "Skills", rootPlugin: { id: "core" } },`, + ); + const root = created.baseDir; + await build({ cwd: root }); + + await expect(clean({ cwd: root })).rejects.toThrow( + /Refusing to clean .* that resolve inside your source tree or config/, + ); + await access(path.join(root, "skills/alpha/SKILL.md")); + }, + ); }); describe("containment follows symlinks instead of trusting the path string", () => {