diff --git a/.github/ai-review/README.md b/.github/ai-review/README.md index d7b61629df..a65d25c202 100644 --- a/.github/ai-review/README.md +++ b/.github/ai-review/README.md @@ -32,15 +32,15 @@ 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 - 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. + 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. - **`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 @@ -71,25 +71,27 @@ 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). 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 +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,10 +163,22 @@ 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 - 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.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..400d0e3973 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,25 @@ 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. 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 { + 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 +266,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 +295,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 +336,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..1d38ff06dc 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: {} @@ -43,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 (future) `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: @@ -85,8 +91,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 }} @@ -542,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