Skip to content

Feature/quarantine manager#229

Merged
Jordan231111 merged 17 commits intomainfrom
feature/QuarantineManager-clean
Mar 30, 2026
Merged

Feature/quarantine manager#229
Jordan231111 merged 17 commits intomainfrom
feature/QuarantineManager-clean

Conversation

@Jordan231111
Copy link
Copy Markdown
Owner

Description

Implemented the changes proposed in #151, mostly following the outlined function prototypes and requirements. For all the edges cases I caught them and propagated an error up to the client. I leave implementation of what to do to the CLI. A lot of the code involving permissions is very messy due to the differences between unix and windows. It might be worth looking into a more OS agnostics method to achieve what we want. In implementing this I noticed a problem with how we handle original file permissions as there was no system suggested for restoring windows file permissions which we do edit as part of making a file read only. This required me to implement a very janky system to deal with how we store permissions and should be fixed/changed in a later issue.

Related Issue

Fixes #151

Type of Change

  • Bug fix
  • New feature
  • Refactoring / CI / Docs
  • Other

How to Test

cargo test quarantine_tests

Checklist

  • Code follows project style (cargo fmt + cargo clippy)
  • Tests added/updated and passing (cargo test)
  • Documentation updated (if applicable)

@Jordan231111 Jordan231111 mentioned this pull request Mar 28, 2026
7 tasks
@GoatHero GoatHero self-assigned this Mar 28, 2026
@GoatHero GoatHero self-requested a review March 28, 2026 19:58
Copy link
Copy Markdown
Owner Author

Review verdict: Request changes.

This should not merge yet.

Blocking issues:

  • The permission hardening code mutates temporary Permissions values but never applies them back to the filesystem, so the quarantine directory/file modes are not actually being tightened.
  • Quarantine storage is derived only from file_name(), so two different source files with the same basename can collide in quarantine and leave multiple manifest entries pointing at the same stored path.
  • The current tests stay on happy paths and do not catch either case, so the green test run is not enough evidence for this subsystem.

Please fix the storage/permission logic and add targeted regression coverage for both behaviors before merging.

Copy link
Copy Markdown
Owner Author

Follow-up hardening changes are now pushed to this PR.

Summary of changes:

  • hardened QuarantineManager so file moves, restore/delete flows, and manifest updates stay consistent under failure paths
  • switched quarantine manifest persistence to atomic temp-file replacement instead of in-place writes
  • fixed non-Unix permission handling and added safer restore guards for missing parents, unknown IDs, and existing destination files
  • aligned the quarantine module surface with the issue contract by exporting QuarantineManager from the module root and using delete_entry
  • replaced the quarantine tests with acceptance-driven coverage for quarantine, restore, delete, persisted reloads, permission-denied behavior, duplicate quarantine rejection, and cross-device move fallback

@GoatHero GoatHero removed their request for review March 30, 2026 15:54
@Jordan231111 Jordan231111 merged commit e1bce15 into main Mar 30, 2026
16 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.

Step 11b/23 (Phase 2): Quarantine — Implement QuarantineManager core operations

2 participants