diff --git a/README.md b/README.md index b7cfbdb..16045f0 100644 --- a/README.md +++ b/README.md @@ -43,7 +43,33 @@ Org-level ruleset workflow that verifies pull requests have an independent emplo Configure it to run via an org [ruleset](#rulesets) that requires this workflow on `pull_request_target` events. See the Rulesets section below for caveats. -**Disclaimer:** this workflow is currently a no-op that will always pass. +The check only reads GitHub pull request metadata via the API; it does not checkout or execute code from the pull request branch. It uses `pull_request_target` so the workflow YAML and scripts always run from `main`, and only `GitHub-Actions@main` is checked out. + +This workflow requires a GitHub App token with read access for repository metadata, pull requests, organization members, and branch administration. It uses the Peer Review Checker app ID `3877737` and the org secret `PEER_REVIEW_CHECKER_PRIVATE_KEY` to generate that token. + +If branch protection cannot be read due to missing permissions, the check fails. If the target branch has no branch-protection review requirement, the check skips. If branch protection cannot be queried due to a transient API error, the script defaults to requiring one independent approval. + +#### Local development + +This script depends on the shared `CLI` utility from `expensify-common`. + +```bash +cd GitHub-Actions +nvm use +npm ci +npm test +npm run verify-peer-review -- --help +``` + +Smoke-test against a real pull request by passing the pull request metadata as CLI arguments and setting `GITHUB_TOKEN` (or `GH_TOKEN`) to a token with the same read permissions as the Peer Review Checker app: + +```bash +GITHUB_TOKEN=... npm run verify-peer-review -- \ + --owner Expensify \ + --repo Auth \ + --pull-request-number 12345 \ + --base-ref main +``` ### `setup-composer-cache` @@ -68,3 +94,4 @@ GitHub [org-level rulesets](https://docs.github.com/en/enterprise-cloud@latest/r - If you need to target or exclude specific paths, that must be implemented manually in the workflow itself. - Due to a GitHub :bug:, PRs that are open when the rule is enabled will get stuck with a pending check that will never get picked up. The easiest way to fix that is to close and reopen the PR. Consider writing a script to close and reopen all open PRs across the org after the check is enabled. - It is less disruptive to [configure the ruleset to `Evaluate` first](https://docs.github.com/en/enterprise-cloud@latest/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/available-rules-for-rulesets#using-evaluate-mode-for-ruleset-workflows), then `Active` once the kinks are worked out. +- For `verifyPeerReview.yml`, start with a ruleset targeting only a test branch, then test the workflow from a GitHub-Actions branch, then from `main`, and only then enable it for the intended repositories and branches. diff --git a/package-lock.json b/package-lock.json index 5a1a191..0a6aa52 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6,6 +6,11 @@ "": { "name": "@expensify/github-actions", "dependencies": { + "@octokit/graphql": "^9.0.3", + "@octokit/plugin-paginate-rest": "^14.0.0", + "@octokit/plugin-throttling": "^11.0.0", + "@octokit/request-error": "^7.0.0", + "@octokit/rest": "^22.0.1", "expensify-common": "2.0.189" }, "devDependencies": { @@ -1070,6 +1075,177 @@ "node": ">=4.0" } }, + "node_modules/@octokit/auth-token": { + "version": "6.0.0", + "resolved": "https://registry.npmjs.org/@octokit/auth-token/-/auth-token-6.0.0.tgz", + "integrity": "sha512-P4YJBPdPSpWTQ1NU4XYdvHvXJJDxM6YwpS0FZHRgP7YFkdVxsWcpWGy/NVqlAA7PcPCnMacXlRm1y2PFZRWL/w==", + "license": "MIT", + "engines": { + "node": ">= 20" + } + }, + "node_modules/@octokit/core": { + "version": "7.0.6", + "resolved": "https://registry.npmjs.org/@octokit/core/-/core-7.0.6.tgz", + "integrity": "sha512-DhGl4xMVFGVIyMwswXeyzdL4uXD5OGILGX5N8Y+f6W7LhC1Ze2poSNrkF/fedpVDHEEZ+PHFW0vL14I+mm8K3Q==", + "license": "MIT", + "dependencies": { + "@octokit/auth-token": "^6.0.0", + "@octokit/graphql": "^9.0.3", + "@octokit/request": "^10.0.6", + "@octokit/request-error": "^7.0.2", + "@octokit/types": "^16.0.0", + "before-after-hook": "^4.0.0", + "universal-user-agent": "^7.0.0" + }, + "engines": { + "node": ">= 20" + } + }, + "node_modules/@octokit/endpoint": { + "version": "11.0.3", + "resolved": "https://registry.npmjs.org/@octokit/endpoint/-/endpoint-11.0.3.tgz", + "integrity": "sha512-FWFlNxghg4HrXkD3ifYbS/IdL/mDHjh9QcsNyhQjN8dplUoZbejsdpmuqdA76nxj2xoWPs7p8uX2SNr9rYu0Ag==", + "license": "MIT", + "dependencies": { + "@octokit/types": "^16.0.0", + "universal-user-agent": "^7.0.2" + }, + "engines": { + "node": ">= 20" + } + }, + "node_modules/@octokit/graphql": { + "version": "9.0.3", + "resolved": "https://registry.npmjs.org/@octokit/graphql/-/graphql-9.0.3.tgz", + "integrity": "sha512-grAEuupr/C1rALFnXTv6ZQhFuL1D8G5y8CN04RgrO4FIPMrtm+mcZzFG7dcBm+nq+1ppNixu+Jd78aeJOYxlGA==", + "license": "MIT", + "dependencies": { + "@octokit/request": "^10.0.6", + "@octokit/types": "^16.0.0", + "universal-user-agent": "^7.0.0" + }, + "engines": { + "node": ">= 20" + } + }, + "node_modules/@octokit/openapi-types": { + "version": "27.0.0", + "resolved": "https://registry.npmjs.org/@octokit/openapi-types/-/openapi-types-27.0.0.tgz", + "integrity": "sha512-whrdktVs1h6gtR+09+QsNk2+FO+49j6ga1c55YZudfEG+oKJVvJLQi3zkOm5JjiUXAagWK2tI2kTGKJ2Ys7MGA==", + "license": "MIT" + }, + "node_modules/@octokit/plugin-paginate-rest": { + "version": "14.0.0", + "resolved": "https://registry.npmjs.org/@octokit/plugin-paginate-rest/-/plugin-paginate-rest-14.0.0.tgz", + "integrity": "sha512-fNVRE7ufJiAA3XUrha2omTA39M6IXIc6GIZLvlbsm8QOQCYvpq/LkMNGyFlB1d8hTDzsAXa3OKtybdMAYsV/fw==", + "license": "MIT", + "dependencies": { + "@octokit/types": "^16.0.0" + }, + "engines": { + "node": ">= 20" + }, + "peerDependencies": { + "@octokit/core": ">=6" + } + }, + "node_modules/@octokit/plugin-request-log": { + "version": "6.0.0", + "resolved": "https://registry.npmjs.org/@octokit/plugin-request-log/-/plugin-request-log-6.0.0.tgz", + "integrity": "sha512-UkOzeEN3W91/eBq9sPZNQ7sUBvYCqYbrrD8gTbBuGtHEuycE4/awMXcYvx6sVYo7LypPhmQwwpUe4Yyu4QZN5Q==", + "license": "MIT", + "engines": { + "node": ">= 20" + }, + "peerDependencies": { + "@octokit/core": ">=6" + } + }, + "node_modules/@octokit/plugin-rest-endpoint-methods": { + "version": "17.0.0", + "resolved": "https://registry.npmjs.org/@octokit/plugin-rest-endpoint-methods/-/plugin-rest-endpoint-methods-17.0.0.tgz", + "integrity": "sha512-B5yCyIlOJFPqUUeiD0cnBJwWJO8lkJs5d8+ze9QDP6SvfiXSz1BF+91+0MeI1d2yxgOhU/O+CvtiZ9jSkHhFAw==", + "license": "MIT", + "dependencies": { + "@octokit/types": "^16.0.0" + }, + "engines": { + "node": ">= 20" + }, + "peerDependencies": { + "@octokit/core": ">=6" + } + }, + "node_modules/@octokit/plugin-throttling": { + "version": "11.0.3", + "resolved": "https://registry.npmjs.org/@octokit/plugin-throttling/-/plugin-throttling-11.0.3.tgz", + "integrity": "sha512-34eE0RkFCKycLl2D2kq7W+LovheM/ex3AwZCYN8udpi6bxsyjZidb2McXs69hZhLmJlDqTSP8cH+jSRpiaijBg==", + "license": "MIT", + "dependencies": { + "@octokit/types": "^16.0.0", + "bottleneck": "^2.15.3" + }, + "engines": { + "node": ">= 20" + }, + "peerDependencies": { + "@octokit/core": "^7.0.0" + } + }, + "node_modules/@octokit/request": { + "version": "10.0.11", + "resolved": "https://registry.npmjs.org/@octokit/request/-/request-10.0.11.tgz", + "integrity": "sha512-+s7HUxjfFqOMS9VlIwDffq0MikjSAK0gSpG73W+meAvVAvX4MBrHYTK5Bj3Uot55qFT4gzUtfzE4mGWY4Br8/Q==", + "license": "MIT", + "dependencies": { + "@octokit/endpoint": "^11.0.3", + "@octokit/request-error": "^7.0.2", + "@octokit/types": "^16.0.0", + "content-type": "^2.0.0", + "json-with-bigint": "^3.5.3", + "universal-user-agent": "^7.0.2" + }, + "engines": { + "node": ">= 20" + } + }, + "node_modules/@octokit/request-error": { + "version": "7.1.0", + "resolved": "https://registry.npmjs.org/@octokit/request-error/-/request-error-7.1.0.tgz", + "integrity": "sha512-KMQIfq5sOPpkQYajXHwnhjCC0slzCNScLHs9JafXc4RAJI+9f+jNDlBNaIMTvazOPLgb4BnlhGJOTbnN0wIjPw==", + "license": "MIT", + "dependencies": { + "@octokit/types": "^16.0.0" + }, + "engines": { + "node": ">= 20" + } + }, + "node_modules/@octokit/rest": { + "version": "22.0.1", + "resolved": "https://registry.npmjs.org/@octokit/rest/-/rest-22.0.1.tgz", + "integrity": "sha512-Jzbhzl3CEexhnivb1iQ0KJ7s5vvjMWcmRtq5aUsKmKDrRW6z3r84ngmiFKFvpZjpiU/9/S6ITPFRpn5s/3uQJw==", + "license": "MIT", + "dependencies": { + "@octokit/core": "^7.0.6", + "@octokit/plugin-paginate-rest": "^14.0.0", + "@octokit/plugin-request-log": "^6.0.0", + "@octokit/plugin-rest-endpoint-methods": "^17.0.0" + }, + "engines": { + "node": ">= 20" + } + }, + "node_modules/@octokit/types": { + "version": "16.0.0", + "resolved": "https://registry.npmjs.org/@octokit/types/-/types-16.0.0.tgz", + "integrity": "sha512-sKq+9r1Mm4efXW1FCk7hFSeJo4QKreL/tTbR0rz/qx/r1Oa2VV83LTA/H/MuCOX7uCIJmQVRKBcbmWoySjAnSg==", + "license": "MIT", + "dependencies": { + "@octokit/openapi-types": "^27.0.0" + } + }, "node_modules/@oxfmt/binding-android-arm-eabi": { "version": "0.57.0", "resolved": "https://registry.npmjs.org/@oxfmt/binding-android-arm-eabi/-/binding-android-arm-eabi-0.57.0.tgz", @@ -2208,6 +2384,18 @@ "node": ">=6.0.0" } }, + "node_modules/before-after-hook": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/before-after-hook/-/before-after-hook-4.0.0.tgz", + "integrity": "sha512-q6tR3RPqIB1pMiTRMFcZwuG5T8vwp+vUvEG0vuI6B+Rikh5BfPp2fQ82c925FOs+b0lcFQ8CFrL+KbilfZFhOQ==", + "license": "Apache-2.0" + }, + "node_modules/bottleneck": { + "version": "2.19.5", + "resolved": "https://registry.npmjs.org/bottleneck/-/bottleneck-2.19.5.tgz", + "integrity": "sha512-VHiNCbI1lKdl44tGrhNfU3lup0Tj/ZBMJB5/2ZbNXRCPuRCO7ed2mgcK4r17y+KB2EfuYuRaVlwNbAeaWGSpbw==", + "license": "MIT" + }, "node_modules/brace-expansion": { "version": "1.1.15", "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.15.tgz", @@ -2474,6 +2662,19 @@ "dev": true, "license": "MIT" }, + "node_modules/content-type": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/content-type/-/content-type-2.0.0.tgz", + "integrity": "sha512-j/O/d7GcZCyNl7/hwZAb606rzqkyvaDctLmckbxLzHvFBzTJHuGEdodATcP3yIRoDrLHkIATJuvzbFlp/ki2cQ==", + "license": "MIT", + "engines": { + "node": ">=18" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/express" + } + }, "node_modules/convert-source-map": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/convert-source-map/-/convert-source-map-2.0.0.tgz", @@ -4589,6 +4790,12 @@ "dev": true, "license": "MIT" }, + "node_modules/json-with-bigint": { + "version": "3.5.8", + "resolved": "https://registry.npmjs.org/json-with-bigint/-/json-with-bigint-3.5.8.tgz", + "integrity": "sha512-eq/4KP6K34kwa7TcFdtvnftvHCD9KvHOGGICWwMFc4dOOKF5t4iYqnfLK8otCRCRv06FXOzGGyqE8h8ElMvvdw==", + "license": "MIT" + }, "node_modules/json5": { "version": "2.2.3", "resolved": "https://registry.npmjs.org/json5/-/json5-2.2.3.tgz", @@ -6268,6 +6475,12 @@ "dev": true, "license": "MIT" }, + "node_modules/universal-user-agent": { + "version": "7.0.3", + "resolved": "https://registry.npmjs.org/universal-user-agent/-/universal-user-agent-7.0.3.tgz", + "integrity": "sha512-TmnEAEAsBJVZM/AADELsK76llnwcf9vMKuPz8JflO1frO8Lchitr0fNaN9d+Ap0BjKtqWqd/J17qeDnXh8CL2A==", + "license": "ISC" + }, "node_modules/update-browserslist-db": { "version": "1.2.3", "resolved": "https://registry.npmjs.org/update-browserslist-db/-/update-browserslist-db-1.2.3.tgz", diff --git a/package.json b/package.json index 76171f3..a05ce09 100644 --- a/package.json +++ b/package.json @@ -10,6 +10,11 @@ "test": "tsx --test tests/*.test.ts" }, "dependencies": { + "@octokit/graphql": "^9.0.3", + "@octokit/plugin-paginate-rest": "^14.0.0", + "@octokit/plugin-throttling": "^11.0.0", + "@octokit/request-error": "^7.0.0", + "@octokit/rest": "^22.0.1", "expensify-common": "2.0.189" }, "devDependencies": { diff --git a/scripts/libs/CollectionUtils.ts b/scripts/libs/CollectionUtils.ts new file mode 100644 index 0000000..a704e51 --- /dev/null +++ b/scripts/libs/CollectionUtils.ts @@ -0,0 +1,7 @@ +function uniqueSorted(values: string[]): string[] { + return [...new Set(values)].sort((a, b) => a.localeCompare(b)); +} + +export default { + uniqueSorted, +}; diff --git a/scripts/libs/GitCommitUtils.ts b/scripts/libs/GitCommitUtils.ts new file mode 100644 index 0000000..1147e79 --- /dev/null +++ b/scripts/libs/GitCommitUtils.ts @@ -0,0 +1,105 @@ +type GitHubPullRequestCommit = { + author: { + login?: string; + } | null; + commit: { + message: string; + author?: { + name?: string | null; + } | null; + }; +}; + +type GitHubCoAuthor = { + displayName: string; + email: string; +}; + +const GITHUB_LOGIN_PATTERN = /^[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,37}[a-zA-Z0-9])?$/; + +function parseCoAuthors(message: string): GitHubCoAuthor[] { + return [...message.matchAll(/^Co-authored-by:\s+(.+?)<([^>]+)>$/gim)].map((match) => ({ + displayName: match[1].trim(), + email: match[2].trim(), + })); +} + +function parseCoAuthorEmails(message: string): string[] { + return parseCoAuthors(message).map((coAuthor) => coAuthor.email); +} + +function resolveNoreplyEmailToLogin(email: string): string | null { + const normalizedEmail = email.trim(); + return normalizedEmail.match(/^(?:\d+\+)?(.+)@users\.noreply\.github\.com$/i)?.[1] ?? null; +} + +function resolveDisplayNameToLogin(displayName: string): string | null { + const trimmedDisplayName = displayName.trim(); + if (!trimmedDisplayName) { + return null; + } + + const candidates = [trimmedDisplayName, trimmedDisplayName.replaceAll(/\s+/g, '')]; + for (const candidate of candidates) { + if (GITHUB_LOGIN_PATTERN.test(candidate)) { + return candidate; + } + } + + return null; +} + +function findAllowedLogin(login: string, allowedLogins: Set): string | null { + for (const allowedLogin of allowedLogins) { + if (allowedLogin.toLowerCase() === login.toLowerCase()) { + return allowedLogin; + } + } + + return null; +} + +function resolveCoAuthorToLogin(coAuthor: GitHubCoAuthor, allowedLogins?: Set): string | null { + const loginFromNoreply = resolveNoreplyEmailToLogin(coAuthor.email); + if (loginFromNoreply) { + return loginFromNoreply; + } + + const loginFromDisplayName = resolveDisplayNameToLogin(coAuthor.displayName); + if (!loginFromDisplayName) { + return null; + } + + if (!allowedLogins) { + return loginFromDisplayName; + } + + return findAllowedLogin(loginFromDisplayName, allowedLogins); +} + +function getCanonicalAuthorLogin(commit: GitHubPullRequestCommit): string { + const authorLogin = commit.author?.login?.trim() ?? ''; + if (authorLogin) { + return authorLogin; + } + + // If the author's profile is private, author.login may be missing. Fall back to the commit author name. + const authorName = commit.commit.author?.name?.trim() ?? ''; + if (authorName) { + return authorName; + } + + throw new Error('Unable to resolve canonical commit author: missing GitHub author login and commit author name.'); +} + +export type {GitHubCoAuthor, GitHubPullRequestCommit}; + +export default { + parseCoAuthorEmails, + parseCoAuthors, + getCanonicalAuthorLogin, + resolveCoAuthorToLogin, + resolveDisplayNameToLogin, + resolveNoreplyEmailToLogin, + findAllowedLogin, +}; diff --git a/scripts/libs/GitHubAPIClient.ts b/scripts/libs/GitHubAPIClient.ts new file mode 100644 index 0000000..4389216 --- /dev/null +++ b/scripts/libs/GitHubAPIClient.ts @@ -0,0 +1,98 @@ +import {graphql as createGraphql} from '@octokit/graphql'; +import {paginateRest} from '@octokit/plugin-paginate-rest'; +import type {PaginateInterface} from '@octokit/plugin-paginate-rest'; +import {throttling} from '@octokit/plugin-throttling'; +import {Octokit} from '@octokit/rest'; + +type GraphqlQuery = (query: string, variables?: Record) => Promise; + +type GraphqlHandler = (query: string, variables?: Record) => Promise; + +type InternalOctokit = InstanceType; + +const OctokitWithPlugins = Octokit.plugin(throttling, paginateRest); + +class GitHubAPIClient { + static internalOctokit: InternalOctokit | undefined; + + static graphqlClient: GraphqlHandler | undefined; + + static initWithToken(token: string): void { + this.internalOctokit = new OctokitWithPlugins({ + auth: token, + throttle: { + retryAfterBaseValue: 2000, + onRateLimit: (retryAfter, options) => { + console.warn(`Request quota exhausted for request ${options.method} ${options.url}`); + + if (options.request.retryCount <= 5) { + console.warn(`Retrying after ${retryAfter} seconds!`); + return true; + } + + return false; + }, + onSecondaryRateLimit: (retryAfter, options) => { + console.warn(`Abuse detected for request ${options.method} ${options.url}`); + }, + }, + }); + + this.graphqlClient = createGraphql.defaults({ + headers: { + authorization: `token ${token}`, + }, + }); + } + + static init(): void { + const token = process.env.GITHUB_TOKEN ?? process.env.GH_TOKEN; + if (!token) { + throw new Error('GITHUB_TOKEN or GH_TOKEN is required'); + } + + this.initWithToken(token); + } + + private static ensureOctokit(): InternalOctokit { + if (!this.internalOctokit) { + this.init(); + } + + if (!this.internalOctokit) { + throw new Error('Failed to initialize GitHub API client'); + } + + return this.internalOctokit; + } + + private static ensureGraphqlClient(): GraphqlHandler { + if (!this.graphqlClient) { + this.init(); + } + + if (!this.graphqlClient) { + throw new Error('Failed to initialize GitHub GraphQL client'); + } + + return this.graphqlClient; + } + + static get octokit(): InternalOctokit['rest'] { + return this.ensureOctokit().rest; + } + + static get graphql(): GraphqlQuery { + const handler = this.ensureGraphqlClient(); + return (query: string, variables?: Record): Promise => + // GraphQL responses are typed at the call site; the handler returns untyped JSON. + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- generic graphql boundary + handler(query, variables) as Promise; + } + + static get paginate(): PaginateInterface { + return this.ensureOctokit().paginate; + } +} + +export default GitHubAPIClient; diff --git a/scripts/libs/GitHubUtils/getEmployeeLogins.ts b/scripts/libs/GitHubUtils/getEmployeeLogins.ts new file mode 100644 index 0000000..c5f405b --- /dev/null +++ b/scripts/libs/GitHubUtils/getEmployeeLogins.ts @@ -0,0 +1,69 @@ +import GitHubAPIClient from '../GitHubAPIClient'; + +const EXPENSIFY_ORG = 'Expensify'; +const EXPENSIFY_EMPLOYEE_TEAM_SLUG = 'expensify-expensify'; + +type TeamMembersResponse = { + organization: { + team: { + members: { + pageInfo: { + hasNextPage: boolean; + endCursor: string | null; + }; + nodes: Array<{ + login: string; + }>; + }; + } | null; + } | null; +}; + +async function getEmployeeLogins(): Promise> { + const employeeLogins = new Set(); + + async function collectPage(cursor: string | null): Promise { + const response: TeamMembersResponse = await GitHubAPIClient.graphql( + ` + query TeamMembers($organization: String!, $teamSlug: String!, $cursor: String) { + organization(login: $organization) { + team(slug: $teamSlug) { + members(first: 100, after: $cursor) { + pageInfo { + hasNextPage + endCursor + } + nodes { + login + } + } + } + } + } + `, + { + organization: EXPENSIFY_ORG, + teamSlug: EXPENSIFY_EMPLOYEE_TEAM_SLUG, + cursor, + }, + ); + + const members = response.organization?.team?.members; + if (!members) { + throw new Error(`${EXPENSIFY_ORG}/${EXPENSIFY_EMPLOYEE_TEAM_SLUG} team could not be found.`); + } + + for (const member of members.nodes) { + employeeLogins.add(member.login); + } + + if (members.pageInfo.hasNextPage) { + await collectPage(members.pageInfo.endCursor); + } + } + + await collectPage(null); + return employeeLogins; +} + +export default getEmployeeLogins; diff --git a/scripts/libs/GitHubUtils/getLatestApprovers.ts b/scripts/libs/GitHubUtils/getLatestApprovers.ts new file mode 100644 index 0000000..b974321 --- /dev/null +++ b/scripts/libs/GitHubUtils/getLatestApprovers.ts @@ -0,0 +1,55 @@ +import CollectionUtils from '../CollectionUtils'; +import GitHubAPIClient from '../GitHubAPIClient'; + +type OpinionatedReviewNode = { + state: string; + author: { + login: string; + } | null; +}; + +type LatestOpinionatedReviewsResponse = { + repository: { + pullRequest: { + latestOpinionatedReviews: { + nodes: OpinionatedReviewNode[]; + }; + } | null; + } | null; +}; + +async function getLatestApprovers({owner, repo, number}: {owner: string; repo: string; number: number}): Promise { + const response = await GitHubAPIClient.graphql( + ` + query LatestOpinionatedReviews($owner: String!, $repo: String!, $prNumber: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $prNumber) { + latestOpinionatedReviews(last: 100, writersOnly: true) { + nodes { + state + author { + login + } + } + } + } + } + } + `, + { + owner, + repo, + prNumber: number, + }, + ); + + const reviews: OpinionatedReviewNode[] = response.repository?.pullRequest?.latestOpinionatedReviews.nodes ?? []; + return CollectionUtils.uniqueSorted( + reviews + .filter((review) => review.state === 'APPROVED') + .map((review) => review.author?.login ?? '') + .filter((login) => login !== ''), + ); +} + +export default getLatestApprovers; diff --git a/scripts/libs/GitHubUtils/getRequiredApprovingReviewCount.ts b/scripts/libs/GitHubUtils/getRequiredApprovingReviewCount.ts new file mode 100644 index 0000000..24fafee --- /dev/null +++ b/scripts/libs/GitHubUtils/getRequiredApprovingReviewCount.ts @@ -0,0 +1,51 @@ +import GitHubAPIClient from '../GitHubAPIClient'; +import isPermissionError from './isPermissionError'; + +const DEFAULT_REQUIRED_APPROVING_REVIEW_COUNT = 1; + +type BranchProtectionResponse = { + repository: { + ref: { + branchProtectionRule: { + requiredApprovingReviewCount: number; + } | null; + } | null; + } | null; +}; + +async function getRequiredApprovingReviewCount({owner, repo, baseRef}: {owner: string; repo: string; baseRef: string}): Promise { + try { + const response = await GitHubAPIClient.graphql( + ` + query RequiredApprovingReviewCount($owner: String!, $repo: String!, $branchRef: String!) { + repository(owner: $owner, name: $repo) { + ref(qualifiedName: $branchRef) { + branchProtectionRule { + requiredApprovingReviewCount + } + } + } + } + `, + { + owner, + repo, + branchRef: `refs/heads/${baseRef}`, + }, + ); + + return response.repository?.ref?.branchProtectionRule?.requiredApprovingReviewCount ?? 0; + } catch (error: unknown) { + if (isPermissionError(error)) { + throw new Error(`Unable to read branch protection rules for ${owner}/${repo}@${baseRef}. Ensure the GitHub App has administration:read permission.`); + } + + const message = error instanceof Error ? error.message : String(error); + console.warn( + `${owner}/${repo}@${baseRef} did not return a branch protection review count (${message}); requiring ${DEFAULT_REQUIRED_APPROVING_REVIEW_COUNT} independent approval(s).`, + ); + return DEFAULT_REQUIRED_APPROVING_REVIEW_COUNT; + } +} + +export default getRequiredApprovingReviewCount; diff --git a/scripts/libs/GitHubUtils/index.ts b/scripts/libs/GitHubUtils/index.ts new file mode 100644 index 0000000..6696758 --- /dev/null +++ b/scripts/libs/GitHubUtils/index.ts @@ -0,0 +1,13 @@ +import getEmployeeLogins from './getEmployeeLogins'; +import getLatestApprovers from './getLatestApprovers'; +import getRequiredApprovingReviewCount from './getRequiredApprovingReviewCount'; +import isBotUser from './isBotUser'; +import listPullRequestCommits from './listPullRequestCommits'; + +export default { + getEmployeeLogins, + getLatestApprovers, + getRequiredApprovingReviewCount, + isBotUser, + listPullRequestCommits, +}; diff --git a/scripts/libs/GitHubUtils/isBotUser.ts b/scripts/libs/GitHubUtils/isBotUser.ts new file mode 100644 index 0000000..77f8036 --- /dev/null +++ b/scripts/libs/GitHubUtils/isBotUser.ts @@ -0,0 +1,11 @@ +const KNOWN_BOT_USERS = new Set(['botify', 'MelvinBot', 'exfy-zapier', 'OSBotify', 'CLABotify']); + +function isBotUser(login: string): boolean { + if (login.endsWith('[bot]')) { + return true; + } + + return KNOWN_BOT_USERS.has(login); +} + +export default isBotUser; diff --git a/scripts/libs/GitHubUtils/isPermissionError.ts b/scripts/libs/GitHubUtils/isPermissionError.ts new file mode 100644 index 0000000..5efea65 --- /dev/null +++ b/scripts/libs/GitHubUtils/isPermissionError.ts @@ -0,0 +1,28 @@ +import {RequestError} from '@octokit/request-error'; + +type GraphQLErrorResponse = { + errors?: Array<{ + type?: string; + message?: string; + }>; +}; + +function isGraphQLErrorResponse(error: unknown): error is GraphQLErrorResponse { + return typeof error === 'object' && error !== null && 'errors' in error; +} + +function isPermissionError(error: unknown): boolean { + if (error instanceof RequestError) { + return error.status === 401 || error.status === 403; + } + + if (isGraphQLErrorResponse(error)) { + const {errors} = error; + return errors?.some((graphQLError) => graphQLError.type === 'FORBIDDEN' || graphQLError.type === 'INSUFFICIENT_SCOPES') ?? false; + } + + const message = error instanceof Error ? error.message : String(error); + return /resource not accessible by integration|must have admin access|insufficient scopes|permission denied/i.test(message); +} + +export default isPermissionError; diff --git a/scripts/libs/GitHubUtils/listPullRequestCommits.ts b/scripts/libs/GitHubUtils/listPullRequestCommits.ts new file mode 100644 index 0000000..e143cdb --- /dev/null +++ b/scripts/libs/GitHubUtils/listPullRequestCommits.ts @@ -0,0 +1,14 @@ +import GitHubAPIClient from '../GitHubAPIClient'; + +async function listPullRequestCommits({owner, repo, number}: {owner: string; repo: string; number: number}) { + return GitHubAPIClient.paginate(GitHubAPIClient.octokit.pulls.listCommits, { + owner, + repo, + // eslint-disable-next-line @typescript-eslint/naming-convention -- Octokit REST API uses snake_case parameters + pull_number: number, + // eslint-disable-next-line @typescript-eslint/naming-convention -- Octokit REST API uses snake_case parameters + per_page: 100, + }); +} + +export default listPullRequestCommits; diff --git a/scripts/libs/GitHubWorkflowUtils.ts b/scripts/libs/GitHubWorkflowUtils.ts new file mode 100644 index 0000000..f5f55fa --- /dev/null +++ b/scripts/libs/GitHubWorkflowUtils.ts @@ -0,0 +1,31 @@ +import {appendFileSync} from 'node:fs'; + +function escapeWorkflowCommandValue(value: string): string { + return value.replaceAll('%', '%25').replaceAll('\r', '%0D').replaceAll('\n', '%0A'); +} + +function escapeWorkflowCommandProperty(value: string): string { + return escapeWorkflowCommandValue(value).replaceAll(':', '%3A').replaceAll(',', '%2C'); +} + +function writeStepSummary(title: string, message: string): void { + const stepSummaryPath = process.env.GITHUB_STEP_SUMMARY; + if (!stepSummaryPath) { + return; + } + appendFileSync(stepSummaryPath, `## ${title}\n\n${message.replaceAll('\n', '\n\n')}\n`); +} + +function emitFailure(error: unknown, title = 'Workflow step failed'): never { + const message = error instanceof Error ? error.message : String(error); + writeStepSummary(title, message); + console.error(`::error title=${escapeWorkflowCommandProperty(title)}::${escapeWorkflowCommandValue(message)}`); + process.exit(1); +} + +export default { + emitFailure, + escapeWorkflowCommandProperty, + escapeWorkflowCommandValue, + writeStepSummary, +}; diff --git a/scripts/verifyPeerReview.ts b/scripts/verifyPeerReview.ts old mode 100755 new mode 100644 index 85d9bbd..98c7e67 --- a/scripts/verifyPeerReview.ts +++ b/scripts/verifyPeerReview.ts @@ -2,6 +2,155 @@ import CLI from 'expensify-common/CLI'; +import CollectionUtils from './libs/CollectionUtils'; +import GitCommitUtils, {type GitHubPullRequestCommit} from './libs/GitCommitUtils'; +import GitHubUtils from './libs/GitHubUtils'; +import GitHubWorkflowUtils from './libs/GitHubWorkflowUtils'; + +type PeerReviewInput = { + owner: string; + repo: string; + number: number; + baseRef: string; + requiredApprovingReviewCount: number; + approvers: string[]; + authors: string[]; + unresolvedExpensifyCoAuthors: string[]; + employeeLogins: Set; +}; + +type PeerReviewResult = {status: 'pass'; reason: string} | {status: 'skip'; reason: string} | {status: 'fail'; error: Error}; + +function formatUsers(users: string[]): string { + return users.length > 0 ? users.join(', ') : '(none)'; +} + +function isExpensifyEmail(email: string): boolean { + return email.trim().toLowerCase().endsWith('@expensify.com'); +} + +function getCommitAuthors( + commits: GitHubPullRequestCommit[], + employeeLogins?: Set, +): { + authors: string[]; + unresolvedExpensifyCoAuthors: string[]; +} { + const authors = new Set(); + const unresolvedExpensifyCoAuthors = new Set(); + + for (const commit of commits) { + const canonicalAuthor = GitCommitUtils.getCanonicalAuthorLogin(commit); + authors.add(canonicalAuthor); + + // Co-authorship between two humans from making and accepting a suggestion does not violate peer review. + // Only parse co-authors when the canonical commit author is a bot. + if (!GitHubUtils.isBotUser(canonicalAuthor)) { + continue; + } + + for (const coAuthor of GitCommitUtils.parseCoAuthors(commit.commit.message)) { + const login = GitCommitUtils.resolveCoAuthorToLogin(coAuthor, employeeLogins); + if (login) { + authors.add(login); + } else if (isExpensifyEmail(coAuthor.email)) { + unresolvedExpensifyCoAuthors.add(coAuthor.email.trim()); + } + } + } + + return { + authors: CollectionUtils.uniqueSorted([...authors]), + unresolvedExpensifyCoAuthors: CollectionUtils.uniqueSorted([...unresolvedExpensifyCoAuthors]), + }; +} + +function getIndependentEmployeeApprovers(approvers: string[], authors: string[], employeeLogins: Set): string[] { + const authorsLowerCase = new Set(authors.map((author) => author.toLowerCase())); + + return approvers.flatMap((approver) => { + if (authorsLowerCase.has(approver.toLowerCase())) { + return []; + } + + const canonicalEmployeeLogin = GitCommitUtils.findAllowedLogin(approver, employeeLogins); + return canonicalEmployeeLogin ? [canonicalEmployeeLogin] : []; + }); +} + +function evaluatePeerReview(input: PeerReviewInput): PeerReviewResult { + const {owner, repo, number, baseRef, requiredApprovingReviewCount, approvers, authors, unresolvedExpensifyCoAuthors, employeeLogins} = input; + const prSlug = `${owner}/${repo}#${number}`; + + if (requiredApprovingReviewCount === 0) { + return { + status: 'skip', + reason: `${prSlug} targets ${baseRef}, which does not require approving reviews.`, + }; + } + + if (approvers.length === 0) { + return { + status: 'skip', + reason: `${prSlug} has no approving reviews from writers; regular branch protection will block merge until an approval exists.`, + }; + } + + if (unresolvedExpensifyCoAuthors.length > 0) { + return { + status: 'fail', + error: new Error(`Unable to resolve Expensify co-author emails to GitHub users: ${formatUsers(unresolvedExpensifyCoAuthors)}`), + }; + } + + if (authors.every((author) => GitHubUtils.isBotUser(author))) { + return { + status: 'fail', + error: new Error(`Unable to verify independent peer review because ${prSlug} has no human commit authors or co-authors.`), + }; + } + + const independentEmployeeApprovers = getIndependentEmployeeApprovers(approvers, authors, employeeLogins); + if (independentEmployeeApprovers.length < requiredApprovingReviewCount) { + return { + status: 'fail', + error: new Error( + [ + `${prSlug} does not have enough independent Expensify employee approvals.`, + `Required independent approvals: ${requiredApprovingReviewCount}`, + `Commit authors/co-authors: ${formatUsers(authors)}`, + `Approvers: ${formatUsers(approvers)}`, + `Independent employee approvers: ${formatUsers(independentEmployeeApprovers)}`, + ].join('\n'), + ), + }; + } + + return { + status: 'pass', + reason: `${prSlug} has ${independentEmployeeApprovers.length} independent Expensify employee approval(s).`, + }; +} + +function getFailureTitle(message: string): string { + if (message.includes('does not have enough independent Expensify employee approvals')) { + return 'Missing independent peer review'; + } + if (message.includes('Unable to resolve Expensify co-author emails')) { + return 'Unresolved Expensify co-author'; + } + if (message.includes('Unable to resolve canonical commit author')) { + return 'Missing commit author'; + } + if (message.includes('has no human commit authors or co-authors')) { + return 'No human commit author'; + } + if (message.includes('Unable to read branch protection rules')) { + return 'Branch protection lookup failed'; + } + return 'Peer review verification failed'; +} + async function main(): Promise { /* eslint-disable @typescript-eslint/naming-convention -- CLI uses kebab-case argument names */ const cli = new CLI({ @@ -34,15 +183,52 @@ async function main(): Promise { const pullRequestNumber = cli.namedArgs['pull-request-number']; const baseRef = cli.namedArgs['base-ref']; - // TODO: replace this no-op with the real independent-peer-review check. - console.log(`Verify peer review skeleton invoked for ${owner}/${repo}#${pullRequestNumber} (base: ${baseRef}). Always passes for now.`); + const [requiredApprovingReviewCount, approvers, commits] = await Promise.all([ + GitHubUtils.getRequiredApprovingReviewCount({owner, repo, baseRef}), + GitHubUtils.getLatestApprovers({owner, repo, number: pullRequestNumber}), + GitHubUtils.listPullRequestCommits({ + owner, + repo, + number: pullRequestNumber, + }), + ]); + + const employeeLogins = requiredApprovingReviewCount > 0 ? await GitHubUtils.getEmployeeLogins() : new Set(); + const {authors, unresolvedExpensifyCoAuthors} = getCommitAuthors(commits, employeeLogins); + + const result = evaluatePeerReview({ + owner, + repo, + number: pullRequestNumber, + baseRef, + requiredApprovingReviewCount, + approvers, + authors, + unresolvedExpensifyCoAuthors, + employeeLogins, + }); + + if (result.status === 'skip' || result.status === 'pass') { + console.log(result.reason); + return; + } + + throw result.error; } -export default {main}; +export type {PeerReviewInput, PeerReviewResult}; + +export default { + main, + evaluatePeerReview, + getIndependentEmployeeApprovers, + getCommitAuthors, + getFailureTitle, +}; if (import.meta.main) { - main().catch((error: unknown) => { - console.error(error instanceof Error ? error.message : String(error)); - process.exit(1); + main().catch((error) => { + const message = error instanceof Error ? error.message : String(error); + GitHubWorkflowUtils.emitFailure(error, getFailureTitle(message)); }); } diff --git a/tests/CollectionUtils.test.ts b/tests/CollectionUtils.test.ts new file mode 100644 index 0000000..2bbf785 --- /dev/null +++ b/tests/CollectionUtils.test.ts @@ -0,0 +1,10 @@ +import assert from 'node:assert/strict'; +import {describe, it} from 'node:test'; + +import CollectionUtils from '../scripts/libs/CollectionUtils'; + +describe('uniqueSorted', () => { + it('deduplicates and sorts values', () => { + assert.deepEqual(CollectionUtils.uniqueSorted(['b', 'a', 'b', 'c']), ['a', 'b', 'c']); + }); +}); diff --git a/tests/GitCommitUtils.test.ts b/tests/GitCommitUtils.test.ts new file mode 100644 index 0000000..fdda2a1 --- /dev/null +++ b/tests/GitCommitUtils.test.ts @@ -0,0 +1,103 @@ +import assert from 'node:assert/strict'; +import {describe, it} from 'node:test'; + +import GitCommitUtils, {type GitHubPullRequestCommit} from '../scripts/libs/GitCommitUtils'; + +function makeCommit(authorLogin: string | undefined, authorName: string | undefined, message: string): GitHubPullRequestCommit { + return { + author: authorLogin ? {login: authorLogin} : null, + commit: { + message, + author: authorName ? {name: authorName} : {}, + }, + }; +} + +describe('resolveNoreplyEmailToLogin', () => { + it('parses standard noreply addresses', () => { + assert.equal(GitCommitUtils.resolveNoreplyEmailToLogin('AndrewGable@users.noreply.github.com'), 'AndrewGable'); + }); + + it('parses numeric noreply prefixes', () => { + assert.equal(GitCommitUtils.resolveNoreplyEmailToLogin('2838819+AndrewGable@users.noreply.github.com'), 'AndrewGable'); + }); +}); + +describe('parseCoAuthorEmails', () => { + it('extracts multiple co-author emails', () => { + const message = [ + 'Some change', + '', + 'Co-authored-by: Andrew Gable ', + 'Co-authored-by: Monil Bhavsar ', + ].join('\n'); + + assert.deepEqual(GitCommitUtils.parseCoAuthorEmails(message), ['AndrewGable@users.noreply.github.com', 'MonilBhavsar@users.noreply.github.com']); + }); +}); + +describe('parseCoAuthors', () => { + it('extracts display names and emails', () => { + const message = 'Change\n\nCo-authored-by: Andrew Gable '; + + assert.deepEqual(GitCommitUtils.parseCoAuthors(message), [{displayName: 'Andrew Gable', email: 'andrew@expensify.com'}]); + }); +}); + +describe('resolveDisplayNameToLogin', () => { + it('parses github usernames from display names', () => { + assert.equal(GitCommitUtils.resolveDisplayNameToLogin('AndrewGable'), 'AndrewGable'); + }); + + it('removes spaces from display names', () => { + assert.equal(GitCommitUtils.resolveDisplayNameToLogin('Andrew Gable'), 'AndrewGable'); + }); + + it('returns null when display name cannot map to a github username', () => { + assert.equal(GitCommitUtils.resolveDisplayNameToLogin("John O'Brien"), null); + }); +}); + +describe('resolveCoAuthorToLogin', () => { + it('prefers noreply email resolution over display name', () => { + assert.equal( + GitCommitUtils.resolveCoAuthorToLogin({ + displayName: 'Wrong Name', + email: 'AndrewGable@users.noreply.github.com', + }), + 'AndrewGable', + ); + }); + + it('falls back to display name when email cannot be resolved', () => { + assert.equal( + GitCommitUtils.resolveCoAuthorToLogin({ + displayName: 'Andrew Gable', + email: 'andrew@expensify.com', + }), + 'AndrewGable', + ); + }); + + it('rejects display names that do not match allowed logins', () => { + assert.equal(GitCommitUtils.resolveCoAuthorToLogin({displayName: 'John Smith', email: 'andrew@expensify.com'}, new Set(['AndrewGable'])), null); + }); + + it('uses canonical allowed login casing', () => { + assert.equal(GitCommitUtils.resolveCoAuthorToLogin({displayName: 'andrew gable', email: 'andrew@expensify.com'}, new Set(['AndrewGable'])), 'AndrewGable'); + }); +}); + +describe('getCanonicalAuthorLogin', () => { + it('returns github author login when present', () => { + assert.equal(GitCommitUtils.getCanonicalAuthorLogin(makeCommit('AndrewGable', undefined, 'Change')), 'AndrewGable'); + }); + + it('falls back to commit author name when github login is missing', () => { + assert.equal(GitCommitUtils.getCanonicalAuthorLogin(makeCommit(undefined, 'AndrewGable', 'Change')), 'AndrewGable'); + }); + + it('throws when canonical author cannot be resolved', () => { + assert.throws(() => GitCommitUtils.getCanonicalAuthorLogin(makeCommit(undefined, undefined, 'Change')), /Unable to resolve canonical commit author/); + }); +}); diff --git a/tests/GitHubUtils.test.ts b/tests/GitHubUtils.test.ts new file mode 100644 index 0000000..e7dc538 --- /dev/null +++ b/tests/GitHubUtils.test.ts @@ -0,0 +1,65 @@ +import assert from 'node:assert/strict'; +import {describe, it, afterEach} from 'node:test'; + +import {RequestError} from '@octokit/request-error'; + +import GitHubAPIClient from '../scripts/libs/GitHubAPIClient'; +import GitHubUtils from '../scripts/libs/GitHubUtils'; + +const context = { + owner: 'Expensify', + repo: 'Auth', + number: 1, + baseRef: 'main', +}; + +describe('getRequiredApprovingReviewCount', () => { + afterEach(() => { + GitHubAPIClient.internalOctokit = undefined; + GitHubAPIClient.graphqlClient = undefined; + }); + + it('returns 0 when branch protection rule is missing', async () => { + GitHubAPIClient.graphqlClient = async () => ({ + repository: { + ref: { + branchProtectionRule: null, + }, + }, + }); + + const count = await GitHubUtils.getRequiredApprovingReviewCount({ + ...context, + baseRef: 'staging', + }); + assert.equal(count, 0); + }); + + it('throws on permission errors', async () => { + GitHubAPIClient.graphqlClient = async () => { + throw new RequestError('Resource not accessible by integration', 403, { + request: { + method: 'POST', + url: 'https://api.github.com/graphql', + headers: {}, + }, + }); + }; + + await assert.rejects(() => GitHubUtils.getRequiredApprovingReviewCount(context), /Unable to read branch protection rules/); + }); +}); + +describe('isBotUser', () => { + it('returns true for GitHub App bot accounts', () => { + assert.equal(GitHubUtils.isBotUser('dependabot[bot]'), true); + }); + + it('returns true for known Expensify bot accounts', () => { + assert.equal(GitHubUtils.isBotUser('MelvinBot'), true); + }); + + it('returns false for human accounts', () => { + assert.equal(GitHubUtils.isBotUser('AndrewGable'), false); + }); +}); diff --git a/tests/GitHubWorkflowUtils.test.ts b/tests/GitHubWorkflowUtils.test.ts new file mode 100644 index 0000000..4db6fd5 --- /dev/null +++ b/tests/GitHubWorkflowUtils.test.ts @@ -0,0 +1,14 @@ +import assert from 'node:assert/strict'; +import {describe, it} from 'node:test'; + +import GitHubWorkflowUtils from '../scripts/libs/GitHubWorkflowUtils'; + +describe('GitHubWorkflowUtils helpers', () => { + it('escapes workflow command values', () => { + assert.equal(GitHubWorkflowUtils.escapeWorkflowCommandValue('a\nb%'), 'a%0Ab%25'); + }); + + it('escapes workflow command properties', () => { + assert.equal(GitHubWorkflowUtils.escapeWorkflowCommandProperty('title:one,two'), 'title%3Aone%2Ctwo'); + }); +}); diff --git a/tests/verifyPeerReviewCli.test.ts b/tests/verifyPeerReviewCli.test.ts index b906c13..f47b343 100644 --- a/tests/verifyPeerReviewCli.test.ts +++ b/tests/verifyPeerReviewCli.test.ts @@ -1,12 +1,16 @@ import assert from 'node:assert/strict'; import {afterEach, beforeEach, describe, it} from 'node:test'; +import GitHubUtils from '../scripts/libs/GitHubUtils'; import VerifyPeerReview from '../scripts/verifyPeerReview'; const ORIGINAL_ARGV = process.argv; describe('main CLI parsing', () => { let originalExit: typeof process.exit; + let originalGetRequiredApprovingReviewCount: typeof GitHubUtils.getRequiredApprovingReviewCount; + let originalGetLatestApprovers: typeof GitHubUtils.getLatestApprovers; + let originalListPullRequestCommits: typeof GitHubUtils.listPullRequestCommits; beforeEach(() => { process.argv = ['tsx', 'scripts/verifyPeerReview.ts']; @@ -14,11 +18,20 @@ describe('main CLI parsing', () => { process.exit = (code?: string | number | null) => { throw new Error(`exit ${code ?? 0}`); }; + originalGetRequiredApprovingReviewCount = GitHubUtils.getRequiredApprovingReviewCount; + originalGetLatestApprovers = GitHubUtils.getLatestApprovers; + originalListPullRequestCommits = GitHubUtils.listPullRequestCommits; + GitHubUtils.getRequiredApprovingReviewCount = async () => 0; + GitHubUtils.getLatestApprovers = async () => []; + GitHubUtils.listPullRequestCommits = async () => []; }); afterEach(() => { process.argv = ORIGINAL_ARGV; process.exit = originalExit; + GitHubUtils.getRequiredApprovingReviewCount = originalGetRequiredApprovingReviewCount; + GitHubUtils.getLatestApprovers = originalGetLatestApprovers; + GitHubUtils.listPullRequestCommits = originalListPullRequestCommits; }); it('parses required pull request CLI arguments', async () => { diff --git a/tests/verifyPeerReviewCoAuthors.test.ts b/tests/verifyPeerReviewCoAuthors.test.ts new file mode 100644 index 0000000..0c774dd --- /dev/null +++ b/tests/verifyPeerReviewCoAuthors.test.ts @@ -0,0 +1,64 @@ +import assert from 'node:assert/strict'; +import {describe, it} from 'node:test'; + +import type {GitHubPullRequestCommit} from '../scripts/libs/GitCommitUtils'; +import VerifyPeerReview from '../scripts/verifyPeerReview'; + +function makeCommit(authorLogin: string | undefined, authorName: string | undefined, message: string): GitHubPullRequestCommit { + return { + author: authorLogin ? {login: authorLogin} : null, + commit: { + message, + author: authorName ? {name: authorName} : {}, + }, + }; +} + +const EMPLOYEE_LOGINS = new Set(['AndrewGable', 'MelvinBot', 'rafecolton']); + +describe('getCommitAuthors', () => { + it('counts co-authors for bot-authored commits', () => { + const result = VerifyPeerReview.getCommitAuthors( + [makeCommit('MelvinBot', undefined, 'Change\n\nCo-authored-by: Andrew Gable ')], + EMPLOYEE_LOGINS, + ); + + assert.deepEqual(result.authors, ['AndrewGable', 'MelvinBot']); + assert.deepEqual(result.unresolvedExpensifyCoAuthors, []); + }); + + it('ignores co-authors when canonical author is human', () => { + const result = VerifyPeerReview.getCommitAuthors([makeCommit('rafecolton', undefined, 'Change\n\nCo-authored-by: Andrew Gable ')]); + + assert.deepEqual(result.authors, ['rafecolton']); + }); + + it('falls back to commit author name when github login is missing', () => { + const result = VerifyPeerReview.getCommitAuthors([makeCommit(undefined, 'AndrewGable', 'Change')]); + + assert.deepEqual(result.authors, ['AndrewGable']); + }); + + it('normalizes expensify email casing and whitespace for unresolved detection', () => { + const result = VerifyPeerReview.getCommitAuthors([makeCommit('MelvinBot', undefined, 'Change\n\nCo-authored-by: John Smith < Andrew@Expensify.com >')], EMPLOYEE_LOGINS); + + assert.deepEqual(result.unresolvedExpensifyCoAuthors, ['Andrew@Expensify.com']); + }); + + it('resolves expensify co-authors from display name when email cannot be mapped', () => { + const result = VerifyPeerReview.getCommitAuthors([makeCommit('MelvinBot', undefined, 'Change\n\nCo-authored-by: Andrew Gable ')], EMPLOYEE_LOGINS); + + assert.deepEqual(result.authors, ['AndrewGable', 'MelvinBot']); + assert.deepEqual(result.unresolvedExpensifyCoAuthors, []); + }); + + it('collects unresolved expensify co-author emails when display name cannot be mapped', () => { + const result = VerifyPeerReview.getCommitAuthors([makeCommit('MelvinBot', undefined, 'Change\n\nCo-authored-by: John Smith ')], EMPLOYEE_LOGINS); + + assert.deepEqual(result.unresolvedExpensifyCoAuthors, ['andrew@expensify.com']); + }); + + it('throws when canonical author cannot be resolved', () => { + assert.throws(() => VerifyPeerReview.getCommitAuthors([makeCommit(undefined, undefined, 'Change')]), /Unable to resolve canonical commit author/); + }); +}); diff --git a/tests/verifyPeerReviewPolicy.test.ts b/tests/verifyPeerReviewPolicy.test.ts new file mode 100644 index 0000000..365ea6f --- /dev/null +++ b/tests/verifyPeerReviewPolicy.test.ts @@ -0,0 +1,139 @@ +import assert from 'node:assert/strict'; +import {describe, it} from 'node:test'; + +import VerifyPeerReview, {type PeerReviewInput} from '../scripts/verifyPeerReview'; + +const baseInput: PeerReviewInput = { + owner: 'Expensify', + repo: 'Auth', + number: 21136, + baseRef: 'main', + requiredApprovingReviewCount: 1, + approvers: [], + authors: [], + unresolvedExpensifyCoAuthors: [], + employeeLogins: new Set(['MonilBhavsar', 'AndrewGable', 'rafecolton']), +}; + +describe('getIndependentEmployeeApprovers', () => { + it('excludes commit authors and non-employees', () => { + const independent = VerifyPeerReview.getIndependentEmployeeApprovers(['AndrewGable', 'MonilBhavsar', 'externalCollab'], ['AndrewGable'], new Set(['AndrewGable', 'MonilBhavsar'])); + + assert.deepEqual(independent, ['MonilBhavsar']); + }); + + it('matches employee logins case-insensitively', () => { + const independent = VerifyPeerReview.getIndependentEmployeeApprovers(['monilbhavsar'], ['AndrewGable'], new Set(['MonilBhavsar'])); + + assert.deepEqual(independent, ['MonilBhavsar']); + }); + + it('excludes commit authors case-insensitively', () => { + const independent = VerifyPeerReview.getIndependentEmployeeApprovers(['andrewgable'], ['AndrewGable'], new Set(['AndrewGable'])); + + assert.deepEqual(independent, []); + }); +}); + +describe('evaluatePeerReview', () => { + it('skips when branch requires no approving reviews', () => { + const result = VerifyPeerReview.evaluatePeerReview({ + ...baseInput, + requiredApprovingReviewCount: 0, + approvers: ['AndrewGable'], + authors: ['AndrewGable'], + }); + + assert.equal(result.status, 'skip'); + }); + + it('skips when there are no approving reviews yet', () => { + const result = VerifyPeerReview.evaluatePeerReview({ + ...baseInput, + approvers: [], + authors: ['MelvinBot', 'AndrewGable'], + }); + + assert.equal(result.status, 'skip'); + }); + + it('fails on self-review from commit co-author', () => { + const result = VerifyPeerReview.evaluatePeerReview({ + ...baseInput, + approvers: ['AndrewGable'], + authors: ['MelvinBot', 'AndrewGable'], + }); + + assert.equal(result.status, 'fail'); + if (result.status === 'fail') { + assert.match(result.error.message, /does not have enough independent Expensify employee approvals/); + } + }); + + it('passes when an independent employee approves', () => { + const result = VerifyPeerReview.evaluatePeerReview({ + ...baseInput, + approvers: ['MonilBhavsar', 'AndrewGable'], + authors: ['MelvinBot', 'AndrewGable'], + }); + + assert.equal(result.status, 'pass'); + }); + + it('fails when independent approver count is below required', () => { + const result = VerifyPeerReview.evaluatePeerReview({ + ...baseInput, + requiredApprovingReviewCount: 2, + approvers: ['MonilBhavsar', 'AndrewGable'], + authors: ['MelvinBot', 'AndrewGable'], + }); + + assert.equal(result.status, 'fail'); + }); + + it('passes when independent approver count meets required', () => { + const result = VerifyPeerReview.evaluatePeerReview({ + ...baseInput, + requiredApprovingReviewCount: 2, + approvers: ['MonilBhavsar', 'rafecolton'], + authors: ['MelvinBot', 'AndrewGable'], + }); + + assert.equal(result.status, 'pass'); + }); + + it('fails when all authors are bots', () => { + const result = VerifyPeerReview.evaluatePeerReview({ + ...baseInput, + approvers: ['AndrewGable'], + authors: ['MelvinBot'], + }); + + assert.equal(result.status, 'fail'); + if (result.status === 'fail') { + assert.match(result.error.message, /no human commit authors or co-authors/); + } + }); + + it('fails on unresolved expensify co-author emails', () => { + const result = VerifyPeerReview.evaluatePeerReview({ + ...baseInput, + approvers: ['AndrewGable'], + authors: ['MelvinBot'], + unresolvedExpensifyCoAuthors: ['andrew@expensify.com'], + }); + + assert.equal(result.status, 'fail'); + if (result.status === 'fail') { + assert.match(result.error.message, /Unable to resolve Expensify co-author emails/); + } + }); +}); + +describe('getFailureTitle', () => { + it('maps failure titles for known messages', () => { + assert.equal(VerifyPeerReview.getFailureTitle('Unable to resolve canonical commit author: missing GitHub author login and commit author name.'), 'Missing commit author'); + assert.equal(VerifyPeerReview.getFailureTitle('Expensify/Auth#1 does not have enough independent Expensify employee approvals.'), 'Missing independent peer review'); + assert.equal(VerifyPeerReview.getFailureTitle('Unable to read branch protection rules for Expensify/Auth@main.'), 'Branch protection lookup failed'); + }); +});