From a30aa89caa5d07830bbf22e45e009807aa625c9d Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 15 Oct 2026 15:12:28 +0100 Subject: [PATCH 1/2] feat: record every pull request review, not only approvals Policies could not see a change request given after an approval, because the CLI only sent approvals. Pull request attestations now carry a reviews list for every provider, with states mapped to APPROVED, CHANGES_REQUESTED, COMMENTED and, on GitHub, DISMISSED. - GitHub: every submitted review, with time and reviewed commit. - GitLab: approvals from approved_by, plus each reviewer's state. - Bitbucket: each participant's state and participated_on. - Azure: each reviewer's vote, leaving out groups. GitHub approvers now come from the same reviews, one per reviewer per commit, so approving the same commit twice no longer lists the reviewer twice. Approvals of different commits are both kept. Needs kosli-dev/server#7448 deployed first; the server drops unknown fields. Part of kosli-dev/server#7422. Co-Authored-By: Claude Opus 5.5 --- internal/azure/azure.go | 62 +++++++-- internal/azure/reviews_test.go | 33 +++++ internal/bitbucket/bitbucket.go | 50 +++++++ internal/bitbucket/reviews_test.go | 45 +++++++ internal/github/build_pr_evidence_test.go | 31 +++++ internal/github/github.go | 15 ++- internal/github/pagination.go | 4 +- internal/github/pr_pagination_test.go | 33 ++++- internal/gitlab/gitlab.go | 70 +++++++++- internal/gitlab/reviews_test.go | 154 ++++++++++++++++++++++ internal/types/types.go | 33 +++++ internal/types/types_test.go | 30 +++++ 12 files changed, 537 insertions(+), 23 deletions(-) create mode 100644 internal/azure/reviews_test.go create mode 100644 internal/bitbucket/reviews_test.go create mode 100644 internal/gitlab/reviews_test.go diff --git a/internal/azure/azure.go b/internal/azure/azure.go index 4bd04b0aa..d721883cc 100644 --- a/internal/azure/azure.go +++ b/internal/azure/azure.go @@ -140,7 +140,7 @@ func (c *AzureConfig) newPRAzureEvidenceV2(pr git.GitPullRequest) (*types.PREvid return evidence, err } evidence.Commits = commits - evidence.Approvers, err = c.GetPullRequestApprovers(*pr.PullRequestId, 2) + evidence.Approvers, evidence.Reviews, err = c.GetPullRequestReviews(*pr.PullRequestId) if err != nil { return evidence, err } @@ -233,25 +233,49 @@ func (c *AzureConfig) PullRequestsForCommit(commit string) ([]git.GitPullRequest // GetPullRequestApprovers returns a list of approvers for a given pull request func (c *AzureConfig) GetPullRequestApprovers(prNumber, version int) ([]any, error) { - var approvers []any + reviewers, err := c.pullRequestReviewers(prNumber) + if err != nil { + return nil, err + } + return azureApprovers(reviewers, version), nil +} + +// GetPullRequestReviews returns a PR's approvers and its reviews: each +// reviewer's current vote, since Azure keeps no history of them. +func (c *AzureConfig) GetPullRequestReviews(prNumber int) ([]any, []types.PRApprovals, error) { + reviewers, err := c.pullRequestReviewers(prNumber) + if err != nil { + return nil, nil, err + } + return azureApprovers(reviewers, 2), azureReviews(reviewers), nil +} + +func (c *AzureConfig) pullRequestReviewers(prNumber int) ([]git.IdentityRefWithVote, error) { ctx := context.Background() client, err := NewAzureClientFromToken(ctx, c.Token, c.OrgURL) if err != nil { - return approvers, err + return nil, err } - reviewers, err := client.GetPullRequestReviewers(ctx, git.GetPullRequestReviewersArgs{ RepositoryId: &c.Repository, PullRequestId: &prNumber, Project: &c.Project, }) if err != nil { - return approvers, err + return nil, err } + return *reviewers, nil +} - for _, r := range *reviewers { +func azureReviewerName(r git.IdentityRefWithVote) string { + return fmt.Sprintf("%s (%s)", *r.DisplayName, *r.UniqueName) +} + +func azureApprovers(reviewers []git.IdentityRefWithVote, version int) []any { + var approvers []any + for _, r := range reviewers { if *r.Vote == 10 { - approverName := fmt.Sprintf("%s (%s)", *r.DisplayName, *r.UniqueName) + approverName := azureReviewerName(r) if version == 1 { approvers = append(approvers, approverName) } else { @@ -262,5 +286,27 @@ func (c *AzureConfig) GetPullRequestApprovers(prNumber, version int) ([]any, err } } } - return approvers, nil + return approvers +} + +// azureReviews leaves out groups: a group's vote is its members' votes, and +// each member who voted is listed in their own right. +func azureReviews(reviewers []git.IdentityRefWithVote) []types.PRApprovals { + reviews := []types.PRApprovals{} + for _, r := range reviewers { + if (r.IsContainer != nil && *r.IsContainer) || r.Vote == nil { + continue + } + var state string + switch { + case *r.Vote > 0: + state = types.ReviewApproved + case *r.Vote < 0: + state = types.ReviewChangesRequested + default: + continue + } + reviews = append(reviews, types.PRApprovals{Username: azureReviewerName(r), State: state}) + } + return reviews } diff --git a/internal/azure/reviews_test.go b/internal/azure/reviews_test.go new file mode 100644 index 000000000..1aaa31956 --- /dev/null +++ b/internal/azure/reviews_test.go @@ -0,0 +1,33 @@ +package azure + +import ( + "testing" + + "github.com/kosli-dev/cli/internal/types" + "github.com/microsoft/azure-devops-go-api/azuredevops/git" + "github.com/stretchr/testify/require" +) + +func azureReviewer(name string, vote int, isGroup bool) git.IdentityRefWithVote { + unique := name + "@example.com" + return git.IdentityRefWithVote{DisplayName: &name, UniqueName: &unique, Vote: &vote, IsContainer: &isGroup} +} + +// Votes per IdentityRefWithVote: 10 approved, 5 approved with suggestions, +// 0 no vote, -5 waiting for author, -10 rejected. +func TestAzureReviews(t *testing.T) { + reviewers := []git.IdentityRefWithVote{ + azureReviewer("Ada", 10, false), + azureReviewer("Bob", 5, false), + azureReviewer("Cy", 0, false), + azureReviewer("Di", -5, false), + azureReviewer("Ed", -10, false), + azureReviewer("Team", 10, true), + } + require.Equal(t, []types.PRApprovals{ + {Username: "Ada (Ada@example.com)", State: types.ReviewApproved}, + {Username: "Bob (Bob@example.com)", State: types.ReviewApproved}, + {Username: "Di (Di@example.com)", State: types.ReviewChangesRequested}, + {Username: "Ed (Ed@example.com)", State: types.ReviewChangesRequested}, + }, azureReviews(reviewers)) +} diff --git a/internal/bitbucket/bitbucket.go b/internal/bitbucket/bitbucket.go index dead8d82e..1c04e6a63 100644 --- a/internal/bitbucket/bitbucket.go +++ b/internal/bitbucket/bitbucket.go @@ -190,6 +190,11 @@ func (c *Config) getPullRequestDetailsFromBitbucket(prApiUrl, prHtmlLink, commit return evidence, err } evidence.Commits = prCommits + + evidence.Reviews, err = bitbucketReviews(participants) + if err != nil { + return evidence, err + } } } else { return evidence, fmt.Errorf("failed to get PR details, got HTTP status %d. Please review repository permissions", response.Resp.StatusCode) @@ -197,6 +202,51 @@ func (c *Config) getPullRequestDetailsFromBitbucket(prApiUrl, prHtmlLink, commit return evidence, nil } +// 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) { + 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) + var state string + switch p["state"] { + case "approved": + state = types.ReviewApproved + case "changes_requested": + state = types.ReviewChangesRequested + case nil: + if participatedOn == "" { + continue + } + state = types.ReviewCommented + default: + continue + } + // A change request is kept without a name: dropping it would read as + // none given. + 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 + } + reviews = append(reviews, review) + } + return reviews, nil +} + // 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 { diff --git a/internal/bitbucket/reviews_test.go b/internal/bitbucket/reviews_test.go new file mode 100644 index 000000000..3a0c20fe0 --- /dev/null +++ b/internal/bitbucket/reviews_test.go @@ -0,0 +1,45 @@ +package bitbucket + +import ( + "encoding/json" + "testing" + + "github.com/kosli-dev/cli/internal/types" + "github.com/stretchr/testify/require" +) + +// Participant shapes follow the participant definition in +// 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 + 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"}, + {"user":{"display_name":"Cy"},"role":"REVIEWER","approved":false,"state":"changes_requested","participated_on":null}, + {"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"} + ]`), &participants)) + + reviews, err := bitbucketReviews(participants) + require.NoError(t, err) + require.Equal(t, []types.PRApprovals{ + {Username: "Ada", State: types.ReviewApproved, Timestamp: 1772370000}, + {Username: "Bob", State: types.ReviewChangesRequested, Timestamp: 1772373600}, + {Username: "Cy", State: types.ReviewChangesRequested}, + {Username: "Di", State: types.ReviewCommented, Timestamp: 1772377200}, + {State: types.ReviewChangesRequested}, + }, reviews) +} + +func TestBitbucketReviews_RejectsABadTimestamp(t *testing.T) { + var participants []any + require.NoError(t, json.Unmarshal([]byte(`[ + {"user":{"display_name":"Ada"},"approved":true,"state":"approved","participated_on":"yesterday"} + ]`), &participants)) + + _, err := bitbucketReviews(participants) + require.ErrorContains(t, err, "participated_on") +} diff --git a/internal/github/build_pr_evidence_test.go b/internal/github/build_pr_evidence_test.go index bea9d701d..ce7555836 100644 --- a/internal/github/build_pr_evidence_test.go +++ b/internal/github/build_pr_evidence_test.go @@ -242,6 +242,37 @@ func TestBuildPREvidence_RecordsWhichCommitAnApprovalCovered(t *testing.T) { }, evidence.Approvers, "a review with no commit is kept, without a commit_sha") } +// Sequence from kosli-dev/server PR 7200: approve, request changes, approve, +// all on one commit, plus a comment and a dismissal. +func TestBuildPREvidence_RecordsEveryReviewAndEachApprovalOnce(t *testing.T) { + const c1 = "1111111111111111111111111111111111111111" + evidence, err := buildPREvidence( + "https://github.com/kosli-dev/cli/pull/671", + "0e723254516c841126e81f76100be57258ff1386", + "MERGED", "tooky", "2026-03-01T09:00:00Z", "", + "title", "feature", "main", c1, + nil, []graphqlReviewNode{ + reviewNode("grace", "APPROVED", "2026-03-01T13:00:00Z", c1), + reviewNode("grace", "CHANGES_REQUESTED", "2026-03-01T14:00:00Z", c1), + reviewNode("ada", "COMMENTED", "2026-03-01T14:30:00Z", c1), + reviewNode("ada", "DISMISSED", "2026-03-01T14:45:00Z", c1), + reviewNode("grace", "APPROVED", "2026-03-01T15:00:00Z", c1), + }, + ) + require.NoError(t, err) + + require.Equal(t, []types.PRApprovals{ + {Username: "grace", State: "APPROVED", Timestamp: 1772370000, CommitSHA: c1}, + {Username: "grace", State: "CHANGES_REQUESTED", Timestamp: 1772373600, CommitSHA: c1}, + {Username: "ada", State: "COMMENTED", Timestamp: 1772375400, CommitSHA: c1}, + {Username: "ada", State: "DISMISSED", Timestamp: 1772376300, CommitSHA: c1}, + {Username: "grace", State: "APPROVED", Timestamp: 1772377200, CommitSHA: c1}, + }, evidence.Reviews) + require.Equal(t, []any{ + types.PRApprovals{Username: "grace", State: "APPROVED", Timestamp: 1772377200, CommitSHA: c1}, + }, evidence.Approvers) +} + func TestBuildPREvidence_RecordsWhoSignedACommit(t *testing.T) { node := graphqlCommitNode{} node.Commit.Oid = "0e723254516c841126e81f76100be57258ff1386" diff --git a/internal/github/github.go b/internal/github/github.go index 47bfe4b39..1e8037454 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -287,7 +287,8 @@ type graphqlSignature struct { WasSignedByGitHub graphql.Boolean } -// graphqlReviewNode is the shared GraphQL node type for approved reviews on a PR. +// graphqlReviewNode is the shared GraphQL node type for reviews on a PR. +// PENDING is never queried: it is unsubmitted, so it has no submittedAt. type graphqlReviewNode struct { Author struct { Login graphql.String @@ -332,7 +333,6 @@ func buildPREvidence( HeadRef: headRef, HeadSHA: headSHA, BaseRef: baseRef, - Approvers: []any{}, Commits: []types.Commit{}, } @@ -388,16 +388,17 @@ func buildPREvidence( if err != nil { return nil, err } - approval := types.PRApprovals{ + review := types.PRApprovals{ Username: string(r.Author.Login), State: string(r.State), Timestamp: submittedAt.Unix(), } if r.Commit != nil { - approval.CommitSHA = string(r.Commit.Oid) + review.CommitSHA = string(r.Commit.Oid) } - evidence.Approvers = append(evidence.Approvers, approval) + evidence.Reviews = append(evidence.Reviews, review) } + evidence.Approvers = types.ApprovalsFrom(evidence.Reviews) return evidence, nil } @@ -438,7 +439,7 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er Reviews struct { Nodes []graphqlReviewNode PageInfo pageInfo - } `graphql:"reviews(first: 100, states: APPROVED, after: $reviewCursor)"` + } `graphql:"reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED], after: $reviewCursor)"` } `graphql:"pullRequest(number: $prNumber)"` } `graphql:"repository(owner: $owner, name: $repo)"` } @@ -531,7 +532,7 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence Reviews struct { Nodes []graphqlReviewNode PageInfo pageInfo - } `graphql:"reviews(first: 100, states: APPROVED, after: $reviewCursor)"` + } `graphql:"reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED], after: $reviewCursor)"` } // Intentionally not paginated, so no cursor is selected: a // commit with more than 100 associated PRs is not a realistic diff --git a/internal/github/pagination.go b/internal/github/pagination.go index 3f5fe2e79..9a3725257 100644 --- a/internal/github/pagination.go +++ b/internal/github/pagination.go @@ -172,7 +172,7 @@ func (c *GithubConfig) allPRReviews(ctx context.Context, run graphqlQueryFunc, r Reviews struct { Nodes []graphqlReviewNode PageInfo pageInfo - } `graphql:"reviews(first: 100, states: APPROVED, after: $cursor)"` + } `graphql:"reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED], after: $cursor)"` } `graphql:"pullRequest(number: $prNumber)"` } `graphql:"repository(owner: $owner, name: $repo)"` } @@ -183,7 +183,7 @@ func (c *GithubConfig) allPRReviews(ctx context.Context, run graphqlQueryFunc, r return page.Nodes, page.PageInfo, nil }) if err != nil { - return nil, fmt.Errorf("draining approvals for %s: %w", ref, err) + return nil, fmt.Errorf("draining reviews for %s: %w", ref, err) } return reviews, nil } diff --git a/internal/github/pr_pagination_test.go b/internal/github/pr_pagination_test.go index 307e7a01d..550603dac 100644 --- a/internal/github/pr_pagination_test.go +++ b/internal/github/pr_pagination_test.go @@ -337,7 +337,7 @@ func TestPREvidenceForCommitV2_DrainErrorNamesThePullRequest(t *testing.T) { require.ErrorContains(t, errors.Unwrap(err), "did not advance", "the cause must stay unwrappable") } -func TestPREvidenceByPRNumber_ApprovalDrainErrorNamesThePullRequest(t *testing.T) { +func TestPREvidenceByPRNumber_ReviewDrainErrorNamesThePullRequest(t *testing.T) { ts := newGraphQLTestServer(t, byPRNumberResponse( connectionJSON([]string{commitNodeJSON("sha1")}, ""), @@ -347,7 +347,7 @@ func TestPREvidenceByPRNumber_ApprovalDrainErrorNamesThePullRequest(t *testing.T _, err := newPaginationConfig(ts).PREvidenceByPRNumber(7) require.ErrorContains(t, err, "test-org/test-repo#7") - require.ErrorContains(t, err, "approvals") + require.ErrorContains(t, err, "reviews") } // withHeadSHA adds the head commit to a PR node, which the shared helpers leave out. @@ -373,3 +373,32 @@ func TestPREvidenceForCommitV2_RecordsHeadSHA(t *testing.T) { require.Len(t, prs, 1) require.Equal(t, "head-sha", prs[0].HeadSHA) } + +// Every query that selects reviews must ask for the same states: one that +// still asked for approvals only would hide a later change request. +func TestReviewQueriesAskForEveryStateButPending(t *testing.T) { + byNumber := newGraphQLTestServer(t, + byPRNumberResponse( + connectionJSON([]string{commitNodeJSON("sha1")}, ""), + connectionJSON([]string{reviewNodeJSON("ada")}, "r1"), + ), + reviewsPageResponse([]string{reviewNodeJSON("grace")}, ""), + ) + _, err := newPaginationConfig(byNumber).PREvidenceByPRNumber(1) + require.NoError(t, err) + + forCommit := newGraphQLTestServer(t, + forCommitResponse(v2PRNodeJSON(7, + connectionJSON([]string{commitNodeJSON("sha1")}, ""), + connectionJSON([]string{reviewNodeJSON("ada")}, "r1"))), + reviewsPageResponse([]string{reviewNodeJSON("grace")}, ""), + ) + _, err = newPaginationConfig(forCommit).PREvidenceForCommitV2("merge-sha") + require.NoError(t, err) + + bodies := append(byNumber.bodies, forCommit.bodies...) + require.Len(t, bodies, 4) + for _, body := range bodies { + require.Contains(t, body, "states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED]") + } +} diff --git a/internal/gitlab/gitlab.go b/internal/gitlab/gitlab.go index 909b952b1..23ab8afca 100644 --- a/internal/gitlab/gitlab.go +++ b/internal/gitlab/gitlab.go @@ -127,11 +127,12 @@ func (c *GitlabConfig) newPRGitlabEvidenceV2(mr *gitlab.BasicMergeRequest) (*typ Title: mr.Title, } evidence.HeadRef, evidence.BaseRef = gitlabMRRefs(mr) - approvers, err := c.GetMergeRequestApprovers(mr.IID, 2) + approvers, reviews, err := c.GetMergeRequestReviews(mr.IID) if err != nil { return evidence, err } evidence.Approvers = approvers + evidence.Reviews = reviews commits, err := c.GetMergeRequestCommits(mr) if err != nil { return evidence, err @@ -166,9 +167,41 @@ func (c *GitlabConfig) GetMergeRequestApprovers(mrIID, version int64) ([]any, er if err != nil { return approvers, err } + return gitlabApprovers(approvals.ApprovedBy, version), nil +} + +// GetMergeRequestReviews returns an MR's approvers and its reviews: each +// reviewer's current decision, since GitLab keeps no history of them. +func (c *GitlabConfig) GetMergeRequestReviews(mrIID int64) ([]any, []types.PRApprovals, error) { + client, err := c.NewGitlabClientFromToken() + if err != nil { + return nil, nil, err + } + approvals, _, err := client.MergeRequestApprovals.GetConfiguration(c.ProjectID(), mrIID) + if err != nil { + return nil, nil, err + } + approvers := gitlabApprovers(approvals.ApprovedBy, 2) + reviewers, resp, err := client.MergeRequests.GetMergeRequestReviewers(c.ProjectID(), mrIID) + if err != nil { + // Older GitLab has no reviewers endpoint. Reviews without its change + // requests would read as none given, so they are not reported at all. + if resp != nil && resp.StatusCode == http.StatusNotFound { + return approvers, nil, nil + } + return nil, nil, err + } + return approvers, gitlabReviews(approvals.ApprovedBy, reviewers), nil +} - for _, approver := range approvals.ApprovedBy { - approverName := fmt.Sprintf("%s (@%s)", approver.User.Name, approver.User.Username) +func gitlabUserName(u *gitlab.BasicUser) string { + return fmt.Sprintf("%s (@%s)", u.Name, u.Username) +} + +func gitlabApprovers(approvedBy []*gitlab.MergeRequestApproverUser, version int64) []any { + var approvers []any + for _, approver := range approvedBy { + approverName := gitlabUserName(approver.User) if version == 1 { approvers = append(approvers, approverName) } else { @@ -177,7 +210,36 @@ func (c *GitlabConfig) GetMergeRequestApprovers(mrIID, version int64) ([]any, er }) } } - return approvers, nil + return approvers +} + +// gitlabReviews takes approvals from approved_by only, as people who are not +// reviewers can approve too. A change request outweighs an approval by the +// same person, as there are no review times to order them by. +func gitlabReviews(approvedBy []*gitlab.MergeRequestApproverUser, reviewers []*gitlab.MergeRequestReviewer) []types.PRApprovals { + reviewerStates := map[string]string{} + for _, r := range reviewers { + reviewerStates[gitlabUserName(r.User)] = r.State + } + reviews := []types.PRApprovals{} + approved := map[string]bool{} + for _, a := range approvedBy { + name := gitlabUserName(a.User) + approved[name] = true + if reviewerStates[name] != "requested_changes" { + reviews = append(reviews, types.PRApprovals{Username: name, State: types.ReviewApproved}) + } + } + for _, r := range reviewers { + name := gitlabUserName(r.User) + switch { + case r.State == "requested_changes": + reviews = append(reviews, types.PRApprovals{Username: name, State: types.ReviewChangesRequested}) + case r.State == "reviewed" && !approved[name]: + reviews = append(reviews, types.PRApprovals{Username: name, State: types.ReviewCommented}) + } + } + return reviews } // GetMergeRequestCommits returns a list of commits for a given MR diff --git a/internal/gitlab/reviews_test.go b/internal/gitlab/reviews_test.go new file mode 100644 index 000000000..adee2941c --- /dev/null +++ b/internal/gitlab/reviews_test.go @@ -0,0 +1,154 @@ +package gitlab + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/kosli-dev/cli/internal/types" + "github.com/stretchr/testify/require" + gitlab "gitlab.com/gitlab-org/api/client-go" +) + +func approvedBy(usernames ...string) []*gitlab.MergeRequestApproverUser { + users := []*gitlab.MergeRequestApproverUser{} + for _, u := range usernames { + users = append(users, &gitlab.MergeRequestApproverUser{User: &gitlab.BasicUser{Name: strings.ToUpper(u), Username: u}}) + } + return users +} + +func reviewer(username, state string) *gitlab.MergeRequestReviewer { + return &gitlab.MergeRequestReviewer{User: &gitlab.BasicUser{Name: strings.ToUpper(username), Username: username}, State: state} +} + +// Reviewer states are GitLab's MergeRequestReviewerState enum: unreviewed, +// reviewed, requested_changes, approved, unapproved, review_started. +func TestGitlabReviews(t *testing.T) { + for _, tc := range []struct { + name string + approvedBy []*gitlab.MergeRequestApproverUser + reviewers []*gitlab.MergeRequestReviewer + want []types.PRApprovals + }{ + { + name: "an approval comes from approved_by, reviewer or not", + approvedBy: approvedBy("ada", "bob"), + reviewers: []*gitlab.MergeRequestReviewer{reviewer("ada", "approved")}, + want: []types.PRApprovals{ + {Username: "ADA (@ada)", State: types.ReviewApproved}, + {Username: "BOB (@bob)", State: types.ReviewApproved}, + }, + }, + { + name: "a reviewer state of approved is not an approval unless approved_by says so", + reviewers: []*gitlab.MergeRequestReviewer{reviewer("ada", "approved")}, + want: []types.PRApprovals{}, + }, + { + name: "requested_changes and reviewed map to the shared states", + reviewers: []*gitlab.MergeRequestReviewer{ + reviewer("ada", "requested_changes"), reviewer("bob", "reviewed"), + }, + want: []types.PRApprovals{ + {Username: "ADA (@ada)", State: types.ReviewChangesRequested}, + {Username: "BOB (@bob)", State: types.ReviewCommented}, + }, + }, + { + name: "a change request outweighs an approval by the same person", + approvedBy: approvedBy("ada"), + reviewers: []*gitlab.MergeRequestReviewer{reviewer("ada", "requested_changes")}, + want: []types.PRApprovals{{Username: "ADA (@ada)", State: types.ReviewChangesRequested}}, + }, + { + name: "a comment does not add a second entry beside an approval", + approvedBy: approvedBy("ada"), + reviewers: []*gitlab.MergeRequestReviewer{reviewer("ada", "reviewed")}, + want: []types.PRApprovals{{Username: "ADA (@ada)", State: types.ReviewApproved}}, + }, + { + name: "states with no decision are left out", + reviewers: []*gitlab.MergeRequestReviewer{ + reviewer("ada", "unreviewed"), reviewer("bob", "review_started"), reviewer("cy", "unapproved"), + }, + want: []types.PRApprovals{}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.want, gitlabReviews(tc.approvedBy, tc.reviewers)) + }) + } +} + +// approvalsServer serves an MR's approvals and reviewers in GitLab's shapes. +func approvalsServer(t *testing.T, approvals, reviewers string) *httptest.Server { + t.Helper() + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case strings.HasSuffix(r.URL.Path, "/merge_requests/3/approvals"): + _, _ = fmt.Fprint(w, approvals) + case strings.HasSuffix(r.URL.Path, "/merge_requests/3/reviewers"): + _, _ = fmt.Fprint(w, reviewers) + case strings.HasSuffix(r.URL.Path, "/merge_requests/3/commits"): + _, _ = fmt.Fprint(w, "[]") + default: + t.Errorf("unexpected request %s", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + t.Cleanup(ts.Close) + return ts +} + +func TestGetMergeRequestReviews_ReadsApprovalsAndReviewers(t *testing.T) { + ts := approvalsServer(t, + `{"approved_by":[{"user":{"name":"Ada","username":"ada"}}]}`, + `[{"user":{"name":"Bob","username":"bob"},"state":"requested_changes","created_at":"2026-03-01T12:00:00Z"}]`, + ) + + approvers, reviews, err := newPaginationConfig(ts.URL).GetMergeRequestReviews(3) + require.NoError(t, err) + require.Equal(t, []any{types.PRApprovals{Username: "Ada (@ada)"}}, approvers) + require.Equal(t, []types.PRApprovals{ + {Username: "Ada (@ada)", State: types.ReviewApproved}, + {Username: "Bob (@bob)", State: types.ReviewChangesRequested}, + }, reviews) +} + +func TestNewPRGitlabEvidenceV2_RecordsReviews(t *testing.T) { + ts := approvalsServer(t, + `{"approved_by":[]}`, + `[{"user":{"name":"Bob","username":"bob"},"state":"requested_changes","created_at":"2026-03-01T12:00:00Z"}]`, + ) + now := time.Now() + mr := &gitlab.BasicMergeRequest{IID: 3, Author: &gitlab.BasicUser{}, CreatedAt: &now, MergedAt: &now} + + evidence, err := newPaginationConfig(ts.URL).newPRGitlabEvidenceV2(mr) + require.NoError(t, err) + require.Equal(t, []types.PRApprovals{{Username: "Bob (@bob)", State: types.ReviewChangesRequested}}, evidence.Reviews) +} + +// GitLab versions without the reviewers endpoint answer 404. Reviews are then +// not reported at all, rather than reported without the change requests. +func TestGetMergeRequestReviews_OmitsReviewsWhenReviewersEndpointIsMissing(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if strings.HasSuffix(r.URL.Path, "/merge_requests/3/approvals") { + _, _ = fmt.Fprint(w, `{"approved_by":[{"user":{"name":"Ada","username":"ada"}}]}`) + return + } + w.WriteHeader(http.StatusNotFound) + _, _ = fmt.Fprint(w, `{"message":"404 Not Found"}`) + })) + t.Cleanup(ts.Close) + + approvers, reviews, err := newPaginationConfig(ts.URL).GetMergeRequestReviews(3) + require.NoError(t, err) + require.Equal(t, []any{types.PRApprovals{Username: "Ada (@ada)"}}, approvers) + require.Nil(t, reviews) +} diff --git a/internal/types/types.go b/internal/types/types.go index b7f2e6807..aea44bbae 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -15,6 +15,9 @@ type PREvidence struct { HeadSHA string `json:"head_sha,omitempty"` BaseRef string `json:"base_ref,omitempty"` Commits []Commit `json:"commits"` + // Reviews holds every review, not only approvals, so a policy can see a + // change request given after an approval. + Reviews []PRApprovals `json:"reviews,omitempty"` } // MarshalJSON keeps "commits" in the payload even when a provider returns no @@ -37,6 +40,36 @@ type PRApprovals struct { CommitSHA string `json:"commit_sha,omitempty"` } +// Review states, whatever the git provider calls them. +const ( + ReviewApproved = "APPROVED" + ReviewChangesRequested = "CHANGES_REQUESTED" + ReviewCommented = "COMMENTED" + // ReviewDismissed is a review GitHub withdrew; it no longer says what it decided. + ReviewDismissed = "DISMISSED" +) + +// ApprovalsFrom lists the approvals among reviews, one per reviewer per commit +// with the latest kept: approving the same commit twice adds nothing, while +// approvals of two commits are two facts a policy may need. +func ApprovalsFrom(reviews []PRApprovals) []any { + approvals := []any{} + index := map[[2]string]int{} + for _, r := range reviews { + if r.State != ReviewApproved { + continue + } + key := [2]string{r.Username, r.CommitSHA} + if i, seen := index[key]; seen { + approvals[i] = r + continue + } + index[key] = len(approvals) + approvals = append(approvals, r) + } + return approvals +} + type Commit struct { SHA string `json:"sha1"` Message string `json:"message"` diff --git a/internal/types/types_test.go b/internal/types/types_test.go index 9767f40e8..b66cdd51b 100644 --- a/internal/types/types_test.go +++ b/internal/types/types_test.go @@ -40,3 +40,33 @@ func TestPREvidenceAlwaysSerialisesCommits(t *testing.T) { }) } } + +func TestPREvidenceOmitsReviewsWhenNoneAreRecorded(t *testing.T) { + for _, reviews := range [][]PRApprovals{nil, {}} { + payload, err := json.Marshal(PREvidence{Reviews: reviews}) + require.NoError(t, err) + require.NotContains(t, string(payload), `"reviews"`) + } +} + +func TestApprovalsFromKeepsTheLatestApprovalOfEachCommitPerReviewer(t *testing.T) { + reviews := []PRApprovals{ + {Username: "ada", State: ReviewApproved, Timestamp: 1, CommitSHA: "c1"}, + {Username: "ada", State: ReviewChangesRequested, Timestamp: 2, CommitSHA: "c1"}, + {Username: "ada", State: ReviewApproved, Timestamp: 3, CommitSHA: "c1"}, + {Username: "bob", State: ReviewCommented, Timestamp: 4, CommitSHA: "c1"}, + {Username: "bob", State: ReviewDismissed, Timestamp: 5, CommitSHA: "c1"}, + {Username: "bob", State: ReviewApproved, Timestamp: 6, CommitSHA: "c2"}, + {Username: "ada", State: ReviewApproved, Timestamp: 7, CommitSHA: "c2"}, + } + require.Equal(t, []any{ + PRApprovals{Username: "ada", State: ReviewApproved, Timestamp: 3, CommitSHA: "c1"}, + PRApprovals{Username: "bob", State: ReviewApproved, Timestamp: 6, CommitSHA: "c2"}, + PRApprovals{Username: "ada", State: ReviewApproved, Timestamp: 7, CommitSHA: "c2"}, + }, ApprovalsFrom(reviews)) +} + +func TestApprovalsFromReturnsAnEmptyListWithNoApprovals(t *testing.T) { + require.Equal(t, []any{}, ApprovalsFrom(nil)) + require.Equal(t, []any{}, ApprovalsFrom([]PRApprovals{{Username: "ada", State: ReviewCommented}})) +} From 3d949643a7786bb31a79e1b362b1f4e3ef2a22c1 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Thu, 15 Oct 2026 15:38:52 +0100 Subject: [PATCH 2/2] fix: tell "no reviews" apart from "reviews not collected" omitempty dropped an empty reviews list as well as a nil one, so a PR with no reviews looked the same as one whose reviews were never read. An empty list is now sent as [], and only a nil one is left out. GitLab now matches approvers to reviewers on username rather than on the display name, which the two endpoints could format differently. Co-Authored-By: Claude Opus 5.5 --- internal/github/build_pr_evidence_test.go | 13 +++++++++++++ internal/github/github.go | 1 + internal/gitlab/gitlab.go | 11 +++++------ internal/gitlab/reviews_test.go | 10 ++++++++++ internal/types/types.go | 11 ++++++++++- internal/types/types_test.go | 16 ++++++++++------ 6 files changed, 49 insertions(+), 13 deletions(-) diff --git a/internal/github/build_pr_evidence_test.go b/internal/github/build_pr_evidence_test.go index ce7555836..9f816af45 100644 --- a/internal/github/build_pr_evidence_test.go +++ b/internal/github/build_pr_evidence_test.go @@ -273,6 +273,19 @@ func TestBuildPREvidence_RecordsEveryReviewAndEachApprovalOnce(t *testing.T) { }, evidence.Approvers) } +func TestBuildPREvidence_RecordsThatAPRHasNoReviews(t *testing.T) { + evidence, err := buildPREvidence( + "https://github.com/kosli-dev/cli/pull/671", + "0e723254516c841126e81f76100be57258ff1386", + "MERGED", "tooky", "2026-03-01T09:00:00Z", "", + "title", "feature", "main", "", + nil, nil, + ) + require.NoError(t, err) + require.NotNil(t, evidence.Reviews, "no reviews is a fact, not a gap") + require.Empty(t, evidence.Reviews) +} + func TestBuildPREvidence_RecordsWhoSignedACommit(t *testing.T) { node := graphqlCommitNode{} node.Commit.Oid = "0e723254516c841126e81f76100be57258ff1386" diff --git a/internal/github/github.go b/internal/github/github.go index 1e8037454..e02e9810e 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -334,6 +334,7 @@ func buildPREvidence( HeadSHA: headSHA, BaseRef: baseRef, Commits: []types.Commit{}, + Reviews: []types.PRApprovals{}, } for _, n := range commitNodes { diff --git a/internal/gitlab/gitlab.go b/internal/gitlab/gitlab.go index 23ab8afca..4e886747c 100644 --- a/internal/gitlab/gitlab.go +++ b/internal/gitlab/gitlab.go @@ -219,15 +219,14 @@ func gitlabApprovers(approvedBy []*gitlab.MergeRequestApproverUser, version int6 func gitlabReviews(approvedBy []*gitlab.MergeRequestApproverUser, reviewers []*gitlab.MergeRequestReviewer) []types.PRApprovals { reviewerStates := map[string]string{} for _, r := range reviewers { - reviewerStates[gitlabUserName(r.User)] = r.State + reviewerStates[r.User.Username] = r.State } reviews := []types.PRApprovals{} approved := map[string]bool{} for _, a := range approvedBy { - name := gitlabUserName(a.User) - approved[name] = true - if reviewerStates[name] != "requested_changes" { - reviews = append(reviews, types.PRApprovals{Username: name, State: types.ReviewApproved}) + approved[a.User.Username] = true + if reviewerStates[a.User.Username] != "requested_changes" { + reviews = append(reviews, types.PRApprovals{Username: gitlabUserName(a.User), State: types.ReviewApproved}) } } for _, r := range reviewers { @@ -235,7 +234,7 @@ func gitlabReviews(approvedBy []*gitlab.MergeRequestApproverUser, reviewers []*g switch { case r.State == "requested_changes": reviews = append(reviews, types.PRApprovals{Username: name, State: types.ReviewChangesRequested}) - case r.State == "reviewed" && !approved[name]: + case r.State == "reviewed" && !approved[r.User.Username]: reviews = append(reviews, types.PRApprovals{Username: name, State: types.ReviewCommented}) } } diff --git a/internal/gitlab/reviews_test.go b/internal/gitlab/reviews_test.go index adee2941c..2143ad4da 100644 --- a/internal/gitlab/reviews_test.go +++ b/internal/gitlab/reviews_test.go @@ -64,6 +64,16 @@ func TestGitlabReviews(t *testing.T) { reviewers: []*gitlab.MergeRequestReviewer{reviewer("ada", "requested_changes")}, want: []types.PRApprovals{{Username: "ADA (@ada)", State: types.ReviewChangesRequested}}, }, + { + name: "people are matched on username, not on display name", + approvedBy: []*gitlab.MergeRequestApproverUser{ + {User: &gitlab.BasicUser{Name: "Ada L", Username: "ada"}}, + }, + reviewers: []*gitlab.MergeRequestReviewer{ + {User: &gitlab.BasicUser{Name: "Ada Lovelace", Username: "ada"}, State: "requested_changes"}, + }, + want: []types.PRApprovals{{Username: "Ada Lovelace (@ada)", State: types.ReviewChangesRequested}}, + }, { name: "a comment does not add a second entry beside an approval", approvedBy: approvedBy("ada"), diff --git a/internal/types/types.go b/internal/types/types.go index aea44bbae..7cb212bbf 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -24,12 +24,21 @@ type PREvidence struct { // commits. The API requires the field on FoundPullRequestV2, so omitting it — // or sending null for a nil slice — produces a payload matching neither // attestation schema (#1081). +// +// It also sends an empty reviews list, which omitempty would drop: a missing +// field means reviews were not collected, an empty one that there were none. func (e PREvidence) MarshalJSON() ([]byte, error) { type prEvidence PREvidence // sheds the method set, so this does not recurse - out := prEvidence(e) + out := struct { + prEvidence + Reviews *[]PRApprovals `json:"reviews,omitempty"` // shadows the embedded field + }{prEvidence: prEvidence(e)} if out.Commits == nil { out.Commits = []Commit{} } + if e.Reviews != nil { + out.Reviews = &e.Reviews + } return json.Marshal(out) } diff --git a/internal/types/types_test.go b/internal/types/types_test.go index b66cdd51b..2b9a57eb8 100644 --- a/internal/types/types_test.go +++ b/internal/types/types_test.go @@ -41,12 +41,16 @@ func TestPREvidenceAlwaysSerialisesCommits(t *testing.T) { } } -func TestPREvidenceOmitsReviewsWhenNoneAreRecorded(t *testing.T) { - for _, reviews := range [][]PRApprovals{nil, {}} { - payload, err := json.Marshal(PREvidence{Reviews: reviews}) - require.NoError(t, err) - require.NotContains(t, string(payload), `"reviews"`) - } +// A missing reviews field means reviews were not collected; an empty list +// means they were, and there were none. +func TestPREvidenceSendsReviewsOnlyWhenCollected(t *testing.T) { + payload, err := json.Marshal(PREvidence{}) + require.NoError(t, err) + require.NotContains(t, string(payload), `"reviews"`) + + payload, err = json.Marshal(PREvidence{Reviews: []PRApprovals{}}) + require.NoError(t, err) + require.Contains(t, string(payload), `"reviews":[]`) } func TestApprovalsFromKeepsTheLatestApprovalOfEachCommitPerReviewer(t *testing.T) {