feat(cleaner): opt-in log of deleted file paths (#247) - #263
Conversation
validateSettingsPartial rejects the entire payload when it sees a key that isn't on its allowlist, and neither cleaner.protectRecycleBin nor the top-level backupMode was listed. SettingsPage spreads the whole cleaner object on every toggle, so each control in Cleaning Preferences — and the registry backup-mode dropdown — failed validation and never reached disk. The renderer updates its own store optimistically, so the UI looked correct until the next restart reloaded the old value. Add both keys to the allowlists with type/value checks, and cover the exact payload shape the settings UI sends so a future key can't regress the whole section again.
Scan History recorded only per-category counts and bytes, so there was no way to answer "which files did that clean actually remove?" — the paths were in hand inside cleanItems and thrown away, kept only for failures. That makes it impossible to check whether a cleanup is what broke an application afterwards. Add a Cleaning Preferences toggle, "Keep a log of deleted files", that records every removed path to deleted-files.jsonl in the Kudu data folder. Off by default: the log is a plaintext index of file paths, and that is not something to start writing without being asked. Implementation notes: - cleanItems is the single chokepoint for all file deletion, so one hook covers manual, scheduled, CLI and cloud-agent cleans alike. Records are buffered and flushed every 500 entries, so a six-figure clean neither balloons memory nor turns into a write per file, and a crash mid-clean still leaves everything deleted up to that point on disk. - JSONL rather than JSON, rotating at 8 MB, so appends stay O(1) and a truncated tail costs one line instead of the whole file. - History entries carry the clean phase's start/end timestamps rather than the paths themselves. One "System Clean" fans out across eight separate IPC calls, so a time window keeps the record-side change to cleanItems alone and keeps six figures of paths off the IPC boundary. - The History detail popup gains a lazily-loaded, paged list of the paths in that window, with CSV export and a reveal-in-folder action. Entries from before this change simply have no window and show nothing. - Clearing Scan History clears the deletion log too — leaving it behind would make "Clear" a half-truth. Registry, debloater, driver and privacy actions raise the same question but have different item shapes; this covers file cleaners only.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af7440ff16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cleanedFrom: new Date(cleanStartRef.current).toISOString(), | ||
| cleanedTo: new Date().toISOString(), |
There was a problem hiding this comment.
When a cloud-agent cleanup overlaps a manual cleanup, src/main/services/cloud-agent.ts:2334 calls the same cleanItems function and appends indistinguishable records, while this history entry later selects records solely by the inclusive wall-clock bounds stored here. Its detail list and CSV can therefore attribute remotely deleted paths to the manual run; persist a per-run identifier on each record and query by that identifier instead.
Useful? React with 👍 / 👎.
| // Opt-in audit trail of what was removed (issue #247). Buffered so a clean of | ||
| // 100k files doesn't turn into 100k appends, and flushed as we go so a crash | ||
| // mid-clean still leaves a record of everything deleted up to that point. | ||
| const logDeletions = getSettings().cleaner.keepDeletionLog === true |
There was a problem hiding this comment.
On Windows, when Recycle Bin protection is disabled and that Cleaner category is selected, src/main/ipc/recycle-bin.ipc.ts:93-109 empties the bin through SHEmptyRecycleBin without calling cleanItems. Because logging is enabled only in this cleanItems hook, the resulting history entry reports the cleanup but shows no deleted paths even with the new setting enabled; capture the bin items before emptying it and record the successful deletions.
Useful? React with 👍 / 👎.
| if (logDeletions) { | ||
| pending.push({ | ||
| ts: new Date().toISOString(), | ||
| path: item.path, |
There was a problem hiding this comment.
When a cached scan item is a directory, safeDelete removes its complete contents recursively, but this record contains only the directory's path. Consequently the per-file history and CSV omit every descendant file that was actually removed, which prevents the audit trail from answering which file caused a later problem; when logging is enabled, collect and record the descendant paths before recursively deleting the directory.
Useful? React with 👍 / 👎.
Review feedback on #263, all three cases where the log could mislead rather than merely omit. Attribute records to the run that made them. A cloud-triggered clean calls the same cleanItems and its records interleave with a manual run's by wall-clock time, so History could list remotely deleted paths as part of a local clean. Records now carry an origin ('local', 'cloud', 'cli') and History queries its own. Records written before this carry no origin and read as local, which is where they came from. Expand directories instead of naming them. A cached scan item is often a whole directory, and rm removes it recursively — so a single record hid every file that actually went, which is exactly what the audit trail exists to answer. Descendants are now enumerated before the delete and recorded on success, capped at 100k per item with the overflow count kept on the directory's record so a truncated list can't read as a complete one. Symlinks and junctions are listed but not descended into, matching what rm removes. Descendants carry size 0 — the bytes stay on the directory record, so a CSV sum doesn't double-count and no per-file stat is needed. Cover the Windows Recycle Bin. SHEmptyRecycleBin destroys the bin wholesale without going through cleanItems, so the one clean that permanently destroys recoverable files logged nothing at all. Its contents are now enumerated by original location before emptying, and on a partial empty the survivors are re-read and diffed rather than guessed at, so the log never names a file that is still there.
|
Thanks — all three were real, all three are fixed in 0aa8f34. They shared a theme worth naming: each was a case where the log could mislead rather than merely omit, which for an audit trail is the failure that matters. 1. Attribution across overlapping runs. Correct — 2. Recursive directory deletion. The sharpest of the three: a cached scan item is frequently a whole directory, so the log named the folder and hid every file that actually went — precisely the question it exists to answer. Descendants are now enumerated before the delete and recorded on success, capped at 100k per item with the overflow count kept on the directory's record so a truncated list can't read as a complete one (the UI badges it). Symlinks and junctions are listed but not descended into, matching what 3. Windows Recycle Bin. Also correct, and the worst omission of the three, since emptying the bin is the one clean that destroys recoverable files. Contents are now enumerated by original location before All three are enumerated only when logging is enabled, so the default-off path costs nothing. Coverage: 2314 tests / 100 files, up 14 — origin filtering and legacy-record fallback, recursive expansion (nesting, size attribution, no-expansion-when-off, nothing-logged-on-failed-delete), and a new Also: the translation bot picked up the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0aa8f3464e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async function listDescendantFiles(dirPath: string): Promise<{ paths: string[]; truncated: number } | null> { | ||
| let rootEntries: Dirent[] | ||
| try { | ||
| rootEntries = await readdir(dirPath, { withFileTypes: true }) |
There was a problem hiding this comment.
When a scanned item is itself a symlink to a directory, scanDirectory includes it because stat follows the link, and this initial readdir(dirPath) follows it again. The descendant list therefore contains the target directory's files, but rm(..., { recursive: true }) removes only the symlink; with logging enabled, History and CSV falsely report all target files as deleted. Check the root with lstat and skip expansion when it is a symbolic link.
Useful? React with 👍 / 👎.
| const result = await safeDelete(item.path) | ||
| if (result.success) { | ||
| totalCleaned += item.size | ||
| filesDeleted++ | ||
| if (logDeletions) { | ||
| const ts = new Date().toISOString() | ||
| const category = item.subcategory || item.category | ||
| const record: DeletedFileRecord = { ts, path: item.path, size: item.size, category, origin } |
There was a problem hiding this comment.
When a cached file disappears after scanning but before cleaning—a common race for temporary files—rm with force: true reports success, so this branch writes a deletion record even though Kudu removed nothing. This makes the new audit trail incorrectly attribute an external deletion to the clean; establish that the path existed immediately before deletion or distinguish the missing-path result before appending the record.
Useful? React with 👍 / 👎.
| let recycleCleaned: CleanResult = { totalCleaned: 0, filesDeleted: 0, filesSkipped: 0, errors: [], needsElevation: false } | ||
| let dbCleaned: CleanResult = { totalCleaned: 0, filesDeleted: 0, filesSkipped: 0, errors: [], needsElevation: false } | ||
| if (fileItemIds.length > 0) fileCleaned = await cleanItems(fileItemIds) | ||
| if (fileItemIds.length > 0) fileCleaned = await cleanItems(fileItemIds, undefined, 'cli') |
There was a problem hiding this comment.
When kudu --cli clean includes the Recycle Bin on Windows and deletion logging is enabled, this changed flow tags ordinary file deletions as cli but still calls the separate cleanRecycleBin helper at line 1539; that helper deletes through PowerShell without enumerating or calling recordDeletions. Thus the CLI clean reports success while every Recycle Bin path is absent from the log. The fresh evidence beyond the earlier IPC comment is this independent CLI-only deletion implementation.
Useful? React with 👍 / 👎.
Second review pass on #263. Same failure class as before: cases where the log states something false rather than merely omitting it. Don't expand a symlinked directory. scanDirectory stats through links, so a symlink to a directory arrives as an ordinary directory item, and readdir would then list the target's contents — while rm(recursive) only unlinks the link. Every file in the target would have been logged as deleted while still sitting on disk. The root is now checked with lstat, which reports the link itself, so only the link gets recorded. Don't log paths that were already gone. rm(force) reports success for a path that no longer exists, so a temp file that disappeared between the scan and the clean — an ordinary race for the directories Kudu cleans — was recorded as something Kudu removed, attributing a third-party deletion to us. The same lstat answers this: no entry, no record. The existing counters keep their current behavior; only the audit trail is held to what we actually removed. Cover the CLI's recycle bin too. `kudu --cli clean` empties the bin through its own PowerShell helper rather than the IPC handler fixed last time, so Windows CLI cleans still logged nothing. The enumerate, diff and record logic moves to services/recycle-bin-log.ts and both callers use it, tagged 'local' and 'cli' respectively — one implementation instead of a third copy to keep in sync.
|
Round two — all three valid again, fixed in a252b80. Same failure class as last time: the log stating something false rather than merely omitting it. Root symlink expansion. Good catch, and the nastiest of the set. Already-missing paths. Also right, and it's the more common one in practice given what Kudu cleans — temp directories churn constantly between scan and clean. CLI recycle bin. Correct that it's a distinct implementation from the IPC handler fixed last round; 2326 tests / 101 files, up 12: a service-level One test went the other way: Translations for the two keys added last round landed via the bot in 39531ec, so all 30 locales are current. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| @@ -1,10 +1,12 @@ | |||
| import { rm, stat, readdir, open, writeFile } from 'fs/promises' | |||
| import { rm, stat, lstat, readdir, open, writeFile } from 'fs/promises' | |||
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a252b80bc0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const survivors = new Set((await listRecycleBinContents()).map((entry) => entry.path)) | ||
| const emptied = before.filter((entry) => !survivors.has(entry.path)) |
There was a problem hiding this comment.
When the post-clean PowerShell enumeration fails in either Windows IPC or CLI flow, listRecycleBinContents() catches the error and returns the same empty array used for a genuinely empty bin. This line consequently treats every pre-clean entry as deleted, so a transient COM/PowerShell failure—especially after a partial empty—creates false deletion records for files still in the Recycle Bin; propagate a distinguishable failure result and skip recording when survivor state is unknown.
Useful? React with 👍 / 👎.
| if (paths.length >= MAX_LOGGED_DESCENDANTS) { | ||
| truncated++ | ||
| continue |
There was a problem hiding this comment.
When the 100,000-path cap is reached and a subsequent entry is a directory, this branch increments truncated only once and skips descending into that directory. For example, an omitted directory containing 10,000 files produces a +1 more badge even though 10,001 paths were removed, making the audit trail materially understate its truncation; either count the skipped subtree or represent truncation without an exact count.
Useful? React with 👍 / 👎.
1 participant
Closes #247.
The gap
Scan History records per-category counts and bytes, nothing else. The paths are in hand inside
cleanItemsand thrown away — kept only for failures, and even those get flattened toerrorCountbefore they reachhistory.json. So "which files did that 16 GB clean actually remove?" has no answer, which is exactly the question you want answered when an app starts misbehaving after a cleanup.What this adds
A Cleaning Preferences toggle, "Keep a log of deleted files", off by default. When on, every removed path is appended to
deleted-files.jsonlin the Kudu data folder, and the Scan History detail popup grows a paged list of the paths behind that run, with CSV export and reveal-in-folder.Off by default because the log is a plaintext index of file paths — not something to start writing without being asked. Nothing changes for anyone who doesn't turn it on.
Design notes
cleanItemsis the single chokepoint for file deletion, so manual, scheduled, CLI and cloud-agent cleans are all covered without touching the eight per-category IPC handlers.cleanedFrom/cleanedTokeeps the record-side change confined tocleanItemsand keeps six figures of paths off the IPC boundary. Pre-existing entries have no window and simply show nothing.Also in here (separate commit)
fix(settings): persist cleaning preferences and registry backup modeWhile wiring the new toggle I found
validateSettingsPartialrejects the entire payload on any unknown key, and neithercleaner.protectRecycleBinnor top-levelbackupModewas on its allowlist. Since SettingsPage spreads the wholecleanerobject on every toggle, every control in Cleaning Preferences and the registry backup-mode dropdown currently fails to persist — the renderer updates optimistically, so it looks saved until the next restart. The new toggle would have hit the same wall. Fixed with both keys allowlisted and a test covering the exact payload shape the UI sends, so the next added key can't silently break the section again.Scope
File cleaners only. Registry, debloater, driver and privacy actions raise the same question but have different item shapes — worth a follow-up, called out rather than half-delivered.
Testing
npm test— 2300 passed / 99 files.cleanItemshook (off by default, batch flush boundary at 500/1000/1200, subcategory fallback), plus validator and channel coverage.npm run buildclean.tsc --noEmitshows no new errors — the two that remain ontsconfig.web.jsonreproduce identically onmain.Follow-up needed
New UI strings were added to
enonly.npm run translateneeds an API key I don't have, so the other 30 locales fall back to English until a maintainer runs it.