Repository navigation
fix(openai-shim): keep terminal usage from a repeated finish_reason - #2252
Conversation
Some OpenAI-compatible providers end a stream twice: one chunk carrying the finish_reason, then a second, identical but for the usage, with choices still populated. The final-usage fallback required an EMPTY choices array, so that second chunk was skipped and its usage dropped. The streaming loop ignores a repeated finish_reason by design, so nothing else picked it up: every assistant message from such a provider stored usage of all zeros. Anything reading the size or the cost of a conversation reads it from there - tokenCountWithEstimation walks back to the newest message carrying usage and trusts it - so automatic compaction on size could not fire, the context warning never warned, and cost tracking recorded nothing. A measured session reached 1763 messages with preTokens: 183 recorded at its compaction boundary. Dropping the empty-choices requirement leaves hasEmittedFinalUsage as the single guard against a double emit, which the existing empty-choices and both-terminal-chunks tests already cover. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (3)Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny.⚙️ CodeRabbit configuration file Files:
Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions.⚙️ CodeRabbit configuration file Files:
Apply the OpenClaude maintainer review rubric from AGENTS.md.⚙️ CodeRabbit configuration file Files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughThe stream converter now emits final usage when a chunk has populated choices, if a stop reason is known and final usage has not already been emitted. A new test checks that repeated stop information produces one terminal usage event with the expected token counts. ChangesOpenAI stream usage emission
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~8 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change allows terminal usage from repeated-stop chunks to be recorded without duplicate emission. No merge-blocking risk was identified. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
euxaristia
left a comment
euxaristia
left a comment
There was a problem hiding this comment.
Verified on head 51adb93. I ran the focused file (22 pass) and the negative control against main's streamConversion.ts (exactly the new test fails, 21 pass), so the test is red-first.
The mechanism holds together:
finish_reasonis processed once (src/services/api/openaiShim/streamConversion.ts:778), so a repeated terminal chunk never emits a secondmessage_deltafrom the loop.- The fallback sits outside that once-guard and is gated on
lastStopReason !== null, only set after a processed finish_reason (line 1079). Usage on a pre-stop chunk cannot fire it. hasEmittedFinalUsageis set in both emit paths (lines 1087 and 1126), so the loop-served shapes keep their single-emit guarantee, and the existing both-terminal-chunks test still passes.
One edge worth a thought, non-blocking: the first post-stop chunk with any truthy usage object now wins, and buildAnthropicUsageFromRawUsage (src/services/api/cacheMetrics.ts:397) normalizes an empty or all-zero usage into a zeros object. A provider that echoes usage: {} on a post-stop chunk would store zeros and set the guard, where the old code stored nothing. Recorded zeros defeat the estimation fallback in tokenCountWithEstimation (src/utils/tokens.ts:572), so that provider's size and cost tracking reads zero. Requiring a non-zero token count in the fallback condition would close it. Not a blocker: the shapes in the wild carry real numbers, as the OpenRouter measurement shows.
…wigpine#2252) Some OpenAI-compatible providers end a stream twice: one chunk carrying the finish_reason, then a second, identical but for the usage, with choices still populated. The final-usage fallback required an EMPTY choices array, so that second chunk was skipped and its usage dropped. The streaming loop ignores a repeated finish_reason by design, so nothing else picked it up: every assistant message from such a provider stored usage of all zeros. Anything reading the size or the cost of a conversation reads it from there - tokenCountWithEstimation walks back to the newest message carrying usage and trusts it - so automatic compaction on size could not fire, the context warning never warned, and cost tracking recorded nothing. A measured session reached 1763 messages with preTokens: 183 recorded at its compaction boundary. Dropping the empty-choices requirement leaves hasEmittedFinalUsage as the single guard against a double emit, which the existing empty-choices and both-terminal-chunks tests already cover. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
3 participants
Summary
openaiStreamToAnthropicno longer requires an emptychoicesarray;hasEmittedFinalUsagestays as the single guard against a double emit.finish_reason, then a second, identical but for theusage, withchoicesstill populated. The streaming loop ignores a repeatedfinish_reasonby design, and the fallback skipped that chunk becausechoiceswas not empty, so its usage was dropped and every assistant message from such a provider stored usage of all zeros.Impact
tokenCountWithEstimationwalks back to the newest message carrying usage and trusts it. With zeros there, automatic compaction on size could not fire, the context warning never warned, and cost tracking recorded nothing. A measured session reached 1763 messages withpreTokens: 183recorded at its compaction boundary. After this change those read real numbers.finish_reason(handled in the loop) and usage on a final empty-choiceschunk (handled by this fallback). Both keep their existing tests, includingemits terminal usage once when both terminal chunks report it, which stays passing becausehasEmittedFinalUsageis already set by the time its third chunk arrives.Testing
I ran the required local preflight, with one documented exception below.
exact commands and results:
bun install --frozen-lockfile— okbun run lint:any-budget— ok (current=778, baseline=779)bun run smoke— ok (CLI + SDK bundles, reports 0.31.0)bun run deadcode— ok (knip: configuration hints only)bun run typecheck— okbun run typecheck:type-tests— oknode bin/openclaude --version— okNODE_DISABLE_COMPILE_CACHE=1 node bin/openclaude --version— okbun run test:provider— 1703 pass / 1 fail. The failure is pre-existing:Claude stream watchdog > falls back when the top-level stream iterator never settles, which fails identically on cleanmain88a2286b(1702 pass / 1 fail). The pass count differs by exactly +1, which is the test added here.npm run test:provider-recommendation— okgit fetch https://github.com/Twigpine/openclaude.git mainthenbun run security:pr-scan -- --base FETCH_HEAD --head HEAD— okfocused tests:
bun test ./src/services/api/openaiShim/streamConversion.test.ts— 22 pass, 0 fail. Negative control: withstreamConversion.tsreverted tomainand the new test kept, exactly that test fails (21 pass, 1 fail) and the emitted stream shows amessage_deltacarrying nousageat all.documented skipped checks, platform limitations, or verified pre-existing failures:
bun run checkcould not be run to completion, and the cause is inmain, not in this PR. Itstest:fullstep stops making progress and spins a single core indefinitely. Two runs on a clean checkout ofmainat88a2286bwith no modifications were left for 4h30m and 2h53m (16512s and 14864s of CPU) and never finished.bun test --feature=UNATTENDED_RETRY --timeout 15000over the whole suite also never finished and recorded zero timed-out tests, which is consistent with a synchronous spin reached only after state accumulates across test files rather than one slow test. Every segment of the suite passes on its own, and all segments together take about nine minutes, so the stalled runs consumed roughly 27x the suite's entire work without completing. I did not isolate the file: the stall occurs wherebun testwrites only to stdout, which is block-buffered when redirected, so the last visible output is unrelated to where execution stopped. Happy to open this as a separate issue with the measurements.The steps inside
checkthat do finish —lint:any-budget,smoke,deadcode— were run individually and pass (above). The unit suite was covered by running it in segments and comparing failure sets against the base:main88a2286bThe two sets differ by two entries, both explained and neither caused by this change:
GitHub 429 stops when every pooled credential is cooling downappears on this branch but not on the base run. It is flaky: running thesrc/servicessegment three times on cleanmainwith no changes, it passed twice and failed on the third run. In isolation it passes 3/3.open build source does not reintroduce Ant employee gate helpersappears on the base run but not here. It reads the built bundle indist/, and it passes on all three trees once each has a currentbun run build.For context on the 104: they are dominated by environment-dependent tests rather than real breakage —
CLAUDE_CONFIG_DIRoverrides,expandTildeon win32, fastMode gates, Anthropic attribution, and the fourscripts/openclaude-bin-compile-cache.test.tslauncher failures. All of them reproduce on the clean base above. Unrelated, but worth flagging: some tests write into the real user config directory (~/.openclaude/backups/) instead of a temporary one.Notes
finish_reason,usageon the later of the two. The added test reproduces that shape with the file's existing fake-SSE helpers, so it needs no network and no provider key.choiceschunk and providers that send it alongside thefinish_reasonboth keep their existing path.Summary by CodeRabbit