ai-driven-dev · blafourcade · Jul 29, 2026 · Jul 29, 2026
@@ -0,0 +1,62 @@
+---
+objective: "Turn the implicit build-cache force:true assumption in EnsureBuiltMarketplaceUseCase into an explicit, tested outDir invariant, after confirming the reported data-loss risk does not exist."
+status: implemented
+---
+
+# Plan: build-cache force invariant
+
+## What was verified before writing anything
+
+`EnsureBuiltMarketplaceUseCase.runBuild` (`src/application/use-cases/shared/ensure-built-marketplace-use-case.ts`) has exactly two callers, both private methods of the same class:
+
+- `build()` (line 112) passes `builtDir`, computed as `builtMarketplaceDir(projectRoot, marketplaceName, target)`, which resolves to `<projectRoot>/.aidd/cache/built/<marketplace>/<target>` (`domain/models/paths.ts`). Used when the resolved source directory does not nest with the cache.
+- `buildViaTemp()` (line 131) passes `temp = join(tmpdir(), 'aidd-built-<target>-<mode>')`, which is `deleteDirectory`'d immediately before the build (line 130), so it is always empty at build time. Used when the source nests with the cache (the "dogfood" case: building the framework's own repo).
+
+Neither directory is user-authored content. `builtDir` is an aidd-owned, regenerable cache; `temp` is a scratch directory this class owns and clears itself. Forcing collision-overwrite at either path is cache invalidation, not destruction of user work. The direct `aidd framework build --flat --force` CLI path (`commands/framework.ts`) threads the user's real `--force` flag independently and is unaffected by anything in this file.
+
+No third caller of `runBuild` exists, and no `outDir` reaching it can point outside `.aidd/cache/built/` or the OS temp dir under the current code. The reported data-loss risk is infirmed.
+
+## A correction to the ticket's premise
+
+The ticket described the mechanism as: `runBuild` passes `force: true` to `build.execute(...)`, and that value "reaches `FlatBuildStrategy`" where `checkCollision` decides whether to throw. This is not how the code works, and it matters for where the comment and test belong.
+
+`FrameworkBuildUseCase.execute(options)` (`src/application/use-cases/framework/framework-build-use-case.ts`) never reads `options.force`. The `force` that `FlatBuildStrategy.checkCollision` actually consults (`this.force`, `flat-build-strategy.ts:32`) is a constructor parameter, fixed when the strategy is built. For the `EnsureBuiltMarketplaceUseCase` path, that happens in the `frameworkBuildFor` closure in `src/infrastructure/deps.ts` (`ctx.force`, hardcoded `true` there), not inside `runBuild`.
+
+So `force: true` at `ensure-built-marketplace-use-case.ts:151` (`build.execute({ ..., force: true })`) is a value the domain type `FrameworkBuildOptions.force` accepts but the use-case never consults; it has no effect on collision behavior. This was confirmed empirically (see Verification, mutation a). `FrameworkBuildOptions.force` therefore appears vestigial as a field on `execute()`'s options. This task leaves the type alone, since removing it is a type-level change out of scope for a comment-plus-test change.
+
+The comment and reasoning documented in `deps.ts:450-454` (added by commit `69b0537c`, "fix(cli): document intentional force on internal build-cache rebuild (#505)", 5 days before this task) already covers the one site where `force` is load-bearing. That work was not redone here.
+
+## What was already done, and the gap that remained
+
+Commit `69b0537c` (PR #505) added a comment at the live `force` site (`deps.ts`) and a test, "force behavior at the cache-rebuild path" (`tests/.../ensure-built-marketplace-use-case.integration.test.ts`), asserting that a real `FlatBuildStrategy` with `force: true` overwrites a colliding cache file instead of throwing `FlatTargetExistsError`. That test is a behavioural pin: it proves the collision-bypass mechanism itself works when `force` is `true`.
+
+It does not pin production wiring, and it does not pin the outDir invariant this task was asked for:
+
+- Its `realBuildFor` constructs `FlatBuildStrategy` with a literal `true` written directly in the test, independent of `deps.ts`. Flipping `deps.ts`'s real `force: true` to `false` does not fail this test (confirmed empirically, see Verification). The test exercises `FlatBuildStrategy`'s own logic, not the actual dependency wiring.
+- Nothing anywhere asserted that `outDir` stays under the build cache or temp dir. A change that pointed either call site in `runBuild` at a live user directory would not have been caught by any existing test, confirmed by pointing `build()`'s outDir at a directory outside the cache: the pre-existing behavioural test failed, but only incidentally, via `OutDirNotDirectoryError` from an unrelated preflight check in `FlatBuildStrategy.preBuild`, not from any assertion about the path itself.
+
+This is exactly the "naive test is nearly worthless" risk described in the task: a test that only asserts "collision doesn't throw" pins that `force` works, but says nothing about where it is allowed to apply.
+
+## What this task adds
+
+1. A comment on `runBuild` itself (`ensure-built-marketplace-use-case.ts`, above the method, not on the `force: true` literal), stating the outDir invariant: every `outDir` reaching this method is either `builtMarketplaceDir()` or a temp dir this class just cleared, never a user directory. Placed on the method rather than the (inert) `force: true` literal, since that literal has no causal role in the invariant.
+2. A new test, "outDir invariant for the cache-rebuild build path", in the same test file. It spies on the injected `buildFor` factory to capture every `outDir` passed to it across both call paths: the direct path (`build()`, non-nested source) and the temp-routed path (`buildViaTemp()`, nested/dogfood source). It asserts each captured value is either under `<projectRoot>/.aidd/cache/built` or under the OS temp dir, plus explicitly checks the dogfood call went through the temp dir. This is independent of the pre-existing behavioural test, which it references but does not duplicate.
+
+`tests/helpers/ports/build-unit-deps.ts` and `tests/helpers/ports/fake-ensure-built-marketplace.ts` were read before writing the test. Neither fits: `build-unit-deps.ts` wires a full unit-test dependency graph around `fakeEnsureBuiltMarketplace()`, a stand-in for the whole use-case under test here, not for the `FrameworkBuildUseCase`/`FrameworkBuildFor` this test needs to spy on. `fake-ensure-built-marketplace.ts` itself uses `as unknown as EnsureBuiltMarketplaceUseCase` for the same reason the new test casts its spy to `FrameworkBuildUseCase`: the class has `private readonly` constructor fields, so no structurally-typed object literal is assignable to it without a cast. The new test's cast follows the file's own pre-existing pattern (lines 74 and 102 of the same file, predating this change).
+
+No production behavior changed. The diff to `src/` is comment-only.
+
+## Verification (real numbers)
+
+1. `npx tsc --noEmit`: clean, no errors.
+2. `pnpm test` from `cli/`: 200 test files, 2153 tests passing, 0 failures (baseline was 2152; +1 for the new test). Confirmed the `src/` diff is comment-only via `git diff --stat -- src/` (3 insertions, 0 deletions, one file).
+3. Biome, one file at a time via the binary directly:
+   - `ensure-built-marketplace-use-case.ts`: "Checked 1 file in 36ms. No fixes applied."
+   - `ensure-built-marketplace-use-case.integration.test.ts`: "Checked 1 file in 9ms. No fixes applied."
+4. Mutation testing, all reverted after observation, suite re-confirmed green (2153/2153) afterward:
+   - (a), as specified by the ticket: flipped `force: true` to `force: false` at `ensure-built-marketplace-use-case.ts:151` (the site the ticket named). Result: no test failed, full suite (200 files / 2153 tests) still green. This is the empirical proof that this literal is inert; it does not reach `FlatBuildStrategy`.
+   - (a), at the actually load-bearing site: flipped `force: true` to `false` in the `frameworkBuildFor` closure in `deps.ts:458`. Result: no test failed either. The pre-existing "force behavior" test does not exercise this closure; it builds its own `FlatBuildStrategy` with a hardcoded `true`. Out of scope to fix here, noted as a gap rather than remediated, since the task scope is the outDir invariant in `ensure-built-marketplace-use-case.ts`.
+   - (b), direct path: changed `build()` (line 112) to pass `join(sourceDir, "..", ".claude")` instead of `builtDir`. Result: the new invariant test failed immediately with a clear `AssertionError` on the outDir check. The pre-existing behavioural test also failed, but only incidentally (`OutDirNotDirectoryError` from `FlatBuildStrategy.preBuild`'s directory-existence guard), not because it checks the path itself. Reverted; suite re-confirmed green.
+   - (b), temp path: changed `buildViaTemp()` (line 129) to compute `temp` as a path escaping the project tree instead of `join(tmpdir(), ...)`. Result: the new invariant test failed with the same `AssertionError`, this time on the dogfood/temp capture, while the pre-existing "force behavior" test (unrelated target/mode) stayed green, proving the new test is the only one covering this call site. Reverted; suite re-confirmed green.
+
+Mutation (b) is the one required by the task's definition of done ("fails if this stops being true"), and it holds for both call sites the invariant covers. Mutation (a) as literally specified does not break anything, because the named site is dead code. This is reported rather than silently "fixed" by moving the assertion to a site the ticket did not ask about, per the task's instruction to stop and report when a premise is wrong.
@@ -60,7 +60,7 @@ export function registerFrameworkCommand(program: Command): void {
             );
             process.exit(1);
           }
-          const result = await useCase.execute({ sourceDir, outDir, target, mode, force });
+          const result = await useCase.execute({ sourceDir, outDir, target, mode });
           if (mode === "flat") {
             output.success(
               `Flat-installed ${result.plugins.length} plugins, ${result.totalFiles} files written under ${result.outDir}`

@@ -134,6 +134,9 @@ export class EnsureBuiltMarketplaceUseCase {
     await this.fs.deleteDirectory(temp);
   }
 
+  // Every outDir reaching this method (see build() and buildViaTemp() above) is either
+  // builtMarketplaceDir() or a temp dir this class just deleteDirectory'd — an aidd-owned
+  // cache, never a user directory — so a collision here is stale-cache reuse, not data loss.
   private async runBuild(
     target: FrameworkBuildTarget,
     mode: FrameworkBuildMode,
@@ -145,7 +148,7 @@ export class EnsureBuiltMarketplaceUseCase {
     if (build === undefined) {
       throw new Error(`No framework build for target '${target}' mode '${mode}'.`);
     }
-    await build.execute({ sourceDir, outDir, target, mode, force: true });
+    await build.execute({ sourceDir, outDir, target, mode });
   }
 
   private async copyDir(from: string, to: string): Promise<void> {

@@ -39,8 +39,6 @@ export interface FrameworkBuildOptions {
   readonly target: FrameworkBuildTarget;
   /** Output layout. Defaults to "marketplace" (Mode A) when absent. */
   readonly mode?: FrameworkBuildMode;
-  /** When true, overwrite existing files at canonical flat paths. Only meaningful in flat mode. */
-  readonly force?: boolean;
 }
 
 export interface BuildPluginResult {

@@ -13,7 +13,7 @@ import type {
   ResolveMarketplaceUseCase,
 } from "../../../../src/application/use-cases/shared/resolve-marketplace-use-case.js";
 import { Marketplace } from "../../../../src/domain/models/marketplace.js";
-import { builtMarketplaceDir } from "../../../../src/domain/models/paths.js";
+import { BUILT_CACHE_SUBDIR, builtMarketplaceDir } from "../../../../src/domain/models/paths.js";
 import type { AssetProvider } from "../../../../src/domain/ports/asset-provider.js";
 import type { JsonSchemaValidator } from "../../../../src/domain/ports/json-schema-validator.js";
 import type { VersionReader } from "../../../../src/domain/ports/version-reader.js";
@@ -269,3 +269,66 @@ describe("force behavior at the cache-rebuild path", () => {
     expect(memFs.getFile(agentPath)).not.toBe("stale cache content from a previous build");
   });
 });
+
+// The "force behavior" suite above proves the collision-bypass fires. It does not prove
+// the bypass only ever fires against an aidd-owned directory — that guarantee lives in
+// which outDir runBuild() is called with, both for a direct build (build()) and a build
+// routed through a temp dir first (buildViaTemp(), used when the cache nests under the
+// source). This pins that outDir, whichever path is taken, never leaves the build cache
+// or the OS temp dir — so a future change that points either call site at a live user
+// directory (e.g. a tool's real config dir) fails here, not in someone's project.
+describe("outDir invariant for the cache-rebuild build path", () => {
+  it("only ever builds into the aidd build cache or the OS temp dir, never a user directory", async () => {
+    const memFs = new InMemoryFileAdapter();
+    const capturedOutDirs: string[] = [];
+    const capturingBuildFor: FrameworkBuildFor = (_target, _mode, outDir) => {
+      capturedOutDirs.push(outDir);
+      return {
+        execute: async () => {
+          await memFs.writeFile(join(outDir, "plugins/aidd-vcs/SKILL.md"), "built content");
+          return { outDir, plugins: [], totalFiles: 1 };
+        },
+      } as unknown as FrameworkBuildUseCase;
+    };
+
+    // Direct path: source lives outside the cache tree → build() writes straight to builtDir.
+    const direct = new EnsureBuiltMarketplaceUseCase(
+      memFs,
+      fakeResolve("/src/framework", "1.0.0"),
+      capturingBuildFor,
+      fakeVersion("5.0.0")
+    );
+    await direct.execute({
+      projectRoot: PROJECT,
+      marketplace: makeMarketplace(),
+      target: "codex",
+      mode: "marketplace",
+    });
+
+    // Dogfood path: source is the project root, so builtDir nests under it → buildViaTemp()
+    // routes the same call through a temp dir instead (see the "builds via a temp dir" test above).
+    const dogfood = new EnsureBuiltMarketplaceUseCase(
+      memFs,
+      fakeResolve(PROJECT, "1.0.0"),
+      capturingBuildFor,
+      fakeVersion("5.0.0")
+    );
+    await dogfood.execute({
+      projectRoot: PROJECT,
+      marketplace: makeMarketplace(),
+      target: "cursor",
+      mode: "marketplace",
+    });
+
+    expect(capturedOutDirs).toHaveLength(2);
+    const cacheRoot = join(PROJECT, BUILT_CACHE_SUBDIR);
+    const tmpRoot = tmpdir();
+    for (const outDir of capturedOutDirs) {
+      const underCache = outDir === cacheRoot || outDir.startsWith(`${cacheRoot}/`);
+      const underTmp = outDir === tmpRoot || outDir.startsWith(`${tmpRoot}/`);
+      expect(underCache || underTmp).toBe(true);
+    }
+    // The dogfood call specifically must have gone through the temp dir, not the cache.
+    expect(capturedOutDirs[1]?.startsWith(`${tmpRoot}/`)).toBe(true);
+  });
+});
@@ -0,0 +1,76 @@
+import { mkdtemp, rm } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join, resolve } from "node:path";
+import { afterEach, beforeEach, describe, expect, it } from "vitest";
+import { FlatTargetExistsError } from "../../src/domain/errors.js";
+import { BundledAssetProviderAdapter } from "../../src/infrastructure/assets/asset-loader.js";
+import {
+  createFrameworkBuildUseCase,
+  type FrameworkBuildDeps,
+} from "../../src/infrastructure/deps.js";
+import { CapturingLogger } from "../helpers/ports/capturing-logger.js";
+import { InMemoryFileAdapter } from "../helpers/ports/in-memory-file-adapter.js";
+import { seedFromDirectory } from "../helpers/ports/seed-from-directory.js";
+
+const FIXTURE_DIR = resolve(process.cwd(), "tests/fixtures/framework");
+// Canonical flat destination for the fixture's "aidd-test" plugin agent, under --target copilot.
+const COLLIDING_RELATIVE_PATH = ".github/agents/aidd-test-code-reviewer.agent.md";
+const STALE_CONTENT = "stale content from a previous build";
+
+function makeDeps(fs: InMemoryFileAdapter): FrameworkBuildDeps {
+  return {
+    fs,
+    assetProvider: new BundledAssetProviderAdapter(),
+    logger: new CapturingLogger(),
+  };
+}
+
+describe("createFrameworkBuildUseCase threads the caller's --force through to the flat build strategy", () => {
+  let outDir: string;
+  let fs: InMemoryFileAdapter;
+
+  beforeEach(async () => {
+    outDir = await mkdtemp(join(tmpdir(), "aidd-force-wiring-"));
+    fs = new InMemoryFileAdapter();
+    await seedFromDirectory(fs, FIXTURE_DIR, { useAbsolutePaths: true });
+    // outDir must exist for both the in-memory fs (fileExists) and the real disk
+    // (FlatBuildStrategy's isDirectory guard is backed by a real fs.stat call).
+    fs.setFile(`${outDir}/.keep`, "");
+    fs.setFile(`${outDir}/${COLLIDING_RELATIVE_PATH}`, STALE_CONTENT);
+  });
+
+  afterEach(async () => {
+    await rm(outDir, { recursive: true, force: true });
+  });
+
+  it("force: false throws FlatTargetExistsError when the canonical destination already exists", async () => {
+    const useCase = createFrameworkBuildUseCase(makeDeps(fs), {
+      target: "copilot",
+      mode: "flat",
+      outDir,
+      force: false,
+    });
+    if (useCase === undefined) throw new Error("copilot:flat must be wired in the registry");
+
+    await expect(
+      useCase.execute({ sourceDir: FIXTURE_DIR, outDir, target: "copilot" })
+    ).rejects.toBeInstanceOf(FlatTargetExistsError);
+    expect(fs.getFile(`${outDir}/${COLLIDING_RELATIVE_PATH}`)).toBe(STALE_CONTENT);
+  });
+
+  it("force: true overwrites the canonical destination instead of throwing", async () => {
+    const useCase = createFrameworkBuildUseCase(makeDeps(fs), {
+      target: "copilot",
+      mode: "flat",
+      outDir,
+      force: true,
+    });
+    if (useCase === undefined) throw new Error("copilot:flat must be wired in the registry");
+
+    await useCase.execute({ sourceDir: FIXTURE_DIR, outDir, target: "copilot" });
+
+    const written = fs.getFile(`${outDir}/${COLLIDING_RELATIVE_PATH}`);
+    expect(written).not.toBe(STALE_CONTENT);
+    expect(written).toContain("aidd-test-code-reviewer");
+  });
+});