From fbe2b60e63831a556ee829ed113ff246824a89f8 Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Wed, 21 Oct 2026 14:25:06 -0700 Subject: [PATCH 01/13] fix(fs): fail when a component directory is a file walkFiles treated ENOTDIR as "directory missing", so a source plugin whose skills (or any component) path was a regular file built successfully and shipped nothing from it. It now throws when the directory itself exists as a file, naming the path. A path below a file is still read as missing. --- src/fs.ts | 29 +++++++++- tests/physical-path-safety.test.ts | 89 ++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 tests/physical-path-safety.test.ts diff --git a/src/fs.ts b/src/fs.ts index 1b77464..73bdcd8 100644 --- a/src/fs.ts +++ b/src/fs.ts @@ -12,7 +12,9 @@ export function toPosix(value: string): string { * Dotfiles are included. Symlinks are followed: a link to a file is listed * as a file and a link to a directory is walked, except a link back to a * directory already being walked, which would loop. Broken links are - * skipped. A missing `dir` yields no files. + * skipped. A missing `dir` yields no files; a `dir` that is a file is an + * error, since reading it as an empty directory would silently drop content + * the author meant to ship. */ export async function walkFiles(dir: string): Promise { const files: string[] = []; @@ -23,6 +25,12 @@ export async function walkFiles(dir: string): Promise { real = await fs.realpath(current); entries = await fs.readdir(current, { withFileTypes: true }); } catch (err) { + if (await isExistingNonDirectory(err, current)) { + throw new Error( + `Expected a directory but found a file: ${current}. Move the file into a directory of that name, or remove it.`, + { cause: err }, + ); + } if (isNotFoundError(err)) { return; } @@ -193,3 +201,22 @@ export function isNotFoundError(error: unknown): boolean { const code = (error as NodeJS.ErrnoException).code; return code === "ENOENT" || code === "ENOTDIR"; } + +/** + * Whether `error` is an ENOTDIR raised because `dir` itself exists as a file, + * rather than because some ancestor of it is a file (which means `dir` is + * simply missing). + */ +async function isExistingNonDirectory( + error: unknown, + dir: string, +): Promise { + if ( + !(error instanceof Error) || + (error as NodeJS.ErrnoException).code !== "ENOTDIR" + ) { + return false; + } + const stats = await fs.stat(dir).catch(() => null); + return stats !== null && !stats.isDirectory(); +} diff --git a/tests/physical-path-safety.test.ts b/tests/physical-path-safety.test.ts new file mode 100644 index 0000000..b69681d --- /dev/null +++ b/tests/physical-path-safety.test.ts @@ -0,0 +1,89 @@ +import { rm, writeFile } from "node:fs/promises"; +import path from "node:path"; +import { Project } from "fixturify-project"; +import { afterEach, describe, expect, it } from "vitest"; +import { build } from "../src/build.js"; +import { walkFiles } from "../src/fs.js"; + +let project: Project | undefined; + +afterEach(async () => { + await project?.dispose(); + project = undefined; +}); + +const skill = (name: string) => + `---\nname: ${name}\ndescription: ${name} skill.\n---\n\n${name} body.\n`; + +function configText(targetsBlock: string, sourceBlock = ""): string { + return `import { defineConfig } from "${path.resolve("src/index.ts")}"; + +export default defineConfig({ + name: "physical-demo", + version: "1.0.0", + ${sourceBlock} + metadata: { description: "P", author: { name: "P" }, license: "MIT" }, + targets: { +${targetsBlock} + } +}); +`; +} + +/** + * A project with one canonical source plugin at `shared/acme` holding the + * skills `alpha` and `beta`. `targetsBlock` and `sourceBlock` are spliced into + * the config verbatim. + */ +async function makeProject( + targetsBlock: string, + sourceBlock = "", +): Promise { + project = new Project("pluginpack-physical", "1.0.0", { + files: { + "pluginpack.config.ts": configText(targetsBlock, sourceBlock), + shared: { + acme: { + skills: { + alpha: { "SKILL.md": skill("alpha") }, + beta: { "SKILL.md": skill("beta") }, + }, + }, + }, + "UNMANAGED.txt": "USER FILE\n", + }, + }); + await project.write(); + return project.baseDir; +} + +describe("walkFiles distinguishes a missing directory from a file", () => { + it("throws when the directory is a file", async () => { + const root = await makeProject(` antigravity: { + outDir: "out", + plugins: { acme: { source: "shared/acme" } } + }`); + await writeFile(path.join(root, "not-a-dir"), "x\n"); + + await expect(walkFiles(path.join(root, "not-a-dir"))).rejects.toThrow( + `Expected a directory but found a file: ${path.join(root, "not-a-dir")}`, + ); + await expect(walkFiles(path.join(root, "missing"))).resolves.toEqual([]); + await expect( + walkFiles(path.join(root, "not-a-dir/below")), + ).resolves.toEqual([]); + }); + + it("fails a build whose source component directory is a file", async () => { + const root = await makeProject(` antigravity: { + outDir: "out", + plugins: { acme: { source: "shared/acme" } } + }`); + await rm(path.join(root, "shared/acme/skills"), { recursive: true }); + await writeFile(path.join(root, "shared/acme/skills"), "oops\n"); + + await expect(build({ cwd: root })).rejects.toThrow( + /Expected a directory but found a file: .*shared\/acme\/skills/, + ); + }); +}); From 886db5c3a0e1bc490aebcb422169309881fa647b Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Wed, 21 Oct 2026 14:26:58 -0700 Subject: [PATCH 02/13] fix(config): keep source.partials inside the project source.partials was a bare string, so "../outside" or "/etc" was accepted and its files were inlined into shipped skill files. Partials end up in published output, so the field now uses the same safe-relative-path check as the other source paths and is rejected at config load. --- README.md | 2 +- src/schema.ts | 4 +++- tests/physical-path-safety.test.ts | 17 +++++++++++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 626eded..4d6c6f6 100644 --- a/README.md +++ b/README.md @@ -583,7 +583,7 @@ rest of the artifact. | ------------ | ------ | -------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `plugins` | string | no | Directory to discover source plugins from. Defaults to `plugins`. | | `skills` | string | no | Repo-level skills directory; creates a root source plugin from sibling component dirs. | -| `partials` | string | no | Directory of reusable `{{> name}}` text fragments, shared across every source plugin (see **Partials**). | +| `partials` | string | no | Directory of reusable `{{> name}}` text fragments, shared across every source plugin (see **Partials**). Safe relative: it must sit inside the project. | | `rootPlugin` | object | no | Metadata for that root skills plugin. Accepts all **`metadata`** fields plus `id`, `name`, `description`. `id` is the source-plugin name used in each target's `from` array. | **`metadata`** (and `source.rootPlugin`) diff --git a/src/schema.ts b/src/schema.ts index f6acb7d..fd2861e 100644 --- a/src/schema.ts +++ b/src/schema.ts @@ -41,7 +41,9 @@ const rootPluginSchema = metadataSchema.extend({ const sourceSchema = z.object({ plugins: z.string().optional(), skills: z.string().optional(), - partials: z.string().optional(), + // Partials are inlined into shipped output, so reading them from outside the + // project would publish files that are not part of it. + partials: safeRelativePath.optional(), rootPlugin: rootPluginSchema.optional(), }); diff --git a/tests/physical-path-safety.test.ts b/tests/physical-path-safety.test.ts index b69681d..8bb49e7 100644 --- a/tests/physical-path-safety.test.ts +++ b/tests/physical-path-safety.test.ts @@ -3,6 +3,7 @@ import path from "node:path"; import { Project } from "fixturify-project"; import { afterEach, describe, expect, it } from "vitest"; import { build } from "../src/build.js"; +import { loadConfig } from "../src/config.js"; import { walkFiles } from "../src/fs.js"; let project: Project | undefined; @@ -57,6 +58,22 @@ async function makeProject( return project.baseDir; } +describe("config path fields stay inside the project", () => { + it("rejects source.partials outside the project at config load", async () => { + const root = await makeProject( + ` antigravity: { + outDir: "out", + plugins: { acme: { source: "shared/acme" } } + }`, + `source: { partials: "../outside" },`, + ); + + await expect(loadConfig(root)).rejects.toThrow( + /Invalid pluginpack config in .*: source\.partials: must be a safe relative path/, + ); + }); +}); + describe("walkFiles distinguishes a missing directory from a file", () => { it("throws when the directory is a file", async () => { const root = await makeProject(` antigravity: { From 598f211b4cbd4c61ae0dac04e723d7871565d6e5 Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Wed, 21 Oct 2026 14:26:58 -0700 Subject: [PATCH 03/13] fix(managed): compare physical paths and check deletions before writing The delete guard, the cross-target collision checks and clean's manifest check all compared path.resolve strings. An alias walked past them: an outDir symlinked into a source plugin let build and clean delete authored skills, and "out" and "OUT" on a case-insensitive filesystem let two targets overwrite each other's files and clean delete the other target's live output. These checks now compare physical keys: the real path of the nearest existing ancestor, folded for case and Unicode normalization when that volume is case-insensitive. loadConfig also rejects an outDir that lands inside any source, overrides, partials, repositoryFiles or rootFiles path, so the alias case fails before anything is written. An outDir that is only a symlink to somewhere safe keeps working. The guard no longer guesses source plugins from component directories under the default plugins/ root. That guess read a generated package such as plugins/acme/skills/ as source and blocked its own prune whenever outDir was the repo root. It now protects what the config reads: an explicit source.plugins, the legacy plugins a target reads through "from", and any directory with a plugin.pluginpack.json. rootFiles sources and each outDir's .git are protected too, and a rootFiles entry that copies a file onto itself is rejected. build and prune now share one plan that runs every check before any write. That plan includes the delete guard over stale files, so --dry-run fails wherever the real command would, and a refused prune no longer leaves a half-written build. A stale path that another target produces in the same build, or that another configured target's manifest claims, has changed owner. It is left in place instead of being deleted from under the new owner. The managed manifest moves to version 2. It records a sha256 for each file. build, prune and clean warn before overwriting or deleting a managed file someone edited, and still go ahead. Version 1 manifests are read and upgraded on the next build. A manifest from a newer pluginpack gets its own error saying to upgrade, distinct from a corrupt one. Manifest writes go through the same containment check as artifact writes, so a symlinked .pluginpack directory cannot redirect them. A failed removal now names the manifest and the recovery step. --- README.md | 10 +- src/build.ts | 168 +++++-- src/cleanup.ts | 79 ++-- src/config.ts | 170 +++++++- src/fs.ts | 75 ++++ src/managed.ts | 423 +++++++++++++----- src/schema.ts | 4 + tests/destructive.test.ts | 12 +- tests/physical-path-safety.test.ts | 675 ++++++++++++++++++++++++++++- 9 files changed, 1416 insertions(+), 200 deletions(-) diff --git a/README.md b/README.md index 4d6c6f6..196e47a 100644 --- a/README.md +++ b/README.md @@ -188,7 +188,13 @@ Each emitted plugin names one `source`. Optional `overrides` are applied after the source, so it can add or replace files for that target. `repositoryFiles` copies a whole directory into the generated repository root. -`pluginpack` writes a `.pluginpack/.json` managed-file manifest for each built target. That manifest lets builds and cleanup commands remove stale generated files without touching source files or unmanaged repo content. +`pluginpack` writes a `.pluginpack/.json` managed-file manifest for each built target. That manifest lets builds and cleanup commands remove stale generated files without touching source files or unmanaged repo content. It also records a sha256 of each file as written. If you edit a generated file and a later `build`, `prune` or `clean` would overwrite or delete it, pluginpack prints a warning naming the file first. It still goes ahead, because the output belongs to pluginpack; move the edit into the source to keep it. Manifests from 0.11 and earlier have no hashes, so they produce no warnings until the next build rewrites them. + +Before it deletes anything, pluginpack compares real paths, with symlinks resolved and, on a case-insensitive filesystem, case folded. It refuses an `outDir` that lands inside a source, overrides, partials, `repositoryFiles` or `rootFiles` path, even through a symlink. It also refuses two targets that write the same file through different spellings, such as `out` and `OUT` on macOS. `--dry-run` runs the same checks, so it fails wherever the real command would. + +An `outDir` outside the project, such as a sibling output repository (`../cursor-plugins`), works. The guard there knows only that repository's `.git` directory and the files the manifest lists, so it cannot protect anything else in that repository from a manifest someone edited by hand. + +**Upgrading from 0.11 (managed files):** the first build rewrites each manifest as version 2. pluginpack 0.11 cannot read a version 2 manifest, so upgrade every machine and CI job that builds the same output together. A manifest from a newer pluginpack fails with a message saying to upgrade. Some configs that 0.11 accepted now fail at load or build: `source.partials` outside the project, an `outDir` inside a source path, and a `rootFiles` entry that copies a file onto itself (`"README.md": "README.md"` with `outDir: "."`). Each error names the field to change. ## Components @@ -606,7 +612,7 @@ rest of the artifact. | Field | Type | Required | Meaning | | ------------------ | ------------------------------- | -------- | ------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `outDir` | string | yes | Output directory for this target, relative to the config root. | +| `outDir` | string | yes | Output directory for this target, relative to the config root. May be outside the project; it may not sit inside a source path. | | `format` | `"agent-plugins"` \| `"legacy"` | no | `codex` only. `"legacy"` keeps the `.codex-plugin/plugin.json` layout; default `"agent-plugins"`. | | `plugins` | record | yes | Emitted plugins, keyed by emitted plugin name (see **`targets..plugins.`**). | | `marketplaceDir` | string (safe relative) | no | Override the marketplace dir (defaults: `.cursor-plugin` / `.claude-plugin`). | diff --git a/src/build.ts b/src/build.ts index 1ec3bdb..a56fb90 100644 --- a/src/build.ts +++ b/src/build.ts @@ -1,12 +1,16 @@ import path from "node:path"; -import { loadConfig } from "./config.js"; -import { writeArtifact } from "./fs.js"; +import { assertOutDirsOutsideSource, loadConfig } from "./config.js"; +import { physicalKey, writeArtifact } from "./fs.js"; import { + assertManagedManifestContained, buildDeleteGuard, + executePrune, normalizeManagedPath, - pruneManagedFiles, + planPrune, readManagedManifest, + warnOnEditedOverwrites, writeManagedManifest, + type PrunePlan, } from "./managed.js"; import { emitTarget, targetNames } from "./adapters.js"; import { @@ -22,18 +26,10 @@ import type { /** Builds every configured (or explicitly selected) target and writes its output, unless `dryRun` is set. */ export async function build(options: BuildOptions = {}): Promise { - const project = await loadConfig(options.cwd, options.configPath); - const targets = options.target - ? [options.target] - : targetNames.filter((target) => project.config.targets[target]); - const guard = await buildDeleteGuard(project); - const artifacts: Artifact[] = []; - for (const target of targets) { - artifacts.push(await emitTarget(project, target, options.outDir)); + const { artifacts, prunes } = await planBuild(options); + for (const plan of prunes) { + await warnOnEditedOverwrites(plan, options.dryRun); } - const owner = assertNoCrossTargetCollisions(artifacts); - await assertNoCollisionsWithBuiltTargets(project, targets, owner); - assertNoUnsubstitutedPartialTags(artifacts); if (!options.dryRun) { // Write every target's new files before pruning any target's stale ones. // If a later target's write throws, no target has had files pruned yet — @@ -44,14 +40,70 @@ export async function build(options: BuildOptions = {}): Promise { for (const artifact of artifacts) { await writeArtifact(artifact.outDir, artifact.files); } - for (const artifact of artifacts) { - await pruneManagedFiles(artifact, { guard }); - await writeManagedManifest(artifact); + for (const plan of prunes) { + await executePrune(plan); + await writeManagedManifest(plan.artifact); } } return artifacts; } +/** + * Emits every selected target and runs every check that could refuse the + * build, without touching disk: cross-target collisions, collisions with other + * targets' manifests, unsubstituted partials, self-referential rootFiles, + * manifest containment, and the delete guard over each target's stale files. + * + * `build` and `prune` both start here, and a dry run stops here, so a dry run + * fails exactly where the real command would, and a refused prune never + * leaves a half-written build behind. `force` relaxes only the delete guard. + */ +export async function planBuild( + options: BuildOptions, + force?: boolean, +): Promise<{ artifacts: Artifact[]; prunes: PrunePlan[] }> { + const project = await loadConfig(options.cwd, options.configPath); + const targets = options.target + ? [options.target] + : targetNames.filter((target) => project.config.targets[target]); + const guard = await buildDeleteGuard(project, force); + const artifacts: Artifact[] = []; + for (const target of targets) { + artifacts.push(await emitTarget(project, target, options.outDir)); + } + if (options.outDir) { + await assertOutDirsOutsideSource( + project, + artifacts.map((artifact) => ({ + target: artifact.target, + outDir: artifact.outDir, + })), + ); + } + const incoming = await assertNoCrossTargetCollisions(artifacts); + const claimedElsewhere = await assertNoCollisionsWithBuiltTargets( + project, + targets, + incoming, + ); + assertNoUnsubstitutedPartialTags(artifacts); + await assertNoSelfReferentialRootFiles(project, artifacts); + for (const artifact of artifacts) { + await assertManagedManifestContained(artifact.outDir, artifact.target); + } + const owners = new Map(claimedElsewhere); + for (const [key, claim] of incoming) { + owners.set(key, claim.target); + } + const prunes: PrunePlan[] = []; + for (const artifact of artifacts) { + prunes.push( + await planPrune(artifact, { dryRun: options.dryRun, guard, owners }), + ); + } + return { artifacts, prunes }; +} + /** * A `{{> name}}` tag that reaches output is broken content: whatever reads the * file gets a template marker instead of the text it was meant to inline. @@ -74,25 +126,69 @@ function assertNoUnsubstitutedPartialTags(artifacts: Artifact[]): void { } } +/** + * A `rootFiles` entry whose destination is its own source makes the authored + * file a managed output: the build copies it onto itself, and the next `clean` + * (or a prune after the entry is removed) deletes the only copy. Refuse it + * before writing anything. (A `repositoryFiles` directory that is the outDir + * is caught earlier, at config load, as an outDir inside a source path.) + */ +async function assertNoSelfReferentialRootFiles( + project: ResolvedProject, + artifacts: Artifact[], +): Promise { + const problems: string[] = []; + for (const artifact of artifacts) { + const targetConfig = project.config.targets[artifact.target]; + for (const [dest, source] of Object.entries( + targetConfig?.rootFiles ?? {}, + )) { + const [destKey, sourceKey] = await Promise.all([ + physicalKey(path.resolve(artifact.outDir, dest)), + physicalKey(path.resolve(project.rootDir, source)), + ]); + if (destKey === sourceKey) { + problems.push( + ` targets.${artifact.target}.rootFiles["${dest}"] copies ${path.resolve(project.rootDir, source)} onto itself`, + ); + } + } + } + if (problems.length > 0) { + throw new Error( + `Invalid pluginpack config: a generated file would replace its own source:\n${problems.join("\n")}\n` + + `pluginpack would then manage, and later delete, the authored file. ` + + `Map each entry to a different destination, or remove it if the file already lives in the output repository.`, + ); + } +} + +/** Which target claims a path, and the path as that target names it. */ +type Claim = { target: TargetName; absolute: string }; + /** * Two targets pointed at overlapping output paths would silently overwrite each * other (and one target's prune could delete the other's files). Catch it. - * Returns the absolute-path -> owning-target map so - * `assertNoCollisionsWithBuiltTargets` can reuse it. + * Keys on physical paths, so `out` and `OUT` on a case-insensitive filesystem, + * or an outDir reached through a symlink, are one path, as they are on disk. + * Returns the physical-key -> claim map so later checks can reuse it. */ -function assertNoCrossTargetCollisions( +async function assertNoCrossTargetCollisions( artifacts: Artifact[], -): Map { - const owner = new Map(); +): Promise> { + const owner = new Map(); const collisions: string[] = []; for (const artifact of artifacts) { for (const managedPath of artifact.managedPaths) { const absolute = path.resolve(artifact.outDir, managedPath); - const previous = owner.get(absolute); - if (previous && previous !== artifact.target) { - collisions.push(` ${previous} and ${artifact.target}: ${absolute}`); + const key = await physicalKey(absolute, { entry: true }); + const previous = owner.get(key); + if (previous && previous.target !== artifact.target) { + collisions.push( + ` ${previous.target} and ${artifact.target}: ${describeAlias(previous.absolute, absolute)}`, + ); } else { - owner.set(absolute, artifact.target); + owner.set(key, { target: artifact.target, absolute }); } } } @@ -113,13 +209,17 @@ function assertNoCrossTargetCollisions( * against that by also checking incoming paths against every other * *configured* target's on-disk managed manifest, not just artifacts present * in the current invocation. + * + * Returns the physical key of every path those other manifests claim, so + * prune can leave a stale path alone when another target owns it now. */ async function assertNoCollisionsWithBuiltTargets( project: ResolvedProject, targets: TargetName[], - incoming: Map, -): Promise { + incoming: Map, +): Promise> { const building = new Set(targets); + const claimed = new Map(); const collisions: string[] = []; for (const other of targetNames) { if (building.has(other)) { @@ -136,10 +236,12 @@ async function assertNoCollisionsWithBuiltTargets( } for (const file of manifest.files) { const absolute = path.resolve(otherOutDir, normalizeManagedPath(file)); - const conflicting = incoming.get(absolute); + const key = await physicalKey(absolute, { entry: true }); + claimed.set(key, other); + const conflicting = incoming.get(key); if (conflicting) { collisions.push( - ` ${other} (previously built) and ${conflicting}: ${absolute}`, + ` ${other} (previously built) and ${conflicting.target}: ${describeAlias(absolute, conflicting.absolute)}`, ); } } @@ -150,4 +252,10 @@ async function assertNoCollisionsWithBuiltTargets( `Run a full \`build\` (no --target) to see every target's paths at once, or fix the outDir configuration.`, ); } + return claimed; +} + +/** One path, or both spellings when two different strings name the same file. */ +export function describeAlias(first: string, second: string): string { + return first === second ? first : `${first} (same file as ${second})`; } diff --git a/src/cleanup.ts b/src/cleanup.ts index 3037f14..7708340 100644 --- a/src/cleanup.ts +++ b/src/cleanup.ts @@ -1,12 +1,14 @@ import path from "node:path"; -import { build } from "./build.js"; +import { describeAlias, planBuild } from "./build.js"; import { targetNames } from "./adapters.js"; import { loadProjectConfig } from "./config.js"; +import { physicalKey } from "./fs.js"; import { buildDeleteGuard, cleanManagedFiles, + executePrune, normalizeManagedPath, - pruneManagedFiles, + pruneResult, readManagedManifest, } from "./managed.js"; import type { @@ -15,7 +17,12 @@ import type { TargetName, } from "./types.js"; -/** Deletes managed files no longer produced by the current build, without rewriting current output. */ +/** + * Deletes managed files no longer produced by the current build, without + * rewriting current output. Runs the same plan, and so the same checks, as + * `build`: the delete guard, cross-target ownership of stale paths, and every + * collision check. `--dry-run` runs them too and only skips the deletes. + */ export async function prune( options: { cwd?: string; @@ -25,21 +32,21 @@ export async function prune( force?: boolean; } = {}, ): Promise { - const project = await loadProjectConfig(options.cwd, options.configPath); - const guard = await buildDeleteGuard(project, options.force); - const artifacts = await build({ - cwd: options.cwd, - configPath: options.configPath, - target: options.target, - dryRun: true, - }); - const results: CleanupResult[] = []; - for (const artifact of artifacts) { - results.push( - await pruneManagedFiles(artifact, { dryRun: options.dryRun, guard }), - ); + const { prunes } = await planBuild( + { + cwd: options.cwd, + configPath: options.configPath, + target: options.target, + dryRun: options.dryRun, + }, + options.force, + ); + if (!options.dryRun) { + for (const plan of prunes) { + await executePrune(plan); + } } - return results; + return prunes.map(pruneResult); } /** Deletes every managed file for the given target(s), tearing the generated output down entirely. */ @@ -78,17 +85,20 @@ export async function clean( /** * Refuses to clean a path another configured target's manifest also claims. * - * `build()` rejects two targets writing overlapping output paths, and `prune()` - * inherits that check for free by running `build({ dryRun: true })` first. - * `clean()` deliberately never builds — teardown has to keep working when the - * source tree or config no longer does, which is often exactly why someone is - * cleaning — so it cannot inherit the check the same way. This is the - * standalone equivalent, working purely from the manifests on disk. + * `build()` and `prune()` share one plan that rejects overlapping output and + * leaves a stale path alone when another target owns it. `clean()` + * deliberately never builds — teardown has to keep working when the source + * tree or config no longer does, which is often exactly why someone is + * cleaning — so it cannot share that plan. This is the standalone check, + * working purely from the manifests on disk. * - * Overlapping manifests can only come from a pluginpack older than that build - * check, or from a manifest edited out of band; either way the file being - * deleted is another target's live output, so refuse rather than delete it. - * `--force` still wins, so nobody ends up unable to tear down their own repo. + * Paths are compared physically, so two outDirs that differ only in case on a + * case-insensitive filesystem, or that alias one directory through a symlink, + * collide here just as they do on disk. Overlapping manifests otherwise come + * from a pluginpack older than the build check, or from a manifest edited out + * of band; either way the file being deleted is another target's live output, + * so refuse rather than delete it. `--force` still wins, so nobody ends up + * unable to tear down their own repo. */ async function assertNoManifestCollisions( project: ResolvedProjectConfig, @@ -98,7 +108,7 @@ async function assertNoManifestCollisions( if (force) { return; } - const owners = new Map(); + const owners = new Map(); const collisions: string[] = []; for (const target of targetNames) { const targetConfig = project.config.targets[target]; @@ -112,14 +122,17 @@ async function assertNoManifestCollisions( } for (const file of manifest.files) { const absolute = path.resolve(outDir, normalizeManagedPath(file)); - const owner = owners.get(absolute); - if (owner && owner !== target) { - if (cleaning.includes(owner) || cleaning.includes(target)) { - collisions.push(` ${owner} and ${target}: ${absolute}`); + const key = await physicalKey(absolute, { entry: true }); + const owner = owners.get(key); + if (owner && owner.target !== target) { + if (cleaning.includes(owner.target) || cleaning.includes(target)) { + collisions.push( + ` ${owner.target} and ${target}: ${describeAlias(owner.absolute, absolute)}`, + ); } continue; } - owners.set(absolute, target); + owners.set(key, { target, absolute }); } } if (collisions.length > 0) { diff --git a/src/config.ts b/src/config.ts index 29322b0..0117963 100644 --- a/src/config.ts +++ b/src/config.ts @@ -4,7 +4,7 @@ import { pathToFileURL } from "node:url"; import { createJiti } from "jiti"; import { z } from "zod"; import { componentDirs } from "./components.js"; -import { exists } from "./fs.js"; +import { exists, isInside, physicalKey } from "./fs.js"; import { loadPartials } from "./partials.js"; import { configSchema, sourcePluginManifestSchema } from "./schema.js"; import { createFilesystemSourceProvider } from "./source.js"; @@ -27,6 +27,7 @@ export async function loadConfig( configPath?: string, ): Promise { const projectConfig = await loadProjectConfig(cwd, configPath); + await assertOutDirsOutsideSource(projectConfig); const { config, rootDir } = projectConfig; const sourceRoot = path.resolve(rootDir, config.source?.plugins ?? "plugins"); const plugins = await discoverSourcePlugins(sourceRoot); @@ -148,19 +149,166 @@ async function discoverSourcePlugins( return plugins; } +/** An authored path the delete guard protects, with the config field that names it. */ +export type SourcePath = { + path: string; + field: string; +}; + /** - * Directories under `sourceRoot` that are actually source plugins. + * Every authored path the config reads from: the paths prune and clean must + * never delete and a target's outDir must never sit inside. * - * 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. + * Derived from the config, not from what is on disk, so `clean` keeps working + * when the source tree no longer loads. `source.plugins` is protected whole + * only when the config sets it. Under the default `plugins/` root — where the + * README's layouts also write generated output — only the legacy source + * plugins a target actually reads through `from`, and directories carrying a + * `plugin.pluginpack.json` (which generated output never contains), count as + * source. Guessing from component directories misreads a generated package + * (`plugins/acme/skills/`) as source and blocks its own prune. */ -export async function listSourcePluginDirs( - sourceRoot: string, -): Promise { +export async function listSourcePaths( + project: ResolvedProjectConfig, +): Promise { + const { rootDir, config } = project; + const paths: SourcePath[] = []; + const add = (value: string, field: string) => + paths.push({ path: path.resolve(rootDir, value), field }); + if (config.source?.skills) { + add(config.source.skills, "source.skills"); + } + if (config.source?.partials) { + add(config.source.partials, "source.partials"); + } + if (config.source?.plugins) { + add(config.source.plugins, "source.plugins"); + } else { + const defaultRoot = path.resolve(rootDir, "plugins"); + const rootPluginId = config.source?.skills + ? (config.source.rootPlugin?.id ?? "core") + : undefined; + const legacyIds = new Set(); + for (const target of Object.values(config.targets)) { + for (const plugin of Object.values(target?.plugins ?? {})) { + for (const id of plugin.from ?? []) { + // The root skills plugin reads only source.skills, protected above. + if (id !== rootPluginId) { + legacyIds.add(id); + } + } + } + } + for (const id of legacyIds) { + paths.push({ + path: path.join(defaultRoot, id), + field: `legacy source plugin "${id}" (from)`, + }); + } + for (const dir of await listManifestPluginDirs(defaultRoot)) { + if (!legacyIds.has(path.basename(dir))) { + paths.push({ path: dir, field: "plugin.pluginpack.json" }); + } + } + } + for (const [name, target] of Object.entries(config.targets)) { + if (!target) { + continue; + } + if (target.repositoryFiles) { + add(target.repositoryFiles, `targets.${name}.repositoryFiles`); + } + for (const [dest, source] of Object.entries(target.rootFiles ?? {})) { + add(source, `targets.${name}.rootFiles["${dest}"]`); + } + for (const [pluginName, plugin] of Object.entries(target.plugins)) { + if (plugin.source) { + add(plugin.source, `targets.${name}.plugins.${pluginName}.source`); + } + if (plugin.overrides) { + add( + plugin.overrides, + `targets.${name}.plugins.${pluginName}.overrides`, + ); + } + } + } + return paths; +} + +/** + * Rejects a config whose target outDir physically resolves inside an authored + * path: building there overwrites source, and prune/clean would then have to + * delete it. Compares physical paths, so an outDir that only reaches source + * through a symlink, or through a case variant the filesystem forgives, is + * caught too. An outDir that is merely a symlink to somewhere safe still works. + * + * Checks every configured outDir, or only `outDirs` when given (the absolute + * outDirs a `--out-dir` override produced). + */ +export async function assertOutDirsOutsideSource( + project: ResolvedProjectConfig, + outDirs?: Array<{ target: string; outDir: string }>, +): Promise { + const sources = await listSourcePaths(project); + const sourceKeys = await Promise.all( + sources.map(async (source) => ({ + ...source, + key: await physicalKey(source.path), + })), + ); + const checked = + outDirs ?? + Object.entries(project.config.targets).flatMap(([target, config]) => + config + ? [{ target, outDir: path.resolve(project.rootDir, config.outDir) }] + : [], + ); + const problems: string[] = []; + for (const { target: name, outDir } of checked) { + const outKey = await physicalKey(outDir); + for (const source of sourceKeys) { + if (isInside(source.key, outKey)) { + problems.push( + ` targets.${name}.outDir (${outDir}) resolves inside ${source.path} (${source.field})`, + ); + } + } + } + if (problems.length > 0) { + throw new Error( + `Invalid pluginpack config in ${project.configPath}: a target writes its output inside an authored source path:\n${problems.join("\n")}\n` + + `A build there overwrites source files, and prune or clean would delete them. ` + + `Point each outDir at a directory outside every source, overrides, partials, repositoryFiles and rootFiles path.`, + ); + } +} + +/** + * Directories directly under `sourceRoot` that carry a + * `plugin.pluginpack.json`. Generated output never contains that file, so + * these are authored source whether or not a target reads them. + */ +async function listManifestPluginDirs(sourceRoot: string): Promise { + if (!(await exists(sourceRoot))) { + return []; + } + const dirs: string[] = []; + const entries = await fs.readdir(sourceRoot, { withFileTypes: true }); + for (const entry of entries) { + const dir = path.join(sourceRoot, entry.name); + if ( + entry.isDirectory() && + (await exists(path.join(dir, "plugin.pluginpack.json"))) + ) { + dirs.push(dir); + } + } + return dirs; +} + +/** Directories under `sourceRoot` that legacy discovery reads as source plugins. */ +async function listSourcePluginDirs(sourceRoot: string): Promise { if (!(await exists(sourceRoot))) { return []; } diff --git a/src/fs.ts b/src/fs.ts index 73bdcd8..df5b273 100644 --- a/src/fs.ts +++ b/src/fs.ts @@ -81,6 +81,81 @@ function fold(value: string): string { return value.normalize("NFC").toLowerCase(); } +/** + * The key two paths share when they name the same file on this host: the + * physical path (symlinks resolved on the deepest existing ancestor), folded + * for case and Unicode normalization when that ancestor's volume compares + * names case-insensitively (APFS and NTFS by default). + * + * Every check that decides whether a delete or write is safe, or whether two + * targets claim the same file, compares these keys. Comparing `path.resolve` + * strings instead lets an alias walk past the check: `out` and `OUT` on APFS, + * or a symlink into a source tree, are different strings but one file. + * + * With `entry` set, only the parent is resolved and the final name is kept, + * which is the file `fs.rm` removes when the final entry is itself a symlink. + * Use it for deletions and ownership; leave it off for a root being protected. + */ +export async function physicalKey( + target: string, + options: { entry?: boolean } = {}, +): Promise { + const absolute = path.resolve(target); + const parent = path.dirname(absolute); + const real = + options.entry && parent !== absolute + ? path.join(await realpathDeep(parent), path.basename(absolute)) + : await realpathDeep(absolute); + return (await isCaseInsensitiveAt(real)) ? fold(real) : real; +} + +const caseInsensitiveDirs = new Map>(); + +/** + * Whether names under `real`'s volume compare case-insensitively. Probes the + * deepest existing component of `real` that has a cased letter: if the + * case-swapped name is the same inode, the directory holding it folds case. + * Falls back to the platform default (folding on macOS and Windows) when no + * component can be probed, since over-matching is the safe direction for a + * check that decides whether to refuse. + */ +async function isCaseInsensitiveAt(real: string): Promise { + let current = real; + for (;;) { + const parent = path.dirname(current); + if (parent === current) { + return process.platform === "darwin" || process.platform === "win32"; + } + const name = path.basename(current); + const swapped = swapCase(name); + // Skip names whose case swap does not round-trip ("ß" -> "SS"): the + // swapped name would be a different name, not a different case. + if (swapped !== name && swapCase(swapped) === name) { + const stats = await fs.lstat(current).catch(() => null); + if (stats) { + let cached = caseInsensitiveDirs.get(parent); + if (!cached) { + cached = fs.lstat(path.join(parent, swapped)).then( + (other) => other.ino === stats.ino && other.dev === stats.dev, + () => false, + ); + caseInsensitiveDirs.set(parent, cached); + } + return cached; + } + } + current = parent; + } +} + +function swapCase(value: string): string { + return [...value] + .map((char) => + char === char.toLowerCase() ? char.toUpperCase() : char.toLowerCase(), + ) + .join(""); +} + /** * Resolves `target` with symlinks followed, or returns `null` if the result * escapes `root`. diff --git a/src/managed.ts b/src/managed.ts index 86f86c4..2347639 100644 --- a/src/managed.ts +++ b/src/managed.ts @@ -1,26 +1,40 @@ +import { createHash } from "node:crypto"; import { promises as fs } from "node:fs"; import path from "node:path"; import { isInside, isInsideFolded, json, + physicalKey, resolveInside, toPosix, + writeArtifact, } from "./fs.js"; -import { listSourcePluginDirs } from "./config.js"; +import { listSourcePaths } from "./config.js"; import type { Artifact, CleanupEntry, CleanupResult, DeleteGuard, + FileValue, ResolvedProjectConfig, TargetName, } from "./types.js"; -type ManagedManifest = { - version: 1; +/** The newest managed-manifest version this pluginpack reads and the one it writes. */ +const MANIFEST_VERSION = 2; + +/** + * A target's managed-file manifest, as read. Version 2 records a sha256 of + * each file as written, so a later build, prune or clean can tell a file it + * wrote from one someone edited since. A version 1 manifest (paths only) reads + * as version 2 with no hashes and is upgraded by the next build. + */ +export type ManagedManifest = { + version: typeof MANIFEST_VERSION; target: TargetName; files: string[]; + sha256: Record; }; /** Output-relative path of a target's managed-file manifest. */ @@ -28,19 +42,57 @@ export function managedManifestPath(target: TargetName): string { return toPosix(path.join(".pluginpack", `${target}.json`)); } -/** Writes the list of files a build produced, for later prune/clean/diff to compare against. */ +/** The hex sha256 of a file's contents, as recorded in the managed manifest. */ +function contentHash(value: FileValue): string { + return createHash("sha256").update(value).digest("hex"); +} + +/** + * Writes the list of files a build produced, with their hashes, for later + * prune/clean/diff to compare against. Goes through the same containment check + * as the artifact itself, so a symlinked `.pluginpack/` cannot redirect the + * write outside the output directory. + */ export async function writeManagedManifest(artifact: Artifact): Promise { + const sha256: Record = {}; + for (const managedPath of artifact.managedPaths) { + const value = artifact.files.get(managedPath); + if (value !== undefined) { + sha256[managedPath] = contentHash(value); + } + } const manifest: ManagedManifest = { - version: 1, + version: MANIFEST_VERSION, target: artifact.target, files: artifact.managedPaths, + sha256, }; - const destination = path.join( + await writeArtifact( artifact.outDir, - managedManifestPath(artifact.target), + new Map([[managedManifestPath(artifact.target), json(manifest)]]), ); - await fs.mkdir(path.dirname(destination), { recursive: true }); - await fs.writeFile(destination, json(manifest)); +} + +/** + * Throws if writing the target's managed manifest would leave the output + * directory. Build runs this before writing anything, so a planted + * `.pluginpack` symlink fails the build up front instead of after the + * artifact is already on disk. + */ +export async function assertManagedManifestContained( + outDir: string, + target: TargetName, +): Promise { + const relative = managedManifestPath(target); + if ( + (await resolveInside(path.resolve(outDir), path.join(outDir, relative))) === + null + ) { + throw new Error( + `Refusing to write through a symlink that leaves the output directory: ${relative} (under ${outDir}). ` + + `Replace the .pluginpack symlink with a real directory.`, + ); + } } /** Reads a target's managed-file manifest, or `null` if none exists yet. */ @@ -58,60 +110,172 @@ export async function readManagedManifest( } throw error; } - let parsed: Partial | null; + const invalid = (reason: string) => + new Error( + `Invalid managed manifest: ${manifestPath} (${reason}). ` + + `It records which files pluginpack may delete. Restore it from version control, ` + + `or delete it and rebuild; pluginpack then leaves any files it listed in place.`, + ); + let parsed: unknown; try { - parsed = JSON.parse(raw) as Partial | null; + parsed = JSON.parse(raw); } catch { - throw new Error(`Invalid managed manifest: ${manifestPath}`); + throw invalid("not valid JSON"); + } + // `JSON.parse` happily returns null, a number, or a string for a truncated + // or hand-edited manifest; those must land on the same clear error as + // malformed JSON, not a TypeError from the field checks below. + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { + throw invalid("not a JSON object"); + } + const record = parsed as Record; + const version = record.version; + if ( + typeof version === "number" && + Number.isInteger(version) && + version > MANIFEST_VERSION + ) { + throw new Error( + `Managed manifest ${manifestPath} has version ${version}, so a newer pluginpack wrote it. ` + + `This pluginpack reads versions 1 to ${MANIFEST_VERSION}. Upgrade pluginpack to build, prune or clean this output.`, + ); + } + if (version !== 1 && version !== 2) { + throw invalid(`unsupported version ${JSON.stringify(version)}`); + } + if (record.target !== target) { + throw invalid( + `target is ${JSON.stringify(record.target)}, expected "${target}"`, + ); } + const files = record.files; if ( - // `JSON.parse` happily returns null, a number, or a string for a - // truncated or hand-edited manifest; those must land on the same clear - // error as malformed JSON, not a TypeError from the field checks below. - !parsed || - typeof parsed !== "object" || - Array.isArray(parsed) || - parsed.version !== 1 || - parsed.target !== target || - !Array.isArray(parsed.files) || - !parsed.files.every((file) => typeof file === "string") + !Array.isArray(files) || + !files.every((file): file is string => typeof file === "string") ) { - throw new Error(`Invalid managed manifest: ${manifestPath}`); + throw invalid('"files" must be an array of paths'); } - return parsed as ManagedManifest; + let sha256: Record = {}; + if (version === 2) { + const hashes = record.sha256; + if ( + !hashes || + typeof hashes !== "object" || + Array.isArray(hashes) || + !Object.values(hashes).every((hash) => typeof hash === "string") + ) { + throw invalid('"sha256" must map each path to a hash'); + } + sha256 = hashes as Record; + } + return { version: MANIFEST_VERSION, target, files, sha256 }; } -/** Deletes files the previous build managed but the current one no longer produces. */ -export async function pruneManagedFiles( +/** What a prune will delete, decided before anything is written or removed. */ +export type PrunePlan = { + artifact: Artifact; + previous: ManagedManifest | null; + stale: string[]; +}; + +/** + * Decides which previously managed files are stale and checks it is safe to + * delete them, without touching disk. Runs on dry runs too, so `--dry-run` + * fails exactly where the real command would. + * + * `owners` maps the physical key of every path another target produces in + * this build, or claims in its own manifest, to that target. A stale path + * another target owns has changed hands (for example a marketplace file moved + * from one target to another sharing the outDir) and is left in place rather + * than deleted from under its new owner. This target's next manifest no + * longer lists it, so the ownership transfer completes on the next build. + */ +export async function planPrune( artifact: Artifact, - options: { dryRun?: boolean; guard?: DeleteGuard } = {}, -): Promise { + options: { + dryRun?: boolean; + guard?: DeleteGuard; + owners?: Map; + } = {}, +): Promise { const previous = await readManagedManifest(artifact.outDir, artifact.target); const current = new Set(artifact.managedPaths.map(normalizeManagedPath)); - const stale = (previous?.files ?? []) - .map(normalizeManagedPath) - .filter((file) => !current.has(file)); - if (!options.dryRun) { - assertNoProtectedDeletions(artifact.outDir, stale, options.guard, "prune"); - } - const entries: CleanupEntry[] = []; - for (const normalized of stale) { - entries.push({ - type: "stale", - target: artifact.target, - path: normalized, - }); - if (!options.dryRun) { - await removeManagedPath(artifact.outDir, normalized); + const stale: string[] = []; + for (const file of (previous?.files ?? []).map(normalizeManagedPath)) { + if (current.has(file)) { + continue; + } + if (options.owners) { + const key = await physicalKey(path.resolve(artifact.outDir, file), { + entry: true, + }); + const owner = options.owners.get(key); + if (owner && owner !== artifact.target) { + continue; + } } + stale.push(file); } + await assertNoProtectedDeletions( + artifact.outDir, + stale, + options.guard, + "prune", + ); + await warnOnEditedFiles( + artifact.outDir, + artifact.target, + stale, + previous, + options.dryRun ? "a real run would delete it" : "deleting it", + ); + return { artifact, previous, stale }; +} + +/** Deletes the stale files a `planPrune` decided on. */ +export async function executePrune(plan: PrunePlan): Promise { + for (const file of plan.stale) { + await removeManagedPath(plan.artifact.outDir, file, plan.artifact.target); + } +} + +/** The cleanup report for a prune plan. */ +export function pruneResult(plan: PrunePlan): CleanupResult { return { - target: artifact.target, - outDir: artifact.outDir, - entries, + target: plan.artifact.target, + outDir: plan.artifact.outDir, + entries: plan.stale.map((file): CleanupEntry => ({ + type: "stale", + target: plan.artifact.target, + path: file, + })), }; } +/** + * Warns before a build overwrites a managed file someone edited since the + * last build. The build still overwrites it: pluginpack owns its output, but + * the edit should not vanish without a word. + */ +export async function warnOnEditedOverwrites( + plan: PrunePlan, + dryRun?: boolean, +): Promise { + const { artifact, previous } = plan; + const incoming = new Map(); + for (const [file, value] of artifact.files) { + incoming.set(file, contentHash(value)); + } + await warnOnEditedFiles( + artifact.outDir, + artifact.target, + artifact.managedPaths, + previous, + dryRun ? "a real build would overwrite it" : "overwriting it", + incoming, + ); +} + /** Deletes every file the previous build managed for a target, including its manifest. */ export async function cleanManagedFiles( outDir: string, @@ -123,71 +287,88 @@ export async function cleanManagedFiles( if (!previous) { return { target, outDir, entries }; } - const files = (previous.files ?? []).map(normalizeManagedPath); - if (!options.dryRun) { - assertNoProtectedDeletions(outDir, files, options.guard, "clean"); - } + const files = previous.files.map(normalizeManagedPath); + await assertNoProtectedDeletions(outDir, files, options.guard, "clean"); + await warnOnEditedFiles( + outDir, + target, + files, + previous, + options.dryRun ? "a real run would delete it" : "deleting it", + ); for (const normalized of files) { entries.push({ type: "deleted", target, path: normalized }); if (!options.dryRun) { - await removeManagedPath(outDir, normalized); + await removeManagedPath(outDir, normalized, target); } } const manifestPath = managedManifestPath(target); entries.push({ type: "deleted", target, path: manifestPath }); if (!options.dryRun) { - await removeManagedPath(outDir, manifestPath); + await removeManagedPath(outDir, manifestPath, target); } return { target, outDir, entries }; } /** - * 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 would refuse to prune generated files. Under the - * default, protect only the directories that really are source plugins. + * Prints a warning for each of `files` whose on-disk content no longer + * matches the hash the manifest recorded when pluginpack wrote it. Files with + * no recorded hash (a version 1 manifest) and files already gone are skipped, + * as are files whose edit already matches what `replacements` will write. + */ +async function warnOnEditedFiles( + outDir: string, + target: TargetName, + files: string[], + previous: ManagedManifest | null, + action: string, + replacements?: Map, +): Promise { + if (!previous) { + return; + } + for (const file of files) { + const recorded = previous.sha256[file]; + if (recorded === undefined) { + continue; + } + let contents: Buffer; + try { + contents = await fs.readFile(path.resolve(outDir, file)); + } catch { + continue; + } + const actual = contentHash(contents); + if (actual !== recorded && actual !== replacements?.get(file)) { + console.warn( + `warning: ${target} managed file ${path.join(outDir, file)} was edited after pluginpack wrote it; ${action}. ` + + `Keep the edit by moving it into the source, or recover it from version control.`, + ); + } + } +} + +/** + * Builds the guard that stops prune/clean from deleting the config, a source + * tree, or an output repository's `.git` directory. * - * 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. + * Takes the loaded config rather than plugin discovery results, so `clean` + * keeps working when the source tree no longer loads. See `listSourcePaths` + * for which authored paths count as source. */ export async function buildDeleteGuard( project: ResolvedProjectConfig, force?: boolean, ): Promise { - const { rootDir, config } = project; - const protectedRoots: string[] = []; - if (config.source?.skills) { - protectedRoots.push(path.resolve(rootDir, config.source.skills)); - } - 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"))), - ); - } - for (const target of Object.values(config.targets)) { - if (!target) { - continue; - } - if (target.repositoryFiles) { - protectedRoots.push(path.resolve(rootDir, target.repositoryFiles)); - } - for (const plugin of Object.values(target.plugins)) { - if (plugin.source) { - protectedRoots.push(path.resolve(rootDir, plugin.source)); - } - if (plugin.overrides) { - protectedRoots.push(path.resolve(rootDir, plugin.overrides)); - } + const protectedRoots = (await listSourcePaths(project)).map( + (source) => source.path, + ); + // An outDir can sit outside the source repo (a sibling output repo), where + // none of the source paths above apply. Its version-control data is the one + // thing there no manifest should ever name. + for (const target of Object.values(project.config.targets)) { + if (target) { + protectedRoots.push(path.resolve(project.rootDir, target.outDir, ".git")); } } return { @@ -197,20 +378,29 @@ export async function buildDeleteGuard( }; } -function assertNoProtectedDeletions( +async function assertNoProtectedDeletions( outDir: string, paths: string[], guard: DeleteGuard | undefined, command: string, -): void { - if (!guard || guard.force) { +): Promise { + if (!guard || guard.force || paths.length === 0) { return; } - const blocked = paths - .map((file) => ({ file, root: protectingRoot(outDir, file, guard) })) - .filter( - (entry): entry is { file: string; root: string } => entry.root !== null, - ); + const roots = [ + ...(guard.configPath ? [guard.configPath] : []), + ...guard.protectedRoots, + ]; + const rootKeys = await Promise.all( + roots.map(async (root) => ({ root, key: await physicalKey(root) })), + ); + const blocked: Array<{ file: string; root: string }> = []; + for (const file of paths) { + const root = await protectingRoot(outDir, file, rootKeys); + if (root !== null) { + blocked.push({ file, root }); + } + } if (blocked.length === 0) { return; } @@ -228,21 +418,25 @@ function assertNoProtectedDeletions( * name what it collided with — "resolves inside " is actionable in a way * that a bare list of refused paths is not. * - * Containment is compared with case and Unicode normalization folded, since the - * filesystem may treat paths differing only in case as the same file. + * Two comparisons, either of which refuses. The lexical one folds case and + * Unicode normalization on every host, so a manifest entry differing from a + * root only by case is refused even where the filesystem would not alias them. + * The physical one compares the entry `fs.rm` would remove against each root + * with symlinks resolved, so an outDir that reaches source through a symlink + * or a case variant is refused too. */ -function protectingRoot( +async function protectingRoot( outDir: string, relativePath: string, - guard: DeleteGuard, -): string | null { + roots: Array<{ root: string; key: string }>, +): Promise { const absolute = path.resolve(outDir, normalizeManagedPath(relativePath)); - if (guard.configPath && isInsideFolded(guard.configPath, absolute)) { - return guard.configPath; + const lexical = roots.find(({ root }) => isInsideFolded(root, absolute)); + if (lexical) { + return lexical.root; } - return ( - guard.protectedRoots.find((root) => isInsideFolded(root, absolute)) ?? null - ); + const key = await physicalKey(absolute, { entry: true }); + return roots.find((root) => isInside(root.key, key))?.root ?? null; } /** Normalizes a managed path to a safe, relative, forward-slash form, throwing if it escapes the output dir. */ @@ -268,6 +462,7 @@ export function normalizeManagedPath(value: string): string { async function removeManagedPath( outDir: string, relativePath: string, + target: TargetName, ): Promise { const root = path.resolve(outDir); const normalized = normalizeManagedPath(relativePath); @@ -285,11 +480,11 @@ async function removeManagedPath( throw error; } if (stats.isSymbolicLink()) { - const target = path.resolve( + const linkTarget = path.resolve( path.dirname(destination), await fs.readlink(destination), ); - if (!isInside(root, target)) { + if (!isInside(root, linkTarget)) { throw new Error( `Refusing to remove a symlink pointing outside the output directory: ${relativePath}`, ); @@ -307,9 +502,11 @@ async function removeManagedPath( try { await fs.rm(destination, { force: true }); } catch (error) { + const manifest = path.join(root, managedManifestPath(target)); 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.`, + `The managed manifest ${manifest} still lists every remaining path, so fix the cause and re-run the same command. ` + + `To stop pluginpack managing these files instead, delete that manifest.`, { cause: error }, ); } diff --git a/src/schema.ts b/src/schema.ts index fd2861e..9c78b4f 100644 --- a/src/schema.ts +++ b/src/schema.ts @@ -162,6 +162,10 @@ export const CLIENT_ONLY_CONTENT_KINDS = [ /** One target's output configuration: where it's written, and which plugins it emits. */ const targetSchema = z.object({ + // Deliberately not safeRelativePath: an outDir may be a sibling output + // repository ("../cursor-plugins"). The delete guard protects this + // project's source and the outDir's .git; it knows nothing else about an + // external repo, which the README calls out. outDir: z.string().min(1), // codex only: "agent-plugins" (default) emits Agent Plugins packages; // "legacy" emits the .codex-plugin layout. See docs/adr/0001. diff --git a/tests/destructive.test.ts b/tests/destructive.test.ts index d5e2002..9196541 100644 --- a/tests/destructive.test.ts +++ b/tests/destructive.test.ts @@ -192,7 +192,12 @@ describe("writeArtifact refuses to write outside its output directory", () => { describe("readManagedManifest rejects a manifest it cannot trust", () => { const invalid: Array<[string, unknown]> = [ - ["a wrong version", { version: 2, target: "cursor", files: [] }], + ["an unknown older version", { version: 0, target: "cursor", files: [] }], + ["a non-numeric version", { version: "2", target: "cursor", files: [] }], + [ + "a version 2 manifest with no hashes", + { version: 2, target: "cursor", files: [] }, + ], ["a mismatched target", { version: 1, target: "claude", files: [] }], [ "a non-array files field", @@ -274,7 +279,10 @@ describe("the delete guard protects a source tree from clean, not just prune", ( await access(path.join(root, "pluginpack.config.ts")); }); - it("refuses to clean the default source.plugins root even when unset in config", async () => { + it("refuses to clean a legacy source plugin a target reads through from, even with source.plugins unset", async () => { + // Under the default plugins/ root only the source plugins a target reads + // are protected, not every directory there: generated output lives there + // too in the README layouts. const created = await makeProject(` cursor: { outDir: ".", plugins: { demo: { from: ["demo"], path: "out/demo", components: ["skills"] } } diff --git a/tests/physical-path-safety.test.ts b/tests/physical-path-safety.test.ts index 8bb49e7..c22183b 100644 --- a/tests/physical-path-safety.test.ts +++ b/tests/physical-path-safety.test.ts @@ -1,14 +1,28 @@ -import { rm, writeFile } from "node:fs/promises"; +import { existsSync } from "node:fs"; +import { + access, + mkdir, + readFile, + rm, + symlink, + writeFile, +} from "node:fs/promises"; import path from "node:path"; -import { Project } from "fixturify-project"; -import { afterEach, describe, expect, it } from "vitest"; +import { fileURLToPath } from "node:url"; +import { Project, type ProjectArgs } from "fixturify-project"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { build } from "../src/build.js"; +import { clean, prune } from "../src/cleanup.js"; import { loadConfig } from "../src/config.js"; -import { walkFiles } from "../src/fs.js"; +import { exists, json, walkFiles } from "../src/fs.js"; +import { managedManifestPath, readManagedManifest } from "../src/managed.js"; + +type DirJSON = NonNullable; let project: Project | undefined; afterEach(async () => { + vi.restoreAllMocks(); await project?.dispose(); project = undefined; }); @@ -16,6 +30,11 @@ afterEach(async () => { const skill = (name: string) => `---\nname: ${name}\ndescription: ${name} skill.\n---\n\n${name} body.\n`; +/** + * A project with one canonical source plugin at `shared/acme` holding the + * skills `alpha` and `beta`. `targetsBlock` and `sourceBlock` are spliced into + * the config verbatim. + */ function configText(targetsBlock: string, sourceBlock = ""): string { return `import { defineConfig } from "${path.resolve("src/index.ts")}"; @@ -31,13 +50,9 @@ ${targetsBlock} `; } -/** - * A project with one canonical source plugin at `shared/acme` holding the - * skills `alpha` and `beta`. `targetsBlock` and `sourceBlock` are spliced into - * the config verbatim. - */ async function makeProject( targetsBlock: string, + extra: DirJSON = {}, sourceBlock = "", ): Promise { project = new Project("pluginpack-physical", "1.0.0", { @@ -52,12 +67,653 @@ async function makeProject( }, }, "UNMANAGED.txt": "USER FILE\n", + ...extra, }, }); await project.write(); return project.baseDir; } +/** Rewrites a target's managed manifest as an older (version 1) pluginpack would. */ +async function writeManifest( + root: string, + outDir: string, + target: string, + files: string[], +): Promise { + const manifestPath = path.join( + root, + outDir, + managedManifestPath(target as never), + ); + await mkdir(path.dirname(manifestPath), { recursive: true }); + await writeFile(manifestPath, json({ version: 1, target, files })); +} + +/** Messages passed to console.warn while `fn` runs. */ +async function captureWarnings(fn: () => Promise): Promise { + const warnings: string[] = []; + vi.spyOn(console, "warn").mockImplementation((message: unknown) => { + warnings.push(String(message)); + }); + await fn(); + return warnings; +} + +const caseInsensitiveFs = existsSync( + path.join( + path.dirname(fileURLToPath(import.meta.url)), + "PHYSICAL-PATH-SAFETY.TEST.TS", + ), +); + +const sourceSurvives = async (root: string) => { + await access(path.join(root, "shared/acme/skills/alpha/SKILL.md")); + await access(path.join(root, "shared/acme/skills/beta/SKILL.md")); + await access(path.join(root, "pluginpack.config.ts")); + await access(path.join(root, "UNMANAGED.txt")); +}; + +describe("the delete guard protects source, not generated packages under plugins/", () => { + it("keeps outDir '.' with the default plugins/ package path operable through a full lifecycle", async () => { + // The agent-plugins default writes the package to plugins/acme/, which + // has a skills/ directory, so a guard that guesses source plugins from + // component directories under plugins/ refuses to prune its own output. + const root = await makeProject(` "agent-plugins": { + outDir: ".", + plugins: { acme: { source: "shared/acme" } } + }`); + await build({ cwd: root }); + await access(path.join(root, "plugins/acme/skills/beta/SKILL.md")); + await rm(path.join(root, "shared/acme/skills/beta"), { recursive: true }); + + await expect(build({ cwd: root })).resolves.toBeDefined(); + await expect( + exists(path.join(root, "plugins/acme/skills/beta/SKILL.md")), + ).resolves.toBe(false); + await expect(prune({ cwd: root })).resolves.toBeDefined(); + await expect(clean({ cwd: root })).resolves.toBeDefined(); + + await expect(exists(path.join(root, "plugins/acme"))).resolves.toBe(false); + await access(path.join(root, "shared/acme/skills/alpha/SKILL.md")); + await access(path.join(root, "UNMANAGED.txt")); + }); + + it("still protects a plugins/ directory that carries plugin.pluginpack.json", async () => { + const root = await makeProject( + ` "agent-plugins": { + outDir: ".", + plugins: { acme: { source: "shared/acme", path: "out/acme" } } + }`, + { + plugins: { + authored: { + "plugin.pluginpack.json": "{}\n", + skills: { gamma: { "SKILL.md": skill("gamma") } }, + }, + }, + }, + ); + await build({ cwd: root }); + await writeManifest(root, ".", "agent-plugins", [ + "plugins/authored/skills/gamma/SKILL.md", + ]); + + await expect(clean({ cwd: root })).rejects.toThrow( + /Refusing to clean 1 path\(s\)[\s\S]*resolves inside .*plugins\/authored/, + ); + await access(path.join(root, "plugins/authored/skills/gamma/SKILL.md")); + }); +}); + +describe("source protection compares physical paths", () => { + async function aliasedSource(): Promise { + const root = await makeProject(` "agent-plugins": { + outDir: "linked-out", + plugins: { acme: { source: "shared/acme", path: "." } } + }`); + await symlink( + path.join(root, "shared/acme"), + path.join(root, "linked-out"), + ); + return root; + } + + it("refuses to build when the outDir is a symlink into a source plugin", async () => { + const root = await aliasedSource(); + + await expect(build({ cwd: root })).rejects.toThrow( + /targets\.agent-plugins\.outDir .* resolves inside .*shared\/acme \(targets\.agent-plugins\.plugins\.acme\.source\)/, + ); + await sourceSurvives(root); + }); + + it("refuses to clean managed paths that reach source through a symlinked outDir", async () => { + // clean never loads the source tree, so this is the delete guard on its own. + const root = await aliasedSource(); + await writeManifest(root, "linked-out", "agent-plugins", [ + "skills/alpha/SKILL.md", + "skills/beta/SKILL.md", + ]); + + await expect(clean({ cwd: root })).rejects.toThrow( + /Refusing to clean 2 path\(s\)[\s\S]*skills\/alpha\/SKILL\.md -> resolves inside .*shared\/acme/, + ); + await sourceSurvives(root); + }); + + it("refuses to load a config whose outDir sits inside a source plugin", async () => { + const root = await makeProject(` antigravity: { + outDir: "shared/acme", + plugins: { acme: { source: "shared/acme" } } + }`); + + await expect(loadConfig(root)).rejects.toThrow( + /targets\.antigravity\.outDir .* resolves inside .*shared\/acme \(targets\.antigravity\.plugins\.acme\.source\)[\s\S]*Point each outDir at a directory outside/, + ); + }); + + it("refuses a --out-dir override inside source", async () => { + const root = await makeProject(` antigravity: { + outDir: "dist/antigravity", + plugins: { acme: { source: "shared/acme" } } + }`); + + await expect( + build({ cwd: root, target: "antigravity", outDir: "shared/acme/out" }), + ).rejects.toThrow(/resolves inside .*shared\/acme/); + await expect(exists(path.join(root, "shared/acme/out"))).resolves.toBe( + false, + ); + }); + + it("still builds and cleans through an outDir symlink that points somewhere safe", async () => { + const root = await makeProject(` antigravity: { + outDir: "linked-out", + plugins: { acme: { source: "shared/acme" } } + }`); + await mkdir(path.join(root, "real-out")); + await symlink(path.join(root, "real-out"), path.join(root, "linked-out")); + + await build({ cwd: root }); + await access(path.join(root, "real-out/acme/skills/alpha/SKILL.md")); + await clean({ cwd: root }); + await sourceSurvives(root); + }); +}); + +describe("cross-target collision checks compare physical paths", () => { + const codexAndAgentPlugins = ( + codexOut: string, + apOut: string, + ) => ` codex: { + outDir: "${codexOut}", + plugins: { acme: { source: "shared/acme" } } + }, + "agent-plugins": { + outDir: "${apOut}", + clients: { codex: {} }, + plugins: { acme: { source: "shared/acme" } } + }`; + + it("rejects two outDirs that alias one directory through a symlink", async () => { + const root = await makeProject(codexAndAgentPlugins("out", "alias-out")); + await mkdir(path.join(root, "out")); + await symlink(path.join(root, "out"), path.join(root, "alias-out")); + + await expect(build({ cwd: root })).rejects.toThrow( + /Targets write overlapping output paths[\s\S]*codex and agent-plugins: .*out\/\.agents\/plugins\/marketplace\.json \(same file as .*alias-out\/\.agents\/plugins\/marketplace\.json\)/, + ); + await expect(exists(path.join(root, "out/plugins"))).resolves.toBe(false); + }); + + it.skipIf(!caseInsensitiveFs)( + "rejects outDirs that differ only by case on a case-insensitive filesystem", + async () => { + const root = await makeProject(codexAndAgentPlugins("out", "OUT")); + + await expect(build({ cwd: root })).rejects.toThrow( + /Targets write overlapping output paths[\s\S]*codex and agent-plugins/, + ); + }, + ); + + it.skipIf(!caseInsensitiveFs)( + "rejects a --target build whose outDir aliases another target's built output by case", + async () => { + const root = await makeProject(codexAndAgentPlugins("out", "OUT")); + await build({ cwd: root, target: "codex" }); + + await expect( + build({ cwd: root, target: "agent-plugins" }), + ).rejects.toThrow( + /overlaps a previously built target's managed files[\s\S]*codex \(previously built\) and agent-plugins/, + ); + }, + ); + + it("refuses to clean one target's manifest entries that alias another's through a symlink", async () => { + const root = await makeProject(codexAndAgentPlugins("out", "alias-out")); + await mkdir(path.join(root, "out")); + await symlink(path.join(root, "out"), path.join(root, "alias-out")); + await mkdir(path.join(root, "out/plugins/acme"), { recursive: true }); + await writeFile(path.join(root, "out/plugins/acme/plugin.json"), "{}\n"); + // Two manifests an older pluginpack could have left behind. + await writeManifest(root, "out", "codex", ["plugins/acme/plugin.json"]); + await writeManifest(root, "alias-out", "agent-plugins", [ + "plugins/acme/plugin.json", + ]); + + await expect(clean({ cwd: root, target: "codex" })).rejects.toThrow( + /Refusing to clean paths claimed by more than one target's managed manifest:[\s\S]*codex and agent-plugins/, + ); + await access(path.join(root, "out/plugins/acme/plugin.json")); + }); +}); + +describe("a stale path another target now owns is left in place", () => { + const entry = `entry: { policy: { installation: "AVAILABLE", authentication: "ON_INSTALL" }, category: "Productivity" }`; + const agentPluginsWithCodex = ` "agent-plugins": { + outDir: "out", + clients: { codex: {} }, + plugins: { acme: { source: "shared/acme", clients: { codex: { ${entry} } } } } + }`; + const codexTarget = ` codex: { + outDir: "out", + plugins: { other: { source: "shared/acme", ${entry} } } + }`; + const agentPluginsWithoutCodex = ` "agent-plugins": { + outDir: "out", + plugins: { acme: { source: "shared/acme" } } + }`; + const marketplace = "out/.agents/plugins/marketplace.json"; + + /** + * Writes a later revision of the project config under its own name. A new + * file per revision, because the config loader caches modules by path. + */ + async function revision( + root: string, + name: string, + targets: string, + ): Promise { + const configPath = path.join(root, `${name}.config.ts`); + await writeFile(configPath, configText(targets)); + return configPath; + } + + it("does not delete a marketplace handed from agent-plugins to codex in one plain build", async () => { + const root = await makeProject(agentPluginsWithCodex); + await build({ cwd: root }); + const configPath = await revision( + root, + "handoff", + `${codexTarget},\n${agentPluginsWithoutCodex}`, + ); + + await build({ cwd: root, configPath }); + + const written = JSON.parse( + await readFile(path.join(root, marketplace), "utf8"), + ) as { plugins: Array<{ name: string }> }; + expect(written.plugins.map((plugin) => plugin.name)).toEqual(["other"]); + expect( + (await readManagedManifest(path.join(root, "out"), "agent-plugins")) + ?.files, + ).not.toContain(".agents/plugins/marketplace.json"); + }); + + it("does not prune a stale path another configured target's manifest claims", async () => { + const root = await makeProject(agentPluginsWithCodex); + await build({ cwd: root }); + // The project swaps targets; agent-plugins' old manifest stays on disk. + await build({ + cwd: root, + configPath: await revision(root, "codex-only", codexTarget), + }); + const configPath = await revision( + root, + "both", + `${codexTarget},\n${agentPluginsWithoutCodex}`, + ); + + const [result] = await prune({ + cwd: root, + configPath, + target: "agent-plugins", + }); + + expect(result.entries.map((entry) => entry.path)).not.toContain( + ".agents/plugins/marketplace.json", + ); + await access(path.join(root, marketplace)); + }); +}); + +describe("dry runs run the same destructive preflight as the real command", () => { + async function protectedStalePath(): Promise { + const root = await makeProject( + ` antigravity: { + outDir: ".", + plugins: { acme: { from: ["core"], path: "generated" } } + }`, + { skills: { gamma: { "SKILL.md": skill("gamma") } } }, + `source: { skills: "skills", rootPlugin: { id: "core" } },`, + ); + await build({ cwd: root }); + const manifest = await readManagedManifest(root, "antigravity"); + await writeManifest(root, ".", "antigravity", [ + ...(manifest?.files ?? []), + "skills/gamma/SKILL.md", + ]); + return root; + } + + it("clean --dry-run fails on a protected path, as clean does", async () => { + const root = await protectedStalePath(); + + await expect(clean({ cwd: root, dryRun: true })).rejects.toThrow( + /Refusing to clean 1 path\(s\)/, + ); + await expect(clean({ cwd: root })).rejects.toThrow( + /Refusing to clean 1 path\(s\)/, + ); + }); + + it("prune --dry-run fails on a protected path, as prune does", async () => { + const root = await protectedStalePath(); + + await expect(prune({ cwd: root, dryRun: true })).rejects.toThrow( + /Refusing to prune 1 path\(s\)/, + ); + await expect(prune({ cwd: root })).rejects.toThrow( + /Refusing to prune 1 path\(s\)/, + ); + await access(path.join(root, "skills/gamma/SKILL.md")); + }); + + it("build --dry-run fails where build would, and build refuses before writing", async () => { + const root = await protectedStalePath(); + await rm(path.join(root, "generated"), { recursive: true }); + + await expect(build({ cwd: root, dryRun: true })).rejects.toThrow( + /Refusing to prune 1 path\(s\)/, + ); + await expect(build({ cwd: root })).rejects.toThrow( + /Refusing to prune 1 path\(s\)/, + ); + await expect(exists(path.join(root, "generated"))).resolves.toBe(false); + await access(path.join(root, "skills/gamma/SKILL.md")); + }); +}); + +describe("rootFiles cannot make an authored file a managed output", () => { + const selfCopy = ` antigravity: { + outDir: ".", + rootFiles: { "README.md": "README.md" }, + plugins: { acme: { source: "shared/acme", path: "generated/acme" } } + }`; + + it("rejects a rootFiles entry that copies a file onto itself, before writing", async () => { + const root = await makeProject(selfCopy, { + "README.md": "HAND-WRITTEN README\n", + }); + + await expect(build({ cwd: root })).rejects.toThrow( + /targets\.antigravity\.rootFiles\["README\.md"\] copies .*README\.md onto itself/, + ); + expect(await readFile(path.join(root, "README.md"), "utf8")).toBe( + "HAND-WRITTEN README\n", + ); + await expect(exists(path.join(root, "generated"))).resolves.toBe(false); + }); + + it("refuses to clean a rootFiles source an older build left in the manifest", async () => { + const root = await makeProject(selfCopy, { + "README.md": "HAND-WRITTEN README\n", + }); + await writeManifest(root, ".", "antigravity", ["README.md"]); + + await expect(clean({ cwd: root })).rejects.toThrow( + /README\.md -> resolves inside .*README\.md/, + ); + await access(path.join(root, "README.md")); + }); + + it("rejects a repositoryFiles directory that is the target's own outDir", async () => { + const root = await makeProject( + ` antigravity: { + outDir: "repo-root", + repositoryFiles: "repo-root", + plugins: { acme: { source: "shared/acme" } } + }`, + { "repo-root": { "README.md": "HAND-WRITTEN README\n" } }, + ); + + // Caught at load, since the outDir is a protected repositoryFiles path. + await expect(build({ cwd: root })).rejects.toThrow( + /targets\.antigravity\.outDir .* resolves inside .*repo-root \(targets\.antigravity\.repositoryFiles\)/, + ); + await access(path.join(root, "repo-root/README.md")); + }); +}); + +describe("the managed manifest records hashes and warns before losing an edit", () => { + const target = ` antigravity: { + outDir: "out", + plugins: { acme: { source: "shared/acme" } } + }`; + + it("writes version 2 with a sha256 for every managed file", async () => { + const root = await makeProject(target); + await build({ cwd: root }); + + const raw = JSON.parse( + await readFile( + path.join(root, "out", managedManifestPath("antigravity")), + "utf8", + ), + ) as { version: number; files: string[]; sha256: Record }; + + expect(raw.version).toBe(2); + expect(Object.keys(raw.sha256).sort()).toEqual([...raw.files].sort()); + expect(raw.sha256["acme/skills/alpha/SKILL.md"]).toMatch(/^[0-9a-f]{64}$/); + }); + + it("reads a version 1 manifest and upgrades it on the next build", async () => { + const root = await makeProject(target); + await build({ cwd: root }); + await writeManifest(root, "out", "antigravity", [ + "acme/skills/alpha/SKILL.md", + ]); + + await expect( + readManagedManifest(path.join(root, "out"), "antigravity"), + ).resolves.toMatchObject({ version: 2, sha256: {} }); + await build({ cwd: root }); + await expect( + readManagedManifest(path.join(root, "out"), "antigravity"), + ).resolves.toMatchObject({ + sha256: { "acme/skills/alpha/SKILL.md": expect.any(String) }, + }); + }); + + it("warns, then still prunes, a stale managed file someone edited", async () => { + const root = await makeProject(target); + await build({ cwd: root }); + const generated = path.join(root, "out/acme/skills/beta/SKILL.md"); + await writeFile(generated, `${skill("beta")}HOTFIX\n`); + await rm(path.join(root, "shared/acme/skills/beta"), { recursive: true }); + + const warnings = await captureWarnings(() => build({ cwd: root })); + + expect(warnings).toEqual([ + expect.stringMatching( + /antigravity managed file .*out\/acme\/skills\/beta\/SKILL\.md was edited after pluginpack wrote it; deleting it/, + ), + ]); + await expect(exists(generated)).resolves.toBe(false); + }); + + it("reports an edited stale file from prune --dry-run without deleting it", async () => { + const root = await makeProject(target); + await build({ cwd: root }); + const generated = path.join(root, "out/acme/skills/beta/SKILL.md"); + await writeFile(generated, "EDITED\n"); + await rm(path.join(root, "shared/acme/skills/beta"), { recursive: true }); + + const warnings = await captureWarnings(() => + prune({ cwd: root, dryRun: true }), + ); + + expect(warnings).toEqual([ + expect.stringMatching( + /was edited after pluginpack wrote it; a real run would delete it/, + ), + ]); + await access(generated); + }); + + it("warns before a build overwrites an edited managed file, but not an unedited one", async () => { + const root = await makeProject(target); + await build({ cwd: root }); + await writeFile( + path.join(root, "out/acme/skills/alpha/SKILL.md"), + "EDITED\n", + ); + + const warnings = await captureWarnings(() => build({ cwd: root })); + + expect(warnings).toEqual([ + expect.stringMatching( + /acme\/skills\/alpha\/SKILL\.md was edited after pluginpack wrote it; overwriting it/, + ), + ]); + expect( + await readFile(path.join(root, "out/acme/skills/alpha/SKILL.md"), "utf8"), + ).toBe(skill("alpha")); + }); + + it("warns before clean deletes an edited managed file", async () => { + const root = await makeProject(target); + await build({ cwd: root }); + await writeFile( + path.join(root, "out/acme/skills/alpha/SKILL.md"), + "EDITED\n", + ); + + const warnings = await captureWarnings(() => clean({ cwd: root })); + + expect(warnings).toEqual([ + expect.stringMatching( + /alpha\/SKILL\.md was edited after pluginpack wrote it; deleting it/, + ), + ]); + }); + + it("tells a manifest from a newer pluginpack apart from a corrupt one", async () => { + const root = await makeProject(target); + const outDir = path.join(root, "out"); + const manifestPath = path.join(outDir, managedManifestPath("antigravity")); + await mkdir(path.dirname(manifestPath), { recursive: true }); + + await writeFile( + manifestPath, + json({ version: 3, target: "antigravity", files: [] }), + ); + await expect(readManagedManifest(outDir, "antigravity")).rejects.toThrow( + `Managed manifest ${manifestPath} has version 3, so a newer pluginpack wrote it. This pluginpack reads versions 1 to 2. Upgrade pluginpack`, + ); + + await writeFile(manifestPath, "{ truncated"); + await expect(readManagedManifest(outDir, "antigravity")).rejects.toThrow( + `Invalid managed manifest: ${manifestPath} (not valid JSON)`, + ); + }); +}); + +describe("managed-manifest writes stay inside the output directory", () => { + it("refuses to build through a symlinked .pluginpack directory", async () => { + const root = await makeProject(` antigravity: { + outDir: "out", + plugins: { acme: { source: "shared/acme" } } + }`); + const outside = path.join(root, "outside-state"); + await mkdir(outside); + const state = json({ + version: 1, + target: "antigravity", + files: [], + note: "USER STATE", + }); + await writeFile(path.join(outside, "antigravity.json"), state); + await mkdir(path.join(root, "out")); + await symlink(outside, path.join(root, "out/.pluginpack")); + + await expect(build({ cwd: root })).rejects.toThrow( + /Refusing to write through a symlink that leaves the output directory: \.pluginpack\/antigravity\.json/, + ); + expect(await readFile(path.join(outside, "antigravity.json"), "utf8")).toBe( + state, + ); + await expect(exists(path.join(root, "out/acme"))).resolves.toBe(false); + }); +}); + +describe("a failed clean says how to recover", () => { + it("names the managed manifest and the recovery step", async () => { + const root = await makeProject(` antigravity: { + outDir: "out", + plugins: { acme: { source: "shared/acme" } } + }`); + await build({ cwd: root }); + const manifest = await readManagedManifest( + path.join(root, "out"), + "antigravity", + ); + await mkdir(path.join(root, "out/blocked")); + await writeManifest(root, "out", "antigravity", [ + ...(manifest?.files ?? []), + "blocked", + ]); + + const manifestPath = path.join( + root, + "out", + managedManifestPath("antigravity"), + ); + await expect(clean({ cwd: root })).rejects.toThrow( + `The managed manifest ${manifestPath} still lists every remaining path, so fix the cause and re-run the same command.`, + ); + await access(manifestPath); + }); +}); + +describe("an external outDir keeps its version-control data", () => { + it("refuses to clean a manifest entry inside the outDir's .git", async () => { + const root = await makeProject( + ` antigravity: { + outDir: "../sibling", + plugins: { acme: { source: "shared/acme" } } + }`, + ); + const sibling = path.join(root, "../sibling"); + await mkdir(path.join(sibling, ".git"), { recursive: true }); + await writeFile(path.join(sibling, ".git/config"), "[core]\n"); + try { + await build({ cwd: root }); + await writeManifest(root, "../sibling", "antigravity", [".git/config"]); + + await expect(clean({ cwd: root })).rejects.toThrow( + /\.git\/config -> resolves inside .*sibling\/\.git/, + ); + await access(path.join(sibling, ".git/config")); + } finally { + await rm(sibling, { recursive: true, force: true }); + } + }); +}); + describe("config path fields stay inside the project", () => { it("rejects source.partials outside the project at config load", async () => { const root = await makeProject( @@ -65,6 +721,7 @@ describe("config path fields stay inside the project", () => { outDir: "out", plugins: { acme: { source: "shared/acme" } } }`, + {}, `source: { partials: "../outside" },`, ); From e2154f36a26dd6cd372c4da1af958f85c2e1f739 Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Wed, 21 Oct 2026 15:23:44 -0700 Subject: [PATCH 04/13] fix(config): reject URL-shaped traversal in config path fields The config schema checked path fields with the same helper that marketplace entry sources use, and that helper accepts any value starting with http:// or https://. Every schema field it guards is a filesystem path, though, and path.resolve reads "https://../../outside" as "../outside". So source.partials, a plugin source, or overrides could still read files from outside the project and ship them. Schema path fields now use a filesystem-only check that gives URLs no exemption. --- src/fs.ts | 27 ++++++++++++++++++++++++--- src/schema.ts | 8 +++++--- tests/physical-path-safety.test.ts | 29 +++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 6 deletions(-) diff --git a/src/fs.ts b/src/fs.ts index df5b273..99c4db6 100644 --- a/src/fs.ts +++ b/src/fs.ts @@ -236,7 +236,12 @@ export function json(value: unknown): string { return `${JSON.stringify(value, null, 2)}\n`; } -/** Whether `value` is a URL, or a relative path that can't escape its base directory. */ +/** + * Whether `value` is a URL, or a relative path that can't escape its base + * directory. For a value that may be either, such as a marketplace entry's + * `source`. A value that is only ever read from or written to disk must use + * `isSafeRelativeFilePath` instead. + */ export function isSafeRelativePath(value: string): boolean { if (!value) { return false; @@ -244,11 +249,27 @@ export function isSafeRelativePath(value: string): boolean { if (value.startsWith("http://") || value.startsWith("https://")) { return true; } - if (path.isAbsolute(value)) { + return isSafeRelativeFilePath(value); +} + +/** + * Whether `value` is a relative filesystem path that can't escape its base + * directory. Unlike `isSafeRelativePath`, a URL gets no exemption: passed to + * `path.resolve`, `https://../../outside` is just a path with a `..` in it, + * and lands outside the base. + */ +export function isSafeRelativeFilePath(value: string): boolean { + if (!value || path.isAbsolute(value)) { return false; } const normalized = path.posix.normalize(value.replace(/\\/g, "/")); - return normalized !== ".." && !normalized.startsWith("../"); + return ( + normalized !== ".." && + !normalized.startsWith("../") && + !path.posix.isAbsolute(normalized) && + // A Windows drive-letter path resolves as absolute on Windows. + !/^[a-zA-Z]:/.test(normalized) + ); } /** Whether the file at `filePath` exists. */ diff --git a/src/schema.ts b/src/schema.ts index 9c78b4f..e6625aa 100644 --- a/src/schema.ts +++ b/src/schema.ts @@ -1,14 +1,16 @@ import { z } from "zod"; -import { isSafeRelativePath } from "./fs.js"; +import { isSafeRelativeFilePath } from "./fs.js"; /** * A path written to or read from disk relative to a root — rejects absolute - * paths and `..` escapes so a config can't write or read outside it. + * paths and `..` escapes so a config can't write or read outside it. Every + * field using it is a filesystem path, so a URL-shaped value gets no + * exemption: `path.resolve` would read `https://../x` as `../x`. */ const safeRelativePath = z .string() .refine( - isSafeRelativePath, + isSafeRelativeFilePath, 'must be a safe relative path (no absolute paths or ".." segments)', ); diff --git a/tests/physical-path-safety.test.ts b/tests/physical-path-safety.test.ts index c22183b..7afe916 100644 --- a/tests/physical-path-safety.test.ts +++ b/tests/physical-path-safety.test.ts @@ -729,6 +729,35 @@ describe("config path fields stay inside the project", () => { /Invalid pluginpack config in .*: source\.partials: must be a safe relative path/, ); }); + + it.each([ + ["source.partials", `source: { partials: "https://../../outside" },`, ""], + [ + "targets.antigravity.plugins.acme.source", + "", + `source: "https://../../outside"`, + ], + ])( + "rejects a URL-shaped %s that resolves outside the project", + async (field, sourceBlock, pluginSource) => { + const root = await makeProject( + ` antigravity: { + outDir: "out", + plugins: { acme: { ${pluginSource || `source: "shared/acme"`} } } + }`, + {}, + sourceBlock, + ); + // path.resolve reads the value as a plain path with ".." segments. + expect(path.resolve(root, "https://../../outside")).toBe( + path.resolve(root, "../outside"), + ); + + await expect(loadConfig(root)).rejects.toThrow( + `${field}: must be a safe relative path`, + ); + }, + ); }); describe("walkFiles distinguishes a missing directory from a file", () => { From bf0913eae9a205d1a19c1c53b606ec2c69c32631 Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Wed, 21 Oct 2026 15:24:51 -0700 Subject: [PATCH 05/13] fix(config): load the config file without jiti's module cache loadProjectConfig imported the config through jiti with its default module cache. A second build() in the same process after pluginpack.config.ts changed on disk built the first config again. The CLI starts a new process per command, but a watcher or test harness calling the programmatic API silently built stale config. The loader now disables the module cache. --- src/config.ts | 4 ++++ tests/physical-path-safety.test.ts | 34 ++++++++++++++++++++++++++---- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/src/config.ts b/src/config.ts index 0117963..cda22f0 100644 --- a/src/config.ts +++ b/src/config.ts @@ -69,8 +69,12 @@ export async function loadProjectConfig( const resolvedConfigPath = configPath ? path.resolve(cwd, configPath) : await findConfig(cwd); + // No module cache: a long-lived process (a watcher, a test harness) that + // calls build() again after the config file changed must get the new + // config, not the module jiti evaluated the first time. const jiti = createJiti(pathToFileURL(resolvedConfigPath).href, { interopDefault: true, + moduleCache: false, }); const loaded = await jiti.import(resolvedConfigPath, { default: true }); const config = parseWithContext( diff --git a/tests/physical-path-safety.test.ts b/tests/physical-path-safety.test.ts index 7afe916..4300318 100644 --- a/tests/physical-path-safety.test.ts +++ b/tests/physical-path-safety.test.ts @@ -328,10 +328,7 @@ describe("a stale path another target now owns is left in place", () => { }`; const marketplace = "out/.agents/plugins/marketplace.json"; - /** - * Writes a later revision of the project config under its own name. A new - * file per revision, because the config loader caches modules by path. - */ + /** Writes a later revision of the project config under its own name. */ async function revision( root: string, name: string, @@ -760,6 +757,35 @@ describe("config path fields stay inside the project", () => { ); }); +describe("the config loader reads the config file fresh each time", () => { + it.each(["pluginpack.config.ts", "pluginpack.config.mjs"])( + "builds the edited %s on a second build() in the same process", + async (name) => { + const antigravity = ` antigravity: { + outDir: "out", + plugins: { acme: { source: "shared/acme" } } + }`; + const root = await makeProject(antigravity, { + "pluginpack.config.ts": null as never, + [name]: configText(antigravity), + }); + expect(existsSync(path.join(root, "pluginpack.config.ts"))).toBe( + name === "pluginpack.config.ts", + ); + const [first] = await build({ cwd: root }); + expect(first.target).toBe("antigravity"); + + await writeFile( + path.join(root, name), + configText(antigravity.replace("antigravity", "cursor")), + ); + const [second] = await build({ cwd: root }); + + expect(second.target).toBe("cursor"); + }, + ); +}); + describe("walkFiles distinguishes a missing directory from a file", () => { it("throws when the directory is a file", async () => { const root = await makeProject(` antigravity: { From d61608486ff9b02a3e80ed1f4eb4e3d1599e6ca7 Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Wed, 21 Oct 2026 15:29:52 -0700 Subject: [PATCH 06/13] fix(fs): probe case sensitivity on the destination volume, once per build physicalKey decided whether to fold case by looking up the case-swapped name of the deepest existing path component in its parent directory. For an outDir on a case-sensitive volume mounted under a case-insensitive parent, that component is the mount point, and its parent answers for the wrong filesystem. Distinct out and OUT directories then got one key and a valid build was refused. The probe also compared inodes, so two hard links whose names differ only by case made a case-sensitive directory look case-insensitive. The probe now asks the volume that holds the nearest existing directory. It looks up names inside that directory, or inside an ancestor on the same device. A directory that lists both spellings is case-sensitive, so hard links no longer fool it. An empty volume gets a short-lived, uniquely named scratch file instead. The platform default remains the last resort. Build and prune plans now run inside a per-plan cache of real paths, nearest directories and per-volume answers. The physical checks had tripled build time on a 3,001-file package. That build now takes about as long as before them (about 1.0s fresh, 0.7s rebuild). --- src/build.ts | 12 +- src/fs.ts | 204 +++++++++++++++++++++++++---- tests/physical-path-safety.test.ts | 119 ++++++++++++++++- 3 files changed, 306 insertions(+), 29 deletions(-) diff --git a/src/build.ts b/src/build.ts index a56fb90..37283ec 100644 --- a/src/build.ts +++ b/src/build.ts @@ -1,6 +1,6 @@ import path from "node:path"; import { assertOutDirsOutsideSource, loadConfig } from "./config.js"; -import { physicalKey, writeArtifact } from "./fs.js"; +import { physicalKey, withPathCache, writeArtifact } from "./fs.js"; import { assertManagedManifestContained, buildDeleteGuard, @@ -58,7 +58,15 @@ export async function build(options: BuildOptions = {}): Promise { * fails exactly where the real command would, and a refused prune never * leaves a half-written build behind. `force` relaxes only the delete guard. */ -export async function planBuild( +export function planBuild( + options: BuildOptions, + force?: boolean, +): Promise<{ artifacts: Artifact[]; prunes: PrunePlan[] }> { + // Nothing in the plan writes, so it can resolve each directory once. + return withPathCache(() => planBuildChecked(options, force)); +} + +async function planBuildChecked( options: BuildOptions, force?: boolean, ): Promise<{ artifacts: Artifact[]; prunes: PrunePlan[] }> { diff --git a/src/fs.ts b/src/fs.ts index 99c4db6..bcf5112 100644 --- a/src/fs.ts +++ b/src/fs.ts @@ -1,3 +1,5 @@ +import { AsyncLocalStorage } from "node:async_hooks"; +import { randomBytes } from "node:crypto"; import { promises as fs } from "node:fs"; import path from "node:path"; import type { FileValue } from "./types.js"; @@ -104,48 +106,198 @@ export async function physicalKey( const parent = path.dirname(absolute); const real = options.entry && parent !== absolute - ? path.join(await realpathDeep(parent), path.basename(absolute)) - : await realpathDeep(absolute); + ? path.join(await cachedRealpathDeep(parent), path.basename(absolute)) + : await cachedRealpathDeep(absolute); return (await isCaseInsensitiveAt(real)) ? fold(real) : real; } -const caseInsensitiveDirs = new Map>(); +/** Lookups `physicalKey` memoizes inside `withPathCache`. */ +type PathCache = { + realpaths: Map>; + directories: Map>; + caseInsensitiveDevices: Map>; +}; + +const pathCache = new AsyncLocalStorage(); + +/** + * Runs `fn` with `physicalKey` memoizing real paths and per-volume case + * sensitivity, so a preflight over thousands of files resolves each + * directory and probes each volume once rather than once per file. + * + * The memo assumes the paths it has seen do not change while `fn` runs. Wrap + * read-only checks only, never code that writes or deletes. Nested calls share + * the outer cache. + */ +export function withPathCache(fn: () => Promise): Promise { + if (pathCache.getStore()) { + return fn(); + } + return pathCache.run( + { + realpaths: new Map(), + directories: new Map(), + caseInsensitiveDevices: new Map(), + }, + fn, + ); +} + +function memoize( + map: Map> | undefined, + key: K, + compute: () => Promise, +): Promise { + if (!map) { + return compute(); + } + let value = map.get(key); + if (!value) { + value = compute(); + map.set(key, value); + } + return value; +} + +/** `realpathDeep`, memoized per path (and so per ancestor) inside `withPathCache`. */ +function cachedRealpathDeep(target: string): Promise { + const absolute = path.resolve(target); + return memoize(pathCache.getStore()?.realpaths, absolute, async () => { + try { + return await fs.realpath(absolute); + } catch (error) { + if (!isNotFoundError(error)) { + throw error; + } + const parent = path.dirname(absolute); + if (parent === absolute) { + return absolute; + } + return path.join( + await cachedRealpathDeep(parent), + path.basename(absolute), + ); + } + }); +} /** - * Whether names under `real`'s volume compare case-insensitively. Probes the - * deepest existing component of `real` that has a cased letter: if the - * case-swapped name is the same inode, the directory holding it folds case. - * Falls back to the platform default (folding on macOS and Windows) when no - * component can be probed, since over-matching is the safe direction for a - * check that decides whether to refuse. + * Whether names on the volume that holds `real` compare case-insensitively. + * `real` may not exist yet; the volume is the one holding its nearest + * existing directory, since that is where the missing tail will be created. */ async function isCaseInsensitiveAt(real: string): Promise { - let current = real; + const nearest = await nearestDirectory(real); + if (!nearest) { + return platformFoldsCase(); + } + return memoize( + pathCache.getStore()?.caseInsensitiveDevices, + nearest.dev, + () => probeVolumeCase(nearest.dir, nearest.dev), + ); +} + +/** The nearest existing directory at or above `real`, and its device. */ +function nearestDirectory( + real: string, +): Promise<{ dir: string; dev: number } | null> { + return memoize(pathCache.getStore()?.directories, real, async () => { + // lstat: `real` is resolved except, for an entry key, its final name, + // which may be a symlink. That name lives in its parent's directory. + const stats = await fs.lstat(real).catch(() => null); + if (stats?.isDirectory()) { + return { dir: real, dev: stats.dev }; + } + const parent = path.dirname(real); + return parent === real ? null : nearestDirectory(parent); + }); +} + +/** + * Probes whether the volume `dir` lives on (device `dev`) folds case. + * + * A name lookup in a directory is answered by the directory's own + * filesystem, so the probe looks up names inside `dir`, or inside an + * ancestor on the same device, never a mount point's name in the parent + * volume. For an existing name with a cased letter: if its case-swapped + * spelling is also listed, the directory holds both and is case-sensitive; + * otherwise, the volume folds case exactly when looking the swapped spelling + * up finds something. (Comparing inodes instead would be fooled by two hard + * links whose names differ only by case.) + * + * When no directory on the volume has a usable name, as on an empty volume + * just mounted, the probe creates and removes one empty, uniquely named file + * in `dir`. Only when that also fails does it fall back to the platform + * default, folding on macOS and Windows: over-matching is the safe direction + * for a check that decides whether to refuse. + */ +async function probeVolumeCase(dir: string, dev: number): Promise { + let current = dir; for (;;) { + const answer = await probeDirectoryEntries(current); + if (answer !== undefined) { + return answer; + } const parent = path.dirname(current); if (parent === current) { - return process.platform === "darwin" || process.platform === "win32"; + break; + } + const parentStats = await fs.stat(parent).catch(() => null); + if (!parentStats || parentStats.dev !== dev) { + break; } - const name = path.basename(current); + current = parent; + } + return (await probeWithScratchFile(dir)) ?? platformFoldsCase(); +} + +async function probeDirectoryEntries( + dir: string, +): Promise { + const names = await fs.readdir(dir).catch(() => null); + if (!names) { + return undefined; + } + const listed = new Set(names); + for (const name of names) { const swapped = swapCase(name); // Skip names whose case swap does not round-trip ("ß" -> "SS"): the // swapped name would be a different name, not a different case. - if (swapped !== name && swapCase(swapped) === name) { - const stats = await fs.lstat(current).catch(() => null); - if (stats) { - let cached = caseInsensitiveDirs.get(parent); - if (!cached) { - cached = fs.lstat(path.join(parent, swapped)).then( - (other) => other.ino === stats.ino && other.dev === stats.dev, - () => false, - ); - caseInsensitiveDirs.set(parent, cached); - } - return cached; - } + if (swapped === name || swapCase(swapped) !== name) { + continue; } - current = parent; + if (listed.has(swapped)) { + return false; + } + return fs.lstat(path.join(dir, swapped)).then( + () => true, + () => false, + ); } + return undefined; +} + +async function probeWithScratchFile(dir: string): Promise { + const name = `.pluginpack-case-probe-${process.pid}-${randomBytes(6).toString("hex")}`; + const probe = path.join(dir, name); + try { + await fs.writeFile(probe, "", { flag: "wx" }); + } catch { + return null; + } + try { + return await fs.lstat(path.join(dir, swapCase(name))).then( + () => true, + () => false, + ); + } finally { + await fs.rm(probe, { force: true }); + } +} + +function platformFoldsCase(): boolean { + return process.platform === "darwin" || process.platform === "win32"; } function swapCase(value: string): string { diff --git a/tests/physical-path-safety.test.ts b/tests/physical-path-safety.test.ts index 4300318..aa847bd 100644 --- a/tests/physical-path-safety.test.ts +++ b/tests/physical-path-safety.test.ts @@ -1,12 +1,16 @@ +import { execFileSync } from "node:child_process"; import { existsSync } from "node:fs"; import { access, + link, mkdir, + mkdtemp, readFile, rm, symlink, writeFile, } from "node:fs/promises"; +import os from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; import { Project, type ProjectArgs } from "fixturify-project"; @@ -14,7 +18,13 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { build } from "../src/build.js"; import { clean, prune } from "../src/cleanup.js"; import { loadConfig } from "../src/config.js"; -import { exists, json, walkFiles } from "../src/fs.js"; +import { + exists, + json, + physicalKey, + walkFiles, + withPathCache, +} from "../src/fs.js"; import { managedManifestPath, readManagedManifest } from "../src/managed.js"; type DirJSON = NonNullable; @@ -107,6 +117,53 @@ const caseInsensitiveFs = existsSync( ), ); +/** + * Mounts an empty case-sensitive APFS volume at a directory under the + * (case-insensitive by default) system temp directory, or returns null where + * that is not possible: not macOS, or `hdiutil` unavailable or refused. + */ +async function mountCaseSensitiveVolume(): Promise { + if (process.platform !== "darwin") { + return null; + } + const base = await mkdtemp(path.join(os.tmpdir(), "pluginpack-casefs-")); + const image = path.join(base, "volume.sparseimage"); + const mountPoint = path.join(base, "mnt"); + try { + await mkdir(mountPoint); + execFileSync("hdiutil", [ + "create", + "-quiet", + "-size", + "16m", + "-fs", + "Case-sensitive APFS", + "-type", + "SPARSE", + "-volname", + "pluginpack-cs", + image, + ]); + execFileSync("hdiutil", [ + "attach", + "-quiet", + "-nobrowse", + "-mountpoint", + mountPoint, + image, + ]); + } catch { + await rm(base, { recursive: true, force: true }); + return null; + } + return mountPoint; +} + +function unmountVolume(mountPoint: string): void { + execFileSync("hdiutil", ["detach", "-quiet", "-force", mountPoint]); + execFileSync("rm", ["-rf", path.dirname(mountPoint)]); +} + const sourceSurvives = async (root: string) => { await access(path.join(root, "shared/acme/skills/alpha/SKILL.md")); await access(path.join(root, "shared/acme/skills/beta/SKILL.md")); @@ -242,6 +299,28 @@ describe("source protection compares physical paths", () => { }); }); +/** + * In a case-sensitive `dir`, two hard links whose names differ only by case + * are one inode: comparing inodes of `name` and `NAME` would conclude the + * directory folds case and alias the distinct files `other` and `OTHER`. + */ +async function expectHardLinksKeepCase(dir: string): Promise { + const links = path.join(dir, "links"); + await mkdir(links); + await writeFile(path.join(links, "name"), "x\n"); + await link(path.join(links, "name"), path.join(links, "NAME")); + await writeFile(path.join(links, "other"), "a\n"); + await writeFile(path.join(links, "OTHER"), "b\n"); + + const keys = await withPathCache(async () => [ + await physicalKey(path.join(links, "name"), { entry: true }), + await physicalKey(path.join(links, "other"), { entry: true }), + await physicalKey(path.join(links, "OTHER"), { entry: true }), + ]); + + expect(new Set(keys).size).toBe(3); +} + describe("cross-target collision checks compare physical paths", () => { const codexAndAgentPlugins = ( codexOut: string, @@ -292,6 +371,44 @@ describe("cross-target collision checks compare physical paths", () => { }, ); + it("builds outDirs that differ only by case on a case-sensitive volume mounted under a case-insensitive parent", async (context) => { + // The volume is the destination filesystem. Probing how its mount point's + // name compares in the parent directory asks the wrong filesystem. + const volume = await mountCaseSensitiveVolume(); + if (!volume) { + context.skip(); + return; + } + try { + const root = await makeProject( + codexAndAgentPlugins( + path.join(volume, "out"), + path.join(volume, "OUT"), + ), + ); + + await build({ cwd: root }); + await access(path.join(volume, "out/plugins/acme/plugin.json")); + await access(path.join(volume, "OUT/plugins/acme/plugin.json")); + await clean({ cwd: root }); + await expect(exists(path.join(volume, "out/plugins"))).resolves.toBe( + false, + ); + await expectHardLinksKeepCase(volume); + } finally { + unmountVolume(volume); + } + }); + + it.skipIf(caseInsensitiveFs)( + "does not read a hard link with a case-swapped name as a case-insensitive directory", + async () => { + const root = await makeProject(codexAndAgentPlugins("out", "OUT")); + await expectHardLinksKeepCase(root); + await build({ cwd: root }); + }, + ); + it("refuses to clean one target's manifest entries that alias another's through a symlink", async () => { const root = await makeProject(codexAndAgentPlugins("out", "alias-out")); await mkdir(path.join(root, "out")); From 1fd883d18e12346cfd07c6f2d872902f343b2953 Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Wed, 21 Oct 2026 15:31:33 -0700 Subject: [PATCH 07/13] fix(build): resolve final symlinks when checking write collisions The cross-target collision check keyed each output path with only its parent directory resolved. That is the right key for deletion, since unlinking a symlink removes the link. A write follows the link, though. A generated SKILL.md that was a symlink to another target's file let both targets build green, and the first target's skill shipped the second target's content. Write collisions now compare fully resolved paths, and ownership of stale paths still compares entries. --- src/build.ts | 48 ++++++++++++++++++++++-------- tests/physical-path-safety.test.ts | 40 +++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 13 deletions(-) diff --git a/src/build.ts b/src/build.ts index 37283ec..01c8d51 100644 --- a/src/build.ts +++ b/src/build.ts @@ -100,7 +100,7 @@ async function planBuildChecked( await assertManagedManifestContained(artifact.outDir, artifact.target); } const owners = new Map(claimedElsewhere); - for (const [key, claim] of incoming) { + for (const [key, claim] of incoming.byEntry) { owners.set(key, claim.target); } const prunes: PrunePlan[] = []; @@ -174,38 +174,58 @@ async function assertNoSelfReferentialRootFiles( /** Which target claims a path, and the path as that target names it. */ type Claim = { target: TargetName; absolute: string }; +/** + * Every path this build writes, keyed two ways. `byWrite` keys the file a + * write lands on, with every symlink resolved, the final entry included: + * writing through a symlinked `SKILL.md` changes the file it points at. Write + * collisions compare these. `byEntry` keys the entry itself (parent resolved, + * final name kept), which is what a prune unlinks; ownership compares these. + */ +type IncomingPaths = { + byWrite: Map; + byEntry: Map; +}; + /** * Two targets pointed at overlapping output paths would silently overwrite each * other (and one target's prune could delete the other's files). Catch it. * Keys on physical paths, so `out` and `OUT` on a case-insensitive filesystem, - * or an outDir reached through a symlink, are one path, as they are on disk. - * Returns the physical-key -> claim map so later checks can reuse it. + * an outDir reached through a symlink, or a generated file that is itself a + * symlink to another target's file, are one path, as they are on disk. + * Returns the keyed claims so later checks can reuse them. */ async function assertNoCrossTargetCollisions( artifacts: Artifact[], -): Promise> { - const owner = new Map(); +): Promise { + const byWrite = new Map(); + const byEntry = new Map(); const collisions: string[] = []; for (const artifact of artifacts) { for (const managedPath of artifact.managedPaths) { const absolute = path.resolve(artifact.outDir, managedPath); - const key = await physicalKey(absolute, { entry: true }); - const previous = owner.get(key); + const claim = { target: artifact.target, absolute }; + const writeKey = await physicalKey(absolute); + const previous = byWrite.get(writeKey); if (previous && previous.target !== artifact.target) { collisions.push( ` ${previous.target} and ${artifact.target}: ${describeAlias(previous.absolute, absolute)}`, ); - } else { - owner.set(key, { target: artifact.target, absolute }); + continue; + } + byWrite.set(writeKey, claim); + const entryKey = await physicalKey(absolute, { entry: true }); + if (!byEntry.has(entryKey)) { + byEntry.set(entryKey, claim); } } } if (collisions.length > 0) { throw new Error( - `Targets write overlapping output paths; give them distinct outDirs:\n${collisions.join("\n")}`, + `Targets write overlapping output paths; give them distinct outDirs:\n${collisions.join("\n")}\n` + + `If an output path is a symlink to another target's file, replace it with a regular file or remove it.`, ); } - return owner; + return { byWrite, byEntry }; } /** @@ -224,7 +244,7 @@ async function assertNoCrossTargetCollisions( async function assertNoCollisionsWithBuiltTargets( project: ResolvedProject, targets: TargetName[], - incoming: Map, + incoming: IncomingPaths, ): Promise> { const building = new Set(targets); const claimed = new Map(); @@ -246,7 +266,9 @@ async function assertNoCollisionsWithBuiltTargets( const absolute = path.resolve(otherOutDir, normalizeManagedPath(file)); const key = await physicalKey(absolute, { entry: true }); claimed.set(key, other); - const conflicting = incoming.get(key); + const conflicting = + incoming.byEntry.get(key) ?? + incoming.byWrite.get(await physicalKey(absolute)); if (conflicting) { collisions.push( ` ${other} (previously built) and ${conflicting.target}: ${describeAlias(absolute, conflicting.absolute)}`, diff --git a/tests/physical-path-safety.test.ts b/tests/physical-path-safety.test.ts index aa847bd..74600f6 100644 --- a/tests/physical-path-safety.test.ts +++ b/tests/physical-path-safety.test.ts @@ -409,6 +409,46 @@ describe("cross-target collision checks compare physical paths", () => { }, ); + it("rejects a generated file that is a symlink to another target's generated file", async () => { + const root = await makeProject( + ` antigravity: { + outDir: "out", + plugins: { first: { source: "shared/acme", path: "first" } } + }, + cursor: { + outDir: "out", + plugins: { second: { source: "shared/other", path: "second" } } + }`, + { + out: { + second: { skills: { alpha: { "SKILL.md": "OLD\n" } } }, + first: { skills: { alpha: {} } }, + }, + }, + ); + await mkdir(path.join(root, "shared/other/skills/alpha"), { + recursive: true, + }); + await writeFile( + path.join(root, "shared/other/skills/alpha/SKILL.md"), + skill("alpha").replace("body", "OTHER"), + ); + await symlink( + "../../../second/skills/alpha/SKILL.md", + path.join(root, "out/first/skills/alpha/SKILL.md"), + ); + + await expect(build({ cwd: root })).rejects.toThrow( + /Targets write overlapping output paths[\s\S]*antigravity and cursor: .*out\/first\/skills\/alpha\/SKILL\.md \(same file as .*out\/second\/skills\/alpha\/SKILL\.md\)/, + ); + expect( + await readFile( + path.join(root, "out/second/skills/alpha/SKILL.md"), + "utf8", + ), + ).toBe("OLD\n"); + }); + it("refuses to clean one target's manifest entries that alias another's through a symlink", async () => { const root = await makeProject(codexAndAgentPlugins("out", "alias-out")); await mkdir(path.join(root, "out")); From b46def66e8ea5d2ee2c03ccfdcf4ef8d46c5679d Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Wed, 21 Oct 2026 15:36:36 -0700 Subject: [PATCH 08/13] fix(managed): check deletion containment before any write or delete The checks that stop prune and clean from deleting through a symlink out of the output directory ran only inside the delete itself. A dry run skipped them and succeeded where the real command refused. A build wrote its new output and then refused at prune time. clean deleted one target's files and then refused for the next. The containment checks now run in the shared preflight for every stale or managed path, on dry runs too. clean now plans every target before deleting anything. A managed path below a directory someone replaced with a file made clean throw a raw ENOTDIR after it had already deleted other files. That managed file cannot exist, so it now counts as already gone, and the file in its way stays. Any other failure to inspect a managed path, such as EACCES on a locked directory, now names the managed manifest, says nothing was deleted, and gives the way out. --- src/build.ts | 7 +- src/cleanup.ts | 55 +++++--- src/managed.ts | 207 ++++++++++++++++++++++++----- tests/physical-path-safety.test.ts | 91 +++++++++++++ 4 files changed, 302 insertions(+), 58 deletions(-) diff --git a/src/build.ts b/src/build.ts index 01c8d51..8f02bd7 100644 --- a/src/build.ts +++ b/src/build.ts @@ -5,6 +5,7 @@ import { assertManagedManifestContained, buildDeleteGuard, executePrune, + managedPathKey, normalizeManagedPath, planPrune, readManagedManifest, @@ -264,11 +265,13 @@ async function assertNoCollisionsWithBuiltTargets( } for (const file of manifest.files) { const absolute = path.resolve(otherOutDir, normalizeManagedPath(file)); - const key = await physicalKey(absolute, { entry: true }); + const key = await managedPathKey(otherOutDir, other, file, { + entry: true, + }); claimed.set(key, other); const conflicting = incoming.byEntry.get(key) ?? - incoming.byWrite.get(await physicalKey(absolute)); + incoming.byWrite.get(await managedPathKey(otherOutDir, other, file)); if (conflicting) { collisions.push( ` ${other} (previously built) and ${conflicting.target}: ${describeAlias(absolute, conflicting.absolute)}`, diff --git a/src/cleanup.ts b/src/cleanup.ts index 7708340..d29806c 100644 --- a/src/cleanup.ts +++ b/src/cleanup.ts @@ -2,14 +2,18 @@ import path from "node:path"; import { describeAlias, planBuild } from "./build.js"; import { targetNames } from "./adapters.js"; import { loadProjectConfig } from "./config.js"; -import { physicalKey } from "./fs.js"; +import { withPathCache } from "./fs.js"; import { buildDeleteGuard, - cleanManagedFiles, + cleanResult, + executeClean, executePrune, + managedPathKey, + planClean, normalizeManagedPath, pruneResult, readManagedManifest, + type CleanPlan, } from "./managed.js"; import type { CleanupResult, @@ -59,27 +63,34 @@ export async function clean( force?: boolean; } = {}, ): Promise { - const project = await loadProjectConfig(options.cwd, options.configPath); - const guard = await buildDeleteGuard(project, options.force); - const targets = options.target - ? [options.target] - : (Object.keys(project.config.targets) as TargetName[]); - await assertNoManifestCollisions(project, targets, options.force); - const results: CleanupResult[] = []; - for (const target of targets) { - const targetConfig = project.config.targets[target]; - if (!targetConfig) { - throw new Error(`Target "${target}" is not configured.`); + // Check every target before deleting anything, so a refusal for one target + // does not leave another already torn down. + const plans = await withPathCache(async () => { + const project = await loadProjectConfig(options.cwd, options.configPath); + const guard = await buildDeleteGuard(project, options.force); + const targets = options.target + ? [options.target] + : (Object.keys(project.config.targets) as TargetName[]); + await assertNoManifestCollisions(project, targets, options.force); + const planned: CleanPlan[] = []; + for (const target of targets) { + const targetConfig = project.config.targets[target]; + if (!targetConfig) { + throw new Error(`Target "${target}" is not configured.`); + } + const outDir = path.resolve(project.rootDir, targetConfig.outDir); + planned.push( + await planClean(outDir, target, { dryRun: options.dryRun, guard }), + ); + } + return planned; + }); + if (!options.dryRun) { + for (const plan of plans) { + await executeClean(plan); } - const outDir = path.resolve(project.rootDir, targetConfig.outDir); - results.push( - await cleanManagedFiles(outDir, target, { - dryRun: options.dryRun, - guard, - }), - ); } - return results; + return plans.map(cleanResult); } /** @@ -122,7 +133,7 @@ async function assertNoManifestCollisions( } for (const file of manifest.files) { const absolute = path.resolve(outDir, normalizeManagedPath(file)); - const key = await physicalKey(absolute, { entry: true }); + const key = await managedPathKey(outDir, target, file, { entry: true }); const owner = owners.get(key); if (owner && owner.target !== target) { if (cleaning.includes(owner.target) || cleaning.includes(target)) { diff --git a/src/managed.ts b/src/managed.ts index 2347639..31261e7 100644 --- a/src/managed.ts +++ b/src/managed.ts @@ -4,6 +4,7 @@ import path from "node:path"; import { isInside, isInsideFolded, + isNotFoundError, json, physicalKey, resolveInside, @@ -206,7 +207,7 @@ export async function planPrune( continue; } if (options.owners) { - const key = await physicalKey(path.resolve(artifact.outDir, file), { + const key = await managedPathKey(artifact.outDir, artifact.target, file, { entry: true, }); const owner = options.owners.get(key); @@ -218,10 +219,12 @@ export async function planPrune( } await assertNoProtectedDeletions( artifact.outDir, + artifact.target, stale, options.guard, "prune", ); + await assertRemovalsContained(artifact.outDir, artifact.target, stale); await warnOnEditedFiles( artifact.outDir, artifact.target, @@ -276,19 +279,37 @@ export async function warnOnEditedOverwrites( ); } -/** Deletes every file the previous build managed for a target, including its manifest. */ -export async function cleanManagedFiles( +/** What a clean will delete for one target, decided before anything is removed. */ +export type CleanPlan = { + target: TargetName; + outDir: string; + /** Every managed path, then the manifest itself; empty when no manifest exists. */ + paths: string[]; +}; + +/** + * Checks it is safe to delete every file the previous build managed for a + * target, without touching disk: the delete guard, containment of every path, + * and a warning for each edited file. Runs on dry runs too. + */ +export async function planClean( outDir: string, target: TargetName, options: { dryRun?: boolean; guard?: DeleteGuard } = {}, -): Promise { +): Promise { const previous = await readManagedManifest(outDir, target); - const entries: CleanupEntry[] = []; if (!previous) { - return { target, outDir, entries }; + return { target, outDir, paths: [] }; } const files = previous.files.map(normalizeManagedPath); - await assertNoProtectedDeletions(outDir, files, options.guard, "clean"); + await assertNoProtectedDeletions( + outDir, + target, + files, + options.guard, + "clean", + ); + await assertRemovalsContained(outDir, target, files); await warnOnEditedFiles( outDir, target, @@ -296,18 +317,27 @@ export async function cleanManagedFiles( previous, options.dryRun ? "a real run would delete it" : "deleting it", ); - for (const normalized of files) { - entries.push({ type: "deleted", target, path: normalized }); - if (!options.dryRun) { - await removeManagedPath(outDir, normalized, target); - } - } - const manifestPath = managedManifestPath(target); - entries.push({ type: "deleted", target, path: manifestPath }); - if (!options.dryRun) { - await removeManagedPath(outDir, manifestPath, target); + return { target, outDir, paths: [...files, managedManifestPath(target)] }; +} + +/** Deletes what a `planClean` decided on, the manifest last. */ +export async function executeClean(plan: CleanPlan): Promise { + for (const file of plan.paths) { + await removeManagedPath(plan.outDir, file, plan.target); } - return { target, outDir, entries }; +} + +/** The cleanup report for a clean plan. */ +export function cleanResult(plan: CleanPlan): CleanupResult { + return { + target: plan.target, + outDir: plan.outDir, + entries: plan.paths.map((file): CleanupEntry => ({ + type: "deleted", + target: plan.target, + path: file, + })), + }; } /** @@ -380,6 +410,7 @@ export async function buildDeleteGuard( async function assertNoProtectedDeletions( outDir: string, + target: TargetName, paths: string[], guard: DeleteGuard | undefined, command: string, @@ -396,7 +427,7 @@ async function assertNoProtectedDeletions( ); const blocked: Array<{ file: string; root: string }> = []; for (const file of paths) { - const root = await protectingRoot(outDir, file, rootKeys); + const root = await protectingRoot(outDir, target, file, rootKeys); if (root !== null) { blocked.push({ file, root }); } @@ -427,6 +458,7 @@ async function assertNoProtectedDeletions( */ async function protectingRoot( outDir: string, + target: TargetName, relativePath: string, roots: Array<{ root: string; key: string }>, ): Promise { @@ -435,10 +467,54 @@ async function protectingRoot( if (lexical) { return lexical.root; } - const key = await physicalKey(absolute, { entry: true }); + const key = await managedPathKey(outDir, target, relativePath, { + entry: true, + }); return roots.find((root) => isInside(root.key, key))?.root ?? null; } +/** + * `physicalKey` for a path a target's managed manifest lists. A failure to + * resolve it (EACCES on a locked directory, say) names the manifest and the + * way out, since every caller runs before anything is deleted. + */ +export async function managedPathKey( + outDir: string, + target: TargetName, + relativePath: string, + options: { entry?: boolean } = {}, +): Promise { + try { + return await physicalKey( + path.resolve(outDir, normalizeManagedPath(relativePath)), + options, + ); + } catch (error) { + throw uncheckedPathError(outDir, target, relativePath, error); + } +} + +function uncheckedPathError( + outDir: string, + target: TargetName, + relativePath: string, + error: unknown, +): Error { + return new Error( + `Cannot check managed path "${relativePath}" under ${outDir}: ${(error as Error).message}. ` + + nothingDeleted(outDir, target), + { cause: error }, + ); +} + +function nothingDeleted(outDir: string, target: TargetName): string { + const manifest = path.join(path.resolve(outDir), managedManifestPath(target)); + return ( + `Nothing was deleted. The managed manifest ${manifest} lists this path. ` + + `Fix the cause and re-run the same command, or remove the entry from that manifest to leave the path alone.` + ); +} + /** Normalizes a managed path to a safe, relative, forward-slash form, throwing if it escapes the output dir. */ export function normalizeManagedPath(value: string): string { const normalized = path.posix.normalize(value.replace(/\\/g, "/")); @@ -459,23 +535,34 @@ export function normalizeManagedPath(value: string): string { return normalized.startsWith("./") ? normalized.slice(2) : normalized; } -async function removeManagedPath( - outDir: string, +/** A managed path that cannot be removed safely. The message says why. */ +class UnsafeRemovalError extends Error {} + +/** + * The absolute path `relativePath` names under `root`, or `null` when there is + * nothing to remove: the entry is missing, or an ancestor is now a file, so + * the managed file cannot exist. Throws `UnsafeRemovalError` when removing the + * entry would reach outside `root`. + */ +async function inspectRemoval( + root: string, relativePath: string, - target: TargetName, -): Promise { - const root = path.resolve(outDir); +): Promise { const normalized = normalizeManagedPath(relativePath); const destination = path.resolve(root, normalized); if (!isInside(root, destination)) { - throw new Error(`Managed path escapes output directory: ${relativePath}`); + throw new UnsafeRemovalError( + `Managed path escapes output directory: ${relativePath}`, + ); } let stats; try { stats = await fs.lstat(destination); } catch (error) { - if (isNotFound(error)) { - return; + // ENOTDIR: an ancestor was replaced by a file, so the managed file is + // gone and the file in its way is not pluginpack's to delete. + if (isNotFoundError(error)) { + return null; } throw error; } @@ -485,7 +572,7 @@ async function removeManagedPath( await fs.readlink(destination), ); if (!isInside(root, linkTarget)) { - throw new Error( + throw new UnsafeRemovalError( `Refusing to remove a symlink pointing outside the output directory: ${relativePath}`, ); } @@ -495,18 +582,70 @@ async function removeManagedPath( // 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( + throw new UnsafeRemovalError( `Refusing to remove a path that resolves outside the output directory: ${relativePath}`, ); } + return destination; +} + +/** + * Runs the containment checks `removeManagedPath` runs, for every path, before + * anything is written or deleted. Dry runs run it too, so `--dry-run` refuses + * a path that escapes the output directory exactly where the real command + * would, and a build refuses before writing new output. + */ +async function assertRemovalsContained( + outDir: string, + target: TargetName, + paths: string[], +): Promise { + const root = path.resolve(outDir); + for (const file of paths) { + try { + await inspectRemoval(root, file); + } catch (error) { + if (error instanceof UnsafeRemovalError) { + throw new Error( + `${error.message} (under ${outDir}). ${nothingDeleted(outDir, target)}`, + { cause: error }, + ); + } + throw uncheckedPathError(outDir, target, file, error); + } + } +} + +async function removeManagedPath( + outDir: string, + relativePath: string, + target: TargetName, +): Promise { + const root = path.resolve(outDir); + const manifest = path.join(root, managedManifestPath(target)); + const recovery = + `The managed manifest ${manifest} still lists every remaining path, so fix the cause and re-run the same command. ` + + `To stop pluginpack managing these files instead, delete that manifest.`; + let destination: string | null; + try { + destination = await inspectRemoval(root, relativePath); + } catch (error) { + if (error instanceof UnsafeRemovalError) { + throw new Error(`${error.message}. ${recovery}`, { cause: error }); + } + throw new Error( + `Failed to check managed path "${relativePath}" under ${outDir}: ${(error as Error).message}. ${recovery}`, + { cause: error }, + ); + } + if (destination === null) { + return; + } try { await fs.rm(destination, { force: true }); } catch (error) { - const manifest = path.join(root, managedManifestPath(target)); throw new Error( - `Failed to remove managed path "${relativePath}" under ${outDir}: ${(error as Error).message}. ` + - `The managed manifest ${manifest} still lists every remaining path, so fix the cause and re-run the same command. ` + - `To stop pluginpack managing these files instead, delete that manifest.`, + `Failed to remove managed path "${relativePath}" under ${outDir}: ${(error as Error).message}. ${recovery}`, { cause: error }, ); } diff --git a/tests/physical-path-safety.test.ts b/tests/physical-path-safety.test.ts index 74600f6..0c29063 100644 --- a/tests/physical-path-safety.test.ts +++ b/tests/physical-path-safety.test.ts @@ -2,6 +2,7 @@ import { execFileSync } from "node:child_process"; import { existsSync } from "node:fs"; import { access, + chmod, link, mkdir, mkdtemp, @@ -586,6 +587,44 @@ describe("dry runs run the same destructive preflight as the real command", () = await access(path.join(root, "skills/gamma/SKILL.md")); }); + describe("with a stale path that crosses a symlink out of the outDir", () => { + async function escapingStalePath(): Promise { + const root = await makeProject( + ` antigravity: { + outDir: "out", + plugins: { acme: { source: "shared/acme" } } + }`, + { outside: { "sentinel.txt": "UNMANAGED\n" }, out: {} }, + ); + await symlink("../outside", path.join(root, "out/link")); + await writeManifest(root, "out", "antigravity", ["link/sentinel.txt"]); + return root; + } + const refusal = + /Refusing to remove a path that resolves outside the output directory: link\/sentinel\.txt \(under .*out\)\. Nothing was deleted\. The managed manifest .*out\/\.pluginpack\/antigravity\.json lists this path/; + + it.each(["clean", "prune"] as const)( + "%s --dry-run refuses it, as the real command does", + async (command) => { + const root = await escapingStalePath(); + const run = command === "clean" ? clean : prune; + + await expect(run({ cwd: root, dryRun: true })).rejects.toThrow(refusal); + await expect(run({ cwd: root })).rejects.toThrow(refusal); + await access(path.join(root, "outside/sentinel.txt")); + }, + ); + + it("build refuses it before writing any output", async () => { + const root = await escapingStalePath(); + + await expect(build({ cwd: root, dryRun: true })).rejects.toThrow(refusal); + await expect(build({ cwd: root })).rejects.toThrow(refusal); + await expect(exists(path.join(root, "out/acme"))).resolves.toBe(false); + await access(path.join(root, "outside/sentinel.txt")); + }); + }); + it("build --dry-run fails where build would, and build refuses before writing", async () => { const root = await protectedStalePath(); await rm(path.join(root, "generated"), { recursive: true }); @@ -843,6 +882,58 @@ describe("a failed clean says how to recover", () => { }); }); +describe("clean when generated directories were replaced or locked", () => { + const target = ` antigravity: { + outDir: "out", + plugins: { acme: { source: "shared/acme" } } + }`; + + it("treats managed files below a directory now replaced by a file as gone", async () => { + const root = await makeProject(target); + await build({ cwd: root }); + await rm(path.join(root, "out/acme/skills"), { recursive: true }); + await writeFile(path.join(root, "out/acme/skills"), "USER FILE\n"); + + await clean({ cwd: root }); + + expect(await readFile(path.join(root, "out/acme/skills"), "utf8")).toBe( + "USER FILE\n", + ); + await expect(exists(path.join(root, "out/acme/plugin.json"))).resolves.toBe( + false, + ); + await expect( + exists(path.join(root, "out", managedManifestPath("antigravity"))), + ).resolves.toBe(false); + }); + + it.skipIf(process.getuid?.() === 0 || process.platform === "win32")( + "names the manifest and deletes nothing when a managed path cannot be inspected", + async () => { + const root = await makeProject(target); + await build({ cwd: root }); + const locked = path.join(root, "out/acme/skills"); + await chmod(locked, 0o000); + try { + const manifestPath = path.join( + root, + "out", + managedManifestPath("antigravity"), + ); + await expect(clean({ cwd: root })).rejects.toThrow( + new RegExp( + `Cannot check managed path "acme/skills/alpha/SKILL\\.md" under .*out: EACCES[\\s\\S]*Nothing was deleted\\. The managed manifest ${manifestPath.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")} lists this path`, + ), + ); + await access(path.join(root, "out/acme/plugin.json")); + await access(manifestPath); + } finally { + await chmod(locked, 0o755); + } + }, + ); +}); + describe("an external outDir keeps its version-control data", () => { it("refuses to clean a manifest entry inside the outDir's .git", async () => { const root = await makeProject( From 5f864e3e1074ded0c7d63f8ebac318d6202052ac Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Wed, 21 Oct 2026 15:39:16 -0700 Subject: [PATCH 09/13] fix(build): keep handed-over paths owned and warn before overwriting their edits When a path moves from one target to another in one build, the donor's new manifest drops it and the recipient's new manifest adds it. build wrote each target's manifest right after that target's prune. If the recipient's prune failed after the donor's manifest was written, neither manifest listed the path, and a later clean left the file behind. build now first writes every target's manifest with its new paths and its old ones, then prunes each target and writes its final manifest. A handed-over path is then always listed by at least one manifest. The hand-edit warning compared a file only against the overwriting target's own previous manifest. A handed-over file is new to that manifest, so an edit to it vanished without a warning. The plan now carries the hash the previous owner recorded, and the warning names the target it came from. --- src/build.ts | 11 ++++ src/managed.ts | 100 +++++++++++++++++++++++++---- tests/physical-path-safety.test.ts | 66 +++++++++++++++++++ 3 files changed, 163 insertions(+), 14 deletions(-) diff --git a/src/build.ts b/src/build.ts index 8f02bd7..4e6a61e 100644 --- a/src/build.ts +++ b/src/build.ts @@ -9,6 +9,7 @@ import { normalizeManagedPath, planPrune, readManagedManifest, + recordHandedOverHashes, warnOnEditedOverwrites, writeManagedManifest, type PrunePlan, @@ -41,6 +42,15 @@ export async function build(options: BuildOptions = {}): Promise { for (const artifact of artifacts) { await writeArtifact(artifact.outDir, artifact.files); } + // Before any target prunes or gives up a path, every target's manifest + // claims its new paths and still lists its old ones. A path handed from + // one target to another is then always listed by at least one manifest: + // if the recipient's prune fails after the donor's final manifest is + // written, the recipient's interim manifest still owns the path, and a + // later clean can remove it. + for (const plan of prunes) { + await writeManagedManifest(plan.artifact, plan.previous); + } for (const plan of prunes) { await executePrune(plan); await writeManagedManifest(plan.artifact); @@ -110,6 +120,7 @@ async function planBuildChecked( await planPrune(artifact, { dryRun: options.dryRun, guard, owners }), ); } + await recordHandedOverHashes(prunes); return { artifacts, prunes }; } diff --git a/src/managed.ts b/src/managed.ts index 31261e7..09cef43 100644 --- a/src/managed.ts +++ b/src/managed.ts @@ -54,7 +54,11 @@ function contentHash(value: FileValue): string { * as the artifact itself, so a symlinked `.pluginpack/` cannot redirect the * write outside the output directory. */ -export async function writeManagedManifest(artifact: Artifact): Promise { +export async function writeManagedManifest( + artifact: Artifact, + retained: ManagedManifest | null = null, +): Promise { + const files = [...artifact.managedPaths]; const sha256: Record = {}; for (const managedPath of artifact.managedPaths) { const value = artifact.files.get(managedPath); @@ -62,10 +66,20 @@ export async function writeManagedManifest(artifact: Artifact): Promise { sha256[managedPath] = contentHash(value); } } + const current = new Set(artifact.managedPaths.map(normalizeManagedPath)); + for (const file of retained?.files ?? []) { + if (!current.has(normalizeManagedPath(file))) { + files.push(file); + const hash = retained?.sha256[file]; + if (hash !== undefined) { + sha256[file] = hash; + } + } + } const manifest: ManagedManifest = { version: MANIFEST_VERSION, target: artifact.target, - files: artifact.managedPaths, + files, sha256, }; await writeArtifact( @@ -172,13 +186,58 @@ export async function readManagedManifest( return { version: MANIFEST_VERSION, target, files, sha256 }; } +/** A hash a target's manifest recorded for a file it wrote. */ +type RecordedHash = { target: TargetName; sha256: string }; + /** What a prune will delete, decided before anything is written or removed. */ export type PrunePlan = { artifact: Artifact; previous: ManagedManifest | null; stale: string[]; + /** + * For a path this target takes over from another target in this build, the + * hash the previous owner recorded, so an edit to it still warns. Filled + * in by `recordHandedOverHashes`. + */ + handedOver: Map; }; +/** + * Fills in each plan's `handedOver` hashes from the previous manifests of the + * other targets in the same build. A path that moves from one target to + * another (the agent-plugins Codex marketplace handed to the codex target, + * say) is new to the recipient's manifest, so without this an edit to it + * would be overwritten without a warning. + */ +export async function recordHandedOverHashes( + plans: PrunePlan[], +): Promise { + const recorded = new Map(); + for (const plan of plans) { + const { outDir, target } = plan.artifact; + for (const [file, sha256] of Object.entries(plan.previous?.sha256 ?? {})) { + const key = await managedPathKey(outDir, target, file, { entry: true }); + recorded.set(key, { target, sha256 }); + } + } + for (const plan of plans) { + const { outDir, target } = plan.artifact; + const own = new Set( + (plan.previous?.files ?? []).map((file) => normalizeManagedPath(file)), + ); + for (const file of plan.artifact.managedPaths) { + if (own.has(normalizeManagedPath(file))) { + continue; + } + const key = await managedPathKey(outDir, target, file, { entry: true }); + const previousOwner = recorded.get(key); + if (previousOwner && previousOwner.target !== target) { + plan.handedOver.set(file, previousOwner); + } + } + } +} + /** * Decides which previously managed files are stale and checks it is safe to * delete them, without touching disk. Runs on dry runs too, so `--dry-run` @@ -229,10 +288,10 @@ export async function planPrune( artifact.outDir, artifact.target, stale, - previous, + ownHashes(artifact.target, previous), options.dryRun ? "a real run would delete it" : "deleting it", ); - return { artifact, previous, stale }; + return { artifact, previous, stale, handedOver: new Map() }; } /** Deletes the stale files a `planPrune` decided on. */ @@ -264,16 +323,17 @@ export async function warnOnEditedOverwrites( plan: PrunePlan, dryRun?: boolean, ): Promise { - const { artifact, previous } = plan; + const { artifact, previous, handedOver } = plan; const incoming = new Map(); for (const [file, value] of artifact.files) { incoming.set(file, contentHash(value)); } + const own = ownHashes(artifact.target, previous); await warnOnEditedFiles( artifact.outDir, artifact.target, artifact.managedPaths, - previous, + (file) => own(file) ?? handedOver.get(file), dryRun ? "a real build would overwrite it" : "overwriting it", incoming, ); @@ -314,7 +374,7 @@ export async function planClean( outDir, target, files, - previous, + ownHashes(target, previous), options.dryRun ? "a real run would delete it" : "deleting it", ); return { target, outDir, paths: [...files, managedManifestPath(target)] }; @@ -350,15 +410,12 @@ async function warnOnEditedFiles( outDir: string, target: TargetName, files: string[], - previous: ManagedManifest | null, + recordedHash: (file: string) => RecordedHash | undefined, action: string, replacements?: Map, ): Promise { - if (!previous) { - return; - } for (const file of files) { - const recorded = previous.sha256[file]; + const recorded = recordedHash(file); if (recorded === undefined) { continue; } @@ -369,15 +426,30 @@ async function warnOnEditedFiles( continue; } const actual = contentHash(contents); - if (actual !== recorded && actual !== replacements?.get(file)) { + if (actual !== recorded.sha256 && actual !== replacements?.get(file)) { + const from = + recorded.target === target + ? "" + : ` (handed over from ${recorded.target})`; console.warn( - `warning: ${target} managed file ${path.join(outDir, file)} was edited after pluginpack wrote it; ${action}. ` + + `warning: ${target} managed file ${path.join(outDir, file)}${from} was edited after pluginpack wrote it; ${action}. ` + `Keep the edit by moving it into the source, or recover it from version control.`, ); } } } +/** Looks up the hashes a target's own previous manifest recorded. */ +function ownHashes( + target: TargetName, + previous: ManagedManifest | null, +): (file: string) => RecordedHash | undefined { + return (file) => { + const sha256 = previous?.sha256[file]; + return sha256 === undefined ? undefined : { target, sha256 }; + }; +} + /** * Builds the guard that stops prune/clean from deleting the config, a source * tree, or an output repository's `.git` directory. diff --git a/tests/physical-path-safety.test.ts b/tests/physical-path-safety.test.ts index 0c29063..967c671 100644 --- a/tests/physical-path-safety.test.ts +++ b/tests/physical-path-safety.test.ts @@ -518,6 +518,72 @@ describe("a stale path another target now owns is left in place", () => { ).not.toContain(".agents/plugins/marketplace.json"); }); + it("warns before overwriting an edited file handed over from another target", async () => { + const root = await makeProject(agentPluginsWithCodex); + await build({ cwd: root }); + await writeFile( + path.join(root, marketplace), + `${await readFile(path.join(root, marketplace), "utf8")}HAND EDIT\n`, + ); + const configPath = await revision( + root, + "handoff", + `${codexTarget},\n${agentPluginsWithoutCodex}`, + ); + + const warnings = await captureWarnings(() => + build({ cwd: root, configPath }), + ); + + expect(warnings).toEqual([ + expect.stringMatching( + /codex managed file .*out\/\.agents\/plugins\/marketplace\.json \(handed over from agent-plugins\) was edited after pluginpack wrote it; overwriting it/, + ), + ]); + }); + + it("keeps a handed-over path owned when the recipient's prune fails", async () => { + const one = `outDir: "out", plugins: { one: { source: "shared/acme" } }`; + const two = `outDir: "out", plugins: { two: { source: "shared/acme" } }`; + const root = await makeProject( + ` antigravity: { ${one}, rootFiles: { "transfer.txt": "authored.txt" } }, + cursor: { ${two}, rootFiles: { "blocked": "authored.txt" } }`, + { "authored.txt": "AUTHORED\n" }, + ); + await build({ cwd: root }); + // A directory where cursor's stale file was makes cursor's prune fail. + await rm(path.join(root, "out/blocked")); + await mkdir(path.join(root, "out/blocked")); + const configPath = await revision( + root, + "handoff", + ` antigravity: { ${one} }, + cursor: { ${two}, rootFiles: { "transfer.txt": "authored.txt" } }`, + ); + + await expect(build({ cwd: root, configPath })).rejects.toThrow( + /Failed to remove managed path "blocked"/, + ); + + const owners: string[] = []; + for (const target of ["antigravity", "cursor"] as const) { + const manifest = await readManagedManifest( + path.join(root, "out"), + target, + ); + if (manifest?.files.includes("transfer.txt")) { + owners.push(target); + } + } + expect(owners).toEqual(["cursor"]); + await rm(path.join(root, "out/blocked"), { recursive: true }); + await clean({ cwd: root, configPath }); + await expect(exists(path.join(root, "out/transfer.txt"))).resolves.toBe( + false, + ); + await access(path.join(root, "authored.txt")); + }); + it("does not prune a stale path another configured target's manifest claims", async () => { const root = await makeProject(agentPluginsWithCodex); await build({ cwd: root }); From c355903ca52283f66c67e50229f05e0dda744052 Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Wed, 21 Oct 2026 15:41:26 -0700 Subject: [PATCH 10/13] fix(managed): protect discovered legacy sources unless pluginpack wrote them The delete guard stopped treating every plugins/ directory with a component directory as source, because that guess blocked a generated package from pruning itself under outDir ".". It went too far: an authored legacy plugin that no target reads through "from" lost all protection. A managed manifest naming one of its files then let clean delete authored source. The guard now protects those directories again, unless pluginpack provably generated them. Proof means every file in the directory is listed, with a recorded hash, in a configured target's managed manifest. A version 1 manifest records paths but not what wrote them, so it does not count. A directory holding any file no manifest lists stays protected too. --- README.md | 2 +- src/config.ts | 71 +++++++++++++++++++++++------- src/managed.ts | 51 ++++++++++++++++++++- tests/physical-path-safety.test.ts | 50 +++++++++++++++++++++ 4 files changed, 157 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 196e47a..040e707 100644 --- a/README.md +++ b/README.md @@ -190,7 +190,7 @@ copies a whole directory into the generated repository root. `pluginpack` writes a `.pluginpack/.json` managed-file manifest for each built target. That manifest lets builds and cleanup commands remove stale generated files without touching source files or unmanaged repo content. It also records a sha256 of each file as written. If you edit a generated file and a later `build`, `prune` or `clean` would overwrite or delete it, pluginpack prints a warning naming the file first. It still goes ahead, because the output belongs to pluginpack; move the edit into the source to keep it. Manifests from 0.11 and earlier have no hashes, so they produce no warnings until the next build rewrites them. -Before it deletes anything, pluginpack compares real paths, with symlinks resolved and, on a case-insensitive filesystem, case folded. It refuses an `outDir` that lands inside a source, overrides, partials, `repositoryFiles` or `rootFiles` path, even through a symlink. It also refuses two targets that write the same file through different spellings, such as `out` and `OUT` on macOS. `--dry-run` runs the same checks, so it fails wherever the real command would. +Before it deletes anything, pluginpack compares real paths, with symlinks resolved and, on a case-insensitive filesystem, case folded. It refuses an `outDir` that lands inside a source, overrides, partials, `repositoryFiles` or `rootFiles` path, even through a symlink. It also refuses two targets that write the same file through different spellings, such as `out` and `OUT` on macOS, or through a generated file that is a symlink to another target's file. Under the default `plugins/` root, a directory with a component directory counts as legacy source and is never deleted, unless pluginpack recorded writing every file in it. `--dry-run` runs the same checks, so it fails wherever the real command would. An `outDir` outside the project, such as a sibling output repository (`../cursor-plugins`), works. The guard there knows only that repository's `.git` directory and the files the manifest lists, so it cannot protect anything else in that repository from a manifest someone edited by hand. diff --git a/src/config.ts b/src/config.ts index cda22f0..07006f8 100644 --- a/src/config.ts +++ b/src/config.ts @@ -169,7 +169,10 @@ export type SourcePath = { * README's layouts also write generated output — only the legacy source * plugins a target actually reads through `from`, and directories carrying a * `plugin.pluginpack.json` (which generated output never contains), count as - * source. Guessing from component directories misreads a generated package + * source here. Other directories legacy discovery would read as source plugins + * are listed by `listUnreferencedSourcePluginDirs`; the delete guard protects + * them unless pluginpack recorded writing every file in them, since guessing + * from component directories alone misreads a generated package * (`plugins/acme/skills/`) as source and blocks its own prune. */ export async function listSourcePaths( @@ -189,20 +192,7 @@ export async function listSourcePaths( add(config.source.plugins, "source.plugins"); } else { const defaultRoot = path.resolve(rootDir, "plugins"); - const rootPluginId = config.source?.skills - ? (config.source.rootPlugin?.id ?? "core") - : undefined; - const legacyIds = new Set(); - for (const target of Object.values(config.targets)) { - for (const plugin of Object.values(target?.plugins ?? {})) { - for (const id of plugin.from ?? []) { - // The root skills plugin reads only source.skills, protected above. - if (id !== rootPluginId) { - legacyIds.add(id); - } - } - } - } + const legacyIds = referencedLegacyIds(project); for (const id of legacyIds) { paths.push({ path: path.join(defaultRoot, id), @@ -240,6 +230,57 @@ export async function listSourcePaths( return paths; } +/** + * Legacy source plugin ids a target reads through `from`, except the root + * skills plugin, which reads only `source.skills`. + */ +function referencedLegacyIds(project: ResolvedProjectConfig): Set { + const { config } = project; + const rootPluginId = config.source?.skills + ? (config.source.rootPlugin?.id ?? "core") + : undefined; + const ids = new Set(); + for (const target of Object.values(config.targets)) { + for (const plugin of Object.values(target?.plugins ?? {})) { + for (const id of plugin.from ?? []) { + if (id !== rootPluginId) { + ids.add(id); + } + } + } + } + return ids; +} + +/** + * Directories under the default `plugins/` root that legacy discovery reads + * as source plugins (a component directory, no `plugin.pluginpack.json`) but + * that `listSourcePaths` does not already protect, because no target reads + * them through `from`. Such a directory is either authored source nobody + * references yet or a generated package (`plugins/acme/skills/` under + * `outDir: "."`); the delete guard tells them apart by provenance. Empty when + * the config sets `source.plugins`, which is then protected whole. + */ +export async function listUnreferencedSourcePluginDirs( + project: ResolvedProjectConfig, +): Promise { + if (project.config.source?.plugins) { + return []; + } + const defaultRoot = path.resolve(project.rootDir, "plugins"); + const legacyIds = referencedLegacyIds(project); + const dirs: string[] = []; + for (const dir of await listSourcePluginDirs(defaultRoot)) { + if ( + !legacyIds.has(path.basename(dir)) && + !(await exists(path.join(dir, "plugin.pluginpack.json"))) + ) { + dirs.push(dir); + } + } + return dirs; +} + /** * Rejects a config whose target outDir physically resolves inside an authored * path: building there overwrites source, and prune/clean would then have to diff --git a/src/managed.ts b/src/managed.ts index 09cef43..0979cfa 100644 --- a/src/managed.ts +++ b/src/managed.ts @@ -9,9 +9,10 @@ import { physicalKey, resolveInside, toPosix, + walkFiles, writeArtifact, } from "./fs.js"; -import { listSourcePaths } from "./config.js"; +import { listSourcePaths, listUnreferencedSourcePluginDirs } from "./config.js"; import type { Artifact, CleanupEntry, @@ -465,6 +466,7 @@ export async function buildDeleteGuard( const protectedRoots = (await listSourcePaths(project)).map( (source) => source.path, ); + protectedRoots.push(...(await listDiscoveredSourceDirs(project))); // An outDir can sit outside the source repo (a sibling output repo), where // none of the source paths above apply. Its version-control data is the one // thing there no manifest should ever name. @@ -480,6 +482,51 @@ export async function buildDeleteGuard( }; } +/** + * The directories legacy discovery reads as source plugins that nothing else + * protects, minus the ones pluginpack provably generated. + * + * A directory counts as generated only when every file in it (managed + * manifests aside) is listed, with a recorded hash, in a configured target's + * managed manifest: pluginpack wrote each of those files. Anything else is + * treated as authored and protected, including a directory a version 1 + * manifest names (it records paths, not what wrote them) and one holding any + * file no manifest lists. Absence of a `from` reference is not evidence: + * authored legacy source does not need one to exist. + */ +async function listDiscoveredSourceDirs( + project: ResolvedProjectConfig, +): Promise { + const dirs = await listUnreferencedSourcePluginDirs(project); + if (dirs.length === 0) { + return []; + } + const written = new Set(); + for (const [name, target] of Object.entries(project.config.targets)) { + if (!target) { + continue; + } + const outDir = path.resolve(project.rootDir, target.outDir); + const manifest = await readManagedManifest( + outDir, + name as TargetName, + ).catch(() => null); + for (const file of Object.keys(manifest?.sha256 ?? {})) { + written.add(path.resolve(outDir, file)); + } + } + const protectedDirs: string[] = []; + for (const dir of dirs) { + const files = (await walkFiles(dir).catch(() => null))?.filter( + (file) => !isInside(path.join(dir, ".pluginpack"), file), + ); + if (!files?.length || !files.every((file) => written.has(file))) { + protectedDirs.push(dir); + } + } + return protectedDirs; +} + async function assertNoProtectedDeletions( outDir: string, target: TargetName, @@ -511,6 +558,8 @@ async function assertNoProtectedDeletions( `Refusing to ${command} ${blocked.length} path(s) that resolve inside your source tree or config:\n` + `${blocked.map(({ file, root }) => ` ${file} -> resolves inside ${root}`).join("\n")}\n` + `This usually means a target outDir overlaps source, overrides, repositoryFiles, or legacy source config. ` + + `A plugins/ directory with a component directory counts as legacy source unless pluginpack recorded writing every file in it ` + + `(a version 2 managed manifest lists each file with a hash). ` + `Fix the config, or re-run with --force to delete anyway.`, ); } diff --git a/tests/physical-path-safety.test.ts b/tests/physical-path-safety.test.ts index 967c671..cae9f1c 100644 --- a/tests/physical-path-safety.test.ts +++ b/tests/physical-path-safety.test.ts @@ -224,6 +224,56 @@ describe("the delete guard protects source, not generated packages under plugins }); }); +describe("the delete guard protects discovered legacy sources nothing references", () => { + const generatedElsewhere = ` antigravity: { + outDir: ".", + plugins: { acme: { source: "shared/acme", path: "generated" } } + }`; + const authored = { + plugins: { + authored: { + skills: { + alpha: { "SKILL.md": skill("alpha") }, + beta: { "SKILL.md": skill("beta") }, + }, + }, + }, + }; + + it("refuses to clean one that a version 1 manifest names", async () => { + const root = await makeProject(generatedElsewhere, authored); + await writeManifest(root, ".", "antigravity", [ + "plugins/authored/skills/alpha/SKILL.md", + ]); + + await expect(clean({ cwd: root })).rejects.toThrow( + /Refusing to clean 1 path\(s\)[\s\S]*plugins\/authored\/skills\/alpha\/SKILL\.md -> resolves inside .*plugins\/authored/, + ); + await access(path.join(root, "plugins/authored/skills/alpha/SKILL.md")); + }); + + it("refuses to clean one holding a file no manifest records writing", async () => { + const root = await makeProject(generatedElsewhere, authored); + const manifestPath = path.join(root, managedManifestPath("antigravity")); + await mkdir(path.dirname(manifestPath), { recursive: true }); + await writeFile( + manifestPath, + json({ + version: 2, + target: "antigravity", + files: ["plugins/authored/skills/alpha/SKILL.md"], + sha256: { "plugins/authored/skills/alpha/SKILL.md": "0".repeat(64) }, + }), + ); + + await expect(clean({ cwd: root })).rejects.toThrow( + /Refusing to clean 1 path\(s\)[\s\S]*resolves inside .*plugins\/authored/, + ); + await access(path.join(root, "plugins/authored/skills/alpha/SKILL.md")); + await access(path.join(root, "plugins/authored/skills/beta/SKILL.md")); + }); +}); + describe("source protection compares physical paths", () => { async function aliasedSource(): Promise { const root = await makeProject(` "agent-plugins": { From 1d70dbc190a6871b4a57f5343c764385a1472c0e Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Wed, 21 Oct 2026 15:46:38 -0700 Subject: [PATCH 11/13] fix(build): refuse to leave a dropped target's output behind build, prune and clean read only the managed manifests of targets the current config lists. A config that dropped a target, or moved an outDir from one target to another (codex to agent-plugins, say), without cleaning first left the old target's files and its .pluginpack manifest in place forever. Some of those files, like a codex hooks file inside an agent-plugins package, made the new output invalid. build and prune now look for manifests in every configured outDir that belong to no target building there, and refuse with the manifest path and the command to run. Pruning those files automatically would delete them on the authority of a manifest alone. No config would describe that output any more, so a target dropped by mistake, or a second project sharing the output repository, would lose its files silently. clean --target now finds the old target's manifest in any configured outDir, even when that target is no longer configured. A plain clean includes those manifests too, and both run them through the same guard and collision checks. --- README.md | 8 +- src/build.ts | 98 +++++++++++++++++++++++ src/cleanup.ts | 77 ++++++++++++------ src/cli.ts | 6 +- tests/physical-path-safety.test.ts | 123 +++++++++++++++++++++++++++-- 5 files changed, 276 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index 040e707..1d2c573 100644 --- a/README.md +++ b/README.md @@ -192,9 +192,11 @@ copies a whole directory into the generated repository root. Before it deletes anything, pluginpack compares real paths, with symlinks resolved and, on a case-insensitive filesystem, case folded. It refuses an `outDir` that lands inside a source, overrides, partials, `repositoryFiles` or `rootFiles` path, even through a symlink. It also refuses two targets that write the same file through different spellings, such as `out` and `OUT` on macOS, or through a generated file that is a symlink to another target's file. Under the default `plugins/` root, a directory with a component directory counts as legacy source and is never deleted, unless pluginpack recorded writing every file in it. `--dry-run` runs the same checks, so it fails wherever the real command would. +If an `outDir` holds a manifest for a target the config no longer builds there, `build` and `prune` refuse to run. That happens when a config drops a target, or moves an outDir to another target, such as `codex` to `agent-plugins`, without cleaning first. No configured target would prune those files, so they would stay forever. Run `pluginpack clean --target ` to delete them. It finds the old target's manifest in any outDir the config uses, even when that target is no longer configured. A plain `clean` deletes them too. To keep the files unmanaged instead, delete the manifest. + An `outDir` outside the project, such as a sibling output repository (`../cursor-plugins`), works. The guard there knows only that repository's `.git` directory and the files the manifest lists, so it cannot protect anything else in that repository from a manifest someone edited by hand. -**Upgrading from 0.11 (managed files):** the first build rewrites each manifest as version 2. pluginpack 0.11 cannot read a version 2 manifest, so upgrade every machine and CI job that builds the same output together. A manifest from a newer pluginpack fails with a message saying to upgrade. Some configs that 0.11 accepted now fail at load or build: `source.partials` outside the project, an `outDir` inside a source path, and a `rootFiles` entry that copies a file onto itself (`"README.md": "README.md"` with `outDir: "."`). Each error names the field to change. +**Upgrading from 0.11 (managed files):** the first build rewrites each manifest as version 2. pluginpack 0.11 cannot read a version 2 manifest, so upgrade every machine and CI job that builds the same output together. A manifest from a newer pluginpack fails with a message saying to upgrade. Some configs that 0.11 accepted now fail at load or build: `source.partials` outside the project, an `outDir` inside a source path, and a `rootFiles` entry that copies a file onto itself (`"README.md": "README.md"` with `outDir: "."`). Each error names the field to change. A build into an outDir that still holds a dropped target's manifest also fails until you run `pluginpack clean --target `. ## Components @@ -809,7 +811,7 @@ Exit codes: ### `clean` -Remove all managed files for configured target outputs. +Remove all managed files in configured target outputs, including those of a target the config no longer builds there. ```bash pluginpack clean [--target copilot|antigravity|cursor|claude|codex|agent-plugins] [--dry-run] @@ -817,7 +819,7 @@ pluginpack clean [--target copilot|antigravity|cursor|claude|codex|agent-plugins Options: -- `--target `: Clean only one configured target. +- `--target `: Clean only one target. It may be one the config no longer builds, if its manifest is in a configured outDir. - `--dry-run`: Print managed files without deleting them. - `--force`: Delete even paths that resolve inside the source tree or config. diff --git a/src/build.ts b/src/build.ts index 4e6a61e..f3a6f33 100644 --- a/src/build.ts +++ b/src/build.ts @@ -1,3 +1,4 @@ +import { promises as fs } from "node:fs"; import path from "node:path"; import { assertOutDirsOutsideSource, loadConfig } from "./config.js"; import { physicalKey, withPathCache, writeArtifact } from "./fs.js"; @@ -5,6 +6,7 @@ import { assertManagedManifestContained, buildDeleteGuard, executePrune, + managedManifestPath, managedPathKey, normalizeManagedPath, planPrune, @@ -23,6 +25,7 @@ import type { Artifact, BuildOptions, ResolvedProject, + ResolvedProjectConfig, TargetName, } from "./types.js"; @@ -99,6 +102,10 @@ async function planBuildChecked( })), ); } + await assertNoOrphanManifests( + project, + artifacts.map(({ target, outDir }) => ({ target, outDir })), + ); const incoming = await assertNoCrossTargetCollisions(artifacts); const claimedElsewhere = await assertNoCollisionsWithBuiltTargets( project, @@ -299,6 +306,97 @@ async function assertNoCollisionsWithBuiltTargets( return claimed; } +/** Where a target's managed manifest lives: `/.pluginpack/.json`. */ +export type ManifestLocation = { target: TargetName; outDir: string }; + +/** + * Managed manifests in the outDirs this config uses that no target builds + * into: a target the config dropped, or one whose outDir moved. Switching an + * outDir from codex to agent-plugins, say, leaves `.pluginpack/codex.json` + * behind. No configured target prunes the files it lists, so they would stay + * forever, and some (a codex hooks file inside an agent-plugins package) + * make the new output invalid. + * + * Scans the outDir of every configured target, plus `building` (the outDirs + * of this invocation, which a `--out-dir` override moves). A manifest is + * expected where its target is configured to write or is writing now. + */ +export async function findOrphanManifests( + project: ResolvedProjectConfig, + building: ManifestLocation[] = [], +): Promise { + const expected = new Map>(); + const scanned = new Map(); + const add = async ({ target, outDir }: ManifestLocation) => { + const key = await physicalKey(outDir); + expected.set(target, (expected.get(target) ?? new Set()).add(key)); + if (!scanned.has(key)) { + scanned.set(key, outDir); + } + }; + for (const [target, config] of Object.entries(project.config.targets)) { + if (config) { + await add({ + target: target as TargetName, + outDir: path.resolve(project.rootDir, config.outDir), + }); + } + } + for (const location of building) { + await add(location); + } + const known = new Set(targetNames); + const orphans: ManifestLocation[] = []; + for (const [key, outDir] of scanned) { + const names = await fs + .readdir(path.join(outDir, ".pluginpack")) + .catch(() => [] as string[]); + for (const name of names.sort()) { + const target = name.endsWith(".json") ? name.slice(0, -5) : ""; + if (!known.has(target)) { + continue; + } + if (!expected.get(target as TargetName)?.has(key)) { + orphans.push({ target: target as TargetName, outDir }); + } + } + } + return orphans; +} + +/** + * Refuses to build or prune while an outDir holds a manifest no target builds + * there (see `findOrphanManifests`). + * + * Pruning those files instead would delete on the authority of a manifest + * alone, with no config left describing that output: a target dropped from + * the config by mistake, or a second project that shares the output + * repository, would lose its files without a word. Refusing costs one + * command, and `clean --target ` (which finds these manifests) deletes + * the files through the same guard and checks as any clean. + */ +async function assertNoOrphanManifests( + project: ResolvedProjectConfig, + building: ManifestLocation[], +): Promise { + const orphans = await findOrphanManifests(project, building); + if (orphans.length === 0) { + return; + } + const lines = orphans.map( + ({ target, outDir }) => + ` ${path.join(outDir, managedManifestPath(target))} (${target})`, + ); + const commands = [...new Set(orphans.map(({ target }) => target))] + .map((target) => `\`pluginpack clean --target ${target}\``) + .join(", then "); + throw new Error( + `Found managed output from a target this config does not build there:\n${lines.join("\n")}\n` + + `No configured target prunes the files these manifests list, so they would stay in the output. ` + + `Run ${commands} to delete them first. To keep the files unmanaged instead, delete the manifest.`, + ); +} + /** One path, or both spellings when two different strings name the same file. */ export function describeAlias(first: string, second: string): string { return first === second ? first : `${first} (same file as ${second})`; diff --git a/src/cleanup.ts b/src/cleanup.ts index d29806c..0ee3948 100644 --- a/src/cleanup.ts +++ b/src/cleanup.ts @@ -1,5 +1,10 @@ import path from "node:path"; -import { describeAlias, planBuild } from "./build.js"; +import { + describeAlias, + findOrphanManifests, + planBuild, + type ManifestLocation, +} from "./build.js"; import { targetNames } from "./adapters.js"; import { loadProjectConfig } from "./config.js"; import { withPathCache } from "./fs.js"; @@ -68,17 +73,26 @@ export async function clean( const plans = await withPathCache(async () => { const project = await loadProjectConfig(options.cwd, options.configPath); const guard = await buildDeleteGuard(project, options.force); - const targets = options.target - ? [options.target] - : (Object.keys(project.config.targets) as TargetName[]); - await assertNoManifestCollisions(project, targets, options.force); + const configured = configuredLocations(project); + const orphans = await findOrphanManifests(project); + const selected = (location: ManifestLocation) => + !options.target || location.target === options.target; + // A target the config dropped, or one whose outDir moved, left its + // manifest in an outDir this config uses; cleaning it is how a user + // removes that output, so it counts as that target's to clean. + const cleaning = [...configured, ...orphans].filter(selected); + if (options.target && cleaning.length === 0) { + throw new Error( + `Target "${options.target}" is not configured, and no outDir this config uses holds a .pluginpack/${options.target}.json manifest.`, + ); + } + await assertNoManifestCollisions( + [...configured, ...orphans], + cleaning, + options.force, + ); const planned: CleanPlan[] = []; - for (const target of targets) { - const targetConfig = project.config.targets[target]; - if (!targetConfig) { - throw new Error(`Target "${target}" is not configured.`); - } - const outDir = path.resolve(project.rootDir, targetConfig.outDir); + for (const { target, outDir } of cleaning) { planned.push( await planClean(outDir, target, { dryRun: options.dryRun, guard }), ); @@ -93,8 +107,20 @@ export async function clean( return plans.map(cleanResult); } +/** Where each configured target's managed manifest lives, in registry order. */ +function configuredLocations( + project: ResolvedProjectConfig, +): ManifestLocation[] { + return targetNames.flatMap((target) => { + const config = project.config.targets[target]; + return config + ? [{ target, outDir: path.resolve(project.rootDir, config.outDir) }] + : []; + }); +} + /** - * Refuses to clean a path another configured target's manifest also claims. + * Refuses to clean a path another target's manifest also claims. * * `build()` and `prune()` share one plan that rejects overlapping output and * leaves a stale path alone when another target owns it. `clean()` @@ -112,21 +138,22 @@ export async function clean( * unable to tear down their own repo. */ async function assertNoManifestCollisions( - project: ResolvedProjectConfig, - cleaning: TargetName[], + manifests: ManifestLocation[], + cleaning: ManifestLocation[], force?: boolean, ): Promise { if (force) { return; } - const owners = new Map(); + const isCleaning = (location: ManifestLocation) => + cleaning.includes(location); + const owners = new Map< + string, + { location: ManifestLocation; absolute: string } + >(); const collisions: string[] = []; - for (const target of targetNames) { - const targetConfig = project.config.targets[target]; - if (!targetConfig) { - continue; - } - const outDir = path.resolve(project.rootDir, targetConfig.outDir); + for (const location of manifests) { + const { target, outDir } = location; const manifest = await readManagedManifest(outDir, target); if (!manifest) { continue; @@ -135,15 +162,15 @@ async function assertNoManifestCollisions( const absolute = path.resolve(outDir, normalizeManagedPath(file)); const key = await managedPathKey(outDir, target, file, { entry: true }); const owner = owners.get(key); - if (owner && owner.target !== target) { - if (cleaning.includes(owner.target) || cleaning.includes(target)) { + if (owner && owner.location !== location) { + if (isCleaning(owner.location) || isCleaning(location)) { collisions.push( - ` ${owner.target} and ${target}: ${describeAlias(owner.absolute, absolute)}`, + ` ${owner.location.target} and ${target}: ${describeAlias(owner.absolute, absolute)}`, ); } continue; } - owners.set(key, { target, absolute }); + owners.set(key, { location, absolute }); } } if (collisions.length > 0) { diff --git a/src/cli.ts b/src/cli.ts index b18c24d..0840f14 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -173,12 +173,14 @@ function createProgram(): Command { program .command("clean") - .description("Remove all managed files for configured target outputs.") + .description( + "Remove all managed files in configured target outputs, including those of a target the config no longer builds there.", + ) .usage(`[--target ${targetList}] [--dry-run]`) .addOption( new Option( "--target ", - "Clean only one configured target.", + "Clean only one target. It may be one the config no longer builds, if its manifest is in a configured outDir.", ).choices([...targetNames]), ) .option("--dry-run", "Print managed files without deleting them.") diff --git a/tests/physical-path-safety.test.ts b/tests/physical-path-safety.test.ts index cae9f1c..d93fd66 100644 --- a/tests/physical-path-safety.test.ts +++ b/tests/physical-path-safety.test.ts @@ -637,11 +637,11 @@ describe("a stale path another target now owns is left in place", () => { it("does not prune a stale path another configured target's manifest claims", async () => { const root = await makeProject(agentPluginsWithCodex); await build({ cwd: root }); - // The project swaps targets; agent-plugins' old manifest stays on disk. - await build({ - cwd: root, - configPath: await revision(root, "codex-only", codexTarget), - }); + // A codex manifest an older pluginpack could have left claiming the + // marketplace agent-plugins still lists. + await writeManifest(root, "out", "codex", [ + ".agents/plugins/marketplace.json", + ]); const configPath = await revision( root, "both", @@ -661,6 +661,113 @@ describe("a stale path another target now owns is left in place", () => { }); }); +describe("managed output from a target the config no longer builds there", () => { + const entry = `entry: { policy: { installation: "AVAILABLE", authentication: "ON_INSTALL" }, category: "Productivity" }`; + const codexIn = (outDir: string) => ` codex: { + outDir: "${outDir}", + plugins: { acme: { source: "shared/acme", ${entry} } } + }`; + const agentPlugins = ` "agent-plugins": { + outDir: "out", + plugins: { acme: { source: "shared/acme" } } + }`; + const hooks = { + "hooks.json": `${JSON.stringify({ + hooks: { + SessionStart: [{ hooks: [{ type: "command", command: "echo hi" }] }], + }, + })}\n`, + }; + + /** Builds codex into out, then switches the config to agent-plugins there. */ + async function switchedWithoutCleaning(): Promise<{ + root: string; + configPath: string; + }> { + const root = await makeProject(codexIn("out")); + await mkdir(path.join(root, "shared/acme/hooks")); + await writeFile( + path.join(root, "shared/acme/hooks/hooks.json"), + hooks["hooks.json"], + ); + await build({ cwd: root }); + await access(path.join(root, "out/plugins/acme/hooks/hooks.json")); + const configPath = path.join(root, "switched.config.ts"); + await writeFile(configPath, configText(agentPlugins)); + return { root, configPath }; + } + + it("refuses to build or prune, naming the manifest and the clean command", async () => { + const { root, configPath } = await switchedWithoutCleaning(); + const manifest = path.join(root, "out", managedManifestPath("codex")); + const message = `Found managed output from a target this config does not build there:\n ${manifest} (codex)\n`; + + await expect(build({ cwd: root, configPath })).rejects.toThrow(message); + await expect(build({ cwd: root, configPath })).rejects.toThrow( + "Run `pluginpack clean --target codex` to delete them first.", + ); + await expect(prune({ cwd: root, configPath })).rejects.toThrow(message); + await expect( + exists(path.join(root, "out", managedManifestPath("agent-plugins"))), + ).resolves.toBe(false); + }); + + it("clean --target removes it, after which the new target builds clean output", async () => { + const { root, configPath } = await switchedWithoutCleaning(); + + const [result] = await clean({ cwd: root, configPath, target: "codex" }); + expect(result.target).toBe("codex"); + await expect(exists(path.join(root, "out/plugins"))).resolves.toBe(false); + await expect(exists(path.join(root, "out/.pluginpack"))).resolves.toBe( + false, + ); + + const [artifact] = await build({ cwd: root, configPath }); + expect(artifact.managedPaths).not.toContain( + "plugins/acme/hooks/hooks.json", + ); + await expect( + exists(path.join(root, "out/plugins/acme/hooks/hooks.json")), + ).resolves.toBe(false); + await clean({ cwd: root, configPath }); + await expect(walkFiles(path.join(root, "out"))).resolves.toEqual([]); + await sourceSurvives(root); + }); + + it("a plain clean removes it along with the configured targets' output", async () => { + const { root, configPath } = await switchedWithoutCleaning(); + + const results = await clean({ cwd: root, configPath }); + + expect(results.map((result) => result.target)).toEqual([ + "agent-plugins", + "codex", + ]); + await expect(walkFiles(path.join(root, "out"))).resolves.toEqual([]); + await sourceSurvives(root); + }); + + it("clean --target finds the manifest a target left in its old outDir", async () => { + const root = await makeProject(`${codexIn("out")},\n${agentPlugins}`); + await build({ cwd: root, target: "codex" }); + const configPath = path.join(root, "moved.config.ts"); + await writeFile( + configPath, + configText(`${codexIn("out2")},\n${agentPlugins}`), + ); + + await expect(build({ cwd: root, configPath })).rejects.toThrow( + /Found managed output[\s\S]*out\/\.pluginpack\/codex\.json \(codex\)/, + ); + await clean({ cwd: root, configPath, target: "codex" }); + + await expect( + exists(path.join(root, "out", managedManifestPath("codex"))), + ).resolves.toBe(false); + await expect(build({ cwd: root, configPath })).resolves.toHaveLength(2); + }); +}); + describe("dry runs run the same destructive preflight as the real command", () => { async function protectedStalePath(): Promise { const root = await makeProject( @@ -1141,7 +1248,11 @@ describe("the config loader reads the config file fresh each time", () => { await writeFile( path.join(root, name), - configText(antigravity.replace("antigravity", "cursor")), + configText( + antigravity + .replace("antigravity", "cursor") + .replace('"out"', '"out-cursor"'), + ), ); const [second] = await build({ cwd: root }); From f2a2cd9edaf194465608a482e7b29f3156bf1e3c Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Wed, 21 Oct 2026 15:48:18 -0700 Subject: [PATCH 12/13] fix(managed): keep a path renamed only by case when pruning On a case-insensitive volume, changing a plugin path from first to FIRST writes the new files into the existing first/ directory. The old spellings then count as stale, and prune deleted them. Those are the files the build had just written, so the build ended with no output for that plugin. A stale path whose physical key matches one of the target's own current paths is now kept, as a path another target owns already was. --- src/managed.ts | 11 ++++++++--- tests/physical-path-safety.test.ts | 24 ++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/managed.ts b/src/managed.ts index 0979cfa..724d0f9 100644 --- a/src/managed.ts +++ b/src/managed.ts @@ -249,7 +249,9 @@ export async function recordHandedOverHashes( * another target owns has changed hands (for example a marketplace file moved * from one target to another sharing the outDir) and is left in place rather * than deleted from under its new owner. This target's next manifest no - * longer lists it, so the ownership transfer completes on the next build. + * longer lists it, so the ownership transfer completes on the next build. A + * stale path that is physically one of this target's own current paths (a + * case-only rename on a case-insensitive volume) is kept for the same reason. */ export async function planPrune( artifact: Artifact, @@ -270,8 +272,11 @@ export async function planPrune( const key = await managedPathKey(artifact.outDir, artifact.target, file, { entry: true, }); - const owner = options.owners.get(key); - if (owner && owner !== artifact.target) { + // Another owner means the path changed hands. This target as the owner + // means the stale spelling is one of its current files under another + // name (`first/` renamed `FIRST/` on a case-insensitive volume), and + // deleting it would delete the file just written. + if (options.owners.has(key)) { continue; } } diff --git a/tests/physical-path-safety.test.ts b/tests/physical-path-safety.test.ts index d93fd66..ef9cbd5 100644 --- a/tests/physical-path-safety.test.ts +++ b/tests/physical-path-safety.test.ts @@ -634,6 +634,30 @@ describe("a stale path another target now owns is left in place", () => { await access(path.join(root, "authored.txt")); }); + it.skipIf(!caseInsensitiveFs)( + "does not prune a path renamed only by case, which is the file just written", + async () => { + const root = await makeProject(` antigravity: { + outDir: "out", + plugins: { acme: { source: "shared/acme", path: "first" } } + }`); + await build({ cwd: root }); + const configPath = await revision( + root, + "renamed", + ` antigravity: { + outDir: "out", + plugins: { acme: { source: "shared/acme", path: "FIRST" } } + }`, + ); + + await build({ cwd: root, configPath }); + + await access(path.join(root, "out/FIRST/skills/alpha/SKILL.md")); + await access(path.join(root, "out/FIRST/plugin.json")); + }, + ); + it("does not prune a stale path another configured target's manifest claims", async () => { const root = await makeProject(agentPluginsWithCodex); await build({ cwd: root }); From b88e62a75aa70d9d48453b2921620b1e179b136f Mon Sep 17 00:00:00 2001 From: Steve Calvert Date: Wed, 21 Oct 2026 16:03:45 -0700 Subject: [PATCH 13/13] fix(managed): let a 0.11 codex package at outDir "." prune itself after upgrade A version 1 manifest records paths but no hashes, so the delete guard treated every plugins/ directory it named as authored legacy source. For codex with outDir ".", the generated package lives at plugins/, and the codex format moved its manifest from .codex-plugin/plugin.json to the package root after 0.11. The first build after upgrading therefore had to prune a stale path inside a protected directory, refused, and never rewrote the manifest as version 2. Build, prune and clean all stayed refused without --force. A file listed without a hash now also counts as written by pluginpack when it sits inside a plugin directory that the same target's config places its output in. That is config-derived, so clean still works when the source tree no longer loads. A directory a version 1 manifest names but no target writes to stays protected, as does any directory holding a file no manifest lists. --- src/managed.ts | 43 +++++++++++--- tests/physical-path-safety.test.ts | 94 ++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 9 deletions(-) diff --git a/src/managed.ts b/src/managed.ts index 724d0f9..483eaa6 100644 --- a/src/managed.ts +++ b/src/managed.ts @@ -13,6 +13,7 @@ import { writeArtifact, } from "./fs.js"; import { listSourcePaths, listUnreferencedSourcePluginDirs } from "./config.js"; +import { targets as targetRegistry } from "./targets/registry.js"; import type { Artifact, CleanupEntry, @@ -492,12 +493,17 @@ export async function buildDeleteGuard( * protects, minus the ones pluginpack provably generated. * * A directory counts as generated only when every file in it (managed - * manifests aside) is listed, with a recorded hash, in a configured target's - * managed manifest: pluginpack wrote each of those files. Anything else is - * treated as authored and protected, including a directory a version 1 - * manifest names (it records paths, not what wrote them) and one holding any - * file no manifest lists. Absence of a `from` reference is not evidence: - * authored legacy source does not need one to exist. + * manifests aside) is one pluginpack recorded writing. A file counts when a + * configured target's managed manifest lists it with a hash, or lists it + * without one (a version 1 manifest from 0.11 or earlier) and the file sits + * inside a plugin directory that same target's config places its output in, + * such as `plugins/acme` for `codex` with `outDir: "."`. The second case + * keeps a project upgrading from 0.11 able to prune a path the new build no + * longer writes; without it the first build refuses and the manifest never + * gains hashes. Anything else is treated as authored and protected: a + * directory a version 1 manifest names but no target's config writes to, and + * one holding any file no manifest lists. Absence of a `from` reference is not + * evidence: authored legacy source does not need one to exist. */ async function listDiscoveredSourceDirs( project: ResolvedProjectConfig, @@ -516,8 +522,26 @@ async function listDiscoveredSourceDirs( outDir, name as TargetName, ).catch(() => null); - for (const file of Object.keys(manifest?.sha256 ?? {})) { - written.add(path.resolve(outDir, file)); + if (!manifest) { + continue; + } + const definition = targetRegistry[name as TargetName]; + const layout = definition.forConfig?.(target) ?? definition; + const pluginDirs = Object.entries(target.plugins).map( + ([pluginName, pluginConfig]) => + path.resolve( + outDir, + layout.resolvePluginPath(pluginName, pluginConfig, target), + ), + ); + for (const file of manifest.files) { + const absolute = path.resolve(outDir, file); + if ( + manifest.sha256[file] !== undefined || + pluginDirs.some((dir) => isInside(dir, absolute)) + ) { + written.add(absolute); + } } } const protectedDirs: string[] = []; @@ -564,7 +588,8 @@ async function assertNoProtectedDeletions( `${blocked.map(({ file, root }) => ` ${file} -> resolves inside ${root}`).join("\n")}\n` + `This usually means a target outDir overlaps source, overrides, repositoryFiles, or legacy source config. ` + `A plugins/ directory with a component directory counts as legacy source unless pluginpack recorded writing every file in it ` + - `(a version 2 managed manifest lists each file with a hash). ` + + `(a managed manifest lists each file with a hash, or lists it inside a plugin directory that target's config writes). ` + + `A file you added there that no manifest lists also makes it count as source; move it out. ` + `Fix the config, or re-run with --force to delete anyway.`, ); } diff --git a/tests/physical-path-safety.test.ts b/tests/physical-path-safety.test.ts index ef9cbd5..ed982b3 100644 --- a/tests/physical-path-safety.test.ts +++ b/tests/physical-path-safety.test.ts @@ -274,6 +274,100 @@ describe("the delete guard protects discovered legacy sources nothing references }); }); +describe("upgrading a codex outDir '.' build from 0.11 (version 1 manifest)", () => { + const codexAtRoot = ` codex: { + outDir: ".", + plugins: { acme: { source: "shared/acme" } } + }`; + + /** + * The files and version 1 manifest pluginpack 0.11.1 writes for `codexAtRoot` + * when the source also had a skill `gamma` (since deleted). 0.11 wrote the + * Codex manifest under `.codex-plugin/`; the package format now writes it at + * the package root, so the first build after upgrading must prune it. + */ + async function seed011(root: string) { + const files: Record = { + ".agents/plugins/marketplace.json": json({ + name: "physical-demo", + interface: { displayName: "physical-demo" }, + plugins: [ + { + name: "acme", + source: "./plugins/acme", + description: "P", + version: "1.0.0", + }, + ], + }), + "plugins/acme/.codex-plugin/plugin.json": json({ + name: "acme", + version: "1.0.0", + description: "P", + author: { name: "P" }, + license: "MIT", + skills: "./skills/", + }), + "plugins/acme/skills/alpha/SKILL.md": skill("alpha"), + "plugins/acme/skills/beta/SKILL.md": skill("beta"), + "plugins/acme/skills/gamma/SKILL.md": skill("gamma"), + }; + for (const [file, body] of Object.entries(files)) { + await mkdir(path.dirname(path.join(root, file)), { recursive: true }); + await writeFile(path.join(root, file), body); + } + await writeManifest(root, ".", "codex", Object.keys(files)); + } + + it("builds, prunes the stale 0.11 paths, and upgrades the manifest", async () => { + const root = await makeProject(codexAtRoot); + await seed011(root); + + await build({ cwd: root }); + + const manifest = await readManagedManifest(root, "codex"); + expect(manifest?.version).toBe(2); + expect( + await exists(path.join(root, "plugins/acme/.codex-plugin/plugin.json")), + ).toBe(false); + expect( + await exists(path.join(root, "plugins/acme/skills/gamma/SKILL.md")), + ).toBe(false); + await access(path.join(root, "plugins/acme/plugin.json")); + await sourceSurvives(root); + }); + + it("prunes and cleans straight off the version 1 manifest", async () => { + const root = await makeProject(codexAtRoot); + await seed011(root); + + await prune({ cwd: root }); + expect( + await exists(path.join(root, "plugins/acme/skills/gamma/SKILL.md")), + ).toBe(false); + await clean({ cwd: root }); + + expect(await exists(path.join(root, "plugins/acme"))).toBe(false); + expect(await exists(path.join(root, managedManifestPath("codex")))).toBe( + false, + ); + await sourceSurvives(root); + }); + + it("still refuses when the package directory holds a file the manifest does not list", async () => { + const root = await makeProject(codexAtRoot, { + plugins: { acme: { agents: { "mine.md": "authored\n" } } }, + }); + await seed011(root); + + await expect(clean({ cwd: root })).rejects.toThrow( + /Refusing to clean \d+ path\(s\)[\s\S]*resolves inside .*plugins\/acme/, + ); + await access(path.join(root, "plugins/acme/agents/mine.md")); + await access(path.join(root, "plugins/acme/skills/alpha/SKILL.md")); + }); +}); + describe("source protection compares physical paths", () => { async function aliasedSource(): Promise { const root = await makeProject(` "agent-plugins": {