fix(sessions): grade artist guesses with the shared judge, not a local comma-split - #24
Conversation
matching.py documents itself as "the authoritative judge for scoring", and
scoring.py uses it. The session guess path — the one every current client
actually hits — did not: it decided artist correctness locally with
artist_parts = [p.strip() for p in entry["artist"].lower().split(",")]
which only understands comma-separated credits. The guess dropdown splits on
the full separator set (, & / + × feat ft featuring with x) and submits one of
those split-out names, so whenever a credit used a non-comma separator every
option the dropdown offered was graded wrong and the track could not be
answered at all.
Measured over 419,688 playable tracks: 1.4% were unanswerable in artist mode
and a further 0.7% had some options mis-graded (2.1% affected). Lower than a
first read of the separator list suggests, because Spotify's API joins
collaborators with commas — the non-comma separators mostly turn up inside band
names ("Jeckyll & Hyde", "Heavy D & The Boyz", "Allie X").
Correctness now delegates to matches_artist via a small pure helper, so the
decision is testable without a database and there is one judge again rather
than two that can drift.
Verified against 20,000 distinct real artist strings (38,231 track/dropdown-
option pairs): 0 cases where the shared judge rejects something the local one
accepted (strict superset), and unanswerable strings go 178 -> 0.
Two incidental hardening changes in the same helper: song mode now requires a
non-empty track_id rather than comparing None, and a missing artist field no
longer raises (the old code called .lower() on entry["artist"] directly).
Adds a python sessions.py self-check in the same style as matching.py, covering
each separator form, accent folding, a single act whose name contains a comma,
and the strictness cases (wrong artist, partial name, blank, skip). Confirmed
the check fails if the old local judge is reintroduced.
Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSession guess validation now uses centralized artist, song, and skip rules. Reveal serialization handles missing track fields. Preview retries use the shared limit. Executable assertions cover the updated behavior. ChangesGuess correctness
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…a fill Addresses CodeRabbit review on #23. The nested `.get()` chain over the embed payload could raise AttributeError, and because it sits inside an asyncio.gather fan-out without return_exceptions, that raise discarded the whole chunk's results: none of mark_recovered/mark_unavailable/mark_failed ran, so the offending track's retry counter never advanced and it could knock out the same playlist's fill on every subsequent attempt. Credit to the review for tracing that consequence. The suggested patch (guard `if not next_data`) closes only part of it. Falsy payloads were already safe — `{}` chains fine. The actual vectors are: - truthy non-dict from json.loads: `"str"`, `42`, `true` - an explicit null at an intermediate level, e.g. `{"props": null}`, because `.get(k, {})` returns the default only for a MISSING key, not a null value - a non-dict `entity`, which survives the chain and then raises downstream in `.get("audioPreview")` / _extract_cover_art Of ten payload shapes, the current chain raises on six; the suggested guard fixes two of those six. So the fix is type-checked at every hop instead: - _embed_entity() walks to the entity node and always returns a dict. - _extract_cover_art() uses `or {}` / `or []` and skips non-dict sources, since a null coverArt or sources would otherwise raise on the next access. - The parse/extract in _scrape_one_preview is wrapped so the function is total rather than total-by-inspection. - gather() takes return_exceptions=True with the exceptions filtered and logged, per the reviewer's offer. Redundant with a total scraper, but this is the exact point where a raise turns into silent loss of persisted work, so the structural guard is worth its three lines. Verified: a raising task now costs one track instead of all 48 in its chunk. Also from the review: /playlist-status and /playlist/{id}/wait-for-previews hard-coded `preview_retry_count < 5` while load_playlist uses MAX_PREVIEW_RETRIES. #23 made that consequential — the client now starts polling off load_playlist's pending count and reads progress off /playlist-status's, so a change to the constant would leave the progress bar unable to finish. Both now take the shared constant. (sessions.py has the same literal; left alone to avoid churn against the open #24.) Adds a `python server.py` self-check covering all ten payload shapes and the cover-art edge cases, in the same style as matching.py and sessions.py. uvicorn imports the module and never runs __main__. Confirmed the check fails when the falsiness-only guard is substituted. Declined the negative-cache nitpick for /tracks/art: #23 makes that path cold (album art now ships inline with /playlist, so the endpoint is only hit for tracks art hasn't reached yet), a DB miss is one indexed lookup, and a negative sentinel would actively delay art that the preview fill is concurrently backfilling into art_cache. Co-Authored-By: Claude <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/sessions.py`:
- Line 80: Update the docstring text near the split-out separator description to
avoid the ambiguous literal “×” flagged by RUF002. Spell out “Unicode
multiplication sign” or equivalent wording while preserving the documented
separator semantics.
- Line 313: Update submit_guess’s reveal construction to safely handle entries
without an "artist" field, matching _is_guess_correct’s behavior and preventing
a KeyError response failure. Add an endpoint-level regression test that submits
a track missing "artist" and verifies a normal response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…or docs Addresses CodeRabbit review on #24. reveal construction (line 313) ------------------------------ _is_guess_correct reads `entry.get("artist") or ""`, then seven lines later the same dict was subscripted directly as entry["name"] / entry["artist"]. Two functions reading one dict disagreeing about whether its keys are guaranteed is the inconsistency worth removing — my own diff introduced it. To be accurate about severity: the KeyError is not reachable today. Every entry start_session writes carries name and artist, sourced from tracks columns that are NOT NULL DEFAULT '', and sessions expire after 2h so no older JSONB shape survives. This is consistency, not a live fix. It is still worth doing: `entry` is deserialized JSONB whose shape has changed before (see _META_KEYS), and the failure mode would be a 500 on a guess the player already committed to. Extracted _reveal_for(tid, entry) rather than inlining .get() calls, so the logic is testable without a database — same reasoning that produced _is_guess_correct in this PR. Response contract is unchanged: the same four keys, in the same shape the frontend reads. The review asked for an endpoint-level test. Declining that specific form: there is no pytest, no test database and no CI in this repo, so it would mean standing up the first backend HTTP test harness inside a review fixup, which is a scope decision for the owner rather than something to smuggle in here. The extracted helper is covered by the existing python sessions.py self-check instead, at the layer the bug would live in — full entry, empty entry, all-null-values entry, and partial entry. Confirmed the check fails with KeyError: 'name' when the direct subscripts are restored. separator docstring (line 80) ----------------------------- The list rendered `×` and `x` adjacent to each other. They are two different rules in _ARTIST_SEP — the multiplication sign splits anywhere, the letter x only at word boundaries — so a reader who cannot tell them apart loses the point of the sentence. Spelled the symbols out in words and pointed at matching._ARTIST_SEP for the exact pattern. The regex itself keeps the literal `×`; it is functional. also ---- Followed through on the MAX_PREVIEW_RETRIES consistency I said I would pick up in this batch when it came up on #23: start_session's pending_count was the third site hard-coding `preview_retry_count < 5`. It feeds the client's "waiting for previews" retry, so it has to mean the same thing as the preview store and /playlist-status. Co-Authored-By: Claude <noreply@anthropic.com>
|
Both addressed in abbde3d. 1.
|
1 participant
The bug
matching.pydescribes itself as "the authoritative judge for scoring" and warns to keep it in lock-step with the frontend.scoring.pyuses it. The session guess path — the one every current client actually hits — did not:That only understands comma-separated credits. But the guess dropdown is built with
splitArtistsRaw, which splits on the full separator set (, & / + × feat ft featuring with x) and submits one of those split-out names. So for any credit using a non-comma separator, every option the dropdown offered was graded wrong — and since the dropdown is the only submission path, the track could not be answered at all.Severity, measured
I want to correct an overstatement from my earlier triage. I'd implied every
feat./&collab was broken; the real number is smaller, because Spotify's API joins collaborators with commas (", ".join(artists[].name)), sofeat.usually lives in the track title, not the artist field. The non-comma separators mostly turn up inside band names.Over all 419,688 playable tracks:
Real examples that were unanswerable:
Jeckyll & Hyde,Heavy D & The Boyz,Bob & Earl,Michel Fugain & Le Big Bazar,aivi & surasshu,Allie X.The fix
Correctness delegates to
matches_artistthrough a small pure helper, so the decision is testable without a database and there is one judge again instead of two that can drift.Two incidental hardening changes in the same helper: song mode now requires a non-empty
track_idrather than comparingNone, and a missing artist field no longer raises — the old code called.lower()onentry["artist"]directly.Verification
Is it a strict superset? That was the main risk — a more permissive judge must not start rejecting anything. Checked against 20,000 distinct real artist strings from the production DB (38,231 track/dropdown-option pairs), running the actual
_is_guess_correct:Does the check have teeth? I reintroduced the old local judge and confirmed
python sessions.pyfails on the first artist assertion, then passes again once restored. (Batch 1 taught me not to trust a green test I haven't tried to break.)Adds a
python sessions.pyself-check in the same style asmatching.py— no pytest inrequirements.txt, so assert-based__main__checks are the existing convention. Covers each separator form, accent folding, a single act whose name contains a comma (Tyler, The Creator), and the strictness cases: wrong artist, partial name, blank, and skip in both modes.Notes for review
GamePage.jsso it can't conflict with fix(playlist): decouple guess autocomplete pool from track playability (closes #22) #23, which edits the same import line.GamePage.js:400checkGuessCorrectis a third, now-dead artist judge (defined, never called — a leftover from before server-side grading). Removing it belongs with the dead-code batch, and it's harmless meanwhile since it cannot be invoked.Bob & Earlnow acceptsBob. That matches the documented policy ("naming ANY credited artist is correct") and what the dropdown offers, so it's the intended behaviour rather than a side effect.Separate bug this surfaced (not fixed here)
The dropdown itself shreds band names, and the root cause is upstream of any judge:
Spotify hands us
artists[]as an array. We flatten it to a string, then downstream code tries to reverse-engineer the boundaries with separator heuristics. The heuristics cannot win:In Love With a GhostIn Love,a GhostAllie XAllieDion (With The Del-Satins)Dion (,The Del-Satins)Jeckyll & HydeJeckyll,HydeAfter this PR those are all gradeable (picking
Jeckyllis correct), so it's no longer a blocker — but players are being shown artists that don't exist, and one option has a stray(. The real fix is to stop discarding the array: persist the artist list astext[]and split on the authoritative boundary, keeping the fuzzy separators only for the embed-only path wheresubtitlegenuinely is free text. Worth its own PR.🤖 Generated with Claude Code
Summary by CodeRabbit