fix(ci): run commitperclip-review against the merge queue's landing commit - #1077
Open
allyblockcast[bot] wants to merge 1 commit into
Open
fix(ci): run commitperclip-review against the merge queue's landing commit#1077allyblockcast[bot] wants to merge 1 commit into
allyblockcast[bot] wants to merge 1 commit into
Conversation
…ommit commitperclip-review.yml only triggered on pull_request_target, so its review/security-review checks never ran against the commit that actually lands on master via the rebase-merge queue -- only ever the PR's own head. #995 landed on master without security-review ever scanning the merged tree (flagged on BLO-21353). - Add merge_group as a trigger, mirroring pr.yml's PR_BASE_SHA/PR_HEAD_SHA fallback pattern for every github.event.pull_request.* reference (base/ head sha, PR number, author, branch). - Recover the PR number under merge_group from the queue entry's branch name (refs/heads/gh-readonly-queue/<base>/pr-<number>-<sha>), since merge_group events carry no github.event.pull_request context. - check-pr-security.mjs now posts the security-review check-run against the actual landing commit (PR_HEAD_SHA, i.e. merge_group.head_sha) under merge_group runs, instead of always the PR's original head. - The "checkout base branch, never PR code" security property is unchanged and unweakened: the workflow still always checks out master for its own script code regardless of trigger, and never executes either the PR diff or the merge-queue candidate -- only calls the GitHub API to read PR/file data. See BLO-22300. Refs: BLO-22300 Co-Authored-By: Paperclip <noreply@paperclip.ing>
Author
13 tasks
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.
Thinking Path
Linked Issues or Issue Description
Refs: BLO-22300 (split out of BLO-21353, deliberately, not under incident pressure)
What Changed
.github/workflows/commitperclip-review.yml: addedmerge_group: types: [checks_requested]alongside the existingpull_request_targettrigger.PR_BASE_SHA/PR_HEAD_SHAenv vars (github.event.pull_request.{base,head}.sha || github.event.merge_group.{base,head}_sha), used by the "Dependency Review" step'sbase-ref/head-refinputs — same pattern aspr.yml.merge_group, recovers the PR number by parsing it out of the queue entry's branch name (refs/heads/gh-readonly-queue/<base>/pr-<number>-<sha>) and looks up the author/branch via the GitHub API, sincemerge_groupevents carry nogithub.event.pull_requestcontext at all.github.event.pull_request.*reference (number,user.login,head.ref) now falls back to that resolved context:github.event.pull_request.number || steps.pr_context.outputs.number, etc..github/scripts/check-pr-security.mjs: addedresolveTargetSha(pr, envHeadSha)and changedmain()to post thesecurity-reviewcheck-run againstPR_HEAD_SHA(falls back topr.head.shawhen unset, i.e. unchanged behavior onpull_request_target) instead of alwayspr.head.sha. This is the actual fix for the ticket's core gap — the check-run now lands on the commit that is actually being merged, not just the PR's original head.resolveTargetShaincheck-pr-security.test.mjs.Verification
node --test .github/scripts/tests/*.mjs— all 10 files pass individually (38 assertions across the touched file's suite,check-pr-security.test.mjsnow has 36 including the 2 newresolveTargetShatests).node ./scripts/check-github-runner-labels.mjs— 20 workflows validated, no new runner-label violations from this change (job still runs onarc-light, unchanged).gh api repos/Blockcast/paperclip/commits/<PR-995-head>/check-runsshowsreview(success) andsecurity-review(neutral);gh api repos/Blockcast/paperclip/commits/<PR-995-merge-commit>/check-runsshows neither — the merged tree was never scanned.merge_group.head_refformat and PR-number embedding against a real queue run:gh api repos/Blockcast/paperclip/actions/runs --jq '.workflow_runs[] | select(.event=="merge_group")'→head_branch: "gh-readonly-queue/master/pr-1030-...", andmerge_group.head_sha(9e784610...) differs from that PR's actual head (e79c1b4d...), confirming these are genuinely different commits and the parsing/fallback logic targets the right one.merge_grouprun is the live verifying signal — will paste the CI run link and landing-commit SHA on BLO-22300 once merged, per the ticket's verifying-signal requirement.Risks
masterregardless of trigger — this workflow never executes the PR diff or the merge-queue candidate's code either way (actions/dependency-review-actiononly diffs manifests via the GitHub API;check-pr-security.mjs/run-quality-gates.mjsonly read PR/file data via the GitHub API). The only thing that varies withmerge_groupis which commit the checks report against (PR_BASE_SHA/PR_HEAD_SHA), not what code executes with secrets in scope.gh-readonly-queue/<base>/pr-<number>-<sha>branch-name format, the parsing step fails loudly (set -euo pipefail+ explicit::error::+exit 1) rather than silently mis-scoping a check-run to the wrong PR.reviewnorsecurity-reviewis a required check (confirmed via GraphQLstatusCheckRollup.isRequired=Falseon BLO-21353), so this change cannot stall or evict merge-queue entries even if something above is wrong — worst case is the same "doesn't run"/"reports against the wrong SHA" gap this PR is fixing, not a new throughput regression.pull_request_targetruns' checkout, secrets exposure, or the set of steps that execute.Model Used
Claude (Anthropic), model ID
claude-sonnet-5[1m]— Sonnet 5 with a 1M context window, running as thePlatformSREEngineeragent inside Paperclip'sclaude_k8sadapter. No extended-thinking/reasoning-effort override for this task; standard tool-use loop (Bash, Read, Edit) for investigation, implementation, and verification.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateRelated in-flight work in the same area (not a duplicate — different concern, no file overlap): #1042 load-sheds stale
merge_groupre-stages inpr.yml; this PR only touchescommitperclip-review.ymlandcheck-pr-security.mjs.