From f42e60a02b84e535167aa5acfd7c587bf33f0221 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Tue, 1 Sep 2026 11:27:41 +0100 Subject: [PATCH 1/2] ci: enable automatic AI review for PR authors with write access Uncomment the pull_request (opened/ready_for_review) trigger and gate the automatic path in resolve.ts to PR authors whose effective repository permission is write/admin, mirroring the manual /ai-review path's WRITE_PERMISSIONS gate. External contributors' PRs are skipped and keep going through the manual /ai-review maintainer path. --- .github/ai-review/README.md | 49 +++++++----- .github/scripts/ai-review/resolve.test.ts | 93 ++++++++++++++++++++++- .github/scripts/ai-review/resolve.ts | 49 +++++++++--- .github/workflows/ai-review.yml | 26 +++---- 4 files changed, 169 insertions(+), 48 deletions(-) diff --git a/.github/ai-review/README.md b/.github/ai-review/README.md index d7b61629df..f38f701148 100644 --- a/.github/ai-review/README.md +++ b/.github/ai-review/README.md @@ -32,8 +32,8 @@ resolve ──────>┤ ├──> adjudicate ──> po - **`resolve`** (`.github/scripts/ai-review/resolve.ts`) decides whether this run should happen at all. It applies the once-per-PR dedup guard, the - draft/bot/fork skips (for the future automatic trigger), and authorization - for manual `/ai-review` requests. There is no size cap: the models review + automatic trigger's draft/bot/fork skips and author write-access gate, and + authorization for manual `/ai-review` requests. There is no size cap: the models review agentically — reading the diff and the changed files via their own tools over many turns, like the local CLI — so PRs of any size are reviewed (very large diffs best-effort, within the model's context/turn budget). One caveat: the @@ -71,25 +71,26 @@ on the same PR: Both bypass the dedup guard and the draft/fork/bot skips (a human explicitly asked). -## Rollout - -The pipeline currently runs only on-demand (`workflow_dispatch` or -`/ai-review`) — the `pull_request` trigger in the workflow is commented out -("shadow mode"). Rollout plan: - -1. Run it manually against a sample of recent real PRs; tune the two prompts - in this directory against what it actually produces. **This only works - end-to-end once the current security fixes are merged to `develop`**: the - prompts, schemas, and validation script are read from a trusted checkout of - the _default branch_ (not the PR under review), and `post-review` checks - out `develop` explicitly — so prompt/script tweaks on a feature branch - don't take effect until they land on `develop`. Use `workflow_dispatch` - against real merged/in-flight PRs post-merge to iterate. -2. Once satisfied, uncomment the `pull_request` trigger block in - `ai-review.yml`. -3. In the same change, disable the Codex GitHub App's automatic reviews at - so PRs aren't - double-reviewed. +## Automatic trigger + +The `pull_request` trigger (`opened` / `ready_for_review`) is live. The +automatic path is **internal PRs only**: `resolve.ts` skips drafts, bots, and +fork PRs, and requires the PR author to hold effective repository **write +access** (`admin`/`write`, the same `WRITE_PERMISSIONS` gate as the manual +`/ai-review` path). A same-repo branch already implies the author could push, +so the permission lookup is defense-in-depth — it also catches access revoked +since the branch was pushed. External contributors' PRs are never reviewed +automatically; a maintainer comments `/ai-review` to request one. + +Prompt/script tweaks take effect only once they land on `develop`: the +prompts, schemas, and validation script are read from a trusted checkout of +the _default branch_ (not the PR under review), and `post-review` checks out +`develop` explicitly. Use `workflow_dispatch` against real merged/in-flight +PRs post-merge to iterate. + +The Codex GitHub App's automatic reviews must stay disabled at + so PRs aren't +double-reviewed. `merged-review.schema.json` uses `pattern` (on `category`) and `minItems` (on `sources`); some OpenAI structured-output strict-mode implementations have @@ -161,6 +162,12 @@ run 400s on the output schema because of this, drop `pattern`/`minItems` from (`/ai-reviewers`, `/ai-review-please`, etc. don't fire). The workflow's job `if:` also pre-filters cheaply on `author_association` as defense-in-depth, but `resolve.ts`'s checks are the actual gate. +- **The automatic trigger requires the PR author to hold write access.** + `resolve.ts` resolves the PR author's effective repository permission and + requires `admin`/`write` before an automatic review runs, on top of the + fork/draft/bot skips — so an external contributor's PR can never spend + review budget or feed the models without a maintainer explicitly asking + via `/ai-review`. - **The only write-capable job runs exclusively trusted code.** `post-review` checks out the base branch (`develop`) explicitly and never the PR head, so a malicious PR cannot smuggle a change into the one job diff --git a/.github/scripts/ai-review/resolve.test.ts b/.github/scripts/ai-review/resolve.test.ts index 89b037c956..0beb52747d 100644 --- a/.github/scripts/ai-review/resolve.test.ts +++ b/.github/scripts/ai-review/resolve.test.ts @@ -10,6 +10,10 @@ import { const REPO = "supabase/cli"; const WORKFLOW_BOT_LOGIN = "github-actions[bot]"; +/** Default PR author in tests; grant them write via `WRITE_AUTHOR_PERMISSION` + * when a test needs to get past the auto trigger's authorization gate. */ +const PR_AUTHOR = "internal-author"; +const WRITE_AUTHOR_PERMISSION = { [PR_AUTHOR]: "write" }; function makePr(overrides: Partial = {}): PrDetails { return { @@ -17,6 +21,7 @@ function makePr(overrides: Partial = {}): PrDetails { state: "open", draft: false, authorIsBot: false, + authorLogin: PR_AUTHOR, headRepoFullName: REPO, baseRepoFullName: REPO, ...overrides, @@ -132,7 +137,10 @@ describe("resolveDecision: auto trigger (pull_request) skip conditions", () => { test("skips a PR that already carries the marker in a prior review from the workflow bot", async () => { const pr = makePr(); - const { io } = makeIo(pr, { reviews: [botMarkedBody(`Nice work.\n${AI_REVIEW_MARKER}`)] }); + const { io } = makeIo(pr, { + reviews: [botMarkedBody(`Nice work.\n${AI_REVIEW_MARKER}`)], + permissionByLogin: WRITE_AUTHOR_PERMISSION, + }); const result = await resolveDecision({ eventName: "pull_request", prNumber: pr.number }, io); expect(result.shouldRun).toBe(false); expect(result.skipReason).toBe( @@ -142,7 +150,10 @@ describe("resolveDecision: auto trigger (pull_request) skip conditions", () => { test("skips a PR that already carries the marker in a prior issue comment from the workflow bot", async () => { const pr = makePr(); - const { io } = makeIo(pr, { comments: [botMarkedBody(`Notice\n${AI_REVIEW_MARKER}`)] }); + const { io } = makeIo(pr, { + comments: [botMarkedBody(`Notice\n${AI_REVIEW_MARKER}`)], + permissionByLogin: WRITE_AUTHOR_PERMISSION, + }); const result = await resolveDecision({ eventName: "pull_request", prNumber: pr.number }, io); expect(result.shouldRun).toBe(false); expect(result.skipReason).toBe( @@ -155,6 +166,7 @@ describe("resolveDecision: auto trigger (pull_request) skip conditions", () => { const { io } = makeIo(pr, { reviews: [{ body: `Fake review\n${AI_REVIEW_MARKER}`, authorLogin: "not-the-workflow-bot" }], comments: [{ body: `Fake notice\n${AI_REVIEW_MARKER}`, authorLogin: "a-random-user" }], + permissionByLogin: WRITE_AUTHOR_PERMISSION, }); const result = await resolveDecision({ eventName: "pull_request", prNumber: pr.number }, io); expect(result.shouldRun).toBe(true); @@ -166,6 +178,7 @@ describe("resolveDecision: auto trigger (pull_request) skip conditions", () => { const { io } = makeIo(pr, { reviews: [{ body: "unrelated review", authorLogin: WORKFLOW_BOT_LOGIN }], comments: [{ body: "unrelated comment", authorLogin: WORKFLOW_BOT_LOGIN }], + permissionByLogin: WRITE_AUTHOR_PERMISSION, }); const result = await resolveDecision({ eventName: "pull_request", prNumber: pr.number }, io); expect(result.shouldRun).toBe(true); @@ -173,6 +186,80 @@ describe("resolveDecision: auto trigger (pull_request) skip conditions", () => { }); }); +describe("resolveDecision: auto trigger (pull_request) author authorization", () => { + test.each([ + ["write", true], + ["admin", true], + ["read", false], + ["none", false], + ])("author permission %s -> shouldRun=%s", async (permission, expectedShouldRun) => { + const pr = makePr(); + const { io, permissionLookups } = makeIo(pr, { + permissionByLogin: { [PR_AUTHOR]: permission }, + }); + const result = await resolveDecision({ eventName: "pull_request", prNumber: pr.number }, io); + expect(result.shouldRun).toBe(expectedShouldRun); + expect(permissionLookups).toEqual([PR_AUTHOR]); + }); + + test("an unresolvable author permission (undefined) is treated as unauthorized", async () => { + const pr = makePr(); + const { io } = makeIo(pr); + const result = await resolveDecision({ eventName: "pull_request", prNumber: pr.number }, io); + expect(result).toEqual({ + shouldRun: false, + skipReason: + `PR author @${PR_AUTHOR} does not have repository write access (permission=n/a); ` + + "a maintainer can comment /ai-review to request a review.", + trigger: "auto", + }); + }); + + test("an unauthorized author gets a descriptive skip reason with their permission", async () => { + const pr = makePr(); + const { io } = makeIo(pr, { permissionByLogin: { [PR_AUTHOR]: "read" } }); + const result = await resolveDecision({ eventName: "pull_request", prNumber: pr.number }, io); + expect(result.skipReason).toBe( + `PR author @${PR_AUTHOR} does not have repository write access (permission=read); ` + + "a maintainer can comment /ai-review to request a review.", + ); + }); + + test("the authorization gate runs before the dedup listing, so an unauthorized PR never lists reviews", async () => { + const pr = makePr(); + const { io, calls } = makeIo(pr, { permissionByLogin: { [PR_AUTHOR]: "read" } }); + const result = await resolveDecision({ eventName: "pull_request", prNumber: pr.number }, io); + expect(result.shouldRun).toBe(false); + expect(calls.listReviews).toBe(0); + expect(calls.listIssueComments).toBe(0); + }); + + test("draft/bot/fork skips fire before any permission lookup", async () => { + for (const overrides of [ + { draft: true }, + { authorIsBot: true }, + { headRepoFullName: "someone/fork" }, + ]) { + const pr = makePr(overrides); + const { io, permissionLookups } = makeIo(pr); + const result = await resolveDecision({ eventName: "pull_request", prNumber: pr.number }, io); + expect(result.shouldRun).toBe(false); + expect(permissionLookups).toEqual([]); + } + }); + + test("workflow_dispatch never looks up the PR author's permission", async () => { + const pr = makePr(); + const { io, permissionLookups } = makeIo(pr); + const result = await resolveDecision( + { eventName: "workflow_dispatch", prNumber: pr.number }, + io, + ); + expect(result.shouldRun).toBe(true); + expect(permissionLookups).toEqual([]); + }); +}); + describe("resolveDecision: manual trigger bypasses auto-only skips", () => { test.each([ ["a draft PR", { draft: true }], @@ -461,7 +548,7 @@ describe("resolveDecision: trigger classification per event shape", () => { test("pull_request is an auto trigger", async () => { const pr = makePr(); - const { io } = makeIo(pr); + const { io } = makeIo(pr, { permissionByLogin: WRITE_AUTHOR_PERMISSION }); const result = await resolveDecision({ eventName: "pull_request", prNumber: pr.number }, io); expect(result.trigger).toBe("auto"); }); diff --git a/.github/scripts/ai-review/resolve.ts b/.github/scripts/ai-review/resolve.ts index 17ceffaa08..d74d88d653 100644 --- a/.github/scripts/ai-review/resolve.ts +++ b/.github/scripts/ai-review/resolve.ts @@ -8,11 +8,11 @@ * - manual (`workflow_dispatch` or an internal maintainer's `/ai-review` * issue comment): a human explicitly asked for a review, so the * marker/dedup guard and the draft/fork/bot skips are bypassed. - * - auto (`pull_request` `opened`/`ready_for_review`, currently commented - * out in the workflow while prompts are tuned): skips drafts, bots, fork - * PRs (v1 is internal-PRs-only; forks go through the manual maintainer - * path), and PRs that already carry a marker comment/review from a prior - * run. + * - auto (`pull_request` `opened`/`ready_for_review`): only PRs whose + * author has repository write access get the automatic review. Skips + * drafts, bots, fork PRs, authors without write access (external + * contributors go through the manual maintainer path), and PRs that + * already carry a marker comment/review from a prior run. * * `resolveDecision` is the pure orchestration function (I/O injected, like * `evaluateAllOpenPrs` in `contribution-gate.ts`) that a test can drive @@ -64,6 +64,8 @@ export interface PrDetails { state: "open" | "closed"; draft: boolean; authorIsBot: boolean; + /** PR author's login, empty when the author account was deleted. */ + authorLogin: string; /** `owner/name` of the fork/branch the PR is from, empty when the head repo was deleted. */ headRepoFullName: string; /** `owner/name` of the repository the PR targets. */ @@ -81,7 +83,7 @@ export interface ResolveIo { fetchPr: (prNumber: number) => Promise; listReviews: (prNumber: number) => Promise; listIssueComments: (prNumber: number) => Promise; - /** Resolve a commenter's effective repository permission; see `fetchAuthorPermission`. */ + /** Resolve a user's effective repository permission; see `fetchAuthorPermission`. */ fetchPermission: (login: string) => Promise; /** React 👀 to the triggering comment, for UX feedback that the request was picked up. */ reactToComment: (commentId: number) => Promise; @@ -173,8 +175,8 @@ export async function resolveDecision(input: ResolveInput, io: ResolveIo): Promi return decideForPr(trigger); } - // Auto trigger (future `pull_request` events): v1 is internal-PRs-only and - // fires at most once per PR. + // Auto trigger (`pull_request` events): internal PRs only, fires at most + // once per PR. if (pr.draft) { return { shouldRun: false, skipReason: "PR is a draft.", trigger }; } @@ -189,6 +191,24 @@ export async function resolveDecision(input: ResolveInput, io: ResolveIo): Promi }; } + // Authoritative auto-trigger authorization: only PRs authored by someone + // with effective repository write access are reviewed automatically. A + // same-repo branch already implies the author could push when they opened + // the PR, so this is defense-in-depth (it also catches access revoked + // since); an unresolvable permission counts as unauthorized. Mirrors the + // manual path's gate above and `contribution-gate.ts`'s `WRITE_PERMISSIONS`. + const authorPermission = await io.fetchPermission(pr.authorLogin); + if (authorPermission === undefined || !WRITE_PERMISSIONS.has(authorPermission)) { + return { + shouldRun: false, + skipReason: + `PR author @${pr.authorLogin} does not have repository write access ` + + `(permission=${authorPermission ?? "n/a"}); ` + + `a maintainer can comment /ai-review to request a review.`, + trigger, + }; + } + const [reviews, comments] = await Promise.all([ io.listReviews(pr.number), io.listIssueComments(pr.number), @@ -245,7 +265,7 @@ interface RestPullRequest { number: number; state: "open" | "closed"; draft: boolean; - user: { type: string } | null; + user: { login: string; type: string } | null; head: { repo: { full_name: string } | null }; base: { repo: { full_name: string } }; } @@ -274,7 +294,12 @@ function assertRestPullRequest(value: unknown): asserts value is RestPullRequest typeof value.number !== "number" || (value.state !== "open" && value.state !== "closed") || typeof value.draft !== "boolean" || - !(value.user === null || (isRecordEntry(value.user) && typeof value.user.type === "string")) || + !( + value.user === null || + (isRecordEntry(value.user) && + typeof value.user.login === "string" && + typeof value.user.type === "string") + ) || !isRecordEntry(value.head) || !( value.head.repo === null || @@ -310,6 +335,10 @@ async function fetchPullRequest(token: string, base: string, prNumber: number): state: pr.state, draft: pr.draft, authorIsBot: pr.user?.type === "Bot", + // Empty when the author account was deleted; `fetchAuthorPermission` + // resolves an empty login to `undefined`, which the auto gate treats as + // unauthorized. + authorLogin: pr.user?.login ?? "", headRepoFullName: pr.head.repo?.full_name ?? "", baseRepoFullName: pr.base.repo.full_name, }; diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml index 3fb34da77f..3360b089e6 100644 --- a/.github/workflows/ai-review.yml +++ b/.github/workflows/ai-review.yml @@ -5,11 +5,13 @@ name: AI Review # exhaustive pass that runs at most once per PR. See # .github/ai-review/README.md for the full design and security model. # -# Two ways to trigger a run today: +# Three ways to trigger a run: # - workflow_dispatch, for testing / ad-hoc runs against any PR number. # - an internal maintainer commenting `/ai-review` on a PR. -# The `pull_request` trigger below is intentionally commented out (shadow -# mode) until the prompts are tuned against real PRs — see the README. +# - automatically, when a PR opens or leaves draft. resolve.ts gates the +# automatic path to PR authors with repository write access; external +# contributors' PRs are skipped and go through the manual `/ai-review` +# maintainer path instead. on: workflow_dispatch: inputs: @@ -20,14 +22,10 @@ on: issue_comment: types: - created - # Shadow-mode rollout: uncomment once the prompts have been tuned against - # recent real PRs (see .github/ai-review/README.md), and disable the Codex - # GitHub App's automatic reviews (chatgpt.com/codex/settings/code-review) - # in the same change so the two don't double-review every PR. - # pull_request: - # types: - # - opened - # - ready_for_review + pull_request: + types: + - opened + - ready_for_review permissions: {} @@ -44,7 +42,7 @@ env: # (even one that isn't `/ai-review`) would cancel an in-flight review via # `cancel-in-progress`. Give those runs their own per-run group so they can # never cancel a real review; only genuine `/ai-review` comments, dispatches, -# and (future) `pull_request` events share the PR's group. The command test is +# and `pull_request` events share the PR's group. The command test is # exact equality (`!= '/ai-review'`), mirroring resolve.ts's first-line check — # `startsWith` would let a near-miss like `/ai-reviewers` (which resolve.ts # rejects) land in the shared group and cancel a running review anyway. @@ -85,8 +83,8 @@ jobs: trigger: ${{ steps.resolve.outputs.trigger }} steps: # Base repo, default ref, pinned explicitly — this job runs trusted - # repository code exclusively, and must keep doing so even if the - # `pull_request` trigger above is ever uncommented. + # repository code exclusively, and must keep doing so even though the + # `pull_request` trigger above hands it PR-authored event payloads. - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: ref: ${{ github.event.repository.default_branch }} From d9916cad114733e21282c639ed363c82a09e4253 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Tue, 1 Sep 2026 11:44:37 +0100 Subject: [PATCH 2/2] ci: stop fresh AI reviews from superseding themselves; address review findings The supersede pass listed reviews AFTER posting, so the just-posted review (a marker-bearing bot review) was swept into its own supersede pass and every fresh review immediately collapsed as 'Superseded by a newer AI review'. Snapshot the prior runs before the POST instead, and make the test fake mirror real GitHub by including posted reviews in later listings. Also addresses the AI review findings on this PR: pull_request runs get their own per-PR concurrency group so a ready_for_review event can't cancel an in-flight maintainer-requested review; the write-access lookup is documented as the authoritative author check (a same-repo branch doesn't prove the author pushed it); the post-review trusted-code claim now acknowledges that pull_request events execute the PR ref's workflow file and why that's safe; README line rewrapped to the 80-column convention. --- .github/ai-review/README.md | 35 +++--- .github/scripts/ai-review/post-review.test.ts | 28 ++++- .github/scripts/ai-review/post-review.ts | 101 +++++++++++------- .github/scripts/ai-review/resolve.ts | 11 +- .github/workflows/ai-review.yml | 27 +++-- 5 files changed, 137 insertions(+), 65 deletions(-) diff --git a/.github/ai-review/README.md b/.github/ai-review/README.md index f38f701148..a65d25c202 100644 --- a/.github/ai-review/README.md +++ b/.github/ai-review/README.md @@ -33,14 +33,14 @@ resolve ──────>┤ ├──> adjudicate ──> po - **`resolve`** (`.github/scripts/ai-review/resolve.ts`) decides whether this run should happen at all. It applies the once-per-PR dedup guard, the automatic trigger's draft/bot/fork skips and author write-access gate, and - authorization for manual `/ai-review` requests. There is no size cap: the models review - agentically — reading the diff and the changed files via their own tools over - many turns, like the local CLI — so PRs of any size are reviewed (very large - diffs best-effort, within the model's context/turn budget). One caveat: the - diff is fetched with `gh pr diff`, which GitHub itself caps (≈300 files / - 20k lines / 1 MB); a PR beyond those limits gets a truncated diff, so the - review is truncated with it. Generating the diff from the base/head refs - instead is a possible follow-up. + authorization for manual `/ai-review` requests. There is no size cap: the + models review agentically — reading the diff and the changed files via + their own tools over many turns, like the local CLI — so PRs of any size + are reviewed (very large diffs best-effort, within the model's context/turn + budget). One caveat: the diff is fetched with `gh pr diff`, which GitHub + itself caps (≈300 files / 20k lines / 1 MB); a PR beyond those limits gets + a truncated diff, so the review is truncated with it. Generating the diff + from the base/head refs instead is a possible follow-up. - **`claude-review`** and **`codex-review`** run **in parallel** — each gives its model an independent, exhaustive pass and produces structured JSON findings validated against `findings.schema.json`. Claude reads the PR's @@ -77,10 +77,11 @@ The `pull_request` trigger (`opened` / `ready_for_review`) is live. The automatic path is **internal PRs only**: `resolve.ts` skips drafts, bots, and fork PRs, and requires the PR author to hold effective repository **write access** (`admin`/`write`, the same `WRITE_PERMISSIONS` gate as the manual -`/ai-review` path). A same-repo branch already implies the author could push, -so the permission lookup is defense-in-depth — it also catches access revoked -since the branch was pushed. External contributors' PRs are never reviewed -automatically; a maintainer comments `/ai-review` to request one. +`/ai-review` path). The permission lookup is the authoritative author check: +a same-repo head branch only proves the branch exists in this repo, not that +the PR author pushed it, so the author's own permission is always resolved. +External contributors' PRs are never reviewed automatically; a maintainer +comments `/ai-review` to request one. Prompt/script tweaks take effect only once they land on `develop`: the prompts, schemas, and validation script are read from a trusted checkout of @@ -170,8 +171,14 @@ run 400s on the output schema because of this, drop `pattern`/`minItems` from via `/ai-review`. - **The only write-capable job runs exclusively trusted code.** `post-review` checks out the base branch (`develop`) explicitly and never - the PR head, so a malicious PR cannot smuggle a change into the one job - that can write back to it. + the PR head, so a PR cannot smuggle a script change into the one job that + can write back to it. The checkout pin alone is not the whole boundary for + `pull_request` runs, though: GitHub executes the workflow FILE from the + PR's own ref for those events. That is safe here because the automatic + path only admits same-repo PRs, whose authors hold write access anyway + (a workflow edit gains them nothing they don't already have), while fork + PRs run with a read-only token and no secrets. `issue_comment` and + `workflow_dispatch` runs always use the default branch's workflow file. - **Model text is sanitized before it's rendered.** `sanitizeModelText()` redacts secret-shaped substrings (`redactSecrets()`; see below), strips HTML comments (so injected diff content can't forge the hidden dedup/supersede diff --git a/.github/scripts/ai-review/post-review.test.ts b/.github/scripts/ai-review/post-review.test.ts index a5911c658a..152746c3f0 100644 --- a/.github/scripts/ai-review/post-review.test.ts +++ b/.github/scripts/ai-review/post-review.test.ts @@ -1030,7 +1030,16 @@ describe("post flow via injected ReviewIo", () => { if (opts.failSupersede) { return Promise.reject(new Error("listReviews failed")); } - return Promise.resolve(opts.reviews ?? []); + // Mirror real GitHub: a review posted earlier in the same run shows + // up in later listings as a marker-bearing bot review. The supersede + // pass must snapshot BEFORE posting or it would wrap the fresh + // review as "superseded" too. + const alreadyPosted = postedReviews.map((payload, i) => ({ + id: 900 + i, + body: payload.body, + authorLogin: "github-actions[bot]", + })); + return Promise.resolve([...(opts.reviews ?? []), ...alreadyPosted]); }, listIssueComments: () => { calls.push("listIssueComments"); @@ -1109,6 +1118,23 @@ describe("post flow via injected ReviewIo", () => { expect(calls.indexOf("postReview")).toBeLessThan(calls.indexOf("updateReviewBody")); }); + test("the freshly posted review is never swept into its own supersede pass", async () => { + const review = makeMergedReview({ findings: [] }); + const { io, updatedReviews, updatedComments, postedReviews, calls } = makeReviewIo({ + diff: SINGLE_HUNK_DIFF, + }); + + await postConsolidatedReview(io, 42, review, footer); + + // With no prior AI review on the PR, nothing may be wrapped as superseded + // — especially not the review this run just posted (which the fake's + // listReviews, like real GitHub, includes in post-POST listings). + expect(postedReviews).toHaveLength(1); + expect(updatedReviews).toEqual([]); + expect(updatedComments).toEqual([]); + expect(calls.indexOf("listReviews")).toBeLessThan(calls.indexOf("postReview")); + }); + test("a review still posts even when the best-effort supersede fails", async () => { const review = makeMergedReview({ findings: [] }); const { io, postedReviews } = makeReviewIo({ diff: SINGLE_HUNK_DIFF, failSupersede: true }); diff --git a/.github/scripts/ai-review/post-review.ts b/.github/scripts/ai-review/post-review.ts index 0e134e813b..e7e287bfa3 100644 --- a/.github/scripts/ai-review/post-review.ts +++ b/.github/scripts/ai-review/post-review.ts @@ -17,12 +17,16 @@ * artifact, so a prompt-injected `Read` of a secret-bearing path can't * smuggle a credential out through the artifact even though the posted * review is already scrubbed at render time. - * - `post` — posts the consolidated review, THEN best-effort supersedes any - * prior AI review on the PR (the marker/dedup guard in `resolve.ts` should - * normally prevent a second run, but `/ai-review` lets a maintainer force - * one; posting before superseding, and treating the supersede as - * best-effort, means a cosmetic supersede failure can never cost the real - * review). + * - `post` — snapshots the PR's prior AI reviews, posts the consolidated + * review, THEN best-effort supersedes the snapshotted ones (the + * marker/dedup guard in `resolve.ts` should normally prevent a second + * run, but `/ai-review` lets a maintainer force one). The snapshot must + * happen BEFORE the POST — the fresh review is itself a marker-bearing + * bot review, so a post-hoc listing would sweep it into its own + * supersede pass and every new review would collapse itself. Posting + * before superseding, and treating both the snapshot and the supersede + * as best-effort, means a cosmetic failure can never cost the real + * review. * * `parseDiffAnchors`, `partitionFindings`, `renderReviewBody`, * `renderInlineComment`, `buildReviewPayload`, `foldInlineCommentsIntoBody`, @@ -842,42 +846,61 @@ export interface ReviewIo { ) => Promise<{ status: number; body?: string }>; } -/** Wraps every prior AI review/comment on the PR in a superseded `
` block. Idempotent. */ -async function supersedePriorRuns(io: ReviewIo, prNumber: number): Promise { - const [reviews, comments] = await Promise.all([ - io.listReviews(prNumber), - io.listIssueComments(prNumber), - ]); +/** The prior AI reviews/comments this run will supersede, snapshotted BEFORE + * the new review is posted. */ +interface PriorRuns { + reviews: MarkedEntry[]; + comments: MarkedEntry[]; +} - for (const review of reviews) { - if ( - review.authorLogin !== WORKFLOW_BOT_LOGIN || - !review.body.includes(AI_REVIEW_MARKER) || - isSuperseded(review.body) - ) { - continue; - } - await io.updateReviewBody(prNumber, review.id, supersededBody(review.body)); - } +/** A marker-bearing AI review/comment by the workflow bot that hasn't been + * superseded yet — the only kind a supersede pass may wrap. */ +function isSupersedableAiEntry(entry: MarkedEntry): boolean { + return ( + entry.authorLogin === WORKFLOW_BOT_LOGIN && + entry.body.includes(AI_REVIEW_MARKER) && + !isSuperseded(entry.body) + ); +} - for (const comment of comments) { - if ( - comment.authorLogin !== WORKFLOW_BOT_LOGIN || - !comment.body.includes(AI_REVIEW_MARKER) || - isSuperseded(comment.body) - ) { - continue; - } - await io.updateIssueCommentBody(comment.id, supersededBody(comment.body)); +/** Snapshots the prior AI reviews/comments to supersede. MUST run before the + * new review is posted: the fresh review is itself a marker-bearing bot + * review, so a post-hoc listing would sweep it into its own supersede pass + * and every new review would immediately collapse as "superseded". + * Best-effort — a listing failure degrades to an empty snapshot (prior runs + * stay unwrapped) rather than costing the real review. */ +async function listPriorRunsBestEffort(io: ReviewIo, prNumber: number): Promise { + try { + const [reviews, comments] = await Promise.all([ + io.listReviews(prNumber), + io.listIssueComments(prNumber), + ]); + return { + reviews: reviews.filter(isSupersedableAiEntry), + comments: comments.filter(isSupersedableAiEntry), + }; + } catch (error) { + console.warn(`Could not list prior AI review runs on PR #${prNumber}: ${String(error)}`); + return { reviews: [], comments: [] }; } } -/** Best-effort wrapper around `supersedePriorRuns`: a cosmetic failure here - * (e.g. a transient 404 on a review that was deleted mid-run) must never - * fail the pipeline after the real review/notice has already been posted. */ -async function supersedePriorRunsBestEffort(io: ReviewIo, prNumber: number): Promise { +/** Wraps the snapshotted prior AI reviews/comments in a superseded `
` + * block. Best-effort: a cosmetic failure here (e.g. a transient 404 on a + * review that was deleted mid-run) must never fail the pipeline after the + * real review has already been posted. */ +async function supersedePriorRunsBestEffort( + io: ReviewIo, + prNumber: number, + prior: PriorRuns, +): Promise { try { - await supersedePriorRuns(io, prNumber); + for (const review of prior.reviews) { + await io.updateReviewBody(prNumber, review.id, supersededBody(review.body)); + } + for (const comment of prior.comments) { + await io.updateIssueCommentBody(comment.id, supersededBody(comment.body)); + } } catch (error) { console.warn(`Could not supersede prior AI review runs on PR #${prNumber}: ${String(error)}`); } @@ -893,6 +916,10 @@ export async function postConsolidatedReview( const anchors = parseDiffAnchors(diff); const payload = buildReviewPayload(review, anchors, footer); + // Snapshot before the POST — see `listPriorRunsBestEffort` for why the + // ordering is load-bearing. + const prior = await listPriorRunsBestEffort(io, prNumber); + const result = await io.postReview(prNumber, payload); if (result.status === 422 && payload.comments.length > 0) { console.warn( @@ -915,7 +942,7 @@ export async function postConsolidatedReview( ); } - await supersedePriorRunsBestEffort(io, prNumber); + await supersedePriorRunsBestEffort(io, prNumber, prior); } // --- Real GitHub I/O (only runs when executed directly) --- diff --git a/.github/scripts/ai-review/resolve.ts b/.github/scripts/ai-review/resolve.ts index d74d88d653..400d0e3973 100644 --- a/.github/scripts/ai-review/resolve.ts +++ b/.github/scripts/ai-review/resolve.ts @@ -192,11 +192,12 @@ export async function resolveDecision(input: ResolveInput, io: ResolveIo): Promi } // Authoritative auto-trigger authorization: only PRs authored by someone - // with effective repository write access are reviewed automatically. A - // same-repo branch already implies the author could push when they opened - // the PR, so this is defense-in-depth (it also catches access revoked - // since); an unresolvable permission counts as unauthorized. Mirrors the - // manual path's gate above and `contribution-gate.ts`'s `WRITE_PERMISSIONS`. + // with effective repository write access are reviewed automatically. This + // is the actual author check, not defense-in-depth — a same-repo head + // branch only proves the branch exists in this repo, not that the AUTHOR + // pushed it (a PR can be opened from a branch someone else pushed). An + // unresolvable permission counts as unauthorized. Mirrors the manual + // path's gate above and `contribution-gate.ts`'s `WRITE_PERMISSIONS`. const authorPermission = await io.fetchPermission(pr.authorLogin); if (authorPermission === undefined || !WRITE_PERMISSIONS.has(authorPermission)) { return { diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml index 3360b089e6..1d38ff06dc 100644 --- a/.github/workflows/ai-review.yml +++ b/.github/workflows/ai-review.yml @@ -41,16 +41,24 @@ env: # comment on EVERY PR; with only the PR number in the group, any comment # (even one that isn't `/ai-review`) would cancel an in-flight review via # `cancel-in-progress`. Give those runs their own per-run group so they can -# never cancel a real review; only genuine `/ai-review` comments, dispatches, -# and `pull_request` events share the PR's group. The command test is -# exact equality (`!= '/ai-review'`), mirroring resolve.ts's first-line check — -# `startsWith` would let a near-miss like `/ai-reviewers` (which resolve.ts -# rejects) land in the shared group and cancel a running review anyway. +# never cancel a real review. The command test is exact equality +# (`!= '/ai-review'`), mirroring resolve.ts's first-line check — `startsWith` +# would let a near-miss like `/ai-reviewers` (which resolve.ts rejects) land +# in a shared group and cancel a running review anyway. +# +# `pull_request` events get their own per-PR `auto` group, separate from the +# manual (`/ai-review` / dispatch) `review` group: an auto event may well +# resolve to a SKIP (dedup, no write access), and letting it share the manual +# group would let e.g. a ready_for_review event cancel an in-flight +# maintainer-requested review and then not replace it. The cost is that an +# auto and a manual run can overlap on the same PR — rare, and self-healing, +# since the later post supersedes the earlier review. concurrency: group: >- ai-review-${{ github.event.pull_request.number || github.event.issue.number || inputs.pr }}-${{ (github.event_name == 'issue_comment' && github.event.comment.body != '/ai-review') - && github.run_id || 'review' }} + && github.run_id + || (github.event_name == 'pull_request' && 'auto' || 'review') }} cancel-in-progress: true jobs: @@ -540,8 +548,11 @@ jobs: # SECURITY-CRITICAL: this is the only job with write permission, so it # must only ever execute trusted base-branch code — never the PR head. # Checking out `develop` explicitly (never `needs.resolve.outputs.head_ref`) - # keeps a malicious PR from smuggling a workflow-file or script change - # into the one job that can write back to the PR. + # keeps a malicious PR from smuggling a script change into the one job + # that can write back to the PR. (For `pull_request` events GitHub runs + # the workflow FILE from the PR's own ref; acceptable because the auto + # path only admits same-repo PRs, whose authors hold write access + # anyway, and fork PRs run with a read-only token and no secrets.) - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: ref: develop