diff --git a/internal/github/build_pr_evidence_test.go b/internal/github/build_pr_evidence_test.go index f3caaff66..4c7c44334 100644 --- a/internal/github/build_pr_evidence_test.go +++ b/internal/github/build_pr_evidence_test.go @@ -220,6 +220,20 @@ func reviewNode(login, state, submittedAt, commit string) graphqlReviewNode { return r } +func TestBuildPREvidence_UnnamedReviewIsKeptOnlyAsAChangeRequest(t *testing.T) { + evidence, err := buildPREvidence("u", "m", "MERGED", "tooky", "2026-03-01T09:00:00Z", "", + "title", "feature", "main", "h", nil, + []graphqlReviewNode{ + reviewNode("", "COMMENTED", "2026-03-01T13:00:00Z", ""), + reviewNode("ghost", "DISMISSED", "2026-03-01T13:00:00Z", ""), + reviewNode("", "APPROVED", "2026-03-01T13:00:00Z", ""), + reviewNode("ghost", "CHANGES_REQUESTED", "2026-03-01T13:00:00Z", ""), + }, nil) + require.NoError(t, err) + require.Equal(t, []types.PRApprovals{{State: "CHANGES_REQUESTED", Timestamp: 1772370000}}, evidence.Reviews) + require.Empty(t, evidence.Approvers) +} + func TestBuildPREvidence_RecordsWhichCommitAnApprovalCovered(t *testing.T) { evidence, err := buildPREvidence( "https://github.com/kosli-dev/cli/pull/671", diff --git a/internal/github/github.go b/internal/github/github.go index afd3c5dc0..1c7b58d46 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -252,9 +252,7 @@ func (c *GithubConfig) PREvidenceForCommitHybrid(commit string) ([]*types.PREvid if err != nil { return nil, err } - if evidence != nil { - result = append(result, evidence) - } + result = append(result, evidence) } return result, nil } @@ -293,11 +291,12 @@ const signerBatchSize = 100 // signerLogins maps each signed commit's SHA to its signer's login. GitHub // types signer as User, so a signer that is not one (e.g. a GitHub App) -// comes back null with a field error. The client decodes data before -// returning errors but drops their path, so any error that came with data -// is accepted and written to debug, when not nil. +// comes back null with a field error, which newGraphQLClient drops. +// Unlike the other queries, any error that comes with data is accepted and +// written to debug output: a node asked for by id that comes back null, or +// with a field GitHub could not resolve, only loses its signer. func (c *GithubConfig) signerLogins(ctx context.Context, query graphqlQueryFunc, - nodes []graphqlCommitNode, debug io.Writer) (map[string]string, error) { + nodes []graphqlCommitNode) (map[string]string, error) { ids := []graphql.ID{} for _, n := range nodes { if n.Commit.Signature != nil { @@ -305,7 +304,7 @@ func (c *GithubConfig) signerLogins(ctx context.Context, query graphqlQueryFunc, } } type signerQuery struct { - Nodes []struct { + Nodes []*struct { Commit struct { Oid graphql.String Signature *struct { @@ -319,13 +318,11 @@ func (c *GithubConfig) signerLogins(ctx context.Context, query graphqlQueryFunc, signers := map[string]string{} for batch := range slices.Chunk(ids, signerBatchSize) { var q signerQuery - // Only a reply with no data is retried: a field error comes back - // the same on every attempt. err := c.retry(ctx, func() error { q = signerQuery{} err := query(ctx, &q, map[string]any{"ids": batch}) if err != nil && len(q.Nodes) > 0 { - if debug != nil { + if debug := c.debugOut(); debug != nil { _, _ = fmt.Fprintf(debug, "[debug-github] ignoring error in commit signer lookup: %v\n", err) } return nil @@ -336,6 +333,9 @@ func (c *GithubConfig) signerLogins(ctx context.Context, query graphqlQueryFunc, return nil, fmt.Errorf("failed to look up commit signers: %w", err) } for _, n := range q.Nodes { + if n == nil { + continue + } if sig := n.Commit.Signature; sig != nil && sig.Signer != nil { signers[string(n.Commit.Oid)] = string(sig.Signer.Login) } @@ -375,17 +375,13 @@ func buildPREvidence( reviewNodes []graphqlReviewNode, signers map[string]string, ) (*types.PREvidence, error) { - createdAt, err := time.Parse(time.RFC3339, createdAtStr) + createdAt, err := unixTime(createdAtStr) if err != nil { return nil, err } - mergedAt := int64(0) - if mergedAtStr != "" { - mergedAtTime, err := time.Parse(time.RFC3339, mergedAtStr) - if err != nil { - return nil, err - } - mergedAt = mergedAtTime.Unix() + mergedAt, err := unixTime(mergedAtStr) + if err != nil { + return nil, err } evidence := &types.PREvidence{ @@ -393,7 +389,7 @@ func buildPREvidence( MergeCommit: mergeCommit, State: state, Author: author, - CreatedAt: createdAt.Unix(), + CreatedAt: createdAt, MergedAt: mergedAt, Title: title, HeadRef: headRef, @@ -410,7 +406,7 @@ func buildPREvidence( if dateStr == "" { dateStr = string(n.Commit.CommittedDate) } - timestamp, err := time.Parse(time.RFC3339, dateStr) + timestamp, err := unixTime(dateStr) if err != nil { return nil, err } @@ -431,12 +427,17 @@ func buildPREvidence( signatureState = &s signedByPlatform = &p } + // A commit with no author is recorded as "", not " <>". + author := "" + if n.Commit.Author.Name != "" || n.Commit.Author.Email != "" { + author = fmt.Sprintf("%s <%s>", string(n.Commit.Author.Name), string(n.Commit.Author.Email)) + } evidence.Commits = append(evidence.Commits, types.Commit{ SHA: string(n.Commit.Oid), Message: string(n.Commit.MessageHeadline), - Author: fmt.Sprintf("%s <%s>", string(n.Commit.Author.Name), string(n.Commit.Author.Email)), + Author: author, AuthorUsername: authorUsername, - Timestamp: timestamp.Unix(), + Timestamp: timestamp, Branch: headRef, URL: string(n.Commit.URL), Verified: verified, @@ -447,14 +448,18 @@ func buildPREvidence( } for _, r := range reviewNodes { - submittedAt, err := time.Parse(time.RFC3339, string(r.SubmittedAt)) + login, keep := reviewer(string(r.Author.Login), string(r.State)) + if !keep { + continue + } + submittedAt, err := unixTime(string(r.SubmittedAt)) if err != nil { return nil, err } review := types.PRApprovals{ - Username: string(r.Author.Login), + Username: login, State: string(r.State), - Timestamp: submittedAt.Unix(), + Timestamp: submittedAt, } if r.Commit != nil { review.CommitSHA = string(r.Commit.Oid) @@ -466,17 +471,52 @@ func buildPREvidence( return evidence, nil } +// reviewer returns a review's login and whether to keep the review. GitHub +// sends a deleted account as a null author or as the "ghost" login. Policies +// read reviews as well as approvers, so a review with no account is kept only +// as a change request: dropping that would read as none given. +func reviewer(login, state string) (string, bool) { + if login == "ghost" { + login = "" + } + return login, login != "" || state == types.ReviewChangesRequested +} + +// unixTime parses a GitHub timestamp. A null one reads as "" and gives 0. +func unixTime(s string) (int64, error) { + if s == "" { + return 0, nil + } + t, err := time.Parse(time.RFC3339, s) + if err != nil { + return 0, err + } + return t.Unix(), nil +} + +// nonNull returns items as values. A null item is an entry the reply could +// not give, so it fails rather than being skipped. owner says what the list +// belongs to. +func nonNull[T any](items []*T, list, owner string) ([]T, error) { + out := make([]T, 0, len(items)) + for i, item := range items { + if item == nil { + return nil, fmt.Errorf("%s[%d] for %s is null in the GitHub reply", list, i, owner) + } + out = append(out, *item) + } + return out, nil +} + // PREvidenceByPRNumber fetches full PR evidence for a single PR number via // GraphQL. Returns an error when the PR does not exist. func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, error) { ctx := context.Background() - ghClient, err := NewGithubClientFromToken(ctx, c.Token, c.BaseURL, c.Debug) + client, err := c.newGraphQLClient(ctx) if err != nil { return nil, err } - httpClient := ghClient.Client() - client := graphql.NewClient(graphqlEndpoint(c.BaseURL), httpClient) var query struct { Repository struct { @@ -496,11 +536,11 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er } HeadRefOid graphql.String Commits struct { - Nodes []graphqlCommitNode + Nodes []*graphqlCommitNode PageInfo pageInfo } `graphql:"commits(first: 100, after: $commitCursor)"` Reviews struct { - Nodes []graphqlReviewNode + Nodes []*graphqlReviewNode PageInfo pageInfo } `graphql:"reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED], after: $reviewCursor)"` } `graphql:"pullRequest(number: $prNumber)"` @@ -522,7 +562,7 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er pr := query.Repository.PullRequest if pr == nil { - return nil, nil + return nil, fmt.Errorf("could not find pull request %d in %s/%s", prNumber, c.Org, c.Repository) } mergeCommit := "" @@ -539,7 +579,7 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er if err != nil { return nil, err } - signers, err := c.signerLogins(ctx, client.Query, commits, c.debugOut()) + signers, err := c.signerLogins(ctx, client.Query, commits) if err != nil { return nil, err } @@ -555,20 +595,17 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence ctx := context.Background() pullRequestsEvidence := []*types.PREvidence{} - ghClient, err := NewGithubClientFromToken(ctx, c.Token, c.BaseURL, c.Debug) + client, err := c.newGraphQLClient(ctx) if err != nil { return pullRequestsEvidence, err } - httpClient := ghClient.Client() - - client := graphql.NewClient(graphqlEndpoint(c.BaseURL), httpClient) var query struct { - Repository struct { + Repository *struct { Object struct { Commit struct { AssociatedPullRequests struct { - Nodes []struct { + Nodes []*struct { Number graphql.Int // Follow-up page queries resolve the PR by number, so they // need the repo it actually lives in, not the configured one. @@ -592,12 +629,12 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence } Commits struct { - Nodes []graphqlCommitNode + Nodes []*graphqlCommitNode PageInfo pageInfo } `graphql:"commits(first: 100, after: $commitCursor)"` Reviews struct { - Nodes []graphqlReviewNode + Nodes []*graphqlReviewNode PageInfo pageInfo } `graphql:"reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED], after: $reviewCursor)"` } @@ -622,11 +659,18 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence if err := client.Query(ctx, &query, variables); err != nil { return pullRequestsEvidence, err } + if query.Repository == nil { + return pullRequestsEvidence, fmt.Errorf("could not find repository %s/%s", c.Org, c.Repository) + } + prs, err := nonNull(query.Repository.Object.Commit.AssociatedPullRequests.Nodes, "associatedPullRequests", "commit "+commit) + if err != nil { + return pullRequestsEvidence, err + } // Only the follow-up pages retry; the initial query keeps its fail-fast // behaviour, which callers already depend on. retrying := c.queryWithRetry(client) - for _, pr := range query.Repository.Object.Commit.AssociatedPullRequests.Nodes { + for _, pr := range prs { ref := prRef{ Owner: string(pr.Repository.Owner.Login), Repo: string(pr.Repository.Name), @@ -648,7 +692,7 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence if err != nil { return pullRequestsEvidence, err } - signers, err := c.signerLogins(ctx, client.Query, commits, c.debugOut()) + signers, err := c.signerLogins(ctx, client.Query, commits) if err != nil { return pullRequestsEvidence, err } @@ -762,8 +806,8 @@ func (c *GithubConfig) listApprovers(ctx context.Context, client *gh.Client, return nil, err } for _, r := range reviews { - login := r.GetUser().GetLogin() - if r.GetState() == "APPROVED" && !seen[login] { + login, keep := reviewer(r.GetUser().GetLogin(), r.GetState()) + if r.GetState() == types.ReviewApproved && keep && !seen[login] { seen[login] = true approvers = append(approvers, login) } diff --git a/internal/github/github_test.go b/internal/github/github_test.go index 220332627..1dfa5faf2 100644 --- a/internal/github/github_test.go +++ b/internal/github/github_test.go @@ -78,10 +78,6 @@ func TestGithubTestSuite(t *testing.T) { suite.Run(t, new(GithubTestSuite)) } -// graphqlNullPRResponse is a minimal valid GraphQL response where the PR is null. -// PREvidenceByPRNumber returns nil, nil in this case. -const graphqlNullPRResponse = `{"data":{"repository":{"pullRequest":null}}}` - func newRetryTestServer(t *testing.T, failCount int) (*httptest.Server, *int) { t.Helper() attempts := 0 @@ -96,7 +92,7 @@ func newRetryTestServer(t *testing.T, failCount int) (*httptest.Server, *int) { return } w.Header().Set("Content-Type", "application/json") - _, _ = fmt.Fprint(w, graphqlNullPRResponse) + _, _ = fmt.Fprint(w, byPRNumberResponse(connectionJSON(nil, ""), connectionJSON(nil, ""))) })) return ts, &attempts } @@ -120,7 +116,7 @@ func TestPREvidenceByPRNumber_RetriesOnGraphQLError(t *testing.T) { pr, err := config.PREvidenceByPRNumber(1) require.NoError(t, err) - require.Nil(t, pr) + require.NotNil(t, pr) require.Equal(t, 3, *attempts, "should have retried twice before succeeding") require.Len(t, sleptDurations, 2, "should have slept between retries") } diff --git a/internal/github/graphql_client.go b/internal/github/graphql_client.go new file mode 100644 index 000000000..6365a2777 --- /dev/null +++ b/internal/github/graphql_client.go @@ -0,0 +1,137 @@ +package github + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + + "github.com/shurcooL/graphql" +) + +// newGraphQLClient is the one GraphQL client every evidence query uses. +func (c *GithubConfig) newGraphQLClient(ctx context.Context) (*graphql.Client, error) { + ghClient, err := NewGithubClientFromToken(ctx, c.Token, c.BaseURL, c.Debug) + if err != nil { + return nil, err + } + httpClient := &http.Client{Transport: &nullFieldErrorTransport{ + base: ghClient.Client().Transport, + debug: c.debugOut(), + }} + return graphql.NewClient(graphqlEndpoint(c.BaseURL), httpClient), nil +} + +// nullFieldErrorTransport drops each GraphQL error that only explains why one +// field came back null, so a null field that is not a list reads the same +// with or without an error. The client returns any error at all as a failure +// and drops its path, so this is the only place the path can still be checked. +type nullFieldErrorTransport struct { + base http.RoundTripper + debug io.Writer +} + +func (t *nullFieldErrorTransport) RoundTrip(req *http.Request) (*http.Response, error) { + resp, err := t.base.RoundTrip(req) + if err != nil || resp.StatusCode != http.StatusOK { + return resp, err + } + body, err := io.ReadAll(resp.Body) + _ = resp.Body.Close() + if err != nil { + return nil, err + } + body = t.withoutNullFieldErrors(body) + resp.Body = io.NopCloser(bytes.NewReader(body)) + resp.ContentLength = int64(len(body)) + resp.Header.Del("Content-Length") + return resp, nil +} + +// withoutNullFieldErrors returns body minus the errors isNullField accepts, +// or body unchanged when it cannot be read or nothing is dropped. +func (t *nullFieldErrorTransport) withoutNullFieldErrors(body []byte) []byte { + var reply map[string]json.RawMessage + if json.Unmarshal(body, &reply) != nil { + return body + } + var errs []json.RawMessage + var data any + if json.Unmarshal(reply["errors"], &errs) != nil || json.Unmarshal(reply["data"], &data) != nil { + return body + } + kept := []json.RawMessage{} + for _, e := range errs { + var parsed struct{ Path []any } + if json.Unmarshal(e, &parsed) == nil && isNullField(data, parsed.Path) { + if t.debug != nil { + _, _ = fmt.Fprintf(t.debug, "[debug-github] ignoring error on null field: %s\n", e) + } + continue + } + kept = append(kept, e) + } + if len(kept) == len(errs) { + return body + } + if len(kept) == 0 { + delete(reply, "errors") + } else { + reply["errors"], _ = json.Marshal(kept) + } + out, err := json.Marshal(reply) + if err != nil { + return body + } + return out +} + +// listFields are the list and connection fields the evidence queries select. +// A null one decodes as an empty list, so its error is the only sign entries +// are missing. +var listFields = map[string]bool{"nodes": true, "commits": true, "reviews": true, "associatedPullRequests": true} + +// isNullField reports whether path names an object field that is null in +// data while everything above it is not. An error anywhere else (a list item, +// a field under a null parent, no path) is more than one missing field. So is +// one within two segments of the root, such as repository or +// repository.pullRequest: that is what the query is about, and GitHub's +// message (e.g. SAML enforcement) is the user's only clue to the cause. +// A field in listFields never counts. +func isNullField(data any, path []any) bool { + if len(path) <= 2 { + return false + } + field, _ := path[len(path)-1].(string) + if listFields[field] { + return false + } + cur := data + for _, seg := range path[:len(path)-1] { + switch s := seg.(type) { + case string: + obj, ok := cur.(map[string]any) + if !ok { + return false + } + cur = obj[s] + case float64: + list, ok := cur.([]any) + if !ok || s < 0 || int(s) >= len(list) { + return false + } + cur = list[int(s)] + default: + return false + } + } + // A path ending in a list index ends on a list, not an object. + obj, ok := cur.(map[string]any) + if !ok { + return false + } + value, present := obj[field] + return present && value == nil +} diff --git a/internal/github/graphql_client_test.go b/internal/github/graphql_client_test.go new file mode 100644 index 000000000..23afc8755 --- /dev/null +++ b/internal/github/graphql_client_test.go @@ -0,0 +1,67 @@ +package github + +import ( + "encoding/json" + "io" + "net/http" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestIsNullField(t *testing.T) { + for _, tc := range []struct { + name string + data string + path []any + want bool + }{ + {"a null field", `{"repository":{"pullRequest":{"mergedAt":null}}}`, + []any{"repository", "pullRequest", "mergedAt"}, true}, + {"a field with a value", `{"repository":{"pullRequest":{"title":"A PR"}}}`, + []any{"repository", "pullRequest", "title"}, false}, + {"a list item", `{"repository":{"pullRequest":{"commits":{"nodes":[{"x":null}]}}}}`, + []any{"repository", "pullRequest", "commits", "nodes", 0.0}, false}, + {"a root field", `{"repository":null}`, []any{"repository"}, false}, + {"a field two segments down", `{"repository":{"pullRequest":null}}`, + []any{"repository", "pullRequest"}, false}, + {"a connection", `{"repository":{"pullRequest":{"reviews":null}}}`, + []any{"repository", "pullRequest", "reviews"}, false}, + {"a non-null connection", `{"repository":{"pullRequest":{"commits":null}}}`, + []any{"repository", "pullRequest", "commits"}, false}, + {"a connection's nodes", `{"repository":{"pullRequest":{"commits":{"nodes":null}}}}`, + []any{"repository", "pullRequest", "commits", "nodes"}, false}, + {"a commit's pull requests", `{"repository":{"object":{"associatedPullRequests":null}}}`, + []any{"repository", "object", "associatedPullRequests"}, false}, + } { + t.Run(tc.name, func(t *testing.T) { + var data any + require.NoError(t, json.Unmarshal([]byte(tc.data), &data)) + require.Equal(t, tc.want, isNullField(data, tc.path)) + }) + } +} + +type replyTransport string + +func (r replyTransport) RoundTrip(*http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, ContentLength: int64(len(r)), + Header: http.Header{"Content-Length": {strconv.Itoa(len(r))}}, + Body: io.NopCloser(strings.NewReader(string(r)))}, nil +} + +// TestNullFieldErrorTransport_LengthMatchesTheNewBody stops a reader that +// trusts the length from reading the old body's size. +func TestNullFieldErrorTransport_LengthMatchesTheNewBody(t *testing.T) { + reply := `{"data":{"repository":{"pullRequest":{"mergedAt":null}}},` + + `"errors":[{"message":"x","path":["repository","pullRequest","mergedAt"]}]}` + resp, err := (&nullFieldErrorTransport{base: replyTransport(reply)}).RoundTrip(&http.Request{}) + require.NoError(t, err) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Less(t, len(body), len(reply), "the error should have been dropped") + require.Equal(t, int64(len(body)), resp.ContentLength) + require.Empty(t, resp.Header.Get("Content-Length")) +} diff --git a/internal/github/odd_replies_test.go b/internal/github/odd_replies_test.go new file mode 100644 index 000000000..075de0f29 --- /dev/null +++ b/internal/github/odd_replies_test.go @@ -0,0 +1,631 @@ +package github + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "regexp" + "sort" + "strconv" + "strings" + "testing" + "time" + + "github.com/kosli-dev/cli/internal/testHelpers" + "github.com/kosli-dev/cli/internal/types" + "github.com/stretchr/testify/require" +) + +// The odd-reply tests feed the PR evidence path replies that GitHub's schema +// allows but the baseline does not use: a nullable field made null, or a list +// emptied, with and without an errors entry naming it (v2.48.0 broke on a null +// signature.signer). They assert the rule in kosli-dev/cli#1283. + +// Reply names: one per kind of query the evidence path sends. +const ( + replyByNumber = "byNumber" + replyForCommit = "forCommit" + replyCommitPage = "commitsPage" + replyReviewPage = "reviewsPage" + replySigners = "signers" +) + +// oddReplyQueryRoutes picks the reply for a request by its query text. Order +// matters: the main queries also contain "commits(first: 100". +var oddReplyQueryRoutes = []struct{ contains, reply string }{ + {"nodes(ids:", replySigners}, + {"associatedPullRequests", replyForCommit}, + {"$commitCursor", replyByNumber}, + {"commits(first: 100, after: $cursor)", replyCommitPage}, + {"reviews(first: 100", replyReviewPage}, +} + +// newOddReplyServer answers each GraphQL query with the reply its text routes +// to, however many times it is asked, so retries and extra pages never run +// the server out of replies. +func newOddReplyServer(t *testing.T, replies map[string]string) *httptest.Server { + t.Helper() + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + var req struct{ Query string } + _ = json.Unmarshal(body, &req) + for _, route := range oddReplyQueryRoutes { + if strings.Contains(req.Query, route.contains) { + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprint(w, replies[route.reply]) + return + } + } + t.Errorf("no reply routed for query: %s", req.Query) + w.WriteHeader(http.StatusInternalServerError) + })) + t.Cleanup(ts.Close) + return ts +} + +const ( + // sha1 is signed by a user GitHub resolves; sha2 is unsigned. + oddSignedCommit = `{"commit":{"id":"C_sha1","oid":"sha1","messageHeadline":"first",` + + `"committedDate":"2026-03-01T12:00:00Z","authoredDate":"2026-03-01T11:30:00Z",` + + `"url":"https://github.com/test-org/test-repo/commit/sha1",` + + `"author":{"name":"Ada","email":"ada@example.com","user":{"login":"ada"}},` + + `"signature":{"isValid":true,"state":"VALID","wasSignedByGitHub":false}}}` + oddUnsignedCommit = `{"commit":{"id":"C_sha2","oid":"sha2","messageHeadline":"second",` + + `"committedDate":"2026-03-01T12:30:00Z","authoredDate":"2026-03-01T12:15:00Z",` + + `"url":"https://github.com/test-org/test-repo/commit/sha2",` + + `"author":{"name":"Ada","email":"ada@example.com","user":{"login":"ada"}},` + + `"signature":null}}` + oddApproval = `{"author":{"login":"grace"},"state":"APPROVED",` + + `"submittedAt":"2026-03-01T13:00:00Z","commit":{"oid":"sha2"}}` + oddChangeRequest = `{"author":{"login":"linus"},"state":"CHANGES_REQUESTED",` + + `"submittedAt":"2026-03-01T12:45:00Z","commit":{"oid":"sha1"}}` + + // The main queries return one commit and one review with a next page, so + // both follow-up page queries run. + oddPRFields = `"title":"A PR","state":"MERGED","headRefName":"feature","baseRefName":"main",` + + `"url":"https://github.com/test-org/test-repo/pull/7","createdAt":"2026-03-01T11:00:00Z",` + + `"mergedAt":"2026-03-01T14:00:00Z",` + oddPRConnections = `"author":{"login":"ada"},"headRefOid":"sha2",` + + `"commits":{"nodes":[` + oddSignedCommit + `],"pageInfo":{"hasNextPage":true,"endCursor":"c1"}},` + + `"reviews":{"nodes":[` + oddApproval + `],"pageInfo":{"hasNextPage":true,"endCursor":"r1"}}` + + oddByNumberReply = `{"data":{"repository":{"pullRequest":{` + oddPRFields + + `"mergeCommit":{"oid":"merge-sha"},` + oddPRConnections + `}}}}` + oddForCommitReply = `{"data":{"repository":{"object":{"associatedPullRequests":{"nodes":[{"number":7,` + + `"repository":{"name":"test-repo","owner":{"login":"test-org"}},` + oddPRFields + oddPRConnections + `}]}}}}}` + oddCommitPageReply = `{"data":{"repository":{"pullRequest":{"commits":{"nodes":[` + oddUnsignedCommit + + `],"pageInfo":{"hasNextPage":false,"endCursor":"c2"}}}}}}` + oddReviewPageReply = `{"data":{"repository":{"pullRequest":{"reviews":{"nodes":[` + oddChangeRequest + + `],"pageInfo":{"hasNextPage":false,"endCursor":"r2"}}}}}}` + oddSignersReply = `{"data":{"nodes":[{"oid":"sha1","signature":{"signer":{"login":"ada"}}}]}}` +) + +func oddBaselineReplies() map[string]string { + return map[string]string{ + replyByNumber: oddByNumberReply, + replyForCommit: oddForCommitReply, + replyCommitPage: oddCommitPageReply, + replyReviewPage: oddReviewPageReply, + replySigners: oddSignersReply, + } +} + +// oddNullable lists the reply paths whose GitHub schema type is nullable, for +// every field the evidence queries select. "[]" stands for a list item. +// Source: https://docs.github.com/public/fpt/schema.docs.graphql, checked +// 2026-10-21. GraphQL never returns null for a non-null (!) field, so only +// these are varied; an emptied list is legal for every list and always varied. +var oddNullable = map[string]bool{ + // Query.repository: Repository; Repository.pullRequest: PullRequest + "data.repository": true, + "data.repository.pullRequest": true, + // PullRequest.mergedAt: DateTime; mergeCommit: Commit; author: Actor + "data.repository.pullRequest.mergedAt": true, + "data.repository.pullRequest.mergeCommit": true, + "data.repository.pullRequest.author": true, + // PullRequestCommitConnection.nodes: [PullRequestCommit]; PageInfo.endCursor: String + "data.repository.pullRequest.commits.nodes": true, + "data.repository.pullRequest.commits.nodes[]": true, + "data.repository.pullRequest.commits.pageInfo.endCursor": true, + // Commit.author: GitActor; GitActor.name/email: String; user: User; Commit.signature: GitSignature + "data.repository.pullRequest.commits.nodes[].commit.author": true, + "data.repository.pullRequest.commits.nodes[].commit.author.name": true, + "data.repository.pullRequest.commits.nodes[].commit.author.email": true, + "data.repository.pullRequest.commits.nodes[].commit.author.user": true, + "data.repository.pullRequest.commits.nodes[].commit.signature": true, + // PullRequest.reviews: PullRequestReviewConnection; nodes: [PullRequestReview] + "data.repository.pullRequest.reviews": true, + "data.repository.pullRequest.reviews.nodes": true, + "data.repository.pullRequest.reviews.nodes[]": true, + "data.repository.pullRequest.reviews.pageInfo.endCursor": true, + // PullRequestReview.author: Actor; submittedAt: DateTime; commit: Commit + "data.repository.pullRequest.reviews.nodes[].author": true, + "data.repository.pullRequest.reviews.nodes[].submittedAt": true, + "data.repository.pullRequest.reviews.nodes[].commit": true, + + // Repository.object: GitObject; Commit.associatedPullRequests: PullRequestConnection; + // PullRequestConnection.nodes: [PullRequest] + "data.repository.object": true, + "data.repository.object.associatedPullRequests": true, + "data.repository.object.associatedPullRequests.nodes": true, + "data.repository.object.associatedPullRequests.nodes[]": true, + "data.repository.object.associatedPullRequests.nodes[].mergedAt": true, + "data.repository.object.associatedPullRequests.nodes[].author": true, + "data.repository.object.associatedPullRequests.nodes[].commits.nodes": true, + "data.repository.object.associatedPullRequests.nodes[].commits.nodes[]": true, + "data.repository.object.associatedPullRequests.nodes[].commits.pageInfo.endCursor": true, + "data.repository.object.associatedPullRequests.nodes[].commits.nodes[].commit.author": true, + "data.repository.object.associatedPullRequests.nodes[].commits.nodes[].commit.author.name": true, + "data.repository.object.associatedPullRequests.nodes[].commits.nodes[].commit.author.email": true, + "data.repository.object.associatedPullRequests.nodes[].commits.nodes[].commit.author.user": true, + "data.repository.object.associatedPullRequests.nodes[].commits.nodes[].commit.signature": true, + "data.repository.object.associatedPullRequests.nodes[].reviews": true, + "data.repository.object.associatedPullRequests.nodes[].reviews.nodes": true, + "data.repository.object.associatedPullRequests.nodes[].reviews.nodes[]": true, + "data.repository.object.associatedPullRequests.nodes[].reviews.pageInfo.endCursor": true, + "data.repository.object.associatedPullRequests.nodes[].reviews.nodes[].author": true, + "data.repository.object.associatedPullRequests.nodes[].reviews.nodes[].submittedAt": true, + "data.repository.object.associatedPullRequests.nodes[].reviews.nodes[].commit": true, + + // Query.nodes: [Node]! (items nullable); Commit.signature: GitSignature; GitSignature.signer: User + "data.nodes[]": true, + "data.nodes[].signature": true, + "data.nodes[].signature.signer": true, +} + +var listIndex = regexp.MustCompile(`\[\d+\]`) + +// oddVariants returns the schema-legal variants of reply: nullable fields made +// null and lists emptied. +func oddVariants(t *testing.T, reply string) []testHelpers.NullVariant { + t.Helper() + all, err := testHelpers.NullVariants([]byte(reply)) + require.NoError(t, err) + legal := []testHelpers.NullVariant{} + for _, v := range all { + if strings.HasSuffix(v.Path, "[]") || oddNullable[listIndex.ReplaceAllString(v.Path, "[]")] { + legal = append(legal, v) + } + } + return legal +} + +type oddEntryPoint struct { + name string + replies []string // the replies this entry point's queries read + run func(c *GithubConfig) (*types.PREvidence, error) +} + +var oddEntryPoints = []oddEntryPoint{ + { + name: "PREvidenceByPRNumber", + replies: []string{replyByNumber, replyCommitPage, replyReviewPage, replySigners}, + run: func(c *GithubConfig) (*types.PREvidence, error) { return c.PREvidenceByPRNumber(7) }, + }, + { + name: "PREvidenceForCommitV2", + replies: []string{replyForCommit, replyCommitPage, replyReviewPage, replySigners}, + run: func(c *GithubConfig) (*types.PREvidence, error) { + prs, err := c.PREvidenceForCommitV2("merge-sha") + if len(prs) == 0 { + return nil, err + } + return prs[0], err + }, + }, +} + +// runOddReply runs ep against replies and describes the result: the outcome, +// plus the evidence fields that differ from want when it is ok. +func runOddReply(t *testing.T, ep oddEntryPoint, replies map[string]string, want *types.PREvidence) string { + t.Helper() + ts := newOddReplyServer(t, replies) + config := &GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", Repository: "test-repo", + Sleep: func(time.Duration) {}} + return testHelpers.EvidenceOutcome([]*types.PREvidence{want}, func() ([]*types.PREvidence, error) { + got, err := ep.run(config) + if got == nil { + return nil, err + } + return []*types.PREvidence{got}, err + }) +} + +func oddBaselineEvidence(t *testing.T, ep oddEntryPoint) *types.PREvidence { + t.Helper() + ts := newOddReplyServer(t, oddBaselineReplies()) + evidence, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", + Repository: "test-repo", Sleep: func(time.Duration) {}}) + require.NoError(t, err) + require.NotNil(t, evidence) + return evidence +} + +func TestOddReplies_BaselineGivesFullEvidence(t *testing.T) { + for _, ep := range oddEntryPoints { + t.Run(ep.name, func(t *testing.T) { + e := oddBaselineEvidence(t, ep) + require.Equal(t, "merge-sha", e.MergeCommit) + require.Equal(t, "MERGED", e.State) + require.Equal(t, "ada", e.Author) + require.NotZero(t, e.CreatedAt) + require.NotZero(t, e.MergedAt) + require.Equal(t, "sha2", e.HeadSHA) + require.Equal(t, []string{"sha1", "sha2"}, shasIn(e)) + require.Equal(t, "ada", e.Commits[0].SignerUsername) + require.True(t, *e.Commits[0].Verified) + require.Nil(t, e.Commits[1].Verified) + require.Len(t, e.Reviews, 2) + require.Equal(t, "sha2", e.Reviews[0].CommitSHA) + require.Equal(t, "CHANGES_REQUESTED", e.Reviews[1].State) + require.Len(t, e.Approvers, 1) + }) + } +} + +// oddAllowedErrors are the nullable fields whose null fails the evidence +// lookup instead of leaving part of it empty, keyed as in oddNullable. +var oddAllowedErrors = map[string]string{ + "data.repository": "the repository every query is about; null means it is missing or unreadable", + "data.repository.pullRequest": "the pull request asked for by number; null means it is missing or unreadable", +} + +// oddWholePRLost are the V2 fields whose null means the commit has no pull +// requests, which is a valid answer. +var oddWholePRLost = map[string]bool{ + "data.repository.object": true, + "data.repository.object.associatedPullRequests": true, + "data.repository.object.associatedPullRequests.nodes": true, +} + +// oddAffected is the evidence a null at path may empty or change. +func oddAffected(reply, path string) []string { + switch { + case reply == replySigners || strings.Contains(path, ".commits"): + return []string{"commits"} + case strings.Contains(path, ".reviews"): + return []string{"approvers", "reviews"} + } + last := path[strings.LastIndex(path, ".")+1:] + return map[string][]string{ + "mergedAt": {"merged_at"}, + "mergeCommit": {"merge_commit"}, + "author": {"author"}, + }[last] +} + +// requireRuleHolds checks one outcome against the rule for a variant of reply +// with the value at path made null, or emptied for a path ending "[]". +func requireRuleHolds(t *testing.T, reply, path, outcome string) { + t.Helper() + require.NotContains(t, outcome, "panic") + key := listIndex.ReplaceAllString(path, "[]") + switch { + case strings.HasSuffix(path, "[]"): + require.NotEqual(t, "error", outcome, "an emptied list is not an error") + case strings.HasSuffix(path, "]") && reply != replySigners: + // A null signer node only loses its signer, which is optional. + require.Equal(t, "error", outcome, "a null list item is missing data, not an optional field") + case oddAllowedErrors[key] != "": + require.Equal(t, "error", outcome, oddAllowedErrors[key]) + case isApprovalAuthor(t, reply, path): + // A deleted account's approval is left out of reviews and approvers. + require.True(t, strings.HasPrefix(outcome, "ok"), "a review by a deleted account is legal, got %q", outcome) + require.Contains(t, outcome, ":approvers", "an approval by nobody must not count") + case oddWholePRLost[key]: + require.Equal(t, "ok lost:PullRequests(1->0)", outcome) + default: + require.True(t, strings.HasPrefix(outcome, "ok"), "a null field is not an error, got %q", outcome) + allowed := oddAffected(reply, path) + for _, diff := range strings.Fields(outcome)[1:] { + _, field, _ := strings.Cut(diff, ":") + require.Contains(t, allowed, field, "a null at %s changed unrelated evidence", path) + } + } +} + +// TestOddReplies_NullsFollowTheRule feeds each entry point every legal +// variant of every reply it reads. +func TestOddReplies_NullsFollowTheRule(t *testing.T) { + for _, ep := range oddEntryPoints { + want := oddBaselineEvidence(t, ep) + for _, reply := range ep.replies { + for _, v := range oddVariants(t, oddBaselineReplies()[reply]) { + t.Run(ep.name+"/"+reply+"/"+v.Path, func(t *testing.T) { + replies := oddBaselineReplies() + replies[reply] = string(v.Body) + requireRuleHolds(t, reply, v.Path, runOddReply(t, ep, replies, want)) + }) + } + } + } +} + +// TestOddReplies_FieldErrorMeansTheSameAsNull adds an errors entry naming +// each nullable field made null, as GitHub does when it cannot resolve one. +func TestOddReplies_FieldErrorMeansTheSameAsNull(t *testing.T) { + for _, ep := range oddEntryPoints { + want := oddBaselineEvidence(t, ep) + for _, reply := range ep.replies { + for _, v := range oddVariants(t, oddBaselineReplies()[reply]) { + if strings.HasSuffix(v.Path, "[]") { + continue + } + t.Run(ep.name+"/"+reply+"/"+v.Path, func(t *testing.T) { + plain := oddBaselineReplies() + plain[reply] = string(v.Body) + withError := oddBaselineReplies() + withError[reply] = withErrorAt(t, string(v.Body), v.Path) + got := runOddReply(t, ep, withError, want) + if reply != replySigners && isRootField(t, v.Path) { + require.Equal(t, "error", got, "GitHub's reason for a missing root field must reach the user") + return + } + if reply != replySigners && holdsList(t, oddBaselineReplies()[reply], v.Path) { + require.Equal(t, "error", got, "a list GitHub could not give would read as empty") + return + } + require.Equal(t, runOddReply(t, ep, plain, want), got) + }) + } + } + } +} + +// isRootField reports whether path is at most two segments below data, such +// as repository.pullRequest or nodes[0], whose errors are never dropped. +func isRootField(t *testing.T, path string) bool { + t.Helper() + return len(errorPath(t, strings.TrimPrefix(path, "data."))) <= 2 +} + +// TestOddReplies_ErrorsThatAreNotANullFieldFail covers errors the client must +// not drop: one on a field under a null parent (a non-null field's error nulls +// its nearest nullable parent), one with no path, and one on a list item. The +// signer lookup is the exception: any error that comes with data only loses +// signers. +func TestOddReplies_ErrorsThatAreNotANullFieldFail(t *testing.T) { + for _, ep := range oddEntryPoints { + for _, reply := range ep.replies { + baseline := oddBaselineReplies()[reply] + all, err := testHelpers.NullVariants([]byte(baseline)) + require.NoError(t, err) + for _, v := range all { + if v.Path == "." || strings.HasSuffix(v.Path, "[]") { + continue + } + for _, child := range keysAt(t, baseline, v.Path) { + t.Run(ep.name+"/"+reply+"/"+v.Path+"."+child, func(t *testing.T) { + replies := oddBaselineReplies() + replies[reply] = withErrorAt(t, string(v.Body), v.Path+"."+child) + requireFailsUnlessSigners(t, reply, v.Path, runOddReply(t, ep, replies, nil)) + }) + } + } + t.Run(ep.name+"/"+reply+"/no path", func(t *testing.T) { + replies := oddBaselineReplies() + replies[reply] = strings.TrimSuffix(baseline, "}") + `,"errors":[{"message":"Something went wrong"}]}` + requireFailsUnlessSigners(t, reply, "", runOddReply(t, ep, replies, nil)) + }) + } + t.Run(ep.name+"/list item", func(t *testing.T) { + replies := oddBaselineReplies() + replies[replyCommitPage] = withErrorAt(t, strings.Replace(oddCommitPageReply, oddUnsignedCommit, "null", 1), + "data.repository.pullRequest.commits.nodes[0]") + require.Equal(t, "error", runOddReply(t, ep, replies, nil)) + }) + } +} + +// requireFailsUnlessSigners expects an error, except from a signer lookup +// reply that still has data, as it does unless nulledPath is "data". +func requireFailsUnlessSigners(t *testing.T, reply, nulledPath, outcome string) { + t.Helper() + if reply == replySigners && nulledPath != "data" { + require.True(t, strings.HasPrefix(outcome, "ok"), "the signer lookup tolerates errors with data, got %q", outcome) + return + } + require.Equal(t, "error", outcome) +} + +// TestOddReplies_ReviewWithNoAuthor covers a deleted account: GraphQL sends a +// null author, and "ghost" is GitHub's stand-in login for one. Policies read +// Reviews, so only a change request is kept there without a name. +func TestOddReplies_ReviewWithNoAuthor(t *testing.T) { + for _, author := range []string{"null", `{"login":"ghost"}`} { + for _, ep := range oddEntryPoints { + t.Run(ep.name+"/approval/"+author, func(t *testing.T) { + replies := oddBaselineReplies() + noAuthor := strings.Replace(oddApproval, `{"login":"grace"}`, author, 1) + for _, name := range []string{replyByNumber, replyForCommit} { + replies[name] = strings.Replace(replies[name], oddApproval, noAuthor, 1) + } + e := runReviewReplies(t, ep, replies) + require.Empty(t, e.Approvers) + require.Len(t, e.Reviews, 1) + require.Equal(t, types.ReviewChangesRequested, e.Reviews[0].State) + }) + t.Run(ep.name+"/change request/"+author, func(t *testing.T) { + replies := oddBaselineReplies() + replies[replyReviewPage] = strings.Replace(replies[replyReviewPage], `{"login":"linus"}`, author, 1) + e := runReviewReplies(t, ep, replies) + require.Len(t, e.Approvers, 1) + require.Len(t, e.Reviews, 2) + require.Equal(t, "", e.Reviews[1].Username) + require.Equal(t, types.ReviewChangesRequested, e.Reviews[1].State) + }) + } + } +} + +func runReviewReplies(t *testing.T, ep oddEntryPoint, replies map[string]string) *types.PREvidence { + t.Helper() + ts := newOddReplyServer(t, replies) + e, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", + Repository: "test-repo", Sleep: func(time.Duration) {}}) + require.NoError(t, err) + return e +} + +// isApprovalAuthor reports whether path is the author of an approving review +// in the named baseline reply. +func isApprovalAuthor(t *testing.T, reply, path string) bool { + t.Helper() + parent, field, _ := cutLast(path) + if field != "author" || !strings.Contains(parent, ".reviews.nodes[") { + return false + } + review, _ := valueAt(t, oddBaselineReplies()[reply], parent).(map[string]any) + return review["state"] == types.ReviewApproved +} + +// holdsList reports whether path in doc is a list or a connection (an object +// holding a nodes list). +func holdsList(t *testing.T, doc, path string) bool { + t.Helper() + switch v := valueAt(t, doc, path).(type) { + case []any: + return true + case map[string]any: + _, ok := v["nodes"] + return ok + } + return false +} + +// cutLast splits "a.b.c" into "a.b" and "c". +func cutLast(path string) (string, string, bool) { + i := strings.LastIndex(path, ".") + if i < 0 { + return "", path, false + } + return path[:i], path[i+1:], true +} + +// valueAt returns the value at path in doc. +func valueAt(t *testing.T, doc, path string) any { + t.Helper() + var cur any + require.NoError(t, json.Unmarshal([]byte(doc), &cur)) + for _, seg := range errorPath(t, path) { + switch s := seg.(type) { + case string: + cur = cur.(map[string]any)[s] + case int: + cur = cur.([]any)[s] + } + } + return cur +} + +// keysAt names the fields of the object at path in doc, if it is one. +func keysAt(t *testing.T, doc, path string) []string { + t.Helper() + obj, _ := valueAt(t, doc, path).(map[string]any) + keys := []string{} + for k := range obj { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + +// withErrorAt adds to body an errors entry whose path names path. +func withErrorAt(t *testing.T, body, path string) string { + t.Helper() + entry, err := json.Marshal(map[string]any{"type": "NOT_FOUND", "message": "Could not resolve", + "path": errorPath(t, strings.TrimPrefix(path, "data."))}) + require.NoError(t, err) + return strings.TrimSuffix(body, "}") + `,"errors":[` + string(entry) + `]}` +} + +// errorPath turns "a.b[2].c" into GraphQL's ["a","b",2,"c"]. A leading +// "data" segment is kept, so callers strip it when building a reply path. +func errorPath(t *testing.T, path string) []any { + t.Helper() + out := []any{} + for _, part := range strings.Split(path, ".") { + name, rest, _ := strings.Cut(part, "[") + out = append(out, name) + for rest != "" { + idx, more, _ := strings.Cut(rest, "]") + i, err := strconv.Atoi(idx) + require.NoError(t, err) + out = append(out, i) + rest = strings.TrimPrefix(more, "[") + } + } + return out +} + +// A null list item's error says what the list belongs to: the commit for its +// pull requests, or the pull request for its commits and reviews. +func TestOddReplies_NullItemErrorNamesItsOwner(t *testing.T) { + for _, ep := range oddEntryPoints { + for _, reply := range ep.replies { + if reply == replySigners { + continue // A null signer node isn't an error. + } + variants, err := testHelpers.NullVariants([]byte(oddBaselineReplies()[reply])) + require.NoError(t, err) + for _, v := range variants { + if !strings.HasSuffix(v.Path, "]") || strings.HasSuffix(v.Path, "[]") { + continue + } + t.Run(ep.name+"/"+reply+"/"+v.Path, func(t *testing.T) { + replies := oddBaselineReplies() + replies[reply] = string(v.Body) + ts := newOddReplyServer(t, replies) + _, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", + Repository: "test-repo", Sleep: func(time.Duration) {}}) + owner := "for test-org/test-repo#7 " + if regexp.MustCompile(`associatedPullRequests\.nodes\[\d+\]$`).MatchString(v.Path) { + owner = "for commit merge-sha " + } + require.ErrorContains(t, err, owner) + }) + } + } + } +} + +// A commit with no author is recorded with an empty author, not " <>". +func TestOddReplies_MissingCommitAuthorIsEmpty(t *testing.T) { + author := `"author":{"name":"Ada","email":"ada@example.com","user":{"login":"ada"}},` + for _, ep := range oddEntryPoints { + t.Run(ep.name, func(t *testing.T) { + replies := oddBaselineReplies() + require.Equal(t, 1, strings.Count(replies[replyCommitPage], author)) + replies[replyCommitPage] = strings.Replace(replies[replyCommitPage], author, `"author":null,`, 1) + ts := newOddReplyServer(t, replies) + evidence, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", + Repository: "test-repo", Sleep: func(time.Duration) {}}) + require.NoError(t, err) + var authors []string + for _, c := range evidence.Commits { + authors = append(authors, c.Author) + } + require.Equal(t, []string{"Ada ", ""}, authors) + }) + t.Run(ep.name+"/name only", func(t *testing.T) { + replies := oddBaselineReplies() + replies[replyCommitPage] = strings.Replace(replies[replyCommitPage], author, `"author":{"name":"Ada","email":null,"user":null},`, 1) + ts := newOddReplyServer(t, replies) + evidence, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", + Repository: "test-repo", Sleep: func(time.Duration) {}}) + require.NoError(t, err) + require.Equal(t, "Ada <>", evidence.Commits[1].Author, "one part is enough to keep the format") + }) + t.Run(ep.name+"/email only", func(t *testing.T) { + replies := oddBaselineReplies() + replies[replyCommitPage] = strings.Replace(replies[replyCommitPage], author, `"author":{"name":null,"email":"ada@example.com","user":null},`, 1) + ts := newOddReplyServer(t, replies) + evidence, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", + Repository: "test-repo", Sleep: func(time.Duration) {}}) + require.NoError(t, err) + require.Equal(t, " ", evidence.Commits[1].Author, "one part is enough to keep the format") + }) + } +} diff --git a/internal/github/pagination.go b/internal/github/pagination.go index cedf3f255..0b2924fb8 100644 --- a/internal/github/pagination.go +++ b/internal/github/pagination.go @@ -20,6 +20,11 @@ const ( restPageSize = 100 ) +// errPullRequestGone is a follow-up page that no longer finds the pull +// request its first page came from. Ending the drain there would drop the +// rest of its commits or reviews without saying so. +var errPullRequestGone = errors.New("the pull request is no longer readable") + // pageInfo is the cursor block shared by every paginated GraphQL connection. type pageInfo struct { HasNextPage graphql.Boolean @@ -142,13 +147,13 @@ func pageVariables(ref prRef, after graphql.String) map[string]any { // caller's first query already returned. Follow-up pages select only commits, // so draining one connection never re-fetches the other. func (c *GithubConfig) allPRCommits(ctx context.Context, run graphqlQueryFunc, ref prRef, - seed []graphqlCommitNode, first pageInfo) ([]graphqlCommitNode, error) { - commits, err := paginate(seed, first, defaultMaxPages, func(after graphql.String) ([]graphqlCommitNode, pageInfo, error) { + seed []*graphqlCommitNode, first pageInfo) ([]graphqlCommitNode, error) { + commits, err := paginate(seed, first, defaultMaxPages, func(after graphql.String) ([]*graphqlCommitNode, pageInfo, error) { var q struct { - Repository struct { - PullRequest struct { + Repository *struct { + PullRequest *struct { Commits struct { - Nodes []graphqlCommitNode + Nodes []*graphqlCommitNode PageInfo pageInfo } `graphql:"commits(first: 100, after: $cursor)"` } `graphql:"pullRequest(number: $prNumber)"` @@ -157,24 +162,27 @@ func (c *GithubConfig) allPRCommits(ctx context.Context, run graphqlQueryFunc, r if err := run(ctx, &q, pageVariables(ref, after)); err != nil { return nil, pageInfo{}, err } + if q.Repository == nil || q.Repository.PullRequest == nil { + return nil, pageInfo{}, errPullRequestGone + } page := q.Repository.PullRequest.Commits return page.Nodes, page.PageInfo, nil }) - if err != nil { - return nil, fmt.Errorf("draining commits for %s: %w", ref, err) + if err == nil { + return nonNull(commits, "commits", ref.String()) } - return commits, nil + return nil, fmt.Errorf("draining commits for %s: %w", ref, err) } // allPRReviews returns every approved review on prNumber, seeded as above. func (c *GithubConfig) allPRReviews(ctx context.Context, run graphqlQueryFunc, ref prRef, - seed []graphqlReviewNode, first pageInfo) ([]graphqlReviewNode, error) { - reviews, err := paginate(seed, first, defaultMaxPages, func(after graphql.String) ([]graphqlReviewNode, pageInfo, error) { + seed []*graphqlReviewNode, first pageInfo) ([]graphqlReviewNode, error) { + reviews, err := paginate(seed, first, defaultMaxPages, func(after graphql.String) ([]*graphqlReviewNode, pageInfo, error) { var q struct { - Repository struct { - PullRequest struct { + Repository *struct { + PullRequest *struct { Reviews struct { - Nodes []graphqlReviewNode + Nodes []*graphqlReviewNode PageInfo pageInfo } `graphql:"reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED], after: $cursor)"` } `graphql:"pullRequest(number: $prNumber)"` @@ -183,11 +191,14 @@ func (c *GithubConfig) allPRReviews(ctx context.Context, run graphqlQueryFunc, r if err := run(ctx, &q, pageVariables(ref, after)); err != nil { return nil, pageInfo{}, err } + if q.Repository == nil || q.Repository.PullRequest == nil { + return nil, pageInfo{}, errPullRequestGone + } page := q.Repository.PullRequest.Reviews return page.Nodes, page.PageInfo, nil }) - if err != nil { - return nil, fmt.Errorf("draining reviews for %s: %w", ref, err) + if err == nil { + return nonNull(reviews, "reviews", ref.String()) } - return reviews, nil + return nil, fmt.Errorf("draining reviews for %s: %w", ref, err) } diff --git a/internal/github/pr_pagination_test.go b/internal/github/pr_pagination_test.go index 2fbde1c56..1256f7495 100644 --- a/internal/github/pr_pagination_test.go +++ b/internal/github/pr_pagination_test.go @@ -566,17 +566,58 @@ func TestPREvidenceByPRNumber_LooksUpSignersInBatchesOf100(t *testing.T) { require.Equal(t, "grace", evidence.Commits[100].SignerUsername) } -func TestSignerLogins_LogsAnIgnoredErrorWhenDebugging(t *testing.T) { +func TestNullFieldErrorTransport_LogsADroppedErrorWhenDebugging(t *testing.T) { ts := newGraphQLTestServer(t, signersResponse(signedBy{sha: "sha1"})) - config := newPaginationConfig(ts) var debug strings.Builder - node := graphqlCommitNode{} - node.Commit.ID = "node-sha1" - node.Commit.Signature = &graphqlSignature{} - - _, err := config.signerLogins(context.Background(), graphql.NewClient(ts.URL+"/api/graphql", nil).Query, - []graphqlCommitNode{node}, &debug) + client := graphql.NewClient(ts.URL+"/api/graphql", &http.Client{ + Transport: &nullFieldErrorTransport{base: http.DefaultTransport, debug: &debug}, + }) + + var q struct { + Nodes []struct { + Oid graphql.String + Signature struct { + Signer *struct{ Login graphql.String } + } + } `graphql:"nodes(ids: $ids)"` + } + err := client.Query(context.Background(), &q, map[string]any{"ids": []graphql.ID{"node-sha1"}}) require.NoError(t, err) require.Contains(t, debug.String(), "[debug-github]") require.Contains(t, debug.String(), "Not Found") } + +// GitHub nulls a node it cannot return by id; that commit only loses its signer. +func TestPREvidenceByPRNumber_ToleratesANullSignerNode(t *testing.T) { + ts := newGraphQLTestServer(t, + byPRNumberResponse(connectionJSON([]string{signedCommitNodeJSON("sha1")}, ""), connectionJSON(nil, "")), + `{"data":{"nodes":[null]},"errors":[{"type":"NOT_FOUND","path":["nodes",0],"message":"Not Found"}]}`, + ) + + evidence, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) + require.NoError(t, err) + require.Equal(t, "", evidence.Commits[0].SignerUsername) + require.Len(t, ts.bodies, 2) +} + +// GitHub reports a SAML-protected repo as a null repository with a FORBIDDEN +// error; its message tells the user what to fix. +func TestPREvidence_KeepsGitHubsReasonForAMissingRepository(t *testing.T) { + const saml = "Resource protected by organization SAML enforcement. You must grant your Personal Access token access to this organization." + reply := `{"data":{"repository":null},"errors":[{"type":"FORBIDDEN","path":["repository"],"message":"` + saml + `"}]}` + for name, run := range map[string]func(*GithubConfig) error{ + "PREvidenceByPRNumber": func(c *GithubConfig) error { + _, err := c.PREvidenceByPRNumber(1) + return err + }, + "PREvidenceForCommitV2": func(c *GithubConfig) error { + _, err := c.PREvidenceForCommitV2("merge-sha") + return err + }, + } { + t.Run(name, func(t *testing.T) { + ts := newGraphQLTestServer(t, reply, reply, reply, reply) + require.ErrorContains(t, run(newPaginationConfig(ts)), saml) + }) + } +} diff --git a/internal/github/rest_pagination_test.go b/internal/github/rest_pagination_test.go index be944a3e9..aa6a5a988 100644 --- a/internal/github/rest_pagination_test.go +++ b/internal/github/rest_pagination_test.go @@ -77,6 +77,18 @@ func TestGetPullRequestApprovers_FindsApproverOnSecondPage(t *testing.T) { require.Contains(t, ts.queries[0], "per_page=100") } +func TestGetPullRequestApprovers_DeletedAccountIsNotCounted(t *testing.T) { + // REST sends a deleted reviewer as user null, and "ghost" elsewhere. + ts := newRestTestServer(t, "/api/v3/repos/test-org/test-repo/pulls/5/reviews", + `[{"state":"APPROVED","user":null}]`, + `[{"state":"APPROVED","user":{"login":"ghost"}},{"state":"APPROVED","user":{"login":"ada"}}]`, + ) + + approvers, err := newRestConfig(ts.URL).GetPullRequestApprovers(5) + require.NoError(t, err) + require.Equal(t, []string{"ada"}, approvers) +} + func TestPullRequestsForCommit_ReturnsErrorFromSecondPage(t *testing.T) { calls := 0 ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {