feat: FakeBus mirrors the .intent-suffixed twin for aliased intents - #411
feat: FakeBus mirrors the .intent-suffixed twin for aliased intents#411JarbasAl wants to merge 7 commits into
Conversation
Old ovos-workshop built the per-intent dispatch topic from the padatious resource filename, so the `.intent` extension leaked onto the wire. The real MessageBusClient now mirrors an intent dispatch onto that suffixed twin when emit_legacy is on. A test double that skipped the mirror would hide the compat path from every harness built on it. FakeBus and AsyncFakeBus now share a `_LegacyIntentBridge` that calls `ovos_spec_tools.intent_topics.legacy_reemit_targets` after the namespace counterpart dispatch, with an `IntentAliasRegistry` filled from the bus's own on() / once() calls. Blanket mode (`intent_reemit_blanket`, off by default) mirrors every intent dispatch for pure-bus listeners that never register. The spec-tools import is guarded: against a release without the intent-topic helpers both fake buses behave exactly as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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)
📝 WalkthroughWalkthroughFakeBus and AsyncFakeBus now bridge canonical and ChangesLegacy intent-topic bridge
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FakeBus
participant LegacyIntentBridge
participant Firehose
participant LocalHandlers
FakeBus->>LegacyIntentBridge: emit intent counterpart
LegacyIntentBridge->>Firehose: publish marked wire frame
LegacyIntentBridge->>LocalHandlers: dispatch unmarked local event
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Hello there! I've processed your latest changes. 🌊I've aggregated the results of the automated checks for this PR below. 🔍 LintThe automated sentinel is back with news. 💂♂️ ❌ ruff: issues found — see job log ⚖️ License CheckReading the fine print with a magnifying glass. 🔍 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🏷️ Release PreviewGet ready for the next big release! 🚀 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🔒 Security (pip-audit)I've checked the digital signatures of our packages. ✍️ ✅ No known vulnerabilities found (47 packages scanned). 📊 CoverageHow much of the logic is under the microscope? 🔬 ✅ 85.3% total coverage Files below 80% coverage (5 files)
Full report: download the 📋 Repo HealthI've checked the repo's posture (aka architectural alignment). 🧘 ✅ All required files present. Latest Version: ✅ 🔨 Build TestsAssembling the puzzle pieces of your PR. 🧩 ✅ All versions pass
Closing the loop on this automated check ♻️ |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The fake buses bridge the canonical `<skill_id>:<intent_name>` dispatch topic and the legacy `.intent`-suffixed spelling with the same two rules the real `MessageBusClient` now uses. The client splits them over a wire send and a wire receive; a fake bus is one process, so both land in `emit`: * a canonical dispatch also fires its suffixed twin, marked in `context` with `_intent_compat_twin`; * a suffixed dispatch that is not already such a twin also fires its canonical spelling. The two cases are mutually exclusive and neither cascades, so one emit reaches each handler exactly once. The alias registry, the per-bus alias table, the `on()` / `remove()` bookkeeping, and the blanket flag are all gone: which handlers exist is not something the bridge needs to know. Only the two pure helpers are imported from ovos-spec-tools, so the existing >=1.6.0a1 floor stays sufficient.
spec-tools#92 (registry removal) merged and released as 1.6.0a2; pin to the simplified intent_topics surface. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Message.forward()/reply() deep-copy the whole context, so the twin marker in message.context rode onto every descendant frame. A handler that forwarded a received twin's context to emit an unrelated suffixed intent branded that frame a twin, and _bridge_intent_topics skipped its canonical spelling -- silent loss for the old-emitter -> new-core population the rule serves. emit now pops the marker into a local decision before local dispatch, so descendants start clean, and _bridge_intent_topics trusts that decision instead of reading context. RULE 1's twin still carries the marker on the wire (its serialized form goes on the "message" firehose), while local handlers receive an unmarked copy -- mirroring the real client, whose local delivery happens on the receive side after the pop. Regression tests build the follow-up via forward()/reply() off a received twin and assert the unrelated canonical topic is still modernized; wire-survival keeps the second-receiver skip property. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ovos_utils/fakebus.py (1)
119-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: avoid constructing the local twin message twice.
message.forward(legacy, message.data)runs twice (Line 120 forwire_twin, Line 123 for the local dispatch). Line 123 duplicates the same object construction Line 120 already did. Not a bug, sinceis_twinsemantics stay correct, but a singleforward()call plus a context copy would reduce duplicated work.🤖 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 `@ovos_utils/fakebus.py` around lines 119 - 123, Update the message forwarding logic around wire_twin to call message.forward(legacy, message.data) only once, then reuse the resulting message for local dispatch while preserving the separate context mutation and existing is_twin semantics.
🤖 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_utils/fakebus.py`:
- Around line 217-219: The intent-topic bridge can propagate exceptions from
both synchronous and asynchronous emit paths; wrap the _bridge_intent_topics
call in FakeBus.emit at ovos_utils/fakebus.py:217-219 and AsyncFakeBus.emit at
ovos_utils/fakebus.py:577-579 with try/except and LOG.exception, matching the
existing counterpart-topic isolation pattern while preserving successful local
handler execution.
---
Nitpick comments:
In `@ovos_utils/fakebus.py`:
- Around line 119-123: Update the message forwarding logic around wire_twin to
call message.forward(legacy, message.data) only once, then reuse the resulting
message for local dispatch while preserving the separate context mutation and
existing is_twin semantics.
🪄 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: 6dc76a56-f66c-4bf9-b96a-e0c013ff4cdd
⛔ Files ignored due to path filters (2)
test/unittests/log_test/configured.logis excluded by!**/*.logtest/unittests/log_test/rotate.logis excluded by!**/*.log
📒 Files selected for processing (4)
ovos_utils/fakebus.pypyproject.tomltest/unittests/log_test/rotate.log.1test/unittests/test_fakebus_intent_legacy_reemit.py
_bridge_intent_topics() ran unguarded in both FakeBus.emit and AsyncFakeBus.emit, unlike the counterpart-topics loop right above it, which isolates per-topic errors with try/except + LOG.exception. A raising bridge helper would propagate out of emit() and take down local handler dispatch with it. Wrap both call sites in the same guard. Addresses CodeRabbit review on ovos-utils#411.
1 participant
Why
OVOS-MSG-1 §2.1.1 assembles the per-intent dispatch topic at runtime as
<skill_id>:<intent_name>. Oldovos-workshopreleases built it from the padatious resource filename, so a skill withfood.order.intentregistered and listened on<skill_id>:food.order.intent. Current workshop is spec-pure and registers<skill_id>:food.order.ovos-bus-client#271makes the real client bridge the two spellings whenemit_legacyis on. This PR gives the test doubles the same behavior. Without it every harness built onFakeBus— ovoscope, the skill test suites, the satellite fakes — hides the compat path, and a skill that passes its tests still misses the dispatch on a real bus.The version skew this covers is unchanged from the outside:
X:Y.intent)X:Y)X:Y.intent)X:Y)Design: two stateless rules
Per the maintainer-approved simplification, the compat layer is two
ifblocks and nothing else:RULE 1. A canonical intent dispatch also fires its
legacy_intent_topictwin, carrying an_intent_compat_twin: Truecontext marker.RULE 2. A suffixed intent dispatch that is not already such a twin also fires its
canonical_intent_topicform.The real client splits these over a wire send and a wire receive; a fake bus is one process, so both land in
emit(), right after the existingcounterpart_topics()dispatch. The two cases are mutually exclusive and neither cascades, so one emit reaches each handler exactly once — provable by inspection, with no bookkeeping to audit.Kill-switch: delete the two
ifblocks. Until then the bridge rides the existingemit_legacyflag, the same one the namespace bridge uses. There is no second flag.What the previous revision had, and why it is gone
IntentAliasRegistry, the per-bus alias table, theon()/once()/remove()/remove_all_listeners()bookkeeping that maintained it, and theintent_reemit_blanketflag (blanket is the behavior now) are all removed. The bridge does not need to know which handlers exist: a twin nobody listens to is a few ignored bytes._LegacyIntentBridgestays as the mixin shared byFakeBusandAsyncFakeBus, so the two doubles cannot drift, but it now holds one stateless method.Dependency order
IntentAliasRegistryandlegacy_reemit_targets. No follow-up floor bump is needed here: after this rewritefakebus.pyimports onlycanonical_intent_topic,legacy_intent_topic, andis_intent_topic, all present in 1.6.0a1.Non-normative
No specification mandates the suffixed topic or the twin. This is transitional tooling scoped to the migration period. New code must produce and consume canonical topics only.
Tests
test/unittests/test_fakebus_intent_legacy_reemit.py, 15 tests (was 26): rule 1 (twin fired, marked, payload and context kept, canonical handler still once, a handler on both spellings hears both frames once each, nothing when compat is off); rule 2 (canonical handler hears an old-style dispatch, suffixed handler still gets the original, a marked twin is not modernized again, no cascade, nothing when compat is off); non-intent topics untouched;AsyncFakeBusparity on both rules and the off switch.Full suite: 951 passed, 1 skipped, 1 pre-existing
test_events.py::TestEventSchedulerInterface::test_00_initfailure also present ondev.Implemented by Claude (opus), orchestrated by Claude Fable.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests