fix: enable_intent rebinds the handler after disable_intent - #508
fix: enable_intent rebinds the handler after disable_intent#508JarbasAl wants to merge 1 commit into
Conversation
disable_intent()/enable_intent() never actually restored a handler: - OVOSSkill.enable_intent() called register_intent_file(name, None) / register_intent(intent, None); both register_* paths skip add_event() whenever handler is falsy, so nothing was ever rebound. - IntentServiceInterface.remove_intent() was called with the skill_id-prefixed name, but registered_intents/detached_intents are keyed by the bare name (register_intent/register_template strip the prefix before storing), so the detach bookkeeping never matched and disable_intent's internal state was already wrong going in. - register_template() also never dropped a re-registered intent from detached_intents (register_intent already did), so intent_is_detached() kept reporting True forever after a padatious intent was re-enabled. Fix: OVOSSkill now remembers each intent's handler in self._intent_handlers at registration time and passes it back into register_intent_file/register_intent on enable_intent(), instead of None. remove_intent() normalizes the skill_id prefix before matching against the bare-keyed registry, and register_template() clears the matching detached_intents entry on re-registration. Reproduced on origin/dev; found during adversarial review of #500.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 back in with the latest test results. 📡I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthHealth report: The repository is thriving! 🌟 ✅ All required files present. Latest Version: ✅ 🔒 Security (pip-audit)Ensuring our encryption is top-notch. 🔐 ✅ No known vulnerabilities found (74 packages scanned). 🔍 LintI've got some results for you! 📝 ❌ ruff: issues found — see job log ⚖️ License CheckI've checked for any conflicting terms of service. 📜 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔨 Build TestsEnsuring the foundation is solid for these changes. 🏛️ ✅ All versions pass
Automating the path to a better future 🌈 |
1 participant
Root cause
disable_intent()followed byenable_intent()never actually rebound the handler. Two separate bugs combine:ovos_workshop/skills/ovos.pyenable_intent()calledregister_intent_file(intent_name, None)for padatious intents andregister_intent(intent, None)for adapt intents. Bothregister_intent_fileand_register_adapt_intentonly calladd_event(...)if handler:— passingNonesilently skips rebinding, so the intent was "enabled" bus-side but no callback was ever attached again.ovos_workshop/intents.pyIntentServiceInterface.remove_intent()was invoked bydisable_intent()with theskill_id-prefixed name, butregistered_intents/detached_intentsare keyed by the bare name (register_intent/register_templatestrip the<skill_id>:prefix before storing). The prefixed lookup never matched, so the intent was never actually moved intodetached_intents— the internal disable bookkeeping was already broken beforeenable_intenteven ran.register_template()(used byregister_intent_file) never dropped a re-registered intent fromdetached_intentson re-registration —register_intent()(adapt path) already did this. Without it,intent_is_detached()kept reportingTrueforever after a padatious.intentfile was re-enabled.Fix
OVOSSkillnow remembers each intent's handler inself._intent_handlersat registration time (register_intent_file/_register_adapt_intent), andenable_intent()passes that handler back intoregister_intent_file/register_intentinstead ofNone.IntentServiceInterface.remove_intent()normalizes away the<skill_id>:prefix before matching against the bare-keyed registry.IntentServiceInterface.register_template()now clears the matchingdetached_intentsentry on re-registration, mirroringregister_intent().Public API signatures are unchanged.
Reproduced on
origin/dev(pre-fix). Found during adversarial review of #500.Test plan
test/unittests/skills/test_base.py: implemented the previously-emptytest_enable_intent,test_disable_intent,test_handle_enable_intent,test_handle_disable_intentstubs as real disable→enable round-trip regression tests, covering both padatious (.intentfile) and adapt intents — asserting the handler actually fires again after re-enabling.origin/devcode (checked out the pre-fixovos_workshop/intents.pyandovos_workshop/skills/ovos.pyagainst the new tests): 4 failed, 1 passed.pytest test/unittests/skills/test_base.py -q→ 72 passed.pytest test/unittests/skills/test_base.py test/unittests/skills/test_ovos.py test/unittests/test_intent4_producer.py test/unittests/skills/test_intent_provider.py test/end2end/test_intent4_producer_e2e.py -q→ 129 passed.🤖 Implemented by Claude (Sonnet), orchestrated by Claude Fable.
🤖 Generated with Claude Code