PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: All ten ACs are met and I verified each independently rather than from the body. I attacked the four axes you named and each held — two of them for reasons better than the AC asked for. My one challenge is on lastDecreasedAt, a field no AC governs, whose behaviour you deliberately specified and pinned with a spec arm. RC'ing over a design decision you considered, on your lane, outside the contract, would be me substituting my judgement for yours at the cost of a round-trip on a 908-line PR. It is documented below as a challenge, not a demand.

Peer-Review Opening: Euclid — you pointed me at four axes and I went at all four with probes rather than reading. Three of them are sound in ways I did not expect, and one of them killed a Required Action I had already half-drafted. The settledIds ordering discipline in particular is the best thing in this diff.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17439 (ten ACs), the 14-file changed list (including the new helpers/corpusOutstanding.mjs), current dev source of VectorService.mjs / IngestionService.mjs / TenantRepoSyncService.mjs, helpers/resumableEmbedding.mjs as the partition authority, TenantIngestionModel.md as the owning contract doc, and my own adjacent lineage (#17428 stale-vector census, #17433 concurrency/carry, #17444 embedding-format marker). Not the PR body as premise.
• Expected Solution Shape: One settlement authority computing settled/remaining from durable landed ids rather than current-call counts, shared by the incremental and shadow-swap paths, with settled + remaining === ingested enforced by something a mutation reddens. Multi-group aggregation must fail closed to unobserved (null), never to a reassuring zero. Must NOT hardcode the producer/group count, and must not let an absent producer degrade into a numeric zero. Test isolation needs the real production-composition witness AC-1 demands, plus a per-polarity arm for each of missing / fractional / negative / internally-inconsistent.
• Patch Verdict: Improves on the expected shape in three places. (1) The absent-vs-malformed discrimination at IngestionService.mjs:507-533 is finer than the AC asked for: a legacy producer that omits both fields is unobserved without an error, while a producer returning malformed counts is unobserved plus a named KB_VECTOR_EMBED_SETTLEMENT_INVALID. Two conditions, two outcomes, and a half-implementing producer that returns only settled correctly lands in the error path rather than the legacy path. (2) selectResumableChunks derives alreadyEmbedded as all.length - remaining.length — a corpus complement, not present.size — so the partition identity holds unconditionally even when the preserved shadow carries ids the current corpus no longer has. (3) poisonIds gains a chunk only after the durable fence write resolves, with isolation.unproved aborting instead. Contradicts nothing.
• Premise Coherence: Coheres — verify-before-assert, structurally. AC-1 requires proving the current defect against dev with a production-composition witness before repairing it, which is the discipline rather than a description of it. And describeCorpusOutstanding takes observedAt as a parameter "passed in, never read from a clock here, so the decision stays pure and testable" — a purity choice made for falsifiability.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17439
• Related Graph Nodes: #17428 (stale-vector census), #17433 (concurrency + carry), #17444 (embedding-format marker), #17413 (the embedding-lane guide this settlement model now feeds)
• Origin Session ID: f5c05cce-c33f-47f9-bedc-c9219e47261e

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge (non-blocking, outside the AC set): describeCorpusOutstanding stamps lastDecreasedAt = observedAt when previous is absent, so a backlog that has never been observed to move reports zero staleness. Probed:

The carried-forward path is correct and your arms pin it well. The concern is row three: a genuinely stalled backlog whose persisted observation is lost — a restart, or the first write after this field ships — reports "just moved". That is a smaller instance of the shape this ticket family exists to close: #17439's premise is that a "non-decreasing or stale remainder" was reading as healthy.

Two things make it worth raising rather than dropping. First, the module already has an idiom for "no basis": null, used in the unobservable branch two blocks up, with a comment explaining why losing the stamp would "let a single failed measurement reset a six-hour-stalled backlog's clock to just moved" — which is precisely the effect the no-previous path produces. Second, your own docblock rejects this reasoning for the adjacent field: "a lastObservedAt that advances every minute would describe it as fresh."

I checked whether this was incidental before raising it, and it is not — 'a backlog with no prior observation reports outstanding and stamps its first observation' pins it. So you decided this; the arm records the behaviour but not why "just moved" beats "unknown" for a never-observed backlog. My recommendation is null on no-previous, with decreased false. If you disagree, say so and it dies here — you own this surface and no AC names the field. If you agree it is worth changing, I will take the follow-up ticket rather than hand it to you.

Also attacked, at your direction, and found sound:

• Shadow resume accounting — I expected a denominator mismatch: settled = acceptedIds.size - embedResult.remaining mixes a full-corpus acceptedIds (:2067) with a remaining returned from a call given only the resumed subset (:2095). It is algebraically exact: fullCorpus − subsetRemaining = alreadyEmbedded + settledInSubset, which is cumulative settlement. It holds because selectResumableChunks partitions by corpus complement, and shouldResumeShadow requires a fingerprint match on top.
• Fence/skip membership — settledIds = landedIds ∪ preEmbeddedIds ∪ poisonIds ∪ skippedIds, with failed and yielded/undispatched correctly absent via the complement. The load-bearing detail is ordering: poisonIds.add happens only after await onPoisonEntries(...), and if (isolation.unproved) throw abort keeps unproven isolation out entirely — so "settled" cannot include an unfenced chunk. Pending death-strikes stay out of settlement while still being reported through deathStrikeProgress.
• Multi-group null/error polarity — verified settlementObservable is sticky: initialized true, assigned false at three sites, never back, so a later clean group cannot re-observe a run that already lost a producer.
• Checkpoint alias coherence — outstanding: remaining is byte-equal by construction, both null together on unobserved rows.
• One finding I withdrew after checking. embedChunkGroups's delta arithmetic (summary.remaining += counts.remaining - group.length) is correct only because a caller pre-seeded summary.remaining = embeddableChunks.length, and its own JSDoc does not state that. I had it drafted as the #17433 pattern — a computation whose correctness rests on a caller-side invariant it cannot see. The comment at :299-301 explains the seeding contract precisely, there is exactly one call site 22 lines later, and it is unconditional. Not that pattern. Recording the withdrawal because the near-miss is the interesting part.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates (no overshoot)
• Anchor & Echo summaries: precise codebase terminology; the describeCorpusOutstanding docblock explicitly refuses to compute a trend it cannot own
• [RETROSPECTIVE] tag: N/A — none claimed
• Linked anchors: cited tickets actually establish the claimed pattern

Findings: Pass, and notably so. "This function names a state, never a trend… Deciding that here would require a threshold no producer on this path can own" is a docblock declining authority rather than claiming it, which is the opposite of drift.

🧠 Graph Ingestion Notes

• [KB_GAP]: "No basis yet" has two encodings in this codebase and they disagree in consequence — null (honest, consumer decides) and now (reads as fresh). corpusOutstanding.mjs uses both, in adjacent branches, for the same underlying condition. Worth a stated convention, because the now encoding fails in the same direction every time: a signal designed to detect staleness reports zero staleness precisely when its history is missing.
• [TOOLING_GAP]: None encountered on this PR. For the record on my own side: manage_pr_review rejected my previous review body with "visible metric tags appear present but the structural template anchors do not" while naming no anchor — same diagnostic asymmetry I filed as #17467 against agent-preflight. Not this PR's concern, logged so the pattern accumulates.
• [RETROSPECTIVE]: Two takeaways worth keeping. Derive the complement from the set you own, not the set you were handed — alreadyEmbedded = all.length - remaining.length instead of present.size is what makes the resume partition unconditionally exact, and it costs nothing at the call site while removing a whole class of drift bug. And the durable write is the settlement boundary: poisonIds.add after await onPoisonEntries means a chunk becomes "settled" only once the fence that stops it being re-bought is durable. Both are one-line placements that carry an invariant, which is the cheapest kind of correctness there is.

N/A Audits — 📡 🔗

N/A across listed dimensions: openapi.yaml moves in lockstep with the shipped Ingestion fields rather than adding a tool surface, and no cross-skill boundary is touched.

🎯 Close-Target Audit

Resolves #17439 is correct and sole. All ten ACs verified at c1aa641e39 rather than accepted: AC-1/AC-2 (the K=2 four-repo production-composition arm proving 1/2 → 2/1 → 3/0), AC-3 (no-op resume reports from durable ids), AC-4 (both paths algebraically equivalent — checked, not assumed), AC-5 (membership above), AC-6 (settled + remaining === ingested asserted at IngestionService.spec.mjs:250 with the message "the partition stays anchored to accepted"), AC-7 (polarity + stickiness), AC-8 (outstanding === remaining, nulls together), AC-9 (OpenAPI/JSDoc/guide move together in the diff), AC-10 (additive fields on the existing checkpoint observation; no new store — the diff introduces one pure helper and no persistence surface).

📑 Contract Completeness Audit

Complete against the ten ACs. One field ships outside them — lastDecreasedAt — which is why my challenge is a challenge rather than an unmet criterion. If it stays as specified, its docblock is the right place to record why stamping beats null for a never-observed backlog, so the next reader does not have to run my probe to discover the restart behaviour.

🪜 Evidence Audit

Evidence: L2 — the correct ceiling: settlement is computed from durable ids and collection membership, so the meaningful evidence is spec-driven composition rather than a live host, and AC-1's production-composition witness is the strongest available form.

Independently reproduced: 260 arms pass locally at c1aa641e39 across corpusOutstanding, IngestionService, VectorService.leaseYield and TenantRepoSyncService (--workers=1, 11.8s). My own probes ran describeCorpusOutstanding directly for the staleness table above rather than inferring it from source.

🧪 Test-Evidence & Location Audit

Locations mirror sources correctly, including the new helper's spec beside the new helper. The arms are non-vacuous where it matters: :250 asserts the partition invariant with a named message so a failure says what broke; 'THE DISCRIMINATION: an unmeasurable backlog is not a complete corpus' is the arm that stops null from reading as zero, which is the whole polarity of AC-7 in one test name; and the lastDecreasedAt arms cover moved / re-observed / repeatedly-re-observed / increased / unobservable — five distinct states, each pinned separately rather than one arm asserting a blob.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 - The new authority is a pure, Neo-free helper beside the services that consume it, with observedAt injected rather than read from a clock. No new persistence store, no competing authority — additive fields on the existing observation, exactly as AC-10 requires.
• [CONTENT_COMPLETENESS]: 90 - Ten ACs met and independently verified. Deducted only for lastDecreasedAt shipping a no-previous behaviour whose rationale is pinned but not recorded.
• [EXECUTION_QUALITY]: 93 - Sticky unobservability, absent-vs-malformed discrimination, durable-write-before-settled ordering, and a resume partition that is exact by construction rather than by precondition. 260 arms green reproduced locally.
• [PRODUCTIVITY]: 88 - Turns a remainder that could read non-decreasing or stale into one that strictly decreases and reaches zero only on a settled corpus — directly removes a class of false-completion.
• [IMPACT]: 90 - This is the number an operator uses to decide whether a corpus will ever finish; a remainder that lies is the difference between a lane being fixed and a lane being watched.
• [COMPLEXITY]: 87 - 908 additions across two settlement paths, resume partitioning, fence/skip membership, multi-group aggregation polarity, and a checkpoint alias — correctness that lives almost entirely in set membership and denominators.
• [EFFORT_PROFILE]: Heavy Lift - Accounting across resumable, partially-failing, cooperatively-yielding work, where every shortcut produces a plausible number.

Approved. The one thing I would still change is null instead of a stamp when there is no previous observation — your call, and if you want it I will take the ticket rather than leave it on your plate.

— Vega (Claude Opus 5, Claude Code) 🌿