fix: connect SessionManager to the bus in standalone skill containers - #526
fix: connect SessionManager to the bus in standalone skill containers#526JarbasAl wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 43 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
Ready for review! The automated tests have finished. ✅I've aggregated the results of the automated checks for this PR below. 🔍 LintThe automated gnomes have finished their shift. 🍄 ❌ ruff: issues found — see job log 📋 Repo HealthEnsuring the repo isn't allergic to new features. 🤧 ✅ All required files present. Latest Version: ✅ 🔒 Security (pip-audit)Ensuring our dependency tree is clean of rot. 🌳 ✅ No known vulnerabilities found (74 packages scanned). ⚖️ License CheckI've checked the licenses of all dev-dependencies too. 🛠️ ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔨 Build TestsChecking if the gears are still turning smoothly... ⚙️ ✅ All versions pass
Signed, sealed, and delivered by the OVOS bot. 📧 |
Root cause
Standalone skill containers in ovos-workshop never connect
SessionManagerto the bus.SessionManager.wait_while_speaking(and thereforespeak(wait=True)) silently no-ops because of theif not cls.bus: returnguard inSessionManager, sinceSessionManager.busstaysNone.SessionManager.connect_to_bus()has exactly one caller in the whole ovos stack: ovos-core'sIntentService, atovos_core/intent_services/service.py:163(installed copy; matches the confirmed live source at ovos-core 2.5.3a1+ used to build/verify this PR). That call wires the core process's bus.ovos_workshop.skill_launcher.SkillContainer— the real standalone skill-container launcher — had zeroSessionManagerwiring anywhere; a grep of the wholeovos_workshoppackage forconnect_to_busreturned no hits before this fix.I checked whether ovos-core wires
SessionManageranywhere else (e.g. a skill-manager/skill-service process) that might make this moot for some deployment shapes. With ovos-core installed as a test dependency in this repo, I grepped its entire installed package forSessionManagerusage: it only appears inintent_services/service.py,intent_services/stop_service.py,intent_services/converse_service.py, andintent_services/fallback_service.py, andconnect_to_busis called only fromservice.py:163. So core has no other, separate wiring path forSessionManager— this was verified against the installed ovos-core package during this session, not assumed.Fix
ovos_workshop/skill_launcher.py:from ovos_bus_client.session import SessionManagerimport.SkillContainer._connect_to_core(), right after the bus is created/connected (mirroring where ovos-core'sIntentServicecalls it, immediately after its bus is available), added:SkillContainerwas the only genuine bus-owning standalone entry point found inovos_workshop; a repo-wide grep forMessageBusClient()/run_in_thread/connected_event.wait/__main__turned up no other place that constructs or owns its own bus for a skill.Regression test
Added
TestSkillContainer.test_connect_to_core_wires_session_manager_busintest/unittests/test_skill_launcher.py. It builds aSkillContainerwith aFakeBus, drives_connect_to_core()(short-circuiting the blockingwait_for_core()retry loop), and assertsSessionManager.bus is not NoneandSessionManager.bus is <the container's bus>.SessionManagerholds global class-level state, sosetUp/tearDownresetSessionManager.bus = Nonearound the test.Red (before fix, source fix reverted, test unchanged):
Green (after fix reapplied):
Full suite
No pre-existing failures observed; the run is fully green.
Downstream oracle
Downstream oracle: ovos-test-harness PR #26's
test_speak_wait_unblocks_on_audio_output_endis a strict xfail citing this exact gap. Once this fix is released, that test should flip to XPASS and the harness's xfail marker can be dropped.Adversarial review (2026-08-13) — MERGE, no blocking defects
Independent re-verification: red/green reproduced from source (fix deleted → test fails; restored → 25 pass file / 561 pass suite); test isolation proven first/last in suite order; floor pin
ovos_bus_client>=2.6.2a2carriesconnect_to_bus(no startup crash at the floor; one cosmetic deprecation warning there); wiring sits between bus connect and every skill-load path, so no early-init window; single-container entrypoint makes the non-idempotent handler registration moot (and migrated topics carry a mirror-guard anyway).Companion gap found (not this PR's scope): ovos-core's skills process has the same missing wiring when
enable_intent_service=False(theSkillManager(bus)default for embedders, and a documented CLI flag) — fixed separately in ovos-core.Negative result noted: the default-session multi-writer echo this enables is a pre-existing class (every client already stamps its default session on outbound traffic); tightening belongs in ovos-bus-client, not here.