Audit: 5-reviewer sweep — 19 fixes across every stage, +24 tests

Correctness: review vetoes persist via a dedupe_veto column (resolve
re-runs no longer overturn humans); diff emits second copies whose
confident version matches no owned copy (spec: pairs own only on both
ids) and fetches the live collection with refresh; resolve pairs
titles.json entries to rows by title so a reshoot photo updates
provenance instead of duplicating rows; version lookups survive empty
/thing results; publisher tie-break now honors the mixed
base/expansion veto and refuses multi-candidate picks; empty-normalized
(non-Latin) titles never count as exact.

Upload: LoginError aborts a run instead of logging N bogus failures
(and 3 identical consecutive failures abort as systemic); Cloudflare
interstitials are detected; added-without-version gets its own logged
status that verify understands; same-game updates run one per pass so
the name-targeted row edit can't overwrite a fresh version; absent
diff outputs fail loudly; pagination clicks are paced.

Web review: a lock serializes freshen/decide (threadpool race dropped
decisions); failed saves roll memory back and always alert the browser
(non-JSON 500s included); session warnings reach the page instead of a
StringIO; state-load failures and dead servers show banners instead of
a blank page; duplicate (title, photos) rows are addressable by
ordinal.

Consistency: shared CONFIDENT_VERSION_STATUSES, client_for(),
Config paths for every artifact, one review-port constant, named
matching thresholds, strict collection-id parsing, error-doc responses
never cached, unknown config keys warn, extract reports dropped vision
entries, fixture generators share escaping + marker text.

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

tests/test_client.py

@@ -131,3 +131,54 @@ def test_401_raises_actionable_auth_error(tmp_path, monkeypatch):
     client, _, _ = make_client(tmp_path, [(401, "Unauthorized")])
     with pytest.raises(BGGAuthError, match="BGG_API_TOKEN"):
         client.get_xml("search", {"query": "catan"})
+
+
+# -- audit-fix regressions ----------------------------------------------
+
+
+def test_http_200_error_document_raises_and_is_never_cached(tmp_path):
+    # BGG serves some errors as HTTP 200 <errors> XML; caching one would
+    # poison every future run for that query
+    from bggpipe.models import BGGResponseError
+
+    errors_xml = "<errors><error><message>Invalid username</message></error></errors>"
+    client, _, _ = make_client(tmp_path, [(200, errors_xml)])
+    with pytest.raises(BGGResponseError):
+        client.get_xml("collection", {"username": "nobody", "own": "1"})
+    assert list((tmp_path / "cache").glob("*.xml")) == []
+
+
+def test_collection_full_merges_and_dedupes_by_collid(tmp_path):
+    base_xml = (
+        '<items totalitems="2">'
+        '<item objectid="13" collid="100" subtype="boardgame">'
+        '<name>Catan</name><status own="1"/></item>'
+        '<item objectid="177" collid="101" subtype="boardgame">'
+        '<name>Advanced Civilization</name><status own="1"/></item>'
+        "</items>"
+    )
+    expansion_xml = (
+        '<items totalitems="1">'
+        '<item objectid="177" collid="101" subtype="boardgameexpansion">'
+        '<name>Advanced Civilization</name><status own="1"/></item>'
+        "</items>"
+    )
+    client, _, _ = make_client(tmp_path, [(200, base_xml), (200, expansion_xml)])
+    items = client.collection_full("eric")
+    assert len(items) == 2  # collid 101 appears in both responses: one copy
+    assert {i.coll_id for i in items} == {100, 101}
+
+
+def test_collection_item_missing_collid_refuses_to_parse(tmp_path):
+    # a truncated response must fail loudly, not coerce ids to 0 and let
+    # the dedupe silently drop owned games
+    from bggpipe.models import BGGResponseError, parse_collection
+
+    bad_xml = (
+        '<items totalitems="1">'
+        '<item objectid="13" subtype="boardgame">'
+        '<name>Catan</name><status own="1"/></item>'
+        "</items>"
+    )
+    with pytest.raises(BGGResponseError):
+        parse_collection(bad_xml)

tests/test_config.py

@@ -1,3 +1,7 @@
+"""Config loading: toml knobs, env-only username, unknown-key warning."""
+
+from __future__ import annotations
+
 from pathlib import Path
 
 from bggpipe.config import Config, load_config
@@ -29,3 +33,14 @@ def test_username_comes_from_env_only(tmp_path, monkeypatch):
     assert load_config(p).bgg_username == "from_env"
     monkeypatch.delenv("BGG_USERNAME")
     assert load_config(p).bgg_username == ""
+
+
+def test_unknown_toml_keys_warn(tmp_path, monkeypatch):
+    # a typo'd knob must not silently fall back to defaults
+    import pytest
+
+    monkeypatch.delenv("BGG_USERNAME", raising=False)
+    p = tmp_path / "config.toml"
+    p.write_text('photo_dir = "oops"\n')
+    with pytest.warns(UserWarning, match="photo_dir"):
+        load_config(p)

tests/test_diff.py

@@ -5,6 +5,7 @@ from __future__ import annotations
 
 from pathlib import Path
 
+from bggpipe.config import Config
 from bggpipe.diff import compute_diff, load_snapshot_collection
 from bggpipe.models import CollectionItem
 
@@ -114,7 +115,10 @@ def test_owned_with_matching_version_is_just_owned():
     assert not result.to_update and not result.to_add
 
 
-def test_owned_with_different_version_reports_disagreement_untouched():
+def test_confident_version_matching_no_copy_is_a_second_copy_to_add():
+    # Spec: a (bgg_id, version_id) pair is owned only if a collection item
+    # matches BOTH. All copies carry different versions -> this is an
+    # additional physical copy; existing entries are never edited.
     result = compute_diff(
         [
             _match(
@@ -127,9 +131,10 @@ def test_owned_with_different_version_reports_disagreement_untouched():
         ],
         [_item(266192, 5, version_id=465063)],
     )
-    assert result.already_owned == ["Wingspan"]
     assert not result.to_update  # additive only: never edit a set version
-    assert "fourth printing" in result.disagreements[0]
+    assert [r["version_id"] for r in result.to_add] == ["521212"]
+    assert "fourth printing" in result.second_copies[0]
+    assert result.already_owned == []
 
 
 def test_version_unknown_owned_by_bare_id():
@@ -169,8 +174,11 @@ def test_pending_rejected_and_unseen_are_reported():
 def test_merged_rows_are_skipped_but_photos_carry_to_survivor():
     matches = [
         _match("Joking Hazard", "193621"),
-        {**_match("Jokin Ha...", "193621", status="merged"),
-         "merged_into": "Joking Hazard", "source_photos": "other.jpg"},
+        {
+            **_match("Jokin Ha...", "193621", status="merged"),
+            "merged_into": "Joking Hazard",
+            "source_photos": "other.jpg",
+        },
     ]
     result = compute_diff(matches, [])  # empty collection -> to_add
     assert result.merged == 1
@@ -178,3 +186,54 @@ def test_merged_rows_are_skipped_but_photos_carry_to_survivor():
     (row,) = result.to_add
     assert row["title_raw"] == "Joking Hazard"
     assert row["source_photos"] == "other.jpg;x.jpg"  # combined
+
+
+def test_versionless_copies_exhaust_then_second_copy_becomes_add():
+    # two confident-version matches, ONE versionless copy: the first consumes
+    # it (to_update), the second is an additional physical copy (to_add)
+    result = compute_diff(
+        [
+            _match("Sorcerer", "39", vstatus="version_auto", vid="111", vname="1st"),
+            _match("Sorcerer", "39", vstatus="version_auto", vid="222", vname="2nd"),
+        ],
+        [_item(39, 701)],
+    )
+    assert [u["version_id"] for u in result.to_update] == ["111"]
+    assert [a["version_id"] for a in result.to_add] == ["222"]
+    assert len(result.second_copies) == 1
+
+
+def test_run_diff_outputs_feed_upload_unchanged(tmp_path, monkeypatch):
+    # the cross-stage contract: whatever run_diff writes, run_upload must
+    # read — a column rename on either side has to fail HERE
+    import shutil
+
+    from bggpipe.diff import run_diff
+    from bggpipe.resolve import write_matches
+    from bggpipe.upload import run_upload
+
+    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",
+    ):
+        shutil.copy(fixtures / name, tmp_path / name)
+    write_matches(
+        cfg.matches_path,
+        [
+            _match("Wingspan", "266192", status="auto"),  # not in the snapshots
+            _match("5 MINUTE DUNGEON", "207830", status="auto"),  # owned
+        ],
+    )
+    result = run_diff(cfg)
+    assert [r["bgg_id"] for r in result.to_add] == ["266192"]
+
+    from test_upload import FakeUploader
+
+    fake = FakeUploader()
+    run_upload(cfg, uploader=fake, sleep=lambda s: None, now=lambda: "t")
+    assert [(j.action, j.bgg_id, j.name) for j in fake.calls] == [
+        ("add", "266192", "Wingspan")
+    ]

tests/test_extract.py

@@ -5,6 +5,7 @@ from __future__ import annotations
 
 import base64
 import io
+import json
 
 import pytest
 import typer
@@ -83,21 +84,21 @@ def test_parse_object_with_titles_and_unidentified():
  "unidentified": [{"location": "top shelf, left of Catan",
                    "partial_text": "WAR", "art_notes": "red spine"}]}
 ```"""
-    titles, unidentified = parse_vision_response(text)
+    titles, unidentified, _ = parse_vision_response(text)
     assert titles[0]["title_raw"] == "Catan"
     assert unidentified[0]["location"] == "top shelf, left of Catan"
 
 
 def test_parse_legacy_bare_array_still_works():
     text = '```json\n[{"title_raw": "Catan", "confidence": "high"}]\n```'
-    titles, unidentified = parse_vision_response(text)
+    titles, unidentified, _ = parse_vision_response(text)
     assert titles[0]["title_raw"] == "Catan"
     assert unidentified == []
 
 
 def test_parse_tolerates_prose_around_json():
     text = 'Here are the games:\n[{"title_raw": "Wingspan"}]\nLet me know!'
-    titles, _ = parse_vision_response(text)
+    titles, _, _ = parse_vision_response(text)
     assert titles[0]["title_raw"] == "Wingspan"
 
 
@@ -106,7 +107,7 @@ def test_parse_drops_malformed_entries():
         '{"titles": [{"title_raw": "Catan"}, {"no_title": true}, "just a string"],'
         ' "unidentified": [{}, "not a dict", {"location": "somewhere"}]}'
     )
-    titles, unidentified = parse_vision_response(text)
+    titles, unidentified, _ = parse_vision_response(text)
     assert len(titles) == 1
     assert len(unidentified) == 1  # empty {} and the bare string are dropped
 
@@ -227,7 +228,6 @@ def test_run_extract_empty_photos_dir_exits(tmp_path):
 
 # -- unidentified sightings ---------------------------------------------
 
-import json  # noqa: E402
 
 OBJECT_PAYLOAD = json.dumps(
     {

tests/test_models.py

@@ -1,3 +1,7 @@
+"""XML parser tests against hand-built API2 response shapes."""
+
+from __future__ import annotations
+
 import pytest
 
 from bggpipe.models import (

tests/test_normalize.py

@@ -1,3 +1,7 @@
+"""Title normalization must be symmetric and aggressive (spec)."""
+
+from __future__ import annotations
+
 from bggpipe.normalize import normalize_title
 
 

tests/test_resolve.py

@@ -14,17 +14,23 @@ from pathlib import Path
 import httpx
 import pytest
 
-from bggpipe.bgg_client import BGGClient
+from bggpipe.bgg_client import BGGClient, cache_key
 from bggpipe.config import Config
 from bggpipe.models import GameVersion
+from bggpipe.normalize import normalize_title
 from bggpipe.resolve import (
     Candidate,
+    MatchRow,
     TitleEntry,
     _dominant,
     _score_version,
+    _truncation_heads,
+    dedupe_matches,
     load_titles,
+    read_matches,
     resolve_entry,
     run_resolve,
+    write_matches,
 )
 
 FIXTURES = Path(__file__).parent / "fixtures" / "bgg_cache"
@@ -191,9 +197,6 @@ def test_score_version_no_overlap():
 
 # -- progressive title truncation (long transcribed box titles) ---------
 
-from bggpipe.normalize import normalize_title  # noqa: E402
-from bggpipe.resolve import _truncation_heads  # noqa: E402
-
 CIV_TITLE = (
     "CIVILIZATION Game of the Heroic Age - The Dawn of History 8000 BC to 250 BC"
 )
@@ -347,13 +350,15 @@ def test_run_resolve_saves_progress_when_token_missing(tmp_path):
 
 # -- post-resolve dedupe ------------------------------------------------
 
-from bggpipe.bgg_client import cache_key as _cache_key  # noqa: E402
-from bggpipe.resolve import dedupe_matches  # noqa: E402
-
 
 def _mrow(
-    title, bgg_id, photos, name="Joking Hazard",
-    vstatus="version_unknown", vid="", status="auto",
+    title,
+    bgg_id,
+    photos,
+    name="Joking Hazard",
+    vstatus="version_unknown",
+    vid="",
+    status="auto",
 ):
     return {
         "title_raw": title,
@@ -446,8 +451,6 @@ def test_dedupe_is_idempotent_and_skips_merged():
 
 
 def test_run_resolve_dedupes_and_keeps_all_rows(tmp_path):
-    from bggpipe.resolve import write_matches as _wm  # noqa: F401
-
     cache = tmp_path / "cache"
     cache.mkdir()
     wingspan_xml = (
@@ -456,7 +459,7 @@ def test_run_resolve_dedupes_and_keeps_all_rows(tmp_path):
         "</item></items>"
     )
     for query in ("Wingspan", "WINGSPAN!"):
-        key = _cache_key(
+        key = cache_key(
             "search", {"query": query, "type": "boardgame,boardgameexpansion"}
         )
         (cache / key).write_text(wingspan_xml)
@@ -482,3 +485,108 @@ def test_run_resolve_dedupes_and_keeps_all_rows(tmp_path):
     assert saved["Wingspan"]["match_status"] == "auto"
     assert saved["WINGSPAN!"]["match_status"] == "merged"
     assert saved["WINGSPAN!"]["merged_into"] == "Wingspan"
+
+
+# -- audit-fix regressions ----------------------------------------------
+
+
+def test_run_resolve_force_rebuilds_from_scratch(client, tmp_path):
+    data_dir = tmp_path / "data"
+    data_dir.mkdir()
+    shutil.copy(TITLES_JSON, data_dir / "titles.json")
+    cfg = Config(data_dir=data_dir)
+    run_resolve(cfg, client=client)
+
+    # poison one row: force must throw it away and re-resolve everything
+    rows = read_matches(cfg.matches_path)
+    rows[0]["match_status"] = "rejected"
+    write_matches(cfg.matches_path, rows)
+
+    forced = run_resolve(cfg, force=True, client=client)
+    assert len(forced) == 7  # every title re-resolved, none skipped
+    fresh = {r["title_raw"]: r for r in read_matches(cfg.matches_path)}
+    assert fresh["Catan"]["match_status"] == "auto"
+
+
+def test_new_photo_of_resolved_game_updates_row_instead_of_duplicating(
+    client, tmp_path
+):
+    data_dir = tmp_path / "data"
+    data_dir.mkdir()
+    shutil.copy(TITLES_JSON, data_dir / "titles.json")
+    cfg = Config(data_dir=data_dir)
+    run_resolve(cfg, client=client)
+    n_rows = len(read_matches(cfg.matches_path))
+
+    # extract sees Catan again on a reshoot photo: the entry's photo set
+    # grows, its (title, photos) key changes
+    titles = json.loads((data_dir / "titles.json").read_text())
+    for entry in titles:
+        if entry["title_raw"] == "Catan":
+            entry["source_photos"] = sorted([*entry["source_photos"], "reshoot.jpg"])
+    (data_dir / "titles.json").write_text(json.dumps(titles))
+
+    assert run_resolve(cfg, client=client) == []  # nothing re-resolved
+    rows = read_matches(cfg.matches_path)
+    assert len(rows) == n_rows  # and no duplicate row appended
+    catan = next(r for r in rows if r["title_raw"] == "Catan")
+    assert "reshoot.jpg" in catan["source_photos"]  # provenance followed
+
+
+def test_dedupe_never_overturns_a_human_veto():
+    a = _mrow("CATAN", "13", "p1.jpg")
+    b = _mrow("Catan", "13", "p2.jpg", status="approved")
+    b["dedupe_veto"] = "1"  # review said: genuinely two copies
+    events = dedupe_matches([a, b], [])
+    assert events == []
+    assert b["match_status"] == "approved"
+
+
+def test_publisher_pick_refuses_multiple_same_publisher_candidates():
+    from bggpipe.resolve import _publisher_pick
+
+    entry = TitleEntry(
+        title_raw="Sorcerer", title_normalized="sorcerer", publisher_hint="SPI"
+    )
+    cands = [
+        _cand(1, exact=True),
+        _cand(2, exact=True),
+    ]
+    for c in cands:
+        c.publishers = ["Simulations Publications, Inc. (SPI)"]
+    assert _publisher_pick(entry, cands) is None
+
+
+def test_publisher_pick_refuses_mixed_base_and_expansion():
+    from bggpipe.resolve import _publisher_pick
+
+    entry = TitleEntry(
+        title_raw="Wingspan", title_normalized="wingspan", publisher_hint="Stonemaier"
+    )
+    base = _cand(1, exact=True, type_="boardgame")
+    expansion = _cand(2, exact=True, type_="boardgameexpansion")
+    for c in (base, expansion):
+        c.publishers = ["Stonemaier Games"]
+    assert _publisher_pick(entry, [base, expansion]) is None
+
+
+def test_resolve_version_handles_unknown_id():
+    from bggpipe.resolve import resolve_version
+
+    class EmptyThings:
+        def things(self, ids, **kwargs):
+            return []
+
+    entry = TitleEntry(title_raw="X", title_normalized="x", publisher_hint="Someone")
+    row = MatchRow(title_raw="X", bgg_id=999999)
+    resolve_version(EmptyThings(), entry, row)  # must not raise
+    assert row.version_status == "version_unknown"
+
+
+def test_empty_normalized_title_never_matches(client):
+    # 风声 normalizes to "" — empty-vs-empty must not count as exact
+    from bggpipe.resolve import _plausible_candidates
+
+    entry = TitleEntry(title_raw="风声", title_normalized="")
+    # any cached query works; candidates must be rejected regardless of name
+    assert _plausible_candidates(client, entry, "Catan") == []

tests/test_review.py

@@ -9,12 +9,13 @@ import json
 from pathlib import Path
 
 import httpx
+import pytest
 from rich.console import Console
 
 from bggpipe.bgg_client import BGGClient
 from bggpipe.config import Config
 from bggpipe.resolve import read_matches, write_matches
-from bggpipe.review import run_review
+from bggpipe.review import ReviewSession, run_review
 
 FIXTURES = Path(__file__).parent / "fixtures" / "bgg_cache"
 
@@ -288,3 +289,96 @@ def test_version_pass_is_skippable(tmp_path):
     )
     (row,) = read_matches(cfg.matches_path)
     assert row["version_status"] == "version_ambiguous"  # untouched, review later
+
+
+# -- audit-fix regressions ----------------------------------------------
+
+
+def test_veto_merge_persists_against_future_dedupe(tmp_path):
+    from bggpipe.resolve import read_matches
+
+    cfg = _setup(
+        tmp_path,
+        [
+            _row(title_raw="CATAN", match_status="merged", merged_into="Catan"),
+            _row(title_raw="Catan", match_status="auto", bgg_id="13"),
+        ],
+    )
+    session = ReviewSession(
+        cfg,
+        console=quiet_console(),
+        input_fn=scripted(),
+        client=unauthorized_client(tmp_path),
+    )
+    merged = next(r for r in session.rows if r["match_status"] == "merged")
+    session.veto_merge(merged)
+    saved = {r["title_raw"]: r for r in read_matches(cfg.matches_path)}
+    assert saved["CATAN"]["match_status"] == "approved"
+    assert saved["CATAN"]["dedupe_veto"] == "1"  # survives resolve re-runs
+
+
+def test_failed_save_never_leaves_memory_ahead_of_disk(tmp_path, monkeypatch):
+    cfg = _setup(tmp_path, [_row(title_raw="Mystery", match_status="unmatched")])
+    session = ReviewSession(
+        cfg,
+        console=quiet_console(),
+        input_fn=scripted(),
+        client=unauthorized_client(tmp_path),
+    )
+    row = session.rows[0]
+
+    import bggpipe.review as review_mod
+
+    def exploding_write(path, rows):
+        raise OSError("disk full")
+
+    monkeypatch.setattr(review_mod, "write_matches", exploding_write)
+    with pytest.raises(OSError):
+        session.decide_reject(row)
+    # memory was rolled back to what disk actually holds
+    assert session.rows[0]["match_status"] == "unmatched"
+    assert session.decisions == 0
+
+
+def test_save_merges_own_decision_over_concurrent_external_rewrite(tmp_path):
+    from bggpipe.resolve import read_matches, write_matches
+
+    cfg = _setup(tmp_path, [_row(title_raw="Mystery", match_status="unmatched")])
+    session = ReviewSession(
+        cfg,
+        console=quiet_console(),
+        input_fn=scripted(),
+        client=unauthorized_client(tmp_path),
+    )
+    row = session.rows[0]
+
+    # resolve appends a new row in another terminal AFTER our session loaded
+    external = read_matches(cfg.matches_path)
+    external.append(_row(title_raw="Newcomer", match_status="auto", bgg_id="7"))
+    write_matches(cfg.matches_path, external)
+
+    session.decide_reject(row)
+    saved = {r["title_raw"]: r for r in read_matches(cfg.matches_path)}
+    assert saved["Mystery"]["match_status"] == "rejected"  # our decision
+    assert "Newcomer" in saved  # their row survived too
+
+
+def test_manual_id_unknown_to_bgg_warns_instead_of_crashing(tmp_path):
+    # an empty /thing result (mistyped id) used to crash the whole session
+    import httpx as _httpx
+
+    empty_things = BGGClient(
+        cache_dir=tmp_path / "cache",
+        transport=_httpx.MockTransport(
+            lambda req: _httpx.Response(200, text='<items total="0"></items>')
+        ),
+        sleep=lambda s: None,
+    )
+    cfg = _setup(tmp_path, [_row(title_raw="Mystery", match_status="unmatched")])
+    session = ReviewSession(
+        cfg, console=quiet_console(), input_fn=scripted(), client=empty_things
+    )
+    session.decide_manual(session.rows[0], 999999)
+    assert session.rows[0]["match_status"] == "approved"
+    assert session.rows[0]["bgg_id"] == "999999"
+    assert any("no game with id 999999" in w for w in session.warnings)

tests/test_upload.py

@@ -111,7 +111,7 @@ class FakeUploader:
 
 
 def test_build_queue_skips_logged_successes():
-    jobs, done, failed = build_queue(
+    jobs, done, failed, _ = build_queue(
         [_add_row(bgg_id="1"), _add_row(bgg_id="2", name="Catan")],
         [_update_row(collid="9")],
         [
@@ -127,7 +127,7 @@ def test_build_queue_skips_logged_successes():
 def test_build_queue_second_copy_is_a_distinct_job():
     # Same game, different version: a separate physical copy, so a
     # logged add of one version must not swallow the other.
-    jobs, done, _ = build_queue(
+    jobs, done, _, _ = build_queue(
         [
             _add_row(bgg_id="1", version_id="10", version_name="First ed."),
             _add_row(bgg_id="1", version_id="11", version_name="Second ed."),
@@ -141,9 +141,11 @@ def test_build_queue_second_copy_is_a_distinct_job():
 
 def test_build_queue_failures_need_retry_flag():
     log = [_log_row(action="add", bgg_id="1", status="failed")]
-    jobs, _, skipped = build_queue([_add_row(bgg_id="1")], [], log)
+    jobs, _, skipped, _ = build_queue([_add_row(bgg_id="1")], [], log)
     assert jobs == [] and skipped == 1
-    jobs, _, skipped = build_queue([_add_row(bgg_id="1")], [], log, retry_failed=True)
+    jobs, _, skipped, _ = build_queue(
+        [_add_row(bgg_id="1")], [], log, retry_failed=True
+    )
     assert len(jobs) == 1 and skipped == 0
 
 
@@ -153,7 +155,7 @@ def test_build_queue_latest_log_entry_wins():
         _log_row(action="add", bgg_id="1", status="failed"),
         _log_row(action="add", bgg_id="1", status="added"),
     ]
-    jobs, done, _ = build_queue([_add_row(bgg_id="1")], [], log, retry_failed=True)
+    jobs, done, _, _ = build_queue([_add_row(bgg_id="1")], [], log, retry_failed=True)
     assert jobs == [] and done == 1
 
 
@@ -300,3 +302,84 @@ def test_fresh_clone_marker_blocks_upload_without_cache_dir(tmp_path):
     _seed_data(tmp_path, to_add=[_add_row()])
     with pytest.raises(typer.Exit):
         run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=NOW)
+
+
+# -- audit-fix regressions ----------------------------------------------
+
+
+def test_login_error_aborts_without_poisoning_the_log(tmp_path):
+    from bggpipe.upload import LoginError
+
+    class BrokenLogin(FakeUploader):
+        def add_game(self, job):
+            raise LoginError("Cloudflare is challenging this browser")
+
+    cfg = _cfg(tmp_path)
+    _seed_data(tmp_path, to_add=[_add_row(bgg_id=str(i)) for i in range(1, 4)])
+    results = run_upload(cfg, uploader=BrokenLogin(), sleep=lambda s: None, now=NOW)
+    assert results == []  # nothing logged: next run retries everything
+    assert not (tmp_path / "upload_log.csv").exists()
+
+
+def test_three_identical_failures_abort_as_systemic(tmp_path):
+    cfg = _cfg(tmp_path)
+    _seed_data(tmp_path, to_add=[_add_row(bgg_id=str(i)) for i in range(1, 6)])
+    fake = FakeUploader(failures={"Wingspan"})  # every job shares the name
+    results = run_upload(cfg, uploader=fake, sleep=lambda s: None, now=NOW)
+    assert len(results) == 3  # aborted after the third identical failure
+    logged = list(csv.DictReader((tmp_path / "upload_log.csv").open()))
+    assert len(logged) == 3  # jobs 4-5 left unlogged and retryable
+
+
+def test_added_no_version_is_done_and_verify_tolerates_it(tmp_path):
+    class NoVersionPicker(FakeUploader):
+        def add_game(self, job):
+            self.calls.append(job)
+            return "added_no_version", "version not in picker; added without version"
+
+    cfg = _cfg(tmp_path)
+    _seed_data(tmp_path, to_add=[_add_row(version_id="99", version_name="4th ed.")])
+    run_upload(cfg, uploader=NoVersionPicker(), sleep=lambda s: None, now=NOW)
+    # done: re-running must NOT re-add (a duplicate collection entry)
+    again = FakeUploader()
+    assert run_upload(cfg, uploader=again, sleep=lambda s: None, now=NOW) == []
+    assert again.calls == []
+    # verify: game present without the version is the EXPECTED outcome
+    log = list(csv.DictReader((tmp_path / "upload_log.csv").open()))
+    assert verify_uploads(log, [_item(1, 10)]) == []
+
+
+def test_missing_to_add_csv_is_a_loud_precondition_failure(tmp_path):
+    cfg = _cfg(tmp_path)  # no diff outputs seeded at all
+    with pytest.raises(typer.Exit):
+        run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=NOW)
+
+
+def test_second_update_for_same_game_is_deferred(tmp_path):
+    # the row-edit flow can't target a collid, so only one update per game
+    # per run is safe
+    cfg = _cfg(tmp_path)
+    _seed_data(
+        tmp_path,
+        to_update=[
+            _update_row(collid="9", bgg_id="2"),
+            _update_row(collid="10", bgg_id="2", vid="26", vname="2nd ed."),
+        ],
+    )
+    fake = FakeUploader()
+    results = run_upload(cfg, uploader=fake, sleep=lambda s: None, now=NOW)
+    assert [r["collid"] for r in results] == ["9"]
+    # after the first lands, the next run picks up the deferred one
+    again = FakeUploader()
+    results = run_upload(cfg, uploader=again, sleep=lambda s: None, now=NOW)
+    assert [j.collid for j in again.calls] == ["10"]
+
+
+def test_real_run_without_credentials_exits_before_any_browser(tmp_path, monkeypatch):
+    monkeypatch.delenv("BGG_USERNAME", raising=False)
+    monkeypatch.delenv("BGG_PASSWORD", raising=False)
+    cfg = _cfg(tmp_path)
+    _seed_data(tmp_path, to_add=[_add_row()])
+    with pytest.raises(typer.Exit):
+        run_upload(cfg, sleep=lambda s: None, now=NOW)  # uploader=None: real path
+    assert not (tmp_path / "upload_log.csv").exists()

tests/test_webreview.py

@@ -383,3 +383,23 @@ def test_own_saves_do_not_count_as_external_changes(tmp_path):
     write_matches(cfg.matches_path, session.rows + [_row(title_raw="X")])
     assert session.reload_if_changed() is True  # someone else's
     assert any(r["title_raw"] == "X" for r in session.rows)
+
+
+def test_session_warnings_surface_in_state(tmp_path):
+    web, cfg = make_client(tmp_path)
+    # a manual id triggers a lookup against the 401-ing client: the session
+    # degrades and the warning must reach the payload (the session console
+    # is a StringIO here — this is the only way the user ever sees it)
+    res = web.post(
+        "/api/decision",
+        json={
+            "title_raw": "Mystery",
+            "source_photos": "shelf.jpg",
+            "action": "manual",
+            "bgg_id": 42,
+        },
+    )
+    assert res.status_code == 200
+    warnings = res.json()["warnings"]
+    assert any("couldn't look up id 42" in w for w in warnings)
+    assert warnings == web.get("/api/state").json()["warnings"]