fix: give FakeBus the intent-topic bridge (RULE 1/RULE 2 parity with MessageBusClient) - #417
Conversation
…MessageBusClient) FakeBus already mirrored MessageBusClient's fixed legacy<->ovos.* namespace migration (NamespaceTranslator), but not the per-intent dispatch-topic bridge the real client applies on every emit: the send-side legacy twin and the receive-side canonical modernize. In-process tests that raw-emit a legacy <skill_id>:IntentName.intent topic never reached a canonical-only listener (ovos-workshop >= 9.3.11a2 dropped its own dual-bind), while a real websocket deployment dealiased fine. Port the same two rules into FakeBus.emit()/AsyncFakeBus.emit(), gated by the existing modernize/ emit_legacy flags and guarded against recursion with the same INTENT_COMPAT_TWIN_KEY marker the real client uses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesFakeBus and AsyncFakeBus now bridge canonical and legacy Intent bridge contracts and guards
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Emitter
participant FakeBus
participant IntentHandler
Emitter->>FakeBus: Emit INTENT frame
FakeBus->>FakeBus: Translate topic and create twin when enabled
FakeBus->>IntentHandler: Dispatch one deduplicated frame
Possibly related PRs
✨ 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 |
Checking the status... all automated tasks are done! ✅I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthEnsuring the repository remains a happy place. 😊 ✅ All required files present. Latest Version: ✅ ⚖️ 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. 🔍 LintI've performed a routine sweep of your changes. 🧹 ❌ ruff: issues found — see job log 📊 CoverageEnsuring no code path is left in the shadows. 🌑 ✅ 85.2% total coverage Files below 80% coverage (5 files)
Full report: download the 🔒 Security (pip-audit)Ensuring no malicious actors are hitching a ride. 🎭 ✅ No known vulnerabilities found (47 packages scanned). 🏷️ Release PreviewChecking if we're ready for the big release. 🏁 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🔨 Build TestsBuild test complete! Let's see if everything fits together. 🧩 ✅ All versions pass
The pulse of the OpenVoiceOS codebase 💓 |
…tent bridge Adversarial review found two defects in the FakeBus RULE1/RULE2 intent-topic bridge (#417): - on() had no subscribe-side dedup for the intent bridge at all, so a handler bound to both the canonical and legacy-suffixed spelling of an intent (the common ovos-workshop dual-bind, which uses a fresh wrapper closure per registration) fired twice for one dispatch. Ported MessageBusClient's per-topic-pair mirror guard (shared by every registration on either spelling, not per-handler) into FakeBus.on()/AsyncFakeBus.on(). - emit() popped the RULE2 twin marker too late (inside _bridge_intent_topic, after the main handler dispatch), and the internally-generated RULE1 twin carried the marker into local handler context. A handler that forwarded that twin's context onto an unrelated topic would incorrectly brand the new frame a twin and silently suppress its own modernization. Marker is now popped at the top of emit()/AsyncFakeBus.emit() before any dispatch, and the RULE1 twin no longer carries the marker into local delivery at all (mirroring the real client, whose receiving process pops it before any local handler runs) -- this also makes the pair guard's payload+ context fingerprint match the canonical dispatch's, which the dedup above depends on. Rewrote test_no_double_fire_dual_listener to assert ONE call (matching the real client) and test_canonical_emit_twin_marked to assert the marker is NOT visible locally; added starvation and marker-leak regression tests. Every new/changed assertion verified to fail against the pre-fix source via patch-revert before the fix, and pass after.
1 participant
FakeBus already knew how to bridge the fixed legacy/spec namespace pairs (things like
speakvsovos.utterance.speak), but it never learned the other bridge that the realMessageBusClientdoes on every emit: turning a canonical intent dispatch into its old.intent-suffixed twin, and turning a raw legacy-suffixed intent emit back into the canonical spelling. Since ovos-workshop stopped binding both names itself, any in-process test that emits directly on the old<skill_id>:IntentName.intenttopic was landing on nothing, even though the exact same message over a real websocket connection reaches the handler fine. This showed up as a real failure in ovos-core's end2end suite (test_intent_alias_backcompat.py::test_legacy_dispatch_topic_fires_handler).The fix ports both halves of that bridge into
FakeBus.emit()andAsyncFakeBus.emit(), copied fromovos_bus_client.client.client.MessageBusClient: a canonical dispatch also fires the legacy-suffixed listener, and a legacy dispatch also fires the canonical listener, using the samemodernize/emit_legacyflags FakeBus already had wired up. The double-fire guard fromMessageBusClient.on()is ported too, keyed per topic pair rather than per handler, because ovos-workshop 9.3.2a1+ binds the same skill method to both spellings through a fresh wrapper closure per registration, so a per-handler guard would miss the duplicate. One deliberate difference from the real wire stays, and it's the same convention FakeBus already uses for the namespace bridge above: the legacy/canonical twin is delivered straight to local listeners and does not re-fire the"message"firehose or carry the wire-only twin marker into local handler context, so oneemit()call still yields exactly one captured message instead of the two frames a real websocket round-trip produces.I added a new test file,
test_fakebus_intent_topic_bridge.py, covering both directions, the no-double-fire case, the marker-based recursion guard, and that unrelated topics are left alone. Before the fix, a legacy emit reaching a canonical-only listener didn't work — I confirmed this by reverting the patch and re-running the test, which then failed as expected. The full ovos-utils suite passes (949 tests, 1 skipped, no new failures).I also cross-checked this against ovos-core directly. I built a venv with an ovos-core worktree plus this ovos-utils branch installed over it, and ran the ovos-core end2end suite.
test_legacy_dispatch_topic_fires_handlerflips from failing to passing for the "legacy" namespace case, exactly as expected. It still fails for the "spec" namespace case, but that's a separate, pre-existing thing: that particular subtest configures the bus withmodernize=False, which turns the bridge off entirely — and it turns it off in the real client the same way, so this isn't something FakeBus can or should paper over. I checked this: with the fix reverted, both subtests failed; with the fix applied, only the "spec" one still fails, for the reason above. Nothing else in the end2end suite regressed (43 tests / 71 subtests passing, same as before).Summary by CodeRabbit
New Features
Bug Fixes
Tests