Repository navigation
fix(bot): drop the audio-features scoring path, spotify returns 403 - #2073
LucasSantana-Dev wants to merge 2 commits into
Conversation
Measured from inside lucky-bot with Lucky's own credentials:
v1/search / artists / albums / tracks HTTP 200
v1/audio-features HTTP 403
So currentFeatures was always null and enrichWithAudioFeatures returned
its input unchanged on the first line. The LRU cache behind it could only
ever hold null.
The Last.fm substitution this would otherwise need already exists.
candidateScorer already scores genre-family overlap in-pass via
calculateGenreFamilyPenalty on Last.fm tags, and the code says so at the
old call site: "Genre-family scoring moved to in-pass
calculateRecommendationScore via Last.fm tags so it works without a
Spotify token." What was left here is energy, valence, tempo and
acousticness, which Last.fm does not expose and nothing can substitute.
Extended access does not bring these back. Since 2025-05-15 it requires a
registered business entity and 250k MAUs, and the deprecation notice says
extended mode PRESERVED these endpoints for apps that already had it on
2024-11-27; it is not a door that opens later. Recorded on the issue.
enrichWithAudioFeatures existed twice, in candidateScorer and in
candidateFallback. Both are gone, along with audioFeatures.ts, the two
spotifyApi fetchers and the SpotifyAudioFeatures types.
Two regression tests used the removed fetchers only as a vehicle to
exercise spotifyFetch's shared AbortSignal and 429 retry. Those behaviours
still matter, so both are retargeted at searchSpotifyTrack rather than
deleted, and say so in a comment.
One test asserted searchSpotifyTrack had been called, which it reached
only through the audio-features path. It now asserts on the queue instead:
the point is that a Spotify-linked user gets tracks, and that survives
arms being retired.
3114 tests pass, type:check clean across all four workspaces, eslint quiet.
Stacked on #2071.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 |
|
Failed to generate code suggestions for PR |
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Graphify review — findings
Removes the audio-features enrichment path from autoplay: deletes audioFeatures.ts (getTrackAudioFeatures), drops its re-exports from queueManipulation.ts, and strips the now-unused audioFeatures param from candidateCollector. Rewrites the associated specs to assert on queue outcomes rather than searchSpotifyTrack/getBatchAudioFeatures calls, and deletes the energy/valence boost and enrichWithAudioFeatures tests.
Worth a look
- Removed public getTrackAudioFeatures barrel export —
packages/bot/src/services/musicManagement/queueManipulation.ts:17· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Removed exported getAudioFeatures API —
packages/bot/src/spotify/spotifyApi.ts· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Removed exported getBatchAudioFeatures API —
packages/bot/src/spotify/spotifyApi.ts· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Removed collectRecommendationCandidates positional parameter —
packages/bot/src/services/musicRecommendation/autoplay/candidateCollector.ts:61· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Removed exported enrichWithAudioFeatures API —
packages/bot/src/services/musicRecommendation/candidateFallback.ts· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 149 functions depend on the 119 functions this change touches.
Health — this change adds coupling hotspots:
- new:
replenishQueue()— 19 callers, 39 callees - new:
calculateRecommendationScore()— 15 callers, 8 callees - new:
collectLastFmCandidates()— 4 callers, 13 callees - new:
collectRecommendationCandidates()— 6 callers, 7 callees - new:
collectSeedSimilarCandidates()— 4 callers, 8 callees - new:
setupLifecycleHandlers()— 3 callers, 8 callees - new:
setupTrackHandlers()— 4 callers, 6 callees - new:
handlePlayerSkip()— 2 callers, 11 callees - …and 10 more — each is listed as a finding
Verification — 149 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 23de645 (diverged from this PR's base — delta is approximate).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 144 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 17 more finding(s) on lines outside this diff (see the check run).
| } | ||
| } | ||
|
|
||
| export async function getArtistPopularity( |
There was a problem hiding this comment.
getArtistPopularity()
high coupling complexity (Ca·Ce = 12).
Grounded coupling-delta finding (deterministic), not an LLM guess.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/bot/src/spotify/spotifyApiRetry.spec.ts">
<violation number="1" location="packages/bot/src/spotify/spotifyApiRetry.spec.ts:148">
P3: The two timer-based Retry-After tests now call searchSpotifyTrack but their mocked success response is still the removed getAudioFeatures shape ({ id, energy, valence }), which searchSpotifyTrack cannot parse (it reads tracks.items[0].id) and therefore returns null. Because the tests assert only attemptCount and never the result, they silently no longer exercise a valid successful parse and leave the exact audio-features artifacts this PR removes. Update both mocks to a search response (e.g. { tracks: { items: [{ id: 't1' }] } }) and assert the parsed id.</violation>
</file>
<file name="packages/bot/src/services/musicRecommendation/autoplay/replenisher.ts">
<violation number="1" location="packages/bot/src/services/musicRecommendation/autoplay/replenisher.ts:552">
P3: Since the `enrichWithAudioFeatures` pass is removed, `enriched` is just a redundant alias for `selected` and the name is now misleading. Drop the alias and use `selected` directly (or rename), so the code reflects that no enrichment happens.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| }) | ||
|
|
||
| const promise = spotifyApi.getAudioFeatures('tok', 't') | ||
| const promise = spotifyApi.searchSpotifyTrack( |
There was a problem hiding this comment.
P3: The two timer-based Retry-After tests now call searchSpotifyTrack but their mocked success response is still the removed getAudioFeatures shape ({ id, energy, valence }), which searchSpotifyTrack cannot parse (it reads tracks.items[0].id) and therefore returns null. Because the tests assert only attemptCount and never the result, they silently no longer exercise a valid successful parse and leave the exact audio-features artifacts this PR removes. Update both mocks to a search response (e.g. { tracks: { items: [{ id: 't1' }] } }) and assert the parsed id.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/bot/src/spotify/spotifyApiRetry.spec.ts, line 148:
<comment>The two timer-based Retry-After tests now call searchSpotifyTrack but their mocked success response is still the removed getAudioFeatures shape ({ id, energy, valence }), which searchSpotifyTrack cannot parse (it reads tracks.items[0].id) and therefore returns null. Because the tests assert only attemptCount and never the result, they silently no longer exercise a valid successful parse and leave the exact audio-features artifacts this PR removes. Update both mocks to a search response (e.g. { tracks: { items: [{ id: 't1' }] } }) and assert the parsed id.</comment>
<file context>
@@ -306,7 +145,11 @@ describe('Spotify API 429 Retry Logic', () => {
})
- const promise = spotifyApi.getAudioFeatures('tok', 't')
+ const promise = spotifyApi.searchSpotifyTrack(
+ 'tok',
+ 'Creep',
</file context>
| currentAudioFeatures, | ||
| currentTrack.author, | ||
| ) | ||
| const enriched = selected |
There was a problem hiding this comment.
P3: Since the enrichWithAudioFeatures pass is removed, enriched is just a redundant alias for selected and the name is now misleading. Drop the alias and use selected directly (or rename), so the code reflects that no enrichment happens.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/bot/src/services/musicRecommendation/autoplay/replenisher.ts, line 552:
<comment>Since the `enrichWithAudioFeatures` pass is removed, `enriched` is just a redundant alias for `selected` and the name is now misleading. Drop the alias and use `selected` directly (or rename), so the code reflects that no enrichment happens.</comment>
<file context>
@@ -562,16 +549,7 @@ async function _replenishQueue(
- currentAudioFeatures,
- currentTrack.author,
- )
+ const enriched = selected
if (requestedBy?.id) {
</file context>
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Graphify review — findings
Removes the getTrackAudioFeatures/audio-features enrichment path from autoplay, deleting audioFeatures.ts and its re-exports from queueManipulation.ts, and drops the corresponding enrichWithAudioFeatures and energy/valence-boost specs. Updates collectRecommendationCandidates callers to drop the now-removed audio-features argument, and rewrites the multi-user blend test to assert on queue outcome instead of searchSpotifyTrack being called.
Worth a look
- Removed exported getTrackAudioFeatures API —
packages/bot/src/services/musicManagement/queueManipulation.ts:17· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Removed exported getAudioFeatures API —
packages/bot/src/spotify/spotifyApi.ts:159· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Removed exported getBatchAudioFeatures API —
packages/bot/src/spotify/spotifyApi.ts:209· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- collectRecommendationCandidates positional contract changed —
packages/bot/src/services/musicRecommendation/autoplay/candidateCollector.ts:61· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Removed exported SpotifyAudioFeatureConstraints type —
packages/bot/src/spotify/spotifyApi.ts:13· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 149 functions depend on the 119 functions this change touches.
Health — this change adds coupling hotspots:
- new:
replenishQueue()— 19 callers, 39 callees - new:
calculateRecommendationScore()— 15 callers, 8 callees - new:
collectLastFmCandidates()— 4 callers, 13 callees - new:
collectRecommendationCandidates()— 6 callers, 7 callees - new:
collectSeedSimilarCandidates()— 4 callers, 8 callees - new:
setupLifecycleHandlers()— 3 callers, 8 callees - new:
setupTrackHandlers()— 4 callers, 6 callees - new:
handlePlayerSkip()— 2 callers, 11 callees - …and 10 more — each is listed as a finding
Verification — 149 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 23de645 (diverged from this PR's base — delta is approximate).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 144 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 17 more finding(s) on lines outside this diff (see the check run).
| } | ||
| } | ||
|
|
||
| export async function getArtistPopularity( |
There was a problem hiding this comment.
getArtistPopularity()
high coupling complexity (Ca·Ce = 12).
Grounded coupling-delta finding (deterministic), not an LLM guess.
…2074) Closes #2072. Replaces #2073, which GitHub auto-closed when #2071 merged with `--delete-branch` and took its base branch with it. Same content, rebuilt on current `main`. ## Measured, with Lucky own credentials from inside lucky-bot ``` v1/search / artists/{id} / albums/{id} / tracks/{id} HTTP 200 v1/audio-features HTTP 403 ``` So `currentFeatures` was always `null` and `enrichWithAudioFeatures` returned its input unchanged on the first line. The 5000-entry LRU cache behind it, with a 24h TTL, could only ever hold `null`. ## The Last.fm replacement already exists The plan was to substitute the Spotify signal with Last.fm tags. That work was already done: `candidateScorer` scores genre-family overlap in-pass via `calculateGenreFamilyPenalty` on Last.fm tags, and the code says so at the old call site: > Genre-family scoring moved to in-pass `calculateRecommendationScore` via Last.fm tags so it works without a Spotify token. This pass is now limited to audio-feature (energy/valence) deltas, which the Last.fm-tag path cant substitute for. What remained is energy, valence, tempo and acousticness. Last.fm does not expose those, so there is nothing to substitute with. ## Extended access does not bring them back Ruled out with sources on #2072. Since 2025-05-15 it requires a registered business entity and 250k MAUs, and the deprecation notice says extended mode **preserved** these endpoints for apps that already had it on 2024-11-27. It is not a door that opens later. ## Removed `enrichWithAudioFeatures` existed **twice**, in `candidateScorer.ts` and in `candidateFallback.ts`. Both are gone, along with `audioFeatures.ts`, `getAudioFeatures`, `getBatchAudioFeatures`, the `SpotifyAudioFeatures` / `SpotifyAudioFeatureConstraints` types, and the `currentFeatures` parameter threaded through the collector chain. ``` 15 files changed, 29 insertions(+), 1164 deletions(-) ``` ## Tests retargeted, not deleted Two regression tests used the removed fetchers only as a **vehicle** to exercise `spotifyFetch` shared `AbortSignal` (#1279) and its 429 retry (PR #808 / Greptile). Those behaviours still matter, so both now run against `searchSpotifyTrack` and carry a comment saying why. ## Verification ``` jest 3122 passed, 1 skipped, 243 suites tsc clean ``` <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Removes Spotify audio-features–based scoring because the endpoint now returns 403. Previously we boosted/penalized candidates using energy/valence/tempo/acousticness; in practice this path was a no-op (always null), so behavior does not change and scoring continues to rely on Last.fm tag–based genre-family checks. - Delete the enrichment path: remove `enrichWithAudioFeatures` (from candidate scorer and fallback) and `audioFeatures.ts`; drop `getAudioFeatures`, `getBatchAudioFeatures`, and the `SpotifyAudioFeatures`/`SpotifyAudioFeatureConstraints` types. - Simplify call sites: remove the `currentFeatures` parameter from candidate collection and replenisher; delete the related cache and the export in queue manipulation. - Keep infra tests: retarget AbortSignal deadline and 429-retry regressions to `searchSpotifyTrack`; update Retry-After tests to return a valid search payload and assert non-null results. Remove an unused `getArtistGenres` import flagged by CodeQL. - No rollout or user migration needed; compile-time errors will surface any stale imports. <sup>Written for commit 7bb2eff. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/2074?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
1 participant
Closes #2072. Stacked on #2071 — review that first; this targets its branch, not
main.Measured, with Lucky own credentials from inside lucky-bot
So
currentFeatureswas alwaysnullandenrichWithAudioFeaturesreturned its input unchanged on the first line. The 5000-entry LRU cache behind it, with a 24h TTL, could only ever holdnull.Option 3 turned out to be already done
The plan was to replace the Spotify signal with Last.fm tags. That replacement already exists:
candidateScorerscores genre-family overlap in-pass viacalculateGenreFamilyPenaltyon Last.fm tags, and the code says so at the old call site:What was left is energy, valence, tempo and acousticness. Last.fm does not expose those, so there is nothing to substitute with. The remaining honest move is removal.
Extended access does not bring them back
Ruled out with sources on #2072. Since 2025-05-15 it requires a registered business entity and 250k MAUs, and the deprecation notice says extended mode preserved these endpoints for apps that already had it on 2024-11-27. It is not a door that opens later.
Removed
enrichWithAudioFeaturesexisted twice, incandidateScorer.tsand incandidateFallback.ts. Both are gone, along withaudioFeatures.ts,getAudioFeatures,getBatchAudioFeatures, and theSpotifyAudioFeatures/SpotifyAudioFeatureConstraintstypes, plus thecurrentFeaturesparameter threaded through the collector chain.Tests retargeted, not deleted
Two regression tests used the removed fetchers only as a vehicle to exercise
spotifyFetchsharedAbortSignal(#1279) and its 429 retry (PR #808 / Greptile). Those behaviours still matter, so both now run againstsearchSpotifyTrackand carry a comment saying why.One test asserted
searchSpotifyTrackhad been called — but it reached that function only through the audio-features path, so it broke the moment the path went. It now asserts on the queue: the point is that a Spotify-linked user gets tracks, and that survives arms being retired. I had introduced that same brittleness in #2071 by asserting on an internal call; this is the correction.Verification
Summary by cubic
Removes the Spotify audio-features scoring path that always 403’d. Before: we attempted energy/valence/tempo/acousticness boosts;
currentFeatureswas null and the pass was a no-op. Now: delete the pass and fetchers; scoring relies on Last.fm tags and other signals. Side effect: fewer Spotify calls and simpler code with no change to recommendation outcomes.enrichWithAudioFeatures(both sites),audioFeatures.ts,getAudioFeatures,getBatchAudioFeatures, related types, and thecurrentFeaturesparameter/thread across collectors and replenisher.replenisher; keep the selected-candidate flow unchanged.searchSpotifyTrack; replace brittle internal-call assertion with a queue outcome assertion; no test deletions.v1/audio-features(extended access will not restore it).Written for commit 118ae20. Summary will update on new commits.