fix(quality): correct ranking behaviour flagged in PR #896 (#3, #4)

#3 priority mode is quality-agnostic again: search_with_fallback (the
priority/hybrid path — best_quality has its own search_all_sources) returned
the first source whose results met a target, so an mp3 with bitrate=None (slskd
omits it often) was deemed unsatisfied and deprioritised, changing which source
wins for users who never opted in. Restore "first source with tracks wins",
byte-for-byte; cross-source quality pooling stays in best_quality mode.

#4 metadata-less FLAC no longer over-claims a hi-res target: matches_target let
a FLAC with no sample_rate/bit_depth satisfy a 24-bit/192k target while a real
16/44 FLAC failed it, so unknown-spec files outranked and discarded genuine CD
FLAC under audiophile/hi-res profiles. An unconfirmable spec now fails the strict
tier and falls to the plain-flac bucket.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

core/download_engine/engine.py

@@ -29,7 +29,6 @@ import asyncio
 import threading
 from typing import Any, Dict, Iterator, List, Optional, Tuple
 
-from core.quality.selection import load_profile_targets, rank_with_targets
 from utils.logging_config import get_logger
 
 logger = get_logger("download_engine")
@@ -393,21 +392,19 @@ class DownloadEngine:
         (tracks, albums) tuple, or ``([], [])`` when every source
         in the chain is exhausted.
 
-        Quality-aware: a source is only accepted when it can deliver a
+        Priority mode is deliberately quality-AGNOSTIC at search time — source
-        candidate that meets a real target in the user's quality profile.
+        order is king and the first source that returns any tracks wins, exactly
-        A source that meets no target is skipped in favour of the next one
+        matching pre-quality-system behaviour byte-for-byte (#896 review #3).
-        (source priority still wins among satisfying sources). RAW tracks are
+        Quality-gating the priority path would deprioritise e.g. a soulseek
-        returned — the per-source ``satisfied`` check is a coarse gate;
+        mp3 whose bitrate slskd omitted (``bitrate=None`` → "unsatisfied"),
-        match-filtering and final quality ranking happen in the orchestrator.
+        changing which source wins and adding latency for users who never opted
-        When no source satisfies a target, the first source's results are
+        in. Cross-source quality pooling is the job of best_quality mode
-        returned as a fallback (unless the profile disables fallback).
+        (``search_all_sources``); final per-result ranking still happens in the
+        orchestrator's match/quality filter. RAW tracks are returned.
 
         Replaces orchestrator's hand-rolled hybrid search loop. The
         chain is ordered (most-preferred first).
         """
-        targets, fallback_enabled = load_profile_targets()
-        first_fallback = None  # (tracks, albums) of the first source with hits
-
         for i, source_name in enumerate(source_chain):
             plugin = self._plugins.get(source_name)
             if plugin is None:
@@ -422,41 +419,13 @@ class DownloadEngine:
                 tracks, albums = await plugin.search(query, timeout, progress_callback)
                 if not tracks:
                     continue
-                try:
+                logger.info(f"{source_name} found {len(tracks)} tracks")
-                    _, satisfied = rank_with_targets(
+                return (tracks, albums)
-                        tracks, targets, fallback_enabled=fallback_enabled,
-                    )
-                except Exception as rank_err:
-                    # Fail open: a ranking bug must never drop a source's real
-                    # download results. Accept them as-is.
-                    logger.warning(
-                        f"Quality ranking failed for {source_name} — accepting "
-                        f"its results unranked: {rank_err}"
-                    )
-                    satisfied = True
-                if satisfied:
-                    logger.info(
-                        f"{source_name} found {len(tracks)} tracks (meets quality target)"
-                    )
-                    return (tracks, albums)
-                logger.info(
-                    f"{source_name} found {len(tracks)} tracks but none meet a "
-                    f"quality target — trying next source"
-                )
-                if first_fallback is None:
-                    first_fallback = (tracks, albums)
             except Exception as e:
                 logger.warning(f"{source_name} search failed: {e}")
 
-        if fallback_enabled and first_fallback is not None:
-            logger.info(
-                "Hybrid search: no source met a quality target — falling back to "
-                "first source's results"
-            )
-            return first_fallback
-
         logger.warning(
-            "Hybrid search: all sources (%s) found nothing acceptable for: %s",
+            "Hybrid search: all sources (%s) found nothing for: %s",
             ', '.join(source_chain), query,
         )
         return ([], [])

core/quality/model.py

@@ -67,21 +67,33 @@ class AudioQuality:
                 if self.sample_rate < target.min_sample_rate:
                     return False
             else:
-                # No sample-rate info — fall back to kbps heuristic for FLAC.
+                # No sample-rate metadata (common on slskd FLAC). Use the kbps
+                # heuristic when a bitrate is present; otherwise we CANNOT
+                # confirm the spec, so fail the strict target rather than
+                # over-claim it — an unknown-spec FLAC must not outrank a known
+                # 16/44 FLAC under a hi-res target (#896 review #4). It falls to
+                # the plain-flac bucket instead.
                 # 16-bit/44.1 kHz ≈ 1411 kbps; 24-bit/96 kHz ≈ 4608 kbps.
                 if self.format.lower() == 'flac' and self.bitrate:
                     required_kbps = _sample_rate_to_min_kbps(target.min_sample_rate, target.bit_depth or 24)
                     if self.bitrate < required_kbps:
                         return False
+                else:
+                    return False
         if target.bit_depth:
             if self.bit_depth is not None:
                 if self.bit_depth < target.bit_depth:
                     return False
             else:
-                # No bit-depth info — use kbps heuristic.
+                # No bit-depth metadata. A hi-res (>=24-bit) target needs proof:
-                # 16-bit FLAC ≈ ≤1450 kbps; 24-bit starts ~1500 kbps.
+                # use the kbps heuristic if a bitrate is present, else fail
-                if self.format.lower() == 'flac' and target.bit_depth == 24:
+                # rather than over-claim. The 16-bit baseline still matches an
-                    if self.bitrate and self.bitrate < 1450:
+                # unknown-spec FLAC (any FLAC is at least CD quality). #896 review #4.
+                if self.format.lower() == 'flac' and target.bit_depth >= 24:
+                    if self.bitrate:
+                        if self.bitrate < 1450:
+                            return False
+                    else:
                         return False
         return True
 

tests/quality/test_engine_fallback.py

@@ -1,15 +1,18 @@
-"""Engine search_with_fallback is quality-aware: it falls through to the next
+"""Engine ``search_with_fallback`` is the PRIORITY-mode search path and is
-source when the current source can deliver no target-satisfying quality, but
+deliberately quality-agnostic: the first source in the chain that returns any
-returns RAW tracks (match-filtering happens later in the orchestrator).
+tracks wins (source order is king), byte-for-byte like the pre-quality-system
+hybrid loop (#896 review #3). It skips unconfigured/unavailable sources,
+swallows per-source exceptions, and returns RAW tracks (match-filtering +
+final quality ranking happen later). Cross-source quality pooling lives in
+best_quality mode (``search_all_sources``), not here.
 """
 
 import asyncio
 
 import pytest
 
-from core.download_engine import engine as engine_mod
 from core.download_engine.engine import DownloadEngine
-from core.quality.model import AudioQuality, QualityTarget
+from core.quality.model import AudioQuality
 
 
 class _Cand:
@@ -19,21 +22,25 @@ class _Cand:
 
 
 class _FakePlugin:
-    def __init__(self, tracks):
+    def __init__(self, tracks, configured=True, raises=False):
         self._tracks = tracks
+        self._configured = configured
+        self._raises = raises
         self.searched = False
 
     def is_configured(self):
-        return True
+        return self._configured
 
     async def search(self, query, timeout=None, progress_callback=None):
         self.searched = True
+        if self._raises:
+            raise RuntimeError("boom")
         return (self._tracks, [])
 
 
 FLAC = AudioQuality('flac', sample_rate=44100, bit_depth=16)
 MP3 = AudioQuality('mp3', bitrate=320)
-WANT_FLAC_ONLY = [QualityTarget(label='FLAC 16', format='flac', bit_depth=16)]
+MP3_NO_BITRATE = AudioQuality('mp3')  # slskd often omits bitrate
 
 
 def _engine_with(plugins):
@@ -42,67 +49,79 @@ def _engine_with(plugins):
     return eng
 
 
-def _patch_profile(monkeypatch, targets, fallback_enabled):
+def test_returns_first_source_with_tracks_source_priority_king():
-    monkeypatch.setattr(
-        engine_mod, 'load_profile_targets',
-        lambda: (targets, fallback_enabled),
-    )
-
-
-def test_escalates_to_next_source_when_first_cannot_meet_target(monkeypatch):
-    _patch_profile(monkeypatch, WANT_FLAC_ONLY, True)
     first = _FakePlugin([_Cand(MP3, 'a-mp3')])
     second = _FakePlugin([_Cand(FLAC, 'b-flac')])
     eng = _engine_with({'first': first, 'second': second})
 
     tracks, _ = asyncio.run(eng.search_with_fallback('q', ['first', 'second']))
 
-    assert [t.name for t in tracks] == ['b-flac']  # escalated to FLAC source
+    assert [t.name for t in tracks] == ['a-mp3']  # first source wins regardless of quality
-    assert second.searched is True
+    assert second.searched is False               # never queried — priority is king
 
 
-def test_stops_on_first_satisfying_source(monkeypatch):
+def test_metadata_less_mp3_still_wins_in_priority_mode():
-    _patch_profile(monkeypatch, WANT_FLAC_ONLY, True)
+    """An mp3 whose bitrate slskd omitted must NOT be deprioritised in priority
-    first = _FakePlugin([_Cand(FLAC, 'a-flac')])
+    mode — the whole point of #896 review #3."""
+    first = _FakePlugin([_Cand(MP3_NO_BITRATE, 'a-mp3')])
     second = _FakePlugin([_Cand(FLAC, 'b-flac')])
     eng = _engine_with({'first': first, 'second': second})
 
     tracks, _ = asyncio.run(eng.search_with_fallback('q', ['first', 'second']))
 
-    assert [t.name for t in tracks] == ['a-flac']
+    assert [t.name for t in tracks] == ['a-mp3']
-    assert second.searched is False  # never queried — source priority king
+    assert second.searched is False
 
 
-def test_returns_raw_tracks_not_pruned(monkeypatch):
+def test_skips_to_next_source_when_first_empty():
-    # A source satisfied by one candidate must still return ALL its tracks so
+    first = _FakePlugin([])
-    # the orchestrator's match filter can pick the correct one.
+    second = _FakePlugin([_Cand(FLAC, 'b-flac')])
-    _patch_profile(monkeypatch, WANT_FLAC_ONLY, True)
+    eng = _engine_with({'first': first, 'second': second})
-    first = _FakePlugin([_Cand(MP3, 'wrong-but-present'), _Cand(FLAC, 'flac')])
+
+    tracks, _ = asyncio.run(eng.search_with_fallback('q', ['first', 'second']))
+
+    assert [t.name for t in tracks] == ['b-flac']
+    assert second.searched is True
+
+
+def test_returns_raw_tracks_not_pruned():
+    # All of the winning source's tracks come back so the orchestrator's match
+    # filter can pick the correct one.
+    first = _FakePlugin([_Cand(MP3, 'one'), _Cand(FLAC, 'two')])
     eng = _engine_with({'first': first})
 
     tracks, _ = asyncio.run(eng.search_with_fallback('q', ['first']))
 
-    names = {t.name for t in tracks}
+    assert {t.name for t in tracks} == {'one', 'two'}
-    assert names == {'wrong-but-present', 'flac'}  # nothing pruned
 
 
-def test_no_source_satisfies_fallback_on_returns_first_source(monkeypatch):
+def test_skips_unconfigured_source():
-    _patch_profile(monkeypatch, WANT_FLAC_ONLY, True)
+    first = _FakePlugin([_Cand(FLAC, 'a')], configured=False)
-    first = _FakePlugin([_Cand(MP3, 'a-mp3')])
+    second = _FakePlugin([_Cand(MP3, 'b')])
-    second = _FakePlugin([_Cand(MP3, 'b-mp3')])
     eng = _engine_with({'first': first, 'second': second})
 
     tracks, _ = asyncio.run(eng.search_with_fallback('q', ['first', 'second']))
 
-    assert [t.name for t in tracks] == ['a-mp3']  # source priority for fallback
+    assert [t.name for t in tracks] == ['b']
+    assert first.searched is False
 
 
-def test_no_source_satisfies_fallback_off_returns_empty(monkeypatch):
+def test_swallows_per_source_exception_and_continues():
-    _patch_profile(monkeypatch, WANT_FLAC_ONLY, False)
+    first = _FakePlugin([], raises=True)
-    first = _FakePlugin([_Cand(MP3, 'a-mp3')])
+    second = _FakePlugin([_Cand(MP3, 'b')])
-    eng = _engine_with({'first': first})
+    eng = _engine_with({'first': first, 'second': second})
 
-    tracks, albums = asyncio.run(eng.search_with_fallback('q', ['first']))
+    tracks, _ = asyncio.run(eng.search_with_fallback('q', ['first', 'second']))
+
+    assert [t.name for t in tracks] == ['b']
+
+
+def test_returns_empty_when_all_sources_empty():
+    first = _FakePlugin([])
+    second = _FakePlugin([])
+    eng = _engine_with({'first': first, 'second': second})
+
+    tracks, albums = asyncio.run(eng.search_with_fallback('q', ['first', 'second']))
 
     assert tracks == []
     assert albums == []

tests/quality/test_model.py

@@ -58,6 +58,32 @@ def test_format_mismatch_rejected():
     assert AudioQuality('mp3', bitrate=320).matches_target(t) is False
 
 
+# ── metadata-less FLAC must not over-claim a hi-res target (#896 review #4) ─
+
+def test_metadata_less_flac_does_not_overclaim_hires_target():
+    """A FLAC with no sample_rate/bit_depth metadata (common on slskd) must NOT
+    satisfy a strict hi-res target — otherwise it outranks and discards a real
+    16/44 FLAC. Unknown spec fails the high tier, falling to a plain flac bucket."""
+    hires = QualityTarget(format='flac', bit_depth=24, min_sample_rate=192000)
+    assert AudioQuality('flac').matches_target(hires) is False
+
+
+def test_metadata_less_flac_does_not_overclaim_bit_depth_only_hires():
+    """Same guard when the hi-res target constrains only bit depth."""
+    hires = QualityTarget(format='flac', bit_depth=24)
+    assert AudioQuality('flac').matches_target(hires) is False
+
+
+def test_metadata_less_flac_matches_plain_flac_target():
+    """A bare FLAC still matches a plain 'flac (any)' target (the baseline)."""
+    assert AudioQuality('flac').matches_target(QualityTarget(format='flac')) is True
+
+
+def test_metadata_less_flac_matches_16bit_baseline_target():
+    """A bare FLAC satisfies the 16-bit baseline (any FLAC is >= CD quality)."""
+    assert AudioQuality('flac').matches_target(QualityTarget(format='flac', bit_depth=16)) is True
+
+
 # ── v2 -> v3 migration preserves the user's priority order ─────────────────
 
 def test_v2_to_v3_preserves_order_and_maps_fields():