fix(stop): rely on bus bridge for legacy mycroft.stop; drain active_skills

JarbasAl · claude · JarbasAl · commit c880937e8205 · 2026-06-30T17:42:28.000+01:00
Address CodeRabbit review on #777 (OVOS-STOP-1 conformance): - handle_global_stop: drop the hand-rolled `mycroft.stop` emit. The spec-tools MIGRATION_MAP bridges `ovos.stop -> mycroft.stop` and the bus re-emits the legacy counterpart by default (emit_legacy ON during the migration window), so the manual emit double-delivered to anyone on both topics. Same fix as ovos-audio #171. Also switch the bare `ovos.utterance.handled` string to `SpecMessage.UTTERANCE_HANDLED` for a single source of truth (the only other spec-topic member in this file; `mycroft.*` / `ovos.skills.converse.force_timeout` are not SpecMessage members and stay as strings). - _drain_global_stop_session: also clear `session.active_skills` (the legacy recency list `get_active_skills` reads for stop-target routing), atomically with active_handlers/converse_handlers/response_mode. Left stale, a committed global_stop could route a later targeted stop to a skill it already drained. The two CI/pyproject nitpicks (dead bus-client git-ref + floor bump) were already resolved upstream by 851c6e1 + 87b940d. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
diff --git a/ovos_core/intent_services/stop_service.py b/ovos_core/intent_services/stop_service.py
@@ -79,22 +79,19 @@ def handle_global_stop(self, message: Message) -> None:
         OVOS-STOP-1 §5.3: the global-stop handler emits the spec broadcast
         ``ovos.stop`` (``SpecMessage.STOP``).
 
-        Back-compat: the MIGRATION_MAP bus bridge only re-delivers ``ovos.stop``
-        on the legacy ``mycroft.stop`` topic when the bridge's legacy direction is
-        enabled, which is NOT guaranteed (it is an opt-in on MessageBusClient and
-        off in the pure-spec path). Skills have NOT migrated their stop handler —
-        ovos-workshop still subscribes ONLY ``mycroft.stop`` — so a spec-only
-        broadcast would silently fail to stop them. We therefore also emit the
-        legacy ``mycroft.stop`` directly until the skill side migrates, mirroring
-        the back-compat per-skill ``{skill_id}.stop.ping`` kept in
-        ``_collect_stop_skills``. The two topics target disjoint subscriber sets
-        (spec vs un-migrated), so this is not a double broadcast to any one skill.
+        Back-compat: ``mycroft.stop ↔ ovos.stop`` is a payload-compatible rename
+        in the spec-tools MIGRATION_MAP, and the bus bridge re-delivers a spec
+        emit on its legacy counterpart by default (``emit_legacy`` defaults ON
+        during the migration window). So emitting ``ovos.stop`` ALSO reaches
+        un-migrated skills still listening on ``mycroft.stop`` — without us
+        hand-rolling a second emit. A manual ``mycroft.stop`` here would
+        double-deliver to anyone subscribed on both topics, so we rely on the
+        bridge (same fix as ovos-audio #171).
         """
         with HandlerLifecycle(self.bus, message,
                               skill_id="stop.openvoiceos",
                               data={"name": "StopService.handle_global_stop"}):
             self.bus.emit(message.forward(SpecMessage.STOP))
-            self.bus.emit(message.forward("mycroft.stop"))
 
     @staticmethod
     def _drain_global_stop_session(session) -> "Session":
@@ -105,13 +102,17 @@ def _drain_global_stop_session(session) -> "Session":
 
         - ``active_handlers``  → ``[]``  (§5.2 / §6.2)
         - ``converse_handlers`` → ``[]`` (§5.2 / §6.2, OVOS-CONVERSE-1 §2.1)
+        - ``active_skills``    → ``[]``  (legacy recency list read by
+          ``get_active_skills`` → stop-target routing; left stale it could
+          route a later targeted stop to a skill this global_stop drained)
         - ``response_mode``    → absent  (§5.2 / §6.1)
 
-        All three are cleared atomically at match time so the drained state is
+        All four are cleared atomically at match time so the drained state is
         committed before dispatch (PIPELINE-1 §4.2).
         """
         session.active_handlers = []
         session.converse_handlers = []
+        session.active_skills = []
         session.clear_response_mode()
         return session
 
diff --git a/test/end2end/test_stop.py b/test/end2end/test_stop.py
@@ -179,9 +179,6 @@ def _run_exact(self, namespace):
                     Message("mycroft.skill.handler.start",
                             {"name": "StopService.handle_global_stop"}),
                     Message(STOP_BROADCAST, {}),  # OVOS-STOP-1 §5.3 spec broadcast
-                    # back-compat: handle_global_stop also emits the legacy
-                    # mycroft.stop directly for un-migrated skills
-                    Message("mycroft.stop", {}),
                     Message("mycroft.skill.handler.complete",
                             {"name": "StopService.handle_global_stop"}),
 
@@ -263,9 +260,6 @@ def _run_not_exact_med(self, namespace):
                     Message("mycroft.skill.handler.start",
                             {"name": "StopService.handle_global_stop"}),
                     Message(STOP_BROADCAST, {}),  # OVOS-STOP-1 §5.3 spec broadcast
-                    # back-compat: handle_global_stop also emits the legacy
-                    # mycroft.stop directly for un-migrated skills
-                    Message("mycroft.stop", {}),
                     Message("mycroft.skill.handler.complete",
                             {"name": "StopService.handle_global_stop"}),
 
@@ -430,10 +424,6 @@ def make_it_count():
                         {"skill_id": "stop.openvoiceos"}),
                 Message(STOP_BROADCAST, {},
                         {"skill_id": "stop.openvoiceos"}),
-                # back-compat: handle_global_stop also emits the legacy mycroft.stop
-                # directly — the un-migrated count skill listens there and stops
-                Message("mycroft.stop", {},
-                        {"skill_id": "stop.openvoiceos"}),
                 Message("mycroft.skill.handler.complete",
                         {"name": "StopService.handle_global_stop"},
                         {"skill_id": "stop.openvoiceos"}),
diff --git a/test/end2end/test_stop_refactor.py b/test/end2end/test_stop_refactor.py
@@ -128,13 +128,11 @@ def _run_global_stop_voc_no_active_skills(self, namespace):
                 message,
                 Message("stop.openvoiceos.activate", {}),
                 Message("stop:global", {}),
-                # StopService wraps the global-stop handler in HandlerLifecycle
+                # OVOS-STOP-1 §5.3: global-stop handler emits spec broadcast only.
+                # Legacy compatibility is provided by the bus bridge (MIGRATION_MAP).
                 Message("mycroft.skill.handler.start",
                         {"name": "StopService.handle_global_stop"}),
-                Message(STOP_BROADCAST, {}),  # OVOS-STOP-1 §5.3 spec broadcast
-                # back-compat: handle_global_stop also emits the legacy mycroft.stop
-                # directly for un-migrated skills (no spec->legacy bridge guaranteed)
-                Message("mycroft.stop", {}),
+                Message(STOP_BROADCAST, {}),  # OVOS-STOP-1 §5.3 spec broadcast (bridged to legacy mycroft.stop)
                 Message("mycroft.skill.handler.complete",
                         {"name": "StopService.handle_global_stop"}),
                 Message(SpecMessage.UTTERANCE_HANDLED, {}),
@@ -175,13 +173,11 @@ def _run_stop_voc_exact_still_works(self, namespace):
                 message,
                 Message("stop.openvoiceos.activate", {}),
                 Message("stop:global", {}),
-                # StopService wraps the global-stop handler in HandlerLifecycle
+                # OVOS-STOP-1 §5.3: global-stop handler emits spec broadcast only.
+                # Legacy compatibility is provided by the bus bridge (MIGRATION_MAP).
                 Message("mycroft.skill.handler.start",
                         {"name": "StopService.handle_global_stop"}),
-                Message(STOP_BROADCAST, {}),  # OVOS-STOP-1 §5.3 spec broadcast
-                # back-compat: handle_global_stop also emits the legacy mycroft.stop
-                # directly for un-migrated skills (no spec->legacy bridge guaranteed)
-                Message("mycroft.stop", {}),
+                Message(STOP_BROADCAST, {}),  # OVOS-STOP-1 §5.3 spec broadcast (bridged to legacy mycroft.stop)
                 Message("mycroft.skill.handler.complete",
                         {"name": "StopService.handle_global_stop"}),
                 Message(SpecMessage.UTTERANCE_HANDLED, {}),
@@ -242,13 +238,11 @@ def _run_global_stop_voc_with_active_skill(self, namespace):
                 message,
                 Message("stop.openvoiceos.activate", {}),
                 Message("stop:global", {}),
-                # StopService wraps the global-stop handler in HandlerLifecycle
+                # OVOS-STOP-1 §5.3: global-stop handler emits spec broadcast only.
+                # Legacy compatibility is provided by the bus bridge (MIGRATION_MAP).
                 Message("mycroft.skill.handler.start",
                         {"name": "StopService.handle_global_stop"}),
-                Message(STOP_BROADCAST, {}),  # OVOS-STOP-1 §5.3 spec broadcast
-                # back-compat: handle_global_stop also emits the legacy mycroft.stop
-                # directly for un-migrated skills (no spec->legacy bridge guaranteed)
-                Message("mycroft.stop", {}),
+                Message(STOP_BROADCAST, {}),  # OVOS-STOP-1 §5.3 spec broadcast (bridged to legacy mycroft.stop)
                 Message("mycroft.skill.handler.complete",
                         {"name": "StopService.handle_global_stop"}),
                 Message(SpecMessage.UTTERANCE_HANDLED, {}),
@@ -416,13 +410,11 @@ def _run_stop_service_is_not_a_skill(self, namespace):
                 message,
                 Message("stop.openvoiceos.activate", {}),
                 Message("stop:global", {}),
-                # StopService wraps the global-stop handler in HandlerLifecycle
+                # OVOS-STOP-1 §5.3: global-stop handler emits spec broadcast only.
+                # Legacy compatibility is provided by the bus bridge (MIGRATION_MAP).
                 Message("mycroft.skill.handler.start",
                         {"name": "StopService.handle_global_stop"}),
-                Message(STOP_BROADCAST, {}),  # OVOS-STOP-1 §5.3 spec broadcast
-                # back-compat: handle_global_stop also emits the legacy mycroft.stop
-                # directly for un-migrated skills (no spec->legacy bridge guaranteed)
-                Message("mycroft.stop", {}),
+                Message(STOP_BROADCAST, {}),  # OVOS-STOP-1 §5.3 spec broadcast (bridged to legacy mycroft.stop)
                 Message("mycroft.skill.handler.complete",
                         {"name": "StopService.handle_global_stop"}),
                 Message(SpecMessage.UTTERANCE_HANDLED, {}),
diff --git a/test/unittests/test_stop_service.py b/test/unittests/test_stop_service.py
@@ -18,6 +18,7 @@
 
 from ovos_bus_client.message import Message
 from ovos_bus_client.session import Session, SessionManager, UtteranceState
+from ovos_spec_tools.messages import SpecMessage
 from ovos_utils.fakebus import FakeBus
 
 from ovos_core.intent_services.stop_service import StopService
@@ -548,9 +549,10 @@ class TestBusHandlers(unittest.TestCase):
 
     def test_handle_global_stop_emits_ovos_stop(self):
         # OVOS-STOP-1 §5.3: global-stop handler emits the spec topic ``ovos.stop``.
-        # Back-compat: it ALSO emits the legacy ``mycroft.stop`` directly, because the
-        # spec->legacy bus bridge is not guaranteed (opt-in / off on the pure-spec
-        # path) and skills have not migrated their stop handler off ``mycroft.stop``.
+        # Back-compat to un-migrated ``mycroft.stop`` listeners is provided by the
+        # bus MIGRATION_MAP bridge (emit_legacy ON by default), NOT a manual
+        # second emit — hand-rolling ``mycroft.stop`` here would double-deliver to
+        # anyone on both topics (cf. ovos-audio #171).
         svc = _make_service()
         emitted = []
         svc.bus.emit = lambda m: emitted.append(m)
@@ -559,10 +561,9 @@ def test_handle_global_stop_emits_ovos_stop(self):
         types = [m.msg_type for m in emitted]
         self.assertIn("mycroft.skill.handler.start", types)
         self.assertIn("ovos.stop", types)
-        self.assertIn("mycroft.stop", types)
         self.assertIn("mycroft.skill.handler.complete", types)
-        # the spec broadcast is emitted before the legacy back-compat one
-        self.assertLess(types.index("ovos.stop"), types.index("mycroft.stop"))
+        # the handler does NOT hand-roll a legacy emit; the bridge mirrors it
+        self.assertNotIn("mycroft.stop", types)
 
     def test_handle_skill_stop_forwards_to_skill(self):
         svc = _make_service()
@@ -698,10 +699,11 @@ def test_targeted_stop_clears_target_response_mode_only(self):
 
     def test_global_stop_drains_both_lists_and_response_mode(self):
         """§5.2: global_stop updated_session sets active_handlers=[],
-        converse_handlers=[], and removes response_mode."""
+        converse_handlers=[], active_skills=[], and removes response_mode."""
         sess = Session("s")
         sess.active_handlers = [{"skill_id": "a", "activated_at": 1.0}]
         sess.converse_handlers = [{"skill_id": "b", "activated_at": 1.0}]
+        sess.active_skills = [["a", 1.0]]
         sess.set_response_mode("c", 9999999999.0)
 
         with patch.object(self.svc, "voc_match",
@@ -714,6 +716,9 @@ def test_global_stop_drains_both_lists_and_response_mode(self):
         self.assertEqual(result.match_type, "stop:global")
         self.assertEqual(result.updated_session.active_handlers, [])
         self.assertEqual(result.updated_session.converse_handlers, [])
+        # §5.2: the legacy recency list is drained too, so a later targeted stop
+        # cannot route to a skill this global_stop already cleared
+        self.assertEqual(result.updated_session.active_skills, [])
         self.assertIsNone(result.updated_session.response_mode)
 
     def test_global_stop_no_positive_pong_drains_session(self):
