Increase default max-patch-size from 1 MB to 4 MB and improve patch-size-exceeded error messages#39118
Conversation
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
|
Also make all the changes in this comment: #39107 (comment) |
|
@copilot Also make all the changes in this comment: #39107 (comment) |
…ssage Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
Done in the latest commit. The patch-size-exceeded error section in the failure issue now includes a collapsible "📥 Download the oversized patch to inspect or apply manually" block with |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR raises the default max-patch-size guardrail for safe-output PR operations from 1024 KB (1 MB) to 4096 KB (4 MB) across the Go compiler/config path, JS safe-output handlers, generated workflow lock files, and docs. It also improves the “patch size exceeded” failure context by adding downloadable patch artifact instructions.
Changes:
- Updated default
max-patch-sizeto 4096 KB across Go config parsing/handler defaults and JS handlers, plus updated tests and docs to match. - Enhanced patch-size-exceeded failure output to include a collapsible section with artifact download + manual inspection/apply instructions.
- Recompiled numerous
.lock.ymlworkflows so embedded handler configs reflect the new default.
Show a summary per file
| File | Description |
|---|---|
| scratchpad/safe-output-environment-variables.md | Updates env var docs to reflect new default max patch size. |
| pkg/workflow/safe_outputs_import_test.go | Updates test expectation/comment for new default. |
| pkg/workflow/safe_outputs_handler_registry.go | Updates Go-side handler fallback default to 4096 KB. |
| pkg/workflow/safe_outputs_config.go | Updates YAML-parse-time default injection to 4096 KB. |
| pkg/workflow/patch_size_validation_test.go | Updates expected compiled handler config JSON default. |
| pkg/workflow/compiler_types.go | Updates struct field comment to reflect new default. |
| pkg/workflow/compiler_safe_outputs_config_test.go | Updates compiler test for default patch size. |
| docs/src/content/docs/reference/safe-outputs.md | Updates reference docs default from 1024 → 4096. |
| docs/src/content/docs/reference/safe-outputs-pull-requests.md | Updates PR safe-outputs docs default from 1024 → 4096. |
| docs/src/content/docs/reference/frontmatter-full.md | Updates frontmatter docs default from 1024 → 4096. |
| actions/setup/js/push_to_pull_request_branch.test.cjs | Updates JS handler test output for new default. |
| actions/setup/js/push_to_pull_request_branch.cjs | Updates JS handler default to 4096 KB. |
| actions/setup/js/handle_agent_failure.test.cjs | Adds tests for new patch download instructions block. |
| actions/setup/js/handle_agent_failure.cjs | Adds patch download instructions to patch-size-exceeded error context; updates example default. |
| actions/setup/js/create_pull_request.cjs | Updates JS handler default to 4096 KB. |
| .github/workflows/weekly-safe-outputs-spec-review.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/weekly-editors-health-check.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/weekly-blog-post-writer.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/update-astro.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/unbloat-docs.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/ubuntu-image-analyzer.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/test-create-pr-error-handling.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/technical-doc-writer.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/spec-extractor.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/spec-enforcer.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/slide-deck-maintainer.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/schema-feature-coverage.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/ruflo-backed-task.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/refiner.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/q.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/pr-sous-chef.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/necromancer.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/mergefest.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/linter-miner.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/layout-spec-maintainer.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/jsweep.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/instructions-janitor.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/hourly-ci-cleaner.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/go-logger.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/glossary-maintainer.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/github-mcp-tools-report.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/functional-pragmatist.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/dictation-prompt.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/design-decision-gate.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/dependabot-worker.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/dead-code-remover.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/daily-workflow-updater.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/daily-safe-output-integrator.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/daily-rendering-scripts-verifier.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/daily-doc-updater.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/daily-compiler-threat-spec-optimizer.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/daily-community-attribution.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/daily-caveman-optimizer.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/daily-astrostylelite-markdown-spellcheck.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/craft.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/code-simplifier.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/code-scanning-fixer.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/cloclo.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/ci-coach.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/chaos-pr-bundle-fuzzer.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/workflows/avenger.lock.yml | Recompiled lock workflow to embed new default (4096). |
| .github/aw/syntax-agentic.md | Updates agentic syntax docs default from 1024 → 4096. |
| .github/aw/safe-outputs-runtime.md | Updates runtime docs default from 1024 → 4096. |
| .github/aw/safe-outputs-management.md | Updates management docs default from 1024 → 4096. |
| .github/aw/safe-outputs-content.md | Updates content docs default from 1024 → 4096. |
| .changeset/patch-increase-default-max-patch-size-4mb.md | Adds changeset entry documenting the default bump. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 80/80 changed files
- Comments generated: 4
| const signedCommits = config.signed_commits !== false; | ||
| const commitTitleSuffix = config.commit_title_suffix || ""; | ||
| const maxSizeKb = config.max_patch_size ? parseInt(String(config.max_patch_size), 10) : 1024; | ||
| const maxSizeKb = config.max_patch_size ? parseInt(String(config.max_patch_size), 10) : 4096; | ||
| const maxCount = config.max || 0; // 0 means no limit |
| const expiresHours = config.expires ? parseInt(String(config.expires), 10) : 0; | ||
| const maxCount = config.max || 1; // PRs are typically limited to 1 | ||
| const maxSizeKb = config.max_patch_size ? parseInt(String(config.max_patch_size), 10) : 1024; | ||
| const maxSizeKb = config.max_patch_size ? parseInt(String(config.max_patch_size), 10) : 4096; |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose, /tdd, and /grill-with-docs — commenting with minor cleanup suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Duplicate run-ID extraction (line 803 & 848): the same 6-line
runUrl.match()pattern is now duplicated three times in the file. Extracting anextractRunId(runUrl)helper would prevent divergence. - Dead ternary (line 829 & 879):
${runUrl ? ... : ""}inside anif (runId)block is always truthy — the else branch is unreachable. - Hardcoded YAML example (line 796):
8192and thedefault: 4096comment will become stale on the next default change; the error message already contains the current limit. - Inconsistent test coverage (test line 1471): the two new patch-size-exceeded tests don't assert
<details>is emitted, while the parallel patch-apply tests do.
Positive Highlights
- ✅ 4 MB is a much more sensible default — the original 1 MB silently wasted AI credits before failing at the push step.
- ✅ The default is updated consistently across Go config, Go handler registry, and both JS handlers — no split-brain defaults.
- ✅ Good use of progressive disclosure (
<details>) in the new error-message section. - ✅ Tests cover both the
runUrland no-runUrlbranches, which are the two real user-facing scenarios. - ✅ 246 lock-file recompilations correctly track the default change with zero manual edits required.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 577.3 AIC · ⌖ 13.8 AIC · ⊞ 27.9K
| context += yamlSnippet; | ||
|
|
||
| // Provide download instructions so the user can inspect what the agent generated | ||
| let runId = ""; |
There was a problem hiding this comment.
[/diagnose] The run-ID extraction pattern is duplicated verbatim from the patch-apply section below (line ~848) and again from an existing block at line ~2848. Three copies of the same 6-line runUrl.match(...) routine is a regression-risk: if the URL format ever changes, all three need updating.
💡 Suggested refactor
Extract a small pure helper at the top of the module:
/**
* Extracts the numeric run ID from a GitHub Actions run URL.
* `@param` {string} runUrl e.g. "https://github.com/owner/repo/actions/runs/12345678"
* `@returns` {string} run ID, or empty string if not found
*/
function extractRunId(runUrl) {
if (!runUrl) return "";
const m = runUrl.match(/\/actions\/runs\/(\d+)/);
return m ? m[1] : "";
}Then every caller becomes a one-liner:
const runId = extractRunId(runUrl);This makes the intent obvious, keeps tests focused on one place, and means a URL-format change only touches one function.
| git push origin aw/manual-apply | ||
| gh pr create --head aw/manual-apply | ||
| \`\`\` | ||
| ${runUrl ? `\nThe patch artifact is available at: [View run and download artifacts](${runUrl})\n` : ""}`; |
There was a problem hiding this comment.
[/diagnose] The runUrl ? ternary is dead code: we are already inside if (runId), and runId can only be non-empty when runUrl is truthy and the regex matched. The else branch "" is unreachable, making this branch condition misleading.
💡 Simplify to
// Before (inside `if (runId)`, runUrl is always truthy here)
${runUrl ? `\nThe patch artifact is available at: [View run and download artifacts](${runUrl})\n` : ""}
// After
\nThe patch artifact is available at: [View run and download artifacts](${runUrl})\nSame simplification applies to the parallel block in the patch-apply section (~line 879).
| for (const type of affectedTypes) { | ||
| const yamlKey = typeToYamlKey[type] || type.replace(/_/g, "-"); | ||
| yamlSnippet += ` ${yamlKey}:\n max-patch-size: 2048 # Example: double the default limit (in KB, default: 1024)\n`; | ||
| yamlSnippet += ` ${yamlKey}:\n max-patch-size: 8192 # Example: double the default limit (in KB, default: 4096)\n`; |
There was a problem hiding this comment.
[/grill-with-docs] The example value 8192 and comment # Example: double the default limit (in KB, default: 4096) are hardcoded. If the default changes again, this snippet will silently become stale. Consider deriving the suggested value from the actual limit in the error string, which already contains the current limit (e.g. maximum allowed size (4096 KB)).
| expect(result).toContain("https://github.com/owner/repo/pull/99"); | ||
| }); | ||
|
|
||
| it("includes download instructions with run ID when runUrl is provided for patch size exceeded", () => { |
There was a problem hiding this comment.
[/tdd] The two new tests for the patch-size-exceeded download block don't assert that a <details> progressive-disclosure block is present, even though the implementation adds one. The parallel patch-apply tests (lines ~1535–1558) do assert <details> — worth adding the same assertion here for consistency and to prevent the disclosure block from being accidentally removed.
|
@copilot review all comments and address unresolved review feedback. Then refresh the branch and rerun checks.
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed for PR #39118: it does not have the 'implementation' label and has only 8 new lines of code in business logic directories (≤100 threshold). |
|
@copilot run pr-finisher skill |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (6 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §27474598968
|
…ctRunId helper, test alignment Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…onsistency Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. The following changes were made in 4c62bd5 (building on the previous download-instructions commit):
All review threads addressed in commits
Hand-off: CI must be re-triggered by a maintainer (close/reopen PR or push) before merge. |
The 1 MB default for
max-patch-sizeoncreate-pull-requestandpush-to-pull-request-branchwas too aggressive — workflows that auto-generate lock files or large artifacts would burn AI credits and then fail at the push step. Raises the default to 4 MB and improves the error message shown when the limit is hit.Changes
safe_outputs_config.go): default injected at YAML parse time changed1024 → 4096KBsafe_outputs_handler_registry.go): fallback defaults in bothcreate_pull_requestandpush_to_pull_request_branchhandlers updatedcreate_pull_request.cjsandpush_to_pull_request_branch.cjsupdatedhandle_agent_failure.cjs): patch-size-exceeded failure issue now includes a collapsible "📥 Download the oversized patch to inspect or apply manually" section withgh run downloadinstructions, so users can retrieve and inspect (or manually apply) the patch the agent generated — matching the existing instructions in the patch-apply-failure sectionrunUrl).lock.ymlworkflow files recompiled with the new defaultdocs/,scratchpad/, and.github/aw/filesThe per-output
max-patch-sizeoverride still works as before for anyone needing a tighter or larger limit.