Merge pull request #633 from Nezreka/codex/quarantine-followups

Harden quarantine approval flows

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 {}

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`
+        # User-approved quarantine restores can bypass quarantine gates
-        # when the user explicitly approves a previously-quarantined
+        # for this one post-processing pass.
-        # file. Skips ONLY the named check; other gates still run.
+        if _should_skip_quarantine_check(context, 'integrity'):
-        _bypass_check = context.get('_skip_quarantine_check')
-        if _bypass_check == '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)
+            _skip_bit_depth = _should_skip_quarantine_check(context, 'bit_depth')
-            if context.get('_skip_quarantine_check') == '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)
+            _skip_bit_depth = _should_skip_quarantine_check(context, 'bit_depth')
-            if context.get('_skip_quarantine_check') == '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}")

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.
 

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
 
 

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
 # ──────────────────────────────────────────────────────────────────────

web_server.py

@@ -6836,8 +6836,7 @@ def delete_quarantine_item(entry_id):
 @app.route('/api/quarantine/<entry_id>/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
+    quarantine gates skipped for this explicit user-approved pass."""
-    is skipped. Other checks still run."""
     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.
+        # User approval means "import this file"; skip all quarantine gates
-        context['_skip_quarantine_check'] = trigger
+        # 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")
+        logger.info(f"[Quarantine] Approved {entry_id} (original_trigger={trigger}, bypass=all) → re-running pipeline")
-        return jsonify({"success": True, "trigger_bypassed": trigger})
+        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

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 `
                     <div class="wishlist-album-track wishlist-track-item">
                         <label class="wishlist-checkbox-wrapper">
                             <input type="checkbox" class="wishlist-select-cb"
-                                   data-track-id="${track.spotifyTrackId}">
+                                   data-track-id="${safeTrackId}">
                             <span class="wishlist-checkbox-custom"></span>
                         </label>
-                        <span class="wishlist-album-track-name">${track.name}</span>
+                        <span class="wishlist-album-track-name">${safeTrackName}</span>
-                        <button class="wishlist-delete-btn wishlist-delete-btn-small" data-track-id="${track.spotifyTrackId}" title="Remove from wishlist">
+                        <button class="wishlist-delete-btn wishlist-delete-btn-small" data-track-id="${safeTrackId}" title="Remove from wishlist">
                             🗑️
                         </button>
                     </div>
-                `).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, '&#39;');
                 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 ? '' : '<span style="opacity: 0.3;">💿</span>';
 
                 albumsHTML += `
                     <div class="wishlist-album-card">
-                        <div class="wishlist-album-header" data-album-id="${albumId}">
+                        <div class="wishlist-album-header" data-album-id="${safeAlbumId}">
                             <label class="wishlist-checkbox-wrapper">
                                 <input type="checkbox" class="wishlist-album-select-all-cb"
-                                       data-album-id="${albumId}">
+                                       data-album-id="${safeAlbumId}">
                                 <span class="wishlist-checkbox-custom"></span>
                             </label>
                             <div class="wishlist-album-image" style="${albumImageStyle}">${albumImageContent}</div>
                             <div class="wishlist-album-info">
-                                <div class="wishlist-album-name">${albumData.albumName}</div>
+                                <div class="wishlist-album-name">${safeAlbumName}</div>
-                                <div class="wishlist-album-artist">${albumData.artistName}</div>
+                                <div class="wishlist-album-artist">${safeArtistName}</div>
                                 <div class="wishlist-album-track-count">${albumData.tracks.length} track${albumData.tracks.length !== 1 ? 's' : ''}</div>
                             </div>
-                            <button class="wishlist-delete-btn wishlist-delete-album-btn" data-album-id="${albumId}" title="Remove all tracks from album">
+                            <button class="wishlist-delete-btn wishlist-delete-album-btn" data-album-id="${safeAlbumId}" title="Remove all tracks from album">
                                 🗑️
                             </button>
                             <div class="wishlist-album-expand-icon" id="expand-icon-${albumId}">▼</div>
                         </div>
-                        <div class="wishlist-album-tracks" id="tracks-${albumId}" style="display: none;">
+                        <div class="wishlist-album-tracks" id="tracks-${safeAlbumId}" style="display: none;">
                             ${tracksListHTML}
                         </div>
                     </div>
@@ -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, '&#39;');
 
                 tracksHTML += `
                     <div class="playlist-track-item-with-image wishlist-track-item">
                         <label class="wishlist-checkbox-wrapper">
                             <input type="checkbox" class="wishlist-select-cb"
-                                   data-track-id="${spotifyTrackId}">
+                                   data-track-id="${safeTrackId}">
                             <span class="wishlist-checkbox-custom"></span>
                         </label>
-                        <div class="playlist-track-image" style="background-image: url('${albumImage}')"></div>
+                        <div class="playlist-track-image" style="background-image: url('${safeAlbumImage}')"></div>
                         <div class="playlist-track-info">
-                            <div class="playlist-track-name">${trackName}</div>
+                            <div class="playlist-track-name">${safeTrackName}</div>
-                            <div class="playlist-track-artist">${artistName} • ${albumName}</div>
+                            <div class="playlist-track-artist">${safeArtistName} • ${safeAlbumName}</div>
                         </div>
-                        <button class="wishlist-delete-btn" data-track-id="${spotifyTrackId}" title="Remove from wishlist">
+                        <button class="wishlist-delete-btn" data-track-id="${safeTrackId}" title="Remove from wishlist">
                             🗑️
                         </button>
                     </div>
@@ -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 = `<button class="cancel-track-btn" title="Cancel this download" onclick="${onclickHandler}('${playlistId}', ${task.track_index})">×</button>`;
                 }
+            } else if (actionsEl && task.status === 'failed' && isQuarantinedTask && task.quarantine_entry_id) {
+                const entryId = escapeHtml(task.quarantine_entry_id);
+                actionsEl.innerHTML = `<button class="approve-quarantine-inline-btn" data-entry-id="${entryId}" title="Approve quarantined file">Approve</button>`;
+                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');
+                console.log('[Wishlist] Leaving completed wishlist modal open for failed-track review');
-                setTimeout(() => {
-                    closeDownloadMissingModal(playlistId);
-                }, 3000); // 3-second delay to show completion message
             }
 
             // Check if any other processes still need polling

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,

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',
     });