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

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>

Author: JarbasAl

ovos_core/intent_services/stop_service.py

@@ -82,21 +82,18 @@ 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).
         """
         self.bus.emit(message.forward(SpecMessage.STOP))
-        self.bus.emit(message.forward("mycroft.stop"))
         # TODO - this needs a confirmation dialog if nothing was stopped
-        self.bus.emit(message.forward("ovos.utterance.handled"))
+        self.bus.emit(message.forward(SpecMessage.UTTERANCE_HANDLED))
 
     @staticmethod
     def _drain_global_stop_session(session) -> "Session":
@@ -107,13 +104,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
 

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,20 +549,20 @@ 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)
         msg = Message("stop:global", {})
         svc.handle_global_stop(msg)
         types = [m.msg_type for m in emitted]
         self.assertIn("ovos.stop", types)
-        self.assertIn("mycroft.stop", types)
-        self.assertIn("ovos.utterance.handled", types)
-        # the spec broadcast is emitted before the legacy back-compat one
-        self.assertLess(types.index("ovos.stop"), types.index("mycroft.stop"))
+        self.assertIn(SpecMessage.UTTERANCE_HANDLED.value, types)
+        # 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()
@@ -695,10 +696,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",
@@ -711,6 +713,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):