fix: deduplicate cron PATH into single global line#4099
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces a hardcoded OPENCODE_BIN with a dynamic Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 a critical issue where cron jobs failed to inherit the user's full environment PATH, leading to problems with tool discovery. By centralizing the management of the crontab's PATH variable and removing redundant inline PATH declarations from individual cron entries, the system now correctly respects user-defined environment configurations, improving robustness and compatibility with various development tools. Highlights
Changelog
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
|
🔍 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: Tue Mar 10 23:34:51 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@setup.sh`:
- Around line 113-138: The PATH line currently embeds the marker comment into
the PATH value (marker variable and path_line in _ensure_cron_path), corrupting
cron PATH; change the implementation so the marker is a separate comment line
(e.g., marker_line="# aidevops-path") and keep path_line as a plain "PATH=..."
without the marker, update the grep checks to look for marker_line, and when
constructing current_crontab prepend/replace both marker_line and path_line as
two distinct lines so the marker is not appended into the PATH string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9df890bb-efa4-4d55-a503-8bd08aeecfe1
📒 Files selected for processing (2)
.agents/scripts/pulse-wrapper.shsetup.sh
![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 correctly addresses an issue where hardcoded PATH variables in cron entries could override user-specific paths. The introduction of a centralized PATH definition in the crontab, managed by the new _ensure_cron_path function, is a solid architectural improvement. The related changes to remove the inline PATH from cron jobs and update pulse-wrapper.sh are consistent with this goal.
However, I've identified several issues in the implementation of _ensure_cron_path. The use of echo to handle crontab content can lead to corruption due to shell expansion. Additionally, the blanket suppression of errors with 2>/dev/null can hide critical issues, and building multi-line strings in variables for streaming can be less efficient and readable than using command groups. I've provided a comment with suggested fixes and improvements to make this function safer and more robust.
Note: Security Review did not run due to the size of the PR.
| _ensure_cron_path() { | ||
| local current_crontab marker="# aidevops-path" | ||
| current_crontab=$(crontab -l 2>/dev/null) || current_crontab="" | ||
|
|
||
| # Build the new PATH line from the current environment | ||
| local path_line="PATH=${PATH} ${marker}" | ||
|
|
||
| if echo "$current_crontab" | grep -qF "$marker"; then | ||
| # Remove old line, prepend updated one | ||
| current_crontab="${path_line} | ||
| $(echo "$current_crontab" | grep -vF "$marker")" | ||
| elif [[ -n "$current_crontab" ]]; then | ||
| # Prepend before first entry | ||
| current_crontab="${path_line} | ||
| ${current_crontab}" | ||
| else | ||
| current_crontab="$path_line" | ||
| fi | ||
|
|
||
| echo "$current_crontab" | crontab - 2>/dev/null || true | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
The use of echo "$current_crontab" is unsafe and can corrupt the user's crontab. If the crontab contains any shell-special characters like $ or * that are not properly escaped for the echo command's context, echo will interpret them, leading to data loss or corruption when rewriting the crontab. For example, a line like * * * * * command $VAR would have $VAR expanded by echo if $VAR is set in the script's environment, or disappear if it's not. To fix this, you should use printf '%s\n' "$current_crontab" which prints the variable's content literally. This should be applied to all instances where current_crontab is being piped or used in command substitution.
Additionally, avoid using 2>/dev/null for blanket suppression of command errors in crontab -l and crontab -. This can hide critical authentication, syntax, or system issues, making debugging difficult. Consider handling errors more explicitly or logging them.
While the current code_suggestion maintains the existing structure for line count compatibility, for future improvements, consider streaming multi-line content to crontab - using a command group {...} with printf statements instead of building a large multi-line string in a variable. This can improve readability and efficiency for very large crontabs.
| _ensure_cron_path() { | |
| local current_crontab marker="# aidevops-path" | |
| current_crontab=$(crontab -l 2>/dev/null) || current_crontab="" | |
| # Build the new PATH line from the current environment | |
| local path_line="PATH=${PATH} ${marker}" | |
| if echo "$current_crontab" | grep -qF "$marker"; then | |
| # Remove old line, prepend updated one | |
| current_crontab="${path_line} | |
| $(echo "$current_crontab" | grep -vF "$marker")" | |
| elif [[ -n "$current_crontab" ]]; then | |
| # Prepend before first entry | |
| current_crontab="${path_line} | |
| ${current_crontab}" | |
| else | |
| current_crontab="$path_line" | |
| fi | |
| echo "$current_crontab" | crontab - 2>/dev/null || true | |
| return 0 | |
| } | |
| _ensure_cron_path() { | |
| local current_crontab marker="# aidevops-path" | |
| current_crontab=$(crontab -l) || current_crontab="" | |
| # Build the new PATH line from the current environment | |
| local path_line="PATH=${PATH} ${marker}" | |
| if printf '%s\n' "$current_crontab" | grep -qF "$marker"; then | |
| # Remove old line, prepend updated one | |
| current_crontab="${path_line}\n$(printf '%s\n' "$current_crontab" | grep -vF "$marker")" | |
| elif [[ -n "$current_crontab" ]]; then | |
| # Prepend before first entry | |
| current_crontab="${path_line}\n${current_crontab}" | |
| else | |
| current_crontab="$path_line" | |
| fi | |
| printf '%s\n' "$current_crontab" | crontab - || true | |
| return 0 | |
| } |
References
- To prevent command injection from shell metacharacters in crontab entries, wrap variables in single quotes. This is a POSIX-compliant method to prevent all shell expansion, including
$(...), backticks, and variable expansion. - Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.
- In shell scripts, stream multi-line content to a file using a command group
{...}withechostatements instead of building a multi-line string in a variable, as it improves readability and efficiency.
alex-solovyev
force-pushed
the
bugfix/cron-path-dedup
branch
from
0e2f273 to
98b36d1
Compare
🔍 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: Wed Mar 11 00:44:02 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
setup.sh (1)
119-147:⚠️ Potential issue | 🟠 MajorKeep the cron marker separate and replace only the aidevops-managed PATH block.
Line 134 appends
# aidevops-pathto thePATH=assignment, which cron parses as part of the value, not as a comment. Lines 136-141 also strip every top-levelPATH=line, so rerunning setup can delete user-managed PATH directives for unrelated cron jobs. Please write the marker as its own line and only replace the tagged aidevops block.In crontab syntax, if an environment-setting line is `PATH=/usr/local/bin:/usr/bin:/bin # aidevops-path`, is `# aidevops-path` treated as a comment or as part of the PATH value? Please cite authoritative `crontab(5)` documentation.🔧 Suggested fix
_ensure_cron_path() { - local current_crontab marker="# aidevops-path" + local current_crontab marker_line="# aidevops-path" current_crontab=$(crontab -l 2>/dev/null) || current_crontab="" # Deduplicate PATH entries (preserving order) local deduped_path="" local -A seen_dirs=() @@ done unset IFS - local path_line="PATH=${deduped_path} ${marker}" + local path_line="PATH=${deduped_path}" + local managed_block="${marker_line} +${path_line}" - # Remove ALL top-level PATH= lines (tagged and untagged bare ones). - # Bare PATH= lines (without a cron schedule) conflict with our managed one. - # Inline PATH= inside cron entries (e.g. * * * * * PATH=...) are handled - # separately by removing them from individual entries. local filtered - filtered=$(echo "$current_crontab" | grep -v "^PATH=" || true) + filtered=$(printf '%s\n' "$current_crontab" | awk -v marker="$marker_line" ' + $0 == marker { skip_managed_path=1; next } + skip_managed_path && /^PATH=/ { skip_managed_path=0; next } + { skip_managed_path=0; print } + ') if [[ -n "$filtered" ]]; then - current_crontab="${path_line} + current_crontab="${managed_block} ${filtered}" else - current_crontab="$path_line" + current_crontab="$managed_block" fi - echo "$current_crontab" | crontab - 2>/dev/null || true + printf '%s\n' "$current_crontab" | crontab - 2>/dev/null || true return 0 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.sh` around lines 119 - 147, The current code appends the marker into the PATH value (marker variable and path_line) and then strips all top‑level PATH= lines (variables current_crontab, marker, path_line, filtered), which can remove unrelated user PATH settings; change the logic to write the aidevops marker as its own line (not appended to PATH), and only replace the aidevops‑managed block: emit a distinct start/end marker line (or a single unique marker line on its own) adjacent to the managed PATH line, then update the filtering to remove only a previous aidevops-managed PATH block (e.g. grep/sed that looks for the marker or the exact PATH preceded/followed by the marker) instead of deleting all top‑level PATH= lines; keep handling inline PATH= in cron entries separately as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@setup.sh`:
- Around line 119-147: The current code appends the marker into the PATH value
(marker variable and path_line) and then strips all top‑level PATH= lines
(variables current_crontab, marker, path_line, filtered), which can remove
unrelated user PATH settings; change the logic to write the aidevops marker as
its own line (not appended to PATH), and only replace the aidevops‑managed
block: emit a distinct start/end marker line (or a single unique marker line on
its own) adjacent to the managed PATH line, then update the filtering to remove
only a previous aidevops-managed PATH block (e.g. grep/sed that looks for the
marker or the exact PATH preceded/followed by the marker) instead of deleting
all top‑level PATH= lines; keep handling inline PATH= in cron entries separately
as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2800a7ce-5dac-46f3-b1b1-6738043a8dc0
📒 Files selected for processing (2)
.agents/scripts/pulse-wrapper.shsetup.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .agents/scripts/pulse-wrapper.sh
alex-solovyev
force-pushed
the
bugfix/cron-path-dedup
branch
from
98b36d1 to
3c5a74e
Compare
🔍 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: Wed Mar 11 01:24:32 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@setup.sh`:
- Around line 141-150: The current filtered computation removes every top-level
PATH= line; change it to only remove the aidevops-managed marker line and the
PATH= line that immediately follows it so user-owned PATH= lines are preserved.
Modify the logic that sets filtered (currently using grep -vE
"^PATH=|^${marker}$") to instead filter out the marker line and, if present, the
single subsequent PATH= line (e.g. use an awk/sed pass that skips the marker and
the next line only when that next line starts with "PATH="). Keep the rest of
the variables and the recomposition of current_crontab using path_block
unchanged (variables: filtered, current_crontab, path_block, marker).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 30d717c4-ef67-474b-8cd8-ea6fab88151a
📒 Files selected for processing (2)
.agents/scripts/pulse-wrapper.shsetup.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .agents/scripts/pulse-wrapper.sh
setup.sh was hardcoding PATH="/usr/local/bin:/usr/bin:/bin" into each cron entry, overriding any global PATH the user set at the top of their crontab. This broke opencode discovery (nvm), bun, cargo, and any other PATH-dependent tool. Changes: - Add _ensure_cron_path() to manage a tagged comment + PATH= line pair at the top of the crontab, built from the current $PATH at setup time - Marker is a separate comment line (crontab does not support inline comments on env var lines — they become part of the value) - Only remove the aidevops-managed marker+PATH pair, not user-owned lines - Deduplicate PATH entries to avoid repetition - Remove inline PATH= from all 5 cron entries (pulse, stats, process-guard, screen-time, profile-readme) - Remove OPENCODE_BIN from pulse cron entry (resolved from PATH at runtime) - Fix pulse-wrapper.sh fallback: command -v instead of /opt/homebrew/bin - Make stats-wrapper cron always regenerate (like pulse) to pick up changes - Use printf instead of echo for crontab content (safe with metacharacters)
alex-solovyev
force-pushed
the
bugfix/cron-path-dedup
branch
from
3c5a74e to
55553aa
Compare
🔍 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: Wed Mar 11 01:33:42 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
1 participant
Summary
setup.shhardcodedPATH="/usr/local/bin:/usr/bin:/bin"in each cron entry, overriding the user's global crontab PATH and breaking nvm/bun/cargo tool discovery_ensure_cron_path()— manages a single taggedPATH=line at the top of crontab, built from the current$PATHat setup timePATH=from all 5 cron entries (pulse, stats, process-guard, screen-time, profile-readme)OPENCODE_BINfrom pulse cron entry — now resolved from PATH at runtime viacommand -vpulse-wrapper.shfallback from hardcoded/opt/homebrew/bin/opencodetocommand -v opencodeBefore
Each entry overrode global PATH, losing nvm, bun, and other user paths.
After
Single PATH line at top, re-running
setup.shupdates it idempotently.Summary by CodeRabbit