Skip to content

Commit 665cb9a

Browse files
authored
Honor greenlight App bot approvals in pytorchbot merge gate (#8543)
**Impact:** pytorch/pytorch merge flow **Risk:** low ## What `getApprovalStatus` now honors approvals from an allowlisted set of GitHub App bots (currently `pytorchgreenlight[bot]`), scoped strictly to pytorch/pytorch. All other approver logic is unchanged. ## Why GitHub App bots authenticate via installations, not as repo collaborators, so their reviews always carry `author_association=NONE` and were silently dropped by the existing `ALLOWED_APPROVER_ASSOCIATIONS` filter. The greenlight auto-land bot posts real APPROVED reviews that need to count toward the merge gate. The exemption is gated on `isPyTorchPyTorch(owner, repo)` so the same bot login cannot be honored on any other supported org/repo. # Notes - Dismissed reviews from the bot still correctly revoke the approval (covered by tests). - Adding a new trusted App means appending to `ALLOWED_APPROVER_BOT_LOGINS`. Signed-off-by: Jean Schmidt <contato@jschmidt.me>
1 parent 0316d9f commit 665cb9a

2 files changed

Lines changed: 91 additions & 0 deletions

File tree

torchci/lib/bot/pytorchBotHandler.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,14 @@ The explanation needs to be clear on why this is needed. Here are some good exam
187187
"OWNER",
188188
];
189189

190+
// GitHub App bots authenticate via installations rather than as repo
191+
// collaborators, so their reviews always carry author_association=NONE.
192+
// Allowlist trusted App identities so their approvals are still honored,
193+
// but only on pytorch/pytorch since that is the sole repo these bots
194+
// review -- other supported orgs/repos must not honor the exemption.
195+
const ALLOWED_APPROVER_BOT_LOGINS = ["pytorchgreenlight[bot]"];
196+
const isPyTorchPyTorchRepo = isPyTorchPyTorch(this.owner, this.repo);
197+
190198
// Find the latest review offered by each authroized reviewer
191199
// But first sort them in case Github ever returns the list unsorted
192200
var latest_reviews: { [user: string]: string } = reviews
@@ -203,6 +211,12 @@ The explanation needs to be clear on why this is needed. Here are some good exam
203211
if (
204212
!ALLOWED_APPROVER_ASSOCIATIONS.includes(
205213
curr_review.author_association
214+
) &&
215+
!(
216+
isPyTorchPyTorchRepo &&
217+
ALLOWED_APPROVER_BOT_LOGINS.includes(
218+
curr_review.user?.login ?? ""
219+
)
206220
)
207221
) {
208222
// Not an authorized approver

torchci/test/mergeCommands.test.ts

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@ import { getFailureMessage, getMessage } from "lib/GeneralUtils";
22
import nock from "nock";
33
import * as probot from "probot";
44
import pytorchBot from "../lib/bot/pytorchBot";
5+
import PytorchBotHandler, {
6+
PytorchbotParams,
7+
} from "../lib/bot/pytorchBotHandler";
58
import * as clickhouse from "../lib/clickhouse";
69
import { handleScope, requireDeepCopy } from "./common";
710
import * as utils from "./utils";
@@ -1614,6 +1617,80 @@ some other text lol
16141617
handleScope(scope);
16151618
});
16161619

1620+
// The greenlight App bot posts reviews with author_association=NONE, so it is
1621+
// only honored as an approver on pytorch/pytorch (see getApprovalStatus).
1622+
const greenlightApprovedReview = {
1623+
user: { login: "pytorchgreenlight[bot]" },
1624+
state: "APPROVED",
1625+
author_association: "NONE",
1626+
submitted_at: "2024-01-01T00:00:00Z",
1627+
};
1628+
1629+
function makeApprovalHandler(
1630+
owner: string,
1631+
repo: string,
1632+
reviews: any[]
1633+
): PytorchBotHandler {
1634+
const params: PytorchbotParams = {
1635+
owner,
1636+
repo,
1637+
prNum: 42,
1638+
ctx: {
1639+
octokit: {
1640+
paginate: jest.fn().mockResolvedValue(reviews),
1641+
pulls: { listReviews: jest.fn() },
1642+
},
1643+
log: jest.fn(),
1644+
},
1645+
url: "https://github.com/pytorch/pytorch/pull/42#issuecomment-123",
1646+
login: "test-user",
1647+
commentId: 123,
1648+
commentBody: "@pytorchbot merge",
1649+
useReactions: false,
1650+
cachedConfigTracker: {} as any,
1651+
};
1652+
return new PytorchBotHandler(params);
1653+
}
1654+
1655+
test("greenlight bot approval (author_association NONE) counts as approved on pytorch/pytorch", async () => {
1656+
const handler = makeApprovalHandler("pytorch", "pytorch", [
1657+
greenlightApprovedReview,
1658+
]);
1659+
expect(await handler.getApprovalStatus()).toBe("approved");
1660+
});
1661+
1662+
test("greenlight bot approval is not honored on a supported-org repo that is not pytorch/pytorch", async () => {
1663+
const handler = makeApprovalHandler("pytorch", "gha-ci-playground", [
1664+
greenlightApprovedReview,
1665+
]);
1666+
expect(await handler.getApprovalStatus()).toBe("");
1667+
});
1668+
1669+
test("non-greenlight approval with author_association NONE is not honored on pytorch/pytorch", async () => {
1670+
const handler = makeApprovalHandler("pytorch", "pytorch", [
1671+
{
1672+
user: { login: "some-random-bot[bot]" },
1673+
state: "APPROVED",
1674+
author_association: "NONE",
1675+
submitted_at: "2024-01-01T00:00:00Z",
1676+
},
1677+
]);
1678+
expect(await handler.getApprovalStatus()).toBe("");
1679+
});
1680+
1681+
test("greenlight bot review dismissed after approval is not honored on pytorch/pytorch", async () => {
1682+
const handler = makeApprovalHandler("pytorch", "pytorch", [
1683+
greenlightApprovedReview,
1684+
{
1685+
user: { login: "pytorchgreenlight[bot]" },
1686+
state: "DISMISSED",
1687+
author_association: "NONE",
1688+
submitted_at: "2024-01-02T00:00:00Z",
1689+
},
1690+
]);
1691+
expect(await handler.getApprovalStatus()).toBe("");
1692+
});
1693+
16171694
test("pytorchmergebot -h rebase command on pull request prints help message and does not execute rebase", async () => {
16181695
const event = requireDeepCopy("./fixtures/pull_request_comment.json");
16191696

0 commit comments

Comments
 (0)