From 3cf75daf2400c35cdaaecbe2c13abba93d1c429a Mon Sep 17 00:00:00 2001 From: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Date: Wed, 23 Sep 2026 01:56:48 -0300 Subject: [PATCH] fix(sse): keep providerResponse in scope for failure usage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #14544 made persistFailureUsage read providerResponse for the CPA auth index, but the closure is declared before handleChatCore's large prettier-ignore try block while `let providerResponse` lived inside it. The name was out of scope, so every failure path threw "ReferenceError: providerResponse is not defined" — and since that try has only a finally, the error escaped handleChatCore instead of returning the upstream failure. Hoist the declaration above the closure. Net-zero lines; the try runs once per call, so the per-request lifetime is unchanged. Adds an end-to-end regression test that drives an upstream 400 through handleChatCore: red on the tip with the ReferenceError, green here. The fix also restores 2 failing tests in chatcore-codex-account-pool and 17 in chatcore-translation-paths that the same ReferenceError had broken. Refs #14544 --- .../fixes/chatcore-cpa-authindex-scope.md | 1 + open-sse/handlers/chatCore.ts | 2 +- .../chatcore-failure-usage-cpa-scope.test.ts | 79 +++++++++++++++++++ 3 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 changelog.d/fixes/chatcore-cpa-authindex-scope.md create mode 100644 tests/unit/chatcore-failure-usage-cpa-scope.test.ts diff --git a/changelog.d/fixes/chatcore-cpa-authindex-scope.md b/changelog.d/fixes/chatcore-cpa-authindex-scope.md new file mode 100644 index 000000000000..063c9a9ef756 --- /dev/null +++ b/changelog.d/fixes/chatcore-cpa-authindex-scope.md @@ -0,0 +1 @@ +- **sse:** stop every upstream failure from throwing out of `handleChatCore` — `persistFailureUsage` read `providerResponse` from outside its scope (regression from #14544), so all 13 failure paths raised `ReferenceError: providerResponse is not defined` instead of returning the upstream error and recording failure usage. diff --git a/open-sse/handlers/chatCore.ts b/open-sse/handlers/chatCore.ts index ff3504ec3585..d285f1f6567f 100644 --- a/open-sse/handlers/chatCore.ts +++ b/open-sse/handlers/chatCore.ts @@ -683,6 +683,7 @@ export async function handleChatCore({ payload?: unknown, maxDepth = 3 ): EffectiveServiceTier | null => resolveReportedServiceTierFor(provider, payload, maxDepth); + let providerResponse; // Failure usage record building extracted to chatCore/failureUsage.ts (#3501); the handler keeps // the fire-and-forget save + computes latencyMs, so the call sites stay byte-identical. const persistFailureUsage = ( @@ -3661,7 +3662,6 @@ export async function handleChatCore({ } // Execute request using executor (handles URL building, headers, fallback, transform) - let providerResponse; let providerUrl; let providerHeaders; let finalBody; diff --git a/tests/unit/chatcore-failure-usage-cpa-scope.test.ts b/tests/unit/chatcore-failure-usage-cpa-scope.test.ts new file mode 100644 index 000000000000..4f80454d8d44 --- /dev/null +++ b/tests/unit/chatcore-failure-usage-cpa-scope.test.ts @@ -0,0 +1,79 @@ +// @ts-nocheck +// Regression guard for #14544: `persistFailureUsage` read `providerResponse`, a `let` that lives +// inside handleChatCore's big `try { … } finally { … }` body, while the closure itself is declared +// before that `try`. The name was therefore out of scope (TS2304 under open-sse typecheck, which +// typecheck:core does not cover) and every failure path that records usage threw +// `ReferenceError: providerResponse is not defined`. There is no `catch` on that `try`, so the +// error escaped handleChatCore instead of returning the upstream error response. +// +// This drives a real failure path end to end: the upstream answers with a non-OK status, so +// handleChatCore must return that failure — not reject. +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import test from "node:test"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-cpa-failure-scope-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const { handleChatCore } = await import("../../open-sse/handlers/chatCore.ts"); + +const originalFetch = globalThis.fetch; + +function noopLog() { + return { debug() {}, info() {}, warn() {}, error() {} }; +} + +async function flushAsyncSideEffects() { + for (let i = 0; i < 5; i++) await new Promise((resolve) => setImmediate(resolve)); +} + +test.afterEach(async () => { + globalThis.fetch = originalFetch; + await flushAsyncSideEffects(); + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +}); + +test.after(() => { + globalThis.fetch = originalFetch; + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); +}); + +test("#14544 regression: an upstream failure is returned, not thrown as a ReferenceError", async () => { + globalThis.fetch = async () => + new Response( + JSON.stringify({ error: { message: "bad request", type: "invalid_request_error" } }), + { status: 400, headers: { "Content-Type": "application/json" } } + ); + + const body = { + model: "openai/gpt-4o-mini", + messages: [{ role: "user", content: "hi" }], + max_tokens: 8, + stream: false, + }; + + let result; + await assert.doesNotReject(async () => { + result = await handleChatCore({ + body: structuredClone(body), + modelInfo: { provider: "openai", model: "gpt-4o-mini", extendedContext: false }, + credentials: { apiKey: "sk-test-not-real", providerSpecificData: {} }, + log: noopLog(), + clientRawRequest: { + endpoint: "/v1/chat/completions", + body: structuredClone(body), + headers: new Headers({ accept: "application/json" }), + }, + userAgent: "test-client/1.0", + }); + }, "handleChatCore must not throw on an upstream failure (persistFailureUsage scope)"); + + assert.equal(result.success, false, "an upstream 400 is a failed request"); + assert.equal(result.status, 400, "the upstream status is surfaced, not replaced"); +});