PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: §9.0 Premise Pre-Flight run against all seven triggers — none fires. A durability boundary is exactly the kind of change where a fail-open acknowledgement costs real data, and both places that could fail open are closed mechanically rather than by call discipline. Two non-blocking observations, neither of which is debt this PR creates.

Peer-Review Opening: The invariant in the PR description — "a job can only become completed after the envelope exists" — is the sort of claim that usually lives in call ordering and then dies the first time a new caller appears. Here it lives in the mutation: markCompleted throws when the CAS affects no row, so a future caller physically cannot emit a false completed receipt. That is the difference between a documented ordering and an enforced one, and it is the right choice for this surface.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16105 (labels, state); #16017 as the originating incident; the changed-file list; sessionSummaryReceiptStore.mjs — specifically the encode/decode path, the acknowledgement SQL, and the match predicate; the SessionService diff for staging order and the in_progress transition; the SQLite.mjs migration for destructiveness; exact-head CI.
• Expected Solution Shape: A durable envelope written before any completed transition, an acknowledgement that cannot land on a stale or replaced envelope, a replay that verifies both document and metadata before claiming recovery, non-destructive migration leaving legacy rows on the existing fallback, and no path where recovery can race a live writer.
• Patch Verdict: Matches on every limb. Two things I specifically went looking for and did not find: a comparison against compressed bytes (gzip is not guaranteed byte-stable across zlib versions, so that would produce spurious mismatches) and a shallow metadata comparison. The predicate compares receipt.document after decode and uses isDeepStrictEqual on metadata — key-order-independent and type-strict, not === on objects and not JSON.stringify.
• Premise Coherence: Coheres with verify-before-assert at the data layer: the whole point is that an acknowledgement must be earned by a read-back rather than inferred from a successful write, which is the same distinction as accepted-versus-queryable on the memory side.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16105
• Related Graph Nodes: #16017 (the 81 vanished summaries — the incident this closes the mechanism for), #13462 (drift repair, retained as the legacy fallback), SummarizationJobs coordinator table

🔬 Depth Floor

Credit first, because it is a second-order property the description does not claim. "Envelope staging is the final side effect of summarizeSession()" is justified in the body by replay safety — graph projection, links, and ingestion have already succeeded. It also silently closes the classic lease bug on this surface. Recovery skips in_progress rows only while expires_at >= now, so an expired lease whose original writer is still alive (a long model call, a GC pause) is eligible for recovery — normally a two-writer race. It is benign here precisely because staging is last: if the slow writer has not staged, there is no envelope and recovery has nothing to replay; if it has staged, its result is final and replaying the identical envelope is idempotent. Worth stating in the code comment next to "deliberately the final side effect," because a future refactor that moves staging earlier for any reason would reopen the race without touching anything that looks like locking.

Challenge 1 (non-blocking): the single Post-Merge Validation item waits on "the next natural local Chroma max-runtime recycle" — an event with no forcing function, no owner-forcing deadline, and no named revalidation window. The mechanism is genuinely proven at L3 by the SIGKILL witness, so this is confirmation rather than a gap; but a PMV that depends on a natural event is the shape that silently never gets checked, and your own [ticket-updated][#16017][#16105] note already observed that a natural recycle is what falsifies the durability AC. Either name a window on #16105 ("if no natural recycle by , force one") or state the forced equivalent as acceptable. This is the same sunset-condition discipline, applied to a validation instead of to substrate.

Challenge 2 (out of scope, worth its own ticket rather than a per-PR paragraph): among the seven disclosed local-suite failures is a profiling threshold missing at 403.2ms against a 400ms bound — a 0.8% margin. A threshold that tight is a flake generator, and the cost is not the red: it is that every future PR body has to spend a paragraph explaining it, which trains readers to skim disclosed failures. Widening it or making it relative to a measured baseline would retire a recurring explanation. Not yours to fix in this PR; naming it so it stops being re-litigated per-PR.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The transferable pattern is where an invariant lives. "Do X before Y" enforced by call ordering degrades the moment a second call site exists; the same invariant enforced inside Y's mutation cannot. Here the acknowledgement is a compare-and-swap whose WHERE pins session_id AND result_envelope AND result_encoding AND result_staged_at, with success defined as changes === 1 — so a replaced envelope, a re-encoded envelope, or a re-staged identical envelope all fail the swap and refuse the acknowledgement. Including the staging timestamp in the predicate is what closes ABA, and that detail is the difference between a CAS and a payload comparison that looks like one.

🎯 Close-Target Audit

• Close-targets identified: #16105
• For each #N: confirmed not epic-labeled — #16105 carries bug, ai, architecture

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket / PR documents the delivered contract
• Implemented PR diff matches it (no drift)

Findings: Pass. Each Deltas claim was independently checked against the diff rather than accepted: one current envelope per existing coordinator row (no journal added); the in_progress transition sets result_envelope = NULL so a new drift repair cannot leave recovery replaying an obsolete result; and legacy rows without envelopes remain #13462 drift-repair candidates, which the nullable migration guarantees by construction.

🪜 Evidence Audit

• Evidence: line present — L3 (disposable Chroma acknowledgement → SIGKILL → restart → exact replay with no model invocation) → L3 required
• Achieved ≥ required: the SIGKILL-and-restart witness at the production seam is the honest instrument for a crash-durability claim, and the nondeterministic-flush handling is disclosed rather than hidden — it deletes the disposable row after restart if it survived, then proves deterministic recovery, so the test cannot pass on a row that merely persisted.
• Evidence-class collapse check: no collapse. Idempotence and interrupted-recovery are covered by separate tests rather than folded into the crash witness.
• Failure disclosure: seven local-suite failures are enumerated with their causes rather than summarised as "flaky." Exact-head CI is 14/14 SUCCESS, re-verified immediately before posting, and CI is the gate per §7.5.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: 14/14 SUCCESS at d658b8f523; author receipts are per-surface and current-head-appropriate, including a re-run of SessionService.spec.mjs in isolation after full-suite contention — which is the correct response to a load-sensitive timeout rather than a re-run of the whole suite hoping for green.
• Reviewer falsifier: run three times, all three refuted. (1) "Does the byte-identity check compare compressed bytes?" — no, it compares decoded receipt.document, so gzip stability is irrelevant. (2) "Is metadata compared with === or JSON.stringify?" — neither; isDeepStrictEqual. Both of those would have been real defects in opposite directions — permanent replay, or undetected drift. (3) "Is the migration destructive?" — five nullable columns, no DROP, legacy rows resolve to the documented fallback.
• Test location: pass — the helper spec mirrors the helper path, and the pre-existing degraded-fallback fixture change (awaiting SessionService.ready() before replacing the collection) fixes a real late-initialization reclaim rather than papering over it.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 - The envelope rides the existing coordinator row rather than introducing a parallel store; the invariant lives in the mutation; recovery is bounded and lease-aware. Placement is right and nothing new was invented to hold state.
• [CONTENT_COMPLETENESS]: 88 - Deltas enumerate the bounded-policy decision, the drift-repair interaction, and the flush-nondeterminism handling. Deducted for a PMV that depends on an unforced natural event.
• [EXECUTION_QUALITY]: 94 - CAS over the full staged tuple including the timestamp, throw-on-missing-envelope, isDeepStrictEqual, decode-before-compare, nullable migration, and short-circuit ordering in the predicate that keeps row.metadata from being dereferenced on a miss.
• [PRODUCTIVITY]: 90 - One commit, ~520 lines of production code against ~675 of specs, closing an incident mechanism rather than the incident's symptom.
• [IMPACT]: 90 - #16017 lost 81 summaries to a restart; after this, a lost deterministic row is reconstructible without re-invoking the model, which converts a data-loss class into a recovery cost.
• [COMPLEXITY]: 80 - Crash-consistency ordering, optimistic concurrency, compression, deep-equality verification, lease interaction, and a schema migration — each individually ordinary, jointly unforgiving.
• [EFFORT_PROFILE]: Heavy Lift - Durability work whose correctness lives in the interaction between five mechanisms.

🌿 The acknowledgement has to be earned by a read-back rather than inferred from a write. That is the same lesson as accepted-versus-queryable, arriving at the storage layer.