fix: use shlex.split() in base_world.check_requirement() to handle quoted arguments#3330
Open
deacon-mp wants to merge 2 commits into
Open
fix: use shlex.split() in base_world.check_requirement() to handle quoted arguments#3330deacon-mp wants to merge 2 commits into
deacon-mp wants to merge 2 commits into
Conversation
Replace command.split(' ') with shlex.split(command) to properly
handle commands with quoted arguments and spaces in paths.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves BaseWorld.check_requirement() command parsing by replacing naive string splitting with shlex.split() so quoted arguments (and paths with spaces) are handled correctly, and adds tests covering these cases.
Changes:
- Use
shlex.split(command)before callingsubprocess.check_output(...)for installed program checks. - Add pytest cases validating subprocess arguments for simple, quoted, and space-containing commands.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| app/utility/base_world.py | Switches command tokenization to shlex.split() for correct handling of quoted args. |
| tests/test_shlex_split.py | Adds tests asserting the argument vector passed to subprocess.check_output for quoted/path-with-spaces scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def check_program_version(command, version, **kwargs): | ||
| output = subprocess.check_output(command.split(' '), stderr=subprocess.STDOUT, shell=False, timeout=10) | ||
| output = subprocess.check_output(shlex.split(command), stderr=subprocess.STDOUT, shell=False, timeout=10) |
| class TestShlexSplit: | ||
| def test_simple_command(self): | ||
| params = {'type': 'installed_program', 'command': 'python3 --version', 'version': '3.0'} | ||
| with patch('subprocess.check_output', return_value=b'Python 3.12.0') as mock: |
|
|
||
| def test_command_with_quotes(self): | ||
| params = {'type': 'installed_program', 'command': 'echo "hello world"', 'version': '0.0.0'} | ||
| with patch('subprocess.check_output', return_value=b'1.0.0') as mock: |
|
|
||
| def test_command_with_spaces_in_path(self): | ||
| params = {'type': 'installed_program', 'command': "'/path/to/my program' --version", 'version': '1.0'} | ||
| with patch('subprocess.check_output', return_value=b'1.5.0') as mock: |
Comment on lines
+19
to
+28
| BaseWorld.check_requirement(params) | ||
| args = mock.call_args[0][0] | ||
| assert args == ['echo', 'hello world'] | ||
|
|
||
| def test_command_with_spaces_in_path(self): | ||
| params = {'type': 'installed_program', 'command': "'/path/to/my program' --version", 'version': '1.0'} | ||
| with patch('subprocess.check_output', return_value=b'1.5.0') as mock: | ||
| BaseWorld.check_requirement(params) | ||
| args = mock.call_args[0][0] | ||
| assert args == ['/path/to/my program', '--version'] |
- Catch ValueError from shlex.split() (unmatched quotes) and return False with a log message instead of propagating an unhandled exception - Patch app.utility.base_world.subprocess.check_output in tests (not the global subprocess) so patches are resilient to import order - Add return-value assertions to all three existing test cases - Add test for unmatched-quote ValueError returning False without calling subprocess - Add test for version below minimum returning False
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
check_requirement() split command string naively. Used shlex.split() for proper handling of quoted args. With tests.