Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request introduces support for the legacy OpenAI Completions API (/v1/completions) across the platform, including frontend response parsing, backend routing, and new transformer implementations for OpenAI and DeepSeek. Key changes include the addition of RequestTypeCompletion and APIFormatOpenAICompletion, along with the necessary logic to aggregate streaming completion chunks. Critical feedback was provided regarding a thread-safety issue in the DeepSeek transformer where request-specific state was stored in a shared field, potentially causing race conditions. Additionally, the reviewer identified the use of a non-standard errors.AsType function that would lead to compilation errors and suggested improvements to JSON unmarshaling logic to ensure errors are correctly propagated.
| BaseURL string | ||
| APIKeyProvider auth.APIKeyProvider | ||
| completion transformer.Outbound | ||
| lastRequestType llm.RequestType |
There was a problem hiding this comment.
The lastRequestType field in OutboundTransformer introduces a race condition because the transformer instance is shared across multiple concurrent requests within a long-lived service. Storing request-specific state in a shared struct field is not thread-safe.
| ctx context.Context, | ||
| llmReq *llm.Request, | ||
| ) (*httpclient.Request, error) { | ||
| t.lastRequestType = llmReq.RequestType |
There was a problem hiding this comment.
Setting t.lastRequestType here is unsafe as OutboundTransformer is a shared object. Concurrent requests will overwrite this value, leading to incorrect delegation in TransformStream, TransformResponse, and TransformError. Consider passing the request type through the context or using separate transformer instances for different API formats in the channel configuration.
| if llmErr, ok := errors.AsType[*llm.ResponseError](rawErr); ok { | ||
| return &httpclient.Error{ | ||
| StatusCode: llmErr.StatusCode, | ||
| Status: http.StatusText(llmErr.StatusCode), | ||
| Body: xjson.MustMarshal(&OpenAIError{Detail: llmErr.Detail}), | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to the previous issue, errors.AsType is not a standard function. Use errors.As to correctly check for *llm.ResponseError.
| if llmErr, ok := errors.AsType[*llm.ResponseError](rawErr); ok { | |
| return &httpclient.Error{ | |
| StatusCode: llmErr.StatusCode, | |
| Status: http.StatusText(llmErr.StatusCode), | |
| Body: xjson.MustMarshal(&OpenAIError{Detail: llmErr.Detail}), | |
| } | |
| } | |
| var llmErr *llm.ResponseError | |
| if errors.As(rawErr, &llmErr) { | |
| return &httpclient.Error{ | |
| StatusCode: llmErr.StatusCode, | |
| Status: http.StatusText(llmErr.StatusCode), | |
| Body: xjson.MustMarshal(&OpenAIError{Detail: llmErr.Detail}), | |
| } | |
| } |
| return nil | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
The UnmarshalJSON method should return an error if it fails to unmarshal the data into both supported formats (string and string array). Returning nil here hides unmarshaling failures, which can lead to unexpected behavior when processing invalid input.
| return nil | |
| return err |
Greptile SummaryThis PR adds OpenAI-compatible legacy Completion API support (
Confidence Score: 5/5Safe to merge; all remaining findings are P2 and do not affect the primary single-choice completion path Previously flagged P1 issues (missing RequestType on outbound request, wrong HTTP status code on error responses) are resolved in the current code. The two remaining findings are edge-case correctness (n > 1 streaming) and a usability gap (OpenAI channel defaults), neither of which blocks the primary use case of single-choice completion via OpenAI or DeepSeek channels. llm/transformer/openai/completion_aggregator.go (n > 1 aggregation) and internal/server/biz/channel_endpoint.go (openAICompatibleDefaultEndpoints missing completion format) Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Router as /v1/completions route
participant CIT as CompletionInboundTransformer
participant Orch as Orchestrator
participant DS as DeepSeek OutboundTransformer
participant COT as CompletionOutboundTransformer
participant API as Provider API
Client->>Router: POST /v1/completions
Router->>CIT: TransformRequest(httpReq)
CIT-->>Orch: llm.Request{RequestType: completion}
Orch->>DS: TransformRequest(llmReq)
DS->>DS: route by RequestType == completion
DS->>COT: TransformRequest(llmReq)
COT-->>DS: httpclient.Request{URL: /beta/completions, RequestType: completion}
DS-->>Orch: httpclient.Request
Orch->>API: POST /beta/completions
API-->>Orch: HTTP Response / Stream
Orch->>DS: TransformStream(req, stream) or TransformResponse(resp)
DS->>DS: route by req.RequestType == completion
DS->>COT: delegate
COT-->>DS: llm.Response{Completion: ...}
DS-->>Orch: llm.Response
Orch->>CIT: TransformResponse(llmResp)
CIT-->>Client: CompletionResponse JSON
Reviews (6): Last reviewed commit: "feat: openai/deepseek completion api, cl..." | Re-trigger Greptile |
| return &httpclient.Request{ | ||
| Method: http.MethodPost, | ||
| URL: url, | ||
| Headers: headers, | ||
| Body: body, | ||
| Auth: &httpclient.AuthConfig{ | ||
| Type: "bearer", | ||
| APIKey: apiKey, | ||
| }, | ||
| APIFormat: string(llm.APIFormatOpenAICompletion), | ||
| Metadata: scope.Metadata(), | ||
| }, nil |
There was a problem hiding this comment.
🔴 CompletionOutboundTransformer.TransformRequest never sets RequestType, breaking DeepSeek stream/response routing
The new CompletionOutboundTransformer.TransformRequest (llm/transformer/openai/completion_outbound.go:116-127) does not set RequestType on the returned httpclient.Request. The DeepSeek outbound transformer relies on req.RequestType to route completion requests: TransformStream at llm/transformer/deepseek/outbound.go:159 checks req.RequestType == string(llm.RequestTypeCompletion), and TransformResponse at llm/transformer/deepseek/outbound.go:167 does the same. Since RequestType is always empty, the condition is never true, and completion streams/responses are incorrectly handled by the base OpenAI chat completions transformer instead of the completion transformer. Other outbound transformers correctly set this field — e.g., image_outbound.go:57 sets rawReq.RequestType = llm.RequestTypeImage.String() and video_outbound.go:90 sets req.RequestType = llm.RequestTypeVideo.String().
| return &httpclient.Request{ | |
| Method: http.MethodPost, | |
| URL: url, | |
| Headers: headers, | |
| Body: body, | |
| Auth: &httpclient.AuthConfig{ | |
| Type: "bearer", | |
| APIKey: apiKey, | |
| }, | |
| APIFormat: string(llm.APIFormatOpenAICompletion), | |
| Metadata: scope.Metadata(), | |
| }, nil | |
| return &httpclient.Request{ | |
| Method: http.MethodPost, | |
| URL: url, | |
| Headers: headers, | |
| Body: body, | |
| Auth: &httpclient.AuthConfig{ | |
| Type: "bearer", | |
| APIKey: apiKey, | |
| }, | |
| RequestType: string(llm.RequestTypeCompletion), | |
| APIFormat: string(llm.APIFormatOpenAICompletion), | |
| Metadata: scope.Metadata(), | |
| }, nil |
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| func (t *OutboundTransformer) TransformStream(ctx context.Context, req *httpclient.Request, stream streams.Stream[*httpclient.StreamEvent]) (streams.Stream[*llm.Response], error) { | ||
| if req.RequestType == string(llm.RequestTypeCompletion) { | ||
| return t.completion.TransformStream(ctx, req, stream) | ||
| } | ||
|
|
||
| return t.Outbound.TransformStream(ctx, req, stream) |
There was a problem hiding this comment.
🔴 DeepSeek TransformStream dereferences req without nil check, unlike TransformResponse
In llm/transformer/deepseek/outbound.go:158-163, TransformStream accesses req.RequestType without checking if req is nil. In contrast, the sibling TransformResponse method at llm/transformer/deepseek/outbound.go:167 properly guards with httpResp.Request != nil. The req parameter in the new TransformStream interface signature is *httpclient.Request, which can be nil — several test call sites already pass nil (e.g., pass_through_test.go:758). If TransformStream is ever called with a nil req for a DeepSeek channel, this will cause a nil pointer dereference panic.
| func (t *OutboundTransformer) TransformStream(ctx context.Context, req *httpclient.Request, stream streams.Stream[*httpclient.StreamEvent]) (streams.Stream[*llm.Response], error) { | |
| if req.RequestType == string(llm.RequestTypeCompletion) { | |
| return t.completion.TransformStream(ctx, req, stream) | |
| } | |
| return t.Outbound.TransformStream(ctx, req, stream) | |
| func (t *OutboundTransformer) TransformStream(ctx context.Context, req *httpclient.Request, stream streams.Stream[*httpclient.StreamEvent]) (streams.Stream[*llm.Response], error) { | |
| if req != nil && req.RequestType == string(llm.RequestTypeCompletion) { | |
| return t.completion.TransformStream(ctx, req, stream) | |
| } | |
| return t.Outbound.TransformStream(ctx, req, stream) | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
Uh oh!
There was an error while loading. Please reload this page.