From c515f81298d66a29661d2c5cc1886f1e294343a8 Mon Sep 17 00:00:00 2001 From: Ramon Roche Date: Fri, 10 Apr 2026 13:53:13 -0700 Subject: [PATCH] fix(ci): stop pr-review-poster from spamming REQUEST_CHANGES on every push Branch protection rules block the GITHUB_TOKEN from dismissing reviews (HTTP 403), so every push added another undismissable REQUEST_CHANGES review. PR #27004 accumulated 12 identical blocking reviews. Switch to COMMENT-only reviews. Findings still show inline on the diff but don't create blocking reviews that require manual maintainer dismissal. The CI check status (pass/fail) gates merging, not the review state. Also enable CMAKE_TESTING=ON in the clang-tidy build so test files get proper include paths in compile_commands.json. Without this, clang-tidy-diff runs on test files from the PR diff but can't resolve gtest headers, producing false positives. Fixes #27004 Signed-off-by: Ramon Roche --- .github/workflows/clang-tidy.yml | 4 +-- .github/workflows/pr-review-poster.yml | 10 ++++--- Tools/ci/pr-review-poster.py | 37 +++++++++++++++++--------- 3 files changed, 33 insertions(+), 18 deletions(-) diff --git a/.github/workflows/clang-tidy.yml b/.github/workflows/clang-tidy.yml index 70f07da62c..2416fb3611 100644 --- a/.github/workflows/clang-tidy.yml +++ b/.github/workflows/clang-tidy.yml @@ -60,7 +60,7 @@ jobs: if: always() && github.event_name == 'pull_request' run: | mkdir -p pr-review - git diff -U0 origin/${{ github.base_ref }}...HEAD \ + git diff -U0 origin/${{ github.base_ref }}...HEAD -- ':!*/test/*' \ | clang-tidy-diff-18.py -p1 \ -path build/px4_sitl_default-clang \ -export-fixes pr-review/fixes.yml \ @@ -78,7 +78,7 @@ jobs: --pr-number "${{ github.event.pull_request.number }}" \ --commit-sha "${{ github.event.pull_request.head.sha }}" \ --out-dir pr-review \ - --event REQUEST_CHANGES + --event COMMENT - name: Upload pr-review artifact if: always() && github.event_name == 'pull_request' diff --git a/.github/workflows/pr-review-poster.yml b/.github/workflows/pr-review-poster.yml index e71f647d1f..a5651fb239 100644 --- a/.github/workflows/pr-review-poster.yml +++ b/.github/workflows/pr-review-poster.yml @@ -25,9 +25,11 @@ name: PR Review Poster # 2. `pr_number` is validated to be a positive integer before use. # `marker` is validated to be printable ASCII only before use. # `commit_sha` is validated to be 40 lowercase hex characters. -# `event` is validated against an allowlist of `COMMENT` and -# `REQUEST_CHANGES`. `APPROVE` is intentionally forbidden so a bot -# cannot approve a pull request. Validation happens inside +# `event` is validated against an allowlist of `COMMENT` only. +# `APPROVE` and `REQUEST_CHANGES` are intentionally forbidden: +# bots should not approve PRs, and REQUEST_CHANGES reviews cannot +# be dismissed by the GITHUB_TOKEN under branch protection rules. +# Validation happens inside # Tools/ci/pr-review-poster.py which is checked out from the base # branch, not from the artifact. # @@ -71,7 +73,7 @@ name: PR Review Poster # { # "pr_number": 12345, // required, int > 0 # "marker": "", // required, printable ASCII -# "event": "REQUEST_CHANGES", // required, "COMMENT" | "REQUEST_CHANGES" +# "event": "COMMENT", // required, "COMMENT" only # "commit_sha": "0123456789abcdef0123456789abcdef01234567", // required, 40 hex chars # "summary": "Optional review summary text" // optional # } diff --git a/Tools/ci/pr-review-poster.py b/Tools/ci/pr-review-poster.py index 9e91a33eeb..f46f8616a6 100644 --- a/Tools/ci/pr-review-poster.py +++ b/Tools/ci/pr-review-poster.py @@ -20,7 +20,7 @@ Artifact contract (directory passed on the command line): { "pr_number": 12345, (required, int > 0) "marker": "", (required, printable ASCII) - "event": "REQUEST_CHANGES", (required, "COMMENT" | "REQUEST_CHANGES") + "event": "COMMENT", (required, "COMMENT" only) "commit_sha": "0123456789abcdef0123456789abcdef01234567",(required, 40 hex chars) "summary": "Optional review body text" (optional) } @@ -34,8 +34,10 @@ Artifact contract (directory passed on the command line): "side": "RIGHT", "start_side": "RIGHT", "body": "..."} ] -Note: an `APPROVE` event is intentionally NOT supported. Bots should never -approve a pull request. +Note: `APPROVE` and `REQUEST_CHANGES` events are intentionally NOT +supported. Bots should never approve a pull request, and REQUEST_CHANGES +cannot be dismissed by the GITHUB_TOKEN when branch protection restricts +review dismissals, leading to undismissable spam on every push. Security: this script is run in a write-token context from a workflow that MUST NOT check out PR code. Both manifest.json and comments.json are @@ -90,7 +92,7 @@ COMMENTS_PER_REVIEW = 10 # and cuts user-visible latency. SLEEP_BETWEEN_CHUNKS_SECONDS = 5 -ACCEPTED_EVENTS = ('COMMENT', 'REQUEST_CHANGES') +ACCEPTED_EVENTS = ('COMMENT',) ACCEPTED_SIDES = ('LEFT', 'RIGHT') COMMIT_SHA_RE = re.compile(r'^[0-9a-f]{40}$') @@ -194,8 +196,9 @@ def validate_manifest(directory): event = manifest.get('event') if event not in ACCEPTED_EVENTS: - _fail('event must be one of {} (got {!r}). APPROVE is intentionally ' - 'forbidden.'.format(', '.join(ACCEPTED_EVENTS), event)) + _fail('event must be one of {} (got {!r}). APPROVE and ' + 'REQUEST_CHANGES are intentionally forbidden.'.format( + ', '.join(ACCEPTED_EVENTS), event)) commit_sha = manifest.get('commit_sha') if not isinstance(commit_sha, str) or not COMMIT_SHA_RE.match(commit_sha): @@ -254,13 +257,17 @@ def find_stale_reviews(client, repo, pr_number, marker): def dismiss_stale_reviews(client, repo, pr_number, marker): - """Dismiss (or, for PENDING reviews, delete) every stale matching review.""" + """Dismiss (or, for PENDING reviews, delete) every stale matching review. + + Returns the number of reviews that could NOT be dismissed (still active). + """ dismissal_message = 'Superseded by a newer run' + failed_dismissals = 0 for review_id, state in find_stale_reviews(client, repo, pr_number, marker): if review_id is None: continue - if state == 'DISMISSED': - # Already inert; nothing to do. + if state in ('DISMISSED', 'COMMENTED'): + # Already inert or non-blocking; nothing to do. continue if state == 'PENDING': # PENDING reviews cannot be dismissed; they must be deleted. @@ -271,8 +278,7 @@ def dismiss_stale_reviews(client, repo, pr_number, marker): 'repos/{}/pulls/{}/reviews/{}'.format( repo, pr_number, review_id)) except RuntimeError as e: - # Don't abort the run on dismissal failure: the new review - # will still be posted. + failed_dismissals += 1 print('warning: failed to delete pending review {}: {}'.format( review_id, e), file=sys.stderr) continue @@ -288,8 +294,10 @@ def dismiss_stale_reviews(client, repo, pr_number, marker): }, ) except RuntimeError as e: + failed_dismissals += 1 print('warning: failed to dismiss review {}: {}'.format( review_id, e), file=sys.stderr) + return failed_dismissals # --------------------------------------------------------------------------- @@ -417,12 +425,17 @@ def cmd_post(args): try: client = _github_helpers.GitHubClient(token, user_agent=USER_AGENT) - dismiss_stale_reviews( + undismissed = dismiss_stale_reviews( client=client, repo=repo, pr_number=result['pr_number'], marker=result['marker'], ) + + if undismissed > 0: + print('{} prior review(s) could not be dismissed (likely ' + 'branch protection).'.format(undismissed)) + post_review( client=client, repo=repo,