From e69c7dabc55c09d1715e9fc1a2222d10b738fd6e Mon Sep 17 00:00:00 2001 From: Simon Castagna Date: Wed, 22 Apr 2026 16:29:17 +0200 Subject: [PATCH 1/8] green: PREvidenceByPRNumber fetches PR data by PR number via GraphQL Adds a new method on GithubConfig that queries GitHub's GraphQL API by PR number rather than commit SHA, avoiding the eventual-consistency issue with associatedPullRequests. FakeGitHubClient gains PRsByNumber to support it in tests. Contract tests cover both the fake and real GitHub. Co-Authored-By: Claude Sonnet 4.6 --- internal/github/fake_github.go | 17 +++ internal/github/github.go | 150 ++++++++++++++++++++++++ internal/github/github_contract_test.go | 60 ++++++++++ internal/testHelpers/testHelpers.go | 6 + 4 files changed, 233 insertions(+) diff --git a/internal/github/fake_github.go b/internal/github/fake_github.go index bac828b0c..ebdae88ef 100644 --- a/internal/github/fake_github.go +++ b/internal/github/fake_github.go @@ -12,10 +12,13 @@ var errInjected = errors.New("injected error") // FakeGitHubClient is an in-memory implementation of types.PRRetriever for // testing. Seed PRsByCommit with the commits and PR evidence you want returned. +// Seed PRsByNumber with PR numbers and their evidence for PREvidenceByPRNumber. // Set Err to simulate a network or API failure. type FakeGitHubClient struct { // PRsByCommit maps a commit SHA to the PR evidence returned for that commit. PRsByCommit map[string][]*types.PREvidence + // PRsByNumber maps a PR number to the PR evidence returned for that number. + PRsByNumber map[int]*types.PREvidence // Err, if set, is returned by all calls regardless of commit. Err error } @@ -38,6 +41,20 @@ func (f *FakeGitHubClient) PREvidenceForCommitV1(commit string) ([]*types.PREvid return prs, nil } +// PREvidenceByPRNumber mirrors the GraphQL API: returns nil with no error for +// PR numbers not present in PRsByNumber (matching the real GitHub behaviour of +// returning null for non-existent pull requests). +func (f *FakeGitHubClient) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, error) { + if f.Err != nil { + return nil, f.Err + } + pr, ok := f.PRsByNumber[prNumber] + if !ok { + return nil, nil + } + return pr, nil +} + // PREvidenceForCommitV2 mirrors the GraphQL API: returns empty with no error // for commits not present in PRsByCommit (matching the real GitHub V2 behaviour // of returning null for unknown commits). diff --git a/internal/github/github.go b/internal/github/github.go index b450cf6c5..c199bc565 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -90,6 +90,156 @@ func ResetGithubRetrieverFunc() { NewGithubRetrieverFunc = defaultNewGithubRetriever } +// PREvidenceByPRNumber fetches full PR evidence for a single PR number via +// GraphQL. Returns nil, nil 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) + if err != nil { + return nil, err + } + httpClient := ghClient.Client() + client := graphql.NewClient(graphqlEndpoint(c.BaseURL), httpClient) + + var query struct { + Repository struct { + PullRequest *struct { + Title graphql.String + State graphql.String + HeadRefName graphql.String + URL graphql.String + CreatedAt graphql.String + MergedAt graphql.String + MergeCommit *struct { + Oid graphql.String + } + Author struct { + Login graphql.String + } + Commits struct { + Nodes []struct { + Commit struct { + Oid graphql.String + MessageHeadline graphql.String + CommittedDate graphql.String + URL graphql.String + Committer struct { + Name graphql.String + Email graphql.String + Date graphql.String + User *struct { + Login graphql.String + } + } + } + } + PageInfo struct { + HasNextPage graphql.Boolean + EndCursor graphql.String + } + } `graphql:"commits(first: 100, after: $commitCursor)"` + Reviews struct { + Nodes []struct { + Author struct { + Login graphql.String + } + State graphql.String + SubmittedAt graphql.String + } + PageInfo struct { + HasNextPage graphql.Boolean + EndCursor graphql.String + } + } `graphql:"reviews(first: 100, states: APPROVED, after: $reviewCursor)"` + } `graphql:"pullRequest(number: $prNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + + variables := map[string]interface{}{ + "owner": graphql.String(c.Org), + "repo": graphql.String(c.Repository), + "prNumber": graphql.Int(prNumber), + "commitCursor": (*graphql.String)(nil), + "reviewCursor": (*graphql.String)(nil), + } + + err = client.Query(ctx, &query, variables) + if err != nil { + return nil, err + } + + pr := query.Repository.PullRequest + if pr == nil { + return nil, nil + } + + createdAt, err := time.Parse(time.RFC3339, string(pr.CreatedAt)) + if err != nil { + return nil, err + } + mergedAt := int64(0) + if pr.MergedAt != "" { + mergedAtTime, err := time.Parse(time.RFC3339, string(pr.MergedAt)) + if err != nil { + return nil, err + } + mergedAt = mergedAtTime.Unix() + } + + mergeCommit := "" + if pr.MergeCommit != nil { + mergeCommit = string(pr.MergeCommit.Oid) + } + + evidence := &types.PREvidence{ + URL: string(pr.URL), + MergeCommit: mergeCommit, + State: string(pr.State), + Author: string(pr.Author.Login), + CreatedAt: createdAt.Unix(), + MergedAt: mergedAt, + Title: string(pr.Title), + HeadRef: string(pr.HeadRefName), + Approvers: []any{}, + Commits: []types.Commit{}, + } + + for _, c := range pr.Commits.Nodes { + timestamp, err := time.Parse(time.RFC3339, string(c.Commit.CommittedDate)) + if err != nil { + return nil, err + } + committerUsername := "" + if c.Commit.Committer.User != nil { + committerUsername = string(c.Commit.Committer.User.Login) + } + evidence.Commits = append(evidence.Commits, types.Commit{ + SHA: string(c.Commit.Oid), + Message: string(c.Commit.MessageHeadline), + Committer: fmt.Sprintf("%s <%s>", string(c.Commit.Committer.Name), string(c.Commit.Committer.Email)), + CommitterUsername: committerUsername, + Timestamp: timestamp.Unix(), + Branch: string(pr.HeadRefName), + URL: string(c.Commit.URL), + }) + } + + for _, r := range pr.Reviews.Nodes { + submittedAt, err := time.Parse(time.RFC3339, string(r.SubmittedAt)) + if err != nil { + return nil, err + } + evidence.Approvers = append(evidence.Approvers, types.PRApprovals{ + Username: string(r.Author.Login), + State: string(r.State), + Timestamp: submittedAt.Unix(), + }) + } + + return evidence, nil +} + func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence, error) { ctx := context.Background() pullRequestsEvidence := []*types.PREvidence{} diff --git a/internal/github/github_contract_test.go b/internal/github/github_contract_test.go index e55921d0c..66dc1bd12 100644 --- a/internal/github/github_contract_test.go +++ b/internal/github/github_contract_test.go @@ -60,6 +60,31 @@ func runGitHubContractTests(t *testing.T, provider types.PRRetriever, commitWith }) } +// prByNumberRetriever is a local interface used to test PREvidenceByPRNumber +// independently of the PRRetriever contract. +type prByNumberRetriever interface { + PREvidenceByPRNumber(int) (*types.PREvidence, error) +} + +func runPRByNumberContractTests(t *testing.T, provider prByNumberRetriever, knownPRNumber int) { + t.Helper() + + t.Run("returns evidence for known PR number", func(t *testing.T) { + pr, err := provider.PREvidenceByPRNumber(knownPRNumber) + require.NoError(t, err) + require.NotNil(t, pr) + require.NotEmpty(t, pr.URL, "URL should be present") + require.NotEmpty(t, pr.State, "State should be present") + require.NotEmpty(t, pr.MergeCommit, "MergeCommit should be present") + }) + + t.Run("returns nil for unknown PR number", func(t *testing.T) { + pr, err := provider.PREvidenceByPRNumber(999999999) + require.NoError(t, err) + require.Nil(t, pr) + }) +} + func TestGitHubContract_Fake(t *testing.T) { commitWithPR := "abc123" commitUnknown := "0000000000000000000000000000000000000000" @@ -109,3 +134,38 @@ func TestGitHubContract_RealGitHub(t *testing.T) { commitUnknown := "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" runGitHubContractTests(t, config, testHelpers.GithubCommitWithPR(), commitUnknown) } + +func TestGitHubContract_Fake_PRByNumber(t *testing.T) { + knownPRNumber := 6 + pr := &types.PREvidence{ + URL: "https://github.com/kosli-dev/cli/pull/6", + State: "MERGED", + MergeCommit: "e21a8afff429e0c87ee523d683f2438113f0a105", + } + client := &FakeGitHubClient{ + PRsByNumber: map[int]*types.PREvidence{ + knownPRNumber: pr, + }, + } + runPRByNumberContractTests(t, client, knownPRNumber) + + t.Run("returns error when Err is injected", func(t *testing.T) { + client.Err = errInjected + defer func() { client.Err = nil }() + _, err := client.PREvidenceByPRNumber(knownPRNumber) + require.Error(t, err) + }) +} + +func TestGitHubContract_RealGitHub_PRByNumber(t *testing.T) { + testHelpers.SkipIfEnvVarUnset(t, []string{"KOSLI_GITHUB_TOKEN"}) + + config := NewGithubConfig( + os.Getenv("KOSLI_GITHUB_TOKEN"), + "", + "kosli-dev", + "cli", + ) + + runPRByNumberContractTests(t, config, testHelpers.GithubPRNumber()) +} diff --git a/internal/testHelpers/testHelpers.go b/internal/testHelpers/testHelpers.go index ef29a3edd..09df22df4 100644 --- a/internal/testHelpers/testHelpers.go +++ b/internal/testHelpers/testHelpers.go @@ -45,6 +45,12 @@ func GithubCommitWithPR() string { return "e21a8afff429e0c87ee523d683f2438113f0a105" } +// GithubPRNumber returns the PR number associated with GithubCommitWithPR in +// kosli-dev/cli. It is stable because merged PRs are never deleted. +func GithubPRNumber() int { + return 6 +} + func CloneGitRepo(url, cloneTo string) (*git.Repository, error) { // the repo worktree filesystem. It has to be osfs so that we can give it a path fs := osfs.New(cloneTo) From 3cc9d9a8d70ae23bfd9d1eea673f4a2ed1322512 Mon Sep 17 00:00:00 2001 From: Simon Castagna Date: Wed, 22 Apr 2026 16:39:33 +0200 Subject: [PATCH 2/8] green: PREvidenceForCommitHybrid fixes GitHub eventual consistency bug Adds PREvidenceForCommitHybrid to the PRRetriever interface and wires it into both call sites (attest pullrequest github, assert pullrequest github). When GitHub's GraphQL associatedPullRequests returns 0 results, the hybrid falls back to REST discovery + per-PR GraphQL so that PRs merged seconds before CI runs are correctly detected. GitLab, Azure, and Bitbucket delegate the method to their existing V2 implementations since they have no eventual consistency issue. Co-Authored-By: Claude Sonnet 4.6 --- cmd/kosli/assertPRGithub.go | 2 +- cmd/kosli/pullrequest.go | 2 +- internal/azure/azure.go | 4 ++ internal/bitbucket/bitbucket.go | 4 ++ internal/github/fake_github.go | 33 +++++++++++++++++ internal/github/github.go | 31 ++++++++++++++++ internal/github/github_contract_test.go | 49 ++++++++++++++++++++++++- internal/gitlab/gitlab.go | 4 ++ internal/types/types.go | 5 +++ 9 files changed, 131 insertions(+), 3 deletions(-) diff --git a/cmd/kosli/assertPRGithub.go b/cmd/kosli/assertPRGithub.go index 05241c89f..8979281ef 100644 --- a/cmd/kosli/assertPRGithub.go +++ b/cmd/kosli/assertPRGithub.go @@ -59,7 +59,7 @@ func newAssertPullRequestGithubCmd(out io.Writer) *cobra.Command { } func (o *assertPullRequestGithubOptions) run(args []string) error { - pullRequestsEvidence, err := o.retriever.PREvidenceForCommitV2(o.commit) + pullRequestsEvidence, err := o.retriever.PREvidenceForCommitHybrid(o.commit) if err != nil { return err } diff --git a/cmd/kosli/pullrequest.go b/cmd/kosli/pullrequest.go index 8766a760f..5f1c329d5 100644 --- a/cmd/kosli/pullrequest.go +++ b/cmd/kosli/pullrequest.go @@ -42,7 +42,7 @@ func (o *attestPROptions) run(args []string) error { o.payload.GitProvider, label = o.getRetriever().ProviderAndLabel() var pullRequestsEvidence []*types.PREvidence - pullRequestsEvidence, err = o.getRetriever().PREvidenceForCommitV2(o.payload.Commit.Sha1) + pullRequestsEvidence, err = o.getRetriever().PREvidenceForCommitHybrid(o.payload.Commit.Sha1) if err != nil { return err } diff --git a/internal/azure/azure.go b/internal/azure/azure.go index 67250c02d..6f6628484 100644 --- a/internal/azure/azure.go +++ b/internal/azure/azure.go @@ -76,6 +76,10 @@ func (c *AzureConfig) PREvidenceForCommitV1(commit string) ([]*types.PREvidence, return pullRequestsEvidence, nil } +func (c *AzureConfig) PREvidenceForCommitHybrid(commit string) ([]*types.PREvidence, error) { + return c.PREvidenceForCommitV2(commit) +} + // This is the new implementation, it will be used for Azure func (c *AzureConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence, error) { pullRequestsEvidence := []*types.PREvidence{} diff --git a/internal/bitbucket/bitbucket.go b/internal/bitbucket/bitbucket.go index 73e2cb7ba..40cb9cc7a 100644 --- a/internal/bitbucket/bitbucket.go +++ b/internal/bitbucket/bitbucket.go @@ -43,6 +43,10 @@ func (c *Config) PREvidenceForCommitV1(commit string) ([]*types.PREvidence, erro return c.getPullRequestsFromBitbucketApi(commit, 1) } +func (c *Config) PREvidenceForCommitHybrid(commit string) ([]*types.PREvidence, error) { + return c.PREvidenceForCommitV2(commit) +} + // This is the new implementation, it will be used for Bitbucket func (c *Config) PREvidenceForCommitV2(commit string) ([]*types.PREvidence, error) { return c.getPullRequestsFromBitbucketApi(commit, 2) diff --git a/internal/github/fake_github.go b/internal/github/fake_github.go index ebdae88ef..962876b1c 100644 --- a/internal/github/fake_github.go +++ b/internal/github/fake_github.go @@ -13,12 +13,16 @@ var errInjected = errors.New("injected error") // FakeGitHubClient is an in-memory implementation of types.PRRetriever for // testing. Seed PRsByCommit with the commits and PR evidence you want returned. // Seed PRsByNumber with PR numbers and their evidence for PREvidenceByPRNumber. +// Seed PRNumbersByCommit + PRsByNumber to exercise the hybrid fallback path +// (V2 empty → V1 discovery → per-PR GraphQL). // Set Err to simulate a network or API failure. type FakeGitHubClient struct { // PRsByCommit maps a commit SHA to the PR evidence returned for that commit. PRsByCommit map[string][]*types.PREvidence // PRsByNumber maps a PR number to the PR evidence returned for that number. PRsByNumber map[int]*types.PREvidence + // PRNumbersByCommit maps a commit SHA to PR numbers for the hybrid fallback path. + PRNumbersByCommit map[string][]int // Err, if set, is returned by all calls regardless of commit. Err error } @@ -41,6 +45,35 @@ func (f *FakeGitHubClient) PREvidenceForCommitV1(commit string) ([]*types.PREvid return prs, nil } +// PREvidenceForCommitHybrid mirrors the hybrid strategy: tries V2 (PRsByCommit) +// first; if empty, falls back through PRNumbersByCommit + PREvidenceByPRNumber. +// Returns empty (no error) when the commit is not found in either map. +func (f *FakeGitHubClient) PREvidenceForCommitHybrid(commit string) ([]*types.PREvidence, error) { + if f.Err != nil { + return nil, f.Err + } + prs, err := f.PREvidenceForCommitV2(commit) + if err != nil { + return nil, err + } + if len(prs) > 0 { + return prs, nil + } + // Fallback: use PRNumbersByCommit for V1-style discovery. + prNumbers := f.PRNumbersByCommit[commit] + var result []*types.PREvidence + for _, n := range prNumbers { + evidence, err := f.PREvidenceByPRNumber(n) + if err != nil { + return nil, err + } + if evidence != nil { + result = append(result, evidence) + } + } + return result, nil +} + // PREvidenceByPRNumber mirrors the GraphQL API: returns nil with no error for // PR numbers not present in PRsByNumber (matching the real GitHub behaviour of // returning null for non-existent pull requests). diff --git a/internal/github/github.go b/internal/github/github.go index c199bc565..affec61d8 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -90,6 +90,37 @@ func ResetGithubRetrieverFunc() { NewGithubRetrieverFunc = defaultNewGithubRetriever } +// PREvidenceForCommitHybrid tries PREvidenceForCommitV2 first. If it returns +// no results it falls back to V1 REST discovery (immediately consistent) + +// PREvidenceByPRNumber for each PR found, preserving all rich V2 fields. +func (c *GithubConfig) PREvidenceForCommitHybrid(commit string) ([]*types.PREvidence, error) { + prs, err := c.PREvidenceForCommitV2(commit) + if err != nil { + return nil, err + } + if len(prs) > 0 { + return prs, nil + } + + // V2 returned nothing — fall back to REST discovery. + restPRs, err := c.PullRequestsForCommit(commit) + if err != nil { + return nil, err + } + + var result []*types.PREvidence + for _, pr := range restPRs { + evidence, err := c.PREvidenceByPRNumber(pr.GetNumber()) + if err != nil { + return nil, err + } + if evidence != nil { + result = append(result, evidence) + } + } + return result, nil +} + // PREvidenceByPRNumber fetches full PR evidence for a single PR number via // GraphQL. Returns nil, nil when the PR does not exist. func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, error) { diff --git a/internal/github/github_contract_test.go b/internal/github/github_contract_test.go index 66dc1bd12..9476ccc86 100644 --- a/internal/github/github_contract_test.go +++ b/internal/github/github_contract_test.go @@ -53,6 +53,15 @@ func runGitHubContractTests(t *testing.T, provider types.PRRetriever, commitWith require.Error(t, err) }) + t.Run("Hybrid returns PRs for commit with PRs", func(t *testing.T) { + prs, err := provider.PREvidenceForCommitHybrid(commitWithPR) + require.NoError(t, err) + require.NotEmpty(t, prs) + require.NotEmpty(t, prs[0].URL, "URL should be present") + require.NotEmpty(t, prs[0].State, "State should be present") + require.NotEmpty(t, prs[0].MergeCommit, "MergeCommit should be present") + }) + t.Run("ProviderAndLabel returns github and pull request", func(t *testing.T) { providerName, label := provider.ProviderAndLabel() require.Equal(t, "github", providerName) @@ -118,6 +127,44 @@ func TestGitHubContract_Fake(t *testing.T) { _, err := client.PREvidenceForCommitV1(commitWithPR) require.Error(t, err) }) + + // Hybrid fallback path: V2 returns empty (PRsByCommit not seeded for this + // commit), so the fake falls back through PRNumbersByCommit + PRsByNumber. + commitFallback := "fallback-commit" + prNumber := testHelpers.GithubPRNumber() + fallbackPR := &types.PREvidence{ + URL: "https://github.com/kosli-dev/cli/pull/6", + State: "MERGED", + MergeCommit: commitFallback, + } + fallbackClient := &FakeGitHubClient{ + PRNumbersByCommit: map[string][]int{ + commitFallback: {prNumber}, + }, + PRsByNumber: map[int]*types.PREvidence{ + prNumber: fallbackPR, + }, + } + + t.Run("Hybrid returns PRs via fallback path when V2 returns empty", func(t *testing.T) { + prs, err := fallbackClient.PREvidenceForCommitHybrid(commitFallback) + require.NoError(t, err) + require.NotEmpty(t, prs) + require.Equal(t, fallbackPR.URL, prs[0].URL) + }) + + t.Run("Hybrid returns empty when commit has no PRs in either path", func(t *testing.T) { + prs, err := fallbackClient.PREvidenceForCommitHybrid("commit-with-no-prs") + require.NoError(t, err) + require.Empty(t, prs) + }) + + t.Run("Hybrid returns error when Err is injected", func(t *testing.T) { + client.Err = errInjected + defer func() { client.Err = nil }() + _, err := client.PREvidenceForCommitHybrid(commitWithPR) + require.Error(t, err) + }) } func TestGitHubContract_RealGitHub(t *testing.T) { @@ -136,7 +183,7 @@ func TestGitHubContract_RealGitHub(t *testing.T) { } func TestGitHubContract_Fake_PRByNumber(t *testing.T) { - knownPRNumber := 6 + knownPRNumber := testHelpers.GithubPRNumber() pr := &types.PREvidence{ URL: "https://github.com/kosli-dev/cli/pull/6", State: "MERGED", diff --git a/internal/gitlab/gitlab.go b/internal/gitlab/gitlab.go index bdb7bae97..a58226763 100644 --- a/internal/gitlab/gitlab.go +++ b/internal/gitlab/gitlab.go @@ -59,6 +59,10 @@ func (c *GitlabConfig) PREvidenceForCommitV1(commit string) ([]*types.PREvidence return pullRequestsEvidence, nil } +func (c *GitlabConfig) PREvidenceForCommitHybrid(commit string) ([]*types.PREvidence, error) { + return c.PREvidenceForCommitV2(commit) +} + // This is the new implementation, it will be used for all VCS providers func (c *GitlabConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence, error) { pullRequestsEvidence := []*types.PREvidence{} diff --git a/internal/types/types.go b/internal/types/types.go index e4d3dd889..cc84f2173 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -32,6 +32,11 @@ type Commit struct { type PRRetriever interface { PREvidenceForCommitV2(string) ([]*PREvidence, error) PREvidenceForCommitV1(string) ([]*PREvidence, error) + // PREvidenceForCommitHybrid tries V2 (GraphQL by commit SHA) first. If it + // returns no results it falls back to V1 REST discovery + per-PR GraphQL so + // that GitHub's eventual consistency on associatedPullRequests never causes + // a false "no PR found". + PREvidenceForCommitHybrid(string) ([]*PREvidence, error) // ProviderAndLabel returns the provider name (e.g. "github") and the label // for a pull request (e.g. "pull request", or "merge request" for GitLab). ProviderAndLabel() (string, string) From 9e6669294ff3eaf633fc149d3f97aa2e99669d5a Mon Sep 17 00:00:00 2001 From: Simon Castagna Date: Wed, 22 Apr 2026 16:46:10 +0200 Subject: [PATCH 3/8] green: retry PREvidenceByPRNumber on GraphQL error with 60s max backoff Wraps the GraphQL query in PREvidenceByPRNumber with a retry loop that sleeps 10s, 20s, then 30s between attempts (60s total max). Any GraphQL error (network, HTTP, or API) triggers a retry. The Sleep field on GithubConfig is injectable for tests so retries complete instantly. Co-Authored-By: Claude Sonnet 4.6 --- internal/github/github.go | 15 ++++++++ internal/github/github_test.go | 62 ++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/internal/github/github.go b/internal/github/github.go index affec61d8..c214c093e 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -21,6 +21,9 @@ type GithubConfig struct { BaseURL string Org string Repository string + // Sleep is called between retries in PREvidenceByPRNumber. Defaults to + // time.Sleep when nil. Override in tests to avoid real delays. + Sleep func(time.Duration) } type GithubFlagsTempValueHolder struct { @@ -195,7 +198,19 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er "reviewCursor": (*graphql.String)(nil), } + sleep := c.Sleep + if sleep == nil { + sleep = time.Sleep + } + retryDelays := []time.Duration{10 * time.Second, 20 * time.Second, 30 * time.Second} err = client.Query(ctx, &query, variables) + for _, delay := range retryDelays { + if err == nil { + break + } + sleep(delay) + err = client.Query(ctx, &query, variables) + } if err != nil { return nil, err } diff --git a/internal/github/github_test.go b/internal/github/github_test.go index 38bf8e0f1..8e9bb9d0c 100644 --- a/internal/github/github_test.go +++ b/internal/github/github_test.go @@ -2,7 +2,11 @@ package github import ( "context" + "fmt" + "net/http" + "net/http/httptest" "testing" + "time" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" @@ -67,3 +71,61 @@ func (suite *GithubTestSuite) TestExtractRepoName() { 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 + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/graphql" { + w.WriteHeader(http.StatusNotFound) + return + } + attempts++ + if attempts <= failCount { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprint(w, graphqlNullPRResponse) + })) + return ts, &attempts +} + +func newRetryConfig(serverURL string, sleepFn func(time.Duration)) *GithubConfig { + return &GithubConfig{ + Token: "fake-token", + BaseURL: serverURL, + Org: "test-org", + Repository: "test-repo", + Sleep: sleepFn, + } +} + +func TestPREvidenceByPRNumber_RetriesOnGraphQLError(t *testing.T) { + ts, attempts := newRetryTestServer(t, 2) + defer ts.Close() + + var sleptDurations []time.Duration + config := newRetryConfig(ts.URL, func(d time.Duration) { sleptDurations = append(sleptDurations, d) }) + + pr, err := config.PREvidenceByPRNumber(1) + require.NoError(t, err) + require.Nil(t, pr) + require.Equal(t, 3, *attempts, "should have retried twice before succeeding") + require.Len(t, sleptDurations, 2, "should have slept between retries") +} + +func TestPREvidenceByPRNumber_ReturnsErrorAfterAllRetriesExhausted(t *testing.T) { + ts, attempts := newRetryTestServer(t, 999) + defer ts.Close() + + config := newRetryConfig(ts.URL, func(time.Duration) {}) + + _, err := config.PREvidenceByPRNumber(1) + require.Error(t, err) + require.Equal(t, 4, *attempts, "should have made 1 initial attempt + 3 retries") +} From 30c218c0ff3a245f54cdc23b9ef576490be5c1e1 Mon Sep 17 00:00:00 2001 From: Simon Castagna Date: Thu, 23 Apr 2026 08:42:54 +0200 Subject: [PATCH 4/8] refactor: extract buildPREvidence to eliminate duplication Defines graphqlCommitNode and graphqlReviewNode as named types shared by both PREvidenceByPRNumber and PREvidenceForCommitV2, and extracts the evidence construction (commit loop, review loop, PREvidence build) into a buildPREvidence helper. Both methods now call the helper instead of duplicating ~50 lines each. Co-Authored-By: Claude Sonnet 4.6 --- internal/github/github.go | 277 ++++++++++++++++---------------------- 1 file changed, 113 insertions(+), 164 deletions(-) diff --git a/internal/github/github.go b/internal/github/github.go index c214c093e..d627c6332 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -124,6 +124,103 @@ func (c *GithubConfig) PREvidenceForCommitHybrid(commit string) ([]*types.PREvid return result, nil } +// graphqlCommitNode is the shared GraphQL node type for commits on a PR, +// used in both PREvidenceByPRNumber and PREvidenceForCommitV2. +type graphqlCommitNode struct { + Commit struct { + Oid graphql.String + MessageHeadline graphql.String + CommittedDate graphql.String + URL graphql.String + Committer struct { + Name graphql.String + Email graphql.String + Date graphql.String + User *struct { + Login graphql.String + } + } + } +} + +// graphqlReviewNode is the shared GraphQL node type for approved reviews on a PR. +type graphqlReviewNode struct { + Author struct { + Login graphql.String + } + State graphql.String + SubmittedAt graphql.String +} + +// buildPREvidence constructs a PREvidence from pre-resolved fields and the +// raw GraphQL commit/review nodes. mergeCommit must be resolved by the caller +// (it differs between commit-SHA queries and PR-number queries). +func buildPREvidence( + url, mergeCommit, state, author, createdAtStr, mergedAtStr, title, headRef string, + commitNodes []graphqlCommitNode, + reviewNodes []graphqlReviewNode, +) (*types.PREvidence, error) { + createdAt, err := time.Parse(time.RFC3339, 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() + } + + evidence := &types.PREvidence{ + URL: url, + MergeCommit: mergeCommit, + State: state, + Author: author, + CreatedAt: createdAt.Unix(), + MergedAt: mergedAt, + Title: title, + HeadRef: headRef, + Approvers: []any{}, + Commits: []types.Commit{}, + } + + for _, n := range commitNodes { + timestamp, err := time.Parse(time.RFC3339, string(n.Commit.CommittedDate)) + if err != nil { + return nil, err + } + committerUsername := "" + if n.Commit.Committer.User != nil { + committerUsername = string(n.Commit.Committer.User.Login) + } + evidence.Commits = append(evidence.Commits, types.Commit{ + SHA: string(n.Commit.Oid), + Message: string(n.Commit.MessageHeadline), + Committer: fmt.Sprintf("%s <%s>", string(n.Commit.Committer.Name), string(n.Commit.Committer.Email)), + CommitterUsername: committerUsername, + Timestamp: timestamp.Unix(), + Branch: headRef, + URL: string(n.Commit.URL), + }) + } + + for _, r := range reviewNodes { + submittedAt, err := time.Parse(time.RFC3339, string(r.SubmittedAt)) + if err != nil { + return nil, err + } + evidence.Approvers = append(evidence.Approvers, types.PRApprovals{ + Username: string(r.Author.Login), + State: string(r.State), + Timestamp: submittedAt.Unix(), + }) + } + + return evidence, nil +} + // PREvidenceByPRNumber fetches full PR evidence for a single PR number via // GraphQL. Returns nil, nil when the PR does not exist. func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, error) { @@ -152,35 +249,14 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er Login graphql.String } Commits struct { - Nodes []struct { - Commit struct { - Oid graphql.String - MessageHeadline graphql.String - CommittedDate graphql.String - URL graphql.String - Committer struct { - Name graphql.String - Email graphql.String - Date graphql.String - User *struct { - Login graphql.String - } - } - } - } + Nodes []graphqlCommitNode PageInfo struct { HasNextPage graphql.Boolean EndCursor graphql.String } } `graphql:"commits(first: 100, after: $commitCursor)"` Reviews struct { - Nodes []struct { - Author struct { - Login graphql.String - } - State graphql.String - SubmittedAt graphql.String - } + Nodes []graphqlReviewNode PageInfo struct { HasNextPage graphql.Boolean EndCursor graphql.String @@ -220,70 +296,16 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er return nil, nil } - createdAt, err := time.Parse(time.RFC3339, string(pr.CreatedAt)) - if err != nil { - return nil, err - } - mergedAt := int64(0) - if pr.MergedAt != "" { - mergedAtTime, err := time.Parse(time.RFC3339, string(pr.MergedAt)) - if err != nil { - return nil, err - } - mergedAt = mergedAtTime.Unix() - } - mergeCommit := "" if pr.MergeCommit != nil { mergeCommit = string(pr.MergeCommit.Oid) } - evidence := &types.PREvidence{ - URL: string(pr.URL), - MergeCommit: mergeCommit, - State: string(pr.State), - Author: string(pr.Author.Login), - CreatedAt: createdAt.Unix(), - MergedAt: mergedAt, - Title: string(pr.Title), - HeadRef: string(pr.HeadRefName), - Approvers: []any{}, - Commits: []types.Commit{}, - } - - for _, c := range pr.Commits.Nodes { - timestamp, err := time.Parse(time.RFC3339, string(c.Commit.CommittedDate)) - if err != nil { - return nil, err - } - committerUsername := "" - if c.Commit.Committer.User != nil { - committerUsername = string(c.Commit.Committer.User.Login) - } - evidence.Commits = append(evidence.Commits, types.Commit{ - SHA: string(c.Commit.Oid), - Message: string(c.Commit.MessageHeadline), - Committer: fmt.Sprintf("%s <%s>", string(c.Commit.Committer.Name), string(c.Commit.Committer.Email)), - CommitterUsername: committerUsername, - Timestamp: timestamp.Unix(), - Branch: string(pr.HeadRefName), - URL: string(c.Commit.URL), - }) - } - - for _, r := range pr.Reviews.Nodes { - submittedAt, err := time.Parse(time.RFC3339, string(r.SubmittedAt)) - if err != nil { - return nil, err - } - evidence.Approvers = append(evidence.Approvers, types.PRApprovals{ - Username: string(r.Author.Login), - State: string(r.State), - Timestamp: submittedAt.Unix(), - }) - } - - return evidence, nil + 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), + pr.Commits.Nodes, pr.Reviews.Nodes, + ) } func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence, error) { @@ -317,22 +339,7 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence } Commits struct { - Nodes []struct { - Commit struct { - Oid graphql.String - MessageHeadline graphql.String - CommittedDate graphql.String - URL graphql.String - Committer struct { - Name graphql.String - Email graphql.String - Date graphql.String - User *struct { - Login graphql.String - } - } - } - } + Nodes []graphqlCommitNode PageInfo struct { HasNextPage graphql.Boolean EndCursor graphql.String @@ -340,13 +347,7 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence } `graphql:"commits(first: 100, after: $commitCursor)"` Reviews struct { - Nodes []struct { - Author struct { - Login graphql.String - } - State graphql.String - SubmittedAt graphql.String - } + Nodes []graphqlReviewNode PageInfo struct { HasNextPage graphql.Boolean EndCursor graphql.String @@ -377,69 +378,17 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence return pullRequestsEvidence, err } - // Print results for demonstration for _, pr := range query.Repository.Object.Commit.AssociatedPullRequests.Nodes { - createdAt, err := time.Parse(time.RFC3339, string(pr.CreatedAt)) + // 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), + pr.Commits.Nodes, pr.Reviews.Nodes, + ) if err != nil { return pullRequestsEvidence, err } - mergedAt := int64(0) - if pr.MergedAt != "" { - mergedAtTime, err := time.Parse(time.RFC3339, string(pr.MergedAt)) - if err != nil { - return pullRequestsEvidence, err - } - mergedAt = mergedAtTime.Unix() - } - - evidence := &types.PREvidence{ - URL: string(pr.URL), - MergeCommit: commit, - State: string(pr.State), - Author: string(pr.Author.Login), - CreatedAt: createdAt.Unix(), - MergedAt: mergedAt, - Title: string(pr.Title), - HeadRef: string(pr.HeadRefName), - Approvers: []any{}, - Commits: []types.Commit{}, - } - - for _, c := range pr.Commits.Nodes { - timestamp, err := time.Parse(time.RFC3339, string(c.Commit.CommittedDate)) - if err != nil { - return pullRequestsEvidence, err - } - - committerUsername := "" - if c.Commit.Committer.User != nil { - committerUsername = string(c.Commit.Committer.User.Login) - } - - evidence.Commits = append(evidence.Commits, types.Commit{ - SHA: string(c.Commit.Oid), - Message: string(c.Commit.MessageHeadline), - Committer: fmt.Sprintf("%s <%s>", string(c.Commit.Committer.Name), string(c.Commit.Committer.Email)), - CommitterUsername: committerUsername, - Timestamp: timestamp.Unix(), - Branch: string(pr.HeadRefName), - URL: string(c.Commit.URL), - }) - } - - for _, r := range pr.Reviews.Nodes { - submittedAt, err := time.Parse(time.RFC3339, string(r.SubmittedAt)) - if err != nil { - return pullRequestsEvidence, err - } - - evidence.Approvers = append(evidence.Approvers, types.PRApprovals{ - Username: string(r.Author.Login), - State: string(r.State), - Timestamp: submittedAt.Unix(), - }) - } - pullRequestsEvidence = append(pullRequestsEvidence, evidence) } return pullRequestsEvidence, nil From 74ee259156fca962cced45a5fdd5e83e71ff466b Mon Sep 17 00:00:00 2001 From: Simon Castagna Date: Thu, 23 Apr 2026 08:56:46 +0200 Subject: [PATCH 5/8] fix: return non-nil empty slice from PREvidenceForCommitHybrid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When no PRs are found via the fallback path, the hybrid was returning a nil slice which serialises to JSON null. The server rejects null for pull_requests — it requires a list. Initialise result as an empty non-nil slice in both GithubConfig and FakeGitHubClient. Co-Authored-By: Claude Sonnet 4.6 --- internal/github/fake_github.go | 2 +- internal/github/github.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/github/fake_github.go b/internal/github/fake_github.go index 962876b1c..67fbbf318 100644 --- a/internal/github/fake_github.go +++ b/internal/github/fake_github.go @@ -61,7 +61,7 @@ func (f *FakeGitHubClient) PREvidenceForCommitHybrid(commit string) ([]*types.PR } // Fallback: use PRNumbersByCommit for V1-style discovery. prNumbers := f.PRNumbersByCommit[commit] - var result []*types.PREvidence + result := []*types.PREvidence{} for _, n := range prNumbers { evidence, err := f.PREvidenceByPRNumber(n) if err != nil { diff --git a/internal/github/github.go b/internal/github/github.go index d627c6332..2b1fdc3f4 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -111,7 +111,7 @@ func (c *GithubConfig) PREvidenceForCommitHybrid(commit string) ([]*types.PREvid return nil, err } - var result []*types.PREvidence + result := []*types.PREvidence{} for _, pr := range restPRs { evidence, err := c.PREvidenceByPRNumber(pr.GetNumber()) if err != nil { From e6aad6538aa923cdd7acbb7ad3c637cb447289c7 Mon Sep 17 00:00:00 2001 From: Simon Castagna Date: Thu, 23 Apr 2026 09:27:12 +0200 Subject: [PATCH 6/8] fix: PREvidenceByPRNumber returns error for unknown PR numbers; simplify retry loop Real GitHub GraphQL returns an error for non-existent PR numbers, not nil. Align fake and contract test to match actual API behaviour. Also simplify the retry loop to a standard for-condition form. Co-Authored-By: Claude Sonnet 4.6 --- internal/github/fake_github.go | 8 ++++---- internal/github/github.go | 7 ++----- internal/github/github_contract_test.go | 4 ++-- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/internal/github/fake_github.go b/internal/github/fake_github.go index 67fbbf318..d4833dd7b 100644 --- a/internal/github/fake_github.go +++ b/internal/github/fake_github.go @@ -74,16 +74,16 @@ func (f *FakeGitHubClient) PREvidenceForCommitHybrid(commit string) ([]*types.PR return result, nil } -// PREvidenceByPRNumber mirrors the GraphQL API: returns nil with no error for -// PR numbers not present in PRsByNumber (matching the real GitHub behaviour of -// returning null for non-existent pull requests). +// PREvidenceByPRNumber mirrors the GraphQL API: returns an error for PR numbers +// not present in PRsByNumber (matching the real GitHub behaviour of returning +// an error for non-existent pull requests). func (f *FakeGitHubClient) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, error) { if f.Err != nil { return nil, f.Err } pr, ok := f.PRsByNumber[prNumber] if !ok { - return nil, nil + return nil, fmt.Errorf("could not resolve to a pull request with number %d", prNumber) } return pr, nil } diff --git a/internal/github/github.go b/internal/github/github.go index 2b1fdc3f4..99531e184 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -280,11 +280,8 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er } retryDelays := []time.Duration{10 * time.Second, 20 * time.Second, 30 * time.Second} err = client.Query(ctx, &query, variables) - for _, delay := range retryDelays { - if err == nil { - break - } - sleep(delay) + for i := 0; err != nil && i < len(retryDelays); i++ { + sleep(retryDelays[i]) err = client.Query(ctx, &query, variables) } if err != nil { diff --git a/internal/github/github_contract_test.go b/internal/github/github_contract_test.go index 9476ccc86..bf387050b 100644 --- a/internal/github/github_contract_test.go +++ b/internal/github/github_contract_test.go @@ -87,9 +87,9 @@ func runPRByNumberContractTests(t *testing.T, provider prByNumberRetriever, know require.NotEmpty(t, pr.MergeCommit, "MergeCommit should be present") }) - t.Run("returns nil for unknown PR number", func(t *testing.T) { + t.Run("returns error for unknown PR number", func(t *testing.T) { pr, err := provider.PREvidenceByPRNumber(999999999) - require.NoError(t, err) + require.Error(t, err) require.Nil(t, pr) }) } From d8fe97c27dbc77585392b4dd0aa4707b98c7d0eb Mon Sep 17 00:00:00 2001 From: Simon Castagna Date: Thu, 23 Apr 2026 09:49:15 +0200 Subject: [PATCH 7/8] refactor: unify retry loop so client.Query appears once Use a delays slice starting with 0 so the initial attempt and all retries share a single call site inside the loop. Co-Authored-By: Claude Sonnet 4.6 --- internal/github/github.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/github/github.go b/internal/github/github.go index 99531e184..d2e86764e 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -278,11 +278,15 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er if sleep == nil { sleep = time.Sleep } - retryDelays := []time.Duration{10 * time.Second, 20 * time.Second, 30 * time.Second} - err = client.Query(ctx, &query, variables) - for i := 0; err != nil && i < len(retryDelays); i++ { - sleep(retryDelays[i]) + delays := []time.Duration{0, 10 * time.Second, 20 * time.Second, 30 * time.Second} + for _, delay := range delays { + if delay > 0 { + sleep(delay) + } err = client.Query(ctx, &query, variables) + if err == nil { + break + } } if err != nil { return nil, err From 5b7033e3fe514b101112af44bd08d781878ae76c Mon Sep 17 00:00:00 2001 From: SimonC Date: Thu, 23 Apr 2026 09:58:34 +0200 Subject: [PATCH 8/8] Update internal/github/github.go Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> --- internal/github/github.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/github/github.go b/internal/github/github.go index d2e86764e..ea2e5176e 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -222,7 +222,7 @@ func buildPREvidence( } // PREvidenceByPRNumber fetches full PR evidence for a single PR number via -// GraphQL. Returns nil, nil when the PR does not exist. +// GraphQL. Returns an error when the PR does not exist. func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, error) { ctx := context.Background()