Skip to content

fix: record PR evidence when a commit is signed by a GitHub App #1275

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
FayeSGW merged 4 commits into from
Oct 20, 2026
Merged

fix: record PR evidence when a commit is signed by a GitHub App #1275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
939db4e
fix: record PR evidence when a commit is signed by a GitHub App (#1274)
dangrondahl Oct 19, 2026
7a6db5b
fix: retry the commit signer lookup when no data comes back
dangrondahl Oct 19, 2026
429d22a
docs: tighten comments on the commit signer lookup
dangrondahl Oct 19, 2026
a31ed99
test: cover a GitHub-signed App commit whose signer cannot be resolved
dangrondahl Oct 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions internal/github/build_pr_evidence_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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,
Expand All @@ -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)
Expand All @@ -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,
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand All @@ -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)

Expand All @@ -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")
Expand All @@ -291,15 +291,15 @@ 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",
"0e723254516c841126e81f76100be57258ff1386",
"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)
Expand All @@ -314,15 +314,15 @@ 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",
"0e723254516c841126e81f76100be57258ff1386",
"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)
Expand All @@ -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)
Expand Down
90 changes: 80 additions & 10 deletions internal/github/github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"net/http"
"net/url"
"os"
"slices"
"strings"
"time"

Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -357,17 +423,13 @@ 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)
p := bool(sig.WasSignedByGitHub)
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),
Expand All @@ -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,
})
}
Expand Down Expand Up @@ -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,
)
}

Expand Down Expand Up @@ -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
Expand Down
30 changes: 17 additions & 13 deletions internal/github/pagination.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading