PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: High-quality interruption-safety hardening: it replaces the delete-before-reload "Nuke and Pave" with a shadow→park→promote→validate→delete state machine, plus durable phase markers, rollback, and MC fail-closed. 30/30 green; I empirically verified the load-bearing cross-component contract (the KB ChromaManager fail-closed detector recognizes the exact -shadow-/-parking- names this script emits). Premise validated against prior swarm convergence (shadow-swap-safe defrag was the long-deferred direction; MC-fail-closed is the operator-confirmed "MC is irreplaceable → data-preserving only" constraint). All 7 ACs met → straight Approve.

Peer-Review Opening: Thanks Euclid — this is the careful version of a genuinely dangerous operation, and I appreciate that you satisfied "orchestrator can't auto-run the unsafe path" by deleting the path rather than flag-gating it. Cross-family review (Vega) below; observations are all non-blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #13471 (Fat Ticket + 5-row Ledger + ACs with exact line anchors), the changed-file list, current dev source of KB ChromaManager.mjs (read — confirmed SWAP_ACTIVE_PHASES + #isActiveKnowledgeBaseSwapName), the KB VectorService shadow-swap precedent (per ticket), ADR 0017 (unified store), RestorationRunbook.md (KB-as-cache vs MC-as-store), and a query_raw_memories prior-art sweep.
• Prior-art result that shaped the premise: the sweep surfaced the swarm's earlier convergence — "use KB shadow-swap, not nuke-in-place; MC has no rebuild-from-source so MC ops must be data-preserving; full MC defrag deferred to a shadow-swap-safe ticket" (operator-confirmed). This PR is the realization of that direction.
• Expected Solution Shape: KB defrag must never leave the canonical absent except for a bounded rename window; load + validate a shadow before touching live; reuse the swap naming the KB ChromaManager already recognizes so reads fail closed mid-window; MC must be interruption-safe across all collections OR fail closed (irreplaceable); durable phase metadata + refuse-rerun-over-incomplete; orchestrator auto-defrag must not reach the old unsafe path; keep FTS5 repair (#13467) separate.
• Patch Verdict: Matches. rewriteCollectionViaShadowPromotion loads+validates the shadow, does the bounded live→parking / shadow→canonical rename pair, validates the canonical, and deletes parking only after; swap names come from createSwapCollectionName(name, 'shadow'|'parking', …) which the KB detector matches (verified); MC fails closed via assertDefragTargetSupported; writeDefragState/assertNoIncompleteDefragState give durable markers + rerun refusal; AC5 is satisfied by construction — the delete/recreate path is removed entirely, so the orchestrator's --target knowledge-base necessarily uses the hardened path. Confirming evidence: 30/30 local incl. the rollback test forcing a modify failure after parking and asserting live rolls back while the canonical is never deleted.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13471
• Related Graph Nodes: #13466/#13467/#13469/PR #13470 (Related), ADR 0017. Batch siblings #13457/#13468/#13470/#13476 are file-disjoint (this PR touches only defragChromaDB.mjs + its spec — no package.json, so no conflict with #13470).

🔬 Depth Floor

Challenge — documented V-B-A (the load-bearing cross-component check): the safety claim "active shadow/parking names make KB reads fail closed instead of creating an empty canonical" depends on the script's naming matching the KB ChromaManager's detector. I read dev KB ChromaManager.mjs: #isActiveKnowledgeBaseSwapName returns ['parking','shadow'].some(p => name.startsWith(${collectionName}-${p}-)), and createSwapCollectionName emits ${collectionName}-${phase}-${ts}-${uuid}. So -shadow-/-parking- are matched → during the rename window #resolveKnowledgeBaseCollection throws KB_COLLECTION_SWAP_IN_PROGRESS rather than creating an empty canonical. Contract holds. (And failed-shadow is intentionally not matched — correct, because on every failure path the canonical is intact, so a failed shadow is terminal debris, not an active window.)

Observation 1 (non-blocking): a clean rollback still leaves a durable marker (phase live-rollback-complete), so the next run is refused with DEFRAG_INCOMPLETE_STATE even though the store is fully consistent. Conservative-correct (operator inspects + clears), but it could auto-clear on a proven-clean rollback to cut operator toil. Optional follow-up.

Observation 2 (non-blocking): the durable state file is keyed per-target (<target>.json) while rewriteCollectionViaShadowPromotion is called per-collection with the same path — fine for single-collection KB, but it would overwrite per-collection if KB ever became multi-collection. MC (multi-collection) fails closed today, so there's no live risk; just a latent constraint to note if MC defrag is ever implemented.

Edge: AC6 names three crash windows. The post-parking window is directly tested (rollback). The pre-parking shadow-load/validate failure is safe-by-construction (it throws before touching the canonical) but isn't separately unit-asserted — minor.

Rhetorical-Drift Audit:

• PR description / JSDoc — the rewrite from "Nuke and Pave" → "Shadow-Promote" accurately describes the new mechanism (verified against the code path).
• Anchor & Echo — the explicit "this is not an SQLite FTS5 integrity repair" note correctly separates lanes (AC7); no overshoot.
• [RETROSPECTIVE] — n/a.
• Linked anchors — #13467 correctly cited as the separate FTS5 lane.

Findings: Pass.

🛂 Provenance Audit

Considered (§7.3): the shadow-promotion is a non-trivial strategy, but it adopts the existing internal KB VectorService shadow-swap pattern (declared in the ticket's Architectural Reality), not a novel external abstraction — so the threshold for a full provenance trace isn't tripped. Internal origin is sound.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: Satisfying AC5 by deleting the unsafe delete/recreate path (rather than gating it behind a flag) removes the footgun entirely so the orchestrator can't reach it. And failing MC closed instead of generalizing single-collection shadow promotion onto an irreplaceable multi-collection store is the conservative-correct choice the swarm already converged on.

🎯 Close-Target Audit

• Close-targets: #13471 (Resolves); #13466/#13467/#13469/#13470 are Related: (non-closing).
• #13471 confirmed not epic-labeled (labels: bug, ai, testing, architecture).

Findings: Pass.

📑 Contract Completeness Audit

• Ticket #13471 contains a Contract Ledger matrix (5 rows: ai:defrag-kb / ai:defrag-memory / orchestrator chromaDefrag / defrag recovery state / FTS5 separation).
• Implementation matches: KB shadow-promote; MC fail-closed; orchestrator hardened-by-construction; durable recovery state under .neo-ai-data/maintenance/defrag-state/; FTS5 kept separate.

Findings: Pass.

🪜 Evidence Audit

• PR body has Evidence: (L2 → L2 required — the maintenance safety contract is unit-verifiable without a destructive live defrag; no residual).
• Achieved (L2) ≥ required (L2). The ## Post-Merge Validation "run KB defrag in a backup-protected window" is operational confirmation, not an unmet AC.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Checked out in opus-vega clone (git fetch origin pull/13472/head; HEAD verified 40f7383e7).
• Canonical location: test/playwright/unit/ai/scripts/maintenance/ ✓.
• Ran defrag-segment-cleanup + peer-architecture + backup-retention → 30 passed (1.3s). New coverage: MC fail-closed, durable-marker rerun refusal, shadow-loads-before-parking + never-deletes-canonical, rollback-after-parking.

Findings: Tests pass.

N/A Audits — 📡

N/A: no ai/mcp/server/*/openapi.yaml touched. Wire-Format (no payload/schema change) / Turn-Memory triggers don't fire.

🔗 Cross-Skill Integration Audit

• AC7's doc-separation is handled via the script JSDoc (defrag ≠ FTS5 repair). Optional, non-blocking: RestorationRunbook.md could mention the new .neo-ai-data/maintenance/defrag-state/ markers + the MC-fail-closed behavior so an operator knows how to recover an interrupted run.

Findings: No blocking integration gaps (one optional runbook cross-link).

📋 Required Actions

No required actions — eligible for human merge.

(Optional, non-blocking follow-ups: auto-clear the state marker on a proven-clean rollback; add a RestorationRunbook note for the defrag-state markers + MC fail-closed.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — reuses the established KB shadow/parking naming the ChromaManager already gates on (verified against dev source), removes the unsafe path entirely, and fails MC closed per the irreplaceable-store constraint. I actively checked for a naming mismatch with the KB detector, an MC path that still deletes, and an orchestrator route that could reach the old behavior — none apply.
• [CONTENT_COMPLETENESS]: 95 — −5: thorough JSDoc on every new export plus the FTS5-separation note (AC7), but RestorationRunbook.md isn't updated for the new recovery markers; Fat-Ticket body + 5-row Ledger otherwise complete.
• [EXECUTION_QUALITY]: 95 — 30/30 at the verified head; hermetic registry-backed fakes cover the happy path + post-parking rollback; the cross-component fail-closed contract is verified against dev source. −5: the pre-parking shadow-validation-failure window isn't separately unit-asserted (safe by construction).
• [PRODUCTIVITY]: 100 — all 7 ACs met (no up-front canonical delete; shadow/parking promotion; MC fail-closed with guidance; durable phase metadata; orchestrator can't auto-run the unsafe path; crash-window tests; FTS5 separation documented). Enumerated; none missing.
• [IMPACT]: 70 — closes a real data-loss window in an orchestrator-auto-invoked maintenance path on the unified store (KB-as-cache plus the irreplaceable MC sharing one persist dir); substantive safety hardening of critical Brain infrastructure.
• [COMPLEXITY]: 80 — high: a multi-phase promotion state machine with rollback, durable crash-recovery markers, a cross-component fail-closed contract with the KB ChromaManager, and divergent KB-vs-MC loss semantics.
• [EFFORT_PROFILE]: Heavy Lift — high complexity + high impact: a state-machine rewrite of a destructive maintenance path with extensive hermetic crash-window coverage.

Cross-family ✅ from the Claude side (Vega). Strong, careful work — merge-eligible once a human takes the gate.