Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
I've completed my sweep! Here's the situation. 🧹I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthHow's the repo's pulse? Let's take a look. 💓 ✅ All required files present. Latest Version: ✅ 🔍 LintEvaluating the overall impact of your changes. 📈 ❌ ruff: issues found — see job log 📡 Channel Compat — stableHere's the latest update from the automation 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-stable.txt 📡 Channel Compat — testingThe results have been compiled and are ready for review. 📑 🚧 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 ⚖️ License CheckEnsuring our EULA (if any) is still valid. 📑 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔒 Security (pip-audit)Security report: No threats detected in the area. ✅ ✅ No known vulnerabilities found (72 packages scanned). 🔨 Build TestsThe build bots have finished their assembly. 🤖
❌ 3.10: Install OK, tests failed May your merges be conflict-free! 🕊️ |
70aaf29 to
fc03a34
Compare
… on set_context (OVOS-CONTEXT-1 §5.0, §1.2)
set_cross_skill_context/remove_cross_skill_context emitted a bare bus
message (mycroft.skill.set_cross_context/remove_cross_context) as the
only mutation - every receiving skill's handle_set_cross_context then
wrote the entry into its OWN private adapt context, which is neither
CONTEXT-1-conformant (§5.0 forbids a mutation topic distinct from the
session it modifies) nor actually shared (§3's shared scope is a bare
key, visible to every intent through the one session all pipeline
stages read - no fan-out required). Both methods now write the shared
entry directly into the session bound to the current dispatch message
(same session.set_intent_context path set_context already uses for
private entries) and keep the legacy bus broadcast only as a
deprecated compat mirror for pre-spec orchestrators, tagged with a
computed removal version (VERSION_MAJOR + 1, the pattern already used
by common_play.py's _OCP_VOC_MATCH_REMOVAL_VERSION) instead of a
hand-written placeholder.
set_context (skill API) and the underlying _AdaptIntentApi.set_context
computed expires_at from config and never exposed turns_remaining, so
the §1.2 flagship one-turn confirmation gate
({"value": ..., "turns_remaining": 1}) was unreachable from the skill
API even though Session.set_intent_context already supports it.
set_context/set_cross_skill_context now take optional
turns_remaining/expires_at kwargs, defaulting to None (unchanged
behaviour).
gh search code --owner OpenVoiceOS "set_cross_skill_context" found no
skill callers org-wide, only docs/spec references and the method's
own definition - fixing the implementation in place changes no
caller's public API.
Three new OVOSSkill instances created by the new unit tests (as locals
that go out of scope) explicitly call default_shutdown() before
returning, so the idempotency guard is set before GC ever drops the
last reference - the __del__ that eventually fires is a no-op instead
of an unbounded-delay call into whatever @patch on
OVOSSkill.default_shutdown another test has active at the time.
test_abstract_app.py's test_default_shutdown was exposed to exactly
that: an OVOSSkill instance from an EARLIER, unrelated test can sit as
cyclic garbage (skill <-> bus event-handler reference cycle, only
collectable by the generational GC, not refcounting) until whichever
later test happens to trigger a GC pass; adding this PR's tests
shifted the allocation count enough that the sweep started landing
inside test_default_shutdown's single-method @patch window, inflating
its call count. TestApp.setUp() now flushes with gc.collect() before
every test in that class, ahead of any patch, so that class no longer
depends on incidental cross-test GC timing.
test/end2end/test_context1_cross_skill_e2e.py boots a real MiniCroft
(ovoscope) with two skills: context1_setter_test_skill calls the real
set_cross_skill_context while handling one template intent,
context1_reader_test_skill registers a different template intent
gated by requires_context on the same bare shared key. The test drops
the legacy mycroft.skill.set_cross_context broadcast before it reaches
the bus for its whole duration, so the reader's gate can only open via
the session write under test, and reads the entry back from the
terminal ovos.utterance.handled message's own session rather than a
private local reference. Adapt is excluded from this run's pipeline in
favor of padacioso: an unrelated, pre-existing ovos-core bug
(IntentManifest.get_slot_names treats the INTENT-4 §5.2
required/optional descriptor dicts as bare slot names) crashes the §7
context-supplied-slot step for any adapt intent combining keyword
requirements with requires_context, independently of this PR.
Fail-before (source revert to the pre-PR baseline, keeping the tests):
the three unit tests fail - two with TypeError: unexpected keyword
argument 'turns_remaining', one with AssertionError on the missing
shared session entry; the end2end test fails with AssertionError:
shared 'person' entry never landed in session.intent_context (the
legacy broadcast alone, which still fires, does not satisfy the gate).
Restoring the fix makes all four pass. Full suite in default order,
run three times: 703 passed each time, 0 regressed. End2end suite: 14
passed (including the new test), 0 regressed.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
fc03a34 to
16015de
Compare
1 participant
Governing clauses (OVOS-CONTEXT-1,
architecturerepo,intent-context.md):set_cross_skill_context/remove_cross_skill_contextinovos_workshop/skills/ovos.pyemittedmycroft.skill.set_cross_context/remove_cross_contextas their only mutation. Every receiving skill's ownhandle_set_cross_contextlistener then calledself.set_context(...), writing the entry into that skill's private adapt context - a bus topic mutating another component's context, which §5.0 rules out, and not even the shared scope §3 defines. Under the CONTEXT-1 session model this fan-out is also unnecessary: a shared entry is a bare key insession.intent_context, already visible to every intent that reads the same session. Both methods now write the shared entry directly into the session bound to the current dispatch message (the sameSession.set_intent_contextpath the privateset_contextalready uses), keeping the bus broadcast only as a deprecated compat mirror for orchestrators that predate the spec - tagged with a computed removal version (VERSION_MAJOR + 1, the same patterncommon_play.py's_OCP_VOC_MATCH_REMOVAL_VERSIONalready uses) instead of a hand-written placeholder.gh search code --owner OpenVoiceOS "set_cross_skill_context"turned up no skill callers org-wide - only the technical-manual docs, the message-spec table, and the method's own definition/tests in this repo. Fixing the implementation in place changes no caller's public API.Separately,
set_context(and the_AdaptIntentApi.set_contextit delegates to) computedexpires_atfromcontext.timeoutconfig and never exposedturns_remaining, even thoughSession.set_intent_contextalready supports it. That made §1.2's flagship one-turn confirmation gate unreachable from the skill API.set_contextandset_cross_skill_contextnow take optionalturns_remaining/expires_atkeyword arguments; both default toNone, so existing callers are unaffected.test/end2end/test_context1_cross_skill_e2e.pyboots a realMiniCroft(ovoscope) with two skills:context1_setter_test_skillcalls the realset_cross_skill_contextwhile handling one template intent,context1_reader_test_skillregisters a different template intent gated byrequires_contexton the same bare shared key. The test drops the legacymycroft.skill.set_cross_contextbroadcast before it reaches the bus for its whole duration, so the reader's gate can only open via the session write under test, and reads the entry back from the terminalovos.utterance.handledmessage's own session rather than a private local reference. Adapt is excluded from this run's pipeline in favor of padacioso: an unrelated, pre-existingovos-corebug (IntentManifest.get_slot_namestreats the INTENT-4 §5.2required/optionaldescriptor dicts as bare slot names) crashes the §7 context-supplied-slot step for any adapt intent combining keyword requirements withrequires_context, independently of this PR.Also fixed: three new
OVOSSkillinstances in the unit tests now calldefault_shutdown()before going out of scope, andTestApp.setUp()intest_abstract_app.pyflushes withgc.collect()before every test - both needed becausetest_default_shutdown's single-method@patchwindow was picking up an unrelated, earlier test's cyclic garbage once this PR's new tests shifted the allocation count enough to move a GC sweep into that window.Fail-before (source reverted to the pre-PR baseline, keeping the tests): the three unit tests fail - two with
TypeError: unexpected keyword argument 'turns_remaining', one with anAssertionErroron the missing shared session entry; the end2end test fails withAssertionError: shared 'person' entry never landed in session.intent_context(the legacy broadcast alone, which still fires, does not satisfy the gate). Restoring the fix makes all four pass. Full unit suite: 703 passed, 0 regressed (run three times in default order). End2end suite: 14 passed, 0 regressed.