Skip to content

fix(app): enforce report-backed quarantine safety - #94

Merged
Muawiya-contact merged 3 commits into
Coding-Moves:mainfrom
alexsmolya:codex/issue-84-hybrid-synthesis
Sep 8, 2026
Merged

fix(app): enforce report-backed quarantine safety#94
Muawiya-contact merged 3 commits into
Coding-Moves:mainfrom
alexsmolya:codex/issue-84-hybrid-synthesis

Conversation

@alexsmolya

Copy link
Copy Markdown
Contributor

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:

  • frontend verdict/category/score data is not trusted or accepted as authority;
  • fresh static rules can tighten the report verdict, but never relax it;
  • generation-scoped report authority prevents superseded scans from retaining quarantine authorization;
  • quarantine requires the exact finding path and checks target metadata without using fragile access-time evidence;
  • missing, stale, or unauthorized findings fail closed, while legitimate Safe/Review findings remain quarantinable.

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.

@Muawiya-contact Muawiya-contact left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@alexsmolya

Copy link
Copy Markdown
Contributor Author

Thanks for catching both points. The cancellation path now records the
cancelled generation and distinguishes a late completion of that generation
from a completion superseded by a newer scan. A late report after Cancel maps
to the existing Ok(None) cancellation result, while a genuinely superseded
generation still returns the superseded error; the report and action lease
remain invalidated in both cases. I also moved the #84 changelog entry into a
new [Unreleased] section above the existing v0.2.0 history.

Clearing the reclaimed tally was deliberate. It is derived from the previous
report, whose actionable rows lose backend authority when a new scan starts or
is cancelled. Keeping the tally would preserve non-actionable historical
display data while making it look current, so this corrective resets it along
with the stale report. I left a separate historical tally/UI treatment out of
scope.

@Muawiya-contact Muawiya-contact left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@Muawiya-contact

Copy link
Copy Markdown
Member

Filed as #109.

@Muawiya-contact
Muawiya-contact merged commit 969ea73 into Coding-Moves:main Sep 8, 2026
15 checks passed
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.

quarantine_finding re-classifies without the graph, so it accepts a file the report calls risky

2 participants