fix(push_repo_memory): correct misleading "git diff format overhead" framing in pre-check error message#43301
Conversation
The pre-check in safe_outputs_handlers.cjs measures total raw file sizes in the memory directory, but the error message incorrectly stated '20% overhead for git diff format' — wording that only applies to the push gate in push_repo_memory.cjs which actually runs git diff. - Fix the error message to say 'configured max-patch-size: X KB' instead of 'configured limit: X KB with 20% overhead for git diff format' - Fix the success message to remove 'with 20% overhead' - Fix the comment at the effective-limit calculation to explain the real reason (matching the push gate factor, not git diff format overhead) - Fix the total-size check comment to remove the misleading 'mirrors the same calculation in push_repo_memory.cjs' framing The 20% buffer (max_patch_size × 1.2) is kept unchanged so that a full rewrite of all memory content stays within the push gate's effective limit. Add changeset entry. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected in src/, lib/, pkg/, internal/, app/, core/, domain/, services/, api/). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR corrects misleading wording in the push_repo_memory safe-outputs pre-check so its messages and comments accurately describe what is being measured (raw file-size sum) and why the max_patch_size × 1.2 threshold is used (alignment with the push gate’s 1.2× factor), without incorrectly attributing it to “git diff format overhead”.
Changes:
- Updated in-code comments in the safe-outputs handler to correctly describe the pre-check’s total-size calculation and its relationship to the push gate.
- Revised the pre-check failure message to remove the incorrect “git diff format overhead” framing.
- Added a changeset entry documenting the messaging/comment clarification.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/safe_outputs_handlers.cjs | Adjusts comments and user-facing success/error messages for the repo-memory size pre-check. |
| .changeset/fix-push-repo-memory-misleading-error-message.md | Release note describing the corrected framing for the pre-check messaging/comments. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes are accurate and an improvement over the previous misleading wording.
Summary:
- Error message:
"with 20% overhead for git diff format"→"configured max-patch-size: X KB"✅ - Success message: spurious
"with 20% overhead"qualifier removed ✅ - Comments: accurately distinguish raw file-size sum (pre-check) vs. diff additions (push gate) ✅
The actual threshold logic (maxPatchSize × 1.2) is unchanged — only the explanation is corrected. The changeset entry is appropriately scoped as a patch. No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 17.9 AIC · ⌖ 5.91 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — good targeted fix; two minor follow-up suggestions.
📋 Key Themes & Highlights
Key Themes
- Error message UX gap: After removing the (wrong) "20% overhead" note, the message now shows two diverging KB values without explaining the 1.2× multiplier that connects them. This could leave users puzzled about the discrepancy.
- Residual comment ambiguity: The inline comment at line 1535–1536 says the factor is "matching the push gate" — which may still imply the two checks are equivalent, when only the factor is shared, not the measurement mechanism.
Positive Highlights
- ✅ Root cause correctly identified: the wrong rationale in comments and messages was the bug, not the threshold itself.
- ✅ Changeset entry is accurate and thorough — clearly communicates what changed and why.
- ✅ The 1.2× threshold is intentionally preserved with a proper explanation in the PR body.
- ✅ Small, surgical diff — easy to review and reason about.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 41.4 AIC · ⌖ 5.22 AIC · ⊞ 6.6K
Comment /matt to run again
There was a problem hiding this comment.
Review: comment accuracy on the guarantee in new comments
The existing review comments (lines 1537 and 1651) have already surfaced the two most important issues. I found one additional concern on the new comment at line 1634 not yet covered.
📋 Findings summary
Not blocking, but worth noting: The new comment at line 1634 says "a full rewrite of all memory content would remain within the push gate limit". This is not strictly guaranteed. The push gate counts Buffer.byteLength(line + "\n") for every +-prefixed diff line — each added line carries one extra byte (the + prefix) on top of the file content. Raw file-size sums omit this per-line overhead. For large files with many lines, the pre-check could pass while the push gate still fails. The comment should soften this to "is designed to keep total content within the push gate limit" or "provides a conservative proxy for the push gate limit".
Otherwise the diff is comment/message-only with no logic changes. The max_patch_size × 1.2 threshold is unchanged and the intent of the fix is correct.
🔎 Code quality review by PR Code Quality Reviewer · 153.5 AIC · ⌖ 6.95 AIC · ⊞ 5.4K
Comment /review to run again
|
🛠️ Agentic Maintenance updated this pull request branch. |
|
🎉 This pull request is included in a new release. Release: |
The
push_repo_memoryMCP pre-check sums raw file sizes in the memory directory, but its error message claimed the 20% buffer exists for "git diff format overhead" — wording copied frompush_repo_memory.cjswhere it actually applies (that gate runsgit diff --cachedand counts added lines). The comment also falsely asserted it "mirrors the same calculation." This made failures look like diff-size problems when the real issue was total accumulated tree size.Changes
"configured limit: X KB with 20% overhead for git diff format"→"configured max-patch-size: X KB"— describes what was actually measured"with 20% overhead"qualifierThe
max_patch_size × 1.2threshold itself is unchanged — a full rewrite of all memory content must fit within the push gate's effective limit, so the factor is still valid; only the stated rationale was wrong.