t1260: Fix setup.sh to install launchd schedulers on macOS for all users#1971
t1260: Fix setup.sh to install launchd schedulers on macOS for all users#1971marcusquinn merged 4 commits intomainfrom
Conversation
- Auto-update check now detects launchd plist OR cron entry (was cron-only) - Add supervisor pulse scheduler install during setup (was completely missing) - Auto-migrate cron entries to launchd on macOS during setup/update - Both interactive and non-interactive modes supported - Existing launchd installations are detected and skipped (idempotent)
Summary of ChangesHello @marcusquinn, 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 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
|
WalkthroughAdds supervisor pulse scheduler enablement and migration to setup.sh: detects existing launchd (macOS) or crontab (Linux), migrates legacy entries where found, and enables the scheduler automatically in non-interactive contexts or with a prompt when interactive. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Setup as setup.sh
participant Helper as auto-update-helper / launchd.sh
participant macLaunchd as macOS Launchd
participant Cron as crontab (Linux)
participant Pulse as supervisor/pulse.sh
User->>Setup: run installer
Setup->>Helper: check existing schedulers (launchd / crontab)
alt macOS: existing launchd
Helper->>macLaunchd: inspect / unload old plist (migrate)
Helper->>macLaunchd: install new plist (use display symlink)
macLaunchd-->>Helper: loaded
else Linux or cron present
Helper->>Cron: inspect crontab entries (migrate or create)
Cron-->>Helper: updated
end
Setup->>Pulse: enable scheduler (silent if non-interactive / prompt if interactive)
Pulse->>Pulse: resolve GH_TOKEN at runtime (gh -> gopass)
Pulse-->>macLaunchd/Cron: scheduled runs invoke pulse.sh
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
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 |
🔍 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 Feb 19 16:35:43 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 correctly addresses the issue of launchd schedulers not being installed on macOS by setup.sh. The logic to detect existing cron jobs and migrate them to launchd is a great addition.
I've noticed significant code duplication between the setup logic for the auto-update scheduler and the supervisor pulse scheduler. To improve maintainability and adhere to the project's rule of extracting repeated logic, I've suggested refactoring this into a single helper function. This will make the code cleaner and easier to manage in the future.
![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 (2)
setup.sh (2)
563-563: Add-Fto crongrepfor consistency with the launchd checks above.Both launchd lookups (lines 526, 561) use
grep -qF(fixed-string), but the cron fallback checks on lines 528 and 563 use plaingrep -q. The strings are safe for regex today, but-Fmakes intent explicit and is consistent with the pair above it.♻️ Proposed fix
- elif crontab -l 2>/dev/null | grep -q "aidevops-supervisor-pulse"; then + elif crontab -l 2>/dev/null | grep -qF "aidevops-supervisor-pulse"; thenThe same applies to line 528:
grep -q "aidevops-auto-update"→grep -qF "aidevops-auto-update".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.sh` at line 563, Update the two cron fallback grep calls to use fixed-string matching like the launchd checks: change the checks that call crontab -l | grep -q "aidevops-auto-update" and crontab -l | grep -q "aidevops-supervisor-pulse" to use grep -qF with the same quoted search strings so the intent is explicit and consistent with the launchd grep -qF checks.
556-559: Consider adding an env-var opt-out for the supervisor pulse, consistent with the auto-update guard.The auto-update block (line 524) is gated by
[[ "${AIDEVOPS_AUTO_UPDATE:-true}" != "false" ]], which lets CI/CD or power users suppress auto-installation via an environment variable. The supervisor pulse block has no equivalent, so there is no non-destructive way to skip it in automated environments.♻️ Suggested guard for parity
- if [[ -x "$supervisor_script" ]]; then + if [[ -x "$supervisor_script" ]] && [[ "${AIDEVOPS_SUPERVISOR_PULSE:-true}" != "false" ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.sh` around lines 556 - 559, Add an opt-out env var guard for the supervisor pulse block similar to the auto-update guard: introduce and check something like AIDEVOPS_SUPERVISOR_PULSE (defaulting to "true") before using the supervisor_script variable and running supervisor-helper.sh; update the conditional that currently tests [[ -x "$supervisor_script" ]] to also require [[ "${AIDEVOPS_SUPERVISOR_PULSE:-true}" != "false" ]] so CI/users can set AIDEVOPS_SUPERVISOR_PULSE="false" to skip the supervisor pulse installation.
🤖 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 563-570: The migration currently suppresses errors from invoking
the cron installer (bash "$supervisor_script" cron install >/dev/null 2>&1 ||
true) and then unconditionally sets _pulse_installed=true, which hides failures;
change the logic so you capture the exit status of the cron install call and
only set _pulse_installed=true when that call succeeds (or when a launchd plist
is detected/created), and if it fails emit a visible warning or error via
print_error/print_warn instead of silently swallowing the failure; update the
macOS migration branch that calls supervisor-helper.sh (the invocation using
"$supervisor_script" cron install) to perform this check and conditional
assignment.
---
Nitpick comments:
In `@setup.sh`:
- Line 563: Update the two cron fallback grep calls to use fixed-string matching
like the launchd checks: change the checks that call crontab -l | grep -q
"aidevops-auto-update" and crontab -l | grep -q "aidevops-supervisor-pulse" to
use grep -qF with the same quoted search strings so the intent is explicit and
consistent with the launchd grep -qF checks.
- Around line 556-559: Add an opt-out env var guard for the supervisor pulse
block similar to the auto-update guard: introduce and check something like
AIDEVOPS_SUPERVISOR_PULSE (defaulting to "true") before using the
supervisor_script variable and running supervisor-helper.sh; update the
conditional that currently tests [[ -x "$supervisor_script" ]] to also require
[[ "${AIDEVOPS_SUPERVISOR_PULSE:-true}" != "false" ]] so CI/users can set
AIDEVOPS_SUPERVISOR_PULSE="false" to skip the supervisor pulse installation.
setup.sh
Outdated
| elif crontab -l 2>/dev/null | grep -q "aidevops-supervisor-pulse"; then | ||
| if [[ "$(uname -s)" == "Darwin" ]]; then | ||
| # macOS: cron entry exists but no launchd plist — migrate silently | ||
| bash "$supervisor_script" cron install >/dev/null 2>&1 || true | ||
| print_info "Supervisor pulse migrated from cron to launchd" | ||
| fi | ||
| _pulse_installed=true | ||
| fi |
There was a problem hiding this comment.
Silent migration failure leaves _pulse_installed=true set, permanently skipping re-installation.
If supervisor-helper.sh cron install fails (line 566), || true suppresses the error and execution falls through to line 569 where _pulse_installed=true is set unconditionally. The scheduler ends up neither installed nor re-attempted, with no user-visible signal. The same pattern exists in the auto-update migration (lines 531/534) but is being repeated here in new code.
Consider checking migration success and only suppressing on macOS when a launchd plist is actually created, or at minimum emit a warning on failure:
🛡️ Proposed fix
elif crontab -l 2>/dev/null | grep -q "aidevops-supervisor-pulse"; then
if [[ "$(uname -s)" == "Darwin" ]]; then
# macOS: cron entry exists but no launchd plist — migrate silently
- bash "$supervisor_script" cron install >/dev/null 2>&1 || true
- print_info "Supervisor pulse migrated from cron to launchd"
+ if bash "$supervisor_script" cron install >/dev/null 2>&1; then
+ print_info "Supervisor pulse migrated from cron to launchd"
+ else
+ print_warning "Supervisor pulse cron-to-launchd migration failed; will re-attempt install"
+ _pulse_installed=false
+ fi
fi
- _pulse_installed=true
+ [[ "$_pulse_installed" != "false" ]] && _pulse_installed=true
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@setup.sh` around lines 563 - 570, The migration currently suppresses errors
from invoking the cron installer (bash "$supervisor_script" cron install
>/dev/null 2>&1 || true) and then unconditionally sets _pulse_installed=true,
which hides failures; change the logic so you capture the exit status of the
cron install call and only set _pulse_installed=true when that call succeeds (or
when a launchd plist is detected/created), and if it fails emit a visible
warning or error via print_error/print_warn instead of silently swallowing the
failure; update the macOS migration branch that calls supervisor-helper.sh (the
invocation using "$supervisor_script" cron install) to perform this check and
conditional assignment.
…intext GH_TOKEN (t1260) - Rename labels: com.aidevops.X -> com.aidevops.aidevops-X for all 3 LaunchAgents Shows as 'aidevops-supervisor-pulse' etc in macOS System Settings Login Items - Auto-migrate old labels to new on install/enable (unload old, install new) - Remove GH_TOKEN from plist and crontab — was stored in plaintext - Add runtime GH_TOKEN resolution in pulse.sh (gh auth token -> gopass fallback) - setup.sh detects both old and new labels, migrates old ones during update
🔍 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 Feb 19 16:46:41 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
macOS derives the Login Items display name from ProgramArguments[0] filename,
not the Label. Create symlinks at ~/.aidevops/bin/aidevops-{supervisor-pulse,
auto-update,todo-watcher} pointing to the real scripts, and use those in the
plist ProgramArguments. System Settings now shows 'aidevops-supervisor-pulse'
and 'aidevops-auto-update' instead of 'bash' and 'auto-update-helper.sh'.
…rror handling (t1260) - Use grep -qF (fixed-string) for cron checks, consistent with launchd checks - Add AIDEVOPS_SUPERVISOR_PULSE env var opt-out, matching AIDEVOPS_AUTO_UPDATE - Surface migration failures as warnings instead of silently swallowing them
🔍 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 Feb 19 16:56:39 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.agents/scripts/auto-update-helper.sh (1)
576-606:⚠️ Potential issue | 🟡 MinorMigration path skips the display symlink (Login Items name stays raw).
Because_migrate_cron_to_launchdruns at Line 585 before the display symlink is created at Line 597, migrated entries will keep the raw script path inProgramArguments[0]. That defeats the new friendly name goal for users coming from cron. Move symlink creation earlier and passdisplay_linkinto migration.🔧 Suggested fix
- # Auto-migrate existing cron entry if present - _migrate_cron_to_launchd "$script_path" "$interval_seconds" - - # Install if not already loaded - if _launchd_is_loaded; then + # Create named symlink so macOS System Settings shows "aidevops-auto-update" + # instead of the raw script name (t1260) + local bin_dir="$HOME/.aidevops/bin" + mkdir -p "$bin_dir" + local display_link="$bin_dir/aidevops-auto-update" + ln -sf "$script_path" "$display_link" + + # Auto-migrate existing cron entry if present (use display link) + _migrate_cron_to_launchd "$display_link" "$interval_seconds" + + # Install if not already loaded + if _launchd_is_loaded; then print_info "Auto-update LaunchAgent already loaded ($LAUNCHD_LABEL)" update_state "enable" "$(get_local_version)" "enabled" return 0 fi - - # Create named symlink so macOS System Settings shows "aidevops-auto-update" - # instead of the raw script name (t1260) - local bin_dir="$HOME/.aidevops/bin" - mkdir -p "$bin_dir" - local display_link="$bin_dir/aidevops-auto-update" - ln -sf "$script_path" "$display_link"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/auto-update-helper.sh around lines 576 - 606, The migration runs before creating the user-friendly symlink so migrated LaunchAgents keep the raw script path; move creation of the display symlink (the ln -sf that sets display_link="$bin_dir/aidevops-auto-update") to before calling _migrate_cron_to_launchd and change the _migrate_cron_to_launchd invocation to pass display_link instead of script_path (or accept an extra display_path parameter) so migrated entries use display_link as ProgramArguments[0]; ensure _generate_auto_update_plist still receives display_link and interval_seconds unchanged.setup.sh (2)
559-561:⚠️ Potential issue | 🟡 Minor
set -euo pipefailwill exit the entire script if these optional installs fail.
setup.shhasset -euo pipefail(line 2). A bare command in thethenbody of anifstatement is subject toset -e. Ifbash "$auto_update_script" enable(line 559) orbash "$supervisor_script" cron install(line 604) returns non-zero, the script terminates mid-setup — poor UX for what are explicitly optional, post-setup steps.🛡️ Proposed fix for both interactive paths
if [[ "$enable_auto" =~ ^[Yy]?$ || -z "$enable_auto" ]]; then - bash "$auto_update_script" enable + bash "$auto_update_script" enable || print_warning "Auto-update enable failed. Run: aidevops auto-update enable" elseif [[ "$enable_pulse" =~ ^[Yy]?$ || -z "$enable_pulse" ]]; then - bash "$supervisor_script" cron install + bash "$supervisor_script" cron install || print_warning "Supervisor pulse enable failed. Run: supervisor-helper.sh cron install" elseAlso applies to: 604-606
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.sh` around lines 559 - 561, Optional post-setup commands (bash "$auto_update_script" enable and bash "$supervisor_script" cron install) are currently subject to set -e and will abort the whole script on failure; change each bare invocation so failures are caught and reported without exiting (e.g., run the command in an if or append a || handler that calls print_warning/print_info), use the unique command references bash "$auto_update_script" enable and bash "$supervisor_script" cron install to locate the spots, and ensure the else branch prints the existing “Skipped. Enable later…” message or a warning including the child command's exit status.
549-551:⚠️ Potential issue | 🟡 MinorNon-interactive path prints unconditional success even on installation failure.
Both the auto-update (lines 549–551) and supervisor pulse (lines 594–596) non-interactive paths use
|| trueto suppress failure, then unconditionally print a success message. A user or CI log will always read "enabled" regardless of whether the underlyingenable/cron installactually succeeded.🛡️ Proposed fix for both blocks
- bash "$auto_update_script" enable >/dev/null 2>&1 || true - print_info "Auto-update enabled (every 10 min). Disable: aidevops auto-update disable" + if bash "$auto_update_script" enable >/dev/null 2>&1; then + print_info "Auto-update enabled (every 10 min). Disable: aidevops auto-update disable" + else + print_warning "Auto-update enable failed. Run: aidevops auto-update enable" + fi- bash "$supervisor_script" cron install >/dev/null 2>&1 || true - print_info "Supervisor pulse enabled (every 2 min). Disable: supervisor-helper.sh cron uninstall" + if bash "$supervisor_script" cron install >/dev/null 2>&1; then + print_info "Supervisor pulse enabled (every 2 min). Disable: supervisor-helper.sh cron uninstall" + else + print_warning "Supervisor pulse enable failed. Run: supervisor-helper.sh cron install" + fiAlso applies to: 594-596
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.sh` around lines 549 - 551, The non-interactive calls that run the auto-update enable and the supervisor pulse cron install (invoked via the variable auto_update_script and the cron install command for supervisor pulse) swallow failures via "|| true" and then always call print_info "Auto-update enabled..." (and the equivalent print_info for supervisor pulse); change both blocks to capture the command exit status instead of masking it (remove "|| true"), run the enable/install command redirecting output to a temp log variable or file, and only call print_info when the exit code is zero; on non-zero exit, call print_error (or print_info with a failure message) including the captured error output so logs reflect real success/failure of auto_update_script enable and the supervisor pulse cron install.
🧹 Nitpick comments (1)
setup.sh (1)
537-537: Cacheuname -srather than forking a subshell twice.
$(uname -s)spawns a subshell each call. Both invocations are in the samemainfunction and always evaluate to the same value. A single cached variable at the top of the block avoids the repeated fork.♻️ Proposed refactor
+ local _os + _os="$(uname -s)" ... - if [[ "$(uname -s)" == "Darwin" ]]; then + if [[ "$_os" == "Darwin" ]]; then ... - if [[ "$(uname -s)" == "Darwin" ]]; then + if [[ "$_os" == "Darwin" ]]; thenAlso applies to: 583-583
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.sh` at line 537, Cache the output of uname -s into a local variable (e.g., kernel or uname_s) at the start of the main block and replace both occurrences of "$(uname -s)" in the if checks (the conditional that starts with if [[ "$(uname -s)" == "Darwin" ]] and the similar check around the later occurrence) with that variable to avoid forking a subshell twice; ensure the variable is declared once in the same scope as the checks so both references use the cached value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.agents/scripts/auto-update-helper.sh:
- Around line 576-606: The migration runs before creating the user-friendly
symlink so migrated LaunchAgents keep the raw script path; move creation of the
display symlink (the ln -sf that sets
display_link="$bin_dir/aidevops-auto-update") to before calling
_migrate_cron_to_launchd and change the _migrate_cron_to_launchd invocation to
pass display_link instead of script_path (or accept an extra display_path
parameter) so migrated entries use display_link as ProgramArguments[0]; ensure
_generate_auto_update_plist still receives display_link and interval_seconds
unchanged.
In `@setup.sh`:
- Around line 559-561: Optional post-setup commands (bash "$auto_update_script"
enable and bash "$supervisor_script" cron install) are currently subject to set
-e and will abort the whole script on failure; change each bare invocation so
failures are caught and reported without exiting (e.g., run the command in an if
or append a || handler that calls print_warning/print_info), use the unique
command references bash "$auto_update_script" enable and bash
"$supervisor_script" cron install to locate the spots, and ensure the else
branch prints the existing “Skipped. Enable later…” message or a warning
including the child command's exit status.
- Around line 549-551: The non-interactive calls that run the auto-update enable
and the supervisor pulse cron install (invoked via the variable
auto_update_script and the cron install command for supervisor pulse) swallow
failures via "|| true" and then always call print_info "Auto-update enabled..."
(and the equivalent print_info for supervisor pulse); change both blocks to
capture the command exit status instead of masking it (remove "|| true"), run
the enable/install command redirecting output to a temp log variable or file,
and only call print_info when the exit code is zero; on non-zero exit, call
print_error (or print_info with a failure message) including the captured error
output so logs reflect real success/failure of auto_update_script enable and the
supervisor pulse cron install.
---
Duplicate comments:
In `@setup.sh`:
- Around line 582-591: The assignment _pulse_installed=true is currently
unconditional after detecting an existing cron entry; change it so the flag is
set only when the supervisor is actually installed/running in the target system:
for macOS (uname -s == "Darwin") move _pulse_installed=true into the success
branch of the bash "$supervisor_script" cron install check (the branch that
calls print_info) so it is set only when migration to launchd succeeded, and for
non-macOS keep/set _pulse_installed=true in the existing cron-detected path; use
the existing symbols _pulse_installed, supervisor_script, the bash
"$supervisor_script" cron install result, print_info and print_warning to locate
and implement the conditional placement.
---
Nitpick comments:
In `@setup.sh`:
- Line 537: Cache the output of uname -s into a local variable (e.g., kernel or
uname_s) at the start of the main block and replace both occurrences of "$(uname
-s)" in the if checks (the conditional that starts with if [[ "$(uname -s)" ==
"Darwin" ]] and the similar check around the later occurrence) with that
variable to avoid forking a subshell twice; ensure the variable is declared once
in the same scope as the checks so both references use the cached value.
1 participant
Summary
setup.sh(run byaidevops update) now installs both schedulers on macOS using launchdProblem
PR #1864 (t1219) added launchd LaunchAgent support for macOS, but:
setup.shonly checked cron for auto-update (line 524:crontab -l | grep "aidevops-auto-update") — missed launchd plist detection entirelysupervisor-helper.sh cron install, which most wouldn't know to doResult: the supervisor pulse scheduler was not running for any user, meaning autonomous task dispatch, PR merging, and worker evaluation were all inactive.
Changes
setup.sh— 3 fixes1. Auto-update detection — now checks
launchctl listforcom.aidevops.auto-updatefirst, falls back tocrontab -l. On macOS, if cron entry exists but no launchd plist, re-runsenableto migrate.2. Supervisor pulse install — new section after auto-update that installs the supervisor pulse scheduler. Interactive mode asks the user; non-interactive enables silently. Checks both launchd and cron to avoid duplicate installation.
3. Cron-to-launchd migration — on macOS, when cron entries exist but launchd plists don't, both schedulers are silently migrated during setup. The
enableandcron installcommands handle the actual migration (remove cron entry, install plist).Behaviour Matrix
Verification
bash -npassesRef #1970
Summary by CodeRabbit