[AlexKantor87]

Part of #1283.

kosli attest pullrequest azure and kosli assert pullrequest azure crashed when Azure DevOps sent a field as null where the code expected a value. The Azure SDK gives every field as a pointer, and the code read most of them without checking. For now we treat every field as optional (#1284 will decide which fields are required).

What changed

• One helper for every pointer. value returns what a pointer holds, or an empty value when it's nil. It replaces every unchecked read in the V1 path (assert) and the V2 path (attest), including the reviewer vote.
• Missing times are 0. unixTime does the same for timestamps.
• Unreadable list entries still fail. The SDK turns a null pull request or commit into an empty one, so a missing pullRequestId or commitId is the only sign. Either now returns an error rather than being recorded as empty.
• A reviewer with no id returns an error. The SDK turns a null reviewer into an empty one, and skipping it could hide a change request.
• A missing author (of the PR, a reviewer or a commit) is recorded as empty, not () or <>.
• A null query result still means no pull requests, as on main. Azure may send that for a commit with none, and failing would break attestations that work today.
• Reviewers with no name (a deleted account) are never approvers. 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 runs a fake Azure DevOps server that the real SDK talks to. It sends replies through V1 and V2 once per field, with that field made null. It checks there's never a crash, and never an error except a missing pullRequestId, commitId or reviewer id. Against main, 51 cases crash.

Not changed

A reviewer with only one of display name and unique name gives text like Ada (). That's for #1284.

Full CLI suite against a local server: 3,849 tests passed.

🤖 Generated with Claude Code

[claude]

Claude finished @AlexKantor87's task in 1m 46s —— View job

---

Follow-up review — commits 231914b4..1e1db322

Reviewed the two new commits (9c837a6a "error on a reviewer with no id", 1e1db322 "record a missing author as empty, not as separators"), scoped to git diff 231914b4..1e1db322 (internal/azure/azure.go +27/-13, internal/azure/odd_replies_test.go +55/-2).

• Fetch previous review comments and PR context
• Diff 231914b4..1e1db322 and identify touched regions
• Check status of previous findings touched by new commits
• Review new commits for bugs/regressions in changed lines
• Post summary comment

Previous finding status:

• azure.go:202 (nil PullRequestId error text mismatch) — unaffected by these commits (lines untouched); already resolved and the thread is marked resolved on GitHub. No action needed.
• mbevc1 finding 1 (null reviewer entry silently dropped) — resolved. pullRequestReviewers (azure.go:296-300) now errors with "reviewer %d of pull request %d has no id" when r.Id is empty, matching the suggested fix; covered by the new TestPREvidenceReviewerWithNoIDFails.
• mbevc1 finding 2 (null createdBy/author rendered as " ()"/" <>") — resolved. The new azureIdentity(format, name, qualifier) helper (azure.go:307-312) returns "" when both parts are empty, used for both PR author and commit author; covered by TestPREvidenceMissingAuthorIsEmpty.
• mbevc1 finding 3 (group reviewer double-counted as approver) — not addressed by these commits, consistent with the PR author's stated scope (deferred to Decide which PR evidence fields are required for each provider #1284).

New review of the changed lines: no bugs or regressions found. The generalization of azureReviewerName into azureIdentity(format, name, qualifier *string) is applied consistently at all three call sites (PR author, commit author, approvers/reviews), and the new id-based error check is a straightforward extension of the existing "missing id is the only sign of a null entry" pattern already used for pull requests and commits — 0 new inline comments.

Note: sandboxed execution here doesn't allow running go build/go test/go vet, so this review is based on static reading of the diff only.

[claude]

Follow-up review — commits deaffee9..ebbbe69

These commits are a merge from origin/main that pulled in a later revision of the internal/testHelpers package (from #1285, which merged with an extra fix after this branch first took it). They don't touch internal/azure/azure.go or internal/azure/odd_replies_test.go.

Previous finding status:

• azure.go:202 (nil PullRequestId error text mismatch between L190-195 and L202) — not addressed. That code wasn't touched by these commits; the inconsistency is still there.

New commits reviewed: internal/testHelpers/oddReplies.go / oddReplies_test.go — fixes EvidenceOutcome so a nil baseline entry matched by a nil fetched entry is no longer misreported as lost:PullRequestN, with a new test (TestEvidenceOutcome_NilInBothIsNotALoss) covering it. No bugs or regressions found in these 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).

[claude]

Follow-up review — commits 07ce04aa..acfd9d1b

These commits are a merge from origin/main (bringing in #1291's Go 1.26.10 change). The only change in git diff 07ce04aa..acfd9d1b is a 3/-2 line tweak to .github/dependabot.yml (Docker image bump schedule: weekly/Tuesday → daily). internal/azure/azure.go and internal/azure/odd_replies_test.go are untouched.

Previous finding status:

• azure.go:202 (nil PullRequestId error text mismatch) — resolved, already confirmed in the prior exchange (fix in 642a1fc, verified via grep/mutation test) and not touched by these commits. Review thread was already marked resolved on GitHub.

No findings were newly touched by these commits, and no new issues found in the changed lines — 0 new inline comments.

[mbevc1]

1. A null reviewer entry is silently dropped (design question).
   The SDK decodes reviewers as []IdentityRefWithVote, so a null entry becomes an empty struct with no vote, which is skipped. A reply of [null, {vote: 10, …}] gives no error. The skip is deliberate (oddAllowedSkips). The GitLab PR errors in this case because skipping "could hide a changes requested review". main panics on the same reply, so erroring breaks nothing that works today.
   Suggestion: error on a reviewer with no id, the same "no id is the only sign" check already used for pullRequestId/commitId. Otherwise, say in the description that reviewers are the exception and why. Erroring means adding id to the reviewer entries that lack one in TestPREvidenceReviewerWithOneName/TestPREvidenceReviewerWithNoIdentity, and listing the null-id case in oddAllowedErrors.

2. A null createdBy records the author as " ()".
   GitLab gives "" in the same case, and this PR's azureReviewerName also gives "" when both names are missing. Tests pass because oddFeeds accepts any change to author.
   Suggestion: build Author with a helper like azureReviewerName, so a missing identity gives "".

3. A group reviewer counts as an approver (already on main, for Decide which PR evidence fields are required for each provider #1284).
   A group reviewer (isContainer: true) with vote: 10 appears in Approvers on V1 and V2, while Reviews leaves it out. Azure gives a group the vote of a member who approves on its behalf, so one approval can count twice towards a minimum-approvers policy.

[AlexKantor87]

Thanks @mbevc1, both fixed.

1. Null reviewer. Fixed in 9c837a6. A reviewer with no id now returns an error, the same check as for pullRequestId and commitId. Skipping it could hide a change request. The fixtures in TestPREvidenceReviewerWithOneName and TestPREvidenceReviewerWithNoIdentity now have ids, and reviewers value[i].id is in oddAllowedErrors.

2. Author as (). Fixed in 1e1db32. One helper, azureIdentity, now formats the PR author, reviewers and commit authors. It gives "" when both parts are missing, so a commit with no author is no longer <> either.

I checked the other PRs for both. A missing commit author was also <> on GitHub (fixed in #1289) and GitLab (#1301, since #1287 had merged). No other provider skipped an unreadable entry the way Azure skipped reviewers.

search: grep -nE 'has no id|azureIdentity(' internal/azure/azure.go

148: Author: azureIdentity("%s (%s)", value(pr.CreatedBy).DisplayName, value(pr.CreatedBy).UniqueName),
216: Author: azureIdentity("%s <%s>", author.Name, author.Email),
298: fmt.Errorf("reviewer %d of pull request %d has no id", i, prNumber)
317: azureIdentity("%s (%s)", r.DisplayName, r.UniqueName)   (approvers)
348: azureIdentity("%s (%s)", r.DisplayName, r.UniqueName)   (reviews)

verified: sed -n '296,299p;307,312p' internal/azure/azure.go

	for i, r := range value(reviewers) {
		if value(r.Id) == "" {
			return nil, fmt.Errorf("reviewer %d of pull request %d has no id", i, prNumber)
		}
func azureIdentity(format string, name, qualifier *string) string {
	if value(name) == "" && value(qualifier) == "" {
		return ""
	}
	return fmt.Sprintf(format, value(name), value(qualifier))
}

mutation: drop the reviewer id check -> TestPREvidenceOddReplies and TestPREvidenceReviewerWithNoIDFails red (5 cases)
mutation: drop the empty check in azureIdentity -> 6 tests red
mutation: commit author back to Sprintf -> TestPREvidenceMissingAuthorIsEmpty red