feat: emit ovos.skill.loaded with derived capabilities - #623
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change derives capabilities from skill instances and emits them in ChangesSkill capability announcements
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Skills now announce their declared capabilities when loaded and after readiness recovery, while retaining the existing load event unchanged. The covered behavior has no identified merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant PluginSkillLoader
participant LoaderBus
participant SkillInstanceBus
PluginSkillLoader->>LoaderBus: emit mycroft.skills.loaded
PluginSkillLoader->>SkillInstanceBus: emit ovos.skill.loaded with skill_id and capabilities
PluginSkillLoader->>SkillInstanceBus: on_ready_status emits ovos.skill.loaded with capabilities
🚥 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 |
Your PR has been successfully processed by the OVOS bot. 📥I've aggregated the results of the automated checks for this PR below. 🔍 LintThe automated results are now available for your perusal. 📂 ❌ ruff: issues found — see job log ⚖️ License CheckScanning for any non-commercial-only restrictions. 💰 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔒 Security (pip-audit)Scanning for any potential denial-of-service vectors. 🚫 ✅ No known vulnerabilities found (72 packages scanned). 📡 Channel Compat — stableChecking if we've met all our check criteria. ✅ 🚧 Channel unresolvable with this checkout — the repo's dependency floors exceed what the channel pins (fleet finding; see the install log). Constraints: https://raw.githubusercontent.com/OpenVoiceOS/OpenVoiceOS/main/constraints-stable.txt 📋 Repo HealthScanning for any signs of 'merge conflict' stress. 😫 ✅ All required files present. Latest Version: ✅ 📡 Channel Compat — testingThe results are fresh out of the pipeline. 🏗️ 🚧 Channel unresolvable with this checkout — the repo's dependency floors exceed what the channel pins (fleet finding; see the install log). Constraints: https://raw.githubusercontent.com/OpenVoiceOS/OpenVoiceOS/main/constraints-testing.txt 🔨 Build TestsI've checked the welds on your new features. 👨🏭 ✅ All versions pass
Crafting a better voice assistant, one commit at a time 🎙️ |
OVOS-INTENT-4 §8.6 fixes `ovos.skill.loaded`: `{"skill_id": "...",
"capabilities": ["converse", "fallback"]}`, session-keyed via
context.session.session_id, capability vocabulary limited to
fallback/common_query/converse.
skill_launcher.py's load-status announcement kept the legacy
`mycroft.skills.loaded` payload unchanged and, beside it, now emits
`ovos.skill.loaded` through the loaded instance's own bus so
context.skill_id rides along and the bus auto-fills context.session.
OVOSSkill.on_ready_status re-emits the same announcement, covering
cold-start recovery when a skill process outlives a core restart.
Capabilities are read off the instance, never guessed: fallback via
isinstance(FallbackSkill), common_query via the _cq_handler a
@common_query-decorated method sets at bind time, converse via
isinstance(ConversationalSkill) -- the only base that wires the
CONVERSE-1 §4 surface (ovos.converse.ping, <skill_id>.converse.ping/
.request). A callable-attribute check was rejected: OVOSGameSkill and
ConversationalGameSkill define their own converse() outside
ConversationalSkill's MRO, and that surface never binds for them, so
"has a converse method" would announce a capability the skill cannot
answer on. The helper lives in ovos_workshop/skills/capabilities.py so
tests can assert its return value directly.
10 tests in test/unittests/test_ovos_skill_loaded.py assert the
capability values (not just that a message fired) for a plain skill
([]), a FallbackSkill (["fallback"]), a common-query skill
(["common_query"]), a ConversationalSkill (["converse"]), and a game
skill that defines converse() outside ConversationalSkill's MRO ([],
proving the callable-attribute check was rejected), plus the unchanged
legacy payload and the readiness re-announcement. Full suite 728
passed, no regressions.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4b6ffcb to
dc3fcff
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
2 participants
This adds the
ovos.skill.loadedannouncement from OVOS-INTENT-4 §8.6 toovos_workshop/skill_launcher.pyandovos_workshop/skills/ovos.py, without touching the legacymycroft.skills.loadedpayload. The reader's decision is whether the capability-derivation rules below match intent, since they had to be adapted from the brief that requested this PR.The load-status handler now emits
ovos.skill.loadedthrough the loaded instance's own bus, right beside the untouched legacy emission, socontext.skill_idrides along and the bus client's ownemit()fills incontext.session.OVOSSkill.on_ready_statusre-emits the same announcement, so a skill process that survives a core restart re-announces its capabilities without a full reload.Capabilities are read off the class, never guessed, in a new
ovos_workshop/skills/capabilities.py::get_skill_capabilities()helper:fallbackfromisinstance(instance, FallbackSkill);common_queryfrom the_cq_handlera@common_query-decorated method sets on the instance at bind time (there is no common-query base class in this repo toisinstanceagainst — the mechanism here is a decorator, not inheritance);conversefrom whether the instance defines aconversemethod at all (OVOSSkillitself has none, so its presence already means a mixin likeConversationalSkillor an ad hoc override supplied one — there is noOVOSSkill.converseto compare identity against, since the base class never defines it).Verified against
origin/devof this repo: themycroft.skills.loadedemission site (skill_launcher.py,_communicate_load_status), the absence ofOVOSSkill.converseand of any common-query base class (both confirmed by grep acrossovos_workshop/), and thatMessageBusClient.emitauto-populatescontext["session"]when absent (checked in the installedovos_bus_clientpackage). Not independently verified: any downstream consumer's exact expectations, since this PR only emits the message and does not add a listener.9 tests in
test/unittests/test_ovos_skill_loaded.pyassert the capability values, not just that a message fired: a plain skill gets[], aFallbackSkillgets["fallback"], a common-query skill gets["common_query"], a converse-overriding skill gets["converse"], the legacy payload is unchanged, andon_ready_statusre-announces. Confirmed red before the fix (collection fails withModuleNotFoundErrorfor the missing capabilities helper) and green after (9 passed). Full suite: 727 passed, no regressions.Summary by CodeRabbit
New Features
Bug Fixes