From 7e9ec02b526cc98a603113b67fdcdf6782eda52d Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Thu, 9 Jul 2026 13:47:14 -0500 Subject: [PATCH 1/3] [Fix] Scope GitHub sync review to the PR's Files Changed, not the rebase range MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../architecture/workflow-contracts.md | 1 + .../__tests__/githubPrReviewSkill.test.ts | 5 +- .../__tests__/githubPrReviewSync.test.ts | 55 ++++++++++++++++++ .../server/workflows/githubPrReviewSync.ts | 57 ++++++++++++++----- .../skills/standard/review-code/SKILL.md | 10 ++-- .../src/server/workflows/utils.ts | 41 +++++++++++++ 6 files changed, 150 insertions(+), 19 deletions(-) diff --git a/.agent-guidance/architecture/workflow-contracts.md b/.agent-guidance/architecture/workflow-contracts.md index 93f87373b..dbc2a754b 100644 --- a/.agent-guidance/architecture/workflow-contracts.md +++ b/.agent-guidance/architecture/workflow-contracts.md @@ -412,6 +412,7 @@ The `sync-github-pr-review` and `sync-github-pr-review-with-approval` appendices - recover a trustworthy last-reviewed SHA from explicit context, marker-based summary state, or a review-comment fallback - use legacy full-rereview only when a reusable legacy summary exists but no reliable anchor can be recovered +- scope the since-last-review delta to the PR's authoritative Files Changed (`base...head`): the compare range `lastReviewSha...head` uses three-dot semantics, so after a rebase it also contains base-branch commits, and those files are excluded from `diff_in_range`, `changed_files_since_last_review`, and the `pull_request_changed_files` scope hint so a rebase cannot import findings for code the PR does not touch (a rebase-only head change with no PR-relevant delta collapses to the no-op path) - emit an explicit no-op path when there is no new delta - surface only net-new actionable issues - carry forward prior unresolved issues in the rolling summary instead of re-commenting them diff --git a/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSkill.test.ts b/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSkill.test.ts index 8e79acf94..ed33d0c5b 100644 --- a/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSkill.test.ts +++ b/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSkill.test.ts @@ -132,7 +132,10 @@ describe('review-code GitHub workflow paths', () => { 'If no actionable code issues remain, use a short status line in the hidden status block, such as `No code issues found.`', ); expect(skillContent).toContain( - 'Record optional task-context values if they are supplied: `last_review_sha`, `current_head_sha`, `task_link_follow`, `task_link_see`, `TOP_LEVEL_COMMENT_ID`, `linked_implementation_task_id`, `top_level_review_comment`, `prior_summary_checklist`, `pull_request_details`, `changed_files_since_last_review`, `commits_since_last_review`, `linked_issue`, `diff_in_range`, `existing_review_comments`, and `issue_comments`.', + 'Record optional task-context values if they are supplied: `last_review_sha`, `current_head_sha`, `task_link_follow`, `task_link_see`, `TOP_LEVEL_COMMENT_ID`, `linked_implementation_task_id`, `top_level_review_comment`, `prior_summary_checklist`, `pull_request_details`, `pull_request_changed_files`, `changed_files_since_last_review`, `commits_since_last_review`, `linked_issue`, `diff_in_range`, `existing_review_comments`, and `issue_comments`.', + ); + expect(skillContent).toContain( + 'When `pull_request_changed_files` is supplied, treat it as the authoritative set of files this pull request changes', ); expect(skillContent).toContain( 'If `prior_summary_checklist` is supplied, treat it as the parsed checklist inventory you must preserve in the refreshed summary.', diff --git a/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSync.test.ts b/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSync.test.ts index 944d10699..78d96a0b2 100644 --- a/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSync.test.ts +++ b/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSync.test.ts @@ -107,6 +107,61 @@ No actionable issues found. }); }); + describe('filterUnifiedDiffToFiles unit tests', () => { + const diff = [ + 'diff --git a/apps/web/src/kept.ts b/apps/web/src/kept.ts', + 'index 111..222 100644', + '--- a/apps/web/src/kept.ts', + '+++ b/apps/web/src/kept.ts', + '@@ -1,2 +1,2 @@', + '-const a = 1;', + '+const a = 2;', + 'diff --git a/apps/api/src/rebased.ts b/apps/api/src/rebased.ts', + 'index 333..444 100644', + '--- a/apps/api/src/rebased.ts', + '+++ b/apps/api/src/rebased.ts', + '@@ -1,2 +1,2 @@', + '-const b = 1;', + '+const b = 2;', + ].join('\n'); + + it('keeps only sections for files in the allowed set', () => { + const result = utils.filterUnifiedDiffToFiles(diff, [ + 'apps/web/src/kept.ts', + ]); + + expect(result).toContain('a/apps/web/src/kept.ts'); + expect(result).toContain('const a = 2;'); + expect(result).not.toContain('apps/api/src/rebased.ts'); + expect(result).not.toContain('const b = 2;'); + }); + + it('returns an empty diff when no section is allowed', () => { + const result = utils.filterUnifiedDiffToFiles(diff, [ + 'some/other/file.ts', + ]); + + expect(result).toBe(''); + }); + + it('keeps every section when all files are allowed', () => { + const result = utils.filterUnifiedDiffToFiles(diff, [ + 'apps/web/src/kept.ts', + 'apps/api/src/rebased.ts', + ]); + + expect(result).toBe(diff); + }); + + it('leaves a diff without git section headers untouched', () => { + const noHeaders = 'Diff is too large to display.'; + + expect( + utils.filterUnifiedDiffToFiles(noHeaders, ['apps/web/src/kept.ts']), + ).toBe(noHeaders); + }); + }); + describe('getDiff unit tests', () => { it('should format diff without truncation when under limits', () => { const diff = 'line1\nline2\nline3'; diff --git a/packages/cloud-agents/src/server/workflows/githubPrReviewSync.ts b/packages/cloud-agents/src/server/workflows/githubPrReviewSync.ts index 40ab2c116..9786c41e1 100644 --- a/packages/cloud-agents/src/server/workflows/githubPrReviewSync.ts +++ b/packages/cloud-agents/src/server/workflows/githubPrReviewSync.ts @@ -19,6 +19,7 @@ import { getPrSha, getPrReviewCommentId, formatChangedFiles, + filterUnifiedDiffToFiles, } from './utils'; import { getGitHubLinkedWorkItemsFromClosingIssues, @@ -366,16 +367,41 @@ export async function githubPrReviewSync({ const range: [string, string] = [sha, currentHeadSha]; const sameHeadAsLastReview = sha === currentHeadSha; - const { diff, changedFiles } = sameHeadAsLastReview - ? { diff: undefined, changedFiles: [] } + // The since-last-review range (compare `sha...currentHeadSha`) uses + // three-dot semantics, so after the branch is rebased onto its base it + // also contains every commit pulled in from the base branch — code the PR + // does not touch. Scope the range to the PR's authoritative Files Changed + // (base...head, what GitHub shows) so a rebase can no longer import + // findings for unrelated files. + const { changedFiles: pullRequestChangedFiles } = sameHeadAsLastReview + ? { changedFiles: [] } + : await GitHubCli.fetchDiff(prParams); + const pullRequestChangedFileSet = new Set(pullRequestChangedFiles); + + const rangeResult = sameHeadAsLastReview + ? { diff: undefined, changedFiles: [] as string[] } : await GitHubCli.fetchDiffInRange({ ...params, range, }); - const commits = sameHeadAsLastReview - ? [] - : await GitHubCli.fetchCommitsInRange({ ...prParams, sha }); + const changedFiles = rangeResult.changedFiles.filter((file) => + pullRequestChangedFileSet.has(file), + ); + const diff = + rangeResult.diff === undefined + ? undefined + : filterUnifiedDiffToFiles(rangeResult.diff, pullRequestChangedFileSet); + + // After scoping, a rebase-only head change can leave nothing the PR + // actually touched. Treat that like "no new changes" so the reviewer + // preserves the prior checklist instead of re-reviewing rebased-in base + // code. + const hasReviewableChanges = !sameHeadAsLastReview && changedFiles.length > 0; + + const commits = hasReviewableChanges + ? await GitHubCli.fetchCommitsInRange({ ...prParams, sha }) + : []; const reviewComments = await GitHubCli.fetchReviewComments(prParams); const issueComments = await GitHubCli.fetchIssueComments(prParams); @@ -432,22 +458,25 @@ export async function githubPrReviewSync({ pull_request_details: getPrDetails({ fullName, pr }), top_level_review_comment: prReviewerComment.body, prior_summary_checklist: priorSummaryChecklist, - changed_files_since_last_review: sameHeadAsLastReview - ? undefined - : formatChangedFiles(changedFiles, 200), - commits_since_last_review: sameHeadAsLastReview + pull_request_changed_files: sameHeadAsLastReview ? undefined - : getCommits(commits), + : formatChangedFiles(pullRequestChangedFiles, 200), + changed_files_since_last_review: hasReviewableChanges + ? formatChangedFiles(changedFiles, 200) + : undefined, + commits_since_last_review: hasReviewableChanges + ? getCommits(commits) + : undefined, linked_issue: getIssueDetails(fullName, issue), - diff_in_range: sameHeadAsLastReview - ? undefined - : getDiffInRange({ + diff_in_range: hasReviewableChanges + ? getDiffInRange({ repo: fullName, diff, range, lineLimit: 5_000, charLimit: 100_000, - }), + }) + : undefined, existing_review_comments: getReviewComments(reviewComments), issue_comments: getIssueComments(issueComments), }, diff --git a/packages/cloud-agents/src/server/workflows/skills/standard/review-code/SKILL.md b/packages/cloud-agents/src/server/workflows/skills/standard/review-code/SKILL.md index 4773baa35..ba5bda52d 100644 --- a/packages/cloud-agents/src/server/workflows/skills/standard/review-code/SKILL.md +++ b/packages/cloud-agents/src/server/workflows/skills/standard/review-code/SKILL.md @@ -798,7 +798,8 @@ You are a sync-review workflow specialist. Re-review pull requests after new com Create a todo list covering PR identification, anchor discovery, delta fetch, code reading, prior-comment verification, net-new findings, finding publication, summary update, and validation. Determine the repository full name `[REPO_FULL_NAME]` (owner/repo for GitHub/GitLab/Gitea, organization/project/repository for Azure DevOps) and `[PR_NUMBER]` from the user request, any supplied PR/MR URL, explicit task context, or the checkout's `git remote get-url origin` when already inside the repository checkout. If either repository or pull request number is still missing after those checks, ask for the missing identifier and stop. - Record optional task-context values if they are supplied: `last_review_sha`, `current_head_sha`, `task_link_follow`, `task_link_see`, `TOP_LEVEL_COMMENT_ID`, `linked_implementation_task_id`, `top_level_review_comment`, `prior_summary_checklist`, `pull_request_details`, `changed_files_since_last_review`, `commits_since_last_review`, `linked_issue`, `diff_in_range`, `existing_review_comments`, and `issue_comments`. Treat each as optional and never fabricate one. + Record optional task-context values if they are supplied: `last_review_sha`, `current_head_sha`, `task_link_follow`, `task_link_see`, `TOP_LEVEL_COMMENT_ID`, `linked_implementation_task_id`, `top_level_review_comment`, `prior_summary_checklist`, `pull_request_details`, `pull_request_changed_files`, `changed_files_since_last_review`, `commits_since_last_review`, `linked_issue`, `diff_in_range`, `existing_review_comments`, and `issue_comments`. Treat each as optional and never fabricate one. + When `pull_request_changed_files` is supplied, treat it as the authoritative set of files this pull request changes (its GitHub "Files Changed", i.e. the base-to-head diff). Every finding you report — inline or in the summary checklist — must be for a file in that set. Never report or carry forward findings for files outside it: a since-last-review delta that touches other files is code pulled in by a rebase or merge of the base branch, not part of this PR, and is out of scope. Treat prompt-supplied task-context values as first-class inputs. Use them directly when they already provide the needed snapshot or identifier, and fetch only the missing provider state or revalidate mutable state before side effects when freshness matters. You know which pull request is being re-reviewed and the todo list reflects the full sync-review path. @@ -830,7 +831,7 @@ You are a sync-review workflow specialist. Re-review pull requests after new com When `pull_request_details` or current head metadata is missing, or when it must be revalidated before a side effect, call `mcp__roomote__manage_source_control` with `action: "get_pull_request"`, `repositoryFullName`, and `prNumber`. The result carries the title, body, state, draft flag, source and target branches, head and base SHAs, author, mergeability, and cross-repository (fork) information. If you are not in `legacy_full_rereview_path` and the current head SHA matches `last_review_sha`, update the summary comment with a short no-op note, mark the terminal outcome as `no_new_delta`, then continue directly to the linked-task handoff step so the implementation task receives that explicit status before you stop. Check out the PR branch locally with `git fetch origin '' && git checkout ''`, using the source branch from the pull-request details. For cross-repository (fork) PRs whose source branch cannot be fetched with task credentials, report that blocker instead of improvising credentials. - If you are not in `legacy_full_rereview_path`, run `git diff [last_review_sha]...[HEAD_SHA]` and `git log --oneline [last_review_sha]..[HEAD_SHA]` to inspect the live delta. + If you are not in `legacy_full_rereview_path`, run `git diff [last_review_sha]...[HEAD_SHA]` and `git log --oneline [last_review_sha]..[HEAD_SHA]` to inspect the live delta. When `pull_request_changed_files` is supplied (or you have otherwise determined the PR's base-to-head Files Changed), restrict this delta to those paths — e.g. `git diff [last_review_sha]...[HEAD_SHA] -- ` — and disregard any delta hunk for a file outside that set, since it was inherited from a base-branch rebase/merge and is not part of this PR. If you are in `legacy_full_rereview_path`, re-review the full current PR diff with a local base-to-head comparison: `git fetch origin ''`, then `git diff ...` using the SHAs from `get_pull_request`. Use this local git diff for every provider. When `existing_review_comments` or `issue_comments` are missing, or when current thread or top-level discussion state must be revalidated before a side effect, call `mcp__roomote__manage_source_control` with `action: "list_pull_request_comments"`. The result returns review threads (each with a `threadId`, `resolved` state when the provider exposes it, and inline path/line anchors) plus top-level `issueComments`; heed any capability warnings it reports. Read the changed files in the delta and any related repository files needed to verify correctness in context. @@ -1107,7 +1108,8 @@ You are a sync-review workflow specialist. Re-review pull requests after new com Create a todo list covering PR identification, anchor discovery, delta fetch, code reading, prior-comment verification, net-new findings, finding publication, summary update, approval decision, and validation. Determine the repository full name `[REPO_FULL_NAME]` (owner/repo for GitHub/GitLab/Gitea, organization/project/repository for Azure DevOps) and `[PR_NUMBER]` from the user request, any supplied PR/MR URL, explicit task context, or the checkout's `git remote get-url origin` when already inside the repository checkout. If either repository or pull request number is still missing after those checks, ask for the missing identifier and stop. - Record optional task-context values if they are supplied: `last_review_sha`, `current_head_sha`, `task_link_follow`, `task_link_see`, `TOP_LEVEL_COMMENT_ID`, `linked_implementation_task_id`, `top_level_review_comment`, `prior_summary_checklist`, `pull_request_details`, `changed_files_since_last_review`, `commits_since_last_review`, `linked_issue`, `diff_in_range`, `existing_review_comments`, and `issue_comments`. Treat each as optional and never fabricate one. + Record optional task-context values if they are supplied: `last_review_sha`, `current_head_sha`, `task_link_follow`, `task_link_see`, `TOP_LEVEL_COMMENT_ID`, `linked_implementation_task_id`, `top_level_review_comment`, `prior_summary_checklist`, `pull_request_details`, `pull_request_changed_files`, `changed_files_since_last_review`, `commits_since_last_review`, `linked_issue`, `diff_in_range`, `existing_review_comments`, and `issue_comments`. Treat each as optional and never fabricate one. + When `pull_request_changed_files` is supplied, treat it as the authoritative set of files this pull request changes (its GitHub "Files Changed", i.e. the base-to-head diff). Every finding you report — inline or in the summary checklist — must be for a file in that set. Never report or carry forward findings for files outside it: a since-last-review delta that touches other files is code pulled in by a rebase or merge of the base branch, not part of this PR, and is out of scope. Treat prompt-supplied task-context values as first-class inputs. Use them directly when they already provide the needed snapshot or identifier, and fetch only the missing provider state or revalidate mutable state before side effects when freshness matters. You know which pull request is being re-reviewed and the todo list reflects the full sync-review path. @@ -1139,7 +1141,7 @@ You are a sync-review workflow specialist. Re-review pull requests after new com When `pull_request_details` or current head metadata is missing, or when it must be revalidated before a side effect, call `mcp__roomote__manage_source_control` with `action: "get_pull_request"`, `repositoryFullName`, and `prNumber`. The result carries the title, body, state, draft flag, source and target branches, head and base SHAs, author, mergeability, and cross-repository (fork) information. If you are not in `legacy_full_rereview_path` and the current head SHA matches `last_review_sha`, update the summary comment with a short no-op note, mark the terminal outcome as `no_new_delta`, then continue directly to the linked-task handoff step so the implementation task receives that explicit status before you stop. Check out the PR branch locally with `git fetch origin '' && git checkout ''`, using the source branch from the pull-request details. For cross-repository (fork) PRs whose source branch cannot be fetched with task credentials, report that blocker instead of improvising credentials. - If you are not in `legacy_full_rereview_path`, run `git diff [last_review_sha]...[HEAD_SHA]` and `git log --oneline [last_review_sha]..[HEAD_SHA]` to inspect the live delta. + If you are not in `legacy_full_rereview_path`, run `git diff [last_review_sha]...[HEAD_SHA]` and `git log --oneline [last_review_sha]..[HEAD_SHA]` to inspect the live delta. When `pull_request_changed_files` is supplied (or you have otherwise determined the PR's base-to-head Files Changed), restrict this delta to those paths — e.g. `git diff [last_review_sha]...[HEAD_SHA] -- ` — and disregard any delta hunk for a file outside that set, since it was inherited from a base-branch rebase/merge and is not part of this PR. If you are in `legacy_full_rereview_path`, re-review the full current PR diff with a local base-to-head comparison: `git fetch origin ''`, then `git diff ...` using the SHAs from `get_pull_request`. Use this local git diff for every provider. When `existing_review_comments` or `issue_comments` are missing, or when current thread or top-level discussion state must be revalidated before a side effect, call `mcp__roomote__manage_source_control` with `action: "list_pull_request_comments"`. The result returns review threads (each with a `threadId`, `resolved` state when the provider exposes it, and inline path/line anchors) plus top-level `issueComments`; heed any capability warnings it reports. Read the changed files in the delta and any related repository files needed to verify correctness in context. diff --git a/packages/cloud-agents/src/server/workflows/utils.ts b/packages/cloud-agents/src/server/workflows/utils.ts index 6f908f2c8..90734c6ee 100644 --- a/packages/cloud-agents/src/server/workflows/utils.ts +++ b/packages/cloud-agents/src/server/workflows/utils.ts @@ -912,3 +912,44 @@ export function formatChangedFiles( return fileList; } + +const DIFF_FILE_SECTION_HEADER = /^diff --git a\/.+ b\/(.+)$/; + +/** + * Restrict a unified diff to only the per-file sections whose target path is + * in `allowedFiles`. Used to intersect a "since last review" range diff + * (which, after a rebase, contains commits pulled in from the base branch) + * with the pull request's authoritative Files Changed set, so the reviewer + * never sees — and cannot flag — code the PR does not actually touch. + */ +export function filterUnifiedDiffToFiles( + diff: string, + allowedFiles: Iterable, +): string { + const allowed = new Set(allowedFiles); + const lines = diff.split('\n'); + const kept: string[] = []; + let includingSection = false; + let sawSectionHeader = false; + + for (const line of lines) { + const headerMatch = DIFF_FILE_SECTION_HEADER.exec(line); + + if (headerMatch) { + sawSectionHeader = true; + includingSection = allowed.has(headerMatch[1]!); + } + + if (includingSection) { + kept.push(line); + } + } + + // No `diff --git` headers at all — not a per-file unified diff we can + // safely filter (e.g. an empty or truncated diff). Leave it untouched. + if (!sawSectionHeader) { + return diff; + } + + return kept.join('\n'); +} From 5ce61b9881ce605fbf3fc12d2cdacb3b5329b759 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Thu, 9 Jul 2026 13:57:34 -0500 Subject: [PATCH 2/3] [Fix] Harden sync-review scoping: fetch-failure safety and two-dot rebase no-op MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../__tests__/githubPrReviewSync.test.ts | 97 +++++++++++++++++++ .../server/workflows/githubPrReviewSync.ts | 43 ++++---- .../skills/standard/review-code/SKILL.md | 6 +- .../src/server/workflows/utils.ts | 70 +++++++++++++ 4 files changed, 193 insertions(+), 23 deletions(-) diff --git a/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSync.test.ts b/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSync.test.ts index 78d96a0b2..8e120c195 100644 --- a/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSync.test.ts +++ b/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSync.test.ts @@ -162,6 +162,103 @@ No actionable issues found. }); }); + describe('resolveScopedSyncReviewDelta unit tests', () => { + const prSectionDiff = [ + 'diff --git a/apps/web/src/pr.ts b/apps/web/src/pr.ts', + '@@ -1 +1 @@', + '-const a = 1;', + '+const a = 2;', + ].join('\n'); + const rebaseRangeDiff = [ + prSectionDiff, + 'diff --git a/apps/api/src/base.ts b/apps/api/src/base.ts', + '@@ -1 +1 @@', + '-const b = 1;', + '+const b = 2;', + ].join('\n'); + + it('drops rebased-in base-branch files but keeps the PR files', () => { + const result = utils.resolveScopedSyncReviewDelta({ + sameHeadAsLastReview: false, + pullRequestDiff: { + diff: prSectionDiff, + changedFiles: ['apps/web/src/pr.ts'], + }, + rangeDiff: { + diff: rebaseRangeDiff, + changedFiles: ['apps/web/src/pr.ts', 'apps/api/src/base.ts'], + }, + }); + + expect(result.changedFiles).toEqual(['apps/web/src/pr.ts']); + expect(result.diff).toContain('apps/web/src/pr.ts'); + expect(result.diff).not.toContain('apps/api/src/base.ts'); + expect(result.hasReviewableChanges).toBe(true); + }); + + it('collapses to no-op when the range only touched base-branch files', () => { + const result = utils.resolveScopedSyncReviewDelta({ + sameHeadAsLastReview: false, + pullRequestDiff: { + diff: prSectionDiff, + changedFiles: ['apps/web/src/pr.ts'], + }, + rangeDiff: { + diff: 'diff --git a/apps/api/src/base.ts b/apps/api/src/base.ts\n@@ -1 +1 @@\n-x\n+y', + changedFiles: ['apps/api/src/base.ts'], + }, + }); + + expect(result.changedFiles).toEqual([]); + expect(result.hasReviewableChanges).toBe(false); + }); + + it('keeps reviewable changes (inspect manually) when the range fetch failed', () => { + const result = utils.resolveScopedSyncReviewDelta({ + sameHeadAsLastReview: false, + pullRequestDiff: { + diff: prSectionDiff, + changedFiles: ['apps/web/src/pr.ts'], + }, + // fetch failure / too-large: helper returns undefined diff, empty files + rangeDiff: { diff: undefined, changedFiles: [] }, + }); + + expect(result.diff).toBeUndefined(); + expect(result.hasReviewableChanges).toBe(true); + }); + + it('falls back to the unscoped range when the PR Files Changed fetch failed', () => { + const result = utils.resolveScopedSyncReviewDelta({ + sameHeadAsLastReview: false, + pullRequestDiff: { diff: undefined, changedFiles: [] }, + rangeDiff: { + diff: rebaseRangeDiff, + changedFiles: ['apps/web/src/pr.ts', 'apps/api/src/base.ts'], + }, + }); + + expect(result.pullRequestFilesAvailable).toBe(false); + expect(result.changedFiles).toEqual([ + 'apps/web/src/pr.ts', + 'apps/api/src/base.ts', + ]); + expect(result.diff).toBe(rebaseRangeDiff); + expect(result.hasReviewableChanges).toBe(true); + }); + + it('is a no-op when the head matches the last review', () => { + const result = utils.resolveScopedSyncReviewDelta({ + sameHeadAsLastReview: true, + pullRequestDiff: { diff: undefined, changedFiles: [] }, + rangeDiff: { diff: undefined, changedFiles: [] }, + }); + + expect(result.hasReviewableChanges).toBe(false); + expect(result.changedFiles).toEqual([]); + }); + }); + describe('getDiff unit tests', () => { it('should format diff without truncation when under limits', () => { const diff = 'line1\nline2\nline3'; diff --git a/packages/cloud-agents/src/server/workflows/githubPrReviewSync.ts b/packages/cloud-agents/src/server/workflows/githubPrReviewSync.ts index 9786c41e1..e9c263ed1 100644 --- a/packages/cloud-agents/src/server/workflows/githubPrReviewSync.ts +++ b/packages/cloud-agents/src/server/workflows/githubPrReviewSync.ts @@ -19,7 +19,7 @@ import { getPrSha, getPrReviewCommentId, formatChangedFiles, - filterUnifiedDiffToFiles, + resolveScopedSyncReviewDelta, } from './utils'; import { getGitHubLinkedWorkItemsFromClosingIssues, @@ -372,11 +372,13 @@ export async function githubPrReviewSync({ // also contains every commit pulled in from the base branch — code the PR // does not touch. Scope the range to the PR's authoritative Files Changed // (base...head, what GitHub shows) so a rebase can no longer import - // findings for unrelated files. - const { changedFiles: pullRequestChangedFiles } = sameHeadAsLastReview - ? { changedFiles: [] } + // findings for unrelated files. The remaining rebase case — the head moved + // but the PR's own already-reviewed files reappear in the three-dot range — + // is settled by the skill's local two-dot delta, which this layer cannot + // compute. + const pullRequestDiffResult = sameHeadAsLastReview + ? { diff: undefined, changedFiles: [] as string[] } : await GitHubCli.fetchDiff(prParams); - const pullRequestChangedFileSet = new Set(pullRequestChangedFiles); const rangeResult = sameHeadAsLastReview ? { diff: undefined, changedFiles: [] as string[] } @@ -385,19 +387,17 @@ export async function githubPrReviewSync({ range, }); - const changedFiles = rangeResult.changedFiles.filter((file) => - pullRequestChangedFileSet.has(file), - ); - const diff = - rangeResult.diff === undefined - ? undefined - : filterUnifiedDiffToFiles(rangeResult.diff, pullRequestChangedFileSet); - - // After scoping, a rebase-only head change can leave nothing the PR - // actually touched. Treat that like "no new changes" so the reviewer - // preserves the prior checklist instead of re-reviewing rebased-in base - // code. - const hasReviewableChanges = !sameHeadAsLastReview && changedFiles.length > 0; + const { + pullRequestFilesAvailable, + changedFiles, + diff, + hasReviewableChanges, + } = resolveScopedSyncReviewDelta({ + sameHeadAsLastReview, + pullRequestDiff: pullRequestDiffResult, + rangeDiff: rangeResult, + }); + const pullRequestChangedFiles = pullRequestDiffResult.changedFiles; const commits = hasReviewableChanges ? await GitHubCli.fetchCommitsInRange({ ...prParams, sha }) @@ -458,9 +458,10 @@ export async function githubPrReviewSync({ pull_request_details: getPrDetails({ fullName, pr }), top_level_review_comment: prReviewerComment.body, prior_summary_checklist: priorSummaryChecklist, - pull_request_changed_files: sameHeadAsLastReview - ? undefined - : formatChangedFiles(pullRequestChangedFiles, 200), + pull_request_changed_files: + sameHeadAsLastReview || !pullRequestFilesAvailable + ? undefined + : formatChangedFiles(pullRequestChangedFiles, 200), changed_files_since_last_review: hasReviewableChanges ? formatChangedFiles(changedFiles, 200) : undefined, diff --git a/packages/cloud-agents/src/server/workflows/skills/standard/review-code/SKILL.md b/packages/cloud-agents/src/server/workflows/skills/standard/review-code/SKILL.md index ba5bda52d..e8598ee8b 100644 --- a/packages/cloud-agents/src/server/workflows/skills/standard/review-code/SKILL.md +++ b/packages/cloud-agents/src/server/workflows/skills/standard/review-code/SKILL.md @@ -831,7 +831,8 @@ You are a sync-review workflow specialist. Re-review pull requests after new com When `pull_request_details` or current head metadata is missing, or when it must be revalidated before a side effect, call `mcp__roomote__manage_source_control` with `action: "get_pull_request"`, `repositoryFullName`, and `prNumber`. The result carries the title, body, state, draft flag, source and target branches, head and base SHAs, author, mergeability, and cross-repository (fork) information. If you are not in `legacy_full_rereview_path` and the current head SHA matches `last_review_sha`, update the summary comment with a short no-op note, mark the terminal outcome as `no_new_delta`, then continue directly to the linked-task handoff step so the implementation task receives that explicit status before you stop. Check out the PR branch locally with `git fetch origin '' && git checkout ''`, using the source branch from the pull-request details. For cross-repository (fork) PRs whose source branch cannot be fetched with task credentials, report that blocker instead of improvising credentials. - If you are not in `legacy_full_rereview_path`, run `git diff [last_review_sha]...[HEAD_SHA]` and `git log --oneline [last_review_sha]..[HEAD_SHA]` to inspect the live delta. When `pull_request_changed_files` is supplied (or you have otherwise determined the PR's base-to-head Files Changed), restrict this delta to those paths — e.g. `git diff [last_review_sha]...[HEAD_SHA] -- ` — and disregard any delta hunk for a file outside that set, since it was inherited from a base-branch rebase/merge and is not part of this PR. + If you are not in `legacy_full_rereview_path`, inspect the live delta with a two-dot diff `git diff [last_review_sha]..[HEAD_SHA]` and `git log --oneline [last_review_sha]..[HEAD_SHA]`. Use two-dot (`..`), not three-dot (`...`): two-dot is the actual content difference between the reviewed commits, so it excludes changes you already reviewed and any base-branch code a rebase replayed on top of. When `pull_request_changed_files` is supplied (or you have otherwise determined the PR's base-to-head Files Changed), restrict the delta to those paths — `git diff [last_review_sha]..[HEAD_SHA] -- ` — and disregard any hunk for a file outside that set, since it was inherited from a base-branch rebase/merge and is not part of this PR. + If that scoped two-dot delta is empty — for example the head SHA changed only because the branch was rebased, with no new PR content — treat it the same as the head-SHA-match case: update the summary comment with a short no-op note, mark the terminal outcome `no_new_delta`, and continue to the linked-task handoff step instead of re-reviewing already-reviewed PR files as net-new. If you are in `legacy_full_rereview_path`, re-review the full current PR diff with a local base-to-head comparison: `git fetch origin ''`, then `git diff ...` using the SHAs from `get_pull_request`. Use this local git diff for every provider. When `existing_review_comments` or `issue_comments` are missing, or when current thread or top-level discussion state must be revalidated before a side effect, call `mcp__roomote__manage_source_control` with `action: "list_pull_request_comments"`. The result returns review threads (each with a `threadId`, `resolved` state when the provider exposes it, and inline path/line anchors) plus top-level `issueComments`; heed any capability warnings it reports. Read the changed files in the delta and any related repository files needed to verify correctness in context. @@ -1141,7 +1142,8 @@ You are a sync-review workflow specialist. Re-review pull requests after new com When `pull_request_details` or current head metadata is missing, or when it must be revalidated before a side effect, call `mcp__roomote__manage_source_control` with `action: "get_pull_request"`, `repositoryFullName`, and `prNumber`. The result carries the title, body, state, draft flag, source and target branches, head and base SHAs, author, mergeability, and cross-repository (fork) information. If you are not in `legacy_full_rereview_path` and the current head SHA matches `last_review_sha`, update the summary comment with a short no-op note, mark the terminal outcome as `no_new_delta`, then continue directly to the linked-task handoff step so the implementation task receives that explicit status before you stop. Check out the PR branch locally with `git fetch origin '' && git checkout ''`, using the source branch from the pull-request details. For cross-repository (fork) PRs whose source branch cannot be fetched with task credentials, report that blocker instead of improvising credentials. - If you are not in `legacy_full_rereview_path`, run `git diff [last_review_sha]...[HEAD_SHA]` and `git log --oneline [last_review_sha]..[HEAD_SHA]` to inspect the live delta. When `pull_request_changed_files` is supplied (or you have otherwise determined the PR's base-to-head Files Changed), restrict this delta to those paths — e.g. `git diff [last_review_sha]...[HEAD_SHA] -- ` — and disregard any delta hunk for a file outside that set, since it was inherited from a base-branch rebase/merge and is not part of this PR. + If you are not in `legacy_full_rereview_path`, inspect the live delta with a two-dot diff `git diff [last_review_sha]..[HEAD_SHA]` and `git log --oneline [last_review_sha]..[HEAD_SHA]`. Use two-dot (`..`), not three-dot (`...`): two-dot is the actual content difference between the reviewed commits, so it excludes changes you already reviewed and any base-branch code a rebase replayed on top of. When `pull_request_changed_files` is supplied (or you have otherwise determined the PR's base-to-head Files Changed), restrict the delta to those paths — `git diff [last_review_sha]..[HEAD_SHA] -- ` — and disregard any hunk for a file outside that set, since it was inherited from a base-branch rebase/merge and is not part of this PR. + If that scoped two-dot delta is empty — for example the head SHA changed only because the branch was rebased, with no new PR content — treat it the same as the head-SHA-match case: update the summary comment with a short no-op note, mark the terminal outcome `no_new_delta`, and continue to the linked-task handoff step instead of re-reviewing already-reviewed PR files as net-new. If you are in `legacy_full_rereview_path`, re-review the full current PR diff with a local base-to-head comparison: `git fetch origin ''`, then `git diff ...` using the SHAs from `get_pull_request`. Use this local git diff for every provider. When `existing_review_comments` or `issue_comments` are missing, or when current thread or top-level discussion state must be revalidated before a side effect, call `mcp__roomote__manage_source_control` with `action: "list_pull_request_comments"`. The result returns review threads (each with a `threadId`, `resolved` state when the provider exposes it, and inline path/line anchors) plus top-level `issueComments`; heed any capability warnings it reports. Read the changed files in the delta and any related repository files needed to verify correctness in context. diff --git a/packages/cloud-agents/src/server/workflows/utils.ts b/packages/cloud-agents/src/server/workflows/utils.ts index 90734c6ee..71f306459 100644 --- a/packages/cloud-agents/src/server/workflows/utils.ts +++ b/packages/cloud-agents/src/server/workflows/utils.ts @@ -953,3 +953,73 @@ export function filterUnifiedDiffToFiles( return kept.join('\n'); } + +export interface ScopedSyncReviewDelta { + /** Whether the PR's authoritative Files Changed were successfully read. */ + pullRequestFilesAvailable: boolean; + /** Range changed files scoped to the PR's Files Changed (unscoped on failure). */ + changedFiles: string[]; + /** Range diff scoped to the PR's Files Changed (unscoped/undefined on failure). */ + diff: string | undefined; + /** + * Whether the reviewer has a delta to act on. False only when a trustworthy, + * successful read genuinely surfaced no PR-relevant change — a fetch + * failure or too-large diff keeps this true so the reviewer inspects + * manually rather than silently collapsing to a no-op. + */ + hasReviewableChanges: boolean; +} + +/** + * Scope a since-last-review compare range to the PR's authoritative Files + * Changed. The compare range uses three-dot semantics, so after a rebase it + * carries base-branch commits; intersecting with the PR's `base...head` files + * drops them. Both diff fetches return `{ diff: undefined, changedFiles: [] }` + * on failure/too-large, so failure is treated as "inspect manually", never as + * "no changes". + */ +export function resolveScopedSyncReviewDelta({ + sameHeadAsLastReview, + pullRequestDiff, + rangeDiff, +}: { + sameHeadAsLastReview: boolean; + pullRequestDiff: { diff: string | undefined; changedFiles: string[] }; + rangeDiff: { diff: string | undefined; changedFiles: string[] }; +}): ScopedSyncReviewDelta { + if (sameHeadAsLastReview) { + return { + pullRequestFilesAvailable: true, + changedFiles: [], + diff: undefined, + hasReviewableChanges: false, + }; + } + + const pullRequestFilesAvailable = pullRequestDiff.diff !== undefined; + const rangeFetchFailed = rangeDiff.diff === undefined; + const pullRequestChangedFileSet = new Set(pullRequestDiff.changedFiles); + + const changedFiles = pullRequestFilesAvailable + ? rangeDiff.changedFiles.filter((file) => + pullRequestChangedFileSet.has(file), + ) + : rangeDiff.changedFiles; + + const diff = + rangeDiff.diff === undefined + ? undefined + : pullRequestFilesAvailable + ? filterUnifiedDiffToFiles(rangeDiff.diff, pullRequestChangedFileSet) + : rangeDiff.diff; + + const hasReviewableChanges = + rangeFetchFailed || !pullRequestFilesAvailable || changedFiles.length > 0; + + return { + pullRequestFilesAvailable, + changedFiles, + diff, + hasReviewableChanges, + }; +} From 7529aeba6b9fb96b4275a4805a2c6b2660c1af9b Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Thu, 9 Jul 2026 14:14:40 -0500 Subject: [PATCH 3/3] [Fix] Review scope is the PR's base...head diff, not the two-dot range 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 --- .../architecture/workflow-contracts.md | 2 +- .../__tests__/githubPrReviewSync.test.ts | 31 +++--- .../server/workflows/githubPrReviewSync.ts | 9 +- .../skills/standard/review-code/SKILL.md | 8 +- .../src/server/workflows/utils.ts | 94 +++++++++++-------- 5 files changed, 84 insertions(+), 60 deletions(-) diff --git a/.agent-guidance/architecture/workflow-contracts.md b/.agent-guidance/architecture/workflow-contracts.md index dbc2a754b..1b3b9fc99 100644 --- a/.agent-guidance/architecture/workflow-contracts.md +++ b/.agent-guidance/architecture/workflow-contracts.md @@ -412,7 +412,7 @@ The `sync-github-pr-review` and `sync-github-pr-review-with-approval` appendices - recover a trustworthy last-reviewed SHA from explicit context, marker-based summary state, or a review-comment fallback - use legacy full-rereview only when a reusable legacy summary exists but no reliable anchor can be recovered -- scope the since-last-review delta to the PR's authoritative Files Changed (`base...head`): the compare range `lastReviewSha...head` uses three-dot semantics, so after a rebase it also contains base-branch commits, and those files are excluded from `diff_in_range`, `changed_files_since_last_review`, and the `pull_request_changed_files` scope hint so a rebase cannot import findings for code the PR does not touch (a rebase-only head change with no PR-relevant delta collapses to the no-op path) +- scope the review to the PR's authoritative Files Changed (`base...head`, from `gh pr diff`): the since-last-review compare range `lastReviewSha...head` is three-dot, so after a rebase it carries base-branch commits — and even a file the PR also touches gets base-branch hunks in its range section. The range is therefore used only to identify _which_ PR files changed since the last review; the diff content fed to the reviewer (`diff_in_range`) comes from the PR's own `base...head` diff for those files, which never contains base-branch hunks. `resolveScopedSyncReviewDelta` computes this and treats a fetch failure or too-large diff as "inspect manually" rather than collapsing to a no-op. The reviewer treats `base...head` as the only in-scope change set and uses a two-dot `lastReviewSha..head` diff purely to detect no-op (empty ⇒ `no_new_delta`, e.g. a rebase-only head change) — never as the review scope - emit an explicit no-op path when there is no new delta - surface only net-new actionable issues - carry forward prior unresolved issues in the rolling summary instead of re-commenting them diff --git a/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSync.test.ts b/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSync.test.ts index 8e120c195..db0dc62bb 100644 --- a/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSync.test.ts +++ b/packages/cloud-agents/src/server/workflows/__tests__/githubPrReviewSync.test.ts @@ -163,21 +163,28 @@ No actionable issues found. }); describe('resolveScopedSyncReviewDelta unit tests', () => { + // The PR's authoritative base...head diff: only its own hunk for the + // shared file (a base-branch hunk on that same file is NOT here). const prSectionDiff = [ 'diff --git a/apps/web/src/pr.ts b/apps/web/src/pr.ts', - '@@ -1 +1 @@', - '-const a = 1;', - '+const a = 2;', + '@@ -10 +10 @@', + '-const pr = 1;', + '+const pr = 2;', ].join('\n'); + // The three-dot range after a rebase: the shared PR file section also + // carries a base-branch hunk, plus an unrelated base-only file. const rebaseRangeDiff = [ - prSectionDiff, + 'diff --git a/apps/web/src/pr.ts b/apps/web/src/pr.ts', + '@@ -1 +1 @@', + '-const base_touched_shared = 1;', + '+const base_touched_shared = 2;', 'diff --git a/apps/api/src/base.ts b/apps/api/src/base.ts', '@@ -1 +1 @@', '-const b = 1;', '+const b = 2;', ].join('\n'); - it('drops rebased-in base-branch files but keeps the PR files', () => { + it('presents the PR base...head hunks, not the range, for changed files', () => { const result = utils.resolveScopedSyncReviewDelta({ sameHeadAsLastReview: false, pullRequestDiff: { @@ -191,7 +198,10 @@ No actionable issues found. }); expect(result.changedFiles).toEqual(['apps/web/src/pr.ts']); - expect(result.diff).toContain('apps/web/src/pr.ts'); + // The PR's own hunk is shown; the base-branch hunk on the same file and + // the unrelated base-only file are both excluded. + expect(result.diff).toContain('const pr = 2;'); + expect(result.diff).not.toContain('base_touched_shared'); expect(result.diff).not.toContain('apps/api/src/base.ts'); expect(result.hasReviewableChanges).toBe(true); }); @@ -210,10 +220,11 @@ No actionable issues found. }); expect(result.changedFiles).toEqual([]); + expect(result.diff).toBeUndefined(); expect(result.hasReviewableChanges).toBe(false); }); - it('keeps reviewable changes (inspect manually) when the range fetch failed', () => { + it('keeps reviewable changes (full PR diff) when the range fetch failed', () => { const result = utils.resolveScopedSyncReviewDelta({ sameHeadAsLastReview: false, pullRequestDiff: { @@ -224,7 +235,7 @@ No actionable issues found. rangeDiff: { diff: undefined, changedFiles: [] }, }); - expect(result.diff).toBeUndefined(); + expect(result.diff).toBe(prSectionDiff); expect(result.hasReviewableChanges).toBe(true); }); @@ -239,10 +250,6 @@ No actionable issues found. }); expect(result.pullRequestFilesAvailable).toBe(false); - expect(result.changedFiles).toEqual([ - 'apps/web/src/pr.ts', - 'apps/api/src/base.ts', - ]); expect(result.diff).toBe(rebaseRangeDiff); expect(result.hasReviewableChanges).toBe(true); }); diff --git a/packages/cloud-agents/src/server/workflows/githubPrReviewSync.ts b/packages/cloud-agents/src/server/workflows/githubPrReviewSync.ts index e9c263ed1..6134bd82d 100644 --- a/packages/cloud-agents/src/server/workflows/githubPrReviewSync.ts +++ b/packages/cloud-agents/src/server/workflows/githubPrReviewSync.ts @@ -12,7 +12,7 @@ import { getPrDetails, getCommits, getIssueDetails, - getDiffInRange, + getDiff, getMarkdownChecklist, getReviewComments, getIssueComments, @@ -469,11 +469,14 @@ export async function githubPrReviewSync({ ? getCommits(commits) : undefined, linked_issue: getIssueDetails(fullName, issue), + // `diff` is the PR's own base...head hunks for the files that changed + // since the last review (never the three-dot range content), so it is + // rendered as the pull-request diff rather than a compare range. diff_in_range: hasReviewableChanges - ? getDiffInRange({ + ? getDiff({ + prNumber, repo: fullName, diff, - range, lineLimit: 5_000, charLimit: 100_000, }) diff --git a/packages/cloud-agents/src/server/workflows/skills/standard/review-code/SKILL.md b/packages/cloud-agents/src/server/workflows/skills/standard/review-code/SKILL.md index e8598ee8b..cbb9929f3 100644 --- a/packages/cloud-agents/src/server/workflows/skills/standard/review-code/SKILL.md +++ b/packages/cloud-agents/src/server/workflows/skills/standard/review-code/SKILL.md @@ -831,8 +831,8 @@ You are a sync-review workflow specialist. Re-review pull requests after new com When `pull_request_details` or current head metadata is missing, or when it must be revalidated before a side effect, call `mcp__roomote__manage_source_control` with `action: "get_pull_request"`, `repositoryFullName`, and `prNumber`. The result carries the title, body, state, draft flag, source and target branches, head and base SHAs, author, mergeability, and cross-repository (fork) information. If you are not in `legacy_full_rereview_path` and the current head SHA matches `last_review_sha`, update the summary comment with a short no-op note, mark the terminal outcome as `no_new_delta`, then continue directly to the linked-task handoff step so the implementation task receives that explicit status before you stop. Check out the PR branch locally with `git fetch origin '' && git checkout ''`, using the source branch from the pull-request details. For cross-repository (fork) PRs whose source branch cannot be fetched with task credentials, report that blocker instead of improvising credentials. - If you are not in `legacy_full_rereview_path`, inspect the live delta with a two-dot diff `git diff [last_review_sha]..[HEAD_SHA]` and `git log --oneline [last_review_sha]..[HEAD_SHA]`. Use two-dot (`..`), not three-dot (`...`): two-dot is the actual content difference between the reviewed commits, so it excludes changes you already reviewed and any base-branch code a rebase replayed on top of. When `pull_request_changed_files` is supplied (or you have otherwise determined the PR's base-to-head Files Changed), restrict the delta to those paths — `git diff [last_review_sha]..[HEAD_SHA] -- ` — and disregard any hunk for a file outside that set, since it was inherited from a base-branch rebase/merge and is not part of this PR. - If that scoped two-dot delta is empty — for example the head SHA changed only because the branch was rebased, with no new PR content — treat it the same as the head-SHA-match case: update the summary comment with a short no-op note, mark the terminal outcome `no_new_delta`, and continue to the linked-task handoff step instead of re-reviewing already-reviewed PR files as net-new. + If you are not in `legacy_full_rereview_path`, first decide whether there is any new delta at all with a two-dot diff `git diff [last_review_sha]..[HEAD_SHA]` and `git log --oneline [last_review_sha]..[HEAD_SHA]`. Two-dot (`..`) is the actual content difference between the two reviewed commits. If it is empty — for example the head SHA changed only because the branch was rebased, with no new content — treat it the same as the head-SHA-match case: update the summary comment with a short no-op note, mark the terminal outcome `no_new_delta`, and continue to the linked-task handoff step instead of re-reviewing. + When there is a delta, the authoritative set of changes you may review is the PR's current Files Changed — its base-to-head diff, `git diff ...` (three-dot from the current base), scoped to the files in `pull_request_changed_files`/`changed_files_since_last_review` and the supplied `diff_in_range` when present. Report findings only for hunks that appear in that current PR diff. A change that is not in the PR's base-to-head diff — including a base-branch modification to a file the PR also touches — is out of scope: it belongs to the base branch, not this PR, and must not be reported or carried forward. Use the two-dot delta and commit log only to focus on what is new since the last review, never as the review scope itself. If you are in `legacy_full_rereview_path`, re-review the full current PR diff with a local base-to-head comparison: `git fetch origin ''`, then `git diff ...` using the SHAs from `get_pull_request`. Use this local git diff for every provider. When `existing_review_comments` or `issue_comments` are missing, or when current thread or top-level discussion state must be revalidated before a side effect, call `mcp__roomote__manage_source_control` with `action: "list_pull_request_comments"`. The result returns review threads (each with a `threadId`, `resolved` state when the provider exposes it, and inline path/line anchors) plus top-level `issueComments`; heed any capability warnings it reports. Read the changed files in the delta and any related repository files needed to verify correctness in context. @@ -1142,8 +1142,8 @@ You are a sync-review workflow specialist. Re-review pull requests after new com When `pull_request_details` or current head metadata is missing, or when it must be revalidated before a side effect, call `mcp__roomote__manage_source_control` with `action: "get_pull_request"`, `repositoryFullName`, and `prNumber`. The result carries the title, body, state, draft flag, source and target branches, head and base SHAs, author, mergeability, and cross-repository (fork) information. If you are not in `legacy_full_rereview_path` and the current head SHA matches `last_review_sha`, update the summary comment with a short no-op note, mark the terminal outcome as `no_new_delta`, then continue directly to the linked-task handoff step so the implementation task receives that explicit status before you stop. Check out the PR branch locally with `git fetch origin '' && git checkout ''`, using the source branch from the pull-request details. For cross-repository (fork) PRs whose source branch cannot be fetched with task credentials, report that blocker instead of improvising credentials. - If you are not in `legacy_full_rereview_path`, inspect the live delta with a two-dot diff `git diff [last_review_sha]..[HEAD_SHA]` and `git log --oneline [last_review_sha]..[HEAD_SHA]`. Use two-dot (`..`), not three-dot (`...`): two-dot is the actual content difference between the reviewed commits, so it excludes changes you already reviewed and any base-branch code a rebase replayed on top of. When `pull_request_changed_files` is supplied (or you have otherwise determined the PR's base-to-head Files Changed), restrict the delta to those paths — `git diff [last_review_sha]..[HEAD_SHA] -- ` — and disregard any hunk for a file outside that set, since it was inherited from a base-branch rebase/merge and is not part of this PR. - If that scoped two-dot delta is empty — for example the head SHA changed only because the branch was rebased, with no new PR content — treat it the same as the head-SHA-match case: update the summary comment with a short no-op note, mark the terminal outcome `no_new_delta`, and continue to the linked-task handoff step instead of re-reviewing already-reviewed PR files as net-new. + If you are not in `legacy_full_rereview_path`, first decide whether there is any new delta at all with a two-dot diff `git diff [last_review_sha]..[HEAD_SHA]` and `git log --oneline [last_review_sha]..[HEAD_SHA]`. Two-dot (`..`) is the actual content difference between the two reviewed commits. If it is empty — for example the head SHA changed only because the branch was rebased, with no new content — treat it the same as the head-SHA-match case: update the summary comment with a short no-op note, mark the terminal outcome `no_new_delta`, and continue to the linked-task handoff step instead of re-reviewing. + When there is a delta, the authoritative set of changes you may review is the PR's current Files Changed — its base-to-head diff, `git diff ...` (three-dot from the current base), scoped to the files in `pull_request_changed_files`/`changed_files_since_last_review` and the supplied `diff_in_range` when present. Report findings only for hunks that appear in that current PR diff. A change that is not in the PR's base-to-head diff — including a base-branch modification to a file the PR also touches — is out of scope: it belongs to the base branch, not this PR, and must not be reported or carried forward. Use the two-dot delta and commit log only to focus on what is new since the last review, never as the review scope itself. If you are in `legacy_full_rereview_path`, re-review the full current PR diff with a local base-to-head comparison: `git fetch origin ''`, then `git diff ...` using the SHAs from `get_pull_request`. Use this local git diff for every provider. When `existing_review_comments` or `issue_comments` are missing, or when current thread or top-level discussion state must be revalidated before a side effect, call `mcp__roomote__manage_source_control` with `action: "list_pull_request_comments"`. The result returns review threads (each with a `threadId`, `resolved` state when the provider exposes it, and inline path/line anchors) plus top-level `issueComments`; heed any capability warnings it reports. Read the changed files in the delta and any related repository files needed to verify correctness in context. diff --git a/packages/cloud-agents/src/server/workflows/utils.ts b/packages/cloud-agents/src/server/workflows/utils.ts index 71f306459..ddbb23f84 100644 --- a/packages/cloud-agents/src/server/workflows/utils.ts +++ b/packages/cloud-agents/src/server/workflows/utils.ts @@ -954,29 +954,21 @@ export function filterUnifiedDiffToFiles( return kept.join('\n'); } -export interface ScopedSyncReviewDelta { - /** Whether the PR's authoritative Files Changed were successfully read. */ - pullRequestFilesAvailable: boolean; - /** Range changed files scoped to the PR's Files Changed (unscoped on failure). */ - changedFiles: string[]; - /** Range diff scoped to the PR's Files Changed (unscoped/undefined on failure). */ - diff: string | undefined; - /** - * Whether the reviewer has a delta to act on. False only when a trustworthy, - * successful read genuinely surfaced no PR-relevant change — a fetch - * failure or too-large diff keeps this true so the reviewer inspects - * manually rather than silently collapsing to a no-op. - */ - hasReviewableChanges: boolean; -} - /** - * Scope a since-last-review compare range to the PR's authoritative Files - * Changed. The compare range uses three-dot semantics, so after a rebase it - * carries base-branch commits; intersecting with the PR's `base...head` files - * drops them. Both diff fetches return `{ diff: undefined, changedFiles: [] }` - * on failure/too-large, so failure is treated as "inspect manually", never as - * "no changes". + * Resolve the reviewable delta for a sync re-review, scoped to the pull + * request's own changes. + * + * The since-last-review compare range (`sha...currentHead`) uses three-dot + * semantics, so after a rebase it carries base-branch commits — and even for a + * file the PR also touches, that file's range section contains base-branch + * hunks that are outside the PR's `base...head` Files Changed. So the range is + * used only to identify *which* PR files changed since the last review; the + * diff content presented for review comes from the PR's authoritative + * `base...head` diff (`gh pr diff`), which never contains base-branch hunks. + * + * Both diff fetches return `{ diff: undefined, changedFiles: [] }` on failure + * or a too-large diff, so failure is treated as "inspect manually" (keep the + * delta), never as "no changes". */ export function resolveScopedSyncReviewDelta({ sameHeadAsLastReview, @@ -984,9 +976,16 @@ export function resolveScopedSyncReviewDelta({ rangeDiff, }: { sameHeadAsLastReview: boolean; + /** The PR's authoritative `base...head` diff (`gh pr diff`). */ pullRequestDiff: { diff: string | undefined; changedFiles: string[] }; + /** The three-dot `sha...currentHead` since-last-review compare range. */ rangeDiff: { diff: string | undefined; changedFiles: string[] }; -}): ScopedSyncReviewDelta { +}): { + pullRequestFilesAvailable: boolean; + changedFiles: string[]; + diff: string | undefined; + hasReviewableChanges: boolean; +} { if (sameHeadAsLastReview) { return { pullRequestFilesAvailable: true, @@ -996,30 +995,45 @@ export function resolveScopedSyncReviewDelta({ }; } - const pullRequestFilesAvailable = pullRequestDiff.diff !== undefined; - const rangeFetchFailed = rangeDiff.diff === undefined; + // Without the authoritative PR diff we cannot scope; fall back to the raw + // range and let the reviewer inspect rather than dropping everything. + if (pullRequestDiff.diff === undefined) { + return { + pullRequestFilesAvailable: false, + changedFiles: rangeDiff.changedFiles, + diff: rangeDiff.diff, + hasReviewableChanges: true, + }; + } + + const pullRequestDiffText = pullRequestDiff.diff; const pullRequestChangedFileSet = new Set(pullRequestDiff.changedFiles); - const changedFiles = pullRequestFilesAvailable - ? rangeDiff.changedFiles.filter((file) => - pullRequestChangedFileSet.has(file), - ) - : rangeDiff.changedFiles; + // Range read failed: we cannot tell which PR files are new since the last + // review, so present the full authoritative PR diff for manual inspection. + if (rangeDiff.diff === undefined) { + return { + pullRequestFilesAvailable: true, + changedFiles: pullRequestDiff.changedFiles, + diff: pullRequestDiffText, + hasReviewableChanges: true, + }; + } - const diff = - rangeDiff.diff === undefined - ? undefined - : pullRequestFilesAvailable - ? filterUnifiedDiffToFiles(rangeDiff.diff, pullRequestChangedFileSet) - : rangeDiff.diff; + // PR files that appear in the since-last-review range. + const changedFiles = rangeDiff.changedFiles.filter((file) => + pullRequestChangedFileSet.has(file), + ); - const hasReviewableChanges = - rangeFetchFailed || !pullRequestFilesAvailable || changedFiles.length > 0; + const diff = + changedFiles.length > 0 + ? filterUnifiedDiffToFiles(pullRequestDiffText, new Set(changedFiles)) + : undefined; return { - pullRequestFilesAvailable, + pullRequestFilesAvailable: true, changedFiles, diff, - hasReviewableChanges, + hasReviewableChanges: changedFiles.length > 0, }; }