fix(ci): skip review policy for check events on non-head commits - #1027
Open
rl-block wants to merge 1 commit into
Open
fix(ci): skip review policy for check events on non-head commits#1027rl-block wants to merge 1 commit into
rl-block wants to merge 1 commit into
Conversation
External check apps post one check run per commit of a pull request (the DCO check does), and Review Policy subscribes to every check_run and check_suite completion, so one push to a 56-commit PR started 56 evaluations of which 55 found no pull request and exited. Drop events whose commit is not an open pull request head before a runner is allocated, in the concurrency classification and the job condition alike. GitHub fills pull_requests only for same-repository heads and leaves head_branch null for forks, so fork events keep today's behaviour and are still evaluated.
🔐 Codex Security Review
Review SummaryOverall Risk: NONE FindingsNo security, correctness, or reliability findings identified in the changed hunks. NotesThe targeted workflow invariant test passes. The full test script could not run in the read-only review environment because no writable temporary directory was available. Generated by Codex Security Review | |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reviewable diff: +12/-0 across 1 file (excludes generated, test, and story files).
Summary
Review Policycurrently starts one evaluation for everycheck_run/check_suitecompletion in the repository. External check apps post one check run per commit of a pull request (the DCO check does), so a single push or body edit on a 56-commit PR started 56 Review Policy runs, 55 of which allocated a runner only to logNo pull request was associated with this eventand exit. The workflow's run counter is at ~100,900 against ~6,700 forPR Gate, almost all from this fan-out. This change drops check events for commits that are not an open pull request head before a runner is allocated. Fork PRs keep today's behaviour.How it works
GitHub fills
check_run.pull_requests(andcheck_suite.pull_requests) only when the check's commit is the head of an open pull request in the same repository, and leavescheck_suite.head_branchnull when the commit came from a fork (Checks API, webhook payloads). The workflow already classifies some events as ignorable in three places: the concurrencygroup(so ignored events never cancel a real evaluation),cancel-in-progress, and theevaluatejob'sif:. The same new condition is added to all three:and its
check_suitetwin. A matching event still creates a workflow run entry (GitHub creates one for every subscribed event), but the job is skipped in the scheduler with no runner time. The head-commit event still haspull_requestspopulated, so the policy re-evaluates exactly once when an external check lands, as before. Fork events havehead_branch == null, fail the first clause, and go through the existing API-based resolution.Diagrams
flowchart LR A["Push / PR edit"] --> B["DCO app posts one check run per commit"] B --> C["check_run: completed x N"] C --> D{"head_branch set and pull_requests empty?"} D -- "yes: not a PR head (N-1 events)" --> E["concurrency group ignored-run_id; evaluate job skipped, no runner"] D -- "no: PR head, or fork" --> F["Resolve pull request via API"] F --> G["Evaluate Review Policy"]Areas of the code involved
.github/workflows/review-policy.ymlgroup,cancel-in-progress, and theevaluatejobif:; a comment aboveconcurrency:explains the fan-out and the fork caveathead_branch != null) is what keeps this from silently skipping external contributors' PRs.github/scripts/evaluate_review_policy_test.pytest_workflow_skips_check_events_for_commits_that_are_not_a_pr_headasserts the condition, including the fork guard, is present in all three expressionsKey technical decisions & trade-offs
if:and concurrency, not in the resolve script. The script already returns "no pull request" for these events, but only after a runner has been allocated; the expression-level filter is the only place GitHub lets a workflow decline work for free. Alternative: leave as is and accept ~10 s of runner time per commit per push.head_branchis null the event is evaluated as today rather than skipped, at the cost of keeping the fan-out for fork PRs. Alternative: skip wheneverpull_requestsis empty, which would stop the policy from re-evaluating when the DCO check completes on a fork PR.check_runtrigger. The policy requiresDCO Checkfromblock-dco-check(.github/review-policy.json), so the event is load-bearing for the head commit; dropping the trigger was not an option.Testing & validation
python3 .github/scripts/evaluate_review_policy_test.py: 73 tests pass, including the new one (the loader parses the workflow with Ruby's YAML, so the expressions are checked as written).python-ruff).2026-09-08T01:33:32Z: 56DCO Checkcheck runs (one per PR commit) followed by 56 Review Policy runs, 55 resolving to no pull request. After merge, the next push to a multi-commit PR should show those 55 as skipped runs with zero duration.