From d27c0a11e5992ece5a2feee9356c09615ddb0b0c Mon Sep 17 00:00:00 2001 From: JarbasAi Date: Thu, 24 Sep 2026 15:21:24 +0100 Subject: [PATCH] fix: cross-skill context writes go to the session, validated before the write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CONTEXT-1 §5.0 forbids a context-mutation bus topic: set_cross_skill_context and remove_cross_skill_context write session.intent_context directly, and the legacy mycroft.skill.set_cross_context / remove_cross_context broadcasts are kept only as compat signals that touch adapt session.context, never session.intent_context. set_cross_skill_context wrote value into session.intent_context before emitting the legacy broadcast, so a non-string context or word landed in the shared session and only the legacy handle_set_cross_context listener later raised ValueError on it. context and word are now validated as strings, with the same wording the legacy handler used, before the session write happens. Added test_set_cross_skill_context_invalid_word_leaves_session_unchanged: set_cross_skill_context("thing", None) raises ValueError and leaves session.intent_context untouched. The test fails against the code at 1e6e6af (word=None reaches the session write) and passes after the fix. Co-Authored-By: Claude Fable 5.1 --- ovos_workshop/intents.py | 67 +++++-- ovos_workshop/skills/ovos.py | 142 ++++++++++++-- .../context1_reader_test_skill/__init__.py | 43 +++++ .../locale/en-us/height.intent | 1 + .../context1_setter_test_skill/__init__.py | 40 ++++ .../locale/en-us/remember.intent | 1 + test/end2end/test_context1_cross_skill_e2e.py | 169 +++++++++++++++++ test/unittests/skills/test_base.py | 173 ++++++++++++++++-- test/unittests/test_abstract_app.py | 12 ++ 9 files changed, 598 insertions(+), 50 deletions(-) create mode 100644 test/end2end/context1_reader_test_skill/__init__.py create mode 100644 test/end2end/context1_reader_test_skill/locale/en-us/height.intent create mode 100644 test/end2end/context1_setter_test_skill/__init__.py create mode 100644 test/end2end/context1_setter_test_skill/locale/en-us/remember.intent create mode 100644 test/end2end/test_context1_cross_skill_e2e.py diff --git a/ovos_workshop/intents.py b/ovos_workshop/intents.py index 2c5a5b23..80ea5db8 100644 --- a/ovos_workshop/intents.py +++ b/ovos_workshop/intents.py @@ -223,7 +223,9 @@ def register_adapt_intent(self, name: str, intent_parser: object): self._iface.register_intent(name, intent_parser) def set_context(self, context: str, word: str, origin: str, - original_key: Optional[str] = None): + original_key: Optional[str] = None, + turns_remaining: Optional[int] = None, + expires_at: Optional[float] = None): """Add adapt-engine context (adapt-engine only). `context` is the munged (alphanumeric_skill_id + context) legacy @@ -235,33 +237,54 @@ def set_context(self, context: str, word: str, origin: str, `session` bound to `msg` (`SessionManager.get(msg)`, ovos-spec-tools >=1.10.3a1) via `Session.set_intent_context` - `forward`/`reply` derived from `msg` stamp from that same bound object (CONTEXT-1 - §5.3), so the write rides out on whatever this call emits next. The - legacy `add_context` message below is a *different* mechanism (the - adapt-engine `session.context` field, not `intent_context`) kept for - cores that still consume it, and warns once per process via + §5.3), so the write rides out on whatever this call emits next. + + With `original_key` unset no session write happens at all. That is + the path the legacy `mycroft.skill.set_cross_context` listener takes + (`OVOSSkill.handle_set_cross_context`): a Message that announces a + context change must not cause a `session.intent_context` write in + every receiver (§5.0), so that path carries the write to the + adapt-engine `session.context` field only, through the legacy + `add_context` message below. That message warns once per process via `_legacy_warn_add_context_once` (see that helper). + + `turns_remaining` and `expires_at` are CONTEXT-1 §2 decay fields, + passed straight through to `Session.set_intent_context`. Omitted, + they keep the pre-existing behaviour: `expires_at` defaults to + `now + context.timeout` (minutes, default 2) **read from the process + making this call**, which on a satellite is not the orchestrator's + configuration, so the §5.3 orchestrator-side default decay never + applies to an entry written here; `turns_remaining` is unset (the + one-turn gate `{"value": null, "turns_remaining": 1}` is §3.2's + flag-context worked example, which §1.2 describes in prose, and it + is reachable by passing `turns_remaining=1` explicitly). """ msg = dig_for_message() or Message("") if "skill_id" not in msg.context: msg.context["skill_id"] = self.skill_id if original_key is not None: session = SessionManager.get(msg) - # OVOS-CONTEXT-1: mirror ovos-core's decay policy - # (`context.timeout`, minutes, default 2) so a skill-side write - # folds into the registry with the SAME `expires_at` a core-side - # write would carry - an omitted `expires_at` here produced - # immortal entries that stripped core's decay stamp for that - # key. One decay policy on both write paths. - context_cfg = Configuration().get('context', {}) - timeout_s = context_cfg.get('timeout', 2) * 60 - expires_at = time.time() + timeout_s if timeout_s > 0 else None + if expires_at is None: + # OVOS-CONTEXT-1: mirror ovos-core's decay policy + # (`context.timeout`, minutes, default 2) so a skill-side + # write folds into the registry with the SAME `expires_at` + # a core-side write would carry - an omitted `expires_at` + # here produced immortal entries that stripped core's decay + # stamp for that key. One decay policy on both write paths. + context_cfg = Configuration().get('context', {}) + timeout_s = context_cfg.get('timeout', 2) * 60 + expires_at = time.time() + timeout_s if timeout_s > 0 else None session.set_intent_context(original_key, word, scope="private", owner_id=self.skill_id, - expires_at=expires_at) - # `add_context` mutates the adapt-engine `session.context` field, a - # different mechanism than `intent_context` above, kept for - # orchestrators that predate OVOS-CONTEXT-1 and still consume it. + expires_at=expires_at, + turns_remaining=turns_remaining) + # `add_context` carries the write to the adapt-engine + # `session.context` field, kept for orchestrators that predate + # OVOS-CONTEXT-1 and still consume it. A modern core folds the same + # key back into `session.intent_context`, so for a modern core this + # is a write-through of the session write above, not a second + # mutation. _legacy_warn_add_context_once() data = {'context': context, 'word': word, 'origin': origin} if original_key is not None: @@ -858,14 +881,18 @@ def register_adapt_intent(self, name: str, intent_parser: object): return self._adapt.register_adapt_intent(name, intent_parser) def _set_context(self, context: str, word: str, origin: str, - original_key: Optional[str] = None): + original_key: Optional[str] = None, + turns_remaining: Optional[int] = None, + expires_at: Optional[float] = None): """Non-warning implementation shared by the deprecated public facade (`set_context`) and OVOSSkill's own supported `set_context`/ `remove_context` API (ovos_workshop/skills/ovos.py), which delegates here so the SUPPORTED base-class path does not itself trigger the facade's external-caller deprecation warning.""" return self._adapt.set_context(context, word, origin, - original_key=original_key) + original_key=original_key, + turns_remaining=turns_remaining, + expires_at=expires_at) def set_context(self, context: str, word: str, origin: str, original_key: Optional[str] = None): diff --git a/ovos_workshop/skills/ovos.py b/ovos_workshop/skills/ovos.py index 32f29194..f64c5234 100644 --- a/ovos_workshop/skills/ovos.py +++ b/ovos_workshop/skills/ovos.py @@ -88,6 +88,11 @@ from ovos_workshop.settings import PrivateSettings from ovos_workshop.skills.capabilities import get_skill_capabilities from ovos_workshop.skills.util import join_word_list, simple_trace +from ovos_workshop.version import VERSION_MAJOR + +# mycroft.skill.set_cross_context/remove_cross_context are pre-OVOS-CONTEXT-1 +# compat broadcasts; deprecated shims are removed in the next MAJOR release. +_CROSS_CONTEXT_REMOVAL_VERSION = f"{VERSION_MAJOR + 1}.0.0" def _typed_slots_map(message: Message) -> Dict[str, Any]: @@ -1800,21 +1805,51 @@ def handle_disable_intent(self, message: Message): def handle_set_cross_context(self, message: Message): """ Add global context to the intent service. + + CONTEXT-1 §5.0: "There is no context-mutation topic: no participant + emits a Message whose purpose is to announce a context change to the + orchestrator or to another component." So this listener MUST NOT + write `session.intent_context`: the shared entry is already on the + session that `set_cross_skill_context` wrote, and a second write per + receiving skill would make one logical mutation into N+1 session + pushes that race each other at handler completion (SESSION-2 §2.6). + + It therefore reaches the `original_key is None` path, which touches + only the pre-CONTEXT-1 adapt `session.context` field through the + legacy `add_context` topic, and never `Session.set_intent_context`. + A skill's own `set_context` call keeps its private session write; + only this broadcast-driven path gives it up. + @param message: `mycroft.skill.set_cross_context` Message """ context = message.data.get('context') word = message.data.get('word') origin = message.data.get('origin') - self.set_context(context, word, origin) + if not isinstance(context, str): + raise ValueError('Context should be a string') + if not isinstance(word, str): + raise ValueError('Word should be a string') + # munged exactly as `set_context` munges it, so the adapt-engine key + # is unchanged; `original_key` is left unset, which is what keeps + # this off the session. + self.intent_service._set_context( + self.alphanumeric_skill_id + context, word, origin) def handle_remove_cross_context(self, message: Message): """ Remove global context from the intent service. + + The mirror of :meth:`handle_set_cross_context`: adapt + `session.context` only, never `session.intent_context`. + @param message: `mycroft.skill.remove_cross_context` Message """ context = message.data.get('context') - self.remove_context(context) + if not isinstance(context, str): + raise ValueError('context should be a string') + self.intent_service._remove_context( + self.alphanumeric_skill_id + context) def _on_event_start(self, message: Message, handler_info: str, skill_data: dict, activation: Optional[bool] = None): @@ -3290,7 +3325,9 @@ def skill_will_match(self, utterance: str, lang: Optional[str] = None, return False return intent.get("skill_id") == self.skill_id - def set_context(self, context: str, word: str = '', origin: str = ''): + def set_context(self, context: str, word: str = '', origin: str = '', + turns_remaining: Optional[int] = None, + expires_at: Optional[float] = None): """ Add context to intent service. @@ -3298,14 +3335,29 @@ def set_context(self, context: str, word: str = '', origin: str = ''): current dispatch message (`Session.intent_context`, private scope owned by this skill) via `IntentServiceInterface`/`_AdaptIntentApi`, so the mutation rides forward on whatever Message this handler - emits next (§5.3). The legacy `add_context` bus message - a - different mechanism, the adapt-engine `session.context` field - is - also emitted, for pre-spec orchestrators only. + emits next (§5.3). The legacy `add_context` bus message is also + emitted, for pre-spec orchestrators only: it carries the write to + the adapt-engine `session.context` field. A core that predates + §5.0 reads it there; a modern core folds the same key back into + `session.intent_context`, so the topic is a write-through of the + session write above and not an independent mutation. + + `turns_remaining`/`expires_at` are the CONTEXT-1 §2 decay fields. + Passing `turns_remaining=1` is the one-turn confirmation-branch + gate; the literal `{"value": ..., "turns_remaining": 1}` is §3.2's + flag-context worked example, and §1.2 describes the same branch in + prose. Left unset, decay stays time-based only, as before this + parameter existed. Args: context: Keyword word: word connected to keyword origin: origin of context + turns_remaining: number of intent matches this entry survives, + per CONTEXT-1 §2/§4. `None` (default) means no + turn-based decay. + expires_at: absolute unix timestamp this entry decays at. `None` + (default) falls back to `context.timeout` config. """ if not isinstance(context, str): raise ValueError('Context should be a string') @@ -3315,7 +3367,9 @@ def set_context(self, context: str, word: str = '', origin: str = ''): original_context = context context = self.alphanumeric_skill_id + context self.intent_service._set_context(context, word, origin, - original_key=original_context) + original_key=original_context, + turns_remaining=turns_remaining, + expires_at=expires_at) def remove_context(self, context: str): """ @@ -3331,30 +3385,92 @@ def remove_context(self, context: str): self.intent_service._remove_context(context, original_key=original_context) - def set_cross_skill_context(self, context: str, word: str = ''): - """ - Tell all skills to add a context to the intent service + def set_cross_skill_context(self, context: str, word: str = '', + turns_remaining: Optional[int] = None, + expires_at: Optional[float] = None): + """ + Add a context entry visible to every skill's intents. + + CONTEXT-1 §3/§5.0: a shared entry is a bare (owner-less) key in + `session.intent_context`, so writing it directly into the session + bound to the current dispatch message (§5.3) already makes it + visible to every other skill's intents reading that same session - + no bus round trip is needed for the mutation itself. The session + write above is the only `session.intent_context` mutation this call + causes, and it is the authoritative one. + + The legacy `mycroft.skill.set_cross_context` broadcast below is kept + for orchestrators that still consume it. Its listener in every + receiving skill (`handle_set_cross_context`) writes only that + skill's adapt `session.context` field and never + `session.intent_context`, so the broadcast adds no second writer of + the shared entry — §5.0 removes the class of context-mutation topic + rather than replacing it, and this emit is a compat surface with a + removal version, not a mechanism. Args: - context: Keyword - word: word connected to keyword + context: Keyword + word: word connected to keyword + turns_remaining: number of intent matches this entry survives, + per CONTEXT-1 §2/§4. `None` (default) means no + turn-based decay. + expires_at: absolute unix timestamp this entry decays at. `None` + (default) falls back to the `context.timeout` + configuration **of the process making this call**, + which on a satellite is not the orchestrator's + configuration. §5.3 gives a deployer-configurable + default decay to the orchestrator, for entries + written without an explicit `turns_remaining` or + `expires_at`; because this path always stamps one, + that orchestrator-side default never reaches a + shared entry. Pass `expires_at` explicitly when the + window matters. """ + if not isinstance(context, str): + raise ValueError('Context should be a string') + if not isinstance(word, str): + raise ValueError('Word should be a string') msg = dig_for_message() or Message("") if "skill_id" not in msg.context: msg.context["skill_id"] = self.skill_id + session = SessionManager.get(msg) + if expires_at is None: + # OVOS-CONTEXT-1: same decay policy as the private set_context + # path (`context.timeout`, minutes, default 2). + context_cfg = Configuration().get('context', {}) + timeout_s = context_cfg.get('timeout', 2) * 60 + expires_at = time.time() + timeout_s if timeout_s > 0 else None + session.set_intent_context(context, word, scope="shared", + expires_at=expires_at, + turns_remaining=turns_remaining) + log_deprecation( + "mycroft.skill.set_cross_context is a pre-OVOS-CONTEXT-1 " + "compat broadcast kept for orchestrators that still consume " + "it; the shared session write above is now the authoritative " + "mutation", _CROSS_CONTEXT_REMOVAL_VERSION) self.bus.emit(msg.forward('mycroft.skill.set_cross_context', {'context': context, 'word': word, 'origin': self.skill_id})) def remove_cross_skill_context(self, context: str): """ - Tell all skills to remove a keyword from the context manager. + Remove a shared context entry from every skill's intents. + + CONTEXT-1 §3/§5.0: same session-delegation + legacy compat emit + as `set_cross_skill_context` above. """ if not isinstance(context, str): raise ValueError('context should be a string') msg = dig_for_message() or Message("") if "skill_id" not in msg.context: msg.context["skill_id"] = self.skill_id + session = SessionManager.get(msg) + session.remove_intent_context(context, scope="shared") + log_deprecation( + "mycroft.skill.remove_cross_context is a pre-OVOS-CONTEXT-1 " + "compat broadcast kept for orchestrators that still consume " + "it; the shared session removal above is now the authoritative " + "mutation", _CROSS_CONTEXT_REMOVAL_VERSION) self.bus.emit(msg.forward('mycroft.skill.remove_cross_context', {'context': context})) diff --git a/test/end2end/context1_reader_test_skill/__init__.py b/test/end2end/context1_reader_test_skill/__init__.py new file mode 100644 index 00000000..614680ff --- /dev/null +++ b/test/end2end/context1_reader_test_skill/__init__.py @@ -0,0 +1,43 @@ +# Copyright 2026 OpenVoiceOS +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Fixture skill for the OVOS-CONTEXT-1 §5.0 cross-skill-context +end-to-end test. + +Registers one template intent gated by a §6 ``requires_context`` +declaration on the SHARED (bare, owner-less) key ``person`` - a DIFFERENT +skill (``context1_setter_test_skill``) is the one that publishes it. +""" +from threading import Event + +from ovos_workshop.skills.ovos import OVOSSkill + + +class Context1ReaderTestSkill(OVOSSkill): + """Registers one template intent that only matches while a different + skill's shared context entry is live.""" + + def initialize(self): + self.handled = Event() + self.last_message = None + self.register_intent_file( + "height.intent", self.handle_height, + requires_context=[{"key": "person", "scope": "shared"}]) + + def handle_height(self, message): + self.last_message = message + self.handled.set() + + +def create_skill(): + return Context1ReaderTestSkill() diff --git a/test/end2end/context1_reader_test_skill/locale/en-us/height.intent b/test/end2end/context1_reader_test_skill/locale/en-us/height.intent new file mode 100644 index 00000000..02121a14 --- /dev/null +++ b/test/end2end/context1_reader_test_skill/locale/en-us/height.intent @@ -0,0 +1 @@ +tall he diff --git a/test/end2end/context1_setter_test_skill/__init__.py b/test/end2end/context1_setter_test_skill/__init__.py new file mode 100644 index 00000000..470353c6 --- /dev/null +++ b/test/end2end/context1_setter_test_skill/__init__.py @@ -0,0 +1,40 @@ +# Copyright 2026 OpenVoiceOS +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Fixture skill for the OVOS-CONTEXT-1 §5.0 cross-skill-context +end-to-end test. + +Handling one padacioso template intent calls the real +``OVOSSkill.set_cross_skill_context`` API - the producer under test - with +the bare shared key ``person``. +""" +from threading import Event + +from ovos_workshop.skills.ovos import OVOSSkill + + +class Context1SetterTestSkill(OVOSSkill): + """Registers one template intent whose handler publishes a + shared-scope context entry for a different skill to pick up.""" + + def initialize(self): + self.handled = Event() + self.register_intent_file("remember.intent", self.handle_remember) + + def handle_remember(self, message): + self.set_cross_skill_context("person", "Bob") + self.handled.set() + + +def create_skill(): + return Context1SetterTestSkill() diff --git a/test/end2end/context1_setter_test_skill/locale/en-us/remember.intent b/test/end2end/context1_setter_test_skill/locale/en-us/remember.intent new file mode 100644 index 00000000..f67ba932 --- /dev/null +++ b/test/end2end/context1_setter_test_skill/locale/en-us/remember.intent @@ -0,0 +1 @@ +remember bob diff --git a/test/end2end/test_context1_cross_skill_e2e.py b/test/end2end/test_context1_cross_skill_e2e.py new file mode 100644 index 00000000..7a27ef06 --- /dev/null +++ b/test/end2end/test_context1_cross_skill_e2e.py @@ -0,0 +1,169 @@ +# Copyright 2026 OpenVoiceOS +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Real-stack end-to-end proof of OVOS-CONTEXT-1 §5.0 for the SHARED +(cross-skill) scope. + + §5.0: "A component writes intent context by mutating the `session` it + carries or replies with... There is no context-mutation topic: no + participant emits a Message whose purpose is to announce a context + change to the orchestrator or to another component." + +Boots a real (mini) OVOS stack via ovoscope's ``MiniCroft`` with TWO real +skills: ``context1_setter_test_skill`` calls the real +``OVOSSkill.set_cross_skill_context`` API while handling one template +intent; ``context1_reader_test_skill`` registers a DIFFERENT template +intent gated by a §6 ``requires_context`` declaration on the same bare, +owner-less shared key. Two utterances are driven through the real +padacioso pipeline in the same session: the setter's intent, then the +reader's. The reader's intent must match only because the setter's +handler put the entry on the SESSION - the test disables the legacy +``mycroft.skill.set_cross_context`` compat broadcast for its whole +duration (drops it before it reaches the bus), so a bug that made the +broadcast the only real mutation path could never pass this test even if +some other component happened to still listen for it. + +Adapt is deliberately excluded from this run's pipeline: 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 +that combines keyword requirements with a `requires_context` declaration, +independently of this PR's change. Padacioso intents don't populate those +manifest fields and are unaffected - and the padatious engine is never +used in this repo's tests. +""" +import sys +from os.path import dirname + +import pytest + +from ovoscope import (get_minicroft, wait_for_match, make_utterance_message, + PADACIOSO_PIPELINE) + +from ovos_bus_client.session import Session, SessionManager +from ovos_spec_tools import SpecMessage + +sys.path.insert(0, dirname(__file__)) + +SETTER_ID = "context1.setter.e2e.test" +READER_ID = "context1.reader.e2e.test" + +DROPPED_LEGACY_TOPICS = {"mycroft.skill.set_cross_context", + "mycroft.skill.remove_cross_context"} + + +def _boot(): + """Boot a real MiniCroft with both CONTEXT-1 fixture skills, padacioso-only.""" + from context1_setter_test_skill import Context1SetterTestSkill + from context1_reader_test_skill import Context1ReaderTestSkill + return get_minicroft([SETTER_ID, READER_ID], + extra_skills={SETTER_ID: Context1SetterTestSkill, + READER_ID: Context1ReaderTestSkill}, + default_pipeline=PADACIOSO_PIPELINE, + wait_for_trained=False) + + +class TestContext1CrossSkillE2E: + """Skill A's `set_cross_skill_context` must open skill B's + `requires_context` gate purely through the session write - the legacy + broadcast is dropped before it ever reaches the bus for this whole + test class.""" + + mc = None + + @classmethod + def setup_class(cls): + from ovos_utils.log import LOG + LOG.set_level("ERROR") + # booting a real MiniCroft runs a real IntentService, whose startup + # calls SessionManager.connect_to_bus(mc.bus) - this mutates the + # process-wide SessionManager.bus class attribute. Save/restore it + # so later tests don't inherit a bus pointing at this (now-stopped) + # MiniCroft instance. + cls._saved_bus = SessionManager.bus + cls.mc = _boot() + + # Drop the legacy compat broadcast before it reaches the bus, for + # the whole class: if the gate below only opened because some + # listener still reacted to `mycroft.skill.set_cross_context`, this + # makes that impossible - the session write is the only thing left + # that could satisfy the reader's gate. + real_emit = cls.mc.bus.emit + + def _emit_dropping_legacy_cross_context(message): + if message.msg_type in DROPPED_LEGACY_TOPICS: + return + return real_emit(message) + + cls._real_emit = real_emit + cls.mc.bus.emit = _emit_dropping_legacy_cross_context + + @classmethod + def teardown_class(cls): + cls.mc.bus.emit = cls._real_emit + cls.mc.stop() + SessionManager.bus = cls._saved_bus + + def test_shared_context_gate_opens_across_skills_in_same_session(self): + setter = self.mc.plugin_skills[SETTER_ID].instance + reader = self.mc.plugin_skills[READER_ID].instance + setter.handled.clear() + reader.handled.clear() + reader.last_message = None + + session = Session("context1-cross-skill-e2e") + + # Turn 1: the setter's intent, publishing the shared "person" entry. + # Wait for the TERMINAL event, not the match event - the match fires + # before the handler (and its set_cross_skill_context call) runs; + # only a forward-derived message emitted AFTER the handler completes + # is guaranteed to carry the mutated session (OVOS-SESSION-2 §2.6). + set_msg = make_utterance_message("remember bob", session=session) + handled = wait_for_match( + self.mc.bus, [str(SpecMessage.UTTERANCE_HANDLED)], + timeout=10, emit=set_msg) + assert handled is not None, "setter intent never matched/completed" + assert setter.handled.wait(5), "setter handler never ran" + + # OVOS-SESSION-2 §2.2: a NAMED session carries no state of its own + # in the registry - the working session travels on the Messages of + # the utterance flow that holds it. Read it back from the terminal + # event's own session (SESSION-2's "the client declares the session + # on every message" discipline), never from a private local + # reference to the `session` object built above. + live = Session.deserialize(handled.context["session"]) + assert "person" in (live.intent_context or {}), ( + "shared 'person' entry never landed in session.intent_context - " + "set_cross_skill_context did not write through the session") + entry = live.intent_context["person"] + assert entry["value"] == "Bob" + + # Turn 2: the reader's intent, same session - only satisfiable while + # the shared "person" entry set above is live. + ask_msg = make_utterance_message("tall he", session=live) + matched = wait_for_match( + self.mc.bus, [f"{READER_ID}:height"], + timeout=10, emit=ask_msg) + assert matched is not None, ( + "OVOS-CONTEXT-1 §5.0: the reader skill's requires_context-gated " + "intent did not match after the setter skill's real " + "set_cross_skill_context() call, with the legacy " + "mycroft.skill.set_cross_context broadcast dropped before it " + "reached the bus - the session write is not reaching the real " + "pipeline's gate") + assert reader.handled.wait(5), "reader handler never ran" + assert reader.last_message is not None + + +if __name__ == "__main__": + pytest.main([__file__, "-v"]) diff --git a/test/unittests/skills/test_base.py b/test/unittests/skills/test_base.py index 31c18ff3..c5d8be0e 100644 --- a/test/unittests/skills/test_base.py +++ b/test/unittests/skills/test_base.py @@ -26,6 +26,7 @@ from ovos_utils.fakebus import FakeBus from ovos_bus_client.message import Message +from ovos_bus_client.session import Session, SessionManager class TestOVOSSkill(unittest.TestCase): @@ -1066,10 +1067,11 @@ def handle_as_if_dispatched(message): "skill_id (skill.a) instead of the true caller (skill.b)") def test_handle_set_cross_context(self): - """Round 2 (C1b) regression: each RECEIVING skill's - handle_set_cross_context must resolve the mirrored key under ITS - OWN skill_id, not the originating broadcaster's - the broadcast - message's context.skill_id is stamped by the ORIGINATOR.""" + """CONTEXT-1 §5.0: a Message that announces a context change must + not cause a `session.intent_context` write in the receiver. The + listener emits the adapt-engine `add_context` compat message and + nothing else, so it carries no resolved `key` and leaves the + session's intent_context untouched.""" bus = FakeBus() skill_b = OVOSSkill(bus=bus, skill_id="skill.b") @@ -1082,33 +1084,170 @@ def handler(message): bus.on("add_context", handler) + session = Session("test_handle_set_cross_context") # broadcast as emitted by the originating skill (skill.a) broadcast = Message("mycroft.skill.set_cross_context", {"context": "kitchen", "word": "kitchen", "origin": "skill.a"}, - {"skill_id": "skill.a"}) + {"skill_id": "skill.a", + "session": session.serialize()}) skill_b.handle_set_cross_context(broadcast) self.assertTrue(received.wait(2)) self.assertEqual(len(payloads), 1) emitted = payloads[0] - self.assertEqual(emitted.data["key"], "kitchen") - self.assertEqual( - emitted.context.get("skill_id"), "skill.b", - "cross-context receiver mirrored the resolved key under the " - "ORIGINATING skill's id instead of its own") + self.assertEqual(emitted.data["context"], "skill_bkitchen") + self.assertNotIn( + "key", emitted.data, + "the broadcast-driven path asked core to mirror a resolved " + "private key, which is a second writer of intent_context") + try: + live = SessionManager.get(broadcast) + self.assertEqual( + dict(live.intent_context or {}), {}, + "the legacy broadcast wrote session.intent_context in the " + "receiving skill (CONTEXT-1 §5.0)") + finally: + SessionManager.sessions.pop(session.session_id, None) def test_handle_remove_cross_context(self): - # TODO - pass + """The mirror of the above: the removal broadcast touches the + adapt-engine field only.""" + bus = FakeBus() + skill_b = OVOSSkill(bus=bus, skill_id="skill.b") - def test_set_cross_skill_contest(self): - # TODO - pass + received = Event() + payloads = [] + + def handler(message): + payloads.append(message) + received.set() + + bus.on("remove_context", handler) + + session = Session("test_handle_remove_cross_context") + session.intent_context = {"kitchen": {"value": "kitchen"}} + broadcast = Message("mycroft.skill.remove_cross_context", + {"context": "kitchen"}, + {"skill_id": "skill.a", + "session": session.serialize()}) + skill_b.handle_remove_cross_context(broadcast) + self.assertTrue(received.wait(2)) + + self.assertEqual(len(payloads), 1) + self.assertEqual(payloads[0].data["context"], "skill_bkitchen") + self.assertNotIn("key", payloads[0].data) + try: + live = SessionManager.get(broadcast) + # the shared entry the originator wrote is untouched here: the + # originator's own `remove_cross_skill_context` removed it on + # the session, and a receiver must not remove it again. + self.assertEqual(dict(live.intent_context or {}), + {"kitchen": {"value": "kitchen"}}) + finally: + SessionManager.sessions.pop(session.session_id, None) + + def test_set_context_turns_remaining_lands_on_session(self): + """OVOS-CONTEXT-1 §1.2's one-turn confirmation gate + (`{"value": ..., "turns_remaining": 1}`) must be reachable from the + skill API.""" + bus = FakeBus() + skill = OVOSSkill(bus=bus, skill_id=self.skill_id) + + session = Session("test_set_context_turns_remaining") + msg = Message("some.intent", {}, {"session": session.serialize()}) + + def dispatch(message): + skill.set_context("confirming_milk", turns_remaining=1) + + dispatch(msg) + + try: + live = SessionManager.get(msg) + entry = live.intent_context[f"{skill.skill_id}:confirming_milk"] + self.assertEqual(entry["turns_remaining"], 1) + finally: + # `default_shutdown()` is idempotent (guarded by + # `_shutdown_done`) - calling it now means the `__del__` that + # runs whenever GC eventually drops `skill` is a no-op, instead + # of an unbounded-delay call into whatever mock/patch another + # test has active on `OVOSSkill.default_shutdown` at the time. + skill.default_shutdown() + + def test_set_cross_skill_context_no_bus_mutation(self): + """OVOS-CONTEXT-1 §5.0: there is no bus topic whose purpose is to + mutate context - `set_cross_skill_context` must write the shared + entry directly into the session, not rely on a Message to do it.""" + bus = FakeBus() + skill = OVOSSkill(bus=bus, skill_id="skill.a") + + session = Session("test_set_cross_skill_context_no_bus") + msg = Message("some.intent", {}, {"session": session.serialize()}) + + received = [] + bus.on("mycroft.skill.set_cross_context", lambda m: received.append(m)) + + def dispatch(message): + skill.set_cross_skill_context("person", "Bob", turns_remaining=3) + + dispatch(msg) + + try: + # the mutation already landed on the session, synchronously, + # independent of whatever legacy compat message rides the bus + live = SessionManager.get(msg) + self.assertIn("person", live.intent_context) + entry = live.intent_context["person"] + self.assertEqual(entry["value"], "Bob") + self.assertEqual(entry["turns_remaining"], 3) + finally: + # see test_set_context_turns_remaining_lands_on_session + skill.default_shutdown() + + def test_set_cross_skill_context_invalid_word_leaves_session_unchanged(self): + """A non-string `word` must raise before the session write, not + after it. Writing first and validating later (in the legacy + `handle_set_cross_context` listener) would leave a bad entry in + the shared session even though the call raised.""" + bus = FakeBus() + skill = OVOSSkill(bus=bus, skill_id="skill.a") + + session = Session("test_set_cross_skill_context_invalid_word") + msg = Message("some.intent", {}, {"session": session.serialize()}) + + def dispatch(message): + with self.assertRaises(ValueError): + skill.set_cross_skill_context("thing", None) + + dispatch(msg) + + try: + live = SessionManager.get(msg) + self.assertNotIn("thing", live.intent_context) + finally: + skill.default_shutdown() def test_remove_cross_skill_context(self): - # TODO - pass + """Symmetric with set_cross_skill_context: removal also lands on + the shared session entry, not only on the legacy broadcast.""" + bus = FakeBus() + skill = OVOSSkill(bus=bus, skill_id="skill.a") + + session = Session("test_remove_cross_skill_context") + session.set_intent_context("person", "Bob", scope="shared") + msg = Message("some.intent", {}, {"session": session.serialize()}) + + def dispatch(message): + skill.remove_cross_skill_context("person") + + dispatch(msg) + + try: + live = SessionManager.get(msg) + self.assertIsNone(live.intent_context["person"]) + finally: + # see test_set_context_turns_remaining_lands_on_session + skill.default_shutdown() def test_register_vocabulary(self): # TODO diff --git a/test/unittests/test_abstract_app.py b/test/unittests/test_abstract_app.py index 352cb494..9d99345c 100644 --- a/test/unittests/test_abstract_app.py +++ b/test/unittests/test_abstract_app.py @@ -11,6 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import gc import unittest from os import remove from unittest.mock import Mock, patch @@ -34,6 +35,17 @@ class TestApp(unittest.TestCase): app = Application(skill_id="TestApplication", gui=gui, bus=bus) + def setUp(self): + # `test_default_shutdown` below patches `OVOSSkill.default_shutdown` + # at the class level for the duration of one test method; any OVOSSkill + # instance from an earlier test that is still only cyclic garbage + # (skill <-> bus event-handler reference cycles, collected by the + # generational GC rather than refcounting) would have its `__del__` + # fire into that same patched mock if collection happens to land + # inside this test's window. Flushing here, before any patch is + # active, keeps that window free of unrelated cross-test garbage. + gc.collect() + def test_gui_init(self): # The passed GUIInterface has len()==0 (empty data), so it evaluates as # falsy and OVOSSkill._startup replaces it with a fresh SkillGUI instance.