Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ jobs:
- uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5
- uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff
with:
go-version: "1.25.12"
go-version: "1.25.13"
- name: gofmt
run: test -z "$(gofmt -l .)" || { echo 'gofmt needed:'; gofmt -l .; exit 1; }
- name: vet
Expand Down
46 changes: 29 additions & 17 deletions internal/engine/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ func (e *Engine) readyNumbers(ctx context.Context) ([]int, error) {
if err != nil {
return nil, err
}
gate := admissionGate{protection: protection}
gate := admissionGate{protection: protection, statusCtx: e.cfg.StatusCtx}
var nums []int
for _, p := range prs {
// Quick check: if the PR is not scheduled, it was likely a bounced
Expand Down Expand Up @@ -343,10 +343,10 @@ func (e *Engine) readyNumbers(ctx context.Context) ([]int, error) {
continue
}
// For an already-scheduled PR the 409 path skips Forgejo's 422
// requirement validation, so re-check the review policy ourselves:
// a PR blocked by approvals or a live changes-requested review would
// never merge and must not consume a staging/CI run.
if blocked, reason, err := gate.blocks(ctx, e.fc, e.cfg.Owner, e.cfg.Repo, p.Number); err != nil {
// requirement validation, so re-check branch protection ourselves:
// a PR blocked by a required status or review policy would never merge
// and must not consume a staging/CI run.
if blocked, reason, err := gate.blocks(ctx, e.fc, e.cfg.Owner, e.cfg.Repo, p.Number, p.Head.Sha); err != nil {
return nil, err
} else if blocked {
e.logger.Info("PR blocked from merge queue by review policy", "pr", p.Number, "reason", reason)
Expand All @@ -368,23 +368,35 @@ func (e *Engine) readyNumbers(ctx context.Context) ([]int, error) {
return nums, nil
}

// admissionGate replicates the branch-protection review policy that Forgejo's
// merge gate applies, so shunt can refuse to queue a PR that can never merge.
// The rule only blocks when it actually requires something; a repo with no
// protection rule (or one without approval/review requirements) admits freely.
// admissionGate replicates the branch-protection status and review policy
// that Forgejo's merge gate applies, so shunt can refuse to queue a PR that
// can never merge. shunt's own status is excluded because it is only written
// after the staged batch passes.
type admissionGate struct {
protection forge.BranchProtection
statusCtx string
}

// blocks reports whether the PR is barred from the queue by the base-branch
// review policy, and why. It mirrors HasEnoughApprovals,
// MergeBlockedByRejectedReview, and MergeBlockedByOfficialReviewRequests:
// approvals must be official, non-dismissed, and (when IgnoreStaleApprovals)
// non-stale; a live REQUEST_CHANGES blocks when BlockOnRejectedReviews; an
// outstanding official review request blocks when
// BlockOnOfficialReviewRequests.
func (g admissionGate) blocks(ctx context.Context, fc ForgeAPI, owner, repo string, num int) (bool, string, error) {
// blocks reports whether the PR is barred from the queue by base-branch
// protection. Required statuses other than shunt's own must be successful.
// Review semantics mirror HasEnoughApprovals, MergeBlockedByRejectedReview,
// and MergeBlockedByOfficialReviewRequests.
func (g admissionGate) blocks(ctx context.Context, fc ForgeAPI, owner, repo string, num int, sha string) (bool, string, error) {
p := g.protection
if p.EnableStatusCheck {
for _, context := range p.StatusCheckContexts {
if context == g.statusCtx {
continue
}
status, ok, err := fc.LatestCommitStatus(ctx, owner, repo, sha, context)
if err != nil {
return false, "", err
}
if !ok || status.Status != "success" {
return true, fmt.Sprintf("required status check %q is not successful", context), nil
}
}
}
if p.RequiredApprovals == 0 && !p.BlockOnRejectedReviews && !p.BlockOnOfficialReviewRequests {
return false, "", nil
}
Expand Down
26 changes: 26 additions & 0 deletions internal/engine/engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,32 @@ func TestAdmissionAllowsPRWithEnoughApprovals(t *testing.T) {
}
}

func TestAdmissionBlocksUnsatisfiedRequiredStatusBeforeStaging(t *testing.T) {
for _, tc := range []struct {
name string
status forge.CommitStatus
wantStaged bool
}{
{name: "missing", wantStaged: false},
{name: "failure", status: forge.CommitStatus{Context: "ci", Status: "failure"}, wantStaged: false},
{name: "success", status: forge.CommitStatus{Context: "ci", Status: "success"}, wantStaged: true},
} {
t.Run(tc.name, func(t *testing.T) {
m := newMock(-1, 1)
m.protection = forge.BranchProtection{EnableStatusCheck: true, StatusCheckContexts: []string{"merge-queue", "ci"}}
m.latestStatus[1] = tc.status
e := New(Config{Owner: "o", Repo: "r", Base: "main", StatusCtx: "merge-queue", StagingBranch: "mq/main/staging"}, m, m)

if err := e.Reconcile(context.Background()); err != nil {
t.Fatalf("reconcile: %v", err)
}
if got := fmt.Sprint(m.staged); (got == "[[1]]") != tc.wantStaged {
t.Fatalf("staged = %s, want staged = %v", got, tc.wantStaged)
}
})
}
}

// TestAdmissionGateHonestState is the honest-state sibling: the gate blocks
// only when the branch-protection rule actually requires something. A PR with
// a dismissed reject, a non-official reject, or no protection rule at all must
Expand Down
2 changes: 1 addition & 1 deletion internal/forge/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ func (c *Client) ListReviews(ctx context.Context, owner, repo string, index int)
// ProtectedBranch returns the branch-protection rule for branch, or
// (zero-value, nil) when the branch has no protection rule (not blocked).
func (c *Client) ProtectedBranch(ctx context.Context, owner, repo, branch string) (BranchProtection, error) {
data, err := c.doRaw(ctx, http.MethodGet, fmt.Sprintf("/repos/%s/branches/%s/protection", repoPath(owner, repo), url.PathEscape(branch)), nil)
data, err := c.doRaw(ctx, http.MethodGet, fmt.Sprintf("/repos/%s/branch_protections/%s", repoPath(owner, repo), url.PathEscape(branch)), nil)
if errors.Is(err, ErrNotFound) {
return BranchProtection{}, nil
}
Expand Down
6 changes: 3 additions & 3 deletions internal/forge/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ func TestProtectedBranchParsesRequirementsAnd404MeansNoRule(t *testing.T) {
var path string
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
path = r.URL.Path
if r.URL.Path == "/api/v1/repos/o/r/branches/main/protection" {
if r.URL.Path == "/api/v1/repos/o/r/branch_protections/main" {
w.Header().Set("Content-Type", "application/json")
fmt.Fprint(w, `{"required_approvals":2,"block_on_rejected_reviews":true,"block_on_official_review_requests":true,"ignore_stale_approvals":true}`)
return
Expand All @@ -132,8 +132,8 @@ func TestProtectedBranchParsesRequirementsAnd404MeansNoRule(t *testing.T) {
if p.RequiredApprovals != 2 || !p.BlockOnRejectedReviews || !p.BlockOnOfficialReviewRequests || !p.IgnoreStaleApprovals {
t.Fatalf("protection = %+v", p)
}
if !strings.Contains(path, "branches/main/protection") {
t.Fatalf("path = %s, want protection endpoint", path)
if !strings.Contains(path, "branch_protections/main") {
t.Fatalf("path = %s, want Forgejo branch-protection endpoint", path)
}

// A branch with no rule returns a zero-value rule, not an error.
Expand Down
Loading