Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shell kwarg #13

Merged
merged 2 commits into from
Nov 15, 2024
Merged

shell kwarg #13

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fdba65f
fix: shell arg + automations
JarbasAl Nov 15, 2024
65ca09a
readme
JarbasAl Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions .github/workflows/build_tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
name: Run Build Tests
on:
push:
branches:
- master
pull_request:
branches:
- dev
workflow_dispatch:

jobs:
build_tests:
strategy:
max-parallel: 2
matrix:
python-version: [3.8, 3.9, "3.10", "3.11" ]
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Setup Python
uses: actions/setup-python@v4
with:
python-version: ${{ matrix.python-version }}
- name: Install Build Tools
run: |
python -m pip install build wheel
- name: Install System Dependencies
run: |
sudo apt-get update
sudo apt install python3-dev swig libssl-dev
- name: Build Source Packages
run: |
python setup.py sdist
- name: Build Distribution Packages
run: |
python setup.py bdist_wheel
- name: Install skill
run: |
pip install .
66 changes: 66 additions & 0 deletions .github/workflows/unit_tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
name: Run UnitTests
on:
pull_request:
branches:
- dev
paths-ignore:
- 'version.py'
- 'examples/**'
- '.github/**'
- '.gitignore'
- 'LICENSE'
- 'CHANGELOG.md'
- 'MANIFEST.in'
- 'README.md'
- 'scripts/**'
push:
branches:
- master
paths-ignore:
- 'version.py'
- 'examples/**'
- '.github/**'
- '.gitignore'
- 'LICENSE'
- 'CHANGELOG.md'
- 'MANIFEST.in'
- 'README.md'
- 'scripts/**'
workflow_dispatch:

jobs:
unit_tests:
strategy:
matrix:
python-version: [3.9, "3.10" ]
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: Set up python ${{ matrix.python-version }}
uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python-version }}
- name: Install System Dependencies
run: |
sudo apt-get update
sudo apt install python3-dev
python -m pip install build wheel
- name: Install core repo
Comment on lines +31 to +48
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Update GitHub Actions versions and consider adding Python 3.11

  1. The following actions should be updated to their latest versions for better security and features:

    • actions/checkout@v2 → actions/checkout@v4
    • actions/setup-python@v2 → actions/setup-python@v4

  2. Consider adding Python 3.11 to the matrix for future compatibility.

Apply this diff:

       strategy:
         matrix:
-          python-version: [3.9, "3.10" ]
+          python-version: [3.9, "3.10", "3.11"]
       runs-on: ubuntu-latest
       steps:
-        - uses: actions/checkout@v2
+        - uses: actions/checkout@v4
         - name: Set up python ${{ matrix.python-version }}
-          uses: actions/setup-python@v2
+          uses: actions/setup-python@v4
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
jobs:
unit_tests:
strategy:
matrix:
python-version: [3.9, "3.10" ]
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: Set up python ${{ matrix.python-version }}
uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python-version }}
- name: Install System Dependencies
run: |
sudo apt-get update
sudo apt install python3-dev
python -m pip install build wheel
- name: Install core repo
jobs:
unit_tests:
strategy:
matrix:
python-version: [3.9, "3.10", "3.11"]
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Set up python ${{ matrix.python-version }}
uses: actions/setup-python@v4
with:
python-version: ${{ matrix.python-version }}
- name: Install System Dependencies
run: |
sudo apt-get update
sudo apt install python3-dev
python -m pip install build wheel
- name: Install core repo
🧰 Tools
🪛 actionlint

38-38: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

40-40: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

run: |
pip install .
- name: Install test dependencies
run: |
pip install pytest pytest-timeout pytest-cov
- name: Install System Dependencies
run: |
sudo apt-get update
- name: Install ovos dependencies
Comment on lines +54 to +57
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove duplicate system dependencies step

This step is redundant as it duplicates the system dependencies installation from lines 43-47, but without actually installing any packages.

Remove these lines as they are already covered by the previous system dependencies installation step.

-      - name: Install System Dependencies
-        run: |
-          sudo apt-get update
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Install System Dependencies
run: |
sudo apt-get update
- name: Install ovos dependencies
- name: Install ovos dependencies

run: |
pip install ovos-plugin-manager
- name: Run unittests
run: |
pytest --cov=ovos-skill-cmd --cov-report xml test
- name: Upload coverage
env:
CODECOV_TOKEN: ${{secrets.CODECOV_TOKEN}}
uses: codecov/codecov-action@v2
Comment on lines +61 to +66
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Update Codecov action version

The Codecov action should be updated to the latest version for better security and features.

Apply this diff:

         env:
           CODECOV_TOKEN: ${{secrets.CODECOV_TOKEN}}
-        uses: codecov/codecov-action@v2
+        uses: codecov/codecov-action@v3
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
run: |
pytest --cov=ovos-skill-cmd --cov-report xml test
- name: Upload coverage
env:
CODECOV_TOKEN: ${{secrets.CODECOV_TOKEN}}
uses: codecov/codecov-action@v2
run: |
pytest --cov=ovos-skill-cmd --cov-report xml test
- name: Upload coverage
env:
CODECOV_TOKEN: ${{secrets.CODECOV_TOKEN}}
uses: codecov/codecov-action@v3
🧰 Tools
🪛 actionlint

66-66: the runner of "codecov/codecov-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

2 changes: 2 additions & 0 deletions MANIFEST.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
recursive-include locale *
include *.txt
9 changes: 5 additions & 4 deletions __init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,13 @@ def initialize(self):
def run(self, message):
script = message.data.get('Script')
script = self.alias.get(script, script)
args = script.split(' ')
shell = self.settings.get('shell', True)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security: Reconsider defaulting shell=True

Setting shell=True by default is a significant security risk as it enables shell injection vulnerabilities. When shell=True, the command is interpreted by the shell, allowing for command chaining, piping, and other shell features that could be exploited.

Suggested fix:

-        shell = self.settings.get('shell', True)
+        shell = self.settings.get('shell', False)

Consider adding a warning log when shell=True is enabled:

if shell:
    LOG.warning("Shell execution enabled - ensure command input is trusted")

args = script.split(' ') if shell else script
try:
LOG.info(f'Running {args}')
Comment on lines +57 to +59
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve command argument handling

The current implementation has several issues:

  1. Simple space splitting doesn't handle quoted arguments correctly
  2. The logging statement should show the actual command that will be executed

Consider using shlex.split() for proper argument parsing:

-        args = script.split(' ') if shell else script
+        import shlex
+        args = script if shell else shlex.split(script)
-        LOG.info(f'Running {args}')
+        LOG.info(f'Running command: {script} (shell={shell})')
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
args = script.split(' ') if shell else script
try:
LOG.info(f'Running {args}')
import shlex
args = script if shell else shlex.split(script)
try:
LOG.info(f'Running command: {script} (shell={shell})')

if self.uid and self.gid:
subprocess.Popen(args, preexec_fn=set_user(self.uid, self.gid))
subprocess.Popen(args, preexec_fn=set_user(self.uid, self.gid), shell=shell)
else:
LOG.info(f'Running {args}')
subprocess.Popen(args)
subprocess.Popen(args, shell=shell)
Comment on lines +61 to +63
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve subprocess execution and error handling

Several improvements needed for safer subprocess execution:

  1. set_user() is called immediately instead of being passed as a reference
  2. No timeout on the subprocess
  3. No output capture or handling

Suggested improvements:

-                subprocess.Popen(args, preexec_fn=set_user(self.uid, self.gid), shell=shell)
-            else:
-                subprocess.Popen(args, shell=shell)
+                process = subprocess.Popen(
+                    args,
+                    shell=shell,
+                    preexec_fn=lambda: set_user(self.uid, self.gid),
+                    stdout=subprocess.PIPE,
+                    stderr=subprocess.PIPE,
+                    text=True
+                )
+            else:
+                process = subprocess.Popen(
+                    args,
+                    shell=shell,
+                    stdout=subprocess.PIPE,
+                    stderr=subprocess.PIPE,
+                    text=True
+                )
+            try:
+                stdout, stderr = process.communicate(timeout=60)
+                if process.returncode != 0:
+                    LOG.error(f"Command failed with exit code {process.returncode}")
+                    LOG.error(f"stderr: {stderr}")
+                else:
+                    LOG.debug(f"Command output: {stdout}")
+            except subprocess.TimeoutExpired:
+                process.kill()
+                LOG.error("Command timed out after 60 seconds")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
subprocess.Popen(args, preexec_fn=set_user(self.uid, self.gid), shell=shell)
else:
LOG.info(f'Running {args}')
subprocess.Popen(args)
subprocess.Popen(args, shell=shell)
process = subprocess.Popen(
args,
shell=shell,
preexec_fn=lambda: set_user(self.uid, self.gid),
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True
)
else:
process = subprocess.Popen(
args,
shell=shell,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True
)
try:
stdout, stderr = process.communicate(timeout=60)
if process.returncode != 0:
LOG.error(f"Command failed with exit code {process.returncode}")
LOG.error(f"stderr: {stderr}")
else:
LOG.debug(f"Command output: {stdout}")
except subprocess.TimeoutExpired:
process.kill()
LOG.error("Command timed out after 60 seconds")

except Exception:
LOG.exception('Could not run script ' + script)
19 changes: 0 additions & 19 deletions scripts/bump_alpha.py
View file
Open in desktop

This file was deleted.

21 changes: 0 additions & 21 deletions scripts/bump_build.py
View file
Open in desktop

This file was deleted.

27 changes: 0 additions & 27 deletions scripts/bump_major.py
View file
Open in desktop

This file was deleted.

24 changes: 0 additions & 24 deletions scripts/bump_minor.py
View file
Open in desktop

This file was deleted.

53 changes: 0 additions & 53 deletions scripts/prepare_translations.py
View file
Open in desktop

This file was deleted.

13 changes: 0 additions & 13 deletions scripts/remove_alpha.py
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

# Define package information
SKILL_CLAZZ = "CmdSkill" # Make sure it matches __init__.py class name
URL = "https://github.com/OVOSHatchery/ovos-skill-cmd"
URL = "https://github.com/OpenVoiceOS/ovos-skill-cmd"
AUTHOR = "forslund"
EMAIL = ""
LICENSE = "Apache2.0"
Expand Down Expand Up @@ -37,7 +37,7 @@ def get_requirements(requirements_filename: str = "requirements.txt"):

# Function to find resource files
def find_resource_files():
resource_base_dirs = ("locale", "ui")
resource_base_dirs = ("locale")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix syntax error in resource_base_dirs tuple definition.

The current definition resource_base_dirs = ("locale") is a string, not a tuple. This will cause the walk() function to iterate over individual characters instead of directory paths.

Apply this fix:

-    resource_base_dirs = ("locale")
+    resource_base_dirs = ("locale",)  # Note the comma to make it a single-element tuple
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
resource_base_dirs = ("locale")
resource_base_dirs = ("locale",)

base_dir = abspath(dirname(__file__))
package_data = ["*.json"]
for res in resource_base_dirs:
Expand Down
13 changes: 13 additions & 0 deletions test/test_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import unittest
from ovos_plugin_manager.skills import find_skill_plugins


class TestPlugin(unittest.TestCase):
@classmethod
def setUpClass(self):
self.skill_id = "ovos-skill-cmd.openvoiceos"
Comment on lines +6 to +8
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix incorrect usage of self in setUpClass

The setUpClass method is a class method and should use cls instead of self. This is a critical issue as it breaks the intended class-level setup pattern.

Apply this fix:

    @classmethod
    def setUpClass(self):
-        self.skill_id = "ovos-skill-cmd.openvoiceos"
+        cls.skill_id = "ovos-skill-cmd.openvoiceos"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@classmethod
def setUpClass(self):
self.skill_id = "ovos-skill-cmd.openvoiceos"
@classmethod
def setUpClass(cls):
cls.skill_id = "ovos-skill-cmd.openvoiceos"


def test_find_plugin(self):
plugins = find_skill_plugins()
self.assertIn(self.skill_id, list(plugins))

54 changes: 54 additions & 0 deletions test/test_skill_loading.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import unittest
from os.path import dirname

from ovos_plugin_manager.skills import find_skill_plugins
from ovos_utils.messagebus import FakeBus
from ovos_workshop.skill_launcher import PluginSkillLoader, SkillLoader
from ovos_skill_cmd import CmdSkill


class TestSkillLoading(unittest.TestCase):
@classmethod
def setUpClass(self):
self.skill_id = "ovos-skill-cmd.openvoiceos"
self.path = dirname(dirname(__file__))

def test_from_class(self):
bus = FakeBus()
skill = CmdSkill()
skill._startup(bus, self.skill_id)
self.assertEqual(skill.bus, bus)
self.assertEqual(skill.skill_id, self.skill_id)

def test_from_plugin(self):
bus = FakeBus()
for skill_id, plug in find_skill_plugins().items():
if skill_id == self.skill_id:
skill = plug()
skill._startup(bus, self.skill_id)
self.assertEqual(skill.bus, bus)
self.assertEqual(skill.skill_id, self.skill_id)
break
else:
raise RuntimeError("plugin not found")

Comment on lines +23 to +34
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor duplicate plugin search logic.

The plugin search logic is duplicated in test_from_plugin and test_from_plugin_loader. Consider extracting this into a helper method.

+    def _find_plugin(self):
+        for skill_id, plug in find_skill_plugins().items():
+            if skill_id == self.skill_id:
+                return plug
+        raise RuntimeError("plugin not found")
+
     def test_from_plugin(self):
         bus = FakeBus()
-        for skill_id, plug in find_skill_plugins().items():
-            if skill_id == self.skill_id:
-                skill = plug()
-                skill._startup(bus, self.skill_id)
-                self.assertEqual(skill.bus, bus)
-                self.assertEqual(skill.skill_id, self.skill_id)
-                break
-        else:
-            raise RuntimeError("plugin not found")
+        plug = self._find_plugin()
+        skill = plug()
+        skill._startup(bus, self.skill_id)
+        self.assertEqual(skill.bus, bus)
+        self.assertEqual(skill.skill_id, self.skill_id)

Also applies to: 42-54

def test_from_loader(self):
bus = FakeBus()
loader = SkillLoader(bus, self.path)
loader.load()
self.assertEqual(loader.instance.bus, bus)
self.assertEqual(loader.instance.root_dir, self.path)

def test_from_plugin_loader(self):
bus = FakeBus()
loader = PluginSkillLoader(bus, self.skill_id)
for skill_id, plug in find_skill_plugins().items():
if skill_id == self.skill_id:
loader.load(plug)
break
else:
raise RuntimeError("plugin not found")

self.assertEqual(loader.skill_id, self.skill_id)
self.assertEqual(loader.instance.bus, bus)
self.assertEqual(loader.instance.skill_id, self.skill_id)
Comment on lines +16 to +54
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add cleanup and negative test cases.

Consider these improvements to make the tests more robust:

  1. Add tearDown method to clean up resources
  2. Add negative test cases to verify error handling

Example additions:

def tearDown(self):
    # Clean up any resources created during tests
    self.bus = None

def test_invalid_skill_id(self):
    bus = FakeBus()
    loader = PluginSkillLoader(bus, "invalid-skill-id")
    with self.assertRaises(RuntimeError):
        loader.load()

def test_invalid_path(self):
    bus = FakeBus()
    loader = SkillLoader(bus, "/invalid/path")
    with self.assertRaises(ValueError):
        loader.load()

Loading