refactor: address code-review findings on the containment fixes

src/build.ts

@@ -26,11 +26,7 @@ export async function build(options: BuildOptions = {}): Promise<Artifact[]> {
   const targets = options.target
     ? [options.target]
     : targetNames.filter((target) => project.config.targets[target]);
-  const guard = await buildDeleteGuard(
-    project.rootDir,
-    project.config,
-    project.configPath,
-  );
+  const guard = await buildDeleteGuard(project);
   const artifacts: Artifact[] = [];
   for (const target of targets) {
     artifacts.push(await emitTarget(project, target, options.outDir));

src/cleanup.ts

@@ -26,12 +26,7 @@ export async function prune(
   } = {},
 ): Promise<CleanupResult[]> {
   const project = await loadProjectConfig(options.cwd, options.configPath);
-  const guard = await buildDeleteGuard(
-    project.rootDir,
-    project.config,
-    project.configPath,
-    options.force,
-  );
+  const guard = await buildDeleteGuard(project, options.force);
   const artifacts = await build({
     cwd: options.cwd,
     configPath: options.configPath,
@@ -58,12 +53,7 @@ export async function clean(
   } = {},
 ): Promise<CleanupResult[]> {
   const project = await loadProjectConfig(options.cwd, options.configPath);
-  const guard = await buildDeleteGuard(
-    project.rootDir,
-    project.config,
-    project.configPath,
-    options.force,
-  );
+  const guard = await buildDeleteGuard(project, options.force);
   const targets = options.target
     ? [options.target]
     : (Object.keys(project.config.targets) as TargetName[]);

src/config.ts

@@ -138,25 +138,12 @@ async function discoverSourcePlugins(
   sourceRoot: string,
 ): Promise<Map<string, SourcePlugin>> {
   const plugins = new Map<string, SourcePlugin>();
-  if (!(await exists(sourceRoot))) {
-    return plugins;
-  }
-  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))) {
-      continue;
-    }
-    const manifestPath = path.join(dir, "plugin.pluginpack.json");
-    const manifest = await readSourceManifest(manifestPath);
-    plugins.set(entry.name, {
-      id: entry.name,
-      dir,
-      manifest,
-    });
+  for (const dir of await listSourcePluginDirs(sourceRoot)) {
+    const id = path.basename(dir);
+    const manifest = await readSourceManifest(
+      path.join(dir, "plugin.pluginpack.json"),
+    );
+    plugins.set(id, { id, dir, manifest });
   }
   return plugins;
 }

src/fs.ts

@@ -24,6 +24,22 @@ export function isInside(root: string, candidate: string): boolean {
   return candidate === root || candidate.startsWith(`${root}${path.sep}`);
 }
 
+/**
+ * `isInside`, comparing the way a case-insensitive filesystem does.
+ *
+ * On APFS and NTFS `fs.rm` resolves `Skills/x` and `skills/x` to the same file,
+ * so an exact-match containment test can be walked past by one letter of case.
+ * Folding case and Unicode normalization over-matches on a case-sensitive host,
+ * which is the right direction for a check that decides whether to refuse.
+ */
+export function isInsideFolded(root: string, candidate: string): boolean {
+  return isInside(root, candidate) || isInside(fold(root), fold(candidate));
+}
+
+function fold(value: string): string {
+  return value.normalize("NFC").toLowerCase();
+}
+
 /**
  * Resolves `target` with symlinks followed, or returns `null` if the result
  * escapes `root`.
@@ -59,7 +75,7 @@ async function realpathDeep(target: string): Promise<string> {
   for (;;) {
     try {
       const real = await fs.realpath(current);
-      return path.join(real, ...missing.reverse());
+      return path.join(real, ...[...missing].reverse());
     } catch (error) {
       if (!isNotFoundError(error)) {
         throw error;

src/managed.ts

@@ -1,13 +1,19 @@
 import { promises as fs } from "node:fs";
 import path from "node:path";
-import { isInside, json, resolveInside, toPosix } from "./fs.js";
+import {
+  isInside,
+  isInsideFolded,
+  json,
+  resolveInside,
+  toPosix,
+} from "./fs.js";
 import { listSourcePluginDirs } from "./config.js";
 import type {
   Artifact,
   CleanupEntry,
   CleanupResult,
   DeleteGuard,
-  PluginpackConfig,
+  ResolvedProjectConfig,
   TargetName,
 } from "./types.js";
 
@@ -141,17 +147,19 @@ export async function cleanManagedFiles(
  * `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.
+ * which is also where the recommended layout writes every target's output, so
+ * protecting it wholesale would refuse to prune generated files. Under the
+ * default, protect only the directories that really are source plugins.
+ *
+ * Takes the loaded config rather than plugin discovery results, so `clean` keeps
+ * working when the source tree no longer loads: the scan below is a shallow
+ * readdir, not a build.
  */
 export async function buildDeleteGuard(
-  rootDir: string,
-  config: PluginpackConfig,
-  configPath: string,
+  project: ResolvedProjectConfig,
   force?: boolean,
 ): Promise<DeleteGuard> {
+  const { rootDir, config } = project;
   const protectedRoots: string[] = [];
   if (config.source?.skills) {
     protectedRoots.push(path.resolve(rootDir, config.source.skills));
@@ -166,7 +174,11 @@ export async function buildDeleteGuard(
       ...(await listSourcePluginDirs(path.resolve(rootDir, "plugins"))),
     );
   }
-  return { protectedRoots, configPath: path.resolve(configPath), force };
+  return {
+    protectedRoots,
+    configPath: path.resolve(project.configPath),
+    force,
+  };
 }
 
 function assertNoProtectedDeletions(
@@ -200,43 +212,20 @@ function assertNoProtectedDeletions(
  * name what it collided with — "resolves inside <path>" 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
- * 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.
+ * Containment is compared with case and Unicode normalization folded, since the
+ * filesystem may treat paths differing only in case as the same file.
  */
 function protectingRoot(
   outDir: string,
   relativePath: string,
   guard: DeleteGuard,
 ): string | null {
   const absolute = path.resolve(outDir, normalizeManagedPath(relativePath));
-  if (guard.configPath && pathsEqual(absolute, guard.configPath)) {
+  if (guard.configPath && isInsideFolded(guard.configPath, absolute)) {
     return guard.configPath;
   }
   return (
-    guard.protectedRoots.find(
-      (root) => pathsEqual(absolute, root) || isUnder(absolute, root),
-    ) ?? null
-  );
-}
-
-/** 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}`)
+    guard.protectedRoots.find((root) => isInsideFolded(root, absolute)) ?? null
   );
 }
 

tests/destructive.test.ts

@@ -494,9 +494,8 @@ 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.
+  // A guard false positive is invisible to a single fresh build — it only
+  // surfaces once something goes stale. These walk the whole cycle instead.
   const layouts: Array<[string, string]> = [
     [
       "README Recommended Shape (output under plugins/<target>/, source.plugins unset)",