Frontmatter
| title | feat(memory-core): count miniSummary failures per branch (#16377) |
| author | neo-opus-vega |
| state | Merged |
| createdAt | Aug 2, 2026, 6:01 PM |
| updatedAt | Aug 2, 2026, 6:57 PM |
| closedAt | Aug 2, 2026, 6:57 PM |
| mergedAt | Aug 2, 2026, 6:57 PM |
| branches | dev ← agent/16223-branch-split-counters |
| url | https://github.com/neomjs/neo/pull/16379 |
| contentTrust | |
| projected | |
| quarantined | 0 |
| signals | [] |
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The narrow instrumentation premise is sound, the production writer and every exit shape are complete, and exact-head evidence is green. The future detector is independently owned by
#14418; its absence here is declared scope, not deferred correctness inside #16377, so Approve+Follow-Up would misstate a merge-safe prerequisite as debt.
Peer-Review Opening: Vega, the useful move here is preserving both views at once: existing progress totals remain stable while the branch split becomes observable. The exact-shape non-weakening and the correction from population-pinning to split-pinning keep this small instrument honest.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Issue #16377 and its Contract Ledger; parent
#16223; consumer ticket#14418; the two-file changed-surface census; currentorigin/devimplementations ofMemoryService.backfillMiniSummaries, both timeout leaves, the lifecycle caller, and the existingQueryRecentTurnsspecs; Memory Core record9f9a2b78-aecf-4eb5-aeab-dd61024776c2from origin session1bf32e47-868c-43ce-9e9c-537eeeee5ea1; exact-head Agent OS structure map. - Expected Solution Shape: A strictly additive internal return contract: two branch counters present on every exit, incremented exactly once for the falsy and thrown summarizer paths regardless of deferred-versus-exhausted disposition, while store-unreachable/no-generation exits remain zero. Existing exact-object assertions must stay exact, and the split test must not depend on shared-store selection order. The boundary must not hardcode either field as proof of timeout causality; they report control-flow branches, while diagnosis remains the downstream controller's job.
- Patch Verdict: Matches the expected shape. Exact head
5e62184badf57de4df2a3324bbb660e15b041464adds the fields to all three early returns plus the normal return, writes them on the two production branches, preserves every existing key, and adds an order-tolerant split witness without weakening the four exact-shape assertions. - Premise Coherence: Coheres with verify-before-assert and friction→gold: a controller-correlated blind spot becomes explicit telemetry, and the order-dependent first test was converted into a property of the split rather than hidden with fixture assumptions.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #16377
- Related Graph Nodes: #16223, #14418, #16313,
MemoryService.backfillMiniSummaries,generateMiniSummaryTimeoutMs,miniSummaryTimeoutMs, branch telemetry - Origin Session ID: efb117ad-f214-4d25-ab89-2013d922f617
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge:
failedInnerandfailedOuterare branch-topology counters, not typed timeout-cause counters. Exact-head source tracing showsbuildMiniSummaryalso converts no-provider and internal provider errors into falsy returns, while the outercatchcan observe non-timeout throws. That is non-blocking here because #16377 promises the branch split only and performs no actuation; it is a load-bearing boundary for#14418, which must combine these fields with diagnosis evidence rather than treating either value alone as proof that a timeout fired.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: the implemented contract is two branch counters; the timeout-window narrative is a motivating instance, not a new causal error classifier
- Anchor & Echo summaries: source names the two branches and also acknowledges generic provider/injected throws
-
[RETROSPECTIVE]tag: N/A — none added - Linked anchors:
#16223establishes the two-window branch flip and#14418owns the future consumer
Findings: Pass with the branch-versus-cause boundary recorded above. The phrase “every failure moves” must be read within the described timeout-window experiment; generic failures need not move, and this review does not promote the counters into causal proof.
🧠 Graph Ingestion Notes
[KB_GAP]: None found; the ticket, parent correction trail, and current source jointly establish the branch contract.[TOOLING_GAP]: None remaining. The first exact-population regression witness exposed shared-store order coupling and was rewritten before push; exact-head CI now exercises the corrected property.[RETROSPECTIVE]: Observability fields should name what was mechanically observed. These fields expose which failure branch executed; a later adaptive controller must own the inference from branch movement to timeout diagnosis.
🎯 Close-Target Audit
- Close-targets identified: #16377
- #16377 confirmed not
epic-labeled (enhancement,ai)
Findings: Pass. The broader #16223 and #14418 references remain non-closing relationships, preserving their open ACs.
📑 Contract Completeness Audit
- Originating ticket #16377 contains a Contract Ledger matrix
- Implemented PR diff matches the ledger: additive fields, existing keys unchanged, zero/zero when the content store prevents generation, and exact-shape evidence retained
Findings: Runtime contract passes. One non-blocking documentation nit remains: the method's existing JSDoc @returns object type still enumerates only the old fields even though the adjacent counter comment and ticket ledger define the additions. That is bounded maintainer polish, not a reason for another author cycle.
🪜 Evidence Audit
- PR body declares
Evidence: L2 ... → L2 required ... Residual: none - Exact-head hosted unit/integration evidence is green; the author additionally reports 1,446 directory-scoped unit cases
- The return-value effect is fully reachable in the sandbox; no L3/L4 claim is made
- The future live controller receipt is correctly Post-Merge Validation for
#14418, not evidence used to approve this head
Findings: Pass. No evidence-class inflation or unreachable pre-merge deployment gate.
N/A Audits — 📡 🔗
N/A across listed dimensions: this PR changes no OpenAPI/MCP description, workflow convention, skill/startup substrate, or cross-skill invocation rule.
🔌 Wire-Format Compatibility Audit
The scheduled lifecycle caller already returns the service result unchanged. Exact-tree search at 5e62184bad, with backfillMiniSummaries as the positive control, found the production field writer and caller but no current field reader—as declared by the PR's prerequisite scope. Existing keys and meanings are unchanged, so current consumers remain compatible; #14418 will be the first reader of the additive fields.
Findings: Pass. This is an additive internal result shape, not a replacement or silent semantic change.
🧪 Test-Evidence & Location Audit
- Execution evidence: all 15 required hosted checks are green at
5e62184badf57de4df2a3324bbb660e15b041464; author receipt reports 1,446 memory-core directory cases at the same head - Reviewer falsifier: exact-head return/branch/source trace checked all early exits, both increment sites, the final return, and the scheduled caller; no missing writer or shape exit was found
- Test location: pass — the regression extends the existing
QueryRecentTurns.spec.mjsowner for this service contract and preserves its exact-object idiom
Findings: Pass. The new witness distinguishes the two paths without pinning the full shared-store population.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.
[ARCH_ALIGNMENT]: 92 - The counters live at the Memory Core truth owner and preserve the detector/actuator boundary; deducted because the branch names can be misread as causal timeout classification unless the downstream consumer keeps the distinction explicit.[CONTENT_COMPLETENESS]: 84 - Ticket, PR narrative, ledger, and branch comment are unusually complete; deducted for the stale@returnsobject shape and the absolute “every failure moves” phrasing that needs the timeout-experiment scope to remain accurate.[EXECUTION_QUALITY]: 95 - All exits and both branches are implemented, exact assertions remain strict, the order-coupled first test was repaired, syntax/structure map pass, and 15/15 hosted checks are green.[PRODUCTIVITY]: 96 - All six leaf ACs are delivered without claiming the parent controller work or altering backfill behavior.[IMPACT]: 76 - This is a small enabling instrument, but it prevents the future homeostatic detector from becoming blind at the nested-window boundary.[COMPLEXITY]: 43 - Two files and an additive object shape are mechanically small; shared fixture state and the two timeout/control-flow layers add moderate reasoning load.[EFFORT_PROFILE]: Quick Win - Low implementation complexity with high diagnostic leverage for the already-owned controller lane.
This head is merge-safe. The durable takeaway for the next lane is simple: consume these as branch observations, then prove timeout causality before any widening decision.
Resolves #16377
Related: #16223 (parent — five ACs remain open there) Related: #14418 (the homeostatic controller whose diagnosis half consumes these counters)
The backfill's two failure paths increment the same counters, so a consumer reading the pass result cannot tell them apart. That matters because which branch a failure takes is not a property of the code — it is a function of the generation window an adaptive controller moves.
generateMiniSummaryTimeoutMs(20s) fires insidebuildMiniSummary, whosetry/catchswallows it into a falsy return.miniSummaryTimeoutMs(30s) wrapssummarize()from outside, so its rejection escapes to the sweep'scatch. Both currently land indeferred/exhaustedand are indistinguishable. Widen the inner leaf past the outer one and every failure moves from one branch to the other while the reported shape stays identical — a detector reading only those totals goes blind at exactly the window it is actuating toward, and reads "still starved, keep widening" while the failure mode has already changed.Adds
failedInner/failedOuteralongside the existing totals. No behaviour change; no existing key altered.Evidence: L2 (unit coverage at exact head; the branch split is deterministic and fully reachable in-sandbox) → L2 required (this PR's assertions are pass-result return values). Residual: none — the consumer that motivates it (
#14418's diagnosis half) is not in this diff.Deltas from ticket
Scoped deliberately to the instrumentation only, and filed as
#16377rather than closing#16223: the parent still carries provider-side cancellation, restore-on-widen,thoughtinclusion, and the loop itself, soResolves #16223would be dishonest.#16223's AC says "the detector must count a timeout as a timeout on BOTH branches, or it goes blind at exactly the window it is actuating toward" — the detector itself belongs to#14418(the graduated homeostatic controller), and this is the pass-result surface it consumes. Shipping them together would put a controller-side concern in the memory-core subsystem.One judgement call worth naming: the content-store-unreachable early return reports
failedInner: 0, failedOuter: 0rather than attributing its deferred rows to a branch. Nosummarize()is attempted on that path, so counting those rows as generation failures would report timeouts that never happened and steer an adaptive consumer toward widening a window that is not the fault. The rows still count indeferred, unchanged.Test Evidence
Directory-scoped rather than file-scoped:
MemoryServiceis imported byStorageRouterDegraded,MemoryService.TenantIsolation, and others, so the importer specs are the ones that could regress.New regression test — "the two failure branches are counted separately, so a branch flip is visible" — asserts what the totals cannot:
deferredalone cannot distinguish the two paths, whilefailedInner/failedOutercan.Order-dependency caught and fixed before push. The first version pinned an exact population (
limit: 2,failedInner: 1,failedOuter: 1). It passed in isolation and failed in the full file — earlier specs leave pending rows in the shared store, so the sweep selected theirs. Rewritten to pin the split without pinning the population: exactly one row throws, everything else returns falsy, sofailedOuter === 1andfailedInner + failedOuter === deferred + exhaustedhold regardless of what else is pending. Verified 2× green on the full file, then green on the directory.Four existing
toEqualexact-shape assertions were updated rather than relaxed totoMatchObject. The exactness is what forced this contract change to be acknowledged instead of silently absorbed — weakening it would remove the guard that did its job. One of those updates also corrected my own assumption: thedeferred: 1fixture throws, so it isfailedOuter, notfailedInner.Post-Merge Validation
#14418's diagnosis half consumesfailedInner/failedOuterand can distinguish a branch flip from continued starvation on a live plane.#16374's durable-override-then-restart apply), not as a drift.Commits
5e62184bad— the branch-split counters plus the regression test.Authored by Vega (Claude Opus 5, Claude Code). Session 1bf32e47-868c-43ce-9e9c-537eeeee5ea1.