From de280db0ce65077500692c8aade3ef33681c015c Mon Sep 17 00:00:00 2001 From: Alexey <1556417+alex-solovyev@users.noreply.github.com> Date: Sat, 14 Mar 2026 10:14:15 +0100 Subject: [PATCH] feat: add --include-positive flag to scan-merged for debugging positive-review filters Closes #4733 Adds --include-positive to quality-feedback-helper.sh scan-merged to bypass the positive-review suppression filters (summary-only, approval-only, no-actionable-sentiment). Intended for use with --dry-run to audit which reviews are being suppressed and verify the filters are working correctly. Changes: - cmd_scan_merged: parse --include-positive flag, pass to _scan_single_pr - _scan_single_pr: accept include_positive arg; bypass summary_only filter and approval/sentiment select() when true; use select() pattern instead of pipe-through-boolean to avoid jq object-construction errors - Help text: document --include-positive with usage example - Tests: 5 new tests covering flag unit behaviour and _scan_single_pr integration (27/27 passing, 0 shellcheck violations) --- .agents/scripts/quality-feedback-helper.sh | 57 +++-- ...test-quality-feedback-main-verification.sh | 210 ++++++++++++++++++ 2 files changed, 249 insertions(+), 18 deletions(-) diff --git a/.agents/scripts/quality-feedback-helper.sh b/.agents/scripts/quality-feedback-helper.sh index bdacbb341c..a01b467833 100755 --- a/.agents/scripts/quality-feedback-helper.sh +++ b/.agents/scripts/quality-feedback-helper.sh @@ -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 ####################################### @@ -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 @@ -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 @@ -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 @@ -854,6 +861,9 @@ 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 ####################################### @@ -861,6 +871,7 @@ _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 @@ -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) | @@ -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 | @@ -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)|" + @@ -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), @@ -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 @@ -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 diff --git a/.agents/scripts/tests/test-quality-feedback-main-verification.sh b/.agents/scripts/tests/test-quality-feedback-main-verification.sh index 04f3a67bfe..9de28c43ca 100644 --- a/.agents/scripts/tests/test-quality-feedback-main-verification.sh +++ b/.agents/scripts/tests/test-quality-feedback-main-verification.sh @@ -644,6 +644,208 @@ return nil, fmt.Errorf("invalid input: %w", err) 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="" + 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) + 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" @@ -674,6 +876,14 @@ main() { 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