fix(app): enforce report-backed quarantine safety - #94
Conversation
Muawiya-contact
left a comment
There was a problem hiding this comment.
This is a serious piece of work, thank you. Taking the verdict from the report the backend produced rather than re-deriving a weaker one is the right answer to #84, and Verdict::strictest being explicit instead of leaning on the enum ordering is a nice touch. The generation lease holds up as far as I can follow it: an old lease can't act after a new scan begins, and cancelling clears the report. I approved CI, everything passes, and the core suite is green here too (65 tests).
Two things.
The cancel path can show an error instead of a cancellation. cancel() takes active straight away, so if the scan finishes in the gap before the walk sees the flag, the join comes back Ok(Some(report)), publish finds no active and returns false, and the user gets a red box saying "scan was superseded by a newer scan" when they just pressed Cancel and started nothing. That used to show the finished report. Ok(None) is the outcome the UI already renders properly, so a publish that fails due to cancellation wants to land there rather than in the error arm. Telling it apart from a real supersede probably means cancel() recording which generation it cancelled.
Changelog line needs to move to [Unreleased]. v0.2.0 went out today, so ### Changed is now inside the released section.
One question, not a change request: clearing the report at scan start means cancelling now wipes what's on screen, including the reclaimed tally. I follow the reasoning, and a stale actionable row is worse. Was dropping the tally deliberate too?
|
Thanks for catching both points. The cancellation path now records the Clearing the reclaimed tally was deliberate. It is derived from the previous |
Muawiya-contact
left a comment
There was a problem hiding this comment.
The PublishOutcome split is exactly right, and scoping the cancellation marker to a generation means a late completion after a new scan still reads as superseded, which it is. Both tests pin the distinction. Changelog's moved too.
CI is green, all 13 checks, app-tauri on all three platforms.
One thing you should know, and it's ours not yours: the tests you added to commands.rs never actually run. The test job is scoped to -p diskern-core -p diskern-cli, and the app-tauri job only runs clippy, so diskern-app tests get compiled and never executed. Your safety tests are real, CI just isn't looking at them yet. I'll open an issue for it.
Approved. Nice work on this one.
|
Filed as #109. |
Fixes #84
Quarantine previously re-classified a finding with rules-only context, which could weaken the graph-aware verdict shown in the completed report and allow a risky file through the backend command.
This fix makes the completed backend report authoritative for the exact finding under review:
The bounded contract is intentional: a completed scan report is the reviewed snapshot, and starting a new scan invalidates the old authority. This change does not claim platform-level filesystem object identity or eliminate every OS-level path TOCTOU window.