diff --git a/.github/scripts/pr-review/prepare.mjs b/.github/scripts/pr-review/prepare.mjs index dd97d40..69f9378 100644 --- a/.github/scripts/pr-review/prepare.mjs +++ b/.github/scripts/pr-review/prepare.mjs @@ -48,6 +48,10 @@ const headSha = required("PR_HEAD_SHA"); const sessionKey = required("SESSION_KEY"); const readinessContextSha256 = required("READINESS_CONTEXT_SHA256"); const maxDiffBytes = positiveInteger("MAX_DIFF_BYTES"); +// Leave a small read margin so the explicit size check reports the limit. +const diffReadBufferBytes = maxDiffBytes + 64 * 1024; +const automaticMaxDiffBytes = positiveInteger("AUTOMATIC_MAX_DIFF_BYTES"); +const largeDiffApproved = required("LARGE_DIFF_APPROVED") === "true"; const chunkTargetBytes = positiveInteger("CHUNK_TARGET_BYTES"); const ledgerPath = path.join(stateDir, "review-ledger.json"); const generationsDir = path.join(stateDir, "generations"); @@ -84,12 +88,18 @@ const diffArgs = (from, to, tripleDot = false) => [ const mergeBase = String(git(repo, ["merge-base", baseTipSha, headSha])).trim(); const fullDiff = Buffer.from(git(repo, diffArgs(mergeBase, headSha), { encoding: null, + maxBuffer: diffReadBufferBytes, })); if (fullDiff.length > maxDiffBytes) { throw new Error( `Pull-request diff is ${fullDiff.length} bytes; the configured total limit is ${maxDiffBytes} bytes.`, ); } +if (!largeDiffApproved && fullDiff.length > automaticMaxDiffBytes) { + throw new Error( + `Pull-request diff is ${fullDiff.length} bytes; automatic reviews are limited to ${automaticMaxDiffBytes} bytes. A repository admin must comment @codex review approve ${headSha} on this PR to review this head.`, + ); +} const effectiveDiffSha256 = sha256(fullDiff); const completed = ledger.generations @@ -217,7 +227,7 @@ function filePatch(record, from, to, tripleDot) { ...diffArgs(from, to, tripleDot), "--", ...paths, - ])); + ], { maxBuffer: diffReadBufferBytes })); } function addedLines(patchText) { diff --git a/.github/scripts/pr-review/test.mjs b/.github/scripts/pr-review/test.mjs index 5f5847e..0f21544 100644 --- a/.github/scripts/pr-review/test.mjs +++ b/.github/scripts/pr-review/test.mjs @@ -150,6 +150,10 @@ assert.doesNotMatch(workflowSource, /\$\{\{ inputs\.(?:model|effort|max-diff-byt assert.match(workflowSource, /^ review:\n(?:(?!^ \S)[\s\S])*?^ REVIEW_MODEL: gpt-6-sol$/m); assert.match(workflowSource, /^ review:\n(?:(?!^ \S)[\s\S])*?^ REVIEW_EFFORT: low$/m); assert.match(workflowSource, /^ review:\n(?:(?!^ \S)[\s\S])*?^ REVIEW_MAX_DIFF_BYTES: '100000000'$/m); +assert.match(workflowSource, /^ review:\n(?:(?!^ \S)[\s\S])*?^ REVIEW_AUTOMATIC_MAX_DIFF_BYTES: '5000000'$/m); +assert.match(workflowSource, /AUTOMATIC_MAX_DIFF_BYTES: \$\{\{ env\.REVIEW_AUTOMATIC_MAX_DIFF_BYTES \}\}/); +assert.match(workflowSource, /LARGE_DIFF_APPROVED: \$\{\{ needs\.resolve\.outputs\.large_diff_approved \}\}/); +assert.match(workflowSource, /getCollaboratorPermissionLevel/); assert.match(workflowSource, /^ publish:\n(?:(?!^ \S)[\s\S])*?^ REVIEW_MODEL: gpt-6-sol$/m); assert.match(workflowSource, /^ publish:\n(?:(?!^ \S)[\s\S])*?^ REVIEW_EFFORT: low$/m); assert.match(workflowSource, /MAX_DIFF_BYTES: \$\{\{ env\.REVIEW_MAX_DIFF_BYTES \}\}/); @@ -537,11 +541,40 @@ try { PR_HEAD_SHA: head, SESSION_KEY: "repo:1:pr:2:v2", MAX_DIFF_BYTES: "1000000", + AUTOMATIC_MAX_DIFF_BYTES: "1000000", + LARGE_DIFF_APPROVED: "false", CHUNK_TARGET_BYTES: "600", READINESS_CONTEXT_SHA256: "context-v1", }, }); assert.equal(result.status, 0, result.stderr); + const prepareWithPolicy = (automaticMax, approved, hardMax = "1000000") => + spawnSync(process.execPath, [ + path.join(path.dirname(new URL(import.meta.url).pathname), "prepare.mjs"), + ], { + cwd: repo, + encoding: "utf8", + env: { + ...process.env, + REPOSITORY_DIR: repo, + PR_REVIEW_STATE_DIR: state, + PR_BASE_SHA: base, + PR_HEAD_SHA: head, + SESSION_KEY: "repo:1:pr:2:v2", + MAX_DIFF_BYTES: hardMax, + AUTOMATIC_MAX_DIFF_BYTES: automaticMax, + LARGE_DIFF_APPROVED: String(approved), + CHUNK_TARGET_BYTES: "600", + READINESS_CONTEXT_SHA256: "context-v1", + }, + }); + const blockedAutomatic = prepareWithPolicy("1000", false); + assert.notEqual(blockedAutomatic.status, 0); + assert.match(blockedAutomatic.stderr, /automatic reviews are limited to 1000 bytes/); + assert.equal(prepareWithPolicy("1000", true).status, 0); + const blockedHardCap = prepareWithPolicy("1000", true, "1000"); + assert.notEqual(blockedHardCap.status, 0); + assert.match(blockedHardCap.stderr, /configured total limit is 1000 bytes/); const ledger = JSON.parse(fs.readFileSync( path.join(state, "review-ledger.json"), "utf8", @@ -578,6 +611,8 @@ try { PR_HEAD_SHA: head, SESSION_KEY: "repo:1:pr:2:v2", MAX_DIFF_BYTES: "1000000", + AUTOMATIC_MAX_DIFF_BYTES: "1000000", + LARGE_DIFF_APPROVED: "false", CHUNK_TARGET_BYTES: "600", READINESS_CONTEXT_SHA256: "context-v1", }, @@ -613,6 +648,8 @@ try { PR_HEAD_SHA: nextHead, SESSION_KEY: "repo:1:pr:2:v2", MAX_DIFF_BYTES: "1000000", + AUTOMATIC_MAX_DIFF_BYTES: "1000000", + LARGE_DIFF_APPROVED: "false", CHUNK_TARGET_BYTES: "600", READINESS_CONTEXT_SHA256: "context-v1", }, @@ -673,6 +710,8 @@ try { PR_HEAD_SHA: mergedHead, SESSION_KEY: "repo:1:pr:2:v2", MAX_DIFF_BYTES: "1000000", + AUTOMATIC_MAX_DIFF_BYTES: "1000000", + LARGE_DIFF_APPROVED: "false", CHUNK_TARGET_BYTES: "600", READINESS_CONTEXT_SHA256: "context-v1", }; @@ -1019,7 +1058,9 @@ fs.writeFileSync(outputFile, JSON.stringify(result)); cwd: repo, encoding: "utf8", env: { ...process.env, REPOSITORY_DIR: repo, PR_REVIEW_STATE_DIR: legacyState, PR_BASE_SHA: base, PR_HEAD_SHA: nextHead, SESSION_KEY: "repo:1:pr:2:v2", - MAX_DIFF_BYTES: "1000000", CHUNK_TARGET_BYTES: "600", + MAX_DIFF_BYTES: "1000000", + AUTOMATIC_MAX_DIFF_BYTES: "1000000", + LARGE_DIFF_APPROVED: "false", CHUNK_TARGET_BYTES: "600", READINESS_CONTEXT_SHA256: "context-v1", GITHUB_OUTPUT: output }, }); assert.equal(prepared.status, 0, prepared.stderr); @@ -1369,6 +1410,8 @@ fs.writeFileSync(outputFile, JSON.stringify(result)); PR_HEAD_SHA: mergedPrHead, SESSION_KEY: "repo:1:pr:2:v2", MAX_DIFF_BYTES: "1000000", + AUTOMATIC_MAX_DIFF_BYTES: "1000000", + LARGE_DIFF_APPROVED: "false", CHUNK_TARGET_BYTES: "600", READINESS_CONTEXT_SHA256: "context-v1", }, diff --git a/.github/scripts/review-request/common.mjs b/.github/scripts/review-request/common.mjs index 6b41852..7380be9 100644 --- a/.github/scripts/review-request/common.mjs +++ b/.github/scripts/review-request/common.mjs @@ -5,6 +5,8 @@ export const REVIEW_REQUEST_SCHEMA_VERSION = 1; // spaces as an indented code block, so the trigger accepts at most three. export const REVIEW_REQUEST_COMMAND = /^ {0,3}@codex(?:[ \t]+review\b.*?)?[ \t]*$/im; +const LARGE_DIFF_APPROVAL_COMMAND = + /^ {0,3}@codex[ \t]+review[ \t]+approve[ \t]+([0-9a-f]{40})[ \t]*$/im; const FENCE = /^ {0,3}(`{3,}|~{3,})/; const BLOCK_QUOTE = /^ {0,3}>/; @@ -100,3 +102,15 @@ export function isReviewRequestComment(comment) { if (type === "bot" || /\[bot\]$/i.test(login)) return false; return REVIEW_REQUEST_COMMAND.test(stripQuotedText(comment?.body)); } + +export function largeDiffApprovalSha(comment) { + const type = String(comment?.user_type ?? "").toLowerCase(); + const login = String(comment?.user_login ?? ""); + if (type === "bot" || /\[bot\]$/i.test(login)) return null; + return LARGE_DIFF_APPROVAL_COMMAND.exec(stripQuotedText(comment?.body))?.[1]?.toLowerCase() ?? null; +} + +export function isAdminLargeDiffApproval({ comment, permission, headSha }) { + return permission === "admin" + && largeDiffApprovalSha(comment) === String(headSha).toLowerCase(); +} diff --git a/.github/scripts/review-request/test.mjs b/.github/scripts/review-request/test.mjs index 909e1fd..70697a0 100644 --- a/.github/scripts/review-request/test.mjs +++ b/.github/scripts/review-request/test.mjs @@ -2,13 +2,48 @@ import assert from "node:assert/strict"; import { + isAdminLargeDiffApproval, isReviewRequestComment, + largeDiffApprovalSha, REVIEW_REQUEST_SCHEMA_VERSION, stripQuotedText, } from "./common.mjs"; assert.equal(REVIEW_REQUEST_SCHEMA_VERSION, 1); +const approvalHead = "a".repeat(40); +const approvalComment = { + body: `@codex review approve ${approvalHead}`, + user_type: "User", + user_login: "admin-user", +}; +assert.equal(isReviewRequestComment(approvalComment), true); +assert.equal(largeDiffApprovalSha(approvalComment), approvalHead); +assert.equal(isAdminLargeDiffApproval({ + comment: approvalComment, permission: "admin", headSha: approvalHead, +}), true); +for (const permission of ["write", "maintain", "read", "none", undefined]) { + assert.equal(isAdminLargeDiffApproval({ + comment: approvalComment, permission, headSha: approvalHead, + }), false); +} +assert.equal(isAdminLargeDiffApproval({ + comment: approvalComment, permission: "admin", headSha: "b".repeat(40), +}), false); +for (const body of [ + `> @codex review approve ${approvalHead}`, + `\`@codex review approve ${approvalHead}\``, + `\`\`\`\n@codex review approve ${approvalHead}\n\`\`\``, + `@codex review approve ${approvalHead.slice(1)}`, + `@codex review approve ${approvalHead} extra`, + `@codex review ${approvalHead}`, +]) { + assert.equal(largeDiffApprovalSha({ ...approvalComment, body }), null, body); +} +assert.equal(largeDiffApprovalSha({ + ...approvalComment, user_type: "Bot", +}), null); + const cases = [ ["bare mention", "@codex", true], ["bare mention with surrounding whitespace", " \n@codex \n\n", true], diff --git a/.github/workflows/codex-openai-review.yml b/.github/workflows/codex-openai-review.yml index d8130ca..da9d584 100644 --- a/.github/workflows/codex-openai-review.yml +++ b/.github/workflows/codex-openai-review.yml @@ -97,6 +97,7 @@ jobs: base_ref: ${{ steps.pr.outputs.base_ref }} head_sha: ${{ steps.pr.outputs.head_sha }} request_comment_id: ${{ steps.pr.outputs.request_comment_id }} + large_diff_approved: ${{ steps.pr.outputs.large_diff_approved }} steps: - name: Check out the exact comment-trigger implementation if: github.event_name == 'issue_comment' @@ -158,7 +159,40 @@ jobs: const eligible = pr.state === 'open' && !pr.draft && (sameRepository || trustedBaseEvent); + let largeDiffApproved = false; + if (event === 'issue_comment' && process.env.COMMENT_REVIEW_REQUESTED === 'true') { + const { pathToFileURL } = require('node:url'); + const path = require('node:path'); + const source = pathToFileURL(path.join( + process.env.GITHUB_WORKSPACE, + '.openai-pr-review-trigger-source', + '.github/scripts/review-request/common.mjs', + )).href; + const { largeDiffApprovalSha, isAdminLargeDiffApproval } = await import(source); + const comment = { + body: context.payload.comment.body, + user_type: context.payload.comment.user.type, + user_login: context.payload.comment.user.login, + }; + if (largeDiffApprovalSha(comment)) { + try { + const { data: access } = await github.rest.repos.getCollaboratorPermissionLevel({ + owner: context.repo.owner, + repo: context.repo.repo, + username: comment.user_login, + }); + largeDiffApproved = isAdminLargeDiffApproval({ + comment, + permission: access.permission, + headSha: pr.head.sha, + }); + } catch (error) { + core.warning(`Could not verify large-diff approval permission: ${error.message}`); + } + } + } core.setOutput('eligible', String(eligible)); + core.setOutput('large_diff_approved', String(largeDiffApproved)); core.setOutput('number', String(pr.number)); core.setOutput('base_sha', pr.base.sha); core.setOutput('base_ref', pr.base.ref); @@ -320,6 +354,7 @@ jobs: REVIEW_MODEL: gpt-6-sol REVIEW_EFFORT: low REVIEW_MAX_DIFF_BYTES: '100000000' + REVIEW_AUTOMATIC_MAX_DIFF_BYTES: '5000000' PULL_REQUEST_NUMBER: ${{ needs.resolve.outputs.number }} PR_BASE_SHA: ${{ needs.resolve.outputs.base_sha }} PR_BASE_REF: ${{ needs.resolve.outputs.base_ref }} @@ -844,6 +879,8 @@ jobs: env: REPOSITORY_DIR: ${{ env.PR_DIFF_REPOSITORY }} MAX_DIFF_BYTES: ${{ env.REVIEW_MAX_DIFF_BYTES }} + AUTOMATIC_MAX_DIFF_BYTES: ${{ env.REVIEW_AUTOMATIC_MAX_DIFF_BYTES }} + LARGE_DIFF_APPROVED: ${{ needs.resolve.outputs.large_diff_approved }} CHUNK_TARGET_BYTES: ${{ inputs.chunk-target-bytes }} READINESS_CONTEXT_SHA256: ${{ steps.prepare_readiness.outputs.context_sha256 }} run: node "$REVIEWER_SOURCE_DIR/.github/scripts/pr-review/prepare.mjs" diff --git a/README.md b/README.md index f778cd7..26ecc3a 100644 --- a/README.md +++ b/README.md @@ -32,6 +32,20 @@ and `actions: write` so the reviewer can restore the latest per-PR Codex session artifact and delete superseded snapshots only after a replacement upload succeeds. +Complete diffs up to 5,000,000 bytes are reviewed automatically. For a larger +diff, an admin of the **calling repository** must post this line as a PR comment +with the current full head commit SHA: + +```text +@codex review approve <40-character-head-sha> +``` + +The shared reviewer checks the comment author's admin permission in the caller +repository and the live head SHA before model work. A push requires a new +approval. This uses the existing `issue_comment` trigger and works for public +and private callers without a GitHub Environment or per-repository approver +configuration. Diffs above 100,000,000 bytes remain rejected. + ## Behavior - Reviews an open, non-draft PR when it is opened, reopened, edited, marked @@ -138,8 +152,8 @@ upload succeeds. - Fork PRs run through the caller repository's trusted default-branch `pull_request_target` workflow and use the caller's explicitly forwarded secret. Secrets from the contributor's fork are not imported or used. -- Automatic PR events intentionally permit an external contributor to consume - review requests when opening, editing, or pushing to an eligible PR. Use a +- Automatic PR events permit an external contributor to consume review + requests up to the 5,000,000-byte automatic limit. Use a dedicated API project with appropriate usage limits and restrict the organization secret to selected repositories. - Complete diffs larger than 100,000,000 bytes fail before Codex runs. The