[SebTardif]

What Problem This Solves

sonos scene apply ignored errors from LeaveGroup, SetMute, and SetVolume, then reported success. Group joins in the same command already returned their errors. A failed ungroup or volume restore looked like a completed scene.

Evidence

Before this change, scene apply Work finished with no error when room A failed to leave the group, set mute, or set volume:

error = <nil>, want "leave group for A"
error = <nil>, want "set mute for A"
error = <nil>, want "set volume for A"

After this change, those three applies return the speaker error and do not print the success record. A scene whose speaker calls succeed still ungroups, joins, and restores volume.

--- PASS: TestSceneApplyReportsLeaveAndLevelErrors/leave (0.00s)
--- PASS: TestSceneApplyReportsLeaveAndLevelErrors/mute (0.00s)
--- PASS: TestSceneApplyReportsLeaveAndLevelErrors/volume (0.00s)
--- PASS: TestSceneApplyUngroupsJoinsAndRestoresAudio (0.00s)

Real behavior proof

• Behavior or issue addressed: A failed ungroup, mute, or volume restore during scene apply was reported as success.
• Real environment tested: macOS, Go, patched tree /tmp/pr-wave/sonoscli-f002 at bd19c2c8ec48. No Sonos speaker was on the network. The CLI command ran with a speaker client that returned busy.
• Exact steps or command run after this patch: go test -count=1 -v -run 'TestSceneApplyReportsLeaveAndLevelErrors|TestSceneApplyUngroupsJoinsAndRestoresAudio' ./internal/cli/
• Evidence after fix: terminal output from that command:

--- PASS: TestSceneApplyUngroupsJoinsAndRestoresAudio (0.00s)
--- PASS: TestSceneApplyReportsLeaveAndLevelErrors/leave (0.00s)
--- PASS: TestSceneApplyReportsLeaveAndLevelErrors/mute (0.00s)
--- PASS: TestSceneApplyReportsLeaveAndLevelErrors/volume (0.00s)
PASS
ok  	github.com/steipete/sonoscli/internal/cli	0.202s

• Observed result after fix: Leave, mute, and volume failures stop scene apply with leave group for A, set mute for A, or set volume for A. The JSON success record is not written. A successful apply still restores grouping and levels.
• What was not tested: A live Sonos room whose group or rendering call fails on the LAN.

Description

scene apply returns the first ungroup, mute, or volume error, including the room name. Join errors were already returned.

[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 makes scene application return room-specific ungroup, mute, and volume errors instead of reporting success.

Example: During sonos scene apply Work, room A's SetVolume call returns busy.

• Before: The error is discarded and the command writes a success record.
• After: The command returns set volume for A: busy, skips subsequent restoration, and writes no success record.

Review scores

Measure	Result	What it means
Overall readiness	🧂 unranked krab (1/6)	The focused patch has no definite introduced correctness defect, but required behavior proof is mock-only and the failure policy awaits owner direction.
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 scene can report successful restoration even when a room rejects ungrouping, mute, or volume changes.
Reason: Reporting failure has clear value, but stopping all remaining work changes the existing best-effort contract and needs the feature owner's direction.

Merge readiness

⛔ Blocked before merge - 1 item remains

This PR addresses errors still silently discarded on main and in v0.3.5; the owner should settle its change from best-effort restoration to stopping at the first failure.

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

Decision needed

• Question: Should scene apply stop at the first ungroup or audio error, or finish independent restoration work and then report failures?
• Recommendation: Preserve best-effort progress: Have the contributor collect room-specific failures, continue restoration where safe, and return a final error without a success record.
• Why: The PR explains false success, but immediate termination changes the owner's documented best-effort contract and is not necessary to report failure.

Before merge

• Add real behavior proof - The supplied terminal output exercises the scene command with fake speaker, topology, and storage owners, so it does not prove the production transport or shipped CLI behavior; provide terminal screenshots, logs, or live output showing LeaveGroup, SetMute, and SetVolume failures produce nonzero exit status without success output, and a successful restore from an existing v0.3.5 scene, with IPs, keys, and non-public endpoints redacted. 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

• Missing end-to-end proof: Missing is a patched CLI run through the production Sonos transport showing each failure returns nonzero without success output, plus successful restoration using an existing v0.3.5 scene.

Agent review details

How this fits together

The scene CLI reads saved grouping and audio settings, resolves current room addresses, and sends grouping and rendering commands through the Sonos SOAP client.

flowchart TD
 A[Scene apply command] --> B[Read saved scene]
 B --> C[Resolve current topology]
 C --> D[Ungroup and rebuild groups]
 D --> E[Restore mute and volume]
 D --> F[Return room error]
 E --> F
 E --> G[Write success record]

Loading

Technical review

Best possible solution:

Report restoration failures through the existing scene command while preserving best-effort progress unless the feature owner approves stopping immediately.

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

Source establishes that transport errors reach these speaker methods and are discarded on main; the submitted tests exercise each path, but no target code was executed during this read-only review.

Is this the best way to solve the issue?

The checks directly prevent false success with minimal production changes, but collecting failures could solve that problem while retaining the documented best-effort behavior.

AGENTS.md: not found in the target repository.

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

Provenance checked

• Apply-time error handling changes intended behavior with a stated reason (ecd9fca: The original scene feature deliberately performs grouping followed by audio restoration and discards these errors; its commit message gives no specific explanation, and the commit-to-PR lookup returned no associated PR.)
• Best-effort documentation changes intended behavior with a stated reason (b085324: The documentation site explicitly describes best-effort, idempotent restoration; the PR explains replacing silent success with errors but does not establish owner approval for immediate termination.)

Testing

Proof path: unit tests only.

Security

None.

Evidence

What I checked:

• Current main still discards failures: The main and v0.3.5 revision ignores LeaveGroup, SetMute, and SetVolume errors before writing scene.apply success; JoinGroup errors already propagate. (internal/cli/scene.go:288, 63f57580a780)
• Introduced change: The pinned diff adds three error checks in the existing command owner, updates documentation and changelog, and adds three distinct failure cases using existing test doubles. (internal/cli/scene.go:288, bd19c2c8ec48)
• Existing best-effort contract: The documentation introduced by Peter Steinberger describes apply as best-effort and idempotent; the command reference retains that wording. The new returns stop later rooms and phases, and no maintainer approval of that direction appears in the discussion or reviews. (docs/scenes.md:26, b08532464f8c)
• Proof and discussion inspected: The full PR body supplies passing go test output with fake speaker, topology, and storage implementations, explicitly without a Sonos speaker; both comments are bot status updates and reviews are empty. The only reported successful check is GitGuardian Security Checks. (internal/cli/scene_test.go:276, bd19c2c8ec48)
• Feature ownership and provenance: The readable original scene implementation contains the discarded errors; subsequent filtering, output, test, and documentation history consistently points to Peter Steinberger. No associated PR explaining the original error policy was returned. Some extended local history searches encountered unavailable older blobs. (internal/cli/scene.go:288, ecd9fca3b972)
• Related work is distinct: fix: do not save a scene when volume or mute cannot be read #32 addresses discarded reads while saving scenes; this PR addresses discarded writes while applying them, so neither replaces the other. (internal/cli/scene.go:168, 63f57580a780)

Likely related people:

• Peter Steinberger: Raw commit 6916839 adds internal/cli/scene.go:278 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 6916839319c8; files: internal/cli/scene.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.

Review metrics

Metric	Value	Why it matters
Production and test changes	Production +9/-3; tests +76/-3	Three focused checks reuse existing owners; the added test cases distinguish ungroup, mute, and volume failures.

Labels

Label changes:

• add P2: False success affects the bounded scene-restore workflow without evidence of an urgent broader regression.
• add merge-risk: 🚨 compatibility: A single room failure now stops later restoration that existing best-effort workflows previously attempted.
• add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
• add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Label justifications:

• P2: False success affects the bounded scene-restore workflow without evidence of an urgent broader regression.
• merge-risk: 🚨 compatibility: A single room failure now stops later restoration that existing best-effort workflows previously attempted.
• 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.

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Provide the production CLI and transport evidence described in the proof assessment.
• Resolve whether restoration should continue after individual failures.

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.

Reviewed November 6, 2026, 2:12 PM ET / 19:12 UTC.