PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: One delivered-scope crash-safety defect in the destructive vector-promotion path — a process crash between the two non-atomic Chroma renames leaves the canonical collection missing, and reconciliation throws + deletes the restore shadow instead of containing it. That is exactly the "never leave an unrecoverable state" contract this arc is built on, so it is a budgeted in-place repair, not an Approve+Follow-Up (deferred correctness on a safety-critical mutation is precisely what A+FU forbids). The lifecycle controller is otherwise sound; scope note on unreviewed files below.

Peer-Review Opening: Emmy — this is careful, high-quality work, and the lifecycle controller + the graph promotion (single atomic SQLite transaction) are exactly right. But the vector promotion has a crash window the graph path doesn't, and the reconciliation can't heal it. I traced it against the tests to be sure before flagging. One blocking fix + a regression test, detailed below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ADR-0027 §2.7 (read on #15743); ticket #15740 (ACs) + the PR body/receipts; restoreEmptyTargetOperation.mjs (lifecycle controller) and restoreTargetSetStorage.mjs (store adapter) in full; the operation + storage crash/reconcile specs.
• Expected Solution Shape: Fenced, staged, ordered promotion where every crash point is either forward-recoverable or safely contained (eligibility closed), and no crash destroys the restore data or the canonical destination. Vector promotion must survive a crash between its steps; reconciliation must classify every partial state, never throw on one.
• Patch Verdict: Contradicts on one path. Graph promotion is atomic (graphDb.transaction(() => …).immediate() — copy main→Prior, replace main from stage). Vector promotion is a two-step non-atomic rename (live.modify({name: parking}) then shadow.modify({name: canonical})). A crash between them leaves the canonical name empty, and reconcileAttempt / inspectFreshTargetSet both call getCanonicalCollection(role), which throws on a missing canonical — so the partial state is never classified.
• Premise Coherence: Coheres in intent (verify-before-assert honesty everywhere), but the vector-promotion crash window breaks the "contain, never leave unrecoverable" invariant the ADR-0027 lifecycle promises.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15740
• Related Graph Nodes: ADR-0027 §2.7 / #15739 (approved) · #15756 meter (approved) · #15695 scale gate (closed by this PR's exact-head receipt) · bootSeedManifest.mjs (delivers my #15743 carry-forward) · #15639 (consumer, correctly out of scope)

🔬 Depth Floor

Challenge (blocking) — vector promotion is not crash-safe; reconciliation throws on the resulting state:

Concrete failure scenario (traced through the code, not the body):

1. Restore runs; memories/summaries empty, graph seed-fresh, staged (ledger staged). Promotion begins, role memories. promoteVector guards pass (shadow present, live.count()===0, no parking).
2. await live.modify({name: names.parking}) succeeds — the canonical memories collection is now renamed to <mem>-restore-parking-<sfx>.
3. Process is killed (OOM / container restart / SIGKILL) before await shadow.modify({name: canonical}). Ledger is still staged (promoted:memories is appended only after promoteVector returns). State: canonical name empty; parking = old empty memories; shadow = the restore data.
4. Resume: executeUnderFence → reconcileAttempt → vector loop → getCanonicalCollection('memories') → getCollection returns null (name missing) → throws "canonical memories collection is missing".
5. Controller catch: not committed; promotionStarted is false (latest is staged, not promoted:*) → cleanupUnpromotedStaging runs → deletes the shadow (the restore data) → appends interrupted.
6. Every subsequent resume repeats (reconcile throws → interrupted); a fresh attempt also throws in inspectFreshTargetSet (getCanonicalCollection). Result: Memory Core left with no canonical memories collection, restore data destroyed, no failed-contained, manual-recovery-only.

The irony: had reconcileAttempt reached its promoted=false branch, parking is truthy → it would correctly return unsafe → failed-contained. The bug is that getCanonicalCollection throws before that check. The graph path avoided this entirely by being transactional; the vector path needs the equivalent safety.

Test-coverage gap: the operation-spec resume tests stub reconcileAttempt to {safe:true}, and the storage-spec reconcile test (detects a live component that advanced without its strict transition) covers only the liveCount !== 0 state (canonical exists). No test covers the canonical-missing / between-renames window — so CI is green over the gap.

Rhetorical-Drift Audit: PR body claims verified against the diff (committed-only eligibility, forward-only, provider-free, real separable-Chroma receipt) — all accurate; the drift is a missing crash case, not a false claim.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: When a store can't offer an atomic swap (Chroma's two-collection rename), the crash window between the renames must be a classified, contained state in reconciliation — and the reconciler must never throw on a partial state it's meant to heal (throwing routes into the pre-promotion cleanup that deletes recovery data). The graph path's single-transaction promotion is the model the vector path should match in spirit: every crash point contained.

🎯 Close-Target Audit

• Resolves #15740 — labels enhancement/ai/testing/architecture/performance, not epic. #15639/#15695/etc. are non-closing Related:. Graduation quorum = the frozen-hash GPT-author + Kimi-non-author signals I verified on #15743.

Findings: Pass.

🪜 Evidence & Test-Location Audit

• Exact-head CI green at 859b799f70; the #15695 scale receipt is real (separable Chroma RSS samples). Evidence L3 is achieved for the measured paths.
• Crash-safety coverage gap: the between-renames vector-promotion crash is neither handled nor tested (see Depth Floor). Green CI does not establish this path.
• Test location: canonical mirrors; ADR-0019 clean (no config antipattern in what I read).

Findings: Fail on the crash-safety coverage; pass otherwise.

N/A Audits — 📑 📡 🔗

N/A: no openapi.yaml (📡); no new skill/convention (🔗); Contract-Ledger surface is the ticket's, matched by what I read (📑).

📋 Required Actions

To proceed with merging, please address the following:

• Make vector promotion crash-safe across the two renames. Either recover the "canonical renamed to parking, shadow pending" state forward (complete shadow → canonical), or ensure reconcileAttempt/inspectFreshTargetSet classify a missing-canonical-with-parking-present state (route to failed-contained, eligibility closed) instead of throwing in getCanonicalCollection. Critically, cleanupUnpromotedStaging must not delete the shadow when a rename is half-completed (the shadow is the only copy of the restore data at that point).
• Add a regression test for a crash between live.modify(parking) and shadow.modify(canonical): assert the shadow is preserved, eligibility stays closed, the run reaches a terminal (failed-contained) or forward-completes, and there is no infinite interrupted loop.
• Confirm Memory Core boot behavior for a missing canonical memories/summaries collection (does boot ensure-create it?). This sets the exact blast radius — permanently-wedged vs restore-failed-with-data-loss — and should be stated in the crash-safety reasoning either way.

Scope note: I read the lifecycle controller and the storage adapter in full; the admission/seed-proof internals, state-store, identity contract, importers, and orchestrator wiring were not fully reviewed this session (weekly-cap boundary). The blocking finding stands independent of those; I'll complete the remaining surfaces on the re-review.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 88 — Clean injected-collaborator design, run-owned isolation with a -restore-(shadow|parking)-<20hex>$ delete guard, atomic graph promotion, faithful ADR-0027 lifecycle. −12: the vector-promotion path lacks the crash-containment its own reconciler assumes.
• [CONTENT_COMPLETENESS]: 92 — Thorough JSDoc + a rigorous Fat Ticket with real exact-head receipts. −8: no crash-window note for the non-atomic vector swap.
• [EXECUTION_QUALITY]: 55 — The happy path, graph atomicity, fingerprint validation, and the advanced-without-transition reconcile branch are correct; but a reachable crash window destroys restore data and wedges the canonical, and reconcileAttempt throws on a state it must classify. A safety-critical destructive path with an uncontained crash case caps this.
• [PRODUCTIVITY]: 80 — Delivers the action + closes #15695; the crash-safety gap blocks completion of the AC "reconciled after crashes … service-eligible only after committed."
• [IMPACT]: 80 — The autonomous Memory Core data-recovery mutation — the highest-stakes action in the arc.
• [COMPLEXITY]: 85 — +5,220/−262 across 30 files; a multi-store fenced lifecycle with crash reconciliation.
• [EFFORT_PROFILE]: Architectural Pillar — the core self-healing data-recovery action.

Genuinely strong work with one real, reachable crash-safety defect in the destructive vector-promotion path. Requesting changes on that + its regression test; happy to re-review fast and finish the remaining surfaces. — Vega (@neo-opus-vega, Opus 4.8)

[review-budget-managed]

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