fix:duplicate_handled_message #262
Conversation
ovos.utterance.handled is now emitted by the fallback pipeline, the skill class should not emit it or we get duplicate events noticed in end2end tests from OpenVoiceOS/ovos-core#571
|
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request involve modifications to several classes within the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #262 +/- ##
==========================================
- Coverage 53.41% 51.38% -2.03%
==========================================
Files 37 36 -1
Lines 4362 4176 -186
==========================================
- Hits 2330 2146 -184
+ Misses 2032 2030 -2 ☔ View full report in Codecov by Sentry. |
|
marking this as a breaking change because if workshop is updated without updating core then some bus messages might be missing and we get a regression (bug where calling deactivate in converse doesnt work because skill is activated again afterwards) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
ovos_workshop/skills/auto_translatable.py (1)
Line range hint
332-348: Update method documentation to reflect current behavior.The docstring mentions that "If the fallback matched the skill is activated with the converse system", but this behavior has been removed. Please update the documentation to accurately reflect the current implementation.
ovos_workshop/skills/ovos.py (2)
Line range hint
807-811: Handle potentialNonevalue fromdig_for_message()in_on_timeoutIn the
_on_timeoutmethod,dig_for_message()may returnNone, which would causemessage.forward()to raise anAttributeError. Consider adding a check to ensuremessageis notNoneor provide a fallback message to avoid potential exceptions.Apply this diff to handle the case:
def _on_timeout(self): """_handle_converse_request timed out and was forcefully killed by ovos-core""" message = dig_for_message() + if not message: + message = Message('') self.bus.emit(message.forward( f"{self.skill_id}.converse.killed", data={"error": "timed out"}))
Line range hint
1180-1180: UseLOG.exception()to include traceback in logsIn the exception handling block, consider using
LOG.exception()instead ofLOG.error(e)to include the traceback information in the logs for easier debugging.Apply this diff to make the change:
-except Exception as e: - LOG.error(e) +except Exception: + LOG.exception("Error occurred during converse handling")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
ovos_workshop/skills/auto_translatable.py(1 hunks)ovos_workshop/skills/common_query_skill.py(1 hunks)ovos_workshop/skills/fallback.py(1 hunks)ovos_workshop/skills/ovos.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ovos_workshop/skills/fallback.py
🔇 Additional comments (4)
ovos_workshop/skills/common_query_skill.py (2)
111-112: LGTM: Enhanced event handler configuration.
The additional parameters provide better control over the event handler behavior:
handler_info: Properly identifies the handleractivation: Ensures skill activation when handling the actionis_intent: Correctly marks this as an intent handler
Line range hint 1-300: Verify alignment with PR objectives.
The changes in this file appear to be supporting modifications rather than directly addressing the stated PR objective of fixing duplicate ovos.utterance.handled event emissions. Please verify if:
- The main fix is implemented in other files not provided for review
- These changes are necessary supporting changes
- The PR description needs to be updated to reflect all changes
ovos_workshop/skills/auto_translatable.py (1)
349-349: LGTM! Simplified fallback handler return.
The change correctly simplifies the fallback handler by directly returning the result from the handler without additional processing. This aligns with the PR objective of preventing duplicate ovos.utterance.handled event emissions, as the event is now handled by the fallback pipeline instead.
ovos_workshop/skills/ovos.py (1)
1173-1173: Ensure skill activation logic remains consistent after removing _conditional_activate
By directly calling self.converse(**kwargs), the activation logic previously handled by _conditional_activate is bypassed. Verify that the skill's activation state is managed correctly during conversational interactions and that the removal of _conditional_activate does not introduce unintended side effects.
Run the following script to confirm that _conditional_activate is no longer used elsewhere and to check for any potential issues with skill activation:
✅ Verification successful
No issues with removal of _conditional_activate
The codebase analysis shows that skill activation is properly managed through the standard activation mechanisms:
-
Skills can be activated/deactivated via:
- Direct calls to
activate()/deactivate() - Intent service messages (
intent.service.skills.activated/deactivated) - Skill-specific messages (
{skill_id}.activate/deactivate)
- Direct calls to
-
The
converse()method is only called for active skills, and the activation state is tracked by the intent service, not by the individual skills.
The removal of _conditional_activate does not impact the skill's activation logic as it's handled by the proper activation mechanisms already in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of `_conditional_activate` in the codebase
# Expected result: No occurrences of `_conditional_activate` remain
rg '_conditional_activate'
Length of output: 26
Script:
#!/bin/bash
# Let's analyze the skill activation logic more comprehensively
# 1. Search for converse method implementation
ast-grep --pattern 'def converse($$$)'
# 2. Look for any activation-related methods or patterns
rg -i "activate|activation" -A 3
# 3. Check the context around the converse call
rg "converse\(" -A 3 -B 3
Length of output: 20955
…e that does nothing this was used only by the ipc display stuff that has been removed years ago and had its own concept of "active" skill
skill activation should be managed by core not skills, fully delegate this task to ovos-core
ovos.utterance.handled is now emitted by the fallback pipeline, the fallback skill class should not emit it or we get duplicate events
companion to OpenVoiceOS/ovos-core#571
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes