From 2fbba9db3ae0b68fe822d482698b5e55ade31fbf Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Fri, 2 Oct 2026 16:30:29 +0100 Subject: [PATCH 1/7] feat(github): record the PR head commit and the commit each approval was given on A policy can tell whether an approval still covers a pull request only by knowing which commit it was given on. Commit dates can't answer that: they come from the commit itself and can be set to anything. GitHub records the reviewed commit on each review and the PR's final commit as headRefOid, so the pull request attestation now carries them as approvers[].commit_sha and head_sha. Both fields are omitted when unknown: head_sha for providers other than GitHub, and commit_sha when GitHub no longer has the reviewed commit. A policy can then treat a missing value as "not shown", rather than compare two empty strings. Co-Authored-By: Claude Opus 5.5 --- internal/github/build_pr_evidence_test.go | 36 ++++++++++--- internal/github/github.go | 21 ++++++-- internal/github/pr_pagination_test.go | 62 +++++++++++++++++++++-- internal/types/types.go | 2 + 4 files changed, 104 insertions(+), 17 deletions(-) diff --git a/internal/github/build_pr_evidence_test.go b/internal/github/build_pr_evidence_test.go index c18935bae..2b1bc08cf 100644 --- a/internal/github/build_pr_evidence_test.go +++ b/internal/github/build_pr_evidence_test.go @@ -40,7 +40,7 @@ func TestBuildPREvidence_RecordsAuthorNotCommitter(t *testing.T) { "", "Introduce kosli evaluate", "introduce-kosli-evaluate", - "main", + "main", "", []graphqlCommitNode{node}, nil, ) @@ -71,7 +71,7 @@ func TestBuildPREvidence_UsesAuthoredDate(t *testing.T) { "https://github.com/kosli-dev/cli/pull/671", "0e723254516c841126e81f76100be57258ff1386", "MERGED", "tooky", "2026-03-01T09:00:00Z", "", - "Introduce kosli evaluate", "introduce-kosli-evaluate", "main", + "Introduce kosli evaluate", "introduce-kosli-evaluate", "main", "", []graphqlCommitNode{node}, nil, ) require.NoError(t, err) @@ -97,7 +97,7 @@ func TestBuildPREvidence_FallsBackToCommittedDate(t *testing.T) { "https://github.com/kosli-dev/cli/pull/671", "0e723254516c841126e81f76100be57258ff1386", "MERGED", "tooky", "2026-03-01T09:00:00Z", "", - "title", "branch", "main", + "title", "branch", "main", "", []graphqlCommitNode{node}, nil, ) require.NoError(t, err) @@ -115,7 +115,7 @@ func TestBuildPREvidence_RecordsBaseRef(t *testing.T) { "https://github.com/kosli-dev/cli/pull/671", "0e723254516c841126e81f76100be57258ff1386", "MERGED", "tooky", "2026-03-01T09:00:00Z", "", - "title", "feature-branch", "main", + "title", "feature-branch", "main", "", nil, nil, ) require.NoError(t, err) @@ -141,7 +141,7 @@ func TestBuildPREvidence_RecordsCommitSignature(t *testing.T) { "https://github.com/kosli-dev/cli/pull/671", "0e723254516c841126e81f76100be57258ff1386", "MERGED", "tooky", "2026-03-01T09:00:00Z", "", - "title", "feature", "main", + "title", "feature", "main", "", []graphqlCommitNode{node}, nil, ) require.NoError(t, err) @@ -167,7 +167,7 @@ func TestBuildPREvidence_EmptyAuthorIsPreserved(t *testing.T) { "2026-03-01T12:00:00Z", "Fix something", "fix-branch", - "main", + "main", "", nil, nil, ) require.NoError(t, err) @@ -196,7 +196,7 @@ func TestBuildPREvidence_UnsignedCommitHasNoSignatureFields(t *testing.T) { "https://github.com/kosli-dev/cli/pull/671", "0e723254516c841126e81f76100be57258ff1386", "MERGED", "tooky", "2026-03-01T09:00:00Z", "", - "title", "feature", "main", + "title", "feature", "main", "", []graphqlCommitNode{node}, nil, ) require.NoError(t, err) @@ -204,3 +204,25 @@ func TestBuildPREvidence_UnsignedCommitHasNoSignatureFields(t *testing.T) { require.Nil(t, evidence.Commits[0].Verified, "unsigned commit must leave verified nil") require.Nil(t, evidence.Commits[0].SignatureState) } + +// An approval whose commit GitHub no longer has must leave commit_sha out of +// the payload rather than send an empty string, and so must an unknown head. +func TestBuildPREvidence_OmitsUnknownReviewedAndHeadCommits(t *testing.T) { + review := graphqlReviewNode{State: "APPROVED", SubmittedAt: "2026-03-01T13:00:00Z"} + review.Author.Login = "grace" + + evidence, err := buildPREvidence( + "https://github.com/kosli-dev/cli/pull/671", + "0e723254516c841126e81f76100be57258ff1386", + "MERGED", "tooky", "2026-03-01T09:00:00Z", "", + "title", "feature-branch", "main", "", + nil, []graphqlReviewNode{review}, + ) + require.NoError(t, err) + + payload, err := json.Marshal(evidence) + require.NoError(t, err) + require.Contains(t, string(payload), `"username":"grace"`) + require.NotContains(t, string(payload), "commit_sha") + require.NotContains(t, string(payload), "head_sha") +} diff --git a/internal/github/github.go b/internal/github/github.go index e77e5b4d9..40733409b 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -288,13 +288,17 @@ type graphqlReviewNode struct { } State graphql.String SubmittedAt graphql.String + // Null when GitHub no longer has the commit the review was given on. + Commit *struct { + Oid graphql.String + } } // buildPREvidence constructs a PREvidence from pre-resolved fields and the // raw GraphQL commit/review nodes. mergeCommit must be resolved by the caller // (it differs between commit-SHA queries and PR-number queries). func buildPREvidence( - url, mergeCommit, state, author, createdAtStr, mergedAtStr, title, headRef, baseRef string, + url, mergeCommit, state, author, createdAtStr, mergedAtStr, title, headRef, baseRef, headSHA string, commitNodes []graphqlCommitNode, reviewNodes []graphqlReviewNode, ) (*types.PREvidence, error) { @@ -320,6 +324,7 @@ func buildPREvidence( MergedAt: mergedAt, Title: title, HeadRef: headRef, + HeadSHA: headSHA, BaseRef: baseRef, Approvers: []any{}, Commits: []types.Commit{}, @@ -369,11 +374,15 @@ func buildPREvidence( if err != nil { return nil, err } - evidence.Approvers = append(evidence.Approvers, types.PRApprovals{ + approval := types.PRApprovals{ Username: string(r.Author.Login), State: string(r.State), Timestamp: submittedAt.Unix(), - }) + } + if r.Commit != nil { + approval.CommitSHA = string(r.Commit.Oid) + } + evidence.Approvers = append(evidence.Approvers, approval) } return evidence, nil @@ -397,6 +406,7 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er Title graphql.String State graphql.String HeadRefName graphql.String + HeadRefOid graphql.String BaseRefName graphql.String URL graphql.String CreatedAt graphql.String @@ -454,7 +464,7 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er return buildPREvidence( string(pr.URL), mergeCommit, string(pr.State), string(pr.Author.Login), - string(pr.CreatedAt), string(pr.MergedAt), string(pr.Title), string(pr.HeadRefName), string(pr.BaseRefName), + string(pr.CreatedAt), string(pr.MergedAt), string(pr.Title), string(pr.HeadRefName), string(pr.BaseRefName), string(pr.HeadRefOid), commits, reviews, ) } @@ -489,6 +499,7 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence Title graphql.String State graphql.String HeadRefName graphql.String + HeadRefOid graphql.String BaseRefName graphql.String URL graphql.String CreatedAt graphql.String @@ -559,7 +570,7 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence // so the commit is by definition the merge commit. evidence, err := buildPREvidence( string(pr.URL), commit, string(pr.State), string(pr.Author.Login), - string(pr.CreatedAt), string(pr.MergedAt), string(pr.Title), string(pr.HeadRefName), string(pr.BaseRefName), + string(pr.CreatedAt), string(pr.MergedAt), string(pr.Title), string(pr.HeadRefName), string(pr.BaseRefName), string(pr.HeadRefOid), commits, reviews, ) if err != nil { diff --git a/internal/github/pr_pagination_test.go b/internal/github/pr_pagination_test.go index fd7fbc20f..b50e6d275 100644 --- a/internal/github/pr_pagination_test.go +++ b/internal/github/pr_pagination_test.go @@ -70,9 +70,17 @@ func commitNodeJSON(sha string) string { `"signature":null}}`, sha, sha, sha) } -// reviewNodeJSON is one node of a GraphQL reviews connection. +// reviewNodeJSON is one node of a GraphQL reviews connection, given on the +// commit "reviewed-by-". func reviewNodeJSON(login string) string { - return fmt.Sprintf(`{"author":{"login":%q},"state":"APPROVED","submittedAt":"2026-03-01T13:00:00Z"}`, login) + return fmt.Sprintf(`{"author":{"login":%q},"state":"APPROVED","submittedAt":"2026-03-01T13:00:00Z",`+ + `"commit":{"oid":"reviewed-by-%s"}}`, login, login) +} + +// reviewNodeWithoutCommitJSON is a review whose commit GitHub no longer has. +func reviewNodeWithoutCommitJSON(login string) string { + return fmt.Sprintf(`{"author":{"login":%q},"state":"APPROVED","submittedAt":"2026-03-01T13:00:00Z",`+ + `"commit":null}`, login) } // connectionJSON wraps nodes with a pageInfo block. An empty cursor means the @@ -85,7 +93,7 @@ func connectionJSON(nodes []string, nextCursor string) string { // prJSON is a full pullRequest object with the given commit and review connections. func prJSON(commits, reviews string) string { - return fmt.Sprintf(`{"title":"A PR","state":"MERGED","headRefName":"feature","baseRefName":"main",`+ + return fmt.Sprintf(`{"title":"A PR","state":"MERGED","headRefName":"feature","headRefOid":"head-sha","baseRefName":"main",`+ `"url":"https://github.com/o/r/pull/1","createdAt":"2026-03-01T11:00:00Z",`+ `"mergedAt":"2026-03-01T14:00:00Z","mergeCommit":{"oid":"merge-sha"},`+ `"author":{"login":"ada"},"commits":%s,"reviews":%s}`, commits, reviews) @@ -198,11 +206,11 @@ func v2PRNodeJSON(number int, commits, reviews string) string { // which need not be the configured repository. func v2PRNodeInRepoJSON(owner, repo string, number int, commits, reviews string) string { return fmt.Sprintf(`{"number":%d,"repository":{"name":%q,"owner":{"login":%q}},`+ - `"title":"A PR","state":"MERGED","headRefName":"feature",`+ + `"title":"A PR","state":"MERGED","headRefName":"feature","headRefOid":"head-sha-%d",`+ `"baseRefName":"main","url":"https://github.com/%s/%s/pull/%d",`+ `"createdAt":"2026-03-01T11:00:00Z","mergedAt":"2026-03-01T14:00:00Z",`+ `"author":{"login":"ada"},"commits":%s,"reviews":%s}`, - number, repo, owner, owner, repo, number, commits, reviews) + number, repo, owner, number, owner, repo, number, commits, reviews) } // forCommitResponse carries no pageInfo for the PR connection: the query does @@ -349,3 +357,47 @@ func TestPREvidenceByPRNumber_ApprovalDrainErrorNamesThePullRequest(t *testing.T require.ErrorContains(t, err, "test-org/test-repo#7") require.ErrorContains(t, err, "approvals") } + +func approverCommitSHAs(evidence *types.PREvidence) []string { + shas := []string{} + for _, a := range evidence.Approvers { + shas = append(shas, a.(types.PRApprovals).CommitSHA) + } + return shas +} + +func TestPREvidenceByPRNumber_RecordsHeadAndReviewedCommits(t *testing.T) { + ts := newGraphQLTestServer(t, + byPRNumberResponse( + connectionJSON([]string{commitNodeJSON("sha1")}, ""), + connectionJSON([]string{reviewNodeJSON("ada")}, "r1"), + ), + reviewsPageResponse([]string{reviewNodeJSON("grace"), reviewNodeWithoutCommitJSON("linus")}, ""), + ) + + evidence, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) + require.NoError(t, err) + require.Equal(t, "head-sha", evidence.HeadSHA) + require.Equal(t, []string{"reviewed-by-ada", "reviewed-by-grace", ""}, approverCommitSHAs(evidence), + "each approval keeps its own commit, on later review pages too; a missing commit stays empty") + // The fake replies whatever is asked, so the field names GitHub must see are checked in the query. + require.Contains(t, ts.bodies[0], "headRefOid") + require.Contains(t, ts.bodies[0], "commit{oid}") + require.Contains(t, ts.bodies[1], "commit{oid}") +} + +func TestPREvidenceForCommitV2_RecordsHeadAndReviewedCommits(t *testing.T) { + ts := newGraphQLTestServer(t, forCommitResponse( + v2PRNodeJSON(7, + connectionJSON([]string{commitNodeJSON("sha1")}, ""), + connectionJSON([]string{reviewNodeJSON("ada")}, "")), + )) + + prs, err := newPaginationConfig(ts).PREvidenceForCommitV2("merge-sha") + require.NoError(t, err) + require.Len(t, prs, 1) + require.Equal(t, "head-sha-7", prs[0].HeadSHA) + require.Equal(t, []string{"reviewed-by-ada"}, approverCommitSHAs(prs[0])) + require.Contains(t, ts.bodies[0], "headRefOid") + require.Contains(t, ts.bodies[0], "commit{oid}") +} diff --git a/internal/types/types.go b/internal/types/types.go index 25db609fb..f4655f7f4 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -12,6 +12,7 @@ type PREvidence struct { MergedAt int64 `json:"merged_at,omitempty"` Title string `json:"title,omitempty"` HeadRef string `json:"head_ref,omitempty"` + HeadSHA string `json:"head_sha,omitempty"` BaseRef string `json:"base_ref,omitempty"` Commits []Commit `json:"commits"` } @@ -33,6 +34,7 @@ type PRApprovals struct { Username string `json:"username"` State string `json:"state,omitempty"` Timestamp int64 `json:"timestamp,omitempty"` + CommitSHA string `json:"commit_sha,omitempty"` } type Commit struct { From 8d295aa3ce39bf06bc033e21b985e077868e0d9b Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Fri, 2 Oct 2026 16:52:42 +0100 Subject: [PATCH 2/7] feat(github): record only current approvals from people with write access, and who signed each commit The pull request attestation listed every review GitHub had marked APPROVED. That included approvals from accounts without write access, which anyone can leave on a public repository, approvals by bots, and approvals the same reviewer later replaced by requesting changes. Approvers are now taken from each reviewer's latest approving or change-requesting review, from people with write access only (latestOpinionatedReviews with writersOnly), keeping approvals by user accounts. GitHub pull request attestations therefore list fewer approvers than before when any of those were present. Each commit now also records signer_username and signed_by_github. A commit's author name and email are whatever its writer put there; a verified signature names the account that holds the signing key, so a policy can tell who actually made the commit. Co-Authored-By: Claude Opus 5.5 --- internal/github/build_pr_evidence_test.go | 80 +++++++++++++++++++++-- internal/github/github.go | 46 +++++++++---- internal/github/pagination.go | 2 +- internal/github/pr_pagination_test.go | 40 ++++++++++-- internal/types/types.go | 2 + 5 files changed, 145 insertions(+), 25 deletions(-) diff --git a/internal/github/build_pr_evidence_test.go b/internal/github/build_pr_evidence_test.go index 2b1bc08cf..de21bc2ef 100644 --- a/internal/github/build_pr_evidence_test.go +++ b/internal/github/build_pr_evidence_test.go @@ -5,6 +5,7 @@ import ( "testing" "time" + "github.com/kosli-dev/cli/internal/types" "github.com/shurcooL/graphql" "github.com/stretchr/testify/require" ) @@ -132,10 +133,7 @@ func TestBuildPREvidence_RecordsCommitSignature(t *testing.T) { node.Commit.CommittedDate = "2026-03-01T12:00:00Z" node.Commit.Author.Name = "Steve Tooke" node.Commit.Author.Email = "tooky@kosli.com" - node.Commit.Signature = &struct { - IsValid graphql.Boolean - State graphql.String - }{IsValid: true, State: "VALID"} + node.Commit.Signature = &graphqlSignature{IsValid: true, State: "VALID"} evidence, err := buildPREvidence( "https://github.com/kosli-dev/cli/pull/671", @@ -208,8 +206,7 @@ func TestBuildPREvidence_UnsignedCommitHasNoSignatureFields(t *testing.T) { // An approval whose commit GitHub no longer has must leave commit_sha out of // the payload rather than send an empty string, and so must an unknown head. func TestBuildPREvidence_OmitsUnknownReviewedAndHeadCommits(t *testing.T) { - review := graphqlReviewNode{State: "APPROVED", SubmittedAt: "2026-03-01T13:00:00Z"} - review.Author.Login = "grace" + review := reviewNode("User", "grace", "APPROVED") evidence, err := buildPREvidence( "https://github.com/kosli-dev/cli/pull/671", @@ -226,3 +223,74 @@ func TestBuildPREvidence_OmitsUnknownReviewedAndHeadCommits(t *testing.T) { require.NotContains(t, string(payload), "commit_sha") require.NotContains(t, string(payload), "head_sha") } + +func signedCommitNode(sha string, sig *graphqlSignature) graphqlCommitNode { + node := graphqlCommitNode{} + node.Commit.Oid = graphql.String(sha) + node.Commit.CommittedDate = "2026-03-01T12:00:00Z" + node.Commit.Signature = sig + return node +} + +// A verified signature names who signed, which the commit's own author +// fields cannot: whoever writes the commit sets those. +func TestBuildPREvidence_RecordsCommitSigner(t *testing.T) { + bySigner := &graphqlSignature{IsValid: true, State: "VALID"} + bySigner.Signer = &struct{ Login graphql.String }{Login: "alice"} + byGitHub := &graphqlSignature{IsValid: true, State: "VALID", WasSignedByGitHub: true} + byGitHub.Signer = &struct{ Login graphql.String }{Login: "web-flow"} + + evidence, err := buildPREvidence( + "https://github.com/kosli-dev/cli/pull/671", + "0e723254516c841126e81f76100be57258ff1386", + "MERGED", "tooky", "2026-03-01T09:00:00Z", "", + "title", "feature", "main", "", + []graphqlCommitNode{ + signedCommitNode("1111111111111111111111111111111111111111", bySigner), + signedCommitNode("2222222222222222222222222222222222222222", byGitHub), + signedCommitNode("3333333333333333333333333333333333333333", nil), + }, nil, + ) + require.NoError(t, err) + require.Len(t, evidence.Commits, 3) + + require.Equal(t, "alice", evidence.Commits[0].SignerUsername) + require.NotNil(t, evidence.Commits[0].SignedByGitHub) + require.False(t, *evidence.Commits[0].SignedByGitHub) + + require.Equal(t, "", evidence.Commits[1].SignerUsername, + "GitHub's own signing account is not who made the commit") + require.NotNil(t, evidence.Commits[1].SignedByGitHub) + require.True(t, *evidence.Commits[1].SignedByGitHub) + + unsigned, err := json.Marshal(evidence.Commits[2]) + require.NoError(t, err) + require.NotContains(t, string(unsigned), "signer_username") + require.NotContains(t, string(unsigned), "signed_by_github") +} + +func reviewNode(typename, login, state string) graphqlReviewNode { + r := graphqlReviewNode{State: graphql.String(state), SubmittedAt: "2026-03-01T13:00:00Z"} + r.Author.Typename = graphql.String(typename) + r.Author.Login = graphql.String(login) + return r +} + +// Only a person's approval counts: a bot's does not, and nor does a later +// request for changes, which GitHub returns as that reviewer's latest review. +func TestBuildPREvidence_KeepsOnlyHumanApprovals(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, []graphqlReviewNode{ + reviewNode("User", "grace", "APPROVED"), + reviewNode("Bot", "github-actions", "APPROVED"), + reviewNode("User", "linus", "CHANGES_REQUESTED"), + }, + ) + require.NoError(t, err) + require.Len(t, evidence.Approvers, 1) + require.Equal(t, "grace", evidence.Approvers[0].(types.PRApprovals).Username) +} diff --git a/internal/github/github.go b/internal/github/github.go index 40733409b..ed302496e 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -274,17 +274,27 @@ type graphqlCommitNode struct { Login graphql.String } } - Signature *struct { - IsValid graphql.Boolean - State graphql.String - } + Signature *graphqlSignature + } +} + +type graphqlSignature struct { + IsValid graphql.Boolean + State graphql.String + WasSignedByGitHub graphql.Boolean + // The account behind the signing key, or null when it matches none. For a + // commit GitHub signed this is GitHub's own web-flow account. + Signer *struct { + Login graphql.String } } -// graphqlReviewNode is the shared GraphQL node type for approved reviews on a PR. +// graphqlReviewNode is the shared GraphQL node type for each reviewer's latest +// approving or change-requesting review on a PR. type graphqlReviewNode struct { Author struct { - Login graphql.String + Typename graphql.String `graphql:"__typename"` + Login graphql.String } State graphql.String SubmittedAt graphql.String @@ -348,13 +358,19 @@ func buildPREvidence( // Capture the commit signature when present. A nil signature node means // the commit is unsigned, which must stay distinct from a present but // invalid signature (verified=false) — so leave the fields nil (server#5892). - var verified *bool + var verified, signedByGitHub *bool var signatureState *string - if n.Commit.Signature != nil { - v := bool(n.Commit.Signature.IsValid) - s := string(n.Commit.Signature.State) + signerUsername := "" + if sig := n.Commit.Signature; sig != nil { + v := bool(sig.IsValid) + s := string(sig.State) + g := bool(sig.WasSignedByGitHub) verified = &v signatureState = &s + signedByGitHub = &g + if sig.Signer != nil && !g { + signerUsername = string(sig.Signer.Login) + } } evidence.Commits = append(evidence.Commits, types.Commit{ SHA: string(n.Commit.Oid), @@ -366,10 +382,16 @@ func buildPREvidence( URL: string(n.Commit.URL), Verified: verified, SignatureState: signatureState, + SignerUsername: signerUsername, + SignedByGitHub: signedByGitHub, }) } for _, r := range reviewNodes { + // A bot's approval is not a second person's review. + if r.State != "APPROVED" || r.Author.Typename != "User" { + continue + } submittedAt, err := time.Parse(time.RFC3339, string(r.SubmittedAt)) if err != nil { return nil, err @@ -424,7 +446,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:"latestOpinionatedReviews(first: 100, writersOnly: true, after: $reviewCursor)"` } `graphql:"pullRequest(number: $prNumber)"` } `graphql:"repository(owner: $owner, name: $repo)"` } @@ -517,7 +539,7 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence Reviews struct { Nodes []graphqlReviewNode PageInfo pageInfo - } `graphql:"reviews(first: 100, states: APPROVED, after: $reviewCursor)"` + } `graphql:"latestOpinionatedReviews(first: 100, writersOnly: true, 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..66b2b0cba 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:"latestOpinionatedReviews(first: 100, writersOnly: true, after: $cursor)"` } `graphql:"pullRequest(number: $prNumber)"` } `graphql:"repository(owner: $owner, name: $repo)"` } diff --git a/internal/github/pr_pagination_test.go b/internal/github/pr_pagination_test.go index b50e6d275..997f4958b 100644 --- a/internal/github/pr_pagination_test.go +++ b/internal/github/pr_pagination_test.go @@ -73,13 +73,13 @@ func commitNodeJSON(sha string) string { // reviewNodeJSON is one node of a GraphQL reviews connection, given on the // commit "reviewed-by-". func reviewNodeJSON(login string) string { - return fmt.Sprintf(`{"author":{"login":%q},"state":"APPROVED","submittedAt":"2026-03-01T13:00:00Z",`+ + return fmt.Sprintf(`{"author":{"__typename":"User","login":%q},"state":"APPROVED","submittedAt":"2026-03-01T13:00:00Z",`+ `"commit":{"oid":"reviewed-by-%s"}}`, login, login) } // reviewNodeWithoutCommitJSON is a review whose commit GitHub no longer has. func reviewNodeWithoutCommitJSON(login string) string { - return fmt.Sprintf(`{"author":{"login":%q},"state":"APPROVED","submittedAt":"2026-03-01T13:00:00Z",`+ + return fmt.Sprintf(`{"author":{"__typename":"User","login":%q},"state":"APPROVED","submittedAt":"2026-03-01T13:00:00Z",`+ `"commit":null}`, login) } @@ -96,7 +96,7 @@ func prJSON(commits, reviews string) string { return fmt.Sprintf(`{"title":"A PR","state":"MERGED","headRefName":"feature","headRefOid":"head-sha","baseRefName":"main",`+ `"url":"https://github.com/o/r/pull/1","createdAt":"2026-03-01T11:00:00Z",`+ `"mergedAt":"2026-03-01T14:00:00Z","mergeCommit":{"oid":"merge-sha"},`+ - `"author":{"login":"ada"},"commits":%s,"reviews":%s}`, commits, reviews) + `"author":{"login":"ada"},"commits":%s,"latestOpinionatedReviews":%s}`, commits, reviews) } func byPRNumberResponse(commits, reviews string) string { @@ -111,7 +111,7 @@ func commitsPageResponse(nodes []string, nextCursor string) string { // reviewsPageResponse is a follow-up page reply selecting only reviews. func reviewsPageResponse(nodes []string, nextCursor string) string { - return fmt.Sprintf(`{"data":{"repository":{"pullRequest":{"reviews":%s}}}}`, + return fmt.Sprintf(`{"data":{"repository":{"pullRequest":{"latestOpinionatedReviews":%s}}}}`, connectionJSON(nodes, nextCursor)) } @@ -139,7 +139,7 @@ func TestPREvidenceByPRNumber_FollowsCommitPages(t *testing.T) { require.Equal(t, []string{"sha1", "sha2", "sha3"}, shasOf(t, newPaginationConfig(ts), 1)) require.Len(t, ts.bodies, 2) require.Contains(t, ts.bodies[1], "c1", "follow-up must carry the cursor") - require.NotContains(t, ts.bodies[1], "reviews(", "follow-up must not re-fetch reviews") + require.NotContains(t, ts.bodies[1], "latestOpinionatedReviews(", "follow-up must not re-fetch reviews") } func TestPREvidenceByPRNumber_FollowsReviewPages(t *testing.T) { @@ -209,7 +209,7 @@ func v2PRNodeInRepoJSON(owner, repo string, number int, commits, reviews string) `"title":"A PR","state":"MERGED","headRefName":"feature","headRefOid":"head-sha-%d",`+ `"baseRefName":"main","url":"https://github.com/%s/%s/pull/%d",`+ `"createdAt":"2026-03-01T11:00:00Z","mergedAt":"2026-03-01T14:00:00Z",`+ - `"author":{"login":"ada"},"commits":%s,"reviews":%s}`, + `"author":{"login":"ada"},"commits":%s,"latestOpinionatedReviews":%s}`, number, repo, owner, number, owner, repo, number, commits, reviews) } @@ -384,6 +384,32 @@ func TestPREvidenceByPRNumber_RecordsHeadAndReviewedCommits(t *testing.T) { require.Contains(t, ts.bodies[0], "headRefOid") require.Contains(t, ts.bodies[0], "commit{oid}") require.Contains(t, ts.bodies[1], "commit{oid}") + for _, body := range ts.bodies { + require.Contains(t, body, "latestOpinionatedReviews(first: 100, writersOnly: true") + require.Contains(t, body, "author{__typename,login}") + } +} + +func TestPREvidenceByPRNumber_RecordsCommitSigners(t *testing.T) { + signed := func(sha, signer string, byGitHub bool) string { + return strings.Replace(commitNodeJSON(sha), `"signature":null`, + fmt.Sprintf(`"signature":{"isValid":true,"state":"VALID","wasSignedByGitHub":%t,"signer":{"login":%q}}`, byGitHub, signer), 1) + } + ts := newGraphQLTestServer(t, byPRNumberResponse( + connectionJSON([]string{signed("sha1", "ada", false), signed("sha2", "web-flow", true), commitNodeJSON("sha3")}, ""), + connectionJSON(nil, ""), + )) + + evidence, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) + require.NoError(t, err) + require.Len(t, evidence.Commits, 3) + require.Equal(t, "ada", evidence.Commits[0].SignerUsername) + require.False(t, *evidence.Commits[0].SignedByGitHub) + require.Equal(t, "", evidence.Commits[1].SignerUsername) + require.True(t, *evidence.Commits[1].SignedByGitHub) + require.Nil(t, evidence.Commits[2].SignedByGitHub, "an unsigned commit records no signature facts") + require.Contains(t, ts.bodies[0], "signer{login}") + require.Contains(t, ts.bodies[0], "wasSignedByGitHub") } func TestPREvidenceForCommitV2_RecordsHeadAndReviewedCommits(t *testing.T) { @@ -400,4 +426,6 @@ func TestPREvidenceForCommitV2_RecordsHeadAndReviewedCommits(t *testing.T) { require.Equal(t, []string{"reviewed-by-ada"}, approverCommitSHAs(prs[0])) require.Contains(t, ts.bodies[0], "headRefOid") require.Contains(t, ts.bodies[0], "commit{oid}") + require.Contains(t, ts.bodies[0], "latestOpinionatedReviews(first: 100, writersOnly: true") + require.Contains(t, ts.bodies[0], "signer{login}") } diff --git a/internal/types/types.go b/internal/types/types.go index f4655f7f4..77c6b0a8f 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -47,6 +47,8 @@ type Commit struct { URL string `json:"url,omitempty"` Verified *bool `json:"verified,omitempty"` SignatureState *string `json:"signature_state,omitempty"` + SignerUsername string `json:"signer_username,omitempty"` + SignedByGitHub *bool `json:"signed_by_github,omitempty"` } type PRRetriever interface { From 53ec15a606624168fc7939a3128254c84a581f7d Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Fri, 2 Oct 2026 16:33:44 +0100 Subject: [PATCH 3/7] fix(never-alone): count only approvals on the PR's final commit, and identify committers by signature The four-eyes policy decided whether an approval still covered a pull request by comparing it with the newest commit's date, and decided who wrote each commit from the commit's author fields. Both come from the commit itself, so they can't show which code a reviewer saw or who made the commit. The policy now: - counts an approval only when the commit it was given on (approvers[].commit_sha) is the PR's head commit (head_sha); - treats a commit as identified only if it has a verified signature by a known account (signer_username) or by GitHub (signed_by_github), and requires an independent approval for the signer as well as the named author; - counts only pull requests in the evaluated repository, passed as the new required "repository" param, because an associated PR elsewhere, such as in a fork, has approvers its author may choose. An attestation without the new fields, from an older CLI, gets no approval. The workflow passes github.repository. The policy had no tests. The new Rego tests run under go test through internal/evaluate, so CI runs them with the rest of the suite. Needs a CLI release that sends head_sha, commit_sha, signer_username and signed_by_github; until CI installs it, the four-eyes result on build trails is red. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/init_kosli.yml | 2 +- bin/never_alone/four-eyes-policy.rego | 85 +++++-- bin/never_alone/four-eyes-policy_test.rego | 236 +++++++++++++++++++ internal/evaluate/never_alone_policy_test.go | 23 ++ 4 files changed, 325 insertions(+), 21 deletions(-) create mode 100644 bin/never_alone/four-eyes-policy_test.rego create mode 100644 internal/evaluate/never_alone_policy_test.go diff --git a/.github/workflows/init_kosli.yml b/.github/workflows/init_kosli.yml index 5ec59c752..5511f8b75 100644 --- a/.github/workflows/init_kosli.yml +++ b/.github/workflows/init_kosli.yml @@ -101,7 +101,7 @@ jobs: --flow ${{inputs.flow_name}} \ --org ${{inputs.kosli_org}} \ --no-assert \ - --params '{"attestation_name": "pr"}' \ + --params '{"attestation_name": "pr", "repository": "${{ github.repository }}"}' \ --output json > "4eyes-eval-${{inputs.trail_name}}.json" || echo '{"allow":false,"violations":["evaluate command failed"]}' > "4eyes-eval-${{inputs.trail_name}}.json" - name: Report four-eyes attestation to Kosli diff --git a/bin/never_alone/four-eyes-policy.rego b/bin/never_alone/four-eyes-policy.rego index 9b31e0dab..ec7783395 100644 --- a/bin/never_alone/four-eyes-policy.rego +++ b/bin/never_alone/four-eyes-policy.rego @@ -26,17 +26,22 @@ attestation_name := name if { is_string(name) } else := "pr-review" +# The repository whose PRs count, as "owner/repo". Required: a PR elsewhere, +# such as in a fork, is one whose approvers the author may choose. +repository := lower(data.params.repository) if is_string(data.params.repository) + # --------------------------------------------------------------------------- # Compliance # --------------------------------------------------------------------------- -# A commit is compliant when an associated PR has independent approval -# covering every author after the latest code commit. There is no exemption -# based on the git author string: it is user-controlled, so matching on it -# would let anyone skip review. +# A commit is compliant when an associated PR in the evaluated repository has +# independent approval, on the PR's final commit, covering every author. There +# is no exemption based on the git author string: it is user-controlled, so +# matching on it would let anyone skip review. trail_compliant(trail) if { attest := pr_attest(trail) some pr in attest.pull_requests + pr_in_repo(pr) all_authors_resolved(pr) has_independent_approval(trail, pr) } @@ -66,42 +71,64 @@ is_resolved_username(u) if { u != "ghost" } -# GitHub usernames of all PR branch commit authors. -pr_commit_authors(pr) := {c.author_username | +# GitHub usernames of everyone who wrote PR branch commits: the named author, +# and the signer, who is who actually made the commit. +pr_commit_authors(pr) := {u | some c in pr.commits - is_resolved_username(c.author_username) + some u in [object.get(c, "author_username", null), object.get(c, "signer_username", null)] + is_resolved_username(u) } -# Approver usernames that can satisfy four-eyes for commits up to cutoff. -approved_approvers_after_cutoff(pr, cutoff) := {a.username | +# Approver usernames whose approval was given on the PR's final commit. Commit +# dates are not used: whoever writes a commit sets them. +approvers_on_head(pr) := {a.username | some a in pr.approvers a.state == "APPROVED" is_resolved_username(a.username) - a.timestamp > cutoff + is_string(pr.head_sha) + pr.head_sha != "" + a.commit_sha == pr.head_sha } -# Latest Unix timestamp among PR branch commits. -latest_commit_ts(pr) := max({c.timestamp | some c in pr.commits}) +# The PR URL is https://///pull/. +pr_in_repo(pr) if { + parts := split(pr.url, "/") + count(parts) == 7 + parts[5] == "pull" + lower(concat("/", [parts[3], parts[4]])) == repository +} -# Every commit on the PR has an author linked to a GitHub account. +# Every commit on the PR has an author linked to a GitHub account and a +# verified signature, by a known account or by GitHub. Without the signature +# the author is only what the commit says, which its writer chooses. all_authors_resolved(pr) if { every c in pr.commits { is_resolved_username(object.get(c, "author_username", null)) + signed_by_known_identity(c) } } +signed_by_known_identity(c) if { + c.verified == true + is_resolved_username(object.get(c, "signer_username", null)) +} + +signed_by_known_identity(c) if { + c.verified == true + c.signed_by_github == true +} + # A commit is the merge commit when the PR's merge_commit field matches the # trail name (which is the commit SHA). Covers squash, regular, and rebase merges. is_merge_commit(trail, pr) if { trail.name == pr.merge_commit } -# Regular commit: PR branch authors + PR author all need independent approval after last code commit. +# Regular commit: PR branch authors + PR author all need independent approval on the final commit. has_independent_approval(trail, pr) if { not is_merge_commit(trail, pr) - cutoff := latest_commit_ts(pr) all_authors := pr_commit_authors(pr) | {pr.author} - eligible_approvers := approved_approvers_after_cutoff(pr, cutoff) + eligible_approvers := approvers_on_head(pr) count(all_authors) > 0 # At least one approver must exist to satisfy four-eyes. @@ -116,9 +143,8 @@ has_independent_approval(trail, pr) if { # The merge button clicker did not write code and requires no separate review. has_independent_approval(trail, pr) if { is_merge_commit(trail, pr) - cutoff := latest_commit_ts(pr) all_authors := pr_commit_authors(pr) - eligible_approvers := approved_approvers_after_cutoff(pr, cutoff) + eligible_approvers := approvers_on_head(pr) count(all_authors) > 0 # At least one approver must exist to satisfy four-eyes. @@ -146,6 +172,10 @@ violations contains "Policy error: input.trails is empty - nothing to evaluate" count(input.trails) == 0 } +violations contains "Policy error: data.params.repository is not set - pass --params '{\"repository\": \"owner/repo\"}'" if { + not repository +} + # Missing attestation: no PR review data collected for this commit. violations contains msg if { some trail in input.trails @@ -169,6 +199,20 @@ violations contains msg if { ) } +# Unverifiable signer: commit has no verified signature by a known account or GitHub. +violations contains msg if { + some trail in input.trails + not trail_compliant(trail) + attest := pr_attest(trail) + some pr in attest.pull_requests + some c in pr.commits + not signed_by_known_identity(c) + msg := sprintf( + "PR %v: commit %v has no verified signature - who made it is unverifiable", + [pr.url, c.sha1], + ) +} + # Missing PR: commit has no associated merged PR. violations contains msg if { some trail in input.trails @@ -187,8 +231,8 @@ violations contains msg if { count(attest.pull_requests) > 0 not any_pr_fully_approved(trail, attest) msg := sprintf( - "Commit %v: no independent approval after latest code commit", - [trail.name], + "Commit %v: no PR in %v has an independent approval on its final commit", + [trail.name, repository], ) } @@ -197,6 +241,7 @@ violations contains msg if { # "unverifiable identity". any_pr_fully_approved(trail, attest) if { some pr in attest.pull_requests + pr_in_repo(pr) all_authors_resolved(pr) has_independent_approval(trail, pr) } diff --git a/bin/never_alone/four-eyes-policy_test.rego b/bin/never_alone/four-eyes-policy_test.rego new file mode 100644 index 000000000..c173a2c7d --- /dev/null +++ b/bin/never_alone/four-eyes-policy_test.rego @@ -0,0 +1,236 @@ +package policy_test + +import data.policy +import rego.v1 + +# Run: opa test bin/never_alone/four-eyes-policy.rego bin/never_alone/four-eyes-policy_test.rego -v + +first := "1111111111111111111111111111111111111111" + +later := "2222222222222222222222222222222222222222" + +merge := "3333333333333333333333333333333333333333" + +params := {"repository": "o/r"} + +# A commit written and signed by user. +commit(sha, ts, user) := signed_commit(sha, ts, user, user) + +# A commit naming author as its author, with a verified signature by signer. +signed_commit(sha, ts, author, signer) := { + "sha1": sha, "author": sprintf("%v <%v@example.com>", [author, author]), "author_username": author, "timestamp": ts, + "verified": true, "signer_username": signer, "signed_by_github": false, +} + +approval(user, sha) := {"username": user, "state": "APPROVED", "timestamp": 1000050, "commit_sha": sha} + +pr(commits, approvers, head) := { + "url": "https://github.com/o/r/pull/1", + "author": "alice", + "merge_commit": merge, + "head_sha": head, + "commits": commits, + "approvers": approvers, +} + +trail(name, prs) := { + "name": name, + "git_commit_info": {"author": "alice ", "sha1": name}, + "compliance_status": {"attestations_statuses": {"pr-review": {"attestation_type": "pull_request", "pull_requests": prs}}}, +} + +allowed(prs) if policy.allow with input as {"trails": [trail(merge, prs)]} with data.params as params + +one_commit := [commit(first, 1000000, "alice")] + +# A later push dated before the approval, as a backdated author or committer date would be. +backdated_push := array.concat(one_commit, [commit(later, 500, "alice")]) + +test_approval_on_head_passes if { + allowed([pr(one_commit, [approval("bob", first)], first)]) +} + +test_push_after_approval_fails if { + not allowed([pr(array.concat(one_commit, [commit(later, 1000100, "alice")]), [approval("bob", first)], later)]) +} + +test_backdated_push_after_approval_fails if { + not allowed([pr(backdated_push, [approval("bob", first)], later)]) +} + +test_reapproval_on_new_head_passes if { + allowed([pr(backdated_push, [approval("bob", first), approval("bob", later)], later)]) +} + +test_second_reviewer_on_new_head_passes if { + allowed([pr(backdated_push, [approval("bob", first), approval("carol", later)], later)]) +} + +test_self_approval_on_head_fails if { + not allowed([pr(one_commit, [approval("alice", first)], first)]) +} + +test_dismissed_review_on_head_fails if { + not allowed([pr(one_commit, [object.union(approval("bob", first), {"state": "DISMISSED"})], first)]) +} + +test_missing_head_and_reviewed_commit_fails if { + not allowed([pr(one_commit, [object.remove(approval("bob", first), ["commit_sha"])], null)]) +} + +test_missing_reviewed_commit_fails if { + not allowed([pr(one_commit, [object.remove(approval("bob", first), ["commit_sha"])], first)]) +} + +test_missing_head_fails if { + not allowed([object.remove(pr(one_commit, [approval("bob", first)], first), ["head_sha"])]) +} + +test_empty_head_and_reviewed_commit_fails if { + not allowed([pr(one_commit, [approval("bob", "")], "")]) +} + +# The PR author must be covered when the trail is not the merge commit. +test_non_merge_trail_approved_on_head_passes if { + policy.allow with input as {"trails": [trail(later, [pr(one_commit, [approval("bob", first)], first)])]} + with data.params as params +} + +test_non_merge_trail_self_approved_by_pr_author_fails if { + not policy.allow with input as {"trails": [trail(later, [pr([commit(first, 1000000, "dave")], [approval("alice", first)], first)])]} + with data.params as params +} + +# The policy reads no author name, so a bot-like or service-account name is reviewed like any other. +test_bot_named_trail_author_needs_review if { + t := object.union(trail(merge, [pr(backdated_push, [approval("bob", first)], later)]), {"git_commit_info": {"author": "alice[bot] ", "sha1": merge}}) + not policy.allow with input as {"trails": [t]} with data.params as params +} + +fork_pr(p) := object.union(p, {"url": "https://github.com/alice/fork/pull/1"}) + +test_fork_pr_approval_does_not_count if { + not allowed([ + pr(backdated_push, [approval("bob", first)], later), + fork_pr(pr(backdated_push, [approval("alice-alt", later)], later)), + ]) +} + +test_unrelated_fork_pr_does_not_block if { + allowed([pr(one_commit, [approval("bob", first)], first), fork_pr(pr(one_commit, [], first))]) +} + +test_enterprise_host_in_repo_passes if { + allowed([object.union(pr(one_commit, [approval("bob", first)], first), {"url": "https://ghe.example.com/o/r/pull/1"})]) +} + +test_malformed_pr_url_fails if { + not allowed([object.union(pr(one_commit, [approval("bob", first)], first), {"url": "https://github.com/o/r/pull/1/files"})]) +} + +test_repository_compared_case_insensitively if { + policy.allow with input as {"trails": [trail(merge, [pr(one_commit, [approval("bob", first)], first)])]} + with data.params as {"repository": "O/R"} +} + +test_missing_repository_param_fails_and_says_why if { + inp := {"trails": [trail(merge, [pr(one_commit, [approval("bob", first)], first)])]} + not policy.allow with input as inp + v := policy.violations with input as inp + some msg in v + contains(msg, "data.params.repository is not set") +} + +test_unapproved_commit_violation_names_the_repository if { + v := policy.violations with input as {"trails": [trail(merge, [pr(backdated_push, [approval("bob", first)], later)])]} + with data.params as params + v == {sprintf("Commit %v: no PR in o/r has an independent approval on its final commit", [merge])} +} + +test_null_head_and_reviewed_commit_fails if { + not allowed([pr(one_commit, [object.union(approval("bob", first), {"commit_sha": null})], null)]) +} + +test_pr_url_compared_case_insensitively if { + allowed([object.union(pr(one_commit, [approval("bob", first)], first), {"url": "https://github.com/O/R/pull/1"})]) +} + +test_non_pull_request_url_fails if { + not allowed([object.union(pr(one_commit, [approval("bob", first)], first), {"url": "https://github.com/o/r/issues/1"})]) +} + +test_unresolved_commit_author_blocks_approval if { + not allowed([pr(array.concat(one_commit, [{"sha1": later, "author": "x ", "timestamp": 1000000}]), [approval("bob", later)], later)]) +} + +test_ghost_commit_author_blocks_approval if { + not allowed([pr(array.concat(one_commit, [commit(later, 1000000, "ghost")]), [approval("bob", later)], later)]) +} + +test_ghost_approver_does_not_count if { + not allowed([pr(one_commit, [approval("ghost", first)], first)]) +} + +test_empty_commit_author_blocks_approval if { + not allowed([pr(array.concat(one_commit, [commit(later, 1000000, "")]), [approval("bob", later)], later)]) +} + +test_unsigned_commit_blocks_approval if { + not allowed([pr([object.remove(commit(first, 1000000, "alice"), ["verified", "signer_username", "signed_by_github"])], [approval("bob", first)], first)]) +} + +test_invalid_signature_blocks_approval if { + not allowed([pr([object.union(commit(first, 1000000, "alice"), {"verified": false})], [approval("bob", first)], first)]) +} + +test_signature_without_known_signer_blocks_approval if { + not allowed([pr([object.remove(commit(first, 1000000, "alice"), ["signer_username"])], [approval("bob", first)], first)]) +} + +test_ghost_signer_blocks_approval if { + not allowed([pr([signed_commit(first, 1000000, "alice", "ghost")], [approval("bob", first)], first)]) +} + +test_commit_signed_by_github_passes if { + allowed([pr([object.union(object.remove(commit(first, 1000000, "alice"), ["signer_username"]), {"signed_by_github": true})], [approval("bob", first)], first)]) +} + +# Naming someone else as the author doesn't let the signer approve their own work. +test_signer_cannot_approve_own_commit_under_another_author if { + not allowed([pr([signed_commit(first, 1000000, "bob", "alice")], [approval("alice", first)], first)]) +} + +test_independent_approval_covers_author_and_signer if { + allowed([pr([signed_commit(first, 1000000, "bob", "alice")], [approval("carol", first)], first)]) +} + +test_unsigned_commit_violation_says_why if { + unsigned := object.remove(commit(first, 1000000, "alice"), ["verified", "signer_username", "signed_by_github"]) + v := policy.violations with input as {"trails": [trail(merge, [pr([unsigned], [approval("bob", first)], first)])]} + with data.params as params + some msg in v + contains(msg, "no verified signature") +} + +test_invalid_github_signature_blocks_approval if { + not allowed([pr([object.union(object.remove(commit(first, 1000000, "alice"), ["signer_username"]), {"signed_by_github": true, "verified": false})], [approval("bob", first)], first)]) +} + +test_non_merge_trail_with_old_approval_fails if { + not policy.allow with input as {"trails": [trail(later, [pr(backdated_push, [approval("bob", first)], later)])]} + with data.params as params +} + +test_unlinked_author_with_known_signer_blocks_approval if { + not allowed([pr([object.remove(commit(first, 1000000, "alice"), ["author_username"])], [approval("bob", first)], first)]) +} + +test_non_merge_trail_pr_author_needs_independent_approval if { + not policy.allow with input as {"trails": [trail(later, [object.union(pr(one_commit, [approval("carol", first)], first), {"author": "carol"})])]} + with data.params as params +} + +test_dismissed_review_on_head_fails_on_non_merge_trail if { + not policy.allow with input as {"trails": [trail(later, [pr(one_commit, [object.union(approval("bob", first), {"state": "DISMISSED"})], first)])]} + with data.params as params +} diff --git a/internal/evaluate/never_alone_policy_test.go b/internal/evaluate/never_alone_policy_test.go new file mode 100644 index 000000000..bd090aa36 --- /dev/null +++ b/internal/evaluate/never_alone_policy_test.go @@ -0,0 +1,23 @@ +package evaluate + +import ( + "context" + "testing" + + "github.com/open-policy-agent/opa/v1/tester" + "github.com/stretchr/testify/require" +) + +// TestNeverAlonePolicy runs the Rego tests for the four-eyes policy this +// repository's CI evaluates, so they run wherever go test does. +func TestNeverAlonePolicy(t *testing.T) { + results, err := tester.Run(context.Background(), + "../../bin/never_alone/four-eyes-policy.rego", + "../../bin/never_alone/four-eyes-policy_test.rego", + ) + require.NoError(t, err) + require.NotEmpty(t, results, "no Rego tests were found") + for _, r := range results { + require.Truef(t, r.Pass(), "%s failed (error: %v)", r.Name, r.Error) + } +} From 3d602f94e830a94dc5a941e5a3befddcd6f75d19 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Mon, 5 Oct 2026 09:21:27 +0100 Subject: [PATCH 4/7] refactor(github): send the platform-signature flag as signed_by_platform The commit record the Kosli server stores is shared by every git provider, so its fields name provider-neutral concepts. "Signed with the hosting platform's own key" applies to GitLab and others as well as GitHub, so the field is signed_by_platform; the GitHub code still reads GitHub's wasSignedByGitHub and maps it. The four-eyes policy reads the new name. Co-Authored-By: Claude Opus 5.5 --- bin/never_alone/four-eyes-policy.rego | 2 +- bin/never_alone/four-eyes-policy_test.rego | 12 +++++----- internal/github/build_pr_evidence_test.go | 10 ++++----- internal/github/github.go | 26 +++++++++++----------- internal/github/pr_pagination_test.go | 6 ++--- internal/types/types.go | 22 +++++++++--------- 6 files changed, 39 insertions(+), 39 deletions(-) diff --git a/bin/never_alone/four-eyes-policy.rego b/bin/never_alone/four-eyes-policy.rego index ec7783395..56e70c5a1 100644 --- a/bin/never_alone/four-eyes-policy.rego +++ b/bin/never_alone/four-eyes-policy.rego @@ -115,7 +115,7 @@ signed_by_known_identity(c) if { signed_by_known_identity(c) if { c.verified == true - c.signed_by_github == true + c.signed_by_platform == true } # A commit is the merge commit when the PR's merge_commit field matches the diff --git a/bin/never_alone/four-eyes-policy_test.rego b/bin/never_alone/four-eyes-policy_test.rego index c173a2c7d..263772034 100644 --- a/bin/never_alone/four-eyes-policy_test.rego +++ b/bin/never_alone/four-eyes-policy_test.rego @@ -19,7 +19,7 @@ commit(sha, ts, user) := signed_commit(sha, ts, user, user) # A commit naming author as its author, with a verified signature by signer. signed_commit(sha, ts, author, signer) := { "sha1": sha, "author": sprintf("%v <%v@example.com>", [author, author]), "author_username": author, "timestamp": ts, - "verified": true, "signer_username": signer, "signed_by_github": false, + "verified": true, "signer_username": signer, "signed_by_platform": false, } approval(user, sha) := {"username": user, "state": "APPROVED", "timestamp": 1000050, "commit_sha": sha} @@ -176,7 +176,7 @@ test_empty_commit_author_blocks_approval if { } test_unsigned_commit_blocks_approval if { - not allowed([pr([object.remove(commit(first, 1000000, "alice"), ["verified", "signer_username", "signed_by_github"])], [approval("bob", first)], first)]) + not allowed([pr([object.remove(commit(first, 1000000, "alice"), ["verified", "signer_username", "signed_by_platform"])], [approval("bob", first)], first)]) } test_invalid_signature_blocks_approval if { @@ -191,8 +191,8 @@ test_ghost_signer_blocks_approval if { not allowed([pr([signed_commit(first, 1000000, "alice", "ghost")], [approval("bob", first)], first)]) } -test_commit_signed_by_github_passes if { - allowed([pr([object.union(object.remove(commit(first, 1000000, "alice"), ["signer_username"]), {"signed_by_github": true})], [approval("bob", first)], first)]) +test_commit_signed_by_platform_passes if { + allowed([pr([object.union(object.remove(commit(first, 1000000, "alice"), ["signer_username"]), {"signed_by_platform": true})], [approval("bob", first)], first)]) } # Naming someone else as the author doesn't let the signer approve their own work. @@ -205,7 +205,7 @@ test_independent_approval_covers_author_and_signer if { } test_unsigned_commit_violation_says_why if { - unsigned := object.remove(commit(first, 1000000, "alice"), ["verified", "signer_username", "signed_by_github"]) + unsigned := object.remove(commit(first, 1000000, "alice"), ["verified", "signer_username", "signed_by_platform"]) v := policy.violations with input as {"trails": [trail(merge, [pr([unsigned], [approval("bob", first)], first)])]} with data.params as params some msg in v @@ -213,7 +213,7 @@ test_unsigned_commit_violation_says_why if { } test_invalid_github_signature_blocks_approval if { - not allowed([pr([object.union(object.remove(commit(first, 1000000, "alice"), ["signer_username"]), {"signed_by_github": true, "verified": false})], [approval("bob", first)], first)]) + not allowed([pr([object.union(object.remove(commit(first, 1000000, "alice"), ["signer_username"]), {"signed_by_platform": true, "verified": false})], [approval("bob", first)], first)]) } test_non_merge_trail_with_old_approval_fails if { diff --git a/internal/github/build_pr_evidence_test.go b/internal/github/build_pr_evidence_test.go index de21bc2ef..47040da7e 100644 --- a/internal/github/build_pr_evidence_test.go +++ b/internal/github/build_pr_evidence_test.go @@ -255,18 +255,18 @@ func TestBuildPREvidence_RecordsCommitSigner(t *testing.T) { require.Len(t, evidence.Commits, 3) require.Equal(t, "alice", evidence.Commits[0].SignerUsername) - require.NotNil(t, evidence.Commits[0].SignedByGitHub) - require.False(t, *evidence.Commits[0].SignedByGitHub) + require.NotNil(t, evidence.Commits[0].SignedByPlatform) + require.False(t, *evidence.Commits[0].SignedByPlatform) require.Equal(t, "", evidence.Commits[1].SignerUsername, "GitHub's own signing account is not who made the commit") - require.NotNil(t, evidence.Commits[1].SignedByGitHub) - require.True(t, *evidence.Commits[1].SignedByGitHub) + require.NotNil(t, evidence.Commits[1].SignedByPlatform) + require.True(t, *evidence.Commits[1].SignedByPlatform) unsigned, err := json.Marshal(evidence.Commits[2]) require.NoError(t, err) require.NotContains(t, string(unsigned), "signer_username") - require.NotContains(t, string(unsigned), "signed_by_github") + require.NotContains(t, string(unsigned), "signed_by_platform") } func reviewNode(typename, login, state string) graphqlReviewNode { diff --git a/internal/github/github.go b/internal/github/github.go index ed302496e..0b0ebeebb 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -358,7 +358,7 @@ func buildPREvidence( // Capture the commit signature when present. A nil signature node means // the commit is unsigned, which must stay distinct from a present but // invalid signature (verified=false) — so leave the fields nil (server#5892). - var verified, signedByGitHub *bool + var verified, signedByPlatform *bool var signatureState *string signerUsername := "" if sig := n.Commit.Signature; sig != nil { @@ -367,23 +367,23 @@ func buildPREvidence( g := bool(sig.WasSignedByGitHub) verified = &v signatureState = &s - signedByGitHub = &g + signedByPlatform = &g if sig.Signer != nil && !g { signerUsername = string(sig.Signer.Login) } } evidence.Commits = append(evidence.Commits, types.Commit{ - SHA: string(n.Commit.Oid), - Message: string(n.Commit.MessageHeadline), - Author: fmt.Sprintf("%s <%s>", string(n.Commit.Author.Name), string(n.Commit.Author.Email)), - AuthorUsername: authorUsername, - Timestamp: timestamp.Unix(), - Branch: headRef, - URL: string(n.Commit.URL), - Verified: verified, - SignatureState: signatureState, - SignerUsername: signerUsername, - SignedByGitHub: signedByGitHub, + SHA: string(n.Commit.Oid), + Message: string(n.Commit.MessageHeadline), + Author: fmt.Sprintf("%s <%s>", string(n.Commit.Author.Name), string(n.Commit.Author.Email)), + AuthorUsername: authorUsername, + Timestamp: timestamp.Unix(), + Branch: headRef, + URL: string(n.Commit.URL), + Verified: verified, + SignatureState: signatureState, + SignerUsername: signerUsername, + SignedByPlatform: signedByPlatform, }) } diff --git a/internal/github/pr_pagination_test.go b/internal/github/pr_pagination_test.go index 997f4958b..9ea0d4e0f 100644 --- a/internal/github/pr_pagination_test.go +++ b/internal/github/pr_pagination_test.go @@ -404,10 +404,10 @@ func TestPREvidenceByPRNumber_RecordsCommitSigners(t *testing.T) { require.NoError(t, err) require.Len(t, evidence.Commits, 3) require.Equal(t, "ada", evidence.Commits[0].SignerUsername) - require.False(t, *evidence.Commits[0].SignedByGitHub) + require.False(t, *evidence.Commits[0].SignedByPlatform) require.Equal(t, "", evidence.Commits[1].SignerUsername) - require.True(t, *evidence.Commits[1].SignedByGitHub) - require.Nil(t, evidence.Commits[2].SignedByGitHub, "an unsigned commit records no signature facts") + require.True(t, *evidence.Commits[1].SignedByPlatform) + require.Nil(t, evidence.Commits[2].SignedByPlatform, "an unsigned commit records no signature facts") require.Contains(t, ts.bodies[0], "signer{login}") require.Contains(t, ts.bodies[0], "wasSignedByGitHub") } diff --git a/internal/types/types.go b/internal/types/types.go index 77c6b0a8f..61a0cd13d 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -38,17 +38,17 @@ type PRApprovals struct { } type Commit struct { - SHA string `json:"sha1"` - Message string `json:"message"` - Author string `json:"author"` - AuthorUsername string `json:"author_username,omitempty"` - Timestamp int64 `json:"timestamp"` - Branch string `json:"branch"` - URL string `json:"url,omitempty"` - Verified *bool `json:"verified,omitempty"` - SignatureState *string `json:"signature_state,omitempty"` - SignerUsername string `json:"signer_username,omitempty"` - SignedByGitHub *bool `json:"signed_by_github,omitempty"` + SHA string `json:"sha1"` + Message string `json:"message"` + Author string `json:"author"` + AuthorUsername string `json:"author_username,omitempty"` + Timestamp int64 `json:"timestamp"` + Branch string `json:"branch"` + URL string `json:"url,omitempty"` + Verified *bool `json:"verified,omitempty"` + SignatureState *string `json:"signature_state,omitempty"` + SignerUsername string `json:"signer_username,omitempty"` + SignedByPlatform *bool `json:"signed_by_platform,omitempty"` } type PRRetriever interface { From ebd82b01dfb59d6894640de4d3ac141b7a8da328 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Tue, 6 Oct 2026 18:46:31 +0100 Subject: [PATCH 5/7] feat(github): record every PR review as facts, and judge them in the four-eyes policy The pull request attestation should carry what GitHub reports and leave every judgement to the policy evaluated in Kosli. The earlier commits on this branch filtered reviews in the CLI (writers only, each reviewer's latest review only, people only) and left out the signer GitHub reports for commits it signs. Those are four-eyes rules, and another policy may want what they dropped. The CLI now records every submitted review in a new reviews field, in any state, with the commit it was given on, author_type ("user", "bot" or "other") and has_write_access, which GitHub reports on each review as authorCanPushToRepository. Unsubmitted reviews are not requested: only their author can see them and they have no submission time. A PR with no reviews records an empty list, so it stays distinct from evidence where reviews weren't recorded. approvers is back to every approval, in the shape it had before, since existing policies and the Kosli UI treat it as the approvals. Signers are recorded as GitHub reports them, including web-flow. The four-eyes policy now reads reviews and decides: an approval counts when it is by a user with write access, on the PR's final commit, and not followed in the same second or later by a request for changes or a dismissal from the same reviewer. Review times are set by GitHub. An attestation without reviews gets no approval. Co-Authored-By: Claude Opus 5.5 --- bin/never_alone/four-eyes-policy.rego | 28 +++++-- bin/never_alone/four-eyes-policy_test.rego | 86 ++++++++++++++++++++-- internal/github/build_pr_evidence_test.go | 69 +++++++++++++---- internal/github/github.go | 57 +++++++++----- internal/github/pagination.go | 6 +- internal/github/pr_pagination_test.go | 31 ++++---- internal/types/types.go | 30 +++++--- 7 files changed, 235 insertions(+), 72 deletions(-) diff --git a/bin/never_alone/four-eyes-policy.rego b/bin/never_alone/four-eyes-policy.rego index 56e70c5a1..ee6cf01ad 100644 --- a/bin/never_alone/four-eyes-policy.rego +++ b/bin/never_alone/four-eyes-policy.rego @@ -71,23 +71,41 @@ is_resolved_username(u) if { u != "ghost" } -# GitHub usernames of everyone who wrote PR branch commits: the named author, -# and the signer, who is who actually made the commit. +# GitHub usernames on PR branch commits: each named author and signing account. pr_commit_authors(pr) := {u | some c in pr.commits some u in [object.get(c, "author_username", null), object.get(c, "signer_username", null)] is_resolved_username(u) } -# Approver usernames whose approval was given on the PR's final commit. Commit -# dates are not used: whoever writes a commit sets them. +# Usernames of people with write access whose approval was given on the PR's +# final commit and not withdrawn. Commit dates are not used: whoever writes a +# commit sets them. Review times are set by GitHub. approvers_on_head(pr) := {a.username | - some a in pr.approvers + some a in pr.reviews a.state == "APPROVED" + a.author_type == "user" + a.has_write_access == true is_resolved_username(a.username) + is_number(a.timestamp) is_string(pr.head_sha) pr.head_sha != "" a.commit_sha == pr.head_sha + not withdrawn(a, pr) +} + +# The same reviewer requested changes or had a review dismissed at the same +# time or later. A review with no usable time counts as later. +withdrawn(approval, pr) if { + some r in pr.reviews + r.username == approval.username + r.state in {"CHANGES_REQUESTED", "DISMISSED"} + not earlier(r, approval) +} + +earlier(r, approval) if { + is_number(r.timestamp) + r.timestamp < approval.timestamp } # The PR URL is https://///pull/. diff --git a/bin/never_alone/four-eyes-policy_test.rego b/bin/never_alone/four-eyes-policy_test.rego index 263772034..cb5785a32 100644 --- a/bin/never_alone/four-eyes-policy_test.rego +++ b/bin/never_alone/four-eyes-policy_test.rego @@ -22,15 +22,21 @@ signed_commit(sha, ts, author, signer) := { "verified": true, "signer_username": signer, "signed_by_platform": false, } -approval(user, sha) := {"username": user, "state": "APPROVED", "timestamp": 1000050, "commit_sha": sha} +# A submitted review by a person with write access. +review(user, sha, state, ts) := { + "username": user, "state": state, "timestamp": ts, "commit_sha": sha, + "author_type": "user", "has_write_access": true, +} + +approval(user, sha) := review(user, sha, "APPROVED", 1000050) -pr(commits, approvers, head) := { +pr(commits, reviews, head) := { "url": "https://github.com/o/r/pull/1", "author": "alice", "merge_commit": merge, "head_sha": head, "commits": commits, - "approvers": approvers, + "reviews": reviews, } trail(name, prs) := { @@ -192,7 +198,7 @@ test_ghost_signer_blocks_approval if { } test_commit_signed_by_platform_passes if { - allowed([pr([object.union(object.remove(commit(first, 1000000, "alice"), ["signer_username"]), {"signed_by_platform": true})], [approval("bob", first)], first)]) + allowed([pr([object.union(commit(first, 1000000, "alice"), {"signer_username": "web-flow", "signed_by_platform": true})], [approval("bob", first)], first)]) } # Naming someone else as the author doesn't let the signer approve their own work. @@ -213,7 +219,7 @@ test_unsigned_commit_violation_says_why if { } test_invalid_github_signature_blocks_approval if { - not allowed([pr([object.union(object.remove(commit(first, 1000000, "alice"), ["signer_username"]), {"signed_by_platform": true, "verified": false})], [approval("bob", first)], first)]) + not allowed([pr([object.union(commit(first, 1000000, "alice"), {"signer_username": "web-flow", "signed_by_platform": true, "verified": false})], [approval("bob", first)], first)]) } test_non_merge_trail_with_old_approval_fails if { @@ -234,3 +240,73 @@ test_dismissed_review_on_head_fails_on_non_merge_trail if { not policy.allow with input as {"trails": [trail(later, [pr(one_commit, [object.union(approval("bob", first), {"state": "DISMISSED"})], first)])]} with data.params as params } + +test_bot_approval_does_not_count if { + not allowed([pr(one_commit, [object.union(approval("bob", first), {"author_type": "bot"})], first)]) +} + +test_approval_from_other_actor_type_does_not_count if { + not allowed([pr(one_commit, [object.union(approval("bob", first), {"author_type": "other"})], first)]) +} + +test_approval_without_write_access_does_not_count if { + not allowed([pr(one_commit, [object.union(approval("bob", first), {"has_write_access": false})], first)]) +} + +test_approval_with_unknown_write_access_does_not_count if { + not allowed([pr(one_commit, [object.remove(approval("bob", first), ["has_write_access"])], first)]) +} + +test_later_request_for_changes_withdraws_approval if { + not allowed([pr(one_commit, [approval("bob", first), review("bob", first, "CHANGES_REQUESTED", 1000060)], first)]) +} + +test_same_second_request_for_changes_withdraws_approval if { + not allowed([pr(one_commit, [approval("bob", first), review("bob", first, "CHANGES_REQUESTED", 1000050)], first)]) +} + +test_later_dismissal_withdraws_approval if { + not allowed([pr(one_commit, [approval("bob", first), review("bob", first, "DISMISSED", 1000060)], first)]) +} + +test_request_for_changes_without_time_withdraws_approval if { + not allowed([pr(one_commit, [approval("bob", first), object.remove(review("bob", first, "CHANGES_REQUESTED", 0), ["timestamp"])], first)]) +} + +test_earlier_request_for_changes_does_not_withdraw if { + allowed([pr(one_commit, [review("bob", first, "CHANGES_REQUESTED", 1000040), approval("bob", first)], first)]) +} + +test_later_comment_does_not_withdraw if { + allowed([pr(one_commit, [approval("bob", first), review("bob", first, "COMMENTED", 1000060)], first)]) +} + +test_another_reviewers_request_for_changes_does_not_withdraw if { + allowed([pr(one_commit, [approval("bob", first), review("carol", first, "CHANGES_REQUESTED", 1000060)], first)]) +} + +test_approval_without_time_does_not_count if { + not allowed([pr(one_commit, [object.remove(approval("bob", first), ["timestamp"])], first)]) +} + +# Attestations from a CLI that records approvers only, with no reviews. +test_pr_without_reviews_fails if { + not allowed([object.union(object.remove(pr(one_commit, [], first), ["reviews"]), {"approvers": [approval("bob", first)]})]) +} + +test_request_for_changes_with_null_time_withdraws_approval if { + not allowed([pr(one_commit, [approval("bob", first), object.union(review("bob", first, "CHANGES_REQUESTED", 0), {"timestamp": null})], first)]) +} + +test_comment_alone_is_not_an_approval if { + not allowed([pr(one_commit, [review("bob", first, "COMMENTED", 1000050)], first)]) +} + +# A commit GitHub signed names web-flow as signer; the named author still needs an independent approval. +test_platform_signed_commit_author_cannot_self_approve if { + not allowed([pr([object.union(commit(first, 1000000, "alice"), {"signer_username": "web-flow", "signed_by_platform": true})], [approval("alice", first)], first)]) +} + +test_platform_signed_commit_without_signer_passes if { + allowed([pr([object.union(object.remove(commit(first, 1000000, "alice"), ["signer_username"]), {"signed_by_platform": true})], [approval("bob", first)], first)]) +} diff --git a/internal/github/build_pr_evidence_test.go b/internal/github/build_pr_evidence_test.go index 47040da7e..8c1c0efdb 100644 --- a/internal/github/build_pr_evidence_test.go +++ b/internal/github/build_pr_evidence_test.go @@ -206,7 +206,7 @@ func TestBuildPREvidence_UnsignedCommitHasNoSignatureFields(t *testing.T) { // An approval whose commit GitHub no longer has must leave commit_sha out of // the payload rather than send an empty string, and so must an unknown head. func TestBuildPREvidence_OmitsUnknownReviewedAndHeadCommits(t *testing.T) { - review := reviewNode("User", "grace", "APPROVED") + review := reviewNode("User", "grace", "APPROVED", true) evidence, err := buildPREvidence( "https://github.com/kosli-dev/cli/pull/671", @@ -258,8 +258,8 @@ func TestBuildPREvidence_RecordsCommitSigner(t *testing.T) { require.NotNil(t, evidence.Commits[0].SignedByPlatform) require.False(t, *evidence.Commits[0].SignedByPlatform) - require.Equal(t, "", evidence.Commits[1].SignerUsername, - "GitHub's own signing account is not who made the commit") + require.Equal(t, "web-flow", evidence.Commits[1].SignerUsername, + "the signer is recorded as GitHub reports it, even when GitHub signed") require.NotNil(t, evidence.Commits[1].SignedByPlatform) require.True(t, *evidence.Commits[1].SignedByPlatform) @@ -269,28 +269,71 @@ func TestBuildPREvidence_RecordsCommitSigner(t *testing.T) { require.NotContains(t, string(unsigned), "signed_by_platform") } -func reviewNode(typename, login, state string) graphqlReviewNode { - r := graphqlReviewNode{State: graphql.String(state), SubmittedAt: "2026-03-01T13:00:00Z"} +func reviewNode(typename, login, state string, canPush bool) graphqlReviewNode { + r := graphqlReviewNode{State: graphql.String(state), SubmittedAt: "2026-03-01T13:00:00Z", + AuthorCanPushToRepository: graphql.Boolean(canPush)} r.Author.Typename = graphql.String(typename) r.Author.Login = graphql.String(login) return r } -// Only a person's approval counts: a bot's does not, and nor does a later -// request for changes, which GitHub returns as that reviewer's latest review. -func TestBuildPREvidence_KeepsOnlyHumanApprovals(t *testing.T) { +// Every review is recorded with who wrote it and their access; deciding which +// ones count is the policy's job. Approvers keeps only the approvals. +func TestBuildPREvidence_RecordsEveryReviewAsFacts(t *testing.T) { + approved := reviewNode("User", "grace", "APPROVED", true) + approved.Commit = &struct{ Oid graphql.String }{Oid: "0e723254516c841126e81f76100be57258ff1386"} evidence, err := buildPREvidence( "https://github.com/kosli-dev/cli/pull/671", "0e723254516c841126e81f76100be57258ff1386", "MERGED", "tooky", "2026-03-01T09:00:00Z", "", "title", "feature", "main", "", nil, []graphqlReviewNode{ - reviewNode("User", "grace", "APPROVED"), - reviewNode("Bot", "github-actions", "APPROVED"), - reviewNode("User", "linus", "CHANGES_REQUESTED"), + approved, + reviewNode("Bot", "github-actions", "APPROVED", true), + reviewNode("User", "linus", "CHANGES_REQUESTED", false), + reviewNode("Mannequin", "ghost-import", "COMMENTED", false), }, ) require.NoError(t, err) - require.Len(t, evidence.Approvers, 1) - require.Equal(t, "grace", evidence.Approvers[0].(types.PRApprovals).Username) + + type fact struct { + user, state, authorType string + canPush bool + } + got := []fact{} + for _, r := range *evidence.Reviews { + require.NotNil(t, r.HasWriteAccess) + got = append(got, fact{r.Username, r.State, r.AuthorType, *r.HasWriteAccess}) + } + require.Equal(t, []fact{ + {"grace", "APPROVED", "user", true}, + {"github-actions", "APPROVED", "bot", true}, + {"linus", "CHANGES_REQUESTED", "user", false}, + {"ghost-import", "COMMENTED", "other", false}, + }, got) + + require.Equal(t, []any{ + types.PRApprovals{Username: "grace", State: "APPROVED", Timestamp: 1772370000}, + types.PRApprovals{Username: "github-actions", State: "APPROVED", Timestamp: 1772370000}, + }, evidence.Approvers, "approvers keeps every approval, in the shape it had before") +} + +// A PR with no reviews records an empty list, which a policy can tell apart +// from evidence where reviews weren't recorded at all. +func TestBuildPREvidence_RecordsNoReviewsAsEmptyList(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) + payload, err := json.Marshal(evidence) + require.NoError(t, err) + require.Contains(t, string(payload), `"reviews":[]`) + + notRecorded, err := json.Marshal(types.PREvidence{}) + require.NoError(t, err) + require.NotContains(t, string(notRecorded), "reviews") } diff --git a/internal/github/github.go b/internal/github/github.go index 0b0ebeebb..f168146ad 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -282,22 +282,21 @@ type graphqlSignature struct { IsValid graphql.Boolean State graphql.String WasSignedByGitHub graphql.Boolean - // The account behind the signing key, or null when it matches none. For a - // commit GitHub signed this is GitHub's own web-flow account. + // The account behind the signing key, or null when it matches none. Signer *struct { Login graphql.String } } -// graphqlReviewNode is the shared GraphQL node type for each reviewer's latest -// approving or change-requesting review on a PR. +// graphqlReviewNode is the shared GraphQL node type for submitted reviews on a PR. type graphqlReviewNode struct { Author struct { Typename graphql.String `graphql:"__typename"` Login graphql.String } - State graphql.String - SubmittedAt graphql.String + State graphql.String + SubmittedAt graphql.String + AuthorCanPushToRepository graphql.Boolean // Null when GitHub no longer has the commit the review was given on. Commit *struct { Oid graphql.String @@ -368,7 +367,7 @@ func buildPREvidence( verified = &v signatureState = &s signedByPlatform = &g - if sig.Signer != nil && !g { + if sig.Signer != nil { signerUsername = string(sig.Signer.Login) } } @@ -387,29 +386,49 @@ func buildPREvidence( }) } + reviews := []types.PRApprovals{} for _, r := range reviewNodes { - // A bot's approval is not a second person's review. - if r.State != "APPROVED" || r.Author.Typename != "User" { - continue - } submittedAt, err := time.Parse(time.RFC3339, string(r.SubmittedAt)) if err != nil { return nil, err } - approval := types.PRApprovals{ - Username: string(r.Author.Login), - State: string(r.State), - Timestamp: submittedAt.Unix(), + canPush := bool(r.AuthorCanPushToRepository) + review := types.PRApprovals{ + Username: string(r.Author.Login), + State: string(r.State), + Timestamp: submittedAt.Unix(), + AuthorType: reviewAuthorType(string(r.Author.Typename)), + HasWriteAccess: &canPush, } if r.Commit != nil { - approval.CommitSHA = string(r.Commit.Oid) + review.CommitSHA = string(r.Commit.Oid) + } + reviews = append(reviews, review) + if r.State == "APPROVED" { + evidence.Approvers = append(evidence.Approvers, types.PRApprovals{ + Username: review.Username, + State: review.State, + Timestamp: review.Timestamp, + }) } - evidence.Approvers = append(evidence.Approvers, approval) } + evidence.Reviews = &reviews return evidence, nil } +// reviewAuthorType maps GitHub's actor type onto the neutral author_type values. +func reviewAuthorType(typename string) string { + switch typename { + case "User": + return "user" + case "Bot": + return "bot" + default: + return "other" + } +} + // PREvidenceByPRNumber fetches full PR evidence for a single PR number via // GraphQL. Returns an error when the PR does not exist. func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, error) { @@ -446,7 +465,7 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er Reviews struct { Nodes []graphqlReviewNode PageInfo pageInfo - } `graphql:"latestOpinionatedReviews(first: 100, writersOnly: true, after: $reviewCursor)"` + } `graphql:"reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED], after: $reviewCursor)"` } `graphql:"pullRequest(number: $prNumber)"` } `graphql:"repository(owner: $owner, name: $repo)"` } @@ -539,7 +558,7 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence Reviews struct { Nodes []graphqlReviewNode PageInfo pageInfo - } `graphql:"latestOpinionatedReviews(first: 100, writersOnly: true, 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 66b2b0cba..fc672ab84 100644 --- a/internal/github/pagination.go +++ b/internal/github/pagination.go @@ -162,7 +162,7 @@ func (c *GithubConfig) allPRCommits(ctx context.Context, run graphqlQueryFunc, r return commits, nil } -// allPRReviews returns every approved review on prNumber, seeded as above. +// allPRReviews returns every submitted review on prNumber, seeded as above. func (c *GithubConfig) allPRReviews(ctx context.Context, run graphqlQueryFunc, ref prRef, seed []graphqlReviewNode, first pageInfo) ([]graphqlReviewNode, error) { reviews, err := paginate(seed, first, defaultMaxPages, func(after graphql.String) ([]graphqlReviewNode, pageInfo, error) { @@ -172,7 +172,7 @@ func (c *GithubConfig) allPRReviews(ctx context.Context, run graphqlQueryFunc, r Reviews struct { Nodes []graphqlReviewNode PageInfo pageInfo - } `graphql:"latestOpinionatedReviews(first: 100, writersOnly: true, 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 9ea0d4e0f..effa8a9ca 100644 --- a/internal/github/pr_pagination_test.go +++ b/internal/github/pr_pagination_test.go @@ -73,13 +73,13 @@ func commitNodeJSON(sha string) string { // reviewNodeJSON is one node of a GraphQL reviews connection, given on the // commit "reviewed-by-". func reviewNodeJSON(login string) string { - return fmt.Sprintf(`{"author":{"__typename":"User","login":%q},"state":"APPROVED","submittedAt":"2026-03-01T13:00:00Z",`+ + return fmt.Sprintf(`{"author":{"__typename":"User","login":%q},"state":"APPROVED","submittedAt":"2026-03-01T13:00:00Z","authorCanPushToRepository":true,`+ `"commit":{"oid":"reviewed-by-%s"}}`, login, login) } // reviewNodeWithoutCommitJSON is a review whose commit GitHub no longer has. func reviewNodeWithoutCommitJSON(login string) string { - return fmt.Sprintf(`{"author":{"__typename":"User","login":%q},"state":"APPROVED","submittedAt":"2026-03-01T13:00:00Z",`+ + return fmt.Sprintf(`{"author":{"__typename":"User","login":%q},"state":"APPROVED","submittedAt":"2026-03-01T13:00:00Z","authorCanPushToRepository":true,`+ `"commit":null}`, login) } @@ -96,7 +96,7 @@ func prJSON(commits, reviews string) string { return fmt.Sprintf(`{"title":"A PR","state":"MERGED","headRefName":"feature","headRefOid":"head-sha","baseRefName":"main",`+ `"url":"https://github.com/o/r/pull/1","createdAt":"2026-03-01T11:00:00Z",`+ `"mergedAt":"2026-03-01T14:00:00Z","mergeCommit":{"oid":"merge-sha"},`+ - `"author":{"login":"ada"},"commits":%s,"latestOpinionatedReviews":%s}`, commits, reviews) + `"author":{"login":"ada"},"commits":%s,"reviews":%s}`, commits, reviews) } func byPRNumberResponse(commits, reviews string) string { @@ -111,7 +111,7 @@ func commitsPageResponse(nodes []string, nextCursor string) string { // reviewsPageResponse is a follow-up page reply selecting only reviews. func reviewsPageResponse(nodes []string, nextCursor string) string { - return fmt.Sprintf(`{"data":{"repository":{"pullRequest":{"latestOpinionatedReviews":%s}}}}`, + return fmt.Sprintf(`{"data":{"repository":{"pullRequest":{"reviews":%s}}}}`, connectionJSON(nodes, nextCursor)) } @@ -139,7 +139,7 @@ func TestPREvidenceByPRNumber_FollowsCommitPages(t *testing.T) { require.Equal(t, []string{"sha1", "sha2", "sha3"}, shasOf(t, newPaginationConfig(ts), 1)) require.Len(t, ts.bodies, 2) require.Contains(t, ts.bodies[1], "c1", "follow-up must carry the cursor") - require.NotContains(t, ts.bodies[1], "latestOpinionatedReviews(", "follow-up must not re-fetch reviews") + require.NotContains(t, ts.bodies[1], "reviews(", "follow-up must not re-fetch reviews") } func TestPREvidenceByPRNumber_FollowsReviewPages(t *testing.T) { @@ -209,7 +209,7 @@ func v2PRNodeInRepoJSON(owner, repo string, number int, commits, reviews string) `"title":"A PR","state":"MERGED","headRefName":"feature","headRefOid":"head-sha-%d",`+ `"baseRefName":"main","url":"https://github.com/%s/%s/pull/%d",`+ `"createdAt":"2026-03-01T11:00:00Z","mergedAt":"2026-03-01T14:00:00Z",`+ - `"author":{"login":"ada"},"commits":%s,"latestOpinionatedReviews":%s}`, + `"author":{"login":"ada"},"commits":%s,"reviews":%s}`, number, repo, owner, number, owner, repo, number, commits, reviews) } @@ -355,13 +355,13 @@ 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") } -func approverCommitSHAs(evidence *types.PREvidence) []string { +func reviewCommitSHAs(evidence *types.PREvidence) []string { shas := []string{} - for _, a := range evidence.Approvers { - shas = append(shas, a.(types.PRApprovals).CommitSHA) + for _, r := range *evidence.Reviews { + shas = append(shas, r.CommitSHA) } return shas } @@ -378,15 +378,16 @@ func TestPREvidenceByPRNumber_RecordsHeadAndReviewedCommits(t *testing.T) { evidence, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) require.NoError(t, err) require.Equal(t, "head-sha", evidence.HeadSHA) - require.Equal(t, []string{"reviewed-by-ada", "reviewed-by-grace", ""}, approverCommitSHAs(evidence), + require.Equal(t, []string{"reviewed-by-ada", "reviewed-by-grace", ""}, reviewCommitSHAs(evidence), "each approval keeps its own commit, on later review pages too; a missing commit stays empty") // The fake replies whatever is asked, so the field names GitHub must see are checked in the query. require.Contains(t, ts.bodies[0], "headRefOid") require.Contains(t, ts.bodies[0], "commit{oid}") require.Contains(t, ts.bodies[1], "commit{oid}") for _, body := range ts.bodies { - require.Contains(t, body, "latestOpinionatedReviews(first: 100, writersOnly: true") + require.Contains(t, body, "reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED]") require.Contains(t, body, "author{__typename,login}") + require.Contains(t, body, "authorCanPushToRepository") } } @@ -405,7 +406,7 @@ func TestPREvidenceByPRNumber_RecordsCommitSigners(t *testing.T) { require.Len(t, evidence.Commits, 3) require.Equal(t, "ada", evidence.Commits[0].SignerUsername) require.False(t, *evidence.Commits[0].SignedByPlatform) - require.Equal(t, "", evidence.Commits[1].SignerUsername) + require.Equal(t, "web-flow", evidence.Commits[1].SignerUsername) require.True(t, *evidence.Commits[1].SignedByPlatform) require.Nil(t, evidence.Commits[2].SignedByPlatform, "an unsigned commit records no signature facts") require.Contains(t, ts.bodies[0], "signer{login}") @@ -423,9 +424,9 @@ func TestPREvidenceForCommitV2_RecordsHeadAndReviewedCommits(t *testing.T) { require.NoError(t, err) require.Len(t, prs, 1) require.Equal(t, "head-sha-7", prs[0].HeadSHA) - require.Equal(t, []string{"reviewed-by-ada"}, approverCommitSHAs(prs[0])) + require.Equal(t, []string{"reviewed-by-ada"}, reviewCommitSHAs(prs[0])) require.Contains(t, ts.bodies[0], "headRefOid") require.Contains(t, ts.bodies[0], "commit{oid}") - require.Contains(t, ts.bodies[0], "latestOpinionatedReviews(first: 100, writersOnly: true") + require.Contains(t, ts.bodies[0], "reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED]") require.Contains(t, ts.bodies[0], "signer{login}") } diff --git a/internal/types/types.go b/internal/types/types.go index 61a0cd13d..862b3f5f8 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -3,18 +3,21 @@ package types import "encoding/json" type PREvidence struct { - MergeCommit string `json:"merge_commit"` - URL string `json:"url"` - State string `json:"state"` - Approvers []any `json:"approvers"` - Author string `json:"author"` - CreatedAt int64 `json:"created_at,omitempty"` - MergedAt int64 `json:"merged_at,omitempty"` - Title string `json:"title,omitempty"` - HeadRef string `json:"head_ref,omitempty"` - HeadSHA string `json:"head_sha,omitempty"` - BaseRef string `json:"base_ref,omitempty"` - Commits []Commit `json:"commits"` + MergeCommit string `json:"merge_commit"` + URL string `json:"url"` + State string `json:"state"` + Approvers []any `json:"approvers"` + // Every submitted review, in any state; Approvers holds the approvals only. + // Nil when the provider's reviews are not recorded, so "none" stays distinct. + Reviews *[]PRApprovals `json:"reviews,omitempty"` + Author string `json:"author"` + CreatedAt int64 `json:"created_at,omitempty"` + MergedAt int64 `json:"merged_at,omitempty"` + Title string `json:"title,omitempty"` + HeadRef string `json:"head_ref,omitempty"` + HeadSHA string `json:"head_sha,omitempty"` + BaseRef string `json:"base_ref,omitempty"` + Commits []Commit `json:"commits"` } // MarshalJSON keeps "commits" in the payload even when a provider returns no @@ -35,6 +38,9 @@ type PRApprovals struct { State string `json:"state,omitempty"` Timestamp int64 `json:"timestamp,omitempty"` CommitSHA string `json:"commit_sha,omitempty"` + // "user", "bot" or "other" + AuthorType string `json:"author_type,omitempty"` + HasWriteAccess *bool `json:"has_write_access,omitempty"` } type Commit struct { From 5b1d0d7ef4fe02894eb3146d96faa40df045c71f Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Tue, 6 Oct 2026 23:27:44 +0100 Subject: [PATCH 6/7] feat(github): record the merge commit, commit total and co-authors merge_commit was set to whatever commit was asked about, not the PR's merge commit. GitHub also links a PR to the commits on its branch, so a branch commit was recorded as the merge commit. It now records GitHub's mergeCommit. An unmerged PR has none, so the field is left out: the server rejects an empty merge_commit on a PR that records reviews. GitHub lists at most 250 commits for a PR. On a 260-commit PR it returned 250 while its own total said 260, so the oldest commits' authors were never seen. commit_count records GitHub's total. An AI agent such as Copilot names the person who asked for it in a Co-authored-by trailer, which GitHub resolves to an account. co_author_usernames records those accounts, up to 99 per commit. Each PR the query asks for adds every commit's authors list to its cost and size. At 100 PRs GitHub rejects the query for exceeding its 500,000-node limit; at 10 it costs 10 points, against 2 before. On the default branch GitHub returns only the PR that merged the commit. Co-Authored-By: Claude Opus 5.5 --- internal/github/build_pr_evidence_test.go | 31 +++++++++ internal/github/github.go | 81 +++++++++++++++-------- internal/github/github_contract_test.go | 8 +-- internal/github/pr_pagination_test.go | 37 +++++++++++ internal/types/types.go | 12 ++++ internal/types/types_test.go | 30 +++++++++ 6 files changed, 169 insertions(+), 30 deletions(-) diff --git a/internal/github/build_pr_evidence_test.go b/internal/github/build_pr_evidence_test.go index 8c1c0efdb..c2e0cff40 100644 --- a/internal/github/build_pr_evidence_test.go +++ b/internal/github/build_pr_evidence_test.go @@ -337,3 +337,34 @@ func TestBuildPREvidence_RecordsNoReviewsAsEmptyList(t *testing.T) { require.NoError(t, err) require.NotContains(t, string(notRecorded), "reviews") } + +// GitHub lists the git author first, then one entry per Co-authored-by trailer; +// a trailer it can't match to an account has no user. +func TestBuildPREvidence_RecordsCoAuthors(t *testing.T) { + type author = struct { + User *struct { + Login graphql.String + } + } + login := func(l string) author { + return author{User: &struct{ Login graphql.String }{Login: graphql.String(l)}} + } + node := graphqlCommitNode{} + node.Commit.Oid = "6a1c03b5d2f84f9e1f0c1b7c0e5d8d1a2b3c4d5e" + node.Commit.CommittedDate = "2026-03-01T12:00:00Z" + node.Commit.Authors.Nodes = []author{login("Copilot"), login("alice"), {}, login("bob")} + plain := graphqlCommitNode{} + plain.Commit.Oid = "0b9e2a7f4c3d1e5a6b8c9d0e1f2a3b4c5d6e7f80" + plain.Commit.CommittedDate = "2026-03-01T12:00:00Z" + plain.Commit.Authors.Nodes = []author{login("carol")} + + evidence, err := buildPREvidence( + "https://github.com/o/r/pull/1", "", "MERGED", "Copilot", + "2026-03-01T11:00:00Z", "", "t", "feature", "main", "", + []graphqlCommitNode{node, plain}, nil, + ) + require.NoError(t, err) + require.Equal(t, []string{"alice", "bob"}, evidence.Commits[0].CoAuthorUsernames, + "the git author is not a co-author, and an unmatched trailer has no account to record") + require.Nil(t, evidence.Commits[1].CoAuthorUsernames) +} diff --git a/internal/github/github.go b/internal/github/github.go index f168146ad..be942ca3b 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -275,6 +275,14 @@ type graphqlCommitNode struct { } } Signature *graphqlSignature + // GitHub lists the git author first, then each Co-authored-by trailer. + Authors struct { + Nodes []struct { + User *struct { + Login graphql.String + } + } + } `graphql:"authors(first: 100)"` } } @@ -304,8 +312,7 @@ type graphqlReviewNode struct { } // buildPREvidence constructs a PREvidence from pre-resolved fields and the -// raw GraphQL commit/review nodes. mergeCommit must be resolved by the caller -// (it differs between commit-SHA queries and PR-number queries). +// raw GraphQL commit/review nodes. func buildPREvidence( url, mergeCommit, state, author, createdAtStr, mergedAtStr, title, headRef, baseRef, headSHA string, commitNodes []graphqlCommitNode, @@ -371,18 +378,25 @@ func buildPREvidence( signerUsername = string(sig.Signer.Login) } } + var coAuthors []string + for i, a := range n.Commit.Authors.Nodes { + if i > 0 && a.User != nil { + coAuthors = append(coAuthors, string(a.User.Login)) + } + } evidence.Commits = append(evidence.Commits, types.Commit{ - SHA: string(n.Commit.Oid), - Message: string(n.Commit.MessageHeadline), - Author: fmt.Sprintf("%s <%s>", string(n.Commit.Author.Name), string(n.Commit.Author.Email)), - AuthorUsername: authorUsername, - Timestamp: timestamp.Unix(), - Branch: headRef, - URL: string(n.Commit.URL), - Verified: verified, - SignatureState: signatureState, - SignerUsername: signerUsername, - SignedByPlatform: signedByPlatform, + SHA: string(n.Commit.Oid), + Message: string(n.Commit.MessageHeadline), + Author: fmt.Sprintf("%s <%s>", string(n.Commit.Author.Name), string(n.Commit.Author.Email)), + AuthorUsername: authorUsername, + Timestamp: timestamp.Unix(), + Branch: headRef, + URL: string(n.Commit.URL), + Verified: verified, + SignatureState: signatureState, + SignerUsername: signerUsername, + SignedByPlatform: signedByPlatform, + CoAuthorUsernames: coAuthors, }) } @@ -459,8 +473,9 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er Login graphql.String } Commits struct { - Nodes []graphqlCommitNode - PageInfo pageInfo + TotalCount graphql.Int + Nodes []graphqlCommitNode + PageInfo pageInfo } `graphql:"commits(first: 100, after: $commitCursor)"` Reviews struct { Nodes []graphqlReviewNode @@ -503,11 +518,17 @@ func (c *GithubConfig) PREvidenceByPRNumber(prNumber int) (*types.PREvidence, er return nil, err } - return buildPREvidence( + evidence, err := buildPREvidence( string(pr.URL), mergeCommit, string(pr.State), string(pr.Author.Login), string(pr.CreatedAt), string(pr.MergedAt), string(pr.Title), string(pr.HeadRefName), string(pr.BaseRefName), string(pr.HeadRefOid), commits, reviews, ) + if err != nil { + return nil, err + } + commitCount := int(pr.Commits.TotalCount) + evidence.CommitCount = &commitCount + return evidence, nil } func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence, error) { @@ -545,14 +566,18 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence URL graphql.String CreatedAt graphql.String MergedAt graphql.String + MergeCommit *struct { + Oid graphql.String + } Author struct { Login graphql.String } Commits struct { - Nodes []graphqlCommitNode - PageInfo pageInfo + TotalCount graphql.Int + Nodes []graphqlCommitNode + PageInfo pageInfo } `graphql:"commits(first: 100, after: $commitCursor)"` Reviews struct { @@ -560,11 +585,11 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence PageInfo pageInfo } `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 - // case, and draining it would mean nesting a page walk per - // PR page (#1082). - } `graphql:"associatedPullRequests(first: 100)"` + // Not paginated: on the default branch GitHub returns only the PR + // that merged the commit. Each PR asked for adds every commit's + // authors list to the query's cost and node count, so 10 keeps + // the cost at 10 points. + } `graphql:"associatedPullRequests(first: 10)"` } `graphql:"... on Commit"` } `graphql:"object(oid: $commitSHA)"` } `graphql:"repository(owner: $owner, name: $repo)"` @@ -607,16 +632,20 @@ func (c *GithubConfig) PREvidenceForCommitV2(commit string) ([]*types.PREvidence if err != nil { return pullRequestsEvidence, err } - // MergeCommit is set to the queried commit SHA — V2 queries by commit SHA - // so the commit is by definition the merge commit. + mergeCommit := "" + if pr.MergeCommit != nil { + mergeCommit = string(pr.MergeCommit.Oid) + } evidence, err := buildPREvidence( - string(pr.URL), commit, string(pr.State), string(pr.Author.Login), + string(pr.URL), mergeCommit, string(pr.State), string(pr.Author.Login), string(pr.CreatedAt), string(pr.MergedAt), string(pr.Title), string(pr.HeadRefName), string(pr.BaseRefName), string(pr.HeadRefOid), commits, reviews, ) if err != nil { return pullRequestsEvidence, err } + commitCount := int(pr.Commits.TotalCount) + evidence.CommitCount = &commitCount pullRequestsEvidence = append(pullRequestsEvidence, evidence) } return pullRequestsEvidence, nil diff --git a/internal/github/github_contract_test.go b/internal/github/github_contract_test.go index 0169229f9..23c3848b9 100644 --- a/internal/github/github_contract_test.go +++ b/internal/github/github_contract_test.go @@ -10,9 +10,9 @@ import ( ) // runGitHubContractTests exercises the types.PRRetriever contract against any -// implementation. commitWithPR must be a commit SHA that has at least one -// associated pull request. commitUnknown must be a validly-formatted SHA that -// does not exist in the repository. +// implementation. commitWithPR must be the merge commit of a pull request. +// commitUnknown must be a validly-formatted SHA that does not exist in the +// repository. // // V1 and V2 have different contracts for unknown commits: // - V2 (GraphQL) returns empty with no error — the GraphQL API returns null @@ -30,7 +30,7 @@ func runGitHubContractTests(t *testing.T, provider types.PRRetriever, commitWith require.NotEmpty(t, prs) require.NotEmpty(t, prs[0].URL, "URL should be present") require.NotEmpty(t, prs[0].State, "State should be present") - require.Equal(t, commitWithPR, prs[0].MergeCommit, "V2 sets MergeCommit to the queried commit SHA") + require.Equal(t, commitWithPR, prs[0].MergeCommit, "MergeCommit is the PR's merge commit") }) t.Run("V2 returns empty with no error for unknown commit", func(t *testing.T) { diff --git a/internal/github/pr_pagination_test.go b/internal/github/pr_pagination_test.go index effa8a9ca..0364c67ea 100644 --- a/internal/github/pr_pagination_test.go +++ b/internal/github/pr_pagination_test.go @@ -430,3 +430,40 @@ func TestPREvidenceForCommitV2_RecordsHeadAndReviewedCommits(t *testing.T) { require.Contains(t, ts.bodies[0], "reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED]") require.Contains(t, ts.bodies[0], "signer{login}") } + +func withCommitTotal(connection string, total int) string { + return strings.Replace(connection, `{"nodes":`, fmt.Sprintf(`{"totalCount":%d,"nodes":`, total), 1) +} + +func TestPREvidenceForCommitV2_RecordsGitHubsMergeCommitAndCommitTotal(t *testing.T) { + merged := strings.Replace( + v2PRNodeJSON(7, withCommitTotal(connectionJSON([]string{commitNodeJSON("sha1")}, ""), 260), connectionJSON(nil, "")), + `"author":`, `"mergeCommit":{"oid":"real-merge-sha"},"author":`, 1) + open := v2PRNodeJSON(9, withCommitTotal(connectionJSON(nil, ""), 0), connectionJSON(nil, "")) + ts := newGraphQLTestServer(t, forCommitResponse(merged, open)) + + prs, err := newPaginationConfig(ts).PREvidenceForCommitV2("queried-sha") + require.NoError(t, err) + require.Len(t, prs, 2) + require.Equal(t, "real-merge-sha", prs[0].MergeCommit, "the merge commit is GitHub's, not the commit asked about") + require.Equal(t, 260, *prs[0].CommitCount) + require.Equal(t, "", prs[1].MergeCommit, "an unmerged PR has no merge commit") + require.Equal(t, 0, *prs[1].CommitCount) + require.Contains(t, ts.bodies[0], "mergeCommit{oid}") + require.Contains(t, ts.bodies[0], "associatedPullRequests(first: 10)") + require.Contains(t, ts.bodies[0], "totalCount") + require.Contains(t, ts.bodies[0], "authors(first: 100){nodes{user{login}}}") +} + +func TestPREvidenceByPRNumber_RecordsCommitTotal(t *testing.T) { + ts := newGraphQLTestServer(t, byPRNumberResponse( + withCommitTotal(connectionJSON([]string{commitNodeJSON("sha1")}, ""), 260), + connectionJSON(nil, ""), + )) + + evidence, err := newPaginationConfig(ts).PREvidenceByPRNumber(1) + require.NoError(t, err) + require.Equal(t, 260, *evidence.CommitCount) + require.Contains(t, ts.bodies[0], "totalCount") + require.Contains(t, ts.bodies[0], "authors(first: 100){nodes{user{login}}}") +} diff --git a/internal/types/types.go b/internal/types/types.go index 862b3f5f8..cc9a9e419 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -18,6 +18,8 @@ type PREvidence struct { HeadSHA string `json:"head_sha,omitempty"` BaseRef string `json:"base_ref,omitempty"` Commits []Commit `json:"commits"` + // The provider's total, which exceeds len(Commits) when it caps the list. + CommitCount *int `json:"commit_count,omitempty"` } // MarshalJSON keeps "commits" in the payload even when a provider returns no @@ -30,6 +32,14 @@ func (e PREvidence) MarshalJSON() ([]byte, error) { if out.Commits == nil { out.Commits = []Commit{} } + // The server accepts an empty merge_commit only in the older shape, which has + // no reviews; in this shape it rejects the whole attestation. + if out.Reviews != nil && out.MergeCommit == "" { + return json.Marshal(struct { + prEvidence + MergeCommit string `json:"merge_commit,omitempty"` + }{prEvidence: out}) + } return json.Marshal(out) } @@ -55,6 +65,8 @@ type Commit struct { SignatureState *string `json:"signature_state,omitempty"` SignerUsername string `json:"signer_username,omitempty"` SignedByPlatform *bool `json:"signed_by_platform,omitempty"` + // Accounts named in Co-authored-by trailers, as the provider resolves them. + CoAuthorUsernames []string `json:"co_author_usernames,omitempty"` } type PRRetriever interface { diff --git a/internal/types/types_test.go b/internal/types/types_test.go index 9767f40e8..2d570cc4a 100644 --- a/internal/types/types_test.go +++ b/internal/types/types_test.go @@ -40,3 +40,33 @@ func TestPREvidenceAlwaysSerialisesCommits(t *testing.T) { }) } } + +// The server rejects an empty merge_commit on a PR that records reviews, so an +// open PR (no merge commit yet) must leave the field out rather than send "". +func TestPREvidenceMergeCommitWhenEmpty(t *testing.T) { + for _, tc := range []struct { + name string + evidence PREvidence + want *string + }{ + {"recorded reviews, no merge commit: omitted", PREvidence{Reviews: &[]PRApprovals{}}, nil}, + {"recorded reviews, merge commit: kept", PREvidence{Reviews: &[]PRApprovals{}, MergeCommit: "abc"}, ptr("abc")}, + {"no reviews recorded, no merge commit: kept empty", PREvidence{}, ptr("")}, + } { + t.Run(tc.name, func(t *testing.T) { + payload, err := json.Marshal(tc.evidence) + require.NoError(t, err) + var decoded map[string]any + require.NoError(t, json.Unmarshal(payload, &decoded)) + got, present := decoded["merge_commit"] + if tc.want == nil { + require.False(t, present, "merge_commit must be left out: %s", payload) + return + } + require.Equal(t, *tc.want, got) + require.Contains(t, decoded, "commits", "the other fields are still serialised") + }) + } +} + +func ptr(s string) *string { return &s } From cc719b5941042a8767b51de3c061e1d1cb587888 Mon Sep 17 00:00:00 2001 From: Alex Kantor Date: Tue, 6 Oct 2026 23:28:01 +0100 Subject: [PATCH 7/7] fix(never_alone): count co-authors, require every commit listed, ignore late approvals The four-eyes policy now: - counts co-authors as authors, and requires each to be a linked account; - fails a PR listing a different number of commits than its total, or with no total, which an older CLI doesn't send; - ignores approvals given after the merge, and requires merged_at to be a number, since Rego treats any number as smaller than a string; - requires the PR author to be a linked account on a commit that isn't the merge commit. A null, empty or "ghost" author made the rule that the PR author can't approve vanish. Co-Authored-By: Claude Opus 5.5 --- bin/never_alone/four-eyes-policy.rego | 50 ++++++++++++- bin/never_alone/four-eyes-policy_test.rego | 86 +++++++++++++++++++++- 2 files changed, 133 insertions(+), 3 deletions(-) diff --git a/bin/never_alone/four-eyes-policy.rego b/bin/never_alone/four-eyes-policy.rego index ee6cf01ad..ca2ee5521 100644 --- a/bin/never_alone/four-eyes-policy.rego +++ b/bin/never_alone/four-eyes-policy.rego @@ -42,6 +42,7 @@ trail_compliant(trail) if { attest := pr_attest(trail) some pr in attest.pull_requests pr_in_repo(pr) + all_commits_listed(pr) all_authors_resolved(pr) has_independent_approval(trail, pr) } @@ -71,11 +72,15 @@ is_resolved_username(u) if { u != "ghost" } -# GitHub usernames on PR branch commits: each named author and signing account. +# GitHub usernames on PR branch commits: each named author, signing account and co-author. pr_commit_authors(pr) := {u | some c in pr.commits some u in [object.get(c, "author_username", null), object.get(c, "signer_username", null)] is_resolved_username(u) +} | {u | + some c in pr.commits + some u in object.get(c, "co_author_usernames", []) + is_resolved_username(u) } # Usernames of people with write access whose approval was given on the PR's @@ -91,6 +96,7 @@ approvers_on_head(pr) := {a.username | is_string(pr.head_sha) pr.head_sha != "" a.commit_sha == pr.head_sha + given_before_merge(a, pr) not withdrawn(a, pr) } @@ -103,6 +109,12 @@ withdrawn(approval, pr) if { not earlier(r, approval) } +# An approval after merge means the code reached main unreviewed. +given_before_merge(approval, pr) if { + is_number(pr.merged_at) + approval.timestamp <= pr.merged_at +} + earlier(r, approval) if { is_number(r.timestamp) r.timestamp < approval.timestamp @@ -116,11 +128,20 @@ pr_in_repo(pr) if { lower(concat("/", [parts[3], parts[4]])) == repository } +# The PR lists every commit GitHub counts. GitHub returns at most 250, so on a +# longer PR the oldest commits' authors would go unchecked. +all_commits_listed(pr) if { + count(pr.commits) == pr.commit_count +} + # Every commit on the PR has an author linked to a GitHub account and a # verified signature, by a known account or by GitHub. Without the signature # the author is only what the commit says, which its writer chooses. all_authors_resolved(pr) if { every c in pr.commits { + every u in object.get(c, "co_author_usernames", []) { + is_resolved_username(u) + } is_resolved_username(object.get(c, "author_username", null)) signed_by_known_identity(c) } @@ -145,6 +166,7 @@ is_merge_commit(trail, pr) if { # Regular commit: PR branch authors + PR author all need independent approval on the final commit. has_independent_approval(trail, pr) if { not is_merge_commit(trail, pr) + is_resolved_username(pr.author) all_authors := pr_commit_authors(pr) | {pr.author} eligible_approvers := approvers_on_head(pr) count(all_authors) > 0 @@ -231,6 +253,29 @@ violations contains msg if { ) } +# Unlisted commits: the PR lists a different number of commits than GitHub counts. +violations contains msg if { + some trail in input.trails + not trail_compliant(trail) + attest := pr_attest(trail) + some pr in attest.pull_requests + is_number(pr.commit_count) + not all_commits_listed(pr) + msg := sprintf( + "PR %v: lists %v commits but GitHub counts %v - some authors can't be checked", + [pr.url, count(pr.commits), pr.commit_count], + ) +} + +violations contains msg if { + some trail in input.trails + not trail_compliant(trail) + attest := pr_attest(trail) + some pr in attest.pull_requests + not is_number(object.get(pr, "commit_count", null)) + msg := sprintf("PR %v: no commit count recorded - re-attest with a current Kosli CLI", [pr.url]) +} + # Missing PR: commit has no associated merged PR. violations contains msg if { some trail in input.trails @@ -249,7 +294,7 @@ violations contains msg if { count(attest.pull_requests) > 0 not any_pr_fully_approved(trail, attest) msg := sprintf( - "Commit %v: no PR in %v has an independent approval on its final commit", + "Commit %v: no PR in %v has an independent approval on its final commit before merge", [trail.name, repository], ) } @@ -260,6 +305,7 @@ violations contains msg if { any_pr_fully_approved(trail, attest) if { some pr in attest.pull_requests pr_in_repo(pr) + all_commits_listed(pr) all_authors_resolved(pr) has_independent_approval(trail, pr) } diff --git a/bin/never_alone/four-eyes-policy_test.rego b/bin/never_alone/four-eyes-policy_test.rego index cb5785a32..c55dcbdbc 100644 --- a/bin/never_alone/four-eyes-policy_test.rego +++ b/bin/never_alone/four-eyes-policy_test.rego @@ -35,10 +35,14 @@ pr(commits, reviews, head) := { "author": "alice", "merge_commit": merge, "head_sha": head, + "merged_at": merged_at, "commits": commits, + "commit_count": count(commits), "reviews": reviews, } +merged_at := 2000000 + trail(name, prs) := { "name": name, "git_commit_info": {"author": "alice ", "sha1": name}, @@ -150,7 +154,7 @@ test_missing_repository_param_fails_and_says_why if { test_unapproved_commit_violation_names_the_repository if { v := policy.violations with input as {"trails": [trail(merge, [pr(backdated_push, [approval("bob", first)], later)])]} with data.params as params - v == {sprintf("Commit %v: no PR in o/r has an independent approval on its final commit", [merge])} + v == {sprintf("Commit %v: no PR in o/r has an independent approval on its final commit before merge", [merge])} } test_null_head_and_reviewed_commit_fails if { @@ -310,3 +314,83 @@ test_platform_signed_commit_author_cannot_self_approve if { test_platform_signed_commit_without_signer_passes if { allowed([pr([object.union(object.remove(commit(first, 1000000, "alice"), ["signer_username"]), {"signed_by_platform": true})], [approval("bob", first)], first)]) } + +test_approval_after_merge_fails if { + not allowed([pr(one_commit, [review("bob", first, "APPROVED", merged_at + 1)], first)]) +} + +test_approval_at_merge_time_passes if { + allowed([pr(one_commit, [review("bob", first, "APPROVED", merged_at)], first)]) +} + +test_unmerged_pr_fails if { + not allowed([object.remove(pr(one_commit, [approval("bob", first)], first), ["merged_at"])]) +} + +test_pr_listing_fewer_commits_than_github_counts_fails if { + p := object.union(pr(one_commit, [approval("bob", first)], first), {"commit_count": 251}) + not allowed([p]) + v := policy.violations with input as {"trails": [trail(merge, [p])]} with data.params as params + some msg in v + contains(msg, "lists 1 commits but GitHub counts 251") +} + +test_pr_without_commit_count_fails if { + not allowed([object.remove(pr(one_commit, [approval("bob", first)], first), ["commit_count"])]) +} + +# An AI agent's commit names the person who asked for it as co-author. +agent_commit := object.union(commit(first, 1000000, "Copilot"), {"signer_username": "web-flow", "signed_by_platform": true, "co_author_usernames": ["alice"]}) + +test_co_author_cannot_self_approve if { + not allowed([pr([agent_commit], [approval("alice", first)], first)]) +} + +test_co_authored_commit_with_independent_approval_passes if { + allowed([pr([agent_commit], [approval("bob", first)], first)]) +} + +test_null_merge_time_and_commit_count_fail if { + not allowed([object.union(pr(one_commit, [approval("bob", first)], first), {"merged_at": null})]) + not allowed([object.union(pr(one_commit, [approval("bob", first)], first), {"commit_count": null})]) + not allowed([object.union(pr(one_commit, [approval("bob", first)], first), {"commit_count": "1"})]) +} + +test_wrong_type_merge_time_fails if { + not allowed([object.union(pr(one_commit, [review("bob", first, "APPROVED", merged_at + 1)], first), {"merged_at": "0"})]) +} + +test_pr_listing_more_commits_than_github_counts_fails if { + not allowed([object.union(pr(one_commit, [approval("bob", first)], first), {"commit_count": 0})]) +} + +test_co_authors_as_a_string_fails if { + not allowed([pr([object.union(commit(first, 1000000, "Copilot"), {"co_author_usernames": "alice"})], [approval("bob", first)], first)]) +} + +test_missing_commit_count_says_to_reattest if { + p := object.remove(pr(one_commit, [approval("bob", first)], first), ["commit_count"]) + v := policy.violations with input as {"trails": [trail(merge, [p])]} with data.params as params + some msg in v + contains(msg, "no commit count recorded") +} + +test_co_author_entries_must_be_accounts if { + not allowed([pr([object.union(commit(first, 1000000, "Copilot"), {"co_author_usernames": [["alice"]]})], [approval("alice", first)], first)]) + not allowed([pr([object.union(commit(first, 1000000, "Copilot"), {"co_author_usernames": ["ghost"]})], [approval("bob", first)], first)]) +} + +test_non_merge_trail_needs_a_resolved_pr_author if { + every author in [null, "", "ghost"] { + not policy.allow with input as {"trails": [trail(later, [object.union(pr(one_commit, [approval("bob", first)], first), {"author": author})])]} + with data.params as params + } +} + +test_null_commit_count_gives_no_count_mismatch_message if { + p := object.union(pr(one_commit, [approval("bob", first)], first), {"commit_count": null}) + v := policy.violations with input as {"trails": [trail(merge, [p])]} with data.params as params + every msg in v { + not contains(msg, "GitHub counts") + } +}