diff --git a/README.md b/README.md index cd630cd..e31ff55 100644 --- a/README.md +++ b/README.md @@ -23,8 +23,8 @@ BGG has no bulk import and no write API. Cataloging a few hundred games by hand 2. **resolve** — Titles are matched to BGG game IDs via the [XML API2](https://boardgamegeek.com/wiki/page/BGG_XML_API2) (exact + fuzzy matching, popularity tiebreaks), then edition cues are matched against BGG's version list for each game. Anything uncertain is flagged rather than guessed. 3. **review** — A local review step for ambiguous matches: pick the right game/version, or leave the version blank. Wrong guesses never reach your collection. 4. **diff** — Your existing BGG collection is fetched and compared, per copy (owning one edition of a game doesn't hide a second edition you also own). -5. **upload** — A Playwright browser session logs into your BGG account and adds each game — with its version, when known — politely and slowly, with dry-run mode, per-game logging, and resumability. -6. **enrich** — Full metadata for every game (designers, player counts, weight, rank, mechanics, artwork URLs, version details) lands in `data/games.json`, ready to power whatever you build next. +5. **upload** — A Playwright browser session logs into your BGG account and adds each game (with its version, when known) politely and slowly. Dry-run mode, per-game logging, and resumability included. +6. **enrich** — Full metadata for every game (designers, player counts, weight, rank, mechanics, artwork URLs, version details) lands in `data/games.json`, the seed data for a future web frontend. Every stage is idempotent and resumable: kill it mid-run, restart, lose nothing. All artifacts are flat CSV/JSON files you can inspect and edit. @@ -32,7 +32,7 @@ Every stage is idempotent and resumable: kill it mid-run, restart, lose nothing. - macOS or Linux, Python 3.12+, [uv](https://docs.astral.sh/uv/) - An [Anthropic API key](https://console.anthropic.com/) (vision extraction) -- A BoardGameGeek account **and a registered BGG application** — as of BGG's [2025 API policy](https://boardgamegeek.com/using_the_xml_api), the XML API requires a Bearer token from a registered app. Register a free non-commercial application at [boardgamegeek.com/applications](https://boardgamegeek.com/applications) (approval can take a week or more — **apply on day one**), then create a token. Each user needs their own — tokens must not be shared. +- A BoardGameGeek account **and a registered BGG application** — as of BGG's [2025 API policy](https://boardgamegeek.com/using_the_xml_api), the XML API requires a Bearer token from a registered app. Register a free non-commercial application at [boardgamegeek.com/applications](https://boardgamegeek.com/applications) (approval can take a week or more, so **apply on day one**), then create a token. Each user needs their own; tokens must not be shared. ## Quick start @@ -44,7 +44,7 @@ uv run playwright install chromium # browser for the upload stage cp .env.example .env # then fill in your keys ``` -Secrets live in environment variables only — never in config files, code, or logs. `.env` is gitignored. If you use [direnv](https://direnv.net/), the committed `.envrc` loads `.env` automatically after a one-time `direnv allow`; otherwise export the variables yourself (e.g. `set -a; source .env; set +a`). +Secrets live in environment variables only, never in config files, code, or logs. `.env` is gitignored. If you use [direnv](https://direnv.net/), the committed `.envrc` loads `.env` automatically after a one-time `direnv allow`; otherwise export the variables yourself (e.g. `set -a; source .env; set +a`). | Variable | Used by | What it is | |---|---|---| @@ -69,7 +69,7 @@ Each stage skips work it has already done; `--force`/`--refresh` flags redo it. ### Taking good shelf photos -Straight-on, one shelf (or part of one) per shot, close enough that spine text is legible to a human — if you can't read it, the model can't either. Overlap between shots is fine: duplicate reads are deduped automatically, with the merge shown (and veto-able) in review. Boxes the model spots but can't identify become retake prompts in `unidentified.json` and the review UI's "reshoot" list: photograph those boxes up close, drop the new photo in `photos/`, and run `extract` again. +Straight-on, one shelf (or part of one) per shot, close enough that spine text is legible to a human. If you can't read it, the model can't either. Overlap between shots is fine: duplicate reads are deduped automatically, with the merge shown (and veto-able) in review. Boxes the model spots but can't identify become retake prompts in `unidentified.json` and the review UI's "reshoot" list: photograph those boxes up close, drop the new photo in `photos/`, and run `extract` again. ## Bring your own shelves @@ -82,7 +82,7 @@ rm -rf data/bgg_cache data/extract_raw Two of those files deserve a word: -- **`data/STUB_DATA.marker`** — the committed CSVs were resolved from *hand-written stub fixtures* (the author's BGG application is still awaiting approval), so every version id in them is a synthetic placeholder. The upload stage refuses to run while this marker exists, precisely so nobody — including a fresh clone — can push placeholder data to a real BGG account. Starting fresh with your own token, you'll never see it again. +- **`data/STUB_DATA.marker`** — the committed CSVs were resolved from *hand-written stub fixtures* (the author's BGG application is still awaiting approval), so every version id in them is a synthetic placeholder. The upload stage refuses to run while this marker exists, precisely so nobody (including a fresh clone) can push placeholder data to a real BGG account. Starting fresh with your own token, you'll never see it again. - **`data/collection_snapshot_*.xml`** — with `BGG_API_TOKEN` set, `diff` fetches your collection live and you don't need these. Without a token (still waiting on approval?), you can use the logged-in-browser exemption: while signed in to BGG, save these two URLs as `data/collection_snapshot_base.xml` and `data/collection_snapshot_expansions.xml` (if you get a "queued" message, refresh after a few seconds): - `https://boardgamegeek.com/xmlapi2/collection?username=YOU&own=1&version=1` - `https://boardgamegeek.com/xmlapi2/collection?username=YOU&own=1&version=1&subtype=boardgameexpansion` diff --git a/scripts/record_fixtures.py b/scripts/record_fixtures.py index 29b7f5f..221bd00 100644 --- a/scripts/record_fixtures.py +++ b/scripts/record_fixtures.py @@ -18,8 +18,6 @@ from fixture_common import FIXTURE_CACHE from bggpipe.bgg_client import BGGClient from bggpipe.resolve import load_titles, resolve_entry -FIXTURE_CACHE = FIXTURE_CACHE - def main() -> None: if not os.environ.get("BGG_API_TOKEN"): diff --git a/src/bggpipe/bgg_client.py b/src/bggpipe/bgg_client.py index b8b2d54..1b07dd0 100644 --- a/src/bggpipe/bgg_client.py +++ b/src/bggpipe/bgg_client.py @@ -199,6 +199,6 @@ def client_for(cfg: Config) -> BGGClient: def cached_paths(cache_dir: Path, endpoint: str) -> list[Path]: - """Cache files for one endpoint — the ONLY sanctioned way to glob the - cache, so the filename layout stays private to cache_key.""" + """Cache files for one endpoint. Glob the cache only through this + helper so the filename layout stays private to cache_key.""" return sorted(cache_dir.glob(f"{endpoint}_*.xml")) diff --git a/src/bggpipe/diff.py b/src/bggpipe/diff.py index e88aee5..b649ce0 100644 --- a/src/bggpipe/diff.py +++ b/src/bggpipe/diff.py @@ -136,10 +136,10 @@ def compute_diff(rows: list[dict], collection: list[CollectionItem]) -> DiffResu c for c in by_object.get(bgg_id, []) if c.coll_id not in consumed_collids ] - # Ordered sub-passes over the confident rows. Greedy per-row handling - # let an EARLIER row's disagreement consume the exact-version copy a - # LATER row matched — producing a duplicate upload. Claims must settle - # strongest-first across ALL rows: exact version matches, then + # Ordered sub-passes over the confident rows: greedy per-row handling + # would let an earlier row's disagreement consume the exact-version copy + # a later row matches, manufacturing a duplicate upload. Claims settle + # strongest-first across all rows — exact version matches, then # versionless upgrades, then disagreement/second-copy handling. confident_rows = [r for r in recognized if is_confident_version(r)] leftover: list[dict] = [] @@ -251,10 +251,6 @@ def compute_diff(rows: list[dict], collection: list[CollectionItem]) -> DiffResu return result -def _write_csv(path: Path, columns: list[str], rows: list[dict]) -> None: - atomic_write_csv(path, columns, rows) # a killed diff never tears the queue - - def run_diff(cfg: Config, *, client: BGGClient | None = None) -> DiffResult: rows = read_matches(cfg.matches_path) if not rows: @@ -289,8 +285,9 @@ def run_diff(cfg: Config, *, client: BGGClient | None = None) -> DiffResult: result = compute_diff(rows, collection) - _write_csv(cfg.to_add_path, TO_ADD_COLUMNS, result.to_add) - _write_csv(cfg.to_update_path, TO_UPDATE_COLUMNS, result.to_update) + # atomic: a killed diff never leaves a torn upload queue + atomic_write_csv(cfg.to_add_path, TO_ADD_COLUMNS, result.to_add) + atomic_write_csv(cfg.to_update_path, TO_UPDATE_COLUMNS, result.to_update) merged_note = f" · {result.merged} merged duplicate(s)" if result.merged else "" typer.echo( diff --git a/src/bggpipe/extract.py b/src/bggpipe/extract.py index 2772f72..cb33f31 100644 --- a/src/bggpipe/extract.py +++ b/src/bggpipe/extract.py @@ -108,8 +108,8 @@ def parse_vision_response(text: str) -> tuple[list[dict], list[dict], int]: """Parse the model's JSON defensively: strip code fences, locate the payload amid any prose. Returns (title entries, unidentified sightings, dropped-malformed-entry count). - A bare JSON array (the pre-unidentified response shape) still parses — - it's all titles.""" + Accepts either response shape: a bare JSON array (all titles) or an + object with titles/unidentified keys.""" cleaned = _CODE_FENCE.sub("", text).strip() if cleaned[:1] in ("[", "{"): # trim trailing prose after a leading JSON payload ("{...}\nNote:") @@ -260,13 +260,13 @@ def rebuild_artifacts( raw_dir: Path, titles_path: Path, unidentified_path: Path ) -> tuple[list[dict], dict[str, list[dict]]]: """Regenerate titles.json and unidentified.json from the per-photo raw - cache. Raw files written before the unidentified feature are bare + cache. A raw file is either an object with titles/unidentified or a bare arrays — still readable.""" entries: list[dict] = [] unidentified: dict[str, list[dict]] = {} for raw_file in sorted(raw_dir.glob("*.json")): data = json.loads(raw_file.read_text()) - if isinstance(data, list): # legacy format + if isinstance(data, list): # bare-array shape entries.extend(data) continue entries.extend(data.get("titles") or []) diff --git a/src/bggpipe/resolve.py b/src/bggpipe/resolve.py index e818c34..aa3d216 100644 --- a/src/bggpipe/resolve.py +++ b/src/bggpipe/resolve.py @@ -155,9 +155,9 @@ def load_titles(path: Path) -> list[TitleEntry]: entries.append( TitleEntry( title_raw=title_raw, - # always recompute: a stale/hand-written stored value would + # always recompute: a hand-written stored value would # silently break exact matching (both sides must normalize - # by the CURRENT rules) + # by the current rules) title_normalized=normalize_title(title_raw), confidence=raw.get("confidence", "high"), publisher_hint=raw.get("publisher_hint") or "", @@ -483,7 +483,7 @@ def read_matches(path: Path) -> list[dict[str, str]]: return [] with path.open(newline="") as f: rows = list(csv.DictReader(f)) - for row in rows: # files written before these columns existed + for row in rows: # optional columns: tolerate rows without them row.setdefault("merged_into", "") row.setdefault("dedupe_veto", "") return rows diff --git a/src/bggpipe/upload.py b/src/bggpipe/upload.py index 54f4910..7663107 100644 --- a/src/bggpipe/upload.py +++ b/src/bggpipe/upload.py @@ -329,10 +329,8 @@ class PlaywrightUploader: try: dialog.get_by_role("listitem").first.wait_for(timeout=15_000) except self._timeout_error as err: - # A version resolve found on BGG cannot legitimately be missing - # from the picker — an unrendered list means a slow page or - # changed markup. Raising keeps the attempt retryable instead - # of a terminal (and false) added_no_version. + # a version resolve found on BGG can't be missing from the + # picker: an unrendered list means a slow page or changed markup raise RuntimeError( "version picker never rendered — site slow or markup " "changed; attempt is retryable" @@ -350,8 +348,8 @@ class PlaywrightUploader: nxt.click() self._page.wait_for_timeout(2_000) # etiquette: paginating hits BGG too else: - # never saw the end of the list: "not in picker" would be a - # false verdict frozen into DONE_STATUSES — stay retryable + # never saw the end of the list: "not in picker" would be a false + # verdict frozen into DONE_STATUSES raise RuntimeError( f"hit MAX_VERSION_PAGES ({MAX_VERSION_PAGES}) without " "finding the version or the end of the list — retryable" diff --git a/tests/test_client.py b/tests/test_client.py index 03b87e3..c67e580 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -135,7 +135,7 @@ def test_401_raises_actionable_auth_error(tmp_path, monkeypatch): client.get_xml("search", {"query": "catan"}) -# -- audit-fix regressions ---------------------------------------------- +# -- response validation and cache hygiene ------------------------------ def test_http_200_error_document_raises_and_is_never_cached(tmp_path): diff --git a/tests/test_diff.py b/tests/test_diff.py index 96d99d3..56fe930 100644 --- a/tests/test_diff.py +++ b/tests/test_diff.py @@ -3,11 +3,19 @@ snapshots as parsing fixtures. No network anywhere.""" from __future__ import annotations +import shutil from pathlib import Path from bggpipe.config import Config -from bggpipe.diff import SNAPSHOT_FILES, compute_diff, load_snapshot_collection +from bggpipe.diff import ( + SNAPSHOT_FILES, + compute_diff, + load_snapshot_collection, + run_diff, +) from bggpipe.models import CollectionItem +from bggpipe.resolve import write_matches +from bggpipe.upload import run_upload FIXTURES = Path(__file__).parent / "fixtures" @@ -171,16 +179,15 @@ def test_unvetoed_bare_duplicate_stays_owned(): 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 + # row A (v3, no match) must not consume the v2 copy that row B exactly + # matches — exact-version claims settle before disagreements 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 + # A's v3 box matches no collection entry: a genuine new copy assert [r["version_id"] for r in result.to_add] == ["3"] assert result.disagreements == [] @@ -276,11 +283,6 @@ def test_versionless_copies_exhaust_then_second_copy_becomes_add(): 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) @@ -307,10 +309,6 @@ def test_run_diff_outputs_feed_upload_unchanged(tmp_path, monkeypatch): 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) @@ -322,7 +320,7 @@ def test_token_without_username_says_so(tmp_path, monkeypatch, capsys): 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 + assert "No BGG_API_TOKEN" not in out # the token IS set; blame the username class _LiveClient: @@ -346,9 +344,6 @@ 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) @@ -360,10 +355,6 @@ def test_live_diff_fetches_fresh_collection(tmp_path, monkeypatch): 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") diff --git a/tests/test_extract.py b/tests/test_extract.py index b4081de..4cf957e 100644 --- a/tests/test_extract.py +++ b/tests/test_extract.py @@ -89,7 +89,7 @@ def test_parse_object_with_titles_and_unidentified(): assert unidentified[0]["location"] == "top shelf, left of Catan" -def test_parse_legacy_bare_array_still_works(): +def test_bare_array_response_shape_parses(): text = '```json\n[{"title_raw": "Catan", "confidence": "high"}]\n```' titles, unidentified, _ = parse_vision_response(text) assert titles[0]["title_raw"] == "Catan" @@ -274,8 +274,8 @@ def test_clean_photo_writes_empty_unidentified(tmp_path): assert json.loads(cfg.unidentified_path.read_text()) == {} -def test_legacy_array_raw_cache_still_rebuilds(tmp_path): - """Raw files written before the unidentified feature are bare arrays.""" +def test_bare_array_raw_cache_rebuilds(tmp_path): + """A raw cache file may be a bare title array; it must still rebuild.""" cfg = _cfg(tmp_path) cfg.photos_dir.mkdir() _write_image(cfg.photos_dir / "old.jpg") diff --git a/tests/test_models.py b/tests/test_models.py index 4d16afd..8e5b5e0 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -119,20 +119,12 @@ def test_error_document_raises(): def test_search_all_items_malformed_raises(): - import pytest - - from bggpipe.models import BGGResponseError, parse_search - xml = '' with pytest.raises(BGGResponseError): parse_search(xml) def test_search_partial_malformed_tolerated_with_warning(): - import pytest - - from bggpipe.models import parse_search - xml = ( '' '' diff --git a/tests/test_resolve.py b/tests/test_resolve.py index 1b20dfc..3beeaff 100644 --- a/tests/test_resolve.py +++ b/tests/test_resolve.py @@ -305,12 +305,10 @@ def test_wrong_year_hint_never_drives_a_version(client): def test_run_resolve_saves_progress_when_token_missing(tmp_path): """Cached titles resolve; uncached ones wait for the token instead of crashing the run and losing everything.""" - import shutil as _shutil - partial_cache = tmp_path / "cache" partial_cache.mkdir() for f in FIXTURES.glob("search_query=Catan-*"): - _shutil.copy(f, partial_cache / f.name) + shutil.copy(f, partial_cache / f.name) data_dir = tmp_path / "data" data_dir.mkdir() @@ -478,16 +476,14 @@ def test_run_resolve_dedupes_and_keeps_all_rows(tmp_path): client = BGGClient(cache_dir=cache, transport=httpx.MockTransport(_no_network)) run_resolve(cfg, client=client) - from bggpipe.resolve import read_matches as _rm - - saved = {r["title_raw"]: r for r in _rm(cfg.matches_path)} + saved = {r["title_raw"]: r for r in read_matches(cfg.matches_path)} assert len(saved) == 2 # no row disappeared assert saved["Wingspan"]["match_status"] == "auto" assert saved["WINGSPAN!"]["match_status"] == "merged" assert saved["WINGSPAN!"]["merged_into"] == "Wingspan" -# -- audit-fix regressions ---------------------------------------------- +# -- re-run and --force behavior ---------------------------------------- def test_run_resolve_force_rebuilds_from_scratch(client, tmp_path): @@ -595,8 +591,6 @@ def test_empty_normalized_title_never_matches(client): 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( @@ -618,8 +612,8 @@ def test_blocked_same_title_entry_defers_the_whole_group(tmp_path): 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") + transport=httpx.MockTransport( + lambda req: httpx.Response(401, text="Unauthorized") ), sleep=lambda s: None, ) @@ -630,7 +624,9 @@ def test_blocked_same_title_entry_defers_the_whole_group(tmp_path): 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 + assert ( + "Blorvath: Quest" not in heads + ) # two-word fallback uses the pre-subtitle head def test_reordered_titles_json_cannot_mispair_editions(client, tmp_path): diff --git a/tests/test_review.py b/tests/test_review.py index f9bf8b9..d9319cb 100644 --- a/tests/test_review.py +++ b/tests/test_review.py @@ -16,6 +16,7 @@ from bggpipe.bgg_client import BGGClient from bggpipe.config import Config from bggpipe.resolve import read_matches, write_matches from bggpipe.review import ReviewSession, run_review +from bggpipe.webreview import DismissStore FIXTURES = Path(__file__).parent / "fixtures" / "bgg_cache" @@ -291,12 +292,10 @@ def test_version_pass_is_skippable(tmp_path): assert row["version_status"] == "version_ambiguous" # untouched, review later -# -- audit-fix regressions ---------------------------------------------- +# -- concurrent-rewrite and degradation safety -------------------------- def test_veto_merge_persists_against_future_dedupe(tmp_path): - from bggpipe.resolve import read_matches - cfg = _setup( tmp_path, [ @@ -327,12 +326,10 @@ def test_failed_save_never_leaves_memory_ahead_of_disk(tmp_path, monkeypatch): ) 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) + monkeypatch.setattr("bggpipe.review.write_matches", exploding_write) with pytest.raises(OSError): session.decide_reject(row) # memory was rolled back to what disk actually holds @@ -341,8 +338,6 @@ def test_failed_save_never_leaves_memory_ahead_of_disk(tmp_path, monkeypatch): 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, @@ -364,13 +359,10 @@ def test_save_merges_own_decision_over_concurrent_external_rewrite(tmp_path): 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='') + transport=httpx.MockTransport( + lambda req: httpx.Response(200, text='') ), sleep=lambda s: None, ) @@ -385,11 +377,9 @@ def test_manual_id_unknown_to_bgg_warns_instead_of_crashing(tmp_path): 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 - + # run() iterates row references snapshotted before any reload; every + # decision must be re-adopted into the current list or it would be + # counted but never written. cfg = _setup( tmp_path, [ @@ -422,10 +412,8 @@ def test_every_tui_decision_after_external_rewrite_is_saved(tmp_path): 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 - + # two same-title entries are two editions: the version lookup must use + # the row's own photo-keyed cues, never a title-keyed last-wins dict entry_good = { "title_raw": "Wingspan", "publisher_hint": "Stonemaier", # matches the fixture's version @@ -445,42 +433,36 @@ def test_fill_version_uses_the_approved_rows_own_cues(tmp_path): title_raw="Wingspan", match_status="ambiguous", source_photos="good.jpg", - candidates_json=_json.dumps( + candidates_json=json.dumps( [{"bgg_id": 266192, "name": "Wingspan", "year": 2019}] ), ) ], ) - (cfg.data_dir / "titles.json").write_text(_json.dumps([entry_good, entry_bad])) + (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 + # the row's own photo (good.jpg) must select entry_good's cues 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) + monkeypatch.setattr("bggpipe.webreview.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"): diff --git a/tests/test_upload.py b/tests/test_upload.py index 343ba11..ce7c025 100644 --- a/tests/test_upload.py +++ b/tests/test_upload.py @@ -15,6 +15,7 @@ from bggpipe.config import Config from bggpipe.models import CollectionItem from bggpipe.upload import ( UPLOAD_LOG_COLUMNS, + LoginError, UploadJob, _scrub, build_queue, @@ -23,7 +24,7 @@ from bggpipe.upload import ( ) -def NOW() -> str: +def _now() -> str: return "2026-08-01T00:00:00+00:00" @@ -69,7 +70,7 @@ def _log_row(action="add", bgg_id="1", collid="", version_id="", status="added") "name": "Game", "version_id": version_id, "status": status, - "timestamp": NOW(), + "timestamp": _now(), "error": "", } @@ -170,7 +171,7 @@ def test_real_run_refuses_while_stub_marker_exists(tmp_path): (cfg.cache_dir / "STUB_FIXTURES.marker").write_text("stub") _seed_data(tmp_path, to_add=[_add_row()]) with pytest.raises(typer.Exit): - run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=NOW) + run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=_now) def test_dry_run_allowed_with_stub_marker_and_writes_nothing(tmp_path, capsys): @@ -178,7 +179,7 @@ def test_dry_run_allowed_with_stub_marker_and_writes_nothing(tmp_path, capsys): cfg.cache_dir.mkdir(parents=True) (cfg.cache_dir / "STUB_FIXTURES.marker").write_text("stub") _seed_data(tmp_path, to_add=[_add_row()], to_update=[_update_row()]) - run_upload(cfg, dry_run=True, now=NOW) + run_upload(cfg, dry_run=True, now=_now) out = capsys.readouterr().out assert "WARNING" in out and "SYNTHETIC" in out assert "would add Wingspan" in out @@ -197,7 +198,7 @@ def test_run_logs_every_attempt_and_continues_past_failures(tmp_path): to_update=[_update_row(collid="9")], ) fake = FakeUploader(failures={"Catan"}) - results = run_upload(cfg, uploader=fake, sleep=lambda s: None, now=NOW) + results = run_upload(cfg, uploader=fake, sleep=lambda s: None, now=_now) assert [r["status"] for r in results] == ["added", "failed", "updated"] logged = list(csv.DictReader((tmp_path / "upload_log.csv").open())) assert len(logged) == 3 @@ -209,10 +210,10 @@ def test_rerun_skips_completed_work(tmp_path): cfg = _cfg(tmp_path) _seed_data(tmp_path, to_add=[_add_row(bgg_id="1"), _add_row(bgg_id="2", name="C")]) fake = FakeUploader() - run_upload(cfg, uploader=fake, sleep=lambda s: None, now=NOW) + run_upload(cfg, uploader=fake, sleep=lambda s: None, now=_now) assert len(fake.calls) == 2 again = FakeUploader() - results = run_upload(cfg, uploader=again, sleep=lambda s: None, now=NOW) + results = run_upload(cfg, uploader=again, sleep=lambda s: None, now=_now) assert again.calls == [] and results == [] @@ -225,7 +226,7 @@ def test_pacing_sleeps_2_to_4s_between_games_only(tmp_path): uploader=FakeUploader(), sleep=sleeps.append, rng=random.Random(42), - now=NOW, + now=_now, ) assert len(sleeps) == 3 # between games, not before the first assert all(2.0 <= s <= 4.0 for s in sleeps) @@ -235,7 +236,7 @@ def test_limit_caps_the_queue(tmp_path): cfg = _cfg(tmp_path) _seed_data(tmp_path, to_add=[_add_row(bgg_id=str(i)) for i in range(1, 5)]) fake = FakeUploader() - run_upload(cfg, uploader=fake, limit=2, sleep=lambda s: None, now=NOW) + run_upload(cfg, uploader=fake, limit=2, sleep=lambda s: None, now=_now) assert len(fake.calls) == 2 @@ -303,22 +304,20 @@ def test_fresh_clone_marker_blocks_upload_without_cache_dir(tmp_path): (tmp_path / "STUB_DATA.marker").write_text("stub-derived CSVs") _seed_data(tmp_path, to_add=[_add_row()]) with pytest.raises(typer.Exit): - run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=NOW) + run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=_now) -# -- audit-fix regressions ---------------------------------------------- +# -- failure isolation and resume --------------------------------------- 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) + 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() @@ -327,7 +326,7 @@ 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) + 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 @@ -341,10 +340,10 @@ def test_added_no_version_is_done_and_verify_tolerates_it(tmp_path): 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) + 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 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())) @@ -354,7 +353,7 @@ def test_added_no_version_is_done_and_verify_tolerates_it(tmp_path): 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) + run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=_now) def test_second_update_for_same_game_is_deferred(tmp_path): @@ -369,11 +368,11 @@ 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) + 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) + results = run_upload(cfg, uploader=again, sleep=lambda s: None, now=_now) assert [j.collid for j in again.calls] == ["10"] @@ -383,7 +382,7 @@ def test_real_run_without_credentials_exits_before_any_browser(tmp_path, monkeyp 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 + run_upload(cfg, sleep=lambda s: None, now=_now) # uploader=None: real path assert not (tmp_path / "upload_log.csv").exists() @@ -395,15 +394,15 @@ def test_same_key_second_copy_survives_limit_and_interrupts(tmp_path): _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) + 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) + 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) == [] + assert run_upload(cfg, uploader=third, sleep=lambda s: None, now=_now) == [] def test_consecutive_failure_counter_resets_on_success(tmp_path): @@ -417,7 +416,7 @@ def test_consecutive_failure_counter_resets_on_success(tmp_path): 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) + run_upload(cfg, uploader=fake, sleep=lambda s: None, now=_now) assert len(fake.calls) == 6 # fail,fail,ok,fail,fail,ok — never aborts @@ -425,14 +424,13 @@ 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) + 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) + # the shortfall is reported once per game, not once per logged add 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"), @@ -456,7 +454,7 @@ 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) + run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=_now) client = _VerifyClient([_item(1, 10, name="Wingspan")]) run_upload( cfg, @@ -464,7 +462,7 @@ def test_run_upload_verify_wiring(tmp_path, capsys): verify=True, client=client, sleep=lambda s: None, - now=NOW, + now=_now, ) assert client.calls == [{"username": "tester", "refresh": True}] assert "Verification OK" in capsys.readouterr().out