Add independent peer review validation workflow#64
Conversation
| if (requiredApprovingReviewCount === 0) { | ||
| console.log(`${owner}/${repo}#${number} targets ${context.baseRef}, which does not require approving reviews.`); | ||
| return; | ||
| } | ||
| if (approvers.length === 0) { | ||
| console.log(`${owner}/${repo}#${number} has no approving reviews from writers; regular branch protection will block merge until an approval exists.`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Given this will cause the check to pass when they are are no reviews, we might need to run this on pull_request_review events too
roryabraham
changed the title
|
Making this WIP while I work on it |
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Ready for review. Since this was the first PR in this repo adding any JS/TS code, this PR also sets up:
|
| 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.`); |
There was a problem hiding this comment.
are branch protection rules also org wide? Do we need to enable this in app?
There was a problem hiding this comment.
we only read branchprotectionrule but may want to also support rulesets
There was a problem hiding this comment.
are branch protection rules also org wide?
They are not. Branch protection rules are configured per repo AFAIK. That said, a protected main branch is standard in all our repos. In App, staging and prod are protected as well, because pushing to them automatically triggers deploys.
we only read branchprotectionrule but may want to also support rulesets
Stepping back, the purpose of this function is to figure out how many reviews are needed for a given branch. If more than 0 reviews are required by branch protection rules, then we don't allow self-merging. The reason we check is actually to prevent enforcing no self-merging if the base branch is some unprotected feature branch.
With that understanding, it's not clear to me why we would need to check rulesets, since they generally aren't what prevent merging pull requests against protected branches. The only exception I'm aware of is the ruleset for linting GitHub Actions and Workflows, but I'm not sure if that's relevant here.
Let me know if there is something I'm missing or some changes I should make. Happy to discuss further
There was a problem hiding this comment.
A repo that requires reviews through rulesets instead of classic branch protection returns no branchProtectionRule, so the count is 0 and the whole check skips.
this case @roryabraham
Fail loudly when GitHub author login and commit author name are both missing instead of silently continuing with an empty author. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Discovered an existing bug we need to address before merge. Alternatively I can address in a follow-up as long as the ruleset isn't set to enforce yet. It will probably be smart to enable the ruleset in evaluate first anyways to make sure it's working before we start blocking PRs on it |
|
@MelvinBot do a thorough analysis of the logic/queries introduced in this pull request. Compare them to the existing |
Cherry-picked merge commits keep the original merger as git author, which caused false peer-review failures when that person also approved. Expand Merge pull request #N commits to source PR authors and skip merge clickers. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Melvin was too slow, so here's my local agent summary of the differences between this PR and the existing chore. 1. Trigger Timing (Fundamental Difference)
This is by design. The two implementations are complementary: the TS workflow prevents self-merges going forward; the PHP chore catches any that slip through or occurred historically. 2. Merge Commit Handling (TS-only logic)The TS version has two layers of merge commit handling that the PHP chore is entirely missing. a) Skips bare git merge commits (commits with 2+ parents): if (GitCommitUtils.isGitMergeCommit(commit)) {
continue;
}b) Recursively follows "Merge pull request #N" messages — when a commit message begins with This handles staging/deploy-style PRs that land feature PRs via merge commits. Without this logic, all authors from every merged commit would be counted as authors of the deploy PR, making true "independence" nearly impossible to satisfy. The PHP chore has no equivalent. On a deploy-style PR it would count every author whose commits appear in the PR's commit list. 3. Employee Detection (Different Sources of Truth)
These sources can diverge. An ex-employee or contractor may exist in one but not the other. 4. Bot User DetectionBoth share the same known-bot set ( // TS only — catches GitHub App bots (e.g. "dependabot[bot]", "some-app[bot]")
if (login.endsWith('[bot]')) {
return true;
}The PHP version would not recognise GitHub App bots and could incorrectly treat them as human commit authors. 5. Co-Author Resolution (Different Fallback Strategies)Both parse the same noreply-email pattern (
The PHP fallback is more accurate when the Whitelist is current; the TS display-name fallback is more fragile (succeeds only if the display name matches a GitHub login exactly, or with whitespace stripped) but requires no internal data source. 6. "No Approvers" Case
This difference is intentional. At pre-merge time the TS check defers to branch protection — the PR literally cannot merge without approval, so running the peer-review logic is pointless. Post-merge, the PHP chore still needs to flag the situation. 7. "All-Bot-Authors" Case
Both ultimately flag the PR, but the TS version surfaces it as a distinct, named error category via 8. Unresolvable Canonical Commit Author// PHP: falls back to commit author name, uses empty string if both are missing — commit silently skipped
$authorLogin = $commit['author']['login'] ?? $commit['commit']['author']['name'] ?? '';
if ($authorLogin) {
$authors[] = $authorLogin;
}// TS: throws loudly — check fails with a named error category
throw new Error('Unable to resolve canonical commit author: missing GitHub author login and commit author name.');9. Branch Protection Error Handling
The TS version distinguishes permission errors from transient API errors, surfacing misconfigured app permissions rather than silently proceeding. Summary Table
|
This reverts commit 3a3b3e8.
This difference was reverted. Updating the above comment accordingly |
Resolve README conflict by keeping both verifyPeerReview and setup-composer-cache documentation sections. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Per @rafecolton's request, I'm going to break this up to separate the less-critical CI/infra setup from the main logic of the check, with a series of smaller PRs that should be easier to review. |
Integrate the split peer-review infrastructure PRs (#76-#81) while keeping the branch's verifyPeerReview implementation and tests. Conflict resolution favors main for tooling (Oxfmt, ESLint, CI workflows, pull_request_target workflow) and keeps branch logic for the peer review script, libs, and policy tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Treat these Expensify GitHub App accounts as bots during peer review author resolution. Co-authored-by: Cursor <cursoragent@cursor.com>
| return approvers.filter((approver) => !authorSet.has(approver) && employeeLogins.has(approver)); | ||
| } |
There was a problem hiding this comment.
👎 GH usernames are case sensitive so we do not need to lower case them
Reuse findAllowedLogin when checking independent employee approvals so login casing mismatches do not skip valid reviewers or authors. Co-authored-by: Cursor <cursoragent@cursor.com>
|
|
||
| if (approvers.length === 0) { | ||
| return { | ||
| status: 'skip', |
There was a problem hiding this comment.
Return a fail result instead of skip when approvers is empty
4 participants
Details
Adds a ruleset-compatible workflow that validates pull requests have enough independent Expensify employee approvals before merge. The validator checks PR commits/co-authors, latest approving reviews, Expensify org membership, and repository write permission using Octokit REST APIs.
Related Issues
Prevent merging PRs that aren't peer reviewed
Manual Tests
Ran TypeScript and workflow validation locally:
Smoke tested the validator against real PRs using synthetic
pull_requestpayloads. Note: my local token could not read branch protection settings, so these runs exercised the fallback path requiring one independent approval.Linked PRs
N/A