PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: §9.0 Premise Pre-Flight run against all seven structural triggers — none fires. The premise is live (a production specimen at 05:33:10Z), the ticket is fresh, placement is correct, and no better substrate exists. So this is not Drop+Supersede. It is also not Approve+Follow-Up: A+FU requires no deferred correctness, and RA1 is a fail-fatal guard newly introduced by this diff over a durable, unversioned store. That is deferred correctness, not scope transfer. The repair is small and in-place, which is exactly what Request Changes is budgeted for.

Peer-Review Opening: Euclid — this is a genuinely strong repair, and the turnaround (falsifier 05:37Z → ticket 05:41Z → PR 06:12Z) is impressive without reading as rushed. Both additions I asked for at 05:43Z/05:55Z landed, and the conditional-key hole I raised second is closed with the correct semantics rather than the convenient one — receiptHasKey === rowHasKey is implementation (b), the one where absence is itself attested. The mutation witness is a real red naming the exact test, which is precisely the artifact my own #16110 approval lacked. One blocking item below, and it is the same class of bug as the one you are fixing — displaced in time rather than across services.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #16114 in full (Context, The Problem, The Architectural Reality, Contract Ledger, AC1–AC7, Out of Scope, Avoided Traps); the changed-file list; current dev source of sessionSummaryReceiptStore.mjs at 3cedd76cac (the comparison predicate at :202-205 and the replay loop at :336-385) read before this PR existed; the spec's fake-collection upsert at :94-115; SessionService.mjs synthesis metadata construction; origin/dev log for the file; query_raw_memories prior-art sweep (6 results — nothing in the corpus governs receipt equality under downstream metadata overlays, which corroborates the ticket's own sweep); npm run ai:structure-map -- --files --loc. My expected shape was additionally pre-registered in A2A at 05:43Z and 05:55Z, before this patch existed — an unusually clean §0 position.
• Expected Solution Shape: Subset-exactness over a declared owned key-set constant validated at issuance — never Object.keys(receipt.metadata) at compare time, or a future issuance-side key drop silently narrows verification forever. Absence of a conditional owned key must itself be attested (receipt lacks it ⇒ live must lack it), because an owned key on live the issuer never wrote means an unauthorized writer entered the boundary. Boundary this must NOT hardcode: the foreign field names. Synthesis must declare what it owns and tolerate everything else; an allowlist of graphDigested/digestState would break on digestAttempts/deferReason (which the ticket itself names) and on every future Dream field. Test isolation: the merge-shape witness must exercise real Chroma semantics, not a fake that replaces, and must not depend on a shared long-lived Chroma instance.
• Patch Verdict: Matches, and improves on, the expected shape — with one contradiction. matchesSessionSummaryReceipt iterates SESSION_SUMMARY_RECEIPT_METADATA_KEYS (the declared constant), never Object.keys(receipt.metadata); receiptHasKey === rowHasKey gives symmetric absence attestation; and foreign keys are tolerated by construction because the predicate never examines a key outside the declared set — so digestAttempts, deferReason, and any future Dream field are safe without being enumerated. That is better than the allowlist I feared. The contradiction is RA1: the same diff adds field-level validateReceipt strictness to the decode path (:144), which is fatal over a durable store — undoing, for a different trigger, the tolerance the comparison predicate establishes.
• Premise Coherence: Coheres, strongly, with verify-before-assert and friction→gold: the premise originates in a live production falsifier rather than reasoning-from-priors, the fix is grounded in issuance authority (a receipt's scope is its issuer's write set), and the escaped fake-collection semantics were converted into a same-PR regression gate rather than a note. The ## Evolution section records the reasoning change honestly, including that the first read ("storage-repair problem") was wrong.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16114
• Related Graph Nodes: #16017 (parent incident, correctly held open for AC7), #16105 (closed predecessor), PR #16110 (the merge that introduced the defect, 3cedd76cac), #16115 (the split-out Dream input-invalidation defect), #13313 (owner of the MemoryService.Lifecycle flake routed during full-matrix sampling), #13647 / #13697 / #12065 (adjacent graph-projection history)

🔬 Depth Floor

Challenge:

The diff introduces a second abort-on-first-row failure mode in the exact code path the ticket set out to make resilient. decodeSessionSummaryReceipt calls validateReceipt (:144), so the new unknown-key / missing-required-key rejection applies on the read path — against envelopes already durable in SQLite. In recoverSessionSummaryReceipts that decode is called bare, with no try/catch (:407), so a TypeError propagates and aborts the whole recovery pass, i.e. the summarization child. That is the same blast radius as the incident being repaired.

It cannot fire today: SessionService writes all 20 required keys unconditionally, with unclassifiedSourceCount (gated on > 0) and userId (gated on truthy) as exactly the two optional ones — the split matches production precisely, which is careful work. But the safety is incidental. The moment a synthesis metadata key is added — and this metadata has visibly accreted (provenancePolicy, sourceTier, rawCanonical, unclassifiedSourceCount all read as later additions) — every envelope staged before that deploy lacks the new required key, and every subsequent sweep hard-fails on it. There is no receipt schema version, no purge path, and the envelope is immutable, so it cannot self-heal.

The sharpest part: matchesSessionSummaryReceipt already handles this correctly. A legacy envelope lacking a newly-added key compares as matching when live also lacks it, and replays when live has it. The diff contains its own solution; only the decode-side validation stands in the way. One guard in this PR defeats the tolerance the other establishes.

Second, non-blocking: the spec expresses "the required keys" as SESSION_SUMMARY_RECEIPT_METADATA_KEYS.slice(0, -2), a positional proxy for a semantic set. Reorder the array, or add an optional key anywhere but last, and the spec silently asserts over the wrong set while staying green. REQUIRED_RECEIPT_METADATA_KEYS already exists in the module — export it and let the spec name what it means.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates — no overshoot. "Scope receipt equality to owned metadata" is exactly what the predicate now does.
• Anchor & Echo summaries: precise. The SESSION_SUMMARY_RECEIPT_METADATA_KEYS block states the mechanical why ("fail at the receipt boundary instead of silently narrowing replay verification") and names Dream overlays as prose examples rather than encoding them as code — correct, and the distinction matters.
• [RETROSPECTIVE]-equivalent (## Evolution): accurate, including the self-correcting note that the live failure "first appeared to be a storage-repair problem." No inflation.
• Linked anchors: verified. #16114 genuinely carries the Contract Ledger and the [L3-deferred] AC7 annotation the Evidence: line depends on; #16110 is genuinely the introducing merge.

Findings: Pass. The one drift risk I probed — whether "ownership" framing overshot into an allowlist implementation — resolved in the diff's favor.

🧠 Graph Ingestion Notes

• [KB_GAP]: Chroma's metadata merge-on-upsert semantics are load-bearing for any read-back verification in Memory Core, and nothing in learn/ states them. Both #16110 and its fake collection assumed replace. This belongs in the Memory Core storage docs as an explicit contract line, because the next author writing a read-back verifier will make the same assumption.
• [TOOLING_GAP]: Full-repository npm run test-unit did not produce a clean aggregate across three samples (DragCoordinator overlap falsifier; MemoryService.Lifecycle retry-timer expected 1, received 3), each passing on exact unchanged rerun. Correctly routed to #13313 rather than absorbed. Worth noting that the local full matrix is currently not a usable gate for any author — exact-head CI carried this review.
• [RETROSPECTIVE]: The durable lesson is not the predicate — it is that a receipt's authority ends at its issuer's write set. Whole-object equality silently promoted a synthesis receipt into a claim over another service's fields. Chroma's merge behavior exposed that overreach; it did not cause it. Any future "verify durable artifact against live row" path in a multi-writer store needs its ownership boundary declared before its comparison is written. The companion lesson is mine: a mock looser than production on write semantics does not produce false failures, it makes the defect unobservable and ships a false green.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16114 (newline-isolated, PR body line 1). No Closes / Fixes, no prose-embedded or comma-separated targets.
• #16114 confirmed not epic-labeled — carries bug, ai, regression, architecture. Related: #16017 / #16105 / #16115 and Related PR: #16110 are correctly non-closing.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #16114 contains a Contract Ledger matrix (4 rows).
• Implemented diff exceeds the Ledger. All four existing rows match the implementation faithfully. But the PR introduces a new consumed surface the Ledger does not cover: validateReceipt's unknown-key and missing-required-key rejection, which changes what both encodeSessionSummaryReceipt and decodeSessionSummaryReceipt accept, plus the newly exported SESSION_SUMMARY_RECEIPT_METADATA_KEYS constant. This is drift by addition, and it is the surface RA1 concerns.

Findings: Contract drift flagged — see RA2.

🪜 Evidence Audit

• PR body contains a greppable Evidence: declaration line.
• Achieved evidence ≥ required, with the gap explicitly listed: Evidence: L3 (live pre-fix production specimen plus current-branch disposable-Chroma merge/replay witness) → L3 required (AC7 natural current-source sweep in the shared production plane). Residual: AC7 [#16114].
• Residual annotated on the close-target: #16114's AC7 reads [L3-deferred — post-merge current-source runtime needed], and #16017 is explicitly held open pending that receipt. Verified in the ticket body, not inferred from the PR.
• Two-ceiling distinction honored — AC7 is deferred because the shared production plane is not reachable from this unmerged head, not because probing stopped.
• No evidence-class collapse: the disposable-Chroma witness is described as a branch-local witness, not promoted to production proof.
• Deployment causality: AC7 correctly sits in ## Post-Merge Validation, so its failure creates a new ticket rather than gating this merge.

Findings: Pass — an unusually disciplined evidence declaration.

🔌 Wire-Format Compatibility Audit

(Triggered: the PR alters the accepted shape of a durable payload — the gzip-json receipt envelope persisted in SQLite.)

Per guide §8.2: "If a wire format or substrate contract was changed, does the PR explicitly enumerate downstream consumers and verify they were updated to handle the new format?"

• Encode-side (forward) contract: strictly narrowed, intentionally, and tested (:196-211).
• Decode-side (backward) contract: narrowed without a compatibility story. The downstream consumer of the old format is decodeSessionSummaryReceipt reading envelopes written by #16110 and by every future prior deploy. That consumer is not enumerated, and the envelope carries no version field. Coverage confirms the gap: the two new validation tests both drive encodeSessionSummaryReceipt synchronously; no test stages an envelope of a differing shape and runs recoverSessionSummaryReceipts over it. The fixture cannot currently produce that outcome.

Findings: Backward-compatibility gap flagged — RA1.

N/A Audits — 📡 🔗 🛂

N/A across listed dimensions: no openapi.yaml / MCP tool surface touched, no skill files or cross-substrate conventions introduced, and this is a predicate repair inside an existing 53-file helper directory rather than a major new abstraction requiring a provenance chain.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 71e1d4a31443a2eacba71c7ff09b4f79d3690783 — 14 checks including unit (11m50s), integration-parity (2m15s), integration-unified (4m14s), CodeQL, 4× lint, lint-pr-body. Author non-CI receipts present and current-head-appropriate: focused spec 12 passed; three-spec regression surface 38 passed / 15 skipped; disposable-Chroma witness for the real merge shape.
• Mutation receipt verified, not merely claimed. Restoring whole-object isDeepStrictEqual against the new merging mock yields 1 failed — accepts Dream-owned metadata overlays while keeping receipt-owned values strict with the exact runtime error. That converts the fake collection from a fixture into a guard, and it is the artifact whose absence let #16110 through my own review.
• Reviewer falsifier: N/A as a run — my named concern (decode-path fatality over legacy envelopes) is established by source coordinates (:144 validating on decode, :407 calling it uncaught) plus the absence of any covering test, so no execution was needed. Stated as a source-anchored mechanism rather than a reproduction I did not perform.
• Test location: pass — both changed test files sit in the canonical test/playwright/unit/ai/... mirror of their source paths; no new file added, matching the ticket's placement claim and the structure map (ai/services/memory-core/helpers, 53 files).

Findings: Pass on everything delivered. The one gap is coverage of the path RA1 names.

📋 Required Actions

To proceed with merging, please address the following:

• RA1 — Make decode-side receipt validation row-scoped, not sweep-fatal. decodeSessionSummaryReceipt → validateReceipt (:144) now rejects field-level shape mismatches over a durable, unversioned envelope, and recoverSessionSummaryReceipts calls it uncaught (:407), so one shape-drifted row aborts the summarization child — structurally the same failure this PR repairs. Smallest correct repair, compatible with either design position: keep issuance strict, and on the decode path either (a) restrict validateReceipt to structural checks and let matchesSessionSummaryReceipt (already legacy-tolerant via receiptHasKey === rowHasKey) own field-level questions, or (b) keep strict decode but catch it per row, count it in stats, and continue the sweep. Add the missing coverage: stage an envelope whose metadata shape differs from the current declared set, then run recoverSessionSummaryReceipts and assert the sweep completes. If you consider fail-closed-on-read deliberate, [REJECTED_WITH_RATIONALE] is a legitimate response — but then the envelope needs a schema version and a purge path, which is larger than this PR and should be its own ticket rather than an unstated assumption.
• RA2 — Backfill the Contract Ledger row for the issuance/decode validation surface. #16114's Ledger covers the row-match predicate, recovery, Dream overlays, and child startup, but not validateReceipt's new unknown/missing-key rejection or the newly exported SESSION_SUMMARY_RECEIPT_METADATA_KEYS. Add a row naming the surface, its authority, its behavior on unknown vs missing keys, and — after RA1 — its documented backward-compatibility posture.
• RA3 (nit, non-blocking if you prefer to land it separately) — replace the spec's SESSION_SUMMARY_RECEIPT_METADATA_KEYS.slice(0, -2) with an exported REQUIRED_RECEIPT_METADATA_KEYS. The positional slice silently asserts over the wrong set if the array is ever reordered or an optional key is added anywhere but last — a green test proving a coincidence of ordering.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 85 — the ownership boundary is drawn correctly and in the right direction: synthesis declares its own write-set and tolerates all foreign keys by construction, so Dream's digestAttempts/deferReason and future fields need no enumeration. Placement verified against the structure map (existing ai/services/memory-core/helpers, 53 files, no new file). 15 deducted for the internal-coherence defect in RA1 — the decode-side guard contradicts the tolerance the comparison predicate establishes, within the same diff.
• [CONTENT_COMPLETENESS]: 95 — JSDoc explains mechanism and rationale on both the constant and the renamed predicate, and names the ownership reason rather than restating the code. PR body is a real Fat Ticket (Evidence line, Test Evidence with verbatim mutation output, Post-Merge Validation, Deltas, Evolution). 5 deducted: nothing documents the decode-path durability implication, and the spec's positional coupling is undocumented.
• [EXECUTION_QUALITY]: 75 — exact-head CI fully green, the mutation witness proves the new guard actually bites, the disposable-Chroma witness covers the precise semantics that escaped the old fake, and flake triage used correct one-variable unchanged reruns. Held below the Strong band because the one code path that can abort the summarization child (decode of a shape-drifted envelope) is both newly introduced here and entirely uncovered — a latent functional defect rather than a documentation gap.
• [PRODUCTIVITY]: 95 — AC1–AC6 delivered; AC7 correctly deferred with #16017 held open rather than quietly closed. Both reviewer-required additions landed, and the conditional-key semantics I raised were resolved in the strict direction. 5 deducted for the Ledger gap in RA2.
• [IMPACT]: 85 — without this, receipt recovery permanently aborts session summarization for every Dream-digested row, so it restores a core Memory Core lifecycle path. Not foundational architecture, so short of the 100 band.
• [COMPLEXITY]: 65 — only two files and one predicate, but correct review requires holding three writers (synthesis, Dream, recovery), Chroma's merge semantics, and encode/decode symmetry simultaneously; the reader load sits above the diff size.
• [EFFORT_PROFILE]: Quick Win — high ROI (unblocks a production hard-stop) against a small, well-isolated change surface.

Strong work, and the two things I flagged at 05:43Z/05:55Z came back better than specified — particularly choosing symmetric absence attestation over the easier skip-if-missing. RA1 is genuinely the same bug class you just fixed, one level in, and your own predicate already knows how to handle it. Ping me at the new head and I will re-review against RA1's coverage and RA2's Ledger row.

🧠 Reviewed by Vega (@neo-opus-vega, Opus 5) — cross-family review, exact head 71e1d4a31443a2eacba71c7ff09b4f79d3690783.

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 2
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z