diff --git a/entry b/entry index bd55298..a1e8898 100755 --- a/entry +++ b/entry @@ -145271,6 +145271,110 @@ function $(cmd, args2, options) { return stdout.trim(); } +// utils/rangeDiff.ts +function computeIncrementalDiff(params) { + try { + const raw2 = $( + "sh", + [ + "-c", + 'old_base=$(git merge-base "$1" "origin/$2") && new_base=$(git merge-base "$3" "origin/$2") && git range-diff --no-color "$old_base..$(git commit-tree "$1^{tree}" -p "$old_base" -m x)" "$new_base..$(git commit-tree "$3^{tree}" -p "$new_base" -m x)"', + "--", + params.beforeSha, + params.baseBranch, + params.headSha + ], + { log: false } + ); + return postProcessRangeDiff(raw2); + } catch (e) { + log.debug(`\xBB range-diff failed: ${e instanceof Error ? e.message : String(e)}`); + return null; + } +} +function isDiffPrefix(ch) { + return ch === " " || ch === "+" || ch === "-"; +} +function postProcessRangeDiff(raw2, contextLines = 3) { + if (!raw2.trim()) return null; + if (/^\d+:\s+\w+\s+=\s+\d+:/m.test(raw2)) return null; + const beforeBuf = []; + let lastFileHdr = null; + let lastHunkHdr = null; + let fileHdrEmitted = true; + let hunkHdrEmitted = true; + let out = ""; + let afterRemaining = 0; + let lastEmittedSeq = -2; + let seq = 0; + let hasChanges = false; + function emit(line) { + if (lastEmittedSeq >= 0 && line.seq > lastEmittedSeq + 1) out += (out ? "\n" : "") + "..."; + out += (out ? "\n" : "") + line.prefix + raw2.slice(line.from, line.to); + lastEmittedSeq = line.seq; + if (lastFileHdr?.seq === line.seq) fileHdrEmitted = true; + if (lastHunkHdr?.seq === line.seq) hunkHdrEmitted = true; + } + function flushBefore() { + if (lastFileHdr && !fileHdrEmitted) emit(lastFileHdr); + if (lastHunkHdr && !hunkHdrEmitted) emit(lastHunkHdr); + for (const line of beforeBuf) { + if (line.seq > lastEmittedSeq) emit(line); + } + beforeBuf.length = 0; + } + let cursor = 0; + while (cursor < raw2.length) { + const eol = raw2.indexOf("\n", cursor); + const lineEnd = eol === -1 ? raw2.length : eol; + if (raw2.charCodeAt(cursor) >= 48 && raw2.charCodeAt(cursor) <= 57) { + cursor = lineEnd + 1; + continue; + } + if (lineEnd - cursor >= 5 && raw2.startsWith(" ", cursor)) { + const prefix = raw2[cursor + 4]; + if (isDiffPrefix(prefix)) { + const contentPos = cursor + 5; + const isOuterChange = prefix !== " "; + let line; + let isChange = false; + if (contentPos >= lineEnd) { + line = { prefix, from: lineEnd, to: lineEnd, seq }; + } else if (isDiffPrefix(raw2[contentPos])) { + isChange = isOuterChange; + line = { prefix, from: contentPos + 1, to: lineEnd, seq }; + } else { + line = { prefix, from: contentPos, to: lineEnd, seq }; + if (raw2.startsWith("## ", contentPos) && !raw2.startsWith("## Commit message", contentPos)) { + lastFileHdr = line; + fileHdrEmitted = false; + lastHunkHdr = null; + hunkHdrEmitted = true; + } else if (raw2.startsWith("@@", contentPos) && !raw2.startsWith("@@ Metadata", contentPos)) { + lastHunkHdr = line; + hunkHdrEmitted = false; + } + } + if (isChange) { + hasChanges = true; + flushBefore(); + emit(line); + afterRemaining = contextLines; + } else if (afterRemaining > 0) { + emit(line); + afterRemaining--; + } else { + if (beforeBuf.length >= contextLines) beforeBuf.shift(); + beforeBuf.push(line); + } + seq++; + } + } + cursor = lineEnd + 1; + } + return hasChanges ? out : null; +} + // mcp/checkout.ts function formatFilesWithLineNumbers(files) { const output = []; @@ -145361,132 +145465,168 @@ async function fetchAndFormatPrDiff(params) { }); return formatFilesWithLineNumbers(filesResponse.data); } -async function checkoutPrBranch(pullNumber, params) { - const { octokit, owner, name, gitToken, toolState } = params; - log.info(`\xBB checking out PR #${pullNumber}...`); - const pr = await octokit.rest.pulls.get({ - owner, - repo: name, - pull_number: pullNumber +async function createTempBranch(params) { + const response = await params.octokit.rest.git.createRef({ + owner: params.owner, + repo: params.repo, + ref: `refs/heads/${params.ref}`, + sha: params.sha }); - const headRepo = pr.data.head.repo; - if (!headRepo) { - throw new Error(`PR #${pullNumber} source repository was deleted`); - } - const isFork = headRepo.full_name !== pr.data.base.repo.full_name; - const baseBranch = pr.data.base.ref; - const headBranch = pr.data.head.ref; - const localBranch = `pr-${pullNumber}`; - const isShallow = $("git", ["rev-parse", "--is-shallow-repository"], { log: false }).trim() === "true"; - let deepenArgs = []; - if (isShallow) { - let depth = 1e3; - try { - const comparison = await octokit.rest.repos.compareCommits({ - owner, - repo: name, - base: baseBranch, - head: `pull/${pullNumber}/head` - }); - depth = comparison.data.behind_by + 10; - log.debug( - `\xBB PR is ${comparison.data.behind_by} commits behind ${baseBranch}, deepening by ${depth}` - ); - } catch { - log.debug(`\xBB compare API failed, falling back to --deepen=${depth}`); + return { + data: response.data, + async [Symbol.asyncDispose]() { + try { + await params.octokit.rest.git.deleteRef({ + owner: params.owner, + repo: params.repo, + ref: `heads/${params.ref}` + }); + log.debug(`\xBB deleted temp branch ${params.ref}`); + } catch (e) { + log.debug( + `\xBB failed to delete temp branch ${params.ref}: ${e instanceof Error ? e.message : String(e)}` + ); + } } - deepenArgs = [`--deepen=${depth}`]; + }; +} +async function ensureBeforeShaReachable(params) { + try { + $("git", ["cat-file", "-t", params.sha], { log: false }); + log.debug(`\xBB before_sha ${params.sha.slice(0, 7)} is reachable`); + return true; + } catch { } - const currentSha = $("git", ["rev-parse", "HEAD"], { log: false }).trim(); - const alreadyOnBranch = currentSha === pr.data.head.sha; - if (alreadyOnBranch) { - log.debug(`already on PR branch ${localBranch}, skipping checkout`); - } else { - log.debug(`\xBB fetching base branch (${baseBranch})...`); - await $git("fetch", [...deepenArgs, "--no-tags", "origin", baseBranch], { - token: gitToken - }); - $("git", ["checkout", "-B", baseBranch, `origin/${baseBranch}`], { log: false }); - log.debug(`\xBB fetching PR #${pullNumber} (${localBranch})...`); - await $git("fetch", ["--no-tags", "origin", `pull/${pullNumber}/head:${localBranch}`], { + const tempBranch = `pullfrog/tmp/${params.sha.slice(0, 12)}`; + try { + var _stack = []; + try { + log.debug(`\xBB before_sha ${params.sha.slice(0, 7)} not reachable, creating temp branch...`); + const _ref = __using(_stack, await createTempBranch({ + octokit: params.octokit, + owner: params.owner, + repo: params.repo, + sha: params.sha, + ref: tempBranch + }), true); + await $git( + "fetch", + ["--no-tags", ...params.isShallow ? ["--depth=1"] : [], "origin", tempBranch], + { token: params.gitToken } + ); + log.debug(`\xBB fetched before_sha via temp branch ${tempBranch}`); + return true; + } catch (_) { + var _error = _, _hasError = true; + } finally { + var _promise2 = __callDispose(_stack, _error, _hasError); + _promise2 && await _promise2; + } + } catch (e) { + log.debug(`\xBB failed to fetch before_sha: ${e instanceof Error ? e.message : String(e)}`); + return false; + } +} +async function checkoutPrBranch(pr, params) { + const { octokit, owner, name, gitToken, toolState, beforeSha } = params; + log.info(`\xBB checking out PR #${pr.number}...`); + const isFork = pr.headRepoFullName !== pr.baseRepoFullName; + const localBranch = `pr-${pr.number}`; + const isShallow = $("git", ["rev-parse", "--is-shallow-repository"], { log: false }).trim() === "true"; + toolState.checkoutSha = $("git", ["rev-parse", "HEAD"], { log: false }).trim(); + const alreadyOnBranch = toolState.checkoutSha === pr.headSha; + log.debug(`\xBB fetching base branch (${pr.baseRef})...`); + await $git("fetch", ["--no-tags", "origin", pr.baseRef], { token: gitToken }); + if (!alreadyOnBranch) { + $("git", ["checkout", "-B", pr.baseRef, `origin/${pr.baseRef}`], { log: false }); + log.debug(`\xBB fetching PR #${pr.number} (${localBranch})...`); + await $git("fetch", ["--no-tags", "origin", `pull/${pr.number}/head:${localBranch}`], { token: gitToken }); $("git", ["checkout", localBranch], { log: false }); - log.debug(`\xBB checked out PR #${pullNumber}`); + log.debug(`\xBB checked out PR #${pr.number}`); + toolState.checkoutSha = $("git", ["rev-parse", "HEAD"], { log: false }).trim(); } - if (alreadyOnBranch) { - log.debug(`\xBB fetching base branch (${baseBranch})...`); - await $git("fetch", [...deepenArgs, "--no-tags", "origin", baseBranch], { - token: gitToken - }); + const beforeShaReachable = beforeSha ? await ensureBeforeShaReachable({ + sha: beforeSha, + octokit, + owner, + repo: name, + gitToken, + isShallow + }) : false; + if (isShallow) { + let deepenDepth = 0; + try { + const [prComparison, beforeShaComparison] = await Promise.all([ + octokit.rest.repos.compareCommits({ + owner, + repo: name, + base: pr.baseRef, + head: toolState.checkoutSha + }), + beforeSha && beforeShaReachable ? octokit.rest.repos.compareCommits({ + owner, + repo: name, + base: pr.baseRef, + head: beforeSha + }) : void 0 + ]); + deepenDepth = Math.max( + prComparison.data.ahead_by, + prComparison.data.behind_by, + beforeShaComparison?.data.ahead_by ?? 0, + beforeShaComparison?.data.behind_by ?? 0 + ) + 10; + log.debug( + `\xBB PR: ${prComparison.data.ahead_by} ahead / ${prComparison.data.behind_by} behind` + (beforeShaComparison ? `, before_sha: ${beforeShaComparison.data.ahead_by} ahead / ${beforeShaComparison.data.behind_by} behind` : "") + `, deepen by ${deepenDepth}` + ); + } catch { + deepenDepth = 1e3; + log.debug(`\xBB compare API failed, falling back to --deepen=${deepenDepth}`); + } + if (deepenDepth) { + log.debug(`\xBB deepening by ${deepenDepth} to reach merge base...`); + await $git("fetch", [`--deepen=${deepenDepth}`, "--no-tags", "origin"], { + token: gitToken + }); + } } if (isFork) { - const remoteName = `pr-${pullNumber}`; - const forkUrl = `https://github.com/${headRepo.full_name}.git`; + const remoteName = `pr-${pr.number}`; + const forkUrl = `https://github.com/${pr.headRepoFullName}.git`; try { $("git", ["remote", "add", remoteName, forkUrl], { log: false }); - log.debug(`\xBB added remote '${remoteName}' for fork ${headRepo.full_name}`); + log.debug(`\xBB added remote '${remoteName}' for fork ${pr.headRepoFullName}`); } catch { $("git", ["remote", "set-url", remoteName, forkUrl], { log: false }); - log.debug(`\xBB updated remote '${remoteName}' for fork ${headRepo.full_name}`); + log.debug(`\xBB updated remote '${remoteName}' for fork ${pr.headRepoFullName}`); } $("git", ["config", `branch.${localBranch}.pushRemote`, remoteName], { log: false }); - $("git", ["config", `branch.${localBranch}.merge`, `refs/heads/${headBranch}`], { log: false }); - log.debug(`\xBB configured branch '${localBranch}' to push to '${remoteName}/${headBranch}'`); - if (!pr.data.maintainer_can_modify) { + $("git", ["config", `branch.${localBranch}.merge`, `refs/heads/${pr.headRef}`], { log: false }); + log.debug(`\xBB configured branch '${localBranch}' to push to '${remoteName}/${pr.headRef}'`); + if (!pr.maintainerCanModify) { log.warning( `\xBB fork PR has maintainer_can_modify=false - push operations will fail. ask the PR author to enable "Allow edits from maintainers" or the fork may be owned by an organization.` ); } } else { $("git", ["config", `branch.${localBranch}.pushRemote`, "origin"], { log: false }); - $("git", ["config", `branch.${localBranch}.merge`, `refs/heads/${headBranch}`], { log: false }); + $("git", ["config", `branch.${localBranch}.merge`, `refs/heads/${pr.headRef}`], { log: false }); } - toolState.issueNumber = pullNumber; + toolState.issueNumber = pr.number; if (isFork) { - toolState.pushUrl = `https://github.com/${headRepo.full_name}.git`; + toolState.pushUrl = `https://github.com/${pr.headRepoFullName}.git`; } toolState.pushDest = { - remoteName: isFork ? `pr-${pullNumber}` : "origin", - remoteBranch: headBranch, + remoteName: isFork ? `pr-${pr.number}` : "origin", + remoteBranch: pr.headRef, localBranch }; await executeLifecycleHook({ event: "post-checkout", script: params.postCheckoutScript }); - return { - prNumber: pullNumber, - isFork, - forkUrl: isFork ? `https://github.com/${headRepo.full_name}.git` : void 0 - }; -} -function deepenForBeforeSha(params) { - const isShallow = $("git", ["rev-parse", "--is-shallow-repository"], { log: false }).trim() === "true"; - if (!isShallow) return; - const maxIterations = 10; - for (let i = 0; i < maxIterations; i++) { - try { - $("git", ["cat-file", "-t", params.beforeSha], { log: false }); - log.debug(`\xBB before_sha ${params.beforeSha.slice(0, 7)} is now reachable`); - return; - } catch { - } - log.debug( - `\xBB deepening by 50 to reach before_sha ${params.beforeSha.slice(0, 7)} (attempt ${i + 1}/${maxIterations})` - ); - try { - $git("fetch", ["--deepen=50", "--no-tags", "origin"], { - token: params.gitToken - }); - } catch { - log.debug(`\xBB deepen for before_sha failed (force-push may have rewritten history)`); - return; - } - } - log.debug( - `\xBB before_sha ${params.beforeSha.slice(0, 7)} not reachable after ${maxIterations * 50} commits` - ); } function CheckoutPrTool(ctx) { return tool({ @@ -145494,32 +145634,60 @@ function CheckoutPrTool(ctx) { description: "Checkout a pull request branch locally. This fetches the PR branch and sets up push configuration for fork PRs. Returns diffPath pointing to the formatted diff file.", parameters: CheckoutPr, execute: execute(async ({ pull_number }) => { - await checkoutPrBranch(pull_number, { + const prResponse = await ctx.octokit.rest.pulls.get({ + owner: ctx.repo.owner, + repo: ctx.repo.name, + pull_number + }); + const headRepo = prResponse.data.head.repo; + if (!headRepo) { + throw new Error(`PR #${pull_number} source repository was deleted`); + } + const pr = { + number: pull_number, + headSha: prResponse.data.head.sha, + headRef: prResponse.data.head.ref, + headRepoFullName: headRepo.full_name, + baseRef: prResponse.data.base.ref, + baseRepoFullName: prResponse.data.base.repo.full_name, + maintainerCanModify: prResponse.data.maintainer_can_modify + }; + await checkoutPrBranch(pr, { octokit: ctx.octokit, owner: ctx.repo.owner, name: ctx.repo.name, gitToken: ctx.gitToken, toolState: ctx.toolState, shell: ctx.payload.shell, - postCheckoutScript: ctx.postCheckoutScript + postCheckoutScript: ctx.postCheckoutScript, + beforeSha: ctx.toolState.beforeSha }); - const event = ctx.payload.event; - if ("before_sha" in event && event.before_sha) { - deepenForBeforeSha({ - gitToken: ctx.gitToken, - beforeSha: event.before_sha + const tempDir = process.env.PULLFROG_TEMP_DIR; + if (!tempDir) { + throw new Error( + "PULLFROG_TEMP_DIR not set - checkout_pr must run in pullfrog action context" + ); + } + const headShort = ctx.toolState.checkoutSha.slice(0, 7); + let incrementalDiffPath; + if (ctx.toolState.beforeSha && ctx.toolState.checkoutSha) { + const beforeShort = ctx.toolState.beforeSha.slice(0, 7); + const incremental = computeIncrementalDiff({ + baseBranch: pr.baseRef, + beforeSha: ctx.toolState.beforeSha, + headSha: ctx.toolState.checkoutSha }); + if (incremental) { + incrementalDiffPath = join2( + tempDir, + `pr-${pull_number}-${beforeShort}-${headShort}-incremental.diff` + ); + writeFileSync(incrementalDiffPath, incremental); + log.info( + `\xBB incremental diff computed (${incremental.length} bytes) \u2192 ${incrementalDiffPath}` + ); + } } - const pr = await ctx.octokit.rest.pulls.get({ - owner: ctx.repo.owner, - repo: ctx.repo.name, - pull_number - }); - const headRepo = pr.data.head.repo; - if (!headRepo) { - throw new Error(`PR #${pull_number} source repository was deleted`); - } - ctx.toolState.checkoutSha = pr.data.head.sha; const formatResult = await fetchAndFormatPrDiff({ octokit: ctx.octokit, owner: ctx.repo.owner, @@ -145529,29 +145697,25 @@ function CheckoutPrTool(ctx) { const diffPreview = formatResult.content.split("\n").slice(0, 100).join("\n"); log.debug(`formatted diff preview (first 100 lines): ${diffPreview}`); - const tempDir = process.env.PULLFROG_TEMP_DIR; - if (!tempDir) { - throw new Error( - "PULLFROG_TEMP_DIR not set - checkout_pr must run in pullfrog action context" - ); - } - const diffPath = join2(tempDir, `pr-${pull_number}.diff`); + const diffPath = join2(tempDir, `pr-${pull_number}-${headShort}.diff`); writeFileSync(diffPath, formatResult.content); log.debug(`wrote diff to ${diffPath} (${formatResult.content.length} bytes)`); + const incrementalInstructions = incrementalDiffPath ? ` IMPORTANT: incrementalDiffPath contains ONLY the changes since the last reviewed version (computed via range-diff). you MUST read incrementalDiffPath FIRST to understand what changed, then use diffPath for full PR context. do NOT skip the incremental diff.` : ""; return { success: true, - number: pr.data.number, - title: pr.data.title, - base: pr.data.base.ref, + number: prResponse.data.number, + title: prResponse.data.title, + base: pr.baseRef, localBranch: `pr-${pull_number}`, - remoteBranch: `refs/heads/${pr.data.head.ref}`, - isFork: headRepo.full_name !== pr.data.base.repo.full_name, - maintainerCanModify: pr.data.maintainer_can_modify, - url: pr.data.html_url, - headRepo: headRepo.full_name, + remoteBranch: `refs/heads/${pr.headRef}`, + isFork: pr.headRepoFullName !== pr.baseRepoFullName, + maintainerCanModify: pr.maintainerCanModify, + url: prResponse.data.html_url, + headRepo: pr.headRepoFullName, diffPath, + incrementalDiffPath, toc: formatResult.toc, - instructions: `the diff file at diffPath contains a table of contents (TOC) at the top listing every changed file with its line range. use the line ranges to read specific files from the diff instead of reading the entire file. for example, if the TOC says "src/foo.ts \u2192 lines 5-42", read lines 5-42 from diffPath to see that file's changes. review files selectively based on relevance rather than reading everything sequentially. the local branch is 'localBranch' (pr-{number}), not the remote branch name. when pushing, omit branchName to use the current branch. do not use remoteBranch as a local branch name.` + instructions: `the diff file at diffPath contains a table of contents (TOC) at the top listing every changed file with its line range. use the line ranges to read specific files from the diff instead of reading the entire file. for example, if the TOC says "src/foo.ts \u2192 lines 5-42", read lines 5-42 from diffPath to see that file's changes. review files selectively based on relevance rather than reading everything sequentially. the local branch is 'localBranch' (pr-{number}), not the remote branch name. when pushing, omit branchName to use the current branch. do not use remoteBranch as a local branch name.` + incrementalInstructions }; }) }); @@ -147356,6 +147520,7 @@ function CreatePullRequestReviewTool(ctx) { if (ctx.toolState.checkoutSha && latestHeadSha && latestHeadSha !== ctx.toolState.checkoutSha) { const fromSha = ctx.toolState.checkoutSha; const toSha = latestHeadSha; + ctx.toolState.beforeSha = fromSha; ctx.toolState.checkoutSha = toSha; log.info( `new commits detected during review: ${fromSha.slice(0, 7)}..${toSha.slice(0, 7)}` @@ -147370,7 +147535,7 @@ function CreatePullRequestReviewTool(ctx) { newCommits: { from: fromSha, to: toSha, - instructions: `New commits were pushed while you were reviewing. Run \`git pull\` to fetch them, then review the incremental diff with \`git diff ${fromSha}...HEAD\`. Submit another review covering only the new changes. Do not repeat feedback from your previous review.` + instructions: `new commits were pushed while you were reviewing. call \`${ghPullfrogMcpName}/checkout_pr\` again to fetch the latest version \u2014 it will compute the incremental diff automatically. submit another review covering only the new changes. do not repeat feedback from your previous review.` } }; } @@ -148014,9 +148179,9 @@ ${learningsStep(6)}`, - **no actionable issues found**: do NOT submit a review. Call \`${ghPullfrogMcpName}/report_progress\` with a brief note (e.g., "Reviewed \u2014 no issues found.").`, IncrementalReview: `### Checklist -1. Checkout the PR via \`${ghPullfrogMcpName}/checkout_pr\` \u2014 this returns PR metadata and a \`diffPath\`. Read the diff to identify the major areas of change. +1. Checkout the PR via \`${ghPullfrogMcpName}/checkout_pr\` \u2014 this returns PR metadata, \`diffPath\` (full diff), and \`incrementalDiffPath\` (changes since last reviewed version, if available). -2. Generate the incremental diff using the \`before_sha\` from EVENT DATA: \`git diff ...HEAD\`. This isolates only the new commits. If the command fails (e.g., force-push rewrote history), fall back to reviewing the full PR diff. +2. If \`incrementalDiffPath\` is present, read it to see what changed since the last review. This is a range-diff that isolates the net changes, filtering out base branch noise. If not present, fall back to reviewing the full PR diff. 3. Fetch previous reviews via \`${ghPullfrogMcpName}/list_pull_request_reviews\`. For the most recent Pullfrog review, call \`${ghPullfrogMcpName}/get_review_comments\` with the review ID to retrieve specific prior line-level feedback. @@ -148862,12 +149027,9 @@ ${permalinkTip} description: "Re-review a PR after new commits are pushed; focus on new changes since the last review", prompt: `Follow these steps to incrementally re-review the PR after new commits were pushed. Focus on what changed since the last review. -1. **CHECKOUT** - Call ${ghPullfrogMcpName}/checkout_pr with the PR number. This gives you the full PR diff via \`diffPath\`. +1. **CHECKOUT** - Call ${ghPullfrogMcpName}/checkout_pr with the PR number. This returns \`diffPath\` (full PR diff) and \`incrementalDiffPath\` (changes since last reviewed version, if available). -2. **INCREMENTAL DIFF** - EVENT DATA contains \`before_sha\` (the HEAD before this push). Generate the incremental diff: - \`git diff ...HEAD\` - This shows the changes introduced by this push. Cross-reference with previous reviews (step 3) to confirm coverage of all unreviewed changes \u2014 the full PR diff fills any gaps. - **If the diff command fails** (e.g., force-push rewrote history), fall back to reviewing the full PR diff from step 1. +2. **INCREMENTAL DIFF** - If \`incrementalDiffPath\` is present, read it to see what changed since the last review. This is a range-diff that isolates only the net changes, filtering out base branch noise. If not present, fall back to reviewing the full PR diff. 3. **FETCH PREVIOUS REVIEWS** - Use ${ghPullfrogMcpName}/list_pull_request_reviews to find previous Pullfrog reviews. For the most recent one, call ${ghPullfrogMcpName}/get_review_comments with the review ID to see specific line-level feedback. This lets you understand what feedback was already given. @@ -150905,6 +151067,9 @@ async function main() { timer.checkpoint("runContextData"); const payload = resolvePayload(resolvedPromptInput, runContext.repoSettings); toolState.model = payload.model; + if (payload.event.trigger === "pull_request_synchronize") { + toolState.beforeSha = payload.event.before_sha; + } const tokenRef = __using(_stack2, await resolveTokens({ push: payload.push }), true); const oidcCredentials = process.env.ACTIONS_ID_TOKEN_REQUEST_URL && process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN ? { requestUrl: process.env.ACTIONS_ID_TOKEN_REQUEST_URL, diff --git a/external.ts b/external.ts index d808768..7bc3426 100644 --- a/external.ts +++ b/external.ts @@ -212,7 +212,7 @@ interface PullRequestSynchronizeEvent extends BasePayloadEvent { title: string; body: string | null; branch: string; - /** SHA before the push -- used to compute incremental diff via `git diff before_sha...HEAD` */ + /** SHA before the push -- used to compute incremental range-diff between PR versions */ before_sha: string; } diff --git a/main.ts b/main.ts index 238f3b5..51fcaec 100644 --- a/main.ts +++ b/main.ts @@ -172,6 +172,9 @@ export async function main(): Promise { // resolve payload to determine shell permission const payload = resolvePayload(resolvedPromptInput, runContext.repoSettings); toolState.model = payload.model; + if (payload.event.trigger === "pull_request_synchronize") { + toolState.beforeSha = payload.event.before_sha; + } // resolve tokens first — acquireNewToken needs OIDC env vars for token exchange await using tokenRef = await resolveTokens({ push: payload.push }); diff --git a/mcp/checkout.ts b/mcp/checkout.ts index 3fc37ba..1cce70b 100644 --- a/mcp/checkout.ts +++ b/mcp/checkout.ts @@ -5,6 +5,7 @@ import { type } from "arktype"; import { log } from "../utils/cli.ts"; import { $git } from "../utils/gitAuth.ts"; import { executeLifecycleHook } from "../utils/lifecycle.ts"; +import { computeIncrementalDiff } from "../utils/rangeDiff.ts"; import { $ } from "../utils/shell.ts"; import type { ToolContext } from "./server.ts"; import { execute, tool } from "./shared.ts"; @@ -139,6 +140,7 @@ export type CheckoutPrResult = { url: string; headRepo: string; diffPath: string; + incrementalDiffPath?: string | undefined; toc: string; instructions: string; }; @@ -166,135 +168,236 @@ export async function fetchAndFormatPrDiff(params: FetchPrDiffParams): Promise { + try { + $("git", ["cat-file", "-t", params.sha], { log: false }); + log.debug(`» before_sha ${params.sha.slice(0, 7)} is reachable`); + return true; + } catch { + // not available locally — create a temporary branch to fetch it + } + + const tempBranch = `pullfrog/tmp/${params.sha.slice(0, 12)}`; + try { + log.debug(`» before_sha ${params.sha.slice(0, 7)} not reachable, creating temp branch...`); + await using _ref = await createTempBranch({ + octokit: params.octokit, + owner: params.owner, + repo: params.repo, + sha: params.sha, + ref: tempBranch, + }); + await $git( + "fetch", + ["--no-tags", ...(params.isShallow ? ["--depth=1"] : []), "origin", tempBranch], + { token: params.gitToken } + ); + log.debug(`» fetched before_sha via temp branch ${tempBranch}`); + return true; + } catch (e) { + log.debug(`» failed to fetch before_sha: ${e instanceof Error ? e.message : String(e)}`); + return false; + } +} + +type CheckoutPrBranchParams = GitContext & { + beforeSha?: string | undefined; +}; + /** * Shared helper to checkout a PR branch and configure fork remotes. * Assumes origin remote is already configured with authentication. - * Updates toolState.issueNumber and toolState.pushUrl (for fork PRs). + * Updates toolState.issueNumber, toolState.checkoutSha, and toolState.pushUrl (for fork PRs). */ -export async function checkoutPrBranch( - pullNumber: number, - params: CheckoutPrBranchParams -): Promise { - const { octokit, owner, name, gitToken, toolState } = params; - log.info(`» checking out PR #${pullNumber}...`); +export async function checkoutPrBranch(pr: PrData, params: CheckoutPrBranchParams): Promise { + const { octokit, owner, name, gitToken, toolState, beforeSha } = params; + log.info(`» checking out PR #${pr.number}...`); - // fetch PR metadata - const pr = await octokit.rest.pulls.get({ - owner, - repo: name, - pull_number: pullNumber, - }); - - const headRepo = pr.data.head.repo; - if (!headRepo) { - throw new Error(`PR #${pullNumber} source repository was deleted`); - } - - const isFork = headRepo.full_name !== pr.data.base.repo.full_name; - const baseBranch = pr.data.base.ref; - const headBranch = pr.data.head.ref; + const isFork = pr.headRepoFullName !== pr.baseRepoFullName; // always use pr-{number} as local branch name for consistency // this avoids naming conflicts and makes push config simpler - const localBranch = `pr-${pullNumber}`; + const localBranch = `pr-${pr.number}`; - // compute deepen depth for shallow clones. actions/checkout uses depth=1 - // by default, which breaks rebase/log because git can't find the merge base. - // use the GitHub compare API to fetch exactly enough history. const isShallow = $("git", ["rev-parse", "--is-shallow-repository"], { log: false }).trim() === "true"; - let deepenArgs: string[] = []; - if (isShallow) { - let depth = 1000; // fallback - try { - const comparison = await octokit.rest.repos.compareCommits({ - owner, - repo: name, - base: baseBranch, - head: `pull/${pullNumber}/head`, - }); - depth = comparison.data.behind_by + 10; - log.debug( - `» PR is ${comparison.data.behind_by} commits behind ${baseBranch}, deepening by ${depth}` - ); - } catch { - log.debug(`» compare API failed, falling back to --deepen=${depth}`); - } - deepenArgs = [`--deepen=${depth}`]; - } - // check if we're already on the correct commit (not just branch name) - // this handles fork PRs where head branch name might match base branch name - const currentSha = $("git", ["rev-parse", "HEAD"], { log: false }).trim(); - const alreadyOnBranch = currentSha === pr.data.head.sha; + toolState.checkoutSha = $("git", ["rev-parse", "HEAD"], { log: false }).trim(); + const alreadyOnBranch = toolState.checkoutSha === pr.headSha; - if (alreadyOnBranch) { - log.debug(`already on PR branch ${localBranch}, skipping checkout`); - } else { - // fetch base branch so origin/ exists for diff operations - log.debug(`» fetching base branch (${baseBranch})...`); - await $git("fetch", [...deepenArgs, "--no-tags", "origin", baseBranch], { - token: gitToken, - }); + // fetch base branch so origin/ exists for diff operations + log.debug(`» fetching base branch (${pr.baseRef})...`); + await $git("fetch", ["--no-tags", "origin", pr.baseRef], { token: gitToken }); + // alreadyOnBranch only matches for repeated checkout_pr calls for the same PR in one session + // (without the tip moving), or if an external setup already checked out the PR head. + // normal PR-triggered runs won't match here — actions/checkout lands on a synthesized + // merge commit whose SHA differs from pr.headSha. + // + // so the fetch+checkout block below will almost always execute, and the fetched HEAD + // might differ from pr.headSha. toolState.checkoutSha is set after to capture the actual SHA. + if (!alreadyOnBranch) { // checkout base branch first to avoid "refusing to fetch into current branch" error // -B creates or resets the branch to match origin/baseBranch - $("git", ["checkout", "-B", baseBranch, `origin/${baseBranch}`], { log: false }); + $("git", ["checkout", "-B", pr.baseRef, `origin/${pr.baseRef}`], { log: false }); // fetch PR branch using pull/{n}/head refspec (works for both fork and same-repo PRs) - log.debug(`» fetching PR #${pullNumber} (${localBranch})...`); - await $git("fetch", ["--no-tags", "origin", `pull/${pullNumber}/head:${localBranch}`], { + log.debug(`» fetching PR #${pr.number} (${localBranch})...`); + await $git("fetch", ["--no-tags", "origin", `pull/${pr.number}/head:${localBranch}`], { token: gitToken, }); // checkout the branch $("git", ["checkout", localBranch], { log: false }); - log.debug(`» checked out PR #${pullNumber}`); + log.debug(`» checked out PR #${pr.number}`); + // make sure toolState.checkoutSha is set to the actual checked-out SHA (which might be different from pr.headSha) + toolState.checkoutSha = $("git", ["rev-parse", "HEAD"], { log: false }).trim(); } - // ensure base branch is fetched (needed for diff operations) - // fetch if we skipped checkout (already on branch) - otherwise already fetched above - if (alreadyOnBranch) { - log.debug(`» fetching base branch (${baseBranch})...`); - await $git("fetch", [...deepenArgs, "--no-tags", "origin", baseBranch], { - token: gitToken, - }); + const beforeShaReachable = beforeSha + ? await ensureBeforeShaReachable({ + sha: beforeSha, + octokit, + owner, + repo: name, + gitToken, + isShallow, + }) + : false; + + // compute deepen depth for shallow clones. actions/checkout uses depth=1 + // by default, which breaks rebase/log because git can't find the merge base. + // use the GitHub compare API to fetch exactly enough history. + // computed after checkout so compareCommits uses the actual checked-out SHA. + if (isShallow) { + let deepenDepth = 0; + try { + // ahead_by = PR commits past merge base, behind_by = base commits past merge base. + // --deepen extends ALL shallow roots equally (can't deepen a single branch), + // so we need the max across both the PR head and before_sha to ensure all + // three points (base, head, before_sha) reach the merge base in a single deepen call. + const [prComparison, beforeShaComparison] = await Promise.all([ + octokit.rest.repos.compareCommits({ + owner, + repo: name, + base: pr.baseRef, + head: toolState.checkoutSha, + }), + beforeSha && beforeShaReachable + ? octokit.rest.repos.compareCommits({ + owner, + repo: name, + base: pr.baseRef, + head: beforeSha, + }) + : undefined, + ]); + deepenDepth = + Math.max( + prComparison.data.ahead_by, + prComparison.data.behind_by, + beforeShaComparison?.data.ahead_by ?? 0, + beforeShaComparison?.data.behind_by ?? 0 + ) + 10; + log.debug( + `» PR: ${prComparison.data.ahead_by} ahead / ${prComparison.data.behind_by} behind` + + (beforeShaComparison + ? `, before_sha: ${beforeShaComparison.data.ahead_by} ahead / ${beforeShaComparison.data.behind_by} behind` + : "") + + `, deepen by ${deepenDepth}` + ); + } catch { + deepenDepth = 1000; + log.debug(`» compare API failed, falling back to --deepen=${deepenDepth}`); + } + // deepen after both branches are fetched so the merge base is reachable from both sides + if (deepenDepth) { + log.debug(`» deepening by ${deepenDepth} to reach merge base...`); + await $git("fetch", [`--deepen=${deepenDepth}`, "--no-tags", "origin"], { + token: gitToken, + }); + } } // configure push remote for this branch // NOTE: This always runs regardless of alreadyOnBranch, because setupGit doesn't configure // fork remotes. This ensures fork PRs can push even when checkout_pr is called after setupGit. if (isFork) { - const remoteName = `pr-${pullNumber}`; + const remoteName = `pr-${pr.number}`; // SECURITY: fork URL without token - auth is injected via GIT_ASKPASS in $git() - const forkUrl = `https://github.com/${headRepo.full_name}.git`; + const forkUrl = `https://github.com/${pr.headRepoFullName}.git`; // add fork as a named remote (suppress logging to avoid "error: remote already exists" spam) try { $("git", ["remote", "add", remoteName, forkUrl], { log: false }); - log.debug(`» added remote '${remoteName}' for fork ${headRepo.full_name}`); + log.debug(`» added remote '${remoteName}' for fork ${pr.headRepoFullName}`); } catch { // remote already exists, update its URL $("git", ["remote", "set-url", remoteName, forkUrl], { log: false }); - log.debug(`» updated remote '${remoteName}' for fork ${headRepo.full_name}`); + log.debug(`» updated remote '${remoteName}' for fork ${pr.headRepoFullName}`); } // set branch push config so `git push` knows where to push $("git", ["config", `branch.${localBranch}.pushRemote`, remoteName], { log: false }); // set merge ref so git knows the remote branch name (may differ from local) - $("git", ["config", `branch.${localBranch}.merge`, `refs/heads/${headBranch}`], { log: false }); - log.debug(`» configured branch '${localBranch}' to push to '${remoteName}/${headBranch}'`); + $("git", ["config", `branch.${localBranch}.merge`, `refs/heads/${pr.headRef}`], { log: false }); + log.debug(`» configured branch '${localBranch}' to push to '${remoteName}/${pr.headRef}'`); // warn if maintainer can't modify (push will likely fail) - if (!pr.data.maintainer_can_modify) { + if (!pr.maintainerCanModify) { log.warning( `» fork PR has maintainer_can_modify=false - push operations will fail. ` + `ask the PR author to enable "Allow edits from maintainers" or the fork may be owned by an organization.` @@ -303,21 +406,21 @@ export async function checkoutPrBranch( } else { // for same-repo PRs, push to origin $("git", ["config", `branch.${localBranch}.pushRemote`, "origin"], { log: false }); - $("git", ["config", `branch.${localBranch}.merge`, `refs/heads/${headBranch}`], { log: false }); + $("git", ["config", `branch.${localBranch}.merge`, `refs/heads/${pr.headRef}`], { log: false }); } // update toolState - toolState.issueNumber = pullNumber; + toolState.issueNumber = pr.number; if (isFork) { - toolState.pushUrl = `https://github.com/${headRepo.full_name}.git`; + toolState.pushUrl = `https://github.com/${pr.headRepoFullName}.git`; } // store push destination so push_branch can use it directly // git config is the primary mechanism, but toolState serves as a reliable fallback // in case git config reads fail in certain environments toolState.pushDest = { - remoteName: isFork ? `pr-${pullNumber}` : "origin", - remoteBranch: headBranch, + remoteName: isFork ? `pr-${pr.number}` : "origin", + remoteBranch: pr.headRef, localBranch, }; @@ -326,50 +429,6 @@ export async function checkoutPrBranch( event: "post-checkout", script: params.postCheckoutScript, }); - - return { - prNumber: pullNumber, - isFork, - forkUrl: isFork ? `https://github.com/${headRepo.full_name}.git` : undefined, - }; -} - -type DeepenForBeforeShaParams = { - gitToken: string; - beforeSha: string; -}; - -function deepenForBeforeSha(params: DeepenForBeforeShaParams): void { - const isShallow = - $("git", ["rev-parse", "--is-shallow-repository"], { log: false }).trim() === "true"; - if (!isShallow) return; - - const maxIterations = 10; - for (let i = 0; i < maxIterations; i++) { - try { - $("git", ["cat-file", "-t", params.beforeSha], { log: false }); - log.debug(`» before_sha ${params.beforeSha.slice(0, 7)} is now reachable`); - return; - } catch { - // not reachable yet, deepen - } - - log.debug( - `» deepening by 50 to reach before_sha ${params.beforeSha.slice(0, 7)} (attempt ${i + 1}/${maxIterations})` - ); - try { - $git("fetch", ["--deepen=50", "--no-tags", "origin"], { - token: params.gitToken, - }); - } catch { - log.debug(`» deepen for before_sha failed (force-push may have rewritten history)`); - return; - } - } - - log.debug( - `» before_sha ${params.beforeSha.slice(0, 7)} not reachable after ${maxIterations * 50} commits` - ); } export function CheckoutPrTool(ctx: ToolContext) { @@ -380,7 +439,28 @@ export function CheckoutPrTool(ctx: ToolContext) { "Returns diffPath pointing to the formatted diff file.", parameters: CheckoutPr, execute: execute(async ({ pull_number }) => { - await checkoutPrBranch(pull_number, { + const prResponse = await ctx.octokit.rest.pulls.get({ + owner: ctx.repo.owner, + repo: ctx.repo.name, + pull_number, + }); + + const headRepo = prResponse.data.head.repo; + if (!headRepo) { + throw new Error(`PR #${pull_number} source repository was deleted`); + } + + const pr: PrData = { + number: pull_number, + headSha: prResponse.data.head.sha, + headRef: prResponse.data.head.ref, + headRepoFullName: headRepo.full_name, + baseRef: prResponse.data.base.ref, + baseRepoFullName: prResponse.data.base.repo.full_name, + maintainerCanModify: prResponse.data.maintainer_can_modify, + }; + + await checkoutPrBranch(pr, { octokit: ctx.octokit, owner: ctx.repo.owner, name: ctx.repo.name, @@ -388,31 +468,39 @@ export function CheckoutPrTool(ctx: ToolContext) { toolState: ctx.toolState, shell: ctx.payload.shell, postCheckoutScript: ctx.postCheckoutScript, + beforeSha: ctx.toolState.beforeSha, }); - // for incremental review/rereview: deepen the clone to include before_sha - // so `git diff before_sha...HEAD` works without the agent needing to fetch manually - const event = ctx.payload.event; - if ("before_sha" in event && event.before_sha) { - deepenForBeforeSha({ - gitToken: ctx.gitToken, - beforeSha: event.before_sha, + const tempDir = process.env.PULLFROG_TEMP_DIR; + if (!tempDir) { + throw new Error( + "PULLFROG_TEMP_DIR not set - checkout_pr must run in pullfrog action context" + ); + } + + const headShort = ctx.toolState.checkoutSha!.slice(0, 7); + + // compute incremental diff if we have a beforeSha to compare against + let incrementalDiffPath: string | undefined; + if (ctx.toolState.beforeSha && ctx.toolState.checkoutSha) { + const beforeShort = ctx.toolState.beforeSha.slice(0, 7); + const incremental = computeIncrementalDiff({ + baseBranch: pr.baseRef, + beforeSha: ctx.toolState.beforeSha, + headSha: ctx.toolState.checkoutSha, }); + if (incremental) { + incrementalDiffPath = join( + tempDir, + `pr-${pull_number}-${beforeShort}-${headShort}-incremental.diff` + ); + writeFileSync(incrementalDiffPath, incremental); + log.info( + `» incremental diff computed (${incremental.length} bytes) → ${incrementalDiffPath}` + ); + } } - const pr = await ctx.octokit.rest.pulls.get({ - owner: ctx.repo.owner, - repo: ctx.repo.name, - pull_number, - }); - - const headRepo = pr.data.head.repo; - if (!headRepo) { - throw new Error(`PR #${pull_number} source repository was deleted`); - } - - ctx.toolState.checkoutSha = pr.data.head.sha; - // fetch PR files and format with line numbers const formatResult = await fetchAndFormatPrDiff({ octokit: ctx.octokit, @@ -422,28 +510,29 @@ export function CheckoutPrTool(ctx: ToolContext) { }); const diffPreview = formatResult.content.split("\n").slice(0, 100).join("\n"); log.debug(`formatted diff preview (first 100 lines):\n${diffPreview}`); - const tempDir = process.env.PULLFROG_TEMP_DIR; - if (!tempDir) { - throw new Error( - "PULLFROG_TEMP_DIR not set - checkout_pr must run in pullfrog action context" - ); - } - const diffPath = join(tempDir, `pr-${pull_number}.diff`); + const diffPath = join(tempDir, `pr-${pull_number}-${headShort}.diff`); writeFileSync(diffPath, formatResult.content); log.debug(`wrote diff to ${diffPath} (${formatResult.content.length} bytes)`); + const incrementalInstructions = incrementalDiffPath + ? ` IMPORTANT: incrementalDiffPath contains ONLY the changes since the last reviewed version ` + + `(computed via range-diff). you MUST read incrementalDiffPath FIRST to understand what changed, ` + + `then use diffPath for full PR context. do NOT skip the incremental diff.` + : ""; + return { success: true, - number: pr.data.number, - title: pr.data.title, - base: pr.data.base.ref, + number: prResponse.data.number, + title: prResponse.data.title, + base: pr.baseRef, localBranch: `pr-${pull_number}`, - remoteBranch: `refs/heads/${pr.data.head.ref}`, - isFork: headRepo.full_name !== pr.data.base.repo.full_name, - maintainerCanModify: pr.data.maintainer_can_modify, - url: pr.data.html_url, - headRepo: headRepo.full_name, + remoteBranch: `refs/heads/${pr.headRef}`, + isFork: pr.headRepoFullName !== pr.baseRepoFullName, + maintainerCanModify: pr.maintainerCanModify, + url: prResponse.data.html_url, + headRepo: pr.headRepoFullName, diffPath, + incrementalDiffPath, toc: formatResult.toc, instructions: `the diff file at diffPath contains a table of contents (TOC) at the top listing every changed file with its line range. ` + @@ -451,7 +540,8 @@ export function CheckoutPrTool(ctx: ToolContext) { `for example, if the TOC says "src/foo.ts → lines 5-42", read lines 5-42 from diffPath to see that file's changes. ` + `review files selectively based on relevance rather than reading everything sequentially. ` + `the local branch is 'localBranch' (pr-{number}), not the remote branch name. ` + - `when pushing, omit branchName to use the current branch. do not use remoteBranch as a local branch name.`, + `when pushing, omit branchName to use the current branch. do not use remoteBranch as a local branch name.` + + incrementalInstructions, } satisfies CheckoutPrResult; }), }); diff --git a/mcp/review.ts b/mcp/review.ts index eb3080a..4678437 100644 --- a/mcp/review.ts +++ b/mcp/review.ts @@ -1,5 +1,6 @@ import type { RestEndpointMethodTypes } from "@octokit/rest"; import { type } from "arktype"; +import { ghPullfrogMcpName } from "../external.ts"; import { apiFetch } from "../utils/apiFetch.ts"; import { getApiUrl } from "../utils/apiUrl.ts"; import { buildPullfrogFooter } from "../utils/buildPullfrogFooter.ts"; @@ -208,7 +209,9 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) { ) { const fromSha = ctx.toolState.checkoutSha; const toSha = latestHeadSha; - // advance checkoutSha so the next review submission tracks correctly + // store old checkoutSha as beforeSha so the next checkout_pr computes an incremental diff + ctx.toolState.beforeSha = fromSha; + // advance checkoutSha so the next review submission tracks correctly (just in case, checkout_pr will overwrite it again) ctx.toolState.checkoutSha = toSha; log.info( @@ -226,10 +229,9 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) { from: fromSha, to: toSha, instructions: - `New commits were pushed while you were reviewing. ` + - `Run \`git pull\` to fetch them, then review the incremental diff ` + - `with \`git diff ${fromSha}...HEAD\`. Submit another review covering ` + - `only the new changes. Do not repeat feedback from your previous review.`, + `new commits were pushed while you were reviewing. ` + + `call \`${ghPullfrogMcpName}/checkout_pr\` again to fetch the latest version — it will compute the incremental diff automatically. ` + + `submit another review covering only the new changes. do not repeat feedback from your previous review.`, }, }; } diff --git a/mcp/selectMode.ts b/mcp/selectMode.ts index 94324d2..fb062c2 100644 --- a/mcp/selectMode.ts +++ b/mcp/selectMode.ts @@ -117,9 +117,9 @@ ${learningsStep(6)}`, IncrementalReview: `### Checklist -1. Checkout the PR via \`${ghPullfrogMcpName}/checkout_pr\` — this returns PR metadata and a \`diffPath\`. Read the diff to identify the major areas of change. +1. Checkout the PR via \`${ghPullfrogMcpName}/checkout_pr\` — this returns PR metadata, \`diffPath\` (full diff), and \`incrementalDiffPath\` (changes since last reviewed version, if available). -2. Generate the incremental diff using the \`before_sha\` from EVENT DATA: \`git diff ...HEAD\`. This isolates only the new commits. If the command fails (e.g., force-push rewrote history), fall back to reviewing the full PR diff. +2. If \`incrementalDiffPath\` is present, read it to see what changed since the last review. This is a range-diff that isolates the net changes, filtering out base branch noise. If not present, fall back to reviewing the full PR diff. 3. Fetch previous reviews via \`${ghPullfrogMcpName}/list_pull_request_reviews\`. For the most recent Pullfrog review, call \`${ghPullfrogMcpName}/get_review_comments\` with the review ID to retrieve specific prior line-level feedback. diff --git a/mcp/server.ts b/mcp/server.ts index 3e0ca17..30b78dd 100644 --- a/mcp/server.ts +++ b/mcp/server.ts @@ -72,6 +72,9 @@ export interface ToolState { issueNumber?: number; // PR HEAD sha at checkout time — used to detect new commits pushed during a review checkoutSha?: string; + // SHA to diff incrementally against — set from event payload on first checkout, + // then from checkoutSha when review.ts detects new commits mid-review + beforeSha?: string; selectedMode?: string; backgroundProcesses: Map; browserDaemon?: BrowserDaemon | undefined; diff --git a/modes.ts b/modes.ts index cc0586e..0d50f01 100644 --- a/modes.ts +++ b/modes.ts @@ -134,12 +134,9 @@ ${permalinkTip} "Re-review a PR after new commits are pushed; focus on new changes since the last review", prompt: `Follow these steps to incrementally re-review the PR after new commits were pushed. Focus on what changed since the last review. -1. **CHECKOUT** - Call ${ghPullfrogMcpName}/checkout_pr with the PR number. This gives you the full PR diff via \`diffPath\`. +1. **CHECKOUT** - Call ${ghPullfrogMcpName}/checkout_pr with the PR number. This returns \`diffPath\` (full PR diff) and \`incrementalDiffPath\` (changes since last reviewed version, if available). -2. **INCREMENTAL DIFF** - EVENT DATA contains \`before_sha\` (the HEAD before this push). Generate the incremental diff: - \`git diff ...HEAD\` - This shows the changes introduced by this push. Cross-reference with previous reviews (step 3) to confirm coverage of all unreviewed changes — the full PR diff fills any gaps. - **If the diff command fails** (e.g., force-push rewrote history), fall back to reviewing the full PR diff from step 1. +2. **INCREMENTAL DIFF** - If \`incrementalDiffPath\` is present, read it to see what changed since the last review. This is a range-diff that isolates only the net changes, filtering out base branch noise. If not present, fall back to reviewing the full PR diff. 3. **FETCH PREVIOUS REVIEWS** - Use ${ghPullfrogMcpName}/list_pull_request_reviews to find previous Pullfrog reviews. For the most recent one, call ${ghPullfrogMcpName}/get_review_comments with the review ID to see specific line-level feedback. This lets you understand what feedback was already given. diff --git a/utils/rangeDiff.test.ts b/utils/rangeDiff.test.ts new file mode 100644 index 0000000..8794467 --- /dev/null +++ b/utils/rangeDiff.test.ts @@ -0,0 +1,236 @@ +import { describe, expect, it } from "vitest"; +import { postProcessRangeDiff } from "./rangeDiff.ts"; + +describe("postProcessRangeDiff", () => { + it("returns null for identical patches", () => { + const input = "1: abc1234 = 1: def5678 x"; + expect(postProcessRangeDiff(input)).toBeNull(); + }); + + it("returns null for empty input", () => { + expect(postProcessRangeDiff("")).toBeNull(); + expect(postProcessRangeDiff(" ")).toBeNull(); + }); + + it("returns null when no changes exist between versions", () => { + const input = [ + "1: abc1234 ! 1: def5678 x", + " ## src/file.ts ##", + " @@ src/file.ts", + " +const a = 1;", + " +const b = 2;", + ].join("\n"); + expect(postProcessRangeDiff(input)).toBeNull(); + }); + + it("strips inner diff prefix from content lines", () => { + const input = [ + "1: abc1234 ! 1: def5678 x", + " ## src/math.ts ##", + " @@ src/math.ts", + " + const a = 1;", + " -+ const b = 2;", + " ++ const b = 3;", + " + const c = 4;", + ].join("\n"); + expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(` + " ## src/math.ts ## + @@ src/math.ts + const a = 1; + - const b = 2; + + const b = 3; + const c = 4;" + `); + }); + + it("handles context lines (inner space prefix)", () => { + const input = [ + "1: abc1234 ! 1: def5678 x", + " ## src/file.ts ##", + " @@ src/file.ts", + " const base = true;", + " - const old = true;", + " + const new_ = true;", + " const end = true;", + ].join("\n"); + expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(` + " ## src/file.ts ## + @@ src/file.ts + const base = true; + -const old = true; + +const new_ = true; + const end = true;" + `); + }); + + it("trims context lines around changes", () => { + const contextBefore = Array.from( + { length: 9 }, + (_, i) => ` +const line${i + 1} = ${i + 1};` + ); + const contextAfter = Array.from( + { length: 6 }, + (_, i) => ` +const line${i + 11} = ${i + 11};` + ); + const input = [ + "1: abc1234 ! 1: def5678 x", + " ## src/large.ts ##", + " @@ src/large.ts", + ...contextBefore, + " -+const line10 = 10;", + " ++const line10 = 100;", + ...contextAfter, + ].join("\n"); + expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(` + " ## src/large.ts ## + @@ src/large.ts + ... + const line7 = 7; + const line8 = 8; + const line9 = 9; + -const line10 = 10; + +const line10 = 100; + const line11 = 11; + const line12 = 12; + const line13 = 13;" + `); + }); + + it("handles multiple files", () => { + const input = [ + "1: abc ! 1: def x", + " ## src/a.ts ##", + " @@ src/a.ts", + " -+old line a", + " ++new line a", + " ## src/b.ts ##", + " @@ src/b.ts", + " +unchanged", + " -+old line b", + " ++new line b", + ].join("\n"); + expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(` + " ## src/a.ts ## + @@ src/a.ts + -old line a + +new line a + ## src/b.ts ## + @@ src/b.ts + unchanged + -old line b + +new line b" + `); + }); + + it("handles new file added in new version", () => { + const input = [ + "1: abc ! 1: def x", + " +## src/new.ts (new) ##", + " +@@ src/new.ts (new)", + " ++export const x = 1;", + " ++export const y = 2;", + ].join("\n"); + expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(` + "+## src/new.ts (new) ## + +@@ src/new.ts (new) + +export const x = 1; + +export const y = 2;" + `); + }); + + it("handles file removed in new version", () => { + const input = [ + "1: abc ! 1: def x", + " -## src/old.ts ##", + " -@@ src/old.ts", + " --export const x = 1;", + ].join("\n"); + expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(` + "-## src/old.ts ## + -@@ src/old.ts + -export const x = 1;" + `); + }); + + it("filters out metadata section via context trimming", () => { + const input = [ + "1: abc ! 1: def x", + " @@ Metadata", + " Author: Test ", + " ## Commit message ##", + " x", + " ## src/file.ts ##", + " @@ src/file.ts", + " +const a = 1;", + " -+const b = 2;", + " ++const b = 3;", + " +const c = 4;", + ].join("\n"); + expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(` + " ## src/file.ts ## + @@ src/file.ts + const a = 1; + -const b = 2; + +const b = 3; + const c = 4;" + `); + }); + + it("uses custom context line count", () => { + const contextBefore = Array.from( + { length: 5 }, + (_, i) => ` +const line${i + 1} = ${i + 1};` + ); + const contextAfter = Array.from( + { length: 5 }, + (_, i) => ` +const line${i + 7} = ${i + 7};` + ); + const input = [ + "1: abc1234 ! 1: def5678 x", + " ## src/file.ts ##", + " @@ src/file.ts", + ...contextBefore, + " -+const changed = old;", + " ++const changed = new;", + ...contextAfter, + ].join("\n"); + expect(postProcessRangeDiff(input, 1)).toMatchInlineSnapshot(` + " ## src/file.ts ## + @@ src/file.ts + ... + const line5 = 5; + -const changed = old; + +const changed = new; + const line7 = 7;" + `); + }); + + it("handles two separate change regions in the same file", () => { + const middle = Array.from({ length: 10 }, (_, i) => ` +const mid${i + 1} = ${i + 1};`); + const input = [ + "1: abc ! 1: def x", + " ## src/file.ts ##", + " @@ src/file.ts", + " -+const first = old;", + " ++const first = new;", + ...middle, + " -+const second = old;", + " ++const second = new;", + ].join("\n"); + expect(postProcessRangeDiff(input)).toMatchInlineSnapshot(` + " ## src/file.ts ## + @@ src/file.ts + -const first = old; + +const first = new; + const mid1 = 1; + const mid2 = 2; + const mid3 = 3; + ... + const mid8 = 8; + const mid9 = 9; + const mid10 = 10; + -const second = old; + +const second = new;" + `); + }); +}); diff --git a/utils/rangeDiff.ts b/utils/rangeDiff.ts new file mode 100644 index 0000000..57dce83 --- /dev/null +++ b/utils/rangeDiff.ts @@ -0,0 +1,182 @@ +import { log } from "./cli.ts"; +import { $ } from "./shell.ts"; + +type ComputeIncrementalDiffParams = { + baseBranch: string; + beforeSha: string; + headSha: string; +}; + +/** + * computes the incremental diff between two versions of a PR using range-diff + * on virtual squash commits created via `git commit-tree`. + * + * each PR version is squashed into a single synthetic commit (merge-base → tip tree), + * then range-diff compares those two single-commit ranges. this: + * - isolates each version's net effect (base branch noise eliminated via per-version merge bases) + * - avoids commit-matching issues that raw range-diff has with rebases/squashes/reordering + * - creates only loose git objects, no branches or refs (unlike temp-branch squash approaches) + * + * unlike fetchAndFormatPrDiff/formatFilesWithLineNumbers, this output has no line numbers. + * range-diff compares *patches* (diffs-of-diffs), not file trees — its hunk headers are + * `@@ file.ts` breadcrumbs, not positional `@@ -X,Y +A,B @@` markers. reconstructing + * line numbers would require cross-referencing with the v2 diff or content-matching against + * file trees, both of which are fragile (duplicate lines, hunk boundary shifts after rebase). + * a structured interdiff approach (diff two parsed patches, compare only +/- keys via Myers) + * could approximate line numbers but loses semantic precision: range-diff understands patch + * structure natively (rename detection, hunk-aware matching, dual-prefix inner/outer changes), + * while flat key-sequence comparison can misalign duplicate lines and can't distinguish + * "new addition to the PR" from "existing code newly modified by the PR". range-diff is the + * right abstraction here — the incremental diff answers "how did the changeset evolve?", + * not "where in the file is this?", and forcing positional line numbers onto it would be + * semantically misleading. + * + * alternatives considered: + * - plain git diff (two-tree or three-dot): includes base branch changes, no PR isolation + * - patch-text diffing (interdiff / diff-of-diffs): fragile, hunk offset noise on rebase + * - range-diff on raw commit ranges: confused by commit reorganization across force-pushes + */ +export function computeIncrementalDiff(params: ComputeIncrementalDiffParams): string | null { + try { + // $1=beforeSha, $2=baseBranch, $3=headSha + const raw = $( + "sh", + [ + "-c", + 'old_base=$(git merge-base "$1" "origin/$2") && ' + + 'new_base=$(git merge-base "$3" "origin/$2") && ' + + "git range-diff --no-color " + + '"$old_base..$(git commit-tree "$1^{tree}" -p "$old_base" -m x)" ' + + '"$new_base..$(git commit-tree "$3^{tree}" -p "$new_base" -m x)"', + "--", + params.beforeSha, + params.baseBranch, + params.headSha, + ], + { log: false } + ); + + return postProcessRangeDiff(raw); + } catch (e) { + log.debug(`» range-diff failed: ${e instanceof Error ? e.message : String(e)}`); + return null; + } +} + +function isDiffPrefix(ch: string): boolean { + return ch === " " || ch === "+" || ch === "-"; +} + +/** + * transforms git range-diff output into a clean incremental diff. + * + * range-diff content lines have two prefix characters: + * 1st (outer): range-diff level — space (same in both), + (new only), - (old only) + * 2nd (inner): original diff level — space (context), + (added), - (removed) + * + * stripping the inner prefix produces a standard unified-diff-like output where + * +/- means "changed between PR versions" rather than "changed vs base branch". + * + * uses a streaming approach: a ring buffer of before-context lines is flushed when + * a change is hit, then afterCount lines of after-context are emitted directly. + * nearest preceding ## / @@ headers are force-included when outside the context window. + */ +export function postProcessRangeDiff(raw: string, contextLines = 3): string | null { + if (!raw.trim()) return null; + if (/^\d+:\s+\w+\s+=\s+\d+:/m.test(raw)) return null; + + type Line = { prefix: string; from: number; to: number; seq: number }; + + const beforeBuf: Line[] = []; + let lastFileHdr: Line | null = null; + let lastHunkHdr: Line | null = null; + let fileHdrEmitted = true; + let hunkHdrEmitted = true; + + let out = ""; + let afterRemaining = 0; + let lastEmittedSeq = -2; + let seq = 0; + let hasChanges = false; + + function emit(line: Line) { + if (lastEmittedSeq >= 0 && line.seq > lastEmittedSeq + 1) out += (out ? "\n" : "") + "..."; + out += (out ? "\n" : "") + line.prefix + raw.slice(line.from, line.to); + lastEmittedSeq = line.seq; + if (lastFileHdr?.seq === line.seq) fileHdrEmitted = true; + if (lastHunkHdr?.seq === line.seq) hunkHdrEmitted = true; + } + + function flushBefore() { + if (lastFileHdr && !fileHdrEmitted) emit(lastFileHdr); + if (lastHunkHdr && !hunkHdrEmitted) emit(lastHunkHdr); + for (const line of beforeBuf) { + if (line.seq > lastEmittedSeq) emit(line); + } + beforeBuf.length = 0; + } + + let cursor = 0; + while (cursor < raw.length) { + const eol = raw.indexOf("\n", cursor); + const lineEnd = eol === -1 ? raw.length : eol; + + if (raw.charCodeAt(cursor) >= 48 && raw.charCodeAt(cursor) <= 57) { + cursor = lineEnd + 1; + continue; + } + + if (lineEnd - cursor >= 5 && raw.startsWith(" ", cursor)) { + const prefix = raw[cursor + 4]; + if (isDiffPrefix(prefix)) { + const contentPos = cursor + 5; + const isOuterChange = prefix !== " "; + let line: Line; + let isChange = false; + + if (contentPos >= lineEnd) { + line = { prefix, from: lineEnd, to: lineEnd, seq }; + } else if (isDiffPrefix(raw[contentPos])) { + isChange = isOuterChange; + line = { prefix, from: contentPos + 1, to: lineEnd, seq }; + } else { + line = { prefix, from: contentPos, to: lineEnd, seq }; + if ( + raw.startsWith("## ", contentPos) && + !raw.startsWith("## Commit message", contentPos) + ) { + lastFileHdr = line; + fileHdrEmitted = false; + lastHunkHdr = null; + hunkHdrEmitted = true; + } else if ( + raw.startsWith("@@", contentPos) && + !raw.startsWith("@@ Metadata", contentPos) + ) { + lastHunkHdr = line; + hunkHdrEmitted = false; + } + } + + if (isChange) { + hasChanges = true; + flushBefore(); + emit(line); + afterRemaining = contextLines; + } else if (afterRemaining > 0) { + emit(line); + afterRemaining--; + } else { + if (beforeBuf.length >= contextLines) beforeBuf.shift(); + beforeBuf.push(line); + } + + seq++; + } + } + + cursor = lineEnd + 1; + } + + return hasChanges ? out : null; +} diff --git a/utils/setup.ts b/utils/setup.ts index c9c3c49..75fa43b 100644 --- a/utils/setup.ts +++ b/utils/setup.ts @@ -64,7 +64,6 @@ export type SetupGitParams = GitContext; * setup git configuration and authentication for the repository. * - configures git identity (user.email, user.name) * - sets up authentication via gitToken (minimal contents:write) - * - for PR events, checks out the PR branch using shared helper * * gitToken is a minimal-permission token (contents + workflows) used for git operations. * it is assumed to be potentially exfiltratable, so it has limited scope.