Repository navigation
fix: resolve device names and preserve opaque IDs - #85
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. Reviewed September 22, 2026, 8:03 AM ET / 12:03 UTC (Revision 6). ClawSweeper reviewWhat this changesResolve device names for Web API playback after a rejected selector, preserve opaque device IDs across engines, and remove playback-only parameters from unrelated mutations. Merge readiness✅ Ready for maintainer review The fix remains necessary on current main and v0.13.0. The earlier missing-ID finding is resolved, and the reviewed patch has no blocking correctness findings. Priority: P2 Review scores
Verification
How this fits togetherspogo routes CLI playback commands through Spotify Connect or Spotify’s Web API. Device selectors come from command flags or profile settings and determine which Spotify device receives playback controls. flowchart TD
A[CLI playback command] --> B[Configured device selector]
B --> C[Web API playback request]
C --> D{Selector rejected with 404?}
D -->|No| E[Return result]
D -->|Yes| F[Discover and match device]
F --> G[Retry with usable device ID]
G --> E
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep device resolution confined to playback, with unchanged raw-ID requests and one shared exact-ID-first matcher. Do we have a high-confidence way to reproduce the issue? Yes, from source: main sends a documented device name unchanged as device_id and also adds it to unrelated mutations. No live Spotify reproduction was executed during this review. Is this the best way to solve the issue? Yes. Playback-local resolution preserves successful opaque-ID requests, shares matching across engines, and avoids adding configuration or persisted state. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning medium; reviewed against 3d4edc230f5e. LabelsLabel changes: No label changes. Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
|
Addressed the nullable device ID finding in c7fa011:
Validated with the targeted regressions repeated 20 times, @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Try raw selectors before Web API name lookup, and share exact-ID-first device matching with Connect and device set. Cover all playback controls, raw-ID scope independence, absent IDs, case and whitespace preservation, and API error behavior. Co-authored-by: VACInc <3279061+VACInc@users.noreply.github.com>
Web API playback previously sent a documented device name such as
Desk Speakeras a raw ID, causing Spotify's404 Device not found. The generic request layer also added that playback-only selector to library and playlist mutations.Keep device selection in one playback helper. Pass the supplied selector through unchanged first; after a 404, resolve a case-insensitive exact device name and retry with its usable ID. This preserves arbitrary Spotify device IDs without guessing their format or requiring device-list permission for successful raw-ID commands. Never retry a named device with a missing ID, and preserve authentication, cooldown, cancellation, and other non-404 failures. Unrelated mutations no longer receive
device_id. Share exact-ID-first, case-insensitive-name matching with Connect anddevice set; reject named transfers with no usable ID. This also fixes existing case-folded ID matching that could select a different device.Validation: the original main fails both name-resolution and unrelated-mutation regressions. The initial PR also fails an opaque-ID regression; the finished implementation passes. Tests cover all nine playback controls, retained command parameters, unusable IDs, unknown devices, lookup failures, cancellation, and raw IDs without discovery. Full Linux race/coverage, lint, dead-code scan, docs build, and built CLI version/completion smoke checks run on Crabbox; platform and Go-floor coverage run in CI.
Thanks @VACInc for the original fix, tests, documentation, and reported live play/pause verification. Maintainer verification uses synthetic Spotify endpoints.