fix(openai): fix stop_reason null and zero usage fields in stream adapter#226
Open
guunergooner wants to merge 1 commit intoclaude-code-best:mainfrom
Open
fix(openai): fix stop_reason null and zero usage fields in stream adapter#226guunergooner wants to merge 1 commit intoclaude-code-best:mainfrom
guunergooner wants to merge 1 commit intoclaude-code-best:mainfrom
Conversation
…pter Signed-off-by: guunergooner <tongchao0923@gmail.com>
Contributor
📝 WalkthroughWalkthroughRefactors OpenAI stream handling to consolidate message assembly into a single AssistantMessage at Changes
Sequence Diagram(s)sequenceDiagram
participant Stream as OpenAI Stream
participant Adapter as streamAdapter
participant QueryModel as queryModelOpenAI
participant Assembler as Message Assembly
rect rgba(100, 200, 150, 0.5)
Note over Stream,Assembler: Stream Processing (New Flow)
Stream->>Adapter: chunk (content_block_start)
Adapter->>QueryModel: yield content_block_start
Stream->>Adapter: chunk (content_block_delta)
Adapter->>QueryModel: yield content_block_delta
Stream->>Adapter: chunk (finish_reason, usage)
Adapter->>Adapter: set pendingFinishReason
Adapter->>Adapter: defer message_delta/message_stop
alt Trailing Usage Chunk
Stream->>Adapter: chunk (choices: [], usage)
Adapter->>Adapter: accumulate usage tokens
Adapter->>Adapter: update cached_tokens
end
Adapter->>Adapter: stream completes
Adapter->>Adapter: emit deferred message_delta<br/>(with full usage)
Adapter->>QueryModel: yield message_delta
Adapter->>QueryModel: yield message_stop
end
rect rgba(150, 150, 200, 0.5)
Note over QueryModel,Assembler: Message Assembly (New Flow)
QueryModel->>Assembler: collect all content_block events
QueryModel->>Assembler: receive message_stop
Assembler->>Assembler: consolidate all blocks
Assembler->>Assembler: inject usage & stop_reason
QueryModel->>QueryModel: yield single AssistantMessage
QueryModel->>QueryModel: reset partialMessage
alt Max Tokens Detected
QueryModel->>QueryModel: yield apiError: 'max_output_tokens'
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/api/openai/index.ts (1)
385-392:⚠️ Potential issue | 🟠 MajorDuplicate cost tracking — this block executes twice for
message_stop.The cost tracking at lines 385-392 duplicates the logic already executed inside the
message_stopcase handler at lines 364-367. Whenevent.type === 'message_stop',addToTotalSessionCostis called twice with the same values, inflating the reported session cost.🐛 Proposed fix: Remove the duplicate block
- // Track cost and token usage (matching the Anthropic path in claude.ts) - if ( - event.type === 'message_stop' && - usage.input_tokens + usage.output_tokens > 0 - ) { - const costUSD = calculateUSDCost(openaiModel, usage as any) - addToTotalSessionCost(costUSD, usage as any, options.model) - } - // Also yield as StreamEvent for real-time display (matching Anthropic path) yield { type: 'stream_event',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/api/openai/index.ts` around lines 385 - 392, There is a duplicated cost-tracking block that runs when event.type === 'message_stop' causing addToTotalSessionCost to be called twice; remove the redundant block (the one that checks event.type === 'message_stop' and calls calculateUSDCost + addToTotalSessionCost) so that cost is only recorded by the existing logic inside the 'message_stop' case handler; ensure calculateUSDCost, addToTotalSessionCost, usage and options.model are still used exactly once for that event type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/formatBriefTimestamp.ts`:
- Around line 79-81: The day-boundary calculation in startOfDay() uses UTC
getters (getUTCFullYear/getUTCMonth/getUTCDate) while formatting uses local
timezone (toLocaleTimeString/toLocaleString), causing inconsistent day
classification vs display; update startOfDay() to use local date components
(getFullYear/getMonth/getDate) so boundaries match local formatting, or
alternatively make the display calls use UTC consistently (e.g., pass timeZone:
'UTC' to toLocaleString/toLocaleTimeString) if UTC-based boundaries are
intentional; if keeping UTC behavior, add a clear comment in startOfDay()
explaining the trade-off and test determinism.
---
Outside diff comments:
In `@src/services/api/openai/index.ts`:
- Around line 385-392: There is a duplicated cost-tracking block that runs when
event.type === 'message_stop' causing addToTotalSessionCost to be called twice;
remove the redundant block (the one that checks event.type === 'message_stop'
and calls calculateUSDCost + addToTotalSessionCost) so that cost is only
recorded by the existing logic inside the 'message_stop' case handler; ensure
calculateUSDCost, addToTotalSessionCost, usage and options.model are still used
exactly once for that event type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8febc2f7-9823-4844-9baa-a8e3065b9ad5
📒 Files selected for processing (6)
src/services/api/openai/__tests__/queryModelOpenAI.test.tssrc/services/api/openai/__tests__/streamAdapter.test.tssrc/services/api/openai/index.tssrc/services/api/openai/streamAdapter.tssrc/utils/__tests__/formatBriefTimestamp.test.tssrc/utils/formatBriefTimestamp.ts
Comment on lines
79
to
81
| function startOfDay(d: Date): number { | ||
| return new Date(d.getFullYear(), d.getMonth(), d.getDate()).getTime() | ||
| return Date.UTC(d.getUTCFullYear(), d.getUTCMonth(), d.getUTCDate()) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or documentation about UTC being intentional
rg -n -C3 "UTC|startOfDay" src/utils/formatBriefTimestamp.ts
# Check for related discussion in PR or commit messages
git log --oneline -10 --all -- src/utils/formatBriefTimestamp.ts 2>/dev/null || echo "Git history not available"Repository: claude-code-best/claude-code
Length of output: 493
🏁 Script executed:
cat -n src/utils/formatBriefTimestamp.tsRepository: claude-code-best/claude-code
Length of output: 2858
Day boundary calculation uses UTC, but display formatting uses local timezone.
The startOfDay() function calculates day boundaries using UTC components (getUTCFullYear(), getUTCMonth(), getUTCDate()), but the subsequent toLocaleTimeString() and toLocaleString() calls (lines 30, 37, 44) format the timestamp in local timezone. This causes a mismatch for users in timezones far from UTC:
- A timestamp at
2026-04-01T22:00:00Zis categorized as April 1 UTC (same day threshold), but displays as April 2 in UTC+10.
If this UTC-based boundary is intentional for test determinism, add a comment explaining the trade-off. Otherwise, use consistent timezone handling for both day boundary calculation and display.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/formatBriefTimestamp.ts` around lines 79 - 81, The day-boundary
calculation in startOfDay() uses UTC getters
(getUTCFullYear/getUTCMonth/getUTCDate) while formatting uses local timezone
(toLocaleTimeString/toLocaleString), causing inconsistent day classification vs
display; update startOfDay() to use local date components
(getFullYear/getMonth/getDate) so boundaries match local formatting, or
alternatively make the display calls use UTC consistently (e.g., pass timeZone:
'UTC' to toLocaleString/toLocaleTimeString) if UTC-based boundaries are
intentional; if keeping UTC behavior, add a clear comment in startOfDay()
explaining the trade-off and test determinism.
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
1 participant
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
stop_reasonalways being null in assembledAssistantMessage— the value captured frommessage_deltawas never applied to the partial messagepartialMessagenot being reset tonullaftermessage_stop, causing a safety fallback that would yield a second identicalAssistantMessage(doubled content in next API request)queryModelOpenAIandstreamAdapterTest plan
bun test src/services/api/openai/__tests__/to verify new tests passSummary by CodeRabbit