[Fix] Scope GitHub sync review to the PR's Files Changed, not the rebase range#49
Merged
Merged
Conversation
…ase range The sync re-review computed its since-last-review delta as compare(lastReviewSha...currentHead) — three-dot semantics, so after the branch was rebased onto its base it also contained every commit pulled in from the base branch. The reviewer then flagged findings in files the PR never touched (observed on #44: 3 findings in ado/index.ts, CommsProviderSection.tsx, opencode-runtime.test.ts inherited from develop), and the merged PR permanently showed them as outstanding. The sync review now fetches the PR's authoritative Files Changed (base...head, via gh pr diff) and intersects the range diff and changed-file list with it, dropping rebased-in base-branch files. A rebase-only head change with no PR-relevant delta collapses to the existing no-op path. The review-code skill also records the authoritative file set (pull_request_changed_files) and is instructed to only report findings within it and to disregard base-branch/rebase hunks when running its own local delta diff. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No code issues found. See task
|
…base no-op Addresses the two review findings on #49: - A fetch failure or too-large diff from either gh pr diff or the compare API returns { diff: undefined, changedFiles: [] }, indistinguishable from a genuine empty result. The no-op decision now keeps the delta (reviewer inspects manually) on failure, and falls back to the unscoped range when the PR's authoritative Files Changed cannot be read, instead of silently suppressing the delta. Extracted the decision into a pure, unit-tested resolveScopedSyncReviewDelta helper. - Filtering the three-dot range to PR file paths still re-shows the PR's own already-reviewed files after a rebase, so it cannot collapse to no-op from the API layer. The review-code skill now inspects the delta with a two-dot diff (git diff last..head) scoped to the PR files — the actual content difference between the reviewed commits, which is empty for a rebase-only head change — and marks no_new_delta when it is empty. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Second review round on #49: - knip: dropped the unused exported ScopedSyncReviewDelta interface (the helper now returns an inline-typed object). - A two-dot last..head diff still leaks base-branch hunks on files the PR also touches: it compares the old reviewed tree with the new rebased tree, so a base-branch change to a shared file shows up as a reviewable change even though it is outside the PR's base...head Files Changed. The only leak-free scope is the PR's own base...head diff. resolveScoped- SyncReviewDelta now uses the three-dot range only to pick which PR files changed since the last review, and presents the PR's authoritative base...head hunks for those files (fed as a pull-request diff, not a compare range). The skill makes base...head the sole in-scope change set and uses two-dot only to detect the no-op/rebase-only case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem this solves
Observed on #44 (2026-07-09): after the branch was rebased onto develop, the openmote sync re-review (
mode=sync) flagged 3 findings in files the PR's diff never touched —apps/api/src/handlers/ado/index.ts,apps/web/src/components/settings/CommsProviderSection.tsx,packages/cloud-agents/src/server/__tests__/opencode-runtime.test.ts— all inherited from other commits already on develop. The merged PR now permanently shows "3 issues outstanding" for code that isn't the PR's.Root cause: the sync review computes its since-last-review delta as the GitHub compare
lastReviewSha...currentHead, which uses three-dot semantics. After a rebase,lastReviewShais no longer an ancestor ofcurrentHead, so the range's merge-base is the old base and the diff includes every base-branch commit the rebase pulled in.What changed
githubPrReviewSync.tsnow also fetches the PR's authoritative Files Changed (base...head, viagh pr diff) and intersects the range diff and changed-file list with it, so rebased-in base-branch files are dropped fromdiff_in_rangeandchanged_files_since_last_review. A rebase-only head change that leaves no PR-relevant delta collapses to the existing no-op path.filterUnifiedDiffToFilesutil (keeps only the per-file diff sections whose target path is in the allowed set; leaves a header-less/truncated diff untouched).pull_request_changed_filesand is instructed to report findings only within it, and to restrict its own localgit diff lastReviewSha...HEADto those paths (disregarding base-branch/rebase hunks) — so the model can't re-import them even when it fetches the delta itself.workflow-contracts.md.Validation
New
filterUnifiedDiffToFilesunit tests (keep-allowed, drop-all, keep-all, header-less passthrough); updated the pinned skill-content assertion. Fullcloud-agentsworkflow suite green (201 tests); lint + typecheck clean; guidance knowledge-check OK.🤖 Generated with Claude Code