Fix push_to_pull_request_branch patch size to use incremental net diff#28198
Fix push_to_pull_request_branch patch size to use incremental net diff#28198
Conversation
…tal net diff Agent-Logs-Url: https://github.com/github/gh-aw/sessions/136f7436-93d2-43c1-a01f-4b7f03a979ee Co-authored-by: mrjf <180956+mrjf@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes push_to_pull_request_branch max_patch_size enforcement for long-running PR branches by checking the incremental net diff against the current PR branch head, instead of relying on cumulative/transport patch size.
Changes:
generate_git_patch.cjs: computes and returnsdiffSize(netgit diffsize) in incremental mode and blocks falling back to non-incremental strategies.safe_outputs_handlers.cjs: forwardsdiffSizeasdiff_sizein the safe-output entry.push_to_pull_request_branch.cjs+ tests: prefermessage.diff_sizefor size validation, with fallback behavior covered by new unit/integration tests.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/generate_git_patch.cjs | Adds incremental net diff sizing (diffSize) and prevents non-incremental fallback strategies. |
| actions/setup/js/safe_outputs_handlers.cjs | Persists diffSize into safe outputs as diff_size. |
| actions/setup/js/push_to_pull_request_branch.cjs | Uses diff_size (when present) for max_patch_size checks; logs both file size and checked size. |
| actions/setup/js/push_to_pull_request_branch.test.cjs | Adds unit tests for preferring diff_size and fallback behavior. |
| actions/setup/js/git_patch_integration.test.cjs | Adds integration test reproducing long-running branch scenario and asserting diffSize is small. |
| .changeset/patch-push-pr-incremental-size-check.md | Documents the behavior change in a changeset entry. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 4
| 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}.`, |
There was a problem hiding this comment.
In incremental mode, this error path triggers when commitCount > 0 but git format-patch produced empty output (since commitCount === 0 returns 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 include commitCount/range in the error to aid diagnosis.
| error: `Cannot generate incremental patch: no incremental commits found between ${baseRef} and ${branchName}.`, | |
| error: `Cannot generate incremental patch: git format-patch produced no output for ${baseRef}..${branchName} despite ${commitCount} incremental commit(s).`, |
| // 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)`); | ||
| } |
There was a problem hiding this comment.
execGitSync(["diff", "--binary", ...]) captures the entire diff output into memory as a UTF-8 string just to measure its byte length. For larger incremental diffs this can be slow and memory-heavy (and may hit the 100MB maxBuffer, making diffSize null and falling back). Consider measuring diff size without buffering the full output (e.g., write diff to a temp file via git diff --output <file> and stat it, or stream to a byte counter) so size measurement is O(1) memory.
| core.info(`${sizeLabel}: ${sizeForCheckKb} KB (maximum allowed: ${maxSizeKb} KB)`); | ||
|
|
||
| if (sizeForCheckKb > maxSizeKb) { | ||
| const msg = `Patch size (${sizeForCheckKb} KB) exceeds maximum allowed size (${maxSizeKb} KB)`; |
There was a problem hiding this comment.
When message.diff_size is used, the rejection error still says "Patch size (...)". This can be confusing because the value being enforced is the incremental net diff, not the transport file size. Consider tailoring the error message to indicate whether the limit was exceeded by the incremental diff size vs. the patch file size (and possibly include both values, since you already log them).
| 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)`; |
| @@ -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)`); | |||
|
|
|||
There was a problem hiding this comment.
max_patch_size validation still only runs for patch transport (!hasBundleFile). If bundle transport is configured, large bundles can bypass the size limit entirely. If max_patch_size is intended to cap push size regardless of transport (as implied by the PR description mentioning bundle backward-compat), consider enforcing the limit against bundle_path file size (or explicitly document that the limit is patch-transport-only).
🧪 Test Quality Sentinel ReportTest Quality Score: 73/100
Test Classification DetailsView All Test Classifications
Minor Suggestions
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@copilot review all comments |
|
Hey The PR includes thorough test coverage (3 new unit tests + 1 integration test reproducing the exact failure scenario), a clear and detailed description, a changeset entry, and stays tightly focused on the single bug. This looks ready for maintainer review!
|
…ile, enforce limit on bundles Agent-Logs-Url: https://github.com/github/gh-aw/sessions/9d54ee1e-974b-45f3-a4d8-d78cd2281713 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all four review comments in abf826f:
All 6954 JS tests pass. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot move git patch utilities to separate file, add tests using local git repo |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/0cfd4b7c-1e48-4723-9182-d3ba7abd9656 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 960e574. Moved git patch utilities into a new
Added All 6972 JS tests pass. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
On long-running branches (e.g. autoloop iteration branches),
push_to_pull_request_branch'smax_patch_sizecheck measured the cumulative diff from the default branch instead of the incremental change against the existing PR branch head. As accepted iterations accumulated, every push eventually exceeded the limit even when each iteration only changed a few KB.Changes
git_patch_utils.cjs(new module): extracts reusable git patch helpers out ofgenerate_git_patch.cjs:sanitizeForFilename,sanitizeBranchNameForPatch,sanitizeRepoSlugForPatch,getPatchPath,getPatchPathForRepobuildExcludePathspecs— builds["--", ":(exclude)<pat>", ...]from an excludedFiles list.computeIncrementalDiffSize— measures the net diff between two refs in a git repo usinggit diff --binary --output=<tmpfile>+fs.statSync(O(1) memory, withfinallycleanup of the temp file). Returnsnullon missing args or git failure.generate_git_patch.cjs(incremental mode):git_patch_utils.cjsfor filename sanitization, path construction, pathspec building, and incremental diff-size measurement. Re-exports the helpers for backward compatibility with existing callers.diffSize= the netgit diff <baseRef>..<branch>size alongside the existingpatchSize. The format-patch file accumulates per-commit metadata + per-commit diffs and is not a faithful proxy for the net change.GITHUB_SHA..HEAD, merge-base with default branch) when Strategy 1 fails to write a patch — those paths produce checkout-base diffs and silently violate the incremental contract. Error message includescommitCountand thebaseRef..branchrange for diagnosis.safe_outputs_handlers.cjs: forwarddiffSizeonto the safe-output entry asdiff_size.push_to_pull_request_branch.cjs:message.diff_sizefor themax_patch_sizecheck; fall back to the transport file size when absent (backward compatible with older MCP outputs).max_patch_sizeon bundle transport as well, using the on-disk bundle file size (ormessage.diff_sizewhen provided). Bundles no longer silently bypass the limit.diff_sizeis authoritative.actions/setup/setup.sh: register the newgit_patch_utils.cjsinSAFE_OUTPUTS_FILESso it is copied to the runtime actions directory.Tests
git_patch_utils.test.cjs— covers the pure helpers and exercisescomputeIncrementalDiffSizeagainst a real local git repository (temp dir +git init+ real commits + realgit diff --output): size matches independently-computedgit diffbytes, temp file is always cleaned up, zero size for identical refs, excludedFiles pathspecs actually shrink the measured diff, null for invalid refs, null for missing args.push_to_pull_request_branch.test.cjs: prefersdiff_size, rejects whendiff_sizeexceeds, falls back to transport file size when absent, enforces limit on bundle file size, and prefersdiff_sizeover bundle file size.git_patch_integration.test.cjsreproducing the long-running branch scenario (~50 KB accumulated onorigin/<branch>, tiny new commit on top) and assertingdiffSizereflects only the new iteration.