Skip to content

t1390: Make pulse-wrapper CodeRabbit sweep conditional on quality changes #2807

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alex-solovyev wants to merge 2 commits into from
Closed

t1390: Make pulse-wrapper CodeRabbit sweep conditional on quality changes #2807

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ea7415
fix: make pulse-wrapper CodeRabbit sweep conditional on quality chang…
alex-solovyev Mar 4, 2026
f463692
fix: include ShellCheck warnings in total issues and write state file…
alex-solovyev Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 108 additions & 3 deletions .agents/scripts/pulse-wrapper.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1407,6 +1407,7 @@ _ensure_quality_issue() {
_quality_sweep_for_repo() {
local repo_slug="$1"
local repo_path="$2"
local slug_safe="${repo_slug//\//-}"

local issue_number
issue_number=$(_ensure_quality_issue "$repo_slug") || return 0
Expand Down Expand Up @@ -1582,16 +1583,120 @@ ${type_breakdown}
fi
fi

# --- 5. CodeRabbit trigger ---
# Always include @coderabbitai mention to trigger a full codebase review
local coderabbit_section="### CodeRabbit
# --- 5. CodeRabbit trigger (conditional — t1390) ---
# Only trigger active @coderabbitai review when metrics indicate a problem:
# - Quality gate status changed to FAIL/ERROR
# - Total issue count spiked by 10+ since last sweep
# - New high/critical severity findings appeared
# Otherwise, post a passive monitoring line to avoid repetitive review requests.
local coderabbit_section=""
local _cr_trigger_reason=""
local _cr_state_file="${HOME}/.aidevops/logs/quality-sweep-state-${slug_safe}.json"

# Collect current metrics from sections 1-4 (variables already in scope)
local _cr_current_gate="${gate_status:-UNKNOWN}"
local _cr_current_sonar_issues="${total_issues:-0}"
local _cr_current_sc_errors="${sc_errors:-0}"
local _cr_current_sc_warnings="${sc_warnings:-0}"
local _cr_current_codacy_issues="${codacy_total:-0}"

# Count high/critical severity issues from SonarCloud breakdown
local _cr_current_high_critical=0
if [[ -n "${sonar_issues:-}" ]] && echo "$sonar_issues" | jq -e '.facets' &>/dev/null; then
_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
Comment on lines +1606 to +1609
Copy link

@gemini-code-assist gemini-code-assist bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
_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
  1. 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.

fi

# Compute total issue count across all tools for delta comparison
local _cr_current_total_issues=$((_cr_current_sonar_issues + _cr_current_sc_errors + _cr_current_sc_warnings + _cr_current_codacy_issues))

# Load previous state
local _cr_prev_gate="UNKNOWN"
local _cr_prev_total_issues=0
local _cr_prev_high_critical=0
if [[ -f "$_cr_state_file" ]]; then
_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
Comment on lines +1620 to +1622
Copy link

@gemini-code-assist gemini-code-assist bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
_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
  1. 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.

fi

# Decision: should we trigger an active CodeRabbit review?
# 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_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
Comment on lines +1627 to +1655
Copy link

@gemini-code-assist gemini-code-assist bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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


# Save current state for next comparison (atomic write via tmp+rename)
local _cr_tmp_file="${_cr_state_file}.tmp.$$"
cat >"$_cr_tmp_file" <<-CRSTATE
{
"gate_status": "${_cr_current_gate}",
"total_issues": ${_cr_current_total_issues},
"sonar_issues": ${_cr_current_sonar_issues},
"shellcheck_errors": ${_cr_current_sc_errors},
"shellcheck_warnings": ${_cr_current_sc_warnings},
"codacy_issues": ${_cr_current_codacy_issues},
"high_critical": ${_cr_current_high_critical},
"timestamp": "${now_iso}"
}
CRSTATE
mv -f "$_cr_tmp_file" "$_cr_state_file"

# Build CodeRabbit section based on decision
if [[ -n "$_cr_trigger_reason" ]]; then
coderabbit_section="### CodeRabbit

**Trigger**: ${_cr_trigger_reason}
Copy link

@gemini-code-assist gemini-code-assist bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

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.


@coderabbitai Please run a full codebase review of this repository. Focus on:
- Security vulnerabilities and credential exposure
- Shell script quality (error handling, quoting, race conditions)
- Code duplication and maintainability
- Documentation accuracy
"
echo "[pulse-wrapper] Quality sweep: CodeRabbit active review triggered — ${_cr_trigger_reason}" >>"$LOGFILE"
else
# Passive monitoring — no @coderabbitai mention
local _cr_delta_display=""
if [[ "$_cr_issue_delta" -gt 0 ]]; then
_cr_delta_display=" (+${_cr_issue_delta})"
elif [[ "$_cr_issue_delta" -lt 0 ]]; then
_cr_delta_display=" (${_cr_issue_delta})"
fi

coderabbit_section="### CodeRabbit

_Monitoring: ${_cr_current_total_issues} issues${_cr_delta_display}, gate ${_cr_current_gate}. No active review requested — metrics stable._
"
fi
tool_count=$((tool_count + 1))

# --- 6. Merged PR review scanner ---
Expand Down