From b5d3ab6c60770ed94fd36a0debccab4227da43cb Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Thu, 6 Aug 2026 10:23:10 -0700 Subject: [PATCH] refactor: address code-review findings on the containment fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #28. The Standards axis of the code review reported after that PR merged; these are its findings. No behaviour change — all 199 tests pass unchanged and each of #28's three reproductions was re-run and still resolves correctly. Duplicated Code, and the pointed one: listSourcePluginDirs was discoverSourcePlugins's scan loop copied verbatim — same exists guard, same readdir({withFileTypes}), same dotfile/non-dir skip, same isSourcePluginDir. Two copies of "what counts as a source plugin" drift, which is the bug class this whole review is about, and #28's own PR body claimed avoiding that duplication was the reason for putting the helper in config.ts. discoverSourcePlugins is now expressed over listSourcePluginDirs. Second instance: managed.ts's `pathsEqual(a, root) || isUnder(a, root)` was fs.ts's isInside plus folding. Replaced by one exported isInsideFolded beside isInside, which also removes a Mysterious Name — isUnder vs isInside gave no hint which one folded case. Data Clump: all three call sites passed project.rootDir, project.config, project.configPath, which is just ResolvedProjectConfig. buildDeleteGuard now takes (project, force), collapsing #28's sync->async ripple to one signature. Deliberately keeps taking the loaded config rather than discovery results, so clean still works when the source tree no longer loads. missing.reverse() was safe — it sat on the return line and the function exited immediately — but safety depended on a reader noticing the adjacent return. [...missing].reverse() costs nothing and removes the reasoning step. Comment style: dropped incident framing from two blocks, keeping the rationale. Co-Authored-By: Claude Opus 5 (1M context) --- src/build.ts | 6 +--- src/cleanup.ts | 14 ++------- src/config.ts | 25 ++++------------ src/fs.ts | 18 ++++++++++- src/managed.ts | 63 ++++++++++++++++----------------------- tests/destructive.test.ts | 5 ++-- 6 files changed, 54 insertions(+), 77 deletions(-) diff --git a/src/build.ts b/src/build.ts index 6a63f2b..1ec3bdb 100644 --- a/src/build.ts +++ b/src/build.ts @@ -26,11 +26,7 @@ export async function build(options: BuildOptions = {}): Promise { 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)); diff --git a/src/cleanup.ts b/src/cleanup.ts index 0721e28..3037f14 100644 --- a/src/cleanup.ts +++ b/src/cleanup.ts @@ -26,12 +26,7 @@ export async function prune( } = {}, ): Promise { 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 { 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[]); diff --git a/src/config.ts b/src/config.ts index f9d1797..29322b0 100644 --- a/src/config.ts +++ b/src/config.ts @@ -138,25 +138,12 @@ async function discoverSourcePlugins( sourceRoot: string, ): Promise> { const plugins = new Map(); - 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; } diff --git a/src/fs.ts b/src/fs.ts index 3e7c0eb..be75184 100644 --- a/src/fs.ts +++ b/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 { 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; diff --git a/src/managed.ts b/src/managed.ts index ea1dcbe..6892530 100644 --- a/src/managed.ts +++ b/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 { + 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,13 +212,8 @@ function assertNoProtectedDeletions( * 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 - * 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, @@ -214,29 +221,11 @@ function protectingRoot( 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 ); } diff --git a/tests/destructive.test.ts b/tests/destructive.test.ts index be3116d..d5e2002 100644 --- a/tests/destructive.test.ts +++ b/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//, source.plugins unset)",