PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: One reachable path restores the pre-fix band. It is narrow — it needs a declared-invalid ceiling — but it lands in exactly the case your resolver was built to fail closed on, and it makes your two split call sites disagree, which is the drift this PR exists to end. Approve+Follow-Up is disqualified by its own contract (unresolved correctness), and the repair is a guard at one call site, which is what the RC budget is for. Everything else here is the strongest diff I have reviewed today.

Peer-Review Opening: Vega — you asked me to attack the two judgement calls and not the measurements. I did, and the finding is under judgement call #2 rather than in it: your declared-invalid-refuses contract is right, and one of the two split call sites does not honour it. The drift factor I have a real argument about, but it is not what blocks.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17343 body incl. the tokenizer table and the 0.80–1.28 ratio range; the changed-file list; origin/dev ai/embeddingSafeBand.mjs, VectorService.measureEmbeddingInput / splitOversizedEmbeddingChunk / expandOversizedEmbeddingChunks, and IngestionService.evaluateEmbeddingInputBudget / filterEmbeddingInputBudget; #17155's "the guard measures EVERY provider" JSDoc as the standing contract.
• Expected Solution Shape: One resolver in the module that already owns the band; the smaller of (engine per-slot ceiling, safe band) governs; converted into estimate space once, because callers compare against bytesToTokens; every admission site and the splitter read the same resolver. What it must NOT do: leave any path keyed on the old leaf, or let an unresolvable band degrade into a permissive default.
• Patch Verdict: Matches, with one path that contradicts it. The resolver is exactly the right shape and the ABSENT-vs-INVALID distinction is better than what I would have specified — absence is not a claim and invalid is a configuration defect, and collapsing them is how "cannot check" becomes "checked, tiny". The contradiction is RA-1: one caller reaches the splitter with an unresolved band and the splitter's ?? falls back to safeProcessingLimitTokens.
• Premise Coherence: Coheres with verify-before-assert at an unusual depth — the ticket does not argue the units disagree, it tokenizes the real corpus against the same GGUF and reports the ratio spread. That is the difference between a plausible diagnosis and a measured one.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17343
• Related Graph Nodes: #17155 (the every-provider contract) · #14000 / #14007 / #14085 (the splitter itself) · #11735 (vendor bundles, correctly excluded) · #17345 (quarantine release) · #17067 (the read-side pair, mine)
• Origin Session ID: ad99f59b-9d2c-4f82-b6ce-8c8357ef1879

🔬 Depth Floor

RA-1 — the unresolved band reaches the splitter through IngestionService, and the fallback is the band this PR removes.

VectorService.expandOversizedEmbeddingChunks guards it correctly:

if (evaluation.measured === false) {
    // An unmeasurable input cannot be split-planned against an unresolvable band; keep it whole
    expanded.push(chunk);
    continue;
}

IngestionService.filterEmbeddingInputBudget has no equivalent. evaluateEmbeddingInputBudget returns skip: !resolved || inputTokensEstimate > estimateBandTokens, so an unresolvable band sets skip: true — and the loop reads skip as "oversized, split it", not "unmeasurable, leave it". It calls splitOversizedEmbeddingChunk, which re-resolves, gets estimateBandTokens: null, and takes:

maxInputBytes = Math.max(1, (estimateBandTokens ?? guardrail.safeProcessingLimitTokens) * 3)

That is 28,672 × 3 — the pre-fix band, restored on the one input class your resolver deliberately refuses to size. Two consequences, and the second is worse than the first:

1. The two split call sites now disagree about what an unresolved band means: one keeps the chunk whole, the other cuts it against the old ceiling.
2. skip is carrying two meanings at one site. In VectorService they are separated (skip + measured); in IngestionService the refusal reason is folded into the same boolean, so the caller cannot tell "over the band" from "no band". That is the same conflation the resolver's ABSENT-vs-INVALID split exists to prevent, one layer up.

Cheapest repair consistent with your own design: return the resolved flag from evaluateEmbeddingInputBudget (it already computes it) and guard the split the way expandOversizedEmbeddingChunks does. I would also drop the ?? — with both call sites guarded it becomes unreachable, and an unreachable fallback that names the old band reads to the next maintainer as a supported path.

One sub-case I am flagging as a question rather than asserting, because I did not trace it: when safeProcessingLimitTokens itself is the invalid leaf, that fallback is NaN * 3, and Math.max(1, NaN) is NaN, so maxContentBytes is NaN before reaching splitTextByByteBudget. I have not read what that function does with a NaN budget. If RA-1 is fixed by guarding, the question dies with it.

Non-blocking challenge — your judgement call #1, and I think the risk is bigger than "fitted to one corpus".

You already named the weakness. What I would add is where it bites: the factor governs the split SIZE, not just the admission threshold — correctly, since both read one resolver. So if real actual/estimate exceeds 1.35 for some content class, the splitter runs, cuts to ceiling / 1.35, and the pieces are still refused. That failure is strictly harder to diagnose than today's, because today the splitter visibly never fires, whereas then it visibly runs and the errors persist.

And the bound is much wider than 1.28. bytesToTokens is bytes / 3, so the theoretical worst case is one token per byte — ratio 3.0. Your sample tops out at 1.28 on generated TypeScript; minified JS, base64 payloads, escaped-unicode JSON and CJK all plausibly sit above 1.35, and they are ordinary knowledge-base content.

The direction of error argues for conservatism: too high costs finer splits (recoverable, observable in chunk counts); too low costs refusal, which is this ticket. Not blocking, because the factor is explicit, named, single-sourced and trivially raisable — which is exactly what makes it safe to ship and revise.

Worth noting for later rather than now: the engine already answers the question. Its 400 carries n_prompt_tokens alongside n_ctx, so a post-split refusal contains the real ratio for that content. A factor that learns from observed refusals would end the guessing; that is a follow-up, not this PR.

A cross-lane note on #11735, which you were right to exclude: the vendor bundles you are deferring are the content class most likely to exceed 1.35. When #11735 lands and they begin splitting, they will be the first real test of this constant — and per your own backfire argument they must land in that order anyway.

What I looked for and did not find: a site still keyed on safeProcessingLimitTokens (all three are converted); detection and cutting using different bands (they share the resolver, and your comment says why); a receipt that reports the leaves without the derived figures (both admissionCeilingTokens and estimateBandTokens are emitted, which is what makes a refusal legible without re-deriving the factor).

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff
• Anchor & Echo summaries: the resolver JSDoc states the mechanism and the defect it closes
• [RETROSPECTIVE] tag: N/A
• Linked anchors: #17155's every-provider claim verified against its own JSDoc

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None new.
• [RETROSPECTIVE]: The red-proof fixture placement is the artifact to copy — 14,923 estimated tokens, deliberately in the band gap between the engine slot and the safe band. A fixture above the safe band would split before this change and prove nothing. Choosing the fixture so that only the defect distinguishes it is the whole skill, and it is rarer than mutation-testing itself.

🎯 Close-Target Audit

• Close-targets identified: #17343
• Confirmed not epic-labeled — carries bug, ai, architecture, agent-os

Findings: Pass.

N/A Audits — 📑 📡 🔗 🛂

N/A: no consumed-contract surface beyond an internal resolver, no OpenAPI, no skill/convention change, no novel abstraction — this consolidates three copies of an existing rule.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 43dcd0beb9, 24/24, verified live. Author receipt: 23 specs importing the three changed modules, 448 passed.
• Reviewer falsifier: not run — RA-1 is a reachability finding, established by reading both call sites and the early-return that only one of them has. A run at this head would not surface it, since no fixture supplies a declared-invalid ceiling to the IngestionService path.
• Test location: pass — VectorService.admissionBand.spec.mjs sits beside its siblings.

Findings: Pass, with the gap RA-1 names: the mutation control covers the band comparison, not the unresolved-band routing.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1: Guard the IngestionService.filterEmbeddingInputBudget split path against an unresolved band, the way VectorService.expandOversizedEmbeddingChunks already does — currently an unresolvable band routes into splitOversizedEmbeddingChunk, whose ?? guardrail.safeProcessingLimitTokens fallback cuts against the 28,672 band this PR removes. Please also add the case to the spec: a declared-invalid ceiling through the IngestionService path, asserting the chunk is left whole rather than split.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 94 - One resolver in the module that already owns the band, consumed by all three sites, with ABSENT and INVALID given different meanings and the reason stated. 6 deducted because one caller reaches the splitter in a state the resolver declares unresolvable.
• [CONTENT_COMPLETENESS]: 98 - The drift-factor JSDoc gives the measured range, the corpus it came from, and why the margin sits above rather than on the worst case. The receipt emits the derived figures beside the leaves so a refusal is legible without re-deriving anything.
• [EXECUTION_QUALITY]: 86 - Mutation-verified with controls proven non-discriminating, and a red-proof fixture placed in the band gap where only the defect distinguishes it. 14 deducted for RA-1 and for skip carrying two meanings at the IngestionService site.
• [PRODUCTIVITY]: 96 - Closes both faults the ticket measured, at every site, with the boundary to #11735 and #17345 stated rather than discovered.
• [IMPACT]: 88 - Unblocks ingestion on a deployment that cannot complete it today, and retires a rule that existed in three drifting copies.
• [COMPLEXITY]: 62 - Small diff over subtle semantics: two ceilings in two units across three call sites, where the hard part was establishing the units disagree at all.
• [EFFORT_PROFILE]: Heavy Lift - 276 lines, but the weight is the tokenizer measurement against the real GGUF that turned a hypothesis into a number.

The ticket's evidence table is the best V-B-A artifact I have read this week: actual/estimate per chunk against the deployment's own model, rather than an argument that estimates and tokenizers differ. RA-1 is one guard away.

🖖 Grace (Claude Opus 5, Claude Code) · session ad99f59b-9d2c-4f82-b6ce-8c8357ef1879

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 1
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z