diff --git a/cli/aidd_docs/tasks/2026_07/2026_07_29_restore-reporting-findings/plan.md b/cli/aidd_docs/tasks/2026_07/2026_07_29_restore-reporting-findings/plan.md new file mode 100644 index 00000000..4701f1f8 --- /dev/null +++ b/cli/aidd_docs/tasks/2026_07/2026_07_29_restore-reporting-findings/plan.md @@ -0,0 +1,113 @@ +--- +objective: "Restore tells the truth about what it did, and marketplace-mode plugins are handled the same way on every command." +status: findings-1-2-done +--- + +# Plan: three findings surfaced during the cartography backlog + +None of these came from a ticket. Each was found while refactoring nearby code, recorded in a PR, and deliberately not fixed at the time to keep those PRs behaviour-preserving. + +## Finding 1 — restore silently drops a drifted file (confirmed) + +`restore-regular-files-use-case.ts:83-103`. Both drift branches end in the same guard: + +```ts +const distFile = distMap.get(manifestFile.relativePath); +if (distFile) drift.push({ ... }); +// no else +``` + +A file the manifest tracks, which is genuinely deleted or modified on disk, is skipped when the current distribution no longer contains it. It is not restored, not counted as kept, and not reported. The user is told restore succeeded while a tracked file stays broken. + +This is reachable whenever the manifest outlives the distribution: a file dropped in a newer framework version, or a tool whose content set changed. + +**Fix.** Collect these into a distinct outcome rather than dropping them. The skeleton already partitions `restored` / `kept`, so a third category is a natural extension: the entry is drifted, but nothing exists to restore it from. Surface it in the command output as a warning, since a silent success is the actual defect. + +## Finding 2 — restore reports two different numbers (confirmed, worse than recorded) + +`apply-plugin-files-use-case.ts:70-100`. The same `execute()` returns a count whose meaning depends on which path ran: + +| Path | Returns | Honours `fileFilter` | +| ---- | ------- | -------------------- | +| `restoreViaBuiltTree` (line 77) | `plugin.files.size`, every file the plugin has | no | +| `restoreViaTranslate` (line 86-99) | only files actually written | yes | + +So on a materializing tool (cursor, opencode), a restore where nothing drifted still reports the full file count as if it had restored everything. + +The `fileFilter` half was not in the original note and is the more serious of the two: a scoped restore may write outside its scope on the built-tree path. Whether that is wrong depends on whether a built subtree can be partially copied at all, so it needs checking before changing. + +**Fix.** Make `restoreViaBuiltTree` count what it actually wrote, so both paths mean the same thing. Treat the `fileFilter` asymmetry as a separate question and confirm intent before touching it. + +## Finding 3 — marketplace-mode plugins take a different path on update/restore (needs verification first) + +`resolveTranslator` returns `ModeAMarketplaceTranslator` when `translationMode === "marketplace"` (factory line 44-45). But `plugin-update-use-case.ts` and `apply-plugin-files-use-case.ts` both keep the translator only if it is a `BuiltTreeMaterializationTranslator` and otherwise fall through to `PluginContentTranslator`, which materializes files. + +If a tool installs a plugin by registering a reference and writing nothing, then update and restore writing files for it would create content install never produced. + +**Not yet actionable.** Two things decide it, and neither is established: + +1. which tools actually set `translationMode: "marketplace"`; +2. whether such a plugin has any `files` in the manifest, since if it has none the fall-through iterates nothing and the concern is empty. + +A spike answers both before any fix. Consistent with the rest of this backlog, this may end up infirmed. + +## Order + +1. Spike finding 3 (cheap, decides whether it is real). +2. Fix finding 1, with a test proving the dropped entry is now reported. +3. Fix finding 2's count, with a test proving a no-op restore on the built-tree path reports zero. +4. Fix finding 3 only if the spike confirms it. + +Findings 1 and 2 both change user-visible output. That is the point in both cases: the current output is wrong. + +## What actually happened (2026-07-29) + +Findings 1 and 2 fixed. Finding 3 untouched — still needs its spike before any code change, and wasn't in this pass's scope. + +### Finding 1 + +Added a third outcome, named `unrestorable`, to the shared drift skeleton (`restore-drift-entries-use-case.ts`): `DriftCollection` is now `{ drift, unrestorable }`, and `RestoreDriftLeaf.buildResult` takes `(restored, kept, unrestorable)`. The null short-circuit (`collectDrift` found nothing at all) now checks both arrays, so a tool with only unrestorable entries no longer collapses to a silent no-op. + +Both leaves (`restore-regular-files-use-case.ts`, `restore-merge-files-use-case.ts`) were rewritten to separate "did this drift" (independent of the dist map) from "can we restore it" (dist map lookup) — previously those were fused, which is exactly how the entry vanished with no `else`. + +The merge path had the same hole, confirmed by tracing it, not assumed: `collectMergeDrift`'s `if (!distFile || distFile.mergeStrategy === "none") continue;` silently dropped genuinely-drifted files under both conditions. Both are now routed to `unrestorable` — a file the current distribution no longer merge-tracks (dropped entirely, or its strategy became `"none"`) has nothing left to restore drift from, same as the regular-files case. An existing test asserting `mergeStrategy: "none"` silently returns `null` was renamed and its assertion changed (see below); a second test was added confirming `"none"` with *no* actual drift still correctly returns `null` (the "not drifted at all" case wasn't part of the bug and stays untouched). + +The count flows up unchanged in shape: `RestoreToolFilesResult`, `RestoreResult` (restore-use-case.ts), and `RestoreAllResult` (restore-all-use-case.ts) each gained an `unrestorable: string[]` field, aggregated by concatenation/flatMap at each layer — never counted as `restored`. + +Surfaced to the user as a warning line in all three command entry points that report restore results: `aidd restore` (restore.ts), `aidd ide restore` (ide.ts), `aidd ai restore` (ai.ts). Each now emits, when `unrestorable.length > 0`: + +``` +Could not restore N file(s) no longer part of the current distribution: +``` + +The "nothing to restore" success gate in all three now also checks `unrestorable.length === 0`, so a tool with only unrestorable entries no longer reports false success. + +### Finding 2 + +`restoreViaBuiltTree` no longer reads `manifest.getPlugins(toolId).find(...)?.files.size` after the fact. `materializeViaBuiltTree` (plugin-helpers.ts) now returns the actual number of files (re)written, and `BuiltTreeMaterializationTranslator.addPlugin` computes that count itself: it now skips writing (and doesn't count) any file whose disk content already matches the built-tree content, mirroring `restoreViaTranslate`'s `isFileAtDesiredState` check exactly — both paths now call the same extracted helper, `isPluginFileAtDesiredState` in plugin-helpers.ts. This was necessary, not just a reporting fix: the built-tree write step previously overwrote every file unconditionally every restore, so "how many files were written" and "the plugin's total file count" were the same number by construction. Making the count honest required making the write conditional. + +`BuiltTreeMaterializationTranslator.addPlugin`'s own return type is widened to `{ skipped; written?: number }` (not the shared `PluginTranslator` interface, which stays `{ skipped }` — this class is always consumed through its concrete type at the two call sites that need the count). `written` is `undefined` only on the rare fallback branch (marketplace lookup fails at restore time and materialization delegates to `ModeBFlatMaterializationTranslator`); `materializeViaBuiltTree` reports that as `0` rather than guessing, since undercounting is safer than falsely claiming files changed. + +**fileFilter read-only finding:** `restoreViaBuiltTree` still ignores `fileFilter` — confirmed, not changed. `BuiltTreeMaterializationTranslator` reads the entire built subtree for a plugin from disk (`readBuiltFiles`/`readFlatFiles`, driven by `listFilesRecursive` over the built dir) and writes it as one unit; there is no per-file hook into that read step that a `fileFilter` predicate could gate without restructuring how the built tree is enumerated. Recommendation: treat verbatim-subtree-copy as intentional for this translator (it exists specifically so restored bytes match `framework build` output 1:1) and either (a) accept that `--files` scoping doesn't apply to built-tree-materialized tools (cursor/opencode plugins) and say so in `--help`/docs, or (b) if partial restore of a built subtree is actually wanted, filter the file list returned by `readBuiltFiles`/`readFlatFiles` before `writeChangedFiles` — a small, isolated change, but a product decision (does "restore only these files" mean anything for a tool that's supposed to mirror the build verbatim?) rather than a bug fix. + +### Existing test assertions changed + +1. `restore-regular-files-use-case.unit.test.ts`: the two tests titled *"silently drops a \[deleted/modified\] file that has no corresponding entry in the dist map"* asserted `result === null` and the file left untouched. Renamed to *"reports a \[deleted/modified\] file with no corresponding dist entry as unrestorable, without touching disk"*; assertions changed to `result?.unrestorable === ["a.md"]`, `restored === []`, `kept === []`, disk still untouched. This is the literal behavior the finding says was wrong — the old assertion encoded the bug. +2. `restore-merge-files-use-case.unit.test.ts`: *"skips merge files whose distribution strategy is 'none'"* asserted `result === null` for a file that was genuinely drifted on disk. Renamed to *"reports a drifted merge file as unrestorable when the distribution strategy is 'none'"*; assertions changed to `unrestorable === ["a.json"]`, disk untouched. Added a new sibling test, *"returns null when a merge file's distribution strategy is 'none' and nothing drifted"*, to keep the genuinely-inert case (no drift at all) covered as returning `null`, since that part of the old test's premise was correct and shouldn't regress. + +### New tests + +- Two regression tests above (deleted/modified file dropped from dist map) in `restore-regular-files-use-case.unit.test.ts`. +- One regression test in `restore-merge-files-use-case.unit.test.ts` (mergeStrategy `"none"` with real drift). +- One in `apply-plugin-files-built-tree.unit.test.ts`: *"reports zero files restored when a built-tree restore finds nothing drifted"* — seeds the user-scope plugin dir with content identical to the built tree, then asserts `result.totalFiles === 0`. (Note: had to seed disk directly rather than rely on the preceding install, because the test helper's `installMarketplacePlugin` uses `PluginAddUseCase`'s default `os.homedir()` while `makeRestoreUseCase` injects a fake `homedir: () => "/home/u"` — those two write to different base directories in this test file already, unrelated to this fix, left as-is.) + +### Verification + +- `npx tsc --noEmit`: clean. +- `pnpm test` (full build + vitest): 198 files, **2146 tests**, 0 failures (baseline was 2144; net +2 from the new merge-drift and built-tree no-op tests — the two regular-files tests were renamed in place, not added). +- Biome: every changed file individually reports "No fixes applied" after one `--write` pass (two files needed one auto-format pass for a line-wrap, then were clean). +- Golden baseline e2e (`tests/golden/golden-baseline.e2e.test.ts`): passed unchanged, no regeneration needed — its only restore scenario (`restore --force` with nothing modified) has zero unrestorable entries, so output is byte-identical. +- Mutation testing: reverted each fix to its buggy form one at a time, confirmed exactly the new/changed tests failed (and only those), then restored the fix and re-ran the full suite green. + - Finding 1: reverted `collectDrift`'s dist-map-miss branch back to a silent `continue`. Both new regular-files tests failed with `result === null` where `unrestorable: ["a.md"]` was expected — 2/2146 failed. + - Finding 1 (merge): reverted the merge leaf's dist-map/`"none"` branch back to a silent `continue`. The new merge test failed the same way — 1/2146 failed. + - Finding 2: reverted `restoreViaBuiltTree` back to `manifest.getPlugins(...).files.size`. The new built-tree no-op test failed (`1` instead of `0`) — 1/2146 failed. diff --git a/cli/src/application/commands/ai.ts b/cli/src/application/commands/ai.ts index 29141957..bc29bb51 100644 --- a/cli/src/application/commands/ai.ts +++ b/cli/src/application/commands/ai.ts @@ -4,6 +4,7 @@ import type { AiToolId } from "../../domain/models/tool-ids.js"; import { AI_TOOL_IDS, isAiToolId } from "../../domain/models/tool-ids.js"; import type { ToolId } from "../../domain/tools/registry.js"; import { createDeps, createMenuDeps } from "../../infrastructure/deps.js"; +import { printUnrestorable } from "../display/restore-display.js"; import { ErrorHandler } from "../error-handler.js"; import { NoManifestError } from "../errors.js"; import { parseGlobalOptions } from "./global-options.js"; @@ -227,6 +228,7 @@ export function registerAiCommand(program: Command): void { output.success( `Restored ${restored} ${restored === 1 ? "file" : "files"}, kept ${kept} ${kept === 1 ? "file" : "files"}` ); + printUnrestorable(output, result.unrestorable); } catch (error) { errorHandler.handle(error); } diff --git a/cli/src/application/commands/ide.ts b/cli/src/application/commands/ide.ts index d85e0cc7..75191fa8 100644 --- a/cli/src/application/commands/ide.ts +++ b/cli/src/application/commands/ide.ts @@ -3,6 +3,7 @@ import { Manifest } from "../../domain/models/manifest.js"; import { DOCS_DIR } from "../../domain/models/paths.js"; import { IDE_TOOL_IDS, type IdeToolId } from "../../domain/models/tool-ids.js"; import { createDeps, createMenuDeps } from "../../infrastructure/deps.js"; +import { printUnrestorable } from "../display/restore-display.js"; import { ErrorHandler } from "../error-handler.js"; import { NoManifestError } from "../errors.js"; import { parseGlobalOptions } from "./global-options.js"; @@ -214,6 +215,7 @@ export function registerIdeCommand(program: Command): void { output.success( `Restored ${result.totalRestored} ${result.totalRestored === 1 ? "file" : "files"}, kept ${result.totalKept} ${result.totalKept === 1 ? "file" : "files"}` ); + printUnrestorable(output, result.unrestorable); } catch (error) { errorHandler.handle(error); } diff --git a/cli/src/application/commands/restore.ts b/cli/src/application/commands/restore.ts index 733d63cc..9fc26b83 100644 --- a/cli/src/application/commands/restore.ts +++ b/cli/src/application/commands/restore.ts @@ -1,5 +1,6 @@ import type { Command } from "commander"; import { createDeps } from "../../infrastructure/deps.js"; +import { printUnrestorable } from "../display/restore-display.js"; import { ErrorHandler } from "../error-handler.js"; import { parseGlobalOptions } from "./global-options.js"; @@ -19,7 +20,11 @@ export function registerRestoreCommand(program: Command): void { for (const e of result.errors) output.warn(`[${e.scope}] ${e.message}`); - if (result.totalRestored === 0 && result.pluginNamesRestored.length === 0) { + if ( + result.totalRestored === 0 && + result.pluginNamesRestored.length === 0 && + result.unrestorable.length === 0 + ) { output.success("Nothing to restore — all files are unmodified."); return; } @@ -31,6 +36,7 @@ export function registerRestoreCommand(program: Command): void { if (result.pluginNamesRestored.length > 0) { output.success(`Restored plugins: ${result.pluginNamesRestored.join(", ")}`); } + printUnrestorable(output, result.unrestorable); } catch (error) { errorHandler.handle(error); } diff --git a/cli/src/application/display/restore-display.ts b/cli/src/application/display/restore-display.ts new file mode 100644 index 00000000..99a96360 --- /dev/null +++ b/cli/src/application/display/restore-display.ts @@ -0,0 +1,8 @@ +import type { CLIOutput } from "../output.js"; + +export function printUnrestorable(output: CLIOutput, unrestorable: readonly string[]): void { + if (unrestorable.length === 0) return; + output.warn( + `Could not restore ${unrestorable.length} file(s) no longer part of the current distribution: ${unrestorable.join(", ")}` + ); +} diff --git a/cli/src/application/use-cases/global/restore-all-use-case.ts b/cli/src/application/use-cases/global/restore-all-use-case.ts index 58f4107c..690c073c 100644 --- a/cli/src/application/use-cases/global/restore-all-use-case.ts +++ b/cli/src/application/use-cases/global/restore-all-use-case.ts @@ -11,6 +11,7 @@ export interface RestoreAllResult { totalKept: number; pluginNamesRestored: string[]; errors: GlobalExecutionError[]; + unrestorable: string[]; } export class RestoreAllUseCase { @@ -42,6 +43,7 @@ export class RestoreAllUseCase { totalKept: restoreResult.totalKept, pluginNamesRestored: restoreResult.restoredPluginNames, errors, + unrestorable: restoreResult.unrestorable, }; } @@ -76,9 +78,15 @@ export class RestoreAllUseCase { interactive: boolean, manifest: Awaited>, errors: GlobalExecutionError[] - ): Promise<{ totalRestored: number; totalKept: number; restoredPluginNames: string[] }> { + ): Promise<{ + totalRestored: number; + totalKept: number; + restoredPluginNames: string[]; + unrestorable: string[]; + }> { + const empty = { totalRestored: 0, totalKept: 0, restoredPluginNames: [], unrestorable: [] }; try { - if (manifest === null) return { totalRestored: 0, totalKept: 0, restoredPluginNames: [] }; + if (manifest === null) return empty; const result = await this.restoreUseCase.execute({ version, docsDir: DOCS_DIR, @@ -92,13 +100,14 @@ export class RestoreAllUseCase { totalRestored: result.totalRestored, totalKept: result.totalKept, restoredPluginNames: result.restoredPluginNames, + unrestorable: result.unrestorable, }; } catch (err) { errors.push({ scope: "config-restore", message: err instanceof Error ? err.message : String(err), }); - return { totalRestored: 0, totalKept: 0, restoredPluginNames: [] }; + return empty; } } } diff --git a/cli/src/application/use-cases/plugin/plugin-helpers.ts b/cli/src/application/use-cases/plugin/plugin-helpers.ts index eff0e160..f23aaeec 100644 --- a/cli/src/application/use-cases/plugin/plugin-helpers.ts +++ b/cli/src/application/use-cases/plugin/plugin-helpers.ts @@ -7,7 +7,9 @@ import type { Plugin } from "../../../domain/models/plugin.js"; import type { PluginDistribution } from "../../../domain/models/plugin-distribution.js"; import type { AiToolId } from "../../../domain/models/tool-ids.js"; import { AI_TOOL_IDS } from "../../../domain/models/tool-ids.js"; +import type { FileReader } from "../../../domain/ports/file-reader.js"; import type { FileWriter } from "../../../domain/ports/file-writer.js"; +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"; @@ -63,10 +65,29 @@ export async function writePluginFiles( await Promise.all(files.map((f) => fs.writeFile(join(baseDir, f.relativePath), f.content))); } +/** 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( + fs: FileReader, + hasher: Hasher, + outputPath: string, + expectedHashValue: string +): Promise { + if (!(await fs.fileExists(outputPath))) return false; + const content = await fs.readFile(outputPath); + return hasher.hash(content).value === expectedHashValue; +} + /** * 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. + * + * 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 + * is reported as 0 rather than guessed. */ export async function materializeViaBuiltTree( translator: BuiltTreeMaterializationTranslator, @@ -76,9 +97,9 @@ export async function materializeViaBuiltTree( projectRoot: string, manifest: Manifest, docsDir: string -): Promise { +): Promise { manifest.removePlugin(toolId, plugin.name); - await translator.addPlugin( + const { written } = await translator.addPlugin( dist, toolId, plugin.source, @@ -87,4 +108,5 @@ export async function materializeViaBuiltTree( plugin.marketplace, docsDir ); + return written ?? 0; } diff --git a/cli/src/application/use-cases/plugin/translator/built-tree-materialization-translator.ts b/cli/src/application/use-cases/plugin/translator/built-tree-materialization-translator.ts index 8d373b35..0c4fadfa 100644 --- a/cli/src/application/use-cases/plugin/translator/built-tree-materialization-translator.ts +++ b/cli/src/application/use-cases/plugin/translator/built-tree-materialization-translator.ts @@ -11,7 +11,7 @@ import type { FileWriter } from "../../../../domain/ports/file-writer.js"; import type { Hasher } from "../../../../domain/ports/hasher.js"; import type { MarketplaceRegistry } from "../../../../domain/ports/marketplace-registry.js"; import type { EnsureBuiltMarketplaceUseCase } from "../../shared/ensure-built-marketplace-use-case.js"; -import { resolvePluginBaseDir, writePluginFiles } from "../plugin-helpers.js"; +import { isPluginFileAtDesiredState, resolvePluginBaseDir } from "../plugin-helpers.js"; import { ModeBFlatMaterializationTranslator } from "./mode-b-flat-materialization-translator.js"; import type { PluginTranslator } from "./plugin-translator.js"; @@ -44,7 +44,7 @@ export class BuiltTreeMaterializationTranslator implements PluginTranslator { marketplace: string | undefined, docsDir: string, previousMcpEntries: ReadonlyMap = new Map() - ): Promise<{ skipped: ReadonlySkipList }> { + ): Promise<{ skipped: ReadonlySkipList; written?: number }> { const resolved = marketplace === undefined ? null : await this.findMarketplace(marketplace, projectRoot); if (marketplace === undefined || resolved === null) { @@ -75,12 +75,27 @@ export class BuiltTreeMaterializationTranslator implements PluginTranslator { ); const baseDir = mode === "flat" ? projectRoot : resolvePluginBaseDir(toolId, projectRoot, this.homedir); - await writePluginFiles(files, baseDir, this.fs); + const written = await this.writeChangedFiles(files, baseDir); manifest.addPlugin( toolId, Plugin.fromDistribution(dist, source, files, new Map(), marketplace) ); - return { skipped: [] }; + return { skipped: [], written }; + } + + // Verbatim-copies the built subtree, but skips files already matching the built + // content on disk so a no-op restore reports (and performs) zero writes. + private async writeChangedFiles(files: InstallationFile[], baseDir: string): Promise { + let written = 0; + for (const f of files) { + const outputPath = join(baseDir, f.relativePath); + if (await isPluginFileAtDesiredState(this.fs, this.hasher, outputPath, f.hash.value)) { + continue; + } + await this.fs.writeFile(outputPath, f.content); + written++; + } + return written; } // Marketplace build emits plugins//; user-scope tools install at diff --git a/cli/src/application/use-cases/restore/restore-tool-files-use-case.ts b/cli/src/application/use-cases/restore/restore-tool-files-use-case.ts index 3ecdf787..44eceb3e 100644 --- a/cli/src/application/use-cases/restore/restore-tool-files-use-case.ts +++ b/cli/src/application/use-cases/restore/restore-tool-files-use-case.ts @@ -31,12 +31,14 @@ export interface RestoreToolFilesOptions { interface SectionRestoreResult { restored: string[]; kept: string[]; + unrestorable: string[]; updatedFiles: InstallationFile[]; } interface MergeSectionRestoreResult { restored: string[]; kept: string[]; + unrestorable: string[]; updatedMergeFiles: MergeFileEntry[]; } @@ -45,6 +47,7 @@ export interface RestoreToolFilesResult { nothingToRestore: boolean; restored: string[]; kept: string[]; + unrestorable: string[]; } export class RestoreToolFilesUseCase { @@ -63,7 +66,13 @@ export class RestoreToolFilesUseCase { const section = await this.restoreSection(options, distMap); const mergeSection = await this.restoreMergeSection(options, distMap); if (section === null && mergeSection === null) { - return { toolId: options.toolId, nothingToRestore: true, restored: [], kept: [] }; + return { + toolId: options.toolId, + nothingToRestore: true, + restored: [], + kept: [], + unrestorable: [], + }; } return this.commitToolRestore(options, section, mergeSection); } @@ -122,7 +131,8 @@ export class RestoreToolFilesUseCase { manifest.addTool(toolId, manifest.getToolVersion(toolId) ?? version, files, mergeFiles); const restored = [...(section?.restored ?? []), ...(mergeSection?.restored ?? [])]; const kept = [...(section?.kept ?? []), ...(mergeSection?.kept ?? [])]; - return { toolId, nothingToRestore: false, restored, kept }; + const unrestorable = [...(section?.unrestorable ?? []), ...(mergeSection?.unrestorable ?? [])]; + return { toolId, nothingToRestore: false, restored, kept, unrestorable }; } private existingFilesAsGenerated( diff --git a/cli/src/application/use-cases/restore/restore-use-case.ts b/cli/src/application/use-cases/restore/restore-use-case.ts index 648c9641..c4ae313c 100644 --- a/cli/src/application/use-cases/restore/restore-use-case.ts +++ b/cli/src/application/use-cases/restore/restore-use-case.ts @@ -69,6 +69,7 @@ interface RestoreResult { totalKept: number; totalPluginFilesRestored: number; restoredPluginNames: string[]; + unrestorable: string[]; } export class RestoreUseCase { @@ -177,6 +178,7 @@ export class RestoreUseCase { totalKept: toolResults.reduce((s, t) => s + t.kept.length, 0), totalPluginFilesRestored: pluginResult.totalFiles, restoredPluginNames: pluginResult.pluginNames, + unrestorable: toolResults.flatMap((t) => t.unrestorable), }; } 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 bb36b513..3ce7b9ae 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,7 @@ 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 { materializeViaBuiltTree } from "../plugin/plugin-helpers.js"; +import { isPluginFileAtDesiredState, materializeViaBuiltTree } from "../plugin/plugin-helpers.js"; import { BuiltTreeMaterializationTranslator } from "../plugin/translator/built-tree-materialization-translator.js"; import { resolvePluginTranslator } from "../plugin/translator/resolve-plugin-translator.js"; import type { EnsureBuiltMarketplaceUseCase } from "./ensure-built-marketplace-use-case.js"; @@ -73,8 +73,15 @@ export class ApplyPluginFilesUseCase { options: ApplyPluginFilesOptions ): Promise { const { toolId, plugin, projectRoot, manifest, docsDir } = options; - await materializeViaBuiltTree(translator, dist, toolId, plugin, projectRoot, manifest, docsDir); - return manifest.getPlugins(toolId).find((p) => p.name === plugin.name)?.files.size ?? 0; + return materializeViaBuiltTree( + translator, + dist, + toolId, + plugin, + projectRoot, + manifest, + docsDir + ); } private async restoreViaTranslate( @@ -87,7 +94,7 @@ export class ApplyPluginFilesUseCase { for (const f of files) { if (fileFilter !== null && fileFilter !== undefined && !fileFilter(f.relativePath)) continue; const outputPath = join(projectRoot, f.relativePath); - if (!(await this.isFileAtDesiredState(outputPath, f.hash.value))) { + if (!(await isPluginFileAtDesiredState(this.fs, this.hasher, outputPath, f.hash.value))) { await this.fs.writeFile(outputPath, f.content); restored++; } @@ -98,13 +105,4 @@ export class ApplyPluginFilesUseCase { ); return restored; } - - private async isFileAtDesiredState( - outputPath: string, - expectedHashValue: string - ): Promise { - if (!(await this.fs.fileExists(outputPath))) return false; - const content = await this.fs.readFile(outputPath); - return this.hasher.hash(content).value === expectedHashValue; - } } diff --git a/cli/src/application/use-cases/shared/restore-drift-entries-use-case.ts b/cli/src/application/use-cases/shared/restore-drift-entries-use-case.ts index b8cbc3d6..ed675323 100644 --- a/cli/src/application/use-cases/shared/restore-drift-entries-use-case.ts +++ b/cli/src/application/use-cases/shared/restore-drift-entries-use-case.ts @@ -1,20 +1,31 @@ import type { Prompter } from "../../../domain/ports/prompter.js"; import { ResolveRestoreDecisionUseCase } from "./resolve-restore-decision.js"; -interface DriftDescriptor { +export interface DriftDescriptor { relativePath: string; reason: "deleted" | "modified"; } +/** + * What a leaf's drift scan found: entries that can actually be restored (`drift`), + * and entries the manifest still tracks as drifted but the current distribution no + * longer provides anything to restore them from (`unrestorable`) — e.g. a file + * dropped in a newer framework version, or a tool whose content set changed. + */ +export interface DriftCollection { + drift: TDrift[]; + unrestorable: DriftDescriptor[]; +} + /** * The I/O leaf: everything that differs between restoring a whole file and * merging drifted keys back into one. The skeleton below never branches on * which leaf it is running — it only calls these three methods. */ export interface RestoreDriftLeaf { - collectDrift(): Promise; + collectDrift(): Promise>; restore(entry: TDrift): Promise; - buildResult(restored: string[], kept: string[]): TResult; + buildResult(restored: string[], kept: string[], unrestorable: string[]): TResult; } /** @@ -35,8 +46,8 @@ export class RestoreDriftEntriesUseCase { force: boolean, interactive: boolean ): Promise { - const drift = await leaf.collectDrift(); - if (drift.length === 0) return null; + const { drift, unrestorable } = await leaf.collectDrift(); + if (drift.length === 0 && unrestorable.length === 0) return null; const restored: string[] = []; const kept: string[] = []; @@ -56,6 +67,10 @@ export class RestoreDriftEntriesUseCase { restored.push(entry.relativePath); } - return leaf.buildResult(restored, kept); + return leaf.buildResult( + restored, + kept, + unrestorable.map((entry) => entry.relativePath) + ); } } diff --git a/cli/src/application/use-cases/shared/restore-merge-files-use-case.ts b/cli/src/application/use-cases/shared/restore-merge-files-use-case.ts index 5b9b6fb2..cac2c371 100644 --- a/cli/src/application/use-cases/shared/restore-merge-files-use-case.ts +++ b/cli/src/application/use-cases/shared/restore-merge-files-use-case.ts @@ -9,6 +9,7 @@ import type { FileMerger } from "../../../domain/ports/file-merger.js"; import type { FileReader } from "../../../domain/ports/file-reader.js"; import type { Hasher } from "../../../domain/ports/hasher.js"; import type { Prompter } from "../../../domain/ports/prompter.js"; +import type { DriftCollection, DriftDescriptor } from "./restore-drift-entries-use-case.js"; import { RestoreDriftEntriesUseCase } from "./restore-drift-entries-use-case.js"; interface MergeDriftEntry { @@ -31,6 +32,7 @@ interface MergeFilesRestoreOptions { export interface MergeFilesRestoreResult { restored: string[]; kept: string[]; + unrestorable: string[]; updatedMergeFiles: MergeFileEntry[]; } @@ -58,9 +60,10 @@ export class RestoreMergeFilesUseCase { options.fileFilter ), restore: (entry) => this.applyOneMergeRestore(entry, options.projectRoot, mergeMap), - buildResult: (restored, kept) => ({ + buildResult: (restored, kept, unrestorable) => ({ restored, kept, + unrestorable, updatedMergeFiles: [...mergeMap.values()], }), }, @@ -74,42 +77,38 @@ export class RestoreMergeFilesUseCase { distMap: Map, projectRoot: string, fileFilter: ((p: string) => boolean) | null - ): Promise { + ): Promise> { const drift: MergeDriftEntry[] = []; + const unrestorable: DriftDescriptor[] = []; for (const entry of mergeFiles) { if (fileFilter && !fileFilter(entry.relativePath)) continue; + const reason = await this.detectMergeDrift(entry, projectRoot); + if (reason === null) continue; + + // A file the current distribution no longer merge-tracks (dropped, or its + // strategy became "none") has nothing left to restore drift from. const distFile = distMap.get(entry.relativePath); - if (!distFile || distFile.mergeStrategy === "none") continue; - const driftEntry = await this.checkOneMergeFileDrift(entry, distFile, projectRoot); - if (driftEntry) drift.push(driftEntry); + if (!distFile || distFile.mergeStrategy === "none") { + unrestorable.push({ relativePath: entry.relativePath, reason }); + continue; + } + drift.push(this.buildDriftEntry(entry, distFile, reason)); } - return drift; + return { drift, unrestorable }; } - private async checkOneMergeFileDrift( + private async detectMergeDrift( entry: MergeFileEntry, - distFile: InstallationFile, projectRoot: string - ): Promise { + ): Promise<"deleted" | "modified" | null> { const diskPath = join(projectRoot, entry.relativePath); - if (!(await this.fs.fileExists(diskPath))) { - return this.buildDriftEntry(entry, distFile, "deleted"); - } - return this.checkModifiedDrift(diskPath, entry, distFile); - } - - private async checkModifiedDrift( - diskPath: string, - entry: MergeFileEntry, - distFile: InstallationFile - ): Promise { + if (!(await this.fs.fileExists(diskPath))) return "deleted"; const diskContent = await this.fs.readFile(diskPath); const diskEntries = extractMergeEntries(diskContent, entry.sectionKey, this.hasher); const hasDrift = Object.keys(entry.entries).some( (key) => diskEntries[key]?.value !== entry.entries[key].value ); - if (!hasDrift) return null; - return this.buildDriftEntry(entry, distFile, "modified"); + return hasDrift ? "modified" : null; } private buildDriftEntry( diff --git a/cli/src/application/use-cases/shared/restore-regular-files-use-case.ts b/cli/src/application/use-cases/shared/restore-regular-files-use-case.ts index c57207e8..7cb4dc6a 100644 --- a/cli/src/application/use-cases/shared/restore-regular-files-use-case.ts +++ b/cli/src/application/use-cases/shared/restore-regular-files-use-case.ts @@ -3,6 +3,7 @@ import { type FileHash, InstallationFile } from "../../../domain/models/file.js" import type { FileReader } from "../../../domain/ports/file-reader.js"; import type { FileWriter } from "../../../domain/ports/file-writer.js"; import type { Prompter } from "../../../domain/ports/prompter.js"; +import type { DriftCollection, DriftDescriptor } from "./restore-drift-entries-use-case.js"; import { RestoreDriftEntriesUseCase } from "./restore-drift-entries-use-case.js"; interface DriftEntry { @@ -23,6 +24,7 @@ interface RegularFilesRestoreOptions { export interface RegularFilesRestoreResult { restored: string[]; kept: string[]; + unrestorable: string[]; updatedFiles: InstallationFile[]; } @@ -53,9 +55,10 @@ export class RestoreRegularFilesUseCase { await this.fs.writeFile(diskPath, entry.content); updatedHashMap.set(entry.relativePath, await this.fs.readFileHash(diskPath)); }, - buildResult: (restored, kept) => ({ + buildResult: (restored, kept, unrestorable) => ({ restored, kept, + unrestorable, updatedFiles: Array.from(updatedHashMap.entries()).map( ([relativePath, hash]) => new InstallationFile({ relativePath, content: "", hash }) ), @@ -71,38 +74,34 @@ export class RestoreRegularFilesUseCase { distMap: Map, projectRoot: string, fileFilter: ((p: string) => boolean) | null - ): Promise { + ): Promise> { const drift: DriftEntry[] = []; + const unrestorable: DriftDescriptor[] = []; for (const manifestFile of manifestFiles) { if (fileFilter && !fileFilter(manifestFile.relativePath)) continue; const diskPath = join(projectRoot, manifestFile.relativePath); - const diskExists = await this.fs.fileExists(diskPath); + const reason = await this.detectDrift(diskPath, manifestFile.hash.value); + if (reason === null) continue; - if (!diskExists) { - const distFile = distMap.get(manifestFile.relativePath); - if (distFile) - drift.push({ - relativePath: manifestFile.relativePath, - content: distFile.content, - reason: "deleted", - }); + const distFile = distMap.get(manifestFile.relativePath); + if (!distFile) { + unrestorable.push({ relativePath: manifestFile.relativePath, reason }); continue; } - - const diskHash = await this.fs.readFileHash(diskPath); - if (diskHash.value !== manifestFile.hash.value) { - const distFile = distMap.get(manifestFile.relativePath); - if (distFile) - drift.push({ - relativePath: manifestFile.relativePath, - content: distFile.content, - reason: "modified", - }); - } + drift.push({ relativePath: manifestFile.relativePath, content: distFile.content, reason }); } - return drift; + return { drift, unrestorable }; + } + + private async detectDrift( + diskPath: string, + manifestHashValue: string + ): Promise<"deleted" | "modified" | null> { + if (!(await this.fs.fileExists(diskPath))) return "deleted"; + const diskHash = await this.fs.readFileHash(diskPath); + return diskHash.value !== manifestHashValue ? "modified" : null; } } diff --git a/cli/tests/application/use-cases/shared/apply-plugin-files-built-tree.unit.test.ts b/cli/tests/application/use-cases/shared/apply-plugin-files-built-tree.unit.test.ts index 8ab6e6fc..4b4ccfbd 100644 --- a/cli/tests/application/use-cases/shared/apply-plugin-files-built-tree.unit.test.ts +++ b/cli/tests/application/use-cases/shared/apply-plugin-files-built-tree.unit.test.ts @@ -128,6 +128,39 @@ describe("RestoreAllPluginsUseCase — built-tree materialization", () => { expect([...plugins[0].files.keys()].sort()).toEqual([...installedRelPaths].sort()); }); + it("reports zero files restored when a built-tree restore finds nothing drifted", async () => { + const deps = await buildUnitDeps(PROJECT_ROOT); + await initAndInstall(deps, PROJECT_ROOT, "cursor"); + const registry = await makeRegistry(); + await installMarketplacePlugin(deps, registry); + + const manifest = await deps.manifestRepo.load(); + if (manifest === null) throw new Error("manifest not found"); + const installedRelPaths = [ + ...(manifest + .getPlugins("cursor") + .find((p) => p.name === "sample-plugin") + ?.files.keys() ?? []), + ]; + expect(installedRelPaths.length).toBeGreaterThan(0); + const builtContent = deps.fs.getFile(BUILT_SKILL); + if (builtContent === undefined) throw new Error("built fixture missing"); + // Seed the user-scope plugin dir with exactly what the built tree already + // materializes, so this restore has nothing left to change. + for (const relativePath of installedRelPaths) { + await deps.fs.writeFile(join(USER_PLUGINS_DIR, relativePath), builtContent); + } + + const result = await makeRestoreUseCase(deps, registry).execute({ + projectRoot: PROJECT_ROOT, + manifest, + docsDir: DOCS_DIR, + fileFilter: null, + }); + + expect(result.totalFiles).toBe(0); + }); + it("keeps the manifest's single entry for the plugin after restore (no duplicate registration)", async () => { const deps = await buildUnitDeps(PROJECT_ROOT); await initAndInstall(deps, PROJECT_ROOT, "cursor"); diff --git a/cli/tests/application/use-cases/shared/restore-merge-files-use-case.unit.test.ts b/cli/tests/application/use-cases/shared/restore-merge-files-use-case.unit.test.ts index a3e9a3c9..d07c881a 100644 --- a/cli/tests/application/use-cases/shared/restore-merge-files-use-case.unit.test.ts +++ b/cli/tests/application/use-cases/shared/restore-merge-files-use-case.unit.test.ts @@ -205,7 +205,7 @@ describe("RestoreMergeFilesUseCase", () => { expect(deps.fs.getFile(join(PROJECT_ROOT, "b.json"))).toBe(JSON.stringify({ x: "disk" })); }); - it("skips merge files whose distribution strategy is 'none'", async () => { + it("reports a drifted merge file as unrestorable when the distribution strategy is 'none'", async () => { const deps = await buildDeps(); await deps.fs.writeFile(join(PROJECT_ROOT, "a.json"), JSON.stringify({ x: "disk" })); const useCase = new RestoreMergeFilesUseCase(deps.fs, deps.hasher, new OverwritePrompter()); @@ -231,6 +231,43 @@ describe("RestoreMergeFilesUseCase", () => { fileFilter: null, }); + expect(result?.unrestorable).toEqual(["a.json"]); + expect(result?.restored).toEqual([]); + expect(result?.kept).toEqual([]); + expect(deps.fs.getFile(join(PROJECT_ROOT, "a.json"))).toBe(JSON.stringify({ x: "disk" })); + }); + + it("returns null when a merge file's distribution strategy is 'none' and nothing drifted", async () => { + const deps = await buildDeps(); + const distContent = JSON.stringify({ x: "framework" }); + await deps.fs.writeFile(join(PROJECT_ROOT, "a.json"), distContent); + const useCase = new RestoreMergeFilesUseCase(deps.fs, deps.hasher, new OverwritePrompter()); + + const result = await useCase.execute({ + mergeFiles: [ + { + relativePath: "a.json", + sectionKey: null, + entries: { x: deps.hasher.hash('"framework"') }, + }, + ], + distMap: new Map([ + [ + "a.json", + new InstallationFile({ + relativePath: "a.json", + content: distContent, + hash: deps.hasher.hash(distContent), + mergeStrategy: "none", + }), + ], + ]), + projectRoot: PROJECT_ROOT, + force: true, + interactive: false, + fileFilter: null, + }); + expect(result).toBeNull(); }); diff --git a/cli/tests/application/use-cases/shared/restore-regular-files-use-case.unit.test.ts b/cli/tests/application/use-cases/shared/restore-regular-files-use-case.unit.test.ts index 93f19d59..3d208eeb 100644 --- a/cli/tests/application/use-cases/shared/restore-regular-files-use-case.unit.test.ts +++ b/cli/tests/application/use-cases/shared/restore-regular-files-use-case.unit.test.ts @@ -268,7 +268,7 @@ describe("RestoreRegularFilesUseCase", () => { expect(deps.fs.getFile(join(PROJECT_ROOT, "b.md"))).toBe("disk modified b"); }); - it("silently drops a deleted file that has no corresponding entry in the dist map", async () => { + it("reports a deleted file with no corresponding dist entry as unrestorable, without touching disk", async () => { const deps = await buildDeps(); const useCase = new RestoreRegularFilesUseCase(deps.fs, new OverwritePrompter()); @@ -281,11 +281,13 @@ describe("RestoreRegularFilesUseCase", () => { fileFilter: null, }); - expect(result).toBeNull(); + expect(result?.unrestorable).toEqual(["a.md"]); + expect(result?.restored).toEqual([]); + expect(result?.kept).toEqual([]); expect(deps.fs.has(join(PROJECT_ROOT, "a.md"))).toBe(false); }); - it("silently drops a modified file that has no corresponding entry in the dist map", async () => { + it("reports a modified file with no corresponding dist entry as unrestorable, without touching disk", async () => { const deps = await buildDeps(); await deps.fs.writeFile(join(PROJECT_ROOT, "a.md"), "disk modified content"); const useCase = new RestoreRegularFilesUseCase(deps.fs, new OverwritePrompter()); @@ -299,7 +301,9 @@ describe("RestoreRegularFilesUseCase", () => { fileFilter: null, }); - expect(result).toBeNull(); + expect(result?.unrestorable).toEqual(["a.md"]); + expect(result?.restored).toEqual([]); + expect(result?.kept).toEqual([]); expect(deps.fs.getFile(join(PROJECT_ROOT, "a.md"))).toBe("disk modified content"); }); });