fix: collapse dual-registered intent aliases at registration time

Root cause of the session blacklist bypass: this plugin subscribes to
both the legacy registration topic (name suffixed `<skill_id>:<file>.intent`)
and the OVOS-INTENT-4 spec topic (`<skill_id>:<file>`, suffix-less), so one
logical skill intent lands as TWO engine entries. Blacklisting one alias
left the other matchable.

This plugin owns its own back-compat for the legacy topic, so it now
collapses the alias at REGISTRATION time instead of at compare time:

- `register_intent` (legacy topic handler) now canonicalizes the intent
  name — strips a trailing `.intent` suffix — before indexing, so both
  wire messages land as a single engine entry, and only appends to the
  manifest when the canonical name is new (`handle_register_template`
  already replaced-in-place via `§8.1 replacement is implicit`).
- `__detach_intent` canonicalizes too, so detaching by either alias works.
- The engine (`padacioso.IntentContainer.add_intent`) keys intents by name
  in a dict, so re-registration under the canonical name was already an
  implicit replace, not a duplicate — no engine-level idempotency fix
  needed here (unlike the padatious plugin's list-based engine).
- The session blacklist filter (`_calc_padacioso_intent`) no longer
  dealiases `i["name"]` — engine matches are canonical by construction
  now. It still canonicalizes the blacklist entries themselves, since old
  sessions/mycroft.conf may still list an intent by its legacy
  `.intent`-suffixed id. A one-time-per-entry LOG.warning flags any such
  legacy blacklist entry as deprecated, naming the offending entry and its
  canonical replacement, so operators can update their config.

Tests: TestSessionBlacklistAlias (compare-time suppression, either alias)
kept/updated; new TestRegistrationCollapse + TestSessionBlacklistCanonicalization
in test/test_registration_collapse.py cover the registration-time collapse
(single manifest entry, detach-by-legacy-name, deprecation warning).
test_malformed_samples.py updated: the warning/index now names the
canonicalized (alias-collapsed) intent name.

Known gap (see the same-shape padatious plugin PR for full detail):
ovos-workshop's register_intent_file() binds a skill's dispatch handler
only to the legacy `<skill_id>:<file>.intent` topic, not the spec
canonical one, so an orchestrator dispatching the canonicalized name no
longer reaches that handler. Not fixable in this plugin.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: JarbasAl

padacioso/opm.py

@@ -199,6 +199,10 @@ def __detach_intent(self, intent_name):
         Args:
             intent_name (str): intent identifier
         """
+        # Detach/removal must key off the same canonical name registration
+        # collapsed onto, so unregistering by either the legacy `.intent`
+        # alias or the OVOS-INTENT-4 canonical id works (ovos-core#831).
+        intent_name = _dealias_intent_name(intent_name)
         if intent_name in self.registered_intents:
             self.registered_intents.remove(intent_name)
             self._intent_context_gates.pop(intent_name, None)
@@ -306,6 +310,15 @@ def register_intent(self, message):
         Args:
             message (Message): message triggering action
         """
+        # ovos-workshop >= 9.3 dual-registers one logical intent under both
+        # the legacy ``padatious:register_intent`` contract (name suffixed
+        # ``.intent``) and the OVOS-INTENT-4 spec contract (suffix-less,
+        # routed via handle_register_template). Collapse the alias to the
+        # canonical name HERE, at registration time, so both wire messages
+        # index a single engine entry instead of two matchable duplicates
+        # (ovos-core#831). This plugin owns its own back-compat.
+        message.data['name'] = _dealias_intent_name(message.data['name'])
+
         lang = message.data.get('lang', self.lang)
         lang = standardize_lang(lang)
         if lang in self.containers:
@@ -319,7 +332,11 @@ def register_intent(self, message):
                     raise
                 registered = True
             if registered:
-                self.registered_intents.append(message.data['name'])
+                # §8.1 replacement is implicit: a re-registration of the same
+                # canonical name replaces the prior manifest entry rather
+                # than stacking a duplicate (mirrors handle_register_template).
+                if message.data['name'] not in self.registered_intents:
+                    self.registered_intents.append(message.data['name'])
                 self._store_context_gate(message.data['name'], message.data)
 
     def register_entity(self, message):
@@ -588,6 +605,56 @@ def shutdown(self):
         self.bus.remove(SpecMessage.INTENT_DISABLE.value, self.handle_disable_intent)
 
 
+def _dealias_intent_name(name: Optional[str]) -> Optional[str]:
+    """Fold the legacy ``<skill_id>:<file>.intent`` id onto the OVOS-INTENT-4
+    canonical ``<skill_id>:<file>`` id.
+
+    ovos-workshop >= 9.3 dual-registers one skill capability under both wire
+    forms during the INTENT-4 migration (the legacy ``padatious:register_intent``
+    contract and the spec ``ovos.intent.register.template`` contract, whose
+    ``intent_name`` already has the ``.intent`` suffix stripped). This plugin
+    folds that onto one canonical engine entry at REGISTRATION time (see
+    ``PadaciosoPipeline.register_intent``/``handle_register_template``), so
+    engine matches (``i["name"]``) are canonical by construction.
+
+    This helper is also used to canonicalize session ``blacklisted_intents``
+    entries, since old sessions/configs may still carry the legacy
+    ``.intent``-suffixed id (ovos-core#831; OVOS-PIPELINE-1 §5.4).
+    """
+    if name and name.endswith(".intent"):
+        return name[:-len(".intent")]
+    return name
+
+
+# Legacy `.intent`-suffixed blacklist entries are deprecated compat, not a
+# stable contract. Warn once per distinct offending entry (not per utterance)
+# so stale mycroft.conf/session config gets flagged without spamming the log.
+_warned_legacy_blacklist_entries = set()
+
+
+def _canonicalize_blacklist(blacklisted_intents: frozenset) -> frozenset:
+    """Canonicalize legacy `.intent`-suffixed session blacklist entries.
+
+    Sessions/config may still list intents by the legacy
+    ``<skill_id>:<file>.intent`` id. Engine matches are canonical by
+    construction (registration-time alias collapse), so the blacklist must be
+    normalized to compare correctly. Logs a one-time deprecation warning per
+    distinct legacy entry pointing at the canonical replacement.
+    """
+    canonical = set()
+    for b in blacklisted_intents:
+        c = _dealias_intent_name(b)
+        canonical.add(c)
+        if c != b and b not in _warned_legacy_blacklist_entries:
+            _warned_legacy_blacklist_entries.add(b)
+            LOG.warning(
+                f"Session blacklisted_intents entry '{b}' uses the deprecated "
+                f"legacy '.intent'-suffixed id; support for this alias will "
+                f"be removed. Update mycroft.conf / session config to use the "
+                f"canonical id '{c}' instead.")
+    return frozenset(canonical)
+
+
 @lru_cache(maxsize=128)  # covers burst of multiple ASR hypotheses without thrashing
 def _calc_padacioso_intent(utt: str,
                            intent_container: FallbackIntentContainer,
@@ -602,6 +669,10 @@ def _calc_padacioso_intent(utt: str,
     @return: matched PadaciosoIntent
     """
     try:
+        blacklisted_intents = _canonicalize_blacklist(blacklisted_intents)
+        # Matches are canonical by construction (registration-time alias
+        # collapse, see PadaciosoPipeline.register_intent), so only the
+        # blacklist needs canonicalizing here.
         intents = [i for i in intent_container.calc_intents(utt)
                    if i is not None
                    and i["name"] not in blacklisted_intents

test/test_malformed_samples.py

@@ -38,7 +38,9 @@ def test_malformed_sample_skipped_valid_indexed(self):
                                       'stop everything'])
         self.assertEqual(warn.call_count, 2)
         container = pipeline.containers['en-US']
-        name = 'skill-persona.openvoiceos:cancel.intent'
+        # registration-time alias collapse (ovos-core#831) canonicalizes the
+        # legacy `.intent`-suffixed name before indexing
+        name = 'skill-persona.openvoiceos:cancel'
         self.assertIn(name, container.intent_samples)
         result = container.calc_intent('stop everything')
         self.assertEqual(result['name'], name)
@@ -58,7 +60,9 @@ def test_warning_names_skill_intent_lang_topic(self):
         with mock.patch('padacioso.opm.LOG.warning') as warn:
             self._register(pipeline, ['{utterance}', 'stop everything'])
         logged = " ".join(str(c) for c in warn.call_args_list)
-        for token in ('skill-persona.openvoiceos', 'cancel.intent',
+        # the warning names the canonical (alias-collapsed) intent name,
+        # since registration-time collapse happens before this log fires
+        for token in ('skill-persona.openvoiceos', 'cancel',
                       'en-US', 'padatious:register_intent'):
             self.assertIn(token, logged)
 

test/test_ovoscope_e2e.py

@@ -122,5 +122,56 @@ def test_blacklisted_skill_is_skipped(self):
         self.expect_no_match("hello", session=sess, timeout=3.0)
 
 
+class TestSessionBlacklistAlias(_PadaciosoHarness):
+    """ovos-workshop >= 9.3 dual-registers each ``.intent`` file under both
+    the legacy ``<skill_id>:<file>.intent`` id and the OVOS-INTENT-4
+    canonical ``<skill_id>:<file>`` id (ovos-core#831). The plugin collapses
+    that alias onto one canonical engine entry at REGISTRATION time (see
+    ``PadaciosoPipeline.register_intent``/``handle_register_template``), so
+    a session blacklist entry naming either alias must suppress the single
+    canonical match, per OVOS-PIPELINE-1 §5.4. Here both messages go through
+    the legacy topic with different names to exercise the blacklist
+    canonicalization path directly; ``test/test_registration_collapse.py``
+    covers the registration-time collapse across both wire topics.
+    """
+
+    LEGACY_NAME = f"{_PadaciosoHarness.SKILL_ID}:hello.intent"
+    NEW_NAME = f"{_PadaciosoHarness.SKILL_ID}:hello"
+
+    def _register_both_aliases(self):
+        self._register_intent(self.LEGACY_NAME, _HELLO_SAMPLES)
+        self._register_intent(self.NEW_NAME, _HELLO_SAMPLES)
+
+    def test_blacklisting_legacy_id_suppresses_new_alias(self):
+        self._register_both_aliases()
+        sess = make_session(
+            "bl-alias-legacy-test",
+            blacklisted_intents=[self.LEGACY_NAME],
+        )
+        self.expect_no_match("hello", session=sess, timeout=3.0)
+
+    def test_blacklisting_new_id_suppresses_legacy_alias(self):
+        self._register_both_aliases()
+        sess = make_session(
+            "bl-alias-new-test",
+            blacklisted_intents=[self.NEW_NAME],
+        )
+        self.expect_no_match("hello", session=sess, timeout=3.0)
+
+    def test_non_blacklisted_intent_still_matches(self):
+        self._register_both_aliases()
+        self._register_intent(f"{self.SKILL_ID}:bye", _BYE_SAMPLES)
+        sess = make_session(
+            "bl-alias-unrelated-test",
+            blacklisted_intents=[f"{self.SKILL_ID}:bye"],
+        )
+        msg = self.send_and_capture(
+            "hello",
+            expected_types=[self.LEGACY_NAME, self.NEW_NAME],
+            session=sess,
+        )
+        self.assertIsNotNone(msg)
+
+
 if __name__ == "__main__":
     unittest.main()

test/test_registration_collapse.py

@@ -0,0 +1,117 @@
+"""Registration-time alias collapse and blacklist canonicalization.
+
+ovos-workshop >= 9.3 dual-registers one logical intent under both the
+legacy ``<skill_id>:<file>.intent`` id (via ``padatious:register_intent``)
+and the OVOS-INTENT-4 canonical ``<skill_id>:<file>`` id (via
+``ovos.intent.register.template`` -> ``handle_register_template``). This
+plugin owns collapsing that alias at REGISTRATION time so both wire
+contracts land as a single engine entry (ovos-core#831).
+
+These tests cover:
+- both registration messages collapse to exactly one manifest entry
+- detaching by the legacy name removes the collapsed entry
+- the session blacklist filter still canonicalizes legacy-named entries
+  (since old sessions/config may carry them) and warns once per entry
+"""
+import unittest
+from unittest import mock
+
+from ovos_bus_client.message import Message
+from ovos_utils.fakebus import FakeBus
+
+from padacioso.opm import PadaciosoPipeline, _warned_legacy_blacklist_entries
+
+SKILL_ID = "collapse.skill"
+LEGACY_NAME = f"{SKILL_ID}:hello.intent"
+NEW_NAME = f"{SKILL_ID}:hello"
+LANG = "en-US"
+SAMPLES = ["hello", "hi there", "hey"]
+
+
+def legacy_register_msg():
+    return Message("padatious:register_intent", {
+        "skill_id": SKILL_ID, "name": LEGACY_NAME, "lang": LANG,
+        "samples": SAMPLES,
+    }, {"skill_id": SKILL_ID})
+
+
+def spec_register_msg():
+    return Message("ovos.intent.register.template", {
+        "skill_id": SKILL_ID, "intent_name": "hello", "lang": LANG,
+        "samples": SAMPLES,
+    }, {"skill_id": SKILL_ID})
+
+
+class TestRegistrationCollapse(unittest.TestCase):
+    def setUp(self):
+        self.pipeline = PadaciosoPipeline(FakeBus())
+
+    def test_both_wire_contracts_collapse_to_one_manifest_entry(self):
+        self.pipeline.register_intent(legacy_register_msg())
+        self.pipeline.handle_register_template(spec_register_msg())
+
+        manifest = self.pipeline.registered_intents
+        self.assertEqual(manifest.count(NEW_NAME), 1)
+        self.assertNotIn(LEGACY_NAME, manifest)
+
+    def test_second_arrival_replaces_not_duplicates_engine_entry(self):
+        self.pipeline.register_intent(legacy_register_msg())
+        self.pipeline.handle_register_template(spec_register_msg())
+
+        container = self.pipeline.containers[LANG]
+        # engine keys intents by name in a dict, so re-registration is
+        # inherently a replace; assert there's exactly one entry present
+        self.assertIn(NEW_NAME, container.intent_samples)
+        self.assertNotIn(LEGACY_NAME, container.intent_samples)
+
+    def test_detach_by_legacy_name_removes_collapsed_entry(self):
+        self.pipeline.register_intent(legacy_register_msg())
+        self.pipeline.handle_register_template(spec_register_msg())
+        self.assertIn(NEW_NAME, self.pipeline.registered_intents)
+
+        self.pipeline.handle_detach_intent(
+            Message("detach_intent", {"intent_name": LEGACY_NAME}))
+
+        self.assertNotIn(NEW_NAME, self.pipeline.registered_intents)
+        self.assertNotIn(NEW_NAME, self.pipeline.containers[LANG].intent_samples)
+
+
+class TestSessionBlacklistCanonicalization(unittest.TestCase):
+    """Matches are canonical by construction; only the blacklist entries
+    (which may still carry legacy-named sessions) need canonicalizing."""
+
+    def setUp(self):
+        _warned_legacy_blacklist_entries.clear()
+        self.pipeline = PadaciosoPipeline(FakeBus())
+        self.pipeline.register_intent(legacy_register_msg())
+        self.pipeline.handle_register_template(spec_register_msg())
+
+    def test_legacy_named_blacklist_entry_suppresses_canonical_match(self):
+        sess = mock.Mock()
+        sess.blacklisted_intents = [LEGACY_NAME]
+        sess.blacklisted_skills = []
+        sess.intent_context = {}
+        with mock.patch("padacioso.opm.SessionManager.get", return_value=sess):
+            intent = self.pipeline.calc_intent(["hello"], lang=LANG)
+        self.assertIsNone(intent)
+
+    def test_legacy_named_blacklist_entry_logs_deprecation_warning_once(self):
+        sess = mock.Mock()
+        sess.blacklisted_intents = [LEGACY_NAME]
+        sess.blacklisted_skills = []
+        sess.intent_context = {}
+        from padacioso.opm import _calc_padacioso_intent
+        _calc_padacioso_intent.cache_clear()
+        with mock.patch("padacioso.opm.SessionManager.get", return_value=sess), \
+                mock.patch("padacioso.opm.LOG.warning") as mock_warn:
+            self.pipeline.calc_intent(["hello"], lang=LANG)
+            self.pipeline.calc_intent(["hi there"], lang=LANG)
+
+        deprecation_calls = [c for c in mock_warn.call_args_list
+                             if LEGACY_NAME in str(c)]
+        self.assertEqual(len(deprecation_calls), 1)
+        self.assertIn(NEW_NAME, str(deprecation_calls[0]))
+
+
+if __name__ == "__main__":
+    unittest.main()