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..9f816af45 100644 --- a/internal/github/build_pr_evidence_test.go +++ b/internal/github/build_pr_evidence_test.go @@ -242,6 +242,50 @@ 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_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 47bfe4b39..e02e9810e 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,8 +333,8 @@ func buildPREvidence( HeadRef: headRef, HeadSHA: headSHA, BaseRef: baseRef, - Approvers: []any{}, Commits: []types.Commit{}, + Reviews: []types.PRApprovals{}, } for _, n := range commitNodes { @@ -388,16 +389,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 +440,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 +533,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..4e886747c 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,35 @@ 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[r.User.Username] = r.State + } + reviews := []types.PRApprovals{} + approved := map[string]bool{} + for _, a := range approvedBy { + 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 { + 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[r.User.Username]: + 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..2143ad4da --- /dev/null +++ b/internal/gitlab/reviews_test.go @@ -0,0 +1,164 @@ +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: "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"), + 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..7cb212bbf 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -15,18 +15,30 @@ 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 // 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) } @@ -37,6 +49,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..2b9a57eb8 100644 --- a/internal/types/types_test.go +++ b/internal/types/types_test.go @@ -40,3 +40,37 @@ func TestPREvidenceAlwaysSerialisesCommits(t *testing.T) { }) } } + +// 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) { + 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}})) +}