Audit round 7, upload cluster: evidence over inference at every exit

Five blind reviewers swept the real-data-era surface; this lands the upload findings, all verified against the code and the documented site behavior before fixing. The two HIGHs shared a root: logging outcomes the browser never proved. add_game waited for an "Add To" button that an owned game's page does not have — so a second-copy add could never succeed, and worse, an add that LANDED but missed the log became an unretryable failure loop (every retry: 30s timeout, logged failed, nothing ever settles). add_game now polls for either button state: "In Collections" without second_copy returns the previously-dead already_present status (the landed-but-unlogged case heals itself on retry); with second_copy it refuses loudly (that flow is unverified — add by hand). A save whose dialog is slow to hide reloads the page and asks for ownership evidence instead of guessing "failed". update_entry no longer trusts the editor merely closing: the cell must settle on text matching the CHOSEN version, else the AJAX save failed server-side and "updated" would mark a job done forever that never touched the site. Per-copy bookkeeping: stale_jobs endorsed per game, so rejecting one of two queued editions let the rejected copy upload on the survivor's endorsement — it now counts endorsements per (bgg_id, version) and retires the game with "re-run diff" when a copy loses its backing. annotate_queue stamped every row sharing a job key with the same log status, so one success marked both vetoed duplicates done; completions are now claimed one row per done log line. Smaller findings: the version-drift note queued a doomed re-add after warning about it (now skips — the entry exists on BGG; re-adding only duplicates); the one-update-per-game deferral rested on a claim the collid-exact editor disproves (removed — same-game updates run together); the 3-identical-failures abort compared exception class only, so three unrelated problems aborted a healthy run (now compares whole messages). Also from the test seat: run_upload's stale filtering finally executes against a real matches.csv in tests; rejected credentials pin that no anonymous storage state is saved; update_entry's three guarded exits each have a test; _scrub's newline flattening is pinned. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016jXZFSTZQKzAC8fqpWSz9g
2026-08-06 00:21:52 -04:00
parent 22fa17b5ee
commit 32b6aae841
8 changed files with 803 additions and 174 deletions
@@ -18,7 +18,9 @@ from bggpipe.upload import (
    LoginError,
    UploadJob,
    _scrub,
+    annotate_queue,
    build_queue,
+    stale_jobs,
    run_upload,
    verify_uploads,
 )
@@ -356,9 +358,9 @@ def test_missing_to_add_csv_is_a_loud_precondition_failure(tmp_path):
        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
+def test_same_game_updates_run_together(tmp_path):
+    # update_entry addresses the copy by collid and the edition by radio
+    # value, so two copies of one game are safe in a single run
    cfg = _cfg(tmp_path)
    _seed_data(
        tmp_path,
@@ -369,11 +371,7 @@ def test_second_update_for_same_game_is_deferred(tmp_path):
    )
    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"]
+    assert [r["collid"] for r in results] == ["9", "10"]


 def test_real_run_without_credentials_exits_before_any_browser(tmp_path, monkeypatch):
@@ -861,3 +859,368 @@ def test_update_targets_collid_and_version_id_exactly():
    assert "objectid=240" in seen["url"]
    assert any('onclick*="53429559"' in s for s in seen["clicked"])  # the copy
    assert any('value="24621"' in s for s in seen["clicked"])  # the edition
+
+
+def test_run_upload_skips_jobs_the_review_retired(tmp_path, capsys):
+    """The queue is a snapshot from the last diff; a review decision taken
+    afterwards (local, rejected, re-matched) outranks it. This must hold
+    through run_upload's actual wiring, not just stale_jobs in isolation —
+    a filter that pruned only to_add would still pass the unit test while
+    uploading a retired version update."""
+    from bggpipe.resolve import MATCH_COLUMNS
+
+    cfg = _cfg(tmp_path)
+    _seed_data(
+        tmp_path,
+        to_add=[_add_row(bgg_id="140509", name="Dungeons & Dragons")],
+        to_update=[_update_row(collid="9", bgg_id="240", name="Britannia")],
+    )
+    blank = dict.fromkeys(MATCH_COLUMNS, "")
+    _write_csv(
+        tmp_path / "matches.csv",
+        MATCH_COLUMNS,
+        [
+            # 140509 was re-reviewed: it's the RPG blue box, kept local
+            {**blank, "title_raw": "D&D", "match_status": "local", "bgg_id": ""},
+            # 240 vanished from matches entirely (removed from the catalog)
+            {**blank, "title_raw": "WINGSPAN", "match_status": "auto", "bgg_id": "1"},
+        ],
+    )
+    fake = FakeUploader()
+    run_upload(cfg, uploader=fake, sleep=lambda s: None, now=_now)
+    assert fake.calls == []
+    out = capsys.readouterr().out
+    assert "Skipping 2" in out
+    # retired jobs leave no trace to be "already done" later
+    assert not (tmp_path / "upload_log.csv").exists()
+
+    # control: an endorsed job still runs
+    _write_csv(
+        tmp_path / "matches.csv",
+        MATCH_COLUMNS,
+        [
+            {**blank, "title_raw": "X", "match_status": "auto", "bgg_id": "140509"},
+            {
+                **blank,
+                "title_raw": "Y",
+                "match_status": "approved",
+                "bgg_id": "240",
+                "version_id": "25",  # per-copy endorsement: version must agree
+            },
+        ],
+    )
+    run_upload(cfg, uploader=fake, sleep=lambda s: None, now=_now)
+    assert [(j.action, j.bgg_id) for j in fake.calls] == [
+        ("add", "140509"),
+        ("update", "240"),
+    ]
+
+
+def test_rejected_credentials_raise_and_persist_no_session(tmp_path, monkeypatch):
+    """If the site still offers Sign In after the form was submitted, the
+    login FAILED — treating it as success would mark the run authed and
+    persist an anonymous storage state that poisons every later run."""
+    from bggpipe.upload import LoginError, PlaywrightUploader
+
+    monkeypatch.setenv("BGG_USERNAME", "someone")
+    monkeypatch.setenv("BGG_PASSWORD", "wrong-pw")
+    state_calls = []
+
+    class _Loc:
+        def __init__(self, n=1):
+            self._n = n
+
+        @property
+        def first(self):
+            return self
+
+        def count(self):
+            return self._n
+
+        def is_visible(self):
+            return True
+
+        def wait_for(self, **kw):
+            return None
+
+        def fill(self, value):
+            pass
+
+        def click(self):
+            pass
+
+    class _Page:
+        title = staticmethod(lambda: "BoardGameGeek")
+
+        def goto(self, url, **kw):
+            pass
+
+        def locator(self, selector):
+            return _Loc()
+
+        def get_by_role(self, role, **kw):
+            return _Loc()
+
+        def wait_for_url(self, pred, **kw):
+            return None  # the URL left /login (site redirects even on failure)
+
+        def wait_for_timeout(self, ms):
+            pass
+
+    class _Context:
+        def storage_state(self, path):
+            state_calls.append(path)
+
+    up = PlaywrightUploader("someone", storage_state=tmp_path / "state.json")
+    up._page = _Page()
+    up._context = _Context()
+    # signed-out before AND after submitting the form: Sign In stays visible
+    monkeypatch.setattr(up, "_signed_out", lambda: True)
+    with pytest.raises(LoginError, match="credentials rejected"):
+        up._ensure_logged_in()
+    assert up._authed is False
+    assert state_calls == []  # no anonymous session saved for reuse
+
+
+def test_update_entry_failure_paths_leave_the_entry_untouched():
+    """The three guarded exits of the collection-cell editor: no cell for
+    the collid, the wanted version id never offered (must press Escape —
+    the editor is open on a REAL entry), and a save that never settles."""
+    from bggpipe.upload import PlaywrightUploader, UploadJob
+
+    class _Timeout(Exception):
+        pass
+
+    def make_page(cell_count=1, radio_appears=True, settles=True):
+        pressed = []
+
+        class _Loc:
+            def __init__(self, selector):
+                self.selector = selector
+
+            @property
+            def first(self):
+                return self
+
+            def count(self):
+                return cell_count if "collection_version" in self.selector else 1
+
+            def wait_for(self, **kw):
+                if "radio" in self.selector and not radio_appears:
+                    raise _Timeout("radio never appeared")
+
+            def click(self):
+                pass
+
+            def text_content(self):
+                return "Editing" if not settles else "Avalon Hill second"
+
+        class _Keyboard:
+            def press(self, key):
+                pressed.append(key)
+
+        class _Page:
+            keyboard = _Keyboard()
+
+            def goto(self, url, **kw):
+                pass
+
+            def locator(self, selector):
+                return _Loc(selector)
+
+            def wait_for_timeout(self, ms):
+                pass
+
+        return _Page(), pressed
+
+    def uploader(page):
+        up = PlaywrightUploader("someone")
+        up._page = page
+        up._authed = True
+        up._timeout_error = _Timeout
+        return up
+
+    job = UploadJob(
+        action="update",
+        bgg_id=240,
+        name="Britannia",
+        collid="53429559",
+        version_id="24621",
+        version_name="AH",
+    )
+
+    page, _ = make_page(cell_count=0)
+    with pytest.raises(RuntimeError, match="re-run diff"):
+        uploader(page).update_entry(job)
+
+    page, pressed = make_page(radio_appears=False)
+    with pytest.raises(RuntimeError, match="entry left untouched"):
+        uploader(page).update_entry(job)
+    assert pressed == ["Escape"]  # the open editor was closed, not abandoned
+
+    page, _ = make_page(settles=False)
+    with pytest.raises(RuntimeError, match="never left its editing state"):
+        uploader(page).update_entry(job)
+
+
+def test_scrub_flattens_multiline_errors_for_the_csv_log():
+    text = "Call log:\n  - waiting for locator\n  - retrying click"
+    assert "\n" not in _scrub(text)
+    assert _scrub(text) == "Call log: - waiting for locator - retrying click"
+
+
+def test_stale_jobs_retires_the_copy_not_just_the_game():
+    """Rejecting ONE of two queued editions must not ride along on the
+    surviving edition's endorsement."""
+    queue = [
+        {"bgg_id": "589", "version_id": "100"},
+        {"bgg_id": "589", "version_id": "200"},
+    ]
+    matches = [
+        {"bgg_id": "589", "match_status": "approved", "version_id": "100"},
+        {"bgg_id": "589", "match_status": "rejected", "version_id": "200"},
+    ]
+    stale = stale_jobs(queue, matches)
+    assert "589" in stale and "no longer endorses that copy" in stale["589"]
+    # both endorsed: nothing stale
+    matches[1]["match_status"] = "approved"
+    assert stale_jobs(queue, matches) == {}
+
+
+def test_annotate_queue_marks_one_done_per_completion():
+    """Vetoed duplicate copies share a job key; one success is one copy."""
+    queue = [
+        {"bgg_id": "13", "version_id": ""},
+        {"bgg_id": "13", "version_id": ""},
+    ]
+    log = [_log_row(bgg_id="13", status="added")]
+    states = [r["state"] for r in annotate_queue(queue, "add", log)]
+    assert states == ["done", ""]
+    log.append(_log_row(bgg_id="13", status="added"))
+    states = [r["state"] for r in annotate_queue(queue, "add", log)]
+    assert states == ["done", "done"]
+
+
+def test_add_game_reports_already_present_on_owned_page():
+    """A game page showing "In Collections" instead of "Add To" is positive
+    evidence a previous attempt landed — retrying the add would duplicate."""
+    from bggpipe.upload import PlaywrightUploader, UploadJob
+
+    class _Btn:
+        def __init__(self, present):
+            self._present = present
+
+        @property
+        def first(self):
+            return self
+
+        def count(self):
+            return 1 if self._present else 0
+
+        def is_visible(self):
+            return self._present
+
+    class _Page:
+        def goto(self, url, **kw):
+            pass
+
+        def get_by_role(self, role, name=None, **kw):
+            return _Btn(name == "In Collections")
+
+        def wait_for_timeout(self, ms):
+            pass
+
+    up = PlaywrightUploader("someone")
+    up._page = _Page()
+    up._authed = True
+    job = UploadJob(action="add", bgg_id="240", name="Britannia")
+    status, note = up.add_game(job)
+    assert status == "already_present"
+    assert "previous attempt" in note
+
+    # a SECOND copy can't go through the unverified In Collections flow
+    job2 = UploadJob(action="add", bgg_id="240", name="Britannia", second_copy=True)
+    with pytest.raises(RuntimeError, match="add this copy by hand"):
+        up.add_game(job2)
+
+
+def test_update_entry_rejects_a_settle_on_the_wrong_version():
+    """The editor closing proves nothing: if the cell re-renders its OLD
+    content the AJAX save failed and "updated" would be a false record."""
+    from bggpipe.upload import PlaywrightUploader, UploadJob
+
+    class _Loc:
+        def __init__(self, selector):
+            self.selector = selector
+
+        @property
+        def first(self):
+            return self
+
+        def count(self):
+            return 1
+
+        def wait_for(self, **kw):
+            pass
+
+        def click(self):
+            pass
+
+        def text_content(self):
+            return "Some Other Edition (2001)"  # the OLD content, not ours
+
+    class _Page:
+        def goto(self, url, **kw):
+            pass
+
+        def locator(self, selector):
+            return _Loc(selector)
+
+        def wait_for_timeout(self, ms):
+            pass
+
+    up = PlaywrightUploader("someone")
+    up._page = _Page()
+    up._authed = True
+    job = UploadJob(
+        action="update",
+        bgg_id="240",
+        name="Britannia",
+        collid="9",
+        version_id="24621",
+        version_name="Avalon Hill second edition",
+    )
+    with pytest.raises(RuntimeError, match="did not land"):
+        up.update_entry(job)
+
+
+def test_version_drift_skips_the_doomed_readd(tmp_path, capsys):
+    """A game the log shows added under a different version is already on
+    BGG — re-adding can only duplicate it, so the queue skips it."""
+    cfg = _cfg(tmp_path)
+    _seed_data(
+        tmp_path,
+        to_add=[_add_row(bgg_id="1", name="Wingspan", version_id="99")],
+        log=[_log_row(bgg_id="1", version_id="55", status="added")],
+    )
+    fake = FakeUploader()
+    run_upload(cfg, uploader=fake, sleep=lambda s: None, now=_now)
+    assert fake.calls == []
+    assert "correct the version by hand" in capsys.readouterr().out
+
+
+def test_abort_needs_identical_messages_not_just_same_class(tmp_path):
+    """Three DIFFERENT per-game failures are three unlucky games, not a
+    systemic outage; only identical messages abort the run."""
+    cfg = _cfg(tmp_path)
+    rows = [_add_row(bgg_id=str(i), name=f"Game {i}") for i in (1, 2, 3, 4)]
+    _seed_data(tmp_path, to_add=rows)
+
+    class _VariedFail(FakeUploader):
+        def add_game(self, job):
+            self.calls.append(job)
+            raise RuntimeError(f"distinct problem with {job.name}")
+
+    varied = _VariedFail()
+    run_upload(cfg, uploader=varied, sleep=lambda s: None, now=_now)
+    assert len(varied.calls) == 4  # no abort: every message differed