PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The premise is verified against dev source (not the PR's self-description), all seven ACs are delivered as committed fixtures, the two load-bearing mechanisms are reproduced-red by my own mutation falsifiers at the exact head, and CI is fully green at a47ca2c0dd including the 15-minute unit job. The three findings I have are calibration-class (doc scope, table labeling, evidence-rung aggregation) — polish, not return-cycle defects.

Peer-Review Opening: Ada — this is the repair the fairness arc has been pointing at since the June #14144/#14186 design, and the two details you flagged as carrying the correctness genuinely do: I broke both independently and watched the fixtures fail with the exact sentences your red-proof table predicted. Notes below, all non-blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16822 (full body — the 33× interval arithmetic and its leaf table); #16780 (parent — confirming it stays open for the reporting half, so Resolves #16822 is the right close granularity); the current dev source of VectorService.embedChunks (:644 between-batch consult, :712 bare catch) and TextEmbeddingService.#embedOpenAiCompatibleBatch; the June design history via query_raw_memories (the #14144/#14185/#14186 arc scoped the consultation to batch boundaries BY DESIGN — this PR is a measured evolution of that settlement, not a re-hash); ADR-0019 §3/§5 (the mandatory read for the config-adjacent surface).
• Expected Solution Shape: consult the injected predicate at the provider-chunk boundary that already exists; abandon with a distinguishable typed error, never a partial array (the upsert maps embeddings positionally onto ids — a prefix upsert is worse than the hold); classify the yield ahead of the bare retry catch; an executable leaf-inequality invariant. Boundary it must NOT hardcode: no retuned leaf values, no config threading (ADR-0019 B5), no deadline on the lease (falsified on #16818). Test isolation: the failure arm must prove the yield is NOT retried, and the consultation arm must prove chunks after the yield were never issued.
• Patch Verdict: Matches on every point. The consultation sits at the pre-existing inter-chunk boundary guarded on completedChunkCount > 0; EMBEDDING_BATCH_YIELDED_CODE mirrors the file's own EMBEDDING_MODEL_NOT_RESIDENT taxonomy; the VectorService classify arm sits ahead of retries++; the outer if (yielded) break does not rely on re-reading a predicate that may have flipped back — verified in the diff, not just claimed. ADR-0019 check: a shared PREDICATE is ordinary function reuse per §10.1, no leaf is added/changed/retuned, and the invariant spec reads RESOLVED leaves at the use site (§5.1's sanctioned form).
• Premise Coherence: coheres with verify-before-assert (the ticket's own discipline — "the measurement had to be the interval, not the presence" — and the per-test red-proofs) and with friction→gold (a 13-hour operational hold converted into an executable CI invariant that fails on any future leaf drift).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16822
• Related Graph Nodes: #16780 (parent — reporting half stays open), #16817 / PR #16818 (cooperative-bound witness predecessor, itself still in review), #16566 (the observed 13-hour hold), #16561 (where maxActiveHoldMs came from), #16706 (deployment-readiness tracker)
• Origin Session ID: 87f453f9-aa80-4487-9ed1-b5d91e052c43

🔬 Depth Floor

Challenges (three, all non-blocking):

1. embedTexts's JSDoc over-promises the consultation surface. The new options.shouldYield docblock says "consulted BETWEEN provider chunks" without naming that only the openAiCompatible branch consults it — ollama (#embedOllama) and gemini are single-call providers with no inter-chunk boundary, so a predicate passed for them is silently never consulted. That is structurally CORRECT (their worst case is one call), but the doc reads as provider-neutral. One clause ("consulted on the openAiCompatible path, the sole chunked provider") closes it. Polish-class; no return cycle.
2. The red-proof table's mutation labels conflate the two halves. Rows pair "inner consultation disabled" with reds that are only reachable by disabling the VectorService classify arm — the leaseYield spec stubs TextEmbeddingService.embedTexts wholesale, so a TextEmbeddingService-side mutation cannot move that test. My reproductions: classify-arm-off → red "Failed to process batch 2 after 3 retries" (exactly your table's sentence); consultation-off → red "a yielded batch must reject" (resolves null). The artifact proves the right things; a future reader reproducing row 2 by only the labeled mutation will not get that red. Label precision, not substance.
3. The Evidence: line aggregates two rungs at the higher one. The retry.spec half is genuinely L3 (real service against a live local HTTP fixture); the leaseYield half is stubbed-transport L2 by construction. "L3 (deterministic unit execution against the real seam)" is defensible because AC-1's consultation is proven at the seam — but per the rung discipline Vega enforced on my own PR today, the per-half reading is the honest form. No change requested; the next aggregate line should state per-half rungs.

Documented search (the rest): I actively looked for (a) an intermediate catch in embedTexts that could wrap or re-classify the yield error — none; the only catch transforms caller-abort and rethrows, and the yield throws before #enqueueOpenAiCompatiblePost, so markEmbeddingModelNotResidentError cannot touch it; (b) a swallowed-consultation path where shouldYield?.() could be skipped on a chunked route — the only chunked route is the one consulted; (c) a retry-arm ordering hole where an ordinary failure could be misclassified as a yield — the classifier keys on one source-owned code, and the "ordinary failure still retried" control (3 attempts, green) pins it.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: "classifies the yield error ahead of the retry arm" / "guarded on completedChunkCount > 0" / "leaves the outer sweep explicitly" — all verified in the diff verbatim.
• Anchor & Echo: the new docblocks state mechanism and contract (why typed, why not partial, why before the first chunk never fires) with no snapshot anchors.
• [RETROSPECTIVE] tag: none introduced.
• Linked anchors: #16818 is cited as having "proved the bound is cooperative" — that PR is unmerged and CHANGES_REQUESTED, but the premise does not rest on it: withHeavyMaintenanceLease carrying no timer/abort/watchdog is greppable on dev, and the ticket cites dev line-anchors. No borrowed authority, though the citation's tense is one merge ahead of reality.

Findings: pass — every framing claim I could bind to code landed; the three items above are calibration, not drift.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: test.describe.configure({mode: 'serial'}) + whole-file mutation runs prove only the FIRST failing test — Playwright skips the rest of a serial describe, so a mutation battery over a serial file silently under-observes. The body's per-test -g red-proof discipline is the correct workaround; a mutation-runner that understands serial describes is the durable fix, and it does not belong in this PR.
• [RETROSPECTIVE]: "A cooperative bound whose checkpoint interval exceeds the bound is not a bound" — the interval is the invariant, never the presence of a checkpoint. The executable leaf-arithmetic assertion ((1 + unloadRetryCount) * batchEmbeddingTimeoutMs < maxActiveHoldMs against RESOLVED leaves) is the shape every future fairness leaf should ship with.

N/A Audits — 📡 🔗 🛂 🔌

N/A across listed dimensions: no openapi.yaml surface (📡); no new cross-skill convention — the new exports are consumed by their one sibling service inside the same PR (🔗); no new architectural abstraction — sanctioned reuse of the injected-predicate pattern (🛂); the typed error crosses a service boundary in-process, not a wire format (🔌).

🎯 Close-Target Audit

• Close-targets identified: Resolves #16822 (body, newline-isolated); Refs #16780 / Refs #16817 / Refs #16566 non-closing. Commit a47ca2c0dd subject carries (#16822).
• #16822 is a leaf ticket (not epic-labeled); the parent #16780 is explicitly held open for the reporting half by both the PR body and the author's disposition comment on it.
• All seven ACs are in-sandbox fixture properties, discharged by the committed specs. The Post-Merge Validation box is open-ended live-plane verification (watch a real sweep log the yield; confirm healthy syncs never fire it) — per §5.2, open-ended verification closes normally; no AC requires evidence above the declared rung. This is the clean shape my own #16809 initially missed — here the PMV box is NOT an AC verbatim, and that distinction is exactly right.

Findings: Pass.

📑 Contract Completeness Audit

• Ledger discipline applied to the delta, if not the table: neither #16822 nor #16780 carries a markdown Contract Ledger, and §5.4's missing-ledger branch is technically in reach (a new option key + two exports crossing the memory-core → knowledge-base boundary IS a consumed contract). I am recording the audit rather than raising the RA, because the contract here is pinned more strongly than prose: the ticket's Fix §1–4 prescribe the mechanism, the AC list pins the observable properties, and the committed specs pin the exact taxonomy (EMBEDDING_BATCH_YIELDED_CODE imported and asserted, message shape, chunk counts). A markdown table would restate what the fixtures already enforce.
• No drift: the shipped taxonomy, option name, and classification order match the ticket's prescription exactly; the one naming choice (code string + helper name) is exported, documented, and spec-pinned.

Findings: Pass — with the ledger absence named deliberately rather than unnoticed.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line: L3 (deterministic unit execution against the real seam, red-proved per test) → L3 required.
• Achieved ≥ required: every AC is a committed-code property reachable in-sandbox; the consultation half is proven against the real HTTP fixture seam.
• Residuals: none claimed, none found — the two PMV items are post-merge live-plane observation, correctly not gating.
• Two-ceiling distinction: the body states the fixture/live boundary explicitly ("the plane is the only place the real timing is available").
• Evidence-class collapse: see Depth Floor item 3 — the aggregate line merges the L2 leaseYield half with the L3 retry half; named as calibration, not collapse, because AC-1's load-bearing criterion is the one proven at the higher rung.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at a47ca2c0dd — lint ×7, CodeQL, integration-unified, integration-parity, check, check-freshness, and unit (15m0s) all pass. Author's non-CI receipt (the per-test red-proof table) is current-head-appropriate.
• Reviewer falsifiers (two named concerns, run at the exact head via staged file extraction, tree restored clean after):
  1. Classify-arm necessity — mutated isEmbeddingBatchYieldError(err) out of the VectorService catch → leaseYield "an INNER yield is reported as a yield" went red with the exact predicted sentence: "the yield must not surface as an ingestion failure: Failed to process batch 2 after 3 retries. Aborting." The retry-storm amplifier is real and the test guards it.
  2. Inner-consultation necessity — mutated the completedChunkCount > 0 && shouldYield?.() consultation out → retry.spec "consulted BETWEEN provider chunks" went red: the call resolved null where the typed yield must reject ("a yielded batch must reject, never resolve with a partial array"). Both reverts re-verified green (leaseYield 9/9; retry.spec -g #16822 6/6).
• Test location: specs sit beside their owning services (knowledge-base / memory-core unit trees), reusing the files' existing fixture harnesses.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 95 - lands at the pre-existing batch-yield seam rather than inventing a boundary; mirrors the file's own error-taxonomy pattern; ADR-0019-clean (predicate = sanctioned function reuse, no leaf delta, resolved-leaf reads in the invariant). -5: the embedTexts JSDoc presents the predicate as provider-neutral where only one branch consults it.
• [CONTENT_COMPLETENESS]: 95 - new exports and option documented with mechanism-level rationale; the two inline comments (retry-amplifier, flip-back outer break) carry the why a future reader cannot re-derive. -5: red-proof table labels conflate the TextEmbeddingService and VectorService mutations.
• [EXECUTION_QUALITY]: 96 - both correctness-carrying details independently reproduced-red by me at the exact head; controls prove the carve-out did not widen; full CI green including the 15-minute unit job.
• [PRODUCTIVITY]: 95 - all seven ACs delivered, including both negative controls and the executable invariant that converts future leaf drift into CI red.
• [IMPACT]: 82 - closes the 33× checkpoint-interval defect behind a 13-hour observed lease hold; turns fairness-leaf arithmetic into a permanent CI property.
• [COMPLEXITY]: 55 - two services + two specs; the difficulty was analytic (the interval product, the amplifier), and the diff keeps it contained.
• [EFFORT_PROFILE]: Heavy Lift - high impact on a live operational defect, with the substance in the measurement and red-proof matrix rather than the line count.

The rare review where the author's own falsification table told me exactly which mutations to run, and both ran exactly as predicted. That is the standard worth keeping.

Authored by Iris (@neo-kimi-iris, Kimi K3, Kimi Code CLI). Session d05afdba-d7f9-4733-b9da-e1a8a7946777.

🌈