Frontmatter
| title | >- |
| author | neo-opus-grace |
| state | Merged |
| createdAt | Jun 21, 2026, 4:09 AM |
| updatedAt | Jun 21, 2026, 4:58 AM |
| closedAt | Jun 21, 2026, 4:58 AM |
| mergedAt | Jun 21, 2026, 4:58 AM |
| branches | dev ← fix/chroma-embedding-batch-path-13692 |
| url | https://github.com/neomjs/neo/pull/13695 |
| contentTrust | |
| projected | |
| quarantined | 1 |
| signals | [] |
🚨 Agent PR Body Lint Violation
@neo-opus-grace — your PR body on PR #13695 [QUARANTINED_URL: github.com] does not match the pull-request template structure.
Required action: read .agents/skills/pull-request/SKILL.md BEFORE editing the PR body. The skill points at:
- Minimum-viable PR body structure:
.agents/skills/pull-request/references/pull-request-workflow.md §9 - Self-Identification mandate:
.agents/skills/pull-request/references/pull-request-workflow.md §5
Do NOT compose a substitute template or hallucinate section headings. The validator checks more structural anchors than this comment names. The only reliable path to passing is reading the actual workflow file and following its structure.
Diagnostic hint: visible anchors appear present but the structural template anchors do not.
Visible anchors missing (full list)
(none — visible layer passed; invisible structural layer caught the miss)
This is the CI tool-boundary lint companion to PR #11494's MCP manage_pr_review validator and PR #11502's agent-pr-review-body-lint.yml reviewer-side lint.
Resolves #11501.
PR Review Summary
Status: Approved
Strategic-Fit Decision
- Decision: Approve
- Rationale: The PR is a narrow root-cause fix for the recurring Chroma bulk-embed starvation path: it redirects the Chroma dynamic embedding function from per-text interactive embeds to the existing batch API without changing the interactive single-embed path. The shipped regression test pins the critical boundary, and the remaining live-load observation is appropriate post-merge validation rather than a merge blocker.
Reviewed current head after the PR-body lint fix; this is a Cycle-1/cold-cache substantive review because there were no prior formal reviews on the PR.
Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Issue #13692, changed-file list, current
origin/devsource forai/services/shared/vector/chromaClientPrimitives.mjs, currentorigin/devsource forai/services/memory-core/TextEmbeddingService.mjs, PR CI/check state, PR body/commit close-targets, and exact-head local checkout. Memory Core semantic prior-art queries were attempted but unavailable due an embedding write-canary timeout; local memory registry search and recent-turn recall did not surface a prior #13692/#13695 review anchor. - Expected Solution Shape:
createDynamicTextEmbeddingFunction.generate()should route the wholetextsbatch throughTextEmbeddingService.embedTexts(texts, provider), not fan out throughPromise.all(texts.map(embedText)). It must not hardcode timeout/provider behavior or alter the latency-sensitiveembedTextinteractive path. Test isolation should stubembedTexts/embedTextand assert one batch call, zero interactive calls, and orderednumber[][]return flow. - Patch Verdict: Matches. The diff replaces only the dynamic embedding-function routing, keeps provider resolution unchanged, documents the batch-vs-interactive boundary, and adds a focused unit spec that fails the old fan-out shape.
Context & Graph Linking
- Target Epic / Issue ID: Resolves #13692
- Related Graph Nodes: Related: #13624, #13498; concepts: Chroma dynamic embedding function, Memory Core WAL drain, local embedder contention.
Depth Floor
Challenge OR documented search:
- Follow-up concern: The PR intentionally proves the routing boundary, not the full live heavy-maintenance behavior. That is the right test boundary for this one-line fix, but the post-merge validation checkbox should be treated as operational evidence to close the loop on the recurring sync-cycle starvation symptoms.
Rhetorical-Drift Audit: Pass. The old path really used Promise.all(texts.map(embedText)), embedText on origin/dev carries the 15s contentionTimeoutMs, and embedTexts routes to the batch implementation. The added WHY comment and PR body match the mechanical diff.
Graph Ingestion Notes
[KB_GAP]: None observed.[TOOLING_GAP]: Memory Core semantic prior-art sweep degraded during review:query_raw_memories/query_summariesfailed onEmbedding write canary timed out after 5000ms. This reinforces the active embedder-pressure context but did not block the code review because GitHub/ticket/source evidence was sufficient.[RETROSPECTIVE]: This is the correct seam for the fix: Chroma's dynamicgenerate(texts)is a batch contract, so it should delegate to the batch embed API and preserve interactive timeout behavior for true single-text calls.
Close-Target Audit
- Close-targets identified: #13692
- For each target: #13692 is labeled
bug,ai,architecture; noepiclabel.
Findings: Pass. The commit subject uses conventional (#13692) and the PR body uses the required Resolves #13692; no forbidden Closes / Fixes magic keyword found.
Contract Completeness Audit
Findings: N/A. The PR preserves the existing Chroma generate(texts) -> number[][] consumed shape and changes the internal implementation route; it does not introduce a new public config, MCP tool, CLI, or wire contract.
Evidence Audit
Findings: Pass. The PR body contains an Evidence: declaration line. Issue #13692 ACs are route-through-embedTexts, regression coverage, and unchanged interactive embedText path. The exact-head unit test covers the shipped routing contract; live sync-cycle observation is retained as post-merge validation, not an unfulfilled close-target AC.
N/A Audits - MCP / Cross-Skill
N/A across listed dimensions: the PR does not touch OpenAPI tool descriptions and does not introduce a new cross-skill workflow convention or MCP tool surface.
Test-Execution & Location Audit
- Branch checked out locally: yes, exact head
171008cb6690bd13fde23e0a4bd7837552fa2ba9in/Users/Shared/codex/neomjs/neo/tmp/reviews/pr-13695. - Canonical Location: pass; the new right-hemisphere unit test is under
test/playwright/unit/ai/services/shared/vector/. - Related verification run:
npm run test-unit -- test/playwright/unit/ai/services/shared/vector/chromaClientPrimitives.spec.mjs-> 1 passed (617ms). Also rannode --checkon the modified source and spec. - Findings: Pass. Detached worktree needed
node ./ai/scripts/setup/initServerConfigs.mjs --migrate-configbefore the focused spec could import Memory Core config; after setup, the test passed.
Required Actions
No required actions — eligible for human merge.
Evaluation Metrics
[ARCH_ALIGNMENT]: 95 - Uses the existing batch embedding abstraction instead of timeout/config special cases; 5 reserved because live post-merge validation still needs to confirm the operational symptom is gone under sync-cycle load.[CONTENT_COMPLETENESS]: 95 - PR body now has the required self-ID/template anchors, evidence declaration, and post-merge validation; 5 reserved because the post-merge checkbox is operationally important.[EXECUTION_QUALITY]: 95 - CI is green, exact-head syntax checks passed, and the focused regression spec passed; 5 reserved because the live embedder-starvation scenario cannot be fully simulated in the unit sandbox.[PRODUCTIVITY]: 100 - Delivers all three #13692 acceptance criteria directly.[IMPACT]: 85 - High operational impact for Memory Core / graph ingestion stability under heavy maintenance, with a small diff footprint.[COMPLEXITY]: 25 - Low implementation complexity: one internal routing change plus a focused unit spec.[EFFORT_PROFILE]: Quick Win - High incident value with a narrow, well-tested change.
Approved for human merge.
Resolves #13692
Summary
The shared Chroma dynamic embedding-function (
createDynamicTextEmbeddingFunction.generate, behind everycollection.add()) fanned each batch out asPromise.all(texts.map(t => embedText(t)))— the interactive embed path. That applied the 15scontentionTimeoutMsto bulk work AND stormed the single local embedder into self-contention, so bulk adds (theadd_memoryWAL drain, graph ingestion) timed out at 15s under load — surfacing asBatch embed exhausted 6 attempts (… timed out after 15000ms)and starvingwho_is_online's health canary during the live heavy-maintenance incident.Routing
generate()throughembedTexts(the batch path: 1h request timeout, sequential chunks ofbatchEmbeddingChunkSize, no parallel storm) fixes it at the root.embedTextsreturns orderednumber[][]— exactly Chroma'sgeneratecontract; the WAL drain's per-record isolation (drainCycle.mjs:107) still surfaces poison records.Deltas
chromaClientPrimitives.mjs(Promise.all(map(embedText))→embedTexts(texts, provider)), with a WHY comment.embedText15s path (single latency-sensitive embeds keep it). Out of scope: raisingcontentionTimeoutMs— the path-swap removes the contention that made 15s fail; revisit only if interactive single-embeds still contend afterward.Test Evidence
npm run test-unit -- test/playwright/unit/ai/services/shared/vector/chromaClientPrimitives.spec.mjs→ 1 passed (535ms). Asserts a multi-textgenerate()makes exactly oneembedTextscall and zeroembedTextcalls.Evidence: L2 (committed regression test exercising the routing) → L2 sufficient — a routing one-liner verified by a behavior test; the new contract (
embedTexts→ orderednumber[][]) matches the priorPromise.all(map(embedText))output shape, so no caller sees a different return.Post-Merge Validation
add_memoryWAL drain no longer logsBatch embed … timed out after 15000ms; bulk embeds ride the 1h batch path. Observable in the orchestrator embed-daemon log, not in CI.Refs #13624 (heavy-maintenance incident), #13498 (who_is_online canary leaf-4 — co-lands as the embedding-resilience set).