From 38e20f2c30c3d9a36c4e96b47c5c65df4c781125 Mon Sep 17 00:00:00 2001 From: Eric Wagoner Date: Sun, 2 Aug 2026 14:10:57 -0400 Subject: [PATCH] =?UTF-8?q?Audit:=205-reviewer=20sweep=20=E2=80=94=2019=20?= =?UTF-8?q?fixes=20across=20every=20stage,=20+24=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CLAUDE.md | 2 +- scripts/fixture_common.py | 36 ++++++++ scripts/write_photo_fixtures.py | 31 +++---- scripts/write_stub_fixtures.py | 9 +- src/bggpipe/bgg_client.py | 19 ++++- src/bggpipe/cli.py | 11 ++- src/bggpipe/config.py | 41 ++++++++++ src/bggpipe/diff.py | 82 +++++++++++-------- src/bggpipe/enrich.py | 2 +- src/bggpipe/extract.py | 24 ++++-- src/bggpipe/models.py | 22 ++++- src/bggpipe/resolve.py | 117 +++++++++++++++----------- src/bggpipe/review.py | 105 +++++++++++++++++------- src/bggpipe/templates/review.html | 74 ++++++++++++----- src/bggpipe/upload.py | 111 +++++++++++++++++++------ src/bggpipe/webreview.py | 72 +++++++++++----- tests/test_client.py | 51 ++++++++++++ tests/test_config.py | 15 ++++ tests/test_diff.py | 69 ++++++++++++++-- tests/test_extract.py | 10 +-- tests/test_models.py | 4 + tests/test_normalize.py | 4 + tests/test_resolve.py | 132 +++++++++++++++++++++++++++--- tests/test_review.py | 96 +++++++++++++++++++++- tests/test_upload.py | 93 +++++++++++++++++++-- tests/test_webreview.py | 20 +++++ 26 files changed, 1003 insertions(+), 249 deletions(-) create mode 100644 scripts/fixture_common.py diff --git a/CLAUDE.md b/CLAUDE.md index efbb369..0a8b822 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -21,7 +21,7 @@ Full design lives in `bgg-shelf-pipeline-spec.md` (read it before changing pipel - `uv sync` — install deps (Python 3.12+, managed by **uv**; use `uv add`, never pip). Playwright needs a one-time `uv run playwright install chromium`. - `uv run bggpipe ` — run a pipeline stage. Non-secret settings come from `config.toml` (username, dirs, vision model, rate limit); `--config` overrides the path. -- `uv run pytest` — 105 tests, all offline against fixtures. Tests marked `live` hit the real BGG API (read-only) and are skipped unless you pass `--run-live`. +- `uv run pytest` — the suite runs fully offline against fixtures. Tests marked `live` hit the real BGG API (read-only) and are skipped unless you pass `--run-live`. - `uv run ruff check` / `uv run ruff format` — lint (rules E, F, I, UP, B, SIM) and format. ## Layout diff --git a/scripts/fixture_common.py b/scripts/fixture_common.py new file mode 100644 index 0000000..413a4f4 --- /dev/null +++ b/scripts/fixture_common.py @@ -0,0 +1,36 @@ +"""Shared plumbing for the two stub-fixture generators. + +Both write into the same cache dirs, so they must agree on XML escaping +(a title containing & or " must not produce malformed XML) and on the +provenance-marker text the upload guard depends on. +""" + +from __future__ import annotations + +from pathlib import Path + +CACHE_MARKER_TEXT = ( + "This cache contains hand-written stub XML, not real BGG " + "responses. Data resolved from it must not be uploaded.\n" +) +DATA_MARKER_TEXT = ( + "The CSVs in this directory were resolved from hand-written stub " + "fixtures, not real BGG data — version_ids are SYNTHETIC. The " + "upload stage refuses to run while this file exists. Delete it " + "only after re-resolving against real recorded fixtures " + "(BGG_API_TOKEN + scripts/record_fixtures.py + resolve --force).\n" +) + + +def esc(text: str) -> str: + """Minimal XML attribute/text escaping for hand-built fixture strings.""" + return str(text).replace("&", "&").replace("<", "<").replace('"', """) + + +def write_cache_marker(target: Path) -> None: + (target / "STUB_FIXTURES.marker").write_text(CACHE_MARKER_TEXT) + + +def write_data_marker(data_dir: Path = Path("data")) -> None: + data_dir.mkdir(parents=True, exist_ok=True) + (data_dir / "STUB_DATA.marker").write_text(DATA_MARKER_TEXT) diff --git a/scripts/write_photo_fixtures.py b/scripts/write_photo_fixtures.py index dead1b2..4d83bc3 100644 --- a/scripts/write_photo_fixtures.py +++ b/scripts/write_photo_fixtures.py @@ -17,6 +17,8 @@ from __future__ import annotations from pathlib import Path +from fixture_common import esc, write_cache_marker, write_data_marker + from bggpipe.bgg_client import cache_key TARGETS = (Path("tests/fixtures/bgg_cache"), Path("data/bgg_cache")) @@ -295,7 +297,7 @@ VERSIONS: dict[int, list[tuple]] = { def search_xml(results: list[tuple]) -> str: items = "".join( f'' - f'' + f'' f'' for bgg_id, name, year, type_, name_type in results ) @@ -306,12 +308,12 @@ def stats_xml(things: list[tuple]) -> str: items = "" for bgg_id, name, year, type_, owned, rank, publishers in things: links = "".join( - f'' + f'' for p in publishers ) items += ( f'' - f'' + f'' f'{links}' f"" f'' @@ -326,14 +328,14 @@ def versions_xml(bgg_id: int, versions: list[tuple]) -> str: version_items = "" for vid, name, year, publishers, languages in versions: links = "".join( - f'' + f'' for p in publishers ) + "".join( - f'' for lang in languages + f'' for lang in languages ) version_items += ( f'' - f'' + f'' f'{links}' ) return ( @@ -343,9 +345,6 @@ def versions_xml(bgg_id: int, versions: list[tuple]) -> str: ) -def _esc(text: str) -> str: - return text.replace("&", "&").replace("<", "<").replace('"', """) - def main() -> None: files: dict[str, str] = {} @@ -364,18 +363,8 @@ def main() -> None: (target / name).write_text(xml) # provenance marker: anything resolved from this cache is stub-derived # and NOT upload-ready; re-recording real fixtures removes the marker - (target / "STUB_FIXTURES.marker").write_text( - "This cache contains hand-written stub XML, not real BGG " - "responses. Data resolved from it must not be uploaded.\n" - ) - DATA_MARKER.parent.mkdir(parents=True, exist_ok=True) - DATA_MARKER.write_text( - "The CSVs in this directory were resolved from hand-written stub " - "fixtures, not real BGG data — version_ids are SYNTHETIC. The " - "upload stage refuses to run while this file exists. Delete it " - "only after re-resolving against real recorded fixtures " - "(BGG_API_TOKEN + scripts/record_fixtures.py + resolve --force).\n" - ) + write_cache_marker(target) + write_data_marker(DATA_MARKER.parent) print(f"Wrote {len(files)} fixture file(s) to {' and '.join(map(str, TARGETS))}") print(f"Wrote {DATA_MARKER} (committed; upload refuses while it exists)") diff --git a/scripts/write_stub_fixtures.py b/scripts/write_stub_fixtures.py index 6ad8da0..72c9c1f 100644 --- a/scripts/write_stub_fixtures.py +++ b/scripts/write_stub_fixtures.py @@ -14,6 +14,8 @@ from __future__ import annotations from pathlib import Path +from fixture_common import esc, write_cache_marker + from bggpipe.bgg_client import cache_key FIXTURE_CACHE = Path("tests/fixtures/bgg_cache") @@ -24,7 +26,7 @@ def search_item(bgg_id: int, name: str, year: int | None, type_: str) -> str: year_xml = f'' if year else "" return ( f'' - f'{year_xml}' + f'{year_xml}' ) @@ -104,10 +106,7 @@ THINGS = { def main() -> None: FIXTURE_CACHE.mkdir(parents=True, exist_ok=True) - (FIXTURE_CACHE / "STUB_FIXTURES.marker").write_text( - "This cache contains hand-written stub XML, not real BGG " - "responses. Data resolved from it must not be uploaded.\n" - ) + write_cache_marker(FIXTURE_CACHE) for query, items in SEARCHES.items(): key = cache_key("search", {"query": query, "type": SEARCH_TYPES}) total = items.count(" XML (bad + # username etc.) — caching one would poison every re-run + raise BGGResponseError( + f"BGG error document for /{endpoint}: {response.text[:200]}" + ) self.cache_dir.mkdir(parents=True, exist_ok=True) cache_path.write_text(response.text) return response.text @@ -137,6 +146,7 @@ class BGGClient: def things( self, ids: Iterable[int], + *, stats: bool = False, versions: bool = False, ) -> list[ThingDetails]: @@ -155,9 +165,9 @@ class BGGClient: def collection( self, username: str, + *, subtype: str | None = None, version: bool = True, - *, refresh: bool = False, ) -> list[CollectionItem]: params = {"username": username, "own": "1"} @@ -179,3 +189,8 @@ class BGGClient: ) seen = {item.coll_id for item in base} return base + [e for e in expansions if e.coll_id not in seen] + + +def client_for(cfg: Config) -> BGGClient: + """The standard injection fallback: every stage's `client or client_for(cfg)`.""" + return BGGClient(cfg.cache_dir, cfg.rate_limit_seconds) diff --git a/src/bggpipe/cli.py b/src/bggpipe/cli.py index 736e6b3..4a7add3 100644 --- a/src/bggpipe/cli.py +++ b/src/bggpipe/cli.py @@ -7,7 +7,7 @@ from typing import Annotated import typer -from bggpipe.config import load_config +from bggpipe.config import DEFAULT_REVIEW_PORT, load_config app = typer.Typer( help="Shelf-to-BGG collection pipeline.", @@ -56,7 +56,9 @@ def review( web: Annotated[ bool, typer.Option("--web", help="Serve the review UI on localhost") ] = False, - port: Annotated[int, typer.Option("--port", help="Port for --web")] = 8377, + port: Annotated[ + int, typer.Option("--port", help="Port for --web") + ] = DEFAULT_REVIEW_PORT, dev: Annotated[ bool, typer.Option("--dev", help="With --web: restart on source changes") ] = False, @@ -122,7 +124,10 @@ def upload( @app.command() def enrich( - refresh: Annotated[bool, typer.Option("--refresh")] = False, + refresh: Annotated[ + bool, + typer.Option("--refresh", help="Re-fetch metadata (ranks/ratings drift)"), + ] = False, config: ConfigOpt = None, ) -> None: """Stage 6: fetch full game + version metadata into games.json.""" diff --git a/src/bggpipe/config.py b/src/bggpipe/config.py index 6fa8100..9e8b3ba 100644 --- a/src/bggpipe/config.py +++ b/src/bggpipe/config.py @@ -10,10 +10,12 @@ from __future__ import annotations import os import tomllib +import warnings from dataclasses import dataclass, replace from pathlib import Path DEFAULT_CONFIG_PATH = Path("config.toml") +DEFAULT_REVIEW_PORT = 8377 @dataclass(frozen=True) @@ -40,6 +42,38 @@ class Config: def matches_path(self) -> Path: return self.data_dir / "matches.csv" + @property + def extract_raw_dir(self) -> Path: + return self.data_dir / "extract_raw" + + @property + def to_add_path(self) -> Path: + return self.data_dir / "to_add.csv" + + @property + def to_update_path(self) -> Path: + return self.data_dir / "to_update.csv" + + @property + def upload_log_path(self) -> Path: + return self.data_dir / "upload_log.csv" + + @property + def games_path(self) -> Path: + return self.data_dir / "games.json" + + @property + def dismissed_path(self) -> Path: + return self.data_dir / "unidentified_dismissed.json" + + @property + def stub_marker_paths(self) -> tuple[Path, Path]: + # gitignored (travels with the stub XML) + committed (guards clones) + return ( + self.cache_dir / "STUB_FIXTURES.marker", + self.data_dir / "STUB_DATA.marker", + ) + def load_config(path: Path | None = None) -> Config: cfg = Config() @@ -53,6 +87,13 @@ def load_config(path: Path | None = None) -> Config: "rate_limit_seconds": float, } updates = {key: caster(raw[key]) for key, caster in known.items() if key in raw} + if unknown := sorted(raw.keys() - known.keys()): + # a typo'd key silently falling back to defaults is a debugging + # trap ("No photos found in photos/") — say so up front + warnings.warn( + f"{p}: ignoring unknown key(s): {', '.join(unknown)}", + stacklevel=2, + ) cfg = replace(cfg, **updates) if username := os.environ.get("BGG_USERNAME"): cfg = replace(cfg, bgg_username=username) diff --git a/src/bggpipe/diff.py b/src/bggpipe/diff.py index d62135f..d4314a3 100644 --- a/src/bggpipe/diff.py +++ b/src/bggpipe/diff.py @@ -8,11 +8,12 @@ Two collection sources: logged-in-user exemption. Outputs both artifacts: -- to_add.csv — recognized games not in the collection (new entries); +- to_add.csv — recognized games not in the collection, including + additional copies whose confident version matches no owned copy; - to_update.csv — owned, VERSION-LESS entries where matching produced a confident version (version_auto/version_approved). Strictly additive: - entries that already carry a version are never touched — a photo/version - disagreement is reported in the summary instead. + entries that already carry a version are never touched — a further copy + with a different version becomes a to_add row instead. """ from __future__ import annotations @@ -24,9 +25,13 @@ from pathlib import Path import typer -from bggpipe.bgg_client import BGGClient +from bggpipe.bgg_client import BGGClient, client_for from bggpipe.config import Config -from bggpipe.models import CollectionItem, parse_collection +from bggpipe.models import ( + CONFIDENT_VERSION_STATUSES, + CollectionItem, + parse_collection, +) from bggpipe.resolve import read_matches TO_ADD_COLUMNS = [ @@ -42,7 +47,6 @@ TO_ADD_COLUMNS = [ TO_UPDATE_COLUMNS = ["collid", "bgg_id", "bgg_name", "version_id", "version_name"] SNAPSHOT_FILES = ("collection_snapshot_base.xml", "collection_snapshot_expansions.xml") -_CONFIDENT_VERSION = ("version_auto", "version_approved") @dataclass @@ -50,7 +54,7 @@ class DiffResult: to_add: list[dict] = field(default_factory=list) to_update: list[dict] = field(default_factory=list) already_owned: list[str] = field(default_factory=list) # title_raw - disagreements: list[str] = field(default_factory=list) + second_copies: list[str] = field(default_factory=list) # notes for adds unseen: list[CollectionItem] = field(default_factory=list) pending: list[str] = field(default_factory=list) # ambiguous/unmatched titles rejected: int = 0 @@ -113,24 +117,27 @@ def compute_diff(rows: list[dict], collection: list[CollectionItem]) -> DiffResu if copies: seen_object_ids.add(bgg_id) - confident = row["version_status"] in _CONFIDENT_VERSION and row["version_id"] + confident = ( + row["version_status"] in CONFIDENT_VERSION_STATUSES and row["version_id"] + ) version_id = int(row["version_id"]) if confident else None - if not copies: + def add_row(row: dict = row, confident: bool = confident) -> dict: photos = {p for p in row["source_photos"].split(";") if p} photos |= merged_photos.get(row["title_raw"], set()) - result.to_add.append( - { - "bgg_id": row["bgg_id"], - "bgg_name": row["bgg_name"], - "year": row["year"], - "type": row["type"], - "version_id": row["version_id"] if confident else "", - "version_name": row["version_name"] if confident else "", - "title_raw": row["title_raw"], - "source_photos": ";".join(sorted(photos)), - } - ) + return { + "bgg_id": row["bgg_id"], + "bgg_name": row["bgg_name"], + "year": row["year"], + "type": row["type"], + "version_id": row["version_id"] if confident else "", + "version_name": row["version_name"] if confident else "", + "title_raw": row["title_raw"], + "source_photos": ";".join(sorted(photos)), + } + + if not copies: + result.to_add.append(add_row()) continue if not confident: @@ -161,12 +168,15 @@ def compute_diff(rows: list[dict], collection: list[CollectionItem]) -> DiffResu } ) else: - # every copy already carries a (different) version — never touch it - result.already_owned.append(row["title_raw"]) - result.disagreements.append( - f"{row['title_raw']}: photo suggests version " - f"{row['version_name']!r} ({row['version_id']}) but the " - "collection entry already has a version set — not changing it" + # no remaining copy can take this version: every copy either + # already carries a different version or was consumed by another + # match — per spec this is an additional physical copy to ADD + # (existing entries are never touched) + result.to_add.append(add_row()) + result.second_copies.append( + f"{row['title_raw']}: adding as a NEW copy with version " + f"{row['version_name']!r} ({row['version_id']}) — every " + "existing entry of this game keeps its current version" ) result.unseen = [ @@ -177,10 +187,12 @@ def compute_diff(rows: list[dict], collection: list[CollectionItem]) -> DiffResu def _write_csv(path: Path, columns: list[str], rows: list[dict]) -> None: path.parent.mkdir(parents=True, exist_ok=True) - with path.open("w", newline="") as f: + tmp = path.with_name(path.name + ".tmp") + with tmp.open("w", newline="") as f: writer = csv.DictWriter(f, fieldnames=columns, extrasaction="ignore") writer.writeheader() writer.writerows(rows) + os.replace(tmp, path) # atomic: a killed diff never leaves a torn queue def run_diff(cfg: Config, *, client: BGGClient | None = None) -> DiffResult: @@ -191,8 +203,8 @@ def run_diff(cfg: Config, *, client: BGGClient | None = None) -> DiffResult: if os.environ.get("BGG_API_TOKEN") and cfg.bgg_username: typer.echo("Fetching live collection from BGG…") - client = client or BGGClient(cfg.cache_dir, cfg.rate_limit_seconds) - collection = client.collection_full(cfg.bgg_username) + client = client or client_for(cfg) + collection = client.collection_full(cfg.bgg_username, refresh=True) else: typer.echo( "No BGG_API_TOKEN — using collection snapshot files in " @@ -202,8 +214,8 @@ def run_diff(cfg: Config, *, client: BGGClient | None = None) -> DiffResult: result = compute_diff(rows, collection) - _write_csv(cfg.data_dir / "to_add.csv", TO_ADD_COLUMNS, result.to_add) - _write_csv(cfg.data_dir / "to_update.csv", TO_UPDATE_COLUMNS, result.to_update) + _write_csv(cfg.to_add_path, TO_ADD_COLUMNS, result.to_add) + _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( @@ -212,9 +224,9 @@ def run_diff(cfg: Config, *, client: BGGClient | None = None) -> DiffResult: f"update(s) · {len(result.pending)} pending review · " f"{result.rejected} rejected{merged_note}" ) - if result.disagreements: - typer.echo("\nVersion disagreements (left untouched):") - for line in result.disagreements: + if result.second_copies: + typer.echo("\nSecond copies (verify these on the dry run before upload):") + for line in result.second_copies: typer.echo(f" - {line}") if result.pending: typer.echo("\nStill pending review: " + ", ".join(result.pending)) diff --git a/src/bggpipe/enrich.py b/src/bggpipe/enrich.py index 449f557..5913379 100644 --- a/src/bggpipe/enrich.py +++ b/src/bggpipe/enrich.py @@ -66,7 +66,7 @@ def run_enrich( key = f"{row['bgg_id']}:{version['version_id']}" if version else row["bgg_id"] targets.append((key, int(row["bgg_id"]), version)) - games_path = cfg.data_dir / "games.json" + games_path = cfg.games_path games: dict = json.loads(games_path.read_text()) if games_path.exists() else {} need = sorted({bgg_id for key, bgg_id, _ in targets if refresh or key not in games}) diff --git a/src/bggpipe/extract.py b/src/bggpipe/extract.py index 507bbfa..ecf1f1c 100644 --- a/src/bggpipe/extract.py +++ b/src/bggpipe/extract.py @@ -103,7 +103,7 @@ def prepare_image(path: Path) -> tuple[str, str]: return base64.standard_b64encode(buffer.getvalue()).decode(), "image/jpeg" -def parse_vision_response(text: str) -> tuple[list[dict], list[dict]]: +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). A bare JSON array (the pre-unidentified response shape) still parses — @@ -133,7 +133,8 @@ def parse_vision_response(text: str) -> tuple[list[dict], list[dict]]: if isinstance(u, dict) and (u.get("location") or u.get("partial_text") or u.get("art_notes")) ] - return titles, unidentified + dropped = len(titles_raw) - len(titles) + return titles, unidentified, dropped def default_vision(model: str) -> VisionFn: @@ -172,7 +173,9 @@ def extract_photo(photo: Path, vision: VisionFn) -> dict: """One photo -> {"titles": [...], "unidentified": [...]} (the raw-cache file format).""" image_b64, media_type = prepare_image(photo) - raw_entries, raw_unidentified = parse_vision_response(vision(image_b64, media_type)) + raw_entries, raw_unidentified, dropped = parse_vision_response( + vision(image_b64, media_type) + ) unidentified = [ { "location": str(u.get("location") or "").strip(), @@ -198,10 +201,10 @@ def extract_photo(photo: Path, vision: VisionFn) -> dict: "source_photos": [photo.name], } ) - return {"titles": entries, "unidentified": unidentified} + return {"titles": entries, "unidentified": unidentified, "dropped": dropped} -def _cues_conflict(a: dict, b: dict) -> bool: +def cues_conflict(a: dict, b: dict) -> bool: """Two sightings conflict if any edition cue is set on both and differs — that means visibly different boxes, i.e. separate editions (spec).""" for key in ("publisher_hint", "edition_hint", "language_hint"): @@ -238,7 +241,7 @@ def dedupe_entries(entries: list[dict]) -> list[dict]: for existing in result: if existing["title_normalized"] == entry[ "title_normalized" - ] and not _cues_conflict(existing, entry): + ] and not cues_conflict(existing, entry): existing.update(_merge(existing, entry)) break else: @@ -298,7 +301,7 @@ def run_extract( ) raise typer.Exit(code=1) - raw_dir = cfg.data_dir / "extract_raw" + raw_dir = cfg.extract_raw_dir raw_dir.mkdir(parents=True, exist_ok=True) vision = vision or default_vision(cfg.model) @@ -314,6 +317,13 @@ def run_extract( if result["unidentified"] else "" ) + if result.get("dropped"): + # the prompt forbids silently omitting a box; so do we + note += ( + f" — DROPPED {result['dropped']} malformed entr" + f"{'y' if result['dropped'] == 1 else 'ies'}; inspect " + f"{raw_path}" + ) typer.echo(f" {photo.name}: {len(result['titles'])} title(s){note}") deduped, unidentified = rebuild_artifacts( diff --git a/src/bggpipe/models.py b/src/bggpipe/models.py index 9c96bc0..c4a0ee7 100644 --- a/src/bggpipe/models.py +++ b/src/bggpipe/models.py @@ -12,6 +12,12 @@ class BGGResponseError(Exception): """The API returned a well-formed error document (e.g. bad username).""" +# The one status predicate the whole pipeline shares: a version is trusted +# for diff/upload/enrich only when matching produced it confidently or a +# human approved it. +CONFIDENT_VERSION_STATUSES = ("version_auto", "version_approved") + + @dataclass(frozen=True) class SearchResult: bgg_id: int @@ -209,6 +215,16 @@ def parse_things_full(xml_text: str) -> list[dict]: return games +def _required_attr(item, name: str) -> str: + value = item.get(name) + if not value: + raise BGGResponseError( + f"collection item missing {name!r} — truncated or unexpected " + "response; refusing to feed it to the diff" + ) + return value + + def parse_collection(xml_text: str) -> list[CollectionItem]: items = [] for item in _root(xml_text).findall("item"): @@ -217,8 +233,10 @@ def parse_collection(xml_text: str) -> list[CollectionItem]: version_item = item.find("version/item") items.append( CollectionItem( - object_id=int(item.get("objectid", 0)), - coll_id=int(item.get("collid", 0)), + # strict: a missing id coerced to 0 would collide in the + # collid dedupe and silently drop owned games from the diff + object_id=int(_required_attr(item, "objectid")), + coll_id=int(_required_attr(item, "collid")), name=item.findtext("name", default=""), subtype=item.get("subtype", "boardgame"), own=status is not None and status.get("own") == "1", diff --git a/src/bggpipe/resolve.py b/src/bggpipe/resolve.py index 8e30b10..c80df47 100644 --- a/src/bggpipe/resolve.py +++ b/src/bggpipe/resolve.py @@ -12,16 +12,17 @@ import csv import json import os import re +from collections import Counter from dataclasses import dataclass, field from pathlib import Path import typer from rapidfuzz import fuzz -from bggpipe.bgg_client import BGGAuthError, BGGClient +from bggpipe.bgg_client import BGGAuthError, BGGClient, client_for from bggpipe.config import Config -from bggpipe.extract import _cues_conflict -from bggpipe.models import GameVersion +from bggpipe.extract import cues_conflict +from bggpipe.models import CONFIDENT_VERSION_STATUSES, GameVersion from bggpipe.normalize import normalize_title FUZZY_THRESHOLD = 90 @@ -30,6 +31,10 @@ FUZZY_THRESHOLD = 90 DOMINANCE_MIN_OWNED = 100 DOMINANCE_FACTOR = 10 VERSION_PLAUSIBLE_SCORE = 2 +# "Is this the publisher on the box?" — one answer, asked in two places +# (candidate tie-break and version scoring): the sites must move together. +PUBLISHER_MATCH_THRESHOLD = 85 +EDITION_NAME_THRESHOLD = 80 MATCH_COLUMNS = [ "title_raw", @@ -45,6 +50,7 @@ MATCH_COLUMNS = [ "version_candidates_json", "source_photos", "merged_into", + "dedupe_veto", ] @@ -127,6 +133,8 @@ class MatchRow: self.version_candidates, ensure_ascii=False ), "source_photos": ";".join(self.source_photos), + "merged_into": "", + "dedupe_veto": "", } @@ -198,6 +206,10 @@ def _plausible_candidates( by_id: dict[int, Candidate] = {} for result in client.search(query): norm = normalize_title(result.name) + # a fully non-Latin title normalizes to "" — empty-vs-empty is not a + # match (and token_sort_ratio("", "") is 100, so guard fuzzy too) + if not entry.title_normalized: + continue exact = norm == entry.title_normalized fuzzy = fuzz.token_sort_ratio(norm, entry.title_normalized) if not exact and fuzzy < FUZZY_THRESHOLD: @@ -259,13 +271,19 @@ def _publisher_pick(entry: TitleEntry, top: list[Candidate]) -> Candidate | None candidate is from that publisher, that's the game.""" if not entry.publisher_hint: return None + if len({c.type for c in top}) > 1: + # mixed base-game/expansion candidates are never auto-resolved — + # the same veto _dominant applies (spec's top failure mode); an + # alternate name can make an expansion "exact" too + return None hint = normalize_title(entry.publisher_hint) matches = [ c for c in top if c.exact and any( - fuzz.partial_ratio(hint, normalize_title(p)) >= 85 for p in c.publishers + fuzz.partial_ratio(hint, normalize_title(p)) >= PUBLISHER_MATCH_THRESHOLD + for p in c.publishers ) ] return matches[0] if len(matches) == 1 else None @@ -296,7 +314,7 @@ def _score_version(entry: TitleEntry, version: GameVersion) -> int: if entry.publisher_hint: hint = normalize_title(entry.publisher_hint) if any( - fuzz.partial_ratio(hint, normalize_title(p)) >= 85 + fuzz.partial_ratio(hint, normalize_title(p)) >= PUBLISHER_MATCH_THRESHOLD for p in version.publishers ): score += 2 @@ -312,18 +330,24 @@ def _score_version(entry: TitleEntry, version: GameVersion) -> int: and fuzz.token_set_ratio( normalize_title(entry.edition_hint), normalize_title(version.name) ) - >= 80 + >= EDITION_NAME_THRESHOLD ): score += 2 return score -def _resolve_version(client: BGGClient, entry: TitleEntry, row: MatchRow) -> None: - """Fill version_* fields on an auto/approved row. Never guess (spec).""" +def resolve_version(client: BGGClient, entry: TitleEntry, row: MatchRow) -> None: + """Fill version_* fields on an auto/approved row. Never guess (spec). + Public: review's manual-id path calls this too.""" if not entry.has_version_cues: row.version_status = "version_unknown" return - (thing,) = client.things([row.bgg_id], versions=True) + things = client.things([row.bgg_id], versions=True) + if not things: + # a manually-typed id BGG doesn't know: nothing to offer + row.version_status = "version_unknown" + return + thing = things[0] scored = sorted( ((v, _score_version(entry, v)) for v in thing.versions), key=lambda pair: -pair[1], @@ -364,14 +388,10 @@ def resolve_entry(client: BGGClient, entry: TitleEntry) -> MatchRow: break row = _classify(client, entry, cands) if row.match_status == "auto": - _resolve_version(client, entry, row) + resolve_version(client, entry, row) return row -def _row_key(title_raw: str, source_photos: str) -> tuple[str, str]: - return (title_raw, source_photos) - - @dataclass(frozen=True) class MergeEvent: loser_title: str @@ -409,9 +429,13 @@ def dedupe_matches(rows: list[dict], titles: list[TitleEntry]) -> list[MergeEven for row in rows: if row["match_status"] not in ("auto", "approved") or not row["bgg_id"]: continue + if row.get("dedupe_veto"): + # a human already ruled "this is a genuinely separate copy" — + # re-running resolve must never overturn that (spec: re-runs + # lose no work, least of all review decisions) + continue confident = ( - row["version_status"] in ("version_auto", "version_approved") - and row["version_id"] + row["version_status"] in CONFIDENT_VERSION_STATUSES and row["version_id"] ) key = (row["bgg_id"], row["version_id"] if confident else "") groups.setdefault(key, []).append(row) @@ -429,7 +453,7 @@ def dedupe_matches(rows: list[dict], titles: list[TitleEntry]) -> list[MergeEven ) survivor = group[0] for loser in group[1:]: - if _cues_conflict(cues(survivor), cues(loser)): + if cues_conflict(cues(survivor), cues(loser)): continue # conflicting edition cues: genuinely two copies loser["match_status"] = "merged" loser["merged_into"] = survivor["title_raw"] @@ -449,8 +473,9 @@ 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 the merged_into column existed + for row in rows: # files written before these columns existed row.setdefault("merged_into", "") + row.setdefault("dedupe_veto", "") return rows @@ -467,39 +492,40 @@ def write_matches(path: Path, rows: list[dict[str, str]]) -> None: os.replace(tmp, path) -def read_existing_keys(path: Path) -> set[tuple[str, str]]: - if not path.exists(): - return set() - with path.open(newline="") as f: - return {_row_key(r["title_raw"], r["source_photos"]) for r in csv.DictReader(f)} - - -def append_rows(path: Path, rows: list[MatchRow]) -> None: - path.parent.mkdir(parents=True, exist_ok=True) - is_new = not path.exists() - with path.open("a", newline="") as f: - writer = csv.DictWriter(f, fieldnames=MATCH_COLUMNS) - if is_new: - writer.writeheader() - for row in rows: - writer.writerow(row.to_csv()) - - def run_resolve( cfg: Config, *, force: bool = False, client: BGGClient | None = None ) -> list[MatchRow]: entries = load_titles(cfg.titles_path) if force and cfg.matches_path.exists(): cfg.matches_path.unlink() - existing = read_existing_keys(cfg.matches_path) - client = client or BGGClient(cfg.cache_dir, cfg.rate_limit_seconds) + existing_rows = read_matches(cfg.matches_path) + client = client or client_for(cfg) + + # Pair entries with existing rows BY TITLE, positionally, not by exact + # (title, photos) key: extract unions a new photo of an already-resolved + # game into its entry, and that must update the row's provenance — not + # re-resolve the game as a duplicate row. Same-title entries only stay + # separate when their cues conflict (two editions), and those pair up + # in stable file order on both sides. + rows_by_title: dict[str, list[dict]] = {} + for row in existing_rows: + rows_by_title.setdefault(row["title_raw"], []).append(row) + seen_per_title: dict[str, int] = {} new_rows: list[MatchRow] = [] skipped = 0 + photos_updated = False blocked: list[str] = [] for entry in entries: - key = _row_key(entry.title_raw, ";".join(entry.source_photos)) - if key in existing: + ix = seen_per_title.get(entry.title_raw, 0) + seen_per_title[entry.title_raw] = ix + 1 + paired = rows_by_title.get(entry.title_raw, []) + if ix < len(paired): + row_dict = paired[ix] + photos = ";".join(entry.source_photos) + if row_dict["source_photos"] != photos: + row_dict["source_photos"] = photos + photos_updated = True skipped += 1 continue try: @@ -515,12 +541,11 @@ def run_resolve( version = f" [{row.version_status}]" if row.version_status else "" typer.echo(f" {entry.title_raw!r} -> {row.match_status}: {detail}{version}") - append_rows(cfg.matches_path, new_rows) - - all_rows = read_matches(cfg.matches_path) + all_rows = existing_rows + [row.to_csv() for row in new_rows] merges = dedupe_matches(all_rows, entries) + if new_rows or photos_updated or merges: + write_matches(cfg.matches_path, all_rows) # atomic full rewrite if merges: - write_matches(cfg.matches_path, all_rows) typer.echo("") for m in merges: typer.echo( @@ -528,9 +553,7 @@ def run_resolve( f"game ({m.bgg_name}, {m.bgg_id}); veto in review if wrong" ) - counts: dict[str, int] = {} - for row in new_rows: - counts[row.match_status] = counts.get(row.match_status, 0) + 1 + counts = Counter(row.match_status for row in new_rows) summary = ", ".join(f"{n} {status}" for status, n in sorted(counts.items())) typer.echo( f"Resolved {len(new_rows)} title(s) ({summary or 'nothing new'}); " diff --git a/src/bggpipe/review.py b/src/bggpipe/review.py index 755b29c..8270e68 100644 --- a/src/bggpipe/review.py +++ b/src/bggpipe/review.py @@ -1,9 +1,11 @@ -"""Stage 3 — human review of ambiguous/unmatched matches, then versions. +"""Stage 3 — the review decision engine, worn by two faces. -A plain rich prompt loop (deliberately dependency-light, per spec). Every -decision rewrites matches.csv atomically, so quitting mid-review — q, -Ctrl-C, or end of input — loses nothing; the next run resumes with -whatever is still ambiguous/unmatched. The version pass is optional and +ReviewSession owns all decision logic and every matches.csv write: the +decision API (decide_*/veto_merge/pending_rows/version_rows) serves both +this module's rich TUI prompt loop and webreview's FastAPI endpoints, and +reload_if_changed() lets either face follow external rewrites (a resolve +run in another terminal). Every decision rewrites matches.csv atomically, +so quitting mid-review loses nothing. The version pass is optional and skippable: version review must never block getting games uploaded. """ @@ -14,18 +16,20 @@ from collections.abc import Callable from pathlib import Path import httpx +import typer from rich.console import Console from rich.panel import Panel from rich.table import Table -from bggpipe.bgg_client import BGGAuthError, BGGClient, BGGQueueTimeout +from bggpipe.bgg_client import BGGAuthError, BGGClient, BGGQueueTimeout, client_for from bggpipe.config import Config from bggpipe.models import BGGResponseError from bggpipe.resolve import ( MatchRow, - _resolve_version, + TitleEntry, load_titles, read_matches, + resolve_version, write_matches, ) @@ -57,8 +61,9 @@ class ReviewSession: self.cfg = cfg self.console = console or Console() self.input_fn = input_fn or (lambda prompt: self.console.input(prompt)) - self.client = client or BGGClient(cfg.cache_dir, cfg.rate_limit_seconds) + self.client = client or client_for(cfg) self.decisions = 0 + self.warnings: list[str] = [] self._load() # -- plumbing ------------------------------------------------------- @@ -99,8 +104,34 @@ class ReviewSession: raise _Quit return answer - def _save(self) -> None: - write_matches(self.cfg.matches_path, self.rows) + def _warn(self, message: str) -> None: + """Degradations must be visible in BOTH faces: the console for the + TUI, and self.warnings for the web UI (whose console is a StringIO).""" + self.warnings.append(message) + self.console.print(f"[yellow]{message}[/yellow]") + + def _save(self, row: dict | None = None) -> None: + """Atomic write of the in-memory rows. If another process rewrote the + file since we loaded (resolve in a second terminal), reload first and + re-apply `row` — the one decision being saved — by identity, so + neither side's work is silently lost.""" + if self.reload_if_changed() and row is not None: + key = (row["title_raw"], row["source_photos"]) + for i, fresh in enumerate(self.rows): + if (fresh["title_raw"], fresh["source_photos"]) == key: + self.rows[i] = row + break + else: + self._warn( + f"{row['title_raw']!r} disappeared from matches.csv while " + "you decided — decision NOT saved" + ) + return + try: + write_matches(self.cfg.matches_path, self.rows) + except OSError: + self._load() # memory must never claim what disk doesn't hold + raise self._loaded_mtimes = self._data_mtimes() # own writes aren't "changes" self.decisions += 1 @@ -111,7 +142,7 @@ class ReviewSession: row["year"] = str(candidate.get("year") or "") row["type"] = candidate.get("type") or "boardgame" self._fill_version(row) - self._save() + self._save(row) def _fill_version(self, row: dict) -> None: """Try version resolution for a just-approved row. Degrades gracefully: @@ -122,9 +153,9 @@ class ReviewSession: return shim = MatchRow(title_raw=row["title_raw"], bgg_id=int(row["bgg_id"])) try: - _resolve_version(self.client, entry, shim) + resolve_version(self.client, entry, shim) except _BGG_ERRORS as err: - self.console.print(f"[yellow]version lookup unavailable: {err}[/yellow]") + self._warn(f"version lookup unavailable ({err}) — recorded version_unknown") row["version_status"] = "version_unknown" return row["version_status"] = shim.version_status @@ -155,9 +186,21 @@ class ReviewSession: row as a distinct, human-confirmed match.""" row["match_status"] = "approved" row["merged_into"] = "" - self._save() + row["dedupe_veto"] = "1" # persists: resolve re-runs must not re-merge + self._save(row) - def cues_for(self, title_raw: str): + def cues_for( + self, title_raw: str, source_photos: str | None = None + ) -> TitleEntry | None: + """The extraction entry behind a row. Same-title entries (two + editions of one game) are told apart by their photo set when given.""" + if source_photos is not None: + for entry in self.titles: + if ( + entry.title_raw == title_raw + and ";".join(entry.source_photos) == source_photos + ): + return entry return self._titles.get(title_raw) def decide_pick(self, row: dict, candidate: dict) -> None: @@ -168,7 +211,7 @@ class ReviewSession: def decide_reject(self, row: dict) -> None: row["match_status"] = "rejected" - self._save() + self._save(row) def decide_version(self, row: dict, version_id: int | None) -> None: """Pick a version from the row's stored candidates, or None -> unknown.""" @@ -186,7 +229,7 @@ class ReviewSession: row["version_status"] = "version_approved" row["version_id"] = str(version_id) row["version_name"] = chosen.get("name") or "" - self._save() + self._save(row) # -- displays ------------------------------------------------------- @@ -247,13 +290,20 @@ class ReviewSession: def _manual_id(self, row: dict, bgg_id: int) -> None: candidate: dict = {"bgg_id": bgg_id} try: - (thing,) = self.client.things([bgg_id]) - candidate.update(name=thing.name, year=thing.year, type=thing.type) + things = self.client.things([bgg_id]) except _BGG_ERRORS as err: - self.console.print( - f"[yellow]couldn't look up id {bgg_id} ({err}) — " - "recording the id with no name[/yellow]" + self._warn( + f"couldn't look up id {bgg_id} ({err}) — recording the id with no name" ) + else: + if things: + thing = things[0] + candidate.update(name=thing.name, year=thing.year, type=thing.type) + else: + self._warn( + f"BGG knows no game with id {bgg_id} — recording it " + "with no name (typo?)" + ) self._apply_choice(row, candidate) def _research(self, query: str) -> list[dict]: @@ -302,15 +352,10 @@ class ReviewSession: # -- entry point ---------------------------------------------------- def run(self) -> None: - pending = [ - r for r in self.rows if r["match_status"] in ("ambiguous", "unmatched") - ] try: - for row in pending: + for row in self.pending_rows(): self._review_match_row(row) - versions = [ - r for r in self.rows if r["version_status"] == "version_ambiguous" - ] + versions = self.version_rows() if versions: answer = self._ask( f"{len(versions)} game(s) have ambiguous editions. " @@ -343,6 +388,6 @@ def run_review( session.console.print( f"{cfg.matches_path} is empty — run `bggpipe resolve` first." ) - return session + raise typer.Exit(code=1) session.run() return session diff --git a/src/bggpipe/templates/review.html b/src/bggpipe/templates/review.html index 5270adf..3e51916 100644 --- a/src/bggpipe/templates/review.html +++ b/src/bggpipe/templates/review.html @@ -65,6 +65,13 @@ } header kbd { background: rgba(255,255,255,.14); color: var(--paper); border-color: rgba(255,255,255,.25); } main { max-width: 62rem; margin: 0 auto; padding: 1.4rem 1.2rem 6rem; } + #banner { max-width: 62rem; margin: 0 auto; padding: 0 1.2rem; } + .banner { + border-radius: 6px; padding: .6rem .9rem; margin-top: .9rem; + font-size: .85rem; line-height: 1.4; + } + .banner.error { background: #f6dcd6; border: 1px solid var(--reject); color: #6b2417; } + .banner.warn { background: var(--kraft); border: 1px solid var(--brass-deep); color: #6b5b33; } h2 { color: var(--paper); font-family: "Iowan Old Style", Palatino, Georgia, serif; @@ -236,6 +243,7 @@ v veto merge · d dismiss +
diff --git a/src/bggpipe/upload.py b/src/bggpipe/upload.py index fabfcd5..2f1b09e 100644 --- a/src/bggpipe/upload.py +++ b/src/bggpipe/upload.py @@ -27,6 +27,7 @@ import os import random import re import time +from collections import Counter from collections.abc import Callable from dataclasses import dataclass from datetime import UTC, datetime @@ -35,7 +36,7 @@ from typing import Protocol import typer -from bggpipe.bgg_client import BGGAuthError, BGGClient +from bggpipe.bgg_client import BGGAuthError, BGGClient, client_for from bggpipe.config import Config from bggpipe.models import CollectionItem @@ -51,7 +52,7 @@ UPLOAD_LOG_COLUMNS = [ "timestamp", "error", ] -DONE_STATUSES = {"added", "updated", "already_present"} +DONE_STATUSES = {"added", "added_no_version", "updated", "already_present"} MAX_VERSION_PAGES = 40 @@ -73,7 +74,7 @@ class UploadJob: return ("add", self.bgg_id, self.version_id) -def _row_key(row: dict) -> tuple[str, str, str]: +def _job_key(row: dict) -> tuple[str, str, str]: if row["action"] == "update": return ("update", row["collid"], "") return ("add", row["bgg_id"], row["version_id"]) @@ -109,7 +110,7 @@ def build_queue( done. Returns (jobs, skipped_done, skipped_failed).""" latest: dict[tuple[str, str, str], str] = {} for row in log_rows: - latest[_row_key(row)] = row["status"] + latest[_job_key(row)] = row["status"] candidates = [ UploadJob( @@ -134,15 +135,25 @@ def build_queue( jobs: list[UploadJob] = [] skipped_done = skipped_failed = 0 + deferred: list[UploadJob] = [] + update_game_seen: set[str] = set() for job in candidates: status = latest.get(job.key) if status in DONE_STATUSES: skipped_done += 1 elif status == "failed" and not retry_failed: skipped_failed += 1 + elif job.action == "update" and job.bgg_id in update_game_seen: + # The row-edit flow finds rows by game name, not collid — a + # second same-game update this run could reopen the copy the + # first one just versioned and overwrite it. One per run; the + # next run (after --verify) picks up the rest. + deferred.append(job) else: + if job.action == "update": + update_game_seen.add(job.bgg_id) jobs.append(job) - return jobs, skipped_done, skipped_failed + return jobs, skipped_done, skipped_failed, deferred def _scrub(text: str) -> str: @@ -155,6 +166,12 @@ def _scrub(text: str) -> str: return text +class LoginError(RuntimeError): + """Authentication is broken (bad credentials, Cloudflare block, changed + login page). Systemic by definition: retrying per-game would hammer the + login endpoint and poison upload_log.csv with misleading failures.""" + + class Uploader(Protocol): def add_game(self, job: UploadJob) -> tuple[str, str]: ... @@ -215,11 +232,16 @@ class PlaywrightUploader: return page = self._page self._goto(f"{BGG}/") + if "just a moment" in (page.title() or "").casefold(): + raise LoginError( + "Cloudflare is challenging this browser ('Just a moment...') " + "— run headed (drop --headless) and click the widget once" + ) if self._signed_out(): user = os.environ.get("BGG_USERNAME", "") password = os.environ.get("BGG_PASSWORD", "") if not (user and password): - raise RuntimeError( + raise LoginError( "BGG_USERNAME and BGG_PASSWORD env vars are required to log in" ) self._goto(f"{BGG}/login") @@ -229,7 +251,13 @@ class PlaywrightUploader: page.locator("#inputUsername").fill(user) page.locator("#inputPassword").fill(password) page.get_by_role("button", name="Sign In").click() - page.wait_for_url(lambda url: "/login" not in url, timeout=120_000) + try: + page.wait_for_url(lambda url: "/login" not in url, timeout=120_000) + except self._timeout_error as err: + raise LoginError( + "login did not complete (wrong credentials, or the login " + "page changed — see docs/bgg-upload-flow.md)" + ) from err self._context.storage_state(path=str(self._storage_state)) self._authed = True @@ -266,6 +294,7 @@ class PlaywrightUploader: if nxt.count() == 0 or nxt.is_disabled(): break nxt.click() + self._page.wait_for_timeout(2_000) # etiquette: paginating hits BGG too # Two-level dismissal: the sub-view has its own Cancel distinct from # the main dialog's. dialog.get_by_role("button", name="Cancel").first.click() @@ -283,13 +312,14 @@ class PlaywrightUploader: "heading", name=re.compile(re.escape(job.name), re.I) ).wait_for(timeout=15_000) dialog.get_by_label("Own").check() - note = "" + status, note = "added", "" if job.version_name and not self._select_version(dialog, job.version_name): + status = "added_no_version" note = f"version {job.version_name!r} not in picker; added without version" dialog.get_by_role("button", name="Save").click() # The dialog is hidden after save, not removed from the DOM. dialog.wait_for(state="hidden", timeout=15_000) - return "added", note + return status, note def update_entry(self, job: UploadJob) -> tuple[str, str]: """Set the version on an EXISTING entry — strictly additive. @@ -332,6 +362,7 @@ def _process( now: Callable[[], str], ) -> list[dict]: results = [] + consecutive: tuple[str, int] = ("", 0) for i, job in enumerate(jobs): if i: sleep(rng.uniform(2.0, 4.0)) # polite pacing between games (spec) @@ -340,6 +371,12 @@ def _process( status, note = uploader.add_game(job) else: status, note = uploader.update_entry(job) + except LoginError as exc: + # Systemic: every remaining job would fail identically. Abort + # WITHOUT logging failures, so the next run just retries. + typer.echo(f" aborting — login is broken: {_scrub(str(exc))}") + typer.echo(f" {len(jobs) - i} job(s) left untouched for the next run.") + break except Exception as exc: # per-game isolation: log it, keep going status, note = "failed", _scrub(f"{type(exc).__name__}: {exc}") row = { @@ -350,12 +387,25 @@ def _process( "version_id": job.version_id, "status": status, "timestamp": now(), - "error": note if status == "failed" else note, + # the column carries degradation notes on successes too + # (e.g. added_no_version), not just failure text + "error": note, } append_log_row(log_path, row) results.append(row) suffix = f" — {note}" if note else "" typer.echo(f" {job.name}: {status}{suffix}") + if status == "failed": + consecutive = (note, consecutive[1] + 1 if note == consecutive[0] else 1) + if consecutive[1] >= 3: + typer.echo( + " aborting — 3 identical consecutive failures look " + "systemic, not per-game; remaining jobs left for the " + "next run" + ) + break + else: + consecutive = ("", 0) return results @@ -370,14 +420,18 @@ def verify_uploads(log_rows: list[dict], collection: list[CollectionItem]) -> li problems = [] latest: dict[tuple[str, str, str], dict] = {} for row in log_rows: - latest[_row_key(row)] = row + latest[_job_key(row)] = row for row in latest.values(): - if row["status"] == "added": + if row["status"] in ("added", "added_no_version"): copies = by_object.get(int(row["bgg_id"]), []) if not copies: problems.append(f"{row['name']}: logged added but not in collection") - elif row["version_id"] and not any( - str(c.version_id or "") == row["version_id"] for c in copies + elif ( + row["status"] == "added" # no_version: absence is expected + and row["version_id"] + and not any( + str(c.version_id or "") == row["version_id"] for c in copies + ) ): problems.append( f"{row['name']}: in collection but no copy has " @@ -419,11 +473,7 @@ def run_upload( # cache marker travels with the stub XML (gitignored, so a fresh clone # loses it), while data/STUB_DATA.marker is COMMITTED alongside the # stub-derived CSVs — so a clone can never upload placeholder ids. - markers = [ - cfg.cache_dir / "STUB_FIXTURES.marker", - cfg.data_dir / "STUB_DATA.marker", - ] - marker = next((m for m in markers if m.exists()), None) + marker = next((m for m in cfg.stub_marker_paths if m.exists()), None) if marker is not None: if not dry_run: typer.echo( @@ -439,12 +489,15 @@ def run_upload( "placeholders, not real BGG data.\n" ) - log_path = cfg.data_dir / "upload_log.csv" - to_add = _read_csv(cfg.data_dir / "to_add.csv") - to_update = _read_csv(cfg.data_dir / "to_update.csv") + log_path = cfg.upload_log_path + if not cfg.to_add_path.exists(): + typer.echo(f"{cfg.to_add_path} not found — run `bggpipe diff` first.") + raise typer.Exit(code=1) + to_add = _read_csv(cfg.to_add_path) + to_update = _read_csv(cfg.to_update_path) log_rows = _read_csv(log_path) - jobs, skipped_done, skipped_failed = build_queue( + jobs, skipped_done, skipped_failed, deferred = build_queue( to_add, to_update, log_rows, retry_failed=retry_failed ) if limit is not None: @@ -456,6 +509,12 @@ def run_upload( f" (skipping {skipped_done} already done, {skipped_failed} " "previously failed — use --retry-failed)" ) + if deferred: + typer.echo( + f"{len(deferred)} version update(s) deferred: one update per game " + "per run (the row-edit flow can't target a collid) — re-run " + "upload after --verify confirms this batch." + ) results: list[dict] = [] if dry_run: @@ -484,9 +543,7 @@ def run_upload( results = _process(real, jobs, log_path, sleep=sleep, rng=rng, now=now) else: results = _process(uploader, jobs, log_path, sleep=sleep, rng=rng, now=now) - counts: dict[str, int] = {} - for row in results: - counts[row["status"]] = counts.get(row["status"], 0) + 1 + counts = Counter(row["status"] for row in results) summary = " · ".join(f"{n} {status}" for status, n in sorted(counts.items())) typer.echo(f"\n{summary or 'nothing to do'}") else: @@ -501,7 +558,7 @@ def _run_verify(cfg: Config, client: BGGClient | None, log_path: Path) -> None: if not cfg.bgg_username: typer.echo("--verify needs BGG_USERNAME in the environment.") return - client = client or BGGClient(cfg.cache_dir, cfg.rate_limit_seconds) + client = client or client_for(cfg) try: collection = client.collection_full(cfg.bgg_username, refresh=True) except BGGAuthError as exc: diff --git a/src/bggpipe/webreview.py b/src/bggpipe/webreview.py index b1403e9..c0372bc 100644 --- a/src/bggpipe/webreview.py +++ b/src/bggpipe/webreview.py @@ -15,6 +15,8 @@ from __future__ import annotations import io import json import os +import threading +from collections import Counter from importlib import resources from pathlib import Path @@ -26,11 +28,10 @@ from pydantic import BaseModel from rich.console import Console from bggpipe.bgg_client import BGGClient +from bggpipe.config import DEFAULT_REVIEW_PORT as DEFAULT_PORT # single home from bggpipe.config import Config from bggpipe.review import ReviewSession -DEFAULT_PORT = 8377 - def load_thumbnails(cache_dir: Path) -> dict[int, str]: """bgg_id -> thumbnail URL, from cached /thing XML only (no live calls). @@ -41,7 +42,7 @@ def load_thumbnails(cache_dir: Path) -> dict[int, str]: for path in cache_dir.glob("thing_*.xml"): try: root = _safe_fromstring(path.read_text()) - except Exception: # noqa: BLE001 — a corrupt cache file must not kill the UI + except Exception: # a corrupt cache file must not kill the UI continue for item in root.findall("item"): thumb = (item.findtext("thumbnail") or "").strip() @@ -80,6 +81,7 @@ class DismissStore: class DecisionBody(BaseModel): title_raw: str source_photos: str + row_ix: int | None = None action: str # "pick" | "manual" | "reject" bgg_id: int | None = None @@ -87,6 +89,7 @@ class DecisionBody(BaseModel): class VersionBody(BaseModel): title_raw: str source_photos: str + row_ix: int | None = None action: str # "pick" | "unknown" version_id: int | None = None @@ -94,6 +97,7 @@ class VersionBody(BaseModel): class VetoBody(BaseModel): title_raw: str source_photos: str + row_ix: int | None = None class DismissBody(BaseModel): @@ -112,7 +116,11 @@ def create_app(cfg: Config, *, client: BGGClient | None = None) -> FastAPI: client=client, ) thumbnails = load_thumbnails(cfg.cache_dir) - dismissed = DismissStore(cfg.data_dir / "unidentified_dismissed.json") + dismissed = DismissStore(cfg.dismissed_path) + # FastAPI runs sync endpoints in a threadpool: without this, a polled + # GET /api/state can freshen()-swap session.rows out from under a + # concurrent decision POST, silently dropping the decision. + lock = threading.Lock() def freshen() -> None: """Serve every request from the current file state: an extract or @@ -121,8 +129,16 @@ def create_app(cfg: Config, *, client: BGGClient | None = None) -> FastAPI: if session.reload_if_changed(): thumbnails = load_thumbnails(cfg.cache_dir) - def find_row(title_raw: str, source_photos: str) -> dict: + def find_row( + title_raw: str, source_photos: str, row_ix: int | None = None + ) -> dict: freshen() + # ordinal first: (title_raw, source_photos) is not unique when one + # photo holds two editions of the same game + if row_ix is not None and 0 <= row_ix < len(session.rows): + row = session.rows[row_ix] + if row["title_raw"] == title_raw and row["source_photos"] == source_photos: + return row for row in session.rows: if row["title_raw"] == title_raw and row["source_photos"] == source_photos: return row @@ -134,12 +150,13 @@ def create_app(cfg: Config, *, client: BGGClient | None = None) -> FastAPI: return {p.name for p in cfg.photos_dir.iterdir() if p.is_file()} def row_payload(row: dict) -> dict: - entry = session.cues_for(row["title_raw"]) + entry = session.cues_for(row["title_raw"], row["source_photos"]) available = photo_names() candidates = json.loads(row["candidates_json"] or "[]") for c in candidates: c["thumbnail"] = thumbnails.get(c.get("bgg_id")) return { + "row_ix": session.rows.index(row), "title_raw": row["title_raw"], "source_photos": row["source_photos"], "match_status": row["match_status"], @@ -156,6 +173,7 @@ def create_app(cfg: Config, *, client: BGGClient | None = None) -> FastAPI: def version_payload(row: dict) -> dict: return { + "row_ix": session.rows.index(row), "title_raw": row["title_raw"], "source_photos": row["source_photos"], "bgg_name": row["bgg_name"], @@ -164,9 +182,7 @@ def create_app(cfg: Config, *, client: BGGClient | None = None) -> FastAPI: def state() -> dict: freshen() - counts: dict[str, int] = {} - for row in session.rows: - counts[row["match_status"]] = counts.get(row["match_status"], 0) + 1 + counts = Counter(row["match_status"] for row in session.rows) resolved_titles = {r["title_raw"] for r in session.rows} rows_by_title: dict[str, dict] = {} for r in session.rows: @@ -216,6 +232,7 @@ def create_app(cfg: Config, *, client: BGGClient | None = None) -> FastAPI: for r in session.merged_rows() ] return { + "warnings": session.warnings[-10:], "pending": [row_payload(r) for r in session.pending_rows()], "versions": [version_payload(r) for r in session.version_rows()], "merges": merges, @@ -240,11 +257,16 @@ def create_app(cfg: Config, *, client: BGGClient | None = None) -> FastAPI: @app.get("/api/state") def api_state() -> dict: - return state() + with lock: + return state() @app.post("/api/decision") def api_decision(body: DecisionBody) -> dict: - row = find_row(body.title_raw, body.source_photos) + with lock: + return _decide(body) + + def _decide(body: DecisionBody) -> dict: + row = find_row(body.title_raw, body.source_photos, body.row_ix) if body.action == "pick": candidates = json.loads(row["candidates_json"] or "[]") chosen = next( @@ -265,7 +287,11 @@ def create_app(cfg: Config, *, client: BGGClient | None = None) -> FastAPI: @app.post("/api/version") def api_version(body: VersionBody) -> dict: - row = find_row(body.title_raw, body.source_photos) + with lock: + return _version(body) + + def _version(body: VersionBody) -> dict: + row = find_row(body.title_raw, body.source_photos, body.row_ix) if body.action == "unknown": session.decide_version(row, None) elif body.action == "pick": @@ -279,19 +305,23 @@ def create_app(cfg: Config, *, client: BGGClient | None = None) -> FastAPI: @app.post("/api/veto-merge") def api_veto_merge(body: VetoBody) -> dict: - row = find_row(body.title_raw, body.source_photos) - if row["match_status"] != "merged": - raise HTTPException(400, "row is not merged") - session.veto_merge(row) - return state() + with lock: + row = find_row(body.title_raw, body.source_photos, body.row_ix) + if row["match_status"] != "merged": + raise HTTPException(400, "row is not merged") + session.veto_merge(row) + return state() @app.post("/api/dismiss") def api_dismiss(body: DismissBody) -> dict: - dismissed.add(_sighting_key(body.photo, body.model_dump(exclude={"photo"}))) - return state() + with lock: + dismissed.add( + _sighting_key(body.photo, body.model_dump(exclude={"photo"})) + ) + return state() @app.get("/photos/{name}") - def photo(name: str): + def photo(name: str) -> FileResponse: if name not in photo_names(): # also blocks any path traversal raise HTTPException(404, "no such photo") return FileResponse(cfg.photos_dir / name) @@ -303,7 +333,7 @@ def create_app(cfg: Config, *, client: BGGClient | None = None) -> FastAPI: } @app.get("/static/{name}") - def static_asset(name: str): + def static_asset(name: str) -> Response: media_type = _STATIC.get(name) # allowlist: no traversal possible if media_type is None: raise HTTPException(404, "no such asset") diff --git a/tests/test_client.py b/tests/test_client.py index 79ea3e3..54fb14c 100644 --- a/tests/test_client.py +++ b/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 XML; caching one would + # poison every future run for that query + from bggpipe.models import BGGResponseError + + errors_xml = "Invalid username" + 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 = ( + '' + '' + 'Catan' + '' + 'Advanced Civilization' + "" + ) + expansion_xml = ( + '' + '' + 'Advanced Civilization' + "" + ) + 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 = ( + '' + '' + 'Catan' + "" + ) + with pytest.raises(BGGResponseError): + parse_collection(bad_xml) diff --git a/tests/test_config.py b/tests/test_config.py index 0c70644..35aef79 100644 --- a/tests/test_config.py +++ b/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) diff --git a/tests/test_diff.py b/tests/test_diff.py index 7f4c28d..efa7ea1 100644 --- a/tests/test_diff.py +++ b/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") + ] diff --git a/tests/test_extract.py b/tests/test_extract.py index 271d63c..b4081de 100644 --- a/tests/test_extract.py +++ b/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( { diff --git a/tests/test_models.py b/tests/test_models.py index d32cefa..e08c2f1 100644 --- a/tests/test_models.py +++ b/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 ( diff --git a/tests/test_normalize.py b/tests/test_normalize.py index ccad912..eb57b17 100644 --- a/tests/test_normalize.py +++ b/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 diff --git a/tests/test_resolve.py b/tests/test_resolve.py index e68cdec..2d53866 100644 --- a/tests/test_resolve.py +++ b/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): "" ) 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") == [] diff --git a/tests/test_review.py b/tests/test_review.py index 4aa9bca..ce4be9f 100644 --- a/tests/test_review.py +++ b/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='') + ), + 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) diff --git a/tests/test_upload.py b/tests/test_upload.py index f113ba4..297afa2 100644 --- a/tests/test_upload.py +++ b/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() diff --git a/tests/test_webreview.py b/tests/test_webreview.py index f3c9fb3..a816adb 100644 --- a/tests/test_webreview.py +++ b/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"]