refactor: delegate handler-lifecycle done-signal to shared HandlerLifecycle util#440
Conversation
…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': <handler>} 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 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 7 minutes and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Beep boop! Data processing finished. ⚙️I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthI've checked the repo's strength (aka test suite robustness). 🏋️♂️ ✅ All required files present. Latest Version: ✅ 🔨 Build TestsThe build pipeline has reached its destination. 📍 ✅ All versions pass
⚖️ License CheckChecking if the licenses are compatible with OVOS. 🧩 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔒 Security (pip-audit)I've performed a digital frisk of this contribution. 👮♂️ ✅ No known vulnerabilities found (72 packages scanned). The automation never sleeps, but I might reboot. 💤 |
… guard The rebase onto origin/dev reverted PR #440 (HandlerLifecycle util delegation) and PR #442 (ovos.utterance.handled version guard) in ovos.py, falling back to the pre-refactor manual emission that mutates the caller's message.context. Restore both: - _on_event_start/_end/_error delegate to HandlerLifecycle(...).{start, complete,error}() from ovos_bus_client.handler (PR #440). The util stamps skill_id on the forwarded copy rather than mutating the caller's message, so the original message.context is left untouched. - _on_event_end guards the ovos.utterance.handled emission on _core_owns_utterance_handled() (PIPELINE-1 §9.5, PR #442): only emit for an older/absent ovos-core; with core >=2.3.0a1 the orchestrator owns it on the matched path. - _on_event_error no longer mutates the shared skill_data dict; the util merges {exception: repr(error)} into the forwarded payload. Tests: - Restore the assertNotIn('skill_id', msg.context) assertion in test_on_event_start (valid again: the util does not mutate the original message context). - Fix test_on_event_end_is_intent_still_emits_utterance_handled to patch _core_owns_utterance_handled=False so it is deterministic regardless of the installed ovos-core version (the test was failing on environments with ovos-core >=2.3.0a1 installed, including origin/dev). - Add test_on_event_end_core_owns_suppresses_utterance_handled covering the >=2.3.0a1 branch (asserts the framework does NOT emit the end-marker when core owns it). Co-Authored-By: Claude <noreply@anthropic.com>
…xfails The hard CI failure on this branch was the stale test_on_event_end_is_intent_still_emits_utterance_handled: #442's _core_owns_utterance_handled() guard suppresses workshop's ovos.utterance.handled emission when ovos-core >= 2.3.0a1 (the orchestrator owns the PIPELINE-1 §9.5 end-marker on the matched path), which CI now installs. The #440 assertion is therefore stale. - test_base.py: replace the stale single test with two deterministic branches (both monkeypatch _core_owns_utterance_handled): * _defers_utterance_handled_to_core — modern core (>=2.3.0a1); workshop must NOT emit ovos.utterance.handled. * _emits_utterance_handled_legacy — absent/old core; workshop still emits (migration-window back-compat). - pyproject.toml: bump floors to the alphas carrying the singleton-registry SessionManager + §9.5 orchestrator ownership: ovos_bus_client >= 2.6.2a2, ovos-spec-tools >= 1.2.3a1, ovos-core >= 2.4.0a1 (test), ovos-adapt-parser >= 1.4.2a1 (test). The two intent-layers e2e xfails added by this PR stay: under the spec's 'every session folds' model (OVOS-CONTEXT-1 §4 / OVOS-SESSION-1), the legacy add_context/in-place context mutation is wiped by the next fold — the genuine fix is to migrate IntentLayers to session.intent_context via ovos.session.sync (§5.3), tracked separately.
* test: xfail intent-layers e2e tests broken on dev, unrelated to this PR test_layers_advance_in_sequence_and_gate_intents and test_inactive_layer_intents_do_not_match fail identically on a clean origin/dev checkout with none of this PR's changes applied: the layer context token is never applied (ovos_workshop/decorators/layers.py), so gated intents never match. xfail them here to unblock this PR's CI; the underlying layers bug needs its own fix in a separate PR. Co-Authored-By: Claude <noreply@anthropic.com> * fix: harden utterance.handled test (§9.5), bump pins; keep layer-e2e xfails The hard CI failure on this branch was the stale test_on_event_end_is_intent_still_emits_utterance_handled: #442's _core_owns_utterance_handled() guard suppresses workshop's ovos.utterance.handled emission when ovos-core >= 2.3.0a1 (the orchestrator owns the PIPELINE-1 §9.5 end-marker on the matched path), which CI now installs. The #440 assertion is therefore stale. - test_base.py: replace the stale single test with two deterministic branches (both monkeypatch _core_owns_utterance_handled): * _defers_utterance_handled_to_core — modern core (>=2.3.0a1); workshop must NOT emit ovos.utterance.handled. * _emits_utterance_handled_legacy — absent/old core; workshop still emits (migration-window back-compat). - pyproject.toml: bump floors to the alphas carrying the singleton-registry SessionManager + §9.5 orchestrator ownership: ovos_bus_client >= 2.6.2a2, ovos-spec-tools >= 1.2.3a1, ovos-core >= 2.4.0a1 (test), ovos-adapt-parser >= 1.4.2a1 (test). The two intent-layers e2e xfails added by this PR stay: under the spec's 'every session folds' model (OVOS-CONTEXT-1 §4 / OVOS-SESSION-1), the legacy add_context/in-place context mutation is wiped by the next fold — the genuine fix is to migrate IntentLayers to session.intent_context via ovos.session.sync (§5.3), tracked separately. * fix: un-xfail intent-layers e2e (depends on ovos-utils#396), bump ovos-utils floor The two intent-layers e2e tests xfailed in this PR are fixed upstream by OpenVoiceOS/ovos-utils#396: FakeBus.emit was folding the message session AFTER the bus handlers ran, using a pre-mutation emit-time snapshot, which wholesale-replaced the SessionManager singleton (Session.update_from is last-writer-wins) and wiped every in-place / synced session mutation the handlers made — notably handle_add_context injecting the layer token into sess.context. The #396 fix reorders FakeBus.emit to fold BEFORE handlers (matching MessageBusClient.on_message receive -> fold -> handle order) and drops the post-handler self-broadcast-back fold, so the legacy add_context -> sess.context frame mechanism survives under folding again. No workshop change is needed; the IntentLayers code is unchanged. - test_intent_layers_e2e.py: revert the xfail markers added in 2aee2bd; both tests pass once ovos-utils >= 0.13.3a2 lands. - pyproject.toml: bump ovos-utils floor to >= 0.13.3a2 (pending #396 release). --------- Co-authored-by: Claude <noreply@anthropic.com>
1 participant
Step 2 of 3 of the handler-lifecycle done-signal refactor: make ovos-workshop delegate its emission of the internal workshop->core done-signal trio to the new
HandlerLifecycleutil in ovos-bus-client (landed in OpenVoiceOS/ovos-bus-client#246).What changed
In
ovos_workshop/skills/ovos.py, the three event-wrapper callbacks emittedmycroft.skill.handler.{start,complete,error}by hand. The emission (only) is now delegated toovos_bus_client.handler.HandlerLifecycle:_on_event_start->HandlerLifecycle(...).start()_on_event_end->HandlerLifecycle(...).complete()_on_event_error->HandlerLifecycle(...).error(error)Zero behavior change
Byte-identical wire format, verified by a before/after equivalence check and by the new tests:
mycroft.skill.handler.start{'name': <handler>}skill_idstamped, session preservedmycroft.skill.handler.complete{'name': <handler>}mycroft.skill.handler.error{'name': <handler>, 'exception': repr(error)}The util stamps
context["skill_id"]on the forwarded copy (does not mutate the caller's message) and builds a fresh payload per emission (does not mutate the sharedskill_datadict) — both strictly safer, with the emitted message unchanged.Preserved (NOT part of the trio)
_on_event_end: still emitsovos.utterance.handledwhenis_intent, and still runs thesettings.store()block._on_event_error: still speaks theskill.errordialog and logs the exception. The util deliberately does not speak; that UX stays in workshop.Dep
Bumps the
ovos-bus-clientfloor to>=2.6.0a1(the published release that carriesovos_bus_client.handler; #246 landed after the 2.5.1a3 tag). Floor in pyproject, not CI.Tests
Fills the previously-stubbed
test_on_event_{start,end,error}intest/unittests/skills/test_base.py, asserting topic + payload +context["skill_id"]/session are unchanged, plus that emptyhandler_infodisables emission andis_intentstill emitsovos.utterance.handled. The existingtest_decorators.pyassertions onmycroft.skill.handler.startpass unchanged.🤖 Generated with Claude Code