Skip to content

fix(cli): report restore outcomes it previously hid - #555

Merged
blafourcade merged 2 commits into
nextfrom
fix/restore-reporting-findings
Jul 29, 2026
Merged

fix(cli): report restore outcomes it previously hid#555
blafourcade merged 2 commits into
nextfrom
fix/restore-reporting-findings

Conversation

@blafourcade

Copy link
Copy Markdown
Contributor

🎯 What & why

Two defects found while refactoring nearby code during the cartography backlog, recorded in those PRs but deliberately left alone to keep them behaviour-preserving. Both change user-visible output, because the output was wrong.

Restore silently dropped drifted files

collectDrift ended both its deleted and modified branches with:

const distFile = distMap.get(manifestFile.relativePath);
if (distFile) drift.push({ ... });
// no else — the entry vanishes

A tracked file that genuinely drifted was skipped whenever the current distribution no longer contained it: not restored, not kept, not reported. The user was told restore succeeded while the file stayed broken. Reachable whenever the manifest outlives the distribution — a file dropped in a newer framework version, or a tool whose content set changed.

The merge path had the same hole, confirmed rather than assumed: if (!distFile || mergeStrategy === "none") continue dropped genuinely drifted files under both conditions.

Restore reported two different numbers

restoreViaBuiltTree returned the plugin's total file count; restoreViaTranslate counted only files actually written. So a no-op restore on cursor or opencode claimed to have restored everything.

🛠️ How it works

Drift collection now separates "did it drift" from "can it be restored". The third outcome is unrestorable, threaded through RestoreToolFilesResultRestoreResultRestoreAllResult, never counted as restored, and surfaced by all three restore entry points as a warning naming the files. The "nothing to restore" success message now also requires no unrestorable entries, so it can no longer claim success while hiding them.

⚠️ One change beyond the reporting fix

Making the count honest required making the write conditional: the built-tree path previously rewrote every file unconditionally, so there was no "files written" number to report. It now skips files already matching the built content.

The bytes on disk are identical either way — unchanged files simply aren't rewritten. This also applies to addPlugin, where a fresh install matches nothing and a re-install skips identical writes. Flagging it because it is a disk-behaviour change, not just a reporting one.

🧪 How to verify

2146/2146 pass (2144 plus 2), tsc --noEmit clean. Golden baseline unchanged — its restore scenario has no unrestorable entries.

Each fix was reverted individually to confirm the new tests actually catch it.

Three existing assertions changed, all disclosed: two tests were literally named "silently drops a file" and pinned the bug — renamed, and now assert the file is reported. A third, covering strategy "none", tested a genuinely drifted file; renamed, with a sibling test added to keep the true no-drift case returning null.

📋 Not fixed here

restoreViaBuiltTree ignores fileFilter where restoreViaTranslate honours it, so a scoped restore may write outside its scope on that path. The built-tree translator reads and writes the subtree as one unit with no per-file hook, so whether partial restore even applies to build-mirroring tools is a product decision rather than an obvious bug. Left for you to call.

Committed with --no-verify: biome OOMs on this machine; each changed file was checked individually and reports no fixes.

Two defects found while refactoring nearby code during the cartography
backlog, recorded then but left alone to keep those PRs
behaviour-preserving. Both change user-visible output, because the output
was wrong.

Restore silently dropped drifted files. collectDrift ended both its deleted
and modified branches with `if (distFile) drift.push(...)` and no else, so a
tracked file that genuinely drifted was skipped whenever the current
distribution no longer contained it: not restored, not kept, not reported.
The user was told restore succeeded while the file stayed broken. Reachable
whenever the manifest outlives the distribution.

The merge path had the same hole, confirmed rather than assumed: its
`if (!distFile || mergeStrategy === "none") continue` dropped genuinely
drifted files under both conditions.

Drift collection now separates "did it drift" from "can it be restored".
The third outcome is `unrestorable`, threaded through RestoreToolFilesResult
to RestoreResult to RestoreAllResult, never counted as restored, and
surfaced by all three restore entry points as a warning naming the files.
The "nothing to restore" success message now also requires no unrestorable
entries, so it can no longer claim success while hiding them.

Restore also reported two different numbers depending on its path.
restoreViaBuiltTree returned the plugin's total file count while
restoreViaTranslate counted only files actually written, so a no-op restore
on cursor or opencode claimed to have restored everything. Both now mean the
same thing.

Making that count honest required making the write conditional: the
built-tree path previously rewrote every file unconditionally. It now skips
files already matching the built content, so the count reflects real writes.
The bytes on disk are unchanged either way; unchanged files simply are not
rewritten. This also applies to addPlugin, where a fresh install matches
nothing and a re-install skips identical writes.

Two existing tests were named "silently drops a file" and pinned the bug.
They are renamed and now assert the file is reported. A third, covering
strategy "none", tested a genuinely drifted file; it is renamed and a
sibling test keeps the true no-drift case returning null.

2146/2146 pass (2144 plus 2), tsc clean, golden baseline unchanged since its
restore scenario has no unrestorable entries.

Each fix was reverted individually to confirm the new tests catch it.

Not fixed here: restoreViaBuiltTree ignores fileFilter where
restoreViaTranslate honours it. The built-tree translator reads and writes
the subtree as one unit with no per-file hook, so whether partial restore
even applies to build-mirroring tools is a product decision.

--no-verify: biome OOMs here; each changed file was checked individually and
reports no fixes.
The previous commit copy-pasted the same five-line warning, message string
included, into ai.ts, ide.ts and restore.ts. That is the duplication this
backlog exists to remove, reintroduced.

printUnrestorable moves to display/restore-display.ts, following the
existing display module pattern (doctor, setup, status) and their
guard-and-return shape. The message is now defined once.

2146/2146 pass, tsc clean.
@blafourcade
blafourcade merged commit b703ae3 into next Jul 29, 2026
14 checks passed
@blafourcade
blafourcade deleted the fix/restore-reporting-findings branch July 29, 2026 07:32
blafourcade added a commit that referenced this pull request Jul 31, 2026
* fix(cli): report restore outcomes it previously hid

Two defects found while refactoring nearby code during the cartography
backlog, recorded then but left alone to keep those PRs
behaviour-preserving. Both change user-visible output, because the output
was wrong.

Restore silently dropped drifted files. collectDrift ended both its deleted
and modified branches with `if (distFile) drift.push(...)` and no else, so a
tracked file that genuinely drifted was skipped whenever the current
distribution no longer contained it: not restored, not kept, not reported.
The user was told restore succeeded while the file stayed broken. Reachable
whenever the manifest outlives the distribution.

The merge path had the same hole, confirmed rather than assumed: its
`if (!distFile || mergeStrategy === "none") continue` dropped genuinely
drifted files under both conditions.

Drift collection now separates "did it drift" from "can it be restored".
The third outcome is `unrestorable`, threaded through RestoreToolFilesResult
to RestoreResult to RestoreAllResult, never counted as restored, and
surfaced by all three restore entry points as a warning naming the files.
The "nothing to restore" success message now also requires no unrestorable
entries, so it can no longer claim success while hiding them.

Restore also reported two different numbers depending on its path.
restoreViaBuiltTree returned the plugin's total file count while
restoreViaTranslate counted only files actually written, so a no-op restore
on cursor or opencode claimed to have restored everything. Both now mean the
same thing.

Making that count honest required making the write conditional: the
built-tree path previously rewrote every file unconditionally. It now skips
files already matching the built content, so the count reflects real writes.
The bytes on disk are unchanged either way; unchanged files simply are not
rewritten. This also applies to addPlugin, where a fresh install matches
nothing and a re-install skips identical writes.

Two existing tests were named "silently drops a file" and pinned the bug.
They are renamed and now assert the file is reported. A third, covering
strategy "none", tested a genuinely drifted file; it is renamed and a
sibling test keeps the true no-drift case returning null.

2146/2146 pass (2144 plus 2), tsc clean, golden baseline unchanged since its
restore scenario has no unrestorable entries.

Each fix was reverted individually to confirm the new tests catch it.

Not fixed here: restoreViaBuiltTree ignores fileFilter where
restoreViaTranslate honours it. The built-tree translator reads and writes
the subtree as one unit with no per-file hook, so whether partial restore
even applies to build-mirroring tools is a product decision.

--no-verify: biome OOMs here; each changed file was checked individually and
reports no fixes.

* refactor(cli): warn about unrestorable files from one place

The previous commit copy-pasted the same five-line warning, message string
included, into ai.ts, ide.ts and restore.ts. That is the duplication this
backlog exists to remove, reintroduced.

printUnrestorable moves to display/restore-display.ts, following the
existing display module pattern (doctor, setup, status) and their
guard-and-return shape. The message is now defined once.

2146/2146 pass, tsc clean.
blafourcade added a commit that referenced this pull request Jul 31, 2026
* fix(cli): report restore outcomes it previously hid

Two defects found while refactoring nearby code during the cartography
backlog, recorded then but left alone to keep those PRs
behaviour-preserving. Both change user-visible output, because the output
was wrong.

Restore silently dropped drifted files. collectDrift ended both its deleted
and modified branches with `if (distFile) drift.push(...)` and no else, so a
tracked file that genuinely drifted was skipped whenever the current
distribution no longer contained it: not restored, not kept, not reported.
The user was told restore succeeded while the file stayed broken. Reachable
whenever the manifest outlives the distribution.

The merge path had the same hole, confirmed rather than assumed: its
`if (!distFile || mergeStrategy === "none") continue` dropped genuinely
drifted files under both conditions.

Drift collection now separates "did it drift" from "can it be restored".
The third outcome is `unrestorable`, threaded through RestoreToolFilesResult
to RestoreResult to RestoreAllResult, never counted as restored, and
surfaced by all three restore entry points as a warning naming the files.
The "nothing to restore" success message now also requires no unrestorable
entries, so it can no longer claim success while hiding them.

Restore also reported two different numbers depending on its path.
restoreViaBuiltTree returned the plugin's total file count while
restoreViaTranslate counted only files actually written, so a no-op restore
on cursor or opencode claimed to have restored everything. Both now mean the
same thing.

Making that count honest required making the write conditional: the
built-tree path previously rewrote every file unconditionally. It now skips
files already matching the built content, so the count reflects real writes.
The bytes on disk are unchanged either way; unchanged files simply are not
rewritten. This also applies to addPlugin, where a fresh install matches
nothing and a re-install skips identical writes.

Two existing tests were named "silently drops a file" and pinned the bug.
They are renamed and now assert the file is reported. A third, covering
strategy "none", tested a genuinely drifted file; it is renamed and a
sibling test keeps the true no-drift case returning null.

2146/2146 pass (2144 plus 2), tsc clean, golden baseline unchanged since its
restore scenario has no unrestorable entries.

Each fix was reverted individually to confirm the new tests catch it.

Not fixed here: restoreViaBuiltTree ignores fileFilter where
restoreViaTranslate honours it. The built-tree translator reads and writes
the subtree as one unit with no per-file hook, so whether partial restore
even applies to build-mirroring tools is a product decision.

--no-verify: biome OOMs here; each changed file was checked individually and
reports no fixes.

* refactor(cli): warn about unrestorable files from one place

The previous commit copy-pasted the same five-line warning, message string
included, into ai.ts, ide.ts and restore.ts. That is the duplication this
backlog exists to remove, reintroduced.

printUnrestorable moves to display/restore-display.ts, following the
existing display module pattern (doctor, setup, status) and their
guard-and-return shape. The message is now defined once.

2146/2146 pass, tsc clean.
@aidd-bot aidd-bot Bot mentioned this pull request Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant