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 21cc91e05566ac3f1455b58b7ed96e4baf01ea17 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Wed, 21 Oct 2026 18:05:01 +0100 Subject: [PATCH 2/7] fix(bitbucket): never crash on a null field in a Bitbucket reply Every field was read with an unchecked type assertion, so a null or missing value (an approver with no state, a PR with no author) panicked with a Go stack trace. Fields are now read through one set of safe getters that give the zero value, and empty timestamps parse to 0. A null item in a list we read (values, participants) is an error naming the list and index, not a silent skip. The only null fields that still fail are the PR's links.self.href and id, which the next request needs. The test nulls each value of realistic replies in turn through both PREvidenceForCommitV1 (assert) and V2 (attest) and asserts the rule. Part of #1283 Co-Authored-By: Claude Opus 5.5 --- internal/bitbucket/bitbucket.go | 151 +++++++----- internal/bitbucket/odd_replies_test.go | 314 +++++++++++++++++++++++++ internal/bitbucket/reviews_test.go | 4 +- 3 files changed, 407 insertions(+), 62 deletions(-) create mode 100644 internal/bitbucket/odd_replies_test.go diff --git a/internal/bitbucket/bitbucket.go b/internal/bitbucket/bitbucket.go index bb28c847e..fa717ff15 100644 --- a/internal/bitbucket/bitbucket.go +++ b/internal/bitbucket/bitbucket.go @@ -26,7 +26,11 @@ type Config struct { // parseRFC3339NanoTimestamp parses a timestamp string in RFC3339Nano format and returns its Unix timestamp. // The fieldName parameter is used for error messages to identify which field failed to parse. +// An empty string gives 0: the field was missing. func parseRFC3339NanoTimestamp(timestampStr, fieldName string) (int64, error) { + if timestampStr == "" { + return 0, nil + } parsedTime, err := time.Parse(time.RFC3339Nano, timestampStr) if err != nil { return 0, fmt.Errorf("failed to parse %s timestamp: %w", fieldName, err) @@ -34,6 +38,38 @@ func parseRFC3339NanoTimestamp(timestampStr, fieldName string) (int64, error) { return parsedTime.Unix(), nil } +// str, obj and boolean read a field from a decoded JSON object, giving the +// zero value when it is missing, null or of another type. +func str(m map[string]any, key string) string { + v, _ := m[key].(string) + return v +} + +func obj(m map[string]any, key string) map[string]any { + v, _ := m[key].(map[string]any) + return v +} + +func boolean(m map[string]any, key string) bool { + v, _ := m[key].(bool) + return v +} + +// objects reads a list of objects. A missing list is empty, but an item that +// is not an object is an error: it is an entry we can't read. +func objects(m map[string]any, key string) ([]map[string]any, error) { + items, _ := m[key].([]any) + out := make([]map[string]any, len(items)) + for i, item := range items { + o, ok := item.(map[string]any) + if !ok { + return nil, fmt.Errorf("%s[%d] in the Bitbucket reply is null or not an object", key, i) + } + out[i] = o + } + return out, nil +} + func (c *Config) ProviderAndLabel() (string, string) { return "bitbucket", "pull request" } @@ -97,16 +133,17 @@ func (c *Config) parseBitbucketResponse(commit string, response *requests.HTTPRe if err != nil { return pullRequestsEvidence, err } - pullRequests, ok := responseData["values"].([]any) - if !ok { - return pullRequestsEvidence, nil + pullRequests, err := objects(responseData, "values") + if err != nil { + return pullRequestsEvidence, err } - for _, prInterface := range pullRequests { - pr := prInterface.(map[string]any) - linksInterface := pr["links"].(map[string]any) - apiLinkMap := linksInterface["self"].(map[string]any) - htmlLinkMap := linksInterface["html"].(map[string]any) - evidence, err := c.getPullRequestDetailsFromBitbucket(apiLinkMap["href"].(string), htmlLinkMap["href"].(string), commit, version) + for _, pr := range pullRequests { + links := obj(pr, "links") + apiURL := str(obj(links, "self"), "href") + if apiURL == "" { + return pullRequestsEvidence, fmt.Errorf("a pull request in the Bitbucket reply has no links.self.href") + } + evidence, err := c.getPullRequestDetailsFromBitbucket(apiURL, str(obj(links, "html"), "href"), commit, version) if err != nil { return pullRequestsEvidence, err } @@ -141,25 +178,27 @@ func (c *Config) getPullRequestDetailsFromBitbucket(prApiUrl, prHtmlLink, commit evidence.URL = prHtmlLink evidence.MergeCommit = commit - evidence.State = responseData["state"].(string) - participants := responseData["participants"].([]any) + evidence.State = str(responseData, "state") + participants, err := objects(responseData, "participants") + if err != nil { + return evidence, err + } approvers := []any{} if len(participants) > 0 { - for _, participantInterface := range participants { - p := participantInterface.(map[string]any) - if p["approved"].(bool) { - user := p["user"].(map[string]any) + for _, p := range participants { + if boolean(p, "approved") { + username := str(obj(p, "user"), "display_name") if version == 1 { - approvers = append(approvers, user["display_name"].(string)) + approvers = append(approvers, username) } else { - approvalTimestamp, err := parseRFC3339NanoTimestamp(p["participated_on"].(string), "participated_on") + approvalTimestamp, err := parseRFC3339NanoTimestamp(str(p, "participated_on"), "participated_on") if err != nil { return evidence, err } approvers = append(approvers, types.PRApprovals{ - Username: user["display_name"].(string), - State: p["state"].(string), + Username: username, + State: str(p, "state"), Timestamp: approvalTimestamp, }) } @@ -170,22 +209,26 @@ func (c *Config) getPullRequestDetailsFromBitbucket(prApiUrl, prHtmlLink, commit c.Logger.Debug("no approvers found") } if version == 2 { - evidence.Author = responseData["author"].(map[string]any)["display_name"].(string) - createdAt, err := parseRFC3339NanoTimestamp(responseData["created_on"].(string), "created_on") + evidence.Author = str(obj(responseData, "author"), "display_name") + createdAt, err := parseRFC3339NanoTimestamp(str(responseData, "created_on"), "created_on") if err != nil { return evidence, err } evidence.CreatedAt = createdAt - mergedAt, err := parseRFC3339NanoTimestamp(responseData["updated_on"].(string), "updated_on") + mergedAt, err := parseRFC3339NanoTimestamp(str(responseData, "updated_on"), "updated_on") if err != nil { return evidence, err } evidence.MergedAt = mergedAt - evidence.Title = responseData["title"].(string) + evidence.Title = str(responseData, "title") evidence.HeadRef = bitbucketBranchName(responseData, "source") evidence.BaseRef = bitbucketBranchName(responseData, "destination") - prCommits, err := c.getPullRequestCommitsFromBitbucket(int(responseData["id"].(float64))) + prID, ok := responseData["id"].(float64) + if !ok { + return evidence, fmt.Errorf("the pull request at %s has no id", prApiUrl) + } + prCommits, err := c.getPullRequestCommitsFromBitbucket(int(prID)) if err != nil { return evidence, err } @@ -205,13 +248,11 @@ func (c *Config) getPullRequestDetailsFromBitbucket(prApiUrl, prHtmlLink, commit // bitbucketReviews gives each participant's current decision. A participant // with no state who has commented is a comment; one who has done neither is // left out. -func bitbucketReviews(participants []any) ([]types.PRApprovals, error) { +func bitbucketReviews(participants []map[string]any) ([]types.PRApprovals, error) { reviews := []types.PRApprovals{} - for _, participantInterface := range participants { - p := participantInterface.(map[string]any) - user, _ := p["user"].(map[string]any) - username, _ := user["display_name"].(string) - participatedOn, _ := p["participated_on"].(string) + for _, p := range participants { + username := str(obj(p, "user"), "display_name") + participatedOn := str(p, "participated_on") var state string switch p["state"] { case "approved": @@ -231,18 +272,15 @@ func bitbucketReviews(participants []any) ([]types.PRApprovals, error) { if username == "" && state != types.ReviewChangesRequested { continue } - review := types.PRApprovals{ - Username: username, - State: state, - } - if participatedOn != "" { - timestamp, err := parseRFC3339NanoTimestamp(participatedOn, "participated_on") - if err != nil { - return nil, err - } - review.Timestamp = timestamp + timestamp, err := parseRFC3339NanoTimestamp(participatedOn, "participated_on") + if err != nil { + return nil, err } - reviews = append(reviews, review) + reviews = append(reviews, types.PRApprovals{ + Username: username, + State: state, + Timestamp: timestamp, + }) } return reviews, nil } @@ -250,7 +288,7 @@ func bitbucketReviews(participants []any) ([]types.PRApprovals, error) { // bitbucketBranchName extracts a branch name from a Bitbucket PR response for // the given side: "source" for the head branch, "destination" for the base. func bitbucketBranchName(prData map[string]any, side string) string { - return prData[side].(map[string]any)["branch"].(map[string]any)["name"].(string) + return str(obj(obj(prData, side), "branch"), "name") } // getPullRequestCommitsFromBitbucket gets the commits of a pull request from the Bitbucket API @@ -286,36 +324,29 @@ func (c *Config) getPullRequestCommitsFromBitbucket(prID int) ([]types.Commit, e return nil, err } - commits, ok := responseData["values"].([]any) - if !ok { - break + commits, err := objects(responseData, "values") + if err != nil { + return nil, err } - for _, commitInterface := range commits { - commit := commitInterface.(map[string]any) - timestamp, err := parseRFC3339NanoTimestamp(commit["date"].(string), "date") + for _, commit := range commits { + timestamp, err := parseRFC3339NanoTimestamp(str(commit, "date"), "date") if err != nil { return nil, err } allCommits = append(allCommits, types.Commit{ - SHA: commit["hash"].(string), - Message: commit["message"].(string), - Author: commit["author"].(map[string]any)["raw"].(string), + SHA: str(commit, "hash"), + Message: str(commit, "message"), + Author: str(obj(commit, "author"), "raw"), Timestamp: timestamp, - URL: commit["links"].(map[string]any)["html"].(map[string]any)["href"].(string), + URL: str(obj(obj(commit, "links"), "html"), "href"), }) } - // Check for next page - nextInterface, hasNext := responseData["next"] - if !hasNext { - break - } - nextURL, ok := nextInterface.(string) - if !ok || nextURL == "" { + currentURL = str(responseData, "next") + if currentURL == "" { break } - currentURL = nextURL c.Logger.Debug("fetching next page of commits from %s", currentURL) } diff --git a/internal/bitbucket/odd_replies_test.go b/internal/bitbucket/odd_replies_test.go new file mode 100644 index 000000000..a09efea07 --- /dev/null +++ b/internal/bitbucket/odd_replies_test.go @@ -0,0 +1,314 @@ +package bitbucket + +import ( + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "regexp" + "strings" + "testing" + + "github.com/kosli-dev/cli/internal/logger" + "github.com/kosli-dev/cli/internal/requests" + "github.com/kosli-dev/cli/internal/testHelpers" + "github.com/kosli-dev/cli/internal/types" + "github.com/stretchr/testify/require" +) + +// Baseline replies for every call PREvidenceForCommitV2 makes. Shapes follow +// https://api.bitbucket.org/swagger.json: paginated_pullrequests (GET +// /repositories/{workspace}/{repo_slug}/commit/{commit}/pullrequests), +// pullrequest and participant (GET .../pullrequests/{pull_request_id}) and +// paginated_changeset of base_commit (GET .../pullrequests/{pull_request_id}/commits), +// as documented at +// https://developer.atlassian.com/cloud/bitbucket/rest/api-group-pullrequests/. +// {{base}} stands for https://api.bitbucket.org and is replaced with the test +// server's address when served, so links the code follows come back to it. +const ( + oddCommit = "4f1c2a9e8d7b6c5a4f3e2d1c0b9a8f7e6d5c4b3a" + + oddCommitPullRequests = `{ + "pagelen": 50, + "page": 1, + "size": 1, + "values": [{ + "type": "pullrequest", + "id": 7, + "title": "Add retry to uploads", + "state": "MERGED", + "author": {"type": "user", "display_name": "Ada Lovelace", "uuid": "{a1b2c3d4-0000-4000-8000-000000000001}", "account_id": "557058:ada", "nickname": "ada"}, + "source": {"branch": {"name": "feature/retry"}, "commit": {"type": "commit", "hash": "9a8b7c6d5e4f"}, "repository": {"type": "repository", "full_name": "acme/widgets", "name": "widgets", "uuid": "{a1b2c3d4-0000-4000-8000-0000000000aa}"}}, + "destination": {"branch": {"name": "main"}, "commit": {"type": "commit", "hash": "1a2b3c4d5e6f"}, "repository": {"type": "repository", "full_name": "acme/widgets", "name": "widgets", "uuid": "{a1b2c3d4-0000-4000-8000-0000000000aa}"}}, + "links": { + "self": {"href": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7"}, + "html": {"href": "https://bitbucket.org/acme/widgets/pull-requests/7"} + } + }] +}` + + oddPullRequest = `{ + "type": "pullrequest", + "id": 7, + "title": "Add retry to uploads", + "description": "Retries uploads on a 503.", + "state": "MERGED", + "draft": false, + "queued": false, + "reason": "", + "close_source_branch": true, + "comment_count": 2, + "task_count": 0, + "created_on": "2026-03-01T12:00:00.000000+00:00", + "updated_on": "2026-03-01T16:30:00.654321+00:00", + "author": {"type": "user", "display_name": "Ada Lovelace", "uuid": "{a1b2c3d4-0000-4000-8000-000000000001}", "account_id": "557058:ada", "nickname": "ada"}, + "closed_by": {"type": "user", "display_name": "Ada Lovelace", "uuid": "{a1b2c3d4-0000-4000-8000-000000000001}", "account_id": "557058:ada", "nickname": "ada"}, + "merge_commit": {"type": "commit", "hash": "4f1c2a9e8d7b"}, + "source": {"branch": {"name": "feature/retry"}, "commit": {"type": "commit", "hash": "9a8b7c6d5e4f"}, "repository": {"type": "repository", "full_name": "acme/widgets", "name": "widgets", "uuid": "{a1b2c3d4-0000-4000-8000-0000000000aa}"}}, + "destination": {"branch": {"name": "main"}, "commit": {"type": "commit", "hash": "1a2b3c4d5e6f"}, "repository": {"type": "repository", "full_name": "acme/widgets", "name": "widgets", "uuid": "{a1b2c3d4-0000-4000-8000-0000000000aa}"}}, + "reviewers": [ + {"type": "user", "display_name": "Grace Hopper", "uuid": "{a1b2c3d4-0000-4000-8000-000000000002}", "account_id": "557058:grace", "nickname": "grace"}, + {"type": "user", "display_name": "Alan Turing", "uuid": "{a1b2c3d4-0000-4000-8000-000000000003}", "account_id": "557058:alan", "nickname": "alan"} + ], + "participants": [ + {"type": "participant", "user": {"type": "user", "display_name": "Grace Hopper", "uuid": "{a1b2c3d4-0000-4000-8000-000000000002}", "account_id": "557058:grace", "nickname": "grace"}, "role": "REVIEWER", "approved": true, "state": "approved", "participated_on": "2026-03-01T13:00:00.123456+00:00"}, + {"type": "participant", "user": {"type": "user", "display_name": "Alan Turing", "uuid": "{a1b2c3d4-0000-4000-8000-000000000003}", "account_id": "557058:alan", "nickname": "alan"}, "role": "REVIEWER", "approved": false, "state": "changes_requested", "participated_on": "2026-03-01T14:00:00.000000+00:00"} + ], + "links": { + "self": {"href": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7"}, + "html": {"href": "https://bitbucket.org/acme/widgets/pull-requests/7"}, + "commits": {"href": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7/commits"} + } +}` + + // Commits come one per page so the code's paging is exercised. + oddPullRequestCommitsPage1 = `{ + "pagelen": 1, + "page": 1, + "values": [{ + "type": "commit", + "hash": "9a8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b", + "date": "2026-03-01T11:00:00+00:00", + "author": {"type": "author", "raw": "Ada Lovelace ", "user": {"type": "user", "display_name": "Ada Lovelace", "uuid": "{a1b2c3d4-0000-4000-8000-000000000001}"}}, + "message": "Add retry to uploads\n", + "parents": [{"type": "commit", "hash": "8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b9c"}], + "links": { + "self": {"href": "{{base}}/2.0/repositories/acme/widgets/commit/9a8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b"}, + "html": {"href": "https://bitbucket.org/acme/widgets/commits/9a8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b"} + } + }], + "next": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7/commits?page=2" +}` + + oddPullRequestCommitsPage2 = `{ + "pagelen": 1, + "page": 2, + "values": [{ + "type": "commit", + "hash": "8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b9c", + "date": "2026-03-01T10:00:00+00:00", + "author": {"type": "author", "raw": "Ada Lovelace ", "user": {"type": "user", "display_name": "Ada Lovelace", "uuid": "{a1b2c3d4-0000-4000-8000-000000000001}"}}, + "message": "Add upload test\n", + "parents": [{"type": "commit", "hash": "1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b"}], + "links": { + "self": {"href": "{{base}}/2.0/repositories/acme/widgets/commit/8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b9c"}, + "html": {"href": "https://bitbucket.org/acme/widgets/commits/8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b9c"} + } + }], + "previous": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7/commits?page=1" +}` +) + +// oddRoutes maps each reply name to the request that gets it. +var oddRoutes = map[string]string{ + "commit_pullrequests": "/2.0/repositories/acme/widgets/commit/" + oddCommit + "/pullrequests", + "pullrequest": "/2.0/repositories/acme/widgets/pullrequests/7", + "pullrequest_commits_1": "/2.0/repositories/acme/widgets/pullrequests/7/commits", + "pullrequest_commits_2": "/2.0/repositories/acme/widgets/pullrequests/7/commits?page=2", +} + +func oddBaselineReplies() map[string]string { + return map[string]string{ + "commit_pullrequests": oddCommitPullRequests, + "pullrequest": oddPullRequest, + "pullrequest_commits_1": oddPullRequestCommitsPage1, + "pullrequest_commits_2": oddPullRequestCommitsPage2, + } +} + +// redirectTransport sends requests for api.bitbucket.org to the test server +// and refuses any other host, so the test never reaches the network. +type redirectTransport struct { + server *url.URL +} + +func (rt redirectTransport) RoundTrip(req *http.Request) (*http.Response, error) { + switch req.URL.Host { + case "api.bitbucket.org": + req = req.Clone(req.Context()) + req.URL.Scheme = rt.server.Scheme + req.URL.Host = rt.server.Host + req.Host = rt.server.Host + case rt.server.Host: + default: + return nil, fmt.Errorf("test refuses request to %s", req.URL) + } + return http.DefaultTransport.RoundTrip(req) +} + +// runOddReplies serves replies and collects evidence for oddCommit through +// the given entry point: 1 for PREvidenceForCommitV1, 2 for V2. +func runOddReplies(t *testing.T, version int, replies map[string]string) ([]*types.PREvidence, error) { + t.Helper() + var base string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + for name, route := range oddRoutes { + if r.URL.RequestURI() == route { + _, _ = w.Write([]byte(strings.ReplaceAll(replies[name], "{{base}}", base))) + return + } + } + http.NotFound(w, r) + })) + defer server.Close() + base = server.URL + serverURL, err := url.Parse(server.URL) + require.NoError(t, err) + + c := &Config{ + Workspace: "acme", + Repository: "widgets", + AccessToken: "test-token", + Logger: logger.NewStandardLogger(), + KosliClient: &requests.Client{ + Logger: logger.NewStandardLogger(), + HttpClient: &http.Client{Transport: redirectTransport{server: serverURL}}, + }, + } + if version == 1 { + return c.PREvidenceForCommitV1(oddCommit) + } + return c.PREvidenceForCommitV2(oddCommit) +} + +func TestOddReplies_BaselineGivesFullEvidenceV1(t *testing.T) { + evidence, err := runOddReplies(t, 1, oddBaselineReplies()) + require.NoError(t, err) + require.Equal(t, []*types.PREvidence{{ + MergeCommit: oddCommit, + URL: "https://bitbucket.org/acme/widgets/pull-requests/7", + State: "MERGED", + Approvers: []any{"Grace Hopper"}, + }}, evidence) +} + +func TestOddReplies_BaselineGivesFullEvidenceV2(t *testing.T) { + evidence, err := runOddReplies(t, 2, oddBaselineReplies()) + require.NoError(t, err) + require.Equal(t, []*types.PREvidence{{ + MergeCommit: oddCommit, + URL: "https://bitbucket.org/acme/widgets/pull-requests/7", + State: "MERGED", + Approvers: []any{types.PRApprovals{ + Username: "Grace Hopper", State: "approved", Timestamp: 1772370000, + }}, + Author: "Ada Lovelace", + CreatedAt: 1772366400, + MergedAt: 1772382600, + Title: "Add retry to uploads", + HeadRef: "feature/retry", + BaseRef: "main", + Commits: []types.Commit{ + { + SHA: "9a8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b", + Message: "Add retry to uploads\n", + Author: "Ada Lovelace ", + Timestamp: 1772362800, + URL: "https://bitbucket.org/acme/widgets/commits/9a8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b", + }, + { + SHA: "8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b9c", + Message: "Add upload test\n", + Author: "Ada Lovelace ", + Timestamp: 1772359200, + URL: "https://bitbucket.org/acme/widgets/commits/8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b9c", + }, + }, + Reviews: []types.PRApprovals{ + {Username: "Grace Hopper", State: types.ReviewApproved, Timestamp: 1772370000}, + {Username: "Alan Turing", State: types.ReviewChangesRequested, Timestamp: 1772373600}, + }, + }}, evidence) +} + +// oddAllowedErrors are the values the code needs for its next request, so +// making them (or anything holding them) null may fail. Keyed by entry point, +// reply and path. +var oddAllowedErrors = map[string]string{ + "V1 commit_pullrequests values[0].links.self.href": "the pull request's API URL, fetched next", + "V2 commit_pullrequests values[0].links.self.href": "the pull request's API URL, fetched next", + "V2 pullrequest id": "the pull request number, used to fetch its commits", +} + +// oddReadLists are the lists the code reads, per reply. A null item in one of +// them is an entry that can't be read, so it must fail. +var oddReadLists = map[string]string{ + "commit_pullrequests": "values", + "pullrequest": "participants", + "pullrequest_commits_1": "values", + "pullrequest_commits_2": "values", +} + +// oddMayError says whether path in reply holds, or is, a value in +// oddAllowedErrors for that entry point. +func oddMayError(entry, reply, path string) bool { + for key := range oddAllowedErrors { + allowed, ok := strings.CutPrefix(key, entry+" "+reply+" ") + if !ok { + continue + } + if path == "." || allowed == path || strings.HasPrefix(allowed, path+".") || strings.HasPrefix(allowed, path+"[") { + return true + } + } + return false +} + +// TestOddReplies makes each value in each reply null in turn, and empties each +// list, and checks the rule of kosli-dev/cli#1283 through both entry points: +// never a panic; a null field is no error unless oddAllowedErrors lists it; a +// null item in a list the code reads is an error; an emptied list is no error. +func TestOddReplies(t *testing.T) { + for _, version := range []int{1, 2} { + entry := fmt.Sprintf("V%d", version) + baseline, err := runOddReplies(t, version, oddBaselineReplies()) + require.NoError(t, err) + + for name := range oddRoutes { + if version == 1 && strings.HasPrefix(name, "pullrequest_commits") { + continue // V1 doesn't fetch commits. + } + variants, err := testHelpers.NullVariants([]byte(oddBaselineReplies()[name])) + require.NoError(t, err, name) + for _, v := range variants { + t.Run(entry+" "+name+" "+v.Path, func(t *testing.T) { + replies := oddBaselineReplies() + replies[name] = string(v.Body) + outcome := testHelpers.EvidenceOutcome(baseline, func() ([]*types.PREvidence, error) { + return runOddReplies(t, version, replies) + }) + require.False(t, strings.HasPrefix(outcome, "panic"), outcome) + switch { + case strings.HasSuffix(v.Path, "[]"): + require.True(t, strings.HasPrefix(outcome, "ok"), "an emptied list must not fail: %s", outcome) + case regexp.MustCompile(`^` + oddReadLists[name] + `\[\d+\]$`).MatchString(v.Path): + require.Equal(t, "error", outcome, "a null list item must fail") + case oddMayError(entry, name, v.Path): + default: + require.True(t, strings.HasPrefix(outcome, "ok"), "a null field must not fail: %s", outcome) + } + }) + } + } + } +} diff --git a/internal/bitbucket/reviews_test.go b/internal/bitbucket/reviews_test.go index 3a0c20fe0..013cb5033 100644 --- a/internal/bitbucket/reviews_test.go +++ b/internal/bitbucket/reviews_test.go @@ -12,7 +12,7 @@ import ( // https://api.bitbucket.org/swagger.json: state is approved, changes_requested // or null; participated_on is the approval time, else the last comment, else null. func TestBitbucketReviews(t *testing.T) { - var participants []any + var participants []map[string]any require.NoError(t, json.Unmarshal([]byte(`[ {"user":{"display_name":"Ada"},"role":"REVIEWER","approved":true,"state":"approved","participated_on":"2026-03-01T13:00:00.123456+00:00"}, {"user":{"display_name":"Bob"},"role":"REVIEWER","approved":false,"state":"changes_requested","participated_on":"2026-03-01T14:00:00+00:00"}, @@ -35,7 +35,7 @@ func TestBitbucketReviews(t *testing.T) { } func TestBitbucketReviews_RejectsABadTimestamp(t *testing.T) { - var participants []any + var participants []map[string]any require.NoError(t, json.Unmarshal([]byte(`[ {"user":{"display_name":"Ada"},"approved":true,"state":"approved","participated_on":"yesterday"} ]`), &participants)) From 95569e73b71bc286d67a995f9de8b7c08f8639c2 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 22 Oct 2026 17:17:00 +0100 Subject: [PATCH 3/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 a05283941782c16094b2a09adcff22efa2ad73e6 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 22 Oct 2026 17:24:10 +0100 Subject: [PATCH 4/7] fix(bitbucket): reject nameless approvals and commit pages that don't advance An approved participant with no user or display_name was counted as an approval by nobody, which records approval evidence that fails open. It is now an entry we can't read and errors, naming participants[i], for both assert (V1) and attest (V2). Commit paging followed next even when it pointed at the page just fetched, so such a reply looped forever. It now errors instead, matching the gitlab and github "did not advance" guards. Part of #1283 Co-Authored-By: Claude Opus 5.5 --- internal/bitbucket/bitbucket.go | 12 +++++++-- internal/bitbucket/odd_replies_test.go | 34 ++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/internal/bitbucket/bitbucket.go b/internal/bitbucket/bitbucket.go index fa717ff15..fb5843e96 100644 --- a/internal/bitbucket/bitbucket.go +++ b/internal/bitbucket/bitbucket.go @@ -186,9 +186,13 @@ func (c *Config) getPullRequestDetailsFromBitbucket(prApiUrl, prHtmlLink, commit approvers := []any{} if len(participants) > 0 { - for _, p := range participants { + for i, p := range participants { if boolean(p, "approved") { username := str(obj(p, "user"), "display_name") + // An approval by nobody would count as approval evidence. + if username == "" { + return evidence, fmt.Errorf("participants[%d] in the Bitbucket reply is an approval with no user.display_name", i) + } if version == 1 { approvers = append(approvers, username) } else { @@ -343,7 +347,11 @@ func (c *Config) getPullRequestCommitsFromBitbucket(prID int) ([]types.Commit, e }) } - currentURL = str(responseData, "next") + next := str(responseData, "next") + if next == currentURL { + return nil, fmt.Errorf("next page did not advance past page %s for pull request %d", currentURL, prID) + } + currentURL = next if currentURL == "" { break } diff --git a/internal/bitbucket/odd_replies_test.go b/internal/bitbucket/odd_replies_test.go index a09efea07..4d812ebbb 100644 --- a/internal/bitbucket/odd_replies_test.go +++ b/internal/bitbucket/odd_replies_test.go @@ -8,6 +8,7 @@ import ( "regexp" "strings" "testing" + "time" "github.com/kosli-dev/cli/internal/logger" "github.com/kosli-dev/cli/internal/requests" @@ -259,6 +260,14 @@ var oddReadLists = map[string]string{ "pullrequest_commits_2": "values", } +// oddUnreadableApprovals are approvals with no identity. Counting one would +// record approval evidence attributed to nobody, so it must fail, naming the +// entry. Keyed by reply and path. +var oddUnreadableApprovals = map[string]string{ + "pullrequest participants[0].user": "participants[0]", + "pullrequest participants[0].user.display_name": "participants[0]", +} + // oddMayError says whether path in reply holds, or is, a value in // oddAllowedErrors for that entry point. func oddMayError(entry, reply, path string) bool { @@ -303,6 +312,9 @@ func TestOddReplies(t *testing.T) { require.True(t, strings.HasPrefix(outcome, "ok"), "an emptied list must not fail: %s", outcome) case regexp.MustCompile(`^` + oddReadLists[name] + `\[\d+\]$`).MatchString(v.Path): require.Equal(t, "error", outcome, "a null list item must fail") + case oddUnreadableApprovals[name+" "+v.Path] != "": + _, err := runOddReplies(t, version, replies) + require.ErrorContains(t, err, oddUnreadableApprovals[name+" "+v.Path]) case oddMayError(entry, name, v.Path): default: require.True(t, strings.HasPrefix(outcome, "ok"), "a null field must not fail: %s", outcome) @@ -312,3 +324,25 @@ func TestOddReplies(t *testing.T) { } } } + +// A commits page whose next link is itself would be fetched forever. +func TestOddReplies_CommitPageThatDoesNotAdvanceFails(t *testing.T) { + replies := oddBaselineReplies() + replies["pullrequest_commits_2"] = `{ + "pagelen": 1, + "page": 2, + "values": [], + "next": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7/commits?page=2" +}` + done := make(chan error, 1) + go func() { + _, err := runOddReplies(t, 2, replies) + done <- err + }() + select { + case err := <-done: + require.ErrorContains(t, err, "did not advance") + case <-time.After(10 * time.Second): + t.Fatal("paging did not stop") + } +} From bba2e6006e8175c1b12e9d72962a478e2b2dc4f8 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 22 Oct 2026 17:28:32 +0100 Subject: [PATCH 5/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 b49f8b954f7afa2e4121fa0a660bd31868e582cd Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 22 Oct 2026 17:29:19 +0100 Subject: [PATCH 6/7] fix(bitbucket): leave nameless approvals out of approvers, keep them in reviews The previous commit made an approval with no user or display_name an error. A deleted account is a legal reply with no identity, so any pull request it ever approved could never be attested again, with no workaround. Now an approval with no identity is left out of the approvers, so it never counts as an approval, and is kept in the reviews with an empty name and its state, so the record still shows an unknown account reviewed. One helper, approvedBy, decides it for V1 and V2. Part of #1283 Co-Authored-By: Claude Opus 5.5 --- internal/bitbucket/bitbucket.go | 24 ++++++++++++------------ internal/bitbucket/odd_replies_test.go | 22 +++++++++++++--------- internal/bitbucket/reviews_test.go | 5 ++++- 3 files changed, 29 insertions(+), 22 deletions(-) diff --git a/internal/bitbucket/bitbucket.go b/internal/bitbucket/bitbucket.go index fb5843e96..628ed8378 100644 --- a/internal/bitbucket/bitbucket.go +++ b/internal/bitbucket/bitbucket.go @@ -186,13 +186,8 @@ func (c *Config) getPullRequestDetailsFromBitbucket(prApiUrl, prHtmlLink, commit approvers := []any{} if len(participants) > 0 { - for i, p := range participants { - if boolean(p, "approved") { - username := str(obj(p, "user"), "display_name") - // An approval by nobody would count as approval evidence. - if username == "" { - return evidence, fmt.Errorf("participants[%d] in the Bitbucket reply is an approval with no user.display_name", i) - } + for _, p := range participants { + if username := approvedBy(p); username != "" { if version == 1 { approvers = append(approvers, username) } else { @@ -249,6 +244,16 @@ func (c *Config) getPullRequestDetailsFromBitbucket(prApiUrl, prHtmlLink, commit return evidence, nil } +// approvedBy names who approved in participant p, or gives "" when p is no +// approval. An approval with no identity, as from a deleted account, counts +// as none: approval evidence must name who gave it. +func approvedBy(p map[string]any) string { + if !boolean(p, "approved") { + return "" + } + return str(obj(p, "user"), "display_name") +} + // bitbucketReviews gives each participant's current decision. A participant // with no state who has commented is a comment; one who has done neither is // left out. @@ -271,11 +276,6 @@ func bitbucketReviews(participants []map[string]any) ([]types.PRApprovals, error default: continue } - // A change request is kept without a name: dropping it would read as - // none given. - if username == "" && state != types.ReviewChangesRequested { - continue - } timestamp, err := parseRFC3339NanoTimestamp(participatedOn, "participated_on") if err != nil { return nil, err diff --git a/internal/bitbucket/odd_replies_test.go b/internal/bitbucket/odd_replies_test.go index 4d812ebbb..a7690b3cd 100644 --- a/internal/bitbucket/odd_replies_test.go +++ b/internal/bitbucket/odd_replies_test.go @@ -260,12 +260,12 @@ var oddReadLists = map[string]string{ "pullrequest_commits_2": "values", } -// oddUnreadableApprovals are approvals with no identity. Counting one would -// record approval evidence attributed to nobody, so it must fail, naming the -// entry. Keyed by reply and path. -var oddUnreadableApprovals = map[string]string{ - "pullrequest participants[0].user": "participants[0]", - "pullrequest participants[0].user.display_name": "participants[0]", +// oddNamelessApprovals make the approval in participants[0] one with no +// identity, as for a deleted account. It must not count as an approval, but +// stays in the reviews. +var oddNamelessApprovals = map[string]bool{ + "pullrequest participants[0].user": true, + "pullrequest participants[0].user.display_name": true, } // oddMayError says whether path in reply holds, or is, a value in @@ -312,9 +312,13 @@ func TestOddReplies(t *testing.T) { require.True(t, strings.HasPrefix(outcome, "ok"), "an emptied list must not fail: %s", outcome) case regexp.MustCompile(`^` + oddReadLists[name] + `\[\d+\]$`).MatchString(v.Path): require.Equal(t, "error", outcome, "a null list item must fail") - case oddUnreadableApprovals[name+" "+v.Path] != "": - _, err := runOddReplies(t, version, replies) - require.ErrorContains(t, err, oddUnreadableApprovals[name+" "+v.Path]) + case oddNamelessApprovals[name+" "+v.Path]: + evidence, err := runOddReplies(t, version, replies) + require.NoError(t, err) + require.Empty(t, evidence[0].Approvers) + if version == 2 { + require.Contains(t, evidence[0].Reviews, types.PRApprovals{State: types.ReviewApproved, Timestamp: 1772370000}) + } case oddMayError(entry, name, v.Path): default: require.True(t, strings.HasPrefix(outcome, "ok"), "a null field must not fail: %s", outcome) diff --git a/internal/bitbucket/reviews_test.go b/internal/bitbucket/reviews_test.go index 013cb5033..95dda926b 100644 --- a/internal/bitbucket/reviews_test.go +++ b/internal/bitbucket/reviews_test.go @@ -20,7 +20,8 @@ func TestBitbucketReviews(t *testing.T) { {"user":{"display_name":"Di"},"role":"PARTICIPANT","approved":false,"state":null,"participated_on":"2026-03-01T15:00:00+00:00"}, {"user":{"display_name":"Ed"},"role":"REVIEWER","approved":false,"state":null,"participated_on":null}, {"user":null,"role":"REVIEWER","approved":false,"state":"changes_requested","participated_on":null}, - {"user":null,"role":"PARTICIPANT","approved":false,"state":null,"participated_on":"2026-03-01T16:00:00+00:00"} + {"user":null,"role":"PARTICIPANT","approved":false,"state":null,"participated_on":"2026-03-01T16:00:00+00:00"}, + {"user":{"display_name":null},"role":"REVIEWER","approved":true,"state":"approved","participated_on":"2026-03-01T17:00:00+00:00"} ]`), &participants)) reviews, err := bitbucketReviews(participants) @@ -31,6 +32,8 @@ func TestBitbucketReviews(t *testing.T) { {Username: "Cy", State: types.ReviewChangesRequested}, {Username: "Di", State: types.ReviewCommented, Timestamp: 1772377200}, {State: types.ReviewChangesRequested}, + {State: types.ReviewCommented, Timestamp: 1772380800}, + {State: types.ReviewApproved, Timestamp: 1772384400}, }, reviews) } From 747773f23109f3b52dd7a909f0e04ef0a968f248 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 22 Oct 2026 17:30:50 +0100 Subject: [PATCH 7/7] fix(bitbucket): cap commit paging so cycling next links end in an error The earlier guard only stopped a next link that was the page just fetched. Two pages pointing at each other still looped forever, and the earlier commit was wrong to say it matched GitLab: GitLab also caps the number of pages. Bitbucket's next is an opaque URL, so there is no page number to check for going up; a cap is the guard that ends every cycle. Commit paging now stops with an error after defaultMaxPages (1000) pages. The same-page check is dropped: the cap covers it. Part of #1283 Co-Authored-By: Claude Opus 5.5 --- internal/bitbucket/bitbucket.go | 15 ++++++---- internal/bitbucket/odd_replies_test.go | 41 ++++++++++++++------------ 2 files changed, 31 insertions(+), 25 deletions(-) diff --git a/internal/bitbucket/bitbucket.go b/internal/bitbucket/bitbucket.go index 628ed8378..cb4bf3def 100644 --- a/internal/bitbucket/bitbucket.go +++ b/internal/bitbucket/bitbucket.go @@ -13,6 +13,10 @@ import ( "github.com/kosli-dev/cli/internal/types" ) +// defaultMaxPages bounds commit paging so next links that cycle end in an +// error. It is set far above the pages a real pull request needs. +const defaultMaxPages = 1000 + type Config struct { Username string Password string @@ -306,7 +310,10 @@ func (c *Config) getPullRequestCommitsFromBitbucket(prID int) ([]types.Commit, e allCommits := []types.Commit{} currentURL := url - for currentURL != "" { + for pages := 0; currentURL != ""; pages++ { + if pages == defaultMaxPages { + return nil, fmt.Errorf("aborting after %d pages of commits for pull request %d", defaultMaxPages, prID) + } reqParams := &requests.RequestParams{ Method: http.MethodGet, URL: currentURL, @@ -347,11 +354,7 @@ func (c *Config) getPullRequestCommitsFromBitbucket(prID int) ([]types.Commit, e }) } - next := str(responseData, "next") - if next == currentURL { - return nil, fmt.Errorf("next page did not advance past page %s for pull request %d", currentURL, prID) - } - currentURL = next + currentURL = str(responseData, "next") if currentURL == "" { break } diff --git a/internal/bitbucket/odd_replies_test.go b/internal/bitbucket/odd_replies_test.go index a7690b3cd..33d3a7974 100644 --- a/internal/bitbucket/odd_replies_test.go +++ b/internal/bitbucket/odd_replies_test.go @@ -329,24 +329,27 @@ func TestOddReplies(t *testing.T) { } } -// A commits page whose next link is itself would be fetched forever. -func TestOddReplies_CommitPageThatDoesNotAdvanceFails(t *testing.T) { - replies := oddBaselineReplies() - replies["pullrequest_commits_2"] = `{ - "pagelen": 1, - "page": 2, - "values": [], - "next": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7/commits?page=2" -}` - done := make(chan error, 1) - go func() { - _, err := runOddReplies(t, 2, replies) - done <- err - }() - select { - case err := <-done: - require.ErrorContains(t, err, "did not advance") - case <-time.After(10 * time.Second): - t.Fatal("paging did not stop") +// Commit pages whose next links cycle would be fetched forever: a page +// pointing at itself, or two pages pointing at each other. +func TestOddReplies_CommitPagesThatCycleFail(t *testing.T) { + for name, next := range map[string]string{ + "page 2 to itself": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7/commits?page=2", + "page 2 to page 1": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7/commits", + } { + t.Run(name, func(t *testing.T) { + replies := oddBaselineReplies() + replies["pullrequest_commits_2"] = `{"pagelen": 1, "page": 2, "values": [], "next": "` + next + `"}` + done := make(chan error, 1) + go func() { + _, err := runOddReplies(t, 2, replies) + done <- err + }() + select { + case err := <-done: + require.ErrorContains(t, err, fmt.Sprintf("aborting after %d pages", defaultMaxPages)) + case <-time.After(20 * time.Second): + t.Fatal("paging did not stop") + } + }) } }