From 939db4e23949b74e8cbf277eb41a1ee49e3bddfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20Gr=C3=B8ndahl?= Date: Mon, 19 Oct 2026 15:35:48 +0200 Subject: [PATCH 1/4] fix: record PR evidence when a commit is signed by a GitHub App (#1274) GitHub types a commit signature's signer as User. For a commit signed by a GitHub App such as Renovate it returns a null signer plus a field error, and shurcooL/graphql turns any error into a failed query, so the whole PR attestation failed. Signers are now looked up in a separate nodes(ids:) query. The client decodes partial data before returning the error, so a field error that came with data is ignored and that commit keeps verified, signature_state and signed_by_platform with an empty signer_username. The lookup is not retried, since a bot signature errors on every try. --- internal/github/build_pr_evidence_test.go | 30 +++---- internal/github/github.go | 67 +++++++++++--- internal/github/pr_pagination_test.go | 103 ++++++++++++++++++++++ 3 files changed, 175 insertions(+), 25 deletions(-) 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..2a4ebe661 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,51 @@ type graphqlCommitNode struct { type graphqlSignature struct { IsValid graphql.Boolean State graphql.String - Signer *struct { - Login graphql.String - } + // Signer is left out: signerLogins looks it up on its own. 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 the login of the user who +// signed it. GitHub types signer as User, so it answers a commit signed by a +// GitHub App (e.g. Renovate) with a null signer and a field error. The +// client still decodes the rest of the response before returning that +// error, so an error that came with data is a field error and is ignored. +func signerLogins(ctx context.Context, query graphqlQueryFunc, nodes []graphqlCommitNode) (map[string]string, error) { + ids := []graphql.ID{} + for _, n := range nodes { + if n.Commit.Signature != nil { + ids = append(ids, string(n.Commit.ID)) + } + } + signers := map[string]string{} + for batch := range slices.Chunk(ids, signerBatchSize) { + var q struct { + Nodes []struct { + Commit struct { + Oid graphql.String + Signature *struct { + Signer *struct { + Login graphql.String + } + } + } `graphql:"... on Commit"` + } `graphql:"nodes(ids: $ids)"` + } + if err := query(ctx, &q, map[string]any{"ids": batch}); err != nil && len(q.Nodes) == 0 { + 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 +} + // 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 +349,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 +399,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 +406,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 +417,7 @@ func buildPREvidence( URL: string(n.Commit.URL), Verified: verified, SignatureState: signatureState, - SignerUsername: signerUsername, + SignerUsername: signers[string(n.Commit.Oid)], SignedByPlatform: signedByPlatform, }) } @@ -477,11 +515,16 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er if err != nil { return nil, err } + // Not retried: a commit signed by a GitHub App errors on every attempt. + signers, err := signerLogins(ctx, client.Query, commits) + 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 +625,16 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence if err != nil { return pullRequestsEvidence, err } + signers, err := signerLogins(ctx, client.Query, commits) + 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/pr_pagination_test.go b/internal/github/pr_pagination_test.go index 550603dac..c7d765cfd 100644 --- a/internal/github/pr_pagination_test.go +++ b/internal/github/pr_pagination_test.go @@ -402,3 +402,106 @@ 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) +} + +// signersResponse answers the signer lookup. A nil login is a signer GitHub +// could not resolve to a user, which it reports as a field error. +func signersResponse(logins map[string]*string) string { + nodes := []string{} + errs := []string{} + for sha, login := range logins { + signer := "null" + if login != nil { + signer = fmt.Sprintf(`{"login":%q}`, *login) + } else { + errs = append(errs, `{"type":"NOT_FOUND","path":["nodes",0,"signature","signer"],`+ + `"message":"Could not resolve to a User with the login of 'renovate[bot]'."}`) + } + nodes = append(nodes, fmt.Sprintf(`{"oid":%q,"signature":{"signer":%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 login(s string) *string { return &s } + +func TestPREvidenceByPRNumber_RecordsACommitSignedByAnApp(t *testing.T) { + ts := newGraphQLTestServer(t, + byPRNumberResponse(connectionJSON([]string{signedCommitNodeJSON("sha1")}, ""), connectionJSON(nil, "")), + signersResponse(map[string]*string{"sha1": nil}), + ) + + 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(map[string]*string{"sha1": login("tooky"), "sha2": nil}), + ) + + 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`) +} + +func TestPREvidenceForCommitV2_RecordsACommitSignedByAnApp(t *testing.T) { + ts := newGraphQLTestServer(t, + forCommitResponse(v2PRNodeJSON(7, connectionJSON([]string{signedCommitNodeJSON("sha1")}, ""), connectionJSON(nil, ""))), + signersResponse(map[string]*string{"sha1": nil}), + ) + + prs, err := newPaginationConfig(ts).PREvidenceForCommitV2("merge-sha") + require.NoError(t, err) + require.Len(t, prs, 1) + require.Len(t, prs[0].Commits, 1) + require.Equal(t, "", prs[0].Commits[0].SignerUsername) + require.True(t, *prs[0].Commits[0].Verified) +} + +// 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_FailsWhenTheSignerLookupReturnsNoData(t *testing.T) { + ts := newGraphQLTestServer(t, + byPRNumberResponse(connectionJSON([]string{signedCommitNodeJSON("sha1")}, ""), connectionJSON(nil, "")), + `{"data":null,"errors":[{"message":"Something went wrong"}]}`, + ) + + _, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) + require.ErrorContains(t, err, "Something went wrong") +} From 7a6db5bce9c013c423c48370b9b3e1e9a808d01a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20Gr=C3=B8ndahl?= Date: Mon, 19 Oct 2026 16:00:20 +0200 Subject: [PATCH 2/4] fix: retry the commit signer lookup when no data comes back A 502, a timeout or a data:null reply on the signer lookup failed the whole PR attestation, since it was the only request in the flow without retries. It now goes through the same retry ladder, but a field error that comes with data is accepted on the first attempt, so a commit signed by a GitHub App still costs one request. The error accepted with data is logged under --debug: the client drops the error's path, so it cannot be narrowed to the signer field, and a different field error would otherwise look exactly like a bot signer. Also covers batching past 100 signed commits and makes the signer fixture's node order and error paths deterministic. --- internal/github/github.go | 60 +++++++++++----- internal/github/pagination.go | 31 ++++---- internal/github/pr_pagination_test.go | 100 +++++++++++++++++++++----- 3 files changed, 144 insertions(+), 47 deletions(-) diff --git a/internal/github/github.go b/internal/github/github.go index 2a4ebe661..b3aa2a628 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -294,29 +294,46 @@ const signerBatchSize = 100 // signed it. GitHub types signer as User, so it answers a commit signed by a // GitHub App (e.g. Renovate) with a null signer and a field error. The // client still decodes the rest of the response before returning that -// error, so an error that came with data is a field error and is ignored. -func signerLogins(ctx context.Context, query graphqlQueryFunc, nodes []graphqlCommitNode) (map[string]string, error) { +// error, so an error that came with data is accepted. The client's error +// type drops the path, so any field error is accepted, not only the +// signer's; debug, when not nil, is told about each one. +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 struct { - Nodes []struct { - Commit struct { - Oid graphql.String - Signature *struct { - Signer *struct { - Login graphql.String - } - } - } `graphql:"... on Commit"` - } `graphql:"nodes(ids: $ids)"` - } - if err := query(ctx, &q, map[string]any{"ids": batch}); err != nil && len(q.Nodes) == 0 { + 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 { @@ -328,6 +345,14 @@ func signerLogins(ctx context.Context, query graphqlQueryFunc, nodes []graphqlCo 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 { @@ -515,8 +540,7 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er if err != nil { return nil, err } - // Not retried: a commit signed by a GitHub App errors on every attempt. - signers, err := signerLogins(ctx, client.Query, commits) + signers, err := c.signerLogins(ctx, client.Query, commits, c.debugOut()) if err != nil { return nil, err } @@ -625,7 +649,7 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence if err != nil { return pullRequestsEvidence, err } - signers, err := signerLogins(ctx, client.Query, commits) + signers, err := c.signerLogins(ctx, client.Query, commits, c.debugOut()) if err != nil { return pullRequestsEvidence, err } diff --git a/internal/github/pagination.go b/internal/github/pagination.go index 9a3725257..910963ba1 100644 --- a/internal/github/pagination.go +++ b/internal/github/pagination.go @@ -65,22 +65,27 @@ 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) }) + } +} + +// retry runs attempt until it succeeds or the retry ladder runs out. +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 c7d765cfd..cc40336ae 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" ) @@ -410,20 +412,23 @@ func signedCommitNodeJSON(sha string) string { fmt.Sprintf(`"id":"node-%s","signature":{"isValid":true,"state":"VALID","wasSignedByGitHub":false}`, sha), 1) } -// signersResponse answers the signer lookup. A nil login is a signer GitHub -// could not resolve to a user, which it reports as a field error. -func signersResponse(logins map[string]*string) string { +// 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 sha, login := range logins { + for i, s := range signers { signer := "null" - if login != nil { - signer = fmt.Sprintf(`{"login":%q}`, *login) + if s.login != "" { + signer = fmt.Sprintf(`{"login":%q}`, s.login) } else { - errs = append(errs, `{"type":"NOT_FOUND","path":["nodes",0,"signature","signer"],`+ - `"message":"Could not resolve to a User with the login of 'renovate[bot]'."}`) + 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}}`, sha, signer)) + 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, ",")) @@ -431,12 +436,10 @@ func signersResponse(logins map[string]*string) string { return fmt.Sprintf(`{"data":{"nodes":[%s]},"errors":[%s]}`, strings.Join(nodes, ","), strings.Join(errs, ",")) } -func login(s string) *string { return &s } - func TestPREvidenceByPRNumber_RecordsACommitSignedByAnApp(t *testing.T) { ts := newGraphQLTestServer(t, byPRNumberResponse(connectionJSON([]string{signedCommitNodeJSON("sha1")}, ""), connectionJSON(nil, "")), - signersResponse(map[string]*string{"sha1": nil}), + signersResponse(signedBy{sha: "sha1"}), ) evidence, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) @@ -454,7 +457,7 @@ func TestPREvidenceByPRNumber_RecordsWhoSignedEachCommit(t *testing.T) { ts := newGraphQLTestServer(t, byPRNumberResponse(connectionJSON([]string{signedCommitNodeJSON("sha1"), signedCommitNodeJSON("sha2")}, ""), connectionJSON(nil, "")), - signersResponse(map[string]*string{"sha1": login("tooky"), "sha2": nil}), + signersResponse(signedBy{"sha1", "tooky"}, signedBy{sha: "sha2"}), ) evidence, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) @@ -470,7 +473,7 @@ func TestPREvidenceByPRNumber_RecordsWhoSignedEachCommit(t *testing.T) { func TestPREvidenceForCommitV2_RecordsACommitSignedByAnApp(t *testing.T) { ts := newGraphQLTestServer(t, forCommitResponse(v2PRNodeJSON(7, connectionJSON([]string{signedCommitNodeJSON("sha1")}, ""), connectionJSON(nil, ""))), - signersResponse(map[string]*string{"sha1": nil}), + signersResponse(signedBy{sha: "sha1"}), ) prs, err := newPaginationConfig(ts).PREvidenceForCommitV2("merge-sha") @@ -496,12 +499,77 @@ func TestCommitQueriesDoNotAskForTheSigner(t *testing.T) { } } -func TestPREvidenceByPRNumber_FailsWhenTheSignerLookupReturnsNoData(t *testing.T) { +func TestPREvidenceByPRNumber_FailsWhenTheSignerLookupNeverReturnsData(t *testing.T) { + noData := `{"data":null,"errors":[{"message":"Something went wrong"}]}` ts := newGraphQLTestServer(t, byPRNumberResponse(connectionJSON([]string{signedCommitNodeJSON("sha1")}, ""), connectionJSON(nil, "")), - `{"data":null,"errors":[{"message":"Something went wrong"}]}`, + 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") } From 429d22a95d5611d16e96a170286ee2f06e232d21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20Gr=C3=B8ndahl?= Date: Mon, 19 Oct 2026 16:25:55 +0200 Subject: [PATCH 3/4] docs: tighten comments on the commit signer lookup --- internal/github/github.go | 15 +++++++-------- internal/github/pagination.go | 1 - 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/internal/github/github.go b/internal/github/github.go index b3aa2a628..afd3c5dc0 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -283,20 +283,19 @@ type graphqlCommitNode struct { type graphqlSignature struct { IsValid graphql.Boolean State graphql.String - // Signer is left out: signerLogins looks it up on its own. + // 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 the login of the user who -// signed it. GitHub types signer as User, so it answers a commit signed by a -// GitHub App (e.g. Renovate) with a null signer and a field error. The -// client still decodes the rest of the response before returning that -// error, so an error that came with data is accepted. The client's error -// type drops the path, so any field error is accepted, not only the -// signer's; debug, when not nil, is told about each one. +// 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{} diff --git a/internal/github/pagination.go b/internal/github/pagination.go index 910963ba1..cedf3f255 100644 --- a/internal/github/pagination.go +++ b/internal/github/pagination.go @@ -69,7 +69,6 @@ func (c *GithubConfig) queryWithRetry(client *graphql.Client) graphqlQueryFunc { } } -// retry runs attempt until it succeeds or the retry ladder runs out. 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} { From a31ed9904338141624fc24f2c66bda0df3bb1956 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20Gr=C3=B8ndahl?= Date: Tue, 20 Oct 2026 11:34:18 +0200 Subject: [PATCH 4/4] test: cover a GitHub-signed App commit whose signer cannot be resolved The reported failing commit was signed with GitHub's own key (wasSignedByGitHub: true) and still came back with a null signer and a NOT_FOUND error, so the V2 test now uses that shape and checks the commit keeps verified, signature_state and signed_by_platform. --- internal/github/pr_pagination_test.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/internal/github/pr_pagination_test.go b/internal/github/pr_pagination_test.go index cc40336ae..2fbde1c56 100644 --- a/internal/github/pr_pagination_test.go +++ b/internal/github/pr_pagination_test.go @@ -470,9 +470,13 @@ func TestPREvidenceByPRNumber_RecordsWhoSignedEachCommit(t *testing.T) { 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{signedCommitNodeJSON("sha1")}, ""), connectionJSON(nil, ""))), + forCommitResponse(v2PRNodeJSON(7, connectionJSON([]string{commit}, ""), connectionJSON(nil, ""))), signersResponse(signedBy{sha: "sha1"}), ) @@ -480,8 +484,11 @@ func TestPREvidenceForCommitV2_RecordsACommitSignedByAnApp(t *testing.T) { require.NoError(t, err) require.Len(t, prs, 1) require.Len(t, prs[0].Commits, 1) - require.Equal(t, "", prs[0].Commits[0].SignerUsername) - require.True(t, *prs[0].Commits[0].Verified) + 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