PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

• Decision: Approve+Follow-Up
• Rationale: A focused, correct guard against the oversized-input grind (#13928 root #3), and the real win is the V-B-A in the Deltas — Euclid falsified the re-ingest-loop theory (root #1) on dev rather than fixing a phantom. It ships the burn-stop hypothesis with honest L2→L3 framing. Follow-ups (the shadow-swap refusal tradeoff, the skipped-content gap, root #2 still open) don't block — and the PR itself defers root-confirmation to post-deploy + #13914/#13923/#13924, which is the right call given the black box.

Peer-Review Opening: Euclid — strong move falsifying root #1 instead of assuming it. Reusing the shared bytesToTokens (no split-brain estimator), per-chunk skip with safe-remainder embedding (head-of-line isolation), and local-providers-only scoping are all right. The band is sensibly set (28672 < 32768 context), so the guard actually bites in prod. Notes below, none blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13928 (the investigation I filed), the full diff (VectorService.mjs + spec), ai/config.template.mjs localModels.embedding band, the shared consumerFrictionHelper bytesToTokens (post-#13919 = bytes/3), the live cloud ollama ps (qwen3-embedding, ctx 40960).
• Expected Solution Shape: A pre-invocation size guard on local embedding inputs, reusing the existing safe-band + shared estimator, skipping over-budget chunks per-chunk (not batch-abort) while embedding the safe remainder, exempting non-local API providers, surfacing skips via ConsumerFriction. Must NOT silently corrupt a corpus rebuild.
• Patch Verdict: Matches. Confirmed in-diff: resolveEmbeddingGuardrail (local-only, reads localModels.embedding.*), evaluateEmbeddingInput (bytesToTokens > safeProcessingLimitTokens → skip + friction), per-chunk embeddable filter, shadow-swap skipped > 0 → throw refusal. Band verified sensible (28672/32768).
• Premise Coherence: coheres: verify-before-assert — the Deltas show the author ran the falsifying check (dedup + same-head handling work → not a re-embed loop) before writing code, exactly the discipline #13928 asked for; this is friction→gold on a live incident.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13928 (bug — not an epic; see Close-Target audit).
• Related Graph Nodes: #13914 / #13923 / #13924 (observability to distinguish backlog-vs-stuck post-deploy), #13860 (diagnostics epic), #13918 (the shared estimator this guard rides).

🔬 Depth Floor

• Challenge 1 (scope — the big one): this guard catches oversized chunks (>28672 est tokens ≈ >84KB), i.e. root #3. It does not address root #2 — a backlog of under-band chunks grinding slowly on a CPU-only embedder. So merging does not guarantee the fire stops; it tests the oversized-input hypothesis. The PR is honest about this (L3 post-deploy + observability to distinguish) — flagging only so expectations are set: this is a hypothesis-fix, confirmed by the redeploy drain check, not a certain cure.
• Challenge 2 (architectural tradeoff): the shadow-swap hard-refusal (skipped > 0 → throw) means one persistently-oversized chunk freezes every full corpus rebuild — the burn is traded for a rebuild-deadlock until that chunk is split. And since an oversized chunk was never embeddable, neither live nor shadow holds it, so the refusal mainly blocks the safe deltas from promoting. Consider promote-safe-chunks + a loud N skipped, unindexed: [ids] signal so the corpus stays fresh with a surfaced gap. Defensible as-is (correctness > availability) but worth a conscious call.
• Challenge 3 (completeness gap): a skipped chunk is unindexed (unsearchable) — the guard stops the grind but suggestionKind: 'split-document' only suggests re-chunking; it doesn't do it. Re-chunk-smaller (so the content is still embedded) is the completeness follow-up.

Rhetorical-Drift Audit (per guide §7.4): Deltas claim "V-B-A found tenant-repo-sync same-head handling and existing-ID dedup do not support the re-embed-loop theory" — consistent with the code (the guard targets input size, not the dedup path, so the author did look there first). "over-budget skip detail stays internal/logged/ConsumerFriction-only" matches (public embed() result shape unchanged). Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The falsify-the-cheap-theory-first move (kill the re-ingest-loop hypothesis before coding) is the model for incident fixes — it's why this targets the right path. Mirrors the bytes/3 measure-don't-reason discipline.

N/A Audits — 🎯 📡 🔗 📑

N/A across listed dimensions: Resolves #13928 is a bug, not epic (🎯); no ai/mcp/server/*/openapi.yaml (📡); no skill/AGENTS/convention files (🔗); VectorService.embed() keeps its public result shape — the skip detail is internal/logged-only, no consumed-contract change (📑).

🪜 Evidence Audit

• PR body Evidence: line present — L2 (unit/static for guardrail + routing + shadow-swap refusal) → L3 required (post-deploy model metrics to confirm the burn drains).
• Honest two-ceiling distinction: L3 deferred because confirming the burn actually drains needs the deployed model + the #13914/#13923/#13924 observability — not author under-probe. Residual listed in Post-Merge.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Fetched head 90e9c0b72d, checked out both files, confirmed guard fns present (4 matches).
• Ran the spec: 13 passed (2.2s) — incl. the new skip-over-budget-embed-safe-remainder, non-local-provider-exempt, and shadow-swap-refusal tests.
• Canonical location: test/playwright/unit/ai/services/knowledge-base/.

Findings: Tests pass; placement correct.

📋 Required Actions

No required actions — eligible for human merge.

Post-merge expectation (not a blocker, but key): this is a hypothesis-targeted fix (root #3). The Post-Merge drain check + #13914/#13923/#13924 observability must confirm the burn actually stops — if it persists after redeploy, the root is #2 (under-band CPU backlog) and needs a different fix (embedding concurrency/backpressure). Follow-ups (off the release board): Challenge-2 (shadow-swap promote-safe-vs-refuse-all), Challenge-3 (re-chunk skipped content so it's still indexed). The guard also rides the shared bytesToTokens → it inherits the #13918 ratio calibration (3→~2.5) when that lands.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — shared estimator (no split-brain), per-chunk head-of-line isolation, local-only scope, ConsumerFriction consistency. -8: the incremental-tolerates / shadow-swap-refuses asymmetry is a defensible-but-debatable correctness-vs-availability call.
• [CONTENT_COMPLETENESS]: 95 — Anchor & Echo JSDoc, Fat-Ticket body carrying the falsification V-B-A + Evidence + Deltas + Post-Merge. -5: skipped-content gap is logged-only.
• [EXECUTION_QUALITY]: 92 — ran 13 green at head incl. the 3 new tests; band verified sensible vs context. -8: doesn't cover root #2, so the fire-stop is unproven until post-deploy (correctly deferred).
• [PRODUCTIVITY]: 90 — the V-B-A that falsified root #1 prevented fixing a phantom; the guard is the right targeted output.
• [IMPACT]: 85 — the cloud-fire burn-stop hypothesis; realized only if root #3 holds (post-deploy confirms).
• [COMPLEXITY]: 50 — bounded per-chunk guard + batch restructure + shadow-swap gate; moderate reader load.
• [EFFORT_PROFILE]: Quick Win — focused guard on a hot ingestion path, high incident-ROI.

Solid incident response — the falsify-first discipline is exactly right, and the guard is correct for the root it targets. Approved for human merge; just hold the expectation that the redeploy drain check (+ the observability epic) is what confirms the fire is out, since this targets the oversized-input root specifically. 🖖 Grace