GH#5147: Clean up orphaned supervisor-helper.sh and supervisor/ during update#5151
GH#5147: Clean up orphaned supervisor-helper.sh and supervisor/ during update#5151alex-solovyev merged 2 commits intomainfrom
Conversation
…date (GH#5147) After the supervisor-to-pulse-wrapper migration, aidevops update (rsync) only adds/overwrites files — it doesn't delete files that no longer exist in the source. Users who installed before the migration retain orphaned supervisor-helper.sh and supervisor/ modules that shadow pulse-wrapper.sh. Add migrate_orphaned_supervisor() to setup-modules/migrations.sh that: - Removes orphaned scripts/supervisor-helper.sh from deployed agents - Removes orphaned scripts/supervisor/ module directory (with safety check) - Removes cron entries referencing supervisor-helper.sh - Removes old launchd plist with com.aidevops.supervisor-pulse label (macOS) The migration is wired into both interactive and non-interactive setup paths, running after cleanup_deprecated_paths and before the pulse scheduler section so old entries are cleaned before new ones are installed. Closes #5147
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 the issue of orphaned files and scheduler entries left behind after the supervisor-to-pulse-wrapper migration. Previously, 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
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a migration function that detects and cleans up orphaned Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable the changed files summary in the walkthrough.Disable the |
🔍 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 17 15:29:39 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
![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 a migration script to clean up orphaned files and scheduler entries from a previous architecture, which is a valuable improvement for maintainability. The changes are well-structured, with a new migration function and a comprehensive test suite. I've identified two areas in the migration script that could be made more robust, specifically regarding the cleanup of crontab entries and launchd agents on macOS. My suggestions have been refined to ensure adherence to repository guidelines against blanket error suppression, aiming for more reliable and debuggable cleanup operations.
| if [[ -f "$old_plist" ]] || _launchd_has_agent "$old_label" 2>/dev/null; then | ||
| launchctl unload "$old_plist" 2>/dev/null || true | ||
| rm -f "$old_plist" | ||
| print_info "Removed orphaned supervisor-pulse LaunchAgent ($old_label)" | ||
| ((++cleaned)) | ||
| fi |
There was a problem hiding this comment.
There's a potential issue in the launchd cleanup logic. If an agent is loaded but its corresponding .plist file is missing (an orphaned agent), launchctl unload "$old_plist" will fail because the path doesn't exist, and the agent won't be removed.
A more robust approach is to use launchctl remove "$old_label", which unloads and removes the agent by its label, working correctly even if the .plist file is gone. This ensures that orphaned agents are properly cleaned up. I've removed 2>/dev/null from the launchctl remove command and the _launchd_has_agent check in the suggestion to avoid blanket suppression of errors, allowing potential issues to be visible for debugging, as per repository guidelines.
| if [[ -f "$old_plist" ]] || _launchd_has_agent "$old_label" 2>/dev/null; then | |
| launchctl unload "$old_plist" 2>/dev/null || true | |
| rm -f "$old_plist" | |
| print_info "Removed orphaned supervisor-pulse LaunchAgent ($old_label)" | |
| ((++cleaned)) | |
| fi | |
| if _launchd_has_agent "$old_label" || [[ -f "$old_plist" ]]; then | |
| # Use `remove` by label, which is more robust as it works even if the plist file is missing. | |
| launchctl remove "$old_label" || true | |
| # Also remove the plist file if it exists. | |
| rm -f "$old_plist" | |
| print_info "Removed orphaned supervisor-pulse LaunchAgent ($old_label)" | |
| ((++cleaned)) | |
| fi |
References
- 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.
setup-modules/migrations.sh
Outdated
| printf '%s\n' "$new_crontab" | crontab - 2>/dev/null || true | ||
| else | ||
| # All entries were supervisor-helper.sh — clear crontab | ||
| echo "" | crontab - 2>/dev/null || true |
There was a problem hiding this comment.
The comment on the preceding line states "clear crontab", but echo "" | crontab - installs a crontab with a single empty line rather than removing it. To truly clear the crontab, crontab -r should be used. This would also be more consistent with the test script (tests/test-migrate-orphaned-supervisor.sh), which uses crontab -r to restore the crontab to its original state. I've removed 2>/dev/null from the suggestion to avoid blanket suppression of errors, allowing potential issues like permissions problems to be visible for debugging, as per repository guidelines.
| echo "" | crontab - 2>/dev/null || true | |
| crontab -r || true |
References
- 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test-migrate-orphaned-supervisor.sh (1)
186-222: Cron test manipulates real system crontab — consider documenting this behavior.The test saves and restores the original crontab, which is the correct approach for testing cron manipulation. However, if the test crashes between lines 196 and 214-219, the system crontab would be left with test entries.
The test entries are clearly marked (
# aidevops: test-keep), so they're identifiable. Consider adding a comment noting this behavior, or wrapping the cron test section in a subshell with a trap for safer cleanup.Optional: Add trap for safer crontab restoration
# ============================================================================ section "Test: removes cron entries referencing supervisor-helper.sh" # ============================================================================ # Install a fake cron entry (only if crontab is available) if command -v crontab &>/dev/null; then # Save current crontab original_crontab=$(crontab -l 2>/dev/null) || original_crontab="" + + # Ensure crontab is restored even if test fails + restore_crontab() { + if [[ -n "$original_crontab" ]]; then + printf '%s\n' "$original_crontab" | crontab - 2>/dev/null || true + else + crontab -r 2>/dev/null || true + fi + } + trap restore_crontab RETURN # Install test crontab with supervisor-helper.sh entry🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-migrate-orphaned-supervisor.sh` around lines 186 - 222, The test manipulates the real system crontab and can leave test entries if it aborts; protect and restore crontab by adding a cleanup trap or running the crontab-modifying block in a subshell that always restores original_crontab on EXIT. Specifically, around the block that sets original_crontab, pipes the test crontab into crontab, calls migrate_orphaned_supervisor, and reads new_crontab, ensure you register a trap (or use a subshell) that will run the same restore logic (using original_crontab and crontab -r fallback) on EXIT/ERR so the code in migrate_orphaned_supervisor and the original_crontab/new_crontab variables are always cleaned up even if the test fails.setup-modules/migrations.sh (1)
1016-1021: Consider usingcrontab -rwhen clearing all entries.When all cron entries were
supervisor-helper.shentries, the current approach installs an empty crontab viaecho "" | crontab -. This works, but leaves an empty crontab file. Usingcrontab -rwould fully remove the crontab.However, this is a minor point — an empty crontab is functionally equivalent and
crontab -rfails if there's no crontab, requiring additional handling.Alternative: use crontab -r for full removal
if [[ -n "$new_crontab" ]]; then printf '%s\n' "$new_crontab" | crontab - 2>/dev/null || true else # All entries were supervisor-helper.sh — clear crontab - echo "" | crontab - 2>/dev/null || true + crontab -r 2>/dev/null || true fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup-modules/migrations.sh` around lines 1016 - 1021, The cleanup branch that currently does echo "" | crontab - when new_crontab is empty should instead remove the crontab entirely using crontab -r, but handle the case where no crontab exists; modify the else block that handles supervisor-helper.sh-only entries to attempt crontab -r and fall back to the existing echo "" | crontab - (or only call crontab -r after verifying a crontab exists with crontab -l) so you fully remove the crontab when present without causing errors when none exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@setup-modules/migrations.sh`:
- Around line 1016-1021: The cleanup branch that currently does echo "" |
crontab - when new_crontab is empty should instead remove the crontab entirely
using crontab -r, but handle the case where no crontab exists; modify the else
block that handles supervisor-helper.sh-only entries to attempt crontab -r and
fall back to the existing echo "" | crontab - (or only call crontab -r after
verifying a crontab exists with crontab -l) so you fully remove the crontab when
present without causing errors when none exists.
In `@tests/test-migrate-orphaned-supervisor.sh`:
- Around line 186-222: The test manipulates the real system crontab and can
leave test entries if it aborts; protect and restore crontab by adding a cleanup
trap or running the crontab-modifying block in a subshell that always restores
original_crontab on EXIT. Specifically, around the block that sets
original_crontab, pipes the test crontab into crontab, calls
migrate_orphaned_supervisor, and reads new_crontab, ensure you register a trap
(or use a subshell) that will run the same restore logic (using original_crontab
and crontab -r fallback) on EXIT/ERR so the code in migrate_orphaned_supervisor
and the original_crontab/new_crontab variables are always cleaned up even if the
test fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0ae46d2-75e8-411c-8f41-e8cc33fd2f0e
📒 Files selected for processing (3)
setup-modules/migrations.shsetup.shtests/test-migrate-orphaned-supervisor.sh
- Line 1020: replace 'echo "" | crontab -' with 'crontab -r' to truly clear the crontab when all entries were supervisor-helper.sh references (Gemini Code Assist MEDIUM suggestion) - Line 1036: replace 'launchctl unload "$old_plist"' with 'launchctl remove "$old_label"' so orphaned agents loaded without a backing plist file on disk are still unloaded correctly (Gemini Code Assist HIGH suggestion) shellcheck: 0 violations | tests: 10/10 pass
🔍 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 17 15:38:44 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
|
Dispatching worker to address Gemini review suggestions (1 high, 1 medium).
|
…(PR #5151) - Replace `launchctl unload "$old_plist"` with `launchctl remove "$old_label" || true` and update condition to check agent registration first; more robust when plist file is missing. Remove 2>/dev/null per repo guidelines. - Replace `echo "" | crontab -` with `crontab -r || true` to truly clear the crontab, consistent with the test script.
After the supervisor-to-pulse-wrapper migration, rsync-based updates only add/overwrite files — they don't delete files removed from the source. Users who installed before the archival retain 14 orphaned scripts in ~/.aidevops/agents/scripts/ that no longer exist upstream. Add them to the deprecated_paths array in cleanup_deprecated_paths() so they are removed on the next aidevops update, following the same pattern used for supervisor-helper.sh in PR #5151. Scripts: pattern-tracker, quality-sweep, quality-loop, review-pulse, self-improve, coderabbit-pulse, coderabbit-task-creator, audit-task-creator, batch-cleanup, coordinator, finding-to-task, objective-runner, ralph-loop, stale-pr. Closes #5155
…5157) After the supervisor-to-pulse-wrapper migration, rsync-based updates only add/overwrite files — they don't delete files removed from the source. Users who installed before the archival retain 14 orphaned scripts in ~/.aidevops/agents/scripts/ that no longer exist upstream. Add them to the deprecated_paths array in cleanup_deprecated_paths() so they are removed on the next aidevops update, following the same pattern used for supervisor-helper.sh in PR #5151. Scripts: pattern-tracker, quality-sweep, quality-loop, review-pulse, self-improve, coderabbit-pulse, coderabbit-task-creator, audit-task-creator, batch-cleanup, coordinator, finding-to-task, objective-runner, ralph-loop, stale-pr. Closes #5155
…visor (GH#5159) - launchd: remove 2>/dev/null from _launchd_has_agent call and launchctl remove; reorder condition to check agent registration first (handles orphaned agents without plist files) - crontab: remove 2>/dev/null from crontab - and crontab -r so permission errors surface for debugging Per Gemini Code Assist review on PR #5151 (1 high, 1 medium finding). The || true guards already prevent script abort on expected failures. shellcheck: 0 violations | tests: 10/10 pass Closes #5159
…visor (GH#5159) (#5175) - launchd: remove 2>/dev/null from _launchd_has_agent call and launchctl remove; reorder condition to check agent registration first (handles orphaned agents without plist files) - crontab: remove 2>/dev/null from crontab - and crontab -r so permission errors surface for debugging Per Gemini Code Assist review on PR #5151 (1 high, 1 medium finding). The || true guards already prevent script abort on expected failures. shellcheck: 0 violations | tests: 10/10 pass Closes #5159
marcusquinn
added
the
review-feedback-scanned
2 participants
Summary
migrate_orphaned_supervisor()migration tosetup-modules/migrations.shthat removes orphanedsupervisor-helper.sh,supervisor/module directory, old cron entries, and old launchd plists left behind after the supervisor-to-pulse-wrapper migrationsetup.shpaths, running aftercleanup_deprecated_pathsand before the pulse scheduler sectiontests/test-migrate-orphaned-supervisor.sh) covering file removal, directory safety checks, cron migration, and idempotencyProblem
After upgrading to v3.0.12,
aidevops update(rsync) only adds/overwrites files — it doesn't delete files removed from the source. Users who installed before the supervisor-to-pulse-wrapper migration (PR #2291, PR #2475) retain orphaned files that shadow the new architecture:~/.aidevops/agents/scripts/supervisor-helper.sh(old entry point)~/.aidevops/agents/scripts/supervisor/(old module directory with pulse.sh, dispatch.sh, etc.)supervisor-helper.sh pulseThe old code has known bugs (stdin consumption in
ai_decide()causing only 1 task per pulse) that are fixed inpulse-wrapper.sh.What the migration does
supervisor-helper.shfrom~/.aidevops/agents/scripts/(does NOT touchsupervisor-archived/)supervisor/directory only if it contains recognizable module files (pulse.sh,dispatch.sh,_common.sh) — preserves user-created directoriessupervisor-helper.sh— setup.sh will installpulse-wrapper.shentries if pulse is enabledcom.aidevops.supervisor-pulselabel (macOS)Verification
bash tests/test-migrate-orphaned-supervisor.sh— 10/10 passshellcheck setup-modules/migrations.sh— 0 violationsbash -n setup.sh— syntax OKCloses #5147
Summary by CodeRabbit
New Features
Tests