Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions .github/scripts/pr-review/prepare.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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) {
Expand All @@ -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")
Expand All @@ -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
) {
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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);
}

Expand Down
98 changes: 98 additions & 0 deletions .github/scripts/pr-review/test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
13 changes: 12 additions & 1 deletion .github/workflows/codex-openai-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
10 changes: 10 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading