fix(test): replace shell=True with shell=False in cli_coverage_steps.py subprocess call #1071
No reviewers
Labels
No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
cleveragents/cleveragents-core!1071
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/m3-shell-true-subprocess"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Replaced
shell=Truewithshell=Falseandshlex.split()for command tokenization infeatures/steps/cli_coverage_steps.py, addressing a security concern identified during the PR #727 review (Finding #3 by @hurui200320).This aligns with the safer pattern already used in
cli_plan_context_commands_steps.py:351and eliminates a potential command injection vector, following defense-in-depth principles.Changes
features/steps/cli_coverage_steps.py: Addedimport shlex, replacedshell=Truewithshlex.split(command)for proper command tokenizationfeatures/steps/androbot/helpers for othershell=Trueusages — no other Python subprocess calls foundQuality Gates
nox -s lintnox -s typechecknox -s unit_testsnox -s integration_testsnox -s coverage_reportCloses #734
Day 43 Review — PR #1071
fix(test): replace shell=True with shell=FalseMilestone: v3.2.0
Status: Mergeable (no conflicts)
Review Notes
This PR has been reviewed for compliance with
CONTRIBUTING.mdstandards. Key checks:Action Items
Closes #NNN)Please ensure all subtasks in the linked issue are complete before merging.
Code Review Report -- PR #1071 (Closes #734)
Reviewer: Automated Code Review
Branch:
fix/m3-shell-true-subprocessCommit:
1849c3d0(Luis Mendes)Scope:
features/steps/cli_coverage_steps.pydiff + close connections to surrounding codeMethodology: 3 global review cycles across all categories (Bug Detection, Security, Performance, Test Coverage, Test Flaws, Code Quality)
Summary
The change correctly replaces
shell=Truewithshell=False+shlex.split()in thestep_run_shell_commandfunction, eliminating a command injection vector identified in PR #727 (Finding #3 by @hurui200320). The fix is consistent with the reference pattern atcli_plan_context_commands_steps.py:351and all acceptance criteria from issue #734 are met.No Critical or High severity issues found. The change is correct, minimal, and well-scoped. Below are minor findings for consideration.
Findings by Category and Severity
1. Bug Detection
cli_coverage_steps.py:27"""Run a CLI command using subprocess shell."""but the function no longer uses a shell (shell=Truewas removed). Suggest updating to"""Run a CLI command as a subprocess."""or similar.cli_coverage_steps.py:44shlex.split()raisesValueErroron malformed quoted strings (e.g.,'echo "unclosed'). The genericexcept Exception as ehandler at line 62 catches this, but the resulting error message ("No closing quotation") provides no context about which command failed. Risk is very low since inputs originate from authored.featurefiles, not runtime user input.2. Security
cli_coverage_steps.py:43-44shell=Truecommand injection vector.shlex.split()properly tokenizes the command string into a list, which is passed directly tosubprocess.run()without shell interpretation. This is the correct defense-in-depth approach.features/steps/*.pyshell=Truein Python step files. All 87 subprocess references across 20 step files were audited. Every realsubprocess.run()call uses list-form commands withshelldefaulting toFalse. The codebase is clean.3. Performance
No issues found.
shlex.split()overhead is negligible for test step functions.4. Test Coverage
features/cli_coverage.feature:7When I run shell command "cleveragents --version") tests thestep_run_shell_commandfunction. This covers the primary path (cleveragents command with arguments), but not: the barecleveragentsbranch (line 38-39), non-cleveragents commands (the else-branch bypass at line 34), or edge cases throughshlex.split(). The test adequately validates the behavioral equivalence of the change but coverage breadth is limited.5. Code Quality / Maintainability
cli_coverage_steps.py:43-44shell=False. WhileFalseis the default, being explicit documents the deliberate security decision -- especially since the original code hadshell=Trueexplicitly. The reference implementation also omits it, so this is consistent but less self-documenting.cli_coverage_steps.py:25I run shell command "{command}"suggests shell execution, but the implementation no longer uses a shell. This is a naming artifact; changing it would require updating feature files and is not worth the churn for this PR.cli_coverage_steps.py:43# Use shell=True to handle quoted arguments correctlywas appropriately removed, but no replacement explains the rationale forshlex.split(). A brief comment like# Tokenize without shell to prevent injection (see #734)could aid future readers. The PR description and commit message provide this context externally.Verification Summary
shell=Trueremovedshlex.split()correctly tokenizes quoted pathssys.executablecontaining spaces)cli_plan_context_commands_steps.py:351)tui_shell_exec_coverage_steps.py:67-- different pattern"I run a shell command")shell=Truein any Python step fileVerdict
APPROVE -- The change is correct, well-scoped, and addresses the security finding appropriately. The minor findings above are suggestions for improvement and do not block merge.
@ -1,5 +1,6 @@"""Step definitions for CLI coverage tests."""import shlex[Low -- Bug] This docstring is now misleading. The function no longer uses a subprocess shell (
shell=Truewas removed). Consider updating to:[Low -- Code Quality] Consider adding explicit
shell=Falseto document the deliberate security decision:This makes the security-conscious choice visible at a glance, especially since the original code had
shell=Trueexplicitly.6077e92148bd18491b8dNew commits pushed, approval review dismissed automatically according to repository settings