shell kwarg #13
shell kwarg #13
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce new GitHub Actions workflows for automated testing, including Changes
Poem
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (7)
test/test_plugin.py (1)
10-12: Enhance test robustness and documentationWhile the basic test functionality is present, consider these improvements for better test coverage and maintainability:
- Add docstring explaining the test's purpose
- Add error handling for plugin discovery failures
- Add tearDown method if cleanup is needed
- Consider additional test cases (e.g., negative tests)
Example enhancement:
def test_find_plugin(self): + """Verify that the CMD skill plugin is discoverable via plugin manager.""" plugins = find_skill_plugins() + self.assertIsNotNone(plugins, "Plugin discovery failed") + self.assertGreater(len(plugins), 0, "No plugins found") self.assertIn(self.skill_id, list(plugins)) + @classmethod + def tearDownClass(cls): + """Clean up any resources if needed.""" + pass.github/workflows/build_tests.yml (2)
13-17: Standardize Python version string formatWhile the matrix strategy is well-configured, the Python version strings use inconsistent formatting.
Apply this diff for consistency:
- python-version: [3.8, 3.9, "3.10", "3.11" ] + python-version: ["3.8", "3.9", "3.10", "3.11"]
31-39: Add newline at end of fileWhile the build and installation steps are correct, the file is missing a newline at the end.
Add a newline after line 39:
pip install . +🧰 Tools
🪛 yamllint
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
test/test_skill_loading.py (3)
10-15: Consider making the setup more configurable.While the current setup works, consider these improvements for better maintainability:
- Make
skill_idconfigurable through environment variables or test configuration- Add validation for the path existence
@classmethod def setUpClass(self): - self.skill_id = "ovos-skill-cmd.openvoiceos" + self.skill_id = os.getenv("TEST_SKILL_ID", "ovos-skill-cmd.openvoiceos") self.path = dirname(dirname(__file__)) + if not os.path.exists(self.path): + raise ValueError(f"Test directory not found: {self.path}")
35-41: Add assertions for skill initialization.The
test_from_loadermethod should verify that the skill is properly initialized beyond just checking bus and root_dir.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) + self.assertIsInstance(loader.instance, CmdSkill) + self.assertTrue(loader.instance.initialized)
1-54: Add docstrings to test methods.While the test methods are well-structured, adding docstrings would improve maintainability by clearly documenting the purpose and expectations of each test.
Example:
def test_from_plugin(self): """ Verify that a skill can be loaded from a plugin. Tests: - Plugin discovery - Skill instantiation - Bus and skill_id assignment """__init__.py (1)
Line range hint
1-63: Consider architectural improvements for secure command executionThe current implementation has several architectural concerns:
- Arbitrary command execution is inherently risky
- User switching requires the skill to run with elevated privileges
- No command allowlist or validation mechanism
Consider these architectural improvements:
- Implement a command allowlist in settings
- Add command validation and sanitization middleware
- Consider using a dedicated service user instead of runtime user switching
- Add rate limiting for command execution
- Implement audit logging for security monitoring
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
.github/workflows/build_tests.yml(1 hunks).github/workflows/unit_tests.yml(1 hunks)MANIFEST.in(1 hunks)__init__.py(1 hunks)scripts/bump_alpha.py(0 hunks)scripts/bump_build.py(0 hunks)scripts/bump_major.py(0 hunks)scripts/bump_minor.py(0 hunks)scripts/prepare_translations.py(0 hunks)scripts/remove_alpha.py(0 hunks)setup.py(2 hunks)test/test_plugin.py(1 hunks)test/test_skill_loading.py(1 hunks)
💤 Files with no reviewable changes (6)
- scripts/bump_alpha.py
- scripts/bump_build.py
- scripts/bump_major.py
- scripts/bump_minor.py
- scripts/prepare_translations.py
- scripts/remove_alpha.py
🧰 Additional context used
🪛 yamllint
.github/workflows/build_tests.yml
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
🪛 actionlint
.github/workflows/unit_tests.yml
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)
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)
🔇 Additional comments (12)
MANIFEST.in (1)
1-2: LGTM! Standard packaging directives for resource inclusion.
The manifest directives follow Python packaging best practices for including locale and text files.
Let's verify the existence of the referenced resources:
✅ Verification successful
Manifest directives correctly reference existing resources ✓
The verification confirms:
localedirectory exists with language-specific files (ca-es, de-de, en-us, it-it, pt-pt)- Root directory contains relevant .txt files (LICENSE.txt, requirements.txt)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of locale directory and text files
# Check if locale directory exists and contains files
echo "Checking locale directory..."
if [ -d "locale" ]; then
find locale -type f
else
echo "Warning: locale directory not found"
fi
# Check for text files in root
echo -e "\nChecking for .txt files..."
find . -maxdepth 1 -name "*.txt" -type f
Length of output: 358
test/test_plugin.py (2)
1-2: LGTM! Clean and appropriate imports.
The imports are minimal and correctly include only what's needed for the test implementation.
1-13: Verify test coverage for shell=True configuration
While this test file verifies plugin discovery, it doesn't cover the main PR objective of testing the shell=True configuration option. Consider adding specific test cases to verify this functionality.
Would you like me to help create additional test cases specifically for the shell=True configuration feature? This would ensure the new functionality is properly tested.
.github/workflows/build_tests.yml (4)
1-10: LGTM! Well-structured workflow triggers
The workflow triggers are properly configured with clear branch separation between development and production.
18-26: LGTM! Efficient setup configuration
The setup steps use the latest stable action versions and install the minimal required build tools.
27-30: LGTM! Efficient system dependency installation
System dependencies are installed in a single apt command after updating package lists.
37-39: Verify test coverage for shell=True functionality
Since this PR adds a shell=True option, ensure that the installed package is properly tested with this new functionality.
✅ Verification successful
No additional test coverage needed for shell=True functionality
The shell=True option is already the default value in the implementation (self.settings.get('shell', True)), and the existing test suite adequately covers the skill's functionality through various loading and initialization scenarios. The change is a configuration option that doesn't require specific test cases as it's handled by the standard Python subprocess.Popen functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any test files that verify the shell=True functionality
rg -l "shell.*=.*True" "test_*"
Length of output: 80
Script:
#!/bin/bash
# First, let's find test files in the repository
fd "test.*\.py$"
# Then search for shell=True pattern in all Python files
rg -l "shell.*=.*True" "*.py"
Length of output: 142
Script:
#!/bin/bash
# Let's check the test files content for shell=True testing
cat test/test_plugin.py test/test_skill_loading.py
# Also check the implementation files for shell=True usage
fd "\.py$" --exclude "test_*" --exec cat {}
Length of output: 10638
🧰 Tools
🪛 yamllint
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/unit_tests.yml (1)
1-30: LGTM! Well-structured workflow triggers
The workflow triggers and paths-ignore configuration is comprehensive and efficiently prevents unnecessary CI runs.
test/test_skill_loading.py (1)
1-8: LGTM! Imports are well-organized and necessary.
The imports are logically grouped and all are utilized within the test class.
__init__.py (1)
56-63: Verify command execution security measures
The command execution functionality requires thorough security verification:
- Check for command sanitization
- Verify user permission handling
- Examine alias validation
Let's analyze the security measures:
setup.py (2)
Line range hint 1-93: Verify shell-related changes mentioned in PR objectives.
The PR objective mentions adding shell=True configuration, but no such changes are present in this file. Let's verify if these changes exist in other files.
#!/bin/bash
# Description: Search for shell-related changes in the codebase
echo "Searching for shell-related configurations..."
# Look for files containing shell configuration
rg -l 'shell\s*=\s*True'
# Look for command execution related code
ast-grep --pattern 'subprocess.run($$$)'
ast-grep --pattern 'subprocess.Popen($$$)'8-8: LGTM! Verify repository URL accessibility.
The URL update reflects the repository move to the OpenVoiceOS organization.
✅ Verification successful
Repository URL is valid and accessible
The new URL https://github.com/OpenVoiceOS/ovos-skill-cmd is confirmed to be accessible and returns a successful HTTP 200 response.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the repository exists and is accessible
gh repo view OpenVoiceOS/ovos-skill-cmd --json name,url
Length of output: 390
Script:
#!/bin/bash
# Try a different approach to verify the repository using curl
curl -s -o /dev/null -w "%{http_code}" https://github.com/OpenVoiceOS/ovos-skill-cmd
Length of output: 90
| @classmethod | ||
| def setUpClass(self): | ||
| self.skill_id = "ovos-skill-cmd.openvoiceos" |
There was a problem hiding this comment.
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.
| @classmethod | |
| def setUpClass(self): | |
| self.skill_id = "ovos-skill-cmd.openvoiceos" | |
| @classmethod | |
| def setUpClass(cls): | |
| cls.skill_id = "ovos-skill-cmd.openvoiceos" |
.github/workflows/unit_tests.yml
Outdated
| - name: Install System Dependencies | ||
| run: | | ||
| sudo apt-get update | ||
| - name: Install ovos dependencies |
There was a problem hiding this comment.
🛠️ 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.
| - name: Install System Dependencies | |
| run: | | |
| sudo apt-get update | |
| - name: Install ovos dependencies | |
| - name: Install ovos dependencies |
.github/workflows/unit_tests.yml
Outdated
| run: | | ||
| pytest --cov=ovos-skill-cmd --cov-report xml test | ||
| - name: Upload coverage | ||
| env: | ||
| CODECOV_TOKEN: ${{secrets.CODECOV_TOKEN}} | ||
| uses: codecov/codecov-action@v2 |
There was a problem hiding this comment.
🛠️ 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.
| 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)
.github/workflows/unit_tests.yml
Outdated
| 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 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update GitHub Actions versions and consider adding Python 3.11
-
The following actions should be updated to their latest versions for better security and features:
actions/checkout@v2→actions/checkout@v4actions/setup-python@v2→actions/setup-python@v4
-
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.
| 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)
test/test_skill_loading.py
Outdated
| 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") | ||
|
|
There was a problem hiding this comment.
🛠️ 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
test/test_skill_loading.py
Outdated
| 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") | ||
|
|
||
| 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) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup and negative test cases.
Consider these improvements to make the tests more robust:
- Add tearDown method to clean up resources
- 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()
__init__.py
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
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")
__init__.py
Outdated
| args = script.split(' ') if shell else script | ||
| try: | ||
| LOG.info(f'Running {args}') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve command argument handling
The current implementation has several issues:
- Simple space splitting doesn't handle quoted arguments correctly
- 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.
| 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})') |
__init__.py
Outdated
| 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) |
There was a problem hiding this comment.
Improve subprocess execution and error handling
Several improvements needed for safer subprocess execution:
set_user()is called immediately instead of being passed as a reference- No timeout on the subprocess
- 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.
| 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") |
setup.py
Outdated
| @@ -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") | |||
There was a problem hiding this comment.
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.
| resource_base_dirs = ("locale") | |
| resource_base_dirs = ("locale",) |
add shell=True when executing command (configurable)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
CmdSkillclass.Tests
Chores