Repository navigation
Fix Metrics Collector full-snapshot persistence budgets - #68628
Conversation
Couple the file, two-copy patch, and upload budgets; validate complete daily/latest snapshots before upload and push. Preserve backing-branch history with trusted validation phase metadata and bound uploaded daily history by its UTC filename. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
There was a problem hiding this comment.
🟢 Approval recommended
The implementation is consistent across source, runtime, tests, documentation, and generated output with no unresolved defects found.
0 open findings
What changed in this PR
Fixes Metrics Collector persistence limits while adding phase-aware, read-only repo-memory validation.
Changes:
- Raises snapshot and patch budgets for complete metrics data.
- Adds baseline/upload/push validation phases and retention checks.
- Adds regression coverage and updates generated workflow/docs.
No confirmed security regressions found; the referenced run confirms the prior 447,189-byte file-cap failure, and generated validator copies match the source and manifest hash.
| File | Description |
|---|---|
.github/workflows/metrics-collector.md |
Defines budgets, validation, and UTC retention pruning. |
.github/workflows/metrics-collector.lock.yml |
Regenerates the compiled workflow. |
actions/setup/js/memory_custom_validation.cjs |
Exposes trusted validation-phase metadata. |
actions/setup/js/memory_custom_validation.test.cjs |
Tests phase values and compatibility default. |
actions/setup/js/validate_memory_step.cjs |
Assigns baseline and upload phases. |
actions/setup/js/push_repo_memory.cjs |
Assigns push phase before commit and retry. |
actions/setup/js/metrics_collector_workflow.test.cjs |
Covers budgets, retention, artifacts, and runtime wiring. |
docs/src/content/docs/reference/repo-memory.md |
Documents memoryPhase behavior. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Blocking issue: the new retention/phase split makes the Metrics Collector stop persisting snapshots once the repo-memory branch contains more than 30 days of history.
### Why I am requesting changes
`baseline` still runs the 30-day upload-window check against the full backing-branch checkout, but this workflow deliberately does not delete omitted daily snapshots from that branch. After enough daily runs, baseline validation starts failing before the agent runs, and `push_repo_memory` cannot repair the state because it stages uploaded files only, not deletions. That turns repo-memory persistence into a permanent warn-and-skip-artifact path instead of a temporary retention guard.🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 29.3 AIC · ⌖ 5.97 AIC · ⊞ 19.8K
Comment /review to run again
| if (memoryPhase === "push") return; | ||
| const cutoff = new Date(`${day}T00:00:00Z`); | ||
| cutoff.setUTCDate(cutoff.getUTCDate() - 29); | ||
| const oldestDay = cutoff.toISOString().slice(0, 10); |
There was a problem hiding this comment.
This phase split will deadlock metrics persistence after the branch ages past 30 days because baseline still validates the full backing-branch checkout against the upload window while push_repo_memory never stages deletions for omitted snapshots.
💡 Why this blocks merge
On day 31, the branch still contains older `metrics/daily/*.json` files, so `validateRepoMemoryBaseline(... phase: "baseline")` starts failing with the 30-day error before the agent runs. The workflow cannot repair that state: the cleanup step only prunes the upload directory, and `push_repo_memory` stages only `filesToCopy`, so historical files that were omitted from the artifact remain on the branch forever. That turns this into a permanent warn-and-skip-artifact condition for every later run.Please either commit deletions for pruned snapshots, or exempt baseline from the upload-retention check so the collector can keep persisting new data.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — this is a well-instrumented bug fix with measured evidence and comprehensive regression coverage; approving with one minor test-robustness suggestion.
📋 Key Themes & Highlights
Key Themes
- Root cause properly addressed, not just symptoms (
/diagnosing-bugs): the fix raisesmax-file-size/max-patch-sizeto measured values (512KiB/1MiB) and adds avalidation.scriptgate that enforces those budgets before upload/push, rather than just bumping numbers and hoping. ThememoryPhasemetadata (baseline/upload/push) correctly distinguishes "this run's upload directory" from "the full backing-branch overlay," so retention checks don't falsely fail when the push phase sees omitted historical files. - Strong regression coverage (
/tdd): the new 355-linemetrics_collector_workflow.test.cjssuite exercises the gate against the real embedded script extracted from the markdown (not a reimplementation), checks budget consistency between the frontmatter, the inline script, and the compiled lock file, and drives the realpush_repo_memory.cjsmain() against a real git repo with mocked network/push effects. This is exactly the kind of "test the actual contract, not a mock of it" approach that prevents silent drift between doc/script/lock. - One minor robustness gap: the regex-based extraction of the validation script from the markdown source (
metrics_collector_workflow.test.cjs:18) has no guard — a future doc edit that shifts indentation would produce an unhelpful null-deref instead of a clear failure message. Flagged inline.
Positive Highlights
- ✅ Clear separation between upload-time retention enforcement (30-day pruning, artifact count/size) and push-time integrity checks (byte-identical latest/daily pair), gated cleanly via
memoryPhase. - ✅ Correctly replaces
find -mtimecleanup (broken by checkout mtime resets) with UTC filename-based pruning — directly addresses the stated root cause. - ✅ Case-insensitive but case-rejecting filename validation (
/^\d{4}-\d{2}-\d{2}\.json$/imatch + explicit lowercase-only enforcement) is a nice defense against silently accepting2026-10-13.JSONas a different file than the canonical lowercase one. - ✅ Docs updated in lockstep with the new
memoryPhaseglobal and its semantics.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 198.7 AIC · ⌖ 14.7 AIC · ⊞ 10.3K
Comment /matt to run again
| const frontmatter = source.split("---")[1]; | ||
| const maxFileBytes = Number(frontmatter.match(/max-file-size: (\d+)/)[1]); | ||
| const maxPatchBytes = Number(frontmatter.match(/max-patch-size: (\d+)/)[1]); | ||
| const gate = source.match(/ validation:\n script: \|\n([\s\S]*?)\npre-agent-steps:/)[1].replace(/^ /gm, ""); |
There was a problem hiding this comment.
[/tdd] This contract test parses the embedded validation script straight out of metrics-collector.md via .match(...)[1] with no guard. If the markdown indentation or the pre-agent-steps: anchor ever shifts even slightly during a future edit, this throws a bare Cannot read properties of null (reading '1') instead of a message pointing at the real cause (doc/validator drift).
💡 Suggested improvement
Guard each extraction with an explicit assertion so failures are self-diagnosing:
const gateMatch = source.match(/ validation:\n script: \|\n([\s\S]*?)\npre-agent-steps:/);
if (!gateMatch) {
throw new Error("Could not locate validation.script block in metrics-collector.md - update this regex if the doc format changed");
}
const gate = gateMatch[1].replace(/^ /gm, "");Same applies to the maxFileBytes/maxPatchBytes/prune/example extractions a few lines above/below - a null-deref here is much harder to debug in CI than a clear "regex no longer matches" error, especially since this suite is explicitly a drift-detection contract test.
@copilot please address this.
Summary
Fix the Metrics Collector persistence failure from run 38502082778. Collection and freshness checks succeeded, but each complete 344-workflow snapshot was 447189 bytes against the implicit 102400-byte file cap; compacting still produces 299336 bytes, and the separate 128KiB patch cap was also insufficient.
memoryPhasemetadata (baseline,upload,push) so upload-only retention checks do not falsely require deleting historical backing-branch files.Validation
make fmt-cjs,make lint-cjs, and finalmake agent-report-progresspassed. Fullmake recompile: 344/344 workflows compiled; only this workflow's generated lock is published. Broad hooks and test markers ran in independent private snapshots/owned temp directories.--approve, warning suppression, dispatch, or rerun.concurrency.queueincompatibilities and dry-run-only removed AIC-output references/if: false; these are not claimed as passing checks.Security review
Exactly one retained read-only security reviewer reviewed authored source, reachable runtime changes, tests, and both normal/dry-run locks. All ten code/normal-lock publication safety questions passed; no confirmed vulnerabilities found. The new validator payload SHA-256 is
7f0b7ce63037fdf31ef02491a4fac4e08a50eca59d5366e6c10bff4d120ab6f2and matches all encoded/embedded instances. No secrets, recipients, action/container pins, MCP inventory, permissions, firewall, or threat-detection controls changed. Validator subprocess token exclusions, timeout, and mutation rejection remain intact. This review permits patch publication, not live execution, and does not clear dry-run/scanner warnings.Retention boundary
The current upload retains at most 30 dated daily snapshots plus latest and accounts for all eligible supplemental JSON. Repo-memory overlays uploaded files; omitted older snapshots remain on the backing branch and in Git history. This fix deliberately adds neither generic deletion support nor a privileged branch-cleanup operation. Raw run evidence and diagnostic dry-run locks remain private and are not committed.