From a24176e787289595d268404781c2de55ccd1c993 Mon Sep 17 00:00:00 2001 From: sec-check Date: Wed, 26 Aug 2026 11:50:40 -0400 Subject: [PATCH] [quality] test: pin self-authored auto-merge sweep merge-safety guards Cover the untested guard branches of the self-authored sweep path: - trySweepSelfAuthoredPR (71.4% -> 98.0%): gone/fetch-pr, closed, draft, not-app-authored at eval, missing-head-sha, not-mergeable, commitGreen error, status-pending, recheck gone/error, head-changed-since-eval (vanished and pushed), merge-failed, merge-not-applied, plus the green merge happy path. - refreshRateLimitCache (33.3% -> 100%): success writes back to the go-github cache via GET /rate_limit; API errors are swallowed. - isHiveAppReviewAuthor (66.7% -> 100%): nil client, nil review, blank appBotLogin, forged author, and trusted App author. - requiredStatusCheckContexts (76.2% -> 95.2%): blank branch, ErrBranchNotProtected as known-empty-set, API error fails closed, contexts+checks parsing with nil check entries. pkg/github coverage: 91.9% -> 92.5%. Refs #4807 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: sec-check --- src/pkg/github/automerge_sweep_guards_test.go | 264 ++++++++++++++++++ 1 file changed, 264 insertions(+) create mode 100644 src/pkg/github/automerge_sweep_guards_test.go diff --git a/src/pkg/github/automerge_sweep_guards_test.go b/src/pkg/github/automerge_sweep_guards_test.go new file mode 100644 index 000000000..d60ee7326 --- /dev/null +++ b/src/pkg/github/automerge_sweep_guards_test.go @@ -0,0 +1,264 @@ +package github + +// Tests for the uncovered guard branches of the self-authored auto-merge +// sweep path (trySweepSelfAuthoredPR), the rate-limit cache refresh, the +// queue-approval trust check (isHiveAppReviewAuthor), and required-checks +// resolution (requiredStatusCheckContexts). These are merge-safety branches: +// each one is a reason the sweep must NOT squash a PR, so each deserves a +// pinned regression test. + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + gh "github.com/google/go-github/v72/github" +) + +// selfSweepFixture drives a one-PR httptest GitHub API for direct +// trySweepSelfAuthoredPR calls. Zero values produce a green, mergeable, +// App-authored open PR that merges cleanly; each field overrides one branch. +type selfSweepFixture struct { + firstGetStatus int // non-zero: first PR fetch returns this HTTP status + state string // PR state; default "open" + draft bool + author string // default testHiveAppBotLogin + headSHA *string + mergeableState string // default "clean" + statusState string // combined-status state; default "success" + statusHTTPCode int // non-zero: status endpoint returns this HTTP status + recheckGetStatus int // non-zero: second PR fetch returns this HTTP status + recheckHeadSHA *string + mergeHTTPCode int // non-zero: merge endpoint returns this HTTP status + mergeApplied bool // "merged" field in merge response; default true via fixture setup +} + +func newSelfSweepGuardAPI(t *testing.T, fx selfSweepFixture) *httptest.Server { + t.Helper() + if fx.state == "" { + fx.state = "open" + } + if fx.author == "" { + fx.author = testHiveAppBotLogin + } + if fx.mergeableState == "" { + fx.mergeableState = "clean" + } + if fx.statusState == "" { + fx.statusState = "success" + } + sha := "sha7" + if fx.headSHA == nil { + fx.headSHA = &sha + } + if fx.recheckHeadSHA == nil { + fx.recheckHeadSHA = fx.headSHA + } + fetches := 0 + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/repos/acme/widget/pulls/7": + fetches++ + if fetches == 1 && fx.firstGetStatus != 0 { + w.WriteHeader(fx.firstGetStatus) + return + } + if fetches > 1 && fx.recheckGetStatus != 0 { + w.WriteHeader(fx.recheckGetStatus) + return + } + head := *fx.headSHA + if fetches > 1 { + head = *fx.recheckHeadSHA + } + json.NewEncoder(w).Encode(map[string]any{ + "number": 7, + "state": fx.state, + "draft": fx.draft, + "mergeable_state": fx.mergeableState, + "mergeable": fx.mergeableState == "clean", + "user": map[string]string{"login": fx.author}, + "head": map[string]string{"sha": head}, + }) + case r.Method == http.MethodGet && r.URL.Path == "/repos/acme/widget/commits/sha7/status": + if fx.statusHTTPCode != 0 { + w.WriteHeader(fx.statusHTTPCode) + return + } + json.NewEncoder(w).Encode(map[string]any{ + "state": fx.statusState, + "total_count": 1, + "statuses": []map[string]string{{"context": "ci/build", "state": fx.statusState}}, + }) + case r.Method == http.MethodGet && r.URL.Path == "/repos/acme/widget/commits/sha7/check-runs": + json.NewEncoder(w).Encode(map[string]any{"total_count": 0, "check_runs": []map[string]string{}}) + case r.Method == http.MethodPut && r.URL.Path == "/repos/acme/widget/pulls/7/merge": + if fx.mergeHTTPCode != 0 { + w.WriteHeader(fx.mergeHTTPCode) + return + } + json.NewEncoder(w).Encode(map[string]any{"merged": fx.mergeApplied, "sha": "merge7"}) + default: + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.String()) + } + })) +} + +func TestTrySweepSelfAuthoredPRGuardBranches(t *testing.T) { + tests := []struct { + name string + fx selfSweepFixture + wantReason string + wantErr bool + }{ + {name: "PR deleted between list and eval", fx: selfSweepFixture{firstGetStatus: http.StatusNotFound}, wantReason: "gone"}, + {name: "PR fetch API error propagates", fx: selfSweepFixture{firstGetStatus: http.StatusInternalServerError}, wantReason: "fetch-pr", wantErr: true}, + {name: "closed PR is never merged", fx: selfSweepFixture{state: "closed"}, wantReason: "closed"}, + {name: "draft PR is never merged", fx: selfSweepFixture{draft: true}, wantReason: "draft"}, + {name: "authorship changed between list and eval", fx: selfSweepFixture{author: "mallory"}, wantReason: "not-app-authored"}, + {name: "missing head SHA blocks merge", fx: selfSweepFixture{headSHA: gh.Ptr("")}, wantReason: "missing-head-sha"}, + {name: "not mergeable blocks merge", fx: selfSweepFixture{mergeableState: "dirty"}, wantReason: "not-mergeable"}, + {name: "commitGreen API error propagates", fx: selfSweepFixture{statusHTTPCode: http.StatusInternalServerError}, wantReason: "status-check", wantErr: true}, + {name: "pending CI blocks merge", fx: selfSweepFixture{statusState: "pending"}, wantReason: "status-pending"}, + {name: "PR deleted between eval and merge", fx: selfSweepFixture{recheckGetStatus: http.StatusNotFound}, wantReason: "gone"}, + {name: "recheck API error propagates", fx: selfSweepFixture{recheckGetStatus: http.StatusInternalServerError}, wantReason: "fetch-pr-recheck", wantErr: true}, + {name: "head SHA vanished at recheck blocks merge", fx: selfSweepFixture{recheckHeadSHA: gh.Ptr("")}, wantReason: "head-changed-since-eval"}, + {name: "push landed between eval and merge blocks merge", fx: selfSweepFixture{recheckHeadSHA: gh.Ptr("newsha")}, wantReason: "head-changed-since-eval"}, + {name: "merge API error propagates", fx: selfSweepFixture{mergeHTTPCode: http.StatusConflict}, wantReason: "merge-failed", wantErr: true}, + {name: "merge accepted but not applied", fx: selfSweepFixture{mergeApplied: false}, wantReason: "merge-not-applied"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + api := newSelfSweepGuardAPI(t, tt.fx) + defer api.Close() + c := newAutoMergeSweepClient(api.URL) + event, reason, err := c.trySweepSelfAuthoredPR(context.Background(), "acme/widget", "acme", "widget", 7) + if (err != nil) != tt.wantErr { + t.Fatalf("err = %v, wantErr = %v", err, tt.wantErr) + } + if reason != tt.wantReason { + t.Fatalf("reason = %q, want %q", reason, tt.wantReason) + } + if event.MergeSHA != "" { + t.Fatalf("event = %+v, want no merge recorded on skip branch", event) + } + }) + } +} + +func TestTrySweepSelfAuthoredPRMergesGreenPR(t *testing.T) { + api := newSelfSweepGuardAPI(t, selfSweepFixture{mergeApplied: true}) + defer api.Close() + c := newAutoMergeSweepClient(api.URL) + event, reason, err := c.trySweepSelfAuthoredPR(context.Background(), "acme/widget", "acme", "widget", 7) + if err != nil || reason != "" { + t.Fatalf("trySweepSelfAuthoredPR = (reason %q, err %v), want clean merge", reason, err) + } + if event.MergeSHA != "merge7" || event.HeadSHA != "sha7" || event.QueuedBy != "" { + t.Fatalf("event = %+v, want merge7 at sha7 with no queuer", event) + } +} + +func TestRefreshRateLimitCache(t *testing.T) { + t.Run("success refreshes the cached verdict", func(t *testing.T) { + hits := 0 + api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet || r.URL.Path != "/rate_limit" { + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + } + hits++ + json.NewEncoder(w).Encode(map[string]any{ + "resources": map[string]any{"core": map[string]int{"limit": 5000, "remaining": 4999, "reset": 0}}, + }) + })) + defer api.Close() + c := newAutoMergeSweepClient(api.URL) + c.refreshRateLimitCache(context.Background()) + if hits != 1 { + t.Fatalf("rate_limit endpoint hit %d times, want exactly 1", hits) + } + }) + + t.Run("API error is swallowed with a warning", func(t *testing.T) { + api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer api.Close() + c := newAutoMergeSweepClient(api.URL) + c.refreshRateLimitCache(context.Background()) // must not panic or retry + }) +} + +func TestIsHiveAppReviewAuthorGuards(t *testing.T) { + review := &gh.PullRequestReview{User: &gh.User{Login: gh.Ptr(testHiveAppBotLogin)}} + + var nilClient *Client + if nilClient.isHiveAppReviewAuthor(review) { + t.Fatal("nil client must never trust a review author") + } + c := &Client{appBotLogin: testHiveAppBotLogin} + if c.isHiveAppReviewAuthor(nil) { + t.Fatal("nil review must never be trusted") + } + if (&Client{appBotLogin: " "}).isHiveAppReviewAuthor(review) { + t.Fatal("blank appBotLogin must never trust any review author") + } + if !c.isHiveAppReviewAuthor(review) { + t.Fatal("App bot review author must be trusted") + } + forged := &gh.PullRequestReview{User: &gh.User{Login: gh.Ptr("mallory")}} + if c.isHiveAppReviewAuthor(forged) { + t.Fatal("non-App review author must never be trusted") + } +} + +func TestRequiredStatusCheckContextsBranches(t *testing.T) { + t.Run("blank branch cannot resolve a required set", func(t *testing.T) { + c := newAutoMergeSweepClient("http://127.0.0.1:0") + if set, known := c.requiredStatusCheckContexts(context.Background(), "acme", "widget", " "); known || set != nil { + t.Fatalf("blank branch = (%v,%v), want unresolved", set, known) + } + }) + + t.Run("unprotected branch is a known empty set", func(t *testing.T) { + api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + json.NewEncoder(w).Encode(map[string]string{"message": "Branch not protected"}) + })) + defer api.Close() + c := newAutoMergeSweepClient(api.URL) + set, known := c.requiredStatusCheckContexts(context.Background(), "acme", "widget", "main") + if !known || len(set) != 0 { + t.Fatalf("unprotected branch = (%v,%v), want known empty set", set, known) + } + }) + + t.Run("API error means required set is unknown", func(t *testing.T) { + api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + defer api.Close() + c := newAutoMergeSweepClient(api.URL) + if set, known := c.requiredStatusCheckContexts(context.Background(), "acme", "widget", "main"); known || set != nil { + t.Fatalf("API error = (%v,%v), want unresolved so caller fails closed", set, known) + } + }) + + t.Run("contexts and checks both populate the required set", func(t *testing.T) { + api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + json.NewEncoder(w).Encode(map[string]any{ + "strict": true, + "contexts": []string{"build-gate"}, + "checks": []map[string]any{{"context": "lint"}, nil}, + }) + })) + defer api.Close() + c := newAutoMergeSweepClient(api.URL) + set, known := c.requiredStatusCheckContexts(context.Background(), "acme", "widget", "main") + if !known || !set["build-gate"] || !set["lint"] || len(set) != 2 { + t.Fatalf("required set = (%v,%v), want {build-gate,lint} known", set, known) + } + }) +}