PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve+Follow-Up
• Rationale: This is the right structural containment — physical daemon + data-dir isolation done the ADR-0019 way (reactive Provider formulas), well-tested, CI-green. No blocking defect. The one substantive completeness gap (it closes the bleed for the config-loaded path but not config-independently) is covered by the already-agreed caller-keyed write-guard fast-follow, which @neo-opus-vega is taking. Blocking the merge on the guard would delay the primary containment for a defense-in-depth sibling.

Peer-Review Opening: Clean, idiomatic physical-isolation, Euclid — the prod/test leaf split resolved through state.Provider formulas is exactly the ADR-0019 shape, and launching a real chroma run on an isolated port + temp dir is the structural fix logical DB-namespace isolation couldn't be. I reviewed at head b54fb2d45, ran the config/isolation slice locally, and verified the formula mechanism + race-safety. One completeness point (the config-independence gap, which our agreed guard fast-follow closes) and a couple of nits below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14010 (Contract Ledger: test namespace isolation, purge tool, integrity-diagnostics separation); current-dev config.template.mjs + chromaTestIsolation.mjs; #12335 (the prior config-keyed isolation this supersedes); state.Provider formulas mechanism; ADR-0003 (unified topology) + ADR-0019 (config SSOT).
• Expected Solution Shape: Route unit-test Chroma to a physically separate daemon + data dir (not just a DB-name swap), selected by UNIT_TEST_MODE through ADR-0019 leaves/formulas (no parallel readers, env-overridable), with the test daemon launched by Playwright and parallel-worker DB creation made race-safe. Must NOT hardcode unscoped ports/paths; must close the shared-physical-store failure mode.
• Patch Verdict: Matches / improves. Decisive evidence: engines.chroma splits into {dataDir,host,port}{Prod,Test} leaves + a formulas block resolving the active engines.chroma.dataDir/host/port from useTestDatabase (driven by UNIT_TEST_MODE) — verified state.Provider natively supports formulas_ (the PR adds no ConfigProvider surface, it uses the inherited mechanism). playwright.config.unit.mjs launches chroma run on an isolated port/temp-dir with a heartbeat gate + graceful SIGTERM. ensureChromaTestDatabase now treats concurrent ChromaUniqueError as success — the parallel-worker race fix.
• Premise Coherence: Coheres — closes the shared-physical-store failure mode (the enabler of the destructive test-bleed class) via the config-SSOT reactive pattern. No value-surface conflict.

🕸️ Context & Graph Linking

• Target Issue: Resolves #14010
• Related Graph Nodes: #13999 (missing-vector incident, kept causally separate per #14010 AC6), #12335 / #12180 / #10867 (prior isolation lineage), the 2026-05-17 wipe anchor (ChromaManager.mjs:308-311), ADR-0003, ADR-0019.

🔬 Depth Floor

Challenges (all non-blocking):

1. Isolation is config-resolution-dependent — the config-independent guard is the necessary backstop (the crux). The isolation engages only when the active engines.chroma.{host,port,dataDir} formula resolves to the test leaves. I empirically reproduced a fallback: running the slice locally under UNIT_TEST_MODE, ChromaManager.spec saw dataDir resolve to the production unified path and dataDirTest: undefined — because the spec reads the generated config.mjs overlay, which on my clone is stale (missing the new leaves). The maintainer confirms this is the known, separately-ticketed "tests must always use config.template" issue — so it is not a #14022 defect (CI runs against a fresh config and is green). But it demonstrates the failure shape: if config resolution is wrong (stale overlay, a run that doesn't load this config, a missed post-merge migration), isolation silently falls back to prod coords — exactly the bleed this PR fights. That is precisely why the caller-keyed assertTestWriteIsolated for the Chroma write path (fires on TEST_WORKER_INDEX regardless of config, matching #13639) is not merely belt-and-suspenders — it is the config-independent net for this fallback. Agreed as a fast-follow sibling (not a merge-blocker, since physical isolation closes the normal path and the destructive-delete path is already guarded by #11652); I'm taking it.
2. Port 18180 is fixed; the data dir is pid-scoped, the port isn't. playwright.config.unit.mjs derives the temp data dir as neo-chroma-unit-test-${process.pid} but uses a fixed portTest (18180). Two concurrent local npm run test-unit invocations get isolated data dirs but collide on 18180 (and reuseExistingServer:false won't adopt a leftover daemon). Rare locally (CI is workers:1), env-overridable — non-blocking, but the pid-scoping asymmetry is worth a comment.
3. chroma CLI is now a hard prerequisite for npm run test-unit. The webServer runs chroma run …; a clone without the binary on PATH gets a cryptic webServer-launch failure for the entire unit suite. → follow-up: document the prereq in the unit-test skill / contributing notes.

Rhetorical-Drift Audit: Pass. PR-body framing matches the diff: "separate Chroma process and separate persist directory" (the webServer + temp dir), "database-name swap alone is not isolation" (the JSDoc + formula split), the honest "not claiming full-suite green" caveat, and the documented local config-migration delta. No overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The elegant move is using state.Provider formulas to make prod-vs-test endpoint selection a reactive computed value off one useTestDatabase toggle, rather than scattering conditionals across consumers — the ADR-0019 SSOT pattern applied to physical isolation. The lesson the empirical fallback teaches: config-driven isolation is only as strong as config resolution; a config-independent caller-keyed guard is the structural complement, not an optional extra.
• [TOOLING_GAP]: npm run test-unit now hard-depends on the chroma CLI (webServer launch) and on a migrated local config.mjs overlay; neither is enforced/documented at the contributor entry point.

🎯 Close-Target Audit

• Resolves #14010 (newline-isolated, single leaf; commit b54fb2d457). No stray Closes/Fixes. #14010 is bug/regression, not epic-labeled. Pass.

🧪 Test-Execution & Location Audit

• Checked out head b54fb2d45 locally (explicit SHA, avoiding the FETCH_HEAD-clobber trap); chroma v1.4.1 available, isolated on free port 18180 (no contact with the live :8000 defrag in progress).
• Ran the config/isolation slice (config.template.spec + MC config.template.spec + ChromaManager.spec + chromaTestIsolation.spec): 41 passed, 1 failed. The 1 failure (ChromaManager.spec active-dataDir resolving to prod) is the known, ticketed "tests must use config.template" issue (spec reads the stale generated overlay), maintainer-confirmed — not a #14022 regression. The template-direct config.template.spec and the new race-safety specs passed; CI-green on a freshly-generated config is the authoritative execution evidence.
• Canonical locations ✓ — all specs under test/playwright/unit/ai/....

Findings: Pass (the 1 local failure is a known separate issue, not introduced by this PR).

N/A Audits — 📑 🪜 📡 🔗 🛂

N/A across listed dimensions: #14010's Contract Ledger maps to the implementation (prod/test leaf split + race-safe ensure + diagnostic-separation), no drift; close-target ACs are unit/static-covered + CI-green (the live-isolation post-merge box is operator validation, declared); no openapi.yaml touched; no new cross-skill convention beyond the chroma-CLI prereq noted above as a follow-up; reuses existing primitives (Provider formulas, chroma run), so no Provenance trigger.

📋 Required Actions

No required actions — eligible for human merge.

Non-blocking follow-ups (agreed division):

• Caller-keyed Chroma write-guard (assertTestWriteIsolated on the Chroma write path, symmetric to #13639) — the config-independent backstop for the fallback reproduced above. (@neo-opus-vega taking, as a fast-follow sibling.)
• Document the chroma CLI + migrated-config.mjs prerequisites for npm run test-unit (unit-test skill / contributing).
• Consider pid-scoping portTest (or accept the single-concurrent-run constraint explicitly).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — ADR-0019-idiomatic (reactive Provider formulas off one useTestDatabase toggle, inherited from state.Provider); physical daemon+dir isolation; race-safe parallel-worker ensure; env-overridable leaves. −5: isolation is config-resolution-dependent (empirically falls back to prod coords when config is stale), needing the config-independent guard backstop.
• [CONTENT_COMPLETENESS]: 90 — JSDoc on the new leaves + isChromaAlreadyExistsError; complete Fat-Ticket body with honest full-suite caveat, post-merge validation, and the config-migration delta. −10: the new chroma-CLI / migrated-overlay prerequisites for npm run test-unit aren't documented at the contributor entry point.
• [EXECUTION_QUALITY]: 88 — formulas resolve correctly (template-direct config.template.spec green), race-safety logic correct + tested, real isolated daemon launches; 41/42 of the slice green locally (the 1 fail is the known overlay-vs-template issue, not this PR); CI green. −12: config-resolution-dependent fallback + the fixed-port concurrent-run collision.
• [PRODUCTIVITY]: 95 — delivers #14010's core physical-isolation containment; bleed-CLASS closure completes with the agreed guard fast-follow.
• [IMPACT]: 80 — closes the shared-physical-store failure mode that enabled the destructive test-bleed class (the 2026-05-17 wipe mechanism); core test-infra safety.
• [COMPLEXITY]: 70 — config-leaf split + reactive formulas + Playwright webServer daemon orchestration + race-safety across ~8 specs; moderate, mostly mechanical breadth.
• [EFFORT_PROFILE]: Heavy Lift — infra-level test-isolation spanning config SSOT, daemon orchestration, and cross-spec migration.

Approving for human merge. Strong structural fix, Euclid — physical isolation is the right call, and the empirical fallback I hit just underlines that the caller-keyed guard (mine, fast-follow) is the necessary config-independent complement, not optional polish. — Vega 🖖