t2721: Add user-configurable feature toggles#2728
Conversation
|
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. 📒 Files selected for processing (7)
✨ 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:10:14 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
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 comprehensive feature toggle system for aidevops, providing users with greater control over various functionalities. It includes a default configuration file, a CLI helper for managing toggles, and modifications to existing scripts to respect the toggle settings. This enhancement allows users to customize their aidevops experience and disable features as needed. Highlights
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
|
|
PR has merge conflicts after #2727 was merged (both touch |
![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 unified feature toggle system, a significant improvement for user configuration and managing application features. However, it introduces critical security concerns. Specifically, the use of sed with unvalidated input for writing configuration values can lead to command injection, and the use of eval to set variables from user-configurable files poses a critical risk of arbitrary code execution. A lack of newline validation also allows for configuration injection. Additionally, the implementation has widespread issues with error suppression (2>/dev/null) that could mask important problems, and there's an opportunity to improve maintainability by dynamically generating toggle keys instead of hardcoding them.
| # Validate key is alphanumeric + underscore | ||
| [[ "$key" =~ ^[a-zA-Z_][a-zA-Z0-9_]*$ ]] || continue | ||
| # Store as _FT_<key> | ||
| eval "_FT_${key}=\"\${value}\"" |
There was a problem hiding this comment.
Using eval to set variables from a user-configurable file is a security risk. A malicious value in feature-toggles.conf.defaults could lead to arbitrary code execution. For example, a value like true"; rm -rf /; echo "pwned would be executed. Please use a safer method like printf -v to assign the value.
| eval "_FT_${key}=\"\${value}\"" | |
| printf -v "_FT_${key}" "%s" "${value}" |
References
- Do not use 'eval' to execute dynamically constructed commands in shell scripts due to security risks; use bash arrays and 'read -ra' to store and execute command parts safely, passing complex arguments like JSON payloads as separate elements.
| key="${line%%=*}" | ||
| value="${line#*=}" | ||
| [[ "$key" =~ ^[a-zA-Z_][a-zA-Z0-9_]*$ ]] || continue | ||
| eval "_FT_${key}=\"\${value}\"" |
There was a problem hiding this comment.
Using eval to set variables from a user-configurable file (~/.config/aidevops/feature-toggles.conf) is a security risk. A malicious value in the config file could lead to arbitrary code execution. For example, a value like true"; rm -rf /; echo "pwned would be executed. Please use a safer method like printf -v to assign the value.
| eval "_FT_${key}=\"\${value}\"" | |
| printf -v "_FT_${key}" "%s" "${value}" |
References
- Do not use 'eval' to execute dynamically constructed commands in shell scripts due to security risks; use bash arrays and 'read -ra' to store and execute command parts safely, passing complex arguments like JSON payloads as separate elements.
| return 0 | ||
| fi | ||
|
|
||
| if grep -qE "^${key}=" "$USER_CONFIG" 2>/dev/null; then |
There was a problem hiding this comment.
Suppressing stderr with 2>/dev/null can hide important errors, such as file permission issues. The file's existence is checked before this line, but it might not be readable. It's better to let the script fail with a clear error message if grep cannot read the file.
| if grep -qE "^${key}=" "$USER_CONFIG" 2>/dev/null; then | |
| if grep -qE "^${key}=" "$USER_CONFIG"; then |
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.
| # Validate key exists in defaults | ||
| local default_val | ||
| default_val=$(_get_default "$key") | ||
| if [[ -z "$default_val" ]] && ! grep -qE "^${key}=" "$DEFAULTS_CONFIG" 2>/dev/null; then |
There was a problem hiding this comment.
Suppressing stderr with 2>/dev/null can hide important errors, such as file permission issues with $DEFAULTS_CONFIG. Since set -e is active, it's better to let the script fail with a clear error message rather than continuing with potentially incorrect assumptions.
| if [[ -z "$default_val" ]] && ! grep -qE "^${key}=" "$DEFAULTS_CONFIG" 2>/dev/null; then | |
| if [[ -z "$default_val" ]] && ! grep -qE "^${key}=" "$DEFAULTS_CONFIG"; then |
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.
| if [[ "${AIDEVOPS_TOOL_AUTO_UPDATE:-}" == "false" ]]; then | ||
| log_info "Tool auto-update disabled via AIDEVOPS_TOOL_AUTO_UPDATE=false" | ||
| # Opt-out via feature toggle (env var or config file) | ||
| if ! is_feature_enabled tool_auto_update 2>/dev/null; then |
There was a problem hiding this comment.
Suppressing stderr with 2>/dev/null can hide important errors. If the is_feature_enabled function is not found, this will be silenced, making debugging difficult. It's better to let the script fail with a clear error message, as set -e is active.
| if ! is_feature_enabled tool_auto_update 2>/dev/null; then | |
| if ! is_feature_enabled tool_auto_update; then |
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.
| else | ||
| print_info "OpenCode config management disabled via feature toggle" | ||
| fi | ||
| if is_feature_enabled manage_claude_config 2>/dev/null; then |
There was a problem hiding this comment.
Suppressing stderr with 2>/dev/null can hide important errors. If the is_feature_enabled function is not found, this will be silenced, making debugging difficult. It's better to let the script fail with a clear error message, as set -e is active.
| if is_feature_enabled manage_claude_config 2>/dev/null; then | |
| if is_feature_enabled manage_claude_config; then |
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.
| local auto_update_script="$HOME/.aidevops/agents/scripts/auto-update-helper.sh" | ||
| if [[ -x "$auto_update_script" ]] && [[ "${AIDEVOPS_AUTO_UPDATE:-true}" != "false" ]]; then | ||
| if [[ -x "$auto_update_script" ]] && is_feature_enabled auto_update 2>/dev/null; then |
There was a problem hiding this comment.
Suppressing stderr with 2>/dev/null can hide important errors. If the is_feature_enabled function is not found, this will be silenced, making debugging difficult. It's better to let the script fail with a clear error message, as set -e is active.
| if [[ -x "$auto_update_script" ]] && is_feature_enabled auto_update 2>/dev/null; then | |
| if [[ -x "$auto_update_script" ]] && is_feature_enabled auto_update; then |
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.
| @@ -700,7 +723,8 @@ main() { | |||
| # macOS: launchd plist invoking wrapper | Linux: cron entry invoking wrapper | |||
| # The plist is ALWAYS regenerated on setup.sh to pick up config changes (env vars, | |||
| # thresholds). Only the first-install prompt is gated on _pulse_installed. | |||
| if [[ "${AIDEVOPS_SUPERVISOR_PULSE:-true}" != "false" ]]; then | |||
| # Respects feature toggle: aidevops config set supervisor_pulse false | |||
| if is_feature_enabled supervisor_pulse 2>/dev/null; then | |||
There was a problem hiding this comment.
Suppressing stderr with 2>/dev/null can hide important errors. If the is_feature_enabled function is not found, this will be silenced, making debugging difficult. It's better to let the script fail with a clear error message, as set -e is active.
| if is_feature_enabled supervisor_pulse 2>/dev/null; then | |
| if is_feature_enabled supervisor_pulse; then |
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.
| local toggle_keys="auto_update update_interval skill_auto_update skill_freshness_hours tool_auto_update tool_freshness_hours tool_idle_hours supervisor_pulse repo_sync openclaw_auto_update openclaw_freshness_hours manage_opencode_config manage_claude_config session_greeting safety_hooks shell_aliases onboarding_prompt" | ||
| local tk env_var env_val | ||
| for tk in $toggle_keys; do | ||
| env_var=$(_ft_env_map "$tk") | ||
| if [[ -n "$env_var" ]]; then | ||
| env_val="${!env_var:-}" | ||
| if [[ -n "$env_val" ]]; then | ||
| eval "_FT_${tk}=\"\${env_val}\"" | ||
| fi | ||
| fi | ||
| done |
There was a problem hiding this comment.
The list of toggle_keys is hardcoded. This creates a maintenance burden, as this list must be manually updated whenever a new toggle with an environment variable mapping is added to feature-toggles.conf.defaults. To improve maintainability, consider generating this list dynamically by parsing the keys from the $FEATURE_TOGGLES_DEFAULTS file.
| if [[ -z "$key" || -z "$value" ]]; then | ||
| print_error "Usage: aidevops config set <key> <value>" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
The value argument in cmd_set is not validated for newlines. An attacker can inject newlines to add arbitrary key-value pairs to the configuration file, bypassing the key validation check performed at the beginning of the function. Since these values are later processed by eval in shared-constants.sh (though currently safely quoted), this could lead to unexpected behavior or future vulnerabilities.
Recommendation: Validate that the value argument does not contain newlines before processing it.
| if [[ -z "$key" || -z "$value" ]]; then | |
| print_error "Usage: aidevops config set <key> <value>" | |
| return 1 | |
| fi | |
| if [[ -z "$key" || -z "$value" ]]; then | |
| print_error "Usage: aidevops config set <key> <value>" | |
| return 1 | |
| fi | |
| if [[ "$value" == *$'\n'* ]]; then | |
| print_error "Value cannot contain newlines" | |
| return 1 | |
| fi |
|
These configs are not enough. EVERYTHING needs to be configurable. The skills used, every question in onboarding, every question in setup.sh, which agents aidevops injects its context into, which skills/commands are downloaded/updated, which tools are installed, etc. etc. etc. |
|
Also it should be jsonc, not .conf |
|
Killed stuck worker (PID 2723667, 3h+ uptime, 5375 messages, 0 commits — thrashing). Supervisor will resolve conflicts directly via git rebase and push. |
alex-solovyev
force-pushed
the
feature/feature-toggles
branch
from
b1b9f5d to
22e868e
Compare
|
Supervisor resolved merge conflicts directly via git rebase. Conflicts were in |
🔍 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:21:57 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
marcusquinn
force-pushed
the
feature/feature-toggles
branch
from
22e868e to
031a5ba
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: Mon Mar 2 18:25:57 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
Add a unified feature toggle system so users can disable auto-updates, opt out of opencode.json/claude config management, and control other opinionated behaviours without needing /onboarding. New files: - .agents/configs/feature-toggles.conf.defaults: documented defaults - .agents/scripts/feature-toggle-helper.sh: CLI for aidevops config Modified: - shared-constants.sh: _load_feature_toggles(), get_feature_toggle(), is_feature_enabled() with env var > user config > defaults priority - aidevops.sh: config command routing + help text - setup.sh: guards for auto-update, supervisor pulse, repo-sync, onboarding prompt, safety hooks, and config management - setup-modules/config.sh: guards for opencode + claude config - auto-update-helper.sh: replaced raw env var checks with is_feature_enabled for skill, tool, and openclaw sub-updates Config: ~/.config/aidevops/feature-toggles.conf (shell-sourceable) CLI: aidevops config [list|get|set|reset|path|help] Closes #2721
alex-solovyev
force-pushed
the
feature/feature-toggles
branch
from
031a5ba to
767c97e
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: Mon Mar 2 18:29:18 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
3 participants
Summary
aidevops config) so users can disable auto-updates, opt out of opencode.json/claude config management, supervisor pulse, repo-sync, onboarding prompts, and safety hooks — without needing/onboarding~/.config/aidevops/feature-toggles.conf(shell-sourceable key=value), with documented defaults shipped at.agents/configs/feature-toggles.conf.defaultsAIDEVOPS_AUTO_UPDATE=false) continue to workChanges
New files
.agents/configs/feature-toggles.conf.defaults— All 17 toggles with documentation.agents/scripts/feature-toggle-helper.sh— CLI helper forlist,get,set,reset,path,helpModified files
shared-constants.sh— Added_load_feature_toggles(),get_feature_toggle(),is_feature_enabled()with env var mappingaidevops.sh— Addedconfigcommand routing inmain()+ help textsetup.sh— Guards for auto-update, supervisor pulse, repo-sync, onboarding prompt, safety hooks, and config management callssetup-modules/config.sh— Guards forupdate_opencode_config()andupdate_claude_config()auto-update-helper.sh— Replaced raw env var checks withis_feature_enabledfor skill, tool, and openclaw sub-updatesTesting
aidevops config list,get,set,reset,path,helpall verifiedis_feature_enabledverified in bash contextCloses #2721