From 5a3ec7c718cdbbcacb258cb181959bdbf382e487 Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Wed, 29 Jul 2026 09:48:57 +0200 Subject: [PATCH 1/2] fix(cli): stop update and restore materializing marketplace plugins claude, codex and copilot set translationMode "marketplace", so resolveTranslator hands them ModeAMarketplaceTranslator, whose contract is to register a plugin reference and write nothing: its addPlugin records an empty files set. Update and restore did not honour that. Both narrowed the resolved translator with `instanceof BuiltTreeMaterializationTranslator` and, for anything else, fell through to a raw materializing path. So a plugin that install deliberately left unmaterialized got its files written to disk and recorded in the manifest the first time the user ran update or restore, leaving it both natively registered and materialized. Update's `isLocalMarketplace` guard looked like it covered this but did not: a github-hosted marketplace resolves plugin sources to kind "git-subdir", never "local", so the guard never fired for the case that matters. Restore had no guard at all. Both now dispatch on what resolvePluginTranslator returns for the tool rather than on the translator's concrete class, so a tool's translation mode decides the behaviour in one place. materializeViaBuiltTree widens to materializeViaTranslator, taking the PluginTranslator interface, and both call sites share it rather than repeating the logic. Install was already correct and is unchanged. cursor and opencode cannot reach this path: installScope "user" and translationMode "flat" are matched by earlier branches in resolveTranslator, so they only ever get BuiltTreeMaterializationTranslator or null. A flat mode regression test covers it. 2150/2150 pass (2146 plus 4), tsc clean, golden baseline unchanged. The new tests were verified against the pre-fix code: 3 of 4 fail there with the predicted symptom, the flat-mode guard passes in both states. Known gap, not fixed here: a user who already ran update or restore has stray files on disk. Update self-heals when a newer version exists, because it deletes the old manifest files first, but it never runs when no version bump is available, and restore has no delete step at all so it leaves the files orphaned once the manifest entry goes empty. Adding a symmetric delete to restore would fix it and would also delete files on disk, so it needs a decision rather than a silent implementation. --- .../plan.md | 102 +++++++++++ .../use-cases/plugin/plugin-helpers.ts | 19 +- .../plugin/plugin-update-use-case.ts | 32 ++-- .../plugin/translator/plugin-translator.ts | 5 +- .../shared/apply-plugin-files-use-case.ts | 25 +-- ...gin-update-mode-a-marketplace.unit.test.ts | 162 ++++++++++++++++++ ...ugin-files-mode-a-marketplace.unit.test.ts | 118 +++++++++++++ 7 files changed, 422 insertions(+), 41 deletions(-) create mode 100644 cli/aidd_docs/tasks/2026_07/2026_07_29_marketplace-mode-materialization/plan.md create mode 100644 cli/tests/application/use-cases/plugin/plugin-update-mode-a-marketplace.unit.test.ts create mode 100644 cli/tests/application/use-cases/shared/apply-plugin-files-mode-a-marketplace.unit.test.ts diff --git a/cli/aidd_docs/tasks/2026_07/2026_07_29_marketplace-mode-materialization/plan.md b/cli/aidd_docs/tasks/2026_07/2026_07_29_marketplace-mode-materialization/plan.md new file mode 100644 index 000000000..3f51474aa --- /dev/null +++ b/cli/aidd_docs/tasks/2026_07/2026_07_29_marketplace-mode-materialization/plan.md @@ -0,0 +1,102 @@ +--- +objective: Stop plugin update and restore from materializing files on disk for Mode A marketplace tools (claude/codex/copilot), whose whole contract is register-only, no materialization. +status: implemented +--- + +## Baseline + +Confirmed 2146 passing tests before touching anything (`pnpm test` from `cli/`, 198 files). + +## What was verified before fixing + +Every claim in the finding was checked against the code, not assumed: + +- `translationMode: "marketplace"` is set on exactly three tools: `src/domain/tools/ai/claude.ts:110`, `codex.ts:235`, `copilot.ts:325`. +- `resolveTranslator` (`plugin-translator-factory.ts:44-45`) routes any `PluginsCapability` with `translationMode === "marketplace"` to `new ModeAMarketplaceTranslator()`. +- `ModeAMarketplaceTranslator.addPlugin` (`mode-a-marketplace-translator.ts:32`) calls `manifest.addPlugin(toolId, Plugin.fromDistribution(dist, source, [], new Map(), marketplace))` — an empty files array, unconditionally. Its docstring: "Plugin files are NOT materialized on disk ... the marketplace sync handles the rest." +- `PluginUpdateUseCase.replacePluginFiles` kept the resolved translator only when `translator instanceof BuiltTreeMaterializationTranslator` (used by cursor/opencode). For claude/codex/copilot the resolved translator is `ModeAMarketplaceTranslator`, which fails that check, so the code fell through to a raw `PluginContentTranslator` + `writePluginFiles` path guarded by `isLocalMarketplace = plugin.source.kind === "local" && plugin.marketplace !== undefined`. +- Traced how a github-sourced marketplace plugin actually gets its `source.kind`: `resolvePluginSourceFromMarketplace` (`plugin-source-resolver.ts`) turns a catalog entry into `{ kind: "git-subdir", ... }` whenever the marketplace itself is `kind: "github"`. So a plugin installed from a github-hosted marketplace has `source.kind === "git-subdir"`, never `"local"`. `isLocalMarketplace` is therefore false for exactly the case that matters, and update wrote files that install never wrote, recording them in the manifest. +- `ApplyPluginFilesUseCase` (restore) has the identical `instanceof BuiltTreeMaterializationTranslator` gate, but its fallback (`restoreViaTranslate`) has **no** `isLocalMarketplace` guard at all — it unconditionally writes translated files and updates the manifest with them for every plugin that doesn't hit the built-tree branch. Restore had the same defect, unconditionally, for every marketplace-mode plugin regardless of source kind. +- Cross-checked against install (`PluginAddUseCase`): a plugin catalogued in a `kind: "github"` marketplace goes through `addGithubMarketplacePlugin`, which bypasses `PluginContentTranslator` entirely and calls `manifest.addPlugin(toolId, Plugin.fromMetadata(...))` with no files — confirming install already gets this right and update/restore were the only broken paths. + +All claims held; nothing in the finding was wrong. + +## The fix + +`resolvePluginTranslator(toolConfig, deps)` already returns the correct strategy for a tool — `BuiltTreeMaterializationTranslator` for cursor/opencode, `ModeAMarketplaceTranslator` for claude/codex/copilot, or `null` for plain native tools. The bug was that both call sites narrowed that result with `instanceof BuiltTreeMaterializationTranslator`, silently discarding the Mode A case and falling back to a generic materializing path with an ad-hoc `source.kind === "local"` special case bolted on. + +The fix removes the `instanceof` narrowing and the `isLocalMarketplace` special case entirely. Both `PluginUpdateUseCase.replacePluginFiles` and `ApplyPluginFilesUseCase.execute` now do: + +``` +const translator = this.resolveTranslator(toolConfig); // PluginTranslator | null, no instanceof filter +if (translator !== null && plugin.marketplace !== undefined) { + return materializeViaTranslator(translator, dist, toolId, plugin, projectRoot, manifest, docsDir); +} +// else: plain native materialization (unchanged raw-translate path) +``` + +`materializeViaBuiltTree` (in `plugin-helpers.ts`) was generalized to `materializeViaTranslator`, widening its parameter type from `BuiltTreeMaterializationTranslator` to the `PluginTranslator` interface. Its body was already translator-agnostic (`manifest.removePlugin` then `translator.addPlugin`, reporting `written ?? 0`), so no logic changed — only the type. This is why the seam is a widening, not a new branch: `PluginTranslator.addPlugin` is a uniform contract (both `BuiltTreeMaterializationTranslator` and `ModeAMarketplaceTranslator` implement it), and the return type gained an optional `written?: number` field so the shared helper compiles against either implementation (`ModeAMarketplaceTranslator.addPlugin` never sets it, so `written ?? 0` correctly reports zero files written for Mode A). + +No `if (toolId === "claude")`-style branching was added anywhere. The dispatch is entirely driven by what `resolvePluginTranslator` returns for that tool's `PluginsCapability`, exactly as install already does it. + +### Why `plugin.marketplace !== undefined` stays as a guard + +This condition already existed before the fix and was kept unchanged. It matters for both translator kinds: `BuiltTreeMaterializationTranslator.addPlugin` has its own internal fallback when `marketplace === undefined` (delegates to `ModeBFlatMaterializationTranslator`), and install's own Mode A branch (`plugin-add-use-case.ts` line 278) only calls the adapter when `marketplace !== undefined` too — a plugin added directly from a local path (no marketplace) on a marketplace-mode tool is deliberately materialized via the generic writePluginFiles path, matching what install does for that same case. The fix preserves this exactly; the condition text is identical to what it was, just no longer combined with the `instanceof` check. + +## Migration: what happens to already-materialized files + +A user who ran `plugin update` (or `restore`) against the pre-fix code on a github-sourced marketplace plugin now has stray files on disk under `.claude/plugins//...` (or the codex/copilot equivalent) plus a manifest entry recording them. + +Behavior after deploying the fix, verified by reading the final code (not assumed): + +- **`plugin update`, when a newer plugin version exists upstream**: self-heals. `replacePluginFiles` unconditionally calls `deleteOldFiles(plugin.files, baseDir)` *before* branching on the translator — using the stale manifest entry's (buggy, non-empty) `files` map. This deletes every stray file, then the Mode A branch re-registers the plugin with an empty files list. No orphans remain. +- **`plugin update`, when no newer version exists**: `updateOnePlugin` returns early (`compareSemver(...) <= 0`) without calling `replacePluginFiles` at all. The stray files and the wrong manifest entry are left untouched indefinitely — update never even looks at this plugin again until a new version ships. +- **`plugin restore`**: does **not** self-heal. `ApplyPluginFilesUseCase` has no equivalent `deleteOldFiles` step anywhere in the file — `restoreViaTranslate` only ever writes/overwrites files it currently computes, never deletes files absent from that set. After the fix, restore re-registers the manifest entry as empty via the Mode A branch, but the previously-materialized files stay on disk, now **orphaned and untracked** (previously they were at least recorded, if wrongly; after restore they're invisible to the manifest entirely). + +This is not implemented as a silent cleanup, per the task's explicit instruction not to delete user files without a decision. Proposed option: add a symmetric `deleteOldFiles`-equivalent step to the Mode A branch of `ApplyPluginFilesUseCase` (delete `plugin.files` under the tool's resolved base dir before calling `materializeViaTranslator`), mirroring what `PluginUpdateUseCase` already does unconditionally. That would make restore self-heal the same way update does when a version bump triggers it, and would also cover the "no version bump" update gap since restore doesn't have a version-gate. This needs an explicit decision because it deletes files the tool previously wrote to the user's project — flagging for follow-up rather than implementing here. + +## Tests + +Characterization tests were run against the pre-fix code first (via `git stash` of the four production-code changes), confirmed to fail with the exact described symptom, then the fix was restored and the tests confirmed green — so the diff demonstrably flips the behavior: + +- `tests/application/use-cases/plugin/plugin-update-mode-a-marketplace.unit.test.ts`: a github-sourced (`git-subdir`) marketplace plugin on claude, updated via `PluginUpdateUseCase`. Asserts zero files under `.claude/plugins/` and an empty `plugin.files` map after update, plus a single non-duplicated manifest entry. A third test in the same file installs the same plugin on **opencode** (flat-mode) and asserts it still re-materializes from the built tree on update — the regression guard for the shared code path. +- `tests/application/use-cases/shared/apply-plugin-files-mode-a-marketplace.unit.test.ts`: same github-sourced plugin on claude, restored via `RestoreAllPluginsUseCase`. Asserts `result.totalFiles === 0`, no files under `.claude/plugins/`, and a single manifest entry with empty files. + +Pre-fix run of these two files: 3 of 4 tests failed with "expected 6 to be 0" (6 = files the fixture plugin contains) and one "expected undefined to be 'aidd-framework'" (duplicate-registration path threw before reaching that assertion cleanly); the opencode regression test passed both before and after, as expected since it exercises a code path the fix does not touch. + +Existing coverage left untouched and still green, providing independent regression evidence: +- `tests/application/use-cases/plugin/plugin-update-built-tree.unit.test.ts` (cursor update via built tree) +- `tests/application/use-cases/shared/apply-plugin-files-built-tree.unit.test.ts` (cursor restore via built tree) +- `tests/application/use-cases/plugin/translator/built-tree-opencode-materialization.integration.test.ts` (opencode translator-level) +- `tests/application/use-cases/plugin/translator/install-plugin-{claude,codex,copilot}-mode-a.integration.test.ts` (install-side Mode A behavior, unchanged) + +## Checked fact: cursor/opencode/flat-mode tools are unaffected + +Read directly from the final code, not inferred: + +- `PluginsCapability.resolveTranslationMode` (`plugins-capability.ts:175-179`) hardcodes `translationMode = "flat"` whenever `params.mode === "flat"` (opencode), and native tools only get `"marketplace"` when explicitly configured with it (claude/codex/copilot). Cursor uses `mode: "native"` with `installScope: "user"` and no `translationMode` set, so its `translationMode` resolves to `null`. +- `resolveTranslator` (`plugin-translator-factory.ts:35`) routes `installScope === "user"` (cursor) or `translationMode === "flat"` (opencode) to `BuiltTreeMaterializationTranslator`, and only checks `translationMode === "marketplace"` afterward. Structurally, for cursor and opencode, `resolveTranslator` can only ever return `BuiltTreeMaterializationTranslator` or (if `builtDeps` is unset) `null` — it can never return `ModeAMarketplaceTranslator`, because the `translationMode === "flat"` / `installScope === "user"` branch returns first. +- Therefore `translator !== null && plugin.marketplace !== undefined` (the new condition) is provably equivalent to the old `translator instanceof BuiltTreeMaterializationTranslator && plugin.marketplace !== undefined` for these two tools specifically, since `translator` can never be anything other than `BuiltTreeMaterializationTranslator` or `null` for them. No behavior change is possible for cursor/opencode from this fix. + +## Verification (real numbers) + +1. `npx tsc --noEmit` — clean, no errors. +2. `pnpm test` from `cli/` — 200 test files, **2150 tests passed**, zero failures. Baseline was 2146; this task added 4 new characterization/fix tests (3 in `plugin-update-mode-a-marketplace.unit.test.ts`, 1 in `apply-plugin-files-mode-a-marketplace.unit.test.ts`), for 2146 + 4 = 2150. No existing assertion was modified. +3. Biome, one file at a time via `./node_modules/.bin/biome check --write `: + - `src/application/use-cases/plugin/plugin-helpers.ts` — no fixes applied. + - `src/application/use-cases/plugin/plugin-update-use-case.ts` — fixed formatting once (line wrap on a multi-arg call), second run: no fixes applied. + - `src/application/use-cases/shared/apply-plugin-files-use-case.ts` — fixed formatting once (same kind of wrap), second run: no fixes applied. + - `src/application/use-cases/plugin/translator/plugin-translator.ts` — no fixes applied. + - `tests/application/use-cases/plugin/plugin-update-mode-a-marketplace.unit.test.ts` — no fixes applied. + - `tests/application/use-cases/shared/apply-plugin-files-mode-a-marketplace.unit.test.ts` — no fixes applied. +4. Mutation test: `git stash` of the four production-code changes (reverting to the exact pre-fix state) while keeping the two new test files. Ran the two new test files: 3 of 4 tests failed, each with the precise symptom the finding predicted (files materialized where none should be: "expected 6 to be 0"; a duplicate-registration assertion failing because the buggy path never emptied `files`). The opencode regression test in the same file passed unaffected, confirming it doesn't accidentally depend on the fix. `git stash pop` restored the fix; re-ran `npx tsc --noEmit` (clean) and `pnpm test` (2150/2150 green again). +5. Golden baseline: `tests/golden/golden-baseline.e2e.test.ts` ran as part of the full suite and passed unchanged (2/2), meaning its command matrix doesn't currently exercise a github-sourced marketplace plugin update/restore. No `UPDATE_GOLDEN=1` regeneration was needed or performed. + +## Files changed + +- `src/application/use-cases/plugin/plugin-helpers.ts` — `materializeViaBuiltTree` → `materializeViaTranslator`, widened to `PluginTranslator`. +- `src/application/use-cases/plugin/plugin-update-use-case.ts` — `replacePluginFiles` routes through the resolved translator generically; `isLocalMarketplace` special case removed; `builtTreeTranslator` → `resolveTranslator`. +- `src/application/use-cases/shared/apply-plugin-files-use-case.ts` — same shape of change for restore; `builtTreeTranslator` → `resolveTranslator`; `restoreViaBuiltTree` → `restoreViaTranslator`. +- `src/application/use-cases/plugin/translator/plugin-translator.ts` — `addPlugin`'s return type gained an optional `written?: number` so the shared helper can report it uniformly across strategies. +- `tests/application/use-cases/plugin/plugin-update-mode-a-marketplace.unit.test.ts` — new. +- `tests/application/use-cases/shared/apply-plugin-files-mode-a-marketplace.unit.test.ts` — new. diff --git a/cli/src/application/use-cases/plugin/plugin-helpers.ts b/cli/src/application/use-cases/plugin/plugin-helpers.ts index f23aaeec5..01096b863 100644 --- a/cli/src/application/use-cases/plugin/plugin-helpers.ts +++ b/cli/src/application/use-cases/plugin/plugin-helpers.ts @@ -13,7 +13,7 @@ import type { Hasher } from "../../../domain/ports/hasher.js"; import type { ManifestRepository } from "../../../domain/ports/manifest-repository.js"; import { getToolConfig, isAiTool } from "../../../domain/tools/registry.js"; import { NoManifestError } from "../../errors.js"; -import type { BuiltTreeMaterializationTranslator } from "./translator/built-tree-materialization-translator.js"; +import type { PluginTranslator } from "./translator/plugin-translator.js"; export function resolvePluginToolIds(toolIds: AiToolId[] | "all", manifest: Manifest): AiToolId[] { if (toolIds !== "all") return toolIds; @@ -79,18 +79,21 @@ export async function isPluginFileAtDesiredState( } /** - * Re-registers a plugin backed by the BUILT tree: drops the existing manifest entry - * and lets the translator re-materialize + re-register it, so update and restore both - * end up with the same single entry an install would have produced. + * Re-registers a marketplace-sourced plugin through its resolved translator: drops the + * existing manifest entry and lets the translator re-add it, so update and restore both + * end up with the same single entry an install would have produced. Works for either + * translation strategy — materializing tools (cursor/opencode) re-copy the BUILT tree; + * Mode A marketplace tools (claude/codex/copilot) register the plugin reference without + * writing any files, matching what install does for them. * * Returns how many files the translator actually (re)wrote — not the plugin's total * file count — so a no-op restore reports zero instead of claiming everything changed. - * `written` is undefined only on the rare fallback path where the translator can't - * resolve the marketplace and delegates to a strategy that doesn't track counts; that + * `written` is undefined for translators that don't track counts (Mode A never writes + * files; the rare built-tree fallback where the marketplace can't be resolved); that * is reported as 0 rather than guessed. */ -export async function materializeViaBuiltTree( - translator: BuiltTreeMaterializationTranslator, +export async function materializeViaTranslator( + translator: PluginTranslator, dist: PluginDistribution, toolId: AiToolId, plugin: Plugin, diff --git a/cli/src/application/use-cases/plugin/plugin-update-use-case.ts b/cli/src/application/use-cases/plugin/plugin-update-use-case.ts index 756a44d65..0021f5fe2 100644 --- a/cli/src/application/use-cases/plugin/plugin-update-use-case.ts +++ b/cli/src/application/use-cases/plugin/plugin-update-use-case.ts @@ -17,12 +17,12 @@ import { getToolConfig, type ToolConfig } from "../../../domain/tools/registry.j import type { BuiltMaterializationDeps } from "../shared/apply-plugin-files-use-case.js"; import { loadPluginManifest, - materializeViaBuiltTree, + materializeViaTranslator, resolvePluginBaseDir, resolvePluginToolIds, writePluginFiles, } from "./plugin-helpers.js"; -import { BuiltTreeMaterializationTranslator } from "./translator/built-tree-materialization-translator.js"; +import type { PluginTranslator } from "./translator/plugin-translator.js"; import { resolvePluginTranslator } from "./translator/resolve-plugin-translator.js"; export interface PluginUpdateOptions { @@ -118,10 +118,10 @@ export class PluginUpdateUseCase { const baseDir = resolvePluginBaseDir(toolId, projectRoot, nodeHomedir); await this.deleteOldFiles(plugin.files, baseDir); const toolConfig = getToolConfig(toolId); - const builtTree = this.builtTreeTranslator(toolConfig); - if (builtTree !== null && plugin.marketplace !== undefined) { - await materializeViaBuiltTree( - builtTree, + const translator = this.resolveTranslator(toolConfig); + if (translator !== null && plugin.marketplace !== undefined) { + await materializeViaTranslator( + translator, dist, toolId, plugin, @@ -134,16 +134,10 @@ export class PluginUpdateUseCase { const { files: newFiles, componentPaths } = new PluginContentTranslator( this.hasher ).translateWithComponentPaths(dist, toolConfig, docsDir); - const isLocalMarketplace = plugin.source.kind === "local" && plugin.marketplace !== undefined; - if (!isLocalMarketplace) await writePluginFiles(newFiles, baseDir, this.fs); + await writePluginFiles(newFiles, baseDir, this.fs); manifest.updatePlugin( toolId, - Plugin.fromDistribution( - dist, - plugin.source, - isLocalMarketplace ? [] : newFiles, - isLocalMarketplace ? new Map() : componentPaths - ) + Plugin.fromDistribution(dist, plugin.source, newFiles, componentPaths) ); } @@ -153,17 +147,17 @@ export class PluginUpdateUseCase { } } - // Materializing tools (cursor/opencode) re-materialize from the BUILT tree so an - // update writes the same content install did — not the raw source transform. - private builtTreeTranslator(toolConfig: ToolConfig): BuiltTreeMaterializationTranslator | null { + // Materializing tools (cursor/opencode) re-materialize from the BUILT tree, and Mode A + // marketplace tools (claude/codex/copilot) re-register without writing files, so an + // update matches whatever install would have done for that tool. + private resolveTranslator(toolConfig: ToolConfig): PluginTranslator | null { if (this.builtDeps === undefined) return null; - const translator = resolvePluginTranslator(toolConfig, { + return resolvePluginTranslator(toolConfig, { fs: this.fs, hasher: this.hasher, homedir: this.builtDeps.homedir, ensureBuilt: this.builtDeps.ensureBuilt, marketplaceRegistry: this.builtDeps.marketplaceRegistry, }); - return translator instanceof BuiltTreeMaterializationTranslator ? translator : null; } } diff --git a/cli/src/application/use-cases/plugin/translator/plugin-translator.ts b/cli/src/application/use-cases/plugin/translator/plugin-translator.ts index ec2e3474a..c53364b79 100644 --- a/cli/src/application/use-cases/plugin/translator/plugin-translator.ts +++ b/cli/src/application/use-cases/plugin/translator/plugin-translator.ts @@ -20,7 +20,8 @@ export interface PluginTranslator { * Add a plugin for a specific tool, writing files and/or registering the plugin reference * in the manifest according to this adapter's strategy. * - * Returns a skip list — non-empty when the plugin contains components the tool cannot consume. + * Returns a skip list — non-empty when the plugin contains components the tool cannot consume — + * and, for strategies that track it, how many files were actually (re)written to disk. * * `previousMcpEntries` — pass the plugin's previous mcpEntries when replacing an existing * plugin install (--replace path). Used for idempotent re-merge of OpenCode MCP servers. @@ -34,5 +35,5 @@ export interface PluginTranslator { marketplace: string | undefined, docsDir: string, previousMcpEntries?: ReadonlyMap - ): Promise<{ skipped: ReadonlySkipList }>; + ): Promise<{ skipped: ReadonlySkipList; written?: number }>; } diff --git a/cli/src/application/use-cases/shared/apply-plugin-files-use-case.ts b/cli/src/application/use-cases/shared/apply-plugin-files-use-case.ts index 3ce7b9aee..fa7d31adc 100644 --- a/cli/src/application/use-cases/shared/apply-plugin-files-use-case.ts +++ b/cli/src/application/use-cases/shared/apply-plugin-files-use-case.ts @@ -11,8 +11,8 @@ import type { MarketplaceRegistry } from "../../../domain/ports/marketplace-regi import type { PluginDistributionReader } from "../../../domain/ports/plugin-distribution-reader.js"; import type { PluginFetcher } from "../../../domain/ports/plugin-fetcher.js"; import type { ToolConfig } from "../../../domain/tools/registry.js"; -import { isPluginFileAtDesiredState, materializeViaBuiltTree } from "../plugin/plugin-helpers.js"; -import { BuiltTreeMaterializationTranslator } from "../plugin/translator/built-tree-materialization-translator.js"; +import { isPluginFileAtDesiredState, materializeViaTranslator } from "../plugin/plugin-helpers.js"; +import type { PluginTranslator } from "../plugin/translator/plugin-translator.js"; import { resolvePluginTranslator } from "../plugin/translator/resolve-plugin-translator.js"; import type { EnsureBuiltMarketplaceUseCase } from "./ensure-built-marketplace-use-case.js"; @@ -46,34 +46,35 @@ export class ApplyPluginFilesUseCase { async execute(options: ApplyPluginFilesOptions): Promise { const localPath = await this.pluginFetcher.fetch(options.plugin.source, options.cacheDir); const dist = await this.pluginDistributionReader.read(localPath); - const builtTree = this.builtTreeTranslator(options.toolConfig); - if (builtTree !== null && options.plugin.marketplace !== undefined) { - return this.restoreViaBuiltTree(builtTree, dist, options); + const translator = this.resolveTranslator(options.toolConfig); + if (translator !== null && options.plugin.marketplace !== undefined) { + return this.restoreViaTranslator(translator, dist, options); } return this.restoreViaTranslate(dist, options); } // Materializing tools (cursor/opencode) must re-materialize from the BUILT tree so - // restored content + hashes match what install wrote — not the raw source transform. - private builtTreeTranslator(toolConfig: ToolConfig): BuiltTreeMaterializationTranslator | null { + // restored content + hashes match what install wrote, and Mode A marketplace tools + // (claude/codex/copilot) must re-register without writing files — not the raw source + // transform in either case. + private resolveTranslator(toolConfig: ToolConfig): PluginTranslator | null { if (this.builtDeps === undefined) return null; - const translator = resolvePluginTranslator(toolConfig, { + return resolvePluginTranslator(toolConfig, { fs: this.fs, hasher: this.hasher, homedir: this.builtDeps.homedir, ensureBuilt: this.builtDeps.ensureBuilt, marketplaceRegistry: this.builtDeps.marketplaceRegistry, }); - return translator instanceof BuiltTreeMaterializationTranslator ? translator : null; } - private async restoreViaBuiltTree( - translator: BuiltTreeMaterializationTranslator, + private async restoreViaTranslator( + translator: PluginTranslator, dist: PluginDistribution, options: ApplyPluginFilesOptions ): Promise { const { toolId, plugin, projectRoot, manifest, docsDir } = options; - return materializeViaBuiltTree( + return materializeViaTranslator( translator, dist, toolId, diff --git a/cli/tests/application/use-cases/plugin/plugin-update-mode-a-marketplace.unit.test.ts b/cli/tests/application/use-cases/plugin/plugin-update-mode-a-marketplace.unit.test.ts new file mode 100644 index 000000000..a9943cd48 --- /dev/null +++ b/cli/tests/application/use-cases/plugin/plugin-update-mode-a-marketplace.unit.test.ts @@ -0,0 +1,162 @@ +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; +import { PluginAddUseCase } from "../../../../src/application/use-cases/plugin/plugin-add-use-case.js"; +import { PluginUpdateUseCase } from "../../../../src/application/use-cases/plugin/plugin-update-use-case.js"; +import { Marketplace } from "../../../../src/domain/models/marketplace.js"; +import { PluginDistributionReaderAdapter } from "../../../../src/infrastructure/adapters/plugin-distribution-reader-adapter.js"; +import { buildUnitDeps, initAndInstall } from "../../../helpers/ports/build-unit-deps.js"; +import { fakeEnsureBuiltMarketplace } from "../../../helpers/ports/fake-ensure-built-marketplace.js"; +import { InMemoryMarketplaceRegistry } from "../../../helpers/ports/in-memory-marketplace-registry.js"; +import { seedFromDirectory } from "../../../helpers/ports/seed-from-directory.js"; + +const PLUGIN_FIXTURE = join(process.cwd(), "tests/fixtures/plugins/claude-format/sample-plugin"); +const PROJECT_ROOT = "/test-project"; +const HOME = "/home/u"; +const BUILT_OPENCODE_SKILL = "/built/opencode/.opencode/skills/sample-plugin-demo/SKILL.md"; + +// A plugin whose catalog entry resolves to a github-hosted source (git-subdir), the shape +// PluginInstallFromMarketplaceUseCase produces for any plugin catalogued in a github marketplace. +const GIT_SUBDIR_SOURCE = { + kind: "git-subdir" as const, + url: "https://github.com/ai-driven-dev/framework.git", + path: "plugins/sample-plugin", +}; + +const PLUGIN_METADATA = { name: "sample-plugin", version: "1.0.0", strict: false }; + +type Deps = Awaited>; + +async function makeGithubRegistry(): Promise { + const registry = new InMemoryMarketplaceRegistry(); + await registry.save( + PROJECT_ROOT, + Marketplace.create({ + name: "aidd-framework", + source: { kind: "github", repo: "ai-driven-dev/framework" }, + scope: "project", + addedAt: "2026-05-01T00:00:00.000Z", + }) + ); + return registry; +} + +function makeUpdateUseCase(deps: Deps, registry: InMemoryMarketplaceRegistry): PluginUpdateUseCase { + return new PluginUpdateUseCase( + deps.fs, + deps.manifestRepo, + deps.pluginFetcher, + new PluginDistributionReaderAdapter(deps.fs), + deps.hasher, + { + ensureBuilt: fakeEnsureBuiltMarketplace(), + marketplaceRegistry: registry, + homedir: () => HOME, + } + ); +} + +/** Installs a github-sourced marketplace plugin on `toolId`, then lowers its recorded + * version so update sees it as stale and re-fetches. */ +async function installStaleGithubPlugin( + deps: Deps, + registry: InMemoryMarketplaceRegistry, + toolId: "claude" | "codex" | "copilot" | "opencode" +): Promise { + await seedFromDirectory(deps.fs, PLUGIN_FIXTURE, { useAbsolutePaths: true }); + deps.pluginFetcher.register(GIT_SUBDIR_SOURCE, PLUGIN_FIXTURE); + + await new PluginAddUseCase( + deps.fs, + deps.manifestRepo, + deps.pluginFetcher, + new PluginDistributionReaderAdapter(deps.fs), + deps.hasher, + deps.logger, + registry, + fakeEnsureBuiltMarketplace() + ).execute({ + source: GIT_SUBDIR_SOURCE, + toolIds: [toolId], + projectRoot: PROJECT_ROOT, + marketplace: "aidd-framework", + interactive: false, + pluginMetadata: PLUGIN_METADATA, + }); + + const manifest = await deps.manifestRepo.load(); + if (manifest === null) throw new Error("manifest not found"); + const plugin = manifest.getPlugins(toolId).find((p) => p.name === "sample-plugin"); + if (plugin === undefined) throw new Error("plugin not found"); + manifest.updatePlugin(toolId, plugin.withVersion("0.0.1")); + await deps.manifestRepo.save(manifest); +} + +describe("PluginUpdateUseCase — Mode A marketplace tools (claude/codex/copilot)", () => { + it("updates a github-sourced marketplace plugin on claude without materializing any files", async () => { + const deps = await buildUnitDeps(PROJECT_ROOT); + await initAndInstall(deps, PROJECT_ROOT, "claude"); + const registry = await makeGithubRegistry(); + await installStaleGithubPlugin(deps, registry, "claude"); + + const beforeManifest = await deps.manifestRepo.load(); + const before = beforeManifest?.getPlugins("claude").find((p) => p.name === "sample-plugin"); + expect(before?.files.size).toBe(0); + expect(deps.fs.listAll().some((p) => p.includes(".claude/plugins/"))).toBe(false); + + const updated = await makeUpdateUseCase(deps, registry).execute({ + toolIds: ["claude"], + projectRoot: PROJECT_ROOT, + }); + + expect(updated).toContain("sample-plugin"); + const manifest = await deps.manifestRepo.load(); + const plugin = manifest?.getPlugins("claude").find((p) => p.name === "sample-plugin"); + expect(plugin?.version).toBe("1.0.0"); + // Mode A's whole contract: register the reference, materialize nothing. Update must + // not write plugin files that install never wrote, nor record them in the manifest. + expect(plugin?.files.size).toBe(0); + expect(deps.fs.listAll().some((p) => p.includes(".claude/plugins/"))).toBe(false); + }); + + it("keeps the manifest's single entry for the plugin after update (no duplicate registration)", async () => { + const deps = await buildUnitDeps(PROJECT_ROOT); + await initAndInstall(deps, PROJECT_ROOT, "claude"); + const registry = await makeGithubRegistry(); + await installStaleGithubPlugin(deps, registry, "claude"); + + await makeUpdateUseCase(deps, registry).execute({ + toolIds: ["claude"], + projectRoot: PROJECT_ROOT, + }); + + const manifest = await deps.manifestRepo.load(); + const plugins = manifest?.getPlugins("claude").filter((p) => p.name === "sample-plugin") ?? []; + expect(plugins).toHaveLength(1); + expect(plugins[0]?.marketplace).toBe("aidd-framework"); + expect(plugins[0]?.source.kind).toBe("git-subdir"); + }); +}); + +describe("PluginUpdateUseCase — flat-mode tools are unaffected (regression guard)", () => { + it("still re-materializes opencode's flat files from the built tree on update", async () => { + const deps = await buildUnitDeps(PROJECT_ROOT); + await initAndInstall(deps, PROJECT_ROOT, "opencode"); + const registry = await makeGithubRegistry(); + await installStaleGithubPlugin(deps, registry, "opencode"); + deps.fs.setFile(BUILT_OPENCODE_SKILL, "# Demo skill v2"); + + const updated = await makeUpdateUseCase(deps, registry).execute({ + toolIds: ["opencode"], + projectRoot: PROJECT_ROOT, + }); + + expect(updated).toContain("sample-plugin"); + const manifest = await deps.manifestRepo.load(); + const plugin = manifest?.getPlugins("opencode").find((p) => p.name === "sample-plugin"); + expect(plugin?.version).toBe("1.0.0"); + expect(plugin?.files.size).toBeGreaterThan(0); + expect( + deps.fs.getFile(join(PROJECT_ROOT, ".opencode/skills/sample-plugin-demo/SKILL.md")) + ).toBe("# Demo skill v2"); + }); +}); diff --git a/cli/tests/application/use-cases/shared/apply-plugin-files-mode-a-marketplace.unit.test.ts b/cli/tests/application/use-cases/shared/apply-plugin-files-mode-a-marketplace.unit.test.ts new file mode 100644 index 000000000..8100a1f3b --- /dev/null +++ b/cli/tests/application/use-cases/shared/apply-plugin-files-mode-a-marketplace.unit.test.ts @@ -0,0 +1,118 @@ +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; +import { PluginAddUseCase } from "../../../../src/application/use-cases/plugin/plugin-add-use-case.js"; +import { RestoreAllPluginsUseCase } from "../../../../src/application/use-cases/restore/restore-all-plugins-use-case.js"; +import { Marketplace } from "../../../../src/domain/models/marketplace.js"; +import { DOCS_DIR } from "../../../../src/domain/models/paths.js"; +import { PluginDistributionReaderAdapter } from "../../../../src/infrastructure/adapters/plugin-distribution-reader-adapter.js"; +import { buildUnitDeps, initAndInstall } from "../../../helpers/ports/build-unit-deps.js"; +import { fakeEnsureBuiltMarketplace } from "../../../helpers/ports/fake-ensure-built-marketplace.js"; +import { InMemoryMarketplaceRegistry } from "../../../helpers/ports/in-memory-marketplace-registry.js"; +import { seedFromDirectory } from "../../../helpers/ports/seed-from-directory.js"; + +const PLUGIN_FIXTURE = join(process.cwd(), "tests/fixtures/plugins/claude-format/sample-plugin"); +const PROJECT_ROOT = "/test-project"; +const HOME = "/home/u"; + +// A plugin whose catalog entry resolves to a github-hosted source (git-subdir), the shape +// PluginInstallFromMarketplaceUseCase produces for any plugin catalogued in a github marketplace. +const GIT_SUBDIR_SOURCE = { + kind: "git-subdir" as const, + url: "https://github.com/ai-driven-dev/framework.git", + path: "plugins/sample-plugin", +}; + +const PLUGIN_METADATA = { name: "sample-plugin", version: "1.0.0", strict: false }; + +type Deps = Awaited>; + +async function makeGithubRegistry(): Promise { + const registry = new InMemoryMarketplaceRegistry(); + await registry.save( + PROJECT_ROOT, + Marketplace.create({ + name: "aidd-framework", + source: { kind: "github", repo: "ai-driven-dev/framework" }, + scope: "project", + addedAt: "2026-05-01T00:00:00.000Z", + }) + ); + return registry; +} + +function makeRestoreUseCase( + deps: Deps, + registry: InMemoryMarketplaceRegistry +): RestoreAllPluginsUseCase { + return new RestoreAllPluginsUseCase( + deps.fs, + deps.hasher, + deps.pluginFetcher, + new PluginDistributionReaderAdapter(deps.fs), + { + ensureBuilt: fakeEnsureBuiltMarketplace(), + marketplaceRegistry: registry, + homedir: () => HOME, + } + ); +} + +/** Installs a github-sourced marketplace plugin on claude, so its manifest entry carries + * both `marketplace` and a non-local `source.kind`. */ +async function installGithubPlugin( + deps: Deps, + registry: InMemoryMarketplaceRegistry +): Promise { + await seedFromDirectory(deps.fs, PLUGIN_FIXTURE, { useAbsolutePaths: true }); + deps.pluginFetcher.register(GIT_SUBDIR_SOURCE, PLUGIN_FIXTURE); + + await new PluginAddUseCase( + deps.fs, + deps.manifestRepo, + deps.pluginFetcher, + new PluginDistributionReaderAdapter(deps.fs), + deps.hasher, + deps.logger, + registry, + fakeEnsureBuiltMarketplace() + ).execute({ + source: GIT_SUBDIR_SOURCE, + toolIds: ["claude"], + projectRoot: PROJECT_ROOT, + marketplace: "aidd-framework", + interactive: false, + pluginMetadata: PLUGIN_METADATA, + }); +} + +describe("RestoreAllPluginsUseCase — Mode A marketplace tools (claude/codex/copilot)", () => { + it("restores a github-sourced marketplace plugin on claude without materializing any files", async () => { + const deps = await buildUnitDeps(PROJECT_ROOT); + await initAndInstall(deps, PROJECT_ROOT, "claude"); + const registry = await makeGithubRegistry(); + await installGithubPlugin(deps, registry); + + const manifest = await deps.manifestRepo.load(); + if (manifest === null) throw new Error("manifest not found"); + const before = manifest.getPlugins("claude").find((p) => p.name === "sample-plugin"); + expect(before?.files.size).toBe(0); + + const result = await makeRestoreUseCase(deps, registry).execute({ + projectRoot: PROJECT_ROOT, + manifest, + docsDir: DOCS_DIR, + fileFilter: null, + }); + + // Mode A's whole contract: register the reference, materialize nothing. Restore must + // not write plugin files that install never wrote, nor record them in the manifest. + expect(result.totalFiles).toBe(0); + expect(deps.fs.listAll().some((p) => p.includes(".claude/plugins/"))).toBe(false); + + const plugins = manifest.getPlugins("claude").filter((p) => p.name === "sample-plugin"); + expect(plugins).toHaveLength(1); + expect(plugins[0]?.files.size).toBe(0); + expect(plugins[0]?.marketplace).toBe("aidd-framework"); + expect(plugins[0]?.source.kind).toBe("git-subdir"); + }); +}); From 58e271d1f06f671bd45b5b7333812c68aee7a8a5 Mon Sep 17 00:00:00 2001 From: Baptiste LAFOURCADE Date: Wed, 29 Jul 2026 10:03:23 +0200 Subject: [PATCH 2/2] fix(cli): clear plugin files a marketplace tool should never have had Users who ran update or restore before the previous commit have stray materialized files for marketplace-mode plugins. Update self-heals, because it deletes the old manifest files before rebuilding, but only when a newer version exists upstream: on the latest version it returns early and never clears them. Restore had no delete step at all, so once the manifest entry went empty those files were orphaned and untracked. Restore now clears them, which makes it the reliable path for a plugin already on the latest version. The delete is bounded three ways. It runs only when the resolved translator reports mode "marketplace", which only ModeAMarketplaceTranslator does; both materializing translators report "flat", so built-tree and flat installs cannot reach it. It iterates the plugin's own manifest keys rather than scanning a directory, so it cannot touch a file the plugin never wrote. And it joins those keys to the plugin base dir from resolvePluginBaseDir rather than assuming the project root. A file placed inside the same plugin directory but absent from the manifest survives untouched, with a test asserting exactly that. An empty manifest entry makes the whole thing a no-op. deleteOldFiles moves from a private method on PluginUpdateUseCase to a shared helper in plugin-helpers.ts, used by both. Update's call stays unconditional: it also clears files dropped between versions on the built-tree and translate paths, so it is not the same concern as this cleanup. 2152/2152 pass (2150 plus 2), tsc clean, golden baseline unchanged. Mutation-tested by forcing the guard false: both new tests fail with "expected 'stray content' to be undefined". --- .../plan.md | 52 +++++++++++++- .../use-cases/plugin/plugin-helpers.ts | 12 ++++ .../plugin/plugin-update-use-case.ts | 9 +-- .../shared/apply-plugin-files-use-case.ts | 15 +++- ...ugin-files-mode-a-marketplace.unit.test.ts | 68 +++++++++++++++++++ 5 files changed, 147 insertions(+), 9 deletions(-) diff --git a/cli/aidd_docs/tasks/2026_07/2026_07_29_marketplace-mode-materialization/plan.md b/cli/aidd_docs/tasks/2026_07/2026_07_29_marketplace-mode-materialization/plan.md index 3f51474aa..39ee97633 100644 --- a/cli/aidd_docs/tasks/2026_07/2026_07_29_marketplace-mode-materialization/plan.md +++ b/cli/aidd_docs/tasks/2026_07/2026_07_29_marketplace-mode-materialization/plan.md @@ -53,7 +53,38 @@ Behavior after deploying the fix, verified by reading the final code (not assume - **`plugin update`, when no newer version exists**: `updateOnePlugin` returns early (`compareSemver(...) <= 0`) without calling `replacePluginFiles` at all. The stray files and the wrong manifest entry are left untouched indefinitely — update never even looks at this plugin again until a new version ships. - **`plugin restore`**: does **not** self-heal. `ApplyPluginFilesUseCase` has no equivalent `deleteOldFiles` step anywhere in the file — `restoreViaTranslate` only ever writes/overwrites files it currently computes, never deletes files absent from that set. After the fix, restore re-registers the manifest entry as empty via the Mode A branch, but the previously-materialized files stay on disk, now **orphaned and untracked** (previously they were at least recorded, if wrongly; after restore they're invisible to the manifest entirely). -This is not implemented as a silent cleanup, per the task's explicit instruction not to delete user files without a decision. Proposed option: add a symmetric `deleteOldFiles`-equivalent step to the Mode A branch of `ApplyPluginFilesUseCase` (delete `plugin.files` under the tool's resolved base dir before calling `materializeViaTranslator`), mirroring what `PluginUpdateUseCase` already does unconditionally. That would make restore self-heal the same way update does when a version bump triggers it, and would also cover the "no version bump" update gap since restore doesn't have a version-gate. This needs an explicit decision because it deletes files the tool previously wrote to the user's project — flagging for follow-up rather than implementing here. +### Implemented: restore now self-heals + +`deleteOldFiles` was extracted from `PluginUpdateUseCase` into a shared, exported function in `plugin-helpers.ts` (`deleteOldFiles(files, baseDir, fs)`), and `ApplyPluginFilesUseCase.restoreViaTranslator` calls it as a new, narrowly-gated step: + +```ts +if (translator.mode === "marketplace" && this.builtDeps !== undefined) { + const baseDir = resolvePluginBaseDir(toolId, projectRoot, this.builtDeps.homedir); + await deleteOldFiles(plugin.files, baseDir, this.fs); +} +``` + +Safety bounds, checked against the final code: + +- **Manifest-scoped only.** `deleteOldFiles` iterates `plugin.files.keys()` — the manifest's own tracked paths — and joins each to `baseDir`. It never lists a directory and never deletes by glob or pattern. A file physically present in the same plugin directory but absent from the manifest entry is untouched (covered by a dedicated test — see below). +- **Marketplace/Mode A branch only.** The gate is `translator.mode === "marketplace"`, the exact discriminant `resolveTranslator` (`plugin-translator-factory.ts:31-48`) uses to hand out `ModeAMarketplaceTranslator`. `BuiltTreeMaterializationTranslator` (cursor/opencode) and the `restoreViaTranslate` fallback (flat mode / non-marketplace installs) declare `mode: "flat"` or never reach this method at all, so the delete is structurally unreachable for them — confirmed by grepping `readonly mode` across every translator implementation. +- **Bounded to the plugin's base dir.** `baseDir` comes from `resolvePluginBaseDir(toolId, projectRoot, homedir)`, the same resolver `PluginUpdateUseCase` uses — not a hardcoded `projectRoot`. Checked that all three Mode A tools (claude, codex, copilot) default to `installScope: "project"` (none sets `installScope: "user"`), so `baseDir === projectRoot` for all three today, but the code doesn't assume that. +- **Missing-file safe.** Both the real `FileAdapter.deleteFile` (`rm(path, { force: true })`) and the in-memory test adapter (`Map.delete`) are no-ops on a path that doesn't exist, so a partially-cleaned install (some stray files already removed by hand) can't crash restore. + +`PluginUpdateUseCase.replacePluginFiles` was refactored to call the same shared `deleteOldFiles` (previously a private method with an identical body) instead of duplicating it — its call stayed in the same unconditional position it already had, before the translator branch, since that unconditional delete is by design (it also clears files removed between versions on the built-tree and translate-fallback paths, not just the Mode A case). Only restore's new call is gated on `mode === "marketplace"`; the shared function itself carries no branch logic. + +This closes a gap the original fix didn't: `plugin update` only self-heals when a newer version exists upstream (`updateOnePlugin` returns early via `compareSemver` otherwise, per the trace above) — a plugin already on the latest version never gets its stray files cleared by update, ever. `restore` has no such version gate, so it is now the reliable self-heal path for stray Mode A files regardless of whether an upstream update is available. + +### Tests + +Added to `tests/application/use-cases/shared/apply-plugin-files-mode-a-marketplace.unit.test.ts`: + +- "cleans up files a pre-fix update/restore materialized, leaving the manifest entry empty" — seeds a manifest entry with a stray non-empty `files` map (simulating a pre-fix run) plus matching files on disk, restores, and asserts both the files and the manifest entries are gone. +- "leaves a file the manifest never tracked untouched, even inside the plugin's own directory" — the safety-critical test. Places an untracked file (`.claude/plugins/sample-plugin/notes.md`) alongside a tracked stray file in the *same* plugin directory, restores, and asserts the untracked file survives with its exact original content while the tracked one is gone. Proves the cleanup is manifest-scoped, not directory-scoped. +- The existing "restores ... without materializing any files" test in the same file already covers the no-op case (empty manifest entry → `deleteOldFiles` iterates zero keys). +- Regression coverage for built-tree (cursor/opencode) and translate-fallback (flat/local-source) restores was not duplicated — `apply-plugin-files-built-tree.unit.test.ts` and `restore-all-use-case.unit.test.ts` (`restoreViaTranslate` for both claude-local and cursor-local installs) already exercise those paths and stayed green through this change, since the new delete is structurally unreachable from them. + +Mutation test: temporarily forced the new `if` guard to `false` (dead code). Both new tests failed with the exact predicted symptom (`expected 'stray content' to be undefined`) — one on the stray-file assertion, one on the tracked-file-gone assertion inside the untracked-survives test. Reverted; full suite re-confirmed green at 2152/2152. ## Tests @@ -100,3 +131,22 @@ Read directly from the final code, not inferred: - `src/application/use-cases/plugin/translator/plugin-translator.ts` — `addPlugin`'s return type gained an optional `written?: number` so the shared helper can report it uniformly across strategies. - `tests/application/use-cases/plugin/plugin-update-mode-a-marketplace.unit.test.ts` — new. - `tests/application/use-cases/shared/apply-plugin-files-mode-a-marketplace.unit.test.ts` — new. + +## Restore self-heal follow-up — verification (real numbers) + +1. `npx tsc --noEmit` — clean, no errors. +2. `pnpm test` from `cli/` — 200 test files, **2152 tests passed**, zero failures. Baseline for this follow-up was the 2150 above; added 2 new tests (both in `apply-plugin-files-mode-a-marketplace.unit.test.ts`). No existing assertion was modified. +3. Biome, one file at a time via `./node_modules/.bin/biome check --write `: + - `src/application/use-cases/plugin/plugin-helpers.ts` — no fixes applied. + - `src/application/use-cases/plugin/plugin-update-use-case.ts` — no fixes applied. + - `src/application/use-cases/shared/apply-plugin-files-use-case.ts` — no fixes applied. + - `tests/application/use-cases/shared/apply-plugin-files-mode-a-marketplace.unit.test.ts` — no fixes applied. +4. Mutation test: forced the new `if (translator.mode === "marketplace" && this.builtDeps !== undefined)` guard to `&& false`, making the cleanup dead code. Both new tests failed with the exact predicted symptom (`expected 'stray content' to be undefined`). Reverted; `pnpm test` re-confirmed 2152/2152 green. +5. Golden baseline: `tests/golden/golden-baseline.e2e.test.ts` passed unchanged (2/2) as part of the full suite; no regeneration needed. + +## Follow-up files changed + +- `src/application/use-cases/plugin/plugin-helpers.ts` — added exported `deleteOldFiles(files, baseDir, fs)`. +- `src/application/use-cases/plugin/plugin-update-use-case.ts` — private `deleteOldFiles` method removed; now calls the shared function in the same unconditional position. +- `src/application/use-cases/shared/apply-plugin-files-use-case.ts` — `restoreViaTranslator` calls `deleteOldFiles` gated on `translator.mode === "marketplace"`. +- `tests/application/use-cases/shared/apply-plugin-files-mode-a-marketplace.unit.test.ts` — 2 new tests (stray-file cleanup, untracked-file survives). diff --git a/cli/src/application/use-cases/plugin/plugin-helpers.ts b/cli/src/application/use-cases/plugin/plugin-helpers.ts index 01096b863..4213c2108 100644 --- a/cli/src/application/use-cases/plugin/plugin-helpers.ts +++ b/cli/src/application/use-cases/plugin/plugin-helpers.ts @@ -65,6 +65,18 @@ export async function writePluginFiles( await Promise.all(files.map((f) => fs.writeFile(join(baseDir, f.relativePath), f.content))); } +/** Deletes exactly the paths a plugin's own manifest entry lists, joined to its base dir. + * Never enumerates the directory or deletes by pattern — only manifest-tracked keys. */ +export async function deleteOldFiles( + files: ReadonlyMap, + baseDir: string, + fs: FileWriter +): Promise { + for (const relativePath of files.keys()) { + await fs.deleteFile(join(baseDir, relativePath)); + } +} + /** Whether the file already on disk matches the content we would write, so a * caller can skip the write and, more importantly, not count it as restored. */ export async function isPluginFileAtDesiredState( diff --git a/cli/src/application/use-cases/plugin/plugin-update-use-case.ts b/cli/src/application/use-cases/plugin/plugin-update-use-case.ts index 0021f5fe2..e737d1501 100644 --- a/cli/src/application/use-cases/plugin/plugin-update-use-case.ts +++ b/cli/src/application/use-cases/plugin/plugin-update-use-case.ts @@ -16,6 +16,7 @@ import type { PluginFetcher } from "../../../domain/ports/plugin-fetcher.js"; import { getToolConfig, type ToolConfig } from "../../../domain/tools/registry.js"; import type { BuiltMaterializationDeps } from "../shared/apply-plugin-files-use-case.js"; import { + deleteOldFiles, loadPluginManifest, materializeViaTranslator, resolvePluginBaseDir, @@ -116,7 +117,7 @@ export class PluginUpdateUseCase { docsDir: string ): Promise { const baseDir = resolvePluginBaseDir(toolId, projectRoot, nodeHomedir); - await this.deleteOldFiles(plugin.files, baseDir); + await deleteOldFiles(plugin.files, baseDir, this.fs); const toolConfig = getToolConfig(toolId); const translator = this.resolveTranslator(toolConfig); if (translator !== null && plugin.marketplace !== undefined) { @@ -141,12 +142,6 @@ export class PluginUpdateUseCase { ); } - private async deleteOldFiles(files: ReadonlyMap, baseDir: string): Promise { - for (const relativePath of files.keys()) { - await this.fs.deleteFile(join(baseDir, relativePath)); - } - } - // Materializing tools (cursor/opencode) re-materialize from the BUILT tree, and Mode A // marketplace tools (claude/codex/copilot) re-register without writing files, so an // update matches whatever install would have done for that tool. diff --git a/cli/src/application/use-cases/shared/apply-plugin-files-use-case.ts b/cli/src/application/use-cases/shared/apply-plugin-files-use-case.ts index fa7d31adc..f2ca30ee2 100644 --- a/cli/src/application/use-cases/shared/apply-plugin-files-use-case.ts +++ b/cli/src/application/use-cases/shared/apply-plugin-files-use-case.ts @@ -11,7 +11,12 @@ import type { MarketplaceRegistry } from "../../../domain/ports/marketplace-regi import type { PluginDistributionReader } from "../../../domain/ports/plugin-distribution-reader.js"; import type { PluginFetcher } from "../../../domain/ports/plugin-fetcher.js"; import type { ToolConfig } from "../../../domain/tools/registry.js"; -import { isPluginFileAtDesiredState, materializeViaTranslator } from "../plugin/plugin-helpers.js"; +import { + deleteOldFiles, + isPluginFileAtDesiredState, + materializeViaTranslator, + resolvePluginBaseDir, +} from "../plugin/plugin-helpers.js"; import type { PluginTranslator } from "../plugin/translator/plugin-translator.js"; import { resolvePluginTranslator } from "../plugin/translator/resolve-plugin-translator.js"; import type { EnsureBuiltMarketplaceUseCase } from "./ensure-built-marketplace-use-case.js"; @@ -74,6 +79,14 @@ export class ApplyPluginFilesUseCase { options: ApplyPluginFilesOptions ): Promise { const { toolId, plugin, projectRoot, manifest, docsDir } = options; + // Mode A never materializes files, so any manifest-tracked path here is a leftover + // from a run before that was true (see plugin-update-use-case.ts's unconditional + // equivalent). Scoped to the manifest's own keys under the plugin's base dir — never + // a directory scan — so it cannot touch files the plugin never wrote. + if (translator.mode === "marketplace" && this.builtDeps !== undefined) { + const baseDir = resolvePluginBaseDir(toolId, projectRoot, this.builtDeps.homedir); + await deleteOldFiles(plugin.files, baseDir, this.fs); + } return materializeViaTranslator( translator, dist, diff --git a/cli/tests/application/use-cases/shared/apply-plugin-files-mode-a-marketplace.unit.test.ts b/cli/tests/application/use-cases/shared/apply-plugin-files-mode-a-marketplace.unit.test.ts index 8100a1f3b..93036f1e9 100644 --- a/cli/tests/application/use-cases/shared/apply-plugin-files-mode-a-marketplace.unit.test.ts +++ b/cli/tests/application/use-cases/shared/apply-plugin-files-mode-a-marketplace.unit.test.ts @@ -86,6 +86,74 @@ async function installGithubPlugin( } describe("RestoreAllPluginsUseCase — Mode A marketplace tools (claude/codex/copilot)", () => { + it("cleans up files a pre-fix update/restore materialized, leaving the manifest entry empty", async () => { + const deps = await buildUnitDeps(PROJECT_ROOT); + await initAndInstall(deps, PROJECT_ROOT, "claude"); + const registry = await makeGithubRegistry(); + await installGithubPlugin(deps, registry); + + const manifest = await deps.manifestRepo.load(); + if (manifest === null) throw new Error("manifest not found"); + const plugin = manifest.getPlugins("claude").find((p) => p.name === "sample-plugin"); + if (plugin === undefined) throw new Error("plugin not found"); + + // Simulate the pre-fix bug: a buggy update/restore materialized files and recorded + // them on the manifest entry, even though Mode A's contract is register-only. + const strayFiles = new Map([ + [".claude/plugins/sample-plugin/commands/greet.md", "stray-hash-1"], + [".claude/plugins/sample-plugin/agents/reviewer.md", "stray-hash-2"], + ]); + manifest.updatePlugin("claude", plugin.withFiles(strayFiles)); + for (const relativePath of strayFiles.keys()) { + await deps.fs.writeFile(join(PROJECT_ROOT, relativePath), "stray content"); + } + + const result = await makeRestoreUseCase(deps, registry).execute({ + projectRoot: PROJECT_ROOT, + manifest, + docsDir: DOCS_DIR, + fileFilter: null, + }); + + for (const relativePath of strayFiles.keys()) { + expect(deps.fs.getFile(join(PROJECT_ROOT, relativePath))).toBeUndefined(); + } + const after = manifest.getPlugins("claude").find((p) => p.name === "sample-plugin"); + expect(after?.files.size).toBe(0); + expect(result.totalFiles).toBe(0); + }); + + it("leaves a file the manifest never tracked untouched, even inside the plugin's own directory", async () => { + const deps = await buildUnitDeps(PROJECT_ROOT); + await initAndInstall(deps, PROJECT_ROOT, "claude"); + const registry = await makeGithubRegistry(); + await installGithubPlugin(deps, registry); + + const manifest = await deps.manifestRepo.load(); + if (manifest === null) throw new Error("manifest not found"); + const plugin = manifest.getPlugins("claude").find((p) => p.name === "sample-plugin"); + if (plugin === undefined) throw new Error("plugin not found"); + + const trackedPath = ".claude/plugins/sample-plugin/commands/greet.md"; + const untrackedPath = ".claude/plugins/sample-plugin/notes.md"; + manifest.updatePlugin("claude", plugin.withFiles(new Map([[trackedPath, "stray-hash"]]))); + await deps.fs.writeFile(join(PROJECT_ROOT, trackedPath), "stray content"); + // Not in the manifest entry — a file the user (or something else) placed alongside the + // plugin's tracked files. The cleanup must never delete this, since it only ever + // iterates the manifest's own keys. + await deps.fs.writeFile(join(PROJECT_ROOT, untrackedPath), "keep me"); + + await makeRestoreUseCase(deps, registry).execute({ + projectRoot: PROJECT_ROOT, + manifest, + docsDir: DOCS_DIR, + fileFilter: null, + }); + + expect(deps.fs.getFile(join(PROJECT_ROOT, trackedPath))).toBeUndefined(); + expect(deps.fs.getFile(join(PROJECT_ROOT, untrackedPath))).toBe("keep me"); + }); + it("restores a github-sourced marketplace plugin on claude without materializing any files", async () => { const deps = await buildUnitDeps(PROJECT_ROOT); await initAndInstall(deps, PROJECT_ROOT, "claude");