test(combo): cover skipProviderBreaker consumer gate (#2743 gap d) (#3832)

CHANGELOG.md

@@ -9,6 +9,7 @@
 ### 🐛 Fixed
 
 - **fix(intelligence): run pricing + models.dev sync from the live startup path** — like the Arena ELO sync (v3.8.24), the external **pricing sync** (`PRICING_SYNC_ENABLED`) and the **models.dev capability sync** (Settings → AI toggle) were only initialized from `server-init.ts`, which the Next standalone runtime never executes — and models.dev had no caller at all. Their toggles were inert in production. Both are now initialized from `instrumentation-node.ts` (self-gated, opt-in preserved, non-blocking, never fatal). (thanks @diegosouzapw)
+- **test(combo): cover the `skipProviderBreaker` consumer gate** — the producer was tested but the consumer (whether a failed combo target trips the whole-provider circuit breaker) was not; the breaker decision is now an exported pure predicate (`shouldRecordProviderBreakerFailure`, behaviour-identical) with direct tests asserting a `connection_cooldown` 503 does not trip the breaker while a plain 503 does. Closes another deferred test gap from [#2743](https://github.com/diegosouzapw/OmniRoute/issues/2743). (thanks @diegosouzapw)
 - **fix(providers): surface the real Devin error + correct the Windsurf auth instructions** — Devin chat returned a generic 502 "Invalid SSE response for non-streaming request" that swallowed the real cause (e.g. "Devin CLI not found"): an error-only SSE chunk (no `choices`) is now propagated with its sanitized message. The Windsurf "Visit windsurf.com/show-auth-token" instruction (the bare URL shows no token without an IDE-supplied `?state=`) now directs users to the `Windsurf: Provide Auth Token` command-palette flow. ([#3324](https://github.com/diegosouzapw/OmniRoute/issues/3324) — thanks @mikmaneggahommie)
 - **fix(grok-web): clearer 403 message for anti-bot / IP-reputation blocks** — a Grok Web subscription validating from a flagged datacenter/VPS IP got a 403 that read like an invalid cookie, sending users to chase a cookie that was actually fine. A non-auth 403 (Cloudflare challenge / anti-bot body) now returns a message stating the cookie is likely OK and the block is IP-reputation-based — retry from a residential IP or configure a proxy (auth-shaped 403s keep the re-paste guidance). ([#3474](https://github.com/diegosouzapw/OmniRoute/issues/3474) — thanks @friedtofu1608)
 - **fix(db): make the mass-pending-migrations safety threshold env-overridable** — restoring a backup DB from an older version could trip "Detected N pending migrations … threshold is 50" with no way to override the hardcoded `50`. The threshold is now configurable via `OMNIROUTE_MAX_PENDING_MIGRATIONS` (resolved at startup; `0` disables the check). ([#3416](https://github.com/diegosouzapw/OmniRoute/issues/3416) — thanks @samuraiIT)

file-size-baseline.json

@@ -28,7 +28,7 @@
     "open-sse/services/batchProcessor.ts": 828,
     "open-sse/services/browserBackedChat.ts": 850,
     "open-sse/services/claudeCodeCompatible.ts": 1202,
-    "open-sse/services/combo.ts": 5131,
+    "open-sse/services/combo.ts": 5162,
     "open-sse/services/rateLimitManager.ts": 1017,
     "open-sse/services/tokenRefresh.ts": 1997,
     "open-sse/services/usage.ts": 3394,
@@ -119,5 +119,6 @@
   "_rebaseline_2026_06_13_3758_chat_early_eof": "Re-baseline #3758 (#3817 mergeado): chat.ts 1392→1425 (+33). Crescimento = retry bounded de STREAM_EARLY_EOF no handleSingleModelChat (contador streamEarlyEofRetries + bloco de retry guardado por shouldRetryStreamEarlyEof). Lógica coesa no handler de chat; não-extraível. Reconciliação tardia — o bump foi esquecido no PR do fix (o de antigravity/models foi feito).",
   "_rebaseline_2026_06_13_3416_migration_threshold": "Re-baseline #3416 (threshold de migrações pendentes via env): migrationRunner.ts 1100→1125 (+25). Crescimento = helper resolveMaxPendingMigrations() que lê OMNIROUTE_MAX_PENDING_MIGRATIONS em call-time (valida finito+>=0, fallback 50) + JSDoc. Lógica coesa de config no runner; não-extraível.",
   "_rebaseline_2026_06_13_3474_grok_403": "Re-baseline #3474 (mensagem clara no 403 anti-bot do Grok): validation.ts 4302→4348 (+46). Crescimento = helper isGrokAntiBotBlock() + branch 403 de 3 tiers (auth-shaped / anti-bot-IP-reputation / upstream-error). Lógica coesa de classificação no validator; não-extraível.",
-  "_rebaseline_2026_06_13_3324_windsurf_devin": "Re-baseline #3324 (windsurf auth text + devin error propagation): route.ts 897→903 (+6, texto da instrução windsurf→fluxo command-palette) + sseParser.ts ADICIONADO como frozen 812 (era 746, +66 = helper extractSSEErrorMessage que faz surface do erro real SSE em vez do 502 genérico). 812 fica 12 acima do cap 800 — helper coeso no parser de SSE, congelado com justificativa (precedente providerLimits/useProviderConnections)."
+  "_rebaseline_2026_06_13_3324_windsurf_devin": "Re-baseline #3324 (windsurf auth text + devin error propagation): route.ts 897→903 (+6, texto da instrução windsurf→fluxo command-palette) + sseParser.ts ADICIONADO como frozen 812 (era 746, +66 = helper extractSSEErrorMessage que faz surface do erro real SSE em vez do 502 genérico). 812 fica 12 acima do cap 800 — helper coeso no parser de SSE, congelado com justificativa (precedente providerLimits/useProviderConnections).",
+  "_rebaseline_2026_06_13_2743d_skipbreaker": "Re-baseline #2743 gap-d (testar consumer do skipProviderBreaker): combo.ts 5131→5162 (+31). Crescimento = extração do boolean inline da decisão de circuit-breaker para o predicado puro EXPORTADO shouldRecordProviderBreakerFailure() (byte-idêntico) + JSDoc, para torná-lo unit-testável sem o harness completo de combo. Shrink estrutural segue com #3501."
 }

open-sse/services/combo.ts

@@ -188,6 +188,35 @@ export function shouldSkipForPredictedTtft(
   );
 }
 
+/**
+ * Decide whether a failed combo target should record a whole-provider circuit-breaker
+ * failure (#1731 / #2743 gap-d). This is the consumer side of `skipProviderBreaker`:
+ *
+ * - Stream-readiness failures (pre-flight zombie/ping probes) never count as provider
+ *   failures — they are a connection-readiness signal, not an upstream outage.
+ * - Only provider-level failure codes (408/429/5xx — see `isProviderFailureCode`) count.
+ * - When the next combo target is on the SAME provider, don't trip the provider breaker:
+ *   a different model on that provider may still succeed.
+ * - G-02 / #2743: when the fallback result carries `skipProviderBreaker` (an embedded
+ *   service supervisor outage signalled via `X-Omni-Fallback-Hint: connection_cooldown`)
+ *   apply connection cooldown ONLY — never trip the whole-provider breaker.
+ *
+ * Pure predicate so the breaker decision is unit-testable without the full combo harness.
+ */
+export function shouldRecordProviderBreakerFailure(args: {
+  isStreamReadinessFailure: boolean;
+  status: number;
+  sameProviderNext: boolean;
+  skipProviderBreaker?: boolean;
+}): boolean {
+  return (
+    !args.isStreamReadinessFailure &&
+    isProviderFailureCode(args.status) &&
+    !args.sameProviderNext &&
+    !args.skipProviderBreaker
+  );
+}
+
 function resolveDelayMs(value: unknown, fallback: number): number {
   const numericValue = Number(value);
   if (!Number.isFinite(numericValue) || numericValue < 0) return fallback;
@@ -1207,9 +1236,7 @@ export function resolveNestedComboTargets(
 
   for (const step of runtimeSteps) {
     if (step.kind === "combo-ref") {
-      resolved.push(
-        ...expandRuntimeStep(step, allCombos, new Set(visited), depth, path, maxDepth)
-      );
+      resolved.push(...expandRuntimeStep(step, allCombos, new Set(visited), depth, path, maxDepth));
       continue;
     }
     resolved.push(step);
@@ -4348,10 +4375,12 @@ export async function handleComboChat({
           const sameProviderNext =
             typeof nextTarget?.provider === "string" && nextTarget.provider === provider;
           if (
-            !isStreamReadinessFailure &&
-            isProviderFailureCode(result.status) &&
-            !sameProviderNext &&
-            !fallbackResult.skipProviderBreaker
+            shouldRecordProviderBreakerFailure({
+              isStreamReadinessFailure,
+              status: result.status,
+              sameProviderNext,
+              skipProviderBreaker: fallbackResult.skipProviderBreaker,
+            })
           ) {
             recordProviderFailure(provider, log, target.connectionId, profile);
           }
@@ -4608,7 +4637,11 @@ async function handleRoundRobinCombo({
     ? resolveResilienceSettings(settings)
     : resolveResilienceSettings(null);
 
-  const orderedTargets = resolveComboTargets(combo, allCombos, clampComboDepth(config.maxComboDepth));
+  const orderedTargets = resolveComboTargets(
+    combo,
+    allCombos,
+    clampComboDepth(config.maxComboDepth)
+  );
   const tagFilteredTargets = await applyRequestTagRouting(orderedTargets, body, log);
   const evalRankedTargets = orderTargetsByEvalScores(tagFilteredTargets, config.evalRouting, log);
   const filteredTargets = filterTargetsByRequestCompatibility(

tests/unit/skip-provider-breaker-consumer-2743.test.ts

@@ -0,0 +1,249 @@
+import test from "node:test";
+import assert from "node:assert/strict";
+
+/**
+ * #2743 — gap d (deferred test debt): CONSUMER-side coverage for `skipProviderBreaker`.
+ *
+ * The PRODUCER is already covered (tests/unit/account-fallback-service.test.ts, the
+ * "G-02" block): `checkFallbackError` returns `skipProviderBreaker: true` for a 503 +
+ * `X-Omni-Fallback-Hint: connection_cooldown` (an embedded-service supervisor outage,
+ * NOT an upstream AI-provider failure).
+ *
+ * What was UNVERIFIED is the CONSUMER gate in `open-sse/services/combo.ts` (~line 4350):
+ * the decision that, when `skipProviderBreaker` is set, the whole-provider circuit
+ * breaker must NOT be tripped (`recordProviderFailure` is skipped) — it is a
+ * connection-level cooldown, not a provider-level failure. The producer test's final
+ * "breaker stays CLOSED" assertion is trivially true there because nothing ever calls
+ * `recordProviderFailure`, so it does not prove the consumer gate works.
+ *
+ * This test drives the consumer end of the flow:
+ *   1. the pure decision predicate (`shouldRecordProviderBreakerFailure`), and
+ *   2. its real effect — wiring the producer result THROUGH the predicate into
+ *      `recordProviderFailure` against a real circuit breaker, asserting the breaker
+ *      stays CLOSED for the skip-hint path and OPENS for the negative-control path.
+ */
+
+const combo = await import("../../open-sse/services/combo.ts");
+const accountFallback = await import("../../open-sse/services/accountFallback.ts");
+const circuitBreaker = await import("../../src/shared/utils/circuitBreaker.ts");
+
+const { shouldRecordProviderBreakerFailure } = combo;
+const {
+  checkFallbackError,
+  recordProviderFailure,
+  isProviderInCooldown,
+  getProviderBreakerState,
+  clearProviderFailure,
+} = accountFallback;
+const { resetAllCircuitBreakers } = circuitBreaker;
+
+// Profile override that makes the provider breaker open on the FIRST recorded
+// failure, so the negative control is deterministic regardless of env-tuned
+// default thresholds (apikey defaults to 12).
+const OPEN_ON_FIRST = { failureThreshold: 1, resetTimeoutMs: 60_000 } as const;
+
+test.beforeEach(() => {
+  resetAllCircuitBreakers();
+});
+
+test.after(() => {
+  resetAllCircuitBreakers();
+});
+
+// ─── 1. Pure decision predicate ──────────────────────────────────────────────
+// Mirrors the four-way gate in combo.ts: only a provider-level failure code, on a
+// target NOT followed by a same-provider target, that is NOT a stream-readiness
+// failure and NOT flagged skipProviderBreaker, records a provider-breaker failure.
+
+test("predicate: plain 503 (no skip hint) DOES record a provider-breaker failure", () => {
+  assert.equal(
+    shouldRecordProviderBreakerFailure({
+      isStreamReadinessFailure: false,
+      status: 503,
+      sameProviderNext: false,
+      skipProviderBreaker: false,
+    }),
+    true
+  );
+});
+
+test("predicate: skipProviderBreaker:true suppresses the provider-breaker failure", () => {
+  assert.equal(
+    shouldRecordProviderBreakerFailure({
+      isStreamReadinessFailure: false,
+      status: 503,
+      sameProviderNext: false,
+      skipProviderBreaker: true,
+    }),
+    false
+  );
+});
+
+test("predicate: skipProviderBreaker undefined behaves like false (records)", () => {
+  assert.equal(
+    shouldRecordProviderBreakerFailure({
+      isStreamReadinessFailure: false,
+      status: 503,
+      sameProviderNext: false,
+      // skipProviderBreaker omitted on purpose
+    }),
+    true
+  );
+});
+
+test("predicate: stream-readiness failure never records (even on a 5xx)", () => {
+  assert.equal(
+    shouldRecordProviderBreakerFailure({
+      isStreamReadinessFailure: true,
+      status: 503,
+      sameProviderNext: false,
+      skipProviderBreaker: false,
+    }),
+    false
+  );
+});
+
+test("predicate: same-provider-next suppresses (a different model may still succeed)", () => {
+  assert.equal(
+    shouldRecordProviderBreakerFailure({
+      isStreamReadinessFailure: false,
+      status: 503,
+      sameProviderNext: true,
+      skipProviderBreaker: false,
+    }),
+    false
+  );
+});
+
+test("predicate: a non-provider-failure code (e.g. 404) never records", () => {
+  assert.equal(
+    shouldRecordProviderBreakerFailure({
+      isStreamReadinessFailure: false,
+      status: 404,
+      sameProviderNext: false,
+      skipProviderBreaker: false,
+    }),
+    false
+  );
+});
+
+// ─── 2. End-to-end consumer effect (producer → predicate → breaker) ───────────
+// Replays the exact combo.ts consumer wiring against a REAL circuit breaker.
+
+/**
+ * Mirror of the combo.ts consumer site: feed a real `checkFallbackError` result
+ * through the extracted predicate and into `recordProviderFailure` exactly as the
+ * combo router does. Returns whether the breaker failure was recorded.
+ */
+function runConsumerGate(opts: {
+  provider: string;
+  status: number;
+  errorText: string;
+  headers: Headers | Record<string, string> | null;
+  sameProviderNext?: boolean;
+  isStreamReadinessFailure?: boolean;
+}): boolean {
+  const fallbackResult = checkFallbackError(
+    opts.status,
+    opts.errorText,
+    0,
+    null,
+    opts.provider,
+    opts.headers ?? null
+  );
+  const shouldRecord = shouldRecordProviderBreakerFailure({
+    isStreamReadinessFailure: opts.isStreamReadinessFailure ?? false,
+    status: opts.status,
+    sameProviderNext: opts.sameProviderNext ?? false,
+    skipProviderBreaker: fallbackResult.skipProviderBreaker,
+  });
+  if (shouldRecord) {
+    // Force the breaker to open on the first failure so the assertion is deterministic.
+    recordProviderFailure(opts.provider, undefined, null, OPEN_ON_FIRST);
+  }
+  return shouldRecord;
+}
+
+test("consumer (positive skip): 503 + connection_cooldown hint → breaker stays CLOSED", () => {
+  const provider = "test-9router-skip-2743";
+  // Sanity: clean start.
+  assert.equal(isProviderInCooldown(provider), false);
+
+  const recorded = runConsumerGate({
+    provider,
+    status: 503,
+    errorText: "9router is not running (state: stopped)",
+    headers: new Headers({ "X-Omni-Fallback-Hint": "connection_cooldown" }),
+  });
+
+  // The producer set skipProviderBreaker:true, so the consumer must NOT record a failure.
+  assert.equal(recorded, false, "recordProviderFailure must be skipped for the cooldown hint");
+  assert.equal(
+    isProviderInCooldown(provider),
+    false,
+    "provider circuit breaker must remain CLOSED for a supervisor cooldown signal"
+  );
+  const state = getProviderBreakerState(provider);
+  assert.equal(state?.failureCount ?? 0, 0, "breaker failure count must not be incremented");
+});
+
+test("consumer (positive skip): five consecutive cooldown-hint 503s keep the breaker CLOSED", () => {
+  const provider = "test-9router-skip-loop-2743";
+  for (let i = 0; i < 5; i++) {
+    const recorded = runConsumerGate({
+      provider,
+      status: 503,
+      errorText: "9router is not running (state: stopped)",
+      headers: new Headers({ "X-Omni-Fallback-Hint": "connection_cooldown" }),
+    });
+    assert.equal(recorded, false, `call ${i + 1} must skip recordProviderFailure`);
+  }
+  assert.equal(
+    isProviderInCooldown(provider),
+    false,
+    "repeated supervisor cooldown signals must never trip the whole-provider breaker"
+  );
+});
+
+test("consumer (negative control): plain 503 WITHOUT the hint trips the breaker", () => {
+  const provider = "test-openai-noskip-2743";
+  assert.equal(isProviderInCooldown(provider), false);
+
+  const recorded = runConsumerGate({
+    provider,
+    status: 503,
+    errorText: "service unavailable",
+    headers: null,
+  });
+
+  // No skip flag → the consumer records the failure → breaker opens (threshold = 1).
+  assert.equal(recorded, true, "a real upstream 503 must record a provider-breaker failure");
+  assert.equal(
+    isProviderInCooldown(provider),
+    true,
+    "provider circuit breaker must OPEN for a real upstream outage"
+  );
+  const state = getProviderBreakerState(provider);
+  assert.ok((state?.failureCount ?? 0) >= 1, "breaker failure count must be incremented");
+
+  clearProviderFailure(provider);
+});
+
+test("consumer (negative control): same-provider-next still suppresses recording", () => {
+  // A real upstream 503 (no skip hint) but the next combo target is the same provider:
+  // the gate must NOT trip the breaker so a different model can still be tried.
+  const provider = "test-openai-sameprovider-2743";
+  const recorded = runConsumerGate({
+    provider,
+    status: 503,
+    errorText: "service unavailable",
+    headers: null,
+    sameProviderNext: true,
+  });
+  assert.equal(recorded, false, "same-provider-next must suppress the provider-breaker failure");
+  assert.equal(
+    isProviderInCooldown(provider),
+    false,
+    "breaker must stay CLOSED for same-provider-next"
+  );
+});