You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Found by a cargo-mutants audit of PR #196. The PR should not merge until this is addressed — it ships a one-click destructive control whose documented safety gate is dead code.
The defect
VerifyReport::is_complete() (crates/app/src/verify.rs) is documented in three places as the check a destructive caller must perform:
WHY callers should check this before acting destructively: a prune driven by an incomplete sweep deletes rows for files that are perfectly intact on a drive that simply was not plugged in.
It is called by nothing outside its own unit tests. Verified — six references total: two doc mentions, the definition, three of its own tests.
What actually gates the prune:
surface
gate
perima prune
--yes and missing > 0
prune_missing_locations (Tauri)
nothing
LibraryHealthPill Remove button
missing > 0 from useMissingCount() — a raw DB count with no relationship to any sweep
A second, related defect
The WHY block in verify.rs justifying its hand-written test stub asserts:
The adapter's SQL is exercised separately
That was false when written. list_locations_for_verify, update_location_statuses, count_missing_locations and soft_delete_missing_locations had no tests at any layer. The comment justified skipping coverage by claiming coverage that did not exist. The audit added 10 adapter tests, each proven to discriminate by mutating the implementation — including dropping vm.machine_id = ?1, JOIN→LEFT JOIN, and dropping status = 'missing' from the prune (which deletes the entire catalogue).
Threat model — one scenario ruled out, one confirmed
Ruled out (do not cite this): "unmounting a watched volume marks the library Missing via FileEvent::Deleted". Disproven at source level. notify 8.2.0's inotify backend never references UNMOUNT or IGNORED, so IN_UNMOUNT falls through and produces no EventKind::Remove. On macOS the fsevent backend does map StreamFlags::UNMOUNT to Remove(RemoveKind::Other), but it emits one event carrying the mount point, which post-#191 relativizes to an empty path and matches zero rows.
Confirmed, and it needs no unmount: a pre-#191 catalogue. #191 recorded relative_path against the scan root while mount_path held the volume root, so reconstruction failed for every row — observed on a real library as 417 of 419 rows pointing at files that were on disk the whole time. A sweep over such a catalogue is complete, fully mounted, and marks essentially every row Missing. is_complete() returns true. The pill reads "417 missing". Remove retires the catalogue.
This matters beyond #191: any future path-reconstruction defect has exactly this shape.
Proposed fix — three layers
A work-in-progress diff exists locally (uncommitted, not yet compile-verified):
Mounted-volume predicate on the prune (structural). soft_delete_missing_locations and count_missing_locations retire/count only rows whose volume is mounted on this device. A Missing row on an unmounted volume cannot be confirmed missing now, so the query refuses to touch it. No caller can opt out. The two predicates must stay byte-identical — the count is what the user authorises, and authorising one number while deleting a different set is worse than not asking.
VerifyReport::looks_implausible() — a sweep marking ≥50% of a ≥20-location sample as newly missing is more likely a broken sweep than a broken disk. Catches the scan records relative_path against the scan root, not the volume root #191 class, which layers 1 and 3 both pass. Warns and requires extra confirmation; never blocks a genuine cleanup.
Layer 1 alone does not cover the #191 scenario (volume mounted, paths wrong). Layer 2 alone does not cover an unplugged drive. All three are needed.
Process note
This is worth recording as a class, not just an instance: a safety property was designed, documented three times, given its own unit tests, and never connected to the thing it protects. Everything looked right in review — the function existed, was tested, and was referenced in prose. Only asking "who calls this?" surfaced it. Relevant to #123 (process gap: intermittent bugs slip past implementation and review) and #188 (no test boots the real app).
Found by a cargo-mutants audit of PR #196. The PR should not merge until this is addressed — it ships a one-click destructive control whose documented safety gate is dead code.
The defect
VerifyReport::is_complete()(crates/app/src/verify.rs) is documented in three places as the check a destructive caller must perform:It is called by nothing outside its own unit tests. Verified — six references total: two doc mentions, the definition, three of its own tests.
What actually gates the prune:
perima prune--yesandmissing > 0prune_missing_locations(Tauri)LibraryHealthPillRemove buttonmissing > 0fromuseMissingCount()— a raw DB count with no relationship to any sweepA second, related defect
The WHY block in
verify.rsjustifying its hand-written test stub asserts:That was false when written.
list_locations_for_verify,update_location_statuses,count_missing_locationsandsoft_delete_missing_locationshad no tests at any layer. The comment justified skipping coverage by claiming coverage that did not exist. The audit added 10 adapter tests, each proven to discriminate by mutating the implementation — including droppingvm.machine_id = ?1,JOIN→LEFT JOIN, and droppingstatus = 'missing'from the prune (which deletes the entire catalogue).Threat model — one scenario ruled out, one confirmed
Ruled out (do not cite this): "unmounting a watched volume marks the library
MissingviaFileEvent::Deleted". Disproven at source level.notify8.2.0's inotify backend never referencesUNMOUNTorIGNORED, soIN_UNMOUNTfalls through and produces noEventKind::Remove. On macOS the fsevent backend does mapStreamFlags::UNMOUNTtoRemove(RemoveKind::Other), but it emits one event carrying the mount point, which post-#191 relativizes to an empty path and matches zero rows.Confirmed, and it needs no unmount: a pre-#191 catalogue. #191 recorded
relative_pathagainst the scan root whilemount_pathheld the volume root, so reconstruction failed for every row — observed on a real library as 417 of 419 rows pointing at files that were on disk the whole time. A sweep over such a catalogue is complete, fully mounted, and marks essentially every rowMissing.is_complete()returnstrue. The pill reads "417 missing". Remove retires the catalogue.This matters beyond #191: any future path-reconstruction defect has exactly this shape.
Proposed fix — three layers
A work-in-progress diff exists locally (uncommitted, not yet compile-verified):
Mounted-volume predicate on the prune (structural).
soft_delete_missing_locationsandcount_missing_locationsretire/count only rows whose volume is mounted on this device. AMissingrow on an unmounted volume cannot be confirmed missing now, so the query refuses to touch it. No caller can opt out. The two predicates must stay byte-identical — the count is what the user authorises, and authorising one number while deleting a different set is worse than not asking.VerifyReport::looks_implausible()— a sweep marking ≥50% of a ≥20-location sample as newly missing is more likely a broken sweep than a broken disk. Catches the scan records relative_path against the scan root, not the volume root #191 class, which layers 1 and 3 both pass. Warns and requires extra confirmation; never blocks a genuine cleanup.is_safe_to_prune()=is_complete() && !looks_implausible(), serialized ontoVerifyReportas a wire field so the frontend gates on the Rust verdict rather than recomputing it in TypeScript — the drift class already filed as Settings modal hardcodes a stale copy of the Rust provider-preset table #193.Layer 1 alone does not cover the #191 scenario (volume mounted, paths wrong). Layer 2 alone does not cover an unplugged drive. All three are needed.
Process note
This is worth recording as a class, not just an instance: a safety property was designed, documented three times, given its own unit tests, and never connected to the thing it protects. Everything looked right in review — the function existed, was tested, and was referenced in prose. Only asking "who calls this?" surfaced it. Relevant to #123 (process gap: intermittent bugs slip past implementation and review) and #188 (no test boots the real app).