Re-audit round 3: 5 blind reviewers, 15 fixes, +18 tests — converging

Round 3's two HIGHs: _fill_version resolved versions with the LAST
same-title entry's cues (photo-aware lookup existed since round 1 but
this caller never used it), and the round-2 diff rework let an earlier
row's disagreement consume the exact-version copy a later row matched.
Diff claims now settle strongest-first across all rows (exact matches,
then versionless upgrades, then disagreement/second-copy), unvetoed
bare duplicates stay owned per spec, and updates are withheld with a
manual-fix note whenever any copy of the game already carries a version
(the row edit targets by name and could hit the wrong copy).

Also: entry-to-row pairing matches by photo overlap before position
(titles.json order churn from reshoot filenames could swap editions);
BGGQueueTimeout defers a title like a missing token; DismissStore
writes atomically, mutates memory only after the write, and
quarantines a torn file instead of bricking the server; version-picker
page-limit exhaustion stays retryable; verify's copy-count shortfall
reports once per game (the old guard was dead code); the upload log
header is created atomically; transient version-lookup failures record
a retryable version_error, not terminal version_unknown; extract
isolates per-photo failures and salvages JSON followed by prose; a
state revision counter stops stale poll responses reverting decisions;
plus the shared-predicate/fsio/docstring consolidation and CLI wiring,
live-diff, verify-wiring, and search-guard tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

tests/test_cli.py

@@ -0,0 +1,64 @@
+"""CLI flag-wiring smoke tests: every option must reach its run_* kwarg.
+
+The commands lazily import their stage modules, so each test monkeypatches
+the stage function at its source module and asserts the received kwargs —
+a transposed or dropped pass-through fails HERE, not on a real run.
+"""
+
+from __future__ import annotations
+
+from typer.testing import CliRunner
+
+from bggpipe.cli import app
+
+runner = CliRunner()
+
+
+def _capture(monkeypatch, module: str, func: str) -> dict:
+    received: dict = {}
+
+    def fake(cfg, **kwargs):
+        received.update(kwargs)
+        received["cfg"] = cfg
+        return []
+
+    monkeypatch.setattr(f"bggpipe.{module}.{func}", fake)
+    return received
+
+
+def test_extract_flags(monkeypatch):
+    received = _capture(monkeypatch, "extract", "run_extract")
+    result = runner.invoke(app, ["extract", "--only", "x.jpg", "--force"])
+    assert result.exit_code == 0
+    assert received["only"] == "x.jpg" and received["force"] is True
+
+
+def test_resolve_force(monkeypatch):
+    received = _capture(monkeypatch, "resolve", "run_resolve")
+    assert runner.invoke(app, ["resolve", "--force"]).exit_code == 0
+    assert received["force"] is True
+
+
+def test_diff_wiring(monkeypatch):
+    received = _capture(monkeypatch, "diff", "run_diff")
+    assert runner.invoke(app, ["diff"]).exit_code == 0
+    assert "cfg" in received
+
+
+def test_upload_flags(monkeypatch):
+    received = _capture(monkeypatch, "upload", "run_upload")
+    result = runner.invoke(
+        app, ["upload", "--dry-run", "--retry-failed", "--limit", "3", "--headless"]
+    )
+    assert result.exit_code == 0
+    assert received["dry_run"] is True
+    assert received["retry_failed"] is True
+    assert received["limit"] == 3
+    assert received["headless"] is True
+    assert received["verify"] is False
+
+
+def test_enrich_refresh(monkeypatch):
+    received = _capture(monkeypatch, "enrich", "run_enrich")
+    assert runner.invoke(app, ["enrich", "--refresh"]).exit_code == 0
+    assert received["refresh"] is True

tests/test_diff.py

@@ -6,14 +6,10 @@ from __future__ import annotations
 from pathlib import Path
 
 from bggpipe.config import Config
-from bggpipe.diff import compute_diff, load_snapshot_collection
+from bggpipe.diff import SNAPSHOT_FILES, compute_diff, load_snapshot_collection
 from bggpipe.models import CollectionItem
 
 FIXTURES = Path(__file__).parent / "fixtures"
-SNAPSHOT_NAMES = (
-    "collection_snapshot_base.xml",
-    "collection_snapshot_expansions.xml",
-)
 
 
 def _item(object_id, coll_id, name="Game", version_id=None, own=True):
@@ -154,17 +150,50 @@ def test_vetoed_duplicate_of_same_version_is_a_real_second_copy():
     assert len(result.second_copies) == 1
 
 
-def test_bare_duplicate_beyond_owned_count_is_added_versionless():
-    # Two vetoed version-unknown rows, one owned copy: the extra bare row
-    # is a version-less second copy, not silently "already owned".
-    rows = [_match("Catan", "13"), _match("Catan", "13")]
-    result = compute_diff(rows, [_item(13, 900)])
+def test_vetoed_bare_duplicate_beyond_owned_count_is_added_versionless():
+    # Two version-unknown rows, one owned copy: only a HUMAN VETO makes the
+    # extra bare row a genuine second copy (spec: a bare id is owned if any
+    # copy exists — an unvetoed typo-read sibling must not upload).
+    vetoed = {**_match("Catan", "13"), "dedupe_veto": "1"}
+    result = compute_diff([_match("Catan", "13"), vetoed], [_item(13, 900)])
     assert result.already_owned == ["Catan"]
     (added,) = result.to_add
     assert added["version_id"] == ""
     assert len(result.second_copies) == 1
 
 
+def test_unvetoed_bare_duplicate_stays_owned():
+    # same shape WITHOUT the veto: both rows owned, nothing uploaded
+    rows = [_match("Catan", "13"), _match("Catan", "13")]
+    result = compute_diff(rows, [_item(13, 900)])
+    assert result.already_owned == ["Catan", "Catan"]
+    assert result.to_add == []
+
+
+def test_earlier_disagreement_cannot_steal_a_later_rows_exact_match():
+    # round-3 ordering bug: row A (v3, no match) must not consume the v2
+    # copy that row B exactly matches — exact matches settle first
+    rows = [
+        _match("Catan", "13", vstatus="version_auto", vid="3", vname="v3"),
+        _match("Catan", "13", vstatus="version_auto", vid="2", vname="v2"),
+    ]
+    result = compute_diff(rows, [_item(13, 900, version_id=2)])
+    assert result.already_owned == ["Catan"]  # B's exact match claims the copy
+    # A's v3 box exists on the shelf and matches no collection entry: a
+    # genuine new copy — NOT a spurious v2 duplicate, NOT a false disagreement
+    assert [r["version_id"] for r in result.to_add] == ["3"]
+    assert result.disagreements == []
+
+
+def test_update_withheld_when_another_copy_is_versioned():
+    # upload's row edit targets by NAME: an update is only safe when every
+    # copy is versionless, else it could overwrite the versioned copy
+    rows = [_match("Catan", "13", vstatus="version_auto", vid="5", vname="5th")]
+    result = compute_diff(rows, [_item(13, 900, version_id=7), _item(13, 901)])
+    assert result.to_update == []
+    assert "set it by hand" in result.disagreements[0]
+
+
 def test_bare_row_does_not_steal_versionless_copy_from_confident_update():
     # ordering independence: the confident row upgrades the versionless
     # copy even when a bare row of the same game appears first in the file
@@ -256,10 +285,7 @@ def test_run_diff_outputs_feed_upload_unchanged(tmp_path, monkeypatch):
     monkeypatch.delenv("BGG_API_TOKEN", raising=False)
     cfg = Config(data_dir=tmp_path)
     fixtures = Path(__file__).parent / "fixtures"
-    for name in (
-        "collection_snapshot_base.xml",
-        "collection_snapshot_expansions.xml",
-    ):
+    for name in SNAPSHOT_FILES:
         shutil.copy(fixtures / name, tmp_path / name)
     write_matches(
         cfg.matches_path,
@@ -290,10 +316,60 @@ def test_token_without_username_says_so(tmp_path, monkeypatch, capsys):
     monkeypatch.delenv("BGG_USERNAME", raising=False)
     cfg = Config(data_dir=tmp_path)  # bgg_username defaults to ""
     fixtures = Path(__file__).parent / "fixtures"
-    for name in SNAPSHOT_NAMES:
+    for name in SNAPSHOT_FILES:
         shutil.copy(fixtures / name, tmp_path / name)
     write_matches(cfg.matches_path, [_match("Catan", "13")])
     run_diff(cfg)
     out = capsys.readouterr().out
     assert "BGG_API_TOKEN is set but BGG_USERNAME is not" in out
     assert "No BGG_API_TOKEN" not in out  # the old message was a lie here
+
+
+class _LiveClient:
+    """Fake client recording collection_full calls for the live branch."""
+
+    def __init__(self, collection, fail_auth=False):
+        self.collection = collection
+        self.fail_auth = fail_auth
+        self.calls: list[dict] = []
+
+    def collection_full(self, username, *, refresh=False):
+        from bggpipe.bgg_client import BGGAuthError
+
+        self.calls.append({"username": username, "refresh": refresh})
+        if self.fail_auth:
+            raise BGGAuthError("token rejected")
+        return self.collection
+
+
+def test_live_diff_fetches_fresh_collection(tmp_path, monkeypatch):
+    # the branch that runs the day the token arrives: must call
+    # collection_full with refresh=True, not serve resolve-era cache
+
+    from bggpipe.diff import run_diff
+    from bggpipe.resolve import write_matches
+
+    monkeypatch.setenv("BGG_API_TOKEN", "tok")
+    monkeypatch.setenv("BGG_USERNAME", "eric")
+    cfg = Config(bgg_username="eric", data_dir=tmp_path)
+    write_matches(cfg.matches_path, [_match("Catan", "13")])
+    client = _LiveClient([_item(13, 1, name="Catan")])
+    result = run_diff(cfg, client=client)
+    assert client.calls == [{"username": "eric", "refresh": True}]
+    assert result.already_owned == ["Catan"]
+
+
+def test_live_diff_falls_back_to_snapshots_on_auth_failure(tmp_path, monkeypatch):
+    import shutil
+
+    from bggpipe.diff import run_diff
+    from bggpipe.resolve import write_matches
+
+    monkeypatch.setenv("BGG_API_TOKEN", "bad")
+    monkeypatch.setenv("BGG_USERNAME", "eric")
+    cfg = Config(bgg_username="eric", data_dir=tmp_path)
+    for name in SNAPSHOT_FILES:
+        shutil.copy(FIXTURES / name, tmp_path / name)
+    write_matches(cfg.matches_path, [_match("5 MINUTE DUNGEON", "207830")])
+    result = run_diff(cfg, client=_LiveClient([], fail_auth=True))
+    assert result.already_owned == ["5 MINUTE DUNGEON"]  # snapshots served

tests/test_models.py

@@ -116,3 +116,29 @@ def test_parse_collection():
 def test_error_document_raises():
     with pytest.raises(BGGResponseError, match="Invalid username"):
         parse_collection(ERROR_XML)
+
+
+def test_search_all_items_malformed_raises():
+    import pytest
+
+    from bggpipe.models import BGGResponseError, parse_search
+
+    xml = '<items total="2"><item type="boardgame"/><item type="boardgame"/></items>'
+    with pytest.raises(BGGResponseError):
+        parse_search(xml)
+
+
+def test_search_partial_malformed_tolerated_with_warning():
+    import pytest
+
+    from bggpipe.models import parse_search
+
+    xml = (
+        '<items total="2">'
+        '<item type="boardgame" id="13"><name type="primary" value="CATAN"/></item>'
+        '<item type="boardgame"/>'
+        "</items>"
+    )
+    with pytest.warns(UserWarning, match="unparseable"):
+        results = parse_search(xml)
+    assert [r.bgg_id for r in results] == [13]

tests/test_resolve.py

@@ -631,3 +631,36 @@ def test_truncation_separator_chosen_by_position():
     heads = _truncation_heads("Blorvath: Quest of the Zzyzx - 2nd Edition")
     assert heads[0] == "Blorvath"
     assert "Blorvath: Quest" not in heads  # the comment's guarantee, now true
+
+
+def test_reordered_titles_json_cannot_mispair_editions(client, tmp_path):
+    # run 1 resolves "Catan" from b.jpg; run 2 prepends a NEW conflicting-cue
+    # "Catan" sighting from a.jpg (sorts earlier). Photo-overlap pairing must
+    # keep the b.jpg row glued to the b.jpg entry — not hand its resolution
+    # (and photos) to the newcomer positionally.
+    data_dir = tmp_path / "data"
+    data_dir.mkdir()
+    entry_b = {
+        "title_raw": "Catan",
+        "language_hint": "English",  # conflicts with a.jpg's German; language
+        "source_photos": ["b.jpg"],  # alone never triggers a versions fetch
+    }
+    (data_dir / "titles.json").write_text(json.dumps([entry_b]))
+    cfg = Config(data_dir=data_dir)
+    run_resolve(cfg, client=client)
+    (row,) = read_matches(cfg.matches_path)
+    assert row["source_photos"] == "b.jpg"
+
+    entry_a = {
+        "title_raw": "Catan",
+        "language_hint": "German",
+        "source_photos": ["a.jpg"],
+    }
+    (data_dir / "titles.json").write_text(json.dumps([entry_a, entry_b]))
+    run_resolve(cfg, client=client)
+
+    rows = read_matches(cfg.matches_path)
+    by_photos = {r["source_photos"]: r for r in rows}
+    assert by_photos["b.jpg"]["match_status"] == "auto"  # kept its resolution
+    assert "a.jpg" in by_photos  # newcomer resolved as its own row
+    assert len(rows) == 2

tests/test_review.py

@@ -419,3 +419,71 @@ def test_every_tui_decision_after_external_rewrite_is_saved(tmp_path):
     )
     assert "Newcomer" in saved  # the external row survived too
     assert session.decisions == 3
+
+
+def test_fill_version_uses_the_approved_rows_own_cues(tmp_path):
+    # round-3 HIGH: two same-title entries are two EDITIONS; the title-only
+    # dict handed every row the LAST entry's cues, scoring the wrong version
+    import json as _json
+
+    entry_good = {
+        "title_raw": "Wingspan",
+        "publisher_hint": "Stonemaier",  # matches the fixture's version
+        "year_hint": 2019,
+        "source_photos": ["good.jpg"],
+    }
+    entry_bad = {
+        "title_raw": "Wingspan",
+        "publisher_hint": "Nobody Media",  # matches nothing
+        "edition_hint": "Imaginary edition",
+        "source_photos": ["bad.jpg"],
+    }
+    cfg = _setup(
+        tmp_path,
+        [
+            _row(
+                title_raw="Wingspan",
+                match_status="ambiguous",
+                source_photos="good.jpg",
+                candidates_json=_json.dumps(
+                    [{"bgg_id": 266192, "name": "Wingspan", "year": 2019}]
+                ),
+            )
+        ],
+    )
+    (cfg.data_dir / "titles.json").write_text(_json.dumps([entry_good, entry_bad]))
+    session = ReviewSession(
+        cfg, console=quiet_console(), input_fn=scripted(), client=fixture_client()
+    )
+    row = session.rows[0]
+    session.decide_pick(row, {"bgg_id": 266192, "name": "Wingspan", "year": 2019})
+    # with last-wins cues (entry_bad) this was version_unknown; the row's own
+    # photo (good.jpg) must select entry_good's cues and find the version
+    assert row["version_status"] == "version_auto"
+    assert row["version_id"] == "465063"
+
+
+def test_dismiss_failure_keeps_ticket_visible(tmp_path, monkeypatch):
+    import bggpipe.webreview as webreview_mod
+    from bggpipe.webreview import DismissStore
+
+    store = DismissStore(tmp_path / "dismissed.json")
+
+    def exploding(path, text):
+        raise OSError("disk full")
+
+    monkeypatch.setattr(webreview_mod, "atomic_write_text", exploding)
+    with pytest.raises(OSError):
+        store.add("photo|loc|txt|art")
+    assert store.keys == set()  # memory never claims what disk doesn't hold
+
+
+def test_corrupt_dismiss_file_is_quarantined_not_fatal(tmp_path):
+    from bggpipe.webreview import DismissStore
+
+    path = tmp_path / "dismissed.json"
+    path.write_text('["torn')
+    with pytest.warns(UserWarning, match="unreadable"):
+        store = DismissStore(path)
+    assert store.keys == set()
+    assert (tmp_path / "dismissed.json.corrupt").exists()

tests/test_upload.py

@@ -22,7 +22,9 @@ from bggpipe.upload import (
     verify_uploads,
 )
 
-NOW = lambda: "2026-08-01T00:00:00+00:00"  # noqa: E731
+
+def NOW() -> str:
+    return "2026-08-01T00:00:00+00:00"
 
 
 def _cfg(tmp_path: Path) -> Config:
@@ -425,3 +427,44 @@ def test_empty_game_name_is_refused_not_uploaded(tmp_path):
     fake = FakeUploader()
     results = run_upload(cfg, uploader=fake, sleep=lambda s: None, now=NOW)
     assert results == [] and fake.calls == []
+
+
+def test_verify_shortfall_reported_once_per_game(tmp_path):
+    # two DONE adds (different versions) of one game, one copy on BGG:
+    # exactly ONE shortfall problem (the old _job_key guard was dead code
+    # and double-reported)
+    log = [
+        _log_row(action="add", bgg_id="7", version_id="1", status="added"),
+        _log_row(action="add", bgg_id="7", version_id="2", status="added"),
+    ]
+    problems = verify_uploads(log, [_item(7, 70, version_id=1)])
+    shortfalls = [p for p in problems if "add(s) logged" in p]
+    assert len(shortfalls) == 1
+
+
+class _VerifyClient:
+    def __init__(self, collection):
+        self.collection = collection
+        self.calls: list[dict] = []
+
+    def collection_full(self, username, *, refresh=False):
+        self.calls.append({"username": username, "refresh": refresh})
+        return self.collection
+
+
+def test_run_upload_verify_wiring(tmp_path, capsys):
+    # verify=True must re-fetch the LIVE collection (refresh) and cross-check
+    cfg = _cfg(tmp_path)
+    _seed_data(tmp_path, to_add=[_add_row(bgg_id="1", name="Wingspan")])
+    run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=NOW)
+    client = _VerifyClient([_item(1, 10, name="Wingspan")])
+    run_upload(
+        cfg,
+        uploader=FakeUploader(),
+        verify=True,
+        client=client,
+        sleep=lambda s: None,
+        now=NOW,
+    )
+    assert client.calls == [{"username": "tester", "refresh": True}]
+    assert "Verification OK" in capsys.readouterr().out