feat: auto-update opt-out via settings.json#2727
Conversation
Add ~/.config/aidevops/settings.json as a persistent config file for user preferences. setup.sh and auto-update-helper.sh read this config before scheduling launchd/cron jobs. If auto_update is false, skip scheduling. - Add get_setting() utility to shared-constants.sh (handles JSON boolean false correctly via jq has() instead of // which treats false as empty) - Gate auto-update, supervisor pulse, and repo sync on settings.json - Priority: env var > settings.json > default (true) - Update auto-update-helper.sh help, status, and check commands - Document settings.json in services.md with supported keys table Closes #2722
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 introduces a new, persistent configuration mechanism for user preferences through a 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
|
WalkthroughThis PR implements a persistent configuration system for aidevops by introducing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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: Mon Mar 2 18:09:44 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.
🧹 Nitpick comments (1)
setup.sh (1)
654-665: Consider extracting repeated jq pattern into a local helper.The same
jq -r 'if has("key") then .key | tostring else "true" end'pattern appears three times. Sincesetup.shdoesn't sourceshared-constants.sh(whereget_setting()lives), you could either:
- Source
shared-constants.shearly and useget_settingdirectly- Add a small local helper at the top of
main()This would reduce the maintenance burden if the settings file path or jq pattern ever changes.
♻️ Option 2: Local helper function
Add near the top of
main():# Local settings reader (shared-constants.sh not sourced in setup.sh) _get_setting() { local key="$1" default="$2" if [[ -f "$HOME/.config/aidevops/settings.json" ]] && command -v jq &>/dev/null; then jq -r --arg k "$key" 'if has($k) then .[$k] | tostring else "'"$default"'" end' \ "$HOME/.config/aidevops/settings.json" 2>/dev/null || echo "$default" else echo "$default" fi }Then replace the three inline blocks with:
local _auto_update_setting="${AIDEVOPS_AUTO_UPDATE:-$(_get_setting "auto_update" "true")}" local _pulse_setting="${AIDEVOPS_SUPERVISOR_PULSE:-$(_get_setting "supervisor_pulse" "true")}" local _repo_sync_setting="${AIDEVOPS_REPO_SYNC:-$(_get_setting "repo_sync" "true")}"Also applies to: 713-722, 853-863
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.sh` around lines 654 - 665, The repeated jq pattern for reading settings should be extracted into a small local helper (e.g., _get_setting) declared near the top of main() so setup.sh doesn't duplicate the jq logic; implement _get_setting(key, default) to check for the settings.json file and jq availability, return the key's value as string or the default on error, then replace the three inline blocks that set _auto_update_setting, _pulse_setting, and _repo_sync_setting with calls like "${AIDEVOPS_AUTO_UPDATE:-$(_get_setting "auto_update" "true")}" to centralize behavior and reduce duplication; reference the helper name _get_setting and main() when making edits.
🤖 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.sh`:
- Around line 654-665: The repeated jq pattern for reading settings should be
extracted into a small local helper (e.g., _get_setting) declared near the top
of main() so setup.sh doesn't duplicate the jq logic; implement
_get_setting(key, default) to check for the settings.json file and jq
availability, return the key's value as string or the default on error, then
replace the three inline blocks that set _auto_update_setting, _pulse_setting,
and _repo_sync_setting with calls like "${AIDEVOPS_AUTO_UPDATE:-$(_get_setting
"auto_update" "true")}" to centralize behavior and reduce duplication; reference
the helper name _get_setting and main() when making edits.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.agents/reference/services.md.agents/scripts/auto-update-helper.sh.agents/scripts/shared-constants.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 introduces a persistent configuration file for user settings, which is a great feature for improving user experience. The implementation is solid, but I've identified a few areas for improvement. My main concerns are around code duplication in setup.sh and some logic in auto-update-helper.sh that doesn't fully respect the new configuration priority, which could lead to confusing behavior. I've also suggested removing stderr suppression from jq calls to improve debuggability, in line with the project's conventions.
| local _auto_update_setting="${AIDEVOPS_AUTO_UPDATE:-}" | ||
| if [[ -z "$_auto_update_setting" ]]; then | ||
| # No env var — check settings.json (requires jq; defaults to "true") | ||
| if [[ -f "$HOME/.config/aidevops/settings.json" ]] && command -v jq &>/dev/null; then | ||
| _auto_update_setting=$(jq -r 'if has("auto_update") then .auto_update | tostring else "true" end' "$HOME/.config/aidevops/settings.json" 2>/dev/null) || _auto_update_setting="true" | ||
| else | ||
| _auto_update_setting="true" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
The logic for reading a setting by checking an environment variable and falling back to settings.json is duplicated three times in this file (for auto_update here, supervisor_pulse at lines 714-721, and repo_sync at lines 855-862). This creates redundancy and makes the script harder to maintain.
Additionally, the jq command used in these blocks suppresses stderr with 2>/dev/null. This hides errors if settings.json is malformed, making it difficult for users to debug configuration problems. This goes against the project's general rule to keep error messages visible.
This logic should be extracted into a local helper function within setup.sh to address both the duplication and the error suppression in one place.
References
- In shell scripts, extract repeated logic into an internal helper function to improve maintainability. This applies even for standalone scripts where external
sourcedependencies are avoided. - When reporting errors for failed file operations in shell scripts, such as 'jq' writes, include the file path in the error message. Avoid suppressing stderr with '2>/dev/null' to ensure that diagnostic information about malformed files or write failures is visible.
- 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.
| if [[ "${AIDEVOPS_AUTO_UPDATE:-}" == "false" ]]; then | ||
| log_info "Auto-update disabled via AIDEVOPS_AUTO_UPDATE=false" | ||
| return 0 | ||
| fi | ||
|
|
||
| # Check settings.json (only when env var is not set) | ||
| if [[ -z "${AIDEVOPS_AUTO_UPDATE:-}" ]] && [[ "$(get_setting "auto_update" "true")" == "false" ]]; then | ||
| log_info "Auto-update disabled via settings.json (auto_update: false)" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
The logic to check if auto-update is disabled is split across two if statements, which involves checking the AIDEVOPS_AUTO_UPDATE variable twice. This can be made more concise and readable by combining them into a single if/elif structure.
| if [[ "${AIDEVOPS_AUTO_UPDATE:-}" == "false" ]]; then | |
| log_info "Auto-update disabled via AIDEVOPS_AUTO_UPDATE=false" | |
| return 0 | |
| fi | |
| # Check settings.json (only when env var is not set) | |
| if [[ -z "${AIDEVOPS_AUTO_UPDATE:-}" ]] && [[ "$(get_setting "auto_update" "true")" == "false" ]]; then | |
| log_info "Auto-update disabled via settings.json (auto_update: false)" | |
| return 0 | |
| fi | |
| if [[ "${AIDEVOPS_AUTO_UPDATE:-}" == "false" ]]; then | |
| log_info "Auto-update disabled via AIDEVOPS_AUTO_UPDATE=false" | |
| return 0 | |
| elif [[ -z "${AIDEVOPS_AUTO_UPDATE:-}" ]] && [[ "$(get_setting "auto_update" "true")" == "false" ]]; then | |
| log_info "Auto-update disabled via settings.json (auto_update: false)" | |
| return 0 | |
| fi |
| if [[ "$(get_setting "auto_update" "true")" == "false" ]]; then | ||
| print_warning "settings.json has auto_update: false — scheduler will install but checks will be skipped" | ||
| print_info "To enable: set auto_update to true in ~/.config/aidevops/settings.json" | ||
| fi |
There was a problem hiding this comment.
The warning logic here is incorrect as it doesn't respect the configuration priority (env var > settings.json). It will show a warning if auto_update: false is in settings.json, even if the user has correctly overridden it with AIDEVOPS_AUTO_UPDATE=true. The check should only trigger if the environment variable is not set.
| if [[ "$(get_setting "auto_update" "true")" == "false" ]]; then | |
| print_warning "settings.json has auto_update: false — scheduler will install but checks will be skipped" | |
| print_info "To enable: set auto_update to true in ~/.config/aidevops/settings.json" | |
| fi | |
| if [[ -z "${AIDEVOPS_AUTO_UPDATE:-}" ]] && [[ "$(get_setting "auto_update" "true")" == "false" ]]; then | |
| print_warning "settings.json has auto_update: false — scheduler will install but checks will be skipped" | |
| print_info "To enable: set auto_update to true in ~/.config/aidevops/settings.json or set AIDEVOPS_AUTO_UPDATE=true" | |
| fi |
| # Check settings.json overrides | ||
| local _settings_auto_update | ||
| _settings_auto_update="$(get_setting "auto_update" "true")" | ||
| if [[ "$_settings_auto_update" == "false" ]]; then | ||
| echo "" | ||
| echo -e " ${YELLOW}Note: settings.json has auto_update: false (checks will be skipped)${NC}" | ||
| fi | ||
|
|
||
| # Check env var overrides (env vars take priority over settings.json) | ||
| if [[ "${AIDEVOPS_AUTO_UPDATE:-}" == "false" ]]; then | ||
| echo "" | ||
| echo -e " ${YELLOW}Note: AIDEVOPS_AUTO_UPDATE=false is set (overrides scheduler)${NC}" | ||
| echo -e " ${YELLOW}Note: AIDEVOPS_AUTO_UPDATE=false is set (overrides scheduler and settings.json)${NC}" | ||
| fi |
There was a problem hiding this comment.
The status output can be confusing by showing two separate messages if both settings.json and the environment variable disable auto-updates. To improve clarity, this should be a single message that reflects the effective configuration based on the priority order (env var > settings.json).
| # Check settings.json overrides | |
| local _settings_auto_update | |
| _settings_auto_update="$(get_setting "auto_update" "true")" | |
| if [[ "$_settings_auto_update" == "false" ]]; then | |
| echo "" | |
| echo -e " ${YELLOW}Note: settings.json has auto_update: false (checks will be skipped)${NC}" | |
| fi | |
| # Check env var overrides (env vars take priority over settings.json) | |
| if [[ "${AIDEVOPS_AUTO_UPDATE:-}" == "false" ]]; then | |
| echo "" | |
| echo -e " ${YELLOW}Note: AIDEVOPS_AUTO_UPDATE=false is set (overrides scheduler)${NC}" | |
| echo -e " ${YELLOW}Note: AIDEVOPS_AUTO_UPDATE=false is set (overrides scheduler and settings.json)${NC}" | |
| fi | |
| # Check for overrides (env vars take priority over settings.json) | |
| if [[ "${AIDEVOPS_AUTO_UPDATE:-}" == "false" ]]; then | |
| echo "" | |
| echo -e " ${YELLOW}Note: AIDEVOPS_AUTO_UPDATE=false is set (overrides scheduler and settings.json)${NC}" | |
| elif [[ "$(get_setting "auto_update" "true")" == "false" ]]; then | |
| echo "" | |
| echo -e " ${YELLOW}Note: settings.json has auto_update: false (checks will be skipped)${NC}" | |
| fi |
| local value | ||
| value=$(jq -r --arg k "$key" \ | ||
| 'if has($k) then .[$k] | tostring else "___MISSING___" end' \ | ||
| "$AIDEVOPS_SETTINGS_FILE" 2>/dev/null) || value="___MISSING___" |
There was a problem hiding this comment.
The jq command suppresses stderr using 2>/dev/null. This prevents visibility of errors if the settings.json file is malformed, making it harder for users to debug their configuration. According to the project's general rules, stderr should not be suppressed for such operations. Removing 2>/dev/null will allow jq to report syntax errors in the JSON file while still allowing the function to fall back to the default value gracefully due to the || value="___MISSING___" part.
| "$AIDEVOPS_SETTINGS_FILE" 2>/dev/null) || value="___MISSING___" | |
| "$AIDEVOPS_SETTINGS_FILE") || value="___MISSING___" |
References
- Avoid suppressing stderr with '2>/dev/null' when sourcing files in shell scripts; syntax errors or missing files should remain visible for debugging even if a fallback is provided.
- When reporting errors for failed file operations in shell scripts, such as 'jq' writes, include the file path in the error message. Avoid suppressing stderr with '2>/dev/null' to ensure that diagnostic information about malformed files or write failures is visible.
- In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting when a command like 'jq' fails on an optional lookup. Do not suppress stderr with '2>/dev/null' so that actual syntax or system errors remain visible for debugging.
- 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.
2 participants
Summary
~/.config/aidevops/settings.jsonas a persistent config file for user preferencessetup.shreadsauto_update,supervisor_pulse, andrepo_synckeys before scheduling launchd/cron jobsauto_update: false, the auto-update scheduler is not installed and checks are skippedtrue)Changes
get_setting()utility function. Handles JSON booleanfalsecorrectly (jq's//operator treatsfalseas empty — useshas()+tostringinstead)cmd_checkrespects settings.json,cmd_enablewarns if disabled,cmd_statusshows settings.json state, help text documents the config fileTesting
get_setting()correctly handles booleanfalse, string"false", missing keys, and missing fileCloses #2722
Summary by CodeRabbit