From f25433ea570be4ee74adb935c3588400df129721 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Mon, 18 May 2026 08:37:11 -0700 Subject: [PATCH] Harden quarantine approval flows --- core/downloads/status.py | 1 + core/imports/pipeline.py | 45 +++++++--- core/imports/quarantine.py | 5 ++ tests/downloads/test_downloads_status.py | 2 + tests/imports/test_quarantine_management.py | 14 +++ web_server.py | 13 +-- webui/static/downloads.js | 98 ++++++++++++++++----- webui/static/style.css | 22 +++++ webui/static/wishlist-tools.js | 4 +- 9 files changed, 164 insertions(+), 40 deletions(-) diff --git a/core/downloads/status.py b/core/downloads/status.py index e9b8d925..7a0ec08c 100644 --- a/core/downloads/status.py +++ b/core/downloads/status.py @@ -312,6 +312,7 @@ def build_batch_status_data(batch_id: str, batch: dict, live_transfers_lookup: d 'ui_state': task.get('ui_state', 'normal'), # normal|cancelling|cancelled 'playlist_id': task.get('playlist_id'), # For V2 system identification 'error_message': task.get('error_message'), # Surface failure reasons to UI + 'quarantine_entry_id': task.get('quarantine_entry_id'), 'has_candidates': bool(task.get('cached_candidates')), # Whether search found results (for clickable review) } _ti = task.get('track_info') if isinstance(task.get('track_info'), dict) else {} diff --git a/core/imports/pipeline.py b/core/imports/pipeline.py index 912ebd8d..2f2f177d 100644 --- a/core/imports/pipeline.py +++ b/core/imports/pipeline.py @@ -35,6 +35,7 @@ from core.imports.context import ( from core.imports.file_integrity import check_audio_integrity, resolve_duration_tolerance from core.imports.filename import extract_track_number_from_filename from core.imports.guards import check_flac_bit_depth, move_to_quarantine +from core.imports.quarantine import entry_id_from_quarantined_filename from core.imports.side_effects import ( emit_track_downloaded, record_download_provenance, @@ -79,6 +80,26 @@ __all__ = [ ] +def _should_skip_quarantine_check(context: dict, check_name: str) -> bool: + bypass = context.get('_skip_quarantine_check') + if bypass == 'all': + return True + if isinstance(bypass, (list, tuple, set)): + return 'all' in bypass or check_name in bypass + return bypass == check_name + + +def _mark_task_quarantined(context: dict, quarantine_path: str | None) -> None: + if not quarantine_path: + return + task_id = context.get('task_id') + if not task_id: + return + with tasks_lock: + if task_id in download_tasks: + download_tasks[task_id]['quarantine_entry_id'] = entry_id_from_quarantined_filename(quarantine_path) + + def build_import_pipeline_runtime( *, automation_engine: Any | None = None, @@ -163,11 +184,9 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta _duration_tolerance_override = resolve_duration_tolerance( config_manager.get('post_processing.duration_tolerance_seconds', 0) ) - # Per-check quarantine bypass — set by `approve_quarantine_entry` - # when the user explicitly approves a previously-quarantined - # file. Skips ONLY the named check; other gates still run. - _bypass_check = context.get('_skip_quarantine_check') - if _bypass_check == 'integrity': + # User-approved quarantine restores can bypass quarantine gates + # for this one post-processing pass. + if _should_skip_quarantine_check(context, 'integrity'): logger.info(f"[Integrity] Skipped (user approval) for {_basename}") integrity = None else: @@ -193,6 +212,7 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta automation_engine, trigger='integrity', ) + _mark_task_quarantined(context, quarantine_path) logger.error(f"File quarantined due to integrity failure: {quarantine_path}") except Exception as quarantine_error: logger.error(f"Quarantine failed ({quarantine_error}), deleting broken file: {file_path}") @@ -227,7 +247,7 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta f"drift={integrity.checks.get('length_drift_s', 'n/a')})" ) - _skip_acoustid = context.get('_skip_quarantine_check') == 'acoustid' + _skip_acoustid = _should_skip_quarantine_check(context, 'acoustid') if _skip_acoustid: logger.info(f"[AcoustID] Skipped (user approval) for {_basename}") try: @@ -273,6 +293,7 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta automation_engine, trigger='acoustid', ) + _mark_task_quarantined(context, quarantine_path) logger.error(f"File quarantined due to verification failure: {quarantine_path}") except Exception as quarantine_error: logger.error(f"Quarantine failed ({quarantine_error}), deleting wrong file: {file_path}") @@ -444,8 +465,9 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta if context['_audio_quality']: logger.info(f"Audio quality detected: {context['_audio_quality']}") - rejection_reason = None if context.get('_skip_quarantine_check') == 'bit_depth' else check_flac_bit_depth(file_path, context) - if context.get('_skip_quarantine_check') == 'bit_depth': + _skip_bit_depth = _should_skip_quarantine_check(context, 'bit_depth') + rejection_reason = None if _skip_bit_depth else check_flac_bit_depth(file_path, context) + if _skip_bit_depth: logger.info(f"[BitDepth] Skipped (user approval) for {_basename}") if rejection_reason: try: @@ -456,6 +478,7 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta automation_engine, trigger='bit_depth', ) + _mark_task_quarantined(context, quarantine_path) logger.info(f"File quarantined due to bit depth filter: {quarantine_path}") except Exception as quarantine_error: logger.error(f"Quarantine failed ({quarantine_error}), deleting file: {file_path}") @@ -575,8 +598,9 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta if context['_audio_quality']: logger.info(f"Audio quality detected: {context['_audio_quality']}") - rejection_reason = None if context.get('_skip_quarantine_check') == 'bit_depth' else check_flac_bit_depth(file_path, context) - if context.get('_skip_quarantine_check') == 'bit_depth': + _skip_bit_depth = _should_skip_quarantine_check(context, 'bit_depth') + rejection_reason = None if _skip_bit_depth else check_flac_bit_depth(file_path, context) + if _skip_bit_depth: logger.info(f"[BitDepth] Skipped (user approval) for {_basename}") if rejection_reason: try: @@ -587,6 +611,7 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta automation_engine, trigger='bit_depth', ) + _mark_task_quarantined(context, quarantine_path) logger.info(f"File quarantined due to bit depth filter: {quarantine_path}") except Exception as quarantine_error: logger.error(f"Quarantine failed ({quarantine_error}), deleting file: {file_path}") diff --git a/core/imports/quarantine.py b/core/imports/quarantine.py index 5b25e299..38be1b41 100644 --- a/core/imports/quarantine.py +++ b/core/imports/quarantine.py @@ -88,6 +88,11 @@ def _entry_id_from_filename(quarantined_filename: str) -> str: return Path(base).stem +def entry_id_from_quarantined_filename(quarantined_filename: str) -> str: + """Derive a quarantine entry id from a quarantined filename or path.""" + return _entry_id_from_filename(os.path.basename(quarantined_filename)) + + def list_quarantine_entries(quarantine_dir: str) -> List[Dict[str, Any]]: """Enumerate quarantined files paired with their sidecars. diff --git a/tests/downloads/test_downloads_status.py b/tests/downloads/test_downloads_status.py index 757f4ddc..aa690866 100644 --- a/tests/downloads/test_downloads_status.py +++ b/tests/downloads/test_downloads_status.py @@ -140,6 +140,7 @@ def test_task_status_includes_v2_state_fields(): 'cancel_requested': True, 'cancel_timestamp': 12345, 'ui_state': 'cancelling', 'playlist_id': 'pl1', 'error_message': 'oh no', 'cached_candidates': [{'x': 1}], + 'quarantine_entry_id': '20260514_120000_song', } batch = {'phase': 'downloading', 'queue': ['t1']} out = st.build_batch_status_data('b1', batch, {}, deps) @@ -149,6 +150,7 @@ def test_task_status_includes_v2_state_fields(): assert t['ui_state'] == 'cancelling' assert t['playlist_id'] == 'pl1' assert t['error_message'] == 'oh no' + assert t['quarantine_entry_id'] == '20260514_120000_song' assert t['has_candidates'] is True diff --git a/tests/imports/test_quarantine_management.py b/tests/imports/test_quarantine_management.py index 0c46055f..97aebe04 100644 --- a/tests/imports/test_quarantine_management.py +++ b/tests/imports/test_quarantine_management.py @@ -4,10 +4,12 @@ import os from core.imports.quarantine import ( approve_quarantine_entry, delete_quarantine_entry, + entry_id_from_quarantined_filename, list_quarantine_entries, recover_to_staging, serialize_quarantine_context, ) +from core.imports.pipeline import _should_skip_quarantine_check # ────────────────────────────────────────────────────────────────────── @@ -146,6 +148,18 @@ def test_list_swallows_corrupt_sidecar_gracefully(tmp_path): assert entries[0]["reason"] == "Unknown reason" +def test_entry_id_helper_handles_paths_and_quarantine_suffix(): + path = "/music/ss_quarantine/20260514_120000_song.flac.quarantined" + assert entry_id_from_quarantined_filename(path) == "20260514_120000_song" + + +def test_quarantine_bypass_all_skips_every_gate(): + context = {"_skip_quarantine_check": "all"} + assert _should_skip_quarantine_check(context, "integrity") is True + assert _should_skip_quarantine_check(context, "acoustid") is True + assert _should_skip_quarantine_check(context, "bit_depth") is True + + # ────────────────────────────────────────────────────────────────────── # delete_quarantine_entry # ────────────────────────────────────────────────────────────────────── diff --git a/web_server.py b/web_server.py index e4e1a022..47a8a653 100644 --- a/web_server.py +++ b/web_server.py @@ -6836,8 +6836,7 @@ def delete_quarantine_item(entry_id): @app.route('/api/quarantine//approve', methods=['POST']) def approve_quarantine_item(entry_id): """One-click approve: restore the file and re-run post-process with the - matching per-check bypass flag set so the original quarantine trigger - is skipped. Other checks still run.""" + quarantine gates skipped for this explicit user-approved pass.""" try: from core.imports.quarantine import approve_quarantine_entry # Restore inside the soulseek download dir so existing path-resolution @@ -6854,8 +6853,10 @@ def approve_quarantine_item(entry_id): "error": "Cannot one-click approve — entry has thin sidecar (no embedded context). Use 'Recover to Staging' instead.", }), 400 restored_path, context, trigger = result - # Mark the bypass so the pipeline skips the trigger that fired. - context['_skip_quarantine_check'] = trigger + # User approval means "import this file"; skip all quarantine gates + # for this one restored pass so multi-reason failures do not loop. + context['_skip_quarantine_check'] = 'all' + context['_approved_quarantine_trigger'] = trigger # Re-dispatch through the same pipeline. Run async so the HTTP # request returns quickly — UI polls /list to see the entry vanish. context_key = f"approve_{entry_id}_{int(time.time())}" @@ -6863,8 +6864,8 @@ def approve_quarantine_item(entry_id): target=lambda: _post_process_matched_download(context_key, context, restored_path), daemon=True, ).start() - logger.info(f"[Quarantine] Approved {entry_id} (bypass={trigger}) → re-running pipeline") - return jsonify({"success": True, "trigger_bypassed": trigger}) + logger.info(f"[Quarantine] Approved {entry_id} (original_trigger={trigger}, bypass=all) → re-running pipeline") + return jsonify({"success": True, "trigger_bypassed": "all", "original_trigger": trigger}) except Exception as e: logger.error(f"[Quarantine] Error approving {entry_id}: {e}") return jsonify({"success": False, "error": str(e)}), 500 diff --git a/webui/static/downloads.js b/webui/static/downloads.js index 23249c0f..8edca77e 100644 --- a/webui/static/downloads.js +++ b/webui/static/downloads.js @@ -1510,46 +1510,54 @@ async function selectWishlistCategory(category) { // Sort tracks by track number albumData.tracks.sort((a, b) => a.trackNumber - b.trackNumber); - const tracksListHTML = albumData.tracks.map(track => ` + const tracksListHTML = albumData.tracks.map(track => { + const safeTrackId = escapeHtml(track.spotifyTrackId || ''); + const safeTrackName = escapeHtml(track.name || 'Unknown Track'); + return `
- ${track.name} -
- `).join(''); + `; + }).join(''); // Handle missing album images with a placeholder + const safeAlbumId = escapeHtml(albumId); + const safeAlbumName = escapeHtml(albumData.albumName || 'Unknown Album'); + const safeArtistName = escapeHtml(albumData.artistName || 'Unknown Artist'); + const safeAlbumImage = escapeHtml(albumData.albumImage || '').replace(/'/g, '''); const albumImageStyle = albumData.albumImage - ? `background-image: url('${albumData.albumImage}')` + ? `background-image: url('${safeAlbumImage}')` : `background: linear-gradient(135deg, rgba(30, 30, 30, 0.9) 0%, rgba(50, 50, 50, 0.9) 100%); display: flex; align-items: center; justify-content: center; font-size: 40px;`; const albumImageContent = albumData.albumImage ? '' : '💿'; albumsHTML += `
-
+
${albumImageContent}
-
${albumData.albumName}
-
${albumData.artistName}
+
${safeAlbumName}
+
${safeArtistName}
${albumData.tracks.length} track${albumData.tracks.length !== 1 ? 's' : ''}
-
▼
- @@ -1601,20 +1609,25 @@ async function selectWishlistCategory(category) { const albumImage = spotifyData?.album?.images?.[0]?.url || ''; const spotifyTrackId = track.spotify_track_id || track.id || ''; + const safeTrackId = escapeHtml(spotifyTrackId); + const safeTrackName = escapeHtml(trackName); + const safeArtistName = escapeHtml(artistName); + const safeAlbumName = escapeHtml(albumName); + const safeAlbumImage = escapeHtml(albumImage).replace(/'/g, '''); tracksHTML += `
-
+
-
${trackName}
-
${artistName} • ${albumName}
+
${safeTrackName}
+
${safeArtistName} • ${safeAlbumName}
-
@@ -3250,6 +3263,41 @@ async function downloadCandidate(taskId, candidate, trackName) { } } +async function approveQuarantineFromDownloadRow(button) { + const entryId = button?.dataset?.entryId || ''; + if (!entryId) { + showToast('Open Quarantine to approve this file.', 'warning'); + return; + } + + const confirmed = await showConfirmDialog({ + title: 'Approve Quarantined File', + message: 'Import this quarantined file and skip quarantine checks for this approved pass?', + confirmText: 'Approve & Import', + cancelText: 'Cancel', + }); + if (!confirmed) return; + + const originalText = button.textContent; + button.disabled = true; + button.textContent = 'Approving...'; + try { + const response = await fetch(`/api/quarantine/${encodeURIComponent(entryId)}/approve`, { method: 'POST' }); + const data = await response.json(); + if (data.success) { + showToast('Approved quarantined file. Re-running post-processing.', 'success'); + } else { + showToast(`Approve failed: ${data.error || 'Unknown error'}`, 'error'); + button.disabled = false; + button.textContent = originalText; + } + } catch (error) { + showToast(`Approve failed: ${error.message}`, 'error'); + button.disabled = false; + button.textContent = originalText; + } +} + function closeCandidatesModal() { const overlay = document.getElementById('candidates-modal-overlay'); if (overlay) { @@ -3374,6 +3422,7 @@ function processModalStatusUpdate(playlistId, data) { const actionsEl = document.getElementById(`actions-${playlistId}-${task.track_index}`); let statusText = ''; + let isQuarantinedTask = false; // V2 SYSTEM: Handle UI state override for cancelling tasks if (isV2Task && uiState === 'cancelling' && task.status !== 'cancelled') { statusText = '🔄 Cancelling...'; @@ -3390,6 +3439,7 @@ function processModalStatusUpdate(playlistId, data) { // failures — the file is recoverable, not lost. const _em = (task.error_message || '').toLowerCase(); if (_em.includes('integrity check failed') || _em.includes('bit depth filter') || _em.includes('verification failed') || _em.includes('quarantin')) { + isQuarantinedTask = true; statusText = '🛡️ Quarantined'; } else { statusText = '❌ Failed'; @@ -3437,6 +3487,13 @@ function processModalStatusUpdate(playlistId, data) { const onclickHandler = isV2Task ? 'cancelTrackDownloadV2' : 'cancelTrackDownload'; actionsEl.innerHTML = ``; } + } else if (actionsEl && task.status === 'failed' && isQuarantinedTask && task.quarantine_entry_id) { + const entryId = escapeHtml(task.quarantine_entry_id); + actionsEl.innerHTML = ``; + const approveBtn = actionsEl.querySelector('.approve-quarantine-inline-btn'); + if (approveBtn) { + approveBtn.addEventListener('click', () => approveQuarantineFromDownloadRow(approveBtn)); + } } else if (actionsEl && ['completed', 'failed', 'cancelled', 'not_found', 'post_processing'].includes(task.status)) { actionsEl.innerHTML = '-'; // No actions available for terminal or processing states } @@ -3535,12 +3592,9 @@ function processModalStatusUpdate(playlistId, data) { setTimeout(() => loadServerPlaylists(), 2000); } - // Auto-close wishlist modal when completed (for auto-processing) + // Keep visible wishlist results open so failed tracks can be reviewed. if (playlistId === 'wishlist') { - console.log('🔄 [Auto-Wishlist] Auto-closing completed wishlist modal to enable next cycle'); - setTimeout(() => { - closeDownloadMissingModal(playlistId); - }, 3000); // 3-second delay to show completion message + console.log('[Wishlist] Leaving completed wishlist modal open for failed-track review'); } // Check if any other processes still need polling diff --git a/webui/static/style.css b/webui/static/style.css index 6e590397..caa6a3eb 100644 --- a/webui/static/style.css +++ b/webui/static/style.css @@ -19904,6 +19904,28 @@ body.helper-mode-active #dashboard-activity-feed:hover { transform: scale(1.05); } +.approve-quarantine-inline-btn { + background: var(--accent-color, #1db954); + color: #ffffff; + border: none; + border-radius: 6px; + padding: 4px 10px; + font-size: 11px; + font-weight: 600; + cursor: pointer; + transition: all 0.2s ease; +} + +.approve-quarantine-inline-btn:hover:not(:disabled) { + filter: brightness(1.1); + transform: scale(1.04); +} + +.approve-quarantine-inline-btn:disabled { + cursor: default; + opacity: 0.65; +} + /* Modal Footer */ .download-missing-modal-footer { background: linear-gradient(135deg, diff --git a/webui/static/wishlist-tools.js b/webui/static/wishlist-tools.js index 8e6df5b2..ee11148f 100644 --- a/webui/static/wishlist-tools.js +++ b/webui/static/wishlist-tools.js @@ -3143,7 +3143,7 @@ function renderQuarantineEntry(entry) { const entryIdAttr = escapeHtml(String(entry.id || '')); const approveLabel = entry.has_full_context ? 'Approve' : 'Recover'; const approveTitle = entry.has_full_context - ? 'Re-run post-processing with only the failing check skipped' + ? 'Re-run post-processing with quarantine checks skipped for this approved file' : 'Legacy entry — move to Staging, finish via Import flow'; const approveCall = entry.has_full_context ? 'approveQuarantineEntryFromButton(this)' @@ -3206,7 +3206,7 @@ function deleteQuarantineEntryFromButton(button) { async function approveQuarantineEntry(entryId) { const ok = await showConfirmDialog({ title: 'Approve Quarantined File', - message: 'Re-run post-processing for this file with only the failing check skipped. The file will be tagged, lyrics generated, and moved into your library. Other quality gates (AcoustID + bit-depth) still run.', + message: 'Re-run post-processing for this file with quarantine checks skipped for this approved pass. The file will be tagged, lyrics generated, and moved into your library.', confirmText: 'Approve & Import', cancelText: 'Cancel', });