From 072fb2fb19caae803bfd50f07b003c2d30d36a10 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Wed, 21 Oct 2026 17:41:01 +0100 Subject: [PATCH 1/7] 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 aeba3dddec09f068432db9e88c95f69be50dd619 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Wed, 21 Oct 2026 18:05:48 +0100 Subject: [PATCH 2/7] fix(azure): never crash on null fields in Azure DevOps replies Every pointer the SDK hands back can be nil when Azure DevOps sends a field as null or leaves it out, and the evidence code dereferenced them unchecked: 51 of the 188 null variants in the new test panicked, through both `assert pullrequest azure` (V1) and attest (V2). One generic helper, value, now reads every reply pointer and gives the zero value when it is missing; unixTime does the same for timestamps. azureApprovers no longer dereferences Vote, so azureReviews loses its separate nil guard. SDK lists hold structs, so a null item decodes to an empty one. A pull request with no pullRequestId, a commit with no commitId, or a null query result is now an error instead of a panic or a silent drop. A null reviewer is skipped, as a reviewer with no vote always was. The test feeds each reply with each value nulled and asserts the rule rather than a golden file. Part of #1283 Co-Authored-By: Claude Opus 5.5 --- internal/azure/azure.go | 99 ++++++---- internal/azure/odd_replies_test.go | 308 +++++++++++++++++++++++++++++ 2 files changed, 372 insertions(+), 35 deletions(-) create mode 100644 internal/azure/odd_replies_test.go diff --git a/internal/azure/azure.go b/internal/azure/azure.go index d721883cc..a14c19bba 100644 --- a/internal/azure/azure.go +++ b/internal/azure/azure.go @@ -12,6 +12,24 @@ import ( "github.com/microsoft/azure-devops-go-api/azuredevops/git" ) +// value gives what an SDK reply pointer holds, or the zero value when the +// reply left the field out or null. +func value[T any](p *T) T { + if p == nil { + var zero T + return zero + } + return *p +} + +// unixTime gives a reply timestamp in Unix seconds, or 0 when it is missing. +func unixTime(t *azuredevops.Time) int64 { + if t == nil { + return 0 + } + return t.Time.Unix() +} + type AzureConfig struct { Token string OrgURL string @@ -98,17 +116,17 @@ func (c *AzureConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence, } func (c *AzureConfig) newPRAzureEvidence(pr git.GitPullRequest) (*types.PREvidence, error) { - prID := strconv.Itoa(*pr.PullRequestId) + prID := strconv.Itoa(value(pr.PullRequestId)) url, err := url.JoinPath(c.OrgURL, c.Project, "_git", c.Repository, "pullrequest", prID) if err != nil { return nil, err } evidence := &types.PREvidence{ URL: url, - MergeCommit: *(pr.LastMergeCommit.CommitId), - State: string(*pr.Status), + MergeCommit: value(value(pr.LastMergeCommit).CommitId), + State: string(value(pr.Status)), } - evidence.Approvers, err = c.GetPullRequestApprovers(*pr.PullRequestId, 1) + evidence.Approvers, err = c.GetPullRequestApprovers(value(pr.PullRequestId), 1) if err != nil { return evidence, err } @@ -117,7 +135,7 @@ func (c *AzureConfig) newPRAzureEvidence(pr git.GitPullRequest) (*types.PREviden // newPRAzureEvidenceV2 creates a new PREvidence for a given pull request in V2 format func (c *AzureConfig) newPRAzureEvidenceV2(pr git.GitPullRequest) (*types.PREvidence, error) { - prID := strconv.Itoa(*pr.PullRequestId) + prID := strconv.Itoa(value(pr.PullRequestId)) prURL, err := url.JoinPath(c.OrgURL, c.Project, "_git", c.Repository, "pullrequest", prID) if err != nil { return nil, err @@ -125,22 +143,22 @@ func (c *AzureConfig) newPRAzureEvidenceV2(pr git.GitPullRequest) (*types.PREvid evidence := &types.PREvidence{ URL: prURL, - MergeCommit: *(pr.LastMergeCommit.CommitId), - State: string(*pr.Status), - Author: fmt.Sprintf("%s (%s)", *pr.CreatedBy.DisplayName, *pr.CreatedBy.UniqueName), - CreatedAt: pr.CreationDate.Time.Unix(), - Title: *pr.Title, + MergeCommit: value(value(pr.LastMergeCommit).CommitId), + State: string(value(pr.Status)), + Author: fmt.Sprintf("%s (%s)", value(value(pr.CreatedBy).DisplayName), value(value(pr.CreatedBy).UniqueName)), + CreatedAt: unixTime(pr.CreationDate), + Title: value(pr.Title), } evidence.HeadRef, evidence.BaseRef = azurePRRefs(pr) - if pr.Status != nil && pr.ClosedDate != nil && *pr.Status == git.PullRequestStatusValues.Completed { - evidence.MergedAt = pr.ClosedDate.Time.Unix() + if value(pr.Status) == git.PullRequestStatusValues.Completed { + evidence.MergedAt = unixTime(pr.ClosedDate) } commits, err := c.GetPullRequestCommits(pr) if err != nil { return evidence, err } evidence.Commits = commits - evidence.Approvers, evidence.Reviews, err = c.GetPullRequestReviews(*pr.PullRequestId) + evidence.Approvers, evidence.Reviews, err = c.GetPullRequestReviews(value(pr.PullRequestId)) if err != nil { return evidence, err } @@ -150,7 +168,7 @@ func (c *AzureConfig) newPRAzureEvidenceV2(pr git.GitPullRequest) (*types.PREvid // azurePRRefs returns the head (source) and base (target) ref names of an Azure // DevOps pull request, kept raw (e.g. "refs/heads/main") to match head_ref. func azurePRRefs(pr git.GitPullRequest) (head, base string) { - return *pr.SourceRefName, *pr.TargetRefName + return value(pr.SourceRefName), value(pr.TargetRefName) } // GetPullRequestCommits returns a list of commits for a given pull request @@ -178,8 +196,12 @@ func (c *AzureConfig) GetPullRequestCommits(pr git.GitPullRequest) ([]types.Comm return commits, fmt.Errorf("draining commits for pull request %s: %w", prID, err) } - for _, commit := range prCommits { - commits = append(commits, commitFromAzureCommit(commit, *pr.SourceRefName)) + for i, commit := range prCommits { + // A null commit decodes to an empty one, so no id is the only sign. + if commit.CommitId == nil { + return commits, fmt.Errorf("commit %d of pull request %d has no commitId", i, value(pr.PullRequestId)) + } + commits = append(commits, commitFromAzureCommit(commit, value(pr.SourceRefName))) } return commits, nil @@ -187,12 +209,13 @@ func (c *AzureConfig) GetPullRequestCommits(pr git.GitPullRequest) ([]types.Comm // commitFromAzureCommit maps an Azure DevOps API commit to a types.Commit. func commitFromAzureCommit(commit git.GitCommitRef, branch string) types.Commit { + author := value(commit.Author) return types.Commit{ - SHA: *commit.CommitId, - Message: *commit.Comment, - Author: fmt.Sprintf("%s <%s>", *commit.Author.Name, *commit.Author.Email), - Timestamp: commit.Author.Date.Time.Unix(), - URL: *commit.Url, + SHA: value(commit.CommitId), + Message: value(commit.Comment), + Author: fmt.Sprintf("%s <%s>", value(author.Name), value(author.Email)), + Timestamp: unixTime(author.Date), + URL: value(commit.Url), Branch: branch, } } @@ -220,15 +243,21 @@ func (c *AzureConfig) PullRequestsForCommit(commit string) ([]git.GitPullRequest if err != nil { return nil, err } - if prQuery != nil { - results := prQuery.Results - if len(*results) > 0 { - prsForCommit := (*results)[0][commit] - return prsForCommit, nil + results := value(value(prQuery).Results) + if len(results) == 0 { + return []git.GitPullRequest{}, nil + } + if results[0] == nil { + return nil, fmt.Errorf("pull request query for commit %s has a null result", commit) + } + prs := results[0][commit] + for i, pr := range prs { + // A null pull request decodes to an empty one, so no id is the only sign. + if pr.PullRequestId == nil { + return nil, fmt.Errorf("pull request %d for commit %s has no pullRequestId", i, commit) } } - - return []git.GitPullRequest{}, nil + return prs, nil } // GetPullRequestApprovers returns a list of approvers for a given pull request @@ -264,17 +293,17 @@ func (c *AzureConfig) pullRequestReviewers(prNumber int) ([]git.IdentityRefWithV if err != nil { return nil, err } - return *reviewers, nil + return value(reviewers), nil } func azureReviewerName(r git.IdentityRefWithVote) string { - return fmt.Sprintf("%s (%s)", *r.DisplayName, *r.UniqueName) + return fmt.Sprintf("%s (%s)", value(r.DisplayName), value(r.UniqueName)) } func azureApprovers(reviewers []git.IdentityRefWithVote, version int) []any { var approvers []any for _, r := range reviewers { - if *r.Vote == 10 { + if value(r.Vote) == 10 { approverName := azureReviewerName(r) if version == 1 { approvers = append(approvers, approverName) @@ -294,14 +323,14 @@ func azureApprovers(reviewers []git.IdentityRefWithVote, version int) []any { func azureReviews(reviewers []git.IdentityRefWithVote) []types.PRApprovals { reviews := []types.PRApprovals{} for _, r := range reviewers { - if (r.IsContainer != nil && *r.IsContainer) || r.Vote == nil { + if value(r.IsContainer) { continue } var state string - switch { - case *r.Vote > 0: + switch vote := value(r.Vote); { + case vote > 0: state = types.ReviewApproved - case *r.Vote < 0: + case vote < 0: state = types.ReviewChangesRequested default: continue diff --git a/internal/azure/odd_replies_test.go b/internal/azure/odd_replies_test.go new file mode 100644 index 000000000..c9a9e527f --- /dev/null +++ b/internal/azure/odd_replies_test.go @@ -0,0 +1,308 @@ +package azure + +import ( + "io" + "net/http" + "net/http/httptest" + "reflect" + "regexp" + "slices" + "strings" + "sync" + "testing" + + "github.com/kosli-dev/cli/internal/testHelpers" + "github.com/kosli-dev/cli/internal/types" + "github.com/stretchr/testify/require" +) + +// The baseline replies follow the shapes in the Azure DevOps REST docs (7.1): +// - PR query: https://learn.microsoft.com/en-us/rest/api/azure/devops/git/pull-request-query/get?view=azure-devops-rest-7.1 +// - PR object: https://learn.microsoft.com/en-us/rest/api/azure/devops/git/pull-requests/get-pull-request?view=azure-devops-rest-7.1 +// - PR commits: https://learn.microsoft.com/en-us/rest/api/azure/devops/git/pull-request-commits/get-pull-request-commits?view=azure-devops-rest-7.1 +// - PR reviewers: https://learn.microsoft.com/en-us/rest/api/azure/devops/git/pull-request-reviewers/list?view=azure-devops-rest-7.1 +// Only the reviewers page carries a sample body; the others are built from the +// documented schemas, in the same fabrikam style. + +const oddMergeCommit = "b60280bc6e62e2f880f1b63c1e24987664d3bda3" + +const oddPRQueryReply = `{ + "queries": [{"items": ["b60280bc6e62e2f880f1b63c1e24987664d3bda3"], "type": "lastMergeCommit"}], + "results": [{ + "b60280bc6e62e2f880f1b63c1e24987664d3bda3": [{ + "repository": { + "id": "3411ebc1-d5aa-464f-9615-0b527bc66719", + "name": "Fabrikam-Fiber-Git", + "url": "https://dev.azure.com/fabrikam/_apis/git/repositories/3411ebc1-d5aa-464f-9615-0b527bc66719", + "project": {"id": "a7573007-bbb3-4341-b726-0c4148a07853", "name": "Fabrikam-Fiber-Git", "state": "wellFormed", "visibility": "private"} + }, + "pullRequestId": 22, + "codeReviewId": 22, + "status": "completed", + "createdBy": { + "displayName": "Normal Paulk", + "url": "https://dev.azure.com/fabrikam/_apis/Identities/d6245f20-2af8-44f4-9451-8107cb2767db", + "id": "d6245f20-2af8-44f4-9451-8107cb2767db", + "uniqueName": "fabrikamfiber16@hotmail.com", + "imageUrl": "https://dev.azure.com/fabrikam/_api/_common/identityImage?id=d6245f20-2af8-44f4-9451-8107cb2767db" + }, + "creationDate": "2026-03-01T10:00:00.0000000Z", + "closedDate": "2026-03-02T15:30:00.0000000Z", + "title": "A new feature", + "description": "Adding a new feature", + "sourceRefName": "refs/heads/npaulk/my_work", + "targetRefName": "refs/heads/new_feature", + "mergeStatus": "succeeded", + "isDraft": false, + "mergeId": "f5fc8381-3fb2-49fe-8a0d-27dcc2d6ef82", + "lastMergeSourceCommit": {"commitId": "b60280bc6e62e2f880f1b63c1e24987664d3bda3", "url": "https://dev.azure.com/fabrikam/_apis/git/repositories/3411ebc1-d5aa-464f-9615-0b527bc66719/commits/b60280bc6e62e2f880f1b63c1e24987664d3bda3"}, + "lastMergeTargetCommit": {"commitId": "f47bbc106853afe3c1b07a81754bce5f4b8dbf62", "url": "https://dev.azure.com/fabrikam/_apis/git/repositories/3411ebc1-d5aa-464f-9615-0b527bc66719/commits/f47bbc106853afe3c1b07a81754bce5f4b8dbf62"}, + "lastMergeCommit": {"commitId": "b60280bc6e62e2f880f1b63c1e24987664d3bda3", "url": "https://dev.azure.com/fabrikam/_apis/git/repositories/3411ebc1-d5aa-464f-9615-0b527bc66719/commits/b60280bc6e62e2f880f1b63c1e24987664d3bda3"}, + "reviewers": [{ + "reviewerUrl": "https://dev.azure.com/fabrikam/_apis/git/repositories/3411ebc1-d5aa-464f-9615-0b527bc66719/pullRequests/22/reviewers/d6245f20-2af8-44f4-9451-8107cb2767db", + "vote": 10, + "displayName": "Normal Paulk", + "uniqueName": "fabrikamfiber16@hotmail.com", + "id": "d6245f20-2af8-44f4-9451-8107cb2767db" + }], + "url": "https://dev.azure.com/fabrikam/_apis/git/repositories/3411ebc1-d5aa-464f-9615-0b527bc66719/pullRequests/22", + "supportsIterations": true + }] + }] +}` + +const oddPRCommitsReply = `{ + "count": 2, + "value": [ + { + "commitId": "52a6ae5ef3a2fde5c3c9e4e0d6b8e7a1d2c3b4a5", + "author": {"name": "Normal Paulk", "email": "fabrikamfiber16@hotmail.com", "date": "2026-03-01T09:00:00Z"}, + "committer": {"name": "Normal Paulk", "email": "fabrikamfiber16@hotmail.com", "date": "2026-03-01T09:00:00Z"}, + "comment": "Add the feature flag", + "url": "https://dev.azure.com/fabrikam/_apis/git/repositories/3411ebc1-d5aa-464f-9615-0b527bc66719/commits/52a6ae5ef3a2fde5c3c9e4e0d6b8e7a1d2c3b4a5" + }, + { + "commitId": "7e9b1d2c3a4f5e6d7c8b9a0f1e2d3c4b5a6f7e8d", + "author": {"name": "Chuck Reinhart", "email": "fabrikamfiber3@hotmail.com", "date": "2026-03-01T09:30:00Z"}, + "committer": {"name": "Chuck Reinhart", "email": "fabrikamfiber3@hotmail.com", "date": "2026-03-01T09:30:00Z"}, + "comment": "Wire the feature into the UI", + "url": "https://dev.azure.com/fabrikam/_apis/git/repositories/3411ebc1-d5aa-464f-9615-0b527bc66719/commits/7e9b1d2c3a4f5e6d7c8b9a0f1e2d3c4b5a6f7e8d" + } + ] +}` + +const oddPRReviewersReply = `{ + "count": 2, + "value": [ + { + "reviewerUrl": "https://dev.azure.com/fabrikam/_apis/git/repositories/3411ebc1-d5aa-464f-9615-0b527bc66719/pullRequests/22/reviewers/d6245f20-2af8-44f4-9451-8107cb2767db", + "vote": 10, + "id": "d6245f20-2af8-44f4-9451-8107cb2767db", + "displayName": "Normal Paulk", + "uniqueName": "fabrikamfiber16@hotmail.com", + "url": "https://dev.azure.com/fabrikam/_apis/Identities/d6245f20-2af8-44f4-9451-8107cb2767db", + "imageUrl": "https://dev.azure.com/fabrikam/_api/_common/identityImage?id=d6245f20-2af8-44f4-9451-8107cb2767db" + }, + { + "reviewerUrl": "https://dev.azure.com/fabrikam/_apis/git/repositories/3411ebc1-d5aa-464f-9615-0b527bc66719/pullRequests/22/reviewers/19d9411e-9a34-45bb-b985-d24d9d87c0c9", + "vote": -10, + "id": "19d9411e-9a34-45bb-b985-d24d9d87c0c9", + "displayName": "Johnnie McLeod", + "uniqueName": "fabrikamfiber2@hotmail.com", + "url": "https://dev.azure.com/fabrikam/_apis/Identities/19d9411e-9a34-45bb-b985-d24d9d87c0c9", + "imageUrl": "https://dev.azure.com/fabrikam/_api/_common/identityImage?id=19d9411e-9a34-45bb-b985-d24d9d87c0c9" + } + ] +}` + +// oddLocations answers the SDK's OPTIONS /_apis discovery call with the +// routes PREvidenceForCommitV2 uses: resource areas (asked first, answered +// empty as on-prem servers do, so the org URL itself is used), PR query, PR +// commits and PR reviewers. Ids and templates are the SDK's own. +const oddLocations = `{"count": 4, "value": [ + {"id": "e81700f7-3be2-46de-8624-2eb35882fcaa", "area": "Location", "resourceName": "ResourceAreas", "routeTemplate": "_apis/{resource}/{areaId}", "resourceVersion": 1, "minVersion": "1.0", "maxVersion": "7.1", "releasedVersion": "7.1"}, + {"id": "b3a6eebe-9cf0-49ea-b6cb-1a4c5f5007b0", "area": "git", "resourceName": "pullRequestQuery", "routeTemplate": "{project}/_apis/{area}/repositories/{repositoryId}/{resource}", "resourceVersion": 1, "minVersion": "1.0", "maxVersion": "7.1", "releasedVersion": "7.1"}, + {"id": "52823034-34a8-4576-922c-8d8b77e9e4c4", "area": "git", "resourceName": "commits", "routeTemplate": "{project}/_apis/{area}/repositories/{repositoryId}/pullRequests/{pullRequestId}/{resource}", "resourceVersion": 1, "minVersion": "1.0", "maxVersion": "7.1", "releasedVersion": "7.1"}, + {"id": "4b6702c7-aa35-4b89-9c96-b9abf6d3e540", "area": "git", "resourceName": "reviewers", "routeTemplate": "{project}/_apis/{area}/repositories/{repositoryId}/pullRequests/{pullRequestId}/{resource}/{reviewerId}", "resourceVersion": 1, "minVersion": "1.0", "maxVersion": "7.1", "releasedVersion": "7.1"} +]}` + +// fakeAzureDevOps serves the replies the SDK asks for. One server is shared +// by every variant because the SDK caches route discovery per base URL for +// the life of the process. +type fakeAzureDevOps struct { + mu sync.Mutex + replies map[string]string // keyed by the last path segment +} + +func (f *fakeAzureDevOps) set(replies map[string]string) { + f.mu.Lock() + defer f.mu.Unlock() + f.replies = replies +} + +func (f *fakeAzureDevOps) ServeHTTP(w http.ResponseWriter, r *http.Request) { + _, _ = io.Copy(io.Discard, r.Body) + w.Header().Set("Content-Type", "application/json; charset=utf-8") + path := r.URL.Path + switch { + case r.Method == http.MethodOptions && strings.HasSuffix(path, "/_apis"): + _, _ = io.WriteString(w, oddLocations) + return + case strings.HasSuffix(path, "/_apis/ResourceAreas"): + _, _ = io.WriteString(w, `{"count": 0, "value": []}`) + return + } + f.mu.Lock() + body, ok := f.replies[path[strings.LastIndex(path, "/")+1:]] + f.mu.Unlock() + if !ok { + http.Error(w, "unexpected "+r.Method+" "+path, http.StatusNotFound) + return + } + _, _ = io.WriteString(w, body) +} + +var oddBaselineReplies = map[string]string{ + "pullRequestQuery": oddPRQueryReply, + "commits": oddPRCommitsReply, + "reviewers": oddPRReviewersReply, +} + +// oddAllowedErrors lists the values whose null may fail evidence collection, +// keyed by reply and path with list indexes written [i]. +var oddAllowedErrors = map[string]string{ + "pullRequestQuery results[i]." + oddMergeCommit + "[i].pullRequestId": "the commits and reviewers requests need it", + "commits value[i].commitId": "a commit with no id reads the same as a null one", +} + +// oddAllowedSkips lists the list items whose null may be left out instead of +// failing, keyed as oddAllowedErrors is. +var oddAllowedSkips = map[string]string{ + "reviewers value[i]": "a null reviewer reads as one with no vote, which has never been a review", +} + +// TestPREvidenceOddReplies nulls each value (and empties each list) of each +// Azure DevOps reply in turn, through V1 (`assert pullrequest azure`) and V2, +// and checks the evidence code never panics, fails only on a null list item +// or an allowed value, and otherwise loses only what that value fed. +func TestPREvidenceOddReplies(t *testing.T) { + fake := &fakeAzureDevOps{} + server := httptest.NewServer(fake) + t.Cleanup(server.Close) + config := NewAzureConfig("pat", server.URL+"/fabrikam", "Fabrikam-Fiber-Git", "Fabrikam-Fiber-Git") + + entryPoints := []struct { + name string + fetch func(string) ([]*types.PREvidence, error) + replies []string + }{ + {"V1", config.PREvidenceForCommitV1, []string{"pullRequestQuery", "reviewers"}}, + {"V2", config.PREvidenceForCommitV2, []string{"pullRequestQuery", "commits", "reviewers"}}, + } + for _, entry := range entryPoints { + fake.set(oddBaselineReplies) + baseline, err := entry.fetch(oddMergeCommit) + require.NoError(t, err) + require.Len(t, baseline, 1) + if entry.name == "V2" { + requireFullAzureEvidence(t, baseline[0]) + } else { + require.Equal(t, []any{"Normal Paulk (fabrikamfiber16@hotmail.com)"}, baseline[0].Approvers) + } + + for _, reply := range entry.replies { + variants, err := testHelpers.NullVariants([]byte(oddBaselineReplies[reply])) + require.NoError(t, err) + for _, v := range variants { + t.Run(entry.name+"/"+reply+"/"+v.Path, func(t *testing.T) { + replies := map[string]string{} + for k, body := range oddBaselineReplies { + replies[k] = body + } + replies[reply] = string(v.Body) + fake.set(replies) + + outcome := testHelpers.EvidenceOutcome(baseline, func() ([]*types.PREvidence, error) { + return entry.fetch(oddMergeCommit) + }) + requireOddRule(t, reply, v.Path, outcome) + }) + } + } + } +} + +var listIndex = regexp.MustCompile(`\[\d+\]`) + +func requireOddRule(t *testing.T, reply, path, outcome string) { + t.Helper() + require.False(t, strings.HasPrefix(outcome, "panic"), outcome) + key := reply + " " + listIndex.ReplaceAllString(path, "[i]") + feeds := oddFeeds(reply, strings.TrimSuffix(path, "[]")) + readItem := strings.HasSuffix(path, "]") && !strings.HasSuffix(path, "[]") && len(feeds) > 0 + if outcome == "error" { + _, allowed := oddAllowedErrors[key] + require.True(t, allowed || readItem, "unexpected error") + return + } + if readItem { + _, skip := oddAllowedSkips[key] + require.True(t, skip, "a null list item that is read must fail, got %q", outcome) + } + for _, diff := range strings.Fields(outcome)[1:] { + field := diff[strings.Index(diff, ":")+1:] + require.True(t, slices.Contains(feeds, field) || slices.Contains(feeds, "*"), + "%s: %s is not fed by this value", diff, field) + } +} + +// oddFeeds names the evidence keys a reply value is read into; "*" for a +// value that holds the pull request itself. +func oddFeeds(reply, path string) []string { + switch reply { + case "commits": + return []string{"commits"} + case "reviewers": + return []string{"approvers", "reviews"} + } + pr := "results[0]." + oddMergeCommit + "[0]" + if path == "." || strings.HasPrefix(pr, path) { + return []string{"*"} + } + field, ok := strings.CutPrefix(path, pr+".") + if !ok { + return nil + } + field, _, _ = strings.Cut(field, ".") + return map[string][]string{ + "pullRequestId": {"*"}, + "status": {"state", "merged_at"}, + "createdBy": {"author"}, + "creationDate": {"created_at"}, + "closedDate": {"merged_at"}, + "title": {"title"}, + "sourceRefName": {"head_ref", "commits"}, + "targetRefName": {"base_ref"}, + "lastMergeCommit": {"merge_commit"}, + }[field] +} + +// requireFullAzureEvidence checks the baseline fills every field Azure +// evidence can carry. HeadSHA is never set for Azure. +func requireFullAzureEvidence(t *testing.T, e *types.PREvidence) { + t.Helper() + v := reflect.ValueOf(*e) + for i := 0; i < v.NumField(); i++ { + name := v.Type().Field(i).Name + if name == "HeadSHA" { + continue + } + require.False(t, v.Field(i).IsZero(), "baseline evidence has no %s", name) + } + require.Len(t, e.Commits, 2) + require.Len(t, e.Approvers, 1) + require.Equal(t, []types.PRApprovals{ + {Username: "Normal Paulk (fabrikamfiber16@hotmail.com)", State: types.ReviewApproved}, + {Username: "Johnnie McLeod (fabrikamfiber2@hotmail.com)", State: types.ReviewChangesRequested}, + }, e.Reviews) +} From deaffee954c85a7ad1eea77518fa6d0ee2106a05 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Wed, 21 Oct 2026 18:15:35 +0100 Subject: [PATCH 3/7] fix(azure): read a null pull request query result as no pull requests Main read a null result in the query reply as no pull requests. The first #1283 commit made it an error, which could fail an attestation that works today if Azure sends that shape for a commit with no PR. No evidence is lost by keeping main's reading. Part of #1283. --- internal/azure/azure.go | 4 +--- internal/azure/odd_replies_test.go | 3 ++- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/azure/azure.go b/internal/azure/azure.go index a14c19bba..360f77eb1 100644 --- a/internal/azure/azure.go +++ b/internal/azure/azure.go @@ -247,9 +247,7 @@ func (c *AzureConfig) PullRequestsForCommit(commit string) ([]git.GitPullRequest if len(results) == 0 { return []git.GitPullRequest{}, nil } - if results[0] == nil { - return nil, fmt.Errorf("pull request query for commit %s has a null result", commit) - } + // A null result indexes as an empty map, so it reads as no pull requests. prs := results[0][commit] for i, pr := range prs { // A null pull request decodes to an empty one, so no id is the only sign. diff --git a/internal/azure/odd_replies_test.go b/internal/azure/odd_replies_test.go index c9a9e527f..0f0b55c48 100644 --- a/internal/azure/odd_replies_test.go +++ b/internal/azure/odd_replies_test.go @@ -178,7 +178,8 @@ var oddAllowedErrors = map[string]string{ // oddAllowedSkips lists the list items whose null may be left out instead of // failing, keyed as oddAllowedErrors is. var oddAllowedSkips = map[string]string{ - "reviewers value[i]": "a null reviewer reads as one with no vote, which has never been a review", + "reviewers value[i]": "a null reviewer reads as one with no vote, which has never been a review", + "pullRequestQuery results[i]": "a null result reads as no pull requests, as on main; Azure may send it for a commit with none", } // TestPREvidenceOddReplies nulls each value (and empties each list) of each From 95569e73b71bc286d67a995f9de8b7c08f8639c2 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 22 Oct 2026 17:17:00 +0100 Subject: [PATCH 4/7] 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 642a1fce7f646efcbc808e3e304f6bb3589c6227 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 22 Oct 2026 17:24:14 +0100 Subject: [PATCH 5/7] fix(azure): error on a reviewer vote with no identity A reviewer who voted but has neither displayName nor uniqueName was counted as " ()". For an approval that records evidence attributed to nobody, which fails open. A change-request vote with no identity is the same unreadable entry, so any counted vote errors, naming the reviewer's index. The check sits where reviewers are fetched, so V1 and V2 share it. Also name a pull request with a nil id the same way in both GetPullRequestCommits errors. Part of #1283. Co-Authored-By: Claude Opus 5.5 --- internal/azure/azure.go | 20 +++++++----- internal/azure/odd_replies_test.go | 50 ++++++++++++++++++++++++++++++ internal/azure/pr_commits_test.go | 14 +++++++++ 3 files changed, 77 insertions(+), 7 deletions(-) diff --git a/internal/azure/azure.go b/internal/azure/azure.go index 360f77eb1..2a0f836de 100644 --- a/internal/azure/azure.go +++ b/internal/azure/azure.go @@ -174,6 +174,12 @@ func azurePRRefs(pr git.GitPullRequest) (head, base string) { // GetPullRequestCommits returns a list of commits for a given pull request func (c *AzureConfig) GetPullRequestCommits(pr git.GitPullRequest) ([]types.Commit, error) { commits := []types.Commit{} + // A nil id is the one input the SDK rejects outright, so it can only be + // named here, not read from the reply. + prID := "with no id" + if pr.PullRequestId != nil { + prID = strconv.Itoa(*pr.PullRequestId) + } ctx := context.Background() client, err := newPRCommitsClient(ctx, c.Token, c.OrgURL) @@ -187,19 +193,13 @@ func (c *AzureConfig) GetPullRequestCommits(pr git.GitPullRequest) ([]types.Comm Project: &c.Project, }, defaultMaxPages) if err != nil { - // A nil id is the one input the SDK rejects outright, so it is also the - // only one that reaches here without an id to name. - prID := "with no id" - if pr.PullRequestId != nil { - prID = strconv.Itoa(*pr.PullRequestId) - } return commits, fmt.Errorf("draining commits for pull request %s: %w", prID, err) } for i, commit := range prCommits { // A null commit decodes to an empty one, so no id is the only sign. if commit.CommitId == nil { - return commits, fmt.Errorf("commit %d of pull request %d has no commitId", i, value(pr.PullRequestId)) + return commits, fmt.Errorf("commit %d of pull request %s has no commitId", i, prID) } commits = append(commits, commitFromAzureCommit(commit, value(pr.SourceRefName))) } @@ -291,6 +291,12 @@ func (c *AzureConfig) pullRequestReviewers(prNumber int) ([]git.IdentityRefWithV if err != nil { return nil, err } + for i, r := range value(reviewers) { + // Counting a vote from nobody would record evidence attributed to no one. + if value(r.Vote) != 0 && value(r.DisplayName) == "" && value(r.UniqueName) == "" { + return nil, fmt.Errorf("reviewer %d of pull request %d has no displayName or uniqueName", i, prNumber) + } + } return value(reviewers), nil } diff --git a/internal/azure/odd_replies_test.go b/internal/azure/odd_replies_test.go index 0f0b55c48..04bf3a711 100644 --- a/internal/azure/odd_replies_test.go +++ b/internal/azure/odd_replies_test.go @@ -1,6 +1,7 @@ package azure import ( + "fmt" "io" "net/http" "net/http/httptest" @@ -307,3 +308,52 @@ func requireFullAzureEvidence(t *testing.T, e *types.PREvidence) { {Username: "Johnnie McLeod (fabrikamfiber2@hotmail.com)", State: types.ReviewChangesRequested}, }, e.Reviews) } + +// A counted vote (10 approved through -10 rejected) from a reviewer with no +// displayName or uniqueName is a review attributed to nobody, so it errors +// through V1 and V2; a reviewer with no vote stays skipped. +func TestPREvidenceReviewerWithNoIdentity(t *testing.T) { + fake := &fakeAzureDevOps{} + server := httptest.NewServer(fake) + t.Cleanup(server.Close) + config := NewAzureConfig("pat", server.URL+"/fabrikam", "Fabrikam-Fiber-Git", "Fabrikam-Fiber-Git") + + for _, tc := range []struct { + name string + vote int + wantErr string + }{ + {"approved", 10, "reviewer 0 of pull request 22 has no displayName or uniqueName"}, + {"approved with suggestions", 5, "reviewer 0 of pull request 22 has no displayName or uniqueName"}, + {"waiting for author", -5, "reviewer 0 of pull request 22 has no displayName or uniqueName"}, + {"rejected", -10, "reviewer 0 of pull request 22 has no displayName or uniqueName"}, + {"no vote", 0, ""}, + } { + for _, entry := range []struct { + name string + fetch func(string) ([]*types.PREvidence, error) + }{ + {"V1", config.PREvidenceForCommitV1}, + {"V2", config.PREvidenceForCommitV2}, + } { + t.Run(entry.name+"/"+tc.name, func(t *testing.T) { + replies := map[string]string{} + for k, body := range oddBaselineReplies { + replies[k] = body + } + replies["reviewers"] = fmt.Sprintf(`{"count": 2, "value": [ + {"vote": %d, "id": "d6245f20-2af8-44f4-9451-8107cb2767db"}, + {"vote": 10, "displayName": "Johnnie McLeod", "uniqueName": "fabrikamfiber2@hotmail.com"} +]}`, tc.vote) + fake.set(replies) + + _, err := entry.fetch(oddMergeCommit) + if tc.wantErr == "" { + require.NoError(t, err) + } else { + require.EqualError(t, err, tc.wantErr) + } + }) + } + } +} diff --git a/internal/azure/pr_commits_test.go b/internal/azure/pr_commits_test.go index 0f2caf470..0c2a5cb71 100644 --- a/internal/azure/pr_commits_test.go +++ b/internal/azure/pr_commits_test.go @@ -154,3 +154,17 @@ func TestGetPullRequestCommits_NilPullRequestIDReturnsErrorNotPanic(t *testing.T require.ErrorContains(t, err, "args.PullRequestId", "the SDK's reason must survive") require.ErrorContains(t, err, "pull request", "the wrap must still say what failed") } + +func TestGetPullRequestCommits_NilPullRequestIDNamedTheSameInEveryError(t *testing.T) { + client := &fakePRCommitsClient{pages: []*git.GetPullRequestCommitsResponseValue{ + {Value: []git.GitCommitRef{{}}}, + }} + original := newPRCommitsClient + newPRCommitsClient = func(context.Context, string, string) (prCommitsClient, error) { return client, nil } + t.Cleanup(func() { newPRCommitsClient = original }) + + config := &AzureConfig{Token: "t", OrgURL: "https://dev.azure.com/o", Project: "p", Repository: "r"} + _, err := config.GetPullRequestCommits(git.GitPullRequest{}) + + require.EqualError(t, err, "commit 0 of pull request with no id has no commitId") +} From bba2e6006e8175c1b12e9d72962a478e2b2dc4f8 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 22 Oct 2026 17:28:32 +0100 Subject: [PATCH 6/7] 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 64538696be8e258f3149ea3ee7e0615ee2f91f81 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 22 Oct 2026 17:29:01 +0100 Subject: [PATCH 7/7] fix(azure): leave nameless reviewers out of approvers instead of erroring The last commit made a vote from a reviewer with no displayName or uniqueName an error. Deleted accounts come back with no identity, so any PR once approved by someone who has since left would fail attestation for good, with no workaround. Such a reviewer is now left out of the approvers, so it never counts as an approval, and kept in reviews with an empty name, so the record still shows an unknown account reviewed. Part of #1283. Co-Authored-By: Claude Opus 5.5 --- internal/azure/azure.go | 14 ++--- internal/azure/odd_replies_test.go | 90 +++++++++++++++++++----------- 2 files changed, 64 insertions(+), 40 deletions(-) diff --git a/internal/azure/azure.go b/internal/azure/azure.go index 2a0f836de..f3d5925aa 100644 --- a/internal/azure/azure.go +++ b/internal/azure/azure.go @@ -291,24 +291,22 @@ func (c *AzureConfig) pullRequestReviewers(prNumber int) ([]git.IdentityRefWithV if err != nil { return nil, err } - for i, r := range value(reviewers) { - // Counting a vote from nobody would record evidence attributed to no one. - if value(r.Vote) != 0 && value(r.DisplayName) == "" && value(r.UniqueName) == "" { - return nil, fmt.Errorf("reviewer %d of pull request %d has no displayName or uniqueName", i, prNumber) - } - } return value(reviewers), nil } +// azureReviewerName gives "" for a reviewer with no identity, such as a +// deleted account, so its vote is never counted as an approval. func azureReviewerName(r git.IdentityRefWithVote) string { + if value(r.DisplayName) == "" && value(r.UniqueName) == "" { + return "" + } return fmt.Sprintf("%s (%s)", value(r.DisplayName), value(r.UniqueName)) } func azureApprovers(reviewers []git.IdentityRefWithVote, version int) []any { var approvers []any for _, r := range reviewers { - if value(r.Vote) == 10 { - approverName := azureReviewerName(r) + if approverName := azureReviewerName(r); value(r.Vote) == 10 && approverName != "" { if version == 1 { approvers = append(approvers, approverName) } else { diff --git a/internal/azure/odd_replies_test.go b/internal/azure/odd_replies_test.go index 04bf3a711..6cdaf0869 100644 --- a/internal/azure/odd_replies_test.go +++ b/internal/azure/odd_replies_test.go @@ -309,51 +309,77 @@ func requireFullAzureEvidence(t *testing.T, e *types.PREvidence) { }, e.Reviews) } -// A counted vote (10 approved through -10 rejected) from a reviewer with no -// displayName or uniqueName is a review attributed to nobody, so it errors -// through V1 and V2; a reviewer with no vote stays skipped. +// A reviewer with no displayName or uniqueName (a deleted account) is left +// out of approvers, so it never counts as an approval, but is kept in reviews +// with an empty name. It never fails evidence collection. func TestPREvidenceReviewerWithNoIdentity(t *testing.T) { fake := &fakeAzureDevOps{} server := httptest.NewServer(fake) t.Cleanup(server.Close) config := NewAzureConfig("pat", server.URL+"/fabrikam", "Fabrikam-Fiber-Git", "Fabrikam-Fiber-Git") + known := "Johnnie McLeod (fabrikamfiber2@hotmail.com)" for _, tc := range []struct { - name string - vote int - wantErr string + name string + vote int + state string // the nameless review's state; "" for none }{ - {"approved", 10, "reviewer 0 of pull request 22 has no displayName or uniqueName"}, - {"approved with suggestions", 5, "reviewer 0 of pull request 22 has no displayName or uniqueName"}, - {"waiting for author", -5, "reviewer 0 of pull request 22 has no displayName or uniqueName"}, - {"rejected", -10, "reviewer 0 of pull request 22 has no displayName or uniqueName"}, + {"approved", 10, types.ReviewApproved}, + {"approved with suggestions", 5, types.ReviewApproved}, + {"waiting for author", -5, types.ReviewChangesRequested}, + {"rejected", -10, types.ReviewChangesRequested}, {"no vote", 0, ""}, } { - for _, entry := range []struct { - name string - fetch func(string) ([]*types.PREvidence, error) - }{ - {"V1", config.PREvidenceForCommitV1}, - {"V2", config.PREvidenceForCommitV2}, - } { - t.Run(entry.name+"/"+tc.name, func(t *testing.T) { - replies := map[string]string{} - for k, body := range oddBaselineReplies { - replies[k] = body - } - replies["reviewers"] = fmt.Sprintf(`{"count": 2, "value": [ + t.Run(tc.name, func(t *testing.T) { + replies := map[string]string{} + for k, body := range oddBaselineReplies { + replies[k] = body + } + replies["reviewers"] = fmt.Sprintf(`{"count": 2, "value": [ {"vote": %d, "id": "d6245f20-2af8-44f4-9451-8107cb2767db"}, {"vote": 10, "displayName": "Johnnie McLeod", "uniqueName": "fabrikamfiber2@hotmail.com"} ]}`, tc.vote) - fake.set(replies) + fake.set(replies) - _, err := entry.fetch(oddMergeCommit) - if tc.wantErr == "" { - require.NoError(t, err) - } else { - require.EqualError(t, err, tc.wantErr) - } - }) - } + v1, err := config.PREvidenceForCommitV1(oddMergeCommit) + require.NoError(t, err) + require.Equal(t, []any{known}, v1[0].Approvers) + + v2, err := config.PREvidenceForCommitV2(oddMergeCommit) + require.NoError(t, err) + require.Equal(t, []any{types.PRApprovals{Username: known, State: "APPROVED"}}, v2[0].Approvers) + wantReviews := []types.PRApprovals{} + if tc.state != "" { + wantReviews = append(wantReviews, types.PRApprovals{Username: "", State: tc.state}) + } + wantReviews = append(wantReviews, types.PRApprovals{Username: known, State: types.ReviewApproved}) + require.Equal(t, wantReviews, v2[0].Reviews) + }) + } +} + +// One name is enough for an approval to count. +func TestPREvidenceReviewerWithOneName(t *testing.T) { + fake := &fakeAzureDevOps{} + server := httptest.NewServer(fake) + t.Cleanup(server.Close) + config := NewAzureConfig("pat", server.URL+"/fabrikam", "Fabrikam-Fiber-Git", "Fabrikam-Fiber-Git") + + for field, want := range map[string]string{ + "displayName": "Normal Paulk ()", + "uniqueName": " (Normal Paulk)", + } { + t.Run(field, func(t *testing.T) { + replies := map[string]string{} + for k, body := range oddBaselineReplies { + replies[k] = body + } + replies["reviewers"] = fmt.Sprintf(`{"count": 1, "value": [{"vote": 10, %q: "Normal Paulk"}]}`, field) + fake.set(replies) + + v1, err := config.PREvidenceForCommitV1(oddMergeCommit) + require.NoError(t, err) + require.Equal(t, []any{want}, v1[0].Approvers) + }) } }