Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds an xAI (Grok) provider: env sample and config entry, explicit provider registration, a new provider implementation with sync/streaming chat & responses plus models listing, non-mutating WithStreaming helpers, provider tests, and client call updates to use WithStreaming. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Prov as xai.Provider
participant Client as llmclient.Client
participant API as x.ai API
rect rgb(245,250,255)
Note over App,Prov: Synchronous (non-streaming) flow
App->>Prov: ChatCompletion(ctx, req)
Prov->>Client: Build HTTP request (set Bearer header)
Client->>API: POST /chat/completions (sync)
API-->>Client: 200 JSON / error
Client-->>Prov: core.ChatResponse / error
Prov-->>App: core.ChatResponse / error
end
rect rgb(235,245,235)
Note over App,Prov: Streaming flow (uses req.WithStreaming())
App->>Prov: StreamChatCompletion(ctx, req.WithStreaming())
Prov->>Client: Build streaming HTTP request
Client->>API: POST /chat/completions (stream)
API-->>Client: SSE / chunked stream
Client-->>Prov: io.ReadCloser (stream)
Prov-->>App: stream reader (caller closes)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
.env.templatecmd/gomodel/main.goconfig/config.gointernal/providers/xai/xai.gointernal/providers/xai/xai_test.go
🧰 Additional context used
📓 Path-based instructions (5)
config/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Configuration must be loaded via Viper from environment variables and .env file, with at least one provider API key required
Files:
config/config.go
internal/providers/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
internal/providers/**/*.go: Provider implementations must implement thecore.Providerinterface with methods: ChatCompletion, StreamChatCompletion, ListModels, Responses, and StreamResponses
Streaming responses must returnio.ReadCloserand the caller is responsible for closing the stream
Files:
internal/providers/xai/xai_test.gointernal/providers/xai/xai.go
internal/providers/*/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Providers must self-register with the factory using an
init()function that callsproviders.RegisterFactory()with the provider name
Files:
internal/providers/xai/xai_test.gointernal/providers/xai/xai.go
internal/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests must be located alongside implementation files using the
*_test.gonaming convention
Files:
internal/providers/xai/xai_test.go
cmd/gomodel/main.go
📄 CodeRabbit inference engine (CLAUDE.md)
Provider packages must be imported in
cmd/gomodel/main.gousing blank imports (_ "gomodel/internal/providers/{name}")
Files:
cmd/gomodel/main.go
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T16:40:36.115Z
Learning: Applies to internal/providers/**/*.go : Provider implementations must implement the `core.Provider` interface with methods: ChatCompletion, StreamChatCompletion, ListModels, Responses, and StreamResponses
📚 Learning: 2025-12-26T16:40:36.115Z
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T16:40:36.115Z
Learning: Applies to config/*.go : Configuration must be loaded via Viper from environment variables and .env file, with at least one provider API key required
Applied to files:
config/config.go
📚 Learning: 2025-12-26T16:40:36.115Z
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T16:40:36.115Z
Learning: Applies to internal/providers/**/*.go : Provider implementations must implement the `core.Provider` interface with methods: ChatCompletion, StreamChatCompletion, ListModels, Responses, and StreamResponses
Applied to files:
internal/providers/xai/xai_test.gocmd/gomodel/main.gointernal/providers/xai/xai.go
📚 Learning: 2025-12-26T16:40:36.115Z
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T16:40:36.115Z
Learning: Applies to internal/providers/**/*.go : Streaming responses must return `io.ReadCloser` and the caller is responsible for closing the stream
Applied to files:
internal/providers/xai/xai_test.go
📚 Learning: 2025-12-26T16:40:36.115Z
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T16:40:36.115Z
Learning: Applies to cmd/gomodel/main.go : Provider packages must be imported in `cmd/gomodel/main.go` using blank imports (`_ "gomodel/internal/providers/{name}"`)
Applied to files:
cmd/gomodel/main.go
📚 Learning: 2025-12-26T16:40:36.115Z
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T16:40:36.115Z
Learning: Applies to internal/providers/*/*.go : Providers must self-register with the factory using an `init()` function that calls `providers.RegisterFactory()` with the provider name
Applied to files:
cmd/gomodel/main.gointernal/providers/xai/xai.go
📚 Learning: 2025-12-26T16:40:36.115Z
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T16:40:36.115Z
Learning: Applies to internal/providers/router.go : Use the ModelRegistry to determine which provider handles each model; return `ErrRegistryNotInitialized` if registry is used before models are loaded
Applied to files:
cmd/gomodel/main.go
📚 Learning: 2025-12-26T16:40:36.115Z
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T16:40:36.115Z
Learning: Applies to internal/providers/registry.go : Use thread-safe access with RWMutex in the ModelRegistry for concurrent access to model mappings
Applied to files:
cmd/gomodel/main.go
🧬 Code graph analysis (1)
internal/providers/xai/xai_test.go (4)
internal/providers/xai/xai.go (1)
New(30-37)internal/core/types.go (5)
ChatResponse(19-26)Model(43-48)Message(13-16)Usage(36-40)ChatRequest(4-10)internal/pkg/llmclient/client.go (1)
Request(178-183)internal/core/responses.go (2)
ResponsesResponse(30-39)ResponsesRequest(5-14)
🔇 Additional comments (16)
cmd/gomodel/main.go (1)
21-21: LGTM!The blank import for the xAI provider correctly triggers its
init()registration and follows the established pattern used by other providers. As per coding guidelines, provider packages must be imported using blank imports..env.template (1)
37-39: LGTM!The xAI provider configuration follows the established pattern of other provider entries in the template.
config/config.go (1)
141-146: LGTM!The xAI provider configuration follows the established pattern used by other providers. Configuration is correctly loaded via Viper from environment variables, as per coding guidelines.
internal/providers/xai/xai.go (5)
18-21: LGTM!The provider correctly self-registers with the factory using an
init()function, as per coding guidelines.
29-37: LGTM!The constructor properly initializes the provider with global hooks support and Bearer token authentication.
59-71: LGTM!ChatCompletion correctly implements the synchronous chat API call with proper error handling.
83-94: LGTM!ListModels correctly implements the models listing endpoint.
96-118: LGTM!Responses and StreamResponses methods correctly implement the Responses API. The same request mutation pattern as StreamChatCompletion applies to StreamResponses (Line 112).
internal/providers/xai/xai_test.go (8)
15-25: LGTM!Basic initialization test correctly verifies provider construction.
27-155: LGTM!Comprehensive table-driven tests covering success and multiple error scenarios (401, 429, 500). Request validation in the test server ensures correct headers and body structure.
157-249: LGTM!Streaming tests correctly verify that the
Streamflag is set in the request and that the response body can be read as expected.
251-344: LGTM!ListModels tests properly verify the GET method and
/modelspath, along with authorization headers.
346-371: LGTM!Context cancellation test properly verifies that the provider respects context lifecycle.
373-519: LGTM!Responses API tests are comprehensive with good coverage of success and error cases, including validation of the response structure with nested fields.
521-644: LGTM!StreamResponses tests correctly verify SSE-style streaming format and the Stream flag in requests.
646-669: LGTM!Context cancellation test for Responses API properly verifies context handling.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
internal/providers/xai/xai_test.go
🧰 Additional context used
📓 Path-based instructions (3)
internal/providers/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
internal/providers/**/*.go: Provider implementations must implement thecore.Providerinterface with methods: ChatCompletion, StreamChatCompletion, ListModels, Responses, and StreamResponses
Streaming responses must returnio.ReadCloserand the caller is responsible for closing the stream
Files:
internal/providers/xai/xai_test.go
internal/providers/*/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Providers must self-register with the factory using an
init()function that callsproviders.RegisterFactory()with the provider name
Files:
internal/providers/xai/xai_test.go
internal/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests must be located alongside implementation files using the
*_test.gonaming convention
Files:
internal/providers/xai/xai_test.go
🧠 Learnings (2)
📚 Learning: 2025-12-26T16:40:36.115Z
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T16:40:36.115Z
Learning: Applies to internal/providers/**/*.go : Provider implementations must implement the `core.Provider` interface with methods: ChatCompletion, StreamChatCompletion, ListModels, Responses, and StreamResponses
Applied to files:
internal/providers/xai/xai_test.go
📚 Learning: 2025-12-26T16:40:36.115Z
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T16:40:36.115Z
Learning: Applies to internal/providers/**/*.go : Streaming responses must return `io.ReadCloser` and the caller is responsible for closing the stream
Applied to files:
internal/providers/xai/xai_test.go
🧬 Code graph analysis (1)
internal/providers/xai/xai_test.go (4)
internal/providers/xai/xai.go (2)
New(30-37)NewWithHTTPClient(40-47)internal/pkg/llmclient/client.go (3)
Request(178-183)Response(186-189)Client(110-116)internal/core/types.go (5)
ChatRequest(4-10)Model(43-48)Message(13-16)ChatResponse(19-26)Usage(36-40)internal/core/responses.go (2)
ResponsesResponse(30-39)ResponsesRequest(5-14)
🔇 Additional comments (9)
internal/providers/xai/xai_test.go (9)
15-25: LGTM!The test correctly verifies that the
Newconstructor initializes the provider with the expected API key and client.
39-116: Excellent test coverage forNewWithHTTPClient!This test addresses the previous review comment and thoroughly verifies that a custom HTTP client is properly wired into the provider. The custom
RoundTripperapproach with header injection is an elegant way to confirm the custom client is being used for actual requests.
118-246: LGTM!The test provides comprehensive coverage of the
ChatCompletionmethod, including success path with detailed response validation and common API error scenarios (401, 429, 500). Request headers and body structure are properly verified.
342-435: LGTM!The test thoroughly validates the
ListModelsendpoint, including HTTP method (GET), correct path (/models), authorization header, and response structure with model details.
437-462: LGTM!The test correctly verifies that
ChatCompletionrespects context cancellation, ensuring the method fails appropriately when the context is cancelled.
464-610: LGTM!The test provides excellent coverage of the
Responsesmethod, including detailed validation of response structure (ID, Object, Model, Status, Output content, Usage tokens) and common API error scenarios.
612-735: LGTM!The test excellently validates the
StreamResponsesmethod with properio.ReadClosercleanup (line 642), verification that theStreamflag is set, and flexible event-based validation of the streaming response content.
737-760: LGTM!The test correctly verifies that
Responsesrespects context cancellation, ensuring proper error handling when the context is cancelled.
1-760: Comprehensive and well-structured test suite!This test file provides excellent coverage of the xAI provider implementation:
✅ All required provider interface methods tested (ChatCompletion, StreamChatCompletion, ListModels, Responses, StreamResponses)
✅ Streaming methods properly closeio.ReadCloserper coding guidelines
✅ Both success and error paths covered (401, 429, 500)
✅ Context cancellation behavior verified
✅ Custom HTTP client integration tested (addressing previous review comment)
✅ Request validation (headers, paths, bodies) and response validation (structure, tokens, content)
✅ Proper resource cleanup with defer statementsThe test suite follows Go testing best practices and aligns with the coding guidelines for provider implementations.
| func TestStreamChatCompletion(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| statusCode int | ||
| responseBody string | ||
| expectedError bool | ||
| }{ | ||
| { | ||
| name: "successful streaming request", | ||
| statusCode: http.StatusOK, | ||
| responseBody: `data: {"id":"chatcmpl-123","object":"chat.completion.chunk","created":1677652288,"model":"grok-2","choices":[{"index":0,"delta":{"content":"Hello"},"finish_reason":null}]} | ||
|
|
||
| data: {"id":"chatcmpl-123","object":"chat.completion.chunk","created":1677652288,"model":"grok-2","choices":[{"index":0,"delta":{"content":"!"},"finish_reason":null}]} | ||
|
|
||
| data: [DONE] | ||
| `, | ||
| expectedError: false, | ||
| }, | ||
| { | ||
| name: "API error", | ||
| statusCode: http.StatusUnauthorized, | ||
| responseBody: `{"error": {"message": "Invalid API key"}}`, | ||
| expectedError: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| // Verify request headers | ||
| if r.Header.Get("Content-Type") != "application/json" { | ||
| t.Errorf("Content-Type = %q, want %q", r.Header.Get("Content-Type"), "application/json") | ||
| } | ||
| authHeader := r.Header.Get("Authorization") | ||
| if !strings.HasPrefix(authHeader, "Bearer ") { | ||
| t.Errorf("Authorization header should start with 'Bearer '") | ||
| } | ||
|
|
||
| // Verify stream is set in request body | ||
| body, err := io.ReadAll(r.Body) | ||
| if err != nil { | ||
| t.Fatalf("failed to read request body: %v", err) | ||
| } | ||
| var req core.ChatRequest | ||
| if err := json.Unmarshal(body, &req); err != nil { | ||
| t.Fatalf("failed to unmarshal request: %v", err) | ||
| } | ||
| if !req.Stream { | ||
| t.Error("Stream should be true in request") | ||
| } | ||
|
|
||
| w.WriteHeader(tt.statusCode) | ||
| _, _ = w.Write([]byte(tt.responseBody)) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| provider := New("test-api-key") | ||
| provider.SetBaseURL(server.URL) | ||
|
|
||
| req := &core.ChatRequest{ | ||
| Model: "grok-2", | ||
| Messages: []core.Message{ | ||
| {Role: "user", Content: "Hello"}, | ||
| }, | ||
| } | ||
|
|
||
| body, err := provider.StreamChatCompletion(context.Background(), req) | ||
|
|
||
| if tt.expectedError { | ||
| if err == nil { | ||
| t.Error("expected error, got nil") | ||
| } | ||
| } else { | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if body == nil { | ||
| t.Fatal("body should not be nil") | ||
| } | ||
| defer func() { _ = body.Close() }() | ||
|
|
||
| // Read and verify the streaming response | ||
| respBody, err := io.ReadAll(body) | ||
| if err != nil { | ||
| t.Fatalf("failed to read response body: %v", err) | ||
| } | ||
| if string(respBody) != tt.responseBody { | ||
| t.Errorf("response body = %q, want %q", string(respBody), tt.responseBody) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM!
The test correctly verifies streaming chat completion, including proper closure of the io.ReadCloser (line 327), verification that the Stream flag is set in the request, and validation of the streaming response format.
💡 Optional: Consider event-based verification for consistency
For consistency with TestStreamResponses (lines 650-659), consider checking for specific events or patterns in the streaming response rather than exact string comparison at line 334-335. This would make the test more resilient to formatting changes.
Example:
-if string(respBody) != tt.responseBody {
- t.Errorf("response body = %q, want %q", string(respBody), tt.responseBody)
-}
+responseStr := string(respBody)
+if !strings.Contains(responseStr, "Hello") {
+ t.Error("response should contain 'Hello'")
+}
+if !strings.Contains(responseStr, "[DONE]") {
+ t.Error("response should end with [DONE]")
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestStreamChatCompletion(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| statusCode int | |
| responseBody string | |
| expectedError bool | |
| }{ | |
| { | |
| name: "successful streaming request", | |
| statusCode: http.StatusOK, | |
| responseBody: `data: {"id":"chatcmpl-123","object":"chat.completion.chunk","created":1677652288,"model":"grok-2","choices":[{"index":0,"delta":{"content":"Hello"},"finish_reason":null}]} | |
| data: {"id":"chatcmpl-123","object":"chat.completion.chunk","created":1677652288,"model":"grok-2","choices":[{"index":0,"delta":{"content":"!"},"finish_reason":null}]} | |
| data: [DONE] | |
| `, | |
| expectedError: false, | |
| }, | |
| { | |
| name: "API error", | |
| statusCode: http.StatusUnauthorized, | |
| responseBody: `{"error": {"message": "Invalid API key"}}`, | |
| expectedError: true, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| // Verify request headers | |
| if r.Header.Get("Content-Type") != "application/json" { | |
| t.Errorf("Content-Type = %q, want %q", r.Header.Get("Content-Type"), "application/json") | |
| } | |
| authHeader := r.Header.Get("Authorization") | |
| if !strings.HasPrefix(authHeader, "Bearer ") { | |
| t.Errorf("Authorization header should start with 'Bearer '") | |
| } | |
| // Verify stream is set in request body | |
| body, err := io.ReadAll(r.Body) | |
| if err != nil { | |
| t.Fatalf("failed to read request body: %v", err) | |
| } | |
| var req core.ChatRequest | |
| if err := json.Unmarshal(body, &req); err != nil { | |
| t.Fatalf("failed to unmarshal request: %v", err) | |
| } | |
| if !req.Stream { | |
| t.Error("Stream should be true in request") | |
| } | |
| w.WriteHeader(tt.statusCode) | |
| _, _ = w.Write([]byte(tt.responseBody)) | |
| })) | |
| defer server.Close() | |
| provider := New("test-api-key") | |
| provider.SetBaseURL(server.URL) | |
| req := &core.ChatRequest{ | |
| Model: "grok-2", | |
| Messages: []core.Message{ | |
| {Role: "user", Content: "Hello"}, | |
| }, | |
| } | |
| body, err := provider.StreamChatCompletion(context.Background(), req) | |
| if tt.expectedError { | |
| if err == nil { | |
| t.Error("expected error, got nil") | |
| } | |
| } else { | |
| if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| if body == nil { | |
| t.Fatal("body should not be nil") | |
| } | |
| defer func() { _ = body.Close() }() | |
| // Read and verify the streaming response | |
| respBody, err := io.ReadAll(body) | |
| if err != nil { | |
| t.Fatalf("failed to read response body: %v", err) | |
| } | |
| if string(respBody) != tt.responseBody { | |
| t.Errorf("response body = %q, want %q", string(respBody), tt.responseBody) | |
| } | |
| } | |
| }) | |
| } | |
| } | |
| func TestStreamChatCompletion(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| statusCode int | |
| responseBody string | |
| expectedError bool | |
| }{ | |
| { | |
| name: "successful streaming request", | |
| statusCode: http.StatusOK, | |
| responseBody: `data: {"id":"chatcmpl-123","object":"chat.completion.chunk","created":1677652288,"model":"grok-2","choices":[{"index":0,"delta":{"content":"Hello"},"finish_reason":null}]} | |
| data: {"id":"chatcmpl-123","object":"chat.completion.chunk","created":1677652288,"model":"grok-2","choices":[{"index":0,"delta":{"content":"!"},"finish_reason":null}]} | |
| data: [DONE] | |
| `, | |
| expectedError: false, | |
| }, | |
| { | |
| name: "API error", | |
| statusCode: http.StatusUnauthorized, | |
| responseBody: `{"error": {"message": "Invalid API key"}}`, | |
| expectedError: true, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| // Verify request headers | |
| if r.Header.Get("Content-Type") != "application/json" { | |
| t.Errorf("Content-Type = %q, want %q", r.Header.Get("Content-Type"), "application/json") | |
| } | |
| authHeader := r.Header.Get("Authorization") | |
| if !strings.HasPrefix(authHeader, "Bearer ") { | |
| t.Errorf("Authorization header should start with 'Bearer '") | |
| } | |
| // Verify stream is set in request body | |
| body, err := io.ReadAll(r.Body) | |
| if err != nil { | |
| t.Fatalf("failed to read request body: %v", err) | |
| } | |
| var req core.ChatRequest | |
| if err := json.Unmarshal(body, &req); err != nil { | |
| t.Fatalf("failed to unmarshal request: %v", err) | |
| } | |
| if !req.Stream { | |
| t.Error("Stream should be true in request") | |
| } | |
| w.WriteHeader(tt.statusCode) | |
| _, _ = w.Write([]byte(tt.responseBody)) | |
| })) | |
| defer server.Close() | |
| provider := New("test-api-key") | |
| provider.SetBaseURL(server.URL) | |
| req := &core.ChatRequest{ | |
| Model: "grok-2", | |
| Messages: []core.Message{ | |
| {Role: "user", Content: "Hello"}, | |
| }, | |
| } | |
| body, err := provider.StreamChatCompletion(context.Background(), req) | |
| if tt.expectedError { | |
| if err == nil { | |
| t.Error("expected error, got nil") | |
| } | |
| } else { | |
| if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| if body == nil { | |
| t.Fatal("body should not be nil") | |
| } | |
| defer func() { _ = body.Close() }() | |
| // Read and verify the streaming response | |
| respBody, err := io.ReadAll(body) | |
| if err != nil { | |
| t.Fatalf("failed to read response body: %v", err) | |
| } | |
| responseStr := string(respBody) | |
| if !strings.Contains(responseStr, "Hello") { | |
| t.Error("response should contain 'Hello'") | |
| } | |
| if !strings.Contains(responseStr, "[DONE]") { | |
| t.Error("response should end with [DONE]") | |
| } | |
| } | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/providers/xai/xai_test.go around lines 248 to 340, replace the
brittle exact string comparison of the streamed response (lines ~334-335) with
event-based verification: read the response stream, split or scan for SSE-like
"data:" events (ignoring blank lines), collect the event payloads in order and
assert they match the expected sequence (e.g., first chunk "Hello", second chunk
"!", then a "[DONE]" marker) — mirror the approach used in TestStreamResponses
(lines ~650-659) so the test is resilient to formatting/whitespace changes.
There was a problem hiding this comment.
Pull request overview
This PR adds support for xAI (Grok) as a new AI provider to the gomodel gateway. The implementation follows the existing provider patterns and includes comprehensive test coverage. Additionally, the PR refactors streaming request handling across all providers by introducing WithStreaming() helper methods to avoid request object mutation.
Key Changes
- Implements xAI provider with support for chat completions, streaming, model listing, and Responses API
- Adds
WithStreaming()helper methods toChatRequestandResponsesRequesttypes to create shallow copies with streaming enabled - Updates OpenAI and Gemini providers to use the new
WithStreaming()pattern instead of directly mutating request objects
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/providers/xai/xai.go |
New xAI provider implementation following the standard provider interface pattern |
internal/providers/xai/xai_test.go |
Comprehensive unit tests covering all xAI provider methods and error scenarios |
internal/providers/openai/openai.go |
Refactored to use WithStreaming() helper instead of direct mutation |
internal/providers/gemini/gemini.go |
Refactored to use WithStreaming() helper instead of direct mutation |
internal/core/types.go |
Added WithStreaming() helper method to ChatRequest |
internal/core/responses.go |
Added WithStreaming() helper method to ResponsesRequest |
config/config.yaml |
Added xai-primary provider configuration |
config/config.go |
Added XAI_API_KEY environment variable support |
cmd/gomodel/main.go |
Added import for xai provider to enable auto-registration |
.env.template |
Added XAI_API_KEY placeholder for configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Responses sends a Responses API request to xAI | ||
| func (p *Provider) Responses(ctx context.Context, req *core.ResponsesRequest) (*core.ResponsesResponse, error) { | ||
| var resp core.ResponsesResponse | ||
| err := p.client.Do(ctx, llmclient.Request{ | ||
| Method: http.MethodPost, | ||
| Endpoint: "/responses", | ||
| Body: req, | ||
| }, &resp) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &resp, nil | ||
| } |
There was a problem hiding this comment.
The xAI (Grok) API may not support the /responses endpoint. Based on xAI's API documentation (as of January 2025), the Grok API is OpenAI-compatible primarily for the /chat/completions endpoint. The /responses endpoint is part of OpenAI's newer Assistants/Responses API, which xAI might not have implemented.
Consider one of the following approaches:
- Verify that xAI actually supports this endpoint through their official documentation or API testing
- If xAI doesn't support this endpoint, consider converting Responses requests to Chat Completion requests (similar to how Gemini and Anthropic providers handle it - see
internal/providers/gemini/gemini.golines 287-298 andinternal/providers/anthropic/anthropic.golines 533-547) - If this endpoint is not supported, document this limitation clearly
The same concern applies to the StreamResponses method on lines 109-116.
| func TestResponses(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| statusCode int | ||
| responseBody string | ||
| expectedError bool | ||
| checkResponse func(*testing.T, *core.ResponsesResponse) | ||
| }{ | ||
| { | ||
| name: "successful request with string input", | ||
| statusCode: http.StatusOK, | ||
| responseBody: `{ | ||
| "id": "resp_123", | ||
| "object": "response", | ||
| "created_at": 1677652288, | ||
| "model": "grok-2", | ||
| "status": "completed", | ||
| "output": [{ | ||
| "id": "msg_123", | ||
| "type": "message", | ||
| "role": "assistant", | ||
| "status": "completed", | ||
| "content": [{ | ||
| "type": "output_text", | ||
| "text": "Hello! How can I help you today?" | ||
| }] | ||
| }], | ||
| "usage": { | ||
| "input_tokens": 10, | ||
| "output_tokens": 20, | ||
| "total_tokens": 30 | ||
| } | ||
| }`, | ||
| expectedError: false, | ||
| checkResponse: func(t *testing.T, resp *core.ResponsesResponse) { | ||
| if resp.ID != "resp_123" { | ||
| t.Errorf("ID = %q, want %q", resp.ID, "resp_123") | ||
| } | ||
| if resp.Object != "response" { | ||
| t.Errorf("Object = %q, want %q", resp.Object, "response") | ||
| } | ||
| if resp.Model != "grok-2" { | ||
| t.Errorf("Model = %q, want %q", resp.Model, "grok-2") | ||
| } | ||
| if resp.Status != "completed" { | ||
| t.Errorf("Status = %q, want %q", resp.Status, "completed") | ||
| } | ||
| if len(resp.Output) != 1 { | ||
| t.Fatalf("len(Output) = %d, want 1", len(resp.Output)) | ||
| } | ||
| if len(resp.Output[0].Content) != 1 { | ||
| t.Fatalf("len(Output[0].Content) = %d, want 1", len(resp.Output[0].Content)) | ||
| } | ||
| if resp.Output[0].Content[0].Text != "Hello! How can I help you today?" { | ||
| t.Errorf("Output text = %q, want %q", resp.Output[0].Content[0].Text, "Hello! How can I help you today?") | ||
| } | ||
| if resp.Usage == nil { | ||
| t.Fatal("Usage should not be nil") | ||
| } | ||
| if resp.Usage.InputTokens != 10 { | ||
| t.Errorf("InputTokens = %d, want 10", resp.Usage.InputTokens) | ||
| } | ||
| if resp.Usage.OutputTokens != 20 { | ||
| t.Errorf("OutputTokens = %d, want 20", resp.Usage.OutputTokens) | ||
| } | ||
| if resp.Usage.TotalTokens != 30 { | ||
| t.Errorf("TotalTokens = %d, want 30", resp.Usage.TotalTokens) | ||
| } | ||
| }, | ||
| }, | ||
| { | ||
| name: "API error - unauthorized", | ||
| statusCode: http.StatusUnauthorized, | ||
| responseBody: `{"error": {"message": "Invalid API key"}}`, | ||
| expectedError: true, | ||
| }, | ||
| { | ||
| name: "rate limit error", | ||
| statusCode: http.StatusTooManyRequests, | ||
| responseBody: `{"error": {"message": "Rate limit exceeded"}}`, | ||
| expectedError: true, | ||
| }, | ||
| { | ||
| name: "server error", | ||
| statusCode: http.StatusInternalServerError, | ||
| responseBody: `{"error": {"message": "Internal server error"}}`, | ||
| expectedError: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| // Verify request headers | ||
| if r.Header.Get("Content-Type") != "application/json" { | ||
| t.Errorf("Content-Type = %q, want %q", r.Header.Get("Content-Type"), "application/json") | ||
| } | ||
| authHeader := r.Header.Get("Authorization") | ||
| if !strings.HasPrefix(authHeader, "Bearer ") { | ||
| t.Errorf("Authorization header should start with 'Bearer '") | ||
| } | ||
|
|
||
| // Verify request path | ||
| if r.URL.Path != "/responses" { | ||
| t.Errorf("Path = %q, want %q", r.URL.Path, "/responses") | ||
| } | ||
|
|
||
| // Verify request body | ||
| body, err := io.ReadAll(r.Body) | ||
| if err != nil { | ||
| t.Fatalf("failed to read request body: %v", err) | ||
| } | ||
| var req core.ResponsesRequest | ||
| if err := json.Unmarshal(body, &req); err != nil { | ||
| t.Fatalf("failed to unmarshal request: %v", err) | ||
| } | ||
|
|
||
| w.WriteHeader(tt.statusCode) | ||
| _, _ = w.Write([]byte(tt.responseBody)) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| provider := New("test-api-key") | ||
| provider.SetBaseURL(server.URL) | ||
|
|
||
| req := &core.ResponsesRequest{ | ||
| Model: "grok-2", | ||
| Input: "Hello", | ||
| } | ||
|
|
||
| resp, err := provider.Responses(context.Background(), req) | ||
|
|
||
| if tt.expectedError { | ||
| if err == nil { | ||
| t.Error("expected error, got nil") | ||
| } | ||
| } else { | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if tt.checkResponse != nil { | ||
| tt.checkResponse(t, resp) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The tests for the Responses API endpoint assume that xAI supports this endpoint without verifying if it actually does. If xAI doesn't support the /responses endpoint natively, these tests will pass against the mock server but fail against the real xAI API.
Consider adding integration tests or documenting the verification that xAI supports this endpoint. Alternatively, if xAI doesn't support this endpoint, the implementation should convert Responses requests to Chat Completion requests like other providers do (see Gemini and Anthropic implementations).
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.claude/settings.local.jsonEXAMPLES.md
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ENTERPILOT/GOModel PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T16:40:36.115Z
Learning: Applies to internal/providers/**/*.go : Provider implementations must implement the `core.Provider` interface with methods: ChatCompletion, StreamChatCompletion, ListModels, Responses, and StreamResponses
🔇 Additional comments (7)
.claude/settings.local.json (1)
7-8: LGTM! Appropriate permissions for xAI integration.The additions correctly grant permissions for test execution and fetching xAI documentation. The WebFetch permission is properly scoped to the specific docs.x.ai domain, which follows security best practices.
EXAMPLES.md (6)
14-14: LGTM! Consistent environment variable addition.The XAI_API_KEY environment variable follows the same pattern as other provider keys, making it easy for users to configure.
191-257: LGTM! Comprehensive xAI examples with Responses API.The xAI examples section is well-structured and covers multiple use cases (basic requests, instructions, structured input, streaming). The use of the
/v1/responsesendpoint is appropriate and aligns with the documentation note about xAI's API conversion.
334-342: LGTM! Consistent Python client example.The xAI example correctly demonstrates using the Grok model through the OpenAI-compatible interface, maintaining consistency with other provider examples.
386-392: LGTM! Consistent Node.js client example.The xAI example properly demonstrates using the Grok model with the OpenAI client library, following the established pattern for other providers.
447-447: LGTM! Clear model prefix mapping.The addition of the
grok-prefix mapping helps users understand how the gateway routes requests to the xAI provider.
470-470: LGTM! Important clarification about Responses API.This note helpfully explains that the
/v1/responsesendpoint is OpenAI-compatible and that xAI internally converts these requests to chat completion format, setting proper expectations for users.
EXAMPLES.md
Outdated
| - `grok-4-1-fast-non-reasoning` - Most capable Grok model (best for complex reasoning) | ||
| - `grok-4-1-fast-non-reasoning` - Faster, cost-effective Grok model |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd / && find . -name "EXAMPLES.md" -type f 2>/dev/null | head -5Repository: ENTERPILOT/GOModel
Length of output: 93
🏁 Script executed:
if [ -f "EXAMPLES.md" ]; then
sed -n '435,440p' EXAMPLES.md
fiRepository: ENTERPILOT/GOModel
Length of output: 234
🌐 Web query:
xAI Grok model names API documentation latest available models 2024 2025
💡 Result:
Here are the Grok model names / IDs that appear in xAI’s public docs and recent announcements (2024–2025):
- Grok‑2 family: grok-2, grok-2-latest, grok-2-1212, grok-2-vision, grok-2-vision-1212. [1][2]
- Grok‑3 family (beta): grok-3, grok-3-beta, grok-3-mini, grok-3-mini-beta. [3][4]
- Grok‑4 family / code variants (2025 listings): grok-4-fast-reasoning (aka grok-4-fast), grok-4-fast-non-reasoning, grok-4-1-fast-reasoning, grok-code-fast-1 (and aliases like grok-code-fast). [4][5]
- Other/legacy: grok-beta, grok-vision-beta, grok-2-mini / grok-2 mini (announced 2024). [1][2][5]
Sources / docs: xAI announcement pages and xAI API/docs mirrors (model lists), plus recent provider/docs summaries. See: x.ai Grok-2 announcement, Grok-3 announcement, xAI docs (models/overview), and 3rd‑party provider docs. [1][3][2][4]
If you want, I can fetch the exact API model IDs and example request snippets from xAI’s API docs (copyable), or verify which of these are available in the public API right now.
🏁 Script executed:
sed -n '240,250p' EXAMPLES.mdRepository: ENTERPILOT/GOModel
Length of output: 312
Fix duplicate model name in xAI models listing.
Lines 437–438 both list grok-4-1-fast-non-reasoning, but the second should be grok-2-mini to match the description "Faster, cost-effective Grok model" and align with the example code at line 246.
- `grok-4-1-fast-non-reasoning` - Most capable Grok model (best for complex reasoning)
-- `grok-4-1-fast-non-reasoning` - Faster, cost-effective Grok model
+- `grok-2-mini` - Faster, cost-effective Grok model📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `grok-4-1-fast-non-reasoning` - Most capable Grok model (best for complex reasoning) | |
| - `grok-4-1-fast-non-reasoning` - Faster, cost-effective Grok model | |
| - `grok-4-1-fast-non-reasoning` - Most capable Grok model (best for complex reasoning) | |
| - `grok-2-mini` - Faster, cost-effective Grok model |
🤖 Prompt for AI Agents
In EXAMPLES.md around lines 437 to 438, the xAI models listing accidentally
repeats `grok-4-1-fast-non-reasoning`; replace the second entry with
`grok-2-mini` so the lines read the intended pair ("Most capable Grok model" and
"Faster, cost-effective Grok model") and align with the example code referenced
at line 246.
| - Gemini 1.5 Pro offers an exceptionally large context window (up to 1M tokens), making it ideal for long-form content analysis. | ||
| - OpenAI GPT-4 models typically support 8K-128K tokens depending on the variant. | ||
| - Anthropic Claude models support up to 200K tokens. | ||
| - xAI Grok models support up to 128K tokens. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the content around line 468 in EXAMPLES.md
sed -n '465,475p' EXAMPLES.mdRepository: ENTERPILOT/GOModel
Length of output: 635
🌐 Web query:
xAI Grok models context window token limit specifications
💡 Result:
Summary of published/commonly reported Grok context-window specs:
- Grok-1 / early Grok variants — reported ~32K tokens (earlier smaller variants also cited ~8K in some third-party reports). (datastudios.org)
- Grok‑1.5 — widely reported / marketed with a 128,000‑token context window. (ai-x.chat)
- Grok‑3 / Grok‑4 (later generations) — third‑party reports indicate deployments with much larger windows (examples: 128K–256K in service; engineering capability claims up to ~1M tokens for research/experimental modes), and Grok 4 has been reported at 256K in coverage. (twitter.com)
- xAI official docs — note tokenization differences between models, image prompt token ranges (256–1,792 tokens), and that image+text must fit within a model’s overall context window; xAI does not publish a single universal token limit per model on that page. (docs.x.ai)
Notes: public numbers vary across xAI posts, X (Twitter) threads, and news/blog coverage; xAI’s documentation emphasizes tokenizer differences and runtime-added tokens, so effective usable tokens may be slightly lower than nominal window sizes. For exact, current per‑model limits (including any paid/serving restrictions) I can fetch the latest xAI docs or X announcements — would you like me to do that now?
Citations:
- 1: https://www.datastudios.org/post/grok-ai-context-window-token-limits-and-memory-architecture-performance-and-retention-behavior?utm_source=openai
- 2: https://ai-x.chat/models/grok1-5/?utm_source=openai
- 3: https://twitter.com/i/grok/share/yVUosQhsQuHlOmvFxE2VLSNAM?utm_source=openai
- 4: https://docs.x.ai/docs/key-information/consumption-and-rate-limits?utm_source=openai
Clarify which xAI Grok model version and context window limit.
The statement "xAI Grok models support up to 128K tokens" is vague and potentially outdated. The 128K limit applies to Grok-1.5, but newer models (Grok-3, Grok-4) support larger context windows (256K or more). Either specify the model version (e.g., "xAI Grok-1.5 supports up to 128K tokens") or note that context windows vary by model version.
🤖 Prompt for AI Agents
In EXAMPLES.md around line 468, the statement "xAI Grok models support up to
128K tokens" is ambiguous and may be outdated; update the text to specify the
model version(s) and their context window limits (for example, state that "xAI
Grok-1.5 supports up to 128K tokens" and note newer models like Grok-3/Grok-4
support larger windows such as 256K or more), or replace the assertion with a
brief note that context window sizes vary by Grok model version and to consult
xAI docs for exact limits.
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.