Merge pull request #70 from OpenVoiceOS/fix/intent4-skip-malformed-templates

fix: skip malformed template samples instead of crashing registration

Author: JarbasAl

padacioso/opm.py

@@ -9,7 +9,8 @@
 from ovos_bus_client.session import SessionManager, Session
 from ovos_config.config import Configuration
 from ovos_plugin_manager.templates.pipeline import ConfidenceMatcherPipeline, IntentHandlerMatch
-from ovos_spec_tools import closest_lang, standardize_lang, SpecMessage, gate_satisfied
+from ovos_spec_tools import closest_lang, standardize_lang, SpecMessage, gate_satisfied, \
+    expand, MalformedTemplate
 from ovos_utils import flatten_list
 from ovos_utils.fakebus import FakeBus
 from ovos_utils.log import LOG, log_deprecation
@@ -241,13 +242,37 @@ def handle_detach_skill(self, message):
             if en["name"].startswith(skill_id_colon):
                 self.__detach_entity(en["name"], en["lang"])
 
-    def _register_object(self, message, object_name, register_func):
+    def _valid_samples(self, samples, topic, name, lang):
+        """Drop malformed template samples, keeping the valid ones.
+
+        OVOS-INTENT-4 §6.3/§5.3 — each sample that fails template expansion
+        is skipped with a WARN naming the owning skill, the intent/entity,
+        the lang, the topic and the reason; the remaining samples are still
+        indexed. An empty return means the registration must be rejected.
+        """
+        skill_id = name.split(':')[0] if ':' in name else None
+        valid = []
+        for sample in samples:
+            try:
+                expand(sample)
+                valid.append(sample)
+            except MalformedTemplate as e:
+                LOG.warning(f"skipping malformed sample on {topic}: "
+                            f"skill_id={skill_id!r} name={name!r} "
+                            f"lang={lang!r} reason={e}")
+        return valid
+
+    def _register_object(self, message, object_name, register_func, lang):
         """Generic method for registering a padacioso object.
 
         Args:
             message (Message): trigger for action
             object_name (str): type of entry to register
             register_func (callable): function to call for registration
+            lang (str): standardized language of the registration
+
+        Returns:
+            bool: True if something was registered
         """
         file_name = message.data.get('file_name')
         samples = message.data.get("samples")
@@ -257,15 +282,23 @@ def _register_object(self, message, object_name, register_func):
 
         if (not file_name or not isfile(file_name)) and not samples:
             LOG.error('Could not find file ' + file_name)
-            return
+            return False
 
         if not samples and isfile(file_name):
             with open(file_name) as f:
                 samples = [line.strip() for line in f.readlines()]
 
+        samples = self._valid_samples(samples, message.msg_type, name, lang)
+        if not samples:  # §6.3 — reject only when nothing valid remains
+            LOG.warning(f"rejecting {object_name} registration on "
+                        f"{message.msg_type}: name={name!r} lang={lang!r} "
+                        f"reason=no valid samples remain")
+            return False
+
         register_func(name, samples)
         # the container was mutated; drop stale cached matches
         _calc_padacioso_intent.cache_clear()
+        return True
 
     def register_intent(self, message):
         """Messagebus handler for registering intents.
@@ -276,16 +309,18 @@ def register_intent(self, message):
         lang = message.data.get('lang', self.lang)
         lang = standardize_lang(lang)
         if lang in self.containers:
-            self.registered_intents.append(message.data['name'])
-            self._store_context_gate(message.data['name'], message.data)
             try:
-                self._register_object(message, 'intent',
-                                      self.containers[lang].add_intent)
+                registered = self._register_object(
+                    message, 'intent', self.containers[lang].add_intent, lang)
             except RuntimeError:
                 name = message.data.get('name', "")
                 # padacioso fails on reloading a skill, just ignore
                 if name not in self.containers[lang].intent_samples:
                     raise
+                registered = True
+            if registered:
+                self.registered_intents.append(message.data['name'])
+                self._store_context_gate(message.data['name'], message.data)
 
     def register_entity(self, message):
         """Messagebus handler for registering entities.
@@ -296,9 +331,9 @@ def register_entity(self, message):
         lang = message.data.get('lang', self.lang)
         lang = standardize_lang(lang)
         if lang in self.containers:
-            self.registered_entities.append(message.data)
-            self._register_object(message, 'entity',
-                                  self.containers[lang].add_entity)
+            if self._register_object(message, 'entity',
+                                     self.containers[lang].add_entity, lang):
+                self.registered_entities.append(message.data)
 
     # ------------------------------------------------------------------
     # OVOS-INTENT-4 bus handlers (consumed alongside the legacy topics)
@@ -339,6 +374,10 @@ def handle_register_template(self, message: Message):
             return
 
         name = self._internal_name(skill_id, intent_name)
+        samples = self._valid_samples(samples, topic, name, lang)
+        if not samples:  # §6.3 — reject only when nothing valid remains
+            self._warn_malformed(topic, data, "no valid samples remain")
+            return
         # §8.1 replacement is implicit: a re-registration replaces the prior entry
         self.__detach_intent(name)
         self.registered_intents.append(name)
@@ -374,6 +413,10 @@ def handle_register_entity(self, message: Message):
             return
 
         name = self._internal_name(skill_id, entity_name)
+        samples = self._valid_samples(samples, topic, name, lang)
+        if not samples:  # §7.2 — reject only when nothing valid remains
+            self._warn_malformed(topic, data, "no valid samples remain")
+            return
         # §8.1 replacement is implicit
         self.__detach_entity(name, lang)
         self.registered_entities = [

test/test_malformed_samples.py

@@ -0,0 +1,124 @@
+"""Malformed template samples must never crash intent registration.
+
+OVOS-INTENT-4 §6.3/§5.3: consumers skip malformed samples with a warning
+(naming skill, intent, lang, topic and reason), index the remaining valid
+samples, and reject the registration only when no valid sample remains —
+the executor never crashes.
+"""
+import unittest
+from unittest import mock
+
+from ovos_bus_client.message import Message
+from ovos_utils.fakebus import FakeBus
+
+from padacioso.opm import PadaciosoPipeline
+
+
+def _pipeline():
+    return PadaciosoPipeline(FakeBus(), {"fuzz": False})
+
+
+class LegacyRegistrationMalformedSamplesTest(unittest.TestCase):
+    """Legacy ``padatious:register_intent`` path."""
+
+    def _register(self, pipeline, samples,
+                  name='skill-persona.openvoiceos:cancel.intent'):
+        pipeline.register_intent(Message('padatious:register_intent',
+                                         {'name': name,
+                                          'lang': 'en-US',
+                                          'samples': samples}))
+
+    def test_malformed_sample_skipped_valid_indexed(self):
+        pipeline = _pipeline()
+        with mock.patch('padacioso.opm.LOG.warning') as warn:
+            # slot-only and unbalanced samples ride along a valid one,
+            # as in released locale files with translated slot names
+            self._register(pipeline, ['{utterance}',
+                                      'cancel {rotina',
+                                      'stop everything'])
+        self.assertEqual(warn.call_count, 2)
+        container = pipeline.containers['en-US']
+        name = 'skill-persona.openvoiceos:cancel.intent'
+        self.assertIn(name, container.intent_samples)
+        result = container.calc_intent('stop everything')
+        self.assertEqual(result['name'], name)
+
+    def test_all_samples_malformed_rejects_registration(self):
+        pipeline = _pipeline()
+        with mock.patch('padacioso.opm.LOG.warning') as warn:
+            self._register(pipeline, ['{utterance}', '{other}'])
+        self.assertTrue(warn.called)
+        container = pipeline.containers['en-US']
+        name = 'skill-persona.openvoiceos:cancel.intent'
+        self.assertNotIn(name, container.intent_samples)
+        self.assertNotIn(name, pipeline.registered_intents)
+
+    def test_warning_names_skill_intent_lang_topic(self):
+        pipeline = _pipeline()
+        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',
+                      'en-US', 'padatious:register_intent'):
+            self.assertIn(token, logged)
+
+    def test_legacy_entity_malformed_sample_skipped(self):
+        pipeline = _pipeline()
+        with mock.patch('padacioso.opm.LOG.warning') as warn:
+            pipeline.register_entity(
+                Message('padatious:register_entity',
+                        {'name': 'skill-persona.openvoiceos:thing',
+                         'lang': 'en-US',
+                         'samples': ['ok value', 'broken {value']}))
+        self.assertEqual(warn.call_count, 1)
+        container = pipeline.containers['en-US']
+        self.assertIn('skill-persona.openvoiceos:thing',
+                      container.entity_samples)
+
+
+class SpecRegistrationMalformedSamplesTest(unittest.TestCase):
+    """INTENT-4 ``ovos.intent.register.template`` / entity paths."""
+
+    def test_template_malformed_sample_skipped(self):
+        pipeline = _pipeline()
+        with mock.patch('padacioso.opm.LOG.warning') as warn:
+            pipeline.handle_register_template(
+                Message('ovos.intent.register.template',
+                        {'skill_id': 'skill-persona.openvoiceos',
+                         'intent_name': 'cancel',
+                         'lang': 'en-US',
+                         'samples': ['{utterance}', 'stop everything']}))
+        self.assertEqual(warn.call_count, 1)
+        name = pipeline._internal_name('skill-persona.openvoiceos', 'cancel')
+        self.assertIn(name, pipeline.containers['en-US'].intent_samples)
+
+    def test_template_all_malformed_rejected(self):
+        pipeline = _pipeline()
+        with mock.patch('padacioso.opm.LOG.warning') as warn:
+            pipeline.handle_register_template(
+                Message('ovos.intent.register.template',
+                        {'skill_id': 'skill-persona.openvoiceos',
+                         'intent_name': 'cancel',
+                         'lang': 'en-US',
+                         'samples': ['{utterance}']}))
+        self.assertTrue(warn.called)
+        name = pipeline._internal_name('skill-persona.openvoiceos', 'cancel')
+        self.assertNotIn(name, pipeline.containers['en-US'].intent_samples)
+        self.assertNotIn(name, pipeline.registered_intents)
+
+    def test_entity_malformed_sample_skipped(self):
+        pipeline = _pipeline()
+        with mock.patch('padacioso.opm.LOG.warning') as warn:
+            pipeline.handle_register_entity(
+                Message('ovos.entity.register',
+                        {'skill_id': 'skill-persona.openvoiceos',
+                         'entity_name': 'thing',
+                         'lang': 'en-US',
+                         'samples': ['broken {value', 'good value']}))
+        self.assertEqual(warn.call_count, 1)
+        name = pipeline._internal_name('skill-persona.openvoiceos', 'thing')
+        self.assertIn(name, pipeline.containers['en-US'].entity_samples)
+
+
+if __name__ == '__main__':
+    unittest.main()