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..ca2ee5521 100644 --- a/bin/never_alone/four-eyes-policy.rego +++ b/bin/never_alone/four-eyes-policy.rego @@ -26,17 +26,23 @@ 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_commits_listed(pr) all_authors_resolved(pr) has_independent_approval(trail, pr) } @@ -66,42 +72,103 @@ is_resolved_username(u) if { u != "ghost" } -# GitHub usernames of all PR branch commit authors. -pr_commit_authors(pr) := {c.author_username | +# 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 - is_resolved_username(c.author_username) + some u in object.get(c, "co_author_usernames", []) + is_resolved_username(u) } -# Approver usernames that can satisfy four-eyes for commits up to cutoff. -approved_approvers_after_cutoff(pr, cutoff) := {a.username | - some a in pr.approvers +# 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.reviews a.state == "APPROVED" + a.author_type == "user" + a.has_write_access == true is_resolved_username(a.username) - a.timestamp > cutoff + is_number(a.timestamp) + is_string(pr.head_sha) + pr.head_sha != "" + a.commit_sha == pr.head_sha + given_before_merge(a, pr) + 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) +} + +# 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 } -# Latest Unix timestamp among PR branch commits. -latest_commit_ts(pr) := max({c.timestamp | some c in pr.commits}) +earlier(r, approval) if { + is_number(r.timestamp) + r.timestamp < approval.timestamp +} + +# 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 +} + +# 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. +# 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) } } +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_platform == 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) + is_resolved_username(pr.author) 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 +183,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 +212,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 +239,43 @@ 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], + ) +} + +# 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 @@ -187,8 +294,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 before merge", + [trail.name, repository], ) } @@ -197,6 +304,8 @@ violations contains msg if { # "unverifiable identity". 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 new file mode 100644 index 000000000..c55dcbdbc --- /dev/null +++ b/bin/never_alone/four-eyes-policy_test.rego @@ -0,0 +1,396 @@ +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_platform": false, +} + +# 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, reviews, head) := { + "url": "https://github.com/o/r/pull/1", + "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}, + "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 before merge", [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_platform"])], [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_platform_passes if { + 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. +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_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 + contains(msg, "no verified signature") +} + +test_invalid_github_signature_blocks_approval if { + 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 { + 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 +} + +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)]) +} + +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") + } +} 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) + } +} diff --git a/internal/github/build_pr_evidence_test.go b/internal/github/build_pr_evidence_test.go index c18935bae..c2e0cff40 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" ) @@ -40,7 +41,7 @@ func TestBuildPREvidence_RecordsAuthorNotCommitter(t *testing.T) { "", "Introduce kosli evaluate", "introduce-kosli-evaluate", - "main", + "main", "", []graphqlCommitNode{node}, nil, ) @@ -71,7 +72,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 +98,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 +116,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) @@ -132,16 +133,13 @@ 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", "0e723254516c841126e81f76100be57258ff1386", "MERGED", "tooky", "2026-03-01T09:00:00Z", "", - "title", "feature", "main", + "title", "feature", "main", "", []graphqlCommitNode{node}, nil, ) require.NoError(t, err) @@ -167,7 +165,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 +194,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 +202,169 @@ 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 := reviewNode("User", "grace", "APPROVED", true) + + 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") +} + +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].SignedByPlatform) + require.False(t, *evidence.Commits[0].SignedByPlatform) + + 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) + + 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_platform") +} + +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 +} + +// 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{ + approved, + reviewNode("Bot", "github-actions", "APPROVED", true), + reviewNode("User", "linus", "CHANGES_REQUESTED", false), + reviewNode("Mannequin", "ghost-import", "COMMENTED", false), + }, + ) + require.NoError(t, err) + + 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") +} + +// 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 e77e5b4d9..be942ca3b 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -274,27 +274,47 @@ type graphqlCommitNode struct { Login graphql.String } } - Signature *struct { - IsValid graphql.Boolean - State graphql.String - } + 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)"` + } +} + +type graphqlSignature struct { + IsValid graphql.Boolean + State graphql.String + WasSignedByGitHub graphql.Boolean + // 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 approved reviews on a PR. +// graphqlReviewNode is the shared GraphQL node type for submitted reviews 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 + AuthorCanPushToRepository graphql.Boolean + // Null when GitHub no longer has the commit the review was given on. + Commit *struct { + Oid graphql.String } - State graphql.String - SubmittedAt 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). +// raw GraphQL commit/review nodes. 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 +340,7 @@ func buildPREvidence( MergedAt: mergedAt, Title: title, HeadRef: headRef, + HeadSHA: headSHA, BaseRef: baseRef, Approvers: []any{}, Commits: []types.Commit{}, @@ -343,42 +364,85 @@ 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, signedByPlatform *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 + signedByPlatform = &g + if sig.Signer != nil { + 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, + 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, }) } + reviews := []types.PRApprovals{} for _, r := range reviewNodes { submittedAt, err := time.Parse(time.RFC3339, string(r.SubmittedAt)) if err != nil { return nil, err } - evidence.Approvers = append(evidence.Approvers, 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 { + 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.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) { @@ -397,6 +461,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 @@ -408,13 +473,14 @@ 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 PageInfo pageInfo - } `graphql:"reviews(first: 100, states: APPROVED, after: $reviewCursor)"` + } `graphql:"reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED], after: $reviewCursor)"` } `graphql:"pullRequest(number: $prNumber)"` } `graphql:"repository(owner: $owner, name: $repo)"` } @@ -452,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.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) { @@ -489,30 +561,35 @@ 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 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 { Nodes []graphqlReviewNode PageInfo pageInfo - } `graphql:"reviews(first: 100, states: APPROVED, after: $reviewCursor)"` + } `graphql:"reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED], after: $reviewCursor)"` } - // Intentionally not paginated, so no cursor is selected: a - // commit with more than 100 associated PRs is not a realistic - // 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)"` @@ -555,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.CreatedAt), string(pr.MergedAt), string(pr.Title), string(pr.HeadRefName), string(pr.BaseRefName), + 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/pagination.go b/internal/github/pagination.go index 3f5fe2e79..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:"reviews(first: 100, states: APPROVED, after: $cursor)"` + } `graphql:"reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED], after: $cursor)"` } `graphql:"pullRequest(number: $prNumber)"` } `graphql:"repository(owner: $owner, name: $repo)"` } @@ -183,7 +183,7 @@ func (c *GithubConfig) allPRReviews(ctx context.Context, run graphqlQueryFunc, r return page.Nodes, page.PageInfo, nil }) if err != nil { - return nil, fmt.Errorf("draining approvals for %s: %w", ref, err) + return nil, fmt.Errorf("draining reviews for %s: %w", ref, err) } return reviews, nil } diff --git a/internal/github/pr_pagination_test.go b/internal/github/pr_pagination_test.go index fd7fbc20f..0364c67ea 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":{"__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","authorCanPushToRepository":true,`+ + `"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 @@ -347,5 +355,115 @@ 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 reviewCommitSHAs(evidence *types.PREvidence) []string { + shas := []string{} + for _, r := range *evidence.Reviews { + shas = append(shas, r.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", ""}, 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, "reviews(first: 100, states: [APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED]") + require.Contains(t, body, "author{__typename,login}") + require.Contains(t, body, "authorCanPushToRepository") + } +} + +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].SignedByPlatform) + 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}") + require.Contains(t, ts.bodies[0], "wasSignedByGitHub") +} + +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"}, reviewCommitSHAs(prs[0])) + require.Contains(t, ts.bodies[0], "headRefOid") + require.Contains(t, ts.bodies[0], "commit{oid}") + 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 25db609fb..cc9a9e419 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -3,17 +3,23 @@ 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"` - 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"` + // 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 @@ -26,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) } @@ -33,18 +47,26 @@ type PRApprovals struct { Username string `json:"username"` 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 { - 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"` + 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"` + // 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 }