diff --git a/.github/workflows/build_tests.yml b/.github/workflows/build_tests.yml index 173d2d25..6764d4b7 100644 --- a/.github/workflows/build_tests.yml +++ b/.github/workflows/build_tests.yml @@ -51,4 +51,6 @@ jobs: with: # Ignore setuptools vulnerability we can't do much about ignore-vulns: | - GHSA-r9hx-vwmv-q579 \ No newline at end of file + GHSA-r9hx-vwmv-q579 + PYSEC-2023-74 + PYSEC-2022-43012 \ No newline at end of file diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index 39bbf2d3..5e97e0e7 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -33,7 +33,7 @@ jobs: run: | pip install pytest pip install pytest-cov - pytest --cov=./test/unittests --cov-report=xml + pytest --cov=./ovos_workshop --cov-report=xml - name: Upload coverage to Codecov uses: codecov/codecov-action@v3 with: diff --git a/.github/workflows/unit_tests.yml b/.github/workflows/unit_tests.yml index eb8a113c..91600b7d 100644 --- a/.github/workflows/unit_tests.yml +++ b/.github/workflows/unit_tests.yml @@ -61,18 +61,6 @@ jobs: # NOTE: additional pytest invocations should also add the --cov-append flag # or they will overwrite previous invocations' coverage reports # (for an example, see OVOS Skill Manager's workflow) - - name: Replace ovos-core with mycroft-core - run: | - pip uninstall ovos-core -y - pip uninstall ovos-lingua-franca -y - pip install git+https://github.com/MycroftAI/mycroft-core - pip install . - - name: Run mycroft unittests - run: | - pytest --cov-append --cov=ovos_workshop --cov-report xml test/unittests - # NOTE: additional pytest invocations should also add the --cov-append flag - # or they will overwrite previous invocations' coverage reports - # (for an example, see OVOS Skill Manager's workflow) - name: Upload coverage env: CODECOV_TOKEN: ${{secrets.CODECOV_TOKEN}} diff --git a/ovos_workshop/skills/base.py b/ovos_workshop/skills/base.py index ec86d828..e3121776 100644 --- a/ovos_workshop/skills/base.py +++ b/ovos_workshop/skills/base.py @@ -200,30 +200,6 @@ class BaseSkill: bus (MycroftWebsocketClient): Optional bus connection """ - def __new__(cls, *args, **kwargs): - if "skill_id" in kwargs and "bus" in kwargs: - skill_id = kwargs["skill_id"] - bus = kwargs["bus"] - try: - # skill follows latest best practices, accepts kwargs and does its own init - return super().__new__(cls, skill_id=skill_id, bus=bus) - except Exception as e: - LOG.info(e) - try: - # skill did not update its init method, let's do some magic to init it manually - skill = super().__new__(cls, *args, **kwargs) - skill._startup(bus, skill_id) - return skill - except Exception as e: - LOG.info(e) - - # skill loader was not used to create skill object, log a warning and - # do the legacy init - LOG.warning(f"{cls.__name__} not fully inited, self.bus and " - f"self.skill_id will only be available in self.initialize. " - f"Pass kwargs `skill_id` and `bus` to resolve this.") - return super().__new__(cls) - def __init__(self, name=None, bus=None, resources_dir=None, settings: JsonStorage = None, gui=None, enable_settings_manager=True, diff --git a/ovos_workshop/skills/mycroft_skill.py b/ovos_workshop/skills/mycroft_skill.py index b86a8c83..92d5dd23 100644 --- a/ovos_workshop/skills/mycroft_skill.py +++ b/ovos_workshop/skills/mycroft_skill.py @@ -14,16 +14,87 @@ # """Common functionality relating to the implementation of mycroft skills.""" +import inspect import shutil from abc import ABCMeta -from os.path import join, exists +from os.path import join, exists, dirname from ovos_utils.log import LOG + from ovos_workshop.skills.base import BaseSkill, is_classic_core class _SkillMetaclass(ABCMeta): - """ To override isinstance checks we need to use a metaclass """ + """ + this metaclass ensures we can load skills like regular python objects + mycroft-core required a skill loader helper class, which created the skill and then finished object init + this means skill_id and bus are not available in init method, mycroft introduced a method named initialize meant for this + + to make skills pythonic and standalone, this metaclass is used to auto init old skills and help in migrating to new standards + + To override isinstance checks we also need to use a metaclass + + TODO: remove compat ovos-core 0.2.0, including MycroftSkill class + """ + + def __call__(cls, *args, **kwargs): + from ovos_bus_client import MessageBusClient + from ovos_utils.messagebus import FakeBus + bus = None + skill_id = None + + if "bus" not in kwargs: + for a in args: + if isinstance(a, MessageBusClient) or isinstance(a, FakeBus): + bus = a + LOG.warning(f"bus should be a kwarg, guessing {a} is the bus") + break + else: + LOG.warning("skill initialized without bus!! this is legacy behaviour and" + " requires you to call skill.bind(bus) or skill._startup(skill_id, bus)\n" + "bus will be required starting on ovos-core 0.1.0") + return super().__call__(*args, **kwargs) + + if "skill_id" in kwargs: + skill_id = kwargs.pop("skill_id") + if "bus" in kwargs: + bus = kwargs.pop("bus") + if not skill_id: + LOG.warning(f"skill_id should be a kwarg, please update {cls.__name__}") + if args and isinstance(args[0], str): + a = args[0] + if a[0].isupper(): # in mycroft name is CamelCase by convention, not skill_id + LOG.debug(f"ambiguous skill_id, ignoring {a} as it appears to be a CamelCase name") + else: + LOG.warning(f"ambiguous skill_id, assuming positional argument: {a}") + skill_id = a + + if not skill_id: + LOG.warning("skill initialized without skill_id!! this is legacy behaviour and" + " requires you to call skill._startup(skill_id, bus)\n" + "skill_id will be required starting on ovos-core 0.1.0") + return super().__call__(*args, **kwargs) + + # by convention skill_id is the folder name + # usually repo.author + # TODO - uncomment once above is deprecated + #skill_id = dirname(inspect.getfile(cls)).split("/")[-1] + #LOG.warning(f"missing skill_id, assuming folder name convention: {skill_id}") + + try: + # skill follows latest best practices, accepts kwargs and does its own init + return super().__call__(skill_id=skill_id, bus=bus, **kwargs) + except TypeError: + LOG.warning("legacy skill signature detected, attempting to init skill manually, " + f"self.bus and self.skill_id will only be available in self.initialize.\n" + + f"__init__ method needs to accept `skill_id` and `bus` to resolve this.") + + # skill did not update its init method, let's do some magic to init it manually + # NOTE: no try: except because all skills must accept this initialization and we want exception + # this is what skill loader does internally + skill = super().__call__(*args, **kwargs) + skill._startup(bus, skill_id) + return skill def __instancecheck__(self, instance): if is_classic_core(): diff --git a/test/unittests/skills/test_mycroft_skill.py b/test/unittests/skills/test_mycroft_skill.py index 071beff2..32d87fd6 100644 --- a/test/unittests/skills/test_mycroft_skill.py +++ b/test/unittests/skills/test_mycroft_skill.py @@ -20,8 +20,8 @@ from os.path import join, dirname, abspath from unittest.mock import MagicMock, patch -import pytest -from adapt.intent import IntentBuilder +#import pytest +from ovos_utils.intents import IntentBuilder from ovos_bus_client import Message from ovos_config.config import Configuration @@ -247,7 +247,7 @@ def check_register_decorators(self, result_list): sorted(result_list, key=lambda d: sorted(d.items()))) self.emitter.reset() - @pytest.mark.skip + #@pytest.mark.skip def test_register_decorators(self): """ Test decorated intents """ path_orig = sys.path diff --git a/test/unittests/test_skill.py b/test/unittests/test_skill.py index 801596e6..8b410561 100644 --- a/test/unittests/test_skill.py +++ b/test/unittests/test_skill.py @@ -12,6 +12,66 @@ from ovos_workshop.skill_launcher import SkillLoader +class LegacySkill(CoreSkill): + def __init__(self, skill_name="LegacySkill", bus=None, **kwargs): + self.inited = True + self.initialized = False + self.startup_called = False + super().__init__(skill_name, bus, **kwargs) + # __new__ calls `_startup` so this should be defined in __init__ + assert self.skill_id is not None + + def initialize(self): + self.initialized = True + + def _startup(self, bus, skill_id=""): + self.startup_called = True + self.initialize() + + +class BadLegacySkill(LegacySkill): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + print(self.bus) # not set, exception in property + + +class GoodLegacySkill(CoreSkill): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + print(self.bus) # maybe not set, exception in property + + +class SpecificArgsSkill(OVOSSkill): + def __init__(self, skill_id="SpecificArgsSkill", bus=None, **kwargs): + self.inited = True + self.initialized = False + self.startup_called = False + super().__init__(skill_id=skill_id, bus=bus, **kwargs) + self.kwargs = kwargs + + def initialize(self): + self.initialized = True + + def _startup(self, bus, skill_id=""): + self.startup_called = True + self.initialize() + + +class KwargSkill(OVOSSkill): + def __init__(self, **kwargs): + self.inited = True + self.initialized = False + self.startup_called = False + super().__init__(**kwargs) + + def initialize(self): + self.initialized = True + + def _startup(self, bus, skill_id=""): + self.startup_called = True + self.initialize() + + class TestSkill(unittest.TestCase): def setUp(self): self.bus = FakeBus() @@ -101,3 +161,49 @@ def test_stop(self): def tearDown(self) -> None: self.skill.unload() + + +class TestSkillNew(unittest.TestCase): + def test_legacy(self): + bus = FakeBus() + + # a legacy skill accepts wrong args, but accepts kwargs + legacy = LegacySkill("LegacyName", bus, skill_id="legacy.mycroft") + self.assertTrue(legacy.inited) + self.assertTrue(legacy.initialized) + self.assertTrue(legacy.startup_called) + self.assertIsNotNone(legacy.skill_id) + self.assertEqual(legacy.bus, bus) + + # a legacy skill not accepting args at all + with self.assertRaises(Exception) as ctxt: + BadLegacySkill() # accesses self.bus in __init__ + self.assertTrue("Accessed MycroftSkill.bus in __init__" in str(ctxt.exception)) + + legacynoargs = LegacySkill() # no exception this time because bus is not used in init + self.assertTrue(legacynoargs.inited) + self.assertFalse(legacynoargs.initialized) + self.assertFalse(legacynoargs.startup_called) + + # a legacy skill fully inited at once + legacy = GoodLegacySkill(skill_id="legacy.mycroft", bus=bus) # accesses self.bus in __init__ + self.assertEqual(legacy.skill_id, "legacy.mycroft") + self.assertEqual(legacy.bus, bus) + + def test_load(self): + bus = FakeBus() + kwarg = KwargSkill(skill_id="kwarg", bus=bus) + self.assertTrue(kwarg.inited) + self.assertTrue(kwarg.initialized) + self.assertTrue(kwarg.startup_called) + self.assertEqual(kwarg.skill_id, "kwarg") + self.assertEqual(kwarg.bus, bus) + + gui = Mock() + args = SpecificArgsSkill("args", bus, gui=gui) + self.assertTrue(args.inited) + self.assertTrue(args.initialized) + self.assertTrue(args.startup_called) + self.assertEqual(args.skill_id, "args") + self.assertEqual(args.bus, bus) + self.assertEqual(args.gui, gui) \ No newline at end of file