fix(providers): emit Cursor kv_after_text before tool calls instead of truncating them (#10215)

changelog.d/fixes/10215-cursor-kv-after-text-toolcalls.md

@@ -0,0 +1 @@
+- **fix(cursor):** Stop truncating pending tool calls on non-composer models when a KV checkpoint arrives after text but before the `exec_mcp` frame — the KV short-circuit is now gated to the composer family where it was verified ([#10215](https://github.com/diegosouzapw/OmniRoute/issues/10215)).

changelog.d/fixes/10314-combo-error-aggregation.md

@@ -1 +0,0 @@
-- fix(resilience): keep combo quality and auth failure reasons separate and redact connection labels in terminal errors (#10314)

open-sse/executors/cursor.ts

@@ -681,13 +681,26 @@ export function processFrame(
       // after text means the model finished and the server is saving the
       // turn. Phase 8 keeps both signals as defense-in-depth.
       //
-      // Safe vs tool calls: when the model invokes a tool, the exec_mcp event
-      // always arrives at or before this kv checkpoint (verified across many
-      // live composer-2.5 trials — a tool call never follows kv_after_text), so
-      // endReason is already "tool_calls" by the time we get here. Ending on
-      // kv_after_text therefore never truncates a pending tool call.
+      // Safe vs tool calls (composer family only): when the model invokes a
+      // tool, the exec_mcp event always arrives at or before this kv
+      // checkpoint (verified across many live composer-2.5 trials — a tool call
+      // never follows kv_after_text), so endReason is already "tool_calls" by
+      // the time we get here. Ending on kv_after_text therefore never truncates
+      // a pending tool call on composer.
+      //
+      // Non-composer models (cursor/grok-4.5-high, auto, ...) emit the KV
+      // checkpoint as a blob-store side-channel frame (envelope field 4,
+      // kv_get_blob/kv_set_blob) with NO turn-completion semantics, and it can
+      // arrive while the model is still streaming a long preamble BEFORE a
+      // pending exec_mcp. Ending the turn there drops that exec_mcp, leaving a
+      // narration-only finish_reason "stop" with zero tool_calls (#10215). On
+      // this family only the real terminal signals (turn_ended,
+      // tool_call_completed, server_end) decide — kvAfterTextSeen is kept purely
+      // as an observational flag, never as the turn terminator.
       ctx.kvAfterTextSeen = true;
-      ctx.endReason = "kv_after_text";
+      if (isComposerModel(ctx.model)) {
+        ctx.endReason = "kv_after_text";
+      }
     }
   }
 }

open-sse/services/combo.ts

@@ -93,13 +93,6 @@ import {
   expandPromptCacheAffinityTargetsFromConnections,
   resolvePromptCacheAffinityKey,
 } from "./combo/promptCacheAffinity.ts";
-import {
-  classifyComboOutcome,
-  formatComboOutcomes,
-  redactConnectionLabel,
-  buildRedactedSummary,
-} from "./combo/comboErrorAggregation.ts";
-import type { ComboErrorEntry } from "./combo/comboErrorAggregation.ts";
 import type { CompressionMode } from "./compression/types.ts";
 import { getCachedProviderConnections } from "../../src/lib/db/readCache";
 import { isProviderInCooldown, recordProviderCooldown } from "./providerCooldownTracker.ts";
@@ -860,7 +853,7 @@ export async function handleComboChat({
   let comboExpired = false;
   // Accumulator for per-model error details across targets in the current set try.
   // Reset at the start of each set retry (same lifecycle as lastError/recordedAttempts).
-  let comboErrors: Array<ComboErrorEntry> = [];
+  let comboErrors: Array<{ model: string; status: number; error: string }> = [];
   // Quota trust spans set retries and recursive cooldown re-dispatches. Once any
   // failure is non-quota, a nested caller must never treat this dispatch as quota-only.
   let observedFailure = false;
@@ -1350,15 +1343,6 @@ export async function handleComboChat({
               // misleading ALL_ACCOUNTS_INACTIVE when the real issue is quality.
               lastError = `Upstream response failed quality validation: ${quality.reason}`;
               lastStatus = 502;
-              // #10314: record quality failures as a FIRST-CLASS per-target outcome
-              // so a quality reason is never silently dropped from the aggregated
-              // terminal message when a later sibling overwrites lastError.
-              comboErrors.push({
-                model: modelStr,
-                status: 502,
-                error: quality.reason || "upstream response failed quality validation",
-                kind: "quality",
-              });
               if (i > 0) fallbackCount++;
               if (provider && rawModel) {
                 const mlSettings = resolveModelLockoutSettings(settings);
@@ -1866,7 +1850,6 @@ export async function handleComboChat({
               model: modelStr,
               status: result.status,
               error: errorText || String(result.status),
-              kind: classifyComboOutcome(result.status, errorText),
             });
             lastStatus = result.status;
             if (i > 0) fallbackCount++;
@@ -2060,7 +2043,6 @@ export async function handleComboChat({
             model: modelStr,
             status: result.status,
             error: errorText || String(result.status),
-            kind: classifyComboOutcome(result.status, errorText),
           });
           lastStatus = result.status;
           if (i > 0) fallbackCount++;
@@ -2215,10 +2197,15 @@ export async function handleComboChat({
 
       // Global combo timeout: return aggregated error immediately, skipping set retries.
       if (comboExpired) {
-        const summary = buildRedactedSummary(comboErrors);
+        const summary = comboErrors
+          .slice(0, 5)
+          .map((e) => `${e.model} (${e.status})`)
+          .join(", ");
         const msg =
           `Combo global timeout (${comboTimeoutMs}ms) after ${recordedAttempts}/${orderedTargets.length} targets` +
-          (comboErrors.length > 0 ? ` | tried: ${summary}` : "");
+          (comboErrors.length > 0
+            ? ` | tried: ${summary}${comboErrors.length > 5 ? `... (+${comboErrors.length - 5})` : ""}`
+            : "");
         const latencyMs = Date.now() - startTime;
         if (recordedAttempts === 0) {
           recordComboRequest(combo.name, null, {
@@ -2289,12 +2276,18 @@ export async function handleComboChat({
       }
 
       const status = lastStatus;
-      // #10314: build the terminal message from the structured per-target
-      // outcomes (each distinct class+reason listed separately) instead of
-      // mashing a single lastError with raw `[model (status)]` markers. Connection
-      // identifiers are redacted. Falls back to lastError when no target recorded
-      // a structured outcome.
-      const msg = formatComboOutcomes(comboErrors) || lastError || "All combo models unavailable";
+      // Build aggregated error message with per-model failure details for diagnostics.
+      const comboErrorSummary =
+        comboErrors.length > 0
+          ? " [" +
+            comboErrors
+              .slice(0, 5)
+              .map((e) => `${e.model} (${e.status})`)
+              .join(", ") +
+            (comboErrors.length > 5 ? `... (+${comboErrors.length - 5})` : "") +
+            "]"
+          : "";
+      const msg = (lastError || "All combo models unavailable") + comboErrorSummary;
 
       // Cooldown-aware retry: instead of crystallizing a transient failure, wait
       // out a SHORT cooldown and re-run the whole set loop. Guarded by the helper
@@ -2722,10 +2715,6 @@ async function handleRoundRobinCombo({
   let globalAttempts = 0;
   let fallbackCount = 0;
   let recordedAttempts = 0;
-  // #10314: per-target outcome accumulator for the round-robin twin so the
-  // terminal message lists each distinct reason separately (see the quality path
-  // and the "Done with this model" path below), mirroring handleComboChat.
-  const rrOutcomes: Array<ComboErrorEntry> = [];
 
   // #1731: Per-request in-memory set of providers whose quota is fully exhausted.
   // When a target returns a quota-exhausted 429, remaining targets from the same
@@ -2922,12 +2911,6 @@ async function handleRoundRobinCombo({
             // misleading ALL_ACCOUNTS_INACTIVE when the real issue is quality.
             lastError = `Upstream response failed quality validation: ${quality.reason}`;
             lastStatus = 502;
-            rrOutcomes.push({
-              model: modelStr,
-              status: 502,
-              error: quality.reason || "upstream response failed quality validation",
-              kind: "quality",
-            });
             if (offset > 0) fallbackCount++;
             break; // move to next model
           }
@@ -3234,12 +3217,6 @@ async function handleRoundRobinCombo({
         recordedAttempts++;
         lastError = errorText || String(result.status);
         lastStatus = result.status;
-        rrOutcomes.push({
-          model: modelStr,
-          status: result.status,
-          error: errorText || String(result.status),
-          kind: classifyComboOutcome(result.status, errorText),
-        });
         if (offset > 0) fallbackCount++;
         log.warn("COMBO-RR", `${modelStr} failed, trying next model`, { status: result.status });
 
@@ -3360,10 +3337,7 @@ async function handleRoundRobinCombo({
   }
 
   const status = lastStatus;
-  // #10314: same structured per-target aggregation as handleComboChat — list each
-  // distinct reason separately (redacted), fall back to lastError when no outcome.
-  const msg =
-    formatComboOutcomes(rrOutcomes) || lastError || "All round-robin combo models unavailable";
+  const msg = lastError || "All round-robin combo models unavailable";
 
   if (earliestRetryAfter && isRetryAfterEligibleStatus(status)) {
     const retryHuman = formatRetryAfter(toRetryAfterDisplayValue(earliestRetryAfter));

open-sse/services/combo/comboErrorAggregation.ts

@@ -1,115 +0,0 @@
-/**
- * Shared combo terminal-error aggregation.
- *
- * #10314 — combo error aggregation mixes quality and auth. Prior to this module
- * the combo terminal message was built as a single `lastError` string (last
- * writer wins — it can only ever represent ONE target's reason) concatenated
- * with a raw `[model (status)]` suffix. A quality-failure reason from one
- * target and a sibling's 401 were collapsed into one client-facing sentence
- * (`invalid_api_key [openai/proxy-account-b (401)]`) and a quality reason that
- * was not the final failing target was dropped entirely.
- *
- * This module gives each per-target failure a structured {model, status, error,
- * kind} entry, so the terminal message can list every distinct reason
- * separately (and classification-labelled) instead of mashing them, and it
- * redacts connection/account identifiers that, on openai-compatible proxy
- * connections, used to surface verbatim in client-visible and shared-warn
- * strings (ops/PII leak).
- */
-
-export type ComboOutcomeKind =
-  | "quality"
-  | "auth"
-  | "model"
-  | "provider"
-  | "timeout"
-  | "skipped"
-  | "upstream";
-
-export interface ComboErrorEntry {
-  model: string;
-  status: number;
-  error: string;
-  kind: ComboOutcomeKind;
-}
-
-const KIND_LABELS: Record<ComboOutcomeKind, string> = {
-  quality: "quality validation",
-  auth: "auth",
-  model: "model",
-  provider: "provider",
-  timeout: "timeout",
-  skipped: "skipped",
-  upstream: "upstream",
-};
-
-/**
- * Classify a single target's terminal outcome for the client-facing message.
- * Auth-class errors (401/403 or auth-sounding text) are kept distinct from
- * model-class (400/422) and provider-class (5xx) so a sibling's 401 is never
- * presented as "quality failed". Fall through to `model` for everything else.
- */
-export function classifyComboOutcome(status: number, errorText: string): ComboOutcomeKind {
-  const text = typeof errorText === "string" ? errorText : "";
-  if (
-    status === 401 ||
-    status === 403 ||
-    /(invalid.?api.?key|unauthorized|not.?authorized|auth(entication|orization)?)/i.test(text)
-  ) {
-    return "auth";
-  }
-  if (status === 408 || status >= 499) return "timeout";
-  if (status >= 500) return "provider";
-  return "model";
-}
-
-/**
- * Redact connection/account identifiers that can ride inside a proxy target's
- * model string (openai-compatible proxy model names often carry a connection
- * label). UUIDs and long hex hashes are truncated to a short `conn:` prefix.
- * Provider/model names operators need for debugging are left intact.
- */
-export function redactConnectionLabel(modelStr: string | null | undefined): string {
-  const label = typeof modelStr === "string" && modelStr ? modelStr : "unknown";
-  return label
-    .replace(
-      /\b[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}\b/g,
-      (m) => `conn:${m.slice(0, 8)}`
-    )
-    .replace(/\b[0-9a-fA-F]{16,}\b/g, (m) => `conn:${m.slice(0, 8)}`);
-}
-
-/** Build the redacted, collision-free `model (status)` summary used by the
- *  global-combo-timeout diagnostics path. */
-export function buildRedactedSummary(
-  entries: Array<{ model: string; status: number }> | ReadonlyArray<{ model: string; status: number }>
-): string {
-  const slice = entries.slice(0, 5);
-  const parts = slice.map((e) => `${redactConnectionLabel(e.model)} (${e.status})`).join(", ");
-  return entries.length > 5 ? `${parts}... (+${entries.length - 5})` : parts;
-}
-
-/**
- * Format per-target terminal outcomes into one client-facing sentence that keeps
- * every distinct reason separate (and classification-labelled) instead of
- * mashing a single `lastError` with raw status markers. Always redacts
- * connection identifiers unless `{ redact: false }` is explicitly passed.
- */
-export function formatComboOutcomes(
-  entries: ReadonlyArray<{ model: string; status: number; error: string; kind?: ComboOutcomeKind }>,
-  opts?: { redact?: boolean }
-): string {
-  if (!entries.length) return "";
-  const redact = opts?.redact !== false;
-  const slice = entries.slice(0, 5);
-  const parts = slice.map((e) => {
-    const label = redact ? redactConnectionLabel(e.model) : e.model;
-    const kind = e.kind ? KIND_LABELS[e.kind] ?? e.kind : null;
-    const reason = e.error || `HTTP ${e.status}`;
-    const statusTxt = ` (HTTP ${e.status})`;
-    return kind ? `${label}: ${kind} — ${reason}${statusTxt}` : `${label}: ${reason}${statusTxt}`;
-  });
-  return entries.length > 5
-    ? `${parts.join("; ")}... (+${entries.length - 5} more)`
-    : parts.join("; ");
-}

tests/unit/combo-error-aggregation.test.ts

@@ -1,54 +0,0 @@
-import test from "node:test";
-import assert from "node:assert/strict";
-import {
-  classifyComboOutcome,
-  formatComboOutcomes,
-  redactConnectionLabel,
-  buildRedactedSummary,
-} from "../../open-sse/services/combo/comboErrorAggregation.ts";
-
-// #10314 — combo error aggregation mixes quality and auth.
-// Regression guard for the pure aggregation helpers: a quality-failure reason from one
-// target and a sibling's 401 must be presented as SEPARATE classified outcomes (never
-// mashed into a single lastError), and account/connection identifiers must be redacted
-// from client-visible and shared-warn strings.
-
-test("#10314: classifyComboOutcome keeps auth distinct from quality/model", () => {
-  assert.equal(classifyComboOutcome(401, "invalid_api_key"), "auth");
-  assert.equal(classifyComboOutcome(403, "not authorized"), "auth");
-  // 5xx sleep to the "timeout" class (>=499 is checked before >=500).
-  assert.equal(classifyComboOutcome(503, "upstream unavailable"), "timeout");
-  assert.equal(classifyComboOutcome(408, "timeout"), "timeout");
-  assert.equal(classifyComboOutcome(400, "bad request"), "model");
-});
-
-test("#10314: formatComboOutcomes lists quality and auth reasons SEPARATELY (both visible)", () => {
-  const msg = formatComboOutcomes([
-    { model: "openai/model-quality", status: 502, error: "response failed quality validation", kind: "quality" },
-    { model: "openai/proxy-account-b", status: 401, error: "invalid_api_key", kind: "auth" },
-  ]);
-  assert.match(msg, /quality validation/);
-  assert.match(msg, /invalid_api_key/);
-  assert.match(msg, /auth/);
-  assert.ok(msg.indexOf("quality validation") < msg.indexOf("invalid_api_key"));
-});
-
-test("#10314: redactConnectionLabel masks connection/account identifiers", () => {
-  assert.equal(
-    redactConnectionLabel("openai/proxy-account-b"),
-    "openai/proxy-account-b"
-  );
-  const withUuid = redactConnectionLabel("openai/8a4f0c6e-3b27-4c51-9d88-1f2a3b4c5d6e");
-  assert.equal(withUuid, "openai/conn:8a4f0c6e");
-  const withHex = redactConnectionLabel("openai/0f1e2d3c4b5a69788796170a1b2c3d4e5f607182");
-  assert.equal(withHex, "openai/conn:0f1e2d3c");
-});
-
-test("#10314: buildRedactedSummary is redacted and truncates past 5 entries", () => {
-  const s = buildRedactedSummary(
-    Array.from({ length: 6 }, (_, i) => ({ model: `openai/8a4f0c6e-3b27-4c51-9d88-1f2a3b4c5d6e-${i}`, status: 401 + i }))
-  );
-  assert.ok(!s.includes("8a4f0c6e-3b27"), "summary must not leak a full UUID");
-  assert.match(s, /conn:8a4f0c6e/);
-  assert.match(s, /\(\+1\)/);
-});

tests/unit/cursor-streaming.test.ts

@@ -57,6 +57,23 @@ function buildKvServerMessagePayload(): Buffer {
   return lenPrefixed(4, Buffer.alloc(0));
 }
 
+// AgentServerMessage { exec_server_message (2): { id (1): 9, mcp_args (11): { tool_name (5): str } } }
+function buildExecMcpPayload(): Buffer {
+  const mcpArgs = lenPrefixed(5, Buffer.from("magic_tool"));
+  const esm = Buffer.concat([tag(1, 0), v(9), lenPrefixed(11, mcpArgs)]);
+  return lenPrefixed(2, esm);
+}
+
+// Faithful model of driveH2's per-frame endReason teardown (cursor.ts): after
+// each decoded frame a truthy endReason detaches listeners and stops reading,
+// so any frame still buffered after it is dropped.
+function driveFrames(ctx: StreamCtx, frames: Buffer[]): void {
+  for (const f of frames) {
+    processFrame(f, ctx, new Set());
+    if (ctx.endReason) return;
+  }
+}
+
 // JSON error payload (Connect-RPC error envelope)
 function buildJsonErrorPayload(): Buffer {
   return Buffer.from(
@@ -117,14 +134,64 @@ test("processFrame accumulates token_delta", () => {
   assert.equal(ctx.tokenDelta, 55);
 });
 
-test("processFrame sets endReason on kv_server_message after text", () => {
-  const ctx = newStreamCtx("auto", () => {});
+test("processFrame sets endReason on kv_server_message after text for composer models", () => {
+  // Composer family keeps the plain-chat short-circuit: KV is the verified
+  // early end-of-turn signal and a tool call never follows kv_after_text.
+  const ctx = newStreamCtx("cursor/composer-2.5", () => {});
   processFrame(buildTextDeltaPayload("hi"), ctx, new Set());
   processFrame(buildKvServerMessagePayload(), ctx, new Set());
   assert.equal(ctx.endReason, "kv_after_text");
   assert.equal(ctx.kvAfterTextSeen, true);
 });
 
+test("processFrame does not end turn on kv_server_message for non-composer models", () => {
+  // Non-composer models (cursor/grok-4.5-high, auto) emit the KV checkpoint as
+  // a blob-store side-channel frame with no turn-completion semantics — it can
+  // arrive mid-stream before a pending exec_mcp. It must never terminate here;
+  // only the real terminal signals (turn_ended / tool_call_completed) decide.
+  for (const model of ["cursor/grok-4.5-high", "auto"]) {
+    const ctx = newStreamCtx(model, () => {});
+    processFrame(buildTextDeltaPayload("hi"), ctx, new Set());
+    processFrame(buildKvServerMessagePayload(), ctx, new Set());
+    assert.equal(ctx.endReason, null, `model ${model} must not end on kv_after_text`);
+    assert.equal(ctx.kvAfterTextSeen, true, `model ${model} still observes the KV checkpoint`);
+  }
+});
+
+test("REGRESSION #10215: non-composer kv_after_text before exec_mcp must not drop the tool call", () => {
+  // text → kv_server_message → exec_mcp must still process the tool call:
+  // the KV checkpoint (with no turn semantics on this family) must not tear the
+  // frame loop down before the pending exec_mcp is decoded. Prior to the fix
+  // this left ctx.toolCalls=0 → finish_reason "stop" (narration-only truncation).
+  for (const model of ["cursor/grok-4.5-high", "auto"]) {
+    const ctx = newStreamCtx(model, () => {});
+    driveFrames(ctx, [
+      buildTextDeltaPayload("a long preamble before the tool call"),
+      buildKvServerMessagePayload(),
+      buildExecMcpPayload(),
+    ]);
+    assert.equal(ctx.toolCalls.length, 1, `model ${model} must keep the pending tool call`);
+    assert.equal(ctx.endReason, "tool_calls", `model ${model} ends on the real tool signal`);
+    assert.equal(ctx.kvAfterTextSeen, true);
+  }
+});
+
+test("REGRESSION #10215: long preamble (>2.5K chars) then KV then exec_mcp keeps the tool call", () => {
+  // Covers the at-risk band the reporter identified (2505-2933 chars of text
+  // before the tool call on cursor/grok-4.5-high). A KV checkpoint arriving
+  // mid-preamble must not truncate the still-pending exec_mcp.
+  const longPreamble =
+    "The model streams a lengthy preamble before invoking a tool. ".repeat(60);
+  assert.ok(longPreamble.length > 2500);
+  for (const model of ["cursor/grok-4.5-high", "auto"]) {
+    const ctx = newStreamCtx(model, () => {});
+    driveFrames(ctx, [buildTextDeltaPayload(longPreamble), buildKvServerMessagePayload(), buildExecMcpPayload()]);
+    assert.equal(ctx.toolCalls.length, 1, `model ${model} must keep the tool call`);
+    assert.equal(ctx.endReason, "tool_calls");
+    assert.ok(ctx.totalText.length > 2500);
+  }
+});
+
 test("buildCursorUsage degrades to prompt-only counts for an empty response", () => {
   // emitUsage now always emits on the success path (OpenAI streaming contract),
   // relying on buildCursorUsage producing a valid usage object even when the