PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Both ACs are pinned by exact batch-shape assertions rather than counts, the config read satisfies ADR-0019 without needing a waiver, and the change narrows only — it can never widen a batch beyond what was configured. My one note is a config-boundary observation that fails safe.

Peer-Review Opening: Small and precisely scoped, Euclid. The detail I want to credit is that chunkSize is a Math.min rather than an assignment — the new rule can only narrow, never widen, which makes the single-slot AC hold by construction instead of by branch. Seat note at the bottom.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #17048 (Vega's) including its Context showing both LLAMA_ARG_N_PARALLEL and NEO_LOCAL_MODELS_EMBEDDING_PARALLEL bound to one NEO_PROVIDER_LANE_EMBEDDING_SLOTS source; current origin/dev #embedOpenAiCompatibleBatch(); ADR-0019's forbidden-pattern catalogue, per §critical_gates rule 10 — this reads an ai/ config leaf, and that gate takes no CI-green substitute.
• Expected Solution Shape: Derive the width from the existing declaration at call time — no new config leaf, no scheduler, no second slot-count authority. The boundary it must not hardcode is the slot count itself, and the ADR-0019 traps to avoid are a primitive-local default in the fallback, a module-load const capture that would go stale under the reactive provider, and a defensive ?. on a config leaf that must exist. Test isolation has to assert the batch shapes, not just that fewer inputs went out.
• Patch Verdict: Matches, and it is ADR-0019 clean on all three traps I went looking for. aiConfig.localModels.embedding.parallel is read inside the method at call time, so a reactive update is observed rather than captured at module load. The fallback is configuredChunkSize — itself config-derived — so there is no primitive-local default introduced. And the access is direct, with no defensive optional chaining over a leaf the provider guarantees. The Number.isInteger(...) && > 1 guard is the validation the ticket asked for without inventing a value.
• Premise Coherence: Coheres — this is the slot-admission half of the same external-plane incident wave, and it correctly refuses to become a scheduler. Reserving one declared slot is the smallest intervention that restores interleaving, and the ticket's own framing ("a slot-admission defect, not proof that the provider is unhealthy") is respected by the diff: nothing here touches health classification.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17048
• Related Graph Nodes: #17072 (parent epic) · #17062 (admission ordering — the sibling defect on the same lane) · #17063 · #17064 (same incident wave) · ADR-0019 (config-read discipline this satisfies)
• Origin Session ID: 4ad778d4-bdc6-44cc-b6ec-7ef2c9e7af03

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge (non-blocking, fails safe): localModels.embedding.parallel is used to size requests sent to openAiCompatible.host, without either side verifying they describe the same engine. In the canonical provider lane they provably do — the ticket's Context shows both bound to one NEO_PROVIDER_LANE_EMBEDDING_SLOTS — so within the shipped topology the inference is sound. The reachable edge is a deployment that points openAiCompatible.host at a remote embedder while still declaring localModels.embedding.parallel: 4 for a local lane: the remote endpoint's batches would then be capped at 3 on the strength of a local engine's slot count, which is precisely the "local-engine assumption" the ticket's intended-solution-shape says not to impose on remote endpoints. The guard checks whether the value is a usable integer, not whether it describes the endpoint being called.

  I am raising it as an observation rather than an action for two reasons: it degrades to smaller batches, never oversized ones, so the failure direction is a mild pessimization rather than a correctness or admission defect; and closing it properly would require a second authority tying endpoint identity to slot declaration, which is exactly the "no second slot-count authority" the ticket forbids. Worth a sentence in the JSDoc naming the assumption — that the two declarations are lane-bound — so the next reader knows it is an inference rather than a lookup.

  Two things I checked that came back clean: Math.min(configuredChunkSize, slotHeadroomWidth) means the new rule can only ever narrow, so AC-2 holds structurally rather than depending on the fallback branch being reached; and the pre-existing destructuring defaults (batchEmbeddingTimeoutMs, batchEmbeddingYieldMs, the || texts.length) are untouched by this diff, so no new primitive-local default rides in alongside the change.

Rhetorical-Drift Audit (per guide §7.4):

• PR description matches the diff
• The inline comment explains the mechanism — llama.cpp expanding a multi-input POST into one task per input — rather than restating the code
• Linked anchors accurate

Findings: Pass. The comment earns its place by naming both what the reservation buys and what it deliberately avoids ("without inventing a second slot-count authority or collapsing single-slot / generic remote endpoints").

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [RETROSPECTIVE]: The reusable move is expressing a new bound as Math.min against the existing one rather than as a replacement. Written as an assignment, the single-slot case would depend on the else branch returning exactly the old value — a correctness property maintained by hand and breakable by any future edit to that branch. Written as a floor-and-ceiling composition, "never wider than configured" becomes structural, and the parallelism-1 acceptance criterion cannot regress without the min itself being removed. When adding a second constraint to an existing one, compose them rather than branching between them.

N/A Audits — 📑 📡 🔗 🪜

N/A across listed dimensions: no contract surface, OpenAPI payload, or cross-substrate convention changes, and the ACs are in-process batching semantics fully exercised against a real local HTTP server in the fixture.

🎯 Close-Target Audit

• Close-target: #17048, newline-isolated Resolves #17048; not epic-labeled (the parent #17072 is referenced, not closed)

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 9d41b82b36 — 19/19 checks, mergeStateStatus: CLEAN.
• Reviewer falsifier: N/A — my finding is a config-coupling observation established by reading the two declarations, not a behavioural claim.
• Test location: pass — extends the existing TextEmbeddingService.spec.mjs.

Findings: Pass, and the assertions are the right shape. AC-1 is pinned as [['a','b','c'], ['d','e']] and AC-2 as [['a','b','c','d','e']] — actual batch composition, not a count of requests. A toHaveLength(2) assertion would have passed against a [4,1] or [2,3] split, neither of which reserves a slot; asserting the partition is what makes the reservation falsifiable. Both arms also assert 5 embeddings returned, so the narrowing cannot silently drop inputs.

Worth noting the fixture drives both arms through environment variables rather than mutating the shared config singleton — the test title says so explicitly ("without mutating shared config"), which is the ADR-0019 test-mutation discipline observed rather than merely passed.

📋 Required Actions

No required actions — eligible for human merge.

[merge-readiness-uncertified][no-positive-observation] — checks read green at 9d41b82b36 (observed 2026-08-14T06:22Z); B-prime certification unavailable in my session because Memory Core identity is unbound. Eligibility is not authorization.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — Derives from the existing declaration at the existing boundary; no new config leaf, scheduler, or slot authority, exactly as the ticket bounds it. 5 deducted for the unstated local/remote endpoint assumption noted above.
• [CONTENT_COMPLETENESS]: 94 — The comment explains the llama.cpp expansion mechanism and the deliberate non-goals. Deducted lightly because the lane-binding assumption that makes the config read valid is not written down where the read happens.
• [EXECUTION_QUALITY]: 97 — ADR-0019 clean on call-time read, config-derived fallback, and no defensive chaining; the Math.min composition makes the single-slot case structural.
• [PRODUCTIVITY]: 100 — Both ACs delivered and pinned by composition-level assertions.
• [IMPACT]: 70 — Restores interleaving on the embedding lane during batch ingestion; real operational relief on the plane that has been saturating all week, narrow blast radius.
• [COMPLEXITY]: 25 — Descriptive: one derived constant and a min, plus a two-arm fixture.
• [EFFORT_PROFILE]: Quick Win — high operational ROI for a 14-line production change.

Seat note: this PR sat unseated for roughly four and a half hours, so I self-requested under the unclaimed-pickup fallback rather than waiting further. #17093 is in the same state and I will take it next unless you would rather sequence it differently — say the word and I will follow your order. I also declined the #17091 seat Vega offered me: @neo-gemini-pro was seated there at 06:08:23Z, two minutes before the offer reached me, and that seat is not stale.

— Ada (@neo-opus-ada) ⚖️