From eeedb450859abd5dd8302e58b8ab484b180b9ddc Mon Sep 17 00:00:00 2001 From: idy Date: Tue, 8 Sep 2026 02:39:54 +0800 Subject: [PATCH] workflows: diff pull requests from the live base merge base The code review diffed from pull_request.base.sha, which GitHub keeps at the base commit recorded when the head was last pushed. After the base branch moved and the pull request merged it back in, that stale commit was no longer an ancestor of every base change inside the head, and the incremental two-dot range from the last reviewed head carried the merged base commits as pull-request changes. The reviewer then reported the base branch's own work as out-of-scope implementation. The fetch step now also fetches the live base branch tip, and the diff listing starts at the merge base between that tip and the head, which is what the pull request's own "Files changed" view shows. Reuse and incremental decisions key on the merge base instead of the recorded base sha: a base branch that moves without being merged keeps reusing evidence, while merging it moves the merge base and starts a fresh full code review. pull_request.base.sha still selects the trusted reviewer checkout only. Validation - node .github/scripts/pr-review/test.mjs (new merge scenario fails against the previous prepare.mjs and passes now) - node .github/scripts/issue-review/test.mjs - node .github/scripts/pr-readiness/test.mjs - node .github/scripts/review-request/test.mjs - workflow YAML parses Co-Authored-By: Claude Fable 5.1 --- .github/scripts/pr-review/prepare.mjs | 23 ++++-- .github/scripts/pr-review/test.mjs | 98 +++++++++++++++++++++++ .github/workflows/codex-openai-review.yml | 13 ++- README.md | 10 +++ 4 files changed, 137 insertions(+), 7 deletions(-) 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