feat(library): managed library integrity check (#92)#110
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/artwork-derivatives-91 #110 +/- ##
=============================================================
+ Coverage 79.1% 79.6% +0.4%
=============================================================
Files 129 131 +2
Lines 6772 6899 +127
Branches 2109 2107 -2
=============================================================
+ Hits 5360 5493 +133
+ Misses 1369 1363 -6
Partials 43 43
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Maintainer acceptance — merge slot 12, strictly after #107BLOCKED — changes required. Blocking backend safety/state issues
Rebase after #107 so artwork derivatives and waveform cascade behavior are present, then rerun CI and destructive-flow manual acceptance. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Re-review: blocking filesystem-boundary and playback-authority fixes requiredThis PR is stacked after #107. Its frontend reconciliation is not sufficient to make destructive cleanup safe. Two backend issues remain. 1. Never traverse a managed-root symlink
Implement this boundary policy:
2. Reconcile playback from backend authority after deletion
After #107 merges, rebase onto the resulting |
7d52bbb to
b7d29f5
Compare
Re-review — still blocked: cleanup does not invalidate late playback work or AirPlay authorityReviewed at
Make the post-commit coordinator path authoritative:
Frontend reload is not a substitute for invalidating the backend work that can re-install or continue rendering deleted media. |
b70ee60 to
8fe18db
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
7c2c708 to
b6a7e80
Compare
Re-review — blocked: current Rust CI is red, and the temp-artwork filter does not match the writerReviewed at
Make the audit recognise the exact temp-file convention used by Please re-run Rust fmt, clippy, and the full Rust test suite. This PR cannot be approved while its CI reports 596/599 tests passing. |
Re-review — still blocked: the integrity audit still fails to recognise the writer's real temporary artwork namesReviewed at
The audit predicate still accepts only Please share an exact predicate between writer and audit, or precisely recognise the generated convention under Add regression coverage using a real generated-style path for both sides of the 24-hour boundary: <24h is excluded; >=24h is reported. No LGTM yet. |
b77a1e2 to
d7240ae
Compare
…ion (#110) The orphan scanner is_temp_artwork_file checked for patterns like .tmp-* and *-tmp.*, but unique_temp_path generates temp files as .{name}.{pid}.{counter}.tmp. The filter now matches any hidden file (starts with .) ending in .tmp under artwork/. Added 24h boundary regression tests: a temp file at 23h59m is excluded, while one at 25h is reported as a stale orphan.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@greptile review |
|
Re-review of head Two blockers remain:
Required implementation:
Because this feature can delete library database entries, manual destructive-safety acceptance is still required after the code blockers are cleared. |
…ion (#110) The orphan scanner is_temp_artwork_file checked for patterns like .tmp-* and *-tmp.*, but unique_temp_path generates temp files as .{name}.{pid}.{counter}.tmp. The filter now matches any hidden file (starts with .) ending in .tmp under artwork/. Added 24h boundary regression tests: a temp file at 23h59m is excluded, while one at 25h is reported as a stale orphan.
The orphan scanner's is_temp_artwork_file used a broad pattern (any
hidden file ending in .tmp under artwork/), which could exclude
unrelated temp-named files that don't follow the unique_temp_path
convention. The filter now matches the exact .{name}.{pid}.{counter}.tmp
shape (numeric pid and counter) and is a shared pub fn so the writer
and scanner can never diverge. Added regression tests: broad patterns
like .foo.tmp are reported as orphans, while exact convention files
under 24h are excluded.
ce65e01 to
d91a156
Compare
…ion (#110) The orphan scanner is_temp_artwork_file checked for patterns like .tmp-* and *-tmp.*, but unique_temp_path generates temp files as .{name}.{pid}.{counter}.tmp. The filter now matches any hidden file (starts with .) ending in .tmp under artwork/. Added 24h boundary regression tests: a temp file at 23h59m is excluded, while one at 25h is reported as a stale orphan.
The orphan scanner's is_temp_artwork_file used a broad pattern (any
hidden file ending in .tmp under artwork/), which could exclude
unrelated temp-named files that don't follow the unique_temp_path
convention. The filter now matches the exact .{name}.{pid}.{counter}.tmp
shape (numeric pid and counter) and is a shared pub fn so the writer
and scanner can never diverge. Added regression tests: broad patterns
like .foo.tmp are reported as orphans, while exact convention files
under 24h are excluded.
5f1d52b to
a29c78a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ries (#110) remove_missing_library_entries previously only cleaned up the stem directory after the DB transaction committed, leaving the deleted song's artwork/thumb_* and artwork/preview_* derivative files behind on disk forever. Collect each deleted song's recorded artwork derivative paths before its DB rows are dropped (mirroring delete_song_from_library, which already does this), and best-effort delete them afterwards via delete_artwork_derivative_if_unreferenced. That helper skips any derivative still referenced by another song sharing the same cover-art digest, so shared artwork is preserved. Make delete::collect_artwork_derivative_paths pub so the integrity cleanup can reuse the same collection logic as the normal song delete. Add regression tests covering both the deletion and the shared- derivative preservation case.
Audit the active managed library for missing/empty referenced files and unreferenced managed files. Present a deterministic report in Settings. After explicit confirmation, remove selected database entries only when their primary media is still missing or empty at mutation time, then reconcile playback, queue and library state.
Add unit tests for integrity actions (check, toggle, confirm, close), IntegrityReportModal rendering paths, and the cleanup confirmation dialog in SettingsDialogHost. Patch coverage now at 95%.
Remove the unused canonical_root parameter from scan_directory (clippy::only_used_in_recursion under Rust 1.97). The parameter was only forwarded to the recursive call and never read in the function body; the caller canonicalize() was also dead work. Update createHarness in the integrity actions test to accept a partial state override (Partial<SettingsOverlayState>) instead of requiring a full SettingsOverlayState. The previous signature caused TS2740 in 14 test cases that only set integrity fields.
Call toggleIntegritySelection with a single hash argument, exercise cleanup-in-progress labels, and disable the integrity check while cleanup runs so patch coverage can hit every coverable line.
…idation Managed roots and nested paths use symlink_metadata before traversal so library audits cannot escape via symlink. Successful remove_missing commits invalidate matching current/loading tracks and clear CDG through the playback coordinator.
The add_artwork_columns test helper unconditionally ran ALTER TABLE songs ADD COLUMN artwork_thumb_path / artwork_preview_path, but cache::open_database already adds these columns via apply_migrations. The duplicate ALTER TABLE failed with 'duplicate column name: artwork_thumb_path' in 3 integrity tests. The helper now checks column_exists before adding, making it a no-op when migrations have already applied the columns.
…ion (#110) The orphan scanner is_temp_artwork_file checked for patterns like .tmp-* and *-tmp.*, but unique_temp_path generates temp files as .{name}.{pid}.{counter}.tmp. The filter now matches any hidden file (starts with .) ending in .tmp under artwork/. Added 24h boundary regression tests: a temp file at 23h59m is excluded, while one at 25h is reported as a stale orphan.
The orphan scanner's is_temp_artwork_file used a broad pattern (any
hidden file ending in .tmp under artwork/), which could exclude
unrelated temp-named files that don't follow the unique_temp_path
convention. The filter now matches the exact .{name}.{pid}.{counter}.tmp
shape (numeric pid and counter) and is a shared pub fn so the writer
and scanner can never diverge. Added regression tests: broad patterns
like .foo.tmp are reported as orphans, while exact convention files
under 24h are excluded.
The temp artwork predicate accepted any path starting with artwork/ and checked only the filename component, so a nested path like artwork/sub/.foo.123.0.tmp would match and receive the 24-hour grace period. The writer (unique_temp_path in artwork.rs) only places temp files as direct children of artwork/, never in subdirectories. Require the path to be exactly artwork/<filename> with no subdirectory components. Add regression tests for nested paths and arbitrary hidden .tmp files being reported as orphans.
Per maintainer review: the canonical recognition logic for temp artwork files now lives beside the writer (unique_temp_path) in artwork.rs as pub(crate) pure helpers, imported by integrity.rs. This ensures the writer and scanner can never diverge. Added table-driven predicate tests in artwork.rs and an integration test proving artwork/.notes.tmp is reported as an orphan.
…PRAGMA The table name was interpolated via format! without quoting or validation. While current callers only pass the hardcoded "songs" literal, add identifier validation and double-quote quoting as defense-in-depth against future untrusted input.
…ries (#110) remove_missing_library_entries previously only cleaned up the stem directory after the DB transaction committed, leaving the deleted song's artwork/thumb_* and artwork/preview_* derivative files behind on disk forever. Collect each deleted song's recorded artwork derivative paths before its DB rows are dropped (mirroring delete_song_from_library, which already does this), and best-effort delete them afterwards via delete_artwork_derivative_if_unreferenced. That helper skips any derivative still referenced by another song sharing the same cover-art digest, so shared artwork is preserved. Make delete::collect_artwork_derivative_paths pub so the integrity cleanup can reuse the same collection logic as the normal song delete. Add regression tests covering both the deletion and the shared- derivative preservation case.
0069894 to
22ac8fb
Compare
| - Library integrity audit never follows managed-root or nested symlinks outside the library ([#110](https://github.com/thedavidweng/OpenKara/issues/110)): top-level managed dirs (`media/`, `stems/`, …) are checked with `symlink_metadata` before `read_dir`; a root that is itself a symlink is reported as an orphan relative path and never opened. Nested symlinks are reported the same way. After a successful `remove_missing_library_entries` commit, the playback coordinator invalidates any current/loading track matching deleted song hashes and clears CDG so realtime playback cannot keep serving deleted media.- Enforce platform-scoped execution-provider choices ([#115](https://github.com/thedavidweng/OpenKara/issues/115)): the Hardware Acceleration settings section now exposes only providers valid for the running binary. macOS and Linux show CPU and XNNPACK; Windows shows CPU, XNNPACK, and DirectML. A single Rust policy table drives the available list, default, stale-config normalization, and write validation. `set_execution_provider` rejects known-but-unavailable providers (e.g. `directml` on macOS) before any config mutation, and `get_settings` normalizes a stale persisted cross-platform value to the platform default without writing to disk. The separation execution context uses the same normalized value. Windows CI now runs the filtered library tests (`cargo test --lib execution_provider`) — the test binary previously failed to load with `STATUS_ENTRYPOINT_NOT_FOUND` (0xC0000139) because `tauri-build` only links the Common Controls v6 manifest into `[[bin]]` targets, not test binaries; `build.rs` now embeds the manifest into all targets so `comctl32.dll` resolves correctly on headless Windows Server runners. | ||
| - Library integrity audit never follows managed-root or nested symlinks outside the library ([#110](https://github.com/thedavidweng/OpenKara/issues/110)): top-level managed dirs (`media/`, `stems/`, …) are checked with `symlink_metadata` before `read_dir`; a root that is itself a symlink is reported as an orphan relative path and never opened. Nested symlinks are reported the same way. After a successful `remove_missing_library_entries` commit, the playback coordinator invalidates any current/loading track matching deleted song hashes and clears CDG so realtime playback cannot keep serving deleted media. | ||
| - Deleting broken library entries now also removes their cover-art thumbnail and preview derivatives ([#110](https://github.com/thedavidweng/OpenKara/issues/110)): `remove_missing_library_entries` previously only cleaned up the stem directory after the DB transaction committed, leaving the song's `artwork/thumb_*` and `artwork/preview_*` files behind forever. It now collects each deleted song's recorded artwork derivative paths before the DB rows are dropped (mirroring `delete_song_from_library`) and best-effort deletes them afterwards via `delete_artwork_derivative_if_unreferenced`, which skips any derivative still referenced by another song sharing the same cover-art digest. | ||
| - Enforce platform-scoped execution-provider choices ([#115](https://github.com/thedavidweng/OpenKara/issues/115)): the Hardware Acceleration settings section now exposes only providers valid for the running binary. macOS and Linux show CPU and XNNPACK; Windows shows CPU, XNNPACK, and DirectML. A single Rust policy table drives the available list, default, stale-config normalization, and write validation. `set_execution_provider` rejects known-but-unavailable providers (e.g. `directml` on macOS) before any config mutation, and `get_settings` normalizes a stale persisted cross-platform value to the platform default without writing to disk. The separation execution context uses the same normalized value. Windows CI now runs the filtered library tests (`cargo test --lib execution_provider`) — the test binary previously failed to load with `STATUS_ENTRYPOINT_NOT_FOUND` (0xC0000139) because `tauri-build` only links the Common Controls v6 manifest into `[[bin]]` targets, not test binaries; `build.rs` now embeds the manifest into all targets so `comctl32.dll` resolves correctly on headless Windows Server runners. |
There was a problem hiding this comment.
🟡 CHANGELOG has duplicated and contradictory entries with missing line breaks
The changelog additions duplicate the same library-integrity note twice (CHANGELOG.md:40-41) and run two separate bullets together on one line without a newline, and elsewhere include two conflicting descriptions of the same temp-file fix, so the changelog no longer accurately reflects the completed change.
Impact: Readers of the changelog get duplicated, merged, and contradictory notes about this feature, undermining the changelog's role as the record of completed changes.
Prompt for agents
The CHANGELOG additions for this PR contain merge artifacts. Line 40 concatenates two distinct bullets (the 'Library integrity audit never follows managed-root or nested symlinks' entry and the 'Enforce platform-scoped execution-provider choices' entry) into a single line with no newline between them, and both of those bullets are then repeated as separate entries on lines 41 and 43, producing exact duplicates. Similarly, lines 29 and 31 contain two contradictory descriptions of the same #110 temp-file filter fix (one says the filter 'matches any hidden file ending in .tmp under artwork/', the other says it 'matches the exact .{name}.{pid}.{counter}.tmp shape') and line 29/32 also concatenate unrelated bullets without newlines. Deduplicate these entries, keep only the description that reflects the shipped behavior (exact .{name}.{pid}.{counter}.tmp match), and ensure each bullet is on its own line.
Was this helpful? React with 👍 or 👎 to provide feedback.
The settings-overlay.integrity-actions test harness predates the EQ and crossfade settings added by the audio-pipeline stack. The mock state and api dependencies were missing eqEnabled, eqGainsDb, crossfadeEnabled, crossfadeDurationMs and the corresponding setEq* / setCrossfade* api functions, causing TS2322 errors on the stacked #110 branch.
Fix: missing EQ/crossfade fields in integrity test mockThe
Commit
|
…o feat/library-integrity-92
1 participant
Summary
docs/references/contracts/library.md) and CHANGELOGCloses #92.
Test plan
cargo test)pnpm test) — 1336 testspnpm buildsucceeds