feat(enrichment): write the master track store at request time — enrich once, reuse everywhere (#541) - #550
Conversation
…ackfill (#541) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… time (#541) Rewire enrich_request_metadata to populate the global tracks store once per unique recording and reuse it: a complete store row (genre+bpm+musical_key) short-circuits ALL providers and copies down, so the same song requested at a second event costs zero API calls (the dedupe win). Thread per-field provenance through a resolved accumulator so the store dual-write carries accurate sources (beatport/tidal/musicbrainz) with no post-hoc guessing; capture the ISRC that was previously discarded into the track identity. Soundcharts audio-features (energy/danceability/...) write behind the dark-by-default gate when an ISRC is in hand and the row lacks energy; bpm/key/genre stay with the existing cascade to avoid equal-precedence churn. The Request's genre/bpm/musical_key columns are still written for the current UI, and the store upsert is wrapped so a store failure never regresses the request's own commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#541) Three review fixes to the cache-aside dual-write so the store dedupe win is actually realized and the request commit is never regressed: 1. Pre-supplied request metadata now reaches the store. Requests legitimately arrive with partial metadata (RequestCreate accepts genre/bpm/musical_key; the frontend submits search-result fields), but _apply_enrichment_result only recorded a field into the store payload when the Request was MISSING it — so a request that already carried a field wrote an incomplete store row that could never satisfy the cache-aside gate (genre AND bpm AND musical_key) and re-hit every provider forever. Seed each core field the Request holds but the accumulator lacks, attributed to the lowest 'legacy' provenance so real later enrichment cleanly overrides it. 2. Energy now backfills onto a trio-complete cached row on a repeat request. The short-circuit returned unconditionally on a complete-trio row, making the Soundcharts gate's cached.energy-is-None arm dead for exactly the rows it serves (contradicting spec §2/§4/§5.4/§7, incl. the dark-rollout-then-enable plan). When the gate is on and the cached row lacks energy, fall through to a Soundcharts-only backfill, still skipping the core cascade so the zero-extra-core-API-calls dedupe win is preserved. 3. A store upsert_track failure no longer poisons the Request commit. A flush-time DB error left the session in PendingRollback so the trailing db.commit() raised PendingRollbackError and the Request's freshly enriched bpm/genre/key were lost — the opposite of the "store is strictly additive, never regress the request commit" guarantee. _safe_upsert_track commits the Request enrichment first, then runs the store write under its own commit/rollback recovery. Adds three regression tests (pre-supplied-field completeness + reuse, energy backfill-on-repeat without re-running the cascade, store-failure preserves request enrichment). Full gate green: ruff/bandit/pytest, coverage 89.20% (>=85%). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
) Three functional gaps in the new cache-aside behavior: - Legacy rows were treated as authoritative cache hits: a complete trio sourced only from 'legacy' (backfill/seed) short-circuited forever, so real providers could never upgrade it. _trio_trusted now requires a non-legacy trio; a legacy row falls through to the cascade and is upgraded, then cached. - Spotify ISRC extraction was nested under the Tidal-token guard, starving the Soundcharts audio-features lookup for DJs without a Tidal token. ISRC is now captured independently (and when the Soundcharts gate is on). - Already-complete submissions never seeded the store (early return + the guest submit path skipped enrichment for full-metadata requests), so search-result submissions never populated the store. _seed_complete_request seeds the trio as 'legacy' on the complete path, and the submit path now enqueues enrichment for complete requests too. Regression tests added for all three; pre-existing presupplied test split to match the corrected legacy-upgrade behavior. Full gate green, 89.26%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 33 minutes and 28 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds ISRC support to request submission and storage, switches enrichment background work to fresh sessions, updates enrichment to write and reuse shared track-store records with provenance rules, and adds a legacy backfill script. ChangesISRC request plumbing and track-store enrichment
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@server/app/scripts/backfill_tracks.py`:
- Around line 47-56: The backfill in backfill_tracks.py currently calls Request
query .all(), which loads every matching row into memory at once. Update the
existing request-fetching logic in the backfill flow to use batched iteration or
streaming with the Request query instead of materializing the full result set,
and keep the processing loop working over chunks so memory usage stays bounded
as the table grows.
- Around line 61-93: Wrap each request in a per-row savepoint inside
backfill_tracks() so one failing upsert does not poison the shared Session. Use
db.begin_nested() around the values/signature/upsert_track path, and keep the
existing exception logging for request.id; if you choose not to use a nested
transaction, you must explicitly rollback after a failed flush before
continuing.
In `@server/app/services/sync/enrichment_pipeline.py`:
- Line 402: Move ISRC/source extraction in the enrichment flow before the cache
lookup so `get_track` can use the resolved Spotify ISRC as well as the
signature. In `enrichment_pipeline.py`, update the logic around the `get_track`
call to resolve `resolved_isrc` first, then query with `get_track(db,
isrc=resolved_isrc, signature=sig)` instead of signature-only. Make the same
ordering change in the related block around `get_track` usage so existing rows
matched by ISRC are found before providers are re-run.
- Around line 330-342: The _trio_trusted helper currently treats any
non-"legacy" source as trusted, which incorrectly includes missing provenance
and "unknown" entries. Update the trust check in _trio_trusted so only explicit
real-provider sources are considered authoritative for genre, bpm, and
musical_key, and make sure rows without provenance do not short-circuit the
cache-aside path.
- Around line 354-362: The seeding path in
enrichment_pipeline._safe_upsert_track currently writes request.musical_key
directly for already-complete requests, which can cache a non-normalized value
before normalize_key() runs. Normalize the complete-request musical_key before
building values/sources in this block (using the same key-normalization logic as
the later normalization path) so cached track data is stored in Camelot form and
reused consistently for future requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c703debd-d254-44fe-95cc-6d1f82f78e2c
📒 Files selected for processing (7)
server/app/api/events.pyserver/app/scripts/backfill_tracks.pyserver/app/services/sync/enrichment_pipeline.pyserver/app/services/tracks/provenance.pyserver/tests/test_backfill_tracks.pyserver/tests/test_enrichment_store_writes.pyserver/tests/test_track_store.py
…ime store) Second-order issues surfaced by CodeRabbit and Codex on the rebased PR: - Cache trust: _trio_trusted now requires real-provider provenance (precedence >= 50) via is_cache_authoritative, not merely source != 'legacy'. Missing/ unknown/low-trust (community/llm) trios no longer short-circuit provider upgrades. - Complete-request path now runs the Soundcharts energy backfill (gate on, row lacks energy) instead of returning before it. - Spotify ISRC is fetched only when a consumer exists (Tidal exact-match needs a token; Soundcharts needs the gate) — no wasted external call otherwise. - _seed_complete_request normalizes the key to Camelot before seeding. - backfill_tracks streams rows (yield_per) and wraps each row in a SAVEPOINT so a poisoned-session error on one row no longer aborts the whole run. Declined CodeRabbit's 'resolve ISRC before the cache lookup': it would add a Spotify API call to every request before the signature cache check, defeating the zero-call dedupe win, to catch a rare different-signature/same-ISRC case that upsert_track's ISRC backfill already heals on the next miss. Regression tests added for each fix. Full gate green, 3237 passed, 89.20%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🔍 Codex (GPT-5.5, xhigh) round-2 on #550 — 3 confirmed [P2]s, all fixed in 22c6067 with regression tests:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@server/tests/test_backfill_tracks.py`:
- Around line 211-212: The test in test_backfill_tracks.py only asserts that no
row exists for bad_sig, but it should also directly verify the rolled-back
poison insert with a NULL-signature check. Update the backfill regression test
near the existing assert on Track.query so it includes an explicit assertion for
Track.signature IS NULL, using the same Track model and db.query path, so the
test fails if a NULL-signature row leaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 10999f56-1182-4a54-b25f-01ee91a9dd53
📒 Files selected for processing (5)
server/app/scripts/backfill_tracks.pyserver/app/services/sync/enrichment_pipeline.pyserver/app/services/tracks/provenance.pyserver/tests/test_backfill_tracks.pyserver/tests/test_enrichment_store_writes.py
🚧 Files skipped from review as they are similar to previous changes (2)
- server/app/scripts/backfill_tracks.py
- server/app/services/sync/enrichment_pipeline.py
…ackfill test (#541) Codex convergence finding (P2): the cache-aside fast path returned before the event-context BPM correction, so a trusted cached BPM (e.g. 66) was served raw into a 128-132 BPM event instead of being half/double-time corrected like the miss path does. Extracted the correction into _apply_bpm_context_correction and call it on BOTH paths — the canonical store value is left unchanged (correction is per-event, request-only). Also: backfill bad-row test now asserts the NULL-signature poison row was rolled back directly (IS NULL), per CodeRabbit — matching the test's stated intent. Regression test test_cache_hit_applies_bpm_context_correction added. Full gate green, 3238 passed, 89.21%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🔍 Codex (GPT-5.5, xhigh) round-3 (convergence) on #550 — 1 confirmed [P2], fixed in a0788db: the cache-aside fast path returned before the event-context BPM correction, so a trusted cached BPM (e.g. 66) was served raw into a 128–132 BPM event instead of being half/double-time corrected like the miss path. Extracted |
…#505) Codex finding (P2): broadening the submit enqueue to complete submissions (#541) scheduled enrich_request_metadata with the request-scoped `db`, which — with Soundcharts enabled — pins a pool connection through Spotify/Soundcharts network calls. Switch to the existing _enrich_with_fresh_session(id) helper so the task runs in its own session and releases the connection promptly (the #505 pattern; also closes the pre-existing submit-path gap). Pinned by test_submit_request_schedules_fresh_session_not_db. Full gate green, 3239 passed, 89.21%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🔍 Codex round-4 (verification) on #550 — 1 confirmed [P2], fixed in 0100e5c: broadening the submit enqueue to complete submissions (#541) scheduled enrichment with the request-scoped |
…-BPM store (#541) From the search→queue consistency audit + final Codex pass: - Codex P2: the global store now keeps the CANONICAL provider BPM. Per-event half/double-time correction applies to the Request only (never written to the tracks row), so a cache hit at another event re-derives its own correction instead of inheriting this event's tempo. - MEDIUM-1: collect submit now enqueues enrichment UNCONDITIONALLY (matches submit_request) — the divergent completeness guard was the pre-#541 pattern, inert only because CollectSubmitRequest lacks trio fields; a latent silent-skip the moment it gains them. - MEDIUM-2 (structural): extracted the verbatim-triplicated _enrich_with_fresh_session into a single shared helper in services/sync/orchestrator.py; events/collect/ requests all import it. Now a new entry point physically cannot drift — importing the one helper gets the master-store write + fresh-session hygiene for free. - Refreshed the stale enrichment_pipeline docstrings to document the #541 master- store dual-write / cache-aside / energy-backfill (they only described the old genre/BPM/key cascade). Tests: canonical-BPM-in-store pin, submit/refresh-metadata/enrich-all fresh-session scheduling, helper-unit-tests + collect test repointed to the shared helper. Full gate green, 3242 passed, 89.25%. MEDIUM-3 (duplicate recommendation matcher) filed as #551. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…store write (#541) Codex P2: the canonical-BPM-in-store rule was enforced on the provider-resolved path but NOT the pre-supplied legacy-seed path. A partial request arriving with a pre-supplied BPM has it event-context-corrected (request.bpm 66->132) before the legacy-seed loop runs; since that BPM isn't in , the seed wrote the event-corrected value to the global tracks row. Stash the pre-correction value (canonical_bpm) and seed THAT. Regression test added. Declining Codex's companion 're-check cache by ISRC before the cascade' finding: its proper resolution is capturing the search-result ISRC at submit (it's already returned by Tidal/Beatport search) — tracked as a follow-up rather than adding a redundant pre-cascade lookup. Data already converges via upsert_track's ISRC backfill, so it is an efficiency gap, not corruption. Full gate green, 3243 passed, 89.25%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🔍 Codex r6 (final verification) — 2 [P2]s, both the ISRC-first theme already deferred to #552 (no new #550-scope issue):
Disposition: declining both for #550, deferred to #552 ("ISRC-first from submit"). The proper fix is capturing the search-result ISRC at submit (Tidal/Beatport/Spotify search already return it) and going ISRC-first on both the cache lookup AND This was the 6th Codex round; rounds 1–5 fixed all substantive #550-scope issues (transaction poisoning, dedupe-defeat, legacy-not-authoritative, energy-backfill paths, BPM canonical-store on both paths, fresh-session scheduling). r6 surfaced only the deferred theme → converged. |
Capture the search-result ISRC at submit and use it as the master-store cache key + identity, instead of re-deriving it during the background cascade. This makes the store genuinely ISRC-first from the moment of submission and closes the duplicate-row edge Codex flagged on #550 (a complete credit-variant submission under a different normalized signature no longer inserts a second row). Backend: - migration 063 adds requests.isrc; Request.isrc model column. - RequestCreate.isrc + CollectSubmitRequest.isrc; create_request and the collect submit store normalize_isrc(isrc). - enrich_request_metadata: cache lookup is now get_track(isrc=request.isrc, sig), resolved_isrc seeds from request.isrc, and _seed_complete_request seeds by ISRC. Manual/typed requests (no ISRC) keep the signature fallback — purely additive. Frontend: - submitRequest + submitCollectRequest forward isrc; the four search->submit surfaces (DJ search modal, guest join, kiosk request modal, collect) pass the chosen result's isrc. DJ accept-recommended is unchanged (RecommendedTrack has no ISRC). Regenerated openapi.json + api-types.generated.ts. Tested: backend ISRC-first cache hit across variant signature + no-duplicate complete-submission collapse (3245 passed, 89.25%); frontend submitRequest / submitCollectRequest forward-isrc unit tests (tsc + eslint + vitest 1393 green); alembic check clean. Closes #552. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
#552 folded in (ISRC-first from submit). Per maintainer call (zero residual edges before merge), the ISRC-at-submit slice is now part of this PR: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/app/services/sync/enrichment_pipeline.py (1)
365-397: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve canonical BPM when reseeding already-complete requests.
A request that was previously served from cache can have
request.bpmcontext-corrected and committed on the request row, then hit this early-return path on/refresh-metadataor bulk re-enrich._seed_complete_request()will reseed the global track row from that event-specific BPM, which breaks the later invariant that only canonical BPM is stored intracks.Please carry a canonical BPM into this path as well (for example, reuse the existing track row’s BPM when one exists, or thread a pre-correction BPM into
_seed_complete_request) and add a regression test covering refresh of a cache-hit request.Also applies to: 465-477
🤖 Prompt for 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. In `@server/app/services/sync/enrichment_pipeline.py` around lines 365 - 397, The `_seed_complete_request` path is reseeding `tracks` with the request’s event-specific BPM instead of the canonical track BPM, which can overwrite the stored canonical value during refresh/re-enrich flows. Update `_seed_complete_request` to prefer an existing canonical BPM from the track row when available, or pass a pre-correction BPM into this helper before the request row is committed, and keep the same treatment in the other reseed path that uses this helper. Add a regression test around a cache-hit request refreshed through `/refresh-metadata` or bulk re-enrich to verify the canonical BPM in `tracks` is preserved.
🤖 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 `@server/tests/test_enrichment_store_writes.py`:
- Around line 917-957: The ISRC identity tests are coupled to the Soundcharts
feature flag and can become flaky when enrich_request_metadata() performs energy
backfill unexpectedly. In the affected ISRC-focused tests, stub get_settings()
so soundcharts_audio_features_enabled is pinned to the intended value before
calling enrich_request_metadata(), keeping the assertions deterministic and
isolated from external adapter behavior.
---
Outside diff comments:
In `@server/app/services/sync/enrichment_pipeline.py`:
- Around line 365-397: The `_seed_complete_request` path is reseeding `tracks`
with the request’s event-specific BPM instead of the canonical track BPM, which
can overwrite the stored canonical value during refresh/re-enrich flows. Update
`_seed_complete_request` to prefer an existing canonical BPM from the track row
when available, or pass a pre-correction BPM into this helper before the request
row is committed, and keep the same treatment in the other reseed path that uses
this helper. Add a regression test around a cache-hit request refreshed through
`/refresh-metadata` or bulk re-enrich to verify the canonical BPM in `tracks` is
preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8fd55c2f-236a-49f6-a4b1-77fa1ddffa3b
📒 Files selected for processing (23)
dashboard/app/(dj)/events/[code]/components/DjSongSearchModal.tsxdashboard/app/collect/[code]/page.tsxdashboard/app/e/[code]/display/components/RequestModal.tsxdashboard/app/join/[code]/page.tsxdashboard/lib/__tests__/api.test.tsdashboard/lib/__tests__/collect-api.test.tsdashboard/lib/api-types.generated.tsdashboard/lib/api.tsserver/alembic/versions/063_add_request_isrc.pyserver/app/api/collect.pyserver/app/api/events.pyserver/app/api/requests.pyserver/app/models/request.pyserver/app/schemas/collect.pyserver/app/schemas/request.pyserver/app/services/request.pyserver/app/services/sync/enrichment_pipeline.pyserver/app/services/sync/orchestrator.pyserver/openapi.jsonserver/tests/test_backfill_tracks.pyserver/tests/test_bg_fresh_sessions.pyserver/tests/test_collect_public.pyserver/tests/test_enrichment_store_writes.py
✅ Files skipped from review due to trivial changes (3)
- server/alembic/versions/063_add_request_isrc.py
- server/app/schemas/request.py
- dashboard/lib/api-types.generated.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- server/tests/test_backfill_tracks.py
- server/app/api/events.py
…sts (#552) Codex P2: the ISRC-first cache lookup get_track(isrc, sig) falls back to the signature when the ISRC misses, so a request carrying ISRC_A could short-circuit on a signature-matched row whose ISRC is a DIFFERENT recording (ISRC_B, same artist/title, different release/remaster) — serving wrong metadata and skipping providers. The fast path now only trusts a row whose ISRC is empty or matches the request ISRC; a mismatch falls through to providers. An ISRC-less trusted row is still used and gets the request ISRC backfilled (the ISRC lookup missed, so no collision). Deeper note: signature is UNIQUE, so two distinct recordings sharing a normalized artist/title still cannot both persist — that is a #540/#542 design matter, not this fix. CodeRabbit Major: pin the Soundcharts gate OFF in the two ISRC identity tests so the cache-hit energy-backfill arm can't make a real API call if the env enables the gate (config-coupled flakiness). Regression tests: different-ISRC row not trusted (providers run); ISRC-less row trusted + ISRC backfilled. Full gate green, 3247 passed, 89.26%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🔍 Codex r7 (reviewed the full PR incl. #552) — 1 [P2], fixed in bc7f7ff: the ISRC-first cache lookup's signature fallback could serve a DIFFERENT recording's metadata when the request's ISRC missed but the signature matched a row with a different non-null ISRC (a different release/remaster of the same artist/title). The fast path now only trusts a row whose ISRC is empty or matches the request's; a mismatch falls through to providers, and an ISRC-less trusted row gets the request ISRC backfilled. (Deeper signature-uniqueness limitation — two distinct recordings with an identical normalized artist/title — is a #540/#542 design matter, noted, not in scope here.) Regression tests added; full gate 3247 @ 89.26%. |
…ding (#552) Codex P2 (store-level ISRC conflict): upsert_track's get_track(isrc, sig) falls back to the signature when the ISRC misses, so a complete submission / provider refresh carrying ISRC_B could land on the signature row of a DIFFERENT recording (ISRC_A, same normalized artist/title) and overwrite A's metadata with B's, never storing B. upsert_track now detects a non-null ISRC mismatch on a signature- fallback match and returns the existing row UNCHANGED (logged), so the store can never be corrupted by a different recording. The recording's data still lives on its Request row. Full multi-recording-per-signature support (ISRC-primary identity instead of unique signature) is a #542/#540 schema matter, noted, out of scope. This complements the cache-READ guard (bc7f7ff); the WRITE guard in upsert_track protects every store writer (seed, dual-write, energy backfill, backfill script). Regression test: same signature + different ISRC does not clobber the existing row. Full gate green, 3248 passed, 89.26%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🔍 Codex r8 — 1 [P2], fixed in 4a13c32: the store-level counterpart of r7. |
…ty (#552) Codex P2: RequestCreate.isrc / CollectSubmitRequest.isrc accepted any <=15-char string, but enrichment treats the stored value as authoritative recording identity (cache key) and sends it to provider by-ISRC endpoints. A malformed value would defeat the ISRC-first cache (no row matches) and drive invalid external calls. Added valid_isrc(): normalize + validate the ISO 3901 shape (^[A-Z]{2}[A-Z0-9]{3}[0-9]{7}$, 12 chars). Both submit schemas now drop a malformed ISRC to None (lenient: an optional metadata field must not 422 the whole request) so only a well-formed ISRC is ever stored or used as a key. Provider- sourced ISRCs are trusted. Tests: valid normalize, hyphenated normalize, malformed->None, absent->None, for both RequestCreate and CollectSubmitRequest. Full gate green, 3254 passed, 89.27%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🔍 Codex r9 — 1 [P2], fixed in 6aa5d57: the submitted ISRC (RequestCreate/CollectSubmitRequest) was unvalidated but used as authoritative identity (cache key) + sent to provider by-ISRC endpoints. Added |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@server/app/services/tracks/store.py`:
- Around line 109-127: The ISRC conflict guard in upsert_track is only reachable
through the initial get_track path, so reconciled rows returned from
_insert_identity_reconciling can still fall through and overwrite a different
recording. Move the ISRC mismatch check out of the current elif chain into a
standalone if that runs after the insert/reconcile branch, using the existing
upsert_track, _insert_identity_reconciling, and get_track flow so any row
resolved by signature fallback is rejected before backfill/overwrite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 61771f4f-5966-4b86-8751-76d080fb427c
📒 Files selected for processing (8)
server/app/schemas/collect.pyserver/app/schemas/request.pyserver/app/services/sync/enrichment_pipeline.pyserver/app/services/track_normalizer.pyserver/app/services/tracks/store.pyserver/tests/test_enrichment_store_writes.pyserver/tests/test_input_validation.pyserver/tests/test_track_store.py
🚧 Files skipped from review as they are similar to previous changes (4)
- server/app/schemas/collect.py
- server/app/schemas/request.py
- server/tests/test_enrichment_store_writes.py
- server/app/services/sync/enrichment_pipeline.py
…#552) CodeRabbit Major: the r8 ISRC-conflict guard was an elif on the INITIAL get_track, so it was bypassed when the conflict surfaced via the reconcile race — insert hits the unique-signature constraint, _insert_identity_reconciling re-reads a different- ISRC row, and that returns through the 'track is None' branch, skipping the elif. Moved the guard to a standalone check AFTER resolution so it covers both the initial signature fallback AND the reconcile re-read. A freshly inserted row has isrc==norm_isrc so it never trips on the insert path. Regression test: a reconcile-race landing on a different-ISRC row does not overwrite it. Full gate green, 3255 passed, 89.27%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Why
enrich_request_metadatawrote genre/bpm/key onto eachRequestrow, so the same recording requested at two events was enriched twice, and energy/duration were never enriched at request time. #540 shipped the mastertracksstore; #548 added the writer foundation. This wires enrichment to populate the global store once per unique recording and reuse it — the next request for the same song costs zero provider calls.What
enrich_request_metadata:dedupe_signature→get_track(); if the store row is complete and trusted, copy fields onto theRequestand skip all providers (the dedupe win). Otherwise run the existing cascade, thread per-field provenance, andupsert_track()while still writingRequest.genre/bpm/musical_keyfor the current UI.bpm/key/genrestay with the existing cascade.legacyprovenance source (precedence 30) + idempotent backfill script (python -m app.scripts.backfill_tracks).Hardening from two adversarial review passes
Workflow review (3 fixed): pre-supplied metadata now reaches the store; store-upsert failure no longer poisons the Request commit (commit-first recovery); energy backfills onto a complete cached row.
Codex review (3 fixed):
legacy-only rows are no longer authoritative cache hits (real providers upgrade them); Spotify ISRC capture decoupled from the Tidal token (feeds Soundcharts tokenless); already-complete search-result submissions now seed the store.Testing
main)🤖 Co-authored by Claude Opus 4.8. Closes #541. Built via TDD + multi-agent + Codex adversarial review.
Summary by CodeRabbit