Skip to content

t4733: quality-feedback-helper.sh: add --include-positive flag to scan-merged #4742

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
marcusquinn wants to merge 1 commit into from
Closed

t4733: quality-feedback-helper.sh: add --include-positive flag to scan-merged #4742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 24 additions & 4 deletions .agents/scripts/quality-feedback-helper.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
# quality-feedback-helper.sh watch --pr 4
# quality-feedback-helper.sh scan-merged --repo owner/repo --batch 20
# quality-feedback-helper.sh scan-merged --repo owner/repo --batch 20 --create-issues
# quality-feedback-helper.sh scan-merged --repo owner/repo --dry-run --include-positive

SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" || exit
source "${SCRIPT_DIR}/shared-constants.sh"
Expand Down Expand Up @@ -564,6 +565,10 @@ cmd_watch() {
# --json Output findings as JSON instead of human-readable
# --dry-run Scan and report findings without creating issues or marking
# PRs as scanned. Useful for identifying false-positive issues.
# --include-positive Disable the positive-review filter. By default, reviews
# with zero inline comments and purely positive/approving body
# text are skipped as false positives. Pass this flag to
# include them — useful for debugging the filter itself.
#
# Returns: 0 on success, 1 on error
#######################################
Expand All @@ -576,6 +581,7 @@ cmd_scan_merged() {
local backfill=false
local tag_actioned=false
local dry_run=false
local include_positive=false

# Parse flags
while [[ $# -gt 0 ]]; do
Expand Down Expand Up @@ -612,6 +618,10 @@ cmd_scan_merged() {
dry_run=true
shift
;;
--include-positive)
include_positive=true
shift
;;
*)
echo "Unknown option for scan-merged: $1" >&2
return 1
Expand Down Expand Up @@ -756,7 +766,7 @@ cmd_scan_merged() {
fi

local findings
findings=$(_scan_single_pr "$repo_slug" "$pr_num" "$min_severity") || {
findings=$(_scan_single_pr "$repo_slug" "$pr_num" "$min_severity" "$include_positive") || {
# In dry-run mode, don't mark PRs as scanned so they can be re-scanned
if [[ "$dry_run" != true ]]; then
gh pr edit "$pr_num" --repo "$repo_slug" --add-label "review-feedback-scanned" >/dev/null 2>&1 || true
Expand Down Expand Up @@ -851,13 +861,15 @@ cmd_scan_merged() {
# $1 - repo slug
# $2 - PR number
# $3 - minimum severity (critical|high|medium)
# $4 - include_positive ("true" to disable positive-review filter; default "false")
# Output: JSON array of findings to stdout
# Returns: 0 on success
#######################################
_scan_single_pr() {
local repo_slug="$1"
local pr_num="$2"
local min_severity="$3"
local include_positive="${4:-false}"

echo -e " Scanning PR #${pr_num}..." >&2

Expand Down Expand Up @@ -934,7 +946,8 @@ _scan_single_pr() {
review_findings=$(printf '%s' "$reviews" | jq \
--arg pr "$pr_num" \
--arg min_sev "$min_severity" \
--argjson inline_counts "$inline_counts_json" '
--argjson inline_counts "$inline_counts_json" \
--argjson include_positive "$([[ "$include_positive" == "true" ]] && echo 'true' || echo 'false')" '
[.[] |
select(.body != null and .body != "" and (.body | length) > 50) |

Expand All @@ -951,8 +964,9 @@ _scan_single_pr() {
# summaries, not actionable findings — capturing them creates false-positive
# quality-debt issues (see GH#4528, incident: issue #3744 / PR #1121).
# Humans and CHANGES_REQUESTED reviews are never skipped by this rule.
# Bypassed when $include_positive is true (--include-positive flag).
(($inline_counts[$login] // 0) == 0 and .state == "COMMENTED" and $reviewer != "human") as $summary_only |
select($summary_only | not) |
select($include_positive or ($summary_only | not)) |

(.body) as $body |
(if ($body | test("security-critical\\.svg|🔴.*critical|CRITICAL"; "i")) then "critical"
Expand Down Expand Up @@ -1010,7 +1024,8 @@ _scan_single_pr() {
# approval-only patterns. This catches "LGTM", "good work", "no further
# comments", and "no further recommendations" (even in long summary
# reviews) regardless of reviewer type or review state.
select((($approval_only or $no_actionable_recommendation or $no_actionable_sentiment) and ($actionable | not)) | not) |
# Bypassed when $include_positive is true (--include-positive flag).
select($include_positive or ((($approval_only or $no_actionable_recommendation or $no_actionable_sentiment) and ($actionable | not)) | not)) |

(if $reviewer == "human" then
true
Expand Down Expand Up @@ -1506,6 +1521,10 @@ scan-merged options:
--dry-run Scan and report findings without creating issues or marking
PRs as scanned. Use to identify false-positive issues before
committing to issue creation.
--include-positive Disable the positive-review filter. By default, reviews
with zero inline comments and purely positive/approving body
text are skipped as false positives. Pass this flag to
include them — useful for debugging the filter itself.

Examples:
quality-feedback-helper.sh status
Expand All @@ -1517,6 +1536,7 @@ Examples:
quality-feedback-helper.sh scan-merged --repo owner/repo --create-issues
quality-feedback-helper.sh scan-merged --repo owner/repo --backfill --create-issues --tag-actioned
quality-feedback-helper.sh scan-merged --repo owner/repo --dry-run
quality-feedback-helper.sh scan-merged --repo owner/repo --dry-run --include-positive

Requirements:
- GitHub CLI (gh) installed and authenticated
Expand Down
161 changes: 158 additions & 3 deletions .agents/scripts/tests/test-quality-feedback-main-verification.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,17 +432,20 @@ test_plain_fence_skips_diff_marker_lines() {
# Helper: run the approval-detection jq filter against a review body.
# Returns "skip" if the review would be skipped, "keep" if it would be kept.
# Mirrors the $approval_only + $actionable logic in _scan_single_pr.
# $4 - include_positive: "true" to bypass all positive filters (default "false")
_test_approval_filter() {
local body="$1"
local state="${2:-COMMENTED}"
local reviewer="${3:-coderabbit}"
local include_positive="${4:-false}"

# Replicate the jq filter from _scan_single_pr review_findings block
local result
result=$(jq -rn \
--arg body "$body" \
--arg state "$state" \
--arg reviewer "$reviewer" '
--arg reviewer "$reviewer" \
--argjson include_positive "$([[ "$include_positive" == "true" ]] && echo 'true' || echo 'false')" '
($body | test(
"^[\\s\\n]*(lgtm|looks good( to me)?|ship it|shipit|:shipit:|:\\+1:|👍|" +
"approved?|great (work|job|change|pr|patch)|nice (work|job|change|pr|patch)|" +
Expand Down Expand Up @@ -474,8 +477,43 @@ _test_approval_filter() {
"\\bworkaround\\b|\\bhack\\b|" +
"```\\s*(suggestion|diff)"; "i")) as $actionable |

# skip = approval-only/no-recommendation AND NOT actionable
if (($approval_only or $no_actionable_recommendation or $no_actionable_sentiment) and ($actionable | not)) then "skip"
# skip = approval-only/no-recommendation AND NOT actionable (unless include_positive)
if ($include_positive) then "keep"
elif (($approval_only or $no_actionable_recommendation or $no_actionable_sentiment) and ($actionable | not)) then "skip"
else "keep"
end
')
echo "$result"
return 0
}

# Helper: run the summary-only bot filter.
# Returns "skip" if the review would be skipped as a summary-only bot review,
# "keep" otherwise. Mirrors the $summary_only logic in _scan_single_pr.
# $1 - reviewer login (e.g. "gemini-code-assist[bot]")
# $2 - review state (e.g. "COMMENTED", "APPROVED")
# $3 - inline comment count for this reviewer
# $4 - include_positive: "true" to bypass (default "false")
_test_summary_only_filter() {
local login="$1"
local state="${2:-COMMENTED}"
local inline_count="${3:-0}"
local include_positive="${4:-false}"

local result
result=$(jq -rn \
--arg login "$login" \
--arg state "$state" \
--argjson inline_count "$inline_count" \
--argjson include_positive "$([[ "$include_positive" == "true" ]] && echo 'true' || echo 'false')" '
(if ($login | test("coderabbit"; "i")) then "coderabbit"
elif ($login | test("gemini|google"; "i")) then "gemini"
elif ($login | test("codacy"; "i")) then "codacy"
else "human"
end) as $reviewer |
($inline_count == 0 and $state == "COMMENTED" and $reviewer != "human") as $summary_only |
if ($include_positive) then "keep"
elif $summary_only then "skip"
else "keep"
end
')
Expand Down Expand Up @@ -600,6 +638,113 @@ return nil, fmt.Errorf("invalid input: %w", err)
return 0
}

# --- GH#4733: --include-positive flag tests ---

test_skips_gemini_summary_only_bot_review() {
# Gemini posts a COMMENTED review with 0 inline comments and a positive
# summary body. This is the exact false-positive pattern from issue #3723 /
# PR #1250: "The changes improve the auditability and reliability of task
# tracking." — no inline comments, no actionable language.
local result
result=$(_test_summary_only_filter "gemini-code-assist[bot]" "COMMENTED" 0)
if [[ "$result" == "skip" ]]; then
print_result "skip Gemini summary-only COMMENTED review with 0 inline comments" 0
else
print_result "skip Gemini summary-only COMMENTED review with 0 inline comments" 1 "expected skip, got ${result}"
fi
return 0
}

test_keeps_gemini_review_with_inline_comments() {
# Gemini COMMENTED review that also has inline comments — must be kept
local result
result=$(_test_summary_only_filter "gemini-code-assist[bot]" "COMMENTED" 3)
if [[ "$result" == "keep" ]]; then
print_result "keep Gemini COMMENTED review that has inline comments" 0
else
print_result "keep Gemini COMMENTED review that has inline comments" 1 "expected keep, got ${result}"
fi
return 0
}

test_keeps_human_summary_only_review() {
# Human COMMENTED review with 0 inline comments — must NOT be filtered
# (the summary-only filter only applies to bots)
local result
result=$(_test_summary_only_filter "alice" "COMMENTED" 0)
if [[ "$result" == "keep" ]]; then
print_result "keep human COMMENTED review even with 0 inline comments" 0
else
print_result "keep human COMMENTED review even with 0 inline comments" 1 "expected keep, got ${result}"
fi
return 0
}

test_include_positive_bypasses_lgtm_filter() {
# With --include-positive, a pure LGTM review must be kept (not skipped)
local result
result=$(_test_approval_filter "LGTM" "COMMENTED" "coderabbit" "true")
if [[ "$result" == "keep" ]]; then
print_result "--include-positive bypasses LGTM filter" 0
else
print_result "--include-positive bypasses LGTM filter" 1 "expected keep, got ${result}"
fi
return 0
}

test_include_positive_bypasses_no_further_comments_filter() {
# With --include-positive, "no further comments" review must be kept
local result
result=$(_test_approval_filter "I've reviewed the changes and have no further comments. Good work." "COMMENTED" "coderabbit" "true")
if [[ "$result" == "keep" ]]; then
print_result "--include-positive bypasses 'no further comments' filter" 0
else
print_result "--include-positive bypasses 'no further comments' filter" 1 "expected keep, got ${result}"
fi
return 0
}

test_include_positive_bypasses_summary_only_filter() {
# With --include-positive, a Gemini summary-only COMMENTED review must be kept
local result
result=$(_test_summary_only_filter "gemini-code-assist[bot]" "COMMENTED" 0 "true")
if [[ "$result" == "keep" ]]; then
print_result "--include-positive bypasses summary-only bot filter" 0
else
print_result "--include-positive bypasses summary-only bot filter" 1 "expected keep, got ${result}"
fi
return 0
}

test_skips_positive_confirmation_body() {
# The specific Gemini body from issue #3723: "The changes improve the
# auditability and reliability of task tracking." — positive confirmation
# with no actionable language. The $summary_only filter catches this when
# inline_count=0 + state=COMMENTED. Verify the approval filter also handles
# it when the body contains no actionable keywords.
local body="The changes improve the auditability and reliability of task tracking."
local actionable
actionable=$(jq -rn --arg body "$body" '
($body | test(
"\\bshould\\b|\\bconsider\\b|\\binstead\\b|\\bsuggest|\\brecommend(ed|ing)?\\b|" +
"\\bwarning\\b|\\bcaution\\b|\\bavoid\\b|\\b(don ?'"'"'?t|do not)\\b|" +
"\\bvulnerab|\\binsecure|\\binjection\\b|\\bxss\\b|\\bcsrf\\b|" +
"\\bbug\\b|\\berror\\b|\\bproblem\\b|\\bfail\\b|\\bincorrect\\b|\\bwrong\\b|\\bmissing\\b|\\bbroken\\b|" +
"\\bnit:|\\btodo:|\\bfixme|\\bhardcoded|\\bdeprecated|" +
"\\brace.condition|\\bdeadlock|\\bleak|\\boverflow|" +
"\\bworkaround\\b|\\bhack\\b|" +
"```\\s*(suggestion|diff)"; "i"))
')
# The body must NOT be actionable — confirming it would be filtered by
# the summary-only rule (0 inline comments + COMMENTED state for bots)
if [[ "$actionable" == "false" ]]; then
print_result "Gemini positive confirmation body has no actionable language" 0
else
print_result "Gemini positive confirmation body has no actionable language" 1 "expected false, got ${actionable}"
fi
return 0
}

main() {
source "$HELPER"

Expand Down Expand Up @@ -627,6 +772,16 @@ main() {
test_keeps_review_with_bug_report
test_keeps_review_with_suggestion_fence

echo ""
echo "Running --include-positive flag tests (GH#4733)"
test_skips_gemini_summary_only_bot_review
test_keeps_gemini_review_with_inline_comments
test_keeps_human_summary_only_review
test_include_positive_bypasses_lgtm_filter
test_include_positive_bypasses_no_further_comments_filter
test_include_positive_bypasses_summary_only_filter
test_skips_positive_confirmation_body

echo "Results: ${TESTS_PASSED}/${TESTS_RUN} passed, ${TESTS_FAILED} failed"
if [[ "$TESTS_FAILED" -gt 0 ]]; then
return 1
Expand Down