From 265d93ffd84f24b8059f2e7dda9e954bc7c6ba22 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sun, 5 Jul 2026 16:07:32 -0300 Subject: [PATCH] fix(combo): restrict the #6216 empty-stream failover to truly empty bodies (restores #3399/#3685 contracts) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 'streaming no recognized content' branch added by #6216 marked ANY stream that ended without content deltas as invalid — sweeping in two regression-guarded pass-through contracts: an empty stream terminated by an explicit 'data: [DONE]' (#3399 context-cache protection) and an incomplete Claude lifecycle (ping only, no message_start; #3685 — stream-readiness timeout territory, not failover). Both unit guards were red on the branch and green on main (86/86 vs 84/86). The branch now fires only for a truly EMPTY body (zero bytes — the Gemini HTTP-200-empty case that motivated #6216), tracked via sawAnyBytes. New guard: '#5976 truly EMPTY streaming body (zero bytes) -> invalid for combo failover'. 87/87 across both files. Also in this pre-flight batch: - agentSkillTools-mcp: api.have upper bound 22 -> 23 (the #6186 catalog addition updated the totals but missed this bound) - delete tests/unit/free-provider-rankings-configured-filter.test.ts: #6251 (server-side configuredOnly/availableOnly) superseded the #6245 client-side toggle it pinned; replacement declared in the test-masking allowlist (tests/unit/freeProviderRankings-filters.test.ts, 11/11) --- config/quality/test-masking-allowlist.json | 4 + open-sse/services/combo/validateQuality.ts | 14 ++- tests/unit/agentSkillTools-mcp.test.ts | 12 ++- ...o-streaming-empty-content-failover.test.ts | 19 ++++ ...rovider-rankings-configured-filter.test.ts | 93 ------------------- 5 files changed, 41 insertions(+), 101 deletions(-) delete mode 100644 tests/unit/free-provider-rankings-configured-filter.test.ts diff --git a/config/quality/test-masking-allowlist.json b/config/quality/test-masking-allowlist.json index 128c05303d..d548c4a40c 100644 --- a/config/quality/test-masking-allowlist.json +++ b/config/quality/test-masking-allowlist.json @@ -25,6 +25,10 @@ "src/shared/components/AutoRoutingBanner.test.tsx": { "replacement": "tests/unit/home-no-autorouting-banner.test.ts", "reason": "v3.8.45 #6164: fix(dashboard) remove the always-on Auto-Routing banner — o COMPONENTE foi deletado junto com o teste (feature removida pelo mantenedor, não mascaramento). O replacement guarda o novo contrato: a home NÃO renderiza o banner e o componente permanece deletado." + }, + "tests/unit/free-provider-rankings-configured-filter.test.ts": { + "replacement": "tests/unit/freeProviderRankings-filters.test.ts", + "reason": "v3.8.45 #6251 supersede #6245: a página Free Provider Rankings migrou do toggle client-side 'Configured Only' (#6245, configuredProviderIds no cliente) para filtros server-side configuredOnly/availableOnly (#6251). O teste antigo pinava a implementação removida (7 asserts quebrados contra código que não existe); o replacement cobre o contrato novo com 11 casos (server-side, lib helper). Verificado legítimo — supersessão documentada no CHANGELOG do #6251." } }, "tests/unit/catalog-updates-v3x.test.ts": "v3.8.45 #6248: fix(providers) remove deprecated MiMo V2 entries — os 5 asserts removidos pinavam specs de modelos mimo-v2-* que deixaram de existir no catálogo (54→49). Asserts seguem a remoção dos modelos, não enfraquecimento. Verificado legítimo. Prune após v3.8.45 mergear para main.", diff --git a/open-sse/services/combo/validateQuality.ts b/open-sse/services/combo/validateQuality.ts index d752a78989..2bab070a21 100644 --- a/open-sse/services/combo/validateQuality.ts +++ b/open-sse/services/combo/validateQuality.ts @@ -97,6 +97,7 @@ export async function validateResponseQuality( let hasContentBlock = false; let hasLifecycleEnd = false; let anyContentFound = false; + let sawAnyBytes = false; const sseLineNormalizer = createSSEDataLineNormalizer(); let pendingEventType = ""; @@ -234,11 +235,13 @@ export async function validateResponseQuality( return { valid: false, reason: "streaming empty content block" }; } - // Non-Claude stream with no recognizable content at all — the stream - // ended without any content deltas (e.g. Gemini returning HTTP 200 - // with an empty body or only metadata chunks). Mark as invalid for - // combo failover so the sibling model gets tried. - if (!anyContentFound && !hasContentBlock) { + // Stream ended with a truly EMPTY body (e.g. Gemini returning HTTP + // 200 with zero bytes) — mark as invalid for combo failover so the + // sibling model gets tried. Streams that carried ANY SSE activity + // (an explicit `data: [DONE]`, ping/metadata events, an incomplete + // Claude lifecycle) keep the pass-through contract (#3399/#3685): + // those are handled by the stream-readiness timeout, not failover. + if (!anyContentFound && !hasContentBlock && !sawAnyBytes) { log.warn?.( "COMBO", "Streaming response ended with no recognized content — marking as invalid for combo failover" @@ -255,6 +258,7 @@ export async function validateResponseQuality( // Accumulate raw bytes for potential replay. bufferedChunks.push(value); + if (value && value.length > 0) sawAnyBytes = true; // Decode incrementally (stream:true keeps multi-byte char state). decodedSoFar += decoder.decode(value, { stream: true }); diff --git a/tests/unit/agentSkillTools-mcp.test.ts b/tests/unit/agentSkillTools-mcp.test.ts index 8a3985f4a9..a34acea595 100644 --- a/tests/unit/agentSkillTools-mcp.test.ts +++ b/tests/unit/agentSkillTools-mcp.test.ts @@ -43,8 +43,14 @@ test("agentSkillTools exports exactly 3 tools", () => { test("each agentSkillTool has name, description, inputSchema, and handler", () => { for (const toolDef of Object.values(agentSkillTools)) { - assert.ok(typeof toolDef.name === "string" && toolDef.name.length > 0, `${toolDef.name}: name missing`); - assert.ok(typeof toolDef.description === "string" && toolDef.description.length > 0, `${toolDef.name}: description missing`); + assert.ok( + typeof toolDef.name === "string" && toolDef.name.length > 0, + `${toolDef.name}: name missing` + ); + assert.ok( + typeof toolDef.description === "string" && toolDef.description.length > 0, + `${toolDef.name}: description missing` + ); assert.ok(toolDef.inputSchema != null, `${toolDef.name}: inputSchema missing`); assert.ok(typeof toolDef.handler === "function", `${toolDef.name}: handler missing`); } @@ -165,7 +171,7 @@ test("omniroute_agent_skills_coverage({}) returns coverage shape", async () => { assert.equal(result.cli.total, 20); assert.ok(typeof result.api.have === "number"); assert.ok(typeof result.cli.have === "number"); - assert.ok(result.api.have >= 0 && result.api.have <= 22); + assert.ok(result.api.have >= 0 && result.api.have <= 23); assert.ok(result.cli.have >= 0 && result.cli.have <= 20); assert.ok(typeof result.totalSkills === "number"); assert.equal(result.totalSkills, result.api.have + result.cli.have + (result.config?.have ?? 0)); diff --git a/tests/unit/combo-streaming-empty-content-failover.test.ts b/tests/unit/combo-streaming-empty-content-failover.test.ts index f623cd1bd1..1d38c10b76 100644 --- a/tests/unit/combo-streaming-empty-content-failover.test.ts +++ b/tests/unit/combo-streaming-empty-content-failover.test.ts @@ -273,3 +273,22 @@ test("#3685 streaming is preserved for non-empty response: clonedResponse body y assert.ok(decoded.includes("Hello"), "decoded body must contain the actual text content"); assert.ok(decoded.includes(", world!"), "decoded body must contain the full text delta"); }); + +test("#5976 truly EMPTY streaming body (zero bytes) → invalid for combo failover", async () => { + // A 200 SSE response whose body closes without emitting a single byte + // (e.g. Gemini returning HTTP 200 with an empty body) cannot carry content — + // fail over to the sibling model. Streams with ANY SSE activity (an explicit + // [DONE], ping/metadata events) keep the pass-through contract (#3399/#3685). + const emptyBody = new ReadableStream({ + start(controller) { + controller.close(); + }, + }); + const res = new Response(emptyBody, { + status: 200, + headers: { "content-type": "text/event-stream" }, + }); + const out = await validateResponseQuality(res, true, silentLog); + assert.equal(out.valid, false, "zero-byte streaming body must trigger failover"); + assert.equal(out.reason, "streaming no recognized content"); +}); diff --git a/tests/unit/free-provider-rankings-configured-filter.test.ts b/tests/unit/free-provider-rankings-configured-filter.test.ts deleted file mode 100644 index 86b16552de..0000000000 --- a/tests/unit/free-provider-rankings-configured-filter.test.ts +++ /dev/null @@ -1,93 +0,0 @@ -/** - * Unit tests for the "Configured Only" filter on the Free Provider Rankings page. - * - * Phase 1 of #6150 — verifies the toggle state, filtering logic, status column, - * cleanup flag, and i18n keys exist in the source code. - */ - -import test from "node:test"; -import assert from "node:assert/strict"; -import { readFileSync } from "node:fs"; -import { join } from "node:path"; - -const root = join(import.meta.dirname, "../.."); -const read = (p: string) => readFileSync(join(root, p), "utf8"); -const pageSrc = read("src/app/(dashboard)/dashboard/free-provider-rankings/page.tsx"); -const en = JSON.parse(read("src/i18n/messages/en.json")); - -test("page declares configuredOnly state", () => { - assert.ok(pageSrc.includes("useState(false)"), "configuredOnly defaults to false"); - assert.ok(pageSrc.includes("setConfiguredOnly"), "setConfiguredOnly setter exists"); -}); - -test("page declares configuredProviderIds state", () => { - assert.ok(pageSrc.includes("configuredProviderIds"), "configuredProviderIds state exists"); - assert.ok(pageSrc.includes("Set"), "configuredProviderIds is typed as Set"); -}); - -test("page fetches /api/providers on mount", () => { - assert.ok(pageSrc.includes('fetch("/api/providers")'), "fetches /api/providers"); - assert.ok(pageSrc.includes("conn?.provider"), "uses optional chaining for conn.provider"); -}); - -test("useEffect has cleanup flag to prevent stale state updates", () => { - assert.ok(pageSrc.includes("let active = true"), "declares cleanup flag"); - assert.ok(pageSrc.includes("if (!active) return"), "guards state update with active flag"); - assert.ok(pageSrc.includes("active = false"), "cleanup function sets active to false"); -}); - -test("displayedRankings filters by configuredProviderIds when toggle is on", () => { - assert.ok(pageSrc.includes("displayedRankings"), "displayedRankings derived variable exists"); - assert.ok( - pageSrc.includes("configuredProviderIds.has(r.id)"), - "filters rankings by configuredProviderIds.has(r.id)" - ); - assert.ok( - pageSrc.includes("configuredOnly\n ? rankings.filter"), - "conditional: when configuredOnly is true, filters rankings" - ); -}); - -test("toggle switch has accessible attributes", () => { - assert.ok(pageSrc.includes('role="switch"'), "toggle has role=switch"); - assert.ok( - pageSrc.includes("aria-checked={configuredOnly}"), - "toggle has aria-checked bound to configuredOnly" - ); - assert.ok( - pageSrc.includes('htmlFor="configured-only-toggle"'), - "label is linked to toggle via htmlFor" - ); -}); - -test("table has a 'Configured' status column", () => { - assert.ok(pageSrc.includes('t("colConfigured")'), "table header includes colConfigured key"); - assert.ok( - pageSrc.includes("configuredProviderIds.has(provider.id)"), - "status column checks configuredProviderIds" - ); -}); - -test("empty state shows noConfiguredProviders when toggle is on", () => { - assert.ok( - pageSrc.includes('t("noConfiguredProviders")'), - "empty state uses noConfiguredProviders i18n key" - ); - assert.ok( - pageSrc.includes("configuredOnly && rankings.length > 0"), - "shows noConfiguredProviders only when toggle is on and data exists" - ); -}); - -test("i18n: en.json has all required filter keys", () => { - const keys = en.freeProviderRankingsPage; - assert.ok(keys, "freeProviderRankingsPage namespace exists in en.json"); - assert.equal(typeof keys.configuredOnly, "string", "configuredOnly is a string"); - assert.equal(typeof keys.configuredOnlyHint, "string", "configuredOnlyHint is a string"); - assert.equal(typeof keys.noConfiguredProviders, "string", "noConfiguredProviders is a string"); - assert.equal(typeof keys.colConfigured, "string", "colConfigured is a string"); - assert.ok(keys.configuredOnly.length > 0, "configuredOnly is non-empty"); - assert.ok(keys.configuredOnlyHint.length > 0, "configuredOnlyHint is non-empty"); - assert.ok(keys.noConfiguredProviders.length > 0, "noConfiguredProviders is non-empty"); - assert.ok(keys.colConfigured.length > 0, "colConfigured is non-empty"); -});