ci: fix merge-check base branch handling#7374
Conversation
The merge-check workflow incorrectly checked whether master could fast-forward to a PR's merged head, which always failed for PRs targeting develop and falsely applied the "needs rebase" label (e.g. dashpay#7339). Replace the master-centric logic with a direct fast-forward check of the PR head against `github.event.pull_request.base.ref`, so a PR is only flagged when it cannot fast-forward into its own base. Additional cleanup: - Drop the `pull_request_review` trigger so review submissions do not rerun the merge-base check and produce duplicate same-name check runs. - Restrict the `push` trigger to `master` so feature/develop branch pushes do not trigger spurious checks. - Add a concurrency group keyed by PR number / ref to cancel stale runs. - Add `issues: write` so the labeler step can actually apply the `needs rebase` label, and gate label/comment steps on `pull_request_target` to avoid running them on push failures. - Pass `base.ref` / PR number via `env:` and quote shell expansions to keep the embedded script injection-safe.
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
This pull request has conflicts, please rebase. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
The failing Because this workflow runs on This PR changes that PR path to validate Since the required check is produced by the broken base-branch workflow, this fix may need maintainer bypass or a temporary branch-protection exception for the |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/merge-check.yml (1)
46-51: ⚡ Quick winConsider replacing third-party action with built-in
ghCLI or pinning to SHA.
actions-ecosystem/action-add-labels@v1uses deprecated Node.js 16 and has open migration issues. The March 2025 tj-actions/changed-files compromise demonstrated how tag-based pins can be exploited to leak secrets.Two options:
- Replace with
ghCLI (recommended): The runner includesghnatively, eliminating third-party dependency risk.- Pin to SHA: If keeping the action, pin to the commit hash for the v1.1.0 tag.
♻️ Option 1: Replace with gh CLI (recommended)
- name: add labels if: failure() && github.event_name == 'pull_request_target' - uses: actions-ecosystem/action-add-labels@v1 - with: - labels: | - needs rebase + env: + GH_TOKEN: ${{ github.token }} + run: gh pr edit ${{ github.event.pull_request.number }} --add-label "needs rebase"♻️ Option 2: Pin action to SHA
- name: add labels if: failure() && github.event_name == 'pull_request_target' - uses: actions-ecosystem/action-add-labels@v1 + uses: actions-ecosystem/action-add-labels@bd52874380e3909a1ac219571df80a3f708ed3e0 # v1.1.0 with: labels: | needs rebaseNote: Verify the SHA by running:
git ls-remote https://github.com/actions-ecosystem/action-add-labels refs/tags/v1.1.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/merge-check.yml around lines 46 - 51, The "add labels" step uses the deprecated actions-ecosystem/action-add-labels@v1 action which runs on unsupported Node.js 16 and poses security risks from tag-based pinning. Replace this action with the native gh CLI command instead: use run with gh pr edit to add the labels directly using the built-in GitHub CLI that's already available in the runner environment. This eliminates the third-party dependency while achieving the same result of adding the "needs rebase" label to the pull request.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/merge-check.yml:
- Around line 46-51: The "add labels" step uses the deprecated
actions-ecosystem/action-add-labels@v1 action which runs on unsupported Node.js
16 and poses security risks from tag-based pinning. Replace this action with the
native gh CLI command instead: use run with gh pr edit to add the labels
directly using the built-in GitHub CLI that's already available in the runner
environment. This eliminates the third-party dependency while achieving the same
result of adding the "needs rebase" label to the pull request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 41de5d72-d159-433a-aa15-dbcee821feb8
📒 Files selected for processing (1)
.github/workflows/merge-check.yml
Use the built-in GitHub CLI (gh pr edit --add-label) to apply the "needs rebase" label on PR merge-check failures, removing the actions-ecosystem/action-add-labels@v1 third-party dependency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Review complete (commit 9ef5dc5) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The workflow correctly fixes the master-centric fast-forward check and tightens trigger/permission handling for pull_request_target. Two convergent observations stand out: the trigger does not include edited, so retargeting a PR leaves a stale required-check result against the old base, and the failure path still invokes a mutable third-party comment action from a privileged context.
🟡 3 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/workflows/merge-check.yml`:
- [SUGGESTION] .github/workflows/merge-check.yml:11: Retargeted PRs are not re-checked against the new base
The fast-forward check now depends on `github.event.pull_request.base.ref`, but `pull_request_target` defaults to the `opened`, `synchronize`, and `reopened` activities only. Changing a PR's base branch is an `edited` activity and does not change the head SHA, so a PR can pass this required check while targeting `develop`, then be retargeted to `master`, and the previous successful `check_merge` result remains attached to the unchanged head SHA without re-running `git merge --ff-only` from `master`. That defeats the invariant this PR is introducing — that each required-check result corresponds to the PR's current base branch. Add `edited` to the activity types (and ideally re-run on `ready_for_review` for draft promotion) so a base change forces re-evaluation.
- [SUGGESTION] .github/workflows/merge-check.yml:55-60: Replace or pin the third-party comment action in the privileged failure path
This job runs under `pull_request_target` with `pull-requests: write` and `issues: write`, and on failure it invokes `mshick/add-pr-comment@v3` via a mutable major-version tag. The earlier commit in this PR already removed the other third-party action (`actions-ecosystem/action-add-labels`) in favor of `gh`. Doing the same for the comment step (e.g. `gh pr comment "$PR_NUMBER" --body ...`) — or, failing that, pinning to a full commit SHA — closes the remaining supply-chain surface in this privileged context.
- [SUGGESTION] .github/workflows/merge-check.yml:38-41: Avoid checking out the untrusted PR head into the privileged workspace
Under `pull_request_target` the job runs with the repo's elevated token. The current step does `git checkout base_branch` and then `git merge --ff-only pr_head`, which on a successful fast-forward materializes the PR's untrusted tree into the workspace. No later step uses that tree today, but this makes the workflow brittle — any future step appended after the merge would silently execute against attacker-controlled files. Use an ancestor-based check that never mutates the worktree, e.g. `git merge-base --is-ancestor base_branch pr_head` (or `git merge-tree`-based dry-run), so the privileged checkout stays at the trusted base.
- Add edited (base retarget) and ready_for_review activity types so the required check reruns on base changes and draft promotion. - Replace third-party mshick/add-pr-comment@v3 with built-in gh CLI, preserving the marker-based update-or-create comment semantics and body text. Uses GH_TOKEN: github.token. - Avoid materialising the untrusted PR head in the privileged workspace: fetch base_branch and pr_head, then verify with git merge-base --is-ancestor base_branch pr_head instead of checking out base and running git merge --ff-only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/merge-check.yml (1)
51-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate failure handlers on an explicit rebase result, not generic step failure.
At Line 51 and Line 60,
failure()catches all upstream failures (for example transient fetch/auth/network issues from Line 39-40), which can incorrectly applyneeds rebaseand post a misleading comment.Suggested change
- - name: Check fast-forward + - name: Check fast-forward + id: ff_check env: BASE_REF: ${{ github.event.pull_request.base.ref }} PR_NUMBER: ${{ github.event.pull_request.number }} run: | set -euo pipefail if [[ "${{ github.event_name }}" == "pull_request_target" ]]; then git fetch --no-tags origin "${BASE_REF}:base_branch" git fetch --no-tags origin "pull/${PR_NUMBER}/head:pr_head" if ! git merge-base --is-ancestor base_branch pr_head; then + echo "needs_rebase=true" >> "$GITHUB_OUTPUT" echo "PR head does not contain the tip of '${BASE_REF}'; rebase required." exit 1 fi + echo "needs_rebase=false" >> "$GITHUB_OUTPUT" echo "PR head fast-forwards from '${BASE_REF}'." else echo "Push event on '${{ github.ref_name }}'; no fast-forward check required." fi - - name: add labels - if: failure() && github.event_name == 'pull_request_target' + - name: add labels + if: failure() && github.event_name == 'pull_request_target' && steps.ff_check.outputs.needs_rebase == 'true' ... - - name: comment - if: failure() && github.event_name == 'pull_request_target' + - name: comment + if: failure() && github.event_name == 'pull_request_target' && steps.ff_check.outputs.needs_rebase == 'true'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/merge-check.yml around lines 51 - 60, The failure() conditions at lines 51 and 60 are too broad and catch all upstream failures including transient network/auth issues, causing incorrect application of the "needs rebase" label and misleading comments. Identify the specific step that checks for rebase requirements (the step around lines 39-40), assign it an explicit step ID, and then replace the generic failure() condition in both the label-adding step and the comment step with a reference to that specific step's result (using failure() on that step's ID only) instead of the global failure() condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/workflows/merge-check.yml:
- Around line 51-60: The failure() conditions at lines 51 and 60 are too broad
and catch all upstream failures including transient network/auth issues, causing
incorrect application of the "needs rebase" label and misleading comments.
Identify the specific step that checks for rebase requirements (the step around
lines 39-40), assign it an explicit step ID, and then replace the generic
failure() condition in both the label-adding step and the comment step with a
reference to that specific step's result (using failure() on that step's ID
only) instead of the global failure() condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4f33e2d5-4257-47ff-aab9-f5ece8b88bdc
📒 Files selected for processing (1)
.github/workflows/merge-check.yml
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR successfully resolves three of four prior findings: it adds edited/ready_for_review triggers, replaces the third-party mshick/add-pr-comment action with gh api, and switches to a worktree-free git merge-base --is-ancestor check that no longer materializes attacker-controlled files in the privileged workspace. Two suggestions remain: the failure handlers still treat any git fetch error as needs rebase, and the unqualified ${BASE_REF}:base_branch refspec could in principle resolve to a same-named tag.
💬 1 nitpick(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Narrow the failure handlers so transient fetch/auth/network failures no longer mislabel PRs as needing rebase. The fast-forward step now sets a needs_rebase=true output only when git merge-base --is-ancestor proves the PR head does not contain the base tip, and the label/comment steps gate on that output instead of the generic failure() condition. Also fetch into explicit refs/tmp/... refs so the local ref names cannot collide with tags or branches in the workspace. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest commit successfully resolves the prior failure-gating concern: handlers now trigger only on an explicit needs_rebase=true output, so transient fetch errors no longer mislabel PRs. One convergent suggestion remains — the unqualified source side of the base-ref fetch refspec can in principle resolve to a same-named tag instead of the branch.
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The tip commit completes the hardening series by qualifying the base-ref fetch source as refs/heads/${BASE_REF}, addressing the prior tag-shadow suggestion. Both general agents confirm no remaining in-scope defects in the workflow. The only flagged item is a commit-history squash preference, which falls outside the verifier's scope (consistent with the prior-run precedent dropping the same class of finding).
Note: posted as a COMMENT review because GitHub does not allow approving my own PR.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
This is intentional behavior. The goal is that CI breaks if develop / pr doesn't ff-only merge to master. The problem is we haven't merged master back into develop. This is a proper fail imo. |
|
Got it — I had misread the invariant. If the intended behavior is that develop/PR must fast-forward to master and the current failure is correctly exposing develop not having master merged back in, this PR is the wrong fix. Closing. |
2 participants
Fix merge-check workflow base handling
Issue being fixed or feature implemented
Fixes the
Check Merge Fast-Forward Onlyworkflow so PRs are validatedagainst their actual base branch instead of always checking whether the result
can fast-forward
master.This was causing false
needs rebasefailures for PRs targetingdevelop:the workflow could successfully fast-forward the PR head onto
develop, thenfail because it tried to fast-forward
masterto thatdevelopresult.What was done
pull_request_reviewtrigger frommerge-check.yml; reviewsubmissions do not change whether a PR branch is fast-forwardable and were
causing duplicate same-name required checks.
local refs, then run
git merge --ff-only pr_headfrombase_branch.masterand made the push path a no-op, since the branchis already the protected target being checked.
issues: writepermission because the failure path applies a PR labelor comment through issue APIs.
pull_request_targetevents.How Has This Been Tested
.github/workflows/merge-check.ymlwith Python YAML loading.run: |shell blocks from the workflow and validatedthem with
bash -n.BASE_REF=developandPR_NUMBER=7339to verify it usesdevelop:base_branchandpull/7339/head:pr_head, then mergespr_headintobase_branch.upstream/develop...ci/fix-merge-check-base; result:ship.Breaking Changes
None.
Checklist