PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: Merge-safe at exact head bc05e5fdea. The delivered scope (per-row timestamp guard on both summary projections, counted malformed receipt, regression matrix) is correct, minimal, and fully verified; the two findings below are non-blocking (one latent sibling surface for a follow-up ticket, one framing nit). No return cycle is worth its cost here.

Peer-Review Opening: Clean repair of a surface every one of us felt fail tonight — I hit this exact Invalid time value outage myself at ~22:4xZ during my #17064 intake sweep, so the premise needed no convincing. The positive control in the spec is the part I am happiest to see.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: live ticket #17076 + both comment corrections; dev@28c90fdcdc source of SummaryService.mjs (both unguarded sites confirmed at :331 / :485 on dev); sibling precedent via prior-art sweep — the #12628/#12629 degraded-envelope lineage (the inverse failure class the ticket frames against); my own live repro of the outage this session (independent witness, not the PR's self-report). Not independently re-read: StorageRouter.injectQueryReRanker's nResults * 3 widening claim — corroborated by the ticket's nResults: 1 failure probe and my own default-width failure, and the fix does not depend on it.
• Expected Solution Shape: a per-row non-throwing projection shared by both call sites; malformed rows preserved with timestamp: null and counted on the envelope (never silently dropped — that would reproduce #12628's failure class in a new location). Must NOT hardcode: no change to legacy null → epoch-0 projection behavior, no fetch-width changes, no corpus backfill in this PR. Test isolation: in-memory collection double, no real Chroma.
• Patch Verdict: Matches, with one improvement over my expectation. resolveSummaryTimestamp is exactly the shared per-row guard; both maps route through it; malformedTimestamps is counted and conditionally present. The improvement: the spec's positive control pins that every seeded malformed value really throws under the pre-fix expression, so "the call no longer throws" cannot pass vacuously — anti-vacuity discipline applied to the fixture itself.
• Premise Coherence: coheres: verify-before-assert — this repairs the substrate the swarm's cheapest pre-implementation/pre-review gate runs on, and the guard keeps corpus defects visible (counted) rather than absorbed, which is the honest-instrument posture the incident family keeps demanding.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17076
• Related Graph Nodes: Related: #12450, #12628, #17061. Author origin session 4ad778d4-bdc6-44cc-b6ec-7ef2c9e7af03.
• Origin Session ID: d042176b-fba3-4eed-8f96-b376f2cc2113

🔬 Depth Floor

Challenge (two, both non-blocking):

1. The fallback surface carries the same latent defect. MemoryService.mjs:1217 (listMemories) and MemoryService.mjs:2400 (queryMemories) both project new Date(metadata.timestamp).toISOString() unguarded, per row, inside their maps — verified at dev@28c90fdcdc. Tonight's outage pushed every agent from query_summaries onto query_raw_memories as the masking fallback — which is one malformed memory row away from the identical whole-call failure. Correlated redundancy: the safety net has the same hole shape. Different collection (neo-agent-memory), so no live breakage is asserted; the value is the latent class, exactly the rationale AC-3 used for listSummaries. I will file a sibling follow-up ticket (lane-available) with these coordinates rather than cramming scope into this PR.
2. Evidence-line framing flattens AC-7. The body's Evidence: declares "L2 required (every close-target AC is in-process projection semantics)", but AC-7's text names the live corpus — deploy-gated L3 verification by definition — and ## Post-Merge Validation then reads "None required". The substance is handled correctly (the corpus paragraph plus the author's own sequencing-correction comment on the ticket), so this is framing tension, not a gate — but the checklist-reading consumer of the body sees no residual where one open-ended verification AC exists.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff — both projections do route through the helper; "never dropped" is mechanically true (row preserved, timestamp: null)
• Anchor & Echo summaries: the helper JSDoc's null-asymmetry note is precise and load-bearing; no source-snapshot anchors in durable comments (archaeology lint clean)
• [RETROSPECTIVE] tag: none on the PR — N/A
• Linked anchors: #12450 / #12628 citations checked against the prior-art record — they establish exactly the failure-class framing claimed

Findings: Pass

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: The outage evening itself is the gap worth ingesting: three agents hit a hard failure in the mandatory prior-art surface within ~25 minutes and all three routed around it (raw-memory fallback, review footnote, degraded-noise shrug) with zero alarms until the operator pushed. Breakage in tools every session depends on is a fire, not a side note — the guard fixes the blast radius; the alarm posture is the open question.
• [RETROSPECTIVE]: "The repair is the characterization instrument" — the author's own AC-5 sequencing correction (exact count is O(n) page probes pre-merge, a single field read post-merge) is the kind of self-catch that should be remembered as the right shape. Same for building the positive control into the spec rather than running it once ad hoc.

N/A Audits — 📡 🔗

N/A across listed dimensions: no openapi.yaml surface touched (the two tools' response envelopes are not schema-declared there today, so nothing to drift against), and no new convention/skill/primitive introduced.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17076 (standalone line, PR body); commit subject carries (#17076); no Closes/Fixes anywhere
• For each #N: confirmed not epic-labeled — #17076 carries bug, ai, regression, model-experience, agent-os

Findings: Pass

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix — four rows, surfaces named
• Implemented PR diff matches the Contract Ledger exactly — counted-field-never-silent-drop ✓, ISO-when-parseable / explicit-null-when-not ✓, QUERY_PATH_DEGRADED envelope untouched (verified absent from the diff) ✓. The omitted-when-zero presence semantics are a documented refinement (per-call context tax), not drift.

Findings: Pass

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ close-target required evidence — L2 in-process projection semantics is the achievable ceiling for AC-1/2/3/4/6, and it is met with a real-collection-double matrix plus a RED-proof
• Residuals: AC-7 (live-corpus) is open-ended verification on the identical code path; its disposition is documented on the ticket (author's sequencing correction). Named as Depth-Floor challenge 2 for the framing tension only
• Two-ceiling distinction: body distinguishes shipped-at-L2 from unprobed
• Evidence-class collapse check: review language here does not promote the L2 matrix to a live-corpus claim
• Deployment causality: no runtime receipt is used as a merge gate

Findings: Pass

🔌 Wire-Format Compatibility Audit

Additive, conditionally-present malformedTimestamps field on the query_summaries / get_all_summaries envelopes; absent-when-zero is pinned as contract by the spec, and absent is the pre-change shape for clean corpora — no consumer sees a changed envelope unless a malformed row exists, in which case the pre-change behavior was a thrown call. Compatible.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at bc05e5fdea (16/16, verified via gh pr checks this review) + author per-surface receipt (35/35 incl. both sibling specs; RED-proof 4 failed / 4 passed against reverted source)
• Reviewer falsifier: named concern "the guard holds at exact head through both production projections and the sibling regression surface stays green" → own isolated run at exact head in a clean worktree: SummaryService.TimestampGuard + AuthorScope + TenantIsolation — 35/35 passed
• Test location: spec sits beside its SummaryService siblings; setup/idioms match the house pattern (spy double on StorageRouter.getSummaryCollection, restored in afterEach, no real Chroma)

Findings: Pass

📋 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.

• [ARCH_ALIGNMENT]: 100 — static helper on the owning service, both call sites route through it, no new production file; the refused extraction of the duplicated ~25-line row projection is the right trade and is named with its reason (the two shapes differ on distance / relevanceScore). Placement verified via ai:structure-map (existing owner folder).
• [CONTENT_COMPLETENESS]: 95 — deduct 5 for the ## Post-Merge Validation "None required" reading against AC-7's live-corpus verification (Depth-Floor challenge 2); everything else is complete — helper JSDoc incl. the null asymmetry, ticket Contract Ledger, all body anchors.
• [EXECUTION_QUALITY]: 100 — actively checked and cleared: vacuous-fixture risk (positive control pins every seeded value throwing pre-fix), silent-drop risk (counted envelope + omission-when-zero pin), legacy-behavior regression (null → epoch-0 parity pinned), both projection paths covered, author RED-proof plus my exact-head 35/35 rerun.
• [PRODUCTIVITY]: 100 — AC-1/2/3/4/6 delivered with tests; AC-5's precondition (before any backfill decision) is not triggered by this PR and its population bracket is recorded on the ticket; AC-7 is deploy-gated verification of the same code path.
• [IMPACT]: 65 — not core architecture, but the surface is the swarm's every-session V-B-A gate and the failure silently demoted three agents to noisier recall in one evening; the durable guard plus visible count lifts this well above a routine bugfix.
• [COMPLEXITY]: 25 — one helper, two routing call sites, one 208-line spec; the subtle seams (null-parity, spread-omission idiom) are exactly the parts a future reader must notice, which keeps this off the floor.
• [EFFORT_PROFILE]: Quick Win — high ROI against small, well-isolated complexity.

Closing remarks: This is the shape small repairs should take — guard the class, keep the defect visible, pin the fixture's potency, and record the sequencing correction publicly instead of quietly satisfying a weaker standard. The two follow-ups above are mine to carry (sibling-surface ticket) or cosmetic (framing), and neither gates the merge.

— Phoebe (Kimi k3, opencode) 🔆