[SebTardif]

What Problem This Solves

sonos open, play-spotify without --enqueue, and SMAPI --open enqueue with play-now. When AddURIToQueue omits FirstTrackNumberEnqueued, those commands called Play and ignored its error, then returned success. Play does not select the new queue item. The command could exit 0 while the new item never started.

Favorites already treat a missing index as track 1 only because they clear the queue first. An ordinary enqueue does not, so track 1 can be an older item. The track is already on the queue when the position is missing, so the error has to say that playback did not start and how to play the queued item.

Evidence

Before this change, Spotify and SMAPI play-now with an empty FirstTrackNumberEnqueued returned no error:

--- FAIL: TestClientEnqueueSpotify_PlayNowMissingTrackNumber
    spotify_enqueue_test.go:224: error = <nil>
--- FAIL: TestEnqueueSMAPIItem_PlayNowMissingTrackNumber
    smapi_enqueue_test.go:97: error = <nil>

The speaker client answered Play with HTTP 200, so the old path reported success. After this change, both calls return this error and do not send Play or Seek:

playback was not started because the speaker did not report the new queue position; the track is already on the queue. Run sonos queue list, then sonos queue play <pos> for this speaker. Do not enqueue the track again

A response that includes track 7 still seeks that track and plays it.

Real behavior proof

• Behavior or issue addressed: Play-now reported success when the speaker omitted the enqueued track number, and the replacement error did not say the track was already queued.
• Real environment tested: macOS, Go, head 161dcb7. No Sonos speaker was on the network. A local HTTP speaker answered AddURIToQueue without FirstTrackNumberEnqueued and would have accepted Play.
• Exact steps or command run after this patch: go test -count=1 -run 'TestClientEnqueueSpotify_PlayNowMissingTrackNumber|TestEnqueueSMAPIItem_PlayNowMissingTrackNumber|TestStartEnqueuedPlaybackMissingPosition|TestClientEnqueueSpotify_PlayNowTrack|TestEnqueueSMAPIItem$' ./internal/sonos/
• Evidence after fix: that command exited 0. The missing-number cases return the error above and do not send Play or Seek.
• Observed result after fix: Spotify and SMAPI play-now say playback was not started, the track is already on the queue, and the next commands are sonos queue list and sonos queue play <pos>. A reported track number still selects that queue item.
• What was not tested: A live Sonos speaker that omits FirstTrackNumberEnqueued was not on the network.

Description

EnqueueSpotify and EnqueueSMAPIItem share startEnqueuedPlayback. A positive track number still uses playFromQueueTrack. A missing number is returned to the caller with the queue list and queue play commands.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs real behavior proof before merge.

What this changes

This PR returns an actionable error when Spotify or SMAPI play-now enqueues an item without receiving its queue position.

Example: Run sonos open spotify:track:abc123 while older items remain queued, and the speaker omits FirstTrackNumberEnqueued.

• Before: The client sends bare Play and returns success without selecting the new item.
• After: The client returns an error explaining that the item is queued and how to select it, without sending Play or Seek.

Review scores

Measure	Result	What it means
Overall readiness	🧂 unranked krab (1/6)	The focused patch resolves the prior finding and has no identified correctness defect, but mock-only evidence does not satisfy the external-PR behavior-proof gate.
Proof confidence	🧂 unranked krab (1/6)	Real behavior proof is necessary before merge. See Before merge.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Needs a maintainer decision · Fix scope: Complete
User problem: A play-now command can report success while the requested item never starts or older content resumes.
Reason: Preventing false success has clear value, but removing the owner-built fallback requires acceptance of the omitted-position compatibility change.

Merge readiness

⛔ Blocked before merge - 1 item remains

Keep this PR open: it addresses a current false-success path, and the updated error resolves the previous recovery-message finding.

Priority: P2
Reviewed head: 161dcb7a859e8926c30824dbcc7326e660fef126
Owner decision: Required. See Decision needed.

Decision needed

• Question: Should omitted-position firmware now receive an error after enqueue instead of the existing best-effort Play fallback?
• Recommendation: Approve explicit failure: Accept the changed exit status and suppressed Play request because the new item cannot safely be selected, retaining the queue recovery instructions.
• Why: This deliberately changes existing playback attempts and successful command exits; the contributor explains the safety benefit, but no owner approval of that compatibility tradeoff is recorded.

Before merge

• Add real behavior proof - The reported macOS run at head 161dcb7 exercises EnqueueSpotify and EnqueueSMAPIItem through mocked RoundTrippers; it shows neither a listening HTTP speaker nor a shipped CLI run. Provide redacted terminal output or transport logs showing omitted-position responses produce the recovery error without subsequent playback requests, existing queue entries survive an upgrade from v0.3.5, queue recovery works, and a positive-position response still selects and plays the new item. A real transport client against an injected HTTP fault is acceptable; redact IPs, API keys, phone numbers, and non-public endpoints. After adding proof, update the PR body, then request a new review with @clawsweeper re-review. The PR author or someone with repository write access can request it.

Findings

None.

Tests

• Low-value test internal/sonos/spotify_enqueue_test.go: Remove TestStartEnqueuedPlaybackMissingPosition: it directly repeats the helper error assertion already covered through both production enqueue methods.
• Missing end-to-end proof: Supply an after-upgrade CLI or real-transport trace from v0.3.5 showing omitted-position Spotify and SMAPI play-now, preserved existing queue entries, recovery, and a positive-position control; the body reports base-failing tests, but no such runtime scenario is shown.

Agent review details

How this fits together

CLI playback commands pass Spotify references or SMAPI items to the Sonos client, which enqueues them through SOAP and selects a reported queue position before playback.

flowchart TD
  A[CLI playback command] --> B[Spotify or SMAPI enqueue]
  B --> C[Speaker AddURIToQueue]
  C --> D[Shared playback decision]
  D --> E[Known position selects queue and plays]
  D --> F[Missing position returns recovery error]

Loading

Technical review

Best possible solution:

Keep one shared play-now decision that selects only a known queue position and otherwise preserves the queue with actionable recovery guidance.

Do we have a high-confidence way to reproduce the issue?

The current-main source establishes the failure path: an omitted field becomes position zero, then both play-now implementations discard the result of bare Play and return success; no runtime reproduction was run during this read-only review.

Is this the best way to solve the issue?

The shared helper is a focused repair using the existing queue playback owner, and it correctly avoids borrowing the favorites-only track-1 fallback.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 63f57580a780.

Provenance checked

• Spotify missing-position fallback changes intended behavior with a stated reason (12e39d3: The original implementation explicitly attempted Play when the speaker omitted the position; no associated PR explanation was returned by the commit lookup.)
• Generic SMAPI missing-position fallback changes intended behavior with a stated reason (ac69aa2: Generic service enqueue retained the same best-effort Play behavior while adding support for non-Spotify service items, as corroborated by SMAPI open/enqueue appears Spotify-only; 网易云音乐/QQ音乐 browse+search work but playback fails #7.)

Testing

Proof path: unit tests only.

Security

None.

Evidence

What I checked:

• Current main retains the false-success fallback: Both enqueue implementations call Play and discard its error when play-now receives no positive queue position; AddURIToQueue explicitly returns zero without error when the response omits the field. (internal/sonos/spotify.go:129, 63f57580a780)
• Previous finding resolved: Comparison with the previously reviewed head shows the error now explains the completed enqueue, names queue list and queue play, and warns against enqueueing again. (internal/sonos/queue.go:78, 161dcb7a859e)
• Supplied proof uses mocked transport: The full PR body reports package-test execution; the named tests install roundTripperFunc rather than using a real HTTP transport or shipped CLI. The body and contributor comment supply no separate runtime trace. (internal/sonos/spotify_enqueue_test.go:205, 161dcb7a859e)
• Favorites establish a distinct safe fallback: fix(favorites): play container favorites via the queue #14 explains why bare Play can resume the wrong transport and why track 1 is safe only after clearing the queue; this PR preserves that favorites behavior. (internal/sonos/favorites.go:76, 63f57580a780)
• Feature history and compatibility intent: Main history attributes the original Spotify fallback and generic SMAPI enqueue work to Peter Steinberger, subsequent SMAPI metadata work to Sengo Zhao, and shared queue playback work to Bero. Historical creation-boundary blobs are unavailable locally; the supplied provenance and readable main blobs establish the relevant behavior. (internal/sonos/smapi_enqueue.go:45, 63f57580a780)

Likely related people:

• Peter Steinberger: Raw commit ac69aa2 adds internal/sonos/smapi_enqueue.go:40 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: ac69aa26f251; files: internal/sonos/smapi_enqueue.go)
• Bero: Raw commit b7cbf54 adds internal/sonos/queue.go:72 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: b7cbf54d2500; files: internal/sonos/queue.go)
• Sengo Zhao: Raw commit 5054fdf adds internal/sonos/smapi_enqueue.go:36 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: 5054fdf86cab; files: internal/sonos/smapi_enqueue.go)

Preflight source

Source unknown. Source comparison at the observed PR snapshot only. Completion is self-reported; body, policy, tests, execution, and report bytes are not verified.

Labels

Label changes:

No label changes.

Label justifications:

• P2: The false-success path affects playback when a speaker omits the newly enqueued position, with limited conditional scope.
• merge-risk: 🚨 compatibility: Existing omitted-position responses will stop triggering Play and will produce a nonzero command result after enqueue.
• rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

History

Review history (1 earlier review cycle)

• reviewed 2026-11-06T20:11:34.817Z sha 81b8868 :: needs real behavior proof before merge. :: [P2] Explain the completed enqueue and how to recover

Reviewed November 7, 2026, 6:01 PM ET / 23:01 UTC (Revision 2).

[SebTardif]

@clawsweeper re-review

Head 161dcb7 says playback was not started, the track is already on the queue, and the next commands are sonos queue list and sonos queue play .

[clawsweeper]

🦞👀
Exact review queued.

Re-review progress:

• State: Complete
• Detail: The durable review result and its route handoff completed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/42423404973
• Updated: 2026-11-07T23:03:05.798Z