Stop only the test video after the onboarding playback window

External review of the 2.0.0~beta1 package: play_test_video stopped
playback blindly five seconds after play() — wall time, not a playback
handle (verbatim legacy behavior). If the test video failed to open, or
the user started something else inside the window, the blind stop killed
whatever was playing instead.

Guard the stop on the playing item still being the bundled test video:
separator/case-normalized path comparison, with the isPlaying/
getPlayingFile race against a natural stop reading as "not our video"
rather than unwinding the script. The rest of the flow (completed toast,
settings reopen) is unchanged.

New unit suite pins the guard decisions and the two flow shapes (happy
path still stops the test video; playback started inside the window is
spared).

Co-authored-by: Claude <noreply@anthropic.com>

Author: matthane

resources/lib/aom/onboarding.py

@@ -19,6 +19,7 @@
 macros — a Phase 7 work item); error toasts pass Gui a custom title/icon.
 """
 
+import os
 import sys
 
 import xbmc
@@ -84,13 +85,40 @@ def play_test_video(self):
             duration_ms=10000)
 
         xbmc.sleep(5000)
-        xbmc.Player().stop()
+        self._stop_if_still_test_video()
 
         self._gui.notification(
             self._gui.localized(STRING_TEST_VIDEO_COMPLETED),
             duration_ms=10000)
         xbmc.executebuiltin(f'Addon.OpenSettings({ADDON_ID})')
 
+    def _stop_if_still_test_video(self):
+        """Stop playback only while the test video is still the playing item.
+
+        The stop fires 5s after play() on wall time, not on a playback
+        handle: by then the test video may have failed to open, or the user
+        may have started something else, and a blind ``Player().stop()``
+        would kill that instead. Paths compare separator/case-normalized
+        (Kodi reports either slash direction on Windows), and the
+        isPlaying/getPlayingFile pair can race a natural stop, so a raise
+        reads as "not our video".
+        """
+        player = xbmc.Player()
+        try:
+            playing = player.getPlayingFile() if player.isPlaying() else None
+        except Exception:
+            playing = None
+        if playing is None:
+            self._log("AOM_Onboarding: test video no longer playing; "
+                      "nothing to stop", xbmc.LOGDEBUG)
+            return
+        if (os.path.normcase(os.path.normpath(playing))
+                != os.path.normcase(os.path.normpath(self._test_video_path))):
+            self._log(f"AOM_Onboarding: another item is playing ({playing}); "
+                      f"leaving it alone", xbmc.LOGDEBUG)
+            return
+        player.stop()
+
     def bypass_test_video(self):
         """Clear new_install without playing the test video.
 

tests/unit/test_onboarding.py

@@ -0,0 +1,126 @@
+"""Unit tests for aom.onboarding's test-video stop guard.
+
+The 5s stop in ``play_test_video`` fires on wall time, not on a playback
+handle, so it must verify the bundled test video is still the playing item
+before stopping (a blind ``Player().stop()`` could kill playback the user
+started inside the window — the legacy behavior).
+
+Kodi is faked via Kodistubs; ``xbmc.Player`` is monkeypatched with a
+scriptable fake so the guard's decision is pinned without real playback.
+"""
+
+import pytest
+
+import resources.lib.aom.onboarding as onboarding
+
+
+class FakePlayer:
+    """Scriptable xbmc.Player: what is playing (or a raise), and the stops."""
+
+    def __init__(self):
+        self.playing_file = None      # None = nothing playing
+        self.raises = False           # getPlayingFile raises (stop race)
+        self.played = []
+        self.stopped = 0
+
+    def play(self, path):
+        self.played.append(path)
+        self.playing_file = path
+
+    def isPlaying(self):
+        return self.raises or self.playing_file is not None
+
+    def getPlayingFile(self):
+        if self.raises:
+            raise RuntimeError('XBMCAddon: no file playing')
+        return self.playing_file
+
+    def stop(self):
+        self.stopped += 1
+        self.playing_file = None
+
+
+TEST_PATH = 'addons/script.audiooffsetmanager/resources/media/test-video.mp4'
+
+
+@pytest.fixture
+def rig(monkeypatch):
+    """(onboarding instance, fake player): one shared player instance so the
+    guard sees the same state play() produced."""
+    player = FakePlayer()
+    monkeypatch.setattr(onboarding.xbmc, 'Player', lambda: player)
+    onb = onboarding._Onboarding()
+    onb._test_video_path = TEST_PATH
+    return onb, player
+
+
+class TestStopGuard:
+
+    def test_stops_while_test_video_still_playing(self, rig):
+        onb, player = rig
+        player.playing_file = TEST_PATH
+        onb._stop_if_still_test_video()
+        assert player.stopped == 1
+
+    def test_path_comparison_is_separator_normalized(self, rig):
+        # Kodi may report the path with redundant or mixed separators; the
+        # comparison is normpath/normcase-based, not string equality.
+        onb, player = rig
+        player.playing_file = TEST_PATH.replace('/resources/',
+                                                '//resources/./')
+        onb._stop_if_still_test_video()
+        assert player.stopped == 1
+
+    def test_leaves_other_playback_alone(self, rig):
+        # The user started something else inside the 5s window: never stop it.
+        onb, player = rig
+        player.playing_file = 'videodb://movies/titles/42'
+        onb._stop_if_still_test_video()
+        assert player.stopped == 0
+
+    def test_no_stop_when_nothing_is_playing(self, rig):
+        # The test video failed to open (or already ended): nothing to stop.
+        onb, player = rig
+        player.playing_file = None
+        onb._stop_if_still_test_video()
+        assert player.stopped == 0
+
+    def test_raise_reads_as_not_ours(self, rig):
+        # isPlaying/getPlayingFile can race a natural stop; a raise must be
+        # treated as "not our video", never propagate out of the script.
+        onb, player = rig
+        player.raises = True
+        onb._stop_if_still_test_video()
+        assert player.stopped == 0
+
+
+class TestPlayFlowWiring:
+
+    def test_happy_path_plays_then_stops_the_test_video(self, rig, monkeypatch):
+        # The full flow still stops the test video when it is (still) the
+        # playing item — the guard replaced the blind stop, not the stop.
+        onb, player = rig
+        monkeypatch.setattr(onboarding.xbmcvfs, 'exists', lambda path: True)
+        monkeypatch.setattr(onboarding.xbmc, 'sleep', lambda ms: None)
+        monkeypatch.setattr(onboarding.xbmc, 'executebuiltin', lambda cmd: None)
+
+        onb.play_test_video()
+
+        assert player.played == [onb._test_video_path]
+        assert player.stopped == 1
+
+    def test_flow_spares_playback_started_inside_the_window(self, rig,
+                                                            monkeypatch):
+        # Regression pin for the review finding: the user starts a different
+        # item during the 5s wait; the flow must not stop it.
+        onb, player = rig
+        monkeypatch.setattr(onboarding.xbmcvfs, 'exists', lambda path: True)
+        monkeypatch.setattr(
+            onboarding.xbmc, 'sleep',
+            lambda ms: player.play('videodb://movies/titles/42'))
+        monkeypatch.setattr(onboarding.xbmc, 'executebuiltin', lambda cmd: None)
+
+        onb.play_test_video()
+
+        assert player.stopped == 0
+        assert player.playing_file == 'videodb://movies/titles/42'