Skip to content

bug: Review the merge-base diff instead of the recorded base sha #32

Description

@idy

Background

The code review stage of codex-openai-review.yml decides what counts as a
pull-request change from pull_request.base.sha. resolve outputs
pr.base.sha, the fetch step pins it as refs/openai-pr-review/base, and
.github/scripts/pr-review/prepare.mjs builds the effective diff as
baseSha...headSha and the incremental range as completed.to_sha..headSha.

GitHub keeps pull_request.base.sha at the base branch commit recorded when
the head was last pushed. It is not the live base tip. Once the base branch
moves and the pull request merges it back in, the recorded base is no longer an
ancestor of every base commit inside the head, so both ranges include the
merged base commits as if the pull request had written them.

Confirmed live on GizClaw/gizclaw#1160, which consumes this workflow at
@latest. The branch merged origin/main to resolve a conflict. GitHub's own
"Files changed" showed only the 35 files of the change, while the review
reported main's Doubao agent-initiative and Flowcraft outbox commits as
"unrelated implementation" that must be split out, and kept doing so on every
later run because the incremental chain started from that generation. The
only way to satisfy the reviewer was a rebase and force-push, which the
consumer's process forbids.

Goal

  • Review exactly the diff GitHub shows for the pull request: from the merge
    base between the live base branch tip and the head.
  • Never present base-branch commits merged into the head as pull-request
    changes, in full or incremental mode.
  • Keep zero-token evidence reuse when the base branch moves without being
    merged.

Non-goals

  • Do not change which commit the trusted reviewer implementation and the
    trusted base checkout come from; pull_request.base.sha keeps that role.
  • Do not change the PR or Issue stages, Check Runs, reactions, or the session
    and evidence model beyond the code-stage keys named below.

Code Changes Tree

.github/
├── scripts/
│   └── pr-review/
│       ├── prepare.mjs            # diff from merge-base(live base tip, head); key reuse and incremental on merge_base_sha
│       └── test.mjs               # merge a moved base into the PR: full review from the new merge base without base-only files; unmerged base move reuses
└── workflows/
    └── codex-openai-review.yml    # resolve outputs base_ref; fetch the live base tip and export PR_BASE_TIP_SHA
README.md                          # matrix row and the merge-base rule

Design

resolve outputs base_ref beside base_sha. The fetch step also fetches
+refs/heads/$PR_BASE_REF:refs/openai-pr-review/base-tip into the bare diff
repository and exports its commit as PR_BASE_TIP_SHA through GITHUB_ENV.
The existing exact-object checks for the recorded base and the head stay.

prepare.mjs reads PR_BASE_TIP_SHA, falling back to PR_BASE_SHA when a
caller does not provide it, and computes mergeBase from that tip and the
head. The full diff, the effective diff hash, and the effective added-line
ranges all start at mergeBase. The reuse check and the incremental
precondition compare completed.merge_base_sha with the current merge base
instead of completed.base_sha. Merging the base branch moves the merge base,
so the next generation is a full review from the new merge base and no
two-dot range ever spans base commits. A base branch that moves without being
merged keeps the merge base, so an unchanged head still reports reused.
generationIdentity records base_tip_sha beside the existing base_sha and
merge_base_sha; base_sha keeps its trusted-checkout meaning for the code
stage identity.

Test And Acceptance Criteria

Acceptance Criteria

  • After the base branch moves and is merged into the pull request, the next
    generation is full, its from_sha and merge_base_sha equal the live base
    tip, and its listing contains only files the pull request changed.
  • After the base branch moves again without being merged, an unchanged head
    reports mode=reused.
  • The recorded base_sha stays what pull_request.base.sha reported, and the
    trusted checkout still uses it.
  • The new scenario fails against the previous prepare.mjs.

Validation

  • node .github/scripts/pr-review/test.mjs
  • node .github/scripts/issue-review/test.mjs
  • node .github/scripts/pr-readiness/test.mjs
  • node .github/scripts/review-request/test.mjs
  • actionlint .github/workflows/codex-openai-review.yml

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions