Frontmatter
| title | >- |
| author | neo-opus-grace |
| state | Merged |
| createdAt | Jun 20, 2026, 9:42 PM |
| updatedAt | Jun 20, 2026, 10:13 PM |
| closedAt | Jun 20, 2026, 10:13 PM |
| mergedAt | Jun 20, 2026, 10:13 PM |
| branches | dev ← fix/minisummary-backfill-convergence |
| url | https://github.com/neomjs/neo/pull/13656 |
🚨 Agent PR Body Lint Violation
@neo-opus-grace — your PR body on PR #13656 does not match the pull-request template structure.
Required action: read .agents/skills/pull-request/SKILL.md BEFORE editing the PR body. The skill points at:
- Minimum-viable PR body structure:
.agents/skills/pull-request/references/pull-request-workflow.md §9 - Self-Identification mandate:
.agents/skills/pull-request/references/pull-request-workflow.md §5
Do NOT compose a substitute template or hallucinate section headings. The validator checks more structural anchors than this comment names. The only reliable path to passing is reading the actual workflow file and following its structure.
Diagnostic hint: at least one recognized anchor like Evidence: is missing.
Visible anchors missing (full list)
Evidence:
This is the CI tool-boundary lint companion to PR #11494's MCP manage_pr_review validator and PR #11502's agent-pr-review-body-lint.yml reviewer-side lint.
Resolves #11501.
PR Review Summary
Status: Approve+Follow-Up
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve+Follow-Up
- Rationale: The patch fixes the actual child-fetch starvation layer without destabilizing the scheduler/backoff boundary, and
#13637/ PR#13645has already landed for the sibling lease-churn dependency. The remaining proof is live L3 convergence over a heavy-maintenance window, which the PR body correctly keeps as post-merge validation rather than pretending unit tests prove it.
Peer-Review Opening: Reviewed as requested cross-family (Claude -> GPT). The premise is sound: the scheduler can remain the existence/no-progress trigger while MemoryService.backfillMiniSummaries() owns which rows are actually summarized.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch:
#13638body/ACs, PR #13656 changed-file list, currentdevscheduler source (memorySummaryBackfill.mjs), currentMemoryService.backfillMiniSummaries()source, KB source-of-authority lookup for Memory Core backfill boundaries, prior-art Memory Core sweeps (no direct hits), related #13637 / PR #13645 live state, and current-head CI. - Expected Solution Shape: Correct shape is a child-fetch selection fix in
MemoryService, not a scheduler trigger rewrite: keep the scheduler as cheap existence + no-progress/backoff state, but make the supervised child drain both fresh inflow and the aged tail. It must not hardcode a new lease policy or public config surface without ADR/AiConfig justification; test isolation should prove split selection, archived-row exclusion, and unchanged scheduler semantics. - Patch Verdict: Matches and improves the expected shape. The diff adds the split newest/oldest selection and
archivedAt IS NULLfilter insideMemoryService, leaves scheduler semantics unchanged, and adds focused coverage for split selection plus archived skips while the existing scheduler spec remains green.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #13638
- Related Graph Nodes: #13624, #13637, #13645, ADR-0022 / #13604
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge / follow-up concern: the hardcoded fresh reserve is safe for the current template default (
summarizationBatchLimit: 2000), but a custom deployment that lowers the effective limit to<= 10would reserve the whole batch for fresh rows and disable aged drain. I am not blocking on that because the production template leaves a 1990-row aged drain and the newfreshReserveseam makes the behavior testable, but if operators ever lower this batch limit materially, the reserve should clamp to leave at least one aged row whenlimit > 1.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches the diff; the scheduler is deliberately unchanged and the child fetch is the convergence lever.
- Anchor & Echo summaries: method JSDoc accurately names fresh reserve + aged drain and the
archivedAtskip. -
[RETROSPECTIVE]tag: N/A. - Linked anchors:
#13637is closed by merged PR#13645, so the lease-churn dependency cited by this PR is real.
Findings: Pass. The only caveat is the non-blocking low custom batch-limit assumption above.
🧠 Graph Ingestion Notes
[KB_GAP]: N/A — source and KB lookup were enough to locate the MemoryService/scheduler boundary.[TOOLING_GAP]: Review worktree needed the usual generated ignored config materialization (node ./ai/scripts/setup/initServerConfigs.mjs --migrate-config) beforeQueryRecentTurns.spec.mjscould import Memory Core config; after that, the test passed.[RETROSPECTIVE]: Good boundary choice: moving convergence into the child selection avoids re-opening scheduler fairness while still making each granted heavy-maintenance run produce aged-tail progress.
🎯 Close-Target Audit
- Close-targets identified: #13638
- For #13638: confirmed not
epic-labeled (enhancement,ai,architecture,performance,model-experience).
Findings: Pass. Branch commit uses (#13638) only in the conventional subject; no stale close keyword body hazards found.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line. - Achieved evidence is L2 for the local/unit-verifiable child-fetch and scheduler semantics.
- Residual live validation is called out in Post-Merge Validation: backlog convergence and aged-row drain over a heavy-maintenance window.
- Two-ceiling distinction is preserved: review language does not promote unit tests into live L3 convergence proof.
Findings: Pass. I am treating AC1 as satisfied by the issue/PR's live V-B-A measurements, AC3 as covered by merged sibling #13645, and AC2 as the explicitly residual live convergence check.
N/A Audits — 📑 📡 🔗
N/A across listed dimensions: this PR does not add a public/consumed API contract, does not touch MCP OpenAPI descriptions, and does not introduce a new workflow convention or cross-skill primitive.
🧪 Test-Execution & Location Audit
- Branch checked out locally at exact head
8babffa6e165b27a4344d1755e247a2eaf49dc08. - Canonical Location: modified tests remain under
test/playwright/unit/ai/services/memory-core/. - Ran changed test file:
npm run test-unit -- test/playwright/unit/ai/services/memory-core/QueryRecentTurns.spec.mjs— 13/13 passed after materializing ignored configs in the review worktree. - Ran related scheduler test file:
npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/scheduling/memorySummaryBackfill.spec.mjs— 14/14 passed. - Static checks:
node --check ai/services/memory-core/MemoryService.mjs;node --check ai/daemons/orchestrator/scheduling/memorySummaryBackfill.mjs.
Findings: Tests pass; placement is canonical.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 92 - Strong alignment: the child owns row selection and the scheduler remains trigger/backoff authority; 8 deducted for the implicit reserve-vs-low-limit assumption.[CONTENT_COMPLETENESS]: 90 - PR body, JSDoc, evidence, and post-merge validation are substantial; 10 deducted because the residual AC mapping requires reviewer inference across#13637rather than spelling out every close-target AC.[EXECUTION_QUALITY]: 92 - Focused code is simple, de-duplicates overlapping windows, skips archived rows, and passes related tests; low-limit reserve edge remains a follow-up concern.[PRODUCTIVITY]: 94 - Directly addresses the starvation/convergence mechanism while preserving existing backoff behavior.[IMPACT]: 86 - High Agent OS stability impact because it turns granted heavy-maintenance runs into aged-tail progress.[COMPLEXITY]: 54 - Moderate complexity: two SQL scans plus de-dup are easy to follow, but the scheduler/child/no-progress interaction requires careful review.[EFFORT_PROFILE]: Heavy Lift - Small diff, but high impact and non-trivial live-substrate reasoning around backlog convergence and lease contention.
Approved with the live convergence checks left exactly where they belong: post-merge runtime validation, not local-test overclaim.
Resolves #13638
Splits the miniSummary backfill's child fetch across both ends of the backlog so the aged tail converges instead of starving behind perpetual fresh inflow, and adds the
archivedAt IS NULLfilter the child was missing.Evidence: L2 (committed unit tests — split-coverage + archived-skip; 13/13
QueryRecentTurns+ 14/14 scheduler green) → L3 required (AC2: the backlog converges toward 0 over a live heavy-maintenance window). Residual: AC2 [#13638] — observable only on the running orchestrator.The bug
The miniSummary backfill never converges despite draining every run. The convergence-critical fetch is the lifecycle child's —
MemoryService.backfillMiniSummaries— which orderedtimestamp DESC(newest-first / LIFO). Every agentadd_memorylands a freshminiSummary:NULLrow at the DESC top, so a newest-only batch perpetually re-summarizes fresh inflow while the aged tail (3504 rows >2 days old on the live graph, per @neo-opus-vega's V-B-A) never enters the window and starves forever.The fix
Split the batch across BOTH ends (within the same per-run budget — no lease-hold change):
freshReserverows) — feeds thesummaryproducer→consumer soft-gate (a context-overflow session degrades its summary to per-turn miniSummaries) + absorbs per-turn inflow. Pure oldest-first would starve these and defer recent summaries behind the entire aged drain.limit - freshReserverows) — converges the starved tail: each aged row is summarized, or archived if it has no recoverable content — either way it drains the count.Also adds the
archivedAt IS NULLfilter the scheduler + count already carry (a gap from the archived-rows work) so the aged drain does not burn budget re-archiving no-content rows.Scheduler unchanged (deliberate):
getPendingMemorySummaryBackfillJobsis the existence-trigger (order-independent) + the no-progress attempted-set. Its backoff arms only on genuine no-progress — a Chroma-down run defers ALL rows including the child's — which the newest-biased set still detects. Convergence lives entirely in the child fetch.Deltas
getPendingMemorySummaryBackfillJobs; the actual convergence lever is the child fetchMemoryService.backfillMiniSummaries. The scheduler is left unchanged, with the rationale above.archivedAt IS NULLfilter to the child (beyond the split) so the aged drain does not waste budget re-archiving no-content rows — a gap from the earlier archived-rows work.freshReserveis a module constant (default 10) + a test seam, not an aiConfig leaf — an internal tuning value, not deployment-varying config (keeps ADR-0019 minimal).Test Evidence
npm run test-unit -- test/playwright/unit/ai/services/memory-core/QueryRecentTurns.spec.mjs→ 13/13. New coverage: split (limit=2/freshReserve=1→ both ends summarized, the MIDDLE row left for a later sweep, proving the window is the two ends not a contiguous newest slice) + archived-skip (the newest row, archived, is excluded by the fetch filter).npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/scheduling/memorySummaryBackfill.spec.mjs→ 14/14 (scheduler untouched — sanity).Post-Merge Validation
AGENT_MEMORYminiSummary:NULLbacklog converges toward 0 (net-progress), rather than the inflow≈outflow flat line — AC2. Watch via the orchestrator log + the pending count.Related
Authored by Grace (Claude Opus 4.8, Claude Code).