t1390: Make pulse-wrapper CodeRabbit sweep conditional on quality changes#2807
t1390: Make pulse-wrapper CodeRabbit sweep conditional on quality changes#2807alex-solovyev wants to merge 2 commits intomainfrom
Conversation
…es (t1390) Only trigger @coderabbitai active review request when quality metrics indicate a problem — gate status changes to FAIL/ERROR, issue count spikes by 10+, or new high/critical severity findings appear. Otherwise post a passive monitoring line to avoid repetitive review requests that CodeRabbit flagged as noise. State is persisted per-repo in quality-sweep-state-{slug}.json so each sweep compares against the previous run. First run always triggers to establish baseline. Closes #2806
|
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 (1)
WalkthroughThe pulse-wrapper.sh script now implements stateful, conditional CodeRabbit triggers. Instead of triggering reviews on every sweep, it tracks quality metrics in per-repository state files and only requests reviews when significant changes occur: quality gate status shifts to ERROR/WARN, issue counts spike ≥10, or new high/critical findings emerge. Otherwise, it posts passive monitoring comments. Changes
Sequence DiagramsequenceDiagram
participant Script as pulse-wrapper.sh
participant Metrics as Quality Tools<br/>(SonarCloud, shellcheck, codacy)
participant StateFile as State File<br/>(per-repo)
participant CodeRabbit as CodeRabbit API
Script->>Metrics: Collect current metrics (gate, issues, H/C findings)
Metrics-->>Script: Return current totals
Script->>StateFile: Load previous state
StateFile-->>Script: Return prior metrics (or empty if first run)
Script->>Script: Compute deltas:<br/>- Gate status change?<br/>- Issues +10 or more?<br/>- New H/C findings?
alt Trigger Conditions Met
Script->>Script: Set _cr_trigger_reason<br/>(gate change / issue spike / new critical)
Script->>CodeRabbit: POST active review request<br/>with trigger reason
CodeRabbit-->>Script: Review initiated
Script->>Script: Log activity event
else No Trigger Conditions
Script->>Script: Build passive monitoring<br/>comment with deltas
Script->>CodeRabbit: POST monitoring status
end
Script->>StateFile: Write current metrics<br/>to state file (JSON)
StateFile-->>Script: State persisted
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
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 |
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 refactors the daily quality sweep to make CodeRabbit review requests conditional, addressing feedback about unnecessary noise from repetitive reviews. It introduces a state-tracking mechanism to monitor changes in quality metrics such as gate status, total issue counts, and high/critical severity findings. An active CodeRabbit review is now only triggered when meaningful regressions or significant changes in quality are detected; otherwise, a passive monitoring message is displayed. 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
|
🔍 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: Wed Mar 4 04:28:27 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 conditional trigger for CodeRabbit reviews during the daily quality sweep, aiming to reduce noise. While the core logic for persisting and comparing quality metrics is sound and robust, utilizing jq for parsing and proper quoting, a potential prompt injection vulnerability has been identified. This occurs in the CodeRabbit trigger section where untrusted metrics are directly embedded into a markdown comment for an LLM, risking model manipulation. Further improvements are also recommended for error handling in shell scripts by avoiding stderr suppression, and for refactoring the logic that constructs trigger reasons to enhance clarity and maintainability.
| if [[ -n "$_cr_trigger_reason" ]]; then | ||
| coderabbit_section="### CodeRabbit | ||
|
|
||
| **Trigger**: ${_cr_trigger_reason} |
There was a problem hiding this comment.
The variable _cr_trigger_reason is directly embedded into the markdown comment sent to CodeRabbit. This variable contains the gate_status (via _cr_current_gate), which is derived from external quality tools. If an attacker can influence the output of these tools to include malicious instructions (e.g., by naming a project or branch in a specific way that ends up in the gate status), they could perform a prompt injection attack against CodeRabbit. This could lead to unintended behavior, such as bypassing security reviews or leaking information from the codebase.
| _cr_current_high_critical=$(echo "$sonar_issues" | jq -r ' | ||
| [.facets[]? | select(.property == "severities") | .values[]? | ||
| | select(.val == "CRITICAL" or .val == "BLOCKER") | .count] | add // 0 | ||
| ' 2>/dev/null) || _cr_current_high_critical=0 |
There was a problem hiding this comment.
Suppressing stderr with 2>/dev/null can hide important diagnostic information, such as syntax errors in the jq query or system-level issues. The existing || _cr_current_high_critical=0 construct already prevents the script from exiting on failure, so the error suppression is not necessary for script continuation and only makes debugging harder. It's better to let potential errors be visible.
| _cr_current_high_critical=$(echo "$sonar_issues" | jq -r ' | |
| [.facets[]? | select(.property == "severities") | .values[]? | |
| | select(.val == "CRITICAL" or .val == "BLOCKER") | .count] | add // 0 | |
| ' 2>/dev/null) || _cr_current_high_critical=0 | |
| _cr_current_high_critical=$(echo "$sonar_issues" | jq -r ' | |
| [.facets[]? | select(.property == "severities") | .values[]? | |
| | select(.val == "CRITICAL" or .val == "BLOCKER") | .count] | add // 0 | |
| ') || _cr_current_high_critical=0 |
References
- 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.
| _cr_prev_gate=$(jq -r '.gate_status // "UNKNOWN"' "$_cr_state_file" 2>/dev/null) || _cr_prev_gate="UNKNOWN" | ||
| _cr_prev_total_issues=$(jq -r '.total_issues // 0' "$_cr_state_file" 2>/dev/null) || _cr_prev_total_issues=0 | ||
| _cr_prev_high_critical=$(jq -r '.high_critical // 0' "$_cr_state_file" 2>/dev/null) || _cr_prev_high_critical=0 |
There was a problem hiding this comment.
Suppressing stderr with 2>/dev/null can hide important diagnostic information, such as a malformed state file or system-level issues with jq. The || construct already prevents the script from exiting on failure, so the error suppression is not necessary for script continuation and only makes debugging harder. It's better to let potential errors be visible.
| _cr_prev_gate=$(jq -r '.gate_status // "UNKNOWN"' "$_cr_state_file" 2>/dev/null) || _cr_prev_gate="UNKNOWN" | |
| _cr_prev_total_issues=$(jq -r '.total_issues // 0' "$_cr_state_file" 2>/dev/null) || _cr_prev_total_issues=0 | |
| _cr_prev_high_critical=$(jq -r '.high_critical // 0' "$_cr_state_file" 2>/dev/null) || _cr_prev_high_critical=0 | |
| _cr_prev_gate=$(jq -r '.gate_status // "UNKNOWN"' "$_cr_state_file") || _cr_prev_gate="UNKNOWN" | |
| _cr_prev_total_issues=$(jq -r '.total_issues // 0' "$_cr_state_file") || _cr_prev_total_issues=0 | |
| _cr_prev_high_critical=$(jq -r '.high_critical // 0' "$_cr_state_file") || _cr_prev_high_critical=0 |
References
- 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.
| if [[ "$_cr_current_gate" == "ERROR" || "$_cr_current_gate" == "WARN" ]] && | ||
| [[ "$_cr_prev_gate" != "$_cr_current_gate" ]]; then | ||
| _cr_trigger_reason="quality gate changed to ${_cr_current_gate} (was ${_cr_prev_gate})" | ||
| fi | ||
|
|
||
| # Condition 2: Total issue count spiked by 10+ | ||
| local _cr_issue_delta=$((_cr_current_total_issues - _cr_prev_total_issues)) | ||
| if [[ "$_cr_issue_delta" -ge 10 ]]; then | ||
| if [[ -n "$_cr_trigger_reason" ]]; then | ||
| _cr_trigger_reason="${_cr_trigger_reason}; issue count spiked by +${_cr_issue_delta}" | ||
| else | ||
| _cr_trigger_reason="issue count spiked by +${_cr_issue_delta} (${_cr_prev_total_issues} -> ${_cr_current_total_issues})" | ||
| fi | ||
| fi | ||
|
|
||
| # Condition 3: New high/critical severity findings | ||
| local _cr_hc_delta=$((_cr_current_high_critical - _cr_prev_high_critical)) | ||
| if [[ "$_cr_hc_delta" -gt 0 ]]; then | ||
| if [[ -n "$_cr_trigger_reason" ]]; then | ||
| _cr_trigger_reason="${_cr_trigger_reason}; +${_cr_hc_delta} new high/critical findings" | ||
| else | ||
| _cr_trigger_reason="+${_cr_hc_delta} new high/critical findings (${_cr_prev_high_critical} -> ${_cr_current_high_critical})" | ||
| fi | ||
| fi | ||
|
|
||
| # Condition 4: First run (no previous state) — trigger to establish baseline | ||
| if [[ ! -f "$_cr_state_file" ]]; then | ||
| _cr_trigger_reason="first sweep — establishing baseline" | ||
| fi |
There was a problem hiding this comment.
The logic for constructing the _cr_trigger_reason can be simplified and made more informative. Currently, if multiple conditions are met, only the first one includes detailed context (like (was X) or (Y -> Z)), while subsequent reasons are less descriptive. Using an array to collect all reasons and then joining them ensures all details are preserved, making the trigger reason more comprehensive.
This refactoring also correctly handles the 'first run' condition as an override, which seems to be the original intent.
local -a _cr_reasons=()
# Condition 1: Quality gate changed to a failing state
if [[ "$_cr_current_gate" == "ERROR" || "$_cr_current_gate" == "WARN" ]] &&
[[ "$_cr_prev_gate" != "$_cr_current_gate" ]]; then
_cr_reasons+=("quality gate changed to ${_cr_current_gate} (was ${_cr_prev_gate})")
fi
# Condition 2: Total issue count spiked by 10+
local _cr_issue_delta=$((_cr_current_total_issues - _cr_prev_total_issues))
if [[ "$_cr_issue_delta" -ge 10 ]]; then
_cr_reasons+=("issue count spiked by +${_cr_issue_delta} (${_cr_prev_total_issues} -> ${_cr_current_total_issues})")
fi
# Condition 3: New high/critical severity findings
local _cr_hc_delta=$((_cr_current_high_critical - _cr_prev_high_critical))
if [[ "$_cr_hc_delta" -gt 0 ]]; then
_cr_reasons+=("+${_cr_hc_delta} new high/critical findings (${_cr_prev_high_critical} -> ${_cr_current_high_critical})")
fi
# Condition 4: First run (no previous state) — trigger to establish baseline
if [[ ! -f "$_cr_state_file" ]]; then
_cr_trigger_reason="first sweep — establishing baseline"
elif [[ ${#_cr_reasons[@]} -gt 0 ]]; then
# Join reasons with "; "
_cr_trigger_reason=$(printf "; %s" "${_cr_reasons[@]}")
_cr_trigger_reason=${_cr_trigger_reason:2} # Remove leading "; "
fi![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 (1)
.agents/scripts/pulse-wrapper.sh (1)
1658-1668: Persist quality state atomically to harden failure recovery.Lines 1658-1668 write directly to the final JSON file. If interrupted mid-write, the next sweep can read partial state and make incorrect trigger decisions. Use tmp+rename.
Proposed refactor
- cat >"$_cr_state_file" <<-CRSTATE + local _cr_state_tmp="${_cr_state_file}.tmp" + cat >"$_cr_state_tmp" <<-CRSTATE { "gate_status": "${_cr_current_gate}", "total_issues": ${_cr_current_total_issues}, "sonar_issues": ${_cr_current_sonar_issues}, "shellcheck_errors": ${_cr_current_sc_errors}, "codacy_issues": ${_cr_current_codacy_issues}, "high_critical": ${_cr_current_high_critical}, "timestamp": "${now_iso}" } CRSTATE + mv "$_cr_state_tmp" "$_cr_state_file"As per coding guidelines: "Automation scripts - focus on: Reliability and robustness" and "Error recovery mechanisms".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/pulse-wrapper.sh around lines 1658 - 1668, The current cat >"$_cr_state_file" <<-CRSTATE block writes the JSON directly and can leave a partial file if interrupted; change it to write to a temp file (e.g., create a secure temp via mktemp like "${_cr_state_file}.tmp.$$" or use mktemp), write the JSON there (using the same HEREDOC/CRSTATE content), fsync or ensure write is complete, then atomically rename/mv the temp to "$_cr_state_file"; ensure any created temp is cleaned on exit/failure (trap) and preserve file permissions if required so reads never see a partial JSON.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/pulse-wrapper.sh:
- Line 1613: The total-issue spike calculation for _cr_current_total_issues
omits shellcheck warnings (_cr_current_sc_warnings), so update the summation
expression used to compute _cr_current_total_issues (and the analogous totals
around the 1663-1665 block) to include _cr_current_sc_warnings along with
_cr_current_sonar_issues, _cr_current_sc_errors and _cr_current_codacy_issues so
warning-only regressions count toward the +10 trigger; locate the variable
assignments named _cr_current_total_issues and any similar total variables and
add _cr_current_sc_warnings to their arithmetic expressions.
---
Nitpick comments:
In @.agents/scripts/pulse-wrapper.sh:
- Around line 1658-1668: The current cat >"$_cr_state_file" <<-CRSTATE block
writes the JSON directly and can leave a partial file if interrupted; change it
to write to a temp file (e.g., create a secure temp via mktemp like
"${_cr_state_file}.tmp.$$" or use mktemp), write the JSON there (using the same
HEREDOC/CRSTATE content), fsync or ensure write is complete, then atomically
rename/mv the temp to "$_cr_state_file"; ensure any created temp is cleaned on
exit/failure (trap) and preserve file permissions if required so reads never see
a partial JSON.
ℹ️ Review info
Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ca76dd1-94d1-4d1e-9bb1-a543d2a7a8c4
📒 Files selected for processing (1)
.agents/scripts/pulse-wrapper.sh
|
Previous worker killed due to extreme thrashing (9679 messages, 0 commits, 3h16m). Re-dispatching to fix failing CI. Resolves #2806 |
… atomically (t1390) - Add _cr_current_sc_warnings to _cr_current_total_issues summation so warning-only regressions count toward the +10 spike trigger - Write state file via tmp+rename (PID-suffixed temp file) to prevent partial writes on interruption - Persist shellcheck_warnings in state JSON for completeness Fixes CodeRabbit review findings on PR #2807. Closes #2806
🔍 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: Wed Mar 4 04:36:11 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
Superseded by #2808 — same t1390 fix with a cleaner implementation. Closing to avoid confusion. |
|
1 participant
Summary
@coderabbitaireview request conditional instead of unconditional, addressing CodeRabbit's own feedback that repetitive requests on stable metrics are noise_Monitoring: N issues, gate PASSING_) when metrics are stableImplementation
Persists sweep metrics per-repo in
~/.aidevops/logs/quality-sweep-state-{slug}.jsonbetween runs. Each sweep compares current metrics (SonarCloud gate status, SonarCloud issues, ShellCheck errors, Codacy issues, high/critical severity counts) against the previous state to decide whether an active CodeRabbit review is warranted.First run always triggers to establish baseline. Subsequent runs only trigger when meaningful changes are detected.
Verification
shellcheckpasses cleanbash -nsyntax check passesCloses #2806
Summary by CodeRabbit