Skip to content

feat: add --include-positive flag to scan-merged for debugging positive-review filters #4748

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

Merged
alex-solovyev merged 1 commit into from
Mar 14, 2026
Merged

feat: add --include-positive flag to scan-merged for debugging positive-review filters #4748

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
57 changes: 39 additions & 18 deletions .agents/scripts/quality-feedback-helper.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,8 @@ 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 Bypass positive-review filters for debugging. Use with
# --dry-run to audit which reviews are being suppressed.
#
# Returns: 0 on success, 1 on error
#######################################
Expand All @@ -578,6 +580,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 @@ -615,6 +618,10 @@ cmd_scan_merged() {
dry_run=true
shift
;;
--include-positive)
include_positive=true
shift
;;
*)
echo "Unknown option for scan-merged: ${flag}" >&2
return 1
Expand Down Expand Up @@ -759,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 @@ -854,13 +861,17 @@ cmd_scan_merged() {
# $1 - repo slug
# $2 - PR number
# $3 - minimum severity (critical|high|medium)
# $4 - include_positive (true|false) — when true, skip positive-review filters
# (summary-only, approval-only, no-actionable-sentiment). Useful for
# debugging false-positive suppression. 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 @@ -937,7 +948,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 @@ -954,7 +966,8 @@ _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.
(($inline_counts[$login] // 0) == 0 and .state == "COMMENTED" and $reviewer != "human") as $summary_only |
# When --include-positive is set, this filter is bypassed for debugging.
(($inline_counts[$login] // 0) == 0 and .state == "COMMENTED" and $reviewer != "human" and ($include_positive | not)) as $summary_only |
select($summary_only | not) |

(.body) as $body |
Expand All @@ -975,6 +988,7 @@ _scan_single_pr() {
# These are false positives — filing quality-debt issues for "LGTM" or
# "no further comments" wastes worker time (GH#4604, incident: issue #3704 / PR #1484).
# Applies to all reviewer types including humans.
# When --include-positive is set, these filters are bypassed for debugging.
($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 @@ -1022,23 +1036,24 @@ _scan_single_pr() {
"\\bworkaround\\b|\\bhack\\b|" +
"```\\s*(suggestion|diff)"; "i")) as $actionable |

# Skip purely approving reviews. Explicit "no suggestions" statements are
# always non-actionable and should be skipped even though they contain the
# token "suggest", which would otherwise trip the actionable heuristic.
# Skip purely approving reviews unless --include-positive is set.
# Explicit "no suggestions" statements are always non-actionable and should
# be skipped even though they contain the token "suggest", which would
# otherwise trip the actionable heuristic.
# Other approval/sentiment patterns are skipped only when no actionable
# critique appears in the body.
select(($no_actionable_suggestion or ((($approval_only or $no_actionable_recommendation or $no_actionable_sentiment or $summary_praise_only) and ($actionable | not))) ) | not) |

(if $reviewer == "human" then
true
elif .state == "APPROVED" then
$actionable
else
($actionable and ($body | test(
"\\*\\*File\\*\\*|```\\s*(suggestion|diff)|" +
"\\bline\\s+[0-9]+\\b|\\bL[0-9]+\\b"; "i")))
end) |
select(.) |
select($include_positive or (($no_actionable_suggestion or ((($approval_only or $no_actionable_recommendation or $no_actionable_sentiment or $summary_praise_only) and ($actionable | not))) ) | not)) |

select(
if $include_positive then true
elif $reviewer == "human" then true
elif .state == "APPROVED" then $actionable
else
($actionable and ($body | test(
"\\*\\*File\\*\\*|```\\s*(suggestion|diff)|" +
"\\bline\\s+[0-9]+\\b|\\bL[0-9]+\\b"; "i")))
end
) |

{
pr: ($pr | tonumber),
Expand Down Expand Up @@ -1523,6 +1538,11 @@ 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 Bypass positive-review filters (summary-only, approval-only,
no-actionable-sentiment). Use with --dry-run to audit which
reviews are being suppressed and verify the filters are correct.
Not recommended for --create-issues runs — will generate
quality-debt issues for purely positive reviews.

Examples:
quality-feedback-helper.sh status
Expand All @@ -1534,6 +1554,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
210 changes: 210 additions & 0 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 @@ -644,6 +644,208 @@
return 0
}

# Helper: run the approval-detection jq filter with include_positive=true.
# Returns "keep" for all reviews when include_positive bypasses filters.
_test_approval_filter_include_positive() {
local body="$1"

# With include_positive=true the filter always returns "keep"
local result
result=$(jq -rn \
--arg body "$body" \
--argjson include_positive 'true' '
if $include_positive then "keep"
else
($body | test("\\bshould\\b|\\bconsider\\b"; "i")) as $actionable |
if $actionable then "keep" else "skip" end
end
')
echo "$result"
return 0
}

test_include_positive_keeps_lgtm_review() {
# With --include-positive, a pure LGTM review must be kept (not filtered)
local result
result=$(_test_approval_filter_include_positive "LGTM")
if [[ "$result" == "keep" ]]; then
print_result "--include-positive keeps LGTM review" 0
else
print_result "--include-positive keeps LGTM review" 1 "expected keep, got ${result}"
fi
return 0
}

test_include_positive_keeps_gemini_positive_summary() {
# With --include-positive, a Gemini-style positive summary must be kept
local result
result=$(_test_approval_filter_include_positive "This pull request successfully addresses the issue by removing an external dependency and improves robustness.")
if [[ "$result" == "keep" ]]; then
print_result "--include-positive keeps Gemini positive summary" 0
else
print_result "--include-positive keeps Gemini positive summary" 1 "expected keep, got ${result}"
fi
return 0
}

test_include_positive_keeps_no_suggestions_review() {
# With --include-positive, a "no suggestions" review must be kept
local result
result=$(_test_approval_filter_include_positive "Review completed. No suggestions at this time.")
if [[ "$result" == "keep" ]]; then
print_result "--include-positive keeps 'no suggestions' review" 0
else
print_result "--include-positive keeps 'no suggestions' review" 1 "expected keep, got ${result}"
fi
return 0
}

# Integration test: _scan_single_pr with include_positive=true returns findings
# for a purely positive review that would otherwise be filtered.
test_scan_single_pr_include_positive_returns_positive_review() {
reset_mock_state

# Mock gh to return a purely positive review (no inline comments, COMMENTED state)
gh() {
local command="$1"
shift
case "$command" in
api)
local endpoint=""

Check warning on line 714 in .agents/scripts/tests/test-quality-feedback-main-verification.sh

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove the unused local variable 'endpoint'. 


See more on https://sonarcloud.io/project/issues?id=marcusquinn_aidevops&issues=AZzroZ3FpV0X0E10kfoD&open=AZzroZ3FpV0X0E10kfoD&pullRequest=4748
while [[ $# -gt 0 ]]; do
case "$1" in
repos/*/pulls/*/comments)
echo "[]"
return 0
;;
repos/*/pulls/*/reviews)
echo '[{"id":1,"user":{"login":"gemini-code-assist[bot]"},"state":"COMMENTED","body":"This pull request successfully addresses the issue and improves robustness. The changes are well-implemented and consistent with the codebase.","submitted_at":"2024-01-01T00:00:00Z","html_url":"https://github.com/example/repo/pull/1#pullrequestreview-1"}]'
return 0
;;
repos/*/git/trees/*)
echo '{"tree":[]}'
return 0
;;
repos/*)
echo "main"
return 0
;;
esac
shift
done
echo "[]"
return 0
;;
label | pr) return 0 ;;
esac
echo "[]"
return 0
}

local findings
findings=$(_scan_single_pr "owner/repo" "1" "medium" "true" 2>/dev/null)
local count
count=$(printf '%s' "$findings" | jq 'length' 2>/dev/null || echo "0")

if [[ "$count" -gt 0 ]]; then
print_result "--include-positive: _scan_single_pr returns positive review" 0
else
print_result "--include-positive: _scan_single_pr returns positive review" 1 "expected >0 findings, got ${count}"
fi

# Restore mock gh
gh() {
local command="$1"
shift
case "$command" in
api)
_mock_gh_api "$@"
return $?
;;
label) return 0 ;;
issue)
_mock_gh_issue "$@"
return $?
;;
esac
echo "unexpected gh call: ${command}" >&2
return 1
}
return 0
}

# Integration test: _scan_single_pr without include_positive filters the same review
test_scan_single_pr_default_filters_positive_review() {
reset_mock_state

# Same mock as above but include_positive=false (default)
gh() {
local command="$1"
shift
case "$command" in
api)
while [[ $# -gt 0 ]]; do
case "$1" in
repos/*/pulls/*/comments)
echo "[]"
return 0
;;
repos/*/pulls/*/reviews)
echo '[{"id":1,"user":{"login":"gemini-code-assist[bot]"},"state":"COMMENTED","body":"This pull request successfully addresses the issue and improves robustness. The changes are well-implemented and consistent with the codebase.","submitted_at":"2024-01-01T00:00:00Z","html_url":"https://github.com/example/repo/pull/1#pullrequestreview-1"}]'
return 0
;;
repos/*/git/trees/*)
echo '{"tree":[]}'
return 0
;;
repos/*)
echo "main"
return 0
;;
esac
shift
done
echo "[]"
return 0
;;
label | pr) return 0 ;;
esac
echo "[]"
return 0
}

local findings
findings=$(_scan_single_pr "owner/repo" "1" "medium" "false" 2>/dev/null)

Check warning on line 818 in .agents/scripts/tests/test-quality-feedback-main-verification.sh

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Define a constant instead of using the literal 'owner/repo' 11 times. 


See more on https://sonarcloud.io/project/issues?id=marcusquinn_aidevops&issues=AZzroZ3FpV0X0E10kfoC&open=AZzroZ3FpV0X0E10kfoC&pullRequest=4748
local count
count=$(printf '%s' "$findings" | jq 'length' 2>/dev/null || echo "0")

if [[ "$count" -eq 0 ]]; then
print_result "default (no --include-positive): _scan_single_pr filters positive review" 0
else
print_result "default (no --include-positive): _scan_single_pr filters positive review" 1 "expected 0 findings, got ${count}"
fi

# Restore mock gh
gh() {
local command="$1"
shift
case "$command" in
api)
_mock_gh_api "$@"
return $?
;;
label) return 0 ;;
issue)
_mock_gh_issue "$@"
return $?
;;
esac
echo "unexpected gh call: ${command}" >&2
return 1
}
return 0
}

main() {
source "$HELPER"

Expand Down Expand Up @@ -674,6 +876,14 @@
test_keeps_review_with_bug_report
test_keeps_review_with_suggestion_fence

echo ""
echo "Running --include-positive flag tests (GH#4733)"
test_include_positive_keeps_lgtm_review
test_include_positive_keeps_gemini_positive_summary
test_include_positive_keeps_no_suggestions_review
test_scan_single_pr_include_positive_returns_positive_review
test_scan_single_pr_default_filters_positive_review

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