Skip to content

fix: close every open issue — glob rules, quarantine manifest, honest totals, the graph stage, and the advisory backlog - #63

Merged
Muawiya-contact merged 33 commits into
mainfrom
fix/open-issue-sweep
Sep 5, 2026
Merged

fix: close every open issue — glob rules, quarantine manifest, honest totals, the graph stage, and the advisory backlog#63
Muawiya-contact merged 33 commits into
mainfrom
fix/open-issue-sweep

Conversation

@Muawiya-contact

Copy link
Copy Markdown
Member

What & why

Every open issue in the tracker, closed in one pass: nine engine and app
bugs, and the seventeen RUSTSEC advisories the audit bot has been filing
since it was written.

One commit per change, all verified locally — cargo fmt --check,
cargo clippy --workspace --all-targets -D warnings (the Tauri crate
included), cargo test (43 tests, 25 of them new), plus an end-to-end
diskern scan over a fixture tree.

Safety bugs

#41 — rule patterns match anywhere in a path. classify used
p.contains(pat), so nothing anchored a pattern to a path component:
/tmp/ fired on /home/user/tmp/tax-return.pdf, /var/log/ on
~/var/log, .dmg on holiday.dmgx. All three come back review,
which is actionable — the app offered to move user data on a substring
match. Patterns are globs now (globset, literal_separator), which the
existing TODO on the field already named. Compiled matchers are cached
in a OnceLock so classification stays one glob test per entry.

#40 — the Firefox rule covered the whole profile.
/mozilla/firefox/profiles with verdict safe matched
%APPDATA%\Mozilla\Firefox\Profiles\<id>\, so places.sqlite,
logins.json and cookies.sqlite were listed under "Safe to remove".
The rule now names cache2, which is the cache it was always about.

#46 — excludes compared characters, not paths. /run also excluded
/runtime-data, and a root typed c:\windows\winsxs never matched the
C:\Windows\WinSxS exclude. Same normalization the rules use, compared
component-wise.

#43 — quarantine was not reversible. The module doc promised a
manifest recording original locations; there wasn't one. The only record
was the value quarantine() returned, and App.jsx dropped it — and
the flattened filenames can't stand in, since / and _ collapse onto
the same character. Now: a JSONL manifest, list/restore/purge in
core, three Tauri commands, and a Quarantine panel that renders before
any scan so files from an earlier session are restorable. The same
collision that made filenames useless could also make two originals land
on one quarantine path and silently overwrite; destinations are unique
now.

#42restore() failed across filesystems. quarantine() handled
EXDEV and fell back to copy+remove; restore() was a bare rename, so the
exact case the move survived was the case the undo failed on — and it is
the normal case, since quarantine lives in app-local-data and scans run
wherever the user points them.

Correctness

#44 — the reclaimable headline overstated twice. Findings and
duplicate waste were added, and a file is often both: two identical 1 GB
installers under /tmp are two findings and a 1 GB duplicate set, so
the headline said 3 GB where 2 GB is everything there is. Duplicate sets
now add only the redundant copies nothing has counted. Protected files
also sat out dedup entirely — they were contributing wasted bytes to a
total the app will never act on, and they are the most expensive things
to hash.

#47min_file_size filtered the whole walk. Documented as "skip
tiny files for dedup purposes", applied inside the walk, so a file under
it never reached the rules engine, the risk model, the report or
files_scanned. Moved to ReportOptions::dedup_min_size, where the doc
comment always said it lived.

#45 — the progress ticker could outlive its scan. Cleanup ran as
statements after the .await, which only run if control reaches them —
and dropping a Tauri command's future skips them entirely. Both cleanups
moved into a ScanRun guard that runs from Drop.

The graph

#48 — the graph stage was in the diagram, not the pipeline.
risk::downgrade had no callers anywhere in the workspace, so a
node_modules three live projects depend on got the same verdict and
the same reasons as an abandoned one. report::build now builds an
ImpactGraph from the entries and feeds referencing_projects through
downgrade. Workspace members that hoist to a shared store all count,
which is where "referenced by 3 projects" comes from rather than always
being 1; a package.json inside node_modules marks nothing, or one
store would look like ten thousand projects.

Two knock-ons, both called out rather than smuggled in:

  • Risky is reachable for the first time, so its bytes stop counting as
    reclaimable — actions::quarantine refuses Risky and the UI renders
    no action for it, so counting them promises space the app won't free.
  • Both front ends printed reasons[0], which is always the matched
    rule, so the new evidence reached the report and stopped there. They
    print every reason now.

The advisory issues (#23#39, #55)

cargo audit reports zero vulnerabilities and seventeen
informational warnings — sixteen unmaintained crates, one unsound. All
seventeen are one dependency wearing seventeen hats: Tauri v2 renders on
Linux through webkit2gtk-4.1, which links the gtk-rs GTK3 bindings,
archived upstream in 2024. That is ten of them; plus glib's unsound
VariantStrIter, fixed in the 0.19 line the GTK3 bindings never moved
to; plus proc-macro-error via glib-macros; plus five unic-* via
urlpattern in tauri-utils. Verified with cargo tree -i. None has
a version that clears it
, so neither the audit job nor the weekly
auto-fix PR has anywhere to go.

Which is the real risk here. The audit job fails while advisories stand,
so it has been red every Monday since it was written and stays red until
Tauri moves to GTK4 — and a check that is always red stops being read.
The eighteenth advisory, in a crate we actually chose, would land in a
run nobody looks at.

So they are listed in .cargo/audit.toml with a reason each and a review
date, and documented in docs/DEPENDENCY-AUTOMATION.md. Verified
locally against a pinned advisory-db: 17 reported without the file, 0
with it
, ids matching exactly. ignore suppresses only those ids — a
new advisory against these same crates still fails, as does anything
else in the tree — the ids are printed into the job summary on every run
so a suppression can't outlive its reason quietly, and editing the file
re-triggers the audit the way moving the lockfile does.
rustsec/audit-check doesn't read the config file, so the workflow
greps the ids out of it rather than keeping a second list to drift.

If you'd rather keep these open as a standing reminder, revert the last
chore(audit) commit — the other 21 stand on their own.

Behaviour changes worth knowing

  • ScanOptions::min_file_size is gone; it is ReportOptions::dedup_min_size.
  • RulesDb gained a private field, so build it with RulesDb::new or
    RulesDb::embedded rather than a struct literal. classify is
    unchanged.
  • Rules database bumped to version 3 (glob patterns).
  • A node_modules next to a live package.json is now Risky rather
    than Review, so the app no longer offers to quarantine it.

Checklist

  • cargo fmt --all and cargo clippy --workspace are clean
  • cargo test --workspace passes
  • Commits are small and focused (one logical change each)
  • Doesn't weaken a safety principle (read-only scans, quarantine
    over deletion, deterministic verdicts) — purge is the one new
    deletion, it is an explicit user action the README already
    described, and it only removes what the manifest says Diskern put
    there

Closes #23
Closes #24
Closes #25
Closes #26
Closes #27
Closes #28
Closes #29
Closes #30
Closes #31
Closes #32
Closes #33
Closes #34
Closes #35
Closes #36
Closes #37
Closes #38
Closes #39
Closes #40
Closes #41
Closes #42
Closes #43
Closes #44
Closes #45
Closes #46
Closes #47
Closes #48
Closes #55

`classify` tested patterns with `p.contains(pat)`, so nothing tied a
pattern to a path component and rules fired well outside the directories
they name: `/tmp/` matched `/home/user/tmp/tax-return.pdf`, `/var/log/`
matched `~/var/log`, and `.dmg` matched `holiday.dmgx`. All three come
back `review`, which is an actionable verdict — the app offered to move
user data on the strength of a substring.

Patterns are globs now, compiled with globset and `literal_separator` set
so `*` stops at a separator. That makes anchoring expressible: `/tmp/**`
is the filesystem root's tmp and nothing else, while `**/node_modules/**`
still reaches any depth. The `patterns` TODO named globset for this.

Compiled matchers are cached in a `OnceLock` on the db rather than built
per call: `report::build` classifies every entry, and a home directory is
millions of them. The cache is `#[serde(skip)]` with a hand-written
`Clone`, so a db that arrives deserialized behaves like one built here.

A pattern that fails to compile is dropped with a warning instead of
panicking mid-scan, and a test asserts every shipped pattern compiles —
a protected rule that silently matches nothing is the worst thing this
module could do quietly.

Closes #41
`firefox-cache` listed `/mozilla/firefox/profiles` with verdict `safe`.
On Windows the profile itself lives at
`%APPDATA%\Mozilla\Firefox\Profiles\<id>\`, which normalizes straight
onto that pattern — so the rule covered `places.sqlite` (bookmarks and
history), `logins.json` and `cookies.sqlite`. They were listed under
"Safe to remove" in the app, and `quarantine_finding` accepted them.

The cache is `cache2` inside the profile, under LocalAppData on Windows
and `~/Library/Caches` on macOS. Naming those directly keeps the rule to
what it claims to cover; the Linux pattern was always cache-only and is
unchanged.

Two tests pin it from both sides: profile data must not come back
`safe`, and cache2 on all three platforms still must.

Closes #40
The matching-semantics section still told rule authors that patterns are
substrings matched anywhere in the path, and pointed at #41 as the fix.
The fix landed, so the guidance has to move with it: how `*` and `**`
differ, why a directory rule needs a trailing `/**`, and why a pattern
that doesn't compile makes a `protected` rule fail open.
The example block above the field table still carried the pre-glob
patterns, which now read as a rule that matches nothing.
`is_excluded` compared raw strings with `starts_with`, which is a
character test, not a path test. Two consequences:

- `/run` also excluded `/runtime-data`, and anything else whose name
  merely begins with those four characters
- a root typed `c:\windows\winsxs` never matched the
  `C:\Windows\WinSxS` exclude, so the component store was walked anyway —
  on the one platform where walking it is slowest

Excludes now go through the same normalization `rules::classify` uses
(lowercased, `\` to `/`) and compare component-wise: a path matches when
it *is* the excluded directory, or when the character after the prefix is
a separator. Normalization happens once per root rather than inside
`process_read_dir`, which runs for every directory the walk opens.

The `excludes` doc called them glob-style; they never were, and now that
`rules` really does take globs the distinction matters.

Closes #46
`find_duplicates_filtered` takes a predicate and only stages, hashes and
groups the entries it accepts. The existing two entry points delegate to
it with "everything", so nothing changes for current callers.

This is the seam `report` needs: a duplicate set is an offer to keep one
copy and drop the rest, and entries the user is not allowed to act on
have no business in that offer. Filtering before stage 1 also means they
are never hashed — hashing is where a real scan spends its time.
`ScanOptions::min_file_size` was documented as "skip tiny files for dedup
purposes" and then applied inside the walk, before anything else ran. A
file under it never reached the rules engine, the risk model, the report
or `files_scanned` — so the default of 1 quietly dropped every zero-byte
file, and anyone raising it to 1 MB to speed dedup up would also have
hidden every small cache file from classification.

The knob moves to where the doc comment always said it lived: dedup. It
is now `ReportOptions::dedup_min_size`, passed to
`find_duplicates_filtered`, and `ScanOptions` no longer has a size filter
at all. `build` and `build_cancellable` keep their signatures and use the
default; `build_with` is the version that takes the options.

Closes #47
Callers that decide eligibility from something they already computed —
`report` classifies every entry before dedup runs — can look the answer
up by position instead of re-deriving it or carrying a set of paths as
large as the disk. No behaviour change: both existing entry points still
accept everything.
Two ways the headline number overstated what a user can actually free.

The halves overlap. `total_reclaimable` was findings + duplicate waste,
and a file is often in both: two identical 1 GB installers under `/tmp`
are two findings at 1 GB each *and* a duplicate set with 1 GB wasted, so
the headline said 3 GB where 2 GB is the whole disk's worth. Findings are
now counted in full, and a duplicate set adds only the redundant copies
nothing has counted yet — `size * (copies - 1 - already counted)`.

Protected files were duplicate candidates. `find_duplicates` ran over
every entry, so two copies of a driver-store file contributed `wasted`
bytes to a total the app will never offer to act on. Classification now
happens before dedup rather than after, which lets protected entries sit
the stage out — they are also the entries most expensive to hash and
least useful to.

Four tests pin the arithmetic from each side: both copies already
findings, neither copy a finding, one of each, and protected.

Closes #44
`quarantine` handled `rename` failing with EXDEV and fell back to copy
then remove. `restore` was a bare `fs::rename`, so the exact case the
move survived was the case the undo failed on.

That case is the normal one, not an edge one: quarantine lives in the
app's local data directory and the scanned files come from wherever the
user pointed the scan — a second drive, or a `/home` on its own
partition. Quarantining worked; restoring returned EXDEV and the file
stayed where the app had put it.

Both directions now go through one `move_file`. The copy-then-remove half
is its own function so it can be tested without two mounts to hand, and
the remove stays last: a failure there leaves the file in both places,
which is recoverable, where the other order would not be.

Closes #42
The module doc promised files are moved "with a manifest recording
original locations, so every action is reversible until the user
explicitly purges quarantine". There was no manifest. `quarantine`
returned a `QuarantineRecord` and the caller dropped it — `App.jsx`
ignores the resolved value of `invoke("quarantine_finding", ...)` — so
reversibility lasted exactly as long as the value nobody kept.

The quarantine filenames could not stand in for it either: flattening
maps `/` and `_` onto the same character, so `1717000000_home_user_.cache_x`
has no single original it can be read back to.

Now `quarantine` appends the record to `manifest.jsonl` in the quarantine
directory before it returns, and this module gains the three operations
that record makes possible:

- `list` — everything still quarantined, read off disk
- `restore_from_manifest` — restore one file and stop listing it
- `purge` — the "purge quarantine" step the README already described

Details that matter more than they look:

- the same flattening collision that made filenames useless could also
  make two originals land on one quarantine path within a second, where
  the second rename silently overwrote the first. Destinations are now
  unique, and names are capped so a deep path can't exceed the 255-byte
  filename limit.
- `purge` removes only what the manifest lists. Anything else in the
  directory is somebody else's file.
- rewrites go through a temp file and a rename, so a crash mid-write
  leaves the previous manifest rather than a truncated one.
- one torn line is skipped with a warning instead of hiding every other
  record behind it.
`start_scan` spawns a thread that emits `scan-progress` every 150ms and
stopped it with two statements after the `.await`. Statements after an
await only run if control reaches them.

It did not, twice over. There used to be a `?` between the await and the
`stop.store`, so a panicking blocking task — a rayon or jwalk worker
dying on an odd filesystem — returned the join error and left the thread
emitting progress for the rest of the process, behind whatever the error
state the UI then showed. That `?` has since moved below the cleanup, but
the shape is still fragile: dropping the command's future, which is how
Tauri cancels a command, skips the cleanup entirely.

Both cleanups now live in a `ScanRun` guard and run from `Drop`. The
slot-clearing goes with the ticker for the same reason and keeps the same
identity check: clear only if the slot still holds *this* scan, so a
short scan that started second can't erase a longer one's handle.

Closes #45
Three commands over the manifest the engine now keeps:

- `list_quarantine` — read-only, everything still quarantined
- `restore_quarantined` — put one file back, addressed by its path *in*
  quarantine, which is the unique one; the same original can be
  quarantined, restored and quarantined again
- `purge_quarantine` — the one command in Diskern that deletes, and it
  deletes only what the manifest lists

All three take `quarantine_dir` from the frontend the way
`quarantine_finding` already does, and all three go through the manifest
rather than the directory listing, so a path the webview invents reaches
no file.
The last missing half of reversibility: somewhere to call restore from.

The panel reads the manifest off disk rather than anything this session
remembers, so it renders before any scan has been run — files quarantined
in an earlier session are restorable without scanning again, which is the
whole point of the manifest existing.

Restoring puts a row back in the report it came from and takes its bytes
back out of the reclaimed running total; a record with no matching
finding (a different scan, or a restart) just refreshes the list.

Purge asks first and says what it did — files removed, bytes freed, and
how many it couldn't remove. It's the only irreversible thing in the app,
so it's a two-step confirm behind a collapsed section rather than a
button sitting next to the scan results.

Closes #43
Quiet by default, matching the cancel button: quarantine is a safety net,
not the thing anyone opened the app to look at. The purge confirmation is
the exception and reuses the red the destructive confirmations already
use.
`graph.rs` compiled and had no callers. This is the half it was missing:
a constructor that turns a scan into project roots, dependency stores,
and the `References` edges between them.

One pass over the entries answers both questions. A directory holding a
marker file (`Cargo.toml`, `package.json`, `pyproject.toml`) is a project
root; a directory named `target`, `node_modules` or a virtualenv that an
entry sits under is a store. A marker *inside* a store marks nothing —
every npm package ships a `package.json`, and treating those as roots
would make one `node_modules` look like ten thousand projects.

A project whose own store wasn't scanned links to the nearest enclosing
project's store of the same kind. That is where "referenced by 3
projects" comes from rather than always being 1: npm workspaces hoist
dependencies to the repository root and Cargo workspaces share one
`target/`, so the members really do reference it.

`referencing_projects` now answers for enclosing directories too. Findings
are files, and nothing references `proj/node_modules/react/index.js`
directly — what projects reference is the store it sits in.
The pipeline in the core README and in `lib.rs` reads

    scanner ──► index ──► dedup ──► graph ──► rules + risk ──► report

and the graph stage wasn't in it. `report::build` called `rules.classify`
and `risk::assess` and stopped there; `risk::downgrade` — the function
that takes `referenced_by` and makes a verdict more cautious — had no
callers anywhere in the workspace.

The visible consequence was that a `node_modules` three live projects
depend on got the same verdict, and the same reasons, as an abandoned
one. "Referenced by 3 projects" is the evidence that makes Diskern
different from every other disk cleaner, and it never reached the report.

`build_with` now builds an `ImpactGraph` from the entries, asks it how
many projects reference each one, and passes that through
`risk::downgrade` before assessing risk. A referenced store picks up a
`referenced by N projects` reason and drops from Review to Risky.

Which makes Risky reachable for the first time, so its bytes stop
counting as reclaimable: `actions::quarantine` refuses Risky and the UI
renders no action for it, so counting those bytes would promise space the
app will not free — the same overstatement #44 fixed for protected files.

Closes #48
`cargo audit` reports seventeen advisories against this lockfile and zero
of them are vulnerabilities: sixteen unmaintained crates and one unsound
one, all reaching the tree through `tauri`.

They are one dependency wearing seventeen hats. Tauri v2 renders on Linux
through webkit2gtk-4.1, which links the gtk-rs GTK3 bindings — archived
upstream in 2024. That is ten of them, plus `glib`'s unsound
`VariantStrIter` (fixed in the 0.19 line the GTK3 bindings never moved
to), plus `proc-macro-error` via `glib-macros`. The five `unic-*` crates
arrive the same way, through `urlpattern` in `tauri-utils`. Verified with
`cargo tree -i`; none has a version that clears it, so neither the audit
job nor the auto-fix PR has anywhere to go.

Which is the actual risk. The audit job fails while advisories stand, so
it has been red every Monday since it was written and will stay red until
Tauri moves to GTK4 — and a check that is always red stops being read.
The eighteenth advisory, in a crate we chose, would land in a run nobody
looks at.

So they are listed in `.cargo/audit.toml` with the reason for each and a
review date. `ignore` suppresses exactly those ids: a new advisory
against these same crates still fails, as does anything anywhere else.
The ids go into the job summary on every run so a suppression cannot
outlive its reason quietly, and editing the file re-runs the audit the
same way moving the lockfile does.

`rustsec/audit-check`, which runs on pull requests, does not read
`.cargo/audit.toml` — it only takes an `ignore` input — so the workflow
greps the ids out of the file rather than keeping a second list.

Closes #23
Closes #24
Closes #25
Closes #26
Closes #27
Closes #28
Closes #29
Closes #30
Closes #31
Closes #32
Closes #33
Closes #34
Closes #35
Closes #36
Closes #37
Closes #38
Closes #39
Closes #55
Grouped the way the changes actually land: new capability (quarantine
manifest, graph evidence), changed behaviour that a user or a caller
would notice (glob rules, exclude matching, the moved dedup knob, the
honest reclaimable total, the audit ignore list), and plain fixes.
The rule reason says what a file is. The reasons after it say why this
copy of it got the verdict it did — and "referenced by 3 projects" is
exactly the line that explains why one `node_modules` is risky and the
one next to it is not.

Printing only the first reason meant the evidence the graph stage now
produces reached the report and stopped there, which is most of the way
to not having produced it.
Same gap as the CLI: the row rendered `reasons[0]`, which is always the
matched rule, so the reference evidence behind a risky verdict never
appeared next to the row it explains.
…llows

The torn line cut off at `tru`, which `typos` reads as a misspelt `true`
rather than as the half-written JSON it is. Cutting the line mid-path
instead keeps it just as invalid and just as realistic.
`quarantine` moved the file and then wrote the manifest line. Serde
refuses a `PathBuf` that isn't valid UTF-8, and Linux and macOS both
allow filenames that aren't — a Latin-1 name out of an old archive is
enough, and `rules::normalize` uses `to_string_lossy`, so such a file
classifies normally and reaches the user as an actionable row.

Quarantining one moved it, failed to encode it, and returned an error.
The file was then gone from its original location, absent from the
manifest, and sitting in quarantine under a flattened name that cannot
be read back to an original — while the UI, seeing the error, told the
user nothing had happened. Unrecoverable except by hand, and caused by
the exact operation whose promise is that it is reversible.

The line is encoded first now, so a record that cannot be written stops
the move instead of following it. If the append itself fails after a
successful move, the move is rolled back rather than left standing.
Both halves of `move_file` replace an existing destination without
asking, so restoring put the quarantined copy over whatever now sat at
the original path — silently, and with no way back.

That is not a corner case. The files Diskern marks `safe` are the ones
applications rebuild: quarantine a browser cache, keep browsing, then
change your mind, and the undo destroys the cache the browser has since
written. `restore` had no caller outside its own tests before this
branch, so the Quarantine panel is what makes it reachable — in the one
module whose stated rule is that nothing is ever destroyed.

An occupied destination is now an error the caller shows, naming the
file and what to do about it, rather than a decision this function makes
on the user's behalf. Checked with `symlink_metadata`, so a broken
symlink in the way counts as in the way.
`restore_from_manifest` and `purge` are read-modify-write: they read the
whole manifest, act, then rewrite it from what they read. A record
appended in between was erased by that rewrite, and the file it named
stayed in quarantine with nothing recording where it belonged —
invisible to `list`, unreachable by `restore`, and not even removed by a
later `purge`.

The app makes that reachable. `quarantine_finding`, `restore_quarantined`
and `purge_quarantine` each run on their own blocking task, and nothing
in the UI stops a user quarantining a finding row while another row's
restore is still in flight.

Every manifest access now goes through one process-wide lock, held
across the read, the action and the rewrite. `list` takes it; the
internal `read_manifest` doesn't, because `Mutex` isn't reentrant and
the operations that rewrite already hold it.

The test drives two threads quarantining against a third restoring and
asserts the invariant that matters: nothing sits in the quarantine
directory without a manifest line naming it. It fails on every run with
the lock neutered.
`purge` on a directory nothing has been moved into read an empty list
and then asked `write_manifest` to rewrite it, which failed because the
directory did not exist. The caller got "io error at
.../manifest.jsonl.tmp: No such file or directory" — which reads like a
disk fault rather than the nothing-to-do it actually is.

Not reachable from the app today, which hides the button until something
is listed, but `purge` is a public entry point and the error was a lie.
`roots` mapped each directory to a single `ProjectKind`, so a root
holding two marker files kept whichever the walk yielded last. A
directory with both `Cargo.toml` and `package.json` — any Rust binary
with a web front end, this repository's own `app/` among them — got one
`References` edge instead of two, and the store that lost stayed
`Review` instead of dropping to `Risky`. The app then offered to
quarantine the build output of a live project.

Worse, *which* store lost depended on the order the walk returned the
two markers in, so the same tree could give different verdicts on
different machines. Deterministic verdicts are the guarantee the README
opens with.

A root now carries a set of kinds, and the maps are ordered, so a given
scan produces the same graph every time. The node stays keyed by path,
so a root that is two projects is still one project to anything counting
references — asserted, along with both marker orders.
`ImpactGraph::from_entries` walks every entry's ancestors — measured at
about 1.3s per million entries — and `build_with` ran it before the
first `cancelled` check, with nothing checking the flag inside it. On a
multi-million-file scan that is several seconds in which the Cancel
button does nothing.

Which undoes part of what the previous round bought: the walk and the
hashing pass were both made to stop promptly, and this stage was
inserted in front of them. Moving the dead spot is not removing it.

`from_entries_cancellable` checks the flag per entry and again per
project root; `build_with` propagates the `None`. `from_entries` keeps
its signature for callers that have nothing to cancel with.
Risky findings were given `reclaimable = 0`, but only Protected entries
were held out of dedup — and `total_reclaimable` treats a
zero-reclaimable finding as "nothing has counted this yet". So a
duplicate set of risky copies added its full `wasted`, and the bytes
walked back into the headline through the half they had just been taken
out of.

Two live npm projects with an identical file in each `node_modules`:
both copies risky, both `reclaimable = 0`, and the total still counted
one copy's worth of them. Which is the overstatement #44 set out to
remove, reintroduced by the risky verdicts #48 made reachable.

There is now one `is_actionable`, used for both what counts towards the
headline and what takes part in dedup. Splitting that definition across
two expressions is what let them disagree.
`is_excluded` built a normalized copy of every path — one allocation for
the separator rewrite, another for a Unicode `to_lowercase` — for every
child of every directory the walk opens, and did it before looking at
the exclude list at all.

It now folds character by character as it compares and stops at the
first one that differs, which for a path not under an exclude is almost
always the first or second. Empty exclude lists return immediately.

Measured over 200k paths with the four default Linux excludes:
38.8ms for the original `starts_with`, 97.2ms after the component-boundary
fix, 36.9ms now — so the correctness fix no longer costs anything. Folding
is ASCII, matching `normalize_exclude`; these are directory names like
`Windows` and `System`, and both sides were cross-checked against the
previous implementation before the swap.
Restore refusing an occupied destination is something a user will meet,
and so is a quarantine being declined outright for a path that cannot be
recorded. Both are new answers to situations that previously had a
silent, worse one.
`b"caf\xe9.dmg"` made `typos` see a misspelt `calf` in the byte literal.
Raw invalid bytes inside an ordinary name test the same thing without
spelling anything.

@Muawiya-contact Muawiya-contact left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@Muawiya-contact
Muawiya-contact merged commit cfcee05 into main Sep 5, 2026
10 checks passed
@Muawiya-contact
Muawiya-contact deleted the fix/open-issue-sweep branch September 5, 2026 09:44
Muawiya-contact added a commit that referenced this pull request Sep 5, 2026
The matrix found both on its first run, and both were in the tests
rather than the engine.

Windows: `a_referenced_store_is_more_cautious_than_an_abandoned_one`
looked for a finding whose path contained `live/node_modules`. Windows
separates with `\`, so it matched nothing and the `unwrap` panicked. It
compares whole paths now, built with `join`, which is separator-correct
everywhere.

macOS: `a_path_that_cannot_be_recorded_is_not_moved` could not create
its fixture — APFS validates filenames as UTF-8 and answers EILSEQ, so
a non-UTF-8 name cannot exist there and the data loss it caused is not
reachable on macOS at all. Linux imposes no such rule, so the test
returns early only there, and a creation failure anywhere else is still
a hard failure rather than a quiet pass.

Worth recording, since it narrows the bug fixed in #63: that loss was
Linux-only in practice on Unix, and reachable on Windows through
unpaired surrogates instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment