Skip to content

workflows: diff pull requests from the live base merge base - #31

Merged
idy merged 1 commit into
mainfrom
codex/review-diff-from-merge-base
Sep 7, 2026
Merged

workflows: diff pull requests from the live base merge base#31
idy merged 1 commit into
mainfrom
codex/review-diff-from-merge-base

Conversation

@idy

@idy idy commented Sep 7, 2026

Copy link
Copy Markdown
Member

Summary

The code review stage diffed from pull_request.base.sha. GitHub keeps that value at the base commit recorded when the head was last pushed, so once the base branch moves and the pull request merges it back in, the stale base is no longer an ancestor of every base change inside the head. The incremental two-dot range from the last reviewed head then carried the merged base commits as pull-request changes, and the reviewer reported the base branch's own work as out-of-scope implementation. This happened on GizClaw/gizclaw#1160 after main was merged in to resolve a conflict: the review flagged main's Doubao and Flowcraft commits while GitHub's own diff showed only the PR's files.

  • The fetch step also fetches the live base branch tip (refs/heads/<base_ref>) and exports PR_BASE_TIP_SHA.
  • prepare.mjs starts the full diff and the effective added-line ranges at the merge base between that tip and the head, which is exactly the PR's "Files changed".
  • Reuse and incremental decisions key on merge_base_sha instead of base_sha. 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 never enter a reviewed range.
  • pull_request.base.sha still selects the trusted reviewer checkout only. PR_BASE_TIP_SHA falls back to it when unset, so existing callers of prepare.mjs keep working.
  • README documents the new matrix row and the merge-base rule.

Validation

Command Result
node .github/scripts/pr-review/test.mjs pass; the new merge scenario fails against the previous prepare.mjs and passes now
node .github/scripts/issue-review/test.mjs pass
node .github/scripts/pr-readiness/test.mjs pass
node .github/scripts/review-request/test.mjs pass
Workflow YAML parse ok

Not run: a live workflow run against a real pull request. The base_ref output and PR_BASE_TIP_SHA export follow the existing fetch step's structure, and the script path is covered by the unit scenario.

Not run cleanly: actionlint reports pre-existing job.workflow_sha / job.workflow_repository expression warnings on lines this PR does not touch; no new diagnostics.

Closes #32

🤖 Generated with Claude Code

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 <noreply@anthropic.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ OpenAI PR Review: FAIL

Conclusion: Not ready. 1 readiness blocker must be resolved before merge.

Review checks

Check Result
PR format ❌ FAIL
Issue design ✅ PASS
Code & plan conformance ✅ PASS

Scope: eeedb45085 · b6904f156a..eeedb45085 · full · 1 diff chunk

Usage: 36s · 119,711 tokens · 76.1% cache hit · 3.214 credits

Blockers

  • pr-linkage Pull request must natively close at least one same-repository Issue.

Summary

All completed code chunks were reviewed with no actionable findings or plan-conformance blockers. No linked Issue evidence was supplied.

Review metadata
  • Commit: eeedb45085
  • Range: b6904f156a..eeedb45085
  • Mode: full
  • Diff chunks: 1
  • Model: gpt-5.6-terra
  • Reasoning effort: medium
  • Session: repo-1309321116-pr-31-v2
  • Generation: d4e3d78ea48e188f8943b953d262186223d64a9d79478bf3a127dc56a0095ada
  • Evidence: ef70e94d3fd285702f1312d7951004dd993c449185fefd86d7fa7b75145d94d9

Totals

  • Input: 117,309
  • Cached input: 89,214
  • Cache write: 28,068
  • Output: 2,402
  • Reasoning: 1,364
  • Total: 119,711
  • Estimated credits: 3.214
  • Credit rate per 1M tokens: 62.5 input / 6.25 cached / 375 output
Token and cache details
Stage Mode Target Time Input Cached Hit Output Total Credits
pr deterministic format rules 0s 0 0 0.0% 0 0 0.000
pr full pr 6s 19,561 9,270 47.4% 339 19,900 0.828
code full chunk 1/1 25s 62,742 46,761 74.5% 1,810 64,552 1.970
code full aggregate 5s 35,006 33,183 94.8% 253 35,259 0.416

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ OpenAI PR Review: PASS

Conclusion: Ready from the OpenAI review perspective. PR format, linked Issue design, and code/plan conformance passed with no actionable findings.

Review checks

Check Result
PR format ✅ PASS
Issue design ✅ PASS
Code & plan conformance ✅ PASS

Scope: eeedb45085 · eeedb45085..eeedb45085 · incremental · 1 diff chunk

Usage: 30s · 192,671 tokens · 75.0% cache hit · 4.537 credits

Summary

The incremental generation changes no code and retains the prior full-review result: no actionable findings or plan-conformance blockers. Linked Issue #32 provides a reviewed, implementation-ready plan.

Review metadata
  • Commit: eeedb45085
  • Range: eeedb45085..eeedb45085
  • Mode: incremental
  • Diff chunks: 1
  • Model: gpt-5.6-terra
  • Reasoning effort: medium
  • Session: repo-1309321116-pr-31-v2
  • Generation: 099cffec34e4910239347104b7f21c2f8e0a6417224ba65499b5fbac6d1e0ddf
  • Evidence: 592e3281be52ce5fe705f894267314e0f2583f66f8282d1395f263946afc065f

Totals

  • Input: 190,913
  • Cached input: 143,195
  • Cache write: 47,691
  • Output: 1,758
  • Reasoning: 600
  • Total: 192,671
  • Estimated credits: 4.537
  • Credit rate per 1M tokens: 62.5 input / 6.25 cached / 375 output
Token and cache details
Stage Mode Target Time Input Cached Hit Output Total Credits
pr deterministic format rules 0s 0 0 0.0% 0 0 0.000
pr incremental pr 7s 36,569 17,969 49.1% 304 36,873 1.389
issue full Issue #32 15s 81,792 78,753 96.3% 952 82,744 1.039
code reused chunk 1 0s 0 0 0.0% 0 0 0.000
code incremental aggregate 8s 72,552 46,473 64.1% 502 73,054 2.109

@idy
idy merged commit 9a785c3 into main Sep 7, 2026
19 checks passed
@idy
idy deleted the codex/review-diff-from-merge-base branch September 7, 2026 18:48
idy added a commit that referenced this pull request Sep 7, 2026
…ew (#34)

A full generation re-reviews the complete current diff from its merge
base, which since #31 happens whenever the base branch is merged into
the pull request. The aggregation still received the previous code
review and was told to preserve still-applicable findings, so blockers
about an earlier range survived diffs that no longer contained their
subject. Only an incremental generation now carries the previous
result, and the full-mode prompt says nothing is carried over.

Validation
- node .github/scripts/pr-review/test.mjs (the full-generation
  assertion fails against the previous run.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

Closes #33

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant