diff --git a/src/handlers/mention.test.ts b/src/handlers/mention.test.ts index 83fd2dd3..53f06a80 100644 --- a/src/handlers/mention.test.ts +++ b/src/handlers/mention.test.ts @@ -1319,6 +1319,7 @@ describe("createMentionHandler rereview command", () => { base: { ref: "main" }, }, }), + listRequestedReviewers: async () => ({ data: { users: [], teams: [] } }), requestReviewers: async (params: { team_reviewers: string[] }) => { requestedTeams = params.team_reviewers; return { data: {} }; diff --git a/src/handlers/mention.ts b/src/handlers/mention.ts index 9d2e7475..0bf9e937 100644 --- a/src/handlers/mention.ts +++ b/src/handlers/mention.ts @@ -31,6 +31,7 @@ import { buildMentionContext } from "../execution/mention-context.ts"; import { buildMentionPrompt } from "../execution/mention-prompt.ts"; import { classifyError, formatErrorComment, postOrUpdateErrorComment } from "../lib/errors.ts"; import { wrapInDetails } from "../lib/formatting.ts"; +import { requestRereviewTeamBestEffort } from "./rereview-team.ts"; /** * Create the mention handler and register it with the event router. @@ -580,21 +581,15 @@ export function createMentionHandler(deps: { mention.prNumber !== undefined && (normalizedQuestion === "review" || normalizedQuestion === "recheck") ) { - const rereviewTeam = (config.review.uiRereviewTeam ?? "").trim() || "aireview"; - - try { - await octokit.rest.pulls.requestReviewers({ - owner: mention.owner, - repo: mention.repo, - pull_number: mention.prNumber, - team_reviewers: [rereviewTeam], - }); - } catch (err) { - logger.warn( - { err, prNumber: mention.prNumber, rereviewTeam }, - "Failed to request ui rereview team for rereview (best-effort)", - ); - } + const configuredTeam = (config.review.uiRereviewTeam ?? "").trim() || "aireview"; + await requestRereviewTeamBestEffort({ + octokit, + owner: mention.owner, + repo: mention.repo, + prNumber: mention.prNumber, + configuredTeam, + logger, + }); return; } diff --git a/src/handlers/rereview-team.test.ts b/src/handlers/rereview-team.test.ts new file mode 100644 index 00000000..f4218ea1 --- /dev/null +++ b/src/handlers/rereview-team.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, test } from "bun:test"; +import type { Logger } from "pino"; +import { + buildRereviewTeamCandidates, + requestRereviewTeamBestEffort, +} from "./rereview-team.ts"; + +function createNoopLogger(): Logger { + return { + fatal: () => undefined, + error: () => undefined, + warn: () => undefined, + info: () => undefined, + debug: () => undefined, + trace: () => undefined, + silent: () => undefined, + } as unknown as Logger; +} + +describe("rereview-team helpers", () => { + test("buildRereviewTeamCandidates normalizes owner/team input and adds alias", () => { + expect(buildRereviewTeamCandidates("xbmc/aireview")).toEqual(["aireview", "ai-review"]); + expect(buildRereviewTeamCandidates("ai-review")).toEqual(["ai-review", "aireview"]); + }); + + test("requestRereviewTeamBestEffort skips request when team already present", async () => { + let requestCalls = 0; + const result = await requestRereviewTeamBestEffort({ + octokit: { + rest: { + pulls: { + listRequestedReviewers: async () => ({ + data: { users: [], teams: [{ slug: "aireview", name: "aireview" }] }, + }), + requestReviewers: async () => { + requestCalls += 1; + return { data: {} }; + }, + }, + }, + }, + owner: "xbmc", + repo: "kodiai", + prNumber: 1, + configuredTeam: "aireview", + logger: createNoopLogger(), + }); + + expect(result.alreadyRequested).toBe(true); + expect(result.requestedTeam).toBeUndefined(); + expect(requestCalls).toBe(0); + }); + + test("requestRereviewTeamBestEffort falls back from aireview to ai-review on 422", async () => { + const attempted: string[] = []; + + const result = await requestRereviewTeamBestEffort({ + octokit: { + rest: { + pulls: { + listRequestedReviewers: async () => ({ data: { users: [], teams: [] } }), + requestReviewers: async (params: { team_reviewers: string[] }) => { + const slug = params.team_reviewers[0] ?? ""; + attempted.push(slug); + if (slug === "aireview") { + const err = new Error("Validation failed") as Error & { status: number }; + err.status = 422; + throw err; + } + return { data: {} }; + }, + }, + }, + }, + owner: "xbmc", + repo: "kodiai", + prNumber: 2, + configuredTeam: "aireview", + logger: createNoopLogger(), + }); + + expect(attempted).toEqual(["aireview", "ai-review"]); + expect(result.requestedTeam).toBe("ai-review"); + expect(result.alreadyRequested).toBe(false); + }); +}); diff --git a/src/handlers/rereview-team.ts b/src/handlers/rereview-team.ts new file mode 100644 index 00000000..e5e9243d --- /dev/null +++ b/src/handlers/rereview-team.ts @@ -0,0 +1,104 @@ +import type { Logger } from "pino"; + +function normalizeConfiguredTeam(team: string): string { + const trimmed = team.trim().toLowerCase(); + if (trimmed.length === 0) return ""; + const lastSlash = trimmed.lastIndexOf("/"); + return lastSlash >= 0 ? trimmed.slice(lastSlash + 1) : trimmed; +} + +export function buildRereviewTeamCandidates(team: string): string[] { + const primary = normalizeConfiguredTeam(team); + if (primary.length === 0) return []; + + const candidates = new Set([primary]); + if (primary === "ai-review") candidates.add("aireview"); + if (primary === "aireview") candidates.add("ai-review"); + return Array.from(candidates); +} + +function getStatusCode(err: unknown): number | undefined { + if (typeof err !== "object" || err === null) return undefined; + const value = (err as { status?: unknown }).status; + return typeof value === "number" ? value : undefined; +} + +export async function requestRereviewTeamBestEffort(options: { + octokit: { + rest: { + pulls: { + listRequestedReviewers: (params: { + owner: string; + repo: string; + pull_number: number; + }) => Promise<{ data: { teams?: Array<{ slug?: string | null; name?: string | null }> } }>; + requestReviewers: (params: { + owner: string; + repo: string; + pull_number: number; + team_reviewers: string[]; + }) => Promise; + }; + }; + }; + owner: string; + repo: string; + prNumber: number; + configuredTeam: string; + logger: Logger; +}): Promise<{ requestedTeam?: string; alreadyRequested: boolean }> { + const { octokit, owner, repo, prNumber, configuredTeam, logger } = options; + const candidates = buildRereviewTeamCandidates(configuredTeam); + if (candidates.length === 0) return { alreadyRequested: false }; + + try { + const requested = await octokit.rest.pulls.listRequestedReviewers({ + owner, + repo, + pull_number: prNumber, + }); + + const already = (requested.data.teams ?? []).some((team) => { + const slug = (team.slug ?? "").trim().toLowerCase(); + const name = (team.name ?? "").trim().toLowerCase(); + return candidates.includes(slug) || candidates.includes(name); + }); + if (already) return { alreadyRequested: true }; + } catch (err) { + logger.warn( + { err, owner, repo, prNumber, configuredTeam }, + "Failed to list requested reviewers; attempting rereview request anyway", + ); + } + + for (const candidate of candidates) { + try { + await octokit.rest.pulls.requestReviewers({ + owner, + repo, + pull_number: prNumber, + team_reviewers: [candidate], + }); + return { requestedTeam: candidate, alreadyRequested: false }; + } catch (err) { + const status = getStatusCode(err); + const canFallback = status === 422 && candidate !== candidates[candidates.length - 1]; + logger.warn( + { + err, + owner, + repo, + prNumber, + configuredTeam, + candidate, + status, + fallbackRemaining: canFallback, + }, + "Failed to request rereview team candidate", + ); + if (!canFallback) break; + } + } + + return { alreadyRequested: false }; +} diff --git a/src/handlers/review.ts b/src/handlers/review.ts index b4d5aff8..96275af9 100644 --- a/src/handlers/review.ts +++ b/src/handlers/review.ts @@ -15,6 +15,7 @@ import { buildReviewOutputKey, ensureReviewOutputNotPublished, } from "./review-idempotency.ts"; +import { requestRereviewTeamBestEffort } from "./rereview-team.ts"; import { $ } from "bun"; import { fetchAndCheckoutPullRequestHeadRef } from "../jobs/workspace.ts"; @@ -61,50 +62,6 @@ export function createReviewHandler(deps: { const rereviewTeamSlugs = new Set(["ai-review", "aireview"]); - async function ensureUiRereviewTeamRequested(options: { - octokit: Awaited>; - owner: string; - repo: string; - prNumber: number; - team: string; - logger: Logger; - }): Promise { - const { octokit, owner, repo, prNumber, team } = options; - const normalized = team.trim().toLowerCase(); - if (normalized.length === 0) return; - - try { - const requested = await octokit.rest.pulls.listRequestedReviewers({ - owner, - repo, - pull_number: prNumber, - }); - const already = - (requested.data.teams ?? []).some((t) => (t.slug ?? "").trim().toLowerCase() === normalized) || - (requested.data.teams ?? []).some((t) => (t.name ?? "").trim().toLowerCase() === normalized); - if (already) return; - } catch (err) { - logger.warn( - { err, owner, repo, prNumber, team }, - "Failed to list requested reviewers; attempting to request ui rereview team anyway", - ); - } - - try { - await octokit.rest.pulls.requestReviewers({ - owner, - repo, - pull_number: prNumber, - team_reviewers: [team], - }); - } catch (err) { - logger.warn( - { err, owner, repo, prNumber, team }, - "Failed to request ui rereview team (best-effort)", - ); - } - } - async function handleReview(event: WebhookEvent): Promise { const payload = event.payload as unknown as | PullRequestOpenedEvent @@ -333,12 +290,12 @@ export function createReviewHandler(deps: { (action === "opened" || action === "ready_for_review") ) { const octokit = await githubApp.getInstallationOctokit(event.installationId); - await ensureUiRereviewTeamRequested({ + await requestRereviewTeamBestEffort({ octokit, owner: apiOwner, repo: apiRepo, prNumber: pr.number, - team: config.review.uiRereviewTeam, + configuredTeam: config.review.uiRereviewTeam, logger, }); }