Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* @supabase/cli

# Files that bots update. Either owner satisfies code-owner review; `supabase-oss` approves only
# bot-opened PRs, through bot-pr-auto-merge.yml.
# bot-opened PRs, through bot-pr-auto-merge.yml and sync-repair.yml.
/.github/workflows/*.yml @supabase/cli @supabase-oss
/package.json @supabase/cli @supabase-oss
/apps/*/package.json @supabase/cli @supabase-oss
Expand Down
27 changes: 27 additions & 0 deletions .github/scripts/ai-review/generate-pr-diff.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,4 +87,31 @@ describe("generatePrDiff", () => {
expect(diff).toContain("+feature");
expect(diff).not.toContain("base-only.txt");
});

test("diffs the pinned head even after the PR branch moves", () => {
const { seed, checkout } = setupRepository();
git(seed, "switch", "-c", "feature");
writeFileSync(join(seed, "reviewed.txt"), "reviewed\n");
git(seed, "add", "reviewed.txt");
git(seed, "commit", "-m", "reviewed change");
publishPullRequest(seed, 88);
const headSha = git(seed, "rev-parse", "HEAD");
writeFileSync(join(seed, "late.txt"), "pushed during the review\n");
git(seed, "add", "late.txt");
git(seed, "commit", "-m", "late change");
publishPullRequest(seed, 88);

const outputPath = join(checkout, "pr.diff");
generatePrDiff({
repositoryPath: checkout,
prNumber: 88,
baseRef: "develop",
outputPath,
headSha,
});

const diff = readFileSync(outputPath, "utf8");
expect(diff).toContain("+reviewed");
expect(diff).not.toContain("late.txt");
});
});
16 changes: 11 additions & 5 deletions .github/scripts/ai-review/generate-pr-diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ export interface GeneratePrDiffOptions {
prNumber: number;
baseRef: string;
outputPath: string;
/** Commit to diff instead of the PR's current head, so a push during the review is not reviewed unseen. */
headSha?: string;
}

function runGit(repositoryPath: string, args: string[]): void {
Expand All @@ -22,19 +24,22 @@ function runGit(repositoryPath: string, args: string[]): void {
}
}

function validateInputs(prNumber: number, baseRef: string): void {
function validateInputs(prNumber: number, baseRef: string, headSha?: string): void {
if (!Number.isSafeInteger(prNumber) || prNumber <= 0) {
throw new Error(`Invalid PR number: ${prNumber}`);
}
if (baseRef.length === 0) {
throw new Error("Base ref must not be empty");
}
if (headSha !== undefined && !/^[0-9a-f]{40}$/.test(headSha)) {
throw new Error(`Invalid head SHA: ${headSha}`);
}
}

export function generatePrDiff(options: GeneratePrDiffOptions): void {
const repositoryPath = resolve(options.repositoryPath);
const outputPath = resolve(options.outputPath);
validateInputs(options.prNumber, options.baseRef);
validateInputs(options.prNumber, options.baseRef, options.headSha);
runGit(repositoryPath, ["check-ref-format", `refs/heads/${options.baseRef}`]);

const baseRef = "refs/ai-review/base";
Expand All @@ -45,7 +50,7 @@ export function generatePrDiff(options: GeneratePrDiffOptions): void {
"--no-tags",
"origin",
`+refs/heads/${options.baseRef}:${baseRef}`,
`+refs/pull/${options.prNumber}/head:${headRef}`,
`+${options.headSha ?? `refs/pull/${options.prNumber}/head`}:${headRef}`,
]);

mkdirSync(dirname(outputPath), { recursive: true });
Expand Down Expand Up @@ -78,14 +83,15 @@ export function generatePrDiff(options: GeneratePrDiffOptions): void {
}

function parseArguments(args: string[]): GeneratePrDiffOptions {
if (args.length !== 2) {
throw new Error("Usage: generate-pr-diff.ts <pr-number> <base-ref>");
if (args.length < 2 || args.length > 3) {
throw new Error("Usage: generate-pr-diff.ts <pr-number> <base-ref> [head-sha]");
}
return {
repositoryPath: process.cwd(),
prNumber: Number(args[0]),
baseRef: args[1] ?? "",
outputPath: "/tmp/ai-review/pr.diff",
headSha: args[2] || undefined,
};
}

Expand Down
59 changes: 52 additions & 7 deletions .github/scripts/ai-review/post-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
type MarkedEntry,
type MergedFinding,
type MergedReview,
parseAiReviewState,
parseDiffAnchors,
partitionFindings,
postConsolidatedReview,
Expand Down Expand Up @@ -535,6 +536,8 @@ describe("renderReviewBody", () => {
trigger: "auto",
runUrl: "https://example.com/run/9",
modelsFooter: "`claude-fable-5` + `gpt-5.6-sol`",
headSha: "a".repeat(40),
reviewsComplete: true,
};

test("shows 'No issues found.' when nothing was posted", () => {
Expand Down Expand Up @@ -672,6 +675,8 @@ describe("buildReviewPayload", () => {
trigger: "manual",
runUrl: "https://example.com/run/1",
modelsFooter: "`claude-fable-5` + `gpt-5.6-sol`",
headSha: "a".repeat(40),
reviewsComplete: true,
};

test("event is always COMMENT", () => {
Expand All @@ -686,6 +691,29 @@ describe("buildReviewPayload", () => {
expect(payload.body).toContain(AI_REVIEW_MARKER);
});

test("posts on the reviewed head and records every unrefuted finding, inline or not", () => {
const review = makeMergedReview({
findings: [
makeFinding({ id: "inline", file: "file.ts", line: 10 }),
makeFinding({ id: "outside", file: "file.ts", line: 99, claim: "x".repeat(70_000) }),
makeFinding({ id: "refuted", adjudication: { verdict: "refuted", reason: "No." } }),
makeFinding({ id: "advisory", category: "next-impact" }),
],
});
const payload = buildReviewPayload(review, anchors, { ...footer, reviewsComplete: false });

expect(payload.commit_id).toBe(footer.headSha);
expect(payload.comments).toHaveLength(1);
expect(payload.body).toContain("truncated");
expect(payload.body).not.toContain(footer.modelsFooter);
expect(payload.body).toContain(AI_REVIEW_MARKER);
expect(parseAiReviewState(payload.body)).toEqual({
head: "a".repeat(40),
complete: false,
findings: 3,
});
});

test("the injected models footer appears in the body verbatim", () => {
const review = makeMergedReview({ findings: [] });
const payload = buildReviewPayload(review, anchors, footer);
Expand Down Expand Up @@ -832,6 +860,8 @@ describe("next-impact findings", () => {
trigger: "auto",
runUrl: "https://example.com/run/2",
modelsFooter: "`claude-fable-5` + `gpt-5.6-sol`",
headSha: "a".repeat(40),
reviewsComplete: true,
};
const anchors = parseDiffAnchors(SINGLE_HUNK_DIFF);

Expand Down Expand Up @@ -883,6 +913,8 @@ describe("foldInlineCommentsIntoBody", () => {
trigger: "manual",
runUrl: "https://example.com/run/1",
modelsFooter: "`claude-fable-5` + `gpt-5.6-sol`",
headSha: "a".repeat(40),
reviewsComplete: true,
};

test("returns the same payload unchanged when there are no inline comments", () => {
Expand Down Expand Up @@ -1047,6 +1079,8 @@ describe("post flow via injected ReviewIo", () => {
trigger: "manual",
runUrl: "https://example.com/run/1",
modelsFooter: "`claude-fable-5` + `gpt-5.6-sol`",
headSha: "a".repeat(40),
reviewsComplete: true,
};

function makeReviewIo(
Expand Down Expand Up @@ -1208,17 +1242,28 @@ describe("post flow via injected ReviewIo", () => {
expect(postedReviews[1]?.body).toContain("Inline comments (GitHub rejected");
});

test("never retries with the fold when there were no inline comments to fold", async () => {
test("retries a rejected pinned review on the current head, still naming the reviewed commit", async () => {
const finding = makeFinding({ file: "file.ts", line: 999 }); // not anchorable -> body-only
const review = makeMergedReview({ findings: [finding] });
const { io, postedReviews } = makeReviewIo({
postReviewStatuses: [422],
});
const { io, postedReviews } = makeReviewIo({ postReviewStatuses: [422, 200] });

await expect(postConsolidatedReview(io, 42, review, SINGLE_HUNK_DIFF, footer)).rejects.toThrow(
/Review POST failed \(status 422\)/,
);
await postConsolidatedReview(io, 42, review, SINGLE_HUNK_DIFF, footer);

expect(postedReviews.map((posted) => posted.commit_id)).toEqual([footer.headSha, undefined]);
expect(parseAiReviewState(postedReviews[1]?.body ?? "")?.head).toBe(footer.headSha);
});

test("an older workflow's review carries no state, so it never retries a body-only 422", async () => {
const finding = makeFinding({ file: "file.ts", line: 999 });
const review = makeMergedReview({ findings: [finding] });
const { io, postedReviews } = makeReviewIo({ postReviewStatuses: [422] });
const unpinned = { ...footer, headSha: undefined, reviewsComplete: undefined };

await expect(
postConsolidatedReview(io, 42, review, SINGLE_HUNK_DIFF, unpinned),
).rejects.toThrow(/Review POST failed \(status 422\)/);
expect(postedReviews).toHaveLength(1);
expect(parseAiReviewState(postedReviews[0]?.body ?? "")).toBeNull();
});

test("throws with GitHub's response body when the retry also 422s, instead of swallowing it", async () => {
Expand Down
55 changes: 48 additions & 7 deletions .github/scripts/ai-review/post-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,29 @@
*/

export const AI_REVIEW_MARKER = "<!-- supabase-ai-review -->";
const AI_REVIEW_STATE_PATTERN =
/^<!-- supabase-ai-review-state head=([0-9a-f]{40}) complete=(true|false) findings=(\d+) -->$/m;

/** What a posted AI review covered, read from the hidden state line at the top of its body. */
export interface AiReviewState {
head: string;
/** Whether both the Claude and the Codex review finished. */
complete: boolean;
/** Findings posted that were not refuted, inline or in the body. */
findings: number;
}

export function renderAiReviewState(state: AiReviewState): string {
return `<!-- supabase-ai-review-state head=${state.head} complete=${state.complete} findings=${state.findings} -->`;
}

export function parseAiReviewState(body: string): AiReviewState | null {
const match = AI_REVIEW_STATE_PATTERN.exec(body);
if (!match?.[1] || !match[2] || !match[3]) {
return null;
}
return { head: match[1], complete: match[2] === "true", findings: Number(match[3]) };
}
const SUPERSEDED_SUMMARY = "Superseded by a newer AI review";
/** Hidden marker `isSuperseded` checks for. Kept out of the human-readable
* `SUPERSEDED_SUMMARY` text, and broken by `sanitizeModelText`, so a model
Expand Down Expand Up @@ -606,6 +629,9 @@ export interface ReviewFooterInfo {
/** e.g. `` `claude-fable-5` + `gpt-5.6-sol` ``. Sourced from
* `.github/ai-models.json` through the workflow's model env vars. */
modelsFooter: string;
/** Commit the models reviewed; absent from older workflow copies, whose reviews carry no state and never approve. */
headSha?: string;
reviewsComplete?: boolean;
}

/** Renders the full review body: summary, findings table, out-of-diff section, refuted details, stats, and footer. */
Expand All @@ -619,7 +645,13 @@ export function renderReviewBody(
);
const verdicts = computeVerdictCounts(review.findings);

const sections: string[] = [`## 🤖 AI Review\n\n${sanitizeModelText(review.summary)}`];
// The markers lead the body so truncation never drops them. Advisory next-impact findings have no thread, so
// counting them keeps any review that raised one from approving a sync.
const state = footer.headSha
? `${renderAiReviewState({ head: footer.headSha, complete: footer.reviewsComplete === true, findings: posted.length + review.findings.filter(isAdvisoryNextImpact).length })}\n`
: "";
const header = `${AI_REVIEW_MARKER}\n${state}`;
const sections: string[] = [`${header}## 🤖 AI Review\n\n${sanitizeModelText(review.summary)}`];

if (posted.length > 0) {
const rows = posted.map(
Expand Down Expand Up @@ -697,8 +729,6 @@ export function renderReviewBody(
`Models: ${footer.modelsFooter} · Trigger: \`${footer.trigger}\` · [Workflow run](${footer.runUrl})`,
"",
"This review runs once per PR. A maintainer can request another with a `/ai-review` comment.",
"",
AI_REVIEW_MARKER,
].join("\n"),
);

Expand All @@ -715,6 +745,7 @@ export interface InlineReviewComment {
}

export interface ReviewPayload {
commit_id?: string;
event: "COMMENT";
body: string;
comments: InlineReviewComment[];
Expand Down Expand Up @@ -763,7 +794,12 @@ export function buildReviewPayload(
const body = renderReviewBody(review, partitioned, footer);
// A body-only review has no 422 fold-retry to truncate it later, so
// truncate the first payload too rather than posting an oversized body.
return { event: "COMMENT", body: truncateReviewBody(body, footer.runUrl), comments };
return {
...(footer.headSha ? { commit_id: footer.headSha } : {}),
event: "COMMENT",
body: truncateReviewBody(body, footer.runUrl),
comments,
};
}

/** Folds every inline comment into the review body, for the 422-retry path when GitHub rejects an anchor. */
Expand Down Expand Up @@ -903,11 +939,12 @@ export async function postConsolidatedReview(
const prior = await listPriorRunsBestEffort(io, prNumber);

const result = await io.postReview(prNumber, payload);
if (result.status === 422 && payload.comments.length > 0) {
if (result.status === 422 && (payload.comments.length > 0 || payload.commit_id)) {
console.warn(
"Review POST rejected an inline anchor (422); retrying once with comments folded into the body.",
"Review POST rejected (422); retrying once on the current head with comments folded into the body.",
);
const folded = foldInlineCommentsIntoBody(payload);
// A force-push can drop the reviewed commit; the state line still names it, so the review cannot vouch for the new head.
const { commit_id: _rejected, ...folded } = foldInlineCommentsIntoBody(payload);
const retryResult = await io.postReview(prNumber, {
...folded,
body: truncateReviewBody(folded.body, footer.runUrl),
Expand Down Expand Up @@ -1138,6 +1175,8 @@ async function runPost(): Promise<void> {
const prDiffPath = requireEnv("PR_DIFF_PATH");
const claudeModel = requireEnv("CLAUDE_MODEL");
const codexModel = requireEnv("CODEX_MODEL");
const headSha = process.env.HEAD_SHA || undefined;
const reviewsComplete = process.env.REVIEWS_COMPLETE === "true";

const raw: unknown = JSON.parse(await Bun.file(mergedReviewPath).text());
assertMergedReview(raw);
Expand All @@ -1147,6 +1186,8 @@ async function runPost(): Promise<void> {
trigger,
runUrl,
modelsFooter: `\`${claudeModel}\` + \`${codexModel}\``,
headSha,
reviewsComplete,
});
console.log(`Posted AI review on PR #${prNumber} (${raw.findings.length} finding(s)).`);
}
Expand Down
1 change: 1 addition & 0 deletions .github/scripts/ai-review/resolve.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ function makePr(overrides: Partial<PrDetails> = {}): PrDetails {
authorLogin: PR_AUTHOR,
headRepoFullName: REPO,
baseRepoFullName: REPO,
headSha: "a".repeat(40),
...overrides,
};
}
Expand Down
Loading
Loading