fix(ci): the last-push-approval check reads a run's actor, not its triggering_actor - #3479
cagataycali wants to merge 3 commits into
Conversation
…iggering_actor resolve_pusher read triggering_actor from the workflow runs on the head sha. That field names the account behind the latest attempt, so GitHub rewrites it when a maintainer approves a held run or re-runs one; actor is the account whose event created the run, which for a push/pull_request run is the pusher. Every run on a first-time contributor's fork starts at action_required, so approving them is a maintainer's ordinary first act -- and the check then named that maintainer as the pusher of a branch in a repository they cannot push to. Measured on the same nine pull_request runs of strands-labs#3448, head b3d2233: actor shipitfast, triggering_actor cagataycali. strands-labs#3467, whose runs were never held, reads shipitfast in both. The event filter does not help, because the approval re-attributes the existing pull_request run rather than creating a new one under a different event. Once that maintainer approved the pull request the check read pusher-only-approval, and check_merge_blockers.py reported the pull request as owed to a reviewer other than the pusher, while require_last_push_approval -- which reads the real pusher -- was satisfied and the pull request was mergeable. The event filter stays, and stays load-bearing: a pull_request_review run's actor is the reviewer. actor agrees with triggering_actor on every case this check already pinned (strands-labs#1894, strands-labs#1920, strands-labs#1722, strands-labs#1035, strands-labs#1921's own), including the two heads whose commit metadata does not answer, so nothing the old field got right is lost. The module docstring, AGENTS.md > PR Workflow, the workflow's permissions comment and the test fixtures carry the correction; the fixtures now build runs through one helper that sets both fields, so they pin the pusher rather than a field name. Closes strands-labs#3452
|
Duplicate of #3478, which is the same one-field fix and is already approved. Closing in favour of it. One difference worth keeping in mind for later: here the run fixtures went through a single helper that sets both fields, so the pre-existing pins stay field-agnostic (pre-fix split was 1 failed / 52 passed rather than partly-renamed fixtures). Not worth a push onto an approved branch. |
Pull request was closed
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
This PR fixes scripts/check_last_push_approval.py to read a workflow run's actor (the account whose event created the run, i.e. the pusher for push/pull_request runs) instead of triggering_actor (the account behind the latest attempt, which GitHub rewrites when a maintainer approves a held run or re-runs one). The old read misattributed the pusher to the approving maintainer on first-time-contributor fork PRs, producing false pusher-only-approval findings on PRs that GitHub's own require_last_push_approval rule was willing to merge. The change is a single field swap with the same null-guard, event filter, and newest-run-wins tie-break; the script is reporting-only (non-gating), so there is no one-way door. Evidence is measured in both directions (three misattributed PRs flip to the real pusher, six unchanged, and actor agrees with triggering_actor on all five previously pinned cases including the two whose commit metadata declines to answer).
What's good
- Regression test (
test_approving_a_held_run_does_not_rename_the_pusher) pins the exact divergence and fails pre-fix, per the AGENTS.md pin-every-reviewed-fix rule. - The new
workflow_runfixture helper sets both attribution fields, so the pre-existing 52 tests now pin the pusher rather than a field name. - Changelog fragment is correctly named
3479-<slug>.md, and the AGENTS.md tables plus the grader test (test_last_push_rule_names_the_update_branch_button.py) are updated in the same change so the doc and its pin cannot drift. - The one deliberately untouched site (the
triggering_actorcomment inlast-push-approval.yml:92) is disclosed with a sound reason: a branch pushed withoutworkflowscope cannot carry a workflow-file edit.
Closes #3452.
What
resolve_pusherreadtriggering_actorfrom the workflow runs on the head sha. That field names the account behind the latest attempt, so GitHub rewrites it when a maintainer approves a held run or re-runs one.actoris the account whose event created the run, which for apush/pull_requestrun is the pusher.Why
Every run on a first-time contributor's fork starts at
action_required, so approving them is a maintainer's ordinary first act -- and the check then named that maintainer as the pusher of a branch in a repository they cannot push to. Reading the samepull_requestruns by both fields:actortriggering_actorshipitfastcagataycalishipitfastshipitfastThe event filter does not help: the approval re-attributes the existing
pull_requestrun rather than creating a new one. Once that maintainer approved the pull request, this check readpusher-only-approvalandcheck_merge_blockers.pyreported the pull request as owed to "a reviewer other than the pusher" -- whilerequire_last_push_approval, which reads the real pusher, was satisfied and the pull request was mergeable.--all-openover this repository, same moment, before and after:cagataycalishipitfastcagataycalishipitfastcagataycalishipitfastNothing the old field got right is lost
The event filter stays, and stays load-bearing -- a
pull_request_reviewrun'sactoris the reviewer.actoragrees withtriggering_actoron every case this check already pinned (#1894, #1920, #1722, #1035 and #1921's own), including the two heads whosecommit.author.logindoes not answer at all.Tests
test_approving_a_held_run_does_not_rename_the_pusherbuilds a run whose two fields differ: 1 failed, 52 passed pre-fix (assert 'cagataycali' == 'shipitfast'), 53 passed after. The other 52 pass on either field, because run fixtures now go through oneworkflow_runhelper that sets both -- so they pin the pusher rather than a field name.Full
tests/50,726 passed / 308 skipped / 0 failed;ruff check,ruff format --checkandmypyclean over 1,944 files.The module docstring,
AGENTS.md > PR Workflowand the rendered report row carry the correction. One further site is left untouched: theactions: readcomment in.github/workflows/last-push-approval.yml:92still says "itstriggering_actoris correct" and needs the same one-line edit, which a branch pushed withoutworkflowscope cannot carry.