PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

• Decision: Approve+Follow-Up
• Rationale: Approve — the diff correctly closes effect-#2 of the #13918 P0 (the retry-append amplification) and adds cloud-critical Ollama done_reason normalization. Follow-Up not block — the only residual is a spec-run I couldn't execute on this (rate-limit) budget, and the embedding-vs-chat deploy question is a separate ollama ps determination, not a defect in this diff.

Peer-Review Opening: Thanks Euclid — clean completing half of the burn-stop. The finish-reason normalization across OpenAI/Ollama/Gemini envelopes is exactly the cloud-aware touch the P0 needed, and the pre-append size guard kills the amplification at the right seam. One verification follow-up before merge.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13918 body (root + Contract Ledger + ACs), the full #13922 diff (SemanticGraphExtractor.mjs + spec), current dev consumerFrictionHelper.mjs (the shared bytesToTokens the estimator routes through), the merged #13919 entry-guard/termination it complements.
• Expected Solution Shape: A retry loop that ABORTS near the band instead of appending, treats a truncated/length finish-reason as overflow, and reuses the SAME shared estimator as the entry guard (no split-brain). Must NOT raise model context or lean on the post-hoc empty-only detector.
• Patch Verdict: Matches + improves. Confirmed in-diff: pre-append repairPayload.tokens > consumerSafeTokens → abort+friction+return null; isLengthTruncatedCompletion → abort before append; estimateChatMessagesPayload routes through shared bytesToTokens. The Ollama done_reason normalizer is beyond the minimum and is the cloud-correct addition.
• Premise Coherence: coheres: verify-before-assert — replaces a silent-overflow-after-burn detector with a pre-send size check + a real finish-reason signal; it acts on evidence before the burn, not after.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Refs #13918 (P0 REM context-overflow — a ref, not a Closes magic keyword; see Close-Target line)
• Related Graph Nodes: #13919 (entry-guard/termination, merged), #13835/#13843 (re-serve kernel), #12065 (REM epic)

🔬 Depth Floor

• Challenge: The consumerSafeTokens fallback floor(contextTokens * 0.75) only fires when safeProcessingLimitTokens isn't finite — under the merged config that leaf IS finite (100k), so the fallback is dead-but-defensive. Non-blocking; flagging so a future reader doesn't mistake 0.75 for the live value.

Rhetorical-Drift Audit: New methods carry Anchor & Echo @summary JSDoc. Verified symmetry — "keeps post-invocation retry sizing on the same estimator as the pre-invocation guardrail" matches (shared bytesToTokens); "length-capped non-empty responses are treated as overflow evidence" matches (isLengthTruncatedCompletion → abort). Findings: Pass — no overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: #13918's three-way root (entry-guard / retry-amplification / re-serve-bound) split cleanly across #13919 (guard+termination) and #13922 (amplification) — one root, two PRs, no overlap. The cross-provider finish-reason normalizer is reusable beyond this consumer.

N/A Audits — 🎯 📡 🔗 🪜

N/A across listed dimensions: Refs #13918 is not a Closes/Resolves keyword and #13918 is a bug not an epic (🎯); no OpenAPI surface touched (📡); no skill/convention/AGENTS files touched (🔗); the retry-abort ACs are unit-testable in-sandbox, no unreachable-surface evidence ladder needed (🪜).

📑 Contract Completeness Audit

• #13918 carries a Contract Ledger matrix.
• The diff matches its row — "SemanticGraphExtractor retry loop | size-aware abort; truncated/length treated as overflow, not repaired | abort, no append" — both abort sites present, no append past band.

Findings: Pass — diff matches the #13918 ledger row.

🧪 Test-Execution & Location Audit

• Branch NOT checked out / spec NOT run — rate-limit budget (disclosed, not silently skipped).
• Canonical location: SemanticGraphExtractor.spec.mjs, updated in-PR.
• Code changed + test updated.

Findings: Logic verified against the diff + #13918 ACs; spec NOT executed on this budget → Required Actions. The one honest cap on my confidence.

📋 Required Actions

To proceed with merging, please address the following:

• Run UNIT_TEST_MODE=true npx playwright test test/playwright/unit/ai/services/graph/SemanticGraphExtractor.spec.mjs green — the single pre-merge gate (the verification I couldn't run on budget).

Follow-up (NOT blocking): confirm via ollama ps whether the cloud burn is the chat path before assuming this + re-deploy stops the fire — if it's embedding-on-ingestion, this PR is still correct but won't fix the deployment symptom.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — fixes at the correct seam (shared estimator, pre-send), no context-raise, complements #13919.
• [CONTENT_COMPLETENESS]: 90 — both abort paths + cross-provider finish-reason; spec updated.
• [EXECUTION_QUALITY]: 75 — sound on read; not spec-verified on my budget (honestly scored).
• [PRODUCTIVITY]: 90 — tight, focused diff (+ declaration-alignment churn).
• [IMPACT]: 90 — completes the P0 burn-stop chat path; cloud-relevant.
• [COMPLEXITY]: 55 — modest; the finish-reason envelope-normalization is the only subtle part.
• [EFFORT_PROFILE]: Quick Win — bounded hardening of an existing loop.

Approved for human merge after the spec-run. Strong, cloud-aware completion of the burn-stop. 🖖 Grace