From 36b5c682a9732fd92998184005c983d75c0c585a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Sun, 31 May 2026 11:59:15 +0100 Subject: [PATCH] fix(github): detect fork PRs by branch (#1182) * fix(github): detect fork PRs by branch * fix(github): align fork branch detection paths * fix(github): skip duplicate branch PR matches * test(github): remove stale branch owner fixtures * test(github): remove stale PAT branch owner fixture * fix(github): surface branch decode errors --- apps/backend/internal/github/graphql.go | 55 ++++--- apps/backend/internal/github/graphql_test.go | 149 ++++++++++++++++-- apps/backend/internal/github/pat_client.go | 17 +- .../internal/github/pat_client_test.go | 57 +++++++ apps/backend/internal/github/poller_test.go | 24 ++- 5 files changed, 247 insertions(+), 55 deletions(-) diff --git a/apps/backend/internal/github/graphql.go b/apps/backend/internal/github/graphql.go index a4fed0dc1e..b012827234 100644 --- a/apps/backend/internal/github/graphql.go +++ b/apps/backend/internal/github/graphql.go @@ -28,6 +28,10 @@ const graphQLPRBatchAlias = "pr" // fields = ~500 nodes, leaving headroom. const graphQLBatchChunkSize = 50 +// graphQLBranchProbeLimit fetches two matches so branch lookup can detect +// ambiguous fork heads instead of linking the first arbitrary PR. +const graphQLBranchProbeLimit = 2 + // reviewNode is one PR review entry from the batched GraphQL query. type reviewNode struct { State string `json:"state"` @@ -111,6 +115,11 @@ type batchedPRResult struct { } `json:"commits"` } +type batchedBranchPRNode struct { + Number int `json:"number"` + batchedPRResult +} + // graphQLError mirrors a single entry in a GraphQL response's "errors" array. type graphQLError struct { Message string `json:"message"` @@ -194,14 +203,14 @@ func prFieldsBlock() string { `commits(last: 1) { nodes { commit { statusCheckRollup { state } } } }` } -// buildBatchedBranchQuery emits one aliased associatedPullRequests block per -// (owner, repo, branch) ref. Used to batch the "find PR by branch" path. +// buildBatchedBranchQuery emits one aliased pullRequests(headRefName:) block +// per (owner, repo, branch). Used to batch the "find PR by branch" path. func buildBatchedBranchQuery(refs []graphQLBranchRef) (string, map[string]any) { var b strings.Builder b.WriteString("query Branches { ") for i, r := range refs { - fmt.Fprintf(&b, `b%d: repository(owner: %q, name: %q) { ref(qualifiedName: %q) { associatedPullRequests(first: 1, states: OPEN) { nodes { number %s } } } } `, - i, r.Owner, r.Repo, "refs/heads/"+r.Branch, prFieldsBlock()) + fmt.Fprintf(&b, `b%d: repository(owner: %q, name: %q) { pullRequests(first: %d, states: OPEN, headRefName: %q) { nodes { number %s } } } `, + i, r.Owner, r.Repo, graphQLBranchProbeLimit, r.Branch, prFieldsBlock()) } b.WriteString(`rateLimit { limit remaining resetAt cost } `) b.WriteString(`}`) @@ -483,7 +492,7 @@ func decodeBatchedPRChunk(refs []graphQLPRRef, data map[string]json.RawMessage, } // runBatchedBranchQuery executes the branch-lookup query in chunks and maps -// each branch ref to its first OPEN associated PR (if any). Result keys are +// each branch name to its unambiguous OPEN PR (if any). Result keys are // "owner/repo/branch" so callers can index by their input refs. func runBatchedBranchQuery(ctx context.Context, exec GraphQLExecutor, refs []graphQLBranchRef) (map[string]*PRStatus, error) { if exec == nil || len(refs) == 0 { @@ -502,37 +511,43 @@ func runBatchedBranchQuery(ctx context.Context, exec GraphQLExecutor, refs []gra if err := graphQLErrorsToErr(resp.Errors); err != nil { return nil, err } - decodeBatchedBranchChunk(chunk, resp.Data, result) + if err := decodeBatchedBranchChunk(chunk, resp.Data, result); err != nil { + return nil, err + } } return result, nil } -func decodeBatchedBranchChunk(refs []graphQLBranchRef, data map[string]json.RawMessage, result map[string]*PRStatus) { +func decodeBatchedBranchChunk(refs []graphQLBranchRef, data map[string]json.RawMessage, result map[string]*PRStatus) error { for i, ref := range refs { - raw, ok := data[fmt.Sprintf("b%d", i)] + alias := fmt.Sprintf("b%d", i) + raw, ok := data[alias] if !ok || len(raw) == 0 || string(raw) == "null" { continue } var inner struct { - Ref *struct { - AssociatedPullRequests struct { - Nodes []struct { - Number int `json:"number"` - batchedPRResult - } `json:"nodes"` - } `json:"associatedPullRequests"` - } `json:"ref"` + PullRequests struct { + Nodes []batchedBranchPRNode `json:"nodes"` + } `json:"pullRequests"` } if err := json.Unmarshal(raw, &inner); err != nil { + return fmt.Errorf("decode branch alias %s: %w", alias, err) + } + node, ok := selectBatchedBranchPRNode(inner.PullRequests.Nodes) + if !ok { continue } - if inner.Ref == nil || len(inner.Ref.AssociatedPullRequests.Nodes) == 0 { - continue - } - node := inner.Ref.AssociatedPullRequests.Nodes[0] status := convertBatchedPRResult(&node.batchedPRResult, ref.Owner, ref.Repo, node.Number) result[graphqlBranchKey(ref.Owner, ref.Repo, ref.Branch)] = status } + return nil +} + +func selectBatchedBranchPRNode(nodes []batchedBranchPRNode) (*batchedBranchPRNode, bool) { + if len(nodes) != 1 { + return nil, false + } + return &nodes[0], true } // graphqlBranchKey is the lookup key used in batched-branch result maps. diff --git a/apps/backend/internal/github/graphql_test.go b/apps/backend/internal/github/graphql_test.go index 6a9f887be3..2e3407c6a2 100644 --- a/apps/backend/internal/github/graphql_test.go +++ b/apps/backend/internal/github/graphql_test.go @@ -54,8 +54,11 @@ func TestBuildBatchedBranchQuery_AliasesAllBranches(t *testing.T) { if !strings.Contains(q, `b0: repository`) || !strings.Contains(q, `b1: repository`) { t.Errorf("expected b0/b1 aliases: %s", q) } - if !strings.Contains(q, `qualifiedName: "refs/heads/feat-1"`) { - t.Errorf("expected qualifiedName for feat-1: %s", q) + if !strings.Contains(q, `pullRequests(first: 2, states: OPEN, headRefName: "feat-1")`) { + t.Errorf("expected headRefName lookup for feat-1: %s", q) + } + if strings.Contains(q, `ref(qualifiedName:`) { + t.Errorf("branch lookup should not require a base-repo ref: %s", q) } } @@ -179,24 +182,35 @@ func TestRunBatchedBranchQuery_SurfacesGraphQLErrors(t *testing.T) { } } +func TestRunBatchedBranchQuery_SurfacesDecodeErrors(t *testing.T) { + exec := &stubGraphQLExecutor{ + response: `{"data": {"b0": {"pullRequests": {"nodes": [{"number": "bad"}]}}}}`, + } + _, err := runBatchedBranchQuery(context.Background(), exec, []graphQLBranchRef{{Owner: "o", Repo: "r", Branch: "feat"}}) + if err == nil { + t.Fatalf("expected malformed branch node to return an error") + } + if !strings.Contains(err.Error(), "decode branch alias b0") { + t.Errorf("expected error to identify branch alias, got: %v", err) + } +} + func TestRunBatchedBranchQuery_DecodesPRNode(t *testing.T) { exec := &stubGraphQLExecutor{ response: `{ "data": { "b0": { - "ref": { - "associatedPullRequests": { - "nodes": [{ - "number": 7, - "state": "OPEN", "title": "branch PR", "url": "https://x/7", - "isDraft": false, "mergeable": "MERGEABLE", "mergeStateStatus": "CLEAN", - "headRefName": "feat", "baseRefName": "main", "headRefOid": "deadbeef", - "author": {"login":"alice"}, - "createdAt": "2026-01-01T00:00:00Z", "updatedAt": "2026-01-01T00:00:00Z", - "reviews": {"nodes": []}, "reviewRequests": {"totalCount": 0}, - "commits": {"nodes": []} - }] - } + "pullRequests": { + "nodes": [{ + "number": 7, + "state": "OPEN", "title": "branch PR", "url": "https://x/7", + "isDraft": false, "mergeable": "MERGEABLE", "mergeStateStatus": "CLEAN", + "headRefName": "feat", "baseRefName": "main", "headRefOid": "deadbeef", + "author": {"login":"alice"}, + "createdAt": "2026-01-01T00:00:00Z", "updatedAt": "2026-01-01T00:00:00Z", + "reviews": {"nodes": []}, "reviewRequests": {"totalCount": 0}, + "commits": {"nodes": []} + }] } } } @@ -217,6 +231,111 @@ func TestRunBatchedBranchQuery_DecodesPRNode(t *testing.T) { } } +func TestRunBatchedBranchQuery_EmptyNodesReturnsNoResult(t *testing.T) { + exec := &stubGraphQLExecutor{ + response: `{ + "data": { + "b0": { + "pullRequests": { + "nodes": [] + } + } + } + }`, + } + got, err := runBatchedBranchQuery(context.Background(), exec, []graphQLBranchRef{ + {Owner: "o", Repo: "r", Branch: "feat"}, + }) + if err != nil { + t.Fatalf("run: %v", err) + } + if len(got) != 0 { + t.Fatalf("expected no branch result, got %#v", got) + } +} + +func TestRunBatchedBranchQuery_SkipsAmbiguousForkHeads(t *testing.T) { + exec := &stubGraphQLExecutor{ + response: `{ + "data": { + "b0": { + "pullRequests": { + "nodes": [{ + "number": 7, + "state": "OPEN", "title": "branch PR A", "url": "https://x/7", + "isDraft": false, "mergeable": "MERGEABLE", "mergeStateStatus": "CLEAN", + "headRefName": "feat", "baseRefName": "main", "headRefOid": "deadbeef", + "author": {"login":"alice"}, + "createdAt": "2026-01-01T00:00:00Z", "updatedAt": "2026-01-01T00:00:00Z", + "reviews": {"nodes": []}, "reviewRequests": {"totalCount": 0}, + "commits": {"nodes": []} + }, { + "number": 8, + "state": "OPEN", "title": "branch PR B", "url": "https://x/8", + "isDraft": false, "mergeable": "MERGEABLE", "mergeStateStatus": "CLEAN", + "headRefName": "feat", "baseRefName": "main", "headRefOid": "cafebabe", + "author": {"login":"bob"}, + "createdAt": "2026-01-01T00:00:00Z", "updatedAt": "2026-01-01T00:00:00Z", + "reviews": {"nodes": []}, "reviewRequests": {"totalCount": 0}, + "commits": {"nodes": []} + }] + } + } + } + }`, + } + got, err := runBatchedBranchQuery(context.Background(), exec, []graphQLBranchRef{ + {Owner: "o", Repo: "r", Branch: "feat"}, + }) + if err != nil { + t.Fatalf("run: %v", err) + } + if len(got) != 0 { + t.Fatalf("expected ambiguous fork heads to be skipped, got %#v", got) + } +} + +func TestRunBatchedBranchQuery_SkipsMultipleBranchMatchesEvenWithOwnerMatch(t *testing.T) { + exec := &stubGraphQLExecutor{ + response: `{ + "data": { + "b0": { + "pullRequests": { + "nodes": [{ + "number": 7, + "state": "OPEN", "title": "fork PR", "url": "https://x/7", + "isDraft": false, "mergeable": "MERGEABLE", "mergeStateStatus": "CLEAN", + "headRefName": "feat", "baseRefName": "main", "headRefOid": "deadbeef", + "author": {"login":"alice"}, + "createdAt": "2026-01-01T00:00:00Z", "updatedAt": "2026-01-01T00:00:00Z", + "reviews": {"nodes": []}, "reviewRequests": {"totalCount": 0}, + "commits": {"nodes": []} + }, { + "number": 9, + "state": "OPEN", "title": "base PR", "url": "https://x/9", + "isDraft": false, "mergeable": "MERGEABLE", "mergeStateStatus": "CLEAN", + "headRefName": "feat", "baseRefName": "main", "headRefOid": "cafebabe", + "author": {"login":"o"}, + "createdAt": "2026-01-01T00:00:00Z", "updatedAt": "2026-01-01T00:00:00Z", + "reviews": {"nodes": []}, "reviewRequests": {"totalCount": 0}, + "commits": {"nodes": []} + }] + } + } + } + }`, + } + got, err := runBatchedBranchQuery(context.Background(), exec, []graphQLBranchRef{ + {Owner: "o", Repo: "r", Branch: "feat"}, + }) + if err != nil { + t.Fatalf("run: %v", err) + } + if len(got) != 0 { + t.Fatalf("expected duplicate branch matches to be skipped, got %#v", got) + } +} + func TestSummarizeReviewState_PrefersChangesRequested(t *testing.T) { mk := func(login, state string, sec int) reviewNode { var n reviewNode diff --git a/apps/backend/internal/github/pat_client.go b/apps/backend/internal/github/pat_client.go index 524ae86bcc..da124acc1f 100644 --- a/apps/backend/internal/github/pat_client.go +++ b/apps/backend/internal/github/pat_client.go @@ -166,16 +166,19 @@ func (c *PATClient) GetPR(ctx context.Context, owner, repo string, number int) ( } func (c *PATClient) FindPRByBranch(ctx context.Context, owner, repo, branch string) (*PR, error) { - var raw []patPR - endpoint := fmt.Sprintf("/repos/%s/%s/pulls?head=%s:%s&state=open&per_page=1", - owner, repo, owner, branch) - if err := c.get(ctx, endpoint, &raw); err != nil { - return nil, fmt.Errorf("find PR by branch: %w", err) + statuses, err := runBatchedBranchQuery(ctx, c, []graphQLBranchRef{{ + Owner: owner, + Repo: repo, + Branch: branch, + }}) + if err != nil { + return nil, fmt.Errorf("find PR by branch %q: %w", branch, err) } - if len(raw) == 0 { + status := statuses[graphqlBranchKey(owner, repo, branch)] + if status == nil { return nil, nil } - return convertPatPR(&raw[0], owner, repo), nil + return status.PR, nil } func (c *PATClient) ListAuthoredPRs(ctx context.Context, owner, repo string) ([]*PR, error) { diff --git a/apps/backend/internal/github/pat_client_test.go b/apps/backend/internal/github/pat_client_test.go index cce9bf625f..9d831d96d7 100644 --- a/apps/backend/internal/github/pat_client_test.go +++ b/apps/backend/internal/github/pat_client_test.go @@ -2,6 +2,7 @@ package github import ( "context" + "encoding/json" "errors" "net/http" "net/http/httptest" @@ -154,6 +155,62 @@ func TestConvertPatPR_MergeableState(t *testing.T) { } } +func TestPATClient_FindPRByBranch_UsesGraphQLHeadRefName(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/graphql" { + t.Errorf("unexpected path %q, want /graphql", r.URL.Path) + http.Error(w, "unexpected path", http.StatusInternalServerError) + return + } + if r.Method != http.MethodPost { + t.Errorf("method = %s, want POST", r.Method) + } + var body struct { + Query string `json:"query"` + } + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Errorf("decode request body: %v", err) + http.Error(w, "bad json", http.StatusBadRequest) + return + } + if !strings.Contains(body.Query, `pullRequests(first: 2, states: OPEN, headRefName: "feature")`) { + t.Errorf("query should look up branch by headRefName, got: %s", body.Query) + } + if strings.Contains(body.Query, `head=acme:feature`) || strings.Contains(body.Query, `ref(qualifiedName:`) { + t.Errorf("query should not require a base-repo branch ref, got: %s", body.Query) + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{ + "data": { + "b0": { + "pullRequests": { + "nodes": [{ + "number": 12, + "state": "OPEN", "title": "fork PR", "url": "https://x/12", + "isDraft": false, "mergeable": "MERGEABLE", "mergeStateStatus": "CLEAN", + "headRefName": "feature", "baseRefName": "main", "headRefOid": "abc123", + "author": {"login":"alice"}, + "createdAt": "2026-01-01T00:00:00Z", "updatedAt": "2026-01-01T00:00:00Z", + "reviews": {"nodes": []}, "reviewRequests": {"totalCount": 0}, + "commits": {"nodes": []} + }] + } + } + } + }`)) + })) + t.Cleanup(srv.Close) + + c := newPATClientPointingAt(t, srv.URL) + pr, err := c.FindPRByBranch(context.Background(), "acme", "widget", "feature") + if err != nil { + t.Fatalf("FindPRByBranch: %v", err) + } + if pr == nil || pr.Number != 12 || pr.HeadBranch != "feature" { + t.Fatalf("unexpected PR: %#v", pr) + } +} + func TestConvertPatRequestedReviewers(t *testing.T) { raw := &patPR{ RequestedReviewers: []struct { diff --git a/apps/backend/internal/github/poller_test.go b/apps/backend/internal/github/poller_test.go index f6d1aa1c60..f3d23be2e4 100644 --- a/apps/backend/internal/github/poller_test.go +++ b/apps/backend/internal/github/poller_test.go @@ -641,19 +641,17 @@ func TestTryBatchedPRWatchCheck_SearchingWatch_DetectsPR(t *testing.T) { gh.branchResponses = []string{`{ "data": { "b0": { - "ref": { - "associatedPullRequests": { - "nodes": [{ - "number": 7, - "state": "OPEN", "title": "branch PR", "url": "https://x/7", - "isDraft": false, "mergeable": "MERGEABLE", "mergeStateStatus": "CLEAN", - "headRefName": "feat", "baseRefName": "main", "headRefOid": "deadbeef", - "author": {"login": "alice"}, - "createdAt": "2026-01-01T00:00:00Z", "updatedAt": "2026-01-01T00:00:00Z", - "reviews": {"nodes": []}, "reviewRequests": {"totalCount": 0}, - "commits": {"nodes": []} - }] - } + "pullRequests": { + "nodes": [{ + "number": 7, + "state": "OPEN", "title": "branch PR", "url": "https://x/7", + "isDraft": false, "mergeable": "MERGEABLE", "mergeStateStatus": "CLEAN", + "headRefName": "feat", "baseRefName": "main", "headRefOid": "deadbeef", + "author": {"login": "alice"}, + "createdAt": "2026-01-01T00:00:00Z", "updatedAt": "2026-01-01T00:00:00Z", + "reviews": {"nodes": []}, "reviewRequests": {"totalCount": 0}, + "commits": {"nodes": []} + }] } } }