fix(uninstall): redirect is not valid on a for-list - #320
Conversation
Line 469 wrote 'for item in $cache_path 2>/dev/null; do'. A redirection cannot be applied to a for-list, so the file did not parse. Removed the redirect; the guard inside the loop already handles a missing path. Found by an estate-wide shellcheck sweep of 5,111 tracked scripts across 375 repos. This file was one of 11 that fail to PARSE (SC1073/SC1072) — shellcheck stops analysing at the failure, so anything after it was never checked either. Verified: shellcheck -S error now reports 0 findings for this file.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe uninstall script now expands the cache-cleanup glob directly in ChangesCache cleanup
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This localized fix restores parsing for the uninstall script without changing its intended guard behavior. No actionable merge-blocking risk remains beyond normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
There was a problem hiding this comment.
Pull Request Overview
The PR successfully resolves the syntax error caused by invalid redirection on the loop header. However, the current implementation fails to satisfy the requirement for robust cache removal. The logic on line 469 utilizes unquoted variables for globbing, which triggers word-splitting and will cause the script to fail for users with spaces in their file paths. While Codacy results are up to standards, this logic bug should be addressed before merging to ensure reliability across all environments.
1 comment outside of the diff
scripts/management/uninstall.sh
line 47⚪ LOW RISK
The variableMAGENTAis assigned but never used. Since other color variables such asCYANandGREENare already used for the script's output formatting, you should remove this unused definition to keep the script clean.
Test suggestions
- Verify script syntax using 'bash -n' or 'shellcheck'\n- [ ] Execute cleanup_cache with a glob pattern that matches multiple files\n- [ ] Execute cleanup_cache with a glob pattern that matches zero files to ensure no error is thrown
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify script syntax using 'bash -n' or 'shellcheck'\n- [ ] Execute cleanup_cache with a glob pattern that matches multiple files\n- [ ] Execute cleanup_cache with a glob pattern that matches zero files to ensure no error is thrown
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| if [[ "$cache_path" == *"*"* ]]; then | ||
| # Glob pattern | ||
| for item in $cache_path 2>/dev/null; do | ||
| for item in $cache_path; do |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Expanding $cache_path without quotes causes the shell to perform word splitting. If any directory in the path (such as the user's home directory) contains spaces, the loop will break the path into multiple invalid segments, causing the cleanup to fail. Refactor the cleanup_cache function to safely handle paths with spaces by avoiding unquoted variable expansion for globs (e.g., using shopt -s nullglob).
🔍 Hypatia Security ScanFindings: 64 issues detected
View findings[
{
"reason": "Issue in build.yml",
"type": "missing_timeout_minutes",
"file": "build.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium"
},
{
"reason": "Issue in push-email-notify.yml",
"type": "missing_timeout_minutes",
"file": "push-email-notify.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium"
},
{
"reason": "Issue in instant-sync.yml",
"type": "secret_action_without_presence_gate",
"file": "instant-sync.yml",
"action": "peter-evans/repository-dispatch",
"rule_module": "workflow_audit",
"severity": "high"
},
{
"reason": "Issue in mirror.yml",
"type": "secret_action_without_presence_gate",
"file": "mirror.yml",
"action": "webfactory/ssh-agent",
"rule_module": "workflow_audit",
"severity": "high"
},
{
"reason": "Issue in mirror.yml",
"type": "secret_action_without_presence_gate",
"file": "mirror.yml",
"action": "webfactory/ssh-agent",
"rule_module": "workflow_audit",
"severity": "high"
},
{
"reason": "Issue in mirror.yml",
"type": "secret_action_without_presence_gate",
"file": "mirror.yml",
"action": "webfactory/ssh-agent",
"rule_module": "workflow_audit",
"severity": "high"
},
{
"reason": "Issue in mirror.yml",
"type": "secret_action_without_presence_gate",
"file": "mirror.yml",
"action": "webfactory/ssh-agent",
"rule_module": "workflow_audit",
"severity": "high"
},
{
"reason": "Issue in mirror.yml",
"type": "secret_action_without_presence_gate",
"file": "mirror.yml",
"action": "webfactory/ssh-agent",
"rule_module": "workflow_audit",
"severity": "high"
},
{
"reason": "Issue in mirror.yml",
"type": "secret_action_without_presence_gate",
"file": "mirror.yml",
"action": "webfactory/ssh-agent",
"rule_module": "workflow_audit",
"severity": "high"
},
{
"reason": "Issue in codeql.yml",
"type": "codeql_missing_actions_language",
"file": "codeql.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium"
}
]Powered by Hypatia Neurosymbolic CI/CD Intelligence |
Line 469 wrote 'for item in $cache_path 2>/dev/null; do'. A redirection cannot be applied to a for-list, so the file did not parse. Removed the redirect; the guard inside the loop already handles a missing path.
Found by an estate-wide shellcheck sweep of 5,111 tracked scripts across 375 repos. This file was one of 11 that fail to parse (
SC1073/SC1072) — shellcheck stops analysing at the failure, so everything after it was never checked either.Verified:
shellcheck -S errornow reports 0 findings for this file.