diff --git a/internal/github/build_pr_evidence_test.go b/internal/github/build_pr_evidence_test.go index 9f816af45..f3caaff66 100644 --- a/internal/github/build_pr_evidence_test.go +++ b/internal/github/build_pr_evidence_test.go @@ -44,7 +44,7 @@ func TestBuildPREvidence_RecordsAuthorNotCommitter(t *testing.T) { "main", "", []graphqlCommitNode{node}, - nil, + nil, nil, ) require.NoError(t, err) require.Len(t, evidence.Commits, 1) @@ -75,7 +75,7 @@ func TestBuildPREvidence_UsesAuthoredDate(t *testing.T) { "MERGED", "tooky", "2026-03-01T09:00:00Z", "", "Introduce kosli evaluate", "introduce-kosli-evaluate", "main", "", - []graphqlCommitNode{node}, nil, + []graphqlCommitNode{node}, nil, nil, ) require.NoError(t, err) require.Len(t, evidence.Commits, 1) @@ -102,7 +102,7 @@ func TestBuildPREvidence_FallsBackToCommittedDate(t *testing.T) { "MERGED", "tooky", "2026-03-01T09:00:00Z", "", "title", "branch", "main", "", - []graphqlCommitNode{node}, nil, + []graphqlCommitNode{node}, nil, nil, ) require.NoError(t, err) require.Len(t, evidence.Commits, 1) @@ -121,7 +121,7 @@ func TestBuildPREvidence_RecordsBaseRef(t *testing.T) { "MERGED", "tooky", "2026-03-01T09:00:00Z", "", "title", "feature-branch", "main", "", - nil, nil, + nil, nil, nil, ) require.NoError(t, err) require.Equal(t, "main", evidence.BaseRef, @@ -145,7 +145,7 @@ func TestBuildPREvidence_RecordsCommitSignature(t *testing.T) { "MERGED", "tooky", "2026-03-01T09:00:00Z", "", "title", "feature", "main", "", - []graphqlCommitNode{node}, nil, + []graphqlCommitNode{node}, nil, nil, ) require.NoError(t, err) require.Len(t, evidence.Commits, 1) @@ -172,7 +172,7 @@ func TestBuildPREvidence_EmptyAuthorIsPreserved(t *testing.T) { "fix-branch", "main", "", - nil, nil, + nil, nil, nil, ) require.NoError(t, err) require.Equal(t, "", evidence.Author, @@ -202,7 +202,7 @@ func TestBuildPREvidence_UnsignedCommitHasNoSignatureFields(t *testing.T) { "MERGED", "tooky", "2026-03-01T09:00:00Z", "", "title", "feature", "main", "", - []graphqlCommitNode{node}, nil, + []graphqlCommitNode{node}, nil, nil, ) require.NoError(t, err) require.Len(t, evidence.Commits, 1) @@ -230,7 +230,7 @@ func TestBuildPREvidence_RecordsWhichCommitAnApprovalCovered(t *testing.T) { nil, []graphqlReviewNode{ reviewNode("grace", "APPROVED", "2026-03-01T13:00:00Z", "1111111111111111111111111111111111111111"), reviewNode("ada", "APPROVED", "2026-03-01T13:00:00Z", ""), - }, + }, nil, ) require.NoError(t, err) @@ -257,7 +257,7 @@ func TestBuildPREvidence_RecordsEveryReviewAndEachApprovalOnce(t *testing.T) { reviewNode("ada", "COMMENTED", "2026-03-01T14:30:00Z", c1), reviewNode("ada", "DISMISSED", "2026-03-01T14:45:00Z", c1), reviewNode("grace", "APPROVED", "2026-03-01T15:00:00Z", c1), - }, + }, nil, ) require.NoError(t, err) @@ -279,7 +279,7 @@ func TestBuildPREvidence_RecordsThatAPRHasNoReviews(t *testing.T) { "0e723254516c841126e81f76100be57258ff1386", "MERGED", "tooky", "2026-03-01T09:00:00Z", "", "title", "feature", "main", "", - nil, nil, + nil, nil, nil, ) require.NoError(t, err) require.NotNil(t, evidence.Reviews, "no reviews is a fact, not a gap") @@ -291,8 +291,7 @@ func TestBuildPREvidence_RecordsWhoSignedACommit(t *testing.T) { node.Commit.Oid = "0e723254516c841126e81f76100be57258ff1386" node.Commit.CommittedDate = "2026-03-01T12:00:00Z" node.Commit.Author.User = &struct{ Login graphql.String }{Login: "octocat"} - node.Commit.Signature = &graphqlSignature{IsValid: true, State: "VALID", - Signer: &struct{ Login graphql.String }{Login: "tooky"}} + node.Commit.Signature = &graphqlSignature{IsValid: true, State: "VALID"} evidence, err := buildPREvidence( "https://github.com/kosli-dev/cli/pull/671", @@ -300,6 +299,7 @@ func TestBuildPREvidence_RecordsWhoSignedACommit(t *testing.T) { "MERGED", "tooky", "2026-03-01T09:00:00Z", "", "title", "feature", "main", "", []graphqlCommitNode{node}, nil, + map[string]string{"0e723254516c841126e81f76100be57258ff1386": "tooky"}, ) require.NoError(t, err) require.Len(t, evidence.Commits, 1) @@ -314,8 +314,7 @@ func TestBuildPREvidence_RecordsACommitGitHubSigned(t *testing.T) { node := graphqlCommitNode{} node.Commit.Oid = "0e723254516c841126e81f76100be57258ff1386" node.Commit.CommittedDate = "2026-03-01T12:00:00Z" - node.Commit.Signature = &graphqlSignature{IsValid: true, State: "VALID", WasSignedByGitHub: true, - Signer: &struct{ Login graphql.String }{Login: "web-flow"}} + node.Commit.Signature = &graphqlSignature{IsValid: true, State: "VALID", WasSignedByGitHub: true} evidence, err := buildPREvidence( "https://github.com/kosli-dev/cli/pull/671", @@ -323,6 +322,7 @@ func TestBuildPREvidence_RecordsACommitGitHubSigned(t *testing.T) { "MERGED", "tooky", "2026-03-01T09:00:00Z", "", "title", "feature", "main", "", []graphqlCommitNode{node}, nil, + map[string]string{"0e723254516c841126e81f76100be57258ff1386": "web-flow"}, ) require.NoError(t, err) require.Len(t, evidence.Commits, 1) @@ -342,7 +342,7 @@ func TestBuildPREvidence_RecordsASignatureWithNoSigner(t *testing.T) { "0e723254516c841126e81f76100be57258ff1386", "MERGED", "tooky", "2026-03-01T09:00:00Z", "", "title", "feature", "main", "", - []graphqlCommitNode{node}, nil, + []graphqlCommitNode{node}, nil, nil, ) require.NoError(t, err) require.Len(t, evidence.Commits, 1) diff --git a/internal/github/github.go b/internal/github/github.go index e02e9810e..afd3c5dc0 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -8,6 +8,7 @@ import ( "net/http" "net/url" "os" + "slices" "strings" "time" @@ -262,6 +263,7 @@ func (c *GithubConfig) PREvidenceForCommitHybrid(commit string) ([]*types.PREvid // used in both PREvidenceByPRNumber and PREvidenceForCommitV2. type graphqlCommitNode struct { Commit struct { + ID graphql.String Oid graphql.String MessageHeadline graphql.String CommittedDate graphql.String @@ -281,12 +283,75 @@ type graphqlCommitNode struct { type graphqlSignature struct { IsValid graphql.Boolean State graphql.String - Signer *struct { - Login graphql.String - } + // No Signer: a signer GitHub can't resolve fails the whole query, so + // signerLogins fetches it separately. WasSignedByGitHub graphql.Boolean } +// signerBatchSize is the most IDs GitHub's nodes(ids:) field accepts. +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. +func (c *GithubConfig) signerLogins(ctx context.Context, query graphqlQueryFunc, + nodes []graphqlCommitNode, debug io.Writer) (map[string]string, error) { + ids := []graphql.ID{} + for _, n := range nodes { + if n.Commit.Signature != nil { + ids = append(ids, string(n.Commit.ID)) + } + } + type signerQuery struct { + Nodes []struct { + Commit struct { + Oid graphql.String + Signature *struct { + Signer *struct { + Login graphql.String + } + } + } `graphql:"... on Commit"` + } `graphql:"nodes(ids: $ids)"` + } + 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 { + _, _ = fmt.Fprintf(debug, "[debug-github] ignoring error in commit signer lookup: %v\n", err) + } + return nil + } + return err + }) + if err != nil { + return nil, fmt.Errorf("failed to look up commit signers: %w", err) + } + for _, n := range q.Nodes { + if sig := n.Commit.Signature; sig != nil && sig.Signer != nil { + signers[string(n.Commit.Oid)] = string(sig.Signer.Login) + } + } + } + return signers, nil +} + +// debugOut is where debug output goes, or nil when --debug is off. +func (c *GithubConfig) debugOut() io.Writer { + if c.Debug { + return os.Stderr + } + return nil +} + // graphqlReviewNode is the shared GraphQL node type for reviews on a PR. // PENDING is never queried: it is unsubmitted, so it has no submittedAt. type graphqlReviewNode struct { @@ -308,6 +373,7 @@ func buildPREvidence( url, mergeCommit, state, author, createdAtStr, mergedAtStr, title, headRef, baseRef, headSHA string, commitNodes []graphqlCommitNode, reviewNodes []graphqlReviewNode, + signers map[string]string, ) (*types.PREvidence, error) { createdAt, err := time.Parse(time.RFC3339, createdAtStr) if err != nil { @@ -357,7 +423,6 @@ func buildPREvidence( // invalid signature (verified=false) — so leave the fields nil (server#5892). var verified, signedByPlatform *bool var signatureState *string - signerUsername := "" if sig := n.Commit.Signature; sig != nil { v := bool(sig.IsValid) s := string(sig.State) @@ -365,9 +430,6 @@ func buildPREvidence( verified = &v signatureState = &s signedByPlatform = &p - if sig.Signer != nil { - signerUsername = string(sig.Signer.Login) - } } evidence.Commits = append(evidence.Commits, types.Commit{ SHA: string(n.Commit.Oid), @@ -379,7 +441,7 @@ func buildPREvidence( URL: string(n.Commit.URL), Verified: verified, SignatureState: signatureState, - SignerUsername: signerUsername, + SignerUsername: signers[string(n.Commit.Oid)], SignedByPlatform: signedByPlatform, }) } @@ -477,11 +539,15 @@ 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()) + if err != nil { + return nil, err + } return buildPREvidence( string(pr.URL), mergeCommit, string(pr.State), string(pr.Author.Login), string(pr.CreatedAt), string(pr.MergedAt), string(pr.Title), string(pr.HeadRefName), string(pr.BaseRefName), - string(pr.HeadRefOid), commits, reviews, + string(pr.HeadRefOid), commits, reviews, signers, ) } @@ -582,12 +648,16 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence if err != nil { return pullRequestsEvidence, err } + signers, err := c.signerLogins(ctx, client.Query, commits, c.debugOut()) + if err != nil { + return pullRequestsEvidence, err + } // MergeCommit is set to the queried commit SHA — V2 queries by commit SHA // so the commit is by definition the merge commit. evidence, err := buildPREvidence( string(pr.URL), commit, string(pr.State), string(pr.Author.Login), string(pr.CreatedAt), string(pr.MergedAt), string(pr.Title), string(pr.HeadRefName), string(pr.BaseRefName), - string(pr.HeadRefOid), commits, reviews, + string(pr.HeadRefOid), commits, reviews, signers, ) if err != nil { return pullRequestsEvidence, err diff --git a/internal/github/pagination.go b/internal/github/pagination.go index 9a3725257..cedf3f255 100644 --- a/internal/github/pagination.go +++ b/internal/github/pagination.go @@ -65,22 +65,26 @@ type graphqlQueryFunc func(ctx context.Context, q any, variables map[string]any) // commit list (#1082). func (c *GithubConfig) queryWithRetry(client *graphql.Client) graphqlQueryFunc { return func(ctx context.Context, q any, variables map[string]any) error { - var err error - for _, delay := range []time.Duration{0, 10 * time.Second, 20 * time.Second, 30 * time.Second} { - if delay > 0 { - // A wait only happens after a failed attempt, so err holds the - // reason every attempt failed. Join keeps it alongside the - // cancellation, so neither side loses its errors.Is. - if waitErr := c.wait(ctx, delay); waitErr != nil { - return errors.Join(err, waitErr) - } - } - if err = client.Query(ctx, q, variables); err == nil { - return nil + return c.retry(ctx, func() error { return client.Query(ctx, q, variables) }) + } +} + +func (c *GithubConfig) retry(ctx context.Context, attempt func() error) error { + var err error + for _, delay := range []time.Duration{0, 10 * time.Second, 20 * time.Second, 30 * time.Second} { + if delay > 0 { + // A wait only happens after a failed attempt, so err holds the + // reason every attempt failed. Join keeps it alongside the + // cancellation, so neither side loses its errors.Is. + if waitErr := c.wait(ctx, delay); waitErr != nil { + return errors.Join(err, waitErr) } } - return err + if err = attempt(); err == nil { + return nil + } } + return err } // wait pauses between retries. It honours ctx cancellation so a caller with a diff --git a/internal/github/pr_pagination_test.go b/internal/github/pr_pagination_test.go index 550603dac..2fbde1c56 100644 --- a/internal/github/pr_pagination_test.go +++ b/internal/github/pr_pagination_test.go @@ -1,6 +1,7 @@ package github import ( + "context" "errors" "fmt" "io" @@ -11,6 +12,7 @@ import ( "time" "github.com/kosli-dev/cli/internal/types" + "github.com/shurcooL/graphql" "github.com/stretchr/testify/require" ) @@ -402,3 +404,179 @@ func TestReviewQueriesAskForEveryStateButPending(t *testing.T) { require.Contains(t, body, "states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED]") } } + +// signedCommitNodeJSON is a commit node with a valid signature and a node ID, +// which the signer lookup is keyed on. +func signedCommitNodeJSON(sha string) string { + return strings.Replace(commitNodeJSON(sha), `"signature":null`, + fmt.Sprintf(`"id":"node-%s","signature":{"isValid":true,"state":"VALID","wasSignedByGitHub":false}`, sha), 1) +} + +// signedBy is one node of a signer lookup reply. An empty login is a signer +// GitHub could not resolve to a user, which it reports as a field error. +type signedBy struct{ sha, login string } + +// signersResponse answers the signer lookup, with nodes in the order given. +func signersResponse(signers ...signedBy) string { + nodes := []string{} + errs := []string{} + for i, s := range signers { + signer := "null" + if s.login != "" { + signer = fmt.Sprintf(`{"login":%q}`, s.login) + } else { + errs = append(errs, fmt.Sprintf(`{"type":"NOT_FOUND","path":["nodes",%d,"signature","signer"],`+ + `"message":"Not Found"}`, i)) + } + nodes = append(nodes, fmt.Sprintf(`{"oid":%q,"signature":{"signer":%s}}`, s.sha, signer)) + } + if len(errs) == 0 { + return fmt.Sprintf(`{"data":{"nodes":[%s]}}`, strings.Join(nodes, ",")) + } + return fmt.Sprintf(`{"data":{"nodes":[%s]},"errors":[%s]}`, strings.Join(nodes, ","), strings.Join(errs, ",")) +} + +func TestPREvidenceByPRNumber_RecordsACommitSignedByAnApp(t *testing.T) { + ts := newGraphQLTestServer(t, + byPRNumberResponse(connectionJSON([]string{signedCommitNodeJSON("sha1")}, ""), connectionJSON(nil, "")), + signersResponse(signedBy{sha: "sha1"}), + ) + + evidence, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) + require.NoError(t, err) + require.Len(t, evidence.Commits, 1) + c := evidence.Commits[0] + require.Equal(t, "", c.SignerUsername) + require.NotNil(t, c.Verified) + require.True(t, *c.Verified) + require.Equal(t, "VALID", *c.SignatureState) + require.NotNil(t, c.SignedByPlatform) +} + +func TestPREvidenceByPRNumber_RecordsWhoSignedEachCommit(t *testing.T) { + ts := newGraphQLTestServer(t, + byPRNumberResponse(connectionJSON([]string{signedCommitNodeJSON("sha1"), signedCommitNodeJSON("sha2")}, ""), + connectionJSON(nil, "")), + signersResponse(signedBy{"sha1", "tooky"}, signedBy{sha: "sha2"}), + ) + + evidence, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) + require.NoError(t, err) + require.Len(t, evidence.Commits, 2) + require.Equal(t, "tooky", evidence.Commits[0].SignerUsername) + require.Equal(t, "", evidence.Commits[1].SignerUsername) + require.Contains(t, ts.bodies[1], `query($ids:[ID!]!){nodes(ids: $ids)`) + require.Contains(t, ts.bodies[1], `node-sha1`) + require.Contains(t, ts.bodies[1], `node-sha2`) +} + +// GitHub can sign an App's commit with its own key and still fail to resolve +// the signer, so the commit must keep signed_by_platform. +func TestPREvidenceForCommitV2_RecordsACommitSignedByAnApp(t *testing.T) { + commit := strings.Replace(signedCommitNodeJSON("sha1"), + `"wasSignedByGitHub":false`, `"wasSignedByGitHub":true`, 1) + ts := newGraphQLTestServer(t, + forCommitResponse(v2PRNodeJSON(7, connectionJSON([]string{commit}, ""), connectionJSON(nil, ""))), + signersResponse(signedBy{sha: "sha1"}), + ) + + prs, err := newPaginationConfig(ts).PREvidenceForCommitV2("merge-sha") + require.NoError(t, err) + require.Len(t, prs, 1) + require.Len(t, prs[0].Commits, 1) + c := prs[0].Commits[0] + require.Equal(t, "", c.SignerUsername) + require.True(t, *c.Verified) + require.Equal(t, "VALID", *c.SignatureState) + require.True(t, *c.SignedByPlatform) +} + +// The signer is the one field GitHub can fail to resolve, so it must stay out +// of the queries whose failure would lose the whole PR. +func TestCommitQueriesDoNotAskForTheSigner(t *testing.T) { + ts := newGraphQLTestServer(t, + byPRNumberResponse(connectionJSON([]string{commitNodeJSON("sha1")}, "c1"), connectionJSON(nil, "")), + commitsPageResponse([]string{commitNodeJSON("sha2")}, ""), + ) + _, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) + require.NoError(t, err) + require.Len(t, ts.bodies, 2) + for _, body := range ts.bodies { + require.NotContains(t, body, "signer") + } +} + +func TestPREvidenceByPRNumber_FailsWhenTheSignerLookupNeverReturnsData(t *testing.T) { + noData := `{"data":null,"errors":[{"message":"Something went wrong"}]}` + ts := newGraphQLTestServer(t, + byPRNumberResponse(connectionJSON([]string{signedCommitNodeJSON("sha1")}, ""), connectionJSON(nil, "")), + noData, noData, noData, noData, + ) + + _, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) + require.ErrorContains(t, err, "Something went wrong") + require.Len(t, ts.bodies, 5) +} + +func TestPREvidenceByPRNumber_RetriesTheSignerLookup(t *testing.T) { + ts := newGraphQLTestServer(t, + byPRNumberResponse(connectionJSON([]string{signedCommitNodeJSON("sha1")}, ""), connectionJSON(nil, "")), + "500", + signersResponse(signedBy{"sha1", "tooky"}), + ) + + evidence, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) + require.NoError(t, err) + require.Equal(t, "tooky", evidence.Commits[0].SignerUsername) +} + +// A field error is not transient, so it must not run down the retry ladder. +func TestPREvidenceByPRNumber_DoesNotRetryASignerFieldError(t *testing.T) { + ts := newGraphQLTestServer(t, + byPRNumberResponse(connectionJSON([]string{signedCommitNodeJSON("sha1")}, ""), connectionJSON(nil, "")), + signersResponse(signedBy{sha: "sha1"}), + ) + + _, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) + require.NoError(t, err) + require.Len(t, ts.bodies, 2) +} + +func TestPREvidenceByPRNumber_LooksUpSignersInBatchesOf100(t *testing.T) { + nodes := []string{} + first := []signedBy{} + for i := range 101 { + sha := fmt.Sprintf("sha%d", i) + nodes = append(nodes, signedCommitNodeJSON(sha)) + if i < 100 { + first = append(first, signedBy{sha, "tooky"}) + } + } + ts := newGraphQLTestServer(t, + byPRNumberResponse(connectionJSON(nodes, ""), connectionJSON(nil, "")), + signersResponse(first...), + signersResponse(signedBy{"sha100", "grace"}), + ) + + evidence, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) + require.NoError(t, err) + require.Len(t, ts.bodies, 3) + require.NotContains(t, ts.bodies[1], `"node-sha100"`) + require.Contains(t, ts.bodies[2], `"node-sha100"`) + require.Equal(t, "grace", evidence.Commits[100].SignerUsername) +} + +func TestSignerLogins_LogsAnIgnoredErrorWhenDebugging(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) + require.NoError(t, err) + require.Contains(t, debug.String(), "[debug-github]") + require.Contains(t, debug.String(), "Not Found") +}