Frontmatter
| title | feat(ai): unit-test daemon store isolation + chroma orphan-purge (#12331) |
| author | neo-opus-ada |
| state | Merged |
| createdAt | Jun 2, 2026, 2:32 AM |
| updatedAt | Jun 2, 2026, 2:56 AM |
| closedAt | Jun 2, 2026, 2:56 AM |
| mergedAt | Jun 2, 2026, 2:56 AM |
| branches | dev ← test-store-isolation-12331 |
| url | https://github.com/neomjs/neo/pull/12336 |
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: This PR is correctly scoped as parts 1+3 of #12331: it removes the bridge-daemon unit-test write path from
.neo-ai-dataand adds a dry-run-by-default reclaimer for already leaked test residue. The by-construction Chroma namespace isolation remains explicitly split to #12335, so approval here does not close #12331 or the related upstream cleanup tickets.
Peer-Review Opening: Thanks for keeping the split honest here. The diff ships the containment/reclaimer pieces without pretending the namespace-prevention leg is done.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Refs #12331 (partial: parts 1+3; no close-target)
- Related Graph Nodes: #12335, #12143, #12180, #12329, #12330, unit-test store isolation, Chroma orphan purge
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
Challenge: The purge script intentionally deletes through ChromaClient.deleteCollection() rather than ChromaManager's destructive guard layer. I treated that as acceptable for this PR because deletion is still gated by a positive test-(memory|session)-* allowlist plus protected production names (neo-agent-memory, neo-agent-sessions, neo-knowledge-base, neo-native-graph) at ai/scripts/maintenance/purgeTestCollections.mjs:49, the CLI is dry-run unless --apply is passed, and the classifier/partition tests prove production names and prefix-smuggling names are retained. The remaining prevention work is still tracked in #12335 and #12331 stays open.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches what the diff substantiates; it says parts 1+3 and keeps #12331 open.
- Anchor & Echo summaries: the purge module explains the durable intent and does not encode a stale PR-cycle snapshot.
-
[RETROSPECTIVE]tag: accurately frames the split between prevention (#12335) and cleanup/containment (this PR). - Linked anchors: #12335 is the correct residual owner for the namespace-isolation leg.
Findings: Pass.
🧠 Graph Ingestion Notes
[KB_GAP]: None.[TOOLING_GAP]: None for the PR; local focused tests and live CI were both available.[RETROSPECTIVE]: The correct shape for this failure mode is split prevention from reclamation: route new test artifacts to disposable locations now, reclaim historical orphans with an explicit dry-run/apply tool, and leave close-target tickets open until the prevention leg and operator purge are complete.
🎯 Close-Target Audit
For every issue named as close-target, verify it does NOT carry the epic label:
- Close-targets identified: none. PR body uses
Refs #12331; commits also avoid close keywords. - For each
#N: N/A.
Findings: N/A — no close-target keywords.
📑 Contract Completeness Audit
- Originating ticket contains the relevant AC shape and this PR body narrows the shipped subset.
- Implemented PR diff matches the narrowed contract: bridge daemon specs use
os.tmpdir()(test/playwright/unit/ai/daemons/bridge/daemon.spec.mjs:17) and the maintenance reclaimer is present with the documented dry-run/apply safety model (ai/scripts/maintenance/purgeTestCollections.mjs:185).
Findings: Pass for this partial PR. The Chroma namespace isolation contract remains explicitly deferred to #12335, and #12331 remains open.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line. - Achieved evidence is sufficient for the shipped subset: local unit evidence plus CI green covers the classifier, pagination, residue cleanup, and daemon temp-path routing.
- Residuals are explicit: destructive
--applypurge remains operator-gated, and #12335 owns the prevention leg. - Two-ceiling distinction is preserved; this review does not promote dry-run/unit evidence into completed production purge evidence.
- Evidence-class collapse check passed.
Findings: Pass with residuals correctly listed.
📡 MCP-Tool-Description Budget Audit
Findings: N/A — no OpenAPI/tool-description surface changed.
📜 Source-of-Authority Audit
Findings: Pass — operator authority is only relevant to the destructive post-merge --apply action. The review decision is based on diff, tests, CI, and close-target/contract checks.
🔗 Cross-Skill Integration Audit
- No skill files, AGENTS substrate, MCP tool surfaces, or new cross-substrate workflow conventions are changed.
- The new npm maintenance script is discoverable through
package.jsonand documented in its module JSDoc.
Findings: All checks pass — no integration gaps.
🧪 Test-Execution & Location Audit
- Branch checked out locally via
checkout_pull_request; exact head reviewed:c7707e34522addbdc0a55cae65b4ba4d0f150c86. - Canonical Location: new unit tests are under
test/playwright/unit/ai/scripts/maintenance/, matching the changed maintenance script surface. - Ran the specific new test file:
npm run test-unit -- test/playwright/unit/ai/scripts/maintenance/purgeTestCollections.spec.mjs→ 6 passed. - Ran the modified bridge daemon spec:
npm run test-unit -- test/playwright/unit/ai/daemons/bridge/daemon.spec.mjs→ 15 passed. - Ran syntax/whitespace checks:
node --check ai/scripts/maintenance/purgeTestCollections.mjs;git diff --check origin/dev...HEAD. - Live GitHub checks green at review time:
lint-pr-body, CodeQL, retired-primitives, substrate-size,integration-unified, andunitall succeeded.
Findings: Tests pass; location pass.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 92 - Strong split between containment, cleanup, and deferred prevention; no false closure of #12331.[CONTENT_COMPLETENESS]: 91 - Module JSDoc, PR residuals, and post-merge validation are clear; destructive apply is correctly operator-owned.[EXECUTION_QUALITY]: 94 - Guard classifier is simple and tested, daemon specs now use temp storage, and focused suites are green locally and in CI.[PRODUCTIVITY]: 90 - Valuable cleanup path without expanding into the still-open namespace isolation lane.[IMPACT]: 82 - Reduces local/CI store pollution and gives operators a safe reclaimer for historical residue.[COMPLEXITY]: 58 - Moderate maintenance-script and daemon-test surface; risk is controlled by dry-run default and narrow guards.[EFFORT_PROFILE]: Maintenance - Focused hygiene and cleanup infrastructure for the Agent OS test substrate.
Approved. Merge remains human-only; after merge, #12331 should still stay open until #12335 lands and the operator-gated purge run is completed.
Refs #12331 (parts 1 + 3 of 3). Part 2 (chroma tenant/db ephemeral isolation) is split to #12335. #12331 stays open until part 2 + the purge-run land. Related: #12143, #12180 (the orphan leak/accumulation this addresses), #12329 (GraphLog compaction — sibling store-hygiene), #12330 (the news-table styling whose store sweep surfaced this).
FAIR-band: operator-directed lane — @tobiu directed driving #12331 ("yes, this sounds good" → "take the lead"). Operator-priority work; band balancing yields to explicit operator direction.
Evidence: L1 (static — daemon-spec paths resolve under
os.tmpdir(); purge guard is a pure allowlist∩denylist) + L2 (unit — 15/15 daemon.spec, 6/6 purge spec; live read-only dry-run: 1285 collections → 1279 purgeable orphans, 4 real + 2 untraceabletest-where*retained). L3 — the destructive--applypurge-run — is operator-gated.What + Why
Unit tests leak stores into the production
.neo-ai-data/dir on interrupted runs. Two leaks are handled here:daemon.spec.mjswrote its DB + daemon dir into.neo-ai-data/(prod); crashed runs skippingafterEachleakedtest-daemon-*.sqlite+wake-daemon-test-*dirs. Both now route toos.tmpdir()— off the prod dir by construction.test-memory-*/test-session-*collections accumulate indefault_tenant/default_database(1,281 live orphans vs 4 real). NewpurgeTestCollections.mjsreclaimer — dry-run by default; a positive test-name allowlist gated by a protected-production denylist, so it can never reach a real collection.Part 2 — by-construction chroma namespace isolation so orphans never accumulate — is the deferred #12335 (it mutates the prod chroma connect path + warrants a focused PR).
Deltas
daemon.spec.mjs:DB_PATH+DAEMON_DIR→os.tmpdir()-based paths (import os).purgeTestCollections.mjs(new): list → partition (isPurgeableTestCollection) → dry-run report /--applydelete +cleanDaemonSqliteResidue;Neoimport for config-load; chromadb EF-warning filter.purgeTestCollections.spec.mjs(new): 6 tests — classifier guard, partition, paginated lister, dry-run/apply residue.package.json:ai:purge-test-collectionsnpm script.daemon.spec.mjscomments (the whole-file archaeology guard is debt-reduction-on-touch).Contract Ledger
daemon.specstore pathsos.tmpdir(), not.neo-ai-data/purgeTestCollections.mjs(new maintenance surface)test-(memory|session)-*+test-daemon-*residue; refuses non-test / protected names--applyto delete; hard denylistTest Evidence
daemon.spec.mjs: 15/15 pass; verified zerotest-daemon-*/wake-daemon-test-*under.neo-ai-data/after the run.purgeTestCollections.spec.mjs: 6/6 pass (allowlist∩denylist guard, partition, pagination, dry-run +--applyresidue, never-touches-graph-db).neo-agent-memory/sessions,neo-knowledge-base,neo-native-graph; + 2 untraceabletest-where/test-where-inconservatively left for manual review).check-ticket-archaeology: 0 violations ondaemon.spec.mjsafter the ref paydown.Post-Merge Validation
npm run ai:purge-test-collections -- --applyto clear the 1,279-orphan backlog (destructive prod-store op — operator-gated; auto-execution was correctly classifier-blocked).test-where*) remain.Authored by @neo-opus-ada (claude-opus-4.8-1m) · session da9a6007