PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve+Follow-Up
• Rationale: Core correctness is verified by source-read and 62 green related tests that map 1:1 onto all seven ACs; no release-blocking defect exists. The remaining findings are a non-blocking observability gap and two edge-case observations. This PR is the critical path to restarting the live ~13k-row Memory Core repair, so another review cycle would cost more than it returns — the observability item is better tracked as a boardless follow-up than as a merge-blocking iteration.

Peer-Review Opening: Strong, well-shaped fix, Euclid. This is the right architectural move — it reuses the existing heavy-maintenance lease and shadow/park/promote machinery rather than forking new primitives, and the resumability is genuinely crash-safe. I verified the load-bearing assumptions against source rather than the PR body, ran the related suite at head, and traced the migration path from whatever marker the current crashed run left on disk. One non-blocking follow-up and a couple of edges to note below.

Single-family reviewer (Vega / Opus). Operator @tobiu is the in-loop merge gate per the stated plan (merge → restart repair/orchestrator/harnesses); this is operator-supervised, not an asleep-human night-shift approval.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #14020 (Fat Ticket + Contract Ledger + 7 ACs + Avoided Traps); changed-file list; current dev source of both production files; sibling precedent runSandman.mjs / backup.mjs lease wrappers + HeavyMaintenanceLeaseService.buildLeasePayload/withHeavyMaintenanceLease; touched-file git history (#14017/#14011/#14005/#13634/#13601/#12140-line). Semantic Memory Core sweep was unavailable (embedding provider down — the very subsystem this PR repairs), so prior-art V-B-A substituted to source-history + the ticket's cited prior tickets.
• Expected Solution Shape: Stream recovered batches into a durable shadow collection before the next provider call; record resumable phase state + skip already-loaded ids on rerun; isolate provider-overcap rows as per-row unrecoverable (abort promotion, preserve shadow); wrap only the CLI auto-run in the existing withHeavyMaintenanceLease (exported fns stay lease-free). Must NOT hardcode a new defrag-specific lock (fork of the scheduling contract) and must NOT re-embed intact rows on load. Tests must mock Chroma + the embed provider; no live defrag.
• Patch Verdict: Matches / improves. The change preserves the existing in-memory path's validation rigor while adding streaming durability. The decisive evidence: addCollectionData (defragChromaDB.mjs:473–478) passes embeddings explicitly to collection.add(...), so the shadow receives the recovered/intact vectors verbatim — Chroma does not re-embed via the collection's embeddingFunction. The premise (preserve intact stored vectors, only re-embed missing) holds through the new streaming path.
• Premise Coherence: Coheres — verify-before-assert (fail-loud unrecoverable counts, fail-closed on the non-resumable rename phases, extraIds ambiguous-resume guard) and friction→gold (a real 5h-loss live incident converted into a durable, resumable maintenance contract). No value-surface conflict.

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #14020
• Related Graph Nodes: #13999 (Memory Core repair incident epic context), #14017 (progress timestamping, separate), #14018 (just-merged sibling), #12140 / #12142 / #12153 (prior unified-Chroma defrag/shadow-promotion hardening). ADR-0017 (unified Chroma store).

🔬 Depth Floor

Challenge (non-blocking):

1. Swallowed unrecoverable-reason (observability). embedRecoverableDocuments (repairMemoryCoreStoredEmbeddings.mjs) does batch-first → per-doc isolation, but the inner catch (error) discards error — only the row index is recorded. The COUNT is fail-loud, but the CAUSE is silent. For the imminent live ~13k-row repair, the operator gets unrecoverablePreview ids with no reason (overcap vs malformed doc vs transient provider blip). → Follow-up below.
2. Per-doc storm under a whole-provider outage (edge). If a batch fails because the provider is entirely down (not one overcap row), the fallback issues up to batchSize (1000) sequential single-doc embeds, all failing, per batch, before aborting. Not a correctness bug — shadow progress is preserved and rerun self-heals — but it's wasted provider load during an outage. Distinguishing "all docs in the isolation pass failed" (→ likely provider-down, abort louder) from "one row failed" (→ true per-row overcap) would tighten it.
3. Resumable-shadow vanished (edge). On resume, client.getCollection({name: shadowName}) throws a raw Chroma "not found" if the recorded shadow was externally deleted. Fail-loud (won't silently restart and lose intent), but a wrapped message ("resumable shadow X no longer exists; clear state to start fresh") would read better.

I also actively verified and cleared: (a) intact vectors are not re-embedded on shadow load (✓ explicit embeddings); (b) resume skipIds filters both intact and missing sets (✓); (c) break-on-first-abort is intentional and consistent with the single-active-collection resume model (state tracks one collectionName/shadowName); (d) old/missing-field state markers fail closed — an old-format memory-core-repair-aborted marker (no collectionName/shadowName, e.g. from the currently-crashed run) is caught by the !resumeState.collectionName || !resumeState.shadowName → throw "manual recovery required" guard, and any non-allowed mid-rename phase (live-parked/shadow-promoted/…) blocks rerun. This migration safety is directly relevant to the operator's restart.

Rhetorical-Drift Audit: Pass. Every PR-body claim is substantiated by the diff: "streams recovered rows into a durable shadow collection" (onDataBatch→addDataFn), "records resumable phase state" (per-batch writeStateFn), "skips already-loaded shadow IDs" (listCollectionIds→skipIds), "isolates provider-overcap rows" (embedRecoverableDocuments), "keeps the exported defrag implementation lease-free and wraps only direct CLI execution" (defragChromaDB untouched; only the import.meta.url entry calls runDefragChromaDBCli). New-function JSDoc matches mechanical behavior.

🧠 Graph Ingestion Notes

• [TOOLING_GAP]: Semantic Memory Core query tools (query_raw_memories/query_summaries) returned Memory Core is not fully operational: Embedding write canary failed … spawn lms ENOENT during this review — expected, since the orchestrator/embedding provider are intentionally stopped. Prior-art V-B-A substituted to git source-history + cited tickets. Flagging because PR-review's prior-art-sweep gate assumes those tools are live.
• [RETROSPECTIVE]: The clean reuse here is the lesson — defrag becomes the Nth heavy-maintenance-lease consumer with zero new locking primitives (the manualHeavyMaintenanceScriptLeaseAdoption spec even asserts "no per-script private locks"), and the resumable shadow rides the existing park/promote/validate/rollback contract. The fix is an implementation correction to obey existing contracts, not a new abstraction — exactly the avoided-trap the ticket named.

🎯 Close-Target Audit

• Close-targets identified: Resolves #14020 (newline-isolated, single leaf; PR body + commit 6c4c16ac44 subject). No stray Closes/Fixes, no comma-separated/prose targets.
• #14020 is a fix/bug ticket, not epic-labeled.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #14020 contains a 3-row Contract Ledger matrix (CLI auto-run lease; MC repair shadow state; missing-vector extraction).
• Implemented diff matches each row: (1) runDefragChromaDBCli acquires lease + defers-on-held with holder log; (2) resume keyed on collectionName+shadowName, malformed/missing-field → fail-closed manual recovery; (3) extractMemoryCoreCollectionData streams batches, skipIds skips loaded, unrecoverable aborts promotion preserving shadow. No drift.

Findings: Pass.

🪜 Evidence Audit

• PR body carries a greppable Evidence: line — L2 focused unit coverage achieved for all close-target ACs; no live defrag was run. Residual: none for #14020.
• Achieved L2 ≥ required: every AC is unit-testable behavior (mocked Chroma + injected withLease/embedFn seams), and I reproduced 62/62 at head. The live lease-deferral / live resumable defrag is correctly an L3/L4 post-merge operator validation (the ticket's own Out-of-Scope forbids live defrag in CI/unit) — sandbox-ceiling, not under-probing. The single unchecked ## Post-Merge Validation box is the honest two-ceiling distinction, not an uncovered AC.

Findings: Pass — close-target ACs covered at L2; live behavior is sandbox-ceiling-bounded and declared as post-merge validation.

N/A Audits — 📡 🔗 🛂

N/A across listed dimensions: no openapi.yaml touched (no MCP tool-description surface); the heavy-maintenance-lease convention already exists and is doc'd/test-enforced — defrag joins it, introducing no new cross-skill convention; this extends existing primitives (lease + shadow-promotion from #12140-line) rather than introducing a major new abstraction, so the Provenance trigger doesn't fire (origin is internal + explicitly cited).

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact head 6c4c16ac44 in this clone (fetched pull/14021/head; deliberately re-checked out the explicit SHA after a git fetch origin dev clobbered FETCH_HEAD, to avoid the known cross-clone false-green).
• Canonical location ✓ — all 4 specs under test/playwright/unit/ai/scripts/maintenance/.
• Ran the exact 4 related specs: 62 passed (941 ms). Test names map onto every AC (resume skipIds, stream-without-retaining, batch→single-doc isolation, wrong-length→unrecoverable, durable phase marker resume, owner:'defrag' lease wrap, 'held' non-error exit + diagnostic, park-failure rollback, "no per-script private locks").
• CI at head: all 9 checks green (CodeQL, unit, integration-unified, lint ×2, lint-pr-body, Analyze, check, classify).

Findings: Tests pass; locations canonical; no test gap.

📋 Required Actions

No required actions — eligible for human merge.

Non-blocking follow-up (suggested, boardless — author's Tier-2 call):

• Capture the per-doc embed failure reason in embedRecoverableDocuments (don't discard the catch (error) binding) and surface it in the unrecoverable report / unrecoverablePreview (e.g. {id, reason}), so the operator can triage overcap vs malformed vs transient during the live repair without manually re-querying each id. Optionally fold in the edge polish from Depth-Floor #2/#3 (all-docs-failed → provider-down signal; wrapped error when a resumable shadow is missing).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — reuses the existing heavy-maintenance lease (explicitly avoids the forked-lock trap the ticket names) and the shadow/park/promote/rollback pattern; seam-injected for unit isolation; fail-closed on the non-resumable rename phases. −5: the per-doc catch discards the embed error, a small departure from the module's stated row-level fail-loud discipline.
• [CONTENT_COMPLETENESS]: 92 — every new function carries Anchor & Echo JSDoc; PR body is a complete Fat Ticket and honors the Contract Ledger. −8: the discarded error binding loses the unrecoverable-reason content the live operator flow will want.
• [EXECUTION_QUALITY]: 90 — verified by source: explicit-embeddings preservation, both-set skipIds resume, per-row overcap isolation, park-failure rollback, extraIds ambiguity guard, old/missing-field markers fail closed to manual recovery; 62 related tests green at head. −10: observability gap + per-doc storm under full-provider outage + raw throw if a resumable shadow was externally deleted (all non-blocking).
• [PRODUCTIVITY]: 100 — all seven ACs delivered, each mapped to a passing test.
• [IMPACT]: 80 — critical-path operational recovery: converts a one-shot 5h-loss re-embed into a resumable operation for a ~13k-row corrupted live Memory Core; scoped to maintenance tooling, not core framework.
• [COMPLEXITY]: 85 — a resumable multi-phase state machine spanning two modules with park/promote/validate/rollback and per-batch durable markers; five ordered promotion phases create high reader load.
• [EFFORT_PROFILE]: Heavy Lift — high complexity (crash-safe resumable state machine) at high operational impact (live corruption recovery).

Approving for human merge. The migration-safety property (old/missing-field markers fail closed to manual recovery) means the currently-crashed on-disk state won't be blind-resumed — the operator should expect either a clean resume from a new-format marker or an explicit "manual recovery required" message on the first restart. Nice work, Euclid.

— Vega 🖖