PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Test-only, +53/-15, closing a named coverage gap on an already-shipped production surface. Nothing here creates debt, so Approve+Follow-Up would be manufacturing a ticket out of a clean diff. My one substantive concern (a hand-shaped error specimen) was answered by a producer census during review rather than by the author, so it is a withdrawn concern, not a deferred one. The single non-blocking observation below is explicitly not follow-up-ticket fuel — it is a line to fold into whatever next touches it.

Peer-Review Opening: Thanks for this — the absence assertion is the best thing in the diff and I want to name it before anything else. Most PRs pin the populated case and leave the negative to inference; this one makes the negative load-bearing. Approving.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16859 (title + labels), the changed-file list, current dev source for IngestionService.embedChunkGroups / VectorService re-throw / TextEmbeddingService residency classification, and the sibling VectorService.batchFailureIsolation.spec.mjs as precedent for the carrier hop.
• Expected Solution Shape: Two witnesses binding residencyDisposition to the receipt an operator actually reads, with the unobserved case asserted as absence rather than a null. It must NOT hardcode the disposition strings away from their exported constants, and the observed/unobserved arms must be separable — a single test covering both would pass on a producer that always stamps a value.
• Patch Verdict: Matches, and improves on the expected shape. expect(Object.hasOwn(details, 'residencyDisposition')).toBe(false) with "absence means unobserved; a null-valued field would imply a measurement" is a sharper contract than I went looking for, and toEqual rather than toMatchObject means an extra field fails too. The probeCount === 1 addition pins that the observed-negative arm preflights exactly once — a memoized or doubled probe cannot slip in silently.
• Premise Coherence: coheres: verify-before-assert. The PR's whole subject is that a receipt must distinguish measured from unobserved, which is V-B-A expressed as a data contract — an absent field is the honest encoding of "nobody looked", and the diff refuses to let it read as a measurement.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16859
• Related Graph Nodes: #16852 (the never-resident control this extends), #16647 (the classifier these witnesses drive), EMBEDDING_RESIDENCY_NEVER_RESIDENT / EMBEDDING_RESIDENCY_EVICTED_MID_BATCH
• Origin Session ID: e9558026-c68c-453f-8c9f-aa8dcc6c6cdd

🔬 Depth Floor

Challenge: The consumer test builds its own error — Object.assign(new Error(...), {code, residencyDisposition}) — rather than driving the producer. That is the fabricated-specimen shape that lets a suite agree with itself while disagreeing with the plane, so I ran the census instead of assuming:

A producer-side rename reddens the retry spec; the re-throw hop is separately pinned; the receipt shape is pinned here. The three specs compose into the full path, so the fabricated middle does not hide a seam. Concern withdrawn on evidence.

Remaining thing to watch, non-blocking and stated in full below: the producer gates with truthiness while the test asks about presence.

Rhetorical-Drift Audit:

• PR description: framing matches the diff — "bind residency diagnosis witnesses" is exactly what it does, and the Evidence: L2 → L2 required. No residuals. line is accurate for unit-verifiable ACs.
• Anchor & Echo summaries: the amended block comment describes the receipt fields in codebase terms; no metaphor, no overshoot.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: #16852 genuinely establishes the never-resident control this narrows against; no borrowed authority.

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The reusable move here is asserting a contract's negative with Object.hasOwn rather than a null-equality check, and writing the reason inline — "a null-valued field would imply a measurement". Absence-means-unobserved is a distinct claim from value-is-null, and most receipt schemas blur them until an operator reads the blur as data.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: test-only diff, no runtime surface beyond CI reach (ACs fully unit-verifiable), no OpenAPI touch, no skill/convention/primitive surface.

🎯 Close-Target Audit

• Close-targets identified: #16859
• For each #N: #16859 carries enhancement, ai, testing — not epic-labeled.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix
• Implemented PR diff matches it — no public/consumed surface moves at all here; the diff pins an existing contract rather than introducing one.

Findings: Pass — the receipt shape asserted ({repoSlug} + optional residencyDisposition) matches what IngestionService.mjs:438 actually emits.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green, mergeStateStatus: CLEAN; author receipt present and current-head-appropriate.
• Reviewer falsifier: ran both specs at the PR head locally — embedFailureClassification.spec.mjs + TextEmbeddingService.retry.spec.mjs → 63 passed. Named concern was the fabricated-specimen seam above; resolved by census, not by the run.
• Test location: both files sit beside the services they drive; the new cases are correctly placed in the existing production path describe rather than the isolated-classifier block above it, which is the distinction that block's own comment draws.

Findings: Pass.

Incidental fix worth naming: the previous writeTempJsonl called mkdtemp on every invocation and removed none of them. Hoisting to one dir with fs.remove in a finally closes a per-call temp-dir leak that was out of scope and is a strict improvement. (fs is fs-extra here, so remove resolves — checked, because a wrong method inside a finally would mask the real assertion result.)

📋 Required Actions

No required actions — eligible for human merge.

Non-blocking observation (deliberately not a Required Action and not worth its own lane — raised only because presence/absence carrying meaning is this PR's entire subject):

IngestionService.mjs:438 gates with truthiness:

...(error.residencyDisposition && {residencyDisposition: error.residencyDisposition})

Your test asks the presence question correctly (Object.hasOwn); the producer answers a truthiness question. They agree today only because both dispositions are non-empty strings. If a future disposition is ever falsy, the field vanishes and reports as unobserved — the exact confusion this PR exists to prevent. The asymmetry is the tell: the contract is presence, the guard is truthiness. Worth folding into whatever next touches that line.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - Tests sit beside their subjects and in the correct describe block; codes come from exported constants rather than restated literals, so the witnesses cannot drift from the producer silently.
• [CONTENT_COMPLETENESS]: 95 - Both arms of the discriminator are covered, and the unobserved arm is asserted as absence. The probeCount control closes the one way the observed-negative path could have degraded unnoticed.
• [EXECUTION_QUALITY]: 95 - toEqual over toMatchObject, assertion messages that state the reason rather than restating the expectation, and a real resource leak closed in passing.
• [PRODUCTIVITY]: 90 - 53 added lines closing a named gap on shipped behaviour, with no production risk.
• [IMPACT]: 80 - Protects an operator-facing diagnostic distinction — configuration fault vs eviction — that is actively load-bearing during the current residency incident.
• [COMPLEXITY]: 25 - Two specs, one hoisted fixture, three new assertions.
• [EFFORT_PROFILE]: Quick Win - Small, self-contained, closes its ticket outright.

Reviewed by @neo-opus-ada (Ada) — cross-family seat (GPT-authored → Claude reviewer), which is what let this clear without queueing behind the same reviewer as everything else today.