fix: do not silently disable fallback skills that predate can_answer - #515
Conversation
can_answer is an opt-in optimization: it lets a skill decline before ovos-core pays for a full fallback request. The base implementation raises NotImplementedError, and _handle_fallback_ack called it unguarded, so the ping handler threw and no pong was ever emitted. From ovos-core's side the skill looked unreachable. Every fallback skill that never overrode can_answer was therefore dead -- wolfie, wikipedia and icanhazdadjokes among the official ones. 'tell me a joke' and 'what is the weather' returned nothing at all on a live install. Treat NotImplementedError as 'has not opted in, so ask it', which is the behaviour those skills were written against. An explicit True or False from a skill that did override can_answer is still respected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe fallback handler now treats an unimplemented ChangesFallback compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 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 |
The bots have spoken! Here's the summary of their findings. 🗣️I've aggregated the results of the automated checks for this PR below. 🔍 LintThe latest check cycle has concluded. 🔄 ❌ ruff: issues found — see job log 🔒 Security (pip-audit)I've checked for any hardcoded credentials. 🔑 ✅ No known vulnerabilities found (74 packages scanned). ⚖️ License CheckEnsuring our legal documentation is accessible. 📖 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔨 Build TestsChecking if the code is properly tempered. ⚔️ ✅ All versions pass
Powered by OVOS scripts and a bit of magic. ✨ |
![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
🤖 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_workshop/skills/fallback.py`:
- Around line 114-122: Restrict the NotImplementedError fallback in the fallback
handling around self.can_answer to cases where the bound method resolves to
FallbackSkill.can_answer; allow exceptions from overridden implementations to
retain their normal failure behavior. Add a regression test covering a subclass
override of can_answer that raises NotImplementedError.
🪄 Autofix
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: 8baf9372-c72c-4be8-9858-704e242a7202
📒 Files selected for processing (2)
ovos_workshop/skills/fallback.pytest/unittests/skills/test_fallback_skill.py
| try: | ||
| can_handle = self.can_answer(message) | ||
| except NotImplementedError: | ||
| # can_answer is an opt-in optimization: it lets a skill decline | ||
| # before ovos-core pays for a full fallback request. A skill that | ||
| # predates it simply has not opted in, so it must still be asked. | ||
| # Treating the base implementation as a refusal silently disables | ||
| # every such skill. | ||
| can_handle = True |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Only apply the affirmative fallback to the base can_answer implementation.
Lines 114-122 also catch NotImplementedError from an overridden can_answer. If an opted-in skill raises that exception from its own unsupported path, this handler emits can_handle=True and causes an unnecessary fallback request that cannot succeed.
Check whether self.can_answer resolves to FallbackSkill.can_answer before setting can_handle=True. Otherwise, call the override and let its exception retain its normal failure behavior. Add a regression test for an override that raises NotImplementedError.
Proposed fix
- try:
- can_handle = self.can_answer(message)
- except NotImplementedError:
- # can_answer is an opt-in optimization: it lets a skill decline
- # before ovos-core pays for a full fallback request. A skill that
- # predates it simply has not opted in, so it must still be asked.
- # Treating the base implementation as a refusal silently disables
- # every such skill.
+ can_answer = self.can_answer
+ if getattr(can_answer, "__func__", None) is FallbackSkill.can_answer:
can_handle = True
+ else:
+ can_handle = can_answer(message)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| can_handle = self.can_answer(message) | |
| except NotImplementedError: | |
| # can_answer is an opt-in optimization: it lets a skill decline | |
| # before ovos-core pays for a full fallback request. A skill that | |
| # predates it simply has not opted in, so it must still be asked. | |
| # Treating the base implementation as a refusal silently disables | |
| # every such skill. | |
| can_handle = True | |
| can_answer = self.can_answer | |
| if getattr(can_answer, "__func__", None) is FallbackSkill.can_answer: | |
| can_handle = True | |
| else: | |
| can_handle = can_answer(message) |
🤖 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_workshop/skills/fallback.py` around lines 114 - 122, Restrict the
NotImplementedError fallback in the fallback handling around self.can_answer to
cases where the bound method resolves to FallbackSkill.can_answer; allow
exceptions from overridden implementations to retain their normal failure
behavior. Add a regression test covering a subclass override of can_answer that
raises NotImplementedError.
1 participant
Found on a live install:
tell me a jokeandwhat is the weatherreturned nothing at all. The utterance reached ovos-core and routed to fallback correctly — then died there.The bug
can_answeris an opt-in optimization: it lets a skill decline before ovos-core pays for a full fallback request. The base implementation raisesNotImplementedError, and_handle_fallback_ackcalled it unguarded:So the ping handler throws, no pong is ever emitted, and from ovos-core's side the skill looks unreachable. Every fallback skill that never overrode
can_answeris dead. Among the official skills on a stock install:can_answerThe only symptom is silence. The traceback (
NotImplementedErroratfallback.py:98) is logged per-skill per-utterance and is easy to read as noise.The change
Treat
NotImplementedErroras "has not opted in, so ask it" — the behaviour those skills were written against. An explicitTrueorFalsefrom a skill that did overridecan_answeris still respected, so the optimization keeps working for skills that use it.Verified
4 new tests in
test/unittests/skills/test_fallback_skill.py, mutation-checked: the legacy-skill test fails against current dev and passes with the fix. The two control tests (explicit accept, explicit decline) pass either way, which is their job.Suite: 544 passed, 2 skipped. Two failures are pre-existing and order-dependent — unpatched dev fails
test_skill_reloadandtest_skill_load_blacklistedin full-suite order, so this change makes that strictly better, not worse.test_ask_yesnoalso fails on unpatched dev in isolation.test_ask_e2e.pyneedsovoscope, absent from my venv.Reproduced against
ovos-workshop9.3.3a1 (current latest prerelease) withovos-core2.6.0a1.Summary by CodeRabbit
Bug Fixes
Tests