-
Notifications
You must be signed in to change notification settings - Fork 425
Fix push_to_pull_request_branch patch size to use incremental net diff #28198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
fc477a9
d99e2a6
abf826f
960e574
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -294,6 +294,23 @@ async function generateGitPatch(branchName, baseBranch, options = {}) { | |
| patchLines: 0, | ||
| }; | ||
| } | ||
|
|
||
| // In incremental mode, the patch must be measured relative to the existing | ||
| // PR branch head (origin/<branch>), never relative to the default branch. | ||
| // If Strategy 1 did not produce a patch (e.g. format-patch yielded empty | ||
| // output for an unusual commit shape), do NOT fall through to Strategy 2 | ||
| // or Strategy 3 — those use GITHUB_SHA..HEAD or merge-base with a remote | ||
| // ref and would produce a checkout-base diff (which can be many MB on a | ||
| // long-running branch). Returning an explicit error preserves the | ||
| // "incremental" contract that the patch reflects only the new commits. | ||
| if (!patchGenerated && mode === "incremental") { | ||
| debugLog(`Strategy 1 (incremental): No patch generated from ${baseRef}..${branchName}, refusing to fall through to checkout-base strategies`); | ||
| return { | ||
| success: false, | ||
| error: `Cannot generate incremental patch: no incremental commits found between ${baseRef} and ${branchName}.`, | ||
| patchPath: patchPath, | ||
| }; | ||
| } | ||
| } catch (branchError) { | ||
| // Branch does not exist locally | ||
| debugLog(`Strategy 1: Branch '${branchName}' does not exist locally - ${getErrorMessage(branchError)}`); | ||
|
|
@@ -450,12 +467,32 @@ async function generateGitPatch(branchName, baseBranch, options = {}) { | |
| }; | ||
| } | ||
|
|
||
| debugLog(`Final: SUCCESS - patchSize=${patchSize} bytes, patchLines=${patchLines}, baseCommit=${baseCommitSha || "(unknown)"}`); | ||
| // In incremental mode, also compute the net diff size between baseRef and the | ||
| // branch tip. The format-patch file size (patchSize) is the sum of every | ||
| // commit's individual diff plus per-commit metadata headers, which can be | ||
| // significantly larger than the actual net change. Consumers (e.g. | ||
| // push_to_pull_request_branch) should validate `max_patch_size` against the | ||
| // incremental net diff so the limit reflects how much the branch will | ||
| // actually change, not the cumulative size of the commit history. See: | ||
| // https://github.com/github/gh-aw/issues for the long-running branch case. | ||
| let diffSize = null; | ||
| if (mode === "incremental" && baseCommitSha && branchName) { | ||
| try { | ||
| const diffOutput = execGitSync(["diff", "--binary", `${baseCommitSha}..${branchName}`, ...excludeArgs()], { cwd }); | ||
| diffSize = Buffer.byteLength(diffOutput, "utf8"); | ||
| debugLog(`Final: Computed incremental net diffSize=${diffSize} bytes (baseRef=${baseCommitSha}..${branchName})`); | ||
| } catch (diffErr) { | ||
| debugLog(`Final: Failed to compute incremental net diffSize - ${getErrorMessage(diffErr)} (will fall back to patchSize)`); | ||
| } | ||
|
||
| } | ||
|
|
||
| debugLog(`Final: SUCCESS - patchSize=${patchSize} bytes, patchLines=${patchLines}, diffSize=${diffSize ?? "(n/a)"} bytes, baseCommit=${baseCommitSha || "(unknown)"}`); | ||
| return { | ||
| success: true, | ||
| patchPath: patchPath, | ||
| patchSize: patchSize, | ||
| patchLines: patchLines, | ||
| diffSize: diffSize, | ||
| baseCommit: baseCommitSha, | ||
| }; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -179,10 +179,26 @@ async function main(config = {}) { | |||||||||
| const patchSizeBytes = Buffer.byteLength(patchContent, "utf8"); | ||||||||||
| const patchSizeKb = Math.ceil(patchSizeBytes / 1024); | ||||||||||
|
|
||||||||||
| core.info(`Patch size: ${patchSizeKb} KB (maximum allowed: ${maxSizeKb} KB)`); | ||||||||||
|
|
||||||||||
| if (patchSizeKb > maxSizeKb) { | ||||||||||
| const msg = `Patch size (${patchSizeKb} KB) exceeds maximum allowed size (${maxSizeKb} KB)`; | ||||||||||
| // Prefer the incremental net diff size (computed by the MCP server when | ||||||||||
| // the patch was generated in incremental mode) over the format-patch file | ||||||||||
| // size for `max_patch_size` validation. The format-patch file accumulates | ||||||||||
| // per-commit metadata and per-commit diffs, which can be much larger than | ||||||||||
| // the actual net change relative to the existing PR branch head — and on | ||||||||||
| // a long-running branch (e.g. autoloop iteration branches) this drift | ||||||||||
| // grows monotonically even when each iteration only changes a few KB. | ||||||||||
| // The diff size, in contrast, is the size of `git diff origin/<branch>..HEAD` | ||||||||||
| // and is what the user actually expects `max-patch-size` to cap. | ||||||||||
| const diffSizeBytesRaw = message.diff_size; | ||||||||||
| const haveDiffSize = typeof diffSizeBytesRaw === "number" && diffSizeBytesRaw >= 0; | ||||||||||
| const sizeForCheckBytes = haveDiffSize ? diffSizeBytesRaw : patchSizeBytes; | ||||||||||
| const sizeForCheckKb = Math.ceil(sizeForCheckBytes / 1024); | ||||||||||
| const sizeLabel = haveDiffSize ? "Incremental patch size" : "Patch size"; | ||||||||||
|
|
||||||||||
| core.info(`Patch file size: ${patchSizeKb} KB`); | ||||||||||
| core.info(`${sizeLabel}: ${sizeForCheckKb} KB (maximum allowed: ${maxSizeKb} KB)`); | ||||||||||
|
|
||||||||||
|
||||||||||
| if (sizeForCheckKb > maxSizeKb) { | ||||||||||
| const msg = `Patch size (${sizeForCheckKb} KB) exceeds maximum allowed size (${maxSizeKb} KB)`; | ||||||||||
|
||||||||||
| const msg = `Patch size (${sizeForCheckKb} KB) exceeds maximum allowed size (${maxSizeKb} KB)`; | |
| const msg = haveDiffSize | |
| ? `Incremental diff size (${sizeForCheckKb} KB) exceeds maximum allowed size (${maxSizeKb} KB). Patch file size: ${patchSizeKb} KB.` | |
| : `Patch file size (${sizeForCheckKb} KB) exceeds maximum allowed size (${maxSizeKb} KB)`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In incremental mode, this error path triggers when
commitCount > 0butgit format-patchproduced empty output (sincecommitCount === 0returns earlier). The message "no incremental commits found" is misleading in that case and will send users down the wrong debugging path. Consider wording that reflects the actual failure (e.g., format-patch produced no output / could not generate patch content) and includecommitCount/range in the error to aid diagnosis.