t2893: Address quality-debt review feedback on pulse-wrapper.sh#2910
t2893: Address quality-debt review feedback on pulse-wrapper.sh#2910marcusquinn merged 1 commit intomainfrom
Conversation
- Rewrite _get_process_tree_cpu with iterative BFS to walk the full descendant tree (not just direct children via pgrep -P). Fixes incorrect CPU calculation when active processes are nested deeper than one level (e.g., node -> shell -> language-server). - Extract CPU-parsing logic into _get_pid_cpu helper to eliminate duplication between parent and child CPU reads. - Deduplicate watchdog kill logic in run_pulse() using kill_reason variable with a single kill+force-kill block, replacing two identical kill sequences. - Guard all kill commands with || true across check_dedup, guard_child_processes, and run_pulse to prevent set -e from aborting cleanup when the target process has already exited. - Switch process-alive checks from ps -p to kill -0 (more reliable, no stdout output). Closes #2893
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 significantly enhances the robustness and accuracy of 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
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe pull request hardens Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Thu Mar 5 13:50:30 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![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 introduces several high-quality improvements to pulse-wrapper.sh. The rewrite of _get_process_tree_cpu to use a proper BFS traversal is an excellent fix for the CPU calculation bug. Deduplicating the kill logic and adding || true guards significantly improves the script's maintainability and robustness. My review includes a few suggestions to further enhance error handling by removing 2>/dev/null from kill -0 checks, aligning with repository guidelines, and a suggestion to make a loop more robust against word splitting.
Note: Security Review did not run due to the size of the PR.
| # Force kill if still alive | ||
| if ps -p "$old_pid" >/dev/null; then | ||
| _force_kill_tree "$old_pid" | ||
| if kill -0 "$old_pid" 2>/dev/null; then |
There was a problem hiding this comment.
While using kill -0 is a good improvement for checking process existence, suppressing stderr with 2>/dev/null can hide important system errors, such as 'Operation not permitted'. This could lead the script to incorrectly assume a process is gone when it's actually running but unkillable by the current user. The general rules for this repository advise against suppressing stderr for this reason.
Consider removing the redirection to ensure all potential errors are logged for debugging, even if it adds some 'No such process' noise.
| if kill -0 "$old_pid" 2>/dev/null; then | |
| if kill -0 "$old_pid"; then |
References
- Avoid blanket suppression of command errors with '2>/dev/null' to ensure critical issues like permission denials remain visible for debugging.
| local p | ||
| for p in $(printf "%s\n" "${all_pids[@]}" | sort -u); do | ||
| local cpu | ||
| cpu=$(_get_pid_cpu "$p") | ||
| total_cpu=$((total_cpu + cpu)) | ||
| done |
There was a problem hiding this comment.
This for p in $(...) loop is subject to word splitting by the shell, which could cause issues if any of the PIDs contained whitespace (though unlikely for PIDs, it's a robustness concern). A more robust pattern for iterating over lines of output is to use a while read loop, which also tends to be more memory-efficient for large inputs. This change also makes the local p declaration on line 316 unnecessary.
| local p | |
| for p in $(printf "%s\n" "${all_pids[@]}" | sort -u); do | |
| local cpu | |
| cpu=$(_get_pid_cpu "$p") | |
| total_cpu=$((total_cpu + cpu)) | |
| done | |
| printf "%s\n" "${all_pids[@]}" | sort -u | while IFS= read -r p; do | |
| local cpu | |
| cpu=$(_get_pid_cpu "$p") | |
| total_cpu=$((total_cpu + cpu)) | |
| done |
| sleep 1 | ||
| if ps -p "$pid" >/dev/null; then | ||
| _force_kill_tree "$pid" | ||
| if kill -0 "$pid" 2>/dev/null; then |
There was a problem hiding this comment.
Suppressing stderr with 2>/dev/null here could mask important system errors like permission denials, which would prevent the script from correctly identifying and handling a process that is alive but not signalable. To align with repository guidelines on error visibility, consider removing the redirection.
| if kill -0 "$pid" 2>/dev/null; then | |
| if kill -0 "$pid"; then |
References
- Avoid blanket suppression of command errors with '2>/dev/null' to ensure critical issues like permission denials remain visible for debugging.
| echo "[pulse-wrapper] ${kill_reason} — killing" >>"$LOGFILE" | ||
| _kill_tree "$opencode_pid" || true | ||
| sleep 2 | ||
| if kill -0 "$opencode_pid" 2>/dev/null; then |
There was a problem hiding this comment.
As with the other kill -0 checks, suppressing stderr can hide important system errors like permission denials, potentially leading to incorrect script behavior. To ensure all errors are visible for debugging as per repository guidelines, it's safer to remove the 2>/dev/null redirection.
| if kill -0 "$opencode_pid" 2>/dev/null; then | |
| if kill -0 "$opencode_pid"; then |
References
- Avoid blanket suppression of command errors with '2>/dev/null' to ensure critical issues like permission denials remain visible for debugging.
1 participant
Summary
_get_process_tree_cpuwith iterative BFS to walk the full descendant tree, fixing incorrect CPU calculations when active processes are nested deeper than one levelrun_pulse()using akill_reasonvariable with a single kill block, replacing two identical kill+force-kill sequences|| trueacrosscheck_dedup,guard_child_processes, andrun_pulseto preventset -efrom aborting cleanup when the target process has already exitedChanges
Fix 1: Recursive process tree CPU calculation (line 260)
Problem:
_get_process_tree_cpuusedpgrep -Pwhich only returns direct children, missing grandchildren and deeper descendants. If active processes were nested deeper (e.g.,node -> shell -> language-server), the watchdog would see near-zero CPU and prematurely kill the process tree.Fix: Iterative BFS that expands
pgrep -Pat each level until no more children are found. Also extracts the duplicated CPU-parsing logic into a_get_pid_cpuhelper function.Fix 2: Deduplicated kill logic (line 1112)
Problem: The kill+force-kill sequence was duplicated for both the stale threshold check and the idle timeout check in the watchdog loop.
Fix: Both checks now set a
kill_reasonvariable, and a single block at the end performs the kill operation. This improves maintainability and ensures consistent behaviour.Fix 3: set -e safety for kill commands
Problem:
killcommands in cleanup paths were not guarded with|| true. If the target process exited between the alive-check and the kill call,set -ewould abort the script, preventing subsequent cleanup.Fix: All kill commands in
check_dedup,guard_child_processes, andrun_pulseare now guarded. Also switched fromps -ptokill -0for process-alive checks (more reliable, no stdout).Verification
Closes #2893
Summary by CodeRabbit
Bug Fixes
Chores