Skip to content

fix(ci): run commitperclip-review against the merge queue's landing commit - #1077

Open
allyblockcast[bot] wants to merge 1 commit into
masterfrom
platformsre/blo-22300-mergegroup-security-review
Open

fix(ci): run commitperclip-review against the merge queue's landing commit#1077
allyblockcast[bot] wants to merge 1 commit into
masterfrom
platformsre/blo-22300-mergegroup-security-review

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work, and Blockcast/paperclip is one of the repos it manages itself, so its own CI is in scope for the platform-reliability lane.
  • Blockcast/paperclip's branch protection landed a GitHub merge queue (rebase-merge method) so PRs can wait their turn instead of racing strict-mode's "behind master" check (BLO-21316).
  • A merge queue's required checks run against a synthetic candidate commit (merge_group.head_sha), not the PR's original head — pr.yml was already ported to merge_group for that reason (BLO-21353).
  • commitperclip-review.yml (the review and security-review checks) was not ported in that pass. It only triggers on pull_request_target, so it only ever scans the PR's original head commit.
  • Because the queue rebases onto master's tip at merge time, the commit that actually lands is a different SHA that security-review never scanned — confirmed empirically: PR ci: authorize exact Paperclip deploy plans (BLO-19955) #995's merge commit f819d716e carries no review/security-review check-run at all, while its PR head 0657fbaf5 does.
  • This PR adds merge_group as a second trigger and branches every github.event.pull_request.* reference the workflow used, mirroring the PR_BASE_SHA/PR_HEAD_SHA fallback pattern already established in pr.yml.
  • The benefit is that security-review's check-run (and the draft security advisory it can file) is now scoped to the commit that is actually being evaluated for merge, closing the governance gap the CTO flagged on BLO-21353.

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: added merge_group: types: [checks_requested] alongside the existing pull_request_target trigger.
  • Added workflow-level PR_BASE_SHA/PR_HEAD_SHA env vars (github.event.pull_request.{base,head}.sha || github.event.merge_group.{base,head}_sha), used by the "Dependency Review" step's base-ref/head-ref inputs — same pattern as pr.yml.
  • Added a "Resolve PR context for merge-queue runs" step that, only under 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, since merge_group events carry no github.event.pull_request context at all.
  • Every remaining 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: added resolveTargetSha(pr, envHeadSha) and changed main() to post the security-review check-run against PR_HEAD_SHA (falls back to pr.head.sha when unset, i.e. unchanged behavior on pull_request_target) instead of always pr.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.
  • Added unit tests for resolveTargetSha in check-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.mjs now has 36 including the 2 new resolveTargetSha tests).
  • node ./scripts/check-github-runner-labels.mjs — 20 workflows validated, no new runner-label violations from this change (job still runs on arc-light, unchanged).
  • Manually confirmed the gap this PR fixes: gh api repos/Blockcast/paperclip/commits/<PR-995-head>/check-runs shows review (success) and security-review (neutral); gh api repos/Blockcast/paperclip/commits/<PR-995-merge-commit>/check-runs shows neither — the merged tree was never scanned.
  • Manually confirmed merge_group.head_ref format 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-...", and merge_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.
  • Once this PR itself lands via the merge queue, its own merge_group run 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

  • Fork-PR secret safety is unchanged. The "Checkout base branch" step still unconditionally checks out master regardless of trigger — this workflow never executes the PR diff or the merge-queue candidate's code either way (actions/dependency-review-action only diffs manifests via the GitHub API; check-pr-security.mjs/run-quality-gates.mjs only read PR/file data via the GitHub API). The only thing that varies with merge_group is which commit the checks report against (PR_BASE_SHA/PR_HEAD_SHA), not what code executes with secrets in scope.
  • New failure mode: PR-number parsing. If GitHub ever changes the 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.
  • Neither review nor security-review is a required check (confirmed via GraphQL statusCheckRollup.isRequired=False on 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.
  • Low risk otherwise: no change to pull_request_target runs' 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 the PlatformSREEngineer agent inside Paperclip's claude_k8s adapter. No extended-thinking/reasoning-effort override for this task; standard tool-use loop (Bash, Read, Edit) for investigation, implementation, and verification.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • If this change affects the UI, I have included before/after screenshots
  • I have updated relevant documentation to reflect my changes
  • I have considered and documented any risks above
  • All Paperclip CI gates are green
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups
  • I will address all Greptile and reviewer comments before requesting merge

Related in-flight work in the same area (not a duplicate — different concern, no file overlap): #1042 load-sheds stale merge_group re-stages in pr.yml; this PR only touches commitperclip-review.yml and check-pr-security.mjs.

…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>
@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-21353
🔗 Paperclip issue: BLO-21316
🔗 Paperclip issue: BLO-22300

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants