Skip to content

Commit 98e0f2c

Browse files
JarbasAlclaude
andcommitted
fix: detach_skill removes intents by skill_id prefix, not substring
handle_detach_skill selected intents to remove with `skill_id in i`, a substring test over the full "<skill_id>:<name>" intent name. A skill whose id is a substring of another skill's id (e.g. "me" against "skill-a.me") wiped the other skill's intents when it detached, and an unregistered id like "skill" cleared every intent containing that text. The entity branch of the same handler already matched on the "<skill_id>:" prefix, as do adapt and padatious. Intents are now selected with startswith(skill_id + ":"), the colon being the intent-name namespace separator (OVOS-MSG-1 §2.1.1), so the owner of "a.b:c" is exactly "a.b". Regression test test/test_detach_skill_prefix.py: 2 failed before the fix (registered_intents emptied to [] in both cases), 2 pass after. Full suite: 90 passed, 2 skipped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 parent e063f0c commit 98e0f2c

2 files changed

Lines changed: 46 additions & 3 deletions

File tree

padacioso/opm.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -304,12 +304,12 @@ def handle_detach_skill(self, message):
304304
message (Message): message triggering action
305305
"""
306306
skill_id = message.data['skill_id']
307-
remove_list = [i for i in self.registered_intents if skill_id in i]
307+
prefix = skill_id + ":"
308+
remove_list = [i for i in self.registered_intents if i.startswith(prefix)]
308309
for i in remove_list:
309310
self.__detach_intent(i)
310-
skill_id_colon = skill_id + ":"
311311
for en in self.registered_entities:
312-
if en["name"].startswith(skill_id_colon):
312+
if en["name"].startswith(prefix):
313313
self.__detach_entity(en["name"], en["lang"])
314314

315315
def _valid_samples(self, samples, topic, name, lang):

test/test_detach_skill_prefix.py

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
"""``detach_skill`` removes only the intents the named skill owns. The owning
2+
skill of ``a.b:c`` is exactly ``a.b`` (colon namespace separator, OVOS-MSG-1
3+
§2.1.1), so a skill whose id is a substring of another skill's id must not
4+
take the other skill's intents down with it."""
5+
import unittest
6+
7+
from ovos_bus_client.message import Message
8+
from ovos_utils.fakebus import FakeBus
9+
10+
from padacioso.opm import PadaciosoPipeline
11+
12+
13+
class TestDetachSkillPrefixMatch(unittest.TestCase):
14+
def setUp(self):
15+
self.p = PadaciosoPipeline(FakeBus(), {"any": 1})
16+
self.lang = self.p.lang
17+
18+
def _register(self, name):
19+
self.p.register_intent(Message(
20+
"padatious:register_intent",
21+
{"name": name, "samples": ["hello " + name.split(":")[1]],
22+
"lang": self.lang}))
23+
24+
def test_substring_skill_id_keeps_other_skills(self):
25+
for name in ("skill-a.me:one", "skill-b.me:bone", "me:mine"):
26+
self._register(name)
27+
self.p.handle_detach_skill(Message("detach_skill", {"skill_id": "me"}))
28+
self.assertEqual(sorted(self.p.registered_intents),
29+
["skill-a.me:one", "skill-b.me:bone"])
30+
names = set(self.p.containers[self.lang].intent_samples)
31+
self.assertIn("skill-a.me:one", names)
32+
self.assertIn("skill-b.me:bone", names)
33+
self.assertNotIn("me:mine", names)
34+
35+
def test_unregistered_substring_skill_id_removes_nothing(self):
36+
for name in ("victim.skill:on", "otherskill:on"):
37+
self._register(name)
38+
self.p.handle_detach_skill(Message("detach_skill", {"skill_id": "skill"}))
39+
self.assertEqual(sorted(self.p.registered_intents),
40+
["otherskill:on", "victim.skill:on"])
41+
names = set(self.p.containers[self.lang].intent_samples)
42+
self.assertEqual(names & {"victim.skill:on", "otherskill:on"},
43+
{"victim.skill:on", "otherskill:on"})

0 commit comments

Comments
 (0)