Don't let self-authored PRs break the BCR PR reviewer (#2904)
Fixes #2885
GitHub doesn't let you review your own PR. So when a BCR PR was opened
by @bazel-io (e.g. bazelbuild/bazel-central-registry#10700), the
`APPROVE` `createReview` call in `reviewPR` threw. The error also
escaped `runPrReviewer` and stopped the whole `review_prs` loop, which
meant no other open PRs got auto-reviewed either (e.g.
bazelbuild/bazel-central-registry#10699).
### Changes
- `reviewPR`: when the PR author is the authenticated bot account, the
reviewer doesn't call `createReview` (it skips both `APPROVE` and
`REQUEST_CHANGES`). If every module has its maintainers' approval, it
posts a comment (only once) and requests a review from `bcr-maintainers`
instead. It still tries to merge, so the PR gets merged on a later run
once a BCR maintainer approves.
- `runPrReviewer`: errors are caught per PR, so a failure on one PR no
longer blocks the rest. If any PR failed, the run is marked failed at
the end (`setFailed`) so the errors still show up.
- Export `reviewPR`/`runPrReviewer` and add unit tests for: the normal
approve+merge path, a self-authored PR, and one PR failing while the
loop keeps going.
### Testing
`cd actions/bcr-pr-reviewer && node --test`: all 12 tests pass.
Note: BCR workflows pin this action by SHA, so a follow-up BCR PR that
bumps the SHA is needed for this to take effect.
diff --git a/actions/bcr-pr-reviewer/index.js b/actions/bcr-pr-reviewer/index.js
index 543a38f..b0a3675 100644
--- a/actions/bcr-pr-reviewer/index.js
+++ b/actions/bcr-pr-reviewer/index.js
@@ -517,10 +517,18 @@
const { data } = await octokit.rest.users.getAuthenticated();
const myLogin = data.login.toLowerCase();
+ // GitHub doesn't allow reviewing your own PR, so PRs opened by the bot itself
+ // (e.g. @bazel-io) must be approved by a BCR maintainer instead.
+ const isSelfAuthored = prAuthor === myLogin;
// Approve the PR if not previously approved and all modules are approved
if (allModulesApproved && !hasSensitiveMetadataChange) {
- if (!approvers.has(myLogin)) {
+ if (isSelfAuthored) {
+ console.log(`Skipping approval: PR #${prNumber} was opened by ${myLogin}, which cannot approve its own PR.`);
+ await postComment(octokit, owner, repo, prNumber,
+ `Hello BCR maintainers, all modules in this PR have been approved by their maintainers, but this PR was opened by @${myLogin} and cannot be approved by itself. Please approve this PR so it can be merged.`);
+ await requestBcrMaintainers(octokit, owner, repo, prNumber);
+ } else if (!approvers.has(myLogin)) {
console.log('Approving the PR');
await octokit.rest.pulls.createReview({
owner,
@@ -589,7 +597,7 @@
}
// Discard previous approvals if not all modules are approved
- if (!allModulesApproved && approvers.has(myLogin)) {
+ if (!allModulesApproved && !isSelfAuthored && approvers.has(myLogin)) {
console.log('Discarding previous approval');
await octokit.rest.pulls.createReview({
owner,
@@ -714,6 +722,7 @@
});
const sixHoursAgo = new Date(Date.now() - 6 * 60 * 60 * 1000);
+ const failedPrs = [];
// Review each PR
for (const pr of prs) {
@@ -745,11 +754,21 @@
}
if (isOughtToBeReviewed) {
- await reviewPR(octokit, owner, repo, pr.number);
+ try {
+ await reviewPR(octokit, owner, repo, pr.number);
+ } catch (error) {
+ // Don't let a single PR stop the review of all other PRs.
+ console.error(`Failed to review PR #${pr.number}: ${error.message}`);
+ failedPrs.push(pr.number);
+ }
} else {
console.log(`Skipping PR #${pr.number} as it has no activity in the past 6 hours.`);
}
}
+
+ if (failedPrs.length > 0) {
+ setFailed(`Failed to review PRs: ${failedPrs.map(n => `#${n}`).join(', ')}`);
+ }
}
async function runDismissApproval(octokit) {
@@ -1078,4 +1097,4 @@
});
}
-module.exports = { getPrApprovers, moduleVersionDir };
+module.exports = { getPrApprovers, moduleVersionDir, reviewPR, runPrReviewer };
diff --git a/actions/bcr-pr-reviewer/index.test.js b/actions/bcr-pr-reviewer/index.test.js
index 61870e9..f449154 100644
--- a/actions/bcr-pr-reviewer/index.test.js
+++ b/actions/bcr-pr-reviewer/index.test.js
@@ -1,6 +1,6 @@
const test = require('node:test');
const assert = require('node:assert/strict');
-const { getPrApprovers, moduleVersionDir } = require('./index.js');
+const { getPrApprovers, moduleVersionDir, reviewPR, runPrReviewer } = require('./index.js');
function fakeOctokit({ commits, reviews }) {
return {
@@ -122,3 +122,123 @@
assert.equal(moduleVersionDir('rules_cc', '.'), null);
assert.equal(moduleVersionDir('rules_cc', ''), null);
});
+
+// A fake octokit for reviewPR/runPrReviewer on a PR touching one module
+// (`foo`) maintained by `alice`, who has approved the PR at its head commit.
+function fakeReviewOctokit({ prAuthor, prs = [], failingPrs = new Set() }) {
+ const calls = { createReview: [], createComment: [], requestReviewers: [], merge: [] };
+ const listCommits = Symbol('listCommits');
+ // getPrApprovers paginates listReviews while requestBcrMaintainers calls it
+ // directly, so it must be both a paginate marker and callable.
+ const listReviews = async () => ({ data: [] });
+ const listComments = Symbol('listComments');
+ const listPulls = Symbol('listPulls');
+ const prData = (pull_number) => ({
+ number: pull_number,
+ draft: false,
+ changed_files: 1,
+ user: { login: prAuthor, id: 42 },
+ head: { sha: 'HEAD' },
+ labels: [{ name: 'presubmit-auto-run' }],
+ requested_reviewers: [],
+ requested_teams: [],
+ });
+ const octokit = {
+ calls,
+ paginate: async (fn, params) => {
+ if (fn === listCommits) return [{ sha: 'HEAD' }];
+ if (fn === listReviews) {
+ return [{ user: { login: 'alice' }, state: 'APPROVED', commit_id: 'HEAD', submitted_at: '2026-09-30T00:00:00Z' }];
+ }
+ if (fn === listComments) return [];
+ if (fn === listPulls) return prs;
+ throw new Error('unexpected paginate call');
+ },
+ request: async (route, params) => {
+ assert.equal(route, 'GET /user/{account_id}');
+ return { data: { login: 'alice' } };
+ },
+ rest: {
+ pulls: {
+ get: async ({ pull_number }) => {
+ if (failingPrs.has(pull_number)) throw new Error(`boom #${pull_number}`);
+ return { data: prData(pull_number) };
+ },
+ list: listPulls,
+ listCommits,
+ listReviews,
+ listFiles: async () => ({ data: [{ filename: 'modules/foo/1.0.0/MODULE.bazel' }] }),
+ createReview: async (params) => {
+ calls.createReview.push(params);
+ if (prAuthor === 'bazel-io') {
+ const error = new Error('Unprocessable Entity: "Can not approve your own pull request"');
+ error.status = 422;
+ throw error;
+ }
+ },
+ requestReviewers: async (params) => { calls.requestReviewers.push(params); },
+ merge: async (params) => { calls.merge.push(params); },
+ },
+ issues: {
+ listComments,
+ createComment: async (params) => { calls.createComment.push(params); },
+ addLabels: async () => {},
+ },
+ repos: {
+ getContent: async ({ path }) => {
+ assert.equal(path, 'modules/foo/metadata.json');
+ const metadata = { maintainers: [{ github: 'alice', github_user_id: 1 }], versions: ['1.0.0'] };
+ return { data: { content: Buffer.from(JSON.stringify(metadata)).toString('base64') } };
+ },
+ },
+ users: {
+ getAuthenticated: async () => ({ data: { login: 'bazel-io' } }),
+ },
+ actions: {
+ listRepoWorkflows: async () => ({ data: { workflows: [{ id: 7, path: '.github/workflows/dismiss_approvals.yml' }] } }),
+ listWorkflowRuns: async () => ({ data: { workflow_runs: [], total_count: 0 } }),
+ },
+ },
+ };
+ return octokit;
+}
+
+test('reviewPR: approves and merges a fully approved PR opened by someone else', async () => {
+ const octokit = fakeReviewOctokit({ prAuthor: 'contributor' });
+
+ await reviewPR(octokit, 'owner', 'repo', 1);
+
+ assert.equal(octokit.calls.createReview.length, 1);
+ assert.equal(octokit.calls.createReview[0].event, 'APPROVE');
+ assert.equal(octokit.calls.merge.length, 1);
+});
+
+test('reviewPR: does not try to approve a PR opened by the bot itself and asks BCR maintainers instead', async () => {
+ const octokit = fakeReviewOctokit({ prAuthor: 'bazel-io' });
+
+ await reviewPR(octokit, 'owner', 'repo', 1);
+
+ assert.deepEqual(octokit.calls.createReview, [], 'GitHub rejects reviews on your own PR');
+ assert.equal(octokit.calls.createComment.length, 1);
+ assert.match(octokit.calls.createComment[0].body, /opened by @bazel-io and cannot be approved by itself/);
+ assert.deepEqual(octokit.calls.requestReviewers.map(r => r.team_reviewers), [['bcr-maintainers']]);
+ // Still attempts the merge, which succeeds once a BCR maintainer has approved.
+ assert.equal(octokit.calls.merge.length, 1);
+});
+
+test('runPrReviewer: a failure on one PR does not stop reviewing the remaining PRs', async () => {
+ const github = require('@actions/github');
+ github.context.payload = { repository: { owner: { login: 'owner' }, name: 'repo' } };
+ const now = new Date().toISOString();
+ const octokit = fakeReviewOctokit({
+ prAuthor: 'contributor',
+ prs: [{ number: 1, updated_at: now }, { number: 2, updated_at: now }],
+ failingPrs: new Set([1]),
+ });
+
+ await runPrReviewer(octokit);
+
+ assert.deepEqual(octokit.calls.merge.map(m => m.pull_number), [2]);
+ assert.equal(process.exitCode, 1, 'the run should still be marked as failed');
+ process.exitCode = 0;
+});
