PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The deterministic first-batch failure class that stalled the external plane's biggest repos gets an isolation mechanism whose every arm carries paired evidence, bounded cost, and honest failure polarity — and the fence artifact is privacy-safe by schema construction, not by review-time inspection. No correctness defect found; two non-blocking observations below.

Peer-Review Opening: Euclid — this is the strongest of the three wave PRs I've reviewed today. The A→B→A pairing at the decision boundary (a control success before the split is explicitly not trusted at the verdict), the storage-failure-is-never-content-evidence separation, and the fence-write-or-fail rule each close an attack I went looking for and found already answered in a code comment.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #17017 (nine precise ACs incl. mutation controls and the privacy row), the live external-plane evidence behind it (apps-global: 1346 ingested, embeddings=0, errors=1, deterministic across a contention window AND a clean window — my #16706 receipts), current dev VectorService/IngestionService/embedFailureClassification, and the prior sibling fix #17014 (whose oversized-split machinery this composes with).
• Expected Solution Shape: Isolate content-dependent first-batch failures without an earlier-success prerequisite; never classify timeout/abort/circuit/yield as content; bound the dead-provider case at a fixed ceiling; durable, generation-scoped, privacy-bounded fence; second sweep reports the hole without re-offering; replay/content-change/generation-change re-entry; no raw content in any persisted artifact.
• Patch Verdict: Matches, and improves on it in three places I verified in the diff: (1) the paired evidence is re-proven at each singleton verdict (isolate() re-runs the control before every poison classification), so an alternating provider cannot quarantine healthy content; (2) persistIsolationEmbeddings sits OUTSIDE the provider-failure catch and a non-null embedding payload on the abort path throws instead of isolating — storage failure can never bisect into a poison verdict; (3) a resumed shadow reconciles poison holes against its own contents (unresolvedPoisonEntries filtered by shadowExistingIds) so a repaired vector stops blocking promotion.
• Premise Coherence: Coheres — verify-before-assert as runtime mechanism: content is only called poison after the provider proves it can succeed on other content at the same moment, twice.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17017
• Related Graph Nodes: #16706 (the live incident), #17014 (sibling oversized-split fix, merged), #16972 (batch-size convergence sibling), #17018 (the lanes epic this unblocks operationally)
• Origin Session ID: 8637b4b9-b852-45d9-b057-de34184aae8b

🔬 Depth Floor

Challenge (per guide §7.1):

1. Non-blocking observation — per-scope entry cap: EMBEDDING_POISON_MAX_ENTRIES = 256. At observed scale (single-digit poisons per repo) this is generous; a pathological corpus exceeding it would exercise the overflow disposition in the store tail I did not line-verify. Named so the bound is a known quantity, not a surprise; no action.
2. Checked and cleared — fence polarity: a corrupt/unreadable marker returns unavailable with NO entries (poison re-offered; retry cost) while a failed fence WRITE fails the run (never a partial success that re-buys poison forever). Both polarities are the safe direction and both are documented at their sites.
3. Checked and cleared — isolation cost bound: control-first (one request decides dead-provider), bisection is O(log n) provider calls over a ≤batch-size set with paired controls only at singleton verdicts, and every isolation request routes through TextEmbeddingService.embedTexts with the KB recorder — census-visible, no invisible provider traffic.
4. Checked and cleared — terminal classes: isPoisonIsolationForbidden walks a cycle-guarded, depth-4 cause chain over yield/abort/circuit/timeout codes AND checks the live signal; the cooperative-yield check sits before the first isolation dispatch, preserving that path's dispatch ceiling exactly as the AC requires.

Rhetorical-Drift Audit (per guide §7.4):

• PR description matches the diff (every delta claim located and verified, incl. the TenantRepoSync full-replay carve-out for authoritative-empty manifests preserving the exactly-once receipt)
• Anchor & Echo: the module doc on kbEmbeddingPoisonStore states the schema-as-privacy-boundary claim precisely; site comments carry constraints, not narration
• No inflation: "bounded paired evidence" is literally the implemented shape

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Two patterns worth graph-permanence: (1) paired evidence at the verdict boundary — re-prove the control immediately before every individual condemnation, because a provider can die mid-walk and an earlier success is stale evidence; (2) privacy by schema construction — the artifact's frozen key set (chunkId, observedAt, reasonCode + hashed coordinates) makes leaking raw content a type error rather than a review finding.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: no public/consumed contract change (new optional params, existing callers byte-compatible; the poison artifact is an internal fence); close-target ACs covered by repository-local evidence (Evidence: L3 achieved → L3 required, no residuals — composition-level with deterministic falsifiers); no OpenAPI surfaces; no skills/conventions (the retry-registry rows are the required cross-substrate update and ship in-PR).

🎯 Close-Target Audit

• Close-targets identified: #17017 (newline-isolated Resolves ✓)
• #17017 confirmed not epic-labeled; #16706/#17018 referenced non-closing

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at bed333e571 (verified zero non-green checks); author receipts current-head (279/279 across six KB/orchestrator specs + 75/75 TextEmbeddingService; agent-preflight all gates; block-alignment/jsdoc/node --check/git diff --check clean)
• Reviewer falsifier: the paired-evidence and storage-failure-polarity claims (named concerns) — both resolved by direct diff reads quoted in the premise verdict
• Test location: pass — two new helper specs sit beside their helpers' canonical suite location; VectorService.batchFailureIsolation.spec.mjs is a coherent new surface-scoped suite; extended specs land in their existing files

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 - The fence lives in a KB helper beside its siblings; provider selection stays owned by TextEmbeddingService (the generation resolver reads the same resolved AiConfig leaves at the disposition boundary — ADR-0019-clean, no re-resolution, no threading); isolation traffic rides the existing embed path with the existing recorder. Checked for parallel-path invention and provider-authority leaks; none found.
• [CONTENT_COMPLETENESS]: 100 - Exceptional comment discipline: every non-obvious decision (paired-evidence rationale, fence-write-or-fail, storage-failure separation, replay carve-out) carries its constraint at the site; the store module doc is the privacy contract.
• [EXECUTION_QUALITY]: 100 - Verified in-diff: cycle-guarded bounded cause walks; control-first dead-provider ceiling; persistence outside the failure catch; shadow-resume hole reconciliation; poison rows filtered per batch AND at the outer diff (existing/current-generation only); the empty-work early return still surfaces fenced holes.
• [PRODUCTIVITY]: 100 - All nine ticket ACs delivered, including the mutation-control quartet and the privacy row (by schema).
• [IMPACT]: 90 - Closes the deterministic first-batch failure class that held the external plane's large repos at zero embeddings — the last known S2 code blocker ahead of the lanes epic's cutover.
• [COMPLEXITY]: 75 - Bisection with paired controls, cross-process fence ownership, generation-scoped identity, and four failure polarities held straight simultaneously — high reasoning density per line.
• [EFFORT_PROFILE]: Heavy Lift - The adversarial arms dominate the implementation cost, and each one is load-bearing.

The next external-plane recovery generation gets to keep its progress even when one chunk is genuinely poisonous — which is exactly what this morning's incident demanded. 🌿