fix(stremio_mcp): verify playback stop and demote stale playing status - #24
Merged
Conversation
Stop no longer treats ADB KEYCODE_MEDIA_STOP delivery as success. It verifies the Stremio session is no longer playing, with pause+back and a bounded force-stop fallback when media-session stop is ignored. playback_status now corroborates claimed PLAYING with a started AudioTrack for the session owner, reporting stalled (without position extrapolation) for Exo-error/stale sessions while preserving real playing and paused results.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Validate and ship the committed reliable Stop and truthful playback-status fixes for stremio-mcp after live dogfooding on Android TV showed two product defects on the merged modernization PR head.
User goals and constraints from the task:
What Changed
tv_control playback stopnow runs a verified stop ladder instead of firingKEYCODE_MEDIA_STOPand claiming success: media-session dispatch + stop key, then pause + back, then a boundedam force-stop com.stremio.one, re-checking the Stremio media session after each step. Success is the post-condition (no active playback) and the call fails closed — with a truthful reason distinct from generic ADB failure text — when the session still plays, when the state is not positively parsed as stopped (BUFFERING/CONNECTING/unknown), or when the verification dump cannot be read.playback_statusparsing was split into a shared_read_session_statusthat scopes to Stremio's session block, recordsownerUid/updated/speed, and reports whether the dump succeeded. A claimedPLAYINGstate is now corroborated against a started Stremio-ownedAudioTrackindumpsys audio; without one it is demoted tostalledwith no position extrapolation. Unreadable dumps surface the ADB failure instead of an authoritative "No active media session found", and ERROR/STOPPED/NONE states are reported explicitly.Risk Assessment
✅ Low: The round-3 fix is a two-line propagation of the existing
dump_okflag into theplayback_statushandler with matching regression tests, and it closes the last outstanding auto-fix finding without affecting the stop ladder or status parsing.Testing
Ran the authoritative locked setup, full unit suite and source compilation (all green), then proved the two user-facing defects with fail-before/pass-after runs of the 12 new regression tests (11 fail on base, all pass on target). Because unit tests alone are not sufficient evidence, I additionally drove the real stdio MCP server end-to-end with a real MCP client against a scripted fake Android TV that reproduces the dogfooded firmware behaviour, capturing redacted CLI transcripts on both base and target: on base, stop reported "Playback: stop" while playback continued and a stale Exo-error session was reported as healthy
playingwith an invented advancing position; on target, stop escalates through dispatch/key/pause+back/force-stop, clears the session, andplayback_statusreports "No active media session found", while the stale session is truthfully demoted tostalledwith a frozen position. A third scenario where the device ignores every stop path shows the fix failing closed with an explicit "Stop failed: ..." message instead of a false success, and genuine play/pause/resume plus the native-adb command set are unchanged. No findings; the worktree is clean and all evidence lives in the dedicated evidence directory.Evidence: Redacted live MCP proof (before/after, all scenarios) — ready to paste into the PR body
Evidence: Full MCP transcript — target commit 71dc962
Evidence: Full MCP transcript — base commit 16d0934 (both defects visible)
Evidence: Fail-closed stop transcripts (base vs target)
Evidence: Regression fail-before / pass-after
### BEFORE (base 16d0934, new tests from 71dc962) Ran 12 tests FAILED (failures=3, errors=8) ### AFTER (71dc962) Ran 12 tests OK ### AFTER - full suite Ran 112 tests OKEvidence: STOP before/after, user-visible MCP tool output
Evidence: Exo-error stale session before/after
Evidence: Fake-ADB Android TV simulator used for the live proof
Evidence: MCP client driver script
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 7 issues found → auto-fixed (3) ✅
src/stremio_mcp.py:527-_is_playback_stoppedaccepts state"stopped", butget_playback_statusinitializesstateto"stopped"and only assigns it for PLAYING/PAUSED/ERROR/STOPPED/NONE. Any other real PlaybackState — most importantlyBUFFERING(6), alsoCONNECTING(8)/SKIPPING(9-11) — leaves the default, so_is_playback_stopped()returns True andmedia_stop()reports success while Stremio is mid-buffer and will resume playing seconds later. This fails open, contrary to the fail-closed requirement. Distinguish "parsed as stopped" from "no state parsed" (e.g. set state to "unknown"/None when no branch matches, and only treat explicit stopped/none as a stopped post-condition).src/stremio_mcp.py:506- Tier 2 callsmedia_pause()and then verifies with_is_playback_stopped(), which rejects state"paused". Pausing therefore produces exactly the state that makes its own verification fail, so tier 2 can only succeed ifnav_backalso makes the session go inactive (appbecomes None). In the common case stop escalates toam force-stop com.stremio.one, killing the app. The same is true when the user issues stop on an already-paused session: nothing is playing, yet the ladder runs to force-stop. Confirm this is the intended aggressiveness, or make the paused-after-our-own-pause case count as stopped.src/stremio_mcp.py:780-_stremio_audio_is_startedreturns False (→ demote tostalled) wheneverowner_uidis known and noAudioPlaybackConfigurationline containsu/pid:<uid>/, without distinguishing "owner present but no started track" from "no owner line seen at all". On any device/Android build whosedumpsys audioformats the owner differently, or where output is offloaded/tunneled and not surfaced as a media AudioTrack config, genuinely playing media is reported asstalledand position extrapolation is disabled — the failure mode the intent forbids ("without breaking genuine playing/paused results"). Returning None when no owner-attributable config line was seen at all would keep the Exo-error demotion while failing safe on unknown formats.src/stremio_mcp.py:635-get_playback_statusnow returns the empty status as soon as noPlayerMediaSession com.stremio.one/...header is found, and_is_playback_stoppedtreats a missingappas "nothing to stop". If playback is owned by a media session other than Stremio's own (external VLC, or a Stremio build whose session tag differs),media_stop()returns True on the first check while media keeps playing — the exact false-success the change is meant to eliminate. Consider having the stop post-condition require positive evidence of no playback (e.g. no active media session claiming PLAYING at all) rather than inferring it from the absence of a Stremio-named session.src/stremio_mcp.py:2114- Whenmedia_stop()fails on its post-condition, every ADB call succeeded, socontroller.last_failureis None and_adb_failure_textreturns "ADB failure (category=unknown): the operation did not complete." The user is told ADB failed when ADB worked fine and the truth is that the session is still playing. Return a distinct message for the still-playing post-condition failure so the reported failure is truthful about its cause.src/stremio_mcp.py:651- TheownerUid=search runs on every line of the scoped block and keeps the last match. If the next-session truncation regex (which requires exactly four leading spaces and a trailing(userId=N)) fails to match on some dumpsys layout, a following session's ownerUid silently becomes the audio-liveness target, misattributing AudioTrack ownership and demoting a healthy Stremio session. Capture the first ownerUid in the block instead.src/stremio_mcp.py:522-_is_playback_stoppedruns the fullget_playback_status, which issuesdumpsys media_session,dumpsys audio, optionallycat /proc/uptime, and alwaysdumpsys media.extractor. With up to three verification rounds that is roughly a dozen adb round trips plus 1.25s of fixed sleeps for a single stop. The extractor duration lookup and position extrapolation are irrelevant to the stop post-condition; a session-scoped liveness check would cut most of that latency.🔧 Fix: fix: fail closed on unparsed playback state during stop
3 issues (1 warning, 2 infos) still open:
src/stremio_mcp.py:626-_read_session_statusreads the dump viasend_shell_command, which discards_run_shell's success flag and returns "" for both "device unreachable / adb error" and "empty dump". On an empty result it returns the default status withapp=None, and_is_playback_stoppedtreats a missingappas stopped (line 542). So if the TV drops off the network mid-session,cmd media_session dispatch stopand the STOP keyevent both fail, the verification dump fails, andmedia_stop()returns True at tier 1 —tv_controlreports "Playback: stop" while media is still playing and ADB is broken. This is the same fail-open shape the change set out to remove, and it is now the only remaining unverified-success path. Use_run_shellhere (or thread its boolean out) and treat a failed dump as not-stopped so the stop post-condition fails closed with the real ADB failure text. Note this is distinct from the previously-ignored VLC/other-session finding: the trigger is a failed dump, not a differently-named session.src/stremio_mcp.py:314-last_stop_failure/_last_stop_stateare per-controller mutable state on the module-level singleton, andmedia_stopclears then sets them across several awaits. Two concurrenttv_controlstop calls on the same server can interleave so one call's failure text describes the other call's session state. The blast radius is a misleading message only (it mirrors the existinglast_failurepattern), but returning the reason frommedia_stopinstead of stashing it on the controller would remove the race entirely.src/stremio_mcp.py:711- The new catch-all branch collapses BUFFERING(6), CONNECTING(8) and the SKIPPING states intostate="unknown", which is correct for the fail-closed stop post-condition but is whatplayback_statusnow prints to the user (State: unknown) during ordinary buffering. Parsing BUFFERING(6)/CONNECTING(8) into their own non-stopped labels would keep the stop semantics identical while making the status output more informative.🔧 Fix: fix: fail closed when stop verification dump fails
1 warning still open:
src/stremio_mcp.py:2236-_read_session_statusnow distinguishes a failed dump (meta["dump_ok"] is False) from a successful dump with no Stremio session, and_is_playback_stoppeduses it — butget_playback_statusdiscards it, so theplayback_statustool still answers "No active media session found" when the ADB dump actually failed (device unreachable, adb error). That is the same false-negative class this change set out to eliminate, just on the read path instead of the stop path: the user is told nothing is playing when the server simply could not look. Surface the failure (e.g. propagatedump_okand return_adb_failure_text(controller)when the dump could not be read) instead of reporting an authoritative "no session".🔧 Fix: fix: report unreadable dump in playback_status truthfully
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
uv sync --lockeduv run --locked python -m unittest discover -s tests(112 tests, OK)uv run --locked python -m compileall -q src testsFail-before: restoredsrc/stremio_mcp.pyfrom base 16d0934 and ran the 12 new regression selectors — 3 failures + 8 errors; worktree restored afterwardsPass-after: same 12 selectors on 71dc962 — OK (tests.test_stremio_mcp.NativeAdbControllerTests.test_media_stop_*,test_stop_*,test_playback_status_*,test_buffering_session_is_not_reported_as_stopped)Manual end-to-end MCP session: realstremio_mcp.cli()server over stdio MCP v1 driven by anmcp.ClientSession, withADB_PATHpointed at a scripted fake Android TV that acceptsinput keyevent 86with rc=0 while keeping the session PLAYING (harness/drive_mcp.pyrun against both base and target sources)Manual fail-closed scenario: fake device ignores dispatch stop, key 86, pause+back ANDam force-stop(harness/stubborn.py, base vs target)Inspected the ADB command log the server actually issued to confirm native Platform Tools adb only — no new transport🔧 **Document** - 1 issue found → auto-fixed ✅
src/stremio_mcp.py:1774- Theplayback_statusMCP tool description still advertisesstate (playing/paused/stopped), but the change can now returnstalled,error,none, andunknown. This is a user-facing schema string in code rather than a documentation file or doc comment, so it was left untouched under the documentation-edit rule; a maintainer should update the enum-like list (and consider whether thetv_controlplaybackstopdescription at line 1752 should note that success is a verified post-condition).🔧 Fix: docs: align MCP tool descriptions with stop/status behavior
✅ Re-checked - no issues remain.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.