Repository navigation
fix(sse): keep providerResponse in scope for failure usage (regression from #14544) - #14599
Merged
Merged
Conversation
#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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a runtime regression that #14544 put on
release/v3.8.51: every request whose upstream answers with a non-OK status madehandleChatCorethrow instead of returning the failure.#14544 added
cpaAuthIndex: readCpaAuthIndex(providerResponse)insidepersistFailureUsage. That closure is declared near the top ofhandleChatCore(line ~688), butlet providerResponselived inside the large// prettier-ignoretry { … } finally { … }body that opens at line 765 — so the name was not in the closure's scope. Because thetrybody is never re-indented (prettier-ignore), both lines sit at two spaces and look like the same scope; the TypeScript AST shows they are not (handleChatCore > Block@510 > ArrowFunction@688vshandleChatCore > Block@510 > Block@765).At runtime the argument object is built synchronously before
saveRequestUsageis called, soReferenceError: providerResponse is not definedis thrown from the closure. Thattryhas nocatch— only afinallythat releases the turn — so the error escapedhandleChatCoreentirely.persistFailureUsageis called on 13 failure paths (429, upstream 4xx/5xx, model unavailable, context overflow, malformed translated response, stream readiness), so all of them were affected, and no failure usage was recorded.Fix: move the
let providerResponse;declaration out of thetryto just before the closure. Net-zero lines (chatCore.tsstays at its 6402 ceiling). Thetryruns once per call — it is not a loop body — so hoisting does not change the variable's per-request lifetime, and there is no otherproviderResponsein the outer scope for it to collide with.readCpaAuthIndexalready acceptsunknown, so a failure before any upstream response exists reads as unattributed, exactly as #14544 intended.Why CI did not catch it
typecheck:coredoes not coveropen-sse/; the error only shows undercheck:open-sse-typecheck(chatCore.ts(706,40): error TS2304: Cannot find name 'providerResponse'). And no existing unit test drove a request through a path that reachespersistFailureUsagewith the new field — until now.Tests
tests/unit/chatcore-failure-usage-cpa-scope.test.ts— drives a real failure path end to end (upstream answers 400) and assertshandleChatCoreresolves withsuccess: false, status: 400instead of rejecting. TDD: red on the tip withGot unwanted rejection … actual: ReferenceError: providerResponse is not defined, green with the fix. It also stands guard against the declaration ever being moved back inside thetry.chatcore-codex-account-poolchatcore-translation-pathsThe two
codex-account-poolfailures on the tip are literallyReferenceError: providerResponse is not defined. The 4 that remain inchatcore-translation-pathsare a different, pre-existing failure — DeepSeek Responses reasoning replay and Codex Responses-native passthrough,AssertionError/database connection is not open, noproviderResponseanywhere — already tracked in #14496, which predates #14544.Validation (on this branch, after the commit)
chatcore-codex-account-pool+chatcore-failure-usagecheck:open-sse-typecheckchatCore.tsTS2304 is gone; only the 8 pre-existingopen-sse/executors/auggie.tserrors remain (TS2769 ×4, TS18047 ×4 — #14496, identical on the tip)typecheck:coresrc/lib/services/cliproxyAccountHealth.ts:157TS2322 (host: options.host ?? externalHost, byte-identical on the tip)check:file-sizechatCore.tsre-measured after the commit's Prettier pass: still 6402, exactly its ceilingRefs #14544