Skip to content

Commit 3199389

Browse files
fix(github): compare the bot login case-insensitively when dismissing its approval (#6614) (#6662)
dismissLatestBotApproval matched review.user.login against `${GITHUB_APP_SLUG}[bot]` with a case-sensitive ===, unlike every other bot-login check in this subsystem (isLoopOverBotComment, normalizeGitHubSlug/isBotActor), which all lowercase first. GitHub's canonical login casing need not match however GITHUB_APP_SLUG is configured. On a mismatch this found nothing and returned { dismissed: false } -- a silent no-op, no error, leaving a stale bot approval standing for a review-evasion or re-review flow that expects it gone. Scoped to the login comparison only: pagination, latest-across-pages selection, the best-effort try/catch and the dismissal write are untouched. Closes #6614 Co-authored-by: luciferlive112116 <291889058+luciferlive112116@users.noreply.github.com>
1 parent e6ef456 commit 3199389

2 files changed

Lines changed: 52 additions & 2 deletions

File tree

src/github/pr-actions.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,15 +132,20 @@ export async function dismissLatestBotApproval(env: Env, installationId: number,
132132
const { owner, repo } = splitRepo(repoFullName);
133133
return await withInstallationTokenRetry(env, installationId, async (token) => {
134134
const octokit = makeInstallationOctokit(env, token, "live", githubRateLimitAdmissionKeyForInstallation(installationId));
135-
const botLogin = `${env.GITHUB_APP_SLUG}[bot]`;
135+
// Compared case-INSENSITIVELY, like every other bot-login check in this subsystem
136+
// (isLoopOverBotComment in comments.ts:104, normalizeGitHubSlug/isBotActor in self-authored.ts). GitHub's
137+
// canonical casing for the login need not match however GITHUB_APP_SLUG happens to be configured, and a
138+
// case-sensitive === would degrade to a silent no-op: no match, no error, a stale bot approval simply
139+
// never dismissed (#6614).
140+
const botLogin = `${env.GITHUB_APP_SLUG}[bot]`.toLowerCase();
136141
// Reviews are returned oldest-first; the LAST matching entry across ALL pages is the bot's most recent
137142
// APPROVE. Stopping at page 1 would find (or miss) the wrong review on a PR with >100 total reviews.
138143
let latestApprovalId: number | undefined;
139144
for (let page = 1; page <= REVIEW_PAGE_LIMIT; page += 1) {
140145
const response = await octokit.request("GET /repos/{owner}/{repo}/pulls/{pull_number}/reviews", { owner, repo, pull_number: pullNumber, per_page: REVIEW_PAGE_SIZE, page });
141146
const batch = response.data as Array<{ id: number; state?: string; user?: { login?: string | null } | null }>;
142147
for (const review of batch) {
143-
if (review.user?.login === botLogin && review.state === "APPROVED") latestApprovalId = review.id;
148+
if (review.user?.login?.toLowerCase() === botLogin && review.state === "APPROVED") latestApprovalId = review.id;
144149
}
145150
if (batch.length < REVIEW_PAGE_SIZE) break;
146151
}

test/unit/github-pr-actions.test.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -567,6 +567,51 @@ describe("GitHub PR action primitives (#778)", () => {
567567
expect(calls[0]?.body).toMatchObject({ message: "stale approval retracted", event: "DISMISS" });
568568
});
569569

570+
it("dismisses the bot's approve review when GitHub returns a different login casing than GITHUB_APP_SLUG (#6614)", async () => {
571+
// The regression: `Gittensory[bot]` vs the default GITHUB_APP_SLUG of `gittensory` matched nothing under
572+
// the old case-sensitive ===, so this returned { dismissed: false } — a SILENT no-op, no error, leaving a
573+
// stale bot approval standing. The human reviewer whose login differs only in case must still be ignored.
574+
const calls: Array<{ url: string; body: Record<string, unknown> }> = [];
575+
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
576+
const url = input.toString();
577+
const method = init?.method ?? "GET";
578+
if (url.includes("/access_tokens")) return Response.json({ token: "t" });
579+
if (url.includes("/pulls/11/reviews") && !url.includes("/dismissals") && method === "GET") {
580+
return Response.json([
581+
{ id: 1, state: "APPROVED", user: { login: "Human-Reviewer" } },
582+
{ id: 2, state: "APPROVED", user: { login: "Gittensory[bot]" } }, // an EARLIER bot approve, mixed case
583+
{ id: 3, state: "CHANGES_REQUESTED", user: { login: "GITTENSORY[BOT]" } },
584+
{ id: 4, state: "APPROVED", user: { login: "GitTensory[Bot]" } }, // the LATEST bot approve — this one
585+
]);
586+
}
587+
if (url.includes("/pulls/11/reviews/4/dismissals") && method === "PUT") {
588+
calls.push({ url, body: init?.body ? JSON.parse(String(init.body)) : {} });
589+
return Response.json({ id: 4, state: "DISMISSED" });
590+
}
591+
return new Response("unexpected", { status: 500 });
592+
});
593+
const result = await dismissLatestBotApproval(envWithKey(), 123, "owner/repo", 11, "stale approval retracted");
594+
expect(result).toEqual({ dismissed: true });
595+
expect(calls).toHaveLength(1); // the mixed-case human's approve (id 1) was never dismissed
596+
});
597+
598+
it("still ignores a review whose author is missing a login entirely (#6614)", async () => {
599+
// The optional-chain's nullish side: `review.user?.login?.toLowerCase()` must not throw on a null author
600+
// (a ghosted/deleted account) and must not match the bot.
601+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
602+
const url = input.toString();
603+
if (url.includes("/access_tokens")) return Response.json({ token: "t" });
604+
if (url.includes("/pulls/12/reviews")) {
605+
return Response.json([
606+
{ id: 1, state: "APPROVED", user: null },
607+
{ id: 2, state: "APPROVED", user: { login: null } },
608+
]);
609+
}
610+
return new Response("unexpected", { status: 500 });
611+
});
612+
await expect(dismissLatestBotApproval(envWithKey(), 123, "owner/repo", 12, "retract")).resolves.toEqual({ dismissed: false });
613+
});
614+
570615
it("is a no-op when the bot never approved this PR", async () => {
571616
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
572617
const url = input.toString();

0 commit comments

Comments
 (0)