From c0c2d992007972cb640720ac5383303324fbcb8f Mon Sep 17 00:00:00 2001 From: JarbasAi Date: Sun, 28 Jun 2026 02:47:21 +0100 Subject: [PATCH] refactor: delegate handler-lifecycle done-signal to HandlerLifecycle util The three event-wrapper callbacks (_on_event_start/_on_event_end/ _on_event_error) emitted the internal workshop->core done-signal trio (mycroft.skill.handler.{start,complete,error}) by hand. Delegate that emission to the shared ovos_bus_client.handler.HandlerLifecycle util (#246) so the wire format lives in one place. Pure DRY: same topics, same payloads ({'name': } on start/complete, + {'exception': repr(error)} on error), same context['skill_id'] stamping and preserved session. The util stamps skill_id on the forwarded copy rather than mutating the caller's message, and builds a fresh payload per emission instead of mutating the shared skill_data dict. Preserved (NOT part of the trio): _on_event_end still emits ovos.utterance.handled when is_intent and still runs settings.store(); _on_event_error still speaks the skill.error dialog and logs the exception (the util deliberately does not speak). Bumps the ovos-bus-client floor to >=2.6.0a1 (carries the util). Fills the previously-stubbed test_on_event_{start,end,error} tests, asserting the emitted topic/payload/context is unchanged. Co-Authored-By: Claude Opus 4.8 --- ovos_workshop/skills/ovos.py | 30 +++++----- pyproject.toml | 2 +- test/unittests/skills/test_base.py | 92 ++++++++++++++++++++++++++++-- 3 files changed, 103 insertions(+), 21 deletions(-) diff --git a/ovos_workshop/skills/ovos.py b/ovos_workshop/skills/ovos.py index 5c476fd1..9e871093 100644 --- a/ovos_workshop/skills/ovos.py +++ b/ovos_workshop/skills/ovos.py @@ -33,6 +33,7 @@ from ovos_bus_client.apis.events import EventSchedulerInterface from ovos_bus_client.apis.gui import GUIInterface from ovos_bus_client.apis.ocp import OCPInterface +from ovos_bus_client.handler import HandlerLifecycle from ovos_bus_client.message import Message, dig_for_message from ovos_bus_client.session import SessionManager, Session from ovos_bus_client.util import get_message_lang @@ -1454,10 +1455,11 @@ def _on_event_start(self, message: Message, handler_info: str, """ if handler_info: # internal workshop->core done-signal (see docstring); NOT a spec - # topic -> emits mycroft.skill.handler.start - msg_type = handler_info + '.start' - message.context["skill_id"] = self.skill_id - self.bus.emit(message.forward(msg_type, skill_data)) + # topic -> emits mycroft.skill.handler.start. Delegated to the + # shared ovos-bus-client HandlerLifecycle util (DRY; same topic, + # payload and context["skill_id"] as before). + HandlerLifecycle(self.bus, message, skill_id=self.skill_id, + data=skill_data, handler_info=handler_info).start() def _on_event_end(self, message: Message, handler_info: str, skill_data: dict, is_intent: bool = False): @@ -1467,10 +1469,10 @@ def _on_event_end(self, message: Message, handler_info: str, """ if handler_info: # internal workshop->core done-signal (see _on_event_start); NOT a - # spec topic -> emits mycroft.skill.handler.complete - msg_type = handler_info + '.complete' - message.context["skill_id"] = self.skill_id - self.bus.emit(message.forward(msg_type, skill_data)) + # spec topic -> emits mycroft.skill.handler.complete. Delegated to + # the shared HandlerLifecycle util (same topic/payload/context). + HandlerLifecycle(self.bus, message, skill_id=self.skill_id, + data=skill_data, handler_info=handler_info).complete() if is_intent: self.bus.emit(message.forward(SpecMessage.UTTERANCE_HANDLED, skill_data)) @@ -1492,15 +1494,15 @@ def _on_event_error(self, error: str, message: Message, handler_info: str, if speak_errors: self.speak(speech) self.log.exception(error) - # append exception information in message - skill_data['exception'] = repr(error) if handler_info: # internal workshop->core done-signal (see _on_event_start); NOT a - # spec topic -> emits mycroft.skill.handler.error - msg_type = handler_info + '.error' + # spec topic -> emits mycroft.skill.handler.error. Delegated to the + # shared HandlerLifecycle util, which merges {"exception": repr(...)} + # into the payload (same topic/payload/context as before). The util + # deliberately does NOT speak; the spoken-error UX above stays here. message = message or Message("") - message.context["skill_id"] = self.skill_id - self.bus.emit(message.forward(msg_type, skill_data)) + HandlerLifecycle(self.bus, message, skill_id=self.skill_id, + data=skill_data, handler_info=handler_info).error(error) def _register_adapt_intent(self, intent_parser: Union[IntentBuilder, Intent, str], diff --git a/pyproject.toml b/pyproject.toml index f68a1e2b..9ebe6254 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -12,7 +12,7 @@ authors = [{name = "jarbasAi", email = "jarbasai@mailfence.com"}] requires-python = ">=3.9" dependencies = [ "ovos-utils>= 0.7.0,<1.0.0", - "ovos_bus_client>=2.2.0a1,<3.0.0", + "ovos_bus_client>=2.6.0a1,<3.0.0", # HandlerLifecycle done-signal util (#246) "ovos-config>=0.0.12,<3.0.0", "ovos-spec-tools>=0.16.1a2", # carries the adapt-free intent-definition primitives (OVOS-INTENT-4) "ovos-yes-no-plugin>=0.3.0,<1.0.0", diff --git a/test/unittests/skills/test_base.py b/test/unittests/skills/test_base.py index 7d07c166..b4ec9dbf 100644 --- a/test/unittests/skills/test_base.py +++ b/test/unittests/skills/test_base.py @@ -297,17 +297,97 @@ def test_find_resource(self): # TODO pass + def _capture(self, fn, *args, **kwargs): + """Run an _on_event_* callback against a private FakeBus and return the + list of (type, data, context) tuples it emitted on the topics of + interest (the handler trio + ovos.utterance.handled).""" + from ovos_spec_tools import SpecMessage + skill = OVOSSkill(bus=FakeBus(), skill_id=self.skill_id) + captured = [] + topics = ["mycroft.skill.handler.start", + "mycroft.skill.handler.complete", + "mycroft.skill.handler.error", + SpecMessage.UTTERANCE_HANDLED.value] + for t in topics: + skill.bus.on(t, lambda m: captured.append( + (m.msg_type, m.data, dict(m.context)))) + fn(skill, *args, **kwargs) + return captured + def test_on_event_start(self): - # TODO - pass + from ovos_bus_client.message import Message + msg = Message("trigger", {}, {"session": {"session_id": "sess1"}}) + skill_data = {"name": "TestSkill.handle_test"} + captured = self._capture(OVOSSkill._on_event_start, msg, + "mycroft.skill.handler", dict(skill_data)) + # exactly one emission: the .start done-signal + starts = [c for c in captured + if c[0] == "mycroft.skill.handler.start"] + self.assertEqual(len(starts), 1) + mtype, data, context = starts[0] + # byte-identical topic + payload + self.assertEqual(mtype, "mycroft.skill.handler.start") + self.assertEqual(data, {"name": "TestSkill.handle_test"}) + # context carries skill_id + preserves originating session + self.assertEqual(context["skill_id"], self.skill_id) + self.assertEqual(context["session"]["session_id"], "sess1") + # original message context not mutated by the util + self.assertNotIn("skill_id", msg.context) + # empty/false handler_info disables emission entirely + none_captured = self._capture(OVOSSkill._on_event_start, msg, "", + dict(skill_data)) + self.assertEqual( + [c for c in none_captured + if c[0].startswith("mycroft.skill.handler")], []) def test_on_event_end(self): - # TODO - pass + from ovos_bus_client.message import Message + msg = Message("trigger", {}, {"session": {"session_id": "sess1"}}) + skill_data = {"name": "TestSkill.handle_test"} + captured = self._capture(OVOSSkill._on_event_end, msg, + "mycroft.skill.handler", dict(skill_data)) + completes = [c for c in captured + if c[0] == "mycroft.skill.handler.complete"] + self.assertEqual(len(completes), 1) + mtype, data, context = completes[0] + self.assertEqual(mtype, "mycroft.skill.handler.complete") + self.assertEqual(data, {"name": "TestSkill.handle_test"}) + self.assertEqual(context["skill_id"], self.skill_id) + self.assertEqual(context["session"]["session_id"], "sess1") + + def test_on_event_end_is_intent_still_emits_utterance_handled(self): + # the ovos.utterance.handled emission must be PRESERVED alongside the + # delegated .complete done-signal when is_intent=True + from ovos_bus_client.message import Message + from ovos_spec_tools import SpecMessage + msg = Message("trigger", {}, {}) + skill_data = {"name": "TestSkill.handle_test"} + captured = self._capture(OVOSSkill._on_event_end, msg, + "mycroft.skill.handler", dict(skill_data), + True) + types = [c[0] for c in captured] + self.assertIn("mycroft.skill.handler.complete", types) + self.assertIn(SpecMessage.UTTERANCE_HANDLED.value, types) def test_on_event_error(self): - # TODO - pass + from ovos_bus_client.message import Message + msg = Message("trigger", {}, {"session": {"session_id": "sess1"}}) + skill_data = {"name": "TestSkill.handle_test"} + err = "boom" # workshop passes str(error) + captured = self._capture(OVOSSkill._on_event_error, err, msg, + "mycroft.skill.handler", dict(skill_data), + False) + errors = [c for c in captured + if c[0] == "mycroft.skill.handler.error"] + self.assertEqual(len(errors), 1) + mtype, data, context = errors[0] + self.assertEqual(mtype, "mycroft.skill.handler.error") + # payload = original {name} + repr(error) under "exception" (identical + # to the pre-refactor skill_data['exception'] = repr(error)) + self.assertEqual(data, {"name": "TestSkill.handle_test", + "exception": repr(err)}) + self.assertEqual(context["skill_id"], self.skill_id) + self.assertEqual(context["session"]["session_id"], "sess1") def test_add_event(self): # TODO