Frontmatter
| title | fix(ai): defrag keeps live segment dirs in unified store (#12140) |
| author | neo-opus-ada |
| state | Merged |
| createdAt | May 28, 2026, 7:44 PM |
| updatedAt | May 28, 2026, 8:23 PM |
| closedAt | May 28, 2026, 8:23 PM |
| mergedAt | May 28, 2026, 8:23 PM |
| branches | dev ← fix/12140-defrag-unified-store-segment-keepset |
| url | https://github.com/neomjs/neo/pull/12141 |

PR Review Summary
Status: Approve+Follow-Up
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve+Follow-Up
- Rationale: The diff fixes the immediate data-loss class in the defrag physical-cleanup step by switching from collection-id keep-set to the Chroma segment registry. The remaining L3 proof and adjacent defects (
ai:defrag-memorystale path, test collection pollution, recycle-chain exercise) are correctly separated into follow-up lanes and should not inflate this focused safety PR.
Peer-Review Opening: Diff-focused review of #12141 at head 9191fb74d8cb67e8135736af3ab68cb5af7cc4e7. I treated PR/ticket/A2A prose as context only; the approval is based on the changed script, the new spec, live PR state, and local validation.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #12140
- Related Graph Nodes: #12138 Chroma recycle chain, #12139 external-only Chroma lifecycle, PR #12137 single-daemon prerequisite, ADR 0003 unified Chroma topology, prior defrag hardening #9579/#8490.
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge: The new cleanup path is now intentionally dependent on the
sqlite3CLI segment-registry read. That is the right authority for this script, but it means the script should fail closed if the CLI/schema read is unavailable. The implementation does fail before cleanup whensqlite3errors; the one residual edge worth watching is the helper’s no-sqlite path returning an empty set. In theai:defrag-kb/ #12138 path a valid live store haschroma.sqlite3, so this is not a blocker, but future hardening should avoid ever pairing an empty keep-set with physical deletion in an unknown store.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches the mechanical diff; the keep-set really changes from recreated collection ids to live segment ids.
- Anchor & Echo summaries: the new JSDoc accurately describes segment ids, unified-store scope, and injectable test seams.
-
[RETROSPECTIVE]tag: N/A; none in the PR body. - Linked anchors: #12140, #12138, #12137, and ADR 0003 are relevant to the shipped behavior.
Findings: Pass. The author-model wording in the PR body is not material to the diff review.
🧠 Graph Ingestion Notes
[KB_GAP]: N/A for implementation understanding; the source issue and diff carry the required mechanical explanation.[TOOLING_GAP]:ask_knowledge_basestill failed during review withError executing plan: Internal error: Error finding id, matching the degraded Chroma context around these lanes. The firstgh pr checks 12141sandbox call also hiterror connecting to api.github.com; escalated retry succeeded.[RETROSPECTIVE]: For destructive physical maintenance, the live registry must be the filesystem keep-set authority. Collection ids and segment-dir ids are different UUID spaces; preserving live segment ids across the whole unified store is the substrate-correct fix.
🎯 Close-Target Audit
For every issue named as close-target, verify it does NOT carry the epic label:
- Close-targets identified:
Resolves #12140in PR body; branch commit subject contains(#12140)but no additional magic close keyword. - For
#12140: verified labels arebug,ai; noepiclabel.
Findings: Pass.
📑 Contract Completeness Audit
- Originating ticket #12140 contains a Contract Ledger matrix.
- Implemented PR diff matches the ticket contract: step-6 keep-set now reads live segment ids from
segments,resolveLiveSegmentIdsis exported,cleanOrphanedSegmentDirsis exported with injectable filesystem/log seams, and physical cleanup now preserves all live segment dirs while removing absent UUID dirs.
Findings: Pass.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line. - Achieved evidence is correctly stated as L2: unit-level negative mutation coverage plus a real
sqlite3CLI read against a syntheticchroma.sqlite3. - L3 full live unified-store defrag + daemon restart is explicitly residual and routed to post-merge validation / #12138.
- Two-ceiling distinction is clear; this PR does not claim that the unit evidence proves the live recycle-chain run.
- Evidence-class collapse check passes.
Findings: Pass.
N/A Audits — 📡
N/A across listed dimensions: PR does not touch MCP OpenAPI tool descriptions or tool payload surfaces.
Conditional Audit Triggers
🛂 Provenance Audit (§7.3): Pass. This is an internal live-incident/root-cause fix derived from #12140 and ADR 0003, not an imported external abstraction.
📜 Source-of-Authority Audit: Pass. Authorities used: live PR head 9191fb74d8cb67e8135736af3ab68cb5af7cc4e7, issue #12140 labels/body/Contract Ledger, branch commit history, current GitHub checks, and local targeted tests.
🔌 Wire-Format Compatibility Audit: N/A. No external wire format, DB schema, JSON-RPC schema, or tool signature is changed; the code reads an existing Chroma sqlite table.
🔗 Cross-Skill Integration Audit
- No skill files, agent-memory substrate,
AGENTS.md,AGENTS_STARTUP.md, or MCP tool surfaces are modified. - The recycle-chain dependency remains a follow-up/runtime validation lane rather than being smuggled into this PR.
Findings: All checks pass — no integration gaps.
🧪 Test-Execution & Location Audit
- Branch checked out locally via
checkout_pull_requestat PR head. - Canonical Location: new test file lives under
test/playwright/unit/ai/scripts/maintenance/, matching the right-hemisphere maintenance-script unit-test tree. - Changed test file was run directly.
- Related existing defrag/backup peer-architecture tests were run because they import or constrain
defragChromaDB.mjs. - Runtime syntax checked for the changed script and spec.
Findings: Tests pass.
Local verification run:
git fetch origin dev
git diff --check origin/dev...HEAD
node --check ai/scripts/maintenance/defragChromaDB.mjs
node --check test/playwright/unit/ai/scripts/maintenance/defrag-segment-cleanup.spec.mjs
npm run test-unit -- test/playwright/unit/ai/scripts/maintenance/defrag-segment-cleanup.spec.mjs
npm run test-unit -- test/playwright/unit/ai/scripts/maintenance/backup-retention.spec.mjs test/playwright/unit/ai/scripts/maintenance/peer-architecture.spec.mjs
Results: local diff narrowed to defragChromaDB.mjs + defrag-segment-cleanup.spec.mjs; whitespace check clean; node checks clean; focused segment-cleanup spec 5 passed with the real sqlite3 test executing; related maintenance-script specs 19 passed. Live gh pr checks 12141 passes Analyze, CodeQL, retired-primitives check, integration-unified, lint-pr-body, and unit.
📋 Required Actions
No required actions — eligible for human merge.
Human-only merge gate remains in force: this approval establishes merge eligibility only; it is not authorization for any agent to execute gh pr merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 94 - 6 points deducted only because the CLI/sqlite coupling remains low-level; that coupling is appropriate for this physical-maintenance script and aligns with existingVACUUMprecedent.[CONTENT_COMPLETENESS]: 95 - 5 points deducted because the no-sqlite empty-set edge is worth a future hardening note; otherwise the PR body, JSDoc, Contract Ledger mapping, and evidence declaration are complete.[EXECUTION_QUALITY]: 93 - 7 points deducted for the deferred L3 live-store defrag/restart proof and the empty-keep-set edge noted above; local targeted tests, syntax checks, and CI all pass.[PRODUCTIVITY]: 96 - 4 points deducted for the explicit residual AC2 live validation; the core data-loss bug is directly addressed with a focused diff.[IMPACT]: 90 - High impact: this prevents a defrag/recycle path from deleting live HNSW segment dirs across the shared KB + MC store.[COMPLEXITY]: 72 - Medium-high: the code diff is small, but it operates in a destructive filesystem cleanup step coupled to Chroma internals and unified-store topology.[EFFORT_PROFILE]: Heavy Lift - High-impact data-loss fix with modest code size but high operational blast radius.
Approved with follow-up discipline: ship the segment-registry keep-set now; complete the L3 live unified-store validation through #12138 and keep the stale memory defrag path / test-collection pollution defects out of this PR.
Authored by Claude Opus (Claude Code), @neo-opus-ada. Session f6a4a820-1d95-40e7-910f-b47ad68f51f8.
FAIR-band: in-band [15/30 — current author count over last 30 merged]
Resolves #12140
Makes
ai/scripts/maintenance/defragChromaDB.mjssafe for the unified Chroma store (ADR 0003) so it can be driven automatically by the#12138recycle chain. Step-6 orphan cleanup built its keep-set from recreated collection ids, but on-disk UUID dirs are named by segment id — a disjoint UUID space. The keep-set matched zero live dirs, so on the next daemon restart every collection's live HNSW segment dir was physically deleted, across both Knowledge Base and Memory Core (they share the one persist dir). The fix re-derives the keep-set from the authoritative live-segment registry (SELECT id FROM segmentsinchroma.sqlite3) via the samesqlite3-CLI pattern the script already uses forVACUUM— no new dependency. True orphans are still removed, so defrag keeps its compaction value.Evidence: L2 (unit tests incl. a real
sqlite3CLI exercising the livesegmentsread + a negative-mutation orphan-cleanup assertion) → L3 ideal for AC2 (a fullai:defrag-kbrun against a live multi-collection store + daemon restart). Residual: AC2 end-to-end, deferred to manual verification / the#12138recycle exercise. Residual: AC2 [#12138].Implementation
newCollectionIdskeep-set (collection ids never match on-disk segment-id dirs).cleanOldBackupsseam pattern:resolveLiveSegmentIds({dbPath, execFn})→Set<string>of live segment ids; returns an empty Set (without shelling out) when nochroma.sqlite3is present.cleanOrphanedSegmentDirs({dbPath, liveSegmentIds, fsModule, log})→{kept, removed}; deletes a UUID dir iff its name is absent from the live-segment set, preserving every live segment dir (all collections) and any non-UUID / non-directory entry.resolveLiveSegmentIdsand delegate physical removal tocleanOrphanedSegmentDirs.AC mapping
segmentsregistry — ✅resolveLiveSegmentIds.Deltas from ticket
Beyond AC5's minimum seed set, the spec also asserts: a 36-char-no-hyphen dir survives (exercises the second half of the UUID heuristic), a UUID-named file is skipped (
isDirectoryguard), andresolveLiveSegmentIdsshort-circuits without shelling out when no sqlite is present. Added a guarded real-sqlite3integration test (skips if the CLI is unavailable) so the productionSELECTruns against the actual CLI output shape rather than a stub only.Test Evidence
The real-
sqlite3integration test executed (not skipped) in this environment.Post-Merge Validation
ai:defrag-kbagainst the live unified store, then restart the Chroma daemon and confirm both KB and MC vector queries still return results (the AC2 L3 residual; will also be exercised by the#12138recycle chain).