Re-audit round 4: 5 blind reviewers over the new surface — 24 fixes, +28 tests

The findings clustered exactly where prediction said: the unreviewed web
layer. The big ones: decisions made while an extract/resolve job runs
are now refused with a 409 (the job's end-of-run rewrite from a
start-of-run snapshot would silently revert them); a cross-origin guard
blocks preflight-free mutations from hostile webpages (bodyless run
triggers, cross-site photo form posts); the JobRunner sets terminal
status in a finally catching BaseException (a greenlet death could
wedge every future run behind 409s) and writes tracebacks into the
visible job log; and a boot token lets clients accept the revision
reset after a server restart instead of freezing forever.

Even the thrice-audited core yielded one HIGH: an unvetoed bare
typo-read sibling of a confident row duplicated its add when the game
wasn't in the collection — diff now treats it as satisfied. Second-copy
adds carry a flag through to_add.csv and the upload log so verify
honestly reports them unverifiable instead of OK. Also: merged_into
chains collapse transitively; diff/enrich treat a BGG queue timeout
like a missing token; enrich prunes orphaned games.json keys; the
wizard shell-quotes .env values and creates the file 0600 from the
first byte; fsio stats the tmp inode before replace and uses unique tmp
names; an explicit missing --config errors; storage state is
owner-only; extract re-extracts corrupt caches, aborts on 3 identical
failures, and exits nonzero when nothing succeeded; torn JSON artifacts
degrade with in-browser warnings instead of 500ing every page; photo
uploads are atomic with cache-invalidation ordered first; the pipeline
page computes `running` before the buttons that depend on it; the
photo dropzone alerts on network failure; and lost-contact banners
clear on recovery everywhere.

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

tests/test_config.py

@@ -8,8 +8,11 @@ from bggpipe.config import Config, load_config
 
 
 def test_defaults_when_no_file(tmp_path, monkeypatch):
+    # no config.toml in cwd and no explicit path -> defaults (an EXPLICIT
+    # missing path errors instead; see the dedicated test)
     monkeypatch.delenv("BGG_USERNAME", raising=False)
-    cfg = load_config(tmp_path / "missing.toml")
+    monkeypatch.chdir(tmp_path)
+    cfg = load_config()
     assert cfg == Config()
     assert cfg.cache_dir == Path("data/bgg_cache")
 
@@ -44,3 +47,10 @@ def test_unknown_toml_keys_warn(tmp_path, monkeypatch):
     p.write_text('photo_dir = "oops"\n')
     with pytest.warns(UserWarning, match="photo_dir"):
         load_config(p)
+
+
+def test_explicit_missing_config_errors_instead_of_silent_defaults(tmp_path):
+    import pytest
+
+    with pytest.raises(FileNotFoundError, match="does not exist"):
+        load_config(tmp_path / "nope.toml")

tests/test_diff.py

@@ -364,3 +364,36 @@ def test_live_diff_falls_back_to_snapshots_on_auth_failure(tmp_path, monkeypatch
     write_matches(cfg.matches_path, [_match("5 MINUTE DUNGEON", "207830")])
     result = run_diff(cfg, client=_LiveClient([], fail_auth=True))
     assert result.already_owned == ["5 MINUTE DUNGEON"]  # snapshots served
+
+
+def test_bare_sibling_of_confident_add_is_not_a_duplicate_upload():
+    # photo A reads "Wingspan" (confident version), photo B misreads
+    # "Wingspam" (bare) resolving to the same absent game: ONE add, not two
+    rows = [
+        _match("Wingspan", "266192", vstatus="version_auto", vid="1", vname="1st"),
+        _match("Wingspam", "266192"),
+    ]
+    result = compute_diff(rows, [])
+    assert len(result.to_add) == 1
+    assert result.already_owned == ["Wingspam"]
+
+
+def test_vetoed_bare_sibling_still_adds_for_absent_game():
+    vetoed = {**_match("Wingspan", "266192"), "dedupe_veto": "1"}
+    rows = [
+        _match("Wingspan", "266192", vstatus="version_auto", vid="1", vname="1st"),
+        vetoed,
+    ]
+    result = compute_diff(rows, [])
+    assert len(result.to_add) == 2
+    assert result.to_add[1]["second_copy"] == "1"
+
+
+def test_second_copy_flag_travels_on_exhausted_adds():
+    rows = [
+        _match("Catan", "13", vstatus="version_auto", vid="123", vname="3rd"),
+        _match("Catan", "13", vstatus="version_auto", vid="123", vname="3rd"),
+    ]
+    result = compute_diff(rows, [_item(13, 900, version_id=123)])
+    (added,) = result.to_add
+    assert added["second_copy"] == "1"

tests/test_extract.py

@@ -317,3 +317,35 @@ def test_low_confidence_reads_are_surfaced(tmp_path, capsys):
     assert "Low-confidence reads" in out
     assert "'Patchwork' (medium)" in out
     assert "'Catan'" not in out.split("Low-confidence reads")[1]
+
+
+def test_corrupt_raw_cache_is_reextracted_not_skipped(tmp_path):
+    cfg = _cfg(tmp_path)
+    cfg.photos_dir.mkdir()
+    _write_image(cfg.photos_dir / "shelf.jpg")
+    vision, calls = _vision_stub('[{"title_raw": "Catan", "confidence": "high"}]')
+    run_extract(cfg, vision=vision)
+    raw = cfg.extract_raw_dir / "shelf.jpg.json"
+    raw.write_text("{torn")  # corrupt the cache
+    run_extract(cfg, vision=vision)
+    assert len(calls) == 2  # re-extracted, not skipped
+    json.loads(raw.read_text())  # cache healed
+
+
+def test_systemic_failures_abort_and_exit_nonzero(tmp_path):
+    import pytest
+    import typer as _typer
+
+    calls = []
+
+    def broken_vision(image_b64, media_type):
+        calls.append(1)
+        raise RuntimeError("invalid x-api-key")
+
+    cfg = _cfg(tmp_path)
+    cfg.photos_dir.mkdir()
+    for i in range(6):
+        _write_image(cfg.photos_dir / f"p{i}.jpg")
+    with pytest.raises(_typer.Exit):
+        run_extract(cfg, vision=broken_vision)
+    assert len(calls) == 3  # aborted after 3 identical failures

tests/test_fsio.py

@@ -0,0 +1,49 @@
+"""fsio carries the project's central resumability promise: a kill or
+crash mid-write must never leave a torn file. These pin that promise
+directly — six modules depend on it."""
+
+from __future__ import annotations
+
+import os
+import stat
+
+import pytest
+
+from bggpipe.fsio import atomic_write_bytes, atomic_write_csv, atomic_write_text
+
+
+def test_failed_write_leaves_original_intact(tmp_path, monkeypatch):
+    target = tmp_path / "artifact.json"
+    target.write_text("precious")
+    # a read-only directory makes the tmp-file write fail
+    os.chmod(tmp_path, stat.S_IRUSR | stat.S_IXUSR)
+    try:
+        with pytest.raises(OSError):
+            atomic_write_text(target, "replacement")
+    finally:
+        os.chmod(tmp_path, 0o755)
+    assert target.read_text() == "precious"
+    assert not list(tmp_path.glob("*.tmp"))  # no leftovers
+
+
+def test_returned_mtime_matches_the_written_file(tmp_path):
+    target = tmp_path / "rows.csv"
+    mtime = atomic_write_csv(target, ["a", "b"], [{"a": "1", "b": "2"}])
+    # ReviewSession records this as "my own write" — it must be the mtime
+    # the file actually carries, or external-change detection breaks
+    assert mtime == target.stat().st_mtime_ns
+
+
+def test_bytes_variant_round_trips(tmp_path):
+    target = tmp_path / "photo.jpg"
+    atomic_write_bytes(target, b"\xff\xd8jpeg")
+    assert target.read_bytes() == b"\xff\xd8jpeg"
+    atomic_write_bytes(target, b"\xff\xd8jpeg2")  # overwrite is atomic too
+    assert target.read_bytes() == b"\xff\xd8jpeg2"
+
+
+def test_concurrent_writers_use_distinct_tmp_names(tmp_path):
+    from bggpipe.fsio import _tmp_for
+
+    target = tmp_path / "x.csv"
+    assert _tmp_for(target) != _tmp_for(target)  # no shared-inode interleave

tests/test_init.py

@@ -43,8 +43,8 @@ def test_creates_dirs_config_and_env_from_nothing(tmp_path, monkeypatch):
     assert (tmp_path / "photos").is_dir() and (tmp_path / "data").is_dir()
     assert (tmp_path / "config.toml").exists()
     env = (tmp_path / ".env").read_text()
-    assert "ANTHROPIC_API_KEY=sk-test-123" in env
-    assert "BGG_USERNAME=eric" in env
+    assert "ANTHROPIC_API_KEY='sk-test-123'" in env
+    assert "BGG_USERNAME='eric'" in env
     assert report.keys_written == ["ANTHROPIC_API_KEY", "BGG_USERNAME"]
     assert set(report.keys_missing) == {"BGG_PASSWORD", "BGG_API_TOKEN"}
 
@@ -117,3 +117,33 @@ def test_secret_values_never_appear_in_output(tmp_path, monkeypatch, capsys):
     _clear_env(monkeypatch)
     _run(tmp_path, answers={"BGG_PASSWORD": "s3cret-value-xyz"})
     assert "s3cret-value-xyz" not in capsys.readouterr().out
+
+
+def test_values_are_shell_quoted_for_source(tmp_path, monkeypatch):
+    # a password with spaces, $, and quotes must survive `source .env`
+    _clear_env(monkeypatch)
+    _run(tmp_path, answers={"BGG_PASSWORD": "pa$s wo'rd"})
+    env = (tmp_path / ".env").read_text()
+    assert "BGG_PASSWORD='pa$s wo'\\''rd'" in env
+
+
+def test_env_parsing_negatives(tmp_path, monkeypatch):
+    # commented, quoted-empty, and export-prefixed lines must parse sanely
+    _clear_env(monkeypatch)
+    (tmp_path / ".env").write_text(
+        "# BGG_PASSWORD=commented-out\n"
+        'ANTHROPIC_API_KEY=""\n'
+        "export BGG_USERNAME='eric'\n"
+    )
+    report = _run(tmp_path)
+    assert "BGG_USERNAME" in report.keys_ready  # export form recognized
+    assert "ANTHROPIC_API_KEY" in report.keys_missing  # quoted-empty ≠ set
+    assert "BGG_PASSWORD" in report.keys_missing  # comments don't count
+
+
+def test_env_file_is_owner_only_from_creation(tmp_path, monkeypatch):
+    import os as _os
+
+    _clear_env(monkeypatch)
+    _run(tmp_path, answers={"BGG_API_TOKEN": "tok-123"})
+    assert _os.stat(tmp_path / ".env").st_mode & 0o777 == 0o600

tests/test_resolve.py

@@ -660,3 +660,15 @@ def test_reordered_titles_json_cannot_mispair_editions(client, tmp_path):
     assert by_photos["b.jpg"]["match_status"] == "auto"  # kept its resolution
     assert "a.jpg" in by_photos  # newcomer resolved as its own row
     assert len(rows) == 2
+
+
+def test_merged_into_chains_resolve_to_terminal_survivor():
+    # X merged into Y in a prior run; this run merges Y into W — X must
+    # point at W, or diff's one-level photo hop loses X's provenance
+    x = _mrow("Wingspam", "266192", "x.jpg", status="merged")
+    x["merged_into"] = "Wingspan Typo"
+    y = _mrow("Wingspan Typo", "266192", "y.jpg", name="Wingspan")
+    w = _mrow("Wingspan", "266192", "w.jpg", name="Wingspan")
+    dedupe_matches([x, y, w], [])
+    assert y["match_status"] == "merged" and y["merged_into"] == "Wingspan"
+    assert x["merged_into"] == "Wingspan"  # chain collapsed

tests/test_upload.py

@@ -466,3 +466,28 @@ def test_run_upload_verify_wiring(tmp_path, capsys):
     )
     assert client.calls == [{"username": "tester", "refresh": True}]
     assert "Verification OK" in capsys.readouterr().out
+
+
+def test_verify_marks_second_copy_adds_unverifiable():
+    log = [
+        {**_log_row(action="add", bgg_id="13", status="added"), "second_copy": "1"},
+    ]
+    problems = verify_uploads(log, [_item(13, 900, version_id=7)])
+    (problem,) = problems
+    assert "can't be verified" in problem and "confirm by eye" in problem
+
+
+def test_drift_warning_suppressed_for_multi_edition_partial_run(tmp_path, capsys):
+    # run 1 added edition A; edition B is still queued alongside A's row:
+    # that's a two-edition game mid-way, not a re-review drift
+    cfg = _cfg(tmp_path)
+    _seed_data(
+        tmp_path,
+        to_add=[
+            _add_row(bgg_id="13", name="Catan", version_id="1", version_name="A"),
+            _add_row(bgg_id="13", name="Catan", version_id="2", version_name="B"),
+        ],
+        log=[_log_row(action="add", bgg_id="13", version_id="1", status="added")],
+    )
+    run_upload(cfg, uploader=FakeUploader(), sleep=lambda s: None, now=_now)
+    assert "manual correction" not in capsys.readouterr().out

tests/test_web_dashboard.py

@@ -4,6 +4,7 @@ stage functions are injected so nothing slow or networked ever runs."""
 from __future__ import annotations
 
 import threading
+import time
 
 import typer
 from fastapi.testclient import TestClient
@@ -315,3 +316,308 @@ def test_non_photo_files_are_invisible(tmp_path):
     web = _app(cfg)
     assert [p["name"] for p in web.get("/api/photos-list").json()] == ["shelf.jpg"]
     assert web.get("/photos/.DS_Store").status_code == 404
+
+
+# -- jobs: live-progress and termination contracts ----------------------
+
+
+def test_running_snapshot_shows_partial_line_then_finishes():
+    import typer as _typer
+
+    runner = JobRunner()
+    release = threading.Event()
+
+    def stage():
+        print("progress: 40%", end="", flush=True)  # no newline yet
+        release.wait()
+        _typer.echo(" done")
+
+    runner.start("extract", stage)
+    for _ in range(100):
+        snap = runner.snapshot()
+        if snap["log"]:
+            break
+        time.sleep(0.01)
+    assert snap["status"] == "running"
+    assert snap["log"] == ["progress: 40%"]
+    release.set()
+    runner.wait()
+    assert runner.snapshot()["log"] == ["progress: 40% done"]
+
+
+def test_log_serves_last_200_lines_and_buffer_is_bounded():
+    import typer as _typer
+
+    from bggpipe.jobs import MAX_LOG_LINES
+
+    runner = JobRunner()
+
+    def stage():
+        for i in range(MAX_LOG_LINES + 300):
+            _typer.echo(f"line {i}")
+
+    runner.start("extract", stage)
+    runner.wait()
+    log = runner.snapshot()["log"]
+    assert len(log) == 200
+    assert log[-1] == f"line {MAX_LOG_LINES + 299}"
+
+
+def test_zero_exit_codes_count_as_done():
+    import typer as _typer
+
+    for exc in (_typer.Exit(), SystemExit(0)):
+        runner = JobRunner()
+        runner.start("diff", lambda exc=exc: (_ for _ in ()).throw(exc))
+        runner.wait()
+        assert runner.snapshot()["status"] == "done"
+
+
+def test_nonzero_systemexit_is_failed():
+    runner = JobRunner()
+    runner.start("diff", lambda: (_ for _ in ()).throw(SystemExit(2)))
+    runner.wait()
+    snap = runner.snapshot()
+    assert snap["status"] == "failed" and "2" in snap["error"]
+
+
+def test_base_exception_cannot_wedge_the_runner():
+    class Rude(BaseException):
+        pass
+
+    runner = JobRunner()
+    runner.start("upload", lambda: (_ for _ in ()).throw(Rude("greenlet died")))
+    runner.wait()
+    snap = runner.snapshot()
+    assert snap["status"] == "failed"
+    assert "Rude" in snap["error"]
+    assert any("Traceback" in line for line in snap["log"])  # diagnosable
+    assert runner.start("extract", lambda: None)  # slot is free again
+    runner.wait()
+
+
+# -- concurrency and restart contracts ----------------------------------
+
+
+def test_revision_bumps_on_mutations_not_reads(tmp_path):
+    from bggpipe.resolve import write_matches
+
+    cfg = _cfg(tmp_path)
+    write_matches(
+        cfg.matches_path,
+        [
+            {
+                "title_raw": "Mystery",
+                "bgg_id": "",
+                "bgg_name": "",
+                "year": "",
+                "type": "",
+                "match_status": "unmatched",
+                "version_id": "",
+                "version_name": "",
+                "version_status": "",
+                "candidates_json": "[]",
+                "version_candidates_json": "[]",
+                "source_photos": "x.jpg",
+            }
+        ],
+    )
+    web = _app(cfg)
+    first = web.get("/api/state").json()
+    assert "boot" in first  # clients detect restarts by boot change
+    second = web.get("/api/state").json()
+    assert second["revision"] == first["revision"]  # reads never bump
+
+    res = web.post(
+        "/api/decision",
+        json={"title_raw": "Mystery", "source_photos": "x.jpg", "action": "reject"},
+    )
+    assert res.json()["revision"] > first["revision"]  # mutations bump
+
+    # external rewrite bumps on the next read
+    rows = []
+    write_matches(cfg.matches_path, rows)
+    bumped = web.get("/api/state").json()
+    assert bumped["revision"] > res.json()["revision"]
+
+
+def test_torn_data_files_degrade_with_warnings_not_500(tmp_path):
+    cfg = _cfg(tmp_path)
+    cfg.unidentified_path.write_text("{torn")
+    cfg.games_path.write_text("[not even close")
+    web = _app(cfg)
+    state = web.get("/api/state")
+    pipeline = web.get("/api/pipeline")
+    library = web.get("/api/library")
+    assert state.status_code == pipeline.status_code == library.status_code == 200
+    assert library.json() == []
+    assert any("unreadable" in w for w in state.json()["warnings"])
+
+
+def test_decisions_locked_out_while_matches_rewriting_job_runs(tmp_path):
+    from bggpipe.resolve import read_matches, write_matches
+
+    cfg = _cfg(tmp_path)
+    write_matches(
+        cfg.matches_path,
+        [
+            {
+                "title_raw": "Mystery",
+                "bgg_id": "",
+                "bgg_name": "",
+                "year": "",
+                "type": "",
+                "match_status": "unmatched",
+                "version_id": "",
+                "version_name": "",
+                "version_status": "",
+                "candidates_json": "[]",
+                "version_candidates_json": "[]",
+                "source_photos": "x.jpg",
+            }
+        ],
+    )
+    release = threading.Event()
+    jobs = JobRunner()
+    web = _app(cfg, stages={"resolve": release.wait}, jobs=jobs)
+    web.post("/api/run/resolve")
+    res = web.post(
+        "/api/decision",
+        json={"title_raw": "Mystery", "source_photos": "x.jpg", "action": "reject"},
+    )
+    assert res.status_code == 409
+    assert "resolve run is rewriting" in res.json()["detail"]
+    # the decision was NOT applied
+    assert read_matches(cfg.matches_path)[0]["match_status"] == "unmatched"
+    release.set()
+    jobs.wait()
+
+
+def test_cross_origin_mutations_are_refused(tmp_path):
+    web = _app(_cfg(tmp_path))
+    # foreign Origin on a same-host request: refused
+    res = web.post("/api/run/extract", headers={"origin": "https://evil.example"})
+    assert res.status_code == 403
+    # foreign Host (DNS rebinding): refused
+    res = web.post(
+        "/api/photos",
+        headers={"host": "evil.example"},
+        files={"files": ("a.jpg", b"x", "image/jpeg")},
+    )
+    assert res.status_code == 403
+    # reads are unaffected
+    assert (
+        web.get("/api/pipeline", headers={"origin": "https://evil.example"}).status_code
+        == 200
+    )
+
+
+def test_concurrent_decisions_and_polls_never_lose_a_decision(tmp_path):
+    from bggpipe.resolve import read_matches, write_matches
+
+    cfg = _cfg(tmp_path)
+    base = {
+        "bgg_id": "",
+        "bgg_name": "",
+        "year": "",
+        "type": "",
+        "match_status": "unmatched",
+        "version_id": "",
+        "version_name": "",
+        "version_status": "",
+        "candidates_json": "[]",
+        "version_candidates_json": "[]",
+        "source_photos": "x.jpg",
+    }
+    rows = [{**base, "title_raw": f"Game{i}"} for i in range(8)]
+    write_matches(cfg.matches_path, rows)
+    web = _app(cfg)
+
+    def decide(i):
+        web.post(
+            "/api/decision",
+            json={
+                "title_raw": f"Game{i}",
+                "source_photos": "x.jpg",
+                "action": "reject",
+            },
+        )
+
+    def poll():
+        web.get("/api/state")
+
+    threads = [threading.Thread(target=decide, args=(i,)) for i in range(8)]
+    threads += [threading.Thread(target=poll) for _ in range(8)]
+    for t in threads:
+        t.start()
+    for t in threads:
+        t.join()
+    saved = read_matches(cfg.matches_path)
+    assert all(r["match_status"] == "rejected" for r in saved)
+
+
+def test_decision_for_vanished_row_is_404_and_touches_nothing(tmp_path):
+    from bggpipe.resolve import read_matches, write_matches
+
+    cfg = _cfg(tmp_path)
+    row = {
+        "title_raw": "Kept",
+        "bgg_id": "",
+        "bgg_name": "",
+        "year": "",
+        "type": "",
+        "match_status": "unmatched",
+        "version_id": "",
+        "version_name": "",
+        "version_status": "",
+        "candidates_json": "[]",
+        "version_candidates_json": "[]",
+        "source_photos": "x.jpg",
+    }
+    write_matches(cfg.matches_path, [row])
+    web = _app(cfg)
+    res = web.post(
+        "/api/decision",
+        json={"title_raw": "Gone", "source_photos": "x.jpg", "action": "reject"},
+    )
+    assert res.status_code == 404
+    assert read_matches(cfg.matches_path)[0]["match_status"] == "unmatched"
+
+
+def test_pipeline_reports_badge_fields(tmp_path):
+    from bggpipe.resolve import write_matches
+
+    cfg = _cfg(tmp_path)
+    base = {
+        "bgg_id": "13",
+        "bgg_name": "Catan",
+        "year": "",
+        "type": "boardgame",
+        "version_id": "",
+        "version_name": "",
+        "candidates_json": "[]",
+        "version_candidates_json": "[]",
+        "source_photos": "x.jpg",
+    }
+    write_matches(
+        cfg.matches_path,
+        [
+            {
+                **base,
+                "title_raw": "A",
+                "match_status": "ambiguous",
+                "version_status": "",
+            },
+            {
+                **base,
+                "title_raw": "B",
+                "match_status": "auto",
+                "version_status": "version_ambiguous",
+                "version_id": "1",
+            },
+        ],
+    )
+    cfg.to_add_path.write_text("bgg_id,bgg_name\n1,X\n2,Y\n")
+    p = _app(cfg).get("/api/pipeline").json()
+    assert p["pending_review"] == 2  # one match + one edition decision
+    assert p["to_add"] == 2  # header excluded