fix: add missing created field to text completion response#1894
Open
qcu266 wants to merge 1 commit intomaximhq:mainfrom
Open
fix: add missing created field to text completion response#1894qcu266 wants to merge 1 commit intomaximhq:mainfrom
qcu266 wants to merge 1 commit intomaximhq:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Provider as Provider Adapter
participant Utils as CreateChunk (utils)
participant Streamer as Streamer
Client->>Provider: Request text completion
Provider-->>Provider: generate completion\ncapture/parse created timestamp (created)
Provider->>Utils: CreateBifrostTextCompletionChunkResponse(..., created)
Utils->>Streamer: emit chunk (includes Created)
Streamer->>Client: stream chunk(s) and final (each includes Created)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/schemas/textcompletions.go`:
- Line 70: Several code paths construct BifrostTextCompletionResponse without
setting the non-omitempty Created field, causing serialized responses to show
created: 0; update every construction site (e.g., the constructors/creations in
framework/streaming/types.go around line ~251, core/providers/replicate/text.go,
core/providers/bedrock/text.go, core/providers/anthropic/text.go,
core/providers/utils/utils.go where responses are built, and any manual
conversions from chat completions in core/schemas/chatcompletions.go) to assign
Created = int(time.Now().Unix()) (or equivalent integer Unix timestamp) when
creating a BifrostTextCompletionResponse or when converting into one; ensure any
path that currently relies on HandleProviderResponse() still preserves or sets
Created if the upstream provider omits it.
| type BifrostTextCompletionResponse struct { | ||
| ID string `json:"id"` | ||
| Choices []BifrostResponseChoice `json:"choices"` | ||
| Created int `json:"created"` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all construction sites for BifrostTextCompletionResponse
echo "=== BifrostTextCompletionResponse construction sites ==="
rg -nP --type=go 'BifrostTextCompletionResponse\s*\{' -A5
echo ""
echo "=== BifrostTextCompletionResponse assignments (& or *) ==="
rg -nP --type=go '&BifrostTextCompletionResponse|:=.*BifrostTextCompletionResponse' -A8
echo ""
echo "=== Created field assignments ==="
rg -nP --type=go -B2 -A2 'Created\s*:'
echo ""
echo "=== Check for BifrostTextCompletionResponse usages ==="
rg -nP --type=go 'BifrostTextCompletionResponse' --type-listRepository: maximhq/bifrost
Length of output: 36563
🏁 Script executed:
#!/bin/bash
# Check if openai and azure HandleProviderResponse/json unmarshaling sets Created
echo "=== OpenAI text completion handling ==="
rg -nP --type=go -A10 'HandleOpenAITextCompletion\(' core/providers/openai/openai.go | head -50
echo ""
echo "=== Check if Created is set after construction in OpenAI ==="
rg -nP --type=go -B5 -A15 'response := &schemas.BifrostTextCompletionResponse{}' core/providers/openai/openai.go
echo ""
echo "=== Check if Created is set after construction in Azure ==="
rg -nP --type=go -B5 -A15 'response := &schemas.BifrostTextCompletionResponse{}' core/providers/azure/azure.go
echo ""
echo "=== Check framework/streaming/types.go for Created assignment to text responses ==="
rg -nP --type=go -B3 -A10 'textResp := &schemas.BifrostTextCompletionResponse' framework/streaming/types.goRepository: maximhq/bifrost
Length of output: 3079
🏁 Script executed:
#!/bin/bash
# Check HandleProviderResponse function to see if it unmarshals JSON
echo "=== HandleProviderResponse implementation ==="
rg -nP --type=go -A20 'func HandleProviderResponse' core/providers/utils/
echo ""
echo "=== Check if text completion responses are deserialized from provider JSON ==="
rg -nP --type=go -B5 -A10 'json.Unmarshal.*Body' core/providers/openai/ | head -60
echo ""
echo "=== Check framework streaming for any Created field assignment in text path ==="
rg -nP --type=go -A30 'textResp := &schemas.BifrostTextCompletionResponse' framework/streaming/types.go | head -50Repository: maximhq/bifrost
Length of output: 3065
🏁 Script executed:
#!/bin/bash
# Get the full HandleProviderResponse function to see if it unmarshals JSON
echo "=== Full HandleProviderResponse function ==="
rg -nP --type=go -A60 'func HandleProviderResponse\[T any\]' core/providers/utils/utils.go | head -80
echo ""
echo "=== Check if sonic or json.Unmarshal is used for response ==="
rg -nP --type=go -A5 'sonic.Unmarshal.*response\|json.Unmarshal.*response' core/providers/utils/utils.goRepository: maximhq/bifrost
Length of output: 2569
Set Created timestamp on all BifrostTextCompletionResponse construction paths.
Line 70 adds a non-omitempty Created field. Code paths that manually construct BifrostTextCompletionResponse (e.g., framework/streaming/types.go:251, core/providers/replicate/text.go:95, core/providers/bedrock/text.go:118, core/providers/anthropic/text.go:89, core/providers/utils/utils.go:1569, core/schemas/chatcompletions.go conversions) do not assign Created, resulting in created: 0 in serialized responses. Paths using HandleProviderResponse() depend on upstream provider JSON including a created field. Ensure all construction sites assign an actual timestamp, typically using int(time.Now().Unix()).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/schemas/textcompletions.go` at line 70, Several code paths construct
BifrostTextCompletionResponse without setting the non-omitempty Created field,
causing serialized responses to show created: 0; update every construction site
(e.g., the constructors/creations in framework/streaming/types.go around line
~251, core/providers/replicate/text.go, core/providers/bedrock/text.go,
core/providers/anthropic/text.go, core/providers/utils/utils.go where responses
are built, and any manual conversions from chat completions in
core/schemas/chatcompletions.go) to assign Created = int(time.Now().Unix()) (or
equivalent integer Unix timestamp) when creating a BifrostTextCompletionResponse
or when converting into one; ensure any path that currently relies on
HandleProviderResponse() still preserves or sets Created if the upstream
provider omits it.
Contributor
|
@qcu266 thanks for the PR - as CodeRabbit suggests - we will need to populate the field across the codebase |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/openai/openai.go`:
- Around line 624-626: The code only assigns created = response.Created inside
the branch that checks response.Usage != nil, so compatible backends that omit
usage chunks leave created unset; update the streaming/response-handling logic
(the loop that processes streaming chunks and later builds the terminal chunk)
to always set created = response.Created whenever response.Created is non-zero
regardless of response.Usage, ensuring the terminal chunk creation (where
created is used) reflects the last non-zero Created value from any chunk.
In `@core/providers/replicate/text.go`:
- Around line 136-144: The code sets bifrostResponse.Created from
response.CompletedAt which is wrong; use response.CreatedAt instead. Edit the
anonymous func that assigns bifrostResponse.Created (the block referencing
response.CompletedAt) to parse and return response.CreatedAt (time.Parse
time.RFC3339 on *response.CreatedAt), falling back to time.Now().Unix() on nil
or parse error, keeping the same return semantics as other provider files
(responses.go, videos.go, images.go, chat.go).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20c5785f-b873-4db2-bffd-a9cd1a73c3e1
📒 Files selected for processing (9)
core/providers/anthropic/text.gocore/providers/bedrock/text.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/replicate/text.gocore/providers/utils/utils.gocore/schemas/chatcompletions.gocore/schemas/textcompletions.goframework/streaming/types.go
qcu266
force-pushed
the
fix/text-completion-response-missing-created-parms
branch
from
b1fd65d to
1bfd5d0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/openai/openai.go`:
- Around line 624-631: There is a duplicated response.Usage = nil assignment and
an extra closing brace that breaks syntax in the aggregation block handling
response chunks; inside the code that sets response.Usage = nil and tracks
created (look for the variables response and created and the surrounding if
response.Created > 0 block), remove the duplicate response.Usage = nil line and
delete the stray '}' so the if/else blocks and function braces balance
correctly, ensuring the final aggregated chunk retains the single intended
assignment and created is set only once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ded4dacd-80f5-41e1-a150-857265368ae0
📒 Files selected for processing (1)
core/providers/openai/openai.go
qcu266
force-pushed
the
fix/text-completion-response-missing-created-parms
branch
from
1bfd5d0 to
1240952
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/schemas/chatcompletions.go (1)
32-35: UnifyCreatedtype toint64for schema consistency.
BifrostChatResponse.Createdisintwhile other response schemas (BifrostTextCompletionResponse,BifrostModel,BifrostImage) useint64for the same field, forcing four explicit type casts in the conversion methods. ChangeCreatedtoint64to eliminate this friction and align with the rest of the schema definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/chatcompletions.go` around lines 32 - 35, Change the Created field on BifrostChatResponse from int to int64 to match other schemas (replace "Created int" with "Created int64" in the BifrostChatResponse struct) and update any conversion/mapper functions that handle BifrostChatResponse (remove the explicit int-to-int64 casts where Created is used); ensure functions that consume Created (conversion helpers or methods that previously cast the value) now accept int64 and compile without the four explicit casts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/schemas/chatcompletions.go`:
- Around line 32-35: Change the Created field on BifrostChatResponse from int to
int64 to match other schemas (replace "Created int" with "Created int64" in the
BifrostChatResponse struct) and update any conversion/mapper functions that
handle BifrostChatResponse (remove the explicit int-to-int64 casts where Created
is used); ensure functions that consume Created (conversion helpers or methods
that previously cast the value) now accept int64 and compile without the four
explicit casts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6118c31e-da41-4b38-a8a7-eaff1aebaf4c
📒 Files selected for processing (9)
core/providers/anthropic/text.gocore/providers/bedrock/text.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/replicate/text.gocore/providers/utils/utils.gocore/schemas/chatcompletions.gocore/schemas/textcompletions.goframework/streaming/types.go
🚧 Files skipped from review as they are similar to previous changes (6)
- core/providers/replicate/text.go
- core/providers/replicate/replicate.go
- core/providers/anthropic/text.go
- core/schemas/textcompletions.go
- framework/streaming/types.go
- core/providers/bedrock/text.go
Author
@akshaydeo ， I have carefully reviewed it and made corresponding modifications in other places, but I'm not sure if there are still any omissions. |
|
|
Author
|
@akshaydeo Do you have any concerns about this PR? |
Contributor
|
Let me review this today |
|
Hope to release this fix sooner. |
qcu266
force-pushed
the
fix/text-completion-response-missing-created-parms
branch
from
1240952 to
7c45afa
Compare
Contributor
Confidence Score: 5/5Safe to merge; the fix is narrowly scoped and all primary response paths are correctly updated. All findings are P2 or lower. The only concern is the lack of a startTime.Unix() fallback in the OpenAI streaming handler for the unlikely case where a custom provider never echoes core/providers/openai/openai.go — the Important Files Changed
Reviews (2): Last reviewed commit: "fix: add missing created field to text c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/openai/openai.go`:
- Line 728: The call to providerUtils.CreateBifrostTextCompletionChunkResponse
in replicate.go is missing the created (int64) argument, causing a signature
mismatch; update the invocation in replicate.go to include the created variable
between currentChunkIndex (chunkIndex) and requestType so the call passes all
eight parameters required by CreateBifrostTextCompletionChunkResponse (id,
usage, finishReason, currentChunkIndex, created, requestType, providerName,
model).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 712634e1-800d-43d5-a7a1-021233725e5c
📒 Files selected for processing (8)
core/providers/anthropic/text.gocore/providers/bedrock/text.gocore/providers/openai/openai.gocore/providers/replicate/replicate.gocore/providers/replicate/text.gocore/providers/utils/utils.gocore/schemas/chatcompletions.gocore/schemas/textcompletions.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/providers/replicate/text.go
- core/providers/replicate/replicate.go
- core/schemas/chatcompletions.go
- core/providers/utils/utils.go
qcu266
force-pushed
the
fix/text-completion-response-missing-created-parms
branch
from
7c45afa to
7e4fac0
Compare
Author
Hi @akshaydeo, just following up on this PR — I’ve pushed the requested updates. Would appreciate another look when you have a moment. Thanks! |
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
4 participants
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
add missing created field to text completion response
ref: https://developers.openai.com/api/reference/resources/completions/methods/create
In some strictly validated client libraries, an error indicating the missing 'created' field will be reported. e.g. zed Self-Hosted OpenAI-compatible servers
Changes
Type of change
Affected areas
How to test
Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
N/A
Checklist
docs/contributing/README.mdand followed the guidelines