Add review checklists workflow - #899
Open
LittleHuba wants to merge 15 commits into
Open
Conversation
Add a "Review Checklists" GitHub Actions workflow that posts checklist findings on pull requests, tracks reviewer acknowledgements, invalidates stale approvals on new pushes, and reports a "review-checklists" commit status that can be required for merges. - New composite action `actions/review_checklists` (post/check/ dismiss_sync/dismiss_edit) implementing the checklist logic. - New workflow `.github/workflows/review_checklists.yml` triggering on pull_request_target, pull_request_review_comment, pull_request_review, and merge_group events. - Configuration file `.github/review_checklists.yml` with a placeholder example checklist to be customized for this repository's needs. - Bazel wiring: pip.parse hub `review_checklists_dependencies` in MODULE.bazel, BUILD targets for the action's requirements and Python scripts/tests, and `actions` added to the root copyright_checker srcs. Verified: `bazel build //actions/review_checklists/...` and `bazel test //actions/review_checklists/...` (5/5 tests pass), plus `//:copyright-check` and `//:format_test` show no new findings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ed apply stages The single-stage Review Checklists workflow ran its "Check acknowledgements" step unconditionally, including when triggered by pull_request_review and pull_request_review_comment events raised for pull requests from forked repositories. GitHub always forces a read-only GITHUB_TOKEN for those events on fork PRs, regardless of the workflow's declared `permissions:` block, so the step's PR-description update (pr.edit()) failed with "403 Resource not accessible by integration". Split the workflow into two stages, following GitHub's documented "pwn request" mitigation pattern (workflow_run): - review_checklists_trigger.yml: unprivileged, reacts to all the original trigger events (pull_request_target, pull_request_review_comment, pull_request_review, merge_group). Runs no checkout and executes no repository/PR-supplied code -- it only captures the raw event payload and uploads it as an artifact. - review_checklists_apply.yml: privileged, triggered via workflow_run once the trigger stage completes. workflow_run always runs with the base repository's permissions regardless of where the original event originated, so this stage can safely hold pull-requests/statuses write access. It downloads the event artifact (outside the workspace), parses it, and runs the same post/dismiss_sync/dismiss_edit/check logic the single-stage workflow used to run directly. actions/review_checklists/action.yml gains optional event-name/event-path inputs so check_acknowledgements.py and dismiss_and_invalidate.py see the original captured event instead of the ambient workflow_run event, with no changes needed to the Python scripts themselves. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reduce what the unprivileged trigger stage must hand off to the
privileged apply stage, and derive as much as possible natively in
stage 2 instead of trusting artifact-supplied data:
- event_name and head_sha are now read directly from the workflow_run
context (github.event.workflow_run.event / .head_sha), which are
reliably correct even for fork-originated PRs. No longer parsed from
the artifact.
- pr_number is resolved authoritatively via the GitHub API (matching
currently-open PRs by head SHA) instead of being trusted from the
artifact.
- dismiss_sync no longer needs before/after SHAs from the event
payload at all: it looks up the immediately preceding
"Review Checklists (Trigger)" run for the same head branch and uses
that run's head SHA as "before", comparing against the current head.
- dismiss_edit (OK-comment edit/delete detection) is dropped entirely.
It was functionally redundant: check_acknowledgements always runs
last and already re-scans the live comment state on every
invocation, so an edited/deleted OK comment is naturally no longer
counted on the very next check. The associated merge-queue notice
refresh moved into check_acknowledgements, which now runs it
unconditionally.
- The trigger stage's artifact shrinks from a full raw webhook payload
to a single whitelisted field ({"action": "..."}), the only piece of
information that cannot be derived natively in stage 2.
Net effect: no full (attacker-influenced) webhook payload is
transferred between stages anymore, and the one output that discerns
routing (the PR number) is now independently verified rather than
blindly trusted.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
gh api forwards its --jq value straight to jq as the filter program, but does not support passing extra jq CLI flags like --arg alongside it (they were being parsed as additional gh api positional arguments, causing "accepts 1 arg(s), received 4"). Use jq's built-in `env.VAR` accessor instead, which reads the already-exported HEAD_SHA environment variable directly inside the filter without needing --arg. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The trigger stage's artifact only ever carried one field: the original event's `action` (opened/reopened/synchronize/edited), used solely to gate the apply stage's `dismiss_sync` step to `action == 'synchronize'`. dismiss_sync already resolves its own 'before' SHA by looking up the previous trigger-workflow run for the same branch and diffing against the current head. When the head SHA hasn't actually moved since that previous run (e.g. on 'edited'/'reopened' without a new push), the diff is empty, so no checklists are affected and the step is naturally a no-op. This means dismiss_sync can safely run unconditionally on every pull_request_target event without needing to know which sub-action fired. With that last use of the artifact gone, drop the entire artifact transfer: the trigger workflow no longer captures/uploads any event data, and the apply workflow no longer downloads/parses it. The trigger job now exists solely to complete and fire the workflow_run trigger. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tion race The 'Resolve PR number' step previously matched open PRs by head.sha == workflow_run.head_sha. If the PR receives another push while the apply run is queued or executing, no open PR will still have that exact SHA as its head by the time this step runs, so pr_number resolves empty and the whole apply run silently no-ops for that (now-superseded) trigger. Match on the head branch reference instead (workflow_run.head_repository.owner.login + head_branch via 'pulls?head=owner:branch'), which stays valid across such races as long as the PR is still open — the exact commit no longer matters for identifying which PR this run belongs to. Downstream steps already re-fetch the PR object and use its live pr.head.sha for everything except the (SHA-independent) merge_group status-setting path, so no script changes were needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Disable the example checklist to not spam users when the infrastructure is merged. Actual production checklists to be merged later.
Relocates the composite action and its scripts from actions/review_checklists/ to tools/review-checklists/, updating all path references: MODULE.bazel's pip.parse requirements_lock, the Bazel BUILD files' visibility/label, requirements.txt(.in)'s embedded paths, and the apply workflow's three 'uses: ./...' steps. Adds tools/review-checklists/README.md documenting: - What the action/workflows do and the acknowledgement model (threaded 'OK' replies to file-level review comment findings). - Why the logic is split into an unprivileged trigger workflow and a privileged workflow_run-triggered apply workflow (pwn-request prevention), and that the apply workflow always reads .github/review_checklists.yml from the base branch, never the PR branch. - A step-by-step list of what a consuming repository must additionally configure: vendoring/referencing the action, creating the checklist config, adjusting the base-branch filter, requiring the 'review-checklists' commit status (not the workflow job) plus an approving-review rule in branch protection, the merge-queue notice's assumption that the queue's merge method is a real merge commit, the 'reply OK in-thread' acknowledgement UX, the diff-position-1 anchor constraint on checklist include patterns, runner/network requirements, and keeping (or updating) the trigger workflow's filename since it's looked up by name for run-history diffing. - The checklist YAML schema and the composite action's inputs. All 5 existing Bazel tests (including the requirements lock check) pass unchanged at the new location. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Switch from create_review(comments=[{path, position: 1, body}]) to the
single-comment create_review_comment(..., subject_type="file") API.
File-level review comments are anchored to a file as a whole rather
than a specific diff position, so checklist include patterns can now
match binary files (images, archives, etc.) without failing to post.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This makes glob builds more reliable on different platforms.
Previously, the merge_group handler in check_acknowledgements.py just
set the commit status to success unconditionally ("checklists assumed
OK"), trusting that the required status check had already validated
the evidence before the PR entered the queue. This left a window where
stale evidence (e.g. dismissed reviews, invalidated acks after the
initial check went green) would not be caught at merge-queue time.
Now the merge_group handler resolves the originating PR (parsed from
the merge-queue's synthetic gh-readonly-queue/<base>/pr-<n>-<sha> head
ref) and re-runs the same acknowledgement validation against its
current live state (changed files, checklist findings, OK replies,
approving reviewers), setting success/pending/failure on the merge
commit SHA accordingly. It performs no PR writes (no evidence/notice
updates), only a read-only re-validation.
Refactored the shared decision logic (approvers vs. acks -> state)
into _acknowledgement_status() so both the live-PR flow and the
merge_group validation path use identical logic.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the apply stage's own PR-number resolution (matching the workflow_run head branch, or a fragile commit-SHA search for pull_request_review*/fork events) with a value read directly from the trusted event payload in the trigger stage and forwarded as a build artifact. This fixes a real bug: for pull_request_review and pull_request_review_comment events, github.event.workflow_run's head_repository/head_branch always reflect the base repository, never the fork the review was actually submitted on, so the previous owner:branch lookup could never resolve a PR number for fork PRs on those events. The PR number is not sensitive/attacker-steerable data (worst case a wrong number just points at the wrong already-public, same-repo PR), so carrying it across the trust boundary this way does not reintroduce the pwn-request risk the two-stage split exists to avoid. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
GITHUB_EVENT_NAME is a GitHub Actions reserved variable name: setting it via a step's env: block is silently ignored for the subprocess, which always sees the runner's real ambient value instead (here, "workflow_run", since that's what actually triggers review_checklists_apply.yml). This meant check_acknowledgements.py's `if event_name == "merge_group"` branch could never be reached when invoked from the apply workflow — confirmed live: for merge_group runs the script instead executed the normal PR-head-sha flow, posting the merge-queue notice and setting the commit status on pr.head.sha instead of the merge queue's synthetic HEAD_SHA, and never actually re-running _validate_checklist_evidence(). Fix: use a non-reserved variable name (CHECKLISTS_EVENT_NAME) to pass the original triggering event name into the script, so a step's own env: can actually override it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…yle)
Python's fnmatch treats '*' as matching across '/' with no distinction
between recursive and single-segment wildcards, so patterns that look
like standard glob syntax behave unintuitively:
- "**/*.md" requires a literal '/' in the matched path, so it silently
fails to match root-level files (discovered while live-testing a
multi-checklist merge-queue scenario).
- There was no way to anchor a pattern to the repo root at all.
Switch include/exclude matching to the pathspec library's gitwildmatch
style, i.e. the same pattern semantics as .gitignore:
- Unanchored patterns ("*.md") match at any depth.
- A leading '/' anchors a pattern to the repo root ("/*.md").
- "**" explicitly matches zero or more path segments, and correctly
matches at the root too ("**/BUILD" now matches a root-level BUILD
file, unlike the previous fnmatch-based implementation).
Added pathspec as a new dependency (requirements.txt.in/BUILD), and
added tests covering root-anchored and unanchored matching. Documented
the new gitignore-style syntax in the README. All 5/5 existing tests
continue to pass unchanged.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| from __future__ import annotations | ||
|
|
||
| import os |
| import pytest | ||
| import yaml | ||
|
|
||
| import helpers |
| import pytest | ||
| import yaml | ||
|
|
||
| import helpers |
| candidate = r.Rlocation(f"_main/{config_relpath}") | ||
| if candidate and os.path.isfile(candidate): | ||
| return candidate | ||
| except (ImportError, Exception): |
| monkeypatch.setenv("GITHUB_REPOSITORY", "org/repo") | ||
| monkeypatch.setenv("PR_NUMBER", "42") | ||
| gh = MagicMock() | ||
| repo, pr = get_repo_and_pr(gh) |
| monkeypatch.setenv("GITHUB_REPOSITORY", "org/repo") | ||
| monkeypatch.setenv("PR_NUMBER", "42") | ||
| gh = MagicMock() | ||
| repo, pr = get_repo_and_pr(gh) |
LittleHuba
force-pushed
the
add-review-checklists-workflow
branch
from
August 11, 2026 12:39
e6fb9d8 to
6f2b4c9
Compare
Public visibility is actually not required for these tools. Recude it to necessary levels.
LittleHuba
marked this pull request as ready for review
August 11, 2026 13:27
LittleHuba
requested review from
bemerybmw,
castler,
crimson11,
hoe-jo and
limdor
as code owners
August 11, 2026 13:27
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
1 participant
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.
Adds a workflow to require acknowledgement of checklists by reviewers.
Currently no checklist is enforced.