feat: OVOS-STOP-1 conformance in stop_service (ovos.stop.ping/pong, global ovos.stop, session drain)#777
Draft
JarbasAl wants to merge 15 commits into
Draft
feat: OVOS-STOP-1 conformance in stop_service (ovos.stop.ping/pong, global ovos.stop, session drain)#777JarbasAl wants to merge 15 commits into
JarbasAl wants to merge 15 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesOVOS-STOP-1 spec migration and session draining
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Hey! I've got some fresh data on your pull request. 📈I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthYour repository is in great shape! ✅ ✅ All required files present. Latest Version: ✅ 🌍 Locale BuildI've gathered the facts for your review. 📖 ✅ Locale properly configured (64 files, 17 languages) Locale directories found:
Localization coverage:
pyproject.toml: ✅
Build manifest: ✅ 31 locale files included in package 🏷️ Release PreviewSetting the stage for the upcoming deployment. 🎭 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🔒 Security (pip-audit)A thorough vetting of your project's dependencies. 📜 ✅ No known vulnerabilities found (110 packages scanned). 📊 CoverageTesting the resilience of our codebase. 🧱 Files below 80% coverage (9 files)
Full report: download the 🔨 Build TestsChecking if the gears are still turning smoothly... ⚙️ ✅ All versions pass
⚖️ License CheckEnsuring our license headers are up to date for 2024. 📅 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔌 Skill Tests (ovoscope)Ensuring the skill's dialogs are as clear as day. ☀️ ✅ 2/36 passed ❌ **TestAdaptIntent** — 0/4
❌ **TestCancelIntentMidSentence** — 0/1
❌ **TestConverse** — 0/1
❌ **TestCountSkills** — 0/4
❌ **TestDeactivate** — 2/3
❌ **TestFallback** — 0/1
❌ **TestGlobalStopVocWithActiveSkill** — 0/1
❌ **TestGlobalStopVocabulary** — 0/2
❌ **TestIntentPipelineRouting** — 0/4
❌ **TestLangDisambiguation** — 0/4
❌ **TestNoSkills** — 0/2
❌ **TestPadatiousIntent** — 0/4
❌ **TestStopNoSkills** — 0/3
❌ **TestStopServiceNotASkill** — 0/1
❌ **TestStopSkillCanHandleFalse** — 0/1
🚌 Bus CoverageA comprehensive review of the skill's bus-level interactions. 🚌 🔴 Coverage Summary
📊 Per-Skill Breakdown
🔍 Detailed Message Type Breakdown
|
| Plugin Type | Wheel | Editable |
|---|---|---|
| pipeline | ✅ | ✅ |
Entry Point Validation:
| Entry Point | Type | Import | Interface |
|---|---|---|---|
ovos-converse-pipeline-plugin |
pipeline | ✅ 17ms | ⊘ |
ovos-fallback-pipeline-plugin |
pipeline | ✅ 2ms | ⊘ |
ovos-stop-pipeline-plugin |
pipeline | ✅ 47ms | ⊘ |
⊘ No settingsmeta.json
✅ requires-python >=3.10 — running Python 3.11
Issues:
⚠️ No settingsmeta.json found⚠️ No settingsmeta.json found
Your digital assistant in the world of OVOS 🤖
JarbasAl
force-pushed
the
feat/stop-1-conformance
branch
from
9e9861c to
fea8d8e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/build_tests.yml (1)
16-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the bus-client ref to an immutable revision.
Using
@feat/session-spec-fieldsmakes CI depend on whatever that branch points to later, so the same commit here can start passing or failing differently over time. Please pin a commit SHA (or a released tag once published) instead.Suggested change
- pre_install_pip: 'git+https://github.com/OpenVoiceOS/ovos-bus-client@feat/session-spec-fields' + pre_install_pip: 'git+https://github.com/OpenVoiceOS/ovos-bus-client@<commit-sha>'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build_tests.yml around lines 16 - 20, The `pre_install_pip` reference for `ovos-bus-client` is using a mutable branch (`feat/session-spec-fields`), which makes CI results change over time. Update the workflow to pin `pre_install_pip` in `.github/workflows/build_tests.yml` to an immutable commit SHA or a released tag instead, keeping the existing `bus-client` dependency target but making the ref stable..github/workflows/ovoscope.yml (1)
22-28: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe end-to-end stack is still non-reproducible.
Every dependency in
post_install_pipis pinned to a moving@devor feature branch. That makes ovoscope results drift as unrelated repos merge, which is especially risky for a spec-conformance PR. Please freeze these to commit SHAs (or a constraints file of immutable refs) so failures stay attributable to this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ovoscope.yml around lines 22 - 28, The end-to-end stack in the workflow configuration is still non-reproducible because `post_install_pip` points to moving `@dev` and feature branches. Update the `ovoscope.yml` workflow entry to use immutable refs for every package in `post_install_pip`—prefer commit SHAs or an equivalent constraints file—so the test stack stays fixed and failures remain attributable to this PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ovos_core/intent_services/stop_service.py`:
- Around line 85-97: handle_global_stop currently broadcasts both ovos.stop and
mycroft.stop unconditionally, which can trigger the same skill twice if it
subscribes to both topics. Update the fallback in handle_global_stop so the
legacy emit is deduped or gated by migration/capability state instead of
assuming disjoint subscribers. Use the existing _collect_stop_skills and
bus.emit flow to locate the stop dispatch logic, and ensure migration-window
skills only receive one stop signal.
- Around line 101-118: Drained global-stop sessions are inconsistent because
_drain_global_stop_session only clears active_handlers and converse_handlers,
while get_active_skills still reads session.active_skills for stop routing.
Update _drain_global_stop_session in StopService to also clear or reset
active_skills atomically with the other session fields so a committed
global_stop cannot later take the targeted-stop path from stale skills; keep the
behavior aligned with get_active_skills and any stop-target selection logic that
depends on the session state.
---
Nitpick comments:
In @.github/workflows/build_tests.yml:
- Around line 16-20: The `pre_install_pip` reference for `ovos-bus-client` is
using a mutable branch (`feat/session-spec-fields`), which makes CI results
change over time. Update the workflow to pin `pre_install_pip` in
`.github/workflows/build_tests.yml` to an immutable commit SHA or a released tag
instead, keeping the existing `bus-client` dependency target but making the ref
stable.
In @.github/workflows/ovoscope.yml:
- Around line 22-28: The end-to-end stack in the workflow configuration is still
non-reproducible because `post_install_pip` points to moving `@dev` and feature
branches. Update the `ovoscope.yml` workflow entry to use immutable refs for
every package in `post_install_pip`—prefer commit SHAs or an equivalent
constraints file—so the test stack stays fixed and failures remain attributable
to this PR.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd92b530-db30-4a9e-bc75-4d14afbe83ac
📒 Files selected for processing (10)
.github/workflows/build_tests.yml.github/workflows/coverage.yml.github/workflows/ovoscope.ymlovos_core/intent_services/converse_service.pyovos_core/intent_services/fallback_service.pyovos_core/intent_services/stop_service.pypyproject.tomltest/end2end/test_stop.pytest/end2end/test_stop_refactor.pytest/unittests/test_stop_service.py
JarbasAl
added a commit
that referenced
this pull request
Jun 28, 2026
…kills 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>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/unittests/test_stop_service.py (1)
721-742: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert
active_skillsis drained in the escalation path.This branch is meant to lock in the stale-routing fix too, but
sess.active_skillsis never populated or asserted here. A regression that forgets to clear the legacy recency list duringmatch_lowescalation would still pass.Proposed test tightening
sess = Session("s") sess.active_handlers = [{"skill_id": "a", "activated_at": 1.0}] sess.converse_handlers = [{"skill_id": "a", "activated_at": 1.0}] + sess.active_skills = [["a", 1.0]] sess.set_response_mode("a", 9999999999.0) @@ self.assertEqual(result.match_type, "stop:global") self.assertEqual(result.updated_session.active_handlers, []) self.assertEqual(result.updated_session.converse_handlers, []) + self.assertEqual(result.updated_session.active_skills, []) self.assertIsNone(result.updated_session.response_mode)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unittests/test_stop_service.py` around lines 721 - 742, The global-stop escalation test does not cover the legacy recency list, so a regression in session draining could still pass. In test_global_stop_no_positive_pong_drains_session, populate Session.active_skills on the setup object and assert it is cleared on result.updated_session after StopService.match_low takes the global_stop path, alongside the existing handler and response_mode checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/unittests/test_stop_service.py`:
- Around line 721-742: The global-stop escalation test does not cover the legacy
recency list, so a regression in session draining could still pass. In
test_global_stop_no_positive_pong_drains_session, populate Session.active_skills
on the setup object and assert it is cleared on result.updated_session after
StopService.match_low takes the global_stop path, alongside the existing handler
and response_mode checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf0f881d-c6bb-4224-b08b-9b83f67757da
📒 Files selected for processing (3)
.github/workflows/ovoscope.ymlovos_core/intent_services/stop_service.pytest/unittests/test_stop_service.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ovoscope.yml
- ovos_core/intent_services/stop_service.py
JarbasAl
added a commit
that referenced
this pull request
Jun 29, 2026
…PELINE-1 §8) (#789) * feat: emit handler done-signal for converse + fallback dispatches The orchestrator's converse and fallback dispatches (PIPELINE-1 §7.3 reserved-name/polymorphic dispatches) run in skills WITHOUT ovos-workshop's handler_info wrapper, so they never produce the framework done-signal (mycroft.skill.handler.{start,complete,error}). A dispatcher observing that signal (PIPELINE-1 §8, the IntentDispatcher) therefore never sees a completion for these dispatches and falls back to its 5-minute handler timeout. Adopt the shared HandlerLifecycle util (ovos_bus_client.handler, bus-client 2.6.0a1) so core reports the dispatch->outcome span it orchestrates: - converse: handle_converse emits handler.start at the converse.request dispatch, registers a one-shot skill.converse.response listener (filtered by the targeted skill_id) -> handler.complete, with a generous timeout backstop -> handler.error. The done-signal is stamped with the targeted skill_id so a dispatcher correlates it by (session_id, skill_id). - fallback: the fallback dispatch is owned by the orchestrator; core translates each registered skill's own lifecycle markers (ovos.skills.fallback.<skill_id>.start/.response) into handler.start/handler.complete, stamped with that skill_id. Wired on register, removed on deregister/shutdown. stop_service is intentionally NOT touched here (owned by PR #777 / STOP-1). Floor ovos-bus-client>=2.6.0a1 (first release shipping ovos_bus_client.handler) and the coupled ovos-spec-tools>=1.1.0a1 it requires. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: drop ovos-spec-tools upper version cap Keep the >=1.1.0a1 floor (bus-client 2.6.0a1 requires it); remove the <2.0.0 max cap so spec-tools is free to float forward. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Update Changelog --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
JarbasAl
added a commit
that referenced
this pull request
Jun 30, 2026
…l events (#788) * feat: orchestrator emits the PIPELINE-1 §8 handler-lifecycle trio Make ovos-core (the orchestrator) the authoritative emitter of the OVOS-PIPELINE-1 §8 handler-lifecycle trio (ovos.intent.handler.{start,complete,error}), wrapping every dispatch: start immediately before the <skill_id>:<intent_name> dispatch (§7), then exactly one of complete (on the framework done-signal) / error (on the framework error signal or the §8.3 timeout). Each trio Message is forward-derived from the dispatch so context (incl. session) is preserved unchanged (§8, MSG-1 §5.1). New IntentDispatcher (ovos_core/intent_services/dispatcher.py) owns the §7 dispatch + §8 trio. Completion is observed across the distributed bus via the skill framework's long-standing legacy done-signals (mycroft.skill.handler.complete/.error) — framework infrastructure, not the user handler (which emits nothing per §8/§11). A per-dispatch §8.3 timeout guarantees exactly one terminal even if the handler never reports; on that path the orchestrator also owns ovos.utterance.handled. Reserved-name dispatches get the trio identically (§7.0/§7.3); the resolved-guard keeps the terminal count at one regardless of the bus namespace bridge. This is additive: the §9.5 end-marker on the ordinary matched path and the §9.2 ovos.intent.matched notification are left to ovos-workshop / follow-up changes and are out of scope here. Dep floors: ovos-bus-client>=2.5.1a1, ovos-spec-tools>=0.17.3a1 (SpecMessage.INTENT_HANDLER_* members). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat: orchestrator owns the PIPELINE-1 §9 utterance-terminal events Complete the orchestrator's ownership of the OVOS-PIPELINE-1 §6.1 per-utterance terminal sequence, on top of the §8 handler-lifecycle trio: - §9.2 ovos.intent.matched — emitted by _dispatch_match on every accepted match, before the dispatch goes out (notification, not a dispatch). Carries skill_id, intent_name (the full <skill_id>:<intent_name> match_type), lang, utterance, slots, pipeline_id. - §9.3 ovos.intent.unmatched — the no-match / all-filtered terminal, replacing the legacy complete_intent_failure (the two are bridged by ovos-spec-tools' MIGRATION_MAP, so emitting the spec topic re-delivers the legacy one to consumers still on it). - §6.4 cancellation now emits the spec ovos.utterance.cancelled. Each utterance terminates with exactly one ovos.utterance.handled (§9.5): core owns it on the no-match, cancel and §8.3-timeout paths; on the ordinary matched path the skill framework still emits it (moving that fully into core is gated on the ovos-workshop reduction). Rename _emit_match_message -> _dispatch_match (it orchestrates the §6.1 post-match steps then dispatches) and correct the IntentDispatcher docstring to scope it to §7 dispatch + §8 trio (the §9.2 notification lives in the service). Verified on a real minicroft: matched path emits matched/start/ complete/handled exactly once each; no-match path emits ovos.intent.unmatched + ovos.utterance.handled (no complete_intent_ failure). test_no_skills / test_lang_detect conformance suites green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: account for §9.2 matched + §8.1 start in activate/fallback e2e The converse-deactivate (test_activate) and fallback (test_fallback) ovoscope scenarios each gain two captured messages now that the orchestrator emits ovos.intent.matched (§9.2) and ovos.intent.handler. start (§8.1) natively before every dispatch — previously the spec trio existed only as uncounted bridge-mirrors of workshop's legacy emit. Verified on a real minicroft: the two extra messages per scenario are exactly ovos.intent.matched + ovos.intent.handler.start (the reserved-name converse:skill / fallback .request dispatches carry no mycroft.skill.handler.* done-signal, so their §8 terminal resolves via the §8.3 timeout after the end-marker, not captured). No spec-topic double-emit: ovos.intent.matched, ovos.intent.handler.start and ovos.utterance.handled each appear exactly once per utterance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat: core emits ovos.utterance.handled on the matched path too (§9.5) The orchestrator owns the universal end-marker on EVERY terminal path. The dispatcher already emitted ovos.utterance.handled on the timeout path; emit it after the complete/error terminals as well, so a matched dispatch also ends with exactly one core-emitted handled (the _pop/resolved guard fires one terminal per dispatch -> one handled). Unmatched/cancel keep their service.py handled. Workshop may still emit its own matched-path handled during the migration window; that core-vs-workshop duplicate is expected and removed later workshop-side. * chore: bump ovos-workshop floor to >=9.0.1a5 (HandlerLifecycle delegation merged) * chore: bump ovos-workshop floor to >=9.0.2a1 (matched-path handled guard) ovos-workshop 9.0.2a1 (#442) guards its matched-path ovos.utterance.handled emission behind a version check on the installed ovos-core, so once core ships the §9.5 matched-path emission the framework stops double-emitting. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: stage version 2.3.0a1 so CI exercises the workshop §9.5 guard This feat bumps core to the 2.3.x line. ovos-workshop 9.0.2a1 suppresses its matched-path ovos.utterance.handled only when the installed ovos-core is >=2.3.0a1; staging the version here lets PR CI install a core that trips that guard, so the e2e exercises core as the single end-marker emitter rather than relying on the published 2.2.x. Release automation re-derives the final version on merge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(deps): floor-pin ovos-m2v-pipeline>=0.3.1a1 (workshop 9.x compatible) The published m2v 0.0.10a1 caps ovos-workshop<9.0.0; with core's workshop floor at 9.0.2a1 pip backtracked m2v down to 0.0.10a1 and hit that cap -> ResolutionImpossible. m2v 0.3.1a1 drops the workshop dependency entirely, so floor-pinning it (the prerelease-floor-pin pattern) forbids the backtrack and the closure resolves. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(deps): floor-pin downstream stack to spec-tools-1.x-ready prereleases core's bus-client>=2.5.1a1 pulls ovos-bus-client 2.5.1a3/2.6.0a1, which require ovos-spec-tools>=1.1.0a1. pip backtracked the mycroft/plugins/skills-essential extras down to stale releases that cap ovos-spec-tools<1.0.0 (e.g. ovos-audio 2.0.1a1) -> ResolutionImpossible. Floor-pin each to its latest prerelease (all spec-tools-1.x-ready) so the resolver can't backtrack into the capped ones, and make core's own ovos-spec-tools floor explicit at >=1.1.0a1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(deps): floor-pin ovos-adapt-parser>=1.4.2a1 (spec-tools-1.x ready) Every ovos-adapt-parser release up to 1.4.1a1 caps ovos-spec-tools<1.0.0; the uncap landed in 1.4.2a1. Floor-pin it so pip can't backtrack into the capped versions while core requires ovos-spec-tools>=1.1.0a1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: align e2e expectations with §9.5 end-marker payload - fallback speak meta carries the skill's dialog/data keys, not just skill. - ovos.utterance.handled (§9.5) is the orchestrator end-marker with EMPTY data; the stop count-to-infinity / ping-pong expectations wrongly carried the handler name on it (KeyError 'name'). The handler name stays on the framework mycroft.skill.handler.complete signal, where it belongs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: orchestrator owns ovos.utterance.handled, dispatcher only signals done PIPELINE-1 §9.5: ovos.utterance.handled is the orchestrator's universal end-marker, not the dispatcher's. The IntentDispatcher owned only the §8 handler-lifecycle trio but was also emitting the §9.5 end-marker on its terminal paths — wrong layer. - IntentDispatcher no longer emits ovos.utterance.handled. Each in-flight dispatch carries a 'done' Event set on its §8 terminal (complete/error/timeout); dispatch() returns the entry. - IntentService._dispatch_match blocks on entry.done (the §8.3 timeout guarantees it fires) then emits the single ovos.utterance.handled, uniformly with the no-match (send_complete_intent_failure) and cancel (send_cancel_event) paths it already owns. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: dispatch entry is a context manager that blocks until the §8 terminal Reads cleaner at the call site — the event-waiting is hidden behind the context manager: with self.intent_dispatcher.dispatch(reply, skill_id, intent_name): pass self.bus.emit(reply.forward(SpecMessage.UTTERANCE_HANDLED, {})) _InFlightDispatch gains __enter__/__exit__ (exit blocks on its done event). Callers that don't want to block can still wait on entry.done directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: keep explicit entry.done.wait() call site (drop context manager) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: guarantee dispatch waiters are always released (review) - terminal handlers wrap the §8 emission in try/finally so entry.done is set even if the bus emission raises (the §8.3 timer is already cancelled by _pop, so nothing else would release a blocked orchestrator). - IntentDispatcher.shutdown() sets each in-flight entry's done before clearing, so a _dispatch_match caller is never left blocked on entry.done.wait() forever. - test_timeout_emits_error_and_releases waits on entry.done instead of a fixed sleep (deterministic); test_orchestrator_emits_handled_after_terminal now asserts ovos.intent.handler.complete precedes ovos.utterance.handled. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): pin §8.3 handler-timeout to 10s in the end2end suite The orchestrator blocks on each handler's §8 terminal before emitting §9.5 ovos.utterance.handled; the production backstop is 5min. e2e handlers report in <1s (or are explicitly stopped), so pin the backstop to 10s — a dropped done-signal then fails the suite in seconds instead of stalling for minutes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: orchestrator emits utterance.handled by reacting to the §8 terminal (non-blocking) Blocking handle_utterance on entry.done.wait() deadlocked the synchronous bus: handle_utterance is itself a bus handler, so blocking it stalled the same path that must deliver the handler's done-signal — the §8.3 timer then fired ovos.intent.handler.error instead of the handler completing (8 e2e tests). Keep the orchestrator as the §9.5 owner, but non-blocking: IntentService now subscribes to the dispatcher's §8 terminal (ovos.intent.handler.complete/error) and emits ovos.utterance.handled in reaction, uniformly with the no-match and cancel paths. The dispatcher reverts to trio-only (no done event, no blocking); the §8.3 timeout still backstops via the error terminal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): expect the active skill's §8 complete terminal during stop The count/ping-pong stop tests inject a long-running intent (daemon) then stop it. When that daemon intent completes (on stop), its dispatch now emits the §8 spec terminal ovos.intent.handler.complete before the §9.5 ovos.utterance.handled, so add it to the expected sequence (got 10 messages, expected 9). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: emit utterance.handled via dispatcher on_terminal callback (deterministic) The reactive subscription (IntentService listening to ovos.intent.handler.complete) raced the ovoscope capture: FakeBus dispatches the terminal to both the capture recorder and the subscription, and when the subscription fired first it emitted the EOF ovos.utterance.handled before the terminal was recorded — so the capture stopped early and dropped the §8 complete (flaky 9-vs-10 message counts). Make it deterministic: the dispatcher invokes an on_terminal callback right AFTER the §8 terminal is on the bus; the orchestrator's _emit_utterance_handled emits the §9.5 end-marker then. Same call stack, so the terminal is always observed before the end-marker. Orchestrator still owns the emission; dispatcher stays non-blocking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Update ovos-utils version in pyproject.toml * StopService: wrap handlers in HandlerLifecycle instead of ad-hoc emission (#796) * Update Changelog * StopService: wrap handlers in HandlerLifecycle Replace rudimentary manual emission of both UTTERANCE_HANDLED (which the orchestrator now owns via IntentDispatcher._notify_terminal) and mycroft.skill.handler.complete with the canonical HandlerLifecycle context manager from ovos-bus-client. HandlerLifecycle consistently emits the full handler-lifecycle trio (start/complete/error) so that the dispatcher can properly track in-flight entries and fire §9.5 UTTERANCE_HANDLED at the right moment. Co-Authored-By: Claude * ConverseService: wrap handle_converse in HandlerLifecycle Same pattern as StopService — handle_converse is called via bus event dispatch after the converse pipeline stage matches, but had no lifecycle signalling. Without it the dispatcher's in-flight entry for converse dispatches would only ever resolve on timeout (10s). Co-Authored-By: Claude * Update stop_service unit tests for HandlerLifecycle test_handle_global_stop_emits_mycroft_stop: check for mycroft.skill.handler.start/complete instead of ovos.utterance.handled. test_handle_skill_stop_forwards_to_skill: HandlerLifecycle emits 3 messages (start, forward, complete), not 1. Co-Authored-By: Claude * Update stop service tests to include additional assertions Added assertions to check for 'mycroft.stop' and 'ovos.utterance.handled' messages in stop service tests. * feat: emit handler done-signal for converse + fallback dispatches (PIPELINE-1 §8) (#789) * feat: emit handler done-signal for converse + fallback dispatches The orchestrator's converse and fallback dispatches (PIPELINE-1 §7.3 reserved-name/polymorphic dispatches) run in skills WITHOUT ovos-workshop's handler_info wrapper, so they never produce the framework done-signal (mycroft.skill.handler.{start,complete,error}). A dispatcher observing that signal (PIPELINE-1 §8, the IntentDispatcher) therefore never sees a completion for these dispatches and falls back to its 5-minute handler timeout. Adopt the shared HandlerLifecycle util (ovos_bus_client.handler, bus-client 2.6.0a1) so core reports the dispatch->outcome span it orchestrates: - converse: handle_converse emits handler.start at the converse.request dispatch, registers a one-shot skill.converse.response listener (filtered by the targeted skill_id) -> handler.complete, with a generous timeout backstop -> handler.error. The done-signal is stamped with the targeted skill_id so a dispatcher correlates it by (session_id, skill_id). - fallback: the fallback dispatch is owned by the orchestrator; core translates each registered skill's own lifecycle markers (ovos.skills.fallback.<skill_id>.start/.response) into handler.start/handler.complete, stamped with that skill_id. Wired on register, removed on deregister/shutdown. stop_service is intentionally NOT touched here (owned by PR #777 / STOP-1). Floor ovos-bus-client>=2.6.0a1 (first release shipping ovos_bus_client.handler) and the coupled ovos-spec-tools>=1.1.0a1 it requires. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: drop ovos-spec-tools upper version cap Keep the >=1.1.0a1 floor (bus-client 2.6.0a1 requires it); remove the <2.0.0 max cap so spec-tools is free to float forward. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Update Changelog --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): ignore TTS mock audio signals in all end2end tests * fix(test): remove ovos.utterance.handled assertion from TestBusHandlers (orchestrator owns it now) * fix: correlate fallback dispatch to its §8 terminal via match_data skill_id The fallback pipeline's match_type (ovos.skills.fallback.<id>.request) carries no ':', so the orchestrator derived the dispatcher correlation key as the whole topic. The framework done-signal (re-emitted mycroft.skill.handler.complete) is stamped with the real skill_id, so IntentDispatcher._pop never matched it and the §8 ovos.intent.handler.complete terminal — and with it the §9.5 ovos.utterance.handled end-marker — only fired on the 5-minute §8.3 handler timeout. Every fallback-handled utterance was affected in production. Derive the correlation key from match_data['skill_id'] when the topic has no ':', so it equals the done-signal's skill_id. Activation is unchanged (match.skill_id stays None, no spurious {skill_id}.activate). test_fallback now asserts the §8 terminal, which can only be captured when correlation succeeds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): align stop suite with §8 trio + live-session resend StopService wraps its global/skill stop handlers in HandlerLifecycle (#796), so the legacy mycroft.skill.handler.{start,complete} done-signal now brackets mycroft.stop / {skill_id}.stop. Update the stop expectations to assert the trio. The ping-pong tests built the stop message from a stale, test-local Session that never saw the count skill's server-side self-activation (the count message, serialized before activation, folds an empty active_skills back into the singleton — correct SESSION-1 last-write-wins). Resend the live singleton session for the stop turn, as a real client tracking responses would, instead of manually activating — so the running skill is in active_skills and the ping-pong path runs without a manual activate crutch. Also drop the §8 ovos.intent.handler.complete from the expected lists where it is filtered via ignore_messages. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: make converse dispatch terminal resolution atomic + session-scoped handle_converse resolved the §8 lifecycle from two threads (the skill.converse.response handler and the timeout timer) with a non-atomic check-then-set on an Event, so both could pass the guard and emit two framework done-signals (complete + error). Claim the resolution under a Lock so exactly one terminal fires. Also ignore acks carrying a different session_id, so a concurrent converse dispatch to the same skill in another session cannot cross-resolve. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): converse §8 trio in deactivate + ignore racy stop-cleanup artifacts - test_deactivate_inside_converse: ConverseService now reports the dispatch via the mycroft.skill.handler.* done-signal which the orchestrator translates to the §8 ovos.intent.handler.complete terminal; assert the trio (+3 messages). - The ping-pong stop tests interrupt a running skill; the async stop-pipeline cleanup (abort_question / converse.force_timeout / audio.speech.stop) fires or not depending on exactly where the stop lands, so it raced the message count in CI. Ignore those artifacts (they are not what the tests assert) and drop the flaky force_timeout async_messages assertion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): stop ping-pong tests assert only through the stop terminal The count-to-infinity ping-pong scenarios interrupt a running skill. After the stop turn's ovos.utterance.handled, the interrupted count daemon exits and races in its own CountSkill complete + a second ovos.utterance.handled — a tail whose exact contents/timing depend on where the stop lands relative to the 1s count loop (it produced a non-reproducible +1 message in CI's parallel workers). Capture only through the deterministic stop turn (eof_msgs=[ovos.utterance.handled]) and drop the racy daemon-completion tail from the expected sequence. The stop routing — ping/pong, activate, stop:skill, the StopService HandlerLifecycle trio, {skill}.stop(.response), and the stop turn's end-marker — is fully asserted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: StopService is a pipeline plugin, not an ovos-workshop skill StopService subclassed OVOSAbstractApplication purely for voc_match/voc_list/locale loading. That base class also registered it as a skill (skill_id=stop.openvoiceos), so it answered the mycroft.stop broadcast with stop.openvoiceos.stop.response — StopService 'stopping itself', a leak that polluted the stop lifecycle. Drop OVOSAbstractApplication and load the stop/global_stop .voc files via ovos-spec-tools LocaleResources (the plugin-agnostic voc matcher, same role common-query/OCP use). self.bus and self.config come from ConfidenceMatcherPipeline. No more skill machinery — no stop.openvoiceos.stop.response. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): assert stop dispatch lifecycle via ovoscope skill_id filter Stopping a running skill produces two concurrent dispatch lifecycles (the stop dispatch + the interrupted skill's own §8 trio/§9.5 terminal) whose messages interleave non-deterministically under load — the source of the persistent count-mismatch flakiness. Assert the stop dispatch lifecycle in isolation via the new ovoscope End2EndTest skill_id filter (skill_id=stop.openvoiceos) with eof_count=2 so capture spans both utterances' ovos.utterance.handled. The full stop §8 trio + §9 terminals are now modelled deterministically; the interrupted skill's §8 trio is covered (uninterrupted) by test_count. Also: TestStopServiceAsSkill -> TestStopServiceNotASkill (regression guard that StopService no longer emits stop.openvoiceos.stop.response), drop the now-dead stop-response ignores, and floor-pin ovoscope>=1.4.0a1 for the new features. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(unit): update test_stop_service for the pipeline-plugin refactor StopService no longer subclasses OVOSAbstractApplication; vocabulary matching is delegated to self._locale (ovos-spec-tools LocaleResources). Drop the removed OVOSAbstractApplication.__init__ patch from the service factory and redirect the voc_match/voc_list patches to svc._locale. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: orchestrator survives a pipeline matcher raising; fix malformed stop .voc Two issues surfaced by the StopService spec-tools refactor (LocaleResources.voc_match is strict where OVOSAbstractApplication.voc_match was lenient): 1. A pipeline matcher raising (here: a malformed .voc) propagated out of the handle_utterance loop and aborted the WHOLE utterance — no match was tried and no §9.3/§9.5 terminal fired. Wrap the match_func call in try/except: log and treat as no-match so iteration continues. Any pipeline plugin can misbehave; one bad matcher must not break the utterance. 2. ca-es/stop.voc, ca-es/global_stop.voc and de-de/global_stop.voc had single-branch groups '(x)' which ovos-spec-tools rejects (a group needs >=2 branches). The old lenient parser treated them as the mandatory token x; drop the parens to preserve that matching with a valid template. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): stop ping-pong tests assert only the deterministic stop messages The §8 SPEC trio (ovos.intent.matched / ovos.intent.handler.start / .complete) is not reliably observed in these concurrent-lifecycle stop scenarios under heavy parallel CI load (the orchestrator's spec-namespace messages drop relative to the legacy done-signal — reproduced only at full-suite xdist scale, never in isolation). Scope the assertion to the deterministic, always-present messages: the stop activation, the stop:skill/stop:global dispatch, the StopService HandlerLifecycle done-signal trio (mycroft.skill.handler.start/complete — which the orchestrator translates into the §8 terminal), and the §9.5 ovos.utterance.handled end-marker. The §8 spec trio is filtered via ignore_messages here and asserted deterministically in the single-lifecycle adapt/padatious suites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: CR2/CR4/CR5/CR7 bugs, meta-commentary cleanup, SpecMessage migration - dispatcher: fix stale timer on shutdown (mark unresolved resolved before clear) - converse_service: fix msg guard ordering before SessionManager.get - converse_service: fix skill_max=0 treated as disabled - fallback_service: fix priority=0 being treated as falsy - service: skip pipeline matchers with empty match_type (CR5) - service: resolve skill_id consistently in INTENT_MATCHED (CR7) - all tests: replace hardcoded spec topics with SpecMessage.X - all tests: add try/finally for MiniCroft cleanup - pyproject.toml: add <2.0.0 upper bound for ovos-spec-tools * fix: replace sleep(2) with deterministic skill-activation poll in stop e2e tests Under parallel CI load (xdist 4 workers) the fixed sleep was too short, causing test_count_infinity_stop_low to get 4 messages instead of 6 (the session hadn't been updated yet, so a global stop fired instead of a skill-specific stop). Replace with _wait_for_active_skill that polls SessionManager.active_skills with a 10s timeout. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
JarbasAl
force-pushed
the
feat/stop-1-conformance
branch
from
3a3e319 to
bfdd349
Compare
JarbasAl
added a commit
that referenced
this pull request
Jun 30, 2026
…kills 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>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/end2end/test_stop_refactor.py`:
- Around line 302-309: The stop-turn setup in test_stop_refactor.py is still
racing on SessionManager.sessions before the session is guaranteed to exist or
be updated. Update the test around SessionManager.sessions[session.session_id]
to wait deterministically for the live session, matching the polling approach
already used in the sibling test_stop.py tests. Keep the existing
SessionManager.sessions/session.serialize/Message flow, but only dereference the
session after the background utterance has been observed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fd4a4b8-bd03-4e73-94b0-2334d6ea2aca
📒 Files selected for processing (6)
.github/workflows/ovoscope.ymlovos_core/intent_services/stop_service.pypyproject.tomltest/end2end/test_stop.pytest/end2end/test_stop_refactor.pytest/unittests/test_stop_service.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ovoscope.yml
- ovos_core/intent_services/stop_service.py
- test/unittests/test_stop_service.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/end2end/test_stop_refactor.py`:
- Around line 302-309: The stop-turn setup in test_stop_refactor.py is still
racing on SessionManager.sessions before the session is guaranteed to exist or
be updated. Update the test around SessionManager.sessions[session.session_id]
to wait deterministically for the live session, matching the polling approach
already used in the sibling test_stop.py tests. Keep the existing
SessionManager.sessions/session.serialize/Message flow, but only dereference the
session after the background utterance has been observed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fd4a4b8-bd03-4e73-94b0-2334d6ea2aca
📒 Files selected for processing (6)
.github/workflows/ovoscope.ymlovos_core/intent_services/stop_service.pypyproject.tomltest/end2end/test_stop.pytest/end2end/test_stop_refactor.pytest/unittests/test_stop_service.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ovoscope.yml
- ovos_core/intent_services/stop_service.py
- test/unittests/test_stop_service.py
🛑 Comments failed to post (1)
test/end2end/test_stop_refactor.py (1)
302-309: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wait for the live session before dereferencing
SessionManager.sessions[...].Line 306 can still race on slower CI runs: after the fixed
time.sleep(2), the background utterance may not have populated/updated the session yet, so this lookup fails before the stop assertions run. The sibling tests intest_stop.pyalready switched this path to deterministic polling.Suggested fix
- session = SessionManager.sessions[session.session_id] + deadline = time.time() + 10 + while time.time() < deadline: + live_session = SessionManager.sessions.get(session.session_id) + if live_session and live_session.is_active(self.skill_id): + session = live_session + break + time.sleep(0.1) + else: + raise TimeoutError( + f"Skill {self.skill_id} did not activate in session {session.session_id}" + )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# The count intent self-activates the skill server-side; the Session # singleton holds the authoritative state (SESSION-1 last-write-wins). # Resend the live session for the stop turn as a real client would, so # the running skill is in active_skills — no manual activation required. deadline = time.time() + 10 while time.time() < deadline: live_session = SessionManager.sessions.get(session.session_id) if live_session and live_session.is_active(self.skill_id): session = live_session break time.sleep(0.1) else: raise TimeoutError( f"Skill {self.skill_id} did not activate in session {session.session_id}" ) message = Message(utt_topic, {"utterances": ["stop"], "lang": session.lang}, {"session": session.serialize(), "source": "A", "destination": "B"})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/end2end/test_stop_refactor.py` around lines 302 - 309, The stop-turn setup in test_stop_refactor.py is still racing on SessionManager.sessions before the session is guaranteed to exist or be updated. Update the test around SessionManager.sessions[session.session_id] to wait deterministically for the live session, matching the polling approach already used in the sibling test_stop.py tests. Keep the existing SessionManager.sessions/session.serialize/Message flow, but only dereference the session after the background utterance has been observed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/end2end/test_stop_refactor.py`:
- Around line 300-302: The test uses _wait_for_active_skill in the stop flow
without defining or importing it, which triggers the unresolved-name failure.
Add an explicit import for _wait_for_active_skill from the shared test helper
module used by the stop tests, or move the helper into a common utilities module
and import it here. Make sure the reference in this test module resolves before
the call site in the stop assertion setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f8b43bc-1a4e-40fb-8121-0140a4271fb7
📒 Files selected for processing (1)
test/end2end/test_stop_refactor.py
Comment on lines
+300
to
+302
| # Wait for the skill to activate before sending stop, matching the | ||
| # deterministic polling in test_stop.py (CI xdist race condition fix) | ||
| _wait_for_active_skill(session.session_id, self.skill_id) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Import or define _wait_for_active_skill before using it.
Line 302 calls _wait_for_active_skill, but this module does not show a local definition or import for that helper. Ruff is already flagging F821, so this test will fail with NameError before the stop assertions run. Reuse the helper via an explicit import, or move it into shared test utilities and import it from both suites.
🧰 Tools
🪛 Ruff (0.15.20)
[error] 302-302: Undefined name _wait_for_active_skill
(F821)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/end2end/test_stop_refactor.py` around lines 300 - 302, The test uses
_wait_for_active_skill in the stop flow without defining or importing it, which
triggers the unresolved-name failure. Add an explicit import for
_wait_for_active_skill from the shared test helper module used by the stop
tests, or move the helper into a common utilities module and import it here.
Make sure the reference in this test module resolves before the call site in the
stop assertion setup.
Source: Linters/SAST tools
JarbasAl
force-pushed
the
feat/stop-1-conformance
branch
from
a8434bb to
5b41560
Compare
JarbasAl
added a commit
that referenced
this pull request
Jun 30, 2026
…l events (#788) * feat: orchestrator emits the PIPELINE-1 §8 handler-lifecycle trio Make ovos-core (the orchestrator) the authoritative emitter of the OVOS-PIPELINE-1 §8 handler-lifecycle trio (ovos.intent.handler.{start,complete,error}), wrapping every dispatch: start immediately before the <skill_id>:<intent_name> dispatch (§7), then exactly one of complete (on the framework done-signal) / error (on the framework error signal or the §8.3 timeout). Each trio Message is forward-derived from the dispatch so context (incl. session) is preserved unchanged (§8, MSG-1 §5.1). New IntentDispatcher (ovos_core/intent_services/dispatcher.py) owns the §7 dispatch + §8 trio. Completion is observed across the distributed bus via the skill framework's long-standing legacy done-signals (mycroft.skill.handler.complete/.error) — framework infrastructure, not the user handler (which emits nothing per §8/§11). A per-dispatch §8.3 timeout guarantees exactly one terminal even if the handler never reports; on that path the orchestrator also owns ovos.utterance.handled. Reserved-name dispatches get the trio identically (§7.0/§7.3); the resolved-guard keeps the terminal count at one regardless of the bus namespace bridge. This is additive: the §9.5 end-marker on the ordinary matched path and the §9.2 ovos.intent.matched notification are left to ovos-workshop / follow-up changes and are out of scope here. Dep floors: ovos-bus-client>=2.5.1a1, ovos-spec-tools>=0.17.3a1 (SpecMessage.INTENT_HANDLER_* members). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat: orchestrator owns the PIPELINE-1 §9 utterance-terminal events Complete the orchestrator's ownership of the OVOS-PIPELINE-1 §6.1 per-utterance terminal sequence, on top of the §8 handler-lifecycle trio: - §9.2 ovos.intent.matched — emitted by _dispatch_match on every accepted match, before the dispatch goes out (notification, not a dispatch). Carries skill_id, intent_name (the full <skill_id>:<intent_name> match_type), lang, utterance, slots, pipeline_id. - §9.3 ovos.intent.unmatched — the no-match / all-filtered terminal, replacing the legacy complete_intent_failure (the two are bridged by ovos-spec-tools' MIGRATION_MAP, so emitting the spec topic re-delivers the legacy one to consumers still on it). - §6.4 cancellation now emits the spec ovos.utterance.cancelled. Each utterance terminates with exactly one ovos.utterance.handled (§9.5): core owns it on the no-match, cancel and §8.3-timeout paths; on the ordinary matched path the skill framework still emits it (moving that fully into core is gated on the ovos-workshop reduction). Rename _emit_match_message -> _dispatch_match (it orchestrates the §6.1 post-match steps then dispatches) and correct the IntentDispatcher docstring to scope it to §7 dispatch + §8 trio (the §9.2 notification lives in the service). Verified on a real minicroft: matched path emits matched/start/ complete/handled exactly once each; no-match path emits ovos.intent.unmatched + ovos.utterance.handled (no complete_intent_ failure). test_no_skills / test_lang_detect conformance suites green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: account for §9.2 matched + §8.1 start in activate/fallback e2e The converse-deactivate (test_activate) and fallback (test_fallback) ovoscope scenarios each gain two captured messages now that the orchestrator emits ovos.intent.matched (§9.2) and ovos.intent.handler. start (§8.1) natively before every dispatch — previously the spec trio existed only as uncounted bridge-mirrors of workshop's legacy emit. Verified on a real minicroft: the two extra messages per scenario are exactly ovos.intent.matched + ovos.intent.handler.start (the reserved-name converse:skill / fallback .request dispatches carry no mycroft.skill.handler.* done-signal, so their §8 terminal resolves via the §8.3 timeout after the end-marker, not captured). No spec-topic double-emit: ovos.intent.matched, ovos.intent.handler.start and ovos.utterance.handled each appear exactly once per utterance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat: core emits ovos.utterance.handled on the matched path too (§9.5) The orchestrator owns the universal end-marker on EVERY terminal path. The dispatcher already emitted ovos.utterance.handled on the timeout path; emit it after the complete/error terminals as well, so a matched dispatch also ends with exactly one core-emitted handled (the _pop/resolved guard fires one terminal per dispatch -> one handled). Unmatched/cancel keep their service.py handled. Workshop may still emit its own matched-path handled during the migration window; that core-vs-workshop duplicate is expected and removed later workshop-side. * chore: bump ovos-workshop floor to >=9.0.1a5 (HandlerLifecycle delegation merged) * chore: bump ovos-workshop floor to >=9.0.2a1 (matched-path handled guard) ovos-workshop 9.0.2a1 (#442) guards its matched-path ovos.utterance.handled emission behind a version check on the installed ovos-core, so once core ships the §9.5 matched-path emission the framework stops double-emitting. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore: stage version 2.3.0a1 so CI exercises the workshop §9.5 guard This feat bumps core to the 2.3.x line. ovos-workshop 9.0.2a1 suppresses its matched-path ovos.utterance.handled only when the installed ovos-core is >=2.3.0a1; staging the version here lets PR CI install a core that trips that guard, so the e2e exercises core as the single end-marker emitter rather than relying on the published 2.2.x. Release automation re-derives the final version on merge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(deps): floor-pin ovos-m2v-pipeline>=0.3.1a1 (workshop 9.x compatible) The published m2v 0.0.10a1 caps ovos-workshop<9.0.0; with core's workshop floor at 9.0.2a1 pip backtracked m2v down to 0.0.10a1 and hit that cap -> ResolutionImpossible. m2v 0.3.1a1 drops the workshop dependency entirely, so floor-pinning it (the prerelease-floor-pin pattern) forbids the backtrack and the closure resolves. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(deps): floor-pin downstream stack to spec-tools-1.x-ready prereleases core's bus-client>=2.5.1a1 pulls ovos-bus-client 2.5.1a3/2.6.0a1, which require ovos-spec-tools>=1.1.0a1. pip backtracked the mycroft/plugins/skills-essential extras down to stale releases that cap ovos-spec-tools<1.0.0 (e.g. ovos-audio 2.0.1a1) -> ResolutionImpossible. Floor-pin each to its latest prerelease (all spec-tools-1.x-ready) so the resolver can't backtrack into the capped ones, and make core's own ovos-spec-tools floor explicit at >=1.1.0a1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(deps): floor-pin ovos-adapt-parser>=1.4.2a1 (spec-tools-1.x ready) Every ovos-adapt-parser release up to 1.4.1a1 caps ovos-spec-tools<1.0.0; the uncap landed in 1.4.2a1. Floor-pin it so pip can't backtrack into the capped versions while core requires ovos-spec-tools>=1.1.0a1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: align e2e expectations with §9.5 end-marker payload - fallback speak meta carries the skill's dialog/data keys, not just skill. - ovos.utterance.handled (§9.5) is the orchestrator end-marker with EMPTY data; the stop count-to-infinity / ping-pong expectations wrongly carried the handler name on it (KeyError 'name'). The handler name stays on the framework mycroft.skill.handler.complete signal, where it belongs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: orchestrator owns ovos.utterance.handled, dispatcher only signals done PIPELINE-1 §9.5: ovos.utterance.handled is the orchestrator's universal end-marker, not the dispatcher's. The IntentDispatcher owned only the §8 handler-lifecycle trio but was also emitting the §9.5 end-marker on its terminal paths — wrong layer. - IntentDispatcher no longer emits ovos.utterance.handled. Each in-flight dispatch carries a 'done' Event set on its §8 terminal (complete/error/timeout); dispatch() returns the entry. - IntentService._dispatch_match blocks on entry.done (the §8.3 timeout guarantees it fires) then emits the single ovos.utterance.handled, uniformly with the no-match (send_complete_intent_failure) and cancel (send_cancel_event) paths it already owns. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: dispatch entry is a context manager that blocks until the §8 terminal Reads cleaner at the call site — the event-waiting is hidden behind the context manager: with self.intent_dispatcher.dispatch(reply, skill_id, intent_name): pass self.bus.emit(reply.forward(SpecMessage.UTTERANCE_HANDLED, {})) _InFlightDispatch gains __enter__/__exit__ (exit blocks on its done event). Callers that don't want to block can still wait on entry.done directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: keep explicit entry.done.wait() call site (drop context manager) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: guarantee dispatch waiters are always released (review) - terminal handlers wrap the §8 emission in try/finally so entry.done is set even if the bus emission raises (the §8.3 timer is already cancelled by _pop, so nothing else would release a blocked orchestrator). - IntentDispatcher.shutdown() sets each in-flight entry's done before clearing, so a _dispatch_match caller is never left blocked on entry.done.wait() forever. - test_timeout_emits_error_and_releases waits on entry.done instead of a fixed sleep (deterministic); test_orchestrator_emits_handled_after_terminal now asserts ovos.intent.handler.complete precedes ovos.utterance.handled. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): pin §8.3 handler-timeout to 10s in the end2end suite The orchestrator blocks on each handler's §8 terminal before emitting §9.5 ovos.utterance.handled; the production backstop is 5min. e2e handlers report in <1s (or are explicitly stopped), so pin the backstop to 10s — a dropped done-signal then fails the suite in seconds instead of stalling for minutes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: orchestrator emits utterance.handled by reacting to the §8 terminal (non-blocking) Blocking handle_utterance on entry.done.wait() deadlocked the synchronous bus: handle_utterance is itself a bus handler, so blocking it stalled the same path that must deliver the handler's done-signal — the §8.3 timer then fired ovos.intent.handler.error instead of the handler completing (8 e2e tests). Keep the orchestrator as the §9.5 owner, but non-blocking: IntentService now subscribes to the dispatcher's §8 terminal (ovos.intent.handler.complete/error) and emits ovos.utterance.handled in reaction, uniformly with the no-match and cancel paths. The dispatcher reverts to trio-only (no done event, no blocking); the §8.3 timeout still backstops via the error terminal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): expect the active skill's §8 complete terminal during stop The count/ping-pong stop tests inject a long-running intent (daemon) then stop it. When that daemon intent completes (on stop), its dispatch now emits the §8 spec terminal ovos.intent.handler.complete before the §9.5 ovos.utterance.handled, so add it to the expected sequence (got 10 messages, expected 9). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: emit utterance.handled via dispatcher on_terminal callback (deterministic) The reactive subscription (IntentService listening to ovos.intent.handler.complete) raced the ovoscope capture: FakeBus dispatches the terminal to both the capture recorder and the subscription, and when the subscription fired first it emitted the EOF ovos.utterance.handled before the terminal was recorded — so the capture stopped early and dropped the §8 complete (flaky 9-vs-10 message counts). Make it deterministic: the dispatcher invokes an on_terminal callback right AFTER the §8 terminal is on the bus; the orchestrator's _emit_utterance_handled emits the §9.5 end-marker then. Same call stack, so the terminal is always observed before the end-marker. Orchestrator still owns the emission; dispatcher stays non-blocking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Update ovos-utils version in pyproject.toml * StopService: wrap handlers in HandlerLifecycle instead of ad-hoc emission (#796) * Update Changelog * StopService: wrap handlers in HandlerLifecycle Replace rudimentary manual emission of both UTTERANCE_HANDLED (which the orchestrator now owns via IntentDispatcher._notify_terminal) and mycroft.skill.handler.complete with the canonical HandlerLifecycle context manager from ovos-bus-client. HandlerLifecycle consistently emits the full handler-lifecycle trio (start/complete/error) so that the dispatcher can properly track in-flight entries and fire §9.5 UTTERANCE_HANDLED at the right moment. Co-Authored-By: Claude * ConverseService: wrap handle_converse in HandlerLifecycle Same pattern as StopService — handle_converse is called via bus event dispatch after the converse pipeline stage matches, but had no lifecycle signalling. Without it the dispatcher's in-flight entry for converse dispatches would only ever resolve on timeout (10s). Co-Authored-By: Claude * Update stop_service unit tests for HandlerLifecycle test_handle_global_stop_emits_mycroft_stop: check for mycroft.skill.handler.start/complete instead of ovos.utterance.handled. test_handle_skill_stop_forwards_to_skill: HandlerLifecycle emits 3 messages (start, forward, complete), not 1. Co-Authored-By: Claude * Update stop service tests to include additional assertions Added assertions to check for 'mycroft.stop' and 'ovos.utterance.handled' messages in stop service tests. * feat: emit handler done-signal for converse + fallback dispatches (PIPELINE-1 §8) (#789) * feat: emit handler done-signal for converse + fallback dispatches The orchestrator's converse and fallback dispatches (PIPELINE-1 §7.3 reserved-name/polymorphic dispatches) run in skills WITHOUT ovos-workshop's handler_info wrapper, so they never produce the framework done-signal (mycroft.skill.handler.{start,complete,error}). A dispatcher observing that signal (PIPELINE-1 §8, the IntentDispatcher) therefore never sees a completion for these dispatches and falls back to its 5-minute handler timeout. Adopt the shared HandlerLifecycle util (ovos_bus_client.handler, bus-client 2.6.0a1) so core reports the dispatch->outcome span it orchestrates: - converse: handle_converse emits handler.start at the converse.request dispatch, registers a one-shot skill.converse.response listener (filtered by the targeted skill_id) -> handler.complete, with a generous timeout backstop -> handler.error. The done-signal is stamped with the targeted skill_id so a dispatcher correlates it by (session_id, skill_id). - fallback: the fallback dispatch is owned by the orchestrator; core translates each registered skill's own lifecycle markers (ovos.skills.fallback.<skill_id>.start/.response) into handler.start/handler.complete, stamped with that skill_id. Wired on register, removed on deregister/shutdown. stop_service is intentionally NOT touched here (owned by PR #777 / STOP-1). Floor ovos-bus-client>=2.6.0a1 (first release shipping ovos_bus_client.handler) and the coupled ovos-spec-tools>=1.1.0a1 it requires. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: drop ovos-spec-tools upper version cap Keep the >=1.1.0a1 floor (bus-client 2.6.0a1 requires it); remove the <2.0.0 max cap so spec-tools is free to float forward. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Update Changelog --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): ignore TTS mock audio signals in all end2end tests * fix(test): remove ovos.utterance.handled assertion from TestBusHandlers (orchestrator owns it now) * fix: correlate fallback dispatch to its §8 terminal via match_data skill_id The fallback pipeline's match_type (ovos.skills.fallback.<id>.request) carries no ':', so the orchestrator derived the dispatcher correlation key as the whole topic. The framework done-signal (re-emitted mycroft.skill.handler.complete) is stamped with the real skill_id, so IntentDispatcher._pop never matched it and the §8 ovos.intent.handler.complete terminal — and with it the §9.5 ovos.utterance.handled end-marker — only fired on the 5-minute §8.3 handler timeout. Every fallback-handled utterance was affected in production. Derive the correlation key from match_data['skill_id'] when the topic has no ':', so it equals the done-signal's skill_id. Activation is unchanged (match.skill_id stays None, no spurious {skill_id}.activate). test_fallback now asserts the §8 terminal, which can only be captured when correlation succeeds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): align stop suite with §8 trio + live-session resend StopService wraps its global/skill stop handlers in HandlerLifecycle (#796), so the legacy mycroft.skill.handler.{start,complete} done-signal now brackets mycroft.stop / {skill_id}.stop. Update the stop expectations to assert the trio. The ping-pong tests built the stop message from a stale, test-local Session that never saw the count skill's server-side self-activation (the count message, serialized before activation, folds an empty active_skills back into the singleton — correct SESSION-1 last-write-wins). Resend the live singleton session for the stop turn, as a real client tracking responses would, instead of manually activating — so the running skill is in active_skills and the ping-pong path runs without a manual activate crutch. Also drop the §8 ovos.intent.handler.complete from the expected lists where it is filtered via ignore_messages. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: make converse dispatch terminal resolution atomic + session-scoped handle_converse resolved the §8 lifecycle from two threads (the skill.converse.response handler and the timeout timer) with a non-atomic check-then-set on an Event, so both could pass the guard and emit two framework done-signals (complete + error). Claim the resolution under a Lock so exactly one terminal fires. Also ignore acks carrying a different session_id, so a concurrent converse dispatch to the same skill in another session cannot cross-resolve. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): converse §8 trio in deactivate + ignore racy stop-cleanup artifacts - test_deactivate_inside_converse: ConverseService now reports the dispatch via the mycroft.skill.handler.* done-signal which the orchestrator translates to the §8 ovos.intent.handler.complete terminal; assert the trio (+3 messages). - The ping-pong stop tests interrupt a running skill; the async stop-pipeline cleanup (abort_question / converse.force_timeout / audio.speech.stop) fires or not depending on exactly where the stop lands, so it raced the message count in CI. Ignore those artifacts (they are not what the tests assert) and drop the flaky force_timeout async_messages assertion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): stop ping-pong tests assert only through the stop terminal The count-to-infinity ping-pong scenarios interrupt a running skill. After the stop turn's ovos.utterance.handled, the interrupted count daemon exits and races in its own CountSkill complete + a second ovos.utterance.handled — a tail whose exact contents/timing depend on where the stop lands relative to the 1s count loop (it produced a non-reproducible +1 message in CI's parallel workers). Capture only through the deterministic stop turn (eof_msgs=[ovos.utterance.handled]) and drop the racy daemon-completion tail from the expected sequence. The stop routing — ping/pong, activate, stop:skill, the StopService HandlerLifecycle trio, {skill}.stop(.response), and the stop turn's end-marker — is fully asserted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: StopService is a pipeline plugin, not an ovos-workshop skill StopService subclassed OVOSAbstractApplication purely for voc_match/voc_list/locale loading. That base class also registered it as a skill (skill_id=stop.openvoiceos), so it answered the mycroft.stop broadcast with stop.openvoiceos.stop.response — StopService 'stopping itself', a leak that polluted the stop lifecycle. Drop OVOSAbstractApplication and load the stop/global_stop .voc files via ovos-spec-tools LocaleResources (the plugin-agnostic voc matcher, same role common-query/OCP use). self.bus and self.config come from ConfidenceMatcherPipeline. No more skill machinery — no stop.openvoiceos.stop.response. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): assert stop dispatch lifecycle via ovoscope skill_id filter Stopping a running skill produces two concurrent dispatch lifecycles (the stop dispatch + the interrupted skill's own §8 trio/§9.5 terminal) whose messages interleave non-deterministically under load — the source of the persistent count-mismatch flakiness. Assert the stop dispatch lifecycle in isolation via the new ovoscope End2EndTest skill_id filter (skill_id=stop.openvoiceos) with eof_count=2 so capture spans both utterances' ovos.utterance.handled. The full stop §8 trio + §9 terminals are now modelled deterministically; the interrupted skill's §8 trio is covered (uninterrupted) by test_count. Also: TestStopServiceAsSkill -> TestStopServiceNotASkill (regression guard that StopService no longer emits stop.openvoiceos.stop.response), drop the now-dead stop-response ignores, and floor-pin ovoscope>=1.4.0a1 for the new features. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(unit): update test_stop_service for the pipeline-plugin refactor StopService no longer subclasses OVOSAbstractApplication; vocabulary matching is delegated to self._locale (ovos-spec-tools LocaleResources). Drop the removed OVOSAbstractApplication.__init__ patch from the service factory and redirect the voc_match/voc_list patches to svc._locale. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: orchestrator survives a pipeline matcher raising; fix malformed stop .voc Two issues surfaced by the StopService spec-tools refactor (LocaleResources.voc_match is strict where OVOSAbstractApplication.voc_match was lenient): 1. A pipeline matcher raising (here: a malformed .voc) propagated out of the handle_utterance loop and aborted the WHOLE utterance — no match was tried and no §9.3/§9.5 terminal fired. Wrap the match_func call in try/except: log and treat as no-match so iteration continues. Any pipeline plugin can misbehave; one bad matcher must not break the utterance. 2. ca-es/stop.voc, ca-es/global_stop.voc and de-de/global_stop.voc had single-branch groups '(x)' which ovos-spec-tools rejects (a group needs >=2 branches). The old lenient parser treated them as the mandatory token x; drop the parens to preserve that matching with a valid template. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(e2e): stop ping-pong tests assert only the deterministic stop messages The §8 SPEC trio (ovos.intent.matched / ovos.intent.handler.start / .complete) is not reliably observed in these concurrent-lifecycle stop scenarios under heavy parallel CI load (the orchestrator's spec-namespace messages drop relative to the legacy done-signal — reproduced only at full-suite xdist scale, never in isolation). Scope the assertion to the deterministic, always-present messages: the stop activation, the stop:skill/stop:global dispatch, the StopService HandlerLifecycle done-signal trio (mycroft.skill.handler.start/complete — which the orchestrator translates into the §8 terminal), and the §9.5 ovos.utterance.handled end-marker. The §8 spec trio is filtered via ignore_messages here and asserted deterministically in the single-lifecycle adapt/padatious suites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: CR2/CR4/CR5/CR7 bugs, meta-commentary cleanup, SpecMessage migration - dispatcher: fix stale timer on shutdown (mark unresolved resolved before clear) - converse_service: fix msg guard ordering before SessionManager.get - converse_service: fix skill_max=0 treated as disabled - fallback_service: fix priority=0 being treated as falsy - service: skip pipeline matchers with empty match_type (CR5) - service: resolve skill_id consistently in INTENT_MATCHED (CR7) - all tests: replace hardcoded spec topics with SpecMessage.X - all tests: add try/finally for MiniCroft cleanup - pyproject.toml: add <2.0.0 upper bound for ovos-spec-tools * fix: replace sleep(2) with deterministic skill-activation poll in stop e2e tests Under parallel CI load (xdist 4 workers) the fixed sleep was too short, causing test_count_infinity_stop_low to get 4 messages instead of 6 (the session hadn't been updated yet, so a global stop fired instead of a skill-specific stop). Replace with _wait_for_active_skill that polls SessionManager.active_skills with a 10s timeout. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
JarbasAl
added a commit
that referenced
this pull request
Jun 30, 2026
…kills 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>
…lobal ovos.stop, session drain)
Adopt the OVOS-STOP-1 spec bus surface in the stop pipeline plugin, relying on
the ovos-spec-tools MIGRATION_MAP bus bridge for the 1:1 legacy renames.
- §5.3 global handler now emits SpecMessage.STOP (ovos.stop); the bus bridges it
to legacy mycroft.stop, so no hand-rolled dual-emit (§3.1 exactly-one).
- §4.2 ping-pong: subscribe SpecMessage.STOP_PONG (ovos.stop.pong) — bridged from
legacy skill.stop.pong; emit the broadcast SpecMessage.STOP_PING (ovos.stop.ping)
once, keeping the per-skill {skill_id}.stop.ping for back-compat with un-migrated
skills (the per-skill ping is a placeholder the broadcast replaces and cannot be
statically bus-bridged).
- §6.1 response_mode for the dispatch target is cleared via the committed session.
Deferred (documented inline):
- §2/§3.1 intent_name rename to exactly "stop"/"global_stop": match_type is both
the reserved intent_name AND the dispatch bus topic (and the blacklisted_intents
key) in the current IntentHandlerMatch contract; separating them needs a
PIPELINE-1 dispatch-layer change.
- §6.2 structured draining of active_handlers/converse_handlers: those
SESSION-1/CONVERSE-1 fields do not yet exist on the bus-client Session (recency
input is session.active_skills).
§6.3 blacklisted_intents is already enforced by the orchestrator (service.py keys
on match_type) and tracks the deferred intent_name decision.
Pin ovos-spec-tools>=0.11.0a1 (STOP-1 MIGRATION_MAP entries). Tests updated to
assert the spec topics (bus bridge preserves legacy).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…xpectations
The ovoscope e2e run regressed on the STOP-1 changes (4 scenarios x both bus
namespaces). Two distinct causes:
1. Real regression (test_count_infinity_global, the *_voc / stop-service-as-skill
global paths): handle_global_stop emitted only the spec ``ovos.stop``. The
spec->legacy bridge is NOT guaranteed (opt-in on MessageBusClient, off on the
pure-spec FakeBus path the ovoscope namespaces use, emit_legacy=False), and
ovos-workshop skills still subscribe ONLY ``mycroft.stop`` (their migration to
``ovos.stop`` is deferred). So a spec-only broadcast silently failed to stop
them and no ``{skill_id}.stop.response`` was produced. handle_global_stop now
also emits the legacy ``mycroft.stop`` directly, mirroring the back-compat
per-skill ``{skill_id}.stop.ping`` already kept in _collect_stop_skills. The
two topics hit disjoint subscriber sets, so it is not a double broadcast to
any one skill.
2. Outdated expected sequences (test_count_infinity_active / _stop_low /
test_stop_with_active_skill_ping_pong):
- the new ``ovos.stop.ping`` broadcast is forwarded from the source utterance
and keeps its original source (directed at skills, exactly like the per-skill
ping), so it must be listed in keep_original_src — it was asserted against the
flipped reply source ('B' vs 'A').
- the pong the bus actually carries is the producer's legacy ``skill.stop.pong``
(ovos-workshop has not migrated to ``ovos.stop.pong``); the captured sequence
must assert that legacy topic, not the spec one.
Global-path expected sequences updated to include the legacy ``mycroft.stop``
back-compat broadcast; the stop-service-as-skill test now expects its idempotent
second stop.response. Unit test asserts both ``ovos.stop`` and ``mycroft.stop``
are emitted, spec first.
Verified locally against the CI-resolved dependency set (ovos-utils 0.12.1a1,
ovos-workshop 8.3.0a1, ovos-spec-tools 0.11.0a1, ovos-bus-client 2.3.0a2,
ovoscope 1.0.0a1, ovos-skill-count 0.0.3a4, padatious): full stop e2e suite
12 passed / 24 subtests passed, stop unit tests 28 passed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Implements the deferred STOP-1 §4.1/§5.2/§6.2 obligations, now that the canonical Session carries active_handlers / converse_handlers / response_mode (ovos-bus-client #234 feat/session-spec-fields + ovos-spec-tools fix/session-fallback-handlers). - §4.1 step 4: _select_stop_target picks the single highest-activated_at positive pong responder from session.active_handlers (spec recency field, not the legacy active_skills list order); activated_at ties break on the head-first list position. Wired into match_high/match_low. - §6.2: a targeted stop's updated_session removes ONLY the dispatch target from active_handlers via Session.remove_active_handler. - §5.2: a global_stop's updated_session now drains active_handlers=[] and converse_handlers=[] and clears response_mode (was returned unmodified) via _drain_global_stop_session. - §6.1: target response_mode clearing kept; disable_response_mode is the structured clear_response_mode(skill_id) shim (single-holder window). The intent_name "stop"/"global_stop" rename stays DEFERRED (coupled to a PIPELINE-1 §3.1 dispatch-shape change). Unit tests added: global_stop drains both lists + clears response_mode; targeted stop removes only the target from active_handlers; multi-pong selects highest-activated_at (+ tie-break, candidate-restriction). Needs ovos-bus-client floor bump to the first published release carrying the active_handlers/converse_handlers/response_mode Session API once #234 lands; current >=2.2.0a1 pin left unchanged to stay resolvable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…kills 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>
StopService no longer inherits OVOSAbstractApplication; vocabulary matching is delegated to self._locale (LocaleResources). Tests in TestStop1Draining that patched svc.voc_match/voc_list directly now patch svc._locale instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
JarbasAl
force-pushed
the
feat/stop-1-conformance
branch
from
5b41560 to
1860155
Compare
…racking on global stop test_stop_refactor.py used _wait_for_active_skill without importing it. TestGlobalStopVocWithActiveSkill also triggered ovoscope's active-skill tracker with the count skill pre-activated, then global_stop drained the session — causing a false failure. Set test_active_skills=False since the point of that test is that global_stop clears the session, not that skills stay active. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pSkillCanHandleFalse
The local Session("123") object has empty active_skills; after
_wait_for_active_skill confirms the skill activated server-side,
re-fetch from SessionManager so the stop utterance carries the live
session with the count skill in active_skills. Without this,
stop_service sees no active skills and falls back to stop:global
instead of stop:skill.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…l_stop) Dispatch a targeted stop on the reserved intent_name `stop` (`<target_skill_id>:stop`, skill_id == the target) and a global stop on `<pipeline_id>:global_stop` (skill_id == the stop plugin's pipeline_id), per OVOS-STOP-1 §2/§3.1. - The `<target>:stop` dispatch reaches skills via the legacy `<skill_id>.stop`: the ovos-spec-tools namespace translator bridges it when the bus emits legacy counterparts; otherwise `handle_skill_stop` re-emits it (guarded so it never double-delivers). The global handler self-dispatches on `<pipeline_id>:global_stop` and emits the `ovos.stop` broadcast. - A stop is a termination, not an activation: stop Matches set `suppress_activation`, and `_dispatch_match` honours it so the target skill is not sent a bogus `.activate` (it would otherwise re-activate the skill being stopped). Converse/fallback activation is unchanged. - e2e: the targeted stop shares the interrupted skill's skill_id, so the concurrent scenarios isolate the stop lifecycle by the stop plugin's pipeline_id (ovoscope filter) and ignore the skill's async `<skill_id>.stop.response`. Fully backward compatible: `mycroft.stop`/`<skill_id>.stop` and the legacy `stop:global`/`stop:skill` handlers are retained. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…3.0a1 (:stop bridge) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A still-active/speaking target stopped mid-dispatch triggers handle_stop_confirmation, which emits converse.force_timeout / abort_question / audio.speech.stop on the stop pipeline_id. These are timing-dependent, so ignore them (as the ping-pong path already does) to keep the pipeline_id-isolated assertion stable under CI parallelism. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tch topics; timeless comments Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adopts the OVOS-STOP-1 spec bus surface in the stop pipeline plugin
(
ovos_core/intent_services/stop_service.py), relying on the ovos-spec-toolsMIGRATION_MAPbus bridge for the 1:1 legacy renames. The skill side is handledseparately by ovos-workshop #415 — this PR is core-only.
Done
handle_global_stopemitsSpecMessage.STOP(
ovos.stop); the bus bridges it to legacymycroft.stop(it is inMIGRATION_MAP), so there is no hand-rolled dual-emit (§3.1 exactly-one).SpecMessage.STOP_PONG(
ovos.stop.pong); the bridge also delivers un-migrated skills' legacyskill.stop.pongto the same handler.SpecMessage.STOP_PING(
ovos.stop.ping) once. The per-skill{skill_id}.stop.pingis kept forback-compat: it is a structural placeholder the broadcast replaces and which
cannot be statically bus-bridged, and the currently-released ovos-workshop
still listens only on the per-skill ping.
response_modeis cleared viathe committed session (
disable_response_mode, already present).ovos-spec-tools>=0.11.0a1(the STOP-1MIGRATION_MAPentries land in thenext spec-tools release; floor-pin per the prerelease rule).
Deferred (documented inline)
"stop"/"global_stop". In thecurrent
IntentHandlerMatchcontract,match_typeis both the reservedintent_name and the dispatch bus topic —
IntentServicedispatches viamessage.reply(match.match_type)and keyssession.blacklisted_intentson it.The orchestrator routes the
stop:global/stop:skilltopics to the handlers.Renaming
match_typeto the bare spec names without losing the dispatch routerequires a PIPELINE-1 dispatch-layer change (separating intent_name from the
dispatch topic). Left as-is to avoid breaking the dispatch contract.
active_handlers/converse_handlers. ThoseSESSION-1/CONVERSE-1 fields do not yet exist on the bus-client
Session; thecurrent recency input is
session.active_skills. Deferred until the fields land.§6.3 blacklisted_intents
Already enforced by the orchestrator (
service.pyfilters onmatch_type in session.blacklisted_intents). It tracks the deferred intent_name decision above(keys on
stop:global/stop:skilluntil that rename lands).Tests
test/unittests/test_stop_service.py— 28 pass; pong-topic + global-emitassertions updated to the spec topics, new
test_spec_broadcast_ping_emitted.test/end2end/test_stop.py::TestStopNoSkills— 3 tests / 6 subtests pass(spec + legacy namespaces); expected sequences updated to the spec topics.
test/end2end/test_stop_refactor.py—TestGlobalStopVocabulary(4 subtests)and
TestStopServiceAsSkillpass; the latter now runs withemit_legacy=Trueso the spec broadcast reaches the un-migrated bundled StopService OVOSSkill.
TestCountSkills/TestGlobalStopVocWithActiveSkill/TestStopSkillCanHandleFalserequire
ovos-skill-count(not installed locally) — not run here; theirexpected sequences were updated for the spec topics + broadcast ping. CI runs them.
test/unittests/(245) green.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes