From 072fb2fb19caae803bfd50f07b003c2d30d36a10 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Wed, 21 Oct 2026 17:41:01 +0100 Subject: [PATCH 1/9] test: add helpers for feeding PR evidence code replies with null fields Part of #1283. Provider tests use these to send their evidence code a realistic reply once per field with that field made null, or a list emptied, and to name the outcome: ok, error or panic, plus which evidence fields came back changed or missing. --- internal/testHelpers/oddReplies.go | 291 ++++++++++++++++++++++++ internal/testHelpers/oddReplies_test.go | 120 ++++++++++ 2 files changed, 411 insertions(+) create mode 100644 internal/testHelpers/oddReplies.go create mode 100644 internal/testHelpers/oddReplies_test.go diff --git a/internal/testHelpers/oddReplies.go b/internal/testHelpers/oddReplies.go new file mode 100644 index 000000000..dd9cd29fa --- /dev/null +++ b/internal/testHelpers/oddReplies.go @@ -0,0 +1,291 @@ +package testHelpers + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "reflect" + "sort" + + "github.com/kosli-dev/cli/internal/types" +) + +// NullVariant is a copy of a JSON document with the value at Path set to null. +// A Path ending in "[]" names a list emptied instead. +type NullVariant struct { + Path string + Body []byte +} + +// NullVariants returns one copy of doc per value it holds, with that value +// made null, plus an emptied copy of each non-empty list. The document itself +// is Path "."; its fields have no leading dot. Key order is kept; a variant +// means the same as doc except where its Path says. +func NullVariants(doc []byte) ([]NullVariant, error) { + root, err := parseOrdered(doc) + if err != nil { + return nil, err + } + variants := nullAt(root, root, ".") + var walk func(n *jsonNode, path string) + walk = func(n *jsonNode, path string) { + switch n.kind { + case kindObject: + for _, f := range n.fields { + child := joinPath(path, f.key) + variants = append(variants, nullAt(root, f.value, child)...) + walk(f.value, child) + } + case kindArray: + for i, item := range n.items { + child := fmt.Sprintf("%s[%d]", path, i) + variants = append(variants, nullAt(root, item, child)...) + walk(item, child) + } + } + } + walk(root, "") + return variants, nil +} + +// nullAt gives root rendered with n made null and, for a non-empty list, +// rendered with it emptied. An already-null n gives nothing. +func nullAt(root, n *jsonNode, path string) []NullVariant { + if n.kind == kindNull { + return nil + } + saved := *n + defer func() { *n = saved }() + + *n = jsonNode{kind: kindNull} + out := []NullVariant{{Path: path, Body: root.render()}} + if saved.kind == kindArray && len(saved.items) > 0 { + *n = jsonNode{kind: kindArray} + out = append(out, NullVariant{Path: path + "[]", Body: root.render()}) + } + return out +} + +func joinPath(parent, key string) string { + if parent == "" { + return key + } + return parent + "." + key +} + +// Outcome runs fn and says what happened: "ok", "error", or "panic: " and the +// panic value. Error text is left out because it carries test server ports. +func Outcome(fn func() error) (out string) { + defer func() { + if r := recover(); r != nil { + out = fmt.Sprintf("panic: %v", r) + } + }() + if err := fn(); err != nil { + return "error" + } + return "ok" +} + +// EvidenceOutcome runs fetch and says what happened, as Outcome does. When it +// succeeds it compares each pull request's API payload with baseline's, key by +// key, so it sees what the server would. "lost:" when a value the baseline had +// came back missing or null, or content it had came back empty or shorter; +// "changed:" for any other difference. +func EvidenceOutcome(baseline []*types.PREvidence, fetch func() ([]*types.PREvidence, error)) string { + var got []*types.PREvidence + out := Outcome(func() error { + var err error + got, err = fetch() + return err + }) + if out != "ok" { + return out + } + if len(got) != len(baseline) { + return fmt.Sprintf("ok lost:PullRequests(%d->%d)", len(baseline), len(got)) + } + for i := range baseline { + if got[i] == nil { + out += fmt.Sprintf(" lost:PullRequest%d", i) + continue + } + want, have := payloadFields(baseline[i]), payloadFields(got[i]) + keys := []string{} + for k := range want { + keys = append(keys, k) + } + for k := range have { + if _, ok := want[k]; !ok { + keys = append(keys, k) + } + } + sort.Strings(keys) + for _, k := range keys { + w, h := want[k], have[k] + if reflect.DeepEqual(w, h) { + continue + } + kind := "changed:" + if (w != nil && h == nil) || (!isEmptyValue(w) && (isEmptyValue(h) || shorter(h, w))) { + kind = "lost:" + } + out += " " + kind + k + } + } + return out +} + +// payloadFields is e as the API receives it, one entry per top-level key. +func payloadFields(e *types.PREvidence) map[string]any { + raw, err := json.Marshal(e) + if err != nil { + panic(err) + } + fields := map[string]any{} + dec := json.NewDecoder(bytes.NewReader(raw)) + dec.UseNumber() + if err := dec.Decode(&fields); err != nil { + panic(err) + } + return fields +} + +func isEmptyValue(v any) bool { + switch x := v.(type) { + case nil: + return true + case string: + return x == "" + case json.Number: + return x == "0" + case []any: + return len(x) == 0 + } + return false +} + +func shorter(have, want any) bool { + h, ok1 := have.([]any) + w, ok2 := want.([]any) + return ok1 && ok2 && len(h) < len(w) +} + +type jsonKind int + +const ( + kindScalar jsonKind = iota + kindNull + kindObject + kindArray +) + +// jsonNode keeps object keys in document order, which encoding/json maps lose. +type jsonNode struct { + kind jsonKind + scalar json.RawMessage + fields []jsonField + items []*jsonNode +} + +type jsonField struct { + key string + value *jsonNode +} + +func parseOrdered(doc []byte) (*jsonNode, error) { + dec := json.NewDecoder(bytes.NewReader(doc)) + dec.UseNumber() + n, err := decodeNode(dec) + if err != nil { + return nil, err + } + if _, err := dec.Token(); err != io.EOF { + return nil, fmt.Errorf("trailing data after JSON document") + } + return n, nil +} + +func decodeNode(dec *json.Decoder) (*jsonNode, error) { + tok, err := dec.Token() + if err != nil { + return nil, err + } + switch v := tok.(type) { + case json.Delim: + switch v { + case '{': + n := &jsonNode{kind: kindObject} + for dec.More() { + keyTok, err := dec.Token() + if err != nil { + return nil, err + } + value, err := decodeNode(dec) + if err != nil { + return nil, err + } + n.fields = append(n.fields, jsonField{key: keyTok.(string), value: value}) + } + _, err := dec.Token() + return n, err + case '[': + n := &jsonNode{kind: kindArray} + for dec.More() { + item, err := decodeNode(dec) + if err != nil { + return nil, err + } + n.items = append(n.items, item) + } + _, err := dec.Token() + return n, err + } + return nil, fmt.Errorf("unexpected delimiter %v", v) + case nil: + return &jsonNode{kind: kindNull}, nil + default: + raw, err := json.Marshal(v) + if err != nil { + return nil, err + } + return &jsonNode{kind: kindScalar, scalar: raw}, nil + } +} + +func (n *jsonNode) render() []byte { + var b bytes.Buffer + n.writeTo(&b) + return b.Bytes() +} + +func (n *jsonNode) writeTo(b *bytes.Buffer) { + switch n.kind { + case kindNull: + b.WriteString("null") + case kindScalar: + b.Write(n.scalar) + case kindObject: + b.WriteByte('{') + for i, f := range n.fields { + if i > 0 { + b.WriteByte(',') + } + key, _ := json.Marshal(f.key) + b.Write(key) + b.WriteByte(':') + f.value.writeTo(b) + } + b.WriteByte('}') + case kindArray: + b.WriteByte('[') + for i, item := range n.items { + if i > 0 { + b.WriteByte(',') + } + item.writeTo(b) + } + b.WriteByte(']') + } +} diff --git a/internal/testHelpers/oddReplies_test.go b/internal/testHelpers/oddReplies_test.go new file mode 100644 index 000000000..3fd5672b3 --- /dev/null +++ b/internal/testHelpers/oddReplies_test.go @@ -0,0 +1,120 @@ +package testHelpers + +import ( + "errors" + "testing" + + "github.com/kosli-dev/cli/internal/types" + "github.com/stretchr/testify/require" +) + +func TestNullVariants_NullsEachValueInTurn(t *testing.T) { + variants, err := NullVariants([]byte(`{"a":{"b":1,"c":"x"},"d":[{"e":true},{"e":false}],"f":[]}`)) + require.NoError(t, err) + + got := map[string]string{} + for _, v := range variants { + got[v.Path] = string(v.Body) + } + require.Equal(t, map[string]string{ + ".": `null`, + "a": `{"a":null,"d":[{"e":true},{"e":false}],"f":[]}`, + "a.b": `{"a":{"b":null,"c":"x"},"d":[{"e":true},{"e":false}],"f":[]}`, + "a.c": `{"a":{"b":1,"c":null},"d":[{"e":true},{"e":false}],"f":[]}`, + "d": `{"a":{"b":1,"c":"x"},"d":null,"f":[]}`, + "d[]": `{"a":{"b":1,"c":"x"},"d":[],"f":[]}`, + "d[0]": `{"a":{"b":1,"c":"x"},"d":[null,{"e":false}],"f":[]}`, + "d[0].e": `{"a":{"b":1,"c":"x"},"d":[{"e":null},{"e":false}],"f":[]}`, + "d[1]": `{"a":{"b":1,"c":"x"},"d":[{"e":true},null],"f":[]}`, + "d[1].e": `{"a":{"b":1,"c":"x"},"d":[{"e":true},{"e":null}],"f":[]}`, + "f": `{"a":{"b":1,"c":"x"},"d":[{"e":true},{"e":false}],"f":null}`, + }, got) +} + +func TestNullVariants_LeavesAnAlreadyNullValueAlone(t *testing.T) { + variants, err := NullVariants([]byte(`{"a":null,"b":1}`)) + require.NoError(t, err) + paths := []string{} + for _, v := range variants { + paths = append(paths, v.Path) + } + require.Equal(t, []string{".", "b"}, paths) +} + +func TestNullVariants_RejectsInvalidJSON(t *testing.T) { + _, err := NullVariants([]byte(`{`)) + require.Error(t, err) +} + +func TestOutcome(t *testing.T) { + require.Equal(t, "ok", Outcome(func() error { return nil })) + require.Equal(t, "error", Outcome(func() error { return errors.New("boom") })) + require.Equal(t, "panic: runtime error: invalid memory address or nil pointer dereference", + Outcome(func() error { + var p *struct{ x int } + return errors.New(string(rune(p.x))) + })) +} + +func TestEvidenceOutcome(t *testing.T) { + baseline := []*types.PREvidence{{URL: "u", Title: "t", Commits: []types.Commit{{SHA: "a"}, {SHA: "b"}}}} + fetch := func(e ...*types.PREvidence) func() ([]*types.PREvidence, error) { + return func() ([]*types.PREvidence, error) { return e, nil } + } + + require.Equal(t, "ok", EvidenceOutcome(baseline, fetch(baseline[0]))) + require.Equal(t, "ok lost:commits lost:title", EvidenceOutcome(baseline, + fetch(&types.PREvidence{URL: "u", Commits: []types.Commit{{SHA: "a"}}}))) + require.Equal(t, "ok changed:commits changed:title", EvidenceOutcome(baseline, + fetch(&types.PREvidence{URL: "u", Title: "x", Commits: []types.Commit{{SHA: "a"}, {SHA: ""}}}))) + require.Equal(t, "ok lost:PullRequests(1->0)", EvidenceOutcome(baseline, fetch())) + require.Equal(t, "error", EvidenceOutcome(baseline, func() ([]*types.PREvidence, error) { + return nil, errors.New("boom") + })) +} + +func TestNullVariants_NullsAndEmptiesARootList(t *testing.T) { + variants, err := NullVariants([]byte(`[{"a":1}]`)) + require.NoError(t, err) + got := map[string]string{} + for _, v := range variants { + got[v.Path] = string(v.Body) + } + require.Equal(t, map[string]string{ + ".": `null`, + ".[]": `[]`, + "[0]": `[null]`, + "[0].a": `[{"a":null}]`, + }, got) +} + +func TestNullVariants_RejectsTrailingData(t *testing.T) { + _, err := NullVariants([]byte(`{"a":1} garbage`)) + require.Error(t, err) +} + +func TestEvidenceOutcome_ReportsANilEntryWithoutPanicking(t *testing.T) { + baseline := []*types.PREvidence{{URL: "u"}} + require.Equal(t, "ok lost:PullRequest0", EvidenceOutcome(baseline, func() ([]*types.PREvidence, error) { + return []*types.PREvidence{nil}, nil + })) +} + +func TestEvidenceOutcome_ComparesWhatTheAPIReceives(t *testing.T) { + baseline := []*types.PREvidence{{URL: "u", Reviews: []types.PRApprovals{}}} + + // Commits are always sent as a list, so nil and empty are the same payload. + require.Equal(t, "ok", EvidenceOutcome([]*types.PREvidence{{URL: "u", Commits: []types.Commit{}}}, + func() ([]*types.PREvidence, error) { return []*types.PREvidence{{URL: "u"}}, nil })) + // No reviews key means reviews were not collected, unlike an empty list. + require.Equal(t, "ok lost:reviews", EvidenceOutcome(baseline, + func() ([]*types.PREvidence, error) { return []*types.PREvidence{{URL: "u"}}, nil })) + // A key only the result has is still compared. + require.Equal(t, "ok changed:reviews changed:title", EvidenceOutcome([]*types.PREvidence{{URL: "u"}}, + func() ([]*types.PREvidence, error) { + return []*types.PREvidence{{URL: "u", Title: "t", Reviews: []types.PRApprovals{}}}, nil + })) + // Empty in both is not a loss. + require.Equal(t, "ok changed:approvers", EvidenceOutcome([]*types.PREvidence{{URL: "u"}}, + func() ([]*types.PREvidence, error) { return []*types.PREvidence{{URL: "u", Approvers: []any{}}}, nil })) +} From b769497adb125746f9a7cb8e1213a9347923924a Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Wed, 21 Oct 2026 18:09:42 +0100 Subject: [PATCH 2/9] fix(github): treat a null field the same with or without a GraphQL error GitHub often nulls a field it cannot resolve and adds an errors entry for it. The GraphQL client fails on any error and drops its path, so one such field (v2.48.0: a signer that is a GitHub App) lost the whole PR evidence. Only the signer lookup tolerated it, and it accepted any error that came with data, including ones that hide missing data. Every evidence query now goes through one client whose transport drops an error only when its path names an object field that is null while everything above it is not. Errors on list items, on fields under a null parent, or with no path still fail. Dropped errors go to stderr with --debug. signerLogins loses its own tolerance. Also: a null timestamp reads as 0, a null list item is an error instead of a zero-value entry, and a missing repository or pull request is an error on every query (it was nil, nil by number, and a silent stop mid-drain). Part of #1283 Co-Authored-By: Claude Opus 5.5 --- internal/github/github.go | 112 ++++--- internal/github/github_test.go | 8 +- internal/github/graphql_client.go | 124 +++++++ internal/github/odd_replies_test.go | 446 ++++++++++++++++++++++++++ internal/github/pagination.go | 43 ++- internal/github/pr_pagination_test.go | 22 +- 6 files changed, 676 insertions(+), 79 deletions(-) create mode 100644 internal/github/graphql_client.go create mode 100644 internal/github/odd_replies_test.go diff --git a/internal/github/github.go b/internal/github/github.go index afd3c5dc0..7e919087e 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -252,9 +252,7 @@ func (c *GithubConfig) PREvidenceForCommitHybrid(commit string) ([]*types.PREvid if err != nil { return nil, err } - if evidence != nil { - result = append(result, evidence) - } + result = append(result, evidence) } return result, nil } @@ -293,11 +291,9 @@ const signerBatchSize = 100 // signerLogins maps each signed commit's SHA to its signer's login. GitHub // types signer as User, so a signer that is not one (e.g. a GitHub App) -// comes back null with a field error. The client decodes data before -// returning errors but drops their path, so any error that came with data -// is accepted and written to debug, when not nil. +// comes back null with a field error, which newGraphQLClient drops. func (c *GithubConfig) signerLogins(ctx context.Context, query graphqlQueryFunc, - nodes []graphqlCommitNode, debug io.Writer) (map[string]string, error) { + nodes []graphqlCommitNode) (map[string]string, error) { ids := []graphql.ID{} for _, n := range nodes { if n.Commit.Signature != nil { @@ -305,7 +301,7 @@ func (c *GithubConfig) signerLogins(ctx context.Context, query graphqlQueryFunc, } } type signerQuery struct { - Nodes []struct { + Nodes []*struct { Commit struct { Oid graphql.String Signature *struct { @@ -319,23 +315,18 @@ func (c *GithubConfig) signerLogins(ctx context.Context, query graphqlQueryFunc, signers := map[string]string{} for batch := range slices.Chunk(ids, signerBatchSize) { var q signerQuery - // Only a reply with no data is retried: a field error comes back - // the same on every attempt. err := c.retry(ctx, func() error { q = signerQuery{} - err := query(ctx, &q, map[string]any{"ids": batch}) - if err != nil && len(q.Nodes) > 0 { - if debug != nil { - _, _ = fmt.Fprintf(debug, "[debug-github] ignoring error in commit signer lookup: %v\n", err) - } - return nil - } - return err + return query(ctx, &q, map[string]any{"ids": batch}) }) if err != nil { return nil, fmt.Errorf("failed to look up commit signers: %w", err) } - for _, n := range q.Nodes { + found, err := nonNull(q.Nodes, "signer nodes") + if err != nil { + return nil, err + } + for _, n := range found { if sig := n.Commit.Signature; sig != nil && sig.Signer != nil { signers[string(n.Commit.Oid)] = string(sig.Signer.Login) } @@ -375,17 +366,13 @@ func buildPREvidence( reviewNodes []graphqlReviewNode, signers map[string]string, ) (*types.PREvidence, error) { - createdAt, err := time.Parse(time.RFC3339, createdAtStr) + createdAt, err := unixTime(createdAtStr) if err != nil { return nil, err } - mergedAt := int64(0) - if mergedAtStr != "" { - mergedAtTime, err := time.Parse(time.RFC3339, mergedAtStr) - if err != nil { - return nil, err - } - mergedAt = mergedAtTime.Unix() + mergedAt, err := unixTime(mergedAtStr) + if err != nil { + return nil, err } evidence := &types.PREvidence{ @@ -393,7 +380,7 @@ func buildPREvidence( MergeCommit: mergeCommit, State: state, Author: author, - CreatedAt: createdAt.Unix(), + CreatedAt: createdAt, MergedAt: mergedAt, Title: title, HeadRef: headRef, @@ -410,7 +397,7 @@ func buildPREvidence( if dateStr == "" { dateStr = string(n.Commit.CommittedDate) } - timestamp, err := time.Parse(time.RFC3339, dateStr) + timestamp, err := unixTime(dateStr) if err != nil { return nil, err } @@ -436,7 +423,7 @@ func buildPREvidence( Message: string(n.Commit.MessageHeadline), Author: fmt.Sprintf("%s <%s>", string(n.Commit.Author.Name), string(n.Commit.Author.Email)), AuthorUsername: authorUsername, - Timestamp: timestamp.Unix(), + Timestamp: timestamp, Branch: headRef, URL: string(n.Commit.URL), Verified: verified, @@ -447,14 +434,14 @@ func buildPREvidence( } for _, r := range reviewNodes { - submittedAt, err := time.Parse(time.RFC3339, string(r.SubmittedAt)) + submittedAt, err := unixTime(string(r.SubmittedAt)) if err != nil { return nil, err } review := types.PRApprovals{ Username: string(r.Author.Login), State: string(r.State), - Timestamp: submittedAt.Unix(), + Timestamp: submittedAt, } if r.Commit != nil { review.CommitSHA = string(r.Commit.Oid) @@ -466,17 +453,40 @@ func buildPREvidence( return evidence, nil } +// unixTime parses a GitHub timestamp. A null one reads as "" and gives 0. +func unixTime(s string) (int64, error) { + if s == "" { + return 0, nil + } + t, err := time.Parse(time.RFC3339, s) + if err != nil { + return 0, err + } + return t.Unix(), nil +} + +// nonNull returns items as values. A null item is an entry the reply could +// not give, so it fails rather than being skipped. +func nonNull[T any](items []*T, list string) ([]T, error) { + out := make([]T, 0, len(items)) + for i, item := range items { + if item == nil { + return nil, fmt.Errorf("%s[%d] is null in the GitHub reply", list, i) + } + out = append(out, *item) + } + return out, nil +} + // PREvidenceByPRNumber fetches full PR evidence for a single PR number via // GraphQL. Returns an error when the PR does not exist. func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, error) { ctx := context.Background() - ghClient, err := NewGithubClientFromToken(ctx, c.Token, c.BaseURL, c.Debug) + client, err := c.newGraphQLClient(ctx) if err != nil { return nil, err } - httpClient := ghClient.Client() - client := graphql.NewClient(graphqlEndpoint(c.BaseURL), httpClient) var query struct { Repository struct { @@ -496,11 +506,11 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er } HeadRefOid graphql.String Commits struct { - Nodes []graphqlCommitNode + Nodes []*graphqlCommitNode PageInfo pageInfo } `graphql:"commits(first: 100, after: $commitCursor)"` Reviews struct { - Nodes []graphqlReviewNode + Nodes []*graphqlReviewNode PageInfo pageInfo } `graphql:"reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED], after: $reviewCursor)"` } `graphql:"pullRequest(number: $prNumber)"` @@ -522,7 +532,7 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er pr := query.Repository.PullRequest if pr == nil { - return nil, nil + return nil, fmt.Errorf("could not find pull request %d in %s/%s", prNumber, c.Org, c.Repository) } mergeCommit := "" @@ -539,7 +549,7 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er if err != nil { return nil, err } - signers, err := c.signerLogins(ctx, client.Query, commits, c.debugOut()) + signers, err := c.signerLogins(ctx, client.Query, commits) if err != nil { return nil, err } @@ -555,20 +565,17 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence ctx := context.Background() pullRequestsEvidence := []*types.PREvidence{} - ghClient, err := NewGithubClientFromToken(ctx, c.Token, c.BaseURL, c.Debug) + client, err := c.newGraphQLClient(ctx) if err != nil { return pullRequestsEvidence, err } - httpClient := ghClient.Client() - - client := graphql.NewClient(graphqlEndpoint(c.BaseURL), httpClient) var query struct { - Repository struct { + Repository *struct { Object struct { Commit struct { AssociatedPullRequests struct { - Nodes []struct { + Nodes []*struct { Number graphql.Int // Follow-up page queries resolve the PR by number, so they // need the repo it actually lives in, not the configured one. @@ -592,12 +599,12 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence } Commits struct { - Nodes []graphqlCommitNode + Nodes []*graphqlCommitNode PageInfo pageInfo } `graphql:"commits(first: 100, after: $commitCursor)"` Reviews struct { - Nodes []graphqlReviewNode + Nodes []*graphqlReviewNode PageInfo pageInfo } `graphql:"reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED], after: $reviewCursor)"` } @@ -622,11 +629,18 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence if err := client.Query(ctx, &query, variables); err != nil { return pullRequestsEvidence, err } + if query.Repository == nil { + return pullRequestsEvidence, fmt.Errorf("could not find repository %s/%s", c.Org, c.Repository) + } + prs, err := nonNull(query.Repository.Object.Commit.AssociatedPullRequests.Nodes, "associatedPullRequests") + if err != nil { + return pullRequestsEvidence, err + } // Only the follow-up pages retry; the initial query keeps its fail-fast // behaviour, which callers already depend on. retrying := c.queryWithRetry(client) - for _, pr := range query.Repository.Object.Commit.AssociatedPullRequests.Nodes { + for _, pr := range prs { ref := prRef{ Owner: string(pr.Repository.Owner.Login), Repo: string(pr.Repository.Name), @@ -648,7 +662,7 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence if err != nil { return pullRequestsEvidence, err } - signers, err := c.signerLogins(ctx, client.Query, commits, c.debugOut()) + signers, err := c.signerLogins(ctx, client.Query, commits) if err != nil { return pullRequestsEvidence, err } diff --git a/internal/github/github_test.go b/internal/github/github_test.go index 220332627..1dfa5faf2 100644 --- a/internal/github/github_test.go +++ b/internal/github/github_test.go @@ -78,10 +78,6 @@ func TestGithubTestSuite(t *testing.T) { suite.Run(t, new(GithubTestSuite)) } -// graphqlNullPRResponse is a minimal valid GraphQL response where the PR is null. -// PREvidenceByPRNumber returns nil, nil in this case. -const graphqlNullPRResponse = `{"data":{"repository":{"pullRequest":null}}}` - func newRetryTestServer(t *testing.T, failCount int) (*httptest.Server, *int) { t.Helper() attempts := 0 @@ -96,7 +92,7 @@ func newRetryTestServer(t *testing.T, failCount int) (*httptest.Server, *int) { return } w.Header().Set("Content-Type", "application/json") - _, _ = fmt.Fprint(w, graphqlNullPRResponse) + _, _ = fmt.Fprint(w, byPRNumberResponse(connectionJSON(nil, ""), connectionJSON(nil, ""))) })) return ts, &attempts } @@ -120,7 +116,7 @@ func TestPREvidenceByPRNumber_RetriesOnGraphQLError(t *testing.T) { pr, err := config.PREvidenceByPRNumber(1) require.NoError(t, err) - require.Nil(t, pr) + require.NotNil(t, pr) require.Equal(t, 3, *attempts, "should have retried twice before succeeding") require.Len(t, sleptDurations, 2, "should have slept between retries") } diff --git a/internal/github/graphql_client.go b/internal/github/graphql_client.go new file mode 100644 index 000000000..e058eb908 --- /dev/null +++ b/internal/github/graphql_client.go @@ -0,0 +1,124 @@ +package github + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + + "github.com/shurcooL/graphql" +) + +// newGraphQLClient is the one GraphQL client every evidence query uses. +func (c *GithubConfig) newGraphQLClient(ctx context.Context) (*graphql.Client, error) { + ghClient, err := NewGithubClientFromToken(ctx, c.Token, c.BaseURL, c.Debug) + if err != nil { + return nil, err + } + httpClient := &http.Client{Transport: &nullFieldErrorTransport{ + base: ghClient.Client().Transport, + debug: c.debugOut(), + }} + return graphql.NewClient(graphqlEndpoint(c.BaseURL), httpClient), nil +} + +// nullFieldErrorTransport drops each GraphQL error that only explains why one +// field came back null, so a null field reads the same with or without an +// error. The client returns any error at all as a failure and drops its path, +// so this is the only place the path can still be checked. +type nullFieldErrorTransport struct { + base http.RoundTripper + debug io.Writer +} + +func (t *nullFieldErrorTransport) RoundTrip(req *http.Request) (*http.Response, error) { + resp, err := t.base.RoundTrip(req) + if err != nil || resp.StatusCode != http.StatusOK { + return resp, err + } + body, err := io.ReadAll(resp.Body) + _ = resp.Body.Close() + if err != nil { + return nil, err + } + resp.Body = io.NopCloser(bytes.NewReader(t.withoutNullFieldErrors(body))) + return resp, nil +} + +// withoutNullFieldErrors returns body minus the errors isNullField accepts, +// or body unchanged when it cannot be read or nothing is dropped. +func (t *nullFieldErrorTransport) withoutNullFieldErrors(body []byte) []byte { + var reply map[string]json.RawMessage + if json.Unmarshal(body, &reply) != nil { + return body + } + var errs []json.RawMessage + var data any + if json.Unmarshal(reply["errors"], &errs) != nil || json.Unmarshal(reply["data"], &data) != nil { + return body + } + kept := []json.RawMessage{} + for _, e := range errs { + var parsed struct{ Path []any } + if json.Unmarshal(e, &parsed) == nil && isNullField(data, parsed.Path) { + if t.debug != nil { + _, _ = fmt.Fprintf(t.debug, "[debug-github] ignoring error on null field: %s\n", e) + } + continue + } + kept = append(kept, e) + } + if len(kept) == len(errs) { + return body + } + if len(kept) == 0 { + delete(reply, "errors") + } else { + reply["errors"], _ = json.Marshal(kept) + } + out, err := json.Marshal(reply) + if err != nil { + return body + } + return out +} + +// isNullField reports whether path names an object field that is null in +// data while everything above it is not. An error anywhere else (a list item, +// a field under a null parent, no path) is more than one missing field. +func isNullField(data any, path []any) bool { + if len(path) == 0 { + return false + } + field, ok := path[len(path)-1].(string) + if !ok { + return false + } + cur := data + for _, seg := range path[:len(path)-1] { + switch s := seg.(type) { + case string: + obj, ok := cur.(map[string]any) + if !ok { + return false + } + cur = obj[s] + case float64: + list, ok := cur.([]any) + if !ok || s < 0 || int(s) >= len(list) { + return false + } + cur = list[int(s)] + default: + return false + } + } + obj, ok := cur.(map[string]any) + if !ok { + return false + } + value, present := obj[field] + return present && value == nil +} diff --git a/internal/github/odd_replies_test.go b/internal/github/odd_replies_test.go new file mode 100644 index 000000000..6aa3cc189 --- /dev/null +++ b/internal/github/odd_replies_test.go @@ -0,0 +1,446 @@ +package github + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "regexp" + "sort" + "strconv" + "strings" + "testing" + "time" + + "github.com/kosli-dev/cli/internal/testHelpers" + "github.com/kosli-dev/cli/internal/types" + "github.com/stretchr/testify/require" +) + +// The odd-reply tests feed the PR evidence path replies that GitHub's schema +// allows but the baseline does not use: a nullable field made null, or a list +// emptied, with and without an errors entry naming it (v2.48.0 broke on a null +// signature.signer). They assert the rule in kosli-dev/cli#1283. + +// Reply names: one per kind of query the evidence path sends. +const ( + replyByNumber = "byNumber" + replyForCommit = "forCommit" + replyCommitPage = "commitsPage" + replyReviewPage = "reviewsPage" + replySigners = "signers" +) + +// oddReplyQueryRoutes picks the reply for a request by its query text. Order +// matters: the main queries also contain "commits(first: 100". +var oddReplyQueryRoutes = []struct{ contains, reply string }{ + {"nodes(ids:", replySigners}, + {"associatedPullRequests", replyForCommit}, + {"$commitCursor", replyByNumber}, + {"commits(first: 100, after: $cursor)", replyCommitPage}, + {"reviews(first: 100", replyReviewPage}, +} + +// newOddReplyServer answers each GraphQL query with the reply its text routes +// to, however many times it is asked, so retries and extra pages never run +// the server out of replies. +func newOddReplyServer(t *testing.T, replies map[string]string) *httptest.Server { + t.Helper() + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + var req struct{ Query string } + _ = json.Unmarshal(body, &req) + for _, route := range oddReplyQueryRoutes { + if strings.Contains(req.Query, route.contains) { + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprint(w, replies[route.reply]) + return + } + } + t.Errorf("no reply routed for query: %s", req.Query) + w.WriteHeader(http.StatusInternalServerError) + })) + t.Cleanup(ts.Close) + return ts +} + +const ( + // sha1 is signed by a user GitHub resolves; sha2 is unsigned. + oddSignedCommit = `{"commit":{"id":"C_sha1","oid":"sha1","messageHeadline":"first",` + + `"committedDate":"2026-03-01T12:00:00Z","authoredDate":"2026-03-01T11:30:00Z",` + + `"url":"https://github.com/test-org/test-repo/commit/sha1",` + + `"author":{"name":"Ada","email":"ada@example.com","user":{"login":"ada"}},` + + `"signature":{"isValid":true,"state":"VALID","wasSignedByGitHub":false}}}` + oddUnsignedCommit = `{"commit":{"id":"C_sha2","oid":"sha2","messageHeadline":"second",` + + `"committedDate":"2026-03-01T12:30:00Z","authoredDate":"2026-03-01T12:15:00Z",` + + `"url":"https://github.com/test-org/test-repo/commit/sha2",` + + `"author":{"name":"Ada","email":"ada@example.com","user":{"login":"ada"}},` + + `"signature":null}}` + oddApproval = `{"author":{"login":"grace"},"state":"APPROVED",` + + `"submittedAt":"2026-03-01T13:00:00Z","commit":{"oid":"sha2"}}` + oddChangeRequest = `{"author":{"login":"linus"},"state":"CHANGES_REQUESTED",` + + `"submittedAt":"2026-03-01T12:45:00Z","commit":{"oid":"sha1"}}` + + // The main queries return one commit and one review with a next page, so + // both follow-up page queries run. + oddPRFields = `"title":"A PR","state":"MERGED","headRefName":"feature","baseRefName":"main",` + + `"url":"https://github.com/test-org/test-repo/pull/7","createdAt":"2026-03-01T11:00:00Z",` + + `"mergedAt":"2026-03-01T14:00:00Z",` + oddPRConnections = `"author":{"login":"ada"},"headRefOid":"sha2",` + + `"commits":{"nodes":[` + oddSignedCommit + `],"pageInfo":{"hasNextPage":true,"endCursor":"c1"}},` + + `"reviews":{"nodes":[` + oddApproval + `],"pageInfo":{"hasNextPage":true,"endCursor":"r1"}}` + + oddByNumberReply = `{"data":{"repository":{"pullRequest":{` + oddPRFields + + `"mergeCommit":{"oid":"merge-sha"},` + oddPRConnections + `}}}}` + oddForCommitReply = `{"data":{"repository":{"object":{"associatedPullRequests":{"nodes":[{"number":7,` + + `"repository":{"name":"test-repo","owner":{"login":"test-org"}},` + oddPRFields + oddPRConnections + `}]}}}}}` + oddCommitPageReply = `{"data":{"repository":{"pullRequest":{"commits":{"nodes":[` + oddUnsignedCommit + + `],"pageInfo":{"hasNextPage":false,"endCursor":"c2"}}}}}}` + oddReviewPageReply = `{"data":{"repository":{"pullRequest":{"reviews":{"nodes":[` + oddChangeRequest + + `],"pageInfo":{"hasNextPage":false,"endCursor":"r2"}}}}}}` + oddSignersReply = `{"data":{"nodes":[{"oid":"sha1","signature":{"signer":{"login":"ada"}}}]}}` +) + +func oddBaselineReplies() map[string]string { + return map[string]string{ + replyByNumber: oddByNumberReply, + replyForCommit: oddForCommitReply, + replyCommitPage: oddCommitPageReply, + replyReviewPage: oddReviewPageReply, + replySigners: oddSignersReply, + } +} + +// oddNullable lists the reply paths whose GitHub schema type is nullable, for +// every field the evidence queries select. "[]" stands for a list item. +// Source: https://docs.github.com/public/fpt/schema.docs.graphql, checked +// 2026-10-21. GraphQL never returns null for a non-null (!) field, so only +// these are varied; an emptied list is legal for every list and always varied. +var oddNullable = map[string]bool{ + // Query.repository: Repository; Repository.pullRequest: PullRequest + "data.repository": true, + "data.repository.pullRequest": true, + // PullRequest.mergedAt: DateTime; mergeCommit: Commit; author: Actor + "data.repository.pullRequest.mergedAt": true, + "data.repository.pullRequest.mergeCommit": true, + "data.repository.pullRequest.author": true, + // PullRequestCommitConnection.nodes: [PullRequestCommit]; PageInfo.endCursor: String + "data.repository.pullRequest.commits.nodes": true, + "data.repository.pullRequest.commits.nodes[]": true, + "data.repository.pullRequest.commits.pageInfo.endCursor": true, + // Commit.author: GitActor; GitActor.name/email: String; user: User; Commit.signature: GitSignature + "data.repository.pullRequest.commits.nodes[].commit.author": true, + "data.repository.pullRequest.commits.nodes[].commit.author.name": true, + "data.repository.pullRequest.commits.nodes[].commit.author.email": true, + "data.repository.pullRequest.commits.nodes[].commit.author.user": true, + "data.repository.pullRequest.commits.nodes[].commit.signature": true, + // PullRequest.reviews: PullRequestReviewConnection; nodes: [PullRequestReview] + "data.repository.pullRequest.reviews": true, + "data.repository.pullRequest.reviews.nodes": true, + "data.repository.pullRequest.reviews.nodes[]": true, + "data.repository.pullRequest.reviews.pageInfo.endCursor": true, + // PullRequestReview.author: Actor; submittedAt: DateTime; commit: Commit + "data.repository.pullRequest.reviews.nodes[].author": true, + "data.repository.pullRequest.reviews.nodes[].submittedAt": true, + "data.repository.pullRequest.reviews.nodes[].commit": true, + + // Repository.object: GitObject; Commit.associatedPullRequests: PullRequestConnection; + // PullRequestConnection.nodes: [PullRequest] + "data.repository.object": true, + "data.repository.object.associatedPullRequests": true, + "data.repository.object.associatedPullRequests.nodes": true, + "data.repository.object.associatedPullRequests.nodes[]": true, + "data.repository.object.associatedPullRequests.nodes[].mergedAt": true, + "data.repository.object.associatedPullRequests.nodes[].author": true, + "data.repository.object.associatedPullRequests.nodes[].commits.nodes": true, + "data.repository.object.associatedPullRequests.nodes[].commits.nodes[]": true, + "data.repository.object.associatedPullRequests.nodes[].commits.pageInfo.endCursor": true, + "data.repository.object.associatedPullRequests.nodes[].commits.nodes[].commit.author": true, + "data.repository.object.associatedPullRequests.nodes[].commits.nodes[].commit.author.name": true, + "data.repository.object.associatedPullRequests.nodes[].commits.nodes[].commit.author.email": true, + "data.repository.object.associatedPullRequests.nodes[].commits.nodes[].commit.author.user": true, + "data.repository.object.associatedPullRequests.nodes[].commits.nodes[].commit.signature": true, + "data.repository.object.associatedPullRequests.nodes[].reviews": true, + "data.repository.object.associatedPullRequests.nodes[].reviews.nodes": true, + "data.repository.object.associatedPullRequests.nodes[].reviews.nodes[]": true, + "data.repository.object.associatedPullRequests.nodes[].reviews.pageInfo.endCursor": true, + "data.repository.object.associatedPullRequests.nodes[].reviews.nodes[].author": true, + "data.repository.object.associatedPullRequests.nodes[].reviews.nodes[].submittedAt": true, + "data.repository.object.associatedPullRequests.nodes[].reviews.nodes[].commit": true, + + // Query.nodes: [Node]! (items nullable); Commit.signature: GitSignature; GitSignature.signer: User + "data.nodes[]": true, + "data.nodes[].signature": true, + "data.nodes[].signature.signer": true, +} + +var listIndex = regexp.MustCompile(`\[\d+\]`) + +// oddVariants returns the schema-legal variants of reply: nullable fields made +// null and lists emptied. +func oddVariants(t *testing.T, reply string) []testHelpers.NullVariant { + t.Helper() + all, err := testHelpers.NullVariants([]byte(reply)) + require.NoError(t, err) + legal := []testHelpers.NullVariant{} + for _, v := range all { + if strings.HasSuffix(v.Path, "[]") || oddNullable[listIndex.ReplaceAllString(v.Path, "[]")] { + legal = append(legal, v) + } + } + return legal +} + +type oddEntryPoint struct { + name string + replies []string // the replies this entry point's queries read + run func(c *GithubConfig) (*types.PREvidence, error) +} + +var oddEntryPoints = []oddEntryPoint{ + { + name: "PREvidenceByPRNumber", + replies: []string{replyByNumber, replyCommitPage, replyReviewPage, replySigners}, + run: func(c *GithubConfig) (*types.PREvidence, error) { return c.PREvidenceByPRNumber(7) }, + }, + { + name: "PREvidenceForCommitV2", + replies: []string{replyForCommit, replyCommitPage, replyReviewPage, replySigners}, + run: func(c *GithubConfig) (*types.PREvidence, error) { + prs, err := c.PREvidenceForCommitV2("merge-sha") + if len(prs) == 0 { + return nil, err + } + return prs[0], err + }, + }, +} + +// runOddReply runs ep against replies and describes the result: the outcome, +// plus the evidence fields that differ from want when it is ok. +func runOddReply(t *testing.T, ep oddEntryPoint, replies map[string]string, want *types.PREvidence) string { + t.Helper() + ts := newOddReplyServer(t, replies) + config := &GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", Repository: "test-repo", + Sleep: func(time.Duration) {}} + return testHelpers.EvidenceOutcome([]*types.PREvidence{want}, func() ([]*types.PREvidence, error) { + got, err := ep.run(config) + if got == nil { + return nil, err + } + return []*types.PREvidence{got}, err + }) +} + +func oddBaselineEvidence(t *testing.T, ep oddEntryPoint) *types.PREvidence { + t.Helper() + ts := newOddReplyServer(t, oddBaselineReplies()) + evidence, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", + Repository: "test-repo", Sleep: func(time.Duration) {}}) + require.NoError(t, err) + require.NotNil(t, evidence) + return evidence +} + +func TestOddReplies_BaselineGivesFullEvidence(t *testing.T) { + for _, ep := range oddEntryPoints { + t.Run(ep.name, func(t *testing.T) { + e := oddBaselineEvidence(t, ep) + require.Equal(t, "merge-sha", e.MergeCommit) + require.Equal(t, "MERGED", e.State) + require.Equal(t, "ada", e.Author) + require.NotZero(t, e.CreatedAt) + require.NotZero(t, e.MergedAt) + require.Equal(t, "sha2", e.HeadSHA) + require.Equal(t, []string{"sha1", "sha2"}, shasIn(e)) + require.Equal(t, "ada", e.Commits[0].SignerUsername) + require.True(t, *e.Commits[0].Verified) + require.Nil(t, e.Commits[1].Verified) + require.Len(t, e.Reviews, 2) + require.Equal(t, "sha2", e.Reviews[0].CommitSHA) + require.Equal(t, "CHANGES_REQUESTED", e.Reviews[1].State) + require.Len(t, e.Approvers, 1) + }) + } +} + +// oddAllowedErrors are the nullable fields whose null fails the evidence +// lookup instead of leaving part of it empty, keyed as in oddNullable. +var oddAllowedErrors = map[string]string{ + "data.repository": "the repository every query is about; null means it is missing or unreadable", + "data.repository.pullRequest": "the pull request asked for by number; null means it is missing or unreadable", +} + +// oddWholePRLost are the V2 fields whose null means the commit has no pull +// requests, which is a valid answer. +var oddWholePRLost = map[string]bool{ + "data.repository.object": true, + "data.repository.object.associatedPullRequests": true, + "data.repository.object.associatedPullRequests.nodes": true, +} + +// oddAffected is the evidence a null at path may empty or change. +func oddAffected(reply, path string) []string { + switch { + case reply == replySigners || strings.Contains(path, ".commits"): + return []string{"commits"} + case strings.Contains(path, ".reviews"): + return []string{"approvers", "reviews"} + } + last := path[strings.LastIndex(path, ".")+1:] + return map[string][]string{ + "mergedAt": {"merged_at"}, + "mergeCommit": {"merge_commit"}, + "author": {"author"}, + }[last] +} + +// requireRuleHolds checks one outcome against the rule for a variant of reply +// with the value at path made null, or emptied for a path ending "[]". +func requireRuleHolds(t *testing.T, reply, path, outcome string) { + t.Helper() + require.NotContains(t, outcome, "panic") + key := listIndex.ReplaceAllString(path, "[]") + switch { + case strings.HasSuffix(path, "[]"): + require.NotEqual(t, "error", outcome, "an emptied list is not an error") + case strings.HasSuffix(path, "]"): + require.Equal(t, "error", outcome, "a null list item is missing data, not an optional field") + case oddAllowedErrors[key] != "": + require.Equal(t, "error", outcome, oddAllowedErrors[key]) + case oddWholePRLost[key]: + require.Equal(t, "ok lost:PullRequests(1->0)", outcome) + default: + require.True(t, strings.HasPrefix(outcome, "ok"), "a null field is not an error, got %q", outcome) + allowed := oddAffected(reply, path) + for _, diff := range strings.Fields(outcome)[1:] { + _, field, _ := strings.Cut(diff, ":") + require.Contains(t, allowed, field, "a null at %s changed unrelated evidence", path) + } + } +} + +// TestOddReplies_NullsFollowTheRule feeds each entry point every legal +// variant of every reply it reads. +func TestOddReplies_NullsFollowTheRule(t *testing.T) { + for _, ep := range oddEntryPoints { + want := oddBaselineEvidence(t, ep) + for _, reply := range ep.replies { + for _, v := range oddVariants(t, oddBaselineReplies()[reply]) { + t.Run(ep.name+"/"+reply+"/"+v.Path, func(t *testing.T) { + replies := oddBaselineReplies() + replies[reply] = string(v.Body) + requireRuleHolds(t, reply, v.Path, runOddReply(t, ep, replies, want)) + }) + } + } + } +} + +// TestOddReplies_FieldErrorMeansTheSameAsNull adds an errors entry naming +// each nullable field made null, as GitHub does when it cannot resolve one. +func TestOddReplies_FieldErrorMeansTheSameAsNull(t *testing.T) { + for _, ep := range oddEntryPoints { + want := oddBaselineEvidence(t, ep) + for _, reply := range ep.replies { + for _, v := range oddVariants(t, oddBaselineReplies()[reply]) { + if strings.HasSuffix(v.Path, "[]") { + continue + } + t.Run(ep.name+"/"+reply+"/"+v.Path, func(t *testing.T) { + plain := oddBaselineReplies() + plain[reply] = string(v.Body) + withError := oddBaselineReplies() + withError[reply] = withErrorAt(t, string(v.Body), v.Path) + require.Equal(t, runOddReply(t, ep, plain, want), runOddReply(t, ep, withError, want)) + }) + } + } + } +} + +// TestOddReplies_ErrorsThatAreNotANullFieldFail covers errors the client must +// not drop: one on a field under a null parent (a non-null field's error nulls +// its nearest nullable parent), one with no path, and one on a list item. +func TestOddReplies_ErrorsThatAreNotANullFieldFail(t *testing.T) { + for _, ep := range oddEntryPoints { + for _, reply := range ep.replies { + baseline := oddBaselineReplies()[reply] + all, err := testHelpers.NullVariants([]byte(baseline)) + require.NoError(t, err) + for _, v := range all { + if v.Path == "." || strings.HasSuffix(v.Path, "[]") { + continue + } + for _, child := range keysAt(t, baseline, v.Path) { + t.Run(ep.name+"/"+reply+"/"+v.Path+"."+child, func(t *testing.T) { + replies := oddBaselineReplies() + replies[reply] = withErrorAt(t, string(v.Body), v.Path+"."+child) + require.Equal(t, "error", runOddReply(t, ep, replies, nil)) + }) + } + } + t.Run(ep.name+"/"+reply+"/no path", func(t *testing.T) { + replies := oddBaselineReplies() + replies[reply] = strings.TrimSuffix(baseline, "}") + `,"errors":[{"message":"Something went wrong"}]}` + require.Equal(t, "error", runOddReply(t, ep, replies, nil)) + }) + } + t.Run(ep.name+"/list item", func(t *testing.T) { + replies := oddBaselineReplies() + replies[replySigners] = withErrorAt(t, `{"data":{"nodes":[null]}}`, "data.nodes[0]") + require.Equal(t, "error", runOddReply(t, ep, replies, nil)) + }) + } +} + +// keysAt names the fields of the object at path in doc, if it is one. +func keysAt(t *testing.T, doc, path string) []string { + t.Helper() + var cur any + require.NoError(t, json.Unmarshal([]byte(doc), &cur)) + for _, seg := range errorPath(t, path) { + switch s := seg.(type) { + case string: + cur = cur.(map[string]any)[s] + case int: + cur = cur.([]any)[s] + } + } + obj, _ := cur.(map[string]any) + keys := []string{} + for k := range obj { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + +// withErrorAt adds to body an errors entry whose path names path. +func withErrorAt(t *testing.T, body, path string) string { + t.Helper() + entry, err := json.Marshal(map[string]any{"type": "NOT_FOUND", "message": "Could not resolve", + "path": errorPath(t, strings.TrimPrefix(path, "data."))}) + require.NoError(t, err) + return strings.TrimSuffix(body, "}") + `,"errors":[` + string(entry) + `]}` +} + +// errorPath turns "a.b[2].c" into GraphQL's ["a","b",2,"c"]. A leading +// "data" segment is kept, so callers strip it when building a reply path. +func errorPath(t *testing.T, path string) []any { + t.Helper() + out := []any{} + for _, part := range strings.Split(path, ".") { + name, rest, _ := strings.Cut(part, "[") + out = append(out, name) + for rest != "" { + idx, more, _ := strings.Cut(rest, "]") + i, err := strconv.Atoi(idx) + require.NoError(t, err) + out = append(out, i) + rest = strings.TrimPrefix(more, "[") + } + } + return out +} diff --git a/internal/github/pagination.go b/internal/github/pagination.go index cedf3f255..22f10f4f1 100644 --- a/internal/github/pagination.go +++ b/internal/github/pagination.go @@ -20,6 +20,11 @@ const ( restPageSize = 100 ) +// errPullRequestGone is a follow-up page that no longer finds the pull +// request its first page came from. Ending the drain there would drop the +// rest of its commits or reviews without saying so. +var errPullRequestGone = errors.New("the pull request is no longer readable") + // pageInfo is the cursor block shared by every paginated GraphQL connection. type pageInfo struct { HasNextPage graphql.Boolean @@ -142,13 +147,13 @@ func pageVariables(ref prRef, after graphql.String) map[string]any { // caller's first query already returned. Follow-up pages select only commits, // so draining one connection never re-fetches the other. func (c *GithubConfig) allPRCommits(ctx context.Context, run graphqlQueryFunc, ref prRef, - seed []graphqlCommitNode, first pageInfo) ([]graphqlCommitNode, error) { - commits, err := paginate(seed, first, defaultMaxPages, func(after graphql.String) ([]graphqlCommitNode, pageInfo, error) { + seed []*graphqlCommitNode, first pageInfo) ([]graphqlCommitNode, error) { + commits, err := paginate(seed, first, defaultMaxPages, func(after graphql.String) ([]*graphqlCommitNode, pageInfo, error) { var q struct { - Repository struct { - PullRequest struct { + Repository *struct { + PullRequest *struct { Commits struct { - Nodes []graphqlCommitNode + Nodes []*graphqlCommitNode PageInfo pageInfo } `graphql:"commits(first: 100, after: $cursor)"` } `graphql:"pullRequest(number: $prNumber)"` @@ -157,24 +162,27 @@ func (c *GithubConfig) allPRCommits(ctx context.Context, run graphqlQueryFunc, r if err := run(ctx, &q, pageVariables(ref, after)); err != nil { return nil, pageInfo{}, err } + if q.Repository == nil || q.Repository.PullRequest == nil { + return nil, pageInfo{}, errPullRequestGone + } page := q.Repository.PullRequest.Commits return page.Nodes, page.PageInfo, nil }) - if err != nil { - return nil, fmt.Errorf("draining commits for %s: %w", ref, err) + if err == nil { + return nonNull(commits, "commits") } - return commits, nil + return nil, fmt.Errorf("draining commits for %s: %w", ref, err) } // allPRReviews returns every approved review on prNumber, seeded as above. func (c *GithubConfig) allPRReviews(ctx context.Context, run graphqlQueryFunc, ref prRef, - seed []graphqlReviewNode, first pageInfo) ([]graphqlReviewNode, error) { - reviews, err := paginate(seed, first, defaultMaxPages, func(after graphql.String) ([]graphqlReviewNode, pageInfo, error) { + seed []*graphqlReviewNode, first pageInfo) ([]graphqlReviewNode, error) { + reviews, err := paginate(seed, first, defaultMaxPages, func(after graphql.String) ([]*graphqlReviewNode, pageInfo, error) { var q struct { - Repository struct { - PullRequest struct { + Repository *struct { + PullRequest *struct { Reviews struct { - Nodes []graphqlReviewNode + Nodes []*graphqlReviewNode PageInfo pageInfo } `graphql:"reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED], after: $cursor)"` } `graphql:"pullRequest(number: $prNumber)"` @@ -183,11 +191,14 @@ func (c *GithubConfig) allPRReviews(ctx context.Context, run graphqlQueryFunc, r if err := run(ctx, &q, pageVariables(ref, after)); err != nil { return nil, pageInfo{}, err } + if q.Repository == nil || q.Repository.PullRequest == nil { + return nil, pageInfo{}, errPullRequestGone + } page := q.Repository.PullRequest.Reviews return page.Nodes, page.PageInfo, nil }) - if err != nil { - return nil, fmt.Errorf("draining reviews for %s: %w", ref, err) + if err == nil { + return nonNull(reviews, "reviews") } - return reviews, nil + return nil, fmt.Errorf("draining reviews for %s: %w", ref, err) } diff --git a/internal/github/pr_pagination_test.go b/internal/github/pr_pagination_test.go index 2fbde1c56..5a8d1def5 100644 --- a/internal/github/pr_pagination_test.go +++ b/internal/github/pr_pagination_test.go @@ -566,16 +566,22 @@ func TestPREvidenceByPRNumber_LooksUpSignersInBatchesOf100(t *testing.T) { require.Equal(t, "grace", evidence.Commits[100].SignerUsername) } -func TestSignerLogins_LogsAnIgnoredErrorWhenDebugging(t *testing.T) { +func TestNullFieldErrorTransport_LogsADroppedErrorWhenDebugging(t *testing.T) { ts := newGraphQLTestServer(t, signersResponse(signedBy{sha: "sha1"})) - config := newPaginationConfig(ts) var debug strings.Builder - node := graphqlCommitNode{} - node.Commit.ID = "node-sha1" - node.Commit.Signature = &graphqlSignature{} - - _, err := config.signerLogins(context.Background(), graphql.NewClient(ts.URL+"/api/graphql", nil).Query, - []graphqlCommitNode{node}, &debug) + client := graphql.NewClient(ts.URL+"/api/graphql", &http.Client{ + Transport: &nullFieldErrorTransport{base: http.DefaultTransport, debug: &debug}, + }) + + var q struct { + Nodes []struct { + Oid graphql.String + Signature struct { + Signer *struct{ Login graphql.String } + } + } `graphql:"nodes(ids: $ids)"` + } + err := client.Query(context.Background(), &q, map[string]any{"ids": []graphql.ID{"node-sha1"}}) require.NoError(t, err) require.Contains(t, debug.String(), "[debug-github]") require.Contains(t, debug.String(), "Not Found") From 19a01a2bb8feeae663c188ba597b9e2ee7476843 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Wed, 21 Oct 2026 18:16:49 +0100 Subject: [PATCH 3/9] fix(github): keep GitHub's reason for a missing repository, tolerate signer errors Review of the previous commit found two regressions against main: - The transport dropped the error on a null root field. A SAML-blocked repo comes back as repository: null with a FORBIDDEN error, so the user saw "could not find repository" (or, by number, a blamed pull request) instead of GitHub's message saying what to fix. Errors within two segments of the root (repository, repository.pullRequest, repository.object, nodes[i]) now always pass through, as on main. - The signer lookup failed the attestation, after the full retry ladder, on a null node GitHub could not return by id. The signer is optional, so that lookup again accepts any error that comes with data and writes it to debug output, and skips null nodes. Adds unit tests for isNullField. The explicit list-index check is removed: a path ending in an index ends on a list, which the object check already rejects. Part of #1283 Co-Authored-By: Claude Opus 5.5 --- internal/github/github.go | 21 ++++++++++---- internal/github/graphql_client.go | 13 +++++---- internal/github/graphql_client_test.go | 33 ++++++++++++++++++++++ internal/github/odd_replies_test.go | 39 ++++++++++++++++++++++---- internal/github/pr_pagination_test.go | 35 +++++++++++++++++++++++ 5 files changed, 123 insertions(+), 18 deletions(-) create mode 100644 internal/github/graphql_client_test.go diff --git a/internal/github/github.go b/internal/github/github.go index 7e919087e..e93dd489b 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -292,6 +292,9 @@ 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, which newGraphQLClient drops. +// Unlike the other queries, any error that comes with data is accepted and +// written to debug output: a node asked for by id that comes back null, or +// with a field GitHub could not resolve, only loses its signer. func (c *GithubConfig) signerLogins(ctx context.Context, query graphqlQueryFunc, nodes []graphqlCommitNode) (map[string]string, error) { ids := []graphql.ID{} @@ -317,16 +320,22 @@ func (c *GithubConfig) signerLogins(ctx context.Context, query graphqlQueryFunc, var q signerQuery err := c.retry(ctx, func() error { q = signerQuery{} - return query(ctx, &q, map[string]any{"ids": batch}) + err := query(ctx, &q, map[string]any{"ids": batch}) + if err != nil && len(q.Nodes) > 0 { + if debug := c.debugOut(); 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) } - found, err := nonNull(q.Nodes, "signer nodes") - if err != nil { - return nil, err - } - for _, n := range found { + for _, n := range q.Nodes { + if n == nil { + continue + } if sig := n.Commit.Signature; sig != nil && sig.Signer != nil { signers[string(n.Commit.Oid)] = string(sig.Signer.Login) } diff --git a/internal/github/graphql_client.go b/internal/github/graphql_client.go index e058eb908..bd4996fa0 100644 --- a/internal/github/graphql_client.go +++ b/internal/github/graphql_client.go @@ -87,13 +87,12 @@ func (t *nullFieldErrorTransport) withoutNullFieldErrors(body []byte) []byte { // isNullField reports whether path names an object field that is null in // data while everything above it is not. An error anywhere else (a list item, -// a field under a null parent, no path) is more than one missing field. +// a field under a null parent, no path) is more than one missing field. So is +// one within two segments of the root, such as repository or +// repository.pullRequest: that is what the query is about, and GitHub's +// message (e.g. SAML enforcement) is the user's only clue to the cause. func isNullField(data any, path []any) bool { - if len(path) == 0 { - return false - } - field, ok := path[len(path)-1].(string) - if !ok { + if len(path) <= 2 { return false } cur := data @@ -115,10 +114,12 @@ func isNullField(data any, path []any) bool { return false } } + // A path ending in a list index ends on a list, not an object. obj, ok := cur.(map[string]any) if !ok { return false } + field, _ := path[len(path)-1].(string) value, present := obj[field] return present && value == nil } diff --git a/internal/github/graphql_client_test.go b/internal/github/graphql_client_test.go new file mode 100644 index 000000000..6b926b0d8 --- /dev/null +++ b/internal/github/graphql_client_test.go @@ -0,0 +1,33 @@ +package github + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestIsNullField(t *testing.T) { + for _, tc := range []struct { + name string + data string + path []any + want bool + }{ + {"a null field", `{"repository":{"pullRequest":{"mergedAt":null}}}`, + []any{"repository", "pullRequest", "mergedAt"}, true}, + {"a field with a value", `{"repository":{"pullRequest":{"title":"A PR"}}}`, + []any{"repository", "pullRequest", "title"}, false}, + {"a list item", `{"repository":{"pullRequest":{"commits":{"nodes":[{"x":null}]}}}}`, + []any{"repository", "pullRequest", "commits", "nodes", 0.0}, false}, + {"a root field", `{"repository":null}`, []any{"repository"}, false}, + {"a field two segments down", `{"repository":{"pullRequest":null}}`, + []any{"repository", "pullRequest"}, false}, + } { + t.Run(tc.name, func(t *testing.T) { + var data any + require.NoError(t, json.Unmarshal([]byte(tc.data), &data)) + require.Equal(t, tc.want, isNullField(data, tc.path)) + }) + } +} diff --git a/internal/github/odd_replies_test.go b/internal/github/odd_replies_test.go index 6aa3cc189..3782071d7 100644 --- a/internal/github/odd_replies_test.go +++ b/internal/github/odd_replies_test.go @@ -305,7 +305,8 @@ func requireRuleHolds(t *testing.T, reply, path, outcome string) { switch { case strings.HasSuffix(path, "[]"): require.NotEqual(t, "error", outcome, "an emptied list is not an error") - case strings.HasSuffix(path, "]"): + case strings.HasSuffix(path, "]") && reply != replySigners: + // A null signer node only loses its signer, which is optional. require.Equal(t, "error", outcome, "a null list item is missing data, not an optional field") case oddAllowedErrors[key] != "": require.Equal(t, "error", outcome, oddAllowedErrors[key]) @@ -353,16 +354,30 @@ func TestOddReplies_FieldErrorMeansTheSameAsNull(t *testing.T) { plain[reply] = string(v.Body) withError := oddBaselineReplies() withError[reply] = withErrorAt(t, string(v.Body), v.Path) - require.Equal(t, runOddReply(t, ep, plain, want), runOddReply(t, ep, withError, want)) + got := runOddReply(t, ep, withError, want) + if reply != replySigners && isRootField(t, v.Path) { + require.Equal(t, "error", got, "GitHub's reason for a missing root field must reach the user") + return + } + require.Equal(t, runOddReply(t, ep, plain, want), got) }) } } } } +// isRootField reports whether path is at most two segments below data, such +// as repository.pullRequest or nodes[0], whose errors are never dropped. +func isRootField(t *testing.T, path string) bool { + t.Helper() + return len(errorPath(t, strings.TrimPrefix(path, "data."))) <= 2 +} + // TestOddReplies_ErrorsThatAreNotANullFieldFail covers errors the client must // not drop: one on a field under a null parent (a non-null field's error nulls -// its nearest nullable parent), one with no path, and one on a list item. +// its nearest nullable parent), one with no path, and one on a list item. The +// signer lookup is the exception: any error that comes with data only loses +// signers. func TestOddReplies_ErrorsThatAreNotANullFieldFail(t *testing.T) { for _, ep := range oddEntryPoints { for _, reply := range ep.replies { @@ -377,24 +392,36 @@ func TestOddReplies_ErrorsThatAreNotANullFieldFail(t *testing.T) { t.Run(ep.name+"/"+reply+"/"+v.Path+"."+child, func(t *testing.T) { replies := oddBaselineReplies() replies[reply] = withErrorAt(t, string(v.Body), v.Path+"."+child) - require.Equal(t, "error", runOddReply(t, ep, replies, nil)) + requireFailsUnlessSigners(t, reply, v.Path, runOddReply(t, ep, replies, nil)) }) } } t.Run(ep.name+"/"+reply+"/no path", func(t *testing.T) { replies := oddBaselineReplies() replies[reply] = strings.TrimSuffix(baseline, "}") + `,"errors":[{"message":"Something went wrong"}]}` - require.Equal(t, "error", runOddReply(t, ep, replies, nil)) + requireFailsUnlessSigners(t, reply, "", runOddReply(t, ep, replies, nil)) }) } t.Run(ep.name+"/list item", func(t *testing.T) { replies := oddBaselineReplies() - replies[replySigners] = withErrorAt(t, `{"data":{"nodes":[null]}}`, "data.nodes[0]") + replies[replyCommitPage] = withErrorAt(t, strings.Replace(oddCommitPageReply, oddUnsignedCommit, "null", 1), + "data.repository.pullRequest.commits.nodes[0]") require.Equal(t, "error", runOddReply(t, ep, replies, nil)) }) } } +// requireFailsUnlessSigners expects an error, except from a signer lookup +// reply that still has data, as it does unless nulledPath is "data". +func requireFailsUnlessSigners(t *testing.T, reply, nulledPath, outcome string) { + t.Helper() + if reply == replySigners && nulledPath != "data" { + require.True(t, strings.HasPrefix(outcome, "ok"), "the signer lookup tolerates errors with data, got %q", outcome) + return + } + require.Equal(t, "error", outcome) +} + // keysAt names the fields of the object at path in doc, if it is one. func keysAt(t *testing.T, doc, path string) []string { t.Helper() diff --git a/internal/github/pr_pagination_test.go b/internal/github/pr_pagination_test.go index 5a8d1def5..1256f7495 100644 --- a/internal/github/pr_pagination_test.go +++ b/internal/github/pr_pagination_test.go @@ -586,3 +586,38 @@ func TestNullFieldErrorTransport_LogsADroppedErrorWhenDebugging(t *testing.T) { require.Contains(t, debug.String(), "[debug-github]") require.Contains(t, debug.String(), "Not Found") } + +// GitHub nulls a node it cannot return by id; that commit only loses its signer. +func TestPREvidenceByPRNumber_ToleratesANullSignerNode(t *testing.T) { + ts := newGraphQLTestServer(t, + byPRNumberResponse(connectionJSON([]string{signedCommitNodeJSON("sha1")}, ""), connectionJSON(nil, "")), + `{"data":{"nodes":[null]},"errors":[{"type":"NOT_FOUND","path":["nodes",0],"message":"Not Found"}]}`, + ) + + evidence, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) + require.NoError(t, err) + require.Equal(t, "", evidence.Commits[0].SignerUsername) + require.Len(t, ts.bodies, 2) +} + +// GitHub reports a SAML-protected repo as a null repository with a FORBIDDEN +// error; its message tells the user what to fix. +func TestPREvidence_KeepsGitHubsReasonForAMissingRepository(t *testing.T) { + const saml = "Resource protected by organization SAML enforcement. You must grant your Personal Access token access to this organization." + reply := `{"data":{"repository":null},"errors":[{"type":"FORBIDDEN","path":["repository"],"message":"` + saml + `"}]}` + for name, run := range map[string]func(*GithubConfig) error{ + "PREvidenceByPRNumber": func(c *GithubConfig) error { + _, err := c.PREvidenceByPRNumber(1) + return err + }, + "PREvidenceForCommitV2": func(c *GithubConfig) error { + _, err := c.PREvidenceForCommitV2("merge-sha") + return err + }, + } { + t.Run(name, func(t *testing.T) { + ts := newGraphQLTestServer(t, reply, reply, reply, reply) + require.ErrorContains(t, run(newPaginationConfig(ts)), saml) + }) + } +} From 95569e73b71bc286d67a995f9de8b7c08f8639c2 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 22 Oct 2026 17:17:00 +0100 Subject: [PATCH 4/9] fix(deps): build with Go 1.26.10 and pin the builder image by digest Snyk fails every branch on CVE-2026-111943, a high-severity flaw in Go's crypto/x509 fixed in 1.26.10; go.mod declared 1.26.7. The Dockerfile built from golang:1.26-alpine3.23, so each build could pull a different toolchain. It now names golang:1.26.10-alpine3.23 by digest. Dependabot can't update a FROM built from ARGs, so the line is literal, and a docker ecosystem entry (golang only) keeps the digest current. --- .github/dependabot.yml | 18 ++++++++++++++++++ Dockerfile | 6 ++---- go.mod | 2 +- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index d9578d29a..8e9a77e83 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -46,6 +46,24 @@ updates: labels: - "dependencies" - "github-actions" + # The Go builder image in Dockerfile, pinned by digest + - package-ecosystem: "docker" + directory: "/" + schedule: + interval: "weekly" + day: "tuesday" + time: "11:00" + commit-message: + prefix: 'chore(deps)' + allow: + - dependency-name: "golang" + # Minor bumps change the Go the image builds with; go.mod moves first. + ignore: + - dependency-name: "golang" + update-types: ["version-update:semver-major", "version-update:semver-minor"] + labels: + - "dependencies" + - "docker" # Docker Compose images - package-ecosystem: "docker-compose" directory: "/" diff --git a/Dockerfile b/Dockerfile index 83fa1cb44..4ddecbc0f 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,15 +1,13 @@ # syntax=docker/dockerfile:1 -ARG GO_VERSION="1.26" -ARG ALPINE_VERSION="3.23" - # The version the binary reports; empty builds dev+. Passed in rather than # read from git tags, so the image cannot depend on the checkout's tags (#1133). ARG VERSION="" ### Go Builder ### -FROM golang:${GO_VERSION}-alpine${ALPINE_VERSION} AS builder +# Pinned by digest so a rebuild gets the same toolchain; Dependabot moves it. +FROM golang:1.26.10-alpine3.23@sha256:107301490755aa2ea1b0b43788b6e62cff2faae45a538028b193becc06aaffba AS builder RUN apk add --update --no-cache git bash make ca-certificates diff --git a/go.mod b/go.mod index a79fd4797..a9ebbb66c 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/kosli-dev/cli -go 1.26.7 +go 1.26.10 require ( cloud.google.com/go/asset v1.29.0 From 6aa0174778c1b3900b524554c2d6c10383ced137 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 22 Oct 2026 17:25:38 +0100 Subject: [PATCH 5/9] fix(github): keep errors on null lists, fail unattributed approvals A null list or connection (reviews, commits.nodes, associatedPullRequests) decodes as an empty list. Dropping GitHub's error for it recorded zero reviews, commits or pull requests with no failure, which could hide a changes-requested review or an unsigned commit. The transport now keeps errors on the list fields the queries select. An approving review with no author was recorded as an approval by nobody, which fails open. Both the GraphQL and the REST approvals path now error, naming the review's index. The transport also sets ContentLength and drops the Content-Length header after rewriting the body, so nothing reads the old size. Part of #1283 Co-Authored-By: Claude Opus 5.5 --- internal/github/github.go | 21 +++++++- internal/github/graphql_client.go | 22 ++++++-- internal/github/graphql_client_test.go | 34 ++++++++++++ internal/github/odd_replies_test.go | 71 +++++++++++++++++++++++-- internal/github/rest_pagination_test.go | 10 ++++ 5 files changed, 148 insertions(+), 10 deletions(-) diff --git a/internal/github/github.go b/internal/github/github.go index e93dd489b..52ba53fa6 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -442,7 +442,10 @@ func buildPREvidence( }) } - for _, r := range reviewNodes { + for i, r := range reviewNodes { + if err := unattributedApproval(i, string(r.State), string(r.Author.Login)); err != nil { + return nil, err + } submittedAt, err := unixTime(string(r.SubmittedAt)) if err != nil { return nil, err @@ -462,6 +465,15 @@ func buildPREvidence( return evidence, nil } +// unattributedApproval fails reviews[i] when it approves with no author: +// counting it would record an approval by nobody. +func unattributedApproval(i int, state, login string) error { + if state == types.ReviewApproved && login == "" { + return fmt.Errorf("reviews[%d] is an approval with no author in the GitHub reply", i) + } + return nil +} + // unixTime parses a GitHub timestamp. A null one reads as "" and gives 0. func unixTime(s string) (int64, error) { if s == "" { @@ -779,6 +791,7 @@ func (c *GithubConfig) listApprovers(ctx context.Context, client *gh.Client, // A reviewer can approve more than once. These entries carry no timestamp, // so a repeated login adds nothing and is dropped. seen := map[string]bool{} + index := 0 for pages := 0; pages < maxPages; pages++ { reviews, resp, err := client.PullRequests.ListReviews(ctx, c.Org, c.Repository, number, opts) if err != nil { @@ -786,7 +799,11 @@ func (c *GithubConfig) listApprovers(ctx context.Context, client *gh.Client, } for _, r := range reviews { login := r.GetUser().GetLogin() - if r.GetState() == "APPROVED" && !seen[login] { + if err := unattributedApproval(index, r.GetState(), login); err != nil { + return nil, err + } + index++ + if r.GetState() == types.ReviewApproved && !seen[login] { seen[login] = true approvers = append(approvers, login) } diff --git a/internal/github/graphql_client.go b/internal/github/graphql_client.go index bd4996fa0..6365a2777 100644 --- a/internal/github/graphql_client.go +++ b/internal/github/graphql_client.go @@ -25,9 +25,9 @@ func (c *GithubConfig) newGraphQLClient(ctx context.Context) (*graphql.Client, e } // nullFieldErrorTransport drops each GraphQL error that only explains why one -// field came back null, so a null field reads the same with or without an -// error. The client returns any error at all as a failure and drops its path, -// so this is the only place the path can still be checked. +// field came back null, so a null field that is not a list reads the same +// with or without an error. The client returns any error at all as a failure +// and drops its path, so this is the only place the path can still be checked. type nullFieldErrorTransport struct { base http.RoundTripper debug io.Writer @@ -43,7 +43,10 @@ func (t *nullFieldErrorTransport) RoundTrip(req *http.Request) (*http.Response, if err != nil { return nil, err } - resp.Body = io.NopCloser(bytes.NewReader(t.withoutNullFieldErrors(body))) + body = t.withoutNullFieldErrors(body) + resp.Body = io.NopCloser(bytes.NewReader(body)) + resp.ContentLength = int64(len(body)) + resp.Header.Del("Content-Length") return resp, nil } @@ -85,16 +88,26 @@ func (t *nullFieldErrorTransport) withoutNullFieldErrors(body []byte) []byte { return out } +// listFields are the list and connection fields the evidence queries select. +// A null one decodes as an empty list, so its error is the only sign entries +// are missing. +var listFields = map[string]bool{"nodes": true, "commits": true, "reviews": true, "associatedPullRequests": true} + // isNullField reports whether path names an object field that is null in // data while everything above it is not. An error anywhere else (a list item, // a field under a null parent, no path) is more than one missing field. So is // one within two segments of the root, such as repository or // repository.pullRequest: that is what the query is about, and GitHub's // message (e.g. SAML enforcement) is the user's only clue to the cause. +// A field in listFields never counts. func isNullField(data any, path []any) bool { if len(path) <= 2 { return false } + field, _ := path[len(path)-1].(string) + if listFields[field] { + return false + } cur := data for _, seg := range path[:len(path)-1] { switch s := seg.(type) { @@ -119,7 +132,6 @@ func isNullField(data any, path []any) bool { if !ok { return false } - field, _ := path[len(path)-1].(string) value, present := obj[field] return present && value == nil } diff --git a/internal/github/graphql_client_test.go b/internal/github/graphql_client_test.go index 6b926b0d8..23afc8755 100644 --- a/internal/github/graphql_client_test.go +++ b/internal/github/graphql_client_test.go @@ -2,6 +2,10 @@ package github import ( "encoding/json" + "io" + "net/http" + "strconv" + "strings" "testing" "github.com/stretchr/testify/require" @@ -23,6 +27,14 @@ func TestIsNullField(t *testing.T) { {"a root field", `{"repository":null}`, []any{"repository"}, false}, {"a field two segments down", `{"repository":{"pullRequest":null}}`, []any{"repository", "pullRequest"}, false}, + {"a connection", `{"repository":{"pullRequest":{"reviews":null}}}`, + []any{"repository", "pullRequest", "reviews"}, false}, + {"a non-null connection", `{"repository":{"pullRequest":{"commits":null}}}`, + []any{"repository", "pullRequest", "commits"}, false}, + {"a connection's nodes", `{"repository":{"pullRequest":{"commits":{"nodes":null}}}}`, + []any{"repository", "pullRequest", "commits", "nodes"}, false}, + {"a commit's pull requests", `{"repository":{"object":{"associatedPullRequests":null}}}`, + []any{"repository", "object", "associatedPullRequests"}, false}, } { t.Run(tc.name, func(t *testing.T) { var data any @@ -31,3 +43,25 @@ func TestIsNullField(t *testing.T) { }) } } + +type replyTransport string + +func (r replyTransport) RoundTrip(*http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusOK, ContentLength: int64(len(r)), + Header: http.Header{"Content-Length": {strconv.Itoa(len(r))}}, + Body: io.NopCloser(strings.NewReader(string(r)))}, nil +} + +// TestNullFieldErrorTransport_LengthMatchesTheNewBody stops a reader that +// trusts the length from reading the old body's size. +func TestNullFieldErrorTransport_LengthMatchesTheNewBody(t *testing.T) { + reply := `{"data":{"repository":{"pullRequest":{"mergedAt":null}}},` + + `"errors":[{"message":"x","path":["repository","pullRequest","mergedAt"]}]}` + resp, err := (&nullFieldErrorTransport{base: replyTransport(reply)}).RoundTrip(&http.Request{}) + require.NoError(t, err) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Less(t, len(body), len(reply), "the error should have been dropped") + require.Equal(t, int64(len(body)), resp.ContentLength) + require.Empty(t, resp.Header.Get("Content-Length")) +} diff --git a/internal/github/odd_replies_test.go b/internal/github/odd_replies_test.go index 3782071d7..b2e50ba50 100644 --- a/internal/github/odd_replies_test.go +++ b/internal/github/odd_replies_test.go @@ -310,6 +310,8 @@ func requireRuleHolds(t *testing.T, reply, path, outcome string) { require.Equal(t, "error", outcome, "a null list item is missing data, not an optional field") case oddAllowedErrors[key] != "": require.Equal(t, "error", outcome, oddAllowedErrors[key]) + case isApprovalAuthor(t, reply, path): + require.Equal(t, "error", outcome, "an approval with no author would count as approval by nobody") case oddWholePRLost[key]: require.Equal(t, "ok lost:PullRequests(1->0)", outcome) default: @@ -359,6 +361,10 @@ func TestOddReplies_FieldErrorMeansTheSameAsNull(t *testing.T) { require.Equal(t, "error", got, "GitHub's reason for a missing root field must reach the user") return } + if reply != replySigners && holdsList(t, oddBaselineReplies()[reply], v.Path) { + require.Equal(t, "error", got, "a list GitHub could not give would read as empty") + return + } require.Equal(t, runOddReply(t, ep, plain, want), got) }) } @@ -422,8 +428,61 @@ func requireFailsUnlessSigners(t *testing.T, reply, nulledPath, outcome string) require.Equal(t, "error", outcome) } -// keysAt names the fields of the object at path in doc, if it is one. -func keysAt(t *testing.T, doc, path string) []string { +// TestOddReplies_ApprovalWithNoAuthorNamesTheReview checks the error says +// which review it could not attribute. +func TestOddReplies_ApprovalWithNoAuthorNamesTheReview(t *testing.T) { + for _, ep := range oddEntryPoints { + t.Run(ep.name, func(t *testing.T) { + replies := oddBaselineReplies() + noAuthor := strings.Replace(oddApproval, `{"login":"grace"}`, "null", 1) + for _, name := range []string{replyByNumber, replyForCommit} { + replies[name] = strings.Replace(replies[name], oddApproval, noAuthor, 1) + } + ts := newOddReplyServer(t, replies) + _, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", + Repository: "test-repo", Sleep: func(time.Duration) {}}) + require.ErrorContains(t, err, "reviews[0]") + }) + } +} + +// isApprovalAuthor reports whether path is the author of an approving review +// in the named baseline reply. +func isApprovalAuthor(t *testing.T, reply, path string) bool { + t.Helper() + parent, field, _ := cutLast(path) + if field != "author" || !strings.Contains(parent, ".reviews.nodes[") { + return false + } + review, _ := valueAt(t, oddBaselineReplies()[reply], parent).(map[string]any) + return review["state"] == types.ReviewApproved +} + +// holdsList reports whether path in doc is a list or a connection (an object +// holding a nodes list). +func holdsList(t *testing.T, doc, path string) bool { + t.Helper() + switch v := valueAt(t, doc, path).(type) { + case []any: + return true + case map[string]any: + _, ok := v["nodes"] + return ok + } + return false +} + +// cutLast splits "a.b.c" into "a.b" and "c". +func cutLast(path string) (string, string, bool) { + i := strings.LastIndex(path, ".") + if i < 0 { + return "", path, false + } + return path[:i], path[i+1:], true +} + +// valueAt returns the value at path in doc. +func valueAt(t *testing.T, doc, path string) any { t.Helper() var cur any require.NoError(t, json.Unmarshal([]byte(doc), &cur)) @@ -435,7 +494,13 @@ func keysAt(t *testing.T, doc, path string) []string { cur = cur.([]any)[s] } } - obj, _ := cur.(map[string]any) + return cur +} + +// keysAt names the fields of the object at path in doc, if it is one. +func keysAt(t *testing.T, doc, path string) []string { + t.Helper() + obj, _ := valueAt(t, doc, path).(map[string]any) keys := []string{} for k := range obj { keys = append(keys, k) diff --git a/internal/github/rest_pagination_test.go b/internal/github/rest_pagination_test.go index be944a3e9..26bbba4b1 100644 --- a/internal/github/rest_pagination_test.go +++ b/internal/github/rest_pagination_test.go @@ -77,6 +77,16 @@ func TestGetPullRequestApprovers_FindsApproverOnSecondPage(t *testing.T) { require.Contains(t, ts.queries[0], "per_page=100") } +func TestGetPullRequestApprovers_ApprovalWithNoUserFails(t *testing.T) { + ts := newRestTestServer(t, "/api/v3/repos/test-org/test-repo/pulls/5/reviews", + `[{"state":"COMMENTED","user":null}]`, + `[{"state":"APPROVED","user":null}]`, + ) + + _, err := newRestConfig(ts.URL).GetPullRequestApprovers(5) + require.ErrorContains(t, err, "reviews[1]") +} + func TestPullRequestsForCommit_ReturnsErrorFromSecondPage(t *testing.T) { calls := 0 ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From bba2e6006e8175c1b12e9d72962a478e2b2dc4f8 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 22 Oct 2026 17:28:32 +0100 Subject: [PATCH 6/9] test: fail when the Dockerfile builds with a different Go than go.mod Dependabot bumps the pinned golang image on its own, and none of the gomod updates touch go.mod's go directive. Snyk reads go.mod, so an image-only bump would leave the next Go stdlib CVE flagged even though the image is fixed. The test makes that bump red until go.mod moves too. --- .github/dependabot.yml | 2 +- internal/version/toolchain_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 internal/version/toolchain_test.go diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 8e9a77e83..8c2a8e9c9 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -57,7 +57,7 @@ updates: prefix: 'chore(deps)' allow: - dependency-name: "golang" - # Minor bumps change the Go the image builds with; go.mod moves first. + # Only patch bumps; TestDockerfileBuildsWithTheGoInGoMod fails until go.mod moves too. ignore: - dependency-name: "golang" update-types: ["version-update:semver-major", "version-update:semver-minor"] diff --git a/internal/version/toolchain_test.go b/internal/version/toolchain_test.go new file mode 100644 index 000000000..6f4dffd44 --- /dev/null +++ b/internal/version/toolchain_test.go @@ -0,0 +1,26 @@ +package version + +import ( + "os" + "regexp" + "testing" + + "github.com/stretchr/testify/require" +) + +// Dependabot bumps the Dockerfile's golang image on its own, but Snyk reads +// the Go version from go.mod, so a bump that skips go.mod hides a stdlib CVE. +func TestDockerfileBuildsWithTheGoInGoMod(t *testing.T) { + goMod, err := os.ReadFile("../../go.mod") + require.NoError(t, err) + dockerfile, err := os.ReadFile("../../Dockerfile") + require.NoError(t, err) + + want := regexp.MustCompile(`(?m)^go (\S+)$`).FindSubmatch(goMod) + require.NotNil(t, want, "no go directive in go.mod") + got := regexp.MustCompile(`(?m)^FROM golang:([0-9.]+)-`).FindSubmatch(dockerfile) + require.NotNil(t, got, "no golang builder image in Dockerfile") + + require.Equal(t, string(want[1]), string(got[1]), + "Dockerfile builds with a different Go than go.mod declares; bump both together") +} From 5fea1253471cf9681d63e3113c24882192029add Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 22 Oct 2026 17:30:02 +0100 Subject: [PATCH 7/9] fix(github): keep a deleted account's review, never count it as approval GitHub sends a deleted account as a null author (GraphQL), a null user (REST reviews) or the "ghost" login. Failing on it, as the previous commit did, would block attestation for good on any PR once approved by someone who has since left. The review is now kept with an empty username and left out of the approvers, on both the GraphQL and REST paths, so it never counts as an approval and never blocks. Part of #1283 Co-Authored-By: Claude Opus 5.5 --- internal/github/github.go | 36 +++++++++++----------- internal/github/odd_replies_test.go | 40 +++++++++++++++---------- internal/github/rest_pagination_test.go | 10 ++++--- 3 files changed, 47 insertions(+), 39 deletions(-) diff --git a/internal/github/github.go b/internal/github/github.go index 52ba53fa6..52db9c7c0 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -442,16 +442,15 @@ func buildPREvidence( }) } - for i, r := range reviewNodes { - if err := unattributedApproval(i, string(r.State), string(r.Author.Login)); err != nil { - return nil, err - } + counted := []types.PRApprovals{} + for _, r := range reviewNodes { submittedAt, err := unixTime(string(r.SubmittedAt)) if err != nil { return nil, err } + login, known := reviewer(string(r.Author.Login)) review := types.PRApprovals{ - Username: string(r.Author.Login), + Username: login, State: string(r.State), Timestamp: submittedAt, } @@ -459,19 +458,23 @@ func buildPREvidence( review.CommitSHA = string(r.Commit.Oid) } evidence.Reviews = append(evidence.Reviews, review) + if known { + counted = append(counted, review) + } } - evidence.Approvers = types.ApprovalsFrom(evidence.Reviews) + evidence.Approvers = types.ApprovalsFrom(counted) return evidence, nil } -// unattributedApproval fails reviews[i] when it approves with no author: -// counting it would record an approval by nobody. -func unattributedApproval(i int, state, login string) error { - if state == types.ReviewApproved && login == "" { - return fmt.Errorf("reviews[%d] is an approval with no author in the GitHub reply", i) +// reviewer returns a review's login and whether an account stands behind it. +// GitHub sends a deleted account as a null author or as the "ghost" login; +// its review is kept, but it must never count as an approval. +func reviewer(login string) (string, bool) { + if login == "ghost" { + login = "" } - return nil + return login, login != "" } // unixTime parses a GitHub timestamp. A null one reads as "" and gives 0. @@ -791,19 +794,14 @@ func (c *GithubConfig) listApprovers(ctx context.Context, client *gh.Client, // A reviewer can approve more than once. These entries carry no timestamp, // so a repeated login adds nothing and is dropped. seen := map[string]bool{} - index := 0 for pages := 0; pages < maxPages; pages++ { reviews, resp, err := client.PullRequests.ListReviews(ctx, c.Org, c.Repository, number, opts) if err != nil { return nil, err } for _, r := range reviews { - login := r.GetUser().GetLogin() - if err := unattributedApproval(index, r.GetState(), login); err != nil { - return nil, err - } - index++ - if r.GetState() == types.ReviewApproved && !seen[login] { + login, known := reviewer(r.GetUser().GetLogin()) + if r.GetState() == types.ReviewApproved && known && !seen[login] { seen[login] = true approvers = append(approvers, login) } diff --git a/internal/github/odd_replies_test.go b/internal/github/odd_replies_test.go index b2e50ba50..848f08887 100644 --- a/internal/github/odd_replies_test.go +++ b/internal/github/odd_replies_test.go @@ -311,7 +311,9 @@ func requireRuleHolds(t *testing.T, reply, path, outcome string) { case oddAllowedErrors[key] != "": require.Equal(t, "error", outcome, oddAllowedErrors[key]) case isApprovalAuthor(t, reply, path): - require.Equal(t, "error", outcome, "an approval with no author would count as approval by nobody") + // A deleted account's approval is kept as a review but not counted. + require.True(t, strings.HasPrefix(outcome, "ok"), "a review by a deleted account is legal, got %q", outcome) + require.Contains(t, outcome, ":approvers", "an approval by nobody must not count") case oddWholePRLost[key]: require.Equal(t, "ok lost:PullRequests(1->0)", outcome) default: @@ -428,21 +430,27 @@ func requireFailsUnlessSigners(t *testing.T, reply, nulledPath, outcome string) require.Equal(t, "error", outcome) } -// TestOddReplies_ApprovalWithNoAuthorNamesTheReview checks the error says -// which review it could not attribute. -func TestOddReplies_ApprovalWithNoAuthorNamesTheReview(t *testing.T) { - for _, ep := range oddEntryPoints { - t.Run(ep.name, func(t *testing.T) { - replies := oddBaselineReplies() - noAuthor := strings.Replace(oddApproval, `{"login":"grace"}`, "null", 1) - for _, name := range []string{replyByNumber, replyForCommit} { - replies[name] = strings.Replace(replies[name], oddApproval, noAuthor, 1) - } - ts := newOddReplyServer(t, replies) - _, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", - Repository: "test-repo", Sleep: func(time.Duration) {}}) - require.ErrorContains(t, err, "reviews[0]") - }) +// TestOddReplies_ApprovalWithNoAuthorIsNotCounted covers a deleted account: +// GraphQL sends a null author, and "ghost" is GitHub's stand-in login for one. +func TestOddReplies_ApprovalWithNoAuthorIsNotCounted(t *testing.T) { + for _, author := range []string{"null", `{"login":"ghost"}`} { + for _, ep := range oddEntryPoints { + t.Run(ep.name+"/"+author, func(t *testing.T) { + replies := oddBaselineReplies() + noAuthor := strings.Replace(oddApproval, `{"login":"grace"}`, author, 1) + for _, name := range []string{replyByNumber, replyForCommit} { + replies[name] = strings.Replace(replies[name], oddApproval, noAuthor, 1) + } + ts := newOddReplyServer(t, replies) + e, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", + Repository: "test-repo", Sleep: func(time.Duration) {}}) + require.NoError(t, err) + require.Empty(t, e.Approvers) + require.Len(t, e.Reviews, 2) + require.Equal(t, "", e.Reviews[0].Username) + require.Equal(t, types.ReviewApproved, e.Reviews[0].State) + }) + } } } diff --git a/internal/github/rest_pagination_test.go b/internal/github/rest_pagination_test.go index 26bbba4b1..aa6a5a988 100644 --- a/internal/github/rest_pagination_test.go +++ b/internal/github/rest_pagination_test.go @@ -77,14 +77,16 @@ func TestGetPullRequestApprovers_FindsApproverOnSecondPage(t *testing.T) { require.Contains(t, ts.queries[0], "per_page=100") } -func TestGetPullRequestApprovers_ApprovalWithNoUserFails(t *testing.T) { +func TestGetPullRequestApprovers_DeletedAccountIsNotCounted(t *testing.T) { + // REST sends a deleted reviewer as user null, and "ghost" elsewhere. ts := newRestTestServer(t, "/api/v3/repos/test-org/test-repo/pulls/5/reviews", - `[{"state":"COMMENTED","user":null}]`, `[{"state":"APPROVED","user":null}]`, + `[{"state":"APPROVED","user":{"login":"ghost"}},{"state":"APPROVED","user":{"login":"ada"}}]`, ) - _, err := newRestConfig(ts.URL).GetPullRequestApprovers(5) - require.ErrorContains(t, err, "reviews[1]") + approvers, err := newRestConfig(ts.URL).GetPullRequestApprovers(5) + require.NoError(t, err) + require.Equal(t, []string{"ada"}, approvers) } func TestPullRequestsForCommit_ReturnsErrorFromSecondPage(t *testing.T) { From 2bde6043f42f1263abdf163bdab70695ec4ee7df Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Sun, 25 Oct 2026 14:00:32 +0000 Subject: [PATCH 8/9] fix(github): keep an unnamed review only as a change request Policies read Reviews, not just Approvers. Keeping a deleted account's approval in Reviews with state APPROVED let a policy that counts approvals there count one by nobody. A review with no account (null author, null REST user, or the "ghost" login) is now kept only when it requests changes, since dropping that would read as none given. An unnamed approval, comment or dismissal is left out of both lists. This matches the Bitbucket rule on main. Part of #1283 Co-Authored-By: Claude Opus 5.5 --- internal/github/build_pr_evidence_test.go | 14 +++++++++ internal/github/github.go | 26 ++++++++-------- internal/github/odd_replies_test.go | 37 ++++++++++++++++------- 3 files changed, 53 insertions(+), 24 deletions(-) diff --git a/internal/github/build_pr_evidence_test.go b/internal/github/build_pr_evidence_test.go index f3caaff66..4c7c44334 100644 --- a/internal/github/build_pr_evidence_test.go +++ b/internal/github/build_pr_evidence_test.go @@ -220,6 +220,20 @@ func reviewNode(login, state, submittedAt, commit string) graphqlReviewNode { return r } +func TestBuildPREvidence_UnnamedReviewIsKeptOnlyAsAChangeRequest(t *testing.T) { + evidence, err := buildPREvidence("u", "m", "MERGED", "tooky", "2026-03-01T09:00:00Z", "", + "title", "feature", "main", "h", nil, + []graphqlReviewNode{ + reviewNode("", "COMMENTED", "2026-03-01T13:00:00Z", ""), + reviewNode("ghost", "DISMISSED", "2026-03-01T13:00:00Z", ""), + reviewNode("", "APPROVED", "2026-03-01T13:00:00Z", ""), + reviewNode("ghost", "CHANGES_REQUESTED", "2026-03-01T13:00:00Z", ""), + }, nil) + require.NoError(t, err) + require.Equal(t, []types.PRApprovals{{State: "CHANGES_REQUESTED", Timestamp: 1772370000}}, evidence.Reviews) + require.Empty(t, evidence.Approvers) +} + func TestBuildPREvidence_RecordsWhichCommitAnApprovalCovered(t *testing.T) { evidence, err := buildPREvidence( "https://github.com/kosli-dev/cli/pull/671", diff --git a/internal/github/github.go b/internal/github/github.go index 52db9c7c0..0651a8820 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -442,13 +442,15 @@ func buildPREvidence( }) } - counted := []types.PRApprovals{} for _, r := range reviewNodes { + login, keep := reviewer(string(r.Author.Login), string(r.State)) + if !keep { + continue + } submittedAt, err := unixTime(string(r.SubmittedAt)) if err != nil { return nil, err } - login, known := reviewer(string(r.Author.Login)) review := types.PRApprovals{ Username: login, State: string(r.State), @@ -458,23 +460,21 @@ func buildPREvidence( review.CommitSHA = string(r.Commit.Oid) } evidence.Reviews = append(evidence.Reviews, review) - if known { - counted = append(counted, review) - } } - evidence.Approvers = types.ApprovalsFrom(counted) + evidence.Approvers = types.ApprovalsFrom(evidence.Reviews) return evidence, nil } -// reviewer returns a review's login and whether an account stands behind it. -// GitHub sends a deleted account as a null author or as the "ghost" login; -// its review is kept, but it must never count as an approval. -func reviewer(login string) (string, bool) { +// reviewer returns a review's login and whether to keep the review. GitHub +// sends a deleted account as a null author or as the "ghost" login. Policies +// read reviews as well as approvers, so a review with no account is kept only +// as a change request: dropping that would read as none given. +func reviewer(login, state string) (string, bool) { if login == "ghost" { login = "" } - return login, login != "" + return login, login != "" || state == types.ReviewChangesRequested } // unixTime parses a GitHub timestamp. A null one reads as "" and gives 0. @@ -800,8 +800,8 @@ func (c *GithubConfig) listApprovers(ctx context.Context, client *gh.Client, return nil, err } for _, r := range reviews { - login, known := reviewer(r.GetUser().GetLogin()) - if r.GetState() == types.ReviewApproved && known && !seen[login] { + login, keep := reviewer(r.GetUser().GetLogin(), r.GetState()) + if r.GetState() == types.ReviewApproved && keep && !seen[login] { seen[login] = true approvers = append(approvers, login) } diff --git a/internal/github/odd_replies_test.go b/internal/github/odd_replies_test.go index 848f08887..354af89f3 100644 --- a/internal/github/odd_replies_test.go +++ b/internal/github/odd_replies_test.go @@ -311,7 +311,7 @@ func requireRuleHolds(t *testing.T, reply, path, outcome string) { case oddAllowedErrors[key] != "": require.Equal(t, "error", outcome, oddAllowedErrors[key]) case isApprovalAuthor(t, reply, path): - // A deleted account's approval is kept as a review but not counted. + // A deleted account's approval is left out of reviews and approvers. require.True(t, strings.HasPrefix(outcome, "ok"), "a review by a deleted account is legal, got %q", outcome) require.Contains(t, outcome, ":approvers", "an approval by nobody must not count") case oddWholePRLost[key]: @@ -430,30 +430,45 @@ func requireFailsUnlessSigners(t *testing.T, reply, nulledPath, outcome string) require.Equal(t, "error", outcome) } -// TestOddReplies_ApprovalWithNoAuthorIsNotCounted covers a deleted account: -// GraphQL sends a null author, and "ghost" is GitHub's stand-in login for one. -func TestOddReplies_ApprovalWithNoAuthorIsNotCounted(t *testing.T) { +// TestOddReplies_ReviewWithNoAuthor covers a deleted account: GraphQL sends a +// null author, and "ghost" is GitHub's stand-in login for one. Policies read +// Reviews, so only a change request is kept there without a name. +func TestOddReplies_ReviewWithNoAuthor(t *testing.T) { for _, author := range []string{"null", `{"login":"ghost"}`} { for _, ep := range oddEntryPoints { - t.Run(ep.name+"/"+author, func(t *testing.T) { + t.Run(ep.name+"/approval/"+author, func(t *testing.T) { replies := oddBaselineReplies() noAuthor := strings.Replace(oddApproval, `{"login":"grace"}`, author, 1) for _, name := range []string{replyByNumber, replyForCommit} { replies[name] = strings.Replace(replies[name], oddApproval, noAuthor, 1) } - ts := newOddReplyServer(t, replies) - e, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", - Repository: "test-repo", Sleep: func(time.Duration) {}}) - require.NoError(t, err) + e := runReviewReplies(t, ep, replies) require.Empty(t, e.Approvers) + require.Len(t, e.Reviews, 1) + require.Equal(t, types.ReviewChangesRequested, e.Reviews[0].State) + }) + t.Run(ep.name+"/change request/"+author, func(t *testing.T) { + replies := oddBaselineReplies() + replies[replyReviewPage] = strings.Replace(replies[replyReviewPage], `{"login":"linus"}`, author, 1) + e := runReviewReplies(t, ep, replies) + require.Len(t, e.Approvers, 1) require.Len(t, e.Reviews, 2) - require.Equal(t, "", e.Reviews[0].Username) - require.Equal(t, types.ReviewApproved, e.Reviews[0].State) + require.Equal(t, "", e.Reviews[1].Username) + require.Equal(t, types.ReviewChangesRequested, e.Reviews[1].State) }) } } } +func runReviewReplies(t *testing.T, ep oddEntryPoint, replies map[string]string) *types.PREvidence { + t.Helper() + ts := newOddReplyServer(t, replies) + e, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", + Repository: "test-repo", Sleep: func(time.Duration) {}}) + require.NoError(t, err) + return e +} + // isApprovalAuthor reports whether path is the author of an approving review // in the named baseline reply. func isApprovalAuthor(t *testing.T, reply, path string) bool { From 6d77d195aca3f67085c4a19c46857630fce2d9c4 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Sun, 25 Oct 2026 19:47:41 +0000 Subject: [PATCH 9/9] fix(github): name the owner in null-item errors; empty missing authors An error for a null list item named only the list and index, such as commits[0], so a user couldn't tell which pull request or commit it was about. It now says what the list belongs to: the commit for its pull requests, or the pull request (owner/repo#n) for its commits and reviews. A commit with no author name and no email was recorded as " <>". It's now "". One part alone keeps the "Name " format. Part of #1283 Co-Authored-By: Claude Opus 5.5 --- internal/github/github.go | 16 ++++--- internal/github/odd_replies_test.go | 70 +++++++++++++++++++++++++++++ internal/github/pagination.go | 4 +- 3 files changed, 83 insertions(+), 7 deletions(-) diff --git a/internal/github/github.go b/internal/github/github.go index 0651a8820..1c7b58d46 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -427,10 +427,15 @@ func buildPREvidence( signatureState = &s signedByPlatform = &p } + // A commit with no author is recorded as "", not " <>". + author := "" + if n.Commit.Author.Name != "" || n.Commit.Author.Email != "" { + author = fmt.Sprintf("%s <%s>", string(n.Commit.Author.Name), string(n.Commit.Author.Email)) + } evidence.Commits = append(evidence.Commits, types.Commit{ SHA: string(n.Commit.Oid), Message: string(n.Commit.MessageHeadline), - Author: fmt.Sprintf("%s <%s>", string(n.Commit.Author.Name), string(n.Commit.Author.Email)), + Author: author, AuthorUsername: authorUsername, Timestamp: timestamp, Branch: headRef, @@ -490,12 +495,13 @@ func unixTime(s string) (int64, error) { } // nonNull returns items as values. A null item is an entry the reply could -// not give, so it fails rather than being skipped. -func nonNull[T any](items []*T, list string) ([]T, error) { +// not give, so it fails rather than being skipped. owner says what the list +// belongs to. +func nonNull[T any](items []*T, list, owner string) ([]T, error) { out := make([]T, 0, len(items)) for i, item := range items { if item == nil { - return nil, fmt.Errorf("%s[%d] is null in the GitHub reply", list, i) + return nil, fmt.Errorf("%s[%d] for %s is null in the GitHub reply", list, i, owner) } out = append(out, *item) } @@ -656,7 +662,7 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence if query.Repository == nil { return pullRequestsEvidence, fmt.Errorf("could not find repository %s/%s", c.Org, c.Repository) } - prs, err := nonNull(query.Repository.Object.Commit.AssociatedPullRequests.Nodes, "associatedPullRequests") + prs, err := nonNull(query.Repository.Object.Commit.AssociatedPullRequests.Nodes, "associatedPullRequests", "commit "+commit) if err != nil { return pullRequestsEvidence, err } diff --git a/internal/github/odd_replies_test.go b/internal/github/odd_replies_test.go index 354af89f3..075de0f29 100644 --- a/internal/github/odd_replies_test.go +++ b/internal/github/odd_replies_test.go @@ -559,3 +559,73 @@ func errorPath(t *testing.T, path string) []any { } return out } + +// A null list item's error says what the list belongs to: the commit for its +// pull requests, or the pull request for its commits and reviews. +func TestOddReplies_NullItemErrorNamesItsOwner(t *testing.T) { + for _, ep := range oddEntryPoints { + for _, reply := range ep.replies { + if reply == replySigners { + continue // A null signer node isn't an error. + } + variants, err := testHelpers.NullVariants([]byte(oddBaselineReplies()[reply])) + require.NoError(t, err) + for _, v := range variants { + if !strings.HasSuffix(v.Path, "]") || strings.HasSuffix(v.Path, "[]") { + continue + } + t.Run(ep.name+"/"+reply+"/"+v.Path, func(t *testing.T) { + replies := oddBaselineReplies() + replies[reply] = string(v.Body) + ts := newOddReplyServer(t, replies) + _, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", + Repository: "test-repo", Sleep: func(time.Duration) {}}) + owner := "for test-org/test-repo#7 " + if regexp.MustCompile(`associatedPullRequests\.nodes\[\d+\]$`).MatchString(v.Path) { + owner = "for commit merge-sha " + } + require.ErrorContains(t, err, owner) + }) + } + } + } +} + +// A commit with no author is recorded with an empty author, not " <>". +func TestOddReplies_MissingCommitAuthorIsEmpty(t *testing.T) { + author := `"author":{"name":"Ada","email":"ada@example.com","user":{"login":"ada"}},` + for _, ep := range oddEntryPoints { + t.Run(ep.name, func(t *testing.T) { + replies := oddBaselineReplies() + require.Equal(t, 1, strings.Count(replies[replyCommitPage], author)) + replies[replyCommitPage] = strings.Replace(replies[replyCommitPage], author, `"author":null,`, 1) + ts := newOddReplyServer(t, replies) + evidence, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", + Repository: "test-repo", Sleep: func(time.Duration) {}}) + require.NoError(t, err) + var authors []string + for _, c := range evidence.Commits { + authors = append(authors, c.Author) + } + require.Equal(t, []string{"Ada ", ""}, authors) + }) + t.Run(ep.name+"/name only", func(t *testing.T) { + replies := oddBaselineReplies() + replies[replyCommitPage] = strings.Replace(replies[replyCommitPage], author, `"author":{"name":"Ada","email":null,"user":null},`, 1) + ts := newOddReplyServer(t, replies) + evidence, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", + Repository: "test-repo", Sleep: func(time.Duration) {}}) + require.NoError(t, err) + require.Equal(t, "Ada <>", evidence.Commits[1].Author, "one part is enough to keep the format") + }) + t.Run(ep.name+"/email only", func(t *testing.T) { + replies := oddBaselineReplies() + replies[replyCommitPage] = strings.Replace(replies[replyCommitPage], author, `"author":{"name":null,"email":"ada@example.com","user":null},`, 1) + ts := newOddReplyServer(t, replies) + evidence, err := ep.run(&GithubConfig{Token: "fake-token", BaseURL: ts.URL, Org: "test-org", + Repository: "test-repo", Sleep: func(time.Duration) {}}) + require.NoError(t, err) + require.Equal(t, " ", evidence.Commits[1].Author, "one part is enough to keep the format") + }) + } +} diff --git a/internal/github/pagination.go b/internal/github/pagination.go index 22f10f4f1..0b2924fb8 100644 --- a/internal/github/pagination.go +++ b/internal/github/pagination.go @@ -169,7 +169,7 @@ func (c *GithubConfig) allPRCommits(ctx context.Context, run graphqlQueryFunc, r return page.Nodes, page.PageInfo, nil }) if err == nil { - return nonNull(commits, "commits") + return nonNull(commits, "commits", ref.String()) } return nil, fmt.Errorf("draining commits for %s: %w", ref, err) } @@ -198,7 +198,7 @@ func (c *GithubConfig) allPRReviews(ctx context.Context, run graphqlQueryFunc, r return page.Nodes, page.PageInfo, nil }) if err == nil { - return nonNull(reviews, "reviews") + return nonNull(reviews, "reviews", ref.String()) } return nil, fmt.Errorf("draining reviews for %s: %w", ref, err) }