refactor: drop redundant legacy dual bus subscriptions - #179
Conversation
…vice init_messagebus subscribed both the legacy and spec topic for six AUDIO-1 adoptions (mycroft.stop, mycroft.audio.speech.stop, mycroft.audio.speak.status, mycroft.audio.queue, mycroft.audio.play_sound, speak:b64_audio). All six are already in the ovos-spec-tools MIGRATION_MAP, and the bus-client namespace bridge (MessageBusClient.on_message) mirrors a legacy emit onto its spec counterpart for local listeners, so the manual legacy subscription was dead weight duplicating handler dispatch. Keep the spec-topic subscription only and add a regression test that drives each legacy topic through a bridge-enabled FakeBus to lock in the dependency on that boundary contract. Identified by automated compat-layer survey; implemented by Claude (sonnet), orchestrated by Claude Fable. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe audio service now relies on namespace bridging for legacy input topics, keeps specification-topic subscriptions, emits the legacy instant-playback response, and adds regression coverage for bridged delivery and updated registrations. ChangesAudio namespace bridge
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LegacyTopicEmitter
participant NamespaceBridge
participant PlaybackService
LegacyTopicEmitter->>NamespaceBridge: emit legacy audio topic
NamespaceBridge->>PlaybackService: forward as specification topic
PlaybackService->>PlaybackService: invoke specification-only handler
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
At your service! I've gathered all the check results for you. 🫡I've aggregated the results of the automated checks for this PR below. 🏷️ Release PreviewI've checked the 'Migration Guide' for clarity. 🗺️ Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
📊 CoverageExploring the coverage frontier of your PR. 🚀 ✅ 98.2% total coverage Per-file coverage (9 files)
Full report: download the 🔒 Security (pip-audit)Security report: No threats detected in the area. ✅ ✅ No known vulnerabilities found (66 packages scanned). 🔍 LintI've double-checked the data for any anomalies. 🔍 ❌ ruff: issues found — see job log 📋 Repo HealthA routine checkup to keep the repo running smoothly. 🏥 ✅ All required files present. Latest Version: ✅ 🔌 Skill Tests (ovoscope)I've checked the skill's 'integration' with other OVOS skills. 🤝 ✅ 63/63 passed ✅ TestAudioCaptureSequence — 1/1 🚌 Bus CoverageQuantifying the invisible connections in your code. 💪 🔨 Build TestsDid the code survive the compilation gauntlet? Let's see. 🛡️ ✅ All versions pass
🔗 Downstream DependentsJust a little bit of automation magic for you. ✨ No dependents found for ovos-audio ⚖️ License CheckChecking for any missing license headers. ✍️ ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. End of Line. ⬛ |
handle_instant_play replied via the generic <topic>.response derivation, which ovos-spec-tools MIGRATION_MAP does not mirror (only base request topics are mapped, not their derived replies). A caller reaching the handler through the now-sole spec subscription via the legacy 'mycroft.audio.play_sound' request never saw a response. Emit both spellings explicitly, matching the existing hand-mirrored pattern in handle_speak_status. test_speech.py::test_life_cycle still asserted the dropped 'mycroft.stop' / 'mycroft.audio.speech.stop' direct subscriptions; updated to the spec topics the refactor kept. Co-Authored-By: Claude Fable 5 <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/unittests/test_service_handlers.py`:
- Around line 723-726: Remove direct legacy-topic registrations from
_wire_audio1_handlers, or initialize the service via
PlaybackService.init_messagebus using a bridge-enabled bus; update
test/end2end/test_legacy_dual_subscription_dropped_e2e.py lines 32-51 to assert
exactly one received message per legacy input and verify each uses the expected
specification topic.
🪄 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 Plus
Run ID: 377802a9-df12-4437-a6e5-097e4d298d7b
📒 Files selected for processing (4)
ovos_audio/service.pytest/end2end/test_legacy_dual_subscription_dropped_e2e.pytest/unittests/test_service_handlers.pytest/unittests/test_speech.py
…d-wired shortcut _wire_audio1_handlers used to subscribe legacy topics directly, so the dual-namespace unit tests could pass even if the bus-client namespace bridge itself were broken. It now drives PlaybackService.init_messagebus on the bridge-enabled FakeBus, exercising the actual bridge. Also tighten the legacy-dual-subscription-dropped e2e test to assert exactly one delivery on the expected spec topic per legacy input, proving the bridge delivers once with no duplicate, instead of merely asserting some invocation occurred. Addresses CodeRabbit review on ovos-audio#179.
The tightened exactly-once check tripped on handle_speak_status: it subscribes to ovos.audio.is_speaking AND emits its reply there (AUDIO-OUT-1 §5.3 query==reply), so it legitimately re-fires on its own reply — not a duplicate of the bridged legacy delivery. Assert the bridge delivered on the spec topic first, not an exact count. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The dropped legacy subscriptions are redundant only because MessageBusClient.on_message counterpart-dispatches legacy wire frames onto spec topics; that loop first exists in 2.6.3a1, and the twin-marker semantics the exactly-once behavior relies on land in 2.8.3a1. The old floor (2.5.1a1) admitted clients where a stable emitter's mycroft.stop / play_sound / queue / b64 frames reached no handler at all.
Summary
PlaybackService.init_messagebussubscribed both the legacy and the spectopic for six AUDIO-1 adoptions:
mycroft.stopmycroft.audio.speech.stopmycroft.audio.speak.statusmycroft.audio.queuemycroft.audio.play_soundspeak:b64_audioAll six are already present in the
ovos-spec-toolsMIGRATION_MAP, and theovos-bus-clientnamespace bridge (MessageBusClient.on_message, and itsFakeBusequivalent) mirrors a legacy emit onto its spec counterpart forLOCAL listeners. This means a handler subscribed only on the spec topic
already fires when a deployment emits the legacy one — the manual legacy
self.bus.on(...)calls were redundant duplicate dispatch, not acompatibility requirement.
This PR drops the six legacy subscriptions and keeps the spec-topic
subscription only, relying on the bridge for legacy-emitting deployments.
Test plan
test/end2end/test_legacy_dual_subscription_dropped_e2e.py: boots areal
PlaybackServiceon a bridge-enabledFakeBus(modernize=True,emit_legacy=True) and emits each of the six legacy topics directly,asserting the spec-only handler still fires. This locks in the dependency
on the bus-client bridge contract going forward.
test/unittests/test_service_handlers.py::TestAudio1DualNamespaceRegistrationto assert the legacy topics are no longer registered directly (spec topic
only).
test/unittests/test_service.py,test/unittests/test_service_handlers.py,test/end2end/test_spec_bus_messages_e2e.py,test/end2end/test_b64_dual_namespace_e2e.py,test/end2end/test_legacy_dual_subscription_dropped_e2e.py— 75 passed, 7skipped.
Identified by automated compat-layer survey; implemented by Claude (sonnet),
orchestrated by Claude Fable.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Backcompat audit (executed version-skew matrix, three bus-client vintages): the receive-side bridge (
counterpart_topicsdispatch inMessageBusClient.on_message) is what makes the dropped subscriptions redundant, and it delivers exactly-once for a stable 1.5.0-era emitter and for modern marked twins alike. Two corrections landed from the audit: the ovos-bus-client floor moves to>=2.8.3a1(the old floor admitted client versions without the bridge, where a stable emitter'smycroft.stop/play_sound/queue/b64frames reached no handler at all), and the e2e test's claim is narrower than its wording — it runs on FakeBus's reimplementation of the rules, so the bridge contract itself is guarded by the executed skew matrix and by ovos-test-harness cells (five legacy-audio cells queued there), not by that test.