diff --git a/.github/scripts/pr-review/prepare.mjs b/.github/scripts/pr-review/prepare.mjs index b6d9fab..dd97d40 100644 --- a/.github/scripts/pr-review/prepare.mjs +++ b/.github/scripts/pr-review/prepare.mjs @@ -38,6 +38,12 @@ const positiveInteger = (name) => { const repo = required("REPOSITORY_DIR"); const stateDir = required("PR_REVIEW_STATE_DIR"); const baseSha = required("PR_BASE_SHA"); +// The live tip of the base branch. GitHub keeps pull_request.base.sha at the +// base commit recorded when the head was last pushed, so after the base branch +// moves and the pull request merges it back in, that stale commit is no longer +// an ancestor of every base change inside the head. Diffing from the merge +// base with the live tip reproduces the pull request's own "Files changed". +const baseTipSha = process.env.PR_BASE_TIP_SHA || baseSha; const headSha = required("PR_HEAD_SHA"); const sessionKey = required("SESSION_KEY"); const readinessContextSha256 = required("READINESS_CONTEXT_SHA256"); @@ -75,7 +81,8 @@ const diffArgs = (from, to, tripleDot = false) => [ `${from}${tripleDot ? "..." : ".."}${to}`, ]; -const fullDiff = Buffer.from(git(repo, diffArgs(baseSha, headSha, true), { +const mergeBase = String(git(repo, ["merge-base", baseTipSha, headSha])).trim(); +const fullDiff = Buffer.from(git(repo, diffArgs(mergeBase, headSha), { encoding: null, })); if (fullDiff.length > maxDiffBytes) { @@ -84,7 +91,6 @@ if (fullDiff.length > maxDiffBytes) { ); } const effectiveDiffSha256 = sha256(fullDiff); -const mergeBase = String(git(repo, ["merge-base", baseSha, headSha])).trim(); const completed = ledger.generations .filter((generation) => generation.status === "completed") @@ -95,7 +101,7 @@ let rangeTripleDot = true; if ( completed && completed.to_sha === headSha - && completed.base_sha === baseSha + && completed.merge_base_sha === mergeBase && completed.effective_diff_sha256 === effectiveDiffSha256 && completed.readiness_context_sha256 === readinessContextSha256 ) { @@ -107,8 +113,12 @@ if ( appendOutput("reused", "true"); process.exit(0); } +// A two-dot range from the last reviewed head only describes pull-request +// work while the merge base is unchanged. Once the base branch is merged into +// the head, that range would present base-branch commits as pull-request +// changes, so the review starts again from the new merge base instead. let completedIsAncestor = false; -if (completed && completed.base_sha === baseSha) { +if (completed && completed.merge_base_sha === mergeBase) { try { git(repo, [ "merge-base", "--is-ancestor", completed.to_sha, headSha, @@ -131,6 +141,7 @@ const generationIdentity = { session_key: sessionKey, mode, base_sha: baseSha, + base_tip_sha: baseTipSha, merge_base_sha: mergeBase, from_sha: fromSha, to_sha: headSha, @@ -319,8 +330,8 @@ for (const record of deltaRecords) { } const effectiveAddedLines = {}; -for (const record of parseNameStatus(baseSha, headSha, true)) { - const patchText = filePatch(record, baseSha, headSha, true); +for (const record of parseNameStatus(mergeBase, headSha)) { + const patchText = filePatch(record, mergeBase, headSha); effectiveAddedLines[record.path] = addedLines(patchText); } diff --git a/.github/scripts/pr-review/test.mjs b/.github/scripts/pr-review/test.mjs index 683c00e..610732e 100644 --- a/.github/scripts/pr-review/test.mjs +++ b/.github/scripts/pr-review/test.mjs @@ -566,6 +566,104 @@ try { assert.equal(updatedLedger.generations.at(-1).mode, "incremental"); assert.equal(updatedLedger.generations.at(-1).from_sha, head); assert.equal(updatedLedger.generations.at(-1).to_sha, nextHead); + assert.equal(updatedLedger.generations.at(-1).merge_base_sha, base); + + // Merging a moved base branch into the pull request must not present the + // base branch's own commits as pull-request changes: the merge base moves, + // so the next generation is a full review from the new merge base whose + // listing excludes the base-only file. The recorded base sha stays stale on + // purpose, mirroring pull_request.base.sha. This runs on copies so the + // original checkpoint chain below is untouched. + const mergedRepo = path.join(temporary, "merged-repo"); + const mergedState = path.join(temporary, "merged-state"); + assert.equal(spawnSync("git", ["clone", "-q", repo, mergedRepo], { + encoding: "utf8", + }).status, 0); + fs.cpSync(state, mergedState, { recursive: true }); + const runMerged = (...args) => { + const result = spawnSync("git", args, { cwd: mergedRepo, encoding: "utf8" }); + assert.equal(result.status, 0, result.stderr); + return result.stdout.trim(); + }; + runMerged("config", "user.name", "Review Test"); + runMerged("config", "user.email", "review@example.com"); + const mergedLedgerBefore = JSON.parse(fs.readFileSync( + path.join(mergedState, "review-ledger.json"), + "utf8", + )); + mergedLedgerBefore.generations.at(-1).status = "completed"; + mergedLedgerBefore.generations.at(-1).completed_at = new Date().toISOString(); + fs.writeFileSync( + path.join(mergedState, "review-ledger.json"), + `${JSON.stringify(mergedLedgerBefore, null, 2)}\n`, + ); + runMerged("checkout", "-q", "-b", "base-branch", base); + fs.writeFileSync(path.join(mergedRepo, "base-only.txt"), "landed on the base branch\n"); + runMerged("add", "base-only.txt"); + runMerged("commit", "-qm", "base branch moves"); + const baseTip = runMerged("rev-parse", "HEAD"); + runMerged("checkout", "-q", "-"); + runMerged("merge", "-q", "--no-edit", "base-branch"); + const mergedHead = runMerged("rev-parse", "HEAD"); + const mergedEnv = { + ...process.env, + REPOSITORY_DIR: mergedRepo, + PR_REVIEW_STATE_DIR: mergedState, + PR_BASE_SHA: base, + PR_BASE_TIP_SHA: baseTip, + PR_HEAD_SHA: mergedHead, + SESSION_KEY: "repo:1:pr:2:v2", + MAX_DIFF_BYTES: "1000000", + CHUNK_TARGET_BYTES: "600", + READINESS_CONTEXT_SHA256: "context-v1", + }; + const merged = spawnSync(process.execPath, [ + path.join(path.dirname(new URL(import.meta.url).pathname), "prepare.mjs"), + ], { cwd: mergedRepo, encoding: "utf8", env: mergedEnv }); + assert.equal(merged.status, 0, merged.stderr); + const mergedLedger = JSON.parse(fs.readFileSync( + path.join(mergedState, "review-ledger.json"), + "utf8", + )); + const mergedGeneration = mergedLedger.generations.at(-1); + assert.equal(mergedGeneration.mode, "full"); + assert.equal(mergedGeneration.merge_base_sha, baseTip); + assert.equal(mergedGeneration.from_sha, baseTip); + assert.equal(mergedGeneration.to_sha, mergedHead); + assert.equal(mergedGeneration.base_sha, base); + assert.equal(mergedGeneration.base_tip_sha, baseTip); + const mergedListing = JSON.parse(fs.readFileSync( + path.join(mergedState, "generations", mergedGeneration.key, "listing.json"), + "utf8", + )); + assert.deepEqual(mergedListing.files.map((file) => file.path), ["large.txt"]); + assert.deepEqual(Object.keys(mergedListing.effective_added_line_ranges), ["large.txt"]); + + // A base branch that moves again without being merged keeps the merge base, + // so an unchanged head reuses the completed generation. + mergedGeneration.status = "completed"; + mergedGeneration.completed_at = new Date().toISOString(); + fs.writeFileSync( + path.join(mergedState, "review-ledger.json"), + `${JSON.stringify(mergedLedger, null, 2)}\n`, + ); + runMerged("checkout", "-q", "base-branch"); + fs.appendFileSync(path.join(mergedRepo, "base-only.txt"), "moves again\n"); + runMerged("add", "base-only.txt"); + runMerged("commit", "-qm", "base branch moves again"); + const movedBaseTip = runMerged("rev-parse", "HEAD"); + runMerged("checkout", "-q", "-"); + const baseMoveOutput = path.join(temporary, "base-move-output.txt"); + fs.writeFileSync(baseMoveOutput, ""); + const reusedAfterBaseMove = spawnSync(process.execPath, [ + path.join(path.dirname(new URL(import.meta.url).pathname), "prepare.mjs"), + ], { + cwd: mergedRepo, + encoding: "utf8", + env: { ...mergedEnv, GITHUB_OUTPUT: baseMoveOutput, PR_BASE_TIP_SHA: movedBaseTip }, + }); + assert.equal(reusedAfterBaseMove.status, 0, reusedAfterBaseMove.stderr); + assert.match(fs.readFileSync(baseMoveOutput, "utf8"), /^mode=reused$/m); const fakeBin = path.join(temporary, "bin"); const codexHome = path.join(temporary, "codex-home"); diff --git a/.github/workflows/codex-openai-review.yml b/.github/workflows/codex-openai-review.yml index 5a6e114..9d5c406 100644 --- a/.github/workflows/codex-openai-review.yml +++ b/.github/workflows/codex-openai-review.yml @@ -94,6 +94,7 @@ jobs: eligible: ${{ steps.pr.outputs.eligible }} number: ${{ steps.pr.outputs.number }} base_sha: ${{ steps.pr.outputs.base_sha }} + base_ref: ${{ steps.pr.outputs.base_ref }} head_sha: ${{ steps.pr.outputs.head_sha }} request_comment_id: ${{ steps.pr.outputs.request_comment_id }} steps: @@ -160,6 +161,7 @@ jobs: core.setOutput('eligible', String(eligible)); core.setOutput('number', String(pr.number)); core.setOutput('base_sha', pr.base.sha); + core.setOutput('base_ref', pr.base.ref); core.setOutput('head_sha', pr.head.sha); core.setOutput('request_comment_id', requestCommentId); @@ -317,6 +319,7 @@ jobs: env: PULL_REQUEST_NUMBER: ${{ needs.resolve.outputs.number }} PR_BASE_SHA: ${{ needs.resolve.outputs.base_sha }} + PR_BASE_REF: ${{ needs.resolve.outputs.base_ref }} PR_HEAD_SHA: ${{ needs.resolve.outputs.head_sha }} REQUEST_COMMENT_ID: ${{ needs.resolve.outputs.request_comment_id }} SESSION_KEY: repo-${{ github.repository_id }}-pr-${{ needs.resolve.outputs.number }}-v2 @@ -369,9 +372,10 @@ jobs: -c "http.extraheader=AUTHORIZATION: basic $basic_auth" \ fetch --no-tags origin \ "+$PR_BASE_SHA:refs/openai-pr-review/base" \ + "+refs/heads/$PR_BASE_REF:refs/openai-pr-review/base-tip" \ "+refs/pull/$PULL_REQUEST_NUMBER/head:$REVIEW_HEAD_REF" then - reason="Could not fetch the exact pull-request base and head objects." + reason="Could not fetch the exact pull-request base, base branch, and head objects." echo "failure_reason=$reason" >> "$GITHUB_OUTPUT" echo "::error title=PR object fetch failed::$reason" exit 1 @@ -392,6 +396,13 @@ jobs: echo "::error title=PR head changed::$reason" exit 1 fi + # The live base branch tip decides the merge base the diff starts + # from; pull_request.base.sha stays at the base commit recorded when + # the head was last pushed and would count base-branch commits merged + # into the head as pull-request changes. + base_tip_sha="$(git --git-dir="$PR_DIFF_REPOSITORY" \ + rev-parse "refs/openai-pr-review/base-tip^{commit}")" + echo "PR_BASE_TIP_SHA=$base_tip_sha" >> "$GITHUB_ENV" - name: Collect untrusted pull-request discussion context id: context diff --git a/README.md b/README.md index 5eace91..446f8db 100644 --- a/README.md +++ b/README.md @@ -214,8 +214,18 @@ stage modes below describe model work and evidence reuse inside that run. | Edit only the PR body | `incremental` field diff | `reused` | `reused` | All three, with only PR metadata re-reviewed | | Edit one linked Issue | `reused` | `incremental` for that Issue; others `reused` | `incremental` plan-conformance aggregation with no repeated complete code diff | All three, with Issue and Code verdicts revalidated | | Push a descendant commit | `reused` | `reused` | `incremental` from the last completed head | All three on the new head | +| Merge the moved base branch into the PR | `reused` | `reused` | `full` from the new merge base; base-branch commits are never reviewed as PR changes | All three on the merge commit | | Rerun an unchanged head | deterministic checks plus `reused` | `reused` | `reused` | All three with zero model tokens | +The code diff always starts at the merge base between the live base branch +tip and the PR head, matching the PR's own "Files changed" view. +`pull_request.base.sha` stays at the base commit recorded when the head was +last pushed, so it is only used to check out trusted reviewer code, never to +decide what counts as a PR change. A base branch that moves without being +merged keeps the merge base and reuses evidence; merging it moves the merge +base and starts a fresh full code review so base-branch commits do not appear +as pull-request changes. + An Issue edit revalidates plan conformance without resending an unchanged complete code diff. A runtime, model, or trusted-policy change intentionally invalidates incompatible session evidence and safely starts the affected