PR Review Summary

Status: Approved

§0 Prio-0 Sanity Gate

Does this make sense + fit the architecture/goals? Yes. #12153 collapsed Chroma into one unified store, which makes the defrag script's old "two physical folders" model obsolete; reframing the two TARGETS as collection-groups over the single store is the necessary, coherent follow-on. The premise holds before any line-level review.

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: Correctly answers the open question I left in #12156 ("confirm whether the graph collection belongs here") — and fixes a real gap doing so: neo-native-graph is a memory-core-owned Chroma collection (memory-core/config.template.mjs:121 + the canonical MEMORY_CORE_COLLECTION_PREFIXES = [memory, session, native-graph]), and it was previously orphaned (in neither defrag group). Target-metadata-only change; the destructive keep-set safety is the inherited #12140 mechanism; the live destructive run is correctly L3/operator-gated. No operator-owned SSOT touched (collection names read from config verbatim).

Peer-Review Opening: Clean, well-scoped. Good call exporting TARGETS for focused coverage instead of duplicating the shape, and the .filter(Boolean) keeps it backward-compatible for configs without a graph collection.

🕸️ Context & Graph Linking

• Resolves #12156 (sub of #12153; enhancement/ai/refactoring/architecture, not epic). Related: #12140 (keep-set), #12153 (unified store).

🔬 Depth Floor

Verification: confirmed neo-native-graph is a real Chroma collection (not the SQLite graph) owned by memory-core; adding it to the memory-core group aligns the defrag with MEMORY_CORE_COLLECTION_PREFIXES and closes the orphan gap. Ran defrag-segment-cleanup.spec.mjs → 6 passed (incl. the new target-adapter-shape test asserting both groups share the unified path while scoping collections per group).

Challenge (non-blocking): the ticket's AC2 — "negative test: defragging one realm preserves the others' live segments" — is satisfied by construction + inheritance, not by a new dedicated test in this PR: the keep-set spans all live segments across the whole store (#12140), so adding a collection to one group can't delete another group's live segments, and the existing removed.toEqual([trueOrphan]) test proves only orphans are cleaned. The new test here verifies the target shape, not realm-A-preserves-realm-B directly. That's acceptable (the mechanism guarantees it and the live realm-composition run is your L3 Post-Merge step), but a tight memory-core defrag keeps neo-knowledge-base segments unit (driving cleanOrphanedSegmentDirs with a two-realm keep-set) would close AC2 literally — worth a small follow-up if you want the AC test-anchored rather than mechanism-anchored.

🎯 Close-Target Audit

• Resolves #12156 — whole-body prose scan clean (only the intended line); closingIssuesReferences = [12156]; not epic. Commit 5acc0eb1a clean. Pass.

🪜 Evidence Audit

• Declares L2 (target adapter + keep-set unit) → L3 required before live operator defrag; the destructive segment-dir deletion is correctly operator-gated in Post-Merge Validation. Honest two-ceiling split. Pass.

🧪 Test-Execution Audit

• Ran the related spec locally → 6 passed. Location canonical. CI green (unit, integration-unified, CodeQL, Analyze, check, lint-pr-body); mergeStateStatus: CLEAN. Tests pass.

📋 Required Actions

No required actions — eligible for human merge.

(Optional follow-up: the AC2 realm-composition unit noted above; the live two-realm defrag is already in your Post-Merge Validation.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 — collection-groups-over-unified-store is exactly the post-#12153 shape; aligns the defrag with the canonical memory-core collection set. 3 deducted: AC2 is mechanism-anchored, not test-anchored, in this PR.
• [CONTENT_COMPLETENESS]: 95 — docstring updated from per-folder to collection-group framing + usage paths corrected; clear deltas/residual. 5 deducted: no inline note on why the graph collection now belongs (the orphan-gap rationale lives only in the PR body).
• [EXECUTION_QUALITY]: 96 — 6/6 pass (ran locally); .filter(Boolean) backward-compatible; target metadata only, destructive logic untouched. 4 deducted: AC2's literal negative-test deferred to the inherited mechanism + L3.
• [PRODUCTIVITY]: 95 — resolves #12156 including its open graph-collection question; AC1 (unified path, collection-scoped) + AC3 (dependency met) done. 5 deducted: AC2 mechanism-anchored.
• [IMPACT]: 50 — operator-maintenance reliability over the unified store; closes a real orphaned-collection defrag gap.
• [COMPLEXITY]: 30 — small target-map change + one focused test; the hard safety work (#12140 keep-set) predates this.
• [EFFORT_PROFILE]: Quick Win — small, correct, gap-closing change building on the unified-store substrate.

Approving — over to @tobiu for merge (live defrag stays your L3 validation).

— Reviewed by Opus 4.8 (Claude Code), session af888fcb-6a68-45bd-8a34-13be477733b1.