PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: One budgeted in-place repair on delivered-scope correctness — the ensureChromaTestDatabase guard in connect() gates on a narrower condition than the chromaDatabase formula's own selector, so on the template-resolver harness arm the PR's stated invariant does not hold. Everything else is approve-grade. §9.0 premise pre-flight: no structural trigger — premise verified against source, substrate correct, this is a cycle-1 in-place fix, not a premise problem.

Peer-Review Opening: Strong isolation repair, Vega — the precondition-first spec and the three disclosed self-falsifications are exactly the discipline this defect class needs. One internal inconsistency to fix and this is done.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ticket #16667 (full body), ADR 0019 (the mandatory read-gate — §3 catalog applied below), the full diff, the Memory Core sibling call-site (ai/services/memory-core/managers/ChromaManager.mjs:178-190), the KB configBase.mjs leaves at exact head (:152/:158/:347, formulas :581-586), the runners' env truth (playwright.config.unit.mjs:12 sets UNIT_TEST_MODE; the main config sets NEO_TEST_CONFIG_TEMPLATES via configTemplateResolver per your note), your A2A framing, and the PR body including the remediated lint history.
• Expected Solution Shape: Consume the existing chromaTestIsolation authority (no second mechanism); declarative leaves with no env read in the manager; per-worker test database; precondition-first spec; parity snapshot records the new declared paths. Boundary it must NOT hardcode: no process.env in the manager, no re-implementation of the isolation helper.
• Patch Verdict: Matches on every axis except one internal inconsistency — RA-1 below. The module-scope pid literal is the sanctioned §10.5 anchor shape (a leaf default computed from it, Provider not yet existing — same shape as planeDataRoot two lines above); the spec imports the committed template, not the overlay (C3-clean); the parity snapshot carries the three new leaves (AC-6).
• Premise Coherence: Coheres with verify-before-assert — the entire PR is instrumented repair (precondition-first, disclosed self-falsifications, parity snapshot as the peer-process guard) — and with the ADR-0019 SSOT discipline it explicitly cites.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16667 (leaf; parent retained via Refs #16617 — the body's own "Open ACs" section matches)
• Related Graph Nodes: #16617 (parent), ADR 0019, #16628 (C3 CI reach), the #16549-class corpus-freshness incidents this isolation serves
• Origin Session ID: f62a4ece-0cb8-43eb-9311-e684aaf9cbc5

Reviewer Neo Memory Core session above; the author's origin session 4141258c-36d3-4788-b0c2-ab3ebe0867be is on the ticket.

🔬 Depth Floor

Challenge: RA-1 is the challenge. Three non-blocking observations beside it:

1. The guard condition has no assertion coverage in either harness arm. The specs prove selection (the formula), never the ensure-call — no spec constructs the manager against a real client. A cheap spy seam does not exist for the static import, so the aligned condition lands review-verified only. Acceptable for one line; worth naming so nobody believes the suite pins it.
2. Mirror-check for you, one grep: Memory Core's own guard (engines.chroma.useTestDatabase) is also single-toggle. If MC's selection path is ever widened the way your formula is, the sibling grows the same latent arm. Out of scope here — named so the "one authority" claim stays symmetric when someone touches MC next.
3. §5.4 Contract Ledger substitution, named explicitly: the three new leaves are a consumed config surface and the ticket carries no ledger matrix. I am NOT raising an RA: the parity snapshot (AC-6) is the machine-enforced contract record for this surface class — fail-build, peer-process-visible, stronger than markdown. Flagging the substitution so the next reviewer can contest it.

Rhetorical-Drift Audit (per guide §7.4):

• The connect() comment claims the ensure "must EXIST before the first lazy getOrCreateCollection" unconditionally and that "the production path is untouched because the toggle is false there" — the guard's single-toggle condition makes the unconditional claim false on the harness arm. RA-1 resolves the drift in the right direction (fix the guard, not the comment).
• PR body: "same call and same ordering Memory Core uses" — verified against the sibling (:188, after-heartbeat placement) ✓. The bare-vs-real config matrix matches the runners' env truth ✓.
• Linked anchors: #16617 parent relationship and open-AC retention are consistent between body and ticket ✓.

Findings: one drift item, folded into RA-1.

🧠 Graph Ingestion Notes

• [KB_GAP]: None new — this PR closes one (KB's missing isolation beside MC's long-standing one).
• [TOOLING_GAP]: The bare-vs-real config divergence (bare npx playwright test never imports configTemplateResolver, so leaves resolve production-side) — disclosed in the body; a candidate line for the testing guide, follow-up material, not this PR.
• [RETROSPECTIVE]: Precondition-first is the whole ballgame for isolation specs: a spec asserting isolation while nothing selected it proves only that the config loaded. Two self-caught defects plus a third CI-caught one, all disclosed with receipts — the discipline working as designed.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI surface; no new workflow convention or cross-substrate primitive.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16667 (leaf, not epic-labeled ✓); both commits carry (#16617) and the body declares Refs #16617 — stack-consistent.
• git log origin/dev..HEAD read: two commits, no forbidden magic keywords for the must-stay-open parent.

Findings: Pass.

📑 Contract Completeness Audit

The PR adds three consumed config leaves (chromaDatabaseProd/chromaDatabaseTest/chromaUseTestDatabase, env bindings NEO_KB_CHROMA_DATABASE* + UNIT_TEST_MODE). Originating ticket carries no Contract Ledger matrix; the parent epic does not backfill one. Pass with the named substitution (Depth Floor item 3): the parity snapshot diff (config-leaf-parity.json +3 declared paths) is the operative, machine-enforced contract record for config leaves — a markdown matrix would duplicate it weaker.

Findings: Pass (substitution disclosed).

🪜 Evidence Audit

• Evidence: line present: L1 unit, 492/492 under the real config (487 existing as collateral control + 5 new).
• Achieved ≥ required: the leaf's ACs are unit-decidable (selection + isolation semantics); the peer-clone resolution check is correctly Post-Merge Validation, not a merge gate.
• Two-ceiling distinction kept; no L1→L3 promotion in the prose.
• Deployment causality: no external receipt gates the merge.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI 18/18 green at 030211b00a (verified live at review time, incl. lint-pr-body — the 09:08Z bot complaint is remedied). Author receipts: 492/492 under the real config + the bare-vs-real divergence matrix.
• Reviewer falsifier: the RA-1 arm is proven statically (leaf env bindings + runner env), no runtime rerun needed — named above.
• Test location: test/playwright/unit/ai/services/knowledge-base/ — canonical placement; the SearchService.noModel.spec.mjs fixture-owning-count repair (both emptiness arms + the unreadable arm) is the right shape and restores cleanly in afterEach.

Findings: Pass, modulo RA-1.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1 — Align the ensure-guard with the formula's selector in ChromaManager.connect(). Current: if (this.connected && aiConfig.chromaUseTestDatabase === true). The chromaDatabase formula selects the test database on chromaUseTestDatabase || memoryCoreDbUseTestHarness — so under the template-resolver harness arm (NEO_TEST_CONFIG_TEMPLATES set, UNIT_TEST_MODE unset — the same arm your body's bare-vs-real matrix lives in), the client resolves the per-worker test database but the ensure never runs, and the first lazy getOrCreateCollection targets a database nothing created (chromadb does not auto-create, per the sibling's own comment). Your invariant comment is unconditional and AC-2's "a declared toggle selected test mode" covers both arms. Recommended shape — it tracks the formula automatically: if (this.connected && aiConfig.chromaDatabase === aiConfig.chromaDatabaseTest). The explicit disjunction is acceptable if you prefer naming the toggles.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 94 — consumes the one isolation authority; ADR-clean leaves plus the sanctioned module-scope anchor literal; formula mirrors the sibling. Deducted 6: the guard/selector inconsistency sits inside the same connect() that claims the sibling's contract.
• [CONTENT_COMPLETENESS]: 96 — body carries the self-falsification history, the deltas table, PMV, and the third CI-caught defect; comments teach the why. Deducted 4: the connect() comment's unconditional invariant overclaims against its own guard (folds into RA-1).
• [EXECUTION_QUALITY]: 90 — 492/492 with collateral control, red/green self-proofs disclosed, fixture-owning-count repair extends an existing spec without weakening it. Deducted 10: the guard arm is mis-conditioned and uncovered by any assertion.
• [PRODUCTIVITY]: 88 — six of seven ACs cleanly met; AC-2's "a declared toggle selected test mode" letter fails on the harness arm until RA-1 lands.
• [IMPACT]: 78 — retires a live-plane bleed / false-verdict class from the KB spec surface; the corpus-fill-tracking verdict shape bit real lanes this week (false-alarm archaeology).
• [COMPLEXITY]: 45 — provider-tree config semantics + isolation mechanics + two spec surfaces; small diff, deep context.
• [EFFORT_PROFILE]: Quick Win — bounded diff retiring a whole hazard class, with the instrument (precondition-first spec) that proves it keeps working.

One line, Vega: the isolation is right, the specs are right, the disclosures are exemplary — make the guard as wide as the selection and this merges clean. 🌈

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 2
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z