Re-audit round 2: 5 blind reviewers, 17 fixes, +12 tests

The re-run confirmed round 1 held and then caught second-order bugs in
its own fixes plus two long-standing ones everyone missed. TUI decisions
after a mid-session reload were counted but never written (rows are now
re-adopted into the fresh list on every save, preferring undecided slots
on duplicate keys); row_ix was computed by equality so duplicate rows
shared an ordinal (identity now, merges included, veto sends it); upload
job keys collided for two same-version copies (completions are counted
per key, so --limit or an interrupt can no longer strand the second
copy); diff consumes collids on exact-version matches (a vetoed
same-version second copy was silently swallowed) and splits mismatches:
report-only disagreement while an unclaimed copy exists, second-copy add
only when every copy is claimed.

Also: XML responses are validated and written atomically before caching
(a torn or truncated 200 body can never poison a re-run), JSON artifacts
write atomically, thing/search parsers refuse missing ids like the
collection parser, empty game names are refused by the upload queue, a
never-rendering version picker fails retryably instead of terminally,
the systemic-failure abort compares exception types, blocked same-title
entries defer as a group so positional pairing can't misalign,
truncation heads pick the earliest separator, diff messages tell the
truth when a token exists without a username, and the shared-constant
sweep now actually covers every module (statuses, search types, marker
names, client_for, ports). pydantic declared as a direct dependency.

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

tests/test_client.py

@@ -1,3 +1,5 @@
+"""BGG client tests: canned transports, fake clocks — never online."""
+
 from __future__ import annotations
 
 import random
@@ -182,3 +184,13 @@ def test_collection_item_missing_collid_refuses_to_parse(tmp_path):
     )
     with pytest.raises(BGGResponseError):
         parse_collection(bad_xml)
+
+
+def test_malformed_xml_raises_and_is_never_cached(tmp_path):
+    from bggpipe.models import BGGResponseError
+
+    torn = '<items total="1"><item type="boardgame" id="13"><na'
+    client, _, _ = make_client(tmp_path, [(200, torn)])
+    with pytest.raises(BGGResponseError):
+        client.get_xml("search", {"query": "catan", "type": "boardgame"})
+    assert list((tmp_path / "cache").glob("*.xml")) == []

tests/test_diff.py

@@ -10,6 +10,10 @@ from bggpipe.diff import 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):
@@ -115,10 +119,10 @@ def test_owned_with_matching_version_is_just_owned():
     assert not result.to_update and not result.to_add
 
 
-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.
+def test_version_mismatch_with_unclaimed_copy_is_report_only():
+    # ONE row, ONE copy with a different version: most likely the same
+    # physical box mis-scored. Spec: report the disagreement, touch nothing,
+    # and never risk uploading a duplicate entry.
     result = compute_diff(
         [
             _match(
@@ -131,10 +135,47 @@ def test_confident_version_matching_no_copy_is_a_second_copy_to_add():
         ],
         [_item(266192, 5, version_id=465063)],
     )
-    assert not result.to_update  # additive only: never edit a set version
-    assert [r["version_id"] for r in result.to_add] == ["521212"]
-    assert "fourth printing" in result.second_copies[0]
-    assert result.already_owned == []
+    assert not result.to_update and not result.to_add
+    assert result.already_owned == ["Wingspan"]
+    assert "fourth printing" in result.disagreements[0]
+
+
+def test_vetoed_duplicate_of_same_version_is_a_real_second_copy():
+    # Two rows, same confident version, ONE owned copy with that version:
+    # a human vetoed the merge ("these ARE two boxes"), so the exact-version
+    # match must consume the copy and the second row must become an add.
+    rows = [
+        _match("Catan", "13", vstatus="version_auto", vid="123", vname="3rd ed."),
+        _match("Catan", "13", vstatus="version_auto", vid="123", vname="3rd ed."),
+    ]
+    result = compute_diff(rows, [_item(13, 900, version_id=123)])
+    assert result.already_owned == ["Catan"]
+    assert [r["version_id"] for r in result.to_add] == ["123"]
+    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)])
+    assert result.already_owned == ["Catan"]
+    (added,) = result.to_add
+    assert added["version_id"] == ""
+    assert len(result.second_copies) == 1
+
+
+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
+    rows = [
+        _match("Catan", "13"),
+        _match("Catan", "13", vstatus="version_auto", vid="55", vname="5th ed."),
+    ]
+    result = compute_diff(rows, [_item(13, 900), _item(13, 901)])
+    assert [u["version_id"] for u in result.to_update] == ["55"]
+    assert result.to_add == []
+    assert result.already_owned.count("Catan") == 2
 
 
 def test_version_unknown_owned_by_bare_id():
@@ -237,3 +278,22 @@ def test_run_diff_outputs_feed_upload_unchanged(tmp_path, monkeypatch):
     assert [(j.action, j.bgg_id, j.name) for j in fake.calls] == [
         ("add", "266192", "Wingspan")
     ]
+
+
+def test_token_without_username_says_so(tmp_path, monkeypatch, capsys):
+    import shutil
+
+    from bggpipe.diff import run_diff
+    from bggpipe.resolve import write_matches
+
+    monkeypatch.setenv("BGG_API_TOKEN", "tok")
+    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:
+        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

tests/test_resolve.py

@@ -590,3 +590,44 @@ def test_empty_normalized_title_never_matches(client):
     entry = TitleEntry(title_raw="风声", title_normalized="")
     # any cached query works; candidates must be rejected regardless of name
     assert _plausible_candidates(client, entry, "Catan") == []
+
+
+def test_blocked_same_title_entry_defers_the_whole_group(tmp_path):
+    # entry1 of a two-edition title is blocked (no token); entry2 must NOT
+    # resolve, or its row would occupy entry1's pairing slot next run
+    import httpx as _httpx
+
+    data_dir = tmp_path / "data"
+    data_dir.mkdir()
+    (data_dir / "titles.json").write_text(
+        json.dumps(
+            [
+                {
+                    "title_raw": "Catan",
+                    "edition_hint": "3rd edition",
+                    "source_photos": ["a.jpg"],
+                },
+                {
+                    "title_raw": "Catan",
+                    "edition_hint": "5th edition",
+                    "source_photos": ["b.jpg"],
+                },
+            ]
+        )
+    )
+    cfg = Config(data_dir=data_dir)
+    blocked_client = BGGClient(
+        cache_dir=tmp_path / "empty_cache",
+        transport=_httpx.MockTransport(
+            lambda req: _httpx.Response(401, text="Unauthorized")
+        ),
+        sleep=lambda s: None,
+    )
+    run_resolve(cfg, client=blocked_client)
+    assert read_matches(cfg.matches_path) == []  # both deferred, none misplaced
+
+
+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

tests/test_review.py

@@ -382,3 +382,40 @@ def test_manual_id_unknown_to_bgg_warns_instead_of_crashing(tmp_path):
     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)
+
+
+def test_every_tui_decision_after_external_rewrite_is_saved(tmp_path):
+    # THE round-2 catch: the TUI iterates row references snapshotted before
+    # any reload; after decision 1 triggers a reload, decisions 2..N used
+    # to be counted but never written.
+    from bggpipe.resolve import read_matches, write_matches
+
+    cfg = _setup(
+        tmp_path,
+        [
+            _row(title_raw="Alpha", match_status="unmatched"),
+            _row(title_raw="Beta", match_status="unmatched"),
+            _row(title_raw="Gamma", match_status="unmatched"),
+        ],
+    )
+    session = ReviewSession(
+        cfg,
+        console=quiet_console(),
+        input_fn=scripted(),
+        client=unauthorized_client(tmp_path),
+    )
+    stale_refs = list(session.pending_rows())  # what run() iterates
+
+    external = read_matches(cfg.matches_path)
+    external.append(_row(title_raw="Newcomer", match_status="auto", bgg_id="7"))
+    write_matches(cfg.matches_path, external)
+
+    for ref in stale_refs:  # decision 1 reloads; 2 and 3 are orphaned refs
+        session.decide_reject(ref)
+
+    saved = {r["title_raw"]: r for r in read_matches(cfg.matches_path)}
+    assert [saved[t]["match_status"] for t in ("Alpha", "Beta", "Gamma")] == (
+        ["rejected"] * 3
+    )
+    assert "Newcomer" in saved  # the external row survived too
+    assert session.decisions == 3

tests/test_upload.py

@@ -383,3 +383,45 @@ def test_real_run_without_credentials_exits_before_any_browser(tmp_path, monkeyp
     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()
+
+
+def test_same_key_second_copy_survives_limit_and_interrupts(tmp_path):
+    # two vetoed duplicate copies share ("add", bgg_id, version): one logged
+    # success must complete exactly ONE of them, not both
+    cfg = _cfg(tmp_path)
+    twin = _add_row(bgg_id="13", name="Catan", version_id="123", version_name="3rd")
+    _seed_data(tmp_path, to_add=[dict(twin), dict(twin)])
+
+    first = FakeUploader()
+    run_upload(cfg, uploader=first, limit=1, sleep=lambda s: None, now=NOW)
+    assert len(first.calls) == 1
+
+    second = FakeUploader()
+    run_upload(cfg, uploader=second, sleep=lambda s: None, now=NOW)
+    assert len(second.calls) == 1  # the second copy, not zero, not two
+
+    third = FakeUploader()
+    assert run_upload(cfg, uploader=third, sleep=lambda s: None, now=NOW) == []
+
+
+def test_consecutive_failure_counter_resets_on_success(tmp_path):
+    class FlakyPairs(FakeUploader):
+        def add_game(self, job):
+            self.calls.append(job)
+            if job.bgg_id in ("1", "2", "4", "5"):
+                raise RuntimeError("dialog never appeared")
+            return "added", ""
+
+    cfg = _cfg(tmp_path)
+    _seed_data(tmp_path, to_add=[_add_row(bgg_id=str(i)) for i in range(1, 7)])
+    fake = FlakyPairs()
+    run_upload(cfg, uploader=fake, sleep=lambda s: None, now=NOW)
+    assert len(fake.calls) == 6  # fail,fail,ok,fail,fail,ok — never aborts
+
+
+def test_empty_game_name_is_refused_not_uploaded(tmp_path):
+    cfg = _cfg(tmp_path)
+    _seed_data(tmp_path, to_add=[_add_row(bgg_id="42", name="")])
+    fake = FakeUploader()
+    results = run_upload(cfg, uploader=fake, sleep=lambda s: None, now=NOW)
+    assert results == [] and fake.calls == []

tests/test_webreview.py

@@ -403,3 +403,32 @@ def test_session_warnings_surface_in_state(tmp_path):
     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"]
+
+
+def test_duplicate_rows_are_individually_decidable_via_row_ix(tmp_path):
+    # two editions of one game in one photo: byte-identical rows. The
+    # ordinal must land each decision on its own row.
+    from bggpipe.resolve import read_matches as read_m
+    from bggpipe.resolve import write_matches as write_m
+
+    cfg = make_cfg(tmp_path)
+    dup = _row(title_raw="Twins", match_status="unmatched")
+    write_m(cfg.matches_path, [dict(dup), dict(dup)])
+    web = TestClient(create_app(cfg, client=unauthorized_client(tmp_path)))
+
+    pending = web.get("/api/state").json()["pending"]
+    assert [p["title_raw"] for p in pending] == ["Twins", "Twins"]
+    assert pending[0]["row_ix"] != pending[1]["row_ix"]  # identity, not ==
+
+    second = pending[1]
+    web.post(
+        "/api/decision",
+        json={
+            "title_raw": second["title_raw"],
+            "source_photos": second["source_photos"],
+            "row_ix": second["row_ix"],
+            "action": "reject",
+        },
+    )
+    rows = read_m(cfg.matches_path)
+    assert [r["match_status"] for r in rows] == ["unmatched", "rejected"]