GH#2926: fix supervisor pulse consent model — default OFF, persist consent, never silently re-enable#2936
Conversation
…926) The supervisor pulse (autonomous AI worker dispatch) had a broken consent model that led to users unknowingly running automated workers under their GitHub identity. Four compounding failures: 1. Feature flag defaulted to ON (true) in all config layers 2. Setup prompt defaulted to YES ([Y/n] — Enter enables) 3. Updates silently re-enabled pulse if cron was manually removed 4. No consent record written — no way to distinguish 'consented' from 'never configured and default kicked in' Fix all four: - Default OFF: supervisor_pulse=false in aidevops.defaults.jsonc and feature-toggles.conf.defaults - Default-no prompt: [y/N] — user must explicitly type 'y' to enable - Consent persisted: writes true/false to config.jsonc via cmd_set, survives updates without re-prompting - Never silently re-enables: if config says false, skip entirely and clean up any existing launchd/cron entries - Non-interactive mode: only installs if config explicitly says true (was: silently installed with no consent) - Env var AIDEVOPS_SUPERVISOR_PULSE overrides all (power user escape) Closes #2926
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 user consent model for the autonomous supervisor pulse feature. The changes ensure that the pulse is off by default, requires explicit user permission to enable, and persists that choice across system updates. This prevents unintended activation of autonomous AI workers and provides users with clear control over their system's behavior, addressing a critical privacy and operational concern. 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
|
|
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. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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 |
![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 the supervisor pulse consent issue by defaulting it to off, requiring explicit user consent, and persisting that choice, which is a significant security improvement. However, a command injection vulnerability was identified in the crontab installation logic where variables are used without proper quoting, potentially allowing arbitrary command execution if the repository is installed in a maliciously named directory. Additionally, minor issues were found in setup.sh where error suppression (2>/dev/null) could hide problems, particularly around persisting user consent, and should be addressed to improve script robustness and debuggability.
| _do_install=true | ||
| # Record explicit consent | ||
| if type cmd_set &>/dev/null; then | ||
| cmd_set "orchestration.supervisor_pulse" "true" 2>/dev/null || true |
There was a problem hiding this comment.
The 2>/dev/null here should be removed. If cmd_set fails, the error should be visible to the user so they understand why their consent wasn't persisted. Silently failing means they will be re-prompted on the next run, which undermines a key goal of this change. The || true correctly prevents the script from exiting.
| cmd_set "orchestration.supervisor_pulse" "true" 2>/dev/null || true | |
| cmd_set "orchestration.supervisor_pulse" "true" || true |
References
- In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting on a command's failure, but do not suppress stderr with '2>/dev/null'. This allows important error messages to be visible for debugging, which is crucial for a command that is persisting user consent.
| _do_install=false | ||
| # Record explicit decline so we never re-prompt on updates | ||
| if type cmd_set &>/dev/null; then | ||
| cmd_set "orchestration.supervisor_pulse" "false" 2>/dev/null || true |
There was a problem hiding this comment.
The 2>/dev/null here should be removed. If cmd_set fails, the error should be visible to the user so they understand why their choice to decline wasn't persisted. Silently failing means they will be re-prompted on the next run, which undermines a key goal of this change. The || true correctly prevents the script from exiting.
| cmd_set "orchestration.supervisor_pulse" "false" 2>/dev/null || true | |
| cmd_set "orchestration.supervisor_pulse" "false" || true |
References
- In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting on a command's failure, but do not suppress stderr with '2>/dev/null'. This allows important error messages to be visible for debugging, which is crucial for a command that is persisting user consent.
| # Remove old-style cron entries (direct opencode invocation) | ||
| ( | ||
| crontab -l 2>/dev/null | grep -v 'aidevops: supervisor-pulse' | ||
| echo "*/2 * * * * OPENCODE_BIN=${opencode_bin} PULSE_DIR=${_aidevops_dir} /bin/bash ${wrapper_script} >> $HOME/.aidevops/logs/pulse-wrapper.log 2>&1 # aidevops: supervisor-pulse" |
There was a problem hiding this comment.
The setup.sh script constructs a crontab entry by concatenating variables opencode_bin and _aidevops_dir without proper quoting or sanitization. If the repository is cloned into a directory with a malicious name (e.g., containing shell metacharacters like ; or $()), or if the opencode binary path contains such characters, an attacker could achieve arbitrary command execution when the cron job runs. This is a classic command injection vulnerability where untrusted input (the directory name) is used to build a command that is later executed by a shell.
| echo "*/2 * * * * OPENCODE_BIN=${opencode_bin} PULSE_DIR=${_aidevops_dir} /bin/bash ${wrapper_script} >> $HOME/.aidevops/logs/pulse-wrapper.log 2>&1 # aidevops: supervisor-pulse" | |
| echo "*/2 * * * * OPENCODE_BIN='${opencode_bin}' PULSE_DIR='${_aidevops_dir}' /bin/bash '${wrapper_script}' >> '$HOME/.aidevops/logs/pulse-wrapper.log' 2>&1 # aidevops: supervisor-pulse" |
| # Also check legacy .conf user override | ||
| if [[ -z "$_pulse_user_config" && -f "${FEATURE_TOGGLES_USER:-$HOME/.config/aidevops/feature-toggles.conf}" ]]; then | ||
| local _legacy_val | ||
| _legacy_val=$(grep -E '^supervisor_pulse=' "${FEATURE_TOGGLES_USER:-$HOME/.config/aidevops/feature-toggles.conf}" 2>/dev/null | tail -1 | cut -d= -f2) |
There was a problem hiding this comment.
The 2>/dev/null here should be removed. Since the existence of the file is checked on line 771, this grep should not fail with "file not found". Suppressing stderr here could mask other issues, like file permission errors, preventing the user from understanding why their legacy configuration is not being read.
| _legacy_val=$(grep -E '^supervisor_pulse=' "${FEATURE_TOGGLES_USER:-$HOME/.config/aidevops/feature-toggles.conf}" 2>/dev/null | tail -1 | cut -d= -f2) | |
| _legacy_val=$(grep -E '^supervisor_pulse=' "${FEATURE_TOGGLES_USER:-$HOME/.config/aidevops/feature-toggles.conf}" | tail -1 | cut -d= -f2) |
References
- Avoid using
2>/dev/nullto suppress errors on file operations if the file's existence has already been verified by a preceding check. This is redundant for 'file not found' errors and can mask other important issues like permissions problems.
🔍 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 17:31:31 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
…etup.sh Address 4 findings from PR #2936 review (GH#2950): - HIGH: Remove 2>/dev/null from cmd_set calls (lines 815, 821) so users see errors when consent persistence fails instead of being silently re-prompted on next run. The || true still prevents set -e exits. - MEDIUM: Remove 2>/dev/null from grep on legacy config (line 773) since file existence is already checked on line 771 — suppressing stderr masks permission errors, not missing-file errors. - MEDIUM: Single-quote variable expansions in crontab entry (line 924) to prevent command injection via directory names containing shell metacharacters.
) * fix: t2950 remove error suppression from consent persistence and quote crontab variables Remove 2>/dev/null from cmd_set calls that persist supervisor pulse consent — errors should be visible so users understand why consent wasn't saved. Remove redundant 2>/dev/null from grep where file existence is already checked. Single-quote variables in crontab entry to prevent command injection via directory names with metacharacters. * fix: address CodeRabbit CHANGES_REQUESTED review feedback on PR #2994 1. Replace grep|tail|cut pipeline with awk for legacy config read — grep exits 1 on no match, which aborts the script under set -euo pipefail. awk always exits 0 and handles the missing-key case safely. 2. Add shell_single_quote() utility and use it for crontab path interpolation — prevents syntax errors and command injection when paths contain apostrophes (e.g. /home/O'Connor/...). Both findings verified with functional tests. ShellCheck clean.
1 participant
Summary
Fixes the broken consent model for supervisor pulse installation that led to users unknowingly running autonomous AI workers under their GitHub identity.
supervisor_pulsedefaults tofalsein all config layers (JSONC defaults, legacy.confdefaults)[Y/n]to[y/N]— user must explicitly type "y" to enabletrue/falsetoconfig.jsoncviacmd_setso the choice survives updates without re-promptingfalse, setup skips entirely and cleans up any existing launchd/cron entriestrue(was: silently installed with no consent)AIDEVOPS_SUPERVISOR_PULSE=true|falseoverrides all config layers (power user escape hatch)Files Changed
.agents/configs/aidevops.defaults.jsoncsupervisor_pulse: true→false.agents/configs/feature-toggles.conf.defaultssupervisor_pulse=true→falsesetup.shUpgrade Path
orchestration.supervisor_pulse=truein theirconfig.jsonc(or env var), pulse continues working — no change in behavior.setup.shrun, they'll be prompted with[y/N]. Their choice is persisted.[y/N]. Non-interactive skips entirely.Testing
bash -n setup.sh— syntax validshellcheck setup.sh— zero violationsCloses #2926