[SebTardif]

What Problem This Solves

sonos scene save read each room's volume and mute and ignored the errors. The zero values are volume 0 and mute off. The command still stored the scene and reported success. A later scene apply would turn that room down and unmute it.

A volume that the speaker really reports as 0 is still stored.

Evidence

Before this change, scene save Morning with the Kitchen GetVolume call returning timeout finished with no error:

error = <nil>, want "get volume for Kitchen"

The same happened when GetMute failed. After this change, that save returns the read error and does not write the scene. A Living Room that reports volume 0 still saves.

--- PASS: TestSceneSaveDoesNotStoreZeroWhenLevelReadFails/volume (0.00s)
--- PASS: TestSceneSaveDoesNotStoreZeroWhenLevelReadFails/mute (0.00s)
--- PASS: TestSceneSaveKeepsAReportedZeroVolume (0.00s)

Real behavior proof

• Behavior or issue addressed: A failed volume or mute read was saved as volume 0 and mute off, then applied later as if the room had been quiet.
• Real environment tested: macOS, Go, patched tree /tmp/pr-wave/sonoscli-f001 at 3067e5ce1f67. No Sonos speaker was on the network. The CLI command ran with a speaker client whose read returned timeout.
• Exact steps or command run after this patch: go test -count=1 -v -run 'TestSceneSaveDoesNotStoreZeroWhenLevelReadFails|TestSceneSaveKeepsAReportedZeroVolume' ./internal/cli/
• Evidence after fix: terminal output from that command:

--- PASS: TestSceneSaveDoesNotStoreZeroWhenLevelReadFails/volume (0.00s)
--- PASS: TestSceneSaveDoesNotStoreZeroWhenLevelReadFails/mute (0.00s)
--- PASS: TestSceneSaveKeepsAReportedZeroVolume (0.00s)
PASS
ok  	github.com/steipete/sonoscli/internal/cli	0.129s

• Observed result after fix: Kitchen volume and mute failures stop the save. The scene name is not stored. A reported volume of 0 for Living Room is stored.
• What was not tested: A live Sonos room whose RenderingControl call fails on the LAN.

Description

scene save returns get volume for <room> or get mute for <room> and leaves the previous scene file unchanged.

[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 stops scene saving when a room's volume or mute read fails, preserving previously saved scenes.

Example: Kitchen's GetVolume request times out during sonos scene save Morning.

• Before: The command reports success and stores Kitchen's volume as 0, which a later scene apply sends to the speaker.
• After: The command returns get volume for Kitchen with the underlying error and leaves the scene store unchanged.

Review scores

Measure	Result	What it means
Overall readiness	🧂 unranked krab (1/6)	The focused patch appears correct and its regression tests address distinct cases, but the required real-behavior evidence remains mock-only.
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: Yes · Fix scope: Complete
User problem: Saving a scene can silently replace real audio settings with volume 0 or mute off after a failed speaker read.
Reason: A saved snapshot should represent successfully read settings; preventing false success and incorrect later playback settings provides clear value with a narrow change.

Merge readiness

⛔ Blocked before merge - 1 item remains

This PR fixes a concrete bug still present in main and v0.3.5: failed audio reads can overwrite a scene with incorrect values; no introduced correctness defect was found.

Priority: P2
Reviewed head: 3067e5ce1f673ae56c23522715a1bea9474e5c2b

Before merge

• Add real behavior proof - The supplied terminal output exercises the scene command with fake speakers and storage, not the production Sonos transport or file store. Please provide terminal screenshots, logs, or live output from the patched CLI showing GetVolume and GetMute failures leave an existing scene file unchanged, plus a successful zero-volume save; include an upgrade from v0.3.5 with saved settings intact and redact IPs, keys, 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

• Missing end-to-end proof: Missing is a real CLI and transport run covering each read failure, unchanged existing storage after upgrading from v0.3.5, and successful saving of a reported volume of 0.

Agent review details

How this fits together

The scene CLI reads topology and per-room audio settings through the Sonos client, stores a named JSON snapshot, and later restores its grouping, volume, and mute values.

flowchart TD
 A[Scene save command] --> B[Discover visible rooms]
 B --> C[Read volume and mute]
 C --> D[Return room error]
 C --> E[Store complete scene]
 E --> F[Scene apply command]
 F --> G[Restore speaker settings]

Loading

Technical review

Best possible solution:

Keep the focused error checks in the existing scene-save owner so only complete, successfully read snapshots reach storage.

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

The source establishes the failure path: Sonos read errors return 0 or false, main ignores those errors, and scene apply later restores those values; no target code was executed during this read-only review.

Is this the best way to solve the issue?

Returning before the existing persistence call fixes the reported read-error bug without adding another owner, changing the scene format, or rejecting a legitimate volume of 0.

AGENTS.md: not found in the target repository.

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

Provenance checked

• Scene save read-error handling changes intended behavior with a stated reason (ecd9fca: The original commit added grouping and per-room audio snapshots; its readable implementation discards read errors, but no stated reason for that choice or associated PR was found.)
• Scene documentation keeps the original intent (b085324: The documentation site describes scenes as snapshots of grouping and per-room audio; the added failure semantics support that contract.)

Testing

Proof path: unit tests only.

Security

None.

Evidence

What I checked:

• Main and released behavior: At the main and v0.3.5 revision, scene save discards GetVolume and GetMute errors before storing their returned values; scene apply sends stored mute and volume values back to speakers. (internal/cli/scene.go:168, 63f57580a780)
• Focused repair: The introduced production hunks return wrapped read errors before the existing store.Put call, retain visible-room filtering, and accept successful zero-volume reads. (internal/cli/scene.go:168, 3067e5ce1f67)
• Supplied proof and discussion: The full PR body supplies passing test output using fake topology, speaker, and storage implementations and explicitly says no Sonos speaker was tested; both discussion comments are bot status messages, with no additional behavior proof or owner decision. (internal/cli/scene_test.go:191, 3067e5ce1f67)
• Persistence boundary: Scene types, JSON serialization, storage location, and apply behavior are unchanged; FileStore construction performs no write, and the failed-read path returns before persistence. (internal/scenes/store.go:25, 3067e5ce1f67)
• Feature history: The original scene implementation contains the ignored errors; subsequent scene filtering, output, tests, and documentation history consistently routes to Peter Steinberger. Local history traversal encountered unavailable older objects, but the original scene blob and GitHub creation patch were independently readable. (internal/cli/scene.go:168, ecd9fca3b972)

Likely related people:

• Peter Steinberger: Raw commit 6916839 adds internal/cli/scene.go:160 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.

Labels

Label changes:

• add P2: Failed reads can corrupt a saved preset and cause unintended volume or mute changes when applied, with impact limited to scene workflows.
• 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: Failed reads can corrupt a saved preset and cause unintended volume or mute changes when applied, with impact limited to scene workflows.
• 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 failure, preservation, and successful zero-volume evidence described in the proof assessment.

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, 1:46 PM ET / 18:46 UTC.