[AlexKantor87]

Part of #1283.

GitHub can send a field as null together with an error that explains why. Our GraphQL library (shurcooL/graphql) fails the whole call on any error, so one empty optional field failed the whole PR attestation. That's what broke v2.48.0 (#1274). #1275 fixed it for the signer only; this fixes it for every field.

What changed

• One GraphQL client for every evidence query (graphql_client.go). Its HTTP transport removes an error from GitHub's reply only when the error just explains one null field. Each removed error goes to the --debug output.
• Errors that matter still fail. An error is kept when it's on a list, has no path, or has nulled a parent object. It's also kept within 2 levels of the root. That keeps GitHub's own message for a missing or SAML-blocked repository or PR.
• The signer lookup still tolerates an error that comes with data, as fix: record PR evidence when a commit is signed by a GitHub App #1275 did. A missing signer only loses that one name.
• Empty timestamps are 0, not an error. A null list item (a commit, review or PR) is an error that names its commit or PR, because skipping it could hide a "changes requested" review.
• A commit with no author is recorded with an empty author, not <>.
• Reviews by deleted accounts. GitHub sends these as a null author or the ghost login. They're never an approver. In reviews we keep one only if it requests changes, because dropping it would read as no objection. This is the same rule Bitbucket has on main.

Tests

• odd_replies_test.go makes each value null, through both PREvidenceByPRNumber and PREvidenceForCommitV2. It checks there's never a crash or unexpected error, and that null plus an error gives the same result as plain null. Against main, 71 cases fail and none crash: shurcooL/graphql returns an error instead.
• New tests for the SAML message, a null signer node, isNullField and deleted-account reviews on the GraphQL and REST paths.

Behaviour change

An error removed by the transport is no longer retried, so it doesn't add 60 seconds of retries.

Full CLI suite against a local server: 4,059 tests passed.

🤖 Generated with Claude Code

[claude]

Claude finished @AlexKantor87's task in 2m 4s —— View job

---

Follow-up review of commits 2bde604..6d77d19

Reviewed git diff 2bde6043..6d77d195 (BEFORE..AFTER) — one new commit, touching internal/github/github.go, internal/github/pagination.go, internal/github/odd_replies_test.go.

• Fetch diff for the new commit
• Re-check prior findings against the new commit
• Flag any new bugs/regressions in changed lines
• Post summary comment

Previous findings — status:

• Null connections/lists silently dropped (graphql_client.go) — not touched by this commit; stays Resolved (from 6aa01747). Inline thread already resolved on GitHub.
• Content-Length mismatch after trimming errors — not touched by this commit; stays Resolved (from 6aa01747). Inline thread already resolved on GitHub.
• mbevc1: null-entry errors don't name the PR or commit — Resolved in 6d77d195. nonNull now takes an owner argument (internal/github/github.go:500), and all three call sites pass it: allPRCommits/allPRReviews pass ref.String() (internal/github/pagination.go:172,201), and the associatedPullRequests drain in PREvidenceForCommitV2 passes "commit " + commit (internal/github/github.go:665). A null commit now reads e.g. commits[0] for org/repo#7 is null in the GitHub reply, and a null associated PR reads associatedPullRequests[0] for commit <sha> is null in the GitHub reply. Covered by the new TestOddReplies_NullItemErrorNamesItsOwner.
• mbevc1: "94 cases failed" couldn't be reproduced — not a code finding (no location), nothing for this commit to address.

Also in this commit: a commit with no author now records "" instead of " <>" (internal/github/github.go:427-435), with new coverage for the no-author/name-only/email-only cases in TestOddReplies_MissingCommitAuthorIsEmpty. This is a sound, narrowly-scoped addition and not a regression.

New issues in the changed lines: none found. Traced both nonNull call sites plus the PREvidenceForCommitV2 commit-owner path and the author-formatting change; logic and new tests line up correctly. 0 new inline comments.

Note: build/test commands (go build, go vet) were not permitted to run in this environment, so this review is based on careful reading of the diff plus the existing test suite rather than an executed run.

[claude]

Follow-up review of commits 6aa0174, 5fea125

Reviewed git diff 20ee9c22..5fea1253 (BEFORE..AFTER).

Previous findings:

• Null connections/lists silently dropped (internal/github/graphql_client.go:136) — Resolved in 6aa01747. A listFields set (nodes, commits, reviews, associatedPullRequests) now makes isNullField return false for those path endings, so the error is kept instead of being swallowed. Covered by new cases in TestIsNullField and the holdsList check added to TestOddReplies_FieldErrorMeansTheSameAsNull. Thread already marked resolved.
• Content-Length mismatch after trimming errors — Resolved in 6aa01747. RoundTrip now recomputes resp.ContentLength from the rewritten body and deletes the stale Content-Length header, verified by the new TestNullFieldErrorTransport_LengthMatchesTheNewBody. Thread already marked resolved.

New commit 5fea1253 (deleted-account reviews) is unrelated to the above findings — it keeps a deleted account's review in the evidence but excludes it from Approvers/approval counting (both GraphQL and REST paths), via the new reviewer() helper. Read through internal/github/github.go and the new odd_replies_test.go/rest_pagination_test.go cases; logic and tests look correct (GraphQL Author is a plain struct so a null author decodes to "" with no nil-pointer risk; REST's GetUser()/GetLogin() are nil-safe).

No new bugs or regressions found in the changed lines; 0 new inline comments.

[AlexKantor87]

This branch now includes #1291's branch, so the Go 1.26.10 change clears the Snyk check here. That's why go.mod, Dockerfile, .github/dependabot.yml and internal/version/toolchain_test.go show in this diff. They're identical to #1291 and drop out of the diff once it merges. Please review those files on #1291, not here.

no search: answers no review finding; it brings in #1291's reviewed files unchanged (git diff origin/claude/go-1.26.10 HEAD on those four files is empty).

[mbevc1]

1. Nit: null-entry errors don't name the PR or commit.
   A null commit gives commits[0] is null in the GitHub reply. In allPRCommits/allPRReviews the nonNull error skips the draining … for <ref> wrap, and associatedPullRequests[i] doesn't name the commit. GitLab fixed the same nit in ad1b4f6.

2. "Before the fix, 94 cases failed": couldn't reproduce. - got 60 failing subtests and no panics.

[AlexKantor87]

Thanks @mbevc1.

1. Null-entry errors. Fixed in 6d77d19. Each error now says what the list belongs to: the commit for associatedPullRequests, or the pull request (owner/repo#n) for commits and reviews. The same commit records a commit with no author as "", not <>, which you found on Azure.

2. "94 cases". You're right, that number was out of date. I ran the odd-reply tests against main and got your 60 failing subtests, with no crashes: shurcooL/graphql returns an error instead. The "94" came from an earlier version of the test file. With the tests 6d77d19 adds, the count is 71, and the description now says that. I re-measured the other three PRs the same way. Bitbucket's 62 still holds. Azure's said "53, 51 of them crashes", but the real number is 51 crashes, so I fixed that too.

search: grep -nE 'nonNull(' internal/github/*.go (non-test)

github.go:665: nonNull(query.Repository.Object.Commit.AssociatedPullRequests.Nodes, "associatedPullRequests", "commit "+commit)
pagination.go:172: nonNull(commits, "commits", ref.String())
pagination.go:201: nonNull(reviews, "reviews", ref.String())

verified: sed -n '500,505p' internal/github/github.go

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] for %s is null in the GitHub reply", list, i, owner)
		}

mutation: drop the owner from the nonNull error -> TestOddReplies_NullItemErrorNamesItsOwner red (9 cases)
mutation: drop the empty-author check -> TestOddReplies_MissingCommitAuthorIsEmpty red (2 cases)