Audit round 8: the export earns its publishing promises
Five blind reviewers over the day-old export stage; ~30 findings verified, the big ones sharing one root — a static-site generator makes promises a pipeline stage doesn't, and the first cut kept none of them. URL stability was empirically false two ways: adding an edition whose key sorted first STOLE the base slug (every colliding URL reshuffled), and removing the base holder renumbered survivors over the stale pages' corpses — wrong content at live URLs, not even 404s. And nothing ever deleted anything: removed games stayed published forever. One mechanism fixes all of it — a manifest (.bggpipe-export.json) in the output directory records which slugs the export owns and which source URL produced each cover. Slugs persist across runs (a published URL never moves and can never be stolen), stale pages are removed (only ever manifest-claimed ones — user files are not ours to touch), replaced box art re-fetches when its URL changes, and "art" is a reserved name so a game called Art can't move into the asset dir. Trust-the-network fixes: a 200 response must LOOK like an image (magic bytes + size) before it's cached, else a CDN interstitial became a permanent "cover" that re-runs skipped forever; downloads go through fsio.atomic_write_bytes instead of a hand-rolled fixed-tmp-name dance (the exact hazard fsio's own docstring warns about); a missing hand-added cover counts as a failure instead of silently shipping coverless; the badge file is sniffed too; CDN pacing raised to 1s and written into the spec as an adjudicated carve-out rather than a code comment's private opinion. Ship-shape: pages write atomically with the index LAST (a killed run can't publish links to pages that don't exist); the CLI exits nonzero on failures so `export && rsync` can't publish an incomplete site; footer/fine-print contrast now clears WCAG AA on the sky background; meta description, og:title/og:image and a favicon stop bare unfurls; the BGG link moved out of the h1; the noart tile is aria-hidden; the search box gained a no-matches message; numeric fields from enrich render instead of crashing the join; years and ids are escaped; the players/playtime formatters are aligned with their JS twins and both sides carry keep-in-sync constraint comments; export moved after enrich in the CLI listing. Twelve export tests now, including the previously-vacuous atomicity test rebuilt to actually interrupt a write. One honest loose end: one full cover re-fetch occurred during rollout that the identical naming code can't explain; the manifest's URL records make any recurrence diagnosable. 352 tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016jXZFSTZQKzAC8fqpWSz9g
This commit is contained in:
co-authored by
Claude Fable 5
parent
f291b9c190
commit
c7fdc60f87
+173
-7
@@ -60,14 +60,17 @@ def _cfg(tmp_path: Path) -> Config:
|
||||
cfg.data_dir.mkdir(parents=True)
|
||||
cfg.games_path.write_text(json.dumps(GAMES))
|
||||
cfg.local_art_dir.mkdir()
|
||||
(cfg.local_art_dir / "abc123.jpg").write_bytes(b"\xff\xd8local")
|
||||
(cfg.local_art_dir / "abc123.jpg").write_bytes(FAKE_JPEG)
|
||||
return cfg
|
||||
|
||||
|
||||
FAKE_JPEG = b"\xff\xd8" + b"\x00" * 100 # magic + enough body to pass sniffing
|
||||
|
||||
|
||||
def _cdn(requests_log: list[str]) -> httpx.Client:
|
||||
def handler(request: httpx.Request) -> httpx.Response:
|
||||
requests_log.append(str(request.url))
|
||||
return httpx.Response(200, content=b"\xff\xd8fake-image")
|
||||
return httpx.Response(200, content=FAKE_JPEG)
|
||||
|
||||
return httpx.Client(transport=httpx.MockTransport(handler))
|
||||
|
||||
@@ -79,7 +82,7 @@ def test_export_writes_pages_art_and_no_shelf_photos(tmp_path):
|
||||
summary = run_export(
|
||||
cfg, out, title="Test Shelves", fetch=_cdn(log), sleep=lambda s: None
|
||||
)
|
||||
assert summary == {"games": 3, "downloaded": 3, "failures": 0}
|
||||
assert summary == {"games": 3, "downloaded": 3, "failures": 0, "removed": 0}
|
||||
|
||||
index = (out / "index.html").read_text()
|
||||
assert "Test Shelves" in index and "Powered by BGG" in index
|
||||
@@ -131,7 +134,7 @@ def test_failed_cover_reports_and_ships_page_anyway(tmp_path):
|
||||
def flaky(request: httpx.Request) -> httpx.Response:
|
||||
if "pic2" in str(request.url):
|
||||
return httpx.Response(503)
|
||||
return httpx.Response(200, content=b"img")
|
||||
return httpx.Response(200, content=FAKE_JPEG)
|
||||
|
||||
summary = run_export(
|
||||
cfg,
|
||||
@@ -148,14 +151,14 @@ def test_failed_cover_reports_and_ships_page_anyway(tmp_path):
|
||||
|
||||
def test_official_badge_upgrades_the_footer(tmp_path):
|
||||
cfg = _cfg(tmp_path)
|
||||
(cfg.data_dir / "powered-by-bgg.png").write_bytes(b"\x89PNG-badge")
|
||||
(cfg.data_dir / "powered-by-bgg.png").write_bytes(b"\x89PNG" + b"\x00" * 100)
|
||||
out = tmp_path / "site"
|
||||
run_export(cfg, out, fetch=_cdn([]), sleep=lambda s: None)
|
||||
assert (out / "art" / "powered-by-bgg.png").exists()
|
||||
index = (out / "index.html").read_text()
|
||||
assert 'img src="art/powered-by-bgg.png" alt="Powered by BGG"' in index
|
||||
assert "art/powered-by-bgg.png" in index and "Powered by BGG" in index
|
||||
detail = (out / "britannia" / "index.html").read_text()
|
||||
assert 'img src="../art/powered-by-bgg.png"' in detail
|
||||
assert "../art/powered-by-bgg.png" in detail
|
||||
|
||||
|
||||
def test_export_without_games_json_exits_with_guidance(tmp_path):
|
||||
@@ -164,3 +167,166 @@ def test_export_without_games_json_exits_with_guidance(tmp_path):
|
||||
|
||||
with pytest.raises(typer.Exit):
|
||||
run_export(cfg, tmp_path / "site", fetch=_cdn([]), sleep=lambda s: None)
|
||||
|
||||
|
||||
def test_urls_survive_adding_and_removing_games(tmp_path):
|
||||
"""The manifest's whole point: a published URL never moves and is
|
||||
never stolen — not by an earlier-sorting new edition, not by the
|
||||
base-slug holder leaving."""
|
||||
cfg = _cfg(tmp_path)
|
||||
out = tmp_path / "site"
|
||||
run_export(cfg, out, fetch=_cdn([]), sleep=lambda s: None)
|
||||
original = (out / "britannia" / "index.html").read_text()
|
||||
assert "Avalon Hill second edition" in original
|
||||
|
||||
# a new same-named edition whose key sorts FIRST must not steal slugs
|
||||
games = json.loads(cfg.games_path.read_text())
|
||||
games["100:1"] = {"bgg_id": 100, "type": "boardgame", "name": "Britannia"}
|
||||
cfg.games_path.write_text(json.dumps(games))
|
||||
run_export(cfg, out, fetch=_cdn([]), sleep=lambda s: None)
|
||||
assert (
|
||||
"Avalon Hill second edition" in (out / "britannia" / "index.html").read_text()
|
||||
)
|
||||
assert (out / "britannia-3" / "index.html").exists() # the newcomer
|
||||
|
||||
# removing the base-slug holder must not renumber the survivors —
|
||||
# and must actually remove the stale page
|
||||
del games["240:24621"]
|
||||
cfg.games_path.write_text(json.dumps(games))
|
||||
summary = run_export(cfg, out, fetch=_cdn([]), sleep=lambda s: None)
|
||||
assert summary["removed"] == 1
|
||||
assert not (out / "britannia" / "index.html").exists()
|
||||
assert (out / "britannia-2" / "index.html").exists() # localgame, unmoved
|
||||
index = (out / "index.html").read_text()
|
||||
assert "britannia/" not in index.replace("britannia-2/", "").replace(
|
||||
"britannia-3/", ""
|
||||
)
|
||||
|
||||
|
||||
def test_changed_cover_url_is_refetched(tmp_path):
|
||||
"""BGG replaces box art under the same extension: the manifest records
|
||||
each cover's source URL so a change re-downloads."""
|
||||
cfg = _cfg(tmp_path)
|
||||
out = tmp_path / "site"
|
||||
run_export(cfg, out, fetch=_cdn([]), sleep=lambda s: None)
|
||||
|
||||
games = json.loads(cfg.games_path.read_text())
|
||||
games["240:24621"]["image"] = "https://cf.geekdo-images.com/orig/pic9.jpg"
|
||||
cfg.games_path.write_text(json.dumps(games))
|
||||
log: list[str] = []
|
||||
summary = run_export(cfg, out, fetch=_cdn(log), sleep=lambda s: None)
|
||||
assert summary["downloaded"] == 1
|
||||
assert any("pic9" in u for u in log)
|
||||
|
||||
|
||||
def test_garbage_200_is_a_failure_not_a_cached_cover(tmp_path):
|
||||
"""A CDN interstitial served with 200 must not be cached as a cover:
|
||||
exists() would then skip it forever."""
|
||||
cfg = _cfg(tmp_path)
|
||||
|
||||
def interstitial(request: httpx.Request) -> httpx.Response:
|
||||
return httpx.Response(200, content=b"<html>challenge page</html>" * 10)
|
||||
|
||||
summary = run_export(
|
||||
cfg,
|
||||
tmp_path / "site",
|
||||
fetch=httpx.Client(transport=httpx.MockTransport(interstitial)),
|
||||
sleep=lambda s: None,
|
||||
)
|
||||
assert summary["failures"] == 3
|
||||
assert summary["downloaded"] == 0
|
||||
# only the local-art copy survives; no CDN garbage was cached
|
||||
names = {f.name for f in (tmp_path / "site" / "art").iterdir()}
|
||||
assert names == {"abc123.jpg"}
|
||||
|
||||
|
||||
def test_missing_local_art_is_a_counted_failure(tmp_path):
|
||||
cfg = _cfg(tmp_path)
|
||||
(cfg.local_art_dir / "abc123.jpg").unlink()
|
||||
summary = run_export(cfg, tmp_path / "site", fetch=_cdn([]), sleep=lambda s: None)
|
||||
assert summary["failures"] == 1 # local_art is the ONLY source: say so
|
||||
|
||||
|
||||
def test_interrupted_download_leaves_no_cover_and_recovers(tmp_path, monkeypatch):
|
||||
"""The non-vacuous atomicity test: die BETWEEN response and write,
|
||||
assert nothing half-made survives and the re-run completes."""
|
||||
import bggpipe.export as export_mod
|
||||
|
||||
cfg = _cfg(tmp_path)
|
||||
out = tmp_path / "site"
|
||||
real_write = export_mod.atomic_write_bytes
|
||||
calls = {"n": 0}
|
||||
|
||||
def die_on_second(path, data):
|
||||
calls["n"] += 1
|
||||
if calls["n"] == 2:
|
||||
raise KeyboardInterrupt
|
||||
real_write(path, data)
|
||||
|
||||
monkeypatch.setattr(export_mod, "atomic_write_bytes", die_on_second)
|
||||
with pytest.raises(KeyboardInterrupt):
|
||||
run_export(cfg, out, fetch=_cdn([]), sleep=lambda s: None)
|
||||
assert len(list((out / "art").glob("*.part"))) == 0
|
||||
|
||||
monkeypatch.setattr(export_mod, "atomic_write_bytes", real_write)
|
||||
summary = run_export(cfg, out, fetch=_cdn([]), sleep=lambda s: None)
|
||||
assert summary["failures"] == 0
|
||||
assert (out / "index.html").exists()
|
||||
|
||||
|
||||
def test_minimal_and_numeric_entries_render(tmp_path):
|
||||
"""Entries vary wildly: a name-only localgame and enrich-shaped
|
||||
numerics (int best counts, int year) must render, not crash."""
|
||||
cfg = Config(data_dir=tmp_path / "data")
|
||||
cfg.data_dir.mkdir(parents=True)
|
||||
cfg.games_path.write_text(
|
||||
json.dumps(
|
||||
{
|
||||
"local:mystery:x.jpg": {"type": "localgame", "name": "Mystery"},
|
||||
"7": {
|
||||
"bgg_id": 7,
|
||||
"type": "boardgame",
|
||||
"name": "Numeric",
|
||||
"year": 1980,
|
||||
"min_players": 1,
|
||||
"max_players": 1,
|
||||
"best_player_counts": [1],
|
||||
"weight": 2,
|
||||
"rating": 7,
|
||||
},
|
||||
}
|
||||
)
|
||||
)
|
||||
out = tmp_path / "site"
|
||||
run_export(cfg, out, fetch=_cdn([]), sleep=lambda s: None)
|
||||
numeric = (out / "numeric" / "index.html").read_text()
|
||||
assert "1 player" in numeric and "best at 1" in numeric
|
||||
assert (out / "mystery" / "index.html").exists()
|
||||
|
||||
|
||||
def test_cli_export_wiring(tmp_path, monkeypatch):
|
||||
from typer.testing import CliRunner
|
||||
|
||||
from bggpipe.cli import app
|
||||
|
||||
received = {}
|
||||
|
||||
def fake_run_export(cfg, out, *, title):
|
||||
received.update(out=out, title=title)
|
||||
return {"games": 0, "downloaded": 0, "failures": 0, "removed": 0}
|
||||
|
||||
monkeypatch.setattr("bggpipe.export.run_export", fake_run_export)
|
||||
runner = CliRunner()
|
||||
result = runner.invoke(
|
||||
app, ["export", "--out", str(tmp_path / "s"), "--title", "T"]
|
||||
)
|
||||
assert result.exit_code == 0
|
||||
assert received["title"] == "T" and received["out"] == tmp_path / "s"
|
||||
assert runner.invoke(app, ["export"]).exit_code != 0 # --out is required
|
||||
|
||||
def failing_run_export(cfg, out, *, title):
|
||||
return {"games": 1, "downloaded": 0, "failures": 2, "removed": 0}
|
||||
|
||||
monkeypatch.setattr("bggpipe.export.run_export", failing_run_export)
|
||||
result = runner.invoke(app, ["export", "--out", str(tmp_path / "s")])
|
||||
assert result.exit_code == 1 # scripted publishes must see failures
|
||||
|
||||
Reference in New Issue
Block a user