Increase Metrics Collector repo-memory patch cap to prevent push_repo_memory gate failures - #49970
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review push_repo_memory patch size computation and ensure it does not measure the entire changes, just the diff. Reuse heleprs from push_evals_state |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated in 154bd8c.
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 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 0 new lines of code in business logic directories. |
|
|
🔍 PR TriageCategory: chore · Risk: low · Total score: 40/100
Raises Metrics Collector repo-memory patch-size cap (10KB → 128KB) to fix Recommended action:
|
There was a problem hiding this comment.
Pull request overview
Raises Metrics Collector’s repo-memory patch cap to handle larger daily snapshots.
Changes:
- Raises the cap to 128KB and recompiles the workflow.
- Changes global repo-memory Git staging behavior and tests.
- Adds an unrelated workflow-skill reference.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/metrics-collector.md |
Configures the larger patch cap. |
.github/workflows/metrics-collector.lock.yml |
Propagates the cap into generated runtime configuration. |
actions/setup/js/push_repo_memory.cjs |
Scopes Git operations to copied memory paths. |
actions/setup/js/push_repo_memory.test.cjs |
Updates source assertions for scoped Git operations. |
.github/skills/agentic-workflows/SKILL.md |
Adds an observability-document reference. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
| } | ||
|
|
||
| // Check if we have any changes to commit | ||
| const changedPathspecs = Array.from(new Set(filesToCopy.map(file => file.relativePath))).sort(); |
| // Check if we have any changes to commit | ||
| const changedPathspecs = Array.from(new Set(filesToCopy.map(file => file.relativePath))).sort(); | ||
|
|
||
| // Check if we have any changes to commit, scoped to managed memory files only. |
There was a problem hiding this comment.
The changes are correct and consistent across all layers.
- The 128KB patch cap is a reasonable fix for observed gate failures on large daily metrics snapshots.
- Scoping
git status --porcelainandgit add --sparsetochangedPathspecsis a correctness improvement that avoids accidentally counting or staging files outside the managed memory scope. - The
cwdparameter is already accepted bygetStagedPatchDiffSizeBytes, so that wiring is correct. - The empty-pathspecs fallback (
git add --sparse .) preserves existing behavior. - Tests accurately reflect the new code paths.
- Lock file is consistent with the workflow source.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 22.7 AIC · ⌖ 10.3 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
Key ObservationsBoth tests are source verification tests that validate the production implementation matches critical security and functional invariants:
Test changes align with production changes: The assertions were updated to reflect the new pathspec-scoped architecture (using Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — one minor dead-code finding; overall the fix is clean and well-reasoned.
📋 Key Themes & Highlights
Key Themes
- Root cause properly addressed: The 10KB cap was genuinely too small for ~131KB daily metrics snapshots; raising to 128KB is a rational, measured increase.
- Pathspec-scoped git ops: Refactoring
git statusandgit addto target only managed memory paths is the right defensive move for sparse-checkout environments. - Dead branch: The
else { addArgs.push(".") }fallback ingit addis unreachable (noted in inline comment).
Positive Highlights
- ✅ PR description clearly documents the root cause with concrete byte counts.
- ✅
metrics-collector.lock.ymlrecompiled in sync — no config drift. - ✅
cwd: workspaceDirconsistently threaded through all git calls. - ✅ Tests updated to assert the new pathspec-based behavior rather than just removing the old assertion.
- ✅ Comment in
metrics-collector.mdexplains why 128KB is needed, which helps future reviewers reason about the cap.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 37.9 AIC · ⌖ 8.03 AIC · ⊞ 7.1K
Comment /matt to run again
| const addArgs = ["add", "--sparse"]; | ||
| if (changedPathspecs.length > 0) { | ||
| addArgs.push("--", ...changedPathspecs); | ||
| } else { |
There was a problem hiding this comment.
[/diagnosing-bugs] Dead code: the else { addArgs.push(".") } branch (lines 546–548) is unreachable — filesToCopy is guaranteed non-empty at this point (line 400 returns early when it is empty), so changedPathspecs will always be non-empty here.
💡 Suggestion
Remove the dead branch and rely on the early-return guarantee:
// changedPathspecs is always non-empty here (filesToCopy emptiness is checked at line 400)
addArgs.push("--", ...changedPathspecs);Leaving the fallback "." in place makes the sparse-checkout safety invariant harder to reason about and could mask a future regression if the early-return is ever removed.
@copilot please address this.
|
@copilot sous-chef triage: Failed checks: Please refresh the branch if needed and then run the skill.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Metrics Collector’s
push_repo_memoryjob failed when a daily metrics update generated a ~131KB patch against a 10KB cap (12KB effective with overhead). This change raises the workflow’s patch-size limit to accommodate current metrics payload size while preserving existing retention/pruning behavior.Scope: Repo-memory cap adjustment (Metrics Collector)
.github/workflows/metrics-collector.mdto set a higher repo-memory patch limit:tools.repo-memory.max-patch-size: 131072(128KB)Scope: Compiled workflow parity
.github/workflows/metrics-collector.lock.ymlso runtime config matches source:push_repo_memory.memories[].max_patch_sizenow131072push_repo_memoryenvMAX_PATCH_SIZEnow131072Scope: No behavioral change outside cap
metrics/daily/*.jsonis unchanged.Run: https://github.com/github/gh-aw/actions/runs/30820387900