Audit round 7, web + stages cluster: 13 more verified findings fixed
The web layer's serialization story had three gaps: /api/run started a stage without the lock, so a decision mid-save could pass the rewrite guard and still be clobbered by the stage's full rewrite (now the start itself serializes); /api/photos accepted a replacement photo while extract was running, permanently pairing the new bytes with the old photo's reads (now refuses like every other mutation); and /api/queue read session rows lock-free and stale (now freshens under the lock). The localhost Host allowlist applied only to writes — a DNS-rebound page could read pipeline state and shelf photos with plain GETs; it now covers all methods (foreign-Origin reads still pass: without CORS headers a cross-origin page can't read the response anyway). Data-loss finds: the off-BGG edit form re-rendered from games.json, which only sees hand data after enrich — so a second save resubmitted pre-save blanks and cleared the first (the detail endpoint now overlays local_games.json live). The local key embeds the photo list, so a new sighting orphaned hand-written facts silently; enrich now migrates them when the title still matches exactly one line, and warns instead of ever dropping. research() left the previous game's version verdicts on the row, riding a stale version_id onto the next pick; it clears all four fields as reopen does. find_row now prefers the version-open sibling on duplicate keys, mirroring _adopt. Re-adding a removed hand-added title silently no-opped behind a 200 — it now rescinds the removal (an explicit undo), and a true duplicate add answers 409. Smaller: parse_search's dedupe collapsed same-id rows under DIFFERENT names, discarding the alternate-name row whose exact match downstream scoring needed (now collapses same-name only; research merges its ballot per game preferring exact evidence); rpgitems rank in their own family so their rank parsed null; the pipeline badge counted review-retired queue rows as pending; the catalog pairing cascade ran per-entry so a tier-3 claim could steal a sibling's exact row (now tier-by-tier across all entries, as resolve does); library cards render a lone player bound without "undefined" and the seats filter tolerates it; added_no_version reads "done · no version" instead of a bare green done. Every finding verified against the code before fixing; each fix carries a regression test. 337 tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016jXZFSTZQKzAC8fqpWSz9g
This commit is contained in:
Eric Wagoner
co-authored by
Claude Fable 5
Claude Fable 5
parent
32b6aae841
commit
e6d45011cc
@@ -173,3 +173,18 @@ def test_parse_things_full_reads_rpggeek_link_vocabulary():
|
||||
assert game["categories"] == ["Modern", "Core Rules (min needed to play)"]
|
||||
assert game["mechanics"] == ["Card Play"]
|
||||
assert game["producers"] == ["Someone"]
|
||||
|
||||
|
||||
def test_parse_search_keeps_alternate_name_duplicates():
|
||||
"""The same id listed under two NAMES is evidence, not noise — the
|
||||
alternate may be the one that exact-matches the query."""
|
||||
xml = """<items total="2">
|
||||
<item type="boardgame" id="140509">
|
||||
<name type="primary" value="Dragones Y Mazmorras"/>
|
||||
</item>
|
||||
<item type="boardgame" id="140509">
|
||||
<name type="alternate" value="Dungeons & Dragons"/>
|
||||
</item>
|
||||
</items>"""
|
||||
results = parse_search(xml)
|
||||
assert [r.name for r in results] == ["Dragones Y Mazmorras", "Dungeons & Dragons"]
|
||||
|
||||
@@ -698,3 +698,66 @@ def test_research_can_target_rpggeek_explicitly(tmp_path):
|
||||
assert row["bgg_id"] == ""
|
||||
with pytest.raises(ValueError, match="needs some text"):
|
||||
session.research(row, " ")
|
||||
|
||||
|
||||
def test_find_row_prefers_the_version_open_sibling(tmp_path):
|
||||
"""Two decided duplicate rows: a version pick with a stale row_ix must
|
||||
land on the sibling whose VERSION is still open, not overwrite the
|
||||
other's earlier human decision (mirrors _adopt)."""
|
||||
cfg = _setup(
|
||||
tmp_path,
|
||||
[
|
||||
_row(
|
||||
title_raw="Wiz-War",
|
||||
match_status="approved",
|
||||
bgg_id="589",
|
||||
version_status="version_approved",
|
||||
version_id="1",
|
||||
),
|
||||
_row(
|
||||
title_raw="Wiz-War",
|
||||
match_status="approved",
|
||||
bgg_id="589",
|
||||
version_status="version_ambiguous",
|
||||
),
|
||||
],
|
||||
)
|
||||
session = ReviewSession(
|
||||
cfg, console=quiet_console(), input_fn=scripted(), client=fixture_client()
|
||||
)
|
||||
row = session.find_row("Wiz-War", "hand-typed-test-list", row_ix=None)
|
||||
assert row["version_status"] == "version_ambiguous"
|
||||
|
||||
|
||||
def test_research_clears_the_previous_games_version(tmp_path):
|
||||
"""A fresh ballot supersedes EVERYTHING about the old match: a
|
||||
surviving version_id would put the old game's edition on whatever the
|
||||
human picks next."""
|
||||
cfg = _setup(
|
||||
tmp_path,
|
||||
[
|
||||
_row(
|
||||
title_raw="Citadels",
|
||||
match_status="approved",
|
||||
bgg_id="478",
|
||||
version_status="version_approved",
|
||||
version_id="99999",
|
||||
version_name="Old Game's Edition",
|
||||
)
|
||||
],
|
||||
)
|
||||
session = ReviewSession(
|
||||
cfg, console=quiet_console(), input_fn=scripted(), client=fixture_client()
|
||||
)
|
||||
# stats batch for these results isn't recorded; decoration may fail
|
||||
session.client = BGGClient(
|
||||
cache_dir=FIXTURES,
|
||||
transport=httpx.MockTransport(
|
||||
lambda req: httpx.Response(401, text="Unauthorized")
|
||||
),
|
||||
)
|
||||
row = session.rows[0]
|
||||
session.research(row, "Citadels")
|
||||
assert row["version_status"] == ""
|
||||
assert row["version_id"] == "" and row["version_name"] == ""
|
||||
assert row["version_candidates_json"] == "[]"
|
||||
|
||||
@@ -633,10 +633,15 @@ def test_pipeline_reports_badge_fields(tmp_path):
|
||||
},
|
||||
],
|
||||
)
|
||||
cfg.to_add_path.write_text("bgg_id,bgg_name\n1,X\n2,Y\n")
|
||||
# queue rows must be ENDORSED by matches.csv or they count as retired
|
||||
cfg.to_add_path.write_text("bgg_id,bgg_name,version_id\n13,Catan,1\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
|
||||
assert p["to_add"] == 1 # header excluded
|
||||
# a queue row review no longer backs is not pending work
|
||||
cfg.to_add_path.write_text("bgg_id,bgg_name,version_id\n13,Catan,1\n99,Gone,\n")
|
||||
p = _app(cfg).get("/api/pipeline").json()
|
||||
assert p["to_add"] == 1
|
||||
|
||||
|
||||
def test_photo_detail_page_serves_with_photos_nav_active(tmp_path):
|
||||
|
||||
@@ -1344,3 +1344,88 @@ def test_local_game_notes_and_art_round_trip(tmp_path):
|
||||
== 400
|
||||
)
|
||||
assert web.post(f"/api/local-game/{key}", json={"year": "19x8"}).status_code == 400
|
||||
|
||||
|
||||
def test_research_endpoint_contract(tmp_path):
|
||||
"""The web UI's manual-search button: 200 replaces the ballot, blank
|
||||
query 400s, BGG-down 502s, and `types` reaches the client (the
|
||||
RPGGeek toggle is the whole point)."""
|
||||
cfg = make_cfg(tmp_path)
|
||||
rows = read_matches(cfg.matches_path)
|
||||
rows.append(_row(title_raw="MYSTERY BOX", match_status="unmatched"))
|
||||
write_matches(cfg.matches_path, rows)
|
||||
|
||||
seen_queries = []
|
||||
|
||||
class _Recorder(BGGClient):
|
||||
def search(self, query, types=None):
|
||||
seen_queries.append((query, types))
|
||||
return []
|
||||
|
||||
client = _Recorder(cache_dir=tmp_path / "no_cache")
|
||||
web = TestClient(create_app(cfg, client=client))
|
||||
body = {
|
||||
"title_raw": "MYSTERY BOX",
|
||||
"source_photos": "shelf.jpg",
|
||||
"query": "Alice is Missing",
|
||||
"types": "rpgitem",
|
||||
}
|
||||
assert web.post("/api/research", json=body).status_code == 200
|
||||
assert ("Alice is Missing", "rpgitem") in seen_queries
|
||||
|
||||
assert web.post("/api/research", json={**body, "query": " "}).status_code == 400
|
||||
|
||||
down = TestClient(create_app(cfg, client=unauthorized_client(tmp_path)))
|
||||
assert down.post("/api/research", json=body).status_code == 502
|
||||
|
||||
|
||||
def test_corrupt_local_games_store_returns_500_not_silence(tmp_path):
|
||||
web, cfg = make_client(tmp_path)
|
||||
cfg.local_games_path.write_text("{torn")
|
||||
res = web.post("/api/local-game/local:x:y.jpg", json={"name": "X"})
|
||||
assert res.status_code == 500
|
||||
assert "local_games.json" in res.json()["detail"]
|
||||
|
||||
|
||||
def test_local_game_detail_reflects_saved_facts_before_enrich(tmp_path):
|
||||
"""The edit form re-renders from the detail payload; serving pre-save
|
||||
values there would resubmit as blanks and clear the store."""
|
||||
cfg = make_cfg(tmp_path)
|
||||
rows = read_matches(cfg.matches_path)
|
||||
rows.append(
|
||||
_row(title_raw="Homebrew Game", match_status="local", source_photos="s.jpg")
|
||||
)
|
||||
write_matches(cfg.matches_path, rows)
|
||||
key = "local:homebrew game:s.jpg"
|
||||
cfg.games_path.write_text(
|
||||
json.dumps({key: {"name": "Homebrew Game", "type": "localgame"}})
|
||||
)
|
||||
web = TestClient(create_app(cfg, client=unauthorized_client(tmp_path)))
|
||||
web.post(f"/api/local-game/{key}", json={"designers": "A Friend", "year": "1998"})
|
||||
detail = web.get(f"/api/library/{key}").json()
|
||||
assert detail["designers"] == ["A Friend"] # live, before any enrich
|
||||
assert detail["year"] == 1998
|
||||
|
||||
|
||||
def test_readding_a_removed_hand_added_title_rescinds_the_removal(tmp_path):
|
||||
"""add → remove → add again must resurrect the line, not silently
|
||||
no-op behind a success response."""
|
||||
web, cfg = make_client(tmp_path)
|
||||
add = {
|
||||
"title": "Homebrew Quest",
|
||||
"publisher": "",
|
||||
"edition": "",
|
||||
"year": "",
|
||||
"language": "",
|
||||
}
|
||||
assert web.post("/api/add-title", json=add).status_code == 200
|
||||
# a second identical add is a refused no-op, not a silent success
|
||||
assert web.post("/api/add-title", json=add).status_code == 409
|
||||
state = web.post(
|
||||
"/api/remove-title",
|
||||
json={"title_raw": "Homebrew Quest", "source_photos": ""},
|
||||
)
|
||||
assert state.status_code == 200
|
||||
assert web.post("/api/add-title", json=add).status_code == 200
|
||||
titles = json.loads(cfg.titles_path.read_text())
|
||||
assert any(t["title_raw"] == "Homebrew Quest" for t in titles)
|
||||
|
||||
Reference in New Issue
Block a user