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
This commit is contained in:
José Almeida
2026-05-31 11:59:15 +01:00
committed by GitHub
parent 137336de86
commit 36b5c682a9
5 changed files with 247 additions and 55 deletions
+35 -20
View File
@@ -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.
+134 -15
View File
@@ -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
+10 -7
View File
@@ -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) {
@@ -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 {
+11 -13
View File
@@ -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": []}
}]
}
}
}