Implement FALLBACK-1 skill-addressed polling - #808
goldyfruit wants to merge 7 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesSession-scoped fallback polling
Pipeline blacklist normalization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FallbackService
participant SkillFallbackHandler
participant Skill
FallbackService->>FallbackService: acquire session polling lock
FallbackService->>SkillFallbackHandler: register skill-specific pong listener
FallbackService->>Skill: emit skill-specific fallback ping
Skill-->>SkillFallbackHandler: return session and skill response
SkillFallbackHandler-->>FallbackService: validate and collect response
FallbackService->>SkillFallbackHandler: remove listener
FallbackService->>FallbackService: release session polling lock
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
5e3af77 to
43e0e88
Compare
|
Reviewed the fallback-pong scoping fix. The concurrency correctness is sound: scoping pongs by a per-query The red CI is not your change. Unit tests pass on 3.10–3.14 and the fallback tests pass in coverage; the 11 reds are stale (2026-07-07): coverage fails only at a Two notes: (1) this pairs with ovos-workshop#465 (skill-side echo) — they're mutually back-compatible in either merge order; (2) spec-direction only, non-blocking: FALLBACK-1 §6.1 prescribes per-skill dotted topics ( Review written by Claude Opus 4.8 (claude-opus-4-8) without human oversight. |
|
Correction to my earlier review — this should be rejected as-is on spec-compliance grounds. The underlying concurrency bug (#807) is real and worth fixing, but The per-call event + registry-snapshot/locking parts of this PR are good and worth keeping; the request-id scoping should be replaced by dotted addressing. Recommend reworking toward §6.1 (or closing in favor of a spec-compliant PR). Not the version-guard/wire-tolerance pattern here — this is a case where the spec dictates the topic scheme. Review written by Claude Opus 4.8 (claude-opus-4-8) without human oversight. |
43e0e88 to
173f567
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ovos_core/intent_services/fallback_service.py (1)
54-80: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
_lifecycle_handlerswith the registry lock.
handle_register_fallbackandhandle_deregister_fallbackrun on bus dispatch threads._wire_lifecycleand_unwire_lifecyclemutate_lifecycle_handlerswithout synchronization. Two concurrent registers for the sameskill_idcan both pass the membership check and attach duplicate listeners, and only the last pair is stored, so the first pair leaks. A register that interleaves with a deregister can also leave listeners attached after deregistration._registered_fallbacks_lockis anRLock, so it can wrap both methods.🔒 Proposed fix
def _wire_lifecycle(self, skill_id: str) -> None: """Translate lifecycle done-signal for a fallback skill.""" - if skill_id in self._lifecycle_handlers: - return + with self._registered_fallbacks_lock: + if skill_id in self._lifecycle_handlers: + return + self._lifecycle_handlers[skill_id] = (_on_start, _on_response)Move the handler definitions above the lock block and register the bus listeners inside it, then apply the same lock to the pop in
_unwire_lifecycle:def _unwire_lifecycle(self, skill_id: str) -> None: - handlers = self._lifecycle_handlers.pop(skill_id, None) + with self._registered_fallbacks_lock: + handlers = self._lifecycle_handlers.pop(skill_id, None) if not handlers: return🤖 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_core/intent_services/fallback_service.py` around lines 54 - 80, Guard lifecycle listener registration and removal with the existing _registered_fallbacks_lock. In _wire_lifecycle, define the callbacks before acquiring the lock, then perform the membership check, bus.on calls, and _lifecycle_handlers assignment atomically inside the RLock; in _unwire_lifecycle, protect the pop and listener removal with the same lock so concurrent registration and deregistration cannot leak or duplicate handlers.
🧹 Nitpick comments (2)
ovos_core/intent_services/fallback_service.py (2)
289-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the unused loop variable.
Ruff reports B007.
priois not used in the loop body.♻️ Proposed change
- for skill_id, prio in sorted_handlers: + for skill_id, _prio in sorted_handlers:🤖 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_core/intent_services/fallback_service.py` at line 289, In the loop over sorted_handlers, rename the unused prio variable to the conventional underscore placeholder while preserving skill_id iteration and the loop body unchanged.Source: Linters/SAST tools
218-220: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
message.forwardfor the outbound fallback ping.
message.replyswapscontext["source"]andcontext["destination"], so the ping is addressed to the utterance source. Similar skill-query pings useforward, andtest/end2end/test_fallback.pykeepsovos.skills.fallback.pingon the original source. Useforwardso skills receive a ping with the expected forward context.♻️ Proposed change
for skill_id in pool: - self.bus.emit(message.reply( - f"{skill_id}.fallback.ping", query_data)) + self.bus.emit(message.forward( + f"{skill_id}.fallback.ping", query_data))🤖 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_core/intent_services/fallback_service.py` around lines 218 - 220, Update the fallback ping loop in the relevant fallback service method to emit each “{skill_id}.fallback.ping” message via message.forward instead of message.reply, preserving the original source and destination context expected by fallback skills.
🤖 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/fallback_service.py`:
- Around line 188-220: Update the fallback candidate query in the surrounding
handler registration and emission flow to use the shared FALLBACK-1 topics,
ovos.skills.fallback.pong for listener registration and
ovos.skills.fallback.ping for emitted requests, instead of constructing topics
from each skill_id. Keep the existing per-skill response validation and
filtering behavior unchanged.
In `@test/unittests/test_fallback_service.py`:
- Around line 354-410: Update
test_concurrent_sessions_do_not_consume_each_others_pongs to configure an
explicit fallback query timeout large enough to cover its polling and delayed
pong delivery, rather than relying on _make_service() defaults. Correct the
docstring to state that the test verifies pong filtering by session ID, not
serialization via _acquire_fallback_session_lock.
---
Outside diff comments:
In `@ovos_core/intent_services/fallback_service.py`:
- Around line 54-80: Guard lifecycle listener registration and removal with the
existing _registered_fallbacks_lock. In _wire_lifecycle, define the callbacks
before acquiring the lock, then perform the membership check, bus.on calls, and
_lifecycle_handlers assignment atomically inside the RLock; in
_unwire_lifecycle, protect the pop and listener removal with the same lock so
concurrent registration and deregistration cannot leak or duplicate handlers.
---
Nitpick comments:
In `@ovos_core/intent_services/fallback_service.py`:
- Line 289: In the loop over sorted_handlers, rename the unused prio variable to
the conventional underscore placeholder while preserving skill_id iteration and
the loop body unchanged.
- Around line 218-220: Update the fallback ping loop in the relevant fallback
service method to emit each “{skill_id}.fallback.ping” message via
message.forward instead of message.reply, preserving the original source and
destination context expected by fallback skills.
🪄 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: 0377377e-d27c-4d0a-8d42-0716ad186a37
📒 Files selected for processing (2)
ovos_core/intent_services/fallback_service.pytest/unittests/test_fallback_service.py
goldyfruit
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/end2end/test_fallback.py (1)
110-115: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the fallback wire contract explicitly.
End2EndTestchecks only keys listed in the expectedMessageand ignores extra keys. Therefore, removingrangefrom the expected payload does not prove that the emitted ping or request omits it. Omittingcontext["session"]also does not detect lost session propagation. (pypi.org)Add
sessionto the expected ping, pong, and request contexts. Add an exact-payload or explicitassertNotIn("range", actual_message.data)check for the skill-addressed ping and fallback request.Proposed session assertions
Message(f"{self.skill_id}.fallback.ping", {"utterances": ["hello world"], - "lang": session.lang}), + "lang": session.lang}, + {"session": session.serialize()}), Message(f"{self.skill_id}.fallback.pong", {"skill_id": self.skill_id, "can_handle": True}, - {"source": "A", "destination": "B"}), + {"session": session.serialize(), + "source": "A", "destination": "B"}), Message(f"ovos.skills.fallback.{self.skill_id}.request", {"utterances": ["hello world"], "lang": session.lang, - "skill_id": self.skill_id}), + "skill_id": self.skill_id}, + {"session": session.serialize()}),Also applies to: 126-128
🤖 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_fallback.py` around lines 110 - 115, Update the expected messages in End2EndTest to include session in the ping, pong, and request contexts, preserving the emitted session value. Strengthen the fallback wire-contract assertions by checking the skill-addressed ping and fallback request payloads exactly or explicitly asserting that range is absent from each actual message’s data; apply the same changes to the additional message cases referenced by the comment.
🤖 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.
Nitpick comments:
In `@test/end2end/test_fallback.py`:
- Around line 110-115: Update the expected messages in End2EndTest to include
session in the ping, pong, and request contexts, preserving the emitted session
value. Strengthen the fallback wire-contract assertions by checking the
skill-addressed ping and fallback request payloads exactly or explicitly
asserting that range is absent from each actual message’s data; apply the same
changes to the additional message cases referenced by the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3cb7562f-0e11-440a-a052-79ade95d9072
📒 Files selected for processing (3)
ovos_core/intent_services/fallback_service.pytest/end2end/test_fallback.pytest/unittests/test_fallback_service.py
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unittests/test_fallback_service.py
- ovos_core/intent_services/fallback_service.py
… for the migration window The skill-addressed fallback ping/pong machinery landed with no compatibility window: any released ovos-workshop (pre-OpenVoiceOS#465, broadcast-only) paired with this branch never gets its fallback ping answered, since the broadcast ping/pong collector was dropped outright. FALLBACK-1 §6.1 makes the addressed topics normative but explicitly sanctions the broadcast poll as an observably-equivalent optimisation, so restore it for one deprecation window (kill-switch OpenVoiceOS#837 conventions): - emit the general `ovos.skills.fallback.ping` broadcast once per poll round alongside the addressed pings, and keep a `ovos.skills.fallback.pong` collector with the same session filter as the addressed collectors. - dedup pongs by skill_id: a skill running fixed ovos-workshop (OpenVoiceOS#465) answers BOTH ping families during the window, and must only count once (first answer wins). - delete the end2end `_wire_skill_addressed_probe` fake and let the real (released) ovos-workshop installed by the test run answer the ping honestly; expected_messages updated to reflect the addressed ping being emitted-but-unanswered against a pre-OpenVoiceOS#465 workshop. - fix the CodeRabbit-flagged flaky session-lock unit test's docstring (it exercises the session-id filter, not the lock) and add a real same-session lock serialization test. - revert the unrelated pipeline-blacklist normalization change to intent_services/service.py (and its test) that had leaked into this branch; it is being split into its own PR. Executed matrix (probe-free harness, real ovoscope + real workshop): fixed-core+fixed-ws, fixed-core+ws-dev, core-dev+fixed-ws, and fixed-core+PyPI ovos-workshop==9.3.9a1 are all GREEN (ping answered, dispatched exactly once, skill spoke exactly once). Narrows OpenVoiceOS#807 (same-session stale-pong residue remains; needs a round nonce -- pre-existing on dev). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Closed by owner ruling (2026-08-13): this pair predates the ratified FALLBACK-1 broadcast-contest shape. Fallback will be reworked on the broadcast pattern established by ovos-core#863 + ovos-workshop#534 (one broadcast question per round, parallel answers, explicit declines, early close) once that pair lands — one migration instead of two. Nothing is broken by the closure: the pair was never merged. |
2 participants
narrows #807 (same-session stale-pong residue remains; needs a round nonce -- pre-existing on dev) by resolving the underlying shared-pong race with the architecture defined by FALLBACK-1.
Architecture
<skill_id>.fallback.pingtopic and collect the corresponding<skill_id>.fallback.pong.message.replyas required by FALLBACK-1 §6.1, then apply priority ordering, session exclusions, fallback mode, and timeout behavior to a locked registry snapshot.This intentionally does not add a generic agent traffic hook, a request-ID compatibility layer, or a shared
ovos.skills.fallback.pongcollector. The coordinated skill-side change is OpenVoiceOS/ovos-workshop#465; both repositories use the same FALLBACK-1 dotted topics.Validation
Summary by CodeRabbit
Rework after adversarial audit (2026-08-12)
The flag-day defect. This PR and its workshop counterpart (#465) did a
flag-day switch to skill-addressed
<skill_id>.fallback.ping/.pongwith nocompatibility window: this branch dropped the broadcast
ovos.skills.fallback.pingpoll and theovos.skills.fallback.pongcollector outright. Executed result: this PR + ANY released ovos-workshop
(including the current 9.3.9a1 dev/floor pin) = fallback dead (the released
workshop only answers the broadcast ping, which this branch stopped
sending). Symmetrically, #465 + any released ovos-core = fallback dead. The
original e2e test only passed because
_wire_skill_addressed_probeintest/end2end/test_fallback.pyfaked the workshop-side pong; it failedagainst the real #465 branch (double pong, 14 messages vs 13 expected).
FALLBACK-1 §6.1 makes the skill-addressed topics normative but explicitly
sanctions the broadcast poll as an observably-equivalent optimisation, so a
dual window is conformant.
The fix. One deprecation window (kill-switch #837 conventions):
ovos.skills.fallback.pingbroadcast once per pollround alongside the addressed pings, and keep a
ovos.skills.fallback.pongcollector with the same session filter asthe addressed collectors.
skill_id: a skill running fixed ovos-workshop (fr-fr/intents #465)answers BOTH ping families during the window and must only count once
(first answer wins). New unit tests cover both orderings.
_wire_skill_addressed_probeand its uses fromtest/end2end/test_fallback.py-- the real (released) ovos-workshopinstalled by the test run now has to answer honestly. Updated
expected_messagesto reflect the addressed ping beingemitted-but-unanswered against a pre-fr-fr/intents #465 workshop, with the broadcast
ping/pong carrying the actual round-trip.
docstring (it exercises the session-id filter, not the lock, kept an
explicit generous
fallback_query_timeout) and added a realsame-session lock-serialization test that exercises
_acquire_fallback_session_lockwith two concurrent same-session polls.ovos_core/intent_services/service.pythat had leaked into thisbranch -- now fix: normalize pipeline ids when matching session blacklists #854 (draft).
Executed matrix (probe-free harness: real ovoscope + real
ovos-skill-fallback-unknown, no fake ping/pong):
All four cells green. Full
ovos-coreunit suite green (329 passed, 9subtests), full end2end suite green (40 passed, 72 subtests) -- including
test/end2end/test_fallback.pyrunning honestly against the real,unmodified, released ovos-workshop with no probe installed.
Verified claims: model re-check only (this session), not human-reviewed
-- matrix cells, red-before-fix on the dedup test (revert-code-keep-test
technique, no
git stash), and the full unit/e2e suites were all actuallyexecuted in this session, not asserted from memory.
Fixes #807 -> narrows #807 (same-session stale-pong residue remains;
needs a round nonce -- pre-existing on dev, out of scope here).