GH#2899: Fix quality-debt review feedback on memory-pressure-monitor.sh#2930
GH#2899: Fix quality-debt review feedback on memory-pressure-monitor.sh#2930marcusquinn merged 1 commit intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis pull request hardens the memory pressure monitor script by addressing security vulnerabilities and performance inefficiencies. Changes include validating integer configuration variables before use, replacing unsafe osascript interpolation with stdin-based parameter passing, removing blanket error suppression, and optimizing process/session parsing to use shell builtins instead of subshells. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses several quality-debt issues and review feedback items identified in the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.agents/scripts/memory-pressure-monitor.sh (1)
322-326: Replacebasenamesubprocess with pure parameter expansion in the hot process loop.Line 325 spawns
basenameinside a nested loop processing monitored processes. This should use bash parameter expansion for consistency with the stated intent on line 322 and to eliminate subprocess overhead in a hot path.Proposed fix
- local cmd_path="${cmd%% *}" - local cmd_name - cmd_name=$(basename "$cmd_path" 2>/dev/null || echo "unknown") + local cmd_path="${cmd%% *}" + local cmd_name="${cmd_path##*/}" + [[ -n "$cmd_name" ]] || cmd_name="unknown"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/memory-pressure-monitor.sh around lines 322 - 326, Replace the subprocess call to basename with pure parameter expansion: instead of cmd_name=$(basename "$cmd_path" 2>/dev/null || echo "unknown"), compute cmd_name from cmd_path using ${cmd_path##*/} and fallback to "unknown" when empty (e.g., cmd_name="${cmd_path##*/}"; [ -z "$cmd_name" ] && cmd_name="unknown"). This keeps the existing extraction logic (cmd_path="${cmd%% *}") and removes the basename subprocess inside the hot loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aidevops.sh`:
- Line 867: The piped call to `_timeout_cmd 30 brew info --json=v2 "$brew_pkg" |
jq -r ...` can hang when the perl fallback is used; change the pattern to write
`_timeout_cmd` stdout to a temporary file, run `jq` against that temp file to
extract `.formulae[0].versions.stable`, assign the result to `latest`, and
ensure the temp file is removed (use mktemp and rm or a short-lived
subshell/trap) so the command no longer relies on a direct pipe between
`_timeout_cmd` and `jq`.
---
Nitpick comments:
In @.agents/scripts/memory-pressure-monitor.sh:
- Around line 322-326: Replace the subprocess call to basename with pure
parameter expansion: instead of cmd_name=$(basename "$cmd_path" 2>/dev/null ||
echo "unknown"), compute cmd_name from cmd_path using ${cmd_path##*/} and
fallback to "unknown" when empty (e.g., cmd_name="${cmd_path##*/}"; [ -z
"$cmd_name" ] && cmd_name="unknown"). This keeps the existing extraction logic
(cmd_path="${cmd%% *}") and removes the basename subprocess inside the hot loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59aa9722-2a63-48da-8475-f8999773366d
📒 Files selected for processing (4)
.agents/scripts/auto-update-helper.sh.agents/scripts/memory-pressure-monitor.sh.agents/scripts/tool-version-check.shaidevops.sh
…2884 - HIGH: Fix AppleScript injection in notify() — pass script via stdin with 'on run argv' instead of -e string interpolation, preventing breakout via crafted process names - MEDIUM: Validate MEMORY_COOLDOWN_SECS and DAEMON_INTERVAL with _validate_int() to prevent arithmetic expansion injection - MEDIUM: Remove redundant 2>/dev/null on cat after file existence check to surface permission errors for debugging - MEDIUM: Optimize _collect_monitored_processes() loop — use read builtin and parameter expansion instead of echo|awk|cut subshells per line - MEDIUM: Optimize _count_interactive_sessions() loop — use read builtin instead of echo|awk subshell per line Closes #2899
marcusquinn
force-pushed
the
bugfix/memory-pressure-review-fixes
branch
from
500ec77 to
37d9aad
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses several quality and security issues in memory-pressure-monitor.sh, including a critical command injection vulnerability. The fixes are well-implemented and significantly improve the script's robustness and performance. The introduction of a portable _timeout_cmd function is also a valuable addition. My review focuses on ensuring this new utility is used safely across all files and that resource management is robust. I've identified one critical issue where _timeout_cmd is piped in a way that can cause hangs on macOS, and a couple of medium-severity opportunities to improve temporary file cleanup using project-established patterns and to avoid suppressing diagnostic output.
I am having trouble creating individual review comments. Click here to see my feedback.
aidevops.sh (867)
The new _timeout_cmd is a great portable utility. However, its output is being piped directly to jq. As noted in the changes to tool-version-check.sh, the perl fallback for timeout on macOS can cause the downstream command in a pipe (in this case, jq) to hang if a timeout occurs. To ensure robustness on all platforms, please use a temporary file here, just as you did in tool-version-check.sh. Additionally, avoid suppressing stderr with 2>/dev/null as this can mask important diagnostic information, as per project guidelines.
latest=$( (\n local _info_log\n _info_log=$(mktemp "${TMPDIR:-/tmp}/brew-info.XXXXXX")\n # Ensure cleanup on subshell exit\n trap 'rm -f "$_info_log"' EXIT\n if _timeout_cmd 30 brew info --json=v2 "$brew_pkg" >"$_info_log"; then\n jq -r '.formulae[0].versions.stable // empty' "$_info_log"\n fi\n) || true)
References
- Suppressing stderr with
2>/dev/nullcan mask important diagnostic information, such as permissions problems or other issues, and should be avoided.
.agents/scripts/tool-version-check.sh (121-129)
To make temporary file cleanup robust and align with the project's established pattern, please use push_cleanup for $_ver_log. This ensures the file is removed even if the function exits unexpectedly. Additionally, avoid suppressing stderr with 2>/dev/null as this can mask important diagnostic information, as per project guidelines.
local _ver_log
_ver_log=$(mktemp "${TMPDIR:-/tmp}/tool-ver.XXXXXX")
push_cleanup "rm -f \"$_ver_log\""
# shellcheck disable=SC2086
timeout_sec 5 "$cmd" $ver_flag >"$_ver_log" || true
version=$(head -1 "$_ver_log" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1 || echo "")
if [[ -z "$version" ]]; then
version=$(head -1 "$_ver_log" | grep -oE '[0-9]+\.[0-9]+' | head -1 || echo "unknown")
fi
rm -f "$_ver_log"
References
- The project has an established pattern for robust resource cleanup using
_save_cleanup_scope,trap '_run_cleanups' RETURN, andpush_cleanupto ensure resources are cleaned up on any exit path, including unexpected ones. - Suppressing stderr with
2>/dev/nullcan mask important diagnostic information, such as permissions problems or other issues, and should be avoided.
.agents/scripts/tool-version-check.sh (386-395)
To ensure robust temporary file cleanup for $_update_log and align with the project's established pattern, please use push_cleanup. This ensures the file is removed even if the script exits unexpectedly.
local _update_log
_update_log=$(mktemp "${TMPDIR:-/tmp}/tool-update.XXXXXX")
push_cleanup "rm -f \"$_update_log\""
if timeout_sec 120 bash -c "$update_cmd" >"$_update_log" 2>&1; then
tail -2 "$_update_log"
echo -e " ${GREEN}✓ Updated${NC}"
else
tail -2 "$_update_log"
echo -e " ${RED}✗ Failed${NC}"
fi
rm -f "$_update_log"
References
- The project has an established pattern for robust resource cleanup using
_save_cleanup_scope,trap '_run_cleanups' RETURN, andpush_cleanupto ensure resources are cleaned up on any exit path, including unexpected ones.
1 participant
Summary
Addresses all 5 unactioned review findings from PR #2884 (Gemini code review) on
.agents/scripts/memory-pressure-monitor.sh.osascript -estring interpolation — crafted process names could break out of the string literal and execute arbitrary commandsosascript - "$title" "$message"stdin piping usingon run argv— values are never interpolated into AppleScript sourceMEMORY_COOLDOWN_SECSnot validated before use in arithmetic context — exploitable viaa[$(cmd)]injection_validate_int()call for bothCOOLDOWN_SECSandDAEMON_INTERVAL, thenreadonly2>/dev/nulloncatafter[[ -f ]]existence check masks permission errors2>/dev/null— the `echo|awk|cutsubshells per line (inefficient)read -r pid rss_kb cmdbuiltin +${cmd%% *}parameter expansionecho|awksubshell per line (inefficient)read -r _ tty _builtinVerification
Closes #2899
Summary by CodeRabbit
Bug Fixes
Performance
Chores